On Sat, Dec 12, 2015 at 6:17 PM, Eric Sunshine <sunshine@xxxxxxxxxxxxxx> wrote: > Right here below the "---" line would be a good place to explain what > changed since the previous version. As an aid for reviewers, it's also > helpful to provide a link to the previous round, like this[1]. > > [1]: http://thread.gmane.org/gmane.comp.version-control.git/281677 > Ok... learning the tricks. >> @@ -69,6 +69,13 @@ include::line-range-format.txt[] >> +--[no-]progress:: >> + Progress status is reported on the standard error stream >> + by default when it is attached to a terminal. This flag >> + enables progress reporting even if not attached to a >> + terminal. Progress information won't be displayed if using >> + `--porcelain` or `--incremental`. > > The actual implementation (below) actively forbids --progress with > --porcelain or --incremental, so the final sentence is misleading. > Perhaps say instead that "--progress is incompatible with --porcelain > and --incremental". > > More below... > Absolutely right.... didn't reflect the 'policy change' in the documentation. Will update for next patch version. >> + >> + if (pi.progress) >> + stop_progress(&pi.progress); > > As noted in the v5 review[2], stop_progress() itself handles NULL > 'struct progress' gracefully, so the 'if (pi.progress)' conditional is > unnecessary, thus the code can be simplified further to merely: > > stop_progress(&pi.progress); > You are right! > > The 'show_progress = 0' seems unnecessary. What if you did something > like this instead? > > if (show_progress > 0 && (incremental || > (output_option & OUTPUT_PORCELAIN))) > die("--progress can't be used with..."); > else if (show_progress < 0) > show_progress = isatty(2); > >> if (0 < abbrev) >> /* one more abbrev length is needed for the boundary commit */ >> abbrev++; Because, if the user didn't provide --[no-]progress option, then the value in show_progress will move forward being -1 and then in assign_blame, there will be progress output if you chose --incremental or porcelain. So, if you chose --incremental or porcelain, we better set the value to 0 to make sure there will be _no_ progress. Agree? Cheers! -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html