Re: [PATCH v2 1/4] promisor-remote: read partialClone config here

Jonathan Tan [off-list ref] writes:

quoted

Currently, the reading of config related to promisor remotes is done in
two places: once in setup.c (which sets the global variable
repository_format_partial_clone, to be read by the code in
promisor-remote.c), and once in promisor-remote.c. This means that care
must be taken to ensure that repository_format_partial_clone is set
before any code in promisor-remote.c accesses it.

The above is very true, but I am puzzled by the chosen direction of
the code movement.

Given that the value in the field repository_format.partial_clone
comes from an extension, and an extension that is not understood by
the version of Git that is running MUST abort the execution of Git,
wouldn't it be guaranteed that, in a correctly written program, the
.partial_clone field must already be set up correctly before
anything else, including those in promissor-remote.c, accesses it?

quoted

To simplify the code, move all such config reading to promisor-remote.c.
By doing this, it will be easier to see when
repository_format_partial_clone is written and, thus, to reason about
the code. This will be especially helpful in a subsequent commit, which
modifies this code.

So, I am not sure if this simplifies the code the way we want to
read our code.  Doing a thing in one place is indeed simpler than
doing it in two places, but it looks like promisor-remote code
should be using the repository-format data more, not the other way
around, at least to me.

Perhaps I am missing some other motivation, though.

Thanks.

I'm reluctant to add more fields to struct repository_format. Right
now, the way it is used is to hold any information we gathered (e.g.
hash type) while determining if a repo is one that we can handle. Any
information we still need is copied somewhere else, and the struct
itself is immediately freed.

If we were to use it for promisor remote config, we would have to
read config into struct repository_format's fields and copy those fields
into struct repository in setup.c, and then access the same fields in
promisor-remote.c. It seems more straightforward to just do everything
in promisor-remote.c - for example, if we needed to change the type of
one of those fields, we would just need to change it in one file instead
of two.

I acknowledge that there still remains the duplication that setup.c
needs to know that extensions.partialClone is a valid extension, and
that promsior-remote.c needs to interpret extensions.partialClone.

Having said that, I don't feel very strongly about keeping everything in
promisor-remote.c, so I can move it into setup.c if that's the reviewer
consensus.