1524: postreview.py should ignore user configuration when generating diffs.

emil****@gmai***** (Google Code) (Is this you? Claim this profile.)
david
 david
Feb. 3, 2014 
What version are you running?

postreview 0.8.

What steps will reproduce the problem?
1. git config diff.external /bin/does-not-exist
2. postreview.py

What is the expected output? What do you see instead?

Expect that a normal diff should be produced and uploaded.
However,

$ postreview.py -d --guess-description --guess-summary
>>> svn info
>>> git rev-parse --git-dir
>>> git symbolic-ref -q HEAD
>>> git svn info
>>> git svn --version
>>> git config --get svn-remote.svn.url
>>> git config --get branch.for-review.merge
>>> git config --get branch.for-review.remote
>>> git config remote.origin.url
>>> repository info: Path: git://internal/test.git, Base path: , Supports 
changesets: False
>>> git config --get reviewboard.url
>>> git merge-base origin/master refs/heads/for-review
>>> git diff --no-color --full-index 
b79bcb4c0c838df4406c01c59370977a53da5425..refs/heads/for-review
Failed to execute command: ['git', 'diff', '--no-color', '--full-index', 
'b79bcb4c0c838df4406c01c59370977a53da5425..refs/heads/for-review']
error: cannot run /bin/does-not-exist: No such file or directory
external diff died, stopping at test/a.txt.

[This log was manually edited diff; real results may vary.]

What operating system are you using? What browser?

Linux.

Please provide any additional information below.

Probably the best solution is to set GIT_CONFIG_NOSYSTEM and 
GIT_CONFIG_NOGLOBAL to insulate usage of git from any system/user/repo-
specific settings.
chipx86
#1 chipx86
  • +Confirmed
  • +Milestone-RBTools-Release1.0
    +Component-RBTools
#2 alex.*****@gmai***** (Google Code) (Is this you? Claim this profile.) 
Easy enough to fix by using "--no-ext-diff", attaching a patch that fixes it for me at 
least.
  • +
    postreview.py.patch 
    --- postreview.py	Fri May 28 10:44:57 2010
    +++ postreview.py	Fri May 28 10:29:25 2010
    @@ -2232,7 +2232,7 @@
                                      split_lines=True)
                 return self.make_svn_diff(ancestor, diff_lines)
             elif self.type == "git":
    -            return execute(["git", "diff", "--no-color", "--full-index",
    +            return execute(["git", "diff", "--no-color", "--full-index", "--no-ext-diff",
                                 rev_range])
     
             return None
david
#3 david 
Fixed in rbtools 78791b9. Thanks!
  • -Confirmed
    +Fixed
#4 vzei****@gmai***** (Google Code) (Is this you? Claim this profile.) 
Sorry but could the decision to add "--no-ext-diff" be please reconsidered?

I don't know what is the reasoning behind explicitly ignoring the user options but IMHO the answer to "RB can't run the non-existent program I configured Git to use" is simply "Don't configure Git to use non-existent programs", not this.

And I can definitely explain why this is a problem for me: I'm using a hack to disable diffs to some "not interesting" files by default, i.e. I have "foobar diff=generated" in .gitattributes and used "git config diff.generated.command true" to just suppress the diff for all files with this attribute. Unfortunately, when I use post-review, the diffs for all these files do get submitted, which is very annoying. And AFAICS there is no way to avoid it.

So could the patch be either simply reverted or, if it's really needed, be at least made optional? In the latter case, please let me know if you'd accept a patch adding an option governing this behaviour.

TIA!
#5 vzei****@gmai***** (Google Code) (Is this you? Claim this profile.) 
It would be really great to get any reply about this because it's annoying to have to change this in every new installation of RBTools. Especially because I still think there is absolutely no good reason for using --no-ext-diff in the first place, so it's a pity that comment #4 apparently went completely unnoticed.

Of course, it's also possible that I'm missing something here, but in this case I'd be really grateful if somebody could please point out what is it.
david
#6 david 
I'm sorry, we missed the earlier comment.
  • -Fixed
    +New
chipx86
#7 chipx86 
I don't know the original rationale for why --no-ext-diff was needed, but I think I agree. We should probably revert this change. In the case where someone needs that option set, I believe they can set it locally for their git install.
#8 emil****@gmai***** (Google Code) (Is this you? Claim this profile.) 
If I recall correctly, the original issue I saw is that someone can configure external diff to run sort of graphical program that does not actually generate a diff to stdout. If I had to guess, probably someone I was helping had their external diff set to p4merge and couldn't figure out why their upload wasn't working.

(As an aside, it seems that there is some debate as to whether one ought to properly use git difftool for that purpose or git with an external diff tool. Both seem to be possible. But the fact remains that with external diff, there isn't a guarantee that what you get on stdout is something RB will be able to understand once it has been uploaded.)

Perhaps RB could try without no-ext-diff but if the upload fails fallback to no no-ext-diff before giving up?
david
#9 david
  • -Milestone-RBTools-Release1.0
david
#10 david
  • -New
    +PendingReview
  • +david
david
#11 david 
Fixed in rbtools master (7160581).

If you set GIT_USE_EXT_DIFF = True in ~/.reviewboardrc, rbtools will not use --no-ext-diff.
  • -PendingReview
    +Fixed