git remote rm considered harmful?

git remote rm considered harmful?

[Jay Soffian]

Yesterday I was not paying attention to a repository I was in when I
used git remote to make a mirror:

$ git remote add --mirror bar git://foo/bar

I realized my mistake and removed the mirror:

$ git remote rm bar

Poof. All of my local branches and their reflogs were gone. Reproduce with:

# Setup a repository
mkdir foo && cd foo && git init
echo foo > foo && git add foo && git commit -m foo

# Make some branches for demonstration purposes
git co -b branch1
git co -b branch2
git co -b branch3
git co master
find .git/refs .git/logs
.git/refs
.git/refs/heads
.git/refs/heads/branch1
.git/refs/heads/branch2
.git/refs/heads/branch3
.git/refs/heads/master
.git/refs/tags
.git/logs
.git/logs/HEAD
.git/logs/refs
.git/logs/refs/heads
.git/logs/refs/heads/branch1
.git/logs/refs/heads/branch2
.git/logs/refs/heads/branch3
.git/logs/refs/heads/master

# Setup a remote
git remote add --mirror bar git://foo/bar

# Oops, didn't mean to do that, remove the repo
git remote rm bar # poof!
find .git/refs .git/logs
.git/refs
.git/refs/heads
.git/refs/tags
.git/logs
.git/logs/HEAD
.git/logs/refs
.git/logs/refs/heads

I just realized that at least the HEAD log is still there. I missed
that originally since:

git reflog
fatal: bad default revision 'HEAD'

(Separate issue, but shouldn't "git reflog" work even if .git/HEAD is invalid?)

Anyway, it would seem to me it should be harder to remove local refs.
This one was somewhat painful to recover from. Not sure what better
behavior would be: should it be harder to do "remote add --mirror" on
a repository with content, should "remote rm" on mirrored repository
require a --force switch with a stern warning first, or...?

Thoughts?

j.

[PATCH] builtin-remote: make rm operation safer in mirrored repository

[Jay Soffian]

"git remote rm <repo>" is happy to remove non-remote branches (and their
reflogs). This may be okay if the repository truely is a mirror, but if the
user had done "git remote add --mirror <repo>" by accident and was just
undoing their mistake, then they are left in a situation that is difficult to
recover from.

After this commit, "git remote rm" skips over non-remote branches and instead
advises the user on how to remove such branches using "git branch -d", which
itself has nice safety checks wrt to branch removal lacking from "git remote
rm".

Signed-off-by: Jay Soffian <jaysoffian@gmail.com>
---
On Sun, Feb 1, 2009 at 9:52 AM, Jay Soffian <jaysoffian@gmail.com> wrote:
> Anyway, it would seem to me it should be harder to remove local refs.
> This one was somewhat painful to recover from. Not sure what better
> behavior would be: should it be harder to do "remote add --mirror" on
> a repository with content, should "remote rm" on mirrored repository
> require a --force switch with a stern warning first, or...?

Perhaps something like this?

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

diff --git a/builtin-remote.c b/builtin-remote.c
index abc8dd8..2629bc5 100644
--- a/builtin-remote.c
+++ b/builtin-remote.c
@@ -310,6 +310,13 @@ static int add_branch_for_removal(const char *refname,
 	struct string_list_item *item;
 	struct known_remote *kr;
 
+	/* don't delete non-remote branches */
+	if (prefixcmp(refname, "refs/remotes")) {
+		warning("not removing non-remote branch; use git branch -d %s to remove",
+			abbrev_branch(refname));
+		return 0;
+	}
+
 	memset(&refspec, 0, sizeof(refspec));
 	refspec.dst = (char *)refname;
 	if (remote_find_tracking(branches->remote, &refspec))
-- 
1.6.1.224.gb56c

Re: [PATCH] builtin-remote: make rm operation safer in mirrored repository

[Jeff King]

On Sun, Feb 01, 2009 at 10:48:29AM -0500, Jay Soffian wrote:

> "git remote rm <repo>" is happy to remove non-remote branches (and their
> reflogs). This may be okay if the repository truely is a mirror, but if the
> user had done "git remote add --mirror <repo>" by accident and was just
> undoing their mistake, then they are left in a situation that is difficult to
> recover from.
> 
> After this commit, "git remote rm" skips over non-remote branches and instead
> advises the user on how to remove such branches using "git branch -d", which
> itself has nice safety checks wrt to branch removal lacking from "git remote
> rm".

I think this is sensible. The point of "git remote rm" removing tracking
branches is just to clean up cruft in a repository that has otherwise
interesting stuff. Blowing away a mirror implies getting rid of all
refs. At which point I have to wonder why you would want to do that
versus just removing the repo entirely. I.e., the common use case for
"git remote rm" on a mirror would seem to be "oops, I did this wrong"
and not "I really want to get rid of all my refs".

So I think a safety valve is reasonable.  I wonder if would be simpler
still to just not delete _any_ refs for a mirrored remote. On the other
hand, your safety valve also protects against unusual refspecs that
might touch refs not in refs/remotes (e.g., if I didn't use
remote.foo.mirror, but I had a manually-written refspec that touched
some subset of refs/heads/). In that case, your safety valve makes more
sense.

>  builtin-remote.c |    7 +++++++
>  1 files changed, 7 insertions(+), 0 deletions(-)

Tests?

-Peff

Re: [PATCH] builtin-remote: make rm operation safer in mirrored repository

[Jay Soffian]

On Mon, Feb 2, 2009 at 8:29 AM, Jeff King <peff@peff.net> wrote:
> In that case, your safety valve makes more
> sense.

Thank you for the review.

>>  builtin-remote.c |    7 +++++++
>>  1 files changed, 7 insertions(+), 0 deletions(-)
>
> Tests?

Yep, will resend w/tests.

j.

[PATCH] builtin-remote: make rm operation safer in mirrored repository

[Jay Soffian]

"git remote rm <repo>" is happy to remove non-remote branches (and their
reflogs). This may be okay if the repository truely is a mirror, but if the
user had done "git remote add --mirror <repo>" by accident and was just
undoing their mistake, then they are left in a situation that is difficult to
recover from.

After this commit, "git remote rm" skips over non-remote branches and instead
advises the user on how to remove such branches using "git branch -d", which
itself has nice safety checks wrt to branch removal lacking from "git remote
rm".

Signed-off-by: Jay Soffian <jaysoffian@gmail.com>
---
This version adds a test case. I also noticed that the check I'd added to
add_branch_for_removal() was generating spurious warnings because I'd added it
in the wrong place; this version moves the check below the
remote_find_tracking() checks.

 builtin-remote.c  |    7 +++++++
 t/t5505-remote.sh |    9 +++++++++
 2 files changed, 16 insertions(+), 0 deletions(-)

diff --git a/builtin-remote.c b/builtin-remote.c
index abc8dd8..571caff 100644
--- a/builtin-remote.c
+++ b/builtin-remote.c
@@ -323,6 +323,13 @@ static int add_branch_for_removal(const char *refname,
 			return 0;
 	}
 
+	/* don't delete non-remote branches */
+	if (prefixcmp(refname, "refs/remotes")) {
+		warning("not removing non-remote branch; use git branch -d %s to remove",
+			abbrev_branch(refname));
+		return 0;
+	}
+
 	/* make sure that symrefs are deleted */
 	if (flags & REF_ISSYMREF)
 		return unlink(git_path("%s", refname));
diff --git a/t/t5505-remote.sh b/t/t5505-remote.sh
index 1f59960..80d40ea 100755
--- a/t/t5505-remote.sh
+++ b/t/t5505-remote.sh
@@ -107,6 +107,15 @@ test_expect_success 'remove remote' '
 )
 '
 
+test_expect_success 'remove remote protects non-remote branches' '
+(
+	cd test &&
+	git config --add remote.oops.fetch "+refs/*:refs/*" &&
+	git remote rm oops 2>stderr &&
+	grep "not removing non-remote branch.* master " stderr
+)
+'
+
 cat > test/expect << EOF
 * remote origin
   URL: $(pwd)/one
-- 
1.6.1.2.308.gd57ba

Re: [PATCH] builtin-remote: make rm operation safer in mirrored repository

[Jeff King]

On Mon, Feb 02, 2009 at 01:40:14PM -0500, Jay Soffian wrote:

> This version adds a test case. I also noticed that the check I'd added to
> add_branch_for_removal() was generating spurious warnings because I'd added it
> in the wrong place; this version moves the check below the
> remote_find_tracking() checks.

This version looks fine to me, and I would be OK with it being applied.

However, I have one small nit. The output produces long lines with a lot
of repeated text (assuming you have multiple matched branches, which is
likely if you have a mirrored setup). So maybe it would be nicer to have
something like:

  warning: non-remote branches were not removed; you can delete them with:
          git branch -d master
          git branch -d next
          git branch -d topic

which is a little more obvious (to me, anyway), and allows you to cut
and paste if you really did want to delete them.

But this should be a pretty seldom seen message, so it might not matter
much.  I just thought I would throw it out there.

-Peff

Re: [PATCH] builtin-remote: make rm operation safer in mirrored repository

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Mon, Feb 02, 2009 at 01:40:14PM -0500, Jay Soffian wrote:
>
>> This version adds a test case. I also noticed that the check I'd added to
>> add_branch_for_removal() was generating spurious warnings because I'd added it
>> in the wrong place; this version moves the check below the
>> remote_find_tracking() checks.
>
> This version looks fine to me, and I would be OK with it being applied.
>
> However, I have one small nit. The output produces long lines with a lot
> of repeated text (assuming you have multiple matched branches, which is
> likely if you have a mirrored setup). So maybe it would be nicer to have
> something like:
>
>   warning: non-remote branches were not removed; you can delete them with:
>           git branch -d master
>           git branch -d next
>           git branch -d topic
>
> which is a little more obvious (to me, anyway), and allows you to cut
> and paste if you really did want to delete them.

Thanks for a review, and I actually shared that exact nit when I first
read the patch.  It would be a very good change to collect them in a list
and show a single warning at the end (I do not have particular preference
about the cut & paste-ability either way myself).
 

Re: [PATCH] builtin-remote: make rm operation safer in mirrored repository

[Jay Soffian]

On Tue, Feb 3, 2009 at 2:54 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Jeff King <peff@peff.net> writes:
>> However, I have one small nit. The output produces long lines with a lot
>> of repeated text (assuming you have multiple matched branches, which is
>> likely if you have a mirrored setup). So maybe it would be nicer to have
>> something like:
>>
>>   warning: non-remote branches were not removed; you can delete them with:
>>           git branch -d master
>>           git branch -d next
>>           git branch -d topic
>>
>> which is a little more obvious (to me, anyway), and allows you to cut
>> and paste if you really did want to delete them.
>
> Thanks for a review, and I actually shared that exact nit when I first
> read the patch.  It would be a very good change to collect them in a list
> and show a single warning at the end (I do not have particular preference
> about the cut & paste-ability either way myself).

This is a tough crowd. I'll see what I can do.

j.

Re: [PATCH] builtin-remote: make rm operation safer in mirrored repository

[Johannes Schindelin]

Hi,

On Tue, 3 Feb 2009, Jay Soffian wrote:

> On Tue, Feb 3, 2009 at 2:54 AM, Junio C Hamano <gitster@pobox.com> wrote:
> > Jeff King <peff@peff.net> writes:
> >> However, I have one small nit. The output produces long lines with a lot
> >> of repeated text (assuming you have multiple matched branches, which is
> >> likely if you have a mirrored setup). So maybe it would be nicer to have
> >> something like:
> >>
> >>   warning: non-remote branches were not removed; you can delete them with:
> >>           git branch -d master
> >>           git branch -d next
> >>           git branch -d topic
> >>
> >> which is a little more obvious (to me, anyway), and allows you to cut
> >> and paste if you really did want to delete them.
> >
> > Thanks for a review, and I actually shared that exact nit when I first
> > read the patch.  It would be a very good change to collect them in a list
> > and show a single warning at the end (I do not have particular preference
> > about the cut & paste-ability either way myself).
> 
> This is a tough crowd. I'll see what I can do.

We polish it until it shines! :-)

Thank you for you efforts,
Dscho

[PATCH 1/2] builtin-remote: make rm() use properly named variable to hold return value

[Jay Soffian]

"i" is a loop counter and should not be used to hold a return value; use
"result" instead which is consistent with the rest of builtin-remote.c.

Signed-off-by: Jay Soffian <jaysoffian@gmail.com>
---
I'm not sure this minor change needed to be broken out into its own commit,
so squash with 2/2 if that's appropriate.

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

diff --git a/builtin-remote.c b/builtin-remote.c
index abc8dd8..21885fb 100644
--- a/builtin-remote.c
+++ b/builtin-remote.c
@@ -543,7 +543,7 @@ static int rm(int argc, const char **argv)
 	struct known_remotes known_remotes = { NULL, NULL };
 	struct string_list branches = { NULL, 0, 0, 1 };
 	struct branches_for_remote cb_data = { NULL, &branches, &known_remotes };
-	int i;
+	int i, result;
 
 	if (argc != 2)
 		usage_with_options(builtin_remote_usage, options);
@@ -583,14 +583,14 @@ static int rm(int argc, const char **argv)
 	 * refs, which are invalidated when deleting a branch.
 	 */
 	cb_data.remote = remote;
-	i = for_each_ref(add_branch_for_removal, &cb_data);
+	result = for_each_ref(add_branch_for_removal, &cb_data);
 	strbuf_release(&buf);
 
-	if (!i)
-		i = remove_branches(&branches);
+	if (!result)
+		result = remove_branches(&branches);
 	string_list_clear(&branches, 1);
 
-	return i;
+	return result;
 }
 
 static void show_list(const char *title, struct string_list *list,
-- 
1.6.1.2.311.g2d7f3

Re: [PATCH 1/2] builtin-remote: make rm() use properly named variable to hold return value

[Jeff King]

On Tue, Feb 03, 2009 at 12:51:12PM -0500, Jay Soffian wrote:

> "i" is a loop counter and should not be used to hold a return value; use
> "result" instead which is consistent with the rest of builtin-remote.c.
> 
> Signed-off-by: Jay Soffian <jaysoffian@gmail.com>
> ---
> I'm not sure this minor change needed to be broken out into its own commit,
> so squash with 2/2 if that's appropriate.

Looks obviously correct to me...

-Peff

[PATCH 2/2] builtin-remote: make rm operation safer in mirrored repository

[Jay Soffian]

"git remote rm <repo>" is happy to remove non-remote branches (and their
reflogs). This may be okay if the repository truely is a mirror, but if the
user had done "git remote add --mirror <repo>" by accident and was just
undoing their mistake, then they are left in a situation that is difficult to
recover from.

After this commit, "git remote rm" skips over non-remote branches and instead
advises the user on how to remove such branches using "git branch -d", which
itself has nice safety checks wrt to branch removal lacking from "git remote
rm".

Signed-off-by: Jay Soffian <jaysoffian@gmail.com>
---

On Tue, Feb 3, 2009 at 2:54 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Jeff King <peff@peff.net> writes:
>> However, I have one small nit. The output produces long lines with a lot
>> of repeated text (assuming you have multiple matched branches, which is
>> likely if you have a mirrored setup). So maybe it would be nicer to have
>> something like:
>>
>>   warning: non-remote branches were not removed; you can delete them with:
>>           git branch -d master
>>           git branch -d next
>>           git branch -d topic
>>
>> which is a little more obvious (to me, anyway), and allows you to cut
>> and paste if you really did want to delete them.
>
> Thanks for a review, and I actually shared that exact nit when I first
> read the patch.  It would be a very good change to collect them in a list
> and show a single warning at the end (I do not have particular preference
> about the cut & paste-ability either way myself).

So much work for what seemed such a minor change. :) I hope this version is
well-polished enough.

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

diff --git a/builtin-remote.c b/builtin-remote.c
index 21885fb..ae1eed4 100644
--- a/builtin-remote.c
+++ b/builtin-remote.c
@@ -298,7 +298,7 @@ static int add_known_remote(struct remote *remote, void *cb_data)
 
 struct branches_for_remote {
 	struct remote *remote;
-	struct string_list *branches;
+	struct string_list *branches, *skipped;
 	struct known_remotes *keep;
 };
 
@@ -323,6 +323,14 @@ static int add_branch_for_removal(const char *refname,
 			return 0;
 	}
 
+	/* don't delete non-remote branches */
+	if (prefixcmp(refname, "refs/remotes")) {
+		if (!prefixcmp(refname, "refs/heads/"))
+			string_list_append(abbrev_branch(refname),
+					   branches->skipped);
+		return 0;
+	}
+
 	/* make sure that symrefs are deleted */
 	if (flags & REF_ISSYMREF)
 		return unlink(git_path("%s", refname));
@@ -542,7 +550,10 @@ static int rm(int argc, const char **argv)
 	struct strbuf buf = STRBUF_INIT;
 	struct known_remotes known_remotes = { NULL, NULL };
 	struct string_list branches = { NULL, 0, 0, 1 };
-	struct branches_for_remote cb_data = { NULL, &branches, &known_remotes };
+	struct string_list skipped = { NULL, 0, 0, 1 };
+	struct branches_for_remote cb_data = {
+		NULL, &branches, &skipped, &known_remotes
+	};
 	int i, result;
 
 	if (argc != 2)
@@ -590,6 +601,18 @@ static int rm(int argc, const char **argv)
 		result = remove_branches(&branches);
 	string_list_clear(&branches, 1);
 
+	if (skipped.nr) {
+		fprintf(stderr, skipped.nr == 1 ?
+			"Note: A non-remote branch was not removed; "
+			"to delete it, use:\n" :
+			"Note: Non-remote branches were not removed; "
+			"to delete them, use:\n");
+		for (i = 0; i < skipped.nr; i++)
+			fprintf(stderr, "  git branch -d %s\n",
+				skipped.items[i].string);
+	}
+	string_list_clear(&skipped, 0);
+
 	return result;
 }
 
diff --git a/t/t5505-remote.sh b/t/t5505-remote.sh
index 1f59960..bc5b7ce 100755
--- a/t/t5505-remote.sh
+++ b/t/t5505-remote.sh
@@ -107,6 +107,32 @@ test_expect_success 'remove remote' '
 )
 '
 
+test_expect_success 'remove remote protects non-remote branches' '
+(
+	cd test &&
+	(cat >expect1 <<EOF
+Note: A non-remote branch was not removed; to delete it, use:
+  git branch -d master
+EOF
+    cat >expect2 <<EOF
+Note: Non-remote branches were not removed; to delete them, use:
+  git branch -d foobranch
+  git branch -d master
+EOF
+) &&
+	git tag footag
+	git config --add remote.oops.fetch "+refs/*:refs/*" &&
+	git remote rm oops 2>actual1 &&
+	git branch foobranch &&
+	git config --add remote.oops.fetch "+refs/*:refs/*" &&
+	git remote rm oops 2>actual2 &&
+	git branch -d foobranch &&
+	git tag -d footag &&
+	test_cmp expect1 actual1 &&
+	test_cmp expect2 actual2
+)
+'
+
 cat > test/expect << EOF
 * remote origin
   URL: $(pwd)/one
-- 
1.6.1.2.311.g2d7f3

Re: [PATCH 2/2] builtin-remote: make rm operation safer in mirrored repository

[Jeff King]

On Tue, Feb 03, 2009 at 12:51:13PM -0500, Jay Soffian wrote:

> So much work for what seemed such a minor change. :) I hope this version is
> well-polished enough.

The more minor the change, the more major the comments. :) Thanks for
sticking with it. This version looks good to me except for one thing:

> +	/* don't delete non-remote branches */
> +	if (prefixcmp(refname, "refs/remotes")) {
> +		if (!prefixcmp(refname, "refs/heads/"))
> +			string_list_append(abbrev_branch(refname),
> +					   branches->skipped);
> +		return 0;
> +	}

Why does this version introduce the "only skip refs/heads/" check?
Shouldn't we also protect other random refs (or if not, shouldn't the
commit message explain why not)?

> +	if (skipped.nr) {
> +		fprintf(stderr, skipped.nr == 1 ?
> +			"Note: A non-remote branch was not removed; "
> +			"to delete it, use:\n" :
> +			"Note: Non-remote branches were not removed; "
> +			"to delete them, use:\n");

Ooh, proper pluralization. Classy.

-Peff

Re: [PATCH 2/2] builtin-remote: make rm operation safer in mirrored repository

[Jay Soffian]

On Wed, Feb 4, 2009 at 10:42 AM, Jeff King <peff@peff.net> wrote:
>> +     /* don't delete non-remote branches */
>> +     if (prefixcmp(refname, "refs/remotes")) {
>> +             if (!prefixcmp(refname, "refs/heads/"))
>> +                     string_list_append(abbrev_branch(refname),
>> +                                        branches->skipped);
>> +             return 0;
>> +     }
>
> Why does this version introduce the "only skip refs/heads/" check?
> Shouldn't we also protect other random refs (or if not, shouldn't the
> commit message explain why not)?

Note that we do protect refs, but we only emit messages about those
refs which are obviously branches. Frankly, I wasn't sure what other
kinds of refs there might be, so wasn't sure what an appropriate
message is for anything other than those under refs/heads.

In particular though, I noticed that w/o this check, I was emitting an
incorrect message about anything under refs/tags. I thought about
saying "and you can clean up these ignored tags like so", but that is
likely to emit a huge number of messages, so I thought it best just to
silently ignore non-remote non-branch refs. Perhaps I should better
explain that in a code comment.

Alternately, it could do something like:

Note: A non-remote branch was not removed; to delete it use:
   git branch -d ...

Note: Tags were not removed, to delete them use:
   git tag -d ...

Note: Some refs were ignored:
   refs/whoknows/whatthisis
   refs/whoknows/whatthiscouldbe

But that's getting a little insane me thinks.

j.

[PATCH] builtin-remote: make rm operation safer in mirrored repository

[Jay Soffian]

"git remote rm <repo>" is happy to remove non-remote refs (and their
reflogs in the case of branches). This may be okay if the repository truely is a mirror, but if the
user had done "git remote add --mirror <repo>" by accident and was just
undoing their mistake, then they are left in a situation that is difficult to
recover from.

After this commit, "git remote rm" skips over non-remote refs. The user is
advised on how remove branches using "git branch -d", which itself has nice
safety checks wrt to branch removal lacking from "git remote rm". Non-remote
non-branch refs are skipped silently.

Signed-off-by: Jay Soffian <jaysoffian@gmail.com>
---
On Wed, Feb 4, 2009 at 10:56 AM, Jay Soffian <jaysoffian@gmail.com> wrote:
> In particular though, I noticed that w/o this check, I was emitting an
> incorrect message about anything under refs/tags. I thought about
> saying "and you can clean up these ignored tags like so", but that is
> likely to emit a huge number of messages, so I thought it best just to
> silently ignore non-remote non-branch refs. Perhaps I should better
> explain that in a code comment.

I think I made what's going on clearer, assuming this is what we want to do.
Slightly revised the commit message.

This thread is getting sotra long, so here's the gmane threaded view for
easier following:

http://thread.gmane.org/gmane.comp.version-control.git/107982/

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

diff --git a/builtin-remote.c b/builtin-remote.c
index 21885fb..db18bcf 100644
--- a/builtin-remote.c
+++ b/builtin-remote.c
@@ -298,7 +298,7 @@ static int add_known_remote(struct remote *remote, void *cb_data)
 
 struct branches_for_remote {
 	struct remote *remote;
-	struct string_list *branches;
+	struct string_list *branches, *skipped;
 	struct known_remotes *keep;
 };
 
@@ -323,6 +323,16 @@ static int add_branch_for_removal(const char *refname,
 			return 0;
 	}
 
+	/* don't delete non-remote refs */
+	if (prefixcmp(refname, "refs/remotes")) {
+		/* advise user how to delete local branches */
+		if (!prefixcmp(refname, "refs/heads/"))
+			string_list_append(abbrev_branch(refname),
+					   branches->skipped);
+		/* silently skip over other non-remote refs */
+		return 0;
+	}
+
 	/* make sure that symrefs are deleted */
 	if (flags & REF_ISSYMREF)
 		return unlink(git_path("%s", refname));
@@ -542,7 +552,10 @@ static int rm(int argc, const char **argv)
 	struct strbuf buf = STRBUF_INIT;
 	struct known_remotes known_remotes = { NULL, NULL };
 	struct string_list branches = { NULL, 0, 0, 1 };
-	struct branches_for_remote cb_data = { NULL, &branches, &known_remotes };
+	struct string_list skipped = { NULL, 0, 0, 1 };
+	struct branches_for_remote cb_data = {
+		NULL, &branches, &skipped, &known_remotes
+	};
 	int i, result;
 
 	if (argc != 2)
@@ -590,6 +603,18 @@ static int rm(int argc, const char **argv)
 		result = remove_branches(&branches);
 	string_list_clear(&branches, 1);
 
+	if (skipped.nr) {
+		fprintf(stderr, skipped.nr == 1 ?
+			"Note: A non-remote branch was not removed; "
+			"to delete it, use:\n" :
+			"Note: Non-remote branches were not removed; "
+			"to delete them, use:\n");
+		for (i = 0; i < skipped.nr; i++)
+			fprintf(stderr, "  git branch -d %s\n",
+				skipped.items[i].string);
+	}
+	string_list_clear(&skipped, 0);
+
 	return result;
 }
 
diff --git a/t/t5505-remote.sh b/t/t5505-remote.sh
index 1f59960..bc5b7ce 100755
--- a/t/t5505-remote.sh
+++ b/t/t5505-remote.sh
@@ -107,6 +107,32 @@ test_expect_success 'remove remote' '
 )
 '
 
+test_expect_success 'remove remote protects non-remote branches' '
+(
+	cd test &&
+	(cat >expect1 <<EOF
+Note: A non-remote branch was not removed; to delete it, use:
+  git branch -d master
+EOF
+    cat >expect2 <<EOF
+Note: Non-remote branches were not removed; to delete them, use:
+  git branch -d foobranch
+  git branch -d master
+EOF
+) &&
+	git tag footag
+	git config --add remote.oops.fetch "+refs/*:refs/*" &&
+	git remote rm oops 2>actual1 &&
+	git branch foobranch &&
+	git config --add remote.oops.fetch "+refs/*:refs/*" &&
+	git remote rm oops 2>actual2 &&
+	git branch -d foobranch &&
+	git tag -d footag &&
+	test_cmp expect1 actual1 &&
+	test_cmp expect2 actual2
+)
+'
+
 cat > test/expect << EOF
 * remote origin
   URL: $(pwd)/one
-- 
1.6.1.2.322.g9a647

Re: [PATCH] builtin-remote: make rm operation safer in mirrored repository

[Jeff King]

On Wed, Feb 04, 2009 at 11:06:07AM -0500, Jay Soffian wrote:

> "git remote rm <repo>" is happy to remove non-remote refs (and their
> reflogs in the case of branches). This may be okay if the repository truely is a mirror, but if the
> user had done "git remote add --mirror <repo>" by accident and was just
> undoing their mistake, then they are left in a situation that is difficult to
> recover from.
> 
> After this commit, "git remote rm" skips over non-remote refs. The user is
> advised on how remove branches using "git branch -d", which itself has nice
> safety checks wrt to branch removal lacking from "git remote rm". Non-remote
> non-branch refs are skipped silently.
> 
> Signed-off-by: Jay Soffian <jaysoffian@gmail.com>

I like it.

Acked-by: Jeff King <peff@peff.net>

-Peff

Re: [PATCH 2/2] builtin-remote: make rm operation safer in mirrored repository

[Jeff King]

On Wed, Feb 04, 2009 at 10:56:06AM -0500, Jay Soffian wrote:

> On Wed, Feb 4, 2009 at 10:42 AM, Jeff King <peff@peff.net> wrote:
> >> +     /* don't delete non-remote branches */
> >> +     if (prefixcmp(refname, "refs/remotes")) {
> >> +             if (!prefixcmp(refname, "refs/heads/"))
> >> +                     string_list_append(abbrev_branch(refname),
> >> +                                        branches->skipped);
> >> +             return 0;
> >> +     }
> >
> > Why does this version introduce the "only skip refs/heads/" check?
> > Shouldn't we also protect other random refs (or if not, shouldn't the
> > commit message explain why not)?
> 
> Note that we do protect refs, but we only emit messages about those
> refs which are obviously branches. Frankly, I wasn't sure what other
> kinds of refs there might be, so wasn't sure what an appropriate
> message is for anything other than those under refs/heads.

Oh, right. I was just reading it wrong. Your new version with the two
comments makes it even more clear.

> Note: A non-remote branch was not removed; to delete it use:
>    git branch -d ...
> 
> Note: Tags were not removed, to delete them use:
>    git tag -d ...
>
> Note: Some refs were ignored:
>    refs/whoknows/whatthisis
>    refs/whoknows/whatthiscouldbe

Nah, I think deleting branches was the main intention. So we are
properly protecting everything now, but only warning about what you
_might_ have meant.

> But that's getting a little insane me thinks.

Agreed.

-Peff