[PATCH v2 0/1] name-rev: use generation numbers if available

[Jacob Keller <jacob.e.keller@intel.com>]

From: Jacob Keller <jacob.keller@gmail.com>

Thanks for the review, Stolee!

Here's the range-diff since v1:

1:  e7e302376dd6 ! 1:  8ab5d987bf93 name-rev: use generation numbers if available
    @@ Commit message
         This heuristic impacts git name-rev, and by extension git describe
         --contains which is built on top of name-rev.
     
    -    Further more, if --annotate-stdin is used, the heuristic is not enabled
    -    because the full history has to be analyzed anyways. This results in
    -    some confusion if a user sees that --annotate-stdin works but a normal
    -    name-rev does not.
    +    Further more, if --all or --annotate-stdin is used, the heuristic is not
    +    enabled because the full history has to be analyzed anyways. This
    +    results in some confusion if a user sees that --annotate-stdin works but
    +    a normal name-rev does not.
     
         If the repository has a commit graph, we can use the generation numbers
         instead of using the commit dates. This is essentially the same check
         except that generation numbers make it exact, where the commit date
         heuristic could be incorrect due to clock errors.
     
    -    Add a test case which covers this behavior and shows how the commit
    -    graph makes the name-rev process work.
    +    Since we're extending the notion of cutoff to more than one variable,
    +    create a series of functions for setting and checking the cutoff. This
    +    avoids duplication and moves access of the global cutoff and
    +    generation_cutoff to as few functions as possible.
    +
    +    Add several test cases including coverage of --all, --annotate-stdin,
    +    and the normal case with the cutoff heuristic, both with and without
    +    commit graphs enabled.
     
         Signed-off-by: Jacob Keller <jacob.keller@gmail.com>
     
    @@ builtin/name-rev.c: struct rev_name {
      static timestamp_t cutoff = TIME_MAX;
      static struct commit_rev_name rev_names;
      
    ++/* Disable the cutoff checks entirely */
    ++static void disable_cutoff(void)
    ++{
    ++	generation_cutoff = 0;
    ++	cutoff = 0;
    ++}
    ++
    ++/* Cutoff searching any commits older than this one */
     +static void set_commit_cutoff(struct commit *commit)
     +{
    -+	timestamp_t generation;
     +
     +	if (cutoff > commit->date)
     +		cutoff = commit->date;
     +
    -+	generation = commit_graph_generation(commit);
    -+	if (generation_cutoff > generation)
    -+		generation_cutoff = generation;
    ++	if (generation_cutoff) {
    ++		timestamp_t generation = commit_graph_generation(commit);
    ++
    ++		if (generation_cutoff > generation)
    ++			generation_cutoff = generation;
    ++	}
    ++}
    ++
    ++/* adjust the commit date cutoff with a slop to allow for slightly incorrect
    ++ * commit timestamps in case of clock skew.
    ++ */
    ++static void adjust_cutoff_timestamp_for_slop(void)
    ++{
    ++	if (cutoff) {
    ++		/* check for undeflow */
    ++		if (cutoff > TIME_MIN + CUTOFF_DATE_SLOP)
    ++			cutoff = cutoff - CUTOFF_DATE_SLOP;
    ++		else
    ++			cutoff = TIME_MIN;
    ++	}
     +}
     +
     +/* Check if a commit is before the cutoff. Prioritize generation numbers
    @@ builtin/name-rev.c: struct rev_name {
     +static int commit_is_before_cutoff(struct commit *commit)
     +{
     +	if (generation_cutoff < GENERATION_NUMBER_INFINITY)
    -+		return commit_graph_generation(commit) < generation_cutoff;
    ++		return generation_cutoff &&
    ++			commit_graph_generation(commit) < generation_cutoff;
     +
     +	return commit->date < cutoff;
     +}
    @@ builtin/name-rev.c: static void name_rev(struct commit *start_commit,
      
      			if (parent_number > 1) {
     @@ builtin/name-rev.c: int cmd_name_rev(int argc, const char **argv, const char *prefix)
    - 		error("Specify either a list, or --all, not both!");
      		usage_with_options(name_rev_usage, opts);
      	}
    --	if (all || annotate_stdin)
    -+	if (all || annotate_stdin) {
    -+		generation_cutoff = 0;
    - 		cutoff = 0;
    -+	}
    + 	if (all || annotate_stdin)
    +-		cutoff = 0;
    ++		disable_cutoff();
      
      	for (; argc; argc--, argv++) {
      		struct object_id oid;
    @@ builtin/name-rev.c: int cmd_name_rev(int argc, const char **argv, const char *pr
      
      		if (peel_tag) {
      			if (!commit) {
    +@@ builtin/name-rev.c: int cmd_name_rev(int argc, const char **argv, const char *prefix)
    + 		add_object_array(object, *argv, &revs);
    + 	}
    + 
    +-	if (cutoff) {
    +-		/* check for undeflow */
    +-		if (cutoff > TIME_MIN + CUTOFF_DATE_SLOP)
    +-			cutoff = cutoff - CUTOFF_DATE_SLOP;
    +-		else
    +-			cutoff = TIME_MIN;
    +-	}
    ++	adjust_cutoff_timestamp_for_slop();
    ++
    + 	for_each_ref(name_ref, &data);
    + 	name_tips();
    + 
     
      ## t/t6120-describe.sh ##
     @@ t/t6120-describe.sh: test_expect_success 'name-rev covers all conditions while looking at parents' '
    @@ t/t6120-describe.sh: test_expect_success 'name-rev covers all conditions while l
     +	test_commit -C non-monotonic E
     +'
     +
    -+test_expect_success 'name-rev with commitGraph handles non-monotonic timestamps' '
    -+	test_config -C non-monotonic core.commitGraph true &&
    ++test_expect_success 'name-rev without commitGraph does not handle non-monotonic timestamps' '
    ++	test_config -C non-monotonic core.commitGraph false &&
     +	(
     +		cd non-monotonic &&
     +
    -+		# Ensure commit graph is up to date
    -+		git -c gc.writeCommitGraph=true gc &&
    ++		rm -rf .git/info/commit-graph* &&
     +
    -+		echo "main~3 tags/D~2" >expect &&
    ++		echo "main~3 undefined" >expect &&
     +		git name-rev --tags main~3 >actual &&
     +
     +		test_cmp expect actual
     +	)
     +'
     +
    -+test_expect_success 'name-rev --all works with non-monotonic' '
    ++test_expect_success 'name-rev --all works with non-monotonic timestamps' '
    ++	test_config -C non-monotonic core.commitGraph false &&
     +	(
     +		cd non-monotonic &&
     +
    ++		rm -rf .git/info/commit-graph* &&
    ++
    ++		cat >tags <<-\EOF &&
    ++		tags/E
    ++		tags/D
    ++		tags/D~1
    ++		tags/D~2
    ++		tags/A
    ++		EOF
    ++
    ++		git log --pretty=%H >revs &&
    ++
    ++		paste -d" " revs tags | sort >expect &&
    ++
    ++		git name-rev --tags --all | sort >actual &&
    ++		test_cmp expect actual
    ++	)
    ++'
    ++
    ++test_expect_success 'name-rev --annotate-stdin works with non-monotonic timestamps' '
    ++	test_config -C non-monotonic core.commitGraph false &&
    ++	(
    ++		cd non-monotonic &&
    ++
    ++		rm -rf .git/info/commit-graph* &&
    ++
     +		cat >expect <<-\EOF &&
     +		E
     +		D
    @@ t/t6120-describe.sh: test_expect_success 'name-rev covers all conditions while l
     +
     +		git log --pretty=%H >revs &&
     +		git name-rev --tags --annotate-stdin --name-only <revs >actual &&
    ++		test_cmp expect actual
    ++	)
    ++'
     +
    ++test_expect_success 'name-rev with commitGraph handles non-monotonic timestamps' '
    ++	test_config -C non-monotonic core.commitGraph true &&
    ++	(
    ++		cd non-monotonic &&
    ++
    ++		git commit-graph write --reachable &&
    ++
    ++		echo "main~3 tags/D~2" >expect &&
    ++		git name-rev --tags main~3 >actual &&
    ++
    ++		test_cmp expect actual
    ++	)
    ++'
    ++
    ++test_expect_success 'name-rev --all works with commitGraph' '
    ++	test_config -C non-monotonic core.commitGraph true &&
    ++	(
    ++		cd non-monotonic &&
    ++
    ++		git commit-graph write --reachable &&
    ++
    ++		cat >tags <<-\EOF &&
    ++		tags/E
    ++		tags/D
    ++		tags/D~1
    ++		tags/D~2
    ++		tags/A
    ++		EOF
    ++
    ++		git log --pretty=%H >revs &&
    ++
    ++		paste -d" " revs tags | sort >expect &&
    ++
    ++		git name-rev --tags --all | sort >actual &&
    ++		test_cmp expect actual
    ++	)
    ++'
    ++
    ++test_expect_success 'name-rev --annotate-stdin works with commitGraph' '
    ++	test_config -C non-monotonic core.commitGraph true &&
    ++	(
    ++		cd non-monotonic &&
    ++
    ++		git commit-graph write --reachable &&
    ++
    ++		cat >expect <<-\EOF &&
    ++		E
    ++		D
    ++		D~1
    ++		D~2
    ++		A
    ++		EOF
    ++
    ++		git log --pretty=%H >revs &&
    ++		git name-rev --tags --annotate-stdin --name-only <revs >actual &&
     +		test_cmp expect actual
     +	)
     +'

Jacob Keller (1):
  name-rev: use generation numbers if available

 builtin/name-rev.c  |  71 +++++++++++++++++++-----
 t/t6120-describe.sh | 132 ++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 189 insertions(+), 14 deletions(-)

-- 
2.35.1.355.ge7e302376dd6