zephyrproject-rtos / west

West, Zephyr's meta-tool
https://docs.zephyrproject.org/latest/guides/west/index.html
Apache License 2.0
230 stars 122 forks source link

west update: consider recommending `git rebase --onto` instead #448

Open mbolivar-nordic opened 3 years ago

mbolivar-nordic commented 3 years ago

Currently, west update prints a message whenever a checked out branch is left behind that recommends rebasing onto the new HEAD with git -C relative/path/to/repo rebase NEW_HEAD left-behind-branch-name.

This doesn't work well if the new head rebased from the old manifest-rev, as it brings along the old commits.

@joerchan noted that git .. rebase --onto NEW_HEAD left-behind-branch-name might be a better recommendation in this case. This "two ref argument" form of --onto is not documented in the git rebase manual as far as I can tell; the manpage just has examples which do git rebase --onto NEW_HEAD ref1 ref2, with three refs total including NEW_HEAD instead of just two.

Needs more investigation to see if we can make a better recommendation to rebase just the commits that were on top of the old manifest-rev via --onto or otherwise.

It occurs to me while writing this that west update knows the old and new manifest-rev commits, so it can probably create a "three-argument" --onto invocation with those.

cc:

joerchan commented 3 years ago

@mbolivar-nordic I suggested the three ref form, but I didn't have a good way to find the needed second ref. git rebase --onto NEW_HEAD left-behind-branch-name-starting-point left-behind-branch-name In my case manifest-rev did not point to this commit, not saying that that is a bad heuristic though.

I made the suggestion before I realized that i was on a branch from zephyrproject-rtos/zephyr, but in the context of NCS, so west update changed to a branch in nrfconnect/sdk-zephyr.

I cannot think of a better suggestion for this scenario other than the first commit on left-behind-branch-name that is not me.

marc-h38 commented 3 years ago

This doesn't work well if the new head rebased from the old manifest-rev, as it brings along the old commits.

Can someone confirm this issue is specifically about a "volatile" manifest-rev where the old manifest-rev is not a parent of the new manifest-rev? AND the old commits have been superseded by slightly different new commits, so the current rebase advice will most likely cause a conflict between old and new upstream? If I got this starting point wrong then you should probably ignore half of what I wrote below. Which half is left as an exercise for the reader :-)

Volatile/mutable branches make everything more complicated, I wish git had a way to label them

git .. rebase --onto NEW_HEAD left-behind-branch-name might be a better recommendation in this case.

I don't see how that can achieve any desired effect. I tried this on a toy example and as expected it seems to rebase the new commits on top of themselves. This had no effect.

This "two ref argument" form of --onto (--onto NEW_HEAD left-behind-branch-name) is not documented in the git rebase manual as far as I can tell;

It may not have examples but I'm pretty sure it is documented, from git help rebase:

[--onto <newbase> ] [<upstream> [<branch>]]

Every time I look at git help rebase (too often!) I mentally translate "upstream" to "excluded" and "branch" to "included" where excluded..included is of course the list of commits to replay --onto newbase. So it helps to read [<upstream> [<branch>]] as [ <excluded>..[<included>] ], in fact the man page almost says that.

So the "two refs argument" form translates to: --onto NEW_HEAD left-behind-branch-name..NEW_HEAD = replaying new commits on top of themselves.

As for many other git commands, the git help rebase page is confusing because it keeps jumping up and down between the low levels of what git can do (anything!) and up to the most common and poorly abstracted high level use cases . While "upstream" is the most common rebase use case and gets all the default values it is still one particular rebase use case and the term "upstream" obscures IMHO the simplicity of what rebase does in all cases: just replaying any commit sequence onto any starting point.

For some sanity I'm omitting the git rebase feature that also switches branch for "convenience"... and more confusion.

git cherry-pick accepts commit sequences and is basically equivalent to git rebase but with a much simpler and clearer user interface. I almost always prefer cherry-pick to rebase in automation.

BTW I often git branch --set-upstream-to manifest-rev Never tried it with a volatile manifest-rev though.

marc-h38 commented 3 years ago

It occurs to me while writing this that west update knows the old and new manifest-rev commits, so it can probably create a "three-argument" --onto invocation with those.

In the case of a volatile manifest-rev I don't think it's possible to do anything without the old manifest-rev. Without the old manifest-rev nothing can draw a line between old upstream commits versus local commits. This is the exact equivalent of a manifest-rev force-push.

I suggested the three ref form, but I didn't have a good way to find the needed second ref.

You could try the reflog of manifest-rev

joerchan commented 3 years ago

You could try the reflog of manifest-rev

I meant automatically, in the script :)

marc-hb commented 3 years ago

I meant automatically, in the script :)

Sorry: which script?

I meant you could suggest git rebase --onto (NEW_)HEAD manifest-rev@{1} branch-left-behind to the user (see also #364)

I agree I would be very reluctant to use any relog in any automation.

mbolivar-nordic commented 3 years ago

Can someone confirm this issue is specifically about a "volatile" manifest-rev where the old manifest-rev is not a parent of the new manifest-rev?

Confirmed, and thanks for the explanation; that was useful.

In the case of a volatile manifest-rev I don't think it's possible to do anything without the old manifest-rev. Without the old manifest-rev nothing can draw a line between old upstream commits versus local commits. This is the exact equivalent of a manifest-rev force-push.

The nice thing about west update is that it is the only command that knows the old manifest-rev. So I guess that's possible.