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

[Jeff King <peff@peff.net>]

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