git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Adam Dinwoodie <adam@dinwoodie.org>
To: git@vger.kernel.org
Cc: Johannes Schindelin <johannes.schindelin@gmx.de>
Subject: cat-file timing window on Cygwin
Date: Fri, 25 Aug 2017 12:25:29 +0100	[thread overview]
Message-ID: <20170825112529.GA10378@dinwoodie.org> (raw)

As of v2.10.0-rc1-4-g321459439 ("cat-file: support --textconv/--filters
in batch mode"), t8010-cat-file-filters.sh has been failing on Cygwin.
Digging into this, the test looks to expose a timing window: it appears
that if `git cat-file --textconv --batch` receives input on stdin too
quickly, it fails to parse some of that input.

Compare the following output, run from the t8010 trash directory after a
failed test run, where adding a `sleep` between the two lines of input
changes the output:

    $ { echo $sha1 hello.txt ; echo $sha1 hello; } | git -c diff.txt.textconv='tr A-Za-z N-ZA-Mn-za-m <' cat-file --textconv --batch
    ce013625030ba8dba906f756967f9e9ca394464a blob 6
    uryyb

    $ { echo $sha1 hello.txt ; sleep 1; echo $sha1 hello; } | git -c diff.txt.textconv='tr A-Za-z N-ZA-Mn-za-m <' cat-file --textconv --batch
    ce013625030ba8dba906f756967f9e9ca394464a blob 6
    uryyb

    ce013625030ba8dba906f756967f9e9ca394464a blob 6
    hello

Similarly, I can get t8010 to pass with the following patch:

diff --git a/t/t8010-cat-file-filters.sh b/t/t8010-cat-file-filters.sh
index d8242e467..3aa1385ad 100755
--- a/t/t8010-cat-file-filters.sh
+++ b/t/t8010-cat-file-filters.sh
@@ -54,7 +54,7 @@
 test_expect_success 'cat-file --textconv --batch works' '
 	sha1=$(git rev-parse -q --verify HEAD:world.txt) &&
 	test_config diff.txt.textconv "tr A-Za-z N-ZA-Mn-za-m <" &&
-	printf "%s hello.txt\n%s hello\n" $sha1 $sha1 |
+	{ printf "%s hello.txt\n" $sha1 && sleep 1 && printf "%s hello\n" $sha1; } |
 	git cat-file --textconv --batch >actual &&
 	printf "%s blob 6\nuryyb\r\n\n%s blob 6\nhello\n\n" \
 		$sha1 $sha1 >expect &&

I don't think blindly applying the patch is the solution here, however.
The correct solution is presumably to work out what is causing cat-file
to discard some of its input and to get it to stop doing that.

(For reference, to get the output above the test was run on the current
master branch, specifically v2.14.1-326-g3dc57ebfb, while the local
installed Git version was v2.14.0, although this behaviour seems to be
consistent since the originating commit.)

             reply	other threads:[~2017-08-25 11:25 UTC|newest]

Thread overview: 11+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-08-25 11:25 Adam Dinwoodie [this message]
2017-08-25 15:08 ` cat-file timing window on Cygwin Jeff King
2017-08-26  0:57   ` Ramsay Jones
2017-08-26 15:43     ` Adam Dinwoodie
2017-08-26 18:53     ` Jeff King
2017-08-26 21:11       ` Adam Dinwoodie
2017-08-27  2:06         ` Ramsay Jones
2017-08-27 11:33           ` Adam Dinwoodie
2017-08-27 15:47             ` Ramsay Jones
2017-08-27 18:53               ` Jason Pyeron
2017-09-08 20:02               ` Ramsay Jones

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=20170825112529.GA10378@dinwoodie.org \
    --to=adam@dinwoodie.org \
    --cc=git@vger.kernel.org \
    --cc=johannes.schindelin@gmx.de \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
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;
as well as URLs for NNTP newsgroup(s).