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

[Pratik Karki <predatoramigo@gmail.com>]

On Mon, Jul 9, 2018 at 10:27 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Pratik Karki <predatoramigo@gmail.com> writes:
>
> > In the upcoming builtin rebase, we will have to start by detaching
> > the HEAD, just like shell script version does. Essentially, we have
> > to do the same thing as `git checkout -q <revision>^0 --`, in pure C.
> >
> > The aforementioned functionality was already present in `sequencer.c`
> > in `do_reset()` function. But `do_reset()` performs more than detaching
> > the HEAD, and performs action specific to `sequencer.c`.
> >
> > So this commit refactors out that part from `do_reset()`, and moves it
> > to a new function called `detach_head_to()`. As this function has
> > nothing to do with the sequencer, and everything to do with what `git
> > checkout -q <revision>^0 --` does, we move that function to checkout.c.
> >
> > This refactoring actually introduces a slight change in behavior:
> > previously, the index was locked before parsing the argument to the
> > todo command `reset`, while it now gets locked *after* that, in the
> > `detach_head_to()` function.
> >
> > It does not make a huge difference, and the upside is that this closes
> > a few (unlikely) code paths where the index would not be unlocked upon
> > error.
> >
> > Signed-off-by: Pratik Karki <predatoramigo@gmail.com>
> > ---
>
> Here is a place to say "unchanged since v3" for a change like this
> one that changes neither the proposed log message above nor the
> patch below to help reviewers who have seen the previous round (they
> can stop reading here).  Hopefully there are more reviewers than you
> who write new code, so spending a bit more time to help them spend
> less would be an overall win for the project.

Thanks for the review. I'll keep this in mind, the next time.