Make git push fail gracefully with nonzero exit status if upstream hg crashes or rejects push #342

riastradh · 2024-12-09T04:45:35Z

Intended to fix the following three issues:

git push exits with zero status when hg rejects push with error:abort #341
cinnabar crashes with core dump rather than failing gracefully if upstream hg rejects push #338
~~push-refs.t assumes ~/.gitconfig sets user.name #339~~ (moved to tests/push-refs.t: Set committer name when making a tag. #343)

Not 100% sure about the logic here in remote_helper_push, but it seems to produce the desired effect in the test cases. In particular, the mechanism for propagating an error through (resetting pushed to empty) seems questionable, and it's not immediately clear to me what to do about multi-part bundlev2 responses.

glandium · 2024-12-10T08:59:22Z

Thank you for the patches. Could you add your tests to tests/push.t instead of adding separate tests?

glandium

Would you mind doing a separate PR for this one?

Edit: of course, github doesn't attach that to the commit I added the comment from: d5438e7

riastradh · 2024-12-10T17:49:46Z

Thank you for the patches. Could you add your tests to tests/push.t instead of adding separate tests?

I can do that if you like, but push.t is already a long narrative which doesn't seem to involve anything that makes commits in git and pushes them to hg. So it wasn't obvious where to thread these test cases into the narrative -- it looked to me like the wrong place to put these tests altogether; likewise push-refs.t.

Are you sure you want me to put these cases in push.t? Should I put them at the end? Reset the repositories first so the history isn't cluttered with extra stuff not germane to whether or not the git commit went through?

riastradh · 2024-12-10T17:50:12Z

Would you mind doing a separate PR for this one?

Done: #343

glandium · 2024-12-11T00:52:10Z

Thank you for the patches. Could you add your tests to tests/push.t instead of adding separate tests?

I can do that if you like, but push.t is already a long narrative which doesn't seem to involve anything that makes commits in git and pushes them to hg. So it wasn't obvious where to thread these test cases into the narrative -- it looked to me like the wrong place to put these tests altogether; likewise push-refs.t.

Are you sure you want me to put these cases in push.t? Should I put them at the end? Reset the repositories first so the history isn't cluttered with extra stuff not germane to whether or not the git commit went through?

These tests use the history that was pulled from mercurial instead of creating fresh commits with git. Technically speaking, you don't need to create more git commits, you just can reset the cinnabar metadata with rollback and push to a new empty (or non-empty) mercurial repo.

From that perspective, add your two tests at the end ought to be enough.

riastradh · 2024-12-15T00:33:06Z

OK, I've moved the tests to the end of tests/push.t instead. Seems awkward to reuse the repository history previously set up by the rest of the test, but it does exhibit the issue and the fix, at least; let me know if I misunderstood what you were asking for.

PKGREVISION++ (No change to devel/py-hg-cinnabarclone PKGREVISION even though the patch also applies to it, because the patch doesn't change any of the content of the py-hg-cinnabarclone package, which consists mainly of one .py file in git-cinnabar. The patch is applied merely for the convenience of sharing distinfo between the two packages.) Derived from the current state of: glandium/git-cinnabar#342

glandium

I have yet to check whether the changes in src/main.rs are what's wanted, but let's already get these comments out.

glandium · 2024-12-18T11:19:51Z

tests/push.t

+XXX This should fail with nonzero exit status and not update remote refs:
+https://github.com/glandium/git-cinnabar/issues/341
+
+  $ cat <<EOF >repo/.hg/hgrc


Please use $REPO here

glandium · 2024-12-18T11:20:46Z

tests/push.t

+  ERROR Changes on branch 'default' resulted in multiple heads
+  To hg::.*/push.t/repo (re)
+   + 687e015...846552c 846552c6f25c1b46e784f59d8249fb31afac2996 -> branches/default/tip (forced update)
+  $ rm repo/.hg/hgrc


glandium · 2024-12-18T11:27:47Z

tests/push.t

-  Run the command again with `git -c cinnabar.check=traceback <command>` to see the full traceback.
-  error: git-remote-hg died of signal 6
+  To hg::.*/push.t/repo (re)
+   ! [remote rejected] 846552c6f25c1b46e784f59d8249fb31afac2996 -> branches/default/tip (nothing changed on remote)


Shouldn't the git log below change too?

Oh, I think I see what's going on: since the first one overwrites the remote refs, even when the second one doesn't do anything, it still keeps the wrong refs from the first push. How about adding a git remote update to reset the remote to what it's supposed to have?

Done -- I put a git remote update in while that part is exhibiting the bug, and removed it with the bug fix.

glandium · 2024-12-18T11:29:14Z

tests/push.t

  $ rm repo/.hg/hgrc
  $ git -C abc-git log --graph --remotes --oneline --no-abbrev-commit
-  * 846552c6f25c1b46e784f59d8249fb31afac2996 x
-  * bc90f2819ad12e294b313097b8763d26ca0c08ae b
+  * 687e015f9f646bb19797d991f2f53087297fbe14 c


Aha! This should change in the previous commit

glandium · 2024-12-18T11:29:46Z

tests/push.t

@@ -560,11 +560,13 @@ https://github.com/glandium/git-cinnabar/issues/341
  \r (no-eol) (esc)
  ERROR Changes on branch 'default' resulted in multiple heads
  To hg::.*/push.t/repo (re)
-   + 687e015...846552c 846552c6f25c1b46e784f59d8249fb31afac2996 -> branches/default/tip (forced update)
+   ! [remote rejected] 846552c6f25c1b46e784f59d8249fb31afac2996 -> branches/default/tip (nothing changed on remote)


The XXX above this should be removed too.

glandium#338 glandium#341

If connection to upstream is hosed, consider nothing to have been pushed. fix glandium#338

fix glandium#341

riastradh · 2024-12-18T15:07:16Z

OK, I think I have addressed all your comments on the tests.

Like I said at the beginning, the changes to src/main.rs probably aren't quite right -- instead of reporting what was rejected, it just pretends nothing changed on the remote. Probably the right thing is to update pushed, or find some way to propagate the detailed failure into status, but this approximation -- good enough to make it fail with nonzero exit status and not spuriously update remote refs -- was easier to draft than the surgery of those data structures.

glandium requested changes Dec 10, 2024

View reviewed changes

riastradh mentioned this pull request Dec 15, 2024

tests/tags.t output is nondeterministic #344

Open

riastradh force-pushed the riastradh-issue338-issue341-pushcrashreject branch from d5438e7 to 184c6d3 Compare December 15, 2024 00:29

glandium requested changes Dec 18, 2024

View reviewed changes

Taylor R Campbell added 3 commits December 18, 2024 14:45

tests/push.t: Add test cases for rejected pushes.

aee1044

glandium#338 glandium#341

Handle I/O errors gracefully on push.

a1e09cb

If connection to upstream is hosed, consider nothing to have been pushed. fix glandium#338

Treat ERROR:ABORT unbundle responses as failure to push anything.

67644cc

fix glandium#341

riastradh force-pushed the riastradh-issue338-issue341-pushcrashreject branch from 184c6d3 to 67644cc Compare December 18, 2024 15:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make git push fail gracefully with nonzero exit status if upstream hg crashes or rejects push #342

Make git push fail gracefully with nonzero exit status if upstream hg crashes or rejects push #342

riastradh commented Dec 9, 2024 •

edited

Loading

glandium commented Dec 10, 2024

glandium left a comment •

edited

Loading

riastradh commented Dec 10, 2024

riastradh commented Dec 10, 2024

glandium commented Dec 11, 2024

riastradh commented Dec 15, 2024

glandium left a comment

glandium Dec 18, 2024

riastradh Dec 18, 2024

glandium Dec 18, 2024

riastradh Dec 18, 2024

glandium Dec 18, 2024

glandium Dec 18, 2024

riastradh Dec 18, 2024

glandium Dec 18, 2024

riastradh Dec 18, 2024

glandium Dec 18, 2024

riastradh Dec 18, 2024

riastradh commented Dec 18, 2024

Make git push fail gracefully with nonzero exit status if upstream hg crashes or rejects push #342

Are you sure you want to change the base?

Make git push fail gracefully with nonzero exit status if upstream hg crashes or rejects push #342

Conversation

riastradh commented Dec 9, 2024 • edited Loading

glandium commented Dec 10, 2024

glandium left a comment • edited Loading

Choose a reason for hiding this comment

riastradh commented Dec 10, 2024

riastradh commented Dec 10, 2024

glandium commented Dec 11, 2024

riastradh commented Dec 15, 2024

glandium left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

riastradh commented Dec 18, 2024

riastradh commented Dec 9, 2024 •

edited

Loading

glandium left a comment •

edited

Loading