* [PATCH v5 0/1] mergetool: remove unconflicted lines @ 2020-12-23 4:53 Felipe Contreras 2020-12-23 4:53 ` [PATCH v5 1/1] mergetool: add automerge configuration Felipe Contreras ` (2 more replies) 0 siblings, 3 replies; 80+ messages in thread From: Felipe Contreras @ 2020-12-23 4:53 UTC (permalink / raw) To: git Cc: Junio C Hamano, David Aguilar, Johannes Sixt, Seth House, Felipe Contreras There's not much to say other that what the commit message of the patch says. Note: no feedback has been ignored; I replied to all the feedback, I didn't hear anything back. Changes since v4: * Improved commit message with suggestions from Phillip Wood. Felipe Contreras (1): mergetool: add automerge configuration Documentation/config/mergetool.txt | 3 +++ git-mergetool.sh | 17 +++++++++++++++++ t/t7610-mergetool.sh | 18 ++++++++++++++++++ 3 files changed, 38 insertions(+) Range-diff: 1: 776c1fbb97 ! 1: 2dc53f4dda mergetool: add automerge configuration @@ Metadata ## Commit message ## mergetool: add automerge configuration - It doesn't make sense to display lines without conflicts in the - different views of all mergetools. + The purpose of mergetools is to resolve conflicts when git cannot + automatically do so. - Only the lines that warrant conflict markers should be displayed. + 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. - Most people would want this behavior on, but in case some don't; add a - new configuration: mergetool.autoMerge. + 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. -- 2.30.0.rc1 ^ permalink raw reply [flat|nested] 80+ messages in thread
* [PATCH v5 1/1] mergetool: add automerge configuration 2020-12-23 4:53 [PATCH v5 0/1] mergetool: remove unconflicted lines Felipe Contreras @ 2020-12-23 4:53 ` Felipe Contreras 2020-12-23 13:34 ` Junio C Hamano 2020-12-24 9:24 ` [PATCH v5 0/1] mergetool: remove unconflicted lines Junio C Hamano 2020-12-27 20:58 ` [PATCH v6 0/1] mergetool: add automerge configuration Seth House 2 siblings, 1 reply; 80+ messages in thread From: Felipe Contreras @ 2020-12-23 4:53 UTC (permalink / raw) To: git Cc: Junio C Hamano, David Aguilar, Johannes Sixt, Seth House, Felipe Contreras 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(+) 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 -- 2.30.0.rc1 ^ permalink raw reply related [flat|nested] 80+ messages in thread
* Re: [PATCH v5 1/1] mergetool: add automerge configuration 2020-12-23 4:53 ` [PATCH v5 1/1] mergetool: add automerge configuration Felipe Contreras @ 2020-12-23 13:34 ` Junio C Hamano 2020-12-23 14:23 ` Felipe Contreras 0 siblings, 1 reply; 80+ messages in thread From: Junio C Hamano @ 2020-12-23 13:34 UTC (permalink / raw) To: Felipe Contreras; +Cc: git, David Aguilar, Johannes Sixt, Seth House 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. ^ permalink raw reply [flat|nested] 80+ messages in thread
* Re: [PATCH v5 1/1] mergetool: add automerge configuration 2020-12-23 13:34 ` Junio C Hamano @ 2020-12-23 14:23 ` Felipe Contreras 2020-12-23 20:21 ` Junio C Hamano 0 siblings, 1 reply; 80+ messages in thread From: Felipe Contreras @ 2020-12-23 14:23 UTC (permalink / raw) To: Junio C Hamano, Felipe Contreras Cc: git, David Aguilar, Johannes Sixt, Seth House 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 ^ permalink raw reply [flat|nested] 80+ messages in thread
* Re: [PATCH v5 1/1] mergetool: add automerge configuration 2020-12-23 14:23 ` Felipe Contreras @ 2020-12-23 20:21 ` Junio C Hamano 2020-12-24 0:14 ` Felipe Contreras 0 siblings, 1 reply; 80+ messages in thread From: Junio C Hamano @ 2020-12-23 20:21 UTC (permalink / raw) To: Felipe Contreras; +Cc: git, David Aguilar, Johannes Sixt, Seth House 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. ^ permalink raw reply [flat|nested] 80+ messages in thread
* Re: [PATCH v5 1/1] mergetool: add automerge configuration 2020-12-23 20:21 ` Junio C Hamano @ 2020-12-24 0:14 ` Felipe Contreras 2020-12-24 0:32 ` Junio C Hamano 0 siblings, 1 reply; 80+ messages in thread From: Felipe Contreras @ 2020-12-24 0:14 UTC (permalink / raw) To: Junio C Hamano, Felipe Contreras Cc: git, David Aguilar, Johannes Sixt, Seth House 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 ^ permalink raw reply [flat|nested] 80+ messages in thread
* Re: [PATCH v5 1/1] mergetool: add automerge configuration 2020-12-24 0:14 ` Felipe Contreras @ 2020-12-24 0:32 ` Junio C Hamano 2020-12-24 1:36 ` Felipe Contreras 0 siblings, 1 reply; 80+ messages in thread From: Junio C Hamano @ 2020-12-24 0:32 UTC (permalink / raw) To: Felipe Contreras; +Cc: git, David Aguilar, Johannes Sixt, Seth House 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. ^ permalink raw reply [flat|nested] 80+ messages in thread
* Re: [PATCH v5 1/1] mergetool: add automerge configuration 2020-12-24 0:32 ` Junio C Hamano @ 2020-12-24 1:36 ` Felipe Contreras 2020-12-24 6:17 ` Junio C Hamano 0 siblings, 1 reply; 80+ messages in thread From: Felipe Contreras @ 2020-12-24 1:36 UTC (permalink / raw) To: Junio C Hamano, Felipe Contreras Cc: git, David Aguilar, Johannes Sixt, Seth House 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 ^ permalink raw reply [flat|nested] 80+ messages in thread
* Re: [PATCH v5 1/1] mergetool: add automerge configuration 2020-12-24 1:36 ` Felipe Contreras @ 2020-12-24 6:17 ` Junio C Hamano 2020-12-24 15:59 ` Felipe Contreras 0 siblings, 1 reply; 80+ messages in thread From: Junio C Hamano @ 2020-12-24 6:17 UTC (permalink / raw) To: Felipe Contreras; +Cc: git, David Aguilar, Johannes Sixt, Seth House 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. ^ permalink raw reply [flat|nested] 80+ messages in thread
* Re: [PATCH v5 1/1] mergetool: add automerge configuration 2020-12-24 6:17 ` Junio C Hamano @ 2020-12-24 15:59 ` Felipe Contreras 2020-12-24 22:32 ` Junio C Hamano 0 siblings, 1 reply; 80+ messages in thread From: Felipe Contreras @ 2020-12-24 15:59 UTC (permalink / raw) To: Junio C Hamano, Felipe Contreras Cc: git, David Aguilar, Johannes Sixt, Seth House 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 ^ permalink raw reply [flat|nested] 80+ messages in thread
* Re: [PATCH v5 1/1] mergetool: add automerge configuration 2020-12-24 15:59 ` Felipe Contreras @ 2020-12-24 22:32 ` Junio C Hamano 2020-12-27 18:01 ` Felipe Contreras 0 siblings, 1 reply; 80+ messages in thread From: Junio C Hamano @ 2020-12-24 22:32 UTC (permalink / raw) To: Felipe Contreras; +Cc: git, David Aguilar, Johannes Sixt, Seth House 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. ^ permalink raw reply [flat|nested] 80+ messages in thread
* Re: [PATCH v5 1/1] mergetool: add automerge configuration 2020-12-24 22:32 ` Junio C Hamano @ 2020-12-27 18:01 ` Felipe Contreras 0 siblings, 0 replies; 80+ messages in thread From: Felipe Contreras @ 2020-12-27 18:01 UTC (permalink / raw) To: Junio C Hamano, Felipe Contreras Cc: git, David Aguilar, Johannes Sixt, Seth House 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 -- Felipe Contreras ^ permalink raw reply [flat|nested] 80+ messages in thread
* Re: [PATCH v5 0/1] mergetool: remove unconflicted lines 2020-12-23 4:53 [PATCH v5 0/1] mergetool: remove unconflicted lines Felipe Contreras 2020-12-23 4:53 ` [PATCH v5 1/1] mergetool: add automerge configuration Felipe Contreras @ 2020-12-24 9:24 ` Junio C Hamano 2020-12-24 16:16 ` Felipe Contreras 2020-12-30 5:47 ` Johannes Schindelin 2020-12-27 20:58 ` [PATCH v6 0/1] mergetool: add automerge configuration Seth House 2 siblings, 2 replies; 80+ messages in thread From: Junio C Hamano @ 2020-12-24 9:24 UTC (permalink / raw) To: Felipe Contreras, Johannes Schindelin Cc: git, David Aguilar, Johannes Sixt, Seth House Felipe Contreras <felipe.contreras@gmail.com> writes: > There's not much to say other that what the commit message of the patch says. > > Note: no feedback has been ignored; I replied to all the feedback, I didn't hear anything back. > > Changes since v4: > > * Improved commit message with suggestions from Phillip Wood. > > Felipe Contreras (1): > mergetool: add automerge configuration This breakage is possibly a fallout from either this patch or 1e2ae142 (t7[5-9]*: adjust the references to the default branch name "main", 2020-11-18). https://github.com/git/git/runs/1602803804#step:7:10358 I cannot quite tell how the two strings compared with 'test' on output line 10355 are different in the output, though. Thanks. ^ permalink raw reply [flat|nested] 80+ messages in thread
* Re: [PATCH v5 0/1] mergetool: remove unconflicted lines 2020-12-24 9:24 ` [PATCH v5 0/1] mergetool: remove unconflicted lines Junio C Hamano @ 2020-12-24 16:16 ` Felipe Contreras 2020-12-30 5:47 ` Johannes Schindelin 1 sibling, 0 replies; 80+ messages in thread From: Felipe Contreras @ 2020-12-24 16:16 UTC (permalink / raw) To: Junio C Hamano, Felipe Contreras, Johannes Schindelin Cc: git, David Aguilar, Johannes Sixt, Seth House Junio C Hamano wrote: > Felipe Contreras <felipe.contreras@gmail.com> writes: > > > There's not much to say other that what the commit message of the patch says. > > > > Note: no feedback has been ignored; I replied to all the feedback, I didn't hear anything back. > > > > Changes since v4: > > > > * Improved commit message with suggestions from Phillip Wood. > > > > Felipe Contreras (1): > > mergetool: add automerge configuration > > This breakage is possibly a fallout from either this patch or > 1e2ae142 (t7[5-9]*: adjust the references to the default branch name > "main", 2020-11-18). > > https://github.com/git/git/runs/1602803804#step:7:10358 It seems likely it's the mergetool patch. This regex '/^=======\r\?$/' is supposed to handle the crlf situation. I can't imagine what would be different in Windows regarding that situation. -- Felipe Contreras ^ permalink raw reply [flat|nested] 80+ messages in thread
* Re: [PATCH v5 0/1] mergetool: remove unconflicted lines 2020-12-24 9:24 ` [PATCH v5 0/1] mergetool: remove unconflicted lines Junio C Hamano 2020-12-24 16:16 ` Felipe Contreras @ 2020-12-30 5:47 ` Johannes Schindelin 2020-12-30 22:33 ` Felipe Contreras 1 sibling, 1 reply; 80+ messages in thread From: Johannes Schindelin @ 2020-12-30 5:47 UTC (permalink / raw) To: Junio C Hamano Cc: Felipe Contreras, git, David Aguilar, Johannes Sixt, Seth House Hi Junio, On Thu, 24 Dec 2020, Junio C Hamano wrote: > Felipe Contreras <felipe.contreras@gmail.com> writes: > > > There's not much to say other that what the commit message of the patch says. > > > > Note: no feedback has been ignored; I replied to all the feedback, I didn't hear anything back. > > > > Changes since v4: > > > > * Improved commit message with suggestions from Phillip Wood. > > > > Felipe Contreras (1): > > mergetool: add automerge configuration > > This breakage is possibly a fallout from either this patch or > 1e2ae142 (t7[5-9]*: adjust the references to the default branch name > "main", 2020-11-18). > > https://github.com/git/git/runs/1602803804#step:7:10358 > > I cannot quite tell how the two strings compared with 'test' on > output line 10355 are different in the output, though. I spent more time than I cared to spend on this, and still have not quite figured out what is the fault, but I can state with conviction that the problem is not even introduced by any merge into `seen`. The `fc/mergetool-automerge` branch itself is already broken: https://github.com/gitgitgadget/git/actions/runs/441233234 Ciao, Dscho ^ permalink raw reply [flat|nested] 80+ messages in thread
* Re: [PATCH v5 0/1] mergetool: remove unconflicted lines 2020-12-30 5:47 ` Johannes Schindelin @ 2020-12-30 22:33 ` Felipe Contreras 0 siblings, 0 replies; 80+ messages in thread From: Felipe Contreras @ 2020-12-30 22:33 UTC (permalink / raw) To: Johannes Schindelin, Junio C Hamano Cc: Felipe Contreras, git, David Aguilar, Johannes Sixt, Seth House Johannes Schindelin wrote: > On Thu, 24 Dec 2020, Junio C Hamano wrote: > > This breakage is possibly a fallout from either this patch or > > 1e2ae142 (t7[5-9]*: adjust the references to the default branch name > > "main", 2020-11-18). > > > > https://github.com/git/git/runs/1602803804#step:7:10358 > > > > I cannot quite tell how the two strings compared with 'test' on > > output line 10355 are different in the output, though. > > I spent more time than I cared to spend on this, and still have not quite > figured out what is the fault, but I can state with conviction that the > problem is not even introduced by any merge into `seen`. The > `fc/mergetool-automerge` branch itself is already broken: > https://github.com/gitgitgadget/git/actions/runs/441233234 Yes, if you didn't have me blocked and read what I said a week ago in [1], you would have saved yourself that time. Seth House has claimed the patch series though. [1] https://lore.kernel.org/git/5fe4bec2da21a_19c92085f@natae.notmuch/ -- Felipe Contreras ^ permalink raw reply [flat|nested] 80+ messages in thread
* [PATCH v6 0/1] mergetool: add automerge configuration 2020-12-23 4:53 [PATCH v5 0/1] mergetool: remove unconflicted lines Felipe Contreras 2020-12-23 4:53 ` [PATCH v5 1/1] mergetool: add automerge configuration Felipe Contreras 2020-12-24 9:24 ` [PATCH v5 0/1] mergetool: remove unconflicted lines Junio C Hamano @ 2020-12-27 20:58 ` Seth House 2020-12-27 20:58 ` [PATCH v6 1/2] " Seth House ` (3 more replies) 2 siblings, 4 replies; 80+ messages in thread From: Seth House @ 2020-12-27 20:58 UTC (permalink / raw) To: git Cc: Seth House, Junio C Hamano, David Aguilar, Johannes Sixt, Felipe Contreras Sorry for the slow turnaround on this. I haven't used Git via email patches before now so it took me quite a few hours to read through tutorials, configure git-send-email and fight missing Perl libs. Please let me know if I did anything incorrectly! I should be able to contribute more quickly from now on. Changes since v5: * Add per-tool configuration that Felipe has a "deep philosophical" opposition to adding. Felipe Contreras (1): mergetool: add automerge configuration Seth House (1): mergetool: Add per-tool support for the autoMerge flag Documentation/config/mergetool.txt | 6 ++++++ git-mergetool.sh | 13 +++++++++++++ t/t7610-mergetool.sh | 18 ++++++++++++++++++ 3 files changed, 37 insertions(+) -- 2.29.2 ^ permalink raw reply [flat|nested] 80+ messages in thread
* [PATCH v6 1/2] mergetool: add automerge configuration 2020-12-27 20:58 ` [PATCH v6 0/1] mergetool: add automerge configuration Seth House @ 2020-12-27 20:58 ` Seth House 2020-12-27 22:06 ` Junio C Hamano 2020-12-27 20:58 ` [PATCH v6 2/2] mergetool: Add per-tool support for the autoMerge flag Seth House ` (2 subsequent siblings) 3 siblings, 1 reply; 80+ messages in thread From: Seth House @ 2020-12-27 20:58 UTC (permalink / raw) To: git; +Cc: Felipe Contreras, Seth House From: Felipe Contreras <felipe.contreras@gmail.com> It doesn't make sense to display easily-solvable conflicts in the different views of all mergetools. Only the chunks that warrant conflict markers should be displayed. In order to unobtrusively do this, add a new configuration: mergetool.autoMerge. 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 | 10 ++++++++++ t/t7610-mergetool.sh | 18 ++++++++++++++++++ 3 files changed, 31 insertions(+) diff --git a/Documentation/config/mergetool.txt b/Documentation/config/mergetool.txt index 16a27443a3..43af7a96f9 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:: + Automatically resolve conflicts that don't require user intervention. diff --git a/git-mergetool.sh b/git-mergetool.sh index e3f6d543fb..6e86d3b492 100755 --- a/git-mergetool.sh +++ b/git-mergetool.sh @@ -274,6 +274,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 +323,15 @@ merge_file () { checkout_staged_file 2 "$MERGED" "$LOCAL" checkout_staged_file 3 "$MERGED" "$REMOTE" + if test "$(git config --bool mergetool.autoMerge)" = "true" + then + git merge-file --diff3 -q -p "$LOCAL" "$BASE" "$REMOTE" >"$DIFF3" + sed -e '/^<<<<<<< /,/^||||||| /d' -e '/^=======\r\?$/,/^>>>>>>> /d' "$DIFF3" >"$BASE" + sed -e '/^||||||| /,/^>>>>>>> /d' -e '/^<<<<<<< /d' "$DIFF3" >"$LOCAL" + sed -e '/^<<<<<<< /,/^=======\r\?$/d' -e '/^>>>>>>> /d' "$DIFF3" >"$REMOTE" + rm -- "$DIFF3" + 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..b75c91199b 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 && + echo -e "base\n\na" >file1 && + git commit -a -m "base" && + echo -e "base\n\nc" >file1 && + git commit -a -m "remote update" && + git checkout -b test${test_count}_a HEAD~ && + echo -e "local\n\nb" >file1 && + git commit -a -m "local update" && + test_must_fail git merge test${test_count}_b && + yes "" | git mergetool file1 && + echo -e "local\n\nc" >expect && + test_cmp expect file1 && + git commit -m "test resolved with mergetool" +' + test_done -- 2.29.2 ^ permalink raw reply related [flat|nested] 80+ messages in thread
* Re: [PATCH v6 1/2] mergetool: add automerge configuration 2020-12-27 20:58 ` [PATCH v6 1/2] " Seth House @ 2020-12-27 22:06 ` Junio C Hamano 2020-12-27 22:29 ` Seth House 0 siblings, 1 reply; 80+ messages in thread From: Junio C Hamano @ 2020-12-27 22:06 UTC (permalink / raw) To: Seth House; +Cc: git, Felipe Contreras Seth House <seth@eseth.com> writes: > ... > 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> > --- Missing Sign-off as a relayer. > diff --git a/t/t7610-mergetool.sh b/t/t7610-mergetool.sh > index 70afdd06fa..b75c91199b 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 && > + echo -e "base\n\na" >file1 && These do not seem to be taken from the version that has been improved by reviwer comments after v3. > + git commit -a -m "base" && > + echo -e "base\n\nc" >file1 && > + git commit -a -m "remote update" && > + git checkout -b test${test_count}_a HEAD~ && > + echo -e "local\n\nb" >file1 && > + git commit -a -m "local update" && > + test_must_fail git merge test${test_count}_b && > + yes "" | git mergetool file1 && > + echo -e "local\n\nc" >expect && > + test_cmp expect file1 && > + git commit -m "test resolved with mergetool" > +' > + > test_done ^ permalink raw reply [flat|nested] 80+ messages in thread
* Re: [PATCH v6 1/2] mergetool: add automerge configuration 2020-12-27 22:06 ` Junio C Hamano @ 2020-12-27 22:29 ` Seth House 2020-12-27 22:59 ` Junio C Hamano 0 siblings, 1 reply; 80+ messages in thread From: Seth House @ 2020-12-27 22:29 UTC (permalink / raw) To: Junio C Hamano; +Cc: git, Felipe Contreras On Sun, Dec 27, 2020 at 02:06:58PM -0800, Junio C Hamano wrote: > Missing Sign-off as a relayer. I haven't come across that in the docs on contributing to Git and my Google searches aren't helping. Do you mind pointing me to what to add? > These do not seem to be taken from the version that has been > improved by reviwer comments after v3. Whoops! Thanks for the catch. Seems I fished the wrong version out of my email. Created a new v7 based off the correct v5. ^ permalink raw reply [flat|nested] 80+ messages in thread
* Re: [PATCH v6 1/2] mergetool: add automerge configuration 2020-12-27 22:29 ` Seth House @ 2020-12-27 22:59 ` Junio C Hamano 0 siblings, 0 replies; 80+ messages in thread From: Junio C Hamano @ 2020-12-27 22:59 UTC (permalink / raw) To: Seth House; +Cc: git, Felipe Contreras Seth House <seth@eseth.com> writes: > On Sun, Dec 27, 2020 at 02:06:58PM -0800, Junio C Hamano wrote: >> Missing Sign-off as a relayer. > > I haven't come across that in the docs on contributing to Git and my > Google searches aren't helping. Do you mind pointing me to what to add? Documentation/SubmittingPatches#sign-off === Certify your work by adding your `Signed-off-by` trailer To improve tracking of who did what, we ask you to certify that you wrote the patch or have the right to pass it on under the same license as ours, by "signing off" your patch. Without sign-off, we cannot accept your patches. If you can certify the below D-C-O: [[dco]] .Developer's Certificate of Origin 1.1 ____ By making a contribution to this project, I certify that: a. The contribution was created in whole or in part by me and I have the right to submit it under the open source license indicated in the file; or b. The contribution is based upon previous work that, to the best of my knowledge, is covered under an appropriate open source license and I have the right under that license to submit that work with modifications, whether created in whole or in part by me, under the same open source license (unless I am permitted to submit under a different license), as indicated in the file; or c. The contribution was provided directly to me by some other person who certified (a), (b) or (c) and I have not modified it. d. I understand and agree that this project and the contribution are public and that a record of the contribution (including all personal information I submit with it, including my sign-off) is maintained indefinitely and may be redistributed consistent with this project or the open source license(s) involved. ____ you add a "Signed-off-by" trailer to your commit, that looks like this: .... Signed-off-by: Random J Developer <random@developer.example.org> .... So, you'd add your own signed-off-by trailer at the end of the trailer list. https://lore.kernel.org/git/pull.805.git.1607091741254.gitgitgadget@gmail.com/ for an example where Johannes Schindelin picked up a patch written by Dennis Ameling and relayed it to the list. Thanks. ^ permalink raw reply [flat|nested] 80+ messages in thread
* [PATCH v6 2/2] mergetool: Add per-tool support for the autoMerge flag 2020-12-27 20:58 ` [PATCH v6 0/1] mergetool: add automerge configuration Seth House 2020-12-27 20:58 ` [PATCH v6 1/2] " Seth House @ 2020-12-27 20:58 ` Seth House 2020-12-27 22:31 ` Junio C Hamano 2020-12-28 0:41 ` [PATCH v7 0/2] mergetool: add automerge configuration Seth House 2020-12-28 1:02 ` [PATCH v6 0/1] " Felipe Contreras 3 siblings, 1 reply; 80+ messages in thread From: Seth House @ 2020-12-27 20:58 UTC (permalink / raw) To: git; +Cc: Seth House Keep the global mergetool flag and add a per-tool override flag so that users may enable the flag for one tool and disable it for another. Signed-off-by: Seth House <seth@eseth.com> --- Documentation/config/mergetool.txt | 3 +++ git-mergetool.sh | 5 ++++- 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/Documentation/config/mergetool.txt b/Documentation/config/mergetool.txt index 43af7a96f9..7f32281a61 100644 --- a/Documentation/config/mergetool.txt +++ b/Documentation/config/mergetool.txt @@ -21,6 +21,9 @@ mergetool.<tool>.trustExitCode:: if the file has been updated, otherwise the user is prompted to indicate the success of the merge. +mergetool.<tool>.autoMerge:: + Automatically resolve conflicts that don't require user intervention. + mergetool.meld.hasOutput:: Older versions of `meld` do not support the `--output` option. Git will attempt to detect whether `meld` supports `--output` diff --git a/git-mergetool.sh b/git-mergetool.sh index 6e86d3b492..81df301734 100755 --- a/git-mergetool.sh +++ b/git-mergetool.sh @@ -323,7 +323,10 @@ merge_file () { checkout_staged_file 2 "$MERGED" "$LOCAL" checkout_staged_file 3 "$MERGED" "$REMOTE" - if test "$(git config --bool mergetool.autoMerge)" = "true" + if test "$( + git config --get --bool "mergetool.$merge_tool.automerge" || + git config --get --bool "mergetool.automerge" || + echo true)" = true then git merge-file --diff3 -q -p "$LOCAL" "$BASE" "$REMOTE" >"$DIFF3" sed -e '/^<<<<<<< /,/^||||||| /d' -e '/^=======\r\?$/,/^>>>>>>> /d' "$DIFF3" >"$BASE" -- 2.29.2 ^ permalink raw reply related [flat|nested] 80+ messages in thread
* Re: [PATCH v6 2/2] mergetool: Add per-tool support for the autoMerge flag 2020-12-27 20:58 ` [PATCH v6 2/2] mergetool: Add per-tool support for the autoMerge flag Seth House @ 2020-12-27 22:31 ` Junio C Hamano 0 siblings, 0 replies; 80+ messages in thread From: Junio C Hamano @ 2020-12-27 22:31 UTC (permalink / raw) To: Seth House; +Cc: git Seth House <seth@eseth.com> writes: > Keep the global mergetool flag and add a per-tool override flag so that > users may enable the flag for one tool and disable it for another. > > Signed-off-by: Seth House <seth@eseth.com> > --- > Documentation/config/mergetool.txt | 3 +++ > git-mergetool.sh | 5 ++++- > 2 files changed, 7 insertions(+), 1 deletion(-) > > diff --git a/Documentation/config/mergetool.txt b/Documentation/config/mergetool.txt > index 43af7a96f9..7f32281a61 100644 > --- a/Documentation/config/mergetool.txt > +++ b/Documentation/config/mergetool.txt > @@ -21,6 +21,9 @@ mergetool.<tool>.trustExitCode:: > if the file has been updated, otherwise the user is prompted to > indicate the success of the merge. > > +mergetool.<tool>.autoMerge:: > + Automatically resolve conflicts that don't require user intervention. > + > mergetool.meld.hasOutput:: > Older versions of `meld` do not support the `--output` option. > Git will attempt to detect whether `meld` supports `--output` > diff --git a/git-mergetool.sh b/git-mergetool.sh > index 6e86d3b492..81df301734 100755 > --- a/git-mergetool.sh > +++ b/git-mergetool.sh > @@ -323,7 +323,10 @@ merge_file () { > checkout_staged_file 2 "$MERGED" "$LOCAL" > checkout_staged_file 3 "$MERGED" "$REMOTE" > > - if test "$(git config --bool mergetool.autoMerge)" = "true" > + if test "$( > + git config --get --bool "mergetool.$merge_tool.automerge" || > + git config --get --bool "mergetool.automerge" || > + echo true)" = true Your [v6 1/2] that you build this step on does not enable the feature by default, but this step does; it deserves to be documented and mentioned in the proposed log message. But I think you'd want to build this step on top of newer one, if only to take the portability fix to the tests, and that patch enables the feature by default, so ... > then > git merge-file --diff3 -q -p "$LOCAL" "$BASE" "$REMOTE" >"$DIFF3" > sed -e '/^<<<<<<< /,/^||||||| /d' -e '/^=======\r\?$/,/^>>>>>>> /d' "$DIFF3" >"$BASE" Thanks. ^ permalink raw reply [flat|nested] 80+ messages in thread
* [PATCH v7 0/2] mergetool: add automerge configuration 2020-12-27 20:58 ` [PATCH v6 0/1] mergetool: add automerge configuration Seth House 2020-12-27 20:58 ` [PATCH v6 1/2] " Seth House 2020-12-27 20:58 ` [PATCH v6 2/2] mergetool: Add per-tool support for the autoMerge flag Seth House @ 2020-12-28 0:41 ` Seth House 2020-12-28 0:41 ` [PATCH v7 1/2] " Seth House ` (3 more replies) 2020-12-28 1:02 ` [PATCH v6 0/1] " Felipe Contreras 3 siblings, 4 replies; 80+ messages in thread From: Seth House @ 2020-12-28 0:41 UTC (permalink / raw) To: git Cc: Seth House, Junio C Hamano, David Aguilar, Johannes Sixt, Felipe Contreras Changes since v6: * Incorporated Junio's help and advice: * Rebased v7 off of the correct v5 version. * Signed off on Felipe's commit. (Although I have minor qualms with Felipe's various wording and even the name of the flag it is decidedly not worth burdening the list with bike-shedding.) Changes since v5: * Add per-tool configuration that Felipe has a "deep philosophical" opposition to adding. Felipe Contreras (1): mergetool: add automerge configuration Seth House (1): mergetool: Add per-tool support for the autoMerge flag Documentation/config/mergetool.txt | 6 ++++++ git-mergetool.sh | 20 ++++++++++++++++++++ t/t7610-mergetool.sh | 18 ++++++++++++++++++ 3 files changed, 44 insertions(+) -- 2.29.2 ^ permalink raw reply [flat|nested] 80+ messages in thread
* [PATCH v7 1/2] mergetool: add automerge configuration 2020-12-28 0:41 ` [PATCH v7 0/2] mergetool: add automerge configuration Seth House @ 2020-12-28 0:41 ` Seth House 2020-12-28 0:41 ` [PATCH v7 2/2] mergetool: Add per-tool support for the autoMerge flag Seth House ` (2 subsequent siblings) 3 siblings, 0 replies; 80+ messages in thread From: Seth House @ 2020-12-28 0:41 UTC (permalink / raw) To: git; +Cc: Felipe Contreras, Seth House From: Felipe Contreras <felipe.contreras@gmail.com> 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> Signed-off-by: Seth House <seth@eseth.com> --- Documentation/config/mergetool.txt | 3 +++ git-mergetool.sh | 17 +++++++++++++++++ t/t7610-mergetool.sh | 18 ++++++++++++++++++ 3 files changed, 38 insertions(+) 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 -- 2.29.2 ^ permalink raw reply related [flat|nested] 80+ messages in thread
* [PATCH v7 2/2] mergetool: Add per-tool support for the autoMerge flag 2020-12-28 0:41 ` [PATCH v7 0/2] mergetool: add automerge configuration Seth House 2020-12-28 0:41 ` [PATCH v7 1/2] " Seth House @ 2020-12-28 0:41 ` Seth House 2020-12-28 1:18 ` Felipe Contreras 2020-12-28 4:54 ` [PATCH v8 0/4] mergetool: add automerge configuration Seth House 2020-12-28 10:29 ` [PATCH v7 0/2] mergetool: add automerge configuration Junio C Hamano 3 siblings, 1 reply; 80+ messages in thread From: Seth House @ 2020-12-28 0:41 UTC (permalink / raw) To: git; +Cc: Seth House Keep the global mergetool flag and add a per-tool override flag so that users may enable the flag for one tool and disable it for another. Signed-off-by: Seth House <seth@eseth.com> --- Documentation/config/mergetool.txt | 3 +++ git-mergetool.sh | 5 ++++- 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/Documentation/config/mergetool.txt b/Documentation/config/mergetool.txt index 7ce6d0d3ac..ef147fc118 100644 --- a/Documentation/config/mergetool.txt +++ b/Documentation/config/mergetool.txt @@ -21,6 +21,9 @@ mergetool.<tool>.trustExitCode:: if the file has been updated, otherwise the user is prompted to indicate the success of the merge. +mergetool.<tool>.autoMerge:: + Remove lines without conflicts from all the files. Defaults to `true`. + mergetool.meld.hasOutput:: Older versions of `meld` do not support the `--output` option. Git will attempt to detect whether `meld` supports `--output` diff --git a/git-mergetool.sh b/git-mergetool.sh index f4db0cac8d..e3c7d78d1d 100755 --- a/git-mergetool.sh +++ b/git-mergetool.sh @@ -334,7 +334,10 @@ merge_file () { checkout_staged_file 2 "$MERGED" "$LOCAL" checkout_staged_file 3 "$MERGED" "$REMOTE" - if test "$(git config --bool mergetool.autoMerge)" != "false" + if test "$( + git config --get --bool "mergetool.$merge_tool.automerge" || + git config --get --bool "mergetool.automerge" || + echo true)" = true then auto_merge fi -- 2.29.2 ^ permalink raw reply related [flat|nested] 80+ messages in thread
* RE: [PATCH v7 2/2] mergetool: Add per-tool support for the autoMerge flag 2020-12-28 0:41 ` [PATCH v7 2/2] mergetool: Add per-tool support for the autoMerge flag Seth House @ 2020-12-28 1:18 ` Felipe Contreras 0 siblings, 0 replies; 80+ messages in thread From: Felipe Contreras @ 2020-12-28 1:18 UTC (permalink / raw) To: Seth House, git; +Cc: Seth House Seth House wrote: > diff --git a/git-mergetool.sh b/git-mergetool.sh > index f4db0cac8d..e3c7d78d1d 100755 > --- a/git-mergetool.sh > +++ b/git-mergetool.sh > @@ -334,7 +334,10 @@ merge_file () { > checkout_staged_file 2 "$MERGED" "$LOCAL" > checkout_staged_file 3 "$MERGED" "$REMOTE" > > - if test "$(git config --bool mergetool.autoMerge)" != "false" > + if test "$( > + git config --get --bool "mergetool.$merge_tool.automerge" || > + git config --get --bool "mergetool.automerge" || > + echo true)" = true This is a per-tool user configuration. Wasn't your argument that some tools would want to disable this flag? That is; the tool, not the user. For example, the author of diffconflicts might want to disable this flag for all its users, or at least disable it by default. How can the winmerge difftool disable this flag? > then > auto_merge > fi -- Felipe Contreras ^ permalink raw reply [flat|nested] 80+ messages in thread
* [PATCH v8 0/4] mergetool: add automerge configuration 2020-12-28 0:41 ` [PATCH v7 0/2] mergetool: add automerge configuration Seth House 2020-12-28 0:41 ` [PATCH v7 1/2] " Seth House 2020-12-28 0:41 ` [PATCH v7 2/2] mergetool: Add per-tool support for the autoMerge flag Seth House @ 2020-12-28 4:54 ` Seth House 2020-12-28 4:54 ` [PATCH v8 1/4] " Seth House ` (4 more replies) 2020-12-28 10:29 ` [PATCH v7 0/2] mergetool: add automerge configuration Junio C Hamano 3 siblings, 5 replies; 80+ messages in thread From: Seth House @ 2020-12-28 4:54 UTC (permalink / raw) To: git Cc: Seth House, Junio C Hamano, David Aguilar, Johannes Sixt, Felipe Contreras Changes since v7: * Add a tool-specific override function to setup_tool based on Junio's original patch feedback. The implementation of initialize_merge_tool is very much not set in stone. Suggestions are very welcome for alternate approaches that are less invasive. Felipe Contreras (1): mergetool: add automerge configuration Seth House (3): mergetool: Add per-tool support for the autoMerge flag mergetool: Break setup_tool out into separate initialization function mergetool: Add automerge_enabled tool-specific override function Documentation/config/mergetool.txt | 6 ++++++ Documentation/git-mergetool--lib.txt | 4 ++++ git-difftool--helper.sh | 2 ++ git-mergetool--lib.sh | 11 ++++++++--- git-mergetool.sh | 22 ++++++++++++++++++++++ t/t7610-mergetool.sh | 18 ++++++++++++++++++ 6 files changed, 60 insertions(+), 3 deletions(-) -- 2.29.2 ^ permalink raw reply [flat|nested] 80+ messages in thread
* [PATCH v8 1/4] mergetool: add automerge configuration 2020-12-28 4:54 ` [PATCH v8 0/4] mergetool: add automerge configuration Seth House @ 2020-12-28 4:54 ` Seth House 2020-12-28 11:30 ` Johannes Sixt 2020-12-28 4:54 ` [PATCH v8 2/4] mergetool: Add per-tool support for the autoMerge flag Seth House ` (3 subsequent siblings) 4 siblings, 1 reply; 80+ messages in thread From: Seth House @ 2020-12-28 4:54 UTC (permalink / raw) To: git; +Cc: Felipe Contreras, Seth House From: Felipe Contreras <felipe.contreras@gmail.com> 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> Signed-off-by: Seth House <seth@eseth.com> --- Documentation/config/mergetool.txt | 3 +++ git-mergetool.sh | 17 +++++++++++++++++ t/t7610-mergetool.sh | 18 ++++++++++++++++++ 3 files changed, 38 insertions(+) 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 -- 2.29.2 ^ permalink raw reply related [flat|nested] 80+ messages in thread
* Re: [PATCH v8 1/4] mergetool: add automerge configuration 2020-12-28 4:54 ` [PATCH v8 1/4] " Seth House @ 2020-12-28 11:30 ` Johannes Sixt 0 siblings, 0 replies; 80+ messages in thread From: Johannes Sixt @ 2020-12-28 11:30 UTC (permalink / raw) To: Seth House; +Cc: Felipe Contreras, git Am 28.12.20 um 05:54 schrieb Seth House: > 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`. This text, starting with "Remove lines", sounds alarming. Isn't it more along the lines of: Consolidate non-conflicting parts, so that only conflicting parts are presented to the merge tool. Defaults to `true`. It would be great to keep the list of config entries sorted. I know that mergetool.prompt is not at the correct location, but that shouldn't be an excuse to make the situation worse. -- Hannes ^ permalink raw reply [flat|nested] 80+ messages in thread
* [PATCH v8 2/4] mergetool: Add per-tool support for the autoMerge flag 2020-12-28 4:54 ` [PATCH v8 0/4] mergetool: add automerge configuration Seth House 2020-12-28 4:54 ` [PATCH v8 1/4] " Seth House @ 2020-12-28 4:54 ` Seth House 2020-12-28 13:09 ` Junio C Hamano 2020-12-28 4:54 ` [PATCH v8 3/4] mergetool: Break setup_tool out into separate initialization function Seth House ` (2 subsequent siblings) 4 siblings, 1 reply; 80+ messages in thread From: Seth House @ 2020-12-28 4:54 UTC (permalink / raw) To: git; +Cc: Seth House Keep the global mergetool flag and add a per-tool override flag so that users may enable the flag for one tool and disable it for another. Signed-off-by: Seth House <seth@eseth.com> --- Documentation/config/mergetool.txt | 3 +++ git-mergetool.sh | 5 ++++- 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/Documentation/config/mergetool.txt b/Documentation/config/mergetool.txt index 7ce6d0d3ac..ef147fc118 100644 --- a/Documentation/config/mergetool.txt +++ b/Documentation/config/mergetool.txt @@ -21,6 +21,9 @@ mergetool.<tool>.trustExitCode:: if the file has been updated, otherwise the user is prompted to indicate the success of the merge. +mergetool.<tool>.autoMerge:: + Remove lines without conflicts from all the files. Defaults to `true`. + mergetool.meld.hasOutput:: Older versions of `meld` do not support the `--output` option. Git will attempt to detect whether `meld` supports `--output` diff --git a/git-mergetool.sh b/git-mergetool.sh index f4db0cac8d..e3c7d78d1d 100755 --- a/git-mergetool.sh +++ b/git-mergetool.sh @@ -334,7 +334,10 @@ merge_file () { checkout_staged_file 2 "$MERGED" "$LOCAL" checkout_staged_file 3 "$MERGED" "$REMOTE" - if test "$(git config --bool mergetool.autoMerge)" != "false" + if test "$( + git config --get --bool "mergetool.$merge_tool.automerge" || + git config --get --bool "mergetool.automerge" || + echo true)" = true then auto_merge fi -- 2.29.2 ^ permalink raw reply related [flat|nested] 80+ messages in thread
* Re: [PATCH v8 2/4] mergetool: Add per-tool support for the autoMerge flag 2020-12-28 4:54 ` [PATCH v8 2/4] mergetool: Add per-tool support for the autoMerge flag Seth House @ 2020-12-28 13:09 ` Junio C Hamano 0 siblings, 0 replies; 80+ messages in thread From: Junio C Hamano @ 2020-12-28 13:09 UTC (permalink / raw) To: Seth House; +Cc: git > Subject: Re: [PATCH v8 2/4] mergetool: Add per-tool support for the autoMerge flag "git shortlog --no-merges --since=2.months" may tell you this but our convention is not to capitalize the word after "<area>:" on the title. > Keep the global mergetool flag and add a per-tool override flag so that > users may enable the flag for one tool and disable it for another. > > Signed-off-by: Seth House <seth@eseth.com> > --- > Documentation/config/mergetool.txt | 3 +++ > git-mergetool.sh | 5 ++++- > 2 files changed, 7 insertions(+), 1 deletion(-) > > diff --git a/Documentation/config/mergetool.txt b/Documentation/config/mergetool.txt > index 7ce6d0d3ac..ef147fc118 100644 > --- a/Documentation/config/mergetool.txt > +++ b/Documentation/config/mergetool.txt > @@ -21,6 +21,9 @@ mergetool.<tool>.trustExitCode:: > if the file has been updated, otherwise the user is prompted to > indicate the success of the merge. > > +mergetool.<tool>.autoMerge:: > + Remove lines without conflicts from all the files. Defaults to `true`. > + This entry needs to mention how it relates to the big red button mergetool.autoMerge and vice versa. E.g. mergetool.autoMerge:: - Remove lines without conflicts from all the files. Defaults to `true`. + Remove lines without conflicts from all the files. Can be + overriden per-tool via `mergetool.<tool>.autoMerge` configuration + variable. Defaults to `true`. would be a good update to the documentation introduced by the previous step. It is somewhat misleading for the per-tool entry added in this patch to say "Defaults to `true`", as the value of the big red button configuration would be the real default. Remove ... the files when the mergetool '<tool>' is in use. See also `mergetool.autoMerge`. or something like that, perhaps. ^ permalink raw reply [flat|nested] 80+ messages in thread
* [PATCH v8 3/4] mergetool: Break setup_tool out into separate initialization function 2020-12-28 4:54 ` [PATCH v8 0/4] mergetool: add automerge configuration Seth House 2020-12-28 4:54 ` [PATCH v8 1/4] " Seth House 2020-12-28 4:54 ` [PATCH v8 2/4] mergetool: Add per-tool support for the autoMerge flag Seth House @ 2020-12-28 4:54 ` Seth House 2020-12-28 11:48 ` Johannes Sixt 2020-12-28 4:54 ` [PATCH v8 4/4] mergetool: Add automerge_enabled tool-specific override function Seth House 2020-12-28 19:29 ` [PATCH v9 0/5] mergetool: add automerge configuration Seth House 4 siblings, 1 reply; 80+ messages in thread From: Seth House @ 2020-12-28 4:54 UTC (permalink / raw) To: git; +Cc: Seth House The tool-specific functions are sometimes needed in scope earlier than when run_merge_tool is called. Signed-off-by: Seth House <seth@eseth.com> --- Documentation/git-mergetool--lib.txt | 4 ++++ git-difftool--helper.sh | 2 ++ git-mergetool--lib.sh | 7 ++++--- git-mergetool.sh | 2 ++ 4 files changed, 12 insertions(+), 3 deletions(-) diff --git a/Documentation/git-mergetool--lib.txt b/Documentation/git-mergetool--lib.txt index 4da9d24096..3e8f59ac0e 100644 --- a/Documentation/git-mergetool--lib.txt +++ b/Documentation/git-mergetool--lib.txt @@ -38,6 +38,10 @@ get_merge_tool_cmd:: get_merge_tool_path:: returns the custom path for a merge tool. +initialize_merge_tool:: + bring merge tool specific functions into scope so they can be used or + overridden. + run_merge_tool:: launches a merge tool given the tool name and a true/false flag to indicate whether a merge base is present. diff --git a/git-difftool--helper.sh b/git-difftool--helper.sh index 46af3e60b7..c47a6d4253 100755 --- a/git-difftool--helper.sh +++ b/git-difftool--helper.sh @@ -61,6 +61,7 @@ launch_merge_tool () { export BASE eval $GIT_DIFFTOOL_EXTCMD '"$LOCAL"' '"$REMOTE"' else + initialize_merge_tool "$merge_tool" run_merge_tool "$merge_tool" fi } @@ -79,6 +80,7 @@ if test -n "$GIT_DIFFTOOL_DIRDIFF" then LOCAL="$1" REMOTE="$2" + initialize_merge_tool "$merge_tool" run_merge_tool "$merge_tool" false else # Launch the merge tool on each path provided by 'git diff' diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh index 7225abd811..e059b3559e 100644 --- a/git-mergetool--lib.sh +++ b/git-mergetool--lib.sh @@ -248,6 +248,10 @@ trust_exit_code () { fi } +initialize_merge_tool () { + # Bring tool-specific functions into scope + setup_tool "$1" || return 1 +} # Entry point for running tools run_merge_tool () { @@ -259,9 +263,6 @@ run_merge_tool () { merge_tool_path=$(get_merge_tool_path "$1") || exit base_present="$2" - # Bring tool-specific functions into scope - setup_tool "$1" || return 1 - if merge_mode then run_merge_cmd "$1" diff --git a/git-mergetool.sh b/git-mergetool.sh index e3c7d78d1d..929192d0f8 100755 --- a/git-mergetool.sh +++ b/git-mergetool.sh @@ -334,6 +334,8 @@ merge_file () { checkout_staged_file 2 "$MERGED" "$LOCAL" checkout_staged_file 3 "$MERGED" "$REMOTE" + initialize_merge_tool "$merge_tool" + if test "$( git config --get --bool "mergetool.$merge_tool.automerge" || git config --get --bool "mergetool.automerge" || -- 2.29.2 ^ permalink raw reply related [flat|nested] 80+ messages in thread
* Re: [PATCH v8 3/4] mergetool: Break setup_tool out into separate initialization function 2020-12-28 4:54 ` [PATCH v8 3/4] mergetool: Break setup_tool out into separate initialization function Seth House @ 2020-12-28 11:48 ` Johannes Sixt 0 siblings, 0 replies; 80+ messages in thread From: Johannes Sixt @ 2020-12-28 11:48 UTC (permalink / raw) To: Seth House; +Cc: git Am 28.12.20 um 05:54 schrieb Seth House: > The tool-specific functions are sometimes needed in scope earlier than > when run_merge_tool is called. You should answer why this change is needed. "are sometimes needed in scope earlier" cannot be true, else we would have a bug that is fixed by this change. But this isn't a bug fix, is it? It would be ok to say "We are going to add another thing that we will need before run_merge_tool; this is a preparation" or something. Which brings me to another point: I do not see that something is added to initialize_merge_tool in a later patch. You are only replacing setup_tool calls by initialize_merge_tool, which forwards to setup_tool. Why do we need a new function? > > Signed-off-by: Seth House <seth@eseth.com> > --- > Documentation/git-mergetool--lib.txt | 4 ++++ > git-difftool--helper.sh | 2 ++ > git-mergetool--lib.sh | 7 ++++--- > git-mergetool.sh | 2 ++ > 4 files changed, 12 insertions(+), 3 deletions(-) > > diff --git a/Documentation/git-mergetool--lib.txt b/Documentation/git-mergetool--lib.txt > index 4da9d24096..3e8f59ac0e 100644 > --- a/Documentation/git-mergetool--lib.txt > +++ b/Documentation/git-mergetool--lib.txt > @@ -38,6 +38,10 @@ get_merge_tool_cmd:: > get_merge_tool_path:: > returns the custom path for a merge tool. > > +initialize_merge_tool:: > + bring merge tool specific functions into scope so they can be used or > + overridden. > + > run_merge_tool:: > launches a merge tool given the tool name and a true/false > flag to indicate whether a merge base is present. [ swapped hunks for better sentence structure ] > diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh > index 7225abd811..e059b3559e 100644 > --- a/git-mergetool--lib.sh > +++ b/git-mergetool--lib.sh > @@ -248,6 +248,10 @@ trust_exit_code () { > fi > } > > +initialize_merge_tool () { > + # Bring tool-specific functions into scope > + setup_tool "$1" || return 1 > +} > > # Entry point for running tools > run_merge_tool () { > @@ -259,9 +263,6 @@ run_merge_tool () { > merge_tool_path=$(get_merge_tool_path "$1") || exit > base_present="$2" > > - # Bring tool-specific functions into scope > - setup_tool "$1" || return 1 > - Before this change, run_merge_tool would exit here when there was an error during setup_tool. But... > diff --git a/git-difftool--helper.sh b/git-difftool--helper.sh > index 46af3e60b7..c47a6d4253 100755 > --- a/git-difftool--helper.sh > +++ b/git-difftool--helper.sh > @@ -61,6 +61,7 @@ launch_merge_tool () { > export BASE > eval $GIT_DIFFTOOL_EXTCMD '"$LOCAL"' '"$REMOTE"' > else > + initialize_merge_tool "$merge_tool" > run_merge_tool "$merge_tool" > fi ... after the change we do not exit anymore. Does it matter? Perhaps initialize_merge_tool "$merge_tool" && run_merge_tool "$merge_tool" > } > @@ -79,6 +80,7 @@ if test -n "$GIT_DIFFTOOL_DIRDIFF" > then > LOCAL="$1" > REMOTE="$2" > + initialize_merge_tool "$merge_tool" > run_merge_tool "$merge_tool" false > else > # Launch the merge tool on each path provided by 'git diff' > diff --git a/git-mergetool.sh b/git-mergetool.sh > index e3c7d78d1d..929192d0f8 100755 > --- a/git-mergetool.sh > +++ b/git-mergetool.sh > @@ -334,6 +334,8 @@ merge_file () { > checkout_staged_file 2 "$MERGED" "$LOCAL" > checkout_staged_file 3 "$MERGED" "$REMOTE" > > + initialize_merge_tool "$merge_tool" > + > if test "$( > git config --get --bool "mergetool.$merge_tool.automerge" || > git config --get --bool "mergetool.automerge" || > ^ permalink raw reply [flat|nested] 80+ messages in thread
* [PATCH v8 4/4] mergetool: Add automerge_enabled tool-specific override function 2020-12-28 4:54 ` [PATCH v8 0/4] mergetool: add automerge configuration Seth House ` (2 preceding siblings ...) 2020-12-28 4:54 ` [PATCH v8 3/4] mergetool: Break setup_tool out into separate initialization function Seth House @ 2020-12-28 4:54 ` Seth House 2020-12-28 11:57 ` Johannes Sixt 2020-12-28 13:19 ` Junio C Hamano 2020-12-28 19:29 ` [PATCH v9 0/5] mergetool: add automerge configuration Seth House 4 siblings, 2 replies; 80+ messages in thread From: Seth House @ 2020-12-28 4:54 UTC (permalink / raw) To: git; +Cc: Seth House Hat-tip to Junio C Hamano for the implementation. Signed-off-by: Seth House <seth@eseth.com> --- git-mergetool--lib.sh | 4 ++++ git-mergetool.sh | 2 +- 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh index e059b3559e..5084ceffeb 100644 --- a/git-mergetool--lib.sh +++ b/git-mergetool--lib.sh @@ -164,6 +164,10 @@ setup_tool () { return 1 } + automerge_enabled () { + true + } + translate_merge_tool_path () { echo "$1" } diff --git a/git-mergetool.sh b/git-mergetool.sh index 929192d0f8..a44afd3822 100755 --- a/git-mergetool.sh +++ b/git-mergetool.sh @@ -336,7 +336,7 @@ merge_file () { initialize_merge_tool "$merge_tool" - if test "$( + if automerge_enabled && test "$( git config --get --bool "mergetool.$merge_tool.automerge" || git config --get --bool "mergetool.automerge" || echo true)" = true -- 2.29.2 ^ permalink raw reply related [flat|nested] 80+ messages in thread
* Re: [PATCH v8 4/4] mergetool: Add automerge_enabled tool-specific override function 2020-12-28 4:54 ` [PATCH v8 4/4] mergetool: Add automerge_enabled tool-specific override function Seth House @ 2020-12-28 11:57 ` Johannes Sixt 2020-12-28 13:19 ` Junio C Hamano 1 sibling, 0 replies; 80+ messages in thread From: Johannes Sixt @ 2020-12-28 11:57 UTC (permalink / raw) To: Seth House; +Cc: git Am 28.12.20 um 05:54 schrieb Seth House: > Hat-tip to Junio C Hamano for the implementation. We usually write this as Helped-by: Junio C Hamano <gitster@pobox.com> > > Signed-off-by: Seth House <seth@eseth.com> > --- > git-mergetool--lib.sh | 4 ++++ > git-mergetool.sh | 2 +- > 2 files changed, 5 insertions(+), 1 deletion(-) > > diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh > index e059b3559e..5084ceffeb 100644 > --- a/git-mergetool--lib.sh > +++ b/git-mergetool--lib.sh > @@ -164,6 +164,10 @@ setup_tool () { > return 1 > } > > + automerge_enabled () { > + true I would have written this as `return 0` instead of `true` like some of the functions above this hunk. > + } > + > translate_merge_tool_path () { > echo "$1" > } > diff --git a/git-mergetool.sh b/git-mergetool.sh > index 929192d0f8..a44afd3822 100755 > --- a/git-mergetool.sh > +++ b/git-mergetool.sh > @@ -336,7 +336,7 @@ merge_file () { > > initialize_merge_tool "$merge_tool" > > - if test "$( > + if automerge_enabled && test "$( > git config --get --bool "mergetool.$merge_tool.automerge" || > git config --get --bool "mergetool.automerge" || > echo true)" = true > -- Hannes ^ permalink raw reply [flat|nested] 80+ messages in thread
* Re: [PATCH v8 4/4] mergetool: Add automerge_enabled tool-specific override function 2020-12-28 4:54 ` [PATCH v8 4/4] mergetool: Add automerge_enabled tool-specific override function Seth House 2020-12-28 11:57 ` Johannes Sixt @ 2020-12-28 13:19 ` Junio C Hamano 1 sibling, 0 replies; 80+ messages in thread From: Junio C Hamano @ 2020-12-28 13:19 UTC (permalink / raw) To: Seth House; +Cc: git Seth House <seth@eseth.com> writes: > Hat-tip to Junio C Hamano for the implementation. That is the least interesting thing the log message for this commit can talk about. Instead readers should be able to learn things like these from the log message (I am not saying the log message should have these in an enumerated list; I am just enumerating these as samples): - Why does this exist? - What does a tool author want to use the mechanism for, and how does the tool author use it? - This mechanism allows tool authors to say "never allow autoMerge for this tool", but there is no provision to let them say "always use autoMerge without allowing users to turn it off". It also needs a bit of documentation update to mention that individual mergetool backend can choose not to trigger the feature at all, even if the user configures it with mergetool.autoMerge and mergetool.<tool>.autoMerge options. Thanks. > Signed-off-by: Seth House <seth@eseth.com> > --- > git-mergetool--lib.sh | 4 ++++ > git-mergetool.sh | 2 +- > 2 files changed, 5 insertions(+), 1 deletion(-) > > diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh > index e059b3559e..5084ceffeb 100644 > --- a/git-mergetool--lib.sh > +++ b/git-mergetool--lib.sh > @@ -164,6 +164,10 @@ setup_tool () { > return 1 > } > > + automerge_enabled () { > + true > + } > + > translate_merge_tool_path () { > echo "$1" > } > diff --git a/git-mergetool.sh b/git-mergetool.sh > index 929192d0f8..a44afd3822 100755 > --- a/git-mergetool.sh > +++ b/git-mergetool.sh > @@ -336,7 +336,7 @@ merge_file () { > > initialize_merge_tool "$merge_tool" > > - if test "$( > + if automerge_enabled && test "$( > git config --get --bool "mergetool.$merge_tool.automerge" || > git config --get --bool "mergetool.automerge" || > echo true)" = true ^ permalink raw reply [flat|nested] 80+ messages in thread
* [PATCH v9 0/5] mergetool: add automerge configuration 2020-12-28 4:54 ` [PATCH v8 0/4] mergetool: add automerge configuration Seth House ` (3 preceding siblings ...) 2020-12-28 4:54 ` [PATCH v8 4/4] mergetool: Add automerge_enabled tool-specific override function Seth House @ 2020-12-28 19:29 ` Seth House 2020-12-28 19:29 ` [PATCH v9 1/5] " Seth House ` (5 more replies) 4 siblings, 6 replies; 80+ messages in thread From: Seth House @ 2020-12-28 19:29 UTC (permalink / raw) To: git Cc: Seth House, Junio C Hamano, David Aguilar, Johannes Sixt, Felipe Contreras Thank you Junio and Johannes for patiently pointing out all my newbie mistakes. Changes since v8: - Improve documentation of what the autoMerge flag does. - Add documentation note for the `automerge_enabled` override. - Junio, is there another place this should also live? - Cross-reference merge.autoMerge and merge.<tool>.autoMerge docs. - Sort the list of mergetool options. Added as a standalone commit in case this change doesn't belong as part of this patchset. I'd be happy to move this to a standalone patch if that's preferable. - Improve all commit messages with full explanations and rationale: - Johannes, I didn't directly respond to your `initialize_merge_tool` question because you're right that explanation should be part of the commit message. Please let me know if that now answers your question and if not we can discuss further in a thread. - Fix omitted exit codes when running `initialize_merge_tool`. - Fix `automerge_enabled` return value for consistency. - Update commit message capitalization to conform to repo norms. - Rephrase commit message 'thanks' as Helped-by markers. Felipe Contreras (1): mergetool: add automerge configuration Seth House (4): mergetool: alphabetize the mergetool config docs mergetool: add per-tool support for the autoMerge flag mergetool: break setup_tool out into separate initialization function mergetool: add automerge_enabled tool-specific override function Documentation/config/mergetool.txt | 28 ++++++++++++++++++++++------ Documentation/git-mergetool--lib.txt | 4 ++++ git-difftool--helper.sh | 2 ++ git-mergetool--lib.sh | 11 ++++++++--- git-mergetool.sh | 22 ++++++++++++++++++++++ t/t7610-mergetool.sh | 18 ++++++++++++++++++ 6 files changed, 76 insertions(+), 9 deletions(-) -- 2.29.2 ^ permalink raw reply [flat|nested] 80+ messages in thread
* [PATCH v9 1/5] mergetool: add automerge configuration 2020-12-28 19:29 ` [PATCH v9 0/5] mergetool: add automerge configuration Seth House @ 2020-12-28 19:29 ` Seth House 2020-12-28 19:29 ` [PATCH v9 2/5] mergetool: alphabetize the mergetool config docs Seth House ` (4 subsequent siblings) 5 siblings, 0 replies; 80+ messages in thread From: Seth House @ 2020-12-28 19:29 UTC (permalink / raw) To: git; +Cc: Felipe Contreras, Seth House From: Felipe Contreras <felipe.contreras@gmail.com> 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> Signed-off-by: Seth House <seth@eseth.com> --- Documentation/config/mergetool.txt | 3 +++ git-mergetool.sh | 17 +++++++++++++++++ t/t7610-mergetool.sh | 18 ++++++++++++++++++ 3 files changed, 38 insertions(+) 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 -- 2.29.2 ^ permalink raw reply related [flat|nested] 80+ messages in thread
* [PATCH v9 2/5] mergetool: alphabetize the mergetool config docs 2020-12-28 19:29 ` [PATCH v9 0/5] mergetool: add automerge configuration Seth House 2020-12-28 19:29 ` [PATCH v9 1/5] " Seth House @ 2020-12-28 19:29 ` Seth House 2020-12-28 19:29 ` [PATCH v9 3/5] mergetool: add per-tool support for the autoMerge flag Seth House ` (3 subsequent siblings) 5 siblings, 0 replies; 80+ messages in thread From: Seth House @ 2020-12-28 19:29 UTC (permalink / raw) To: git; +Cc: Seth House The ordering in this file has drifted a little. Let's make things better while we're adding new entres. :) Signed-off-by: Seth House <seth@eseth.com> --- Documentation/config/mergetool.txt | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/Documentation/config/mergetool.txt b/Documentation/config/mergetool.txt index 7ce6d0d3ac..3291fa7102 100644 --- a/Documentation/config/mergetool.txt +++ b/Documentation/config/mergetool.txt @@ -1,7 +1,3 @@ -mergetool.<tool>.path:: - Override the path for the given tool. This is useful in case - your tool is not in the PATH. - mergetool.<tool>.cmd:: Specify the command to invoke the specified merge tool. The specified command is evaluated in shell with the following @@ -13,6 +9,10 @@ mergetool.<tool>.cmd:: merged; 'MERGED' contains the name of the file to which the merge tool should write the results of a successful merge. +mergetool.<tool>.path:: + Override the path for the given tool. This is useful in case + your tool is not in the PATH. + mergetool.<tool>.trustExitCode:: For a custom merge command, specify whether the exit code of the merge command can be used to determine whether the merge was @@ -40,6 +40,9 @@ mergetool.meld.useAutoMerge:: value of `false` avoids using `--auto-merge` altogether, and is the default value. +mergetool.autoMerge:: + Remove lines without conflicts from all the files. Defaults to `true`. + mergetool.keepBackup:: After performing a merge, the original file with conflict markers can be saved as a file with a `.orig` extension. If this variable @@ -53,14 +56,11 @@ mergetool.keepTemporaries:: preserved, otherwise they will be removed after the tool has exited. Defaults to `false`. +mergetool.prompt:: + Prompt before each invocation of the merge resolution program. + mergetool.writeToTemp:: Git writes temporary 'BASE', 'LOCAL', and 'REMOTE' versions of conflicting files in the worktree by default. Git will attempt to use a temporary directory for these files when set `true`. Defaults to `false`. - -mergetool.prompt:: - Prompt before each invocation of the merge resolution program. - -mergetool.autoMerge:: - Remove lines without conflicts from all the files. Defaults to `true`. -- 2.29.2 ^ permalink raw reply related [flat|nested] 80+ messages in thread
* [PATCH v9 3/5] mergetool: add per-tool support for the autoMerge flag 2020-12-28 19:29 ` [PATCH v9 0/5] mergetool: add automerge configuration Seth House 2020-12-28 19:29 ` [PATCH v9 1/5] " Seth House 2020-12-28 19:29 ` [PATCH v9 2/5] mergetool: alphabetize the mergetool config docs Seth House @ 2020-12-28 19:29 ` Seth House 2020-12-28 19:29 ` [PATCH v9 4/5] mergetool: break setup_tool out into separate initialization function Seth House ` (2 subsequent siblings) 5 siblings, 0 replies; 80+ messages in thread From: Seth House @ 2020-12-28 19:29 UTC (permalink / raw) To: git; +Cc: Seth House, Junio C Hamano, Johannes Sixt Keep the global mergetool flag and add a per-tool override flag so that users may enable the flag for one tool and disable it for another. Helped-by: Junio C Hamano <gitster@pobox.com> Helped-by: Johannes Sixt <j6t@kdbg.org> Signed-off-by: Seth House <seth@eseth.com> --- Documentation/config/mergetool.txt | 15 ++++++++++++++- git-mergetool.sh | 5 ++++- 2 files changed, 18 insertions(+), 2 deletions(-) diff --git a/Documentation/config/mergetool.txt b/Documentation/config/mergetool.txt index 3291fa7102..bde472d49a 100644 --- a/Documentation/config/mergetool.txt +++ b/Documentation/config/mergetool.txt @@ -1,3 +1,9 @@ +mergetool.<tool>.autoMerge:: + A mergetool-specific override for the global `mergetool.autoMerge` + configuration flag. This allows individual mergetools to enable or + disable the flag regardless of the global setting. See + `mergetool.autoMerge` for the full description. + mergetool.<tool>.cmd:: Specify the command to invoke the specified merge tool. The specified command is evaluated in shell with the following @@ -41,7 +47,14 @@ mergetool.meld.useAutoMerge:: default value. mergetool.autoMerge:: - Remove lines without conflicts from all the files. Defaults to `true`. + During a merge Git will automatically resolve as many conflicts as + possible and then wrap conflict markers around any conflicts that it + cannot resolve. This flag consolidates the non-conflicting parts into + the corresponding 'LOCAL' and 'REMOTE' files so that only the + unresolved conflicts are presented to the merge tool. Can be overriden + per-tool via the `mergetool.<tool>.autoMerge` configuration variable. + Note: individual mergetool scripts can elect to ignore user preferences + entirely. Defaults to `true`. mergetool.keepBackup:: After performing a merge, the original file with conflict markers diff --git a/git-mergetool.sh b/git-mergetool.sh index f4db0cac8d..e3c7d78d1d 100755 --- a/git-mergetool.sh +++ b/git-mergetool.sh @@ -334,7 +334,10 @@ merge_file () { checkout_staged_file 2 "$MERGED" "$LOCAL" checkout_staged_file 3 "$MERGED" "$REMOTE" - if test "$(git config --bool mergetool.autoMerge)" != "false" + if test "$( + git config --get --bool "mergetool.$merge_tool.automerge" || + git config --get --bool "mergetool.automerge" || + echo true)" = true then auto_merge fi -- 2.29.2 ^ permalink raw reply related [flat|nested] 80+ messages in thread
* [PATCH v9 4/5] mergetool: break setup_tool out into separate initialization function 2020-12-28 19:29 ` [PATCH v9 0/5] mergetool: add automerge configuration Seth House ` (2 preceding siblings ...) 2020-12-28 19:29 ` [PATCH v9 3/5] mergetool: add per-tool support for the autoMerge flag Seth House @ 2020-12-28 19:29 ` Seth House 2020-12-29 8:50 ` Johannes Sixt 2020-12-28 19:29 ` [PATCH v9 5/5] mergetool: add automerge_enabled tool-specific override function Seth House 2021-01-30 5:46 ` [PATCH v10 0/3] mergetool: add hideResolved configuration (was automerge) Seth House 5 siblings, 1 reply; 80+ messages in thread From: Seth House @ 2020-12-28 19:29 UTC (permalink / raw) To: git; +Cc: Seth House This is preparation for the following commit where we need to source the mergetool shell script to look for overrides before `run_merge_tool` is called. Previously `run_merge_tool` both sourced that script and invoked the mergetool. In the case of the following commit, we need the result of the `automerge_enabled` override, if it exists, well before we actually run `run_merge_tool`. A new function `initialize_merge_tool` was chosen for consistency with `run_merge_tool` since `setup_tool` and `setup_user_tool` are not exposed or called directly. Signed-off-by: Seth House <seth@eseth.com> --- Documentation/git-mergetool--lib.txt | 4 ++++ git-difftool--helper.sh | 2 ++ git-mergetool--lib.sh | 7 ++++--- git-mergetool.sh | 2 ++ 4 files changed, 12 insertions(+), 3 deletions(-) diff --git a/Documentation/git-mergetool--lib.txt b/Documentation/git-mergetool--lib.txt index 4da9d24096..3e8f59ac0e 100644 --- a/Documentation/git-mergetool--lib.txt +++ b/Documentation/git-mergetool--lib.txt @@ -38,6 +38,10 @@ get_merge_tool_cmd:: get_merge_tool_path:: returns the custom path for a merge tool. +initialize_merge_tool:: + bring merge tool specific functions into scope so they can be used or + overridden. + run_merge_tool:: launches a merge tool given the tool name and a true/false flag to indicate whether a merge base is present. diff --git a/git-difftool--helper.sh b/git-difftool--helper.sh index 46af3e60b7..234dd6944e 100755 --- a/git-difftool--helper.sh +++ b/git-difftool--helper.sh @@ -61,6 +61,7 @@ launch_merge_tool () { export BASE eval $GIT_DIFFTOOL_EXTCMD '"$LOCAL"' '"$REMOTE"' else + initialize_merge_tool "$merge_tool" && run_merge_tool "$merge_tool" fi } @@ -79,6 +80,7 @@ if test -n "$GIT_DIFFTOOL_DIRDIFF" then LOCAL="$1" REMOTE="$2" + initialize_merge_tool "$merge_tool" && run_merge_tool "$merge_tool" false else # Launch the merge tool on each path provided by 'git diff' diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh index 7225abd811..e059b3559e 100644 --- a/git-mergetool--lib.sh +++ b/git-mergetool--lib.sh @@ -248,6 +248,10 @@ trust_exit_code () { fi } +initialize_merge_tool () { + # Bring tool-specific functions into scope + setup_tool "$1" || return 1 +} # Entry point for running tools run_merge_tool () { @@ -259,9 +263,6 @@ run_merge_tool () { merge_tool_path=$(get_merge_tool_path "$1") || exit base_present="$2" - # Bring tool-specific functions into scope - setup_tool "$1" || return 1 - if merge_mode then run_merge_cmd "$1" diff --git a/git-mergetool.sh b/git-mergetool.sh index e3c7d78d1d..929192d0f8 100755 --- a/git-mergetool.sh +++ b/git-mergetool.sh @@ -334,6 +334,8 @@ merge_file () { checkout_staged_file 2 "$MERGED" "$LOCAL" checkout_staged_file 3 "$MERGED" "$REMOTE" + initialize_merge_tool "$merge_tool" + if test "$( git config --get --bool "mergetool.$merge_tool.automerge" || git config --get --bool "mergetool.automerge" || -- 2.29.2 ^ permalink raw reply related [flat|nested] 80+ messages in thread
* Re: [PATCH v9 4/5] mergetool: break setup_tool out into separate initialization function 2020-12-28 19:29 ` [PATCH v9 4/5] mergetool: break setup_tool out into separate initialization function Seth House @ 2020-12-29 8:50 ` Johannes Sixt 2020-12-29 17:23 ` Seth House 0 siblings, 1 reply; 80+ messages in thread From: Johannes Sixt @ 2020-12-29 8:50 UTC (permalink / raw) To: Seth House; +Cc: git Am 28.12.20 um 20:29 schrieb Seth House: > diff --git a/git-mergetool.sh b/git-mergetool.sh > index e3c7d78d1d..929192d0f8 100755 > --- a/git-mergetool.sh > +++ b/git-mergetool.sh > @@ -334,6 +334,8 @@ merge_file () { > checkout_staged_file 2 "$MERGED" "$LOCAL" > checkout_staged_file 3 "$MERGED" "$REMOTE" > > + initialize_merge_tool "$merge_tool" In my earlier review, I was not explicit about the lack of error handling of this invocation, because I hoped that you would notice it yourself. `initialize_merge_tool` does have a few failure modes via `setup_tool` that are not really unlikely; ignoring errors would be wrong, I think. Before this change, the errors would be handled as part of the failing `run_merge_tool` call at the end of function `merge_file`. But now errors are ignored. Just appending `|| return` would not be appropriate at this point because a lot has already happened before the call that has to be rewound. Would it be possible to move the call above the `mergetool_tmpdir_init` call, so that nothing has to be rewound? > + > if test "$( > git config --get --bool "mergetool.$merge_tool.automerge" || > git config --get --bool "mergetool.automerge" || > -- Hannes ^ permalink raw reply [flat|nested] 80+ messages in thread
* Re: [PATCH v9 4/5] mergetool: break setup_tool out into separate initialization function 2020-12-29 8:50 ` Johannes Sixt @ 2020-12-29 17:23 ` Seth House 0 siblings, 0 replies; 80+ messages in thread From: Seth House @ 2020-12-29 17:23 UTC (permalink / raw) To: Johannes Sixt; +Cc: git On Tue, Dec 29, 2020 at 09:50:44AM +0100, Johannes Sixt wrote: > Would it be possible to move the call above the > `mergetool_tmpdir_init` call, so that nothing has to be rewound? Ah, I see. Good suggestion. Yes, moving that call higher works just fine. E.g.: diff --git a/git-mergetool.sh b/git-mergetool.sh index a44afd3822..36c1920dd6 100755 --- a/git-mergetool.sh +++ b/git-mergetool.sh @@ -276,6 +276,8 @@ merge_file () { ext= esac + initialize_merge_tool "$merge_tool" || return + mergetool_tmpdir_init if test "$MERGETOOL_TMPDIR" != "." @@ -334,8 +336,6 @@ merge_file () { checkout_staged_file 2 "$MERGED" "$LOCAL" checkout_staged_file 3 "$MERGED" "$REMOTE" - initialize_merge_tool "$merge_tool" - if automerge_enabled && test "$( git config --get --bool "mergetool.$merge_tool.automerge" || git config --get --bool "mergetool.automerge" || Thanks. I'll roll that change into a v10 patch series later today or tomorrow to give a little more time for any other feedback. ^ permalink raw reply related [flat|nested] 80+ messages in thread
* [PATCH v9 5/5] mergetool: add automerge_enabled tool-specific override function 2020-12-28 19:29 ` [PATCH v9 0/5] mergetool: add automerge configuration Seth House ` (3 preceding siblings ...) 2020-12-28 19:29 ` [PATCH v9 4/5] mergetool: break setup_tool out into separate initialization function Seth House @ 2020-12-28 19:29 ` Seth House 2020-12-29 2:01 ` Felipe Contreras 2021-01-06 5:55 ` Junio C Hamano 2021-01-30 5:46 ` [PATCH v10 0/3] mergetool: add hideResolved configuration (was automerge) Seth House 5 siblings, 2 replies; 80+ messages in thread From: Seth House @ 2020-12-28 19:29 UTC (permalink / raw) To: git; +Cc: Seth House, Junio C Hamano The author or maintainer of a mergetool may optionally elect disable (or enable) the `autoMerge` feature for that mergetool even if the user has chosen differently using the `mergetool.autoMerge` and `mergetool.<tool>.autoMerge` options. To add a tool-specific override, edit the `mergetools/<tool>` shell script for that tool and add an `automerge_enabled` function: automerge_enabled () { return 1 } Disabling may be desirable if the mergetool wants or needs access to the original, unmodified 'LOCAL', 'REMOTE', and 'BASE' versions of the conflicted file. For example: - A tool may use a custom conflict resolution algorithm and prefer to ignore the results of Git's conflict resolution. - A tool may want to visually compare/constrast the version of the file from before the merge (saved to 'LOCAL', 'REMOTE', and 'BASE') with Git's conflict resolution results (saved to 'MERGED'). - A student or researcher working on a new algorithm may want to directly compare the result of that algorithm with the result of Git's algorithm. Helped-by: Junio C Hamano <gitster@pobox.com> Signed-off-by: Seth House <seth@eseth.com> --- git-mergetool--lib.sh | 4 ++++ git-mergetool.sh | 2 +- 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh index e059b3559e..567991abbc 100644 --- a/git-mergetool--lib.sh +++ b/git-mergetool--lib.sh @@ -164,6 +164,10 @@ setup_tool () { return 1 } + automerge_enabled () { + return 0 + } + translate_merge_tool_path () { echo "$1" } diff --git a/git-mergetool.sh b/git-mergetool.sh index 929192d0f8..a44afd3822 100755 --- a/git-mergetool.sh +++ b/git-mergetool.sh @@ -336,7 +336,7 @@ merge_file () { initialize_merge_tool "$merge_tool" - if test "$( + if automerge_enabled && test "$( git config --get --bool "mergetool.$merge_tool.automerge" || git config --get --bool "mergetool.automerge" || echo true)" = true -- 2.29.2 ^ permalink raw reply related [flat|nested] 80+ messages in thread
* RE: [PATCH v9 5/5] mergetool: add automerge_enabled tool-specific override function 2020-12-28 19:29 ` [PATCH v9 5/5] mergetool: add automerge_enabled tool-specific override function Seth House @ 2020-12-29 2:01 ` Felipe Contreras 2021-01-06 5:55 ` Junio C Hamano 1 sibling, 0 replies; 80+ messages in thread From: Felipe Contreras @ 2020-12-29 2:01 UTC (permalink / raw) To: Seth House, git; +Cc: Seth House, Junio C Hamano Seth House wrote: > Disabling may be desirable if the mergetool wants or needs access to the > original, unmodified 'LOCAL', 'REMOTE', and 'BASE' versions of the > conflicted file. For example: > > - A tool may use a custom conflict resolution algorithm and prefer to > ignore the results of Git's conflict resolution. If git's conflict resolution decides there are no conflicts, how would such tool "ignore" that? > - A tool may want to visually compare/constrast the version of the file > from before the merge (saved to 'LOCAL', 'REMOTE', and 'BASE') with > Git's conflict resolution results (saved to 'MERGED'). Can't such tool use "git checkout-index" for that? > - A student or researcher working on a new algorithm may want to > directly compare the result of that algorithm with the result of Git's > algorithm. 1. If git's algorithm decides there are no conflicts, and the new algorithm decides there are conflicts, how would such researcher find that out? 2. Can't such researcher simply do: git -c mergetool.automerge=false mergetool? Cheers. -- Felipe Contreras ^ permalink raw reply [flat|nested] 80+ messages in thread
* Re: [PATCH v9 5/5] mergetool: add automerge_enabled tool-specific override function 2020-12-28 19:29 ` [PATCH v9 5/5] mergetool: add automerge_enabled tool-specific override function Seth House 2020-12-29 2:01 ` Felipe Contreras @ 2021-01-06 5:55 ` Junio C Hamano 2021-01-07 3:58 ` Seth House 1 sibling, 1 reply; 80+ messages in thread From: Junio C Hamano @ 2021-01-06 5:55 UTC (permalink / raw) To: Seth House; +Cc: git Seth House <seth@eseth.com> writes: > diff --git a/git-mergetool.sh b/git-mergetool.sh > index 929192d0f8..a44afd3822 100755 > --- a/git-mergetool.sh > +++ b/git-mergetool.sh > @@ -336,7 +336,7 @@ merge_file () { > > initialize_merge_tool "$merge_tool" > > - if test "$( > + if automerge_enabled && test "$( > git config --get --bool "mergetool.$merge_tool.automerge" || > git config --get --bool "mergetool.automerge" || > echo true)" = true This allows the tool author to say "nobody ever is allowed to use my tool with the automerge feature". I know I may have suggested something like that, but I am not sure if we want to be all that draconian. If the user explicitly says "I want the new behaviour enabled for this particular merge tool", we are better off letting the user use it and take responsibility for the possible breakage. My preference would probably be - if "mergetool.$merge_tool.automerge" is set to 'true' or 'false', that's final. - Your automerge_enabled helper that is by default 'true' (but allows individual merge_tool to return 'false') is asked, and if it says 'false', that's final. But 'true' from automerge_enabled is not final at this step. - if "mergetool.automerge" is set to 'true' or 'false', that's final. - otherwise, your automerge_enabled helper's answer (either 'true' or 'false') gives the final answer. That way, those who use a broad "mergetool.automerge = true/false" would still honor what automerge_enabled yields (which is "enabled by default but individual merge_tool can set its default to be disabled"), while individual mergetool.$merge_tool.automerge configuration would always win. Hmm? ^ permalink raw reply [flat|nested] 80+ messages in thread
* Re: [PATCH v9 5/5] mergetool: add automerge_enabled tool-specific override function 2021-01-06 5:55 ` Junio C Hamano @ 2021-01-07 3:58 ` Seth House 2021-01-07 6:38 ` Junio C Hamano 0 siblings, 1 reply; 80+ messages in thread From: Seth House @ 2021-01-07 3:58 UTC (permalink / raw) To: Junio C Hamano; +Cc: git On Tue, Jan 05, 2021 at 09:55:26PM -0800, Junio C Hamano wrote: > If the user explicitly says "I want the new behaviour enabled for > this particular merge tool", we are better off letting the user use > it and take responsibility for the possible breakage. Good suggestion. Agreed on all counts. I'll roll that preference hierarchy into the v10 patch set. ^ permalink raw reply [flat|nested] 80+ messages in thread
* Re: [PATCH v9 5/5] mergetool: add automerge_enabled tool-specific override function 2021-01-07 3:58 ` Seth House @ 2021-01-07 6:38 ` Junio C Hamano 2021-01-07 9:27 ` Seth House 0 siblings, 1 reply; 80+ messages in thread From: Junio C Hamano @ 2021-01-07 6:38 UTC (permalink / raw) To: Seth House; +Cc: git Seth House <seth@eseth.com> writes: > On Tue, Jan 05, 2021 at 09:55:26PM -0800, Junio C Hamano wrote: >> If the user explicitly says "I want the new behaviour enabled for >> this particular merge tool", we are better off letting the user use >> it and take responsibility for the possible breakage. > > Good suggestion. Agreed on all counts. I'll roll that preference > hierarchy into the v10 patch set. By the way, do you have any idea why we see test breakages only on macos when this topic is merged to 'seen'? https://github.com/git/git/runs/1659807735?check_suite_focus=true#step:4:1641 https://github.com/git/git/runs/1659807735?check_suite_focus=true#step:5:2641 Thanks. ^ permalink raw reply [flat|nested] 80+ messages in thread
* Re: [PATCH v9 5/5] mergetool: add automerge_enabled tool-specific override function 2021-01-07 6:38 ` Junio C Hamano @ 2021-01-07 9:27 ` Seth House 2021-01-07 21:26 ` Junio C Hamano 0 siblings, 1 reply; 80+ messages in thread From: Seth House @ 2021-01-07 9:27 UTC (permalink / raw) To: Junio C Hamano; +Cc: git On Wed, Jan 06, 2021 at 10:38:09PM -0800, Junio C Hamano wrote: > By the way, do you have any idea why we see test breakages only on > macos when this topic is merged to 'seen'? Thanks for those links. I have an OSX machine nearby and will investigate tomorrow. Related: are the Windows tests affected by this patch? I wanted to check for myself but I've been struggling with getting Git-for-Windows installed in a VM. ^ permalink raw reply [flat|nested] 80+ messages in thread
* Re: [PATCH v9 5/5] mergetool: add automerge_enabled tool-specific override function 2021-01-07 9:27 ` Seth House @ 2021-01-07 21:26 ` Junio C Hamano 2021-01-08 15:04 ` Johannes Schindelin 0 siblings, 1 reply; 80+ messages in thread From: Junio C Hamano @ 2021-01-07 21:26 UTC (permalink / raw) To: Seth House; +Cc: git, Johannes Schindelin Seth House <seth@eseth.com> writes: > On Wed, Jan 06, 2021 at 10:38:09PM -0800, Junio C Hamano wrote: >> By the way, do you have any idea why we see test breakages only on >> macos when this topic is merged to 'seen'? > > Thanks for those links. I have an OSX machine nearby and will > investigate tomorrow. > > Related: are the Windows tests affected by this patch? I wanted to check > for myself but I've been struggling with getting Git-for-Windows > installed in a VM. On the left hand side of the page I gave the links to, it shows that 'windows-build' job is failing (and windows-test jobs are not run as a consequence). I am not sure why it failed, but I have a feeling that the build machinery hasn't even seen the code being built when it errored out. cf. https://github.com/git/git/runs/1659807855?check_suite_focus=true#step:3:40 So we cannot tell (yet). ^ permalink raw reply [flat|nested] 80+ messages in thread
* Re: [PATCH v9 5/5] mergetool: add automerge_enabled tool-specific override function 2021-01-07 21:26 ` Junio C Hamano @ 2021-01-08 15:04 ` Johannes Schindelin 0 siblings, 0 replies; 80+ messages in thread From: Johannes Schindelin @ 2021-01-08 15:04 UTC (permalink / raw) To: Junio C Hamano; +Cc: Seth House, git Hi Junio, On Thu, 7 Jan 2021, Junio C Hamano wrote: > Seth House <seth@eseth.com> writes: > > > On Wed, Jan 06, 2021 at 10:38:09PM -0800, Junio C Hamano wrote: > >> By the way, do you have any idea why we see test breakages only on > >> macos when this topic is merged to 'seen'? > > > > Thanks for those links. I have an OSX machine nearby and will > > investigate tomorrow. > > > > Related: are the Windows tests affected by this patch? I wanted to check > > for myself but I've been struggling with getting Git-for-Windows > > installed in a VM. > > On the left hand side of the page I gave the links to, it shows that > 'windows-build' job is failing (and windows-test jobs are not run as > a consequence). I am not sure why it failed, but I have a feeling > that the build machinery hasn't even seen the code being built when > it errored out. > > cf. https://github.com/git/git/runs/1659807855?check_suite_focus=true#step:3:40 > > So we cannot tell (yet). There are unfortunately intermittent failures while downloading git-sdk-64-minimal; That's what this job is seeing. I restarted that build: https://github.com/git/git/runs/1659807855?check_suite_focus=true#step:3:40 Ciao, Dscho ^ permalink raw reply [flat|nested] 80+ messages in thread
* [PATCH v10 0/3] mergetool: add hideResolved configuration (was automerge) 2020-12-28 19:29 ` [PATCH v9 0/5] mergetool: add automerge configuration Seth House ` (4 preceding siblings ...) 2020-12-28 19:29 ` [PATCH v9 5/5] mergetool: add automerge_enabled tool-specific override function Seth House @ 2021-01-30 5:46 ` Seth House 2021-01-30 5:46 ` [PATCH v10 1/3] mergetool: add hideResolved configuration Seth House ` (3 more replies) 5 siblings, 4 replies; 80+ messages in thread From: Seth House @ 2021-01-30 5:46 UTC (permalink / raw) To: git Cc: Seth House, Junio C Hamano, David Aguilar, Johannes Sixt, Felipe Contreras Changes since v9: - Rename automerge to hideResolved. Several mergetools have a feature that they call "automerge", "auto merge", or "auto solve" and Git has a `mergetool.meld.useAutoMerge` flag so I think it's better to avoid potential confusion. Plus Git already performed the merge and this doesn't do any additional merging, instead it just "hides" conflicts that Git already resolved. - Reworked and consolidated commits and commit messages. - Add preference hierarchy. Followed Junio's suggestions: https://lore.kernel.org/git/xmqqpn2ivcc1.fsf@gitster.c.googlers.com/ - Switch from sed to two merge-file calls. Thanks to everyone who helped with all the suggestions and fixup!s to get sed working cross-platform. Unfortunately there's not a great, portable method to preserve carriage returns when using both autocrlf and MSYS2: https://lore.kernel.org/git/20210120232447.GA35105@ellen/ Although calling merge-file twice is (much) less efficient than sed it's still fairly quick for small files. For large files it's likely opening those files in a mergetool will have a higher overhead than the merge-file invocations: https://lore.kernel.org/git/20210122010902.GA48178@ellen/ A potential future optimisation could be to augment the C implementation (xmerge.c ?) with a flag to write two files as the merge is being performed instead of writing conflict markers. - Kept `initialize_merge_tool` wrapper. I updated the commit message where `initialize_merge_tool` is introduced to try and better explain my thinking for not simply exposing `setup_tool` instead. I'm happy to switch that if anyone still feels it should be switched. Seth House (3): mergetool: add hideResolved configuration mergetool: break setup_tool out into separate initialization function mergetool: add per-tool support and overrides for the hideResolved flag Documentation/config/mergetool.txt | 15 +++++++++++++++ Documentation/git-mergetool--lib.txt | 4 ++++ git-difftool--helper.sh | 6 ++++++ git-mergetool--lib.sh | 11 ++++++++--- git-mergetool.sh | 26 ++++++++++++++++++++++++++ t/t7610-mergetool.sh | 18 ++++++++++++++++++ 6 files changed, 77 insertions(+), 3 deletions(-) -- 2.29.2 ^ permalink raw reply [flat|nested] 80+ messages in thread
* [PATCH v10 1/3] mergetool: add hideResolved configuration 2021-01-30 5:46 ` [PATCH v10 0/3] mergetool: add hideResolved configuration (was automerge) Seth House @ 2021-01-30 5:46 ` Seth House 2021-01-30 8:08 ` Junio C Hamano 2021-01-30 5:46 ` [PATCH v10 2/3] mergetool: break setup_tool out into separate initialization function Seth House ` (2 subsequent siblings) 3 siblings, 1 reply; 80+ messages in thread From: Seth House @ 2021-01-30 5:46 UTC (permalink / raw) To: git; +Cc: Seth House, Felipe Contreras The purpose of a mergetool is to help the user resolve any conflicts that Git cannot automatically resolve. If there is a conflict that must be resolved manually Git will write a file named MERGED which contains everything Git was able to resolve by itself and also everything that it was not able to resolve wrapped in conflict markers. One way to think of MERGED is as a two- or three-way diff. If each "side" of the conflict markers is separately extracted an external tool can represent those conflicts as a side-by-side diff. However many mergetools instead diff LOCAL and REMOTE both of which contain versions of the file from before the merge. Since the conflicts Git resolved automatically are not present it forces the user to manually re-resolve those conflicts. Some mergetools also show MERGED but often only for reference and not as the focal point to resolve the conflicts. This adds a `mergetool.hideResolved` flag that will overwrite LOCAL and REMOTE with each corresponding "side" of a conflicted file and thus hide all conflicts that Git was able to resolve itself. Overwriting these files will immediately benefit any mergetool that uses them without requiring any changes. No adverse effects were noted in a small survey of popular mergetools[1] so this behavior defaults to `true`. However it can be globally disabled by setting `mergetool.hideResolved` to `false`. [1] https://www.eseth.org/2020/mergetools.html https://github.com/whiteinge/eseth/blob/c884424769fffb05d87afb33b2cf80cecb4044c3/2020/mergetools.md Original-implementation-by: Felipe Contreras <felipe.contreras@gmail.com> Helped-by: Felipe Contreras <felipe.contreras@gmail.com> Signed-off-by: Seth House <seth@eseth.com> --- Documentation/config/mergetool.txt | 9 +++++++++ git-mergetool.sh | 14 ++++++++++++++ t/t7610-mergetool.sh | 18 ++++++++++++++++++ 3 files changed, 41 insertions(+) diff --git a/Documentation/config/mergetool.txt b/Documentation/config/mergetool.txt index 16a27443a3..3171bacf91 100644 --- a/Documentation/config/mergetool.txt +++ b/Documentation/config/mergetool.txt @@ -40,6 +40,15 @@ mergetool.meld.useAutoMerge:: value of `false` avoids using `--auto-merge` altogether, and is the default value. +mergetool.hideResolved:: + During a merge Git will automatically resolve as many conflicts as + possible and then wrap conflict markers around any conflicts that it + cannot resolve. This flag writes the non-conflicting parts into the + corresponding 'LOCAL' and 'REMOTE' files so that only the unresolved + conflicts are presented to the merge tool. Can be overriden per-tool + via the `mergetool.<tool>.hideResolved` configuration variable. + Defaults to `true`. + mergetool.keepBackup:: After performing a merge, the original file with conflict markers can be saved as a file with a `.orig` extension. If this variable diff --git a/git-mergetool.sh b/git-mergetool.sh index e3f6d543fb..5b0d15ed89 100755 --- a/git-mergetool.sh +++ b/git-mergetool.sh @@ -239,6 +239,13 @@ checkout_staged_file () { fi } +hide_resolved () { + git merge-file --ours -q -p "$LOCAL" "$BASE" "$REMOTE" >"$LCONFL" + git merge-file --theirs -q -p "$LOCAL" "$BASE" "$REMOTE" >"$RCONFL" + mv -- "$LCONFL" "$LOCAL" + mv -- "$RCONFL" "$REMOTE" +} + merge_file () { MERGED="$1" @@ -276,7 +283,9 @@ merge_file () { BACKUP="$MERGETOOL_TMPDIR/${BASE}_BACKUP_$$$ext" LOCAL="$MERGETOOL_TMPDIR/${BASE}_LOCAL_$$$ext" + LCONFL="$MERGETOOL_TMPDIR/${BASE}_LOCAL_LCONFL_$$$ext" REMOTE="$MERGETOOL_TMPDIR/${BASE}_REMOTE_$$$ext" + RCONFL="$MERGETOOL_TMPDIR/${BASE}_REMOTE_RCONFL_$$$ext" BASE="$MERGETOOL_TMPDIR/${BASE}_BASE_$$$ext" base_mode= local_mode= remote_mode= @@ -322,6 +331,11 @@ merge_file () { checkout_staged_file 2 "$MERGED" "$LOCAL" checkout_staged_file 3 "$MERGED" "$REMOTE" + if test "$(git config --get mergetool.hideResolved)" != "false" + then + hide_resolved + 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..0e34b87e37 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 hideResolved' ' + test_config mergetool.hideResolved 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 -- 2.29.2 ^ permalink raw reply related [flat|nested] 80+ messages in thread
* Re: [PATCH v10 1/3] mergetool: add hideResolved configuration 2021-01-30 5:46 ` [PATCH v10 1/3] mergetool: add hideResolved configuration Seth House @ 2021-01-30 8:08 ` Junio C Hamano 0 siblings, 0 replies; 80+ messages in thread From: Junio C Hamano @ 2021-01-30 8:08 UTC (permalink / raw) To: Seth House; +Cc: git, Felipe Contreras Seth House <seth@eseth.com> writes: > +mergetool.hideResolved:: > + During a merge Git will automatically resolve as many conflicts as > + possible and then wrap conflict markers around any conflicts that it > + cannot resolve. This flag writes the non-conflicting parts into the > + corresponding 'LOCAL' and 'REMOTE' files so that only the unresolved > + conflicts are presented to the merge tool. Can be overriden per-tool > + via the `mergetool.<tool>.hideResolved` configuration variable. > + Defaults to `true`. This description makes the readers expect that the configuration variable is a boolean, and setting it to 'no' would disable the feature, but ... > @@ -322,6 +331,11 @@ merge_file () { > checkout_staged_file 2 "$MERGED" "$LOCAL" > checkout_staged_file 3 "$MERGED" "$REMOTE" > > + if test "$(git config --get mergetool.hideResolved)" != "false" ... without --type=bool, any boolean 'false' value that is not exactly spelled 'false' won't be normalized and fail this test. I haven't read the remaining 2 patches, so I cannot yet tell if I can just insert "--type=bool" here and everything would be fine, or if there are other fallouts for doing so. > + then > + hide_resolved > + 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..0e34b87e37 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 hideResolved' ' > + test_config mergetool.hideResolved true && > + test_when_finished "git reset --hard" && > + git checkout -b test${test_count}_b master && As a new feature, this should work with the tip of 'master', but I think the t7610 test forces the initial branch name to be 'main'. I'll tweak locally while queuing. > + 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 ^ permalink raw reply [flat|nested] 80+ messages in thread
* [PATCH v10 2/3] mergetool: break setup_tool out into separate initialization function 2021-01-30 5:46 ` [PATCH v10 0/3] mergetool: add hideResolved configuration (was automerge) Seth House 2021-01-30 5:46 ` [PATCH v10 1/3] mergetool: add hideResolved configuration Seth House @ 2021-01-30 5:46 ` Seth House 2021-01-30 5:46 ` [PATCH v10 3/3] mergetool: add per-tool support and overrides for the hideResolved flag Seth House 2021-02-09 20:07 ` [PATCH v11 0/3] mergetool: add hideResolved configuration (was automerge) Seth House 3 siblings, 0 replies; 80+ messages in thread From: Seth House @ 2021-01-30 5:46 UTC (permalink / raw) To: git; +Cc: Seth House This is preparation for the following commit where we need to source the mergetool shell script to look for overrides before `run_merge_tool` is called. Previously `run_merge_tool` both sourced that script and invoked the mergetool. In the case of the following commit, we need the result of the `hide_resolved` override, if present, before we actually run `run_merge_tool`. The new `initialize_merge_tool` wrapper is exposed and documented as a public interface for consistency with the existing `run_merge_tool` which is also public. Although `setup_tool` could instead be exposed directly, the related `setup_user_tool` would probably also want to be elevated to match and this felt the cleanest to me. Signed-off-by: Seth House <seth@eseth.com> --- Documentation/git-mergetool--lib.txt | 4 ++++ git-difftool--helper.sh | 6 ++++++ git-mergetool--lib.sh | 7 ++++--- git-mergetool.sh | 2 ++ 4 files changed, 16 insertions(+), 3 deletions(-) diff --git a/Documentation/git-mergetool--lib.txt b/Documentation/git-mergetool--lib.txt index 4da9d24096..3e8f59ac0e 100644 --- a/Documentation/git-mergetool--lib.txt +++ b/Documentation/git-mergetool--lib.txt @@ -38,6 +38,10 @@ get_merge_tool_cmd:: get_merge_tool_path:: returns the custom path for a merge tool. +initialize_merge_tool:: + bring merge tool specific functions into scope so they can be used or + overridden. + run_merge_tool:: launches a merge tool given the tool name and a true/false flag to indicate whether a merge base is present. diff --git a/git-difftool--helper.sh b/git-difftool--helper.sh index 46af3e60b7..992124cc67 100755 --- a/git-difftool--helper.sh +++ b/git-difftool--helper.sh @@ -61,6 +61,9 @@ launch_merge_tool () { export BASE eval $GIT_DIFFTOOL_EXTCMD '"$LOCAL"' '"$REMOTE"' else + initialize_merge_tool "$merge_tool" + # ignore the error from the above --- run_merge_tool + # will diagnose unusable tool by itself run_merge_tool "$merge_tool" fi } @@ -79,6 +82,9 @@ if test -n "$GIT_DIFFTOOL_DIRDIFF" then LOCAL="$1" REMOTE="$2" + initialize_merge_tool "$merge_tool" + # ignore the error from the above --- run_merge_tool + # will diagnose unusable tool by itself run_merge_tool "$merge_tool" false else # Launch the merge tool on each path provided by 'git diff' diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh index 7225abd811..e059b3559e 100644 --- a/git-mergetool--lib.sh +++ b/git-mergetool--lib.sh @@ -248,6 +248,10 @@ trust_exit_code () { fi } +initialize_merge_tool () { + # Bring tool-specific functions into scope + setup_tool "$1" || return 1 +} # Entry point for running tools run_merge_tool () { @@ -259,9 +263,6 @@ run_merge_tool () { merge_tool_path=$(get_merge_tool_path "$1") || exit base_present="$2" - # Bring tool-specific functions into scope - setup_tool "$1" || return 1 - if merge_mode then run_merge_cmd "$1" diff --git a/git-mergetool.sh b/git-mergetool.sh index 5b0d15ed89..865f12551a 100755 --- a/git-mergetool.sh +++ b/git-mergetool.sh @@ -272,6 +272,8 @@ merge_file () { ext= esac + initialize_merge_tool "$merge_tool" || return + mergetool_tmpdir_init if test "$MERGETOOL_TMPDIR" != "." -- 2.29.2 ^ permalink raw reply related [flat|nested] 80+ messages in thread
* [PATCH v10 3/3] mergetool: add per-tool support and overrides for the hideResolved flag 2021-01-30 5:46 ` [PATCH v10 0/3] mergetool: add hideResolved configuration (was automerge) Seth House 2021-01-30 5:46 ` [PATCH v10 1/3] mergetool: add hideResolved configuration Seth House 2021-01-30 5:46 ` [PATCH v10 2/3] mergetool: break setup_tool out into separate initialization function Seth House @ 2021-01-30 5:46 ` Seth House 2021-01-30 8:08 ` Junio C Hamano 2021-02-09 20:07 ` [PATCH v11 0/3] mergetool: add hideResolved configuration (was automerge) Seth House 3 siblings, 1 reply; 80+ messages in thread From: Seth House @ 2021-01-30 5:46 UTC (permalink / raw) To: git; +Cc: Seth House, Johannes Sixt, Junio C Hamano Keep the global mergetool flag and add a per-tool override flag so that users may enable the flag for one tool and disable it for another. In addition, the author or maintainer of a mergetool may optionally elect to set the default `hideResolved` value for that mergetool. To disable the feature for a specific tool, edit the `mergetools/<tool>` shell script for that tool and add a `hide_resolved_enabled` function: hide_resolved_enabled () { return 1 } Disabling may be desirable if the mergetool wants or needs access to the original, unmodified 'LOCAL' and 'REMOTE' versions of the conflicted file. For example: - A tool may use a custom conflict resolution algorithm and prefer to ignore the results of Git's conflict resolution. - A tool may want to visually compare/constrast the version of the file from before the merge (saved to 'LOCAL', 'REMOTE', and 'BASE') with Git's conflict resolution results (saved to 'MERGED'). Helped-by: Johannes Sixt <j6t@kdbg.org> Helped-by: Junio C Hamano <gitster@pobox.com> Signed-off-by: Seth House <seth@eseth.com> --- Documentation/config/mergetool.txt | 6 ++++++ git-mergetool--lib.sh | 4 ++++ git-mergetool.sh | 14 ++++++++++++-- 3 files changed, 22 insertions(+), 2 deletions(-) diff --git a/Documentation/config/mergetool.txt b/Documentation/config/mergetool.txt index 3171bacf91..046816fb07 100644 --- a/Documentation/config/mergetool.txt +++ b/Documentation/config/mergetool.txt @@ -13,6 +13,12 @@ mergetool.<tool>.cmd:: merged; 'MERGED' contains the name of the file to which the merge tool should write the results of a successful merge. +mergetool.<tool>.hideResolved:: + A mergetool-specific override for the global `mergetool.hideResolved` + configuration flag. This allows individual mergetools to enable or + disable the flag regardless of the global setting. See + `mergetool.hideResolved` for the full description. + mergetool.<tool>.trustExitCode:: For a custom merge command, specify whether the exit code of the merge command can be used to determine whether the merge was diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh index e059b3559e..11f00dde41 100644 --- a/git-mergetool--lib.sh +++ b/git-mergetool--lib.sh @@ -164,6 +164,10 @@ setup_tool () { return 1 } + hide_resolved_enabled () { + return 0 + } + translate_merge_tool_path () { echo "$1" } diff --git a/git-mergetool.sh b/git-mergetool.sh index 865f12551a..6cf3884277 100755 --- a/git-mergetool.sh +++ b/git-mergetool.sh @@ -333,9 +333,19 @@ merge_file () { checkout_staged_file 2 "$MERGED" "$LOCAL" checkout_staged_file 3 "$MERGED" "$REMOTE" - if test "$(git config --get mergetool.hideResolved)" != "false" + # hideResolved preferences hierarchy: + # First respect user's tool-specific configuration if exists. + if test "$(git config --get "mergetool.$merge_tool.hideResolved")" != "false" then - hide_resolved + # Next respect tool-specified configuration. + if hide_resolved_enabled + then + # Finally respect if user has a global disable. + if test "$(git config --get "mergetool.hideResolved")" != "false" + then + hide_resolved + fi + fi fi if test -z "$local_mode" || test -z "$remote_mode" -- 2.29.2 ^ permalink raw reply related [flat|nested] 80+ messages in thread
* Re: [PATCH v10 3/3] mergetool: add per-tool support and overrides for the hideResolved flag 2021-01-30 5:46 ` [PATCH v10 3/3] mergetool: add per-tool support and overrides for the hideResolved flag Seth House @ 2021-01-30 8:08 ` Junio C Hamano 0 siblings, 0 replies; 80+ messages in thread From: Junio C Hamano @ 2021-01-30 8:08 UTC (permalink / raw) To: Seth House; +Cc: git, Johannes Sixt Seth House <seth@eseth.com> writes: > Keep the global mergetool flag and add a per-tool override flag so that > users may enable the flag for one tool and disable it for another. In > addition, the author or maintainer of a mergetool may optionally elect > to set the default `hideResolved` value for that mergetool. OK. > To disable the feature for a specific tool, edit the `mergetools/<tool>` > shell script for that tool and add a `hide_resolved_enabled` function: > > hide_resolved_enabled () { > return 1 > } > > Disabling may be desirable if the mergetool wants or needs access to the > original, unmodified 'LOCAL' and 'REMOTE' versions of the conflicted > file. The above sounds as if it is a hint/help for end users, but it is unreasonable to expect all end users of a particular <tool> to edit part of their Git installation. I suspect that you didn't mean it that way, and instead it is meant to advise (new) tool authors who will add mergetools/<tool> for their own tool, and when read with that in mind, it does make sort-of sense (except that when you are author of this thing, you won't "edit" as if you are modifying something that already exists---you'd be the one who is adding the <tool> under mergetools/ directory). For an end-user, to disable the feature for a tool, you'd just configure mergetool.<tool>.hideResolved to 'false', right? > For example: > > - A tool may use a custom conflict resolution algorithm and prefer to > ignore the results of Git's conflict resolution. > - A tool may want to visually compare/constrast the version of the file > from before the merge (saved to 'LOCAL', 'REMOTE', and 'BASE') with > Git's conflict resolution results (saved to 'MERGED'). > > Helped-by: Johannes Sixt <j6t@kdbg.org> > Helped-by: Junio C Hamano <gitster@pobox.com> > Signed-off-by: Seth House <seth@eseth.com> > --- > Documentation/config/mergetool.txt | 6 ++++++ > git-mergetool--lib.sh | 4 ++++ > git-mergetool.sh | 14 ++++++++++++-- > 3 files changed, 22 insertions(+), 2 deletions(-) > > diff --git a/Documentation/config/mergetool.txt b/Documentation/config/mergetool.txt > index 3171bacf91..046816fb07 100644 > --- a/Documentation/config/mergetool.txt > +++ b/Documentation/config/mergetool.txt > @@ -13,6 +13,12 @@ mergetool.<tool>.cmd:: > merged; 'MERGED' contains the name of the file to which the merge > tool should write the results of a successful merge. > > +mergetool.<tool>.hideResolved:: > + A mergetool-specific override for the global `mergetool.hideResolved` > + configuration flag. This allows individual mergetools to enable or > + disable the flag regardless of the global setting. See > + `mergetool.hideResolved` for the full description. This description is iffy. The configuration allows "users" to enable or disable the feature for individual mergetools, overriding the 'mergetool.hideResolved' global setting, no? The above paragraph reads as if the tool author is enabling/disabling it. > diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh > index e059b3559e..11f00dde41 100644 > --- a/git-mergetool--lib.sh > +++ b/git-mergetool--lib.sh > @@ -164,6 +164,10 @@ setup_tool () { > return 1 > } > > + hide_resolved_enabled () { > + return 0 > + } > + > translate_merge_tool_path () { > echo "$1" > } > diff --git a/git-mergetool.sh b/git-mergetool.sh > index 865f12551a..6cf3884277 100755 > --- a/git-mergetool.sh > +++ b/git-mergetool.sh > @@ -333,9 +333,19 @@ merge_file () { > checkout_staged_file 2 "$MERGED" "$LOCAL" > checkout_staged_file 3 "$MERGED" "$REMOTE" > > - if test "$(git config --get mergetool.hideResolved)" != "false" > + # hideResolved preferences hierarchy: > + # First respect user's tool-specific configuration if exists. > + if test "$(git config --get "mergetool.$merge_tool.hideResolved")" != "false" The same "--type=bool" comment applies to this step, too. > then > + # Next respect tool-specified configuration. > + if hide_resolved_enabled > + then > + # Finally respect if user has a global disable. > + if test "$(git config --get "mergetool.hideResolved")" != "false" > + then > + hide_resolved > + fi > + fi > fi I am not sure if I understand this logic. If the user says "for the tool <tool>, set hideresolved to true/false" explicitly, I think it should be final. Even if the tool's author expresses that s/he prefers not to have to work on a pre-munged input by setting hide_resolved_enabled to false, if the end-user says s/he wants to use it on that tool, we do not want to help the tool to override the user's wish. If the user says "use hideresolved feature, as I like it in general" by setting mergetool.hideResolved, on the other hand, it may be also reasonable to heed "no, I recommend against it for this tool" for individual tool whose hide_resolved_enabled returns false. And if the global is set to 'false', the user says "I do not want it", and it may be iffy to let individual tool to countermand it. WHen dealing with either of these variables, therefore, you'd need to know if the variable is not set at all, or if the variable is set to true, or to false. Even if we default to enabled, we need to be able to tell if the user didn't say anything (and we enabled the feature for the user because of our default choice), or if the user explicitly said s/he wants it. In other words, you'd need to treat mergetool.hideResolved and mergetool.$merge_tool.hideResolved as tristates. Here is how "git config --type=bool" can be used to normalize various ways to spell true/false and tell between "not set" and "set to some value": $ git -c a.b config --type=bool a.b; echo $? true 0 $ git -c a.b=yes config --type=bool a.b; echo $? true 0 $ git -c a.b=0 config --type=bool a.b; echo $? false 0 $ git config --type=bool a.b; echo $? 1 IOW, if "git config --type=bool" fails, the user does not have the variable set. If it succeeds, you'd get normalized 'true/false' string on its standard output. Using that technique, here is my attempt to rewrite the above logic, with commentary. global_config=mergetool.hideResolved tool_config=mergetool.$merge_tool.hideResolved if enabled=$(git config --type=bool "$tool_config") then # The user explicitly says true or false, so there # is no point in asking any other source of preferences ; elif enabled=$(git config --type=bool "$global_config") then # There is a blanket preference for all tools, and 'true' # means "I like the hide-resolved in general, so use it # when appropriate" by the user. We can let the tool # author to override and disable, though. # # On the other hand, when set to 'false', it is "I really # don't like the feature in general, so do not use it # anywhere", which we take it as final, without letting # the tool override it. if test "$enabled" = true && hide_resolved_enabled then enabled=true else enabled=false fi else # The user does not have preference. Ask the tool if hide_resolved_enabled then enabled=true else enabled=false fi fi # Now we know if the feature should be used. if test "$enabled" = true then hide_resolved fi ^ permalink raw reply [flat|nested] 80+ messages in thread
* [PATCH v11 0/3] mergetool: add hideResolved configuration (was automerge) 2021-01-30 5:46 ` [PATCH v10 0/3] mergetool: add hideResolved configuration (was automerge) Seth House ` (2 preceding siblings ...) 2021-01-30 5:46 ` [PATCH v10 3/3] mergetool: add per-tool support and overrides for the hideResolved flag Seth House @ 2021-02-09 20:07 ` Seth House 2021-02-09 20:07 ` [PATCH v11 1/3] mergetool: add hideResolved configuration Seth House ` (3 more replies) 3 siblings, 4 replies; 80+ messages in thread From: Seth House @ 2021-02-09 20:07 UTC (permalink / raw) To: git Cc: Seth House, Junio C Hamano, David Aguilar, Johannes Sixt, Felipe Contreras Changes since v10: - Update the calls to `git config` to return normalized strings. Junio, thank you for explaining the existence/omission and normalized strings tristate. I missed that in the docs and that's perfect. - Adopt Junio's replacement preference hierarchy conditionals to respect opt-ins and not just opt-outs. Your suggested code worked out-of-box in all the scenarios I could think to test. \o/ - Tweak the mergetool.hideResolved docs to call out the role of LOCAL and REMOTE. - Reword commit messages and docs to better differentiate between config flags users set and code that merge tool maintainers write. Seth House (3): mergetool: add hideResolved configuration mergetool: break setup_tool out into separate initialization function mergetool: add per-tool support and overrides for the hideResolved flag Documentation/config/mergetool.txt | 14 ++++++++ Documentation/git-mergetool--lib.txt | 4 +++ git-difftool--helper.sh | 6 ++++ git-mergetool--lib.sh | 11 ++++-- git-mergetool.sh | 52 ++++++++++++++++++++++++++++ t/t7610-mergetool.sh | 18 ++++++++++ 6 files changed, 102 insertions(+), 3 deletions(-) -- 2.29.2 ^ permalink raw reply [flat|nested] 80+ messages in thread
* [PATCH v11 1/3] mergetool: add hideResolved configuration 2021-02-09 20:07 ` [PATCH v11 0/3] mergetool: add hideResolved configuration (was automerge) Seth House @ 2021-02-09 20:07 ` Seth House 2021-03-09 2:29 ` [PATCH] mergetool: do not enable hideResolved by default Jonathan Nieder 2021-02-09 20:07 ` [PATCH v11 2/3] mergetool: break setup_tool out into separate initialization function Seth House ` (2 subsequent siblings) 3 siblings, 1 reply; 80+ messages in thread From: Seth House @ 2021-02-09 20:07 UTC (permalink / raw) To: git; +Cc: Seth House, Felipe Contreras The purpose of a mergetool is to help the user resolve any conflicts that Git cannot automatically resolve. If there is a conflict that must be resolved manually Git will write a file named MERGED which contains everything Git was able to resolve by itself and also everything that it was not able to resolve wrapped in conflict markers. One way to think of MERGED is as a two- or three-way diff. If each "side" of the conflict markers is separately extracted an external tool can represent those conflicts as a side-by-side diff. However many mergetools instead diff LOCAL and REMOTE both of which contain versions of the file from before the merge. Since the conflicts Git resolved automatically are not present it forces the user to manually re-resolve those conflicts. Some mergetools also show MERGED but often only for reference and not as the focal point to resolve the conflicts. This adds a `mergetool.hideResolved` flag that will overwrite LOCAL and REMOTE with each corresponding "side" of a conflicted file and thus hide all conflicts that Git was able to resolve itself. Overwriting these files will immediately benefit any mergetool that uses them without requiring any changes to the tool. No adverse effects were noted in a small survey of popular mergetools[1] so this behavior defaults to `true`. However it can be globally disabled by setting `mergetool.hideResolved` to `false`. [1] https://www.eseth.org/2020/mergetools.html https://github.com/whiteinge/eseth/blob/c884424769fffb05d87afb33b2cf80cecb4044c3/2020/mergetools.md Original-implementation-by: Felipe Contreras <felipe.contreras@gmail.com> Signed-off-by: Seth House <seth@eseth.com> --- Documentation/config/mergetool.txt | 10 ++++++++++ git-mergetool.sh | 14 ++++++++++++++ t/t7610-mergetool.sh | 18 ++++++++++++++++++ 3 files changed, 42 insertions(+) diff --git a/Documentation/config/mergetool.txt b/Documentation/config/mergetool.txt index 16a27443a3..b858191970 100644 --- a/Documentation/config/mergetool.txt +++ b/Documentation/config/mergetool.txt @@ -40,6 +40,16 @@ mergetool.meld.useAutoMerge:: value of `false` avoids using `--auto-merge` altogether, and is the default value. +mergetool.hideResolved:: + During a merge Git will automatically resolve as many conflicts as + possible and write the 'MERGED' file containing conflict markers around + any conflicts that it cannot resolve; 'LOCAL' and 'REMOTE' normally + represent the versions of the file from before Git's conflict + resolution. This flag causes 'LOCAL' and 'REMOTE' to be overwriten so + that only the unresolved conflicts are presented to the merge tool. Can + be configured per-tool via the `mergetool.<tool>.hideResolved` + configuration variable. Defaults to `true`. + mergetool.keepBackup:: After performing a merge, the original file with conflict markers can be saved as a file with a `.orig` extension. If this variable diff --git a/git-mergetool.sh b/git-mergetool.sh index e3f6d543fb..40a103443d 100755 --- a/git-mergetool.sh +++ b/git-mergetool.sh @@ -239,6 +239,13 @@ checkout_staged_file () { fi } +hide_resolved () { + git merge-file --ours -q -p "$LOCAL" "$BASE" "$REMOTE" >"$LCONFL" + git merge-file --theirs -q -p "$LOCAL" "$BASE" "$REMOTE" >"$RCONFL" + mv -- "$LCONFL" "$LOCAL" + mv -- "$RCONFL" "$REMOTE" +} + merge_file () { MERGED="$1" @@ -276,7 +283,9 @@ merge_file () { BACKUP="$MERGETOOL_TMPDIR/${BASE}_BACKUP_$$$ext" LOCAL="$MERGETOOL_TMPDIR/${BASE}_LOCAL_$$$ext" + LCONFL="$MERGETOOL_TMPDIR/${BASE}_LOCAL_LCONFL_$$$ext" REMOTE="$MERGETOOL_TMPDIR/${BASE}_REMOTE_$$$ext" + RCONFL="$MERGETOOL_TMPDIR/${BASE}_REMOTE_RCONFL_$$$ext" BASE="$MERGETOOL_TMPDIR/${BASE}_BASE_$$$ext" base_mode= local_mode= remote_mode= @@ -322,6 +331,11 @@ merge_file () { checkout_staged_file 2 "$MERGED" "$LOCAL" checkout_staged_file 3 "$MERGED" "$REMOTE" + if test "$(git config --type=bool mergetool.hideResolved)" != "false" + then + hide_resolved + 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 04b0095072..cec4a860ef 100755 --- a/t/t7610-mergetool.sh +++ b/t/t7610-mergetool.sh @@ -842,4 +842,22 @@ test_expect_success 'mergetool --tool-help shows recognized tools' ' grep meld mergetools ' +test_expect_success 'mergetool hideResolved' ' + test_config mergetool.hideResolved 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 -- 2.29.2 ^ permalink raw reply related [flat|nested] 80+ messages in thread
* [PATCH] mergetool: do not enable hideResolved by default 2021-02-09 20:07 ` [PATCH v11 1/3] mergetool: add hideResolved configuration Seth House @ 2021-03-09 2:29 ` Jonathan Nieder 2021-03-09 5:42 ` Seth House 2021-03-10 8:06 ` Junio C Hamano 0 siblings, 2 replies; 80+ messages in thread From: Jonathan Nieder @ 2021-03-09 2:29 UTC (permalink / raw) To: Junio C Hamano; +Cc: git, Felipe Contreras, Seth House, Dana Dahlstrom A typical mergetool uses four panes, showing the content of the file being resolved from MERGE_BASE ('BASE'), HEAD ('LOCAL'), MERGE_HEAD ('REMOTE'), and the working copy. This allows understanding the conflicts in context: by seeing the entire content of the file from MERGE_HEAD, say, we can see the full intent of the code we are pulling in and understand what they were trying to do that conflicted with our own changes. Sometimes, though, the exact content of these three competing versions of a file is not so important. Especially if the mergetool supports folding unchanged lines, the new 'mergetool.hideResolved' feature can be helpful for allowing a person resolving a merge to focus on the portion with conflicts. For sections of the file where BASE matched LOCAL or REMOTE, this feature makes all three versions match the resolved version, so that the user resolving can focus exclusively on the portions with conflicts. In other words, hideResolved makes a multi-pane merge tool show a similar amount of information to the file with conflict markers with conflictstyle=diff3, saving the operator from having to pay attention to parts that resolved cleanly. 98ea309b3f (mergetool: add hideResolved configuration, 2021-02-09) which introduced this setting enabled it by default, explaining: No adverse effects were noted in a small survey of popular mergetools[1] so this behavior defaults to `true`. However it can be globally disabled by setting `mergetool.hideResolved` to `false`. In practice, however, this has proved confusing for users. No indication is shown in the UI that the base, local, and remote versions shown have been modified by additional resolution. Especially in cases where conflicts involve elements beyond textual conflict, it has resulted in incorrect resolutions and wasted work to figure out what happened. Flip the default back to the traditional behavior of `false`: although the old behavior involves slightly slower merges in the only-textual-conflicts case, it prevents this kind of painful moment of betrayal by one's tools, which is more important. Should we want to migrate to hideResolved=true in the future, we still can. It just requires a more careful migration, including a period where "git mergetool" shows a warning or errors out in affected cases. Reported-by: Dana Dahlstrom <dahlstrom@google.com> Signed-off-by: Jonathan Nieder <jrnieder@gmail.com> --- Hi, Seth House wrote: > No adverse effects were noted in a small survey of popular mergetools[1] > so this behavior defaults to `true`. However it can be globally disabled > by setting `mergetool.hideResolved` to `false`. Thanks much for protecting this by a flag. We tried this out internally at Google when it hit "next" and not too long later realized that the new default of "true" is not workable for us. I don't believe it's the right default for Git, either, hence this patch. Thanks for working on the merge resolution workflow; it's much appreciated. Sincerely, Jonathan Documentation/config/mergetool.txt | 2 +- git-mergetool.sh | 9 ++------- 2 files changed, 3 insertions(+), 8 deletions(-) diff --git a/Documentation/config/mergetool.txt b/Documentation/config/mergetool.txt index 90f76f5b9b..cafbbef46a 100644 --- a/Documentation/config/mergetool.txt +++ b/Documentation/config/mergetool.txt @@ -53,7 +53,7 @@ mergetool.hideResolved:: resolution. This flag causes 'LOCAL' and 'REMOTE' to be overwriten so that only the unresolved conflicts are presented to the merge tool. Can be configured per-tool via the `mergetool.<tool>.hideResolved` - configuration variable. Defaults to `true`. + configuration variable. Defaults to `false`. mergetool.keepBackup:: After performing a merge, the original file with conflict markers diff --git a/git-mergetool.sh b/git-mergetool.sh index 911470a5b2..f751d9cfe2 100755 --- a/git-mergetool.sh +++ b/git-mergetool.sh @@ -358,13 +358,8 @@ merge_file () { enabled=false fi else - # The user does not have a preference. Ask the tool. - if hide_resolved_enabled - then - enabled=true - else - enabled=false - fi + # The user does not have a preference. Default to disabled. + enabled=false fi if test "$enabled" = true -- 2.31.0.rc1.246.gcd05c9c855 ^ permalink raw reply related [flat|nested] 80+ messages in thread
* Re: [PATCH] mergetool: do not enable hideResolved by default 2021-03-09 2:29 ` [PATCH] mergetool: do not enable hideResolved by default Jonathan Nieder @ 2021-03-09 5:42 ` Seth House 2021-03-10 1:23 ` Jonathan Nieder 2021-03-10 8:06 ` Junio C Hamano 1 sibling, 1 reply; 80+ messages in thread From: Seth House @ 2021-03-09 5:42 UTC (permalink / raw) To: Jonathan Nieder; +Cc: Junio C Hamano, git, Felipe Contreras, Dana Dahlstrom On Mon, Mar 08, 2021 at 06:29:35PM -0800, Jonathan Nieder wrote: > A typical mergetool uses four panes, showing the content of the file > being resolved from MERGE_BASE ('BASE'), HEAD ('LOCAL'), MERGE_HEAD > ('REMOTE'), and the working copy. This allows understanding the > conflicts in context: by seeing the entire content of the file from > MERGE_HEAD, say, we can see the full intent of the code we are pulling > in and understand what they were trying to do that conflicted with our > own changes. Well said. Agreed on all counts. The very early days of these patch sets touched on this exact discussion point. (I'd link to it but that early discussion was a tad...unfocused.) I make semi-frequent reference of those versions of the conflicted file in the way you describe and have disabled hideResolved for a merge tool I maintain for that reason. > No adverse effects were noted in a small survey of popular mergetools[1] > so this behavior defaults to `true`. However it can be globally disabled > by setting `mergetool.hideResolved` to `false`. > > In practice, however, this has proved confusing for users. No > indication is shown in the UI that the base, local, and remote > versions shown have been modified by additional resolution. Compelling point. This flag drastically changes what LOCAL and REMOTE represent with little to no explanation. There are three options to achieve the same end-goal of hideResolved that I've thought of: 1. Individual merge tools should do this work, not Git. A merge tool already has all the information needed to hide already-resolved conflicts since that is what MERGED represents. Conflict markers *are* a two-way diff and a merge tool should display them as such, rather than display the textual markers verbatim. In many ways this is the ideal approach -- all merge tools could be doing this with existing Git right now but none have seemingly thought of doing so yet. 2. Git could pass six versions of the conflicted file to a merge tool, rather than the current four. Merge tools could accept LOCAL, REMOTE, BASE, MERGED (as most currently do), and also LCONFL and RCONFL files. The latter two being copies of MERGED but "pre split" by Git into the left conflicts and the right conflicts. This would spare the merge tool the work of splitting MERGED. It may encourage them to continue displaying LOCAL and REMOTE as useful context but also make it easy to diff LCONFL with RCONFL and use that diff to actually resolve the conflict. It could also make things worse, as many tools simply diff _every_ file Git gives them regardless if that makes sense or not (>_<). 3. Git could overwrite LOCAL and REMOTE to display only unresolved conflicts. (The current hideResolved addition.) This has the pragmatic benefit of requiring the least amount of change for all merge tools, but to your point above, *destroys* valuable data -- the additional context to help understand where the conflicts came from -- and that data can't be viewd without running additional Git commands to fetch it. Defaulting hideResolved to off is a fine change IMO. We don't have a way to communicate to the end-user that LOCAL and REMOTE represent something markedly different than what they have traditionally represented, so having this be an opt-in will force the user to read the docs and understand the ramifications. I really appreciate your thoughts that accompanied this patch. Sorry for the long response but your email made me want to ask the question: Does the need to default hideResolved to off mean that it is the wrong approach? Thinking through an end-user's workflow: would a user want to configure two copies of the same merge tool -- one with hideResolved and one without? An easy conflict could benefit from the former but if it's a tricky conflict the user would have to exit the tool and reopen the same tool without the flag. That sounds like an annoying workflow, and although the user would now have that extra, valuable context it would also put them squarely back into the current state of viewing already-resolved conflicts. I know the Option 3, hideResolved, is merged and has that momentum and this patch looks good to me -- but perhaps Option 2 is more "correct", or Option 1, or yet another option I haven't thought of. Thoughts? ^ permalink raw reply [flat|nested] 80+ messages in thread
* Re: [PATCH] mergetool: do not enable hideResolved by default 2021-03-09 5:42 ` Seth House @ 2021-03-10 1:23 ` Jonathan Nieder 0 siblings, 0 replies; 80+ messages in thread From: Jonathan Nieder @ 2021-03-10 1:23 UTC (permalink / raw) To: Seth House; +Cc: Junio C Hamano, git, Felipe Contreras, Dana Dahlstrom Hi, Seth House wrote: > The very early days of these patch sets touched on this exact discussion > point. (I'd link to it but that early discussion was a tad...unfocused.) > I make semi-frequent reference of those versions of the conflicted file > in the way you describe and have disabled hideResolved for a merge tool > I maintain for that reason. Thanks. Do you have a public example of a merge that was produced in such a way? It might help focus the discussion. For concreteness' sake: in the repository that Dana mentioned, one can see some merges from before hideResolved at https://android.googlesource.com/platform/tools/idea/+log/mirror-goog-studio-master-dev/build.txt. The xml files there (I'm not sure these are the right ones for me to focus on, just commenting as I observe) remind me of other routine conflicts with xml I've had to resolve in the past, e.g. at https://git.eclipse.org/r/c/jgit/jgit/+/134451/3. Having information from each side of the merge and not a mixture can be very helpful in this kind of case. That's especially true when the three-way merge algorithm didn't end up lining up the files correctly, which has happened from time to time to me in files with repetitive structure. [...] > There are three options to achieve the same end-goal of hideResolved > that I've thought of: > > 1. Individual merge tools should do this work, not Git. > > A merge tool already has all the information needed to hide > already-resolved conflicts since that is what MERGED represents. > Conflict markers *are* a two-way diff and a merge tool should > display them as such, rather than display the textual markers > verbatim. > > In many ways this is the ideal approach -- all merge tools could be > doing this with existing Git right now but none have seemingly > thought of doing so yet. One obstacle to this is that a merge tool can't count on the file in the worktree containing pristine conflict markers, because the user may have already started to work on the merge resolution. > 2. Git could pass six versions of the conflicted file to a merge tool, > rather than the current four. > > Merge tools could accept LOCAL, REMOTE, BASE, MERGED (as most > currently do), and also LCONFL and RCONFL files. The latter two > being copies of MERGED but "pre split" by Git into the left > conflicts and the right conflicts. > > This would spare the merge tool the work of splitting MERGED. It may > encourage them to continue displaying LOCAL and REMOTE as useful > context but also make it easy to diff LCONFL with RCONFL and use > that diff to actually resolve the conflict. It could also make > things worse, as many tools simply diff _every_ file Git gives them > regardless if that makes sense or not (>_<). Interesting! I kind of like this, especially if it were something the tool could opt in to. That said, I'm not the best person to ask, since I never ended up finding a good workflow using mergetool for my own use; instead, I tend to do the work of a merge tool "by hand": - gradually resolving the merge in each diff3-style conflict hunk by removing common lines from base+local and base+remote until there is nothing left in base - in harder cases, making the worktree match the local version, putting the diff from base to remote in a temporary file, and then hunk by hunk applying it - in even harder cases, using git-imerge <https://github.com/mhagger/git-imerge> [...] > 3. Git could overwrite LOCAL and REMOTE to display only unresolved > conflicts. > > (The current hideResolved addition.) This has the pragmatic benefit > of requiring the least amount of change for all merge tools, That's a good argument for having the option available, *as long as the user explicitly turns it on*. [...] > Does the need to default hideResolved to off mean that it is the wrong > approach? One disadvantage relative to (1) is that the mergetool has no way to visually distinguish the automatically resolved portion. For that reason, I suspect this will never be something we can make the default. But in principle I'm not against it existing. The implementation is concise and maintainable. The documentation adds a little user-facing complexity; I think as long as we're able to keep it clear and well maintained, that should be okay. git-mergetool.txt probably ought to mention the hideResolved setting. Otherwise, users can have a confusing experience if they set the config once and forget about it later. [...] > Thinking through an end-user's workflow: would a user want to configure > two copies of the same merge tool -- one with hideResolved and one > without? An easy conflict could benefit from the former but if it's > a tricky conflict the user would have to exit the tool and reopen the > same tool without the flag. That sounds like an annoying workflow, and > although the user would now have that extra, valuable context it would > also put them squarely back into the current state of viewing > already-resolved conflicts. > > I know the Option 3, hideResolved, is merged and has that momentum and > this patch looks good to me -- but perhaps Option 2 is more "correct", > or Option 1, or yet another option I haven't thought of. Thoughts? I suspect option 1 is indeed more correct. Dana mentions that some mergetools (p4merge?) use different colors to highlight the 'automatically resolved' portions, something that isn't possible using option 3. Thanks, Jonathan ^ permalink raw reply [flat|nested] 80+ messages in thread
* Re: [PATCH] mergetool: do not enable hideResolved by default 2021-03-09 2:29 ` [PATCH] mergetool: do not enable hideResolved by default Jonathan Nieder 2021-03-09 5:42 ` Seth House @ 2021-03-10 8:06 ` Junio C Hamano 2021-03-11 1:57 ` Junio C Hamano 1 sibling, 1 reply; 80+ messages in thread From: Junio C Hamano @ 2021-03-10 8:06 UTC (permalink / raw) To: Jonathan Nieder; +Cc: git, Felipe Contreras, Seth House, Dana Dahlstrom Jonathan Nieder <jrnieder@gmail.com> writes: > diff --git a/git-mergetool.sh b/git-mergetool.sh > index 911470a5b2..f751d9cfe2 100755 > --- a/git-mergetool.sh > +++ b/git-mergetool.sh > @@ -358,13 +358,8 @@ merge_file () { > enabled=false > fi > else > - # The user does not have a preference. Ask the tool. > - if hide_resolved_enabled > - then > - enabled=true > - else > - enabled=false > - fi > + # The user does not have a preference. Default to disabled. > + enabled=false OK. So the logic used to be - If the user has preference for a specific backend, use it; - If the user says the feature is unwanted, that is final; - If the user says it generally is OK to use the feature, let each backend set the preference; - If there is no preference, let each backend set the preference. As we want to disable the feature for any backend when the user does not explicitly say the feature is wanted (either in general, or for a specific backend), the change in the above hunk is exactly want we want to see. Looking good. Let's not revert the series and disable by default. Should I expect an updated log message, though? What was in the proposed log message sounded more unsubstantiated complaint than giving readable reasons why the feature is unwanted, but both the response by Seth and your response to Seth's response had material that made it more convincing why we would want to disable this by default, e.g. "with little to no explanation", "We don't have a way to communicate to the end-user" (both by Seth), "when ... didn't end up lining up the files correctly", "no way to visually distinguish" (yours) are all good ingredients to explain why this feature is prone to subtly and silently give wrong information to the end-users. Thanks. ^ permalink raw reply [flat|nested] 80+ messages in thread
* Re: [PATCH] mergetool: do not enable hideResolved by default 2021-03-10 8:06 ` Junio C Hamano @ 2021-03-11 1:57 ` Junio C Hamano 2021-03-12 23:12 ` Junio C Hamano 0 siblings, 1 reply; 80+ messages in thread From: Junio C Hamano @ 2021-03-11 1:57 UTC (permalink / raw) To: Jonathan Nieder; +Cc: git, Felipe Contreras, Seth House, Dana Dahlstrom Junio C Hamano <gitster@pobox.com> writes: > As we want to disable the feature for any backend when the user does > not explicitly say the feature is wanted (either in general, or for > a specific backend), the change in the above hunk is exactly want we > want to see. > > Looking good. Let's not revert the series and disable by default. > > Should I expect an updated log message, though? What was in the > proposed log message sounded more unsubstantiated complaint than > giving readable reasons why the feature is unwanted, but both the > response by Seth and your response to Seth's response had material > that made it more convincing why we would want to disable this by > default, e.g. "with little to no explanation", "We don't have a way > to communicate to the end-user" (both by Seth), "when ... didn't end > up lining up the files correctly", "no way to visually distinguish" > (yours) are all good ingredients to explain why this feature is > prone to subtly and silently give wrong information to the > end-users. For tonight's pushout, I'll use the patch as-is and merge it in 'seen'. Thanks. ^ permalink raw reply [flat|nested] 80+ messages in thread
* Re: [PATCH] mergetool: do not enable hideResolved by default 2021-03-11 1:57 ` Junio C Hamano @ 2021-03-12 23:12 ` Junio C Hamano 2021-03-12 23:29 ` Jonathan Nieder 0 siblings, 1 reply; 80+ messages in thread From: Junio C Hamano @ 2021-03-12 23:12 UTC (permalink / raw) To: Jonathan Nieder; +Cc: git, Felipe Contreras, Seth House, Dana Dahlstrom Junio C Hamano <gitster@pobox.com> writes: > Junio C Hamano <gitster@pobox.com> writes: > >> As we want to disable the feature for any backend when the user does >> not explicitly say the feature is wanted (either in general, or for >> a specific backend), the change in the above hunk is exactly want we >> want to see. >> >> Looking good. Let's not revert the series and disable by default. >> >> Should I expect an updated log message, though? What was in the >> proposed log message sounded more unsubstantiated complaint than >> giving readable reasons why the feature is unwanted, but both the >> response by Seth and your response to Seth's response had material >> that made it more convincing why we would want to disable this by >> default, e.g. "with little to no explanation", "We don't have a way >> to communicate to the end-user" (both by Seth), "when ... didn't end >> up lining up the files correctly", "no way to visually distinguish" >> (yours) are all good ingredients to explain why this feature is >> prone to subtly and silently give wrong information to the >> end-users. > > For tonight's pushout, I'll use the patch as-is and merge it in > 'seen'. Any progress here? ^ permalink raw reply [flat|nested] 80+ messages in thread
* Re: [PATCH] mergetool: do not enable hideResolved by default 2021-03-12 23:12 ` Junio C Hamano @ 2021-03-12 23:29 ` Jonathan Nieder 2021-03-12 23:36 ` Junio C Hamano 0 siblings, 1 reply; 80+ messages in thread From: Jonathan Nieder @ 2021-03-12 23:29 UTC (permalink / raw) To: Junio C Hamano; +Cc: git, Felipe Contreras, Seth House, Dana Dahlstrom Junio C Hamano wrote: > Junio C Hamano <gitster@pobox.com> writes: > > Junio C Hamano <gitster@pobox.com> writes: >>> As we want to disable the feature for any backend when the user does >>> not explicitly say the feature is wanted (either in general, or for >>> a specific backend), the change in the above hunk is exactly want we >>> want to see. >>> >>> Looking good. Let's not revert the series and disable by default. >>> >>> Should I expect an updated log message, though? [...] >> For tonight's pushout, I'll use the patch as-is and merge it in >> 'seen'. > > Any progress here? Sorry for the delay. I should be able to send out an improved log message (more concise and summarizing the supporting info from this thread) later this afternoon. Thanks, Jonathan ^ permalink raw reply [flat|nested] 80+ messages in thread
* Re: [PATCH] mergetool: do not enable hideResolved by default 2021-03-12 23:29 ` Jonathan Nieder @ 2021-03-12 23:36 ` Junio C Hamano 2021-03-13 8:36 ` [PATCH v2 0/2] " Jonathan Nieder 0 siblings, 1 reply; 80+ messages in thread From: Junio C Hamano @ 2021-03-12 23:36 UTC (permalink / raw) To: Jonathan Nieder; +Cc: git, Felipe Contreras, Seth House, Dana Dahlstrom Jonathan Nieder <jrnieder@gmail.com> writes: >> Any progress here? > > Sorry for the delay. I should be able to send out an improved log > message (more concise and summarizing the supporting info from this > thread) later this afternoon. Thanks. I think this is the last known regression in the -rc, and an update before the final happens on coming Monday is very much appreciated. ^ permalink raw reply [flat|nested] 80+ messages in thread
* [PATCH v2 0/2] mergetool: do not enable hideResolved by default 2021-03-12 23:36 ` Junio C Hamano @ 2021-03-13 8:36 ` Jonathan Nieder 2021-03-13 8:38 ` [PATCH 1/2] " Jonathan Nieder 2021-03-13 8:41 ` [PATCH 2/2] doc: describe mergetool configuration in git-mergetool(1) Jonathan Nieder 0 siblings, 2 replies; 80+ messages in thread From: Jonathan Nieder @ 2021-03-13 8:36 UTC (permalink / raw) To: Junio C Hamano; +Cc: git, Felipe Contreras, Seth House, Dana Dahlstrom Junio C Hamano wrote: > Jonathan Nieder <jrnieder@gmail.com> writes: >> I should be able to send out an improved log >> message (more concise and summarizing the supporting info from this >> thread) later this afternoon. > > Thanks. I think this is the last known regression in the -rc, and > an update before the final happens on coming Monday is very much > appreciated. A little late, but here it is. Thoughts of all kinds welcome, as always. Jonathan Nieder (2): mergetool: do not enable hideResolved by default doc: describe mergetool configuration in git-mergetool page Documentation/config/mergetool.txt | 2 +- Documentation/git-mergetool.txt | 4 ++++ git-mergetool.sh | 9 ++------- 3 files changed, 7 insertions(+), 8 deletions(-) ^ permalink raw reply [flat|nested] 80+ messages in thread
* [PATCH 1/2] mergetool: do not enable hideResolved by default 2021-03-13 8:36 ` [PATCH v2 0/2] " Jonathan Nieder @ 2021-03-13 8:38 ` Jonathan Nieder 2021-03-13 8:41 ` [PATCH 2/2] doc: describe mergetool configuration in git-mergetool(1) Jonathan Nieder 1 sibling, 0 replies; 80+ messages in thread From: Jonathan Nieder @ 2021-03-13 8:38 UTC (permalink / raw) To: Junio C Hamano; +Cc: git, Felipe Contreras, Seth House, Dana Dahlstrom When 98ea309b3f (mergetool: add hideResolved configuration, 2021-02-09) introduced the mergetool.hideResolved setting to reduce the clutter in viewing non-conflicted sections of files in a mergetool, it enabled it by default, explaining: No adverse effects were noted in a small survey of popular mergetools[1] so this behavior defaults to `true`. In practice, alas, adverse effects do appear. A few issues: 1. No indication is shown in the UI that the base, local, and remote versions shown have been modified by additional resolution. This is inherent in the design: the idea of mergetool.hideResolved is to convince a mergetool that expects pristine local, base, and remote files to show partially resolved verisons of those files instead; there is no additional source of information accessible to the mergetool to see where the resolution has happened. (By contrast, a mergetool generating the partial resolution from conflict markers for itself would be able to hilight the resolved sections with a different color.) A user accustomed to seeing the files without partial resolution gets no indication that this behavior has changed when they upgrade Git. 2. If the computed merge did not line up the files correctly (for example due to repeated sections in the file), the partially resolved files can be misleading and do not have enough information to reconstruct what happened and compute the correct merge result. 3. Resolving a conflict can involve information beyond the textual conflict. For example, if the local and remote versions added overlapping functionality in different ways, seeing the full unresolved versions of each alongside the base gives information about each side's intent that makes it possible to come up with a resolution that combines those two intents. By contrast, when starting with partially resolved versions of those files, one can produce a subtly wrong resolution that includes redundant extra code added by one side that is not needed in the approach taken on the other. All that said, a user wanting to focus on textual conflicts with reduced clutter can still benefit from mergetool.hideResolved=true as a way to deemphasize sections of the code that resolve cleanly without requiring any changes to the invoked mergetool. The caveats described above are reduced when the user has explicitly turned this on, because then the user is aware of them. Flip the default to 'false'. Reported-by: Dana Dahlstrom <dahlstrom@google.com> Helped-by: Seth House <seth@eseth.com> Signed-off-by: Jonathan Nieder <jrnieder@gmail.com> --- Only difference from v1 is the commit message. Documentation/config/mergetool.txt | 2 +- git-mergetool.sh | 9 ++------- 2 files changed, 3 insertions(+), 8 deletions(-) diff --git a/Documentation/config/mergetool.txt b/Documentation/config/mergetool.txt index 90f76f5b9ba..cafbbef46ae 100644 --- a/Documentation/config/mergetool.txt +++ b/Documentation/config/mergetool.txt @@ -53,7 +53,7 @@ mergetool.hideResolved:: resolution. This flag causes 'LOCAL' and 'REMOTE' to be overwriten so that only the unresolved conflicts are presented to the merge tool. Can be configured per-tool via the `mergetool.<tool>.hideResolved` - configuration variable. Defaults to `true`. + configuration variable. Defaults to `false`. mergetool.keepBackup:: After performing a merge, the original file with conflict markers diff --git a/git-mergetool.sh b/git-mergetool.sh index 911470a5b2c..f751d9cfe20 100755 --- a/git-mergetool.sh +++ b/git-mergetool.sh @@ -358,13 +358,8 @@ merge_file () { enabled=false fi else - # The user does not have a preference. Ask the tool. - if hide_resolved_enabled - then - enabled=true - else - enabled=false - fi + # The user does not have a preference. Default to disabled. + enabled=false fi if test "$enabled" = true -- 2.31.0.rc2.261.g7f71774620 ^ permalink raw reply related [flat|nested] 80+ messages in thread
* [PATCH 2/2] doc: describe mergetool configuration in git-mergetool(1) 2021-03-13 8:36 ` [PATCH v2 0/2] " Jonathan Nieder 2021-03-13 8:38 ` [PATCH 1/2] " Jonathan Nieder @ 2021-03-13 8:41 ` Jonathan Nieder 2021-03-13 23:34 ` Junio C Hamano 2021-03-13 23:37 ` Junio C Hamano 1 sibling, 2 replies; 80+ messages in thread From: Jonathan Nieder @ 2021-03-13 8:41 UTC (permalink / raw) To: Junio C Hamano; +Cc: git, Felipe Contreras, Seth House, Dana Dahlstrom In particular, this describes mergetool.hideResolved, which can help users discover this setting (either because it may be useful to them or in order to understand mergetool's behavior if they have forgotten setting it in the past). Tested by running make -C Documentation git-mergetool.1 man Documentation/git-mergetool.1 and reading through the page. Signed-off-by: Jonathan Nieder <jrnieder@gmail.com> --- Documentation/git-mergetool.txt | 4 ++++ 1 file changed, 4 insertions(+) Thanks for reading. diff --git a/Documentation/git-mergetool.txt b/Documentation/git-mergetool.txt index 6b14702e784..e587c7763a7 100644 --- a/Documentation/git-mergetool.txt +++ b/Documentation/git-mergetool.txt @@ -99,6 +99,10 @@ success of the resolution after the custom tool has exited. (see linkgit:git-config[1]). To cancel `diff.orderFile`, use `-O/dev/null`. +CONFIGURATION +------------- +include::config/mergetool.txt[] + TEMPORARY FILES --------------- `git mergetool` creates `*.orig` backup files while resolving merges. -- 2.31.0.rc2.261.g7f71774620 ^ permalink raw reply related [flat|nested] 80+ messages in thread
* Re: [PATCH 2/2] doc: describe mergetool configuration in git-mergetool(1) 2021-03-13 8:41 ` [PATCH 2/2] doc: describe mergetool configuration in git-mergetool(1) Jonathan Nieder @ 2021-03-13 23:34 ` Junio C Hamano 2021-03-13 23:37 ` Junio C Hamano 1 sibling, 0 replies; 80+ messages in thread From: Junio C Hamano @ 2021-03-13 23:34 UTC (permalink / raw) To: Jonathan Nieder; +Cc: git, Felipe Contreras, Seth House, Dana Dahlstrom Jonathan Nieder <jrnieder@gmail.com> writes: > diff --git a/Documentation/git-mergetool.txt b/Documentation/git-mergetool.txt > index 6b14702e784..e587c7763a7 100644 > --- a/Documentation/git-mergetool.txt > +++ b/Documentation/git-mergetool.txt > @@ -99,6 +99,10 @@ success of the resolution after the custom tool has exited. > (see linkgit:git-config[1]). To cancel `diff.orderFile`, > use `-O/dev/null`. > > +CONFIGURATION > +------------- > +include::config/mergetool.txt[] > + It is a nice touch. We don't have much explanation other than the description below ... mergetool.hideResolved:: During a merge Git will automatically resolve as many conflicts as possible and write the 'MERGED' file containing conflict markers around any conflicts that it cannot resolve; 'LOCAL' and 'REMOTE' normally represent the versions of the file from before Git's conflict resolution. This flag causes 'LOCAL' and 'REMOTE' to be overwriten so that only the unresolved conflicts are presented to the merge tool. Can be configured per-tool via the `mergetool.<tool>.hideResolved` configuration variable. Defaults to `false`. ... which appears in the included file on the feature. Thanks. ^ permalink raw reply [flat|nested] 80+ messages in thread
* Re: [PATCH 2/2] doc: describe mergetool configuration in git-mergetool(1) 2021-03-13 8:41 ` [PATCH 2/2] doc: describe mergetool configuration in git-mergetool(1) Jonathan Nieder 2021-03-13 23:34 ` Junio C Hamano @ 2021-03-13 23:37 ` Junio C Hamano 1 sibling, 0 replies; 80+ messages in thread From: Junio C Hamano @ 2021-03-13 23:37 UTC (permalink / raw) To: Jonathan Nieder; +Cc: git, Felipe Contreras, Seth House, Dana Dahlstrom Jonathan Nieder <jrnieder@gmail.com> writes: > Tested by running > > make -C Documentation git-mergetool.1 > man Documentation/git-mergetool.1 > > and reading through the page. Nice. Also applying this step and running cd Documentation && ./doc-diff HEAD^ HEAD would was a trivial way to see the change ;-) ^ permalink raw reply [flat|nested] 80+ messages in thread
* [PATCH v11 2/3] mergetool: break setup_tool out into separate initialization function 2021-02-09 20:07 ` [PATCH v11 0/3] mergetool: add hideResolved configuration (was automerge) Seth House 2021-02-09 20:07 ` [PATCH v11 1/3] mergetool: add hideResolved configuration Seth House @ 2021-02-09 20:07 ` Seth House 2021-02-09 20:07 ` [PATCH v11 3/3] mergetool: add per-tool support and overrides for the hideResolved flag Seth House 2021-02-09 22:11 ` [PATCH v11 0/3] mergetool: add hideResolved configuration (was automerge) Junio C Hamano 3 siblings, 0 replies; 80+ messages in thread From: Seth House @ 2021-02-09 20:07 UTC (permalink / raw) To: git; +Cc: Seth House This is preparation for the following commit where we need to source the mergetool shell script to look for overrides before `run_merge_tool` is called. Previously `run_merge_tool` both sourced that script and invoked the mergetool. In the case of the following commit, we need the result of the `hide_resolved` override, if present, before we actually run `run_merge_tool`. The new `initialize_merge_tool` wrapper is exposed and documented as a public interface for consistency with the existing `run_merge_tool` which is also public. Although `setup_tool` could instead be exposed directly, the related `setup_user_tool` would probably also want to be elevated to match and this felt the cleanest to me. Signed-off-by: Seth House <seth@eseth.com> --- Documentation/git-mergetool--lib.txt | 4 ++++ git-difftool--helper.sh | 6 ++++++ git-mergetool--lib.sh | 7 ++++--- git-mergetool.sh | 2 ++ 4 files changed, 16 insertions(+), 3 deletions(-) diff --git a/Documentation/git-mergetool--lib.txt b/Documentation/git-mergetool--lib.txt index 4da9d24096..3e8f59ac0e 100644 --- a/Documentation/git-mergetool--lib.txt +++ b/Documentation/git-mergetool--lib.txt @@ -38,6 +38,10 @@ get_merge_tool_cmd:: get_merge_tool_path:: returns the custom path for a merge tool. +initialize_merge_tool:: + bring merge tool specific functions into scope so they can be used or + overridden. + run_merge_tool:: launches a merge tool given the tool name and a true/false flag to indicate whether a merge base is present. diff --git a/git-difftool--helper.sh b/git-difftool--helper.sh index 46af3e60b7..992124cc67 100755 --- a/git-difftool--helper.sh +++ b/git-difftool--helper.sh @@ -61,6 +61,9 @@ launch_merge_tool () { export BASE eval $GIT_DIFFTOOL_EXTCMD '"$LOCAL"' '"$REMOTE"' else + initialize_merge_tool "$merge_tool" + # ignore the error from the above --- run_merge_tool + # will diagnose unusable tool by itself run_merge_tool "$merge_tool" fi } @@ -79,6 +82,9 @@ if test -n "$GIT_DIFFTOOL_DIRDIFF" then LOCAL="$1" REMOTE="$2" + initialize_merge_tool "$merge_tool" + # ignore the error from the above --- run_merge_tool + # will diagnose unusable tool by itself run_merge_tool "$merge_tool" false else # Launch the merge tool on each path provided by 'git diff' diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh index 78f3647ed9..4a8e36c792 100644 --- a/git-mergetool--lib.sh +++ b/git-mergetool--lib.sh @@ -250,6 +250,10 @@ trust_exit_code () { fi } +initialize_merge_tool () { + # Bring tool-specific functions into scope + setup_tool "$1" || return 1 +} # Entry point for running tools run_merge_tool () { @@ -261,9 +265,6 @@ run_merge_tool () { merge_tool_path=$(get_merge_tool_path "$1") || exit base_present="$2" - # Bring tool-specific functions into scope - setup_tool "$1" || return 1 - if merge_mode then run_merge_cmd "$1" diff --git a/git-mergetool.sh b/git-mergetool.sh index 40a103443d..e5eac935f3 100755 --- a/git-mergetool.sh +++ b/git-mergetool.sh @@ -272,6 +272,8 @@ merge_file () { ext= esac + initialize_merge_tool "$merge_tool" || return + mergetool_tmpdir_init if test "$MERGETOOL_TMPDIR" != "." -- 2.29.2 ^ permalink raw reply related [flat|nested] 80+ messages in thread
* [PATCH v11 3/3] mergetool: add per-tool support and overrides for the hideResolved flag 2021-02-09 20:07 ` [PATCH v11 0/3] mergetool: add hideResolved configuration (was automerge) Seth House 2021-02-09 20:07 ` [PATCH v11 1/3] mergetool: add hideResolved configuration Seth House 2021-02-09 20:07 ` [PATCH v11 2/3] mergetool: break setup_tool out into separate initialization function Seth House @ 2021-02-09 20:07 ` Seth House 2021-02-09 22:11 ` [PATCH v11 0/3] mergetool: add hideResolved configuration (was automerge) Junio C Hamano 3 siblings, 0 replies; 80+ messages in thread From: Seth House @ 2021-02-09 20:07 UTC (permalink / raw) To: git; +Cc: Seth House, Johannes Sixt, Junio C Hamano Add a per-tool override flag so that users may enable the flag for one tool and disable it for another by setting `mergetool.<tool>.hideResolved` to `false`. In addition, the author or maintainer of a mergetool may optionally override the default `hideResolved` value for that mergetool. If the `mergetools/<tool>` shell script contains a `hide_resolved_enabled` function it will be called when the mergetool is invoked and the return value will be used as the default for the `hideResolved` flag. hide_resolved_enabled () { return 1 } Disabling may be desirable if the mergetool wants or needs access to the original, unmodified 'LOCAL' and 'REMOTE' versions of the conflicted file. For example: - A tool may use a custom conflict resolution algorithm and prefer to ignore the results of Git's conflict resolution. - A tool may want to visually compare/constrast the version of the file from before the merge (saved to 'LOCAL', 'REMOTE', and 'BASE') with Git's conflict resolution results (saved to 'MERGED'). Helped-by: Johannes Sixt <j6t@kdbg.org> Helped-by: Junio C Hamano <gitster@pobox.com> Signed-off-by: Seth House <seth@eseth.com> --- Documentation/config/mergetool.txt | 5 +++++ git-mergetool--lib.sh | 4 ++++ git-mergetool.sh | 36 +++++++++++++++++++++++++++++- 3 files changed, 44 insertions(+), 1 deletion(-) diff --git a/Documentation/config/mergetool.txt b/Documentation/config/mergetool.txt index b858191970..90f76f5b9b 100644 --- a/Documentation/config/mergetool.txt +++ b/Documentation/config/mergetool.txt @@ -13,6 +13,11 @@ mergetool.<tool>.cmd:: merged; 'MERGED' contains the name of the file to which the merge tool should write the results of a successful merge. +mergetool.<tool>.hideResolved:: + Allows the user to override the global `mergetool.hideResolved` value + for a specific tool. See `mergetool.hideResolved` for the full + description. + mergetool.<tool>.trustExitCode:: For a custom merge command, specify whether the exit code of the merge command can be used to determine whether the merge was diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh index 4a8e36c792..542a6a75eb 100644 --- a/git-mergetool--lib.sh +++ b/git-mergetool--lib.sh @@ -166,6 +166,10 @@ setup_tool () { return 1 } + hide_resolved_enabled () { + return 0 + } + translate_merge_tool_path () { echo "$1" } diff --git a/git-mergetool.sh b/git-mergetool.sh index e5eac935f3..911470a5b2 100755 --- a/git-mergetool.sh +++ b/git-mergetool.sh @@ -333,7 +333,41 @@ merge_file () { checkout_staged_file 2 "$MERGED" "$LOCAL" checkout_staged_file 3 "$MERGED" "$REMOTE" - if test "$(git config --type=bool mergetool.hideResolved)" != "false" + # hideResolved preferences hierarchy. + global_config="mergetool.hideResolved" + tool_config="mergetool.${merge_tool}.hideResolved" + + if enabled=$(git config --type=bool "$tool_config") + then + # The user has a specific preference for a specific tool and no + # other preferences should override that. + : ; + elif enabled=$(git config --type=bool "$global_config") + then + # The user has a general preference for all tools. + # + # 'true' means the user likes the feature so we should use it + # where possible but tool authors can still override. + # + # 'false' means the user doesn't like the feature so we should + # not use it anywhere. + if test "$enabled" = true && hide_resolved_enabled + then + enabled=true + else + enabled=false + fi + else + # The user does not have a preference. Ask the tool. + if hide_resolved_enabled + then + enabled=true + else + enabled=false + fi + fi + + if test "$enabled" = true then hide_resolved fi -- 2.29.2 ^ permalink raw reply related [flat|nested] 80+ messages in thread
* Re: [PATCH v11 0/3] mergetool: add hideResolved configuration (was automerge) 2021-02-09 20:07 ` [PATCH v11 0/3] mergetool: add hideResolved configuration (was automerge) Seth House ` (2 preceding siblings ...) 2021-02-09 20:07 ` [PATCH v11 3/3] mergetool: add per-tool support and overrides for the hideResolved flag Seth House @ 2021-02-09 22:11 ` Junio C Hamano 2021-02-09 23:27 ` Seth House 3 siblings, 1 reply; 80+ messages in thread From: Junio C Hamano @ 2021-02-09 22:11 UTC (permalink / raw) To: Seth House; +Cc: git, David Aguilar, Johannes Sixt, Felipe Contreras Seth House <seth@eseth.com> writes: > Changes since v10: > Seth House (3): > mergetool: add hideResolved configuration > mergetool: break setup_tool out into separate initialization function > mergetool: add per-tool support and overrides for the hideResolved > flag Thanks for all these iterations. The resuling series looks good to me. ^ permalink raw reply [flat|nested] 80+ messages in thread
* Re: [PATCH v11 0/3] mergetool: add hideResolved configuration (was automerge) 2021-02-09 22:11 ` [PATCH v11 0/3] mergetool: add hideResolved configuration (was automerge) Junio C Hamano @ 2021-02-09 23:27 ` Seth House 0 siblings, 0 replies; 80+ messages in thread From: Seth House @ 2021-02-09 23:27 UTC (permalink / raw) To: Junio C Hamano; +Cc: git, David Aguilar, Johannes Sixt, Felipe Contreras On Tue, Feb 09, 2021 at 02:11:05PM -0800, Junio C Hamano wrote: > Thanks for all these iterations. The resuling series looks good to > me. Woot! Thanks for all the great feedback and for walking me through the process. I'm really happy with where this ended up. ^ permalink raw reply [flat|nested] 80+ messages in thread
* Re: [PATCH v7 0/2] mergetool: add automerge configuration 2020-12-28 0:41 ` [PATCH v7 0/2] mergetool: add automerge configuration Seth House ` (2 preceding siblings ...) 2020-12-28 4:54 ` [PATCH v8 0/4] mergetool: add automerge configuration Seth House @ 2020-12-28 10:29 ` Junio C Hamano 2020-12-28 14:40 ` Felipe Contreras 3 siblings, 1 reply; 80+ messages in thread From: Junio C Hamano @ 2020-12-28 10:29 UTC (permalink / raw) To: Seth House; +Cc: git, David Aguilar, Johannes Sixt, Felipe Contreras Seth House <seth@eseth.com> writes: > * Signed off on Felipe's commit. (Although I have minor qualms with > Felipe's various wording and even the name of the flag it is > decidedly not worth burdening the list with bike-shedding.) Even when the original is a horrible patch in your opinion that is laden with bugs, as long as the original author signed it off (which means that the original author certifies that it can be included in and distributed by the project under our licensing terms, and agrees to the fact that the original author did so will be recorded in perpetuity), you can relay such a patch as-is, and you are required (i.e. SubmittingPatches is pretty clear that without your sign-off we cannot accept) to sign it off to record the provenance of the code. The other side of the above coin is that you are not endorsing or vounching for the patch when you sign it off, so your name is not smudged by wording and flag name chosen in a way that you may consider poor. So "Although..." part is not a good objection against signing it off. In other words, sign-off is not about assuring quality. Also, instead of relaying as-is, you can relay a patch with your improvements rolled into the same patch (i.e. not as follow-up fixes). Some (or major) parts of the original patch may still remain in the edited result and you'd need to keep original author's sign-off as-is [*1*]. In this topic's case, 2/2 would be a feature enhancement on top of 1/2, so relaying 1/2 as-is would be OK, but in a case where an promising patch was sent with sign-off and bugs, then gets abandoned by the original author, fixing the bug in the patch you relay in place (i.e. not as follow-up patches) may even be necessary to keep bisectability. When you do so, you'd typically do: Subject: [PATCH] title of the patch ... original author's log message, possibly copyedited ... by you + <Comment on what you did on top of the original can come here> Signed-off-by: Original Author <ori@ginal.au.thor> + [or brief comment here] + Signed-off-by: Your Name <you@your.do.main> (1) add your sign-off at the end (2) explain what you changed relative to the original, either inside [] on the line before your sign-off, or at the end of the log message proper. to indicate that it is not relayed as-is; this allows you to take responsibility of an unintended breakage your "fixes" might have caused. [Footnote] *1* The result may become something that no longer aligns the original author's opinion, but that is OK. The sign-off by the original author just says that the original author has the right to contribute (the remaining part of) the patch and the original author agrees that the record of author's involvement in the patch (including sign-off) will be kept. It is not about assuring quality of the final work by the original author, either. ^ permalink raw reply [flat|nested] 80+ messages in thread
* Re: [PATCH v7 0/2] mergetool: add automerge configuration 2020-12-28 10:29 ` [PATCH v7 0/2] mergetool: add automerge configuration Junio C Hamano @ 2020-12-28 14:40 ` Felipe Contreras 0 siblings, 0 replies; 80+ messages in thread From: Felipe Contreras @ 2020-12-28 14:40 UTC (permalink / raw) To: Junio C Hamano, Seth House Cc: git, David Aguilar, Johannes Sixt, Felipe Contreras Junio C Hamano wrote: > Also, instead of relaying as-is, you can relay a patch with your > improvements rolled into the same patch (i.e. not as follow-up > fixes). Some (or major) parts of the original patch may still > remain in the edited result and you'd need to keep original author's > sign-off as-is [*1*]. Yes, you *can*, but doing so in this case would be against the author's wishes, and a violation of the Developer Certificate of Origin. > In this topic's case, 2/2 would be a feature enhancement on top of > 1/2, so relaying 1/2 as-is would be OK, but in a case where an > promising patch was sent with sign-off and bugs, then gets abandoned > by the original author, fixing the bug in the patch you relay in > place (i.e. not as follow-up patches) may even be necessary to keep > bisectability. When you do so, you'd typically do: If the author doesn't object (which is usually the case), this makes sense. But if the author objects, you would be violating clause (d) of the DCO. Just because the only way to do X is to violate laws, terms, or agreements doesn't mean that's what you should do. You can simply not do X. -- Felipe Contreras ^ permalink raw reply [flat|nested] 80+ messages in thread
* RE: [PATCH v6 0/1] mergetool: add automerge configuration 2020-12-27 20:58 ` [PATCH v6 0/1] mergetool: add automerge configuration Seth House ` (2 preceding siblings ...) 2020-12-28 0:41 ` [PATCH v7 0/2] mergetool: add automerge configuration Seth House @ 2020-12-28 1:02 ` Felipe Contreras 3 siblings, 0 replies; 80+ messages in thread From: Felipe Contreras @ 2020-12-28 1:02 UTC (permalink / raw) To: Seth House, git Cc: Seth House, Junio C Hamano, David Aguilar, Johannes Sixt, Felipe Contreras Seth House wrote: > Sorry for the slow turnaround on this. I haven't used Git via email > patches before now so it took me quite a few hours to read through > tutorials, configure git-send-email and fight missing Perl libs. What distribution are you using? > Changes since v5: This is not v6 of my patch series; it's v1 of yours, which I think should have a different title. What happens when I want to do v6? Other than that (and the fact that you initially used the wrong version as a baseline), I'm fine with your approach of doing a patch on top of mine. Cheers. -- Felipe Contreras ^ permalink raw reply [flat|nested] 80+ messages in thread
end of thread, other threads:[~2021-03-13 23:38 UTC | newest] Thread overview: 80+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-12-23 4:53 [PATCH v5 0/1] mergetool: remove unconflicted lines Felipe Contreras 2020-12-23 4:53 ` [PATCH v5 1/1] mergetool: add automerge configuration Felipe Contreras 2020-12-23 13:34 ` Junio C Hamano 2020-12-23 14:23 ` Felipe Contreras 2020-12-23 20:21 ` Junio C Hamano 2020-12-24 0:14 ` Felipe Contreras 2020-12-24 0:32 ` Junio C Hamano 2020-12-24 1:36 ` Felipe Contreras 2020-12-24 6:17 ` Junio C Hamano 2020-12-24 15:59 ` Felipe Contreras 2020-12-24 22:32 ` Junio C Hamano 2020-12-27 18:01 ` Felipe Contreras 2020-12-24 9:24 ` [PATCH v5 0/1] mergetool: remove unconflicted lines Junio C Hamano 2020-12-24 16:16 ` Felipe Contreras 2020-12-30 5:47 ` Johannes Schindelin 2020-12-30 22:33 ` Felipe Contreras 2020-12-27 20:58 ` [PATCH v6 0/1] mergetool: add automerge configuration Seth House 2020-12-27 20:58 ` [PATCH v6 1/2] " Seth House 2020-12-27 22:06 ` Junio C Hamano 2020-12-27 22:29 ` Seth House 2020-12-27 22:59 ` Junio C Hamano 2020-12-27 20:58 ` [PATCH v6 2/2] mergetool: Add per-tool support for the autoMerge flag Seth House 2020-12-27 22:31 ` Junio C Hamano 2020-12-28 0:41 ` [PATCH v7 0/2] mergetool: add automerge configuration Seth House 2020-12-28 0:41 ` [PATCH v7 1/2] " Seth House 2020-12-28 0:41 ` [PATCH v7 2/2] mergetool: Add per-tool support for the autoMerge flag Seth House 2020-12-28 1:18 ` Felipe Contreras 2020-12-28 4:54 ` [PATCH v8 0/4] mergetool: add automerge configuration Seth House 2020-12-28 4:54 ` [PATCH v8 1/4] " Seth House 2020-12-28 11:30 ` Johannes Sixt 2020-12-28 4:54 ` [PATCH v8 2/4] mergetool: Add per-tool support for the autoMerge flag Seth House 2020-12-28 13:09 ` Junio C Hamano 2020-12-28 4:54 ` [PATCH v8 3/4] mergetool: Break setup_tool out into separate initialization function Seth House 2020-12-28 11:48 ` Johannes Sixt 2020-12-28 4:54 ` [PATCH v8 4/4] mergetool: Add automerge_enabled tool-specific override function Seth House 2020-12-28 11:57 ` Johannes Sixt 2020-12-28 13:19 ` Junio C Hamano 2020-12-28 19:29 ` [PATCH v9 0/5] mergetool: add automerge configuration Seth House 2020-12-28 19:29 ` [PATCH v9 1/5] " Seth House 2020-12-28 19:29 ` [PATCH v9 2/5] mergetool: alphabetize the mergetool config docs Seth House 2020-12-28 19:29 ` [PATCH v9 3/5] mergetool: add per-tool support for the autoMerge flag Seth House 2020-12-28 19:29 ` [PATCH v9 4/5] mergetool: break setup_tool out into separate initialization function Seth House 2020-12-29 8:50 ` Johannes Sixt 2020-12-29 17:23 ` Seth House 2020-12-28 19:29 ` [PATCH v9 5/5] mergetool: add automerge_enabled tool-specific override function Seth House 2020-12-29 2:01 ` Felipe Contreras 2021-01-06 5:55 ` Junio C Hamano 2021-01-07 3:58 ` Seth House 2021-01-07 6:38 ` Junio C Hamano 2021-01-07 9:27 ` Seth House 2021-01-07 21:26 ` Junio C Hamano 2021-01-08 15:04 ` Johannes Schindelin 2021-01-30 5:46 ` [PATCH v10 0/3] mergetool: add hideResolved configuration (was automerge) Seth House 2021-01-30 5:46 ` [PATCH v10 1/3] mergetool: add hideResolved configuration Seth House 2021-01-30 8:08 ` Junio C Hamano 2021-01-30 5:46 ` [PATCH v10 2/3] mergetool: break setup_tool out into separate initialization function Seth House 2021-01-30 5:46 ` [PATCH v10 3/3] mergetool: add per-tool support and overrides for the hideResolved flag Seth House 2021-01-30 8:08 ` Junio C Hamano 2021-02-09 20:07 ` [PATCH v11 0/3] mergetool: add hideResolved configuration (was automerge) Seth House 2021-02-09 20:07 ` [PATCH v11 1/3] mergetool: add hideResolved configuration Seth House 2021-03-09 2:29 ` [PATCH] mergetool: do not enable hideResolved by default Jonathan Nieder 2021-03-09 5:42 ` Seth House 2021-03-10 1:23 ` Jonathan Nieder 2021-03-10 8:06 ` Junio C Hamano 2021-03-11 1:57 ` Junio C Hamano 2021-03-12 23:12 ` Junio C Hamano 2021-03-12 23:29 ` Jonathan Nieder 2021-03-12 23:36 ` Junio C Hamano 2021-03-13 8:36 ` [PATCH v2 0/2] " Jonathan Nieder 2021-03-13 8:38 ` [PATCH 1/2] " Jonathan Nieder 2021-03-13 8:41 ` [PATCH 2/2] doc: describe mergetool configuration in git-mergetool(1) Jonathan Nieder 2021-03-13 23:34 ` Junio C Hamano 2021-03-13 23:37 ` Junio C Hamano 2021-02-09 20:07 ` [PATCH v11 2/3] mergetool: break setup_tool out into separate initialization function Seth House 2021-02-09 20:07 ` [PATCH v11 3/3] mergetool: add per-tool support and overrides for the hideResolved flag Seth House 2021-02-09 22:11 ` [PATCH v11 0/3] mergetool: add hideResolved configuration (was automerge) Junio C Hamano 2021-02-09 23:27 ` Seth House 2020-12-28 10:29 ` [PATCH v7 0/2] mergetool: add automerge configuration Junio C Hamano 2020-12-28 14:40 ` Felipe Contreras 2020-12-28 1:02 ` [PATCH v6 0/1] " Felipe Contreras
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.