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?

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, &sections->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)
+			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,