{"thread":{"id":"8927","subject":"[PATCH] Fix \"git log --parent -m\" from emitting duplicated parent info","startedAt":"2007-07-08T14:37:50Z","lastAt":"2007-07-09T06:19:09Z","messageCount":10,"participants":["Marco Costalba","Junio C Hamano","Johannes Schindelin"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"46758","messageId":"e5bfff550707080737j56c161bdrf0c079b61877085b@mail.gmail.com","threadId":"8927","inReplyTo":null,"subject":"[PATCH] Fix \"git log --parent -m\" from emitting duplicated parent info","fromName":"Marco Costalba","fromEmail":"mcostalba@gmail.com","sentAt":"2007-07-08T14:37:50Z","receivedAt":"2007-07-08T14:37:50Z","isPatch":true,"sender":{"key":"mcostalba@gmail.com","avatar":null},"body":"Without this patch in case of a merge, duplicated parents are\nomitted in first line output, but still listed in following\nparents information.\n\nSigned-off-by: Marco Costalba \n---\n\nFollowing the hint of someone I submit this patch.\n\nPlease take it as a wish of not reverting the patch ;-)\n\n\n log-tree.c | 25 +++++++++++++++++--------\n revision.h | 1 +\n 2 files changed, 18 insertions(+), 8 deletions(-)\n\ndiff --git a/log-tree.c b/log-tree.c\nindex 24aea6b..e5d40fe 100644\n--- a/log-tree.c\n+++ b/log-tree.c\n@@ -6,11 +6,11 @@\n\n struct decoration name_decoration = { \"object names\" };\n\n-static void clear_tmp_mark(struct commit_list *p)\n+static void clear_flag(struct commit_list *p, unsigned int flag)\n {\n \twhile (p) {\n \t\tstruct commit *c = p->item;\n-\t\tc->object.flags &= ~TMP_MARK;\n+\t\tc->object.flags &= ~flag;\n \t\tp = p->next;\n \t}\n }\n@@ -23,7 +23,7 @@ static void show_parents(\n \t * be used locally, but the user should clean\n \t * things up after it is done with them.\n \t */\n-\tclear_tmp_mark(commit->parents);\n+\tclear_flag(commit->parents, TMP_MARK);\n \tfor (p = commit->parents; p ; p = p->next) {\n \t\tstruct commit *parent = p->item;\n \t\tif (parent->object.flags & TMP_MARK)\n@@ -31,7 +31,7 @@ static void show_parents(struct\n \t\tprintf(\" %s\", diff_unique_abbrev(parent->object.sha1, abbrev));\n \t\tparent->object.flags |= TMP_MARK;\n \t}\n-\tclear_tmp_mark(commit->parents);\n+\tclear_flag(commit->parents, TMP_MARK);\n }\n\n static void show_decorations(struct commit *commit)\n@@ -391,16 +391,24 @@ static int log_tree_diff(\n \t\t/* If we show individual diffs, show the parent info */\n \t\tlog->parent = parents->item;\n \t}\n-\n \tshowed_log = 0;\n+\tclear_flag(parents, TMP_MARK_2);\n+\n \tfor (;;) {\n \t\tstruct commit *parent = parents->item;\n\n-\t\tdiff_tree_sha1(parent->object.sha1, sha1, \"\", &opt->diffopt);\n-\t\tlog_tree_diff_flush(opt);\n+\t\t/* TMP_MARK_2 is a general purpose flag that can\n+\t\t * be used locally nested with TMP_MARK, but the user\n+\t\t * should clean things up after it is done with them.\n+\t\t*/\n+\t\tif (!opt->parents || !(parent->object.flags & TMP_MARK_2)) {\n\n-\t\tshowed_log |= !opt->loginfo;\n+\t\t\tdiff_tree_sha1(parent->object.sha1, sha1, \"\", &opt->diffopt);\n+\t\t\tlog_tree_diff_flush(opt); // will use TMP_MARK\n\n+\t\t\tshowed_log |= !opt->loginfo;\n+\t\t\tparent->object.flags |= TMP_MARK_2;\n+\t\t}\n \t\t/* Set up the log info for the next parent, if any.. */\n \t\tparents = parents->next;\n \t\tif (!parents)\n@@ -408,6 +416,7 @@ static int log_tree_diff(\n \t\tlog->parent = parents->item;\n \t\topt->loginfo = log;\n \t}\n+\tclear_flag(parents, TMP_MARK_2);\n \treturn showed_log;\n }\n\ndiff --git a/revision.h b/revision.h\nindex f46b4d5..403507f 100644\n--- a/revision.h\n+++ b/revision.h\n@@ -10,6 +10,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 TMP_MARK_2\t(1u<<9) /* for isolated cases; clean after use */\n\n struct rev_info;\n struct log_info;\n-- \n1.5.3.rc0.65.g39a4d-dirty\n"},{"id":"46775","messageId":"7v4pke3fx0.fsf@assigned-by-dhcp.cox.net","threadId":"8927","inReplyTo":"e5bfff550707080737j56c161bdrf0c079b61877085b@mail.gmail.com","subject":"Re: [PATCH] Fix \"git log --parent -m\" from emitting duplicated parent info","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2007-07-08T18:32:11Z","receivedAt":"2007-07-08T18:32:11Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Marco Costalba\" writes:\n\n> Without this patch in case of a merge, duplicated parents are\n> omitted in first line output, but still listed in following\n> parents information.\n>\n> Signed-off-by: Marco Costalba \n> ---\n>\n> Following the hint of someone I submit this patch.\n>\n> Please take it as a wish of not reverting the patch ;-)\n\nSorry, I am puzzled... which patch are you talking about\npossibly getting reverted?\n\nIn any case, the caller asked \"log\" to:\n\n 1. simplify the history, by giving pathspec,\n 2. show the list of parents,\n 3. show the diff with each of the parents.\n\nIf we omit dupilicated parents in the simplified view from 2.,\nwe should be consistent in the output from 3.; I think what your\npatch does makes sense.\n\nI'll look at it again later.\n"},{"id":"46809","messageId":"7vd4z2za2g.fsf@assigned-by-dhcp.cox.net","threadId":"8927","inReplyTo":"e5bfff550707080737j56c161bdrf0c079b61877085b@mail.gmail.com","subject":"Re: [PATCH] Fix \"git log --parent -m\" from emitting duplicated parent info","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2007-07-09T00:37:27Z","receivedAt":"2007-07-09T00:37:27Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"This is whitespace damaged in a funny way (some lines do not\nhave leading whitespace while some do).\n\nlog_tree_diff() hence log_tree_commit() is caled by codepaths\nother than \"git log -p\", and I am not convinced that this patch\nwould not break them. Worrysome.\n\nI wonder why we did not actually remove the duplicate parents\nfrom \"commit->parents\" list after history simplification when we\noriginally did 88494239, instead of filtering only on the output\npath. Anybody remember the reason?\n"},{"id":"46812","messageId":"Pine.LNX.4.64.0707090214280.4248@racer.site","threadId":"8927","inReplyTo":"7vd4z2za2g.fsf@assigned-by-dhcp.cox.net","subject":"Re: [PATCH] Fix \"git log --parent -m\" from emitting duplicated parent info","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2007-07-09T01:15:11Z","receivedAt":"2007-07-09T01:15:11Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Sun, 8 Jul 2007, Junio C Hamano wrote:\n\n> I wonder why we did not actually remove the duplicate parents\n> >from \"commit->parents\" list after history simplification when we\n> originally did 88494239, instead of filtering only on the output\n> path. Anybody remember the reason?\n\nI do not have that commit here.\n\nCiao,\nDscho\n"},{"id":"46811","messageId":"7v4pkez832.fsf@assigned-by-dhcp.cox.net","threadId":"8927","inReplyTo":"7vd4z2za2g.fsf@assigned-by-dhcp.cox.net","subject":"Re: [PATCH] Fix \"git log --parent -m\" from emitting duplicated parent info","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2007-07-09T01:20:17Z","receivedAt":"2007-07-09T01:20:17Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> This is whitespace damaged in a funny way (some lines do not\n> have leading whitespace while some do).\n>\n> log_tree_diff() hence log_tree_commit() is caled by codepaths\n> other than \"git log -p\", and I am not convinced that this patch\n> would not break them. Worrysome.\n>\n> I wonder why we did not actually remove the duplicate parents\n> from \"commit->parents\" list after history simplification when we\n> originally did 88494423, instead of filtering only on the output\n> path. Anybody remember the reason?\n\nIn any case, I think this patch would let us revert 88494239 and\nthe \"matching\" patch for git-log I sent out earlier, and also\nmakes your patch unnecessary.\n\nSeems to pass the test suite, and your original example of\nchecking addafaf with path limiter of \"diff.h\".\n\nI am a bit worried about the possible fallout, though. There\nmight be some callers that expect to find the same number of\nparents in commit->parents list and what is recorded in the\ncommit object itself, although I do not think of any, and I tend\nto think it is broken if there is one.\n\n-- >8 --\n[PATCH] remove duplicated parents after history simplification\n\nWhen we simplify history due to path limits, the parents list\nfor a rewritten commit can end up having duplicates. Instead of\nfiltering them out in the output codepath like earlier commit\n88494423 did, remove them much earlier, when the parent\nrewriting actually happens.\n\nSigned-off-by: Junio C Hamano \n---\n\n revision.c | 22 +++++++++++++++++++++-\n 1 files changed, 21 insertions(+), 1 deletions(-)\n\ndiff --git a/revision.c b/revision.c\nindex 5184716..b8a7571 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -1308,6 +1308,26 @@ static enum rewrite_result rewrite_one(struct rev_info *revs, struct commit **pp\n \t}\n }\n \n+static int remove_duplicate_parents(struct commit *commit)\n+{\n+\tstruct commit_list *p;\n+\tstruct commit_list **pp = &commit->parents;\n+\n+\t/* Examine existing parents while marking ones we have seen... */\n+\tfor (p = commit->parents; p; p = p->next) {\n+\t\tstruct commit *parent = p->item;\n+\t\tif (parent->object.flags & TMP_MARK)\n+\t\t\tcontinue;\n+\t\tparent->object.flags |= TMP_MARK;\n+\t\t*pp = p;\n+\t\tpp = &p->next;\n+\t}\n+\t/* ... and clear the temporary mark */\n+\tfor (p = commit->parents; p; p = p->next)\n+\t\tp->item->object.flags &= ~TMP_MARK;\n+\treturn 0;\n+}\n+\n static int rewrite_parents(struct rev_info *revs, struct commit *commit)\n {\n \tstruct commit_list **pp = &commit->parents;\n@@ -1324,7 +1344,7 @@ static int rewrite_parents(struct rev_info *revs, struct commit *commit)\n \t\t}\n \t\tpp = &parent->next;\n \t}\n-\treturn 0;\n+\treturn remove_duplicate_parents(commit);\n }\n \n static int commit_match(struct commit *commit, struct rev_info *opt)\n"},{"id":"46814","messageId":"Pine.LNX.4.64.0707090220281.4248@racer.site","threadId":"8927","inReplyTo":"7vd4z2za2g.fsf@assigned-by-dhcp.cox.net","subject":"Re: [PATCH] Fix \"git log --parent -m\" from emitting duplicated parent info","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2007-07-09T01:22:57Z","receivedAt":"2007-07-09T01:22:57Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nI only found this:\n\nhttp://thread.gmane.org/gmane.comp.version-control.git/29222\n\nCiao,\nDscho\n"},{"id":"46813","messageId":"7vwsxaxt5j.fsf@assigned-by-dhcp.cox.net","threadId":"8927","inReplyTo":"Pine.LNX.4.64.0707090214280.4248@racer.site","subject":"Re: [PATCH] Fix \"git log --parent -m\" from emitting duplicated parent info","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2007-07-09T01:28:08Z","receivedAt":"2007-07-09T01:28:08Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Sorry, not 88494239 but this one.\n\ncommit 884944239f2ab673cedfaa5e7999d31fd6a46331\nAuthor: Junio C Hamano \nDate: Sun Jan 29 15:24:42 2006 -0800\n\n rev-list: omit duplicated parents.\n \n Showing the same parent more than once for a commit does not\n make much sense downstream, so stop it.\n \n This can happen with an incorrectly made merge commit that\n merges the same parent twice, but can happen in an otherwise\n sane development history while squishing the history by taking\n into account only commits that touch specified paths.\n \n For example,\n \n \t$ git rev-list --max-count=1 --parents addafaf -- rev-list.c\n \n would have to show this commit ancestry graph:\n \n .---o---.\n / \\\n .---*---o---.\n / 93b74bc \\\n ---*---o---o-----o---o-----o addafaf\n d8f6b34 \\ /\n .---o---o---.\n \\ /\n .---*---.\n 3815f42\n \n where 5 independent development tracks, only two of which have\n changes in the specified paths since they forked. The last\n change for the other three development tracks was done by the\n same commit before they forked, and we were showing that three\n times.\n \n Signed-off-by: Junio C Hamano \n"},{"id":"46815","messageId":"7vps32xsof.fsf@assigned-by-dhcp.cox.net","threadId":"8927","inReplyTo":"Pine.LNX.4.64.0707090220281.4248@racer.site","subject":"Re: [PATCH] Fix \"git log --parent -m\" from emitting duplicated parent info","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2007-07-09T01:38:24Z","receivedAt":"2007-07-09T01:38:24Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> I only found this:\n>\n> http://thread.gmane.org/gmane.comp.version-control.git/29222\n\nThanks. As it happens, the patch I just sent out is a moral\nequivalent of your patch in that thread ;-)\n\nWe still may need to think about being careful upon --full-history\nthough.\n\nSecond try, this time not doing the simplification when --full-history\nis given.\n\n-- >8 --\n[PATCH] remove duplicated parents after history simplification\n\nWhen we simplify history due to path limits, the parents list\nfor a rewritten commit can end up having duplicates. Instead of\nfiltering them out in the output codepath like earlier commit\n88494423 did, remove them much earlier, when the parent\ninformation actually gets rewritten.\n\nSigned-off-by: Junio C Hamano \n---\n\n revision.c | 21 +++++++++++++++++++++\n 1 files changed, 21 insertions(+), 0 deletions(-)\n\ndiff --git a/revision.c b/revision.c\nindex 5184716..740e6a8 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -1308,6 +1308,25 @@ static enum rewrite_result rewrite_one(struct rev_info *revs, struct commit **pp\n \t}\n }\n \n+static void remove_duplicate_parents(struct commit *commit)\n+{\n+\tstruct commit_list *p;\n+\tstruct commit_list **pp = &commit->parents;\n+\n+\t/* Examine existing parents while marking ones we have seen... */\n+\tfor (p = commit->parents; p; p = p->next) {\n+\t\tstruct commit *parent = p->item;\n+\t\tif (parent->object.flags & TMP_MARK)\n+\t\t\tcontinue;\n+\t\tparent->object.flags |= TMP_MARK;\n+\t\t*pp = p;\n+\t\tpp = &p->next;\n+\t}\n+\t/* ... and clear the temporary mark */\n+\tfor (p = commit->parents; p; p = p->next)\n+\t\tp->item->object.flags &= ~TMP_MARK;\n+}\n+\n static int rewrite_parents(struct rev_info *revs, struct commit *commit)\n {\n \tstruct commit_list **pp = &commit->parents;\n@@ -1324,6 +1343,8 @@ static int rewrite_parents(struct rev_info *revs, struct commit *commit)\n \t\t}\n \t\tpp = &parent->next;\n \t}\n+\tif (revs->simplify_history)\n+\t\tremove_duplicate_parents(commit);\n \treturn 0;\n }\n \n"},{"id":"46833","messageId":"e5bfff550707082246k39286517o58fe89d80e05e08@mail.gmail.com","threadId":"8927","inReplyTo":"7vps32xsof.fsf@assigned-by-dhcp.cox.net","subject":"Re: [PATCH] Fix \"git log --parent -m\" from emitting duplicated parent info","fromName":"Marco Costalba","fromEmail":"mcostalba@gmail.com","sentAt":"2007-07-09T05:46:10Z","receivedAt":"2007-07-09T05:46:10Z","isPatch":true,"sender":{"key":"mcostalba@gmail.com","avatar":null},"body":"On 7/9/07, Junio C Hamano wrote:\n> Johannes Schindelin writes:\n>\n> Second try, this time not doing the simplification when --full-history\n> is given.\n>\n\nProbably I'm wrong but I would think the first original version with\npost-filtering is more\ncorrect.\n\nIndeed we can use and _do_ use --full-history not only for printing\nall revisions but to _walk_ the tree in a certain way, different from\ndefault.\n\nIMHO --parents + --full-history is absolutely legal because the first\nflag indicate how to walk the tree and the second flag indicate what\nto show to the user.\n\nI agree the original patch is much uglier then your last version but I\nwould say is more correct.\n\nThanks\nMarco\n"},{"id":"46834","messageId":"7vodimw142.fsf@assigned-by-dhcp.cox.net","threadId":"8927","inReplyTo":"e5bfff550707082246k39286517o58fe89d80e05e08@mail.gmail.com","subject":"Re: [PATCH] Fix \"git log --parent -m\" from emitting duplicated parent info","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2007-07-09T06:19:09Z","receivedAt":"2007-07-09T06:19:09Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Marco Costalba\" writes:\n\n> Indeed we can use and _do_ use --full-history not only for printing\n> all revisions but to _walk_ the tree in a certain way, different from\n> default.\n\nYes, --full-history essentially tells the machnery to walk all\nside branches even when a merge takes all the specified paths\nfrom one parent's version. You will end up walking all five\nbranches from the first pentapus \"addafaf\" in git.git history,\neven if history simplification with pathspec would have narrowed\nit down to three paths if you did not give that option.\n\n> IMHO --parents + --full-history is absolutely legal because the first\n> flag indicate how to walk the tree and the second flag indicate what\n> to show to the user.\n\nI think we are in agreement. You are telling the machinery to\nwalk all five branches with --full-history. But you are not\ntelling the machinery to keep the intermediate commits (i.e. you\ndo not give --sparse), so these five branches can be simplified\nto lead to the same parent. I think I was confused by dense vs\nsimplify_history when I wrote that message in the thread\nJohannes pointed out:\n\n\thttp://thread.gmane.org/gmane.comp.version-control.git/29222\n\nMaking the duplicate parent removal depend on \"dense\" makes\nsense, but the rewrite_parent() codepath is used only when dense\nanyway.\n\nThanks for a timely objection and sanity checking.\n"}]}