Re: [sort-of-BUG] merge-resolve cannot resolve "content/mode" conflict

[Jeff King]

On Mon, Apr 04, 2016 at 10:34:34AM -0700, Junio C Hamano wrote:

> Jeff King  writes:
> 
> > Imagine a merge where one side changes the content of a path and the
> > other changes the mode. Here's a minimal reproduction:
> >
> >   git init repo && cd repo &&
> >
> >   echo base >file &&
> >   git add file &&
> >   git commit -m base &&
> >
> >   echo changed >file &&
> >   git commit -am content &&
> >
> >   git checkout -b side HEAD^
> >   chmod +x file &&
> >   git commit -am mode
> > ...
> > This is a leftover from my experiments with merge-resolve versus
> > merge-recursive last fall, which resulted in a few actual bug-fixes. I
> > looked into fixing this case, too, at that time. It seemed possible, but
> > a little more involved than you might think (because the logic is driven
> > by a bunch of case statements, and this adds a multiplicative layer to
> > the cases; we might need to resolve the permissions, and _then_ see if
> > the content can be resolved).
> 
> Perhaps I am missing some other codepath in the "multiplicative"
> layer, but is this not sufficient?

Sorry for the super-slow reply; just cleaning out my "to respond to"
pile, which has gotten pretty deep.

Looking at it again, I think you are right. I seemed to recall that
there were multiple case arms where we dealt with the permissions, but I
cannot find such a spot now. So I think the solution you outlined looks
good.

> - if test "$6" != "$7"
> + # Three-way merge of the permissions
> + perm= ;# assume the result is the same from stage #2, i.e. $6
> + if test "$6" = "$7" || test "$5" = "$7"
> + then
> + : nothing
> + elif test "$5" = "$6"
>   then
> + case "$7" in
> + 100644) perm=-x ;;
> + 100755) perm=+x ;;
> + *) echo "ERROR: $7: funny filemode not handled." >&2 ;;
> + esac

We reject symlinks and submodules earlier, so I think this "funny
filemode" error really should only be truly-funny entries. Good.

-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [sort-of-BUG] merge-resolve cannot resolve "content/mode" conflict

[Junio C Hamano]

Jeff King  writes:

> Imagine a merge where one side changes the content of a path and the
> other changes the mode. Here's a minimal reproduction:
>
>   git init repo && cd repo &&
>
>   echo base >file &&
>   git add file &&
>   git commit -m base &&
>
>   echo changed >file &&
>   git commit -am content &&
>
>   git checkout -b side HEAD^
>   chmod +x file &&
>   git commit -am mode
> ...
> This is a leftover from my experiments with merge-resolve versus
> merge-recursive last fall, which resulted in a few actual bug-fixes. I
> looked into fixing this case, too, at that time. It seemed possible, but
> a little more involved than you might think (because the logic is driven
> by a bunch of case statements, and this adds a multiplicative layer to
> the cases; we might need to resolve the permissions, and _then_ see if
> the content can be resolved).

Perhaps I am missing some other codepath in the "multiplicative"
layer, but is this not sufficient?

The convoluted "update-index/chmod" dance is to help those on
filesystems that lack proper executable bits.  Otherwise the last
"update-index --chmod" is not needed.


 git-merge-one-file.sh | 25 +++--
 1 file changed, 23 insertions(+), 2 deletions(-)

diff --git a/git-merge-one-file.sh b/git-merge-one-file.sh
index 424b034..36bcdcc 100755
--- a/git-merge-one-file.sh
+++ b/git-merge-one-file.sh
@@ -142,8 +142,19 @@ case "${1:-.}${2:-.}${3:-.}" in
git checkout-index -f --stage=2 -- "$4" && cat "$src1" >"$4" || exit 1
rm -f -- "$orig" "$src1" "$src2"
 
-   if test "$6" != "$7"
+   # Three-way merge of the permissions
+   perm= ;# assume the result is the same from stage #2, i.e. $6
+   if test "$6" = "$7" || test "$5" = "$7"
+   then
+   : nothing
+   elif test "$5" = "$6"
then
+   case "$7" in
+   100644) perm=-x ;;
+   100755) perm=+x ;;
+   *) echo "ERROR: $7: funny filemode not handled." >&2 ;;
+   esac
+   else
if test -n "$msg"
then
msg="$msg, "
@@ -157,7 +168,17 @@ case "${1:-.}${2:-.}${3:-.}" in
echo "ERROR: $msg in $4" >&2
exit 1
fi
-   exec git update-index -- "$4"
+
+   if test -n "$perm"
+   then
+   chmod "$perm" -- "$4"
+   fi &&
+   git update-index -- "$4" &&
+   if test -n "$perm"
+   then
+   git update-index --chmod="$perm" -- "$4"
+   fi
+   exit
;;
 
 *)
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[sort-of-BUG] merge-resolve cannot resolve "content/mode" conflict

[Jeff King]

Imagine a merge where one side changes the content of a path and the
other changes the mode. Here's a minimal reproduction:

  git init repo && cd repo &&

  echo base >file &&
  git add file &&
  git commit -m base &&

  echo changed >file &&
  git commit -am content &&

  git checkout -b side HEAD^
  chmod +x file &&
  git commit -am mode

If I merge that with merge-recursive, I get what you'd expect: mode
10755, and content "changed".

However, with merge-resolve, I get a conflict:

  $ git merge -s resolve master
  Trying really trivial in-index merge...
  error: Merge requires file-level merging
  Nope.
  Trying simple merge.
  Simple merge failed, trying Automatic merge.
  Auto-merging file
  ERROR: permissions conflict: 100644->100755,100644 in file
  fatal: merge program failed
  Automatic merge failed; fix conflicts and then commit the result.

I think this is only a half-bug, really. It's definitely a funny
situation, and it's not unreasonable for a merge driver to punt on a
funny situation rather than resolving it. But I would say:

  - it would probably be a nice improvement to resolve this as
merge-recursive does

  - the "ERROR" message is silly and misleading; the permissions resolve
just fine, it is only that the combination with the content-level
change confuses the script (but the output does not mention that).

This is a leftover from my experiments with merge-resolve versus
merge-recursive last fall, which resulted in a few actual bug-fixes. I
looked into fixing this case, too, at that time. It seemed possible, but
a little more involved than you might think (because the logic is driven
by a bunch of case statements, and this adds a multiplicative layer to
the cases; we might need to resolve the permissions, and _then_ see if
the content can be resolved).

So I didn't actually come up with a patch, but I figured I'd write it up
here for posterity. And just didn't get around to it until now.

-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html