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

From: Jeff King <peff@peff.net>
Date: Feb 14, 2009, 00:22 UTC
Message-ID: <20090214002253.GA7769@coredump.intra.peff.net>
In-Reply-To: <7vtz6yabsy.fsf@gitster.siamese.dyndns.org>

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.