On Fri, Jun 14, 2024 at 10:04:50AM -0700, Junio C Hamano wrote:
> > static void add_pushurl_alias(struct remote_state *remote_state,
> > @@ -79,6 +79,7 @@ static void add_pushurl_alias(struct remote_state *remote_state,
> > char *alias = alias_url(url, &remote_state->rewrites_push);
> > if (alias)
> > add_pushurl(remote, alias);
> > + free(alias);
> > }
>
> OK. I wondered if we want to strdup(url) in my review on the
> previous step, but now we are making the add_url() responsible
> for making a copy, we instead do the opposite, i.e. free alias
> that was allocated for us because we no longer need it.
Yeah. Possibly the two should be squashed. I was trying to make this
patch a little less long/confusing, but maybe breaking things up just
posed new questions. :)
> > @@ -336,7 +338,7 @@ static void read_branches_file(struct remote_state *remote_state,
> > else
> > frag = to_free = repo_default_branch_name(the_repository, 0);
> >
> > - add_url_alias(remote_state, remote, strbuf_detach(&buf, NULL));
> > + add_url_alias(remote_state, remote, buf.buf);
>
> It is curious that you delay ...
>
> > @@ -347,6 +349,7 @@ static void read_branches_file(struct remote_state *remote_state,
> > refspec_appendf(&remote->push, "HEAD:refs/heads/%s", frag);
> > remote->fetch_tags = 1; /* always auto-follow */
> >
> > + strbuf_release(&buf);
> > free(to_free);
> > }
>
> ... strbuf_release() of the buf to the very end of the function.
>
> In the original, buf became invalid by doing strbuf_detach(), so we
> could do strbuf_release() immediately after add_url_alias() returns
> to us if we wanted to.
Right. I had originally written it that way, since that would be the
mechanical conversion. But since there was already cleanup at the bottom
of the function, it felt more natural to shuffle it there. Which is
correct as long as there are no other references to buf nor early
returns. You can't see that from the context, but it is true in this
case.
Bumping to --inter-hunk-context=4 does it a little more obvious.
> > @@ -431,15 +434,13 @@ static int handle_config(const char *key, const char *value,
> > else if (!strcmp(subkey, "prunetags"))
> > remote->prune_tags = git_config_bool(key, value);
> > else if (!strcmp(subkey, "url")) {
> > - char *v;
> > - if (git_config_string(&v, key, value))
> > - return -1;
> > - add_url(remote, v);
> > + if (!value)
> > + return config_error_nonbool(key);
> > + add_url(remote, value);
>
> OK. config_string() does (1) check for "I exist hence I am true"
> boolean, and (2) give us a duplicate of the value. We do not want
> the latter, so we do the former ourselves here. The same story
> repeats for pushurl below (ellided).
Yep. If we followed through on the "bool means reset" approach, then these
extra NULL checks would go away in favor of one in add_url(). I didn't
pursue that here. And if we do, I think we should do it in all the other
spots, too, for consistency.
-Peff
