git@vger.kernel.org mailing list mirror (one of many)
 help / Atom feed
From: Duy Nguyen <pclouds@gmail.com>
To: "brian m. carlson" <sandals@crustytoothpaste.net>, Duy Nguyen <pclouds@gmail.com>, Git Mailing List <git@vger.kernel.org>, Jeff King <peff@peff.net>
Subject: Re: [PATCH 10/20] sha1_name: convert struct disambiguate_state to object_id
Date: Tue, 21 Mar 2017 17:17:05 +0700
Message-ID: <CACsJy8D_9s7e4MuMVuWr2GJ4Y5FF_5bWSUCOxEc50NkrqdB77A@mail.gmail.com> (raw)
In-Reply-To: <20170320223250.vyzqyqejxrr4dfp4@genre.crustytoothpaste.net>

On Tue, Mar 21, 2017 at 5:32 AM, brian m. carlson
<sandals@crustytoothpaste.net> wrote:
> On Mon, Mar 20, 2017 at 08:07:09PM +0700, Duy Nguyen wrote:
>> On Sun, Mar 19, 2017 at 4:19 AM, brian m. carlson
>> <sandals@crustytoothpaste.net> wrote:
>> > @@ -332,7 +332,7 @@ static int init_object_disambiguation(const char *name, int len,
>> >                 ds->hex_pfx[i] = c;
>> >                 if (!(i & 1))
>> >                         val <<= 4;
>> > -               ds->bin_pfx[i >> 1] |= val;
>> > +               ds->bin_pfx.hash[i >> 1] |= val;
>>
>> The indexing makes me a bit nervous, especially since diff context
>> here is too narrow to see. It would be nice if this code (at the
>> beginning of init_object_disambiguation) is converted here too
>>
>>         if (len < MINIMUM_ABBREV || len > GIT_SHA1_HEXSZ)
>>                 return -1;
>
> Well, I think that's the way I would have written that text at the top
> of the function.  I expect that we'll end up turning GIT_SHA1_HEXSZ into
> a global named something like current_hash_len via global
> search-and-replace, so it will always be the right length.
>
> The indexing should be safe because len is guaranteed to be sufficiently
> small, and I feel like it we would have seen it break by now if it had
> had an overflow.  i will always be in the range [0, 40) (for SHA-1), so
> i >> 1 should always be in [0, 20).
>
> Am I understanding you correctly and if so, does that assuage your
> concerns, or did you mean something else?

There's a disconnect between object_id (which goes with GIT_MAX_RAWSZ)
and the code here which still checks upper bound as GIT_SHA1_HEXSZ.
But I guess eventually GIT_SHA1_HEXSZ will be undefined and gone. This
is just a temporary state. So forget about my paranoid comment. All
good.
-- 
Duy

  reply index

Thread overview: 31+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-03-18 21:19 [PATCH 00/20] object_id part 7 brian m. carlson
2017-03-18 21:19 ` [PATCH 01/20] Define new hash-size constants for allocating memory brian m. carlson
2017-03-18 21:19 ` [PATCH 02/20] Convert GIT_SHA1_HEXSZ used for allocation to GIT_MAX_HEXSZ brian m. carlson
2017-03-18 21:19 ` [PATCH 03/20] Convert GIT_SHA1_RAWSZ used for allocation to GIT_MAX_RAWSZ brian m. carlson
2017-03-18 21:19 ` [PATCH 04/20] builtin/diff: convert to struct object_id brian m. carlson
2017-03-18 21:19 ` [PATCH 05/20] builtin/pull: convert portions " brian m. carlson
2017-03-18 21:19 ` [PATCH 06/20] builtin/receive-pack: " brian m. carlson
2017-03-20 12:56   ` Duy Nguyen
2017-03-20 23:17     ` brian m. carlson
2017-03-21 10:26       ` Duy Nguyen
2017-03-18 21:19 ` [PATCH 07/20] fsck: convert init_skiplist " brian m. carlson
2017-03-18 21:19 ` [PATCH 08/20] parse-options-cb: convert sha1_array_append caller " brian m. carlson
2017-03-18 21:19 ` [PATCH 09/20] test-sha1-array: convert most code " brian m. carlson
2017-03-18 21:19 ` [PATCH 10/20] sha1_name: convert struct disambiguate_state to object_id brian m. carlson
2017-03-20 13:07   ` Duy Nguyen
2017-03-20 22:32     ` brian m. carlson
2017-03-21 10:17       ` Duy Nguyen [this message]
2017-03-18 21:19 ` [PATCH 11/20] sha1_name: convert disambiguate_hint_fn to take object_id brian m. carlson
2017-03-18 21:19 ` [PATCH 12/20] submodule: convert check_for_new_submodule_commits to object_id brian m. carlson
2017-03-18 21:19 ` [PATCH 13/20] builtin/pull: convert to struct object_id brian m. carlson
2017-03-18 21:19 ` [PATCH 14/20] sha1-array: convert internal storage for struct sha1_array to object_id brian m. carlson
2017-03-18 21:19 ` [PATCH 15/20] Make sha1_array_append take a struct object_id * brian m. carlson
2017-03-18 21:19 ` [PATCH 16/20] Convert remaining callers of sha1_array_lookup to object_id brian m. carlson
2017-03-18 21:19 ` [PATCH 17/20] Convert sha1_array_lookup to take struct object_id brian m. carlson
2017-03-18 21:19 ` [PATCH 18/20] Convert sha1_array_for_each_unique and for_each_abbrev to object_id brian m. carlson
2017-03-18 21:19 ` [PATCH 19/20] Rename sha1_array to oid_array brian m. carlson
2017-03-20 12:25   ` Duy Nguyen
2017-03-21  0:54     ` brian m. carlson
2017-03-18 21:19 ` [PATCH 20/20] Documentation: update and rename api-sha1-array.txt brian m. carlson
2017-03-20 13:14 ` [PATCH 00/20] object_id part 7 Duy Nguyen
2017-03-21  1:16   ` brian m. carlson

Reply instructions:

You may reply publically 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=CACsJy8D_9s7e4MuMVuWr2GJ4Y5FF_5bWSUCOxEc50NkrqdB77A@mail.gmail.com \
    --to=pclouds@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=peff@peff.net \
    --cc=sandals@crustytoothpaste.net \
    /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

git@vger.kernel.org mailing list mirror (one of many)

Archives are clonable:
	git clone --mirror https://public-inbox.org/git
	git clone --mirror http://ou63pmih66umazou.onion/git
	git clone --mirror http://czquwvybam4bgbro.onion/git
	git clone --mirror http://hjrcffqmbrq6wope.onion/git

Newsgroups are available over NNTP:
	nntp://news.public-inbox.org/inbox.comp.version-control.git
	nntp://ou63pmih66umazou.onion/inbox.comp.version-control.git
	nntp://czquwvybam4bgbro.onion/inbox.comp.version-control.git
	nntp://hjrcffqmbrq6wope.onion/inbox.comp.version-control.git
	nntp://news.gmane.org/gmane.comp.version-control.git

 note: .onion URLs require Tor: https://www.torproject.org/
       or Tor2web: https://www.tor2web.org/

AGPL code for this site: git clone https://public-inbox.org/ public-inbox