[v13,00/16] ls-tree: "--object-only" and "--format" opts

Message

Teng Long March 21, 2022, 7:33 a.m. UTC

Diff from v12:

Current patch is based by the review patch of Ævar Arnfjörð Bjarmason in [1].

In terms of the review commits, I replied Ævar Arnfjörð Bjarmason [2] and listed the
actions, therefore this v13 patch was implemented by these actions.  

Links:

1. https://public-inbox.org/git/[e-mail]/
2. https://public-inbox.org/git/[e-mail]/

Thanks.

Johannes Schindelin (1):
  cocci: allow padding with `strbuf_addf()`

Teng Long (5):
  ls-tree: rename "retval" to "recurse" in "show_tree()"
  ls-tree: simplify nesting if/else logic in "show_tree()"
  ls-tree: fix "--name-only" and "--long" combined use bug
  ls-tree: slightly refactor `show_tree()`
  ls-tree: support --object-only option for "git-ls-tree"

Ævar Arnfjörð Bjarmason (10):
  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: introduce struct "show_tree_data"
  ls-tree: introduce "--format" option
  ls-tree tests: add tests for --name-status
  ls-tree: detect and error on --name-only --name-status
  ls-tree: remove FIELD_*, just use MODE_*
  ls-tree: split up "fast path" callbacks

 Documentation/git-ls-tree.txt   |  68 +++++-
 builtin/ls-tree.c               | 367 ++++++++++++++++++++++++++------
 contrib/coccinelle/strbuf.cocci |   6 +-
 t/t3101-ls-tree-dirname.sh      |  55 ++---
 t/t3103-ls-tree-misc.sh         |  15 ++
 t/t3104-ls-tree-format.sh       |  76 +++++++
 6 files changed, 493 insertions(+), 94 deletions(-)
 create mode 100755 t/t3104-ls-tree-format.sh

Range-diff against v12:
 -:  ---------- >  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:  99e6d47108 ls-tree: rename "retval" to "recurse" in "show_tree()"
 -:  ---------- >  6:  a8d9b78dea ls-tree: simplify nesting if/else logic in "show_tree()"
 -:  ---------- >  7:  25a07e048f ls-tree: fix "--name-only" and "--long" combined use bug
 1:  f449146b4d !  8:  55f1e10d7e ls-tree: slightly refactor `show_tree()`
    @@ builtin/ls-tree.c
     -#define LS_SHOW_SIZE 16
     +#define LS_TREE_ONLY (1 << 1)
     +#define LS_SHOW_TREES (1 << 2)
    -+#define LS_NAME_ONLY (1 << 3)
    -+#define LS_SHOW_SIZE (1 << 4)
      static int abbrev;
      static int ls_options;
      static struct pathspec pathspec;
    @@ builtin/ls-tree.c
      	NULL
      };
      
    -+static enum mutx_option {
    -+	MODE_UNSPECIFIED = 0,
    ++static enum ls_tree_cmdmode {
    ++	MODE_LONG = 1,
     +	MODE_NAME_ONLY,
    -+	MODE_LONG,
     +} cmdmode;
     +
     +static int parse_shown_fields(void)
 2:  80311adc7c !  9:  85d0b0da1d ls-tree: 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 enum mutx_option {
    - 	MODE_LONG,
    +@@ builtin/ls-tree.c: static enum ls_tree_cmdmode {
    + 	MODE_NAME_ONLY,
      } cmdmode;
      
     -static int parse_shown_fields(void)
 3:  459080f549 = 10:  eddcf903ad cocci: allow padding with `strbuf_addf()`
 4:  cb717d08be ! 11:  7451242daa ls-tree: introduce "--format" option
    @@ Commit message
         idea and suggestion, this commit makes modifications in terms of the
         original discussion on community [1].
     
    +    In [1] there was a "GIT_TEST_LS_TREE_FORMAT_BACKEND" variable to
    +    ensure that we had test coverage for passing tests that would
    +    otherwise use show_tree() through show_tree_fmt(), and thus that the
    +    formatting mechanism could handle all the same cases as the
    +    non-formatting options.
    +
    +    Somewhere in subsequent re-rolls of that we seem to have drifted away
    +    from what the goal of these tests should be. We're trying to ensure
    +    correctness of show_tree_fmt(). We can't tell if we "hit [the]
    +    fast-path" here, and instead of having an explicit test for that, we
    +    can just add it to something our "test_ls_tree_format" tests for.
    +
         Here is the statistics about performance tests:
     
         1. Default format (hitten the builtin formats):
    @@ Commit message
     
         Links:
                 [1] https://public-inbox.org/git/[e-mail]/
    +            [2] https://lore.kernel.org/git/cb[e-mail]/
     
         Signed-off-by: Ævar Arnfjörð Bjarmason <[e-mail]>
         Signed-off-by: Teng Long <[e-mail]>
    @@ builtin/ls-tree.c: static const char *ls_tree_prefix;
      struct show_tree_data {
      	unsigned mode;
      	enum object_type type;
    -@@ builtin/ls-tree.c: static enum mutx_option {
    - 	MODE_LONG,
    +@@ builtin/ls-tree.c: static enum ls_tree_cmdmode {
    + 	MODE_NAME_ONLY,
      } cmdmode;
      
     +static void expand_objectsize(struct strbuf *line, const struct object_id *oid,
    @@ t/t3104-ls-tree-format.sh (new)
     +	opts=$2 &&
     +	fmtopts=$3 &&
     +	shift 2 &&
    -+	git ls-tree $opts -r HEAD >expect.raw &&
    -+	sed "s/^/> /" >expect <expect.raw &&
    -+	git ls-tree --format="> $format" -r $fmtopts HEAD >actual &&
    -+	test_cmp expect actual
    ++
    ++	test_expect_success "ls-tree '--format=<$format>' is like options '$opts $fmtopts'" '
    ++		git ls-tree $opts -r HEAD >expect &&
    ++		git ls-tree --format="$format" -r $fmtopts HEAD >actual &&
    ++		test_cmp expect actual
    ++	'
    ++
    ++	test_expect_success "ls-tree '--format=<$format>' on optimized v.s. non-optimized path" '
    ++		git ls-tree --format="$format" -r $fmtopts HEAD >expect &&
    ++		git ls-tree --format="> $format" -r $fmtopts HEAD >actual.raw &&
    ++		sed "s/^> //" >actual <actual.raw &&
    ++		test_cmp expect actual
    ++	'
     +}
     +
    -+test_expect_success 'ls-tree --format=<default-like>' '
    -+	test_ls_tree_format \
    -+		"%(objectmode) %(objecttype) %(objectname)%x09%(path)" \
    -+		""
    -+'
    ++test_ls_tree_format \
    ++	"%(objectmode) %(objecttype) %(objectname)%x09%(path)" \
    ++	""
     +
    -+test_expect_success 'ls-tree --format=<long-like>' '
    -+	test_ls_tree_format \
    -+		"%(objectmode) %(objecttype) %(objectname) %(objectsize:padded)%x09%(path)" \
    -+		"--long"
    -+'
    ++test_ls_tree_format \
    ++	"%(objectmode) %(objecttype) %(objectname) %(objectsize:padded)%x09%(path)" \
    ++	"--long"
     +
    -+test_expect_success 'ls-tree --format=<name-only-like>' '
    -+	test_ls_tree_format \
    -+		"%(path)" \
    -+		"--name-only"
    -+'
    ++test_ls_tree_format \
    ++	"%(path)" \
    ++	"--name-only"
     +
    -+test_expect_success 'ls-tree combine --format=<default-like> and -t' '
    -+	test_ls_tree_format \
    ++test_ls_tree_format \
     +	"%(objectmode) %(objecttype) %(objectname)%x09%(path)" \
     +	"-t" \
     +	"-t"
    -+'
     +
    -+test_expect_success 'ls-tree combine --format=<default-like> and --full-name' '
    -+	test_ls_tree_format \
    ++test_ls_tree_format \
     +	"%(objectmode) %(objecttype) %(objectname)%x09%(path)" \
     +	"--full-name" \
     +	"--full-name"
    -+'
     +
    -+test_expect_success 'ls-tree combine --format=<default-like> and --full-tree' '
    -+	test_ls_tree_format \
    ++test_ls_tree_format \
     +	"%(objectmode) %(objecttype) %(objectname)%x09%(path)" \
     +	"--full-tree" \
     +	"--full-tree"
    -+'
     +
    -+test_expect_success 'ls-tree hit fast-path with --format=<default-like>' '
    -+	git ls-tree -r HEAD >expect &&
    -+	git ls-tree --format="%(objectmode) %(objecttype) %(objectname)%x09%(path)" -r HEAD >actual &&
    -+	test_cmp expect actual
    -+'
    -+
    -+test_expect_success 'ls-tree hit fast-path with --format=<name-only-like>' '
    -+	git ls-tree -r --name-only HEAD >expect &&
    -+	git ls-tree --format="%(path)" -r HEAD >actual &&
    -+	test_cmp expect actual
    -+'
     +test_done
 5:  55b1c4379d ! 12:  2bcd08ebd0 ls-tree: support --object-only option for "git-ls-tree"
    @@ Documentation/git-ls-tree.txt: OPTIONS
      	Instead of showing the full 40-byte hexadecimal object
     
      ## builtin/ls-tree.c ##
    -@@ builtin/ls-tree.c: static int line_termination = '\n';
    - #define LS_SHOW_TREES (1 << 2)
    - #define LS_NAME_ONLY (1 << 3)
    - #define LS_SHOW_SIZE (1 << 4)
    -+#define LS_OBJECT_ONLY (1 << 5)
    - static int abbrev;
    - static int ls_options;
    - static struct pathspec pathspec;
     @@ builtin/ls-tree.c: static const char *format;
      static const char *default_format = "%(objectmode) %(objecttype) %(objectname)%x09%(path)";
      static const char *long_format = "%(objectmode) %(objecttype) %(objectname) %(objectsize:padded)%x09%(path)";
    @@ builtin/ls-tree.c: static const char *format;
      	unsigned mode;
      	enum object_type type;
     @@ builtin/ls-tree.c: static const  char * const ls_tree_usage[] = {
    - static enum mutx_option {
    - 	MODE_UNSPECIFIED = 0,
    + static enum ls_tree_cmdmode {
    + 	MODE_LONG = 1,
      	MODE_NAME_ONLY,
     +	MODE_OBJECT_ONLY,
    - 	MODE_LONG,
      } cmdmode;
      
    + static void expand_objectsize(struct strbuf *line, const struct object_id *oid,
     @@ builtin/ls-tree.c: static int parse_shown_fields(unsigned int *shown_fields)
      		*shown_fields = FIELD_PATH_NAME;
      		return 0;
    @@ t/t3103-ls-tree-misc.sh: test_expect_success 'ls-tree fails with non-zero exit c
      test_done
     
      ## t/t3104-ls-tree-format.sh ##
    -@@ t/t3104-ls-tree-format.sh: test_expect_success 'ls-tree --format=<name-only-like>' '
    - 		"--name-only"
    - '
    +@@ t/t3104-ls-tree-format.sh: test_ls_tree_format \
    + 	"%(path)" \
    + 	"--name-only"
      
    -+test_expect_success 'ls-tree --format=<object-only-like>' '
    -+	test_ls_tree_format \
    -+		"%(objectname)" \
    -+		"--object-only"
    -+'
    ++test_ls_tree_format \
    ++	"%(objectname)" \
    ++	"--object-only"
     +
    -+test_expect_success 'ls-tree --format=<object-only-like> --abbrev' '
    -+	test_ls_tree_format \
    -+		"%(objectname)" \
    -+		"--object-only --abbrev" \
    -+		"--abbrev"
    -+'
    ++test_ls_tree_format \
    ++	"%(objectname)" \
    ++	"--object-only --abbrev" \
    ++	"--abbrev"
     +
    - test_expect_success 'ls-tree combine --format=<default-like> and -t' '
    - 	test_ls_tree_format \
    + test_ls_tree_format \
      	"%(objectmode) %(objecttype) %(objectname)%x09%(path)" \
    -@@ t/t3104-ls-tree-format.sh: test_expect_success 'ls-tree hit fast-path with --format=<name-only-like>' '
    - 	git ls-tree --format="%(path)" -r HEAD >actual &&
    - 	test_cmp expect actual
    - '
    -+
    -+test_expect_success 'ls-tree hit fast-path with --format=<object-only-like>' '
    -+	git ls-tree -r --object-only HEAD >expect &&
    -+	git ls-tree --format="%(objectname)" -r HEAD >actual &&
    -+	test_cmp expect actual
    -+'
    - test_done
    + 	"-t" \
 -:  ---------- > 13:  a5c73de057 ls-tree tests: add tests for --name-status
 -:  ---------- > 14:  89402a8518 ls-tree: detect and error on --name-only --name-status
 -:  ---------- > 15:  b8afca193a ls-tree: remove FIELD_*, just use MODE_*
 -:  ---------- > 16:  010e3c0ece ls-tree: split up "fast path" callbacks

Comments

Junio C Hamano March 21, 2022, 7:07 p.m. UTC | #1

Teng Long <[e-mail]> writes:

> Diff from v12:
>
> Current patch is based by the review patch of Ævar Arnfjörð Bjarmason in [1].
>
> In terms of the review commits, I replied Ævar Arnfjörð Bjarmason [2] and listed the
> actions, therefore this v13 patch was implemented by these actions.  
>
> Links:
>
> 1. https://public-inbox.org/git/[e-mail]/
> 2. https://public-inbox.org/git/[e-mail]/
>
> Thanks.

Thanks for sticking to this topic.  Will queue.