| Message ID | 20231207072458.GC1277973@coredump.intra.peff.net (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 0dda4ce9f697a9ddfc1b2a3825dc07827a99d942 |
| Headers | show |
| Series | bonus config cleanups | expand |
On Thu, Dec 07, 2023 at 02:24:58AM -0500, Jeff King wrote: [snip] > diff --git a/imap-send.c b/imap-send.c > index 996651e4f8..5b0fe4f95a 100644 > --- a/imap-send.c > +++ b/imap-send.c > @@ -1346,7 +1346,7 @@ static int git_imap_config(const char *var, const char *val, > server.port = git_config_int(var, val, ctx->kvi); > else if (!strcmp("imap.host", var)) { > if (!val) { > - git_die_config("imap.host", "Missing value for 'imap.host'"); > + return error("Missing value for 'imap.host'"); Nit: while at it we might also mark this error for translation. Not worth a reroll on its own though. Patrick
On Thu, Dec 07, 2023 at 09:57:55AM +0100, Patrick Steinhardt wrote: > On Thu, Dec 07, 2023 at 02:24:58AM -0500, Jeff King wrote: > [snip] > > diff --git a/imap-send.c b/imap-send.c > > index 996651e4f8..5b0fe4f95a 100644 > > --- a/imap-send.c > > +++ b/imap-send.c > > @@ -1346,7 +1346,7 @@ static int git_imap_config(const char *var, const char *val, > > server.port = git_config_int(var, val, ctx->kvi); > > else if (!strcmp("imap.host", var)) { > > if (!val) { > > - git_die_config("imap.host", "Missing value for 'imap.host'"); > > + return error("Missing value for 'imap.host'"); > > Nit: while at it we might also mark this error for translation. Not > worth a reroll on its own though. This string goes away entirely in the next patch, so I don't think we need to mark it here. Thanks, Taylor
On Fri, Dec 08, 2023 at 05:58:52PM -0500, Taylor Blau wrote: > On Thu, Dec 07, 2023 at 09:57:55AM +0100, Patrick Steinhardt wrote: > > On Thu, Dec 07, 2023 at 02:24:58AM -0500, Jeff King wrote: > > [snip] > > > diff --git a/imap-send.c b/imap-send.c > > > index 996651e4f8..5b0fe4f95a 100644 > > > --- a/imap-send.c > > > +++ b/imap-send.c > > > @@ -1346,7 +1346,7 @@ static int git_imap_config(const char *var, const char *val, > > > server.port = git_config_int(var, val, ctx->kvi); > > > else if (!strcmp("imap.host", var)) { > > > if (!val) { > > > - git_die_config("imap.host", "Missing value for 'imap.host'"); > > > + return error("Missing value for 'imap.host'"); > > > > Nit: while at it we might also mark this error for translation. Not > > worth a reroll on its own though. > > This string goes away entirely in the next patch, so I don't think we > need to mark it here. > > Thanks, > Taylor Ah, true. Never mind in that case. Patrick
On Fri, Dec 08, 2023 at 05:58:52PM -0500, Taylor Blau wrote: > On Thu, Dec 07, 2023 at 09:57:55AM +0100, Patrick Steinhardt wrote: > > On Thu, Dec 07, 2023 at 02:24:58AM -0500, Jeff King wrote: > > [snip] > > > diff --git a/imap-send.c b/imap-send.c > > > index 996651e4f8..5b0fe4f95a 100644 > > > --- a/imap-send.c > > > +++ b/imap-send.c > > > @@ -1346,7 +1346,7 @@ static int git_imap_config(const char *var, const char *val, > > > server.port = git_config_int(var, val, ctx->kvi); > > > else if (!strcmp("imap.host", var)) { > > > if (!val) { > > > - git_die_config("imap.host", "Missing value for 'imap.host'"); > > > + return error("Missing value for 'imap.host'"); > > > > Nit: while at it we might also mark this error for translation. Not > > worth a reroll on its own though. > > This string goes away entirely in the next patch, so I don't think we > need to mark it here. Right. It's a little confusing because it is converted along with some other spots in the next patch. But in one of those other spots, we earlier switched it (in patch 2) from die() to error(), and we _did_ mark it for translation as we did so. I did it there because in patch 2 we touch multiple messages, and the other ones don't end up as config_error_nonbool(), so we do want them translated. I'm not sure if there would have been an easier ordering to the series. I could have pulled the "mark for translation" bits from patch 2 into their own patch (after this one makes some of the messages go away), but then I'd expect somebody would review patch 2 and say "why not mark them for translation while we're here?". :) -Peff
diff --git a/imap-send.c b/imap-send.c index 996651e4f8..5b0fe4f95a 100644 --- a/imap-send.c +++ b/imap-send.c @@ -1346,7 +1346,7 @@ static int git_imap_config(const char *var, const char *val, server.port = git_config_int(var, val, ctx->kvi); else if (!strcmp("imap.host", var)) { if (!val) { - git_die_config("imap.host", "Missing value for 'imap.host'"); + return error("Missing value for 'imap.host'"); } else { if (starts_with(val, "imap:")) val += 5;
The point of git_die_config() is to let configset users mention the file/line info for invalid config, like: if (!git_config_get_int("foo.bar", &value)) { if (!is_ok(value)) git_die_config("foo.bar"); } Using it from within a config callback is unnecessary, because we can simply return an error, at which point the config machinery will mention the file/line of the offending variable. Worse, using git_die_config() can actually produce the wrong location when the key is found in multiple spots. For instance, with config like: [imap] host host = foo we'll report the line number of the "host = foo" line, but the problem is on the implicit-bool "host" line. We can fix it by just returning an error code. Signed-off-by: Jeff King <peff@peff.net> --- imap-send.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)