On Fri, Mar 3, 2023 at 5:45 PM Junio C Hamano <gitster@xxxxxxxxx> wrote:
> > With this patch, we leverage the original merge commit to handle the most
> > obvious case:
> > - HEAD tree has to match the tree of the first parent of the original merge
> > commit.
> > - MERGE_HEAD tree has to match the tree of the second parent of the original
> > merge commit.
> > - At least one tree in the merge bases of HEAD/MERGE_HEAD has to match
> > a tree in the merge bases of the parent commits of the original merge
> > commit.
>
> The first two conditions look a bit too restrictive for the purpose
> of reusing previous conflict resolution, while I am not sure ...
>
> > If all of those conditions are met, we can safely use the tree of the
> > original merge commit as the resulting tree of this merge that is being
> > attempted at the time.
>
> ...if the "at least one" in the last condition is a safe and
> sensible loosening; if it introduces a mismerge by ignoring bases
> that are different from the original, then it is a bit too bold to
> declare that we can safely use the tree of the original.
>
I think the conditions hold _but_ I will think it through or perhaps
create a few scenarios that we could talk about. Will come back to it
in a few days.
I agree that the current restrictions make it too narrow. Very
restricted scenarios would match at the moment. I will start working
on making this a little bit more accepting to widen the scope.
> What was the motivating usecase behind this new feature? Was it
> more about reusing the structural merge conflict resolution, or
> about the textual merge conflict resolution? For the latter, after
> doing the usual three-way file-level merge and seeing a conflicted
> textual merge, requiring the match of the blob objects for only these
> conflicted paths and taking the previous merge result may give you a
> safe way to raising the chance to find more reusable merges.
Usercase can be at the moment trying to rebase (with merges) on top of
an exact base copy. In cases like this, git just crashes on merge
commits. An easy example:
git checkout v2.38.0
git commit --amend --no-edit
git rebase --rebase-merges --onto HEAD v2.38.0 v2.39.0
>
> > +static void load_tree_oids(struct oid_array *oids, struct commit_list *bases)
> > +{
> > + struct commit_list *i;
> > + for (i = bases; i; i = i->next)
>
> Using 'i' for a pointer looks novel. Don't.
>
Thanks for the comments on code. At least it doesn't sound like I
messed up big time.... so far.
>
> I somehow thought that reverting the trashed working tree and the
> index to their original state was not the responsibility for a merge
> strategy but for the caller? Shouldn't this restoration be on the
> caller's side?
>
> Oh, has this code even touched anything in the working tree and the
> index at this point? It looks more like everything we did above in
> order to punt by returning 2 was to see if the condition for us to
> reuse the resulting tree holds and nothing else.
>
> Ah, "restore()" is misnamed, perhaps? I thought it was about "oh,
> we made a mess and need to go back to the state that was given to us
> before failing", but is this the real "ok, the condition holds and
> we can just reuse the tree from the previous merge"? Then it makes
> sense for the code to attempt to check out that tree and return 2
> when it fails. Only the function name was misleading.
>
calling _git restore_ to do that hence _restore_ :-) But it's ok. I
can give it a better name.
