From: Junio C Hamano <hidden> Date: 2016-06-15 23:04:43
Mike Hommey [off-list ref] 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.
From: Mike Hommey <hidden> Date: 2016-06-15 23:04:43
On Mon, May 11, 2015 at 07:04:20PM -0700, Junio C Hamano wrote:
Mike Hommey [off-list ref] writes:
quoted
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
From: Mike Hommey <hidden> Date: 2016-06-15 23:04:44
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.
From: Eric Sunshine <hidden> Date: 2016-06-15 23:04:44
On Mon, May 11, 2015 at 11:12 PM, Mike Hommey [off-list ref] 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.
From: Mike Hommey <hidden> Date: 2016-06-15 23:04:44
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 <redacted>
---
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.