better regex for what files PIFT should test

If the current regex is /(\.diff|\.patch)$/ then a new regex could be /(?<=-d6|\.d6)(\.diff|\.patch)$/ which uses negative look-behind to detect only .patch and .diff where it is not named with -d6 or .d6 before the final extension. Untested but should work from previous experience.

As for defining what the common ground for patch name ending for backports is a good question. I think -d6 and .d6 suffixes are common, so it makes sense to support both. Better one can only tell with a drupal.org database dump, taking a look at names of the files attached to issues.

Maintainability for later versions should also be considered. D5 backport patches are also posted in some issues. What if the .d7.patch becomes the backport patch (while we are working on D8?).

Sure, that would be /(?<=-d6|\.d6|-d5|\.d5)(\.diff|\.patch)$/

Oh, wow, sorry we should catch where this does not match, not where this matches, so make it: /(?<!-d6|\.d6|-d5|\.d5)(\.diff|\.patch)$/. To break it down, () is a separated "subsearch" ?<! means we should look back in the pattern and check that the value inside the subsearch is not there. The different options are the possible values separated with | so they are in an OR relationship.

Note: D6 is just as common as d6 in my experience, so we should support both cases. And, instead of instead of "unwinding" all the exclusion options, why not just use [.-][Dd][56]? That'll make it much easier to maintain this regexp in the future as we add/remove other versions of core...

dww. your regex as-is matches -d and -D without a number, so I assume you meant to use () around it before the question mark. If we are only going for these combinations, then yours is absolutely simpler.

Would be nice if project module allowed you to add a metadata tag to each attachment, indicating if it's a patch and what branch it's a patch on.

@gabor: the ? was related to the "why not" of the english prose, and wasn't included inside the code tag for the regexp. ;) Sorry that wasn't clear.

@mfb: yes it would, i said the same thing to hunmonk in IRC and he laughed at me, saying that was way out of scope...

I think it would also be useful to allow the underscore character to separate the patch name from a version segment. So, to modify dww's pseudo-regex in #5, I'd go with:

[.-_][Dd][56]

/me notices that the issues he brought up in original conversations with hunmonk that were dismissed re-emerge.

Just putting the latest version of the regex at the bottom of the thread. Make sure everybody's on the same page. :)

/(?<![.-_][Dd][56])(\.diff|\.patch)$/

I can't think of any more improvements, so +1 from me.

Could we also include "drupal5"/"drupal6" and CVS branch names "DRUPAL-5/DRUPAL-6"? Most often, I'm creating patches on dedicated core test sites, checked out into corresponding directory names, so all of these should not match:

whatever.d6.patch
drupal6.whatever.patch
drupal-DRUPAL-6.whatever.patch

I'd also like to propose to remove .diff from the regex, so we have a (yes, special) way of uploading patches that are NOT intended to be tested at all.

I like whatever.d6.patch as that is consistent with file extensions like pifr.pages.inc and is short and sweet.

well, if we want to test the suffix only, *d6.patch/*d5.patch would be sufficient.

My suggestion was based on my personal workflow/habit. I am using a script to create patches, which automatically takes the parent folder name (i.e. "drupal6" or "drupal-DRUPAL-6") as basename and extends it with ".patch". Afterwards, I'm just adding a descriptor in between, i.e. resulting in "drupal6.whatever-issue.patch".

If I am forced to, I can certainly append ".d6" to my descriptors where appropriate, or automate my environment even further ;)

d6/D6/d5/D5 seems reasonable to me. Doesn't seem like we need a lot more flexibility than that.

We'll need to document the creation in drupal.org/patch somewhere - if one or two people post patches don't meet the convention, then, well, that's tough, and it's easy to repost the same thing again differently named. Far worse things happen to issues than that.

Keep it simple. Also, pretty much talk about separating the "dX" suffix from the rest of the filename, while it's very unlikely that someone names a D7 patch "fixed6.patch" (note it *must* end in "d6" or "d5") - hence:

/(?<!d[56])(\.diff|\.patch)$/i

or if you prefer case-sensitive (while not really required):

/(?<![Dd][56])(\.diff|\.patch)$/

i like #13 better, too.

#13 is .d6.patch....right? If so then I would prefer that.

I've always seen the version suffix somehow separated from the rest of the patch name, so having the most common separators in the regexp is better imHo

+1 for the #13

#13 works for me too.

Files to match:

whatever.patch
whatever.diff

Always .patch and also .diff.

Files NOT to match:

whatever.d5.patch
whatever_d5.patch
whatever-d5.patch
whatever.d6.patch
whatever_d6.patch
whatever-d6.patch
whatever.d5_0.patch
whatever.d5_1.patch
whatever.d5_12.patch
whatever.d6_0.patch
whatever.d6_1.patch
whatever.d6_12.patch

"whatever" is an arbitrary string (*) of course. The prefix for d5/d6 can be a dot (.), hiven (-), or underscore (_).

Uploads could go farther than even foo.d5_13.patch, right? Might as well future-proof the _X check.

/(?<![._-][Dd][56](_\d+)?)(\.diff|\.patch)$/

I would use two regexes here, if (preg_match('/\.(patch|diff)$/') && !preg_match('/[._-]d[56](_\d+)?\./)

FWIW, this would be the required regular expression:

/(?<![._-][Dd][56])(?<!_\d)(?<!_\d\d)(\.diff|\.patch)$/

That's right. I forgot that look-behind assertions have to be fixed-length.

#31 won't work because it would not match foo_0.diff, which we do want to be tested.

The presence of _XX doesn't affect whether it is tested or not, so it does not belong in the look-behind assertion.

/(?<![._-][Dd][56])(_\d+)?\.(diff|patch)$/

I'm undecided if this is easier or harder to maintain than two preg_match() calls.

Sorry, either we need a regex guru/magician here, or I think this simply cannot be done in one preg_match() at all. Attached is a screenshot of several different approaches, including the negation of the complete matching criteria - without any luck.

Thus, I'd agree with chx: Implement 1 regex to match positive, and another one to match negative, i.e.

PASS 1 (positive):

/\.(diff|patch)$/

PASS 2 (negative):

/[._-][Dd][56]/

Critical as per:
http://drupal.org/node/329973

There are a few outstanding issues that need to be addressed before the maintenance

#35: I'm thinking it would be better to check the actual filename as the other is more of an editable user input field. (as I'm sure you know)

I think just having a regex match and regex exclude setting would cover this as chx pointed out in #30.

It can be done, it's just even uglier than most:

/(?<![._-][Dd][56]|[._-][Dd][56]_\d|[._-][Dd][56]_\d\d)\.(diff|patch)$/

Consider this proof of concept rather than something that should be used, though.

All my tests pass on #39.

However, it won't catch three appended numbers, i.e. whatever.d6_123.patch, so I'd still prefer two chained tests, as mentioned in #34 and #30.

We definitely have few patch names which are close to or over three digit suffixes now. I wish I didn't know that piece of information.