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 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html