[v2,1/1] "git lfs" allows users to specify the custom storage location by configuration variable lfs.storage, but when "git p4" interacts with GitLFS pointers, it always used the hardcoded default that is the .git/lfs/ directory, without paying attention

Commit Message

Linus Arver via GitGitGadget Dec. 9, 2019, 2:28 p.m. UTC

From: panzercheg <[e-mail]>

Use the value configured in lfs.storage, if exists, as all the
"git" operations do, for consistency.

Signed-off-by: r.burenkov <[e-mail]>
---
 git-p4.py | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

Comments

Junio C Hamano Dec. 9, 2019, 10:27 p.m. UTC | #1

"panzercheg via GitGitGadget" <[e-mail]> writes:

>Subject: Re: [PATCH v2 1/1] "git lfs" allows users to specify the custom storage location by configuration variable lfs.storage, but when "git p4" interacts with GitLFS pointers, it always used the hardcoded default that is the .git/lfs/ directory, without paying attention to the configuration.

Oops, what happened here?

I wonder/I wish if GGG can be a bit more helpful when seeing a
commit that looks "strange".

> From: panzercheg <[e-mail]>
>
> Use the value configured in lfs.storage, if exists, as all the
> "git" operations do, for consistency.
>
> Signed-off-by: r.burenkov <[e-mail]>

Please make sure that the name/email as the author matches whom you
sign-off the patch as.



> ---
>  git-p4.py | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/git-p4.py b/git-p4.py
> index 60c73b6a37..0b3a07cb31 100755
> --- a/git-p4.py
> +++ b/git-p4.py
> @@ -1257,9 +1257,15 @@ def generatePointer(self, contentFile):
>              pointerFile = re.sub(r'Git LFS pointer for.*\n\n', '', pointerFile)
>  
>          oid = re.search(r'^oid \w+:(\w+)', pointerFile, re.MULTILINE).group(1)
> +        # if someone use external lfs.storage ( not in local repo git )
> +        lfs_path = gitConfig('lfs.storage')
> +        if not lfs_path:
> +            lfs_path = 'lfs'
> +        if not os.path.isabs(lfs_path):
> +            lfs_path = os.path.join(os.getcwd(), '.git', lfs_path)
>          localLargeFile = os.path.join(
> -            os.getcwd(),
> -            '.git', 'lfs', 'objects', oid[:2], oid[2:4],
> +            lfs_path,
> +            'objects', oid[:2], oid[2:4],
>              oid,
>          )
>          # LFS Spec states that pointer files should not have the executable bit set.

Eric Sunshine Dec. 9, 2019, 10:50 p.m. UTC | #2

On Mon, Dec 9, 2019 at 5:27 PM Junio C Hamano <[e-mail]> wrote:
> >Subject: Re: [PATCH v2 1/1] "git lfs" allows users to specify the custom storage location by configuration variable lfs.storage, but when "git p4" interacts with GitLFS pointers, it always used the hardcoded default that is the .git/lfs/ directory, without paying attention to the configuration.
>
> Oops, what happened here?
>
> I wonder/I wish if GGG can be a bit more helpful when seeing a
> commit that looks "strange".

There is an open issue[1] regarding that.

[1]: https://github.com/gitgitgadget/gitgitgadget/issues/120

Johannes Schindelin Dec. 10, 2019, 12:19 p.m. UTC | #3

Hi Junio,

On Mon, 9 Dec 2019, Junio C Hamano wrote:

> "panzercheg via GitGitGadget" <[e-mail]> writes:
>
> >Subject: Re: [PATCH v2 1/1] "git lfs" allows users to specify the custom storage location by configuration variable lfs.storage, but when "git p4" interacts with GitLFS pointers, it always used the hardcoded default that is the .git/lfs/ directory, without paying attention to the configuration.
>
> Oops, what happened here?
>
> I wonder/I wish if GGG can be a bit more helpful when seeing a
> commit that looks "strange".

There is already a ticket about that:
https://github.com/gitgitgadget/gitgitgadget/issues/120

All it requires is a contributor with a little time :-)

> > From: panzercheg <[e-mail]>
> >
> > Use the value configured in lfs.storage, if exists, as all the
> > "git" operations do, for consistency.
> >
> > Signed-off-by: r.burenkov <[e-mail]>
>
> Please make sure that the name/email as the author matches whom you
> sign-off the patch as.

This, too, should be addressed as part of above-mentioned ticket.

Ciao,
Dscho

Junio C Hamano Dec. 11, 2019, 5:47 p.m. UTC | #4

Johannes Schindelin <[e-mail]> writes:

> On Mon, 9 Dec 2019, Junio C Hamano wrote:
>
>> "panzercheg via GitGitGadget" <[e-mail]> writes:
>>
>> >Subject: Re: [PATCH v2 1/1] "git lfs" allows users to specify the custom storage location by configuration variable lfs.storage, but when "git p4" interacts with GitLFS pointers, it always used the hardcoded default that is the .git/lfs/ directory, without paying attention to the configuration.
>>
>> Oops, what happened here?
>>
>> I wonder/I wish if GGG can be a bit more helpful when seeing a
>> commit that looks "strange".
>
> There is already a ticket about that:
> https://github.com/gitgitgadget/gitgitgadget/issues/120
>
> All it requires is a contributor with a little time :-)
>
>> > From: panzercheg <[e-mail]>
>> >
>> > Use the value configured in lfs.storage, if exists, as all the
>> > "git" operations do, for consistency.
>> >
>> > Signed-off-by: r.burenkov <[e-mail]>
>>
>> Please make sure that the name/email as the author matches whom you
>> sign-off the patch as.
>
> This, too, should be addressed as part of above-mentioned ticket.

Tooling improvement is fine, but let's not sink too much time on
tangents and steal time from *this* patch.

Would the following version (which I munged by hand) be close enough
to what the author would have sent out in the ideal world?  If so,
let's queue it.

-- >8 --
Subject: git-p4: honor lfs.storage configuration variable
From: r.burenkov <[e-mail]>

"git lfs" allows users to specify the custom storage location by the
configuration variable `lfs.storage`, but when "git p4" interacts with
GitLFS pointers, it always uses the hardcoded default that is the
`.git/lfs/` directory, without paying attention to the configuration.

Use the value configured in `lfs.storage`, if exists, as all the
"git" operations do, for consistency.

Signed-off-by: r.burenkov <[e-mail]>
---
 git-p4.py | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/git-p4.py b/git-p4.py
index 60c73b6a37..0b3a07cb31 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -1257,9 +1257,15 @@ def generatePointer(self, contentFile):
             pointerFile = re.sub(r'Git LFS pointer for.*\n\n', '', pointerFile)
 
         oid = re.search(r'^oid \w+:(\w+)', pointerFile, re.MULTILINE).group(1)
+        # if someone use external lfs.storage ( not in local repo git )
+        lfs_path = gitConfig('lfs.storage')
+        if not lfs_path:
+            lfs_path = 'lfs'
+        if not os.path.isabs(lfs_path):
+            lfs_path = os.path.join(os.getcwd(), '.git', lfs_path)
         localLargeFile = os.path.join(
-            os.getcwd(),
-            '.git', 'lfs', 'objects', oid[:2], oid[2:4],
+            lfs_path,
+            'objects', oid[:2], oid[2:4],
             oid,
         )
         # LFS Spec states that pointer files should not have the executable bit set.

Johannes Schindelin Dec. 12, 2019, 7:47 p.m. UTC | #5

Hi Junio,

On Wed, 11 Dec 2019, Junio C Hamano wrote:

> Johannes Schindelin <[e-mail]> writes:
>
> > On Mon, 9 Dec 2019, Junio C Hamano wrote:
> >
> >> "panzercheg via GitGitGadget" <[e-mail]> writes:
> >>
> >> >Subject: Re: [PATCH v2 1/1] "git lfs" allows users to specify the custom storage location by configuration variable lfs.storage, but when "git p4" interacts with GitLFS pointers, it always used the hardcoded default that is the .git/lfs/ directory, without paying attention to the configuration.
> >>
> >> Oops, what happened here?
> >>
> >> I wonder/I wish if GGG can be a bit more helpful when seeing a
> >> commit that looks "strange".
> >
> > There is already a ticket about that:
> > https://github.com/gitgitgadget/gitgitgadget/issues/120
> >
> > All it requires is a contributor with a little time :-)
> >
> >> > From: panzercheg <[e-mail]>
> >> >
> >> > Use the value configured in lfs.storage, if exists, as all the
> >> > "git" operations do, for consistency.
> >> >
> >> > Signed-off-by: r.burenkov <[e-mail]>
> >>
> >> Please make sure that the name/email as the author matches whom you
> >> sign-off the patch as.
> >
> > This, too, should be addressed as part of above-mentioned ticket.
>
> Tooling improvement is fine, but let's not sink too much time on
> tangents and steal time from *this* patch.

I fully agree.

> Would the following version (which I munged by hand) be close enough
> to what the author would have sent out in the ideal world?  If so,
> let's queue it.

I obviously cannot speak for r.burenkov, but from my point of view, the
below patch is very much the intended outcome.

Thanks,
Dscho

>
> -- >8 --
> Subject: git-p4: honor lfs.storage configuration variable
> From: r.burenkov <[e-mail]>
>
> "git lfs" allows users to specify the custom storage location by the
> configuration variable `lfs.storage`, but when "git p4" interacts with
> GitLFS pointers, it always uses the hardcoded default that is the
> `.git/lfs/` directory, without paying attention to the configuration.
>
> Use the value configured in `lfs.storage`, if exists, as all the
> "git" operations do, for consistency.
>
> Signed-off-by: r.burenkov <[e-mail]>
> ---
>  git-p4.py | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/git-p4.py b/git-p4.py
> index 60c73b6a37..0b3a07cb31 100755
> --- a/git-p4.py
> +++ b/git-p4.py
> @@ -1257,9 +1257,15 @@ def generatePointer(self, contentFile):
>              pointerFile = re.sub(r'Git LFS pointer for.*\n\n', '', pointerFile)
>
>          oid = re.search(r'^oid \w+:(\w+)', pointerFile, re.MULTILINE).group(1)
> +        # if someone use external lfs.storage ( not in local repo git )
> +        lfs_path = gitConfig('lfs.storage')
> +        if not lfs_path:
> +            lfs_path = 'lfs'
> +        if not os.path.isabs(lfs_path):
> +            lfs_path = os.path.join(os.getcwd(), '.git', lfs_path)
>          localLargeFile = os.path.join(
> -            os.getcwd(),
> -            '.git', 'lfs', 'objects', oid[:2], oid[2:4],
> +            lfs_path,
> +            'objects', oid[:2], oid[2:4],
>              oid,
>          )
>          # LFS Spec states that pointer files should not have the executable bit set.
> --
> gitgitgadget
>

Patch

diff --git a/git-p4.py b/git-p4.py
index 60c73b6a37..0b3a07cb31 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -1257,9 +1257,15 @@  def generatePointer(self, contentFile):
             pointerFile = re.sub(r'Git LFS pointer for.*\n\n', '', pointerFile)
 
         oid = re.search(r'^oid \w+:(\w+)', pointerFile, re.MULTILINE).group(1)
+        # if someone use external lfs.storage ( not in local repo git )
+        lfs_path = gitConfig('lfs.storage')
+        if not lfs_path:
+            lfs_path = 'lfs'
+        if not os.path.isabs(lfs_path):
+            lfs_path = os.path.join(os.getcwd(), '.git', lfs_path)
         localLargeFile = os.path.join(
-            os.getcwd(),
-            '.git', 'lfs', 'objects', oid[:2], oid[2:4],
+            lfs_path,
+            'objects', oid[:2], oid[2:4],
             oid,
         )
         # LFS Spec states that pointer files should not have the executable bit set.