Re: [PATCH v2 4/5] check-ignore: allow incremental streaming of queries via --stdin
On Mon, Apr 22, 2013 at 11:03:44AM -0700, Junio C Hamano wrote: Adam Spiers g...@adamspiers.org writes: On Thu, Apr 11, 2013 at 03:11:32PM -0400, Jeff King wrote: I always get a little nervous with sleeps in the test suite, as they are indicative that we are trying to avoid some race condition, which means that the test can fail when the system is under load, or when a tool like valgrind is used which drastically alters the timing (e.g., if check-ignore takes longer than 1 second to produce its answer, we may fail here). Agreed, especially here where my btrfs filesystems see fit to kindly freeze my system for a few seconds many times each day :-/ Is there a simpler way to test this? Like: ... I'll re-roll using your approach. I think I missed this one and it already is in 'next'. I'll hold it back so please make your re-roll into an incremental update. Will do - will probably take a few days more though, since I'm currently catching up on a post-travel work backlog. -- 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 v2 4/5] check-ignore: allow incremental streaming of queries via --stdin
Adam Spiers g...@adamspiers.org writes: On Thu, Apr 11, 2013 at 03:11:32PM -0400, Jeff King wrote: I always get a little nervous with sleeps in the test suite, as they are indicative that we are trying to avoid some race condition, which means that the test can fail when the system is under load, or when a tool like valgrind is used which drastically alters the timing (e.g., if check-ignore takes longer than 1 second to produce its answer, we may fail here). Agreed, especially here where my btrfs filesystems see fit to kindly freeze my system for a few seconds many times each day :-/ Is there a simpler way to test this? Like: ... I'll re-roll using your approach. I think I missed this one and it already is in 'next'. I'll hold it back so please make your re-roll into an incremental update. Thanks. -- 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 v2 4/5] check-ignore: allow incremental streaming of queries via --stdin
Some callers, such as the git-annex web assistant, find it useful to
invoke git check-ignore as a persistent background process, which can
then have queries fed to its STDIN at any point, and the corresponding
response consumed from its STDOUT. For this we need to invoke
check_ignore() once per line of standard input, and flush standard
output after each result.
The above use case suggests that empty STDIN is actually a reasonable
scenario (e.g. when the caller doesn't know in advance whether any
queries need to be fed to the background process until after it's
already started), so we make the minor behavioural change that no
pathspec given. is no longer emitted in when STDIN is empty.
Even though check_ignore() could now be changed to operate on a single
pathspec, we keep it operating on an array of pathspecs since that is
a more convenient way of consuming the existing pathspec API.
Signed-off-by: Adam Spiers g...@adamspiers.org
---
builtin/check-ignore.c | 15 +--
t/t0008-ignores.sh | 28 +++-
2 files changed, 28 insertions(+), 15 deletions(-)
diff --git a/builtin/check-ignore.c b/builtin/check-ignore.c
index e2d3006..c00a7d6 100644
--- a/builtin/check-ignore.c
+++ b/builtin/check-ignore.c
@@ -107,10 +107,9 @@ static int check_ignore(struct path_exclude_check *check,
static int check_ignore_stdin_paths(struct path_exclude_check *check, const
char *prefix)
{
struct strbuf buf, nbuf;
- char **pathspec = NULL;
- size_t nr = 0, alloc = 0;
+ char *pathspec[2] = { NULL, NULL };
int line_termination = null_term_line ? 0 : '\n';
- int num_ignored;
+ int num_ignored = 0;
strbuf_init(buf, 0);
strbuf_init(nbuf, 0);
@@ -121,14 +120,10 @@ static int check_ignore_stdin_paths(struct
path_exclude_check *check, const char
die(line is badly quoted);
strbuf_swap(buf, nbuf);
}
- ALLOC_GROW(pathspec, nr + 1, alloc);
- pathspec[nr] = xcalloc(strlen(buf.buf) + 1, sizeof(*buf.buf));
- strcpy(pathspec[nr++], buf.buf);
+ pathspec[0] = buf.buf;
+ num_ignored += check_ignore(check, prefix, (const char
**)pathspec);
+ maybe_flush_or_die(stdout, check-ignore to stdout);
}
- ALLOC_GROW(pathspec, nr + 1, alloc);
- pathspec[nr] = NULL;
- num_ignored = check_ignore(check, prefix, (const char **)pathspec);
- maybe_flush_or_die(stdout, attribute to stdout);
strbuf_release(buf);
strbuf_release(nbuf);
return num_ignored;
diff --git a/t/t0008-ignores.sh b/t/t0008-ignores.sh
index 7af93ba..fbf12ae 100755
--- a/t/t0008-ignores.sh
+++ b/t/t0008-ignores.sh
@@ -216,11 +216,7 @@ test_expect_success_multi 'empty command line' '' '
test_expect_success_multi '--stdin with empty STDIN' '' '
test_check_ignore --stdin 1 /dev/null
- if test -n $quiet_opt; then
- test_stderr
- else
- test_stderr no pathspec given.
- fi
+ test_stderr
'
test_expect_success '-q with multiple args' '
@@ -692,5 +688,27 @@ do
'
done
+test_expect_success 'setup: have stdbuf?' '
+ if which stdbuf /dev/null 21
+ then
+ test_set_prereq STDBUF
+ fi
+'
+
+test_expect_success STDBUF 'streaming support for --stdin' '
+ (
+ echo one
+ sleep 2
+ echo two
+ ) | stdbuf -oL git check-ignore -v -n --stdin out
+ pid=$!
+ sleep 1
+ grep ^\.gitignore:1:oneone out
+ test $( wc -l out ) = 1
+ sleep 2
+ grep ^:: two out
+ test $( wc -l out ) = 2
+ ( wait $pid || kill $pid || : ) 2/dev/null
+'
test_done
--
1.8.2.1.342.gfa7285d
--
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 v2 4/5] check-ignore: allow incremental streaming of queries via --stdin
On Thu, Apr 11, 2013 at 01:05:12PM +0100, Adam Spiers wrote: +test_expect_success 'setup: have stdbuf?' ' + if which stdbuf /dev/null 21 + then + test_set_prereq STDBUF + fi +' + +test_expect_success STDBUF 'streaming support for --stdin' ' + ( + echo one + sleep 2 + echo two + ) | stdbuf -oL git check-ignore -v -n --stdin out + pid=$! + sleep 1 + grep ^\.gitignore:1:oneone out + test $( wc -l out ) = 1 + sleep 2 + grep ^:: two out + test $( wc -l out ) = 2 + ( wait $pid || kill $pid || : ) 2/dev/null +' I always get a little nervous with sleeps in the test suite, as they are indicative that we are trying to avoid some race condition, which means that the test can fail when the system is under load, or when a tool like valgrind is used which drastically alters the timing (e.g., if check-ignore takes longer than 1 second to produce its answer, we may fail here). Is there a simpler way to test this? Like: # Set up a long-running check-ignore connected by pipes. mkfifo in out (git check-ignore ... in out ) # We cannot just echo in because check-ignore # would get EOF after echo exited; instead we open # the descriptor in our shell, and then echo to the # fd. We make sure to close it at the end, so that # the subprocess does get EOF and dies properly. exec 9in test_when_finished exec 9- # Now we can do interactive tests echo 9 one read response out test $response = ... echo 9 two read response out test $response = ... Hmm. Maybe simpler wasn't the right word. :) But it avoids any sleeps or race conditions. -Peff -- 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 v2 4/5] check-ignore: allow incremental streaming of queries via --stdin
On Thu, Apr 11, 2013 at 03:11:32PM -0400, Jeff King wrote: I always get a little nervous with sleeps in the test suite, as they are indicative that we are trying to avoid some race condition, which means that the test can fail when the system is under load, or when a tool like valgrind is used which drastically alters the timing (e.g., if check-ignore takes longer than 1 second to produce its answer, we may fail here). Agreed, especially here where my btrfs filesystems see fit to kindly freeze my system for a few seconds many times each day :-/ Is there a simpler way to test this? Like: # Set up a long-running check-ignore connected by pipes. mkfifo in out (git check-ignore ... in out ) # We cannot just echo in because check-ignore # would get EOF after echo exited; instead we open # the descriptor in our shell, and then echo to the # fd. We make sure to close it at the end, so that # the subprocess does get EOF and dies properly. exec 9in test_when_finished exec 9- # Now we can do interactive tests echo 9 one read response out test $response = ... echo 9 two read response out test $response = ... Hmm. Maybe simpler wasn't the right word. :) But it avoids any sleeps or race conditions. The shell source is strong with this one ;-) Congrats - I first tried with FIFOs (hence my other patch which moves the PIPE test prerequisite definition into the core framework - the original intention was to reuse it here) but failed to get it working. I'll re-roll using your approach. -- 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 v2 4/5] check-ignore: allow incremental streaming of queries via --stdin
On Thu, Apr 11, 2013 at 09:31:41PM +0100, Adam Spiers wrote: The shell source is strong with this one ;-) Congrats - I first tried with FIFOs (hence my other patch which moves the PIPE test prerequisite definition into the core framework - the original intention was to reuse it here) but failed to get it working. I'll re-roll using your approach. Thanks. If it make you feel any better, it took about 20 minutes of experimenting and at least three head-scratching wait, that _should_ have worked moments to get it right. :) The rest of the series looked good to me, though I admit I did not think too hard about the --non-matching patch, as it looked like you and Junio had already given some thought to the output format, which is the tricky part. -Peff -- 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 v2 4/5] check-ignore: allow incremental streaming of queries via --stdin
At 13:05 +0100 11 Apr 2013, Adam Spiers g...@adamspiers.org wrote: The above use case suggests that empty STDIN is actually a reasonable scenario (e.g. when the caller doesn't know in advance whether any queries need to be fed to the background process until after it's already started), so we make the minor behavioural change that no pathspec given. is no longer emitted in when STDIN is empty. The last in there looks to be misplaced. Was that originally something like in the case? If so the removed words should be restored or the lingering one removed as well. -- 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 v2 4/5] check-ignore: allow incremental streaming of queries via --stdin
On Thu, Apr 11, 2013 at 05:04:30PM -0400, Aaron Schrab wrote: At 13:05 +0100 11 Apr 2013, Adam Spiers g...@adamspiers.org wrote: The above use case suggests that empty STDIN is actually a reasonable scenario (e.g. when the caller doesn't know in advance whether any queries need to be fed to the background process until after it's already started), so we make the minor behavioural change that no pathspec given. is no longer emitted in when STDIN is empty. The last in there looks to be misplaced. Was that originally something like in the case? If so the removed words should be restored or the lingering one removed as well. I'll remove it; thanks. -- 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
