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
