{"thread":{"id":"48330","subject":"[PATCH 0/3] Some add-on patches on top of dj/runtime-prefix","startedAt":"2018-04-20T08:03:47Z","lastAt":"2018-04-25T07:46:23Z","messageCount":21,"participants":["Johannes Schindelin","Ævar Arnfjörð Bjarmason","Martin Ågren","Philip Oakley","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"345193","messageId":"cover.1524211375.git.johannes.schindelin@gmx.de","threadId":"48330","inReplyTo":null,"subject":"[PATCH 0/3] Some add-on patches on top of dj/runtime-prefix","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2018-04-20T08:03:14Z","receivedAt":"2018-04-20T08:03:47Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"We carried a slightly different version of the git_setup_gettext() patch\n(which took care of releasing the buffer allocated by system_path()),\nand we carry two more patches that touch the same area, so now that\ndj/runtime-prefix hit `next`, it seems a good time to contribute those,\ntoo.\n\n\nJohannes Schindelin (2):\n gettext: avoid initialization if the locale dir is not present\n git_setup_gettext: plug memory leak\n\nPhilip Oakley (1):\n Avoid multiple PREFIX definitions\n\n Makefile | 2 +-\n exec-cmd.c | 4 ++--\n gettext.c | 10 +++++++++-\n sideband.c | 10 +++++-----\n 4 files changed, 17 insertions(+), 9 deletions(-)\n\n\nbase-commit: 8a3641ab3abcf492e9443f88d82a7a22fa8b4816\nPublished-As: https://github.com/dscho/git/releases/tag/runtime-prefix-addons-v1\nFetch-It-Via: git fetch https://github.com/dscho/git runtime-prefix-addons-v1\n-- \n2.17.0.windows.1.15.gaa56ade3205\n\n"},{"id":"345194","messageId":"6be8678b7d6b4d9116ea4666a0b28ff4dd938690.1524211375.git.johannes.schindelin@gmx.de","threadId":"48330","inReplyTo":"cover.1524211375.git.johannes.schindelin@gmx.de","subject":"[PATCH 1/3] gettext: avoid initialization if the locale dir is not present","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2018-04-20T08:03:34Z","receivedAt":"2018-04-20T08:04:06Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"The runtime of a simple `git.exe version` call on Windows is currently\ndominated by the gettext setup, adding a whopping ~150ms to the ~210ms\ntotal.\n\nGiven that this cost is added to each and every git.exe invocation goes\nthrough common-main's invocation of git_setup_gettext(), and given that\nscripts have to call git.exe dozens, if not hundreds, of times, this is\na substantial performance penalty.\n\nThis is particularly pointless when considering that Git for Windows\nships without localization (to keep the installer's size to a bearable\n~34MB): all that time setting up gettext is for naught.\n\nSo let's be smart about it and skip setting up gettext if the locale\ndirectory is not even present.\n\nSigned-off-by: Johannes Schindelin \n---\n gettext.c | 3 +++\n 1 file changed, 3 insertions(+)\n\ndiff --git a/gettext.c b/gettext.c\nindex baba28343c3..701355d66e7 100644\n--- a/gettext.c\n+++ b/gettext.c\n@@ -163,6 +163,9 @@ void git_setup_gettext(void)\n \tif (!podir)\n \t\tpodir = system_path(GIT_LOCALE_PATH);\n \n+\tif (!is_directory(podir))\n+\t\treturn;\n+\n \tbindtextdomain(\"git\", podir);\n \tsetlocale(LC_MESSAGES, \"\");\n \tsetlocale(LC_TIME, \"\");\n-- \n2.17.0.windows.1.15.gaa56ade3205\n\n\n"},{"id":"345195","messageId":"c9502718c0ea1801a1123f976f4f8ca5332372fb.1524211375.git.johannes.schindelin@gmx.de","threadId":"48330","inReplyTo":"cover.1524211375.git.johannes.schindelin@gmx.de","subject":"[PATCH 2/3] git_setup_gettext: plug memory leak","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2018-04-20T08:03:53Z","receivedAt":"2018-04-20T08:04:23Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"The system_path() function returns a freshly-allocated string. We need\nto release it.\n\nSigned-off-by: Johannes Schindelin \n---\n gettext.c | 9 +++++++--\n 1 file changed, 7 insertions(+), 2 deletions(-)\n\ndiff --git a/gettext.c b/gettext.c\nindex 701355d66e7..7272771c8e4 100644\n--- a/gettext.c\n+++ b/gettext.c\n@@ -159,18 +159,23 @@ static void init_gettext_charset(const char *domain)\n void git_setup_gettext(void)\n {\n \tconst char *podir = getenv(GIT_TEXT_DOMAIN_DIR_ENVIRONMENT);\n+\tchar *p = NULL;\n \n \tif (!podir)\n-\t\tpodir = system_path(GIT_LOCALE_PATH);\n+\t\tpodir = p = system_path(GIT_LOCALE_PATH);\n \n-\tif (!is_directory(podir))\n+\tif (!is_directory(podir)) {\n+\t\tfree(p);\n \t\treturn;\n+\t}\n \n \tbindtextdomain(\"git\", podir);\n \tsetlocale(LC_MESSAGES, \"\");\n \tsetlocale(LC_TIME, \"\");\n \tinit_gettext_charset(\"git\");\n \ttextdomain(\"git\");\n+\n+\tfree(p);\n }\n \n /* return the number of columns of string 's' in current locale */\n-- \n2.17.0.windows.1.15.gaa56ade3205\n\n\n"},{"id":"345196","messageId":"d134afdd22c9cdfd763213b37efac4ef0399a374.1524211375.git.johannes.schindelin@gmx.de","threadId":"48330","inReplyTo":"cover.1524211375.git.johannes.schindelin@gmx.de","subject":"[PATCH 3/3] Avoid multiple PREFIX definitions","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2018-04-20T08:04:12Z","receivedAt":"2018-04-20T08:04:45Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Philip Oakley \n\nThe short and sweet PREFIX can be confused when used in many places.\n\nRename both usages to better describe their purpose. EXEC_CMD_PREFIX is\nused in full to disambiguate it from the nearby GIT_EXEC_PATH.\n\nThe PREFIX in sideband.c, while nominally independant of the exec_cmd\nPREFIX, does reside within libgit[1], so the definitions would clash\nwhen taken together with a PREFIX given on the command line for use by\nexec_cmd.c.\n\nNoticed when compiling Git for Windows using MSVC/Visual Studio [1] which\nreports the conflict beteeen the command line definition and the\ndefinition in sideband.c within the libgit project.\n\n[1] the libgit functions are brought into a single sub-project\nwithin the Visual Studio construction script provided in contrib,\nand hence uses a single command for both exec_cmd.c and sideband.c.\n\nSigned-off-by: Philip Oakley \nSigned-off-by: Johannes Schindelin \n---\n Makefile | 2 +-\n exec-cmd.c | 4 ++--\n sideband.c | 10 +++++-----\n 3 files changed, 8 insertions(+), 8 deletions(-)\n\ndiff --git a/Makefile b/Makefile\nindex 111e93d3bea..49cec672242 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -2271,7 +2271,7 @@ exec-cmd.sp exec-cmd.s exec-cmd.o: EXTRA_CPPFLAGS = \\\n \t'-DGIT_EXEC_PATH=\"$(gitexecdir_SQ)\"' \\\n \t'-DGIT_LOCALE_PATH=\"$(localedir_relative_SQ)\"' \\\n \t'-DBINDIR=\"$(bindir_relative_SQ)\"' \\\n-\t'-DPREFIX=\"$(prefix_SQ)\"'\n+\t'-DFALLBACK_RUNTIME_PREFIX=\"$(prefix_SQ)\"'\n \n builtin/init-db.sp builtin/init-db.s builtin/init-db.o: GIT-PREFIX\n builtin/init-db.sp builtin/init-db.s builtin/init-db.o: EXTRA_CPPFLAGS = \\\ndiff --git a/exec-cmd.c b/exec-cmd.c\nindex 3b0a039083a..02d31ee8971 100644\n--- a/exec-cmd.c\n+++ b/exec-cmd.c\n@@ -48,7 +48,7 @@ static const char *system_prefix(void)\n \t !(prefix = strip_path_suffix(executable_dirname, GIT_EXEC_PATH)) &&\n \t !(prefix = strip_path_suffix(executable_dirname, BINDIR)) &&\n \t !(prefix = strip_path_suffix(executable_dirname, \"git\"))) {\n-\t\tprefix = PREFIX;\n+\t\tprefix = FALLBACK_RUNTIME_PREFIX;\n \t\ttrace_printf(\"RUNTIME_PREFIX requested, \"\n \t\t\t\t\"but prefix computation failed. \"\n \t\t\t\t\"Using static fallback '%s'.\\n\", prefix);\n@@ -243,7 +243,7 @@ void git_resolve_executable_dir(const char *argv0)\n */\n static const char *system_prefix(void)\n {\n-\treturn PREFIX;\n+\treturn FALLBACK_RUNTIME_PREFIX;\n }\n \n /*\ndiff --git a/sideband.c b/sideband.c\nindex 6d7f943e438..325bf0e974a 100644\n--- a/sideband.c\n+++ b/sideband.c\n@@ -13,7 +13,7 @@\n * the remote died unexpectedly. A flush() concludes the stream.\n */\n \n-#define PREFIX \"remote: \"\n+#define DISPLAY_PREFIX \"remote: \"\n \n #define ANSI_SUFFIX \"\\033[K\"\n #define DUMB_SUFFIX \" \"\n@@ -49,7 +49,7 @@ int recv_sideband(const char *me, int in_stream, int out)\n \t\tswitch (band) {\n \t\tcase 3:\n \t\t\tstrbuf_addf(&outbuf, \"%s%s%s\", outbuf.len ? \"\\n\" : \"\",\n-\t\t\t\t PREFIX, buf + 1);\n+\t\t\t\t DISPLAY_PREFIX, buf + 1);\n \t\t\tretval = SIDEBAND_REMOTE_ERROR;\n \t\t\tbreak;\n \t\tcase 2:\n@@ -67,7 +67,7 @@ int recv_sideband(const char *me, int in_stream, int out)\n \t\t\t\tint linelen = brk - b;\n \n \t\t\t\tif (!outbuf.len)\n-\t\t\t\t\tstrbuf_addstr(&outbuf, PREFIX);\n+\t\t\t\t\tstrbuf_addstr(&outbuf, DISPLAY_PREFIX);\n \t\t\t\tif (linelen > 0) {\n \t\t\t\t\tstrbuf_addf(&outbuf, \"%.*s%s%c\",\n \t\t\t\t\t\t linelen, b, suffix, *brk);\n@@ -81,8 +81,8 @@ int recv_sideband(const char *me, int in_stream, int out)\n \t\t\t}\n \n \t\t\tif (*b)\n-\t\t\t\tstrbuf_addf(&outbuf, \"%s%s\",\n-\t\t\t\t\t outbuf.len ? \"\" : PREFIX, b);\n+\t\t\t\tstrbuf_addf(&outbuf, \"%s%s\", outbuf.len ?\n+\t\t\t\t\t \"\" : DISPLAY_PREFIX, b);\n \t\t\tbreak;\n \t\tcase 1:\n \t\t\twrite_or_die(out, buf + 1, len);\n-- \n2.17.0.windows.1.15.gaa56ade3205\n"},{"id":"345204","messageId":"87o9iedwqn.fsf@evledraar.gmail.com","threadId":"48330","inReplyTo":"6be8678b7d6b4d9116ea4666a0b28ff4dd938690.1524211375.git.johannes.schindelin@gmx.de","subject":"Re: [PATCH 1/3] gettext: avoid initialization if the locale dir is not present","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-04-20T09:59:28Z","receivedAt":"2018-04-20T09:59:34Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Fri, Apr 20 2018, Johannes Schindelin wrote:\n\n> The runtime of a simple `git.exe version` call on Windows is currently\n> dominated by the gettext setup, adding a whopping ~150ms to the ~210ms\n> total.\n>\n> Given that this cost is added to each and every git.exe invocation goes\n> through common-main's invocation of git_setup_gettext(), and given that\n> scripts have to call git.exe dozens, if not hundreds, of times, this is\n> a substantial performance penalty.\n>\n> This is particularly pointless when considering that Git for Windows\n> ships without localization (to keep the installer's size to a bearable\n> ~34MB): all that time setting up gettext is for naught.\n>\n> So let's be smart about it and skip setting up gettext if the locale\n> directory is not even present.\n>\n> Signed-off-by: Johannes Schindelin \n> ---\n> gettext.c | 3 +++\n> 1 file changed, 3 insertions(+)\n>\n> diff --git a/gettext.c b/gettext.c\n> index baba28343c3..701355d66e7 100644\n> --- a/gettext.c\n> +++ b/gettext.c\n> @@ -163,6 +163,9 @@ void git_setup_gettext(void)\n> \tif (!podir)\n> \t\tpodir = system_path(GIT_LOCALE_PATH);\n>\n> +\tif (!is_directory(podir))\n> +\t\treturn;\n> +\n> \tbindtextdomain(\"git\", podir);\n> \tsetlocale(LC_MESSAGES, \"\");\n> \tsetlocale(LC_TIME, \"\");\n\nI wish we could fix the root cause and just make gettext faster (or ship\nwith a small shimmy of our own), but leaving that aside, I don't see how\nthis patch makes sense.\n\nIf you don't ship git for windows with gettext or a podir, then compile\nwith NO_GETTEXT, then we won't even compile this function you're\npatching here. See line 30 and beyond of gettext.h.\n\nAre you instead compiling git for windows with gettext support with an\noptional add-on package for i18n, so then this podir conditional would\npass?\n\nIn any case, if this is actually the desired behavior I think it's worth\nclearly explaining the interaction with NO_GETTEXT in the commit\nmessage, and if you *actually* don't want NO_GETTEXT I think it's useful\nto guard this with something like MAYBE_GETTEXT on top, so this code\ndoesn't unintentionally hide installation errors on other\nplatforms. I.e. something like:\n\n\tint have_podir = is_directory(podir);\n\tif (!have_podir)\n#ifdef MAYBE_GETTEXT\n\t\treturn;\n#else\n\t\tBUG(\"Broken installation, can't find '%s'!\", podir);\n#endif\n"},{"id":"345208","messageId":"CAN0heSpda1ZnFXgoCEgxGdBk-JYUSPAV0A=mQYMtOq7w8x+5=w@mail.gmail.com","threadId":"48330","inReplyTo":"87o9iedwqn.fsf@evledraar.gmail.com","subject":"Re: [PATCH 1/3] gettext: avoid initialization if the locale dir is not present","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2018-04-20T10:54:35Z","receivedAt":"2018-04-20T10:54:40Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"On 20 April 2018 at 11:59, Ævar Arnfjörð Bjarmason wrote:\n>\n> On Fri, Apr 20 2018, Johannes Schindelin wrote:\n>\n>> The runtime of a simple `git.exe version` call on Windows is currently\n>> dominated by the gettext setup, adding a whopping ~150ms to the ~210ms\n>> total.\n>>\n>> Given that this cost is added to each and every git.exe invocation goes\n>> through common-main's invocation of git_setup_gettext(), and given that\n>> scripts have to call git.exe dozens, if not hundreds, of times, this is\n>> a substantial performance penalty.\n>>\n>> This is particularly pointless when considering that Git for Windows\n>> ships without localization (to keep the installer's size to a bearable\n>> ~34MB): all that time setting up gettext is for naught.\n>>\n>> So let's be smart about it and skip setting up gettext if the locale\n>> directory is not even present.\n\n> If you don't ship git for windows with gettext or a podir, then compile\n> with NO_GETTEXT, then we won't even compile this function you're\n> patching here. See line 30 and beyond of gettext.h.\n>\n> Are you instead compiling git for windows with gettext support with an\n> optional add-on package for i18n, so then this podir conditional would\n> pass?\n>\n> In any case, if this is actually the desired behavior I think it's worth\n> clearly explaining the interaction with NO_GETTEXT in the commit\n> message, and if you *actually* don't want NO_GETTEXT I think it's useful\n> to guard this with something like MAYBE_GETTEXT on top, so this code\n> doesn't unintentionally hide installation errors on other\n> platforms. I.e. something like:\n>\n> int have_podir = is_directory(podir);\n> if (!have_podir)\n> #ifdef MAYBE_GETTEXT\n> return;\n> #else\n> BUG(\"Broken installation, can't find '%s'!\", podir);\n> #endif\n\nIs it fair to say that such a broken installation would function more or\nless well before and after Dscho's patch, and that your approach would\nrender such an installation quite useless? Do we have some other similar\ncases where we go BUG(\"I could not find a resource. I could manage\nfairly well without it, but you seemed to want it when you installed\nme.\")? I wonder what other well-respected software do if they can't find\ntheir translations.\n\nI suppose the logic could be the other way round, i.e., with a flag\nREALLY_REQUIRE_GETTEXT_AT_RUNTIME. But I also wonder if a user who does\nnot notice that the installation is broken without us screaming BUG in\ntheir face, really minds the \"brokenness\" that much.\n\nDisclaimer: I do not use translated Git, nor other non-English-language\nsoftware.\n\nMartin\n"},{"id":"345209","messageId":"87lgdidt30.fsf@evledraar.gmail.com","threadId":"48330","inReplyTo":"CAN0heSpda1ZnFXgoCEgxGdBk-JYUSPAV0A=mQYMtOq7w8x+5=w@mail.gmail.com","subject":"Re: [PATCH 1/3] gettext: avoid initialization if the locale dir is not present","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-04-20T11:18:27Z","receivedAt":"2018-04-20T11:18:33Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Fri, Apr 20 2018, Martin Ågren wrote:\n\n> On 20 April 2018 at 11:59, Ævar Arnfjörð Bjarmason wrote:\n>>\n>> On Fri, Apr 20 2018, Johannes Schindelin wrote:\n>>\n>>> The runtime of a simple `git.exe version` call on Windows is currently\n>>> dominated by the gettext setup, adding a whopping ~150ms to the ~210ms\n>>> total.\n>>>\n>>> Given that this cost is added to each and every git.exe invocation goes\n>>> through common-main's invocation of git_setup_gettext(), and given that\n>>> scripts have to call git.exe dozens, if not hundreds, of times, this is\n>>> a substantial performance penalty.\n>>>\n>>> This is particularly pointless when considering that Git for Windows\n>>> ships without localization (to keep the installer's size to a bearable\n>>> ~34MB): all that time setting up gettext is for naught.\n>>>\n>>> So let's be smart about it and skip setting up gettext if the locale\n>>> directory is not even present.\n>\n>> If you don't ship git for windows with gettext or a podir, then compile\n>> with NO_GETTEXT, then we won't even compile this function you're\n>> patching here. See line 30 and beyond of gettext.h.\n>>\n>> Are you instead compiling git for windows with gettext support with an\n>> optional add-on package for i18n, so then this podir conditional would\n>> pass?\n>>\n>> In any case, if this is actually the desired behavior I think it's worth\n>> clearly explaining the interaction with NO_GETTEXT in the commit\n>> message, and if you *actually* don't want NO_GETTEXT I think it's useful\n>> to guard this with something like MAYBE_GETTEXT on top, so this code\n>> doesn't unintentionally hide installation errors on other\n>> platforms. I.e. something like:\n>>\n>> int have_podir = is_directory(podir);\n>> if (!have_podir)\n>> #ifdef MAYBE_GETTEXT\n>> return;\n>> #else\n>> BUG(\"Broken installation, can't find '%s'!\", podir);\n>> #endif\n>\n> Is it fair to say that such a broken installation would function more or\n> less well before and after Dscho's patch, and that your approach would\n> render such an installation quite useless?\n\nYes my thown out there for the purposes of discussion suggestion may\nbreak things for Dscho, or not. I'm just saying that we should document\n*what* the use-case is.\n\nBecause it's not just important to massage the code so it works now, but\nto document what the use-case is, so someone down the line who things\n\"oh why are we doing that\" has a clear answer.\n\n> Do we have some other similar\n> cases where we go BUG(\"I could not find a resource. I could manage\n> fairly well without it, but you seemed to want it when you installed\n> me.\")? I wonder what other well-respected software do if they can't find\n> their translations.\n\nE.g. my recent 1aca69c019 (\"perl Git::LoadCPAN: emit better errors under\nNO_PERL_CPAN_FALLBACKS\", 2018-03-03) does similar things, we *could*\ncarry on in certain cases instead of dying there (or not, depending on\nthe codepath).\n\nBut in general, I don't think there's any reasonable use-cases for git\nneeding to repair from a broken or semi-broken installation that aren't\nso specific that they can be explicitly declared, e.g. in this\nhypothetical MAYBE_GETTEXT case.\n\nOtherwise if it's not conditional, e.g. my git on Debian that won't ever\nneed this is going to just subtly regress because the FS is broken or\nwhatever, and I don't think we should have such code in git running\nunconditionally.\n\n> I suppose the logic could be the other way round, i.e., with a flag\n> REALLY_REQUIRE_GETTEXT_AT_RUNTIME. But I also wonder if a user who does\n> not notice that the installation is broken without us screaming BUG in\n> their face, really minds the \"brokenness\" that much.\n\nWithout the BUG(...) that user is going to spend time fiddling with his\ni18n settings before finally realizing that his package is broken, much\nbetter to just emit an error so it's obvious that things are broken,\nrather than taking the fallback path.\n"},{"id":"345257","messageId":"nycvar.QRO.7.76.6.1804202110550.4241@ZVAVAG-6OXH6DA.rhebcr.pbec.zvpebfbsg.pbz","threadId":"48330","inReplyTo":"87lgdidt30.fsf@evledraar.gmail.com","subject":"Re: [PATCH 1/3] gettext: avoid initialization if the locale dir is not present","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2018-04-20T19:35:53Z","receivedAt":"2018-04-20T19:36:19Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Ævar,\n\nOn Fri, 20 Apr 2018, Ævar Arnfjörð Bjarmason wrote:\n\n> On Fri, Apr 20 2018, Martin Ågren wrote:\n> \n> > On 20 April 2018 at 11:59, Ævar Arnfjörð Bjarmason wrote:\n> >>\n> >> On Fri, Apr 20 2018, Johannes Schindelin wrote:\n> >>\n> >>> The runtime of a simple `git.exe version` call on Windows is currently\n> >>> dominated by the gettext setup, adding a whopping ~150ms to the ~210ms\n> >>> total.\n> >>>\n> >>> Given that this cost is added to each and every git.exe invocation goes\n> >>> through common-main's invocation of git_setup_gettext(), and given that\n> >>> scripts have to call git.exe dozens, if not hundreds, of times, this is\n> >>> a substantial performance penalty.\n> >>>\n> >>> This is particularly pointless when considering that Git for Windows\n> >>> ships without localization (to keep the installer's size to a bearable\n> >>> ~34MB): all that time setting up gettext is for naught.\n> >>>\n> >>> So let's be smart about it and skip setting up gettext if the locale\n> >>> directory is not even present.\n> >\n> >> If you don't ship git for windows with gettext or a podir, then compile\n> >> with NO_GETTEXT, then we won't even compile this function you're\n> >> patching here. See line 30 and beyond of gettext.h.\n> >>\n> >> Are you instead compiling git for windows with gettext support with an\n> >> optional add-on package for i18n, so then this podir conditional would\n> >> pass?\n> >>\n> >> In any case, if this is actually the desired behavior I think it's worth\n> >> clearly explaining the interaction with NO_GETTEXT in the commit\n> >> message, and if you *actually* don't want NO_GETTEXT I think it's useful\n> >> to guard this with something like MAYBE_GETTEXT on top, so this code\n> >> doesn't unintentionally hide installation errors on other\n> >> platforms. I.e. something like:\n> >>\n> >> int have_podir = is_directory(podir);\n> >> if (!have_podir)\n> >> #ifdef MAYBE_GETTEXT\n> >> return;\n> >> #else\n> >> BUG(\"Broken installation, can't find '%s'!\", podir);\n> >> #endif\n> >\n> > Is it fair to say that such a broken installation would function more or\n> > less well before and after Dscho's patch, and that your approach would\n> > render such an installation quite useless?\n> \n> Yes my thown out there for the purposes of discussion suggestion may\n> break things for Dscho, or not. I'm just saying that we should document\n> *what* the use-case is.\n\nI think you underestimate the scope of your willful breakage. It is not\njust \"break things for Dscho\". It is breaking things for every single last\nuser of Git for Windows. If we ever do that, I will make sure to announce\nthat together with your name and your postal address on it.\n\n> Because it's not just important to massage the code so it works now, but\n> to document what the use-case is, so someone down the line who things\n> \"oh why are we doing that\" has a clear answer.\n\nLet's face it: if gettext would ever fail to work in case of a missing\npodir, then it would fail for every installation using a locale for which\nwe just happen not to have any translation.\n\nSo we know that your patch would not only break things, but break things\ntotally without reason!\n\n> > Do we have some other similar\n> > cases where we go BUG(\"I could not find a resource. I could manage\n> > fairly well without it, but you seemed to want it when you installed\n> > me.\")? I wonder what other well-respected software do if they can't find\n> > their translations.\n> \n> E.g. my recent 1aca69c019 (\"perl Git::LoadCPAN: emit better errors under\n> NO_PERL_CPAN_FALLBACKS\", 2018-03-03) does similar things, we *could*\n> carry on in certain cases instead of dying there (or not, depending on\n> the codepath).\n> \n> But in general, I don't think there's any reasonable use-cases for git\n> needing to repair from a broken or semi-broken installation that aren't\n> so specific that they can be explicitly declared, e.g. in this\n> hypothetical MAYBE_GETTEXT case.\n\nÆvar, you need to understand one thing, and you need to understand it\nright now: the vast majority of Git users will not compile their Git. Not.\nEver. Really, I am not kidding. They use whatever built version they can\nget most conveniently.\n\nIt is even more true on Windows, where it may be easier to build Git for\nWindows now (what with all the work I put into the Git for Windows SDK),\nbut it is still an expensive endeavor.\n\nAnd that is even assuming that every Git user is at liberty to build their\nown software, which is completely untrue in a large chunk of our business.\n\nSo in order to give users who want localization what they want, without\nburdening users who do not want localization, we use the exact same build\nof Git for Windows for both, and the entire difference is that one comes\nwith the podir, and the other without.\n\nOkay, I am lying, at the moment we do not even have anything end-user\nfacing that ships with the podir. But I distinctly want to leave the door\nopen for that.\n\nAnd if you think that this is out of sheer laziness on my part, then I\nhate to break it to you that this is for *very* good reasons: maintenance\ncost.\n\nIt probably has not crossed your mind that the entire Git test suite fails\nto pass if you run it on Git for Windows when built with NO_GETTEXT\ndefined?\n\nYep. It fails. With a honking\n\n\tBUG: your vsnprintf is broken (returned -1)\n\nAnd if you build it without NO_GETTEXT, it passes without that error.\nCrazy, huh?\n\nSo unless you acquire quite a bit more experience with maintaining Git on\na common platform, I would like to implore you to stop trying to break it.\nThank you very much.\n\n> Otherwise if it's not conditional, e.g. my git on Debian that won't ever\n> need this is going to just subtly regress because the FS is broken or\n> whatever, and I don't think we should have such code in git running\n> unconditionally.\n\nI have not the faintest idea what the heck you are talking about.\n\nIf the podir exists, then it is used. No change from before.\n\nIf the podir does not exist (which is unlikely in your setup, but there\nyou go, it might happen), the *only* difference you might notice is that\nthings get slightly faster (but probably not even noticably so, on your\nplatform). That is literally the only difference you could possibly notice\nwith vs without my patch. Seriously.\n\n> > I suppose the logic could be the other way round, i.e., with a flag\n> > REALLY_REQUIRE_GETTEXT_AT_RUNTIME. But I also wonder if a user who does\n> > not notice that the installation is broken without us screaming BUG in\n> > their face, really minds the \"brokenness\" that much.\n> \n> Without the BUG(...) that user is going to spend time fiddling with his\n\nDon't exclude half of humanity, for your own sake.\n\n> i18n settings before finally realizing that his package is broken, much\n> better to just emit an error so it's obvious that things are broken,\n> rather than taking the fallback path.\n\nAnd then you will also add code to output BUG BUG BUG when the actual\n*language* subdirectory is missing?\n\nYou would *need* to do that, and you know this as well as I do, because\neven if your Git installation is \"broken\" and does not bring its l18n\nstuff, *another package will have it*, and therefore podir *exists*! And\ntherefore your suggested change would not only break Git for Windows, it\nwould also totally not do what you want it to do!\n\nAnd even then, I totally, utterly hate to break it to you: even on Ubuntu,\nit is quite, quite common to have the git package installed *without* the\nlanguage-pack-de-base or whatever you need for your locale.\n\nThe reason is the same as for Git for Windows: we want to save on space,\nso you do not get localization files that you are not even interested in,\ninstead you get a leaner download.\n\nAnd in this case -- even on Ubuntu -- your patch does not make sense, but\nsimply breaks things for no reason other than some well-intended but\nhorribly broken idea that you should force the user to have a\n/usr/share/locale/ even if they may never even want it, and even if things\nwork very well without it, thankyouverymuch.\n\nIn short: I totally disagree with your reasoning to break things that work\nvery well, with a BUG() that is prone to confuse and scare end users no\nless.\n\nCiao,\nDscho"},{"id":"345323","messageId":"87efj9dmx5.fsf@evledraar.gmail.com","threadId":"48330","inReplyTo":"nycvar.QRO.7.76.6.1804202110550.4241@ZVAVAG-6OXH6DA.rhebcr.pbec.zvpebfbsg.pbz","subject":"Re: [PATCH 1/3] gettext: avoid initialization if the locale dir is not present","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-04-21T07:43:50Z","receivedAt":"2018-04-21T07:43:57Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Fri, Apr 20 2018, Johannes Schindelin wrote:\n\n> Hi Ævar,\n>\n> On Fri, 20 Apr 2018, Ævar Arnfjörð Bjarmason wrote:\n>\n>> On Fri, Apr 20 2018, Martin Ågren wrote:\n>>\n>> > On 20 April 2018 at 11:59, Ævar Arnfjörð Bjarmason wrote:\n>> >>\n>> >> On Fri, Apr 20 2018, Johannes Schindelin wrote:\n>> >>\n>> >>> The runtime of a simple `git.exe version` call on Windows is currently\n>> >>> dominated by the gettext setup, adding a whopping ~150ms to the ~210ms\n>> >>> total.\n>> >>>\n>> >>> Given that this cost is added to each and every git.exe invocation goes\n>> >>> through common-main's invocation of git_setup_gettext(), and given that\n>> >>> scripts have to call git.exe dozens, if not hundreds, of times, this is\n>> >>> a substantial performance penalty.\n>> >>>\n>> >>> This is particularly pointless when considering that Git for Windows\n>> >>> ships without localization (to keep the installer's size to a bearable\n>> >>> ~34MB): all that time setting up gettext is for naught.\n>> >>>\n>> >>> So let's be smart about it and skip setting up gettext if the locale\n>> >>> directory is not even present.\n>> >\n>> >> If you don't ship git for windows with gettext or a podir, then compile\n>> >> with NO_GETTEXT, then we won't even compile this function you're\n>> >> patching here. See line 30 and beyond of gettext.h.\n>> >>\n>> >> Are you instead compiling git for windows with gettext support with an\n>> >> optional add-on package for i18n, so then this podir conditional would\n>> >> pass?\n>> >>\n>> >> In any case, if this is actually the desired behavior I think it's worth\n>> >> clearly explaining the interaction with NO_GETTEXT in the commit\n>> >> message, and if you *actually* don't want NO_GETTEXT I think it's useful\n>> >> to guard this with something like MAYBE_GETTEXT on top, so this code\n>> >> doesn't unintentionally hide installation errors on other\n>> >> platforms. I.e. something like:\n>> >>\n>> >> int have_podir = is_directory(podir);\n>> >> if (!have_podir)\n>> >> #ifdef MAYBE_GETTEXT\n>> >> return;\n>> >> #else\n>> >> BUG(\"Broken installation, can't find '%s'!\", podir);\n>> >> #endif\n>> >\n>> > Is it fair to say that such a broken installation would function more or\n>> > less well before and after Dscho's patch, and that your approach would\n>> > render such an installation quite useless?\n>>\n>> Yes my thown out there for the purposes of discussion suggestion may\n>> break things for Dscho, or not. I'm just saying that we should document\n>> *what* the use-case is.\n>\n> I think you underestimate the scope of your willful breakage. It is not\n> just \"break things for Dscho\". It is breaking things for every single last\n> user of Git for Windows. If we ever do that, I will make sure to announce\n> that together with your name and your postal address on it.\n>\n>> Because it's not just important to massage the code so it works now, but\n>> to document what the use-case is, so someone down the line who things\n>> \"oh why are we doing that\" has a clear answer.\n>\n> Let's face it: if gettext would ever fail to work in case of a missing\n> podir, then it would fail for every installation using a locale for which\n> we just happen not to have any translation.\n>\n> So we know that your patch would not only break things, but break things\n> totally without reason!\n>\n>> > Do we have some other similar\n>> > cases where we go BUG(\"I could not find a resource. I could manage\n>> > fairly well without it, but you seemed to want it when you installed\n>> > me.\")? I wonder what other well-respected software do if they can't find\n>> > their translations.\n>>\n>> E.g. my recent 1aca69c019 (\"perl Git::LoadCPAN: emit better errors under\n>> NO_PERL_CPAN_FALLBACKS\", 2018-03-03) does similar things, we *could*\n>> carry on in certain cases instead of dying there (or not, depending on\n>> the codepath).\n>>\n>> But in general, I don't think there's any reasonable use-cases for git\n>> needing to repair from a broken or semi-broken installation that aren't\n>> so specific that they can be explicitly declared, e.g. in this\n>> hypothetical MAYBE_GETTEXT case.\n>\n> Ævar, you need to understand one thing, and you need to understand it\n> right now: the vast majority of Git users will not compile their Git. Not.\n> Ever. Really, I am not kidding. They use whatever built version they can\n> get most conveniently.\n>\n> It is even more true on Windows, where it may be easier to build Git for\n> Windows now (what with all the work I put into the Git for Windows SDK),\n> but it is still an expensive endeavor.\n>\n> And that is even assuming that every Git user is at liberty to build their\n> own software, which is completely untrue in a large chunk of our business.\n>\n> So in order to give users who want localization what they want, without\n> burdening users who do not want localization, we use the exact same build\n> of Git for Windows for both, and the entire difference is that one comes\n> with the podir, and the other without.\n>\n> Okay, I am lying, at the moment we do not even have anything end-user\n> facing that ships with the podir. But I distinctly want to leave the door\n> open for that.\n>\n> And if you think that this is out of sheer laziness on my part, then I\n> hate to break it to you that this is for *very* good reasons: maintenance\n> cost.\n>\n> It probably has not crossed your mind that the entire Git test suite fails\n> to pass if you run it on Git for Windows when built with NO_GETTEXT\n> defined?\n>\n> Yep. It fails. With a honking\n>\n> \tBUG: your vsnprintf is broken (returned -1)\n>\n> And if you build it without NO_GETTEXT, it passes without that error.\n> Crazy, huh?\n>\n> So unless you acquire quite a bit more experience with maintaining Git on\n> a common platform, I would like to implore you to stop trying to break it.\n> Thank you very much.\n>\n>> Otherwise if it's not conditional, e.g. my git on Debian that won't ever\n>> need this is going to just subtly regress because the FS is broken or\n>> whatever, and I don't think we should have such code in git running\n>> unconditionally.\n>\n> I have not the faintest idea what the heck you are talking about.\n>\n> If the podir exists, then it is used. No change from before.\n>\n> If the podir does not exist (which is unlikely in your setup, but there\n> you go, it might happen), the *only* difference you might notice is that\n> things get slightly faster (but probably not even noticably so, on your\n> platform). That is literally the only difference you could possibly notice\n> with vs without my patch. Seriously.\n>\n>> > I suppose the logic could be the other way round, i.e., with a flag\n>> > REALLY_REQUIRE_GETTEXT_AT_RUNTIME. But I also wonder if a user who does\n>> > not notice that the installation is broken without us screaming BUG in\n>> > their face, really minds the \"brokenness\" that much.\n>>\n>> Without the BUG(...) that user is going to spend time fiddling with his\n>\n> Don't exclude half of humanity, for your own sake.\n>\n>> i18n settings before finally realizing that his package is broken, much\n>> better to just emit an error so it's obvious that things are broken,\n>> rather than taking the fallback path.\n>\n> And then you will also add code to output BUG BUG BUG when the actual\n> *language* subdirectory is missing?\n>\n> You would *need* to do that, and you know this as well as I do, because\n> even if your Git installation is \"broken\" and does not bring its l18n\n> stuff, *another package will have it*, and therefore podir *exists*! And\n> therefore your suggested change would not only break Git for Windows, it\n> would also totally not do what you want it to do!\n>\n> And even then, I totally, utterly hate to break it to you: even on Ubuntu,\n> it is quite, quite common to have the git package installed *without* the\n> language-pack-de-base or whatever you need for your locale.\n>\n> The reason is the same as for Git for Windows: we want to save on space,\n> so you do not get localization files that you are not even interested in,\n> instead you get a leaner download.\n>\n> And in this case -- even on Ubuntu -- your patch does not make sense, but\n> simply breaks things for no reason other than some well-intended but\n> horribly broken idea that you should force the user to have a\n> /usr/share/locale/ even if they may never even want it, and even if things\n> work very well without it, thankyouverymuch.\n>\n> In short: I totally disagree with your reasoning to break things that work\n> very well, with a BUG() that is prone to confuse and scare end users no\n> less.\n\nYou raise a lot of good points, although I wish you could phrase them\nwithout the threats of doxxing.\n\nThe suggestion of doing that BUG(...) assertion was, to be clear, not\nmentioned as a \"we should definitely do this\", but \"why don't we do\nthis?\".\n\nI still stand by the observation that the \"why don't we\" isn't clear at\nall from your patch's commit message and that should be fixed. It\ndoesn't mention any of the actual reasons for the change which have been\nrevealed in this follow-up thread.\n\nI.e. that you're aiming to compile a version of git that has a\nhot-swappable \"locale\" directory, and don't want to slow down those\nusers who don't have it with pointless gettext setup.\n\nI know that the \"vast majority of Git users will not compile their\nGit\". I'm not talking about that, but that there are certainly packagers\nwho know that they're building for an installation where\n/usr/share/locale is expected never expected to just disappear.\n\nAsserting this particular thing isn't going to be that useful, since\ngettext has fallback behavior anyway. It just looks suspicious to me\nthat git at runtime has fallback behavior for stuff not existing that we\n*know* we installed during 'make install'.\n\nThat's why I mentioned that we should consider doing something similar\nto NO_PERL_CPAN_FALLBACKS. I.e. that there's a 1=1 mapping between what\nthe packager declared was going to happen, and what fallback behaviors\nthe runtime package is using.\n"},{"id":"345345","messageId":"nycvar.QRO.7.76.6.1804211217220.4241@ZVAVAG-6OXH6DA.rhebcr.pbec.zvpebfbsg.pbz","threadId":"48330","inReplyTo":"87efj9dmx5.fsf@evledraar.gmail.com","subject":"Re: [PATCH 1/3] gettext: avoid initialization if the locale dir is not present","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2018-04-21T10:17:53Z","receivedAt":"2018-04-21T10:18:18Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Ævar,\n\nOn Sat, 21 Apr 2018, Ævar Arnfjörð Bjarmason wrote:\n\n> I still stand by the observation that the \"why don't we\" isn't clear at\n> all from your patch's commit message and that should be fixed. It\n> doesn't mention any of the actual reasons for the change which have been\n> revealed in this follow-up thread.\n\nFair enough.\n\nWill fix,\nDscho"},{"id":"345363","messageId":"cover.1524309209.git.johannes.schindelin@gmx.de","threadId":"48330","inReplyTo":"cover.1524211375.git.johannes.schindelin@gmx.de","subject":"[PATCH v2 0/3] Some add-on patches on top of dj/runtime-prefix","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2018-04-21T11:13:48Z","receivedAt":"2018-04-21T11:14:12Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"We carried a slightly different version of the git_setup_gettext() patch\n(which took care of releasing the buffer allocated by system_path()),\nand we carry two more patches that touch the same area, so now that\ndj/runtime-prefix hit `next`, it seems a good time to contribute those,\ntoo.\n\nChanges since v1:\n\n- clarified in v1 why we cannot simply force users to recompile with NO_GETTEXT\n instead.\n\n\nJohannes Schindelin (2):\n gettext: avoid initialization if the locale dir is not present\n git_setup_gettext: plug memory leak\n\nPhilip Oakley (1):\n Avoid multiple PREFIX definitions\n\n Makefile | 2 +-\n exec-cmd.c | 4 ++--\n gettext.c | 10 +++++++++-\n sideband.c | 10 +++++-----\n 4 files changed, 17 insertions(+), 9 deletions(-)\n\n\nbase-commit: b46fe60e1d7235603a29499822493bd3791195da\nPublished-As: https://github.com/dscho/git/releases/tag/runtime-prefix-addons-v2\nFetch-It-Via: git fetch https://github.com/dscho/git runtime-prefix-addons-v2\n-- \n2.17.0.windows.1.15.gaa56ade3205\n\n"},{"id":"345364","messageId":"4ac5d3b9ace6ea8a41e97c036c8122f0d26d3c5a.1524309209.git.johannes.schindelin@gmx.de","threadId":"48330","inReplyTo":"cover.1524309209.git.johannes.schindelin@gmx.de","subject":"[PATCH v2 1/3] gettext: avoid initialization if the locale dir is not present","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2018-04-21T11:14:08Z","receivedAt":"2018-04-21T11:14:31Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"The runtime of a simple `git.exe version` call on Windows is currently\ndominated by the gettext setup, adding a whopping ~150ms to the ~210ms\ntotal.\n\nGiven that this cost is added to each and every git.exe invocation goes\nthrough common-main's invocation of git_setup_gettext(), and given that\nscripts have to call git.exe dozens, if not hundreds, of times, this is\na substantial performance penalty.\n\nThis is particularly pointless when considering that Git for Windows\nships without localization (to keep the installer's size to a bearable\n~34MB): all that time setting up gettext is for naught.\n\nTo be clear, Git for Windows *needs* to be compiled with localization,\nfor the following reasons:\n\n- to allow users to copy add-on localization in case they want it, and\n\n- to fix the nasty error message\n\n\tBUG: your vsnprintf is broken (returned -1)\n\n by using libgettext's override of vsnprintf() that does not share the\n behavior of msvcrt.dll's version of vsnprintf().\n\nSo let's be smart about it and skip setting up gettext if the locale\ndirectory is not even present.\n\nSince localization might be missing for not-yet-supported locales, this\nwill not break anything.\n\nSigned-off-by: Johannes Schindelin \n---\n gettext.c | 3 +++\n 1 file changed, 3 insertions(+)\n\ndiff --git a/gettext.c b/gettext.c\nindex baba28343c3..701355d66e7 100644\n--- a/gettext.c\n+++ b/gettext.c\n@@ -163,6 +163,9 @@ void git_setup_gettext(void)\n \tif (!podir)\n \t\tpodir = system_path(GIT_LOCALE_PATH);\n \n+\tif (!is_directory(podir))\n+\t\treturn;\n+\n \tbindtextdomain(\"git\", podir);\n \tsetlocale(LC_MESSAGES, \"\");\n \tsetlocale(LC_TIME, \"\");\n-- \n2.17.0.windows.1.15.gaa56ade3205\n\n\n"},{"id":"345365","messageId":"34971f7dd4c4b5df1f7924759d5aee9784c82fc6.1524309209.git.johannes.schindelin@gmx.de","threadId":"48330","inReplyTo":"cover.1524309209.git.johannes.schindelin@gmx.de","subject":"[PATCH v2 2/3] git_setup_gettext: plug memory leak","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2018-04-21T11:14:28Z","receivedAt":"2018-04-21T11:14:50Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"The system_path() function returns a freshly-allocated string. We need\nto release it.\n\nSigned-off-by: Johannes Schindelin \n---\n gettext.c | 9 +++++++--\n 1 file changed, 7 insertions(+), 2 deletions(-)\n\ndiff --git a/gettext.c b/gettext.c\nindex 701355d66e7..7272771c8e4 100644\n--- a/gettext.c\n+++ b/gettext.c\n@@ -159,18 +159,23 @@ static void init_gettext_charset(const char *domain)\n void git_setup_gettext(void)\n {\n \tconst char *podir = getenv(GIT_TEXT_DOMAIN_DIR_ENVIRONMENT);\n+\tchar *p = NULL;\n \n \tif (!podir)\n-\t\tpodir = system_path(GIT_LOCALE_PATH);\n+\t\tpodir = p = system_path(GIT_LOCALE_PATH);\n \n-\tif (!is_directory(podir))\n+\tif (!is_directory(podir)) {\n+\t\tfree(p);\n \t\treturn;\n+\t}\n \n \tbindtextdomain(\"git\", podir);\n \tsetlocale(LC_MESSAGES, \"\");\n \tsetlocale(LC_TIME, \"\");\n \tinit_gettext_charset(\"git\");\n \ttextdomain(\"git\");\n+\n+\tfree(p);\n }\n \n /* return the number of columns of string 's' in current locale */\n-- \n2.17.0.windows.1.15.gaa56ade3205\n\n\n"},{"id":"345366","messageId":"nycvar.QRO.7.76.6.1804211316370.4241@ZVAVAG-6OXH6DA.rhebcr.pbec.zvpebfbsg.pbz","threadId":"48330","inReplyTo":"cover.1524309209.git.johannes.schindelin@gmx.de","subject":"[PATCH v2 3/3] Avoid multiple PREFIX definitions","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2018-04-21T11:18:42Z","receivedAt":"2018-04-21T11:19:06Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Philip Oakley \n\nThe short and sweet PREFIX can be confused when used in many places.\n\nRename both usages to better describe their purpose. EXEC_CMD_PREFIX is\nused in full to disambiguate it from the nearby GIT_EXEC_PATH.\n\nThe PREFIX in sideband.c, while nominally independant of the exec_cmd\nPREFIX, does reside within libgit[1], so the definitions would clash\nwhen taken together with a PREFIX given on the command line for use by\nexec_cmd.c.\n\nNoticed when compiling Git for Windows using MSVC/Visual Studio [1] which\nreports the conflict beteeen the command line definition and the\ndefinition in sideband.c within the libgit project.\n\n[1] the libgit functions are brought into a single sub-project\nwithin the Visual Studio construction script provided in contrib,\nand hence uses a single command for both exec_cmd.c and sideband.c.\n\nSigned-off-by: Philip Oakley \nSigned-off-by: Johannes Schindelin \n---\n Makefile | 2 +-\n exec-cmd.c | 4 ++--\n sideband.c | 10 +++++-----\n 3 files changed, 8 insertions(+), 8 deletions(-)\n\ndiff --git a/Makefile b/Makefile\nindex 111e93d3bea..49cec672242 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -2271,7 +2271,7 @@ exec-cmd.sp exec-cmd.s exec-cmd.o: EXTRA_CPPFLAGS = \\\n \t'-DGIT_EXEC_PATH=\"$(gitexecdir_SQ)\"' \\\n \t'-DGIT_LOCALE_PATH=\"$(localedir_relative_SQ)\"' \\\n \t'-DBINDIR=\"$(bindir_relative_SQ)\"' \\\n-\t'-DPREFIX=\"$(prefix_SQ)\"'\n+\t'-DFALLBACK_RUNTIME_PREFIX=\"$(prefix_SQ)\"'\n \n builtin/init-db.sp builtin/init-db.s builtin/init-db.o: GIT-PREFIX\n builtin/init-db.sp builtin/init-db.s builtin/init-db.o: EXTRA_CPPFLAGS = \\\ndiff --git a/exec-cmd.c b/exec-cmd.c\nindex 3b0a039083a..02d31ee8971 100644\n--- a/exec-cmd.c\n+++ b/exec-cmd.c\n@@ -48,7 +48,7 @@ static const char *system_prefix(void)\n \t !(prefix = strip_path_suffix(executable_dirname, GIT_EXEC_PATH)) &&\n \t !(prefix = strip_path_suffix(executable_dirname, BINDIR)) &&\n \t !(prefix = strip_path_suffix(executable_dirname, \"git\"))) {\n-\t\tprefix = PREFIX;\n+\t\tprefix = FALLBACK_RUNTIME_PREFIX;\n \t\ttrace_printf(\"RUNTIME_PREFIX requested, \"\n \t\t\t\t\"but prefix computation failed. \"\n \t\t\t\t\"Using static fallback '%s'.\\n\", prefix);\n@@ -243,7 +243,7 @@ void git_resolve_executable_dir(const char *argv0)\n */\n static const char *system_prefix(void)\n {\n-\treturn PREFIX;\n+\treturn FALLBACK_RUNTIME_PREFIX;\n }\n \n /*\ndiff --git a/sideband.c b/sideband.c\nindex 6d7f943e438..325bf0e974a 100644\n--- a/sideband.c\n+++ b/sideband.c\n@@ -13,7 +13,7 @@\n * the remote died unexpectedly. A flush() concludes the stream.\n */\n \n-#define PREFIX \"remote: \"\n+#define DISPLAY_PREFIX \"remote: \"\n \n #define ANSI_SUFFIX \"\\033[K\"\n #define DUMB_SUFFIX \" \"\n@@ -49,7 +49,7 @@ int recv_sideband(const char *me, int in_stream, int out)\n \t\tswitch (band) {\n \t\tcase 3:\n \t\t\tstrbuf_addf(&outbuf, \"%s%s%s\", outbuf.len ? \"\\n\" : \"\",\n-\t\t\t\t PREFIX, buf + 1);\n+\t\t\t\t DISPLAY_PREFIX, buf + 1);\n \t\t\tretval = SIDEBAND_REMOTE_ERROR;\n \t\t\tbreak;\n \t\tcase 2:\n@@ -67,7 +67,7 @@ int recv_sideband(const char *me, int in_stream, int out)\n \t\t\t\tint linelen = brk - b;\n \n \t\t\t\tif (!outbuf.len)\n-\t\t\t\t\tstrbuf_addstr(&outbuf, PREFIX);\n+\t\t\t\t\tstrbuf_addstr(&outbuf, DISPLAY_PREFIX);\n \t\t\t\tif (linelen > 0) {\n \t\t\t\t\tstrbuf_addf(&outbuf, \"%.*s%s%c\",\n \t\t\t\t\t\t linelen, b, suffix, *brk);\n@@ -81,8 +81,8 @@ int recv_sideband(const char *me, int in_stream, int out)\n \t\t\t}\n \n \t\t\tif (*b)\n-\t\t\t\tstrbuf_addf(&outbuf, \"%s%s\",\n-\t\t\t\t\t outbuf.len ? \"\" : PREFIX, b);\n+\t\t\t\tstrbuf_addf(&outbuf, \"%s%s\", outbuf.len ?\n+\t\t\t\t\t \"\" : DISPLAY_PREFIX, b);\n \t\t\tbreak;\n \t\tcase 1:\n \t\t\twrite_or_die(out, buf + 1, len);\n-- \n2.17.0.windows.1.15.gaa56ade3205\n"},{"id":"345396","messageId":"AE53F7B8907A4FE3BFDC8806B5C21AB3@PhilipOakley","threadId":"48330","inReplyTo":"d134afdd22c9cdfd763213b37efac4ef0399a374.1524211375.git.johannes.schindelin@gmx.de","subject":"Re: [PATCH 3/3] Avoid multiple PREFIX definitions","fromName":"Philip Oakley","fromEmail":"philipoakley@iee.org","sentAt":"2018-04-22T15:32:24Z","receivedAt":"2018-04-22T15:32:38Z","isPatch":true,"sender":{"key":"philipoakley@iee.email","avatar":"https://avatars.githubusercontent.com/u/914343?v=4"},"body":"From: \"Johannes Schindelin\" \n> From: Philip Oakley \n>\n> The short and sweet PREFIX can be confused when used in many places.\n>\n> Rename both usages to better describe their purpose. EXEC_CMD_PREFIX is\n> used in full to disambiguate it from the nearby GIT_EXEC_PATH.\n\n@dcsho; Thanks for keeping up with this and all your work. LGTM Philip.\n\n>\n> The PREFIX in sideband.c, while nominally independant of the exec_cmd\n> PREFIX, does reside within libgit[1], so the definitions would clash\n> when taken together with a PREFIX given on the command line for use by\n> exec_cmd.c.\n>\n> Noticed when compiling Git for Windows using MSVC/Visual Studio [1] which\n> reports the conflict beteeen the command line definition and the\n> definition in sideband.c within the libgit project.\n>\n> [1] the libgit functions are brought into a single sub-project\n> within the Visual Studio construction script provided in contrib,\n> and hence uses a single command for both exec_cmd.c and sideband.c.\n>\n> Signed-off-by: Philip Oakley \n> Signed-off-by: Johannes Schindelin \n> ---\n> Makefile | 2 +-\n> exec-cmd.c | 4 ++--\n> sideband.c | 10 +++++-----\n> 3 files changed, 8 insertions(+), 8 deletions(-)\n>\n> diff --git a/Makefile b/Makefile\n> index 111e93d3bea..49cec672242 100644\n> --- a/Makefile\n> +++ b/Makefile\n> @@ -2271,7 +2271,7 @@ exec-cmd.sp exec-cmd.s exec-cmd.o: EXTRA_CPPFLAGS =\n> \\\n> '-DGIT_EXEC_PATH=\"$(gitexecdir_SQ)\"' \\\n> '-DGIT_LOCALE_PATH=\"$(localedir_relative_SQ)\"' \\\n> '-DBINDIR=\"$(bindir_relative_SQ)\"' \\\n> - '-DPREFIX=\"$(prefix_SQ)\"'\n> + '-DFALLBACK_RUNTIME_PREFIX=\"$(prefix_SQ)\"'\n>\n> builtin/init-db.sp builtin/init-db.s builtin/init-db.o: GIT-PREFIX\n> builtin/init-db.sp builtin/init-db.s builtin/init-db.o: EXTRA_CPPFLAGS = \\\n> diff --git a/exec-cmd.c b/exec-cmd.c\n> index 3b0a039083a..02d31ee8971 100644\n> --- a/exec-cmd.c\n> +++ b/exec-cmd.c\n> @@ -48,7 +48,7 @@ static const char *system_prefix(void)\n> !(prefix = strip_path_suffix(executable_dirname, GIT_EXEC_PATH)) &&\n> !(prefix = strip_path_suffix(executable_dirname, BINDIR)) &&\n> !(prefix = strip_path_suffix(executable_dirname, \"git\"))) {\n> - prefix = PREFIX;\n> + prefix = FALLBACK_RUNTIME_PREFIX;\n> trace_printf(\"RUNTIME_PREFIX requested, \"\n> \"but prefix computation failed. \"\n> \"Using static fallback '%s'.\\n\", prefix);\n> @@ -243,7 +243,7 @@ void git_resolve_executable_dir(const char *argv0)\n> */\n> static const char *system_prefix(void)\n> {\n> - return PREFIX;\n> + return FALLBACK_RUNTIME_PREFIX;\n> }\n>\n> /*\n> diff --git a/sideband.c b/sideband.c\n> index 6d7f943e438..325bf0e974a 100644\n> --- a/sideband.c\n> +++ b/sideband.c\n> @@ -13,7 +13,7 @@\n> * the remote died unexpectedly. A flush() concludes the stream.\n> */\n>\n> -#define PREFIX \"remote: \"\n> +#define DISPLAY_PREFIX \"remote: \"\n>\n> #define ANSI_SUFFIX \"\\033[K\"\n> #define DUMB_SUFFIX \" \"\n> @@ -49,7 +49,7 @@ int recv_sideband(const char *me, int in_stream, int\n> out)\n> switch (band) {\n> case 3:\n> strbuf_addf(&outbuf, \"%s%s%s\", outbuf.len ? \"\\n\" : \"\",\n> - PREFIX, buf + 1);\n> + DISPLAY_PREFIX, buf + 1);\n> retval = SIDEBAND_REMOTE_ERROR;\n> break;\n> case 2:\n> @@ -67,7 +67,7 @@ int recv_sideband(const char *me, int in_stream, int\n> out)\n> int linelen = brk - b;\n>\n> if (!outbuf.len)\n> - strbuf_addstr(&outbuf, PREFIX);\n> + strbuf_addstr(&outbuf, DISPLAY_PREFIX);\n> if (linelen > 0) {\n> strbuf_addf(&outbuf, \"%.*s%s%c\",\n> linelen, b, suffix, *brk);\n> @@ -81,8 +81,8 @@ int recv_sideband(const char *me, int in_stream, int\n> out)\n> }\n>\n> if (*b)\n> - strbuf_addf(&outbuf, \"%s%s\",\n> - outbuf.len ? \"\" : PREFIX, b);\n> + strbuf_addf(&outbuf, \"%s%s\", outbuf.len ?\n> + \"\" : DISPLAY_PREFIX, b);\n> break;\n> case 1:\n> write_or_die(out, buf + 1, len);\n> -- \n> 2.17.0.windows.1.15.gaa56ade3205\n>\n\n"},{"id":"345593","messageId":"xmqqzi1t74zo.fsf@gitster-ct.c.googlers.com","threadId":"48330","inReplyTo":"cover.1524309209.git.johannes.schindelin@gmx.de","subject":"Re: [PATCH v2 0/3] Some add-on patches on top of dj/runtime-prefix","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-04-24T01:44:27Z","receivedAt":"2018-04-24T01:44:33Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> We carried a slightly different version of the git_setup_gettext() patch\n> (which took care of releasing the buffer allocated by system_path()),\n> and we carry two more patches that touch the same area, so now that\n> dj/runtime-prefix hit `next`, it seems a good time to contribute those,\n> too.\n>\n> Changes since v1:\n>\n> - clarified in v1 why we cannot simply force users to recompile with NO_GETTEXT\n> instead.\n>\n>\n> Johannes Schindelin (2):\n> gettext: avoid initialization if the locale dir is not present\n> git_setup_gettext: plug memory leak\n>\n> Philip Oakley (1):\n> Avoid multiple PREFIX definitions\n>\n> Makefile | 2 +-\n> exec-cmd.c | 4 ++--\n> gettext.c | 10 +++++++++-\n> sideband.c | 10 +++++-----\n> 4 files changed, 17 insertions(+), 9 deletions(-)\n>\n>\n> base-commit: b46fe60e1d7235603a29499822493bd3791195da\n\nBasing your work on the tip of 'next' is good for discussion, but\nnot readily usable for final application. Let me see if I can\nuntangle the dependents to come up with a reasonable base.\n\nThe changes themselves looked reasonable. Thanks.\n"},{"id":"345594","messageId":"xmqqvach74qe.fsf@gitster-ct.c.googlers.com","threadId":"48330","inReplyTo":"xmqqzi1t74zo.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v2 0/3] Some add-on patches on top of dj/runtime-prefix","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-04-24T01:50:01Z","receivedAt":"2018-04-24T01:50:06Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n>> base-commit: b46fe60e1d7235603a29499822493bd3791195da\n>\n> Basing your work on the tip of 'next' is good for discussion, but\n> not readily usable for final application. Let me see if I can\n> untangle the dependents to come up with a reasonable base.\n\nI'll queue this on top of a merge of 'dj/runtime-prefix' into 'master'.\nMerging the resulting topic into 'next' and applying these patches\ndirectly on top of 'next' result in identical trees, of course ;-)\n\n"},{"id":"345600","messageId":"xmqqa7tt72c9.fsf@gitster-ct.c.googlers.com","threadId":"48330","inReplyTo":"xmqqvach74qe.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v2 0/3] Some add-on patches on top of dj/runtime-prefix","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-04-24T02:41:42Z","receivedAt":"2018-04-24T02:42:00Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Junio C Hamano writes:\n>\n>>> base-commit: b46fe60e1d7235603a29499822493bd3791195da\n>>\n>> Basing your work on the tip of 'next' is good for discussion, but\n>> not readily usable for final application. Let me see if I can\n>> untangle the dependents to come up with a reasonable base.\n>\n> I'll queue this on top of a merge of 'dj/runtime-prefix' into 'master'.\n> Merging the resulting topic into 'next' and applying these patches\n> directly on top of 'next' result in identical trees, of course ;-)\n\nActually, these trivially rebase on top of dj/runtime-prefix, so\nI'll queue them like so without taking it hostage to other things in\n'master'. We'd want to keep these mergeable to any integration\nbranch that dj/runtime-prefix would be merged to, so that is the\nmost logical organization, I would think, even though I do not\nimmediately see the reason why we would want to merge\ndj/runtime-prefix to 'maint' and lower right now.\n\nThanks.\n"},{"id":"345645","messageId":"nycvar.QRO.7.76.6.1804241646400.64@ZVAVAG-6OXH6DA.rhebcr.pbec.zvpebfbsg.pbz","threadId":"48330","inReplyTo":"xmqqa7tt72c9.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v2 0/3] Some add-on patches on top of dj/runtime-prefix","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2018-04-24T14:48:04Z","receivedAt":"2018-04-24T14:48:20Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Tue, 24 Apr 2018, Junio C Hamano wrote:\n\n> Junio C Hamano writes:\n> \n> > Junio C Hamano writes:\n> >\n> >>> base-commit: b46fe60e1d7235603a29499822493bd3791195da\n> >>\n> >> Basing your work on the tip of 'next' is good for discussion, but\n> >> not readily usable for final application. Let me see if I can\n> >> untangle the dependents to come up with a reasonable base.\n> >\n> > I'll queue this on top of a merge of 'dj/runtime-prefix' into 'master'.\n> > Merging the resulting topic into 'next' and applying these patches\n> > directly on top of 'next' result in identical trees, of course ;-)\n> \n> Actually, these trivially rebase on top of dj/runtime-prefix, so\n> I'll queue them like so without taking it hostage to other things in\n> 'master'. We'd want to keep these mergeable to any integration\n> branch that dj/runtime-prefix would be merged to, so that is the\n> most logical organization, I would think, even though I do not\n> immediately see the reason why we would want to merge\n> dj/runtime-prefix to 'maint' and lower right now.\n> \n> Thanks.\n\nThank you, and sorry for the trouble. I am just too used to a Continuous\nIntegration setting with exactly one integration branch.\n\nI will make an effort in the future to figure out the best base branch for\npatches that do not apply cleanly on `master` but require more stuff from\n`next`/`pu`.\n\nCiao,\nDscho\n"},{"id":"345725","messageId":"xmqqr2n43wh5.fsf@gitster-ct.c.googlers.com","threadId":"48330","inReplyTo":"nycvar.QRO.7.76.6.1804241646400.64@ZVAVAG-6OXH6DA.rhebcr.pbec.zvpebfbsg.pbz","subject":"Re: [PATCH v2 0/3] Some add-on patches on top of dj/runtime-prefix","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-04-25T01:28:54Z","receivedAt":"2018-04-25T01:29:01Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> Thank you, and sorry for the trouble. I am just too used to a Continuous\n> Integration setting with exactly one integration branch.\n\nFixing problems close to the source (i.e. picking an appropriately\naged base) and making sure everthing works near the tip ala CI style\nare not opposing goals. It just takes an extra step (i.e. trial\nmerge and testing the result) and discipline. Until one gets used\nto do it so much that one can do in one's sleep, that is ;-)\n\n> I will make an effort in the future to figure out the best base branch for\n> patches that do not apply cleanly on `master` but require more stuff from\n> `next`/`pu`.\n\nThe easiest is to leave that to the maintainer most of the time, as\nthat is what maintainers do.\n\nThanks.\n\nI really want to see that the runtime prefix stuff mature enough\nduring this cycle, so these follow-up patches are all very much\nappreciated.\n"},{"id":"345741","messageId":"nycvar.QRO.7.76.6.1804250944080.4978@tvgsbejvaqbjf.bet","threadId":"48330","inReplyTo":"xmqqr2n43wh5.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v2 0/3] Some add-on patches on top of dj/runtime-prefix","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2018-04-25T07:46:03Z","receivedAt":"2018-04-25T07:46:23Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Wed, 25 Apr 2018, Junio C Hamano wrote:\n\n> I really want to see that the runtime prefix stuff mature enough during\n> this cycle, so these follow-up patches are all very much appreciated.\n\nFWIW I merged these patches (including my touch-ups) into Git for Windows'\n`master` branch early.\n\n(I should actually not say that I merged it, because that would have\nmerged way too much, as Git for Windows' `master` is still based on\nv2.17.0. So I cherry-picked instead.)\n\nThat should help mature those patches in one big hurry. If they're not yet\nperfect, that is.\n\nCiao,\nDscho\n"}]}