Re: [PATCH v1 1/3] Introduce config variable "diff.primer"

From: Junio C Hamano <gitster@pobox.com>
Date: Jan 25, 2009, 20:34 UTC
Message-ID: <7v1vurf7lq.fsf@gitster.siamese.dyndns.org>
In-Reply-To: <1232904657-31831-2-git-send-email-keith@cs.ucla.edu>

Keith Cascio <keith@cs.ucla.edu> writes:

Show 12 quoted lines

> Introduce config variable "diff.primer".
> Allows user to specify arbitrary options
> to pass to diff on every invocation,
> including internal invocations from other
> programs, e.g. git-gui.
> Introduce diff command-line options:
> --no-primer, --machine-friendly
> Protect git-format-patch, git-apply,
> git-am, git-rebase, git-gui and gitk
> from inapplicable options.
>
> Signed-off-by: Keith Cascio <keith@cs.ucla.edu>

Your Subject is good; in a shortlog output that will be taken 3 months down the road, it will still tell us what this patch was about, among 100 other patches that are about different topics.

The proposed commit log message describes what the patch does, but it does not explain what problem it solves, nor why the approach the patch takes to solve that problem is good. Lines that are too short and too dense without paragraph breaks do not help readability either.

Show 10 quoted lines

> ---
>  Documentation/config.txt       |   14 +++++++
>  Documentation/diff-options.txt |   13 ++++++
>  Makefile                       |    2 +
>  builtin-log.c                  |    1 +
>  diff.c                         |   83 +++++++++++++++++++++++++++++++++++-----
>  diff.h                         |   15 ++++++-
>  git-gui/lib/diff.tcl           |    8 +++-
>  gitk-git/gitk                  |   16 ++++----
>  8 files changed, 129 insertions(+), 23 deletions(-)

You can work around the backward incompatibility you are introducing for known users that you broke with your patch, by including updates to them, and that is what your patches to git-gui and gitk are, but that is a sure sign that the approach is flawed.

The point of lowlevel plumbing (e.g. diff-{files,index,tree}) is to give people's scripts an interface that they can rely on. It is not about giving a magic interface that all the users are somehow magically upgraded without change, when the underlying git is upgraded.

If a script X does not use "ignore whitespace" without an explicit request from the end user when it runs diff-tree internally, installing a new version of diff-tree should *NOT* magically make script X to run it with "ignore whitespace", because you do not know what the script X uses diff-tree output for and how, even when the end user sets diff.primer to get "ignore whitespace" applied to his command line invocation of "git diff" Porcelain. Imagine a case where the operation of that script X relies on seeing at least two context lines around the hunk in order to correctly parse textual diff output from "diff-index -p", and the user sets "-U1" in diff.primer --- you would break the script and it is not fair to blame the script for not explicitly passing -U3 and relying on the default.

Scriptability by definition means you do not know how scripts written by people around plumbing use the output; I do not think you can sensibly say "this should not be turned on in a machine friendly output, but this is safe to use".

I would not be opposed to an enhancement to the plumbing that the scripts can use to say "I am willing to take any option (or perhaps "these options") given to me via diff.primer". Some scripts may want to be just a pass-thru of whatever the underlying git-diff-* command outputs, and it may be a handy way to magically upgrade them to allow their invocation of lowlevel plumbing to be affected by what the end-user configured. But that magic upgrade has to be an opt/in process.

There are funny indentation to align the same variable names on two adjacent lines and such; please don't.