Re: [PATCH] apply: Avoid ambiguous pointer provenance for CHERI/Arm's Morello

[Junio C Hamano <gitster@pobox.com>]

Jessica Clarke <jrtc27@jrtc27.com> writes:

>> I actually wonder if it results in code that is much easier to
>> follow if we did:
>> 
>> * Introduce an "enum apply_symlink_treatment" that has
>>   APPLY_SYMLINK_GOES_AWAY and APPLY_SYMLINK_IN_RESULT as its
>>   possible values;
>> 
>> * Make register_symlink_changes() and check_symlink_changes()
>>   work with "enum apply_symlink_treatment";
>> 
>> * (optional) stop using string_list() to store the symlink_changes;
>>   use strintmap and use strintmap_set() and strintmap_get() to
>>   access its entries, so that the ugly implementation detail
>>   (i.e. "the container type we use only has a (void *) field to
>>   store caller-supplied data, so we cast an integer and a pointer
>>   back and forth") can be safely hidden.
>
> Those would be better if you want a less-minimal change.

The first two at least would make an understandable change, as
opposed to the code as posted, which is totally opaque why we need
to take size_t and cast it to uintptr_t in the first place.

I have no confidence in myself that I would not accept a future
patch that reverts the change made by this patch happily.  I usually
try to be careful to go back to the originating commit by running
"blame" on the preimage and may find your commit, but that's only
"usually".  If I were to work only with the file contents after
applying this patch, because no clue ...

	ent->util = (void *)((uintptr_t)what | ((uintptr_t)ent->util));

... on this line of code hints why we must be called with size_t and
have to cast it here, instead of working with uintptr_t throughout,
I am reasonably sure I'd happily take such a patch and break your
"fix" here.

If we make the code pass around the enum, have a temporary variable
of the same enum type to compute the next value we stuff in
ent->util, and make an assignment of that enum value to ent->util,
it is much less likely that I'd blindly accept a patch to take us
back to deal with uintptr_t directly.