Re: [PATCH v2 1/9] Makefile: sort "po/git.pot" by file location

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

On Thu, May 19 2022, Jiang Xin wrote:

> From: Jiang Xin <zhiyou.jx@alibaba-inc.com>
>
> Before feeding xgettext with more C souce files which may be ignored
> by various compiler conditions, add new option "--sort-by-file" to
> xgettext program to create stable message template file "po/git.pot".
>
> With this update, the newly generated "po/git.pot" will has the same
> entries while in a different order. We won't checkin the newly generated
> "po/git.pot", because we will remove it from tree in a later commit.
>
> Signed-off-by: Jiang Xin <zhiyou.jx@alibaba-inc.com>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
>  Makefile | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/Makefile b/Makefile
> index f8bccfab5e..83e968e2a4 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -2706,6 +2706,7 @@ XGETTEXT_FLAGS = \
>  	--force-po \
>  	--add-comments=TRANSLATORS: \
>  	--msgid-bugs-address="Git Mailing List <git@vger.kernel.org>" \
> +	--sort-by-file \
>  	--from-code=UTF-8
>  XGETTEXT_FLAGS_C = $(XGETTEXT_FLAGS) --language=C \
>  	--keyword=_ --keyword=N_ --keyword="Q_:1,2"

I'm not opposed to this change, but between this and 2/9 I'm still
unsure what the aim is exactly, and if the results are desired.

In 2/9 you correctly point out that we have messages we've missed due to
LOCALIZED_C being platform-specific.

That should never happen (although your implementation in 2/9 may have
some small issues, I'll reply there separately), i.e. we should always
have "make pot" generate the same po/git.pot from the same commit
whether you're on linux, mac os x etc.

But AFAICT we have a "stable" sort order now, it's in whatever order we
feed the files to xgettext, which ultimately comes down to e.g. the list
of $(LIB_OBJS) in the Makefile.

I've been looking over the libintl documentation to see what exactly
these sort options do, and how they differ from the default, and it's
not really described.

AFAICT the xgettext behavior we have now is that we'll process the files
we have in order, and for those files extract the messages we have as we
see them.

One fringe benefit of that is that e.g. "make pot
XGETTEXT_INCLUDE_TESTS=Y" (which I think I've only ever used, and
probably only ~10 years ago) will get added at the end, but now it'll be
added wherever t/t0200 sorts.

Then because compose the builtin objs and lib objs by concatenation, but
don't $(sort) them in the Makefile most of this change is due to us
e.g. sorting builtin/* before parse-options.c or whatever.

But oddly we also have cases like this:

        strbuf_addf(&header, print_file_item_data.modified_fmt,
                    _("staged"), _("unstaged"), _("path"));

Before this we'd list those in that order in the git.pot, but now
because of --sort-by-file we'll list any messages on the same line in
sorted msgid order, not in the orderd they appear in. Another example is
e.g. this in builtin/blame.c:

    OPT_STRING_LIST(0, "ignore-rev", &ignore_rev_list, N_("rev"), N_("ignore <rev> when blaming"))

Where before we'd list them in that order, but now it's with "ignore.."
before "rev".

I think this change would be easier to follow & explain if you first
made this change:
	
	diff --git a/Makefile b/Makefile
	index 61aadf3ce88..3726fe8064a 100644
	--- a/Makefile
	+++ b/Makefile
	@@ -2715,10 +2715,9 @@ XGETTEXT_FLAGS_SH = $(XGETTEXT_FLAGS) --language=Shell \
	 	--keyword=gettextln --keyword=eval_gettextln
	 XGETTEXT_FLAGS_PERL = $(XGETTEXT_FLAGS) --language=Perl \
	 	--keyword=__ --keyword=N__ --keyword="__n:1,2"
	-LOCALIZED_C = $(C_OBJ:o=c) $(LIB_H) $(GENERATED_H)
	-LOCALIZED_SH = $(SCRIPT_SH)
	-LOCALIZED_SH += git-sh-setup.sh
	-LOCALIZED_PERL = $(SCRIPT_PERL)
	+LOCALIZED_C = $(sort $(C_OBJ:o=c) $(LIB_H) $(GENERATED_H))
	+LOCALIZED_SH = $(sort $(SCRIPT_SH) git-sh-setup.sh)
	+LOCALIZED_PERL = $(sort $(SCRIPT_PERL))
	 
	 ifdef XGETTEXT_INCLUDE_TESTS
	 LOCALIZED_C += t/t0200/test.c

Which would sort things within C, SH and Perl files (but not among
them). Then this change would AFAICT only:

 * Change that "within one line" sort order, as noted above
 * Sort across C/SH/Perl.

I'm mostly "meh" on the result, but it's also because I genuinely don't
get what the goal was. Is it because in 2/9 you'll end up using
$(FOUND_C_SOURCES), which we derive from either "git ls-files" or
"find", the latter of which has an unstable sort order?

Even if that's the case a smaller change is to just use $(sort) as noted
above. I like that a tiny bit more because then the "--join-existing" we
do for SH/Perl is an append, whereas now we'll re-sort the whole thing.

I also don't think it matters much either way, so whatever you're happy
with, the above all came from trying to follow along...