Re: [PATCH] branch: implement shortcut to delete last branch
On Tue, Mar 27 2018, Aaron Greenberg wrote:
Show 23 quoted lines
> This patch gives git-branch the ability to delete the previous
> checked-out branch using the "-" shortcut. This shortcut already exists
> for git-checkout, git-merge, and git-revert. A common workflow is
>
> 1. Do some work on a local topic-branch and push it to a remote.
> 2. 'remote/topic-branch' gets merged in to 'remote/master'.
> 3. Switch back to local master and fetch 'remote/master'.
> 4. Delete previously checked-out local topic-branch.
>
> $ git checkout -b topic-a
> $ # Do some work...
> $ git commit -am "Implement feature A"
> $ git push origin topic-a
>
> $ git checkout master
> $ git branch -d topic-a
> $ # With this patch, a user could simply type
> $ git branch -d -
>
> "-" is a useful shortcut for cleaning up a just-merged branch
> (or a just switched-from branch.)
>
> Signed-off-by: Aaron Greenberg <p@aaronjgreenberg.com>
So just a tip on this E-Mail chain/patch submission. When you submit a v2 make the subject "[PATCH v2] ...", see Documentation/SubmittingPatches, also instead of sending two mails with the same subject better to put any comments not in the commit message...
> ---
...right here, below the triple dash, and CC the people commenting on the initial thread. With that, some comments on the change below:
Show 15 quoted lines
> builtin/branch.c | 3 +++
> t/t3200-branch.sh | 8 ++++++++
> 2 files changed, 11 insertions(+)
>
> diff --git a/builtin/branch.c b/builtin/branch.c
> index 6d0cea9..9e37078 100644
> --- a/builtin/branch.c
> +++ b/builtin/branch.c
> @@ -221,6 +221,9 @@ static int delete_branches(int argc, const char **argv, int force, int kinds,
> char *target = NULL;
> int flags = 0;
>
> + if (!strcmp(argv[i], "-"))
> + argv[i] = "@{-1}";
> +If we just do this, then when I do the following:
1. be on the 'foo' branch
2. checkout 'bar', commit
3. checkout 'foo'
4. git branch -d -I get this message:
error: The branch 'bar' is not fully merged.
If you are sure you want to delete it, run 'git branch -D bar'While that works, I think it's better UI for us to suggest what's actually the important alternation to the user's command, i.e. replace -d with -D, otherwise they'll think "oh '-' doesn't work, let's try to name the branch", only to get the same error. I.e. this on top:
Show changes to builtin/branch.c +9 −5
diff --git a/builtin/branch.c b/builtin/branch.c
index cdf2de4f1d..081a4384ce 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -157,17 +157,18 @@ static int branch_merged(int kind, const char *name,
static int check_branch_commit(const char *branchname, const char *refname,
const struct object_id *oid, struct commit *head_rev,
- int kinds, int force)
+ int kinds, int force, int resolved_dash)
{
struct commit *rev = lookup_commit_reference(oid);
if (!rev) {
- error(_("Couldn't look up commit object for '%s'"), refname);
+ error(_("Couldn't look up commit object for '%s'"), resolved_dash ? "-" : refname);
return -1;
}
if (!force && !branch_merged(kinds, branchname, rev, head_rev)) {
error(_("The branch '%s' is not fully merged.\n"
"If you are sure you want to delete it, "
- "run 'git branch -D %s'."), branchname, branchname);
+ "run 'git branch -D %s'."), branchname,
+ resolved_dash ? "-" : branchname);
return -1;
}
return 0;
@@ -220,9 +221,12 @@ static int delete_branches(int argc, const char **argv, int force, int kinds,
for (i = 0; i < argc; i++, strbuf_reset(&bname)) {
char *target = NULL;
int flags = 0;
+ int resolved_dash = 0;
- if (!strcmp(argv[i], "-"))
+ if (!strcmp(argv[i], "-")) {
argv[i] = "@{-1}";
+ resolved_dash = 1;
+ }
strbuf_branchname(&bname, argv[i], allowed_interpret);
free(name);
@@ -255,7 +259,7 @@ static int delete_branches(int argc, const char **argv, int force, int kinds,
if (!(flags & (REF_ISSYMREF|REF_ISBROKEN)) &&
check_branch_commit(bname.buf, name, &oid, head_rev, kinds,
- force)) {
+ force, resolved_dash)) {
ret = 1;
goto next;
}
There are other error messages there, but as far as I can tell it's best
if those just talk about the "bar" branch, but have a look.
A test for that with i18ngrep left as an exercise...
> strbuf_branchname(&bname, argv[i], allowed_interpret);
> free(name);
> name = mkpathdup(fmt, bname.buf);
> diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
> index 6c0b7ea..78c25aa 100755
> --- a/t/t3200-branch.sh
> +++ b/t/t3200-branch.sh
> @@ -776,6 +776,14 @@ test_expect_success 'deleting currently checked out branch fails' '
> test_must_fail git branch -d my7
> '
>
> +test_expect_success 'test deleting last branch' '
> + git checkout -b my7.1 &&
> + git checkout - &&
> + test_path_is_file .git/refs/heads/my7.1 &&
> + git branch -d - &&
> + test_path_is_missing .git/refs/heads/my7.1
> +'
> +
> test_expect_success 'test --track without .fetch entries' '
> git branch --track my8 &&
> test "$(git config branch.my8.remote)" &&
I don't know how much this applies to the existing commands you
mentioned (looks like not), but for "branch" specifically this looks
very incomplete, in particular:
* There's other modes where it takes commit-ish, e.g. "git branch foo
bar" to create a new branch foo starting at bar, but with your patch
"git branch foo -" won't work, even though there's no reason not to
think it does.
* There's no docs here to explain this difference, or TODO tests for
maybe making that work later. Your patch would be a lot easier to
review if you went through t/t3200-branch.sh, found the commit-ish
occurances you don't support, and added failing tests for those, and
explain in the commit message something to the effect of "I'm only
making *this* work, here's the cases that *don't* work".
