# cmd_cherry in builtin/log.c? 7 messages from 2010-12-07 to 2010-12-09. Participants: Thiago Farina, Jonathan Nieder, René Scharfe, Junio C Hamano. Thread: https://gitlist.dev/t/25982 ## Thiago Farina, 2010-12-07 16:02 Subject: cmd_cherry in builtin/log.c? Message-ID: URL: https://gitlist.dev/e/AANLkTikXmc-3BBkxy-D8GNLAMXUOPghp78GpStZ-_wBW%40mail.gmail.com ``` I was looking into builtin/log.c to see how it does --reverse and I saw that cmd_cherry is there. I'm wondering, why is it there? What was the reason to make the decision to put it there and not in builtin/cherry.c? ``` ## Jonathan Nieder, 2010-12-07 17:39 Subject: Re: cmd_cherry in builtin/log.c? Message-ID: <20101207173923.GA21483@burratino> URL: https://gitlist.dev/e/20101207173923.GA21483%40burratino In-Reply-To: ``` Hi, Thiago Farina wrote: > I was looking into builtin/log.c to see how it does --reverse and I > saw that cmd_cherry is there. > > I'm wondering, why is it there? Good question. So let's check. $ git log --oneline -Scmd_cherry builtin/log.c 81b50f3 Move 'builtin-*' into a 'builtin/' subdirectory That wasn't too helpful. Okay, okay. $ git log --oneline -Scmd_cherry -- builtin-log.c 81b50f3 Move 'builtin-*' into a 'builtin/' subdirectory e827633 Built-in cherry Running "git show e827633" reveals that the core of the original script is for c in $inup do git-diff-tree -p $c done | git-patch-id | while read id name do echo $name >>$patch/$id done while the core of the builtin version is get_patch_ids(&revs, &patch_id_opts, prefix); The latter function is static, introduced by v1.4.1~12^2~5 (format-patch: introduce "--ignore-if-in-upstream", 2006-06-25). So the answer is that "git cherry" is considered a variant on "git log" (like format-patch, show, and whatchanged) and that it uses "git log" internals. Hope that helps, Jonathan ``` ## René Scharfe, 2010-12-07 18:57 Subject: Re: cmd_cherry in builtin/log.c? Message-ID: <4CFE83A2.8070004@lsrfire.ath.cx> URL: https://gitlist.dev/e/4CFE83A2.8070004%40lsrfire.ath.cx In-Reply-To: <20101207173923.GA21483@burratino> ``` Am 07.12.2010 18:39, schrieb Jonathan Nieder: > while the core of the builtin version is > > get_patch_ids(&revs, &patch_id_opts, prefix); > > The latter function is static, introduced by v1.4.1~12^2~5 > (format-patch: introduce "--ignore-if-in-upstream", 2006-06-25). > > So the answer is that "git cherry" is considered a variant on > "git log" (like format-patch, show, and whatchanged) and that it uses > "git log" internals. That's right. get_patch_ids() could be moved into patch-ids.c now and then cmd_cherry() could get its own file in builtin/, though. René ``` ## Junio C Hamano, 2010-12-07 20:30 Subject: Re: cmd_cherry in builtin/log.c? Message-ID: <7vaakhpbq2.fsf@alter.siamese.dyndns.org> URL: https://gitlist.dev/e/7vaakhpbq2.fsf%40alter.siamese.dyndns.org In-Reply-To: <4CFE83A2.8070004@lsrfire.ath.cx> ``` René Scharfe writes: > Am 07.12.2010 18:39, schrieb Jonathan Nieder: >> while the core of the builtin version is >> >> get_patch_ids(&revs, &patch_id_opts, prefix); >> >> The latter function is static, introduced by v1.4.1~12^2~5 >> (format-patch: introduce "--ignore-if-in-upstream", 2006-06-25). >> >> So the answer is that "git cherry" is considered a variant on >> "git log" (like format-patch, show, and whatchanged) and that it uses >> "git log" internals. > > That's right. get_patch_ids() could be moved into patch-ids.c now and > then cmd_cherry() could get its own file in builtin/, though. Right, but the key word is "could". Is it hurting _anything_ to have it in the current place? I doubt it. ``` ## Thiago Farina, 2010-12-08 11:57 Subject: Re: cmd_cherry in builtin/log.c? Message-ID: URL: https://gitlist.dev/e/AANLkTik7%2BWAk0mx0G7b565eCihB5SGNBDuYLdfPzq7pr%40mail.gmail.com In-Reply-To: <20101207173923.GA21483@burratino> ``` On Tue, Dec 7, 2010 at 3:39 PM, Jonathan Nieder wrote: > Hi, > > Thiago Farina wrote: > >> I was looking into builtin/log.c to see how it does --reverse and I >> saw that cmd_cherry is there. >> >> I'm wondering, why is it there? > > Good question.  So let's check. > >  $ git log --oneline -Scmd_cherry builtin/log.c >  81b50f3 Move 'builtin-*' into a 'builtin/' subdirectory > > That wasn't too helpful.  Okay, okay. > >  $ git log --oneline -Scmd_cherry -- builtin-log.c >  81b50f3 Move 'builtin-*' into a 'builtin/' subdirectory >  e827633 Built-in cherry > > Running "git show e827633" reveals that the core of the original > script is > >        for c in $inup >        do >                git-diff-tree -p $c >        done | git-patch-id | >        while read id name >        do >                echo $name >>$patch/$id >        done > > while the core of the builtin version is > >        get_patch_ids(&revs, &patch_id_opts, prefix); > > The latter function is static, introduced by v1.4.1~12^2~5 > (format-patch: introduce "--ignore-if-in-upstream", 2006-06-25). > > So the answer is that "git cherry" is considered a variant on > "git log" (like format-patch, show, and whatchanged) and that it uses > "git log" internals. > Yup, thanks for digging into it. Now makes sense. ``` ## Jonathan Nieder, 2010-12-08 18:09 Subject: Re: cmd_cherry in builtin/log.c? Message-ID: <20101208180925.GE5687@burratino> URL: https://gitlist.dev/e/20101208180925.GE5687%40burratino In-Reply-To: ``` Thiago Farina wrote: > Yup, thanks for digging into it. Now makes sense. Mm, I was hoping for "thanks for teaching me to dig into it". :) ``` ## René Scharfe, 2010-12-09 21:14 Subject: Re: cmd_cherry in builtin/log.c? Message-ID: <4D0146BA.2090801@lsrfire.ath.cx> URL: https://gitlist.dev/e/4D0146BA.2090801%40lsrfire.ath.cx In-Reply-To: <7vaakhpbq2.fsf@alter.siamese.dyndns.org> ``` Am 07.12.2010 21:30, schrieb Junio C Hamano: > René Scharfe writes: > >> Am 07.12.2010 18:39, schrieb Jonathan Nieder: >>> while the core of the builtin version is >>> >>> get_patch_ids(&revs, &patch_id_opts, prefix); >>> >>> The latter function is static, introduced by v1.4.1~12^2~5 >>> (format-patch: introduce "--ignore-if-in-upstream", 2006-06-25). >>> >>> So the answer is that "git cherry" is considered a variant on >>> "git log" (like format-patch, show, and whatchanged) and that it uses >>> "git log" internals. >> >> That's right. get_patch_ids() could be moved into patch-ids.c now and >> then cmd_cherry() could get its own file in builtin/, though. > > Right, but the key word is "could". Is it hurting _anything_ to have it > in the current place? I doubt it. Indeed. Moving cherry's code into its own file is be a code clean up. There would be no benefit to users, future developers should have a somewhat easier time navigating the code while any developers currently working on the code would get it pulled from under them. Such a clean up can be useful to include at the start of a patch series that contains actual user visible improvements. René ```