Re: [PATCH v2 3/4] sequencer: refactor the code to detach HEAD to checkout.c

From: Junio C Hamano <gitster@pobox.com>
Date: Jul 3, 2018, 21:26 UTC
Message-ID: <xmqqmuv8nh7v.fsf@gitster-ct.c.googlers.com>
In-Reply-To: <20180702091509.15950-4-predatoramigo@gmail.com>

Pratik Karki <predatoramigo@gmail.com> writes:

Show 12 quoted lines

> The motivation behind this commit is to extract the core part of
> do_reset() from sequencer.c and move it to a new detach_head_to()
> function in checkout.c.
>
> Here the index only gets locked after performing the first part of
> `do_reset()` rather than before which essentially derives the `oid`
> from the specified label/name passed to the `do_reset()` function.
> It also fixes two bugs: there were two `return error()` statements in
> the `[new root]` case that would have failed to unlock the index.
>
> The new function will be used in the next commit by the builtin rebase,
> to perform the initial checkout.

I like the split of do_reset() into this "detach-to" part that is less specific to replaying an existing commit and the rest, which as you said incidentally corrects the locking issue.

It is not clear to me why checkout.[ch] is a better place to house this function, though.