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

From

Junio C Hamano <gitster@pobox.com>

Date

Dec 9, 2013, 19:23 UTC

Message-ID

<xmqqiouxzv52.fsf@gitster.dls.corp.google.com>

In-Reply-To

<CAJYzjmdg8v6-kZ+xtD9GT=vVTs7AEX_iEoroxdi4F4rjoTogWw@mail.gmail.com>

Samuel Bronson <naesten@gmail.com> writes:

>> 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?

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.

>>                 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.

>> 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.