[PATCH 0/3] git remote update: Check args and fallback to remotes

[PATCH 0/3] git remote update: Check args and fallback to remotes

[Finn Arne Gangstad]

This series is on top of next.

git remote update <non-existing> would previously silently do nothing.
With this patch series, it will (with 1/3) error out when non-existing groups
are given, and with 2/3 & 3/3 it will use a remote if a group cannot be found.

This enables "git remote update origin" for example. All previous uses
of "git remote update <x>" that did something useful should still work
exactly as before.

There seems to be no current way to check for the existence of a configured
remote, so 2/3 adds a remote_is_configured() function which checks for a
configured remote.

Finn Arne Gangstad (3):
  git remote update: Report error for non-existing groups
  remote: New function remote_is_configured()
  git remote update: Fallback to remote if group does not exist

 Documentation/git-remote.txt |    2 +-
 builtin-remote.c             |   17 ++++++++++++++---
 remote.c                     |   11 +++++++++++
 remote.h                     |    1 +
 4 files changed, 27 insertions(+), 4 deletions(-)

[PATCH 1/3] git remote update: Report error for non-existing groups

[Finn Arne Gangstad]

Previosly, git remote update <non-existing-group> would just silently fail
and do nothing. Now it will report an error saying that the group does
not exist.

Signed-off-by: Finn Arne Gangstad <finnag@pvv.org>
---
 builtin-remote.c |   11 ++++++++---
 1 files changed, 8 insertions(+), 3 deletions(-)

diff --git a/builtin-remote.c b/builtin-remote.c
index 3146eb4..51df99b 100644
--- a/builtin-remote.c
+++ b/builtin-remote.c
@@ -1188,16 +1188,18 @@ struct remote_group {
 	struct string_list *list;
 } remote_group;
 
-static int get_remote_group(const char *key, const char *value, void *cb)
+static int get_remote_group(const char *key, const char *value, void *num_hits)
 {
 	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)
+			if (space > 1) {
 				string_list_append(xstrndup(value, space),
 						remote_group.list);
+				++*((int *)num_hits);
+			}
 			value += space + (value[space] != '\0');
 			space = strcspn(value, " \t\n");
 		}
@@ -1227,8 +1229,11 @@ static int update(int argc, const char **argv)
 
 	remote_group.list = &list;
 	for (i = 1; i < argc; i++) {
+		int groups_found = 0;
 		remote_group.name = argv[i];
-		result = git_config(get_remote_group, NULL);
+		result = git_config(get_remote_group, &groups_found);
+		if (!groups_found && (i != 1 || strcmp(argv[1], "default")))
+			die("No such remote group: '%s'", argv[i]);
 	}
 
 	if (!result && !list.nr  && argc == 2 && !strcmp(argv[1], "default"))
-- 
1.6.2.1.471.gdfdaa

Re: [PATCH 1/3] git remote update: Report error for non-existing groups

[Junio C Hamano]

Finn Arne Gangstad <finnag@pvv.org> writes:

> @@ -1227,8 +1229,11 @@ static int update(int argc, const char **argv)
>  
>  	remote_group.list = &list;
>  	for (i = 1; i < argc; i++) {
> +		int groups_found = 0;
>  		remote_group.name = argv[i];
> -		result = git_config(get_remote_group, NULL);
> +		result = git_config(get_remote_group, &groups_found);
> +		if (!groups_found && (i != 1 || strcmp(argv[1], "default")))
> +			die("No such remote group: '%s'", argv[i]);

I think you are trying to be silent about the case where the caller feeds
you the default_argv[] array with this, but do we want to be more explicit
about this so that we do die when the end user explicitly says "default"
from the command line?

Re: [PATCH 1/3] git remote update: Report error for non-existing groups

[Finn Arne Gangstad]

On Tue, Apr 07, 2009 at 07:16:21PM -0700, Junio C Hamano wrote:
> Finn Arne Gangstad <finnag@pvv.org> writes:
> 
> > @@ -1227,8 +1229,11 @@ static int update(int argc, const char **argv)
> >  
> >  	remote_group.list = &list;
> >  	for (i = 1; i < argc; i++) {
> > +		int groups_found = 0;
> >  		remote_group.name = argv[i];
> > -		result = git_config(get_remote_group, NULL);
> > +		result = git_config(get_remote_group, &groups_found);
> > +		if (!groups_found && (i != 1 || strcmp(argv[1], "default")))
> > +			die("No such remote group: '%s'", argv[i]);
> 
> I think you are trying to be silent about the case where the caller feeds
> you the default_argv[] array with this, but do we want to be more explicit
> about this so that we do die when the end user explicitly says "default"
> from the command line?

Are you thinking that "git remote update default" should only be allowed
if you have configured a group named default?

The old code would allow "git remote update default" and actually do the
same as "git remote update", so I wanted to keep the (possibly unwanted?)
behaviour. If we want to disallow it, we can just do
if (!groups_found && argv != default_argv) instead.

- Finn Arne

Re: [PATCH 1/3] git remote update: Report error for non-existing groups

[Junio C Hamano]

Finn Arne Gangstad <finnag@pvv.org> writes:

> On Tue, Apr 07, 2009 at 07:16:21PM -0700, Junio C Hamano wrote:
>> Finn Arne Gangstad <finnag@pvv.org> writes:
>> 
>> > @@ -1227,8 +1229,11 @@ static int update(int argc, const char **argv)
>> >  
>> >  	remote_group.list = &list;
>> >  	for (i = 1; i < argc; i++) {
>> > +		int groups_found = 0;
>> >  		remote_group.name = argv[i];
>> > -		result = git_config(get_remote_group, NULL);
>> > +		result = git_config(get_remote_group, &groups_found);
>> > +		if (!groups_found && (i != 1 || strcmp(argv[1], "default")))
>> > +			die("No such remote group: '%s'", argv[i]);
>> 
>> I think you are trying to be silent about the case where the caller feeds
>> you the default_argv[] array with this, but do we want to be more explicit
>> about this so that we do die when the end user explicitly says "default"
>> from the command line?
>
> Are you thinking that "git remote update default" should only be allowed
> if you have configured a group named default?

I have no preference either way, and that is why I asked.

"git remote update" without explicit "default" is obviously what your code
try not to say "No such remote group" to, and that probably is a sane
thing to do.

I don't know what users want to see when they say "default" explicitly
without having an explicit configuration.  Should it do the same thing as
"git remote update"?

Re: [PATCH 1/3] git remote update: Report error for non-existing groups

[Jeff King]

On Wed, Apr 08, 2009 at 01:20:36AM -0700, Junio C Hamano wrote:

> I don't know what users want to see when they say "default" explicitly
> without having an explicit configuration.  Should it do the same thing as
> "git remote update"?

I'm not sure we have a choice anymore; is it worth breaking
compatibility to "fix" something that doesn't actually seem to be
harming anyone?

-Peff

Re: [PATCH 1/3] git remote update: Report error for non-existing groups

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Wed, Apr 08, 2009 at 01:20:36AM -0700, Junio C Hamano wrote:
>
>> I don't know what users want to see when they say "default" explicitly
>> without having an explicit configuration.  Should it do the same thing as
>> "git remote update"?
>
> I'm not sure we have a choice anymore; is it worth breaking
> compatibility to "fix" something that doesn't actually seem to be
> harming anyone?

Nope.  I do not use the "remote update" myself to begin with, and I
somehow suspect that the reason it does not seem to be harming anyone is
because nobody sane uses these "remote groups".

Anyway, I took the patch as-is already.

[PATCH 2/3] remote: New function remote_is_configured()

[Finn Arne Gangstad]

Previously, there was no beautiful way to check for the existence of
a configured remote. remote_get for example would always create the remote
"on demand".

This new function returns 1 if the remote is configured, 0 otherwise.

Signed-off-by: Finn Arne Gangstad <finnag@pvv.org>
---
 remote.c |   11 +++++++++++
 remote.h |    1 +
 2 files changed, 12 insertions(+), 0 deletions(-)

diff --git a/remote.c b/remote.c
index d12140e..a06761a 100644
--- a/remote.c
+++ b/remote.c
@@ -667,6 +667,17 @@ struct remote *remote_get(const char *name)
 	return ret;
 }
 
+int remote_is_configured(const char *name)
+{
+	int i;
+	read_config();
+
+	for (i = 0; i < remotes_nr; i++)
+		if (!strcmp(name, remotes[i]->name))
+			return 1;
+	return 0;
+}
+
 int for_each_remote(each_remote_fn fn, void *priv)
 {
 	int i, result = 0;
diff --git a/remote.h b/remote.h
index de3d21b..99706a8 100644
--- a/remote.h
+++ b/remote.h
@@ -45,6 +45,7 @@ struct remote {
 };
 
 struct remote *remote_get(const char *name);
+int remote_is_configured(const char *name);
 
 typedef int each_remote_fn(struct remote *remote, void *priv);
 int for_each_remote(each_remote_fn fn, void *priv);
-- 
1.6.2.1.471.gdfdaa

[PATCH 3/3] git remote update: Fallback to remote if group does not exist

[Finn Arne Gangstad]

Previously, git remote update <remote> would fail unless there was
a remote group configured with the same name as the remote.
git remote update will now fall back to using the remote if no matching
group can be found.

This enables "git remote update -p <remote>..." to fetch and prune one
or more remotes, for example.

Signed-off-by: Finn Arne Gangstad <finnag@pvv.org>
---
 Documentation/git-remote.txt |    2 +-
 builtin-remote.c             |   10 ++++++++--
 2 files changed, 9 insertions(+), 3 deletions(-)

diff --git a/Documentation/git-remote.txt b/Documentation/git-remote.txt
index 0b6e67d..9e2b4ea 100644
--- a/Documentation/git-remote.txt
+++ b/Documentation/git-remote.txt
@@ -16,7 +16,7 @@ SYNOPSIS
 'git remote set-head' <name> [-a | -d | <branch>]
 'git remote show' [-n] <name>
 'git remote prune' [-n | --dry-run] <name>
-'git remote update' [-p | --prune] [group]
+'git remote update' [-p | --prune] [group | remote]...
 
 DESCRIPTION
 -----------
diff --git a/builtin-remote.c b/builtin-remote.c
index 51df99b..ca7c639 100644
--- a/builtin-remote.c
+++ b/builtin-remote.c
@@ -1232,8 +1232,14 @@ static int update(int argc, const char **argv)
 		int groups_found = 0;
 		remote_group.name = argv[i];
 		result = git_config(get_remote_group, &groups_found);
-		if (!groups_found && (i != 1 || strcmp(argv[1], "default")))
-			die("No such remote group: '%s'", argv[i]);
+		if (!groups_found && (i != 1 || strcmp(argv[1], "default"))) {
+			struct remote *remote;
+			if (!remote_is_configured(argv[i]))
+				die("No such remote or remote group: %s",
+				    argv[i]);
+			remote = remote_get(argv[i]);
+			string_list_append(remote->name, remote_group.list);
+		}
 	}
 
 	if (!result && !list.nr  && argc == 2 && !strcmp(argv[1], "default"))
-- 
1.6.2.1.471.gdfdaa

Re: [PATCH 0/3] git remote update: Check args and fallback to remotes

[Jeff King]

On Mon, Apr 06, 2009 at 03:40:59PM +0200, Finn Arne Gangstad wrote:

> This series is on top of next.
> 
> git remote update <non-existing> would previously silently do nothing.
> With this patch series, it will (with 1/3) error out when non-existing groups
> are given, and with 2/3 & 3/3 it will use a remote if a group cannot be found.

Great, this was on my todo list so I am happy that procrastination saved
me some work. :)

The patches look fine to me, except that there are no tests. The patch
below adds a "remote groups" test script. There is a slight bit of
overlap with the update tests from t5505, but I don't think it is a
problem.

It is intended to be applied before your series. Your 1/3 would switch
t5506.3 from expect_failure to expect_success, and 3/3 would switch
t5506.6 from failure to success.

-- >8 --
Subject: [PATCH] add tests for remote groups

This tries to systematically cover existing behavior, and
also mark some expect_failure cases for desired behavior.

Signed-off-by: Jeff King <peff@peff.net>
---
 t/t5506-remote-groups.sh |   81 ++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 81 insertions(+), 0 deletions(-)
 create mode 100755 t/t5506-remote-groups.sh

diff --git a/t/t5506-remote-groups.sh b/t/t5506-remote-groups.sh
new file mode 100755
index 0000000..6653a9c
--- /dev/null
+++ b/t/t5506-remote-groups.sh
@@ -0,0 +1,81 @@
+#!/bin/sh
+
+test_description='git remote group handling'
+. ./test-lib.sh
+
+mark() {
+	echo "$1" >mark
+}
+
+update_repo() {
+	(cd $1 &&
+	echo content >>file &&
+	git add file &&
+	git commit -F ../mark)
+}
+
+update_repos() {
+	update_repo one $1 &&
+	update_repo two $1
+}
+
+repo_fetched() {
+	if test "`git log -1 --pretty=format:%s $1 --`" = "`cat mark`"; then
+		echo >&2 "repo was fetched: $1"
+		return 0
+	fi
+	echo >&2 "repo was not fetched: $1"
+	return 1
+}
+
+test_expect_success 'setup' '
+	mkdir one && (cd one && git init) &&
+	mkdir two && (cd two && git init) &&
+	git remote add -m master one one &&
+	git remote add -m master two two
+'
+
+test_expect_success 'no group updates all' '
+	mark update-all &&
+	update_repos &&
+	git remote update &&
+	repo_fetched one &&
+	repo_fetched two
+'
+
+test_expect_failure 'nonexistant group produces error' '
+	mark nonexistant &&
+	update_repos &&
+	test_must_fail git remote update nonexistant &&
+	! repo_fetched one &&
+	! repo_fetched two
+'
+
+test_expect_success 'updating group updates all members' '
+	mark group-all &&
+	update_repos &&
+	git config --add remotes.all one &&
+	git config --add remotes.all two &&
+	git remote update all &&
+	repo_fetched one &&
+	repo_fetched two
+'
+
+test_expect_success 'updating group does not update non-members' '
+	mark group-some &&
+	update_repos &&
+	git config --add remotes.some one &&
+	git remote update some &&
+	repo_fetched one &&
+	! repo_fetched two
+'
+
+test_expect_failure 'updating remote name updates that remote' '
+	mark remote-name &&
+	update_repos &&
+	git remote update one &&
+	repo_fetched one &&
+	! repo_fetched two
+'
+
+test_done
-- 
1.6.2.2.585.g1e067