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

[Lukas Fleischer <lfleischer@lfos.de>]

On Sun, 01 Nov 2015 at 21:42:37, Eric Sunshine wrote:
> 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;
> 
> [...]
> 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.)
> [...]

Sounds good to me. I changed the code to initialize the strbuf only
once, save the prefix length and reset using strbuf_setlen() in every
loop iteration. Thanks!