Re: [PATCH 1/5] Add a new option 'merges' to revision.c
- From
- Koosha Khajehmoogahi <koosha@posteo.de>
- Date
- Mar 23, 2015, 00:42 UTC
- Message-ID
- <550F6175.9050606@posteo.de>
- In-Reply-To
- <xmqqoankbodw.fsf@gitster.dls.corp.google.com>
On 03/23/2015 12:31 AM, Junio C Hamano wrote:
Show 12 quoted lines
> Koosha Khajehmoogahi <koosha@posteo.de> writes:
>
>> @@ -1800,9 +1817,14 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg
>> revs->show_all = 1;
>> } else if (!strcmp(arg, "--remove-empty")) {
>> revs->remove_empty_trees = 1;
>> + } else if (starts_with(arg, "--merges=")) {
>> + if (parse_merges_opt(revs, arg + 9))
>> + die("unknown option: %s", arg);
>
> This one makes sense to me (so does what the parse_merges_opt()
> does).In fact this change was written by you in your previous kind review :-)
I will add a 'From: Junio C Hamano <gitster@pobox.com>' header to my next patch.
Show 11 quoted lines
>
>> } else if (!strcmp(arg, "--merges")) {
>> + revs->max_parents = -1;
>> revs->min_parents = 2;
>
> But is this change warranted? An existing user who is not at all
> interested in the new --merges= option may be relying on the fact
> that "--merges" does not affect the value of max_parents and she can
> say "log --max-parents=2 --merges" to see only the two-way merges,
> for example. This change just broke her, and I do not see why it is
> a good thing.The point is that if you have your log.merges conf option set to 'hide' and you use git log --merges (two mutually conflicting options), git will silently exit without anything to show. We need to clear the max_parents set by parse_merges_opt() as the user should be able to continue to use --merges without problem. But, your point is also valid.
Show 7 quoted lines
>
>> } else if (!strcmp(arg, "--no-merges")) {
>> + revs->min_parents = 0;
>> revs->max_parents = 1;
>
> Likewise.
> Similarly, without this, if log.merges is set to 'only' and you use git log --no-merges, you will still see the merges in the output.