From: "Kristoffer Haugsbakk" <kristofferhaugsbakk@fastmail•com>
To: "Junio C Hamano" <gitster@pobox•com>,
"Kristoffer Haugsbakk" <kristofferhaugsbakk@fastmail•com>
Cc: git@vger•kernel.org
Subject: Re: [PATCH 1/2] revision: add rdiff_other_arg to rev_info
Date: Tue, 23 Sep 2025 17:53:54 +0200 [thread overview]
Message-ID: <ba9b7fb2-c990-44fb-a506-0800d02854a9@app.fastmail.com> (raw)
In-Reply-To: <xmqqikharvyl.fsf@gitster.g>
On Mon, Sep 22, 2025, at 23:58, Junio C Hamano wrote:
> 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.
>> 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)
> 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)
next prev parent reply other threads:[~2025-09-23 15:54 UTC|newest]
Thread overview: 15+ messages / expand[flat|nested] mbox.gz Atom feed top
2025-09-22 21:10 [PATCH 0/2] format-patch: handle range-diff on notes correctly for single patches kristofferhaugsbakk
2025-09-22 21:10 ` [PATCH 1/2] revision: add rdiff_other_arg to rev_info kristofferhaugsbakk
2025-09-22 21:58 ` Junio C Hamano
2025-09-23 15:53 ` Kristoffer Haugsbakk [this message]
2025-09-23 17:35 ` Junio C Hamano
2025-09-23 17:47 ` Kristoffer Haugsbakk
2025-09-23 21:18 ` Junio C Hamano
2025-09-22 21:10 ` [PATCH 2/2] format-patch: handle range-diff on notes correctly for single patches kristofferhaugsbakk
2025-09-22 22:01 ` Junio C Hamano
2025-09-23 16:26 ` Kristoffer Haugsbakk
2025-09-23 21:20 ` Junio C Hamano
2025-09-25 17:07 ` [PATCH v2 0/3] " kristofferhaugsbakk
2025-09-25 17:07 ` [PATCH v2 1/3] range-diff: rename other_arg to log_arg kristofferhaugsbakk
2025-09-25 17:07 ` [PATCH v2 2/3] revision: add rdiff_log_arg to rev_info kristofferhaugsbakk
2025-09-25 17:07 ` [PATCH v2 3/3] format-patch: handle range-diff on notes correctly for single patches kristofferhaugsbakk
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=ba9b7fb2-c990-44fb-a506-0800d02854a9@app.fastmail.com \
--to=kristofferhaugsbakk@fastmail$(echo .)com \
--cc=git@vger$(echo .)kernel.org \
--cc=gitster@pobox$(echo .)com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox