cmd_cherry in builtin/log.c?

cmd_cherry in builtin/log.c?

[Thiago Farina]

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?

Re: cmd_cherry in builtin/log.c?

[Jonathan Nieder]

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

Re: cmd_cherry in builtin/log.c?

[René Scharfe]

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é

Re: cmd_cherry in builtin/log.c?

[Junio C Hamano]

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.

Re: cmd_cherry in builtin/log.c?

[René Scharfe]

Am 07.12.2010 21:30, schrieb Junio C Hamano:
> 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é

Re: cmd_cherry in builtin/log.c?

[Thiago Farina]

On Tue, Dec 7, 2010 at 3:39 PM, Jonathan Nieder <jrnieder@gmail.com> 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.

Re: cmd_cherry in builtin/log.c?

[Jonathan Nieder]

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". :)