[09/15] name-rev: restructure parsing commits and applying date cutoff
diff mbox series

Message ID 20190919214712.7348-10-szeder.dev@gmail.com
State New
Headers show
Series
  • name-rev: eliminate recursion
Related show

Commit Message

SZEDER Gábor Sept. 19, 2019, 9:47 p.m. UTC
At the beginning of the recursive name_rev() function it parses the
commit it got as parameter, and returns early if the commit is older
than a cutoff limit.

Restructure this so the caller parses the commit and checks its date,
and doesn't invoke name_rev() if the commit to be passed as parameter
is older than the cutoff, i.e. both name_ref() before calling
name_rev() and name_rev() itself as it iterates over the parent
commits.

This makes eliminating the recursion a bit easier to follow, and it
will be moved back to name_rev() after the recursion is eliminated.

Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
 builtin/name-rev.c | 29 ++++++++++++++++-------------
 1 file changed, 16 insertions(+), 13 deletions(-)

Comments

René Scharfe Sept. 21, 2019, 12:37 p.m. UTC | #1
Am 19.09.19 um 23:47 schrieb SZEDER Gábor:
> At the beginning of the recursive name_rev() function it parses the
> commit it got as parameter, and returns early if the commit is older
> than a cutoff limit.
>
> Restructure this so the caller parses the commit and checks its date,
> and doesn't invoke name_rev() if the commit to be passed as parameter
> is older than the cutoff, i.e. both name_ref() before calling
> name_rev() and name_rev() itself as it iterates over the parent
> commits.
>
> This makes eliminating the recursion a bit easier to follow, and it
> will be moved back to name_rev() after the recursion is eliminated.
>
> Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
> ---
>  builtin/name-rev.c | 29 ++++++++++++++++-------------
>  1 file changed, 16 insertions(+), 13 deletions(-)
>
> diff --git a/builtin/name-rev.c b/builtin/name-rev.c
> index 42cea5c881..99643aa4dc 100644
> --- a/builtin/name-rev.c
> +++ b/builtin/name-rev.c
> @@ -107,11 +107,6 @@ static void name_rev(struct commit *commit,
>  	struct commit_list *parents;
>  	int parent_number = 1;
>
> -	parse_commit(commit);
> -
> -	if (commit->date < cutoff)
> -		return;
> -
>  	if (!create_or_update_name(commit, tip_name, taggerdate, generation,
>  				   distance, from_tag))
>  		return;
> @@ -119,6 +114,12 @@ static void name_rev(struct commit *commit,
>  	for (parents = commit->parents;
>  			parents;
>  			parents = parents->next, parent_number++) {
> +		struct commit *parent = parents->item;
> +
> +		parse_commit(parent);
> +		if (parent->date < cutoff)
> +			continue;
> +
>  		if (parent_number > 1) {
>  			size_t len;
>  			char *new_name;
> @@ -131,11 +132,11 @@ static void name_rev(struct commit *commit,
>  				new_name = xstrfmt("%.*s^%d", (int)len, tip_name,
>  						   parent_number);
>

The check now also skips this allocation for old commits...

> -			name_rev(parents->item, new_name, taggerdate, 0,
> +			name_rev(parent, new_name, taggerdate, 0,
>  				 distance + MERGE_TRAVERSAL_WEIGHT,
>  				 from_tag);
>  		} else {
> -			name_rev(parents->item, tip_name, taggerdate,
> +			name_rev(parent, tip_name, taggerdate,
>  				 generation + 1, distance + 1,
>  				 from_tag);
>  		}
> @@ -269,16 +270,18 @@ static int name_ref(const char *path, const struct object_id *oid, int flags, vo
>  	if (o && o->type == OBJ_COMMIT) {
>  		struct commit *commit = (struct commit *)o;
>  		int from_tag = starts_with(path, "refs/tags/");
> -		const char *tip_name;
>
>  		if (taggerdate == TIME_MAX)
>  			taggerdate = commit->date;
>  		path = name_ref_abbrev(path, can_abbreviate_output);
> -		if (deref)
> -			tip_name = xstrfmt("%s^0", path);
> -		else
> -			tip_name = xstrdup(path);
> -		name_rev(commit, tip_name, taggerdate, 0, 0, from_tag);
> +		if (commit->date >= cutoff) {
> +			const char *tip_name;
> +			if (deref)
> +				tip_name = xstrfmt("%s^0", path);
> +			else
> +				tip_name = xstrdup(path);

... and this allocation here as well.  If this improves performance
in a meaningful way then perhaps it should be kept at this place?
And if it doesn't, then an additional allocation might not hurt much?

Just a thought, I still didn't measure..

> +			name_rev(commit, tip_name, taggerdate, 0, 0, from_tag);
> +		}
>  	}
>  	return 0;
>  }
>

Patch
diff mbox series

diff --git a/builtin/name-rev.c b/builtin/name-rev.c
index 42cea5c881..99643aa4dc 100644
--- a/builtin/name-rev.c
+++ b/builtin/name-rev.c
@@ -107,11 +107,6 @@  static void name_rev(struct commit *commit,
 	struct commit_list *parents;
 	int parent_number = 1;
 
-	parse_commit(commit);
-
-	if (commit->date < cutoff)
-		return;
-
 	if (!create_or_update_name(commit, tip_name, taggerdate, generation,
 				   distance, from_tag))
 		return;
@@ -119,6 +114,12 @@  static void name_rev(struct commit *commit,
 	for (parents = commit->parents;
 			parents;
 			parents = parents->next, parent_number++) {
+		struct commit *parent = parents->item;
+
+		parse_commit(parent);
+		if (parent->date < cutoff)
+			continue;
+
 		if (parent_number > 1) {
 			size_t len;
 			char *new_name;
@@ -131,11 +132,11 @@  static void name_rev(struct commit *commit,
 				new_name = xstrfmt("%.*s^%d", (int)len, tip_name,
 						   parent_number);
 
-			name_rev(parents->item, new_name, taggerdate, 0,
+			name_rev(parent, new_name, taggerdate, 0,
 				 distance + MERGE_TRAVERSAL_WEIGHT,
 				 from_tag);
 		} else {
-			name_rev(parents->item, tip_name, taggerdate,
+			name_rev(parent, tip_name, taggerdate,
 				 generation + 1, distance + 1,
 				 from_tag);
 		}
@@ -269,16 +270,18 @@  static int name_ref(const char *path, const struct object_id *oid, int flags, vo
 	if (o && o->type == OBJ_COMMIT) {
 		struct commit *commit = (struct commit *)o;
 		int from_tag = starts_with(path, "refs/tags/");
-		const char *tip_name;
 
 		if (taggerdate == TIME_MAX)
 			taggerdate = commit->date;
 		path = name_ref_abbrev(path, can_abbreviate_output);
-		if (deref)
-			tip_name = xstrfmt("%s^0", path);
-		else
-			tip_name = xstrdup(path);
-		name_rev(commit, tip_name, taggerdate, 0, 0, from_tag);
+		if (commit->date >= cutoff) {
+			const char *tip_name;
+			if (deref)
+				tip_name = xstrfmt("%s^0", path);
+			else
+				tip_name = xstrdup(path);
+			name_rev(commit, tip_name, taggerdate, 0, 0, from_tag);
+		}
 	}
 	return 0;
 }