Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Return value of git checkout with lfs errors is 0 #731

Closed
andyneff opened this issue Oct 12, 2015 · 12 comments
Closed

Return value of git checkout with lfs errors is 0 #731

andyneff opened this issue Oct 12, 2015 · 12 comments
Labels
bug

Comments

@andyneff
Copy link
Contributor

@andyneff andyneff commented Oct 12, 2015

When I have a long checkout, some of my files were failing due to poor network conditions, however the git checkout command I was using did not return an error code.

To reproduce this:

git init
git lfs init
git config -f .gitconfig lfs.url https://lfs-test-server/username/project.git
git remote add origin http://github.com/username/project.git
git fetch
git checkout andy_point
echo $?

I temporarily disconnected the network device to simulate network errors,

Downloading external/src/SOURCES/ippicv_linux_20141027.tgz (26.73 MB)
Downloading external/src/SOURCES/ipython-3.1.0.tar.gz (10.41 MB)
Error accessing media: external/src/SOURCES/ipython-3.1.0.tar.gz (532092d3f06f82b1d8d1e5c37097eae19fcf025f8f6a4b670dd49c3c338d5624)

Errors logged to .git/lfs/objects/logs/20151012T141710.374778719.log
Use `git lfs logs last` to view the log.
Downloading external/src/SOURCES/itsdangerous-0.24.tar.gz (45.45 KB)
Error accessing media: external/src/SOURCES/itsdangerous-0.24.tar.gz (cbb3fcf8d3e33df861709ecaf89d9e6629cff0a217bc2848f1b41cd30d360519)

Errors logged to .git/lfs/objects/logs/20151012T141710.402204943.log
Use `git lfs logs last` to view the log.
Downloading external/src/SOURCES/json-c-0.12.tar.gz (489.67 KB)
Error accessing media: external/src/SOURCES/json-c-0.12.tar.gz (000c01b2b3f82dcb4261751eb71f1b084404fb7d6a282f06074d3c17078b9f3f)

Errors logged to .git/lfs/objects/logs/20151012T141711.315578821.log
Use `git lfs logs last` to view the log.
Downloading external/src/SOURCES/jsonschema-2.4.0.tar.gz (47.16 KB)
Error accessing media: external/src/SOURCES/jsonschema-2.4.0.tar.gz (1298a2f1b2f4c4a7b921cccd159e4e42f6d7b0fb75c86c0cdecfc71f061833fa)

Errors logged to .git/lfs/objects/logs/20151012T141713.571663218.log
Use `git lfs logs last` to view the log.
Downloading external/src/SOURCES/kombu-3.0.26.tar.gz (366.75 KB)
Error accessing media: external/src/SOURCES/kombu-3.0.26.tar.gz (1f565abd44c4b7dfaa4dd543d52f982d2f006aba0a2b3830542b4d25a801fe09)

Errors logged to .git/lfs/objects/logs/20151012T141713.808404969.log
Use `git lfs logs last` to view the log.
Downloading external/src/SOURCES/lapack-3.5.0.tgz (6.02 MB)
Downloading external/src/SOURCES/lapack-3.5.0_manpages.tgz (1.33 MB)

However the return code for git checkout is 0

I would think it makes sense that git checkout should return non-zero in this case. If not, how can I detect that errors occurred and fix them/try again?

git lfs version 1.0.0 AMD64, also tested 0.5.4
git version 2.4.5
Running Centos 6.6 Linux

@technoweenie

@technoweenie

This comment has been minimized.

Copy link
Member

@technoweenie technoweenie commented Oct 12, 2015

Obviously we can't control what git checkout does. We should double check that git lfs smudge is returning a non zero exit code though. Also, do you have the required key set?

$ git config -l | grep lfs
filter.lfs.clean=git-lfs clean %f
filter.lfs.smudge=git-lfs smudge %f
filter.lfs.required=true

That "required" option should force it to error if the smudge command fails. I'm assuming you do though, and that the smudge command needs to be fixed.

@technoweenie technoweenie added the bug label Oct 12, 2015
@rubyist

This comment has been minimized.

Copy link
Contributor

@rubyist rubyist commented Oct 12, 2015

We definitely don't currently exit 1 when there's an error on smudge.

@andyneff

This comment has been minimized.

Copy link
Contributor Author

@andyneff andyneff commented Oct 12, 2015

@technoweenie filter.lfs.required is true.

@andyneff

This comment has been minimized.

Copy link
Contributor Author

@andyneff andyneff commented Oct 12, 2015

#732 Does what I expected. Thanks @rubyist!

@andyneff

This comment has been minimized.

Copy link
Contributor Author

@andyneff andyneff commented Oct 12, 2015

Just a side note on #732, the rest of the files after the failed file are not attempted (liek they were before the commit) I'm 100% ok with that and think it's good behavior. But others may have considered it a feature... I get why this is how it has to be because of the decoupled nature of smudge and checkout.

That being said, I suppose for those that WANT that behavior, the logical command would be git lfs fetch to do this? I just did a similar test on that (using 26af3bc). git lfs fetch appears to skip the failed fails and continue on. However...

  • It does NOT return a non-zero error code
  • It does NOT print to stdout or stderr that something went wrong
  • It does NOT generate any logs that I can find
@sinbad

This comment has been minimized.

Copy link
Contributor

@sinbad sinbad commented Oct 13, 2015

Any errors from the transfer queue are written to stderr unless debugging or it's a fatal error:

    for _, err := range q.Errors() {
        if Debugging || lfs.IsFatalError(err) {
            LoggedError(err, err.Error())
        } else {
            Error(err.Error())
        }
    }

Download errors are not considered fatal, and that's because you usually don't want to abort all downloads when one of them fails. For example a server problem which means 1 file is not provided should not prevent retrieval of all other files. For this reason I think it's wrong for the smudge filter to halt the checkout if one download fails - if it's intermittent that's fine, but you need to avoid halting a user in their tracks permanently because of a server-side problem. GIT_LFS_SKIP_SMUDGE does offer a way to bypass the problems but only by downloading nothing instead.

So my personal opinion is that changing smudge to fail the checkout as in #732 sounds reasonable but will actually cause more problems in practice. It's a shame we can't cause git checkout to return a non-zero result at the end while not halting the checkout. I wonder whether the wrapper commands we've talked about for clone/checkout might resolve that as well as the performance issues.

@sinbad

This comment has been minimized.

Copy link
Contributor

@sinbad sinbad commented Oct 13, 2015

Having said that, git lfs fetch should return a non-zero when there were errors. It just shouldn't stop early like git checkout will.

@jhoblitt

This comment has been minimized.

Copy link

@jhoblitt jhoblitt commented Oct 20, 2015

I've run into this with v1.0.0 as well when there are transfer errors. Is #734 a complete fix for this?

Error accessing media: test/cal-53535-i-797722_1.fits (f6b0597622934ba80fabc1d1c6afb83004215e9fc00432350444c18082030d2f)

Errors logged to .git/lfs/objects/logs/20151020T111115.396104362.log
Use `git lfs logs last` to view the log.
Checking out files: 100% (226/226), done.
[master] ~/tmp $ echo $?
0
@technoweenie

This comment has been minimized.

Copy link
Member

@technoweenie technoweenie commented Oct 20, 2015

@jhoblitt What command did you run?

BTW: #734 is merged for Git LFS 1.0.1 and is not part of any released version of Git LFS.

@jhoblitt

This comment has been minimized.

Copy link

@jhoblitt jhoblitt commented Oct 20, 2015

@technoweenie The snippet I posted was from a git clone... but it definitely happens with git-lfs fetch. There seem to be network glitches effecting our lfs backend server (not github) but I suspect any sort of I/O failure would manifest the same way. This is an issue for me as I'm doing unattended clones/fetches from a CI system.

@technoweenie

This comment has been minimized.

Copy link
Member

@technoweenie technoweenie commented Oct 20, 2015

#734 only affects fetch, but #732 does the same for the internal smudge filter used during clones.

@jhoblitt

This comment has been minimized.

Copy link

@jhoblitt jhoblitt commented Oct 20, 2015

@technoweenie 👍 Looks good with current master.

$ git-lfs version
git-lfs/1.0.0 (GitHub; linux amd64; go 1.5.1; git 3a673d6)
                                                  ^^^^^^^
$ git clone -c credential.helper='!f() { cat > /dev/null; echo username=; echo password=; }; f' https://github.com/lsst/afwdata-cowboy.git
Cloning into 'afwdata-cowboy'...
remote: Counting objects: 411, done.
remote: Total 411 (delta 0), reused 0 (delta 0), pack-reused 411
Receiving objects: 100% (411/411), 54.89 KiB | 0 bytes/s, done.
Resolving deltas: 100% (51/51), done.
Checking connectivity... done.
Downloading CFHT/D2/sdss.dat (110.68 KB)
Downloading CFHT/D2/template.dat (4.25 MB)
Downloading CFHT/D4/cal-53535-i-797722_1.fits (93.60 MB)
Downloading CFHT/D4/cal-53535-i-797722_1_tmpl.fits (84.34 MB)
Downloading CFHT/D4/cal-53535-i-797722_2.fits (93.60 MB)
Downloading CFHT/D4/cal-53535-i-797722_2_tmpl.fits (84.34 MB)
Downloading CFHT/D4/cal-53535-i-797722_3.fits (93.60 MB)
Downloading CFHT/D4/cal-53535-i-797722_3_tmpl.fits (84.34 MB)
Downloading CFHT/D4/cal-53535-i-797722_4.fits (93.60 MB)
Downloading CFHT/D4/cal-53535-i-797722_4_tmpl.fits (84.34 MB)
Downloading CFHT/D4/cal-53535-i-797722_5.fits (93.60 MB)
Downloading CFHT/D4/cal-53535-i-797722_5_tmpl.fits (84.34 MB)
Downloading CFHT/D4/cal-53535-i-797722_6.fits (93.60 MB)
Downloading CFHT/D4/cal-53535-i-797722_6_tmpl.fits (84.34 MB)
Downloading CFHT/D4/cal-53535-i-797722_7.fits (93.60 MB)
Downloading CFHT/D4/cal-53535-i-797722_7_tmpl.fits (84.34 MB)
Downloading CFHT/D4/cal-53535-i-797722_8.fits (93.60 MB)
Downloading CFHT/D4/cal-53535-i-797722_8_tmpl.fits (84.34 MB)
Downloading CFHT/D4/cal-53535-i-797722_9.fits (93.60 MB)
Downloading CFHT/D4/cal-53535-i-797722_9_tmpl.fits (84.34 MB)
Downloading CFHT/D4/cal-53535-i-797722_small_1.fits (658.12 KB)
Downloading CFHT/D4/cal-53535-i-797722_small_1_tmpl.fits (658.12 KB)
Downloading DC3a-Sim/sci/v26-e0/v26-e0-c011-a00.sci.fits (10.02 MB)
Downloading DC3a-Sim/sci/v26-e0/v26-e0-c011-a10.sci.fits (10.02 MB)
Downloading DC3a-Sim/sci/v5-e0/v5-e0-c011-a00.sci.fits (10.02 MB)
Downloading DC3a-Sim/sci/v5-e0/v5-e0-c011-a10.sci.fits (10.02 MB)
Downloading ImSim/bias/v0/R23/S11/imsim_0_R23_S11_C00.fits (9.86 MB)
Downloading ImSim/bias/v0/R23/S11/imsim_0_R23_S11_C01.fits (9.86 MB)
Downloading ImSim/bias/v0/R23/S11/imsim_0_R23_S11_C02.fits (9.86 MB)
Downloading ImSim/bias/v0/R23/S11/imsim_0_R23_S11_C03.fits (9.86 MB)
Downloading ImSim/bias/v0/R23/S11/imsim_0_R23_S11_C04.fits (9.86 MB)
Downloading ImSim/bias/v0/R23/S11/imsim_0_R23_S11_C05.fits (9.86 MB)
Downloading ImSim/bias/v0/R23/S11/imsim_0_R23_S11_C06.fits (9.86 MB)
Downloading ImSim/bias/v0/R23/S11/imsim_0_R23_S11_C07.fits (9.86 MB)
Error accessing media: ImSim/bias/v0/R23/S11/imsim_0_R23_S11_C07.fits (a26457b4cafef8de5bba11b53c6474988d67cfdc9eee455a0daa248ea3f7fa79)

Errors logged to .git/lfs/objects/logs/20151020T154616.813764632.log
Use `git lfs logs last` to view the log.
error: external filter git-lfs smudge %f failed 2
error: external filter git-lfs smudge %f failed
fatal: ImSim/bias/v0/R23/S11/imsim_0_R23_S11_C07.fits: smudge filter lfs failed
warning: Clone succeeded, but checkout failed.
You can inspect what was checked out with 'git status'
and retry the checkout with 'git checkout -f HEAD'

$ echo $?
128
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.