On Sun, Sep 06, 2015 at 10:24:12AM -0700, Junio C Hamano wrote:
> >> + /* 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.
> >> + 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html