Re: [PATCH 4/4] builtin-remote: add set-head verb

On Fri, Feb 13, 2009 at 02:09:01AM -0800, Junio C Hamano wrote:

The entire series looks sane from a very cursory look; especially the
earlier ones are obviously good.

I also think it looks good. You raised a few style points below which I
thought were sensible and won't bother repeating.

Hmph, what does "-a" stand for?  I would have expected to see "-u" that
stands for "update" here.

It was immediately obvious to me as "auto" (I think I even suggested
"-a" in another thread, so maybe that is why it seems so sensible to
me). However, I think as a rule it is nice to always provide a "long"
alternative for every option. With parse-options, there really is no
downside; it is literally s/0/"auto"/ on the option line. And I think
it:

  - lets people who want to illustrate a command in a more readable
    manner do so (e.g., if they are showing it to somebody else)

  - makes it more obvious in the documentation and usage message what
    the command does. That is, I remember commands much better as "this
    is the --auto option, whose meaningful name reminds me that it does
    X, and -a is the obvious shorthand" rather than "-a does X".

  - enables extra parse-options syntax like automatic "--no-" handling
    (though I doubt anyone is likely to use "--no-auto" in this case,
    the point is that it is easier to allow such constructs consistently
    than to try to guess when people might want it)

Which is maybe more argument than you cared to read about this
particular option, but I want to make clear that I think this should be
the case for just about every command-line option we add.

Also it may be better to be more explicit about both the syntax and the
semantics of `<branch>`.  Do you expect "refs/remotes/<name>/master" or
just "master" (I assume the latter)?  Is it an error if the branch does

I thought it was obvious that you would do:

  git remote set-head master

in the same way that you would do:

  git remote add -m master $remote $url

But I suppose clarifying it doesn't hurt.

-Peff

PS There is a thread elsewhere on the list discussing "what can I do to
make life easy for reviewers?". I think this series (and Jay's patches
in general) model many good behaviors: clearly labeled versions,
discussion of what changed in each version, proper threading, and cc'ing
people who have been involved.