Subject: [PATCH v3 0/8] commit-graph: drop CHECK_OIDS, peel in callers

From: Taylor Blau <[email-masked]>
Date: Wed, 13 May 2020 15:59:25 -0600

Hi,

Here is a tiny reroll to my series to drop the commit-graph's behavior
of complaining about non-commit OIDs on input with '--stdin-commits'. I
promised this reroll after Peff's review beginning in [1].

Not a great deal has changed, but I mostly took every suggestion that
Peff put forward (in the sub-thread beginning at [1]). The "big deals"
since last time are:

  - 'add_ref_to_set()' got cleaned up to use 'oid_object_info'.

  - progress meter updates moved after their action

  - 'graph_write()' got cleaned up to break the loop into two loops
    executed conditionally (based on which of '--stdin-packs' or
    '--stdin-commits' was passed) , as well as a cleanup label was
    introduced and some UNLEAK cruft was cleaned up.

For convenience, a range-diff since v2 (with the changes on master
applied since then elided out) is included below.

Thanks again for your review.

-Taylor

[1]: https://lore.kernel.org/git/[email-masked]/

Taylor Blau (8):
  commit-graph.c: extract 'refs_cb_data'
  commit-graph.c: show progress of finding reachable commits
  commit-graph.c: peel refs in 'add_ref_to_set'
  builtin/commit-graph.c: extract 'read_one_commit()'
  builtin/commit-graph.c: dereference tags in builtin
  commit-graph.c: simplify 'fill_oids_from_commits'
  t5318: reorder test below 'graph_read_expect'
  commit-graph: drop COMMIT_GRAPH_WRITE_CHECK_OIDS flag

 Documentation/git-commit-graph.txt |  6 ++-
 builtin/commit-graph.c             | 73 +++++++++++++++++++-----------
 commit-graph.c                     | 62 +++++++++++--------------
 commit-graph.h                     |  4 +-
 t/t5318-commit-graph.sh            | 25 ++++++----
 5 files changed, 95 insertions(+), 75 deletions(-)

Range-diff against v2:
 1:  43286c3c45 = 60:  0efbcfcf3a commit-graph.c: extract 'refs_cb_data'
 2:  cb56a9a73b ! 61:  773522c745 commit-graph.c: show progress of finding reachable commits
    @@ commit-graph.c: static void compute_bloom_filters(struct write_commit_graph_cont

      static int add_ref_to_set(const char *refname,
     @@ commit-graph.c: static int add_ref_to_set(const char *refname,
    - {
      	struct refs_cb_data *data = (struct refs_cb_data *)cb_data;

    -+	display_progress(data->progress, oidset_size(data->commits) + 1);
    -+
      	oidset_insert(data->commits, oid);
    ++
    ++	display_progress(data->progress, oidset_size(data->commits));
    ++
      	return 0;
      }
    +
     @@ commit-graph.c: int write_commit_graph_reachable(struct object_directory *odb,

      	memset(&data, 0, sizeof(data));
 3:  85c388a077 <  -:  ---------- commit-graph.c: peel refs in 'add_ref_to_set'
 -:  ---------- > 62:  74b424f1ac commit-graph.c: peel refs in 'add_ref_to_set'
 4:  cef441b465 ! 63:  c37e94907b builtin/commit-graph.c: extract 'read_one_commit()'
    @@ builtin/commit-graph.c: static int write_option_parse_split(const struct option
      	return 0;
      }

    -+static int read_one_commit(struct oidset *commits, char *hash)
    ++static int read_one_commit(struct oidset *commits, const char *hash)
     +{
     +	struct object_id oid;
     +	const char *end;
     +
    -+	if (parse_oid_hex(hash, &oid, &end)) {
    -+		error(_("unexpected non-hex object ID: %s"), hash);
    -+		return 1;
    -+	}
    ++	if (parse_oid_hex(hash, &oid, &end))
    ++		return error(_("unexpected non-hex object ID: %s"), hash);
     +
     +	oidset_insert(commits, &oid);
     +	return 0;
    @@ builtin/commit-graph.c: static int graph_write(int argc, const char **argv)
      	}

     -	string_list_init(&lines, 0);
    -+	string_list_init(&pack_indexes, 0);
    - 	if (opts.stdin_packs || opts.stdin_commits) {
    - 		struct strbuf buf = STRBUF_INIT;
    --
    --		while (strbuf_getline(&buf, stdin) != EOF)
    +-	if (opts.stdin_packs || opts.stdin_commits) {
    +-		struct strbuf buf = STRBUF_INIT;
    ++	struct strbuf buf = STRBUF_INIT;
    ++	if (opts.stdin_packs) {
    ++		string_list_init(&pack_indexes, 0);
    +
    + 		while (strbuf_getline(&buf, stdin) != EOF)
     -			string_list_append(&lines, strbuf_detach(&buf, NULL));
    --
    ++			string_list_append(&pack_indexes,
    ++					   strbuf_detach(&buf, NULL));
    ++	} else if (opts.stdin_commits) {
    ++		oidset_init(&commits, 0);
    ++		flags |= COMMIT_GRAPH_WRITE_CHECK_OIDS;
    +
     -		if (opts.stdin_packs)
     -			pack_indexes = &lines;
    - 		if (opts.stdin_commits) {
    +-		if (opts.stdin_commits) {
     -			struct string_list_item *item;
     -			oidset_init(&commits, lines.nr);
     -			for_each_string_list_item(item, &lines) {
    @@ builtin/commit-graph.c: static int graph_write(int argc, const char **argv)
     -				}
     -
     -				oidset_insert(&commits, &oid);
    --			}
    -+			oidset_init(&commits, 0);
    - 			flags |= COMMIT_GRAPH_WRITE_CHECK_OIDS;
    - 		}
    -
     +		while (strbuf_getline(&buf, stdin) != EOF) {
    -+			char *line = strbuf_detach(&buf, NULL);
    -+			if (opts.stdin_commits) {
    -+				int result = read_one_commit(&commits, line);
    -+				if (result)
    -+					return result;
    -+			} else
    -+				string_list_append(&pack_indexes, line);
    -+		}
    -+
    - 		UNLEAK(buf);
    ++			if (read_one_commit(&commits, buf.buf)) {
    ++				result = 1;
    ++				goto cleanup;
    + 			}
    +-			flags |= COMMIT_GRAPH_WRITE_CHECK_OIDS;
    + 		}
    +-
    +-		UNLEAK(buf);
      	}

      	if (write_commit_graph(odb,
    @@ builtin/commit-graph.c: static int graph_write(int argc, const char **argv)
      		result = 1;

     -	UNLEAK(lines);
    ++cleanup:
     +	UNLEAK(pack_indexes);
    ++	strbuf_release(&buf);
      	return result;
      }

 5:  d449d83ce2 <  -:  ---------- builtin/commit-graph.c: dereference tags in builtin
 -:  ---------- > 64:  56403dd377 builtin/commit-graph.c: dereference tags in builtin
 6:  61887870c7 = 65:  6afb08a927 commit-graph.c: simplify 'fill_oids_from_commits'
 7:  e393b16097 = 66:  8fdd2792df t5318: reorder test below 'graph_read_expect'
 8:  ad373f05ff ! 67:  3cb0bd306c commit-graph: drop COMMIT_GRAPH_WRITE_CHECK_OIDS flag
    @@ Commit message
         If callers do wish to retain this behavior, they can easily work around
         this change by doing the following:

    -        git for-each-ref --format='%(objectname) %(objecttype) %(*objecttype)' |
    -        awk '/commit/ { print $1 }' |
    -        git commit-graph write --stdin-commits
    +         git for-each-ref --format='%(objectname) %(objecttype) %(*objecttype)' |
    +         awk '
    +           !/commit/ { print "not-a-commit:"$1 }
    +            /commit/ { print $1 }
    +         ' |
    +         git commit-graph write --stdin-commits

         To make it so that valid OIDs that refer to non-existent objects are
         indeed an error after loosening the error handling, perform an extra
         lookup to make sure that object indeed exists before sending it to the
         commit-graph internals.

    +    Helped-by: Jeff King <[email-masked]>
         Signed-off-by: Taylor Blau <[email-masked]>

      ## Documentation/git-commit-graph.txt ##
    @@ Documentation/git-commit-graph.txt: with `--stdin-commits` or `--reachable`.)
      commits starting at all refs. (Cannot be combined with `--stdin-commits`

      ## builtin/commit-graph.c ##
    -@@ builtin/commit-graph.c: static int read_one_commit(struct oidset *commits, struct progress *progress,
    +@@
    + #include "commit-graph.h"
    + #include "object-store.h"
    + #include "progress.h"
    ++#include "tag.h"

    - 	display_progress(progress, oidset_size(commits) + 1);
    + static char const * const builtin_commit_graph_usage[] = {
    + 	N_("git commit-graph verify [--object-dir <objdir>] [--shallow] [--[no-]progress]"),
    +@@ builtin/commit-graph.c: static int write_option_parse_split(const struct option *opt, const char *arg,
    + static int read_one_commit(struct oidset *commits, struct progress *progress,
    + 			   const char *hash)
    + {
    +-	struct commit *result;
    ++	struct object *result;
    + 	struct object_id oid;
    + 	const char *end;

    -+	if (oid_object_info(the_repository, &oid, NULL) < 0) {
    -+		error(_("object %s does not exist"), hash);
    -+		return 1;
    -+	}
    -+
    - 	result = lookup_commit_reference_gently(the_repository, &oid, 1);
    - 	if (result)
    - 		oidset_insert(commits, &result->object.oid);
    --	else {
    --		error(_("invalid commit object id: %s"), hash);
    --		return 1;
    --	}
    - 	return 0;
    - }
    + 	if (parse_oid_hex(hash, &oid, &end))
    + 		return error(_("unexpected non-hex object ID: %s"), hash);
    +
    +-	result = lookup_commit_reference_gently(the_repository, &oid, 1);
    +-	if (result)
    +-		oidset_insert(commits, &result->object.oid);
    +-	else
    +-		return error(_("invalid commit object id: %s"), hash);
    ++	result = deref_tag(the_repository, parse_object(the_repository, &oid),
    ++			   NULL, 0);
    ++	if (!result)
    ++		return error(_("invalid object: %s"), hash);
    ++	else if (object_as_type(the_repository, result, OBJ_COMMIT, 1))
    ++		oidset_insert(commits, &result->oid);
    +
    + 	display_progress(progress, oidset_size(commits));

     @@ builtin/commit-graph.c: static int graph_write(int argc, const char **argv)
    - 		struct strbuf buf = STRBUF_INIT;
    - 		if (opts.stdin_commits) {
    - 			oidset_init(&commits, 0);
    --			flags |= COMMIT_GRAPH_WRITE_CHECK_OIDS;
    - 			if (opts.progress)
    - 				progress = start_delayed_progress(
    - 					_("Collecting commits from input"), 0);
    + 					   strbuf_detach(&buf, NULL));
    + 	} else if (opts.stdin_commits) {
    + 		oidset_init(&commits, 0);
    +-		flags |= COMMIT_GRAPH_WRITE_CHECK_OIDS;
    + 		if (opts.progress)
    + 			progress = start_delayed_progress(
    + 				_("Collecting commits from input"), 0);

      ## commit-graph.c ##
     @@ commit-graph.c: struct write_commit_graph_context {
    @@ commit-graph.c: int write_commit_graph(struct object_directory *odb,
      	ctx->total_bloom_filter_data_size = 0;

      ## commit-graph.h ##
    -@@ commit-graph.h: struct commit_graph *parse_commit_graph(void *graph_map, size_t graph_size);
    - int generation_numbers_enabled(struct repository *r);
    -
    - enum commit_graph_write_flags {
    --	COMMIT_GRAPH_WRITE_APPEND     = (1 << 0),
    --	COMMIT_GRAPH_WRITE_PROGRESS   = (1 << 1),
    --	COMMIT_GRAPH_WRITE_SPLIT      = (1 << 2),
    +@@ commit-graph.h: enum commit_graph_write_flags {
    + 	COMMIT_GRAPH_WRITE_APPEND     = (1 << 0),
    + 	COMMIT_GRAPH_WRITE_PROGRESS   = (1 << 1),
    + 	COMMIT_GRAPH_WRITE_SPLIT      = (1 << 2),
     -	/* Make sure that each OID in the input is a valid commit OID. */
     -	COMMIT_GRAPH_WRITE_CHECK_OIDS = (1 << 3),
     -	COMMIT_GRAPH_WRITE_BLOOM_FILTERS = (1 << 4),
    -+	COMMIT_GRAPH_WRITE_APPEND        = (1 << 0),
    -+	COMMIT_GRAPH_WRITE_PROGRESS      = (1 << 1),
    -+	COMMIT_GRAPH_WRITE_SPLIT         = (1 << 2),
    -+	COMMIT_GRAPH_WRITE_BLOOM_FILTERS = (1 << 3)
    ++	COMMIT_GRAPH_WRITE_BLOOM_FILTERS = (1 << 3),
      };

      enum commit_graph_split_flags {
    @@ t/t5318-commit-graph.sh: graph_read_expect() {
     +	# non-existent OID
     +	echo $ZERO_OID >in &&
     +	test_expect_code 1 git commit-graph write --stdin-commits <in 2>stderr &&
    -+	test_i18ngrep "does not exist" stderr &&
    ++	test_i18ngrep "invalid object" stderr &&
     +	# valid commit and tree OID
     +	git rev-parse HEAD HEAD^{tree} >in &&
     +	git commit-graph write --stdin-commits <in &&
--
2.26.0.113.ge9739cdccc