Re: [PATCH v4 4/5] git-p4: add support for large file systems

From: Lars Schneider <larsxschneider@gmail.com>
Date: Sep 9, 2015, 20:09 UTC
Message-ID: <353BE0F5-8262-4094-95E6-982C29570F6B@gmail.com>
In-Reply-To: <xmqq37yn7bni.fsf@gitster.mtv.corp.google.com>

On 09 Sep 2015, at 19:20, Junio C Hamano <gitster@pobox.com> wrote:

Show 16 quoted lines

> larsxschneider@gmail.com writes:
> 
>> @@ -2226,17 +2355,16 @@ 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.')
> 
> This looks like an unfortunate limitation to me.
> 
> Is it really necessary that you need to reject unrelated attributes
> the user has (presumably for a good reason)?  It seems to me that
> you only need to _add_ entries to make file extension based decision
> to send paths selectively to LFS?

No, it is not necessary. I will remove this limitation.

Show 6 quoted lines

> 
> Also the exact format of these attributes entries looks like very
> closely tied to GitHub LFS and not generic (for example, there is no
> reason to expect that any large-file support would always use the
> "filter" mechanism or the gitattributes mechanism for that
> matter), …

Agreed. Instead of just the filter name I will replace everything after the pathname with a single git-p4 config value:

['*.' + f.replace(' ', '[:space:]') + ' %s\n' % largeFileSystem().attributes()
    for f in sorted(gitConfigList("git-p4.largeFileExtensions"))
] +
['/' + f.replace(' ', '[:space:]') + ' %s\n' % largeFileSystem().attributes()
    for f in sorted(self.largeFiles) if not self.hasLargeFileExtension(f)
]

This, of course, would only work for gitattributes based solutions.

Show 27 quoted lines

> 
> +    def writeGitAttributesToStream(self):
> +        self.writeToGitStream(
> +            '100644',
> +            '.gitattributes',
> +            [
> +                '\n',
> +                '#\n',
> +                '# %s\n' % largeFileSystem().attributeDescription(),
> +                '#\n',
> +            ] +
> +            ['*.' + f.replace(' ', '[:space:]') + ' filter=%s -text\n'
> +                % largeFileSystem().attributeFilter()
> +                for f in sorted(gitConfigList("git-p4.largeFileExtensions"))
> +            ] +
> +            ['/' + f.replace(' ', '[:space:]') + ' filter=%s -text\n'
> +                % largeFileSystem().attributeFilter()
> +                for f in sorted(self.largeFiles) if not self.hasLargeFileExtension(f)
> +            ]
> +        )
> +
> 
> ... so while I can see the code like the above needs to exist
> somewhere in "git p4" to support GitHub LFS, I am not sure if it
> belongs to the generic part of the code.  For the same reason, I do
> not know if these replacements with largeFileSystem().getters() are
> really adding much value.

I have the impression you would prefer to move all the attributes code from the generic code to the GitLFS code? I will explore that solution and see if I can come up with a nice generic interface.

Show 19 quoted lines

> 
> How is collaboration between those who talk to the same p4 depot
> backed by GitHub LFS expected to work?  You use config to set size
> limits and list of file extensions in your repository, and grab new
> changes from p4 and turn them into Git commits (with pointers to LFS
> and the .gitattributes file that records your choice of the config).
> I as a new member to the same project come, clone the resulting Git
> repository from you and then what do I do before talking to the same
> p4 depot to further update the Git history?  Are the values recorded
> in .gitattributes (which essentially were derived from your
> configuration) somehow reflected back automatically to my config so
> that our world view would become consistent?  Perhaps you added
> 'iso' to largeFileExtensions before I cloned from you, and that
> would be present in the copy of .gitattributes I obtained from you.
> I may be trying to add a new ".iso" file, and I presume that an
> existing .gitattributes entry you created based on the file
> extension automatically covers it from the LFS side, but does my
> "git p4 submit" also know what to do, or would it be broken until I
> add a set of configrations that matches yours?

Well, you have a very good point here. From my point of view you can use git-p4 in two ways:

_Way 1_: Your project is stored in Perforce and it will stay in Perforce. You use git-p4 on a regular basis to interact with the Perforce repository. _Way 2_: Your project is stored in Perforce and you want to migrate it to Git. You use git-p4 once to perform the migration. Afterwards you never touch git-p4 or Perforce again.

My large file system feature is intended to be used only for _Way 2_ for exactly the reasons you mentioned. Would it still be OK to add this feature to git-p4? Maybe with a appropriate warning in the docs?

Thanks, Lars