es/format-patch-{inter,range}diff, was Re: What's cooking in git.git (Aug 2018, #06; Wed, 29)

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Aug 30, 2018, 14:01 UTC
Message-ID: <nycvar.QRO.7.76.6.1808301548560.71@tvgsbejvaqbjf.bet>
In-Reply-To: <xmqqbm9kajhu.fsf@gitster-ct.c.googlers.com>

Hi Junio,

On Wed, 29 Aug 2018, Junio C Hamano wrote:

Show 15 quoted lines

> * es/format-patch-interdiff (2018-07-23) 6 commits
>  - format-patch: allow --interdiff to apply to a lone-patch
>  - log-tree: show_log: make commentary block delimiting reusable
>  - interdiff: teach show_interdiff() to indent interdiff
>  - format-patch: teach --interdiff to respect -v/--reroll-count
>  - format-patch: add --interdiff option to embed diff in cover letter
>  - format-patch: allow additional generated content in make_cover_letter()
>  (this branch is used by es/format-patch-rangediff.)
> 
>  "git format-patch" learned a new "--interdiff" option to explain
>  the difference between this version and the previous atttempt in
>  the cover letter (or after the tree-dashes as a comment).
> 
>  What's the doneness of this one?
>  cf. <CAPig+cSuYUYSPTuKx08wcmQM-G12_-W2T4BS07fA=6grM1b8Gw@mail.gmail.com>

I looked over the changes online, and I think they are good.

The only slightly iffy thing I found was using the function parameter `rerolled` as printf-style format in https://github.com/gitgitgadget/git/compare/es/format-patch-interdiff~6...es/format-patch-interdiff#diff-71d4a6bddc3e479f9abf11900878a0b2R1430:

	static const char *diff_title(struct strbuf *sb, int reroll_count,
			       const char *generic, const char *rerolled)
	{
		if (reroll_count <= 0)
			strbuf_addstr(sb, generic);
		else /* RFC may be v0, so allow -v1 to diff against v0 */
			strbuf_addf(sb, rerolled, reroll_count - 1);
		return sb->buf;
	}

I guess that's okay, though. (I would have done it differently, but that would have meant playing sentence lego with the "Interdiff against v%d:" string.)

Show 18 quoted lines

> * es/format-patch-rangediff (2018-08-14) 10 commits
>  - format-patch: allow --range-diff to apply to a lone-patch
>  - format-patch: add --creation-factor tweak for --range-diff
>  - format-patch: teach --range-diff to respect -v/--reroll-count
>  - format-patch: extend --range-diff to accept revision range
>  - format-patch: add --range-diff option to embed diff in cover letter
>  - range-diff: relieve callers of low-level configuration burden
>  - range-diff: publish default creation factor
>  - range-diff: respect diff_option.file rather than assuming 'stdout'
>  - Merge branch 'es/format-patch-interdiff' into es/format-patch-rangediff
>  - Merge branch 'js/range-diff' into es/format-patch-rangediff
>  (this branch uses es/format-patch-interdiff.)
> 
>  "git format-patch" learned a new "--range-diff" option to explain
>  the difference between this version and the previous atttempt in
>  the cover letter (or after the tree-dashes as a comment).
> 
>  What's the doneness of this one?

I just had a look at the diff online, and I think this is ready for next.

Personally, I would have put the infer_range_diff_ranges() function (https://github.com/gitgitgadget/git/compare/es/format-patch-rangediff~8...es/format-patch-rangediff#diff-71d4a6bddc3e479f9abf11900878a0b2R1448) into `range-diff.c`, but it is too minor a thing to ask for a new patch series iteration.

It also looks slightly murky to me that `show_range_diff()` is now using a copy of the `diffopts` (see https://github.com/gitgitgadget/git/compare/es/format-patch-rangediff~8...es/format-patch-rangediff#diff-ab6f5eca48b8e84edf999acbe3fe7553R435), but I have no idea how to do this in a more elegant manner, either.

In short: from my point of view, both topics are ready for `next`.

Ciao, Dscho