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

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(-)

Hi, and welcome!

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

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.

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

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

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.

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().

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.

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

Regards,

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

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 ;-)

That is indeed an interesting analogy.

On Mon, Mar 21, 2016 at 1:47 PM, Junio C Hamano <gitster@pobox.com> wrote:

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.

Eric Sunshine <sunshine@sunshineco.com> writes:

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.

On Mon, Mar 21, 2016 at 1:24 PM, Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> wrote:

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.

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

On Mon, Mar 21, 2016 at 2:53 PM, elena petrashen <elena.petrashen@gmail.com> wrote:

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.)

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.

On Mon, Mar 21, 2016 at 12:14 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:

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