Subject: Re: [PATCH v5 13/17] read-tree: show progress by default

From: Derrick Stolee <[email-masked]>
Date: Mon, 21 Oct 2019 11:14:08 -0400

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 <[email-masked]>
>>
>> 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 <[email-masked]>
>> ---
>>   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