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

[Antoine Pelisse]

On Tue, Dec 17, 2013 at 6:54 PM, Junio C Hamano gits...@pobox.com wrote:
 Samuel Bronson naes...@gmail.com writes:

 On Mon, Dec 16, 2013 at 4:32 PM, Junio C Hamano gits...@pobox.com wrote:
 Samuel Bronson naes...@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?

 + 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,
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Samuel Bronson]

On Tue, Dec 17, 2013 at 5:09 PM, Junio C Hamano gits...@pobox.com wrote:
 My point was that I did not see much value in reading the orderfile
 data from anything but a file.  At that point, you are not testing
 the diff -O orderfile option, but if strbuf_readline() reads from
 a non-regular file.

Oh, good point, now that you state it explicitly.  I'll remove it.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Junio C Hamano]

Samuel Bronson naes...@gmail.com writes:

 On Tue, Dec 17, 2013 at 5:09 PM, Junio C Hamano gits...@pobox.com wrote:
 My point was that I did not see much value in reading the orderfile
 data from anything but a file.  At that point, you are not testing
 the diff -O orderfile option, but if strbuf_readline() reads from
 a non-regular file.

 Oh, good point, now that you state it explicitly.  I'll remove it.

Or you can study the fix-up I (tentatively) queued on top of your
series in 'pu'.  Also see $gmane/239409.

Thanks.

24331790 (FIXUP! tests, 2013-12-17)

diff --git a/t/t4056-diff-order.sh b/t/t4056-diff-order.sh
index f906dea..db0e427 100755
--- a/t/t4056-diff-order.sh
+++ b/t/t4056-diff-order.sh
@@ -22,14 +22,12 @@ test_expect_success 'setup' '
*Makefile
*.txt
*.h
-   *
EOF
 
cat order_file_2 -\EOF 
*Makefile
*.h
*.c
-   *
EOF
 
cat expect_none -\EOF 
@@ -77,27 +75,30 @@ test_expect_success 'orderfile is a directory' '
 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
-'
+   git diff -Oorder_file_$i --name-only HEAD^..HEAD actual 
+   test_cmp expect_$i actual
+   '
 
test_expect_success PIPE orderfile is fifo ($i) '
-   rm -f order_fifo 
-   mkfifo order_fifo 
-   cat order_file_$i order_fifo 
-   git diff -O order_fifo --name-only HEAD^..HEAD actual 
-   test_cmp expect_$i actual
-'
+   rm -f order_fifo 
+   mkfifo order_fifo 
+   {
+   cat order_file_$i order_fifo 
+   } 
+   git diff -O order_fifo --name-only HEAD^..HEAD actual 
+   wait 
+   test_cmp expect_$i actual
+   '
 
test_expect_success orderfile using config ($i) '
-   git -c diff.orderfile=order_file_$i diff --name-only HEAD^..HEAD 
actual 
-   test_cmp expect_$i actual
-'
+   git -c diff.orderfile=order_file_$i diff --name-only 
HEAD^..HEAD actual 
+   test_cmp expect_$i actual
+   '
 
test_expect_success cancelling configured orderfile ($i) '
-   git -c diff.orderfile=order_file_$i diff -O/dev/null --name-only 
HEAD^..HEAD actual 
-   test_cmp expect_none actual
-'
+   git -c diff.orderfile=order_file_$i diff -O/dev/null 
--name-only HEAD^..HEAD actual 
+   test_cmp expect_none actual
+   '
 done
 
 test_done
-- 
1.8.5.2-297-g3e57c29

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Samuel Bronson]

The -O flag really shouldn't silently fail to do anything when given a
path that it can't read from.

However, it should be able to read from un-mmappable files, such as:

 * pipes/fifos

 * /dev/null:  It's a character device (at least on Linux)

 * ANY empty file:

   Quoting Linux mmap(2), SUSv3 specifies that mmap() should fail if
   length is 0.  However, in kernels before 2.6.12, mmap() succeeded in
   this case: no mapping was created and the call returned addr.  Since
   kernel 2.6.12, mmap() fails with the error EINVAL for this case.

We especially want -O/dev/null to work, since we will be documenting
it as the way to cancel diff.orderfile when we add that.

(Note: -O/dev/null did have the right effect, since the existing error
handling essentially worked out to silently ignore the orderfile.  But
this was probably more coincidence than anything else.)

So, lets toss all of that logic to get the file mmapped and just use
strbuf_read_file() instead, which gives us decent error handling
practically for free.

Signed-off-by: Samuel Bronson naes...@gmail.com
---
 diffcore-order.c  | 23 ---
 t/t4056-diff-order.sh | 23 +++
 2 files changed, 31 insertions(+), 15 deletions(-)

diff --git a/diffcore-order.c b/diffcore-order.c
index 23e9385..a63f332 100644
--- a/diffcore-order.c
+++ b/diffcore-order.c
@@ -10,28 +10,21 @@ static int order_cnt;
 
 static void prepare_order(const char *orderfile)
 {
-   int fd, cnt, pass;
+   int cnt, pass;
+   struct strbuf sb = STRBUF_INIT;
void *map;
char *cp, *endp;
-   struct stat st;
-   size_t sz;
+   ssize_t sz;
 
if (order)
return;
 
-   fd = open(orderfile, O_RDONLY);
-   if (fd  0)
-   return;
-   if (fstat(fd, st)) {
-   close(fd);
-   return;
-   }
-   sz = xsize_t(st.st_size);
-   map = mmap(NULL, sz, PROT_READ|PROT_WRITE, MAP_PRIVATE, fd, 0);
-   close(fd);
-   if (map == MAP_FAILED)
-   return;
+   sz = strbuf_read_file(sb, orderfile, 0);
+   if (sz  0)
+   die_errno(_(failed to read orderfile '%s'), orderfile);
+   map = strbuf_detach(sb, NULL);
endp = (char *) map + sz;
+
for (pass = 0; pass  2; pass++) {
cnt = 0;
cp = map;
diff --git a/t/t4056-diff-order.sh b/t/t4056-diff-order.sh
index 398b3f6..eb471e7 100755
--- a/t/t4056-diff-order.sh
+++ b/t/t4056-diff-order.sh
@@ -61,12 +61,35 @@ test_expect_success no order (=tree object order) '
test_cmp expect_none actual
 '
 
+test_expect_success 'missing orderfile' '
+   rm -f bogus_file 
+   test_must_fail git diff -Obogus_file --name-only HEAD^..HEAD
+'
+
+test_expect_success 'unreadable orderfile' '
+   touch unreadable_file 
+   chmod -r unreadable_file 
+   test_must_fail git diff -Ounreadable_file --name-only HEAD^..HEAD
+'
+
+test_expect_success 'orderfile is a directory' '
+   test_must_fail git diff -O/ --name-only HEAD^..HEAD
+'
+
 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
 '
+
+   test_expect_success PIPE orderfile is fifo ($i) '
+   rm -f order_fifo 
+   mkfifo order_fifo 
+   cat order_file_$i order_fifo 
+   git diff -O order_fifo --name-only HEAD^..HEAD actual 
+   test_cmp expect_$i actual
+'
 done
 
 test_done
-- 
1.8.4.3

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Junio C Hamano]

Samuel Bronson naes...@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.

 +
 + test_expect_success PIPE orderfile is fifo ($i) '
 + 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?

 + test_cmp expect_$i actual
 +'
  done
  
  test_done
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Samuel Bronson]

On Mon, Dec 16, 2013 at 4:09 PM, Junio C Hamano gits...@pobox.com wrote:
 Samuel Bronson naes...@gmail.com writes:

 +test_expect_success 'unreadable orderfile' '
 + touch unreadable_file 
 + chmod -r unreadable_file 

   - this test probably needs restricted to people with sane
 filesystems; I think POSIXPERM prerequisite and also SANITY
 prerequisite are needed, at least.

Hmm, yeah, you've got a point; now that I think more carefully, the
most FAT can do is something like chmod -w, nothing with the r
permissions.  Oops.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Samuel Bronson]

On Mon, Dec 16, 2013 at 4:32 PM, Junio C Hamano gits...@pobox.com wrote:
 Samuel Bronson naes...@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?

 + 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?
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html