Re: [PATCH v2 00/14] rebase: reset_head() related fixes and improvements

From: Phillip Wood <phillip.wood123@gmail.com>
Date: Jan 26, 2022, 10:53 UTC
Message-ID: <aedee582-7f00-7e12-c378-35b0cbc6b565@gmail.com>
In-Reply-To: <xmqqtufhpk1a.fsf@gitster.g>

Hi Junio

On 09/12/2021 21:04, Junio C Hamano wrote:

Show 14 quoted lines

> "Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes:
> 
>> Thanks for the comments on V1. I have tried to improve the commit messages
>> to explain better the motivation and implications of the changes in this
>> series and I have added some more tests. I have rebased onto v2.34.0 to
>> avoid some merges conflicts.
> 
> Thanks.
> 
> It still is not clear in the cover letter what the overall theme of
> the topic is, and the original cover letter was deliberately vague
> by saying "Fix *some* issues".  Random assortment of changes to
> various code paths, the only common trait among them being they are
> somehow related to reset_head()?

So the theme started out as "convert 'rebase -i' to use reset_head() and stop forking 'git checkout'" which I thought would be one to two patches. Then I started looking at the code and realized that reset_head() needed some work before we could use it for 'rebase -i' and that work ended up dominating the series. I've updated the cover letter as you suggested.

Best Wishes

Phillip

Show 33 quoted lines

>> Cover letter for V1: Fix some issues with the implementation and use of
>> reset_head(). The last patch was previously posted as [1], I have updated
>> the commit message and rebased it onto the fixes in this series. There are a
>> couple of small conflicts merging this into seen, I think they should be
>> easy to resolve (in rebase.c take both sides in reset.c take the changed
>> lines from each side). These patches are based on pw/rebase-of-a-tag-fix
> 
> I've read the patches through.  It does revolve around the use of
> reset_head().  I would have appreciated if the cover letter said
> something along this line:
> 
>      reset.c::reset_head() started its life at ac7f467f
>      (builtin/rebase: support running "git rebase <upstream>",
>      2018-08-07) as a way to detach the HEAD to replay the commits
>      during "git rebase", but over time it learned to do many things,
>      like switching the tip of the branch to another commit,
>      recording the old value of HEAD in ORIG_HEAD while it does so,
>      recording reflog entries for both HEAD and for the branch.
> 
>      The API into the function got clunky and it is harder than
>      necessary for the callers to use the function correctly, which
>      led to a handful of bugs that this series is going to fix.
> 
>         ... list of bugs here ...
> 
>      Later steps of this series revamps the API so that it is harder
>      to use it incorrectly to prevent future bugs.
> 
> Anyway, I think the series is more or less in a very good shape,
> even though a few comments I threw at this round may result in a
> further improvement.
> 
> Thanks for working on this.