git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
To: Johannes Schindelin <Johannes.Schindelin@gmx.de>
Cc: "Junio C Hamano" <gitster@pobox.com>,
	"Git Mailing List" <git@vger.kernel.org>,
	"Nguyễn Thái Ngọc Duy" <pclouds@gmail.com>
Subject: Re: PCRE v2 compile error, was Re: What's cooking in git.git (May 2017, #01; Mon, 1)
Date: Wed, 3 May 2017 17:10:33 +0200	[thread overview]
Message-ID: <CACBZZX6_5krLp93PmsW639-N4f1efUT5rPnN+5im=d9-66=QbQ@mail.gmail.com> (raw)
In-Reply-To: <alpine.DEB.2.20.1705031139090.3480@virtualbox>

[Just replying to you & Duy in the same mail, easier]

On Wed, May 3, 2017 at 11:45 AM, Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
> Hi Ævar,
>
> On Tue, 2 May 2017, Ævar Arnfjörð Bjarmason wrote:
>
>> On Tue, May 2, 2017 at 6:05 PM, Johannes Schindelin
>> <Johannes.Schindelin@gmx.de> wrote:
>>
>> > [... complaint about the abrupt change that makes Git for Windows fail
>> >  to build because pcre2.h is missing ...]
>>
>> I think it's worth it to copy/paste the commit message where I made
>> this change, since we're having this discussion in this thread:
>>
>>     Makefile & configure: make PCRE v2 the default PCRE implementation
>>
>>     Change the USE_LIBPCRE=YesPlease & --with-libpcre flags to the
>>     Makefile & configure script, respectively, to mean use PCRE v2, not
>>     PCRE v1.
>>
>>     The legacy library previously available via those options is still
>>     available on request via USE_LIBPCRE1=YesPlease or
>>     --with-libpcre1. The existing USE_LIBPCRE2=YesPlease & --with-libpcre2
>>     still explicitly ask for v2.
>>
>>     The v2 PCRE is stable & end-user compatible, all this change does is
>>     change the default. Someone building a new git is likely to also have
>>     packaged PCRE v2 sometime in the last 2 years since it was released.
>>     If not they can choose to use the legacy v2 library by making the
>>     trivial s/USE_LIBPCRE/USE_LIBPCRE1/ change, or package up PCRE v2.
>>
>>     New releases of PCRE v2 are already faster than PCRE v1, and not only
>>     is all significant development is happening on v2, but bugs in
>>     reported against v1 have started getting WONTFIX'd asking users to
>>     just upgrade to v2.
>>
>>     So it makes sense to give our downstream distributors a nudge to
>>     switch over to it.
>
> I see where you are coming from.
>
> At this point, I feel that someone should recall into our collective
> memory what happened when we made a change similar in nature that broke
> existing build setups: by requiring REG_STARTEND all of a sudden ("you can
> easily flip the NO_REGEX switch", as if everybody should know about those
> Makefile flags we have).

And as a result grep/log -G got faster by default, and more
importantly since v2.10.1 which includes your 2f8952250a and made a
REG_STARTEND engine a hard requirement nobody using git is
mysteriously going to miss grep results because of some stray \0 in
the string being matched.

I agree that I should drop the patch to make v2 the default from this
series for now. Clearly it's controversial, and can be considered on
its own merits once the supporting code is in. I'll do that in the
next submission, which I'm planning to send after v2.13.0 comes out.

But aside from that, I must say I completely disagree with this line
of argument. I think to the extent that we did anything wrong with
NO_REGEX it's that I should have noticed that we could use
REG_STARTEND after I imported gawk's engine in compat/regex &
submitted a patch like yours back in 2010. Instead we got 6 years of
git releases where

Similarly, if PCRE v2 is less buggy & faster then we're looking at man
days/weeks and possibly months/years saved given how many people use
git v.s. how many are developing it & write build scripts for it.

Leaving user benefits on the table to optimize for reducing the
one-time inconvenience of packagers is the wrong move.

> [...]
On Wed, May 3, 2017 at 1:15 PM, Duy Nguyen <pclouds@gmail.com> wrote:
> On Wed, May 3, 2017 at 4:45 PM, Johannes Schindelin
> <Johannes.Schindelin@gmx.de> wrote:
>>>     So it makes sense to give our downstream distributors a nudge to
>>>     switch over to it.
>
> Some contributor (i.e. me) was not happy with this nudging though. The
> other day I switched to some branch (probably 'pu') and the build
> failed because, guess what, I didn't have pcre2 installed. If I set
> USE_LIBPCRE1 then I lose pcre support when switching to other
> branches. And no, I don't want to install libpcre2, not when I'm
> forced to this way.

Assuming that we'd want to change the default to v2, can you think of
a better way to handle this transition? As pointed out in
<CACBZZX6-qZLEGob6CEwpJ7jtEBG6WLPdHQsO4DsbkNZ8di5mjg@mail.gmail.com> I
can't.

Perhaps the best way would be to migrate this change directly to
master if there's consensus to move to v2 by default, that would avoid
the pain for developers related to switching between concurrent
branches while the change is cooking.

> 188 packages on Gentoo optionally depend on libpcre, 6 packages on
> libpcre2. Chances that a Gentoo user has libpcre2 already are rather
> low.

It sounds like two things are being conflated here, surely "a Gentoo
user", i.e. someone who's not actively contributing to git.git will
just get this via `emerge git` if this change were released, since the
ebuild will depend on libpcre2?

> I'll revisit my installation when the level of libpcre2 support
> grows a bit more than that.

There being few existing reverse dependencies seems like a strange
reason not to run `sudo emerge libpcre2`, but whatever, your dev env,
and we won't be breaking v1 support any time soon.

> You can nudge distributors directly, probably more efficient too.

My assumption in making this change this way is that there's a
decreasing amount of git users who'll get a change if:

* 1. It's made mandatory to compile git (100%)
* 2. It's made a default flag that must be turned off (e.g. our HTTP
support via curl)
* 3. It's something users actually notice being missing, e.g. PCRE
support in general via -P
* 4. It's the default mode of some opt-in flag, e.g. this change where
USE_LIBPCRE=YesPlease gives you v2, not v1
* 5. it's advertised prominently in release notes / the installation
guide / on-list.

Your suggestion is some variant of doing #5, whereas I'm currently
trying to aim for #4 (and in the future I think we should probably go
for #2 for PCRE, but that's another discussion).

You can E-Mail some distributors directly, but there's a big long tail
of git builders who maintain build scripts for their company, e.g. I
maintain one of those. If it's just advertised as #5 I'd probably miss
it unless I was paying close attention, particularly since there's no
#3 since I'd be building with v1 already.

Which is why I think if we're going to nudge distributors to move to
v2 it makes sense to do so as this patch is doing it, i.e. if they're
asking for PCRE already make the build fail until they declare
explicitly whether they'd like v1 or v2.

  parent reply	other threads:[~2017-05-03 15:10 UTC|newest]

Thread overview: 36+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-05-01  5:35 What's cooking in git.git (May 2017, #01; Mon, 1) Junio C Hamano
2017-05-01 14:25 ` Ævar Arnfjörð Bjarmason
2017-05-01 16:21   ` Brandon Williams
2017-05-01 17:44     ` Ævar Arnfjörð Bjarmason
2017-05-01 19:29   ` Jeff King
2017-05-01 23:21   ` Junio C Hamano
2017-05-02  8:23     ` Ævar Arnfjörð Bjarmason
2017-05-02  9:35       ` Junio C Hamano
2017-05-02 12:31         ` Ævar Arnfjörð Bjarmason
2017-05-02 11:11 ` Johannes Schindelin
2017-05-02 12:09 ` PCRE v2 compile error, was " Johannes Schindelin
2017-05-02 12:27   ` Ævar Arnfjörð Bjarmason
2017-05-02 16:05     ` Johannes Schindelin
2017-05-02 18:52       ` Ævar Arnfjörð Bjarmason
2017-05-02 20:51         ` Kevin Daudt
2017-05-02 21:16           ` Ævar Arnfjörð Bjarmason
2017-05-03  9:45         ` Johannes Schindelin
2017-05-03 11:15           ` Duy Nguyen
2017-05-03 15:10           ` Ævar Arnfjörð Bjarmason [this message]
2017-05-04  9:11             ` Johannes Schindelin
2017-05-04 10:24               ` Ævar Arnfjörð Bjarmason
2017-05-04 11:32                 ` Johannes Schindelin
2017-05-04 11:53                   ` Ævar Arnfjörð Bjarmason
2017-05-08  6:30                   ` Ævar Arnfjörð Bjarmason
2017-05-08  7:10                     ` Junio C Hamano
2017-05-08 23:32                       ` brian m. carlson
2017-05-09  0:00                         ` Ævar Arnfjörð Bjarmason
2017-05-09  0:37                           ` brian m. carlson
2017-05-09 10:40                             ` Johannes Schindelin
2017-05-09 11:29                               ` Ævar Arnfjörð Bjarmason
2017-05-09 11:12                             ` Ævar Arnfjörð Bjarmason
2017-05-09 14:22                               ` demerphq
2017-05-09 14:46                                 ` Ævar Arnfjörð Bjarmason
2017-05-02 17:43     ` Brandon Williams
2017-05-02 17:49       ` Ævar Arnfjörð Bjarmason
2017-05-05  1:12 ` Ramsay Jones

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='CACBZZX6_5krLp93PmsW639-N4f1efUT5rPnN+5im=d9-66=QbQ@mail.gmail.com' \
    --to=avarab@gmail.com \
    --cc=Johannes.Schindelin@gmx.de \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=pclouds@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).