Re: [PATCH] am: match --signoff to the original scripted version

From: Jeff King <peff@peff.net>
Date: Sep 8, 2015, 06:18 UTC
Message-ID: <20150908061836.GD26331@sigill.intra.peff.net>
In-Reply-To: <xmqqd1xvbgw3.fsf@gitster.mtv.corp.google.com>

On Sun, Sep 06, 2015 at 10:24:12AM -0700, Junio C Hamano wrote:

Show 11 quoted lines

> >> +       /* Does it end with our own sign-off? */
> >> +       strbuf_addf(&mine, "\n%s%s\n",
> >> +                   sign_off_header,
> >> +                   fmt_name(getenv("GIT_COMMITTER_NAME"),
> >> +                            getenv("GIT_COMMITTER_EMAIL")));
> >
> > Maybe use git_committer_info() here?
> 
> Perhaps, but I wanted to make sure I am doing the same thing as the
> codepath of sequencer.c::append_signoff(), which the original ended
> up calling.  git_committer_info() does way more than that, no?

Not really. I think git_committer_info(IDENT_STRICT | IDENT_NO_DATE) runs the exact same code, with one exception: we would also set the ident_explicitly_given variables. But nobody in builtin/am.c calls committer_ident_sufficiently_given(). And if they did, I think the change would be an improvement.

Show 7 quoted lines

> >> +       if (mine.len < sb->len &&
> >> +           !strcmp(mine.buf, sb->buf + sb->len - mine.len))
> >
> > Perhaps use ends_with()?
> 
> This caller already _knows_ how long the sb->buf string is; it is
> pointless to have ends_with() run strlen() on it.

That actually goes double. We know sb.len. The ends_with() function is built around strip_suffix(), which both operate on strings. But we do not have ends_with_mem() built around strip_suffix_mem().

But we also know mine.len. Even strip_suffix_mem() assumes the suffix itself is a string. So what you really want is:

  int strip_suffix_mem_mem(const char *buf, size_t *len,
                           const char *suffix, size_t suffix_len);

and then you can trivially build the existing strip_suffix_mem() around it, build strip_suffix() around that, and then build ends_with(), ends_with_mem() and ends_with_mem_mem() around those. And don't forget strbuf_ends_with(), strbuf_ends_with_mem(), and strbuf_ends_with_strbuf()

:) I am only half tongue in cheek. The proliferation of names is tedious (and not appropriate for an -rc regression fix), but I do think the resulting code is a lot more obvious as:

  if (strbuf_ends_with_strbuf(&sb, &mine))
      ...

or even:

  if (ends_with_mem_mem(sb->buf, sb->len, mine.buf, mine.len))
      ...

Of course given that this is run only once per program, and that these _are_ in fact strings, we can probably not bother to optimize it and just accept:

  if (ends_with(sb->buf, mine.buf))
      ...

But if you want to go all-out on optimization, I think you can replace your strcmp with memcmp:

  if (mine.len < sb->len &&
      !memcmp(mine.buf, sb->buf + sb->len - mine.len, mine.len))

(assuming that memcmp is in fact faster than strcmp).

-Peff