Skip to content
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

Prevent clean files from showing up as untracked #601

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

Conversation

alistair-broomhead
Copy link
Contributor

Looking at the code in porcelain.path_to_tree_path, this looks like there was an attempt at this in 85b5383, however relpath is non-destructive, and has no effect unless the result is assigned to something. If this were assigned when path is relative, then relpath would assume path was at root, and so we must check that the path is absolute before calling relpath.

Fixes #598

Looking at the code in porcelain.path_to_tree_path,
this looks like there was an attempt at this in
85b5383, however
relpath is non-destructive, and has no effect unless
the result is assigned to something. If this were
assigned when path is relative, then relpath would
assume path was at root, and so we must check that
the path is absolute before calling relpath.

Fixes jelmer#598
@alistair-broomhead alistair-broomhead force-pushed the 598-status-shows-all-files-untracked branch from 5a577ec to d9f3de5 Compare January 13, 2018 10:57
@codecov
Copy link

codecov bot commented Jan 13, 2018

Codecov Report

Merging #601 into master will decrease coverage by 0.03%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #601      +/-   ##
==========================================
- Coverage   90.97%   90.94%   -0.04%     
==========================================
  Files          73       73              
  Lines       18470    18316     -154     
  Branches     1977     1961      -16     
==========================================
- Hits        16803    16657     -146     
+ Misses       1265     1260       -5     
+ Partials      402      399       -3
Impacted Files Coverage Δ
dulwich/tests/test_porcelain.py 100% <100%> (ø) ⬆️
dulwich/porcelain.py 74.18% <100%> (+0.09%) ⬆️
dulwich/fastexport.py 75.32% <0%> (-3.9%) ⬇️
dulwich/config.py 86.08% <0%> (-2.56%) ⬇️
dulwich/refs.py 88.19% <0%> (-0.55%) ⬇️
dulwich/tests/test_fastexport.py 96.8% <0%> (-0.29%) ⬇️
dulwich/patch.py 92.57% <0%> (-0.05%) ⬇️
dulwich/client.py 74.54% <0%> (-0.04%) ⬇️
dulwich/tests/test_pack.py 98.78% <0%> (-0.02%) ⬇️
dulwich/repo.py 90.13% <0%> (-0.01%) ⬇️
... and 6 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 17d058b...d9f3de5. Read the comment docs.

@alistair-broomhead
Copy link
Contributor Author

Rebased on top of current master to check for conflicts (there were none)

@jelmer jelmer added the bug label Jan 22, 2018
:param path: A path
:return: A path formatted for use in e.g. an index
"""
os.path.relpath(path, repopath)
if os.path.isabs(path):
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"path" will be relative to os.getcwd(), so I think we should unconditionally call relpath since path_to_tree_path should return the relative path to the repository.

Copy link
Contributor Author

@alistair-broomhead alistair-broomhead Jun 18, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry it's been a few months so I can't quite remember, but when I was testing this it seemed like in some cases this function receives an absolute path. This was in fact responsible for #598 as it was seeing these clean files from the repo as not being the same as the files from the listing, and so did not think they were tracked. I'd need to look at what's happened in the code since January, but this was definitely a false assumption back then, and so needed testing in the code. It's also possible that the calling code is what's at fault, and that the fix belongs elsewhere. I have to admit I was somewhat surprised to see #600 accepted so quickly but this not, given the comparative levels of complexity and scope,

It looks like there may need to be some communication with @r0mainK from #635 as there are some functional changes here and some ruggedisation there, and it would be good to make sure that both of these are done without losing one or the other.

@jelmer
Copy link
Owner

jelmer commented Jun 15, 2018

See also #601

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

Successfully merging this pull request may close these issues.

Git status shows all files as untracked
2 participants