remote show/prune: strange -n(--dry-run) option.

remote show/prune: strange -n(--dry-run) option.

[Olivier Marin]

Hello,

The git-remote documentation talks about a mysterious -n option for show
and prune that comes from the old git-remote.perl script. This flag was
used to prevent the script from calling ls-remote more than once, FWIU.
Today, the builtin accept an (un)?related -n(--dry-run) flag that does
nothing, actually. It seems broken.

So, is it safe to drop it entirely or is it better to just remove it
from the documentation for compatibility? In the second case, how long
should we wait before using --dry-run for something different?

I would like to make "git remote prune" more verbose and use --dry-run
to really prevent it from deleting stale tracking branches.

$ git remote prune origin -n
Pruning origin
From: git://.../myproject.git
  * [stale branch]    bla
  * [stale branch]    bli
  * [stale branch]    blu

What about something like that ?

Olivier.

[PATCH] Documentation/git-remote.txt: remove description for useless -n option

[Olivier Marin]

From: Olivier Marin <dkr@freesurf.fr>

This option comes from the original git-remote.perl script and is not
used nor needed in the current builtin.

So, remove it from the documentation so that we can reuse it later for
something else.

Signed-off-by: Olivier Marin <dkr@freesurf.fr>
---
  Documentation/git-remote.txt |    7 -------
  1 files changed, 0 insertions(+), 7 deletions(-)

diff --git a/Documentation/git-remote.txt b/Documentation/git-remote.txt
index e97dc09..e51d232 100644
--- a/Documentation/git-remote.txt
+++ b/Documentation/git-remote.txt
@@ -61,9 +61,6 @@ configuration settings for the remote are removed.
  'show'::

  Gives some information about the remote <name>.
-+
-With `-n` option, the remote heads are not queried first with
-`git ls-remote <name>`; cached information is used instead.

  'prune'::

@@ -71,10 +68,6 @@ Deletes all stale tracking branches under <name>.
  These stale branches have already been removed from the remote repository
  referenced by <name>, but are still locally available in
  "remotes/<name>".
-+
-With `-n` option, the remote heads are not confirmed first with `git
-ls-remote <name>`; cached information is used instead.  Use with
-caution.

  'update'::

-- 1.5.6.rc2.160.ga44ac

[PATCH] Documentation/git-remote.txt: remove description for useless -n option

[dkr+ml.git]

From: Olivier Marin <dkr@freesurf.fr>

This option comes from the original git-remote.perl script and is not
used nor needed in the current builtin.

So, remove it from the documentation so that we can reuse it later for
something else.

Signed-off-by: Olivier Marin <dkr@freesurf.fr>
---

My MUA destroyed the previous patch! Sorry.

 Documentation/git-remote.txt |    7 -------
 1 files changed, 0 insertions(+), 7 deletions(-)

diff --git a/Documentation/git-remote.txt b/Documentation/git-remote.txt
index e97dc09..e51d232 100644
--- a/Documentation/git-remote.txt
+++ b/Documentation/git-remote.txt
@@ -61,9 +61,6 @@ configuration settings for the remote are removed.
 'show'::
 
 Gives some information about the remote <name>.
-+
-With `-n` option, the remote heads are not queried first with
-`git ls-remote <name>`; cached information is used instead.
 
 'prune'::
 
@@ -71,10 +68,6 @@ Deletes all stale tracking branches under <name>.
 These stale branches have already been removed from the remote repository
 referenced by <name>, but are still locally available in
 "remotes/<name>".
-+
-With `-n` option, the remote heads are not confirmed first with `git
-ls-remote <name>`; cached information is used instead.  Use with
-caution.
 
 'update'::
 
-- 
1.5.6.rc2.160.ga44ac

Re: [PATCH] Documentation/git-remote.txt: remove description for useless -n option

[Junio C Hamano]

dkr+ml.git@free.fr writes:

> From: Olivier Marin <dkr@freesurf.fr>
>
> This option comes from the original git-remote.perl script and is not
> used nor needed in the current builtin.

Is this something we would want to document as a new feature, or just a
regression that makes the existing feature unusable when disconnected from
the network that needs to be fixed in the code?

Re: [PATCH] Documentation/git-remote.txt: remove description for useless -n option

[Olivier Marin]

Junio C Hamano a écrit :
>> From: Olivier Marin <dkr@freesurf.fr>
>>
>> This option comes from the original git-remote.perl script and is not
>> used nor needed in the current builtin.
> 
> Is this something we would want to document as a new feature, or just a
> regression that makes the existing feature unusable when disconnected from
> the network that needs to be fixed in the code?

OK, I restored the original behaviour for "git remote show", patch will follow.

But for "git remote prune" I don't no what to do. The perl script behaviour was
to delete all refs for the remote when called with -n. It seems really dangerous
to me, especially if I have no connection to restore them. The current builtin
doesn't honor the flag and with my patch it just does nothing.

So, do you prefere that I remove the documentation for git remote prune -n or
restore the old behaviour that was probably never used (but maybe I'm wrong)?

Olivier.

[PATCH] remote show: fix the -n option

[Olivier Marin]

From: Olivier Marin <dkr@freesurf.fr>

The perl version accepted a -n flag, to show local informations only
without querying remote heads, that seems to have been lost in the C
rewrite.

This restores the older behaviour and add a test case.

Signed-off-by: Olivier Marin <dkr@freesurf.fr>
---
 builtin-remote.c  |   48 ++++++++++++++++++++++++++----------------------
 t/t5505-remote.sh |   17 +++++++++++++++++
 2 files changed, 43 insertions(+), 22 deletions(-)

diff --git a/builtin-remote.c b/builtin-remote.c
index c49f00f..cb9e282 100644
--- a/builtin-remote.c
+++ b/builtin-remote.c
@@ -421,10 +421,10 @@ static void show_list(const char *title, struct path_list *list)
 
 static int show_or_prune(int argc, const char **argv, int prune)
 {
-	int dry_run = 0, result = 0;
+	int no_query = 0, result = 0;
 	struct option options[] = {
 		OPT_GROUP("show specific options"),
-		OPT__DRY_RUN(&dry_run),
+		OPT_BOOLEAN('n', NULL, &no_query, "do not query remotes"),
 		OPT_END()
 	};
 	struct ref_states states;
@@ -442,21 +442,25 @@ static int show_or_prune(int argc, const char **argv, int prune)
 		struct transport *transport;
 		const struct ref *ref;
 		struct strbuf buf;
-		int i, got_states;
+		int i, got_states = 1;
 
 		states.remote = remote_get(*argv);
 		if (!states.remote)
 			return error("No such remote: %s", *argv);
-		transport = transport_get(NULL, states.remote->url_nr > 0 ?
-			states.remote->url[0] : NULL);
-		ref = transport_get_remote_refs(transport);
-		transport_disconnect(transport);
 
 		read_branches();
-		got_states = get_ref_states(ref, &states);
-		if (got_states)
-			result = error("Error getting local info for '%s'",
-					states.remote->name);
+
+		if (!no_query) {
+			transport = transport_get(NULL, states.remote->url_nr > 0 ?
+				states.remote->url[0] : NULL);
+			ref = transport_get_remote_refs(transport);
+			transport_disconnect(transport);
+
+			got_states = get_ref_states(ref, &states);
+			if (got_states)
+				result = error("Error getting local info for '%s'",
+						states.remote->name);
+		}
 
 		if (prune) {
 			for (i = 0; i < states.stale.nr; i++) {
@@ -486,17 +490,17 @@ static int show_or_prune(int argc, const char **argv, int prune)
 			printf("\n");
 		}
 
-		if (got_states)
-			continue;
-		strbuf_init(&buf, 0);
-		strbuf_addf(&buf, "  New remote branch%%s (next fetch will "
-			"store in remotes/%s)", states.remote->name);
-		show_list(buf.buf, &states.new);
-		strbuf_release(&buf);
-		show_list("  Stale tracking branch%s (use 'git remote prune')",
-				&states.stale);
-		show_list("  Tracked remote branch%s",
-				&states.tracked);
+		if (!got_states) {
+			strbuf_init(&buf, 0);
+			strbuf_addf(&buf, "  New remote branch%%s (next fetch will "
+				"store in remotes/%s)", states.remote->name);
+			show_list(buf.buf, &states.new);
+			strbuf_release(&buf);
+			show_list("  Stale tracking branch%s (use 'git remote prune')",
+					&states.stale);
+			show_list("  Tracked remote branch%s",
+					&states.tracked);
+		}
 
 		if (states.remote->push_refspec_nr) {
 			printf("  Local branch%s pushed with 'git push'\n   ",
diff --git a/t/t5505-remote.sh b/t/t5505-remote.sh
index 0d7ed1f..c6a7bfb 100755
--- a/t/t5505-remote.sh
+++ b/t/t5505-remote.sh
@@ -138,6 +138,23 @@ test_expect_success 'show' '
 	 test_cmp expect output)
 '
 
+cat > test/expect << EOF
+* remote origin
+  URL: $(pwd)/one/.git
+  Remote branch merged with 'git pull' while on branch master
+    master
+  Local branches pushed with 'git push'
+    master:upstream +refs/tags/lastbackup
+EOF
+
+test_expect_success 'show -n' '
+	(mv one one.unreachable &&
+	 cd test &&
+	 git remote show -n origin > output &&
+	 mv ../one.unreachable ../one &&
+	 test_cmp expect output)
+'
+
 test_expect_success 'prune' '
 	(cd one &&
 	 git branch -m side side2) &&
-- 1.5.6.rc2.121.gaeb64.dirty

Re: [PATCH] remote show: fix the -n option

[Johannes Schindelin]

Hi,

On Mon, 9 Jun 2008, Olivier Marin wrote:

> The perl version accepted a -n flag, to show local informations only 
> without querying remote heads, that seems to have been lost in the C 
> rewrite.

Would have been nice to Cc: the author of the C rewrite.

> diff --git a/builtin-remote.c b/builtin-remote.c
> index c49f00f..cb9e282 100644
> --- a/builtin-remote.c
> +++ b/builtin-remote.c
> @@ -421,10 +421,10 @@ static void show_list(const char *title, struct path_list *list)
>  
>  static int show_or_prune(int argc, const char **argv, int prune)
>  {
> -	int dry_run = 0, result = 0;
> +	int no_query = 0, result = 0;

Why?

>  	struct option options[] = {
>  		OPT_GROUP("show specific options"),
> -		OPT__DRY_RUN(&dry_run),
> +		OPT_BOOLEAN('n', NULL, &no_query, "do not query remotes"),

Why?


> +			transport = transport_get(NULL, states.remote->url_nr > 0 ?

Please rewrap.

> @@ -486,17 +490,17 @@ static int show_or_prune(int argc, const char **argv, int prune)
>  			printf("\n");
>  		}
>  
> -		if (got_states)
> -			continue;
> -		strbuf_init(&buf, 0);
> -		strbuf_addf(&buf, "  New remote branch%%s (next fetch will "
> -			"store in remotes/%s)", states.remote->name);
> -		show_list(buf.buf, &states.new);
> -		strbuf_release(&buf);
> -		show_list("  Stale tracking branch%s (use 'git remote prune')",
> -				&states.stale);
> -		show_list("  Tracked remote branch%s",
> -				&states.tracked);
> +		if (!got_states) {
> +			strbuf_init(&buf, 0);
> +			strbuf_addf(&buf, "  New remote branch%%s (next fetch will "
> +				"store in remotes/%s)", states.remote->name);
> +			show_list(buf.buf, &states.new);
> +			strbuf_release(&buf);
> +			show_list("  Stale tracking branch%s (use 'git remote prune')",
> +					&states.stale);
> +			show_list("  Tracked remote branch%s",
> +					&states.tracked);
> +		}
>  
>  		if (states.remote->push_refspec_nr) {
>  			printf("  Local branch%s pushed with 'git push'\n   ",

Maybe we need two different values of got_states; not calling ls-remote 
and then showing things is okay, but calling ls-remote, getting an error 
and _then_ showing stuff is not okay, IMO.

Thanks,
Dscho

Re: [PATCH] remote show: fix the -n option

[Olivier Marin]

Johannes Schindelin a écrit :
> 
> Would have been nice to Cc: the author of the C rewrite.

Sorry for that, will do it next time.

>>  	struct option options[] = {
>>  		OPT_GROUP("show specific options"),
>> -		OPT__DRY_RUN(&dry_run),
>> +		OPT_BOOLEAN('n', NULL, &no_query, "do not query remotes"),
> 
> Why?

Because I think it's something different. It's more like in "route -n" than --dry-run
in "patch --dry-run". Don't you think ?

>> +			transport = transport_get(NULL, states.remote->url_nr > 0 ?
> 
> Please rewrap.

I'm not sure what you are talking about. Should I wrap after "NULL," instead of "?"?

> Maybe we need two different values of got_states; not calling ls-remote 
> and then showing things is okay, but calling ls-remote, getting an error 
> and _then_ showing stuff is not okay, IMO.

In fact, it seems that get_ref_states() always return 0 or just die when an error
occur. And that transport_get_remote_refs() never return if something goes wrong.

So, what about removing got_states and use !no_query instead ?

Olivier.

Re: [PATCH] remote show: fix the -n option

[Johannes Schindelin]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1479 bytes --]

Hi,

On Mon, 9 Jun 2008, Olivier Marin wrote:

> Johannes Schindelin a écrit :
> 
> >>  	struct option options[] = {
> >>  		OPT_GROUP("show specific options"),
> >> -		OPT__DRY_RUN(&dry_run),
> >> +		OPT_BOOLEAN('n', NULL, &no_query, "do not query remotes"),
> > 
> > Why?
> 
> Because I think it's something different. It's more like in "route -n" 
> than --dry-run in "patch --dry-run". Don't you think ?

No, I think that the information about stale branches and if the branches 
are up-to-date is missing.  In that sense, it is not like "route -n" at 
all, which just skips one convenience step, but really a dry run, because 
the result is different (as opposed to differently displayed).

> >> +			transport = transport_get(NULL, states.remote->url_nr > 0 ?
> > 
> > Please rewrap.
> 
> I'm not sure what you are talking about. Should I wrap after "NULL," 
> instead of "?"?

It is a too long line (way over 80 characters).  So yes, you should wrap 
after the NULL here.

> > Maybe we need two different values of got_states; not calling 
> > ls-remote and then showing things is okay, but calling ls-remote, 
> > getting an error and _then_ showing stuff is not okay, IMO.
> 
> In fact, it seems that get_ref_states() always return 0 or just die when 
> an error occur. And that transport_get_remote_refs() never return if 
> something goes wrong.
> 
> So, what about removing got_states and use !no_query instead ?

Hrmpf.  I did not mean to die() there...

Ciao,
Dscho

Re: [PATCH] remote show: fix the -n option

[Olivier Marin]

Johannes Schindelin a écrit :
> 
> No, I think that the information about stale branches and if the branches 
> are up-to-date is missing.  In that sense, it is not like "route -n" at 
> all, which just skips one convenience step, but really a dry run, because 
> the result is different (as opposed to differently displayed).

Am I wrong if I say that dry run is for commands that modify something? For
example there is no "diff --dry-run" probably because diff does not change
anything. A dry run has no real meaning for diff.

This the same for "git remote show": it's a read-only command, it just display
a summary of the remote and does not modify anything. With -n, it just skips
the ls-remote (read-only) step and yes the result can be different, some parts
can be missing. Exactly like "route -n", we skip the dns resolution, the host
names are missing.

Now, if we talk about "prune", I completely agree. A --dry-run flag make sens.
But it's not the same thing than the "show -n" one. For what reason would I
want to ask "prune" to skip the ls-remote step? What I would find more useful
is to make "prune" show what it is doing (like "update") and add a --dry-run
option to say "just show me, but do not touch anything". And we can even add a
-p flag to "update" to say "prune at the same time".

> It is a too long line (way over 80 characters).  So yes, you should wrap 
> after the NULL here.

Will fix. (my tabs were only 4 spaces long)

>> In fact, it seems that get_ref_states() always return 0 or just die when 
>> an error occur. And that transport_get_remote_refs() never return if 
>> something goes wrong.
>>
>> So, what about removing got_states and use !no_query instead ?
> 
> Hrmpf.  I did not mean to die() there...

I don't understand. Is it ok or not?

Thanks for your comments,
Olivier.

Re: [PATCH] remote show: fix the -n option

[Johannes Schindelin]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1207 bytes --]

Hi,

On Mon, 9 Jun 2008, Olivier Marin wrote:

> Johannes Schindelin a écrit :
> > 
> > No, I think that the information about stale branches and if the 
> > branches are up-to-date is missing.  In that sense, it is not like 
> > "route -n" at all, which just skips one convenience step, but really a 
> > dry run, because the result is different (as opposed to differently 
> > displayed).
> 
> Am I wrong if I say that dry run is for commands that modify something? 
> For example there is no "diff --dry-run" probably because diff does not 
> change anything. A dry run has no real meaning for diff.

For me, a dry run is something that avoids the high-cost operations.

Something like, uhm, a dry run of a ship.

> >> In fact, it seems that get_ref_states() always return 0 or just die 
> >> when an error occur. And that transport_get_remote_refs() never 
> >> return if something goes wrong.
> >>
> >> So, what about removing got_states and use !no_query instead ?
> > 
> > Hrmpf.  I did not mean to die() there...
> 
> I don't understand. Is it ok or not?

I would not like to remove the got_states.  I think this is the wrong 
direction.  Rather change the die() into a return error().

Ciao,
Dscho

Re: [PATCH] remote show: fix the -n option

[Olivier Marin]

Johannes Schindelin a écrit :
> 
> For me, a dry run is something that avoids the high-cost operations.

See:
 http://en.wikipedia.org/wiki/Dry_Run_(testing)
 http://www.askoxford.com/concise_oed/dryrun?view=uk
 http://encarta.msn.com/dictionary_1861689507/dry_run.html

It's more "do something as it was for real but it's not". It has nothing
to do with high-cost operations or something like that.

Yes?

> I would not like to remove the got_states.  I think this is the wrong 
> direction.  Rather change the die() into a return error().

OK, I will try that.

-- 
Olivier.

Re: [PATCH] remote show: fix the -n option

[Olivier Marin]

Olivier Marin a écrit :
> Johannes Schindelin a écrit :
> 
>> I would not like to remove the got_states.  I think this is the wrong 
>> direction.  Rather change the die() into a return error().
> 
> OK, I will try that.
> 

After reading some more code, I can say that changing die() in return
error() won't change anything here because, in get_ref_states() we only
die() if get_fetch_map() return an error. But guess what, get_fetch_map()
never return an error. It just die() or return 0. And I can't change it
without breaking "clone" and "fetch".

So, what I think is:

  Those changes are not in the scope of my patch. I can provide an other
  one for that, if you really care about. But IMHO it's not a problem to
  die(). Maybe we can simply remove the if () die().

  I will send a v2 patch with the changes we both agree.

Olivier.

Re: [PATCH] remote show: fix the -n option

[Johannes Schindelin]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1003 bytes --]

Hi,

On Mon, 9 Jun 2008, Olivier Marin wrote:

> Olivier Marin a écrit :
> > Johannes Schindelin a écrit :
> > 
> >> I would not like to remove the got_states.  I think this is the wrong 
> >> direction.  Rather change the die() into a return error().
> > 
> > OK, I will try that.
> > 
> 
> After reading some more code, I can say that changing die() in return 
> error() won't change anything here because, in get_ref_states() we only 
> die() if get_fetch_map() return an error. But guess what, 
> get_fetch_map() never return an error. It just die() or return 0. And I 
> can't change it without breaking "clone" and "fetch".

So you think it is okay, because the result is the same?  I think not.  I 
think this is exactly the way of thinking that makes reusing unlibified 
parts of Git's source code hard.  I think that this is exactly the style 
of programming I try to avoid, because it messes up clean concepts.

And I am utterly embarassed that we are talking about my code here.

Ciao,
Dscho

[PATCH v2] remote show: fix the -n option

[Olivier Marin]

From: Olivier Marin <dkr@freesurf.fr>

The perl version accepted a -n flag, to show local informations only
without querying remote heads, that seems to have been lost in the C
revrite.

This restores the older behaviour and add a test case.

Signed-off-by: Olivier Marin <dkr@freesurf.fr>
---
 builtin-remote.c  |   44 +++++++++++++++++++++++---------------------
 t/t5505-remote.sh |   17 +++++++++++++++++
 2 files changed, 40 insertions(+), 21 deletions(-)

diff --git a/builtin-remote.c b/builtin-remote.c
index c49f00f..efe74c7 100644
--- a/builtin-remote.c
+++ b/builtin-remote.c
@@ -421,10 +421,10 @@ static void show_list(const char *title, struct path_list *list)
 
 static int show_or_prune(int argc, const char **argv, int prune)
 {
-	int dry_run = 0, result = 0;
+	int no_query = 0, result = 0;
 	struct option options[] = {
 		OPT_GROUP("show specific options"),
-		OPT__DRY_RUN(&dry_run),
+		OPT_BOOLEAN('n', NULL, &no_query, "do not query remotes"),
 		OPT_END()
 	};
 	struct ref_states states;
@@ -442,21 +442,23 @@ static int show_or_prune(int argc, const char **argv, int prune)
 		struct transport *transport;
 		const struct ref *ref;
 		struct strbuf buf;
-		int i, got_states;
+		int i;
 
 		states.remote = remote_get(*argv);
 		if (!states.remote)
 			return error("No such remote: %s", *argv);
-		transport = transport_get(NULL, states.remote->url_nr > 0 ?
-			states.remote->url[0] : NULL);
-		ref = transport_get_remote_refs(transport);
-		transport_disconnect(transport);
 
 		read_branches();
-		got_states = get_ref_states(ref, &states);
-		if (got_states)
-			result = error("Error getting local info for '%s'",
-					states.remote->name);
+
+		if (!no_query) {
+			transport = transport_get(NULL,
+				states.remote->url_nr > 0 ?
+				states.remote->url[0] : NULL);
+			ref = transport_get_remote_refs(transport);
+			transport_disconnect(transport);
+
+			get_ref_states(ref, &states);
+		}
 
 		if (prune) {
 			for (i = 0; i < states.stale.nr; i++) {
@@ -486,17 +488,17 @@ static int show_or_prune(int argc, const char **argv, int prune)
 			printf("\n");
 		}
 
-		if (got_states)
-			continue;
-		strbuf_init(&buf, 0);
-		strbuf_addf(&buf, "  New remote branch%%s (next fetch will "
-			"store in remotes/%s)", states.remote->name);
-		show_list(buf.buf, &states.new);
-		strbuf_release(&buf);
-		show_list("  Stale tracking branch%s (use 'git remote prune')",
-				&states.stale);
-		show_list("  Tracked remote branch%s",
+		if (!no_query) {
+			strbuf_init(&buf, 0);
+			strbuf_addf(&buf, "  New remote branch%%s (next fetch "
+				"will store in remotes/%s)", states.remote->name);
+			show_list(buf.buf, &states.new);
+			strbuf_release(&buf);
+			show_list("  Stale tracking branch%s (use 'git remote "
+				"prune')", &states.stale);
+			show_list("  Tracked remote branch%s",
 				&states.tracked);
+		}
 
 		if (states.remote->push_refspec_nr) {
 			printf("  Local branch%s pushed with 'git push'\n   ",
diff --git a/t/t5505-remote.sh b/t/t5505-remote.sh
index 0d7ed1f..c6a7bfb 100755
--- a/t/t5505-remote.sh
+++ b/t/t5505-remote.sh
@@ -138,6 +138,23 @@ test_expect_success 'show' '
 	 test_cmp expect output)
 '
 
+cat > test/expect << EOF
+* remote origin
+  URL: $(pwd)/one/.git
+  Remote branch merged with 'git pull' while on branch master
+    master
+  Local branches pushed with 'git push'
+    master:upstream +refs/tags/lastbackup
+EOF
+
+test_expect_success 'show -n' '
+	(mv one one.unreachable &&
+	 cd test &&
+	 git remote show -n origin > output &&
+	 mv ../one.unreachable ../one &&
+	 test_cmp expect output)
+'
+
 test_expect_success 'prune' '
 	(cd one &&
 	 git branch -m side side2) &&

Re: [PATCH v2] remote show: fix the -n option

[Johannes Schindelin]

Hi,

On Mon, 9 Jun 2008, Olivier Marin wrote:

> diff --git a/builtin-remote.c b/builtin-remote.c
> index c49f00f..efe74c7 100644
> --- a/builtin-remote.c
> +++ b/builtin-remote.c
> @@ -421,10 +421,10 @@ static void show_list(const char *title, struct path_list *list)
>  
>  static int show_or_prune(int argc, const char **argv, int prune)
>  {
> -	int dry_run = 0, result = 0;
> +	int no_query = 0, result = 0;

Just for the record (not that I think anybody will care): I do not like 
this change.

> @@ -442,21 +442,23 @@ static int show_or_prune(int argc, const char **argv, int prune)
>  		struct transport *transport;
>  		const struct ref *ref;
>  		struct strbuf buf;
> -		int i, got_states;
> +		int i;
>  
>  		states.remote = remote_get(*argv);
>  		if (!states.remote)
>  			return error("No such remote: %s", *argv);
> -		transport = transport_get(NULL, states.remote->url_nr > 0 ?
> -			states.remote->url[0] : NULL);
> -		ref = transport_get_remote_refs(transport);
> -		transport_disconnect(transport);
>  
>  		read_branches();
> -		got_states = get_ref_states(ref, &states);
> -		if (got_states)
> -			result = error("Error getting local info for '%s'",
> -					states.remote->name);

And I do not like this change either.  It proliferates the "we just die() 
and do not care about reusing the code where die()ing is not desired" 
paradigm.

Sad,
Dscho

Re: [PATCH v2] remote show: fix the -n option

[Olivier Marin]

Johannes Schindelin a écrit :
> 
> And I do not like this change either.  It proliferates the "we just die() 
> and do not care about reusing the code where die()ing is not desired" 
> paradigm.

I agree and I'm OK to try to do something about that. But not in that patch.

This patch is just to fix a regression.

Olivier.

Re: [PATCH v2] remote show: fix the -n option

[Johannes Schindelin]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 507 bytes --]

Hi,

On Mon, 9 Jun 2008, Olivier Marin wrote:

> Johannes Schindelin a écrit :
> > 
> > And I do not like this change either.  It proliferates the "we just 
> > die() and do not care about reusing the code where die()ing is not 
> > desired" paradigm.
> 
> I agree and I'm OK to try to do something about that. But not in that patch.
> 
> This patch is just to fix a regression.

But did you not now make it harder to fix "that"?  By relying on the die() 
behaviour in your regression fix?

Whatever,
Dscho

Re: [PATCH v2] remote show: fix the -n option

[Olivier Marin]

Johannes Schindelin a écrit :
> 
> But did you not now make it harder to fix "that"?  By relying on the die() 
> behaviour in your regression fix?

If I change return path for some functions, I will have to check all the
callers anyway. So, no I don't think it make things harder to fix. Also
I don't like to add dead code.

Please, let me do this fix so that I can post my next patches. After that
I will be happy to work on what you asked.

Olivier.

[PATCH] builtin-remote: make reuse of code easier by not die()ing

[Johannes Schindelin]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1538 bytes --]


By mistake, this programmer used a die() call when an error() was much
more appropriate.  Code reuse was not possible, hence this fix.

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

	On Mon, 9 Jun 2008, Olivier Marin wrote:

	> Johannes Schindelin a écrit :
	> > 
	> > But did you not now make it harder to fix "that"?  By relying 
	> > on the die() behaviour in your regression fix?
	> 
	> If I change return path for some functions, I will have to check 
	> all the callers anyway. So, no I don't think it make things harder to 
	> fix. Also I don't like to add dead code.
	> 
	> Please, let me do this fix so that I can post my next patches. 
	> After that I will be happy to work on what you asked.

	Wow, that patch was hard ;-)

	BTW this thread shows -- again -- how hard it is to push toward 
	libification.  People seem to actively block it.

 builtin-remote.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/builtin-remote.c b/builtin-remote.c
index 2641e20..9939c96 100644
--- a/builtin-remote.c
+++ b/builtin-remote.c
@@ -243,7 +243,7 @@ static int get_ref_states(const struct ref *ref, struct ref_states *states)
 
 	for (i = 0; i < states->remote->fetch_refspec_nr; i++)
 		if (get_fetch_map(ref, states->remote->fetch + i, &tail, 1))
-			die("Could not get fetch map for refspec %s",
+			return error("Could not get fetch map for refspec %s",
 				states->remote->fetch_refspec[i]);
 
 	states->new.strdup_paths = states->tracked.strdup_paths = 1;
-- 
1.5.6.rc1.181.gb439d

Re: [PATCH] builtin-remote: make reuse of code easier by not die()ing

[Olivier Marin]

Johannes Schindelin a écrit :
> 
> 	Wow, that patch was hard ;-)

This is just what you wanted? I'm a little disappointed. ;-)

> 	BTW this thread shows -- again -- how hard it is to push toward 
> 	libification.  People seem to actively block it.

I don't see how this patch will solve the real problem. You just hide the die()
because get_fetch_map() still can die() and you add dead code. Now, the next
person that will use get_ref_states() will think it always return. Seems worse
to me.

IMHO, if you really want to libify you have to really analyze what should be
done. Split the work in coherent steps, write some specs to explain where you
want to go, how you planed to do it and why?

If you want to go this way, I'm ready to help you to do the hard work.

Olivier.

Re: [PATCH v2] remote show: fix the -n option

[Junio C Hamano]

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

> On Mon, 9 Jun 2008, Olivier Marin wrote:
>
>> diff --git a/builtin-remote.c b/builtin-remote.c
>> index c49f00f..efe74c7 100644
>> --- a/builtin-remote.c
>> +++ b/builtin-remote.c
>> @@ -421,10 +421,10 @@ static void show_list(const char *title, struct path_list *list)
>>  
>>  static int show_or_prune(int argc, const char **argv, int prune)
>>  {
>> -	int dry_run = 0, result = 0;
>> +	int no_query = 0, result = 0;
>
> Just for the record (not that I think anybody will care): I do not like 
> this change.

I do not think nobody cares ;-).

At least I care enough to point out that I think you are wrong in this
case.  "show -n" in the scripted version was never about "dry-run" but
was about "no-query".

The problem with the area of the code this patch touches is that compared
to the scripted version, show and prune now share their codepaths a bit
more, and it is less easy to keep -n disabled for prune (I think it is a
nonsense because you cannot "prune" sensibly without looking at what the
remote has.  It was a bug in the scripted version and losing it in C
rewrite was a "silent bugfix") while resurrecting -n for show (which is a
quick way to view where the URL points at without bothering to see what
remote branches there are).

I think a sensible thing to do would be to:

 - Agree that "-n" in the sense that "do not query" and "--dry-run" in the
   sense that "do not do anything but report what you would do" are
   different options.

 - Resurrect "show -n" as a quick way to view URLs without bothering to
   contact the remote end that is needed to show "the tracked branches"
   information.

 - Forbid "prune -n", which is nonsense.

 - Make "prune --dry-run" truly useful --- contact the other end, and
   report what will be pruned without really pruning them.

 - Perhaps as an enhancement, "show -n" could show what tracking branches
   we have from the remote, even though the information may be stale.
   The scripted version did not do this, I think, and it would be an
   improvement.

I am CC'ing Shawn who authored 859607d (Teach 'git remote' how to cleanup
stale tracking branches., 2007-02-02) to give him a chance to point out
why I am wrong in saying "prune -n" is nonsense.  Maybe there is a valid
use case for that option, even though I do not see one.

Re: [PATCH v2] remote show: fix the -n option

[Shawn O. Pearce]

Junio C Hamano <gitster@pobox.com> wrote:
> At least I care enough to point out that I think you are wrong in this
> case.  "show -n" in the scripted version was never about "dry-run" but
> was about "no-query".
...
> I am CC'ing Shawn who authored 859607d (Teach 'git remote' how to cleanup
> stale tracking branches., 2007-02-02) to give him a chance to point out
> why I am wrong in saying "prune -n" is nonsense.  Maybe there is a valid
> use case for that option, even though I do not see one.

I agree with you Junio.  "prune -n" is nonsense.  You cannot know
what to remove locally that the remote no longer advertises without
querying the remote.

So "prune -n" is nonsense and should issue an error.  "prune --dry-run"
is different and means "query, show what you would delete, but don't
actually delete".

Likewise "show --dry-run" is nonsense.  What does it mean to show
what would show without showing it?  Just show it.   :)

-- 
Shawn.

Re: [PATCH v2] remote show: fix the -n option

[Johannes Schindelin]

Hi,

On Mon, 9 Jun 2008, Shawn O. Pearce wrote:

> Likewise "show --dry-run" is nonsense.  What does it mean to show what 
> would show without showing it?  Just show it.  :)

Ah, that clarifies it.

Ciao,
Dscho

[PATCH 0/4] remote show/prune improvement

[Olivier Marin]

From: Olivier Marin <dkr@freesurf.fr>

Junio C Hamano a écrit :
> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
>> On Mon, 9 Jun 2008, Olivier Marin wrote:
>>
>>> diff --git a/builtin-remote.c b/builtin-remote.c
>>> index c49f00f..efe74c7 100644
>>> --- a/builtin-remote.c
>>> +++ b/builtin-remote.c
>>> @@ -421,10 +421,10 @@ static void show_list(const char *title, struct path_list *list)
>>>  
>>>  static int show_or_prune(int argc, const char **argv, int prune)
>>>  {
>>> -	int dry_run = 0, result = 0;
>>> +	int no_query = 0, result = 0;
>> Just for the record (not that I think anybody will care): I do not like 
>> this change.
> 
> I do not think nobody cares ;-).
> 
> At least I care enough to point out that I think you are wrong in this
> case.  "show -n" in the scripted version was never about "dry-run" but
> was about "no-query".
> 
> The problem with the area of the code this patch touches is that compared
> to the scripted version, show and prune now share their codepaths a bit
> more, and it is less easy to keep -n disabled for prune (I think it is a
> nonsense because you cannot "prune" sensibly without looking at what the
> remote has.  It was a bug in the scripted version and losing it in C
> rewrite was a "silent bugfix") while resurrecting -n for show (which is a
> quick way to view where the URL points at without bothering to see what
> remote branches there are).
> 
> I think a sensible thing to do would be to:
> 
>  - Agree that "-n" in the sense that "do not query" and "--dry-run" in the
>    sense that "do not do anything but report what you would do" are
>    different options.
> 
>  - Resurrect "show -n" as a quick way to view URLs without bothering to
>    contact the remote end that is needed to show "the tracked branches"
>    information.
> 
>  - Forbid "prune -n", which is nonsense.
> 
>  - Make "prune --dry-run" truly useful --- contact the other end, and
>    report what will be pruned without really pruning them.
> 
>  - Perhaps as an enhancement, "show -n" could show what tracking branches
>    we have from the remote, even though the information may be stale.
>    The scripted version did not do this, I think, and it would be an
>    improvement.

  [1/4] remote show: fix the -n option
  [2/4] builtin-remote: split show_or_prune() in two separate functions.
  [3/4] remote prune: print the list of pruned branches
  [4/4] remote show: list tracked remote branches with -n.

 Documentation/git-remote.txt |    9 +--
 builtin-remote.c             |  160 ++++++++++++++++++++++++++++++------------
 t/t5505-remote.sh            |   36 ++++++++++
 3 files changed, 154 insertions(+), 51 deletions(-)

Olivier.

[PATCH 1/4] remote show: fix the -n option

[Olivier Marin]

From: Olivier Marin <dkr@freesurf.fr>

The perl version accepted a -n flag, to show local informations only
without querying remote heads, that seems to have been lost in the C
revrite.

This restores the older behaviour and add a test case.

Signed-off-by: Olivier Marin <dkr@freesurf.fr>
---
 Documentation/git-remote.txt |    2 +-
 builtin-remote.c             |   44 +++++++++++++++++++++--------------------
 t/t5505-remote.sh            |   17 ++++++++++++++++
 3 files changed, 41 insertions(+), 22 deletions(-)

diff --git a/Documentation/git-remote.txt b/Documentation/git-remote.txt
index 782b055..7bd024e 100644
--- a/Documentation/git-remote.txt
+++ b/Documentation/git-remote.txt
@@ -12,7 +12,7 @@ SYNOPSIS
 'git-remote' [-v | --verbose]
 'git-remote' add [-t <branch>] [-m <master>] [-f] [--mirror] <name> <url>
 'git-remote' rm <name>
-'git-remote' show <name>
+'git-remote' show [-n] <name>
 'git-remote' prune <name>
 'git-remote' update [group]
 
diff --git a/builtin-remote.c b/builtin-remote.c
index c49f00f..efe74c7 100644
--- a/builtin-remote.c
+++ b/builtin-remote.c
@@ -421,10 +421,10 @@ static void show_list(const char *title, struct path_list *list)
 
 static int show_or_prune(int argc, const char **argv, int prune)
 {
-	int dry_run = 0, result = 0;
+	int no_query = 0, result = 0;
 	struct option options[] = {
 		OPT_GROUP("show specific options"),
-		OPT__DRY_RUN(&dry_run),
+		OPT_BOOLEAN('n', NULL, &no_query, "do not query remotes"),
 		OPT_END()
 	};
 	struct ref_states states;
@@ -442,21 +442,23 @@ static int show_or_prune(int argc, const char **argv, int prune)
 		struct transport *transport;
 		const struct ref *ref;
 		struct strbuf buf;
-		int i, got_states;
+		int i;
 
 		states.remote = remote_get(*argv);
 		if (!states.remote)
 			return error("No such remote: %s", *argv);
-		transport = transport_get(NULL, states.remote->url_nr > 0 ?
-			states.remote->url[0] : NULL);
-		ref = transport_get_remote_refs(transport);
-		transport_disconnect(transport);
 
 		read_branches();
-		got_states = get_ref_states(ref, &states);
-		if (got_states)
-			result = error("Error getting local info for '%s'",
-					states.remote->name);
+
+		if (!no_query) {
+			transport = transport_get(NULL,
+				states.remote->url_nr > 0 ?
+				states.remote->url[0] : NULL);
+			ref = transport_get_remote_refs(transport);
+			transport_disconnect(transport);
+
+			get_ref_states(ref, &states);
+		}
 
 		if (prune) {
 			for (i = 0; i < states.stale.nr; i++) {
@@ -486,17 +488,17 @@ static int show_or_prune(int argc, const char **argv, int prune)
 			printf("\n");
 		}
 
-		if (got_states)
-			continue;
-		strbuf_init(&buf, 0);
-		strbuf_addf(&buf, "  New remote branch%%s (next fetch will "
-			"store in remotes/%s)", states.remote->name);
-		show_list(buf.buf, &states.new);
-		strbuf_release(&buf);
-		show_list("  Stale tracking branch%s (use 'git remote prune')",
-				&states.stale);
-		show_list("  Tracked remote branch%s",
+		if (!no_query) {
+			strbuf_init(&buf, 0);
+			strbuf_addf(&buf, "  New remote branch%%s (next fetch "
+				"will store in remotes/%s)", states.remote->name);
+			show_list(buf.buf, &states.new);
+			strbuf_release(&buf);
+			show_list("  Stale tracking branch%s (use 'git remote "
+				"prune')", &states.stale);
+			show_list("  Tracked remote branch%s",
 				&states.tracked);
+		}
 
 		if (states.remote->push_refspec_nr) {
 			printf("  Local branch%s pushed with 'git push'\n   ",
diff --git a/t/t5505-remote.sh b/t/t5505-remote.sh
index 0d7ed1f..c6a7bfb 100755
--- a/t/t5505-remote.sh
+++ b/t/t5505-remote.sh
@@ -138,6 +138,23 @@ test_expect_success 'show' '
 	 test_cmp expect output)
 '
 
+cat > test/expect << EOF
+* remote origin
+  URL: $(pwd)/one/.git
+  Remote branch merged with 'git pull' while on branch master
+    master
+  Local branches pushed with 'git push'
+    master:upstream +refs/tags/lastbackup
+EOF
+
+test_expect_success 'show -n' '
+	(mv one one.unreachable &&
+	 cd test &&
+	 git remote show -n origin > output &&
+	 mv ../one.unreachable ../one &&
+	 test_cmp expect output)
+'
+
 test_expect_success 'prune' '
 	(cd one &&
 	 git branch -m side side2) &&
-- 
1.5.6.rc2.160.gd660c

[PATCH 2/4] builtin-remote: split show_or_prune() in two separate functions

[Olivier Marin]

From: Olivier Marin <dkr@freesurf.fr>

This allow us to add different features to each of them and keep the
code simple at the same time. Also create a get_remote_ref_states()
to avoid duplicated code.

Signed-off-by: Olivier Marin <dkr@freesurf.fr>
---
 builtin-remote.c |  101 +++++++++++++++++++++++++++++++++++------------------
 1 files changed, 67 insertions(+), 34 deletions(-)

diff --git a/builtin-remote.c b/builtin-remote.c
index efe74c7..745a4ee 100644
--- a/builtin-remote.c
+++ b/builtin-remote.c
@@ -419,7 +419,32 @@ static void show_list(const char *title, struct path_list *list)
 	printf("\n");
 }
 
-static int show_or_prune(int argc, const char **argv, int prune)
+static int get_remote_ref_states(const char *name,
+				 struct ref_states *states,
+				 int query)
+{
+	struct transport *transport;
+	const struct ref *ref;
+
+	states->remote = remote_get(name);
+	if (!states->remote)
+		return error("No such remote: %s", name);
+
+	read_branches();
+
+	if (query) {
+		transport = transport_get(NULL, states->remote->url_nr > 0 ?
+			states->remote->url[0] : NULL);
+		ref = transport_get_remote_refs(transport);
+		transport_disconnect(transport);
+
+		get_ref_states(ref, states);
+	}
+
+	return 0;
+}
+
+static int show(int argc, const char **argv)
 {
 	int no_query = 0, result = 0;
 	struct option options[] = {
@@ -431,42 +456,15 @@ static int show_or_prune(int argc, const char **argv, int prune)
 
 	argc = parse_options(argc, argv, options, builtin_remote_usage, 0);
 
-	if (argc < 1) {
-		if (!prune)
-			return show_all();
-		usage_with_options(builtin_remote_usage, options);
-	}
+	if (argc < 1)
+		return show_all();
 
 	memset(&states, 0, sizeof(states));
 	for (; argc; argc--, argv++) {
-		struct transport *transport;
-		const struct ref *ref;
 		struct strbuf buf;
 		int i;
 
-		states.remote = remote_get(*argv);
-		if (!states.remote)
-			return error("No such remote: %s", *argv);
-
-		read_branches();
-
-		if (!no_query) {
-			transport = transport_get(NULL,
-				states.remote->url_nr > 0 ?
-				states.remote->url[0] : NULL);
-			ref = transport_get_remote_refs(transport);
-			transport_disconnect(transport);
-
-			get_ref_states(ref, &states);
-		}
-
-		if (prune) {
-			for (i = 0; i < states.stale.nr; i++) {
-				const char *refname = states.stale.items[i].util;
-				result |= delete_ref(refname, NULL);
-			}
-			goto cleanup_states;
-		}
+		get_remote_ref_states(*argv, &states, !no_query);
 
 		printf("* remote %s\n  URL: %s\n", *argv,
 			states.remote->url_nr > 0 ?
@@ -513,7 +511,42 @@ static int show_or_prune(int argc, const char **argv, int prune)
 			}
 			printf("\n");
 		}
-cleanup_states:
+
+		/* NEEDSWORK: free remote */
+		path_list_clear(&states.new, 0);
+		path_list_clear(&states.stale, 0);
+		path_list_clear(&states.tracked, 0);
+	}
+
+	return result;
+}
+
+static int prune(int argc, const char **argv)
+{
+	int no_query = 0, result = 0;
+	struct option options[] = {
+		OPT_GROUP("prune specific options"),
+		OPT_BOOLEAN('n', NULL, &no_query, "do not query remotes"),
+		OPT_END()
+	};
+	struct ref_states states;
+
+	argc = parse_options(argc, argv, options, builtin_remote_usage, 0);
+
+	if (argc < 1)
+		usage_with_options(builtin_remote_usage, options);
+
+	memset(&states, 0, sizeof(states));
+	for (; argc; argc--, argv++) {
+		int i;
+
+		get_remote_ref_states(*argv, &states, !no_query);
+
+		for (i = 0; i < states.stale.nr; i++) {
+			const char *refname = states.stale.items[i].util;
+			result |= delete_ref(refname, NULL);
+		}
+
 		/* NEEDSWORK: free remote */
 		path_list_clear(&states.new, 0);
 		path_list_clear(&states.stale, 0);
@@ -634,9 +667,9 @@ int cmd_remote(int argc, const char **argv, const char *prefix)
 	else if (!strcmp(argv[0], "rm"))
 		result = rm(argc, argv);
 	else if (!strcmp(argv[0], "show"))
-		result = show_or_prune(argc, argv, 0);
+		result = show(argc, argv);
 	else if (!strcmp(argv[0], "prune"))
-		result = show_or_prune(argc, argv, 1);
+		result = prune(argc, argv);
 	else if (!strcmp(argv[0], "update"))
 		result = update(argc, argv);
 	else {
-- 
1.5.6.rc2.160.gd660c

[PATCH 3/4] remote prune: print the list of pruned branches

[Olivier Marin]

From: Olivier Marin <dkr@freesurf.fr>

This command is really too quiet which make it unconfortable to use.

Also implement a --dry-run option, in place of the original -n one, to
list stale tracking branches that will be pruned, but do not actually
prune them.

Add a test case for --dry-run.

Signed-off-by: Olivier Marin <dkr@freesurf.fr>
---
 Documentation/git-remote.txt |    7 +++----
 builtin-remote.c             |   20 ++++++++++++++++----
 t/t5505-remote.sh            |   18 ++++++++++++++++++
 3 files changed, 37 insertions(+), 8 deletions(-)

diff --git a/Documentation/git-remote.txt b/Documentation/git-remote.txt
index 7bd024e..345943a 100644
--- a/Documentation/git-remote.txt
+++ b/Documentation/git-remote.txt
@@ -13,7 +13,7 @@ SYNOPSIS
 'git-remote' add [-t <branch>] [-m <master>] [-f] [--mirror] <name> <url>
 'git-remote' rm <name>
 'git-remote' show [-n] <name>
-'git-remote' prune <name>
+'git-remote' prune [-n | --dry-run] <name>
 'git-remote' update [group]
 
 DESCRIPTION
@@ -80,9 +80,8 @@ These stale branches have already been removed from the remote repository
 referenced by <name>, but are still locally available in
 "remotes/<name>".
 +
-With `-n` option, the remote heads are not confirmed first with `git
-ls-remote <name>`; cached information is used instead.  Use with
-caution.
+With `--dry-run` option, report what branches will be pruned, but do no
+actually prune them.
 
 'update'::
 
diff --git a/builtin-remote.c b/builtin-remote.c
index 745a4ee..851bdde 100644
--- a/builtin-remote.c
+++ b/builtin-remote.c
@@ -521,12 +521,14 @@ static int show(int argc, const char **argv)
 	return result;
 }
 
+#define SUMMARY_WIDTH (2 * DEFAULT_ABBREV + 3)
+
 static int prune(int argc, const char **argv)
 {
-	int no_query = 0, result = 0;
+	int dry_run = 0, result = 0;
 	struct option options[] = {
 		OPT_GROUP("prune specific options"),
-		OPT_BOOLEAN('n', NULL, &no_query, "do not query remotes"),
+		OPT__DRY_RUN(&dry_run),
 		OPT_END()
 	};
 	struct ref_states states;
@@ -540,11 +542,21 @@ static int prune(int argc, const char **argv)
 	for (; argc; argc--, argv++) {
 		int i;
 
-		get_remote_ref_states(*argv, &states, !no_query);
+		get_remote_ref_states(*argv, &states, 1);
+
+		printf("Pruning %s\n", *argv);
+		if (states.stale.nr)
+			printf("From: %s\n", states.remote->url[0]);
 
 		for (i = 0; i < states.stale.nr; i++) {
 			const char *refname = states.stale.items[i].util;
-			result |= delete_ref(refname, NULL);
+
+			if (!dry_run)
+				result |= delete_ref(refname, NULL);
+
+			printf(" * %-*s %s\n", SUMMARY_WIDTH, "[stale branch]",
+				refname + strlen("refs/remotes/")
+				+ strlen(*argv) + 1);
 		}
 
 		/* NEEDSWORK: free remote */
diff --git a/t/t5505-remote.sh b/t/t5505-remote.sh
index c6a7bfb..c27cfad 100755
--- a/t/t5505-remote.sh
+++ b/t/t5505-remote.sh
@@ -165,6 +165,24 @@ test_expect_success 'prune' '
 	 ! git rev-parse refs/remotes/origin/side)
 '
 
+cat > test/expect << EOF
+Pruning origin
+From: $(pwd)/one/.git
+ * [stale branch]    side2
+EOF
+
+test_expect_success 'prune --dry-run' '
+	(cd one &&
+	 git branch -m side2 side) &&
+	(cd test &&
+	 git remote prune --dry-run origin > output &&
+	 git rev-parse refs/remotes/origin/side2 &&
+	 ! git rev-parse refs/remotes/origin/side &&
+	(cd ../one &&
+	 git branch -m side side2) &&
+	 test_cmp expect output)
+'
+
 test_expect_success 'add --mirror && prune' '
 	(mkdir mirror &&
 	 cd mirror &&
-- 
1.5.6.rc2.160.gd660c

Re: [PATCH 3/4] remote prune: print the list of pruned branches

[Junio C Hamano]

Olivier Marin <dkr+ml.git@free.fr> writes:

> diff --git a/builtin-remote.c b/builtin-remote.c
> index 745a4ee..851bdde 100644
> --- a/builtin-remote.c
> +++ b/builtin-remote.c
> ...  
> +		printf("Pruning %s\n", *argv);
> +		if (states.stale.nr)
> +			printf("From: %s\n", states.remote->url[0]);

Thanks.  I've queued the series (with minor fixups and rewording) to
'next' already, hoping that we can merge this fix to 'master' before
1.5.6.

But I am very tempted to also apply the following on top.  Thoughts?

-- >8 --
[PATCH] "remote prune": be quiet when there is nothing to prune

The previous commit made it always say "Pruning $remote" but reported the
URL only when there is something to prune.  Make it consistent by not
saying anything at all when there is nothing to prune.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin-remote.c |    5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/builtin-remote.c b/builtin-remote.c
index 4b00cf9..145dd85 100644
--- a/builtin-remote.c
+++ b/builtin-remote.c
@@ -560,12 +560,13 @@ static int prune(int argc, const char **argv)
 
 		get_remote_ref_states(*argv, &states, 1);
 
-		printf("Pruning %s\n", *argv);
-		if (states.stale.nr)
+		if (states.stale.nr) {
+			printf("Pruning %s\n", *argv);
 			printf("URL: %s\n",
 			       states.remote->url_nr
 			       ? states.remote->url[0]
 			       : "(no URL)");
+		}
 
 		for (i = 0; i < states.stale.nr; i++) {
 			const char *refname = states.stale.items[i].util;
-- 
1.5.6.rc2.26.g8c37

Re: [PATCH 3/4] remote prune: print the list of pruned branches

[Olivier Marin]

Junio C Hamano a écrit :
> 
> Thanks.  I've queued the series (with minor fixups and rewording) to
> 'next' already, hoping that we can merge this fix to 'master' before
> 1.5.6.

Thanks. I find your "would prune/pruned" better.

> But I am very tempted to also apply the following on top.  Thoughts?

Actually, I did that to stay consistent with "git remote update" and, as
a user, I prefer to see something. That said, I not opposed to your patch.

Olivier.

[PATCH 4/4] remote show: list tracked remote branches with -n

[Olivier Marin]

From: Olivier Marin <dkr@freesurf.fr>

Signed-off-by: Olivier Marin <dkr@freesurf.fr>
---
 builtin-remote.c  |   25 +++++++++++++++++++++++--
 t/t5505-remote.sh |    3 ++-
 2 files changed, 25 insertions(+), 3 deletions(-)

diff --git a/builtin-remote.c b/builtin-remote.c
index 851bdde..de4a4f2 100644
--- a/builtin-remote.c
+++ b/builtin-remote.c
@@ -444,6 +444,25 @@ static int get_remote_ref_states(const char *name,
 	return 0;
 }
 
+static int append_ref_to_tracked_list(const char *refname,
+	const unsigned char *sha1, int flags, void *cb_data)
+{
+	struct ref_states *states = cb_data;
+	struct strbuf buf;
+
+	strbuf_init(&buf, 0);
+	strbuf_addf(&buf, "%s/", states->remote->name);
+	if (strncmp(buf.buf, refname, buf.len)) {
+		strbuf_release(&buf);
+		return 0;
+	}
+
+	path_list_append(skip_prefix(refname, strbuf_detach(&buf, NULL)),
+		&states->tracked);
+
+	return 0;
+}
+
 static int show(int argc, const char **argv)
 {
 	int no_query = 0, result = 0;
@@ -494,10 +513,12 @@ static int show(int argc, const char **argv)
 			strbuf_release(&buf);
 			show_list("  Stale tracking branch%s (use 'git remote "
 				"prune')", &states.stale);
-			show_list("  Tracked remote branch%s",
-				&states.tracked);
 		}
 
+		if (no_query)
+			for_each_remote_ref(append_ref_to_tracked_list, &states);
+		show_list("  Tracked remote branch%s", &states.tracked);
+
 		if (states.remote->push_refspec_nr) {
 			printf("  Local branch%s pushed with 'git push'\n   ",
 				states.remote->push_refspec_nr > 1 ?
diff --git a/t/t5505-remote.sh b/t/t5505-remote.sh
index c27cfad..ec5ea54 100755
--- a/t/t5505-remote.sh
+++ b/t/t5505-remote.sh
@@ -137,12 +137,13 @@ test_expect_success 'show' '
 	 git remote show origin > output &&
 	 test_cmp expect output)
 '
-
 cat > test/expect << EOF
 * remote origin
   URL: $(pwd)/one/.git
   Remote branch merged with 'git pull' while on branch master
     master
+  Tracked remote branch
+    side
   Local branches pushed with 'git push'
     master:upstream +refs/tags/lastbackup
 EOF
-- 
1.5.6.rc2.160.gd660c

Re: [PATCH 4/4] remote show: list tracked remote branches with -n

[Junio C Hamano]

Olivier Marin <dkr+ml.git@free.fr> writes:

> From: Olivier Marin <dkr@freesurf.fr>
>
> Signed-off-by: Olivier Marin <dkr@freesurf.fr>
> ---
>  builtin-remote.c  |   25 +++++++++++++++++++++++--
>  t/t5505-remote.sh |    3 ++-
>  2 files changed, 25 insertions(+), 3 deletions(-)
>
> diff --git a/builtin-remote.c b/builtin-remote.c
> index 851bdde..de4a4f2 100644
> --- a/builtin-remote.c
> +++ b/builtin-remote.c
> @@ -444,6 +444,25 @@ static int get_remote_ref_states(const char *name,
>  	return 0;
>  }
>  
> +static int append_ref_to_tracked_list(const char *refname,
> +	const unsigned char *sha1, int flags, void *cb_data)
> +{
> +	struct ref_states *states = cb_data;
> +	struct strbuf buf;
> +
> +	strbuf_init(&buf, 0);
> +	strbuf_addf(&buf, "%s/", states->remote->name);
> +	if (strncmp(buf.buf, refname, buf.len)) {
> +		strbuf_release(&buf);
> +		return 0;
> +	}

Doesn't this have the same issue Shawn fixed in 7ad2458 (Make "git-remote
rm" delete refs acccording to fetch specs, 2008-06-01)?

Re: [PATCH v2 4/4] remote show: list tracked remote branches with -n

[Olivier Marin]

From: Olivier Marin <dkr@freesurf.fr>

Signed-off-by: Olivier Marin <dkr@freesurf.fr>
---

Junio C Hamano a écrit :
> Olivier Marin <dkr+ml.git@free.fr> writes:
> 
>> +static int append_ref_to_tracked_list(const char *refname,
>> +	const unsigned char *sha1, int flags, void *cb_data)
>> +{
>> +	struct ref_states *states = cb_data;
>> +	struct strbuf buf;
>> +
>> +	strbuf_init(&buf, 0);
>> +	strbuf_addf(&buf, "%s/", states->remote->name);
>> +	if (strncmp(buf.buf, refname, buf.len)) {
>> +		strbuf_release(&buf);
>> +		return 0;
>> +	}
> 
> Doesn't this have the same issue Shawn fixed in 7ad2458 (Make "git-remote
> rm" delete refs acccording to fetch specs, 2008-06-01)?

You are right. This version should fix this.

 builtin-remote.c  |   22 ++++++++++++++++++++--
 t/t5505-remote.sh |    2 ++
 2 files changed, 22 insertions(+), 2 deletions(-)

diff --git a/builtin-remote.c b/builtin-remote.c
index 851bdde..d55d320 100644
--- a/builtin-remote.c
+++ b/builtin-remote.c
@@ -444,6 +444,22 @@ static int get_remote_ref_states(const char *name,
 	return 0;
 }
 
+static int append_ref_to_tracked_list(const char *refname,
+	const unsigned char *sha1, int flags, void *cb_data)
+{
+	struct ref_states *states = cb_data;
+	struct refspec refspec;
+
+	memset(&refspec, 0, sizeof(refspec));
+	refspec.dst = (char *)refname;
+	if (!remote_find_tracking(states->remote, &refspec)) {
+		path_list_append(skip_prefix(refspec.src, "refs/heads/"),
+			&states->tracked);
+	}
+
+	return 0;
+}
+
 static int show(int argc, const char **argv)
 {
 	int no_query = 0, result = 0;
@@ -494,10 +510,12 @@ static int show(int argc, const char **argv)
 			strbuf_release(&buf);
 			show_list("  Stale tracking branch%s (use 'git remote "
 				"prune')", &states.stale);
-			show_list("  Tracked remote branch%s",
-				&states.tracked);
 		}
 
+		if (no_query)
+			for_each_ref(append_ref_to_tracked_list, &states);
+		show_list("  Tracked remote branch%s", &states.tracked);
+
 		if (states.remote->push_refspec_nr) {
 			printf("  Local branch%s pushed with 'git push'\n   ",
 				states.remote->push_refspec_nr > 1 ?
diff --git a/t/t5505-remote.sh b/t/t5505-remote.sh
index c27cfad..fbf0d30 100755
--- a/t/t5505-remote.sh
+++ b/t/t5505-remote.sh
@@ -143,6 +143,8 @@ cat > test/expect << EOF
   URL: $(pwd)/one/.git
   Remote branch merged with 'git pull' while on branch master
     master
+  Tracked remote branches
+    master side
   Local branches pushed with 'git push'
     master:upstream +refs/tags/lastbackup
 EOF

Re: [PATCH 0/4] remote show/prune improvement

[Jakub Narebski]

Olivier Marin <dkr+ml.git@free.fr> writes:

> 
>   [1/4] remote show: fix the -n option
>   [2/4] builtin-remote: split show_or_prune() in two separate functions.
>   [3/4] remote prune: print the list of pruned branches
>   [4/4] remote show: list tracked remote branches with -n.
> 
>  Documentation/git-remote.txt |    9 +--
>  builtin-remote.c             |  160 ++++++++++++++++++++++++++++++------------

I like this series... but the [4/4] lacks documentation (all other
patches update documentation).

-- 
Jakub Narebski
Poland
ShadeHawk on #git

Re: [PATCH 0/4] remote show/prune improvement

[Olivier Marin]

Jakub Narebski a écrit :
> Olivier Marin <dkr+ml.git@free.fr> writes:
> 
>>   [1/4] remote show: fix the -n option
>>   [2/4] builtin-remote: split show_or_prune() in two separate functions.
>>   [3/4] remote prune: print the list of pruned branches
>>   [4/4] remote show: list tracked remote branches with -n.
>>
>>  Documentation/git-remote.txt |    9 +--
>>  builtin-remote.c             |  160 ++++++++++++++++++++++++++++++------------
> 
> I like this series... but the [4/4] lacks documentation (all other
> patches update documentation).
> 

I'm not sure, it's a minor change. Perhaps, I can squashed it in 1/4 instead.

What do you think?

Olivier.

Re: [PATCH 0/4] remote show/prune improvement

[Jakub Narebski]

Dnia wtorek 10. czerwca 2008 18:10, Olivier Marin napisał:
> Jakub Narebski a écrit :
>> Olivier Marin <dkr+ml.git@free.fr> writes:
>> 
>>>   [1/4] remote show: fix the -n option
>>>   [2/4] builtin-remote: split show_or_prune() in two separate
>>>         functions. 
>>>   [3/4] remote prune: print the list of pruned branches
>>>   [4/4] remote show: list tracked remote branches with -n.
>> 
>> I like this series... but the [4/4] lacks documentation (all other
>> patches update documentation).

Ah, sorry, my mistake.  It looks like [4/4] is just improvement
to [1/4], which is documented. 
 
> I'm not sure, it's a minor change. Perhaps, I can squashed it in
> 1/4 instead. 
> 
> What do you think?

Perhaps it could be, but this is not strictly necessary.

After reading patches a bit more carefully, I think that the features
are documented well enough, and any Documentation (and patches) 
improvements are not necessary, and further changes can happen "in 
tree".


In "[PATCH 1/4] remote show: fix the -n option" you have:
> --- a/Documentation/git-remote.txt
> +++ b/Documentation/git-remote.txt
[...]
> -'git-remote' show <name>
> +'git-remote' show [-n] <name>

while in Documentation/git-remote.txt there is remainder of Perl
implementation

   'show'::

   Gives some information about the remote <name>.
   +
   With `-n` option, the remote heads are not queried first with
   `git ls-remote <name>`; cached information is used instead.

The information about using `git ls-remote <name>` is no longer fully
accurate in builtin version, and perhaps could be removed.


In "[PATCH 3/4] remote prune: print the list of pruned branches":
> --- a/Documentation/git-remote.txt
> +++ b/Documentation/git-remote.txt
[...]
> -'git-remote' prune <name>
> +'git-remote' prune [-n | --dry-run] <name>
[...]
> -With `-n` option, the remote heads are not confirmed first with `git
> -ls-remote <name>`; cached information is used instead.  Use with
> -caution.
> +With `--dry-run` option, report what branches will be pruned, but do
> +no actually prune them.

No `git ls-remote` is mentioned there, as it should be.

-- 
Jakub Narebski
Poland