Re: [PATCH v2] diff: Add diff.orderfile configuration variable

Samuel Bronson [off-list ref] writes:

quoted

If somebody has diff.orderfile configuration that points at a custom
ordering, and wants to send out a patch (or show a diff) with the
standard order, how would the "overriding" command line look like?
Would it be "git diff -O/dev/null"?

It looks like that works ... and so do files that don't exist.  What
do you think should happen with -O file-that-does-not-exist, and how
do you suppose it should be tested?

I think the original code is too loose on diagnosing errors.
Perhaps something like this is needed.

diff --git a/diffcore-order.c b/diffcore-order.c
index 23e9385..dd103e3 100644
--- a/diffcore-order.c
+++ b/diffcore-order.c

@@ -20,8 +20,11 @@ static void prepare_order(const char *orderfile)
 		return;
 
 	fd = open(orderfile, O_RDONLY);
-	if (fd < 0)
+	if (fd < 0) {
+		if (errno != ENOENT || errno != ENOTDIR)
+			die(_("orderfile '%s' does not exist."));
 		return;
+	}
 	if (fstat(fd, &st)) {
 		close(fd);
 		return;

After having fixed this, will /dev/null still work everywhere, or will
we want a new diff flag to unset the option?  (I see that "git diff
/dev/null some-file" works fine with msysgit, which doesn't seem to
actually be linked with MSYS, but I don't know *why* it works, and I
don't know what other non-POSIXoid ports exist.)

I *think* it should be OK to use "-O/dev/null" for that purpose, but
the primary thing I was hinting at with the rhetoric question was
that it probably needs to be documented there.

Also, I'm starting to wonder if I shouldn't split this into two patches:

    1.  diff: Add tests for -O flag
    2.  diff: Add diff.orderfile configuration variable

(If so, I would obviously want to rewrite the above test to avoid the
configuration option.)

Surely, and thanks.

quoted

                EOF
                cat >order_file_2 <<-\EOF &&

I'd kind of prefer to keep a blank line between one EOF and the next
cat, if that's okay with you.

Alright.  Making it easier to spot the grouping, it would make it
easier to read.

quoted

Quoting the EOF like the above will help the readers by signaling
them that they do not have to wonder if there is some substitution
going on in the here text.

Perhaps, but probably only after they've scrutinized their shell
manuals to figure out what the - and the \ are for.  (I had to check
two: dash(1) wasn't clear enough for me about the quoting ...)

Yes and no.  The no primarily comes from that nobody will stay to be
novice forever.