| Message ID | 20190117130615.18732-77-pclouds@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Convert diff opt parser to parse_options() | expand |
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 > 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 > >
On Fri, Jan 18, 2019 at 3:10 AM Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote: > > 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? That's another 75 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. Sorry somehow I forgot about breaking down the series. Part of the reason though was I wanted to show that we got somewhere in the end, it's not just random restructuring patches that may end up getting reverted. If there are lots of changes in this series, I'll resend in smaller series. For the revision.c conversion I'll make sure to send in small series. > Ciao, > Johannes >
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);
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 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(-)