Luke Shumaker authored Jul 15, 2018 and Eli Schwartz committed Oct 16, 2018

 - Implement the TODO by keeping a list/set of architectures found via
   "$repo-$arch" directory names, and another list/set of architectures
   named in arch=() in the PKGBUILD(s).  This means turning the simple
   `compgen` in to a loop.

 - While we're at it loading PKGBUILDs in a loop, fix that it clearly
   isn't doing anything with the $pkgver argument; it should verify that
   the version in the found PKGBUILD(s) matches that argument.

 - Use extglob to more strictly match the "arch" part of the "repo-arch"
   dirname; the existing glob wouldn't have behaved correctly for values
   of $repo containing a "-" (like "community-testing").  We don't
   currently test with any of those, but it makes me nervous.

 - Also make that extglob change in checkRemovedPackage.  While we're at
   it, let Bash do the glob expansion normally and check it with
   __isGlobfile, instead of compgen; it means we don't have to worry about
   escaping the non-glob part of the string.  Not that we expect it to
   contain anything needing escaping, but again, it makes me nervous.

Luke Shumaker authored Jul 15, 2018 and Eli Schwartz committed Oct 16, 2018

It is important that db-remove be able to remove a single pkgname, without
being able to look it up by pkgbase in SVN.  For instance, when a split
package update removes one of its members; there will be no reference to
the removed pkgname in SVN, and it won't be removed by db-update.  If
db-remove doesn't accept pkgnames, then this outdated orphan could not be
removed.

As of the source_pkgbuild rewrite, this is only ever done once.

Introduce "db-functions-$VCS" which will eventually contain all
VCS-specific code, and make this configurable in config.

Move private arch_svn function and svn acl handling here, and introduce
a new source_pkgbuild function to handle discovering PKGBUILDs from the
configured VCS and sourcing them to extract metadata.

The PKGBUILD is the only file we ever check out from version control,
and only ever to scrape information from it, except for when we actually
want to db-move a whole directory (which is by necessity considerably
dependent on the VCS in use).

source_pkgbuild is inspired by commits from the dbscripts rewrite,
authored by Florian Pritz <bluewind@xinu.at>

Luke Shumaker authored Jul 13, 2018 and Eli Schwartz committed Oct 07, 2018

The problem statement:

  checkPackageDB and checkRemovedPackageDB need bit of information on the
  package they're checking: a full list of pkgnames with that pkgbase, the
  list of pkgarches, and (for checkPackageDB only) the full version.  In
  dbscripts itself, we can get that directly from the .db files; however,
  since the test suite is checking the ability of dbscripts to edit those
  .db files, that's obviously not a good solution.

  The current solution is to get this information from the PKGBUILD...
  which we also count on dbscripts to correctly keep track of.  Wait,
  that's skipping ahead, let's back up.

  The current solution is to get this information from the PKGBUILD.  For
  checkRemovedPackageDB, that's easy; we just get it from trunk, as that's
  the most up-to-date information on the package as-it-would-have-existed
  (if that sounded a little hand-wavey, it was).  But for checkPackageDB,
  it's a little trickier, because of 2 factors working together: (1) there
  might be different versions on different repos, and (2) unlike
  checkRemovedPackageDB, checkPackageDB actually cares about pkgver.  So,
  checkPackageDB "guesses" the location in a slightly sloppy way, and is
  tagged with a "FIXME".

What todo about it?

  There are a couple of things to observe:

   - Hidden in the hand-waving in assuming that "trunk" is fine for
     checkRemovedPackageDB is the assumption that neither pkgname=() nor
     arch=() is going to change between versions.  Which is a fine
     assumption, because we don't change those things between versions in
     any of our test cases.
   - We're counting on dbscripts correctly keeping track of which PKGBUILD
     is correct for which repo... which is one of the thing's we're trying
     to test, we shouldn't be counting on it.  That's actually a bigger
     problem than the original "FIXME"!

  So, putting those things together, let's (1) take the code under test out
  of the equation, and (2) remove any suggestion that the version of the
  PKGBUILD makes a difference to pkgnames/pkgarches: Let's have both
  functions that that information from the PKGBUILDs under "fixtures/",
  rather than getting PKGBUILDs from VCS.

  That just leaves one question: How to get the correct pkgver in
  checkPackageDB?  The obvious answer is: Each test case knows what the
  version should be; add it as an argument, and adjust every testcase that
  calls it.

Luke Shumaker authored Jul 13, 2018 and Eli Schwartz committed Oct 07, 2018

With BATS setting up error traps, we get used to writing simple `[[ foo ]]`
assertions and not having to check the result.  However, using `!` to
invert the result DOESN'T trigger the trap.  It works OK when it is the
last command in a function, as it still affects the function's return
value, so then the trap triggers in the caller (rather than in the
function).  This means that a check may run successfully when it should
fail.

So, replace all uses of bare `! cmd` with `if cmd; then return 1; fi`,
unless it is the final statement in a function (as it is in
sourceballs.bats:__checkRemovedSourcePackage())

The mistake meant that checkRemovedPackage() was effectively equivalent to
checkRemovedPackageDB().  This meant that no one noticed that db-updates's
"add package with inconsistent name fails" test called checkRemovedPackage
instead of checkRemovedPackageDB.  Fixing the ! issue means that the test
now fails, so change which function it calls.

Levente Polyak authored Sep 03, 2018 and Eli Schwartz committed Sep 09, 2018

This silences a useless error message that confuses the user.

bsdtar doesn't like it when the stream gets closed before it finishes
which may be the case when grep found its match on potentially huge
archives. Instead of suppressing the whole stderr , we find all matches
with grep, then use a second pass with `tail` to find only the last
match, which ensures the stream remains open for bsdtar but we may still
catch and see useful messages on stderr.

This works because tail has the useful property of not closing early.

Levente Polyak authored Sep 03, 2018 and Eli Schwartz committed Sep 09, 2018

The old travis-ci.org is deprecated and this project was migrated.

Jelle van der Waa authored Aug 26, 2018 and Eli Schwartz committed Aug 26, 2018

Updating the archweb database is now handled by archweb itself and
deployed on the server using a systemd unit/service. These scripts are
no longer used.

Signed-off-by: Jelle van der Waa <jelle@vdwaa.nl>

Luke Shumaker authored Apr 04, 2018 and Eli Schwartz committed Jul 03, 2018

 - ftpdir-cleanup: ${pkg[0]} -> ${pkgs[0]}

   This mistake was introduced in 76285251

 - test: common.bash: $pkgarches[@] -> ${pkgarches[@]}

   This mistake was introduced in 76285251

Use arrays via nameref, since makepkg does not support multidimensional
arrays, and assigning to/retrieving from an array variable with an
unknown name requires this.

Requires bash 4.3, requires that architectures never contain chars that
are invalid in a variable name -- which makepkg explicitly requires in
order to support architecture-dependent sources/dependencies, and also
incorporates into linting checks which forbid anything but [[:alnum:]_]
so that's all good.

pacman 5.1 enforces this restriction. OTOH it is a simpler setup to set
this as the user homedir directly in account creation (just like
makechrootpkg has always done) than to create an additional,
world-writable, directory.

dockerfile: don't use tmpfs for /build

The print_all_package_names function changed its behavior, adapt

Luke Shumaker authored Jun 17, 2014 and Eli Schwartz committed Apr 08, 2018

This does correct handling of
 - executing a program by symlink
 - any weird characters in the full path
 - I'm sure there's another case I thought about when I originally did
   this.

Luke Shumaker authored Mar 15, 2018 and Eli Schwartz committed Apr 08, 2018

Search for unquoted variables using the command:

    grep -Prn --exclude-dir=.git '(?<!["=]|\[\[ |\[\[ -[zn] )\$(?!{?#|\(|\? )'

and ignore a bunch of false positives.

You may verify that the only differences between the lines removed and
lines added are double-quotes:

        diff -u \
          <(git show|sed -n 's/^-//p'|grep -v '^-- a/'|sed 's/"//g') \
          <(git show|sed -n 's/^+//p'|grep -v '^++ b/'|sed 's/"//g')

Luke Shumaker authored Jun 18, 2014 and Eli Schwartz committed Apr 08, 2018

This is a leftover change from the patch that became 0432cffc; v2 of that
patch broke db-functions:getpkgfiles, but the testsuite didn't catch it.
This patch adds a testcase that catches this type of breakage.

Luke Shumaker authored Jun 18, 2014 and Eli Schwartz committed Mar 21, 2018

 - db-functions: getpkgfile: The .sig file extension should be part of
   the filename parameter, not part of the message format string.

 - db-functions: arch_repo_modify: Shouldn't use ${action}
   string-interpolation in the message format string.  Since the
   entire message is a command, and we're using @Q to escape arguments
   anyway, go ahead and just construct the entire command as a single
   string that way, and feed it to '%s'.

Luke Shumaker authored Jun 18, 2014 and Eli Schwartz committed Mar 21, 2018

Using the following command to find unquoted variables (and ignoring
more than a a few false positives),

    grep -Prn --exclude-dir=.git '(?<!["=]|\[\[ |\[\[ -[zn] )\$(?!{?#|\(|\? )'

one is lead to find a few cleanups that are something other than "add
double-quotes".  That's what these are.  We'll leave dumb adding of
double-quotes for another commit.

Most of these are still fixing quoting issues, just with a better fix.

 - parse_pkgbuilds.sh: Avoid having to escape quotes in `eval` strings
   by using `declare -p`. Updates the logic copied from makepkg, with the
   latest logic copied from makepkg. See
   https://git.archlinux.org/pacman.git/commit/?id=9e52a36794552b77ecf26f7f34b226d096978f1e
 - sourceballs: Avoid using ary=($string) to do field separation by
   using `read` and test that multiple licenses actually work as
   expected.
 - sourceballs: Replace `[[ -z ${ary[*]} ]]` with test for the array
   length
 - db-functions: Replace mangling echo field separators using sed, with
   printf formatters
 - db-functions: Replace for/echo loop to print an array line by line,
   with `printf '%s\n'`
 - db-functions: set_repo_permissions: Line up error messages, quote
   "$group"
 - db-move: Replace `$(echo ${array[@]})` with `${array[*]}`
 - testing2x: Use `"$@"` instead of `$*` when looping over an array

Also, not really quoting related but on the same line as a quoting
issue, optimize:

 - db-functions: Replace
	[[ -n "$(... | sort | uniq -D)" ]]
   with
	! ... | awk 'a[$0]++{exit 1}'

Luke Shumaker authored Mar 14, 2018 and Eli Schwartz committed Mar 19, 2018

Instead, compare the value of $v to 'true'.

Luke Shumaker authored Apr 17, 2016 and Eli Schwartz committed Mar 19, 2018

(By default, prefer `grep &>/dev/null`)

`grep -q` may exit as soon as it finds a match; this is a good optimization
for when the input is a file.  However, if the input is the output of
another program, then that other program will receive SIGPIPE, and further
writes will fail.  When this happens, it might (bsdtar does) print a
message about a "write error" to stderr.  Which is going to confuse and
alarm the user.

In one of the cases (in common.bash, in the test suite), this had
already been mitigated by wrapping bsdtar in "echo "$(bsdtar ...)", as
Bash builtin echo doesn't complain if it gets SIGPIPE.  However, that
means we're storing the entire output of bsdtar in memory, which is
silly.  Additionally, the way it was implemented is also wrong;
because it was being used with `grep -qv` instead of just `grep -q`,
it *always* found a non-matching line (even something inconsequential
like `%NAME%`), and *never* triggered a test failure.

Looking at a few of these cases, it might also make sense to switch to
using `bsdtar tf` instead of `bsdtar xf` when checking membership, but
that's work for another day.

Luke Shumaker authored Apr 17, 2016 and Eli Schwartz committed Mar 19, 2018

It is a method of notifying text-editors that a file is in Bash syntax
without giving it a propper shebang (which would be confusing, as it
would suggest that the file should be executable), as well as working
across virtually all text-editors (unlike "-*- Mode: Bash -*-" or
whatever).

pkgs should be passed as a single argument

Luke Shumaker authored Feb 22, 2018 and Eli Schwartz committed Mar 15, 2018

 - ftpdir-cleanup.bats: Glob expansion does not occur in [[ -f ]] tests.
   The [[ ! -f .../${pkgname}-*${PKGEXT} ]] checks were checking that there
   were no files containing a literal '*' for that part of their name.
   Obviously, this isn't what was intended.

 - sourceballs.bats: [ -r ] checks explode if the glob returns >1 file.
   Avoid using them if the path being checked contains a glob.

Luke Shumaker authored Feb 22, 2018 and Eli Schwartz committed Mar 15, 2018

I managed to stumble across a bug in BATS where the run() function
screwed with the global IFS.  The bug has been fixed in git, but isn't
in a release yet.

https://github.com/sstephenson/bats/issues/89

Anyway, this bug breaks __getCheckSum().  Fortunately, we have avoided
tripping it so far because luck has it that we never call
__getCheckSum() after run() in the same test.

So, there's nothing actually broken here, but it makes me nervous.  So
go ahead and modify __getCheckSum to not rely on IFS.

And, while we're at it: declare the result variable and set it as
separate commands.  Doing both in the same command masks the exit code
of the subshell expansion.  We don't explicitly check the exit code,
but BATS runs the test suite with `set -e`, so splitting it does mean
that BATS will now detect errors from sha1sum.  We don't really expect
that to happen, but if BATS will give us error checking on it for
free, why not?

Luke Shumaker authored Feb 22, 2018 and Eli Schwartz committed Mar 15, 2018

These are things that were (IMO) missed in 5afac1ed.  I found them using:

    git grep -E '(plain|msg|msg2|warning|error|die) "[^"]*\$'

I went a little above-and-beyond for escaping strings for the error
messages in db-functions' arch_repo_add and arch_repo_remove.  The
code should explain itself, but I wanted to point it out, as it's more than
the usual "slap %s in there, and move the ${...} to the right".

Use less duplicated logic, opening the way to more elegantly change
the interface for the repo-add/repo-remove scripts which are themselves
symlinked together and mostly have the same interface.

The current glob `*.pkg.tar.?z` is both less restrictive and more
restrictive than makepkg, as it accepts any valid unicode character.

To be more exact, it's almost completely orthogonal to the one in makepkg.

makepkg only accepts .tar.gz, .tar.bz2, .tar.xz, .tar.lzo, .tar.lrz, and
.tar.Z and most of those fail to match against a two-char compression type.

dbscripts accepts .pkg.tar.💩z which incidentally is what I think of
cherry-picking xz and gz as supported methods.

Since this can be anything makepkg.conf accepts, it needs to be able to
match all that, unless we decide to perform additional restrictions in
which case we should still explicitly list each allowed extension. Using
bash extended globbing allows us to do this relatively painlessly.

Document the fact that this has *always* been some sort of glob, and
update the two cases where this was (not!) being evaluated by bash
[[ ... ]], to use a not-elegant-at-all proxy function is_globfile() to
evaluate globs *before* testing if they exist.

This fully removes the use of find from the codebase, leads to a
micro-optimization in a couple cases, and ensures that $PKGEXT is
consistently treated as a shell globbing character (which is important
because it is used as one).

Of the eight instances in these files:

- One was unnecessary as `cat` can natively consume all files passed to
  it and no directory traversal was in use.

- Two were unnecessary as they were hardcoded to read a single file....

- Another four were only being used to strip leading directory paths,
  and can be replaced by globstar and ${filepath##*/}

- The final two were checking the modification time of the files, and
  can be replaced with touch(1) and [[ -nt ]]. Although this introduces
  an additional temporary file, this is not such a big deal.

Don't bother emitting errors. bash doesn't show globbing errors if it
cannot read a directory to try globbing there. And the former code never
aborted on errors anyway, as without `set -o pipefail` the sort command
swallowed the return code.

Luke Shumaker authored Feb 16, 2018 and Eli Schwartz committed Feb 20, 2018

Commit b61a7148 introduced a regression that in a db-functions function
called by db-update, config:PKGEXT is treated like a fixed string, instead
of as a glob.  Because of inadequacy of the test-suite, this did not cause
a test failure.

https://lists.archlinux.org/pipermail/arch-projects/2018-February/004742.html

The broken function checks if the repo already contains a package that
matches the one being released.  This did not trigger a test-suite failure
because right after the broken check there is another check that checks for
the exact filename of the new package file.  In the test-suite, all
package files have the same extension, so the latter check always "saved"
us.

So, modify a relevant test case (a test case that verifies that we can't
release the same package twice) such that the second time we release the
package, we set PKGEXT to something different (.gz, instead of the normal
.xz).  This way, we can be sure that when db-update rejects the operation,
it's because of the glob check, not the exact-filename check.

This is made slightly tricky because the __buildPackage() test helper
routine tries to cache builds, to speed up the tests.  Without
modification, this means that it will just ignore the new PKGEXT.  So,
modify it to only use the cache if it can successfully copy build artifacts
out, in a way that respects PKGEXT.

PKGEXT is a makepkg variable referring to a fixed filename suffix, but
we were also using it to mean a bash glob referring to candidate
filenames. This is wrong, so rename it to PKGEXTS which is more
descriptive of its purpose.

Exclude the testsuite from this change, as the testsuite actually uses
PKGEXT for its intended purpose. Fix the testsuite to consistently use
PKGEXT, as it hardcoded the file extension in several cases, and pin
its value to .pkg.tar.xz