Re: [PATCH] Add option -b/--branch to clone for select a new HEAD

From: Jeff King <peff@peff.net>
Date: Aug 26, 2009, 16:10 UTC
Message-ID: <20090826161059.GC32741@coredump.intra.peff.net>
In-Reply-To: <20090826155029.GA5750@atjola.homenet>

On Wed, Aug 26, 2009 at 05:50:29PM +0200, Björn Steinbrink wrote:

> Using guess_remote_head() seems pretty wrong. With -b given, you don't
> want to guess anymore, you _know_ which one you want. Unfortunately, I
> don't see a straight-forward way to handle that (but I'm totally
> clueless about the code, so don't let me scare you ;-)).

Thanks for pointing this out, Björn (I really should have noticed it on first review, but I guess many eyes, shallow bugs, etc. :) ).

This code is a little bit confusing, so let me explain:

  - we look up the remote HEAD, getting its commit sha1. If the protocol
    supports it, we also get its symref information.

  - we then pass the result to guess_remote_head. _If_ we have symref
    information, then we can quit immediately, as the symref contains
    what we want. If it doesn't, then we proceed with trying to match up
    the commit sha1 with one of the other refs.

So if you want to create a "remote_head" object via "-b" which acts as if it was the remote HEAD, you would need to actually create a new ref object and set the "symref" field appropriately.

But I don't think there is any need to do that here. We simply want to avoid calling guess_remote_head at all, since we know there is nothing to guess at. We do still need to know whether remote_head is non-NULL later, though.

So I think the code should probably look like this (totally untested):

  remote_head = find_ref_by_name(refs, "HEAD");
  if (option_branch) {
          strbuf_addf(&branch_head, "%s%s", src_ref_prefix, option_branch);
          head_points_at = find_ref_by_name(refs, branch_head.buf);
          if (!head_points_at)
                  warning("remote branch not found, etc");
  }
  if (!head_points_at)
    head_points_at = guess_remote_head(remote_head, mapped_refs, 0);

and then initialize head_points_at to NULL instead of remote_head.

-Peff