Re: [PATCH 12/19] tests: fix broken &&-chains in `{...}` groups
- From
Eric Sunshine <sunshine@sunshineco.com>
- Date
- Dec 11, 2021, 07:32 UTC
- Message-ID
- <CAPig+cRDKM3E5ZEV-A8Ze7pzDnM+s=xs0ioW_qXoM3yYfu7jSw@mail.gmail.com>
- In-Reply-To
- <20211210093827.a5s5mby3if5f7a4u@fs>
On Fri, Dec 10, 2021 at 4:38 AM Fabian Stelzer <fs@gigacodes.de> wrote:
Show 18 quoted lines
> On 09.12.2021 00:11, Eric Sunshine wrote:
> >Fix broken &&-chains in `{...}` groups in order to reduce the number of
> >possible lurking bugs.
> > {
> >- echo "*.t filter=rot13"
> >+ echo "*.t filter=rot13" &&
> > echo "*.i ident"
> > } >.gitattributes &&
> > {
> >- echo "expanded-keywords ident"
> >+ echo "expanded-keywords ident" &&
> > echo "expanded-keywords-crlf ident text eol=crlf"
> > } >>.gitattributes &&
> >
>
> Wouldn't some of these be better off as heredocs as well?
> There are a couple more below. I personally don't much mind either way but
> since you changed quite a few in an earlier commit why not these?It's been months since I made these changes, but I think there were at least a couple reasons for not converting these to here-docs. First, in these cases, there were only one or two missing `&&` per block. Had I bulk converted them to here-docs, it would have made for a much more noisy patch, which would have taxed reviewers more, and reviewer-fatigue is a real concern when crafting a lengthy patch series like this one. In the "here-doc conversion" patch, on the other hand, many of those cases involved a significant number of missing `&&`; often every line was missing `&&`. So, the changes in that patch was going to be very noisy anyhow, whether I added missing `&&` or converted to here-docs.
Second...
Show 6 quoted lines
> > {
> > echoid insert 444 1 2 3 4 5 a b c d e &&
> >- echoid contains 44 441 440 444 4440 4444
> >+ echoid contains 44 441 440 444 4440 4444 &&
> > echo clear
> > } | test-tool oidtree >actual &&... there are a number of cases like this which look like they could easily be converted to here-doc, but in fact `echoid` is a function call, so a here-doc wouldn't work. Also...
Show 6 quoted lines
> > {
> > echo $(git rev-parse refs/tags/A) refs/tags/A &&
> >- echo $(git rev-parse refs/tags/A^0) "refs/tags/A^{}"
> >+ echo $(git rev-parse refs/tags/A^0) "refs/tags/A^{}" &&
> > echo $(git rev-parse refs/tags/C) refs/tags/C
> > } >expect &&... this sort of thing could certainly become a here-doc because $(...) will work in a here-doc, but when there is a preponderance of this sort of `{ echo && ... }` block in the test script, it would feel inconsistent to convert a few of them to here-docs.