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

2 messages, 2 authors, 2016-06-15 · open the first message on its own page

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

From: Junio C Hamano <hidden>
Date: 2016-06-15 22:59:28

Samuel Bronson [off-list ref] writes:
On Mon, Dec 16, 2013 at 4:32 PM, Junio C Hamano [off-list ref] wrote:
quoted
Samuel Bronson [off-list ref] writes:
quoted
 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?
quoted
quoted
+     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?
How about not doing a fifo?

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

From: Antoine Pelisse <hidden>
Date: 2016-06-15 22:59:28

On Tue, Dec 17, 2013 at 6:54 PM, Junio C Hamano [off-list ref] wrote:
Samuel Bronson [off-list ref] writes:
quoted
On Mon, Dec 16, 2013 at 4:32 PM, Junio C Hamano [off-list ref] wrote:
quoted
Samuel Bronson [off-list ref] writes:
quoted
 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?
quoted
quoted
+     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?
How about not doing a fifo?
That would certainly defeat the purpose of the test, which is to test
against a fifo :-)
I'm not sure about the deadlock though. Both read and write will wait
for each other to start operating on the fifo.

You can probably fix the &&-chain by doing something like:

    mkfifo order_fifo && {
        cat order_file_$i >order_fifo &
        git diff -O order_fifo --name-only HEAD^..HEAD >actual
    } && ...

Also, "rm -f order_fifo" should probably be done in test_when_finished
rather than at the beginning of the test.

Antoine,
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help