Re: [PATCH] format-patch: do not let its diff-options affect --range-diff (was Re: [PATCH 2/2] format-patch: allow for independent diff & range-diff options)
From: Eric Sunshine <hidden>
Date: 2018-11-30 09:32:00
On Thu, Nov 29, 2018 at 11:27 PM Junio C Hamano [off-list ref] wrote:
Junio C Hamano [off-list ref] writes:quoted
In any case, I tend to agree with the conclusion in the downthread by Dscho that we should just clearly mark that invocations of the "format-patch --range-diff" command with additional diff options is an experimental feature that may not do anything sensible in the upcoming release, and declare that the UI to pass diff options to affect only the range-diff part may later be invented. IOW, I am coming a bit stronger than Dscho's suggestion in that we should not even pretend that we aimed to make the options used for range-diff customizable when driven from format-patch in the upcoming release, or aimed to make --range-diff option compatible with other diff options given to the format-patch command.
I agree that this way forward makes sense. It's clear that I overlooked how there could be unexpected interactions from passing git-format-patch's own diff_options to show_range_diff(), so taking time to think it through without the pressure of a looming release is preferable to rushing out some "fixes".
So how about doing this on top of 'master' instead? As this leaks *no* information wrt how range-diff machinery should behave from the format-patch side by not passing any diffopt, as long as the new code I added to show_range_diff() comes up with a reasonable default diffopts (for which I really would appreciate extra sets of eyes to make sure), this change by definition cannot be wrong (famous last words).
I, myself, was going to suggest this approach of leaking none of the git-format-patch's options into range_diff(), so I think it is a good one. Later, we can selectively pass certain _sensible_ options into show_range_diff() once we figure out the correct UI (for instance, --range-diff-opts=nopatch,creation-factor:60). A couple comments on the patch itself...
quoted hunk ↗ jump to hunk
diff --git a/range-diff.c b/range-diff.c@@ -460,7 +460,11 @@ int show_range_diff(const char *range1, const char *range2, - memcpy(&opts, diffopt, sizeof(opts)); + if (diffopt) + memcpy(&opts, diffopt, sizeof(opts)); + else + repo_diff_setup(the_repository, &opts);
The first attempt at adding --range-diff to git-format-patch invoked the git-range-diff command, so no diff_options were passed at all. After Dscho libified the range-diff machinery in one of his major re-rolls, I took advantage of that to avoid the subprocess invocation. Another benefit of calling show_range_diff() directly is that when "git format-patch --stdout --range-diff=..." is sent to the terminal, the range-diff gets colored output for free. I'm pleased to see that that still works after this change.
quoted hunk ↗ jump to hunk
diff --git a/range-diff.h b/range-diff.h@@ -5,6 +5,11 @@ +/* + * Compare series of commmits in RANGE1 and RANGE2, and emit to the + * standard output. NULL can be passed to DIFFOPT to use the built-in + * default. + */
It is more correct to say that the range-diff is emitted to diffopt->file (which may be stdout). Thanks for working on this.