Quantcast

Small rerere in rebase regression

classic Classic list List threaded Threaded
9 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Small rerere in rebase regression

Johannes Sixt-3
Even though I operate with jc/rerere-multi for some time now,
I noticed this regression only recently. Watch out for the
duplicated "Recorded preimage" message:

gittest@side:1078> strace -f -o /tmp/git-rebase-conflict.strace git rebase -i master
error: could not apply 59f92e0... side

When you have resolved this problem, run "git rebase --continue".
If you prefer to skip this patch, run "git rebase --skip" instead.
To check out the original branch and stop rebasing, run "git rebase --abort".

Recorded preimage for 'a'
Recorded preimage for 'a'
Could not apply 59f92e09c890c08442eb1df91eaecec3d1656c86... side

According to the strace, the first "Recorded preimage" is from
git cherry-pick, the second is from "git rerere" in this section
in git-rebase--interactive.sh:

die_with_patch () {
        echo "$1" > "$state_dir"/stopped-sha
        make_patch "$1"
        git rerere
        die "$2"
}

The buglet bisects to

629716d256a792179325c2cc7945bb2d81dda8c2 is the first bad commit
commit 629716d256a792179325c2cc7945bb2d81dda8c2
Author: Junio C Hamano <[hidden email]>
Date:   Thu Jul 30 15:49:18 2015 -0700

    rerere: do use multiple variants
   
    This enables the multiple-variant support for real.  Multiple
    conflicts of the same shape can have differences in contexts where
    they appear, interfering the replaying of recorded resolution of one
    conflict to another, and in such a case, their resolutions are
    recorded as different variants under the same conflict ID.
   
    We still need to adjust garbage collection codepaths for this
    change, but the basic "replay" functionality is functional with
    this change.
   
    Signed-off-by: Junio C Hamano <[hidden email]>

I'm not sure whether the new behavior is a defect in rerere.c or a
consequence of the extra rerere call in interactive rebase...

-- Hannes
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Small rerere in rebase regression

Junio C Hamano
Johannes Sixt <[hidden email]> writes:

> I'm not sure whether the new behavior is a defect in rerere.c or a
> consequence of the extra rerere call in interactive rebase...

Interesting.  When running an unnecessary "git rerere" (because it
already was run and recorded the preimage), we used to be silent.
With the updated code, we say something.

At least it seems that we do not record the same preimage as a
different variant, so the regression is hopefully merely cosmetic.

I do not think it is a crime to say "git rerere" repeatedly without
changing anything, so I do not think "rebase -i"'s call to rerere
in die_with_patch is wrong, even though some calls to it seem to be
made after seeing a "git merge" fail (which means we know 'rerere'
has been run already).

I think all plumbing that can turn an index into unmerged state has
a call to git_rerere() at the end (the only obvious exception being
"update-index" that lets you stuff higher stage entries to the
index), so as long as "rebase -i" uses them correctly, I suspect
that die_with_patch shouldn't have to have a call to "git rerere".
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Small rerere in rebase regression

Johannes Sixt-3
Am 23.05.2016 um 21:43 schrieb Junio C Hamano:

> Johannes Sixt <[hidden email]> writes:
>
>> I'm not sure whether the new behavior is a defect in rerere.c or a
>> consequence of the extra rerere call in interactive rebase...
>
> Interesting.  When running an unnecessary "git rerere" (because it
> already was run and recorded the preimage), we used to be silent.
> With the updated code, we say something.
>
> At least it seems that we do not record the same preimage as a
> different variant, so the regression is hopefully merely cosmetic.
>
> I do not think it is a crime to say "git rerere" repeatedly without
> changing anything, so I do not think "rebase -i"'s call to rerere
> in die_with_patch is wrong, even though some calls to it seem to be
> made after seeing a "git merge" fail (which means we know 'rerere'
> has been run already).
>
> I think all plumbing that can turn an index into unmerged state has
> a call to git_rerere() at the end (the only obvious exception being
> "update-index" that lets you stuff higher stage entries to the
> index), so as long as "rebase -i" uses them correctly, I suspect
> that die_with_patch shouldn't have to have a call to "git rerere".

I also come to the conclusion that die_with_patch shouldn't have to have
a call to "git rerere". die_with_patch can be called after "git
cherry-pick", "git merge", "git commit", all of which have their own
rerere() invocation.

However, calling "git rerere" after a failed "git commit" may be
destructive: it would record a resolution even though the commit has not
be completed. Think of an squash commit being aborted because the user
notices an error in the last minute. If that error is in a conflict
resolution, that wrong resolution would be recorded.

-- Hannes

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Small rerere in rebase regression

Junio C Hamano
Johannes Sixt <[hidden email]> writes:

> I also come to the conclusion that die_with_patch shouldn't have to
> have a call to "git rerere". die_with_patch can be called after "git
> cherry-pick", "git merge", "git commit", all of which have their own
> rerere() invocation.
>
> However, calling "git rerere" after a failed "git commit" may be
> destructive: it would record a resolution even though the commit has
> not be completed. Think of an squash commit being aborted because the
> user notices an error in the last minute. If that error is in a
> conflict resolution, that wrong resolution would be recorded.

So, the behaviour change you observed uncovered a small bug in
"rebase -i" that was covered by the old limitation of "rerere" that
refrained from creating preimage when there already is one?

I think removing the call to "git rerere" there is a safe and
sensible thing regardless, but perhaps authors of "rebase -i" had
their own reasons.  I dunno (it is unlikely I'll have a chance to do
blame and digging today).
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Small rerere in rebase regression

Johannes Schindelin
Hi,

On Mon, 23 May 2016, Junio C Hamano wrote:

> Johannes Sixt <[hidden email]> writes:
>
> > I also come to the conclusion that die_with_patch shouldn't have to
> > have a call to "git rerere". die_with_patch can be called after "git
> > cherry-pick", "git merge", "git commit", all of which have their own
> > rerere() invocation.
> >
> > However, calling "git rerere" after a failed "git commit" may be
> > destructive: it would record a resolution even though the commit has
> > not be completed. Think of an squash commit being aborted because the
> > user notices an error in the last minute. If that error is in a
> > conflict resolution, that wrong resolution would be recorded.
>
> So, the behaviour change you observed uncovered a small bug in "rebase
> -i" that was covered by the old limitation of "rerere" that refrained
> from creating preimage when there already is one?
>
> I think removing the call to "git rerere" there is a safe and sensible
> thing regardless, but perhaps authors of "rebase -i" had their own
> reasons.  I dunno (it is unlikely I'll have a chance to do blame and
> digging today).

I introduced this in ecfe72ff658124ea301ec7b2bb8b987689303685, as a (late)
answer to f131dd492f098f9f565df93df13e35c734284590.

Hannes, could you quickly test whether
https://github.com/dscho/git/tree/interactive-rebase calls rerere twice,
too? (Please call interactive rebase with the GIT_USE_REBASE_HELPER=true
to avoid running the interactive rebase twice.)

I have a hunch that it does not call rerere twice, which would be a nice
bonus in that patch thicket (so far, I split the patches into seven patch
series, in addition to the ones I already sent, still have to do one
thorough review before I'll continue submitting them).

Ciao,
Dscho
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Small rerere in rebase regression

Johannes Sixt-3
Am 24.05.2016 um 15:18 schrieb Johannes Schindelin:
> Hannes, could you quickly test whether
> https://github.com/dscho/git/tree/interactive-rebase calls rerere twice,
> too? (Please call interactive rebase with the GIT_USE_REBASE_HELPER=true
> to avoid running the interactive rebase twice.)
>
> I have a hunch that it does not call rerere twice, which would be a nice
> bonus in that patch thicket

It prints the message only once:

gittest@master:1007> GIT_USE_REBASE_HELPER=true git rebase -i side
error: could not apply fa62fea... master

When you have resolved this problem, run "git rebase --continue".
If you prefer to skip this patch, run "git rebase --skip" instead.
To check out the original branch and stop rebasing, run "git rebase
--abort".

Recorded preimage for 'a'
Could not apply fa62fea... mastergittest@master|REBASE-i 1/1:1008> 5~

(Take note of the missing LF at the end of the message.)

Can this result be interpreted as another indication that the "git
rerere" call in die_with_patch can be removed, or are the two git-rebase
implementations too different to be comparable?

-- Hannes

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Small rerere in rebase regression

Johannes Sixt-3
In reply to this post by Junio C Hamano
Am 24.05.2016 um 00:11 schrieb Junio C Hamano:

> Johannes Sixt <[hidden email]> writes:
>> However, calling "git rerere" after a failed "git commit" may be
>> destructive: it would record a resolution even though the commit has
>> not be completed. Think of an squash commit being aborted because the
>> user notices an error in the last minute. If that error is in a
>> conflict resolution, that wrong resolution would be recorded.
>
> So, the behaviour change you observed uncovered a small bug in
> "rebase -i" that was covered by the old limitation of "rerere" that
> refrained from creating preimage when there already is one?

I had a closer look, and found that the failure mode I described does
not occur: after having resolved a commit, the user can call "git rebase
--continue", but the "git commit" invocation that occurs there is
guarded with a mere 'die', not 'die_with_patch'.

All other "git commit" guarded with 'die_with_patch' are either not
interactive or are after a non-conflicting merge or cherry-pick (and so
there was no opportunity to resolve a conflict).

-- Hannes

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Small rerere in rebase regression

Johannes Schindelin
In reply to this post by Johannes Sixt-3
Hi Hannes,

On Tue, 24 May 2016, Johannes Sixt wrote:

> Am 24.05.2016 um 15:18 schrieb Johannes Schindelin:
> > Hannes, could you quickly test whether
> > https://github.com/dscho/git/tree/interactive-rebase calls rerere twice,
> > too? (Please call interactive rebase with the GIT_USE_REBASE_HELPER=true
> > to avoid running the interactive rebase twice.)
> >
> > I have a hunch that it does not call rerere twice, which would be a nice
> > bonus in that patch thicket
>
> It prints the message only once:

Thanks for confirming.

> Could not apply fa62fea... mastergittest@master|REBASE-i 1/1:1008> 5~
>
> (Take note of the missing LF at the end of the message.)

Oops. Good catch, thank you! I just fixed this:

        https://github.com/dscho/git/commit/0393d7bb2d

> Can this result be interpreted as another indication that the "git rerere"
> call in die_with_patch can be removed, or are the two git-rebase
> implementations too different to be comparable?

The code used by the rebase--helper is not *quite* the same as the entire
cherry-pick code path. For one, we run the sequencer directly, without any
of cherry-pick's option parsing.

Having said that, yes, we inherit sequencer's rerere() call that was part
of cherry-pick originally ever since aa1a011 (Make cherry-pick use rerere
for conflict resolution., 2008-08-10) (amd which was moved to
builtin/revert.c in 81b50f3 (Move 'builtin-*' into a 'builtin/'
subdirectory, 2010-02-22) and refactored out into the sequencer in 043a449
(sequencer: factor code out of revert builtin, 2012-01-11)).

I did notice that rerere() call when implementing rebase -i's
functionality in the sequencer, and dropped the extra call in
error_with_patch() (the libified version of rebase -i's die_with_patch
function).

In short: yes, the explicit `git rerere` call can be dropped, that is
essentially what I did in the rebase--helper branch.

Ciao,
Dscho

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Small rerere in rebase regression

Johannes Schindelin
Hi Hannes,

On Wed, 25 May 2016, Johannes Schindelin wrote:

> On Tue, 24 May 2016, Johannes Sixt wrote:
>
> > Could not apply fa62fea... mastergittest@master|REBASE-i 1/1:1008> 5~
> >
> > (Take note of the missing LF at the end of the message.)
>
> Oops. Good catch, thank you! I just fixed this:
>
> https://github.com/dscho/git/commit/0393d7bb2d

Argh. I fixed it in https://github.com/dscho/git/commit/0704d6f425 of
course.

Ciao,
Dscho
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Loading...