Re: [PATCH v5 13/17] read-tree: show progress by default
On 10/22/2019 11:48 PM, Junio C Hamano wrote: > Derrick Stolee 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
Re: [PATCH v5 13/17] read-tree: show progress by default
Derrick Stolee 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.
Re: [PATCH v5 13/17] read-tree: show progress by default
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 >> >> 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 >> --- >> 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
Re: [PATCH v5 13/17] read-tree: show progress by default
Hi Stolee On 21/10/2019 14:56, Derrick Stolee via GitGitGadget wrote: From: Derrick Stolee 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 --- 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);
