Hi Eric, On Thu, Nov 21, 2019 at 08:12:00AM -0500, Eric Sunshine wrote: > 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. Good point. I think in general, I have a very passive writing style so I'll make a note to write my commit messages more assertively. > > > 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. Will do. Thanks, Denton > > [1]: https://lore.kernel.org/git/CAPig+cTj5qOCFRoD4cZOg7BjVvetQWTgdRHzSvAfgtX2YgUXPg@xxxxxxxxxxxxxx/
