{"thread":{"id":"31115","subject":"[PATCH 0/2] Small log simplifications","startedAt":"2012-07-27T17:21:37Z","lastAt":"2012-07-29T06:24:52Z","messageCount":10,"participants":["Martin von Zweigbergk","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"195950","messageId":"1343409699-27199-1-git-send-email-martin.von.zweigbergk@gmail.com","threadId":"31115","inReplyTo":null,"subject":"[PATCH 0/2] Small log simplifications","fromName":"Martin von Zweigbergk","fromEmail":"martin.von.zweigbergk@gmail.com","sentAt":"2012-07-27T17:21:37Z","receivedAt":"2012-07-27T17:21:37Z","isPatch":true,"sender":{"key":"martinvonz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/891642?v=4"},"body":"These are just some things I noticed while looking at the revision\nwalking code. I'm not very familiar with the code at all, so the\npatches may very well be completely wrong; view them as newbie\nquestions in patch-form if you like.\n\n\nMartin von Zweigbergk (2):\n remove unnecessary parameter from get_patch_ids()\n log: remove redundant check for merge commit\n\n builtin/log.c | 18 ++++--------------\n 1 file changed, 4 insertions(+), 14 deletions(-)\n\n-- \n1.7.11.1.104.ge7b44f1\n"},{"id":"195952","messageId":"1343409699-27199-2-git-send-email-martin.von.zweigbergk@gmail.com","threadId":"31115","inReplyTo":"1343409699-27199-1-git-send-email-martin.von.zweigbergk@gmail.com","subject":"[PATCH 1/2] remove unnecessary parameter from get_patch_ids()","fromName":"Martin von Zweigbergk","fromEmail":"martin.von.zweigbergk@gmail.com","sentAt":"2012-07-27T17:21:38Z","receivedAt":"2012-07-27T17:21:38Z","isPatch":true,"sender":{"key":"martinvonz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/891642?v=4"},"body":"get_patch_ids() takes an already initialized rev_info and a\nprefix. The prefix is used when initalizing a second rev_info. Since\nthe initialized rev_info already has a prefix and it seems the prefix\ndoesn't change, we can used the prefix from the initialized rev_info\nto initialize the second rev_info.\n\nSigned-off-by: Martin von Zweigbergk \n---\n builtin/log.c | 8 ++++----\n 1 file changed, 4 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/log.c b/builtin/log.c\nindex ecc2793..7a92e3f 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -696,7 +696,7 @@ static int reopen_stdout(struct commit *commit, const char *subject,\n \treturn 0;\n }\n \n-static void get_patch_ids(struct rev_info *rev, struct patch_ids *ids, const char *prefix)\n+static void get_patch_ids(struct rev_info *rev, struct patch_ids *ids)\n {\n \tstruct rev_info check_rev;\n \tstruct commit *commit;\n@@ -717,7 +717,7 @@ static void get_patch_ids(struct rev_info *rev, struct patch_ids *ids, const cha\n \tinit_patch_ids(ids);\n \n \t/* given a range a..b get all patch ids for b..a */\n-\tinit_revisions(&check_rev, prefix);\n+\tinit_revisions(&check_rev, rev->prefix);\n \to1->flags ^= UNINTERESTING;\n \to2->flags ^= UNINTERESTING;\n \tadd_pending_object(&check_rev, o1, \"o1\");\n@@ -1306,7 +1306,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)\n \t\t\tif (hashcmp(o[0].item->sha1, o[1].item->sha1) == 0)\n \t\t\t\treturn 0;\n \t\t}\n-\t\tget_patch_ids(&rev, &ids, prefix);\n+\t\tget_patch_ids(&rev, &ids);\n \t}\n \n \tif (!use_stdout)\n@@ -1525,7 +1525,7 @@ int cmd_cherry(int argc, const char **argv, const char *prefix)\n \t\t\treturn 0;\n \t}\n \n-\tget_patch_ids(&revs, &ids, prefix);\n+\tget_patch_ids(&revs, &ids);\n \n \tif (limit && add_pending_commit(limit, &revs, UNINTERESTING))\n \t\tdie(_(\"Unknown commit %s\"), limit);\n-- \n1.7.11.1.104.ge7b44f1\n"},{"id":"195951","messageId":"1343409699-27199-3-git-send-email-martin.von.zweigbergk@gmail.com","threadId":"31115","inReplyTo":"1343409699-27199-1-git-send-email-martin.von.zweigbergk@gmail.com","subject":"[PATCH 2/2] log: remove redundant check for merge commit","fromName":"Martin von Zweigbergk","fromEmail":"martin.von.zweigbergk@gmail.com","sentAt":"2012-07-27T17:21:39Z","receivedAt":"2012-07-27T17:21:39Z","isPatch":true,"sender":{"key":"martinvonz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/891642?v=4"},"body":"While walking the revision list in get_patch_ids and cmd_cherry, we\nignore merges by checking if there is more than one parent. However,\nsince the revision walk was initialized with revs.ignore_merges = 1,\nthis would never happen. Remove the unnecessary checks.\n\nAlso re-initializing rev_info fields to the same values already set in\ninit_revisions().\n\nSigned-off-by: Martin von Zweigbergk \n---\n builtin/log.c | 10 ----------\n 1 file changed, 10 deletions(-)\n\ndiff --git a/builtin/log.c b/builtin/log.c\nindex 7a92e3f..8182a18 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -726,10 +726,6 @@ static void get_patch_ids(struct rev_info *rev, struct patch_ids *ids)\n \t\tdie(_(\"revision walk setup failed\"));\n \n \twhile ((commit = get_revision(&check_rev)) != NULL) {\n-\t\t/* ignore merges */\n-\t\tif (commit->parents && commit->parents->next)\n-\t\t\tcontinue;\n-\n \t\tadd_commit_patch_id(commit, ids);\n \t}\n \n@@ -1509,8 +1505,6 @@ int cmd_cherry(int argc, const char **argv, const char *prefix)\n \n \tinit_revisions(&revs, prefix);\n \trevs.diff = 1;\n-\trevs.combine_merges = 0;\n-\trevs.ignore_merges = 1;\n \tDIFF_OPT_SET(&revs.diffopt, RECURSIVE);\n \n \tif (add_pending_commit(head, &revs, 0))\n@@ -1534,10 +1528,6 @@ int cmd_cherry(int argc, const char **argv, const char *prefix)\n \tif (prepare_revision_walk(&revs))\n \t\tdie(_(\"revision walk setup failed\"));\n \twhile ((commit = get_revision(&revs)) != NULL) {\n-\t\t/* ignore merges */\n-\t\tif (commit->parents && commit->parents->next)\n-\t\t\tcontinue;\n-\n \t\tcommit_list_insert(commit, &list);\n \t}\n \n-- \n1.7.11.1.104.ge7b44f1\n"},{"id":"195973","messageId":"7vfw8czvj5.fsf@alter.siamese.dyndns.org","threadId":"31115","inReplyTo":"1343409699-27199-3-git-send-email-martin.von.zweigbergk@gmail.com","subject":"Re: [PATCH 2/2] log: remove redundant check for merge commit","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-07-27T22:30:54Z","receivedAt":"2012-07-27T22:30:54Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Martin von Zweigbergk writes:\n\n> While walking the revision list in get_patch_ids and cmd_cherry, we\n> ignore merges by checking if there is more than one parent. However,\n> since the revision walk was initialized with revs.ignore_merges = 1,\n> this would never happen. Remove the unnecessary checks.\n>\n> Also re-initializing rev_info fields to the same values already set in\n> init_revisions().\n>\n> Signed-off-by: Martin von Zweigbergk \n\nI think the in-loop checks were totally redundant. I suspect that\nexplicit initialization to revs.ignore_merges outself can be called\nbelt-and-braces defensive programming to avoid getting surprised by\nfuture changes to what init_revisions() would do, so I do not have\nstrong preference either way.\n\n> ---\n> builtin/log.c | 10 ----------\n> 1 file changed, 10 deletions(-)\n>\n> diff --git a/builtin/log.c b/builtin/log.c\n> index 7a92e3f..8182a18 100644\n> --- a/builtin/log.c\n> +++ b/builtin/log.c\n> @@ -726,10 +726,6 @@ static void get_patch_ids(struct rev_info *rev, struct patch_ids *ids)\n> \t\tdie(_(\"revision walk setup failed\"));\n> \n> \twhile ((commit = get_revision(&check_rev)) != NULL) {\n> -\t\t/* ignore merges */\n> -\t\tif (commit->parents && commit->parents->next)\n> -\t\t\tcontinue;\n> -\n> \t\tadd_commit_patch_id(commit, ids);\n> \t}\n> \n> @@ -1509,8 +1505,6 @@ int cmd_cherry(int argc, const char **argv, const char *prefix)\n> \n> \tinit_revisions(&revs, prefix);\n> \trevs.diff = 1;\n> -\trevs.combine_merges = 0;\n> -\trevs.ignore_merges = 1;\n> \tDIFF_OPT_SET(&revs.diffopt, RECURSIVE);\n> \n> \tif (add_pending_commit(head, &revs, 0))\n> @@ -1534,10 +1528,6 @@ int cmd_cherry(int argc, const char **argv, const char *prefix)\n> \tif (prepare_revision_walk(&revs))\n> \t\tdie(_(\"revision walk setup failed\"));\n> \twhile ((commit = get_revision(&revs)) != NULL) {\n> -\t\t/* ignore merges */\n> -\t\tif (commit->parents && commit->parents->next)\n> -\t\t\tcontinue;\n> -\n> \t\tcommit_list_insert(commit, &list);\n> \t}\n"},{"id":"195975","messageId":"7v1ujwzuw1.fsf@alter.siamese.dyndns.org","threadId":"31115","inReplyTo":"1343409699-27199-2-git-send-email-martin.von.zweigbergk@gmail.com","subject":"Re: [PATCH 1/2] remove unnecessary parameter from get_patch_ids()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-07-27T22:44:46Z","receivedAt":"2012-07-27T22:44:46Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Martin von Zweigbergk writes:\n\n> get_patch_ids() takes an already initialized rev_info and a\n> prefix. The prefix is used when initalizing a second rev_info. Since\n> the initialized rev_info already has a prefix and it seems the prefix\n> doesn't change, we can used the prefix from the initialized rev_info\n> to initialize the second rev_info.\n\ns/it seems the prefix doesn't change/the prefix never changes/;\n\n> Signed-off-by: Martin von Zweigbergk \n\nI think this change shouldn't change the behaviour and is good.\n\nJust FYI, during the revision traversal, prefix affects two things\nin the current code:\n\n (1) it is used as the basename for prune data that determine which\n parts of the tree structure to pay attention to (e.g. \"cd t &&\n git log .\").\n\n (2) it is used as the basename for diff output when revs->diffopt\n is used (e.g. \"cd t && git log -p\").\n\nAnd check_rev is used with neither. It does not want to prune any\npart of the tree nor it wants to filter with fancier options like\ngrep/pickaxe (i.e. it does not use setup_revisions()).\n\nIn that sense it probably does not even matter if you did not pass\nrev->prefix down to check_rev, and instead gave NULL to it, but that\nonly holds true in the current codebase, so I think what your patch\ndoes is the right thing to do.\n\nThanks.\n\n> builtin/log.c | 8 ++++----\n> 1 file changed, 4 insertions(+), 4 deletions(-)\n>\n> diff --git a/builtin/log.c b/builtin/log.c\n> index ecc2793..7a92e3f 100644\n> --- a/builtin/log.c\n> +++ b/builtin/log.c\n> @@ -696,7 +696,7 @@ static int reopen_stdout(struct commit *commit, const char *subject,\n> \treturn 0;\n> }\n> \n> -static void get_patch_ids(struct rev_info *rev, struct patch_ids *ids, const char *prefix)\n> +static void get_patch_ids(struct rev_info *rev, struct patch_ids *ids)\n> {\n> \tstruct rev_info check_rev;\n> \tstruct commit *commit;\n> @@ -717,7 +717,7 @@ static void get_patch_ids(struct rev_info *rev, struct patch_ids *ids, const cha\n> \tinit_patch_ids(ids);\n> \n> \t/* given a range a..b get all patch ids for b..a */\n> -\tinit_revisions(&check_rev, prefix);\n> +\tinit_revisions(&check_rev, rev->prefix);\n> \to1->flags ^= UNINTERESTING;\n> \to2->flags ^= UNINTERESTING;\n> \tadd_pending_object(&check_rev, o1, \"o1\");\n> @@ -1306,7 +1306,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)\n> \t\t\tif (hashcmp(o[0].item->sha1, o[1].item->sha1) == 0)\n> \t\t\t\treturn 0;\n> \t\t}\n> -\t\tget_patch_ids(&rev, &ids, prefix);\n> +\t\tget_patch_ids(&rev, &ids);\n> \t}\n> \n> \tif (!use_stdout)\n> @@ -1525,7 +1525,7 @@ int cmd_cherry(int argc, const char **argv, const char *prefix)\n> \t\t\treturn 0;\n> \t}\n> \n> -\tget_patch_ids(&revs, &ids, prefix);\n> +\tget_patch_ids(&revs, &ids);\n> \n> \tif (limit && add_pending_commit(limit, &revs, UNINTERESTING))\n> \t\tdie(_(\"Unknown commit %s\"), limit);\n"},{"id":"195985","messageId":"CAOeW2eG_5G3qQTguvWsSDfLU1s5p=B8WT=v8ywe=G1-VJTfGNw@mail.gmail.com","threadId":"31115","inReplyTo":"7v1ujwzuw1.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 1/2] remove unnecessary parameter from get_patch_ids()","fromName":"Martin von Zweigbergk","fromEmail":"martin.von.zweigbergk@gmail.com","sentAt":"2012-07-28T01:54:50Z","receivedAt":"2012-07-28T01:54:50Z","isPatch":true,"sender":{"key":"martinvonz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/891642?v=4"},"body":"On Fri, Jul 27, 2012 at 3:44 PM, Junio C Hamano wrote:\n> Martin von Zweigbergk writes:\n>\n> s/it seems the prefix doesn't change/the prefix never changes/;\n\n:-) Thanks for confirming.\n\n> In that sense it probably does not even matter if you did not pass\n> rev->prefix down to check_rev, and instead gave NULL to it, but that\n> only holds true in the current codebase, so I think what your patch\n> does is the right thing to do.\n\nMakes sense. Thanks for explaining!\n"},{"id":"195991","messageId":"CAOeW2eFROgZ+taqFvCHoSTVrQZnaQZo567e=73BcA+d5uSjKDA@mail.gmail.com","threadId":"31115","inReplyTo":"7vfw8czvj5.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 2/2] log: remove redundant check for merge commit","fromName":"Martin von Zweigbergk","fromEmail":"martin.von.zweigbergk@gmail.com","sentAt":"2012-07-28T04:52:59Z","receivedAt":"2012-07-28T04:52:59Z","isPatch":true,"sender":{"key":"martinvonz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/891642?v=4"},"body":"On Fri, Jul 27, 2012 at 3:30 PM, Junio C Hamano wrote:\n> Martin von Zweigbergk writes:\n>> Also re-initializing rev_info fields to the same values already set in\n>> init_revisions().\n\nOops, that should have been \" _avoid_ re-initializing\".\n\n> I suspect that\n> explicit initialization to revs.ignore_merges outself can be called\n> belt-and-braces defensive programming to avoid getting surprised by\n> future changes to what init_revisions() would do, so I do not have\n> strong preference either way.\n\nI suspected the same, but OTOH, if it was ever changed, one would have\nto go through all call sites anyway. I checked the other calls to\ninit_revisions (48 of them, I think) and I found only 3 other places\nwhere a field was re-initialized to the same value. In this case it\nwas done inconsistently even within the file. Seeing init_revisions()\nfollowed by \"revs.ignore_merges = 1;\" in one place but not the other\ncan easily lead one to believe that the other place does not ignore\nmerges.\n\nDo you want a reroll with updated commit messages (the missing \"avoid\"\nabove, the dropped \"seems like\" about the prefix in 1/2)? Since you\ndon't have a strong preference about the explicit initialization, I\nassume I can leave those hunks in. I would clarify my reasoning,\nthough.\n\nMartin\n"},{"id":"195992","messageId":"7v7gtoxw80.fsf@alter.siamese.dyndns.org","threadId":"31115","inReplyTo":"CAOeW2eFROgZ+taqFvCHoSTVrQZnaQZo567e=73BcA+d5uSjKDA@mail.gmail.com","subject":"Re: [PATCH 2/2] log: remove redundant check for merge commit","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-07-28T05:58:55Z","receivedAt":"2012-07-28T05:58:55Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Martin von Zweigbergk writes:\n\n> Do you want a reroll with updated commit messages (the missing \"avoid\"\n> above, the dropped \"seems like\" about the prefix in 1/2)?\n\nNah, I've already queued them with log message tweaks. Thanks for\nasking, though.\n"},{"id":"196052","messageId":"CAOeW2eELzcqnecZqea2K0vO818PTYf-P=xEoSKb5GcW7W0Nf+A@mail.gmail.com","threadId":"31115","inReplyTo":"CAOeW2eG4mKTWm7PEGF_t0F9c_X1gkQaDw7HSrCiZaDte7PvOdQ@mail.gmail.com","subject":"Re: [PATCH 2/2] log: remove redundant check for merge commit","fromName":"Martin von Zweigbergk","fromEmail":"martin.von.zweigbergk@gmail.com","sentAt":"2012-07-29T04:57:49Z","receivedAt":"2012-07-29T04:57:49Z","isPatch":true,"sender":{"key":"martinvonz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/891642?v=4"},"body":"Sorry, I meant to CC the list. See below.\n\nOn Sat, Jul 28, 2012 at 9:56 PM, Martin von Zweigbergk\n wrote:\n> On Fri, Jul 27, 2012 at 11:52 PM, Junio C Hamano wrote:\n>> Junio C Hamano writes:\n>>\n>> It seems to have some interaction with your other topic, though.\n>> These two patches alone will pass the existing tests, but merging it\n>> with mz/rebase-range breaks t3412. I didn't look into it, but\n>> perhaps this breaks \"git cherry\" in some way?\n>\n> Yes, it breaks \"git cherry\" quite badly, by not ignoring merges at\n> all. I incorrectly assumed that ignore_merges was about revision\n> traversal, but now I think it's only diff output from 'git log' (and\n> possibly others). What I think tricked me was seeing that\n> \"ignore_merges = 1\" closely followed by a comment saying \"ignore\n> merges\". But now I think the explicit code to \"ignore merges\" is\n> necessary (as show by the failing test case), but can be replaced by\n> \"rev_info.max_parents = 1\". Setting \"ignore_merges = 1\", OTOH, now\n> seems doubly redundant: not only does it set the same value as was set\n> in init_revisions, but it's also irrelevant. Since cherry doesn't\n> generate any diff output, I think ignore_merges is never used.\n> Flipping the values of all of \"ignore_merges\", \"combine_merges\" and\n> \"diff\" does not have any effect on test cases at least. I hope my\n> explanation makes some sense at least...\n>\n> I'll send a reroll when I get time.\n"},{"id":"196054","messageId":"7vpq7fw0cr.fsf@alter.siamese.dyndns.org","threadId":"31115","inReplyTo":"CAOeW2eELzcqnecZqea2K0vO818PTYf-P=xEoSKb5GcW7W0Nf+A@mail.gmail.com","subject":"Re: [PATCH 2/2] log: remove redundant check for merge commit","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-07-29T06:24:52Z","receivedAt":"2012-07-29T06:24:52Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Martin von Zweigbergk writes:\n\n>> I incorrectly assumed that ignore_merges was about revision\n>> traversal, but now I think it's only diff output from 'git log' (and\n>> possibly others).\n\nYeah, I realized the same after I wrote the response last night and\nwent to bed. I am glad you figured all out yourself.\n"}]}