Thanks.
I do not know a difftool user, and I wasn't paying close attention to the
discussion but I recall these points raised and I do not recall the
resolutions:
* 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?
* 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?
* 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 [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?
On Mon, Mar 19, 2012 at 2:00 PM, Junio C Hamano [off-list ref] wrote:
Thanks.
I do not know a difftool user, and I wasn't paying close attention to the
discussion but I recall these points raised and I do not recall the
resolutions:
* 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?
Tim asserted that this is not the case. The options should pass
through. Hopefully there aren't any behavior changes between perl
versions and this option.
I sent a patch adding a test case to cover this scenario. I would
prefer that we avoid a regression. If there's a regression then we
can do without Getopt::Long, IMO.
I believe users should be oblivious as to which options are for
difftool and which are for diff. E.g. `git difftool --cached -t
xxdiff` -- the user does not care that --cached is for diff and -t is
for difftool.
* 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?
I do not have Windows to test with, but this supposedly works since
Git.pm does not use git.exe either. The git.exe stuff was originally
there because difftool originally did exec('git.exe', ...). It was
later changed to use system() and it's possible that we could have
dropped the .exe extension at that time.
But, like I said, I don't have any Windows machines with which to
verify this behavior, and highly appreciate feedback from Windows
folks.
* 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.
This doesn't seem like too big of an issue. Had we documented the old
exit codes then it would be a bigger concern. I would personally
prefer to preserve the exit code from diff itself, but if that
complicates it too much then the new behavior is ok.
* 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?
I'll defer to Tim on this one. This seems like an oversight. It
seems like something should be done to handle it.
I recall that we made $GIT_PREFIX available to aliases and builtins
not too long ago. That may help here. See
1f5d271f5e8f7b1e2a5b296ff43ca4087eb08244.
Also.. I think we need some tests to cover the new behavior. A test
to cover the subdirectory behavior would be especially helpful given
the note about [7/9].
--
David
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 [off-list ref] wrote:
* 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'.
* 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.