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

From: panzercheg <[email protected]>

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

index 60c73b6a37..2bd0497c31 100755
@@ -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

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

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.

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 <[email protected]>

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

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

From: panzercheg <[email protected]>

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

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

index 60c73b6a37..0b3a07cb31 100755
@@ -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

Oops, what happened here?

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

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

There is an open issue[1] regarding that.

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

Hi Junio,

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

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

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

Ciao,
Dscho

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 <[email protected]>

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

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

index 60c73b6a37..0b3a07cb31 100755
@@ -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

Hi Junio,

I fully agree.

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

Thanks,
Dscho