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

From: Junio C Hamano <gitster@pobox.com>
Date: Jul 31, 2015, 20:08 UTC
Message-ID: <xmqqr3no3xau.fsf@gitster.dls.corp.google.com>
In-Reply-To: <CAPig+cTX0Wav2QSFi734uLVpjdmwFKzcesgkpmVKCPomMB_HyA@mail.gmail.com>

Eric Sunshine <sunshine@sunshineco.com> writes:

Show 15 quoted lines

> On Fri, Jul 31, 2015 at 3:01 PM, David Turner <dturner@twopensource.com> 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:

Show 6 quoted lines

>  	/*
> -	 * $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.