pre-commit hook complains .git/index.lock being held

[mlawren]

I'm trying to use a pre-commit hook to normalize vcard files[1], which wants to call git-add. But I'm running into the issue that something already has a lock on .git/index.lock.

Mar 03 13:01:58 b xandikos[1445521]: fatal: Unable to create '/var/lib/xandikos/family/contacts/41001478-d6f0-4861-84a5-89d1545b0a02/.git/index.lock': File exists.
Mar 03 13:01:58 b xandikos[1445521]: Another git process seems to be running in this repository, e.g.
Mar 03 13:01:58 b xandikos[1445521]: an editor opened by 'git commit'. Please make sure all processes
Mar 03 13:01:58 b xandikos[1445521]: are terminated then try again. If it still fails, a git process
Mar 03 13:01:58 b xandikos[1445521]: may have crashed in this repository earlier:
Mar 03 13:01:58 b xandikos[1445521]: remove the file manually to continue.

Is it xandikos itself holding the lock?

[1] the desire is to make the vcf consistent in terms of newlines, field order, newline at end of file, etc. Different clients produce different kinds of vcf and trying to debug who changed what is rather hard. Would be nice of xandikos did that itself.

[mlawren]

An additional related issue is that the hook is called in the .git directory which strikes me as ... well, different to command-line interaction. Since there don't seem to be any GIT_* environment variables set perhaps this could be adjusted as well?

[jelmer]

Which version of dulwich are you using? I think there was a fix for this in 0.20.27.

[jelmer]

[1] the desire is to make the vcf consistent in terms of newlines, field order, newline at end of file, etc. Different clients produce different kinds of vcf and trying to debug who changed what is rather hard. Would be nice of xandikos did that itself.

Normalizing file formatting is a different topic, please file a different issue about that.

[mlawren]

Which version of dulwich are you using? I think there was a fix for this in 0.20.27.

I have 0.20.15-1 in Debian stable, but (one quick upgrade later) I see the same behaviour with 0.20.31-1.1 and xandikos 0.2.2-15.

[1] the desire is to make the vcf consistent in terms of newlines, field order, newline at end of file, etc. Different clients produce different kinds of vcf and trying to debug who changed what is rather hard. Would be nice of xandikos did that itself.

Normalizing file formatting is a different topic, please file a different issue about that.

Will do.

[mlawren]

I'm not a Pythonista, but this looks a little suspicious to me: Starting at git.py line 674 the _import_one method appears to lock the index file via locked_index() which takes a lock with dulwich's GitFile class:

            with locked_index(self.repo.index_path()) as index:
                p = os.path.join(self.repo.path, name)
                with open(p, "wb") as f:
                    f.writelines(data)
                st = os.lstat(p)
                blob = Blob.from_string(b"".join(data))
                encoded_name = name.encode(DEFAULT_ENCODING)
                if encoded_name not in index or blob.id != index[encoded_name].sha:
                    self.repo.object_store.add_object(blob)
                    index[encoded_name] = IndexEntry(
                        *index_entry_from_stat(st, blob.id, 0)
                    )
                    self._commit_tree(
                        index, message.encode(DEFAULT_ENCODING), author=author
                    )
                return blob.id

As I read the above, the last action taken before releasing the lock is to make a commit. I could imagine that dulwich commit doesn't mind that it has already taken a lock, but any external process (e.g. pre-commit) might run into troubles. Am I way off base here?

[jelmer]

Oh, right - the fix in Dulwich just addresses the path. So we currently just rely on the Git index lock to prevent concurrent commits to the repository. Imagine that two PUT requests happen at the same time that both want to add a new file - this prevents the one committing first from ending up with both new files in their commit and the other with an empty commit.

We could perhaps add another lock that is specific to Xandikos instead, though downside of that would be that it doesn't prevent concurrent commits with somebody using another tool to manipulate the git repository.

[mlawren]

We could perhaps add another lock that is specific to Xandikos instead, though downside of that would be that it doesn't prevent concurrent commits with somebody using another tool to manipulate the git repository.

This can already happen in practice: if I modify the index by hand, or switch branches before a xandikos request comes in the results will be unexpected. I think this risk is just an inherent part of directly exposing the Git repository, and I have been aware of this in the back of my mind as I've been accessing the Git repo.

So I for one (as a personal user of xandikos) would find a Xandikos-specific thread lock acceptable, given my current desire for working triggers. However, if we managed to solve #157 then my need for a pre-commit hook goes away and we could push this issue's discussion into the future somewhere.

If you really need to enforce user/xandikos separation you can't fully expose the Git working tree. Possible ideas:

1. A CLI wrapper around git that takes a xandikos lock before running commands, or explicitly begins and commits (in the SQL sense) the repo in a transactional manner. But I don't personally find that a desirable situation and won't attempt to justify or promote such an effort.

2. xandikos could operate in its own private (hidden) working tree, pushing to a bare repo which becomes the "exposed" interface to be cloned by the user for non-xandikos activity.

3. Does dulwich support Git blob and ref modification without changing the working tree? What if each xandikos thread/handler updates its own private/internal branch, which gets merged into master (and associated working tree) shortly afterwards?

2 and 3 probably involve something of an architectural change though, with uncertain benefits and downsides...

[jelmer]

Those sorts of races can't happen today if you're only accessing the repository through Xandikos, since Xandikos will always keep the working tree clean.

Re (1) I don't think a cli wrapper is sensible - a major part of the point of using a git repository is that you can use it with regular git tools.

(2) would require creating a separate working tree (for every commit?) which adds extra complexity and performance overhead and possibly having to redo that when you lose a race.

Dulwich does support modification without changing the working tree - in fact, it will happily use a bare git repository. However, bare repositories don't have pre-commit hooks since there is no working tree. (3) doesn't really solve anything except perhaps make it clearer what sort of post-processing has been done, since you'd still need to run the pre-commit hook in the one working tree that is subject to races.

Another alternative to consider is whether we can perhaps lock the branch rather than the index so you could still e.g. add files from the pre-commit hook. I'll take a look this evening.

[jelmer]

The attached branch removes the index lock, but doesn't yet do the branch locking - we'll need a change in Dulwich for that.

[mlawren]

Thanks for the attempt to solve this. I see a few CI errors on that branch, and I'm not really in a position to manually test dulwich, but if you release new Debian packages I can definately give them a go. Otherwise I'll go quiet for a bit until further feedback/input is needed.