Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Revisit expected hash meaning #7202

Open
adutra opened this issue Jul 7, 2023 · 1 comment
Open

Revisit expected hash meaning #7202

adutra opened this issue Jul 7, 2023 · 1 comment

Comments

@adutra
Copy link
Contributor

adutra commented Jul 7, 2023

When an expected hash is provided for a commit, merge or transplant operation, the new storage model validates that the hash exists and is reachable from the current branch HEAD:

if (referenceHash.isPresent()) {
ObjId referenceObjId = hashToObjId(referenceHash.get());
if (!referenceObjId.equals(headId())) {
RefMapping refMapping = new RefMapping(persist);
e = refMapping.commitInChain(branch, head, referenceHash, emptyList());
}
}

However it does not enforce that the expected hash is the current HEAD. IOW, the operation may succeed even if extra, non-conflicting commits were added to the target branch in the meantime.

This is a bit in contradiction with the merge/transplant API docs for the expected hash parameter:

+ "The 'hash' commit must be reachable from the current HEAD of the branch.\n"
+ "In this case 'hash' indicates the state of contents known to the client and serves to ensure that the "
+ "operation is performed on the contents that the client expects.\n"

The commit docs OTOH are a bit closer to reality:

+ "If both 'name' and 'hash' are given, 'hash' must be reachable from the current HEAD of the branch.\n"
+ "In this case 'hash' indicates the state of contents that should be used for validating incoming changes\n"
+ "(commits / merges / transplants).\n"

It would be good to give users the ability to enforce that the expected hash is the current HEAD and/or revisit the docs for this parameter.

@adutra
Copy link
Contributor Author

adutra commented Aug 7, 2023

Currently only 2 endpoints enforce that the expected hash is the current HEAD: assignReference and deleteReference. Both throw HTTP 409 (Conflict) if that's not the case. The reported conflict type is Conflict.ConflictType#UNEXPECTED_HASH.

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

No branches or pull requests

1 participant