Web lists-archives.com

Re: [PATCH] format-patch: respect --stat when explicitly specified




On Tue, Nov 6, 2018 at 5:31 PM Leif Lindholm <leif.lindholm@xxxxxxxxxx> wrote:
> > > diff --git a/builtin/log.c b/builtin/log.c
> > > index 061d4fd86..07e6ae2c1 100644
> > > --- a/builtin/log.c
> > > +++ b/builtin/log.c
> > > @@ -1009,7 +1009,8 @@ static void show_diffstat(struct rev_info *rev,
> > >
> > >         memcpy(&opts, &rev->diffopt, sizeof(opts));
> > >         opts.output_format = DIFF_FORMAT_SUMMARY | DIFF_FORMAT_DIFFSTAT;
> > > -       opts.stat_width = MAIL_DEFAULT_WRAP;
> > > +       if (rev->diffopt.stat_width == -1)
> >
> > I don't think we get -1 here when stat_width is not defined. The
> > "undefined" value is zero but I'm pretty sure we get MAIL_DEFAULT_WRAP
> > in here unless you specify --stat.
>
> From what I could tell, if nothing is specified on command line,
> rev->diffopt.stat_width is set to -1 at this point (I assumed by
> rev->cmd_log_init_defaults(), but didn't go digging).

I thought the same but could find where cmd_log_.. is called by
format-patch. I was not even sure if I read the code correctly so I
ran the command through gdb. It was definitely not called.

> The patched version certainly gives the <= 2.16.* behaviour with
> --stat and still restricts stat lines to 72 characters without.
>
> > So I think you can just drop the below assignment. But if you want to
> > be on the safe side, check for zero stat_width instead of -1 and set
> > MAIL_DEFAULT_WRAP.
> >
> > > +               opts.stat_width = MAIL_DEFAULT_WRAP;
> >
> > How about a test to make sure this will not be broken in future?
>
> Sure. Only today was the first time I had a look at the git sources,
> so some guidance would be most appreciated.

No problem (and if you don't have time to do it, just say the word and
I will continue; this is my bug after all)

> Should I add a function to t/t4014-format-patch.sh and just put
> something longer than a/file for the expect template?

First of all the README file in that directory could give pretty good
basic instructions.

Back to this test, I think you could start by copying to a new test
(the whole "test_expect_success" block, optionally including the
"expect" file creation too), add --stat there and see what it looks
like. For stat testing, t4052 could also be a good example. Or perhaps
the test should be added in t4052 because it already supports long
file name ($name is 120 char long).
-- 
Duy