Re: [PATCH 2/2 v2] check-attr: Add --stdin-paths option

[Dmitry Potapov <dpotapov@gmail.com>]

On Wed, Oct 08, 2008 at 08:24:43AM -0700, Shawn O. Pearce wrote:
> Dmitry Potapov <dpotapov@gmail.com> wrote:
> > This allows multiple paths to be specified on stdin.
> 
> > diff --git a/Documentation/git-check-attr.txt b/Documentation/git-check-attr.txt
> > index 2b821f2..0839a57 100644
> > --- a/Documentation/git-check-attr.txt
> > +++ b/Documentation/git-check-attr.txt
> > @@ -9,6 +9,7 @@ git-check-attr - Display gitattributes information.
> >  SYNOPSIS
> >  --------
> >  'git check-attr' attr... [--] pathname...
> > +'git check-attr' --stdin-paths attr... < <list-of-paths
> 
> I wonder if the option should just be "--stdin".

I used "--stdin-paths" because git hash-object uses it, while "--stdin"
means to read the object from standard input. OTOH, we are never going
to read the object from standard input in check-attr and some other git
commands use "--stdin" to mean: read the list of paths from the standard
input. So, I fully agree here.

> And since its being
> used mostly by automated tools (gitk/git-gui) I wonder if a -z should
> also be supported for input termination with NUL instead of LF.

I have added it, but after I did, I start to wonder whether it is the
right thing to do to unquote NUL terminated input line?

NUL terminated makes sense when you feed raw-bytes, and if the first
byte happen to be a quote character, I suppose it should be treated
just as any other byte, not as a sign that the string is quited. But
then I looked at git checkout-index, and it unquotes string even if it
is NUL terminated. I don't think it is the right thing to do, but just
to be consistent, I have decided to leave as-is, i.e. to unquote a NUL
terminated string.

> > +test_expect_success 'attribute test: read paths from stdin' '
> 
> A test case for the quoting might also be good.

As far as I can tell, there is no test case for special characters in
filenames when these filenames are given as arguments. And there are a
few problems with them. First, it is using colon as a separator in
output, which breaks parsing of a filename containing colons.  Second,
I still have not figured out how to specify filenames with special
characters in gitattributes. The documentation does not say anything
and was lazy to study the code. Does gitattributes understand quote
strings in filenames?


Anyway, here is interdiff to my previous patch, which addresses two
first points as I described above. (I can resend the full patch if
necessary).

-- >8 --
From: Dmitry Potapov <dpotapov@gmail.com>
Date: Sun, 12 Oct 2008 18:08:43 +0400
Subject: [PATCH] check-attr: Add --stdin option

---
 Documentation/git-check-attr.txt |    8 ++++++--
 builtin-check-attr.c             |   13 +++++++++----
 t/t0003-attributes.sh            |    2 +-
 3 files changed, 16 insertions(+), 7 deletions(-)

diff --git a/Documentation/git-check-attr.txt b/Documentation/git-check-attr.txt
index 0839a57..14e4374 100644
--- a/Documentation/git-check-attr.txt
+++ b/Documentation/git-check-attr.txt
@@ -9,7 +9,7 @@ git-check-attr - Display gitattributes information.
 SYNOPSIS
 --------
 'git check-attr' attr... [--] pathname...
-'git check-attr' --stdin-paths attr... < <list-of-paths
+'git check-attr' --stdin [-z] attr... < <list-of-paths
 
 DESCRIPTION
 -----------
@@ -18,9 +18,13 @@ For every pathname, this command will list if each attr is 'unspecified',
 
 OPTIONS
 -------
---stdin-paths::
+--stdin::
 	Read file names from stdin instead of from the command-line.
 
+-z::
+	Only meaningful with `--stdin`; paths are separated with
+	NUL character instead of LF.
+
 \--::
 	Interpret all preceding arguments as attributes, and all following
 	arguments as path names. If not supplied, only the first argument will
diff --git a/builtin-check-attr.c b/builtin-check-attr.c
index fa1e4d5..02a8292 100644
--- a/builtin-check-attr.c
+++ b/builtin-check-attr.c
@@ -7,12 +7,16 @@
 static int stdin_paths;
 static const char * const check_attr_usage[] = {
 "git check-attr attr... [--] pathname...",
-"git check-attr --stdin-paths attr... < <list-of-paths>",
+"git check-attr --stdin attr... < <list-of-paths>",
 NULL
 };
 
+static int null_term_line;
+
 static const struct option check_attr_options[] = {
-	OPT_BOOLEAN(0 , "stdin-paths", &stdin_paths, "read file names from stdin"),
+	OPT_BOOLEAN(0 , "stdin", &stdin_paths, "read file names from stdin"),
+	OPT_BOOLEAN('z', NULL, &null_term_line,
+		"input paths are terminated by a null character"),
 	OPT_END()
 };
 
@@ -41,10 +45,11 @@ static void check_attr_stdin_paths(int cnt, struct git_attr_check *check,
 	const char** name)
 {
 	struct strbuf buf, nbuf;
+	int line_termination = null_term_line ? 0 : '\n';
 
 	strbuf_init(&buf, 0);
 	strbuf_init(&nbuf, 0);
-	while (strbuf_getline(&buf, stdin, '\n') != EOF) {
+	while (strbuf_getline(&buf, stdin, line_termination) != EOF) {
 		if (buf.buf[0] == '"') {
 			strbuf_reset(&nbuf);
 			if (unquote_c_style(&nbuf, buf.buf, NULL))
@@ -90,7 +95,7 @@ int cmd_check_attr(int argc, const char **argv, const char *prefix)
 	if (cnt <= 0)
 		errstr = "No attribute specified";
 	else if (stdin_paths && doubledash < argc)
-		errstr = "Can't specify files with --stdin-paths";
+		errstr = "Can't specify files with --stdin";
 	if (errstr) {
 		error (errstr);
 		usage_with_options(check_attr_usage, check_attr_options);
diff --git a/t/t0003-attributes.sh b/t/t0003-attributes.sh
index f6901b4..1c77192 100755
--- a/t/t0003-attributes.sh
+++ b/t/t0003-attributes.sh
@@ -60,7 +60,7 @@ a/b/h: test: a/b/h
 a/b/d/g: test: a/b/d/*
 EOF
 
-	sed -e "s/:.*//" < expect | git check-attr --stdin-paths test > actual &&
+	sed -e "s/:.*//" < expect | git check-attr --stdin test > actual &&
 	test_cmp expect actual
 '
 
-- 
1.6.0.2.521.g49aa8.dirty

-- >8 --