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

[Sebastian Schuberth <sschuberth@gmail.com>]

On Wed, Apr 20, 2016 at 10:10 AM,  <larsxschneider@gmail.com> wrote:

> --- a/git-p4.py
> +++ b/git-p4.py
> @@ -1064,8 +1064,17 @@ class GitLFS(LargeFileSystem):
>          if pointerProcess.wait():
>              os.remove(contentFile)
>              die('git-lfs pointer command failed. Did you install the extension?')
> -        pointerContents = [i+'\n' for i in pointerFile.split('\n')[2:][:-1]]
> -        oid = pointerContents[1].split(' ')[1].split(':')[1][:-1]
> +
> +        # Git LFS removed the preamble in the output of the 'pointer' command

git-lfs did not remove the output. It simply goes to stderr instead of
stdout now. That said, could a fix simply be to capture both stdout
and sterr? If the output to the streams remain interleaved it should
look exactly like before.

> +        # 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]

Why do we need to remove the preamble at all, if present? If all we
want is the oid, we should simply only look at the line that starts
with that keyword, which would skip any preamble. Which is what you
already do here. However, I'd probably use .splitlines() instead of
.split('\n') and .startswith('oid ') (note the trailing space) instead
of .startswith('oid') to ensure "oid" is a separate word.

But then again, I wonder why there's so much split() logic involved in
extracting the oid. Couldn't we replace all of that with a regexp like

oid = re.search(r"^oid \w+:(\w+)", pointerFile, re.MULTILINE).group(1)

-- 
Sebastian Schuberth