diff mbox series

[v3,1/1] ls-tree.c: support `--oid-only` option for "git-ls-tree"

Message ID 6c15b4c176b7c03072fa59a4efd9f6fea7d62eae.1637567328.git.dyroneteng@gmail.com (mailing list archive)
State Superseded
Headers show
Series ls-tree.c: support `--oid-only` option | expand

Commit Message

Teng Long Nov. 22, 2021, 8:07 a.m. UTC
Sometimes, we only want to get the objects from output of `ls-tree`
and commands like `sed` or `cut` is usually used to intercept the
origin output to achieve this purpose in practical.

This commit supply an option names `--oid-only` to let `git ls-tree`
only print out the OID of the object. `--oid-only` and `--name-only`
are mutually exclusive in use.

Signed-off-by: Teng Long <dyroneteng@gmail.com>
---
 Documentation/git-ls-tree.txt |  8 +++++--
 builtin/ls-tree.c             | 27 ++++++++++++++++-------
 t/t3104-ls-tree-oid.sh        | 40 +++++++++++++++++++++++++++++++++++
 3 files changed, 65 insertions(+), 10 deletions(-)
 create mode 100755 t/t3104-ls-tree-oid.sh

Comments

Peter Baumann Nov. 22, 2021, 6:11 p.m. UTC | #1
[ Sorry if you receive this mail twice, it seems like it didn't get
through the first time. ]

On Mon, Nov 22, 2021 at 9:50 AM Teng Long <dyroneteng@gmail.com> wrote:
>
> Sometimes, we only want to get the objects from output of `ls-tree`
> and commands like `sed` or `cut` is usually used to intercept the
> origin output to achieve this purpose in practical.
>
> This commit supply an option names `--oid-only` to let `git ls-tree`
> only print out the OID of the object. `--oid-only` and `--name-only`
> are mutually exclusive in use.
>
> Signed-off-by: Teng Long <dyroneteng@gmail.com>
> ---
>  Documentation/git-ls-tree.txt |  8 +++++--
>  builtin/ls-tree.c             | 27 ++++++++++++++++-------
>  t/t3104-ls-tree-oid.sh        | 40 +++++++++++++++++++++++++++++++++++
>  3 files changed, 65 insertions(+), 10 deletions(-)
>  create mode 100755 t/t3104-ls-tree-oid.sh
>
> diff --git a/Documentation/git-ls-tree.txt b/Documentation/git-ls-tree.txt
> index db02d6d79a..bc711dc00a 100644
> --- a/Documentation/git-ls-tree.txt
> +++ b/Documentation/git-ls-tree.txt
> @@ -10,7 +10,8 @@ SYNOPSIS
>  --------
>  [verse]
>  'git ls-tree' [-d] [-r] [-t] [-l] [-z]
> -           [--name-only] [--name-status] [--full-name] [--full-tree] [--abbrev[=<n>]]
> +           [--name-only] [--name-status] [--oid-only]
> +           [--full-name] [--full-tree] [--abbrev[=<n>]]
>             <tree-ish> [<path>...]
>

Shouldn't the synopsis also indicate that the options are exclusive, e.g.
  [--name-only | --oid-only]  ?

Besides adding the new --oid-only mode, you also add one letter acronyms for
  [-n | --name-only]
  [-s | --name-status ]
and one letter abbreviation
  [-o | --oid-only ]
which are all undocumented in the help page. If we want the short one
letter version,
they should be documented. For me, it is at least questionable why we
introduce them
and more so in a commit adding --oid-only.


>  DESCRIPTION
> @@ -59,7 +60,10 @@ OPTIONS
>  --name-only::
>  --name-status::
>         List only filenames (instead of the "long" output), one per line.
> -
> +       Cannot be used with `--oid-only` together.
> +--oid-only::
> +       List only OIDs of the objects, one per line. Cannot be used with
> +       `--name-only` or `--name-status` together.
>  --abbrev[=<n>]::
>         Instead of showing the full 40-byte hexadecimal object
>         lines, show the shortest prefix that is at least '<n>'
> diff --git a/builtin/ls-tree.c b/builtin/ls-tree.c
> index 3a442631c7..1e4a82e669 100644
> --- a/builtin/ls-tree.c
> +++ b/builtin/ls-tree.c
> @@ -18,19 +18,26 @@ static int line_termination = '\n';
>  #define LS_RECURSIVE 1
>  #define LS_TREE_ONLY 2
>  #define LS_SHOW_TREES 4
> -#define LS_NAME_ONLY 8
> -#define LS_SHOW_SIZE 16
> +#define LS_SHOW_SIZE 8
>  static int abbrev;
>  static int ls_options;
>  static struct pathspec pathspec;
>  static int chomp_prefix;
>  static const char *ls_tree_prefix;
>
> -static const  char * const ls_tree_usage[] = {
> +static const char * const ls_tree_usage[] = {
>         N_("git ls-tree [<options>] <tree-ish> [<path>...]"),
>         NULL
>  };
>
> +enum {
> +       MODE_UNSPECIFIED = 0,
> +       MODE_NAME_ONLY,
> +       MODE_OID_ONLY
> +};
> +
> +static int cmdmode = MODE_UNSPECIFIED;
> +
>  static int show_recursive(const char *base, int baselen, const char *pathname)
>  {
>         int i;
> @@ -90,7 +97,12 @@ static int show_tree(const struct object_id *oid, struct strbuf *base,
>         else if (ls_options & LS_TREE_ONLY)
>                 return 0;
>
> -       if (!(ls_options & LS_NAME_ONLY)) {
> +       if (cmdmode == 2) {
> +               printf("%s\n", find_unique_abbrev(oid, abbrev));
> +               return 0;
> +       }
> +
> +       if (cmdmode == 0) {
>                 if (ls_options & LS_SHOW_SIZE) {
>                         char size_text[24];
>                         if (!strcmp(type, blob_type)) {
> @@ -135,10 +147,9 @@ int cmd_ls_tree(int argc, const char **argv, const char *prefix)
>                             N_("terminate entries with NUL byte"), 0),
>                 OPT_BIT('l', "long", &ls_options, N_("include object size"),
>                         LS_SHOW_SIZE),
> -               OPT_BIT(0, "name-only", &ls_options, N_("list only filenames"),
> -                       LS_NAME_ONLY),
> -               OPT_BIT(0, "name-status", &ls_options, N_("list only filenames"),
> -                       LS_NAME_ONLY),
> +               OPT_CMDMODE('n', "name-only", &cmdmode, N_("list only filenames"), MODE_NAME_ONLY),
> +               OPT_CMDMODE('s', "name-status", &cmdmode, N_("list only filenames"), MODE_NAME_ONLY),
> +               OPT_CMDMODE('o', "oid-only", &cmdmode, N_("list only oids"), MODE_OID_ONLY),
>                 OPT_SET_INT(0, "full-name", &chomp_prefix,
>                             N_("use full path names"), 0),
>                 OPT_BOOL(0, "full-tree", &full_tree,
> diff --git a/t/t3104-ls-tree-oid.sh b/t/t3104-ls-tree-oid.sh
> new file mode 100755
> index 0000000000..4c02cdd3c3
> --- /dev/null
> +++ b/t/t3104-ls-tree-oid.sh
> @@ -0,0 +1,40 @@
> +#!/bin/sh
> +
> +test_description='git ls-tree oids handling.'
> +
> +. ./test-lib.sh
> +
> +test_expect_success 'setup' '
> +       echo 111 >1.txt &&
> +       echo 222 >2.txt &&
> +       mkdir -p path0/a/b/c &&
> +       echo 333 >path0/a/b/c/3.txt &&
> +       find *.txt path* \( -type f -o -type l \) -print |

I don't see the test using any symbolic links. Why are we searching for
symbolic links with "-type l" here?

-Peter

> +       xargs git update-index --add &&
> +       tree=$(git write-tree) &&
> +       echo $tree
> +'

> +
> +test_expect_success 'usage: --oid-only' '
> +       git ls-tree --oid-only $tree >current &&
> +       git ls-tree $tree | awk "{print \$3}" >expected &&
> +       test_cmp current expected
> +'
> +
> +test_expect_success 'usage: --oid-only with -r' '
> +       git ls-tree --oid-only -r $tree >current &&
> +       git ls-tree -r $tree | awk "{print \$3}" >expected &&
> +       test_cmp current expected
> +'
> +
> +test_expect_success 'usage: --oid-only with --abbrev' '
> +       git ls-tree --oid-only --abbrev=6 $tree >current &&
> +       git ls-tree --abbrev=6 $tree | awk "{print \$3}" > expected &&
> +       test_cmp current expected
> +'
> +
> +test_expect_failure 'usage: incompatible options: --name-only with --oid-only' '
> +       test_incompatible_usage git ls-tree --oid-only --name-only
> +'
> +
> +test_done
> --
> 2.33.1.10.g438dd9044d.dirty
>
Junio C Hamano Nov. 22, 2021, 6:54 p.m. UTC | #2
Teng Long <dyroneteng@gmail.com> writes:

> Sometimes, we only want to get the objects from output of `ls-tree`
> and commands like `sed` or `cut` is usually used to intercept the
> origin output to achieve this purpose in practical.

"in practical" -> "in practice".

That's true and that is exactly this plumbing command was designed
to be used.

> This commit supply an option names `--oid-only` to let `git ls-tree`
> only print out the OID of the object. `--oid-only` and `--name-only`
> are mutually exclusive in use.

    Teach the "--oid-only" option to tell the command to only show
    the object name, just like "--name-only" option tells the
    command to only show the path component, for each entry.  These
    two options are mutually exclusive.

perhaps?

The above leaves "mode-only" and "type-only".  I wonder if it is a
better design to add just one new option, --hide-fields, and make
the existing --name-only into a synonym to

    git ls-tree --hide-fields=mode,type,object $T

which would mean we do not need to end up with four mutually
exclusive commands, and anybody who wants to only see object names
can do

    git ls-tree --hide-fields=mode,type,file $T

Note: the above uses the terminology in the OUTPUT FORMAT section;
if we want to use "name" instead of "file", I am perfectly OK with
it, but then we should update the documentation to match.

Come to think of it, I think "--show-fields" may work even better
than "--hide-fields".  We can use it to get rid of the "--long"
option:

    git ls-tree --show-fields=mode,type,object,size,file $T

would be equivelent to

    git ls-tree --long $T

The field order may need to be thought through, especially when "-z"
output is not being used.  We may need a rule to require "file" to
be at the end, if exists, or even simpler rule "you can choose which
fields are shown but the order they come out is not affected" (i.e.
"--show-fields=mode,type" and "--show-fields=type,mode" give the
same output).

I am OK if we started with "only a single field allowed" and extend
it to support multiple fields later (until that happens, we cannot
emulate the "--long" output, though).  Then we do not have to answer
two tricky questions, what to do with the output order, and what
field separators are used in the output.
Đoàn Trần Công Danh Nov. 23, 2021, 12:14 a.m. UTC | #3
On 2021-11-22 16:07:28+0800, Teng Long <dyroneteng@gmail.com> wrote:
> Sometimes, we only want to get the objects from output of `ls-tree`
> and commands like `sed` or `cut` is usually used to intercept the
> origin output to achieve this purpose in practical.
> 
> This commit supply an option names `--oid-only` to let `git ls-tree`
> only print out the OID of the object. `--oid-only` and `--name-only`
> are mutually exclusive in use.
> 
> Signed-off-by: Teng Long <dyroneteng@gmail.com>
> ---
>  Documentation/git-ls-tree.txt |  8 +++++--
>  builtin/ls-tree.c             | 27 ++++++++++++++++-------
>  t/t3104-ls-tree-oid.sh        | 40 +++++++++++++++++++++++++++++++++++
>  3 files changed, 65 insertions(+), 10 deletions(-)
>  create mode 100755 t/t3104-ls-tree-oid.sh
> 
> diff --git a/Documentation/git-ls-tree.txt b/Documentation/git-ls-tree.txt
> index db02d6d79a..bc711dc00a 100644
> --- a/Documentation/git-ls-tree.txt
> +++ b/Documentation/git-ls-tree.txt
> @@ -10,7 +10,8 @@ SYNOPSIS
>  --------
>  [verse]
>  'git ls-tree' [-d] [-r] [-t] [-l] [-z]
> -	    [--name-only] [--name-status] [--full-name] [--full-tree] [--abbrev[=<n>]]
> +	    [--name-only] [--name-status] [--oid-only]

Please indicate those options are incompatible (as someone else said):

	[--name-only | --name-status | --oid-only]

> +	    [--full-name] [--full-tree] [--abbrev[=<n>]]
>  	    <tree-ish> [<path>...]
>  
>  DESCRIPTION
> @@ -59,7 +60,10 @@ OPTIONS
>  --name-only::
>  --name-status::
>  	List only filenames (instead of the "long" output), one per line.
> -
> +	Cannot be used with `--oid-only` together.
> +--oid-only::
> +	List only OIDs of the objects, one per line. Cannot be used with
> +	`--name-only` or `--name-status` together.
>  --abbrev[=<n>]::
>  	Instead of showing the full 40-byte hexadecimal object
>  	lines, show the shortest prefix that is at least '<n>'
> diff --git a/builtin/ls-tree.c b/builtin/ls-tree.c
> index 3a442631c7..1e4a82e669 100644
> --- a/builtin/ls-tree.c
> +++ b/builtin/ls-tree.c
> @@ -18,19 +18,26 @@ static int line_termination = '\n';
>  #define LS_RECURSIVE 1
>  #define LS_TREE_ONLY 2
>  #define LS_SHOW_TREES 4
> -#define LS_NAME_ONLY 8
> -#define LS_SHOW_SIZE 16
> +#define LS_SHOW_SIZE 8
>  static int abbrev;
>  static int ls_options;
>  static struct pathspec pathspec;
>  static int chomp_prefix;
>  static const char *ls_tree_prefix;
>  
> -static const  char * const ls_tree_usage[] = {
> +static const char * const ls_tree_usage[] = {
>  	N_("git ls-tree [<options>] <tree-ish> [<path>...]"),
>  	NULL
>  };
>  
> +enum {
> +	MODE_UNSPECIFIED = 0,
> +	MODE_NAME_ONLY,
> +	MODE_OID_ONLY
> +};
> +
> +static int cmdmode = MODE_UNSPECIFIED;
> +
>  static int show_recursive(const char *base, int baselen, const char *pathname)
>  {
>  	int i;
> @@ -90,7 +97,12 @@ static int show_tree(const struct object_id *oid, struct strbuf *base,
>  	else if (ls_options & LS_TREE_ONLY)
>  		return 0;
>  
> -	if (!(ls_options & LS_NAME_ONLY)) {
> +	if (cmdmode == 2) {

I think it's better to use the enum name:

	if (cmdmode == MODE_OID_ONLY) {

> +		printf("%s\n", find_unique_abbrev(oid, abbrev));
> +		return 0;
> +	}
> +
> +	if (cmdmode == 0) {

Ditto:

	if (cmdmode == MODE_UNSPECIFIED) {

Speaking about this, where will MODE_NAME_ONLY be used?

>  		if (ls_options & LS_SHOW_SIZE) {
>  			char size_text[24];
>  			if (!strcmp(type, blob_type)) {
> @@ -135,10 +147,9 @@ int cmd_ls_tree(int argc, const char **argv, const char *prefix)
>  			    N_("terminate entries with NUL byte"), 0),
>  		OPT_BIT('l', "long", &ls_options, N_("include object size"),
>  			LS_SHOW_SIZE),
> -		OPT_BIT(0, "name-only", &ls_options, N_("list only filenames"),
> -			LS_NAME_ONLY),
> -		OPT_BIT(0, "name-status", &ls_options, N_("list only filenames"),
> -			LS_NAME_ONLY),
> +		OPT_CMDMODE('n', "name-only", &cmdmode, N_("list only filenames"), MODE_NAME_ONLY),
> +		OPT_CMDMODE('s', "name-status", &cmdmode, N_("list only filenames"), MODE_NAME_ONLY),
> +		OPT_CMDMODE('o', "oid-only", &cmdmode, N_("list only oids"), MODE_OID_ONLY),
>  		OPT_SET_INT(0, "full-name", &chomp_prefix,
>  			    N_("use full path names"), 0),
>  		OPT_BOOL(0, "full-tree", &full_tree,
> diff --git a/t/t3104-ls-tree-oid.sh b/t/t3104-ls-tree-oid.sh
> new file mode 100755
> index 0000000000..4c02cdd3c3
> --- /dev/null
> +++ b/t/t3104-ls-tree-oid.sh
> @@ -0,0 +1,40 @@
> +#!/bin/sh
> +
> +test_description='git ls-tree oids handling.'
> +
> +. ./test-lib.sh
> +
> +test_expect_success 'setup' '
> +	echo 111 >1.txt &&
> +	echo 222 >2.txt &&
> +	mkdir -p path0/a/b/c &&
> +	echo 333 >path0/a/b/c/3.txt &&
> +	find *.txt path* \( -type f -o -type l \) -print |
> +	xargs git update-index --add &&
> +	tree=$(git write-tree) &&
> +	echo $tree
> +'
> +
> +test_expect_success 'usage: --oid-only' '
> +	git ls-tree --oid-only $tree >current &&
> +	git ls-tree $tree | awk "{print \$3}" >expected &&
> +	test_cmp current expected
> +'
> +
> +test_expect_success 'usage: --oid-only with -r' '
> +	git ls-tree --oid-only -r $tree >current &&
> +	git ls-tree -r $tree | awk "{print \$3}" >expected &&
> +	test_cmp current expected
> +'
> +
> +test_expect_success 'usage: --oid-only with --abbrev' '
> +	git ls-tree --oid-only --abbrev=6 $tree >current &&
> +	git ls-tree --abbrev=6 $tree | awk "{print \$3}" > expected &&
> +	test_cmp current expected
> +'
> +
> +test_expect_failure 'usage: incompatible options: --name-only with --oid-only' '
> +	test_incompatible_usage git ls-tree --oid-only --name-only
> +'
> +
> +test_done

It seems like you haven't updated the test code from v2
Ævar Arnfjörð Bjarmason Nov. 23, 2021, 1:09 a.m. UTC | #4
On Mon, Nov 22 2021, Junio C Hamano wrote:

> Teng Long <dyroneteng@gmail.com> writes:
>
>> Sometimes, we only want to get the objects from output of `ls-tree`
>> and commands like `sed` or `cut` is usually used to intercept the
>> origin output to achieve this purpose in practical.
>
> "in practical" -> "in practice".
>
> That's true and that is exactly this plumbing command was designed
> to be used.
>
>> This commit supply an option names `--oid-only` to let `git ls-tree`
>> only print out the OID of the object. `--oid-only` and `--name-only`
>> are mutually exclusive in use.
>
>     Teach the "--oid-only" option to tell the command to only show
>     the object name, just like "--name-only" option tells the
>     command to only show the path component, for each entry.  These
>     two options are mutually exclusive.
>
> perhaps?
>
> The above leaves "mode-only" and "type-only".  I wonder if it is a
> better design to add just one new option, --hide-fields, and make
> the existing --name-only into a synonym to
>
>     git ls-tree --hide-fields=mode,type,object $T
>
> which would mean we do not need to end up with four mutually
> exclusive commands, and anybody who wants to only see object names
> can do
>
>     git ls-tree --hide-fields=mode,type,file $T
>
> Note: the above uses the terminology in the OUTPUT FORMAT section;
> if we want to use "name" instead of "file", I am perfectly OK with
> it, but then we should update the documentation to match.
>
> Come to think of it, I think "--show-fields" may work even better
> than "--hide-fields".  We can use it to get rid of the "--long"
> option:
>
>     git ls-tree --show-fields=mode,type,object,size,file $T
>
> would be equivelent to
>
>     git ls-tree --long $T
>
> The field order may need to be thought through, especially when "-z"
> output is not being used.  We may need a rule to require "file" to
> be at the end, if exists, or even simpler rule "you can choose which
> fields are shown but the order they come out is not affected" (i.e.
> "--show-fields=mode,type" and "--show-fields=type,mode" give the
> same output).
>
> I am OK if we started with "only a single field allowed" and extend
> it to support multiple fields later (until that happens, we cannot
> emulate the "--long" output, though).  Then we do not have to answer
> two tricky questions, what to do with the output order, and what
> field separators are used in the output.

All of which (and more) would also be addressed in an obvious way by
just supporting --format as I suggested in
https://lore.kernel.org/git/211115.86o86lqe3c.gmgdl@evledraar.gmail.com/;
don't you think that's a better approach?

As noted in
https://lore.kernel.org/git/211115.86mtm5saz7.gmgdl@evledraar.gmail.com/
we could start by simply dying if the format is not on a small list of
formats we handle, i.e. not implement the strbuf_expand() change to
start with.

A --show-fields and --hide-fields seems like a rather elaborate
interface in lieu of just having a --format.

You'd presumably then want a --field-seperator and
--name-field-separator (we use SP and TAB for the two, currently), so
we've got N option now just to emulate what a --format would do for us.
Junio C Hamano Nov. 23, 2021, 1:26 a.m. UTC | #5
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

> All of which (and more) would also be addressed in an obvious way by
> just supporting --format as I suggested in
> https://lore.kernel.org/git/211115.86o86lqe3c.gmgdl@evledraar.gmail.com/;
> don't you think that's a better approach?

That is what I would call over-engineering that I would rather not
to have in low level plumbing.

I am all for making _parsing_ the output from the tool easier by
scripts; I am not interested in eliminating the _output_ by scripts.
They should capture and format the pieces we output in any way they
want.

So, no, I do not think it is a better approach at all.
Ævar Arnfjörð Bjarmason Nov. 23, 2021, 2:28 a.m. UTC | #6
On Mon, Nov 22 2021, Junio C Hamano wrote:

> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
>
>> All of which (and more) would also be addressed in an obvious way by
>> just supporting --format as I suggested in
>> https://lore.kernel.org/git/211115.86o86lqe3c.gmgdl@evledraar.gmail.com/;
>> don't you think that's a better approach?
>
> That is what I would call over-engineering that I would rather not
> to have in low level plumbing.
>
> I am all for making _parsing_ the output from the tool easier by
> scripts; I am not interested in eliminating the _output_ by scripts.
> They should capture and format the pieces we output in any way they
> want.
>
> So, no, I do not think it is a better approach at all.

We've got --format for for-each-ref and family (also branch etc.), and
for the "log" family.

I'm not sure I understand what you're saying, do you think if we could
go back and change it that the "FIELD NAMES" in git-for-each-ref (which
is plumbing) would have been better done as
--field-name=refname,objecttype,... etc?

Having used it extensively it's been very hard to have the flexibility
of formatting, e.g. to specify arbitrary delimiters.

It also leaves the door open to teaching ls-tree etc. the %(if) syntax
in the ref-filter, e.g. if you'd like to only print out certain data for
certain object types or whatever.
Junio C Hamano Nov. 23, 2021, 2:55 a.m. UTC | #7
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

>> That is what I would call over-engineering that I would rather not
>> to have in low level plumbing.
>> ...
> We've got --format for for-each-ref and family (also branch etc.), and
> for the "log" family.

But I didn't comment on them. ls-tree is a lot lower-level plumbing
where --format does not belong in my mind.
Junio C Hamano Nov. 23, 2021, 3:35 a.m. UTC | #8
Junio C Hamano <gitster@pobox.com> writes:

> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
>
>>> That is what I would call over-engineering that I would rather not
>>> to have in low level plumbing.
>>> ...
>> We've got --format for for-each-ref and family (also branch etc.), and
>> for the "log" family.
>
> But I didn't comment on them. ls-tree is a lot lower-level plumbing
> where --format does not belong in my mind.

There is a lot more practical reason why I'd prefer a less flexible
and good enough interface.

I can see, without coding it myself but from mere memory of how the
code looked like, how such a "we allow you to choose which field to
include, but you do not get to choose the order of fields or any
other string in the output" can be done with minimum disruption to
the existing code and without introducing a bug.  On the other hand,
I am fairly certain that anything more flexible than that will risk
new bugs involved in any large shuffling of the code, which I am
getting tired of.

So there.
Ævar Arnfjörð Bjarmason Nov. 23, 2021, 11:04 a.m. UTC | #9
On Mon, Nov 22 2021, Junio C Hamano wrote:

> Junio C Hamano <gitster@pobox.com> writes:
>
>> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
>>
>>>> That is what I would call over-engineering that I would rather not
>>>> to have in low level plumbing.
>>>> ...
>>> We've got --format for for-each-ref and family (also branch etc.), and
>>> for the "log" family.
>>
>> But I didn't comment on them. ls-tree is a lot lower-level plumbing
>> where --format does not belong in my mind.

Yes, you're just talking about ls-tree here. I'm just trying to
understand what you meant with:

    I am not interested in eliminating the _output_ by scripts.
    They should capture and format the pieces we output in any way they
    want.

Which reads to me like "we provide the data, you pretty-fy it". In this
case the proposed feature doesn't need a patch to git, but can also be
done as:

    git ls-tree HEAD | cut -d$'\t' -f1 | cut -d ' ' -f3

I think it's useful. I'm just trying to understand what you think about
such plumbing output.

> There is a lot more practical reason why I'd prefer a less flexible
> and good enough interface.
>
> I can see, without coding it myself but from mere memory of how the
> code looked like, how such a "we allow you to choose which field to
> include, but you do not get to choose the order of fields or any
> other string in the output" can be done with minimum disruption to
> the existing code and without introducing a bug.  On the other hand,
> I am fairly certain that anything more flexible than that will risk
> new bugs involved in any large shuffling of the code, which I am
> getting tired of.

To be clear, I wasn't talking about running with the WIP patch I had in
<211115.86o86lqe3c.gmgdl@evledraar.gmail.com> here, but that the
interface wolud leave the door open to it. So something like the below.

This works to do what --oid-only does without adding that switch,
instead we add it tou our list of 4 supported hardcoded formats, all of
which map to one of the MODE_* flags.

We could just document that we support that limited list for now, and
that we might add more in the future.

So it's just a way of adding a new MODE_* without supporting an ever
growing list of flags, --oid-only, --objectmode-only, --objectsize-only
etc.

Then if we'd ever want to generalize this in the future we can pick up
someting like my WIP patch and we'd have support for any arbitrary
format.

diff --git a/builtin/ls-tree.c b/builtin/ls-tree.c
index 1e4a82e669a..e1e746ae02a 100644
--- a/builtin/ls-tree.c
+++ b/builtin/ls-tree.c
@@ -30,10 +30,11 @@ static const char * const ls_tree_usage[] = {
 	NULL
 };
 
-enum {
+enum ls_tree_cmdmode {
 	MODE_UNSPECIFIED = 0,
 	MODE_NAME_ONLY,
-	MODE_OID_ONLY
+	MODE_OID_ONLY,
+	MODE_LONG,
 };
 
 static int cmdmode = MODE_UNSPECIFIED;
@@ -131,11 +132,22 @@ static int show_tree(const struct object_id *oid, struct strbuf *base,
 	return retval;
 }
 
+static struct {
+	enum ls_tree_cmdmode cmdmode;
+	const char *fmt;
+} allowed_formats[] = {
+	{ MODE_UNSPECIFIED,	"%(objectmode) %(objecttype) %(objectname)%09%(path)" },
+	{ MODE_NAME_ONLY,	"%(path)" },
+	{ MODE_OID_ONLY,	"%(objectname)" },
+	{ MODE_LONG,		"%(objectmode) %(objecttype) %(objectsize) %(objectname)%09%(path)" },
+};
+
 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 *format = NULL;
 	const struct option ls_tree_options[] = {
 		OPT_BIT('d', NULL, &ls_options, N_("only show trees"),
 			LS_TREE_ONLY),
@@ -149,7 +161,8 @@ int cmd_ls_tree(int argc, const char **argv, const char *prefix)
 			LS_SHOW_SIZE),
 		OPT_CMDMODE('n', "name-only", &cmdmode, N_("list only filenames"), MODE_NAME_ONLY),
 		OPT_CMDMODE('s', "name-status", &cmdmode, N_("list only filenames"), MODE_NAME_ONLY),
-		OPT_CMDMODE('o', "oid-only", &cmdmode, N_("list only oids"), MODE_OID_ONLY),
+		OPT_STRING(0 , "format", &format, N_("format"),
+			   N_("(limited) format to use for the output")),
 		OPT_SET_INT(0, "full-name", &chomp_prefix,
 			    N_("use full path names"), 0),
 		OPT_BOOL(0, "full-tree", &full_tree,
@@ -170,6 +183,22 @@ int cmd_ls_tree(int argc, const char **argv, const char *prefix)
 		ls_tree_prefix = prefix = NULL;
 		chomp_prefix = 0;
 	}
+
+	if (format && cmdmode)
+		die(_("--format and --name-only, --long etc. are incompatible"));
+	if (format) {
+		size_t i;
+
+		for (i = 0; i <= ARRAY_SIZE(allowed_formats); i++) {
+			if (i == ARRAY_SIZE(allowed_formats))
+				die(_("your --format=%s is not on the whitelist of supported formats"), format);
+			if (!strcmp(format, allowed_formats[i].fmt)) {
+				cmdmode = allowed_formats[i].cmdmode;
+				break;
+			}
+		}
+	}
+
 	/* -d -r should imply -t, but -d by itself should not have to. */
 	if ( (LS_TREE_ONLY|LS_RECURSIVE) ==
 	    ((LS_TREE_ONLY|LS_RECURSIVE) & ls_options))
diff mbox series

Patch

diff --git a/Documentation/git-ls-tree.txt b/Documentation/git-ls-tree.txt
index db02d6d79a..bc711dc00a 100644
--- a/Documentation/git-ls-tree.txt
+++ b/Documentation/git-ls-tree.txt
@@ -10,7 +10,8 @@  SYNOPSIS
 --------
 [verse]
 'git ls-tree' [-d] [-r] [-t] [-l] [-z]
-	    [--name-only] [--name-status] [--full-name] [--full-tree] [--abbrev[=<n>]]
+	    [--name-only] [--name-status] [--oid-only]
+	    [--full-name] [--full-tree] [--abbrev[=<n>]]
 	    <tree-ish> [<path>...]
 
 DESCRIPTION
@@ -59,7 +60,10 @@  OPTIONS
 --name-only::
 --name-status::
 	List only filenames (instead of the "long" output), one per line.
-
+	Cannot be used with `--oid-only` together.
+--oid-only::
+	List only OIDs of the objects, one per line. Cannot be used with
+	`--name-only` or `--name-status` together.
 --abbrev[=<n>]::
 	Instead of showing the full 40-byte hexadecimal object
 	lines, show the shortest prefix that is at least '<n>'
diff --git a/builtin/ls-tree.c b/builtin/ls-tree.c
index 3a442631c7..1e4a82e669 100644
--- a/builtin/ls-tree.c
+++ b/builtin/ls-tree.c
@@ -18,19 +18,26 @@  static int line_termination = '\n';
 #define LS_RECURSIVE 1
 #define LS_TREE_ONLY 2
 #define LS_SHOW_TREES 4
-#define LS_NAME_ONLY 8
-#define LS_SHOW_SIZE 16
+#define LS_SHOW_SIZE 8
 static int abbrev;
 static int ls_options;
 static struct pathspec pathspec;
 static int chomp_prefix;
 static const char *ls_tree_prefix;
 
-static const  char * const ls_tree_usage[] = {
+static const char * const ls_tree_usage[] = {
 	N_("git ls-tree [<options>] <tree-ish> [<path>...]"),
 	NULL
 };
 
+enum {
+	MODE_UNSPECIFIED = 0,
+	MODE_NAME_ONLY,
+	MODE_OID_ONLY
+};
+
+static int cmdmode = MODE_UNSPECIFIED;
+
 static int show_recursive(const char *base, int baselen, const char *pathname)
 {
 	int i;
@@ -90,7 +97,12 @@  static int show_tree(const struct object_id *oid, struct strbuf *base,
 	else if (ls_options & LS_TREE_ONLY)
 		return 0;
 
-	if (!(ls_options & LS_NAME_ONLY)) {
+	if (cmdmode == 2) {
+		printf("%s\n", find_unique_abbrev(oid, abbrev));
+		return 0;
+	}
+
+	if (cmdmode == 0) {
 		if (ls_options & LS_SHOW_SIZE) {
 			char size_text[24];
 			if (!strcmp(type, blob_type)) {
@@ -135,10 +147,9 @@  int cmd_ls_tree(int argc, const char **argv, const char *prefix)
 			    N_("terminate entries with NUL byte"), 0),
 		OPT_BIT('l', "long", &ls_options, N_("include object size"),
 			LS_SHOW_SIZE),
-		OPT_BIT(0, "name-only", &ls_options, N_("list only filenames"),
-			LS_NAME_ONLY),
-		OPT_BIT(0, "name-status", &ls_options, N_("list only filenames"),
-			LS_NAME_ONLY),
+		OPT_CMDMODE('n', "name-only", &cmdmode, N_("list only filenames"), MODE_NAME_ONLY),
+		OPT_CMDMODE('s', "name-status", &cmdmode, N_("list only filenames"), MODE_NAME_ONLY),
+		OPT_CMDMODE('o', "oid-only", &cmdmode, N_("list only oids"), MODE_OID_ONLY),
 		OPT_SET_INT(0, "full-name", &chomp_prefix,
 			    N_("use full path names"), 0),
 		OPT_BOOL(0, "full-tree", &full_tree,
diff --git a/t/t3104-ls-tree-oid.sh b/t/t3104-ls-tree-oid.sh
new file mode 100755
index 0000000000..4c02cdd3c3
--- /dev/null
+++ b/t/t3104-ls-tree-oid.sh
@@ -0,0 +1,40 @@ 
+#!/bin/sh
+
+test_description='git ls-tree oids handling.'
+
+. ./test-lib.sh
+
+test_expect_success 'setup' '
+	echo 111 >1.txt &&
+	echo 222 >2.txt &&
+	mkdir -p path0/a/b/c &&
+	echo 333 >path0/a/b/c/3.txt &&
+	find *.txt path* \( -type f -o -type l \) -print |
+	xargs git update-index --add &&
+	tree=$(git write-tree) &&
+	echo $tree
+'
+
+test_expect_success 'usage: --oid-only' '
+	git ls-tree --oid-only $tree >current &&
+	git ls-tree $tree | awk "{print \$3}" >expected &&
+	test_cmp current expected
+'
+
+test_expect_success 'usage: --oid-only with -r' '
+	git ls-tree --oid-only -r $tree >current &&
+	git ls-tree -r $tree | awk "{print \$3}" >expected &&
+	test_cmp current expected
+'
+
+test_expect_success 'usage: --oid-only with --abbrev' '
+	git ls-tree --oid-only --abbrev=6 $tree >current &&
+	git ls-tree --abbrev=6 $tree | awk "{print \$3}" > expected &&
+	test_cmp current expected
+'
+
+test_expect_failure 'usage: incompatible options: --name-only with --oid-only' '
+	test_incompatible_usage git ls-tree --oid-only --name-only
+'
+
+test_done