git-p4: use lfs.storage instead of local .git/lfs #483
Conversation
Welcome to GitGitGadgetHi @panzercheg, and welcome to GitGitGadget, the GitHub App to send patch series to the Git mailing list from GitHub Pull Requests. Please make sure that this Pull Request has a good description, as it will be used as cover letter. Also, it is a good idea to review the commit messages one last time, as the Git project expects them in a quite specific form:
It is in general a good idea to await the automated test ("Checks") in this Pull Request before contributing the patches, e.g. to avoid trivial issues such as unportable code. Contributing the patchesBefore you can contribute the patches, your GitHub username needs to be added to the list of permitted users. Any already-permitted user can do that, by adding a comment to your PR of the form Both the person who commented An alternative is the channel Once on the list of permitted usernames, you can contribute the patches to the Git mailing list by adding a PR comment After you submit, GitGitGadget will respond with another comment that contains the link to the cover letter mail in the Git mailing list archive. Please make sure to monitor the discussion in that thread and to address comments and suggestions. If you want to see what email(s) would be sent for a submit request, add a PR comment If you do not want to subscribe to the Git mailing list just to be able to respond to a mail, you can download the mbox ("raw") file corresponding to the mail you want to reply to from the Git mailing list. If you use GMail, you can upload that raw mbox file via: curl -g --user "<EMailAddress>:<Password>" --url "imaps://imap.gmail.com/INBOX" -T /path/to/raw.txtNeed help?New contributors who want advice are encouraged to join git-mentoring@googlegroups.com, where volunteers who regularly contribute to Git are willing to answer newbie questions, give advice, or otherwise provide mentoring to interested contributors. You must join in order to post or view messages, but anyone can join. You may also be able to find help in real time in the developer IRC channel, |
panzercheg
force-pushed
the
git-p4-improve-lfs-logic
branch
from
9dfe4cf
to
6429147
Compare
|
/allow |
|
User panzercheg is now allowed to use GitGitGadget. WARNING: panzercheg has no public email address set on GitHub |
|
@panzercheg please wrap the commit message at <=76 columns per line. You may also want to consider using your real name in the "Signed-off-by:" line and as your |
panzercheg
force-pushed
the
git-p4-improve-lfs-logic
branch
2 times, most recently
from
ebbc915
to
73d0dfc
Compare
commit message fixed |
|
Thanks @panzercheg ! |
|
/submit |
|
Error: Could not determine full name of panzercheg |
|
/submit |
|
Submitted as pull.483.git.1575466209.gitgitgadget@gmail.com WARNING: panzercheg has no public email address set on GitHub |
![gitgitgadget[bot]](https://avatars.githubusercontent.com/in/12836?s=60&v=4){kind=small}
| @@ -1257,9 +1257,13 @@ def generatePointer(self, contentFile): | |||
| pointerFile = re.sub(r'Git LFS pointer for.*\n\n', '', pointerFile) | |||
There was a problem hiding this comment.
On the Git mailing list, Junio C Hamano wrote (reply to this):
"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.
There was a problem hiding this comment.
First at all - thank you for your comments
The reason why you ended up with this unreadably long subject line
is because your commit did not follow the recommended
Yep, this is my mistake - i thought the subject of the letter is "git-p4: use lfs.storage instead of local .git/lfs"But in https://public-inbox.org/git/pull.483.git.1575466209.gitgitgadget@gmail.com/#t i see the next email subject:"Subject: [PATCH 0/1] git-p4: use lfs.storage instead of local .git/lfs" and "Subject: [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." - so i little confused how that works.
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?
git.config is the reference to [1] and in .lfsconfig we doesnt have configuration for some kind lfs storage, as writed in [*2*], so its config file, which called via cmd = [ "git", "config" ] - it can be global or local .gitconfig file.
Possible rewrite
Yes, thats make sense. Thanks. The reason why I suspect so is because [*1*] says that the value of Thank you for your comment - its more correct to use in that way
[Reference]
1 git@692e179
2 https://github.com/git-lfs/git-lfs/blob/master/docs/man/git-lfs-config.5.ronn
There was a problem hiding this comment.
@panzercheg first of all, Junio cannot see your replies here, you really need to send them as mail, as indicated in the reply to this link at the top of the comment you replied to.
Also, the /preview command needs to be in a top-level comment, not in a code comment thread.
panzercheg
force-pushed
the
git-p4-improve-lfs-logic
branch
from
73d0dfc
to
a6867b2
Compare
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>
panzercheg
force-pushed
the
git-p4-improve-lfs-logic
branch
from
a6867b2
to
e65375c
Compare
|
/submit |
|
Submitted as pull.483.v2.git.1575901722.gitgitgadget@gmail.com WARNING: panzercheg has no public email address set on GitHub |
| @@ -1257,9 +1257,15 @@ def generatePointer(self, contentFile): | |||
| pointerFile = re.sub(r'Git LFS pointer for.*\n\n', '', pointerFile) | |||
There was a problem hiding this comment.
On the Git mailing list, Junio C Hamano wrote (reply to this):
"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.
There was a problem hiding this comment.
On the Git mailing list, Eric Sunshine wrote (reply to this):
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
There was a problem hiding this comment.
On the Git mailing list, Johannes Schindelin wrote (reply to this):
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
There was a problem hiding this comment.
On the Git mailing list, Junio C Hamano wrote (reply to this):
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
There was a problem hiding this comment.
On the Git mailing list, Johannes Schindelin wrote (reply to this):
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
>
|
This patch series was integrated into pu via git@f471729. |
|
This branch is now known as |
|
This patch series was integrated into pu via git@d87a668. |
|
This patch series was integrated into pu via git@6ef61ec. |
|
This patch series was integrated into pu via git@c69c402. |
|
This patch series was integrated into pu via git@3dd7aa6. |
|
This patch series was integrated into next via git@0e6e76a. |
|
This patch series was integrated into pu via git@6dd924c. |
|
This patch series was integrated into pu via git@20aa6d8. |
|
This patch series was integrated into next via git@20aa6d8. |
|
This patch series was integrated into master via git@20aa6d8. |
|
Closed via 20aa6d8. |
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.