Support file resolution in Git submodules for apply_edits.py [chromium/src : main]
Sergio Solano (Gerrit)
Sergio Solano added 1 comment![]( "Open in Gerrit")
Related details
- Bryan Enrique Gonzalez
- Daniel Angulo
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Sergio Solano (Gerrit)
Sergio Solano voted and added 1 comment![]( "Open in Gerrit")
Votes added by Sergio Solano
| Commit-Queue | +1 |
1 comment
Hi, Stephen, could you help me with a code review? I think this complements the work on https://chromium-review.git.corp.google.com/c/chromium/src/+/7757140. This allows files to be discovered in git subrepos during the apply_edits step of the spanification.
Related details
- Bryan Enrique Gonzalez
- Daniel Angulo
- Stephen Nusko
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Stephen Nusko (Gerrit)
Stephen Nusko added 1 comment![]( "Open in Gerrit")
# Some rewrites target submodules (e.g. dawn, skia, angle, webrtc, ...), so
# if they are specifically passed as an argument, we should also call
# git ls-files there.
if paths:
for p in paths:
real_path = os.path.realpath(p)
if os.path.isdir(real_path):
# Walk up the directory tree to find if this path is within a submodule
curr = real_path
submodule_root = None
while curr and curr != os.path.dirname(curr):
if os.path.exists(os.path.join(curr, '.git')):
submodule_root = curr
break
curr = os.path.dirname(curr)
if submodule_root:
submodule_files = run_ls_files(cwd=submodule_root)
files.extend([
os.path.realpath(os.path.join(submodule_root, f))
for f in submodule_files
])
return filesIs there a way we could make a test for this logic? At least in our spanify plugin where we want this?
Could we introduce a test case that has a file inside a git submodule? That would fail before this change and applies the rewrite afterwards?
The actual logic seems legit and correct but I'd feel better with a test showing it working.
Related details
- Bryan Enrique Gonzalez
- Daniel Angulo
- Sergio Solano
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Sergio Solano (Gerrit)
Sergio Solano voted and added 2 comments![]( "Open in Gerrit")
Votes added by Sergio Solano
| Auto-Submit | +1 |
2 comments
Hi,Stephen, i've added a unit test testSubmoduleSupport.
The test mocks a submodule/subrepo environment (the .git directory and the targeted git ls-files calls). This validates the added logic is needed to discover files within a submodule and the 'rewrite' part is already proven by the existing spanify tests.
From your comment i understand that ideally we would have a subrepo and inside it a file to be rewritten. Am i getting your idea right? Is that to validate the rest of the spanify pipeline?
I've also tested the listing of submodules for ANGLE and this was missing to see it being rewritten. Bryan's also tested it in Skia.
# Some rewrites target submodules (e.g. dawn, skia, angle, webrtc, ...), so
# if they are specifically passed as an argument, we should also call
# git ls-files there.
if paths:
for p in paths:
real_path = os.path.realpath(p)
if os.path.isdir(real_path):
# Walk up the directory tree to find if this path is within a submodule
curr = real_path
submodule_root = None
while curr and curr != os.path.dirname(curr):
if os.path.exists(os.path.join(curr, '.git')):
submodule_root = curr
break
curr = os.path.dirname(curr)
if submodule_root:
submodule_files = run_ls_files(cwd=submodule_root)
files.extend([
os.path.realpath(os.path.join(submodule_root, f))
for f in submodule_files
])
return filesIs there a way we could make a test for this logic? At least in our spanify plugin where we want this?
Could we introduce a test case that has a file inside a git submodule? That would fail before this change and applies the rewrite afterwards?
The actual logic seems legit and correct but I'd feel better with a test showing it working.
- Daniel Angulo
- Stephen Nusko
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daniel Angulo (Gerrit)
Daniel Angulo added 1 comment![]( "Open in Gerrit")
if os.path.exists(os.path.join(curr, '.git')):does this match for only for files or directories?
Related details
- Sergio Solano
- Stephen Nusko
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |