Re: [PATCH va/i18n-even-more] rebase-interactive: trim leading whitespace from progress count

From: Jakub Narębski <jnareb@gmail.com>
Date: Jul 28, 2016, 18:45 UTC
Message-ID: <579A52BD.8050009@gmail.com>
In-Reply-To: <CAPig+cSBZ_Vey2BW8S4D+7ufG1U3a-N402s4zZZJ2OE=xH1+tQ@mail.gmail.com>

W dniu 2016-07-28 o 20:22, Eric Sunshine pisze:

Show 33 quoted lines

> On Thu, Jul 28, 2016 at 1:47 PM, Johannes Sixt <j6t@kdbg.org> wrote:
>> Interactive rebase uses 'wc -l' to write the current patch number
>> in a progress report. Some implementations of 'wc -l' produce spaces
>> before the number, leading to ugly output such as
>>
>>   Rebasing (     3/8)
>>
>> Remove the spaces using a trivial arithmetic evaluation.
>>
>> Before 9588c52 (i18n: rebase-interactive: mark strings for
>> translation) this was not a problem because printf was used to
>> generate the text. Since that commit, the count is interpolated
>> directly from a shell variable into the text, where the spaces
>> remain. The total number of patches does not have this problem
>> even though it is interpolated from a shell variable in the same
>> manner, because the variable is set by an arithmetic evaluation.
>>
>> Later in the script, there is a virtually identical case where
>> leading spaces are trimmed, but it uses a pattern substitution:
>>
>> todocount=$(git stripspace --strip-comments <"$todo" | wc -l)
>> todocount=${todocount##* }
>>
>> I did not choose this idiom because it adds a line of code, and
>> there is already an arithmetic evaluation in the vicinity of the
>> line that is changed here.
> 
> On the other hand, to a newcomer (not familiar with this patch),
> ${foo##* } is an obvious and intentional stripping of whitespace,
> whereas taking advantage of a side-effect of arithmetic evaluation to
> achieve the same is quite subtle and likely to be interpreted as
> pointless, thus forces the reader to consult 'blame' to understand why
> the code is the way it is.

On the gripping hand, the number of currently processed commits (instructions) in an interactive rebase is a number, and arithmetic expansion can be understood as shell equivalent of casting to integer.

Show 12 quoted lines

>> Signed-off-by: Johannes Sixt <j6t@kdbg.org>
>> ---
>> diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
>> @@ -121,7 +121,7 @@ mark_action_done () {
>>         sed -e 1q < "$todo" >> "$done"
>>         sed -e 1d < "$todo" >> "$todo".new
>>         mv -f "$todo".new "$todo"
>> -       new_count=$(git stripspace --strip-comments <"$done" | wc -l)
>> +       new_count=$(( $(git stripspace --strip-comments <"$done" | wc -l) ))
>>         echo $new_count >"$msgnum"
>>         total=$(($new_count + $(git stripspace --strip-comments <"$todo" | wc -l)))
>>         echo $total >"$end"