Re: [PATCH v2 2/3] notes.c: fixed tip when target and append note are both empty

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Nov 7, 2022, 22:33 UTC
Message-ID: <221107.86bkpitmw6.gmgdl@evledraar.gmail.com>
In-Reply-To: <Y2l9yfOhiT7r2+H6@nand.local>

On Mon, Nov 07 2022, Taylor Blau wrote:

Show 26 quoted lines

> On Mon, Nov 07, 2022 at 03:40:03PM +0100, Ævar Arnfjörð Bjarmason wrote:
>> > @@ -567,9 +567,15 @@ static int append_edit(int argc, const char **argv, const char *prefix)
>> >  	struct notes_tree *t;
>> >  	struct object_id object, new_note;
>> >  	const struct object_id *note;
>> > -	char *logmsg;
>> > +	char *logmsg = NULL;
>> >  	const char * const *usage;
>> > -	struct note_data d = { 0, 0, NULL, STRBUF_INIT };
>> > +	struct note_data d = {
>> > +		.given = 0,
>> > +		.use_editor = 0,
>> > +		.edit_path = NULL,
>>
>> Most of this is an unrelated "use designated init" cleanup, which is
>> good, but let's do that in a different commit if we need it. And then
>> you can drop all of these...
>>
>> > +		.buf = STRBUF_INIT
>>
>> ...and only need to keep this one.
>
> I don't mind seeing the cleanup here, but I don't see why we need to
> keep that portion of the hunk at all for this series.
>
> IOW, your "...and only need to keep this one" does not make sense to me.

I was still on my initial read-through, and was assuming that it was a prep change for the 3/3 adding a new field, before I saw the 3/3...

> Changing "char *logmsg" to be initialized to NULL is important, though,
> since we now only set it conditionally still free it unconditionally. So
> that should be kept.