{"thread":{"id":"48165","subject":"[PATCH] branch: implement shortcut to delete last branch","startedAt":"2018-03-27T18:46:53Z","lastAt":"2018-03-27T19:26:20Z","messageCount":5,"participants":["Aaron Greenberg","Jonathan Nieder","Ævar Arnfjörð Bjarmason"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"343162","messageId":"1522176390-646-1-git-send-email-p@aaronjgreenberg.com","threadId":"48165","inReplyTo":null,"subject":"[PATCH] branch: implement shortcut to delete last branch","fromName":"Aaron Greenberg","fromEmail":"p@aaronjgreenberg.com","sentAt":"2018-03-27T18:46:29Z","receivedAt":"2018-03-27T18:46:53Z","isPatch":true,"sender":{"key":"p@aaronjgreenberg.com","avatar":null},"body":"With the approvals listed in [*1*] and in accordance with the\nguidelines set out in Documentation/SubmittingPatches, I am submitting\nthis patch to be applied upstream.\n\nAfter work on this patch is done, I'll look into picking up where the\nprior work done in [*2*] left off.\n\nIs there anything else that needs to be done before this can be\naccepted?\n\n[Reference]\n\n*1* https://public-inbox.org/git/1521844835-23956-2-git-send-email-p@aaronjgreenberg.com/\n*2* https://public-inbox.org/git/1488007487-12965-1-git-send-email-kannan.siddharth12@gmail.com/\n\n"},{"id":"343163","messageId":"1522176390-646-2-git-send-email-p@aaronjgreenberg.com","threadId":"48165","inReplyTo":"1522176390-646-1-git-send-email-p@aaronjgreenberg.com","subject":"[PATCH] branch: implement shortcut to delete last branch","fromName":"Aaron Greenberg","fromEmail":"p@aaronjgreenberg.com","sentAt":"2018-03-27T18:46:30Z","receivedAt":"2018-03-27T18:47:11Z","isPatch":true,"sender":{"key":"p@aaronjgreenberg.com","avatar":null},"body":"This patch gives git-branch the ability to delete the previous\nchecked-out branch using the \"-\" shortcut. This shortcut already exists\nfor git-checkout, git-merge, and git-revert. A common workflow is\n\n1. Do some work on a local topic-branch and push it to a remote.\n2. 'remote/topic-branch' gets merged in to 'remote/master'.\n3. Switch back to local master and fetch 'remote/master'.\n4. Delete previously checked-out local topic-branch.\n\n$ git checkout -b topic-a\n$ # Do some work...\n$ git commit -am \"Implement feature A\"\n$ git push origin topic-a\n\n$ git checkout master\n$ git branch -d topic-a\n$ # With this patch, a user could simply type\n$ git branch -d -\n\n\"-\" is a useful shortcut for cleaning up a just-merged branch\n(or a just switched-from branch.)\n\nSigned-off-by: Aaron Greenberg <p@aaronjgreenberg.com>\n---\n builtin/branch.c  | 3 +++\n t/t3200-branch.sh | 8 ++++++++\n 2 files changed, 11 insertions(+)\n\ndiff --git a/builtin/branch.c b/builtin/branch.c\nindex 6d0cea9..9e37078 100644\n--- a/builtin/branch.c\n+++ b/builtin/branch.c\n@@ -221,6 +221,9 @@ static int delete_branches(int argc, const char **argv, int force, int kinds,\n \t\tchar *target = NULL;\n \t\tint flags = 0;\n \n+\t\tif (!strcmp(argv[i], \"-\"))\n+\t\t\targv[i] = \"@{-1}\";\n+\n \t\tstrbuf_branchname(&bname, argv[i], allowed_interpret);\n \t\tfree(name);\n \t\tname = mkpathdup(fmt, bname.buf);\ndiff --git a/t/t3200-branch.sh b/t/t3200-branch.sh\nindex 6c0b7ea..78c25aa 100755\n--- a/t/t3200-branch.sh\n+++ b/t/t3200-branch.sh\n@@ -776,6 +776,14 @@ test_expect_success 'deleting currently checked out branch fails' '\n \ttest_must_fail git branch -d my7\n '\n \n+test_expect_success 'test deleting last branch' '\n+\tgit checkout -b my7.1 &&\n+\tgit checkout  - &&\n+\ttest_path_is_file .git/refs/heads/my7.1 &&\n+\tgit branch -d - &&\n+\ttest_path_is_missing .git/refs/heads/my7.1\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.7.4\n\n"},{"id":"343164","messageId":"20180327191139.GD4343@aiede.svl.corp.google.com","threadId":"48165","inReplyTo":"1522176390-646-2-git-send-email-p@aaronjgreenberg.com","subject":"Re: [PATCH] branch: implement shortcut to delete last branch","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2018-03-27T19:11:39Z","receivedAt":"2018-03-27T19:11:49Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nAaron Greenberg wrote:\n\n> This patch gives git-branch the ability to delete the previous\n> checked-out branch using the \"-\" shortcut. This shortcut already exists\n> for git-checkout, git-merge, and git-revert. A common workflow is\n>\n> 1. Do some work on a local topic-branch and push it to a remote.\n> 2. 'remote/topic-branch' gets merged in to 'remote/master'.\n> 3. Switch back to local master and fetch 'remote/master'.\n> 4. Delete previously checked-out local topic-branch.\n\nThanks for a clear example.\n\n[...]\n>  builtin/branch.c  | 3 +++\n>  t/t3200-branch.sh | 8 ++++++++\n>  2 files changed, 11 insertions(+)\n[...]\n> With the approvals listed in [*1*] and in accordance with the\n> guidelines set out in Documentation/SubmittingPatches, I am submitting\n> this patch to be applied upstream.\n>\n> After work on this patch is done, I'll look into picking up where the\n> prior work done in [*2*] left off.\n>\n> Is there anything else that needs to be done before this can be\n> accepted?\n>\n> [Reference]\n>\n> *1* https://public-inbox.org/git/1521844835-23956-2-git-send-email-p@aaronjgreenberg.com/\n> *2* https://public-inbox.org/git/1488007487-12965-1-git-send-email-kannan.siddharth12@gmail.com/\n\nFor the future, please don't use a separate cover letter message in a\nsingle-patch series like this one.  Instead, please put any discussion\nthat you don't want to go in the commit message after the three-dash\ndivider in the same message as the patch, like the diffstat.  See the\nsection \"Sending your patches\" in Documentation/SubmittingPatches for\nmore details:\n\n| You often want to add additional explanation about the patch,\n| other than the commit message itself.  Place such \"cover letter\"\n| material between the three-dash line and the diffstat.  For\n| patches requiring multiple iterations of review and discussion,\n| an explanation of changes between each iteration can be kept in\n| Git-notes and inserted automatically following the three-dash\n| line via `git format-patch --notes`.\n\nThat makes it easier for reviewers to see all the information in one\nplace and in particular can help them in fleshing out the commit\nmessage if it is missing details.\n\n[...]\n> diff --git a/builtin/branch.c b/builtin/branch.c\n> index 6d0cea9..9e37078 100644\n> --- a/builtin/branch.c\n> +++ b/builtin/branch.c\n> @@ -221,6 +221,9 @@ static int delete_branches(int argc, const char **argv, int force, int kinds,\n>  \t\tchar *target = NULL;\n>  \t\tint flags = 0;\n>  \n> +\t\tif (!strcmp(argv[i], \"-\"))\n> +\t\t\targv[i] = \"@{-1}\";\n> +\n>  \t\tstrbuf_branchname(&bname, argv[i], allowed_interpret);\n\nThis makes me wonder: should the \"-\" shortcut be handled in\nstrbuf_branchname itself?  That would presumably simplify callers like\nthis one.\n\n[...]\n> --- a/t/t3200-branch.sh\n> +++ b/t/t3200-branch.sh\n> @@ -776,6 +776,14 @@ test_expect_success 'deleting currently checked out branch fails' '\n>  \ttest_must_fail git branch -d my7\n>  '\n>  \n> +test_expect_success 'test deleting last branch' '\n> +\tgit checkout -b my7.1 &&\n\nThis naming scheme feels likely to conflict with other patches.\nHow about something like\n\n\tgit checkout -B previous &&\n\tgit checkout -B new-branch &&\n\tgit show-ref --verify refs/heads/previous &&\n\tgit branch -d - &&\n\ttest_must_fail git show-ref --verify refs/heads/previous\n\n?\n\n> +\tgit checkout  - &&\n> +\ttest_path_is_file .git/refs/heads/my7.1 &&\n> +\tgit branch -d - &&\n> +\ttest_path_is_missing .git/refs/heads/my7.1\n\nnot specific to this test, but this is relying on low-level details\nand means that an implementation that e.g. deleted a loose ref but\nkept a packed ref would pass the test despite being broken.\n\nSome of the other tests appear to use show-ref, so that might work\nwell.\n\nNo need to act on this, since what you have here is at least\nconsistent with some of the other tests in the file.  In other words,\nit might be even better to address this throughout the file in a\nseparate patch.\n\n> +'\n> +\n\nA few questions that the tests leave unanswered for me:\n\n 1. Does \"git branch -d -\" refuse to delete an un-merged branch\n    like \"git branch -d topic\" would?  (That seems like a valuable\n    thing to test for typo protection reasons.)\n\n 2. What happens if there is no previous branch, as in e.g. a new\n    clone?\n\n 3. What does the error message look like when it cannot delete the\n    previous branch for whatever reason?  Does it identify the branch\n    that can't be deleted?\n\n>  test_expect_success 'test --track without .fetch entries' '\n>  \tgit branch --track my8 &&\n>  \ttest \"$(git config branch.my8.remote)\" &&\n\nThanks and hope that helps,\nJonathan\n"},{"id":"343165","messageId":"87tvt1wce3.fsf@evledraar.gmail.com","threadId":"48165","inReplyTo":"1522176390-646-2-git-send-email-p@aaronjgreenberg.com","subject":"Re: [PATCH] branch: implement shortcut to delete last branch","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-03-27T19:23:48Z","receivedAt":"2018-03-27T19:23:58Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Tue, Mar 27 2018, Aaron Greenberg wrote:\n\n> This patch gives git-branch the ability to delete the previous\n> checked-out branch using the \"-\" shortcut. This shortcut already exists\n> for git-checkout, git-merge, and git-revert. A common workflow is\n>\n> 1. Do some work on a local topic-branch and push it to a remote.\n> 2. 'remote/topic-branch' gets merged in to 'remote/master'.\n> 3. Switch back to local master and fetch 'remote/master'.\n> 4. Delete previously checked-out local topic-branch.\n>\n> $ git checkout -b topic-a\n> $ # Do some work...\n> $ git commit -am \"Implement feature A\"\n> $ git push origin topic-a\n>\n> $ git checkout master\n> $ git branch -d topic-a\n> $ # With this patch, a user could simply type\n> $ git branch -d -\n>\n> \"-\" is a useful shortcut for cleaning up a just-merged branch\n> (or a just switched-from branch.)\n>\n> Signed-off-by: Aaron Greenberg <p@aaronjgreenberg.com>\n\nSo just a tip on this E-Mail chain/patch submission. When you submit a\nv2 make the subject \"[PATCH v2] ...\", see\nDocumentation/SubmittingPatches, also instead of sending two mails with\nthe same subject better to put any comments not in the commit message...\n\n> ---\n\n...right here, below the triple dash, and CC the people commenting on\nthe initial thread. With that, some comments on the change below:\n\n>  builtin/branch.c  | 3 +++\n>  t/t3200-branch.sh | 8 ++++++++\n>  2 files changed, 11 insertions(+)\n>\n> diff --git a/builtin/branch.c b/builtin/branch.c\n> index 6d0cea9..9e37078 100644\n> --- a/builtin/branch.c\n> +++ b/builtin/branch.c\n> @@ -221,6 +221,9 @@ static int delete_branches(int argc, const char **argv, int force, int kinds,\n>  \t\tchar *target = NULL;\n>  \t\tint flags = 0;\n>\n> +\t\tif (!strcmp(argv[i], \"-\"))\n> +\t\t\targv[i] = \"@{-1}\";\n> +\n\nIf we just do this, then when I do the following:\n\n    1. be on the 'foo' branch\n    2. checkout 'bar', commit\n    3. checkout 'foo'\n    4. git branch -d -\n\nI get this message:\n\n    error: The branch 'bar' is not fully merged.\n    If you are sure you want to delete it, run 'git branch -D bar'\n\nWhile that works, I think it's better UI for us to suggest what's\nactually the important alternation to the user's command, i.e. replace\n-d with -D, otherwise they'll think \"oh '-' doesn't work, let's try to\nname the branch\", only to get the same error. I.e. this on top:\n\ndiff --git a/builtin/branch.c b/builtin/branch.c\nindex cdf2de4f1d..081a4384ce 100644\n--- a/builtin/branch.c\n+++ b/builtin/branch.c\n@@ -157,17 +157,18 @@ static int branch_merged(int kind, const char *name,\n\n static int check_branch_commit(const char *branchname, const char *refname,\n \t\t\t       const struct object_id *oid, struct commit *head_rev,\n-\t\t\t       int kinds, int force)\n+\t\t\t       int kinds, int force, int resolved_dash)\n {\n \tstruct commit *rev = lookup_commit_reference(oid);\n \tif (!rev) {\n-\t\terror(_(\"Couldn't look up commit object for '%s'\"), refname);\n+\t\terror(_(\"Couldn't look up commit object for '%s'\"), resolved_dash ? \"-\" : refname);\n \t\treturn -1;\n \t}\n \tif (!force && !branch_merged(kinds, branchname, rev, head_rev)) {\n \t\terror(_(\"The branch '%s' is not fully merged.\\n\"\n \t\t      \"If you are sure you want to delete it, \"\n-\t\t      \"run 'git branch -D %s'.\"), branchname, branchname);\n+\t\t      \"run 'git branch -D %s'.\"), branchname,\n+\t\t      resolved_dash ? \"-\" : branchname);\n \t\treturn -1;\n \t}\n \treturn 0;\n@@ -220,9 +221,12 @@ static int delete_branches(int argc, const char **argv, int force, int kinds,\n \tfor (i = 0; i < argc; i++, strbuf_reset(&bname)) {\n \t\tchar *target = NULL;\n \t\tint flags = 0;\n+\t\tint resolved_dash = 0;\n\n-\t\tif (!strcmp(argv[i], \"-\"))\n+\t\tif (!strcmp(argv[i], \"-\")) {\n \t\t\targv[i] = \"@{-1}\";\n+\t\t\tresolved_dash = 1;\n+\t\t}\n\n \t\tstrbuf_branchname(&bname, argv[i], allowed_interpret);\n \t\tfree(name);\n@@ -255,7 +259,7 @@ static int delete_branches(int argc, const char **argv, int force, int kinds,\n\n \t\tif (!(flags & (REF_ISSYMREF|REF_ISBROKEN)) &&\n \t\t    check_branch_commit(bname.buf, name, &oid, head_rev, kinds,\n-\t\t\t\t\tforce)) {\n+\t\t\t\t\tforce, resolved_dash)) {\n \t\t\tret = 1;\n \t\t\tgoto next;\n \t\t}\n\nThere are other error messages there, but as far as I can tell it's best\nif those just talk about the \"bar\" branch, but have a look.\n\nA test for that with i18ngrep left as an exercise...\n\n>  \t\tstrbuf_branchname(&bname, argv[i], allowed_interpret);\n>  \t\tfree(name);\n>  \t\tname = mkpathdup(fmt, bname.buf);\n> diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh\n> index 6c0b7ea..78c25aa 100755\n> --- a/t/t3200-branch.sh\n> +++ b/t/t3200-branch.sh\n> @@ -776,6 +776,14 @@ test_expect_success 'deleting currently checked out branch fails' '\n>  \ttest_must_fail git branch -d my7\n>  '\n>\n> +test_expect_success 'test deleting last branch' '\n> +\tgit checkout -b my7.1 &&\n> +\tgit checkout  - &&\n> +\ttest_path_is_file .git/refs/heads/my7.1 &&\n> +\tgit branch -d - &&\n> +\ttest_path_is_missing .git/refs/heads/my7.1\n> +'\n> +\n>  test_expect_success 'test --track without .fetch entries' '\n>  \tgit branch --track my8 &&\n>  \ttest \"$(git config branch.my8.remote)\" &&\n\nI don't know how much this applies to the existing commands you\nmentioned (looks like not), but for \"branch\" specifically this looks\nvery incomplete, in particular:\n\n * There's other modes where it takes commit-ish, e.g. \"git branch foo\n   bar\" to create a new branch foo starting at bar, but with your patch\n   \"git branch foo -\" won't work, even though there's no reason not to\n   think it does.\n\n * There's no docs here to explain this difference, or TODO tests for\n   maybe making that work later. Your patch would be a lot easier to\n   review if you went through t/t3200-branch.sh, found the commit-ish\n   occurances you don't support, and added failing tests for those, and\n   explain in the commit message something to the effect of \"I'm only\n   making *this* work, here's the cases that *don't* work\".\n"},{"id":"343166","messageId":"87sh8lwca4.fsf@evledraar.gmail.com","threadId":"48165","inReplyTo":"87tvt1wce3.fsf@evledraar.gmail.com","subject":"Re: [PATCH] branch: implement shortcut to delete last branch","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-03-27T19:26:11Z","receivedAt":"2018-03-27T19:26:20Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Tue, Mar 27 2018, Ævar Arnfjörð Bjarmason wrote:\n\n> [...]With that, some comments on the change below:\n\nAlso, didn't mean to gang up on you. I only saw Jonathan's E-Mail after\nI sent mine, and it covered some of the same stuff.\n"}]}