git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / code / Atom feed
From: Carlo Arenas <carenas@gmail.com>
To: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
Cc: git@vger.kernel.org
Subject: Re: [PATCH 1/3] grep: make pcre1_tables version agnostic
Date: Sat, 27 Jul 2019 19:50:07 -0700	[thread overview]
Message-ID: <CAPUEspg255TAE-1ozc8CuC=k9oeP_WLD4TrFur5p=XY3s8PX3Q@mail.gmail.com> (raw)
In-Reply-To: <875znn6nvt.fsf@evledraar.gmail.com>

On Sat, Jul 27, 2019 at 4:47 PM Ævar Arnfjörð Bjarmason
<avarab@gmail.com> wrote:
>
> On Sat, Jul 27 2019, Carlo Marcelo Arenas Belón wrote:
>
> > 6d4b5747f0 ("grep: change internal *pcre* variable & function names
> > to be *pcre1*", 2017-05-25), renamed most variables to be PCRE1
> > specific to give space to similarly named variables for PCRE2, but
> > in this case the change wasn't needed as the types were compatible
> > enough (const unsigned char* vs const uint8_t*) to be shared.

reading this again, had to admit there is a fair amount of guessing on
intent, but reading commits and the email discussion couldn't come
up with a better explanation.

is the root cause for the bug different?, could it be that the pcre2 API
was misunderstood? and you expected this pointer will be free together
with the context? (as it is done when a cloned context with tables is
used?)

> Both the v1 and v2 functions return const unsigned char *. I don't know
> where I got the uint8_t from. This makes more sense.

the type in PCRE2 is uint8_t, the documentation has a bug[1]
almost every platform git cares for would have unsigned char = uint8_t
though.

> The point of 6d4b5747f0 was not to only split out those variables we
> couldn't get away with re-using. Then I would have later re-used
> e.g. pcre1_jit_on & pcre2_jit_on as just pcre_jit_on. We could also do
> that now.

IMHO pcre_jit_on makes more sense as a bit, with local variables being
used for the pcre*_config() call with the right type.(uint32_t != int)

> I think doing that & this part of the your changes makes things less
> readable. The two code branches we compile with ifdefs are mutually
> exclusive, so having the variables be unique helps with eyeballing /
> reasoning when changing the code.

I thought that too until the typo[2] in the pcre?_jit_on variable (now
in next) kind of
proved us wrong.  Maybe the names are too similar?

either way, would you rather drop this patch and make a replica variable?
should I rebase to next with Reviewed-By tags so that all other changes
in flight that would conflict could be corrected?

Carlo

[1] https://bugs.exim.org/show_bug.cgi?id=2420
[2] https://public-inbox.org/git/20190722181923.21572-1-dev+git@drbeat.li/

  reply	other threads:[~2019-07-28  2:50 UTC|newest]

Thread overview: 15+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-07-27 20:27 [PATCH 0/3] grep: memory leak in PCRE2 Carlo Marcelo Arenas Belón
2019-07-27 20:27 ` [PATCH 1/3] grep: make pcre1_tables version agnostic Carlo Marcelo Arenas Belón
2019-07-27 23:47   ` Ævar Arnfjörð Bjarmason
2019-07-28  2:50     ` Carlo Arenas [this message]
2019-07-27 20:27 ` [PATCH 2/3] grep: use pcre_tables also for PCRE2 Carlo Marcelo Arenas Belón
2019-07-27 20:27 ` [PATCH 3/3] grep: plug leak of pcre chartables in PCRE2 Carlo Marcelo Arenas Belón
2019-07-27 23:48   ` Ævar Arnfjörð Bjarmason
2019-07-28  1:41     ` Carlo Arenas
2019-07-29 20:34       ` René Scharfe
2019-07-30  0:08         ` Carlo Arenas
2019-07-30 16:52           ` René Scharfe
2019-08-01 17:09 ` [PATCH v2] grep: avoid leak of " Carlo Marcelo Arenas Belón
2019-08-02 16:19   ` Junio C Hamano
2019-08-03 18:50     ` Carlo Arenas
2019-08-05 19:34       ` Junio C Hamano

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='CAPUEspg255TAE-1ozc8CuC=k9oeP_WLD4TrFur5p=XY3s8PX3Q@mail.gmail.com' \
    --to=carenas@gmail.com \
    --cc=avarab@gmail.com \
    --cc=git@vger.kernel.org \
    --subject='Re: [PATCH 1/3] grep: make pcre1_tables version agnostic' \
    /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

Code repositories for project(s) associated with this 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).