Skip to content

Conversation

@nathanhi
Copy link
Contributor

@nathanhi nathanhi commented Feb 1, 2016

This PR fixes the reported behaviour in #557, where Git reports
a delta, if an attempt was made to track empty files with LFS.
If there already have been empty files tracked with LFS, these
need to be untracked manually, as this fix only prevents that
newly created, empty files can be tracked by LFS.

Problem is, that Git never runs the clean filter on 0-byte files:

https://github.com/git/git/blob/80980a1d5c2678ab9031d7c60faf38b9631eb1ce/diff.c#L2777

This leads to a diff between the file in the repository (pointer file) and
the actual file content. Apparently the git-fat project seems to be affected
by this, too (jedbrown/git-fat#18).
The fix/workaround for this in LFS is simple; just don't create pointers
for 0-byte target files, it's useless to store them in LFS anyway.

I'm not too sure though if my current patch is sufficient - it just checks
if the given file from the parameters has a size == 0, but apparently the
pointer creation + hash calculation is done from the stdin content. So just
let me know if this needs a rework with length calculation of the stdin
io.Reader.

@technoweenie
Copy link
Contributor

I'm not too sure though if my current patch is sufficient - it just checks
if the given file from the parameters has a size == 0, but apparently the
pointer creation + hash calculation is done from the stdin content.

I think this should be handled based on the stdin content only. The filename is given with an optional %f parameter. It is possible (though unlikely) that someone can run the git lfs clean command without it.

Also, the shell tests are more for integration tests. I think it should actually init a new repo and add an empty file, with the appropriate checks. Maybe even test pushing and cloning afterwards. We care about what commands the user will actually be running.

There should be a good spot in the Go tests to make sure that cleaning an empty file results in no pointer.

Thanks for hacking on this :)

@nathanhi
Copy link
Contributor Author

nathanhi commented Feb 1, 2016

Sorry, took me a while, I'm fairly new to Go. I've moved the size verification from command_clean.go to pointer.go, so that Encode() directly returns an empty Buffer on an empty input string. Did, of course, also add a test for that.

lfs/pointer.go Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd prefer an early return here:

func (p *Pointer) Encoded() string {
    var buffer bytes.Buffer
    if p.Size == 0 {
        return buffer.String()
    }

This fits common go conventions better.

@nathanhi
Copy link
Contributor Author

nathanhi commented Feb 1, 2016

@technoweenie: Sure, makes sense. Done.

@technoweenie
Copy link
Contributor

Cool, thanks for digging in, especially in an unfamiliar language :)

@technoweenie technoweenie merged commit 763ed1f into git-lfs:master Feb 2, 2016
@technoweenie
Copy link
Contributor

BTW, I added an integration test to check this behaves correctly in Git: test-zero-len-file.sh. The first test asserts that the git blob created for the empty file is actually blank, and not an LFS pointer with the SHA-256 signature of "". The second test asserts that the repo has a clean git status after cloning.

@technoweenie technoweenie mentioned this pull request Feb 2, 2016
15 tasks
@nathanhi nathanhi deleted the 0byte-files branch February 3, 2016 21:13
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.

2 participants