Discussion:
Bug#932477: git-debpush checking that patches (un)apply
Add Reply
Ian Jackson
2019-07-19 22:00:01 UTC
Reply
Permalink
Package: git-debpush
Version: 9.3

It would be nice to check that the patches are right. (In
gbp/unapplied, that they apply and are up to date.)

This would involve git-apply, at the very least. Running gbp pq would
add an unwanted dependency and also it is very slow. The main
reason dgit and gdr use gbp pq is to do the weird DEP-3 Debian patch
metadata thing (which should probably be abolished in Debian
completely...) - but this is not needed for a "do they apply" check.
dgit has the absurd fallback to dpkg-source patch application (git
grep "absurd") too but I think we can simply refuse to work with
packages whose patches can't be git-applied.

For dpm and nofix it might be possible to check that the patches are
right by reverse applying them in reverse order, and expecting the
resulting tree to be identical to upstream.

linear is probably too hard to check in the general case.

Unfortunately all of this really needs a playtree and we don't want
git-debpush to grow a reference to Dgit.pm, so before we can do this
we want to convert playtree_setup to shell (and put a copy in each of
the .debs, like we do with Dgit.pm).

Ian.
--
Ian Jackson <***@chiark.greenend.org.uk> These opinions are my own.

If I emailed you from an address @fyvzl.net or @evade.org.uk, that is
a private address which bypasses my fierce spamfilter.
Sean Whitton
2019-07-21 11:00:02 UTC
Reply
Permalink
control: tag -1 +patch

Hello,
Post by Ian Jackson
It would be nice to check that the patches are right. (In
gbp/unapplied, that they apply and are up to date.)
This would involve git-apply, at the very least. Running gbp pq would
add an unwanted dependency and also it is very slow. The main
reason dgit and gdr use gbp pq is to do the weird DEP-3 Debian patch
metadata thing (which should probably be abolished in Debian
completely...) - but this is not needed for a "do they apply" check.
dgit has the absurd fallback to dpkg-source patch application (git
grep "absurd") too but I think we can simply refuse to work with
packages whose patches can't be git-applied.
For dpm and nofix it might be possible to check that the patches are
right by reverse applying them in reverse order, and expecting the
resulting tree to be identical to upstream.
I've implemented this in branch 'series/git-debpush-apply-patches-v1' of
repo <https://git.spwhitton.name/dgit>.

Regarding the dpm and nofix check (which I'm less sure about than the
gbp&unapplied check):

- should we always expect to get the unmodified upstream source just by
unapplying patches?

- should the two calls to check_fail in check_patches_unapply be
distinct named checks?

- needs a proper test case. I have ad hoc tested with nofix.

The four tagupl tests pass if you hack the full path to
git-playtree-create into the script -- it seems that using-intree has
not been taught to make git-playtree-create available to git-debpush in
the right way.
--
Sean Whitton
Ian Jackson
2019-07-21 15:30:02 UTC
Reply
Permalink
Post by Sean Whitton
I've implemented this in branch 'series/git-debpush-apply-patches-v1' of
repo <https://git.spwhitton.name/dgit>.
Thanks. Looking at it now.

I am folding your fixup! commits into my branch. I will add your
S-o-b to those too. That means I can't push this revised branch until
I hear from you that that is what you intended .... (Next time can
you please squaash! and provide your own S-o-b and then I won't have
to ask.)
Post by Sean Whitton
The four tagupl tests pass if you hack the full path to
git-playtree-create into the script -- it seems that using-intree has
not been taught to make git-playtree-create available to git-debpush in
the right way.
Oops. I will fix this.

Ian.
--
Ian Jackson <***@chiark.greenend.org.uk> These opinions are my own.

If I emailed you from an address @fyvzl.net or @evade.org.uk, that is
a private address which bypasses my fierce spamfilter.
Sean Whitton
2019-07-21 18:10:02 UTC
Reply
Permalink
Hello,
Post by Ian Jackson
Post by Sean Whitton
I've implemented this in branch 'series/git-debpush-apply-patches-v1' of
repo <https://git.spwhitton.name/dgit>.
Thanks. Looking at it now.
I am folding your fixup! commits into my branch. I will add your
S-o-b to those too.
754eb51a * fixup! git-playtree-setup: Provide to git-debpush
b0d26dd0 * git-playtree-setup: Provide to git-debpush

Signed-off-by: Sean Whitton <***@spwhitton.name>

bb6c28d7 * fixup! git-playtree-setup: Rewrite in shell and call it from Perl
b65a6e88 * git-playtree-setup: Rewrite in shell and call it from Perl
Post by Ian Jackson
That means I can't push this revised branch until I hear from you that
that is what you intended .... (Next time can you please squaash! and
provide your own S-o-b and then I won't have to ask.)
Good point.
--
Sean Whitton
Ian Jackson
2019-07-21 15:40:02 UTC
Reply
Permalink
Re
git-debpush: check_treesame: Also pass --quiet to 'git diff'

When dgit fails for similar reasons we have it print a diffstat and
also print a git diff rune so that the user can see what has changed.

See near l.4598 in dgit. We probably don't want something that
sophisticated but maybe something a bit simpler can be done that would
be helpful.

Ian.
Ian Jackson
2019-07-21 16:00:01 UTC
Reply
Permalink
Post by Sean Whitton
The four tagupl tests pass if you hack the full path to
git-playtree-create into the script -- it seems that using-intree has
not been taught to make git-playtree-create available to git-debpush in
the right way.
I have fixed this. I have also renamed git-playtree-create to
git-playtree-setup (sorry).

The result is in salsa/wip.dpp. That's a rebase of your branch.
I have:
- folded in your two fixup!s with your s-o-b added.
if this is not what you meant please delete the wip.dpp branch
and let me know
- added my own commits to fix the using-intree issue and the
rename. One rename commit below your work and one squash!
above it to be folded into your two commits
- included your --quiet

I ran the tagupl test and it passes now without a hack.
I have not yet reviewed your substantive patch.

FYI I intend to push to master everything up to and including
salsa/wip. So don't rebase below there.

Everything above there I hereby pass back to you and you can do what
you like to it. It consists of the top of your banch with my rename
commit on top. (So all I did to your branch was steal the fixup!s
from the bottom and add my rename on top.)

Lunch now...

Ian.
--
Ian Jackson <***@chiark.greenend.org.uk> These opinions are my own.

If I emailed you from an address @fyvzl.net or @evade.org.uk, that is
a private address which bypasses my fierce spamfilter.
Ian Jackson
2019-07-21 17:10:01 UTC
Reply
Permalink
Post by Ian Jackson
The result is in salsa/wip.dpp. That's a rebase of your branch.
...
Post by Ian Jackson
FYI I intend to push to master everything up to and including
salsa/wip. So don't rebase below there.
I have had to force push both salsa/wip.dpp and salsa/wip because I
discovered that the actual rename of the script had ended up in the
wrong commit.

Ian.
Ian Jackson
2019-07-21 17:10:01 UTC
Reply
Permalink
Post by Sean Whitton
I've implemented this in branch 'series/git-debpush-apply-patches-v1' of
repo <https://git.spwhitton.name/dgit>.
Hi. I've reviewed your substantive code. Looking reasonably good but
I do have some nontrivial comments.
Post by Sean Whitton
Regarding the dpm and nofix check (which I'm less sure about than the
- should we always expect to get the unmodified upstream source just by
unapplying patches?
Yes.
Post by Sean Whitton
- should the two calls to check_fail in check_patches_unapply be
distinct named checks?
See below.

What I am reviewing here is the combination of:
fixup! git-debpush: Check that patches are (un)applicable
git-debpush: Check that patches are (un)applicable
squashed together.
Post by Sean Whitton
+get_quilt_series () {
+ if ! [ -s "debian/patches/series" ]; then return 0; fi
+ while read patch; do
+ shopt -s extglob; patch="${patch%%?( )#*}"; shopt -u extglob
+ if ! [ -z "$patch" ]; then
+ echo "$patch";
+ fi
+ done <debian/patches/series
+}
This seems like a very long way of writing a very short sed rune ?
Post by Sean Whitton
+check_patches_apply () {
+ local playtree="gdp/apply-patches"
+ local git_apply_rc
+
+ # checking out the upstream source and then d/patches on top
+ # ensures this check will work for gbp, unapplied and also
+ # baredebian
+ setup_playtree "$playtree" "$upstream_committish"
+ # git-checkout will fail if there is no d/patches, which is fine
+ get_quilt_series >../series
+ while read patch; do
+ set +e
+ git apply "debian/patches/$patch"
+ git_apply_rc=$?
+ set -e
+ if ! [ $git_apply_rc = 0 ]; then
+ fail_check patches-applicable \
+ "'git apply' failed to apply patch $patch"
+ break
+ fi
+ done <../series
This separation of the <../series from while makes it a bit harder to
follow. Maybe write one of
+ <../series while read patch; do
+ while read patch <../series; do
?
Post by Sean Whitton
+check_patches_unapply () {
+ local playtree="gdp/unapply-patches"
+ local git_apply_rc
This function is a largely a clone and hack of the previous one. Can
you combine them ?
Post by Sean Whitton
+# resolve $branch to a committish
+branch_committish="$(git rev-parse --verify ${branch}^{commit})"
I think this is spelled "commitish". git(1) spells it "commit-ish".
Hrm, I see that you are consistent about this in git-debpush and I am
differently consistent about it everywhere else. Not sure I want to
argue...

But more to the point you are here precisely turning a commitish into
a commit id so the name is just wrong :-).
Post by Sean Whitton
diff --git a/git-debpush.1.pod b/git-debpush.1.pod
index a554fd56..e73f0092 100644
--- a/git-debpush.1.pod
+++ b/git-debpush.1.pod
@@ -206,6 +206,18 @@ Ignore any differences between the upstream source in the upstream tag
and the upstream source in the branch to be tagged (this check is only
run when using B<--quilt=gbp> or B<--quilt=unapplied>).
+=item B<patches-applicable>
+
+Ignore the fact that git-apply(1) cannot apply all the quilt patches
+to the upstream source (this check is only run when using
+B<--quilt=baredebian>, B<--quilt=gbp> or B<--quilt=unapplied>).
Maybe it would be better to phrase this in terms of git-debpush's
expectation.

With --quilt=gbp and --quilt=unapplied, the quilt patches should
apply cleanly to the upstream source.
Post by Sean Whitton
+=item B<patches-reverse-applicable>
+
+Ignore the fact that git-apply(1) cannot reverse-apply all the quilt
+patches to produce the unmodified upstream source (this check is only
+run when using B<--quilt=dpm> or B<--quilt=nofix>).
It just occurs to me now that we could alternatively apply the
specified patches to the upstream, since we have been provided with
the upstream tree. So maybe add --quilt=dpm and nofix to the above
and say also:

With --quilt=dpm and --quilt-nofix, applying the quilt patches
to upstream should produce exactly what you are tagging/pushing.

I guess the "patches apply" is one fail check name then ?

Thanks,
Ian.
Sean Whitton
2019-07-22 09:20:01 UTC
Reply
Permalink
Hello,
Post by Ian Jackson
Post by Sean Whitton
I've implemented this in branch 'series/git-debpush-apply-patches-v1' of
repo <https://git.spwhitton.name/dgit>.
Hi. I've reviewed your substantive code. Looking reasonably good but
I do have some nontrivial comments.
Thank you for a useful review.

Revised series is in branch 'series/git-debpush-apply-patches-v2' of
repo <https://git.spwhitton.name/dgit>.
Post by Ian Jackson
This seems like a very long way of writing a very short sed rune ?
Now that there's only one while loop, I don't believe this comment
applies.

In the revised series, I find the explicit -z check (l. 146) more
readable than avoiding that check by means of sed. So I'd like to keep
the use of extglob.
Post by Ian Jackson
This separation of the <../series from while makes it a bit harder to
follow. Maybe write one of
+ <../series while read patch; do
+ while read patch <../series; do
?
The first one is a syntax error and the second one feeds the first line
of ../series to the loop over and over. So unless we are willing to
spawn an additional process by doing `cat ../series | while` (I'd rather
not), I think we have to live with this.
--
Sean Whitton
Ian Jackson
2019-07-22 13:20:01 UTC
Reply
Permalink
Post by Sean Whitton
Post by Ian Jackson
This seems like a very long way of writing a very short sed rune ?
Now that there's only one while loop, I don't believe this comment
applies.
I agree.
Post by Sean Whitton
In the revised series, I find the explicit -z check (l. 146) more
readable than avoiding that check by means of sed. So I'd like to keep
the use of extglob.
Yes, I agree.
Post by Sean Whitton
Post by Ian Jackson
This separation of the <../series from while makes it a bit harder to
follow. Maybe write one of
+ <../series while read patch; do
+ while read patch <../series; do
?
The first one is a syntax error and the second one feeds the first line
of ../series to the loop over and over. So unless we are willing to
spawn an additional process by doing `cat ../series | while` (I'd rather
not), I think we have to live with this.
Oops. TIL. Sorry for the noise.

I'm afraid I still have some niggles.
Post by Sean Whitton
+ # checking out the upstream source and then d/patches on top
+ # ensures this check will work for a variety of quilt modes
+ git checkout -b upstream "$upstream_committish"
+ # git-checkout will fail if there is no d/patches, which is fine
Unfortunately this has rather poor error handling.
- In a common case (no d/patches) it will print an
unwanted message to stderr
- If git explodes for some other reason (ENOSPC) it will blunder on

How about getting all of debian/ ? That will definitely be present
and it is also more regular because it's the same way other
dpkg-source (and so all other tools) combine upstream and packaging.
Post by Sean Whitton
+ if [ -s "debian/patches/series" ]; then
+ while read patch; do
...
Post by Sean Whitton
+ # if $git_apply_rc is 0 that indicates both that patch application
+ # succeeded and that there were actually patches to apply
+ if $should_match_branch && [ $git_apply_rc = 0 ]; then
I think this test is a syntax error if git_apply_rc is unset as it
will be if there were no patches.

But, I don't think an empty patch series is special here. Indeed in
some quilt modes one possible common failure case will be "forgot to
generate the patches".

So I would fix this by setting git_apply_rc to 0 before the loop.
Post by Sean Whitton
+=item
+
+With B<--quilt=dpm> and B<--quilt=nofix>, applying the quilt patches
+to the upstream source should produce exactly the upstream source to
+be tagged.
This is confusing. How about "produce exactly the package to be
tagged". That glosses over debian/ but that's fine in this context.
Post by Sean Whitton
+t-expect-fail "'git apply' failed to apply patch 0002-Edit-the-.c-file.patch ('patches-applicable' check)" \
+t-tagupl-test --baredebian
+
+git reset --hard HEAD~1
It would be nice to have a test for the nofix/dpm branch equal check,
but currently we are lacking a test case for that enitrely. (Because,
previously, I convinced myself it was not a special case and the
general dgit tests would cover it.)

I'll see if I can produce a test case for this based on one of the gdr
setup things.

At some point you or I will want to rebase this onto master. It's
getting a bit behind... (This is good, right?)

Ian.
Sean Whitton
2019-07-22 14:50:02 UTC
Reply
Permalink
Hello,
Post by Ian Jackson
Unfortunately this has rather poor error handling.
- In a common case (no d/patches) it will print an
unwanted message to stderr
- If git explodes for some other reason (ENOSPC) it will blunder on
How about getting all of debian/ ? That will definitely be present
and it is also more regular because it's the same way other
dpkg-source (and so all other tools) combine upstream and packaging.
That's much better.
Post by Ian Jackson
Post by Sean Whitton
+ if [ -s "debian/patches/series" ]; then
+ while read patch; do
...
Post by Sean Whitton
+ # if $git_apply_rc is 0 that indicates both that patch application
+ # succeeded and that there were actually patches to apply
+ if $should_match_branch && [ $git_apply_rc = 0 ]; then
I think this test is a syntax error if git_apply_rc is unset as it
will be if there were no patches.
But, I don't think an empty patch series is special here. Indeed in
some quilt modes one possible common failure case will be "forgot to
generate the patches".
Good point.
Post by Ian Jackson
So I would fix this by setting git_apply_rc to 0 before the loop.
Yes.
Post by Ian Jackson
Post by Sean Whitton
+=item
+
+With B<--quilt=dpm> and B<--quilt=nofix>, applying the quilt patches
+to the upstream source should produce exactly the upstream source to
+be tagged.
This is confusing. How about "produce exactly the package to be
tagged". That glosses over debian/ but that's fine in this context.
'package' could only mean 'source package' ...

I've put 'source tree' ('tree' alone seems too gittish).
Post by Ian Jackson
At some point you or I will want to rebase this onto master. It's
getting a bit behind... (This is good, right?)
Indeed! I've rebased onto salsa/master and pushed branch
'series/git-debpush-apply-patches-v3' to <https://git.spwhitton.name/dgit>.
--
Sean Whitton
Loading...