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

From: Jay Soffian <jaysoffian@gmail.com>
Date: Feb 13, 2009, 10:21 UTC
Message-ID: <76718490902130221q3114fbdbt89692e79158530ff@mail.gmail.com>
In-Reply-To: <7vtz6yabsy.fsf@gitster.siamese.dyndns.org>

On Fri, Feb 13, 2009 at 5:09 AM, Junio C Hamano <gitster@pobox.com> wrote:

> Calling the subcommand a "verb" is somewhat new, though.  Existing
> documentation for git commands that take multiple actions seem to call
> them subcommands, including "git-remote.txt" itself.

Okay.

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

--automatic -- as in figure out the name automatically from the other side.

> Also it may be better to be more explicit about both the syntax and the
> semantics of `<branch>`.

Okay.

> Do you expect "refs/remotes/<name>/master" or
> just "master" (I assume the latter)?

Yes, the latter.  If you did the wrong thing the error ought clue you in:

$ ./git remote set-head origin refs/remotes/origin/master error: Not a valid ref: refs/remotes/origin/refs/remotes/origin/master

> Is it an error if the branch does
> not exist in the specified hierarchy?

Yes it is an error per-above. Well, at least on-top of next it is.

> Can you force to set to a branch
> that does not exist in your tracking side (yet) but you know exists on the
> remote side already?

No.

Show 14 quoted lines

>> diff --git a/builtin-remote.c b/builtin-remote.c
>> index 465c87a..677e20e 100644
>> --- a/builtin-remote.c
>> +++ b/builtin-remote.c
>> @@ -658,7 +659,8 @@ static void free_remote_ref_states(struct ref_states *states)
>>       string_list_clear(&states->new, 0);
>>       string_list_clear(&states->stale, 0);
>>       string_list_clear(&states->tracked, 0);
>> -     free(states->head_name);
>> +     if (states->head_name)
>> +             free(states->head_name);
>>  }
>
> Regression?

Indeed.

> set_head()?

Yep.

Show 14 quoted lines

> The code will scale better, especially for a young subcommand that may acquire
> new options, if the check is done by each codepath that deals with a
> specific option to do this kind of check.  That is, e.g.
>
>        if (opt_delete) {
>                error if the arg is not remote (alone)
>                do the "delete" thing
>        } else if (opt_update) {
>                error if the arg is not remote (alone)
>                do the "update" thing
>        } else {
>                error if the args are not (remote, branch)
>                do the "set" thing
>        }

Got it. I really am trying to match existing code, but it seems the standards have gotten higher, so I need to do better than existing code.

Thanks,

j.