Skip to content

Parsing of 'snap' in versions doesn't work like the code suggests it does #2691

@fullermd

Description

@fullermd

The code in libpkg/pkg_version.c has some special magic parsing for strings like 'alpha' and 'beta', but also 'snap', which causes them to be treated differently from straight alphabetic version bits if they follow a number in the parsing (e.g., 1alpha is treated as 1.alpha, while 1alph is just 1alph).

However, the handling of 'snap' is broken, so it doesn't get treated like the code suggests. Commit 507aaa8 claims it wants to make 'snap' parse after 'alpha', and presumably meant to bump the value to accomplish that. However, it bumped the namelen field, which means the later code does strncasecamp() with a length of 6, instead of the 4 that 'snap' actually takes up, so it never matches right.

The effective behavior appears the same as if 'snap' were just removed from the matching list totally, so it gets collapsed down to 's' like any other alphabetic string, unless it's alone in the final component. In some testing,

# Alone as the last component it matches, so winds up in the right place before 'p' but after 'pl'.
% pkg version -t '1.snap' '1.p'
<
% pkg version -t '1.snap' '1.pl'
>

# But with a trailing component so strncasecmp() doesn't fall off into NUL, 's > p' always happens;
# snap is just the same as any other string starting with 's'.
% pkg version -t '1.snap.1' '1.p.1'
>
% pkg version -t '1.snap.1' '1.pl.1'
>
% pkg version -t '1.snap.1' '1.snacks.1'
=

# Only comes about when it's _alone_ in the component, not preceeded by the number.
% pkg version -t '1snap' '1alpha' 
>
% pkg version -t '1.snap' '1.alpha' 
<

# And any following patch level means it's just another 's' word again too.
% pkg version -t '1.snap1' '1.alpha1'  
>
% pkg version -t '1.snap1' '1.snack1'
=

Considering the change was 2.5 years ago and it appears not to have been noticed, that's presumably the correct behavior by definition now. It's certainly very misleading how magic that is, and not at all clear from the code. It's not clear that magic trailing '.snap' is intended or useful, especially since it goes against the commit log that caused the current behavior.

If 'snap' really should be magic, it should probably be magic everywhere, so the namelen should be fixed and maybe the value tweaked somehow. It's not clear from current behavior that it really should be magic though. My suspicion is that just removing it from that magic list so it's the same as any other letter combo is probably what we should do.

At the least, if it's very specifically meant to be magic ONLY as the sole contents of the final component, ITWBNI if that could be explicitly called out in the code, not just a weird side effect of all the interacting subtleties of the parsing.

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type
    No fields configured for issues without a type.

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions