On 2024/01/25 19:13:08 +0000, James Cook <falsifian@falsifian.org> wrote:
> > > 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.
> 
> Yes, I think gotd would want to be able to interact with git-annex 
> repos. Git-annex repos are also regular git repos, and are synchronized 
> using git fetch or push (or perhaps more low-level equivalent git 
> commands). So it's quite plausible someone would want to access 
> their git-annex repo via gotd. If I understand right, gotd would 
> fail to open the repo if git-annex had added one of the strange 
> remotes to the config file.
> 
> If you would like to update the gotd code in a similar way it would 
> be helpful since I don't yet use gotd. Alternatively, I wonder if 
> there's room to refactor so that the two implementations share more 
> code. I am tempted to take a look but don't know if I'll find time.

Well, this is the diff for gotd.  I don't really know however where to
add the test case for the missing url in gotd regress.

diff /home/op/w/got
commit - b624328edd13c021ae254c221e3ebbfebb49ff6a
path + /home/op/w/got
blob - 0a66f6c60ab7ddde4e975a741714b22ada064ea2
file + lib/read_gitconfig.c
--- lib/read_gitconfig.c
+++ lib/read_gitconfig.c
@@ -51,6 +51,18 @@ get_boolean_val(char *val)
 	strcmp(val, "1") == 0);
 }
 
+static int
+skip_node(struct got_gitconfig *gitconfig,
+    struct got_gitconfig_list_node *node)
+{
+	/*
+	 * Skip config nodes which do not describe remotes, and remotes
+	 * which do not have a fetch URL defined (as used by git-annex).
+	 */
+	return (strncasecmp("remote \"", node->field, 8) != 0 ||
+	    got_gitconfig_get_str(gitconfig, node->field, "url") == NULL);
+}
+
 const struct got_error *
 got_repo_read_gitconfig(int *gitconfig_repository_format_version,
     char **gitconfig_author_name, char **gitconfig_author_email,
@@ -179,7 +191,7 @@ got_repo_read_gitconfig(int *gitconfig_repository_form
 		if (err)
 			return err;
 		TAILQ_FOREACH(node, &sections->fields, link) {
-			if (strncasecmp("remote \"", node->field, 8) != 0)
+			if (skip_node(gitconfig, node))
 				continue;
 			nalloc++;
 		}
@@ -196,7 +208,7 @@ got_repo_read_gitconfig(int *gitconfig_repository_form
 			char *name, *end, *mirror;
 			const char *fetch_url, *send_url;
 
-			if (strncasecmp("remote \"", node->field, 8) != 0)
+			if (skip_node(gitconfig, node) != 0)
 				continue;
 
 			remote = &(*remotes)[i];
@@ -213,12 +225,6 @@ got_repo_read_gitconfig(int *gitconfig_repository_form
 
 			fetch_url = got_gitconfig_get_str(gitconfig,
 			    node->field, "url");
-			if (fetch_url == NULL) {
-				err = got_error(GOT_ERR_GITCONFIG_SYNTAX);
-				free(remote->name);
-				remote->name = NULL;
-				goto done;
-			}
 			remote->fetch_url = strdup(fetch_url);
 			if (remote->fetch_url == NULL) {
 				err = got_error_from_errno("strdup");
@@ -232,14 +238,6 @@ got_repo_read_gitconfig(int *gitconfig_repository_form
 			if (send_url == NULL)
 				send_url = got_gitconfig_get_str(gitconfig,
 				    node->field, "url");
-			if (send_url == NULL) {
-				err = got_error(GOT_ERR_GITCONFIG_SYNTAX);
-				free(remote->name);
-				remote->name = NULL;
-				free(remote->fetch_url);
-				remote->fetch_url = NULL;
-				goto done;
-			}
 			remote->send_url = strdup(send_url);
 			if (remote->send_url == NULL) {
 				err = got_error_from_errno("strdup");