Re: [PATCH v2 2/2] git-p4: fix Git LFS pointer parsing
Show 5 quoted lines
> On 20 Apr 2016, at 23:01, Junio C Hamano <gitster@pobox.com> wrote:
>
> larsxschneider@gmail.com writes:
>
>> - pointerContents = [i+'\n' for i in pointerFile.split('\n')[2:][:-1]]pointerFile was split here by '\n'. The splitting removes the newline and the i+'\n' adds it again. This back and forth makes it unnecessary hard to read.
Show 25 quoted lines
>> - oid = pointerContents[1].split(' ')[1].split(':')[1][:-1]
>> +
>> + # Git LFS removed the preamble in the output of the 'pointer' command
>> + # starting from version 1.2.0. Check for the preamble here to support
>> + # earlier versions.
>> + # c.f. https://github.com/github/git-lfs/commit/da2935d9a739592bc775c98d8ef4df9c72ea3b43
>> + preamble = 'Git LFS pointer for ' + contentFile + '\n\n'
>> + if pointerFile.startswith(preamble):
>> + pointerFile = pointerFile[len(preamble):]
>> +
>> + oidEntry = [i for i in pointerFile.split('\n') if i.startswith('oid')]
>> + oid = oidEntry[0].split(' ')[1].split(':')[1]
>> localLargeFile = os.path.join(
>> os.getcwd(),
>> '.git', 'lfs', 'objects', oid[:2], oid[2:4],
>> @@ -1073,7 +1082,7 @@ class GitLFS(LargeFileSystem):
>> )
>> # LFS Spec states that pointer files should not have the executable bit set.
>> gitMode = '100644'
>> - return (gitMode, pointerContents, localLargeFile)
>> + return (gitMode, pointerFile, localLargeFile)
>
> It seems to me that you used to return pointerContents which is an
> array of lines (each of which are LF terminated); the updated one
> returns pointerFile which is a bare string with many lines.The pointerContents is a string with LF separators (see comment above). The only difference is that the old implementation was a list of strings each with a LF at the end ['line1\n'],['line2\n'] and the new implementation is one string with LF separators 'line1\nline2\n'.
pointerContents goes through a few layers and is eventually used in the "writeToGitStream" method [1] like this:
for d in contents:
self.gitStream.write(d)At this point it doesn't matter if it is an array of strings or one string.
[1] https://github.com/git/git/blob/e6ac6e1f7d54584c2b03f073b5f329a37f4a9561/git-p4.py#L2401-L2402
> Is that change intentional? Does the difference matter to the
> caller of this method? Even if it doesn't, is it a good idea to
> change it as part of this commit?
Yes, it was intentional. I wanted to make the code simpler/more readable. As shown above the change doesn't matter to the caller method.
I understand your concern of making this change part of the commit. However, I don't see another way because passing pointerFile as is in the old implementation would brake Git LFS 1.x support (because pointerFile contains the preamble).
What would be the best way forward? A v3 with a better commit message mentioning the array -> string change?
Thanks for the review, Lars
larsxschneider@gmail.com
