git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Junio C Hamano <gitster@pobox.com>
To: Kaartic Sivaraam <kaartic.sivaraam@gmail.com>
Cc: Shourya Shukla <shouryashukla.oo@gmail.com>,
	christian.couder@gmail.com, git@vger.kernel.org,
	Johannes.Schindelin@gmx.de, liu.denton@gmail.com,
	Christian Couder <chriscool@tuxfamily.org>,
	Taylor Blau <me@ttaylorr.com>
Subject: Re: [PATCH v2 1/4] t7401: modernize style
Date: Thu, 13 Aug 2020 09:46:14 -0700	[thread overview]
Message-ID: <xmqqd03u4hpl.fsf@gitster.c.googlers.com> (raw)
In-Reply-To: <c70f7bb5d1289f5318da7b99e41cf5828a451174.camel@gmail.com> (Kaartic Sivaraam's message of "Thu, 13 Aug 2020 13:36:49 +0530")

Kaartic Sivaraam <kaartic.sivaraam@gmail.com> writes:

>> @@ -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 >out &&
>> +	cut -c1-7 out
>
> In any case, I believe we can avoid the 'cut' altogether in both places
> by doing something like this instead:
>
>    git rev-parse --short=7 HEAD

Ah, I missed the fact that this was a helper function and most of
the error status is discarded anyway.  For example, we still run the
rev-parse even after the for loop fails.

If the focus of this test script were to ensure that rev-parse works
correctly, being careful to catch its exit status might have had a
good value, but for that, all the other operations that happen in
this helper function (including the "what happens when the loop body
fails for $name that is not at the end of the argument list?") must
also be checked for their exit status in the first place.

Since that is not done, and since testing rev-parse should not have
to be part of the job for submodule test, the net effect of the
above change has quite dubious value---it clobbered a file 'out'
that may be used by the caller.

Doing "cd" without introducing a subshell is a bit harder to fix, as
test_tick relies on the global counter in the topmost process.  It
can be done, but I do not think it is worth doing here.  Most of the
users of this helper function call it in var=$(add_file ...)
subshell anyway (so test_tick is incrementing the timestamp
independently for each caller and discarding the resulting
timestamp).  As a NEEDSWORK comment added in the series says, this
script may need a bit more work.

I agree with you that the split of "rev-parse | cut -c1-7" into two
statements and clobbering 'out' is a bad change---that part should
be reverted.  The style change on 'echo "$name" >"$name"' line is
OK, though.

Thanks.

> My quick check shows the test script is happy with this change.
>
>>  	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 >out &&
>> +	cut -c1-7 out
>>  )
>>  
>>  test_expect_success 'modified submodule(backward)' "

  reply	other threads:[~2020-08-13 16:46 UTC|newest]

Thread overview: 33+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-08-05 17:49 [GSoC][RESEND][PATCH 0/4] t7401: modernize, cleanup and warn Shourya Shukla
2020-08-05 17:49 ` [PATCH 1/4] t7401: modernize style Shourya Shukla
2020-08-05 19:37   ` Denton Liu
2020-08-05 20:49     ` Taylor Blau
2020-08-06  8:45       ` Shourya Shukla
2020-08-05 17:49 ` [PATCH 2/4] t7401: change test_i18ncmp syntax for clarity Shourya Shukla
2020-08-05 20:58   ` Taylor Blau
2020-08-05 21:23   ` Junio C Hamano
2020-08-05 17:49 ` [PATCH 3/4] t7401: ensure uniformity in the '--for-status' test Shourya Shukla
2020-08-05 21:01   ` Taylor Blau
2020-08-05 22:25     ` Junio C Hamano
2020-08-05 22:26       ` Taylor Blau
2020-08-05 21:30   ` Junio C Hamano
2020-08-06  8:50     ` Shourya Shukla
2020-08-06 17:18       ` Junio C Hamano
2020-08-05 17:49 ` [PATCH 4/4] t7401: add a WARNING and a NEEDSWORK Shourya Shukla
2020-08-05 21:04   ` Taylor Blau
2020-08-05 21:36   ` Junio C Hamano
2020-08-06 11:27     ` Shourya Shukla
2020-08-06 21:11       ` Junio C Hamano
2020-08-07 14:55         ` Christian Couder
2020-08-12 19:27 ` [GSoC][PATCH v2 0/4] t7401: modernize, cleanup and more Shourya Shukla
2020-08-12 19:27   ` [PATCH v2 1/4] t7401: modernize style Shourya Shukla
2020-08-13  8:06     ` Kaartic Sivaraam
2020-08-13 16:46       ` Junio C Hamano [this message]
2020-08-14 14:41         ` Shourya Shukla
2020-08-14 17:06           ` Junio C Hamano
2020-08-12 19:27   ` [PATCH v2 2/4] t7401: change test_i18ncmp syntax for clarity Shourya Shukla
2020-08-12 20:57     ` Junio C Hamano
2020-08-12 21:02       ` Junio C Hamano
2020-08-14 14:57         ` Shourya Shukla
2020-08-12 19:27   ` [PATCH v2 3/4] t7401: change indentation for enhanced readability Shourya Shukla
2020-08-12 19:27   ` [PATCH v2 4/4] t7401: add a NEEDSWORK Shourya Shukla

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=xmqqd03u4hpl.fsf@gitster.c.googlers.com \
    --to=gitster@pobox.com \
    --cc=Johannes.Schindelin@gmx.de \
    --cc=chriscool@tuxfamily.org \
    --cc=christian.couder@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=kaartic.sivaraam@gmail.com \
    --cc=liu.denton@gmail.com \
    --cc=me@ttaylorr.com \
    --cc=shouryashukla.oo@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).