Re: [PATCH v2 01/23] Update messages in preparation for i18n
On Thu, Jul 19, 2018 at 8:18 PM Junio C Hamano wrote:
> > --- a/config.c
> > +++ b/config.c
> > @@ -461,7 +461,7 @@ int git_config_from_parameters(config_fn_t fn, void
> > *data)
> > envw = xstrdup(env);
> >
> > if (sq_dequote_to_argv(envw, &argv, &nr, &alloc) < 0) {
> > - ret = error("bogus format in " CONFIG_DATA_ENVIRONMENT);
> > + ret = error("bogus format in %s", CONFIG_DATA_ENVIRONMENT);
> > goto out;
> > }
> >
>
> Good job spotting that the original wanted to say, but failed to
> say, that CONFIG_DATA_ENVIRONMENT as the source of broken data we
> detected. But I am not sure CONFIG_DATA_ENVIRONMENT is what we want
> to report as the source of bad data to the end users. One-shot
> configuration we get form "git -c VAR=VAL" are placed in the
> environment as an internal implementation detail, so from their
> point of view, the place we saw broken data coming from is their
> command line "git -c VAR=VAL" one-shot configuration.
I think I'll leave this one out for future follow up. The change here
is needed because otherwise we can't wrap the string in _(). This
patch, as you noted, is already big and getting more complicated than
it should be.
--
Duy
Re: [PATCH v2 01/23] Update messages in preparation for i18n
Junio C Hamano writes:
>> @@ -622,7 +626,7 @@ int cmd_config(int argc, const char **argv, const char
>> *prefix)
>> * location; error out even if XDG_CONFIG_HOME
>> * is set and points at a sane location.
>> */
>> -die("$HOME not set");
>> +die("$HOME is not set");
>
> Meh. There are many verb-less messages e.g. "only one X at a time"
> that are not given a new verb in this patch.
Just to save wasted work due to misunderstanding, this is merely
"Meh" (I do not think it is worth it, and I wouldn't have included
this hunk if I were doing the patch, but since the patch has already
been written, I do not mind having it, either), not "Reject" (drop
this and resend).
Thanks.
Re: [PATCH v2 01/23] Update messages in preparation for i18n
Nguyễn Thái Ngọc Duy writes:
> static void check_argc(int argc, int min, int max) {
> if (argc >= min && argc <= max)
> return;
> - error("wrong number of arguments");
> + if (min == max)
> + error("wrong number of arguments, should be %d", min);
> + else
> + error("wrong number of arguments, should be from %d to %d",
> + min, max);
> usage_with_options(builtin_config_usage, builtin_config_options);
> }
Good. This was the only instance of
> - some messages are improved to give more information
I spotted.
> @@ -622,7 +626,7 @@ int cmd_config(int argc, const char **argv, const char
> *prefix)
>* location; error out even if XDG_CONFIG_HOME
>* is set and points at a sane location.
>*/
> - die("$HOME not set");
> + die("$HOME is not set");
Meh. There are many verb-less messages e.g. "only one X at a time"
that are not given a new verb in this patch.
> @@ -819,7 +823,7 @@ int cmd_config(int argc, const char **argv, const char
> *prefix)
> if (ret < 0)
> return ret;
> if (ret == 0)
> - die("No such section!");
> + die("no such section: %s", argv[0]);
> }
> else if (actions == ACTION_REMOVE_SECTION) {
> int ret;
> @@ -830,7 +834,7 @@ int cmd_config(int argc, const char **argv, const char
> *prefix)
> if (ret < 0)
> return ret;
> if (ret == 0)
> - die("No such section!");
> + die("no such section: %s", argv[0]);
> }
There are check_argc() calls before these two hunks to ensure that
access to argv[0] is safe, so these are good.
> @@ -2638,7 +2638,7 @@ static void read_object_list_from_stdin(void)
> if (feof(stdin))
> break;
> if (!ferror(stdin))
> - die("fgets returned NULL, not EOF, not error!");
> + die("BUG: fgets returned NULL, not EOF, not
> error!");
This is not BUG("...") because it is not *our* bug but a bug in platform's
stdio?
> @@ -456,10 +456,10 @@ static int create_graft(int argc, const char **argv,
> int force, int gentle)
> return -1;
> }
>
> - if (remove_signature(&buf)) {
> - warning(_("the original commit '%s' has a gpg signature."),
> old_ref);
> - warning(_("the signature will be removed in the replacement
> commit!"));
> - }
> + if (remove_signature(&buf))
> + warning(_("the original commit '%s' has a gpg signature.\n"
> + "The signature will be removed in the replacement
> commit!"),
> + old_ref);
Unlike a compound sentence, for which translators may appreciate
that they can reorder the parts of it to suit their language, I do
not see why these two independent sentences should be placed in a
single warning().
Or is this primarily about avoiding repeated appearance of
"warning:" labels, i.e.
warning: sentence one
warning: sentence two
I am not sure if these belong to this "simple mechanical conversion
or otherwise uncontrovercial improvement" series.
> diff --git a/config.c b/config.c
> index f4a208a166..6ba07989f1 100644
> --- a/config.c
> +++ b/config.c
> @@ -461,7 +461,7 @@ int git_config_from_parameters(config_fn_t fn, void *data)
> envw = xstrdup(env);
>
> if (sq_dequote_to_argv(envw, &argv, &nr, &alloc) < 0) {
> - ret = error("bogus format in " CONFIG_DATA_ENVIRONMENT);
> + ret = error("bogus format in %s", CONFIG_DATA_ENVIRONMENT);
> goto out;
> }
>
Good job spotting that the original wanted to say, but failed to
say, that CONFIG_DATA_ENVIRONMENT as the source of broken data we
detected. But I am not sure CONFIG_DATA_ENVIRONMENT is what we want
to report as the source of bad data to the end users. One-shot
configuration we get form "git -c VAR=VAL" are placed in the
environment as an internal implementation detail, so from their
point of view, the place we saw broken data coming from is their
command line "git -c VAR=VAL" one-shot configuration.
> @@ -1409,11 +1409,11 @@ static int git_default_push_config(const char *var,
> const char *value)
> push_default = PUSH_DEFAULT_UPSTREAM;
> else if (!strcmp(value, "current"))
> push_default = PUSH_DEFAULT_CURRENT;
> - else {
> - error("malformed value for %s: %s", var, value);
> - return error("Must be one of nothing, matching, simple,
> "
> - "upstream or current.");
> - }
> + else
> + return error("malformed value fo
[PATCH v2 01/23] Update messages in preparation for i18n
Many messages will be marked for translation in the following
commits. This commit updates some of them to be more consistent and
reduce diff noise in those commits. Changes are
- keep the first letter of die(), error() and warning() in lowercase
- no full stop in die(), error() or warning() if it's single sentence
messages
- indentation
- some messages are turned to BUG(), or prefixed with "BUG:" and will
not be marked for i18n
- some messages are improved to give more information
- some messages are broken down by sentence to be i18n friendly
(on the same token, combine multiple warning() into one big string)
- the trailing \n is converted to printf_ln if possible, or deleted
if not redundant
- errno_errno() is used instead of explicit strerror()
Signed-off-by: Nguyễn Thái Ngọc Duy
---
archive-zip.c | 2 +-
builtin/blame.c | 2 +-
builtin/checkout.c| 4 +--
builtin/commit.c | 6 ++--
builtin/config.c | 20 +++--
builtin/fast-export.c | 42 +--
builtin/fmt-merge-msg.c | 2 +-
builtin/grep.c| 10 +++
builtin/log.c | 6 ++--
builtin/merge.c | 2 +-
builtin/pack-objects.c| 18 ++--
builtin/replace.c | 32 ++---
builtin/rm.c | 2 +-
config.c | 12
connect.c | 33 +++--
convert.c | 6 ++--
diff.c| 4 +--
dir.c | 4 +--
pkt-line.c| 2 +-
reflog-walk.c | 4 +--
refs.c| 12
refspec.c | 2 +-
sequencer.c | 8 +++---
sha1-file.c | 8 +++---
t/t1400-update-ref.sh | 8 +++---
t/t3005-ls-files-relative.sh | 4 +--
t/t5801-remote-helpers.sh | 6 ++--
t/t7063-status-untracked-cache.sh | 2 +-
transport-helper.c| 48 +++
transport.c | 8 +++---
30 files changed, 163 insertions(+), 156 deletions(-)
diff --git a/archive-zip.c b/archive-zip.c
index 74f3fe9103..48d843489c 100644
--- a/archive-zip.c
+++ b/archive-zip.c
@@ -309,7 +309,7 @@ static int write_zip_entry(struct archiver_args *args,
if (is_utf8(path))
flags |= ZIP_UTF8;
else
- warning("Path is not valid UTF-8: %s", path);
+ warning("path is not valid UTF-8: %s", path);
}
if (pathlen > 0x) {
diff --git a/builtin/blame.c b/builtin/blame.c
index 5a0388aaef..b77ba6ae82 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -408,7 +408,7 @@ static void parse_color_fields(const char *s)
}
if (next == EXPECT_COLOR)
- die (_("must end with a color"));
+ die(_("must end with a color"));
colorfield[colorfield_nr].hop = TIME_MAX;
string_list_clear(&l, 0);
diff --git a/builtin/checkout.c b/builtin/checkout.c
index 4c41d8b4c8..626868637a 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -1191,12 +1191,12 @@ int cmd_checkout(int argc, const char **argv, const
char *prefix)
if (opts.track != BRANCH_TRACK_UNSPECIFIED && !opts.new_branch) {
const char *argv0 = argv[0];
if (!argc || !strcmp(argv0, "--"))
- die (_("--track needs a branch name"));
+ die(_("--track needs a branch name"));
skip_prefix(argv0, "refs/", &argv0);
skip_prefix(argv0, "remotes/", &argv0);
argv0 = strchr(argv0, '/');
if (!argv0 || !argv0[1])
- die (_("Missing branch name; try -b"));
+ die(_("missing branch name; try -b"));
opts.new_branch = argv0 + 1;
}
diff --git a/builtin/commit.c b/builtin/commit.c
index 9bcbb0c25c..d3276a10b2 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -1647,9 +1647,9 @@ int cmd_commit(int argc, const char **argv, const char
*prefix)
unlink(git_path_squash_msg());
if (commit_index_files())
- die (_("Repository has been updated, but unable to write\n"
-"new_index file. Check that disk is not full and quota
is\n"
-"not exceeded, and then \"git reset HEAD\" to recover."));
+ die(_("repository has been updated, but unable to write\n"
+ "new_index file. Check that disk is not full and quota
is\n"
+ "not exceeded, and then \"git reset HEAD\" to recover."));
rerere(0);
run_command_v_opt(argv_gc_auto, RUN_GIT_CMD);
diff --git a/builtin/config.c b
[PATCH v2 01/23] Update messages in preparation for i18n
Many messages will be marked for translation in the following
commits. This commit updates some of them to be more consistent and
reduce diff noise in those commits. Changes are
- keep the first letter of die(), error() and warning() in lowercase
- no full stop in die(), error() or warning() if it's single sentence
messages
- indentation
- some messages are turned to BUG(), or prefixed with "BUG:" and will
not be marked for i18n
- some messages are improved to give more information
- some messages are broken down by sentence to be i18n friendly
- the trailing \n is converted to printf_ln if possible
- errno_errno() is used instead of explicit strerror()
Signed-off-by: Nguyễn Thái Ngọc Duy
---
archive-zip.c | 2 +-
builtin/config.c | 18 +++-
builtin/grep.c| 10 +++
builtin/pack-objects.c| 10 +++
builtin/replace.c | 22 +++---
config.c | 2 +-
connect.c | 33 +++--
convert.c | 6 ++--
dir.c | 4 +--
pkt-line.c| 2 +-
refs.c| 12
refspec.c | 2 +-
sequencer.c | 8 +++---
sha1-file.c | 8 +++---
t/t1400-update-ref.sh | 8 +++---
t/t3005-ls-files-relative.sh | 4 +--
t/t5801-remote-helpers.sh | 6 ++--
t/t7063-status-untracked-cache.sh | 2 +-
transport-helper.c| 48 +++
transport.c | 8 +++---
20 files changed, 111 insertions(+), 104 deletions(-)
diff --git a/archive-zip.c b/archive-zip.c
index 74f3fe9103..48d843489c 100644
--- a/archive-zip.c
+++ b/archive-zip.c
@@ -309,7 +309,7 @@ static int write_zip_entry(struct archiver_args *args,
if (is_utf8(path))
flags |= ZIP_UTF8;
else
- warning("Path is not valid UTF-8: %s", path);
+ warning("path is not valid UTF-8: %s", path);
}
if (pathlen > 0x) {
diff --git a/builtin/config.c b/builtin/config.c
index b29d26dede..ebeb4c5638 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -110,7 +110,7 @@ static int option_parse_type(const struct option *opt,
const char *arg,
* --int' and '--type=bool
* --type=int'.
*/
- error("only one type at a time.");
+ error("only one type at a time");
usage_with_options(builtin_config_usage,
builtin_config_options);
}
@@ -160,7 +160,11 @@ static struct option builtin_config_options[] = {
static void check_argc(int argc, int min, int max) {
if (argc >= min && argc <= max)
return;
- error("wrong number of arguments");
+ if (min == max)
+ error("wrong number of arguments, should be %d", min);
+ else
+ error("wrong number of arguments, should be from %d to %d",
+ min, max);
usage_with_options(builtin_config_usage, builtin_config_options);
}
@@ -595,7 +599,7 @@ int cmd_config(int argc, const char **argv, const char
*prefix)
if (use_global_config + use_system_config + use_local_config +
!!given_config_source.file + !!given_config_source.blob > 1) {
- error("only one config file at a time.");
+ error("only one config file at a time");
usage_with_options(builtin_config_usage,
builtin_config_options);
}
@@ -622,7 +626,7 @@ int cmd_config(int argc, const char **argv, const char
*prefix)
* location; error out even if XDG_CONFIG_HOME
* is set and points at a sane location.
*/
- die("$HOME not set");
+ die("$HOME is not set");
if (access_or_warn(user_config, R_OK, 0) &&
xdg_config && !access_or_warn(xdg_config, R_OK, 0)) {
@@ -664,7 +668,7 @@ int cmd_config(int argc, const char **argv, const char
*prefix)
}
if (HAS_MULTI_BITS(actions)) {
- error("only one action at a time.");
+ error("only one action at a time");
usage_with_options(builtin_config_usage,
builtin_config_options);
}
if (actions == 0)
@@ -819,7 +823,7 @@ int cmd_config(int argc, const char **argv, const char
*prefix)
if (ret < 0)
return ret;
if (ret == 0)
- die("No such section!");
+ die("no such section: %s", argv[0]);
}
else if (actions == ACTION_REMOVE_SECTION) {
int ret;
@@ -830,7 +834,7 @@ int cmd_config(int argc, const char **argv,
