On Sat, Dec 12, 2015 at 6:17 PM, Eric Sunshine <[email protected]> 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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
