Re: [Alt. PATCH] format-patch: do not use diff UI config

From: Jeff King <peff@peff.net>
Date: Sep 9, 2010, 19:43 UTC
Message-ID: <20100909194323.GA1146@sigill.intra.peff.net>
In-Reply-To: <2a6b8c51903fd6a22606b8f592b1a2e11ea68741.1284020917.git.trast@student.ethz.ch>

On Thu, Sep 09, 2010 at 10:36:54AM +0200, Thomas Rast wrote:

Show 13 quoted lines

> format-patch read and used the diff UI config, such as diff.renames,
> diff.noprefix and diff.mnemnoicprefix.  These have a history of
> breaking rebase and patch application in general; cf. 840b3ca (rebase:
> protect against diff.renames configuration, 2008-11-10).
> 
> Instead of continually putting more options inside git-rebase to avoid
> these issues, this patch takes the stance that output from
> format-patch is intended primarily for git-am and only as a side
> effect also for human consumption.  Hence, ignore the diff UI config
> entirely when coming from format-patch.
> 
> Note that all existing calls to git_log_config except for the one in
> git_format_config use a NULL callback.

This was my first thought upon reading Oded's patch, too. We would want to cut out anything that will cause format-patch to create a patch that could not be applied. So from your list:

Show 16 quoted lines

> This is a bolder approach that just outright ignores the backwards
> compatibility complaints Junio had in 840b3ca.  Among the variables
> parsed in git_diff_ui_config, namely
> 
>   color.diff (and its legacy alias diff.color)
>   diff.renames
>   diff.autorefreshindex
>   diff.mnemonicprefix
>   diff.noprefix
>   diff.external
>   diff.wordregex
>   diff.ignoresubmodules
> 
> arguably only diff.renames (and perhaps diff.ignoresubmodules, I don't
> use them) should affect format-patch.  Everything else undermines the
> guarantee (by having a consistent format) that format-patch|am works.

I would agree that diff.renames should probably be the only thing we want to allow (because it is not about making a broken diff, but because the receiver may or may not support it, and we already know that git-rebase will handle it).

diff.external is debatable. If your external diff is producing real, applicable diffs, then it is fine to use it. I have to wonder why you would use an external diff, then. I guess because it's faster, or maybe has an algorithm that produces equivalent but easier-to-read results (e.g., patience before we had --patience)?

> So now I'm not so sure about diff.renames.  Perhaps it needs to be
> retained, but that requires a special case since we cannot move it to
> git_diff_basic_config() (which affects diff-* plumbing too).

I think it is reasonable to just move an explicit "diff.renames" check into format_patch, and then set the diff_options appropriately. It requires special case code because it _is_ a special case.

-Peff