Bug#932612: git-debpush: detect unstitched gdr branch
Sean Whitton writes ("Bug#932612: git-debpush: detect unstitched gdr branch"):
> On Mon 22 Jul 2019 at 02:40PM +01, Ian Jackson wrote:
> > We could add a failing git-debpush invocation to some random gdr-test.
> > (adding the dependency and regenerating d/t/control)
>
> It would be good to have a test case, but perhaps you could take a look,
> since manipulating the test suite is much easier for you.
Willdo.
> Given that it's gathering commits, I've pushed my new series to branch
> 'series/git-debpush-check-unstitched-v2' of repo
> <https://git.spwhitton.name/dgit>.
Great, that all LGTM.
Rebased onto my master, and pushed to salsa/wip (rebasing)
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#932612: git-debpush: detect unstitched gdr branch
Hello,
On Mon 22 Jul 2019 at 02:40PM +01, Ian Jackson wrote:
> Another way to detect this would be that the branch is not ff of where
> it is going to be pushed. Indeed, it will be not ff of the local
> tracking branch for the remote. This might be a useful thing to check
> anyway: it seems undesirable to make a signed tag, and then
> predictably fall over during push.
git-tag alone doesn't check for this, so I'd be inclined not to add that
check. ISTM that git-debpush's checks should all be Debian-specific,
and we assume the user knows the standard git side of things. But I'm
not sure.
Also not helpful if you haven't fetched recently, which is I think when
this problem is most likely to occur.
(Detached heads might not count as standard git. Being behind an
upstream remote branch does.)
> But this doesn't help in the gdr case because with a detached HEAD, it
> wouldn't be pushed anywhere.
>
> I worry about a bad failure mode. You detach your head, in the middle
> of gdr work, to do some kind of something. You forget to reattach and
> instead git-debpush. Because we have always split the view, dgit on
> the git-debpush server might make a pseudomerge binding in your
> unstitched branch. If you then go away, you'll leave a history
> divergence between salsa and dgit.
>
> Re this:
>
> (i) I think but I am not sure that dgit in split view mode will still
> try to check that you are ff of something. But I think "something" is
> the last maintainer upload. So that won't stop this. And anyway we
> always have --overwrite so even that check will be gone.
>
> (ii) Maybe we should have a non-splitting "gdr" "quilt mode" for
> git-debpush, where we can avoid this problem more easily.
I'd really like to avoid this. Branch layouts which work without any
layout-specific options with dgit should ideally not lose this property
when it comes to git-debpush.
> (iii) Also you could do all of the above mid-rebase (!) I think this
> is a general problem. Certainly we should check for that. And, when
> I look at it like that I think maybe the answer is this:
>
> git-debpush should not push a detached HEAD unless you explicitly say
> HEAD or name the commitish or something. Ie the "just push what I am
> looking at" should require --force if you are not on a branch.
>
> I'm not sure whether we want even git-debpush HEAD to require --force.
> Some people may have a tendency to use that because it is "more
> explicit and therefore safer" and if we take it to mean "push HEAD
> even if it's detached" when the default is not to push detached
> things, we are interpreting them in the opposite way to what they
> meant. So I would be inclined to require --force for any case where
> what we push starts out as literally precisely HEAD (whether
> explicitly specified, or defaulted) and is detached. (So "HEAD~0"
> would not need forcing.)
>
> What do you think ?
This is easy for the user to understand and is helpful. Implemented
(and adhoc tested; it's v. simple).
> As for the code...
>
>> +# git-debrebase branch format checks
>> +
>> +# only check branches, since you can't run `git debrebase conclude` on
>> +# non-branches
>> +case "$branch" in
>> +refs/heads/*)
>> +# see "STITCHING, PSEUDO-MERGES, FFQ RECORD" in git-debrebase(5)
>> +ffq_prev_ref="${branch/refs/refs/ffq-prev}"
>
> Don't you find this very confusing ? I would have written
> +ffq_prev_ref="refs/ffq-prev/${branch#refs/}"
> Up to you.
That's better, thanks.
> Apart from that LGTM. Do you think we should have a test case ?
>
> We could add a failing git-debpush invocation to some random gdr-test.
> (adding the dependency and regenerating d/t/control)
It would be good to have a test case, but perhaps you could take a look,
since manipulating the test suite is much easier for you.
Given that it's gathering commits, I've pushed my new series to branch
'series/git-debpush-check-unstitched-v2' of repo
.
--
Sean Whitton
signature.asc
Description: PGP signature
Bug#932612: git-debpush: detect unstitched gdr branch
> On Sun 21 Jul 2019 at 03:02PM +01, Ian Jackson wrote:
> > Look for the ffq-prev ref. Any gdr unstitched branch will have such a
> > corresponding ref. See STITCHING, PSEUDO-MERGES, FFQ RECORD in
> > git-debrebase(5).
>
> Thanks. Patch enclosed.
Nice work.
LGTM. One tiny code comment. Also a big digression...
> > For a detached head, this is not determinable, and gdr conclude is not
> > possible either. So this means that git-debpush of a non-branch
> > commitish must either be a forceable fail, or must skip this check.
>
> I prefer the latter is better because I don't want to impose anything
> extra on non-gdr users.
Right.
Another way to detect this would be that the branch is not ff of where
it is going to be pushed. Indeed, it will be not ff of the local
tracking branch for the remote. This might be a useful thing to check
anyway: it seems undesirable to make a signed tag, and then
predictably fall over during push.
But this doesn't help in the gdr case because with a detached HEAD, it
wouldn't be pushed anywhere.
I worry about a bad failure mode. You detach your head, in the middle
of gdr work, to do some kind of something. You forget to reattach and
instead git-debpush. Because we have always split the view, dgit on
the git-debpush server might make a pseudomerge binding in your
unstitched branch. If you then go away, you'll leave a history
divergence between salsa and dgit.
Re this:
(i) I think but I am not sure that dgit in split view mode will still
try to check that you are ff of something. But I think "something" is
the last maintainer upload. So that won't stop this. And anyway we
always have --overwrite so even that check will be gone.
(ii) Maybe we should have a non-splitting "gdr" "quilt mode" for
git-debpush, where we can avoid this problem more easily.
(iii) Also you could do all of the above mid-rebase (!) I think this
is a general problem. Certainly we should check for that. And, when
I look at it like that I think maybe the answer is this:
git-debpush should not push a detached HEAD unless you explicitly say
HEAD or name the commitish or something. Ie the "just push what I am
looking at" should require --force if you are not on a branch.
I'm not sure whether we want even git-debpush HEAD to require --force.
Some people may have a tendency to use that because it is "more
explicit and therefore safer" and if we take it to mean "push HEAD
even if it's detached" when the default is not to push detached
things, we are interpreting them in the opposite way to what they
meant. So I would be inclined to require --force for any case where
what we push starts out as literally precisely HEAD (whether
explicitly specified, or defaulted) and is detached. (So "HEAD~0"
would not need forcing.)
What do you think ?
As for the code...
> +# git-debrebase branch format checks
> +
> +# only check branches, since you can't run `git debrebase conclude` on
> +# non-branches
> +case "$branch" in
> +refs/heads/*)
> +# see "STITCHING, PSEUDO-MERGES, FFQ RECORD" in git-debrebase(5)
> +ffq_prev_ref="${branch/refs/refs/ffq-prev}"
Don't you find this very confusing ? I would have written
+ffq_prev_ref="refs/ffq-prev/${branch#refs/}"
Up to you.
Apart from that LGTM. Do you think we should have a test case ?
We could add a failing git-debpush invocation to some random gdr-test.
(adding the dependency and regenerating d/t/control)
Ian.
Bug#932612: git-debpush: detect unstitched gdr branch
control: tag -1 +patch
Hello,
On Sun 21 Jul 2019 at 03:02PM +01, Ian Jackson wrote:
> Sean Whitton writes ("Bug#932612: git-debpush: detect unstitched gdr branch"):
>> Package: git-debpush
>> Version: 9.4
>> Severity: wishlist
> ...
>> There should be a check for an unstitched gdr branch. Ian, can you
>> chime in with how this ought to be detected, please?
>
> Look for the ffq-prev ref. Any gdr unstitched branch will have such a
> corresponding ref. See STITCHING, PSEUDO-MERGES, FFQ RECORD in
> git-debrebase(5).
Thanks. Patch enclosed.
> For a detached head, this is not determinable, and gdr conclude is not
> possible either. So this means that git-debpush of a non-branch
> commitish must either be a forceable fail, or must skip this check.
I prefer the latter is better because I don't want to impose anything
extra on non-gdr users.
--
Sean Whitton
From ff37177f4dca9a9b19ad9f036660ff8908e51506 Mon Sep 17 00:00:00 2001
From: Sean Whitton
Date: Mon, 22 Jul 2019 10:39:56 +0100
Subject: [PATCH] git-debpush: Check for unstitched git-debrebase branch
Signed-off-by: Sean Whitton
---
git-debpush | 14 ++
git-debpush.1.pod | 5 +
2 files changed, 19 insertions(+)
diff --git a/git-debpush b/git-debpush
index 982aa752..67d8feee 100755
--- a/git-debpush
+++ b/git-debpush
@@ -414,6 +414,20 @@ case "$quilt_mode" in
;;
esac
+# git-debrebase branch format checks
+
+# only check branches, since you can't run `git debrebase conclude` on
+# non-branches
+case "$branch" in
+refs/heads/*)
+# see "STITCHING, PSEUDO-MERGES, FFQ RECORD" in git-debrebase(5)
+ffq_prev_ref="${branch/refs/refs/ffq-prev}"
+if git show-ref --quiet --verify "$ffq_prev_ref"; then
+fail_check unstitched \
+ "this looks like an unstitched git-debrebase branch, which should not be pushed"
+fi
+esac
+
# Summary
if $failed_check; then
diff --git a/git-debpush.1.pod b/git-debpush.1.pod
index dabc18f7..42e3470f 100644
--- a/git-debpush.1.pod
+++ b/git-debpush.1.pod
@@ -236,6 +236,11 @@ Ignore apparently pushing the dgit view of a package (as produced by
B) to the maintainer branch, where the dgit view and the
maintainer view of the package are not identical.
+=item B
+
+Ignore the fact that the branch to be pushed seems to be a
+git-debrebase(1) branch in an unstitched state (see git-debrebase(5)).
+
=back
=back
--
2.20.1
signature.asc
Description: PGP signature
Bug#932612: git-debpush: detect unstitched gdr branch
Sean Whitton writes ("Bug#932612: git-debpush: detect unstitched gdr branch"):
> Package: git-debpush
> Version: 9.4
> Severity: wishlist
...
> There should be a check for an unstitched gdr branch. Ian, can you
> chime in with how this ought to be detected, please?
Look for the ffq-prev ref. Any gdr unstitched branch will have such a
corresponding ref. See STITCHING, PSEUDO-MERGES, FFQ RECORD in
git-debrebase(5).
For a detached head, this is not determinable, and gdr conclude is not
possible either. So this means that git-debpush of a non-branch
commitish must either be a forceable fail, or must skip this check.
> git-debpush should absolutely not run `git debrebase conclude` on the
> user's behalf before pushing, because neither git-debpush nor tag2upload
> should *ever* make commits to the maintainer's branch (the gdr stitch is
> on the maintainer's branch even in split view).
Right.
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#932612: git-debpush: detect unstitched gdr branch
Package: git-debpush Version: 9.4 Severity: wishlist Hello, There should be a check for an unstitched gdr branch. Ian, can you chime in with how this ought to be detected, please? git-debpush should absolutely not run `git debrebase conclude` on the user's behalf before pushing, because neither git-debpush nor tag2upload should *ever* make commits to the maintainer's branch (the gdr stitch is on the maintainer's branch even in split view). -- Sean Whitton signature.asc Description: PGP signature
