Re: [PATCH] PRItime: wrap PRItime for better l10n compatibility

[Junio C Hamano]

Johannes Schindelin  writes:

> On Sat, 22 Jul 2017, Junio C Hamano wrote:
>
>> Johannes Schindelin  writes:
>> 
>> >> >> A very small hack on gettext.
>> >
>> > I am 100% opposed to this hack. It is already cumbersome enough to find
>> > out what is involved in i18n (it took *me* five minutes to find out that
>> > much of the information is in po/README, with a lot of information stored
>> > *on an external site*, and I still managed to miss the `make pot` target).
>> >
>> > If at all, we need to make things easier instead of harder.
>> >
>> > Requiring potential volunteers to waste their time to compile an
>> > unnecessary fork of gettext? Not so great an idea.
>> >
>> > Plus, each and every Git build would now have to compile their own
>> > gettext, too, as the vanilla one would not handle the .po files containing
>> > %!!!
>> >
>> > And that requirement would impact instantaneously people like me, and even
>> > worse: some other packagers might be unaware of the new requirement which
>> > would not be caught during the build, and neither by the test suite.
>> > Double bad idea.
>> 
>> If I understand correctly, the patch hacks the input processing of
>> xgettext (which reads our source code and generates po/git.pot) so
>> that when it sees PRItime, pretend that it saw PRIuMAX, causing it
>> to output % in its output.
>
> Oh, I missed that. That's even worse, as it precludes what you were
> wishing for: to replace timestamp_t by a signed data type eventually.

Yup, Jiang's plan was to update the custom edition of xgettext when
it happens and the Makefile patch has a provision to avoid mistakes.

If Jiang's patch were extended so that xgettext would take a command
line option "--custom-priformat=PRItime=PRIuMAX" and upstreamed
and wildy deployed already, that would have been a good solution.
That might be a preferred outcome that may benefit other projects,
but it won't happen for at least 3 years if not more X-<.

Re: [PATCH] PRItime: wrap PRItime for better l10n compatibility

[Junio C Hamano]

Johannes Schindelin  writes:

> Hi Junio,
>
> On Sat, 22 Jul 2017, Junio C Hamano wrote:
>
>> Johannes Schindelin  writes:
>> 
>> > On Fri, 21 Jul 2017, Junio C Hamano wrote:
>> >
>> >> Jean-Noël Avila  writes:
>> >> 
>> >> > Le 20/07/2017 à 20:57, Junio C Hamano a écrit :
>> >> >>
>> >> >> +  git diff --quiet HEAD && git diff --quiet --cached
>> >> >> +
>> >> >> +  @for s in $(LOCALIZED_C) $(LOCALIZED_SH) $(LOCALIZED_PERL); \
>> >> >
>> >> > Does PRIuMAX make sense for perl and sh files?
>> >> 
>> >> Not really; I did this primarily because I would prefer to keep
>> >> things consistent, anticipating there may be some other things we
>> >> need to replace before running gettext(1) for other reasons later.
>> >
>> > It would add unnecessary churn, too, to add those specific exclusions and
>> > make things inconsistent: the use of PRItime in Perl or shell scripts
>> > would already make those scripts barf. And if it is unnecessary churn...
>> > let's not do it?
>> 
>> Sorry, but I cannot quite tell if you are in favor of limiting the
>> set of source files that go through the sed substitution (because we
>> know PRIuMAX is just as nonsensical as PRItime in perl and shell
>> source), or if you are in favor of keeping the patch as-is (because
>> changing the set of source files is a churn and substitutions would
>> not hurt)?
>
> I was in favor of keeping the simplest strategy: simply cover all files,
> including Perl and Unix shell scripts. It would not bring any benefit to
> exclude them.

OK.  I actually was OK to limit the potential damage to C sources,
but it does not matter that much in the bigger picture.

Re: [PATCH] PRItime: wrap PRItime for better l10n compatibility

[Johannes Schindelin]

Hi,

On Mon, 24 Jul 2017, Jiang Xin wrote:

> 2017-07-22 19:28 GMT+08:00 Johannes Schindelin :
> >
> > On Sat, 22 Jul 2017, Jiang Xin wrote:
> >
> >> 2017-07-22 7:34 GMT+08:00 Junio C Hamano :
> >> > Jiang Xin  writes:
> >> >
> >> >> A very small hack on gettext.
> >
> > I am 100% opposed to this hack.
> 
> It's really very small, see:
> 
> *  https://github.com/jiangxin/gettext/commit/b0a72643

I don't care about size. Insecure, pampered white men may be concerned
about size; not I.

I am concerned about an *unnecessary* deviation from a well-established
and well-maintained piece of software that would all of a sudden be
version-coupled with Git.

> > If at all, we need to make things easier instead of harder.
> 
> If it is only the l10n coordinate's duty to generate po/git.pot, the
> tweak is OK.

No. You want to keep the bus number high.

> I agree.  We just go with the sed-then-cleanup version until we meet
> ambiguities (I mean some words other than PRItime need to be
> replaced).

Even then. Even then you should want to avoid version-coupling Git
versions with gettext versions. Because that's precisely what you would
do, as gettext would now have to know about *this* Git version's
interpretation of certain data type names.

Ciao,
Dscho

Re: [PATCH] PRItime: wrap PRItime for better l10n compatibility

[Johannes Schindelin]

Hi,

On Sat, 22 Jul 2017, Junio C Hamano wrote:

> Johannes Schindelin  writes:
> 
> >> >> A very small hack on gettext.
> >
> > I am 100% opposed to this hack. It is already cumbersome enough to find
> > out what is involved in i18n (it took *me* five minutes to find out that
> > much of the information is in po/README, with a lot of information stored
> > *on an external site*, and I still managed to miss the `make pot` target).
> >
> > If at all, we need to make things easier instead of harder.
> >
> > Requiring potential volunteers to waste their time to compile an
> > unnecessary fork of gettext? Not so great an idea.
> >
> > Plus, each and every Git build would now have to compile their own
> > gettext, too, as the vanilla one would not handle the .po files containing
> > %!!!
> >
> > And that requirement would impact instantaneously people like me, and even
> > worse: some other packagers might be unaware of the new requirement which
> > would not be caught during the build, and neither by the test suite.
> > Double bad idea.
> 
> If I understand correctly, the patch hacks the input processing of
> xgettext (which reads our source code and generates po/git.pot) so
> that when it sees PRItime, pretend that it saw PRIuMAX, causing it
> to output % in its output.

Oh, I missed that. That's even worse, as it precludes what you were
wishing for: to replace timestamp_t by a signed data type eventually.

Ciao,
Dscho

Re: [PATCH] PRItime: wrap PRItime for better l10n compatibility

[Johannes Schindelin]

Hi Junio,

On Sat, 22 Jul 2017, Junio C Hamano wrote:

> Johannes Schindelin  writes:
> 
> > On Fri, 21 Jul 2017, Junio C Hamano wrote:
> >
> >> Jean-Noël Avila  writes:
> >> 
> >> > Le 20/07/2017 à 20:57, Junio C Hamano a écrit :
> >> >>
> >> >> +   git diff --quiet HEAD && git diff --quiet --cached
> >> >> +
> >> >> +   @for s in $(LOCALIZED_C) $(LOCALIZED_SH) $(LOCALIZED_PERL); \
> >> >
> >> > Does PRIuMAX make sense for perl and sh files?
> >> 
> >> Not really; I did this primarily because I would prefer to keep
> >> things consistent, anticipating there may be some other things we
> >> need to replace before running gettext(1) for other reasons later.
> >
> > It would add unnecessary churn, too, to add those specific exclusions and
> > make things inconsistent: the use of PRItime in Perl or shell scripts
> > would already make those scripts barf. And if it is unnecessary churn...
> > let's not do it?
> 
> Sorry, but I cannot quite tell if you are in favor of limiting the
> set of source files that go through the sed substitution (because we
> know PRIuMAX is just as nonsensical as PRItime in perl and shell
> source), or if you are in favor of keeping the patch as-is (because
> changing the set of source files is a churn and substitutions would
> not hurt)?

I was in favor of keeping the simplest strategy: simply cover all files,
including Perl and Unix shell scripts. It would not bring any benefit to
exclude them.

Ciao,
Dscho

Re: [PATCH] PRItime: wrap PRItime for better l10n compatibility

[Junio C Hamano]

Jiang Xin  writes:

> 2017-07-23 10:33 GMT+08:00 Jean-Noël AVILA :
>> Plus, I hope that some day, instead of translators finding afterwards
>> that a change broke i18n capabilities, developpers would have some kind
>> of sanity check. Requiring special versions of i18n tooling stops this hope.
>
> It would be fun to create some tools to help l10n guys finding l10n
> changes on every git commit.

It is not just fun but would be useful, I would think.

Re: [PATCH] PRItime: wrap PRItime for better l10n compatibility

[Junio C Hamano]

Jiang Xin  writes:

>> So let's go with Junio's patch.
>
> I agree.  We just go with the sed-then-cleanup version until we meet
> ambiguities (I mean some words other than PRItime need to be
> replaced).

OK, thanks for all involved to get us to a conclusion.  Jiang, I saw
you already made an announcement for the second round.  Thank you
very much for doing so without waiting me---I was stuck on something
else and my morning was blown X-<.  I'll still try to tag -rc1 by
the end of business today.

Re: [PATCH] PRItime: wrap PRItime for better l10n compatibility

[Jiang Xin]

2017-07-23 10:33 GMT+08:00 Jean-Noël AVILA :
> Plus, I hope that some day, instead of translators finding afterwards
> that a change broke i18n capabilities, developpers would have some kind
> of sanity check. Requiring special versions of i18n tooling stops this hope.
>

It would be fun to create some tools to help l10n guys finding l10n
changes on every git commit.


-- 
Jiang Xin

Re: [PATCH] PRItime: wrap PRItime for better l10n compatibility

[Jiang Xin]

2017-07-22 23:48 GMT+08:00 Junio C Hamano :
> Johannes Schindelin  writes:
>
>>> >> A very small hack on gettext.
>>
>> I am 100% opposed to this hack. It is already cumbersome enough to find
>> out what is involved in i18n (it took *me* five minutes to find out that
>> much of the information is in po/README, with a lot of information stored
>> *on an external site*, and I still managed to miss the `make pot` target).
>>
>> If at all, we need to make things easier instead of harder.
>>
>> Requiring potential volunteers to waste their time to compile an
>> unnecessary fork of gettext? Not so great an idea.
>>
>> Plus, each and every Git build would now have to compile their own
>> gettext, too, as the vanilla one would not handle the .po files containing
>> %!!!
>>
>> And that requirement would impact instantaneously people like me, and even
>> worse: some other packagers might be unaware of the new requirement which
>> would not be caught during the build, and neither by the test suite.
>> Double bad idea.
>
> If I understand correctly, the patch hacks the input processing of
> xgettext (which reads our source code and generates po/git.pot) so
> that when it sees PRItime, pretend that it saw PRIuMAX, causing it
> to output % in its output.
>
> In our workflow,
>
> * The po/git.pot file is updated only by the l10n coordinator,
>   and then the result is committed to our tree.
>
> * Translators build on that commit by (1) running msgmerge which
>   takes po/git.pot and wiggles its entries into their existing
>   po/$lang.po file so that po/$lang.po file has new entries from
>   po/git.pot and (2) editing po/$lang.po file.  The result is
>   committed to our tree.
>
> * The build procedure builders use runs the resulting
>   po/$lang.po files through msgfmt to produce po/$lang.mo files,
>   which will be installed.
>
> As long as the first step results in % (not % or
> anything that plain vanilla msgmerge and msgfmt do not understand),
> the second step and third step do not require any hacked version of
> gettext tools.
>
> Even though I tend to agree with your conclusion that pre-processing
> our source before passing it to xgettext is probably a better
> solution in the longer term, I think the most of the objections in
> your message come from your misunderstanding of what Jiang's patch
> does and are not based on facts.  My understanding is that
> translators do not need to compile a custom msgmerge and builders do
> not need a custom msgfmt.
>

I appreciate Junio's explanation. I totally agree.

-- 
Jiang Xin

Re: [PATCH] PRItime: wrap PRItime for better l10n compatibility

[Jiang Xin]

2017-07-22 19:28 GMT+08:00 Johannes Schindelin :
> Hi,
>
> On Sat, 22 Jul 2017, Jiang Xin wrote:
>
>> 2017-07-22 7:34 GMT+08:00 Junio C Hamano :
>> > Jiang Xin  writes:
>> >
>> >> A very small hack on gettext.
>
> I am 100% opposed to this hack.

It's really very small, see:

*  https://github.com/jiangxin/gettext/commit/b0a72643
*  
https://public-inbox.org/git/a87e7252bf9de8a87e5dc7712946f72459778d6c.1500684532.git.worldhello@gmail.com/

> It is already cumbersome enough to find
> out what is involved in i18n (it took *me* five minutes to find out that
> much of the information is in po/README, with a lot of information stored
> *on an external site*, and I still managed to miss the `make pot` target).
>
> If at all, we need to make things easier instead of harder.

If it is only the l10n coordinate's duty to generate po/git.pot, the
tweak is OK.  But if other guys need to recreate po/git.pot, it's
hard, especially for guys working on Mac or Windows.

>
> Requiring potential volunteers to waste their time to compile an
> unnecessary fork of gettext? Not so great an idea.
>
> Plus, each and every Git build would now have to compile their own
> gettext, too, as the vanilla one would not handle the .po files containing
> %!!!

No, only l10n coordinator and potential po/git.pot generator are involved.

>
> So let's go with Junio's patch.

I agree.  We just go with the sed-then-cleanup version until we meet
ambiguities (I mean some words other than PRItime need to be
replaced).

-- 
Jiang Xin

Re: [PATCH] PRItime: wrap PRItime for better l10n compatibility

[Junio C Hamano]

Jean-Noël AVILA  writes:

> Le 22/07/2017 à 02:43, Jiang Xin a écrit :
>>
>> Benefit of using the tweak version of gettext:
>>
>> 1. `make pot` can be run in a tar extract directory (without git controlled).
>
> This issue is real for packet maintainers who can patch the original
> source and run their own set of utilities outside of a git repo. This
> can be possible with Junio's proposition by writing the files to a
> temporary directory before running the xgettext, then removing the
> temporary directory.
>
> Please note that with respect to this issue, the patched xgettext
> approach is completely disruptive.

OK, so what you are saying is that my assumption that Jiang (at
least for now, and his successor l10n coordinator in sometime in the
future) would be the only one who needs to have access to the
machinery to update po/git.pot and that it does not matter that much
what that exact machinery is as long as the resulting po/git.pot
lists messages with % and other known ones because plain
vanilla tools will grok such po/git.pot file just fine, were both
too optimistic.

I think binary packagers, who update the software with their own
changes, produce their own modified po/git.pot and have that
translated into multiple languages, are capable of coping with any
method we use ourselves, but being capable of doing something and
being happy to do that thing are two different things, and we need
to aim for the latter---we should not make things unnecessarily
cumbersome for them.

So I'll leave the s/PRItime/PRIuMAX/ patch in the 'master' without
Jiang's change for 2.14-rc1.  The approach to require private
edition of xgettext, while it may technically be a fun exercise,
would not fly very well in the real world.

For those who want to work with a tarball extract without being in a
Git repository, it would be sufficient fot them to run "git init &&
git commit --allow-empty -m import" immediately after extracting the
tarball, even if we require that "make pot" must be run in a clean
repository.  And I'd prefer to go that route than copying into a
temporary directory, primarily because I do not want to having to
worry about what to copy---when we know we pass $foo.c through
xgettext, we know we want to put the modified copy of $foo.c in the
temporary, but I do not want to even think if we need to also copy
the header files $foo.c "#include"s, for example.

Thanks.

Re: [PATCH] PRItime: wrap PRItime for better l10n compatibility

[Jean-Noël AVILA]

Le 22/07/2017 à 02:43, Jiang Xin a écrit :
>
> Benefit of using the tweak version of gettext:
>
> 1. `make pot` can be run in a tar extract directory (without git controlled).

This issue is real for packet maintainers who can patch the original
source and run their own set of utilities outside of a git repo. This
can be possible with Junio's proposition by writing the files to a
temporary directory before running the xgettext, then removing the
temporary directory.

Please note that with respect to this issue, the patched xgettext
approach is completely disruptive.

> 2. do not need to run `git reset --hard`.

Same as before.

> 3.  it's quick (nobody cares).
>

Requiring patched tools is really breaking collaboration. Git made a
great case of relying on standard tools (not even GNU versions), so that
would really go backward.


Plus, I hope that some day, instead of translators finding afterwards
that a change broke i18n capabilities, developpers would have some kind
of sanity check. Requiring special versions of i18n tooling stops this hope.

<>

Re: [PATCH] PRItime: wrap PRItime for better l10n compatibility

[Junio C Hamano]

Johannes Schindelin  writes:

> On Fri, 21 Jul 2017, Junio C Hamano wrote:
>
>> Jean-Noël Avila  writes:
>> 
>> > Le 20/07/2017 à 20:57, Junio C Hamano a écrit :
>> >>
>> >> + git diff --quiet HEAD && git diff --quiet --cached
>> >> +
>> >> + @for s in $(LOCALIZED_C) $(LOCALIZED_SH) $(LOCALIZED_PERL); \
>> >
>> > Does PRIuMAX make sense for perl and sh files?
>> 
>> Not really; I did this primarily because I would prefer to keep
>> things consistent, anticipating there may be some other things we
>> need to replace before running gettext(1) for other reasons later.
>
> It would add unnecessary churn, too, to add those specific exclusions and
> make things inconsistent: the use of PRItime in Perl or shell scripts
> would already make those scripts barf. And if it is unnecessary churn...
> let's not do it?

Sorry, but I cannot quite tell if you are in favor of limiting the
set of source files that go through the sed substitution (because we
know PRIuMAX is just as nonsensical as PRItime in perl and shell
source), or if you are in favor of keeping the patch as-is (because
changing the set of source files is a churn and substitutions would
not hurt)?

I am actually OK to change the above loop to process only the C
sources; I am not OK to change it to process only date.c which
happens to be the only source that has PRItime that matters in this
context, of course.

Thanks.

Re: [PATCH] PRItime: wrap PRItime for better l10n compatibility

[Junio C Hamano]

Johannes Schindelin  writes:

>> >> A very small hack on gettext.
>
> I am 100% opposed to this hack. It is already cumbersome enough to find
> out what is involved in i18n (it took *me* five minutes to find out that
> much of the information is in po/README, with a lot of information stored
> *on an external site*, and I still managed to miss the `make pot` target).
>
> If at all, we need to make things easier instead of harder.
>
> Requiring potential volunteers to waste their time to compile an
> unnecessary fork of gettext? Not so great an idea.
>
> Plus, each and every Git build would now have to compile their own
> gettext, too, as the vanilla one would not handle the .po files containing
> %!!!
>
> And that requirement would impact instantaneously people like me, and even
> worse: some other packagers might be unaware of the new requirement which
> would not be caught during the build, and neither by the test suite.
> Double bad idea.

If I understand correctly, the patch hacks the input processing of
xgettext (which reads our source code and generates po/git.pot) so
that when it sees PRItime, pretend that it saw PRIuMAX, causing it
to output % in its output.

In our workflow, 

* The po/git.pot file is updated only by the l10n coordinator,
  and then the result is committed to our tree.

* Translators build on that commit by (1) running msgmerge which
  takes po/git.pot and wiggles its entries into their existing
  po/$lang.po file so that po/$lang.po file has new entries from
  po/git.pot and (2) editing po/$lang.po file.  The result is
  committed to our tree.

* The build procedure builders use runs the resulting
  po/$lang.po files through msgfmt to produce po/$lang.mo files,
  which will be installed.

As long as the first step results in % (not % or
anything that plain vanilla msgmerge and msgfmt do not understand),
the second step and third step do not require any hacked version of
gettext tools.

Even though I tend to agree with your conclusion that pre-processing
our source before passing it to xgettext is probably a better
solution in the longer term, I think the most of the objections in
your message come from your misunderstanding of what Jiang's patch
does and are not based on facts.  My understanding is that
translators do not need to compile a custom msgmerge and builders do
not need a custom msgfmt.

Re: [PATCH] PRItime: wrap PRItime for better l10n compatibility

[Johannes Schindelin]

Hi,

On Sat, 22 Jul 2017, Jiang Xin wrote:

> 2017-07-22 7:34 GMT+08:00 Junio C Hamano :
> > Jiang Xin  writes:
> >
> >> A very small hack on gettext.

I am 100% opposed to this hack. It is already cumbersome enough to find
out what is involved in i18n (it took *me* five minutes to find out that
much of the information is in po/README, with a lot of information stored
*on an external site*, and I still managed to miss the `make pot` target).

If at all, we need to make things easier instead of harder.

Requiring potential volunteers to waste their time to compile an
unnecessary fork of gettext? Not so great an idea.

Plus, each and every Git build would now have to compile their own
gettext, too, as the vanilla one would not handle the .po files containing
%!!!

And that requirement would impact instantaneously people like me, and even
worse: some other packagers might be unaware of the new requirement which
would not be caught during the build, and neither by the test suite.
Double bad idea.

So let's go with Junio's patch.

Ciao,
Dscho

Re: [PATCH] PRItime: wrap PRItime for better l10n compatibility

[Johannes Schindelin]

Hi,

On Fri, 21 Jul 2017, Junio C Hamano wrote:

> Jean-Noël Avila  writes:
> 
> > Le 20/07/2017 à 20:57, Junio C Hamano a écrit :
> >>
> >> +  git diff --quiet HEAD && git diff --quiet --cached
> >> +
> >> +  @for s in $(LOCALIZED_C) $(LOCALIZED_SH) $(LOCALIZED_PERL); \
> >
> > Does PRIuMAX make sense for perl and sh files?
> 
> Not really; I did this primarily because I would prefer to keep
> things consistent, anticipating there may be some other things we
> need to replace before running gettext(1) for other reasons later.

It would add unnecessary churn, too, to add those specific exclusions and
make things inconsistent: the use of PRItime in Perl or shell scripts
would already make those scripts barf. And if it is unnecessary churn...
let's not do it?

Ciao,
Dscho

Re: [PATCH] PRItime: wrap PRItime for better l10n compatibility

[Junio C Hamano]

Jiang Xin  writes:

> But it is rare to maintain po/git.pot file for 'maint' branch.  And if
> I need, I will switch to a different version of gettext.  Makefile
> will throw a error message, if I use a wrong version of gettext.

Is that the "v1" in the check in your Makefile patch is about?  That
is, when we need to change the underlying type, your updated gettext
would say something different from "v1" and you will have Makefile
update to expect that new version?  Then it would be workable, I'd
guess.

>> Compared to that, Dscho's "hack" at least ties what PRItime is
>> replaced with and what the source code says by being in the
>> Makefile, which is tracked alongside the rest of the source.  So I
>> somehow feel that the approach has smaller chance of going wrong.
>
> Benefit of using the tweak version of gettext:
>
> 1. `make pot` can be run in a tar extract directory (without git controlled).
> 2. do not need to run `git reset --hard`.
> 3.  it's quick (nobody cares).

This is about a tool and workflow only you (and your successor, when
the time comes) need to use, so ultimately I'd prefer to leave it up
to you, but I'd want to make sure you are making your decision with
sound rationale.  I personally do not think 1. and 2. above are real
issues in practice, because (1) you'd be committing the result of
"make pot" so that translators can work off of it---which at least
to me means that being able to run on a tarball extract is not a
useful feature, and (2) when you are about to run xgettext to
extract strings from the message, you do not want to have local
modifications, extracting potentially modified strings with
potentially offset line numbers in the resulting pot file, so
having to run "git reset --hard" at the end is not a problem.

I do not know if 3. is a practical issue or not (I did try "make
pot" with the tip of 'master' tonight myself, and I didn't find it
much slower than the unmodified one).

Having said all that, again, I'd prefer to leave this up to you, so
unless I hear that you changed your mind, the tip of 'master' I'll
tag, probably sometime tomorrow my time, will have your patch to the
Makefile to rely on your private edition of xgettext.

Thanks.

Re: [PATCH] PRItime: wrap PRItime for better l10n compatibility

[Jiang Xin]

2017-07-22 7:34 GMT+08:00 Junio C Hamano :
> Jiang Xin  writes:
>
>> A very small hack on gettext.  When extract l10n messages to pot file
>> with `xgettext`, will grep "PRItime", and do "sed s/PRItime/PRIuMAX"
>> inside `xgettext`.
>>
>> See this patch:
>> https://github.com/jiangxin/gettext/commit/b0a726431c93b5a1ca0fe749de376b0752e75fb0
>> ...
>>  gettext-tools/src/x-c.c  | 17 -
>>  gettext-tools/src/xgettext.c |  2 +-
>>  2 files changed, 17 insertions(+), 2 deletions(-)
>
> I do not think the size of the "hack" is much of an issue.  There is
> no way you can sell this patch to the upstream, which would mean
> that we would have to be relying on our own private edition of the
> external tool, and that is what I feel very uncomfortable about.

I never think about that, and I won't sell it to the upstream. ;)

> You are not passing % through the toolchain and instead
> turning it into %, which is less risky than the obvious
> alternative, but when we switch to a signed timestamp_t type and
> need to change it something else (e.g.  PRIdMAX), you'd need to make
> sure you update that private edition that matches the source being
> compiled.

That's why I grep "PRItime to PRIuMAX" from `xgettext --version`.
When we need something else, we can tweak the "check-xgettext" task
again in Makefile, to match with a grep-"PRItime to PRIdMAX" version
of `xgettext`.

>  You might even be asked to do the po/git.pot thing for
> both 'maint' and 'master' at the same time, when the former still
> uses unsigned timestamp_t while the latter switched to signed one,
> which would mean you'd need two hacked versions of gettext handy and
> choose the "right" one.

But it is rare to maintain po/git.pot file for 'maint' branch.  And if
I need, I will switch to a different version of gettext.  Makefile
will throw a error message, if I use a wrong version of gettext.

> Compared to that, Dscho's "hack" at least ties what PRItime is
> replaced with and what the source code says by being in the
> Makefile, which is tracked alongside the rest of the source.  So I
> somehow feel that the approach has smaller chance of going wrong.

Benefit of using the tweak version of gettext:

1. `make pot` can be run in a tar extract directory (without git controlled).
2. do not need to run `git reset --hard`.
3.  it's quick (nobody cares).

-- 
Jiang Xin

Re: [PATCH] PRItime: wrap PRItime for better l10n compatibility

[Junio C Hamano]

Jiang Xin  writes:

> A very small hack on gettext.  When extract l10n messages to pot file
> with `xgettext`, will grep "PRItime", and do "sed s/PRItime/PRIuMAX"
> inside `xgettext`.
>
> See this patch:
> https://github.com/jiangxin/gettext/commit/b0a726431c93b5a1ca0fe749de376b0752e75fb0
> ...
>  gettext-tools/src/x-c.c  | 17 -
>  gettext-tools/src/xgettext.c |  2 +-
>  2 files changed, 17 insertions(+), 2 deletions(-)

I do not think the size of the "hack" is much of an issue.  There is
no way you can sell this patch to the upstream, which would mean
that we would have to be relying on our own private edition of the
external tool, and that is what I feel very uncomfortable about.

You are not passing % through the toolchain and instead
turning it into %, which is less risky than the obvious
alternative, but when we switch to a signed timestamp_t type and
need to change it something else (e.g.  PRIdMAX), you'd need to make
sure you update that private edition that matches the source being
compiled.  You might even be asked to do the po/git.pot thing for
both 'maint' and 'master' at the same time, when the former still
uses unsigned timestamp_t while the latter switched to signed one,
which would mean you'd need two hacked versions of gettext handy and
choose the "right" one.

Compared to that, Dscho's "hack" at least ties what PRItime is
replaced with and what the source code says by being in the
Makefile, which is tracked alongside the rest of the source.  So I
somehow feel that the approach has smaller chance of going wrong.

Am I missing some concerns you had that you think the tweaked
gettext would solve better?

Re: [PATCH] PRItime: wrap PRItime for better l10n compatibility

[Jiang Xin]

2017-07-22 6:40 GMT+08:00 Junio C Hamano :
> Jiang Xin  writes:
>
>> Sorry, I'm late. I want to try a safer way to change PRItime to
>> PRInMax using a hacked version of gettext.
>
> Why?  A vanilla version of gettext tool that is fed a known PRIuMAX
> in its input would be a safer choice, I would have thought.
>
> I've already queued the patch you responded to on 'master', but
> haven't tagged -rc1 yet, so it is possible for you to update on top
> of it before -rc1 is tagged.  I do not yet understand why you think
> a modified version of gettext would be a safer way to go, though.
>
> Thanks.

A very small hack on gettext.  When extract l10n messages to pot file
with `xgettext`, will grep "PRItime", and do "sed s/PRItime/PRIuMAX"
inside `xgettext`.

See this patch:
https://github.com/jiangxin/gettext/commit/b0a726431c93b5a1ca0fe749de376b0752e75fb0

---
 gettext-tools/src/x-c.c  | 17 -
 gettext-tools/src/xgettext.c |  2 +-
 2 files changed, 17 insertions(+), 2 deletions(-)

diff --git a/gettext-tools/src/x-c.c b/gettext-tools/src/x-c.c
index 1844a5df7..51dd0a9bc 100644
--- a/gettext-tools/src/x-c.c
+++ b/gettext-tools/src/x-c.c
@@ -1802,6 +1802,12 @@ phase6_unget (token_ty *tp)
 static bool
 is_inttypes_macro (const char *name)
 {
+  /* PRItime is a maro for timestamp_t in git.git. */
+  if (!strncmp(name, "PRItime", 7))
+{
+  return true;
+}
+
   /* Syntax:
  P R I { d | i | o | u | x | X }
  { { | LEAST | FAST } { 8 | 16 | 32 | 64 } | MAX | PTR }  */
@@ -1843,8 +1849,17 @@ phase8a_get (token_ty *tp)
   phase6_get (tp);
   if (tp->type == token_type_name && is_inttypes_macro (tp->string))
 {
+  char *new_string;
   /* Turn PRIdXXX into "".  */
-  char *new_string = xasprintf ("<%s>", tp->string);
+  if (!strncmp(tp->string, "PRItime", 7))
+{
+  /* Replace PRItime with PRIuMAX for git.git project */
+  new_string = xasprintf ("<%s>", "PRIuMAX");
+}
+  else
+{
+  new_string = xasprintf ("<%s>", tp->string);
+}
   free (tp->string);
   tp->string = new_string;
   tp->comment = add_reference (savable_comment);
diff --git a/gettext-tools/src/xgettext.c b/gettext-tools/src/xgettext.c
index f848d76d1..0350a1bee 100644
--- a/gettext-tools/src/xgettext.c
+++ b/gettext-tools/src/xgettext.c
@@ -676,7 +676,7 @@ main (int argc, char *argv[])
   /* Version information requested.  */
   if (do_version)
 {
-  printf ("%s (GNU %s) %s\n", basename (program_name),
PACKAGE, VERSION);
+  printf ("%s (GNU %s) %s (PRItime to PRIuMAX for
git.git)\n", basename (program_name), PACKAGE, VERSION);
   /* xgettext: no-wrap */
   printf (_("Copyright (C) %s Free Software Foundation, Inc.\n\
 License GPLv3+: GNU GPL version 3 or later
\n\
--
2.14.0.rc0.1.g3ccfa2fb49
-- 
蒋鑫

华为技术有限公司
邮件: xin.ji...@huawei.com, worldhello@gmail.com
博客: http://www.worldhello.net/
微博: http://weibo.com/gotgit/
电话: 18601196889

Re: [PATCH] PRItime: wrap PRItime for better l10n compatibility

[Junio C Hamano]

Jiang Xin  writes:

> Sorry, I'm late. I want to try a safer way to change PRItime to
> PRInMax using a hacked version of gettext.

Why?  A vanilla version of gettext tool that is fed a known PRIuMAX
in its input would be a safer choice, I would have thought.

I've already queued the patch you responded to on 'master', but
haven't tagged -rc1 yet, so it is possible for you to update on top
of it before -rc1 is tagged.  I do not yet understand why you think
a modified version of gettext would be a safer way to go, though.

Thanks.

Re: [PATCH] PRItime: wrap PRItime for better l10n compatibility

[Jiang Xin]

2017-07-21 2:19 GMT+08:00 Junio C Hamano :
> Junio C Hamano  writes:
>
>> Johannes Schindelin  writes:
>>
>>> But there may be hope. Since the character sequence "PRItime" is highly
>>> unlikely to occur in Git's source code in any context other than the
>>> format to print/parse timestamp_t, it should be possible to automate a the
>>> string replacement
>>>
>>>  git ls-files -z \*.[ch] |
>>>  xargs -0r sed -i 's/PRItime/PRIuMAX/g'
>>>
>>> (assuming, of course, that you use GNU sed, not BSD sed, for which the
>>> `-i` needs to read `-i ''` instead) as part of the update?
>>
>> I somehow missed this bit.
>>
>> Given that this needs to be done only once every release by only one
>> person (i.e. the l10n coordinator who updates *.pot file), as long
>> as the procedure is automated as much as possible to ease the pain
>> for the l10n coordinator and clearly described in the "Maintaining
>> the po/git.pot file" section of po/README, something along that line
>> does sound like a very tempting approach.  If it works well, it is
>> certainly much easier for normal developers than the other possible
>> alternatives I mentioned in my previous response.
>
> So, I was offline for most of the day yesterday and with this issue
> blocking the release candidate, didn't manage to tag -rc1.
>
> The use of "make pot" from the top-level is already described in
> po/README, so the only thing that we need is something like this
> change.  I'll follow up this message with a sample output from the
> updated process to ask others to sanity check the result (they are
> tiny) in a separate message.
>
> Thanks.
>
>
>  Makefile | 10 ++
>  1 file changed, 10 insertions(+)
>
> diff --git a/Makefile b/Makefile
> index ba4359ef8d..7069a12f75 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -2216,12 +2216,22 @@ LOCALIZED_SH += t/t0200/test.sh
>  LOCALIZED_PERL += t/t0200/test.perl
>  endif
>
> +## Note that this is only meant to run by the localization coordinator
> +## under a very controlled condition, i.e. (1) it is to be run in a
> +## Git repository (not a tarball extract), (2) any local modifications
> +## will be lost.
>  po/git.pot: $(GENERATED_H) FORCE
> +   @for s in $(LOCALIZED_C) $(LOCALIZED_SH) $(LOCALIZED_PERL); \
> +   do \
> +   sed -e 's|PRItime|PRIuMAX|g' <"$$s" >"$$s+" && \
> +   cat "$$s+" >"$$s" && rm "$$s+"; \
> +   done
> $(QUIET_XGETTEXT)$(XGETTEXT) -o$@+ $(XGETTEXT_FLAGS_C) $(LOCALIZED_C)
> $(QUIET_XGETTEXT)$(XGETTEXT) -o$@+ --join-existing 
> $(XGETTEXT_FLAGS_SH) \
> $(LOCALIZED_SH)
> $(QUIET_XGETTEXT)$(XGETTEXT) -o$@+ --join-existing 
> $(XGETTEXT_FLAGS_PERL) \
> $(LOCALIZED_PERL)
> +   @git reset --hard
> mv $@+ $@
>
>  .PHONY: pot

Sorry, I'm late. I want to try a safer way to change PRItime to
PRInMax using a hacked version of gettext.

We can change Makefile like this:

--- a/Makefile
+++ b/Makefile
@@ -2216,7 +2216,14 @@ LOCALIZED_SH += t/t0200/test.sh
 LOCALIZED_PERL += t/t0200/test.perl
 endif

-po/git.pot: $(GENERATED_H) FORCE
+check_gettext:
+   @if ! $(XGETTEXT) --version | grep -q -i PRItime; then \
+   echo >&2 "Error: must use a hacked xgettext, which
can handle PRItime macro properly."; \
+   echo >&2 "Error: download the hacked version of
gettext from https://github.com/..; ; \
+   exit 1; \
+   fi
+
+po/git.pot: check_gettext $(GENERATED_H) FORCE
   $(QUIET_XGETTEXT)$(XGETTEXT) -o$@+ $(XGETTEXT_FLAGS_C) $(LOCALIZED_C)
   $(QUIET_XGETTEXT)$(XGETTEXT) -o$@+ --join-existing
$(XGETTEXT_FLAGS_SH) \
   $(LOCALIZED_SH)

But I'm not sure I can handle this in this very busy weekend.

-- 
Jiang Xin

Re: [PATCH] PRItime: wrap PRItime for better l10n compatibility

[Junio C Hamano]

Jean-Noël Avila  writes:

> Le 20/07/2017 à 20:57, Junio C Hamano a écrit :
>>
>> +git diff --quiet HEAD && git diff --quiet --cached
>> +
>> +@for s in $(LOCALIZED_C) $(LOCALIZED_SH) $(LOCALIZED_PERL); \
>
> Does PRIuMAX make sense for perl and sh files?

Not really; I did this primarily because I would prefer to keep
things consistent, anticipating there may be some other things we
need to replace before running gettext(1) for other reasons later.

I do not mind removing them, if Jiang/Dscho already reviewed and
tested _this_ version (which we do not yet know), I'd prefer not to
touch it for the upcoming release.

Thanks for reading it over.


>> +do \
>> +sed -e 's|PRItime|PRIuMAX|g' <"$$s" >"$$s+" && \
>> +cat "$$s+" >"$$s" && rm "$$s+"; \
>> +done
>> +
>>  $(QUIET_XGETTEXT)$(XGETTEXT) -o$@+ $(XGETTEXT_FLAGS_C) $(LOCALIZED_C)
>>  $(QUIET_XGETTEXT)$(XGETTEXT) -o$@+ --join-existing $(XGETTEXT_FLAGS_SH) 
>> \
>>  $(LOCALIZED_SH)
>>  $(QUIET_XGETTEXT)$(XGETTEXT) -o$@+ --join-existing 
>> $(XGETTEXT_FLAGS_PERL) \
>>  $(LOCALIZED_PERL)
>> +
>> +git reset --hard
>>  mv $@+ $@
>>  
>>  .PHONY: pot

Re: [PATCH] PRItime: wrap PRItime for better l10n compatibility

[Jean-Noël Avila]

Le 20/07/2017 à 20:57, Junio C Hamano a écrit :
>
> + git diff --quiet HEAD && git diff --quiet --cached
> +
> + @for s in $(LOCALIZED_C) $(LOCALIZED_SH) $(LOCALIZED_PERL); \

Does PRIuMAX make sense for perl and sh files?

> + do \
> + sed -e 's|PRItime|PRIuMAX|g' <"$$s" >"$$s+" && \
> + cat "$$s+" >"$$s" && rm "$$s+"; \
> + done
> +
>   $(QUIET_XGETTEXT)$(XGETTEXT) -o$@+ $(XGETTEXT_FLAGS_C) $(LOCALIZED_C)
>   $(QUIET_XGETTEXT)$(XGETTEXT) -o$@+ --join-existing $(XGETTEXT_FLAGS_SH) 
> \
>   $(LOCALIZED_SH)
>   $(QUIET_XGETTEXT)$(XGETTEXT) -o$@+ --join-existing 
> $(XGETTEXT_FLAGS_PERL) \
>   $(LOCALIZED_PERL)
> +
> + git reset --hard
>   mv $@+ $@
>  
>  .PHONY: pot

Re: [PATCH] PRItime: wrap PRItime for better l10n compatibility

[Junio C Hamano]

> The use of "make pot" from the top-level is already described in
> po/README, so the only thing that we need is something like this
> change.  I'll follow up this message with a sample output from the
> updated process to ask others to sanity check the result (they are
> tiny) in a separate message.

So I am inclined to apply this directly on 'master' before tagging
the first release candidate that includes timestamp_t; I'll wait for
the earth to rotate once for comments, though.

Thanks.

-- >8 --
Subject: [PATCH] Makefile: help gettext tools to cope with our custom PRItime 
format

We started using our own timestamp_t type and PRItime format
specifier to go along with it, so that we can later change the
underlying type and output format more easily, but this does not
play well with gettext tools.

Because gettext tools need to keep the *.po file portable across
platforms, they have to special-case the format specifiers like
PRIuMAX that are known types in inttypes.h, instead of letting CPP
handle strings like

"%" PRIuMAX " seconds ago"

as an ordinary string concatenation.  They fundamentally cannot do
the same for our own custom type/format.

Given that po/git.pot needs to be generated only once every release
and by only one person, i.e. the l10n coordinator, let's update the
Makefile rule to generate po/git.pot so that gettext tools are run
on a munged set of sources in which all mentions of PRItime are
replaced with PRIuMAX, which is what we happen to use right now.

This way, developers do not have to care that PRItime does not play
well with gettext, and translators do not have to care that we use
our own PRItime.

The credit for the idea to munge the source files goes to Dscho.
Possible bugs are mine.

Helped-by: Jiang Xin 
Helped-by: Johannes Schindelin 
Signed-off-by: Junio C Hamano 
---
 Makefile | 20 
 1 file changed, 20 insertions(+)

diff --git a/Makefile b/Makefile
index ba4359ef8d..527502835f 100644
--- a/Makefile
+++ b/Makefile
@@ -2216,12 +2216,32 @@ LOCALIZED_SH += t/t0200/test.sh
 LOCALIZED_PERL += t/t0200/test.perl
 endif
 
+## Note that this is meant to be run only by the localization coordinator
+## under a very controlled condition, i.e. (1) it is to be run in a
+## Git repository (not a tarball extract), (2) any local modifications
+## will be lost.
+## Gettext tools cannot work with our own custom PRItime type, so
+## we replace PRItime with PRIuMAX.  Weneed to update this if we
+## switch to a signed type with PRIdMAX.
+
 po/git.pot: $(GENERATED_H) FORCE
+   # All modifications will be reverted at the end, so we do not
+   # want to have any local changes
+   git diff --quiet HEAD && git diff --quiet --cached
+
+   @for s in $(LOCALIZED_C) $(LOCALIZED_SH) $(LOCALIZED_PERL); \
+   do \
+   sed -e 's|PRItime|PRIuMAX|g' <"$$s" >"$$s+" && \
+   cat "$$s+" >"$$s" && rm "$$s+"; \
+   done
+
$(QUIET_XGETTEXT)$(XGETTEXT) -o$@+ $(XGETTEXT_FLAGS_C) $(LOCALIZED_C)
$(QUIET_XGETTEXT)$(XGETTEXT) -o$@+ --join-existing $(XGETTEXT_FLAGS_SH) 
\
$(LOCALIZED_SH)
$(QUIET_XGETTEXT)$(XGETTEXT) -o$@+ --join-existing 
$(XGETTEXT_FLAGS_PERL) \
$(LOCALIZED_PERL)
+
+   git reset --hard
mv $@+ $@
 
 .PHONY: pot
-- 
2.14.0-rc0-194-g965e058453

Re: [PATCH] PRItime: wrap PRItime for better l10n compatibility

[Junio C Hamano]

Junio C Hamano  writes:

> The use of "make pot" from the top-level is already described in
> po/README, so the only thing that we need is something like this
> change.  I'll follow up this message with a sample output from the
> updated process to ask others to sanity check the result (they are
> tiny) in a separate message.

Without the Makefile patch in the previous message, I ran "make pot"
and saved the resulting po/git.pot to git.pot-old.  And then after
"git reset --hard", I applied the Makefile patch and ran "make pot"
again, which gave me an updated po/git.pot file.  The difference is
shown below.

As expected, these look sensible to me.  All the hits from 

git grep '_(.*PRItime'

are included in the difference.




--- git.pot-old 2017-07-20 11:17:29.608343390 -0700
+++ po/git.pot  2017-07-20 11:18:14.744342564 -0700
@@ -8,7 +8,7 @@
 msgstr ""
 "Project-Id-Version: PACKAGE VERSION\n"
 "Report-Msgid-Bugs-To: Git Mailing List \n"
-"POT-Creation-Date: 2017-07-20 11:17-0700\n"
+"POT-Creation-Date: 2017-07-20 11:18-0700\n"
 "PO-Revision-Date: YEAR-MO-DA HO:MI+ZONE\n"
 "Last-Translator: FULL NAME \n"
 "Language-Team: LANGUAGE \n"
@@ -1388,17 +1388,67 @@
 msgid "in the future"
 msgstr ""
 
-#: date.c:122 date.c:129 date.c:136 date.c:143 date.c:149 date.c:156
-#: date.c:167 date.c:175 date.c:180
-msgid "%"
-msgid_plural "%"
+#: date.c:122
+#, c-format
+msgid "% second ago"
+msgid_plural "% seconds ago"
+msgstr[0] ""
+msgstr[1] ""
+
+#: date.c:129
+#, c-format
+msgid "% minute ago"
+msgid_plural "% minutes ago"
+msgstr[0] ""
+msgstr[1] ""
+
+#: date.c:136
+#, c-format
+msgid "% hour ago"
+msgid_plural "% hours ago"
+msgstr[0] ""
+msgstr[1] ""
+
+#: date.c:143
+#, c-format
+msgid "% day ago"
+msgid_plural "% days ago"
+msgstr[0] ""
+msgstr[1] ""
+
+#: date.c:149
+#, c-format
+msgid "% week ago"
+msgid_plural "% weeks ago"
+msgstr[0] ""
+msgstr[1] ""
+
+#: date.c:156
+#, c-format
+msgid "% month ago"
+msgid_plural "% months ago"
+msgstr[0] ""
+msgstr[1] ""
+
+#: date.c:167
+#, c-format
+msgid "% year"
+msgid_plural "% years"
 msgstr[0] ""
 msgstr[1] ""
 
 #. TRANSLATORS: "%s" is " years"
 #: date.c:170
-msgid "%s, %"
-msgid_plural "%s, %"
+#, c-format
+msgid "%s, % month ago"
+msgid_plural "%s, % months ago"
+msgstr[0] ""
+msgstr[1] ""
+
+#: date.c:175 date.c:180
+#, c-format
+msgid "% year ago"
+msgid_plural "% years ago"
 msgstr[0] ""
 msgstr[1] ""
 

Re: [PATCH] PRItime: wrap PRItime for better l10n compatibility

[Junio C Hamano]

Junio C Hamano  writes:

> Johannes Schindelin  writes:
>
>> But there may be hope. Since the character sequence "PRItime" is highly
>> unlikely to occur in Git's source code in any context other than the
>> format to print/parse timestamp_t, it should be possible to automate a the
>> string replacement
>>
>>  git ls-files -z \*.[ch] |
>>  xargs -0r sed -i 's/PRItime/PRIuMAX/g'
>>
>> (assuming, of course, that you use GNU sed, not BSD sed, for which the
>> `-i` needs to read `-i ''` instead) as part of the update?
>
> I somehow missed this bit.
>
> Given that this needs to be done only once every release by only one
> person (i.e. the l10n coordinator who updates *.pot file), as long
> as the procedure is automated as much as possible to ease the pain
> for the l10n coordinator and clearly described in the "Maintaining
> the po/git.pot file" section of po/README, something along that line
> does sound like a very tempting approach.  If it works well, it is
> certainly much easier for normal developers than the other possible
> alternatives I mentioned in my previous response.

So, I was offline for most of the day yesterday and with this issue
blocking the release candidate, didn't manage to tag -rc1.

The use of "make pot" from the top-level is already described in
po/README, so the only thing that we need is something like this
change.  I'll follow up this message with a sample output from the
updated process to ask others to sanity check the result (they are
tiny) in a separate message.

Thanks.


 Makefile | 10 ++
 1 file changed, 10 insertions(+)

diff --git a/Makefile b/Makefile
index ba4359ef8d..7069a12f75 100644
--- a/Makefile
+++ b/Makefile
@@ -2216,12 +2216,22 @@ LOCALIZED_SH += t/t0200/test.sh
 LOCALIZED_PERL += t/t0200/test.perl
 endif
 
+## Note that this is only meant to run by the localization coordinator
+## under a very controlled condition, i.e. (1) it is to be run in a
+## Git repository (not a tarball extract), (2) any local modifications
+## will be lost.
 po/git.pot: $(GENERATED_H) FORCE
+   @for s in $(LOCALIZED_C) $(LOCALIZED_SH) $(LOCALIZED_PERL); \
+   do \
+   sed -e 's|PRItime|PRIuMAX|g' <"$$s" >"$$s+" && \
+   cat "$$s+" >"$$s" && rm "$$s+"; \
+   done
$(QUIET_XGETTEXT)$(XGETTEXT) -o$@+ $(XGETTEXT_FLAGS_C) $(LOCALIZED_C)
$(QUIET_XGETTEXT)$(XGETTEXT) -o$@+ --join-existing $(XGETTEXT_FLAGS_SH) 
\
$(LOCALIZED_SH)
$(QUIET_XGETTEXT)$(XGETTEXT) -o$@+ --join-existing 
$(XGETTEXT_FLAGS_PERL) \
$(LOCALIZED_PERL)
+   @git reset --hard
mv $@+ $@
 
 .PHONY: pot

Re: [PATCH] PRItime: wrap PRItime for better l10n compatibility

[Junio C Hamano]

Johannes Schindelin  writes:

> But there may be hope. Since the character sequence "PRItime" is highly
> unlikely to occur in Git's source code in any context other than the
> format to print/parse timestamp_t, it should be possible to automate a the
> string replacement
>
>   git ls-files -z \*.[ch] |
>   xargs -0r sed -i 's/PRItime/PRIuMAX/g'
>
> (assuming, of course, that you use GNU sed, not BSD sed, for which the
> `-i` needs to read `-i ''` instead) as part of the update?

I somehow missed this bit.

Given that this needs to be done only once every release by only one
person (i.e. the l10n coordinator who updates *.pot file), as long
as the procedure is automated as much as possible to ease the pain
for the l10n coordinator and clearly described in the "Maintaining
the po/git.pot file" section of po/README, something along that line
does sound like a very tempting approach.  If it works well, it is
certainly much easier for normal developers than the other possible
alternatives I mentioned in my previous response.

It is brittle as you already said, but perhaps a bit of anchoring
like \, the brittle-ness may not hurt in practice.  

I didn't look at "git grep PRItime" output for hits, though.

Re: [PATCH] PRItime: wrap PRItime for better l10n compatibility

[Junio C Hamano]

Johannes Schindelin  writes:

>> Gettext handles macros such as PRIuMAX in commit 8b45c5df1 ("Add
>> support for ISO C 99  format string directive macros.",
>> 2002-07-23 12:33:13 +).
>
> Wow. This is ugly.
>
> If I understand correctly, then this will not even work correctly for
> PRIuMAX on Windows: ...

I think it is the other way around.  Without such a special-casing,
and producing message identifier that has "%" in it, they
cannot get a .po source that is usable across platforms.

Imagine a hypothetical xgettext that does not have the special case,
but just does what CPP does.  Running it on a platform where
uintmax_t is "unsigned long" would have "%lu" in resulting .po for a
message that uses "%" + PRIuMAX.  But the same source compiled on a
platform where uintmax_t is larger would pass "%llu" in calls to
_(...) it makes; such a string will not be found at runtime in the
corresponding .mo file, because you started with "%lu" in the .po
file.  By leaving a special marker % in the .po file by
special casing, the toolchain can make sure that the actual
parameter given to a _(...) can be found at runtime in .mo file that
was produced by compiling the .po file for the target platform.

So our own PRItime was a good idea for maintainability's point of
view of _our_ code, but it was not very friendly to i18n.

I can see two possibly usable approaches to make it i18n-friendly
while retaining our ability to later change the underlying type of
timestamp_t.  But neither is very pretty.

 - One is what was in Jiang's earlier proposal.

 - Another is to replace PRItime with PRIuMAX _but_ leave a comment
   to tell us that it wanted to be PRItime, so that we can later
   "git grep" for such a comment if/when we want to update the type
   of timestamp_t.

Re: [PATCH] PRItime: wrap PRItime for better l10n compatibility

[Johannes Schindelin]

Hi Jian (or is it Xin?),

On Wed, 19 Jul 2017, Jiang Xin wrote:

> 2017-07-19 1:35 GMT+08:00 Junio C Hamano :
> > Jiang Xin  writes:
> >
> >>> Two potential issues are:
> >>>
> >>>  - After this patch, there still are quite a many
> >>>
> >>> printf("time is %"PRItime" ...\n", timestamp)
> >>>
> >>>so the burden on the programmers having to remember when it is
> >>>required to use format_raw_time() becomes unclear, and makes the
> >>>change/churn larger when an existing message needs to be marked
> >>>for translation.
> >>>
> >>>  - The static struct strbuf here is a cheap way to avoid leaks, but
> >>>at the same time it is unfriendly to threaded code.  We could
> >>>instead do:
> >>>
> >>> void append_PRItime(struct strbuf *buf, timestamp_t time);
> >>>
> >>>to fix that trivially, but the damage to the caller obviously is
> >>>much larger going this way.
> >>>
> >>
> >> I wonder if we can replace the original %lu for timestamp with PRIuMAX
> >> instead.  PRIuMAX works fine with gettext utils.
> >
> > I think the question can better be answered if we know how gettext
> > tools special case PRIuMAX.  One thing that may be problematic is
> > that timestamp can later become a signed type and use of one level
> > of redirection in the current code via PRItime and via timestamp_t
> > is a good way to keep such a transition much easier.  Reverting it
> > to use PRIuMAX would make such a transition much harder.
> 
> Gettext handles macros such as PRIuMAX in commit 8b45c5df1 ("Add
> support for ISO C 99  format string directive macros.",
> 2002-07-23 12:33:13 +).

Wow. This is ugly.

If I understand correctly, then this will not even work correctly for
PRIuMAX on Windows: I highly doubt that the gettext library will interpret
%I64u in the format string correctly to be what % in the po file
refers to.

But there may be hope. Since the character sequence "PRItime" is highly
unlikely to occur in Git's source code in any context other than the
format to print/parse timestamp_t, it should be possible to automate a the
string replacement

git ls-files -z \*.[ch] |
xargs -0r sed -i 's/PRItime/PRIuMAX/g'

(assuming, of course, that you use GNU sed, not BSD sed, for which the
`-i` needs to read `-i ''` instead) as part of the update?

For all the reasons Junio mentioned, I, too, would be reluctant to change
the source code to cull all of the PRItime mentions, as it is pleasing
from a semantic point of view that we know what the heck we are talking
about here.

BTW *thank you so much* for your Herculean effort to keep going with the
translation.

Ciao,
Dscho

Re: [PATCH] PRItime: wrap PRItime for better l10n compatibility

[Jiang Xin]

2017-07-19 1:35 GMT+08:00 Junio C Hamano :
> Jiang Xin  writes:
>
>>> Two potential issues are:
>>>
>>>  - After this patch, there still are quite a many
>>>
>>> printf("time is %"PRItime" ...\n", timestamp)
>>>
>>>so the burden on the programmers having to remember when it is
>>>required to use format_raw_time() becomes unclear, and makes the
>>>change/churn larger when an existing message needs to be marked
>>>for translation.
>>>
>>>  - The static struct strbuf here is a cheap way to avoid leaks, but
>>>at the same time it is unfriendly to threaded code.  We could
>>>instead do:
>>>
>>> void append_PRItime(struct strbuf *buf, timestamp_t time);
>>>
>>>to fix that trivially, but the damage to the caller obviously is
>>>much larger going this way.
>>>
>>
>> I wonder if we can replace the original %lu for timestamp with PRIuMAX
>> instead.  PRIuMAX works fine with gettext utils.
>
> I think the question can better be answered if we know how gettext
> tools special case PRIuMAX.  One thing that may be problematic is
> that timestamp can later become a signed type and use of one level
> of redirection in the current code via PRItime and via timestamp_t
> is a good way to keep such a transition much easier.  Reverting it
> to use PRIuMAX would make such a transition much harder.

Gettext handles macros such as PRIuMAX in commit 8b45c5df1 ("Add
support for ISO C 99  format string directive macros.",
2002-07-23 12:33:13 +).

Macros such as PRIuMAX are hard coded, and we can not hack gettext
easily and directly.

For example, some code in commit 8b45c5df1 (see: See:
http://git.savannah.gnu.org/cgit/gettext.git/commit/?id=8b45c5df18e1976a1275297db73978bfe6144c7e):

+/* 8a. Convert ISO C 99 section 7.8.1 format string directives to string
+   literal placeholders.  */
+
+/* Test for an ISO C 99 section 7.8.1 format string directive.  */
+static bool
+is_inttypes_macro (name)
+ const char *name;
+{
+  /* Syntax:
+ P R I { d | i | o | u | x | X }
+ { { | LEAST | FAST } { 8 | 16 | 32 | 64 } | MAX | PTR }  */
+  if (name[0] == 'P' && name[1] == 'R' && name[2] == 'I')
+{
+  name += 3;
+  if (name[0] == 'd' || name[0] == 'i' || name[0] == 'o' ||
name[0] == 'u'
+  || name[0] == 'x' || name[0] == 'X')
+{
+  name += 1;
+  if (name[0] == 'M' && name[1] == 'A' && name[2] == 'X'
+  && name[3] == '\0')
+return true;
+  if (name[0] == 'P' && name[1] == 'T' && name[2] == 'R'
+  && name[3] == '\0')
+return true;
+  if (name[0] == 'L' && name[1] == 'E' && name[2] == 'A'
+  && name[3] == 'S' && name[4] == 'T')
+name += 5;
+  else if (name[0] == 'F' && name[1] == 'A' && name[2] == 'S'
+   && name[3] == 'T')
+name += 4;
+  if (name[0] == '8' && name[1] == '\0')
+return true;
+  if (name[0] == '1' && name[1] == '6' && name[2] == '\0')
+return true;
+  if (name[0] == '3' && name[1] == '2' && name[2] == '\0')
+return true;
+  if (name[0] == '6' && name[1] == '4' && name[2] == '\0')
+return true;
+}
+}
+  return false;
+}


+ if (*format == '<')
{
- if (*format == 'h')
+ spec.c99_directives =
+   (const char **)
+   xrealloc (spec.c99_directives,
+ 2 * (spec.c99_directives_count + 1)
+ * sizeof (const char *));
+ spec.c99_directives[2 * spec.c99_directives_count] = format;
+
+ format++;
+ /* Parse ISO C 99 section 7.8.1 format string directive.
+Syntax:
+P R I { d | i | o | u | x | X }
+{ { | LEAST | FAST } { 8 | 16 | 32 | 64 } | MAX | PTR }  */
+ if (*format != 'P')
+   goto bad_format;
+ format++;
+ if (*format != 'R')
+   goto bad_format;
+ format++;
+ if (*format != 'I')
+   goto bad_format;
+ format++;
+
+ switch (*format)
+   {
+   case 'i': case 'd':
+ type = FAT_INTEGER;
+ break;
+   case 'u': case 'o': case 'x': case 'X':
+ type = FAT_INTEGER | FAT_UNSIGNED;
+ break;
+   default:
+ goto bad_format;
+   }
+ format++;
+
+ if (format[0] == 'M' && format[1] == 'A' && format[2] == 'X')
+   {
+ type |= FAT_SIZE_INTMAX_T;
+ format += 3;
+   }
+ else if (format[0] == 'P' && format[1] == 'T' && format[2] == 'R')

-- 
Jiang Xin

Re: [PATCH] PRItime: wrap PRItime for better l10n compatibility

[Junio C Hamano]

Jiang Xin  writes:

>> Two potential issues are:
>>
>>  - After this patch, there still are quite a many
>>
>> printf("time is %"PRItime" ...\n", timestamp)
>>
>>so the burden on the programmers having to remember when it is
>>required to use format_raw_time() becomes unclear, and makes the
>>change/churn larger when an existing message needs to be marked
>>for translation.
>>
>>  - The static struct strbuf here is a cheap way to avoid leaks, but
>>at the same time it is unfriendly to threaded code.  We could
>>instead do:
>>
>> void append_PRItime(struct strbuf *buf, timestamp_t time);
>>
>>to fix that trivially, but the damage to the caller obviously is
>>much larger going this way.
>>
>
> I wonder if we can replace the original %lu for timestamp with PRIuMAX
> instead.  PRIuMAX works fine with gettext utils.

I think the question can better be answered if we know how gettext
tools special case PRIuMAX.  One thing that may be problematic is
that timestamp can later become a signed type and use of one level
of redirection in the current code via PRItime and via timestamp_t
is a good way to keep such a transition much easier.  Reverting it
to use PRIuMAX would make such a transition much harder.

Re: [PATCH] PRItime: wrap PRItime for better l10n compatibility

[Jiang Xin]

2017-07-18 1:10 GMT+08:00 Junio C Hamano :
> Jiang Xin  writes:
>
>> Commit cb71f8bdb5 ("PRItime: introduce a new "printf format" for
>> timestamps", 2017-04-21) does not play well with i18n framework. The
>> static string concatenation cannot be correctly interpreted by
>> gettext utilities, such as xgettext.
>>
>> Wrap PRItime in format_raw_time() function, so that we can extract
>> correct l10n messages into "po/git.pot".
>>
>> Reported-by: Jean-Noël Avila 
>> Signed-off-by: Jiang Xin 
>> ...
>> @@ -191,6 +200,15 @@ struct date_mode *date_mode_from_type(enum 
>> date_mode_type type)
>>   return 
>>  }
>>
>> +const char *format_raw_time(timestamp_t time)
>> +{
>> + static struct strbuf time_buf = STRBUF_INIT;
>> +
>> + strbuf_reset(_buf);
>> + strbuf_addf(_buf, "%"PRItime, time);
>> + return time_buf.buf;
>> +}
>
> Hmm.
>
> Two potential issues are:
>
>  - After this patch, there still are quite a many
>
> printf("time is %"PRItime" ...\n", timestamp)
>
>so the burden on the programmers having to remember when it is
>required to use format_raw_time() becomes unclear, and makes the
>change/churn larger when an existing message needs to be marked
>for translation.
>
>  - The static struct strbuf here is a cheap way to avoid leaks, but
>at the same time it is unfriendly to threaded code.  We could
>instead do:
>
> void append_PRItime(struct strbuf *buf, timestamp_t time);
>
>to fix that trivially, but the damage to the caller obviously is
>much larger going this way.
>

I wonder if we can replace the original %lu for timestamp with PRIuMAX
instead.  PRIuMAX works fine with gettext utils.

-- 
Jiang Xin

Re: [PATCH] PRItime: wrap PRItime for better l10n compatibility

[Junio C Hamano]

Jiang Xin  writes:

> Commit cb71f8bdb5 ("PRItime: introduce a new "printf format" for
> timestamps", 2017-04-21) does not play well with i18n framework. The
> static string concatenation cannot be correctly interpreted by
> gettext utilities, such as xgettext.
>
> Wrap PRItime in format_raw_time() function, so that we can extract
> correct l10n messages into "po/git.pot".
>
> Reported-by: Jean-Noël Avila 
> Signed-off-by: Jiang Xin 
> ...
> @@ -191,6 +200,15 @@ struct date_mode *date_mode_from_type(enum 
> date_mode_type type)
>   return 
>  }
>  
> +const char *format_raw_time(timestamp_t time)
> +{
> + static struct strbuf time_buf = STRBUF_INIT;
> +
> + strbuf_reset(_buf);
> + strbuf_addf(_buf, "%"PRItime, time);
> + return time_buf.buf;
> +}

Hmm.

Two potential issues are:

 - After this patch, there still are quite a many 

printf("time is %"PRItime" ...\n", timestamp)

   so the burden on the programmers having to remember when it is
   required to use format_raw_time() becomes unclear, and makes the
   change/churn larger when an existing message needs to be marked
   for translation.

 - The static struct strbuf here is a cheap way to avoid leaks, but
   at the same time it is unfriendly to threaded code.  We could
   instead do:

void append_PRItime(struct strbuf *buf, timestamp_t time);

   to fix that trivially, but the damage to the caller obviously is
   much larger going this way.

[PATCH] PRItime: wrap PRItime for better l10n compatibility

[Jiang Xin]

Commit cb71f8bdb5 ("PRItime: introduce a new "printf format" for
timestamps", 2017-04-21) does not play well with i18n framework. The
static string concatenation cannot be correctly interpreted by
gettext utilities, such as xgettext.

Wrap PRItime in format_raw_time() function, so that we can extract
correct l10n messages into "po/git.pot".

Reported-by: Jean-Noël Avila 
Signed-off-by: Jiang Xin 
---
 archive-zip.c  |  4 ++--
 builtin/blame.c|  4 ++--
 builtin/fsck.c |  3 ++-
 builtin/log.c  |  4 ++--
 builtin/receive-pack.c |  4 ++--
 builtin/rev-parse.c|  2 +-
 cache.h|  1 +
 date.c | 63 --
 fetch-pack.c   |  2 +-
 refs/files-backend.c   |  6 ++---
 upload-pack.c  |  2 +-
 vcs-svn/fast_export.c  |  8 ---
 12 files changed, 63 insertions(+), 40 deletions(-)

diff --git a/archive-zip.c b/archive-zip.c
index e8913e5a26..92df24815e 100644
--- a/archive-zip.c
+++ b/archive-zip.c
@@ -600,8 +600,8 @@ static void dos_time(timestamp_t *timestamp, int *dos_date, 
int *dos_time)
struct tm *t;
 
if (date_overflows(*timestamp))
-   die("timestamp too large for this system: %"PRItime,
-   *timestamp);
+   die("timestamp too large for this system: %s",
+   format_raw_time(*timestamp));
time = (time_t)*timestamp;
t = localtime();
*timestamp = time;
diff --git a/builtin/blame.c b/builtin/blame.c
index bda1a78726..97bd99dd51 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -245,11 +245,11 @@ static int emit_one_suspect_detail(struct blame_origin 
*suspect, int repeat)
get_commit_info(suspect->commit, , 1);
printf("author %s\n", ci.author.buf);
printf("author-mail %s\n", ci.author_mail.buf);
-   printf("author-time %"PRItime"\n", ci.author_time);
+   printf("author-time %s\n", format_raw_time(ci.author_time));
printf("author-tz %s\n", ci.author_tz.buf);
printf("committer %s\n", ci.committer.buf);
printf("committer-mail %s\n", ci.committer_mail.buf);
-   printf("committer-time %"PRItime"\n", ci.committer_time);
+   printf("committer-time %s\n", format_raw_time(ci.committer_time));
printf("committer-tz %s\n", ci.committer_tz.buf);
printf("summary %s\n", ci.summary.buf);
if (suspect->commit->object.flags & UNINTERESTING)
diff --git a/builtin/fsck.c b/builtin/fsck.c
index 99dea7adf6..1f6fc674c3 100644
--- a/builtin/fsck.c
+++ b/builtin/fsck.c
@@ -407,7 +407,8 @@ static void fsck_handle_reflog_oid(const char *refname, 
struct object_id *oid,
if (timestamp && name_objects)
add_decoration(fsck_walk_options.object_names,
obj,
-   xstrfmt("%s@{%"PRItime"}", refname, 
timestamp));
+   xstrfmt("%s@{%s}", refname,
+   format_raw_time(timestamp)));
obj->used = 1;
mark_object_reachable(obj);
} else {
diff --git a/builtin/log.c b/builtin/log.c
index c6362cf92e..83b303f8af 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -920,8 +920,8 @@ static void get_patch_ids(struct rev_info *rev, struct 
patch_ids *ids)
 static void gen_message_id(struct rev_info *info, char *base)
 {
struct strbuf buf = STRBUF_INIT;
-   strbuf_addf(, "%s.%"PRItime".git.%s", base,
-   (timestamp_t) time(NULL),
+   strbuf_addf(, "%s.%s.git.%s", base,
+   format_raw_time((timestamp_t) time(NULL)),

git_committer_info(IDENT_NO_NAME|IDENT_NO_DATE|IDENT_STRICT));
info->message_id = strbuf_detach(, NULL);
 }
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index cabdc55e09..75f26a1bc5 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -460,12 +460,12 @@ static char *prepare_push_cert_nonce(const char *path, 
timestamp_t stamp)
struct strbuf buf = STRBUF_INIT;
unsigned char sha1[20];
 
-   strbuf_addf(, "%s:%"PRItime, path, stamp);
+   strbuf_addf(, "%s:%s", path, format_raw_time(stamp));
hmac_sha1(sha1, buf.buf, buf.len, cert_nonce_seed, 
strlen(cert_nonce_seed));;
strbuf_release();
 
/* RFC 2104 5. HMAC-SHA1-80 */
-   strbuf_addf(, "%"PRItime"-%.*s", stamp, 20, sha1_to_hex(sha1));
+   strbuf_addf(, "%s-%.*s", format_raw_time(stamp), 20, 
sha1_to_hex(sha1));
return strbuf_detach(, NULL);
 }
 
diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c
index c78b7b33d6..c55872196f 100644
--- a/builtin/rev-parse.c
+++ b/builtin/rev-parse.c
@@ -219,7 +219,7 @@ static void show_datestring(const char *flag, const char 
*datestr)
/* date handling requires both flags and revs