[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 
Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 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  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  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  writes:

> On Wed, Oct 10, 2012 at 9:21 PM, Johannes Sixt  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  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

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

[Junio C Hamano]

Johannes Sixt  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

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  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 
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 
---
 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  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 
> 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 
> ---
>  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/14/2012 6:29, schrieb Junio C Hamano:
> Johannes Sixt  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  writes:

> 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".

I see a disturbing gap between "EINVAL is not supposed to be even
possible" and "therefore it is safe to ignore".  Our usual attitude
towards catching errors is to ignore only specific things that are
expected to happen and known to be safe, e.g. the original code
before that patch which special cased to ignore EPIPE with

if (fflush(f)) {
if (errno == EPIPE)
exit(0);
die_errno("write failure on '%s'", desc);
}

where we know we are often downstream of something else and
diagnosing EPIPE as an error is actively wrong.

Unless you assume that *no* other platform has the same glitch as
Windows has with respect to fflush(f) returning EINVAL, or any other
platform that may return EINVAL from fflush(f) has the exactly same
glitch as Windows, ignoring EINVAL unconditionally everywhere is
wrong.  Perhaps the next broken platform may return EINVAL when it
should return EIO or something.  Ideally, that earlier workaround
should have done a logica equivalent of:


if (fflush(f)) {
#ifdef WINDOWS  
/*
 * On Windows, EPIPE is returned only by the first write()
 * after the reading end has closed its handle; subsequent
 * write()s return EINVAL.
 */
if (errno == EINVAL && this is after a second write)
errno = EPIPE;
#endif
if (errno == EPIPE)
exit(0);
die_errno("write failure on '%s'", desc);
}

and did so not in-line at the calling site but in a compat/ wrapper
for fflush() to eliminate the need for the ifdef.

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

I am not saying we should not deal with EINVAL on Windows at all.  I
am just saying ignoring EINVAL on other platforms is wrong, and
these two shouldn't have been mutually incompatible goals.
--
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/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/16/2012 8:39, schrieb 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.

The error is reproducible with this command on Windows:

$ git rev-list HEAD | sed 1q
42b333d8bb8709dfc5783dd4c44bdb6012c2c17d
fatal: write failure on 'stdout': Invalid argument

Let's do as you suggested.

--- 8< ---
From: Johannes Sixt 
Subject: [PATCH] maybe_flush_or_die: move a too-loose Windows specific error
 check to compat

Commit b2f5e268 (Windows: Work around an oddity when a pipe with no reader
is written to) introduced a check for EINVAL after fflush() to fight
spurious "Invalid argument" errors on Windows when a pipe was broken. But
this check may hide real errors on systems that do not have the this odd
behavior. Introduce an fflush wrapper in compat/mingw.* so that the treatment
is only applied on Windows.

Signed-off-by: Johannes Sixt 
---
 compat/mingw.c | 22 ++
 compat/mingw.h |  3 +++
 write_or_die.c |  7 +--
 3 files changed, 26 insertions(+), 6 deletions(-)

diff --git a/compat/mingw.c b/compat/mingw.c
index afc892d..4e63838 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -335,6 +335,28 @@ FILE *mingw_freopen (const char *filename, const char 
*otype, FILE *stream)
return freopen(filename, otype, stream);
 }
 
+#undef fflush
+int mingw_fflush(FILE *stream)
+{
+   int ret = fflush(stream);
+
+   /*
+* write() is used behind the scenes of stdio output functions.
+* Since git code does not check for errors after each stdio write
+* operation, it can happen that write() is called by a later
+* stdio function even if an earlier write() call failed. In the
+* case of a pipe whose readable end was closed, only the first
+* call to write() reports EPIPE on Windows. Subsequent write()
+* calls report EINVAL. It is impossible to notice whether this
+* fflush invocation triggered such a case, therefore, we have to
+* catch all EINVAL errors whole-sale.
+*/
+   if (ret && errno == EINVAL)
+   errno = EPIPE;
+
+   return ret;
+}
+
 /*
  * The unit of FILETIME is 100-nanoseconds since January 1, 1601, UTC.
  * Returns the 100-nanoseconds ("hekto nanoseconds") since the epoch.
diff --git a/compat/mingw.h b/compat/mingw.h
index 61a6521..eeb08d1 100644
--- a/compat/mingw.h
+++ b/compat/mingw.h
@@ -185,6 +185,9 @@ FILE *mingw_fopen (const char *filename, const char *otype);
 FILE *mingw_freopen (const char *filename, const char *otype, FILE *stream);
 #define freopen mingw_freopen
 
+int mingw_fflush(FILE *stream);
+#define fflush mingw_fflush
+
 char *mingw_getcwd(char *pointer, int len);
 #define getcwd mingw_getcwd
 
diff --git a/write_or_die.c b/write_or_die.c
index d45b536..960f448 100644
--- a/write_or_die.c
+++ b/write_or_die.c
@@ -34,12 +34,7 @@ void maybe_flush_or_die(FILE *f, const char *desc)
return;
}
if (fflush(f)) {
-   /*
-* On Windows, EPIPE is returned only by the first write()
-* after the reading end has closed its handle; subsequent
-* write()s return EINVAL.
-*/
-   if (errno == EPIPE || errno == EINVAL)
+   if (errno == EPIPE)
exit(0);
die_errno("write failure on '%s'", desc);
}
-- 
1.8.0.rc2.1248.g3f5b13f
--
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  writes:

> diff --git a/compat/mingw.c b/compat/mingw.c
> index afc892d..4e63838 100644
> --- a/compat/mingw.c
> +++ b/compat/mingw.c
> @@ -335,6 +335,28 @@ FILE *mingw_freopen (const char *filename, const char 
> *otype, FILE *stream)
>   return freopen(filename, otype, stream);
>  }
>  
> +#undef fflush
> +int mingw_fflush(FILE *stream)
> +{
> + int ret = fflush(stream);

The "#undef" above is a bit unfortunate.

Whenever I see this construct I start to wonder "I know this is to
disable our own #define we have elsewhere that renames fflush() to
mingw_fflush(), but what happens if the system include implements
fflush() as a macro?"

A better organization might be

 - make "int mingw_fflush(FILE *);" declaration available to all the
   callers and to this part of the file; and

 - make "#define fflush(x) mingw_fflush(x)" macro visible when
   compiling the rest of the system, but make it invisible to the
   implementation of the emulation function.

The latter implies that a function in the emulation layer, if it
needs to fflush(), would explicitly call mingw_fflush().

I know you did this knowing that it is not an issue on your
platform, and this file is only used on your platform anyway, so I
do not think we should address such a reorganization right now, but
it is something we may want to keep an eye on, as other people may
later try to stub away a real macro imitating this part of the code.

Thanks for following through.

Sometimes discussions on our list result in participant feeling
satisified with the conclusion without completing the last mile of
producing and applying the patch, which I find only after a few
month when I'm trawling the list archive for anything we missed.

Now I'll have to do my part and queue this to my tree ;-)
--
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