From: Omar Polo <op@omarpolo.com>
Subject: Re: allow remotes without urls in git config; add tests
To: James Cook <falsifian@falsifian.org>
Cc: gameoftrees@openbsd.org
Date: Wed, 24 Jan 2024 19:54:23 +0100

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.

I'm ok with this going in as-is.