AW: [PATCH v5] git gui: add directly calling merge tool from configuration

> -----Ursprüngliche Nachricht-----
> Von: Boesch, Tobias
> Gesendet: Montag, 16. September 2024 10:42
> An: Johannes Sixt <j6t@kdbg.org>
> Cc: git@vger.kernel.org; ToBoMi via GitGitGadget <gitgitgadget@gmail.com>
> Betreff: AW: [PATCH v5] git gui: add directly calling merge tool from
> configuration
>
>
>
> > -----Ursprüngliche Nachricht-----
> > Von: Johannes Sixt <j6t@kdbg.org>
> > Gesendet: Samstag, 14. September 2024 15:33
> > An: Boesch, Tobias <tobias.boesch@miele.com>
> > Cc: git@vger.kernel.org; ToBoMi via GitGitGadget
> > <gitgitgadget@gmail.com>
> > Betreff: Re: [PATCH v5] git gui: add directly calling merge tool from
> > configuration
> >
> > Am 12.09.24 um 12:17 schrieb ToBoMi via GitGitGadget:
> > > Configuration example:
> > > [merge]
> > >   tool = vscode
> > > [mergetool "vscode"]
> > >   path = the/path/to/Code.exe
> > >   cmd = \"Code.exe\" --wait --merge \"$LOCAL\" \"$REMOTE\"
> > \"$BASE\" \"$MERGED\"
> >
> > This example is not up-to-date anymore, is it?
> >
> > Also, below are two cases where "mergetool.cmd" is mentioned incorrectly.
> >
> > > Without the "mergetool.cmd" configuration and an unsupported
> > "merge.tool"
> > > entry, git gui behaves mainly as before this change and informs the
> > > user about an unsupported merge tool. In addtition it also shows a
> > > hint to add a configuration entry to use the tool as an unsupported
> > > tool with degraded support.
> > >
> > > If a wrong "mergetool.cmd" is configured by accident, it gets
> > > handled by git gui already. In this case git gui informs the user
> > > that the merge tool couldn't be opened. This behavior is preserved
> > > by this change and should not change.
> >
> > > --- a/git-gui/lib/mergetool.tcl
> > > +++ b/git-gui/lib/mergetool.tcl
> > > @@ -272,8 +272,26 @@ proc merge_resolve_tool2 {} {
> > >           }
> > >   }
> > >   default {
> > > -         error_popup [mc "Unsupported merge tool '%s'" $tool]
> > > -         return
> > > +         set tool_cmd [get_config mergetool.$tool.cmd]
> > > +         if {$tool_cmd ne {}} {
> > > +                 if {([string first {[} $tool_cmd] != -1) || ([string first {]}
> > $tool_cmd] != -1)} {
> > > +                         error_popup [mc "Unable to process square
> > brackets in mergetool.$tool.cmd configuration option.
> >
> > This $tool in the format string breaks [mc]. It must be %s and an
> > argument. I'll fix this up while queuing.
> >
> > > +
> > > +Please remove the square brackets."]
> > > +                         return
> > > +                 } else {
> > > +                         set cmdline {}
> > > +                         foreach command_part $tool_cmd {
> > > +                                 lappend cmdline [subst -nobackslashes
> > -nocommands $command_part]
> > > +                         }
> > > +                 }
> > > +         } else {
> > > +                 error_popup [mc "Unsupported merge tool '%s'.
> > > +
> > > +To use this tool, configure \"mergetool.%s.cmd\" as shown in the
> > > +git-config\> +manual page." $tool $tool]
> >
> > I am surprised that the backslash does not paste the two lines
> > together without a space. "git-config" and "manual" do appear as
> > separate words in the error message. Nevertheless, since I do not know
> > how this pans out in the translation files, I will remove the line continuation
> and write all on one line.
> >
>
> True I also don't know why.
> You could add a whitespace after the newline and have code matching the
> documentation of tcl:
>
> "\<newline>whiteSpace
> A single space character replaces the backslash, newline, and all spaces and
> tabs after the newline. [...]"
> From https://www.tcl.tk/man/tcl8.7/TclCmd/Tcl.html#M24
>
> That doesn't change the error message (stays good) in my tests and makes the
> code compliant to the tcl docs.
>
> > > +                 return
> > > +         }
> > >   }
> > >   }
> >
> > Thank you for your contribution! Below is the range-diff between this
> > submission and the queued version.
> >
>
> Thank you for fixing the issues left open and your patient review.
> (Based on your comments and if there is no further notice - I assume that this
> patch will be processed by your side without further submissions from my
> side)

>
> > -- Hannes
> >
> > 1:  03e92d6 ! 1:  8ff65c7 git gui: add directly calling merge tool
> > from configuration
> >     @@ Commit message
> >          [merge]
> >                  tool = vscode
> >          [mergetool "vscode"]
> >     -            path = the/path/to/Code.exe
> >     -            cmd = \"Code.exe\" --wait --merge \"$LOCAL\" \"$REMOTE\"
> > \"$BASE\" \"$MERGED\"
> >     +            cmd = \"the/path/to/Code.exe\" --wait --merge \"$LOCAL\"
> > \"$REMOTE\" \"$BASE\" \"$MERGED\"
> >
> >     -    Without the "mergetool.cmd" configuration and an unsupported
> > "merge.tool"
> >     -    entry, git gui behaves mainly as before this change and informs the user
> >     -    about an unsupported merge tool. In addtition it also shows a hint to
> add
> >     -    a configuration entry to use the tool as an unsupported tool with
> > degraded
> >     -    support.
> >     +    Without the "mergetool.<mergetool name>.cmd" entry and an
> > unsupported
> >     +    "merge.tool" entry, git gui behaves mainly as before this change and
> >     +    informs the user about an unsupported merge tool. In addtition, it also
> >     +    shows a hint to add a configuration entry to use the tool as an
> >     +    unsupported tool with degraded support.
> >
> >     -    If a wrong "mergetool.cmd" is configured by accident, it gets handled
> >     -    by git gui already. In this case git gui informs the user that the merge
> >     -    tool couldn't be opened. This behavior is preserved by this change and
> >     -    should not change.
> >     +    If a wrong "mergetool.<mergetool name>.cmd" is configured by
> > accident,
> >     +    it gets handled by git gui already. In this case git gui informs the
> >     +    user that the merge tool couldn't be opened. This behavior is preserved
> >     +    by this change and should not change.
> >
> >          "Beyond Compare 3" and "Visual Studio Code" were tested as manually
> >          configured merge tools.
> >
> >          Signed-off-by: Tobias Boesch <tobias.boesch@miele.com>
> >     +    Signed-off-by: Johannes Sixt <j6t@kdbg.org>
> >
> >       ## lib/mergetool.tcl ##
> >      @@ lib/mergetool.tcl: proc merge_resolve_tool2 {} {
> >     @@ lib/mergetool.tcl: proc merge_resolve_tool2 {} {
> >      +              set tool_cmd [get_config mergetool.$tool.cmd]
> >      +              if {$tool_cmd ne {}} {
> >      +                      if {([string first {[} $tool_cmd] != -1) || ([string first {]}
> > $tool_cmd] != -1)} {
> >     -+                              error_popup [mc "Unable to process square
> > brackets in mergetool.$tool.cmd configuration option.
> >     ++                              error_popup [mc "Unable to process square
> > brackets in \"mergetool.%s.cmd\" configuration option.
> >      +
> >     -+Please remove the square brackets."]
> >     ++Please remove the square brackets." $tool]
> >      +                              return
> >      +                      } else {
> >      +                              set cmdline {}
> >     @@ lib/mergetool.tcl: proc merge_resolve_tool2 {} {
> >      +              } else {
> >      +                      error_popup [mc "Unsupported merge tool '%s'.
> >      +
> >     -+To use this tool, configure \"mergetool.%s.cmd\" as shown in the
> > git- config\
> >     -+manual page." $tool $tool]
> >     ++To use this tool, configure \"mergetool.%s.cmd\" as shown in the
> > git- config manual page." $tool $tool]
> >      +                      return
> >      +              }
> >             }