patcht6300: don't run cat-file on non-existent object

In t6300, some tests are guarded behind some prerequisites. Thus, objects created by those tests ain't available if those prerequisites is unsatistified. Attempting to run "cat-file" on those objects will run into failure.

In fact, running t6300 in an environment without gpg(1), we'll see those warnings:

	fatal: Not a valid object name refs/tags/signed-empty
	fatal: Not a valid object name refs/tags/signed-short
	fatal: Not a valid object name refs/tags/signed-long

Let's put those commands into the real tests, in order to:

* skip their execution if prerequisites aren't satistified.
* check their exit status code

Signed-off-by: Đoàn Trần Công Danh <congdanhqx@gmail.com>
---
 t/t6300-for-each-ref.sh | 27 ++++++++++++++++-----------
 1 file changed, 16 insertions(+), 11 deletions(-)

Hi Danh,

On Tue, 17 Aug 2021, Đoàn Trần Công Danh wrote:

Makes sense.

Here, we pipe the output of `cat-file` to `tail` and then `wc`. But below:

... we break the _first_ pipe apart, redirecting into `out` instead. I am not sure that this patch should change that as it does, I would think that a regular code move (with re-indentation) would be preferable.

Besides, while it is legal and works, I don't think we ever start with the redirection. Read: it should probably be `tail -n +6 <out` instead.

This necessarily has to be different from the original code (i.e. the code could not have been moved verbatim) because it uses `$3`, which at this point has a different value.

My suggestion: mention this in the commit message, other reviewers or future readers might stumble over this otherwise.

This one is new. Do we need it?

Thanks, Dscho

Running t6300 in an environment without gpg(1), we'll see those warnings:

	fatal: Not a valid object name refs/tags/signed-empty
	fatal: Not a valid object name refs/tags/signed-short
	fatal: Not a valid object name refs/tags/signed-long

Because, those objects will be created only when GPG is satistified. This series try to clean those errors.

Change from v1:
* Make 1/2 as near pure-code-move; and
* Use 2/2 as a code change to preserve status code for cat-file
* Mention reasons that 1/2 couldn't be pure-code-move.

Đoàn Trần Công Danh (2):
  t6300: don't run cat-file on non-existent object
  t6300: check for cat-file exit status code

 t/t6300-for-each-ref.sh | 29 ++++++++++++++++++-----------
 1 file changed, 18 insertions(+), 11 deletions(-)

Range-diff against v1:
1:  6d36f3a8df ! 1:  b813d6f2ad t6300: don't run cat-file on non-existent object
    @@ Commit message
         * skip their execution if prerequisites aren't satistified.
         * check their exit status code
     
    +    The expected value for objects with type: commit needs to be
    +    computed outside the test because we can't relies on "$3" there.
    +    Furthermore, to prevent the accidental usage of that computed
    +    expected value, BUG out on unknown object's type.
    +
         Signed-off-by: Đoàn Trần Công Danh <congdanhqx@gmail.com>
     
      ## t/t6300-for-each-ref.sh ##
    @@ t/t6300-for-each-ref.sh: test_atom() {
     -		esac
     -		# Leave $expect unquoted to lose possible leading whitespaces
     -		echo $expect >expected
    ++		# for commit leg, $3 is changed there
     +		expect=$(printf '%s' "$3" | wc -c)
      		test_expect_${4:-success} $PREREQ "basic atom: $1 contents:size" '
     +			type=$(git cat-file -t "$ref") &&
     +			case $type in
     +			tag)
     +				# We cannot use $3 as it expects sanitize_pgp to run
    -+				git cat-file tag $ref >out &&
    -+				expect=$(<out tail -n +6 | wc -c) ;;
    ++				expect=$(git cat-file tag $ref | tail -n +6 | wc -c) ;;
     +			tree | blob)
     +				expect="" ;;
     +			commit)
-:  ---------- > 2:  68ee769121 t6300: check for cat-file exit status code

In t6300, some tests are guarded behind some prerequisites. Thus, objects created by those tests ain't available if those prerequisites is unsatistified. Attempting to run "cat-file" on those objects will run into failure.

In fact, running t6300 in an environment without gpg(1), we'll see those warnings:

	fatal: Not a valid object name refs/tags/signed-empty
	fatal: Not a valid object name refs/tags/signed-short
	fatal: Not a valid object name refs/tags/signed-long

Let's put those commands into the real tests, in order to:

* skip their execution if prerequisites aren't satistified.
* check their exit status code

The expected value for objects with type: commit needs to be computed outside the test because we can't relies on "$3" there. Furthermore, to prevent the accidental usage of that computed expected value, BUG out on unknown object's type.

Signed-off-by: Đoàn Trần Công Danh <congdanhqx@gmail.com>
---
 t/t6300-for-each-ref.sh | 27 ++++++++++++++++-----------
 1 file changed, 16 insertions(+), 11 deletions(-)

Đoàn Trần Công Danh  <congdanhqx@gmail.com> writes:

is -> are.

relies -> rely

In test_atom(), we're piping the output of cat-file to tail(1), thus, losing its exit status.

Let's use a temporary file to preserve git exit status code.

Signed-off-by: Đoàn Trần Công Danh <congdanhqx@gmail.com>
---
 t/t6300-for-each-ref.sh | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

Đoàn Trần Công Danh  <congdanhqx@gmail.com> writes:

It is not wrong per-se, but do we need a redirect '<' here? "tail" takes filename(s) on the command line, but is there a reason to feed the contents from the standard input?

Hi Danh,

On Wed, 18 Aug 2021, Đoàn Trần Công Danh wrote:

Thank you for accommodating my concerns so quickly. The code was still so present in my mind that I did not have to go back to remind myself, but it was sufficient to look through the range-diff.

This version happily gets my `Reviewed-by:`.

Thanks, Dscho

Running t6300 in an environment without gpg(1), we'll see those warnings:

	fatal: Not a valid object name refs/tags/signed-empty
	fatal: Not a valid object name refs/tags/signed-short
	fatal: Not a valid object name refs/tags/signed-long

Because, those objects will be created only when GPG is satistified. This series try to clean those errors.

Change in v3 from v2:
* Fix grammar in 1/2 commit's message
* Let tail open input file instead of using shell redirection.

Change in v2 from v1:
* Make 1/2 as near pure-code-move; and
* Use 2/2 as a code change to preserve status code for cat-file
* Mention reasons that 1/2 couldn't be pure-code-move.

Đoàn Trần Công Danh (2):
  t6300: don't run cat-file on non-existent object
  t6300: check for cat-file exit status code

 t/t6300-for-each-ref.sh | 29 ++++++++++++++++++-----------
 1 file changed, 18 insertions(+), 11 deletions(-)

Range-diff against v2:
1:  b813d6f2ad ! 1:  b1b9771913 t6300: don't run cat-file on non-existent object
    @@ Commit message
     
         In t6300, some tests are guarded behind some prerequisites.
         Thus, objects created by those tests ain't available if those
    -    prerequisites is unsatistified.  Attempting to run "cat-file"
    +    prerequisites are unsatistified.  Attempting to run "cat-file"
         on those objects will run into failure.
     
         In fact, running t6300 in an environment without gpg(1),
    @@ Commit message
         * check their exit status code
     
         The expected value for objects with type: commit needs to be
    -    computed outside the test because we can't relies on "$3" there.
    +    computed outside the test because we can't rely on "$3" there.
         Furthermore, to prevent the accidental usage of that computed
         expected value, BUG out on unknown object's type.
     
2:  68ee769121 ! 2:  83d532528b t6300: check for cat-file exit status code
    @@ t/t6300-for-each-ref.sh: test_atom() {
      				# We cannot use $3 as it expects sanitize_pgp to run
     -				expect=$(git cat-file tag $ref | tail -n +6 | wc -c) ;;
     +				git cat-file tag $ref >out &&
    -+				expect=$(tail -n +6 <out | wc -c) &&
    ++				expect=$(tail -n +6 out | wc -c) &&
     +				rm -f out ;;
      			tree | blob)
      				expect="" ;;

In t6300, some tests are guarded behind some prerequisites. Thus, objects created by those tests ain't available if those prerequisites are unsatistified. Attempting to run "cat-file" on those objects will run into failure.

In fact, running t6300 in an environment without gpg(1), we'll see those warnings:

	fatal: Not a valid object name refs/tags/signed-empty
	fatal: Not a valid object name refs/tags/signed-short
	fatal: Not a valid object name refs/tags/signed-long

Let's put those commands into the real tests, in order to:

* skip their execution if prerequisites aren't satistified.
* check their exit status code

The expected value for objects with type: commit needs to be computed outside the test because we can't rely on "$3" there. Furthermore, to prevent the accidental usage of that computed expected value, BUG out on unknown object's type.

Signed-off-by: Đoàn Trần Công Danh <congdanhqx@gmail.com>
---
 t/t6300-for-each-ref.sh | 27 ++++++++++++++++-----------
 1 file changed, 16 insertions(+), 11 deletions(-)

In test_atom(), we're piping the output of cat-file to tail(1), thus, losing its exit status.

Let's use a temporary file to preserve git exit status code.

Signed-off-by: Đoàn Trần Công Danh <congdanhqx@gmail.com>
---

 Junio wrote:

 > It is not wrong per-se, but do we need a redirect '<' here?  "tail"
 > takes filename(s) on the command line, but is there a reason to feed
 > the contents from the standard input?

 Well, no reason. In v1, I replaced the left hand side of pipe with <out,
 then in v2, I moved it to the end of command.

 Changed to not use shell redirection.

 t/t6300-for-each-ref.sh | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)