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