Hi Paul,

These are important questions, and I'm glad you've asked them. I will
try to provide a fair amount of detail along with short answers.

Paul Sokolovsky <paul.sokolovsky@linaro.org> writes:

Hello Marti,

On Wed, 4 Sep 2019 16:39:22 +0000
"Bolivar, Marti" <marti.bolivar@nordicsemi.no> wrote:

Marti Bolivar <marti.bolivar@nordicsemi.no> writes:

Hi,

West v0.6.1 contains a ... bug ...

It won't cause data loss, but it will cause west update to fail to
check out the correct revision in some cases.

Speaking of data loss... I figure I have local changes in modules
managed (stroked thru) initially checked out by west. Can west be
trusted:

1) To never touch those changes during normal development process, i.e.
building and running apps?

Short answer is "yes".

First, west doesn't change the contents of the manifest repository,
ever. In upstream zephyr's case, that means builtin west commands won't
edit files in the zephyr repository, only read them. The lone exception
is "west init", which is used upstream to clone the zephyr repository
itself. And if you try to run "west init" again in a way that would
touch an existing installation, it aborts without doing anything -- in
part, to avoid touching existing files in unexpected ways.

This behavior is tested, e.g.:

https://github.com/zephyrproject-rtos/west/blob/master/tests/test_project.py#L227

You can of course shoot yourself in the foot with something like this:

$ west forall -c "rm -rf .git *" # DO NOT RUN THIS

but I don't think that's what you're asking about.

Side note:
this is unlike Google repo, which pulls and can rebase the
manifest repository when you run "repo sync", then attempts to
rebase all your local working copies. I've seen this cause lots of
problems, so not touching zephyr (and not rebasing modules by
default) was an important constraint when we were designing west.

Second, none of the west extension commands in the zephyr repository
(completion, boards, build, sign, flash, debug, debugserver, attach) run
git, so they won't check out any changes that way.

There are of course ways to shoot yourself in the foot with the zephyr
extensions by asking them to overwrite a file in your working copy or
build directory you care about, e.g.:

$ west sign [...] -B some-file-i-care-about
$ west build --pristine=always -d build-dir-with-unsaved-.config-changes

But, well, "you asked for it, you got it" (YAFIYGI).

2) To likewise never overwrite those changes on "west update"?

Not by default; see below.

I guess it boils down to questions: a) "Is it true that west never tries
to perform any unattended module updates, and does that only if
explicitly asked (west update)" and

Updates to modules via git only happens during west update -- again,
barring YAFIYGI commands like:

$ west forall -c "git reset --hard HEAD"

West doesn't perform unattended updates. I personally hate tools that
update me without asking first. West won't download updates and ask you
if you want to apply them (like a web browser), either: you need to
explicitly ask west to pull changes from the network and/or apply them to
your working trees.

b) "Is it true that west, when
asked to update modules, doesn't run any workcopy-destructive commands,
like git reset, some forms of git checkout; or, if it does, it first
performs (regression-tested) check for local changes in the
corresponding modules ?"

The default "west update" behavior uses "git checkout --detach".

From git-checkout(1):

Prepare to work on top of <commit>, by detaching HEAD at
it (see "DETACHED HEAD" section), and updating the index
and the files in the working tree. Local modifications to
the files in the working tree are kept, so that the
resulting working tree will be the state recorded in the
commit plus the local modifications.

Beyond preserving working tree state (unless there is a bug in git
itself), this leaves your existing branches behind untouched.

This is also tested, e.g.:

https://github.com/zephyrproject-rtos/west/blob/master/tests/test_project.py#L186

Note that if local changes conflict with what would be checked out, git
checkout --detach simply fails with "Your local changes to the following
files would be overwritten by checkout: <list-of-files>".

In that case, "west update" fails on that project, keeps going to the
others, and ultimately exits with an error. I would argue it has done
the right thing by leaving your code alone in the problematic project,
while trying to do its job everywhere it can.

When that happens, "west update" also prints an error at the very end of
the command output alerting you that something went wrong, like this:

ERROR: update failed for projects: <list-of-failed-projects>

This is meant to help the user in case the error output scrolls past
their terminal window too quickly.

Now, you *can* ask west to rebase locally checked out branches with
"west update --rebase" -- YAFIYGI.

Of course, no test suite is perfect, but I hope this addresses your
concerns, or at least answers your questions. We're always trying to
improve the test suite and welcome any suggestions.

To address a potential follow-up, I am looking into adding better
coverage metrics to the test suite (it's tricky because a lot of the
code is run in python subprocesses, as is the case in the above
test_project.py example, so coverage from those runs needs to be
collected and combined to produce meaningful results). From there, we
can look at what's missing and make improvements.

That of course does not address behavioral concerns like "what happens
to my working tree in this situation?", which is a separate and ongoing
effort. If you have any specific concerns, please let me know and I'll
do my best to add missing tests as needed.

Thanks,
Marti