{"thread":{"id":"17483","subject":"git remote rm considered harmful?","startedAt":"2009-02-01T14:52:40Z","lastAt":"2009-02-04T16:18:25Z","messageCount":17,"participants":["Jay Soffian","Jeff King","Junio C Hamano","Johannes Schindelin"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"102755","messageId":"76718490902010652w7a332550w5e0658a390c7dea9@mail.gmail.com","threadId":"17483","inReplyTo":null,"subject":"git remote rm considered harmful?","fromName":"Jay Soffian","fromEmail":"jaysoffian@gmail.com","sentAt":"2009-02-01T14:52:40Z","receivedAt":"2009-02-01T14:52:40Z","isPatch":false,"sender":{"key":"jaysoffian@gmail.com","avatar":"https://avatars.githubusercontent.com/u/155970?v=4"},"body":"Yesterday I was not paying attention to a repository I was in when I\nused git remote to make a mirror:\n\n$ git remote add --mirror bar git://foo/bar\n\nI realized my mistake and removed the mirror:\n\n$ git remote rm bar\n\nPoof. All of my local branches and their reflogs were gone. Reproduce with:\n\n# Setup a repository\nmkdir foo && cd foo && git init\necho foo > foo && git add foo && git commit -m foo\n\n# Make some branches for demonstration purposes\ngit co -b branch1\ngit co -b branch2\ngit co -b branch3\ngit co master\nfind .git/refs .git/logs\n.git/refs\n.git/refs/heads\n.git/refs/heads/branch1\n.git/refs/heads/branch2\n.git/refs/heads/branch3\n.git/refs/heads/master\n.git/refs/tags\n.git/logs\n.git/logs/HEAD\n.git/logs/refs\n.git/logs/refs/heads\n.git/logs/refs/heads/branch1\n.git/logs/refs/heads/branch2\n.git/logs/refs/heads/branch3\n.git/logs/refs/heads/master\n\n# Setup a remote\ngit remote add --mirror bar git://foo/bar\n\n# Oops, didn't mean to do that, remove the repo\ngit remote rm bar # poof!\nfind .git/refs .git/logs\n.git/refs\n.git/refs/heads\n.git/refs/tags\n.git/logs\n.git/logs/HEAD\n.git/logs/refs\n.git/logs/refs/heads\n\nI just realized that at least the HEAD log is still there. I missed\nthat originally since:\n\ngit reflog\nfatal: bad default revision 'HEAD'\n\n(Separate issue, but shouldn't \"git reflog\" work even if .git/HEAD is invalid?)\n\nAnyway, it would seem to me it should be harder to remove local refs.\nThis one was somewhat painful to recover from. Not sure what better\nbehavior would be: should it be harder to do \"remote add --mirror\" on\na repository with content, should \"remote rm\" on mirrored repository\nrequire a --force switch with a stern warning first, or...?\n\nThoughts?\n\nj.\n"},{"id":"102756","messageId":"1233503309-40144-1-git-send-email-jaysoffian@gmail.com","threadId":"17483","inReplyTo":"76718490902010652w7a332550w5e0658a390c7dea9@mail.gmail.com","subject":"[PATCH] builtin-remote: make rm operation safer in mirrored repository","fromName":"Jay Soffian","fromEmail":"jaysoffian@gmail.com","sentAt":"2009-02-01T15:48:29Z","receivedAt":"2009-02-01T15:48:29Z","isPatch":true,"sender":{"key":"jaysoffian@gmail.com","avatar":"https://avatars.githubusercontent.com/u/155970?v=4"},"body":"\"git remote rm <repo>\" is happy to remove non-remote branches (and their\nreflogs). This may be okay if the repository truely is a mirror, but if the\nuser had done \"git remote add --mirror <repo>\" by accident and was just\nundoing their mistake, then they are left in a situation that is difficult to\nrecover from.\n\nAfter this commit, \"git remote rm\" skips over non-remote branches and instead\nadvises the user on how to remove such branches using \"git branch -d\", which\nitself has nice safety checks wrt to branch removal lacking from \"git remote\nrm\".\n\nSigned-off-by: Jay Soffian <jaysoffian@gmail.com>\n---\nOn Sun, Feb 1, 2009 at 9:52 AM, Jay Soffian <jaysoffian@gmail.com> wrote:\n> Anyway, it would seem to me it should be harder to remove local refs.\n> This one was somewhat painful to recover from. Not sure what better\n> behavior would be: should it be harder to do \"remote add --mirror\" on\n> a repository with content, should \"remote rm\" on mirrored repository\n> require a --force switch with a stern warning first, or...?\n\nPerhaps something like this?\n\n builtin-remote.c |    7 +++++++\n 1 files changed, 7 insertions(+), 0 deletions(-)\n\ndiff --git a/builtin-remote.c b/builtin-remote.c\nindex abc8dd8..2629bc5 100644\n--- a/builtin-remote.c\n+++ b/builtin-remote.c\n@@ -310,6 +310,13 @@ static int add_branch_for_removal(const char *refname,\n \tstruct string_list_item *item;\n \tstruct known_remote *kr;\n \n+\t/* don't delete non-remote branches */\n+\tif (prefixcmp(refname, \"refs/remotes\")) {\n+\t\twarning(\"not removing non-remote branch; use git branch -d %s to remove\",\n+\t\t\tabbrev_branch(refname));\n+\t\treturn 0;\n+\t}\n+\n \tmemset(&refspec, 0, sizeof(refspec));\n \trefspec.dst = (char *)refname;\n \tif (remote_find_tracking(branches->remote, &refspec))\n-- \n1.6.1.224.gb56c\n"},{"id":"102857","messageId":"20090202132909.GD8487@sigio.peff.net","threadId":"17483","inReplyTo":"1233503309-40144-1-git-send-email-jaysoffian@gmail.com","subject":"Re: [PATCH] builtin-remote: make rm operation safer in mirrored repository","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2009-02-02T13:29:09Z","receivedAt":"2009-02-02T13:29:09Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Feb 01, 2009 at 10:48:29AM -0500, Jay Soffian wrote:\n\n> \"git remote rm <repo>\" is happy to remove non-remote branches (and their\n> reflogs). This may be okay if the repository truely is a mirror, but if the\n> user had done \"git remote add --mirror <repo>\" by accident and was just\n> undoing their mistake, then they are left in a situation that is difficult to\n> recover from.\n> \n> After this commit, \"git remote rm\" skips over non-remote branches and instead\n> advises the user on how to remove such branches using \"git branch -d\", which\n> itself has nice safety checks wrt to branch removal lacking from \"git remote\n> rm\".\n\nI think this is sensible. The point of \"git remote rm\" removing tracking\nbranches is just to clean up cruft in a repository that has otherwise\ninteresting stuff. Blowing away a mirror implies getting rid of all\nrefs. At which point I have to wonder why you would want to do that\nversus just removing the repo entirely. I.e., the common use case for\n\"git remote rm\" on a mirror would seem to be \"oops, I did this wrong\"\nand not \"I really want to get rid of all my refs\".\n\nSo I think a safety valve is reasonable.  I wonder if would be simpler\nstill to just not delete _any_ refs for a mirrored remote. On the other\nhand, your safety valve also protects against unusual refspecs that\nmight touch refs not in refs/remotes (e.g., if I didn't use\nremote.foo.mirror, but I had a manually-written refspec that touched\nsome subset of refs/heads/). In that case, your safety valve makes more\nsense.\n\n>  builtin-remote.c |    7 +++++++\n>  1 files changed, 7 insertions(+), 0 deletions(-)\n\nTests?\n\n-Peff\n"},{"id":"102858","messageId":"76718490902020536g6f4bcee2i76ee046a8dc7d46@mail.gmail.com","threadId":"17483","inReplyTo":"20090202132909.GD8487@sigio.peff.net","subject":"Re: [PATCH] builtin-remote: make rm operation safer in mirrored repository","fromName":"Jay Soffian","fromEmail":"jaysoffian@gmail.com","sentAt":"2009-02-02T13:36:15Z","receivedAt":"2009-02-02T13:36:15Z","isPatch":true,"sender":{"key":"jaysoffian@gmail.com","avatar":"https://avatars.githubusercontent.com/u/155970?v=4"},"body":"On Mon, Feb 2, 2009 at 8:29 AM, Jeff King <peff@peff.net> wrote:\n> In that case, your safety valve makes more\n> sense.\n\nThank you for the review.\n\n>>  builtin-remote.c |    7 +++++++\n>>  1 files changed, 7 insertions(+), 0 deletions(-)\n>\n> Tests?\n\nYep, will resend w/tests.\n\nj.\n"},{"id":"102878","messageId":"1233600014-82346-1-git-send-email-jaysoffian@gmail.com","threadId":"17483","inReplyTo":"76718490902020536g6f4bcee2i76ee046a8dc7d46@mail.gmail.com","subject":"[PATCH] builtin-remote: make rm operation safer in mirrored repository","fromName":"Jay Soffian","fromEmail":"jaysoffian@gmail.com","sentAt":"2009-02-02T18:40:14Z","receivedAt":"2009-02-02T18:40:14Z","isPatch":true,"sender":{"key":"jaysoffian@gmail.com","avatar":"https://avatars.githubusercontent.com/u/155970?v=4"},"body":"\"git remote rm <repo>\" is happy to remove non-remote branches (and their\nreflogs). This may be okay if the repository truely is a mirror, but if the\nuser had done \"git remote add --mirror <repo>\" by accident and was just\nundoing their mistake, then they are left in a situation that is difficult to\nrecover from.\n\nAfter this commit, \"git remote rm\" skips over non-remote branches and instead\nadvises the user on how to remove such branches using \"git branch -d\", which\nitself has nice safety checks wrt to branch removal lacking from \"git remote\nrm\".\n\nSigned-off-by: Jay Soffian <jaysoffian@gmail.com>\n---\nThis version adds a test case. I also noticed that the check I'd added to\nadd_branch_for_removal() was generating spurious warnings because I'd added it\nin the wrong place; this version moves the check below the\nremote_find_tracking() checks.\n\n builtin-remote.c  |    7 +++++++\n t/t5505-remote.sh |    9 +++++++++\n 2 files changed, 16 insertions(+), 0 deletions(-)\n\ndiff --git a/builtin-remote.c b/builtin-remote.c\nindex abc8dd8..571caff 100644\n--- a/builtin-remote.c\n+++ b/builtin-remote.c\n@@ -323,6 +323,13 @@ static int add_branch_for_removal(const char *refname,\n \t\t\treturn 0;\n \t}\n \n+\t/* don't delete non-remote branches */\n+\tif (prefixcmp(refname, \"refs/remotes\")) {\n+\t\twarning(\"not removing non-remote branch; use git branch -d %s to remove\",\n+\t\t\tabbrev_branch(refname));\n+\t\treturn 0;\n+\t}\n+\n \t/* make sure that symrefs are deleted */\n \tif (flags & REF_ISSYMREF)\n \t\treturn unlink(git_path(\"%s\", refname));\ndiff --git a/t/t5505-remote.sh b/t/t5505-remote.sh\nindex 1f59960..80d40ea 100755\n--- a/t/t5505-remote.sh\n+++ b/t/t5505-remote.sh\n@@ -107,6 +107,15 @@ test_expect_success 'remove remote' '\n )\n '\n \n+test_expect_success 'remove remote protects non-remote branches' '\n+(\n+\tcd test &&\n+\tgit config --add remote.oops.fetch \"+refs/*:refs/*\" &&\n+\tgit remote rm oops 2>stderr &&\n+\tgrep \"not removing non-remote branch.* master \" stderr\n+)\n+'\n+\n cat > test/expect << EOF\n * remote origin\n   URL: $(pwd)/one\n-- \n1.6.1.2.308.gd57ba\n"},{"id":"102933","messageId":"20090203072418.GD21367@sigill.intra.peff.net","threadId":"17483","inReplyTo":"1233600014-82346-1-git-send-email-jaysoffian@gmail.com","subject":"Re: [PATCH] builtin-remote: make rm operation safer in mirrored repository","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2009-02-03T07:24:18Z","receivedAt":"2009-02-03T07:24:18Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Feb 02, 2009 at 01:40:14PM -0500, Jay Soffian wrote:\n\n> This version adds a test case. I also noticed that the check I'd added to\n> add_branch_for_removal() was generating spurious warnings because I'd added it\n> in the wrong place; this version moves the check below the\n> remote_find_tracking() checks.\n\nThis version looks fine to me, and I would be OK with it being applied.\n\nHowever, I have one small nit. The output produces long lines with a lot\nof repeated text (assuming you have multiple matched branches, which is\nlikely if you have a mirrored setup). So maybe it would be nicer to have\nsomething like:\n\n  warning: non-remote branches were not removed; you can delete them with:\n          git branch -d master\n          git branch -d next\n          git branch -d topic\n\nwhich is a little more obvious (to me, anyway), and allows you to cut\nand paste if you really did want to delete them.\n\nBut this should be a pretty seldom seen message, so it might not matter\nmuch.  I just thought I would throw it out there.\n\n-Peff\n"},{"id":"102935","messageId":"7vy6woc5vv.fsf@gitster.siamese.dyndns.org","threadId":"17483","inReplyTo":"20090203072418.GD21367@sigill.intra.peff.net","subject":"Re: [PATCH] builtin-remote: make rm operation safer in mirrored repository","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-02-03T07:54:28Z","receivedAt":"2009-02-03T07:54:28Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> On Mon, Feb 02, 2009 at 01:40:14PM -0500, Jay Soffian wrote:\n>\n>> This version adds a test case. I also noticed that the check I'd added to\n>> add_branch_for_removal() was generating spurious warnings because I'd added it\n>> in the wrong place; this version moves the check below the\n>> remote_find_tracking() checks.\n>\n> This version looks fine to me, and I would be OK with it being applied.\n>\n> However, I have one small nit. The output produces long lines with a lot\n> of repeated text (assuming you have multiple matched branches, which is\n> likely if you have a mirrored setup). So maybe it would be nicer to have\n> something like:\n>\n>   warning: non-remote branches were not removed; you can delete them with:\n>           git branch -d master\n>           git branch -d next\n>           git branch -d topic\n>\n> which is a little more obvious (to me, anyway), and allows you to cut\n> and paste if you really did want to delete them.\n\nThanks for a review, and I actually shared that exact nit when I first\nread the patch.  It would be a very good change to collect them in a list\nand show a single warning at the end (I do not have particular preference\nabout the cut & paste-ability either way myself).\n \n"},{"id":"102965","messageId":"76718490902030638y36299191i1fcc2ab8646b9593@mail.gmail.com","threadId":"17483","inReplyTo":"7vy6woc5vv.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] builtin-remote: make rm operation safer in mirrored repository","fromName":"Jay Soffian","fromEmail":"jaysoffian@gmail.com","sentAt":"2009-02-03T14:38:55Z","receivedAt":"2009-02-03T14:38:55Z","isPatch":true,"sender":{"key":"jaysoffian@gmail.com","avatar":"https://avatars.githubusercontent.com/u/155970?v=4"},"body":"On Tue, Feb 3, 2009 at 2:54 AM, Junio C Hamano <gitster@pobox.com> wrote:\n> Jeff King <peff@peff.net> writes:\n>> However, I have one small nit. The output produces long lines with a lot\n>> of repeated text (assuming you have multiple matched branches, which is\n>> likely if you have a mirrored setup). So maybe it would be nicer to have\n>> something like:\n>>\n>>   warning: non-remote branches were not removed; you can delete them with:\n>>           git branch -d master\n>>           git branch -d next\n>>           git branch -d topic\n>>\n>> which is a little more obvious (to me, anyway), and allows you to cut\n>> and paste if you really did want to delete them.\n>\n> Thanks for a review, and I actually shared that exact nit when I first\n> read the patch.  It would be a very good change to collect them in a list\n> and show a single warning at the end (I do not have particular preference\n> about the cut & paste-ability either way myself).\n\nThis is a tough crowd. I'll see what I can do.\n\nj.\n"},{"id":"102967","messageId":"alpine.DEB.1.00.0902031553310.6573@intel-tinevez-2-302","threadId":"17483","inReplyTo":"76718490902030638y36299191i1fcc2ab8646b9593@mail.gmail.com","subject":"Re: [PATCH] builtin-remote: make rm operation safer in mirrored repository","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2009-02-03T14:53:54Z","receivedAt":"2009-02-03T14:53:54Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Tue, 3 Feb 2009, Jay Soffian wrote:\n\n> On Tue, Feb 3, 2009 at 2:54 AM, Junio C Hamano <gitster@pobox.com> wrote:\n> > Jeff King <peff@peff.net> writes:\n> >> However, I have one small nit. The output produces long lines with a lot\n> >> of repeated text (assuming you have multiple matched branches, which is\n> >> likely if you have a mirrored setup). So maybe it would be nicer to have\n> >> something like:\n> >>\n> >>   warning: non-remote branches were not removed; you can delete them with:\n> >>           git branch -d master\n> >>           git branch -d next\n> >>           git branch -d topic\n> >>\n> >> which is a little more obvious (to me, anyway), and allows you to cut\n> >> and paste if you really did want to delete them.\n> >\n> > Thanks for a review, and I actually shared that exact nit when I first\n> > read the patch.  It would be a very good change to collect them in a list\n> > and show a single warning at the end (I do not have particular preference\n> > about the cut & paste-ability either way myself).\n> \n> This is a tough crowd. I'll see what I can do.\n\nWe polish it until it shines! :-)\n\nThank you for you efforts,\nDscho\n"},{"id":"103001","messageId":"1233683473-87893-1-git-send-email-jaysoffian@gmail.com","threadId":"17483","inReplyTo":"76718490902030638y36299191i1fcc2ab8646b9593@mail.gmail.com","subject":"[PATCH 1/2] builtin-remote: make rm() use properly named variable to hold return value","fromName":"Jay Soffian","fromEmail":"jaysoffian@gmail.com","sentAt":"2009-02-03T17:51:12Z","receivedAt":"2009-02-03T17:51:12Z","isPatch":true,"sender":{"key":"jaysoffian@gmail.com","avatar":"https://avatars.githubusercontent.com/u/155970?v=4"},"body":"\"i\" is a loop counter and should not be used to hold a return value; use\n\"result\" instead which is consistent with the rest of builtin-remote.c.\n\nSigned-off-by: Jay Soffian <jaysoffian@gmail.com>\n---\nI'm not sure this minor change needed to be broken out into its own commit,\nso squash with 2/2 if that's appropriate.\n\n builtin-remote.c |   10 +++++-----\n 1 files changed, 5 insertions(+), 5 deletions(-)\n\ndiff --git a/builtin-remote.c b/builtin-remote.c\nindex abc8dd8..21885fb 100644\n--- a/builtin-remote.c\n+++ b/builtin-remote.c\n@@ -543,7 +543,7 @@ static int rm(int argc, const char **argv)\n \tstruct known_remotes known_remotes = { NULL, NULL };\n \tstruct string_list branches = { NULL, 0, 0, 1 };\n \tstruct branches_for_remote cb_data = { NULL, &branches, &known_remotes };\n-\tint i;\n+\tint i, result;\n \n \tif (argc != 2)\n \t\tusage_with_options(builtin_remote_usage, options);\n@@ -583,14 +583,14 @@ static int rm(int argc, const char **argv)\n \t * refs, which are invalidated when deleting a branch.\n \t */\n \tcb_data.remote = remote;\n-\ti = for_each_ref(add_branch_for_removal, &cb_data);\n+\tresult = for_each_ref(add_branch_for_removal, &cb_data);\n \tstrbuf_release(&buf);\n \n-\tif (!i)\n-\t\ti = remove_branches(&branches);\n+\tif (!result)\n+\t\tresult = remove_branches(&branches);\n \tstring_list_clear(&branches, 1);\n \n-\treturn i;\n+\treturn result;\n }\n \n static void show_list(const char *title, struct string_list *list,\n-- \n1.6.1.2.311.g2d7f3\n"},{"id":"103002","messageId":"1233683473-87893-2-git-send-email-jaysoffian@gmail.com","threadId":"17483","inReplyTo":"1233683473-87893-1-git-send-email-jaysoffian@gmail.com","subject":"[PATCH 2/2] builtin-remote: make rm operation safer in mirrored repository","fromName":"Jay Soffian","fromEmail":"jaysoffian@gmail.com","sentAt":"2009-02-03T17:51:13Z","receivedAt":"2009-02-03T17:51:13Z","isPatch":true,"sender":{"key":"jaysoffian@gmail.com","avatar":"https://avatars.githubusercontent.com/u/155970?v=4"},"body":"\"git remote rm <repo>\" is happy to remove non-remote branches (and their\nreflogs). This may be okay if the repository truely is a mirror, but if the\nuser had done \"git remote add --mirror <repo>\" by accident and was just\nundoing their mistake, then they are left in a situation that is difficult to\nrecover from.\n\nAfter this commit, \"git remote rm\" skips over non-remote branches and instead\nadvises the user on how to remove such branches using \"git branch -d\", which\nitself has nice safety checks wrt to branch removal lacking from \"git remote\nrm\".\n\nSigned-off-by: Jay Soffian <jaysoffian@gmail.com>\n---\n\nOn Tue, Feb 3, 2009 at 2:54 AM, Junio C Hamano <gitster@pobox.com> wrote:\n> Jeff King <peff@peff.net> writes:\n>> However, I have one small nit. The output produces long lines with a lot\n>> of repeated text (assuming you have multiple matched branches, which is\n>> likely if you have a mirrored setup). So maybe it would be nicer to have\n>> something like:\n>>\n>>   warning: non-remote branches were not removed; you can delete them with:\n>>           git branch -d master\n>>           git branch -d next\n>>           git branch -d topic\n>>\n>> which is a little more obvious (to me, anyway), and allows you to cut\n>> and paste if you really did want to delete them.\n>\n> Thanks for a review, and I actually shared that exact nit when I first\n> read the patch.  It would be a very good change to collect them in a list\n> and show a single warning at the end (I do not have particular preference\n> about the cut & paste-ability either way myself).\n\nSo much work for what seemed such a minor change. :) I hope this version is\nwell-polished enough.\n\n builtin-remote.c  |   27 +++++++++++++++++++++++++--\n t/t5505-remote.sh |   26 ++++++++++++++++++++++++++\n 2 files changed, 51 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin-remote.c b/builtin-remote.c\nindex 21885fb..ae1eed4 100644\n--- a/builtin-remote.c\n+++ b/builtin-remote.c\n@@ -298,7 +298,7 @@ static int add_known_remote(struct remote *remote, void *cb_data)\n \n struct branches_for_remote {\n \tstruct remote *remote;\n-\tstruct string_list *branches;\n+\tstruct string_list *branches, *skipped;\n \tstruct known_remotes *keep;\n };\n \n@@ -323,6 +323,14 @@ static int add_branch_for_removal(const char *refname,\n \t\t\treturn 0;\n \t}\n \n+\t/* don't delete non-remote branches */\n+\tif (prefixcmp(refname, \"refs/remotes\")) {\n+\t\tif (!prefixcmp(refname, \"refs/heads/\"))\n+\t\t\tstring_list_append(abbrev_branch(refname),\n+\t\t\t\t\t   branches->skipped);\n+\t\treturn 0;\n+\t}\n+\n \t/* make sure that symrefs are deleted */\n \tif (flags & REF_ISSYMREF)\n \t\treturn unlink(git_path(\"%s\", refname));\n@@ -542,7 +550,10 @@ static int rm(int argc, const char **argv)\n \tstruct strbuf buf = STRBUF_INIT;\n \tstruct known_remotes known_remotes = { NULL, NULL };\n \tstruct string_list branches = { NULL, 0, 0, 1 };\n-\tstruct branches_for_remote cb_data = { NULL, &branches, &known_remotes };\n+\tstruct string_list skipped = { NULL, 0, 0, 1 };\n+\tstruct branches_for_remote cb_data = {\n+\t\tNULL, &branches, &skipped, &known_remotes\n+\t};\n \tint i, result;\n \n \tif (argc != 2)\n@@ -590,6 +601,18 @@ static int rm(int argc, const char **argv)\n \t\tresult = remove_branches(&branches);\n \tstring_list_clear(&branches, 1);\n \n+\tif (skipped.nr) {\n+\t\tfprintf(stderr, skipped.nr == 1 ?\n+\t\t\t\"Note: A non-remote branch was not removed; \"\n+\t\t\t\"to delete it, use:\\n\" :\n+\t\t\t\"Note: Non-remote branches were not removed; \"\n+\t\t\t\"to delete them, use:\\n\");\n+\t\tfor (i = 0; i < skipped.nr; i++)\n+\t\t\tfprintf(stderr, \"  git branch -d %s\\n\",\n+\t\t\t\tskipped.items[i].string);\n+\t}\n+\tstring_list_clear(&skipped, 0);\n+\n \treturn result;\n }\n \ndiff --git a/t/t5505-remote.sh b/t/t5505-remote.sh\nindex 1f59960..bc5b7ce 100755\n--- a/t/t5505-remote.sh\n+++ b/t/t5505-remote.sh\n@@ -107,6 +107,32 @@ test_expect_success 'remove remote' '\n )\n '\n \n+test_expect_success 'remove remote protects non-remote branches' '\n+(\n+\tcd test &&\n+\t(cat >expect1 <<EOF\n+Note: A non-remote branch was not removed; to delete it, use:\n+  git branch -d master\n+EOF\n+    cat >expect2 <<EOF\n+Note: Non-remote branches were not removed; to delete them, use:\n+  git branch -d foobranch\n+  git branch -d master\n+EOF\n+) &&\n+\tgit tag footag\n+\tgit config --add remote.oops.fetch \"+refs/*:refs/*\" &&\n+\tgit remote rm oops 2>actual1 &&\n+\tgit branch foobranch &&\n+\tgit config --add remote.oops.fetch \"+refs/*:refs/*\" &&\n+\tgit remote rm oops 2>actual2 &&\n+\tgit branch -d foobranch &&\n+\tgit tag -d footag &&\n+\ttest_cmp expect1 actual1 &&\n+\ttest_cmp expect2 actual2\n+)\n+'\n+\n cat > test/expect << EOF\n * remote origin\n   URL: $(pwd)/one\n-- \n1.6.1.2.311.g2d7f3\n"},{"id":"103151","messageId":"20090204153446.GD6896@sigill.intra.peff.net","threadId":"17483","inReplyTo":"1233683473-87893-1-git-send-email-jaysoffian@gmail.com","subject":"Re: [PATCH 1/2] builtin-remote: make rm() use properly named variable to hold return value","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2009-02-04T15:34:46Z","receivedAt":"2009-02-04T15:34:46Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Feb 03, 2009 at 12:51:12PM -0500, Jay Soffian wrote:\n\n> \"i\" is a loop counter and should not be used to hold a return value; use\n> \"result\" instead which is consistent with the rest of builtin-remote.c.\n> \n> Signed-off-by: Jay Soffian <jaysoffian@gmail.com>\n> ---\n> I'm not sure this minor change needed to be broken out into its own commit,\n> so squash with 2/2 if that's appropriate.\n\nLooks obviously correct to me...\n\n-Peff\n"},{"id":"103154","messageId":"20090204154227.GE6896@sigill.intra.peff.net","threadId":"17483","inReplyTo":"1233683473-87893-2-git-send-email-jaysoffian@gmail.com","subject":"Re: [PATCH 2/2] builtin-remote: make rm operation safer in mirrored repository","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2009-02-04T15:42:27Z","receivedAt":"2009-02-04T15:42:27Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Feb 03, 2009 at 12:51:13PM -0500, Jay Soffian wrote:\n\n> So much work for what seemed such a minor change. :) I hope this version is\n> well-polished enough.\n\nThe more minor the change, the more major the comments. :) Thanks for\nsticking with it. This version looks good to me except for one thing:\n\n> +\t/* don't delete non-remote branches */\n> +\tif (prefixcmp(refname, \"refs/remotes\")) {\n> +\t\tif (!prefixcmp(refname, \"refs/heads/\"))\n> +\t\t\tstring_list_append(abbrev_branch(refname),\n> +\t\t\t\t\t   branches->skipped);\n> +\t\treturn 0;\n> +\t}\n\nWhy does this version introduce the \"only skip refs/heads/\" check?\nShouldn't we also protect other random refs (or if not, shouldn't the\ncommit message explain why not)?\n\n> +\tif (skipped.nr) {\n> +\t\tfprintf(stderr, skipped.nr == 1 ?\n> +\t\t\t\"Note: A non-remote branch was not removed; \"\n> +\t\t\t\"to delete it, use:\\n\" :\n> +\t\t\t\"Note: Non-remote branches were not removed; \"\n> +\t\t\t\"to delete them, use:\\n\");\n\nOoh, proper pluralization. Classy.\n\n-Peff\n"},{"id":"103157","messageId":"76718490902040756m1f5c6f37o45865c51ad1a2e6d@mail.gmail.com","threadId":"17483","inReplyTo":"20090204154227.GE6896@sigill.intra.peff.net","subject":"Re: [PATCH 2/2] builtin-remote: make rm operation safer in mirrored repository","fromName":"Jay Soffian","fromEmail":"jaysoffian@gmail.com","sentAt":"2009-02-04T15:56:06Z","receivedAt":"2009-02-04T15:56:06Z","isPatch":true,"sender":{"key":"jaysoffian@gmail.com","avatar":"https://avatars.githubusercontent.com/u/155970?v=4"},"body":"On Wed, Feb 4, 2009 at 10:42 AM, Jeff King <peff@peff.net> wrote:\n>> +     /* don't delete non-remote branches */\n>> +     if (prefixcmp(refname, \"refs/remotes\")) {\n>> +             if (!prefixcmp(refname, \"refs/heads/\"))\n>> +                     string_list_append(abbrev_branch(refname),\n>> +                                        branches->skipped);\n>> +             return 0;\n>> +     }\n>\n> Why does this version introduce the \"only skip refs/heads/\" check?\n> Shouldn't we also protect other random refs (or if not, shouldn't the\n> commit message explain why not)?\n\nNote that we do protect refs, but we only emit messages about those\nrefs which are obviously branches. Frankly, I wasn't sure what other\nkinds of refs there might be, so wasn't sure what an appropriate\nmessage is for anything other than those under refs/heads.\n\nIn particular though, I noticed that w/o this check, I was emitting an\nincorrect message about anything under refs/tags. I thought about\nsaying \"and you can clean up these ignored tags like so\", but that is\nlikely to emit a huge number of messages, so I thought it best just to\nsilently ignore non-remote non-branch refs. Perhaps I should better\nexplain that in a code comment.\n\nAlternately, it could do something like:\n\nNote: A non-remote branch was not removed; to delete it use:\n   git branch -d ...\n\nNote: Tags were not removed, to delete them use:\n   git tag -d ...\n\nNote: Some refs were ignored:\n   refs/whoknows/whatthisis\n   refs/whoknows/whatthiscouldbe\n\nBut that's getting a little insane me thinks.\n\nj.\n"},{"id":"103158","messageId":"1233763567-6155-1-git-send-email-jaysoffian@gmail.com","threadId":"17483","inReplyTo":"76718490902040756m1f5c6f37o45865c51ad1a2e6d@mail.gmail.com","subject":"[PATCH] builtin-remote: make rm operation safer in mirrored repository","fromName":"Jay Soffian","fromEmail":"jaysoffian@gmail.com","sentAt":"2009-02-04T16:06:07Z","receivedAt":"2009-02-04T16:06:07Z","isPatch":true,"sender":{"key":"jaysoffian@gmail.com","avatar":"https://avatars.githubusercontent.com/u/155970?v=4"},"body":"\"git remote rm <repo>\" is happy to remove non-remote refs (and their\nreflogs in the case of branches). This may be okay if the repository truely is a mirror, but if the\nuser had done \"git remote add --mirror <repo>\" by accident and was just\nundoing their mistake, then they are left in a situation that is difficult to\nrecover from.\n\nAfter this commit, \"git remote rm\" skips over non-remote refs. The user is\nadvised on how remove branches using \"git branch -d\", which itself has nice\nsafety checks wrt to branch removal lacking from \"git remote rm\". Non-remote\nnon-branch refs are skipped silently.\n\nSigned-off-by: Jay Soffian <jaysoffian@gmail.com>\n---\nOn Wed, Feb 4, 2009 at 10:56 AM, Jay Soffian <jaysoffian@gmail.com> wrote:\n> In particular though, I noticed that w/o this check, I was emitting an\n> incorrect message about anything under refs/tags. I thought about\n> saying \"and you can clean up these ignored tags like so\", but that is\n> likely to emit a huge number of messages, so I thought it best just to\n> silently ignore non-remote non-branch refs. Perhaps I should better\n> explain that in a code comment.\n\nI think I made what's going on clearer, assuming this is what we want to do.\nSlightly revised the commit message.\n\nThis thread is getting sotra long, so here's the gmane threaded view for\neasier following:\n\nhttp://thread.gmane.org/gmane.comp.version-control.git/107982/\n\n builtin-remote.c  |   29 +++++++++++++++++++++++++++--\n t/t5505-remote.sh |   26 ++++++++++++++++++++++++++\n 2 files changed, 53 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin-remote.c b/builtin-remote.c\nindex 21885fb..db18bcf 100644\n--- a/builtin-remote.c\n+++ b/builtin-remote.c\n@@ -298,7 +298,7 @@ static int add_known_remote(struct remote *remote, void *cb_data)\n \n struct branches_for_remote {\n \tstruct remote *remote;\n-\tstruct string_list *branches;\n+\tstruct string_list *branches, *skipped;\n \tstruct known_remotes *keep;\n };\n \n@@ -323,6 +323,16 @@ static int add_branch_for_removal(const char *refname,\n \t\t\treturn 0;\n \t}\n \n+\t/* don't delete non-remote refs */\n+\tif (prefixcmp(refname, \"refs/remotes\")) {\n+\t\t/* advise user how to delete local branches */\n+\t\tif (!prefixcmp(refname, \"refs/heads/\"))\n+\t\t\tstring_list_append(abbrev_branch(refname),\n+\t\t\t\t\t   branches->skipped);\n+\t\t/* silently skip over other non-remote refs */\n+\t\treturn 0;\n+\t}\n+\n \t/* make sure that symrefs are deleted */\n \tif (flags & REF_ISSYMREF)\n \t\treturn unlink(git_path(\"%s\", refname));\n@@ -542,7 +552,10 @@ static int rm(int argc, const char **argv)\n \tstruct strbuf buf = STRBUF_INIT;\n \tstruct known_remotes known_remotes = { NULL, NULL };\n \tstruct string_list branches = { NULL, 0, 0, 1 };\n-\tstruct branches_for_remote cb_data = { NULL, &branches, &known_remotes };\n+\tstruct string_list skipped = { NULL, 0, 0, 1 };\n+\tstruct branches_for_remote cb_data = {\n+\t\tNULL, &branches, &skipped, &known_remotes\n+\t};\n \tint i, result;\n \n \tif (argc != 2)\n@@ -590,6 +603,18 @@ static int rm(int argc, const char **argv)\n \t\tresult = remove_branches(&branches);\n \tstring_list_clear(&branches, 1);\n \n+\tif (skipped.nr) {\n+\t\tfprintf(stderr, skipped.nr == 1 ?\n+\t\t\t\"Note: A non-remote branch was not removed; \"\n+\t\t\t\"to delete it, use:\\n\" :\n+\t\t\t\"Note: Non-remote branches were not removed; \"\n+\t\t\t\"to delete them, use:\\n\");\n+\t\tfor (i = 0; i < skipped.nr; i++)\n+\t\t\tfprintf(stderr, \"  git branch -d %s\\n\",\n+\t\t\t\tskipped.items[i].string);\n+\t}\n+\tstring_list_clear(&skipped, 0);\n+\n \treturn result;\n }\n \ndiff --git a/t/t5505-remote.sh b/t/t5505-remote.sh\nindex 1f59960..bc5b7ce 100755\n--- a/t/t5505-remote.sh\n+++ b/t/t5505-remote.sh\n@@ -107,6 +107,32 @@ test_expect_success 'remove remote' '\n )\n '\n \n+test_expect_success 'remove remote protects non-remote branches' '\n+(\n+\tcd test &&\n+\t(cat >expect1 <<EOF\n+Note: A non-remote branch was not removed; to delete it, use:\n+  git branch -d master\n+EOF\n+    cat >expect2 <<EOF\n+Note: Non-remote branches were not removed; to delete them, use:\n+  git branch -d foobranch\n+  git branch -d master\n+EOF\n+) &&\n+\tgit tag footag\n+\tgit config --add remote.oops.fetch \"+refs/*:refs/*\" &&\n+\tgit remote rm oops 2>actual1 &&\n+\tgit branch foobranch &&\n+\tgit config --add remote.oops.fetch \"+refs/*:refs/*\" &&\n+\tgit remote rm oops 2>actual2 &&\n+\tgit branch -d foobranch &&\n+\tgit tag -d footag &&\n+\ttest_cmp expect1 actual1 &&\n+\ttest_cmp expect2 actual2\n+)\n+'\n+\n cat > test/expect << EOF\n * remote origin\n   URL: $(pwd)/one\n-- \n1.6.1.2.322.g9a647\n"},{"id":"103162","messageId":"20090204161654.GA9218@sigill.intra.peff.net","threadId":"17483","inReplyTo":"76718490902040756m1f5c6f37o45865c51ad1a2e6d@mail.gmail.com","subject":"Re: [PATCH 2/2] builtin-remote: make rm operation safer in mirrored repository","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2009-02-04T16:16:54Z","receivedAt":"2009-02-04T16:16:54Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Feb 04, 2009 at 10:56:06AM -0500, Jay Soffian wrote:\n\n> On Wed, Feb 4, 2009 at 10:42 AM, Jeff King <peff@peff.net> wrote:\n> >> +     /* don't delete non-remote branches */\n> >> +     if (prefixcmp(refname, \"refs/remotes\")) {\n> >> +             if (!prefixcmp(refname, \"refs/heads/\"))\n> >> +                     string_list_append(abbrev_branch(refname),\n> >> +                                        branches->skipped);\n> >> +             return 0;\n> >> +     }\n> >\n> > Why does this version introduce the \"only skip refs/heads/\" check?\n> > Shouldn't we also protect other random refs (or if not, shouldn't the\n> > commit message explain why not)?\n> \n> Note that we do protect refs, but we only emit messages about those\n> refs which are obviously branches. Frankly, I wasn't sure what other\n> kinds of refs there might be, so wasn't sure what an appropriate\n> message is for anything other than those under refs/heads.\n\nOh, right. I was just reading it wrong. Your new version with the two\ncomments makes it even more clear.\n\n> Note: A non-remote branch was not removed; to delete it use:\n>    git branch -d ...\n> \n> Note: Tags were not removed, to delete them use:\n>    git tag -d ...\n>\n> Note: Some refs were ignored:\n>    refs/whoknows/whatthisis\n>    refs/whoknows/whatthiscouldbe\n\nNah, I think deleting branches was the main intention. So we are\nproperly protecting everything now, but only warning about what you\n_might_ have meant.\n\n> But that's getting a little insane me thinks.\n\nAgreed.\n\n-Peff\n"},{"id":"103163","messageId":"20090204161825.GA9428@sigill.intra.peff.net","threadId":"17483","inReplyTo":"1233763567-6155-1-git-send-email-jaysoffian@gmail.com","subject":"Re: [PATCH] builtin-remote: make rm operation safer in mirrored repository","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2009-02-04T16:18:25Z","receivedAt":"2009-02-04T16:18:25Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Feb 04, 2009 at 11:06:07AM -0500, Jay Soffian wrote:\n\n> \"git remote rm <repo>\" is happy to remove non-remote refs (and their\n> reflogs in the case of branches). This may be okay if the repository truely is a mirror, but if the\n> user had done \"git remote add --mirror <repo>\" by accident and was just\n> undoing their mistake, then they are left in a situation that is difficult to\n> recover from.\n> \n> After this commit, \"git remote rm\" skips over non-remote refs. The user is\n> advised on how remove branches using \"git branch -d\", which itself has nice\n> safety checks wrt to branch removal lacking from \"git remote rm\". Non-remote\n> non-branch refs are skipped silently.\n> \n> Signed-off-by: Jay Soffian <jaysoffian@gmail.com>\n\nI like it.\n\nAcked-by: Jeff King <peff@peff.net>\n\n-Peff\n"}]}