[BUG] gitk: breaks with both version and file limits

[BUG] gitk: breaks with both version and file limits

[Yann Dirson]

As an example, on a current git tree, the following command triggers
an 'Error: expected boolean value but got ""' dialog when scrolling to
the bottom of the graph, and leaves the bottom of the graph not drawn.
This happens with current master, but not with 1.2.4.

gitk --all ^v1.0.6 -- Makefile 

While checking with git-bisect while limiting to path gitk, I noticed
the amusing detail that "git-bisect visualize" is impacted by this bug.

git-bisect shows that the commit triggering the problem is
79b2c75e043ad85f9a6b1a8d890b601a2f761a0e ("gitk: replace parent and
children arrays with lists" on Apr 2)

Hope this helps,
-- 
Yann Dirson    <ydirson@altern.org> |
Debian-related: <dirson@debian.org> |   Support Debian GNU/Linux:
                                    |  Freedom, Power, Stability, Gratis
     http://ydirson.free.fr/        | Check <http://www.debian.org/>

Re: [BUG] gitk: breaks with both version and file limits

[Junio C Hamano]

Yann Dirson <ydirson@altern.org> writes:

> As an example, on a current git tree, the following command triggers
> an 'Error: expected boolean value but got ""' dialog when scrolling to
> the bottom of the graph, and leaves the bottom of the graph not drawn.
> This happens with current master, but not with 1.2.4.

A workaround errorproofing would be to do something like this
(note that I do not really speak Tcl).

The real issue is "git-rev-list --boundary ran..ge -- path" does
not show boundary.  I am not sure if it even worked in the past;
will take a look.

-- >8 --
gitk: [lindex $list $no_such_idx] returns "" which is not a valid bool

When asking for commit that is not listed for some reason in
commitlisted, gitk barfed because return value from lindex was
"" which was not a bool acceptable in ($bool ? A : B) construct.

We are expecting "1" here for listed commits (this comes from
getcommitlines), so explicitly check for it.

Signed-off-by: Junio C Hamano <junkio@cox.net>
---
diff --git a/gitk b/gitk
index f88c06e..5d2b0cc 100755
--- a/gitk
+++ b/gitk
@@ -1460,7 +1460,7 @@ proc drawcmittext {id row col rmx} {
     global linehtag linentag linedtag
     global mainfont namefont canvxmax
 
-    set ofill [expr {[lindex $commitlisted $row]? "blue": "white"}]
+    set ofill [expr {("1" == [lindex $commitlisted $row]) ? "blue" : "white"}]
     set x [xc $row $col]
     set y [yc $row]
     set orad [expr {$linespc / 3}]

Re: [BUG] gitk: breaks with both version and file limits

[Paul Mackerras]

Junio C Hamano writes:

> The real issue is "git-rev-list --boundary ran..ge -- path" does
> not show boundary.  I am not sure if it even worked in the past;
> will take a look.

Ah... and in addextraid I wasn't appending a 0 to commitlisted, so
commitlisted got out of sync with the displayorder list.  I just
committed this patch, which keeps them in sync.  I think that's
preferable to working around the missing elements.

Paul.

diff --git a/gitk b/gitk
index f88c06e..87e7162 100755
--- a/gitk
+++ b/gitk
@@ -1116,11 +1116,12 @@ proc layoutrows {row endrow last} {
 
 proc addextraid {id row} {
     global displayorder commitrow commitinfo
-    global commitidx
+    global commitidx commitlisted
     global parentlist childlist children
 
     incr commitidx
     lappend displayorder $id
+    lappend commitlisted 0
     lappend parentlist {}
     set commitrow($id) $row
     readcommit $id
@@ -1500,7 +1501,7 @@ proc drawcmittext {id row col rmx} {
 proc drawcmitrow {row} {
     global displayorder rowidlist
     global idrowranges idrangedrawn iddrawn
-    global commitinfo commitlisted parentlist numcommits
+    global commitinfo parentlist numcommits
 
     if {$row >= $numcommits} return
     foreach id [lindex $rowidlist $row] {

[PATCH] rev-list --boundary: show boundary commits even when limited otherwise.

[Junio C Hamano]

The boundary commits are shown for UI like gitk to draw them as
soon as topo-order sorting allows, and should not be omitted by
get_revision() filtering logic.  As long as their immediate
child commits are shown, we should not filter them out.

Signed-off-by: Junio C Hamano <junkio@cox.net>

---

  > The real issue is "git-rev-list --boundary ran..ge -- path" does
  > not show boundary.  I am not sure if it even worked in the past;
  > will take a look.

  I _think_ this fixes it, but this is rev-list which I feel a
  bit uneasy touching.  A lower-impact approach of not filtering
  BOUNDARY commits at all does not work well; it  shows many
  isolated open-circle commits in gitk that do not touch the
  specified paths.

  I got tired of updating the private object.flags users, so
  just reserved the lower 16 bits for use of revision.c while I
  was at it.

 http-push.c |   10 +++++-----
 rev-list.c  |    4 ++--
 revision.c  |   29 +++++++++++++++++++++++++++--
 revision.h  |    3 ++-
 4 files changed, 36 insertions(+), 10 deletions(-)

298196646f35b6f5bc2131074204826d39aff8cf
diff --git a/http-push.c b/http-push.c
index 19a0f77..e7f7f44 100644
--- a/http-push.c
+++ b/http-push.c
@@ -60,12 +60,12 @@ #define LOCK_REQUEST "<?xml version=\"1.
 #define LOCK_TIME 600
 #define LOCK_REFRESH 30
 
-/* bits #0-6 in revision.h */
+/* bits #0-15 in revision.h */
 
-#define LOCAL    (1u << 7)
-#define REMOTE   (1u << 8)
-#define FETCHING (1u << 9)
-#define PUSHING  (1u << 10)
+#define LOCAL    (1u<<16)
+#define REMOTE   (1u<<17)
+#define FETCHING (1u<<18)
+#define PUSHING  (1u<<19)
 
 /* We allow "recursive" symbolic refs. Only within reason, though */
 #define MAXDEPTH 5
diff --git a/rev-list.c b/rev-list.c
index 963707a..f5511e7 100644
--- a/rev-list.c
+++ b/rev-list.c
@@ -8,9 +8,9 @@ #include "tree-walk.h"
 #include "diff.h"
 #include "revision.h"
 
-/* bits #0-6 in revision.h */
+/* bits #0-15 in revision.h */
 
-#define COUNTED		(1u<<7)
+#define COUNTED		(1u<<16)
 
 static const char rev_list_usage[] =
 "git-rev-list [OPTION] <commit-id>... [ -- paths... ]\n"
diff --git a/revision.c b/revision.c
index 0505f3f..e1f9816 100644
--- a/revision.c
+++ b/revision.c
@@ -750,6 +750,17 @@ static void rewrite_parents(struct rev_i
 	}
 }
 
+static void mark_boundary_to_show(struct commit *commit)
+{
+	struct commit_list *p = commit->parents;
+	while (p) {
+		commit = p->item;
+		p = p->next;
+		if (commit->object.flags & BOUNDARY)
+			commit->object.flags |= BOUNDARY_SHOW;
+	}
+}
+
 struct commit *get_revision(struct rev_info *revs)
 {
 	struct commit_list *list = revs->commits;
@@ -787,8 +798,20 @@ struct commit *get_revision(struct rev_i
 		}
 		if (commit->object.flags & SHOWN)
 			continue;
-		if (!(commit->object.flags & BOUNDARY) &&
-		    (commit->object.flags & UNINTERESTING))
+
+		/* We want to show boundary commits only when their
+		 * children are shown.  When path-limiter is in effect,
+		 * rewrite_parents() drops some commits from getting shown,
+		 * and there is no point showing boundary parents that
+		 * are not shown.  After rewrite_parents() rewrites the
+		 * parents of a commit that is shown, we mark the boundary
+		 * parents with BOUNDARY_SHOW.
+		 */
+		if (commit->object.flags & BOUNDARY_SHOW) {
+			commit->object.flags |= SHOWN;
+			return commit;
+		}
+		if (commit->object.flags & UNINTERESTING)
 			continue;
 		if (revs->min_age != -1 && (commit->date > revs->min_age))
 			continue;
@@ -801,6 +824,8 @@ struct commit *get_revision(struct rev_i
 			if (revs->parents)
 				rewrite_parents(revs, commit);
 		}
+		if (revs->boundary)
+			mark_boundary_to_show(commit);
 		commit->object.flags |= SHOWN;
 		return commit;
 	} while (revs->commits);
diff --git a/revision.h b/revision.h
index 8970b57..4b27043 100644
--- a/revision.h
+++ b/revision.h
@@ -7,7 +7,8 @@ #define TREECHANGE	(1u<<2)
 #define SHOWN		(1u<<3)
 #define TMP_MARK	(1u<<4) /* for isolated cases; clean after use */
 #define BOUNDARY	(1u<<5)
-#define ADDED		(1u<<6)	/* Parents already parsed and added? */
+#define BOUNDARY_SHOW	(1u<<6)
+#define ADDED		(1u<<7)	/* Parents already parsed and added? */
 
 struct rev_info;
 
-- 
1.3.0.rc4.g878b