Re: [PATCH] difftool: eliminate use of global variables

From: Junio C Hamano <gitster@pobox.com>
Date: Feb 5, 2025, 17:30 UTC
Message-ID: <xmqq7c64pada.fsf@gitster.g>
In-Reply-To: <Z6MThg8oEEtx5xur@pks.im>

Patrick Steinhardt <ps@pks.im> writes:

Show 8 quoted lines

>> +struct difftool_state {
>> +	int has_symlinks;
>> +	int symlinks;
>> +	int trust_exit_code;
>> +};
>
> Why do we have both `has_symlinks` and `symlinks`? The latter gets set
> to `has_symlinks` anyway, so it's a confusing to have both.

I had the same reaction, but one aspect of the topic is about "encapsulate the existing globals into a state structure", and since these two are there in the original as globals, it would be easier to validate the correctness of the conversion to have both in the struct to keep the rewrite more faithful to the original. It would be more appropriate to do it in a separate step to turn them into one, if (I haven't thought about it, so this is still an "if" to me) it makes the results easier to follow.

The other aspect to lose the reference to the_repository indeed can be presented as a separate and independent change, and that may make the patch easier to view.

> Also, I think it would make sense to rename the struct to
> `difftool_options`, as it tracks options rather than state.

Great suggestion.

Thanks.