Re: [PATCH 2/3] setup: do not use invalid `repository_format`

From: Jeff King <peff@peff.net>
To: "Martin Ågren" <martin.agren@gmail.com>
Cc: Git Mailing List <git@vger.kernel.org>,
	"brian m . carlson" <sandals@crustytoothpaste.net>
Subject: Re: [PATCH 2/3] setup: do not use invalid `repository_format`
Date: Wed, 19 Dec 2018 18:17:16 -0500	[thread overview]
Message-ID: <20181219231716.GA21283@sigill.intra.peff.net> (raw)
In-Reply-To: <CAN0heSpc_sap1cZktteRn3hyeuRx2w86Hd2kqxu4XFgHs75_Kw@mail.gmail.com>

On Wed, Dec 19, 2018 at 10:46:52PM +0100, Martin Ågren wrote:

> >   2. Arguably we should not even look at extensions.* unless we see a
> >      version >= 1. But we do process them as we parse the config file.
> >      This is mostly an oversight, I think. We have to handle them as we
> >      see them, because they may come out of order with respect to the
> >      repositoryformatversion field. But we could put them into a
> >      string_list, and then only process them after we've decided which
> >      version we have.
> 
> I hadn't thought too much about this. I guess that for some simpler
> extensions--versions dependencies it would be feasible to first parse
> everything, then, depending on the version we've identified, forget
> about any "irrelevant" extensions. Again, nothing I've thought much
> about, and seems to be safely out of scope for this patch.

The decision is actually pretty straight-forward: if version < 1, ignore
extensions, otherwise respect them (and complain about any we don't know
about).

So I think we could just do in verify_repository_format() something
like:

  if (version < 1) {
    /* "undo" any extensions we might have parsed */
    data->precious_objects = 0;
    FREE_AND_NULL(data->partial_clone);
    data->worktree_config = 0;
    data->hash_algo = GIT_HASH_SHA1;
  } else {
    /* complain about unknown extension; we already do this! */
  }

It's a little ugly to have to know about all the extensions here, but we
already initialize them in read_repository_format(). We could probably
factor that out into a shared function.

-Peff