On Fri, Jun 16, 2017 at 09:28:32PM +0200, SZEDER Gábor wrote:
> 'struct remote' stores refspecs twice: once in their original string
> form in remote->{fetch,push}_refspecs and once in their parsed form in
> remote->{fetch,push}. This is necessary, because we need the refspecs
> for lazy parsing after we finished reading the configuration: we don't
> want to die() on a bogus refspec while reading the configuration of a
> remote we are not going to access at all.
>
> However, storing refspecs in both forms has some drawbacks:
>
> - The same information is stored twice, wasting memory.
I don't think this is really that important. It's rare to have more than
a handful of refspecs, and they're short strings.
> - remote->{fetch,push}_refspecs, i.e. the string arrays are
> conveniently ALLOC_GROW()-able with associated
> {fetch,push}_refspec_{nr,alloc} fields, but remote->{fetch,push}
> are not.
> - Wherever remote->{fetch,push} are accessed, the number of parsed
> refspecs in there is specified by remote->{fetch,push}_refspec_nr.
> This requires us to keep the two arrays in sync and makes adding
> additional refspecs cumbersome and error prone.
I think the two-arrays things is more important, because keeping them in
sync is error-prone. On the other hand, if the array are manipulated by
accessors, that keeps the complexity in one spot.
The lazy parsing is an additional headache on top of that, but that's
orthogonal. Without the lazy parsing, then a single add_refspec() would
be simple and keep everything in sync.
> - And worst of all, it pissed me off while working on
> sg/clone-refspec-from-command-line-config ;)
That I can sympathize with. ;)
> The idea is to parse refspecs gently while reading the configuration:
> this way we won't need to store all refspecs as strings, and won't
> die() on a bogus refspec right away. A bogus refspec, if there's one,
> will be stored in the remote it belongs to, so it will be available
> later when that remote is accessed and can be used in the error
> message.
The "turn a parsed refspec struct back into a refspec string" makes me a
little nervous, just because it needs to be the inverse of the parsing
function. So now we have two new things that need to be kept in sync.
If we forget the "storing it twice" argument, would it make sense to
convert the parallel arrays of items into a single array-of-struct?
I.e.:
struct configured_refspec {
const char *string;
struct refspec refspec;
unsigned parsed:1;
}
I guess that may run into problems where we really need an
array-of-refspec to pass into sub-functions. So going further, could we
just have "struct refspec" store the text form it was parsed from?
-Peff