[v5,13/17] read-tree: show progress by default
diff mbox series

Message ID a229e1ee0cb96c5f8c2c5d430641c386bc082a2d.1571666187.git.gitgitgadget@gmail.com
State New
Headers show
Series
  • New sparse-checkout builtin and "cone" mode
Related show

Commit Message

James via GitGitGadget Oct. 21, 2019, 1:56 p.m. UTC
From: Derrick Stolee <dstolee@microsoft.com>

The read-tree builtin has a --verbose option that signals to show
progress and other data while updating the index. Update this to
be on by default when stderr is a terminal window.

This will help tools like 'git sparse-checkout' to automatically
benefit from progress indicators when a user runs these commands.

Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
---
 builtin/read-tree.c | 1 +
 1 file changed, 1 insertion(+)

Comments

Phillip Wood Oct. 21, 2019, 3:04 p.m. UTC | #1
Hi Stolee

On 21/10/2019 14:56, Derrick Stolee via GitGitGadget wrote:
> From: Derrick Stolee <dstolee@microsoft.com>
> 
> The read-tree builtin has a --verbose option that signals to show
> progress and other data while updating the index. Update this to
> be on by default when stderr is a terminal window.
> 
> This will help tools like 'git sparse-checkout' to automatically
> benefit from progress indicators when a user runs these commands.
> 
> Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
> ---
>   builtin/read-tree.c | 1 +
>   1 file changed, 1 insertion(+)
> 
> diff --git a/builtin/read-tree.c b/builtin/read-tree.c
> index ca5e655d2f..69963d83dc 100644
> --- a/builtin/read-tree.c
> +++ b/builtin/read-tree.c
> @@ -162,6 +162,7 @@ int cmd_read_tree(int argc, const char **argv, const char *cmd_prefix)
>   	opts.head_idx = -1;
>   	opts.src_index = &the_index;
>   	opts.dst_index = &the_index;
> +	opts.verbose_update = isatty(2);

I'm slightly wary of changing the output of plumbing commands like this. 
If a script wants progress output it can already get it by passing 
--verbose. With this change a script that does not want that output now 
has to pass --no-verbose. If 'git sparse-checkout' wants progress 
indicators why not put the isatty() check there and pass the appropriate 
option to read-tree?

Best Wishes

Phillip

>   	git_config(git_read_tree_config, NULL);
>   
>
Derrick Stolee Oct. 21, 2019, 3:14 p.m. UTC | #2
On 10/21/2019 11:04 AM, Phillip Wood wrote:
> Hi Stolee
> 
> On 21/10/2019 14:56, Derrick Stolee via GitGitGadget wrote:
>> From: Derrick Stolee <dstolee@microsoft.com>
>>
>> The read-tree builtin has a --verbose option that signals to show
>> progress and other data while updating the index. Update this to
>> be on by default when stderr is a terminal window.
>>
>> This will help tools like 'git sparse-checkout' to automatically
>> benefit from progress indicators when a user runs these commands.
>>
>> Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
>> ---
>>   builtin/read-tree.c | 1 +
>>   1 file changed, 1 insertion(+)
>>
>> diff --git a/builtin/read-tree.c b/builtin/read-tree.c
>> index ca5e655d2f..69963d83dc 100644
>> --- a/builtin/read-tree.c
>> +++ b/builtin/read-tree.c
>> @@ -162,6 +162,7 @@ int cmd_read_tree(int argc, const char **argv, const char *cmd_prefix)
>>       opts.head_idx = -1;
>>       opts.src_index = &the_index;
>>       opts.dst_index = &the_index;
>> +    opts.verbose_update = isatty(2);
> 
> I'm slightly wary of changing the output of plumbing commands like this. If a script wants progress output it can already get it by passing --verbose. With this change a script that does not want that output now has to pass --no-verbose.

If a script is calling this, then won't stderr not be a terminal window, and
isatty(2) return 0? Or, if the script is run with stderr passing through to
a terminal, then the user would see progress while running the script, which
seems like a side-effect but not one that will cause a broken script.

> If 'git sparse-checkout' wants progress indicators why not put the isatty() check there and pass the appropriate option to read-tree?

It is not necessary for the sparse-checkout builtin by the end of the series,
as the extra process is removed and unpack_trees() is called directly. However,
it does seem like a helpful user-interaction for people still using the "old"
way of interacting with the sparse-checkout feature.

Thanks,
-Stolee
Junio C Hamano Oct. 23, 2019, 3:48 a.m. UTC | #3
Derrick Stolee <stolee@gmail.com> writes:

>> I'm slightly wary of changing the output of plumbing commands
>> like this. If a script wants progress output it can already get
>> it by passing --verbose. With this change a script that does not
>> want that output now has to pass --no-verbose.
>
> If a script is calling this, then won't stderr not be a terminal window, and
> isatty(2) return 0?

Unless the script tries to capture the error output and react
differently depending on the error message from the plumbing (which
is not localized), iow most of the time, standard error stream is
left unredirected and likely to be connected to the terminal if the
script is driven from a terminal command line.

> Or, if the script is run with stderr passing through to
> a terminal, then the user would see progress while running the script, which
> seems like a side-effect but not one that will cause a broken script.

It will show unwanted output to the end users, no?  That is the
complaint about having to pass --no-verbose, if I understand
correctly, if the script does not want to show the progress output.
Derrick Stolee Oct. 23, 2019, 12:50 p.m. UTC | #4
On 10/22/2019 11:48 PM, Junio C Hamano wrote:
> Derrick Stolee <stolee@gmail.com> writes:
> 
>>> I'm slightly wary of changing the output of plumbing commands
>>> like this. If a script wants progress output it can already get
>>> it by passing --verbose. With this change a script that does not
>>> want that output now has to pass --no-verbose.
>>
>> If a script is calling this, then won't stderr not be a terminal window, and
>> isatty(2) return 0?
> 
> Unless the script tries to capture the error output and react
> differently depending on the error message from the plumbing (which
> is not localized), iow most of the time, standard error stream is
> left unredirected and likely to be connected to the terminal if the
> script is driven from a terminal command line.
> 
>> Or, if the script is run with stderr passing through to
>> a terminal, then the user would see progress while running the script, which
>> seems like a side-effect but not one that will cause a broken script.
> 
> It will show unwanted output to the end users, no?  That is the
> complaint about having to pass --no-verbose, if I understand
> correctly, if the script does not want to show the progress output.

I'm happy to have attempted the change and start this discussion. It
sounds like this one patch could be ejected to no loss to the full
series.

Thanks,
-Stolee
Phillip Wood Oct. 24, 2019, 10:18 a.m. UTC | #5
On 23/10/2019 04:48, Junio C Hamano wrote:
> Derrick Stolee <stolee@gmail.com> writes:
> 
>>> I'm slightly wary of changing the output of plumbing commands
>>> like this. If a script wants progress output it can already get
>>> it by passing --verbose. With this change a script that does not
>>> want that output now has to pass --no-verbose.
>>
>> If a script is calling this, then won't stderr not be a terminal window, and
>> isatty(2) return 0?
> 
> Unless the script tries to capture the error output and react
> differently depending on the error message from the plumbing (which
> is not localized), iow most of the time, standard error stream is
> left unredirected and likely to be connected to the terminal if the
> script is driven from a terminal command line.
> 
>> Or, if the script is run with stderr passing through to
>> a terminal, then the user would see progress while running the script, which
>> seems like a side-effect but not one that will cause a broken script.
> 
> It will show unwanted output to the end users, no?  That is the
> complaint about having to pass --no-verbose, if I understand
> correctly, if the script does not want to show the progress output.

Yes that was my objection.

Best Wishes

Phillip
Phillip Wood Oct. 24, 2019, 10:18 a.m. UTC | #6
Hi Stolee

On 23/10/2019 13:50, Derrick Stolee wrote:
> On 10/22/2019 11:48 PM, Junio C Hamano wrote:
>> Derrick Stolee <stolee@gmail.com> writes:
>>
>>>> I'm slightly wary of changing the output of plumbing commands
>>>> like this. If a script wants progress output it can already get
>>>> it by passing --verbose. With this change a script that does not
>>>> want that output now has to pass --no-verbose.
>>>
>>> If a script is calling this, then won't stderr not be a terminal window, and
>>> isatty(2) return 0?
>>
>> Unless the script tries to capture the error output and react
>> differently depending on the error message from the plumbing (which
>> is not localized), iow most of the time, standard error stream is
>> left unredirected and likely to be connected to the terminal if the
>> script is driven from a terminal command line.
>>
>>> Or, if the script is run with stderr passing through to
>>> a terminal, then the user would see progress while running the script, which
>>> seems like a side-effect but not one that will cause a broken script.
>>
>> It will show unwanted output to the end users, no?  That is the
>> complaint about having to pass --no-verbose, if I understand
>> correctly, if the script does not want to show the progress output.
> 
> I'm happy to have attempted the change and start this discussion. It
> sounds like this one patch could be ejected to no loss to the full
> series.

Thanks.  Enabling --verbose by default on a tty might have made sense
originally but I'm worried doing it now will lead to confusing output
where read-tree is a small part of a larger operation and the script was
written on the assumption that read-tree will be quiet.

Best Wishes

Phillip

> Thanks,
> -Stolee
>

Patch
diff mbox series

diff --git a/builtin/read-tree.c b/builtin/read-tree.c
index ca5e655d2f..69963d83dc 100644
--- a/builtin/read-tree.c
+++ b/builtin/read-tree.c
@@ -162,6 +162,7 @@  int cmd_read_tree(int argc, const char **argv, const char *cmd_prefix)
 	opts.head_idx = -1;
 	opts.src_index = &the_index;
 	opts.dst_index = &the_index;
+	opts.verbose_update = isatty(2);
 
 	git_config(git_read_tree_config, NULL);