| Message ID | 20201223045358.100754-2-felipe.contreras@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | mergetool: remove unconflicted lines | expand |
Felipe Contreras <felipe.contreras@gmail.com> writes: > +auto_merge () { > + git merge-file --diff3 --marker-size=7 -q -p "$LOCAL" "$BASE" "$REMOTE" >"$DIFF3" > + if test -s "$DIFF3" > + then We do not want to ignore the exit status from the command. IOW, I think the above wants to be rather if git merge-file ... >"$DIFF3" && test -s "$DIFF3" then ... to catch a merge-file that writes halfway and then crashes (doing the same check in different ways are probably possible, of course) > + sed -e '/^<<<<<<< /,/^||||||| /d' -e '/^=======\r\?$/,/^>>>>>>> /d' "$DIFF3" >"$BASE" Does everybody's sed take "\?" and interprets it as zero-or-one? POSIX uses BRE and it doesn't like \? as far as I recall, and "-E" to force ERE is a GNUism. > + sed -e '/^||||||| /,/^>>>>>>> /d' -e '/^<<<<<<< /d' "$DIFF3" >"$LOCAL" > + sed -e '/^<<<<<<< /,/^=======\r\?$/d' -e '/^>>>>>>> /d' "$DIFF3" >"$REMOTE" I'd feel safer if these resulting $BASE, $LOCAL and $REMOTE are validated to be conflict-marker free (i.e. '^\([<|=>]\)\1\1\1\1\1\1' does not appear) to make sure there was no funny virtual ancestor that records a conflicted recursive merge result confused our logic. When we see an unfortunate sign that it happened, we can revert the automerge and let the tool handle the original input. > + fi > + rm -- "$DIFF3" > +} > + "$DIFF3" is always created (unless shell redirection into it fails), so "rm --" would be fine in practice, I guess, but "rm -f --" would not hurt. > + DIFF3="$MERGETOOL_TMPDIR/${BASE}_DIFF3_$$$ext" $MERGETOOL_TMPDIR is either "mktemp -d -t "git-mergetool-XXXXXX" or ".". Also, we liberally pass "$DIFF3" to "sed" as an argument and assume that the command would take it as a filename and not an option. For the above reason, "rm --", while it is not wrong per-se, can be just a simple "rm", as there is no funny leading letters in "$DIFF3" that requires disambiguation.
Junio C Hamano wrote: > Felipe Contreras <felipe.contreras@gmail.com> writes: > > > +auto_merge () { > > + git merge-file --diff3 --marker-size=7 -q -p "$LOCAL" "$BASE" "$REMOTE" >"$DIFF3" > > + if test -s "$DIFF3" > > + then > > We do not want to ignore the exit status from the command. IOW, I > think the above wants to be rather > > if git merge-file ... >"$DIFF3" && > test -s "$DIFF3" > then > ... That doesn't work. "git merge-file" always returns non-zero status when it succeeds (it's the number of conflicts generated). > > + sed -e '/^<<<<<<< /,/^||||||| /d' -e '/^=======\r\?$/,/^>>>>>>> /d' "$DIFF3" >"$BASE" > > Does everybody's sed take "\?" and interprets it as zero-or-one? I don't know. > POSIX uses BRE and it doesn't like \? as far as I recall, and "-E" > to force ERE is a GNUism. Another possibility is \s\*. It's less specific though. > > + sed -e '/^||||||| /,/^>>>>>>> /d' -e '/^<<<<<<< /d' "$DIFF3" >"$LOCAL" > > + sed -e '/^<<<<<<< /,/^=======\r\?$/d' -e '/^>>>>>>> /d' "$DIFF3" >"$REMOTE" > > I'd feel safer if these resulting $BASE, $LOCAL and $REMOTE are > validated to be conflict-marker free (i.e. '^\([<|=>]\)\1\1\1\1\1\1' > does not appear) to make sure there was no funny virtual ancestor > that records a conflicted recursive merge result confused our logic. > > When we see an unfortunate sign that it happened, we can revert the > automerge and let the tool handle the original input. What if the original file does have these markers? Which is probably something we should be checking beforehand and not attempt an automerge in those cases. Or we could add the --base option to "git merge-file" so we don't have to do that work by hand. > > + fi > > + rm -- "$DIFF3" > > +} > > + > > "$DIFF3" is always created (unless shell redirection into it fails), > so "rm --" would be fine in practice, I guess, but "rm -f --" would > not hurt. I just did the same as below: rm -- "$BACKUP" > > + DIFF3="$MERGETOOL_TMPDIR/${BASE}_DIFF3_$$$ext" > > $MERGETOOL_TMPDIR is either "mktemp -d -t "git-mergetool-XXXXXX" or > ".". Also, we liberally pass "$DIFF3" to "sed" as an argument and > assume that the command would take it as a filename and not an > option. > > For the above reason, "rm --", while it is not wrong per-se, can be > just a simple "rm", as there is no funny leading letters in "$DIFF3" > that requires disambiguation. Other parts of the file do this: rm -f -- "$LOCAL" "$REMOTE" "$BASE" "$BACKUP" I'm just following what the script already does. Cheers.
Felipe Contreras <felipe.contreras@gmail.com> writes: > Junio C Hamano wrote: >> Felipe Contreras <felipe.contreras@gmail.com> writes: >> >> > +auto_merge () { >> > + git merge-file --diff3 --marker-size=7 -q -p "$LOCAL" "$BASE" "$REMOTE" >"$DIFF3" >> > + if test -s "$DIFF3" >> > + then >> >> We do not want to ignore the exit status from the command. IOW, I >> think the above wants to be rather >> >> if git merge-file ... >"$DIFF3" && >> test -s "$DIFF3" >> then >> ... > > That doesn't work. > > "git merge-file" always returns non-zero status when it succeeds (it's > the number of conflicts generated). Ah, I forgot about that one. I think "the number of conflicts" was a UI mistake (the original that it mimics is "merge" from RCS suite, which uses 1 and 2 for "conflicts" and "trouble") but we know we will get conflicts, so it is wrong to expect success from the command. Deliberately ignoring the return status is the right thing to do. > What if the original file does have these markers? > > Which is probably something we should be checking beforehand and not > attempt an automerge in those cases. Yes, that is a much better approach to avoid unnecessary work. When we made the conflict marker length configurable, we were hoping that we no longer have to worry about the cases where payload files (original or ours or theirs) have lines that are confusingly similar to the conflict markers, but because we are interfacing external tools that are unaware of the facility, it probably would not help us in this case all that much. FWIW, we use a fiarly large size for our own files in t/ and Documentation/ directories ourselves, and it does help topic branch merges somewhat frequently.
Junio C Hamano wrote: > Felipe Contreras <felipe.contreras@gmail.com> writes: > > > Junio C Hamano wrote: > >> Felipe Contreras <felipe.contreras@gmail.com> writes: > >> > >> > +auto_merge () { > >> > + git merge-file --diff3 --marker-size=7 -q -p "$LOCAL" "$BASE" "$REMOTE" >"$DIFF3" > >> > + if test -s "$DIFF3" > >> > + then > >> > >> We do not want to ignore the exit status from the command. IOW, I > >> think the above wants to be rather > >> > >> if git merge-file ... >"$DIFF3" && > >> test -s "$DIFF3" > >> then > >> ... > > > > That doesn't work. > > > > "git merge-file" always returns non-zero status when it succeeds (it's > > the number of conflicts generated). > > Ah, I forgot about that one. I think "the number of conflicts" was > a UI mistake (the original that it mimics is "merge" from RCS suite, > which uses 1 and 2 for "conflicts" and "trouble") but we know we > will get conflicts, so it is wrong to expect success from the > command. Deliberately ignoring the return status is the right thing > to do. I agree. My bet is that nobody is checking the return status of "git merge-file" to find out the number of conflicts. Plus, how can you check the difference between 255 conflicts and error -1? But that's the situation we are in now. > > What if the original file does have these markers? > > > > Which is probably something we should be checking beforehand and not > > attempt an automerge in those cases. > > Yes, that is a much better approach to avoid unnecessary work. > > When we made the conflict marker length configurable, we were hoping > that we no longer have to worry about the cases where payload files > (original or ours or theirs) have lines that are confusingly similar > to the conflict markers, but because we are interfacing external tools > that are unaware of the facility, it probably would not help us in > this case all that much. > > FWIW, we use a fiarly large size for our own files in t/ and > Documentation/ directories ourselves, and it does help topic branch > merges somewhat frequently. We could do something like --marker-size=13 to minimize the chances of that happening. In that case I would prefer '/^<\{13\} /' (to avoid too many characters). I see those regexes used elsewhere in git, but I don't know how portable that is. If we wanted to make sure none of those markers remain it's not enough to check for '^[<|=>]{13}', what follows up should be a space, or some delimiter, not another < for example. So maybe '^[<|=>]{13}[^<|=>]'? So, do we want those three things? 1. A non-standard marker-size 2. Check beforehand the existence of those markers and disable automerge 3. Check afterwards the existence of those markers and disable automerge Cheers.
Felipe Contreras <felipe.contreras@gmail.com> writes: >> Ah, I forgot about that one. I think "the number of conflicts" was >> a UI mistake (the original that it mimics is "merge" from RCS suite, >> which uses 1 and 2 for "conflicts" and "trouble") but we know we >> will get conflicts, so it is wrong to expect success from the >> command. Deliberately ignoring the return status is the right thing >> to do. > > I agree. My bet is that nobody is checking the return status of "git > merge-file" to find out the number of conflicts. Plus, how can you check > the difference between 255 conflicts and error -1? Yup, I already mentioned UI mistake so you do not have to repeat it to consume more bandwidth. We're in agreement already. > We could do something like --marker-size=13 to minimize the chances of > that happening. > > In that case I would prefer '/^<\{13\} /' (to avoid too many > characters). I see those regexes used elsewhere in git, but I don't know > how portable that is. If it is used elsewhere with "sed", then that would be OK, but if it is not with "sed" but with "grep", that's quite a different story. > So, do we want those three things? > > 1. A non-standard marker-size > 2. Check beforehand the existence of those markers and disable > automerge > 3. Check afterwards the existence of those markers and disable > automerge I do not think 3 is needed if we do 2 and I do not think 1 would particularly be useful *UNLESS* the code consults with the attribute system to see what marker size the path uses to avoid crashing with the non-standard marker-size the path already uses. So the easiest would be not to do anything for now, with a note about known limitations in the doc. The second easiest would be to do 2. alone. We could do 1. to be more complete but I tend to think that it is better to leave it as #leftoverbits.
Junio C Hamano wrote: > Felipe Contreras <felipe.contreras@gmail.com> writes: > > >> Ah, I forgot about that one. I think "the number of conflicts" was > >> a UI mistake (the original that it mimics is "merge" from RCS suite, > >> which uses 1 and 2 for "conflicts" and "trouble") but we know we > >> will get conflicts, so it is wrong to expect success from the > >> command. Deliberately ignoring the return status is the right thing > >> to do. > > > > I agree. My bet is that nobody is checking the return status of "git > > merge-file" to find out the number of conflicts. Plus, how can you check > > the difference between 255 conflicts and error -1? > > Yup, I already mentioned UI mistake so you do not have to repeat You said it was a UI mistake, not me. I am a different mind than yours. This [1] is the first time *you* communicated it was a UI mistake. This [2] is the first time *I* communicated it was a UI mistake. I communicated that fact after you, so I did not repeat anything, because I hadn't said that before. *You* did, not *me*. > it to consume more bandwidth. This is what is consuming bandwidth. Not me stating *for the first time* that I agree what you just stated. You could have skipped what I said *for the first time*, if you didn't find it particularly interesting, and that would have saved bandwidth. > > We could do something like --marker-size=13 to minimize the chances of > > that happening. > > > > In that case I would prefer '/^<\{13\} /' (to avoid too many > > characters). I see those regexes used elsewhere in git, but I don't know > > how portable that is. > > If it is used elsewhere with "sed", then that would be OK, but if it > is not with "sed" but with "grep", that's quite a different story. In t/t3427-rebase-subtree.sh there is: sed -e "s%\([0-9a-f]\{40\} \)files_subtree/%\1%" Not sure if that counts. There's other places in the tests. However, I don't see the point if the marker-size is a low enough number, like 7. > > So, do we want those three things? > > > > 1. A non-standard marker-size > > 2. Check beforehand the existence of those markers and disable > > automerge > > 3. Check afterwards the existence of those markers and disable > > automerge > > I do not think 3 is needed if we do 2 and I do not think 1 would > particularly be useful *UNLESS* the code consults with the attribute > system to see what marker size the path uses to avoid crashing with > the non-standard marker-size the path already uses. But what is more likely? a) That the marker-size is 7 (the default), or b) that the marker-size is not the default, but that there's a marker-size attribute *and* the value is precisely 13? I think a) is way more likely than b). > So the easiest would be not to do anything for now, with a note > about known limitations in the doc. The second easiest would be to > do 2. alone. We could do 1. to be more complete but I tend to think > that it is better to leave it as #leftoverbits. OK. I think 1. is low-hanging fruit, but I'm fine with not doing anything, or trying 2. I don't think 2. would be that hard, so I will try that before re-rolling the series. (unless somebody replies to my other pending arguments) Cheers. [1] https://lore.kernel.org/git/xmqqblek8e94.fsf@gitster.c.googlers.com/ [2] https://lore.kernel.org/git/5fe3dd62e12f8_7855a2081f@natae.notmuch/
Felipe Contreras <felipe.contreras@gmail.com> writes: >> Yup, I already mentioned UI mistake so you do not have to repeat > > You said it was a UI mistake, not me. I am a different mind than yours. Yes, but the point is that I do not need to nor particularly want to hear your opinion on the behaviour of "git merge-file". I know (and others reading the thread on the list also know) that the exit code of the command is misdesigned already. > I communicated that fact after you, so I did not repeat anything, > because I hadn't said that before. *You* did, not *me*. Again, please realize that on list discussion is a team effort to come up together a better design of a shared solution. And if you already know that (I don't read your mind ;-), please act like you do, too. Thanks.
Junio C Hamano wrote: > Felipe Contreras <felipe.contreras@gmail.com> writes: > > >> Yup, I already mentioned UI mistake so you do not have to repeat > > > > You said it was a UI mistake, not me. I am a different mind than yours. > > Yes, but the point is that I do not need to nor particularly want to > hear your opinion on the behaviour of "git merge-file". Then disregard the comment. > I know (and others reading the thread on the list also know) that the > exit code of the command is misdesigned already. Unless you can read minds, you don't know that. And even if you do, I don't know what you know. I can't read your mind. Plus, they can disregard the comment as well. > > I communicated that fact after you, so I did not repeat anything, > > because I hadn't said that before. *You* did, not *me*. > > Again, please realize that on list discussion is a team effort to > come up together a better design of a shared solution. Which is why agreement in a team with different minds and different viewpoints is important. Just to show a few instances of Jeff King telling you he agrees with you: 1. "I agree it's not all that useful in that example" [1]. 16 Dec 2020 2. "I agree with the current definition" [2]. 18 Dec 2020 (same thread) 3. "I agree the two should behave the same" [3]. 18 Dec 2020 (same mail) The fact that you value Jeff King's agreement and don't care what some other members in the community think, is a personal vale judgement, and doesn't necessarily mean the viewpoints of such community members are objectively worthless. Cheers. [1] https://lore.kernel.org/git/X9pUc2HXUr3+WHbR@coredump.intra.peff.net/ [2] https://lore.kernel.org/git/X9xJ6BHM9VY0%2FyLs@coredump.intra.peff.net/ [3] https://lore.kernel.org/git/X9xJ6BHM9VY0%2FyLs@coredump.intra.peff.net/
Felipe Contreras <felipe.contreras@gmail.com> writes: > Junio C Hamano wrote: >> Felipe Contreras <felipe.contreras@gmail.com> writes: >> >> >> Yup, I already mentioned UI mistake so you do not have to repeat >> > >> > You said it was a UI mistake, not me. I am a different mind than yours. >> >> Yes, but the point is that I do not need to nor particularly want to >> hear your opinion on the behaviour of "git merge-file". > >> I know (and others reading the thread on the list also know) that the >> exit code of the command is misdesigned already. > > Unless you can read minds, you don't know that. Actually I do, because they heard from me already ;-). If this were the case where our messages crossed, perhaps, but in this case yours was a response to my message. >> Again, please realize that on list discussion is a team effort to >> come up together a better design of a shared solution. > > Which is why agreement in a team with different minds and different > viewpoints is important. It is not like opinions on all points are important. Whether the exit code from merge-file is or is not a UI mistake does NOT have any influence on what we were discussing. My opinion is that exit code from merge-file is a UI mistake, but even if you disagree with that, that would not change the conclusion we already reached that the code should ignore its exit status, like you originally wrote. I am already trying to ignore your opinions on things that do not matter in the context of this project, as you told me earlier ;-) But just like patches, messages are written only once but read by many people, so I'd always aim for reducing noise at the source. Anyway, happy holidays and pleasant new year to you and to everybody.
Junio C Hamano wrote: > Felipe Contreras <felipe.contreras@gmail.com> writes: > > > Junio C Hamano wrote: > >> Felipe Contreras <felipe.contreras@gmail.com> writes: > >> > >> >> Yup, I already mentioned UI mistake so you do not have to repeat > >> > > >> > You said it was a UI mistake, not me. I am a different mind than yours. > >> > >> Yes, but the point is that I do not need to nor particularly want to > >> hear your opinion on the behaviour of "git merge-file". > > > >> I know (and others reading the thread on the list also know) that the > >> exit code of the command is misdesigned already. > > > > Unless you can read minds, you don't know that. > > Actually I do, because they heard from me already ;-). They heard that you *think* it's a UI mistake. The fact that you think something is a mistake doesn't necessarily mean it's actually a mistake, and other community members might think otherwise. You do not dictate what others on the list know. > >> Again, please realize that on list discussion is a team effort to > >> come up together a better design of a shared solution. > > > > Which is why agreement in a team with different minds and different > > viewpoints is important. > > It is not like opinions on all points are important. Whether the > exit code from merge-file is or is not a UI mistake does NOT have > any influence on what we were discussing. Which is why I initially did not express such an opinion. But you did, presumably you had some reason to do so, so I simply did the same and expressed mine. > I am already trying to ignore your opinions on things that do not > matter in the context of this project, as you told me earlier ;-) > But just like patches, messages are written only once but read by > many people, so I'd always aim for reducing noise at the source. What you consider noise others might not. Good writers say you should not assume what your readers know. Yes, some readers might think exactly like you do, and they don't need what you consider obvious information. But for every person that thinks exactly like you, there are dozens that don't, and it's those you should keep in mind. Most people err on the side of not providing enough information to the minds dissimilar to theirs. This is called the curse of knowledge [1]. I try not to do that. > Anyway, happy holidays and pleasant new year to you and to > everybody. Same to you. Cheers. [1] https://en.wikipedia.org/wiki/Curse_of_knowledge
diff --git a/Documentation/config/mergetool.txt b/Documentation/config/mergetool.txt index 16a27443a3..7ce6d0d3ac 100644 --- a/Documentation/config/mergetool.txt +++ b/Documentation/config/mergetool.txt @@ -61,3 +61,6 @@ mergetool.writeToTemp:: mergetool.prompt:: Prompt before each invocation of the merge resolution program. + +mergetool.autoMerge:: + Remove lines without conflicts from all the files. Defaults to `true`. diff --git a/git-mergetool.sh b/git-mergetool.sh index e3f6d543fb..f4db0cac8d 100755 --- a/git-mergetool.sh +++ b/git-mergetool.sh @@ -239,6 +239,17 @@ checkout_staged_file () { fi } +auto_merge () { + git merge-file --diff3 --marker-size=7 -q -p "$LOCAL" "$BASE" "$REMOTE" >"$DIFF3" + if test -s "$DIFF3" + then + sed -e '/^<<<<<<< /,/^||||||| /d' -e '/^=======\r\?$/,/^>>>>>>> /d' "$DIFF3" >"$BASE" + sed -e '/^||||||| /,/^>>>>>>> /d' -e '/^<<<<<<< /d' "$DIFF3" >"$LOCAL" + sed -e '/^<<<<<<< /,/^=======\r\?$/d' -e '/^>>>>>>> /d' "$DIFF3" >"$REMOTE" + fi + rm -- "$DIFF3" +} + merge_file () { MERGED="$1" @@ -274,6 +285,7 @@ merge_file () { BASE=${BASE##*/} fi + DIFF3="$MERGETOOL_TMPDIR/${BASE}_DIFF3_$$$ext" BACKUP="$MERGETOOL_TMPDIR/${BASE}_BACKUP_$$$ext" LOCAL="$MERGETOOL_TMPDIR/${BASE}_LOCAL_$$$ext" REMOTE="$MERGETOOL_TMPDIR/${BASE}_REMOTE_$$$ext" @@ -322,6 +334,11 @@ merge_file () { checkout_staged_file 2 "$MERGED" "$LOCAL" checkout_staged_file 3 "$MERGED" "$REMOTE" + if test "$(git config --bool mergetool.autoMerge)" != "false" + then + auto_merge + fi + if test -z "$local_mode" || test -z "$remote_mode" then echo "Deleted merge conflict for '$MERGED':" diff --git a/t/t7610-mergetool.sh b/t/t7610-mergetool.sh index 70afdd06fa..ccabd04823 100755 --- a/t/t7610-mergetool.sh +++ b/t/t7610-mergetool.sh @@ -828,4 +828,22 @@ test_expect_success 'mergetool -Oorder-file is honored' ' test_cmp expect actual ' +test_expect_success 'mergetool automerge' ' + test_config mergetool.automerge true && + test_when_finished "git reset --hard" && + git checkout -b test${test_count}_b master && + test_write_lines >file1 base "" a && + git commit -a -m "base" && + test_write_lines >file1 base "" c && + git commit -a -m "remote update" && + git checkout -b test${test_count}_a HEAD~ && + test_write_lines >file1 local "" b && + git commit -a -m "local update" && + test_must_fail git merge test${test_count}_b && + yes "" | git mergetool file1 && + test_write_lines >expect local "" c && + test_cmp expect file1 && + git commit -m "test resolved with mergetool" +' + test_done
The purpose of mergetools is to resolve conflicts when git cannot automatically do so. In order to do that git has added markers in the specific areas that need resolving, which the user must manually fix. The tool is supposed to help with that. However, by passing the original BASE, LOCAL, and REMOTE files, many changes without conflict are presented to the user when in fact nothing needs to be done for those. We can fix that by propagating the final version of the file with the automatic merge to all the panes of the mergetool (BASE, LOCAL, and REMOTE), and only make them differ on the places where there are actual conflicts. As most people will want the new behavior, we enable it by default. Users that do not want the new behavior can set the new configuration mergetool.autoMerge to false. See Seth House's blog post [1] for the idea, and the rationale. [1] https://www.eseth.org/2020/mergetools.html Original-idea-by: Seth House <seth@eseth.com> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com> --- Documentation/config/mergetool.txt | 3 +++ git-mergetool.sh | 17 +++++++++++++++++ t/t7610-mergetool.sh | 18 ++++++++++++++++++ 3 files changed, 38 insertions(+)