Re: [PATCH 1/2] Fix "git diff" setup code

Linus Torvalds [off-list ref] writes:

For some inexplicable reason, "git diff" would call "diff_setup_done()" 
iff we hadn't given an explicit output format.

That makes no sense, since much of what diff_setup_done() does is exactly 
about checking the output format!

We did not even have _any_ setup_done() call in "git diff"
itself in the original, because setup_revisions() has its own
call to setup_done(), and it always called setup_revisions()
before you reached that part of the code you have in your patch.
Commit 047fbe906b375e8a3a7564ad0e4443f62dd528a2 ("builtin-diff:
turn recursive on when defaulting to --patch format.") added the
call to setup_done() only when we are defaulting to output
format, to re-validate the consistency of output format options.

The screw-up was commit fcfa33ec905fcde1c16e7cbbe00d7147b89f1f01
(diff: make more cases implicit --no-index) that made the call
to setup_revisions() conditional if setup_diff_no_index()
decides to take over, but it forgot that setup_diff_no_index()
does not call setup_done().

So I tend to think the attached is a better fix.

---
 diff-lib.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/diff-lib.c b/diff-lib.c
index f5568c3..da55713 100644
--- a/diff-lib.c
+++ b/diff-lib.c

@@ -298,6 +298,8 @@ int setup_diff_no_index(struct rev_info *revs,
 	revs->diffopt.nr_paths = 2;
 	revs->diffopt.no_index = 1;
 	revs->max_count = -2;
+	if (diff_setup_done(&revs->diffopt) < 0)
+		die("diff_setup_done failed");
 	return 0;
 }
 

On Fri, 14 Sep 2007, Junio C Hamano wrote:

So I tend to think the attached is a better fix.

Ahh, yes, that explains the conditional. 

But whatever gets us to actually verify our options, and fill in the right 
defaults is ok by me!

		Linus