Re: Patch name changes

[David Holland <dholland-pkgtech%netbsd.org@localhost>]

On Fri, Jun 11, 2010 at 10:06:21AM +0200, Thomas Klausner wrote:
 > On Thu, Jun 10, 2010 at 07:07:32PM +0000, David Holland wrote:
 > > I don't think this is a good idea. A few packages already follow this
 > > model (or some approximation of it) and IME it doesn't really make
 > > updating any easier; it just makes it somewhat harder to work with the
 > > patch directory by e.g. making it harder to write shell globs that
 > > exclude editor backup files.
 > 
 > What kind of globs do you use on your patch-directory?
 >
 > What exactly is the problem you're seeing? How would it change with
 > this naming change?

patches/patch-??, which does not match e.g. patches/patch-aa~, which
will appear after updating patch comments, because I generally use
emacs. One can always explicitly remove the editor backups, but that's
a hassle, to some extent a risk of mistyping and removing everything,
and it's also not always advisable to do this before one has finished
hacking.

If you're asking why this comes up, it's usually searching for
references to some identifier or other.

 > > I've tried three times now to wrangle the 234 patches in xview, the
 > > intent being to move most of the changes to one or more distfile
 > > patches. In order for that to be useful in the long run the changes
 > > need to be sorted by topic; getting there without accidentally losing
 > > any bits turns out to be a nontrivial problem.
 > 
 > And I think that's exactly the problem we'd be seeing with
 > patches-per-topic. It's perhaps not so hard to create a new patch
 > that's per topic, but for package updates, I don't see how it can be
 > automated in any meaningful way -- and I think package updates are the
 > most common reason for changes in pkgsrc.
 >
 > > [pkgquilt]

Basically the premise (of quilt or Mercurial's mq extension) is that
you get to keep a patch collection and then do mkpatches independently
for each patch. I'm not entirely sure what else you might mean by
"automated" but I *think* that covers it...

 > > In any event, if we really want long patch names, they should be
 > > patch-aa-mumble, not just patch-mumble, or the application order will
 > > depend on locale settings and case-sensitivity and other horrors,
 > > which will inevitably lead to weird problems.
 > 
 > Since each file is only patched once, we don't care about application
 > order (staying with patch-per-file, which is what I was suggesting).

No, we don't, until someone makes a mistake (this happens from time to
time) or some end user hits a bug in patch (this too) and trying to
figure out what happened suddenly becomes impossible because it
depends on the user's setting of LC_COLLATE or whatnot that they
didn't report to us and possibly don't themselves understand.

Maybe this is a small risk.

 > >  > Dillo and/or I will improve mkpatches to handle this automatically.
 > > 
 > > Can you also fix mkpatches to not throw away comments, or do something
 > > to persuade people who are (apparently) using old versions that do to
 > > not do so? I have several times seen subsequent commits blithely erase
 > > patch comments I'd added.
 > 
 > mkpatches already keeps comments, but it seems some people are not
 > using it. If we let pkglint complain about missing comments, that will
 > be much easier to see for them...

It already does:

valkyrie% pwd
/usr/pkgsrc/net/rdist6
valkyrie% pkglint -Wall
WARN: PLIST:3: Manual page missing for bin/rdistd.
WARN: patches/patch-aa:3: Comment expected.
WARN: patches/patch-ab:3: Comment expected.
WARN: patches/patch-ac:3: Comment expected.
WARN: patches/patch-ad:3: Comment expected.
WARN: patches/patch-ae:3: Comment expected.
WARN: patches/patch-af:3: Comment expected.
WARN: patches/patch-ag:3: Comment expected.
WARN: patches/patch-ah:3: Comment expected.
WARN: patches/patch-aj:3: Comment expected.
WARN: patches/patch-ak:3: Comment expected.
WARN: patches/patch-al:3: Comment expected.
WARN: patches/patch-am:3: Comment expected.
0 errors and 13 warnings found.

It does not do this without -Wall, so if what you're suggesting is to
turn this warning on by default rather than in -Wall, let's just do
that.

-- 
David A. Holland
dholland%netbsd.org@localhost