On Fri, Aug 7, 2020 at 4:45 AM Martin Ågren <martin.agren@xxxxxxxxx> wrote: > On Thu, 6 Aug 2020 at 22:26, Eric Sunshine <sunshine@xxxxxxxxxxxxxx> wrote: > > On Thu, Aug 6, 2020 at 4:09 PM Martin Ågren <martin.agren@xxxxxxxxx> wrote: > > > - echo "$fifth branch 'fifth' of ." | > > > + echo "$fifth branch fifth of ." | > > > > This one is a bit weird. It really seems as if the intent was to quote > > the word "fifth" in the merge message, so dropping the quotes > > altogether seems wrong. However, the file 'msg' is never even > > consulted in this test (or any other test), so is this just "dead > > code" (including the leading 'fifth=' assignment which also is > > otherwise unused outside the 'echo')? > > Huh, good catch. [...] So I should be able to safely drop this > "dead code" entirely. That could be done atop this series if there is no other reason to re-roll. > > > - git tag -a -m 'annotated' anno HEAD && > > > + git tag -a -m "annotated" anno HEAD && > > > > There are a fair number of these quoted single-token arguments > > containing no special characters which don't actually need to be > > quoted at all, so an alternative would be simply to drop the quotes > > altogether, making the commands less syntactically noisy. However, > > that might be outside the intended scope of this change. > > If we say that "don't use quotes if you don't need to" is a reasonable > thing to strive for, I can drop these in a reroll. I think I'll be > injecting a patch anyway for the "msg" you pointed out in t4200, so I > can certainly tweak this patch to be a bit more aggressive in dropping > unnecessary quotes. No need. Matching the local convention makes sense, and I don't insist upon such a change at all. Mine was a non-actionable observation, and it's entirely subjective anyhow. I don't feel strongly about whether the series should be re-rolled. It's true that dropping that dead code (mentioned above) would make more sense coming before the patch which fixes up the quoting, but it wouldn't bother me if the dead-code removal was done as a follow-on patch.
