Patrick Steinhardt <ps@xxxxxx> writes:
>> +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.
