v8, 8 partsls-tree: remove commented-out code

Diff from v6 (Origin) and v7 (RFC by Ævar):

1. [v6] Performance Regression

In v6, Ævar pointed out that there's a nearly 10% performance regression under linux [1]. This is cause by in V6, I chose to use a bitwisp operation to check whether the specified field to be printed, this will separate the original to many "printf" to combined the final output format. But some "checks" are unnecessary, like we will check whether to print the "mode" and the "type", but we do not really need to do that because only print them are meaningless.

So in commit cb881183cb if this patch, I kept some parts of bitwise logic in "show_tree" because it's more intuitive than before I think. Then, move the original logic to function "show_default", now in "show_tree" the procedure is clearer, first "show_tree_init", then check whether it's asked only to print objectname or filename, or to print a default format. After this, the performance regression problem was solved, here is the performance test result based on linux in my env:

    $hyperfine --warmup=10 "/opt/git/master/bin/git ls-tree -r HEAD"
    Benchmark 1: /opt/git/master/bin/git ls-tree -r HEAD
    Time (mean ± σ):     105.8 ms ±   2.7 ms    [User: 85.7 ms, System: 20.0 ms]
    Range (min … max):   101.5 ms … 111.3 ms    28 runs
    
    $hyperfine --warmup=10 "/opt/git/ls-tree-oid-only/bin/git ls-tree -r HEAD"
    Benchmark 1: /opt/git/ls-tree-oid-only/bin/git ls-tree -r HEAD
    Time (mean ± σ):     105.0 ms ±   3.0 ms    [User: 83.7 ms, System: 21.2 ms]
    Range (min … max):    99.3 ms … 109.5 ms    27 runs
    
    $hyperfine --warmup=10 "/opt/git/master/bin/git ls-tree -r -l HEAD"
    Benchmark 1: /opt/git/master/bin/git ls-tree -r -l HEAD
    Time (mean ± σ):     337.4 ms ±  10.9 ms    [User: 308.3 ms, System: 29.0 ms]
    Range (min … max):   323.0 ms … 355.0 ms    10 runs
    
    $hyperfine --warmup=10 "/opt/git/ls-tree-oid-only/bin/git ls-tree -r -l HEAD"
    Benchmark 1: /opt/git/ls-tree-oid-only/bin/git ls-tree -r -l HEAD
    Time (mean ± σ):     337.6 ms ±   6.2 ms    [User: 309.4 ms, System: 28.1 ms]
    Range (min … max):   330.4 ms … 349.9 ms    10 runs

  2. [v6] Bugs in "t3104"

  Ævar found that[2] I forgot to supply a <tree-ish> in tests, that's obviously a
  bug need to fix and already done in this patch.

  3. [RFC v7 by Ævar] the pre-works commits

  Ævar helped to do some pre-works commits in V7, they are 2fcff7e0d4, 6fd1dd9383,
  208654b5e2, 2637464fd8 and d77c895a4b, I think these are all reasonable, so I
  just cherry-pick to this patch and continue the work base on them.

  First, is to support `--object-only`, had been mentioned above. The second commit is
  to create a "shown_data" struct to prepare the next "--format" commit for reusing the
  struct. The last one (ls-tree.c: introduce "--format" option) is to support the
  "--format" option.
  
  Ævar posted a commit for mainly supporting "--format" in RFC v7[3] and give some design
  and performance test context. My commit based on Ævar's (I'm not sure I have to mark
  something about Ævar in commit message, because I only made some modifications but the
  idea is from Ævar) but exists some changes:

      1).  Changed the format field names, the original's and the current's are:

          objectmode -> mode
          objecttype -> type
          objectname -> object
          path -> file

          The original's are ok, just I prefer to make the name more simple to memorize and
          type, in addition, the current Documentation/git-ls-tree.txt, at "Output Format"
          section use "<mode> SP <type> SP <object> TAB <file>" to describe the format.

          I think the names with "object" prefix are from Documentation/git-for-each-ref.txt,
          use a "objectname" is not a redundant expression because there are also "authorname"
          and "refname" to be distingushed in `git-for-each-ref`, but in "git-ls-tree",
          currently, seems like no necessary, but I'm not so much sure about the naming rules
          if I was missing something.
      
      2).  OPT_CMDMODE and OPT_BIT:

          I noticed Ævar uses "OPT_BIT" in his patch but I use "OPT_CMDMODE" (actually
          OPT_CMDMODE also is Ævar teached me) and they seems like both supporting to
          make a mutual exclusive betweem options. I didn't change them to "OPT_BIT"
          because they looked like working well, plz told me if I misunderstood. 

2. Performance comparation between "master" and v8

      1). Default format( "git ls-tree -r" vs "hitten builtin formats" vs "miss builtin formats")

        $hyperfine --warmup=10 "/opt/git/master/bin/git ls-tree -r HEAD"
        Benchmark 1: /opt/git/master/bin/git ls-tree -r HEAD
        Time (mean ± σ):     105.2 ms ±   3.3 ms    [User: 84.3 ms, System: 20.8 ms]
        Range (min … max):    99.2 ms … 113.2 ms    28 runs
    
        $hyperfine --warmup=10 "/opt/git/ls-tree-oid-only/bin/git ls-tree -r --format='%(mode) %(type) %(object)%x09%(file)'  HEAD"
        Benchmark 1: /opt/git/ls-tree-oid-only/bin/git ls-tree -r --format='%(mode) %(type) %(object)%x09%(file)'  HEAD
        Time (mean ± σ):     106.4 ms ±   2.7 ms    [User: 86.1 ms, System: 20.2 ms]
        Range (min … max):   100.2 ms … 110.5 ms    29 runs

        $hyperfine --warmup=10 "/opt/git/ls-tree-oid-only/bin/git ls-tree -r --format='> %(mode) %(type) %(object)%x09%(file)'  HEAD"
        Benchmark 1: /opt/git/ls-tree-oid-only/bin/git ls-tree -r --format='> %(mode) %(type) %(object)%x09%(file)'  HEAD
        Time (mean ± σ):     145.3 ms ±   3.9 ms    [User: 119.0 ms, System: 26.2 ms]
        Range (min … max):   139.7 ms … 150.8 ms    20 runs

      2). Default format that including object size (( "git ls-tree -r -l" vs "hitten builtin formats" vs "miss builtin formats"))

        $hyperfine --warmup=10 "/opt/git/master/bin/git ls-tree -r -l HEAD"
        Benchmark 1: /opt/git/master/bin/git ls-tree -r -l HEAD
        Time (mean ± σ):     335.1 ms ±   6.5 ms    [User: 304.6 ms, System: 30.4 ms]
        Range (min … max):   327.5 ms … 348.4 ms    10 runs
    
        $hyperfine --warmup=10 "/opt/git/ls-tree-oid-only/bin/git ls-tree -r --format='%(mode) %(type) %(object) %(size:padded)%x09%(file)'  HEAD"
        Benchmark 1: /opt/git/ls-tree-oid-only/bin/git ls-tree -r --format='%(mode) %(type) %(object) %(size:padded)%x09%(file)'  HEAD
        Time (mean ± σ):     337.2 ms ±   8.2 ms    [User: 309.2 ms, System: 27.9 ms]
        Range (min … max):   328.8 ms … 349.4 ms    10 runs

        $hyperfine --warmup=10 "/opt/git/ls-tree-oid-only/bin/git ls-tree -r --format='> %(mode) %(type) %(object) %(size:padded)%x09%(file)'  HEAD"
        Benchmark 1: /opt/git/ls-tree-oid-only/bin/git ls-tree -r --format='> %(mode) %(type) %(object) %(size:padded)%x09%(file)'  HEAD
        Time (mean ± σ):     396.9 ms ±   8.9 ms    [User: 364.2 ms, System: 32.7 ms]
        Range (min … max):   379.6 ms … 408.6 ms    10 runs

Thanks.

[1] https://public-inbox.org/git/RFC-cover-0.7-00000000000-20211217T131635Z-avarab@gmail.com/ [2] https://public-inbox.org/git/211217.86o85f8jey.gmgdl@evledraar.gmail.com/ [3] https://public-inbox.org/git/RFC-patch-6.7-eac299f06ff-20211217T131635Z-avarab@gmail.com/

Teng Long (3):
  ls-tree.c: support --object-only option for "git-ls-tree"
  ls-tree.c: introduce struct "shown_data"
  ls-tree.c: introduce "--format" option

Ævar Arnfjörð Bjarmason (5):
  ls-tree: remove commented-out code
  ls-tree: add missing braces to "else" arms
  ls-tree: use "enum object_type", not {blob,tree,commit}_type
  ls-tree: use "size_t", not "int" for "struct strbuf"'s "len"
  ls-tree: split up the "init" part of show_tree()

 Documentation/git-ls-tree.txt |  55 +++++-
 builtin/ls-tree.c             | 315 +++++++++++++++++++++++++++-------
 t/t3104-ls-tree-oid.sh        |  51 ++++++
 t/t3105-ls-tree-format.sh     |  55 ++++++
 4 files changed, 415 insertions(+), 61 deletions(-)
 create mode 100755 t/t3104-ls-tree-oid.sh
 create mode 100755 t/t3105-ls-tree-format.sh

Range-diff against v7:
-:  ---------- > 1:  2fcff7e0d4 ls-tree: remove commented-out code
-:  ---------- > 2:  6fd1dd9383 ls-tree: add missing braces to "else" arms
-:  ---------- > 3:  208654b5e2 ls-tree: use "enum object_type", not {blob,tree,commit}_type
-:  ---------- > 4:  2637464fd8 ls-tree: use "size_t", not "int" for "struct strbuf"'s "len"
-:  ---------- > 5:  d77c895a4b ls-tree: split up the "init" part of show_tree()
1:  2e449d1c79 ! 6:  cb881183cb ls-tree.c: support `--object-only` option for "git-ls-tree"
    @@ Metadata
     Author: Teng Long <dyroneteng@gmail.com>
     
      ## Commit message ##
    -    ls-tree.c: support `--object-only` option for "git-ls-tree"
    +    ls-tree.c: support --object-only option for "git-ls-tree"
     
         We usually pipe the output from `git ls-trees` to tools like
         `sed` or `cut` when we only want to extract some fields.
    @@ Commit message
     
         Teach the "--object-only" option to the command to only show the
         object name. This option cannot be used together with
    -    "--name-only" or "--long" (mutually exclusive).
    +    "--name-only" or "--long" , they are mutually exclusive (actually
    +    "--name-only" and "--long" can be combined together before, this
    +    commit by the way fix this bug).
    +
    +    A simple refactoring was done to the "show_tree" function, intead by
    +    using bitwise operations to recognize the format for printing to
    +    stdout. The reason for doing this is that we don't want to increase
    +    the readability difficulty with the addition of "-object-only",
    +    making this part of the logic easier to read and expand.
    +
    +    In terms of performance, there is no loss comparing to the
    +    "master" (2ae0a9cb8298185a94e5998086f380a355dd8907), here are the
    +    results of the performance tests in my environment based on linux
    +    repository:
    +
    +        $hyperfine --warmup=10 "/opt/git/master/bin/git ls-tree -r HEAD"
    +        Benchmark 1: /opt/git/master/bin/git ls-tree -r HEAD
    +        Time (mean ± σ):     105.8 ms ±   2.7 ms    [User: 85.7 ms, System: 20.0 ms]
    +        Range (min … max):   101.5 ms … 111.3 ms    28 runs
    +
    +        $hyperfine --warmup=10 "/opt/git/ls-tree-oid-only/bin/git ls-tree -r HEAD"
    +        Benchmark 1: /opt/git/ls-tree-oid-only/bin/git ls-tree -r HEAD
    +        Time (mean ± σ):     105.0 ms ±   3.0 ms    [User: 83.7 ms, System: 21.2 ms]
    +        Range (min … max):    99.3 ms … 109.5 ms    27 runs
    +
    +        $hyperfine --warmup=10 "/opt/git/master/bin/git ls-tree -r -l HEAD"
    +        Benchmark 1: /opt/git/master/bin/git ls-tree -r -l HEAD
    +        Time (mean ± σ):     337.4 ms ±  10.9 ms    [User: 308.3 ms, System: 29.0 ms]
    +        Range (min … max):   323.0 ms … 355.0 ms    10 runs
    +
    +        $hyperfine --warmup=10 "/opt/git/ls-tree-oid-only/bin/git ls-tree -r -l HEAD"
    +        Benchmark 1: /opt/git/ls-tree-oid-only/bin/git ls-tree -r -l HEAD
    +        Time (mean ± σ):     337.6 ms ±   6.2 ms    [User: 309.4 ms, System: 28.1 ms]
    +        Range (min … max):   330.4 ms … 349.9 ms    10 runs
     
         Signed-off-by: Teng Long <dyroneteng@gmail.com>
     
    @@ builtin/ls-tree.c
      	NULL
      };
      
    +-static int show_recursive(const char *base, size_t baselen, const char *pathname)
     +enum {
     +	MODE_UNSPECIFIED = 0,
     +	MODE_NAME_ONLY,
    @@ builtin/ls-tree.c
     +
     +static int cmdmode = MODE_UNSPECIFIED;
     +
    - static int show_recursive(const char *base, int baselen, const char *pathname)
    ++static int parse_shown_fields(void)
    ++{
    ++	if (cmdmode == MODE_NAME_ONLY) {
    ++		shown_bits = SHOW_FILE_NAME;
    ++		return 0;
    ++	}
    ++	if (cmdmode == MODE_OBJECT_ONLY) {
    ++		shown_bits = SHOW_OBJECT_NAME;
    ++		return 0;
    ++	}
    ++	if (!ls_options || (ls_options & LS_RECURSIVE)
    ++	    || (ls_options & LS_SHOW_TREES)
    ++	    || (ls_options & LS_TREE_ONLY))
    ++		shown_bits = SHOW_DEFAULT;
    ++	if (cmdmode == MODE_LONG)
    ++		shown_bits = SHOW_DEFAULT | SHOW_SIZE;
    ++	return 1;
    ++}
    ++
    ++static int show_recursive(const char *base, size_t baselen,
    ++			  const char *pathname)
      {
      	int i;
    -@@ builtin/ls-tree.c: static int show_tree(const struct object_id *oid, struct strbuf *base,
    - {
    - 	int retval = 0;
    - 	int baselen;
    -+	int interspace = 0;
    - 	const char *type = blob_type;
      
    - 	if (S_ISGITLINK(mode)) {
    -@@ builtin/ls-tree.c: static int show_tree(const struct object_id *oid, struct strbuf *base,
    - 		 *
    - 		 * Something similar to this incomplete example:
    - 		 *
    --		if (show_subprojects(base, baselen, pathname))
    --			retval = READ_TREE_RECURSIVE;
    -+		 * if (show_subprojects(base, baselen, pathname))
    -+		 *	retval = READ_TREE_RECURSIVE;
    - 		 *
    - 		 */
    - 		type = commit_type;
    +@@ builtin/ls-tree.c: static int show_recursive(const char *base, size_t baselen, const char *pathname
    + 	return 0;
    + }
    + 
    ++static int show_default(const struct object_id *oid, enum object_type type,
    ++			const char *pathname, unsigned mode,
    ++			struct strbuf *base)
    ++{
    ++	size_t baselen = base->len;
    ++
    ++	if (shown_bits & SHOW_SIZE) {
    ++		char size_text[24];
    ++		if (type == OBJ_BLOB) {
    ++			unsigned long size;
    ++			if (oid_object_info(the_repository, oid, &size) == OBJ_BAD)
    ++				xsnprintf(size_text, sizeof(size_text), "BAD");
    ++			else
    ++				xsnprintf(size_text, sizeof(size_text),
    ++					  "%" PRIuMAX, (uintmax_t)size);
    ++		} else {
    ++			xsnprintf(size_text, sizeof(size_text), "-");
    ++		}
    ++		printf("%06o %s %s %7s\t", mode, type_name(type),
    ++		find_unique_abbrev(oid, abbrev), size_text);
    ++	} else {
    ++		printf("%06o %s %s\t", mode, type_name(type),
    ++		find_unique_abbrev(oid, abbrev));
    ++	}
    ++	baselen = base->len;
    ++	strbuf_addstr(base, pathname);
    ++	write_name_quoted_relative(base->buf,
    ++				   chomp_prefix ? ls_tree_prefix : NULL, stdout,
    ++				   line_termination);
    ++	strbuf_setlen(base, baselen);
    ++	return 1;
    ++}
    ++
    + static int show_tree_init(enum object_type *type, struct strbuf *base,
    + 			  const char *pathname, unsigned mode, int *retval)
    + {
     @@ builtin/ls-tree.c: static int show_tree(const struct object_id *oid, struct strbuf *base,
    - 	else if (ls_options & LS_TREE_ONLY)
    - 		return 0;
    + 	if (show_tree_init(&type, base, pathname, mode, &retval))
    + 		return retval;
      
     -	if (!(ls_options & LS_NAME_ONLY)) {
     -		if (ls_options & LS_SHOW_SIZE) {
     -			char size_text[24];
    --			if (!strcmp(type, blob_type)) {
    +-			if (type == OBJ_BLOB) {
     -				unsigned long size;
     -				if (oid_object_info(the_repository, oid, &size) == OBJ_BAD)
     -					xsnprintf(size_text, sizeof(size_text),
    @@ builtin/ls-tree.c: static int show_tree(const struct object_id *oid, struct strb
     -				else
     -					xsnprintf(size_text, sizeof(size_text),
     -						  "%"PRIuMAX, (uintmax_t)size);
    --			} else
    +-			} else {
     -				xsnprintf(size_text, sizeof(size_text), "-");
    --			printf("%06o %s %s %7s\t", mode, type,
    +-			}
    +-			printf("%06o %s %s %7s\t", mode, type_name(type),
     -			       find_unique_abbrev(oid, abbrev),
     -			       size_text);
    -+	if (shown_bits & SHOW_MODE) {
    -+		printf("%06o", mode);
    -+		interspace = 1;
    -+	}
    -+	if (shown_bits & SHOW_TYPE) {
    -+		printf("%s%s", interspace ? " " : "", type);
    -+		interspace = 1;
    -+	}
    -+	if (shown_bits & SHOW_OBJECT_NAME) {
    -+		printf("%s%s", interspace ? " " : "",
    -+		       find_unique_abbrev(oid, abbrev));
    -+		if (!(shown_bits ^ SHOW_OBJECT_NAME))
    -+			goto LINE_FINISH;
    -+		interspace = 1;
    -+	}
    -+	if (shown_bits & SHOW_SIZE) {
    -+		char size_text[24];
    -+		if (!strcmp(type, blob_type)) {
    -+			unsigned long size;
    -+			if (oid_object_info(the_repository, oid, &size) == OBJ_BAD)
    -+				xsnprintf(size_text, sizeof(size_text), "BAD");
    -+			else
    -+				xsnprintf(size_text, sizeof(size_text),
    -+					  "%"PRIuMAX, (uintmax_t)size);
    - 		} else
    --			printf("%06o %s %s\t", mode, type,
    +-		} else {
    +-			printf("%06o %s %s\t", mode, type_name(type),
     -			       find_unique_abbrev(oid, abbrev));
    -+			xsnprintf(size_text, sizeof(size_text), "-");
    -+		printf("%s%7s", interspace ? " " : "", size_text);
    -+		interspace = 1;
    -+	}
    -+	if (shown_bits & SHOW_FILE_NAME) {
    -+		if (interspace)
    -+			printf("\t");
    -+		baselen = base->len;
    -+		strbuf_addstr(base, pathname);
    -+		write_name_quoted_relative(base->buf,
    -+					   chomp_prefix ? ls_tree_prefix : NULL,
    -+					   stdout,
    -+					   line_termination
    -+					   ? CQ_NO_TERMINATOR_C_QUOTED
    -+					   : CQ_NO_TERMINATOR_AS_IS);
    -+		strbuf_setlen(base, baselen);
    +-		}
    ++	if (!(shown_bits ^ SHOW_OBJECT_NAME)) {
    ++		printf("%s%c", find_unique_abbrev(oid, abbrev), line_termination);
    ++		return retval;
      	}
     -	baselen = base->len;
     -	strbuf_addstr(base, pathname);
    @@ builtin/ls-tree.c: static int show_tree(const struct object_id *oid, struct strb
     -				   stdout, line_termination);
     -	strbuf_setlen(base, baselen);
     +
    -+LINE_FINISH:
    -+	putchar(line_termination);
    ++	if (!(shown_bits ^ SHOW_FILE_NAME)) {
    ++		baselen = base->len;
    ++		strbuf_addstr(base, pathname);
    ++		write_name_quoted_relative(base->buf,
    ++					   chomp_prefix ? ls_tree_prefix : NULL,
    ++					   stdout, line_termination);
    ++		strbuf_setlen(base, baselen);
    ++	}
    ++
    ++	if (!(shown_bits ^ SHOW_DEFAULT) ||
    ++	    !(shown_bits ^ (SHOW_DEFAULT | SHOW_SIZE)))
    ++		show_default(oid, type, pathname, mode, base);
    ++
      	return retval;
      }
      
    -+static int parse_shown_fields(void)
    -+{
    -+	if (cmdmode == MODE_NAME_ONLY) {
    -+		shown_bits = SHOW_FILE_NAME;
    -+		return 0;
    -+	}
    -+	if (cmdmode == MODE_OBJECT_ONLY) {
    -+		shown_bits = SHOW_OBJECT_NAME;
    -+		return 0;
    -+	}
    -+	if (!ls_options || (ls_options & LS_RECURSIVE)
    -+	    || (ls_options & LS_SHOW_TREES)
    -+	    || (ls_options & LS_TREE_ONLY))
    -+		shown_bits = SHOW_DEFAULT;
    -+	if (cmdmode == MODE_LONG)
    -+		shown_bits = SHOW_DEFAULT | SHOW_SIZE;
    -+	return 1;
    -+}
    -+
    - int cmd_ls_tree(int argc, const char **argv, const char *prefix)
    - {
    - 	struct object_id oid;
     @@ builtin/ls-tree.c: int cmd_ls_tree(int argc, const char **argv, const char *prefix)
      			LS_SHOW_TREES),
      		OPT_SET_INT('z', NULL, &line_termination,
    @@ builtin/ls-tree.c: int cmd_ls_tree(int argc, const char **argv, const char *pref
      		die("Not a valid object name %s", argv[0]);
      
     +	parse_shown_fields();
    ++
      	/*
      	 * show_recursive() rolls its own matching code and is
      	 * generally ignorant of 'struct pathspec'. The magic mask
     
    - ## quote.c ##
    -@@ quote.c: void quote_two_c_style(struct strbuf *sb, const char *prefix, const char *path,
    - 
    - void write_name_quoted(const char *name, FILE *fp, int terminator)
    - {
    --	if (terminator) {
    -+	if (0 < terminator || terminator == CQ_NO_TERMINATOR_C_QUOTED)
    - 		quote_c_style(name, NULL, fp, 0);
    --	} else {
    -+	else
    - 		fputs(name, fp);
    --	}
    --	fputc(terminator, fp);
    -+	if (0 <= terminator)
    -+		fputc(terminator, fp);
    - }
    - 
    - void write_name_quoted_relative(const char *name, const char *prefix,
    -
    - ## quote.h ##
    -@@ quote.h: int unquote_c_style(struct strbuf *, const char *quoted, const char **endp);
    - #define CQUOTE_NODQ 01
    - size_t quote_c_style(const char *name, struct strbuf *, FILE *, unsigned);
    - void quote_two_c_style(struct strbuf *, const char *, const char *, unsigned);
    -+/*
    -+ * Write a name, typically a filename, followed by a terminator that
    -+ * separates it from what comes next.
    -+ * When terminator is NUL, the name is given as-is.  Otherwise, the
    -+ * name is c-quoted, suitable for text output.  HT and LF are typical
    -+ * values used for the terminator, but other positive values are possible.
    -+ *
    -+ * In addition to non-negative values two special values in terminator
    -+ * are possible.
    -+ *
    -+ * -1: show the name c-quoted, without adding any terminator.
    -+ * -2: show the name as-is, without adding any terminator.
    -+ */
    -+#define CQ_NO_TERMINATOR_C_QUOTED	(-1)
    -+#define CQ_NO_TERMINATOR_AS_IS		(-2)
    - 
    - void write_name_quoted(const char *name, FILE *, int terminator);
    -+/*
    -+ * Similar to the above, but the name is first made relative to the prefix
    -+ * before being shown.
    -+ */
    - void write_name_quoted_relative(const char *name, const char *prefix,
    - 				FILE *fp, int terminator);
    - 
    -
    - ## t/t3103-ls-tree-misc.sh ##
    -@@ t/t3103-ls-tree-misc.sh: test_expect_success 'ls-tree fails with non-zero exit code on broken tree' '
    - 	test_must_fail git ls-tree -r HEAD
    - '
    - 
    -+test_expect_success 'usage: incompatible options: --name-status with --long' '
    -+	test_expect_code 129 git ls-tree --long --name-status
    -+'
    -+
    -+test_expect_success 'usage: incompatible options: --name-only with --long' '
    -+	test_expect_code 129 git ls-tree --long --name-only
    -+'
    -+
    - test_done
    -
      ## t/t3104-ls-tree-oid.sh (new) ##
     @@
     +#!/bin/sh
    @@ t/t3104-ls-tree-oid.sh (new)
     +'
     +
     +test_expect_success 'usage: incompatible options: --name-only with --object-only' '
    -+	test_expect_code 129 git ls-tree --object-only --name-only
    ++	test_expect_code 129 git ls-tree --object-only --name-only $tree
     +'
     +
     +test_expect_success 'usage: incompatible options: --name-status with --object-only' '
    -+	test_expect_code 129 git ls-tree --object-only --name-status
    ++	test_expect_code 129 git ls-tree --object-only --name-status $tree
     +'
     +
     +test_expect_success 'usage: incompatible options: --long with --object-only' '
    -+	test_expect_code 129 git ls-tree --object-only --long
    ++	test_expect_code 129 git ls-tree --object-only --long $tree
     +'
     +
     +test_done
-:  ---------- > 7:  296ebacafe ls-tree.c: introduce struct "shown_data"
-:  ---------- > 8:  e0add802fb ls-tree.c: introduce "--format" option

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>

Remove code added in f35a6d3bce7 (Teach core object handling functions about gitlinks, 2007-04-09), later patched in 7d0b18a4da1 (Add output flushing before fork(), 2008-08-04), and then finally ending up in its current form in d3bee161fef (tree.c: allow read_tree_recursive() to traverse gitlink entries, 2009-01-25). All while being commented-out!

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/ls-tree.c | 9 ---------
 1 file changed, 9 deletions(-)

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>

Add missing {} to the "else" arms in show_tree() per the CodingGuidelines.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/ls-tree.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>

Change the ls-tree.c code to use type_name() on the enum instead of using the string constants. This doesn't matter either way for performance, but makes this a bit easier to read as we'll no longer need a strcmp() here.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/ls-tree.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>

The "struct strbuf"'s "len" member is a "size_t", not an "int", so let's change our corresponding types accordingly. This also changes the "len" and "speclen" variables, which are likewise used to store the return value of strlen(), which returns "size_t", not "int".

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/ls-tree.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>

Split up the "init" part of the show_tree() function where we decide what the "type" is, and whether we'll return early. This makes things a bit less readable for now, but we'll soon re-use this in a sibling function, and avoiding the duplication will be worth it.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/ls-tree.c | 30 +++++++++++++++++++-----------
 1 file changed, 19 insertions(+), 11 deletions(-)

Teng Long <dyroneteng@gmail.com> writes:

Don't we need some comment that explains what the function does, what its return value means, etc.?

It seems that even from its returned value, the caller cannot tell if *retval was set by the function or not. Perhaps it makes a much cleaner API to assign 0 to *retval at the beginning of this function, just like the original did so anyway? ...

... It would mean we can lose this initialization.

Junio C Hamano writes:

Oh, sorry for that, I did not notice the "retval" before because the naming is unimpressive and the tests were passed, though...

I just looked at it, actually, it's important, not as what it is named, it affects the result. The "retval" actually determine whether to CONTINUE reading the current "tree" or BREAK into the next one [1] .

So, I think this commit should be modified despite the tests are passed, firstly, I want to rename "retval" to another name that makes sense, then just make the relevant "if" and "return" logic more clearly with the newname, finally, it'll be consistent with the definitions in "read_tree_at()" at "tree.c" [1].

[1] https://github.com/dyrone/git/blob/master/tree.c#L40

Thanks.

Junio C Hamano <gitster@pobox.com> 于2022年1月4日周二 10:06写道：

We usually pipe the output from `git ls-trees` to tools like `sed` or `cut` when we only want to extract some fields.

When we want only the pathname component, we can pass `--name-only` option to omit such a pipeline, but there are no options for extracting other fields.

Teach the "--object-only" option to the command to only show the object name. This option cannot be used together with "--name-only" or "--long" , they are mutually exclusive (actually "--name-only" and "--long" can be combined together before, this commit by the way fix this bug).

A simple refactoring was done to the "show_tree" function, intead by using bitwise operations to recognize the format for printing to stdout. The reason for doing this is that we don't want to increase the readability difficulty with the addition of "-object-only", making this part of the logic easier to read and expand.

In terms of performance, there is no loss comparing to the "master" (2ae0a9cb8298185a94e5998086f380a355dd8907), here are the results of the performance tests in my environment based on linux repository:

    $hyperfine --warmup=10 "/opt/git/master/bin/git ls-tree -r HEAD"
    Benchmark 1: /opt/git/master/bin/git ls-tree -r HEAD
    Time (mean ± σ):     105.8 ms ±   2.7 ms    [User: 85.7 ms, System: 20.0 ms]
    Range (min … max):   101.5 ms … 111.3 ms    28 runs

    $hyperfine --warmup=10 "/opt/git/ls-tree-oid-only/bin/git ls-tree -r HEAD"
    Benchmark 1: /opt/git/ls-tree-oid-only/bin/git ls-tree -r HEAD
    Time (mean ± σ):     105.0 ms ±   3.0 ms    [User: 83.7 ms, System: 21.2 ms]
    Range (min … max):    99.3 ms … 109.5 ms    27 runs

    $hyperfine --warmup=10 "/opt/git/master/bin/git ls-tree -r -l HEAD"
    Benchmark 1: /opt/git/master/bin/git ls-tree -r -l HEAD
    Time (mean ± σ):     337.4 ms ±  10.9 ms    [User: 308.3 ms, System: 29.0 ms]
    Range (min … max):   323.0 ms … 355.0 ms    10 runs

    $hyperfine --warmup=10 "/opt/git/ls-tree-oid-only/bin/git ls-tree -r -l HEAD"
    Benchmark 1: /opt/git/ls-tree-oid-only/bin/git ls-tree -r -l HEAD
    Time (mean ± σ):     337.6 ms ±   6.2 ms    [User: 309.4 ms, System: 28.1 ms]
    Range (min … max):   330.4 ms … 349.9 ms    10 runs

Signed-off-by: Teng Long <dyroneteng@gmail.com>
---
 Documentation/git-ls-tree.txt |   7 +-
 builtin/ls-tree.c             | 140 +++++++++++++++++++++++++---------
 t/t3104-ls-tree-oid.sh        |  51 +++++++++++++
 3 files changed, 159 insertions(+), 39 deletions(-)
 create mode 100755 t/t3104-ls-tree-oid.sh

Teng Long <dyroneteng@gmail.com> writes:

The resulting code looks unnecessarily complex and brittle; some SHOW_FOO mean SHOW_FOO_ONLY_AND_NOTHING_ELSE while other SHOW_BAR means SHOW_BAR_BUT_WE_MAY_SHOW_OTHER_THINGS_IN_LATER_PART, and the distinction is not clear from their names (which means it is hard to later extend and enhance the behaviour of the code).

Is the use of XOR operator significant here?

I.e. "if (shown_bits & SHOW_FILE_NAME)" would have been a much more natural way to guard "this is a block that shows the file name", than "the result MUST BE all bits off if we flip SHOW_FILE_NAME bit off". If various SHOW_FOO bits are meant to be mutually exclusive, then "if ((shown_bits & SHOW_FILE_NAME) == SHOW_FILE_NAME)" would also make sense, but as I said upfront, it is unclear to me if shown_bits are meant to be a collection of "this bit means this field is shown (and it implies nothing else)", so I dunno.

Junio C Hamano <gitster@pobox.com> writes:

I agree with you that the relevant code is not very clear, So I think I will take these steps:

1. Rename "shown_bits" -> "shown_fields"

   essentially we want to show the fields but not bits to user that
   may firstly solve the unclear nameing problem for "shown_bits"
   itself.

2. Rename related macro definitions of "shown_fields by :

	   SHOW_FILE_NAME -> FILE_NAME_FIELD
    	   SHOW_SIZE -> SIZE_FIELD
    	   SHOW_OBJECT_NAME -> OBJECT_NAME_FIELD
    	   SHOW_TYPE -> TYPE_FIELD
    	   SHOW_DEFAULT -> DEFAULT_FIELDS

    I think the confusion comes from " SHOW_FOO_ONLY_AND_NOTHING_ELSE"
    and " SHOW_BAR_BUT_WE_MAY_SHOW_OTHER_THINGS_IN_LATER_PART" is
    because some macros's is named by mixed the "flags" and "the
    operation of flags" together. 

    So with renamings, we try to unify these definitions meaning,
    they are just used for defining a "field" with a specified
    non-repetitive bits.
    
    After that, you can show many "fields" by combining any of "fields",
    such as what the builtin "DEFAULT_FIELDS" does, it shows all the
    fields but except the "size" field.

    By far, the "field(s)" only means the definition themselfs, and
    no business with "which one/ones" or  "how" to shown.

3. Decide "which" fields need to show

   The definition of "WHICH_FIELDS_TO_SHOWN" is by "shown_fields",
   it's used for parse from the options and compute it's value
   by function "parse_shown_fields()".

   Actually, the function already represent what work it do,
   but the problem is I didn't notice to rename "show_bits"
   to "show_fields" before. This may bring some confusion,
   because we are not going to show the bits but the field(s).
   So, I will do the <STEP.1>.

4. Decide "How" fields to be shown

    Now we have already know the field(s) we care about, next
    step is to show the fields.

    Not significant. Both work and It's all right for me. Your
    readability is better, and now I know how to handle this
    situation better.

    E.g, if we only want to show a "filename" field
    (with `--name-only`), we will use a way like
    "if ((shown_fields & FILE_NAME_FIELD) == FILE_NAME_FIELD)"
    to judge this situation.

    And if we want to show fields in a builtin way
    (as described in 'git-ls-tree.txt', default output format
    is compatible with what `--index-info --stdin` of
    'git update-index' expects.), we will use "DEFAULT_FIELDS"
    instead.

I am not sure if this solves the problem you are considering as I may misunderstand, I can quickly finish this new patch and we can look at it then。

Thanks.

"show_data" is a struct that packages the necessary fields for reusing. This commit is a front-loaded commit for support "--format" argument and does not affect any existing functionality.

Signed-off-by: Teng Long <dyroneteng@gmail.com>
---
 builtin/ls-tree.c | 47 +++++++++++++++++++++++++++++------------------
 1 file changed, 29 insertions(+), 18 deletions(-)

Teng Long <dyroneteng@gmail.com> writes:

Is that shown_data?

What's a front-loaded commit? Is that some joke around a washing machine that I do not quite get, or something?

Yes, sorry for that mislead.

I know this word recently (from an English training institute), feel sorry if it confuses you, I think it means pre-prepared.

I found two demo sentences from: https://dictionary.cambridge.org/us/dictionary/english/front-load

I'm not sure that I used it right or wrong, if it's the wrong way or just make others feel like in haze, I will use “pre-prepared” preferentially next time.

Thanks.

Junio C Hamano <gitster@pobox.com> 于2022年1月4日周二 07:21写道：

Add a --format option to ls-tree. It has an existing default output, and then --long and --name-only options to emit the default output along with the objectsize and, or to only emit object paths.

Rather than add --type-only, --object-only etc. we can just support a --format using a strbuf_expand() similar to "for-each-ref --format". We might still add such options in the future for convenience.

The --format implementation is slower than the existing code, but this change does not cause any performance regressions. We'll leave the existing show_tree() unchanged, and only run show_tree_fmt() in if a --format different than the hardcoded built-in ones corresponding to the existing modes is provided.

I.e. something like the "--long" output would be much slower with this, mainly due to how we need to allocate various things to do with quote.c instead of spewing the output directly to stdout.

The new option of '--format' comes from Ævar Arnfjörð Bjarmasonn's idea and suggestion, this commit makes modifications in terms of the original discussion on community [1].

Here is the statistics about performance tests:

1. Default format (hitten the builtin formats):

    "git ls-tree <tree-ish>" vs "--format='%(mode) %(type) %(object)%x09%(file)'"

    $hyperfine --warmup=10 "/opt/git/master/bin/git ls-tree -r HEAD"
    Benchmark 1: /opt/git/master/bin/git ls-tree -r HEAD
    Time (mean ± σ):     105.2 ms ±   3.3 ms    [User: 84.3 ms, System: 20.8 ms]
    Range (min … max):    99.2 ms … 113.2 ms    28 runs

    $hyperfine --warmup=10 "/opt/git/ls-tree-oid-only/bin/git ls-tree -r --format='%(mode) %(type) %(object)%x09%(file)'  HEAD"
    Benchmark 1: /opt/git/ls-tree-oid-only/bin/git ls-tree -r --format='%(mode) %(type) %(object)%x09%(file)'  HEAD
    Time (mean ± σ):     106.4 ms ±   2.7 ms    [User: 86.1 ms, System: 20.2 ms]
    Range (min … max):   100.2 ms … 110.5 ms    29 runs

2. Default format includes object size (hitten the builtin formats):

    "git ls-tree -l <tree-ish>" vs "--format='%(mode) %(type) %(object) %(size:padded)%x09%(file)'"

    $hyperfine --warmup=10 "/opt/git/master/bin/git ls-tree -r -l HEAD"
    Benchmark 1: /opt/git/master/bin/git ls-tree -r -l HEAD
    Time (mean ± σ):     335.1 ms ±   6.5 ms    [User: 304.6 ms, System: 30.4 ms]
    Range (min … max):   327.5 ms … 348.4 ms    10 runs

    $hyperfine --warmup=10 "/opt/git/ls-tree-oid-only/bin/git ls-tree -r --format='%(mode) %(type) %(object) %(size:padded)%x09%(file)'  HEAD"
    Benchmark 1: /opt/git/ls-tree-oid-only/bin/git ls-tree -r --format='%(mode) %(type) %(object) %(size:padded)%x09%(file)'  HEAD
    Time (mean ± σ):     337.2 ms ±   8.2 ms    [User: 309.2 ms, System: 27.9 ms]
    Range (min … max):   328.8 ms … 349.4 ms    10 runs

Links: [1] https://public-inbox.org/git/RFC-patch-6.7-eac299f06ff-20211217T131635Z-avarab@gmail.com/

Signed-off-by: Teng Long <dyroneteng@gmail.com>
---
 Documentation/git-ls-tree.txt |  50 ++++++++-
 builtin/ls-tree.c             | 191 ++++++++++++++++++++++++++++------
 t/t3105-ls-tree-format.sh     |  55 ++++++++++
 3 files changed, 259 insertions(+), 37 deletions(-)
 create mode 100755 t/t3105-ls-tree-format.sh

Hi Teng,

On Sat, 1 Jan 2022, Teng Long wrote:

This, along with two other similar instances, triggers the `static-analysis` job in the CI failure of `seen`. The suggested diff is:

-- snip --

Hi Teng,

On Tue, 4 Jan 2022, Johannes Schindelin wrote:

In addition to that, you need these to quiet down the `linux-leaks` job of our CI build, which is also failing with your patches:

-- snipsnap --

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

Thanks. I will fix it in next patch.

Johannes Schindelin <Johannes.Schindelin@gmx.de> 于2022年1月4日周二 23:17写道：

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

The second and third I will optimize in the next patch.

The first one. Actually I am a little puzzled from this :

"strbuf_addf(line, "%7s", "-");" here is used to align the columns with a width of seven chars, not repeat one DASH to seven.

A little weird about the fix recommendation of "strbuf_addstr(line, "-");" , because it will only add a single DASH here.

It's the identical result which compares to the "master"[1] I think with the current codes and I tested the "strbuf_addf()" simply and it seems to work fine.

[1] https://github.com/git/git/blob/master/builtin/ls-tree.c#L106

Thanks.

Johannes Schindelin <Johannes.Schindelin@gmx.de> 于2022年1月4日周二 22:38写道：

Hi Teng,

On Wed, 5 Jan 2022, Teng Long wrote:

Ah. I misremembered and thought that `"% 7s"` would do that, but you're correct. See below for more on this.

But first, I wonder why the test suite passes with the `strbuf_addstr()` call... Is this line not covered by any test case?

About the `%7s` thing: The most obvious resolution is to use `" -"` with `strbuf_addstr()`. And I would argue that this is the best resolution.

If you disagree (and want to spin up a full `sprintf()` every time, just to add those six space characters), feel free to integrate the following into your patch series:

-- snip --
From a390fcf7eec261c7f0e341bda79f2b1f326d151e Mon Sep 17 00:00:00 2001
From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Wed, 5 Jan 2022 14:02:19 +0100
Subject: [PATCH] cocci: allow padding with `strbuf_addf()`

A convenient way to pad strings is to use something like `strbuf_addf(&buf, "%20s", "Hello, world!")`.

However, the Coccinelle rule that forbids a format `"%s"` with a constant string argument cast too wide a net, and also forbade such padding.

Let's be a bit stricter in that Coccinelle rule.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 contrib/coccinelle/strbuf.cocci | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

On Wed, Jan 5, 2022 at 9:09 PM Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:

Definitely, me too.

I agree that's a quick fix in that way. Can you feed me more info about why you think it's the best resolution?

I appreciate the input of 'coccinelle' and the commit.

The current relevant rules of 'strbuf' was added in commit [1], the purpose of it seems like to forbid some inefficient use cases and chase the performance profit as much as possible.

I think "<SP*6>-" and "%7s", they both with the same result, the former benefits in performance, the later benefits in readability. So let's do a simple performance test under "linux", then think about which is better for this case:

    Benchmark 1: /opt/git/ls-tree-oid-only-addf/bin/git ls-tree -r
--format='> %(mode) %(type) %(object) %(size:padded)%x09%(file)'  HEAD
    Time (mean ± σ):     387.7 ms ±   8.8 ms    [User: 357.6 ms,
System: 30.0 ms]
    Range (min … max):   377.5 ms … 399.5 ms    10 runs

    Benchmark 1: /opt/git/ls-tree-oid-only-addstr/bin/git ls-tree -r
--format='> %(mode) %(type) %(object) %(size:padded)%x09%(file)'  HEAD
    Time (mean ± σ):     388.9 ms ±   9.0 ms    [User: 362.7 ms,
System: 26.1 ms]
    Range (min … max):   373.4 ms … 399.8 ms    10 runs

It's with a slight performance difference between the two.

So, I decided to integrate your patch as a new commit in the current patchset and is it ok for me to mention it's from your guidance in the commit message or a "helped-by" something like this?

Thanks.

[1] https://github.com/git/git/commit/28c23cd4c3902449aff72cb9a4a703220be0d6ac

Diff from v8:

1). [Delete] ls-tree: split up the "init" part of show_tree() 2). [New] ls-tree: optimize naming and handling of "return" in show_tree()

In last patch v8, I use commit "ls-tree: split up the "init" part of show_tree()" (d77c895a4b) from RFC patch v7 [1] and I did not notice the handling of return value which was called "retval" in it. As Junio mentioned in his reply [2], I took a look about the "retval" and found it needs to be handled with care.

So, instead I introduced this new commit (instead of the old commit) to take responsiblies for renaming "retval" and try to let the relevant codes maybe more easier to understand.

3). [Append] ls-tree.c: support --object-only option for "git-ls-tree"

Junio pointed out that some of these fields are ambiguous [2] and on the necessary of using xOR.

I modified the commit and it's basically according to the steps I replied in [3], except the issue of using xOR, Junio suggest me to use "if ((shown_fields & FILE_NAME_FIELD) == FILE_NAME_FIELD)" instead, but in here I think use "if (shown_fields == FILE_NAME_FIELD)" is enough.

4). [Append] ls-tree.c: introduce struct "show_tree_data"

I had a spelling mistake which was found by Junio ("show_data" should be "shown_data"), that reminded me the "show_data" is a little abtract, so I rename it to "show_tree_data" which is the data struct used in function "show_tree", I think it's a bit better than before.

5). [Append] ls-tree.c: introduce "--format" option

Doc format modifications. Fix strbuf leaks and some other non-functional modifications in "ls-tree.c"

6). [New] cocci: allow padding with `strbuf_addf()`

Fix the static-analysis issue[4] which found by Johannes Schindelin. 

Thanks.

[1] https://public-inbox.org/git/xmqqwnjgfe4t.fsf@gitster.g/ [2] https://public-inbox.org/git/xmqqwnjgfe4t.fsf@gitster.g/ [3] https://public-inbox.org/git/20220104072951.10153-1-dyroneteng@gmail.com/#t [4] https://public-inbox.org/git/nycvar.QRO.7.76.6.2201051348050.7076@tvgsbejvaqbjf.bet/

Teng Long (5):
  ls-tree: optimize naming and handling of "return" in show_tree()
  ls-tree.c: support --object-only option for "git-ls-tree"
  ls-tree.c: introduce struct "show_tree_data"
  ls-tree.c: introduce "--format" option
  cocci: allow padding with `strbuf_addf()`

Ævar Arnfjörð Bjarmason (4):
  ls-tree: remove commented-out code
  ls-tree: add missing braces to "else" arms
  ls-tree: use "enum object_type", not {blob,tree,commit}_type
  ls-tree: use "size_t", not "int" for "struct strbuf"'s "len"

 Documentation/git-ls-tree.txt   |  55 +++++-
 builtin/ls-tree.c               | 332 ++++++++++++++++++++++++++------
 contrib/coccinelle/strbuf.cocci |   2 +-
 t/t3104-ls-tree-oid.sh          |  51 +++++
 t/t3105-ls-tree-format.sh       |  55 ++++++
 5 files changed, 427 insertions(+), 68 deletions(-)
 create mode 100755 t/t3104-ls-tree-oid.sh
 create mode 100755 t/t3105-ls-tree-format.sh

Range-diff against v8:
 1:  d77c895a4b <  -:  ---------- ls-tree: split up the "init" part of show_tree()
 -:  ---------- >  1:  2fcff7e0d4 ls-tree: remove commented-out code
 -:  ---------- >  2:  6fd1dd9383 ls-tree: add missing braces to "else" arms
 -:  ---------- >  3:  208654b5e2 ls-tree: use "enum object_type", not {blob,tree,commit}_type
 -:  ---------- >  4:  2637464fd8 ls-tree: use "size_t", not "int" for "struct strbuf"'s "len"
 -:  ---------- >  5:  75503c41a7 ls-tree: optimize naming and handling of "return" in show_tree()
 2:  cb881183cb !  6:  e0274f079a ls-tree.c: support --object-only option for "git-ls-tree"
    @@ builtin/ls-tree.c
      static struct pathspec pathspec;
      static int chomp_prefix;
      static const char *ls_tree_prefix;
    -+static unsigned int shown_bits;
    -+#define SHOW_FILE_NAME 1
    -+#define SHOW_SIZE (1 << 1)
    -+#define SHOW_OBJECT_NAME (1 << 2)
    -+#define SHOW_TYPE (1 << 3)
    -+#define SHOW_MODE (1 << 4)
    -+#define SHOW_DEFAULT 29 /* 11101 size is not shown to output by default */
    ++static unsigned int shown_fields;
    ++#define FIELD_FILE_NAME 1
    ++#define FIELD_SIZE (1 << 1)
    ++#define FIELD_OBJECT_NAME (1 << 2)
    ++#define FIELD_TYPE (1 << 3)
    ++#define FIELD_MODE (1 << 4)
    ++#define FIELD_DEFAULT 29 /* 11101 size is not shown to output by default */
    ++#define FIELD_LONG_DEFAULT  (FIELD_DEFAULT | FIELD_SIZE)
      
      static const  char * const ls_tree_usage[] = {
      	N_("git ls-tree [<options>] <tree-ish> [<path>...]"),
    @@ builtin/ls-tree.c
     +static int parse_shown_fields(void)
     +{
     +	if (cmdmode == MODE_NAME_ONLY) {
    -+		shown_bits = SHOW_FILE_NAME;
    ++		shown_fields = FIELD_FILE_NAME;
     +		return 0;
     +	}
     +	if (cmdmode == MODE_OBJECT_ONLY) {
    -+		shown_bits = SHOW_OBJECT_NAME;
    ++		shown_fields = FIELD_OBJECT_NAME;
     +		return 0;
     +	}
     +	if (!ls_options || (ls_options & LS_RECURSIVE)
     +	    || (ls_options & LS_SHOW_TREES)
     +	    || (ls_options & LS_TREE_ONLY))
    -+		shown_bits = SHOW_DEFAULT;
    ++		shown_fields = FIELD_DEFAULT;
     +	if (cmdmode == MODE_LONG)
    -+		shown_bits = SHOW_DEFAULT | SHOW_SIZE;
    ++		shown_fields = FIELD_LONG_DEFAULT;
     +	return 1;
     +}
     +
    @@ builtin/ls-tree.c: static int show_recursive(const char *base, size_t baselen, c
     +{
     +	size_t baselen = base->len;
     +
    -+	if (shown_bits & SHOW_SIZE) {
    ++	if (shown_fields & FIELD_SIZE) {
     +		char size_text[24];
     +		if (type == OBJ_BLOB) {
     +			unsigned long size;
    @@ builtin/ls-tree.c: static int show_recursive(const char *base, size_t baselen, c
     +	return 1;
     +}
     +
    - static int show_tree_init(enum object_type *type, struct strbuf *base,
    - 			  const char *pathname, unsigned mode, int *retval)
    + static void init_type(unsigned mode, enum object_type *type)
      {
    + 	if (S_ISGITLINK(mode))
     @@ builtin/ls-tree.c: static int show_tree(const struct object_id *oid, struct strbuf *base,
    - 	if (show_tree_init(&type, base, pathname, mode, &retval))
    - 		return retval;
    + 	if (type == OBJ_BLOB && (ls_options & LS_TREE_ONLY))
    + 		return !READ_TREE_RECURSIVE;
      
     -	if (!(ls_options & LS_NAME_ONLY)) {
     -		if (ls_options & LS_SHOW_SIZE) {
    @@ builtin/ls-tree.c: static int show_tree(const struct object_id *oid, struct strb
     -			printf("%06o %s %s\t", mode, type_name(type),
     -			       find_unique_abbrev(oid, abbrev));
     -		}
    -+	if (!(shown_bits ^ SHOW_OBJECT_NAME)) {
    ++	if (shown_fields == FIELD_OBJECT_NAME) {
     +		printf("%s%c", find_unique_abbrev(oid, abbrev), line_termination);
    -+		return retval;
    ++		return recursive;
      	}
     -	baselen = base->len;
     -	strbuf_addstr(base, pathname);
    @@ builtin/ls-tree.c: static int show_tree(const struct object_id *oid, struct strb
     -				   stdout, line_termination);
     -	strbuf_setlen(base, baselen);
     +
    -+	if (!(shown_bits ^ SHOW_FILE_NAME)) {
    ++	if (shown_fields == FIELD_FILE_NAME) {
     +		baselen = base->len;
     +		strbuf_addstr(base, pathname);
     +		write_name_quoted_relative(base->buf,
     +					   chomp_prefix ? ls_tree_prefix : NULL,
     +					   stdout, line_termination);
     +		strbuf_setlen(base, baselen);
    ++		return recursive;
     +	}
     +
    -+	if (!(shown_bits ^ SHOW_DEFAULT) ||
    -+	    !(shown_bits ^ (SHOW_DEFAULT | SHOW_SIZE)))
    ++	if (shown_fields >= FIELD_DEFAULT)
     +		show_default(oid, type, pathname, mode, base);
     +
    - 	return retval;
    + 	return recursive;
      }
      
     @@ builtin/ls-tree.c: int cmd_ls_tree(int argc, const char **argv, const char *prefix)
 3:  296ebacafe !  7:  725c4d0187 ls-tree.c: introduce struct "shown_data"
    @@ Metadata
     Author: Teng Long <dyroneteng@gmail.com>
     
      ## Commit message ##
    -    ls-tree.c: introduce struct "shown_data"
    +    ls-tree.c: introduce struct "show_tree_data"
     
    -    "show_data" is a struct that packages the necessary fields for
    -    reusing. This commit is a front-loaded commit for support
    -    "--format" argument and does not affect any existing functionality.
    +    "show_tree_data" is a struct that packages the necessary fields for
    +    "show_tree()". This commit is a pre-prepared commit for supporting
    +    "--format" option and it does not affect any existing functionality.
     
         Signed-off-by: Teng Long <dyroneteng@gmail.com>
     
      ## builtin/ls-tree.c ##
    -@@ builtin/ls-tree.c: static unsigned int shown_bits;
    - #define SHOW_MODE (1 << 4)
    - #define SHOW_DEFAULT 29 /* 11101 size is not shown to output by default */
    +@@ builtin/ls-tree.c: static unsigned int shown_fields;
    + #define FIELD_DEFAULT 29 /* 11101 size is not shown to output by default */
    + #define FIELD_LONG_DEFAULT  (FIELD_DEFAULT | FIELD_SIZE)
      
    -+struct shown_data {
    ++struct show_tree_data {
     +	unsigned mode;
     +	enum object_type type;
     +	const struct object_id *oid;
    @@ builtin/ls-tree.c: static int show_recursive(const char *base, size_t baselen,
     -static int show_default(const struct object_id *oid, enum object_type type,
     -			const char *pathname, unsigned mode,
     -			struct strbuf *base)
    -+static int show_default(struct shown_data *data)
    ++static int show_default(struct show_tree_data *data)
      {
     -	size_t baselen = base->len;
     +	size_t baselen = data->base->len;
      
    - 	if (shown_bits & SHOW_SIZE) {
    + 	if (shown_fields & FIELD_SIZE) {
      		char size_text[24];
     -		if (type == OBJ_BLOB) {
     +		if (data->type == OBJ_BLOB) {
    @@ builtin/ls-tree.c: static int show_default(const struct object_id *oid, enum obj
      
     @@ builtin/ls-tree.c: static int show_tree(const struct object_id *oid, struct strbuf *base,
      {
    - 	int retval = 0;
    + 	int recursive = 0;
      	size_t baselen;
     -	enum object_type type = OBJ_BLOB;
    -+	struct shown_data data = {
    ++	struct show_tree_data data = {
     +		.mode = mode,
     +		.type = OBJ_BLOB,
     +		.oid = oid,
    @@ builtin/ls-tree.c: static int show_tree(const struct object_id *oid, struct strb
     +		.base = base,
     +	};
      
    --	if (show_tree_init(&type, base, pathname, mode, &retval))
    -+	if (show_tree_init(&data.type, base, pathname, mode, &retval))
    - 		return retval;
    --
    - 	if (!(shown_bits ^ SHOW_OBJECT_NAME)) {
    - 		printf("%s%c", find_unique_abbrev(oid, abbrev), line_termination);
    - 		return retval;
    +-	init_type(mode, &type);
    ++	init_type(mode, &data.type);
    + 	init_recursive(base, pathname, &recursive);
    + 
    +-	if (type == OBJ_TREE && recursive && !(ls_options & LS_SHOW_TREES))
    ++	if (data.type == OBJ_TREE && recursive && !(ls_options & LS_SHOW_TREES))
    + 		return recursive;
    +-	if (type == OBJ_BLOB && (ls_options & LS_TREE_ONLY))
    ++	if (data.type == OBJ_BLOB && (ls_options & LS_TREE_ONLY))
    + 		return !READ_TREE_RECURSIVE;
    + 
    + 	if (shown_fields == FIELD_OBJECT_NAME) {
     @@ builtin/ls-tree.c: static int show_tree(const struct object_id *oid, struct strbuf *base,
    + 	}
      
    - 	if (!(shown_bits ^ SHOW_DEFAULT) ||
    - 	    !(shown_bits ^ (SHOW_DEFAULT | SHOW_SIZE)))
    + 	if (shown_fields >= FIELD_DEFAULT)
     -		show_default(oid, type, pathname, mode, base);
     +		show_default(&data);
      
    - 	return retval;
    + 	return recursive;
      }
 4:  e0add802fb !  8:  7df58483a4 ls-tree.c: introduce "--format" option
    @@ Commit message
             Range (min … max):   328.8 ms … 349.4 ms    10 runs
     
         Links:
    -    [1] https://public-inbox.org/git/RFC-patch-6.7-eac299f06ff-20211217T131635Z-avarab@gmail.com/
    +            [1] https://public-inbox.org/git/RFC-patch-6.7-eac299f06ff-20211217T131635Z-avarab@gmail.com/
     
         Signed-off-by: Teng Long <dyroneteng@gmail.com>
     
    @@ Documentation/git-ls-tree.txt: quoted as explained for the configuration variabl
     +For example, if you want to only print the <object> and <file> fields with a
     +JSON style, executing with a specific "--format" like
     +
    -+		git ls-tree --format='{"object":"%(object)", "file":"%(file)"}' <tree-ish>
    ++        git ls-tree --format='{"object":"%(object)", "file":"%(file)"}' <tree-ish>
     +
     +The output format changes to:
     +
    -+		{"object":"<object>", "file":"<file>"}
    ++        {"object":"<object>", "file":"<file>"}
     +
     +FIELD NAMES
     +-----------
    @@ builtin/ls-tree.c: enum {
      
      static int cmdmode = MODE_UNSPECIFIED;
      
    --static int parse_shown_fields(void)
     +static const char *format;
     +static const char *default_format = "%(mode) %(type) %(object)%x09%(file)";
     +static const char *long_format = "%(mode) %(type) %(object) %(size:padded)%x09%(file)";
     +static const char *name_only_format = "%(file)";
     +static const char *object_only_format = "%(object)";
     +
    + static int parse_shown_fields(void)
    + {
    + 	if (cmdmode == MODE_NAME_ONLY) {
    +@@ builtin/ls-tree.c: static int parse_shown_fields(void)
    + 	return 1;
    + }
    + 
     +static void expand_objectsize(struct strbuf *line, const struct object_id *oid,
     +			      const enum object_type type, unsigned int padded)
    - {
    --	if (cmdmode == MODE_NAME_ONLY) {
    --		shown_bits = SHOW_FILE_NAME;
    --		return 0;
    ++{
     +	if (type == OBJ_BLOB) {
     +		unsigned long size;
     +		if (oid_object_info(the_repository, oid, &size) < 0)
    @@ builtin/ls-tree.c: enum {
     +		strbuf_addf(line, "%7s", "-");
     +	} else {
     +		strbuf_addstr(line, "-");
    - 	}
    --	if (cmdmode == MODE_OBJECT_ONLY) {
    --		shown_bits = SHOW_OBJECT_NAME;
    --		return 0;
    ++	}
     +}
     +
     +static size_t expand_show_tree(struct strbuf *line, const char *start,
     +			       void *context)
     +{
    -+	struct shown_data *data = context;
    ++	struct show_tree_data *data = context;
     +	const char *end;
     +	const char *p;
     +	unsigned int errlen;
    -+	size_t len;
    -+	len = strbuf_expand_literal_cb(line, start, NULL);
    ++	size_t len = strbuf_expand_literal_cb(line, start, NULL);
    ++
     +	if (len)
     +		return len;
    -+
     +	if (*start != '(')
     +		die(_("bad ls-tree format: as '%s'"), start);
     +
    @@ builtin/ls-tree.c: enum {
     +	} else if (skip_prefix(start, "(size)", &p)) {
     +		expand_objectsize(line, data->oid, data->type, 0);
     +	} else if (skip_prefix(start, "(object)", &p)) {
    -+		strbuf_addstr(line, find_unique_abbrev(data->oid, abbrev));
    ++		strbuf_add_unique_abbrev(line, data->oid, abbrev);
     +	} else if (skip_prefix(start, "(file)", &p)) {
     +		const char *name = data->base->buf;
     +		const char *prefix = chomp_prefix ? ls_tree_prefix : NULL;
    @@ builtin/ls-tree.c: enum {
     +		strbuf_addstr(data->base, data->pathname);
     +		name = relative_path(data->base->buf, prefix, &sb);
     +		quote_c_style(name, &quoted, NULL, 0);
    -+		strbuf_addstr(line, quoted.buf);
    ++		strbuf_addbuf(line, &quoted);
    ++		strbuf_release(&sb);
    ++		strbuf_release(&quoted);
     +	} else {
     +		errlen = (unsigned long)len;
     +		die(_("bad ls-tree format: %%%.*s"), errlen, start);
    - 	}
    --	if (!ls_options || (ls_options & LS_RECURSIVE)
    --	    || (ls_options & LS_SHOW_TREES)
    --	    || (ls_options & LS_TREE_ONLY))
    --		shown_bits = SHOW_DEFAULT;
    --	if (cmdmode == MODE_LONG)
    --		shown_bits = SHOW_DEFAULT | SHOW_SIZE;
    --	return 1;
    ++	}
     +	return len;
    - }
    - 
    ++}
    ++
      static int show_recursive(const char *base, size_t baselen,
    + 			  const char *pathname)
    + {
     @@ builtin/ls-tree.c: static int show_recursive(const char *base, size_t baselen,
      	return 0;
      }
      
    -+static int show_tree_init(enum object_type *type, struct strbuf *base,
    -+			  const char *pathname, unsigned mode, int *retval)
    ++static void init_recursive(struct strbuf *base, const char *pathname,
    ++				int *recursive)
     +{
    -+	if (S_ISGITLINK(mode)) {
    ++	if (show_recursive(base->buf, base->len, pathname))
    ++		*recursive = READ_TREE_RECURSIVE;
    ++}
    ++
    ++static void init_type(unsigned mode, enum object_type *type)
    ++{
    ++	if (S_ISGITLINK(mode))
     +		*type = OBJ_COMMIT;
    -+	} else if (S_ISDIR(mode)) {
    -+		if (show_recursive(base->buf, base->len, pathname)) {
    -+			*retval = READ_TREE_RECURSIVE;
    -+			if (!(ls_options & LS_SHOW_TREES))
    -+				return 1;
    -+		}
    ++	else if (S_ISDIR(mode))
     +		*type = OBJ_TREE;
    -+	}
    -+	else if (ls_options & LS_TREE_ONLY)
    -+		return 1;
    -+	return 0;
     +}
     +
     +static int show_tree_fmt(const struct object_id *oid, struct strbuf *base,
     +			 const char *pathname, unsigned mode, void *context)
     +{
     +	size_t baselen;
    -+	int retval = 0;
    ++	int recursive = 0;
     +	struct strbuf line = STRBUF_INIT;
    -+	struct shown_data data = {
    ++	struct show_tree_data data = {
     +		.mode = mode,
     +		.type = OBJ_BLOB,
     +		.oid = oid,
    @@ builtin/ls-tree.c: static int show_recursive(const char *base, size_t baselen,
     +		.base = base,
     +	};
     +
    -+	if (show_tree_init(&data.type, base, pathname, mode, &retval))
    -+		return retval;
    ++	init_type(mode, &data.type);
    ++	init_recursive(base, pathname, &recursive);
    ++
    ++	if (data.type == OBJ_TREE && recursive && !(ls_options & LS_SHOW_TREES))
    ++		return recursive;
    ++	if (data.type == OBJ_BLOB && (ls_options & LS_TREE_ONLY))
    ++		return !READ_TREE_RECURSIVE;
     +
     +	baselen = base->len;
     +	strbuf_expand(&line, format, expand_show_tree, &data);
     +	strbuf_addch(&line, line_termination);
     +	fwrite(line.buf, line.len, 1, stdout);
    ++	strbuf_release(&line);
     +	strbuf_setlen(base, baselen);
    -+	return retval;
    ++	return recursive;
     +}
     +
    -+static int parse_shown_fields(void)
    -+{
    -+	if (cmdmode == MODE_NAME_ONLY ||
    -+	    (format && !strcmp(format, name_only_format))) {
    -+		shown_bits = SHOW_FILE_NAME;
    -+		return 1;
    -+	}
    -+
    -+	if (cmdmode == MODE_OBJECT_ONLY ||
    -+	    (format && !strcmp(format, object_only_format))) {
    -+		shown_bits = SHOW_OBJECT_NAME;
    -+		return 1;
    -+	}
    -+
    -+	if (!ls_options || (ls_options & LS_RECURSIVE)
    -+	    || (ls_options & LS_SHOW_TREES)
    -+	    || (ls_options & LS_TREE_ONLY)
    -+		|| (format && !strcmp(format, default_format)))
    -+		shown_bits = SHOW_DEFAULT;
    -+
    -+	if (cmdmode == MODE_LONG ||
    -+		(format && !strcmp(format, long_format)))
    -+		shown_bits = SHOW_DEFAULT | SHOW_SIZE;
    -+	return 1;
    -+}
    -+
    - static int show_default(struct shown_data *data)
    + static int show_default(struct show_tree_data *data)
      {
      	size_t baselen = data->base->len;
    -@@ builtin/ls-tree.c: static int show_default(struct shown_data *data)
    +@@ builtin/ls-tree.c: static int show_default(struct show_tree_data *data)
      	return 1;
      }
      
    --static int show_tree_init(enum object_type *type, struct strbuf *base,
    --			  const char *pathname, unsigned mode, int *retval)
    +-static void init_type(unsigned mode, enum object_type *type)
     -{
    --	if (S_ISGITLINK(mode)) {
    +-	if (S_ISGITLINK(mode))
     -		*type = OBJ_COMMIT;
    --	} else if (S_ISDIR(mode)) {
    --		if (show_recursive(base->buf, base->len, pathname)) {
    --			*retval = READ_TREE_RECURSIVE;
    --			if (!(ls_options & LS_SHOW_TREES))
    --				return 1;
    --		}
    +-	else if (S_ISDIR(mode))
     -		*type = OBJ_TREE;
    --	}
    --	else if (ls_options & LS_TREE_ONLY)
    --		return 1;
    --	return 0;
    +-}
    +-
    +-static void init_recursive(struct strbuf *base, const char *pathname,
    +-				int *recursive)
    +-{
    +-	if (show_recursive(base->buf, base->len, pathname))
    +-		*recursive = READ_TREE_RECURSIVE;
     -}
     -
      static int show_tree(const struct object_id *oid, struct strbuf *base,
    @@ builtin/ls-tree.c: int cmd_ls_tree(int argc, const char **argv, const char *pref
     +	 * The generic show_tree_fmt() is slower than show_tree(), so
     +	 * take the fast path if possible.
     +	 */
    -+	if (format && (!strcmp(format, default_format) ||
    -+				   !strcmp(format, long_format) ||
    -+				   !strcmp(format, name_only_format) ||
    -+				   !strcmp(format, object_only_format)))
    ++	if (format &&
    ++	    (!strcmp(format, default_format) ||
    ++	     !strcmp(format, long_format) ||
    ++	     !strcmp(format, name_only_format) ||
    ++	     !strcmp(format, object_only_format)))
     +		fn = show_tree;
     +	else if (format)
     +		fn = show_tree_fmt;
 -:  ---------- >  9:  8dafb2b377 cocci: allow padding with `strbuf_addf()`

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>

Remove code added in f35a6d3bce7 (Teach core object handling functions about gitlinks, 2007-04-09), later patched in 7d0b18a4da1 (Add output flushing before fork(), 2008-08-04), and then finally ending up in its current form in d3bee161fef (tree.c: allow read_tree_recursive() to traverse gitlink entries, 2009-01-25). All while being commented-out!

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/ls-tree.c | 9 ---------
 1 file changed, 9 deletions(-)

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>

Add missing {} to the "else" arms in show_tree() per the CodingGuidelines.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/ls-tree.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>

Change the ls-tree.c code to use type_name() on the enum instead of using the string constants. This doesn't matter either way for performance, but makes this a bit easier to read as we'll no longer need a strcmp() here.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/ls-tree.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>

The "struct strbuf"'s "len" member is a "size_t", not an "int", so let's change our corresponding types accordingly. This also changes the "len" and "speclen" variables, which are likewise used to store the return value of strlen(), which returns "size_t", not "int".

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/ls-tree.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

The variable which "show_tree()" return is named "retval", a name that's a little hard to understand. This commit tries to make the variable and the related codes more clear in the context.

The change is based on three steps. The first is to rename "retval" to a more meaningful name.

The second is that there are different "return" cases in "show_tree", some places use "return retval;", some just directly use "return 0;", this maybe cause some confusion when reading these "returns". For this , we change all the "return" cases to the new uniform name.

The last is there are some nested "if" judgments surround the "returns", this even make the codes here a little hard to understand. So we put some logic in individual methods, "init_type()" and "init_recursive()".

After the steps, let us look at "show_tree()" again. It has a uniform return variable name now, and first we init the "type" by "mode", then call "init_recursive" to init the value of "recursive" which means whether to go on reading recusively into the "tree". The codes here become a little bit clearer, so we do not need to take a look at "read_tree_at()" in "tree.c" to make sure the context of the return value.

Signed-off-by: Teng Long <dyronetengb@gmail.com>
---
 builtin/ls-tree.c | 38 ++++++++++++++++++++++++--------------
 1 file changed, 24 insertions(+), 14 deletions(-)

Teng Long <dyroneteng@gmail.com> writes:

What this one is doing sounds more like setting the type variable based on the mode bits, and doing only half a job at it. The name "init" does not sound like a good match to what it does.

If we make it a separate function, we probably should add the "else" clause to set *type to OBJ_BLOB there, so that the caller does not say "we'd assume it is BLOB initially, but tweak it based on mode bits".

I.e.

	type = get_type(mode);

where

	static enum object_type get_type(unsigned int mode)
	{
		return (S_ISGITLINK(mode) 
		        ? OBJ_COMMIT
			: S_ISDIR(mode)
			? OBJ_TREE
			: OBJ_BLOB);
	}

or something like that, perhaps? But I think open-coding the whole thing, after losing the "We assume BLOB" initialization, would be much easier to follow, i.e.

	if (S_ISGITLINK(mode))
		type = OBJ_COMMIT;
	else if (S_ISDIR(mode))
		type = OBJ_TREE;
	else
		type = OBJ_BLOB;

without adding init_type() helper function.

This is even less readable. In the original, it was clear that we only call show_recursive() on a path that is a true directory; we now seem to unconditionally make a call to it. Is that intended?

	Side note.  show_recursive() has a confusing name; it does
	not show anything---it only decides if we want to go
	recursive.

At least, losing the "we assume recursive is 0" upfront in the variable declaration and writing

	if (type == OBJ_TREE && show_recursive(...))
		recursive = READ_TREE_RECURSIVE;
	else
		recursive = 0;

here, without introducing init_recursive(), would make it easier to follow. If we really want to add a new function, perhaps

	recursive = get_recursive(type, base, pathname);
 
where

	static int get_recursive(enum object_type type,
				 struct strbuf *base, const char *pathname)
	{
		if (type == OBJ_TREE && show_recursive(...))
			return READ_TREE_RECURSIVE;
		else
			return 0;
	}

but I fail to see the point of doing so; open-coded 4 lines here would make the flow of thought much better to me.

In any case, I think your splitting the original into "this is about type" and "this is about the recursive bit" is a good idea to help making the resulting code easier to follow.

We are looking at an entry that is a directory. We are running in recursive mode. And we are not told to show the directory itself in the output. We skip the rest of the function, which is about to show this single entry. Makes sense.

Negation of a non-zero integer constant is 0, so it is the same as the original that returned 0, but I am not sure if it is enhancing or hurting readability of the code. The user of the value, in tree.c::read_tree_at(), knows that the possible and valid values are 0 and READ_TREE_RECURSIVE, so returning 0 would probably be a better idea. After all, the initializer in the original for the variable definition of "retval" used "0", not "!READ_TREE_RECURSIVE".

The name "recursive" is much more specific than the overly generic "retval". Its value is to be consumed by read_tree_at(), i.e. our caller, to decide if we want it to recurse into the contents of the directory. I would have called it "recurse" (or even "to_recurse"), if I were doing this change, though.

On Fri, Jan 7, 2022 at 4:44 AM Junio C Hamano <gitster@pobox.com> wrote:

Thanks, will apply in the next patch.

We usually pipe the output from `git ls-trees` to tools like `sed` or `cut` when we only want to extract some fields.

When we want only the pathname component, we can pass `--name-only` option to omit such a pipeline, but there are no options for extracting other fields.

Teach the "--object-only" option to the command to only show the object name. This option cannot be used together with "--name-only" or "--long" , they are mutually exclusive (actually "--name-only" and "--long" can be combined together before, this commit by the way fix this bug).

A simple refactoring was done to the "show_tree" function, intead by using bitwise operations to recognize the format for printing to stdout. The reason for doing this is that we don't want to increase the readability difficulty with the addition of "-object-only", making this part of the logic easier to read and expand.

In terms of performance, there is no loss comparing to the "master" (2ae0a9cb8298185a94e5998086f380a355dd8907), here are the results of the performance tests in my environment based on linux repository:

    $hyperfine --warmup=10 "/opt/git/master/bin/git ls-tree -r HEAD"
    Benchmark 1: /opt/git/master/bin/git ls-tree -r HEAD
    Time (mean ± σ):     105.8 ms ±   2.7 ms    [User: 85.7 ms, System: 20.0 ms]
    Range (min … max):   101.5 ms … 111.3 ms    28 runs

    $hyperfine --warmup=10 "/opt/git/ls-tree-oid-only/bin/git ls-tree -r HEAD"
    Benchmark 1: /opt/git/ls-tree-oid-only/bin/git ls-tree -r HEAD
    Time (mean ± σ):     105.0 ms ±   3.0 ms    [User: 83.7 ms, System: 21.2 ms]
    Range (min … max):    99.3 ms … 109.5 ms    27 runs

    $hyperfine --warmup=10 "/opt/git/master/bin/git ls-tree -r -l HEAD"
    Benchmark 1: /opt/git/master/bin/git ls-tree -r -l HEAD
    Time (mean ± σ):     337.4 ms ±  10.9 ms    [User: 308.3 ms, System: 29.0 ms]
    Range (min … max):   323.0 ms … 355.0 ms    10 runs

    $hyperfine --warmup=10 "/opt/git/ls-tree-oid-only/bin/git ls-tree -r -l HEAD"
    Benchmark 1: /opt/git/ls-tree-oid-only/bin/git ls-tree -r -l HEAD
    Time (mean ± σ):     337.6 ms ±   6.2 ms    [User: 309.4 ms, System: 28.1 ms]
    Range (min … max):   330.4 ms … 349.9 ms    10 runs

Signed-off-by: Teng Long <dyroneteng@gmail.com>
---
 Documentation/git-ls-tree.txt |   7 +-
 builtin/ls-tree.c             | 141 +++++++++++++++++++++++++---------
 t/t3104-ls-tree-oid.sh        |  51 ++++++++++++
 3 files changed, 160 insertions(+), 39 deletions(-)
 create mode 100755 t/t3104-ls-tree-oid.sh

"show_tree_data" is a struct that packages the necessary fields for "show_tree()". This commit is a pre-prepared commit for supporting "--format" option and it does not affect any existing functionality.

Signed-off-by: Teng Long <dyroneteng@gmail.com>
---
 builtin/ls-tree.c | 50 +++++++++++++++++++++++++++++------------------
 1 file changed, 31 insertions(+), 19 deletions(-)

Add a --format option to ls-tree. It has an existing default output, and then --long and --name-only options to emit the default output along with the objectsize and, or to only emit object paths.

Rather than add --type-only, --object-only etc. we can just support a --format using a strbuf_expand() similar to "for-each-ref --format". We might still add such options in the future for convenience.

The --format implementation is slower than the existing code, but this change does not cause any performance regressions. We'll leave the existing show_tree() unchanged, and only run show_tree_fmt() in if a --format different than the hardcoded built-in ones corresponding to the existing modes is provided.

I.e. something like the "--long" output would be much slower with this, mainly due to how we need to allocate various things to do with quote.c instead of spewing the output directly to stdout.

The new option of '--format' comes from Ævar Arnfjörð Bjarmasonn's idea and suggestion, this commit makes modifications in terms of the original discussion on community [1].

Here is the statistics about performance tests:

1. Default format (hitten the builtin formats):

    "git ls-tree <tree-ish>" vs "--format='%(mode) %(type) %(object)%x09%(file)'"

    $hyperfine --warmup=10 "/opt/git/master/bin/git ls-tree -r HEAD"
    Benchmark 1: /opt/git/master/bin/git ls-tree -r HEAD
    Time (mean ± σ):     105.2 ms ±   3.3 ms    [User: 84.3 ms, System: 20.8 ms]
    Range (min … max):    99.2 ms … 113.2 ms    28 runs

    $hyperfine --warmup=10 "/opt/git/ls-tree-oid-only/bin/git ls-tree -r --format='%(mode) %(type) %(object)%x09%(file)'  HEAD"
    Benchmark 1: /opt/git/ls-tree-oid-only/bin/git ls-tree -r --format='%(mode) %(type) %(object)%x09%(file)'  HEAD
    Time (mean ± σ):     106.4 ms ±   2.7 ms    [User: 86.1 ms, System: 20.2 ms]
    Range (min … max):   100.2 ms … 110.5 ms    29 runs

2. Default format includes object size (hitten the builtin formats):

    "git ls-tree -l <tree-ish>" vs "--format='%(mode) %(type) %(object) %(size:padded)%x09%(file)'"

    $hyperfine --warmup=10 "/opt/git/master/bin/git ls-tree -r -l HEAD"
    Benchmark 1: /opt/git/master/bin/git ls-tree -r -l HEAD
    Time (mean ± σ):     335.1 ms ±   6.5 ms    [User: 304.6 ms, System: 30.4 ms]
    Range (min … max):   327.5 ms … 348.4 ms    10 runs

    $hyperfine --warmup=10 "/opt/git/ls-tree-oid-only/bin/git ls-tree -r --format='%(mode) %(type) %(object) %(size:padded)%x09%(file)'  HEAD"
    Benchmark 1: /opt/git/ls-tree-oid-only/bin/git ls-tree -r --format='%(mode) %(type) %(object) %(size:padded)%x09%(file)'  HEAD
    Time (mean ± σ):     337.2 ms ±   8.2 ms    [User: 309.2 ms, System: 27.9 ms]
    Range (min … max):   328.8 ms … 349.4 ms    10 runs

Links:
	[1] https://public-inbox.org/git/RFC-patch-6.7-eac299f06ff-20211217T131635Z-avarab@gmail.com/

Signed-off-by: Teng Long <dyroneteng@gmail.com>
---
 Documentation/git-ls-tree.txt |  50 ++++++++++-
 builtin/ls-tree.c             | 158 ++++++++++++++++++++++++++++++----
 t/t3105-ls-tree-format.sh     |  55 ++++++++++++
 3 files changed, 243 insertions(+), 20 deletions(-)
 create mode 100755 t/t3105-ls-tree-format.sh

Hi Teng,

On Fri, 7 Jan 2022 at 06:34, Teng Long <dyroneteng@gmail.com> wrote:

Above, there is a missing space just before "are". That causes the manpage to render a little bit funny.

I had to re-read this to understand. How about the following?

    It is possible to print in a custom format by using the `--format`
    option, which is able to interpolate different fields using a
    `%(fieldname)` notation.

Just a suggestion. Feel free to tweak or ignore. :-)

Nice!

Martin

On Tue, Jan 11, 2022 at 3:41 AM Martin Ågren <martin.agren@gmail.com> wrote:

Thanks for reviewing.  will be fixed in the next patch.

Yours reads more smoothly than mine, it will be applied in next patch.

Thanks.

A convenient way to pad strings is to use something like `strbuf_addf(&buf, "%20s", "Hello, world!")`.

However, the Coccinelle rule that forbids a format `"%s"` with a constant string argument cast too wide a net, and also forbade such padding.

The original rule was introduced by commit:

    https://github.com/git/git/commit/28c23cd4c3902449aff72cb9a4a703220be0d6ac

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Signed-off-by: Teng Long <dyroneteng@gmail.com>
---
 contrib/coccinelle/strbuf.cocci | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Hi Teng,

On Thu, 6 Jan 2022, Teng Long wrote:

Doing this in 9/9 is too late, by this time you already introduced the code site that requires this workaround.

At the same time, I wonder why you want to defend spinning up the full-blown `printf()` machinery just to pad text that you can easily pad yourself. It sounds like a lot of trouble to me to introduce this patch and then use an uncommon method to pad a fixed string at runtime. Too much trouble for my liking.

Ciao, Dscho

I am not sure whether I have sent the email repeatedly, because it is not shown on mailist. If so, sorry to bother you.

Johannes Schindelin writes:

Yes, you are correct. Will fixed if the patch is still remained to next one.

I may not have explained it clearly in the cover. Sorry for that, I'm going to explain some more here, please correct me if there is something wrong or the method is not recommended or is not best practice in community.

Firstly, the patch needs to be introduced I think and it has nothing to do with using " -" or "%7s" here, because the fix recommandation is not accurate in terms of the "static-analysis" report if someone just uses the "addf" api:

-               strbuf_addf(line, "%7s", "-");
+               strbuf_addstr(line, "-");

They have different execution results and bring confusion to people. 

Then secondly, about the using "strbuf_addf(line, "%7s" , "-");" or "strbuf_addstr(line, " -");". I think you prefer the later and I prefer the former, right? (I'm not a native English speaker, so I just want to make sure I understand whole your meannings).

If I understand everything correctly so far, it's good :)

As I metioned in a previous reply [1], I think there is no performance issue here..

Why I prefer more of the former that is because, for the single line, it's more readable I think. Maybe it's not going to modify very often, but If someone want to know what this is, might have to do a count. So I don't think this is any more readable than "%7s".

Here's what I think and looking forward to your reply.

Thanks.

[1] https://public-inbox.org/git/CADMgQSRxko6nC0zfDiVVfL2ZkdQVbBq0s59Er+6Nmg9vz4uJKQ@mail.gmail.com/

Hi Teng,

On Mon, 10 Jan 2022, Teng Long wrote:

I strongly disagree. Using a format requires the reader to interpret a `printf()` format, to remember (if they ever knew) the rules about padding with `%<number>s` formats, and then to satisfy themselves that the result is correct.

That's quite the cognitive load you put on the reader for something as trivial as " -".

Not a fan, Johannes

Ok. I will modify the next patch according to your opinion, I just hope to understand the problems and make better contributions in the future.

Thanks.

On Mon, Jan 10, 2022 at 8:49 PM Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

Both "more readable" and "cognitive load" are quite subjective.

FWIW, I have a slight preference to the former because I do not have to count the whitespaces to figure out at which column the construct is trying to align to. Most of the time, however, I may not deeply care if the thing is aligned exactly, and " -" might be easier to scan and getting alarmed by seeing "%7s" to wonder if there is something unusual going on. When I am reading not for finding out the precise output format but for general correctness, bunch of unknown number of spaces followed by a dash might be easier to see.

But once you know the language, "%7s" is not so alarming, and it does make it easier to see both for casual scanning and for counting columns. It also is likely that those who know the language would make more efficient developers to fix and/or enhance the code, so I prefer to optimize the code for them.

On Mon, Jan 10 2022, Johannes Schindelin wrote:

I think you can argue that, but saying that this series must change that existing "%7s" format just because it happened to trip over an existin coccinelle rule as code was changed from printf() to strbuf_addf() is going overboard.

Also, the ls-tree output has existing alignment issues, and the documentation says:

    "right-justified with minimum width of 7 characters"

So I'd think we'd want to keep the %7s, and in some future change change that format to be dynamic so we'd align things properly if some fields were longer than 7 characters.

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

Good to point this out.  Thanks.

On Thu, Jan 06 2022, Teng Long wrote:

Let's refer to commits like this:

    28c23cd4c39 (strbuf.cocci: suggest strbuf_addbuf() to add one strbuf to an other, 2019-01-25)

That file currently has:

     18:format F =~ "s";
     26:format F =~ "s";
     47:format F =~ "s";

If we're fixing these let's fix the other logic errors as well.

On Tue, Jan 11, 2022 at 2:02 AM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:

OK, I will.

Thanks for the reminding， they'll be applied in the next patch.

On Mon, Jan 10, 2022 at 07:00:59PM +0100, Ævar Arnfjörð Bjarmason wrote:

I find it helpful to have an alias like:

    $ git config alias.ll
    !git always --no-pager log -1 --pretty='tformat:%h (%s, %ad)' --date=short

in my $HOME/.gitconfig so that I can easily format commits in the standard way.

I think that this alias came from Peff, but I can't remember.

Thanks, Taylor

Am 11.01.22 um 17:42 schrieb Taylor Blau:

You can shorten "--pretty='tformat:%h (%s, %ad)' --date=short" to "--pretty=reference" or "--format=reference". For me that's easy enough to remember that I don't need an alias.

Silly question, going further off-topic: What's "git always" doing?

René

On Tue, Jan 11, 2022 at 08:06:00PM +0100, René Scharfe wrote:

Ah, of course. Peff's copy likely predates `--pretty=reference`, and I inherited the cruft from him. Your suggestion has the nice benefit of colorizing the output when going to the terminal.

Oops, I should have mentioned. It's another alias to ensure that the following command is always run in a Git repository (either the current one or a hand-picked default):

    $ git config alias.always
    !git rev-parse 2>/dev/null || cd ~/src/git; git

I often read mail out of my home directory, and the above works with my `:Git` command in Vim (which passes its arguments to `git always` and inserts the result back into my buffer). That way I don't have to first `:cd ~/src/git` and then `:Git ll xyz`, I can just `:Git ll xyz` and it does what I meant most of the time.

Thanks, Taylor

On Wed, Jan 12, 2022 at 4:11 AM Taylor Blau <me@ttaylorr.com> wrote:

The same question is clear now。 Thanks for the explanations from Taylor Blau and René Scharfe.

On Tue, Jan 11 2022, Taylor Blau wrote:

Nowadays you can do this as:

    git show -s --pretty=reference

See Documentation/SubmittingPatches

I use:

    $ git help reference
    'reference' is aliased to '!git --no-pager log --pretty=reference -1'

On Wed, Jan 12, 2022 at 4:40 AM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:

Make sense. Thanks.

On Wed, Jan 12, 2022 at 12:42 AM Taylor Blau <me@ttaylorr.com> wrote:

Wow. That's cool and efficient.

Thanks.

Major diff from v9:

    1. Exchange the order of "cocci: allow padding with `strbuf_addf()`"
and "introduce "--format" option".
       
       (Advice from Johannes Schindelin) 
       Let coccillene's changes take effect before the commit with the rule
       check failure. 

    2. ls-tree.c: support --object-only option for "git-ls-tree"
       
       (Advice from Junio C Hamano)
       Rename the return value name from "recursive" to "recurse".
       Rename "init_type()" to "get_type()".
       Remove "init_recursive()"       

    3. ls-tree.c: introduce "--format" option
       
       (Advice from Martin Ågren)
       Fix some document errors and wording.
       Bug fixing in "t3105" (Always return 129 without specifying an argument).

    4. cocci: allow padding with `strbuf_addf()`  

       (Advice from Ævar Arnfjörð Bjarmason)
       Fix the other logic errors as well.

Thanks.

Teng Long (5):
  ls-tree: optimize naming and handling of "return" in show_tree()
  ls-tree.c: support --object-only option for "git-ls-tree"
  ls-tree.c: introduce struct "show_tree_data"
  cocci: allow padding with `strbuf_addf()`
  ls-tree.c: introduce "--format" option

Ævar Arnfjörð Bjarmason (4):
  ls-tree: remove commented-out code
  ls-tree: add missing braces to "else" arms
  ls-tree: use "enum object_type", not {blob,tree,commit}_type
  ls-tree: use "size_t", not "int" for "struct strbuf"'s "len"

 Documentation/git-ls-tree.txt   |  56 +++++-
 builtin/ls-tree.c               | 328 +++++++++++++++++++++++++-------
 contrib/coccinelle/strbuf.cocci |   6 +-
 t/t3104-ls-tree-oid.sh          |  51 +++++
 t/t3105-ls-tree-format.sh       |  55 ++++++
 5 files changed, 426 insertions(+), 70 deletions(-)
 create mode 100755 t/t3104-ls-tree-oid.sh
 create mode 100755 t/t3105-ls-tree-format.sh

Range-diff against v9:
 1:  75503c41a7 <  -:  ---------- ls-tree: optimize naming and handling of "return" in show_tree()
 -:  ---------- >  1:  2fcff7e0d4 ls-tree: remove commented-out code
 -:  ---------- >  2:  6fd1dd9383 ls-tree: add missing braces to "else" arms
 -:  ---------- >  3:  208654b5e2 ls-tree: use "enum object_type", not {blob,tree,commit}_type
 -:  ---------- >  4:  2637464fd8 ls-tree: use "size_t", not "int" for "struct strbuf"'s "len"
 -:  ---------- >  5:  b04188c822 ls-tree: optimize naming and handling of "return" in show_tree()
 2:  e0274f079a !  6:  bcfbc935b8 ls-tree.c: support --object-only option for "git-ls-tree"
    @@ builtin/ls-tree.c
      {
      	int i;
      
    -@@ builtin/ls-tree.c: static int show_recursive(const char *base, size_t baselen, const char *pathname
    - 	return 0;
    +@@ builtin/ls-tree.c: static enum object_type get_type(unsigned int mode)
    + 	        : OBJ_BLOB);
      }
      
     +static int show_default(const struct object_id *oid, enum object_type type,
    @@ builtin/ls-tree.c: static int show_recursive(const char *base, size_t baselen, c
     +	return 1;
     +}
     +
    - static void init_type(unsigned mode, enum object_type *type)
    + static int show_tree(const struct object_id *oid, struct strbuf *base,
    + 		const char *pathname, unsigned mode, void *context)
      {
    - 	if (S_ISGITLINK(mode))
     @@ builtin/ls-tree.c: static int show_tree(const struct object_id *oid, struct strbuf *base,
      	if (type == OBJ_BLOB && (ls_options & LS_TREE_ONLY))
    - 		return !READ_TREE_RECURSIVE;
    + 		return 0;
      
     -	if (!(ls_options & LS_NAME_ONLY)) {
     -		if (ls_options & LS_SHOW_SIZE) {
    @@ builtin/ls-tree.c: static int show_tree(const struct object_id *oid, struct strb
     -		}
     +	if (shown_fields == FIELD_OBJECT_NAME) {
     +		printf("%s%c", find_unique_abbrev(oid, abbrev), line_termination);
    -+		return recursive;
    ++		return recurse;
      	}
     -	baselen = base->len;
     -	strbuf_addstr(base, pathname);
    @@ builtin/ls-tree.c: static int show_tree(const struct object_id *oid, struct strb
     +					   chomp_prefix ? ls_tree_prefix : NULL,
     +					   stdout, line_termination);
     +		strbuf_setlen(base, baselen);
    -+		return recursive;
    ++		return recurse;
     +	}
     +
     +	if (shown_fields >= FIELD_DEFAULT)
     +		show_default(oid, type, pathname, mode, base);
     +
    - 	return recursive;
    + 	return recurse;
      }
      
     @@ builtin/ls-tree.c: int cmd_ls_tree(int argc, const char **argv, const char *prefix)
 3:  725c4d0187 !  7:  3ddffa1027 ls-tree.c: introduce struct "show_tree_data"
    @@ builtin/ls-tree.c: static unsigned int shown_fields;
      static const  char * const ls_tree_usage[] = {
      	N_("git ls-tree [<options>] <tree-ish> [<path>...]"),
      	NULL
    -@@ builtin/ls-tree.c: static int show_recursive(const char *base, size_t baselen,
    - 	return 0;
    +@@ builtin/ls-tree.c: static enum object_type get_type(unsigned int mode)
    + 	        : OBJ_BLOB);
      }
      
     -static int show_default(const struct object_id *oid, enum object_type type,
    @@ builtin/ls-tree.c: static int show_default(const struct object_id *oid, enum obj
      }
      
     @@ builtin/ls-tree.c: static int show_tree(const struct object_id *oid, struct strbuf *base,
    - {
    - 	int recursive = 0;
      	size_t baselen;
    --	enum object_type type = OBJ_BLOB;
    + 	enum object_type type = get_type(mode);
    + 
     +	struct show_tree_data data = {
     +		.mode = mode,
    -+		.type = OBJ_BLOB,
    ++		.type = type,
     +		.oid = oid,
     +		.pathname = pathname,
     +		.base = base,
     +	};
    - 
    --	init_type(mode, &type);
    -+	init_type(mode, &data.type);
    - 	init_recursive(base, pathname, &recursive);
    - 
    --	if (type == OBJ_TREE && recursive && !(ls_options & LS_SHOW_TREES))
    -+	if (data.type == OBJ_TREE && recursive && !(ls_options & LS_SHOW_TREES))
    - 		return recursive;
    --	if (type == OBJ_BLOB && (ls_options & LS_TREE_ONLY))
    -+	if (data.type == OBJ_BLOB && (ls_options & LS_TREE_ONLY))
    - 		return !READ_TREE_RECURSIVE;
    - 
    - 	if (shown_fields == FIELD_OBJECT_NAME) {
    ++
    + 	if (type == OBJ_TREE && show_recursive(base->buf, base->len, pathname))
    + 		recurse = READ_TREE_RECURSIVE;
    + 	if (type == OBJ_TREE && recurse && !(ls_options & LS_SHOW_TREES))
     @@ builtin/ls-tree.c: static int show_tree(const struct object_id *oid, struct strbuf *base,
      	}
      
    @@ builtin/ls-tree.c: static int show_tree(const struct object_id *oid, struct strb
     -		show_default(oid, type, pathname, mode, base);
     +		show_default(&data);
      
    - 	return recursive;
    + 	return recurse;
      }
 -:  ---------- >  8:  4b58a707c2 cocci: allow padding with `strbuf_addf()`
 4:  7df58483a4 !  9:  db058bf670 ls-tree.c: introduce "--format" option
    @@ Documentation/git-ls-tree.txt: OPTIONS
     +--format=<format>::
     +	A string that interpolates `%(fieldname)` from the result
     +	being shown. It also interpolates `%%` to `%`, and
    -+	`%xx` where `xx`are hex digits interpolates to character
    ++	`%xx` where `xx` are hex digits interpolates to character
     +	with hex code `xx`; for example `%00` interpolates to
     +	`\0` (NUL), `%09` to `\t` (TAB) and `%0a` to `\n` (LF).
     +	When specified, `--format` cannot be combined with other
    @@ Documentation/git-ls-tree.txt: quoted as explained for the configuration variabl
      
     +Customized format:
     +
    -+It's support to print customized format by `%(fieldname)` with `--format` option.
    ++It is possible to print in a custom format by using the `--format` option,
    ++which is able to interpolate different fields using a `%(fieldname)` notation.
     +For example, if you want to only print the <object> and <file> fields with a
     +JSON style, executing with a specific "--format" like
     +
    @@ builtin/ls-tree.c: static int parse_shown_fields(void)
      static int show_recursive(const char *base, size_t baselen,
      			  const char *pathname)
      {
    -@@ builtin/ls-tree.c: static int show_recursive(const char *base, size_t baselen,
    - 	return 0;
    +@@ builtin/ls-tree.c: static enum object_type get_type(unsigned int mode)
    + 	        : OBJ_BLOB);
      }
      
    -+static void init_recursive(struct strbuf *base, const char *pathname,
    -+				int *recursive)
    -+{
    -+	if (show_recursive(base->buf, base->len, pathname))
    -+		*recursive = READ_TREE_RECURSIVE;
    -+}
    -+
    -+static void init_type(unsigned mode, enum object_type *type)
    -+{
    -+	if (S_ISGITLINK(mode))
    -+		*type = OBJ_COMMIT;
    -+	else if (S_ISDIR(mode))
    -+		*type = OBJ_TREE;
    -+}
    -+
     +static int show_tree_fmt(const struct object_id *oid, struct strbuf *base,
     +			 const char *pathname, unsigned mode, void *context)
     +{
     +	size_t baselen;
    -+	int recursive = 0;
    ++	int recurse = 0;
     +	struct strbuf line = STRBUF_INIT;
    ++	enum object_type type = get_type(mode);
    ++
     +	struct show_tree_data data = {
     +		.mode = mode,
    -+		.type = OBJ_BLOB,
    ++		.type = type,
     +		.oid = oid,
     +		.pathname = pathname,
     +		.base = base,
     +	};
     +
    -+	init_type(mode, &data.type);
    -+	init_recursive(base, pathname, &recursive);
    -+
    -+	if (data.type == OBJ_TREE && recursive && !(ls_options & LS_SHOW_TREES))
    -+		return recursive;
    -+	if (data.type == OBJ_BLOB && (ls_options & LS_TREE_ONLY))
    -+		return !READ_TREE_RECURSIVE;
    ++	if (type == OBJ_TREE && show_recursive(base->buf, base->len, pathname))
    ++		recurse = READ_TREE_RECURSIVE;
    ++	if (type == OBJ_TREE && recurse && !(ls_options & LS_SHOW_TREES))
    ++		return recurse;
    ++	if (type == OBJ_BLOB && (ls_options & LS_TREE_ONLY))
    ++		return 0;
     +
     +	baselen = base->len;
     +	strbuf_expand(&line, format, expand_show_tree, &data);
    @@ builtin/ls-tree.c: static int show_recursive(const char *base, size_t baselen,
     +	fwrite(line.buf, line.len, 1, stdout);
     +	strbuf_release(&line);
     +	strbuf_setlen(base, baselen);
    -+	return recursive;
    ++	return recurse;
     +}
     +
      static int show_default(struct show_tree_data *data)
      {
      	size_t baselen = data->base->len;
    -@@ builtin/ls-tree.c: static int show_default(struct show_tree_data *data)
    - 	return 1;
    - }
    - 
    --static void init_type(unsigned mode, enum object_type *type)
    --{
    --	if (S_ISGITLINK(mode))
    --		*type = OBJ_COMMIT;
    --	else if (S_ISDIR(mode))
    --		*type = OBJ_TREE;
    --}
    --
    --static void init_recursive(struct strbuf *base, const char *pathname,
    --				int *recursive)
    --{
    --	if (show_recursive(base->buf, base->len, pathname))
    --		*recursive = READ_TREE_RECURSIVE;
    --}
    --
    - static int show_tree(const struct object_id *oid, struct strbuf *base,
    - 		const char *pathname, unsigned mode, void *context)
    - {
     @@ builtin/ls-tree.c: int cmd_ls_tree(int argc, const char **argv, const char *prefix)
      	struct object_id oid;
      	struct tree *tree;
    @@ t/t3105-ls-tree-format.sh (new)
     +. ./test-lib.sh
     +
     +test_expect_success 'ls-tree --format usage' '
    -+	test_expect_code 129 git ls-tree --format=fmt -l &&
    -+	test_expect_code 129 git ls-tree --format=fmt --name-only &&
    -+	test_expect_code 129 git ls-tree --format=fmt --name-status &&
    -+	test_expect_code 129 git ls-tree --format=fmt --object-only
    ++	test_expect_code 129 git ls-tree --format=fmt -l HEAD &&
    ++	test_expect_code 129 git ls-tree --format=fmt --name-only HEAD &&
    ++	test_expect_code 129 git ls-tree --format=fmt --name-status HEAD &&
    ++	test_expect_code 129 git ls-tree --format=fmt --object-only HEAD
     +'
     +
     +test_expect_success 'setup' '
 5:  8dafb2b377 <  -:  ---------- cocci: allow padding with `strbuf_addf()`

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>

Remove code added in f35a6d3bce7 (Teach core object handling functions about gitlinks, 2007-04-09), later patched in 7d0b18a4da1 (Add output flushing before fork(), 2008-08-04), and then finally ending up in its current form in d3bee161fef (tree.c: allow read_tree_recursive() to traverse gitlink entries, 2009-01-25). All while being commented-out!

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/ls-tree.c | 9 ---------
 1 file changed, 9 deletions(-)

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>

Add missing {} to the "else" arms in show_tree() per the CodingGuidelines.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/ls-tree.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>

Change the ls-tree.c code to use type_name() on the enum instead of using the string constants. This doesn't matter either way for performance, but makes this a bit easier to read as we'll no longer need a strcmp() here.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/ls-tree.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>

The "struct strbuf"'s "len" member is a "size_t", not an "int", so let's change our corresponding types accordingly. This also changes the "len" and "speclen" variables, which are likewise used to store the return value of strlen(), which returns "size_t", not "int".

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/ls-tree.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

The variable which "show_tree()" return is named "retval", a name that's a little hard to understand. This commit tries to make the variable and the related codes more clear in the context.

The commit firstly rename "retval" to "recurse" which is a more meaningful name than before. Secondly, "get_type()" is introduced to setup the "type" by "mode", this will remove some of the nested if. After this, The codes here become a little bit clearer, so we do not need to take a look at "read_tree_at()" in "tree.c" to make sure the context of the return value.

Signed-off-by: Teng Long <dyronetengb@gmail.com>
---
 builtin/ls-tree.c | 33 ++++++++++++++++++---------------
 1 file changed, 18 insertions(+), 15 deletions(-)

On Thu, Jan 13 2022, Teng Long wrote:

Re the $subject: Is "optimize naming" here just referring to the s/retval/recurse/g?

Personally I think just a s/retval/ret/g here would make more senes if we're doing any change at all, and in either case having this variable re-rename split up as its own commit would make the proposed control flow changes clearer.

This new function is a re-invention of the object_type() utility in cache.h, and isn't needed. I.e....

...just drop it and do this:

	-       enum object_type type = get_type(mode);
	+       enum object_type type = object_type(mode);

On Thu, Jan 13, 2022 at 2:55 PM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:

Yes.

Do you mean that I can split the current one into two commits, one does the renaming work and another one does the left work?

If so, I will do this in the next patch.

You are absolutely correct. I will replace get_type() to object_type() in the next patch.

On Fri, Jan 14 2022, Teng Long wrote:

Yes, at least I would find it easier to read :)

Thanks!

We usually pipe the output from `git ls-trees` to tools like `sed` or `cut` when we only want to extract some fields.

When we want only the pathname component, we can pass `--name-only` option to omit such a pipeline, but there are no options for extracting other fields.

Teach the "--object-only" option to the command to only show the object name. This option cannot be used together with "--name-only" or "--long" , they are mutually exclusive (actually "--name-only" and "--long" can be combined together before, this commit by the way fix this bug).

A simple refactoring was done to the "show_tree" function, intead by using bitwise operations to recognize the format for printing to stdout. The reason for doing this is that we don't want to increase the readability difficulty with the addition of "-object-only", making this part of the logic easier to read and expand.

In terms of performance, there is no loss comparing to the "master" (2ae0a9cb8298185a94e5998086f380a355dd8907), here are the results of the performance tests in my environment based on linux repository:

    $hyperfine --warmup=10 "/opt/git/master/bin/git ls-tree -r HEAD"
    Benchmark 1: /opt/git/master/bin/git ls-tree -r HEAD
    Time (mean ± σ):     105.8 ms ±   2.7 ms    [User: 85.7 ms, System: 20.0 ms]
    Range (min … max):   101.5 ms … 111.3 ms    28 runs

    $hyperfine --warmup=10 "/opt/git/ls-tree-oid-only/bin/git ls-tree -r HEAD"
    Benchmark 1: /opt/git/ls-tree-oid-only/bin/git ls-tree -r HEAD
    Time (mean ± σ):     105.0 ms ±   3.0 ms    [User: 83.7 ms, System: 21.2 ms]
    Range (min … max):    99.3 ms … 109.5 ms    27 runs

    $hyperfine --warmup=10 "/opt/git/master/bin/git ls-tree -r -l HEAD"
    Benchmark 1: /opt/git/master/bin/git ls-tree -r -l HEAD
    Time (mean ± σ):     337.4 ms ±  10.9 ms    [User: 308.3 ms, System: 29.0 ms]
    Range (min … max):   323.0 ms … 355.0 ms    10 runs

    $hyperfine --warmup=10 "/opt/git/ls-tree-oid-only/bin/git ls-tree -r -l HEAD"
    Benchmark 1: /opt/git/ls-tree-oid-only/bin/git ls-tree -r -l HEAD
    Time (mean ± σ):     337.6 ms ±   6.2 ms    [User: 309.4 ms, System: 28.1 ms]
    Range (min … max):   330.4 ms … 349.9 ms    10 runs

Signed-off-by: Teng Long <dyroneteng@gmail.com>
---
 Documentation/git-ls-tree.txt |   7 +-
 builtin/ls-tree.c             | 141 +++++++++++++++++++++++++---------
 t/t3104-ls-tree-oid.sh        |  51 ++++++++++++
 3 files changed, 160 insertions(+), 39 deletions(-)
 create mode 100755 t/t3104-ls-tree-oid.sh

On Thu, Jan 13 2022, Teng Long wrote:

In the RFC series I sent this was first implemented in terms of the --format option, and I skipped the custom implementation you're adding here: https://lore.kernel.org/git/RFC-patch-7.7-5e34df4f8dd-20211217T131635Z-avarab@gmail.com/

I think in terms of patch series structure it would make sense to do that, and then have this custom --object-only implementation in terms of not-"--format " follow from that, and thus with the tests for the two (we'd add the tests you're adding here first, just for a --format="%(objectname)" or whatever) we'd see that the two are 1=1 equivalent in terms of functionality, but that this one is <X>% more optimized.

On Thu, Jan 13, 2022 at 3:02 PM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:

Sorry, the "not-"--format" means?

Please allow me to understand your advice, if we put the commit of introducing "--format" before the commit of introducing "--object-only", will be better because it's possible to supply more optimized performance (if we have) information in the commit message.

On Fri, Jan 14 2022, Teng Long wrote:

Sorry about not being clear. I mean there's two potential implementations. One that't is terms of --format='%(objectname)', and the other with your custom (faster) code to implement it.

Yes, you get the functionality you need with a simple alias of --format='%(objectname)' to --object-name (or whatever), so the only reason to carry the extra code is for optimization.

I wonder if the extra difference in performance is still something you care about, or if just the --format implementation would be OK.

But in any case, starting with a simpler implementation and testing it makes the progression easier to reason about.

On Fri, Jan 14, 2022 at 7:59 PM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:

Actually, at first, I wanted to achieve this in a simple way, as the "--object-only" implementation.

With the discussion in the community, I think both of them can achieve this purpose. "--object-only" is more intuitive, while "--format "is more flexible. For example, if the terminal supports automatic completion, the function of this option can be clearly known with typing TAB and lower costs of use and understanding. "--format" also works, but maybe have to check the help document to see if there are fields that support the same purpose.

Because the community had a different opinion about it. Junio, might prefer an "--object-only" approach, if I understand the context correctly.

So I have some inclination to support both. However, I can accept that only "--format" is supported.

So in the next patch, I hope to do some refactoring of the commit to support "--object-only" as the top commit. If in the end, we decide that "--format" is enough, we can discard the top "--object-only" commit.

I know you guys currently are busy on the new 2.35 release, so a later reply is OK.

Thanks.

On Tue, Jan 18 2022, Teng Long wrote:

I'm only talking about how it's implemented internally, not whether we have an --object-only option in the UI. I think it's good to have the option for completion etc.

I.e. in my RFC implementation of it here it's just a trivial wrapper around specifying a --format: https://lore.kernel.org/git/RFC-patch-7.7-5e34df4f8dd-20211217T131635Z-avarab@gmail.com/; Implementing it is 6 lines of trivial C code boilerplate.

But when you picked that up & ran with it you ended up carrying your original implementation: https://lore.kernel.org/git/e0274f079a7d381b9a936bfcd53bad64167c18b8.1641440700.git.dyroneteng@gmail.com/

I'm not saying we shouldn't have that, but that in any case a sequence of:

 1. Add a --format option
 2. Add a --object-only alias for a --format (what my RFC 7/7 does)
 3. Add a custom more optimized --object-only implementation

Would make the patch progression much easier to read, and we'd consider the correctness of --object-only (1 and 2) separate from the optimization question (3).

But maybe we won't need (3) at all in the end, i.e. is (1 and 2) fast enough for it not to matter (I think probably "yes", but I don't have a strong opinion on that).

*nod*, now that I read ahead I think you pretty much agree with that plan :)

Now would be a good time :)

I was reminded of this because Junio's proposed it for next at https://lore.kernel.org/git/xmqqr18jnr2t.fsf@gitster.g/

I think per the above & other replies of mine (including not matters of code arrangement opinion, but e.g. the doc formatting bug) we'll need at least one more re-roll of this. Thanks for sticking with this & working on this!

I'll indicate that in a reply to that "What's Cooking" report.

On Fri, Feb 4, 2022 at 9:04 PM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:

Sorry for the late reply, I had a vacation in the last two weeks (Chinese New Year).

I have to say it's a very valuable recommendation and at the same time I recognise that spending more time on organizing commits ahead is important and make small steps(or commits) sufficiently.

Thanks for mentioning that. I will back work on it this week.

Thanks.

"show_tree_data" is a struct that packages the necessary fields for "show_tree()". This commit is a pre-prepared commit for supporting "--format" option and it does not affect any existing functionality.

Signed-off-by: Teng Long <dyroneteng@gmail.com>
---
 builtin/ls-tree.c | 44 +++++++++++++++++++++++++++++---------------
 1 file changed, 29 insertions(+), 15 deletions(-)

On Thu, Jan 13 2022, Teng Long wrote:

Is the only reason this is split off from 9/9 because you're injecting a 8/9 commit for the coccinelle rule change, and wanted to find some logical cut-off between the two?

For both this & 9/9 this seems to mostly/substantially be code I wrote and submitted as part of https://lore.kernel.org/git/RFC-patch-6.7-eac299f06ff-20211217T131635Z-avarab@gmail.com;

The convention we use in such cases is to retain the "Author" header and just add your own Signed-off-by to patches you're modifying/splitting up.

On Thu, Jan 13, 2022 at 3:07 PM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:

I hope "show_tree()" and "the show_tree_format()" to share this structure, so I made this a pre-prepared and non-functional commit.

After that, in the 9/9, the structure can be used directly and focus on the functionality changes. If we merge this commit with 9/9, 9/9 will contain a part of the changes that let "show_tree()" use the new structure, which has nothing to do with "show_tree_format()" actually, because we designed them to go through different execution logic. So, personally, I prefer not to mix them together.

So, the commit of "show_tree_data()" originally was not for "coccinelle". The only thing that is certain is that coccinelle also should go before 9/9 I think. With regard to 8/9 and 7/9, I think the current order is OK because they're not related.

Oops. Sorry for that, I misunderstood it before and I'll be fixed in the next path.

Thanks.

A convenient way to pad strings is to use something like `strbuf_addf(&buf, "%20s", "Hello, world!")`.

However, the Coccinelle rule that forbids a format `"%s"` with a constant string argument cast too wide a net, and also forbade such padding.

The original rule was introduced by commit:

    28c23cd4c39 (strbuf.cocci: suggest strbuf_addbuf() to add one strbuf to an other, 2019-01-25)

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Signed-off-by: Teng Long <dyroneteng@gmail.com>
---
 contrib/coccinelle/strbuf.cocci | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

Add a --format option to ls-tree. It has an existing default output, and then --long and --name-only options to emit the default output along with the objectsize and, or to only emit object paths.

Rather than add --type-only, --object-only etc. we can just support a --format using a strbuf_expand() similar to "for-each-ref --format". We might still add such options in the future for convenience.

The --format implementation is slower than the existing code, but this change does not cause any performance regressions. We'll leave the existing show_tree() unchanged, and only run show_tree_fmt() in if a --format different than the hardcoded built-in ones corresponding to the existing modes is provided.

I.e. something like the "--long" output would be much slower with this, mainly due to how we need to allocate various things to do with quote.c instead of spewing the output directly to stdout.

The new option of '--format' comes from Ævar Arnfjörð Bjarmasonn's idea and suggestion, this commit makes modifications in terms of the original discussion on community [1].

Here is the statistics about performance tests:

1. Default format (hitten the builtin formats):

    "git ls-tree <tree-ish>" vs "--format='%(mode) %(type) %(object)%x09%(file)'"

    $hyperfine --warmup=10 "/opt/git/master/bin/git ls-tree -r HEAD"
    Benchmark 1: /opt/git/master/bin/git ls-tree -r HEAD
    Time (mean ± σ):     105.2 ms ±   3.3 ms    [User: 84.3 ms, System: 20.8 ms]
    Range (min … max):    99.2 ms … 113.2 ms    28 runs

    $hyperfine --warmup=10 "/opt/git/ls-tree-oid-only/bin/git ls-tree -r --format='%(mode) %(type) %(object)%x09%(file)'  HEAD"
    Benchmark 1: /opt/git/ls-tree-oid-only/bin/git ls-tree -r --format='%(mode) %(type) %(object)%x09%(file)'  HEAD
    Time (mean ± σ):     106.4 ms ±   2.7 ms    [User: 86.1 ms, System: 20.2 ms]
    Range (min … max):   100.2 ms … 110.5 ms    29 runs

2. Default format includes object size (hitten the builtin formats):

    "git ls-tree -l <tree-ish>" vs "--format='%(mode) %(type) %(object) %(size:padded)%x09%(file)'"

    $hyperfine --warmup=10 "/opt/git/master/bin/git ls-tree -r -l HEAD"
    Benchmark 1: /opt/git/master/bin/git ls-tree -r -l HEAD
    Time (mean ± σ):     335.1 ms ±   6.5 ms    [User: 304.6 ms, System: 30.4 ms]
    Range (min … max):   327.5 ms … 348.4 ms    10 runs

    $hyperfine --warmup=10 "/opt/git/ls-tree-oid-only/bin/git ls-tree -r --format='%(mode) %(type) %(object) %(size:padded)%x09%(file)'  HEAD"
    Benchmark 1: /opt/git/ls-tree-oid-only/bin/git ls-tree -r --format='%(mode) %(type) %(object) %(size:padded)%x09%(file)'  HEAD
    Time (mean ± σ):     337.2 ms ±   8.2 ms    [User: 309.2 ms, System: 27.9 ms]
    Range (min … max):   328.8 ms … 349.4 ms    10 runs

Links:
	[1] https://public-inbox.org/git/RFC-patch-6.7-eac299f06ff-20211217T131635Z-avarab@gmail.com/

Signed-off-by: Teng Long <dyroneteng@gmail.com>
---
 Documentation/git-ls-tree.txt |  51 +++++++++++++-
 builtin/ls-tree.c             | 129 +++++++++++++++++++++++++++++++++-
 t/t3105-ls-tree-format.sh     |  55 +++++++++++++++
 3 files changed, 230 insertions(+), 5 deletions(-)
 create mode 100755 t/t3105-ls-tree-format.sh

On Thu, Jan 13 2022, Teng Long wrote:

Let's split up this re-flow only change into its own commit? I.e. the only non-whitespace change here is beginning with [--format].

If it was the right thing to do to re-flow this then we didn't need [--format=<format>] to exist to do so...

Removing this \n breaks the formatting in the file. See "make man && man ./Documentation/git-ls-tree.1". The ./Documentation/doc-diff utility is also handy for sanity checking the documentation formatting.

These new docs make sense & seem to cover all the basis, thanks!

Here because we've added --format discussing the previous pseudo-format as a "default" format becomes confusing. Let's instead say:

        The output format of `ls-tree` is determined by either the `--format` option,
        or other format-altering options such as `--name-long` etc. (see `--format` above).

        The use of certain `--format` directives is equivalent to using those options,
        but invoking the full formatting machinery can be slower than using an appropriate
        formatting option.

        In cases where the `--format` would exactly map to an existing option `ls-tree` will
        use the appropriate faster path. Thus the default format is equivalent to:
        ---
        %(mode) %(type) %(object)%x09%(file)
        ---

Or something like that. We could then discuss e.g. --name-long being `%(mode) %(type) %(object) %(size:padded)%x09%(file)` when we discuss that option.

This one-liner is guaranteed to result in invalid JSON on some repositories, both because JSON is inherently a bad fit for git's data model (JSON needs to be in one Unicode encoding, Git's tree data might me in a mixture of encodings), and because it'll break if the file includes a '"'.

I think it's better to just replace this with some example involving -z, or at least prominently note that this is broken in the general case, but can be used ad-hoc to quickly check things with "jq" or whatever.

In https://lore.kernel.org/git/cover.1641043500.git.dyroneteng@gmail.com/ you noted that you changed the field names of e.g. "objectname" to "object" etc. You're right that I picked these as-is from the git-for-each-ref formatting.

1/3 of your reasoning for doing so was to make it consistent with the documentation examples of e.g.:

     <mode> SP <type> SP <object> TAB <file>

I think in any case (as noted above) we should change those to use the --format), so that leaves just:

 - "I prefer to make the name more simple to memorize and type"
 - "I think the names with "object" prefix are [from git-for-each-ref
   and the object* prefixes aren't redundant there, but would be here]".

I think both of those still apply, but I think having these consistent with git-for-each-ref outweighs the slight benefit of shorter names.

Right now only a handful of things support these sort of --format directives, but we've already got RFC/WIP patches to add that to git-cat-file, and are likely to add more in the future.

I'd also like us to eventually be able to combine what are now separate built-ins with their own --format to expose more deeply some internal APIs via IPC. E.g. now you can do this:

    git for-each-ref --format='%(refname) %(tree)'

But to list each of those trees you'd need to pipe that output into this new 'git ls-tree --format. But imagine being able to do something like:

    git for-each-ref --format='%(refname) %(git-ls-tree --format %%(objectname) %(tree))'

Where we'd just invoke git-ls-tree for you without running a full sub-process. I think both for that hypothetical and working with the two --formats now having to use %(type) in some places but %(objecttype) etc. in others is just needlessly confusing. Let's just consistently use the same format names everywhere.

Specifically for your s/path/file/ name change, that's just inaccurate, consider:

    $ ./git ls-tree --format="%(mode) %(type) %(file)" -t HEAD -- t/README
    040000 tree t
    100644 blob t/README

And:

    $ $ (cd t && ../git ls-tree --format="%(mode) %(type) %(file)" -t -r HEAD -- README)
    040000 tree ./
    100644 blob README

I.e. we talk about <path> in the existing SYNOPSIS for a reason. That we had a "<file>" in the existing format demo was a bug/shorthand that we shouldn't be propagating further.

One advantage of keeping the variable names I picked in https://lore.kernel.org/git/RFC-patch-6.7-eac299f06ff-20211217T131635Z-avarab@gmail.com/ is that they align, so you can instantly see that the first two are equivalent until the "%x09".

It also makes it easier to review to avoid such churn, to see what you really changed I'm looking at a local version of a range-diff where I renamed these, the struct you renamed etc. back just to see what you /really/ changed. I.e. what are functional v.s. renaming changes.

Here you changed my '"%"PRIuMAX' to '"%" PRIuMAX'. The former is the prevailing style in this codebase, and avoiding the formatting churn makes the inter-diff easier to read.

Ditto some harder to review interdiff due to renaming churn. I.e. s/line/sb/ in both this and expand_show_tree(). I really wouldn't care at all except because of all the manual work in reviewing the inter-diff between my original version & this derived version.

In the case of "line" that's not even an improvement. With a --format we're not building a "line", the user is free to insert any arbitrary directives including \n's, so we might be working on multiple lines.

This & several other changes v.s. my version are good, e.g. here I seem to have repeated the logic error I noted for your version (i.e omitting "HEAD"), oops!

As I noted in my RFC CL (https://lore.kernel.org/git/RFC-cover-0.7-00000000000-20211217T131635Z-avarab@gmail.com/):

	"the tests for ls-tree are really
	lacking. E.g. I seem to have a rather obvious bug in how -t and the
	--format interact here, but no test catches it."

So first, in my version of adding --format I was careful to make --name-only etc. imply a given --format, and then only at the last minute would we take the "fast path": https://lore.kernel.org/git/RFC-patch-6.7-eac299f06ff-20211217T131635Z-avarab@gmail.com/

You rewrote that in https://lore.kernel.org/git/e0add802fbbabde7e7b3743127b2d4047f1ce760.1641043500.git.dyroneteng@gmail.com/ and qremoved the limited "GIT_TEST_LS_TREE_FORMAT_BACKEND" testing I added, so now the internal --format machinery can't be run through the existing tests we do have.

Even with that re-added I really wouldn't trust that this code is doing the right thing (and as noted, I don't trust my own RFC version either). I think e.g. our "coverage" Makefile targets would be a good start as a first approximation, i.e. running the /ls-tree/ tests and seeing if we have full coverage.

As I noted in 7/9 I think this patch is 9/9 still mostly something I wrote, so that the "author" and Signed-off-by should be preserved. The below is a range-diff of an amended version I've been looking at in trying to review this. It undoes several (but not all) of your formatting/renaming-only changes, just so that I could see what the non-formatting changes were:

1:  6c96dff15c5 ! 1:  917bb168d45 ls-tree: add a --format=<fmt> option
    @@
      ## Metadata ##
    -Author: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
    +Author: Teng Long <dyroneteng@gmail.com>
     
      ## Commit message ##
    -    ls-tree: add a --format=<fmt> option
    +    ls-tree.c: introduce "--format" option
     
         Add a --format option to ls-tree. It has an existing default output,
         and then --long and --name-only options to emit the default output
    @@ Commit message
     
         The --format implementation is slower than the existing code, but this
         change does not cause any performance regressions. We'll leave the
    -    existing show_tree() unchanged, and only run show_tree_format() in if
    +    existing show_tree() unchanged, and only run show_tree_fmt() in if
         a --format different than the hardcoded built-in ones corresponding to
         the existing modes is provided.
     
    -    "Slower" here can bee seen via the the following "hyperfine"
    -    command. This uses GIT_TEST_LS_TREE_FORMAT_BACKEND=<bool> to force the
    -    use of the new backend:
    -
    -        $ hyperfine -L env false,true -L f "-r,-r -l,-r --name-only,-r --format='%(objectname)'" 'GIT_TEST_LS_TREE_FORMAT_BACKEND={env} ./git -C ~/g/linux ls-tree {f} HEAD' -r 10
    -        Benchmark 1: GIT_TEST_LS_TREE_FORMAT_BACKEND=false ./git -C ~/g/linux ls-tree -r HEAD
    -          Time (mean ± σ):      86.1 ms ±   0.6 ms    [User: 65.2 ms, System: 20.9 ms]
    -          Range (min … max):    85.2 ms …  87.5 ms    10 runs
    +    I.e. something like the "--long" output would be much slower with
    +    this, mainly due to how we need to allocate various things to do with
    +    quote.c instead of spewing the output directly to stdout.
     
    -        Benchmark 2: GIT_TEST_LS_TREE_FORMAT_BACKEND=true ./git -C ~/g/linux ls-tree -r HEAD
    -          Time (mean ± σ):     122.5 ms ±   0.6 ms    [User: 101.3 ms, System: 21.1 ms]
    -          Range (min … max):   121.8 ms … 123.4 ms    10 runs
    +    The new option of '--format' comes from Ævar Arnfjörð Bjarmasonn's
    +    idea and suggestion, this commit makes modifications in terms of the
    +    original discussion on community [1].
     
    -        Benchmark 3: GIT_TEST_LS_TREE_FORMAT_BACKEND=false ./git -C ~/g/linux ls-tree -r -l HEAD
    -          Time (mean ± σ):     277.7 ms ±   1.3 ms    [User: 234.6 ms, System: 43.0 ms]
    -          Range (min … max):   275.9 ms … 279.7 ms    10 runs
    +    Here is the statistics about performance tests:
     
    -        Benchmark 4: GIT_TEST_LS_TREE_FORMAT_BACKEND=true ./git -C ~/g/linux ls-tree -r -l HEAD
    -          Time (mean ± σ):     332.8 ms ±   2.6 ms    [User: 282.0 ms, System: 50.7 ms]
    -          Range (min … max):   329.6 ms … 338.2 ms    10 runs
    +    1. Default format (hitten the builtin formats):
     
    -        Benchmark 5: GIT_TEST_LS_TREE_FORMAT_BACKEND=false ./git -C ~/g/linux ls-tree -r --name-only HEAD
    -          Time (mean ± σ):      71.8 ms ±   0.4 ms    [User: 54.1 ms, System: 17.6 ms]
    -          Range (min … max):    71.2 ms …  72.5 ms    10 runs
    +        "git ls-tree <tree-ish>" vs "--format='%(mode) %(type) %(object)%x09%(file)'"
     
    -        Benchmark 6: GIT_TEST_LS_TREE_FORMAT_BACKEND=true ./git -C ~/g/linux ls-tree -r --name-only HEAD
    -          Time (mean ± σ):      86.6 ms ±   0.5 ms    [User: 65.7 ms, System: 20.7 ms]
    -          Range (min … max):    85.9 ms …  87.4 ms    10 runs
    +        $hyperfine --warmup=10 "/opt/git/master/bin/git ls-tree -r HEAD"
    +        Benchmark 1: /opt/git/master/bin/git ls-tree -r HEAD
    +        Time (mean ± σ):     105.2 ms ±   3.3 ms    [User: 84.3 ms, System: 20.8 ms]
    +        Range (min … max):    99.2 ms … 113.2 ms    28 runs
     
    -        Benchmark 7: GIT_TEST_LS_TREE_FORMAT_BACKEND=false ./git -C ~/g/linux ls-tree -r --format='%(objectname)' HEAD
    -          Time (mean ± σ):      85.8 ms ±   0.6 ms    [User: 66.2 ms, System: 19.5 ms]
    -          Range (min … max):    85.0 ms …  86.9 ms    10 runs
    +        $hyperfine --warmup=10 "/opt/git/ls-tree-oid-only/bin/git ls-tree -r --format='%(mode) %(type) %(object)%x09%(file)'  HEAD"
    +        Benchmark 1: /opt/git/ls-tree-oid-only/bin/git ls-tree -r --format='%(mode) %(type) %(object)%x09%(file)'  HEAD
    +        Time (mean ± σ):     106.4 ms ±   2.7 ms    [User: 86.1 ms, System: 20.2 ms]
    +        Range (min … max):   100.2 ms … 110.5 ms    29 runs
     
    -        Benchmark 8: GIT_TEST_LS_TREE_FORMAT_BACKEND=true ./git -C ~/g/linux ls-tree -r --format='%(objectname)' HEAD
    -          Time (mean ± σ):      85.3 ms ±   0.2 ms    [User: 66.6 ms, System: 18.7 ms]
    -          Range (min … max):    85.0 ms …  85.7 ms    10 runs
    +    2. Default format includes object size (hitten the builtin formats):
     
    -        Summary
    -          'GIT_TEST_LS_TREE_FORMAT_BACKEND=false ./git -C ~/g/linux ls-tree -r --name-only HEAD' ran
    -            1.19 ± 0.01 times faster than 'GIT_TEST_LS_TREE_FORMAT_BACKEND=true ./git -C ~/g/linux ls-tree -r --format='%(objectname)' HEAD'
    -            1.19 ± 0.01 times faster than 'GIT_TEST_LS_TREE_FORMAT_BACKEND=false ./git -C ~/g/linux ls-tree -r --format='%(objectname)' HEAD'
    -            1.20 ± 0.01 times faster than 'GIT_TEST_LS_TREE_FORMAT_BACKEND=false ./git -C ~/g/linux ls-tree -r HEAD'
    -            1.21 ± 0.01 times faster than 'GIT_TEST_LS_TREE_FORMAT_BACKEND=true ./git -C ~/g/linux ls-tree -r --name-only HEAD'
    -            1.71 ± 0.01 times faster than 'GIT_TEST_LS_TREE_FORMAT_BACKEND=true ./git -C ~/g/linux ls-tree -r HEAD'
    -            3.87 ± 0.03 times faster than 'GIT_TEST_LS_TREE_FORMAT_BACKEND=false ./git -C ~/g/linux ls-tree -r -l HEAD'
    -            4.64 ± 0.05 times faster than 'GIT_TEST_LS_TREE_FORMAT_BACKEND=true ./git -C ~/g/linux ls-tree -r -l HEAD'
    +        "git ls-tree -l <tree-ish>" vs "--format='%(mode) %(type) %(object) %(size:padded)%x09%(file)'"
     
    -    I.e. something like the "--long" output would be much slower with
    -    this, mainly due to how we need to allocate various things to do with
    -    quote.c instead of spewing the output directly to stdout.
    +        $hyperfine --warmup=10 "/opt/git/master/bin/git ls-tree -r -l HEAD"
    +        Benchmark 1: /opt/git/master/bin/git ls-tree -r -l HEAD
    +        Time (mean ± σ):     335.1 ms ±   6.5 ms    [User: 304.6 ms, System: 30.4 ms]
    +        Range (min … max):   327.5 ms … 348.4 ms    10 runs
     
    -    But even a --format='%(objectname)' is fast with the new backend, so
    -    this is viable as a replacement for adding new formats, and we'll pay
    -    for this added complexity as a one-off, and not again every time a new
    -    format needs to be added. See [1] for an example of what it would
    -    otherwise take to add an --object-name flag.
    +        $hyperfine --warmup=10 "/opt/git/ls-tree-oid-only/bin/git ls-tree -r --format='%(mode) %(type) %(object) %(size:padded)%x09%(file)'  HEAD"
    +        Benchmark 1: /opt/git/ls-tree-oid-only/bin/git ls-tree -r --format='%(mode) %(type) %(object) %(size:padded)%x09%(file)'  HEAD
    +        Time (mean ± σ):     337.2 ms ±   8.2 ms    [User: 309.2 ms, System: 27.9 ms]
    +        Range (min … max):   328.8 ms … 349.4 ms    10 runs
     
    -    1. https://lore.kernel.org/git/2e449d1c792ff81da5f22c8bf65ed33c393d62f8.1639721750.git.dyroneteng@gmail.com/
    +    Links:
    +            [1] https://public-inbox.org/git/RFC-patch-6.7-eac299f06ff-20211217T131635Z-avarab@gmail.com/
     
    -    Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
    +    Signed-off-by: Teng Long <dyroneteng@gmail.com>
     
    - ## builtin/ls-tree.c ##
    -@@ builtin/ls-tree.c: static struct pathspec pathspec;
    - static int chomp_prefix;
    - static const char *ls_tree_prefix;
    + ## Documentation/git-ls-tree.txt ##
    +@@ Documentation/git-ls-tree.txt: SYNOPSIS
    + --------
    + [verse]
    + 'git ls-tree' [-d] [-r] [-t] [-l] [-z]
    +-	    [--name-only] [--name-status] [--object-only] [--full-name] [--full-tree] [--abbrev[=<n>]]
    +-	    <tree-ish> [<path>...]
    ++	    [--name-only] [--name-status] [--object-only]
    ++	    [--full-name] [--full-tree] [--abbrev[=<n>]]
    ++	    [--format=<format>] <tree-ish> [<path>...]
      
    -+/*
    -+ * The format equivalents that show_tree() is prepared to handle.
    -+ */
    -+static const char *ls_tree_format_d = "%(objectmode) %(objecttype) %(objectname)%x09%(path)";
    -+static const char *ls_tree_format_l = "%(objectmode) %(objecttype) %(objectname) %(objectsize:padded)%x09%(path)";
    -+static const char *ls_tree_format_n = "%(path)";
    + DESCRIPTION
    + -----------
    +@@ Documentation/git-ls-tree.txt: OPTIONS
    + 	Do not limit the listing to the current working directory.
    + 	Implies --full-name.
    + 
    ++--format=<format>::
    ++	A string that interpolates `%(fieldname)` from the result
    ++	being shown. It also interpolates `%%` to `%`, and
    ++	`%xx` where `xx` are hex digits interpolates to character
    ++	with hex code `xx`; for example `%00` interpolates to
    ++	`\0` (NUL), `%09` to `\t` (TAB) and `%0a` to `\n` (LF).
    ++	When specified, `--format` cannot be combined with other
    ++	format-altering options, including `--long`, `--name-only`
    ++	and `--object-only`.
     +
    - static const  char * const ls_tree_usage[] = {
    - 	N_("git ls-tree [<options>] <tree-ish> [<path>...]"),
    - 	NULL
    - };
    + [<path>...]::
    + 	When paths are given, show them (note that this isn't really raw
    + 	pathnames, but rather a list of patterns to match).  Otherwise
    +@@ Documentation/git-ls-tree.txt: OPTIONS
      
    -+struct read_tree_ls_tree_data {
    -+	const char *format;
    -+	struct strbuf sb_scratch;
    -+	struct strbuf sb_tmp;
    -+};
    + Output Format
    + -------------
    ++
    ++Default format:
    ++
    +         <mode> SP <type> SP <object> TAB <file>
    + 
    + This output format is compatible with what `--index-info --stdin` of
    +@@ Documentation/git-ls-tree.txt: quoted as explained for the configuration variable `core.quotePath`
    + (see linkgit:git-config[1]).  Using `-z` the filename is output
    + verbatim and the line is terminated by a NUL byte.
    + 
    ++Customized format:
     +
    ++It is possible to print in a custom format by using the `--format` option,
    ++which is able to interpolate different fields using a `%(fieldname)` notation.
    ++For example, if you want to only print the <object> and <file> fields with a
    ++JSON style, executing with a specific "--format" like
    ++
    ++        git ls-tree --format='{"object":"%(object)", "file":"%(file)"}' <tree-ish>
    ++
    ++The output format changes to:
    ++
    ++        {"object":"<object>", "file":"<file>"}
    ++
    ++FIELD NAMES
    ++-----------
    ++
    ++Various values from structured fields can be used to interpolate
    ++into the resulting output. For each outputing line, the following
    ++names can be used:
    ++
    ++mode::
    ++	The mode of the object.
    ++type::
    ++	The type of the object (`blob` or `tree`).
    ++object::
    ++	The name of the object.
    ++size[:padded]::
    ++	The size of the object ("-" if it's a tree).
    ++	It also supports a padded format of size with "%(size:padded)".
    ++file::
    ++	The filename of the object.
    ++
    + GIT
    + ---
    + Part of the linkgit:git[1] suite
    +
    + ## builtin/ls-tree.c ##
    +@@ builtin/ls-tree.c: static unsigned int shown_fields;
    + #define FIELD_DEFAULT 29 /* 11101 size is not shown to output by default */
    + #define FIELD_LONG_DEFAULT  (FIELD_DEFAULT | FIELD_SIZE)
    + 
     +struct expand_ls_tree_data {
     +	unsigned mode;
     +	enum object_type type;
     +	const struct object_id *oid;
     +	const char *pathname;
    -+	const char *basebuf;
    -+	struct strbuf *sb_scratch;
    -+	struct strbuf *sb_tmp;
    ++	struct strbuf *base;
     +};
     +
    - static int show_recursive(const char *base, size_t baselen, const char *pathname)
    + static const  char * const ls_tree_usage[] = {
    + 	N_("git ls-tree [<options>] <tree-ish> [<path>...]"),
    + 	NULL
    +@@ builtin/ls-tree.c: enum {
    + 
    + static int cmdmode = MODE_UNSPECIFIED;
    + 
    ++static const char *format;
    ++static const char *ls_tree_format_d = "%(objectmode) %(objecttype) %(objectname)%x09%(path)";
    ++static const char *ls_tree_format_l = "%(objectmode) %(objecttype) %(objectname) %(objectsize:padded)%x09%(path)";
    ++static const char *ls_tree_format_n = "%(path)";
    ++static const char *ls_tree_format_o = "%(objectname)";
    ++
    + static int parse_shown_fields(void)
      {
    - 	int i;
    -@@ builtin/ls-tree.c: static int show_recursive(const char *base, size_t baselen, const char *pathname
    - 	return 0;
    + 	if (cmdmode == MODE_NAME_ONLY) {
    +@@ builtin/ls-tree.c: static int parse_shown_fields(void)
    + 	return 1;
      }
      
    -+static void expand_objectsize(struct strbuf *sb,
    -+			      const struct object_id *oid,
    -+			      const enum object_type type,
    -+			      unsigned int padded)
    ++static void expand_objectsize(struct strbuf *sb, const struct object_id *oid,
    ++			      const enum object_type type, unsigned int padded)
     +{
     +	if (type == OBJ_BLOB) {
     +		unsigned long size;
     +		if (oid_object_info(the_repository, oid, &size) < 0)
    -+			die(_("could not get object info about '%s'"), oid_to_hex(oid));
    ++			die(_("could not get object info about '%s'"),
    ++			    oid_to_hex(oid));
     +		if (padded)
     +			strbuf_addf(sb, "%7"PRIuMAX, (uintmax_t)size);
     +		else
    @@ builtin/ls-tree.c: static int show_recursive(const char *base, size_t baselen, c
     +	}
     +}
     +
    -+static size_t expand_show_tree(struct strbuf *sb,
    -+			       const char *start,
    ++static size_t expand_show_tree(struct strbuf *sb, const char *start,
     +			       void *context)
     +{
     +	struct expand_ls_tree_data *data = context;
     +	const char *end;
     +	const char *p;
    -+	size_t len;
    ++	unsigned int errlen;
    ++	size_t len = strbuf_expand_literal_cb(sb, start, NULL);
     +
    -+	len = strbuf_expand_literal_cb(sb, start, NULL);
     +	if (len)
     +		return len;
    -+
     +	if (*start != '(')
    -+		die(_("bad format as of '%s'"), start);
    ++		die(_("bad ls-tree format: as '%s'"), start);
    ++
     +	end = strchr(start + 1, ')');
     +	if (!end)
    -+		die(_("ls-tree format element '%s' does not end in ')'"),
    -+		    start);
    -+	len = end - start + 1;
    ++		die(_("bad ls-tree format: element '%s' does not end in ')'"), start);
     +
    ++	len = end - start + 1;
     +	if (skip_prefix(start, "(objectmode)", &p)) {
     +		strbuf_addf(sb, "%06o", data->mode);
     +	} else if (skip_prefix(start, "(objecttype)", &p)) {
    @@ builtin/ls-tree.c: static int show_recursive(const char *base, size_t baselen, c
     +	} else if (skip_prefix(start, "(objectsize)", &p)) {
     +		expand_objectsize(sb, data->oid, data->type, 0);
     +	} else if (skip_prefix(start, "(objectname)", &p)) {
    -+		strbuf_addstr(sb, find_unique_abbrev(data->oid, abbrev));
    ++		strbuf_add_unique_abbrev(sb, data->oid, abbrev);
     +	} else if (skip_prefix(start, "(path)", &p)) {
    -+		const char *name = data->basebuf;
    ++		const char *name = data->base->buf;
     +		const char *prefix = chomp_prefix ? ls_tree_prefix : NULL;
    -+
    -+		if (prefix)
    -+			name = relative_path(name, prefix, data->sb_scratch);
    -+		quote_c_style(name, data->sb_tmp, NULL, 0);
    -+		strbuf_add(sb, data->sb_tmp->buf, data->sb_tmp->len);
    -+
    -+		strbuf_reset(data->sb_tmp);
    -+		/* The relative_path() function resets "scratch" */
    ++		struct strbuf quoted = STRBUF_INIT;
    ++		struct strbuf s = STRBUF_INIT;
    ++		strbuf_addstr(data->base, data->pathname);
    ++		name = relative_path(data->base->buf, prefix, &s);
    ++		quote_c_style(name, &quoted, NULL, 0);
    ++		strbuf_addbuf(sb, &quoted);
    ++		strbuf_release(&s);
    ++		strbuf_release(&quoted);
     +	} else {
    -+		unsigned int errlen = (unsigned long)len;
    -+		die(_("bad ls-tree format specifiec %%%.*s"), errlen, start);
    ++		errlen = (unsigned long)len;
    ++		die(_("bad ls-tree format: %%%.*s"), errlen, start);
     +	}
    -+
     +	return len;
     +}
     +
    - static int show_tree_init(enum object_type *type, struct strbuf *base,
    - 			  const char *pathname, unsigned mode, int *retval)
    + static int show_recursive(const char *base, size_t baselen,
    + 			  const char *pathname)
      {
    -@@ builtin/ls-tree.c: static int show_tree_init(enum object_type *type, struct strbuf *base,
    +@@ builtin/ls-tree.c: static int show_recursive(const char *base, size_t baselen,
      	return 0;
      }
      
    +-static int show_default(const struct object_id *oid, enum object_type type,
    +-			const char *pathname, unsigned mode,
    +-			struct strbuf *base)
     +static int show_tree_fmt(const struct object_id *oid, struct strbuf *base,
     +			 const char *pathname, unsigned mode, void *context)
    -+{
    -+	struct read_tree_ls_tree_data *data = context;
    -+	struct expand_ls_tree_data my_data = {
    + {
    +-	size_t baselen = base->len;
    ++	size_t baselen;
    ++	int recurse = 0;
    ++	struct strbuf line = STRBUF_INIT;
    ++	enum object_type type = object_type(mode);
    ++
    ++	struct expand_ls_tree_data data = {
     +		.mode = mode,
    -+		.type = OBJ_BLOB,
    ++		.type = type,
     +		.oid = oid,
     +		.pathname = pathname,
    -+		.sb_scratch = &data->sb_scratch,
    -+		.sb_tmp = &data->sb_tmp,
    ++		.base = base,
     +	};
    -+	struct strbuf sb = STRBUF_INIT;
    -+	int retval = 0;
    -+	size_t baselen;
     +
    -+	if (show_tree_init(&my_data.type, base, pathname, mode, &retval))
    -+		return retval;
    ++	if (type == OBJ_TREE && show_recursive(base->buf, base->len, pathname))
    ++		recurse = READ_TREE_RECURSIVE;
    ++	if (type == OBJ_TREE && recurse && !(ls_options & LS_SHOW_TREES))
    ++		return recurse;
    ++	if (type == OBJ_BLOB && (ls_options & LS_TREE_ONLY))
    ++		return 0;
     +
     +	baselen = base->len;
    -+	strbuf_addstr(base, pathname);
    -+	strbuf_reset(&sb);
    -+	my_data.basebuf = base->buf;
    -+
    -+	strbuf_expand(&sb, data->format, expand_show_tree, &my_data);
    -+	strbuf_addch(&sb, line_termination);
    -+	fwrite(sb.buf, sb.len, 1, stdout);
    ++	strbuf_expand(&line, format, expand_show_tree, &data);
    ++	strbuf_addch(&line, line_termination);
    ++	fwrite(line.buf, line.len, 1, stdout);
    ++	strbuf_release(&line);
     +	strbuf_setlen(base, baselen);
    -+
    -+	return retval;
    ++	return recurse;
     +}
     +
    - static int show_tree(const struct object_id *oid, struct strbuf *base,
    - 		const char *pathname, unsigned mode, void *context)
    - {
    ++static int show_default(struct expand_ls_tree_data *data)
    ++{
    ++	size_t baselen = data->base->len;
    + 
    + 	if (shown_fields & FIELD_SIZE) {
    + 		char size_text[24];
    +-		if (type == OBJ_BLOB) {
    ++		if (data->type == OBJ_BLOB) {
    + 			unsigned long size;
    +-			if (oid_object_info(the_repository, oid, &size) == OBJ_BAD)
    ++			if (oid_object_info(the_repository, data->oid, &size) == OBJ_BAD)
    + 				xsnprintf(size_text, sizeof(size_text), "BAD");
    + 			else
    + 				xsnprintf(size_text, sizeof(size_text),
    +@@ builtin/ls-tree.c: static int show_default(const struct object_id *oid, enum object_type type,
    + 		} else {
    + 			xsnprintf(size_text, sizeof(size_text), "-");
    + 		}
    +-		printf("%06o %s %s %7s\t", mode, type_name(type),
    +-		find_unique_abbrev(oid, abbrev), size_text);
    ++		printf("%06o %s %s %7s\t", data->mode, type_name(data->type),
    ++		find_unique_abbrev(data->oid, abbrev), size_text);
    + 	} else {
    +-		printf("%06o %s %s\t", mode, type_name(type),
    +-		find_unique_abbrev(oid, abbrev));
    ++		printf("%06o %s %s\t", data->mode, type_name(data->type),
    ++		find_unique_abbrev(data->oid, abbrev));
    + 	}
    +-	baselen = base->len;
    +-	strbuf_addstr(base, pathname);
    +-	write_name_quoted_relative(base->buf,
    ++	baselen = data->base->len;
    ++	strbuf_addstr(data->base, data->pathname);
    ++	write_name_quoted_relative(data->base->buf,
    + 				   chomp_prefix ? ls_tree_prefix : NULL, stdout,
    + 				   line_termination);
    +-	strbuf_setlen(base, baselen);
    ++	strbuf_setlen(data->base, baselen);
    + 	return 1;
    + }
    + 
    +@@ builtin/ls-tree.c: static int show_tree(const struct object_id *oid, struct strbuf *base,
    + 	size_t baselen;
    + 	enum object_type type = object_type(mode);
    + 
    ++	struct expand_ls_tree_data data = {
    ++		.mode = mode,
    ++		.type = type,
    ++		.oid = oid,
    ++		.pathname = pathname,
    ++		.base = base,
    ++	};
    ++
    + 	if (type == OBJ_TREE && show_recursive(base->buf, base->len, pathname))
    + 		recurse = READ_TREE_RECURSIVE;
    + 	if (type == OBJ_TREE && recurse && !(ls_options & LS_SHOW_TREES))
    +@@ builtin/ls-tree.c: static int show_tree(const struct object_id *oid, struct strbuf *base,
    + 	}
    + 
    + 	if (shown_fields >= FIELD_DEFAULT)
    +-		show_default(oid, type, pathname, mode, base);
    ++		show_default(&data);
    + 
    + 	return recurse;
    + }
     @@ builtin/ls-tree.c: int cmd_ls_tree(int argc, const char **argv, const char *prefix)
      	struct object_id oid;
      	struct tree *tree;
      	int i, full_tree = 0;
    -+	const char *implicit_format = NULL;
    -+	const char *format = NULL;
    -+	struct read_tree_ls_tree_data read_tree_cb_data = {
    -+		.sb_scratch = STRBUF_INIT,
    -+		.sb_tmp = STRBUF_INIT,
    -+	};
    ++	read_tree_fn_t fn = show_tree;
      	const struct option ls_tree_options[] = {
      		OPT_BIT('d', NULL, &ls_options, N_("only show trees"),
      			LS_TREE_ONLY),
    @@ builtin/ls-tree.c: int cmd_ls_tree(int argc, const char **argv, const char *pref
      		OPT_BOOL(0, "full-tree", &full_tree,
      			 N_("list entire tree; not just current directory "
      			    "(implies --full-name)")),
    -+		OPT_STRING_F(0 , "format", &format, N_("format"),
    -+			     N_("format to use for the output"), PARSE_OPT_NONEG),
    ++		OPT_STRING_F(0, "format", &format, N_("format"),
    ++			     N_("format to use for the output"),
    ++			     PARSE_OPT_NONEG),
      		OPT__ABBREV(&abbrev),
      		OPT_END()
      	};
    -+	read_tree_fn_t fn = show_tree;
    - 
    - 	git_config(git_default_config, NULL);
    - 	ls_tree_prefix = prefix;
     @@ builtin/ls-tree.c: int cmd_ls_tree(int argc, const char **argv, const char *prefix)
    - 	if ( (LS_TREE_ONLY|LS_RECURSIVE) ==
      	    ((LS_TREE_ONLY|LS_RECURSIVE) & ls_options))
      		ls_options |= LS_SHOW_TREES;
    -+	if (ls_options & LS_NAME_ONLY)
    -+		implicit_format = ls_tree_format_n;
    -+	if (ls_options & LS_SHOW_SIZE)
    -+		implicit_format = ls_tree_format_l;
    -+
    -+	if (format && implicit_format)
    -+		usage_msg_opt(_("providing --format cannot be combined with other format-altering options"),
    -+			      ls_tree_usage, ls_tree_options);
    -+	if (implicit_format)
    -+		format = implicit_format;
    -+	if (!format)
    -+		format = ls_tree_format_d;
      
    ++	if (format && cmdmode)
    ++		usage_msg_opt(
    ++			_("--format can't be combined with other format-altering options"),
    ++			ls_tree_usage, ls_tree_options);
      	if (argc < 1)
      		usage_with_options(ls_tree_usage, ls_tree_options);
    + 	if (get_oid(argv[0], &oid))
     @@ builtin/ls-tree.c: int cmd_ls_tree(int argc, const char **argv, const char *prefix)
      	tree = parse_tree_indirect(&oid);
      	if (!tree)
      		die("not a tree object");
    +-	return !!read_tree(the_repository, tree,
    +-			   &pathspec, show_tree, NULL);
     +
     +	/*
     +	 * The generic show_tree_fmt() is slower than show_tree(), so
     +	 * take the fast path if possible.
     +	 */
    -+	if (format && (!strcmp(format, ls_tree_format_d) ||
    -+		       !strcmp(format, ls_tree_format_l) ||
    -+		       !strcmp(format, ls_tree_format_n)))
    ++	if (format &&
    ++	    (!strcmp(format, ls_tree_format_d) ||
    ++	     !strcmp(format, ls_tree_format_l) ||
    ++	     !strcmp(format, ls_tree_format_n) ||
    ++	     !strcmp(format, ls_tree_format_o)))
     +		fn = show_tree;
     +	else if (format)
     +		fn = show_tree_fmt;
    -+	/*
    -+	 * Allow forcing the show_tree_fmt(), to test that it can
    -+	 * handle the test suite.
    -+	 */
    -+	if (git_env_bool("GIT_TEST_LS_TREE_FORMAT_BACKEND", 0))
    -+		fn = show_tree_fmt;
     +
    -+	read_tree_cb_data.format = format;
    - 	return !!read_tree(the_repository, tree,
    --			   &pathspec, show_tree, NULL);
    -+			   &pathspec, fn, &read_tree_cb_data);
    ++	return !!read_tree(the_repository, tree, &pathspec, fn, NULL);
      }
     
      ## t/t3105-ls-tree-format.sh (new) ##
    @@ t/t3105-ls-tree-format.sh (new)
     +. ./test-lib.sh
     +
     +test_expect_success 'ls-tree --format usage' '
    -+	test_expect_code 129 git ls-tree --format=fmt -l &&
    -+	test_expect_code 129 git ls-tree --format=fmt --name-only &&
    -+	test_expect_code 129 git ls-tree --format=fmt --name-status
    ++	test_expect_code 129 git ls-tree --format=fmt -l HEAD &&
    ++	test_expect_code 129 git ls-tree --format=fmt --name-only HEAD &&
    ++	test_expect_code 129 git ls-tree --format=fmt --name-status HEAD &&
    ++	test_expect_code 129 git ls-tree --format=fmt --object-only HEAD
     +'
     +
     +test_expect_success 'setup' '
    @@ t/t3105-ls-tree-format.sh (new)
     +	test_ls_tree_format \
     +		"%(path)" \
     +		"--name-only"
    ++'
     +
    ++test_expect_success 'ls-tree --format=<object-only-like>' '
    ++	test_ls_tree_format \
    ++		"%(objectname)" \
    ++		"--object-only"
     +'
     +
     +test_done

On Thu, Jan 13, 2022 at 5:04 PM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:

Agree, especially if "--format" comes earlier as you mentioned in another reply.

The doc change should only include the new "--format" here, so we just: