| Message ID | a229e1ee0cb96c5f8c2c5d430641c386bc082a2d.1571666187.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | New sparse-checkout builtin and "cone" mode | expand |
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); > >
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
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.
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
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
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 >
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);