git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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

From
Jay Soffian <jaysoffian@gmail.com>
Date
Feb 4, 2009, 16:06 UTC
Message-ID
<1233763567-6155-1-git-send-email-jaysoffian@gmail.com>
In-Reply-To
<76718490902040756m1f5c6f37o45865c51ad1a2e6d@mail.gmail.com>

"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:
Show 6 quoted lines
> 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
Previous: Jay SoffianNext: Jeff King
Message 14 of 17 in “git remote rm considered harmful?”
  1. Jay SoffianFeb 1, 2009
  2. builtin-remote: make rm operation safer in mirrored repositoryJay Soffian, Feb 1, 2009
  3. Jeff KingFeb 2, 2009
  4. Jay SoffianFeb 2, 2009
  5. builtin-remote: make rm operation safer in mirrored repositoryJay Soffian, Feb 2, 2009
  6. Jeff KingFeb 3, 2009
  7. Junio C HamanoFeb 3, 2009
  8. Jay SoffianFeb 3, 2009
  9. Johannes SchindelinFeb 3, 2009
  10. 1/2 builtin-remote: make rm() use properly named variable to hold return valueJay Soffian, Feb 3, 2009
  11. 2/2 builtin-remote: make rm operation safer in mirrored repositoryJay Soffian, Feb 3, 2009
  12. Jeff KingFeb 4, 2009
  13. Jay SoffianFeb 4, 2009
  14. builtin-remote: make rm operation safer in mirrored repositoryJay Soffian, Feb 4, 2009
  15. Jeff KingFeb 4, 2009
  16. Jeff KingFeb 4, 2009
  17. Jeff KingFeb 4, 2009

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.