threads / patch / 42094

patchUpdate git-p4 to be compatible with git-lfs 1.2

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

## tl;dr

10 messages between Apr 20, 2016 and Apr 28, 2016. Diffs are folded; open one to read it.

replies: 9people: 5as markdown or json

Ben Woosley· Apr 20, 2016, 18:28 UTC · lore
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.
---
 .travis.yml | 9 ++++++++-
 git-p4.py   | 7 ++++++-
 2 files changed, 14 insertions(+), 2 deletions(-)
Show changes to 2 files +14 −3

.travis.yml, git-p4.py

diff --git a/.travis.yml b/.travis.yml
index 78e433b..71510ee 100644
--- a/.travis.yml
+++ b/.travis.yml
@@ -23,7 +23,6 @@ env:
   global:
     - DEVELOPER=1
     - P4_VERSION="15.2"
-    - GIT_LFS_VERSION="1.1.0"
     - DEFAULT_TEST_TARGET=prove
     - GIT_PROVE_OPTS="--timer --jobs 3 --state=failed,slow,save"
     - GIT_TEST_OPTS="--verbose --tee"
@@ -31,6 +30,14 @@ env:
     # t9810 occasionally fails on Travis CI OS X
     # t9816 occasionally fails with "TAP out of sequence errors" on Travis CI OS X
     - GIT_SKIP_TESTS="t9810 t9816"
+  matrix:
+    - GIT_LFS_VERSION="1.2.0"
+    - GIT_LFS_VERSION="1.1.0"
+
+matrix:
+  exclude:
+    - os: osx
+      env: GIT_LFS_VERSION="1.1.0"
 
 before_install:
   - >
diff --git a/git-p4.py b/git-p4.py
index 527d44b..6c06d17 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -1064,7 +1064,12 @@ def generatePointer(self, contentFile):
         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]]
+        pointerLines = pointerFile.split('\n')
+        # In git-lfs < 1.2, the pointer output included some extraneous information
+        # this was removed in https://github.com/github/git-lfs/pull/1105
+        if pointerLines[0].startswith('Git LFS pointer for'):
+            pointerLines = pointerLines[2:]
+        pointerContents = [i+'\n' for i in pointerLines[:-1]]
         oid = pointerContents[1].split(' ')[1].split(':')[1][:-1]
         localLargeFile = os.path.join(
             os.getcwd(),

--
https://github.com/git/git/pull/231
Luke Diamand· Apr 20, 2016, 19:00 UTC · re: Ben Woosley · lore

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

On 20 April 2016 at 19:28, Ben Woosley <Ben.Woosley@gmail.com> wrote:
Show 10 quoted lines
> 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

Show 57 quoted lines
> ---
>  .travis.yml | 9 ++++++++-
>  git-p4.py   | 7 ++++++-
>  2 files changed, 14 insertions(+), 2 deletions(-)
>
> diff --git a/.travis.yml b/.travis.yml
> index 78e433b..71510ee 100644
> --- a/.travis.yml
> +++ b/.travis.yml
> @@ -23,7 +23,6 @@ env:
>    global:
>      - DEVELOPER=1
>      - P4_VERSION="15.2"
> -    - GIT_LFS_VERSION="1.1.0"
>      - DEFAULT_TEST_TARGET=prove
>      - GIT_PROVE_OPTS="--timer --jobs 3 --state=failed,slow,save"
>      - GIT_TEST_OPTS="--verbose --tee"
> @@ -31,6 +30,14 @@ env:
>      # t9810 occasionally fails on Travis CI OS X
>      # t9816 occasionally fails with "TAP out of sequence errors" on Travis CI OS X
>      - GIT_SKIP_TESTS="t9810 t9816"
> +  matrix:
> +    - GIT_LFS_VERSION="1.2.0"
> +    - GIT_LFS_VERSION="1.1.0"
> +
> +matrix:
> +  exclude:
> +    - os: osx
> +      env: GIT_LFS_VERSION="1.1.0"
>
>  before_install:
>    - >
> diff --git a/git-p4.py b/git-p4.py
> index 527d44b..6c06d17 100755
> --- a/git-p4.py
> +++ b/git-p4.py
> @@ -1064,7 +1064,12 @@ def generatePointer(self, contentFile):
>          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]]
> +        pointerLines = pointerFile.split('\n')
> +        # In git-lfs < 1.2, the pointer output included some extraneous information
> +        # this was removed in https://github.com/github/git-lfs/pull/1105
> +        if pointerLines[0].startswith('Git LFS pointer for'):
> +            pointerLines = pointerLines[2:]
> +        pointerContents = [i+'\n' for i in pointerLines[:-1]]
>          oid = pointerContents[1].split(' ')[1].split(':')[1][:-1]
>          localLargeFile = os.path.join(
>              os.getcwd(),
>
> --
> https://github.com/git/git/pull/231
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
Ben Woosley· Apr 20, 2016, 19:13 UTC · re: Luke Diamand · lore

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

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

On Wed, Apr 20, 2016 at 12:00 PM, Luke Diamand <luke@diamand.org> wrote:
Show 78 quoted lines
> 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
>
>
>
>> ---
>>  .travis.yml | 9 ++++++++-
>>  git-p4.py   | 7 ++++++-
>>  2 files changed, 14 insertions(+), 2 deletions(-)
>>
>> diff --git a/.travis.yml b/.travis.yml
>> index 78e433b..71510ee 100644
>> --- a/.travis.yml
>> +++ b/.travis.yml
>> @@ -23,7 +23,6 @@ env:
>>    global:
>>      - DEVELOPER=1
>>      - P4_VERSION="15.2"
>> -    - GIT_LFS_VERSION="1.1.0"
>>      - DEFAULT_TEST_TARGET=prove
>>      - GIT_PROVE_OPTS="--timer --jobs 3 --state=failed,slow,save"
>>      - GIT_TEST_OPTS="--verbose --tee"
>> @@ -31,6 +30,14 @@ env:
>>      # t9810 occasionally fails on Travis CI OS X
>>      # t9816 occasionally fails with "TAP out of sequence errors" on Travis CI OS X
>>      - GIT_SKIP_TESTS="t9810 t9816"
>> +  matrix:
>> +    - GIT_LFS_VERSION="1.2.0"
>> +    - GIT_LFS_VERSION="1.1.0"
>> +
>> +matrix:
>> +  exclude:
>> +    - os: osx
>> +      env: GIT_LFS_VERSION="1.1.0"
>>
>>  before_install:
>>    - >
>> diff --git a/git-p4.py b/git-p4.py
>> index 527d44b..6c06d17 100755
>> --- a/git-p4.py
>> +++ b/git-p4.py
>> @@ -1064,7 +1064,12 @@ def generatePointer(self, contentFile):
>>          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]]
>> +        pointerLines = pointerFile.split('\n')
>> +        # In git-lfs < 1.2, the pointer output included some extraneous information
>> +        # this was removed in https://github.com/github/git-lfs/pull/1105
>> +        if pointerLines[0].startswith('Git LFS pointer for'):
>> +            pointerLines = pointerLines[2:]
>> +        pointerContents = [i+'\n' for i in pointerLines[:-1]]
>>          oid = pointerContents[1].split(' ')[1].split(':')[1][:-1]
>>          localLargeFile = os.path.join(
>>              os.getcwd(),
>>
>> --
>> https://github.com/git/git/pull/231
>> --
>> To unsubscribe from this list: send the line "unsubscribe git" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
Lars Schneider· Apr 20, 2016, 19:36 UTC · re: Ben Woosley · lore

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

Show 19 quoted lines
> 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:
Show 13 quoted lines
> 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
Show 60 quoted lines
>> 
>> 
>> 
>>> ---
>>> .travis.yml | 9 ++++++++-
>>> git-p4.py   | 7 ++++++-
>>> 2 files changed, 14 insertions(+), 2 deletions(-)
>>> 
>>> diff --git a/.travis.yml b/.travis.yml
>>> index 78e433b..71510ee 100644
>>> --- a/.travis.yml
>>> +++ b/.travis.yml
>>> @@ -23,7 +23,6 @@ env:
>>>   global:
>>>     - DEVELOPER=1
>>>     - P4_VERSION="15.2"
>>> -    - GIT_LFS_VERSION="1.1.0"
>>>     - DEFAULT_TEST_TARGET=prove
>>>     - GIT_PROVE_OPTS="--timer --jobs 3 --state=failed,slow,save"
>>>     - GIT_TEST_OPTS="--verbose --tee"
>>> @@ -31,6 +30,14 @@ env:
>>>     # t9810 occasionally fails on Travis CI OS X
>>>     # t9816 occasionally fails with "TAP out of sequence errors" on Travis CI OS X
>>>     - GIT_SKIP_TESTS="t9810 t9816"
>>> +  matrix:
>>> +    - GIT_LFS_VERSION="1.2.0"
>>> +    - GIT_LFS_VERSION="1.1.0"
>>> +
>>> +matrix:
>>> +  exclude:
>>> +    - os: osx
>>> +      env: GIT_LFS_VERSION="1.1.0"
>>> 
>>> before_install:
>>>   - >
>>> diff --git a/git-p4.py b/git-p4.py
>>> index 527d44b..6c06d17 100755
>>> --- a/git-p4.py
>>> +++ b/git-p4.py
>>> @@ -1064,7 +1064,12 @@ def generatePointer(self, contentFile):
>>>         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]]
>>> +        pointerLines = pointerFile.split('\n')
>>> +        # In git-lfs < 1.2, the pointer output included some extraneous information
>>> +        # this was removed in https://github.com/github/git-lfs/pull/1105
>>> +        if pointerLines[0].startswith('Git LFS pointer for'):
>>> +            pointerLines = pointerLines[2:]
>>> +        pointerContents = [i+'\n' for i in pointerLines[:-1]]
>>>         oid = pointerContents[1].split(' ')[1].split(':')[1][:-1]
>>>         localLargeFile = os.path.join(
>>>             os.getcwd(),
>>> 
>>> --
>>> https://github.com/git/git/pull/231
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe git" in
>>> the body of a message to majordomo@vger.kernel.org
>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
Ben Woosley· Apr 20, 2016, 21:10 UTC · re: Lars Schneider · lore

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

Thanks! Some thoughts:
* 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.
* 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.
* 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.

Best, Ben

On Wed, Apr 20, 2016 at 12:36 PM, Lars Schneider <larsxschneider@gmail.com> wrote:

Show 121 quoted lines
>
>> 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
>
>>>
>>>
>>>
>>>> ---
>>>> .travis.yml | 9 ++++++++-
>>>> git-p4.py   | 7 ++++++-
>>>> 2 files changed, 14 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/.travis.yml b/.travis.yml
>>>> index 78e433b..71510ee 100644
>>>> --- a/.travis.yml
>>>> +++ b/.travis.yml
>>>> @@ -23,7 +23,6 @@ env:
>>>>   global:
>>>>     - DEVELOPER=1
>>>>     - P4_VERSION="15.2"
>>>> -    - GIT_LFS_VERSION="1.1.0"
>>>>     - DEFAULT_TEST_TARGET=prove
>>>>     - GIT_PROVE_OPTS="--timer --jobs 3 --state=failed,slow,save"
>>>>     - GIT_TEST_OPTS="--verbose --tee"
>>>> @@ -31,6 +30,14 @@ env:
>>>>     # t9810 occasionally fails on Travis CI OS X
>>>>     # t9816 occasionally fails with "TAP out of sequence errors" on Travis CI OS X
>>>>     - GIT_SKIP_TESTS="t9810 t9816"
>>>> +  matrix:
>>>> +    - GIT_LFS_VERSION="1.2.0"
>>>> +    - GIT_LFS_VERSION="1.1.0"
>>>> +
>>>> +matrix:
>>>> +  exclude:
>>>> +    - os: osx
>>>> +      env: GIT_LFS_VERSION="1.1.0"
>>>>
>>>> before_install:
>>>>   - >
>>>> diff --git a/git-p4.py b/git-p4.py
>>>> index 527d44b..6c06d17 100755
>>>> --- a/git-p4.py
>>>> +++ b/git-p4.py
>>>> @@ -1064,7 +1064,12 @@ def generatePointer(self, contentFile):
>>>>         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]]
>>>> +        pointerLines = pointerFile.split('\n')
>>>> +        # In git-lfs < 1.2, the pointer output included some extraneous information
>>>> +        # this was removed in https://github.com/github/git-lfs/pull/1105
>>>> +        if pointerLines[0].startswith('Git LFS pointer for'):
>>>> +            pointerLines = pointerLines[2:]
>>>> +        pointerContents = [i+'\n' for i in pointerLines[:-1]]
>>>>         oid = pointerContents[1].split(' ')[1].split(':')[1][:-1]
>>>>         localLargeFile = os.path.join(
>>>>             os.getcwd(),
>>>>
>>>> --
>>>> https://github.com/git/git/pull/231
>>>> --
>>>> To unsubscribe from this list: send the line "unsubscribe git" in
>>>> the body of a message to majordomo@vger.kernel.org
>>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
Lars Schneider· Apr 22, 2016, 08:10 UTC · re: Ben Woosley · lore

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

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

SZEDER Gábor· Apr 25, 2016, 16:25 UTC · re: Lars Schneider · lore

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

Show 5 quoted lines
> > * 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.

I agree that checking compatibility with an older Git LFS version doesn't worth the extra 10 minutes. However, since Git LFS is only involved in two test scripts and for the rest of the test suite it doesn't matter at all, doing a full build and running the whole test suite for the sole sake of a different Git LFS version is definitely unnecessary.

The Bash completion and prompt scripts are in a similar situation: there are only two test scripts involved and the rest of the test suite couldn't care less. However, we definitely want to support older Bash versions as well, all the way back to v3.0, and there are a few commits fixing breakages reported by users of old Bash versions.

As I somehow grew fond of those Bash scripts over the years, I put together a couple of patches allowing me to say 'cd t && make -j4 full-bash-test', which runs the completion and prompt tests with multiple Bash versions. For the seven major and minor releases including and after v3.0 it usually takes less than 8 seconds. As far as runtime goes, I think that's well worth it.

You can have a look at these patches at
  https://github.com/szeder/git completion-test-multiple-bash-versions
and perhaps you could even adapt it to LFS and/or p4 somehow.
> 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...

I'm not sure we should be consistent in this case, at least not solely for consistency's sake and not in git.git. Taking what I did for Bash and doing it for different versions of LFS, p4, etc. could perhaps keep the runtime under control, but t/Makefile would surely get out of control rather quickly. Putting these into a travis-ci matrix is so much simpler, but the runtime makes it infeasible, of course.

I think the best we can do is to keep this out of git.git and let (hope?) developers interested in a particular subsystem do this "multiple version compatibility" tests as they see fit.

Junio C Hamano· Apr 25, 2016, 17:24 UTC · re: SZEDER Gábor · lore

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

SZEDER Gábor <szeder@ira.uka.de> writes:
Show 16 quoted lines
> You can have a look at these patches at
>
>   https://github.com/szeder/git completion-test-multiple-bash-versions
>
> and perhaps you could even adapt it to LFS and/or p4 somehow.
>
>> 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...  
>
> I'm not sure we should be consistent in this case, at least not solely
> for consistency's sake and not in git.git. Taking what I did for Bash
> and doing it for different versions of LFS, p4, etc. could perhaps
> keep the runtime under control, but t/Makefile would surely get out
> of control rather quickly.  Putting these into a travis-ci matrix is
> so much simpler, but the runtime makes it infeasible, of course.

I took a brief look of your branch, and I like its approach. If I understood your approach correctly, you:

 * Group selected tests in t/ as "these are bash related tests I
   care about" in t/Makefile;
 * Add Travis test target to build Git with specific versions of
   bash, and run the above target instead of the full test to
   exercise the version of bash you are testing.

And I agree that the same can be done for LFS versions and P4 versions. Only a handful tests in t/ are about these niches.

> I think the best we can do is to keep this out of git.git and let
> (hope?) developers interested in a particular subsystem do this
> "multiple version compatibility" tests as they see fit.
SZEDER Gábor· Apr 25, 2016, 23:10 UTC · re: Junio C Hamano · lore

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

Quoting Junio C Hamano <gitster@pobox.com>:
Show 24 quoted lines
> SZEDER Gábor <szeder@ira.uka.de> writes:
>
>> You can have a look at these patches at
>>
>>   https://github.com/szeder/git completion-test-multiple-bash-versions
>>
>> and perhaps you could even adapt it to LFS and/or p4 somehow.
>>
>>> 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...
>>
>> I'm not sure we should be consistent in this case, at least not solely
>> for consistency's sake and not in git.git. Taking what I did for Bash
>> and doing it for different versions of LFS, p4, etc. could perhaps
>> keep the runtime under control, but t/Makefile would surely get out
>> of control rather quickly.  Putting these into a travis-ci matrix is
>> so much simpler, but the runtime makes it infeasible, of course.
>
> I took a brief look of your branch, and I like its approach.  If I
> understood your approach correctly, you:
>
>  * Group selected tests in t/ as "these are bash related tests I
>    care about" in t/Makefile;
Yes.
>  * Add Travis test target to build Git with specific versions of
>    bash, and run the above target instead of the full test to
>    exercise the version of bash you are testing.
Not quite.
   * Add t/Makefile targets to run a Bash-related test script with a
     specific Bash version, one target for each script-version pair,
     and a target to run all Bash-related tests with all listed
     Bash-versions.
   * Extend the travis-ci config so that, after building Git as usual
     and running the full test suite as usual, it additionaly runs all
     Bash-related tests will all listed Bash versions on Linux builds.
> And I agree that the same can be done for LFS versions and P4
> versions.  Only a handful tests in t/ are about these niches.

Luckily for me, running a test script with a specific Bash version is as trivial as '/path/to/bash-vX.Y t9902-completion.sh'. No modifications to the test scripts or to lib-bash.sh were necessary.

OTOH, Git LFS and p4 tests, AFAICS, rely on git-lfs and p4 binaries being available in $PATH, and the p4 tests need two binaries. So there is more work that has to be done, as we would need a way to override those binaries found in $PATH, either through an environment variable or a command line option. Bonus points for a solution that would work equally well with LFS, p4 and Bash: then perhaps we could have a single unified block of Makefile metaprogramming, which could generate all those test targets from a list of dependency-specific tests and a list of paths to different versions of that dependency. Then it might even be suitable for inclusion in git.git.

>> I think the best we can do is to keep this out of git.git and let
>> (hope?) developers interested in a particular subsystem do this
>> "multiple version compatibility" tests as they see fit.
Lars Schneider· Apr 28, 2016, 06:09 UTC · re: SZEDER Gábor · lore

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

On 26 Apr 2016, at 01:10, SZEDER Gábor <szeder@ira.uka.de> wrote:
Show 62 quoted lines
> 
> Quoting Junio C Hamano <gitster@pobox.com>:
> 
>> SZEDER Gábor <szeder@ira.uka.de> writes:
>> 
>>> You can have a look at these patches at
>>> 
>>>  https://github.com/szeder/git completion-test-multiple-bash-versions
>>> 
>>> and perhaps you could even adapt it to LFS and/or p4 somehow.
>>> 
>>>> 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...
>>> 
>>> I'm not sure we should be consistent in this case, at least not solely
>>> for consistency's sake and not in git.git. Taking what I did for Bash
>>> and doing it for different versions of LFS, p4, etc. could perhaps
>>> keep the runtime under control, but t/Makefile would surely get out
>>> of control rather quickly.  Putting these into a travis-ci matrix is
>>> so much simpler, but the runtime makes it infeasible, of course.
>> 
>> I took a brief look of your branch, and I like its approach.  If I
>> understood your approach correctly, you:
>> 
>> * Group selected tests in t/ as "these are bash related tests I
>>   care about" in t/Makefile;
> 
> Yes.
> 
>> * Add Travis test target to build Git with specific versions of
>>   bash, and run the above target instead of the full test to
>>   exercise the version of bash you are testing.
> 
> Not quite.
> 
>  * Add t/Makefile targets to run a Bash-related test script with a
>    specific Bash version, one target for each script-version pair,
>    and a target to run all Bash-related tests with all listed
>    Bash-versions.
> 
>  * Extend the travis-ci config so that, after building Git as usual
>    and running the full test suite as usual, it additionaly runs all
>    Bash-related tests will all listed Bash versions on Linux builds.
> 
>> And I agree that the same can be done for LFS versions and P4
>> versions.  Only a handful tests in t/ are about these niches.
> 
> Luckily for me, running a test script with a specific Bash version is
> as trivial as '/path/to/bash-vX.Y t9902-completion.sh'.  No
> modifications to the test scripts or to lib-bash.sh were necessary.
> 
> OTOH, Git LFS and p4 tests, AFAICS, rely on git-lfs and p4 binaries
> being available in $PATH, and the p4 tests need two binaries.  So
> there is more work that has to be done, as we would need a way to
> override those binaries found in $PATH, either through an environment
> variable or a command line option.  Bonus points for a solution that
> would work equally well with LFS, p4 and Bash: then perhaps we could
> have a single unified block of Makefile metaprogramming, which could
> generate  all those test targets from a list of dependency-specific
> tests and a list of paths to different versions of that dependency.
> Then it might even be suitable for inclusion in git.git.

Thanks for sharing this! It's awesome! I will try to find a generic solution based on your work that handles bash versions, LFS versions, and P4 versions! Stay tuned :-)

Cheers, Lars

Show 7 quoted lines
> 
> 
>>> I think the best we can do is to keep this out of git.git and let
>>> (hope?) developers interested in a particular subsystem do this
>>> "multiple version compatibility" tests as they see fit.
> 
>

← back to recent threads