Package: release.debian.org
Severity: normal
Tags: bookworm
User: release.debian....@packages.debian.org
Usertags: pu
X-Debbugs-Cc: d...@packages.debian.org
Control: affects -1 + src:dgit
[ Reason ]
Two users separately disscovered a misssing safety catch in dgit:
dgit push won't notice if the changelog for your upload states a
version which is lower or the same as exists in the archive (unless
the currently-being-pushed version wasn't itself previously uploaded
with dgit).
https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=1050711
[ Impact ]
Users using dgit from stable might make broken uploads. Typically,
this happens when they're unintentionally uploading some modifications
which were erroneously based on an out-of-date version of the package.
Such an upload is then rejected by the ftpmaster archive. But
misleading git tags have been made and published. Additionally, the
broken upload remains in the dgit history, and mighht end up as noise
in maintainer git histories.
None of this is a violation of the constraints of the dgit data model,
but:
- It can be very confusing to humans.
- Some maintainers really hate this kind of git noise,
so this malfunction can generate social friction.
- If the uploader doesn't notice, their intended changes will
not actually end up in the target Debian suite.
[ Tests ]
I have added a test for this situation to the test suite. That test
fails before the fix, and passes afterwards. The test is part of the
autopkgtests.
When backporting the relevant commits to the bookworm branch, I chose
to include the new test. I have done a formal local run of the
autopkgtests for 10.7+deb12u2. The test runs and passes as expected.
Additionally, I built and installed 10.7+deb12u2 locally, and ran an
ad-hoc manual test with dgit-test-dummy (which I deliberately put into
this anomalous state for testing this bug).
[ Risks ]
The logic of the check might be wrong. There's a boolean condition,
and a version number inequality comparison. However, I think the code
is right because of the new test case. and also because adding this
check only broke two of the existing tests, which, on inspection, did
indeed play fast and loose with versions.
Some downstreams might have workflows that trip the new check.
There is a a --force option for it, but no configuration setting.
This could affect downstreams who are running Debian stable to manage
some other set of packages; but it would only affect downstreams who
are working with source packages *and* routinely reuse (or rewind) source
package versions. I doubt other tooling, besides dgit, would work
well if you did that; for example, apt's handling of the resulting
debs would be poor.
The new test case is brand new and might be wrong. Also, I had to
resolve a conflict in d/tests/control (which I did by regenerating
it). So perhaps the autopkgtests might be broken. However, I have
run them with bookworm's autopkgtest(1), and the overall impact of any
such breakage seems like ti would be low.
[ Checklist ]
[x] *all* changes are documented in the d/changelog
[x] I reviewed all changes and I approve them
[x] attach debdiff against the package in (old)stable
[x] the issue is verified as fixed in unstable
[ Changes ]
* Add the new check to dgit's "dopush" function
* Provide a way to override the check
* Add the override to two test cases that play fast and loose
* Add a new test case for this specific situation
Some more information is available in the commit messages,
which can be found here:
https://salsa.debian.org/dgit-team/dgit/-/commits/bookworm/
[ Other info ]
Two users experienced lossage due this missing check in the same week:
See #1050711 and #1050924. I don't know why this is, but perhaps
we are seeing more parttial adoption of dgit within some teams.
Both of these users found this dgit behaviour disturbing, and were
significantly inconvenienced; in particular, both were doing
authorised team uploads, and were concerned that the malfunction might
upset the principal maintainers of the respective packages.
diff --git a/debian/changelog b/debian/changelog
index bf03d2744..55aca1076 100644
--- a/debian/changelog
+++ b/debian/changelog
@@ -1,3 +1,11 @@
+dgit (10.7+deb12u2) unstable; urgency=medium
+
+ * Prevent pushing older versions than is in the archive.
+ Closes: #1050711. [Reports from Helmut Grohne and Phil Hands]
+ Backported from dgit 11.3.
+
+ -- Ian Jackson <ijack...@chiark.greenend.org.uk> Sun, 03 Sep 2023 00:49:57
+0100
+
dgit (10.7+deb12u1) bookworm; urgency=medium
* Use the old /updates security map only for buster. Fixes fetching from
diff --git a/debian/tests/control b/debian/tests/control
index a22400b17..99ef53414 100644
--- a/debian/tests/control
+++ b/debian/tests/control
@@ -100,7 +100,7 @@ Tests: trustingpolicy-replay
Tests-Directory: tests/tests
Depends: dgit, dgit-infrastructure, devscripts, debhelper (>=8), fakeroot,
build-essential, chiark-utils-bin, bc, faketime, liburi-perl, dput-ng
-Tests: absurd-gitapply badcommit-rewrite build-modes build-modes-long
build-modes-source checkout clone-clogsigpipe debpolicy-dbretry
debpolicy-newreject debpolicy-quilt-gbp debpolicy-taintrm defdistro-rpush
defdistro-setup distropatches-reject dpkgsourceignores-correct
drs-push-masterupdate drs-push-rejects dsd-divert fetch-localgitonly
fetch-somegit-notlast forcesplit-linear forcesplit-overwrite gbp-orig gitconfig
gitworktree import-dsc import-maintmangle import-native import-nonnative
import-tarbomb inarchivecopy mismatches-contents mismatches-dscchanges
multisuite orig-include-exclude orig-include-exclude-chkquery overwrite-chkclog
overwrite-junk overwrite-splitbrains overwrite-version pbuilder protocol-compat
push-buildproductsdir push-newpackage push-newrepeat push-nextdgit push-source
push-source-with-changes quilt quilt-gbp quilt-gbp-build-modes
quilt-include-binaries quilt-singlepatch quilt-splitbrains quilt-useremail
rpush rpush-quilt rpush-source sourceonlypolicy tag-updates unrepresentable
unrepresentable-single-dpkg unrepresentable-single-git version-opt
+Tests: absurd-gitapply badcommit-rewrite build-modes build-modes-long
build-modes-source checkout clone-clogsigpipe debpolicy-dbretry
debpolicy-newreject debpolicy-quilt-gbp debpolicy-taintrm defdistro-rpush
defdistro-setup distropatches-reject dpkgsourceignores-correct
drs-push-masterupdate drs-push-rejects dsd-divert fetch-localgitonly
fetch-somegit-notlast forcesplit-linear forcesplit-overwrite gbp-orig gitconfig
gitworktree import-dsc import-maintmangle import-native import-nonnative
import-pushold import-tarbomb inarchivecopy mismatches-contents
mismatches-dscchanges multisuite orig-include-exclude
orig-include-exclude-chkquery overwrite-chkclog overwrite-junk
overwrite-splitbrains overwrite-version pbuilder protocol-compat
push-buildproductsdir push-newpackage push-newrepeat push-nextdgit push-source
push-source-with-changes quilt quilt-gbp quilt-gbp-build-modes
quilt-include-binaries quilt-singlepatch quilt-splitbrains quilt-useremail
rpush rpush-quilt rpush-source sourceonlypolicy tag-updates unrepresentable
unrepresentable-single-dpkg unrepresentable-single-git version-opt
Tests-Directory: tests/tests
Depends: dgit, dgit-infrastructure, devscripts, debhelper (>=8), fakeroot,
build-essential, chiark-utils-bin, bc, faketime, liburi-perl
diff --git a/dgit b/dgit
index 541420921..dd2b301a6 100755
--- a/dgit
+++ b/dgit
@@ -103,7 +103,7 @@ our $chase_dsc_distro=1;
our %forceopts = map { $_=>0 }
qw(unrepresentable unsupported-source-format
dsc-changes-mismatch changes-origs-exactly
- uploading-binaries uploading-source-only
+ uploading-binaries uploading-old-version uploading-source-only
reusing-version
push-tainted
import-gitapply-absurd
@@ -4680,6 +4680,7 @@ END
git_fetch_us();
}
my $archive_hash = fetch_from_archive();
+ my $archive_dsc = $dsc;
if (!$archive_hash) {
$new_package or
fail __ "package appears to be new in this suite;".
@@ -4737,6 +4738,16 @@ END
my $upstreamversion = upstreamversion $clogp->{Version};
+ if (defined $archive_dsc &&
+ version_compare($archive_dsc->{Version}, $cversion) >= 0 &&
+ !forceing [qw(uploading-old-version)]) {
+ fail f_ <<'END', $archive_dsc->{Version}, $csuite, $cversion;
+You seem to be trying to push an old version.
+Version current in archive: %s (in suite %s)
+Version you are trying to upload: %s
+END
+ }
+
if (madformat_wantfixup($format)) {
# user might have not used dgit build, so maybe do this now:
if (do_split_brain()) {
diff --git a/tests/tests/dpkgsourceignores-correct
b/tests/tests/dpkgsourceignores-correct
index f71c3a46e..f3d70fa7b 100755
--- a/tests/tests/dpkgsourceignores-correct
+++ b/tests/tests/dpkgsourceignores-correct
@@ -48,6 +48,6 @@ git add .
git commit -m 'want these'
t-dgit --quilt=smash -wgf build-source
-t-dgit -wgf push-built
+t-dgit --force-uploading-old-version -wgf push-built
t-ok
diff --git a/tests/tests/gitworktree b/tests/tests/gitworktree
index e0f0e0d7e..e8963b70b 100755
--- a/tests/tests/gitworktree
+++ b/tests/tests/gitworktree
@@ -22,6 +22,6 @@ git add modification
git commit -m 'want this'
t-dgit -wgf quilt-fixup
-t-dgit -wgf --quilt=nofix push-source
+t-dgit -wgf --quilt=nofix push-source --force-uploading-old-version
t-ok
diff --git a/tests/tests/import-pushold b/tests/tests/import-pushold
new file mode 100755
index 000000000..ed73fa34c
--- /dev/null
+++ b/tests/tests/import-pushold
@@ -0,0 +1,37 @@
+#!/bin/bash
+set -e
+. tests/lib
+
+t-tstunt-parsechangelog
+
+p=example
+old=1.0-1
+current=1.0-1.100
+
+t-archive $p $current
+t-git-none
+
+mkdir $p
+cd $p
+git init
+t-dgit import-dsc $troot/pkg-srcs/${p}_${old}.dsc main
+git checkout main
+
+t-dgit fetch
+
+: "attempt push of old"
+
+t-expect-fail 'trying to push an old version' \
+t-dgit push-source --deliberately-not-fast-forward --overwrite
+
+: "attempt push of current"
+
+t-dgit checkout sid
+
+t-expect-fail 'trying to push an old version' \
+t-dgit push-source --deliberately-not-fast-forward --overwrite
+
+t-dgit push-source --force-uploading-old-version
+t-pushed-good dgit/sid
+
+t-ok
