Matthieu Moy <Matthieu.Moy@xxxxxxx> writes:
> Change the option parsing logic in revision.c to accept detached forms
> like `-S foo' in addition to `-Sfoo'. The rest of git already accepted
> this form, but revision.c still used its own option parsing.
>
> This patch does not handle --stat-name-width and --stat-width, which are
> special-cases where diff_long_opt do not apply. They are handled in a
> separate patch to ease review.
>
> Original patch by Matthieu Moy, plus refactoring by Jonathan Nieder.
>
> Signed-off-by: Matthieu Moy <Matthieu.Moy@xxxxxxx>
> ---
> diff.c | 87 ++++++++++++++++++++++++++++++++++--------
> diff.h | 7 +++
> t/t4013-diff-various.sh | 5 ++
> t/t4013/diff.log_-S_F_master | 7 +++
> t/t4202-log.sh | 12 ++---
> 5 files changed, 95 insertions(+), 23 deletions(-)
> create mode 100644 t/t4013/diff.log_-S_F_master
>
> diff --git a/diff.c b/diff.c
> index 17873f3..d89ea20 100644
> --- a/diff.c
> +++ b/diff.c
> @@ -2990,9 +2990,50 @@ static int opt_arg(const char *arg, int arg_short, const char *arg_long, int *va
>
> static int diff_scoreopt_parse(const char *opt);
>
> +static inline int short_opt(char opt, const char **argv,
> + const char **optarg)
> +{
> + const char *arg = argv[0];
> + if (arg[0] != '-' || arg[1] != opt)
> + return 0;
> + if (arg[2] != '\0') {
> + *optarg = arg + strlen("-c");
Just a style thing, but I think "arg + 2" is much easier to read in this
particular case, as it won't risk tempting readers to go "Huh? What does
that 'c' mean"? I know the code wants to skip over an option that is a
single dash followed by a single byte and 'c' is just a placeholder for
that single unknown byte, but when the reader reaches that realization,
the reader already knows that the code wants to skip exactly two bytes,
no? strlen("--") in diff_long_opt() is much easier to justify, though. I
like the reduction of "arg + N"s that follow prefixcmp(arg, "--whatever")s
in diff_opt_parse() this patch gives us in general. I just think the
above "-c" is overdoing it.
Do we have an option that can take a zero-length string as its value and
do something meaningful? I don't think of any offhand ("log -S'' -p" is
not it---it may be meaningful but it is not useful), but this code would
start giving "-p" instead of "" to the option in such a case.
A longer option always required '=' before the value, so it won't share
the above issue (if it is an issue, that is).
Other than that minor style issue and a nagging -X<empty> worry, the
resulting code looks a lot nicer than the original.
Thanks.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html