Re: [PATCH 7/7] commit-graph.c: introduce '--[no-]check-oids'

["SZEDER Gábor" <szeder.dev@gmail.com>]

On Tue, Apr 14, 2020 at 10:31:37PM -0600, Taylor Blau wrote:
> On Tue, Apr 14, 2020 at 10:29:30PM -0600, Taylor Blau wrote:
> > Whoops. I sent the wrong version of this patch. It should be the below:
> 
> Double whoops. I was on the wrong branch, and hit send too early. *This*
> is the version of the patch that I meant to send ;).
> 
> --- >8 ---
> 
> Subject: [PATCH] commit-graph.c: introduce '--[no-]check-oids'
> 
> When operating on a stream of commit OIDs on stdin, 'git commit-graph
> write' checks that each OID refers to an object that is indeed a commit.
> This is convenient to make sure that the given input is well-formed, but
> can sometimes be undesirable.
> 
> For example, server operators may wish to feed the refnames that were

s/the refnames/full commit object IDs pointed to by refs/

or something similar.

> updated during a push to 'git commit-graph write --input=stdin-commits',
> and silently discard refs that don't point at commits.

s/refs/<something along the lines of the above>/

> This can be done
> by combing the output of 'git for-each-ref' with '--format
> %(*objecttype)', but this requires opening up a potentially large number
> of objects.  Instead, it is more convenient to feed the updated refs to

s/refs/.../

> the commit-graph machinery, and let it throw out refs that don't point

s/refs/.../

> to commits.
> 
> Introduce '--[no-]check-oids' to make such a behavior possible. With
> '--check-oids' (the default behavior to retain backwards compatibility),
> 'git commit-graph write' will barf on a non-commit line in its input.
> With 'no-check-oids', such lines will be silently ignored, making the

s/no-check-oids/--no-check-oids/

> above possible by specifying this option.
> 
> No matter which is supplied, 'git commit-graph write' retains the
> behavior from the previous commit of rejecting non-OID inputs like
> "HEAD" and "refs/heads/foo" as before.

See? :)  This is why all those s/// are necessary.

> Signed-off-by: Taylor Blau <me@ttaylorr.com>
> ---
>  Documentation/git-commit-graph.txt |  5 +++++
>  builtin/commit-graph.c             | 11 ++++++++---
>  commit-graph.c                     |  2 +-
>  t/t5318-commit-graph.sh            | 28 ++++++++++++++++++++++++++++
>  4 files changed, 42 insertions(+), 4 deletions(-)
> 
> diff --git a/Documentation/git-commit-graph.txt b/Documentation/git-commit-graph.txt
> index 46f7f7c573..91e8027b86 100644
> --- a/Documentation/git-commit-graph.txt
> +++ b/Documentation/git-commit-graph.txt
> @@ -82,6 +82,11 @@ tip with the previous tip.
>  Finally, if `--expire-time=<datetime>` is not specified, let `datetime`
>  be the current time. After writing the split commit-graph, delete all
>  unused commit-graph whose modified times are older than `datetime`.
> ++
> +The `--[no-]check-oids` option decides whether or not OIDs are required
> +to be commits. By default, `--check-oids` is implied, generating an
> +error on non-commit objects. If `--no-check-oids` is given, non-commits
> +are silently discarded.

What happens with OIDs of tags, in particular with OIDs of tags that
can be peeled down to commit objects?  According to (my (too
pedantic?) interpretation of) the above description they will trigger
an error with '--check-oids' or will be ignored with
'--no-check-oids'.  The implementation, however, accepts those oids
and peels them down to commit objects; I think this is the right
behaviour.

What happens with OIDs that name non-existing objects?

>  'verify'::
> 
> diff --git a/builtin/commit-graph.c b/builtin/commit-graph.c
> index c69716aa7e..2d0a8e822a 100644
> --- a/builtin/commit-graph.c
> +++ b/builtin/commit-graph.c
> @@ -11,7 +11,7 @@ static char const * const builtin_commit_graph_usage[] = {
>  	N_("git commit-graph verify [--object-dir <objdir>] [--shallow] [--[no-]progress]"),
>  	N_("git commit-graph write [--object-dir <objdir>] [--append] "
>  	   "[--split[=<strategy>]] [--reachable|--stdin-packs|--stdin-commits] "
> -	   "[--[no-]progress] <split options>"),
> +	   "[--[no-]progress] [--[no-]check-oids] <split options>"),
>  	NULL
>  };
> 
> @@ -23,7 +23,7 @@ static const char * const builtin_commit_graph_verify_usage[] = {
>  static const char * const builtin_commit_graph_write_usage[] = {
>  	N_("git commit-graph write [--object-dir <objdir>] [--append] "
>  	   "[--split[=<strategy>]] [--reachable|--stdin-packs|--stdin-commits] "
> -	   "[--[no-]progress] <split options>"),
> +	   "[--[no-]progress] [--[no-]check-oids] <split options>"),
>  	NULL
>  };
> 
> @@ -36,6 +36,7 @@ static struct opts_commit_graph {
>  	int split;
>  	int shallow;
>  	int progress;
> +	int check_oids;
>  } opts;
> 
>  static struct object_directory *find_odb(struct repository *r,
> @@ -163,6 +164,8 @@ static int graph_write(int argc, const char **argv)
>  			N_("allow writing an incremental commit-graph file"),
>  			PARSE_OPT_OPTARG | PARSE_OPT_NONEG,
>  			write_option_parse_split),
> +		OPT_BOOL(0, "check-oids", &opts.check_oids,
> +			N_("require OIDs to be commits")),
>  		OPT_INTEGER(0, "max-commits", &split_opts.max_commits,
>  			N_("maximum number of commits in a non-base split commit-graph")),
>  		OPT_INTEGER(0, "size-multiple", &split_opts.size_multiple,
> @@ -173,6 +176,7 @@ static int graph_write(int argc, const char **argv)
>  	};
> 
>  	opts.progress = isatty(2);
> +	opts.check_oids = 1;
>  	split_opts.size_multiple = 2;
>  	split_opts.max_commits = 0;
>  	split_opts.expire_time = 0;
> @@ -227,7 +231,8 @@ static int graph_write(int argc, const char **argv)
> 
>  				oidset_insert(&commits, &oid);
>  			}
> -			flags |= COMMIT_GRAPH_WRITE_CHECK_OIDS;
> +			if (opts.check_oids)
> +				flags |= COMMIT_GRAPH_WRITE_CHECK_OIDS;
>  		}
> 
>  		UNLEAK(buf);
> diff --git a/commit-graph.c b/commit-graph.c
> index f60346baee..b8737f0ce9 100644
> --- a/commit-graph.c
> +++ b/commit-graph.c
> @@ -145,7 +145,7 @@ static int verify_commit_graph_lite(struct commit_graph *g)
>  	 *
>  	 * There should only be very basic checks here to ensure that
>  	 * we don't e.g. segfault in fill_commit_in_graph(), but
> -	 * because this is a very hot codepath nothing that e.g. loops
> +	 e because this is a very hot codepath nothing that e.g. loops

Bogus hunk, perhaps?

>  	 * over g->num_commits, or runs a checksum on the commit-graph
>  	 * itself.
>  	 */
> diff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh
> index e874a12696..7960cefa1b 100755
> --- a/t/t5318-commit-graph.sh
> +++ b/t/t5318-commit-graph.sh
> @@ -49,6 +49,34 @@ test_expect_success 'exit with correct error on bad input to --stdin-commits' '
>  	test_i18ngrep "invalid commit object id" stderr
>  '
> 
> +graph_expect_commits() {
> +	test-tool read-graph >got
> +	if ! grep "num_commits: $1" got
> +	then
> +		echo "graph_expect_commits: expected $1 commit(s), got:"
> +		cat got
> +		false
> +	fi
> +}
> +
> +test_expect_success 'ignores non-commit OIDs to --input=stdin-commits with --no-check-oids' '
> +	test_when_finished rm -rf "$objdir/info/commit-graph" &&
> +	cd "$TRASH_DIRECTORY/full" &&
> +	# write a graph to ensure layers are/are not added appropriately
> +	git rev-parse HEAD~1 >base &&
> +	git commit-graph write --stdin-commits <base &&
> +	graph_expect_commits 2 &&
> +	# bad input is rejected
> +	echo HEAD >bad &&
> +	test_expect_code 1 git commit-graph write --stdin-commits <bad 2>err &&
> +	test_i18ngrep "unexpected non-hex object ID: HEAD" err &&
> +	graph_expect_commits 2 &&
> +	# update with valid commit OID, ignore tree OID
> +	git rev-parse HEAD HEAD^{tree} >in &&
> +	git commit-graph write --stdin-commits --no-check-oids <in &&
> +	graph_expect_commits 3
> +'
> +
>  graph_git_two_modes() {
>  	git -c core.commitGraph=true $1 >output
>  	git -c core.commitGraph=false $1 >expect
> --
> 2.26.0.106.g9fadedd637
>