{"thread":{"id":"14607","subject":"[PATCH 1/2] Add test to show that show-branch misses out the 8th column","startedAt":"2008-07-23T00:50:35Z","lastAt":"2008-07-23T22:07:15Z","messageCount":8,"participants":["Johannes Schindelin","Junio C Hamano","Jon Loeliger","Petr Baudis"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"84417","messageId":"alpine.DEB.1.00.0807230148130.8986@racer","threadId":"14607","inReplyTo":null,"subject":"[PATCH 1/2] Add test to show that show-branch misses out the 8th column","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2008-07-23T00:50:35Z","receivedAt":"2008-07-23T00:50:35Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"\nNoticed by Pasky.\n\nSigned-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>\n---\n t/t3202-show-branch-octopus.sh |   59 ++++++++++++++++++++++++++++++++++++++++\n 1 files changed, 59 insertions(+), 0 deletions(-)\n create mode 100755 t/t3202-show-branch-octopus.sh\n\ndiff --git a/t/t3202-show-branch-octopus.sh b/t/t3202-show-branch-octopus.sh\nnew file mode 100755\nindex 0000000..8d50c23\n--- /dev/null\n+++ b/t/t3202-show-branch-octopus.sh\n@@ -0,0 +1,59 @@\n+#!/bin/sh\n+\n+test_description='test show-branch with more than 8 heads'\n+\n+. ./test-lib.sh\n+\n+numbers=\"1 2 3 4 5 6 7 8 9 10\"\n+\n+test_expect_success 'setup' '\n+\n+\t> file &&\n+\tgit add file &&\n+\ttest_tick &&\n+\tgit commit -m initial &&\n+\n+\tfor i in $numbers\n+\tdo\n+\t\tgit checkout -b branch$i master &&\n+\t\t> file$i &&\n+\t\tgit add file$i &&\n+\t\ttest_tick &&\n+\t\tgit commit -m branch$i || break\n+\tdone\n+\n+'\n+\n+cat > expect << EOF\n+! [branch1] branch1\n+ ! [branch2] branch2\n+  ! [branch3] branch3\n+   ! [branch4] branch4\n+    ! [branch5] branch5\n+     ! [branch6] branch6\n+      ! [branch7] branch7\n+       ! [branch8] branch8\n+        ! [branch9] branch9\n+         * [branch10] branch10\n+----------\n+         * [branch10] branch10\n+        +  [branch9] branch9\n+       +   [branch8] branch8\n+      +    [branch7] branch7\n+     +     [branch6] branch6\n+    +      [branch5] branch5\n+   +       [branch4] branch4\n+  +        [branch3] branch3\n+ +         [branch2] branch2\n++          [branch1] branch1\n++++++++++* [branch10^] initial\n+EOF\n+\n+test_expect_failure 'show-branch with more than 8 branches' '\n+\n+\tgit show-branch $(for i in $numbers; do echo branch$i; done) > out &&\n+\ttest_cmp expect out\n+\n+'\n+\n+test_done\n-- \n1.6.0.rc0.22.gf2096d.dirty\n"},{"id":"84418","messageId":"alpine.DEB.1.00.0807230150480.8986@racer","threadId":"14607","inReplyTo":"alpine.DEB.1.00.0807230148130.8986@racer","subject":"[PATCH 2/2] sort_in_topological_order(): avoid setting a commit flag","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2008-07-23T00:51:36Z","receivedAt":"2008-07-23T00:51:36Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"\nWe used to set the TOPOSORT flag of commits during the topological\nsorting, but we can just as well use the member \"indegree\" for it:\nindegree is now incremented by 1 in the cases where the commit used\nto have the TOPOSORT flag.\n\nThis is the same behavior as before, since indegree could not be\nnon-zero when TOPOSORT was unset.\n\nIncidentally, this fixes the bug in show-branch where the 8th column\nwas not shown: show-branch sorts the commits in topological order,\nassuming that all the commit flags are available for show-branch's\nprivate matters.\n\nBut this was not true: TOPOSORT was identical to the flag corresponding\nto the 8th ref.  So the flags for the 8th column were unset by the\ntopological sorting.\n\nSigned-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>\n---\n\n\tThis is another late-night patch done by yours-truly.  However,\n\tI tried extra hard to make sure that every occurrence of\n\tindegree was properly changed, and I am pretty certain that\n\tthe reasoning with the unset TOPOSORT is correct.\n\n\tBut please check (I know, not necessary to ask for extra review\n\tfor my patches,\tbut nevertheless).\n\n commit.c                       |   13 ++++++-------\n revision.h                     |    3 +--\n t/t3202-show-branch-octopus.sh |    2 +-\n 3 files changed, 8 insertions(+), 10 deletions(-)\n\ndiff --git a/commit.c b/commit.c\nindex 5148ec5..9dacfb8 100644\n--- a/commit.c\n+++ b/commit.c\n@@ -436,8 +436,7 @@ void sort_in_topological_order(struct commit_list ** list, int lifo)\n \t/* Mark them and clear the indegree */\n \tfor (next = orig; next; next = next->next) {\n \t\tstruct commit *commit = next->item;\n-\t\tcommit->object.flags |= TOPOSORT;\n-\t\tcommit->indegree = 0;\n+\t\tcommit->indegree = 1;\n \t}\n \n \t/* update the indegree */\n@@ -446,7 +445,7 @@ void sort_in_topological_order(struct commit_list ** list, int lifo)\n \t\twhile (parents) {\n \t\t\tstruct commit *parent = parents->item;\n \n-\t\t\tif (parent->object.flags & TOPOSORT)\n+\t\t\tif (parent->indegree)\n \t\t\t\tparent->indegree++;\n \t\t\tparents = parents->next;\n \t\t}\n@@ -464,7 +463,7 @@ void sort_in_topological_order(struct commit_list ** list, int lifo)\n \tfor (next = orig; next; next = next->next) {\n \t\tstruct commit *commit = next->item;\n \n-\t\tif (!commit->indegree)\n+\t\tif (commit->indegree == 1)\n \t\t\tinsert = &commit_list_insert(commit, insert)->next;\n \t}\n \n@@ -486,7 +485,7 @@ void sort_in_topological_order(struct commit_list ** list, int lifo)\n \t\tfor (parents = commit->parents; parents ; parents = parents->next) {\n \t\t\tstruct commit *parent=parents->item;\n \n-\t\t\tif (!(parent->object.flags & TOPOSORT))\n+\t\t\tif (!parent->indegree)\n \t\t\t\tcontinue;\n \n \t\t\t/*\n@@ -494,7 +493,8 @@ void sort_in_topological_order(struct commit_list ** list, int lifo)\n \t\t\t * when all their children have been emitted thereby\n \t\t\t * guaranteeing topological order.\n \t\t\t */\n-\t\t\tif (!--parent->indegree) {\n+\t\t\tif (--parent->indegree == 1) {\n+\t\t\t\tparent->indegree = 0;\n \t\t\t\tif (!lifo)\n \t\t\t\t\tinsert_by_date(parent, &work);\n \t\t\t\telse\n@@ -505,7 +505,6 @@ void sort_in_topological_order(struct commit_list ** list, int lifo)\n \t\t * work_item is a commit all of whose children\n \t\t * have already been emitted. we can emit it now.\n \t\t */\n-\t\tcommit->object.flags &= ~TOPOSORT;\n \t\t*pptr = work_item;\n \t\tpptr = &work_item->next;\n \t}\ndiff --git a/revision.h b/revision.h\nindex fa68c65..f64e8ce 100644\n--- a/revision.h\n+++ b/revision.h\n@@ -12,8 +12,7 @@\n #define CHILD_SHOWN\t(1u<<6)\n #define ADDED\t\t(1u<<7)\t/* Parents already parsed and added? */\n #define SYMMETRIC_LEFT\t(1u<<8)\n-#define TOPOSORT\t(1u<<9)\t/* In the active toposort list.. */\n-#define ALL_REV_FLAGS\t((1u<<10)-1)\n+#define ALL_REV_FLAGS\t((1u<<9)-1)\n \n struct rev_info;\n struct log_info;\ndiff --git a/t/t3202-show-branch-octopus.sh b/t/t3202-show-branch-octopus.sh\nindex 8d50c23..7fe4a6e 100755\n--- a/t/t3202-show-branch-octopus.sh\n+++ b/t/t3202-show-branch-octopus.sh\n@@ -49,7 +49,7 @@ cat > expect << EOF\n +++++++++* [branch10^] initial\n EOF\n \n-test_expect_failure 'show-branch with more than 8 branches' '\n+test_expect_success 'show-branch with more than 8 branches' '\n \n \tgit show-branch $(for i in $numbers; do echo branch$i; done) > out &&\n \ttest_cmp expect out\n-- \n1.6.0.rc0.22.gf2096d.dirty\n"},{"id":"84420","messageId":"7v7ibdifbp.fsf@gitster.siamese.dyndns.org","threadId":"14607","inReplyTo":"alpine.DEB.1.00.0807230150480.8986@racer","subject":"Re: [PATCH 2/2] sort_in_topological_order(): avoid setting a commit flag","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-07-23T01:01:30Z","receivedAt":"2008-07-23T01:01:30Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:\n\n> This is the same behavior as before, since indegree could not be\n> non-zero when TOPOSORT was unset.\n>\n> Incidentally, this fixes the bug in show-branch where the 8th column\n> was not shown: show-branch sorts the commits in topological order,\n> assuming that all the commit flags are available for show-branch's\n> private matters.\n\nDo people still actively use show-branch as a G/CUI, especially after that\n\"log --graph\" thing was introduced?\n\nIf that is the case, it might also make sense to stop using the object\nflags but allocate necessary number of bits (not restricted to 25 or so)\npointed at by commit->util field to remove its limitation.\n\nHint, hint...\n"},{"id":"84523","messageId":"488750CB.1040400@freescale.com","threadId":"14607","inReplyTo":"7v7ibdifbp.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH 2/2] sort_in_topological_order(): avoid setting a commit flag","fromName":"Jon Loeliger","fromEmail":"jdl@freescale.com","sentAt":"2008-07-23T15:39:55Z","receivedAt":"2008-07-23T15:39:55Z","isPatch":true,"sender":{"key":"jdl@jdl.com","avatar":"https://gravatar.com/avatar/75ce9a10b151acd2c28ec4ab2136dba7b2ff1634530bd04b155981a749d08a64?d=mp&s=160"},"body":"Junio C Hamano wrote:\n> \n> Do people still actively use show-branch as a G/CUI, especially after that\n> \"log --graph\" thing was introduced?\n\nAt the risk of sounding Old School, yes.\n\nWhile the \"log --graph\" thing is Really Slick, it has\ngrumbly-factors, IMO.  First, I have to set up an alias\nall the time, as \"git log --graph --pretty=oneline\" is\ngrumpy typing.  Second, I always have to widen my screen\nto accommodate reasonable looking output or colrm it.\n(Having it self-colrm to window-width would be nice.)\n(Sure, --abbrev-commit too, but see \"First,\" above. :-))\nFinally, I frequently like seeing a self-limited history\nwhen all the branches reach their common ancestor.\n\nHTH,\njdl\n"},{"id":"84577","messageId":"7vprp4ctkp.fsf@gitster.siamese.dyndns.org","threadId":"14607","inReplyTo":"alpine.DEB.1.00.0807230150480.8986@racer","subject":"Re: [PATCH 2/2] sort_in_topological_order(): avoid setting a commit flag","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-07-23T19:02:30Z","receivedAt":"2008-07-23T19:02:30Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:\n\n> @@ -494,7 +493,8 @@ void sort_in_topological_order(struct commit_list ** list, int lifo)\n>  \t\t\t * when all their children have been emitted thereby\n>  \t\t\t * guaranteeing topological order.\n>  \t\t\t */\n> -\t\t\tif (!--parent->indegree) {\n> +\t\t\tif (--parent->indegree == 1) {\n> +\t\t\t\tparent->indegree = 0;\n>  \t\t\t\tif (!lifo)\n>  \t\t\t\t\tinsert_by_date(parent, &work);\n>  \t\t\t\telse\n> @@ -505,7 +505,6 @@ void sort_in_topological_order(struct commit_list ** list, int lifo)\n>  \t\t * work_item is a commit all of whose children\n>  \t\t * have already been emitted. we can emit it now.\n>  \t\t */\n> -\t\tcommit->object.flags &= ~TOPOSORT;\n>  \t\t*pptr = work_item;\n>  \t\tpptr = &work_item->next;\n>  \t}\n\nThese two hunks look suspicious.\n\nThe \"tips\" used to enter that while() loop with zero indegree, its parents\nexamined and then entered the final list pointed by pptr with the toposort\nscratch variables removed and indegree set to zero.  Now with the new +1\nbased code, they enter the while() loop with 1 indegree, and enter the\nfinal list with indegree set to 1.\n\nA parent that has only one child that is \"tip\" is discovered in the\nwhile() loop, its indegree decremented (so it goes down to zero in the\noriginal code and 1 in yours) and enters work queue to be processed.  It\nused to have the toposort scratch variable removed in the second hunk\nabove, but that is done in the first hunk in your version.\n\nSo after this patch, indegree will be all zero for non-tip commits but\nwill be one for tip commits.  Is this intended?\n\nI'd suggest dropping the \"parent->indegree = 0\" assignment and turn the\nsecond hunk into \"commit->indgree = 0\" assignment.\n"},{"id":"84590","messageId":"alpine.DEB.1.00.0807232014260.8986@racer","threadId":"14607","inReplyTo":"7vprp4ctkp.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH 2/2] sort_in_topological_order(): avoid setting a commit flag","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2008-07-23T19:33:53Z","receivedAt":"2008-07-23T19:33:53Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Wed, 23 Jul 2008, Junio C Hamano wrote:\n\n> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:\n> \n> > @@ -494,7 +493,8 @@ void sort_in_topological_order(struct commit_list ** list, int lifo)\n> >  \t\t\t * when all their children have been emitted thereby\n> >  \t\t\t * guaranteeing topological order.\n> >  \t\t\t */\n> > -\t\t\tif (!--parent->indegree) {\n> > +\t\t\tif (--parent->indegree == 1) {\n> > +\t\t\t\tparent->indegree = 0;\n> >  \t\t\t\tif (!lifo)\n> >  \t\t\t\t\tinsert_by_date(parent, &work);\n> >  \t\t\t\telse\n> > @@ -505,7 +505,6 @@ void sort_in_topological_order(struct commit_list ** list, int lifo)\n> >  \t\t * work_item is a commit all of whose children\n> >  \t\t * have already been emitted. we can emit it now.\n> >  \t\t */\n> > -\t\tcommit->object.flags &= ~TOPOSORT;\n> >  \t\t*pptr = work_item;\n> >  \t\tpptr = &work_item->next;\n> >  \t}\n> \n> These two hunks look suspicious.\n> \n> The \"tips\" used to enter that while() loop with zero indegree, its \n> parents examined and then entered the final list pointed by pptr with \n> the toposort scratch variables removed and indegree set to zero.  Now \n> with the new +1 based code, they enter the while() loop with 1 indegree, \n> and enter the final list with indegree set to 1.\n\nAlmost correct.  The way I did it the if() is entered with indegree == \n1, but is set indegree to 0 right away.\n\nI did it this way because of these two lines before the if():\n\n                        if (!parent->indegree)\n                                continue;\n\nThese are the replacement for the previous\n\n\t\t\tif (!(parent->object.flags & TOPOSORT))\n                                continue;\n\nNow, if indegree was not set to 0, that if () would not trigger, but in \nthe next one (the first hunk you quoted), indegree was decremented and \nfailed the test == 1.\n\nHowever, that is correct only by pure chance; I certainly missed that.  \nThe correct fix according to my thinking would be to set the indegree to 0 \nwhen the tips are inserted, too.\n\n> A parent that has only one child that is \"tip\" is discovered in the \n> while() loop, its indegree decremented (so it goes down to zero in the \n> original code and 1 in yours) and enters work queue to be processed.  \n> It used to have the toposort scratch variable removed in the second hunk \n> above, but that is done in the first hunk in your version.\n> \n> So after this patch, indegree will be all zero for non-tip commits but\n> will be one for tip commits.  Is this intended?\n\nNo.\n\n> I'd suggest dropping the \"parent->indegree = 0\" assignment and turn the\n> second hunk into \"commit->indgree = 0\" assignment.\n\nYeah, that is much simpler.\n\nThanks,\nDscho\n"},{"id":"84620","messageId":"20080723214942.GZ10151@machine.or.cz","threadId":"14607","inReplyTo":"7v7ibdifbp.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH 2/2] sort_in_topological_order(): avoid setting a commit flag","fromName":"Petr Baudis","fromEmail":"pasky@suse.cz","sentAt":"2008-07-23T21:49:42Z","receivedAt":"2008-07-23T21:49:42Z","isPatch":true,"sender":{"key":"pasky@ucw.cz","avatar":"https://avatars.githubusercontent.com/u/18439?v=4"},"body":"On Tue, Jul 22, 2008 at 06:01:30PM -0700, Junio C Hamano wrote:\n> Do people still actively use show-branch as a G/CUI, especially after that\n> \"log --graph\" thing was introduced?\n\nTo me, show-branch is just more convenient to use; I can see more easily\nwhich patches are with which branches, which is useful especially for my\nnew sick-twisted use of feature branches for individual patches, thus\nhaving a lot of interdependencies.\n\n> If that is the case, it might also make sense to stop using the object\n> flags but allocate necessary number of bits (not restricted to 25 or so)\n> pointed at by commit->util field to remove its limitation.\n> \n> Hint, hint...\n\nMaybe I will hit it soon... ;-)\n\n-- \n\t\t\t\tPetr \"Pasky\" Baudis\nAs in certain cults it is possible to kill a process if you know\nits true name.  -- Ken Thompson and Dennis M. Ritchie\n"},{"id":"84627","messageId":"7vd4l4b6gc.fsf@gitster.siamese.dyndns.org","threadId":"14607","inReplyTo":"20080723214942.GZ10151@machine.or.cz","subject":"Re: [PATCH 2/2] sort_in_topological_order(): avoid setting a commit flag","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-07-23T22:07:15Z","receivedAt":"2008-07-23T22:07:15Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Petr Baudis <pasky@suse.cz> writes:\n\n> On Tue, Jul 22, 2008 at 06:01:30PM -0700, Junio C Hamano wrote:\n>> Do people still actively use show-branch as a G/CUI, especially after that\n>> \"log --graph\" thing was introduced?\n>\n> To me, show-branch is just more convenient to use; I can see more easily\n> which patches are with which branches, which is useful especially for my\n> new sick-twisted use of feature branches for individual patches, thus\n> having a lot of interdependencies.\n\nHeh, I still recall hearing from many people that its output is hard to\ndecipher and UI is unintuitive.  What changed their mind, I have to\nwonder...\n"}]}