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

[Leif Lindholm <leif.lindholm@linaro.org>]

On Tue, Nov 06, 2018 at 06:13:00PM +0100, Duy Nguyen wrote:
> On Tue, Nov 6, 2018 at 5:31 PM Leif Lindholm <leif.lindholm@linaro.org> 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.

Huh...

> > 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)

Weeeell, if you're offering, I would certainly appreciate not having
to dig deeper into this. I've got a patch review backlog the length of
my arm in another project...

> > 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).

(Thanks!)

/
    Leif