Elijah Newren <newren@xxxxxxxxx> writes:
> On Wed, Dec 16, 2020 at 10:50 AM Sergey Organov <sorganov@xxxxxxxxx> wrote:
>>
>> Move logic that handles implying -p on -c/--cc from
>> log_setup_revisions_tweak() to diff_merges_setup_revs(), where it
>> belongs.
>
> A very minor point, but I'd probably drop the "where it belongs";
> while I think the new place makes sense for it, it reads to me like
> you're either relying on a consensus to move it or implying there was
> a mistake to not put it here previously, neither of which makes sense.
Well, it was meant to be an excuse for not moving it there earlier in
the patch series indeed. I just overlooked this piece of code that
logically belongs to the diff-merges module. I think you need to
consider the state of the sources right before this patch to see the
point of phrasing it like this.
That said, I'm fine removing this either.
> Much more importantly, this patch doesn't do what you said in
> discussions on the previous round. It'd be helpful if the commit
> message called out that you are just moving the logic for now and that
> a subsequent patch will tweak the logic to only trigger this for
> -c/--cc and not for --diff-merges=.* flags.
I believe this patch is useful by itself, even without any future
improvements (that we actually discussed), if any, so I don't see the
point in describing what this patch doesn't do.
OTOH, the commit message seems to be clear enough to expect this patch
to be pure refactoring, without any functional changes, no?
Thanks,
-- Sergey
>
>
>> Signed-off-by: Sergey Organov <sorganov@xxxxxxxxx>
>> ---
>> builtin/log.c | 4 ----
>> diff-merges.c | 7 ++++++-
>> 2 files changed, 6 insertions(+), 5 deletions(-)
>>
>> diff --git a/builtin/log.c b/builtin/log.c
>> index 63875c3aeec9..c3caf0955b2b 100644
>> --- a/builtin/log.c
>> +++ b/builtin/log.c
>> @@ -723,10 +723,6 @@ static void log_setup_revisions_tweak(struct rev_info *rev,
>> rev->prune_data.nr == 1)
>> rev->diffopt.flags.follow_renames = 1;
>>
>> - /* Turn --cc/-c into -p --cc/-c when -p was not given */
>> - if (!rev->diffopt.output_format && rev->combine_merges)
>> - rev->diffopt.output_format = DIFF_FORMAT_PATCH;
>> -
>> if (rev->first_parent_only)
>> diff_merges_default_to_first_parent(rev);
>> }
>> diff --git a/diff-merges.c b/diff-merges.c
>> index 0165fa22fcd1..2ac25488d53e 100644
>> --- a/diff-merges.c
>> +++ b/diff-merges.c
>> @@ -127,6 +127,11 @@ void diff_merges_setup_revs(struct rev_info *revs)
>> revs->first_parent_merges = 0;
>> if (revs->combined_all_paths && !revs->combine_merges)
>> die("--combined-all-paths makes no sense without -c or --cc");
>> - if (revs->combine_merges)
>> + if (revs->combine_merges) {
>> revs->diff = 1;
>> + /* Turn --cc/-c into -p --cc/-c when -p was not given */
>> + if (!revs->diffopt.output_format)
>> + revs->diffopt.output_format = DIFF_FORMAT_PATCH;
>> + }
>> +
>> }
>> --
>> 2.25.1
>>
