Push not writing to standard error

> Chase Brammer wrote:
> 
> >                                    saving the standard error from bash
> > to a file during a push doesn't seem to be working.  I am only able to
> > get standard output, which doesn't give the progress of the push
> > (counting, delta, compressing, and writing status).
> [...]
> > git push origin master --progress > ~/push_error_output.txt 2>&1
> [...]
> > Idea's on work arounds or upcoming patches to fix this?
> 
> None from me.  But some hints for a patch:
> 
>  - As the man page says,
> 
>    --progress
> 
> 	Progress status is reported on the standard error stream
> 	by default when it is attached to a terminal, unless -q is
> 	specified. This flag forces progress status even if the
> 	standard error stream is not directed to a terminal.
> 
>    It looks like this facility is not working.
> 
>  - Terminals are distinguished from nonterminals with isatty()
> 
>  - The "Counting objects..." output comes from pack-objects.
>    Running with GIT_TRACE=1 reveals that the --progress option is
>    not being passed to pack-objects as it should be.
> 
>  - Is this a regression?  If so, narrowing the regression window
>    with a few rounds of "git bisect" could be helpful.

> It looks like transport_set_verbosity gets called correctly, and then
> sets the "progress" flag for the transport. But for the push side, I
> don't see any transports actually looking at that flag. I think there
> needs to be code in git_transport_push to handle the progress flag, and
> it just isn't there.

>
> On Tue, Oct 12, 2010 at 03:32:04PM -0400, Jeff King wrote:
>
> > It looks like transport_set_verbosity gets called correctly, and then
> > sets the "progress" flag for the transport. But for the push side, I
> > don't see any transports actually looking at that flag. I think there
> > needs to be code in git_transport_push to handle the progress flag, and
> > it just isn't there.
>
> Here's a quick 5-minute patch. It works on my test case:
>
>  rm -rf parent child
>  git init parent &&
>  git clone parent child &&
>  cd child &&
>  echo content >file && git add file && git commit -m one &&
>  git push --progress origin master:foo >foo.out 2>&1 &&
>  cat foo.out
>
> but I didn't even run the test suite. Maybe somebody more clueful in the
> area can pick it up?
>
> diff --git a/builtin/send-pack.c b/builtin/send-pack.c
> index 481602d..efd9be6 100644
> --- a/builtin/send-pack.c
> +++ b/builtin/send-pack.c
> @@ -48,6 +48,7 @@ static int pack_objects(int fd, struct ref *refs, struct extra_have_objects *ext
>                NULL,
>                NULL,
>                NULL,
> +               NULL,
>        };
>        struct child_process po;
>        int i;
> @@ -59,6 +60,8 @@ static int pack_objects(int fd, struct ref *refs, struct extra_have_objects *ext
>                argv[i++] = "--delta-base-offset";
>        if (args->quiet)
>                argv[i++] = "-q";
> +       if (args->progress)
> +               argv[i++] = "--progress";
>        memset(&po, 0, sizeof(po));
>        po.argv = argv;
>        po.in = -1;
> diff --git a/send-pack.h b/send-pack.h
> index 60b4ba6..fcf4707 100644
> --- a/send-pack.h
> +++ b/send-pack.h
> @@ -4,6 +4,7 @@
>  struct send_pack_args {
>        unsigned verbose:1,
>                quiet:1,
> +               progress:1,
>                porcelain:1,
>                send_mirror:1,
>                force_update:1,
> diff --git a/transport.c b/transport.c
> index 4dba6f8..0078660 100644
> --- a/transport.c
> +++ b/transport.c
> @@ -789,6 +789,7 @@ static int git_transport_push(struct transport *transport, struct ref *remote_re
>        args.use_thin_pack = data->options.thin;
>        args.verbose = (transport->verbose > 0);
>        args.quiet = (transport->verbose < 0);
> +       args.progress = transport->progress;
>        args.dry_run = !!(flags & TRANSPORT_PUSH_DRY_RUN);
>        args.porcelain = !!(flags & TRANSPORT_PUSH_PORCELAIN);
>

> On Tue, Oct 12, 2010 at 02:37:50PM -0600, Chase Brammer wrote:
>
>> Wow, I am amazed at how quick you churned that out.  I haven't
>> participated in the git patch and release cycle, so forgive my
>> ignorance.  Do you think that this will be released in the next
>> release (1.7.3.2) ? If so, any expectations on release date?
>
> Well, at 5 minutes it was really only 1 line of code per minute. ;)
>
> I'm hoping that somebody else on the list who has worked in the
> transport code recently can comment on whether this is the right fix.
> Did you test it? Does it fix your issue?
>
> If it seems OK, then somebody needs to submit a cleaned-up version with
> commit message to Junio, who will probably cook it in "next" for at
> least a few weeks, and then hopefully it would be in v1.7.3.2. He does
> maintenance releases as-needed, which seems to generally be every few
> weeks.
>
> -Peff
>

> On Tue, Oct 12, 2010 at 03:32:04PM -0400, Jeff King wrote:
>
>> It looks like transport_set_verbosity gets called correctly, and then
>> sets the "progress" flag for the transport. But for the push side, I
>> don't see any transports actually looking at that flag. I think there
>> needs to be code in git_transport_push to handle the progress flag, and
>> it just isn't there.
>
> Here's a quick 5-minute patch. It works on my test case:
>
>   rm -rf parent child
>   git init parent &&
>   git clone parent child &&
>   cd child &&
>   echo content >file && git add file && git commit -m one &&
>   git push --progress origin master:foo >foo.out 2>&1 &&
>   cat foo.out

>
> but I didn't even run the test suite. Maybe somebody more clueful in the
> area can pick it up?
>
> diff --git a/builtin/send-pack.c b/builtin/send-pack.c
> index 481602d..efd9be6 100644
> --- a/builtin/send-pack.c
> +++ b/builtin/send-pack.c
> @@ -48,6 +48,7 @@ static int pack_objects(int fd, struct ref *refs, struct extra_have_objects *ext
>  		NULL,
>  		NULL,
>  		NULL,
> +		NULL,
>  	};
>  	struct child_process po;
>  	int i;
> @@ -59,6 +60,8 @@ static int pack_objects(int fd, struct ref *refs, struct extra_have_objects *ext
>  		argv[i++] = "--delta-base-offset";
>  	if (args->quiet)
>  		argv[i++] = "-q";
> +	if (args->progress)
> +		argv[i++] = "--progress";
>  	memset(&po, 0, sizeof(po));
>  	po.argv = argv;
>  	po.in = -1;
> diff --git a/send-pack.h b/send-pack.h
> index 60b4ba6..fcf4707 100644
> --- a/send-pack.h
> +++ b/send-pack.h
> @@ -4,6 +4,7 @@
>  struct send_pack_args {
>  	unsigned verbose:1,
>  		quiet:1,
> +		progress:1,
>  		porcelain:1,
>  		send_mirror:1,
>  		force_update:1,
> diff --git a/transport.c b/transport.c
> index 4dba6f8..0078660 100644
> --- a/transport.c
> +++ b/transport.c
> @@ -789,6 +789,7 @@ static int git_transport_push(struct transport *transport, struct ref *remote_re
>  	args.use_thin_pack = data->options.thin;
>  	args.verbose = (transport->verbose > 0);
>  	args.quiet = (transport->verbose < 0);
> +	args.progress = transport->progress;
>  	args.dry_run = !!(flags & TRANSPORT_PUSH_DRY_RUN);
>  	args.porcelain = !!(flags & TRANSPORT_PUSH_PORCELAIN);
>  

> > Here's a quick 5-minute patch. It works on my test case:
> >
> >   rm -rf parent child
> >   git init parent &&
> >   git clone parent child &&
> >   cd child &&
> >   echo content >file && git add file && git commit -m one &&
> >   git push --progress origin master:foo >foo.out 2>&1 &&
> >   cat foo.out
> 
> Does it still work with "git push" without --progress?  I didn't apply nor
> test, but just wondering as the manpage description suggests progress is
> implicitly set when standard error is terminal even when there is no
> command line --progress is given, and also interaction with -q option, but
> the patch does not seem to show such subtleties...