-
Notifications
You must be signed in to change notification settings - Fork 42
fix(vcs): remove empty paths from repo.get_tracked_files() #669
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
base: main
Are you sure you want to change the base?
Conversation
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.
I don't understand why this would make a difference?
Git (at least some versions of it?) seem to not like the empty argument:
|
That part I can understand, but I don't see how this patch changes that. |
src/taskgraph/util/vcs.py
Outdated
cmd = ["ls-tree", "-r", "--name-only", rev] | ||
if paths: | ||
cmd.extend(paths) | ||
return self.run(*cmd).splitlines() |
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.
To Julien's point, this isn't removing any empty strings:
>>> a = []
>>> [1,2,3,*a]
[1, 2, 3]
>>> a = ['']
>>> [1,2,3,*a]
[1, 2, 3, '']
If the idea is to filter out, you'd probably need something like [p for p in paths if p]
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.
Indeed. (Also, it'd be good to understand/document why/how an empty string would come in here, naively that seems like it'd be a bug elsewhere.)
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.
Ah thanks, that makes more sense. Looks like the empty string is ultimately coming from graph_config.vcs_root
which I assumed would be absolute, but I guess it's relative.
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.
I decided to strip out empty strings rather than make graph_config.vcs_path
absolute to avoid causing issues elsewhere.
90126e7
to
c4a23e8
Compare
c4a23e8
to
0b89ccc
Compare
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.
I'm not sure I like the semantics here (vs fixing the caller to not pass the empty string) but it's probably fine.
While looking at possibly changing the callers as @jcristau suggested I stumbled upon the fact that |
The test should be exercising this code path, so I'm not really sure why it passes in Taskgraph but failed in another case in CI. Maybe it's related to the version of Git on the worker.