[v4,0/2] cat-file: quote-format name in error when using -z

Message

Toon Claes March 3, 2023, 7:17 p.m. UTC

Hi,

This is the fourth revision of my patch to fix the error message for
git-cat-file(1) when using --batch and -z.

I was asked to provide test coverage for both code paths that produce this kind
of error message. Instead I've decided to extract the error formatting into a
separate function. For two reasons:

 * It deduplicates code.

 * The code path that was not tested is hit by --batch-all-objects. I was not
  able to set up a test that hits that code. I'm happy to write that test, if
  anyone can give me a pointer how to produce a "missing" error with that
  option.


Toon



Toon Claes (2):
  cat-file: extract printing batch error message into function
  cat-file: quote-format name in error when using -z

 builtin/cat-file.c  | 71 +++++++++++++++++++++++++++------------------
 t/t1006-cat-file.sh |  8 +++++
 2 files changed, 51 insertions(+), 28 deletions(-)

Range-diff against v3:
-:  ---------- > 1:  b2f4ce88f2 cat-file: extract printing batch error message into function
1:  a56932572c ! 2:  9e31796dc1 cat-file: quote-format name in error when using -z
    @@ builtin/cat-file.c
      #include "object-store.h"
      #include "promisor-remote.h"
      #include "mailmap.h"
    -@@ builtin/cat-file.c: static void batch_object_write(const char *obj_name,
    - 						       &data->oid, &data->info,
    - 						       OBJECT_INFO_LOOKUP_REPLACE);
    - 		if (ret < 0) {
    -+			struct strbuf quoted = STRBUF_INIT;
    -+
    -+			if (opt->nul_terminated &&
    -+			    obj_name) {
    -+				quote_c_style(obj_name, &quoted, NULL, 0);
    -+				obj_name = quoted.buf;
    -+			}
    -+
    - 			printf("%s missing\n",
    - 			       obj_name ? obj_name : oid_to_hex(&data->oid));
    -+			strbuf_release(&quoted);
    - 			fflush(stdout);
    - 			return;
    - 		}
    -@@ builtin/cat-file.c: static void batch_one_object(const char *obj_name,
    - 	result = get_oid_with_context(the_repository, obj_name,
    - 				      flags, &data->oid, &ctx);
    - 	if (result != FOUND) {
    +@@ builtin/cat-file.c: static void batch_print_error(const char *obj_name,
    + 			      enum get_oid_result result,
    + 			      struct batch_options* opt)
    + {
     +		struct strbuf quoted = STRBUF_INIT;
     +
    -+		if (opt->nul_terminated) {
    ++		if (opt->nul_terminated &&
    ++		    obj_name) {
     +			quote_c_style(obj_name, &quoted, NULL, 0);
     +			obj_name = quoted.buf;
     +		}
    @@ builtin/cat-file.c: static void batch_one_object(const char *obj_name,
      		switch (result) {
      		case MISSING_OBJECT:
      			printf("%s missing\n", obj_name);
    -@@ builtin/cat-file.c: static void batch_one_object(const char *obj_name,
    - 			       result);
    +@@ builtin/cat-file.c: static void batch_print_error(const char *obj_name,
      			break;
      		}
    -+
    -+		strbuf_release(&quoted);
      		fflush(stdout);
    - 		return;
    - 	}
    ++		strbuf_release(&quoted);
    + }
    +
    + /*

      ## t/t1006-cat-file.sh ##
     @@ t/t1006-cat-file.sh: test_expect_success FUNNYNAMES '--batch-check, -z with newline in input' '
--
2.39.2

Comments

Junio C Hamano March 3, 2023, 8:14 p.m. UTC | #1

Toon Claes <[e-mail]> writes:

> I was asked to provide test coverage for both code paths that produce this kind
> of error message. Instead I've decided to ...

Is code refactoring a substitute for making sure we have adequate
test coverage?  If not, "Instead I've decided to" is a strange thing
to say at this point in the cover letter.

Relative to the previous round, you cleaned up the code by
introducing a helper function to show error messages, which gave us
reduced duplication of the code.  This is a very good thing and is
worth mentioning.

You however didn't get around to add adequate test coverage to the
resulting code, which was one thing that the previous round was
found lacking, and you can use help to improve these patches in this
area, to help them get merged.  Some of the error cases happen only
in a corrupt repository that lacks objects that are expected to be
there (e.g. a tree points at a blob in a non-lazy clone and blob is
missing), so such a test may have to deliberately corrupt the test
repository by removing .git/object/??/* files.  

Hopefully we'll hear from those who are willing to help.  Thanks for
working on this topic.