Re: [PATCH v2 1/3] remote: move static variables into per-repository struct

From: Glen Choo <chooglen@google.com>
Date: Oct 14, 2021, 17:25 UTC
Message-ID: <kl6lk0if1pt3.fsf@chooglen-macbookpro.roam.corp.google.com>
In-Reply-To: <xmqqlf2wfzfz.fsf@gitster.g>

Junio C Hamano <gitster@pobox.com> writes:

Show 10 quoted lines

>> +struct rewrite {
>> +	const char *base;
>> +	size_t baselen;
>> +	struct counted_string *instead_of;
>> +	int instead_of_nr;
>> +	int instead_of_alloc;
>> +};
>> +struct rewrites {
>
> Missing a blank line between two type decls.

Thanks!

Show 10 quoted lines

>
>> +	struct rewrite **rewrite;
>> +	int rewrite_alloc;
>> +	int rewrite_nr;
>> +};
>
> It is a bit sad that we still have to keep "struct rewrites"; if we
> see how .remotes and .branches are handled, we probably should have
> left the <array, alloc, nr> 3-tuple for "struct rewrite" without an
> extra layer.

I considered doing this, but I found it non-trivial because some functions use "struct rewrites" without consideration for whether it is remote_state->rewrites or remote_state->rewrites_push i.e. the "struct rewrites" abstraction is doing its job.

> ...such a restructuring would not belong here.  OK.

By "here", I assume you mean "the restructuring doesn't belong in this topic" vs "the restructuring belongs in this topic but not this patch".

Show 21 quoted lines

>> +struct remote_state {
>> +	int config_loaded;
>> +
>> +	struct remote **remotes;
>> +	int remotes_alloc;
>> +	int remotes_nr;
>> +	struct hashmap remotes_hash;
>> +
>> +	struct branch **branches;
>> +	int branches_alloc;
>> +	int branches_nr;
>> +
>> +	struct branch *current_branch;
>> +	const char *pushremote_name;
>> +
>> +	struct rewrites rewrites;
>> +	struct rewrites rewrites_push;
>> +};
>> +void remote_state_clear(struct remote_state *remote_state);
>
> Missing a blank line after "struct" decl.

Thanks!