{"thread":{"id":"34510","subject":"git log anomalities","startedAt":"2013-07-22T09:08:54Z","lastAt":"2013-07-31T22:55:21Z","messageCount":9,"participants":["Uwe Kleine-König","Thomas Rast","Junio C Hamano","Jeff King"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"223914","messageId":"20130722090854.GA22222@pengutronix.de","threadId":"34510","inReplyTo":null,"subject":"git log anomalities","fromName":"Uwe Kleine-König","fromEmail":"u.kleine-koenig@pengutronix.de","sentAt":"2013-07-22T09:08:54Z","receivedAt":"2013-07-22T09:08:54Z","isPatch":false,"sender":{"key":"u.kleine-koenig@pengutronix.de","avatar":"https://gravatar.com/avatar/354b5e3ceb2806a2f1e1e382ac29ddbdad18288654da62b61eb13583a857eee7?d=mp&s=160"},"body":"Hello,\n\ntoday I looked at the changes to drivers/net/ethernet/freescale/fec.c in\nthe kernel since v3.8 using\n\n\tgit log --stat v3.8.. --full-diff -- drivers/net/ethernet/freescale/fec.c\n\nwhich looks as expected. But when I added --graph the diffstats change.\nE.g. for 793fc0964be1921f15a44be58b066f22b925d90b it changes from:\n\n drivers/net/ethernet/freescale/Makefile | 3 +-\n drivers/net/ethernet/freescale/fec.c | 1966 -----------------------------\n drivers/net/ethernet/freescale/fec_main.c | 1966 +++++++++++++++++++++++++++++\n drivers/net/ethernet/freescale/fec_ptp.c | 3 -\n 4 files changed, 1968 insertions(+), 1970 deletions(-)\n\nto\n\n| Documentation/devicetree/bindings/net/dsa/dsa.txt | 91 +\n| .../bindings/net/marvell-orion-mdio.txt | 3 +\n| Documentation/networking/ip-sysctl.txt | 35 +-\n| MAINTAINERS | 2 +-\n| arch/arm/net/bpf_jit_32.c | 5 +-\n| arch/arm/plat-orion/common.c | 54 +-\n| arch/powerpc/net/bpf_jit_comp.c | 12 +-\n| ...\n| 404 files changed, 15373 insertions(+), 8563 deletions(-)\n\nIs that a bug, or a feature I don't understand?\n\n(I'm using Debian's 1:1.8.3.2-1)\n\nBest regards\nUwe\n\n-- \nPengutronix e.K. | Uwe Kleine-König |\nIndustrial Linux Solutions | http://www.pengutronix.de/ |\n"},{"id":"223920","messageId":"87bo5u28l6.fsf@linux-k42r.v.cablecom.net","threadId":"34510","inReplyTo":"20130722090854.GA22222@pengutronix.de","subject":"Re: git log anomalities","fromName":"Thomas Rast","fromEmail":"trast@inf.ethz.ch","sentAt":"2013-07-22T10:40:01Z","receivedAt":"2013-07-22T10:40:01Z","isPatch":false,"sender":{"key":"tr@thomasrast.ch","avatar":"https://avatars.githubusercontent.com/u/153510?v=4"},"body":"Uwe Kleine-König writes:\n\n> Hello,\n>\n> today I looked at the changes to drivers/net/ethernet/freescale/fec.c in\n> the kernel since v3.8 using\n>\n> \tgit log --stat v3.8.. --full-diff -- drivers/net/ethernet/freescale/fec.c\n>\n> which looks as expected. But when I added --graph the diffstats change.\n> E.g. for 793fc0964be1921f15a44be58b066f22b925d90b it changes from:\n>\n> drivers/net/ethernet/freescale/Makefile | 3 +-\n> drivers/net/ethernet/freescale/fec.c | 1966 -----------------------------\n> drivers/net/ethernet/freescale/fec_main.c | 1966 +++++++++++++++++++++++++++++\n> drivers/net/ethernet/freescale/fec_ptp.c | 3 -\n> 4 files changed, 1968 insertions(+), 1970 deletions(-)\n>\n> to\n>\n> | Documentation/devicetree/bindings/net/dsa/dsa.txt | 91 +\n> | .../bindings/net/marvell-orion-mdio.txt | 3 +\n> | Documentation/networking/ip-sysctl.txt | 35 +-\n[...]\n> | 404 files changed, 15373 insertions(+), 8563 deletions(-)\n>\n> Is that a bug, or a feature I don't understand?\n\nNice catch. It's a bad interaction between --full-diff, --stat and\n--parents (which --graph implies in an internal-workings kind of way).\n\nThe parent rewriting gets to mess with the history *before* we generate\nthe diffs. Normally this wouldn't matter, because the pathspec filter\nwould then again exclude all but the \"real\" change from the diff. But\nwith --full-diff, you see all the changes between 793fc096 and the next\ncommit that touches the given pathspec (47a5247f), which is -- at this\nstage, after rewriting -- the \"parent\".\n\nI suspect to fix this we'll need to separate the \"real\" from the\n\"rewritten\" parents, which might take a bit of work.\n\n-- \nThomas Rast\ntrast@{inf,student}.ethz.ch\n"},{"id":"223937","messageId":"a598aec3e3c90de4d2c08e58ee0a4828edc80ac2.1374527806.git.trast@inf.ethz.ch","threadId":"34510","inReplyTo":"20130722090854.GA22222@pengutronix.de","subject":"[PATCH] log: use true parents for diff even when rewriting","fromName":"Thomas Rast","fromEmail":"trast@inf.ethz.ch","sentAt":"2013-07-22T21:22:01Z","receivedAt":"2013-07-22T21:22:01Z","isPatch":true,"sender":{"key":"tr@thomasrast.ch","avatar":"https://avatars.githubusercontent.com/u/153510?v=4"},"body":"When using pathspec filtering in combination with diff-based log\noutput, parent simplification happens before the diff is computed.\nThe diff is therefore against the *simplified* parents.\n\nThis works okay, arguably by accident, in the normal case: the pruned\ncommits did not affect the paths being filtered, so the diff against\nthe prune-result is the same as against the diff against the true\nparents.\n\nHowever, --full-diff breaks this guarantee, and indeed gives pretty\nspectacular results when comparing the output of\n\n git log --graph --stat ...\n git log --graph --full-diff --stat ...\n\n(--graph internally kicks in parent simplification, much like\n--parents).\n\nReported-by: Uwe Kleine-König \nSigned-off-by: Thomas Rast \n---\n\nPerhaps like this. It's getting a bit late, so I'm not sure if I'm\nmissing another user of the \"true\" parent list, but it does fix the\nissue you reported.\n\n combine-diff.c | 3 ++-\n commit.c | 16 ++++++++++++++++\n commit.h | 3 +++\n log-tree.c | 2 +-\n revision.c | 30 +++++++++++++++++++++++++++++-\n revision.h | 15 +++++++++++++++\n t/t6012-rev-list-simplify.sh | 6 ++++++\n 7 files changed, 72 insertions(+), 3 deletions(-)\n\ndiff --git a/combine-diff.c b/combine-diff.c\nindex 6dc0609..abd2397 100644\n--- a/combine-diff.c\n+++ b/combine-diff.c\n@@ -10,6 +10,7 @@\n #include \"refs.h\"\n #include \"userdiff.h\"\n #include \"sha1-array.h\"\n+#include \"revision.h\"\n \n static struct combine_diff_path *intersect_paths(struct combine_diff_path *curr, int n, int num_parent)\n {\n@@ -1383,7 +1384,7 @@ void diff_tree_combined(const unsigned char *sha1,\n void diff_tree_combined_merge(const struct commit *commit, int dense,\n \t\t\t struct rev_info *rev)\n {\n-\tstruct commit_list *parent = commit->parents;\n+\tstruct commit_list *parent = get_saved_parents(commit);\n \tstruct sha1_array parents = SHA1_ARRAY_INIT;\n \n \twhile (parent) {\ndiff --git a/commit.c b/commit.c\nindex e5862f6..5ecdb38 100644\n--- a/commit.c\n+++ b/commit.c\n@@ -377,6 +377,22 @@ unsigned commit_list_count(const struct commit_list *l)\n \treturn c;\n }\n \n+struct commit_list *copy_commit_list(struct commit_list *list)\n+{\n+\tstruct commit_list *head = NULL;\n+\tstruct commit_list **pp = &head;\n+\twhile (list) {\n+\t\tstruct commit_list *new;\n+\t\tnew = xmalloc(sizeof(struct commit_list));\n+\t\tnew->item = list->item;\n+\t\tnew->next = NULL;\n+\t\t*pp = new;\n+\t\tpp = &new->next;\n+\t\tlist = list->next;\n+\t}\n+\treturn head;\n+}\n+\n void free_commit_list(struct commit_list *list)\n {\n \twhile (list) {\ndiff --git a/commit.h b/commit.h\nindex 35cc4e2..ca766d2 100644\n--- a/commit.h\n+++ b/commit.h\n@@ -62,6 +62,9 @@ struct commit_list *commit_list_insert_by_date(struct commit *item,\n \t\t\t\t struct commit_list **list);\n void commit_list_sort_by_date(struct commit_list **list);\n \n+/* Shallow copy of the input list */\n+struct commit_list *copy_commit_list(struct commit_list *list);\n+\n void free_commit_list(struct commit_list *list);\n \n /* Commit formats */\ndiff --git a/log-tree.c b/log-tree.c\nindex a49d8e8..b1d5747 100644\n--- a/log-tree.c\n+++ b/log-tree.c\n@@ -738,7 +738,7 @@ static int log_tree_diff(struct rev_info *opt, struct commit *commit, struct log\n \tsha1 = commit->tree->object.sha1;\n \n \t/* Root commit? */\n-\tparents = commit->parents;\n+\tparents = get_saved_parents(commit);\n \tif (!parents) {\n \t\tif (opt->show_root_diff) {\n \t\t\tdiff_root_tree_sha1(sha1, \"\", &opt->diffopt);\ndiff --git a/revision.c b/revision.c\nindex 84ccc05..3fdd0ae 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -15,6 +15,7 @@\n #include \"string-list.h\"\n #include \"line-log.h\"\n #include \"mailmap.h\"\n+#include \"commit-slab.h\"\n \n volatile show_early_output_fn_t show_early_output;\n \n@@ -2763,7 +2764,7 @@ static int commit_match(struct commit *commit, struct rev_info *opt)\n \treturn retval;\n }\n \n-static inline int want_ancestry(struct rev_info *revs)\n+static inline int want_ancestry(const struct rev_info *revs)\n {\n \treturn (revs->rewrite_parents || revs->children.name);\n }\n@@ -2820,6 +2821,7 @@ enum commit_action simplify_commit(struct rev_info *revs, struct commit *commit)\n \tif (action == commit_show &&\n \t !revs->show_all &&\n \t revs->prune && revs->dense && want_ancestry(revs)) {\n+\t\tsave_parents(commit);\n \t\tif (rewrite_parents(revs, commit, rewrite_one) < 0)\n \t\t\treturn commit_error;\n \t}\n@@ -3069,3 +3071,29 @@ void put_revision_mark(const struct rev_info *revs, const struct commit *commit)\n \tfputs(mark, stdout);\n \tputchar(' ');\n }\n+\n+define_commit_slab(saved_parents, struct commit_list *);\n+struct saved_parents saved_parents_slab;\n+static int saved_parents_initialized;\n+\n+void save_parents(struct commit *commit)\n+{\n+\tstruct commit_list **pp;\n+\n+\tif (!saved_parents_initialized) {\n+\t\tinit_saved_parents(&saved_parents_slab);\n+\t\tsaved_parents_initialized = 1;\n+\t}\n+\n+\tpp = saved_parents_at(&saved_parents_slab, commit);\n+\tassert(*pp == NULL);\n+\t*pp = copy_commit_list(commit->parents);\n+}\n+\n+struct commit_list *get_saved_parents(struct commit *commit)\n+{\n+\tif (!saved_parents_initialized)\n+\t\treturn commit->parents;\n+\n+\treturn *saved_parents_at(&saved_parents_slab, commit);\n+}\ndiff --git a/revision.h b/revision.h\nindex 95859ba..0717518 100644\n--- a/revision.h\n+++ b/revision.h\n@@ -273,4 +273,19 @@ enum rewrite_result {\n \n extern int rewrite_parents(struct rev_info *revs, struct commit *commit,\n \trewrite_parent_fn_t rewrite_parent);\n+\n+/*\n+ * Save a copy of the parent list, and return the saved copy. This is\n+ * used by the log machinery to retrieve the original parents when\n+ * commit->parents has been modified by history simpification.\n+ *\n+ * You may only call save_parents() once per commit (this is checked\n+ * for non-root commits).\n+ *\n+ * get_original_parents() will transparently return commit->parents if\n+ * history simplification is off.\n+ */\n+extern void save_parents(struct commit *commit);\n+extern struct commit_list *get_original_parents(struct commit *commit);\n+\n #endif\ndiff --git a/t/t6012-rev-list-simplify.sh b/t/t6012-rev-list-simplify.sh\nindex 57ce239..fde5e71 100755\n--- a/t/t6012-rev-list-simplify.sh\n+++ b/t/t6012-rev-list-simplify.sh\n@@ -127,4 +127,10 @@ test_expect_success 'full history simplification without parent' '\n \t}\n '\n \n+test_expect_success '--full-diff is not affected by --parents' '\n+\tgit log -p --pretty=\"%H\" --full-diff -- file >expected &&\n+\tgit log -p --pretty=\"%H\" --full-diff --parents -- file >actual &&\n+\ttest_cmp expected actual\n+'\n+\n test_done\n-- \n1.8.3.3.1180.gbf33142\n"},{"id":"223940","messageId":"7v61w2clli.fsf@alter.siamese.dyndns.org","threadId":"34510","inReplyTo":"a598aec3e3c90de4d2c08e58ee0a4828edc80ac2.1374527806.git.trast@inf.ethz.ch","subject":"Re: [PATCH] log: use true parents for diff even when rewriting","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-07-22T21:48:25Z","receivedAt":"2013-07-22T21:48:25Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Thomas Rast writes:\n\n> When using pathspec filtering in combination with diff-based log\n> output, parent simplification happens before the diff is computed.\n> The diff is therefore against the *simplified* parents.\n>\n> This works okay, arguably by accident, in the normal case: the pruned\n> commits did not affect the paths being filtered, so the diff against\n> the prune-result is the same as against the diff against the true\n> parents.\n>\n> However, --full-diff breaks this guarantee, and indeed gives pretty\n> spectacular results when comparing the output of\n>\n> git log --graph --stat ...\n> git log --graph --full-diff --stat ...\n>\n> (--graph internally kicks in parent simplification, much like\n> --parents).\n>\n> Reported-by: Uwe Kleine-König \n> Signed-off-by: Thomas Rast \n> ---\n>\n> Perhaps like this. It's getting a bit late, so I'm not sure if I'm\n> missing another user of the \"true\" parent list, but it does fix the\n> issue you reported.\n\nConceptually I can see how this will change the history\nsimplification in the vertical direction (skipping the ancestry\nchain and jumping directly to the closest grandparent that touched\nthe specified path), but I am not sure how well this interacts with\nhistory simplification in the horizontal direciton (culling\nirrelevant side branches from the merge).\n\n> @@ -2820,6 +2821,7 @@ enum commit_action simplify_commit(struct rev_info *revs, struct commit *commit)\n> \tif (action == commit_show &&\n> \t !revs->show_all &&\n> \t revs->prune && revs->dense && want_ancestry(revs)) {\n> +\t\tsave_parents(commit);\n> \t\tif (rewrite_parents(revs, commit, rewrite_one) < 0)\n> \t\t\treturn commit_error;\n> \t}\n\nAfter this, rewritten parent list may have shrunk by dropping\nirrelevant side branches, but saved parents would have the full\nparents list. When we decide how to show diff depending on the\nnumber of remaining parents, we would still use the rewritten\nparents (which may have been reduced to a single strand of pearls)\nand your change will use the original parents (which may be\nmultiple).\n\nI also have to wonder if we always want to incur this save-parents\noverhead, or we are better off limiting it to only when --full-diff\nis in effect.\n\nThanks for getting the ball rolling, anyway.\n"},{"id":"223973","messageId":"87k3khpwhh.fsf@linux-k42r.v.cablecom.net","threadId":"34510","inReplyTo":"7v61w2clli.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] log: use true parents for diff even when rewriting","fromName":"Thomas Rast","fromEmail":"trast@inf.ethz.ch","sentAt":"2013-07-23T07:27:06Z","receivedAt":"2013-07-23T07:27:06Z","isPatch":true,"sender":{"key":"tr@thomasrast.ch","avatar":"https://avatars.githubusercontent.com/u/153510?v=4"},"body":"Junio C Hamano writes:\n\n> Thomas Rast writes:\n>\n>> When using pathspec filtering in combination with diff-based log\n>> output, parent simplification happens before the diff is computed.\n>> The diff is therefore against the *simplified* parents.\n>>\n>> This works okay, arguably by accident, in the normal case: the pruned\n>> commits did not affect the paths being filtered, so the diff against\n>> the prune-result is the same as against the diff against the true\n>> parents.\n>>\n>> However, --full-diff breaks this guarantee, and indeed gives pretty\n>> spectacular results when comparing the output of\n>>\n>> git log --graph --stat ...\n>> git log --graph --full-diff --stat ...\n>>\n>> (--graph internally kicks in parent simplification, much like\n>> --parents).\n\nHmm, I stopped writing the message midway through. There should be\nanother two paragraphs here about storing the original parent list on\nthe side for later use when showing the diff.\n\n>> Perhaps like this. It's getting a bit late, so I'm not sure if I'm\n>> missing another user of the \"true\" parent list, but it does fix the\n>> issue you reported.\n>\n> Conceptually I can see how this will change the history\n> simplification in the vertical direction (skipping the ancestry\n> chain and jumping directly to the closest grandparent that touched\n> the specified path), but I am not sure how well this interacts with\n> history simplification in the horizontal direciton (culling\n> irrelevant side branches from the merge).\n\nBut isn't that similarly confusing for the user as Uwe's original\nproblem? Suddenly we'd be showing a merge commit as an ordinary one,\nsimply because the merged history did not affect the filtered\npathspecs. Thus we would show everything that has been merged on the\n*other* files as a big diff. Would that be useful? It would certainly\nbe a big difference in how the commit is shown.\n\n> I also have to wonder if we always want to incur this save-parents\n> overhead, or we are better off limiting it to only when --full-diff\n> is in effect.\n\nI haven't quite convinced myself that it is 100% safe to use the\nrewritten parents when --full-diff is not in effect...\n\n-- \nThomas Rast\ntrast@{inf,student}.ethz.ch\n"},{"id":"223974","messageId":"20130723074952.GB1754@pengutronix.de","threadId":"34510","inReplyTo":"87k3khpwhh.fsf@linux-k42r.v.cablecom.net","subject":"Re: [PATCH] log: use true parents for diff even when rewriting","fromName":"Uwe Kleine-König","fromEmail":"u.kleine-koenig@pengutronix.de","sentAt":"2013-07-23T07:49:52Z","receivedAt":"2013-07-23T07:49:52Z","isPatch":true,"sender":{"key":"u.kleine-koenig@pengutronix.de","avatar":"https://gravatar.com/avatar/354b5e3ceb2806a2f1e1e382ac29ddbdad18288654da62b61eb13583a857eee7?d=mp&s=160"},"body":"Hello Thomas,\n\nOn Tue, Jul 23, 2013 at 09:27:06AM +0200, Thomas Rast wrote:\n> Junio C Hamano writes:\n> > Conceptually I can see how this will change the history\n> > simplification in the vertical direction (skipping the ancestry\n> > chain and jumping directly to the closest grandparent that touched\n> > the specified path), but I am not sure how well this interacts with\n> > history simplification in the horizontal direciton (culling\n> > irrelevant side branches from the merge).\n> \n> But isn't that similarly confusing for the user as Uwe's original\n> problem? Suddenly we'd be showing a merge commit as an ordinary one,\n> simply because the merged history did not affect the filtered\n> pathspecs. Thus we would show everything that has been merged on the\n> *other* files as a big diff. Would that be useful? It would certainly\n> be a big difference in how the commit is shown.\nthe merge is only included in the output if on both parent paths the\nfile is touched. So this is a non-issue, isn't it? (Well, only if it has\nmore than 2 parents and not all ancestor paths touch the file, the\nnumber of parents shown is changed.)\n\nBest regards\nUwe\n\n-- \nPengutronix e.K. | Uwe Kleine-König |\nIndustrial Linux Solutions | http://www.pengutronix.de/ |\n"},{"id":"223999","messageId":"7vob9t6og4.fsf@alter.siamese.dyndns.org","threadId":"34510","inReplyTo":"a598aec3e3c90de4d2c08e58ee0a4828edc80ac2.1374527806.git.trast@inf.ethz.ch","subject":"Re: [PATCH] log: use true parents for diff even when rewriting","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-07-23T19:55:39Z","receivedAt":"2013-07-23T19:55:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Thomas Rast writes:\n\n> +define_commit_slab(saved_parents, struct commit_list *);\n> +struct saved_parents saved_parents_slab;\n> +static int saved_parents_initialized;\n> +\n> +void save_parents(struct commit *commit)\n> +{\n> +\tstruct commit_list **pp;\n> +\n> +\tif (!saved_parents_initialized) {\n> +\t\tinit_saved_parents(&saved_parents_slab);\n> +\t\tsaved_parents_initialized = 1;\n> +\t}\n> +\n> +\tpp = saved_parents_at(&saved_parents_slab, commit);\n> +\tassert(*pp == NULL);\n> +\t*pp = copy_commit_list(commit->parents);\n> +}\n> +\n> +struct commit_list *get_saved_parents(struct commit *commit)\n\nUse \"const struct commit *\" here, as combine-diff.c has a const pointer.\n\n> +{\n> +\tif (!saved_parents_initialized)\n> +\t\treturn commit->parents;\n> +\n> +\treturn *saved_parents_at(&saved_parents_slab, commit);\n> +}\n\nclear_commit_slab() is not used, failing -Wunused -Werror compilation.\n\n> diff --git a/revision.h b/revision.h\n> index 95859ba..0717518 100644\n> --- a/revision.h\n> +++ b/revision.h\n> @@ -273,4 +273,19 @@ enum rewrite_result {\n> \n> extern int rewrite_parents(struct rev_info *revs, struct commit *commit,\n> \trewrite_parent_fn_t rewrite_parent);\n> +\n> +/*\n> + * Save a copy of the parent list, and return the saved copy. This is\n> + * used by the log machinery to retrieve the original parents when\n> + * commit->parents has been modified by history simpification.\n> + *\n> + * You may only call save_parents() once per commit (this is checked\n> + * for non-root commits).\n> + *\n> + * get_original_parents() will transparently return commit->parents if\n> + * history simplification is off.\n> + */\n> +extern void save_parents(struct commit *commit);\n> +extern struct commit_list *get_original_parents(struct commit *commit);\n> +\n\ns/_original/_saved/ here, and \"const struct commit *\".\n\nBy the way, when the only single parameter is a named type, you can\nsafely omit the name of the parameter from the declaration without\nlosing clarity.\n\n> #endif\n> diff --git a/t/t6012-rev-list-simplify.sh b/t/t6012-rev-list-simplify.sh\n> index 57ce239..fde5e71 100755\n> --- a/t/t6012-rev-list-simplify.sh\n> +++ b/t/t6012-rev-list-simplify.sh\n> @@ -127,4 +127,10 @@ test_expect_success 'full history simplification without parent' '\n> \t}\n> '\n> \n> +test_expect_success '--full-diff is not affected by --parents' '\n> +\tgit log -p --pretty=\"%H\" --full-diff -- file >expected &&\n> +\tgit log -p --pretty=\"%H\" --full-diff --parents -- file >actual &&\n> +\ttest_cmp expected actual\n> +'\n> +\n> test_done\n"},{"id":"224384","messageId":"e7f2ead2267ff78940aab00fe36c378a2ce5d85e.1375301293.git.trast@inf.ethz.ch","threadId":"34510","inReplyTo":"a598aec3e3c90de4d2c08e58ee0a4828edc80ac2.1374527806.git.trast@inf.ethz.ch","subject":"[PATCH v2] log: use true parents for diff even when rewriting","fromName":"Thomas Rast","fromEmail":"trast@inf.ethz.ch","sentAt":"2013-07-31T20:13:20Z","receivedAt":"2013-07-31T20:13:20Z","isPatch":true,"sender":{"key":"tr@thomasrast.ch","avatar":"https://avatars.githubusercontent.com/u/153510?v=4"},"body":"When using pathspec filtering in combination with diff-based log\noutput, parent simplification happens before the diff is computed.\nThe diff is therefore against the *simplified* parents.\n\nThis works okay, arguably by accident, in the normal case:\nsimplification reduces to one parent as long as the commit is TREESAME\nto it. So the simplified parent of any given commit must have the\nsame tree contents on the filtered paths as its true (unfiltered)\nparent.\n\nHowever, --full-diff breaks this guarantee, and indeed gives pretty\nspectacular results when comparing the output of\n\n git log --graph --stat ...\n git log --graph --full-diff --stat ...\n\n(--graph internally kicks in parent simplification, much like\n--parents).\n\nTo fix it, store a copy of the parent list before simplification (in a\nslab) whenever --full-diff is in effect. Then use the stored parents\ninstead of the simplified ones in the commit display code paths. The\nlatter do not actually check for --full-diff to avoid duplicated code;\nthey just grab the original parents if save_parents() has not been\ncalled for this revision walk.\n\nFor ordinary commits it should be obvious that this is the right thing\nto do.\n\nMerge commits are a bit subtle. Observe that with default\nsimplification, merge simplification is an all-or-nothing decision:\neither the merge is TREESAME to one parent and disappears, or it is\ndifferent from all parents and the parent list remains intact.\nRedundant parents are not pruned, so the existing code also shows them\nas a merge.\n\nSo if we do show a merge commit, the parent list just consists of the\nrewrite result on each parent. Running, e.g., --cc on this in\n--full-diff mode is not very useful: if any commits were skipped, some\nhunks will disagree with all sides of the merge (with one side,\nbecause commits were skipped; with the others, because they didn't\nhave those changes in the first place). This triggers --cc showing\nthese hunks spuriously.\n\nTherefore I believe that even for merge commits it is better to show\nthe diffs wrt. the original parents.\n\nReported-by: Uwe Kleine-König \nHelped-by: Junio C Hamano \nHelped-by: Ramsay Jones \nSigned-off-by: Thomas Rast \n---\n\nNew in this version:\n\n* Junio's and Ramsay's suggestions squashed\n\n* Moved the slab variable from file-static to within the struct\n rev_info. In practice there is no difference, because you cannot\n run two walks in parallel anyway (they would stomp over each others'\n parent lists!). But it was easy, feels slightly cleaner, and avoids\n having yet another global variable.\n\n* Wrote an actual commit message. I hope the logic wrt. merge commits\n is correct.\n\n\n combine-diff.c | 3 ++-\n commit.c | 16 ++++++++++++++++\n commit.h | 3 +++\n log-tree.c | 2 +-\n revision.c | 43 ++++++++++++++++++++++++++++++++++++++++++-\n revision.h | 20 ++++++++++++++++++++\n t/t6012-rev-list-simplify.sh | 6 ++++++\n 7 files changed, 90 insertions(+), 3 deletions(-)\n\ndiff --git a/combine-diff.c b/combine-diff.c\nindex 6dc0609..3d2aaf3 100644\n--- a/combine-diff.c\n+++ b/combine-diff.c\n@@ -10,6 +10,7 @@\n #include \"refs.h\"\n #include \"userdiff.h\"\n #include \"sha1-array.h\"\n+#include \"revision.h\"\n \n static struct combine_diff_path *intersect_paths(struct combine_diff_path *curr, int n, int num_parent)\n {\n@@ -1383,7 +1384,7 @@ void diff_tree_combined(const unsigned char *sha1,\n void diff_tree_combined_merge(const struct commit *commit, int dense,\n \t\t\t struct rev_info *rev)\n {\n-\tstruct commit_list *parent = commit->parents;\n+\tstruct commit_list *parent = get_saved_parents(rev, commit);\n \tstruct sha1_array parents = SHA1_ARRAY_INIT;\n \n \twhile (parent) {\ndiff --git a/commit.c b/commit.c\nindex e5862f6..5ecdb38 100644\n--- a/commit.c\n+++ b/commit.c\n@@ -377,6 +377,22 @@ unsigned commit_list_count(const struct commit_list *l)\n \treturn c;\n }\n \n+struct commit_list *copy_commit_list(struct commit_list *list)\n+{\n+\tstruct commit_list *head = NULL;\n+\tstruct commit_list **pp = &head;\n+\twhile (list) {\n+\t\tstruct commit_list *new;\n+\t\tnew = xmalloc(sizeof(struct commit_list));\n+\t\tnew->item = list->item;\n+\t\tnew->next = NULL;\n+\t\t*pp = new;\n+\t\tpp = &new->next;\n+\t\tlist = list->next;\n+\t}\n+\treturn head;\n+}\n+\n void free_commit_list(struct commit_list *list)\n {\n \twhile (list) {\ndiff --git a/commit.h b/commit.h\nindex d912a9d..f9504f7 100644\n--- a/commit.h\n+++ b/commit.h\n@@ -62,6 +62,9 @@ struct commit_list *commit_list_insert_by_date(struct commit *item,\n \t\t\t\t struct commit_list **list);\n void commit_list_sort_by_date(struct commit_list **list);\n \n+/* Shallow copy of the input list */\n+struct commit_list *copy_commit_list(struct commit_list *list);\n+\n void free_commit_list(struct commit_list *list);\n \n /* Commit formats */\ndiff --git a/log-tree.c b/log-tree.c\nindex a49d8e8..8534d91 100644\n--- a/log-tree.c\n+++ b/log-tree.c\n@@ -738,7 +738,7 @@ static int log_tree_diff(struct rev_info *opt, struct commit *commit, struct log\n \tsha1 = commit->tree->object.sha1;\n \n \t/* Root commit? */\n-\tparents = commit->parents;\n+\tparents = get_saved_parents(opt, commit);\n \tif (!parents) {\n \t\tif (opt->show_root_diff) {\n \t\t\tdiff_root_tree_sha1(sha1, \"\", &opt->diffopt);\ndiff --git a/revision.c b/revision.c\nindex 84ccc05..e3ca936 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -15,6 +15,7 @@\n #include \"string-list.h\"\n #include \"line-log.h\"\n #include \"mailmap.h\"\n+#include \"commit-slab.h\"\n \n volatile show_early_output_fn_t show_early_output;\n \n@@ -2763,7 +2764,7 @@ static int commit_match(struct commit *commit, struct rev_info *opt)\n \treturn retval;\n }\n \n-static inline int want_ancestry(struct rev_info *revs)\n+static inline int want_ancestry(const struct rev_info *revs)\n {\n \treturn (revs->rewrite_parents || revs->children.name);\n }\n@@ -2820,6 +2821,14 @@ enum commit_action simplify_commit(struct rev_info *revs, struct commit *commit)\n \tif (action == commit_show &&\n \t !revs->show_all &&\n \t revs->prune && revs->dense && want_ancestry(revs)) {\n+\t\t/*\n+\t\t * --full-diff on simplified parents is no good: it\n+\t\t * will show spurious changes from the commits that\n+\t\t * were elided. So we save the parents on the side\n+\t\t * when --full-diff is in effect.\n+\t\t */\n+\t\tif (revs->full_diff)\n+\t\t\tsave_parents(revs, commit);\n \t\tif (rewrite_parents(revs, commit, rewrite_one) < 0)\n \t\t\treturn commit_error;\n \t}\n@@ -3038,6 +3047,8 @@ struct commit *get_revision(struct rev_info *revs)\n \tc = get_revision_internal(revs);\n \tif (c && revs->graph)\n \t\tgraph_update(revs->graph, c);\n+\tif (!c)\n+\t\tfree_saved_parents(revs);\n \treturn c;\n }\n \n@@ -3069,3 +3080,33 @@ void put_revision_mark(const struct rev_info *revs, const struct commit *commit)\n \tfputs(mark, stdout);\n \tputchar(' ');\n }\n+\n+define_commit_slab(saved_parents, struct commit_list *);\n+\n+void save_parents(struct rev_info *revs, struct commit *commit)\n+{\n+\tstruct commit_list **pp;\n+\n+\tif (!revs->saved_parents_slab) {\n+\t\trevs->saved_parents_slab = xmalloc(sizeof(struct saved_parents));\n+\t\tinit_saved_parents(revs->saved_parents_slab);\n+\t}\n+\n+\tpp = saved_parents_at(revs->saved_parents_slab, commit);\n+\tassert(*pp == NULL);\n+\t*pp = copy_commit_list(commit->parents);\n+}\n+\n+struct commit_list *get_saved_parents(struct rev_info *revs, const struct commit *commit)\n+{\n+\tif (!revs->saved_parents_slab)\n+\t\treturn commit->parents;\n+\n+\treturn *saved_parents_at(revs->saved_parents_slab, commit);\n+}\n+\n+void free_saved_parents(struct rev_info *revs)\n+{\n+\tif (revs->saved_parents_slab)\n+\t\tclear_saved_parents(revs->saved_parents_slab);\n+}\ndiff --git a/revision.h b/revision.h\nindex 95859ba..e7f1d21 100644\n--- a/revision.h\n+++ b/revision.h\n@@ -25,6 +25,7 @@\n struct rev_info;\n struct log_info;\n struct string_list;\n+struct saved_parents;\n \n struct rev_cmdline_info {\n \tunsigned int nr;\n@@ -187,6 +188,9 @@ struct rev_info {\n \n \t/* line level range that we are chasing */\n \tstruct decoration line_log_data;\n+\n+\t/* copies of the parent lists, for --full-diff display */\n+\tstruct saved_parents *saved_parents_slab;\n };\n \n #define REV_TREE_SAME\t\t0\n@@ -273,4 +277,20 @@ enum rewrite_result {\n \n extern int rewrite_parents(struct rev_info *revs, struct commit *commit,\n \trewrite_parent_fn_t rewrite_parent);\n+\n+/*\n+ * Save a copy of the parent list, and return the saved copy. This is\n+ * used by the log machinery to retrieve the original parents when\n+ * commit->parents has been modified by history simpification.\n+ *\n+ * You may only call save_parents() once per commit (this is checked\n+ * for non-root commits).\n+ *\n+ * get_saved_parents() will transparently return commit->parents if\n+ * history simplification is off.\n+ */\n+extern void save_parents(struct rev_info *revs, struct commit *commit);\n+extern struct commit_list *get_saved_parents(struct rev_info *revs, const struct commit *commit);\n+extern void free_saved_parents(struct rev_info *revs);\n+\n #endif\ndiff --git a/t/t6012-rev-list-simplify.sh b/t/t6012-rev-list-simplify.sh\nindex 57ce239..fde5e71 100755\n--- a/t/t6012-rev-list-simplify.sh\n+++ b/t/t6012-rev-list-simplify.sh\n@@ -127,4 +127,10 @@ test_expect_success 'full history simplification without parent' '\n \t}\n '\n \n+test_expect_success '--full-diff is not affected by --parents' '\n+\tgit log -p --pretty=\"%H\" --full-diff -- file >expected &&\n+\tgit log -p --pretty=\"%H\" --full-diff --parents -- file >actual &&\n+\ttest_cmp expected actual\n+'\n+\n test_done\n-- \n1.8.4.rc0.408.gad6868d\n"},{"id":"224407","messageId":"20130731225520.GB25882@sigill.intra.peff.net","threadId":"34510","inReplyTo":"e7f2ead2267ff78940aab00fe36c378a2ce5d85e.1375301293.git.trast@inf.ethz.ch","subject":"Re: [PATCH v2] log: use true parents for diff even when rewriting","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-07-31T22:55:21Z","receivedAt":"2013-07-31T22:55:21Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jul 31, 2013 at 10:13:20PM +0200, Thomas Rast wrote:\n\n> When using pathspec filtering in combination with diff-based log\n> output, parent simplification happens before the diff is computed.\n> The diff is therefore against the *simplified* parents.\n> \n> This works okay, arguably by accident, in the normal case:\n> simplification reduces to one parent as long as the commit is TREESAME\n> to it. So the simplified parent of any given commit must have the\n> same tree contents on the filtered paths as its true (unfiltered)\n> parent.\n> \n> However, --full-diff breaks this guarantee, and indeed gives pretty\n> spectacular results when comparing the output of\n> \n> git log --graph --stat ...\n> git log --graph --full-diff --stat ...\n> \n> (--graph internally kicks in parent simplification, much like\n> --parents).\n\nYour description (and solution) make a lot of sense to me. Another code\npath that has a similar problem is the \"-g\" reflog walker. It rewrites\nthe parents based on the reflog, and the diffs it produces are mostly\nuseless (e.g., try \"git stash list -p\").\n\nShould we be applying the same technique there?\n\nI guess it might bother people who really want to see the diff between\ntwo lines in the reflog (e.g., comparing the results stored by \"rebase\"\nwith the previous version). I think that is rare enough that \"git diff\"\nwould be a better tool in that case, though.\n\nAnd arguably, \"log -g\" should not be rewriting the parents at all,\nbecause it makes \"--graph\" pointless (and indeed, we disallow the\ncombination). So potentially the solution is to stop the rewriting\nentirely, not mask it for the diffs. But doing so might be a good\ninterim solution until somebody feels like rewriting the reflog walker.\n\n-Peff\n"}]}