{"thread":{"id":"41769","subject":"[PATCH][Outreachy] branch -D: allow - as abbreviation of @{-1}","startedAt":"2016-03-21T15:15:17Z","lastAt":"2016-03-21T19:27:12Z","messageCount":8,"participants":["Elena Petrashen","Matthieu Moy","Junio C Hamano","Eric Sunshine","Stefan Beller"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"281344","messageId":"1458573317-15532-1-git-send-email-elena.petrashen@gmail.com","threadId":"41769","inReplyTo":null,"subject":"[PATCH][Outreachy] branch -D: allow - as abbreviation of @{-1}","fromName":"Elena Petrashen","fromEmail":"elena.petrashen@gmail.com","sentAt":"2016-03-21T15:15:17Z","receivedAt":"2016-03-21T15:15:17Z","isPatch":true,"sender":{"key":"elena.petrashen@gmail.com","avatar":"https://gravatar.com/avatar/a2fd27bdf84eb2b7fc7b034a358aa6777fe86d5a9ac9339f122aea2b6b7689d4?d=mp&s=160"},"body":"Signed-off-by: Elena Petrashen \n\n---\nThis micro-patch is meant to allow “-“ as a short-hand for\n“@{-1} for branch -D (Cf. $gmane/230828):\n\n* git branch (-d | -D) is not supposed to accept any other\narguments except for branch name so it makes sense to replace\nthe argv[i] with @{-1}. We will not lose the opportunity to\nuse it for something different for other git branch uses if\nwe will decide it’s required.\n\n* the small allow_dash_as_prev_branch_alias function can be\nreused to teach git branch -m to allow “-“ as a short-hand for\n“@{-1} as well and possibly makes it easy to understand what’s\ngoing on in the code\n\n* if there’s no previous branch in the repository yet, a\nspecific warning message is given\n\nThank you! Looking forward to any feedback.\n\n Documentation/git-branch.txt | 5 ++++-\n builtin/branch.c | 18 +++++++++++++++---\n t/t3200-branch.sh | 10 ++++++++++\n 3 files changed, 29 insertions(+), 4 deletions(-)\n\ndiff --git a/Documentation/git-branch.txt b/Documentation/git-branch.txt\nindex 4a7037f..9f43665 100644\n--- a/Documentation/git-branch.txt\n+++ b/Documentation/git-branch.txt\n@@ -64,7 +64,10 @@ to happen.\n \n With a `-d` or `-D` option, `` will be deleted. You may\n specify more than one branch for deletion. If the branch currently\n-has a reflog then the reflog will also be deleted.\n+has a reflog then the reflog will also be deleted. \n+As a special case, the \"@{-N}\" syntax for the N-th last branch\n+deletes the specified branch. You may also specify - which is synonymous\n+with \"@{-1}\".\n \n Use `-r` together with `-d` to delete remote-tracking branches. Note, that it\n only makes sense to delete remote-tracking branches if they no longer exist\ndiff --git a/builtin/branch.c b/builtin/branch.c\nindex 7b45b6b..9614d18 100644\n--- a/builtin/branch.c\n+++ b/builtin/branch.c\n@@ -169,6 +169,8 @@ static int check_branch_commit(const char *branchname, const char *refname,\n \treturn 0;\n }\n \n+\n+\n static void delete_branch_config(const char *branchname)\n {\n \tstruct strbuf buf = STRBUF_INIT;\n@@ -178,6 +180,12 @@ static void delete_branch_config(const char *branchname)\n \tstrbuf_release(&buf);\n }\n \n+static void allow_dash_as_prev_branch_alias(const char **argv, int dash_position)\n+{\n+\tif (!strcmp(argv[dash_position], \"-\"))\n+\t\targv[dash_position] = \"@{-1}\";\n+}\n+\n static int delete_branches(int argc, const char **argv, int force, int kinds,\n \t\t\t int quiet)\n {\n@@ -210,10 +218,12 @@ static int delete_branches(int argc, const char **argv, int force, int kinds,\n \t\tif (!head_rev)\n \t\t\tdie(_(\"Couldn't look up commit object for HEAD\"));\n \t}\n+\n \tfor (i = 0; i < argc; i++, strbuf_release(&bname)) {\n \t\tconst char *target;\n \t\tint flags = 0;\n \n+\t\tallow_dash_as_prev_branch_alias(argv, i);\n \t\tstrbuf_branchname(&bname, argv[i]);\n \t\tif (kinds == FILTER_REFS_BRANCHES && !strcmp(head, bname.buf)) {\n \t\t\terror(_(\"Cannot delete the branch '%s' \"\n@@ -231,9 +241,11 @@ static int delete_branches(int argc, const char **argv, int force, int kinds,\n \t\t\t\t\t | RESOLVE_REF_ALLOW_BAD_NAME,\n \t\t\t\t\t sha1, &flags);\n \t\tif (!target) {\n-\t\t\terror(remote_branch\n-\t\t\t ? _(\"remote-tracking branch '%s' not found.\")\n-\t\t\t : _(\"branch '%s' not found.\"), bname.buf);\n+\t\t\terror(!strcmp(bname.buf, \"@{-1}\")\n+\t\t\t\t? _(\"There is no previous branch that could be referred to at the moment.\")\n+\t\t\t\t: remote_branch\n+\t\t\t\t\t? _(\"remote-tracking branch '%s' not found.\")\n+\t\t\t\t\t: _(\"branch '%s' not found.\"), bname.buf);\n \t\t\tret = 1;\n \t\t\tcontinue;\n \t\t}\ndiff --git a/t/t3200-branch.sh b/t/t3200-branch.sh\nindex a897248..d21369f 100755\n--- a/t/t3200-branch.sh\n+++ b/t/t3200-branch.sh\n@@ -403,6 +403,16 @@ test_expect_success 'test deleting branch without config' '\n \ttest_i18ncmp expect actual\n '\n \n+test_expect_success 'test deleting \"-\" deletes previous branch' '\n+\tgit checkout -b prev &&\n+\ttest_commit prev &&\n+\tgit checkout master &&\n+\tgit branch -D - >actual &&\n+\tsha1=$(git rev-parse prev | cut -c 1-7) &&\n+\techo \"Deleted branch prev (was $sha1).\" >expect &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success 'test --track without .fetch entries' '\n \tgit branch --track my8 &&\n \ttest \"$(git config branch.my8.remote)\" &&\n-- \n2.8.0.rc3.12.g047057b.dirty\n"},{"id":"281349","messageId":"vpqfuvju4km.fsf@anie.imag.fr","threadId":"41769","inReplyTo":"1458573317-15532-1-git-send-email-elena.petrashen@gmail.com","subject":"Re: [PATCH][Outreachy] branch -D: allow - as abbreviation of @{-1}","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2016-03-21T17:24:41Z","receivedAt":"2016-03-21T17:24:41Z","isPatch":true,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Hi, and welcome!\n\nElena Petrashen writes:\n\n> Signed-off-by: Elena Petrashen \n>\n> ---\n> This micro-patch is meant to allow “-“ as a short-hand for\n> “@{-1} for branch -D (Cf. $gmane/230828):\n\nIs it a good thing to do?\n\nI find \"git checkout -\" to be a very nice shortcut, because users very\noften want to get back to the branch they used to be before.\n\nBut I'm not sure how often people want to delete (force-delete according\nto your message) the branch they just come from. It might be less\ndangerous to give incentive to the user to spell the branch name\ncompletely to avoid mistake. As analogy, my shell knows \"cd -\" but I\ncan't \"rm -fr -\" and I'm happy about it.\n\nNot a strong objection, but I think you can motivate the change better\nin the commit message, or give it up if I convinced you that it wasn't a\ngood idea.\n\n> * git branch (-d | -D) is not supposed to accept any other\n> arguments except for branch name so it makes sense to replace\n> the argv[i] with @{-1}. We will not lose the opportunity to\n> use it for something different for other git branch uses if\n> we will decide it’s required.\n\nThis could go inside the commit message, not below the ---.\n\n> +As a special case, the \"@{-N}\" syntax for the N-th last branch\n> +deletes the specified branch. \n\nIt's not really a special case. The @{-N} syntax works everywhere,\ndoesn't it?\n\n> --- a/builtin/branch.c\n> +++ b/builtin/branch.c\n> @@ -169,6 +169,8 @@ static int check_branch_commit(const char *branchname, const char *refname,\n> \treturn 0;\n> }\n> \n> +\n> +\n\nDon't include useless changes in your patches. Using \"git add -p\" and\n\"git commit\" without -a helps. Review your patch carefully and eliminate\nsuch artefacts before sending: they only distract reviewers.\n\n> +static void allow_dash_as_prev_branch_alias(const char **argv, int dash_position)\n\nThe function name looks overly verbose and I'm not sure it describes\nexactly what it does: it does not really \"allow\", but it \"expands\" it.\nI'd call it expand_dash_shortcut().\n\n> \t\tif (!target) {\n> -\t\t\terror(remote_branch\n> -\t\t\t ? _(\"remote-tracking branch '%s' not found.\")\n> -\t\t\t : _(\"branch '%s' not found.\"), bname.buf);\n> +\t\t\terror(!strcmp(bname.buf, \"@{-1}\")\n> +\t\t\t\t? _(\"There is no previous branch that could be referred to at the moment.\")\n\nThis is not directly related to your change: the user could already\nwrite \"@{-1}\" and may already have wanted a better error message. I\nthink this could/should be split into a separate patch.\n\n> --- a/t/t3200-branch.sh\n> +++ b/t/t3200-branch.sh\n> @@ -403,6 +403,16 @@ test_expect_success 'test deleting branch without config' '\n> \ttest_i18ncmp expect actual\n> '\n> \n> +test_expect_success 'test deleting \"-\" deletes previous branch' '\n> +\tgit checkout -b prev &&\n> +\ttest_commit prev &&\n> +\tgit checkout master &&\n> +\tgit branch -D - >actual &&\n> +\tsha1=$(git rev-parse prev | cut -c 1-7) &&\n\ngit rev-parse --short avoids this \"cut\".\n\nRegards,\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"281352","messageId":"xmqqtwjzn2nu.fsf@gitster.mtv.corp.google.com","threadId":"41769","inReplyTo":"vpqfuvju4km.fsf@anie.imag.fr","subject":"Re: [PATCH][Outreachy] branch -D: allow - as abbreviation of @{-1}","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-03-21T17:47:49Z","receivedAt":"2016-03-21T17:47:49Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Matthieu Moy writes:\n\n> But I'm not sure how often people want to delete (force-delete according\n> to your message) the branch they just come from.\n\nOne that I heard was this sequence:\n\n $ git checkout -b work master\n $ work work work ...\n $ git checkout master\n $ git merge work\n $ git branch -d work\n\nwhere their argument was that they are done with the work branch,\nand it no longer is needed.\n\nAs you may be able to guess, I don't personally subscribe to that\nworkflow (I'd keep the topic a lot longer, until the result of the\nmerge is proven to be good in the field), but probably these people\nare more perfect developers than I am ;-)\n\n> It might be less\n> dangerous to give incentive to the user to spell the branch name\n> completely to avoid mistake. As analogy, my shell knows \"cd -\" but I\n> can't \"rm -fr -\" and I'm happy about it.\n\nThat is indeed an interesting analogy.\n"},{"id":"281355","messageId":"CAPig+cQNBeBaLb-znNgxOanUz++pQLae=c8vUtFXjqxOns4d-g@mail.gmail.com","threadId":"41769","inReplyTo":"xmqqtwjzn2nu.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH][Outreachy] branch -D: allow - as abbreviation of @{-1}","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2016-03-21T17:56:10Z","receivedAt":"2016-03-21T17:56:10Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Mar 21, 2016 at 1:47 PM, Junio C Hamano wrote:\n> Matthieu Moy writes:\n>> But I'm not sure how often people want to delete (force-delete according\n>> to your message) the branch they just come from.\n>\n> One that I heard was this sequence:\n>\n> $ git checkout -b work master\n> $ work work work ...\n> $ git checkout master\n> $ git merge work\n> $ git branch -d work\n>\n> where their argument was that they are done with the work branch,\n> and it no longer is needed.\n\nI frequently use throwaway branches when messing around with some idea\nor when reviewing patches submitted to the mailing list, and the\nworkflow ends up being similar to the above:\n\n $ git checkout -b throwaway master\n $ ...work work work...\n $ git checkout master\n $ git branch -D throwaway\n\nSo, I can see how having \"git branch -D\" (force-delete) recognize \"-\"\ncould be a convenience.\n"},{"id":"281356","messageId":"xmqqegb3n1if.fsf@gitster.mtv.corp.google.com","threadId":"41769","inReplyTo":"CAPig+cQNBeBaLb-znNgxOanUz++pQLae=c8vUtFXjqxOns4d-g@mail.gmail.com","subject":"Re: [PATCH][Outreachy] branch -D: allow - as abbreviation of @{-1}","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-03-21T18:12:40Z","receivedAt":"2016-03-21T18:12:40Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Sunshine writes:\n\n> On Mon, Mar 21, 2016 at 1:47 PM, Junio C Hamano wrote:\n>> Matthieu Moy writes:\n>>> But I'm not sure how often people want to delete (force-delete according\n>>> to your message) the branch they just come from.\n>>\n>> One that I heard was this sequence:\n>>\n>> $ git checkout -b work master\n>> $ work work work ...\n>> $ git checkout master\n>> $ git merge work\n>> $ git branch -d work\n>>\n>> where their argument was that they are done with the work branch,\n>> and it no longer is needed.\n>\n> I frequently use throwaway branches when messing around with some idea\n> or when reviewing patches submitted to the mailing list, and the\n> workflow ends up being similar to the above:\n>\n> $ git checkout -b throwaway master\n> $ ...work work work...\n> $ git checkout master\n> $ git branch -D throwaway\n>\n> So, I can see how having \"git branch -D\" (force-delete) recognize \"-\"\n> could be a convenience.\n\nI guess that it would make some sense in ancient world, but there is\ndetached HEAD for that workflow, so it is unlikely that I'd find it\nuseful myself (in my worldview, throw-away work is discardable by\ndefault, which is why I start from detached HEAD, until I find that\nthe result is more interesting than I originally thought and decide\nto save it to a more permanent branch with \"checkout -b\" from\nthere).\n\nBut of course people are different.\n\nIn any case, we found two plausible explanation why people may want\nto do this. I however tend to agree with Matthieu that it may be\nsafer not to give a short-hand access to destructive operations.\n"},{"id":"281362","messageId":"CAPig+cQ-kmHeQsMMKdj0joSHWEnGfirSM1WbC-6G2BiMLnoUGg@mail.gmail.com","threadId":"41769","inReplyTo":"CAJPOeMediYQMwvgqhOquVh+KT61gdpUew9ernjuOYuf8By=hZQ@mail.gmail.com","subject":"Re: [PATCH][Outreachy] branch -D: allow - as abbreviation of @{-1}","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2016-03-21T19:14:26Z","receivedAt":"2016-03-21T19:14:26Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"[please don't top-post; respond inline instead]\n\nOn Mon, Mar 21, 2016 at 2:53 PM, elena petrashen\n wrote:\n> Thank you for the feedback!\n> The safety concert is indeed a good point. Would it maybe make\n> sense to request user to confirm this operation? I.e:\n> $git delete -D -\n> You've requested to delete \"foo\" branch. Proceed with deleting? y/n\n\nRather than requiring the user to stop and answer a question, an\nalternative would be to perform the deletion as requested but then\ngive advice about how to recover if the wrong branch was deleted by\nmistake, much in the way advice is given when switching to a detached\nhead. (And, the advice could be suppressed by an \"advice\"\nconfiguration variable similar to how other advice messages can be\nsuppressed.)\n\n> Also, do you think - shortcut is justifiable for $git branch -m when\n> referring to the \"old branch\"?\n\nIt comes up once in a while that I've switched away from a branch and\nthen decide I want to rename it, but it's infrequent enough that it's\ndifficult to say if it would be generally useful.\n"},{"id":"281363","messageId":"CAPig+cS-07dMStzAp+J+9_FJkREdRZcACg1CMoz_YEu9AezmJQ@mail.gmail.com","threadId":"41769","inReplyTo":"vpqfuvju4km.fsf@anie.imag.fr","subject":"Re: [PATCH][Outreachy] branch -D: allow - as abbreviation of @{-1}","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2016-03-21T19:26:15Z","receivedAt":"2016-03-21T19:26:15Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Mar 21, 2016 at 1:24 PM, Matthieu Moy\n wrote:\n> Elena Petrashen writes:\n>> * git branch (-d | -D) is not supposed to accept any other\n>> arguments except for branch name so it makes sense to replace\n>> the argv[i] with @{-1}. We will not lose the opportunity to\n>> use it for something different for other git branch uses if\n>> we will decide it’s required.\n>\n> This could go inside the commit message, not below the ---.\n\nNo, I think this does indeed belong here in the commentary area (below\n\"---\"); not in the commit message. The previous version of the patch\nincorrectly mapped \"-\" to \"@{-1}\" for all argv[], not just for the\nbranch name, and the above paragraph is explaining that the mistake\nhas been fixed in the current version of the patch. Whatever minor\nadditional information the above paragraph states is already implied\nby the patch title and by common sense, so it probably wouldn't be\nhelpful to bulk up the commit message just for that.\n"},{"id":"281364","messageId":"CAGZ79kbmMAyd3rDokCD6RSLuoJDKfPG0m2+HszUuRYG_eHdC6A@mail.gmail.com","threadId":"41769","inReplyTo":"CAPig+cQ-kmHeQsMMKdj0joSHWEnGfirSM1WbC-6G2BiMLnoUGg@mail.gmail.com","subject":"Re: [PATCH][Outreachy] branch -D: allow - as abbreviation of @{-1}","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-03-21T19:27:12Z","receivedAt":"2016-03-21T19:27:12Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Mon, Mar 21, 2016 at 12:14 PM, Eric Sunshine wrote:\n> [please don't top-post; respond inline instead]\n>\n> On Mon, Mar 21, 2016 at 2:53 PM, elena petrashen\n> wrote:\n>> Thank you for the feedback!\n>> The safety concert is indeed a good point. Would it maybe make\n>> sense to request user to confirm this operation? I.e:\n>> $git delete -D -\n>> You've requested to delete \"foo\" branch. Proceed with deleting? y/n\n>\n> Rather than requiring the user to stop and answer a question, an\n> alternative would be to perform the deletion as requested but then\n> give advice about how to recover if the wrong branch was deleted by\n> mistake, much in the way advice is given when switching to a detached\n> head. (And, the advice could be suppressed by an \"advice\"\n> configuration variable similar to how other advice messages can be\n> suppressed.)\n\nThere is some advice, but it is not spelled out for beginners IMO\n\n Deleted branch test_branch (was c62f9fe).\n\nI would assume intermediate Git users would know c62f9fe to be a sha1 which\ncan be used to retrieve the content of the deleted branch. It is a good\nidea nevertheless to add\n\n To recover the branch: \"git branch test_branch c62f9fe\n\nI would not even make it configurable to suppress it as it is just 2 more\nlines.\n\nThanks,\nStefan\n\n>\n>> Also, do you think - shortcut is justifiable for $git branch -m when\n>> referring to the \"old branch\"?\n>\n> It comes up once in a while that I've switched away from a branch and\n> then decide I want to rename it, but it's infrequent enough that it's\n> difficult to say if it would be generally useful.\n> --\n> To unsubscribe from this list: send the line \"unsubscribe git\" in\n> the body of a message to majordomo@vger.kernel.org\n> More majordomo info at http://vger.kernel.org/majordomo-info.html\n"}]}