Re: [Quilt-dev] [PATCH] `quilt header -a' corrupts diffstat-carrying headers

[Jean Delvare]

Hi Don,

Le jeudi 14 janvier 2010 05:54, Don Mullis a écrit :
> The sequence
>     quilt refresh --diffstat
>     echo "Appended line" | quilt header -a
> puts the appended line _after_ the diffstat, contrary to expectation.

Contrary to your expectations, maybe. Not to mine.

That being said, I never use "quilt header -a" myself, and I have to
admit I am curious what others may be using it for. It doesn't seem
very useful to me.

> 
> Instead, strip out the diffstat info (as is done already for "-r"),
> and append stdin to what remains of the header.

I don't see any inconsistency at the moment. -r replaces the whole
header, -a appends at the end of the header. The command doesn't care
about the contents of the header. Whether you find it convenient in
practice is a different issue.

With your proposed change, "quilt header -a" would act contrary to
my expectations for sure. I wouldn't expect an "append" operation to
remove anything.

The only behavioral change I would consider is the following: if
a diffstat section is present _and_ it is preceded by "---" _and_ it
is at the end of the header comment, then "quilt header -a" could be
made to insert the additional comment right before the "---". But
even that might go against the expectations of some users.

> ---
>  quilt/header.in           |    9 ++++++++-
>  quilt/scripts/patchfns.in |   17 +++++++++++++++++
>  test/header.test          |   36 ++++++++++++++++++++++++++++++++++++
>  3 files changed, 61 insertions(+), 1 deletion(-)
> 
> Index: quilt/quilt/header.in
> ===================================================================
> --- quilt.orig/quilt/header.in        2010-01-13 20:07:00.000000000 -0800
> +++ quilt/quilt/header.in     2010-01-13 20:13:53.000000000 -0800
> @@ -139,7 +139,14 @@ else
>       tmp2=$(gen_tempfile) || exit 1
>       add_exit_handler "rm -f $tmp $tmp2"
>  
> -     (       if [ -z "$opt_replace" ]
> +     (       if [ -n "$opt_append" ]
> +             then
> +                     # diffstat lines are always at the end of the
> +                     # comment section proper, and are preceded by
> +                     # a "---" line and followed by a blank line.

This assumption is incorrect. Quilt shouldn't have any expectations
about the header contents. When it adds the diffstat section, it puts
it at the end, simply because it must put it somewhere, and appending
is the most natural operation. But when it updates the diffstat
section, it leaves it where it found it. Having the diffstat at the
end of the comment is a habit shared by many, but we do not want
quilt to enforce that.

> +                     cat_file $patch_file_or_null | patch_header \
> +                             | strip_diffstat_and_padding
> +             elif [ -n "$opt_edit" ]
>               then
>                       cat_file $patch_file_or_null | patch_header
>               fi
> Index: quilt/test/header.test
> ===================================================================
> --- quilt.orig/test/header.test       2010-01-13 20:07:00.000000000 -0800
> +++ quilt/test/header.test    2010-01-13 20:29:27.000000000 -0800
> @@ -45,5 +45,41 @@
>       > -foo
>       > +bar
>  
> +     $ quilt refresh --diffstat
> +     > Refreshed patch %{P}patch
> +
> +     $ quilt header -r
> +     < Header2
> +     > Replaced header of patch patches/patch
> +     $ head -n 2  %{P}patch
> +     > Header2
> +     > Index: d/foo
> +
> +     $ quilt refresh --diffstat
> +     > Refreshed patch %{P}patch
> +
> +     $ quilt header -a
> +     < Appended
> +     > Appended text to header of patch patches/patch
> +
> +     $ quilt header
> +     > Header2
> +     > Appended
> +     $ head -n 3 %{P}patch
> +     > Header2
> +     > Appended
> +     > Index: d/foo
> +
> +     $ cat patches/patch
> +     > Header2
> +     > Appended
> +     > Index: d/foo
> +     > ===================================================================
> +     > --- d.orig/foo
> +     > +++ d/foo
> +     > @@ -1 +1 @@
> +     > -foo
> +     > +bar
> +
>       $ cd ..
>       $ rm -rf d
> Index: quilt/quilt/scripts/patchfns.in
> ===================================================================
> --- quilt.orig/quilt/scripts/patchfns.in      2010-01-13 20:07:00.000000000 
> -0800
> +++ quilt/quilt/scripts/patchfns.in   2010-01-13 20:13:53.000000000 -0800
> @@ -852,6 +852,23 @@ strip_diffstat()
>       '
>  }
>  
> +strip_diffstat_and_padding()
> +{
> +     # 'eat' stores lines that match the first pattern but may not
> +     # be part of a diffstat.
> +     awk '
> +     /#? .* \| / || /^---$/ \
> +             { eat = eat $0 "\n"
> +               next }
> +     /^#? .* files? changed(, .* insertions?\(\+\))?(, .* deletions?\(-\))?/ 
> \
> +             { eat = ""
> +               getline
> +               next }
> +             { print eat $0
> +               eat = "" }
> +     '
> +}
> +
>  in_array()
>  {
>       local a=$1
> 

-- 
Jean Delvare
Suse L3