Subject: Re: [PATCH v3 1/2] worktrees: add die_if_shared_symref

From: Junio C Hamano <[email-masked]>
Date: Fri, 31 Jul 2015 13:08:09 -0700

Eric Sunshine <[email-masked]> writes:

> On Fri, Jul 31, 2015 at 3:01 PM, David Turner <[email-masked]> wrote:
>> Add a new function, die_if_shared_symref, which works like
>> die_if_checked_out, but for all references.  Refactor
>> die_if_checked_out to work in terms of die_if_shared_symref.
>>
>> Soon, we will use die_if_shared_symref to protect notes merges in
>> worktrees.
>
> I wonder if the diagnostic:
>
>     'blorp' is already checked out at '/path/name/'
>
> emitted by check_linked_checkout() needs to be adjusted for this
> change. It still makes sense for die_if_checked_out(), but sounds odd
> for die_if_shared_symref().

True.  Lift the dying one callstack up, and make the lower level
helper check_shared_symref() or something that returns NULL (ok) or
that '/path/name' upon an error?

Also I suspect that this comment will become hard to grok after the
commit is actually made:

>  	/*
> -	 * $GIT_COMMON_DIR/HEAD is practically outside
> +	 * $GIT_COMMON_DIR/$symref is practically outside
>  	 * $GIT_DIR so resolve_ref_unsafe() won't work (it
>  	 * uses git_path). Parse the ref ourselves.
>  	 */

A reviewer who is viewing both the pre- and post- text of the patch
can see it used to say HEAD and now it is extended to $symref, but
it would help to have "(e.g. HEAD)" after "...DIR/$symref", I think.