threads / patch / 57281

patcht0001: replace "test [-d|-f]" with test_path_is_* functions

Subject: [GSoC] [PATCH 0/1] t0001: replace "test [-d|-f]" with test_path_is_* functions

## tl;dr

13 messages between Jan 21, 2022 and Feb 14, 2022. Diffs are folded; open one to read it.

replies: 12people: 4as markdown or json

Shaoxuan Yuan· Jan 21, 2022, 10:21 UTC · lore

As a microproject, I found that the "test [-d|-f]" in t0001 test script can be replaced by appropriate helper functions.

Shaoxuan Yuan (1):
  t0001-init.sh use test_path_is_* functions
 t/t0001-init.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
-- 
2.25.1
Shaoxuan Yuan· Jan 21, 2022, 10:21 UTC · re: Shaoxuan Yuan · lore

[GSoC] [PATCH 1/1] t0001: replace "test [-d|-f]" with test_path_is_* functions

Signed-off-by: Shaoxuan Yuan <shaoxuan.yuan02@gmail.com>
---
 t/t0001-init.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Show changes to t/t0001-init.sh +1 −1
diff --git a/t/t0001-init.sh b/t/t0001-init.sh
index 3235ab4d53..c72a28d3a5 100755
--- a/t/t0001-init.sh
+++ b/t/t0001-init.sh
@@ -6,7 +6,7 @@ TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 check_config () {
-	if test -d "$1" && test -f "$1/config" && test -d "$1/refs"
+	if test_path_is_dir "$1" && test_path_is_file "$1/config" && test_path_is_dir "$1/refs"
 	then
 		: happy
 	else
-- 
2.25.1
Taylor Blau· Jan 21, 2022, 18:46 UTC · re: Shaoxuan Yuan · lore

Re: [GSoC] [PATCH 1/1] t0001: replace "test [-d|-f]" with test_path_is_* functions

Hi Shaoxuan,
On Fri, Jan 21, 2022 at 06:21:09PM +0800, Shaoxuan Yuan wrote:
Show 13 quoted lines
> diff --git a/t/t0001-init.sh b/t/t0001-init.sh
> index 3235ab4d53..c72a28d3a5 100755
> --- a/t/t0001-init.sh
> +++ b/t/t0001-init.sh
> @@ -6,7 +6,7 @@ TEST_PASSES_SANITIZE_LEAK=true
>  . ./test-lib.sh
>
>  check_config () {
> -	if test -d "$1" && test -f "$1/config" && test -d "$1/refs"
> +	if test_path_is_dir "$1" && test_path_is_file "$1/config" && test_path_is_dir "$1/refs"
>  	then
>  		: happy
>  	else

Looks very reasonable to me. Indeed, this line comes from 6adcca3fe8 (Fix initialization of a bare repository, 2007-08-27) which predates 2caf20c52b (test-lib: user-friendly alternatives to test [-d|-f|-e], 2010-08-10) when these helpers were originally introduced.

I thought that we could probably just shorten this to calling "test_path_is_file" twice: once for "$1/config" and a second time for "$1/refs", but that assumes "$1" is non-empty. And to ensure that you'd need another check, which amounts to the same amount of code overall.

So the fix here looks good to me, and thanks for your contribution!

Thanks, Taylor

Junio C Hamano· Jan 21, 2022, 20:49 UTC · re: Taylor Blau · lore

Re: [GSoC] [PATCH 1/1] t0001: replace "test [-d|-f]" with test_path_is_* functions

Taylor Blau <me@ttaylorr.com> writes:
Show 15 quoted lines
>> -	if test -d "$1" && test -f "$1/config" && test -d "$1/refs"
>> +	if test_path_is_dir "$1" && test_path_is_file "$1/config" && test_path_is_dir "$1/refs"
>>  	then
>>  		: happy
>>  	else
>
> Looks very reasonable to me. Indeed, this line comes from 6adcca3fe8
> (Fix initialization of a bare repository, 2007-08-27) which predates
> 2caf20c52b (test-lib: user-friendly alternatives to test [-d|-f|-e],
> 2010-08-10) when these helpers were originally introduced.
>
> I thought that we could probably just shorten this to calling
> "test_path_is_file" twice: once for "$1/config" and a second time for
> "$1/refs", but that assumes "$1" is non-empty. And to ensure that you'd
> need another check, which amounts to the same amount of code overall.
I had the same thought.

Since the first "$GIT_DIR must be a directory" matters only when the caller is crazy enough to have a bare repository at the root of the filesystem and to think that it is a good idea to say "" is the "$GIT_DIR" (in which case, "test -d ''" would fail, even though the tests for /config and /refs are checking the right thing), I do not see much downside from omitting the first one, but I think that is something we need to do _outside_ the topic of this change, which is purely "modernize, using the helpers we already have, without changing what we do".

Shaoxuan Yuan· Jan 24, 2022, 05:56 UTC · re: Junio C Hamano · lore

Re: [GSoC] [PATCH 1/1] t0001: replace "test [-d|-f]" with test_path_is_* functions

On Sat, Jan 22, 2022 at 4:49 AM Junio C Hamano <gitster@pobox.com> wrote:
Show 31 quoted lines
>
> Taylor Blau <me@ttaylorr.com> writes:
>
> >> -    if test -d "$1" && test -f "$1/config" && test -d "$1/refs"
> >> +    if test_path_is_dir "$1" && test_path_is_file "$1/config" && test_path_is_dir "$1/refs"
> >>      then
> >>              : happy
> >>      else
> >
> > Looks very reasonable to me. Indeed, this line comes from 6adcca3fe8
> > (Fix initialization of a bare repository, 2007-08-27) which predates
> > 2caf20c52b (test-lib: user-friendly alternatives to test [-d|-f|-e],
> > 2010-08-10) when these helpers were originally introduced.
> >
> > I thought that we could probably just shorten this to calling
> > "test_path_is_file" twice: once for "$1/config" and a second time for
> > "$1/refs", but that assumes "$1" is non-empty. And to ensure that you'd
> > need another check, which amounts to the same amount of code overall.
>
> I had the same thought.
>
> Since the first "$GIT_DIR must be a directory" matters only when the
> caller is crazy enough to have a bare repository at the root of the
> filesystem and to think that it is a good idea to say "" is the
> "$GIT_DIR" (in which case, "test -d ''" would fail, even though the
> tests for /config and /refs are checking the right thing), I do not
> see much downside from omitting the first one, but I think that is
> something we need to do _outside_ the topic of this change, which is
> purely "modernize, using the helpers we already have, without
> changing what we do"
>
Yes I feel the same way, one patch for one thing :)
Shaoxuan Yuan· Feb 10, 2022, 03:11 UTC · re: Junio C Hamano · lore

Re: [GSoC] [PATCH 1/1] t0001: replace "test [-d|-f]" with test_path_is_* functions

Hi Junio,
Since I didn't see this change in seen or next, do you plan to apply it?

-- Thanks, Shaoxuan

On Sat, Jan 22, 2022 at 4:49 AM Junio C Hamano <gitster@pobox.com> wrote:
Show 32 quoted lines
>
> Taylor Blau <me@ttaylorr.com> writes:
>
> >> -    if test -d "$1" && test -f "$1/config" && test -d "$1/refs"
> >> +    if test_path_is_dir "$1" && test_path_is_file "$1/config" && test_path_is_dir "$1/refs"
> >>      then
> >>              : happy
> >>      else
> >
> > Looks very reasonable to me. Indeed, this line comes from 6adcca3fe8
> > (Fix initialization of a bare repository, 2007-08-27) which predates
> > 2caf20c52b (test-lib: user-friendly alternatives to test [-d|-f|-e],
> > 2010-08-10) when these helpers were originally introduced.
> >
> > I thought that we could probably just shorten this to calling
> > "test_path_is_file" twice: once for "$1/config" and a second time for
> > "$1/refs", but that assumes "$1" is non-empty. And to ensure that you'd
> > need another check, which amounts to the same amount of code overall.
>
> I had the same thought.
>
> Since the first "$GIT_DIR must be a directory" matters only when the
> caller is crazy enough to have a bare repository at the root of the
> filesystem and to think that it is a good idea to say "" is the
> "$GIT_DIR" (in which case, "test -d ''" would fail, even though the
> tests for /config and /refs are checking the right thing), I do not
> see much downside from omitting the first one, but I think that is
> something we need to do _outside_ the topic of this change, which is
> purely "modernize, using the helpers we already have, without
> changing what we do".
>
>
Junio C Hamano· Feb 10, 2022, 07:12 UTC · re: Shaoxuan Yuan · lore

Re: [GSoC] [PATCH 1/1] t0001: replace "test [-d|-f]" with test_path_is_* functions

Shaoxuan Yuan <shaoxuan.yuan02@gmail.com> writes:
> Since I didn't see this change in seen or next, do you plan to apply it?

I actually wasn't, as my understanding of it was primarily your practice.

Shaoxuan Yuan· Feb 10, 2022, 07:21 UTC · re: Junio C Hamano · lore

Re: [GSoC] [PATCH 1/1] t0001: replace "test [-d|-f]" with test_path_is_* functions

On Thu, Feb 10, 2022 at 3:12 PM Junio C Hamano <gitster@pobox.com> wrote:
Show 7 quoted lines
>
> Shaoxuan Yuan <shaoxuan.yuan02@gmail.com> writes:
>
> > Since I didn't see this change in seen or next, do you plan to apply it?
>
> I actually wasn't, as my understanding of it was primarily your
> practice.
Understood, thanks for the reply.

-- Thanks, Shaoxuan

Junio C Hamano· Feb 10, 2022, 17:23 UTC · re: Shaoxuan Yuan · lore

Re: [GSoC] [PATCH 1/1] t0001: replace "test [-d|-f]" with test_path_is_* functions

Shaoxuan Yuan <shaoxuan.yuan02@gmail.com> writes:
Show 10 quoted lines
> On Thu, Feb 10, 2022 at 3:12 PM Junio C Hamano <gitster@pobox.com> wrote:
>>
>> Shaoxuan Yuan <shaoxuan.yuan02@gmail.com> writes:
>>
>> > Since I didn't see this change in seen or next, do you plan to apply it?
>>
>> I actually wasn't, as my understanding of it was primarily your
>> practice.
>
> Understood, thanks for the reply.

FWIW, I have the posted patch plus the following SQUASH??? fix-up parked in the 'seen' branch. As the script is quiescent right now, I do not mind merging it down, now we spent more time on it ;-)

 t/t0001-init.sh | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
Show changes to t/t0001-init.sh +2 −1
diff --git a/t/t0001-init.sh b/t/t0001-init.sh
index c72a28d3a5..d479303efa 100755
--- a/t/t0001-init.sh
+++ b/t/t0001-init.sh
@@ -6,7 +6,8 @@ TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 check_config () {
-	if test_path_is_dir "$1" && test_path_is_file "$1/config" && test_path_is_dir "$1/refs"
+	if test_path_is_dir "$1" &&
+	   test_path_is_file "$1/config" && test_path_is_dir "$1/refs"
 	then
 		: happy
 	else
-- 
2.35.1-102-g2b9c120970
Shaoxuan Yuan· Feb 11, 2022, 09:56 UTC · re: Junio C Hamano · lore

Re: [GSoC] [PATCH 1/1] t0001: replace "test [-d|-f]" with test_path_is_* functions

Hi Junio,
On Fri, Feb 11, 2022 at 1:23 AM Junio C Hamano <gitster@pobox.com> wrote:
> FWIW, I have the posted patch plus the following SQUASH??? fix-up

I'm not so sure what does "SQUASH???" mean especially the three question marks, i.e. is it just an incidental text or a commit message convention? Is it for the convenience of grepping through the "git log" outputs (cause I found the commit 50d631c71c right away by grepping through the "git log" output)?

Show 20 quoted lines
> parked in the 'seen' branch.  As the script is quiescent right now,
> I do not mind merging it down, now we spent more time on it ;-)
>
>  t/t0001-init.sh | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/t/t0001-init.sh b/t/t0001-init.sh
> index c72a28d3a5..d479303efa 100755
> --- a/t/t0001-init.sh
> +++ b/t/t0001-init.sh
> @@ -6,7 +6,8 @@ TEST_PASSES_SANITIZE_LEAK=true
>  . ./test-lib.sh
>
>  check_config () {
> -       if test_path_is_dir "$1" && test_path_is_file "$1/config" && test_path_is_dir "$1/refs"
> +       if test_path_is_dir "$1" &&
> +          test_path_is_file "$1/config" && test_path_is_dir "$1/refs"
>         then
>                 : happy
>         else
Yeah, I think wrapping it around is a good idea :-)
> --
> 2.35.1-102-g2b9c120970
>
-- 
Thanks & Regards,
Shaoxuan
Junio C Hamano· Feb 11, 2022, 16:53 UTC · re: Shaoxuan Yuan · lore

Re: [GSoC] [PATCH 1/1] t0001: replace "test [-d|-f]" with test_path_is_* functions

Shaoxuan Yuan <shaoxuan.yuan02@gmail.com> writes:
Show 9 quoted lines
> On Fri, Feb 11, 2022 at 1:23 AM Junio C Hamano <gitster@pobox.com> wrote:
>> FWIW, I have the posted patch plus the following SQUASH??? fix-up
>
> I'm not so sure what does "SQUASH???" mean especially the three
> question marks, i.e. is it just an incidental text or a commit message
> convention?
> Is it for the convenience of grepping through the
> "git log" outputs (cause I found the commit 50d631c71c right away by
> grepping through the "git log" output)?

It is primarily to remind me not to merge the branch down to 'next' without dealing with it.

> Yeah, I think wrapping it around is a good idea :-)
Then will squash it in and merge it down.
Thanks.
Christian Couder· Feb 14, 2022, 08:45 UTC · re: Shaoxuan Yuan · lore

Re: [GSoC] [PATCH 1/1] t0001: replace "test [-d|-f]" with test_path_is_* functions

On Mon, Feb 14, 2022 at 9:32 AM Shaoxuan Yuan <shaoxuan.yuan02@gmail.com> wrote:
Show 6 quoted lines
> On Fri, Feb 11, 2022 at 1:23 AM Junio C Hamano <gitster@pobox.com> wrote:
> > FWIW, I have the posted patch plus the following SQUASH??? fix-up
>
> I'm not so sure what does "SQUASH???" mean especially the three
> question marks, i.e. is it just an incidental text or a commit message
> convention?

It means that you might want to squash the fix-up commit or a similar commit into your commit (or one of your commits in case of a commit/patch series), and then resubmit a new version.

> Is it for the convenience of grepping through the
> "git log" outputs (cause I found the commit 50d631c71c right away by
> grepping through the "git log" output)?

It is for convenience that it's named "SQUASH???" as everyone (who is familiar with the mailing list) then knows what needs to be done on the proposed commit(s).

> > parked in the 'seen' branch.  As the script is quiescent right now,
> > I do not mind merging it down, now we spent more time on it ;-)

Alternatively as Junio says he is ok with merging that down, you might just accept his offer and he will squash the "SQUASH???" commit for you before merging the result into the "next" branch.

Shaoxuan Yuan· Feb 14, 2022, 08:53 UTC · re: Christian Couder · lore

Re: [GSoC] [PATCH 1/1] t0001: replace "test [-d|-f]" with test_path_is_* functions

On Mon, Feb 14, 2022 at 4:45 PM Christian Couder <christian.couder@gmail.com> wrote:

Show 20 quoted lines
>
> On Mon, Feb 14, 2022 at 9:32 AM Shaoxuan Yuan <shaoxuan.yuan02@gmail.com> wrote:
> > On Fri, Feb 11, 2022 at 1:23 AM Junio C Hamano <gitster@pobox.com> wrote:
> > > FWIW, I have the posted patch plus the following SQUASH??? fix-up
> >
> > I'm not so sure what does "SQUASH???" mean especially the three
> > question marks, i.e. is it just an incidental text or a commit message
> > convention?
>
> It means that you might want to squash the fix-up commit or a similar
> commit into your commit (or one of your commits in case of a
> commit/patch series), and then resubmit a new version.
>
> > Is it for the convenience of grepping through the
> > "git log" outputs (cause I found the commit 50d631c71c right away by
> > grepping through the "git log" output)?
>
> It is for convenience that it's named "SQUASH???" as everyone (who is
> familiar with the mailing list) then knows what needs to be done on
> the proposed commit(s).
Yeah, now I see :)
Show 6 quoted lines
> > > parked in the 'seen' branch.  As the script is quiescent right now,
> > > I do not mind merging it down, now we spent more time on it ;-)
>
> Alternatively as Junio says he is ok with merging that down, you might
> just accept his offer and he will squash the "SQUASH???" commit for
> you before merging the result into the "next" branch.

Yeah, I saw the squashed version in the latest "What's cooking in git.git". Thanks for the help and suggestions :)

-- 
Thanks & Regards,
Shaoxuan

← back to recent threads