Re: [RFC PATCH] git-p4: add option to store files in Git LFS on import

From: Luke Diamand <luke@diamand.org>
Date: Aug 30, 2015, 09:08 UTC
Message-ID: <CAE5ih7_yvzEgij9XEvoDnMMvkCeB77SUfnkf7=xr6r8eN6SCog@mail.gmail.com>
In-Reply-To: <1440764691-62254-2-git-send-email-larsxschneider@gmail.com>

Comments below.

Show 9 quoted lines

>
> diff --git a/Documentation/git-p4.txt b/Documentation/git-p4.txt
> index 82aa5d6..a188840 100644
> --- a/Documentation/git-p4.txt
> +++ b/Documentation/git-p4.txt
> @@ -252,6 +252,18 @@ Git repository:
>         Use a client spec to find the list of interesting files in p4.
>         See the "CLIENT SPEC" section below.
>

<snip>

Show 10 quoted lines

>
> +def mkdir_p(path):
> +    # Copied from http://stackoverflow.com/questions/600268/mkdir-p-functionality-in-python
> +    try:
> +        os.makedirs(path)
> +    except OSError as exc: # Python >2.5
> +        if exc.errno == errno.EEXIST and os.path.isdir(path):
> +            pass
> +        else:
> +            raise

Sigh. We need to upgrade to Python 3....

Coujld also just do:

   if not path.isdir(path) os.makedirs(path)

(Although there is a tiny race hazard if something else creates the same path between isdir and makedir, but the way you're using it, seems unlikely.

Show 55 quoted lines

> +
>  def die(msg):
>      if verbose:
>          raise Exception(msg)
> @@ -1994,6 +2005,11 @@ class P4Sync(Command, P4UserMap):
>                  optparse.make_option("-/", dest="cloneExclude",
>                                       action="append", type="string",
>                                       help="exclude depot path"),
> +                optparse.make_option("--use-lfs-if-size-exceeds", dest="lfsMinimumFileSize", type="int",
> +                                     help="Use LFS to store files bigger than the given threshold in bytes."),
> +                optparse.make_option("--use-lfs-for-extension", dest="lfsFileExtensions",
> +                                     action="append", type="string",
> +                                     help="Use LFS to store files with the given file extension(s)."),
>          ]
>          self.description = """Imports from Perforce into a git repository.\n
>      example:
> @@ -2025,6 +2041,9 @@ class P4Sync(Command, P4UserMap):
>          self.clientSpecDirs = None
>          self.tempBranches = []
>          self.tempBranchLocation = "git-p4-tmp"
> +        self.lfsFiles = []
> +        self.lfsMinimumFileSize = None
> +        self.lfsFileExtensions = []
>
>          if gitConfig("git-p4.syncFromOrigin") == "false":
>              self.syncWithOrigin = False
> @@ -2145,6 +2164,63 @@ class P4Sync(Command, P4UserMap):
>
>          return branches
>
> +    def writeToGitStream(self, gitMode, relPath, contents):
> +        self.gitStream.write('M %s inline %s\n' % (gitMode, relPath))
> +        self.gitStream.write('data %d\n' % sum(len(d) for d in contents))
> +        for d in contents:
> +            self.gitStream.write(d)
> +        self.gitStream.write('\n')
> +
> +    def writeGitAttributesToStream(self):
> +        gitAttributes = [f + ' filter=lfs -text\n' for f in self.lfsFiles if not self.hasFileLFSExtension(f)]
> +        self.writeToGitStream(
> +            '100644',
> +            '.gitattributes',
> +            ['*.' + f + ' filter=lfs -text\n' for f in self.lfsFileExtensions] +
> +            [f + ' filter=lfs -text\n' for f in self.lfsFiles if not self.hasFileLFSExtension(f)]
> +        )
> +
> +    def hasFileLFSExtension(self, relPath):
> +        return reduce(
> +            lambda a, b: a or b,
> +            [relPath.endswith('.' + e) for e in self.lfsFileExtensions],
> +            False
> +        )
> +
> +    def isFileLargerThanLFSTreshold(self, relPath, contents):
> +        return self.lfsMinimumFileSize and sum(len(d) for d in contents) >= self.lfsMinimumFileSize

Could have a command-line option "--try-compress-first" (or some such) which compresses the file, and it it's very compressible, leaves it alone. It would trade speed of cloning with not-using-LFS-needlessly.

Show 16 quoted lines

> +
> +    def generateLFSPointerFile(self, relPath, contents):
> +        # Write P4 content to temp file
> +        p4ContentTempFile = tempfile.NamedTemporaryFile(prefix='git-lfs', delete=False)
> +        for d in contents:
> +            p4ContentTempFile.write(d)
> +        p4ContentTempFile.flush()
> +
> +        # Generate LFS pointer file based on P4 content
> +        lfsProcess = subprocess.Popen(
> +            ['git', 'lfs', 'pointer', '--file=' + p4ContentTempFile.name],
> +            stdout=subprocess.PIPE
> +        )
> +        lfsPointerFile = lfsProcess.stdout.read()
> +        if lfsProcess.wait():
> +            die('git-lfs command failed. Did you install the extension?')

We're going to leave the P4 content file lying around undeleted here; is there any way to avoid that and cleanup nicely?

Show 32 quoted lines

> +        contents = [i+'\n' for i in lfsPointerFile.split('\n')[2:][:-1]]
> +
> +        # Write P4 content to LFS
> +        oid = contents[1].split(' ')[1].split(':')[1][:-1]
> +        oidPath = os.path.join(self.cloneDestination, '.git', 'lfs', 'objects', oid[:2], oid[2:4])
> +        mkdir_p(oidPath)
> +        shutil.move(p4ContentTempFile.name, os.path.join(oidPath, oid))
> +
> +        # Update Git attributes
> +        self.lfsFiles.append(relPath)
> +        self.writeGitAttributesToStream()
> +
> +        # LFS Spec states that pointer files should not have the executable bit set.
> +        gitMode = '100644'
> +        return (gitMode, contents)
> +
>      # output one file from the P4 stream
>      # - helper for streamP4Files
>
> @@ -2213,17 +2289,13 @@ class P4Sync(Command, P4UserMap):
>              text = regexp.sub(r'$\1$', text)
>              contents = [ text ]
>
> -        self.gitStream.write("M %s inline %s\n" % (git_mode, relPath))
> +        if relPath == '.gitattributes':
> +            die('.gitattributes already exists in P4.')
>
> -        # total length...
> -        length = 0
> -        for d in contents:
> -            length = length + len(d)
> +        if self.isFileLargerThanLFSTreshold(relPath, contents) or self.hasFileLFSExtension(relPath):

s/Treshold/Threshold/g

Show 32 quoted lines

> +            (git_mode, contents) = self.generateLFSPointerFile(relPath, contents)
>
> -        self.gitStream.write("data %d\n" % length)
> -        for d in contents:
> -            self.gitStream.write(d)
> -        self.gitStream.write("\n")
> +        self.writeToGitStream(git_mode, relPath, contents)
>
>      def streamOneP4Deletion(self, file):
>          relPath = self.stripRepoPath(file['path'], self.branchPrefixes)
> @@ -2231,6 +2303,10 @@ class P4Sync(Command, P4UserMap):
>              sys.stderr.write("delete %s\n" % relPath)
>          self.gitStream.write("D %s\n" % relPath)
>
> +        if relPath in self.lfsFiles:
> +            self.lfsFiles.remove(relPath)
> +            self.writeGitAttributesToStream()
> +
>      # handle another chunk of streaming data
>      def streamP4FilesCb(self, marshalled):
>
> diff --git a/t/t9822-git-p4-lfs.sh b/t/t9822-git-p4-lfs.sh
> new file mode 100755
> index 0000000..b27bf29
> --- /dev/null
> +++ b/t/t9822-git-p4-lfs.sh
> @@ -0,0 +1,277 @@
> +#!/bin/sh
> +
> +test_description='Clone repositories and store files in LFS'
> +
> +( git lfs help ) >/dev/null 2>&1 || {

Does this need to be in a subshell?

Show 38 quoted lines

> +       skip_all='skipping git p4 LFS tests; no git lfs'
> +       test_done
> +}
> +
> +. ./lib-git-p4.sh
> +
> +test_expect_success 'start p4d' '
> +       start_p4d
> +'
> +
> +test_expect_success 'Create repo with binary files' '
> +       client_view "//depot/... //client/..." &&
> +       (
> +               cd "$cli" &&
> +               echo "text" >file.txt &&
> +               echo "bin 13 bytes" >file.dat &&
> +               p4 add file.txt &&
> +               p4 add file.dat &&
> +               p4 submit -d "Add text and binary file" &&
> +               echo "bin 13 bytes" >file2.bin &&
> +               p4 add file2.bin &&
> +               p4 submit -d "Add another binary file with same content"
> +               echo "bin 14 bytess" >file3.bin &&
> +               p4 add file3.bin &&
> +               p4 submit -d "Add another binary file with different content"
> +       )
> +'
> +
> +test_expect_success 'Store files in LFS based on size (10 bytes)' '
> +       client_view "//depot/... //client/..." &&
> +       git p4 clone --use-client-spec --use-lfs-if-size-exceeds=10 --destination="$git" //depot@all &&
> +       test_when_finished cleanup_git &&
> +       (
> +               cd "$git" &&
> +
> +               cat >expect <<-\EOF &&
> +               .git/lfs/objects/d4/43/d443795c1aa3ff7e62afd89d6a86bb84ceba0305f6c22151aa8ee95077a39101
> +               .git/lfs/objects/e5/fe/e5fec48503cd7b85eb9ffaea3311cde2fe9542078b9640369032b26bb5403fff

This feels like it could be very fragile. Every time a new file gets added to the tests we'll end up having to mess around with SHA1 digests. Surely all we care about is that LFS can recreate the files we gave it originally?

Plus it makes it quite hard to understand what's going on!

Show 237 quoted lines

> +               EOF
> +               find ".git/lfs/objects" -type f >actual &&
> +               test_cmp expect actual &&
> +
> +               cat >expect <<-\EOF &&
> +               version https://git-lfs.github.com/spec/v1
> +               oid sha256:d443795c1aa3ff7e62afd89d6a86bb84ceba0305f6c22151aa8ee95077a39101
> +               size 13
> +               EOF
> +               cat file.dat >actual &&
> +               test_cmp expect actual &&
> +
> +               cat >expect <<-\EOF &&
> +               version https://git-lfs.github.com/spec/v1
> +               oid sha256:d443795c1aa3ff7e62afd89d6a86bb84ceba0305f6c22151aa8ee95077a39101
> +               size 13
> +               EOF
> +               cat file2.bin >actual &&
> +               test_cmp expect actual &&
> +
> +               cat >expect <<-\EOF &&
> +               version https://git-lfs.github.com/spec/v1
> +               oid sha256:e5fec48503cd7b85eb9ffaea3311cde2fe9542078b9640369032b26bb5403fff
> +               size 14
> +               EOF
> +               cat file3.bin >actual &&
> +               test_cmp expect actual &&
> +
> +               cat >expect <<-\EOF &&
> +               file.dat filter=lfs -text
> +               file2.bin filter=lfs -text
> +               file3.bin filter=lfs -text
> +               EOF
> +               cat .gitattributes >actual &&
> +               test_cmp expect actual
> +       )
> +'
> +
> +test_expect_success 'Store files in LFS based on size (14 bytes)' '
> +       client_view "//depot/... //client/..." &&
> +       git p4 clone --use-client-spec --use-lfs-if-size-exceeds=14 --destination="$git" //depot@all &&
> +       test_when_finished cleanup_git &&
> +       (
> +               cd "$git" &&
> +
> +               cat >expect <<-\EOF &&
> +               .git/lfs/objects/e5/fe/e5fec48503cd7b85eb9ffaea3311cde2fe9542078b9640369032b26bb5403fff
> +               EOF
> +               find ".git/lfs/objects" -type f >actual &&
> +               test_cmp expect actual &&
> +
> +               cat >expect <<-\EOF &&
> +               bin 13 bytes
> +               EOF
> +               cat file.dat >actual &&
> +               test_cmp expect actual &&
> +
> +               cat >expect <<-\EOF &&
> +               bin 13 bytes
> +               EOF
> +               cat file2.bin >actual &&
> +               test_cmp expect actual &&
> +
> +               cat >expect <<-\EOF &&
> +               version https://git-lfs.github.com/spec/v1
> +               oid sha256:e5fec48503cd7b85eb9ffaea3311cde2fe9542078b9640369032b26bb5403fff
> +               size 14
> +               EOF
> +               cat file3.bin >actual &&
> +               test_cmp expect actual &&
> +
> +               cat >expect <<-\EOF &&
> +               file3.bin filter=lfs -text
> +               EOF
> +               cat .gitattributes >actual &&
> +               test_cmp expect actual
> +       )
> +'
> +
> +test_expect_success 'Store files in LFS based on extension (dat)' '
> +       client_view "//depot/... //client/..." &&
> +       git p4 clone --use-client-spec --use-lfs-for-extension=dat --destination="$git" //depot@all &&
> +       test_when_finished cleanup_git &&
> +       (
> +               cd "$git" &&
> +
> +               cat >expect <<-\EOF &&
> +               .git/lfs/objects/d4/43/d443795c1aa3ff7e62afd89d6a86bb84ceba0305f6c22151aa8ee95077a39101
> +               EOF
> +               find ".git/lfs/objects" -type f >actual &&
> +               test_cmp expect actual &&
> +
> +               cat >expect <<-\EOF &&
> +               version https://git-lfs.github.com/spec/v1
> +               oid sha256:d443795c1aa3ff7e62afd89d6a86bb84ceba0305f6c22151aa8ee95077a39101
> +               size 13
> +               EOF
> +               cat file.dat >actual &&
> +               test_cmp expect actual &&
> +
> +               cat >expect <<-\EOF &&
> +               bin 13 bytes
> +               EOF
> +               cat file2.bin >actual &&
> +               test_cmp expect actual &&
> +
> +               cat >expect <<-\EOF &&
> +               bin 14 bytess
> +               EOF
> +               cat file3.bin >actual &&
> +               test_cmp expect actual &&
> +
> +               cat >expect <<-\EOF &&
> +               *.dat filter=lfs -text
> +               EOF
> +               cat .gitattributes >actual &&
> +               test_cmp expect actual
> +       )
> +'
> +
> +test_expect_success 'Store files in LFS based on size (14 bytes) and extension (dat)' '
> +       client_view "//depot/... //client/..." &&
> +       git p4 clone \
> +               --use-client-spec \
> +               --use-lfs-if-size-exceeds=14 \
> +               --use-lfs-for-extension=dat \
> +               --destination="$git" //depot@all &&
> +       test_when_finished cleanup_git &&
> +       (
> +               cd "$git" &&
> +
> +               cat >expect <<-\EOF &&
> +               .git/lfs/objects/d4/43/d443795c1aa3ff7e62afd89d6a86bb84ceba0305f6c22151aa8ee95077a39101
> +               .git/lfs/objects/e5/fe/e5fec48503cd7b85eb9ffaea3311cde2fe9542078b9640369032b26bb5403fff
> +               EOF
> +               find ".git/lfs/objects" -type f >actual &&
> +               test_cmp expect actual &&
> +
> +               cat >expect <<-\EOF &&
> +               version https://git-lfs.github.com/spec/v1
> +               oid sha256:d443795c1aa3ff7e62afd89d6a86bb84ceba0305f6c22151aa8ee95077a39101
> +               size 13
> +               EOF
> +               cat file.dat >actual &&
> +               test_cmp expect actual &&
> +
> +               cat >expect <<-\EOF &&
> +               bin 13 bytes
> +               EOF
> +               cat file2.bin >actual &&
> +               test_cmp expect actual &&
> +
> +               cat >expect <<-\EOF &&
> +               version https://git-lfs.github.com/spec/v1
> +               oid sha256:e5fec48503cd7b85eb9ffaea3311cde2fe9542078b9640369032b26bb5403fff
> +               size 14
> +               EOF
> +               cat file3.bin >actual &&
> +               test_cmp expect actual &&
> +
> +               cat >expect <<-\EOF &&
> +               *.dat filter=lfs -text
> +               file3.bin filter=lfs -text
> +               EOF
> +               cat .gitattributes >actual &&
> +               test_cmp expect actual
> +       )
> +'
> +
> +test_expect_success 'Remove file from repo and store files in LFS based on size (10 bytes)' '
> +       client_view "//depot/... //client/..." &&
> +       (
> +               cd "$cli" &&
> +               p4 delete file3.bin &&
> +               p4 submit -d "Remove file"
> +       ) &&
> +
> +       git p4 clone --use-client-spec --use-lfs-if-size-exceeds=10 --destination="$git" //depot@all &&
> +       test_when_finished cleanup_git &&
> +       (
> +               cd "$git" &&
> +
> +               # Note that file3 remains here as it referenced in the history
> +               cat >expect <<-\EOF &&
> +               .git/lfs/objects/d4/43/d443795c1aa3ff7e62afd89d6a86bb84ceba0305f6c22151aa8ee95077a39101
> +               .git/lfs/objects/e5/fe/e5fec48503cd7b85eb9ffaea3311cde2fe9542078b9640369032b26bb5403fff
> +               EOF
> +               find ".git/lfs/objects" -type f >actual &&
> +               test_cmp expect actual &&
> +
> +               cat >expect <<-\EOF &&
> +               version https://git-lfs.github.com/spec/v1
> +               oid sha256:d443795c1aa3ff7e62afd89d6a86bb84ceba0305f6c22151aa8ee95077a39101
> +               size 13
> +               EOF
> +               cat file.dat >actual &&
> +               test_cmp expect actual &&
> +
> +               cat >expect <<-\EOF &&
> +               version https://git-lfs.github.com/spec/v1
> +               oid sha256:d443795c1aa3ff7e62afd89d6a86bb84ceba0305f6c22151aa8ee95077a39101
> +               size 13
> +               EOF
> +               cat file2.bin >actual &&
> +               test_cmp expect actual &&
> +
> +               cat >expect <<-\EOF &&
> +               file.dat filter=lfs -text
> +               file2.bin filter=lfs -text
> +               EOF
> +               cat .gitattributes >actual &&
> +               test_cmp expect actual
> +       )
> +'
> +
> +test_expect_success 'Clone repo with existing .gitattributes file' '
> +       client_view "//depot/... //client/..." &&
> +       (
> +               cd "$cli" &&
> +
> +               echo "*.txt text" >.gitattributes &&
> +               p4 add .gitattributes &&
> +               p4 submit -d "Add .gitattributes"
> +       ) &&
> +
> +       test_must_fail git p4 clone --use-client-spec --destination="$git" //depot 2>errs &&
> +       grep ".gitattributes already exists in P4." errs
> +'
> +
> +test_expect_success 'kill p4d' '
> +       kill_p4d
> +'
> +
> +test_done
> --
> 1.9.5 (Apple Git-50.3)
>