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

From: Sebastian Schuberth <sschuberth@gmail.com>
Date: Apr 20, 2016, 08:59 UTC
Message-ID: <CAHGBnuOjb+zmzwJeY-hbU4MYKpOMCQSRtDC4hS4zvCD5DQzv3w@mail.gmail.com>
In-Reply-To: <1461139809-6573-3-git-send-email-larsxschneider@gmail.com>

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

Show 10 quoted lines

> --- 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.

Show 9 quoted lines

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