git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [PATCH 4/4] git-difftool: use git-mergetool--lib for "--tool-help"

From
David Aguilar <davvid@gmail.com>
Date
Jan 25, 2013, 09:55 UTC
Message-ID
<CAJDDKr5Xd4-e6VO-iO=EmwOcg0RimJHyO4ey-dFk6aENt+qZBQ@mail.gmail.com>
In-Reply-To
<20130125091918.GV7498@serenity.lan>
On Fri, Jan 25, 2013 at 1:19 AM, John Keeping <john@keeping.me.uk> wrote:
Show 23 quoted lines
> On Thu, Jan 24, 2013 at 09:29:58PM -0800, David Aguilar wrote:
>> On Thu, Jan 24, 2013 at 11:55 AM, John Keeping <john@keeping.me.uk> wrote:
>> > The "--tool-help" option to git-difftool currently displays incorrect
>> > output since it uses the names of the files in
>> > "$GIT_EXEC_PATH/mergetools/" rather than the list of command names in
>> > git-mergetool--lib.
>> >
>> > Fix this by simply delegating the "--tool-help" argument to the
>> > show_tool_help function in git-mergetool--lib.
>>
>> Very nice.
>>
>> One thought I had was that the unified show_tool_help should
>> probably check TOOL_MODE=diff and skip over the
>> !can_diff entries.
>>
>> The current output of "git difftool --tool-help" before your
>> patches has the problem that it will list tools such as
>> "tortoisemerge" as "valid but not available" because it
>> does not differentiate between missing and !can_diff.
>
> list_merge_tool_candidates does this for us, so it should Just Work
> since we use that to generate the list of tools that we loop over.

Yup, kind of. I looked more closely and found a lot of special-cases around vim which I've eliminated in the series I just sent (which includes your patches as its base).

list_merge_tool_candidates() has a bunch of other special cases for $EDITOR, $DISPLAY, $GNOME-something and such so I think we should keep using it only for the guess_merge_tool() path.

I honestly want to remove list_merge_tool_candidates every time I read it, but I recognize that it does serve a purpose for users who have not configured anything.

In order to be useful for the documentation I think the code could be refactored a tiny bit more beyond what I've sent so far. The gathering of available tools can be split off from the reporting, and then show_tool_help() can use the gatherer. With what I sent, though, there's at least a 1:1 correspondance between the name of the scriptlets and the names accepted by --tool=<tool>, which is the first step towards doing that.

I have to check out for now but I'll keep poking at this when the weekend rolls around.

cheers,
-- 
David
Previous: John KeepingNext: John Keeping
Message 8 of 10 in “Fix "git difftool --tool-help"”
  1. 0/4 Fix "git difftool --tool-help"John Keeping, Jan 24, 2013
  2. 1/4 git-mergetool: move show_tool_help to mergetool--libJohn Keeping, Jan 24, 2013
  3. 2/4 git-mergetool: remove redundant assignmentJohn Keeping, Jan 24, 2013
  4. 3/4 git-mergetool: don't hardcode 'mergetool' in show_tool_helpJohn Keeping, Jan 24, 2013
  5. 4/4 git-difftool: use git-mergetool--lib for "--tool-help"John Keeping, Jan 24, 2013
  6. David AguilarJan 25, 2013
  7. John KeepingJan 25, 2013
  8. David AguilarJan 25, 2013
  9. John KeepingJan 25, 2013
  10. Junio C HamanoJan 24, 2013

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.