Bug: version 2.4 seems to have broken `git clone --progress`

Bug: version 2.4 seems to have broken `git clone --progress`

[Jack O'Connor]

In git 2.3.7 I could run the following command and see progress in the
terminal, despite the redirection of stdout and stderr:

    git clone https://github.com/oconnor663/dotfiles --progress 2>&1 | cat

As of 2.4, that command no longer shows progress. When I bisect, the
responsible commit is 2879bc3b0c3acc89f0415ac0d0e3946599d9fc88
("transport-helper: ask the helper to set progress and verbosity
options after asking for its capabilities"). Can anyone suggest a
workaround?

-- Jack O'Connor

Re: Bug: version 2.4 seems to have broken `git clone --progress`

[Junio C Hamano]

"Jack O'Connor" <oconnor663@gmail.com> writes:

> In git 2.3.7 I could run the following command and see progress in the
> terminal, despite the redirection of stdout and stderr:
>
>     git clone https://github.com/oconnor663/dotfiles --progress 2>&1 | cat
>
> As of 2.4, that command no longer shows progress. When I bisect, the
> responsible commit is 2879bc3b0c3acc89f0415ac0d0e3946599d9fc88
> ("transport-helper: ask the helper to set progress and verbosity
> options after asking for its capabilities"). Can anyone suggest a
> workaround?
>
> -- Jack O'Connor

That commit is by Mike Hommey <mh@glandium.org> so I'd imagine that
CC'ing the author of the patch would be the first thing to do ;-)

I am kind of surprised that the commit changes behaviour, though.
If I didn't hear that you had trouble in 2.4, I would have suspected
85cb8906 (progress: no progress in background, 2015-04-13) instead.

Re: Bug: version 2.4 seems to have broken `git clone --progress`

[Mike Hommey]

On Mon, May 11, 2015 at 02:06:15PM -0700, Junio C Hamano wrote:
> "Jack O'Connor" <oconnor663@gmail.com> writes:
> 
> > In git 2.3.7 I could run the following command and see progress in the
> > terminal, despite the redirection of stdout and stderr:
> >
> >     git clone https://github.com/oconnor663/dotfiles --progress 2>&1 | cat
> >
> > As of 2.4, that command no longer shows progress. When I bisect, the
> > responsible commit is 2879bc3b0c3acc89f0415ac0d0e3946599d9fc88
> > ("transport-helper: ask the helper to set progress and verbosity
> > options after asking for its capabilities"). Can anyone suggest a
> > workaround?
> >
> > -- Jack O'Connor
> 
> That commit is by Mike Hommey <mh@glandium.org> so I'd imagine that
> CC'ing the author of the patch would be the first thing to do ;-)
> 
> I am kind of surprised that the commit changes behaviour, though.
> If I didn't hear that you had trouble in 2.4, I would have suspected
> 85cb8906 (progress: no progress in background, 2015-04-13) instead.

So, the reason this is happening is that 2879bc3 moved sending the
progress helper option earlier, and for clone, it's early enough that
transport_set_verbosity happens afterwards. Since
transport_set_verbosity only sets the progress bit, and nothing re-emits
a helper option command when it changes, we're left with the default,
which is that no progress is shown if the output file descripto is not
a tty.

I can see two ways to fix this:
- Make transport_set_verbosity call transport->set_option instead of
  defering to standard_options() in transport-helper.c.
- Declare that transport_set_verbosity must be used before any other
  transport_set_option, and change clone to invoke it first. Note that
  fetch and push already do that, so this is only currently a problem
  for clone.

Junio, what do you think?

Mike

Re: Bug: version 2.4 seems to have broken `git clone --progress`

[Junio C Hamano]

Mike Hommey <mh@glandium.org> writes:

> So, the reason this is happening is that 2879bc3 moved sending the
> progress helper option earlier, and for clone, it's early enough that
> transport_set_verbosity happens afterwards. Since
> transport_set_verbosity only sets the progress bit, and nothing re-emits
> a helper option command when it changes, we're left with the default,
> which is that no progress is shown if the output file descripto is not
> a tty.
>
> I can see two ways to fix this:
> - Make transport_set_verbosity call transport->set_option instead of
>   defering to standard_options() in transport-helper.c.
> - Declare that transport_set_verbosity must be used before any other
>   transport_set_option, and change clone to invoke it first. Note that
>   fetch and push already do that, so this is only currently a problem
>   for clone.
>
> Junio, what do you think?

The latter sounds like more appropriate as a lower-impact short-term
fix, so let's have that for now.

I however wonder if there are other settings that can be flipped
after we started talking to the helper to cause a similar issue,
and to prevent such breakages once and for all, we may have to
take the former route in the longer term.  But I think that can be
done later after the dust settles.

Thanks for a quick diagnosis.

Re: Bug: version 2.4 seems to have broken `git clone --progress`

[Mike Hommey]

On Mon, May 11, 2015 at 07:04:20PM -0700, Junio C Hamano wrote:
> Mike Hommey <mh@glandium.org> writes:
> 
> > So, the reason this is happening is that 2879bc3 moved sending the
> > progress helper option earlier, and for clone, it's early enough that
> > transport_set_verbosity happens afterwards. Since
> > transport_set_verbosity only sets the progress bit, and nothing re-emits
> > a helper option command when it changes, we're left with the default,
> > which is that no progress is shown if the output file descripto is not
> > a tty.
> >
> > I can see two ways to fix this:
> > - Make transport_set_verbosity call transport->set_option instead of
> >   defering to standard_options() in transport-helper.c.
> > - Declare that transport_set_verbosity must be used before any other
> >   transport_set_option, and change clone to invoke it first. Note that
> >   fetch and push already do that, so this is only currently a problem
> >   for clone.
> >
> > Junio, what do you think?
> 
> The latter sounds like more appropriate as a lower-impact short-term
> fix, so let's have that for now.
> 
> I however wonder if there are other settings that can be flipped
> after we started talking to the helper to cause a similar issue,
> and to prevent such breakages once and for all, we may have to
> take the former route in the longer term.  But I think that can be
> done later after the dust settles.

AFAICT, verbosity/progress is the only thing that is really treated
differently. transport_set_options always sends options to the remote
helper wire.

Mike

[PATCH] clone: call transport_set_verbosity before anything else on the newly created transport

[Mike Hommey]

Commit 2879bc3 made the progress and verbosity options sent to remote helper
earlier than they previously were. But nothing else after that would send
updates if the value is changed later on with transport_set_verbosity.

While for fetch and push, transport_set_verbosity is the first thing that
is done after creating the transport, it was not the case for clone. So
commit 2879bc3 broke changing progress and verbosity for clone, for urls
requiring a remote helper only (so, not git:// urls, for instance).

Moving transport_set_verbosity to just after the transport is created
works around the issue.
---
 builtin/clone.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Note that another long term way to fix this would be to move setting
verbosity and progress to the transport_get call itself.

The patch is against v2.4.0, if that matters.

diff --git a/builtin/clone.c b/builtin/clone.c
index 53a2e5a..13030ee 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -906,6 +906,8 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 
 	remote = remote_get(option_origin);
 	transport = transport_get(remote, remote->url[0]);
+	transport_set_verbosity(transport, option_verbosity, option_progress);
+
 	path = get_repo_path(remote->url[0], &is_bundle);
 	is_local = option_local != 0 && path && !is_bundle;
 	if (is_local) {
@@ -932,8 +934,6 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 	if (option_single_branch)
 		transport_set_option(transport, TRANS_OPT_FOLLOWTAGS, "1");
 
-	transport_set_verbosity(transport, option_verbosity, option_progress);
-
 	if (option_upload_pack)
 		transport_set_option(transport, TRANS_OPT_UPLOADPACK,
 				     option_upload_pack);
-- 
2.4.0.1.gde5e018

Re: [PATCH] clone: call transport_set_verbosity before anything else on the newly created transport

[Eric Sunshine]

On Mon, May 11, 2015 at 11:12 PM, Mike Hommey <mh@glandium.org> wrote:
> Commit 2879bc3 made the progress and verbosity options sent to remote helper
> earlier than they previously were. But nothing else after that would send
> updates if the value is changed later on with transport_set_verbosity.
>
> While for fetch and push, transport_set_verbosity is the first thing that
> is done after creating the transport, it was not the case for clone. So
> commit 2879bc3 broke changing progress and verbosity for clone, for urls
> requiring a remote helper only (so, not git:// urls, for instance).
>
> Moving transport_set_verbosity to just after the transport is created
> works around the issue.

Missing sign-off.

> ---
> diff --git a/builtin/clone.c b/builtin/clone.c
> index 53a2e5a..13030ee 100644
> --- a/builtin/clone.c
> +++ b/builtin/clone.c
> @@ -906,6 +906,8 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
>
>         remote = remote_get(option_origin);
>         transport = transport_get(remote, remote->url[0]);
> +       transport_set_verbosity(transport, option_verbosity, option_progress);
> +
>         path = get_repo_path(remote->url[0], &is_bundle);
>         is_local = option_local != 0 && path && !is_bundle;
>         if (is_local) {
> @@ -932,8 +934,6 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
>         if (option_single_branch)
>                 transport_set_option(transport, TRANS_OPT_FOLLOWTAGS, "1");
>
> -       transport_set_verbosity(transport, option_verbosity, option_progress);
> -
>         if (option_upload_pack)
>                 transport_set_option(transport, TRANS_OPT_UPLOADPACK,
>                                      option_upload_pack);
> --
> 2.4.0.1.gde5e018

[PATCH] clone: call transport_set_verbosity before anything else on the newly created transport

[Mike Hommey]

Commit 2879bc3 made the progress and verbosity options sent to remote helper
earlier than they previously were. But nothing else after that would send
updates if the value is changed later on with transport_set_verbosity.

While for fetch and push, transport_set_verbosity is the first thing that
is done after creating the transport, it was not the case for clone. So
commit 2879bc3 broke changing progress and verbosity for clone, for urls
requiring a remote helper only (so, not git:// urls, for instance).

Moving transport_set_verbosity to just after the transport is created
works around the issue.

Signed-off-by: Mike Hommey <mh@glandium.org>
---
 builtin/clone.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Note that another long term way to fix this would be to move setting
verbosity and progress to the transport_get call itself.

The patch is against v2.4.0, if that matters.

diff --git a/builtin/clone.c b/builtin/clone.c
index 53a2e5a..13030ee 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -906,6 +906,8 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 
 	remote = remote_get(option_origin);
 	transport = transport_get(remote, remote->url[0]);
+	transport_set_verbosity(transport, option_verbosity, option_progress);
+
 	path = get_repo_path(remote->url[0], &is_bundle);
 	is_local = option_local != 0 && path && !is_bundle;
 	if (is_local) {
@@ -932,8 +934,6 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 	if (option_single_branch)
 		transport_set_option(transport, TRANS_OPT_FOLLOWTAGS, "1");
 
-	transport_set_verbosity(transport, option_verbosity, option_progress);
-
 	if (option_upload_pack)
 		transport_set_option(transport, TRANS_OPT_UPLOADPACK,
 				     option_upload_pack);
-- 
2.4.0.1.gde5e018