Keith Cascio <keith@xxxxxxxxxxx> writes:
> 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@xxxxxxxxxxx>
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.
> ---
> 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.
--
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
