diff mbox series

[1/4] t7401: modernize style

Message ID 20200805174921.16000-2-shouryashukla.oo@gmail.com
State New, archived
Headers show
Series t7401: modernize, cleanup and warn | expand

Commit Message

Shourya Shukla Aug. 5, 2020, 5:49 p.m. UTC
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(-)

Comments

Denton Liu Aug. 5, 2020, 7:37 p.m. UTC | #1
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.
Taylor Blau Aug. 5, 2020, 8:49 p.m. UTC | #2
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
Shourya Shukla Aug. 6, 2020, 8:45 a.m. UTC | #3
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 mbox series

Patch

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)' "