Uploaded image for project: 'Jenkins'
  1. Jenkins
  2. JENKINS-59139

don't set GIT_LFS_SKIP_SMUDGE=1 anymore while cloning with git 2.15+

    Details

    • Similar Issues:

      Description

      Since commit 853603cccd4434b116ef9b8e094c3f5b815aa75a, we set GIT_LFS_SKIP_SMUDGE=1  to improve performance, and require users to add the "Git LFS pull after checkout" additional behaviour to resolve pointers. 

      This was mostly done because git clone performance was pretty bad with the smudge filter enabled, and in some configurations, (ssh clone URLs), git lfs pull didn't work.

      Since then, a lot has improved:

      Clone performance through the smudge filter has improved since https://github.com/git-lfs/git-lfs/pull/2511.

      Backends should support the git-lfs-authenticate dance as described in https://github.com/git-lfs/git-lfs/blob/master/docs/api/authentication.md#ssh, which retrieves a temporary authorization token to do the clone over https.

      I propose to just do a git clone without setting GIT_LFS_SKIP_SMUDGE=1. Most systems should have done a git lfs install before, so they will resolve the lfs pointers during a clone. We can keep the "Git LFS pull after checkout" additional behaviour, for systems that don't have setup global smudge filters via git lfs install.

        Attachments

          Issue Links

            Activity

            flokli Florian Klink created issue -
            markewaite Mark Waite made changes -
            Field Original Value New Value
            Assignee Mark Waite [ markewaite ]
            Hide
            markewaite Mark Waite added a comment - - edited

            Your description seems reasonable to me. I want to see the results of evaluating a pull request on the Linux (Amazon Linux, CentOS, Debian, Ubuntu), Windows, and FreeBSD environments where I run tests regularly. Will you submit the pull request?

            PR 2511 to git-lfs seems to indicate that specific versions of command line git and of git lfs are needed in order to gain the benefit of the change. Can you confirm that command line git versions 2.15 and later are needed and that git lfs 2.3.0 and later are needed?

            If so, then the pull request will need to check the command line git version and the git lfs version before removing the GIT_LFS_SKIP_SMUDGE environment variable. If that conditional is not included in the change, then many operating systems that deliver older versions of command line git or older versions of git lfs will suffer a performance loss from removing that environment variable.

            Operating System CLI git version delivered
            Amazon Linux 2 2.17
            CentOS 6 1.7.1
            CentOS 7 1.8
            Debian 9 2.11
            Debian 10 2.20
            Ubuntu 16 2.7
            Ubuntu 18 2.17
            FreeBSD 12 2.21
            Show
            markewaite Mark Waite added a comment - - edited Your description seems reasonable to me. I want to see the results of evaluating a pull request on the Linux (Amazon Linux, CentOS, Debian, Ubuntu), Windows, and FreeBSD environments where I run tests regularly. Will you submit the pull request? PR 2511 to git-lfs seems to indicate that specific versions of command line git and of git lfs are needed in order to gain the benefit of the change. Can you confirm that command line git versions 2.15 and later are needed and that git lfs 2.3.0 and later are needed? If so, then the pull request will need to check the command line git version and the git lfs version before removing the GIT_LFS_SKIP_SMUDGE environment variable. If that conditional is not included in the change, then many operating systems that deliver older versions of command line git or older versions of git lfs will suffer a performance loss from removing that environment variable. Operating System CLI git version delivered Amazon Linux 2 2.17 CentOS 6 1.7.1 CentOS 7 1.8 Debian 9 2.11 Debian 10 2.20 Ubuntu 16 2.7 Ubuntu 18 2.17 FreeBSD 12 2.21
            Hide
            flokli Florian Klink added a comment -

            I did some quick digging, git >= 2.15 and git-lfs >= 2.3.0 sounds reasonable, by looking at the tags containing commits part of those PRs.

            `git-client-plugin` currently lacks a way to determine the version of git-lfs, but that should not be too hard to add.

             

            However, changing the logic might be a bit more involved if we want to do the version check as described above (and I think it makes sense to do so).

             

            Currently, the `GitLFSPull` `GitSCMExtension` ("Git LFS pull after checkout") simply calls `CliGitAPIImpl.lfsRemote` with `repos.get(0).getName()` - and inside `CliGitAPIImpl`, all LFS-related functionality is conditional on `(lfsRemote != null)`.

             

            If we now want to rely on the smudge filter for recent enough git(-lfs) versions, and only have the `GitLFSPull` `GitSCMExtension` to trigger `git lfs pull` for old git(-lfs) versions,`GitLFSPull` should be changed to simply flip a boolean in `CliGitAPIImpl`, and the logic in there should  be made around that. Does that sound right to you?

            Show
            flokli Florian Klink added a comment - I did some quick digging, git >= 2.15 and git-lfs >= 2.3.0 sounds reasonable, by looking at the tags containing commits part of those PRs. `git-client-plugin` currently lacks a way to determine the version of git-lfs, but that should not be too hard to add.   However, changing the logic might be a bit more involved if we want to do the version check as described above (and I think it makes sense to do so).   Currently, the `GitLFSPull` `GitSCMExtension` ("Git LFS pull after checkout") simply calls `CliGitAPIImpl.lfsRemote` with `repos.get(0).getName()` - and inside `CliGitAPIImpl`, all LFS-related functionality is conditional on `(lfsRemote != null)`.   If we now want to rely on the smudge filter for recent enough git(-lfs) versions, and only have the `GitLFSPull` `GitSCMExtension` to trigger `git lfs pull` for old git(-lfs) versions,`GitLFSPull` should be changed to simply flip a boolean in `CliGitAPIImpl`, and the logic in there should  be made around that. Does that sound right to you?
            Hide
            markewaite Mark Waite added a comment -

            That sounds reasonable to me.

            Show
            markewaite Mark Waite added a comment - That sounds reasonable to me.
            Hide
            flokli Florian Klink added a comment -

            I fear I won't find the time to actually do this, sorry - hopefully somebody else can pick up from here.

            Show
            flokli Florian Klink added a comment - I fear I won't find the time to actually do this, sorry - hopefully somebody else can pick up from here.
            markewaite Mark Waite made changes -
            Labels newbie-friendly
            markewaite Mark Waite made changes -
            Summary don't set GIT_LFS_SKIP_SMUDGE=1 anymore while cloning don't set GIT_LFS_SKIP_SMUDGE=1 anymore while cloning with git 2.15+
            renescheibe René Scheibe made changes -
            Link This issue relates to JENKINS-56569 [ JENKINS-56569 ]
            renescheibe René Scheibe made changes -
            Link This issue relates to JENKINS-47531 [ JENKINS-47531 ]
            Hide
            renescheibe René Scheibe added a comment -

            Please also keep JENKINS-47531 in mind. When a password is provided, it can only be used if git-lfs did not yet run automatically on checkout.

            See https://github.com/jenkinsci/git-client-plugin/pull/280.

            Show
            renescheibe René Scheibe added a comment - Please also keep JENKINS-47531 in mind. When a password is provided, it can only be used if git-lfs did not yet run automatically on checkout. See https://github.com/jenkinsci/git-client-plugin/pull/280 .
            Hide
            yamhai yamhai added a comment -

            I've submitted a PR for this: https://github.com/jenkinsci/git-client-plugin/pull/476. Can you review it?

            Should I modify it as per Florian's last comment, where as far as I understand, all I need to do is change (lfsRemote != null) logic to something like (isLfsPresent) being initialized from `GitLFSPull`. Is that right?

            Show
            yamhai yamhai added a comment - I've submitted a PR for this:  https://github.com/jenkinsci/git-client-plugin/pull/476 . Can you review it? Should I modify it as per Florian's last comment, where as far as I understand, all I need to do is change  (lfsRemote != null)  logic to something like (isLfsPresent) being initialized from `GitLFSPull`. Is that right?
            renescheibe René Scheibe made changes -
            Description Since [https://github.com/jenkinsci/git-client-plugin/commit/853603cccd4434b116ef9b8e094c3f5b815aa75a,] we set GIT_LFS_SKIP_SMUDGE=1  to improve performance, and require users to add the "Git LFS pull after checkout" "Additional behaviour" to resolve pointers.

             

            This was mostly done because git clone performance was pretty bad with the smudge filter enabled, and in some configurations, (ssh clone URLs), git lfs pull didn't work.

             

            Since then, a lot has improved:

             

            Clone Performance through the smudge filter has improved since [https://github.com/git-lfs/git-lfs/pull/2511.|https://github.com/git-lfs/git-lfs/pull/2511

            Backends should support the `git-lfs-authenticate` dance as described in [https://github.com/git-lfs/git-lfs/blob/master/docs/api/authentication.md#ssh], which retrieves a temporary authorization token to do the clone over https.

             

            I propose to just do a git clone without setting `GIT_LFS_SKIP_SMUDGE=1`. Most systems should have done a `git lfs install` before, so they will resolve the lfs pointers during a clone. We can keep the  "Git LFS pull after checkout" "Additional behaviour", for systems that don't have setup global smudge filters via `git lfs install`.
            Since commit [853603cccd4434b116ef9b8e094c3f5b815aa75a|https://github.com/jenkinsci/git-client-plugin/commit/853603cccd4434b116ef9b8e094c3f5b815aa75a], we set {{GIT_LFS_SKIP_SMUDGE=1}}  to improve performance, and require users to add the "Git LFS pull after checkout" additional behaviour to resolve pointers. 

            This was mostly done because git clone performance was pretty bad with the smudge filter enabled, and in some configurations, (ssh clone URLs), git lfs pull didn't work.

            Since then, a lot has improved:

            Clone performance through the smudge filter has improved since [https://github.com/git-lfs/git-lfs/pull/2511].

            Backends should support the {{git-lfs-authenticate}} dance as described in [https://github.com/git-lfs/git-lfs/blob/master/docs/api/authentication.md#ssh], which retrieves a temporary authorization token to do the clone over https.

            I propose to just do a git clone without setting {{GIT_LFS_SKIP_SMUDGE=1}}. Most systems should have done a {{git lfs install}} before, so they will resolve the lfs pointers during a clone. We can keep the "Git LFS pull after checkout" additional behaviour, for systems that don't have setup global smudge filters via {{git lfs install}}.

              People

              • Assignee:
                Unassigned
                Reporter:
                flokli Florian Klink
              • Votes:
                0 Vote for this issue
                Watchers:
                5 Start watching this issue

                Dates

                • Created:
                  Updated: