Re: What's cooking in git.git (Sep 2021, #08; Mon, 27)

On Thu, Sep 30, 2021 at 1:16 AM Jeff King [off-list ref] wrote:

On Wed, Sep 29, 2021 at 11:43:39AM -0700, Neeraj Singh wrote:

quoted

It seems to me that one problem is that the new "primary" objdir code doesn't
disable ref updates since the GIT_QUARANTINE_ENVIRONMENT setting isn't set.
If we fix that we should be forbidding ref updates.

I've included a path that fixes my test case. This is on top of:
https://lore.kernel.org/git/CANQDOddqwVtWfC4eEP3fJB4sUiszGX8bLqoEVLcMf=v+jzx19g@mail.gmail.com/ (local)

Ah, right, I forgot we had that "forbid ref updates in quarantine" magic
(despite being the one who added it).

I do think this improves the safety, but things are still a bit more
dangerous because we're handling it all in a single process, which sees
both the quarantine and non-quarantine states. I wrote more details in
this reply a few minutes ago:

  https://lore.kernel.org/git/YVVmssXlaFM6yD5W@coredump.intra.peff.net/ (local)

(sorry, it's long; search for the paragraph starting with "Whereas doing
it in-process").

quoted

When creating a subprocess with a temporary ODB, we set the
GIT_QUARANTINE_ENVIRONMENT env var to tell child Git processes not
to update refs, since the tmp-objdir may go away.

Introduce a similar mechanism for in-process temporary ODBs when
we call tmp_objdir_replace_primary_odb.

Peff's test case was invoking ref updates via the cachetextconv
setting. That particular code silently does nothing when a ref
update is forbidden

Oh good. I was worried that it would generate ugly errors, rather than
silently skipping the update.

quoted

@@ -2126,7 +2146,7 @@ int ref_transaction_prepare(struct ref_transaction *transaction,
              break;
      }

-     if (getenv(GIT_QUARANTINE_ENVIRONMENT)) {
+     if (getenv(GIT_QUARANTINE_ENVIRONMENT) || !ref_updates_are_enabled()) {
              strbuf_addstr(err,
                            _("ref updates forbidden inside quarantine environment"));
              return -1;

I think the overall goal of this patch makes sense, but this
conditional, along with tmp-objdir reaching out to the refs code, feels
funny. Should we perhaps have a single:

  int tmp_objdir_is_primary(struct repository *r)
  {
        if (the_tmp_objdir &&
            !strcmp(the_tmp_objdir->path.buf, r->objects->odb->path))
                return 1; /* our temporary is the primary */

        if (getenv(GIT_QUARANTINE_PATH))
                return 1; /* our caller put us in quarantine */

        return 0;
  }

Then it's all nicely abstracted and the ref code does not have to know
the details of GIT_QUARANTINE_PATH in the first place.

If you do got that route, the strcmp() might need to be a little more
careful about whether r->objects can be NULL (I didn't check).
Alternatively, I kind of wonder if "struct object_directory" should just
have a flag that says "is_temporary".

Actually, wouldn't this be the safest approach, for my particular
usecase?  By having the quarantine in place, no refs will be updated,
which removes the risk of new refs mentioning objects that will go
away.  The only issue that could arise would be from new objects
referencing objects that will go away.  But the new objects are also
written to the temporary object store when it remains the primary, and
thus those new objects will also be deleted when the temporary object
store is.

In contrast, moving the temporary object store to the end of the
alternates list would allow new objects to be written that reference
the objects that will go away.  Using pretend_object_file() will also
allow new objects to be written that reference the objects that will
go away.

Said another way, I don't think anything should be writing a critical
file that needs to be durable when we're in the middle of a
"read-only" process like git-log.  The only things written should be
temporary stuff, like the automatic remerge calculation from
merge-ort, the textconv cache optimization stuff, or perhaps future
gitattributes transformation caching.  All that stuff can safely be
blown away at the completion of each merge.