Re: [PATCH 1/2] Fix "git diff" setup code
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Sep 14, 2007, 18:19 UTC
- Message-ID
- <7v4phxaz3o.fsf@gitster.siamese.dyndns.org>
- In-Reply-To
- <alpine.LFD.0.999.0709141014130.16478@woody.linux-foundation.org>
Linus Torvalds <torvalds@linux-foundation.org> writes:
Show 5 quoted lines
> 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; }