On Fri, Mar 18, 2016 at 12:10 PM, Junio C Hamano <gitster@xxxxxxxxx> 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.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html