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

[Jonas Fonseca <fonseca@diku.dk>]

[-- 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]);