Thiago FarinaI 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?
Thiago Farina
Jonathan NiederRe: cmd_cherry in builtin/log.c?
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é ScharfeRe: cmd_cherry in builtin/log.c?
Am 07.12.2010 18:39, schrieb Jonathan Nieder:
Show 10 quoted lines
> 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 HamanoRe: cmd_cherry in builtin/log.c?
René Scharfe <rene.scharfe@lsrfire.ath.cx> writes:
Show 14 quoted lines
> 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.
René ScharfeRe: cmd_cherry in builtin/log.c?
Am 07.12.2010 21:30, schrieb Junio C Hamano:
Show 19 quoted lines
> René Scharfe <rene.scharfe@lsrfire.ath.cx> 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é
Thiago FarinaRe: cmd_cherry in builtin/log.c?
On Tue, Dec 7, 2010 at 3:39 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:
Show 43 quoted lines
> 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 NiederRe: cmd_cherry in builtin/log.c?
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". :)