[2.22.0] difftool no longer passes through to git diff if diff.tool is unset

[2.22.0] difftool no longer passes through to git diff if diff.tool is unset

[Pugh, Logan]

Note: This issue was originally discussed on this StackOverflow thread: https://stackoverflow.com/q/56675863

Prior to Git version 2.22.0 I was able to use git difftool without configuring diff.tool or merge.tool and it would show the diff using git diff.

E.g. with Git 2.21.0:

~/gits/src/git$ git difftool --no-index color.c color.h
diff --git a/color.c b/color.h
index ebb222ec33..98894d6a17 100644

In Git version 2.22.0 an error message about diff.tool not being configured is displayed:

~/gits/src/git$ git difftool --no-index color.c color.h

This message is displayed because 'diff.tool' is not configured.
See 'git difftool --tool-help' or 'git help config' for more details.
'git difftool' will now attempt to use one of the following tools:
kompare emerge vimdiff

The following commit is thought to have caused the regression: https://github.com/git/git/commit/05fb8726cccc74908853c166248ff9b6abdafae5

Please let me know if I can provide any more info.

Thanks,

Logan

Re: [2.22.0] difftool no longer passes through to git diff if diff.tool is unset

[Denton Liu]

Hi Logan,

On Wed, Jun 19, 2019 at 11:54:22PM +0000, Pugh, Logan wrote:
> Note: This issue was originally discussed on this StackOverflow thread: https://stackoverflow.com/q/56675863
> 
> Prior to Git version 2.22.0 I was able to use git difftool without configuring diff.tool or merge.tool and it would show the diff using git diff.
> 
> E.g. with Git 2.21.0:
> 
> ~/gits/src/git$ git difftool --no-index color.c color.h
> diff --git a/color.c b/color.h
> index ebb222ec33..98894d6a17 100644
> 
> In Git version 2.22.0 an error message about diff.tool not being configured is displayed:
> 
> ~/gits/src/git$ git difftool --no-index color.c color.h
> 
> This message is displayed because 'diff.tool' is not configured.
> See 'git difftool --tool-help' or 'git help config' for more details.
> 'git difftool' will now attempt to use one of the following tools:
> kompare emerge vimdiff
> 
> The following commit is thought to have caused the regression: https://github.com/git/git/commit/05fb8726cccc74908853c166248ff9b6abdafae5
> 
> Please let me know if I can provide any more info.
> 
> Thanks,
> 
> Logan

Thanks for the report. I'll take a look at this later today.

Re: [2.22.0] difftool no longer passes through to git diff if diff.tool is unset

[Denton Liu]

On Wed, Jun 19, 2019 at 09:17:44PM -0400, Denton Liu wrote:
> Hi Logan,
> 
> On Wed, Jun 19, 2019 at 11:54:22PM +0000, Pugh, Logan wrote:
> > Note: This issue was originally discussed on this StackOverflow thread: https://stackoverflow.com/q/56675863
> > 
> > Prior to Git version 2.22.0 I was able to use git difftool without configuring diff.tool or merge.tool and it would show the diff using git diff.
> > 
> > E.g. with Git 2.21.0:
> > 
> > ~/gits/src/git$ git difftool --no-index color.c color.h
> > diff --git a/color.c b/color.h
> > index ebb222ec33..98894d6a17 100644
> > 
> > In Git version 2.22.0 an error message about diff.tool not being configured is displayed:
> > 
> > ~/gits/src/git$ git difftool --no-index color.c color.h
> > 
> > This message is displayed because 'diff.tool' is not configured.
> > See 'git difftool --tool-help' or 'git help config' for more details.
> > 'git difftool' will now attempt to use one of the following tools:
> > kompare emerge vimdiff
> > 
> > The following commit is thought to have caused the regression: https://github.com/git/git/commit/05fb8726cccc74908853c166248ff9b6abdafae5
> > 
> > Please let me know if I can provide any more info.
> > 
> > Thanks,
> > 
> > Logan
> 
> Thanks for the report. I'll take a look at this later today.

Using the following command on git.git,

	$ HOME=/dev/null ./git --exec-path=. difftool --no-index color.c color.h

I did a quick bisect on the issue and it seems like the cause of this
bug is actually 287ab28bfa (diff: reuse diff setup for --no-index case,
2019-02-16). I'll do a bit more digging tomorrow.

(CCing Peff into this)

Re: [2.22.0] difftool no longer passes through to git diff if diff.tool is unset

[Jeff King]

On Wed, Jun 19, 2019 at 10:45:17PM -0400, Denton Liu wrote:

> Using the following command on git.git,
> 
> 	$ HOME=/dev/null ./git --exec-path=. difftool --no-index color.c color.h
> 
> I did a quick bisect on the issue and it seems like the cause of this
> bug is actually 287ab28bfa (diff: reuse diff setup for --no-index case,
> 2019-02-16). I'll do a bit more digging tomorrow.

I don't know much about how git-difftool works, but it looks like it
sets GIT_EXTERNAL_DIFF=git-difftool--helper.

Prior to 287ab28bfa, we would not have respected any external diff
command when running git-diff. But after it, we do.

In the case that he user has not provided --no-index, then this all
works as I guess difftool is meant to: it runs the helper and says "hey,
you have not configured this".

It seems like the behavior of the above command prior to 287ab28bfa was
not intentional. It would run git-diff, expecting it to trigger the
helper, but it never did (and instead just did a normal no-index diff).

So it seems like the new behavior is actually the right thing, as it
makes the --no-index case consistent with the regular one? I'm not
at all clear why you would run "difftool" here if you it is not
configured and you just want the straight diff output.

-Peff

Re: [2.22.0] difftool no longer passes through to git diff if diff.tool is unset

[Pugh, Logan]

> On Wed, Jun 19, 2019 at 10:45:17PM -0400, Denton Liu wrote:
> 
>> Using the following command on git.git,
>> 
>> 	$ HOME=/dev/null ./git --exec-path=. difftool --no-index color.c color.h
>> 
>> I did a quick bisect on the issue and it seems like the cause of this
>> bug is actually 287ab28bfa (diff: reuse diff setup for --no-index case,
>> 2019-02-16). I'll do a bit more digging tomorrow.
> 
> I don't know much about how git-difftool works, but it looks like it
> sets GIT_EXTERNAL_DIFF=git-difftool--helper.
> 
> Prior to 287ab28bfa, we would not have respected any external diff
> command when running git-diff. But after it, we do.
> 
> In the case that he user has not provided --no-index, then this all
> works as I guess difftool is meant to: it runs the helper and says "hey,
> you have not configured this".
> 
> It seems like the behavior of the above command prior to 287ab28bfa was
> not intentional. It would run git-diff, expecting it to trigger the
> helper, but it never did (and instead just did a normal no-index diff).
> 
> So it seems like the new behavior is actually the right thing, as it
> makes the --no-index case consistent with the regular one? I'm not
> at all clear why you would run "difftool" here if you it is not
> configured and you just want the straight diff output.
> 
> -Peff


Hi Peff,

Thanks for the explanation. It sounds like I was under the incorrect 
assumption that I could use the difftool command the same way as the 
diff command. Part of my confusion could be blamed on the git-difftool 
documentation (https://git-scm.com/docs/git-difftool) which near the top 
states:

 > git difftool is a frontend to git diff and accepts the same options 
and arguments. See git-diff[1].

My use case is a CLI program I've written that processes and then 
compares two arbitrary files using the git difftool apparatus as 
configured by the end user, leaving the choice to them whether to use 
the internal diff tool or an external tool.

Now, if I'm understanding correctly, I should not rely on the behavior 
of git difftool --no-index passing through to git diff. I could add 
another CLI switch and code path to my program that calls git diff 
directly instead of git difftool but the passthrough behavior seemed 
more elegant at the time.

Ideally, in my mind, git difftool should work as it says on the tin, as 
a straight up passthrough to git diff *unless* explicitly configured to 
use external tools (e.g. diff.tool and diff.guitool).

Hopefully that makes sense, please let me know if I am off-base.

Thanks,

Logan

Re: [2.22.0] difftool no longer passes through to git diff if diff.tool is unset

[Jeff King]

On Thu, Jun 20, 2019 at 07:29:36PM +0000, Pugh, Logan wrote:

> Thanks for the explanation. It sounds like I was under the incorrect 
> assumption that I could use the difftool command the same way as the 
> diff command. Part of my confusion could be blamed on the git-difftool 
> documentation (https://git-scm.com/docs/git-difftool) which near the top 
> states:

Well, it _is_ true that you can use it the same way. It's just that you
need to configure it to use whatever 3rd-party tool you want (and if you
do not want to configure a tool, then you are better off just using
git-diff directly). It was only due to a bug/historical accident that it
behaved just like git-diff in the no-index case (but not in the regular
case -- AFAICT, that would have been broken for your script always).

> My use case is a CLI program I've written that processes and then 
> compares two arbitrary files using the git difftool apparatus as 
> configured by the end user, leaving the choice to them whether to use 
> the internal diff tool or an external tool.
> 
> Now, if I'm understanding correctly, I should not rely on the behavior 
> of git difftool --no-index passing through to git diff. I could add 
> another CLI switch and code path to my program that calls git diff 
> directly instead of git difftool but the passthrough behavior seemed 
> more elegant at the time.
> 
> Ideally, in my mind, git difftool should work as it says on the tin, as 
> a straight up passthrough to git diff *unless* explicitly configured to 
> use external tools (e.g. diff.tool and diff.guitool).

That does make some sense to me for your use case. But I'm worried it
would be a worse experience for people new to difftool (they run it and
scratch their heads why it does not do anything different, whereas now
they get walked through an interactive configuration).

I dunno. I do not use difftool myself, so I don't have strong opinions.

In the meantime, I think you can probably switch behavior in your script
by checking if the diff.tool config is set. It might be nice if difftool
had a better way to query that without you having to know if it's
configured. Or in your case I suppose even better would just be an
option like "--if-not-configured-just-use-regular-diff". Then it would
do what you want, without impacting users who do want the interactive
setup.

-Peff

Re: [2.22.0] difftool no longer passes through to git diff if diff.tool is unset

[Pugh, Logan]

> Well, it _is_ true that you can use it the same way. It's just that you
> need to configure it to use whatever 3rd-party tool you want (and if you
> do not want to configure a tool, then you are better off just using
> git-diff directly). It was only due to a bug/historical accident that it
> behaved just like git-diff in the no-index case (but not in the regular
> case -- AFAICT, that would have been broken for your script always).

Yes, it would seem that I had only stumbled upon the broken behavior 
because of my --no-index use case, but at least the inconsistency is fixed.

> That does make some sense to me for your use case. But I'm worried it
> would be a worse experience for people new to difftool (they run it and
> scratch their heads why it does not do anything different, whereas now
> they get walked through an interactive configuration).

That is a fair point. UX matters even for CLI programs. Prior to it 
being fixed, I myself was confused as to why I was getting a git-diff 
output when trying to use an external tool with git-difftool but hadn't 
configured it correctly. At least now there is some feedback when the 
configuration is invalid.

> In the meantime, I think you can probably switch behavior in your script
> by checking if the diff.tool config is set. It might be nice if difftool
> had a better way to query that without you having to know if it's
> configured.

What I ended up doing in my app was just requiring the user to be 
explicit (via separate arguments) about whether they wanted to use 
git-diff or git-difftool. My app also accepts additional arguments that 
get passed through to git-diff/difftool and I didn't want to have to 
check those in addition to git-config. I think that would have been 
significantly more complex to implement.

> Or in your case I suppose even better would just be an
> option like "--if-not-configured-just-use-regular-diff". Then it would
> do what you want, without impacting users who do want the interactive
> setup.

If such an option was considered I would be in favor of it. Maybe call 
it "--no-tutorial" or perhaps "--diff-fallback".

But having fixed my app, I'm content with the status quo too, now.

Thanks,

Logan

Re: [2.22.0] difftool no longer passes through to git diff if diff.tool is unset

[Jeff King]

On Tue, Jun 25, 2019 at 11:09:08PM +0000, Pugh, Logan wrote:

> > Or in your case I suppose even better would just be an
> > option like "--if-not-configured-just-use-regular-diff". Then it would
> > do what you want, without impacting users who do want the interactive
> > setup.
> 
> If such an option was considered I would be in favor of it. Maybe call 
> it "--no-tutorial" or perhaps "--diff-fallback".
> 
> But having fixed my app, I'm content with the status quo too, now.

Yeah, those are definitely better names. :)

I think we're on the same page about a good path forward, then. I don't
plan to work on this myself, but maybe it would be a good #leftoverbits
candidate for somebody wanting to get started on modifying Git.

-Peff