[PATCH][Outreachy] branch -D: allow - as abbreviation of @{-1}

[PATCH][Outreachy] branch -D: allow - as abbreviation of @{-1}

[Elena Petrashen]

Signed-off-by: Elena Petrashen <elena.petrashen@gmail.com>

---
This micro-patch is meant to allow “-“ as a short-hand for
“@{-1} for branch -D (Cf. $gmane/230828):

* git branch (-d | -D) is not supposed to accept any other
arguments except for branch name so it makes sense to replace
the argv[i] with @{-1}. We will not lose the opportunity to
use it for something different for other git branch uses if
we will decide it’s required.

* the small allow_dash_as_prev_branch_alias function can be
reused to teach git branch -m to allow “-“ as a short-hand for
“@{-1}  as well and possibly makes it easy to understand what’s
going on in the code

* if there’s no previous branch in the repository yet, a
specific warning message is given

Thank you! Looking forward to any feedback.

 Documentation/git-branch.txt |  5 ++++-
 builtin/branch.c             | 18 +++++++++++++++---
 t/t3200-branch.sh            | 10 ++++++++++
 3 files changed, 29 insertions(+), 4 deletions(-)

diff --git a/Documentation/git-branch.txt b/Documentation/git-branch.txt
index 4a7037f..9f43665 100644
--- a/Documentation/git-branch.txt
+++ b/Documentation/git-branch.txt
@@ -64,7 +64,10 @@ to happen.
 
 With a `-d` or `-D` option, `<branchname>` will be deleted.  You may
 specify more than one branch for deletion.  If the branch currently
-has a reflog then the reflog will also be deleted.
+has a reflog then the reflog will also be deleted. 
+As a special case, the "@{-N}" syntax for the N-th last branch
+deletes the specified branch.  You may also specify - which is synonymous
+with "@{-1}".
 
 Use `-r` together with `-d` to delete remote-tracking branches. Note, that it
 only makes sense to delete remote-tracking branches if they no longer exist
diff --git a/builtin/branch.c b/builtin/branch.c
index 7b45b6b..9614d18 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -169,6 +169,8 @@ static int check_branch_commit(const char *branchname, const char *refname,
 	return 0;
 }
 
+
+
 static void delete_branch_config(const char *branchname)
 {
 	struct strbuf buf = STRBUF_INIT;
@@ -178,6 +180,12 @@ static void delete_branch_config(const char *branchname)
 	strbuf_release(&buf);
 }
 
+static void allow_dash_as_prev_branch_alias(const char **argv, int dash_position)
+{
+	if (!strcmp(argv[dash_position], "-"))
+		argv[dash_position] = "@{-1}";
+}
+
 static int delete_branches(int argc, const char **argv, int force, int kinds,
 			   int quiet)
 {
@@ -210,10 +218,12 @@ static int delete_branches(int argc, const char **argv, int force, int kinds,
 		if (!head_rev)
 			die(_("Couldn't look up commit object for HEAD"));
 	}
+
 	for (i = 0; i < argc; i++, strbuf_release(&bname)) {
 		const char *target;
 		int flags = 0;
 
+		allow_dash_as_prev_branch_alias(argv, i);
 		strbuf_branchname(&bname, argv[i]);
 		if (kinds == FILTER_REFS_BRANCHES && !strcmp(head, bname.buf)) {
 			error(_("Cannot delete the branch '%s' "
@@ -231,9 +241,11 @@ static int delete_branches(int argc, const char **argv, int force, int kinds,
 					    | RESOLVE_REF_ALLOW_BAD_NAME,
 					    sha1, &flags);
 		if (!target) {
-			error(remote_branch
-			      ? _("remote-tracking branch '%s' not found.")
-			      : _("branch '%s' not found."), bname.buf);
+			error(!strcmp(bname.buf, "@{-1}")
+				? _("There is no previous branch that could be referred to at the moment.")
+				: remote_branch
+					? _("remote-tracking branch '%s' not found.")
+					: _("branch '%s' not found."), bname.buf);
 			ret = 1;
 			continue;
 		}
diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
index a897248..d21369f 100755
--- a/t/t3200-branch.sh
+++ b/t/t3200-branch.sh
@@ -403,6 +403,16 @@ test_expect_success 'test deleting branch without config' '
 	test_i18ncmp expect actual
 '
 
+test_expect_success 'test deleting "-" deletes previous branch' '
+	git checkout -b prev &&
+	test_commit prev &&
+	git checkout master &&
+	git branch -D - >actual &&
+	sha1=$(git rev-parse prev | cut -c 1-7) &&
+	echo "Deleted branch prev (was $sha1)." >expect &&
+	test_cmp expect actual
+'
+
 test_expect_success 'test --track without .fetch entries' '
 	git branch --track my8 &&
 	test "$(git config branch.my8.remote)" &&
-- 
2.8.0.rc3.12.g047057b.dirty

Re: [PATCH][Outreachy] branch -D: allow - as abbreviation of @{-1}

[Matthieu Moy]

Hi, and welcome!

Elena Petrashen <elena.petrashen@gmail.com> writes:

> Signed-off-by: Elena Petrashen <elena.petrashen@gmail.com>
>
> ---
> This micro-patch is meant to allow “-“ as a short-hand for
> “@{-1} for branch -D (Cf. $gmane/230828):

Is it a good thing to do?

I find "git checkout -" to be a very nice shortcut, because users very
often want to get back to the branch they used to be before.

But I'm not sure how often people want to delete (force-delete according
to your message) the branch they just come from. It might be less
dangerous to give incentive to the user to spell the branch name
completely to avoid mistake. As analogy, my shell knows "cd -" but I
can't "rm -fr -" and I'm happy about it.

Not a strong objection, but I think you can motivate the change better
in the commit message, or give it up if I convinced you that it wasn't a
good idea.

> * git branch (-d | -D) is not supposed to accept any other
> arguments except for branch name so it makes sense to replace
> the argv[i] with @{-1}. We will not lose the opportunity to
> use it for something different for other git branch uses if
> we will decide it’s required.

This could go inside the commit message, not below the ---.

> +As a special case, the "@{-N}" syntax for the N-th last branch
> +deletes the specified branch. 

It's not really a special case. The @{-N} syntax works everywhere,
doesn't it?

> --- a/builtin/branch.c
> +++ b/builtin/branch.c
> @@ -169,6 +169,8 @@ static int check_branch_commit(const char *branchname, const char *refname,
>  	return 0;
>  }
>  
> +
> +

Don't include useless changes in your patches. Using "git add -p" and
"git commit" without -a helps. Review your patch carefully and eliminate
such artefacts before sending: they only distract reviewers.

> +static void allow_dash_as_prev_branch_alias(const char **argv, int dash_position)

The function name looks overly verbose and I'm not sure it describes
exactly what it does: it does not really "allow", but it "expands" it.
I'd call it expand_dash_shortcut().

>  		if (!target) {
> -			error(remote_branch
> -			      ? _("remote-tracking branch '%s' not found.")
> -			      : _("branch '%s' not found."), bname.buf);
> +			error(!strcmp(bname.buf, "@{-1}")
> +				? _("There is no previous branch that could be referred to at the moment.")

This is not directly related to your change: the user could already
write "@{-1}" and may already have wanted a better error message. I
think this could/should be split into a separate patch.

> --- a/t/t3200-branch.sh
> +++ b/t/t3200-branch.sh
> @@ -403,6 +403,16 @@ test_expect_success 'test deleting branch without config' '
>  	test_i18ncmp expect actual
>  '
>  
> +test_expect_success 'test deleting "-" deletes previous branch' '
> +	git checkout -b prev &&
> +	test_commit prev &&
> +	git checkout master &&
> +	git branch -D - >actual &&
> +	sha1=$(git rev-parse prev | cut -c 1-7) &&

git rev-parse --short avoids this "cut".

Regards,

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

Re: [PATCH][Outreachy] branch -D: allow - as abbreviation of @{-1}

[Junio C Hamano]

Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:

> But I'm not sure how often people want to delete (force-delete according
> to your message) the branch they just come from.

One that I heard was this sequence:

    $ git checkout -b work master
    $ work work work ...
    $ git checkout master
    $ git merge work
    $ git branch -d work

where their argument was that they are done with the work branch,
and it no longer is needed.

As you may be able to guess, I don't personally subscribe to that
workflow (I'd keep the topic a lot longer, until the result of the
merge is proven to be good in the field), but probably these people
are more perfect developers than I am ;-)

> It might be less
> dangerous to give incentive to the user to spell the branch name
> completely to avoid mistake. As analogy, my shell knows "cd -" but I
> can't "rm -fr -" and I'm happy about it.

That is indeed an interesting analogy.

Re: [PATCH][Outreachy] branch -D: allow - as abbreviation of @{-1}

[Eric Sunshine]

On Mon, Mar 21, 2016 at 1:47 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:
>> But I'm not sure how often people want to delete (force-delete according
>> to your message) the branch they just come from.
>
> One that I heard was this sequence:
>
>     $ git checkout -b work master
>     $ work work work ...
>     $ git checkout master
>     $ git merge work
>     $ git branch -d work
>
> where their argument was that they are done with the work branch,
> and it no longer is needed.

I frequently use throwaway branches when messing around with some idea
or when reviewing patches submitted to the mailing list, and the
workflow ends up being similar to the above:

    $ git checkout -b throwaway master
    $ ...work work work...
    $ git checkout master
    $ git branch -D throwaway

So, I can see how having "git branch -D" (force-delete) recognize "-"
could be a convenience.

Re: [PATCH][Outreachy] branch -D: allow - as abbreviation of @{-1}

[Junio C Hamano]

Eric Sunshine <sunshine@sunshineco.com> writes:

> On Mon, Mar 21, 2016 at 1:47 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:
>>> But I'm not sure how often people want to delete (force-delete according
>>> to your message) the branch they just come from.
>>
>> One that I heard was this sequence:
>>
>>     $ git checkout -b work master
>>     $ work work work ...
>>     $ git checkout master
>>     $ git merge work
>>     $ git branch -d work
>>
>> where their argument was that they are done with the work branch,
>> and it no longer is needed.
>
> I frequently use throwaway branches when messing around with some idea
> or when reviewing patches submitted to the mailing list, and the
> workflow ends up being similar to the above:
>
>     $ git checkout -b throwaway master
>     $ ...work work work...
>     $ git checkout master
>     $ git branch -D throwaway
>
> So, I can see how having "git branch -D" (force-delete) recognize "-"
> could be a convenience.

I guess that it would make some sense in ancient world, but there is
detached HEAD for that workflow, so it is unlikely that I'd find it
useful myself (in my worldview, throw-away work is discardable by
default, which is why I start from detached HEAD, until I find that
the result is more interesting than I originally thought and decide
to save it to a more permanent branch with "checkout -b" from
there).

But of course people are different.

In any case, we found two plausible explanation why people may want
to do this.  I however tend to agree with Matthieu that it may be
safer not to give a short-hand access to destructive operations.

Re: [PATCH][Outreachy] branch -D: allow - as abbreviation of @{-1}

[Eric Sunshine]

[please don't top-post; respond inline instead]

On Mon, Mar 21, 2016 at 2:53 PM, elena petrashen
<elena.petrashen@gmail.com> wrote:
> Thank you for the feedback!
> The safety concert is indeed a good point. Would it maybe make
> sense to request user to confirm this operation? I.e:
> $git delete -D -
> You've requested to delete "foo" branch. Proceed with deleting? y/n

Rather than requiring the user to stop and answer a question, an
alternative would be to perform the deletion as requested but then
give advice about how to recover if the wrong branch was deleted by
mistake, much in the way advice is given when switching to a detached
head. (And, the advice could be suppressed by an "advice"
configuration variable similar to how other advice messages can be
suppressed.)

> Also, do you think - shortcut is justifiable for $git branch -m when
> referring to the "old branch"?

It comes up once in a while that I've switched away from a branch and
then decide I want to rename it, but it's infrequent enough that it's
difficult to say if it would be generally useful.

Re: [PATCH][Outreachy] branch -D: allow - as abbreviation of @{-1}

[Eric Sunshine]

On Mon, Mar 21, 2016 at 1:24 PM, Matthieu Moy
<Matthieu.Moy@grenoble-inp.fr> wrote:
> Elena Petrashen <elena.petrashen@gmail.com> writes:
>> * git branch (-d | -D) is not supposed to accept any other
>> arguments except for branch name so it makes sense to replace
>> the argv[i] with @{-1}. We will not lose the opportunity to
>> use it for something different for other git branch uses if
>> we will decide it’s required.
>
> This could go inside the commit message, not below the ---.

No, I think this does indeed belong here in the commentary area (below
"---"); not in the commit message. The previous version of the patch
incorrectly mapped "-" to "@{-1}" for all argv[], not just for the
branch name, and the above paragraph is explaining that the mistake
has been fixed in the current version of the patch. Whatever minor
additional information the above paragraph states is already implied
by the patch title and by common sense, so it probably wouldn't be
helpful to bulk up the commit message just for that.

Re: [PATCH][Outreachy] branch -D: allow - as abbreviation of @{-1}

[Stefan Beller]

On Mon, Mar 21, 2016 at 12:14 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
> [please don't top-post; respond inline instead]
>
> On Mon, Mar 21, 2016 at 2:53 PM, elena petrashen
> <elena.petrashen@gmail.com> wrote:
>> Thank you for the feedback!
>> The safety concert is indeed a good point. Would it maybe make
>> sense to request user to confirm this operation? I.e:
>> $git delete -D -
>> You've requested to delete "foo" branch. Proceed with deleting? y/n
>
> Rather than requiring the user to stop and answer a question, an
> alternative would be to perform the deletion as requested but then
> give advice about how to recover if the wrong branch was deleted by
> mistake, much in the way advice is given when switching to a detached
> head. (And, the advice could be suppressed by an "advice"
> configuration variable similar to how other advice messages can be
> suppressed.)

There is some advice, but it is not spelled out for beginners IMO

    Deleted branch test_branch (was c62f9fe).

I would assume intermediate Git users would know c62f9fe to be a sha1 which
can be used to retrieve the content of the deleted branch. It is a good
idea nevertheless to add

    To recover the branch: "git branch test_branch c62f9fe

I would not even make it configurable to suppress it as it is just 2 more
lines.

Thanks,
Stefan

>
>> Also, do you think - shortcut is justifiable for $git branch -m when
>> referring to the "old branch"?
>
> It comes up once in a while that I've switched away from a branch and
> then decide I want to rename it, but it's infrequent enough that it's
> difficult to say if it would be generally useful.
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html