On Wed, Nov 20, 2019 at 7:46 PM Denton Liu <liu.denton@xxxxxxxxx> 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. [1]: https://lore.kernel.org/git/CAPig+cTj5qOCFRoD4cZOg7BjVvetQWTgdRHzSvAfgtX2YgUXPg@xxxxxxxxxxxxxx/
