Darcs-git: a few notes for Git hackers

Hi,

Here are a few notes about Git that should probably be taken into account by people working on Git itself or on Git wrappers. The notes apply to Linus' Git-0.6, which is the code I'm using in Darcs-git; some of them might no longer be applicable to Darcs.

1. Darcs-git uses the fact that Git updates are atomic when reading
from a Git repository.  Darcs-git almost writes to Git repositories
atomically, with one exception: it performs a non-atomic
read/update/write cycle on .git/HEAD.

For that reason, I'm taking a high-level lock on .git repositories whenever I write them. The lockfile is ``.git/lock''. I haven't thought about whether Darcs can be easily coerced into accessing Git repos atomically; have people writing Git wrappers found the need for a global lock?

2. The files git.h and git.c in Darcs-git are a simple ``libgit'' that
contains just enough functionality for Darcs-git; they use the
functionality of sha1_file.c and read_cache.c from Git-0.6.

I've found a few problems with the interfaces in these files:

 - the global variables sha1_file_directory, active_cache, active_nr
   and active_alloc are not marked ``extern'' in cache.h.  This breaks
   linkers that don't grok common symbols, such as the one in GHCi
   (silly GHCi).

 - the function write_sha1_file takes the metadata and the data in a
   contiguous buffer, which is a problem when the data has been
   allocated by a higher layer.  I'm currently working around the
   problem by memcpy-ing everything into a temp buffer, but that's
   obviously not a good thing.  I don't care whether write_sha1_file
   is changed to use a writev-like interface, or to take the metadata
   explicitly (as in char *type, unsigned long length).

 - there is no (usable) function to write a tree; there's the code in
   write_tree.c, but it's not generally useful.  See the function
   ``git_write_tree_done'' in git.c for the type of interface I'm
   thinking of.

 - there's no way to have multiple simultaneous caches, short of
   hacking at the values of Git's global variables by hand.

As I'd rather not maintain my own version of Git, I'd be mighty grateful if some friendly Git hacker could fix the above.

                                        Juliusz

Dear diary, on Mon, May 09, 2005 at 08:01:25PM CEST, I got a letter where Juliusz Chroboczek <Juliusz.Chroboczek@pps.jussieu.fr> told me that...

FWIW, Cogito does not lock at all yet - this is one of the things which should be fixed soon.

See the Brad Robert's patches of Apr 21. I've decided not to apply them since it appears a lot has changed since then and it would be some pain; but they may be a worthy starting point for a more up-to-date patch.

Ahoj,

I see. Let me know if you decide to use a different name for the lock file so I can switch to using the same one as yours.

                                        Julek

Juliusz Chroboczek wrote:

Are you using flock(), or some other contraption that breaks if a process dies unexpectedly?

	-hpa

No, I'm using a file that is created by the NFS-safe equivalent of open(O_CREAT | O_EXCL). This is what Darcs has been doing basically forever.

Darcs usually doesn't die unexpectedly -- it's a Haskell program, so bugs usually manifest themselves with an exception being thrown allowing Darcs to clean-up after itself.

The one exception is when Darcs gets killed by the OOM killer (which, as you doubtless know, doesn't give any advance warning to a process, thus making it impossible for a process to deal with it gracefully). In such cases, manual intervention is necessary anyway -- a file could have been written half-way.

                                        Juliusz

Juliusz Chroboczek wrote:

In the case of git, it should not be necessary even then; there might be a broken file in the repository but nothing would reference it so it shouldn't have any effect. Functionally speaking, operations on the git repository are in themselves atomic.

	-hpa

Yes, you're right.

I still prefer using a lockfile to flock -- NFS-safety is important for us. And experience with the Darcs user base (who are probably less Unix-savvy then the Git userbase) shows that they have no problem doing

  $ ps
  $ rm _darcs/lock
  $ darcs check

when Darcs complains about a stray lockfile.

                                        Juliusz

Since there's interest, I'll pull tip of your tree and re-do them. I haven't bothered todate since no one seemed interested. Do you want them piece meal like I did last time or just one big diff?

Later, Brad

Dear diary, on Tue, May 10, 2005 at 12:50:33AM CEST, I got a letter where Brad Roberts <braddr@puremagic.com> told me that...

Piece meal would be excellent.

I'd rather see it done against the tip of the Linus tree.

On Mon, 9 May 2005, Brad Roberts wrote:

I wasn't able to finish redoing these against linus tip, but I got most of it done (patches 1-14 of the original 19):

  http://gameboy2.puremagic.com:8090/
  rsync://gameboy2.puremagic.com/git/

The second, third, and forth to last changes need a careful review, they're direct applications of the original patches which were lightly tested during the first round and nothing other than compile tested in this round.

I suspect the remaining parts of the original patch series will go in fairly smoothly. If no one gets to them before tonight I'll finish it up after work.

Later, Brad

The commit comments:

Signed-off-by: Brad Roberts <braddr@puremagic.com>

!-------------------------------------------------------------flip-

- remove the no-longer-true comment about the cache being in native byte order
- move the cache_header struct into read-cache.c since it's in internal detail
  of the cache, not a publicly accessed element

!-------------------------------------------------------------flip-

Drop the active_cache and active_nr parameters to write_cache

!-------------------------------------------------------------flip-

- Introduce set_cache_entry(ce, pos)
- Migrate update-cache.c, the only place that does a active_cache[pos] = ce to use it
- Migrate all the same style code in read-cache.c to use it also, except for
  read_cache itself which is setting up the initial active_cache entries

TODO: rewrite the code that deal with pointers into the active_cache array such as
read-tree.c's merging code.

!-------------------------------------------------------------flip-

Introduce get_cache_entry(int pos) and use it for all trivial calls like:
  ce = active_cache[pos]

TODO: rework the non-trivial active_cache manipulations

!-------------------------------------------------------------flip-

remove active_cache_changed from cache.h

!-------------------------------------------------------------flip-

Introduce get_num_cache_entries() and migrate all the trivial callers to it

!-------------------------------------------------------------flip-

Remove active_alloc from cache.h

!-------------------------------------------------------------flip-

Restructure the diff algorythm to use indexes rather than pointer math. The resulting code is probably a little less efficient but abstracts the data structure.

!-------------------------------------------------------------flip-

Restructure the write tree algorythm to use indexes rather than moving the base pointer and reducing the num entries and start using the cache abstraction apis.

!-------------------------------------------------------------flip-

Move from pointer math to indexes and use the abstractions

!-------------------------------------------------------------flip-

- convert the last caller from touching active_cache directly
- drop active_cache and active_nr from cache.h

!-------------------------------------------------------------flip-

On Mon, 9 May 2005, Juliusz Chroboczek wrote:

I think most things are using the O_CREAT | O_EXCL write to a file and then rename or link/unlink to the desired location. I have some code to do this with refs/*/* as well, and I think people have generally settled on symlinking HEAD to something in refs/heads/. So it shouldn't be necessary to lock the whole repository, unless you're doing some operation like swapping two heads.

Should be trivial to fix.

I've got some patches to make new functions of the write_sha1_file sort easier to write cleanly (for making git-*-pull clean); it wouldn't be too hard to have an open/write/close set.

I'm working on making this cleaner. Are you wanting to write a tree from something other than a cache?

I can post my patches, but Linus is on vacation, so they couldn't go into the mainline until Friday or so anyway.

	-Daniel
*This .sig left intentionally blank*