Unreliable 'git rebase --onto'
diff mbox series

Message ID 20200108214349.GA17624@lxhi-065.adit-jv.com
State New
Headers show
Series
  • Unreliable 'git rebase --onto'
Related show

Commit Message

Eugeniu Rosca Jan. 8, 2020, 9:43 p.m. UTC
Hello Git community,

Below is a simple reproduction scenario for what looks to be a bug (?)
in 'git rebase --onto' (v2.25.0-rc1-19-g042ed3e048af).

I would appreciate your confirmation of the misbehavior.
If the behavior is correct/expected, I would appreciate some feedback
how to avoid it in future, since it occurs with the default parameters.

1. git clone https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git

2. ### Cherry pick an upstream commit, to contrast the results with
   'git rebase --onto':
   $ git checkout -b v4.18-cherry-pick v4.18
   $ git cherry-pick 463fa44eec2fef50
   Auto-merging drivers/input/touchscreen/atmel_mxt_ts.c
   warning: inexact rename detection was skipped due to too many files.
   warning: you may want to set your merge.renamelimit variable to at least 7216 and retry the command.
   [v4.18-cherry-pick bd142b45bf3a] Input: atmel_mxt_ts - disable IRQ across suspend
    Author: Evan Green <evgreen@chromium.org>
    Date: Wed Oct 2 14:00:21 2019 -0700
    1 file changed, 4 insertions(+)

3. ### In spite of the warning, the result matches the original commit:
   $ vimdiff <(git show 463fa44eec2fef50) <(git show v4.18-cherry-pick)

4. ### Now, backport the same commit via 'git rebase --onto'
   $ git rebase --onto v4.18 463fa44eec2fef50~ 463fa44eec2fef50
   First, rewinding head to replay your work on top of it...
   Applying: Input: atmel_mxt_ts - disable IRQ across suspend

5. ### The result is different:
   $ git branch v4.18-rebase-onto
   $ git diff v4.18-cherry-pick v4.18-rebase-onto


In a nutshell, purely from user's perspective:
 - I get a warning from 'git cherry pick', with perfect results
 - I get no warning from 'git rebase --onto', with wrong results

Does git still behave expectedly? TIA!

Comments

SZEDER Gábor Jan. 8, 2020, 10:35 p.m. UTC | #1
On Wed, Jan 08, 2020 at 10:43:49PM +0100, Eugeniu Rosca wrote:
> Hello Git community,
> 
> Below is a simple reproduction scenario for what looks to be a bug (?)
> in 'git rebase --onto' (v2.25.0-rc1-19-g042ed3e048af).
> 
> I would appreciate your confirmation of the misbehavior.
> If the behavior is correct/expected, I would appreciate some feedback
> how to avoid it in future, since it occurs with the default parameters.
> 
> 1. git clone https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git
> 
> 2. ### Cherry pick an upstream commit, to contrast the results with
>    'git rebase --onto':
>    $ git checkout -b v4.18-cherry-pick v4.18
>    $ git cherry-pick 463fa44eec2fef50
>    Auto-merging drivers/input/touchscreen/atmel_mxt_ts.c
>    warning: inexact rename detection was skipped due to too many files.
>    warning: you may want to set your merge.renamelimit variable to at least 7216 and retry the command.
>    [v4.18-cherry-pick bd142b45bf3a] Input: atmel_mxt_ts - disable IRQ across suspend
>     Author: Evan Green <evgreen@chromium.org>
>     Date: Wed Oct 2 14:00:21 2019 -0700
>     1 file changed, 4 insertions(+)
> 
> 3. ### In spite of the warning, the result matches the original commit:
>    $ vimdiff <(git show 463fa44eec2fef50) <(git show v4.18-cherry-pick)
> 
> 4. ### Now, backport the same commit via 'git rebase --onto'
>    $ git rebase --onto v4.18 463fa44eec2fef50~ 463fa44eec2fef50
>    First, rewinding head to replay your work on top of it...
>    Applying: Input: atmel_mxt_ts - disable IRQ across suspend
> 
> 5. ### The result is different:
>    $ git branch v4.18-rebase-onto
>    $ git diff v4.18-cherry-pick v4.18-rebase-onto
> 
> diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c b/drivers/input/touchscreen/atmel_mxt_ts.c
> index b45958e89cc5..2345b587662b 100644
> --- a/drivers/input/touchscreen/atmel_mxt_ts.c
> +++ b/drivers/input/touchscreen/atmel_mxt_ts.c
> @@ -3139,8 +3139,6 @@ static int __maybe_unused mxt_suspend(struct device *dev)
>  
>  	mutex_unlock(&input_dev->mutex);
>  
> -	disable_irq(data->irq);
> -
>  	return 0;
>  }
>  
> @@ -3162,6 +3160,8 @@ static int __maybe_unused mxt_resume(struct device *dev)
>  
>  	mutex_unlock(&input_dev->mutex);
>  
> +	disable_irq(data->irq);
> +
>  	return 0;
>  }
> 
> 
> In a nutshell, purely from user's perspective:
>  - I get a warning from 'git cherry pick', with perfect results
>  - I get no warning from 'git rebase --onto', with wrong results
> 
> Does git still behave expectedly? TIA!

This is a known issue with the 'am' backend of 'git rebase'.

The good news is that work is already well under way to change the
default backend from 'am' to 'merge', which will solve this issue.
From the log message of aa523de170 (rebase: change the default backend
from "am" to "merge", 2019-12-24):

  The am-backend drops information and thus limits what we can do:
  [...]
    * reduction in context from only having a few lines beyond those
      changed means that when context lines are non-unique we can apply
      patches incorrectly.[2]
  [...]
  [2] https://lore.kernel.org/git/CABPp-BGiu2nVMQY_t-rnFR5GQUz_ipyEE8oDocKeO+>

Alas, there is unexpected bad news: with that commit the runtime of
your 'git rebase --onto' command goes from <1sec to over 50secs.
Cc-ing Elijah, author of that patch...
Elijah Newren Jan. 9, 2020, 12:55 a.m. UTC | #2
user 0m9.644s
sys 0m3.620s
On Wed, Jan 8, 2020 at 2:36 PM SZEDER Gábor <szeder.dev@gmail.com> wrote:
>
> On Wed, Jan 08, 2020 at 10:43:49PM +0100, Eugeniu Rosca wrote:
> > Hello Git community,
> >
> > Below is a simple reproduction scenario for what looks to be a bug (?)
> > in 'git rebase --onto' (v2.25.0-rc1-19-g042ed3e048af).
> >
> > I would appreciate your confirmation of the misbehavior.
> > If the behavior is correct/expected, I would appreciate some feedback
> > how to avoid it in future, since it occurs with the default parameters.
> >
> > 1. git clone https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git
> >
> > 2. ### Cherry pick an upstream commit, to contrast the results with
> >    'git rebase --onto':
> >    $ git checkout -b v4.18-cherry-pick v4.18
> >    $ git cherry-pick 463fa44eec2fef50
> >    Auto-merging drivers/input/touchscreen/atmel_mxt_ts.c
> >    warning: inexact rename detection was skipped due to too many files.
> >    warning: you may want to set your merge.renamelimit variable to at least 7216 and retry the command.

Lots of renames...

> >    [v4.18-cherry-pick bd142b45bf3a] Input: atmel_mxt_ts - disable IRQ across suspend
> >     Author: Evan Green <evgreen@chromium.org>
> >     Date: Wed Oct 2 14:00:21 2019 -0700
> >     1 file changed, 4 insertions(+)
> >
> > 3. ### In spite of the warning, the result matches the original commit:
> >    $ vimdiff <(git show 463fa44eec2fef50) <(git show v4.18-cherry-pick)
> >
> > 4. ### Now, backport the same commit via 'git rebase --onto'
> >    $ git rebase --onto v4.18 463fa44eec2fef50~ 463fa44eec2fef50
> >    First, rewinding head to replay your work on top of it...
> >    Applying: Input: atmel_mxt_ts - disable IRQ across suspend
> >
> > 5. ### The result is different:
> >    $ git branch v4.18-rebase-onto
> >    $ git diff v4.18-cherry-pick v4.18-rebase-onto
> >
> > diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c b/drivers/input/touchscreen/atmel_mxt_ts.c
> > index b45958e89cc5..2345b587662b 100644
> > --- a/drivers/input/touchscreen/atmel_mxt_ts.c
> > +++ b/drivers/input/touchscreen/atmel_mxt_ts.c
> > @@ -3139,8 +3139,6 @@ static int __maybe_unused mxt_suspend(struct device *dev)
> >
> >       mutex_unlock(&input_dev->mutex);
> >
> > -     disable_irq(data->irq);
> > -
> >       return 0;
> >  }
> >
> > @@ -3162,6 +3160,8 @@ static int __maybe_unused mxt_resume(struct device *dev)
> >
> >       mutex_unlock(&input_dev->mutex);
> >
> > +     disable_irq(data->irq);
> > +
> >       return 0;
> >  }
> >
> >
> > In a nutshell, purely from user's perspective:
> >  - I get a warning from 'git cherry pick', with perfect results
> >  - I get no warning from 'git rebase --onto', with wrong results
> >
> > Does git still behave expectedly? TIA!
>
> This is a known issue with the 'am' backend of 'git rebase'.
>
> The good news is that work is already well under way to change the
> default backend from 'am' to 'merge', which will solve this issue.
> From the log message of aa523de170 (rebase: change the default backend
> from "am" to "merge", 2019-12-24):
>
>   The am-backend drops information and thus limits what we can do:
>   [...]
>     * reduction in context from only having a few lines beyond those
>       changed means that when context lines are non-unique we can apply
>       patches incorrectly.[2]
>   [...]
>   [2] https://lore.kernel.org/git/CABPp-BGiu2nVMQY_t-rnFR5GQUz_ipyEE8oDocKeO+>
>
> Alas, there is unexpected bad news: with that commit the runtime of
> your 'git rebase --onto' command goes from <1sec to over 50secs.
> Cc-ing Elijah, author of that patch...

I see slowdown, but not nearly as big as you report:

$ git checkout -b v4.18-cherry-pick v4.18
$ time git cherry-pick 463fa44eec2fef50
Auto-merging drivers/input/touchscreen/atmel_mxt_ts.c
warning: inexact rename detection was skipped due to too many files.
warning: you may want to set your merge.renamelimit variable to at
least 7216 and retry the command.
[v4.18-cherry-pick 88d39cdf3e80] Input: atmel_mxt_ts - disable IRQ
across suspend
 Author: Evan Green <evgreen@chromium.org>
 Date: Wed Oct 2 14:00:21 2019 -0700
 1 file changed, 4 insertions(+)

real 0m1.110s
user 0m0.956s
sys 0m0.284s
$ time git rebase --onto v4.18 463fa44eec2fef50~ 463fa44eec2fef5
First, rewinding head to replay your work on top of it...
Applying: Input: atmel_mxt_ts - disable IRQ across suspend

real 0m1.643s
user 0m1.296s
sys 0m0.264s
$ time git rebase -m --onto v4.18 463fa44eec2fef50~ 463fa44eec2fef50
warning: inexact rename detection was skipped due to too many files.
warning: you may want to set your merge.renamelimit variable to at
least 7216 and retry the command.
Successfully rebased and updated detached HEAD.

real 0m13.305s
user 0m9.644s
sys 0m3.620s




Interestingly, turning off rename detection only speeds it up a little bit:
$ time git rebase -m -Xno-renames --onto v4.18 463fa44eec2fef50~
463fa44eec2fef50
Successfully rebased and updated detached HEAD.

real 0m11.955s
user 0m8.732s
sys 0m3.424s


This is an interesting testcase; I'm going to try to find some time to
dig in further.
Eugeniu Rosca Jan. 9, 2020, 10:53 a.m. UTC | #3
Hi Elijah, hi Szeder,

On Wed, Jan 08, 2020 at 02:06:22PM -0800, Elijah Newren wrote:
> This looks like a known bug in rebase, in particular in the am-backend that
> rebase uses by default.  If I'm correct that it's just a context region
> issue, then this is the same bug that was recently discussed at
> https://lore.kernel.org/git/CAN_72e2h2avv-U9BVBYqXVKiC+5kHy-pjejyMSD3X22uRXE39g@mail.gmail.com/.
> The current plan is to switch the default over to the merge backend (the
> same machinery that cherry-pick uses), which doesn't suffer from the same
> shortcomings (you can see the current work being done in this area at
> https://lore.kernel.org/git/pull.679.v3.git.git.1577217299.gitgitgadget@gmail.com/
> ).

Thank you for your feedback and references, here and in [*].

Once hit by this or similar issues, I think there is high chance for
people to go through the same feelings as described by Pavel in [**]:

  ---
  That's so scary that I'm going to stop using "git rebase" for now.
  ---

Some years ago I was hit by 'git merge' producing slightly different
results compared to 'git rebase --onto' and 'git cherry-pick A..B'
(maybe I can come up with a reproduction scenario for that too).

Since then, I usually contrast the outcomes of merging, cherry-picking
and rebasing, to make sure they match, but that's painful and
time-consuming.

> In the mean time, you can pass the -m flag to rebase to avoid these types
> of problems.  In fact, if you could retry with -m you may be able to
> confirm whether it's the same issue.

Indeed, neither `git rebase -m` nor `git rebase -i` exhibit the problem.

[*] https://lore.kernel.org/git/CABPp-BHsy75UGm4wTOP2_AYik_dZi-_BxtAn-hyi-ZrNRRWGuw@mail.gmail.com/T/#m1cbf80ef56c260a626146d61291d7fbabd108f1b
[**] https://lore.kernel.org/git/CAN_72e2h2avv-U9BVBYqXVKiC+5kHy-pjejyMSD3X22uRXE39g@mail.gmail.com/

Thanks again.
Eugeniu Rosca Jan. 9, 2020, 11:13 a.m. UTC | #4
Hi Szeder,

On Wed, Jan 08, 2020 at 11:35:57PM +0100, SZEDER Gábor wrote:
> This is a known issue with the 'am' backend of 'git rebase'.
> 
> The good news is that work is already well under way to change the
> default backend from 'am' to 'merge', which will solve this issue.
> From the log message of aa523de170 (rebase: change the default backend
> from "am" to "merge", 2019-12-24):
> 
>   The am-backend drops information and thus limits what we can do:
>   [...]
>     * reduction in context from only having a few lines beyond those
>       changed means that when context lines are non-unique we can apply
>       patches incorrectly.[2]
>   [...]
>   [2] https://lore.kernel.org/git/CABPp-BGiu2nVMQY_t-rnFR5GQUz_ipyEE8oDocKeO+>
> 
> Alas, there is unexpected bad news: with that commit the runtime of
> your 'git rebase --onto' command goes from <1sec to over 50secs.
> Cc-ing Elijah, author of that patch...

[$.02] I would personally take the route of regaining users' trust in
'git rebase' first, with fixing the performance penalty later on.

I was quite impressed by the recent 2.24.0 performance improvements,
which tells there might be room for improvement for `git rebase` too,
once it is fixed.
SZEDER Gábor Jan. 9, 2020, 3:03 p.m. UTC | #5
On Wed, Jan 08, 2020 at 04:55:46PM -0800, Elijah Newren wrote:
> > Alas, there is unexpected bad news: with that commit the runtime of
> > your 'git rebase --onto' command goes from <1sec to over 50secs.
> > Cc-ing Elijah, author of that patch...
> 
> I see slowdown, but not nearly as big as you report:

The linux repo is big, my notebook is small, the poor thing :)

> $ time git rebase -m --onto v4.18 463fa44eec2fef50~ 463fa44eec2fef50
> warning: inexact rename detection was skipped due to too many files.
> warning: you may want to set your merge.renamelimit variable to at
> least 7216 and retry the command.
> Successfully rebased and updated detached HEAD.
> 
> real 0m13.305s
> user 0m9.644s
> sys 0m3.620s

> Interestingly, turning off rename detection only speeds it up a little bit:
> $ time git rebase -m -Xno-renames --onto v4.18 463fa44eec2fef50~
> 463fa44eec2fef50
> Successfully rebased and updated detached HEAD.
> 
> real 0m11.955s
> user 0m8.732s
> sys 0m3.424s
> 
> 
> This is an interesting testcase; I'm going to try to find some time to
> dig in further.

The culprits are two seemingly unnecessary back-and-forth checkouts.

I didn't realize I could use 'git rebase -m', so ran some tests with
it, and turns out that the slowdown started with 68aa495b59 (rebase:
implement --merge via the interactive machinery, 2018-12-11), where
the runtime suddenly went from <1.5s to 45+s.

Running 'git rebase -i --onto <those-same-commits>' is just as slow,
and it appears that it has always been (the oldest I tried was
v1.8.0), and it spends a long time both before and after popping up
the editor for the rebase instructions.  That's highly suspicious, so:

  $ git log --oneline -1
  94710cac0ef4 (HEAD, tag: v4.18) Linux 4.18
  $ git rebase -i --onto v4.18 463fa44eec2fef50~ 463fa44eec2fef50
  hint: Waiting for your editor to close the file... 
  # Hit ctrl-z in the editor
  $ git log --oneline -1
  463fa44eec2f (HEAD) Input: atmel_mxt_ts - disable IRQ across suspend

Oh.

So 'git rebase -i' apparently checks out the tip commit of the
to-be-rebased revision range before invoking the editor for the rebase
instructions, only to check out the --onto commit (i.e. the commit
we've started from!) to apply the selected commit on top.

And indeed those two checkouts account for all the wasted runtime:

  $ time { git checkout 463fa44eec2fef50 && git checkout v4.18 ; }
  Updating files: 100% (49483/49483), done.
  Previous HEAD position was 94710cac0ef4 Linux 4.18
  HEAD is now at 463fa44eec2f Input: atmel_mxt_ts - disable IRQ across suspend
  Updating files: 100% (49483/49483), done.
  Previous HEAD position was 463fa44eec2f Input: atmel_mxt_ts - disable IRQ across suspend
  HEAD is now at 94710cac0ef4 Linux 4.18
  
  real    0m48.801s
  user    0m13.963s
  sys     0m5.114s
Elijah Newren Jan. 9, 2020, 5:53 p.m. UTC | #6
Hi,

On Thu, Jan 9, 2020 at 7:03 AM SZEDER Gábor <szeder.dev@gmail.com> wrote:
>
> On Wed, Jan 08, 2020 at 04:55:46PM -0800, Elijah Newren wrote:
> > > Alas, there is unexpected bad news: with that commit the runtime of
> > > your 'git rebase --onto' command goes from <1sec to over 50secs.
> > > Cc-ing Elijah, author of that patch...
> >
> > I see slowdown, but not nearly as big as you report:
>
> The linux repo is big, my notebook is small, the poor thing :)

It went to just over 64secs on my home laptop (older and with spinny
disks), so yeah, a big difference from my work machine which has an
SSD.

> > $ time git rebase -m --onto v4.18 463fa44eec2fef50~ 463fa44eec2fef50
> > warning: inexact rename detection was skipped due to too many files.
> > warning: you may want to set your merge.renamelimit variable to at
> > least 7216 and retry the command.
> > Successfully rebased and updated detached HEAD.
> >
> > real 0m13.305s
> > user 0m9.644s
> > sys 0m3.620s
>
> > Interestingly, turning off rename detection only speeds it up a little bit:
> > $ time git rebase -m -Xno-renames --onto v4.18 463fa44eec2fef50~
> > 463fa44eec2fef50
> > Successfully rebased and updated detached HEAD.
> >
> > real 0m11.955s
> > user 0m8.732s
> > sys 0m3.424s
> >
> >
> > This is an interesting testcase; I'm going to try to find some time to
> > dig in further.
>
> The culprits are two seemingly unnecessary back-and-forth checkouts.
>
> I didn't realize I could use 'git rebase -m', so ran some tests with
> it, and turns out that the slowdown started with 68aa495b59 (rebase:
> implement --merge via the interactive machinery, 2018-12-11), where
> the runtime suddenly went from <1.5s to 45+s.
>
> Running 'git rebase -i --onto <those-same-commits>' is just as slow,
> and it appears that it has always been (the oldest I tried was
> v1.8.0), and it spends a long time both before and after popping up
> the editor for the rebase instructions.  That's highly suspicious, so:
>
>   $ git log --oneline -1
>   94710cac0ef4 (HEAD, tag: v4.18) Linux 4.18
>   $ git rebase -i --onto v4.18 463fa44eec2fef50~ 463fa44eec2fef50
>   hint: Waiting for your editor to close the file...
>   # Hit ctrl-z in the editor
>   $ git log --oneline -1
>   463fa44eec2f (HEAD) Input: atmel_mxt_ts - disable IRQ across suspend
>
> Oh.
>
> So 'git rebase -i' apparently checks out the tip commit of the
> to-be-rebased revision range before invoking the editor for the rebase
> instructions, only to check out the --onto commit (i.e. the commit
> we've started from!) to apply the selected commit on top.
>
> And indeed those two checkouts account for all the wasted runtime:
>
>   $ time { git checkout 463fa44eec2fef50 && git checkout v4.18 ; }
>   Updating files: 100% (49483/49483), done.
>   Previous HEAD position was 94710cac0ef4 Linux 4.18
>   HEAD is now at 463fa44eec2f Input: atmel_mxt_ts - disable IRQ across suspend
>   Updating files: 100% (49483/49483), done.
>   Previous HEAD position was 463fa44eec2f Input: atmel_mxt_ts - disable IRQ across suspend
>   HEAD is now at 94710cac0ef4 Linux 4.18
>
>   real    0m48.801s
>   user    0m13.963s
>   sys     0m5.114s

Oh, cool, sounds like you're already investigating and found the problem.
Elijah Newren Jan. 9, 2020, 6:05 p.m. UTC | #7
On Thu, Jan 9, 2020 at 2:53 AM Eugeniu Rosca <erosca@de.adit-jv.com> wrote:
>
> Hi Elijah, hi Szeder,
>
> On Wed, Jan 08, 2020 at 02:06:22PM -0800, Elijah Newren wrote:
> > This looks like a known bug in rebase, in particular in the am-backend that
> > rebase uses by default.  If I'm correct that it's just a context region
> > issue, then this is the same bug that was recently discussed at
> > https://lore.kernel.org/git/CAN_72e2h2avv-U9BVBYqXVKiC+5kHy-pjejyMSD3X22uRXE39g@mail.gmail.com/.
> > The current plan is to switch the default over to the merge backend (the
> > same machinery that cherry-pick uses), which doesn't suffer from the same
> > shortcomings (you can see the current work being done in this area at
> > https://lore.kernel.org/git/pull.679.v3.git.git.1577217299.gitgitgadget@gmail.com/
> > ).
>
> Thank you for your feedback and references, here and in [*].
>
> Once hit by this or similar issues, I think there is high chance for
> people to go through the same feelings as described by Pavel in [**]:
>
>   ---
>   That's so scary that I'm going to stop using "git rebase" for now.
>   ---

Yep, I understand; that kind of feeling is why I wanted to jump in and
try to help fix it.  I want merge/rebase/cherry-pick to be reliable.

> Some years ago I was hit by 'git merge' producing slightly different
> results compared to 'git rebase --onto' and 'git cherry-pick A..B'
> (maybe I can come up with a reproduction scenario for that too).

If you can, I'd be interested to see it and take a look.  I'd normally
assume it was just some case where A..B included "evil" merge commits
(merge commits that made additional changes not part of the actual
merging) since rebasing or cherry-picking such a range would exclude
the merge commits and thus drop those changes -- but you identified a
real bug with the default rebase backend so I'm interested to see if
you happen to have more bugs I should know about.

>
> Since then, I usually contrast the outcomes of merging, cherry-picking
> and rebasing, to make sure they match, but that's painful and
> time-consuming.
>
> > In the mean time, you can pass the -m flag to rebase to avoid these types
> > of problems.  In fact, if you could retry with -m you may be able to
> > confirm whether it's the same issue.
>
> Indeed, neither `git rebase -m` nor `git rebase -i` exhibit the problem.

That's good news.

Unfortunately, you should note that git-2.25 is going to have the same
bug you reported; there are still some loose ends with my series to
make -m the default, and the 2.25 release is expected within a few
days, so my change of default won't happen until 2.26.  (That series
would have needed to be completed several weeks ago for it to go into
2.25).
Eugeniu Rosca Jan. 10, 2020, 12:06 a.m. UTC | #8
Hi Elijah,

On Thu, Jan 09, 2020 at 10:05:52AM -0800, Elijah Newren wrote:
> On Thu, Jan 9, 2020 at 2:53 AM Eugeniu Rosca <erosca@de.adit-jv.com> wrote:
> > Some years ago I was hit by 'git merge' producing slightly different
> > results compared to 'git rebase --onto' and 'git cherry-pick A..B'
> > (maybe I can come up with a reproduction scenario for that too).
> 
> If you can, I'd be interested to see it and take a look.  I'd normally
> assume it was just some case where A..B included "evil" merge commits
> (merge commits that made additional changes not part of the actual
> merging) since rebasing or cherry-picking such a range would exclude
> the merge commits and thus drop those changes -- but you identified a
> real bug with the default rebase backend so I'm interested to see if
> you happen to have more bugs I should know about.

Here is a _simplified_ scenario to get a totally unexpected result from
'git merge' (initially reproduced years ago, but still happening on
2.25.0.rc2):

   ## Preparation
0. git --version
   git version 2.25.0.rc2
1. git clone https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git
2. git remote add linux-stable https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git
3. git fetch linux-stable

   # Reproduction
4. git checkout f7a8e38f07a1
5. git merge --no-edit e18da11fc0f959
   ## Merge v4.4.3 commit
   https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/commit/?id=e18da11fc0f959
   which is a linux-stable backport of vanilla v4.5-rc1 commit
   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=f7a8e38f07a1
   the latter being checked out at step 4.

6. git show HEAD
   ## Inspect the _automatic_ conflict resolution performed by git in
   drivers/mtd/nand/nand_base.c. Git decided to integrate e18da11fc0f959
   alongside f7a8e38f07a1, while essentially they are the same commit.
   We end up with two times commit f7a8e38f07a1.

What do you think about that?

> Unfortunately, you should note that git-2.25 is going to have the same
> bug you reported; there are still some loose ends with my series to
> make -m the default, and the 2.25 release is expected within a few
> days, so my change of default won't happen until 2.26.  (That series
> would have needed to be completed several weeks ago for it to go into
> 2.25).

Thanks for this piece of information and for the time/effort spent!
Elijah Newren Jan. 10, 2020, 2:35 a.m. UTC | #9
Hi Eugeniu,

On Thu, Jan 9, 2020 at 4:06 PM Eugeniu Rosca <roscaeugeniu@gmail.com> wrote:
>
> Hi Elijah,
>
> On Thu, Jan 09, 2020 at 10:05:52AM -0800, Elijah Newren wrote:
> > On Thu, Jan 9, 2020 at 2:53 AM Eugeniu Rosca <erosca@de.adit-jv.com> wrote:
> > > Some years ago I was hit by 'git merge' producing slightly different
> > > results compared to 'git rebase --onto' and 'git cherry-pick A..B'
> > > (maybe I can come up with a reproduction scenario for that too).
> >
> > If you can, I'd be interested to see it and take a look.  I'd normally
> > assume it was just some case where A..B included "evil" merge commits
> > (merge commits that made additional changes not part of the actual
> > merging) since rebasing or cherry-picking such a range would exclude
> > the merge commits and thus drop those changes -- but you identified a
> > real bug with the default rebase backend so I'm interested to see if
> > you happen to have more bugs I should know about.
>
> Here is a _simplified_ scenario to get a totally unexpected result from
> 'git merge' (initially reproduced years ago, but still happening on
> 2.25.0.rc2):
>
>    ## Preparation
> 0. git --version
>    git version 2.25.0.rc2
> 1. git clone https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git
> 2. git remote add linux-stable https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git
> 3. git fetch linux-stable
>
>    # Reproduction
> 4. git checkout f7a8e38f07a1
> 5. git merge --no-edit e18da11fc0f959
>    ## Merge v4.4.3 commit
>    https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/commit/?id=e18da11fc0f959
>    which is a linux-stable backport of vanilla v4.5-rc1 commit
>    https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=f7a8e38f07a1
>    the latter being checked out at step 4.
>
> 6. git show HEAD
>    ## Inspect the _automatic_ conflict resolution performed by git in
>    drivers/mtd/nand/nand_base.c. Git decided to integrate e18da11fc0f959
>    alongside f7a8e38f07a1, while essentially they are the same commit.
>    We end up with two times commit f7a8e38f07a1.
>
> What do you think about that?

Ooh, interesting case; thanks for sending it along.  I think this is
the same as https://lore.kernel.org/git/20190816184051.GB13894@sigill.intra.peff.net/
, which struck git itself not that long back.  It didn't do any actual
harm, though, it was just surprising.  I'm not familiar with the xdiff
part of the codebase, so I don't know if this is a heuristic thing, or
something more along the lines of the diff3 issues mentioned at
https://www.cis.upenn.edu/~bcpierce/papers/diff3-short.pdf.  I read up
on this area a little bit a few months ago and I'd like to dig more at
the diff3 stuff in general, but it may be a little while.  If you see
more issues like this, though, I'm definitely interested in saving and
cataloging them for when I get back to this.

Elijah

Patch
diff mbox series

diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c b/drivers/input/touchscreen/atmel_mxt_ts.c
index b45958e89cc5..2345b587662b 100644
--- a/drivers/input/touchscreen/atmel_mxt_ts.c
+++ b/drivers/input/touchscreen/atmel_mxt_ts.c
@@ -3139,8 +3139,6 @@  static int __maybe_unused mxt_suspend(struct device *dev)
 
 	mutex_unlock(&input_dev->mutex);
 
-	disable_irq(data->irq);
-
 	return 0;
 }
 
@@ -3162,6 +3160,8 @@  static int __maybe_unused mxt_resume(struct device *dev)
 
 	mutex_unlock(&input_dev->mutex);
 
+	disable_irq(data->irq);
+
 	return 0;
 }