Re: [PATCH v2 13/21] t5703: simplify one-time-sed generation logic

> On Wed, Nov 20, 2019 at 7:46 PM Denton Liu <liu.denton@gmail.com> wrote:
> > In inconsistency(), we had two `git rev-parse` invocations in the
> > upstream of a pipe within a command substitution. In case this
> > invocation ever failed, its exit code would be swallowed up and we would
> > not know about it.
> >
> > Pull the command substitutions out into variable assignments so that
> > their return codes are not lost.
> >
> > Drop the pipe into tr because command substitutions should already strip
> > leading and trailing whitespace, including newlines.
> 
> Saying that command substitution _should_ strip the whitespace leaves
> the reader in doubt as to whether there are situations in which it
> might or might not do so. More accurately, command substitution _does_
> strip the whitespace, so please drop "should" from this sentence
> altogether.

> 
> > Finally, convert the printf into an echo because it isn't necessary
> > anymore.
> 
> This is quite misleading. 'printf' was _never_ necessary; 'echo' was
> an appropriate alternative even before the other changes made by this
> patch. Worse, though, this sentence provides no context about _why_ it
> is safe to change 'printf' to 'echo', so the reader is left with even
> more questions trying to understand the validity of this change than
> if you had merely omitted this sentence. My review email[1] provided
> exact reasoning about why this change is safe. Paraphrasing that
> explanation for this patch's commit message would go a long way toward
> convincing the reader that the change makes sense.