[PATCH 0/1] git-p4: use lfs.storage instead of local .git/lfs

[PATCH 0/1] git-p4: use lfs.storage instead of local .git/lfs

[r.burenkov via GitGitGadget]

Use lfs.storage if it defined in git.config. If lfs.storage not define -
used local .git/lfs.

Original code uses local .git/lfs in sync/clone operations, but if you have
external lfs storage better to use it.

panzercheg (1):
  git-p4: use lfs.storage instead of local .git/lfs Use lfs.storage if
    it defined in git.config. If lfs.storage not define - used local
    .git/lfs. Original code uses local .git/lfs in sync/clone
    operations, but if you have external lfs storage better to use it.

 git-p4.py | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)


base-commit: d9f6f3b6195a0ca35642561e530798ad1469bd41
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-483%2Fpanzercheg%2Fgit-p4-improve-lfs-logic-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-483/panzercheg/git-p4-improve-lfs-logic-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/483
-- 
gitgitgadget

[PATCH 1/1] git-p4: use lfs.storage instead of local .git/lfs Use lfs.storage if it defined in git.config. If lfs.storage not define - used local .git/lfs. Original code uses local .git/lfs in sync/clone operations, but if you have external lfs storage better to use it.

[panzercheg via GitGitGadget]

From: panzercheg <panzercheg@gmail.com>

Signed-off-by: r.burenkov <panzercheg@gmail.com>
---
 git-p4.py | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/git-p4.py b/git-p4.py
index 60c73b6a37..2bd0497c31 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -1257,9 +1257,13 @@ 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 = os.path.join(os.getcwd(), '.git', 'lfs')
+        if gitConfig('lfs.storage'):
+            lfs_path = gitConfig('lfs.storage')
         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

Re: [PATCH 1/1] git-p4: use lfs.storage instead of local .git/lfs Use lfs.storage if it defined in git.config. If lfs.storage not define - used local .git/lfs. Original code uses local .git/lfs in sync/clone operations, but if you have external lfs storage better to use it.

[Junio C Hamano]

"panzercheg via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: panzercheg <panzercheg@gmail.com>

> Subject: Re: [PATCH 1/1] git-p4: use lfs.storage instead of local .git/lfs Use lfs.storage if it defined in git.config. If lfs.storage not define - used local .git/lfs. Original code uses local .git/lfs in sync/clone operations, but if you have external lfs storage better to use it.

The reason why you ended up with this unreadably long subject line
is because your commit did not follow the recommended

    <area>: summary of the change in 50 or so characters

    description of the issue being tackled,
    where it came from,
    why the current behaviour is considered not ideal, etc.

    what the proposed change does, and why it is the good idea.

format, each clearly separated into their own paragraphs.

Especially the title MUST be in its own, single-line paragraph.

Let's dissect what we see on the single long subject line.


    git-p4: use lfs.storage instead of local .git/lfs

    Use lfs.storage if it defined in git.config.

What is "git.config"?  Is it our own usual configuration file(s), or
is it the Git-LFS customized .lfsconfig file at the root of the
repository, or something else?

Grammo: "Use lfs.storage if it is defined ..."

But do not start your proposed log message with the conclusion.
Before "Use X", we want to see readers convinced why using X is a
good idea; in other words, you need to tell the reason why the
current system that does not use X is not ideal, and explain that
the change makes the world a better place by teaching Git to use X.

    If lfs.storage not define - used local .git/lfs.
    Original code uses local .git/lfs in sync/clone operations,

These are saying pretty much the same thing, that can easily
inferred from the "Use lfs.storage if it is defined.", iow, without
having much original information content.

    but if you have external lfs storage better to use it.

This is probably the sole line that attempts to justify the change.
It should be made stronger, I would think.

Possible rewrite (don't use it literally---I am not an GitLFS user so
I may be writing nonsense here):

    git-p4: prefer lfs.storage over local .git/lfs directory

    "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.

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

    Signed-off-by: ...


>
> Signed-off-by: r.burenkov <panzercheg@gmail.com>
> ---
>  git-p4.py | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/git-p4.py b/git-p4.py
> index 60c73b6a37..2bd0497c31 100755
> --- a/git-p4.py
> +++ b/git-p4.py
> @@ -1257,9 +1257,13 @@ 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 )

s/use/uses/; also lose SP after '(' and before ')'.

> +        lfs_path = os.path.join(os.getcwd(), '.git', 'lfs')
> +        if gitConfig('lfs.storage'):
> +            lfs_path = gitConfig('lfs.storage')

Shouldn't the above three lines be more like this?

	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)

The reason why I suspect so is because [*1*] says that the value of
the lfs.storage that is not an absolute path is relative to ".git".

Thanks.



[Reference]

*1* https://github.com/git-lfs/git-lfs/blob/master/docs/man/git-lfs-config.5.ronn



>          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.

[PATCH v2 0/1] git-p4: use lfs.storage instead of local .git/lfs

[r.burenkov via GitGitGadget]

Use lfs.storage if it defined in git.config. If lfs.storage not define -
used local .git/lfs.

Original code uses local .git/lfs in sync/clone operations, but if you have
external lfs storage better to use it.

panzercheg (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.

 git-p4.py | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)


base-commit: d9f6f3b6195a0ca35642561e530798ad1469bd41
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-483%2Fpanzercheg%2Fgit-p4-improve-lfs-logic-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-483/panzercheg/git-p4-improve-lfs-logic-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/483

Range-diff vs v1:

 1:  73d0dfc9dd ! 1:  e65375c528 git-p4: use lfs.storage instead of local .git/lfs Use lfs.storage if it defined in git.config. If lfs.storage not define - used local .git/lfs. Original code uses local .git/lfs in sync/clone operations, but if you have external lfs storage better to use it.
     @@ -1,10 +1,13 @@
      Author: panzercheg <panzercheg@gmail.com>
      
     -    git-p4: use lfs.storage instead of local .git/lfs
     -    Use lfs.storage if it defined in git.config.
     -    If lfs.storage not define - used local .git/lfs.
     -    Original code uses local .git/lfs in sync/clone operations,
     -    but if you have external lfs storage better to use it.
     +    "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.
     +
     +    Use the value configured in lfs.storage, if exists, as all the
     +    "git" operations do, for consistency.
      
          Signed-off-by: r.burenkov <panzercheg@gmail.com>
      
     @@ -16,9 +19,11 @@
       
               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 = os.path.join(os.getcwd(), '.git', 'lfs')
     -+        if gitConfig('lfs.storage'):
     -+            lfs_path = gitConfig('lfs.storage')
     ++        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],

-- 
gitgitgadget

[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.

[panzercheg via GitGitGadget]

From: panzercheg <panzercheg@gmail.com>

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

Signed-off-by: r.burenkov <panzercheg@gmail.com>
---
 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

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.

[Junio C Hamano]

"panzercheg via GitGitGadget" <gitgitgadget@gmail.com> 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 <panzercheg@gmail.com>
>
> Use the value configured in lfs.storage, if exists, as all the
> "git" operations do, for consistency.
>
> Signed-off-by: r.burenkov <panzercheg@gmail.com>

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.

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.

[Eric Sunshine]

On Mon, Dec 9, 2019 at 5:27 PM Junio C Hamano <gitster@pobox.com> 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

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.

[Johannes Schindelin]

Hi Junio,

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

> "panzercheg via GitGitGadget" <gitgitgadget@gmail.com> 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 <panzercheg@gmail.com>
> >
> > Use the value configured in lfs.storage, if exists, as all the
> > "git" operations do, for consistency.
> >
> > Signed-off-by: r.burenkov <panzercheg@gmail.com>
>
> 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

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.

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> On Mon, 9 Dec 2019, Junio C Hamano wrote:
>
>> "panzercheg via GitGitGadget" <gitgitgadget@gmail.com> 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 <panzercheg@gmail.com>
>> >
>> > Use the value configured in lfs.storage, if exists, as all the
>> > "git" operations do, for consistency.
>> >
>> > Signed-off-by: r.burenkov <panzercheg@gmail.com>
>>
>> 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 <panzercheg@gmail.com>

"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 <panzercheg@gmail.com>
---
 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

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.

[Johannes Schindelin]

Hi Junio,

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

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>
> > On Mon, 9 Dec 2019, Junio C Hamano wrote:
> >
> >> "panzercheg via GitGitGadget" <gitgitgadget@gmail.com> 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 <panzercheg@gmail.com>
> >> >
> >> > Use the value configured in lfs.storage, if exists, as all the
> >> > "git" operations do, for consistency.
> >> >
> >> > Signed-off-by: r.burenkov <panzercheg@gmail.com>
> >>
> >> 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 <panzercheg@gmail.com>
>
> "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 <panzercheg@gmail.com>
> ---
>  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
>