[Quilt-dev] Re: diffstat patch

[John Vandenberg]

Jean,

Thanks for the comments.

On 9/3/05, Jean Delvare <address@hidden> wrote:
> Hi John,
> 
> I have merged part of your diffstat patch into quilt, and have comments
> on the parts I did not merge.
>
> > --- quilt.orig/quilt/refresh.in       2005-08-08 10:08:18.%N +1000
> > +++ quilt/quilt/refresh.in    2005-08-17 02:07:21.%N +1000
> > @@ -258,26 +258,27 @@
> >  then
> >       diffstat="$(@DIFFSTAT@ -p$opt_strip_level $tmp_patch)" || die 1
> >       @AWK@ '
> > -         function print_diffstat(arr, i) {
> > +         function print_diffstat() {
> >             split(diffstat, arr, "\n")
> >             for (i=1; i in arr; i++)
> >               print prefix arr[i]
> >           }
> 
> Not merged, as it isn't correct. This change makes "arr" and "i" be
> global variables rather than local variables, which doesn't make sense.
> I agree that awk really has a strange syntax for declaring local
> variable, but that's the way it is.
> 
> > -         BEGIN       { split(diffstat, ds_arr, /\n/) }
> 
> Merged, nice catch.
> 
> >                       { prefix=""
> >                         if (index($0, "#") == 1)
> >                           prefix="#"
> >                       }
> > -         /^#? .* \| /        { eat = eat $0 "\n"
> > +         /^#? .* \|  *[1-9][0-9]* [+-][+-]*/ { eat = eat $0 "\n"
>                           next }
> 
> I've made it more simple:
> 
>             /^#? .* \|  *[1-9][0-9]* /
> 
> Yours wasn't correct (as explained in a previous post) and would fail to
> catch alternative diffstat formats such as -f0. I don't think this will
> be a problem, as a false positive is rather improbable. Having a "|"
> followed by a number in a header line wouldn't be sufficient, this line
> would also have to immediately precede a real diffstat section (without
> even a blank line between them.) If such a case is ever reported, we'll
> see, but until then this new regular expression looks like a good
> tradeoff between the false positive risk and the flexibility towards
> alternative diffstat formats.

Agreed.

> >           /^#? .* files? changed(, .* insertions?\(\+\))?(, .* 
> > deletions?\(-\))?/ \
> >                       { print_diffstat()
> >                         diffstat = "" ; eat = ""
> >                         next }
> >                       { print eat $0
> > -                       eat = "" }
> > +                       eat = ""
> > +                       add_nl=(length($0) > 1) }
> >           END         { printf "%s", eat
> >                         if (diffstat != "") {
> > +                         if (add_nl) print prefix
> >                           print_diffstat()
> >                           print prefix
> >                         }
> 
> Not merged, as quilt isn't supposed to enforce any particular layout in
> the header. The layout is up to the user, and tastes vary.

No problems.  Joe Green's recent --pad patch does a far better job of
what I was attempting to do here.

Regarding the test case, I have moved test/Makefile to
test/Makefile.in, and was going to include the test when $(DIFFSTAT)
was not empty.  Does that sound reasonable?

--
John