Re: [PATCH] Update git-p4 to be compatible with git-lfs 1.2

From: Lars Schneider <larsxschneider@gmail.com>
Date: Apr 22, 2016, 08:10 UTC
Message-ID: <E230B4FD-79B5-4CA7-9F0D-A4F8F3470296@gmail.com>
In-Reply-To: <CAC5gnOx9W3S3f=92T_LNWBfFm71xe9uNY3O_exxetcf2Xjp+Ew@mail.gmail.com>

Show 63 quoted lines

> 
> 
> On Wed, Apr 20, 2016 at 12:36 PM, Lars Schneider
> <larsxschneider@gmail.com> wrote:
>> 
>>> On Wed, Apr 20, 2016 at 12:00 PM, Luke Diamand <luke@diamand.org> wrote:
>>>> On 20 April 2016 at 19:28, Ben Woosley <Ben.Woosley@gmail.com> wrote:
>>>>> From: Ben Woosley <ben.woosley@gmail.com>
>>>>> 
>>>>> The git lfs pointer output was changed in:
>>>>> https://github.com/github/git-lfs/pull/1105
>>>>> 
>>>>> This was causing Mac Travis runs to fail, as homebrew had updated to 1.2
>>>>> while Linux was pinned at 1.1 via GIT_LFS_VERSION.
>>>>> 
>>>>> The travis builds against 1.1 and 1.2 both on linux. Mac can't do the same as
>>>>> it takes the latest homebrew version regardless.
>>>> 
>>>> Is this related to the very similar thread going on here:
>>>> 
>>>> http://thread.gmane.org/gmane.comp.version-control.git/291917/focus=291926
>>>> 
>>>> Thanks
>>>> Luke
>> 
>> 
>> On 20 Apr 2016, at 21:13, Ben Woosley <ben.woosley@gmail.com> wrote:
>> 
>>> Yep it's addressing the same problem - I developed this independently
>>> after having only viewed the github repo:
>>> https://github.com/git/git/pull/231
>>> 
>>> Things I like about my patch:
>>> 1) it maintains ongoing support for git-lfs 1.1 by retaining it in the travis CI
>>> 2) it's a fairly minimal intervention into the existing behavior
>>> 
>>> Lars how about adding my Travis changes to your patch?
>>> Here's a look at the CI output: https://travis-ci.org/git/git/builds/124105972
>>> 
>>> Best,
>>> Ben
>> 
>> 
>> Thanks a lot for your fix! It's great to see other people than
>> me actually using this feature :)
>> 
>> I already sent a v2 with LFS 1.x support, too:
>> http://article.gmane.org/gmane.comp.version-control.git/291993
>> Would you mind reviewing it, too?
>> Sorry for the duplicated work :-(
>> 
>> Your Travis changes are 100% correct and a very nice way to ensure we
>> support Git LFS 1.1 and Git LFS 1.2. Unfortunately we run all other Git
>> tests twice which increases the overall build time a lot (because we
>> can only run 5 build jobs in parallel with the free Travis CI plan).
>> I am not sure if testing an outdated LFS version justifies the increased
>> build times...
>> 
>> Best,
>> Lars
>> 
>> PS: Please see Junio's comment regarding top posting:
>> http://article.gmane.org/gmane.comp.version-control.git/291932

> On 20 Apr 2016, at 23:10, Ben Woosley <Ben.Woosley@gmail.com> wrote:
> 
> Thanks! Some thoughts:

Again, please see Junio's comment regarding top posting: http://article.gmane.org/gmane.comp.version-control.git/291932

> 
> * One option on the Travis front would be to just test one combination
> of the 1.1 build - e.g. linux + clang + 1.1, so you'll stay within the
> 5 parallel builds while also having some coverage on lfs 1.1.

TBH I still think testing an outdated Git LFS version does not justify +10 extra minutes of computing. Plus if we want to be consistent we would need to do the same for LFS 1.0, 1.2, and for pretty much every other dependency...

> * It might be risky to match on contentFile when looking for the
> prefix - there could be expansions or other modifications applied by
> git-lfs between input and output. I would think it a bit safer to just
> match on the beginning of the line.

Agreed. I will incorporate this in my next version and CC you.

> * Have you guys thought about splitting out git-p4? It seems like a
> good candidate for an extension / add-on, and would remove the soft
> git-lfs dependency.

Yes, this was previously discussed here: http://article.gmane.org/gmane.comp.version-control.git/276822/

Cheers, Lars