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

Nguyen Thai Ngoc Duy [off-list ref] writes:

2012/1/10 Junio C Hamano [off-list ref]:

quoted

quoted

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.

On Tue, Jan 10, 2012 at 11:59 AM, Junio C Hamano [off-list ref] wrote:

quoted

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?

yeah, I split the first two into update_head() but dropped it for some
reasons I don't remember. That would make the main function easier to
follow. I'll look at it again.
-- 
Duy