-
-
Notifications
You must be signed in to change notification settings - Fork 89
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
hook: Ignore updates to remote branches in reference-transaction #913
base: master
Are you sure you want to change the base?
Conversation
c9f1ef6
to
10cbbe5
Compare
Are there any additional ways I should verify this change? Also I'm getting some lint errors from |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! Ignore the lint failure for now. The linter runs using Rust nightly. Normally, I would just fix the issue in a separate commit, but the issue is occurring inside a macro expansion, so we can't actually fix it ourselves.
git-branchless-hook/src/lib.rs
Outdated
&& match CategorizedReferenceName::new(ref_name) { | ||
CategorizedReferenceName::RemoteBranch { name: _, prefix: _ } => false, | ||
_ => true, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, this is how we destructure enums. Let's just make a few minor changes:
- Let's explicitly list out the missing cases in
_
. The idea is that if a new case is added, the exhaustiveness checker will make us look at thismatch
again and update it to ensure that the logic is right.- Now that I think about all of the cases... let's return
false
for bothRemoteBranch
andOther
. Occasionally, other tooling generates references that we also don't care about (such asgit bisect
or perhaps Jujutsu'skeep
refs).
- Now that I think about all of the cases... let's return
- In this case, we can write
Foo { .. }
instead ofFoo { field1: _, field2: _ }
to ignore all fields in the enum case.- Actually, I don't recommend doing this in general, but the
CategorizedReferenceName
enum case was somewhat poorly designed. Very often we ignore the enum fields and use one of the helper methods on the type. I don't think the fields ought to have been attached to the enum cases to begin with.
- Actually, I don't recommend doing this in general, but the
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Going through and updating the failing tests for this change. Returning false for Other
appears to skip processing updates to HEAD
- just wanted to double check that this behavior is expected?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pink I think it's fine (although it didn't occur to me ahead of time that HEAD
is an Other
reference). It looks like none of the tests are failing as a result. You can update the snapshot tests manually or by using the cargo-insta
tool. The HEAD
reference is also tracked via the post-checkout
hook, so we're not losing any data by dropping the update.
Also, I forgot to mention that you can collapse the match cases by using |
:
match expr {
Foo::Bar { .. } | Foo::Baz { .. } => false,
Foo::Qux => true,
}
(You can do this as long as the set of variables bound in each sub-pattern is the same. Since we're using ..
, we're not binding any variables, so it should work.)
I merged #916, so if you want to get rid of the lint errors in CI, you can try updating on top of that. |
bc5a2c0
to
08455de
Compare
Follow up to #879 (comment) - this PR filters out updates to remote branches from the
reference-transaction
hook.Test Plan
CI and unit tests
% TEST_GIT=$(which git) TEST_GIT_EXEC_PATH=$(git --exec-path) cargo test