[GSoC][PATCH 0/1] Introduction & microproject

[GSoC][PATCH 0/1] Introduction & microproject

[Rasmus Jonsson]

Good morning, 

I intend to apply to git for this year's Google Summer of Code. I am a
Computer Science student at Gothenburg University working toward a 
master's degree. Last year I completed GSoC with LibreOffice [1].

Working on git appeals to me due to its widespread usage and the number
of people who will benefit from my contributions.

I read the introductory material, picked a microproject, looked at
previous commits and produced the patch below.

It appears that some of the listed project ideas[2] are "taken" or at
least being looked at, so I'm also looking at issues on GitGitGadget with
the label possible-gsoc-project.

Regards, 
Rasmus

Rasmus Jonsson (1):
  t1050: clean up checks for file existence

 t/t1050-large.sh | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

-- 
2.20.1

[GSoC][PATCH 1/1] t1050: clean up checks for file existence

[Rasmus Jonsson]

Replace "test -f" with test_path_is_file, which gives more verbose
and accurate output.

Signed-off-by: Rasmus Jonsson <wasmus@zom.bi>
---
 t/t1050-large.sh | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/t/t1050-large.sh b/t/t1050-large.sh
index d3b2adb28b..667fc2a745 100755
--- a/t/t1050-large.sh
+++ b/t/t1050-large.sh
@@ -53,7 +53,8 @@ test_expect_success 'add a large file or two' '
 	for p in .git/objects/pack/pack-*.pack
 	do
 		count=$(( $count + 1 ))
-		if test -f "$p" && idx=${p%.pack}.idx && test -f "$idx"
+		if test_path_is_file "$p" && idx=${p%.pack}.idx &&
+		   test_path_is_file "$idx"
 		then
 			continue
 		fi
@@ -65,7 +66,7 @@ test_expect_success 'add a large file or two' '
 	test $cnt = 2 &&
 	for l in .git/objects/??/??????????????????????????????????????
 	do
-		test -f "$l" || continue
+		test_path_is_file "$l" || continue
 		bad=t
 	done &&
 	test -z "$bad" &&
@@ -76,7 +77,8 @@ test_expect_success 'add a large file or two' '
 	for p in .git/objects/pack/pack-*.pack
 	do
 		count=$(( $count + 1 ))
-		if test -f "$p" && idx=${p%.pack}.idx && test -f "$idx"
+		if test_path_is_file "$p" && idx=${p%.pack}.idx &&
+		   test_path_is_file "$idx"
 		then
 			continue
 		fi
@@ -111,7 +113,7 @@ test_expect_success 'packsize limit' '
 		count=0 &&
 		for pi in .git/objects/pack/pack-*.idx
 		do
-			test -f "$pi" && count=$(( $count + 1 ))
+			test_path_is_file "$pi" && count=$(( $count + 1 ))
 		done &&
 		test $count = 2 &&
 
-- 
2.20.1

Re: [GSoC][PATCH 1/1] t1050: clean up checks for file existence

[Junio C Hamano]

Rasmus Jonsson <wasmus@zom.bi> writes:

> Replace "test -f" with test_path_is_file, which gives more verbose
> and accurate output.
>
> Signed-off-by: Rasmus Jonsson <wasmus@zom.bi>
> ---
>  t/t1050-large.sh | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/t/t1050-large.sh b/t/t1050-large.sh
> index d3b2adb28b..667fc2a745 100755
> --- a/t/t1050-large.sh
> +++ b/t/t1050-large.sh
> @@ -53,7 +53,8 @@ test_expect_success 'add a large file or two' '
>  	for p in .git/objects/pack/pack-*.pack
>  	do
>  		count=$(( $count + 1 ))
> -		if test -f "$p" && idx=${p%.pack}.idx && test -f "$idx"
> +		if test_path_is_file "$p" && idx=${p%.pack}.idx &&
> +		   test_path_is_file "$idx"

This is not wrong per-se, but assignment to idx logically is tied
stronger to its use (i.e. the first test_path_is_file is about the
.pack half of the packfile, the assignment to idx *and* the second
test_path_is_file is about the corresponding .idx half), so either
write it like so:

        if test_path_is_file "$p" &&
           idx=${p%.pack}.idx && test_path_is_file "$idx"

or each piece on its own line, if the result becomes overly long:

        if test_path_is_file "$p" &&
           idx=${p%.pack}.idx &&
	   test_path_is_file "$idx"

All the changes in this patch make sense, otherwise.  Thanks.

> @@ -65,7 +66,7 @@ test_expect_success 'add a large file or two' '
>  	test $cnt = 2 &&
>  	for l in .git/objects/??/??????????????????????????????????????

It is totally an unrelated tangent, but brian, are the lines of this
kind on your radar?  The object names in SHA-256 world would not be
caught with the pattern right?  The fix probably belongs to next to
where OID_REGEX is defined in test-lib.sh (this is a glob and not a
regex, though).  Perhaps the original should have been written like

	# somewhere in test-lib.sh
	HEXGLOB='[0-9a-f]'
	HEXGLOB38=$HEXGLOB$HEXGLOB$HEXGLOB$HEXGLOB$HEXGLOB$HEXGLOB ;# 6
	HEXGLOB38=$HEXGLOB38$HEXGLOB38$HEXGLOB38$HEXGLOB38$HEXGLOB38$HEXGLOB38 ;# 36
	HEXGLOB38=$HEXGLOB$HEXGLOB$HEXGLOB38

	OBJFANOUTGLOB=$HEXGLOB$HEXGLOB
	OBJFILEGLOB=$HEXGLOB38

	...

	for l in .git/objects/$OBJFANOUTGLOB/$OBJFILEGLOB

and then SHA-256 series would just update OBJFANOUTGLOB and
OBJFILEGLOB patterns, or something like that?

>  	do
> -		test -f "$l" || continue
> +		test_path_is_file "$l" || continue
>  		bad=t
>  	done &&
>  	test -z "$bad" &&
> @@ -76,7 +77,8 @@ test_expect_success 'add a large file or two' '
>  	for p in .git/objects/pack/pack-*.pack
>  	do
>  		count=$(( $count + 1 ))
> -		if test -f "$p" && idx=${p%.pack}.idx && test -f "$idx"
> +		if test_path_is_file "$p" && idx=${p%.pack}.idx &&
> +		   test_path_is_file "$idx"
>  		then
>  			continue
>  		fi
> @@ -111,7 +113,7 @@ test_expect_success 'packsize limit' '
>  		count=0 &&
>  		for pi in .git/objects/pack/pack-*.idx
>  		do
> -			test -f "$pi" && count=$(( $count + 1 ))
> +			test_path_is_file "$pi" && count=$(( $count + 1 ))
>  		done &&
>  		test $count = 2 &&

Re: [GSoC][PATCH 1/1] t1050: clean up checks for file existence

[Junio C Hamano]

Rasmus Jonsson <wasmus@zom.bi> writes:

> Replace "test -f" with test_path_is_file, which gives more verbose
> and accurate output.

Does it?  "test -f" is designed to be silent, while the helper
function is designed to be verbose to help debugging the tests,
but the accuracy of the latter simply rests on the former.

Also

> Subject: Re: [GSoC][PATCH 1/1] t1050: clean up checks for file existence

"test -f <path>" is not unclean.  It just is that it is less helpful
than using test_path_is_file.

	Subject: [PATCH] t1050: use test_path_is_file instead of "test -f"

	Use test_path_is_file instead of "test -f"; this will help
	when running the test with the "-v" option for debugging.

> Signed-off-by: Rasmus Jonsson <wasmus@zom.bi>

Re: [GSoC][PATCH 1/1] t1050: clean up checks for file existence

[brian m. carlson]

[-- Attachment #1: Type: text/plain, Size: 1707 bytes --]

On 2020-02-22 at 17:43:06, Junio C Hamano wrote:
> > @@ -65,7 +66,7 @@ test_expect_success 'add a large file or two' '
> >  	test $cnt = 2 &&
> >  	for l in .git/objects/??/??????????????????????????????????????
> 
> It is totally an unrelated tangent, but brian, are the lines of this
> kind on your radar?  The object names in SHA-256 world would not be
> caught with the pattern right?  The fix probably belongs to next to
> where OID_REGEX is defined in test-lib.sh (this is a glob and not a
> regex, though).  Perhaps the original should have been written like
> 
> 	# somewhere in test-lib.sh
> 	HEXGLOB='[0-9a-f]'
> 	HEXGLOB38=$HEXGLOB$HEXGLOB$HEXGLOB$HEXGLOB$HEXGLOB$HEXGLOB ;# 6
> 	HEXGLOB38=$HEXGLOB38$HEXGLOB38$HEXGLOB38$HEXGLOB38$HEXGLOB38$HEXGLOB38 ;# 36
> 	HEXGLOB38=$HEXGLOB$HEXGLOB$HEXGLOB38
> 
> 	OBJFANOUTGLOB=$HEXGLOB$HEXGLOB
> 	OBJFILEGLOB=$HEXGLOB38
> 
> 	...
> 
> 	for l in .git/objects/$OBJFANOUTGLOB/$OBJFILEGLOB
> 
> and then SHA-256 series would just update OBJFANOUTGLOB and
> OBJFILEGLOB patterns, or something like that?

I actually just saw that particular file the other day, but had not yet
written a patch.  I think we could write a pattern for that which would
be useful.

We do have test_oid_to_path, which takes an object ID and inserts a
slash after the first two characters, so we could do something like
this:

  for l in .git/objects/$(test_oid_to_path $ZERO_OID | sed -e 's/0/[0-9a-f]/g')

That's not very readable, though.  I'll try to find something a little
better, or hide it somewhere behind a variable in the test setup code
with a comment.
-- 
brian m. carlson: Houston, Texas, US
OpenPGP: https://keybase.io/bk2204

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 868 bytes --]

[GSoC][PATCH v2] t1050: replace test -f with test_path_is_file

[Rasmus Jonsson]

Use test_path_is_file() instead of 'test -f' for better debugging
information.

Signed-off-by: Rasmus Jonsson <wasmus@zom.bi>
---

Improved formatting of the code as suggested and corrected the commit
message.

 t/t1050-large.sh | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/t/t1050-large.sh b/t/t1050-large.sh
index d3b2adb28b..184b479a21 100755
--- a/t/t1050-large.sh
+++ b/t/t1050-large.sh
@@ -53,7 +53,8 @@ test_expect_success 'add a large file or two' '
 	for p in .git/objects/pack/pack-*.pack
 	do
 		count=$(( $count + 1 ))
-		if test -f "$p" && idx=${p%.pack}.idx && test -f "$idx"
+		if test_path_is_file "$p" &&
+		   idx=${p%.pack}.idx && test_path_is_file "$idx"
 		then
 			continue
 		fi
@@ -65,7 +66,7 @@ test_expect_success 'add a large file or two' '
 	test $cnt = 2 &&
 	for l in .git/objects/??/??????????????????????????????????????
 	do
-		test -f "$l" || continue
+		test_path_is_file "$l" || continue
 		bad=t
 	done &&
 	test -z "$bad" &&
@@ -76,7 +77,8 @@ test_expect_success 'add a large file or two' '
 	for p in .git/objects/pack/pack-*.pack
 	do
 		count=$(( $count + 1 ))
-		if test -f "$p" && idx=${p%.pack}.idx && test -f "$idx"
+		if test_path_is_file "$p" &&
+		   idx=${p%.pack}.idx && test_path_is_file "$idx"
 		then
 			continue
 		fi
@@ -111,7 +113,7 @@ test_expect_success 'packsize limit' '
 		count=0 &&
 		for pi in .git/objects/pack/pack-*.idx
 		do
-			test -f "$pi" && count=$(( $count + 1 ))
+			test_path_is_file "$pi" && count=$(( $count + 1 ))
 		done &&
 		test $count = 2 &&
 
-- 
2.20.1

Re: [GSoC][PATCH 1/1] t1050: clean up checks for file existence

[Jeff King]

On Sat, Feb 22, 2020 at 08:13:35AM +0100, Rasmus Jonsson wrote:

> diff --git a/t/t1050-large.sh b/t/t1050-large.sh
> index d3b2adb28b..667fc2a745 100755
> --- a/t/t1050-large.sh
> +++ b/t/t1050-large.sh
> @@ -53,7 +53,8 @@ test_expect_success 'add a large file or two' '
>  	for p in .git/objects/pack/pack-*.pack
>  	do
>  		count=$(( $count + 1 ))
> -		if test -f "$p" && idx=${p%.pack}.idx && test -f "$idx"
> +		if test_path_is_file "$p" && idx=${p%.pack}.idx &&
> +		   test_path_is_file "$idx"
>  		then
>  			continue
>  		fi

I was confused at first why these tests use "continue", since it seems
like these conditions would be errors that could cause a test failure
(and if they're not, we probably wouldn't want to use test_path_is_file,
since it's purpose is to complain noisily).

But the part that didn't quite make it into the diff context is
something like this:

  for p in ...
    if test -f ...
    then
      continue
    fi
    bad=t
  done &&
  test -z "$bad"

I think this could be written more clearly as:

  for p in ...
    test -f ... || return 1
  done

We explicitly run the test snippets in a shell function to allow this
kind of early return.

That's orthogonal to your patch, but it might be worth doing on top, or
as a preparatory patch.

But there's one more interesting bit. The loose-object loop from the
next hunk does this:

  for l in ...
    test -f "$l" || continue
    bad=t
  done &&
  test -z "$bad"

In other words, it's checking the opposite case: the test fails if the
file _does_ exist. And so it seems like using test_path_is_file would be
the wrong thing there (it would complain noisily in the success case,
and not at all in the failure case).

I suspect this could be written more clearly by looking at the output of
`git count-objects`, or perhaps just:

  {
    # ignore exit code; will fail when the glob matches nothing
    find objects/??/ -type f >loose-objects
    test_must_be_empty loose-objects
  }

either of which would solve the "match a loose object with any length"
problem that Junio brought up.

-Peff