Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
Deferring review until after HackWeek – please ping me if you need a review more urgently |
szokeasaurusrex
left a comment
There was a problem hiding this comment.
Some minor comments, otherwise lgtm!
| // Try to get the current ref from the VCS if not provided | ||
| // Note: git_repo_head_ref will return an error for detached HEAD states, | ||
| // which .ok() converts to None - this prevents sending "HEAD" as a branch name | ||
| // In that case, the user will need to provide a valid branch name. |
There was a problem hiding this comment.
This comment appears to be outdated, as we are no longer calling .ok() on None here
f6fa2a9 to
76521d3
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
szokeasaurusrex
left a comment
There was a problem hiding this comment.
I think Cursor identified a potentially legit concern with the test
c4afc9d to
b83b917
Compare
szokeasaurusrex
left a comment
There was a problem hiding this comment.
lgtm, I have a question though. Please check it before merging
| // Try to get the current ref from the VCS if not provided | ||
| // Note: git_repo_head_ref will return an error for detached HEAD states, | ||
| // which the error handling converts to None - this prevents sending "HEAD" as a branch name | ||
| // In that case, the user will need to provide a valid branch name. |
There was a problem hiding this comment.
[question] Does this imply that the head_ref is required? From what I can tell right now, we don't validate its presence locally, so if it is required by the server, we would likely get a 4xx response when attempting the upload. If that is the case, we should likely return an error here, rather than swallow the error.
There was a problem hiding this comment.
It is not required, it is optional on the backend so no response failure if not provided.
Adds default handling of
head_ref(branch name) to themobile-app uploadsubcommand. This will leverage thehead.shorthand()value if no value is provided by the user.Also handles the detached HEAD state with some logging that will require the user to specify using the command arguments.