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

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.

From: Olivier Marin <[email protected]>

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.

---

My MUA destroyed the previous patch! Sorry.

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

index e97dc09..e51d232 100644
@@ -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

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.

From: Olivier Marin <[email protected]>

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.

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

index c49f00f..cb9e282 100644
@@ -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   ",

index 0d7ed1f..c6a7bfb 100755
@@ -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

Hi,

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

Sorry for that, will do it next time.

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 ?

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

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.

Hi,

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).

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

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

Ciao,
Dscho

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".

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

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

Thanks for your comments,
Olivier.

Hi,

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

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

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

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?

OK, I will try that.

Olivier.

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.

Hi,

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

From: Olivier Marin <[email protected]>

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.

---
 builtin-remote.c  |   44 +++++++++++++++++++++++---------------------
 t/t5505-remote.sh |   17 +++++++++++++++++
 2 files changed, 40 insertions(+), 21 deletions(-)

index c49f00f..efe74c7 100644
@@ -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   ",

index 0d7ed1f..c6a7bfb 100755
@@ -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) &&

Hi,

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.

Hi,

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

Whatever,
Dscho

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.

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

---

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(-)

index 2641e20..9939c96 100644
@@ -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

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

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.

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.

From: Olivier Marin <[email protected]>

Junio C Hamano a écrit :

[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.

From: Olivier Marin <[email protected]>

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.

---
 Documentation/git-remote.txt |    2 +-
 builtin-remote.c             |   44 +++++++++++++++++++++--------------------
 t/t5505-remote.sh            |   17 ++++++++++++++++
 3 files changed, 41 insertions(+), 22 deletions(-)

index 782b055..7bd024e 100644
@@ -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]
 

index 0d7ed1f..c6a7bfb 100755
@@ -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

From: Olivier Marin <[email protected]>

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.

---
 builtin-remote.c |  101 +++++++++++++++++++++++++++++++++++------------------
 1 files changed, 67 insertions(+), 34 deletions(-)

index efe74c7..745a4ee 100644
@@ -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

From: Olivier Marin <[email protected]>

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.

---
 Documentation/git-remote.txt |    7 +++----
 builtin-remote.c             |   20 ++++++++++++++++----
 t/t5505-remote.sh            |   18 ++++++++++++++++++
 3 files changed, 37 insertions(+), 8 deletions(-)

index 7bd024e..345943a 100644
@@ -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'::
 

index 745a4ee..851bdde 100644
@@ -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 */

index c6a7bfb..c27cfad 100755
@@ -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

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

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.

From: Olivier Marin <[email protected]>

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

index 851bdde..de4a4f2 100644
@@ -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 ?

index c27cfad..ec5ea54 100755
@@ -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

From: Olivier Marin <[email protected]>

---

You are right. This version should fix this.

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

index 851bdde..d55d320 100644
@@ -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 ?

index c27cfad..fbf0d30 100755
@@ -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

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

Jakub Narebski
Poland
ShadeHawk on #git

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

What do you think?

Olivier.

Ah, sorry, my mistake. It looks like [4/4] is just improvement to [1/4], which is documented.

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:

[...]

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":

[...]

[...]

Jakub Narebski
Poland

...

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.

Hi,

Ah, that clarifies it.

Ciao,
Dscho

From: Olivier Marin <[email protected]>

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.

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

index e97dc09..e51d232 100644
@@ -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