| Message ID | pull.1773.git.1724066944786.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | git gui: add directly calling merge tool from gitconfig | expand |
Am 19.08.24 um 13:29 schrieb ToBoMi via GitGitGadget: > From: deboeto <tobias.boesch@miele.com> > > * git Gui can open a merge tool when conflicts are > detected. The merge tools that are allowed to > call have to be hard coded into git Gui > althgough there are configuration options for > merge tools git in the git config. Git calls > the configured merge tools directly from the > config while git Gui doesn't. > * git Gui can now call the tool configured in the > gitconfig directly. > * Can be enabled through setting > gui.mergeToolFromConfig Can we do better than having a new configuration variable? Let's say you have configured merge.tool=vscode. This tool is not supported, but you have configured mergetool.vscode.cmd suitably. Can we not use the latter configuration variable unconditionally? Likewise, say, you have configured merge.tool=bc3. This one *is* supported. What could go wrong if mergetool.bc3.cmd is used instead of the built-in command line? The behavior would change for users that configured mergetool.$tool.cmd for a supported tool. But would it change for the worse? BTW, the code builds different command lines depending on whether a base file is available or not. How does mergetool.$tool.cmd handle the cases? > * Disabled by default, since option is most likely > never set > * bc3 and vscode tested > > Signed-off-by: deboeto <tobias.boesch@miele.com> Some remarks on the commit message: - The Signed-off-by line has legal consequences. Therefore, we require that authors use their genuine name, not an alias. Also, the From line must match the Signed-off-by line. - Please have a look at the commit messages in the code base. The formatting presented here is very unusual. Please write in full sentences including punctuation, and use paragraphs where needed. - Please state the problem that is being solved (in present tense). This should motivate the change, i.e., provide a convincing argument why the change is needed. Then state what the solution is in imperative mood, that is, an instruction to the code to change in such and such way. Use examples to clarify how the new feature can be used. > --- > git gui: add directly calling merge tool from gitconfig > > Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1773%2FToBoMi%2Fadd_merge_tool_from_config_file-v1 > Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1773/ToBoMi/add_merge_tool_from_config_file-v1 > Pull-Request: https://github.com/gitgitgadget/git/pull/1773 > > Documentation/config/gui.txt | 4 ++++ > git-gui/lib/mergetool.tcl | 11 +++++++++-- > 2 files changed, 13 insertions(+), 2 deletions(-) > > diff --git a/Documentation/config/gui.txt b/Documentation/config/gui.txt > index 171be774d24..e63d0b46e7c 100644 > --- a/Documentation/config/gui.txt > +++ b/Documentation/config/gui.txt > @@ -55,3 +55,7 @@ gui.blamehistoryctx:: > linkgit:gitk[1] for the selected commit, when the `Show History > Context` menu item is invoked from 'git gui blame'. If this > variable is set to zero, the whole history is shown. > + > +gui.mergeToolFromConfig:: > + If true, allow to call the merge tool configured in gitconfig > + in git gui directly. > \ No newline at end of file Unfortunately, Documentation/config/gui.txt is not part of the Git GUI repository. Any changes to the documentation must be submitted as separate patch. Please be careful not to introduce an incomplete last lines. Take note of "No newline at end of file". It should not be there. > diff --git a/git-gui/lib/mergetool.tcl b/git-gui/lib/mergetool.tcl > index e688b016ef6..fbd0889612a 100644 > --- a/git-gui/lib/mergetool.tcl > +++ b/git-gui/lib/mergetool.tcl > @@ -272,8 +272,15 @@ proc merge_resolve_tool2 {} { > } > } > default { > - error_popup [mc "Unsupported merge tool '%s'" $tool] > - return > + if {[is_config_true gui.mergetoolfromconfig]} { > + set path [get_config mergetool.$tool.path] At this point, the value assigned to $path here is already available in $merge_tool_path. > + set cmdline_config [get_config mergetool.$tool.cmd] > + set cmdline_substituted [subst -nobackslashes -nocommands $cmdline_config] > + set cmdline [lreplace $cmdline_substituted 0 0 $path] I haven't yet taken the time to study what these lines do (I am far from fluent in Tcl) and have no opinion, yet. > + } else { > + error_popup [mc "Unsupported merge tool '%s'" $tool] > + return > + } > } > } > > > base-commit: b9849e4f7631d80f146d159bf7b60263b3205632 -- Hannes
> -----Ursprüngliche Nachricht----- > Von: Johannes Sixt <j6t@kdbg.org> > Gesendet: Samstag, 24. August 2024 15:38 > An: Boesch, Tobias <tobias.boesch@miele.com> > Cc: git@vger.kernel.org; ToBoMi via GitGitGadget <gitgitgadget@gmail.com> > Betreff: Re: [PATCH] git gui: add directly calling merge tool from gitconfig > Thanks for the review. > Am 19.08.24 um 13:29 schrieb ToBoMi via GitGitGadget: > > From: deboeto <tobias.boesch@miele.com> > > > > * git Gui can open a merge tool when conflicts are > > detected. The merge tools that are allowed to > > call have to be hard coded into git Gui > > althgough there are configuration options for > > merge tools git in the git config. Git calls > > the configured merge tools directly from the > > config while git Gui doesn't. > > * git Gui can now call the tool configured in the > > gitconfig directly. > > * Can be enabled through setting > > gui.mergeToolFromConfig > > Can we do better than having a new configuration variable? Let's say you have > configured merge.tool=vscode. This tool is not supported, but you have > configured mergetool.vscode.cmd suitably. Can we not use the latter > configuration variable unconditionally? > I think that would work. I'll change the patch to use the mergetool.cmd variable as the trigger for using the configured merge tool. If the mergetool configuration option is set to a supported tool (merge.tool = vscode) it will cause git gui to use the supported tool (hard coded into the source code - as before this change). If the mergetool configuration option is set to an UNsupported tool and mergetool.cmd is set for the chosen mergetool it will use the command from that option. Patch will be submitted soon. > Likewise, say, you have configured merge.tool=bc3. This one *is* supported. > What could go wrong if mergetool.bc3.cmd is used instead of the built-in > command line? The behavior would change for users that configured > mergetool.$tool.cmd for a supported tool. But would it change for the worse? > I tested this together with the change of the activating option mentioned before. With that in mind I cannot create a case were the mergetool.cmd is used on a supported tool, because mergetool.cmd is only used for UNsupported tools. 1. Assuming the merge.tool is set to a supported tool: - In this case the supported tool is used, no matter if the mergetool.cmd is set or not 2. Assuming the merge.tool is set to an UNsupported tool: - Then the variable IS evaluated - If it is set to an invalid command or a wrong mergetool.path is given, Git gui will complain as before this change that the command is not Found in PATH > BTW, the code builds different command lines depending on whether a base > file is available or not. How does mergetool.$tool.cmd handle the cases? > Currently it doesn't. I don't know if it should, because I guess that git also has no other possibility than to call this command for a merge unconditionally - even when the base file name is empty. I haven't had such a case that I can remember. How can it be triggered? Doesn't all merges have a common ancestor as long as the histories are related? > > * Disabled by default, since option is most likely > > never set > > * bc3 and vscode tested > > > > Signed-off-by: deboeto <tobias.boesch@miele.com> > > Some remarks on the commit message: > > - The Signed-off-by line has legal consequences. Therefore, we require that > authors use their genuine name, not an alias. Also, the From line must match > the Signed-off-by line. > > - Please have a look at the commit messages in the code base. The formatting > presented here is very unusual. Please write in full sentences including > punctuation, and use paragraphs where needed. > > - Please state the problem that is being solved (in present tense). This should > motivate the change, i.e., provide a convincing argument why the change is > needed. Then state what the solution is in imperative mood, that is, an > instruction to the code to change in such and such way. Use examples to > clarify how the new feature can be used. Corrected - please review again. > > > --- > > git gui: add directly calling merge tool from gitconfig > > > > Published-As: > > https://github.com/gitgitgadget/git/releases/tag/pr- > 1773%2FToBoMi%2Fad > > d_merge_tool_from_config_file-v1 > > Fetch-It-Via: git fetch https://github.com/gitgitgadget/git > > pr-1773/ToBoMi/add_merge_tool_from_config_file-v1 > > Pull-Request: https://github.com/gitgitgadget/git/pull/1773 > > > > Documentation/config/gui.txt | 4 ++++ > > git-gui/lib/mergetool.tcl | 11 +++++++++-- > > 2 files changed, 13 insertions(+), 2 deletions(-) > > > > diff --git a/Documentation/config/gui.txt > > b/Documentation/config/gui.txt index 171be774d24..e63d0b46e7c > 100644 > > --- a/Documentation/config/gui.txt > > +++ b/Documentation/config/gui.txt > > @@ -55,3 +55,7 @@ gui.blamehistoryctx:: > > linkgit:gitk[1] for the selected commit, when the `Show History > > Context` menu item is invoked from 'git gui blame'. If this > > variable is set to zero, the whole history is shown. > > + > > +gui.mergeToolFromConfig:: > > + If true, allow to call the merge tool configured in gitconfig > > + in git gui directly. > > \ No newline at end of file > > Unfortunately, Documentation/config/gui.txt is not part of the Git GUI > repository. Any changes to the documentation must be submitted as separate > patch. > > Please be careful not to introduce an incomplete last lines. Take note of "No > newline at end of file". It should not be there. > > > diff --git a/git-gui/lib/mergetool.tcl b/git-gui/lib/mergetool.tcl > > index e688b016ef6..fbd0889612a 100644 > > --- a/git-gui/lib/mergetool.tcl > > +++ b/git-gui/lib/mergetool.tcl > > @@ -272,8 +272,15 @@ proc merge_resolve_tool2 {} { > > } > > } > > default { > > - error_popup [mc "Unsupported merge tool '%s'" $tool] > > - return > > + if {[is_config_true gui.mergetoolfromconfig]} { > > + set path [get_config mergetool.$tool.path] > > At this point, the value assigned to $path here is already available in > $merge_tool_path. > > > + set cmdline_config [get_config mergetool.$tool.cmd] > > + set cmdline_substituted [subst -nobackslashes - > nocommands $cmdline_config] > > + set cmdline [lreplace $cmdline_substituted 0 0 $path] > > I haven't yet taken the time to study what these lines do (I am far from fluent > in Tcl) and have no opinion, yet. > > > + } else { > > + error_popup [mc "Unsupported merge tool '%s'" > $tool] > > + return > > + } > > } > > } > > > > > > base-commit: b9849e4f7631d80f146d159bf7b60263b3205632 > > -- Hannes ------------------------------------------------------------------------------------------------- imperial-Werke oHG, Sitz Bünde, Registergericht Bad Oeynhausen - HRA 4825
> -----Ursprüngliche Nachricht----- > Von: Johannes Sixt <j6t@kdbg.org> > Gesendet: Samstag, 24. August 2024 15:38 > An: Boesch, Tobias <tobias.boesch@miele.com> > Cc: git@vger.kernel.org; ToBoMi via GitGitGadget <gitgitgadget@gmail.com> > Betreff: Re: [PATCH] git gui: add directly calling merge tool from gitconfig > Continuing my incomplete last reply: > Am 19.08.24 um 13:29 schrieb ToBoMi via GitGitGadget: > > From: deboeto <tobias.boesch@miele.com> > > > > * git Gui can open a merge tool when conflicts are > > detected. The merge tools that are allowed to > > call have to be hard coded into git Gui > > althgough there are configuration options for > > merge tools git in the git config. Git calls > > the configured merge tools directly from the > > config while git Gui doesn't. > > * git Gui can now call the tool configured in the > > gitconfig directly. > > * Can be enabled through setting > > gui.mergeToolFromConfig > > Can we do better than having a new configuration variable? Let's say you have > configured merge.tool=vscode. This tool is not supported, but you have > configured mergetool.vscode.cmd suitably. Can we not use the latter > configuration variable unconditionally? > > Likewise, say, you have configured merge.tool=bc3. This one *is* supported. > What could go wrong if mergetool.bc3.cmd is used instead of the built-in > command line? The behavior would change for users that configured > mergetool.$tool.cmd for a supported tool. But would it change for the worse? > > BTW, the code builds different command lines depending on whether a base > file is available or not. How does mergetool.$tool.cmd handle the cases? > > > * Disabled by default, since option is most likely > > never set > > * bc3 and vscode tested > > > > Signed-off-by: deboeto <tobias.boesch@miele.com> > > Some remarks on the commit message: > > - The Signed-off-by line has legal consequences. Therefore, we require that > authors use their genuine name, not an alias. Also, the From line must match > the Signed-off-by line. > > - Please have a look at the commit messages in the code base. The formatting > presented here is very unusual. Please write in full sentences including > punctuation, and use paragraphs where needed. > > - Please state the problem that is being solved (in present tense). This should > motivate the change, i.e., provide a convincing argument why the change is > needed. Then state what the solution is in imperative mood, that is, an > instruction to the code to change in such and such way. Use examples to > clarify how the new feature can be used. > > > --- > > git gui: add directly calling merge tool from gitconfig > > > > Published-As: > > https://github.com/gitgitgadget/git/releases/tag/pr- > 1773%2FToBoMi%2Fad > > d_merge_tool_from_config_file-v1 > > Fetch-It-Via: git fetch https://github.com/gitgitgadget/git > > pr-1773/ToBoMi/add_merge_tool_from_config_file-v1 > > Pull-Request: https://github.com/gitgitgadget/git/pull/1773 > > > > Documentation/config/gui.txt | 4 ++++ > > git-gui/lib/mergetool.tcl | 11 +++++++++-- > > 2 files changed, 13 insertions(+), 2 deletions(-) > > > > diff --git a/Documentation/config/gui.txt > > b/Documentation/config/gui.txt index 171be774d24..e63d0b46e7c > 100644 > > --- a/Documentation/config/gui.txt > > +++ b/Documentation/config/gui.txt > > @@ -55,3 +55,7 @@ gui.blamehistoryctx:: > > linkgit:gitk[1] for the selected commit, when the `Show History > > Context` menu item is invoked from 'git gui blame'. If this > > variable is set to zero, the whole history is shown. > > + > > +gui.mergeToolFromConfig:: > > + If true, allow to call the merge tool configured in gitconfig > > + in git gui directly. > > \ No newline at end of file > > Unfortunately, Documentation/config/gui.txt is not part of the Git GUI > repository. Any changes to the documentation must be submitted as separate > patch. > Configuration option will be removed in the next patch version. Therefore the documentation change is no longer needed. > Please be careful not to introduce an incomplete last lines. Take note of "No > newline at end of file". It should not be there. > See above. > > diff --git a/git-gui/lib/mergetool.tcl b/git-gui/lib/mergetool.tcl > > index e688b016ef6..fbd0889612a 100644 > > --- a/git-gui/lib/mergetool.tcl > > +++ b/git-gui/lib/mergetool.tcl > > @@ -272,8 +272,15 @@ proc merge_resolve_tool2 {} { > > } > > } > > default { > > - error_popup [mc "Unsupported merge tool '%s'" $tool] > > - return > > + if {[is_config_true gui.mergetoolfromconfig]} { > > + set path [get_config mergetool.$tool.path] > > At this point, the value assigned to $path here is already available in > $merge_tool_path. > True - corrected in the next patch. > > + set cmdline_config [get_config mergetool.$tool.cmd] > > + set cmdline_substituted [subst -nobackslashes - > nocommands $cmdline_config] > > + set cmdline [lreplace $cmdline_substituted 0 0 $path] > > I haven't yet taken the time to study what these lines do (I am far from fluent > in Tcl) and have no opinion, yet. > They replace the variables one can put into mergetool.cmd like $REMOTE or $LOCAL. Without this substitution command they are not replaced with the real file paths. See this example for vscode: cmd = \"Code.exe\" --wait --merge \"$LOCAL\" \"$REMOTE\" \"$BASE\" \"$MERGED\" > > + } else { > > + error_popup [mc "Unsupported merge tool '%s'" > $tool] > > + return > > + } > > } > > } > > > > > > base-commit: b9849e4f7631d80f146d159bf7b60263b3205632 > > -- Hannes ------------------------------------------------------------------------------------------------- imperial-Werke oHG, Sitz Bünde, Registergericht Bad Oeynhausen - HRA 4825
diff --git a/Documentation/config/gui.txt b/Documentation/config/gui.txt index 171be774d24..e63d0b46e7c 100644 --- a/Documentation/config/gui.txt +++ b/Documentation/config/gui.txt @@ -55,3 +55,7 @@ gui.blamehistoryctx:: linkgit:gitk[1] for the selected commit, when the `Show History Context` menu item is invoked from 'git gui blame'. If this variable is set to zero, the whole history is shown. + +gui.mergeToolFromConfig:: + If true, allow to call the merge tool configured in gitconfig + in git gui directly. \ No newline at end of file diff --git a/git-gui/lib/mergetool.tcl b/git-gui/lib/mergetool.tcl index e688b016ef6..fbd0889612a 100644 --- a/git-gui/lib/mergetool.tcl +++ b/git-gui/lib/mergetool.tcl @@ -272,8 +272,15 @@ proc merge_resolve_tool2 {} { } } default { - error_popup [mc "Unsupported merge tool '%s'" $tool] - return + if {[is_config_true gui.mergetoolfromconfig]} { + set path [get_config mergetool.$tool.path] + set cmdline_config [get_config mergetool.$tool.cmd] + set cmdline_substituted [subst -nobackslashes -nocommands $cmdline_config] + set cmdline [lreplace $cmdline_substituted 0 0 $path] + } else { + error_popup [mc "Unsupported merge tool '%s'" $tool] + return + } } }