patcht1050: clean up checks for file existence

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(-)

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(-)

Rasmus Jonsson <wasmus@zom.bi> writes:

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.

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?

On 2020-02-22 at 17:43:06, Junio C Hamano wrote:

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.

Rasmus Jonsson <wasmus@zom.bi> writes:

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

"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.

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

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

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(-)