[tig PATCH] fix off-by-one on parent selection

[tig PATCH] fix off-by-one on parent selection

[Jeff King]

Originally, we use "git rev-list -1 --parents" to get the
list of parents, and therefore the 0th slot was the commit
in question, the 1st slot was the 1st parent, and so forth.

Commit 0a46941 switched this to use --pretty=format:%P, so
that the menu-selection code could be easily used (which
counts items starting from 0). However, we only use the menu
code in the case of multiple parents.  For a single parent,
this introduced an off-by-one where we look just past the
parent we want.

This patch fixes it by explicitly selecting the 0th parent
for the single parent case.

Signed-off-by: Jeff King <peff@peff.net>
---
This is an old bug, but I finally got a chance to track it down.

There is a related buglet elsewhere in select_commit_parent. Now that we
ask git to print only the parents, we will get no output at all for a
parent-less commit. This will cause iobuf_read to return an error, and
we will print "Failed to get parent information" instead of "The
selected commit has no parents" (or "Path '%s' does not exist" if we are
blaming the parent of a commit that introduced a file).

AFAICT, fixing it would mean improving iobuf_read to differentiate "no
output" from "there were errors". I'll leave that sort of infrastructure
refactoring to you if you want to do it. The resulting bug is quite
minor.

 tig.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/tig.c b/tig.c
index 074e414..60932d4 100644
--- a/tig.c
+++ b/tig.c
@@ -4012,7 +4012,9 @@ select_commit_parent(const char *id, char rev[SIZEOF_REV], const char *path)
 		return FALSE;
 	}
 
-	if (parents > 1 && !open_commit_parent_menu(buf, &parents))
+	if (parents == 1)
+		parents = 0;
+	else if (!open_commit_parent_menu(buf, &parents))
 		return FALSE;
 
 	string_copy_rev(rev, &buf[41 * parents]);
-- 
1.7.1.248.gf52fc

Re: [tig PATCH] fix off-by-one on parent selection

[Jonas Fonseca]

[-- Attachment #1: Type: text/plain, Size: 2250 bytes --]

On Mon, May 10, 2010 at 04:55, Jeff King <peff@peff.net> wrote:
> This patch fixes it by explicitly selecting the 0th parent
> for the single parent case.
> [...]
> This is an old bug, but I finally got a chance to track it down.

Thanks for fixing it.

> There is a related buglet elsewhere in select_commit_parent. Now that we
> ask git to print only the parents, we will get no output at all for a
> parent-less commit. This will cause iobuf_read to return an error, and
> we will print "Failed to get parent information" instead of "The
> selected commit has no parents" (or "Path '%s' does not exist" if we are
> blaming the parent of a commit that introduced a file).
>
> AFAICT, fixing it would mean improving iobuf_read to differentiate "no
> output" from "there were errors". I'll leave that sort of infrastructure
> refactoring to you if you want to do it. The resulting bug is quite
> minor.

The spaced damaged patch below fixes the first error.
--- >8 --- >8 --- >8 ---
diff --git a/tig.c b/tig.c
index 35b0cfa..f5bb1b9 100644
--- a/tig.c
+++ b/tig.c
@@ -1028,7 +1028,7 @@ io_read_buf(struct io *io, char buf[], size_t bufsize)
                string_ncopy_do(buf, bufsize, result, strlen(result));
        }

-       return io_done(io) && result;
+       return io_done(io) && !io_error(io);
 }

 static bool
--- 8< --- 8< --- 8< ---

However, it seems that the output of the command that was previously
used for fetching parents and the current one pretty printing using
the %P flag is also the cause of the breakage.

In the tig repository, trying to "blame" the parent of b801d8b2b shows
reproduces the problem. Commit b801d8b2b replaced cgit.c with tig.c,
which means there is no parent blame to show.

Before:
> git rev-list -1 --parents b801d8b2b -- tig.c
b801d8b2bc1a6aac6b9744f21f7a10a51e16c53e
.. i.e no parents as expected.

Now:
> git log --no-color -1 --pretty=format:%P b801d8b2b -- tig.c
a7bc4b1447f974fbbe400c3657d9ec3d0fda133e
.. i.e. the parent of b801d8b2b, but where tig.c does not exist.

The attached patch addresses this problem by reverting back to the
command used before.

A related question is why the hell I chose to switch to using %P in
commit 0a4694191613f887151a52f0c70e6b6181ea5fb6 ...

--
Jonas Fonseca

[-- Attachment #2: fix-parent-blame.patch --]
[-- Type: application/octet-stream, Size: 1207 bytes --]

 tig.c |   12 ++++--------
 1 files changed, 4 insertions(+), 8 deletions(-)

diff --git a/tig.c b/tig.c
index 35b0cfa..b2dbca7 100644
--- a/tig.c
+++ b/tig.c
@@ -3995,17 +3995,15 @@ select_commit_parent(const char *id, char rev[SIZEOF_REV], const char *path)
 {
 	char buf[SIZEOF_STR * 4];
 	const char *revlist_argv[] = {
-		"git", "log", "--no-color", "-1",
-			"--pretty=format:%P", id, "--", path, NULL
+		"git", "rev-list", "-1", "--parents", id, "--", path, NULL
 	};
 	int parents;
 
-	if (!io_run_buf(revlist_argv, buf, sizeof(buf)) ||
-	    (parents = strlen(buf) / 40) < 0) {
+	if (!io_run_buf(revlist_argv, buf, sizeof(buf))) {
 		report("Failed to get parent information");
 		return FALSE;
 
-	} else if (parents == 0) {
+	} else if ((parents = (strlen(buf) / 40) - 1) <= 0) {
 		if (path)
 			report("Path '%s' does not exist in the parent", path);
 		else
@@ -4013,9 +4011,7 @@ select_commit_parent(const char *id, char rev[SIZEOF_REV], const char *path)
 		return FALSE;
 	}
 
-	if (parents == 1)
-		parents = 0;
-	else if (!open_commit_parent_menu(buf, &parents))
+	if (parents > 1 && !open_commit_parent_menu(buf, &parents))
 		return FALSE;
 
 	string_copy_rev(rev, &buf[41 * parents]);

Re: [tig PATCH] fix off-by-one on parent selection

[Jeff King]

On Sat, May 22, 2010 at 01:19:12PM -0400, Jonas Fonseca wrote:

> > AFAICT, fixing it would mean improving iobuf_read to differentiate "no
> > output" from "there were errors". I'll leave that sort of infrastructure
> > refactoring to you if you want to do it. The resulting bug is quite
> > minor.
> 
> The spaced damaged patch below fixes the first error.
> --- >8 --- >8 --- >8 ---
> diff --git a/tig.c b/tig.c
> index 35b0cfa..f5bb1b9 100644
> --- a/tig.c
> +++ b/tig.c
> @@ -1028,7 +1028,7 @@ io_read_buf(struct io *io, char buf[], size_t bufsize)
>                 string_ncopy_do(buf, bufsize, result, strlen(result));
>         }
> 
> -       return io_done(io) && result;
> +       return io_done(io) && !io_error(io);
>  }
> 
>  static bool
> --- 8< --- 8< --- 8< ---

Yeah, that works for me. I was hesitant to do it because I wasn't sure
if other callers were relying on that behavior, but it looks like all
calls properly check the output for sanity.

> However, it seems that the output of the command that was previously
> used for fetching parents and the current one pretty printing using
> the %P flag is also the cause of the breakage.
> 
> In the tig repository, trying to "blame" the parent of b801d8b2b shows
> reproduces the problem. Commit b801d8b2b replaced cgit.c with tig.c,
> which means there is no parent blame to show.
> 
> Before:
> > git rev-list -1 --parents b801d8b2b -- tig.c
> b801d8b2bc1a6aac6b9744f21f7a10a51e16c53e
> .. i.e no parents as expected.
> 
> Now:
> > git log --no-color -1 --pretty=format:%P b801d8b2b -- tig.c
> a7bc4b1447f974fbbe400c3657d9ec3d0fda133e
> .. i.e. the parent of b801d8b2b, but where tig.c does not exist.

This confused me at first, because those outputs should be the same, but
now I see: the pretty %P does not respect history simplification, so you
get the _true_ parent of b801d8b2b, and not the simplified one when
limiting history to 'tig.c'.

So yes, the rev-list version is better, because it at least realizes
that tig.c has no parent.

But I think neither is ideal. What we probably want is to detect the
rename between the two, and do the blame from the parent using cgit.c.
git-blame will already recognize content coming from another file and
give us that filename, but we are moving to the parent ourselves, so we
have to do that rename detection manually (IOW, this behavior triggers
_only_ when you are trying to blame the parent of a commit which
simultaneously introduced the line and moved the filename).

Patch is below. After writing it, I realized that tig.c is not
actually a rename of cgit.c (they are too dissimilar). It does provide
the correct "Path 'tig.c' does not exist in the parent". And you can see
the rename-following behavior with something like:

  perl -e 'print "$_\n" for 1..1000' >old
  git add . && git commit -m added
  perl -pe 's/^1$/foo/' <old >new
  git add new && git rm old && git commit -m moved

Now try "tig blame new". For all of the lines but the first, blaming the
parent gets you the correct "The selected commit has no parents". But
parent-blaming the first line will correctly re-blame using the filename
"old".

There are some possible optimizations that I didn't implement:

  1. With this patch, we always check for a rename to the parent. But we
     really only need to do so if the commit in question introduced the
     file. One way to detect that is by first running "git diff-tree
     $file", and only doing the rename detection if the file was added.
     We could also potentially use the rev-list in select_commit_parent
     to see that we have no parents. That would mean combining
     select_commit_parent and my new follow_parent_rename.

  2. The diff-tree could potentially be combined with the one we execute
     immediately after in setup_blame_parent_line. But I don't think it
     is worth it. In the rename-follow, we have to look at _all_ of the
     files, as they are potential sources. But in
     setup_blame_parent_line, we are generating diffs and can restrict
     our diff to only the file of interest. I don't think there is a way
     to say "consider all files as rename sources, but only show the
     patch for this one file".

diff --git a/tig.c b/tig.c
index 28679f9..cfa26ce 100644
--- a/tig.c
+++ b/tig.c
@@ -3991,12 +3991,12 @@ open_commit_parent_menu(char buf[SIZEOF_STR], int *parents)
 }
 
 static bool
-select_commit_parent(const char *id, char rev[SIZEOF_REV], const char *path)
+select_commit_parent(const char *id, char rev[SIZEOF_REV])
 {
 	char buf[SIZEOF_STR * 4];
 	const char *revlist_argv[] = {
 		"git", "log", "--no-color", "-1",
-			"--pretty=format:%P", id, "--", path, NULL
+			"--pretty=format:%P", id, "--", NULL
 	};
 	int parents;
 
@@ -4006,10 +4006,7 @@ select_commit_parent(const char *id, char rev[SIZEOF_REV], const char *path)
 		return FALSE;
 
 	} else if (parents == 0) {
-		if (path)
-			report("Path '%s' does not exist in the parent", path);
-		else
-			report("The selected commit has no parents");
+		report("The selected commit has no parents");
 		return FALSE;
 	}
 
@@ -4022,6 +4019,48 @@ select_commit_parent(const char *id, char rev[SIZEOF_REV], const char *path)
 	return TRUE;
 }
 
+static int
+follow_parent_rename(const char *id, const char *parent, const char *dest,
+		     char source[SIZEOF_STR])
+{
+	const char *diff_argv[] = {
+		"git", "diff-tree", "-z", "--name-status", "-M",
+		parent, id, "--", NULL
+	};
+	struct io io = {};
+	char *buf;
+
+	if (!io_run(&io, diff_argv, opt_cdup, IO_RD))
+		return FALSE;
+
+	while ((buf = io_get(&io, 0, TRUE))) {
+		int status = buf[0];
+
+		if (!(buf = io_get(&io, 0, TRUE)))
+			break;
+
+		if (status == 'A' && !strcmp(buf, dest)) {
+			report("Path '%s' does not exist in the parent", dest);
+			io_done(&io);
+			return FALSE;
+		}
+
+		if (status == 'R') {
+			string_ncopy_do(source, SIZEOF_STR, buf, strlen(buf));
+			if (!(buf = io_get(&io, 0, TRUE)))
+				break;
+			if (!strcmp(buf, dest)) {
+				io_done(&io);
+				return TRUE;
+			}
+		}
+	}
+
+	string_ncopy_do(source, SIZEOF_STR, dest, strlen(dest));
+	io_done(&io);
+	return TRUE;
+}
+
 /*
  * Pager backend
  */
@@ -5190,9 +5229,9 @@ blame_request(struct view *view, enum request request, struct line *line)
 
 	case REQ_PARENT:
 		if (check_blame_commit(blame, TRUE) &&
-		    select_commit_parent(blame->commit->id, opt_ref,
-					 blame->commit->filename)) {
-			string_copy(opt_file, blame->commit->filename);
+		    select_commit_parent(blame->commit->id, opt_ref) &&
+		    follow_parent_rename(blame->commit->id, opt_ref,
+					 blame->commit->filename, opt_file)) {
 			setup_blame_parent_line(view, blame);
 			open_view(view, REQ_VIEW_BLAME, OPEN_REFRESH);
 		}

Re: [tig PATCH] fix off-by-one on parent selection

[Jeff King]

On Sun, May 23, 2010 at 03:40:52AM -0400, Jeff King wrote:

> Now try "tig blame new". For all of the lines but the first, blaming the
> parent gets you the correct "The selected commit has no parents". But
> parent-blaming the first line will correctly re-blame using the filename
> "old".

By the way, there is one minor bug remaining after this patch:

>  	case REQ_PARENT:
>  		if (check_blame_commit(blame, TRUE) &&
> -		    select_commit_parent(blame->commit->id, opt_ref,
> -					 blame->commit->filename)) {
> -			string_copy(opt_file, blame->commit->filename);
> +		    select_commit_parent(blame->commit->id, opt_ref) &&
> +		    follow_parent_rename(blame->commit->id, opt_ref,
> +					 blame->commit->filename, opt_file)) {
>  			setup_blame_parent_line(view, blame);
>  			open_view(view, REQ_VIEW_BLAME, OPEN_REFRESH);
>  		}

We may write some new filename into opt_file in the follow_parent_rename
call, but setup_blame_parent_line always diffs the original file. Which
means we lose the line position when following a rename.

We need to do the equivalent of:

  git diff -U0 \
    opt_ref:opt_file \
    blame->commit->id:blame->commit->filename

IOW, to blame directly between the two blobs. Sadly, I don't think there
is a plumbing command to do this, so we are stuck using regular "git
diff", which may have surprises in the config.

The patch below works for my simple tests.  I think we probably want to
be doing this anyway for the multiple-parent case. I didn't test, but I
don't think that diff-tree invocation is going to produce any output for
a merge commit.

diff --git a/tig.c b/tig.c
index cfa26ce..4388c2f 100644
--- a/tig.c
+++ b/tig.c
@@ -5177,15 +5177,21 @@ check_blame_commit(struct blame *blame, bool check_null_id)
 static void
 setup_blame_parent_line(struct view *view, struct blame *blame)
 {
+	char from[SIZEOF_REF+SIZEOF_STR];
+	char to[SIZEOF_REF+SIZEOF_STR];
 	const char *diff_tree_argv[] = {
-		"git", "diff-tree", "-U0", blame->commit->id,
-			"--", blame->commit->filename, NULL
+		"git", "diff", "--no-textconv", "--no-extdiff", "--no-color",
+		"-U0", from, to, "--", NULL
 	};
 	struct io io = {};
 	int parent_lineno = -1;
 	int blamed_lineno = -1;
 	char *line;
 
+	snprintf(from, sizeof(from), "%s:%s", opt_ref, opt_file);
+	snprintf(to, sizeof(to), "%s:%s", blame->commit->id,
+		 blame->commit->filename);
+
 	if (!io_run(&io, diff_tree_argv, NULL, IO_RD))
 		return;
 

[PATCH] Improve parent blame to detect renames by using the previous information

[Jonas Fonseca]

>From git commit 96e117099c0e4f7d508eb071f60b6275038f6f37:

 It gives the parent commit of the blamed commit, _and_ a path in that
 parent commit that corresponds to the blamed path --- in short, it is
 the origin that would have been blamed (or passed blame through) for
 the line _if_ the blamed commit did not change that line.

This functionality was released in git version 1.6.3 in 2009-05-06.
---
 NEWS  |    2 +
 tig.c |   99 +++++++++++++++-------------------------------------------------
 2 files changed, 25 insertions(+), 76 deletions(-)

 I finally got some more time to dig around this. What if we simply uses
 the information given by the porcelain output's previous line? It
 handles your simple test case, and navigating in the tig repository. It
 also makes it possible to delete a lot of code.

diff --git a/NEWS b/NEWS
index 190e5dc..499bdbc 100644
--- a/NEWS
+++ b/NEWS
@@ -38,6 +38,8 @@ Bug fixes:
  - Fix unbind to behave as if the keybinding was never defined.
  - Fix unbind to also cover built-in run requests.
  - Fix parsing of unknown keymap names.
+ - Blame view: fix parent blame to detect renames. It uses "previous"
+   line info from the blame porcelain output added in git version 1.6.3.
 
 tig-0.15
 --------
diff --git a/tig.c b/tig.c
index 01f48c3..044da28 100644
--- a/tig.c
+++ b/tig.c
@@ -3977,73 +3977,6 @@ parse_author_line(char *ident, const char **author, struct time *time)
 	}
 }
 
-static bool
-open_commit_parent_menu(char buf[SIZEOF_STR], int *parents)
-{
-	char rev[SIZEOF_REV];
-	const char *revlist_argv[] = {
-		"git", "log", "--no-color", "-1", "--pretty=format:%s", rev, NULL
-	};
-	struct menu_item *items;
-	char text[SIZEOF_STR];
-	bool ok = TRUE;
-	int i;
-
-	items = calloc(*parents + 1, sizeof(*items));
-	if (!items)
-		return FALSE;
-
-	for (i = 0; i < *parents; i++) {
-		string_copy_rev(rev, &buf[SIZEOF_REV * i]);
-		if (!io_run_buf(revlist_argv, text, sizeof(text)) ||
-		    !(items[i].text = strdup(text))) {
-			ok = FALSE;
-			break;
-		}
-	}
-
-	if (ok) {
-		*parents = 0;
-		ok = prompt_menu("Select parent", items, parents);
-	}
-	for (i = 0; items[i].text; i++)
-		free((char *) items[i].text);
-	free(items);
-	return ok;
-}
-
-static bool
-select_commit_parent(const char *id, char rev[SIZEOF_REV], const char *path)
-{
-	char buf[SIZEOF_STR * 4];
-	const char *revlist_argv[] = {
-		"git", "log", "--no-color", "-1",
-			"--pretty=format:%P", id, "--", path, NULL
-	};
-	int parents;
-
-	if (!io_run_buf(revlist_argv, buf, sizeof(buf)) ||
-	    (parents = strlen(buf) / 40) < 0) {
-		report("Failed to get parent information");
-		return FALSE;
-
-	} else if (parents == 0) {
-		if (path)
-			report("Path '%s' does not exist in the parent", path);
-		else
-			report("The selected commit has no parents");
-		return FALSE;
-	}
-
-	if (parents == 1)
-		parents = 0;
-	else if (!open_commit_parent_menu(buf, &parents))
-		return FALSE;
-
-	string_copy_rev(rev, &buf[41 * parents]);
-	return TRUE;
-}
-
 /*
  * Pager backend
  */
@@ -4898,7 +4831,8 @@ struct blame_commit {
 	const char *author;		/* Author of the commit. */
 	struct time time;		/* Date from the author ident. */
 	char filename[128];		/* Name of file. */
-	bool has_previous;		/* Was a "previous" line detected. */
+	char parent_id[SIZEOF_REV];	/* Parent/previous SHA1 ID. */
+	char parent_filename[128];	/* Parent/previous name of file. */
 };
 
 struct blame {
@@ -5097,7 +5031,11 @@ blame_read(struct view *view, char *line)
 		string_ncopy(commit->title, line, strlen(line));
 
 	} else if (match_blame_header("previous ", &line)) {
-		commit->has_previous = TRUE;
+		if (strlen(line) <= SIZEOF_REV)
+			return FALSE;
+		string_copy_rev(commit->parent_id, line);
+		line += SIZEOF_REV;
+		string_ncopy(commit->parent_filename, line, strlen(line));
 
 	} else if (match_blame_header("filename ", &line)) {
 		string_ncopy(commit->filename, line, strlen(line));
@@ -5153,15 +5091,21 @@ check_blame_commit(struct blame *blame, bool check_null_id)
 static void
 setup_blame_parent_line(struct view *view, struct blame *blame)
 {
+	char from[SIZEOF_REF + SIZEOF_STR];
+	char to[SIZEOF_REF + SIZEOF_STR];
 	const char *diff_tree_argv[] = {
-		"git", "diff-tree", "-U0", blame->commit->id,
-			"--", blame->commit->filename, NULL
+		"git", "diff", "--no-textconv", "--no-extdiff", "--no-color",
+			"-U0", from, to, "--", NULL
 	};
 	struct io io;
 	int parent_lineno = -1;
 	int blamed_lineno = -1;
 	char *line;
 
+	snprintf(from, sizeof(from), "%s:%s", opt_ref, opt_file);
+	snprintf(to, sizeof(to), "%s:%s", blame->commit->id,
+		 blame->commit->filename);
+
 	if (!io_run(&io, IO_RD, NULL, diff_tree_argv))
 		return;
 
@@ -5204,10 +5148,13 @@ blame_request(struct view *view, enum request request, struct line *line)
 		break;
 
 	case REQ_PARENT:
-		if (check_blame_commit(blame, TRUE) &&
-		    select_commit_parent(blame->commit->id, opt_ref,
-					 blame->commit->filename)) {
-			string_copy(opt_file, blame->commit->filename);
+		if (!check_blame_commit(blame, TRUE))
+			break;
+		if (!*blame->commit->parent_id) {
+			report("The selected commit has no parents");
+		} else {
+			string_copy_rev(opt_ref, blame->commit->parent_id);
+			string_copy_rev(opt_file, blame->commit->parent_filename);
 			setup_blame_parent_line(view, blame);
 			open_view(view, REQ_VIEW_BLAME, OPEN_REFRESH);
 		}
@@ -5228,7 +5175,7 @@ blame_request(struct view *view, enum request request, struct line *line)
 					"-C", "-M", "HEAD", "--", view->vid, NULL
 			};
 
-			if (!blame->commit->has_previous) {
+			if (!*blame->commit->parent_id) {
 				diff_index_argv[1] = "diff";
 				diff_index_argv[2] = "--no-color";
 				diff_index_argv[6] = "--";
-- 
1.7.1.354.ge64bd

Re: [TIG PATCH] Improve parent blame to detect renames by using the previous information

[Jonas Fonseca]

Sorry, forgot to prefix this mail properly ...

On Sat, Jun 5, 2010 at 15:56, Jonas Fonseca <fonseca@diku.dk> wrote:
> From git commit 96e117099c0e4f7d508eb071f60b6275038f6f37:
>
>  It gives the parent commit of the blamed commit, _and_ a path in that
>  parent commit that corresponds to the blamed path --- in short, it is
>  the origin that would have been blamed (or passed blame through) for
>  the line _if_ the blamed commit did not change that line.
>
> This functionality was released in git version 1.6.3 in 2009-05-06.
> ---
>  NEWS  |    2 +
>  tig.c |   99 +++++++++++++++-------------------------------------------------
>  2 files changed, 25 insertions(+), 76 deletions(-)
>
>  I finally got some more time to dig around this. What if we simply uses
>  the information given by the porcelain output's previous line? It
>  handles your simple test case, and navigating in the tig repository. It
>  also makes it possible to delete a lot of code.
>
> diff --git a/NEWS b/NEWS
> index 190e5dc..499bdbc 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -38,6 +38,8 @@ Bug fixes:
>  - Fix unbind to behave as if the keybinding was never defined.
>  - Fix unbind to also cover built-in run requests.
>  - Fix parsing of unknown keymap names.
> + - Blame view: fix parent blame to detect renames. It uses "previous"
> +   line info from the blame porcelain output added in git version 1.6.3.
>
>  tig-0.15
>  --------
> diff --git a/tig.c b/tig.c
> index 01f48c3..044da28 100644
> --- a/tig.c
> +++ b/tig.c
> @@ -3977,73 +3977,6 @@ parse_author_line(char *ident, const char **author, struct time *time)
>        }
>  }
>
> -static bool
> -open_commit_parent_menu(char buf[SIZEOF_STR], int *parents)
> -{
> -       char rev[SIZEOF_REV];
> -       const char *revlist_argv[] = {
> -               "git", "log", "--no-color", "-1", "--pretty=format:%s", rev, NULL
> -       };
> -       struct menu_item *items;
> -       char text[SIZEOF_STR];
> -       bool ok = TRUE;
> -       int i;
> -
> -       items = calloc(*parents + 1, sizeof(*items));
> -       if (!items)
> -               return FALSE;
> -
> -       for (i = 0; i < *parents; i++) {
> -               string_copy_rev(rev, &buf[SIZEOF_REV * i]);
> -               if (!io_run_buf(revlist_argv, text, sizeof(text)) ||
> -                   !(items[i].text = strdup(text))) {
> -                       ok = FALSE;
> -                       break;
> -               }
> -       }
> -
> -       if (ok) {
> -               *parents = 0;
> -               ok = prompt_menu("Select parent", items, parents);
> -       }
> -       for (i = 0; items[i].text; i++)
> -               free((char *) items[i].text);
> -       free(items);
> -       return ok;
> -}
> -
> -static bool
> -select_commit_parent(const char *id, char rev[SIZEOF_REV], const char *path)
> -{
> -       char buf[SIZEOF_STR * 4];
> -       const char *revlist_argv[] = {
> -               "git", "log", "--no-color", "-1",
> -                       "--pretty=format:%P", id, "--", path, NULL
> -       };
> -       int parents;
> -
> -       if (!io_run_buf(revlist_argv, buf, sizeof(buf)) ||
> -           (parents = strlen(buf) / 40) < 0) {
> -               report("Failed to get parent information");
> -               return FALSE;
> -
> -       } else if (parents == 0) {
> -               if (path)
> -                       report("Path '%s' does not exist in the parent", path);
> -               else
> -                       report("The selected commit has no parents");
> -               return FALSE;
> -       }
> -
> -       if (parents == 1)
> -               parents = 0;
> -       else if (!open_commit_parent_menu(buf, &parents))
> -               return FALSE;
> -
> -       string_copy_rev(rev, &buf[41 * parents]);
> -       return TRUE;
> -}
> -
>  /*
>  * Pager backend
>  */
> @@ -4898,7 +4831,8 @@ struct blame_commit {
>        const char *author;             /* Author of the commit. */
>        struct time time;               /* Date from the author ident. */
>        char filename[128];             /* Name of file. */
> -       bool has_previous;              /* Was a "previous" line detected. */
> +       char parent_id[SIZEOF_REV];     /* Parent/previous SHA1 ID. */
> +       char parent_filename[128];      /* Parent/previous name of file. */
>  };
>
>  struct blame {
> @@ -5097,7 +5031,11 @@ blame_read(struct view *view, char *line)
>                string_ncopy(commit->title, line, strlen(line));
>
>        } else if (match_blame_header("previous ", &line)) {
> -               commit->has_previous = TRUE;
> +               if (strlen(line) <= SIZEOF_REV)
> +                       return FALSE;
> +               string_copy_rev(commit->parent_id, line);
> +               line += SIZEOF_REV;
> +               string_ncopy(commit->parent_filename, line, strlen(line));
>
>        } else if (match_blame_header("filename ", &line)) {
>                string_ncopy(commit->filename, line, strlen(line));
> @@ -5153,15 +5091,21 @@ check_blame_commit(struct blame *blame, bool check_null_id)
>  static void
>  setup_blame_parent_line(struct view *view, struct blame *blame)
>  {
> +       char from[SIZEOF_REF + SIZEOF_STR];
> +       char to[SIZEOF_REF + SIZEOF_STR];
>        const char *diff_tree_argv[] = {
> -               "git", "diff-tree", "-U0", blame->commit->id,
> -                       "--", blame->commit->filename, NULL
> +               "git", "diff", "--no-textconv", "--no-extdiff", "--no-color",
> +                       "-U0", from, to, "--", NULL
>        };
>        struct io io;
>        int parent_lineno = -1;
>        int blamed_lineno = -1;
>        char *line;
>
> +       snprintf(from, sizeof(from), "%s:%s", opt_ref, opt_file);
> +       snprintf(to, sizeof(to), "%s:%s", blame->commit->id,
> +                blame->commit->filename);
> +
>        if (!io_run(&io, IO_RD, NULL, diff_tree_argv))
>                return;
>
> @@ -5204,10 +5148,13 @@ blame_request(struct view *view, enum request request, struct line *line)
>                break;
>
>        case REQ_PARENT:
> -               if (check_blame_commit(blame, TRUE) &&
> -                   select_commit_parent(blame->commit->id, opt_ref,
> -                                        blame->commit->filename)) {
> -                       string_copy(opt_file, blame->commit->filename);
> +               if (!check_blame_commit(blame, TRUE))
> +                       break;
> +               if (!*blame->commit->parent_id) {
> +                       report("The selected commit has no parents");
> +               } else {
> +                       string_copy_rev(opt_ref, blame->commit->parent_id);
> +                       string_copy_rev(opt_file, blame->commit->parent_filename);
>                        setup_blame_parent_line(view, blame);
>                        open_view(view, REQ_VIEW_BLAME, OPEN_REFRESH);
>                }
> @@ -5228,7 +5175,7 @@ blame_request(struct view *view, enum request request, struct line *line)
>                                        "-C", "-M", "HEAD", "--", view->vid, NULL
>                        };
>
> -                       if (!blame->commit->has_previous) {
> +                       if (!*blame->commit->parent_id) {
>                                diff_index_argv[1] = "diff";
>                                diff_index_argv[2] = "--no-color";
>                                diff_index_argv[6] = "--";
> --
> 1.7.1.354.ge64bd
>
>



-- 
Jonas Fonseca

Re: [PATCH] Improve parent blame to detect renames by using the previous information

[Jeff King]

On Sat, Jun 05, 2010 at 03:56:05PM -0400, Jonas Fonseca wrote:

>  I finally got some more time to dig around this. What if we simply uses
>  the information given by the porcelain output's previous line? It
>  handles your simple test case, and navigating in the tig repository. It
>  also makes it possible to delete a lot of code.

Yes, I think that is the right way to go. The whole time I was doing the
other patches, I kept thinking that we had something like this in the
blame output, but when I looked I couldn't find it (which I can't see
how I would manage now, it's quite obvious to see).

So I think it does the right thing, and I see you also included my fix:

> +	char from[SIZEOF_REF + SIZEOF_STR];
> +	char to[SIZEOF_REF + SIZEOF_STR];
>  	const char *diff_tree_argv[] = {
> -		"git", "diff-tree", "-U0", blame->commit->id,
> -			"--", blame->commit->filename, NULL
> +		"git", "diff", "--no-textconv", "--no-extdiff", "--no-color",
> +			"-U0", from, to, "--", NULL
>  	};
>  	struct io io;
>  	int parent_lineno = -1;
>  	int blamed_lineno = -1;
>  	char *line;
>  
> +	snprintf(from, sizeof(from), "%s:%s", opt_ref, opt_file);
> +	snprintf(to, sizeof(to), "%s:%s", blame->commit->id,
> +		 blame->commit->filename);
> +

to handle the line-jumping properly.

One minor bug:

> @@ -5204,10 +5148,13 @@ blame_request(struct view *view, enum request request, struct line *line)
>  		break;
>  
>  	case REQ_PARENT:
> -		if (check_blame_commit(blame, TRUE) &&
> -		    select_commit_parent(blame->commit->id, opt_ref,
> -					 blame->commit->filename)) {
> -			string_copy(opt_file, blame->commit->filename);
> +		if (!check_blame_commit(blame, TRUE))
> +			break;
> +		if (!*blame->commit->parent_id) {
> +			report("The selected commit has no parents");
> +		} else {
> +			string_copy_rev(opt_ref, blame->commit->parent_id);
> +			string_copy_rev(opt_file, blame->commit->parent_filename);

This second string_copy_rev should be a string_ncopy, shouldn't it?

-Peff

Re: [PATCH] Improve parent blame to detect renames by using the previous information

[Jonas Fonseca]

On Sun, Jun 6, 2010 at 18:35, Jeff King <peff@peff.net> wrote:
>
> On Sat, Jun 05, 2010 at 03:56:05PM -0400, Jonas Fonseca wrote:
>
> One minor bug:
>
> > @@ -5204,10 +5148,13 @@ blame_request(struct view *view, enum request request, struct line *line)
> >               break;
> >
> >       case REQ_PARENT:
> > -             if (check_blame_commit(blame, TRUE) &&
> > -                 select_commit_parent(blame->commit->id, opt_ref,
> > -                                      blame->commit->filename)) {
> > -                     string_copy(opt_file, blame->commit->filename);
> > +             if (!check_blame_commit(blame, TRUE))
> > +                     break;
> > +             if (!*blame->commit->parent_id) {
> > +                     report("The selected commit has no parents");
> > +             } else {
> > +                     string_copy_rev(opt_ref, blame->commit->parent_id);
> > +                     string_copy_rev(opt_file, blame->commit->parent_filename);
>
> This second string_copy_rev should be a string_ncopy, shouldn't it?

Oh, yes, a regular copy and paste bug. Thanks for noticing. I will
include this and consider tagging another release.

--
Jonas Fonseca

Re: [PATCH] Improve parent blame to detect renames by using the previous information

[Jonas Fonseca]

On Sun, Jun 6, 2010 at 18:35, Jeff King <peff@peff.net> wrote:
> On Sat, Jun 05, 2010 at 03:56:05PM -0400, Jonas Fonseca wrote:
>
> So I think it does the right thing, and I see you also included my fix:
>
>> +     char from[SIZEOF_REF + SIZEOF_STR];
>> +     char to[SIZEOF_REF + SIZEOF_STR];
>>       const char *diff_tree_argv[] = {
>> -             "git", "diff-tree", "-U0", blame->commit->id,
>> -                     "--", blame->commit->filename, NULL
>> +             "git", "diff", "--no-textconv", "--no-extdiff", "--no-color",
>> +                     "-U0", from, to, "--", NULL
>>       };
>>       struct io io;
>>       int parent_lineno = -1;
>>       int blamed_lineno = -1;
>>       char *line;
>>
>> +     snprintf(from, sizeof(from), "%s:%s", opt_ref, opt_file);
>> +     snprintf(to, sizeof(to), "%s:%s", blame->commit->id,
>> +              blame->commit->filename);
>> +
>
> to handle the line-jumping properly.

Yes, I accidently squashed everything together in the patch I sent.
Before pushing the final version I move this part to a separate commit
with you as the author and made it use string_format.

-- 
Jonas Fonseca