RFC: two minor tweaks to check-ignore to help git-annex assistant

2013-04-08 Thread Adam Spiers
Hi all,

I was recently informed by the author of git-annex that my
implementation of git check-ignore has two minor deficiencies which
currently prevent him from adding .gitignore support to the git-annex
assistant (web UI):

1. When accepting a list of files to check via --stdin, no results
   are calculated until EOF is hit.  This prevents it being used
   as a persistent background query process which streams results
   to its caller.  (This is inconsistent with check-attr, which
   *does* support stream-like behaviour.)

2. Even if it did support streaming, the lack of output for files
   which don't match any ignore pattern make it impossible for the
   consumer to distinguish between the two cases a) the file
   doesn't match any pattern and b) it does match but the output
   hasn't arrived yet.

Both of these are pretty trivial to fix.  The first is fixed by
changing check_ignore_stdin_paths() to invoke check_ignore() per input
line, rather than collecting all input from STDIN and then invoking
check_ignore() on the whole lot.  (I have not implemented this yet,
but may well be able to do it this week, thanks to it being one of
SUSE's hack weeks :-)

I already have a rough fix for the second issue, but I wanted to
solicit feedback on the appropriate UI changes before proceeding much
further.  Does something like the below patch seem reasonable, modulo
the lack of tests?  In case the UI changes I am proposing are not
clear from the patch, here's some example output from running it
inside a clone of the git source tree:

$ git check-ignore -v -n foo.tar.{gz,bz2}
.gitignore:203:*.tar.gz foo.tar.gz
::  foo.tar.bz2

So the number of output fields does not change depending on whether
the pattern matches or not, and any caller can determine whether it
does simply by checking whether the first field is non-empty.

Also, does it make sense to write a new test to accompany the fix to
the first (streaming) issue?

Thanks,
Adam

-- 8 --
Subject: [PATCH] check-ignore: add -n / --non-matching option

If `-n` or `--non-matching` are specified, non-matching pathnames will
also be output, in which case all fields in each output record except
for pathname will be empty.  This can be useful when running
non-interactively, so that files can be incrementally streamed to
STDIN of a long-running check-ignore process, and for each of these
files, STDOUT will indicate whether that file matched a pattern or
not.  (Without this option, it would be impossible to tell whether the
absence of output for a given file meant that it didn't match any
pattern, or that the output hadn't been generated yet.)

Signed-off-by: Adam Spiers g...@adamspiers.org
---
 Documentation/git-check-ignore.txt | 15 +
 builtin/check-ignore.c | 43 --
 2 files changed, 42 insertions(+), 16 deletions(-)

diff --git a/Documentation/git-check-ignore.txt 
b/Documentation/git-check-ignore.txt
index 854e4d0..7e3cabc 100644
--- a/Documentation/git-check-ignore.txt
+++ b/Documentation/git-check-ignore.txt
@@ -39,6 +39,12 @@ OPTIONS
below).  If `--stdin` is also given, input paths are separated
with a NUL character instead of a linefeed character.
 
+-n, --non-matching::
+   Show given paths which don't match any pattern.  This only
+   makes sense when `--verbose` is enabled, otherwise it would
+   not be possible to distinguish between paths which match a
+   pattern and those which don't.
+
 OUTPUT
 --
 
@@ -65,6 +71,15 @@ are also used instead of colons and hard tabs:
 
 source NULL linenum NULL pattern NULL pathname NULL
 
+If `-n` or `--non-matching` are specified, non-matching pathnames will
+also be output, in which case all fields in each output record except
+for pathname will be empty.  This can be useful when running
+non-interactively, so that files can be incrementally streamed to
+STDIN of a long-running check-ignore process, and for each of these
+files, STDOUT will indicate whether that file matched a pattern or
+not.  (Without this option, it would be impossible to tell whether the
+absence of output for a given file meant that it didn't match any
+pattern, or that the output hadn't been generated yet.)
 
 EXIT STATUS
 ---
diff --git a/builtin/check-ignore.c b/builtin/check-ignore.c
index 0240f99..498fd65 100644
--- a/builtin/check-ignore.c
+++ b/builtin/check-ignore.c
@@ -5,7 +5,7 @@
 #include pathspec.h
 #include parse-options.h
 
-static int quiet, verbose, stdin_paths;
+static int quiet, verbose, stdin_paths, show_non_matching;
 static const char * const check_ignore_usage[] = {
 git check-ignore [options] pathname...,
 git check-ignore [options] --stdin  list-of-paths,
@@ -22,21 +22,28 @@ static const struct option check_ignore_options[] = {
N_(read file names from stdin)),
OPT_BOOLEAN('z', NULL, null_term_line,
N_(input paths are 

Re: RFC: two minor tweaks to check-ignore to help git-annex assistant

2013-04-08 Thread Junio C Hamano
Adam Spiers g...@adamspiers.org writes:

 I already have a rough fix for the second issue, but I wanted to
 solicit feedback on the appropriate UI changes before proceeding much
 further.  Does something like the below patch seem reasonable, modulo
 the lack of tests?  In case the UI changes I am proposing are not
 clear from the patch, here's some example output from running it
 inside a clone of the git source tree:

 $ git check-ignore -v -n foo.tar.{gz,bz2}
 .gitignore:203:*.tar.gz foo.tar.gz
 ::  foo.tar.bz2

 So the number of output fields does not change depending on whether
 the pattern matches or not, and any caller can determine whether it
 does simply by checking whether the first field is non-empty.

Haven't looked at the proposed patch very carefully, but the design
looks sound.  The above output screams empty! nothing!, and I do
not think there is any other way :: will show up in that position.

 Also, does it make sense to write a new test to accompany the fix to
 the first (streaming) issue?

Would it be tricky to write safely not to get stuck?  You feed one
line, stop feeding, while checking that the output has arrived, and
then kill the whole thing?  Feels somewhat yucky, but sounds doable.
--
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