Re: [PATCH v3] diff: make diff_free_filespec_data accept NULL

From: Junio C Hamano <gitster@pobox.com>
Date: Nov 10, 2020, 17:02 UTC
Message-ID: <xmqqblg587sa.fsf@gitster.c.googlers.com>
In-Reply-To: <nycvar.QRO.7.76.6.2011101257540.18437@tvgsbejvaqbjf.bet>

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

Show 15 quoted lines

>>  void diff_free_filespec_data(struct diff_filespec *s)
>>  {
>> +	if (!s)
>> +		return;
>> +
>>  	diff_free_filespec_blob(s);
>>  	FREE_AND_NULL(s->cnt_data);
>
> We can write this in a more canonical way without wasting all that many
> lines:
>
> 	if (s) {
> 		diff_free_filespec_blob(s);
> 		FREE_AND_NULL(s->cnt_data);
> 	}

On only this part. Please do not use this one, as what was posted is better.

By making it clear that we do not do anything when given NULL upfront, it lets readers concentrate on the main part of the function---"now we are done with NULL, what happens on a real case?" And when readers are doing so, one extra indentation level is just an extra noise that does not help.

In this particular case, it is important more from coding discipline than anything else---for a small enough function like this one whose main part fits on less than fourth of a screen, extra indentation does not matter, but all code tends to grow and it starts to matter if/when we need to clean more things in the function.

Everything else said in the review was excellent and very helpful.

Thanks.