Re: [PATCH v2 2/2] git-p4: fix Git LFS pointer parsing

From: Lars Schneider <larsxschneider@gmail.com>
Date: Apr 22, 2016, 07:53 UTC
Message-ID: <780ED4D6-AD96-4BAF-A522-C724C3938EED@gmail.com>
In-Reply-To: <xmqq60vcuh9v.fsf@gitster.mtv.corp.google.com>

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