Download raw body.
allow remotes without urls in git config; add tests
Stefan Sperling <stsp@stsp.name> writes:
> On Wed, Jan 24, 2024 at 07:54:23PM +0100, Omar Polo wrote:
>> On 2024/01/24 18:16:02 +0000, James Cook <falsifian@falsifian.org> wrote:
>> > I have some git repos where .git/config has "remote" sections without
>> > urls, like this:
>> >
>> > [remote "h0-rsync"]
>> > annex-rsyncurl = (some url)
>> > annex-uuid = (some uuid)
>> > skipFetchAll = true
>> >
>> > Running tog and many got commands there results in:
>> >
>> > $ tog
>> > got-read-gitconfig: gitconfig syntax error
>> > tog: gitconfig syntax error
>> >
>> > The below diff makes got-read-gitconfig instead ignore these sections,
>> > and adds test coverage for got fetch and got send commands that
>> > read the git config.
>>
>> The idea sounds fine to me.
>>
>> > I didn't touch the implementation in lib/read_gitconfig.c because
>> > that seems to be only used by gotd, which I don't (yet) use and so
>> > couldn't easily test. Is that duplicate implementation still needed?
>> > I actually made my change there first and it took me a while to
>> > realize it was the wrong place.
>>
>> I think this is due to the fact that gotd doesn't use the libexec
>> helpers, so you end up with two "interfaces": one that runs
>> got-read-gitconfig under the hood (lib/read_gitconfig_privsep.c) and one
>> that doesn't (lib/read_gitconfig.c).
>>
>> (i've been bitten by this separation in the past too, but there are more
>> files like that in lib/ which do the same thing.)
>>
>> I think however that if we change the behaviour of got-read-gitconfig we
>> should keep the same in lib/read_gitconfig. I can tweak the latter in a
>> follow-up if you prefer though.
>>
>> I never used git-annex and I don't know how much a server should care,
>> but I think that if got(1) can work on a repository, then gotd should be
>> able to do it too.
>>
>> > diff d0980f09b20ac878dcbc62fb940adf273a363f00 b9782d5c1f3fc2459e8cde0b58a1e128e43a2149
>> > commit - d0980f09b20ac878dcbc62fb940adf273a363f00
>> > commit + b9782d5c1f3fc2459e8cde0b58a1e128e43a2149
>> > blob - 99f2a5a5d4fdc582151efa06eb679bf4bfd09a69
>> > blob + d3e9edfcd3e76bd3faa07932fbdf44acbdebed5c
>> > --- libexec/got-read-gitconfig/got-read-gitconfig.c
>> > +++ libexec/got-read-gitconfig/got-read-gitconfig.c
>> > @@ -248,8 +248,10 @@ gitconfig_remotes_request(struct imsgbuf *ibuf, struct
>> > remotes[i].fetch_url = got_gitconfig_get_str(gitconfig,
>> > node->field, "url");
>> > if (remotes[i].fetch_url == NULL) {
>> > - err = got_error(GOT_ERR_GITCONFIG_SYNTAX);
>> > - goto done;
>> > + free(remotes[i].name);
>> > + remotes[i].name = NULL;
>> > + --nremotes;
>> > + continue;
>>
>> If there is only one remote section and it doesn't have a url, nremotes
>> becomes zero. This is not a problem, as that's normally handled,
>> however i wonder if having just one loop and resizing nremotes
>> on-the-fly wouldn't be simpler than this dance. It would be a bigger
>> change though.
>
> This is essentially the same change as James proposed, just in a
> different style to avoid having to reshuffle counters and freeing
> memory that wasn't used.
> I don't want to steal your commit James, so I first tried to phrase
> this change in prose. But I ended up deleting what I had written since
> it seemed much easier to just write the diff.
>
> I'm a bit short on time so I only compiled-tested this.
> Does Jame's test still pass with this?
With your modification to got-read-gitconfig.c and his new regressions,
your diff throws 2 failed regressions because of "privsep peer process
closed pipe". From me playing around with it to get it pass regressions
we need James's modificaion of
>
> diff /home/stsp/src/got
> commit - d0980f09b20ac878dcbc62fb940adf273a363f00
> path + /home/stsp/src/got
> blob - 99f2a5a5d4fdc582151efa06eb679bf4bfd09a69
> file + libexec/got-read-gitconfig/got-read-gitconfig.c
> --- libexec/got-read-gitconfig/got-read-gitconfig.c
> +++ libexec/got-read-gitconfig/got-read-gitconfig.c
> @@ -230,10 +230,15 @@ gitconfig_remotes_request(struct imsgbuf *ibuf, struct
>
> i = 0;
> TAILQ_FOREACH(node, §ions->fields, link) {
> - char *name, *end, *mirror;
> + char *name, *end, *fetch_url, *mirror;
>
> if (strncasecmp("remote \"", node->field, 8) != 0)
> continue;
> +
> + fetch_url = got_gitconfig_get_str(gitconfig,
> + node->field, "url");
> + if (fetch_url == NULL)
--nremotes;
> + continue;
>
> name = strdup(node->field + 8);
> if (name == NULL) {
> @@ -245,22 +250,12 @@ gitconfig_remotes_request(struct imsgbuf *ibuf, struct
> *end = '\0';
> remotes[i].name = name;
>
> - remotes[i].fetch_url = got_gitconfig_get_str(gitconfig,
> - node->field, "url");
> - if (remotes[i].fetch_url == NULL) {
> - err = got_error(GOT_ERR_GITCONFIG_SYNTAX);
> - goto done;
> - }
> + remotes[i].fetch_url = fetch_url;
>
> remotes[i].send_url = got_gitconfig_get_str(gitconfig,
> node->field, "pushurl");
> if (remotes[i].send_url == NULL)
> - remotes[i].send_url = got_gitconfig_get_str(gitconfig,
> - node->field, "url");
> - if (remotes[i].send_url == NULL) {
> - err = got_error(GOT_ERR_GITCONFIG_SYNTAX);
> - goto done;
> - }
> + remotes[i].send_url = fetch_url;
>
> remotes[i].mirror_references = 0;
> mirror = got_gitconfig_get_str(gitconfig, node->field,
to get it to pass all the regressions. This is because on the remote
we are ignoring, send_gitconfig_remotes will still try and read it.
There are 3 strlen functions that throw segfaults because they are
passed in NULL (as the fields of the remote were never populated and
remain NULL). The only edge case I can think of this not working is if
git-annex puts an actual remote section with a url after a remote that
references an "annex" or the remote we are patching to ignore.
allow remotes without urls in git config; add tests