WARNING: Git merges are tricky. Rebasing is better?

WARNING: Git merges are tricky. Rebasing is better?

[Leo Famulari]

Take note that Git merges can be tricky and hide mistakes! Please
consider using a rebasing workflow instead of a merging workflow for
your branches.

For example, today we merged the version-1.4.0 branch into the master
branch, with commit 276f40fdc349d2ad62582b23ea55e061b689cfc0.

After the merge, we got a report on the #guix IRC channel [0] that the
Epiphany package's source hash was incorrect. It was using the hash of
the previously packaged version of Epiphany.

Using Git, we can see that in commit 291c4fa0ba, Epiphany was updated to
version 41.2, with a correct change in hash:

https://git.savannah.gnu.org/cgit/guix.git/commit/?id=291c4fa0baf24b9d600872fce99441f5471cdb40

Later, the version-1.4.0 branch was merged into master.

View the Git log from the commit where Epiphany was updated through the
merge:

$ git log --patch 291c4fa0baf^..276f40fdc34 gnu/packages/gnome.scm

It does not show that the update of Epiphany's source hash was reverted.

And to be sure that something is wrong, we can examine the file based on
the merge commit:

------
$ git show 276f40fdc349d2ad62582b23ea55e061b689cfc0:gnu/packages/gnome.scm | grep "define-public epiphany" -A11
(define-public epiphany
  (package
    (name "epiphany")
    (version "41.2")
    (source (origin
              (method url-fetch)
              (uri (string-append "mirror://gnome/sources/epiphany/"
                                  (version-major version) "/"
                                  "epiphany-" version ".tar.xz"))
              (sha256
               (base32
                "0r7m34xzz3shdfxf2abxb069izak3yv3ijlg29qy4pfmyawkilfs"))
------           ^
                *This is the hash of Epiphany 40.3, the old version!*

Git's automatic merge conflict resolution algorithm did the wrong thing
here. And Git does not show the reversion in `git log`, hiding it from
review.

My guess is that commit f7afefba0 ("gnu: epiphany: Fix build with
libportal-0.5."), which happened on the master branch while the
version-1.4.0 branch was forked off, made Git think that this line was
more "up to date" on master than on version-1.4.0, causing it to select
the old hash when faced with the conflict.

Once again, consider using a workflow based on rebasing instead of
merging for forked branches that will live for weeks to a month or two.
This type of mistake cannot be obscured when rebasing.

[0] http://logs.guix.gnu.org/guix/2022-01-17.log#195313

Re: WARNING: Git merges are tricky. Rebasing is better?

[Kyle Meyer]

Leo Famulari writes:

> ------
> $ git show 276f40fdc349d2ad62582b23ea55e061b689cfc0:gnu/packages/gnome.scm | grep "define-public epiphany" -A11
> (define-public epiphany
>   (package
>     (name "epiphany")
>     (version "41.2")
>     (source (origin
>               (method url-fetch)
>               (uri (string-append "mirror://gnome/sources/epiphany/"
>                                   (version-major version) "/"
>                                   "epiphany-" version ".tar.xz"))
>               (sha256
>                (base32
>                 "0r7m34xzz3shdfxf2abxb069izak3yv3ijlg29qy4pfmyawkilfs"))
> ------           ^
>                 *This is the hash of Epiphany 40.3, the old version!*
>
> Git's automatic merge conflict resolution algorithm did the wrong thing
> here. And Git does not show the reversion in `git log`, hiding it from
> review.
>
> My guess is that commit f7afefba0 ("gnu: epiphany: Fix build with
> libportal-0.5."), which happened on the master branch while the
> version-1.4.0 branch was forked off, made Git think that this line was
> more "up to date" on master than on version-1.4.0, causing it to select
> the old hash when faced with the conflict.

Fwiw I don't think Git automatically resolved that conflict:

  $ git checkout 276f40fdc349d2ad62582b23ea55e061b689cfc0^
  $ git merge 276f40fdc349d2ad62582b23ea55e061b689cfc0^2
  
  $ git status -- gnu/packages/gnome.scm
  HEAD detached at b2f6b6f6b9
  You have unmerged paths.
  
  Unmerged paths:
  	both modified:   gnu/packages/gnome.scm
  
  no changes added to commit
  
  $ git diff -- gnu/packages/gnome.scm
  diff --cc gnu/packages/gnome.scm
  index d8d34c89ed,6c63b8bc59..0000000000
  --- a/gnu/packages/gnome.scm
  +++ b/gnu/packages/gnome.scm
  @@@ -6433,13 -6415,10 +6421,12 @@@ supports playlists, song ratings, and a
                                    name "-" version ".tar.xz"))
                (sha256
                 (base32
   -              "0ddjwcd77nw0rxb5x5bz5hd671m8gya9827p8rsnb58x103kpai8"))))
   +              "0ddjwcd77nw0rxb5x5bz5hd671m8gya9827p8rsnb58x103kpai8"))
   +            ;; XXX: Remove when upgrading to 42.0
   +            (patches (search-patches "eog-update-libportal-usage.patch"))))
       (build-system meson-build-system)
       (arguments
  -     `(#:meson ,meson-0.59         ;positional arguments error with meson 0.60
  -       #:configure-flags
  +     `(#:configure-flags
          ;; Otherwise, the RUNPATH will lack the final 'eog' path component.
          (list (string-append "-Dc_link_args=-Wl,-rpath="
                               (assoc-ref %outputs "out") "/lib/eog"))
  @@@ -6798,9 -6776,8 +6784,17 @@@ a secret password store, an adblocker, 
                                      "epiphany-" version ".tar.xz"))
                  (sha256
                   (base32
  ++<<<<<<< HEAD
   +                "0r7m34xzz3shdfxf2abxb069izak3yv3ijlg29qy4pfmyawkilfs"))
   +              (patches
   +               (search-patches "epiphany-update-libportal-usage.patch"))))
  ++||||||| d91de53caa
  ++                "0r7m34xzz3shdfxf2abxb069izak3yv3ijlg29qy4pfmyawkilfs"))))
  ++
  ++=======
  +                 "0k7b22zq3z1kllzqxgwsvwb1lp0j6rjb3k1hvhna3i573wc4mpji"))))
  + 
  ++>>>>>>> 276f40fdc349d2ad62582b23ea55e061b689cfc0^2
        (build-system meson-build-system)
        (arguments
         `(#:glib-or-gtk? #t

Re: WARNING: Git merges are tricky. Rebasing is better?

[Leo Famulari]

On Mon, Jan 17, 2022 at 05:26:08PM -0500, Kyle Meyer wrote:
> Fwiw I don't think Git automatically resolved that conflict:
> 
>   $ git checkout 276f40fdc349d2ad62582b23ea55e061b689cfc0^
>   $ git merge 276f40fdc349d2ad62582b23ea55e061b689cfc0^2
[...]   
>   ++<<<<<<< HEAD
>    +                "0r7m34xzz3shdfxf2abxb069izak3yv3ijlg29qy4pfmyawkilfs"))
>    +              (patches
>    +               (search-patches "epiphany-update-libportal-usage.patch"))))
>   ++||||||| d91de53caa
>   ++                "0r7m34xzz3shdfxf2abxb069izak3yv3ijlg29qy4pfmyawkilfs"))))
>   ++
>   ++=======
>   +                 "0k7b22zq3z1kllzqxgwsvwb1lp0j6rjb3k1hvhna3i573wc4mpji"))))
>   + 
>   ++>>>>>>> 276f40fdc349d2ad62582b23ea55e061b689cfc0^2

So, you think it was a mistake made when resolving conflicts by hand? I
can certainly understand that.

Either way, it's bad that the Git history doesn't show these types of
changes.

Re: WARNING: Git merges are tricky. Rebasing is better?

[Kyle Meyer]

Leo Famulari writes:

> On Mon, Jan 17, 2022 at 05:26:08PM -0500, Kyle Meyer wrote:
>> Fwiw I don't think Git automatically resolved that conflict:
>> 
>>   $ git checkout 276f40fdc349d2ad62582b23ea55e061b689cfc0^
>>   $ git merge 276f40fdc349d2ad62582b23ea55e061b689cfc0^2
> [...]   
>>   ++<<<<<<< HEAD
>>    +                "0r7m34xzz3shdfxf2abxb069izak3yv3ijlg29qy4pfmyawkilfs"))
>>    +              (patches
>>    +               (search-patches "epiphany-update-libportal-usage.patch"))))
>>   ++||||||| d91de53caa
>>   ++                "0r7m34xzz3shdfxf2abxb069izak3yv3ijlg29qy4pfmyawkilfs"))))
>>   ++
>>   ++=======
>>   +                 "0k7b22zq3z1kllzqxgwsvwb1lp0j6rjb3k1hvhna3i573wc4mpji"))))
>>   + 
>>   ++>>>>>>> 276f40fdc349d2ad62582b23ea55e061b689cfc0^2
>
> So, you think it was a mistake made when resolving conflicts by hand? I
> can certainly understand that.

Yes.

> Either way, it's bad that the Git history doesn't show these types of
> changes.

Right, inspecting merge results can be confusing.  By default that
change is pruned from diffs as "uninteresting" because the merge result
matched the content in one of the parents (discussed in the "COMBINED
DIFF FORMAT" section of git-diff's manpage and the --diff-merges
description of git-show's manpage).

  $ git show 276f40fd | grep 0r7m34xzz
  $ git show --diff-merges=combined 276f40fd | grep 0r7m34xzz
   +                "0r7m34xzz3shdfxf2abxb069izak3yv3ijlg29qy4pfmyawkilfs"))

Re: WARNING: Git merges are tricky. Rebasing is better?

[Maxim Cournoyer]

Hello Kyle and Leo,

Kyle Meyer <kyle@kyleam.com> writes:

> Leo Famulari writes:
>
>> On Mon, Jan 17, 2022 at 05:26:08PM -0500, Kyle Meyer wrote:
>>> Fwiw I don't think Git automatically resolved that conflict:
>>> 
>>>   $ git checkout 276f40fdc349d2ad62582b23ea55e061b689cfc0^
>>>   $ git merge 276f40fdc349d2ad62582b23ea55e061b689cfc0^2
>> [...]   
>>>   ++<<<<<<< HEAD
>>>    +                "0r7m34xzz3shdfxf2abxb069izak3yv3ijlg29qy4pfmyawkilfs"))
>>>    +              (patches
>>>    +               (search-patches "epiphany-update-libportal-usage.patch"))))
>>>   ++||||||| d91de53caa
>>>   ++                "0r7m34xzz3shdfxf2abxb069izak3yv3ijlg29qy4pfmyawkilfs"))))
>>>   ++
>>>   ++=======
>>>   +                 "0k7b22zq3z1kllzqxgwsvwb1lp0j6rjb3k1hvhna3i573wc4mpji"))))
>>>   + 
>>>   ++>>>>>>> 276f40fdc349d2ad62582b23ea55e061b689cfc0^2
>>
>> So, you think it was a mistake made when resolving conflicts by hand? I
>> can certainly understand that.
>
> Yes.
>
>> Either way, it's bad that the Git history doesn't show these types of
>> changes.
>
> Right, inspecting merge results can be confusing.  By default that
> change is pruned from diffs as "uninteresting" because the merge result
> matched the content in one of the parents (discussed in the "COMBINED
> DIFF FORMAT" section of git-diff's manpage and the --diff-merges
> description of git-show's manpage).
>
>   $ git show 276f40fd | grep 0r7m34xzz
>   $ git show --diff-merges=combined 276f40fd | grep 0r7m34xzz
>    +                "0r7m34xzz3shdfxf2abxb069izak3yv3ijlg29qy4pfmyawkilfs"))

Thanks for this interesting discussion, and apologies for mis-resolving
the conflict :-)

Maxim