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