Re: [PATCH v6 2/5] rebase -i: support --committer-date-is-author-date

[Phillip Wood <phillip.wood123@xxxxxxxxx>]

Hi Danh

On 16/07/2020 14:06, Đoàn Trần Công Danh wrote:
> Hi Phillip,
>
> On 2020-07-16 09:23:17+0100, Phillip Wood <phillip.wood123@xxxxxxxxx> wrote:
> > > > [...]
> > > > +	if (opts->committer_date_is_author_date) {
> > > > +		struct ident_split ident;
> > > > +		struct strbuf date = STRBUF_INIT;
> > > > +
> > > > +		if (split_ident_line(&ident, author, (int)strlen(author)) < 0) {
> > > > +			res = error(_("malformed ident line '%s'"), author);
> > >
> > > I've checked with the translation for my native language (vi).
> > > The translators seem to misread ident (as in identity) as
> > > indent (as in indentation).
> > >
> > > The translation in po/vi.po:25045 (of v2.28.0-rc0) reads:
> > >
> > > 	#~ msgid "malformed ident line"
> > > 	#~ msgstr "thụt đầu dòng dị hình"
> > >
> > > Translating back to English, it reads: "malformed indentation".
> > >
> > > Hence, I think it would read better if we write:
> > >
> > > 	res = error(_("malformed identity line '%s'"), author);
> > >
> > > 3 more characters is not that much :)
> >
> > Looking through the existing strings "invalid ident line: %.*s" is already
> > used by am so maybe we should reuse that (log.c also contains "invalid ident
> > line: %s"). Is the translation correct?
> >
> > #: builtin/am.c:1270
> > #, c-format
> > msgid "invalid ident line: %.*s"
> > msgstr "dòng thụt lề không hợp lệ: %.*s"
>
> That translation isn't correct either.
> It seems like it's recurring pattern.
> I'll take it to the Vietnamese translation team.
>
> Anyway, I've checked with other translation that I can understand
> in part. I think
>
> 	invalid ident line: %s
>
> is better candidate for the message.

Yes I realized after sending my email that the string we're printing is 
NUL terminated so we don't need to specify the length with '*'. I think 
the best thing would be to change the message in this patch to 'invalid 
ident line: %s' and then have a follow up after this is merged to change 
all the "invalid ident line" messages to use "identity" instead. Would 
you be interested in taking on the follow up patch?

Best Wishes

Phillip

> Since Spanish translation also mis-translates the message:
>
> 	es.po:9836:msgid "invalid ident line: %.*s"
> 	es.po-9837-msgstr "sangría no válida: %.*s"
>
> "sangría" also means "indentation" in this context.
>
> Thanks,
> -Danh
>
> > Best Wishes
> >
> > Phillip
> >
> > > > +			goto out;
> > > > +		}
> > > > +		if (!ident.date_begin) {
> > > > +			res = error(_("corrupted author without date information"));
> > > > +			goto out;
> > > > +		}
> > > > +
> > > > +		strbuf_addf(&date, "@%.*s %.*s",
> > > > +			    (int)(ident.date_end - ident.date_begin),
> > > > +			    ident.date_begin,
> > > > +			    (int)(ident.tz_end - ident.tz_begin),
> > > > +			    ident.tz_begin);
> > > > +		res = setenv("GIT_COMMITTER_DATE", date.buf, 1);
> > > > +		strbuf_release(&date);
> > > > +
> > > > +		if (res)
> > > > +			goto out;
> > > > +	}
> > > > +
> > > >    	if (write_index_as_tree(&tree, r->index, r->index_file, 0, NULL)) {
> > > >    		res = error(_("git write-tree failed to write a tree"));
> > > >    		goto out;
> > > > @@ -2532,6 +2578,11 @@ static int read_populate_opts(struct replay_opts *opts)
> > > >    			opts->signoff = 1;
> > > >    		}
> > > > +		if (file_exists(rebase_path_cdate_is_adate())) {
> > > > +			opts->allow_ff = 0;
> > > > +			opts->committer_date_is_author_date = 1;
> > > > +		}
> > > > +
> > > >    		if (file_exists(rebase_path_reschedule_failed_exec()))
> > > >    			opts->reschedule_failed_exec = 1;
> > > > @@ -2622,6 +2673,8 @@ int write_basic_state(struct replay_opts *opts, const char *head_name,
> > > >    		write_file(rebase_path_drop_redundant_commits(), "%s", "");
> > > >    	if (opts->keep_redundant_commits)
> > > >    		write_file(rebase_path_keep_redundant_commits(), "%s", "");
> > > > +	if (opts->committer_date_is_author_date)
> > > > +		write_file(rebase_path_cdate_is_adate(), "%s", "");
> > > >    	if (opts->reschedule_failed_exec)
> > > >    		write_file(rebase_path_reschedule_failed_exec(), "%s", "");
> > > > @@ -3542,6 +3595,10 @@ static int do_merge(struct repository *r,
> > > >    			goto leave_merge;
> > > >    		}
> > > > +		if (opts->committer_date_is_author_date)
> > > > +			argv_array_pushf(&cmd.env_array, "GIT_COMMITTER_DATE=%s",
> > > > +					 author_date_from_env_array(&cmd.env_array));
> > > > +
> > > >    		cmd.git_cmd = 1;
> > > >    		argv_array_push(&cmd.args, "merge");
> > > >    		argv_array_push(&cmd.args, "-s");
> > > > @@ -3819,7 +3876,8 @@ static int pick_commits(struct repository *r,
> > > >    	setenv(GIT_REFLOG_ACTION, action_name(opts), 0);
> > > >    	if (opts->allow_ff)
> > > >    		assert(!(opts->signoff || opts->no_commit ||
> > > > -				opts->record_origin || opts->edit));
> > > > +				opts->record_origin || opts->edit ||
> > > > +				opts->committer_date_is_author_date));
> > > >    	if (read_and_refresh_cache(r, opts))
> > > >    		return -1;
> > > > diff --git a/sequencer.h b/sequencer.h
> > > > index 0bee85093e..4ab94119ae 100644
> > > > --- a/sequencer.h
> > > > +++ b/sequencer.h
> > > > @@ -45,6 +45,7 @@ struct replay_opts {
> > > >    	int verbose;
> > > >    	int quiet;
> > > >    	int reschedule_failed_exec;
> > > > +	int committer_date_is_author_date;
> > > >    	int mainline;
> > > > diff --git a/t/t3422-rebase-incompatible-options.sh b/t/t3422-rebase-incompatible-options.sh
> > > > index 55ca46786d..c8234062c6 100755
> > > > --- a/t/t3422-rebase-incompatible-options.sh
> > > > +++ b/t/t3422-rebase-incompatible-options.sh
> > > > @@ -61,7 +61,6 @@ test_rebase_am_only () {
> > > >    }
> > > >    test_rebase_am_only --whitespace=fix
> > > > -test_rebase_am_only --committer-date-is-author-date
> > > >    test_rebase_am_only -C4
> > > >    test_expect_success REBASE_P '--preserve-merges incompatible with --signoff' '
> > > > diff --git a/t/t3436-rebase-more-options.sh b/t/t3436-rebase-more-options.sh
> > > > index 4f8a6e51c9..50a63d8ebe 100755
> > > > --- a/t/t3436-rebase-more-options.sh
> > > > +++ b/t/t3436-rebase-more-options.sh
> > > > @@ -9,6 +9,9 @@ test_description='tests to ensure compatibility between am and interactive backe
> > > >    . "$TEST_DIRECTORY"/lib-rebase.sh
> > > > +GIT_AUTHOR_DATE="1999-04-02T08:03:20+05:30"
> > > > +export GIT_AUTHOR_DATE
> > > > +
> > > >    # This is a special case in which both am and interactive backends
> > > >    # provide the same output. It was done intentionally because
> > > >    # both the backends fall short of optimal behaviour.
> > > > @@ -21,11 +24,20 @@ test_expect_success 'setup' '
> > > >    	test_write_lines "line 1" "new line 2" "line 3" >file &&
> > > >    	git commit -am "update file" &&
> > > >    	git tag side &&
> > > > +	test_commit commit1 foo foo1 &&
> > > > +	test_commit commit2 foo foo2 &&
> > > > +	test_commit commit3 foo foo3 &&
> > > >    	git checkout --orphan master &&
> > > > +	rm foo &&
> > > >    	test_write_lines "line 1" "        line 2" "line 3" >file &&
> > > >    	git commit -am "add file" &&
> > > > -	git tag main
> > > > +	git tag main &&
> > > > +
> > > > +	mkdir test-bin &&
> > > > +	write_script test-bin/git-merge-test <<-\EOF
> > > > +	exec git-merge-recursive "$@"
> > > > +	EOF
> > > >    '
> > > >    test_expect_success '--ignore-whitespace works with apply backend' '
> > > > @@ -52,6 +64,50 @@ test_expect_success '--ignore-whitespace is remembered when continuing' '
> > > >    	git diff --exit-code side
> > > >    '
> > > > +test_ctime_is_atime () {
> > > > +	git log $1 --format=%ai >authortime &&
> > > > +	git log $1 --format=%ci >committertime &&
> > > > +	test_cmp authortime committertime
> > > > +}
> > > > +
> > > > +test_expect_success '--committer-date-is-author-date works with apply backend' '
> > > > +	GIT_AUTHOR_DATE="@1234 +0300" git commit --amend --reset-author &&
> > > > +	git rebase --apply --committer-date-is-author-date HEAD^ &&
> > > > +	test_ctime_is_atime -1
> > > > +'
> > > > +
> > > > +test_expect_success '--committer-date-is-author-date works with merge backend' '
> > > > +	GIT_AUTHOR_DATE="@1234 +0300" git commit --amend --reset-author &&
> > > > +	git rebase -m --committer-date-is-author-date HEAD^ &&
> > > > +	test_ctime_is_atime -1
> > > > +'
> > > > +
> > > > +test_expect_success '--committer-date-is-author-date works with rebase -r' '
> > > > +	git checkout side &&
> > > > +	GIT_AUTHOR_DATE="@1234 +0300" git merge --no-ff commit3 &&
> > > > +	git rebase -r --root --committer-date-is-author-date &&
> > > > +	test_ctime_is_atime
> > > > +'
> > > > +
> > > > +test_expect_success '--committer-date-is-author-date works when forking merge' '
> > > > +	git checkout side &&
> > > > +	GIT_AUTHOR_DATE="@1234 +0300" git merge --no-ff commit3 &&
> > > > +	PATH="./test-bin:$PATH" git rebase -r --root --strategy=test \
> > > > +					--committer-date-is-author-date &&
> > > > +	test_ctime_is_atime
> > > > +'
> > > > +
> > > > +test_expect_success '--committer-date-is-author-date works when committing conflict resolution' '
> > > > +	git checkout commit2 &&
> > > > +	GIT_AUTHOR_DATE="@1980 +0000" git commit --amend --only --reset-author &&
> > > > +	test_must_fail git rebase -m --committer-date-is-author-date \
> > > > +		--onto HEAD^^ HEAD^ &&
> > > > +	echo resolved > foo &&
> > >
> > > Nitpick: no space after ">" :D