[PATCH] remote: fix "update [group...]"

[PATCH] remote: fix "update [group...]"

[Johannes Schindelin]

The rewrite in C inadvertently broke updating with remote groups: when you
pass parameters to "git remote update", it used to look up "remotes.<group>"
for every parameter, and interpret the value as a list of remotes to update.

Also, no parameter, or a single parameter "default" should update all
remotes that have not been marked with "skipDefaultUpdate".

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---

	I only realised the breakage today, sorry.

 builtin-remote.c  |   53 ++++++++++++++++++++++++++++++++------
 t/t5505-remote.sh |   72 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 116 insertions(+), 9 deletions(-)

diff --git a/builtin-remote.c b/builtin-remote.c
index fc01bcc..4d2ca16 100644
--- a/builtin-remote.c
+++ b/builtin-remote.c
@@ -477,24 +477,59 @@ cleanup_states:
 	return result;
 }
 
-static int update_one(struct remote *remote, void *priv)
+static int get_one_remote_for_update(struct remote *remote, void *priv)
 {
+	struct path_list *list = priv;
 	if (!remote->skip_default_update)
-		return fetch_remote(remote->name);
+		path_list_append(xstrdup(remote->name), list);
+	return 0;
+}
+
+struct remote_group {
+	const char *name;
+	struct path_list *list;
+} remote_group;
+
+static int get_remote_group(const char *key, const char *value)
+{
+	if (!prefixcmp(key, "remotes.") &&
+			!strcmp(key + 8, remote_group.name)) {
+		char *space = strchr(value, ' ');
+		while (space) {
+			path_list_append(xstrndup(value, space - value),
+					remote_group.list);
+			value = space + 1;
+			space = strchr(value, ' ');
+		}
+		path_list_append(xstrdup(value), remote_group.list);
+	}
+
 	return 0;
 }
 
 static int update(int argc, const char **argv)
 {
-	int i;
+	int i, result = 0;
+	struct path_list list = { NULL, 0, 0, 0 };
 
-	if (argc < 2)
-		return for_each_remote(update_one, NULL);
+	if (argc < 2 || (argc == 2 && !strcmp(argv[1], "default")))
+		result = for_each_remote(get_one_remote_for_update, &list);
+	else {
+		remote_group.list = &list;
+		for (i = 1; i < argc; i++) {
+			remote_group.name = argv[i];
+			result = git_config(get_remote_group);
+		}
+	}
+	if (result)
+		return result;
 
-	for (i = 1; i < argc; i++)
-		if (fetch_remote(argv[i]))
-			return 1;
-	return 0;
+	for (i = 0; i < list.nr; i++)
+		result |= fetch_remote(list.items[i].path);
+	list.strdup_paths = 1;
+	path_list_clear(&list, 0);
+
+	return result;
 }
 
 static int get_one_entry(struct remote *remote, void *priv)
diff --git a/t/t5505-remote.sh b/t/t5505-remote.sh
index 0a25c8b..b9757c0 100755
--- a/t/t5505-remote.sh
+++ b/t/t5505-remote.sh
@@ -10,10 +10,12 @@ setup_repository () {
 	git init &&
 	>file &&
 	git add file &&
+	test_tick &&
 	git commit -m "Initial" &&
 	git checkout -b side &&
 	>elif &&
 	git add elif &&
+	test_tick &&
 	git commit -m "Second" &&
 	git checkout master
 	)
@@ -113,6 +115,7 @@ test_expect_success 'show' '
 	 git branch -d -r origin/master &&
 	 (cd ../one &&
 	  echo 1 > file &&
+	  test_tick &&
 	  git commit -m update file) &&
 	 git remote show origin > output &&
 	 git diff expect output)
@@ -144,4 +147,73 @@ test_expect_success 'add --mirror && prune' '
 	 git rev-parse --verify refs/heads/side)
 '
 
+cat > one/expect << EOF
+  apis/master
+  apis/side
+  drosophila/another
+  drosophila/master
+  drosophila/side
+EOF
+
+test_expect_success 'update' '
+
+	(cd one &&
+	 git remote add drosophila ../two &&
+	 git remote add apis ../mirror &&
+	 git remote update &&
+	 git branch -r > output &&
+	 git diff expect output)
+
+'
+
+cat > one/expect << EOF
+  drosophila/another
+  drosophila/master
+  drosophila/side
+  manduca/master
+  manduca/side
+  megaloprepus/master
+  megaloprepus/side
+EOF
+
+test_expect_success 'update with arguments' '
+
+	(cd one &&
+	 for b in $(git branch -r)
+	 do
+		git branch -r -d $b || break
+	 done &&
+	 git remote add manduca ../mirror &&
+	 git remote add megaloprepus ../mirror &&
+	 git config remotes.phobaeticus "drosophila megaloprepus" &&
+	 git config remotes.titanus manduca &&
+	 git remote update phobaeticus titanus &&
+	 git branch -r > output &&
+	 git diff expect output)
+
+'
+
+cat > one/expect << EOF
+  apis/master
+  apis/side
+  manduca/master
+  manduca/side
+  megaloprepus/master
+  megaloprepus/side
+EOF
+
+test_expect_success 'update default' '
+
+	(cd one &&
+	 for b in $(git branch -r)
+	 do
+		git branch -r -d $b || break
+	 done &&
+	 git config remote.drosophila.skipDefaultUpdate true &&
+	 git remote update default &&
+	 git branch -r > output &&
+	 git diff expect output)
+
+'
+
 test_done
-- 
1.5.4.3.542.g98d7c

Re: [PATCH] remote: fix "update [group...]"

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> The rewrite in C inadvertently broke updating with remote groups: when you
> pass parameters to "git remote update", it used to look up "remotes.<group>"
> for every parameter, and interpret the value as a list of remotes to update.
>
> Also, no parameter, or a single parameter "default" should update all
> remotes that have not been marked with "skipDefaultUpdate".

My reading of "sub update_remote {}" seems to indicate there still are
some differences.  What the scripted version did was:

   - Without extra parameter, update_remote() is run with "default";
   - With parameters, update_remote() is run for each of them;

  Then inside update_remote():

   - grab "remotes.$name" configuration; if exists, then it is split using
     "split(' ')", which is different from split(/ /).

     Compare:
          my @a = split(/ /, " A quick\t brown fox  ");
          my @b = split(' ', " A quick\t brown fox  ");
	  print "<" . join("><", @a), ">\n";
	  print "<" . join("><", @b), ">\n";
     The latter returns ('A', 'quick', 'brown', 'fox').

   - If no "remotes.$name" configuration exists, and if the name is
     "default", then grab all remotes that do not have
     remote.$foo.skipDefaultUpdate set to true.

   - Otherwise complain and die.

  Then fetch each of them.

> +static int get_one_remote_for_update(struct remote *remote, void *priv)
>  {
> +	struct path_list *list = priv;
>  	if (!remote->skip_default_update)
> +		path_list_append(xstrdup(remote->name), list);
> +	return 0;
> +}

This is called when no extra parameter is given or the name is default.
It behaves differently from the scripted version if remote.default
configuration exists, doesn't it?

I suspect this is a regression to a useful feature.  The user can specify
an explicit list of remotes to fetch from when he says "update default" or
just "update", without having to say skipdefaultupdate for all the usually
uninteresting remotes.

> +struct remote_group {
> +	const char *name;
> +	struct path_list *list;
> +} remote_group;
> +
> +static int get_remote_group(const char *key, const char *value)
> +{
> +	if (!prefixcmp(key, "remotes.") &&
> +			!strcmp(key + 8, remote_group.name)) {
> +		char *space = strchr(value, ' ');
> +		while (space) {
> +			path_list_append(xstrndup(value, space - value),
> +					remote_group.list);
> +			value = space + 1;
> +			space = strchr(value, ' ');
> +		}
> +		path_list_append(xstrdup(value), remote_group.list);
> +	}
> +

This does not mimick the magic "split(' ', $conf)" in the scripted
version.  We probably need a library function to parse whitespace
separated list of tokens, which may help other config parsers (color?  I
was too lazy to check).

>  static int update(int argc, const char **argv)
>  {
> +	int i, result = 0;
> +	struct path_list list = { NULL, 0, 0, 0 };
>  
> +	if (argc < 2 || (argc == 2 && !strcmp(argv[1], "default")))
> +		result = for_each_remote(get_one_remote_for_update, &list);
> +	else {
> +		remote_group.list = &list;
> +		for (i = 1; i < argc; i++) {
> +			remote_group.name = argv[i];
> +			result = git_config(get_remote_group);
> +		}
> +	}
> +	if (result)
> +		return result;
>  
> +	for (i = 0; i < list.nr; i++)
> +		result |= fetch_remote(list.items[i].path);
> +	list.strdup_paths = 1;
> +	path_list_clear(&list, 0);

This setting of strdup_paths after the fact and causing clear to free
things that were allocated while strdup_paths were set to 0 made me go
"Huh?", which means it needs explanation or code clarification.

> +
> +	return result;

The scripted one did not stop fetching upon error and this does not
regress it, but it reports error, which I think is a good change.

Re: [PATCH] remote: fix "update [group...]"

[Johannes Schindelin]

Hi,

On Mon, 3 Mar 2008, Junio C Hamano wrote:

> My reading of "sub update_remote {}" seems to indicate there still are
> some differences.

I will take care of your comments tomorrow (i.e. after sleeping), and yes, 
you are correct, I missed the fact that remotes.default has a special 
meaning.

Will fix,
Dscho

[PATCH v2] remote: fix "update [group...]"

[Johannes Schindelin]

The rewrite in C inadvertently broke updating with remote groups: when you
pass parameters to "git remote update", it used to look up "remotes.<group>"
for every parameter, and interpret the value as a list of remotes to update.

Also, no parameter, or a single parameter "default" should update all
remotes that have not been marked with "skipDefaultUpdate".

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---

	On Mon, 3 Mar 2008, Junio C Hamano wrote:

	> > +static int get_one_remote_for_update([...]
	>
	> This is called when no extra parameter is given or the name is 
	> default. It behaves differently from the scripted version if 
	> remote.default configuration exists, doesn't it?

	remotes.default, yes.  Fixed.

	> > +static int get_remote_group(const char *key, const char *value)
	> > +{
	> > +	if (!prefixcmp(key, "remotes.") &&
	> > +			!strcmp(key + 8, remote_group.name)) {
	> > +		char *space = strchr(value, ' ');
	> > +		while (space) {
	> > +			path_list_append(xstrndup(value, space - value),
	> > +					remote_group.list);
	> > +			value = space + 1;
	> > +			space = strchr(value, ' ');
	> > +		}
	> > +		path_list_append(xstrdup(value), remote_group.list);
	> > +	}
	> > +
	> 
	> This does not mimick the magic "split(' ', $conf)" in the scripted
	> version.

	I was not aware of that Perl feature.  Fixed.

	> > +	list.strdup_paths = 1;
	> > +	path_list_clear(&list, 0);
	> 
	> This setting of strdup_paths after the fact and causing clear to 
	> free things that were allocated while strdup_paths were set to 0 
	> made me go "Huh?", which means it needs explanation or code 
	> clarification.

	Fixed with a comment.

	> > +	return result;
	> 
	> The scripted one did not stop fetching upon error and this does 
	> not regress it, but it reports error, which I think is a good 
	> change.

	I overlooked that regression, but yes, it was out of a habit to 
	expect programs to fail when some part of them fails.

 builtin-remote.c  |   59 +++++++++++++++++++++++++++++-----
 t/t5505-remote.sh |   92 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 142 insertions(+), 9 deletions(-)

diff --git a/builtin-remote.c b/builtin-remote.c
index fc01bcc..aa90cc9 100644
--- a/builtin-remote.c
+++ b/builtin-remote.c
@@ -477,24 +477,65 @@ cleanup_states:
 	return result;
 }
 
-static int update_one(struct remote *remote, void *priv)
+static int get_one_remote_for_update(struct remote *remote, void *priv)
 {
+	struct path_list *list = priv;
 	if (!remote->skip_default_update)
-		return fetch_remote(remote->name);
+		path_list_append(xstrdup(remote->name), list);
+	return 0;
+}
+
+struct remote_group {
+	const char *name;
+	struct path_list *list;
+} remote_group;
+
+static int get_remote_group(const char *key, const char *value)
+{
+	if (!prefixcmp(key, "remotes.") &&
+			!strcmp(key + 8, remote_group.name)) {
+		/* split list by white space */
+		int space = strcspn(value, " \t\n");
+		while (*value) {
+			if (space > 1)
+				path_list_append(xstrndup(value, space),
+						remote_group.list);
+			value += space + (value[space] != '\0');
+			space = strcspn(value, " \t\n");
+		}
+	}
+
 	return 0;
 }
 
 static int update(int argc, const char **argv)
 {
-	int i;
+	int i, result = 0;
+	struct path_list list = { NULL, 0, 0, 0 };
+	static const char *default_argv[] = { NULL, "default", NULL };
 
-	if (argc < 2)
-		return for_each_remote(update_one, NULL);
+	if (argc < 2) {
+		argc = 2;
+		argv = default_argv;
+	}
 
-	for (i = 1; i < argc; i++)
-		if (fetch_remote(argv[i]))
-			return 1;
-	return 0;
+	remote_group.list = &list;
+	for (i = 1; i < argc; i++) {
+		remote_group.name = argv[i];
+		result = git_config(get_remote_group);
+	}
+
+	if (!result && !list.nr  && argc == 2 && !strcmp(argv[1], "default"))
+		result = for_each_remote(get_one_remote_for_update, &list);
+
+	for (i = 0; i < list.nr; i++)
+		result |= fetch_remote(list.items[i].path);
+
+	/* all names were strdup()ed or strndup()ed */
+	list.strdup_paths = 1;
+	path_list_clear(&list, 0);
+
+	return result;
 }
 
 static int get_one_entry(struct remote *remote, void *priv)
diff --git a/t/t5505-remote.sh b/t/t5505-remote.sh
index 0a25c8b..f45ea68 100755
--- a/t/t5505-remote.sh
+++ b/t/t5505-remote.sh
@@ -10,10 +10,12 @@ setup_repository () {
 	git init &&
 	>file &&
 	git add file &&
+	test_tick &&
 	git commit -m "Initial" &&
 	git checkout -b side &&
 	>elif &&
 	git add elif &&
+	test_tick &&
 	git commit -m "Second" &&
 	git checkout master
 	)
@@ -113,6 +115,7 @@ test_expect_success 'show' '
 	 git branch -d -r origin/master &&
 	 (cd ../one &&
 	  echo 1 > file &&
+	  test_tick &&
 	  git commit -m update file) &&
 	 git remote show origin > output &&
 	 git diff expect output)
@@ -144,4 +147,93 @@ test_expect_success 'add --mirror && prune' '
 	 git rev-parse --verify refs/heads/side)
 '
 
+cat > one/expect << EOF
+  apis/master
+  apis/side
+  drosophila/another
+  drosophila/master
+  drosophila/side
+EOF
+
+test_expect_success 'update' '
+
+	(cd one &&
+	 git remote add drosophila ../two &&
+	 git remote add apis ../mirror &&
+	 git remote update &&
+	 git branch -r > output &&
+	 git diff expect output)
+
+'
+
+cat > one/expect << EOF
+  drosophila/another
+  drosophila/master
+  drosophila/side
+  manduca/master
+  manduca/side
+  megaloprepus/master
+  megaloprepus/side
+EOF
+
+test_expect_success 'update with arguments' '
+
+	(cd one &&
+	 for b in $(git branch -r)
+	 do
+		git branch -r -d $b || break
+	 done &&
+	 git remote add manduca ../mirror &&
+	 git remote add megaloprepus ../mirror &&
+	 git config remotes.phobaeticus "drosophila megaloprepus" &&
+	 git config remotes.titanus manduca &&
+	 git remote update phobaeticus titanus &&
+	 git branch -r > output &&
+	 git diff expect output)
+
+'
+
+cat > one/expect << EOF
+  apis/master
+  apis/side
+  manduca/master
+  manduca/side
+  megaloprepus/master
+  megaloprepus/side
+EOF
+
+test_expect_success 'update default' '
+
+	(cd one &&
+	 for b in $(git branch -r)
+	 do
+		git branch -r -d $b || break
+	 done &&
+	 git config remote.drosophila.skipDefaultUpdate true &&
+	 git remote update default &&
+	 git branch -r > output &&
+	 git diff expect output)
+
+'
+
+cat > one/expect << EOF
+  drosophila/another
+  drosophila/master
+  drosophila/side
+EOF
+
+test_expect_success 'update default (overridden, with funny whitespace)' '
+
+	(cd one &&
+	 for b in $(git branch -r)
+	 do
+		git branch -r -d $b || break
+	 done &&
+	 git config remotes.default "$(printf "\t drosophila  \n")" &&
+	 git remote update default &&
+	 git branch -r > output &&
+	 git diff expect output)
+
+'
+
 test_done
-- 
1.5.4.3.570.g97e7d