Re: [PATCH v2 3/6] clone: factor out checkout code

From: Junio C Hamano <gitster@pobox.com>
Date: Jan 10, 2012, 04:59 UTC
Message-ID: <7vhb04b1bc.fsf@alter.siamese.dyndns.org>
In-Reply-To: <CACsJy8DZpA0sQ6ZYjgrp8PsRTsYm0nOfSXcDOEhB2TRjqwbM0Q@mail.gmail.com>

Nguyen Thai Ngoc Duy <pclouds@gmail.com> writes:

Show 11 quoted lines

> 2012/1/10 Junio C Hamano <gitster@pobox.com>:
>>
>>> Read HEAD from disk instead of relying on local variable
>>> our_head_points_at, so that if earlier code fails to make HEAD
>>> properly, it'll be detected.
>>
>> The end result might be more or less the same with your patch from the
>> end-user's point of view, but "if earlier code fails", shouldn't you
>> detect and diagnose it right there?
>
> Sure, but another fence does not harm.

But that is not "another" fence but is the _only_ fence, as you do not check after running update_ref of "HEAD".

> There's also one thing I missed in the commit message that it makes
> update head code and checkout code more independent. Update head code
> does not need to maintain our_head_points_at at the end for checkout
> anymore.

I like that reasoning in general. The logic ought to be:

 - Learn what the remote has;

 - Combine it with --branch parameter, determine what local branch our
   head _should_ point at;

 - Make our head point at it, and check it out.

I wonder if we can somehow make the above logic more clear in the code. Perhaps the first two could be made into a single helper function "decide_local_branch()", and the third would be the "checkout()" function in your patch, updated to take "const char *" parameter or something?

> The lack of HEAD probably won't happen because HEAD is created by
> default in init-db. This is mainly to catch invalid HEAD (like putting
> "refs/tags/something" in HEAD).

Sorry; what I meant by "lack" was "... if earlier code fails to make HEAD properly" case.