app: manifest: Add the ability to display untracked files and directories

[carlescufi]

This new command-line switch implements something similar to `git
status`'s untracked files display.

The code examines the workspace and prints out all files and folders
that are not inside of a west project.

Fixes: #622
Supersedes #702

[carlescufi]

Thanks @marc-hb for the review! Will address those comments soon!

[marc-hb]

I think adding some pydoc is more important and useful than usual considering the lack of... test :-) Just some suggestions below, please rephrase better.

(It's not a critical feature so I'm fine with the lack of test)

[marc-hb]

I tend to have a lot of untracked stuff in one workspace so I gave this a very quick try. Seems to work but I found one annoying issue: symbolic links are resolved in the display :-( Could you avoid that?

west manifest --untracked

../other_workspace/real_repo        # resolved symbolic link

crosstool-ng      # plain directory, expected
crosstool-ng/builds/xtensa-apl-elf  # more resolved symlinks
crosstool-ng/builds/xtensa-byt-elf
crosstool-ng/builds/xtensa-cnl-elf

The logging with west -vv manifest --untracked is unexpected: It starts with prefixes but the new statements miss them?!

west.manifest: DEBUG: resolving self import submanifests
west.manifest: DEBUG: found submanifest directory: submanifests
west.manifest: DEBUG: resolved self import

_find_untracked in: /home/me/WS
processing folder: /home/me/WS/.west
processing folder: /home/me/WS/acpi-scripts

Code LGTM! I hoped the pydocs could be in a better English but they're OK ;-D

[carlescufi]

I tend to have a lot of untracked stuff in one workspace so I gave this a very quick try. Seems to work but I found one annoying issue: symbolic links are resolved in the display :-( Could you avoid that?

@marc-hb this was intentional, is it not what git status does? indeed it is not

The logging with west -vv manifest --untracked is unexpected: It starts with prefixes but the new statements miss them?!

Indeed, will fix

[pdgendt]

EDIT: One more use case that I think becomes really ugly with @marc-hb's propose approach. Let's say you are switching projects from active to inactive using the group filter, and that is part of your workflow. With your proposal, actual (inactive) west projects cloned by west would show as untracked when you disable a group filter after having it enabled! I think that is very ugly

Agreed on the addition here, besides that I sometimes edit the manifest.path or manifest.file in my .west/config file, and the current proposal would work better in seeing what I left behind from switching over.

[marc-hb]

Let's say you are switching projects from active to inactive using the group filter, and that is part of your workflow. With your proposal, actual (inactive) west projects cloned by west would show as untracked when you disable a group filter after having it enabled! I think that is very ugly

No, because .is_cloned() is true in that case. It would only show "mistakes" that break west update.

You should try it, it's a one-line change ppaths = [ p for p in ppaths if p.is_cloned() ]

[marc-hb]

walk_up was added in Python 3.12 and will not work on older versions.

The most basic test would have found that.

Agreed on the addition here, besides that I sometimes edit the manifest.path or manifest.file in my .west/config file, and the current proposal would work better in seeing what I left behind from switching over.

I don't see why sorry.

Maybe we don't need a test for this example specifically, but the number of discussions about corner cases and specifying their behavior changed my mind and convinced me this feature is complex enough to require at least one test to get it started. It's really not that simple of a feature.

[carlescufi]

Let's say you are switching projects from active to inactive using the group filter, and that is part of your workflow. With your proposal, actual (inactive) west projects cloned by west would show as untracked when you disable a group filter after having it enabled! I think that is very ugly

No, because .is_cloned() is true in that case. It would only show "mistakes" that break west update.

OK, I was referring to your earlier proposal using .exists() instead of is_cloned().
But I will reiterate my statement before status, diff, compare do not operate on the workspace at all, they operate on the individual Git projects. That is also why I prefer this in west manifest, which operates on the manifest and the workspace. I also dislike that the output depends on whether the projects are cloned or not, because, as I've mentioned before, I think this command should restrict itself to analyzing the parts of the workspace that are not covered by the manifest.

But I have to agree with you that, while I do not share your view, I can understand why others would prefer your approach, that's why I've asked or additional opinions.

Maybe we don't need a test for this example specifically, but the number of discussions about corner cases and specifying their behavior changed my mind and convinced me this feature is complex enough to require at least one test to get it started. It's really not that simple of a feature.

Completely agree here. I am working on a basic test.

[marc-hb]

But I will reiterate my statement before status, diff, compare do not operate on the workspace at all, they operate on the individual Git projects.

They operate on the list of .is_cloned() projects and ignore everything else. So a command considering projects-that-could-be-cloned would be a new (and admittedly minor) inconsistency.

That is also why I prefer this in west manifest, which operates on the manifest and the workspace.

Fair.

I also dislike that the output depends on whether the projects are cloned or not, because, as I've mentioned before, I think this command should restrict itself to analyzing the parts of the workspace that are not covered by the manifest.

This sounds like the manifest is a very static thing and this just reminded that because of import it is not really static. This causes plenty of tricky corner cases and "dynamic" manifest dilemma

• Partial imports Incomplete or changing imports are much more complicated than you think
• Better support for projects with imports when users do not want/have them locally #652
• etc.

Curious how this new feature deals with partial and changing imports.

But I have to agree with you that, while I do not share your view, I can understand why others would prefer your approach, that's why I've asked or additional opinions.

If it is really just a one-line change (not 100% sure of that yet), then maybe we could have both based on some additional flag? I don't mind which one we get first and what the default would be.

BTW, did you ever consider and discuss a brand west untracked command?

Completely agree here. I am working on a basic test.

Thank you! That has become my biggest concern now.

[carlescufi]

@marc-hb another reaon why I don't like operating on is_cloned() is that that function does not check whether the "right repo" is cloned. It only checks if there's a Git repo at all. So for me then this is confusing, because if I have another Git repo in it, sure, west update will not fail because it adds a remote and just fetches, and that will work with any Git repo, but it will leave a mix of repos in the folder.
That's why I would rather not complicate it and use the simplest approach.

Alternatively, the only viable option to this is west untracked and then add some sort of --all or --uncloned to enable the behaviour you'd like. But I think this is overkill.

CC @pdgendt.

[carlescufi]

@marc-hb I also spoke to @joerchan, who wrote the initial proposal for this feature.
His main use cases were:

1. To detect a folder that is no longer being used at all by west (this is covered in both your approach and mine), because the original manifest has changed and that project has been moved to another location)
2. To detect which projects are inactive so that he knows that west update won't act on them (even if they are cloned) (this would not be fulfilled neither by your approach nor mine)

Instead, we have agreed that we could add a west list --inactive in order to cover 2. (Which I will add nonetheless because I do think it is a worthy addition). See #794.

[carlescufi]

@marc-hb @pdgendt added a test for the feature.

[pdgendt]

s/folders/directories/ and s/folder/directory/ 😅

[pdgendt]

Re-approved

[mbolivar]

@carlescufi I know you've been looking for this feature for a long time. Thanks for making the effort to work on it.

Given that a lot of the discussion centered around the behavior related to symlinks, I think it would make sense to have at least some basic coverage of the behavior of this command when symbolic links are encountered.

Since that is not always possible on Windows, I would recommend a separate test function like this:

@pytest.mark.skipif(sys.platform.startswith("win"), reason="symbolic links not tested on Windows")
def test_manifest_untracked_with_symlinks():
    # basic test coverage for desired behavior identified by yourself and marc-hb goes here

(see https://docs.pytest.org/en/7.1.x/how-to/skipping.html)

I'm not going to -1 over this, but I think it would give us all some peace of mind to bite the bullet and get those tests in.

[carlescufi]

Given that a lot of the discussion centered around the behavior related to symlinks, I think it would make sense to have at least some basic coverage of the behavior of this command when symbolic links are encountered.

Since that is not always possible on Windows, I would recommend a separate test function like this:

Thanks @mbolivar for the suggestion. I've implemented it now and avoided running it on Widows as you suggested, because you do need to enable developer mode or run an elevated terminal to be able to run the symlink tests on Windows.

[carlescufi]

Last push:

• Added a bit more documentation on the help of --untracked
• Added a few more checks in the test