Do not force push to secp256k1-zkp branch

[tsusanka]

We're using this repository as a submodule in trezor-firmware and we are bound the secp256k1-zkp branch. We are currently locked to commit 9ecd8bf, but this commit is not in the git tree anymore. Try clean clone and check out this commit:

git clone [email protected]:ElementsProject/secp256k1-zkp.git
git checkout 9ecd8bf38a77a97b44e8c830b286c65051f2f707
fatal: reference is not a tree: 9ecd8bf38a77a97b44e8c830b286c65051f2f707

It seems to me someone forced-push into this branch removing any reference to this commit. Is that correct?

1. Please do not force push to this branch
2. Which commit should we use? What is the new corresponding one?

[real-or-random]

Uh, hm. It's certainly our practice to rebase secp256k1-zkp from time to time onto secp256k1, and then to force-push branches, and in the best case we would like to keep it that way but I see that this creates problems.

First we thought that the problem is that the commit is not in the repo anymore..., so I pushed the branch 2019-05-rebase-previous-head which currently is at 9ecd8bf. That'll make sure that the commit is not lost.

However I think the problem is that your submodule points to secp256k1-zkp branch and the commit is not on that branch. Or does your submodule work again now?

[real-or-random]

Okay, so after reading your comment again, I think the problem was just that the commit was not in the repo anymore. The commit should be there in the repo again, even though the branch has been force-pushed to. Can you confirm that this solves the problem? In the future we'll then need to make sure that we don't remove existing commits from the repo.

Otherwise we need to think about a different solution.

[tsusanka]

Can you confirm that this solves the problem?

Yes, it does.

In the future we'll then need to make sure that we don't remove existing commits from the repo.

Maybe consider naming the branch secp256k1-zkp-trezor or similar so it's clear that it is not to be deleted?

[instagibbs]

I think in general a policy of not blowing away branches is a good idea.

…

On Fri, May 31, 2019, 8:56 AM Tomas Susanka ***@***.***> wrote: Can you confirm that this solves the problem? Yes, it does. In the future we'll then need to make sure that we don't remove existing commits from the repo. Maybe consider naming the branch secp256k1-zkp-trezor or similar so it's clear it is no to be deleted? — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#68>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABMAFU5JPRDL5M3UYNIXPD3PYEN6JANCNFSM4HRZPQKQ> .

[real-or-random]

Maybe consider naming the branch secp256k1-zkp-trezor or similar so it's clear that it is not to be deleted?

Yes, it's just a quick fix for now. We'll need to discuss in detail how we do this for future updates, and then we can rename the branch to a proper name or make it tag or something else.

[tsusanka]

Sounds good. We need the commit there with a reference, other than that we don't care much. Let me know in case you decide to rename.

[romanz]

I would suggest forking ElementsProject/secp256k1-zkp under https://github.com/trezor/ (similar to how it's done with vendor/micropython dependency, IIUC).
This way secp256k1-zkp dependency updates will be coordinated with the TREZOR team via PRs to the trezor/secp256k1-zkp and trezor/trezor-firmware repositories.
What do you think?

[tsusanka]

@romanz we can do that, but then you will need to file PRs to both trezor/secp256k1-zkp and trezor-firmware to bump the submodule. But yes, it's possible as well.

[real-or-random]

Yeah so actually we were happy that we could all changes necessary for Trezor cleanly in secp256k1-zkp so that we don't need to maintain yet another fork. I think we'll want to keep that model. I'll discuss with @apoelstra and come back here.

[instagibbs]

Sorry if I was ambiguous, I means as long as there was a canonical way of keeping old(renamed?) branches that's an improvement, even if the default branch is force pushed.

[apoelstra]

I vote that every time we rebase we simply rename the old branch from secp256k1-zkp to secp256k1-zkp-YYYY-MM-DD.

[apoelstra]

@real-or-random did you push the old secp256k1-zkp somewhere? can you push it again under the name secp256k1-zkp-2019-05-31?

[real-or-random]

Yep, it's here:

I pushed the branch 2019-05-rebase-previous-head which currently is at 9ecd8bf.

edit: yes, sorry, will push again under new name

[real-or-random]

Okay, now there is a secp256k1-zkp-2019-05-30 branch:
https://github.com/ElementsProject/secp256k1-zkp/tree/secp256k1-zkp-2019-05-30

Sorry again for all the trouble!

[tsusanka]

Ok! No worries.