[35353] tinyca2 Lint Report

[Eric Hall]

On Thu, Mar 27, 2008 at 06:42:35AM -0500, Ryan Schmidt wrote:
> On Mar 26, 2008, at 11:28, Anders F Bj?rklund wrote:
> 
> > Landon Fuller wrote:
> >
> >> On Mar 26, 2008, at 12:13 AM, Ryan Schmidt wrote:
> >>
> >>> Patchfile naming: The old guide was contradictory, and in one place,
> >>> recommended the naming scheme "patch-*" while in another place it
> >>> recommended "patch-*.diff". The new guide is now consistent in
> >>> recommending "patch-*.diff".
> >>
> >> The original documentation (which I wrote) was originally
> >> consistent with the FreeBSD
> >> patch specification:
> >> 	http://www.freebsd.org/doc/en_US.ISO8859-1/books/porters-handbook/
> >> slow-patch.html
> >>
> >> We then adopted the semantic of patch-foobar.diff, where 'foobar'
> >> was a feature that
> >> covered many files -- this was due to the headache of patch-per-
> >> file for a sizable set
> >> of diffs.
> >
> > I agree with Landon here, the current lint warning seems misguided ?
> >
> > This was discussed at length before it was implemented too, with there
> > being two different kinds of patches. But apparently "consistency"  
> > once
> > again triumphed, and one of the types of patch naming was warned  
> > about.
> > If anything, it should be eased up to allow anything like "patch-*"...
> >
> > We had "patch-foo.c" and "patch-bar.h", and also "patch-foobar.diff".
> > i.e. either patch-FILENAME (BSD style) or patch-ISSUE.diff (multi- 
> > file)
> 
> Thanks for clarifying that. I didn't realize the .diff extension was  
> previously only used for multi-file patches, or rather, that the  
> recommendation to use the .diff extension was meant only for multi- 
> file patches. I had assumed someone had added something to the guide  
> in one place and forgotten to update the recommendation in the other  
> place. It seems an arbitrary distinction to me. A diff file is after  
> all *always* a diff file, whether it patches one file or five. But at  
> least now I understand the previously unexplained discrepancy in  
> patchfile name recommendations.
> 
> I still believe diff/patch files should always have the .diff  
> extension but I'll admit that having port lint warn about it is a bit  
> pedantic. Let's change port lint to only check for "patch-*".
> 
> The guide doesn't mention the .diff extension for patchfiles at all.  
> It has several examples of patchfile used in a port, none of which  
> end in .diff. I would like to suggest that we change the guide to  
> suggest using the .diff extension always. If people are against this  
> suggestion, then we should at least add to the guide the recommended  
> patchfile naming scheme for multi-file patches. I thought I had filed  
> a ticket about this topic before but I can't find it now, but I would  
> be happy to file one once we decide what the documentation should say.
> 

	I'd prefer that patches don't always end in .diff, but map to the
filenames for the single-file patches.  Patches that are multi-file I
lean towards no .diff extension, that's a very minor preference.

[snip]

> ...but I have a trailing space after the first line, port lint says  
> this:
> 
> $ port lint
> Can't map the URL 'file://.' to a port description file ("wrong #  
> args: should be "sha1 action ?file?"").
> Please verify that the directory and portfile syntax are correct.
> To use the current port, you must be in a port's directory.
> (you might also see this message if a pseudo-port such as
> outdated or installed expands to no ports).
> can't read "portvariants": no such variable
> Error: Status 1 encountered during processing.
> $
> 
> We can change port lint to just check for trailing whitespace after a  
> backslash. In light of the above, it seems it won't even get  
> executed, but maybe we should still leave a check in there, in case  
> we can some day improve port lint to be able to run in spite of such  
> a syntax error.

	I agree, trailing whitespace after a backslash (that's intended to
make a multi-line continuation) is an error, and port lint should 
catch that.  It'd be better if it caught it w/o being fatal, at least
its a very obvious error as shown above.


> 
> The warnings about missing blank lines are a bit too pedantic even  
> for my taste and I would remove them.
> 
> I attached new proposed patches to the ticket:
> 
> http://trac.macosforge.org/projects/macports/ticket/14799

	Looks good to me, thanks for improving the missing blank
lines part (I'll was being lazy there).


		-eric