On 3/15/2020 8:20 AM, Derrick Stolee wrote: > And I was coming back to this thread shortly after waking up since > I realized why the test fallout was bigger than I anticipated: this > change shouldn't enable "--quiet" but instead "--no-progress". The > loss of messages like "Cloning from ..." is actually a problematic > behavior change. > > I'll send a v2 using "--no-progress" instead. ...and then I find out that "git clone --no-progress" does not stop the "Updating files" message because it does not pass the progress option to that subsystem. Please eject this patch for now, since "--quiet" is too aggressive and "--no-progress" doesn't work. ...pause for change of topic... After seeing too many of these "let's plumb the user-facing progress option from the builtin into the underlying subsystem" patches, I have half a mind to completely rework how we handle "--[no-]progress". Here is a proposal for making the progress API easier to use, and hopefully clean up our code around it: 1. Add a GIT_PROGRESS environment variable that is a boolean for showing progress or not. 2. Update Git's option-parsing to check for --[no-]progress in every builtin (before the builtins do their own parsing). If present, this overrides GIT_PROGRESS. Otherwise, if GIT_PROGRESS is unset, initialize GIT_PROGRESS to the opposite of isatty(2). 3. Update start_progress_delay() and start_progress() to return NULL if GIT_PROGRESS=0. 4. Refactor the callers of start_progress[_delay]() to call it unconditionally and remove the variables used in the old conditions. While typically adding global state is undesirable, the current mechanism of passing progress flags from builtins down to lower layers (and adding --[no-]progress to subcommands) has shown to be difficult to keep consistent and makes the rest of the code messier. I'm interested in pursuing this refactor, but only if the community thinks it is a good idea. There are probably better alternatives, too. Please let me know. Thanks, -Stolee
