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

Commit Message

Teng Long Dec. 8, 2021, 2:08 a.m. UTC

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" (mutually exclusive).

Signed-off-by: Teng Long <[e-mail]>
---
 Documentation/git-ls-tree.txt |   7 +-
 builtin/ls-tree.c             | 125 ++++++++++++++++++++++++----------
 t/t3103-ls-tree-misc.sh       |   8 +++
 t/t3104-ls-tree-oid.sh        |  51 ++++++++++++++
 4 files changed, 154 insertions(+), 37 deletions(-)
 create mode 100755 t/t3104-ls-tree-oid.sh

Comments

Junio C Hamano Dec. 15, 2021, 7:25 p.m. UTC | #1

Teng Long <[e-mail]> writes:

> 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" (mutually exclusive).

I notice that this changed from --oid to --object and I agree that
it would probably be more friendly to end users.  In fact, this

    $ sed -ne '/^SYNOPSIS/,/^DESCRIPTION/p' Documentation/git-*.txt |
      grep -e -oid

did not find any hits.

> diff --git a/builtin/ls-tree.c b/builtin/ls-tree.c
> index 3a442631c7..beaa8bf13b 100644
> --- a/builtin/ls-tree.c
> +++ b/builtin/ls-tree.c
> @@ -16,21 +16,38 @@
>  
>  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_TREE_ONLY 1 << 1
> +#define LS_SHOW_TREES 1 << 2
> +#define LS_NAME_ONLY 1 << 3
> +#define LS_SHOW_SIZE 1 << 4
> +#define LS_OBJECT_ONLY 1 << 5

It usually is a good idea to enclose these in () so that they are
safe to use in any context in a statement.

Luckily, bitwise-or and bitwise-and, which are the most likely
candidates for these symbols to be used with, bind looser than
left-shift, so something like 

	if ((LS_TREE_ONLY | LS_SHOW_TREES) & opt)
		... do this ...

is safe either way, but (LS_TREE_ONLY + LS_SHOW_TREES) would have
different value with and without () around (1 << N).

> +static unsigned int shown_bits = 0;

Style: we do not initialize statics explicitly to zero.

> +#define SHOW_DEFAULT 29 /* 11101 size is not shown to output by default */
> +#define SHOW_MODE 1 << 4
> +#define SHOW_TYPE 1 << 3
> +#define SHOW_OBJECT_NAME 1 << 2
> +#define SHOW_SIZE 1 << 1
> +#define SHOW_FILE_NAME 1

Likewise.  It is a bit curious to see these listed in decreasing
order, though.

>  static const  char * const ls_tree_usage[] = {
>  	N_("git ls-tree [<options>] <tree-ish> [<path>...]"),
>  	NULL
>  };
>  
> +enum {
> +	MODE_UNSPECIFIED = 0,
> +	MODE_NAME_ONLY,
> +	MODE_OBJECT_ONLY,
> +	MODE_LONG

It is a good idea to leave a comma even after the last element,
_unless_ there is a strong reason why the element that currently is
at the last MUST stay to be last when new elements are added to the
enum.  That way, a future patch that adds a new element can add it
to the list with a patch noise with fewer lines.

> +};
> +
> +static int cmdmode = MODE_UNSPECIFIED;

This is also initializing a static variable to zero, and arguments
can be made either way: (A) unspecified is set to zero in enum
definition exactly so that we can use zero to signal the variable is
unspecified, so an explicit zero initialization here goes against
the spirit of choosing 0 as MODE_UNSPECIFIED; or (B) enum definition
can be scrambled in future changes to use something other than zero
for MODE_UNSPECIFIED, and explicitly writing it like this is more
future-proof.

I am OK with the way it is written (i.e. (B)).

> @@ -66,6 +83,7 @@ static int show_tree(const struct object_id *oid, struct strbuf *base,
>  {
>  	int retval = 0;
>  	int baselen;
> +	int follow = 0;

A better name, anybody?

This bit is to keep track of the fact that we made _some_ output
already so any further output needs an inter-field space before
writing what it wants to write out.

>  	const char *type = blob_type;
>  
>  	if (S_ISGITLINK(mode)) {
> @@ -74,8 +92,8 @@ 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;
>  		 *
>  		 */

Nice ;-)

>  		type = commit_type;
> @@ -90,35 +108,67 @@ 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 (ls_options & LS_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
> -				xsnprintf(size_text, sizeof(size_text), "-");
> -			printf("%06o %s %s %7s\t", mode, type,
> -			       find_unique_abbrev(oid, abbrev),
> -			       size_text);
> +	if (shown_bits & SHOW_MODE) {
> +		printf("%06o",mode);

SP before ','.

> +		follow = 1;
> +	}
> +	if (shown_bits & SHOW_TYPE) {
> +		printf("%s%s", follow == 1 ? " " : "", type);
> +		follow = 1;
> +	}
> +	if (shown_bits & SHOW_OBJECT_NAME) {
> +		printf("%s%s", follow == 1 ? " " : "",
> +		       find_unique_abbrev(oid, abbrev));
> +		if (!(shown_bits ^ SHOW_OBJECT_NAME))
> +			printf("%c", line_termination);

Curious.  I wonder if we can get rid of these two lines (and the
line_termination bit in the SHOW_FILE_NAME part), and have an
unconditional 

	putchar(line_termination);

at the end of the function.

That way, we could in the future choose to introduce a feature to
show only <mode, type, size> and nothing else, which may be useful
for taking per-type stats.

We need to stop using write_name_quoted_relative() in SHOW_FILE_NAME
part, because the helper insists that the name written by it must be
at the end of the entry, if we go that route, but it may be a good
change in the longer term.

> +		follow = 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,
> -			       find_unique_abbrev(oid, abbrev));
> +			xsnprintf(size_text, sizeof(size_text), "-");
> +		printf("%s%7s", follow == 1 ? " " : "", size_text);
> +		follow = 1;
> +	}
> +	if (shown_bits & SHOW_FILE_NAME) {
> +		if (follow)
> +			printf("\t");
> +		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);
>  	}

But the above nits aside, the updated organization of this function
looks much cleaner than the original.  Nicely reorganized.

> -	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 retval;
>  }

Thanks.

Teng Long Dec. 16, 2021, 12:16 p.m. UTC | #2

> I notice that this changed from --oid to --object and I agree that
> it would probably be more friendly to end users.  In fact, this
>
>    $ sed -ne '/^SYNOPSIS/,/^DESCRIPTION/p' Documentation/git-*.txt |
>      grep -e -oid
>
> did not find any hits.

Thank you for comfirming that.

> It usually is a good idea to enclose these in () so that they are
> safe to use in any context in a statement.
> 
> Luckily, bitwise-or and bitwise-and, which are the most likely
> candidates for these symbols to be used with, bind looser than
> left-shift, so something like 
> 
> 	if ((LS_TREE_ONLY | LS_SHOW_TREES) & opt)
> 		... do this ...
> 
> is safe either way, but (LS_TREE_ONLY + LS_SHOW_TREES) would have
> different value with and without () around (1 << N).

Make sense.

Will fix in patch v6.


> Style: we do not initialize statics explicitly to zero.
> ...
> Likewise.  It is a bit curious to see these listed in decreasing
> order, though.

Will fix.

> It is a good idea to leave a comma even after the last element,
> _unless_ there is a strong reason why the element that currently is
> at the last MUST stay to be last when new elements are added to the
> enum.  That way, a future patch that adds a new element can add it
> to the list with a patch noise with fewer lines.

Very clear explanation.

Will fix.

> A better name, anybody?
> 
> This bit is to keep track of the fact that we made _some_ output
> already so any further output needs an inter-field space before
> writing what it wants to write out.

I found a word "interspace", it looks like a little better than the old
one. I will rename to it in next patch, and If there's a better idea,
will apply further.

> SP before ','.

Will fix.


> Curious.  I wonder if we can get rid of these two lines (and the
> line_termination bit in the SHOW_FILE_NAME part), and have an
> unconditional 
> 
> 	putchar(line_termination);
> 
> at the end of the function.
> 
> That way, we could in the future choose to introduce a feature to
> show only <mode, type, size> and nothing else, which may be useful
> for taking per-type stats.
> 
> We need to stop using write_name_quoted_relative() in SHOW_FILE_NAME
> part, because the helper insists that the name written by it must be
> at the end of the entry, if we go that route, but it may be a good
> change in the longer term.

Let me try to represent to make sure I understand your suggestion
sufficiently.

"write_name_quoted_relative" is used to compute the relative file name
by "prefix" and output the name and a line termination to the given FD.

We do not want use "write_name_quoted_relative" in here because the
function alway output a line termination after "name", this may bring
some inconvenience because the "name" may not be the last field in the
future.

So, instead:

We need to calculate the file name (relative path and quotes if need)
without "write_name_quoted_relative"  and then output the line
termination before return.


Thanks.

Junio C Hamano Dec. 16, 2021, 9:26 p.m. UTC | #3

Teng Long <[e-mail]> writes:

>> A better name, anybody?
>> 
>> This bit is to keep track of the fact that we made _some_ output
>> already so any further output needs an inter-field space before
>> writing what it wants to write out.
>
> I found a word "interspace", it looks like a little better than the old
> one. I will rename to it in next patch, and If there's a better idea,
> will apply further.

Yeah, it really is needs_inter_field_space but that is way too long.

>> We need to stop using write_name_quoted_relative() in SHOW_FILE_NAME
>> part, because the helper insists that the name written by it must be
>> at the end of the entry, if we go that route, but it may be a good
>> change in the longer term.
>
> Let me try to represent to make sure I understand your suggestion
> sufficiently.
>
> "write_name_quoted_relative" is used to compute the relative file name
> by "prefix" and output the name and a line termination to the given FD.
>
> We do not want use "write_name_quoted_relative" in here because the
> function alway output a line termination after "name", this may bring
> some inconvenience because the "name" may not be the last field in the
> future.
>
> So, instead:
>
> We need to calculate the file name (relative path and quotes if need)
> without "write_name_quoted_relative"  and then output the line
> termination before return.

I think we are on the same page.  We can work backwards, I think.

We have a repetitive

        if (mode should be shown) {
                show mode;
                record that we have already shown something;
        }
        if (type should be shown) {
                give inter-field-space if we have shown something;
                show type;
                record that we have already shown something;
        }
        ...

that ends with

        if (name should be shown) {
                give inter-field-space if we have shown something;
                show name PLUS line termination;
	}

But if we can make the last step to

        if (name should be shown) {
                give inter-field-space if we have shown something;
                show name;
	}

	give line termination;

it gets easier to support a combination that does not show name, and
we can have inter-record separator.

But write_name_quoted_relative() does not give the caller a choice
to have no terminator, so we need to do something like this:

	if (shown_bits & SHOW_FILE_NAME) {
		const char *name;
                struct strbuf name_buf = STRBUF_INIT;

		if (follow)
			printf("\t");
		baselen = base->len;
		strbuf_addstr(base, pathname);
                
		name = relative_path(base->buf, 
				     chomp_prefix ? ls_tree_prefix : NULL,
                                     &name_buf);
		if (line_termination)
			quote_c_style(name, NULL, stdout, 0);
		else
			fputs(name, stdout);
		strbuf_release(&name_buf);
		strbuf_setlen(base, baselen);
	}

I initially thought that extending write_name_quoted() and
write_name_quoted_relative() to accept a special value or two for
terminator to tell it not to add terminator would be sufficient (see
below).  I however think it is way too ugly to have the "add no
terminator and do not quote" option at write_name_quoted() level,
simply because the caller that chooses as-is can simply do fputs()
itself without bothering to use write_name_quoted().  So I am not
convinced that it will a good idea.

If we were to go that "ugly helper" route, the above can become even
simpler and closer to what you originally wrote, e.g.

	if (shown_bits & SHOW_FILE_NAME) {
		if (follow)
			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);
	}


 quote.c |  5 +++--
 quote.h | 19 +++++++++++++++++++
 2 files changed, 22 insertions(+), 2 deletions(-)

diff --git c/quote.c w/quote.c
index 26719d21d1..cbbcd8563f 100644
--- c/quote.c
+++ w/quote.c
@@ -340,12 +340,13 @@ 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 {
 		fputs(name, fp);
 	}
-	fputc(terminator, fp);
+	if (0 <= terminator)
+		fputc(terminator, fp);
 }
 
 void write_name_quoted_relative(const char *name, const char *prefix,
diff --git c/quote.h w/quote.h
index 87ff458b06..5c8c7cf952 100644
--- c/quote.h
+++ w/quote.h
@@ -85,7 +85,26 @@ int unquote_c_style(struct strbuf *, const char *quoted, const char **endp);
 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);

Ævar Arnfjörð Bjarmason Dec. 16, 2021, 9:29 p.m. UTC | #4

On Thu, Dec 16 2021, Junio C Hamano wrote:

> Teng Long <[e-mail]> writes:
>
>>> A better name, anybody?
>>> 
>>> This bit is to keep track of the fact that we made _some_ output
>>> already so any further output needs an inter-field space before
>>> writing what it wants to write out.
>>
>> I found a word "interspace", it looks like a little better than the old
>> one. I will rename to it in next patch, and If there's a better idea,
>> will apply further.
>
> Yeah, it really is needs_inter_field_space but that is way too long.
>
>>> We need to stop using write_name_quoted_relative() in SHOW_FILE_NAME
>>> part, because the helper insists that the name written by it must be
>>> at the end of the entry, if we go that route, but it may be a good
>>> change in the longer term.
>>
>> Let me try to represent to make sure I understand your suggestion
>> sufficiently.
>>
>> "write_name_quoted_relative" is used to compute the relative file name
>> by "prefix" and output the name and a line termination to the given FD.
>>
>> We do not want use "write_name_quoted_relative" in here because the
>> function alway output a line termination after "name", this may bring
>> some inconvenience because the "name" may not be the last field in the
>> future.
>>
>> So, instead:
>>
>> We need to calculate the file name (relative path and quotes if need)
>> without "write_name_quoted_relative"  and then output the line
>> termination before return.
>
> I think we are on the same page.  We can work backwards, I think.
>
> We have a repetitive
>
>         if (mode should be shown) {
>                 show mode;
>                 record that we have already shown something;
>         }
>         if (type should be shown) {
>                 give inter-field-space if we have shown something;
>                 show type;
>                 record that we have already shown something;
>         }
>         ...
>
> that ends with
>
>         if (name should be shown) {
>                 give inter-field-space if we have shown something;
>                 show name PLUS line termination;
> 	}
>
> But if we can make the last step to
>
>         if (name should be shown) {
>                 give inter-field-space if we have shown something;
>                 show name;
> 	}
>
> 	give line termination;
>
> it gets easier to support a combination that does not show name, and
> we can have inter-record separator.
>
> But write_name_quoted_relative() does not give the caller a choice
> to have no terminator, so we need to do something like this:
>
> 	if (shown_bits & SHOW_FILE_NAME) {
> 		const char *name;
>                 struct strbuf name_buf = STRBUF_INIT;
>
> 		if (follow)
> 			printf("\t");
> 		baselen = base->len;
> 		strbuf_addstr(base, pathname);
>                 
> 		name = relative_path(base->buf, 
> 				     chomp_prefix ? ls_tree_prefix : NULL,
>                                      &name_buf);
> 		if (line_termination)
> 			quote_c_style(name, NULL, stdout, 0);
> 		else
> 			fputs(name, stdout);
> 		strbuf_release(&name_buf);
> 		strbuf_setlen(base, baselen);
> 	}
>
> I initially thought that extending write_name_quoted() and
> write_name_quoted_relative() to accept a special value or two for
> terminator to tell it not to add terminator would be sufficient (see
> below).  I however think it is way too ugly to have the "add no
> terminator and do not quote" option at write_name_quoted() level,
> simply because the caller that chooses as-is can simply do fputs()
> itself without bothering to use write_name_quoted().  So I am not
> convinced that it will a good idea.
>
> If we were to go that "ugly helper" route, the above can become even
> simpler and closer to what you originally wrote, e.g.
>
> 	if (shown_bits & SHOW_FILE_NAME) {
> 		if (follow)
> 			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);
> 	}
>
>
>  quote.c |  5 +++--
>  quote.h | 19 +++++++++++++++++++
>  2 files changed, 22 insertions(+), 2 deletions(-)
>
> diff --git c/quote.c w/quote.c
> index 26719d21d1..cbbcd8563f 100644
> --- c/quote.c
> +++ w/quote.c
> @@ -340,12 +340,13 @@ 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 {
>  		fputs(name, fp);
>  	}
> -	fputc(terminator, fp);
> +	if (0 <= terminator)
> +		fputc(terminator, fp);
>  }
>  
>  void write_name_quoted_relative(const char *name, const char *prefix,
> diff --git c/quote.h w/quote.h
> index 87ff458b06..5c8c7cf952 100644
> --- c/quote.h
> +++ w/quote.h
> @@ -85,7 +85,26 @@ int unquote_c_style(struct strbuf *, const char *quoted, const char **endp);
>  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);
>  

In my "just make ls-tree support --format" this whole thing is just:
    
    +               if (prefix)
    +                       name = relative_path(name, prefix, &scratch);
    +               quote_c_style(name, sb, NULL, 0);
    +               strbuf_addch(sb, line_termination);

But of course that's an implementation that's moved away from the "write
stuff to a FH for me" API in quote.c.

So I haven't looked carefully here, but you need this API just because
of some constraint in the write_name_quoted()?

Perhaps just running with that approach is better? Whether it's stealing
that approach, or doing away with --object-only for a --format...

Patch

diff --git a/Documentation/git-ls-tree.txt b/Documentation/git-ls-tree.txt
index db02d6d79a..729370f235 100644
--- a/Documentation/git-ls-tree.txt
+++ b/Documentation/git-ls-tree.txt
@@ -10,7 +10,7 @@  SYNOPSIS
 --------
 [verse]
 'git ls-tree' [-d] [-r] [-t] [-l] [-z]
-	    [--name-only] [--name-status] [--full-name] [--full-tree] [--abbrev[=<n>]]
+	    [--name-only] [--name-status] [--object-only] [--full-name] [--full-tree] [--abbrev[=<n>]]
 	    <tree-ish> [<path>...]
 
 DESCRIPTION
@@ -59,6 +59,11 @@  OPTIONS
 --name-only::
 --name-status::
 	List only filenames (instead of the "long" output), one per line.
+	Cannot be combined with `--object-only`.
+
+--object-only::
+	List only names of the objects, one per line. Cannot be combined
+	with `--name-only` or `--name-status`.
 
 --abbrev[=<n>]::
 	Instead of showing the full 40-byte hexadecimal object
diff --git a/builtin/ls-tree.c b/builtin/ls-tree.c
index 3a442631c7..beaa8bf13b 100644
--- a/builtin/ls-tree.c
+++ b/builtin/ls-tree.c
@@ -16,21 +16,38 @@ 
 
 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_TREE_ONLY 1 << 1
+#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;
 static int chomp_prefix;
 static const char *ls_tree_prefix;
+static unsigned int shown_bits = 0;
+#define SHOW_DEFAULT 29 /* 11101 size is not shown to output by default */
+#define SHOW_MODE 1 << 4
+#define SHOW_TYPE 1 << 3
+#define SHOW_OBJECT_NAME 1 << 2
+#define SHOW_SIZE 1 << 1
+#define SHOW_FILE_NAME 1
 
 static const  char * const ls_tree_usage[] = {
 	N_("git ls-tree [<options>] <tree-ish> [<path>...]"),
 	NULL
 };
 
+enum {
+	MODE_UNSPECIFIED = 0,
+	MODE_NAME_ONLY,
+	MODE_OBJECT_ONLY,
+	MODE_LONG
+};
+
+static int cmdmode = MODE_UNSPECIFIED;
+
 static int show_recursive(const char *base, int baselen, const char *pathname)
 {
 	int i;
@@ -66,6 +83,7 @@  static int show_tree(const struct object_id *oid, struct strbuf *base,
 {
 	int retval = 0;
 	int baselen;
+	int follow = 0;
 	const char *type = blob_type;
 
 	if (S_ISGITLINK(mode)) {
@@ -74,8 +92,8 @@  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;
@@ -90,35 +108,67 @@  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 (ls_options & LS_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
-				xsnprintf(size_text, sizeof(size_text), "-");
-			printf("%06o %s %s %7s\t", mode, type,
-			       find_unique_abbrev(oid, abbrev),
-			       size_text);
+	if (shown_bits & SHOW_MODE) {
+		printf("%06o",mode);
+		follow = 1;
+	}
+	if (shown_bits & SHOW_TYPE) {
+		printf("%s%s", follow == 1 ? " " : "", type);
+		follow = 1;
+	}
+	if (shown_bits & SHOW_OBJECT_NAME) {
+		printf("%s%s", follow == 1 ? " " : "",
+		       find_unique_abbrev(oid, abbrev));
+		if (!(shown_bits ^ SHOW_OBJECT_NAME))
+			printf("%c", line_termination);
+		follow = 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,
-			       find_unique_abbrev(oid, abbrev));
+			xsnprintf(size_text, sizeof(size_text), "-");
+		printf("%s%7s", follow == 1 ? " " : "", size_text);
+		follow = 1;
+	}
+	if (shown_bits & SHOW_FILE_NAME) {
+		if (follow)
+			printf("\t");
+		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);
 	}
-	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 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;
@@ -133,12 +183,14 @@  int cmd_ls_tree(int argc, const char **argv, const char *prefix)
 			LS_SHOW_TREES),
 		OPT_SET_INT('z', NULL, &line_termination,
 			    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('l', "long", &cmdmode, N_("include object size"),
+			    MODE_LONG),
+		OPT_CMDMODE(0, "name-only", &cmdmode, N_("list only filenames"),
+			    MODE_NAME_ONLY),
+		OPT_CMDMODE(0, "name-status", &cmdmode, N_("list only filenames"),
+			    MODE_NAME_ONLY),
+		OPT_CMDMODE(0, "object-only", &cmdmode, N_("list only objects"),
+			    MODE_OBJECT_ONLY),
 		OPT_SET_INT(0, "full-name", &chomp_prefix,
 			    N_("use full path names"), 0),
 		OPT_BOOL(0, "full-tree", &full_tree,
@@ -169,6 +221,7 @@  int cmd_ls_tree(int argc, const char **argv, const char *prefix)
 	if (get_oid(argv[0], &oid))
 		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
diff --git a/t/t3103-ls-tree-misc.sh b/t/t3103-ls-tree-misc.sh
index 14520913af..75e38b0a51 100755
--- a/t/t3103-ls-tree-misc.sh
+++ b/t/t3103-ls-tree-misc.sh
@@ -22,4 +22,12 @@  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
diff --git a/t/t3104-ls-tree-oid.sh b/t/t3104-ls-tree-oid.sh
new file mode 100755
index 0000000000..81304e7b13
--- /dev/null
+++ b/t/t3104-ls-tree-oid.sh
@@ -0,0 +1,51 @@ 
+#!/bin/sh
+
+test_description='git ls-tree objects handling.'
+
+. ./test-lib.sh
+
+test_expect_success 'setup' '
+	test_commit A &&
+	test_commit B &&
+	mkdir -p C &&
+	test_commit C/D.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: --object-only' '
+	git ls-tree --object-only $tree >current &&
+	git ls-tree $tree >result &&
+	cut -f1 result | cut -d " " -f3 >expected &&
+	test_cmp current expected
+'
+
+test_expect_success 'usage: --object-only with -r' '
+	git ls-tree --object-only -r $tree >current &&
+	git ls-tree -r $tree >result &&
+	cut -f1 result | cut -d " " -f3 >expected &&
+	test_cmp current expected
+'
+
+test_expect_success 'usage: --object-only with --abbrev' '
+	git ls-tree --object-only --abbrev=6 $tree >current &&
+	git ls-tree --abbrev=6 $tree >result &&
+	cut -f1 result | cut -d " " -f3 >expected &&
+	test_cmp current expected
+'
+
+test_expect_success 'usage: incompatible options: --name-only with --object-only' '
+	test_expect_code 129 git ls-tree --object-only --name-only
+'
+
+test_expect_success 'usage: incompatible options: --name-status with --object-only' '
+	test_expect_code 129 git ls-tree --object-only --name-status
+'
+
+test_expect_success 'usage: incompatible options: --long with --object-only' '
+	test_expect_code 129 git ls-tree --object-only --long
+'
+
+test_done