| Message ID | 54315fecfe373d8020f2172b9b43e02c0dae137d.1577454401.git.liu.denton@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | t: replace incorrect test_must_fail usage (part 2) | expand |
Denton Liu <liu.denton@gmail.com> writes: > In one case, we were using a redirection operator to feed input into > sed. However, since sed is capable of opening its own files, make sed > open its own files instead of redirecting input into it. Could you please write in the commit message what advantages does this change bring? > Signed-off-by: Denton Liu <liu.denton@gmail.com> > --- > t/t4124-apply-ws-rule.sh | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/t/t4124-apply-ws-rule.sh b/t/t4124-apply-ws-rule.sh > index 21a4adc73a..2b19ef9811 100755 > --- a/t/t4124-apply-ws-rule.sh > +++ b/t/t4124-apply-ws-rule.sh > @@ -42,7 +42,7 @@ apply_patch () { > shift > fi && > >target && > - sed -e "s|\([ab]\)/file|\1/target|" <patch | > + sed -e "s|\([ab]\)/file|\1/target|" patch | > $should_fail git apply "$@" > }
Jakub Narebski <jnareb@gmail.com> writes: > Denton Liu <liu.denton@gmail.com> writes: > >> In one case, we were using a redirection operator to feed input into >> sed. However, since sed is capable of opening its own files, make sed >> open its own files instead of redirecting input into it. > > Could you please write in the commit message what advantages does this > change bring? A fair question. My version of short answer is "nothing---it is not wrong to write it either way, and it is not worth the patch churn to rewrite it from one form to the other, once the script is written". If we were to extend these tests in such a way that the command needs to read from more than one input file, though, dropping the redirection like the patch does is a good first step toward that, i.e. extending sed -e "expression" patch to sed -e "expression" patch-1 patch-2 ... would look more natural than starting from sed -e "expression" <patch to end at the same place, so as a preliminary clean-up, a change like this one may have value, but because there is no such further updates planned, so...
Hi Jakub and Junio, On Mon, Dec 30, 2019 at 03:27:45PM -0800, Junio C Hamano wrote: > Jakub Narebski <jnareb@gmail.com> writes: > > > Denton Liu <liu.denton@gmail.com> writes: > > > >> In one case, we were using a redirection operator to feed input into > >> sed. However, since sed is capable of opening its own files, make sed > >> open its own files instead of redirecting input into it. > > > > Could you please write in the commit message what advantages does this > > change bring? > > A fair question. > > My version of short answer is "nothing---it is not wrong to write it > either way, and it is not worth the patch churn to rewrite it from > one form to the other, once the script is written". In one of my earliest contributions to Git, Gábor suggested that since since 'head' and 'sed' can open its own input file, there's no need to redirect its standard input[1]. I assumed that letting these programs open their own input was one of those minor "style cleanup" things that was done such as removing spaces after redirection operators, indenting compound command blocks, etc. Whenever I've been cleaning tests up, if I noticed a `sed <` in the area, I'd apply the transformation as a style cleanup. I guess the only tangible benefit I can think of is that programs which have a file open can optimise on the fact that they can perform random access on the file whereas with stdin, only linear access is allowed. I don't know enough about the internals of various seds to be able to comment on whether any of them actually take advantage of this, however. Anyway, if no one else feels strongly about this, I can drop this patch and I'll stop doing this cleanup in the future. Thanks, Denton [1]: https://lore.kernel.org/git/20190317130539.GA23160@szeder.dev/
On Wed, Jan 1, 2020 at 3:25 AM Denton Liu <liu.denton@gmail.com> wrote: > I assumed that letting these programs open their own input was one of > those minor "style cleanup" things that was done such as removing > spaces after redirection operators, indenting compound command blocks, > etc. Whenever I've been cleaning tests up, if I noticed a `sed <` in the > area, I'd apply the transformation as a style cleanup. > > Anyway, if no one else feels strongly about this, I can drop this patch > and I'll stop doing this cleanup in the future. As a reviewer, I'd just as soon not see that particular change which increases reviewer burden yet provides no practical benefit. This is especially true for these lengthy patch series which can easily lead to reviewer fatigue. (In fact, after reviewing this series -- and its predecessors -- I was strongly considering asking you to limit these cleanup series to 10 or fewer patches rather than 16, precisely due to feeling reviewer fatigue.)
Hi Eric, On Wed, Jan 01, 2020 at 03:33:59AM -0500, Eric Sunshine wrote: > (In fact, after reviewing this series -- and its > predecessors -- I was strongly considering asking you to limit these > cleanup series to 10 or fewer patches rather than 16, precisely due to > feeling reviewer fatigue.) Will do. You've been unbelievably helpful as the primary reviewer for these cleanup series and I'd like to make sure that I don't cause you to burn out. Thanks for your patience, Denton
diff --git a/t/t4124-apply-ws-rule.sh b/t/t4124-apply-ws-rule.sh index 21a4adc73a..2b19ef9811 100755 --- a/t/t4124-apply-ws-rule.sh +++ b/t/t4124-apply-ws-rule.sh @@ -42,7 +42,7 @@ apply_patch () { shift fi && >target && - sed -e "s|\([ab]\)/file|\1/target|" <patch | + sed -e "s|\([ab]\)/file|\1/target|" patch | $should_fail git apply "$@" }
In one case, we were using a redirection operator to feed input into sed. However, since sed is capable of opening its own files, make sed open its own files instead of redirecting input into it. Signed-off-by: Denton Liu <liu.denton@gmail.com> --- t/t4124-apply-ws-rule.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)