From: Junio C Hamano <gitster@pobox.com>
Date: Mon, 22 Sep 2025 21:58:10 GMT
Subject: Re: [PATCH 1/2] revision: add rdiff_other_arg to rev_info
Message-ID: <xmqqikharvyl.fsf@gitster.g>
In-Reply-To: <bb065767336.1758574974.git.code@khaugsbakk.name>

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?

> 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.

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"?
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)?

Thanks.