Push not writing to standard error

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

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.

On Tue, Oct 12, 2010 at 02:21:17PM -0500, Jonathan Nieder wrote:

Chase Brammer wrote:

quoted

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

[...]

quoted

git push origin master --progress > ~/push_error_output.txt 2>&1

[...]

quoted

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

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

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 [off-list ref] 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

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 [off-list ref] wrote:

On Tue, Oct 12, 2010 at 02:37:50PM -0600, Chase Brammer wrote:

quoted

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