| Message ID | 20200805174921.16000-2-shouryashukla.oo@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | t7401: modernize, cleanup and warn | expand |
Hi Shourya, On Wed, Aug 05, 2020 at 11:19:18PM +0530, Shourya Shukla wrote: > The tests in 't7401-submodule-summary.sh' were written a long time ago > and have some violations with respect to our CodingGuidelines such as > incorrect spacing in usages of the redirection operator and absence > of line feed between statements in case of the '|' (pipe) operator. I'm not aware of anywhere in CodingGuidelines that says you can't have the pipe operator on a single line. I assume you're referring to the part that reads If a command sequence joined with && or || or | spans multiple lines, put each command on a separate line and put && and || and | operators at the end of each line, rather than the start. Note that says "If a command sequence [...] spans multiple lines", which doesn't apply in our case. > Update it to match the CodingGuidelines. > > Mentored-by: Christian Couder <chriscool@tuxfamily.org> > Mentored-by: Kaartic Sivaraam <kaartic.sivaraam@gmail.com> > Signed-off-by: Shourya Shukla <shouryashukla.oo@gmail.com> > --- > t/t7401-submodule-summary.sh | 8 +++++--- > 1 file changed, 5 insertions(+), 3 deletions(-) > > diff --git a/t/t7401-submodule-summary.sh b/t/t7401-submodule-summary.sh > index 9bc841d085..4439fb7c17 100755 > --- a/t/t7401-submodule-summary.sh > +++ b/t/t7401-submodule-summary.sh > @@ -16,12 +16,13 @@ add_file () { > owd=$(pwd) > cd "$sm" > for name; do > - echo "$name" > "$name" && > + echo "$name" >"$name" && > git add "$name" && > test_tick && > git commit -m "Add $name" > done >/dev/null > - git rev-parse --verify HEAD | cut -c1-7 > + git rev-parse --verify HEAD | > + cut -c1-7 For the reason above, I disagree with this change as-is. However, one useful thing that you can do here is breaking the pipe up entirely. We want to avoid is having a git command in the upstream of a pipe. This is because the return code of a pipe comes from the last command executed so if the rev-parse fails, its return code is swallowed and we have no way of knowing. You could break the pipe up by storing the output of the rev-parse in an intermediate file and then have cut read from that file.
On Wed, Aug 05, 2020 at 03:37:55PM -0400, Denton Liu wrote: > Hi Shourya, > > On Wed, Aug 05, 2020 at 11:19:18PM +0530, Shourya Shukla wrote: > > The tests in 't7401-submodule-summary.sh' were written a long time ago > > and have some violations with respect to our CodingGuidelines such as > > incorrect spacing in usages of the redirection operator and absence > > of line feed between statements in case of the '|' (pipe) operator. > > I'm not aware of anywhere in CodingGuidelines that says you can't have > the pipe operator on a single line. I assume you're referring to the > part that reads > > If a command sequence joined with && or || or | spans multiple > lines, put each command on a separate line and put && and || and > | operators at the end of each line, rather than the start. > > Note that says "If a command sequence [...] spans multiple lines", which > doesn't apply in our case. > > > Update it to match the CodingGuidelines. > > > > Mentored-by: Christian Couder <chriscool@tuxfamily.org> > > Mentored-by: Kaartic Sivaraam <kaartic.sivaraam@gmail.com> > > Signed-off-by: Shourya Shukla <shouryashukla.oo@gmail.com> > > --- > > t/t7401-submodule-summary.sh | 8 +++++--- > > 1 file changed, 5 insertions(+), 3 deletions(-) > > > > diff --git a/t/t7401-submodule-summary.sh b/t/t7401-submodule-summary.sh > > index 9bc841d085..4439fb7c17 100755 > > --- a/t/t7401-submodule-summary.sh > > +++ b/t/t7401-submodule-summary.sh > > @@ -16,12 +16,13 @@ add_file () { > > owd=$(pwd) > > cd "$sm" > > for name; do > > - echo "$name" > "$name" && > > + echo "$name" >"$name" && This change is good. > > git add "$name" && > > test_tick && > > git commit -m "Add $name" > > done >/dev/null > > - git rev-parse --verify HEAD | cut -c1-7 > > + git rev-parse --verify HEAD | > > + cut -c1-7 > > For the reason above, I disagree with this change as-is. However, one > useful thing that you can do here is breaking the pipe up entirely. We > want to avoid is having a git command in the upstream of a pipe. This is > because the return code of a pipe comes from the last command executed > so if the rev-parse fails, its return code is swallowed and we have no > way of knowing. > > You could break the pipe up by storing the output of the rev-parse in an > intermediate file and then have cut read from that file. This is a good suggestion (I was preparing to write an email to say the same thing, but I'm glad that I checked Denton's response before doing so). Something like: git rev-parse --verify HEAD >out && cut -c1-7 out would suffice and be in good style. Thanks, Taylor
On 05/08 04:49, Taylor Blau wrote: > On Wed, Aug 05, 2020 at 03:37:55PM -0400, Denton Liu wrote: > > Hi Shourya, > > > > On Wed, Aug 05, 2020 at 11:19:18PM +0530, Shourya Shukla wrote: > > > The tests in 't7401-submodule-summary.sh' were written a long time ago > > > and have some violations with respect to our CodingGuidelines such as > > > incorrect spacing in usages of the redirection operator and absence > > > of line feed between statements in case of the '|' (pipe) operator. > > > > I'm not aware of anywhere in CodingGuidelines that says you can't have > > the pipe operator on a single line. I assume you're referring to the > > part that reads > > > > If a command sequence joined with && or || or | spans multiple > > lines, put each command on a separate line and put && and || and > > | operators at the end of each line, rather than the start. > > > > Note that says "If a command sequence [...] spans multiple lines", which > > doesn't apply in our case. > > > > > Update it to match the CodingGuidelines. > > > > > > Mentored-by: Christian Couder <chriscool@tuxfamily.org> > > > Mentored-by: Kaartic Sivaraam <kaartic.sivaraam@gmail.com> > > > Signed-off-by: Shourya Shukla <shouryashukla.oo@gmail.com> > > > --- > > > t/t7401-submodule-summary.sh | 8 +++++--- > > > 1 file changed, 5 insertions(+), 3 deletions(-) > > > > > > diff --git a/t/t7401-submodule-summary.sh b/t/t7401-submodule-summary.sh > > > index 9bc841d085..4439fb7c17 100755 > > > --- a/t/t7401-submodule-summary.sh > > > +++ b/t/t7401-submodule-summary.sh > > > @@ -16,12 +16,13 @@ add_file () { > > > owd=$(pwd) > > > cd "$sm" > > > for name; do > > > - echo "$name" > "$name" && > > > + echo "$name" >"$name" && > > This change is good. > > > > git add "$name" && > > > test_tick && > > > git commit -m "Add $name" > > > done >/dev/null > > > - git rev-parse --verify HEAD | cut -c1-7 > > > + git rev-parse --verify HEAD | > > > + cut -c1-7 > > > > For the reason above, I disagree with this change as-is. However, one > > useful thing that you can do here is breaking the pipe up entirely. We > > want to avoid is having a git command in the upstream of a pipe. This is > > because the return code of a pipe comes from the last command executed > > so if the rev-parse fails, its return code is swallowed and we have no > > way of knowing. > > > > You could break the pipe up by storing the output of the rev-parse in an > > intermediate file and then have cut read from that file. > > This is a good suggestion (I was preparing to write an email to say the > same thing, but I'm glad that I checked Denton's response before doing > so). Something like: > > git rev-parse --verify HEAD >out && > cut -c1-7 out > > would suffice and be in good style. Sure. I will make the amends.
diff --git a/t/t7401-submodule-summary.sh b/t/t7401-submodule-summary.sh index 9bc841d085..4439fb7c17 100755 --- a/t/t7401-submodule-summary.sh +++ b/t/t7401-submodule-summary.sh @@ -16,12 +16,13 @@ add_file () { owd=$(pwd) cd "$sm" for name; do - echo "$name" > "$name" && + echo "$name" >"$name" && git add "$name" && test_tick && git commit -m "Add $name" done >/dev/null - git rev-parse --verify HEAD | cut -c1-7 + git rev-parse --verify HEAD | + cut -c1-7 cd "$owd" } commit_file () { @@ -125,7 +126,8 @@ commit_file sm1 && head3=$( cd sm1 && git reset --hard HEAD~2 >/dev/null && - git rev-parse --verify HEAD | cut -c1-7 + git rev-parse --verify HEAD | + cut -c1-7 ) test_expect_success 'modified submodule(backward)' "
The tests in 't7401-submodule-summary.sh' were written a long time ago and have some violations with respect to our CodingGuidelines such as incorrect spacing in usages of the redirection operator and absence of line feed between statements in case of the '|' (pipe) operator. Update it to match the CodingGuidelines. Mentored-by: Christian Couder <chriscool@tuxfamily.org> Mentored-by: Kaartic Sivaraam <kaartic.sivaraam@gmail.com> Signed-off-by: Shourya Shukla <shouryashukla.oo@gmail.com> --- t/t7401-submodule-summary.sh | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-)