Re: [PATCH 76/76] am: avoid diff_opt_parse()
- From
Johannes Schindelin <johannes.schindelin@gmx.de>
- Date
- Jan 17, 2019, 20:10 UTC
- Message-ID
- <nycvar.QRO.7.76.6.1901172104380.41@tvgsbejvaqbjf.bet>
- In-Reply-To
- <20190117130615.18732-77-pclouds@gmail.com>
Hi Duy,
the change itself looks good, but...
On Thu, 17 Jan 2019, Nguyễn Thái Ngọc Duy wrote:
> diff_opt_parse() is a heavy hammer to just set diff filter. But it's > the only way because of the diff_status_letters[] mapping. Add a new > API to set diff filter and use it in git-am. diff_opt_parse()'s only > remaining call site in revision.c will be gone soon and having it here
... "will be gone soon"? Does that mean that you mail-bomb another mega patch series iteration once you did that, now sending 77 or 78 patches?
I don't know about others, but I can only afford to spend a fraction of my waking hours on reviews, and even back when Christian sent the built-in am as a loooong patch series it was *already* a big problem. Thankfully he seems to have decided to never do that again.
It would probably make sense to break your 76-strong patch series down into at least four separate patch series, they would still be as long as my Azure Pipelines one (which is longer than I am actually comfortable with, but in my case, it was necessary, while your patch series consists of many, mostly independent patches that could even be wrapped into individual patch series of 1 or 2). It's just way too much to review if you present it in the current manner.
Ciao, Johannes
Show 61 quoted lines
> just because of git-am does not make sense.
>
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
> builtin/am.c | 4 ++--
> diff.c | 6 ++++++
> diff.h | 2 ++
> 3 files changed, 10 insertions(+), 2 deletions(-)
>
> diff --git a/builtin/am.c b/builtin/am.c
> index 95370313b6..0cbf285459 100644
> --- a/builtin/am.c
> +++ b/builtin/am.c
> @@ -1515,11 +1515,11 @@ static int fall_back_threeway(const struct am_state *state, const char *index_pa
> * review them with extra care to spot mismerges.
> */
> struct rev_info rev_info;
> - const char *diff_filter_str = "--diff-filter=AM";
>
> repo_init_revisions(the_repository, &rev_info, NULL);
> rev_info.diffopt.output_format = DIFF_FORMAT_NAME_STATUS;
> - diff_opt_parse(&rev_info.diffopt, &diff_filter_str, 1, rev_info.prefix);
> + rev_info.diffopt.filter |= diff_filter_bit('A');
> + rev_info.diffopt.filter |= diff_filter_bit('M');
> add_pending_oid(&rev_info, "HEAD", &our_tree, 0);
> diff_setup_done(&rev_info.diffopt);
> run_diff_index(&rev_info, 1);
> diff --git a/diff.c b/diff.c
> index daccc8226f..b8e58e817b 100644
> --- a/diff.c
> +++ b/diff.c
> @@ -4756,6 +4756,12 @@ static unsigned filter_bit_tst(char status, const struct diff_options *opt)
> return opt->filter & filter_bit[(int) status];
> }
>
> +unsigned diff_filter_bit(char status)
> +{
> + prepare_filter_bits();
> + return filter_bit[(int) status];
> +}
> +
> static int diff_opt_diff_filter(const struct option *option,
> const char *optarg, int unset)
> {
> diff --git a/diff.h b/diff.h
> index 03c6afda22..f88482705c 100644
> --- a/diff.h
> +++ b/diff.h
> @@ -233,6 +233,8 @@ struct diff_options {
> struct option *parseopts;
> };
>
> +unsigned diff_filter_bit(char status);
> +
> void diff_emit_submodule_del(struct diff_options *o, const char *line);
> void diff_emit_submodule_add(struct diff_options *o, const char *line);
> void diff_emit_submodule_untracked(struct diff_options *o, const char *path);
> --
> 2.20.0.482.g66447595a7
>
>