[PATCH] git-update-index: report(...) now flushes stdout after printing the report line

[PATCH] git-update-index: report(...) now flushes stdout after printing the report line

[Sebastian Thiel]

This makes it equivalent to the behavior of git-hash-object and allows tools to
write one path
to stdin, flush and assure the work is done once it reads the corresponding
report line.
Previously attempting to do that would result in the program blocking as
git-update-index
did not flush its report line (yet). External programs use the git-hash-object
like behavior
to precisely control when which work is done while providing just-in-time
feedback to the end-user.
---
 builtin-update-index.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/builtin-update-index.c b/builtin-update-index.c
index 92beaaf..08bf933 100644
--- a/builtin-update-index.c
+++ b/builtin-update-index.c
@@ -37,6 +37,7 @@ static void report(const char *fmt, ...)
 	va_start(vp, fmt);
 	vprintf(fmt, vp);
 	putchar('\n');
+	maybe_flush_or_die(stdout, "line to stdout");
 	va_end(vp);
 }
 
-- 
1.6.5.3.172.g9e796

Re: [PATCH] git-update-index: report(...) now flushes stdout after printing the report line

[Nanako Shiraishi]

Junio, could you tell us what happened to this thread?

Re: [PATCH] git-update-index: report(...) now flushes stdout after printing the report line

[Junio C Hamano]

Nanako Shiraishi <nanako3@lavabit.com> writes:

> Junio, could you tell us what happened to this thread?

I didn't feel I had enough energy to read the commit log message after
seeing it was badly linewrapped and didn't have a sign-off, so I didn't
read it.

I've read it now; it is unclear from the proposed commit log message how
this fits in the larger picture.  Presumably this change is meant to be
useful when driving update-index through --stdin?  To see if I got the
intention right, let me try paraphrasing it...

    update-index: flush standard output after each action is reported

    A scripted Porcelain that runs "git update-index --stdin" might want
    to use a bidirectional pipe, while feeding one path at a time and
    reading the output from report() every time after feeding a path.

    Such a Porcelain would deadlock if the standard output is not flushed
    after report().

I don't know if the above is what Sebastian meant, though..

An obvious question, when phrased this way, is "what impact does this
change have for scripted Porcelains that don't use bi-di pipe?"  I think
the answer would be "The I/O overhead for flushing would increase", but I
don't know if it would be "... would increase but it is still negligible"
or "... would increase too much to make it noticeably or unusably slow
especially if it feeds hundreds of paths".  If it is the latter, this may
need to be controlled by another command line option.

Sebastian, care to redo the justification, make it a bit more readable,
and add your sign-off?

Thanks.

Re: [PATCH] git-update-index: report(...) now flushes stdout after printing the report line

[Sebastian Thiel]

I'd like to add that since version 1.6.5, non-tty's do not receive any progress
information anymore. The patch causing this says it wants to, in short words,
unify the push and fetch handling regarding the way progress messages are sent.

Now third-party wrappers, such as git-python, are unable to provide any progress
information anymore for possibly lengthy operations.

This is why I clearly recommend to add some kind of a "progress-force" flag that
turns progress messages on again for send-pack and receive-pack.

Re: [PATCH] git-update-index: report(...) now flushes stdout after printing the report line

[Sebastian Thiel]

Sorry for the badly formatted message, and thanks a lot for the correction which
is what my post should have been in the first place.

Redoing the commit is not what will be needed for git-python to work properly
which is why I will tell the whole story before submitting any(more) patches.

With git v1.6.5, git-push was adjusted not to provide progress messages anymore
if the device attached to stderr is not a tty. Previously, this was only the
case with git-fetch. For git-python, and other callers of the commandline such
as tortoise-git, there currently is no way to provide progress information to
the user unless they (somehow) simulate a tty which appears unfeasible. When
using these tools, time consuming operations tend to appear as if they are
hanging. One might argue that most code is fetched and pushed in a matter of
seconds, but if git is used to store large binary data, processing and
transferring it will take time.

The issue mentioned with git-update-index and it's missing flush that would
cause a deadlock in some porcelain can be fixed trivially, but seen in the
context of the git-push and git-pull a more thorough solution might be more
appealing. As mentioned by Junio, a default flush after each report might slow
down some existing porcelain, and a commandline option would be part of the
proper solution. I would argue though that a separate option would add quite
some complexity to the command as it is a very specialized one. Instead I would
recommend checking whether --stdin is given on the commandline, and flush stdout
if that is true. This would natively make the command behave like
git-hash-object and git-cat-file. If --stdin is not provided, report is not
required to flush after every call as the commandline options are processed
without additional user interaction.

Adding a commandline option to git-push and git-pull that enforces progress
messages to be printed to stderr would be a feasible and simple fix that would
clearly improve the usability of tortoise-git and git-python to name only two.

That said, I hope I managed to make myself clear enough this time to help the
people in charge to figure out how to solve the issue. Once the desired solution
has been sketched out and the desired new commandline options have been named,
it could even be me to implement it if necessary, as I'd consider it a gentle
start into the world of the git codebase.

Thanks for picking this up again,
Sebastian

Re: [PATCH] git-update-index: report(...) now flushes stdout after printing the report line

[Tay Ray Chuan]

Hi,

(you dropped the Cc list; fixed that for you.)

On Sun, Jan 3, 2010 at 6:41 PM, Sebastian Thiel <byronimo@gmail.com> wrote:
> Adding a commandline option to git-push and git-pull that enforces progress
> messages to be printed to stderr would be a feasible and simple fix that would
> clearly improve the usability of tortoise-git and git-python to name only two.
>
> That said, I hope I managed to make myself clear enough this time to help the
> people in charge to figure out how to solve the issue. Once the desired solution
> has been sketched out and the desired new commandline options have been named,
> it could even be me to implement it if necessary, as I'd consider it a gentle
> start into the world of the git codebase.

from your above message solely and setting aside your original patch,
I presume that you want to introduce the ability to force progress
reporting even if stderr isn't a terminal.

I am working a feature (display progress for http operations) that
happens to add this ability to git-push and git-fetch, by specifying
the --progress option.

Regarding git-pull - I guess it's only git-fetch (being
transport-related) that reports progress?

-- 
Cheers,
Ray Chuan

Re: [PATCH] git-update-index: report(...) now flushes stdout after printing the report line

[Sebastian Thiel]

Thanks Ray Chuan, for the clarification, the progress is supposed to
be sent in git-push and git-fetch ( not git-pull as I mentioned ).
A --progress flag would already do it for me, is there a way to fetch
your code from somewhere for a test run ?

When do you think will your changes be available for a mainline merge,
or would it even be possible to separate the --progress adjustment
from your feature to merge it into mainline individually ?

Thanks,
Sebastian

Re: [PATCH] git-update-index: report(...) now flushes stdout after printing the report line

[Junio C Hamano]

Tay Ray Chuan <rctay89@gmail.com> writes:

> from your above message solely and setting aside your original patch,
> I presume that you want to introduce the ability to force progress
> reporting even if stderr isn't a terminal.
>
> I am working a feature (display progress for http operations) that
> happens to add this ability to git-push and git-fetch, by specifying
> the --progress option.
>
> Regarding git-pull - I guess it's only git-fetch (being
> transport-related) that reports progress?

Are you talking about this topic?

 * tc/clone-v-progress (2009-12-26) 4 commits
  - clone: use --progress to force progress reporting
  - clone: set transport->verbose when -v/--verbose is used
  - git-clone.txt: reword description of progress behaviour
  - check stderr with isatty() instead of stdout when deciding to show progress

What do people think about it?  I vaguely recall that somebody asked to
add a warning to release notes on the behaviour change to this series, and
I think it may be a worthwhile thing to do (e.g. "Earlier we did X but now
we do Y; change things in this way if you want us to keep doing X"), but
otherwise I think it is a sensible change.

Re: [PATCH] git-update-index: report(...) now flushes stdout after printing the report line

[Tay Ray Chuan]

Hi,

On Wed, Jan 6, 2010 at 9:04 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Tay Ray Chuan <rctay89@gmail.com> writes:
>
>> from your above message solely and setting aside your original patch,
>> I presume that you want to introduce the ability to force progress
>> reporting even if stderr isn't a terminal.
>>
>> I am working a feature (display progress for http operations) that
>> happens to add this ability to git-push and git-fetch, by specifying
>> the --progress option.
>>
>> Regarding git-pull - I guess it's only git-fetch (being
>> transport-related) that reports progress?
>
> Are you talking about this topic?
>
>  * tc/clone-v-progress (2009-12-26) 4 commits
>  - clone: use --progress to force progress reporting
>  - clone: set transport->verbose when -v/--verbose is used
>  - git-clone.txt: reword description of progress behaviour
>  - check stderr with isatty() instead of stdout when deciding to show progress

no, I'm not referring to that - the topic I mentioned is still off-list.

> What do people think about it?  I vaguely recall that somebody asked to
> add a warning to release notes on the behaviour change to this series, and
> I think it may be a worthwhile thing to do (e.g. "Earlier we did X but now
> we do Y; change things in this way if you want us to keep doing X"), but
> otherwise I think it is a sensible change.

Yes, that request was from Dscho, and Miklos said something to that
effect as well. Could you advise how one could go about adding such a
warning, as I'm not sure about the release schedule details.

-- 
Cheers,
Ray Chuan