Skip to content

Add atomic push - #138

Merged
ZPascal merged 2 commits into
ad-m:masterfrom
sleepypikachu:master
Oct 4, 2022
Merged

ZPascal merged 2 commits into
ad-m:masterfrom
sleepypikachu:master

Conversation

@sleepypikachu

Copy link
Copy Markdown
Contributor

Since time elapses between the checkout the github workflow performs and the eventual push this action invokes the remote HEAD may have changed. If this is the case the HEAD update will be rejected but any tag (and their commits) will be pushed.

In general I think this operation should be atomic, either we push everything or we push nothing.

Force pushes still work the way you would expect (i.e. if we force the HEAD update with --atomic everything is still pushed)

This also protects from the situation where someone else has seized your tag name in the meantime but not updated your HEAD.

See git docs for more information - https://git-scm.com/docs/git-push#Documentation/git-push.txt---no-atomic

Since time elapses between the checkout the github workflow performs and the eventual push this action invokes the remote HEAD may have changed. If this is the case the HEAD update will be rejected but any tag (and their commits) will be pushed.

In general I think this operation should be atomic, either we push everything or we push nothing.

Force pushes still work the way you would expect (i.e. if we force the HEAD update with --atomic everything is still pushed)

This also protects from the situation where someone else has seized your tag name in the meantime but not updated your HEAD.

See git docs for more information - https://git-scm.com/docs/git-push#Documentation/git-push.txt---no-atomic
@ZPascal

ZPascal commented Oct 3, 2022

Copy link
Copy Markdown
Collaborator

Hi @sleepypikachu, in general it looks good to me. Do you've tested it?

@sleepypikachu

Copy link
Copy Markdown
Contributor Author

Yes, it is working well for us. We're using this behaviour from my fork :-)

@ZPascal

ZPascal commented Oct 3, 2022

Copy link
Copy Markdown
Collaborator

Hi @sleepypikachu, thanks for the update. I'm currently thinking about the case, if the server side do not support the function, will the complete call fail or only the beforehand executed pull?
Maybe we're on a safer side to handle it with an automatic enabled default parameter, but with opportunity to disable it.

@sleepypikachu

Copy link
Copy Markdown
Contributor Author

Hmmm. This is a github action and github does support this feature. So it's hard to imagine what would cause someone to unset this flag.

If you feel strongly I'm happy to implement. Just want to understand the motivation.

@ZPascal

ZPascal commented Oct 4, 2022

Copy link
Copy Markdown
Collaborator

I'm thinking of a case for GitHub Enterprise support. I'm not sure if there's a possibility to disable the function inside a GTE system.

@sleepypikachu

Copy link
Copy Markdown
Contributor Author

Gotcha, no problem. Will implement.

@ZPascal ZPascal left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@ZPascal

ZPascal commented Oct 4, 2022

Copy link
Copy Markdown
Collaborator

@sleepypikachu Thanks for the contribution! I'll merge the PR.

@ZPascal
ZPascal merged commit 35284cf into ad-m:master Oct 4, 2022
@ZPascal ZPascal mentioned this pull request Oct 12, 2022
2 tasks done
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants