Re: [PATCH v2 2/2] grep: stop looking at random places for .gitattributes

[Johannes Sixt]

Am 10/15/2012 18:54, schrieb Junio C Hamano:
 Ideally, that earlier workaround
 should have done a logica equivalent of:
 ...
 and did so not in-line at the calling site but in a compat/ wrapper
 for fflush() to eliminate the need for the ifdef.

Fair enough.

 But reverting the EINVAL check from write_or_die.c is out of question,
 because that handles a real case.

Correction: I can't reproduce the error messages that this was working
around anymore in a brief test. I'll revert the check locally and see what
happens.

-- Hannes
--
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 2/2] grep: stop looking at random places for .gitattributes

[Johannes Sixt]

Am 10/14/2012 6:29, schrieb Junio C Hamano:
 Johannes Sixt j.s...@viscovery.net writes:
 
 It might be worth it. We already have a similar special case in
 write_or_die.c:maybe_flush_or_die() for Windows, although it is not about
 a colon in a path name.

 Perhaps like this.
 
 Hrm, the we already have one b2f5e26 (Windows: Work around an
 oddity when a pipe with no reader is written to., 2007-08-17) was
 what you added while I was looking the other way ;-) as a part of
 Windows specific pull.
 
 That change, and this patch, seem to cover the cases to be ignored
 with a bit too wide a net to my taste.  On other systems, and even
 on Windows if the path does not have any colon, EINVAL is something
 we would want to noticbbe and report, as a potential problem, no?

For fopen(), EINVAL should occur only if the mode argument is wrong, which
it isn't. For fflush() (as in write_or_die.c), EINVAL is not even listed
as possible error code. Therefore, catching EINVAL wholesale should not be
a problem, IMO, at least not on other systems.

On Windows, it is more problematic because there is a table of customary
Windows API error codes, which are mapped to errno values, and EINVAL is
used for all other Windows error codes (and for a few listed ones), and
ignoring EINVAL might indeed miss something worth to be reported.

Sooo... I don't mind if you do not pick up this patch because it handles a
rather theoretic case, i.e., where a project with strange paths somehow
ended up on a Windows drive.

But reverting the EINVAL check from write_or_die.c is out of question,
because that handles a real case.

-- Hannes
--
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 2/2] grep: stop looking at random places for .gitattributes

[Junio C Hamano]

Johannes Sixt j.s...@viscovery.net writes:

 It might be worth it. We already have a similar special case in
 write_or_die.c:maybe_flush_or_die() for Windows, although it is not about
 a colon in a path name.

 Perhaps like this.

Hrm, the we already have one b2f5e26 (Windows: Work around an
oddity when a pipe with no reader is written to., 2007-08-17) was
what you added while I was looking the other way ;-) as a part of
Windows specific pull.

That change, and this patch, seem to cover the cases to be ignored
with a bit too wide a net to my taste.  On other systems, and even
on Windows if the path does not have any colon, EINVAL is something
we would want to notice and report, as a potential problem, no?

 --- 8 ---
 From: Johannes Sixt j...@kdbg.org
 Subject: [PATCH] attr: do not warn on path with colon on Windows

 In the same spirit as commit 8e950dab (attr: failure to open a
 .gitattributes file is OK with ENOTDIR), ignore EINVAL errors. On
 Windows, a path that contains a colon that is not the one after the
 drive letter is not allowed and is reported with errno set to
 EINVAL.

 Signed-off-by: Johannes Sixt j...@kdbg.org
 ---
  attr.c | 10 +-
  1 file changed, 9 insertions(+), 1 deletion(-)

 diff --git a/attr.c b/attr.c
 index 8010429..ac945ad 100644
 --- a/attr.c
 +++ b/attr.c
 @@ -354,7 +354,15 @@ static struct attr_stack *read_attr_from_file(const char 
 *path, int macro_ok)
   int lineno = 0;
  
   if (!fp) {
 - if (errno != ENOENT  errno != ENOTDIR)
 + /*
 +  * If path does not exist, it is not worth mentioning,
 +  * but I/O errors and permission errors are.
 +  *
 +  * On Windows, EINVAL is reported if path contains a colon
 +  * that is not the driver letter separator. Such a path
 +  * cannot exist in the file system and is also uninteresting.
 +  */
 + if (errno != ENOENT  errno != ENOTDIR  errno != EINVAL)
   warn_on_inaccessible(path);
   return NULL;
   }
--
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 2/2] grep: stop looking at random places for .gitattributes

[Johannes Sixt]

Am 10/11/2012 17:51, schrieb Junio C Hamano:
 Johannes Sixt j.s...@viscovery.net writes:
 
 I saw EINVAL errors when 'git grep pattern rev' was run on Windows. The
 reason is that the code attempted to access rev:dir/.gitattributes in
 the worktree, which is an invalid path on Windows due to the colon. The
 lack of this warning indicates that the attempts to access these files are
 eliminated.
 
 It means that whenever we ask for attributes for a tracked file that
 is inside a directory whose name contains a colon, we would get the
 same error on Windows, no?  Perhaps Windows may not let you create
 such a directory in the first place, but you may still get a
 repository of a cross platform project that contains such a path.

Your assessment is correct.

 What I am wondering is if we should do something similar to 8e950da
 (attr: failure to open a .gitattributes file is OK with ENOTDIR,
 2012-09-13), at least on Windows, for EINVAL.

It might be worth it. We already have a similar special case in
write_or_die.c:maybe_flush_or_die() for Windows, although it is not about
a colon in a path name.

Perhaps like this.

--- 8 ---
From: Johannes Sixt j...@kdbg.org
Subject: [PATCH] attr: do not warn on path with colon on Windows

In the same spirit as commit 8e950dab (attr: failure to open a
.gitattributes file is OK with ENOTDIR), ignore EINVAL errors. On
Windows, a path that contains a colon that is not the one after the
drive letter is not allowed and is reported with errno set to
EINVAL.

Signed-off-by: Johannes Sixt j...@kdbg.org
---
 attr.c | 10 +-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/attr.c b/attr.c
index 8010429..ac945ad 100644
--- a/attr.c
+++ b/attr.c
@@ -354,7 +354,15 @@ static struct attr_stack *read_attr_from_file(const char 
*path, int macro_ok)
int lineno = 0;
 
if (!fp) {
-   if (errno != ENOENT  errno != ENOTDIR)
+   /*
+* If path does not exist, it is not worth mentioning,
+* but I/O errors and permission errors are.
+*
+* On Windows, EINVAL is reported if path contains a colon
+* that is not the driver letter separator. Such a path
+* cannot exist in the file system and is also uninteresting.
+*/
+   if (errno != ENOENT  errno != ENOTDIR  errno != EINVAL)
warn_on_inaccessible(path);
return NULL;
}
-- 
1.8.0.rc2.1237.g5522246
--
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 2/2] grep: stop looking at random places for .gitattributes

[Junio C Hamano]

Johannes Sixt j.s...@viscovery.net writes:

 I saw EINVAL errors when 'git grep pattern rev' was run on Windows. The
 reason is that the code attempted to access rev:dir/.gitattributes in
 the worktree, which is an invalid path on Windows due to the colon. The
 lack of this warning indicates that the attempts to access these files are
 eliminated.

It means that whenever we ask for attributes for a tracked file that
is inside a directory whose name contains a colon, we would get the
same error on Windows, no?  Perhaps Windows may not let you create
such a directory in the first place, but you may still get a
repository of a cross platform project that contains such a path.

What I am wondering is if we should do something similar to 8e950da
(attr: failure to open a .gitattributes file is OK with ENOTDIR,
2012-09-13), at least on Windows, for EINVAL.

--
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 2/2] grep: stop looking at random places for .gitattributes

[Nguyễn Thái Ngọc Duy]

grep searches for .gitattributes using name field in struct
grep_source but that field is not real on-disk path name. For example,
grep pattern rev fills the field with rev:path, which is
non-existent usually until somebody exploits it to drive git away.

This patch passes real paths down to grep_source_load_driver(). Except
grepping a blob, all other cases should have right paths down to
grep_source_load_driver(). In other words, .gitattributes are still
respected.

Initial-work-by: Jeff King p...@peff.net
Signed-off-by: Nguyễn Thái Ngọc Duy pclo...@gmail.com
---
 builtin/grep.c | 22 --
 grep.c | 11 ---
 grep.h |  4 +++-
 t/t7008-grep-binary.sh | 22 ++
 4 files changed, 45 insertions(+), 14 deletions(-)

diff --git a/builtin/grep.c b/builtin/grep.c
index 377c904..f6c5ba2 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -86,7 +86,7 @@ static pthread_cond_t cond_result;
 static int skip_first_line;
 
 static void add_work(struct grep_opt *opt, enum grep_source_type type,
-const char *name, const void *id)
+const char *name, const char *path, const void *id)
 {
grep_lock();
 
@@ -94,7 +94,7 @@ static void add_work(struct grep_opt *opt, enum 
grep_source_type type,
pthread_cond_wait(cond_write, grep_mutex);
}
 
-   grep_source_init(todo[todo_end].source, type, name, id);
+   grep_source_init(todo[todo_end].source, type, name, path, id);
if (opt-binary != GREP_BINARY_TEXT)
grep_source_load_driver(todo[todo_end].source);
todo[todo_end].done = 0;
@@ -371,7 +371,8 @@ static void *lock_and_read_sha1_file(const unsigned char 
*sha1, enum object_type
 }
 
 static int grep_sha1(struct grep_opt *opt, const unsigned char *sha1,
-const char *filename, int tree_name_len)
+const char *filename, int tree_name_len,
+const char *path)
 {
struct strbuf pathbuf = STRBUF_INIT;
 
@@ -385,7 +386,7 @@ static int grep_sha1(struct grep_opt *opt, const unsigned 
char *sha1,
 
 #ifndef NO_PTHREADS
if (use_threads) {
-   add_work(opt, GREP_SOURCE_SHA1, pathbuf.buf, sha1);
+   add_work(opt, GREP_SOURCE_SHA1, pathbuf.buf, path, sha1);
strbuf_release(pathbuf);
return 0;
} else
@@ -394,7 +395,7 @@ static int grep_sha1(struct grep_opt *opt, const unsigned 
char *sha1,
struct grep_source gs;
int hit;
 
-   grep_source_init(gs, GREP_SOURCE_SHA1, pathbuf.buf, sha1);
+   grep_source_init(gs, GREP_SOURCE_SHA1, pathbuf.buf, path, 
sha1);
strbuf_release(pathbuf);
hit = grep_source(opt, gs);
 
@@ -414,7 +415,7 @@ static int grep_file(struct grep_opt *opt, const char 
*filename)
 
 #ifndef NO_PTHREADS
if (use_threads) {
-   add_work(opt, GREP_SOURCE_FILE, buf.buf, filename);
+   add_work(opt, GREP_SOURCE_FILE, buf.buf, filename, filename);
strbuf_release(buf);
return 0;
} else
@@ -423,7 +424,7 @@ static int grep_file(struct grep_opt *opt, const char 
*filename)
struct grep_source gs;
int hit;
 
-   grep_source_init(gs, GREP_SOURCE_FILE, buf.buf, filename);
+   grep_source_init(gs, GREP_SOURCE_FILE, buf.buf, filename, 
filename);
strbuf_release(buf);
hit = grep_source(opt, gs);
 
@@ -479,7 +480,7 @@ static int grep_cache(struct grep_opt *opt, const struct 
pathspec *pathspec, int
if (cached || (ce-ce_flags  CE_VALID) || 
ce_skip_worktree(ce)) {
if (ce_stage(ce))
continue;
-   hit |= grep_sha1(opt, ce-sha1, ce-name, 0);
+   hit |= grep_sha1(opt, ce-sha1, ce-name, 0, ce-name);
}
else
hit |= grep_file(opt, ce-name);
@@ -518,7 +519,8 @@ static int grep_tree(struct grep_opt *opt, const struct 
pathspec *pathspec,
strbuf_add(base, entry.path, te_len);
 
if (S_ISREG(entry.mode)) {
-   hit |= grep_sha1(opt, entry.sha1, base-buf, tn_len);
+   hit |= grep_sha1(opt, entry.sha1, base-buf, tn_len,
+base-buf + tn_len);
}
else if (S_ISDIR(entry.mode)) {
enum object_type type;
@@ -548,7 +550,7 @@ static int grep_object(struct grep_opt *opt, const struct 
pathspec *pathspec,
   struct object *obj, const char *name)
 {
if (obj-type == OBJ_BLOB)
-   return grep_sha1(opt, obj-sha1, name, 0);
+   return grep_sha1(opt, obj-sha1, name, 0, NULL);
if (obj-type == OBJ_COMMIT || obj-type == OBJ_TREE) {
  

Re: [PATCH v2 2/2] grep: stop looking at random places for .gitattributes

[Johannes Sixt]

Am 10/10/2012 15:59, schrieb Nguyễn Thái Ngọc Duy:
 This patch passes real paths down to grep_source_load_driver(). Except
 grepping a blob, all other cases should have right paths down to

... grepping a blob or tree object...

 grep_source_load_driver(). In other words, .gitattributes are still
 respected.
...
 +test_expect_success 'grep --cached respects binary diff attribute (2)' '
 + git add .gitattributes 
 + rm .gitattributes 
 + git grep --cached text t actual 
 + test_cmp expect actual 
 + git checkout .gitattributes 
 + git rm --cached .gitattributes
 +'

This should perhaps be test_when_finished git rm --cached .gitattributes.

 +
 +test_expect_success 'grep tree respects binary diff attribute' '

I was confused by the word tree here. Isn't pathspec more correct?

 + git commit -m new 
 + echo Binary file HEAD:t matches expect 
 + git grep text HEAD -- t actual 
 + test_cmp expect actual 
 + git reset HEAD^
 +'

And in yet another test, should

git grep text HEAD:t

/not/ respect the binary attribute?

At any rate, I don't observe the warnings anymore with this series.

Thanks,
-- Hannes
--
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 2/2] grep: stop looking at random places for .gitattributes

[Junio C Hamano]

Johannes Sixt j.s...@viscovery.net writes:

 At any rate, I don't observe the warnings anymore with this series.

What kind of warnings have you been getting?  Earlier we had a bug
in the jk/config-warn-on-inaccessible-paths series that made it warn
when we tried to open a .gitattribute file and open() returned an
error other than ENOENT.  The bug was that we saw unnecessary errors
when a directory that used to exist no longer exists in the working
tree; we would instead get ENOTDIR in such a case that needs to be
ignored.

The problem was supposed to be fixed by 8e950da (attr: failure to
open a .gitattributes file is OK with ENOTDIR, 2012-09-13); if you
are still seeing the error, that error still may need to be
addressed, regardless of Nguyễn's patch.

--
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 2/2] grep: stop looking at random places for .gitattributes

[Nguyen Thai Ngoc Duy]

On Wed, Oct 10, 2012 at 9:21 PM, Johannes Sixt j.s...@viscovery.net wrote:
 + git commit -m new 
 + echo Binary file HEAD:t matches expect 
 + git grep text HEAD -- t actual 
 + test_cmp expect actual 
 + git reset HEAD^
 +'

 And in yet another test, should

 git grep text HEAD:t

 /not/ respect the binary attribute?

Gray area. Is it ok to do that without documenting it (i.e. common
sense)? I have something in mind that could do that, but it also makes
git grep text HEAD^{tree} not respect attributes.
-- 
Duy
--
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 2/2] grep: stop looking at random places for .gitattributes

[Junio C Hamano]

Nguyen Thai Ngoc Duy pclo...@gmail.com writes:

 On Wed, Oct 10, 2012 at 9:21 PM, Johannes Sixt j.s...@viscovery.net wrote:
 + git commit -m new 
 + echo Binary file HEAD:t matches expect 
 + git grep text HEAD -- t actual 
 + test_cmp expect actual 
 + git reset HEAD^
 +'

 And in yet another test, should

 git grep text HEAD:t

 /not/ respect the binary attribute?

 Gray area. Is it ok to do that without documenting it (i.e. common
 sense)? I have something in mind that could do that, but it also makes
 git grep text HEAD^{tree} not respect attributes.

Personally, I do not think HEAD:t is worth worrying about.

We could use the get_sha1_with_context() to get t out of HEAD:t,
and we could even enhance get_sha1_with_context() to also preserve
the value of what came before the colon, but that would mean that
these three

git grep text HEAD:t/t0200
git grep text $(git rev-parse HEAD:t/t0200)
git grep text $(git rev-parse HEAD:t):t0200

would behave differently; only the first one has any chance of
applying the correct set of .gitattributes.  All of them would be
able to use the .gitattributes file contained in the tree object
that corresponds to t/t0200 (if we updated attr.c to read from tree
objects, that is), but the latter two would skip .gitattributes
files from the top-level and t directories, still using the final
fallback definition from $GIT_DIR/info/attributes file.

If we have to draw a line somewhere, the saner place to draw it is
to stop at

git grep text HEAD -- t/t0200


--
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 2/2] grep: stop looking at random places for .gitattributes

[Johannes Sixt]

Am 10/10/2012 21:56, schrieb Junio C Hamano:
 Johannes Sixt j.s...@viscovery.net writes:
 
 At any rate, I don't observe the warnings anymore with this series.
 
 What kind of warnings have you been getting?  Earlier we had a bug
 in the jk/config-warn-on-inaccessible-paths series that made it warn
 when we tried to open a .gitattribute file and open() returned an
 error other than ENOENT.  The bug was that we saw unnecessary errors
 when a directory that used to exist no longer exists in the working
 tree; we would instead get ENOTDIR in such a case that needs to be
 ignored.

I saw EINVAL errors when 'git grep pattern rev' was run on Windows. The
reason is that the code attempted to access rev:dir/.gitattributes in
the worktree, which is an invalid path on Windows due to the colon. The
lack of this warning indicates that the attempts to access these files are
eliminated.

-- Hannes
--
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