| Message ID | 68e0bd9e2d6f0a89d60db730eb77507d6a17a5ae.1615813658.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | commit: add --trailer option | expand |
On Mon, Mar 15, 2021 at 2:07 PM ZheNing Hu via GitGitGadget <gitgitgadget@gmail.com> wrote: > > From: ZheNing Hu <adlternative@gmail.com> > > When using `interpret-trailers`, if user accidentally fill in the > wrong option values (e.g. `--if-exists=addd` or `--where=begin` or > `--if-missing=do-nothing`), git will exit quietly, and the user may > not even know what happened. > > So lets provides warnings when git parsing this three options: > "unknown value '%s' for key ('where'|'--if-exist'|'--if-missing')". > This will remind the user :"Oh, it was because of my spelling error." > (or using a synonym for error). I am not against this, but I just want to say that when I previously worked on `interpret-trailers` I think I implemented or suggested such warnings, but they were rejected. I think the reason they were rejected was to improve compatibility with future versions of Git where more options would be implemented. For example if in a few years someone implements `--where=middle` and some people use it in a script like this: git interpret-trailers --where=middle --trailer foo=bar Then when such a script would be used with a recent version of Git it would work well, while if it would be used with an old version of Git it would emit warnings. And these warnings might actually be more annoying than the fact that the trailer is not put in the middle. I might be wrong and there might have been other reasons though. Also things might have changed since that time, as not many options if any have been added since then.
Christian Couder <christian.couder@gmail.com> 于2021年3月16日周二 下午1:53写道： > I am not against this, but I just want to say that when I previously > worked on `interpret-trailers` I think I implemented or suggested such > warnings, but they were rejected. > > I think the reason they were rejected was to improve compatibility > with future versions of Git where more options would be implemented. > > For example if in a few years someone implements `--where=middle` and > some people use it in a script like this: > > git interpret-trailers --where=middle --trailer foo=bar > > Then when such a script would be used with a recent version of Git it > would work well, while if it would be used with an old version of Git > it would emit warnings. And these warnings might actually be more > annoying than the fact that the trailer is not put in the middle. > Well, this is indeed a situation I did not foresee. As a user, I don't see an error | warning reminder which may make me a little confused. At the same time, some sub-command like `git cat-file`, If user give a wrong format: $ git cat-file --batch-check="%(deltabases)" git will tell us, "fatal: unknown format element: deltabases". It's easy for user to check. > I might be wrong and there might have been other reasons though. Also > things might have changed since that time, as not many options if any > have been added since then. Thanks for your patiently explanation. If these warnings are really unnecessary, I will drop this patch.
diff --git a/builtin/interpret-trailers.c b/builtin/interpret-trailers.c index 84748eafc01b..6d8e86a44d43 100644 --- a/builtin/interpret-trailers.c +++ b/builtin/interpret-trailers.c @@ -24,19 +24,38 @@ static enum trailer_if_missing if_missing; static int option_parse_where(const struct option *opt, const char *arg, int unset) { - return trailer_set_where(&where, arg); + int ret; + + ret = trailer_set_where(&where, arg); + if (ret) + warning(_("unknown value '%s' for key 'where'"), + arg); + return ret; + } static int option_parse_if_exists(const struct option *opt, const char *arg, int unset) { - return trailer_set_if_exists(&if_exists, arg); + int ret; + + ret = trailer_set_if_exists(&if_exists, arg); + if (ret) + warning(_("unknown value '%s' for key 'if_exists'"), + arg); + return ret; } static int option_parse_if_missing(const struct option *opt, const char *arg, int unset) { - return trailer_set_if_missing(&if_missing, arg); + int ret; + + ret = trailer_set_if_missing(&if_missing, arg); + if (ret) + warning(_("unknown value '%s' for key 'if_missing'"), + arg); + return ret; } static void new_trailers_clear(struct list_head *trailers)