Arvados

12107-faster-git @ 32341a4777692a28358f2af5afa0d9e975e2da73

• If the commit is already present, just tag it.
• If the commit isn't present, use fetch instead of pack-objects|unpack-objects.

Running services/api tests on my Debian 9 VM produced a lot of errors, like this one:

74) Error:
JobTest#test_verify_job_status_[["state",_nil,_[["state",_"Queued"]]]]:
Commit::GitError: git --git-dir /home/lucas/arvados_local/services/api/tmp/internal.git fetch --no-tags file:///home/lucas/arvados_local/services/api/tmp/git/test/zzzzz-s0uqq-382brsig8rp3666/.git 077ba2ad3ea24a929091a9e6ce545c93199b8e57:refs/tags/zzzzz-8i9sb-3tdw50ao2bulisx 2>&1: pid 23753 exit 1: 
    app/models/commit.rb:240:in `must_pipe'
    app/models/commit.rb:228:in `block in must_git'
    app/models/commit.rb:227:in `each'
    app/models/commit.rb:227:in `must_git'
    app/models/commit.rb:161:in `tag_in_internal_repository'
    app/models/job.rb:443:in `tag_version_in_internal_repository'
    test_after_commit (1.1.0) lib/test_after_commit/database_statements.rb:11:in `block in transaction'
    test_after_commit (1.1.0) lib/test_after_commit/database_statements.rb:5:in `transaction'
    test/unit/job_test.rb:234:in `block (2 levels) in <class:JobTest>’

Not sure what's causing this. The git version I'm using is 2.11.0.

Not sure why the same tests didn't fail for me but I have been able to reproduce this with "git fetch".

I found this:

No, out of security concerns; imagine you included some proprietary source code by mistake, and undo the damage by forcing a push with a branch that does not have the incriminating code. Usually you do not control the garbage-collection on the server, yet you still do not want other people to fetch “by SHA-1”.

Silently exiting 1 is rather unfortunate, and most references to this question say the remote "server" decides whether to honor a fetch by sha1.

Ok, I ran the tests on jenkins just to be sure, and they passed without issues. LGTM, thanks!

(continuing from note-11, which I saved mid-investigation)

1. Check whether commit is already present in internal.git. If so, just tag it and stop.
2. Ask git for a list of branches that include the desired commit. Take the first one and git-fetch it. Tag the desired commit. If all of these steps work, stop.
3. Last resort: pack-objects|unpack-objects like we did before.

There are a number of things that can go wrong in step 2 (commit isn't on any branch, regexp isn't good enough to catch the branch name, selected branch gets non-FF pushed just before we fetch and no longer has the commit we want) but in all of these cases, we just try the pack-unpack fallback -- if that doesn't work, nothing would.

12107-faster-git @ bd5a371cc0f57aea20c0954f532db5890dae5c59

• initializes the internal.git repo at test setup time instead of preserving internal.git from one test run to the next (tests were passing for me with broken code, because a previous version's test suite had already fetched all the tested commits into internal.git)
• adds tests for a "not on any branch" commit and a "not at tip of any branch" commit

https://ci.curoverse.com/job/developer-run-tests/462/

Just one comment:

• File services/api/app/models/commit.rb
  • Lines 176 & 181: I think those must_git() calls could be condensed into one by using the ensure block, or maybe place it immediately after the begin/rescue block?

The rest LGTM.

Lucas Di Pentima wrote:

• Lines 176 & 181: I think those must_git() calls could be condensed into one by using the ensure block, or maybe place it immediately after the begin/rescue block?

Added protective comment:

        # Even if all of the above steps succeeded, we might still not
        # have the right commit due to a race, in which case tag_cmd
        # will fail, and we'll need to fall back to pack|unpack. So
        # don't be tempted to condense this tag_cmd and the one in the
        # rescue block into a single attempt.
        must_git(dst_gitdir, tag_cmd)