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

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

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:

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