Skip to content

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

ahal
Copy link
Collaborator

@ahal ahal commented Apr 3, 2025

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.

@ahal ahal self-assigned this Apr 3, 2025
@ahal ahal requested a review from a team as a code owner April 3, 2025 14:02
@ahal ahal requested a review from gabrielbusta April 3, 2025 14:02
Copy link
Contributor

@jcristau jcristau left a 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?

@bhearsum
Copy link
Contributor

bhearsum commented Apr 3, 2025

I don't understand why this would make a difference?

Git (at least some versions of it?) seem to not like the empty argument:

~/repos/translations gpu-multi:main ⇡9 *75 ❯ git ls-tree -r --name-only HEAD '' | wc -l        
fatal: empty string is not a valid pathspec. please use . instead if you meant to match all paths
0
~/repos/translations gpu-multi:main ⇡9 *75 ❯ echo $?                             
0
~/repos/translations gpu-multi:main ⇡9 *75 ❯ git ls-tree -r --name-only HEAD | wc -l   
695

@jcristau
Copy link
Contributor

jcristau commented Apr 3, 2025

That part I can understand, but I don't see how this patch changes that.

cmd = ["ls-tree", "-r", "--name-only", rev]
if paths:
cmd.extend(paths)
return self.run(*cmd).splitlines()
Copy link
Contributor

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]

Copy link
Contributor

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.)

Copy link
Collaborator Author

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.

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 decided to strip out empty strings rather than make graph_config.vcs_path absolute to avoid causing issues elsewhere.

@ahal ahal force-pushed the ahal/push-xxqkyoxuzxrs branch from 90126e7 to c4a23e8 Compare April 3, 2025 15:26
@ahal ahal force-pushed the ahal/push-xxqkyoxuzxrs branch from c4a23e8 to 0b89ccc Compare April 3, 2025 15:27
@ahal ahal changed the title fix(vcs): remove empty string from command in repo.get_tracked_files() fix(vcs): remove empty paths from repo.get_tracked_files() Apr 3, 2025
@ahal ahal requested review from jcristau and hneiva April 3, 2025 15:47
Copy link
Contributor

@jcristau jcristau left a 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.

@bhearsum
Copy link
Contributor

bhearsum commented Apr 7, 2025

While looking at possibly changing the callers as @jcristau suggested I stumbled upon the fact that vcs_root ends up being an empty string most of the time due to #673. That may negate the need for this patch, although this (or something similar) is probably a fix that we want even if there's no current bustage.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants