Re: [PATCH 1/2] revision: add rdiff_other_arg to rev_info

From: Kristoffer Haugsbakk <kristofferhaugsbakk@fastmail.com>
Date: Sep 23, 2025, 15:53 UTC
Message-ID: <ba9b7fb2-c990-44fb-a506-0800d02854a9@app.fastmail.com>
In-Reply-To: <xmqqikharvyl.fsf@gitster.g>

On Mon, Sep 22, 2025, at 23:58, Junio C Hamano wrote:

Show 8 quoted lines

> kristofferhaugsbakk@fastmail.com writes:
>
>> git-format-patch(1) is supposed to treat Git notes the same between
>> notes output beneath the commit message and the notes output for the
>> range-diff.
>
> Is this an opinion, or are there things that existing pieces of code
> already do to achieve such a behaviour already?

What I mean is that

    Notes (...)

Beneath the commit message and

    ### Notes (...) ###

In the range-diff should be from the same namespaces. It shouldn’t be, for example:

    Notes (presentation):

Beneath the commit message while the range-diff has:

    ### Notes (testing) ###
    ...
    ### Notes (scratchpad) ###

That’s the point of passing `--notes` to range-diff.

Am I missing something in the explanation? The commit message might need a rewrite.

Show 46 quoted lines

>> diff --git a/revision.h b/revision.h
>> index 21e288c5baa..26c18a0934b 100644
>> --- a/revision.h
>> +++ b/revision.h
>> @@ -334,6 +334,7 @@ struct rev_info {
>>  	/* range-diff */
>>  	const char *rdiff1;
>>  	const char *rdiff2;
>> +	struct strvec rdiff_other_arg;
>>  	int creation_factor;
>>  	const char *rdiff_title;
>
> When embedding a struct A in a struct B, we should always make sure
> that initialization macro/function for struct B is updated so that
> the initialization for struct A is done correctly for the new member.
>
> We do have REV_INFO_INIT for "struct rev_info"
>
>         #define REV_INFO_INIT { \
>                 .abbrev = DEFAULT_ABBREV, \
>                 .simplify_history = 1, \
>                 .pruning.flags.recursive = 1, \
>                 ...
>                 .expand_tabs_in_log_default = 8, \
>         }
>
> that does not allow any existing callers to leave it uninitialized
> or get away by zero-initializing, so all the users must be using it
> or the system before your patch is already buggy.
>
> And we do have STRVEC_INIT that we must use in that initializer.
>
>         extern const char *empty_strvec[];
>
>         struct strvec {
>                 const char **v;
>                 size_t nr;
>                 size_t alloc;
>         };
>
>         #define STRVEC_INIT { \
>                 .v = empty_strvec, \
>         }
>
> So this step forgets to update revision.h to teach STRVEC_INIT on
> the new rdiff_other_arg member.

Thanks for the explanation. I’ve added `.rdiff_other_arg = STRVEC_INIT \` to `REV_INFO_INIT`.

> Back when it was a random one-shot variable in range-diff, it might
> not have mattered all that much, but now we have it as a proper
> member of the struct, can we give it a name better than 'other_arg"?

I’ve had that thought too. But then I forgot what a better name would be.

Could it be as simple as `log_arg` or `log_args`?

(This could be added as a preliminary patch)

`builtin/range-diff.c` uses `range_diff_options.other_arg` to pass `--notes` but also (and quite recently[1]) to pass `--merges`.

    /* If `--diff-merges` was specified, imply `--merges` */

† 1: f8043236 (range-diff: optionally include merge commits' diffs in
    the analysis, 2024-12-16)

Show 5 quoted lines

> Or is it the case that truly any random crap can be slurped into the
> array and thrown back at "git log" without range-diff machinery
> understanding what it is doing at all (which I would not be
> surprised, as some parts of our code base is written in somewhat a
> sloppy way)?

Well anything you throw into it will ultimately end up in the git-log(1) child process in `range-diff.c:73`.[2] But this doesn’t seem like a problem given that all the arguments are “programmed” as constant strings?

† 2: on ca2559c1 (The tenth batch, 2025-09-18)