Re: [PATCH v4 2/3] diff: Let "git diff -O" read orderfile from any file, fail properly

From: Samuel Bronson <naesten@gmail.com>
Date: Dec 17, 2013, 05:03 UTC
Message-ID: <CAJYzjmd_EWcQ5OzuZBQwhkfAtdxbPbvhVxUSsh98SzMzyz=-8w@mail.gmail.com>
In-Reply-To: <xmqqwqj4mqhe.fsf@gitster.dls.corp.google.com>

On Mon, Dec 16, 2013 at 4:32 PM, Junio C Hamano <gitster@pobox.com> wrote:

Show 11 quoted lines

> Samuel Bronson <naesten@gmail.com> writes:
>
>>  for i in 1 2
>>  do
>>       test_expect_success "orderfile using option ($i)" '
>>       git diff -Oorder_file_$i --name-only HEAD^..HEAD >actual &&
>>       test_cmp expect_$i actual
>>  '
>
> This funny indentation in the previous step needs to be fixed, and
> the added block below should match.

Even though this results in oddly-indented --verbose output?

Show 11 quoted lines

>> +     rm -f order_fifo &&
>> +     mkfifo order_fifo &&
>> +     cat order_file_$i >order_fifo &
>> +     git diff -O order_fifo --name-only HEAD^..HEAD >actual &&
>
> I think this part can be racy depending on which between cat and
> "git diff" are scheduled first, no?  Try running this test under
> load and I think you will see it deadlocked.
>
> Besides, the above breaks && chain; even if mkfifo breaks (hence not
> allowing cat to run), "git diff" will go ahead and run, no?

Hmm.  Well, what I really wanted to put here was a "process substitution":

    git diff -O <(cat order_file_$i) --name-only HEAD^..HEAD >actual &&

but I did not see this feature listed in the dash(1) manpage, so I assumed it wasn't allowed by POSIX. And, having looked, I indeed don't see it mentioned in POSIX either.

I'm not terribly surprised that I screwed up the translation to FIFOs; how would I really want to do it?