From: Junio C Hamano <gitster@pobox•com>
To: Samuel Bronson <naesten@gmail•com>
Cc: git@vger•kernel.org, Jonathan Nieder <jrnieder@gmail•com>,
Anders Waldenborg <anders@0x63•nu>
Subject: Re: [PATCH v2] diff: Add diff.orderfile configuration variable
Date: Mon, 09 Dec 2013 11:23:05 -0800 [thread overview]
Message-ID: <xmqqiouxzv52.fsf@gitster.dls.corp.google.com> (raw)
In-Reply-To: <CAJYzjmdg8v6-kZ+xtD9GT=vVTs7AEX_iEoroxdi4F4rjoTogWw@mail.gmail.com> (Samuel Bronson's message of "Fri, 6 Dec 2013 21:43:22 -0500")
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?
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.
>> 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.
next prev parent reply other threads:[~2013-12-09 19:23 UTC|newest]
Thread overview: 35+ messages / expand[flat|nested] mbox.gz Atom feed top
2013-10-21 10:31 [PATCH] diff: Add diff.orderfile configuration variable Anders Waldenborg
2013-10-21 18:40 ` Jonathan Nieder
2013-10-25 10:24 ` Anders Waldenborg
2013-12-06 6:48 ` [PATCH v2] " Samuel Bronson
2013-12-06 18:11 ` Junio C Hamano
2013-12-07 2:43 ` Samuel Bronson
2013-12-09 19:23 ` Junio C Hamano [this message]
2013-12-14 22:18 ` [PATCH v3 0/3] " Samuel Bronson
2013-12-14 22:18 ` [PATCH v3 1/3] diff: Tests for "git diff -O" Samuel Bronson
2013-12-14 22:18 ` [PATCH v3 2/3] diff: Let "git diff -O" read orderfile from any file, failing when appropriate Samuel Bronson
2013-12-16 18:43 ` Junio C Hamano
2013-12-14 22:18 ` [RFC v3 3/3] diff: Add diff.orderfile configuration variable Samuel Bronson
2013-12-16 18:53 ` Junio C Hamano
2013-12-16 19:21 ` Samuel Bronson
2013-12-16 20:09 ` [PATCH v4 0/3] " Samuel Bronson
2013-12-16 20:09 ` [PATCH v4 1/3] diff: Tests for "git diff -O" Samuel Bronson
2013-12-16 20:09 ` [PATCH v4 2/3] diff: Let "git diff -O" read orderfile from any file, fail properly Samuel Bronson
2013-12-16 21:09 ` Junio C Hamano
2013-12-17 4:06 ` Samuel Bronson
2013-12-16 21:32 ` Junio C Hamano
2013-12-17 5:03 ` Samuel Bronson
2013-12-17 17:54 ` Junio C Hamano
2013-12-17 20:37 ` Antoine Pelisse
2013-12-17 22:09 ` Junio C Hamano
2013-12-18 4:28 ` Samuel Bronson
2013-12-18 5:47 ` Junio C Hamano
2013-12-17 23:11 ` Junio C Hamano
2013-12-16 20:09 ` [PATCH v4 3/3] diff: Add diff.orderfile configuration variable Samuel Bronson
2013-12-19 0:08 ` [PATCH v5 0/3] " Samuel Bronson
2013-12-19 0:40 ` Junio C Hamano
2013-12-19 0:08 ` [PATCH v5 1/3] diff: Tests for "git diff -O" Samuel Bronson
2013-12-19 0:08 ` [PATCH v5 2/3] diff: Let "git diff -O" read orderfile from any file, fail properly Samuel Bronson
2014-01-10 20:10 ` [PATCH sb/diff-orderfile-config] diff test: reading a directory as a file need not error out Jonathan Nieder
2014-01-10 23:30 ` Junio C Hamano
2013-12-19 0:08 ` [PATCH v5 3/3] diff: Add diff.orderfile configuration variable Samuel Bronson
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=xmqqiouxzv52.fsf@gitster.dls.corp.google.com \
--to=gitster@pobox$(echo .)com \
--cc=anders@0x63$(echo .)nu \
--cc=git@vger$(echo .)kernel.org \
--cc=jrnieder@gmail$(echo .)com \
--cc=naesten@gmail$(echo .)com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox