From: Jeff King <peff@peff.net>
To: Junio C Hamano <gitster@pobox.com>
Cc: David Kastrup <dak@gnu.org>,
Quint Guvernator <quintus.public@gmail.com>,
git@vger.kernel.org
Subject: Re: [PATCH] general style: replaces memcmp() with proper starts_with()
Date: Thu, 13 Mar 2014 13:55:15 -0400 [thread overview]
Message-ID: <20140313175515.GA18897@sigill.intra.peff.net> (raw)
In-Reply-To: <xmqq4n32j8i7.fsf@gitster.dls.corp.google.com>
On Thu, Mar 13, 2014 at 10:47:28AM -0700, Junio C Hamano wrote:
> >> --- a/builtin/cat-file.c
> >> +++ b/builtin/cat-file.c
> >> @@ -82,7 +82,7 @@ static int cat_one_file(int opt, const char *exp_type, const char *obj_name)
> >> enum object_type type;
> >> unsigned long size;
> >> char *buffer = read_sha1_file(sha1, &type, &size);
> >> - if (memcmp(buffer, "object ", 7) ||
> >> + if (!starts_with(buffer, "object ") ||
> >
> > [...]
> >
> >> The original hunks show that the code knows and relies on magic
> >> numbers 7 and 8 very clearly and there are rooms for improvement.
> >
> > Like: what if the file is empty?
>
> Yes.
I think this one is actually OK. The result of read_sha1_file is
NUL-terminated, and we know that starts_with reads byte-by-byte (the
prior memcmp is wrong, but only if you care about accessing bytes past
the end of the NUL).
That whole piece of code seems silly, though, IMHO. It should be using
parse_tag or peel_to_type.
-Peff
prev parent reply other threads:[~2014-03-13 17:55 UTC|newest]
Thread overview: 22+ messages / expand[flat|nested] mbox.gz Atom feed top
2014-03-12 14:43 [PATCH] general style: replaces memcmp() with proper starts_with() Quint Guvernator
2014-03-12 17:56 ` Jeff King
2014-03-12 19:39 ` Junio C Hamano
2014-03-12 19:49 ` Jeff King
2014-03-12 20:08 ` Junio C Hamano
2014-03-12 21:14 ` Jeff King
2014-03-12 21:39 ` Jeff King
2014-03-12 22:06 ` Jeff King
2014-03-12 22:38 ` Junio C Hamano
2014-03-13 3:33 ` Quint Guvernator
2014-03-13 17:46 ` Junio C Hamano
2014-03-14 4:57 ` Jeff King
2014-03-14 14:51 ` Quint Guvernator
2014-03-14 16:56 ` Junio C Hamano
2014-03-12 20:51 ` René Scharfe
2014-03-12 21:16 ` David Kastrup
2014-03-12 21:45 ` René Scharfe
2014-03-12 20:52 ` David Kastrup
2014-03-12 22:37 ` Junio C Hamano
2014-03-13 6:27 ` David Kastrup
2014-03-13 17:47 ` Junio C Hamano
2014-03-13 17:55 ` Jeff King [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20140313175515.GA18897@sigill.intra.peff.net \
--to=peff@peff.net \
--cc=dak@gnu.org \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
--cc=quintus.public@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).