Re: [Quilt-dev] Time for a new release?

[Peter Williams]

Andreas Gruenbacher wrote:
> Hello,
>
> On Friday 08 July 2005 12:27, Peter Williams wrote:
>
> > Jean Delvare wrote:
> >
> > > Hi all,
> > >
> > > What about a new release of quilt soon? There have been quite a few
> > > improvements in CVS recently, it would be great if more people could
> > > take benefit of these.
> > >
> > > Andreas?
> >
> > Before you make this release could you please apply the attached patch?
> >
> > It adds a "description" which can be used to view and set the
> > description component of a patch's header.  This will enable me to add
> > mechanisms for adding/viewing patch descriptions to gquilt.
>
>
> I have a few comments:
>
>  - I would rather make this the header command. We are usually calling what's 
> above the actual patch the header, no?

Yes, but this command only extracts/sets the description component of 
the header which may also contain diffstat data.  I.e. it's all about 
the MANUALLY created components of the header file.  I agree that 
"description" is rather a long name and that a shorter one would be 
preferable, e.g. "descr"?.

The "files" command goes part way towards providing the diffstat data 
perhaps it could be modified with an option to add diffstat data to its 
output?

>  - I don't see why it's useful or necessary to remove the diffstat part. Any 
> reasons? Presumably...

That's automatically generated and the description isn't.  Omitting it 
means that the user doesn't have to worry about reproducing it when the 
change the description using the -s option.

This would become even more important if we were to add an append (e.g. 
-a) option to append text to the existing description as an optional 
alternative to the -s option.  Arguably, this would be more useful to 
somebody using quilt from the command line than the -s option would be. 
 Appending comments after the diffstat data would probably break "refresh".

>  - Instead of patch_description_tail, please reuse the patch_description 
> function from patchfns (which is probably misnamed, and should be called 
> patch_header instead.)

Yes, it should be renamed BUT it doesn't do the same thing as 
patch_description_tail (which in fact is the complement of what 
patch_description should be i.e. it extracts everything EXCEPT the 
description).  This enables a new description to be attached to the 
front of the patch file without disturbing the patch or the diffstat 
data if they exist.

>  - Is removing whitespace in the header really needed?

Neatness counts :-)

> I don't think anybody 
> cares much about whitespace in the header at all; it's annyoing in the actual 
> patch though.

To me whitespace at the end of lines is annoying everywhere :-)

Unfortunately, some editors are difficult to configure so that they 
don't leave them dotted around and this provides an opportunity to get 
rid of them.

>  - We can easily get rid of the temporary files and use pipes instead. I can 
> easily do that when checking in the final version if you wish.

That would be fine.  I'm not really happy with all the temporary files 
either.  If I was doing this in Python I would just use strings :-).

>  - The command must not touch $patch~refresh.

I hope I didn't do that.  If I did it was unintentional.  Does this mean 
that the access times on the patch file have to be remain unaltered?  If 
so how is that accomplished?

>  - I see a dangling opt_format variable.

OK.  If you didn't already guess this file is a modified copy

>  - Could you please provide a test case?

I'll try.  I'll have to figure out how your test harness works first ;-).

Peter
--
Peter Williams                                   address@hidden

"Learning, n. The kind of ignorance distinguishing the studious."
 -- Ambrose Bierce