{"thread":{"id":"25982","subject":"cmd_cherry in builtin/log.c?","startedAt":"2010-12-07T16:02:36Z","lastAt":"2010-12-09T21:14:34Z","messageCount":7,"participants":["Thiago Farina","Jonathan Nieder","René Scharfe","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"157489","messageId":"AANLkTikXmc-3BBkxy-D8GNLAMXUOPghp78GpStZ-_wBW@mail.gmail.com","threadId":"25982","inReplyTo":null,"subject":"cmd_cherry in builtin/log.c?","fromName":"Thiago Farina","fromEmail":"tfransosi@gmail.com","sentAt":"2010-12-07T16:02:36Z","receivedAt":"2010-12-07T16:02:36Z","isPatch":false,"sender":{"key":"tfransosi@gmail.com","avatar":"https://avatars.githubusercontent.com/u/970071?v=4"},"body":"I was looking into builtin/log.c to see how it does --reverse and I\nsaw that cmd_cherry is there.\n\nI'm wondering, why is it there? What was the reason to make the\ndecision to put it there and not in builtin/cherry.c?\n"},{"id":"157503","messageId":"20101207173923.GA21483@burratino","threadId":"25982","inReplyTo":"AANLkTikXmc-3BBkxy-D8GNLAMXUOPghp78GpStZ-_wBW@mail.gmail.com","subject":"Re: cmd_cherry in builtin/log.c?","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2010-12-07T17:39:23Z","receivedAt":"2010-12-07T17:39:23Z","isPatch":false,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nThiago Farina wrote:\n\n> I was looking into builtin/log.c to see how it does --reverse and I\n> saw that cmd_cherry is there.\n> \n> I'm wondering, why is it there?\n\nGood question.  So let's check.\n\n $ git log --oneline -Scmd_cherry builtin/log.c\n 81b50f3 Move 'builtin-*' into a 'builtin/' subdirectory\n\nThat wasn't too helpful.  Okay, okay.\n\n $ git log --oneline -Scmd_cherry -- builtin-log.c\n 81b50f3 Move 'builtin-*' into a 'builtin/' subdirectory\n e827633 Built-in cherry\n\nRunning \"git show e827633\" reveals that the core of the original\nscript is\n\n\tfor c in $inup\n\tdo\n\t\tgit-diff-tree -p $c\n\tdone | git-patch-id |\n\twhile read id name\n\tdo\n\t\techo $name >>$patch/$id\n\tdone\n\nwhile the core of the builtin version is\n\n\tget_patch_ids(&revs, &patch_id_opts, prefix);\n\nThe latter function is static, introduced by v1.4.1~12^2~5\n(format-patch: introduce \"--ignore-if-in-upstream\", 2006-06-25).\n\nSo the answer is that \"git cherry\" is considered a variant on\n\"git log\" (like format-patch, show, and whatchanged) and that it uses\n\"git log\" internals.\n\nHope that helps,\nJonathan\n"},{"id":"157521","messageId":"4CFE83A2.8070004@lsrfire.ath.cx","threadId":"25982","inReplyTo":"20101207173923.GA21483@burratino","subject":"Re: cmd_cherry in builtin/log.c?","fromName":"René Scharfe","fromEmail":"rene.scharfe@lsrfire.ath.cx","sentAt":"2010-12-07T18:57:38Z","receivedAt":"2010-12-07T18:57:38Z","isPatch":false,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 07.12.2010 18:39, schrieb Jonathan Nieder:\n> while the core of the builtin version is\n> \n> \tget_patch_ids(&revs, &patch_id_opts, prefix);\n> \n> The latter function is static, introduced by v1.4.1~12^2~5\n> (format-patch: introduce \"--ignore-if-in-upstream\", 2006-06-25).\n> \n> So the answer is that \"git cherry\" is considered a variant on\n> \"git log\" (like format-patch, show, and whatchanged) and that it uses\n> \"git log\" internals.\n\nThat's right.  get_patch_ids() could be moved into patch-ids.c now and\nthen cmd_cherry() could get its own file in builtin/, though.\n\nRené\n"},{"id":"157532","messageId":"7vaakhpbq2.fsf@alter.siamese.dyndns.org","threadId":"25982","inReplyTo":"4CFE83A2.8070004@lsrfire.ath.cx","subject":"Re: cmd_cherry in builtin/log.c?","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-12-07T20:30:29Z","receivedAt":"2010-12-07T20:30:29Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe <rene.scharfe@lsrfire.ath.cx> writes:\n\n> Am 07.12.2010 18:39, schrieb Jonathan Nieder:\n>> while the core of the builtin version is\n>> \n>> \tget_patch_ids(&revs, &patch_id_opts, prefix);\n>> \n>> The latter function is static, introduced by v1.4.1~12^2~5\n>> (format-patch: introduce \"--ignore-if-in-upstream\", 2006-06-25).\n>> \n>> So the answer is that \"git cherry\" is considered a variant on\n>> \"git log\" (like format-patch, show, and whatchanged) and that it uses\n>> \"git log\" internals.\n>\n> That's right.  get_patch_ids() could be moved into patch-ids.c now and\n> then cmd_cherry() could get its own file in builtin/, though.\n\nRight, but the key word is \"could\".  Is it hurting _anything_ to have it\nin the current place?  I doubt it.\n"},{"id":"157599","messageId":"AANLkTik7+WAk0mx0G7b565eCihB5SGNBDuYLdfPzq7pr@mail.gmail.com","threadId":"25982","inReplyTo":"20101207173923.GA21483@burratino","subject":"Re: cmd_cherry in builtin/log.c?","fromName":"Thiago Farina","fromEmail":"tfransosi@gmail.com","sentAt":"2010-12-08T11:57:59Z","receivedAt":"2010-12-08T11:57:59Z","isPatch":false,"sender":{"key":"tfransosi@gmail.com","avatar":"https://avatars.githubusercontent.com/u/970071?v=4"},"body":"On Tue, Dec 7, 2010 at 3:39 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:\n> Hi,\n>\n> Thiago Farina wrote:\n>\n>> I was looking into builtin/log.c to see how it does --reverse and I\n>> saw that cmd_cherry is there.\n>>\n>> I'm wondering, why is it there?\n>\n> Good question.  So let's check.\n>\n>  $ git log --oneline -Scmd_cherry builtin/log.c\n>  81b50f3 Move 'builtin-*' into a 'builtin/' subdirectory\n>\n> That wasn't too helpful.  Okay, okay.\n>\n>  $ git log --oneline -Scmd_cherry -- builtin-log.c\n>  81b50f3 Move 'builtin-*' into a 'builtin/' subdirectory\n>  e827633 Built-in cherry\n>\n> Running \"git show e827633\" reveals that the core of the original\n> script is\n>\n>        for c in $inup\n>        do\n>                git-diff-tree -p $c\n>        done | git-patch-id |\n>        while read id name\n>        do\n>                echo $name >>$patch/$id\n>        done\n>\n> while the core of the builtin version is\n>\n>        get_patch_ids(&revs, &patch_id_opts, prefix);\n>\n> The latter function is static, introduced by v1.4.1~12^2~5\n> (format-patch: introduce \"--ignore-if-in-upstream\", 2006-06-25).\n>\n> So the answer is that \"git cherry\" is considered a variant on\n> \"git log\" (like format-patch, show, and whatchanged) and that it uses\n> \"git log\" internals.\n>\n\nYup, thanks for digging into it. Now makes sense.\n"},{"id":"157617","messageId":"20101208180925.GE5687@burratino","threadId":"25982","inReplyTo":"AANLkTik7+WAk0mx0G7b565eCihB5SGNBDuYLdfPzq7pr@mail.gmail.com","subject":"Re: cmd_cherry in builtin/log.c?","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2010-12-08T18:09:25Z","receivedAt":"2010-12-08T18:09:25Z","isPatch":false,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Thiago Farina wrote:\n\n> Yup, thanks for digging into it. Now makes sense.\n\nMm, I was hoping for \"thanks for teaching me to dig into it\". :)\n"},{"id":"157728","messageId":"4D0146BA.2090801@lsrfire.ath.cx","threadId":"25982","inReplyTo":"7vaakhpbq2.fsf@alter.siamese.dyndns.org","subject":"Re: cmd_cherry in builtin/log.c?","fromName":"René Scharfe","fromEmail":"rene.scharfe@lsrfire.ath.cx","sentAt":"2010-12-09T21:14:34Z","receivedAt":"2010-12-09T21:14:34Z","isPatch":false,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 07.12.2010 21:30, schrieb Junio C Hamano:\n> René Scharfe <rene.scharfe@lsrfire.ath.cx> writes:\n> \n>> Am 07.12.2010 18:39, schrieb Jonathan Nieder:\n>>> while the core of the builtin version is\n>>>\n>>> \tget_patch_ids(&revs, &patch_id_opts, prefix);\n>>>\n>>> The latter function is static, introduced by v1.4.1~12^2~5\n>>> (format-patch: introduce \"--ignore-if-in-upstream\", 2006-06-25).\n>>>\n>>> So the answer is that \"git cherry\" is considered a variant on\n>>> \"git log\" (like format-patch, show, and whatchanged) and that it uses\n>>> \"git log\" internals.\n>>\n>> That's right.  get_patch_ids() could be moved into patch-ids.c now and\n>> then cmd_cherry() could get its own file in builtin/, though.\n> \n> Right, but the key word is \"could\".  Is it hurting _anything_ to have it\n> in the current place?  I doubt it.\n\nIndeed.  Moving cherry's code into its own file is be a code clean up.\nThere would be no benefit to users, future developers should have a\nsomewhat easier time navigating the code while any developers currently\nworking on the code would get it pulled from under them.\n\nSuch a clean up can be useful to include at the start of a patch series\nthat contains actual user visible improvements.\n\nRené\n"}]}