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

Glen Choo [off-list ref] writes:

remote.c does not work with non-the_repository because it stores its
state as static variables. To support non-the_repository, we can use a
per-repository struct for the remotes subsystem.

Prepare for this change by defining a struct remote_state that holds
the remotes subsystem state and move the static variables of remote.c
into the_repository->remote_state.

That all sounds very sensible, but ...

Missing a blank line between two type decls.

+	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.  But this step is about moving things around and
wrapping the set of existing static file-scope-global variables into
a structure, so such a restructuring would not belong here.  OK.

+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.

Junio C Hamano [off-list ref] writes:

quoted

+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!

quoted

+	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".

quoted

+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!