[PATCH] revision: remove stray whitespace when name empty

[PATCH] revision: remove stray whitespace when name empty

[Emily Shaffer]

Teach show_object_with_name() to avoid writing a space before a name
which is empty. Also teach tests for rev-list --objects --filter to not
require a space between the object ID and name.

show_object_with_name() inserts a space between an object's OID and name
regardless of whether the name is empty or not. This causes 'git
cat-file (--batch | --batch-check)' to fail to discover the type of root
directories:

  git rev-list --objects --filter=tree:1 --max-count=1 HEAD \
    | git cat-file --batch-check
  git rev-parse HEAD: | xargs -I% git cat-file -t %
  git rev-list --objects --filter=tree:1 --max-count=1 HEAD \
    | xargs -I% echo "AA%AA"
  git rev-list --objects --filter=tree:1 --max-count=1 HEAD \
    | cut -f 1 -d ' ' | git cat-file --batch-check

The extra space provided by 'show_object_with_name()' sticks to the
output of 'git rev-list' even if it's piped into a file.

Signed-off-by: Emily Shaffer <emilyshaffer@google.com>
---
I don't see any reason _not_ to remove this stray whitespace at the end,
since it seems like it just gets in the way of easy scripting. I also
think this case will only present itself for root trees.

Not sure if making the whitespace optional is the right solution for the
test, although I couldn't come up with a cleaner approach. Maybe
something like this would be better, even though handling it in the
regex is shorter?

  if [[ -z "$name" ]] then
    grep "^$hash" actual
  else
    grep "^$hash $name" actual
  fi

 - Emily

 revision.c                          | 3 ++-
 t/t6112-rev-list-filters-objects.sh | 2 +-
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/revision.c b/revision.c
index d4aaf0ef25..260258b371 100644
--- a/revision.c
+++ b/revision.c
@@ -40,7 +40,8 @@ void show_object_with_name(FILE *out, struct object *obj, const char *name)
 {
 	const char *p;
 
-	fprintf(out, "%s ", oid_to_hex(&obj->oid));
+	fprintf(out, "%s%s", oid_to_hex(&obj->oid),
+		strcmp(name, "") == 0 ? "" : " ");
 	for (p = name; *p && *p != '\n'; p++)
 		fputc(*p, out);
 	fputc('\n', out);
diff --git a/t/t6112-rev-list-filters-objects.sh b/t/t6112-rev-list-filters-objects.sh
index acd7f5ab80..e05faa7a67 100755
--- a/t/t6112-rev-list-filters-objects.sh
+++ b/t/t6112-rev-list-filters-objects.sh
@@ -288,7 +288,7 @@ expect_has () {
 	name=$2 &&
 
 	hash=$(git -C r3 rev-parse $commit:$name) &&
-	grep "^$hash $name$" actual
+	grep "^$hash \?$name$" actual
 }
 
 test_expect_success 'verify tree:1 includes root trees' '
-- 
2.22.0.rc2.383.gf4fbbf30c2-goog

Re: [PATCH] revision: remove stray whitespace when name empty

[Eric Sunshine]

On Fri, Jun 7, 2019 at 6:59 PM Emily Shaffer <emilyshaffer@google.com> wrote:
> Teach show_object_with_name() to avoid writing a space before a name
> which is empty. Also teach tests for rev-list --objects --filter to not
> require a space between the object ID and name.
> [...]
> Signed-off-by: Emily Shaffer <emilyshaffer@google.com>
> ---
> Not sure if making the whitespace optional is the right solution for the
> test, although I couldn't come up with a cleaner approach. Maybe
> something like this would be better, even though handling it in the
> regex is shorter?
>
>   if [[ -z "$name" ]] then

In Git, we avoid Bash-isms. Just use 'test'. And, style is to place
'then' on its own line.

    if test -z "$name"
    then

> diff --git a/revision.c b/revision.c
> @@ -40,7 +40,8 @@ void show_object_with_name(FILE *out, struct object *obj, const char *name)
>  {
> -       fprintf(out, "%s ", oid_to_hex(&obj->oid));
> +       fprintf(out, "%s%s", oid_to_hex(&obj->oid),
> +               strcmp(name, "") == 0 ? "" : " ");
>         for (p = name; *p && *p != '\n'; p++)
>                 fputc(*p, out);
>         fputc('\n', out);

It's subjective, but this starts to be a bit too noisy and unreadable.
An alternative:

    fputs(oid_to_hex(...), out);
    if (*name)
        putc(' ', out);

> diff --git a/t/t6112-rev-list-filters-objects.sh b/t/t6112-rev-list-filters-objects.sh
> @@ -288,7 +288,7 @@ expect_has () {
>         hash=$(git -C r3 rev-parse $commit:$name) &&
> -       grep "^$hash $name$" actual
> +       grep "^$hash \?$name$" actual

This would be the first use of \? with 'grep' in the test suite. I
would worry about portability. Your suggestion earlier to tailor
'grep' invocation based upon whether $name is empty would be safer.

Re: [PATCH] revision: remove stray whitespace when name empty

[Jeff King]

On Fri, Jun 07, 2019 at 03:59:00PM -0700, Emily Shaffer wrote:

> Teach show_object_with_name() to avoid writing a space before a name
> which is empty. Also teach tests for rev-list --objects --filter to not
> require a space between the object ID and name.
> [...]
> ---
> I don't see any reason _not_ to remove this stray whitespace at the end,
> since it seems like it just gets in the way of easy scripting. I also
> think this case will only present itself for root trees.

I'm a bit worried that this might break existing scripts. As ugly as
trailing whitespace is, it does tell you something here: that the object
is a root tree and not a commit.

So in the past I have done things like:

  git rev-list --objects --all | grep ' '

to get only the non-commits. I'm undecided on whether we're straying
into https://xkcd.com/1172/ territory here. I'd be more in favor if this
were making things significantly easier, but...

> show_object_with_name() inserts a space between an object's OID and name
> regardless of whether the name is empty or not. This causes 'git
> cat-file (--batch | --batch-check)' to fail to discover the type of root
> directories:
> 
>   git rev-list --objects --filter=tree:1 --max-count=1 HEAD \
>     | git cat-file --batch-check
>   git rev-parse HEAD: | xargs -I% git cat-file -t %
>   git rev-list --objects --filter=tree:1 --max-count=1 HEAD \
>     | xargs -I% echo "AA%AA"
>   git rev-list --objects --filter=tree:1 --max-count=1 HEAD \
>     | cut -f 1 -d ' ' | git cat-file --batch-check

Your patch only helps with this at all because you're using the "tree:1"
filter. It would not help:

  git rev-list --objects HEAD | git cat-file --batch-check

because there you'll have actual names which cat-file will choke on. So
it seems like this is helping only a very limited use case.

cat-file actually does know how to split on whitespace. Unfortunately it
does not do so by default, because that breaks some cases. See
97be04077f (cat-file: only split on whitespace when %(rest) is used,
2013-08-02).

So you _can_ do:

  git rev-list --objects HEAD |
  git cat-file --batch-check='%(objectname) %(objecttype) %(rest)'

But:

  1. That puts the %(rest) bits in your output, which you may not want.

  2. You have to actually specify the full format, so you might have to
     repeat batch-check's default format items.

I think it would be reasonable for cat-file to have an option to split
on whitespace (and if not given explicitly by the user, default to the
presence of %(rest) as we do now).

Alternatively, it would be reasonable to me to have an option for
"rev-list --objects" to have an option to suppress the filename (and
space) entirely.

I think in the longer run along those lines that "--objects" should
allow cat-file style pretty-print formats, which would eliminate the
need to pipe to cat-file in the first place. That makes this parsing
problem go away entirely, and it's way more efficient to boot (rev-list
already knows the types!).

-Peff

Re: [PATCH] revision: remove stray whitespace when name empty

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> Your patch only helps with this at all because you're using the "tree:1"
> ...
> because there you'll have actual names which cat-file will choke on. So
> it seems like this is helping only a very limited use case.
> ...
> Alternatively, it would be reasonable to me to have an option for
> "rev-list --objects" to have an option to suppress the filename (and
> space) entirely.

Yup, I think that is a more reasonable short-term change compared to
the patch being discussed, and ...

> I think in the longer run along those lines that "--objects" should
> allow cat-file style pretty-print formats, which would eliminate the
> need to pipe to cat-file in the first place. That makes this parsing
> problem go away entirely, and it's way more efficient to boot (rev-list
> already knows the types!).

... of course this makes tons of sense.

Thanks.

Re: [PATCH] revision: remove stray whitespace when name empty

[Emily Shaffer]

On Fri, Jun 07, 2019 at 08:42:45PM -0400, Eric Sunshine wrote:
> On Fri, Jun 7, 2019 at 6:59 PM Emily Shaffer <emilyshaffer@google.com> wrote:
> > Teach show_object_with_name() to avoid writing a space before a name
> > which is empty. Also teach tests for rev-list --objects --filter to not
> > require a space between the object ID and name.
> > [...]
> > Signed-off-by: Emily Shaffer <emilyshaffer@google.com>
> > ---
> > Not sure if making the whitespace optional is the right solution for the
> > test, although I couldn't come up with a cleaner approach. Maybe
> > something like this would be better, even though handling it in the
> > regex is shorter?
> >
> >   if [[ -z "$name" ]] then
> 
> In Git, we avoid Bash-isms. Just use 'test'. And, style is to place
> 'then' on its own line.
> 
>     if test -z "$name"
>     then
> 
> > diff --git a/revision.c b/revision.c
> > @@ -40,7 +40,8 @@ void show_object_with_name(FILE *out, struct object *obj, const char *name)
> >  {
> > -       fprintf(out, "%s ", oid_to_hex(&obj->oid));
> > +       fprintf(out, "%s%s", oid_to_hex(&obj->oid),
> > +               strcmp(name, "") == 0 ? "" : " ");
> >         for (p = name; *p && *p != '\n'; p++)
> >                 fputc(*p, out);
> >         fputc('\n', out);
> 
> It's subjective, but this starts to be a bit too noisy and unreadable.
> An alternative:
> 
>     fputs(oid_to_hex(...), out);
>     if (*name)
>         putc(' ', out);
> 
> > diff --git a/t/t6112-rev-list-filters-objects.sh b/t/t6112-rev-list-filters-objects.sh
> > @@ -288,7 +288,7 @@ expect_has () {
> >         hash=$(git -C r3 rev-parse $commit:$name) &&
> > -       grep "^$hash $name$" actual
> > +       grep "^$hash \?$name$" actual
> 
> This would be the first use of \? with 'grep' in the test suite. I
> would worry about portability. Your suggestion earlier to tailor
> 'grep' invocation based upon whether $name is empty would be safer.


Thanks for the review, Eric. I'm going to try again from scratch with
Peff and Junio's suggestion about adding an option to remove object
names, and I'll try to keep your general style concerns in mind when I
do so.

 - Emily

Re: [PATCH] revision: remove stray whitespace when name empty

[Emily Shaffer]

On Mon, Jun 10, 2019 at 09:29:14AM -0700, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
> 
> > Your patch only helps with this at all because you're using the "tree:1"
> > ...
> > because there you'll have actual names which cat-file will choke on. So
> > it seems like this is helping only a very limited use case.
> > ...
> > Alternatively, it would be reasonable to me to have an option for
> > "rev-list --objects" to have an option to suppress the filename (and
> > space) entirely.
> 
> Yup, I think that is a more reasonable short-term change compared to
> the patch being discussed, and ...

I like this too. Starting work on it today.

> 
> > I think in the longer run along those lines that "--objects" should
> > allow cat-file style pretty-print formats, which would eliminate the
> > need to pipe to cat-file in the first place. That makes this parsing
> > problem go away entirely, and it's way more efficient to boot (rev-list
> > already knows the types!).
> 
> ... of course this makes tons of sense.

Probably not going to start work on this now, but I will make a note to
myself to have a look if I have some spare time soon, and keep an eye
out for reviews in that area.

> 
> Thanks.

Thanks all for the feedback, I'll have a reroll soon.

 - Emily

Re: [PATCH] revision: remove stray whitespace when name empty

[Jeff King]

On Wed, Jun 12, 2019 at 12:37:45PM -0700, Emily Shaffer wrote:

> > > Alternatively, it would be reasonable to me to have an option for
> > > "rev-list --objects" to have an option to suppress the filename (and
> > > space) entirely.
> > 
> > Yup, I think that is a more reasonable short-term change compared to
> > the patch being discussed, and ...
> 
> I like this too. Starting work on it today.

Great!

> > > I think in the longer run along those lines that "--objects" should
> > > allow cat-file style pretty-print formats, which would eliminate the
> > > need to pipe to cat-file in the first place. That makes this parsing
> > > problem go away entirely, and it's way more efficient to boot (rev-list
> > > already knows the types!).
> > 
> > ... of course this makes tons of sense.
> 
> Probably not going to start work on this now, but I will make a note to
> myself to have a look if I have some spare time soon, and keep an eye
> out for reviews in that area.

That's fine. I suspect it will be quite involved, because the format
placeholders you'd want for "objects" are going to be more like
"cat-file" ones than like the existing pretty.c ones. So I think this
really implies unifying those format systems, which is a long-term
project that Olga has been working on.

-Peff

[PATCH v2] rev-list: teach --oid-only to enable piping

[Emily Shaffer]

Allow easier parsing by cat-file by giving rev-list an option to print
only the OID of an object without any additional information. This is a
short-term shim; later on, rev-list should be taught how to print the
types of objects it finds in a format similar to cat-file's.

Before this commit, the output from rev-list needed to be massaged
before being piped to cat-file, like so:

  git rev-list --objects HEAD | cut -f 1 -d ' ' \
    | git cat-file --batch-check

This was especially unexpected when dealing with root trees, as an
invisible whitespace exists at the end of the OID:

  git rev-list --objects --filter=tree:1 --max-count=1 HEAD \
    | xargs -I% echo "AA%AA"

Now, it can be piped directly, as in the added test case:

  git rev-list --objects --oid-only HEAD | git cat-file --batch-check

Signed-off-by: Emily Shaffer <emilyshaffer@google.com>
Change-Id: I489bdf0a8215532e540175188883ff7541d70e1b
---
It didn't appear that using an existing --pretty string would do the
trick, as the formatting options for --pretty apply specifically to
commit objects (you can specify the commit hash and the tree hash, but
I didn't see a way to more generally pretty-print all types of objects).
rev-list doesn't appear to use parse-options-h, so I added a new option
as simply as I could see without breaking existing style; it seemed
wrong to add a flag to the `struct rev_list_info` as the definition of
struct and flag values alike are contained in bisect.h. There are a
couple other options to rev-list which are stored as globals.

At the moment, this doesn't work with --abbrev, and although I think
it'd be a good fit, right now --abbrev doesn't seem to work without
--abbrev-commit, and both those options end up being eaten by
setup_revisions() rather than by rev-list itself.

 - Emily

 builtin/rev-list.c       | 21 ++++++++++++++++++++-
 t/t6000-rev-list-misc.sh | 18 ++++++++++++++++++
 2 files changed, 38 insertions(+), 1 deletion(-)

diff --git a/builtin/rev-list.c b/builtin/rev-list.c
index 9f31837d30..ff07ea9564 100644
--- a/builtin/rev-list.c
+++ b/builtin/rev-list.c
@@ -48,7 +48,7 @@ static const char rev_list_usage[] =
 "    --children\n"
 "    --objects | --objects-edge\n"
 "    --unpacked\n"
-"    --header | --pretty\n"
+"    --header | --pretty | --oid-only\n"
 "    --abbrev=<n> | --no-abbrev\n"
 "    --abbrev-commit\n"
 "    --left-right\n"
@@ -75,6 +75,8 @@ enum missing_action {
 };
 static enum missing_action arg_missing_action;
 
+static int arg_oid_only; /* display only the oid of each object encountered */
+
 #define DEFAULT_OIDSET_SIZE     (16*1024)
 
 static void finish_commit(struct commit *commit, void *data);
@@ -90,6 +92,11 @@ static void show_commit(struct commit *commit, void *data)
 		return;
 	}
 
+	if (arg_oid_only) {
+		printf("%s\n", oid_to_hex(&commit->object.oid));
+		return;
+	}
+
 	graph_show_commit(revs->graph);
 
 	if (revs->count) {
@@ -255,6 +262,10 @@ static void show_object(struct object *obj, const char *name, void *cb_data)
 	display_progress(progress, ++progress_counter);
 	if (info->flags & REV_LIST_QUIET)
 		return;
+	if (arg_oid_only) {
+		printf("%s\n", oid_to_hex(&obj->oid));
+		return;
+	}
 	show_object_with_name(stdout, obj, name);
 }
 
@@ -484,6 +495,11 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix)
 		if (skip_prefix(arg, "--missing=", &arg))
 			continue; /* already handled above */
 
+		if (!strcmp(arg, ("--oid-only"))) {
+			arg_oid_only = 1;
+			continue;
+		}
+
 		usage(rev_list_usage);
 
 	}
@@ -499,6 +515,9 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix)
 		/* Only --header was specified */
 		revs.commit_format = CMIT_FMT_RAW;
 
+	if (arg_oid_only && revs.commit_format != CMIT_FMT_UNSPECIFIED)
+		die(_("cannot combine --oid-only and --pretty or --header"));
+
 	if ((!revs.commits && reflog_walk_empty(revs.reflog_info) &&
 	     (!(revs.tag_objects || revs.tree_objects || revs.blob_objects) &&
 	      !revs.pending.nr) &&
diff --git a/t/t6000-rev-list-misc.sh b/t/t6000-rev-list-misc.sh
index 0507999729..996ba1799b 100755
--- a/t/t6000-rev-list-misc.sh
+++ b/t/t6000-rev-list-misc.sh
@@ -48,6 +48,24 @@ test_expect_success 'rev-list --objects with pathspecs and copied files' '
 	! grep one output
 '
 
+test_expect_success 'rev-list --objects --oid-only has no whitespace/names' '
+	git rev-list --objects --oid-only HEAD >output &&
+	! grep wanted_file output &&
+	! grep unwanted_file output &&
+	! grep " " output
+'
+
+test_expect_success 'rev-list --objects --oid-only is usable by cat-file' '
+	git rev-list --objects --oid-only --all >list-output &&
+	git cat-file --batch-check <list-output >cat-output &&
+	! grep missing cat-output
+'
+
+test_expect_success 'rev-list --oid-only is incompatible with --pretty' '
+	test_must_fail git rev-list --objects --oid-only --pretty HEAD &&
+	test_must_fail git rev-list --objects --oid-only --header HEAD
+'
+
 test_expect_success 'rev-list A..B and rev-list ^A B are the same' '
 	git commit --allow-empty -m another &&
 	git tag -a -m "annotated" v1.0 &&
-- 
2.22.0.rc2.383.gf4fbbf30c2-goog

Re: [PATCH v2] rev-list: teach --oid-only to enable piping

[Jeff King]

On Thu, Jun 13, 2019 at 02:51:03PM -0700, Emily Shaffer wrote:

> It didn't appear that using an existing --pretty string would do the
> trick, as the formatting options for --pretty apply specifically to
> commit objects (you can specify the commit hash and the tree hash, but
> I didn't see a way to more generally pretty-print all types of objects).

Right, it would be a big change to start custom-formatting the
non-commits. I think this is a good step in the right direction.

> rev-list doesn't appear to use parse-options-h, so I added a new option
> as simply as I could see without breaking existing style; it seemed
> wrong to add a flag to the `struct rev_list_info` as the definition of
> struct and flag values alike are contained in bisect.h. There are a
> couple other options to rev-list which are stored as globals.

I think that's reasonable. This is definitely a rev-list option (not a
revision.c one). The interaction between bisect.h and rev-list is a bit
funny, but it's probably simpler not to try solving it here.

> +	if (arg_oid_only && revs.commit_format != CMIT_FMT_UNSPECIFIED)
> +		die(_("cannot combine --oid-only and --pretty or --header"));

As you've implemented it here, I definitely agree that this conflicts
with --pretty. But I think it also interacts badly with other options,
like "--graph".

TBH, I am not sure that "rev-list --graph --objects" is all that useful,
because the non-commit objects are not prefixed with the graph lines.
And without "--objects", this new option is not useful because the
default is already to show only the oid.

But I wonder if things would be simpler if we did not touch the commit
code path at all. I.e., if this were simply "--no-object-names", and it
touched only show_object(). And then you do not have to worry about
funny interactions with commit formatting at all. It would be valid to
do:

  git rev-list --pretty=raw --objects --no-object-names HEAD

if you really wanted to (though I admit I do not have an immediate use
for it).

> @@ -255,6 +262,10 @@ static void show_object(struct object *obj, const char *name, void *cb_data)
>  	display_progress(progress, ++progress_counter);
>  	if (info->flags & REV_LIST_QUIET)
>  		return;
> +	if (arg_oid_only) {
> +		printf("%s\n", oid_to_hex(&obj->oid));
> +		return;
> +	}
>  	show_object_with_name(stdout, obj, name);
>  }
>  

A minor style point, but I think this might be easier to follow without
the early return, since we are really choosing to do A or B. Writing:

  if (arg_oid_only)
	printf(...);
  else
	show_object_with_name(...);

shows that more clearly, I think.

> [...]

Otherwise, this looks good, including the tests. One thing I did notice
in the tests:

> +test_expect_success 'rev-list --objects --oid-only is usable by cat-file' '
> +	git rev-list --objects --oid-only --all >list-output &&
> +	git cat-file --batch-check <list-output >cat-output &&
> +	! grep missing cat-output
> +'

Usually we prefer to look for the expected output, rather than making
sure we did not find the unexpected. But I'm not sure if that might be
burdensome in this case (i.e., if there's a bunch of cruft coming out of
"rev-list" that would be annoying to match, and might even change as
people add more tests). So I'm OK with it either way.

-Peff

Re: [PATCH v2] rev-list: teach --oid-only to enable piping

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> But I wonder if things would be simpler if we did not touch the commit
> code path at all. I.e., if this were simply "--no-object-names", and it
> touched only show_object().

Yeah, that sounds more tempting.  And the refined code structure you
suggested ...

>> @@ -255,6 +262,10 @@ static void show_object(struct object *obj, const char *name, void *cb_data)
>>  	display_progress(progress, ++progress_counter);
>>  	if (info->flags & REV_LIST_QUIET)
>>  		return;
>> +	if (arg_oid_only) {
>> +		printf("%s\n", oid_to_hex(&obj->oid));
>> +		return;
>> +	}
>>  	show_object_with_name(stdout, obj, name);
>>  }
>>  
>
> A minor style point, but I think this might be easier to follow without
> the early return, since we are really choosing to do A or B. Writing:
>
>   if (arg_oid_only)
> 	printf(...);
>   else
> 	show_object_with_name(...);
>
> shows that more clearly, I think.

... is a good way to clearly show that intention, I would think.

Re: [PATCH v2] rev-list: teach --oid-only to enable piping

[Emily Shaffer]

On Fri, Jun 14, 2019 at 01:25:59PM -0700, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
> 
> > But I wonder if things would be simpler if we did not touch the commit
> > code path at all. I.e., if this were simply "--no-object-names", and it
> > touched only show_object().
> 
> Yeah, that sounds more tempting.  And the refined code structure you
> suggested ...
> 
> >> @@ -255,6 +262,10 @@ static void show_object(struct object *obj, const char *name, void *cb_data)
> >>  	display_progress(progress, ++progress_counter);
> >>  	if (info->flags & REV_LIST_QUIET)
> >>  		return;
> >> +	if (arg_oid_only) {
> >> +		printf("%s\n", oid_to_hex(&obj->oid));
> >> +		return;
> >> +	}
> >>  	show_object_with_name(stdout, obj, name);
> >>  }
> >>  
> >
> > A minor style point, but I think this might be easier to follow without
> > the early return, since we are really choosing to do A or B. Writing:
> >
> >   if (arg_oid_only)
> > 	printf(...);
> >   else
> > 	show_object_with_name(...);
> >
> > shows that more clearly, I think.
> 
> ... is a good way to clearly show that intention, I would think.

Sounds good. Thanks, both; I'll reroll that quickly today.

Re: [PATCH v2] rev-list: teach --oid-only to enable piping

[Emily Shaffer]

On Fri, Jun 14, 2019 at 12:07:28PM -0400, Jeff King wrote:
> On Thu, Jun 13, 2019 at 02:51:03PM -0700, Emily Shaffer wrote:

> > +test_expect_success 'rev-list --objects --oid-only is usable by cat-file' '
> > +	git rev-list --objects --oid-only --all >list-output &&
> > +	git cat-file --batch-check <list-output >cat-output &&
> > +	! grep missing cat-output
> > +'
> 
> Usually we prefer to look for the expected output, rather than making
> sure we did not find the unexpected. But I'm not sure if that might be
> burdensome in this case (i.e., if there's a bunch of cruft coming out of
> "rev-list" that would be annoying to match, and might even change as
> people add more tests). So I'm OK with it either way.

My (newbie) opinion is that in this case, we specifically want to know
that cat-file didn't choke on objects which we know exist (since they
came from rev-list). I have the feeling that checking for the exact
objects returned instead (or a sample of them) would be more brittle and
would also make the wording of the test less direct.

So if there's no complaint either way, I'd prefer to leave it the way it
is.

By the way, rev-list-misc.sh has a number of other existing "! grep ..."
lines.

 - Emily

[PATCH v3] rev-list: teach --no-object-names to enable piping

[Emily Shaffer]

Allow easier parsing by cat-file by giving rev-list an option to print
only the OID of a non-commit object without any additional information.
This is a short-term shim; later on, rev-list should be taught how to
print the types of objects it finds in a format similar to cat-file's.

Before this commit, the output from rev-list needed to be massaged
before being piped to cat-file, like so:

  git rev-list --objects HEAD | cut -f 1 -d ' ' \
    | git cat-file --batch-check

This was especially unexpected when dealing with root trees, as an
invisible whitespace exists at the end of the OID:

  git rev-list --objects --filter=tree:1 --max-count=1 HEAD \
    | xargs -I% echo "AA%AA"

Now, it can be piped directly, as in the added test case:

  git rev-list --objects --no-object-names HEAD | git cat-file --batch-check

Signed-off-by: Emily Shaffer <emilyshaffer@google.com>
Change-Id: I489bdf0a8215532e540175188883ff7541d70e1b
---
Based on Peff and Junio's comments, made following changes since v2:

 - Removed interaction with commit objects
 - Renamed option to "no-object-names"
 - Removed warnings when new option is combined with commit-formatting
   options (and reflected in usage)
 - Simplified logic in show_object()

Thanks for the thoughts, all.

 - Emily

 builtin/rev-list.c       | 14 +++++++++++++-
 t/t6000-rev-list-misc.sh | 18 ++++++++++++++++++
 2 files changed, 31 insertions(+), 1 deletion(-)

diff --git a/builtin/rev-list.c b/builtin/rev-list.c
index 660172b014..7e2598fd22 100644
--- a/builtin/rev-list.c
+++ b/builtin/rev-list.c
@@ -49,6 +49,7 @@ static const char rev_list_usage[] =
 "    --objects | --objects-edge\n"
 "    --unpacked\n"
 "    --header | --pretty\n"
+"    --no-object-names\n"
 "    --abbrev=<n> | --no-abbrev\n"
 "    --abbrev-commit\n"
 "    --left-right\n"
@@ -75,6 +76,9 @@ enum missing_action {
 };
 static enum missing_action arg_missing_action;
 
+/* display only the oid of each object encountered */
+static int arg_no_object_names;
+
 #define DEFAULT_OIDSET_SIZE     (16*1024)
 
 static void finish_commit(struct commit *commit);
@@ -255,7 +259,10 @@ static void show_object(struct object *obj, const char *name, void *cb_data)
 	display_progress(progress, ++progress_counter);
 	if (info->flags & REV_LIST_QUIET)
 		return;
-	show_object_with_name(stdout, obj, name);
+	if (arg_no_object_names)
+		printf("%s\n", oid_to_hex(&obj->oid));
+	else
+		show_object_with_name(stdout, obj, name);
 }
 
 static void show_edge(struct commit *commit)
@@ -484,6 +491,11 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix)
 		if (skip_prefix(arg, "--missing=", &arg))
 			continue; /* already handled above */
 
+		if (!strcmp(arg, ("--no-object-names"))) {
+			arg_no_object_names = 1;
+			continue;
+		}
+
 		usage(rev_list_usage);
 
 	}
diff --git a/t/t6000-rev-list-misc.sh b/t/t6000-rev-list-misc.sh
index 0507999729..5d87171b99 100755
--- a/t/t6000-rev-list-misc.sh
+++ b/t/t6000-rev-list-misc.sh
@@ -48,6 +48,24 @@ test_expect_success 'rev-list --objects with pathspecs and copied files' '
 	! grep one output
 '
 
+test_expect_success 'rev-list --objects --no-object-names has no space/names' '
+	git rev-list --objects --no-object-names HEAD >output &&
+	! grep wanted_file output &&
+	! grep unwanted_file output &&
+	! grep " " output
+'
+
+test_expect_success 'rev-list --objects --no-object-names works with cat-file' '
+	git rev-list --objects --no-object-names --all >list-output &&
+	git cat-file --batch-check <list-output >cat-output &&
+	! grep missing cat-output
+'
+
+test_expect_success 'rev-list --oid-only is incompatible with --pretty' '
+	test_must_fail git rev-list --objects --oid-only --pretty HEAD &&
+	test_must_fail git rev-list --objects --oid-only --header HEAD
+'
+
 test_expect_success 'rev-list A..B and rev-list ^A B are the same' '
 	git commit --allow-empty -m another &&
 	git tag -a -m "annotated" v1.0 &&
-- 
2.22.0.410.gd8fdbe21b5-goog

Re: [PATCH v3] rev-list: teach --no-object-names to enable piping

[Junio C Hamano]

Emily Shaffer <emilyshaffer@google.com> writes:

> Allow easier parsing by cat-file by giving rev-list an option to print
> only the OID of a non-commit object without any additional information.
> This is a short-term shim; later on, rev-list should be taught how to
> print the types of objects it finds in a format similar to cat-file's.
>
> Before this commit, the output from rev-list needed to be massaged
> before being piped to cat-file, like so:
>
>   git rev-list --objects HEAD | cut -f 1 -d ' ' \
>     | git cat-file --batch-check

Write this with '|' at the end of the first line; that way the shell
knows you haven't finished your sentence without any backslash.

> diff --git a/builtin/rev-list.c b/builtin/rev-list.c
> index 660172b014..7e2598fd22 100644
> --- a/builtin/rev-list.c
> +++ b/builtin/rev-list.c
> @@ -49,6 +49,7 @@ static const char rev_list_usage[] =
>  "    --objects | --objects-edge\n"
>  "    --unpacked\n"
>  "    --header | --pretty\n"
> +"    --no-object-names\n"

Ideally, this should be "--[no-]object-names", i.e. --object-names
which may be the default when using --objects could be tweaked with
--no-object-names and then again turned on with a --object-names
later on the command line, following the usual "last one wins".

> @@ -75,6 +76,9 @@ enum missing_action {
>  };
>  static enum missing_action arg_missing_action;
>  
> +/* display only the oid of each object encountered */
> +static int arg_no_object_names;

And this would become

    static int show_object_names = 1;

that can be turned off via --no-object-names (and --object-names
would flip it on).  It would reduce the double negation brain
twister, hopefully.

Re: [PATCH v3] rev-list: teach --no-object-names to enable piping

[Emily Shaffer]

On Mon, Jun 17, 2019 at 03:32:34PM -0700, Junio C Hamano wrote:
> Emily Shaffer <emilyshaffer@google.com> writes:
> 
> > Allow easier parsing by cat-file by giving rev-list an option to print
> > only the OID of a non-commit object without any additional information.
> > This is a short-term shim; later on, rev-list should be taught how to
> > print the types of objects it finds in a format similar to cat-file's.
> >
> > Before this commit, the output from rev-list needed to be massaged
> > before being piped to cat-file, like so:
> >
> >   git rev-list --objects HEAD | cut -f 1 -d ' ' \
> >     | git cat-file --batch-check
> 
> Write this with '|' at the end of the first line; that way the shell
> knows you haven't finished your sentence without any backslash.

Oh cool. Will do.

> 
> > diff --git a/builtin/rev-list.c b/builtin/rev-list.c
> > index 660172b014..7e2598fd22 100644
> > --- a/builtin/rev-list.c
> > +++ b/builtin/rev-list.c
> > @@ -49,6 +49,7 @@ static const char rev_list_usage[] =
> >  "    --objects | --objects-edge\n"
> >  "    --unpacked\n"
> >  "    --header | --pretty\n"
> > +"    --no-object-names\n"
> 
> Ideally, this should be "--[no-]object-names", i.e. --object-names
> which may be the default when using --objects could be tweaked with
> --no-object-names and then again turned on with a --object-names
> later on the command line, following the usual "last one wins".

Sure, good point. Will fix.

> 
> > @@ -75,6 +76,9 @@ enum missing_action {
> >  };
> >  static enum missing_action arg_missing_action;
> >  
> > +/* display only the oid of each object encountered */
> > +static int arg_no_object_names;
> 
> And this would become
> 
>     static int show_object_names = 1;
> 
> that can be turned off via --no-object-names (and --object-names
> would flip it on).  It would reduce the double negation brain
> twister, hopefully.
> 

I'll send a new patch today. Thanks.

 - Emily

[PATCH v4] rev-list: teach --no-object-names to enable piping

[Emily Shaffer]

Allow easier parsing by cat-file by giving rev-list an option to print
only the OID of a non-commit object without any additional information.
This is a short-term shim; later on, rev-list should be taught how to
print the types of objects it finds in a format similar to cat-file's.

Before this commit, the output from rev-list needed to be massaged
before being piped to cat-file, like so:

  git rev-list --objects HEAD | cut -f 1 -d ' ' |
    git cat-file --batch-check

This was especially unexpected when dealing with root trees, as an
invisible whitespace exists at the end of the OID:

  git rev-list --objects --filter=tree:1 --max-count=1 HEAD |
    xargs -I% echo "AA%AA"

Now, it can be piped directly, as in the added test case:

  git rev-list --objects --no-object-names HEAD | git cat-file --batch-check

Signed-off-by: Emily Shaffer <emilyshaffer@google.com>
Change-Id: I489bdf0a8215532e540175188883ff7541d70e1b
---
Since v3, added a corresponding "--object-names" arg to pair with
"--no-object-names", and "last-one-wins" logic. Also added a test to
validate this new arg and the logic.

I did not take Junio's suggestion of naming the arg "show_object_names"
as "arg_show_object_names" better matches the existing style of
builtin/revlist.c.

In adding the test, I noticed that I had left in a test about --oid-only
that doesn't apply after the changes from v2->v3; that test is removed.

 - Emily

 builtin/rev-list.c       | 19 ++++++++++++++++++-
 t/t6000-rev-list-misc.sh | 20 ++++++++++++++++++++
 2 files changed, 38 insertions(+), 1 deletion(-)

diff --git a/builtin/rev-list.c b/builtin/rev-list.c
index 660172b014..301ccb970b 100644
--- a/builtin/rev-list.c
+++ b/builtin/rev-list.c
@@ -49,6 +49,7 @@ static const char rev_list_usage[] =
 "    --objects | --objects-edge\n"
 "    --unpacked\n"
 "    --header | --pretty\n"
+"    --[no-]object-names\n"
 "    --abbrev=<n> | --no-abbrev\n"
 "    --abbrev-commit\n"
 "    --left-right\n"
@@ -75,6 +76,9 @@ enum missing_action {
 };
 static enum missing_action arg_missing_action;
 
+/* display only the oid of each object encountered */
+static int arg_show_object_names = 1;
+
 #define DEFAULT_OIDSET_SIZE     (16*1024)
 
 static void finish_commit(struct commit *commit);
@@ -255,7 +259,10 @@ static void show_object(struct object *obj, const char *name, void *cb_data)
 	display_progress(progress, ++progress_counter);
 	if (info->flags & REV_LIST_QUIET)
 		return;
-	show_object_with_name(stdout, obj, name);
+	if (arg_show_object_names)
+		show_object_with_name(stdout, obj, name);
+	else
+		printf("%s\n", oid_to_hex(&obj->oid));
 }
 
 static void show_edge(struct commit *commit)
@@ -484,6 +491,16 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix)
 		if (skip_prefix(arg, "--missing=", &arg))
 			continue; /* already handled above */
 
+		if (!strcmp(arg, ("--no-object-names"))) {
+			arg_show_object_names = 0;
+			continue;
+		}
+
+		if (!strcmp(arg, ("--object-names"))) {
+			arg_show_object_names = 1;
+			continue;
+		}
+
 		usage(rev_list_usage);
 
 	}
diff --git a/t/t6000-rev-list-misc.sh b/t/t6000-rev-list-misc.sh
index 0507999729..52a9e38d66 100755
--- a/t/t6000-rev-list-misc.sh
+++ b/t/t6000-rev-list-misc.sh
@@ -48,6 +48,26 @@ test_expect_success 'rev-list --objects with pathspecs and copied files' '
 	! grep one output
 '
 
+test_expect_success 'rev-list --objects --no-object-names has no space/names' '
+	git rev-list --objects --no-object-names HEAD >output &&
+	! grep wanted_file output &&
+	! grep unwanted_file output &&
+	! grep " " output
+'
+
+test_expect_success 'rev-list --objects --no-object-names works with cat-file' '
+	git rev-list --objects --no-object-names --all >list-output &&
+	git cat-file --batch-check <list-output >cat-output &&
+	! grep missing cat-output
+'
+
+test_expect_success '--no-object-names and --object-names are last-one-wins' '
+	git rev-list --objects --no-object-names --object-names --all >output &&
+	grep wanted_file output &&
+	git rev-list --objects --object-names --no-object-names --all >output &&
+	! grep wanted_file output
+'
+
 test_expect_success 'rev-list A..B and rev-list ^A B are the same' '
 	git commit --allow-empty -m another &&
 	git tag -a -m "annotated" v1.0 &&
-- 
2.22.0.410.gd8fdbe21b5-goog

Re: [PATCH v4] rev-list: teach --no-object-names to enable piping

[Junio C Hamano]

Emily Shaffer <emilyshaffer@google.com> writes:

> Since v3, added a corresponding "--object-names" arg to pair with
> "--no-object-names", and "last-one-wins" logic. Also added a test to
> validate this new arg and the logic.

Thanks for a quick turnaround (unfortunately, I was OOO yesterday
and I am half-sick today, so please expect slow responses---sorry
about that).

> In adding the test, I noticed that I had left in a test about --oid-only
> that doesn't apply after the changes from v2->v3; that test is removed.

I noticed in range-diff, too.  So now --object-names can be used
with --pretty (not that "rev-list --pretty --objects" makes much
sense in the first place, so no point in testing that it works).

Re: [PATCH v4] rev-list: teach --no-object-names to enable piping

[Emily Shaffer]

On Wed, Jun 19, 2019 at 07:08:14AM -0700, Junio C Hamano wrote:
> Emily Shaffer <emilyshaffer@google.com> writes:
> 
> > Since v3, added a corresponding "--object-names" arg to pair with
> > "--no-object-names", and "last-one-wins" logic. Also added a test to
> > validate this new arg and the logic.
> 
> Thanks for a quick turnaround (unfortunately, I was OOO yesterday
> and I am half-sick today, so please expect slow responses---sorry
> about that).
> 
> > In adding the test, I noticed that I had left in a test about --oid-only
> > that doesn't apply after the changes from v2->v3; that test is removed.
> 
> I noticed in range-diff, too.  So now --object-names can be used
> with --pretty (not that "rev-list --pretty --objects" makes much
> sense in the first place, so no point in testing that it works).

Yeah, it works. It looks weird, but it works pretty much as you'd
expect:

emilyshaffer@podkayne:~/git-second [stray-whitespace]$ tg2 rev-list
--object-names --objects --pretty=short --max-count=1 HEAD | head -n 20
commit 701c66d5f2fafe163892fa0968ce8bca041dbc92
Author: Emily Shaffer <emilyshaffer@google.com>

    rev-list: teach --no-object-names to enable piping

    d4b1d372d16aaff35b221afce017f90542fd9293 
    41d4cd23fd97f599053a19555a173894da71e560 .clang-format
    42cdc4bbfb05934bb9c3ed2fe0e0d45212c32d7a .editorconfig

emilyshaffer@podkayne:~/git-second [stray-whitespace]$ tg2 rev-list
--no-object-names --objects --pretty=short --max-count=1 HEAD | head -n
20
commit 701c66d5f2fafe163892fa0968ce8bca041dbc92
Author: Emily Shaffer <emilyshaffer@google.com>

    rev-list: teach --no-object-names to enable piping

    d4b1d372d16aaff35b221afce017f90542fd9293
    41d4cd23fd97f599053a19555a173894da71e560
    42cdc4bbfb05934bb9c3ed2fe0e0d45212c32d7a

Ah, but when I was grabbing these samples, I noticed that I didn't
update the manpage for rev-list. So please wait while I reroll again...
sorry!

 - Emily

[PATCH v5] rev-list: teach --no-object-names to enable piping

[Emily Shaffer]

Allow easier parsing by cat-file by giving rev-list an option to print
only the OID of a non-commit object without any additional information.
This is a short-term shim; later on, rev-list should be taught how to
print the types of objects it finds in a format similar to cat-file's.

Before this commit, the output from rev-list needed to be massaged
before being piped to cat-file, like so:

  git rev-list --objects HEAD | cut -f 1 -d ' ' |
    git cat-file --batch-check

This was especially unexpected when dealing with root trees, as an
invisible whitespace exists at the end of the OID:

  git rev-list --objects --filter=tree:1 --max-count=1 HEAD |
    xargs -I% echo "AA%AA"

Now, it can be piped directly, as in the added test case:

  git rev-list --objects --no-object-names HEAD | git cat-file --batch-check

Signed-off-by: Emily Shaffer <emilyshaffer@google.com>
Change-Id: I489bdf0a8215532e540175188883ff7541d70e1b
---
Since v4, added the new options to `git help rev-list`.

 Documentation/git-rev-list.txt     |  1 +
 Documentation/rev-list-options.txt | 10 ++++++++++
 builtin/rev-list.c                 | 19 ++++++++++++++++++-
 t/t6000-rev-list-misc.sh           | 20 ++++++++++++++++++++
 4 files changed, 49 insertions(+), 1 deletion(-)

diff --git a/Documentation/git-rev-list.txt b/Documentation/git-rev-list.txt
index 88609ff435..9392760b25 100644
--- a/Documentation/git-rev-list.txt
+++ b/Documentation/git-rev-list.txt
@@ -48,6 +48,7 @@ SYNOPSIS
 	     [ --date=<format>]
 	     [ [ --objects | --objects-edge | --objects-edge-aggressive ]
 	       [ --unpacked ]
+	       [ --object-names | --no-object-names ]
 	       [ --filter=<filter-spec> [ --filter-print-omitted ] ] ]
 	     [ --missing=<missing-action> ]
 	     [ --pretty | --header ]
diff --git a/Documentation/rev-list-options.txt b/Documentation/rev-list-options.txt
index 71a1fcc093..286fc163f1 100644
--- a/Documentation/rev-list-options.txt
+++ b/Documentation/rev-list-options.txt
@@ -708,6 +708,16 @@ ifdef::git-rev-list[]
 	Only useful with `--objects`; print the object IDs that are not
 	in packs.
 
+--object-names::
+	Only useful with `--objects`; print the names of the object IDs
+	that are found. This is the default behavior.
+
+--no-object-names::
+	Only useful with `--objects`; does not print the names of the object
+	IDs that are found. This inverts `--object-names`. This flag allows
+	the output to be more easily parsed by commands such as
+	linkgit:git-cat-file[1].
+
 --filter=<filter-spec>::
 	Only useful with one of the `--objects*`; omits objects (usually
 	blobs) from the list of printed objects.  The '<filter-spec>'
diff --git a/builtin/rev-list.c b/builtin/rev-list.c
index 660172b014..301ccb970b 100644
--- a/builtin/rev-list.c
+++ b/builtin/rev-list.c
@@ -49,6 +49,7 @@ static const char rev_list_usage[] =
 "    --objects | --objects-edge\n"
 "    --unpacked\n"
 "    --header | --pretty\n"
+"    --[no-]object-names\n"
 "    --abbrev=<n> | --no-abbrev\n"
 "    --abbrev-commit\n"
 "    --left-right\n"
@@ -75,6 +76,9 @@ enum missing_action {
 };
 static enum missing_action arg_missing_action;
 
+/* display only the oid of each object encountered */
+static int arg_show_object_names = 1;
+
 #define DEFAULT_OIDSET_SIZE     (16*1024)
 
 static void finish_commit(struct commit *commit);
@@ -255,7 +259,10 @@ static void show_object(struct object *obj, const char *name, void *cb_data)
 	display_progress(progress, ++progress_counter);
 	if (info->flags & REV_LIST_QUIET)
 		return;
-	show_object_with_name(stdout, obj, name);
+	if (arg_show_object_names)
+		show_object_with_name(stdout, obj, name);
+	else
+		printf("%s\n", oid_to_hex(&obj->oid));
 }
 
 static void show_edge(struct commit *commit)
@@ -484,6 +491,16 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix)
 		if (skip_prefix(arg, "--missing=", &arg))
 			continue; /* already handled above */
 
+		if (!strcmp(arg, ("--no-object-names"))) {
+			arg_show_object_names = 0;
+			continue;
+		}
+
+		if (!strcmp(arg, ("--object-names"))) {
+			arg_show_object_names = 1;
+			continue;
+		}
+
 		usage(rev_list_usage);
 
 	}
diff --git a/t/t6000-rev-list-misc.sh b/t/t6000-rev-list-misc.sh
index 0507999729..52a9e38d66 100755
--- a/t/t6000-rev-list-misc.sh
+++ b/t/t6000-rev-list-misc.sh
@@ -48,6 +48,26 @@ test_expect_success 'rev-list --objects with pathspecs and copied files' '
 	! grep one output
 '
 
+test_expect_success 'rev-list --objects --no-object-names has no space/names' '
+	git rev-list --objects --no-object-names HEAD >output &&
+	! grep wanted_file output &&
+	! grep unwanted_file output &&
+	! grep " " output
+'
+
+test_expect_success 'rev-list --objects --no-object-names works with cat-file' '
+	git rev-list --objects --no-object-names --all >list-output &&
+	git cat-file --batch-check <list-output >cat-output &&
+	! grep missing cat-output
+'
+
+test_expect_success '--no-object-names and --object-names are last-one-wins' '
+	git rev-list --objects --no-object-names --object-names --all >output &&
+	grep wanted_file output &&
+	git rev-list --objects --object-names --no-object-names --all >output &&
+	! grep wanted_file output
+'
+
 test_expect_success 'rev-list A..B and rev-list ^A B are the same' '
 	git commit --allow-empty -m another &&
 	git tag -a -m "annotated" v1.0 &&
-- 
2.22.0.410.gd8fdbe21b5-goog

Re: [PATCH v2] rev-list: teach --oid-only to enable piping

[Jeff King]

On Fri, Jun 14, 2019 at 04:29:46PM -0700, Emily Shaffer wrote:

> On Fri, Jun 14, 2019 at 12:07:28PM -0400, Jeff King wrote:
> > On Thu, Jun 13, 2019 at 02:51:03PM -0700, Emily Shaffer wrote:
> 
> > > +test_expect_success 'rev-list --objects --oid-only is usable by cat-file' '
> > > +	git rev-list --objects --oid-only --all >list-output &&
> > > +	git cat-file --batch-check <list-output >cat-output &&
> > > +	! grep missing cat-output
> > > +'
> > 
> > Usually we prefer to look for the expected output, rather than making
> > sure we did not find the unexpected. But I'm not sure if that might be
> > burdensome in this case (i.e., if there's a bunch of cruft coming out of
> > "rev-list" that would be annoying to match, and might even change as
> > people add more tests). So I'm OK with it either way.
> 
> My (newbie) opinion is that in this case, we specifically want to know
> that cat-file didn't choke on objects which we know exist (since they
> came from rev-list). I have the feeling that checking for the exact
> objects returned instead (or a sample of them) would be more brittle and
> would also make the wording of the test less direct.
> 
> So if there's no complaint either way, I'd prefer to leave it the way it
> is.

Yeah, that's fine with me if it seems more clear to use grep here (and I
was on the fence).

> By the way, rev-list-misc.sh has a number of other existing "! grep ..."
> lines.

It never fails that when I complain about a style issue, the surrounding
code is full of the same thing. ;) I'd have to look at each one to
determine if they're sensible or not, and it's probably not worth
anybody's time to do that cleanup at this point in time.

-Peff

Re: [PATCH v4] rev-list: teach --no-object-names to enable piping

[Jeff King]

On Wed, Jun 19, 2019 at 12:31:34PM -0700, Emily Shaffer wrote:

> > I noticed in range-diff, too.  So now --object-names can be used
> > with --pretty (not that "rev-list --pretty --objects" makes much
> > sense in the first place, so no point in testing that it works).
> 
> Yeah, it works. It looks weird, but it works pretty much as you'd
> expect:

I think that something like:

  git rev-list --in-commit-order --oneline --objects HEAD

could make sense to get a rough idea of which commits are mentioning
which objects (though in most cases I'd probably use "log --find-object"
these days, since it's more precise; rev-list is looking in a funny
reverse order).

-Peff

Re: [PATCH v5] rev-list: teach --no-object-names to enable piping

[Jeff King]

On Wed, Jun 19, 2019 at 01:56:56PM -0700, Emily Shaffer wrote:

> Allow easier parsing by cat-file by giving rev-list an option to print
> only the OID of a non-commit object without any additional information.
> This is a short-term shim; later on, rev-list should be taught how to
> print the types of objects it finds in a format similar to cat-file's.
> [...]

I missed some of the intermediate rounds, but fortunately Junio already
said everything I was going to. :) This version looks good to me, though
with one minor nit:

> diff --git a/Documentation/git-rev-list.txt b/Documentation/git-rev-list.txt
> index 88609ff435..9392760b25 100644
> --- a/Documentation/git-rev-list.txt
> +++ b/Documentation/git-rev-list.txt
> @@ -48,6 +48,7 @@ SYNOPSIS
>  	     [ --date=<format>]
>  	     [ [ --objects | --objects-edge | --objects-edge-aggressive ]
>  	       [ --unpacked ]
> +	       [ --object-names | --no-object-names ]
>  	       [ --filter=<filter-spec> [ --filter-print-omitted ] ] ]
>  	     [ --missing=<missing-action> ]
>  	     [ --pretty | --header ]

Here you put --object-names along with the --objects. Which kind of
makes sense, but everything else in that block is about choosing _which_
commits to show. In the short help, you put it near --pretty:

> @@ -49,6 +49,7 @@ static const char rev_list_usage[] =
>  "    --objects | --objects-edge\n"
>  "    --unpacked\n"
>  "    --header | --pretty\n"
> +"    --[no-]object-names\n"
>  "    --abbrev=<n> | --no-abbrev\n"
>  "    --abbrev-commit\n"
>  "    --left-right\n"

which I think makes more sense. I think maybe you were trying to imply
that "--object-names" is not useful unless you're also using
"--objects". Which is true, but I'm not sure it's obvious from that mass
of brackets (and I think is sufficiently covered in the actual option
descriptions you give later).

> +test_expect_success '--no-object-names and --object-names are last-one-wins' '
> +	git rev-list --objects --no-object-names --object-names --all >output &&
> +	grep wanted_file output &&
> +	git rev-list --objects --object-names --no-object-names --all >output &&
> +	! grep wanted_file output
> +'

We don't generally test this behavior for each option, since it would
lead to a ton of uninteresting tests (and parse-options generally just
handles it).  But after our discussion about --no-abbrev, I can see how
you might be more interested in the topic. :) So I'm OK with it either
way.

-Peff