| Message ID | 20201228192919.1195211-6-seth@eseth.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | mergetool: add automerge configuration | expand |
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.
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?
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.
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.
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.
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).
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
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
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(-)