Reference matching gets confused when reference part is not included

tl;dr Reference parsing may miss some when faced with ambiguity in messages like foo#1 Fix foo#2. Below, we see why that happens, and explore three possible solutions:

1. Leaving the current implementation in place while adding a special case
2. Extending the current implementation to be more versatile
3. Like 2 but with a mess of regexps

I try to be verbose but not too much, hopefully it's mostly code and not my rambling.

(Title a little wonky, my apologies)

It seems like commitlint#3231 stems from the conventional-commits-parser package. The issue is created with commits which look like foo#1 Fix something

We can see the following:

// References start with foo#
const preset = {
    issuePrefixes: ['foo#'],
};

const parse = require('.');
console.log('Fix:');
console.log(parse.sync('foo#1 Fix something', preset).references);
console.log('Bix:');
console.log(parse.sync('foo#1 Bix something', preset).references);

Gives us:

Fix:
[]
Bix:
[
  {
    action: null,
    owner: null,
    repository: null,
    issue: '1',
    raw: 'foo#1',
    prefix: 'foo#'
  }
]

Looking into the code, this seems to be rooted in getReferences: https://github.com/conventional-changelog/conventional-changelog/blob/c696fa35f93e0ee13728d6cf1221587ac6386311/packages/conventional-commits-parser/lib/parser.js#L51-L53

In the first case, regex.references matches the Fix something, which does not contain a referencePart (the foo#1). So while the reApplicable gets assigned to regex.references, and it matches a value, the inner exec against referenceParts returns nothing, and overall nothing is returned.

In the second case, regex.references does not find anything, so reApplicable is assigned to CATCH_ALL, allowing the inner exec to find the reference.

What can be done

There are multiple solutions available, but I will admit to probably being blind to many uses of this package, so I'm going for solutions which hopefully don't break compat in any meaningful way. To help ease the process, they are each given a hilarious title.

1. If at first you don't succeed

The tamest option is to try the two different values of reApplicable if the first doesn't succeed. Something like:

function getReferences (input, regex) {
    const references = getReferencesParts(input, regex.references, regex);

    if (references.length) {
        return references;
    }

    return getReferencesParts(input, CATCH_ALL, regex);
}

// The original implementation
function getReferencesParts (input, refRegex, regex) {
    const references = []
    let referenceSentences
    let referenceMatch

    while ((referenceSentences = refRegex.exec(input))) {
        const action = referenceSentences[1] || null
        const sentence = referenceSentences[2]

        while ((referenceMatch = regex.referenceParts.exec(sentence))) {
            let owner = null
            let repository = referenceMatch[1] || ''
            const ownerRepo = repository.split('/')

            if (ownerRepo.length > 1) {
                owner = ownerRepo.shift()
                repository = ownerRepo.join('/')
            }

            const reference = {
                action: action,
                owner: owner,
                repository: repository || null,
                issue: referenceMatch[3],
                raw: referenceMatch[0],
                prefix: referenceMatch[2]
            }

            references.push(reference)
        }
    }

    return references;
}

This passes the existing tests (but not style guide, sorry!) with flying green colours.

2. Why not both

While the first solution handles the case mentioned at the top, it will not find references like

foo#2 Fix bug stemming from foo#1

The current implementation and 1 will find foo#1 but not foo#2. One way to solve that is to go ham and apply both regexps (references and referenceParts), deduplicating the result. That would look something like:

// getReferenceParts from the snippet above. Forgive me for the below, not great code, just as a PoC
function getReferences (input, regex) {
    console.log(regex.referenceParts);
    const realReferences = getReferencesParts(input, regex.references, regex);
    const partReferences = getReferencesParts(input, CATCH_ALL, regex);

    const normRef = (ref) => _.pick(ref, ['owner', 'repository', 'issue', 'prefix']);
    // Dedup partReferences, horrible performance, truly a monstrosity
    const solelyPartReferences = partReferences.filter((ref) =>
        !realReferences.find(
            (otherRef) => _.isEqual(normRef(ref), normRef(otherRef))
        )
    );

    return realReferences.concat(solelyPartReferences);
}

This passes all existing tests with one exception: should work with options in cli.spec.js. In that test, it find an additional reference. The given fixture contains:

Close #10036, Closes #10000
Fixed #13233
fix #9338

The original does not find #10000, while the new implementation does. 50 points to Hufflepuff or some such.

3. All together

It may be possible to create a regex which can match both the reference itself and the surrounding context. I'm not quite sure what exactly that will look like, but that can't possibly stop me from suggesting it.

Concrete suggestion

Personally it feels like the 2nd approach may be better. It catches more such cases, with the downside of potentially being too reference-matching-happy.

If any of these (or related) are interesting, I'm more than willing to create a PR.

Thanks for your time.