merge-recursive call in git-am -3 chokes, autocrlf issue?

Hi everyone,

I helped Scott R. "WebDragon" Godin on IRC[1] with a bug internal to git-rebase. It manifests like this:

  $ git rebase --stat develop
  First, rewinding head to replay your work on top of it...
   .gitmeta                                           |    2 +-
   Products/index.php                                 |    1 +
   index.php                                          |   11 -----
   res/includes/featured/featured-TEMPLATE.php        |    6 +-
   res/includes/featured/featured-aerotube.php        |    2 +-
   res/includes/featured/featured-beltfeederclock.php |    2 +-
   res/includes/featured/featured-fiberglasstanks.php |    2 +-
   res/includes/featured/featured-handypolaris2.php   |    2 +-
   res/includes/featured/featured-hydrotech.php       |    2 +-
   .../featured/featured-uv_sterilization.php         |    2 +-
   res/includes/inc_meta.php                          |    1 +
   res/includes/inc_nav.php                           |   42 ++++++++++++++++---
   res/java/featurebox.js                             |    2 +-
   13 files changed, 48 insertions(+), 29 deletions(-)
  Applying: Begin restyling/content work on new footer
  Using index info to reconstruct a base tree...
  Falling back to patching base and 3-way merge...
  error: Your local changes to 'res/css/stylehome.css' would be overwritten by merge.  Aborting.
  Please, commit your changes or stash them before you can merge.
  Failed to merge in the changes.
  [...]

I don't know much about the merging machinery, but I figured I could help him poke around, so here's what we gathered:

* He uses 1.7.0.1 on Fedora [2]

* The repo is fairly ordinary except for[3]: setgitperms.perl contrib hooks, core.autocrlf = true

* Editing git-am to use git-merge-resolve instead fixes the issue.

* With GIT_MERGE_VERBOSITY=5 it says [I don't think there's anything useful in there, but who knows]:

    $ git rebase develop
    First, rewinding head to replay your work on top of it...
    Applying: Begin restyling/content work on new footer
    Using index info to reconstruct a base tree...
    Falling back to patching base and 3-way merge...
    Merging HEAD with Begin restyling/content work on new footer
    Merging:
    9e2793f remove innerbox sizing js, as no longer necessary: matching bg color obviates need for equal sized boxes
    virtual Begin restyling/content work on new footer
    found 1 common ancestor(s):
    virtual cef31479147bd3ba2922c3506ec1c70ee5b22729
    error: Your local changes to 'res/css/stylehome.css' would be overwritten by merge.  Aborting.
    Please, commit your changes or stash them before you can merge.
    fatal: merging of trees fe2928d5311cfcf5e668b13cd85608589928f848 and b2ad8208510f713acf1be1c9e62856c51175c6d0 failed
    Failed to merge in the changes.

* Immediately before the git-merge-{recursive,resolve} call, the following outputs may be relevant:

-- 8< -- git diff-files --patch-with-raw
:100644 100644 c1ff94058b86004de4dc1693b6dcd2fccfb28d52 0000000000000000000000000000000000000000 M res/css/stylehome.css
:100644 100644 b653183cc466262996c319ce21fbc29d1164b942 0000000000000000000000000000000000000000 M res/includes/inc_footerhome.php
:100644 100644 29182f14b8e54525649685cdfe718a9a4204ab0e 0000000000000000000000000000000000000000 M res/includes/inc_meta.php
:100644 100644 5910568e733b631d3e3cc73b74ee89a71e4140a2 0000000000000000000000000000000000000000 M res/includes/inc_validate.php

-- >8 --

-- 8< -- git diff-index --patch-with-raw HEAD
:100644 100644 c1ff94058b86004de4dc1693b6dcd2fccfb28d52 0000000000000000000000000000000000000000 M res/css/stylehome.css
:100644 100644 b653183cc466262996c319ce21fbc29d1164b942 0000000000000000000000000000000000000000 M res/includes/inc_footerhome.php
:100644 100644 29182f14b8e54525649685cdfe718a9a4204ab0e 0000000000000000000000000000000000000000 M res/includes/inc_meta.php
:100644 100644 5910568e733b631d3e3cc73b74ee89a71e4140a2 0000000000000000000000000000000000000000 M res/includes/inc_validate.php

-- >8 --

Not sure if it's relevant, but the differences shown here and in the diffstat for the rebased patch above both list 'res/includes/inc_meta.php'.

[I only just noticed that we probably should have used diff-index --cached for the second snippet; I hope that this doesn't make the data worthless...]

We tried the differences between $base_tree and {$his_tree,HEAD} too, but they were too large to be practical.

Since there is some difference between files and index, but neither the modes nor the contents actually show any, my best guess is that it's autocrlf's fault. Then again, who knows.

I did try a theory, but it worked well[4] so that's not it:

* base has a file foo
* side..master recodes foo to crlf and changes bar
* master..side changes bar differently to trigger the 3-way logic Then rebase side on master.

In code:

git init foo
cd foo
seq 1 20 > foo
git add foo
git commit -m initial
seq 100 120 > bar
git add bar
git commit -m bar
seq 1 20 | sed 's/$/\r/' > foo
git add foo
git commit -m crlf
sed -i 's/101/a/;s/102/b/;s/103/c/' bar
git add bar
git commit -m 'change bar'
git checkout -b side HEAD~2
sed -i 's/105/d/;s/106/e/;s/107/f/' bar
git add bar
git commit -m 'change bar differently'
git config core.autocrlf true
git rebase master

[1] http://colabti.org/irclogger/irclogger_log/git?date=2010-03-18#l3682
[2] http://colabti.org/irclogger/irclogger_log/git?date=2010-03-18#l3818
[3] http://colabti.org/irclogger/irclogger_log/git?date=2010-03-18#l3898
[4] modulo the slight problem that I can't get rid of the false dirty state of foo after that, but if I understood autocrlf right that's expected?

Thomas Rast
trast@{inf,student}.ethz.ch

It turns out it's not actually a bug in git-am/git-rebase.

while changing merge-recursive to merge-resolve in git-am solved the plain git rebase branch1 branch2 issue, I still ran into trouble when trying to do a rebase -i so I could squash a commit or two together.

Ilari stepped up to the plate [1] and it was quickly determined that the working copy cache is somehow left dirty after checkout.

doener stepped back in, and while pulling the hooks out that used setgitperms.perl, temporarily, it became obvious that one of them was the culprit, and further testing at this point would allow the rebase to continue, albiet without permissions being set. using git update-index --refresh allowed things to go on normally, when added to the post-checkout hook calling setgitperms.perl [2].

So my recommendation at this point is to patch the instructions within setgitperms.perl to add 'git update-index --refresh' to the end of the post-checkout hook.

I've since reset git-am to use recursive again (instead of resolve) and done several rebases (both with and without -i) and all seems well and normal, and this has made my day.

patch follows:

--8<--

Subject: [PATCH] revise setgitperms.perl hook script description to fix rebase issue

add index-refresh command to post-checkout post-merge script hooks to keep working tree from being marked dirty during a rebase action

---
 contrib/hooks/setgitperms.perl |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

index a577ad0..286835d 100644
@@ -17,6 +17,7 @@
 #      #!/bin/sh
 #     SUBDIRECTORY_OK=1 . git-sh-setup
 #     $GIT_DIR/hooks/setgitperms.perl -w
+#     git update-index --refresh
 #
 use strict;
 use Getopt::Long;

--8<--

[1] http://colabti.org/irclogger/irclogger_log/git?date=2010-03-31#l1556
[2] http://colabti.org/irclogger/irclogger_log/git?date=2010-03-31#l1743

(please respond to the list as opposed to my email box directly,
unless you are supplying private information you don't want public
on the list)

Ahh. If you muck with work tree files and the index in pre-commit, post-merge, or post-checkout hook (especially if you make an up-to-date work tree file stat-dirty), I can imagine that you would need to "refresh" so that unchanged paths would appear unchanged in the index not to confuse your caller.

I however think the patch probably "fixes" the issue at the worst point. Wouldn't either of these alternatives be better?

(1) Perhaps the caller of "pre-commit/post-merge/post-checkout" hook should instead refresh the index when the hook returns, _iff_ we expect that majority of these hooks are used to munge the work tree or the index; or

(2) Because you already established that setgitperms script is the culprit that leaves the index unrefreshed, instead of forcing all the callers of the script, it should do the refresh for its callers before it exits.

Good call.

I talked it over with Todd Zullinger and he came up with the following patch, which I tested on my end to my complete satisfaction, rebases and merges go smoothly.

it's still necessary however, to --no-commit on merges so that you can fix the permissions before your umask blots them out and they wind up in the commit and saved in the gitmeta file

As a result, my usual modus operandi currently is:
git checkout master
git merge --no-ff --no-commit develop find . -perm 0600 -or -perm 0700 |grep -v .git/ ...fix perms back to where they should be git add -A
git commit

which is somewhat less than optimal, but otherwise setgitperms.perl is doing what it should.

Revised patch follows:
--8<--
Subject: [PATCH] Revise setgitperms.perl to fix dirty tree problem when rebasing/merging

reference:
http://comments.gmane.org/gmane.comp.version-control.git/142548

Note that it will be necessary to not only copy the changed setgitperms.perl from /usr/share/git-core/contrib/hooks/ to /usr/share/git-core/templates/hooks/ but additionally every git repository you currently use this script with, will also need to be updated with the new version. This process is regrettably not automatic simply because git was updated on your system.

---
 contrib/hooks/setgitperms.perl |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

index a577ad0..e571560 100644
@@ -91,6 +91,10 @@ if ($write_mode) {
        }
     }
     close IN;
+
+    # Make sure the index isn't left dirty
+    # http://comments.gmane.org/gmane.comp.version-control.git/142548
+    system("git update-index --refresh");
 }
 elsif ($read_mode) {
     # Handle merge conflicts in the .gitperms file

1.7.1

--8<--

-- 
(please respond to the list as opposed to my email box directly,
unless you are supplying private information you don't want public
on the list)

On 05/04/2010 05:47 PM, Scott R. Godin wrote:

Hadn't seen any response to this so I'm reposting in the hopes that this will make it to the next release.

(please respond to the list as opposed to my email box directly,
unless you are supplying private information you don't want public
on the list)

Could you fix the "format=flowed" and also sign-off the patch?