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

On Fri, Feb 13, 2009 at 5:09 AM, Junio C Hamano [off-list ref] 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.

quoted

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.

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.