Re: en/merge-restore-to-pristine (Was: Re: What's cooking in git.git (Jul 2022, #04; Wed, 13))

From: ZheNing Hu <adlternative@gmail.com>
Date: Jul 17, 2022, 16:58 UTC
Message-ID: <CAOLTT8R3y4m_ZSuT198_7X16mCu+BtbuMi5756PKs+fAzxpd8A@mail.gmail.com>
In-Reply-To: <CABPp-BFzehrH19zRSbLBB42nEs50_UCcfV2B22ZJFYtPCX=Zsw@mail.gmail.com>

Elijah Newren <newren@gmail.com> 于2022年7月17日周日 11:46写道：

Show 28 quoted lines

>
> Hi ZheNing,
>
> On Wed, Jul 13, 2022 at 7:36 PM Junio C Hamano <gitster@pobox.com> wrote:
> >
> > * en/merge-restore-to-pristine (2022-06-21) 6 commits
> >  - merge: do not exit restore_state() prematurely
> >  - merge: ensure we can actually restore pre-merge state
> >  - merge: make restore_state() restore staged state too
> >  - merge: fix save_state() to work when there are racy-dirty files
> >  - merge: remove unused variable
> >  - t6424: make sure a failed merge preserves local changes
> >
> >  When "git merge" finds that it cannot perform a merge, it should
> >  restore the working tree to the state before the command was
> >  initiated, but in some corner cases it didn't.
> >
> >  Needs review.
> >  source: <pull.1231.v2.git.1655621424.gitgitgadget@gmail.com>
>
> Looks like other reviewers aren't stepping forward (this has been in
> "Needs review" for the last 6 "What's cooking" reports), which may
> suggest others aren't as interested in this fix.  Since this was for
> an issue you reported, and which you even volunteered to help
> shepherd[1], perhaps you could step forward as a reviewer even if
> you're not that familiar with the code?  Some things to look at and
> report on:
>

Sorry that I missed patch update before. And this is my first time as a reviewer. I may leak some experience :)

>   * Does it fix the issue?  (You reported that v1 did, again at [1],
> but perhaps you could retest for v2?)

Yes, I have checked the test, it is good enough to solve my problem.

>   * Does it appear I've addressed the issues Junio brought up about v1?

Yes, Junio said restore_state() will not be called correctly because we are using only one merge strategy, which has been solved by the patch: "merge: ensure we can actually restore pre-merge state".

Show 7 quoted lines

>   * Even if you can't analyze the changes deeply, you can respond to
> my patches with a "walk through" where you try to explain what the
> different hunks of the patches are doing in your own words.  Even
> folks unfamiliar with code areas can sometimes catch simple mistakes
> that way, and even if you catch nothing, it means there's another
> person more familiar with that code area.
>

I have checked(walk through) them carefully. Look at them :)

> I've had a little more time lately, so if you or someone does catch
> something in the review, I can try to update the series.
>
> [1] https://lore.kernel.org/git/CAOLTT8RpGGioOyaMw5tkeWXmHpOaBW9UH8JghUvBRQ50ZcDdYQ@mail.gmail.com/

Thanks

ZheNing Hu