Re: [PATCH 3/4] builtin/remote: rework how remote refs get renamed

[Junio C Hamano <gitster@pobox.com>]

Patrick Steinhardt <ps@pks.im> writes:

> But more importantly it is also extremely inperformant. The number of

Is "inperformant" a real word?  "it performs extremely poorly"?

> +static void renamed_refname(struct rename_info *rename,
> +			    const char *refname,
> +			    struct strbuf *out)
> +{
> +	strbuf_reset(out);
> +	strbuf_addstr(out, refname);
> +	strbuf_splice(out, strlen("refs/remotes/"), strlen(rename->old_name),
> +		      rename->new_name, strlen(rename->new_name));
> +}
> +

The function name felt somewhat iffy (sounded as if you are letting
a third-party know that you have renamed a ref), but I cannot come
up with a better alternative X-<.

> +static int rename_one_reflog_entry(const char *old_refname UNUSED,
> +				   struct object_id *old_oid,
> +				   struct object_id *new_oid,
> +				   const char *committer,
> +				   timestamp_t timestamp, int tz,
> +				   const char *msg, void *cb_data)
>  {
>  	struct rename_info *rename = cb_data;

Using a name of a system call for an unrelated variable, even if a
local one in a function scope, makes me nauseous.  Not a new problem
introduced by this change, though.

> +	struct strbuf *identity = rename->buf1;
> +	struct strbuf *name = rename->buf2;
> +	struct strbuf *mail = rename->buf3;
> +	struct ident_split ident;
> +	const char *date;
> +	int error;
> +
> +	if (split_ident_line(&ident, committer, strlen(committer)) < 0)
> +		return -1;
> +
> +	strbuf_reset(name);
> +	strbuf_add(name, ident.name_begin, ident.name_end - ident.name_begin);
> +	strbuf_reset(mail);
> +	strbuf_add(mail, ident.mail_begin, ident.mail_end - ident.mail_begin);
> +
> +	date = show_date(timestamp, tz, DATE_MODE(NORMAL));
> +	strbuf_reset(identity);
> +	strbuf_addstr(identity, fmt_ident(name->buf, mail->buf,
> +					  WANT_BLANK_IDENT, date, 0));

It is somewhat unfortunate that we need to do all of the above only
so that we can recreate the full ident with the given committer with
a timestamp that is given separately.  This probably cannot be helped,
though.  The backend may not be keeping this information as a single
string anyway.

> +static int rename_one_reflog(const char *old_refname,
> +			     const struct object_id *old_oid,
> +			     struct rename_info *rename)
> +{
> +	struct strbuf *message = rename->buf1;

As these temporary strbuf's passed around as part of the rename_info
structure are never released or recreated during the run, this is
safe, but feels dirty, because we saw rename_one_reflog_entry() uses
this exact one for totally different purpose.  Perhaps it would make
it easier to follow if you left "message" uninitialized here, before
refs_for_each_reflog_ent() returns.  And then ...

> +	int error;
> +
> +	if (!refs_reflog_exists(get_main_ref_store(the_repository), old_refname))
> +		return 0;
> +
> +	error = refs_for_each_reflog_ent(get_main_ref_store(the_repository),
> +					 old_refname, rename_one_reflog_entry, rename);
> +	if (error < 0)
> +		return error;
> +
> +	/*
> +	 * Manually write the reflog entry for the now-renamed ref. We cannot
> +	 * rely on `rename_one_ref()` to do this for us as that would screw
> +	 * over order in which reflog entries are being written.
> +	 *
> +	 * Furthermore, we only append the entry in case the reference
> +	 * resolves. Missing references shouldn't have reflogs anyway.
> +	 */

... give the "message" synonym to rename->buf1 here.

> +	strbuf_reset(message);
> +	strbuf_addf(message, "remote: renamed %s to %s", old_refname,
> +		    rename->new_refname->buf);
> +
> +	error = ref_transaction_update_reflog(rename->tx_create, rename->new_refname->buf,
> +					      old_oid, old_oid, git_committer_info(0),
> +					      message->buf, rename->index++, rename->err);
> +	if (error < 0)
> +		return error;
> +
> +	return error;
> +}

> +static int rename_one_ref(const char *old_refname, const char *referent,
> +			  const struct object_id *oid,
> +			  int flags, void *cb_data)
> +{
> +	struct rename_info *rename = cb_data;
> +	struct strbuf *new_referent = rename->buf1;
> +	const char *ptr = old_refname;
> +	int error;
> +
> +	if (!skip_prefix(ptr, "refs/remotes/", &ptr) ||
> +	    !skip_prefix(ptr, rename->old_name, &ptr) ||
> +	    !skip_prefix(ptr, "/", &ptr)) {
> +		error = 0;
> +		goto out;
>  	}
> -	strbuf_release(&buf);
>  
> -	return 0;
> +	renamed_refname(rename, old_refname, rename->new_refname);
> +
> +	if (flags & REF_ISSYMREF) {
> +		/*
> +		 * Stupidly enough `referent` is not pointing to the immediate
> +		 * target of a symref, but it's the recursively resolved value.
> +		 * So symrefs pointing to symrefs would be misresolved, and
> +		 * unborn symrefs don't have any value for the `referent` at all.
> +		 */
> +		referent = refs_resolve_ref_unsafe(get_main_ref_store(the_repository),
> +						   old_refname, RESOLVE_REF_NO_RECURSE,
> +						   NULL, NULL);
> +		renamed_refname(rename, referent, new_referent);
> +		oid = NULL;

Yuck, but this cannot be helped, I guess X-<.

> +	struct rename_info rename = {
> +		.buf1 = &buf,
> +		.buf2 = &buf2,
> +		.buf3 = &buf3,

These can be embedded in the struct, not left as three separate
strbuf instances whose addresses are known to this struct, no?  We'd
need to do strbuf_release() on them at the end anyway, so it would
not be a huge deal, though.

>  	strbuf_release(&buf);
>  	strbuf_release(&buf2);
>  	strbuf_release(&buf3);
> +	strbuf_release(&err);
>  	return result;
>  }