# Push not writing to standard error 10 messages from 2010-10-12 to 2010-10-18. Participants: Chase Brammer, Jonathan Nieder, Jeff King, Junio C Hamano, Scott R. Godin. Thread: https://gitlist.dev/t/25431 ## Chase Brammer, 2010-10-12 19:04 Subject: Push not writing to standard error Message-ID: URL: https://gitlist.dev/e/AANLkTim6j7cXj2-1JnKdNLb8KFJK86F02tzeByDBskEa%40mail.gmail.com ``` First time on the mailing list, but I enjoy the IRC channel.  Excuse me if this is a logged bug, or if there is a known workaround. When using git outside of bash, or 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).  This does however work just fine with git fetch. For example: git fetch origin master --progress > /fetch_error_ouput.txt 2>&1 Works just fine and writes a long file with the progress data. However, the following push doesn't write any data (even when pushing large data sets to verify progress output happens) git push origin master --progress > ~/push_error_output.txt 2>&1 As far as I can tell this is a bug with push.  I am a bit biased because I really need this feature, but it seems to me that this is a fairly large bug because pushing is such a pillar to all things git. Idea's on work arounds or upcoming patches to fix this? Thanks Chase Brammer ``` ## Jonathan Nieder, 2010-10-12 19:21 Subject: Re: Push not writing to standard error Message-ID: <20101012192117.GD16237@burratino> URL: https://gitlist.dev/e/20101012192117.GD16237%40burratino In-Reply-To: ``` 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. Thanks for the report. ``` ## Jeff King, 2010-10-12 19:32 Subject: Re: Push not writing to standard error Message-ID: <20101012193204.GA8620@sigill.intra.peff.net> URL: https://gitlist.dev/e/20101012193204.GA8620%40sigill.intra.peff.net In-Reply-To: <20101012192117.GD16237@burratino> ``` On Tue, Oct 12, 2010 at 02:21:17PM -0500, Jonathan Nieder wrote: > 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. -Peff ``` ## Jeff King, 2010-10-12 19:38 Subject: Re: Push not writing to standard error Message-ID: <20101012193830.GB8620@sigill.intra.peff.net> URL: https://gitlist.dev/e/20101012193830.GB8620%40sigill.intra.peff.net In-Reply-To: <20101012193204.GA8620@sigill.intra.peff.net> ``` 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); ``` ## Chase Brammer, 2010-10-12 20:37 Subject: Re: Push not writing to standard error Message-ID: URL: https://gitlist.dev/e/AANLkTim_pjJ76J0ctSQO%3DeYsVtkZAgq2nhm0fskjjo%2Bg%40mail.gmail.com In-Reply-To: <20101012193830.GB8620@sigill.intra.peff.net> ``` 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? Chase On Tue, Oct 12, 2010 at 1:38 PM, Jeff King wrote: > > 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); > ``` ## Jeff King, 2010-10-12 20:48 Subject: Re: Push not writing to standard error Message-ID: <20101012204845.GA12790@sigill.intra.peff.net> URL: https://gitlist.dev/e/20101012204845.GA12790%40sigill.intra.peff.net In-Reply-To: ``` 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 ``` ## Chase Brammer, 2010-10-12 22:18 Subject: Re: Push not writing to standard error Message-ID: URL: https://gitlist.dev/e/AANLkTikVE_iQ8nMdq7_G9aX17M3hQ4vM%3Doe2o%3DqynR-U%40mail.gmail.com In-Reply-To: <20101012204845.GA12790@sigill.intra.peff.net> ``` Peff Thanks for all the help. It worked fantastic. I hope you don't mind me packing this into a commit and submitting it to Junio. It is something I really need in the next release. I don't know much about protocol here, and I don't want to step on toes. Chase On Tue, Oct 12, 2010 at 2:48 PM, Jeff King wrote: > 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 > ``` ## Junio C Hamano, 2010-10-13 17:33 Subject: Re: Push not writing to standard error Message-ID: <7vzkuim1zx.fsf@alter.siamese.dyndns.org> URL: https://gitlist.dev/e/7vzkuim1zx.fsf%40alter.siamese.dyndns.org In-Reply-To: <20101012193830.GB8620@sigill.intra.peff.net> ``` Jeff King writes: > 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 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... > > 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); > ``` ## Jeff King, 2010-10-13 17:45 Subject: Re: Push not writing to standard error Message-ID: <20101013174543.GA13752@sigill.intra.peff.net> URL: https://gitlist.dev/e/20101013174543.GA13752%40sigill.intra.peff.net In-Reply-To: <7vzkuim1zx.fsf@alter.siamese.dyndns.org> ``` On Wed, Oct 13, 2010 at 10:33:22AM -0700, Junio C Hamano wrote: > > 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... Yes, it works in both of those cases. The transport code already does the right thing to set transport->progress (see the code at the end of transport_set_verbosity). And we even pass that value on to remote helpers, which presumably make use of it. But the internal git_transport_push simply ignored it (probably because it predates the rest of the transport code, but I didn't check). What concerns me a bit is that "git push --no-progress" does not do what I expected (turn off progress, but keep the status table which would otherwise be suppressed by "-q"). Instead, --no-progress is silently ignored. We should at least set it to NONEG to generate an error, but ideally we would handle it properly. However, that bug exists with or without my patch. The transport code seems to only ever consider "force progress" or "default progress", but never "no progress". -Peff ``` ## Scott R. Godin, 2010-10-18 16:39 Subject: Re: Push not writing to standard error Message-ID: <4CBC784A.1040805@mhg2.com> URL: https://gitlist.dev/e/4CBC784A.1040805%40mhg2.com In-Reply-To: ``` On 10/12/2010 03:04 PM, Chase Brammer wrote: > git fetch origin master --progress> /fetch_error_ouput.txt 2>&1 Just as a small tip, you can shorthand this in bash using git fech origin master --progress >& /fetch_error_output.txt HTH :) -- (please respond to the list as opposed to my email box directly, unless you are supplying private information you don't want public on the list) ```