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

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

[Thomas Rast]

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
 
diff --git a/res/css/stylehome.css b/res/css/stylehome.css
diff --git a/res/includes/inc_footerhome.php b/res/includes/inc_footerhome.php
diff --git a/res/includes/inc_meta.php b/res/includes/inc_meta.php
diff --git a/res/includes/inc_validate.php b/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
 
diff --git a/res/css/stylehome.css b/res/css/stylehome.css
diff --git a/res/includes/inc_footerhome.php b/res/includes/inc_footerhome.php
diff --git a/res/includes/inc_meta.php b/res/includes/inc_meta.php
diff --git a/res/includes/inc_validate.php b/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

Re: [BUG] [RESOLVED] merge-recursive call in git-am -3 chokes, autocrlf issue?

[Scott R. Godin]

On 03/18/2010 08:49 PM, Thomas Rast wrote:
> Hi everyone,
>
> I helped Scott R. "WebDragon" Godin on IRC[1] with a bug internal to
> git-rebase.  It manifests like this:

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(-)

diff --git a/contrib/hooks/setgitperms.perl b/contrib/hooks/setgitperms.perl
index a577ad0..286835d 100644
--- a/contrib/hooks/setgitperms.perl
+++ b/contrib/hooks/setgitperms.perl
@@ -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)

Re: [BUG] [RESOLVED] merge-recursive call in git-am -3 chokes, autocrlf issue?

[Junio C Hamano]

"Scott R. Godin" <scottg.wp-hackers@mhg2.com> writes:

> 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.

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.

Re: [BUG] [RESOLVED] merge-recursive call in git-am -3 chokes, autocrlf issue?

[Scott R. Godin]

On 04/01/2010 01:27 PM, Junio C Hamano wrote:
> 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(-)

diff --git a/contrib/hooks/setgitperms.perl b/contrib/hooks/setgitperms.perl
index a577ad0..e571560 100644
--- a/contrib/hooks/setgitperms.perl
+++ b/contrib/hooks/setgitperms.perl
@@ -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)

[PATCH] setgitperms.perl dirty index problem (was Re: [BUG] [RESOLVED] merge-recursive call in git-am -3 chokes, autocrlf issue?)

[Scott R. Godin]

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.

> On 04/01/2010 01:27 PM, Junio C Hamano wrote:
>> 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(-)
>
> diff --git a/contrib/hooks/setgitperms.perl
> b/contrib/hooks/setgitperms.perl
> index a577ad0..e571560 100644
> --- a/contrib/hooks/setgitperms.perl
> +++ b/contrib/hooks/setgitperms.perl
> @@ -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


-- 
(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)

Re: [PATCH] setgitperms.perl dirty index problem

[Junio C Hamano]

"Scott R. Godin" <scottg.wp-hackers@mhg2.com> writes:

> 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.

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