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

doc: west: add note about west update --keep-descendants option #31201

Merged
merged 1 commit into from
Jan 14, 2021

Conversation

marc-hb
Copy link
Collaborator

@marc-hb marc-hb commented Jan 9, 2021

The previous and short description wasn't enough for me to understand
the interesting trade-off of --keep-descendants, thanks to
@mbolivar-nordic for clarifying this on Slack.

For reference this option was added in west commit 11b8588303 part of
zephyrproject-rtos/west#165

Signed-off-by: Marc Herbert marc.herbert@intel.com

A plain ``west update`` never fails because it does not trying to hold on
to your commits and simply leaves them aside.

``west --keep-descendants`` offers an intermediate option that never
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Forgot update here and above, will fix.

Copy link
Contributor

@mbolivar-nordic mbolivar-nordic left a comment

Choose a reason for hiding this comment

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

Thanks. Other than the issue you already noticed, there's just one more.

``git``, or you can use ``git -C <project_path> rebase --abort`` to
ignore incoming changes for the moment.

A plain ``west update`` never fails because it does not trying to hold on
Copy link
Contributor

Choose a reason for hiding this comment

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

Not quite: west update will fail if there are uncommitted changes in the working tree which conflict with the new HEAD.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm conscious of uncommitted changes but the purpose of this new note and of the existing text right before it is to compare the different update options and uncommitted changes will make ANY update option will fail, correct? So uncommitted changes should be mentioned earlier in a non-option specific part, agreed? Not sure exactly where and how though.

So how about the following phrasing in the note:

A plain west update never causes any git conflict because...

and:

west update --keep-descendants offers an intermediate option that never git conflicts either but...

?

Copy link
Contributor

Choose a reason for hiding this comment

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

Fair. How about:

With a clean working tree, a plain ``west update`` never fails [...]

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done

Copy link
Contributor

@mbolivar-nordic mbolivar-nordic left a comment

Choose a reason for hiding this comment

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

Just one more nit.

ignore incoming changes for the moment.

With a clean working tree, a plain ``west update`` never fails
because it does not trying to hold on to your commits and simply
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
because it does not trying to hold on to your commits and simply
because it does not try to hold on to your commits and simply

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good catch, thanks for the thorough review.

I'm going to click on "commit suggestion". Last time I was in a discussion about this (a very long time ago) this project didn't allow github's most popular "squash commits" feature, it was "force pushes only (#14444) However you're the second person suggesting a fixup commit like this in just 2 days so it looks like the policy is not as strict now? Plus this is a very small commit so very little confusion in the worst of cases.

Copy link
Contributor

Choose a reason for hiding this comment

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

Policy hasn't changed; you can use the commit suggestion button, but you need to fetch the resulting fix-up commits, squash, and force push.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done.

but you need to fetch the resulting fix-up commits, squash, and force push.

Thanks. For changing one word it's not worth it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks. For changing one word it's not worth it.

Your call, of course; it nevertheless has value as a convenient way to provide the exact changes that are being requested.

Copy link
Collaborator

Choose a reason for hiding this comment

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

but you need to fetch the resulting fix-up commits, squash, and force push.

Thanks. For changing one word it's not worth it.

Agree, for a single word change like this, not so useful to hit commit button, but syntax highlighting the suggested change is very nice.

And if there are 3+ commit suggestions like this, it's much easier to just commit them all, and then git fetch + git rebase with squash is easy.

The previous and short description wasn't enough for me to understand
the interesting trade-off of --keep-descendants, thanks to
@mbolivar-nordic for clarifying this on Slack.

For reference this option was added in west commit 11b8588303 part of
zephyrproject-rtos/west#165

Signed-off-by: Marc Herbert <marc.herbert@intel.com>
@MaureenHelm MaureenHelm merged commit d7ac0e0 into zephyrproject-rtos:master Jan 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants