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

From: elena petrashen <elena.petrashen@gmail.com>
Date: Mar 21, 2016, 15:12 UTC
Message-ID: <CAJPOeMd-zeThDhx-OWW_6LwG9A+U3UU-oKPOZGcycR-xQRPcgg@mail.gmail.com>
In-Reply-To: <CAPig+cQYYPmUtOoTGDzzQJC8n+9UoVEskmnZ19E=rroyU5+Pkg@mail.gmail.com>

Hi Eric, Junio,

thank you very much for your feedback! I honestly apologize I got so many things wrong.

I'll try to minimize the scope a little bit for the next attempt to make sure my approach is good first and then expand: i.e will only teach git branch to delete "-" & give out an error message in case there is no previous branch to make sure I got the basics correct.

Elena

On Fri, Mar 18, 2016 at 9:13 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:

Show 30 quoted lines

>
> On Fri, Mar 18, 2016 at 12:10 PM, Junio C Hamano <gitster@pobox.com> wrote:
> > On the submitted patch itself, in decreasing order of importance:
> >
> >  * The approach you took turns every "-" that appears as a command
> >    line argument into "@{-1}", but it does so without even checking
> >    where "-" appears on the command line is meant to take a branch
> >    name.  This closes the door to later add an option that takes "-"
> >    as an argument that means something different (e.g. one common
> >    use of "-" is to mean "the standard input" when a filename is
> >    expected).
> >
> >  * There is no explanation and justification in the proposed log
> >    message why you took a particular approach.  Why is that a good
> >    approach?  What are the possible downsides?  What were the
> >    alternatives (if any), and why is the approach chosen is better
> >    than them?
> >
> >  * We forbid declaration-after-statement in our codebase.
> >
> >  * When you do not yet have the "branch I was previously on", I
> >    imagine that your implementation would give you this:
> >    [...]
> >
> >  * You do not need the brace pairs around the body of these new
> >    "for" or "if" statements.
>
> Also for consideration: You'd probably also want to add a test or two
> or three to verify that "-" works as intended, and a documentation
> update may be warranted.