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.

From: Junio C Hamano <gitster@pobox.com>
Date: Dec 4, 2019, 16:51 UTC
Message-ID: <xmqqh82g5910.fsf@gitster-ct.c.googlers.com>
In-Reply-To: <73d0dfc9dd7e6e800297b5084de84b727e28b900.1575466209.git.gitgitgadget@gmail.com>

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

Show 15 quoted lines

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

Show 8 quoted lines

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