Re: [PATCH v2] sha1-file: remove OBJECT_INFO_SKIP_CACHED

[Jonathan Nieder <jrnieder@gmail.com>]

Hi,

Jeff King wrote:
> On Fri, Jan 03, 2020 at 04:13:31PM -0800, Jonathan Nieder wrote:

>> To follow up on Junio's hint in his review: callers can inject
>> additional cached objects by using pretend_object_file.  Junio
>> described how this would make sense as a mechanism for building
>> the virtual ancestor object, but we don't do that.  In fact, the
>> only caller is fake_working_tree_commit in "git blame", a read-only
>> code path. *phew*
>>
>> -- >8 --
>> Subject: sha1-file: document how to use pretend_object_file
>> [...]
>> +/*
>> + * Add an object file to the in-memory object store, without writing it
>> + * to disk.
>> + *
>> + * Callers are responsible for calling write_object_file to record the
>> + * object in persistent storage before writing any other new objects
>> + * that reference it.
>> + */
>>  int pretend_object_file(void *, unsigned long, enum object_type,
>>  			struct object_id *oid);
>
> I think this is an improvement over the status quo, but it's still a
> potential trap for code which happens to run in the same process (see my
> other email in the thread).
>
> Should the message perhaps be even more scary?

A pet peeve of mine is warning volume escalation: if it becomes common
for us to say

 * Warning: callers are reponsible for [...]

then new warnings trying to stand out might say

 * WARNING: callers are responsible for [...]

and then after we are desensitized to that, we may switch to

 * WARNING WARNING WARNING, not the usual blah-blah: callers are

and so on.  The main way I have found to counteract that is to make
the "dangerous curve" markers context-specific enough that people
don't learn to ignore them.  After all, sometimes a concurrency
warning is important to me, at other times warnings about clarity may
be what attract my interest, and so on.

I don't have a good suggestion here.  Perhaps "Callers are responsible
for" is too slow and something more terse would help?

 /*
  * Adds an object to the in-memory object store, without writing it
  * to disk.
  *
  * Use with caution!  Unless you call write_object_file to record the
  * in-memory object to persistent storage, any other new objects that
  * reference it will point to a missing (in memory only) object,
  * resulting in a corrupt repository.
  */

It would be even better if we have some automated way to catch this
kind of issue.  Should tests run "git fsck" after each test?  Should
write_object_file have a paranoid mode that checks integrity?

I don't know an efficient way to do that.  Ultimately I am comfortable
counting on reviewers to be aware of this kind of pitfall.  While
nonlocal invariants are always hard to maintain, this pitfall is
inherent in the semantics of the function, so I am not too worried
that reviewers will overlook it.

A less error-prone interface would tie the result of
pretend_object_file to a short-lived overlay on the_repository without
affecting global state.  We could even enforce read-only access in
that overlay.  I don't think the "struct repository" interface and
callers are ready for that yet, though.

Thanks,
Jonathan