"Junio C Hamano" <gitster@xxxxxxxxx> writes:
> > static int git_clone_config(const char *k, const char *v, void *cb)
> > {
> > + if (!strcmp(k, "clone.defaultremotename")) {
> > + if (remote_name != default_remote_name)
> > + free(remote_name);
> > + remote_name = xstrdup(v);
>
> This feels strange. The usual arrangement is
>
> - initialize the variable to NULL (or any value that the code
> can tell that nobody touched it);
>
> - let git_config() callback to update the variable, taking care
> of freeing and strduping as needed. Note that free(NULL) is
> kosher.
>
> - let parse_options() to further update the variable, taking
> care of freeing and strduping as needed.
>
> - finally, if the variable is still NULL, give it its default
> value.
>
> so there is no room for the "if the variable has the value of the
> fallback default, do things differently" logic to be in the
> git_config() callback function.
I agree, it felt pretty awkward while writing it! I was hoping to match
the start-up sequence you'd mentioned in my original attempt [1] but I'm
happy to move the default value assignment down.
> > @@ -1153,10 +1154,20 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
> >
> > /*
> > * re-read config after init_db and write_config to pick up any config
> > - * injected by --template and --config, respectively
> > + * injected by --template and --config, respectively.
> > */
>
> Squash this "oops, I forgot to finish the sentence" to the step the
> mistake was introduced, i.e. "use more conventional..."
How embarrassing, I thought I'd gotten all of those. Will do.
Sean
--
[1] Original attempt at this feature, in separate thread because of the
divergence: https://lore.kernel.org/git/xmqqlfi1utwi.fsf@xxxxxxxxxxxxxxxxxxxxxx/
