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

Re: [PATCH 0/9 v2] difftool: teach command to perform directory diffs

From
Tim Henigan <tim.henigan@gmail.com>
Date
Mar 20, 2012, 13:07 UTC
Message-ID
<CAFouetj2gME+X47EUAQTUKX_GJ8PW=cSWRj7VUdBsi91Aa9Wyw@mail.gmail.com>
In-Reply-To
<7vhaxk9ui6.fsf@alter.siamese.dyndns.org>
I have successfully tested on:
  - Linux with Git v1.7.10-rc0 and 1
  - Windows 7 with msysgit v1.7.9

I installed cygwin Git on Windows 7, but found that 'git difftool' would not work, even before my patches were applied. It appears that I need to learn more about cygwin before I can use it as a test platform.

On Mon, Mar 19, 2012 at 5:00 PM, Junio C Hamano <gitster@pobox.com> wrote:
Show 6 quoted lines
>
>  * In [1/9], use of pass_through would mean 'git difftool' must be fed the
>   options for difftool and then options meant for underlying diff
>   machinery.  Is this limitation still there?  If so, isn't this a
>   regression?  Shouldn't it at least be advertised to the users a lot
>   stronger in documentation?

Both the documentation and testing has shown that 'pass_through' works as I expected. That is, options that difftool cares about are extracted from @ARGV while all others are passed on to the underlying 'git diff' command. The order of the arguments/options does not matter. We "use 5.008;" at the top of the script, so I believe this change is correct and safe. That being said, I have a limited number of computers/configurations to test on.

>  * In [4/9], you remove the .exe extension when running git, which was
>   there since the beginning of difftool 5c38ea3 (contrib: add 'git
>   difftool' for launching common merge tools, 2009-01-16).  I think it is
>   safe but are Windows folks OK with this?
My testing on Windows with msysgit has shown that this change is safe.
 Also, even though I had other problems when testing on cygwin (noted
at the top of the email), the 'difftool' script was able to
successfully execute 'git diff --raw -z' without appending '.exe'.
Show 5 quoted lines
>  * In [6/9], the exit code in the failure case seem to be modified from
>   that of the underlying "git diff" to whatever croak gives us.  Is this
>   a regression, or nobody cares error status from difftool?  I personally
>   suspect it is the latter, but just double-checking if you have
>   considered it.

In my testing, if "Git::command_noisy(('diff', @ARGV))" fails, the exit code from 'git diff' is passed back to the terminal. So not only is the 'git diff' exit code echoed to the terminal, but '$?' also contains the correct exit code.

>  * In [7/9], difftool--helper declares SUBDIRECTORY_OK, but there doesn't
>   seem to be any inclusion of git-sh-setup in this script, and the patch
>   does not have any effort to prepend $prefix to paths relative to $cwd.
>   What good does the variable do here?

This was an oversight. It was copied over from a previous implementation that still used ". git-sh-setup".

So the only outstanding changes that I am planning are:
  1) Remove SUBDIRECTORY_OK
  2) Add tests for the new '--dir-diff' option

Would it be better to resend the complete patches series with these changes or just add new patches to the end?

Please let me know if I missed anything else.
Previous: Junio C HamanoNext: Junio C Hamano
Message 25 of 26 in “difftool: teach command to perform directory diffs”
  1. 0/9 difftool: teach command to perform directory diffsTim Henigan, Mar 17, 2012
  2. 1/9 difftool: parse options using Getopt::LongTim Henigan, Mar 17, 2012
  3. David AguilarMar 17, 2012
  4. Tim HeniganMar 17, 2012
  5. David AguilarMar 18, 2012
  6. 2/9 difftool: exit(0) when usage is printedTim Henigan, Mar 17, 2012
  7. 3/9 difftool: remove explicit change of PATHTim Henigan, Mar 17, 2012
  8. 4/9 difftool: stop appending '.exe' to gitTim Henigan, Mar 17, 2012
  9. Junio C HamanoMar 17, 2012
  10. Tim HeniganMar 17, 2012
  11. Junio C HamanoMar 17, 2012
  12. 0/9 difftool: teach command to perform directory diffsTim Henigan, Mar 18, 2012
  13. 1/9 difftool: parse options using Getopt::LongTim Henigan, Mar 18, 2012
  14. 2/9 difftool: exit(0) when usage is printedTim Henigan, Mar 18, 2012
  15. 3/9 difftool: remove explicit change of PATHTim Henigan, Mar 18, 2012
  16. 4/9 difftool: stop appending '.exe' to gitTim Henigan, Mar 18, 2012
  17. 5/9 difftool: eliminate setup_environment functionTim Henigan, Mar 18, 2012
  18. 6/9 difftool: replace system call with Git::command_noisyTim Henigan, Mar 18, 2012
  19. 7/9 difftool: teach difftool to handle directory diffsTim Henigan, Mar 18, 2012
  20. 8/9 difftool: teach dir-diff to copy modified files back to working treeTim Henigan, Mar 18, 2012
  21. 9/9 difftool: print list of valid tools with '--tool-help'Tim Henigan, Mar 18, 2012
  22. Junio C HamanoMar 19, 2012
  23. David AguilarMar 20, 2012
  24. Junio C HamanoMar 20, 2012
  25. Tim HeniganMar 20, 2012
  26. Junio C HamanoMar 20, 2012

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.