Summary

Improve our usage of --no-ext-diff with git.

Review Request #5372 — Created Jan. 31, 2014 and submitted Feb. 2, 2014, 11:31 p.m.

Information

Owner

david

Repository

RBTools

Branch

master

Bugs

1524

Depends On

Reviewers

Groups

rbtools

People

Description

Improve our usage of --no-ext-diff with git.

At some point in the past, we added --no-ext-diff to the git command line. This
was intended to make things work when people had set their diff tool to
something that didn't produce a diff, such as graphical tools like 'p4merge'.
Unfortunately, this removes a bunch of functionality with legitimate uses.

This change adds a flag for .reviewboardrc, GIT_USE_EXT_DIFF, which can be set
to True to allow using external diff commands.

Testing Done

Posted some changes with git using --debug. Saw that without the config
setting, it passed --no-ext-diff on the command lin, and with it, it didn't.

Issues

Description	From	Last Updated
You could probably simplify this with: if self.user_config.get('GIT_USE_EXT_DIFF', False):	chipx86	Feb. 2, 2014, 11:25 p.m.

What happens when a graphical diff tool (say, Kaleidoscope) is used? Won't it launch the process?

david Jan. 31, 2014, 7:21 p.m.

It will launch it, and there's not really anything we can do about that. The nice thing about this is that once they quit that, it'll go through and post it using --no-ext-diff.

chipx86 Feb. 1, 2014, 5:28 p.m.

Hmm, that seems like an annoying regression. Though, they should be using git difftool for such things.

Forcing a standard git diff is right for most situations, but clearly not all (the example of a filtering script in the bug is one such example).

Instead of trying both with and without, and possibly causing problems (showing a graphical app, running through less, etc.), what are your thoughts on keeping --no-ext-diff by default, and having a .reviewboardrc option for turning this off?

Description:

		Improve our usage of --no-ext-diff with git.

		At some point in the past, we added --no-ext-diff to the git command line. This
		was intended to make things work when people had set their diff tool to
		something that didn't produce a diff, such as graphical tools like 'p4merge'.
		Unfortunately, this removes a bunch of functionality with legitimate uses.

~		This change fixes the diff process to try first with the user's configured diff
~		tool. If that fails, we'll try again with --no-ext-diff.
	~	This change adds a flag for .reviewboardrc, GIT_USE_EXT_DIFF, which can be set
	~	to True to allow using external diff commands.

Testing Done:

~		Posted some changes with git.
	~	Posted some changes with git using --debug. Saw that without the config
	+	setting, it passed --no-ext-diff on the command lin, and with it, it didn't.

Diff:

Revision 2 (+26 -18)

Show changes

rbtools/clients/git.py

Ship it!

```
Looks good. One suggestion.
```

rbtools/clients/git.py (Diff revision 2)

The issue has been resolved. Show all issues

You could probably simplify this with:

if self.user_config.get('GIT_USE_EXT_DIFF', False):

Status: Closed (submitted)

Change Summary:

Pushed to master (7160581).