Re: [PATCH 28/29] blame: move scoreboard setup to libgit

Jeff Smith [off-list ref] writes:

Signed-off-by: Jeff Smith <redacted>
---
 blame.c         | 279 +++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 blame.h         |  10 +-
 builtin/blame.c | 276 -------------------------------------------------------
 3 files changed, 281 insertions(+), 284 deletions(-)

...
+static struct commit *find_single_initial(struct rev_info *revs,
+					  const char **name_p)
+{
+	int i;
+	struct commit *found = NULL;
+	const char *name = NULL;
+
+	/*
+	 * There must be one and only one negative commit, and it must be
+	 * the boundary.
+	 */
+	for (i = 0; i < revs->pending.nr; i++) {
+		struct object *obj = revs->pending.objects[i].item;
+		if (!(obj->flags & UNINTERESTING))
+			continue;
+		obj = deref_tag(obj, NULL, 0);
+		if (obj->type != OBJ_COMMIT)
+			die("Non commit %s?", revs->pending.objects[i].name);
+		if (found)
+			die("More than one commit to dig up from, %s and %s?",
+			    revs->pending.objects[i].name, name);
+		found = (struct commit *) obj;
+		name = revs->pending.objects[i].name;
+	}
+
+	if (!name)
+		found = dwim_reverse_initial(revs, &name);
+	if (!name)
+		die("No commit to dig up from?");
+
+	if (name_p)
+		*name_p = name;
+	return found;
+}
+...
-static struct commit *find_single_initial(struct rev_info *revs,
-					  const char **name_p)
-{
-	int i;
-	const char *final_commit_name = NULL;
-	struct commit *found = NULL;
-
-...
-
-	if (!final_commit_name)
-		found = dwim_reverse_initial(revs, &final_commit_name);
-	if (!final_commit_name)
-		die("No commit to dig up from?");
-
-	if (name_p)
-		*name_p = final_commit_name;
-	return found;
-}

In a patch whose primary purpose is to move code between files,
making what used to be public to static and vice versa is an
integral part of moving code.  That is why we want to see a patch
organized in such a way that comparing the lines that are lost from
builtin/blame.c and the lines that are added to blame.[ch] is made
easy.

And from that point of view, it was somewhat irritating to find this
kind of meaningless change.  If you didn't like the name of the
variable "final-commit-name", that shold have been renamed while the
code was still in builtin/blame.c

The end result looks OK anyway (I've checked 29/29 as well).

Thanks.

I had meant to change the name to match what is in find_single_final.
While the intent was for it to change while in builtin/blame.c,
apparently I missed that in the shuffle.

On Thu, May 25, 2017 at 12:53 AM, Junio C Hamano [off-list ref] wrote:

Jeff Smith [off-list ref] writes:

quoted

Signed-off-by: Jeff Smith <redacted>
---
 blame.c         | 279 +++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 blame.h         |  10 +-
 builtin/blame.c | 276 -------------------------------------------------------
 3 files changed, 281 insertions(+), 284 deletions(-)

...
+static struct commit *find_single_initial(struct rev_info *revs,
+                                       const char **name_p)
+{
+     int i;
+     struct commit *found = NULL;
+     const char *name = NULL;
+
+     /*
+      * There must be one and only one negative commit, and it must be
+      * the boundary.
+      */
+     for (i = 0; i < revs->pending.nr; i++) {
+             struct object *obj = revs->pending.objects[i].item;
+             if (!(obj->flags & UNINTERESTING))
+                     continue;
+             obj = deref_tag(obj, NULL, 0);
+             if (obj->type != OBJ_COMMIT)
+                     die("Non commit %s?", revs->pending.objects[i].name);
+             if (found)
+                     die("More than one commit to dig up from, %s and %s?",
+                         revs->pending.objects[i].name, name);
+             found = (struct commit *) obj;
+             name = revs->pending.objects[i].name;
+     }
+
+     if (!name)
+             found = dwim_reverse_initial(revs, &name);
+     if (!name)
+             die("No commit to dig up from?");
+
+     if (name_p)
+             *name_p = name;
+     return found;
+}
+...
-static struct commit *find_single_initial(struct rev_info *revs,
-                                       const char **name_p)
-{
-     int i;
-     const char *final_commit_name = NULL;
-     struct commit *found = NULL;
-
-...
-
-     if (!final_commit_name)
-             found = dwim_reverse_initial(revs, &final_commit_name);
-     if (!final_commit_name)
-             die("No commit to dig up from?");
-
-     if (name_p)
-             *name_p = final_commit_name;
-     return found;
-}

In a patch whose primary purpose is to move code between files,
making what used to be public to static and vice versa is an
integral part of moving code.  That is why we want to see a patch
organized in such a way that comparing the lines that are lost from
builtin/blame.c and the lines that are added to blame.[ch] is made
easy.

And from that point of view, it was somewhat irritating to find this
kind of meaningless change.  If you didn't like the name of the
variable "final-commit-name", that shold have been renamed while the
code was still in builtin/blame.c

The end result looks OK anyway (I've checked 29/29 as well).

Thanks.