Re: [PATCH 3/4] Add support for matching full refs in hideRefs

[Eric Sunshine <sunshine@sunshineco.com>]

On Sun, Nov 1, 2015 at 2:34 PM, Lukas Fleischer <lfleischer@lfos.de> wrote:
> In addition to matching stripped refs, one can now add hideRefs patterns
> that the full (unstripped) ref is matched against. To distinguish
> between stripped and full matches, those new patterns must be prefixed
> with a circumflex (^).
>
> This commit also removes support for the undocumented and unintended
> hideRefs settings "have" (suppressing all "have" lines) and
> "capabilities^{}" (suppressing the capabilities line).
>
> Signed-off-by: Lukas Fleischer <lfleischer@lfos.de>
> ---
> diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
> @@ -1198,7 +1200,15 @@ static void reject_updates_to_hidden(struct command *commands)
>         struct command *cmd;
>
>         for (cmd = commands; cmd; cmd = cmd->next) {
> -               if (cmd->error_string || !ref_is_hidden(cmd->ref_name))

Would it make sense to retain the cmd->error_string check here in
order to avoid doing the extra refname_full construction work below?

    if (cmd->error_string)
        continue;

> +               const char *refname = cmd->ref_name;
> +               struct strbuf refname_full_buf = STRBUF_INIT;
> +               const char *refname_full;
> +
> +               strbuf_addf(&refname_full_buf, "%s%s", get_git_namespace(),
> +                               refname);
> +               refname_full = strbuf_detach(&refname_full_buf, NULL);
> +
> +               if (cmd->error_string || !ref_is_hidden(refname, refname_full))
>                         continue;

This is leaking refname_full each time through the loop since it never
gets free()d. If you restructure the code like this, it might be
easier to avoid leaks:

    for (cmd = ...; ... ; ...) {
        if (cmd->error_string)
            continue;
        strbuf_addf(&refname_full, "%s%s", ...);
        if (ref_is_hidden(...)) {
            if (is_null_sha1(...))
                cmd->error_string = "...";
            else
                cmd->error_string = "...";
        }
        strbuf_release(&refname_full);
    }

As a micro-optimization, you could also pre-populate the strbuf with
get_git_namespace() outside the loop and remember the length. Then,
each time through the loop, just append cmd->ref_name, do your
processing, and, at the bottom of the loop, set the strbuf back to the
remembered length. (And, you still need to free the strbuf after the
loop.)

>                 if (is_null_sha1(cmd->new_sha1))
>                         cmd->error_string = "deny deleting a hidden ref";