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

[Jeff King]

On Mon, Apr 08, 2013 at 07:13:11PM +0100, Adam Spiers wrote:

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

I think flushing on each line is reasonable, though you are also
introducing a deadlock possibility for callers which do not read back
the output in real-time. For example, if I write N paths out then read N
ignore-lines back in, I risk a situation where I am blocked on write()
to check-ignore, and it is blocked on write back to me. Somebody has to
buffer (the pipe buffers give you some leeway, but they are limited).

Given how new check-ignore is, and that we have not advertised any
particular buffering scheme so far, it's probably OK to switch without
worrying about breaking existing callers.

But if this is a mode of operation that we expect people to use (here
and for check-attr), we should advertise the flushing behavior, and
probably warn about the deadlock (I don't think adding a "--no-flush"
option is worth it, as it would just mean buffering in check-ignore,
which the caller could just as easily do itself).

-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: RFC: two minor tweaks to check-ignore to help git-annex assistant

[Junio C Hamano]

Adam Spiers  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