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

[Jeffrey Smith]

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  wrote:
> Jeff Smith  writes:
>
>> Signed-off-by: Jeff Smith 
>> ---
>>  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, );
>> + 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, _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.
>
>

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

[Junio C Hamano]

Jeff Smith  writes:

> Signed-off-by: Jeff Smith 
> ---
>  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, );
> + 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, _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.

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

[Jeff Smith]

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

diff --git a/blame.c b/blame.c
index 798e61b..f6c9cb7 100644
--- a/blame.c
+++ b/blame.c
@@ -4,6 +4,7 @@
 #include "mergesort.h"
 #include "diff.h"
 #include "diffcore.h"
+#include "tag.h"
 #include "blame.h"
 
 void blame_origin_decref(struct blame_origin *o)
@@ -49,7 +50,7 @@ static struct blame_origin *make_origin(struct commit 
*commit, const char *path)
  * Locate an existing origin or create a new one.
  * This moves the origin to front position in the commit util list.
  */
-struct blame_origin *get_origin(struct commit *commit, const char *path)
+static struct blame_origin *get_origin(struct commit *commit, const char *path)
 {
struct blame_origin *o, *l;
 
@@ -142,9 +143,9 @@ static void set_commit_buffer_from_strbuf(struct commit *c, 
struct strbuf *sb)
  * Prepare a dummy commit that represents the work tree (or staged) item.
  * Note that annotating work tree item never works in the reverse.
  */
-struct commit *fake_working_tree_commit(struct diff_options *opt,
-   const char *path,
-   const char *contents_from)
+static struct commit *fake_working_tree_commit(struct diff_options *opt,
+  const char *path,
+  const char *contents_from)
 {
struct commit *commit;
struct blame_origin *origin;
@@ -410,6 +411,13 @@ void blame_sort_final(struct blame_scoreboard *sb)
  compare_blame_final);
 }
 
+static int compare_commits_by_reverse_commit_date(const void *a,
+ const void *b,
+ void *c)
+{
+   return -compare_commits_by_commit_date(a, b, c);
+}
+
 /*
  * For debugging -- origin is refcounted, and this asserts that
  * we do not underflow.
@@ -483,6 +491,32 @@ static void queue_blames(struct blame_scoreboard *sb, 
struct blame_origin *porig
 }
 
 /*
+ * Fill the blob_sha1 field of an origin if it hasn't, so that later
+ * call to fill_origin_blob() can use it to locate the data.  blob_sha1
+ * for an origin is also used to pass the blame for the entire file to
+ * the parent to detect the case where a child's blob is identical to
+ * that of its parent's.
+ *
+ * This also fills origin->mode for corresponding tree path.
+ */
+static int fill_blob_sha1_and_mode(struct blame_origin *origin)
+{
+   if (!is_null_oid(>blob_oid))
+   return 0;
+   if (get_tree_entry(origin->commit->object.oid.hash,
+  origin->path,
+  origin->blob_oid.hash, >mode))
+   goto error_out;
+   if (sha1_object_info(origin->blob_oid.hash, NULL) != OBJ_BLOB)
+   goto error_out;
+   return 0;
+ error_out:
+   oidclr(>blob_oid);
+   origin->mode = S_IFINVALID;
+   return -1;
+}
+
+/*
  * We have an origin -- check if the same path exists in the
  * parent and return an origin structure to represent it.
  */
@@ -1574,3 +1608,240 @@ void assign_blame(struct blame_scoreboard *sb, int opt)
sanity_check_refcnt(sb);
}
 }
+
+static const char *get_next_line(const char *start, const char *end)
+{
+   const char *nl = memchr(start, '\n', end - start);
+   return nl ? nl + 1 : end;
+}
+
+/*
+ * To allow quick access to the contents of nth line in the
+ * final image, prepare an index in the scoreboard.
+ */
+static int prepare_lines(struct blame_scoreboard *sb)
+{
+   const char *buf = sb->final_buf;
+   unsigned long len = sb->final_buf_size;
+   const char *end = buf + len;
+   const char *p;
+   int *lineno;
+   int num = 0;
+
+   for (p = buf; p < end; p = get_next_line(p, end))
+   num++;
+
+   ALLOC_ARRAY(sb->lineno, num + 1);
+   lineno = sb->lineno;
+
+   for (p = buf; p < end; p = get_next_line(p, end))
+   *lineno++ = p - buf;
+
+   *lineno = len;
+
+   sb->num_lines = num;
+   return sb->num_lines;
+}
+
+static struct commit *find_single_final(struct rev_info *revs,
+   const char **name_p)
+{
+   int i;
+   struct commit *found = NULL;
+   const char *name = NULL;
+
+   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)
+