All of lore.kernel.org
 help / color / mirror / Atom feed
From: Junio C Hamano <gitster@pobox.com>
To: Eric Sunshine <sunshine@sunshineco.com>
Cc: Dinesh Polathula <dpdineshp2@gmail.com>, Git List <git@vger.kernel.org>
Subject: Re: [PATCH] Allow "-" as a short-hand for "@{-1}" in "git branch -d @{-1}"
Date: Sun, 06 Mar 2016 13:09:29 -0800	[thread overview]
Message-ID: <xmqqa8mbtit2.fsf@gitster.mtv.corp.google.com> (raw)
In-Reply-To: <CAPig+cTHBLGjn_tnAKE2i_zv9TRxdVbNfONpxg=ZvRSz9-4t=Q@mail.gmail.com> (Eric Sunshine's message of "Sun, 6 Mar 2016 14:35:55 -0500")

Eric Sunshine <sunshine@sunshineco.com> writes:

>> +       if (argc == 3 && !strcmp(argv[2], "-"))
> ...
> To address these issues, it seems like a more correct place to
> recognize "-" as an alias would be somewhere within
> builtin/branch.d:delete_branches().

I agree with all your review comments, and the above would also
cover another issue you did not mention above: "git branch -d/-D"
actually takes one or more branches to delete.

Another issue to consider is if we get a sensible error message.

What should the error message say if a user did these in a freshly
created empty repository?

    $ git commit --allow-empty -m initial
    $ git commit --allow-empty -m second
    $ git branch side master^
    $ git branch -d -

Currently, you would get

    $ git branch -d -
    error: branch '-' not found.

Is it OK to say

    error: branch '@{-1}' not found.

when the user never said '@{-1}' from the command line?

I am not saying it _is_ a problem--it may very well be acceptable,
as long as the users understand that '-' is merely a short-hand for
'@{-1}'.  Either way, the log message needs to have some evidence
that the patch author thought about the issue, the resolution the
patch author chose for the issue, and the reason why a particular
resolution was chosen.

Thanks.

      reply	other threads:[~2016-03-06 21:09 UTC|newest]

Thread overview: 4+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2016-03-06 12:48 [PATCH] Allow "-" as a short-hand for "@{-1}" in "git branch -d @{-1}" Dinesh Polathula
2016-03-06 12:48 ` Dinesh Polathula
2016-03-06 19:35   ` Eric Sunshine
2016-03-06 21:09     ` Junio C Hamano [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=xmqqa8mbtit2.fsf@gitster.mtv.corp.google.com \
    --to=gitster@pobox.com \
    --cc=dpdineshp2@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=sunshine@sunshineco.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.