Bug#932477: git-debpush checking that patches (un)apply

[Sean Whitton]

Hello,

On Mon 22 Jul 2019 at 02:15PM +01, Ian Jackson wrote:

> 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.

>> +if [ -s "debian/patches/series" ]; then
>> +while read patch; do
> ...
>> +# 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.

> So I would fix this by setting git_apply_rc to 0 before the loop.

Yes.

>> +=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).

> 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 .

-- 
Sean Whitton


signature.asc
Description: PGP signature

Bug#932477: git-debpush checking that patches (un)apply

[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.

> 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.

> > 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.

> +# 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
> +git checkout "$branch_commit" -- debian/patches ||:

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.

> +if [ -s "debian/patches/series" ]; then
> +while read patch; do
...
> +# 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.

> +=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.

> +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.

Bug#932477: git-debpush checking that patches (un)apply

[Sean Whitton]

Hello,

On Sun 21 Jul 2019 at 06:05PM +01, Ian Jackson wrote:

> Sean Whitton writes ("Bug#932477: git-debpush checking that patches 
> (un)apply"):
>> 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>.

> 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.

> 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


signature.asc
Description: PGP signature

Bug#932477: git-debpush checking that patches (un)apply

[Sean Whitton]

Hello,

On Sun 21 Jul 2019 at 04:25PM +01, Ian Jackson wrote:

> Sean Whitton writes ("Re: Bug#932477: git-debpush checking that patches 
> (un)apply"):
>> 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 

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

Signed-off-by: Sean Whitton 

> 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


signature.asc
Description: PGP signature

Bug#932477: git-debpush checking that patches (un)apply

[Ian Jackson]

Sean Whitton writes ("Bug#932477: git-debpush checking that patches (un)apply"):
> 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.

> 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?

Yes.

> - 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.

> +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  +}

This seems like a very long way of writing a very short sed rune ?

> +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
> +git checkout "$branch_committish" -- debian/patches ||:
> +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
?

> +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 ?

> +# 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 :-).

> 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
> +
> +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.

> +=item B
> +
> +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.

Bug#932477: git-debpush checking that patches (un)apply

[Ian Jackson]

Ian Jackson writes ("Re: Bug#932477: git-debpush checking that patches 
(un)apply"):
> The result is in salsa/wip.dpp.  That's a rebase of your branch.
...
> 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.

Bug#932477: git-debpush checking that patches (un)apply

[Ian Jackson]

> 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 JacksonThese 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.

Bug#932477: git-debpush checking that patches (un)apply

[Ian Jackson]

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.

Bug#932477: git-debpush checking that patches (un)apply

[Ian Jackson]

Sean Whitton writes ("Re: Bug#932477: git-debpush checking that patches 
(un)apply"):
> 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.)

> 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 JacksonThese 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.

Bug#932477: git-debpush checking that patches (un)apply

[Sean Whitton]

control: tag -1 +patch

Hello,

On Fri 19 Jul 2019 at 10:55PM +01, Ian Jackson wrote:

> 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 .

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


signature.asc
Description: PGP signature

Bug#932477: git-debpush checking that patches (un)apply

[Ian Jackson]

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 JacksonThese 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.