Re: [sort-of-BUG] merge-resolve cannot resolve "content/mode" conflict
On Mon, Apr 04, 2016 at 10:34:34AM -0700, Junio C Hamano wrote: > Jeff Kingwrites: > > > 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
Jeff Kingwrites: > 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
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