Re: [PATCH] Prototype register-path

[ludo@gnu.org (Ludovic Courtès)]

[-- Attachment #1: Type: text/plain, Size: 2701 bytes --]

Hi reepca!

I gave this patch set a try and looked at the code, and it looks very
good to me!

“make check TESTS=tests/store.scm” passes now with the fixes you posted
today (‘getenv’ & co.)

Like you say, what’s missing from ‘register-path’ now is a timestamp
reset phase (‘reset-timestamps’ in (gnu build install) should cover
that), and a deduplication phase (which you’ll have to implement).  That
would be a good next step IMO.

For deduplication, you already know the (guix hash) API that will allow
you to do that I guess.  The “deduplication database” is simply the
/gnu/store/.links directory.  Each entry there is a hard link to a file;
the name of the entry is the hex representation of the sha256 hash of
that file.  So when inserting a file in the store, all you need is to
look up its hash in /gnu/store/.links and hard-link from there.  See
what I mean?

Some minor suggestions about the code:

  • Please add a copyright line for yourself in files you modify, and
    add the usual GPLv3+ header in new files.  :-)

  • When you add new ‘with-something’ macros, you can tell Emacs (if
    that’s what you use) to ident them like the other ‘with-’ forms by
    modifying .dir-locals.el; there are several examples of that there.

  • I think ‘register-path’ doesn’t have to explicitly do a bunch of
    ‘getenv’ calls.  Instead it should use the variables defined in
    (guix config).  For instance ‘%store-directory’ and
    ‘%state-directory’ are already defined using ‘getenv’.  There’s
    currently nothing for NIX_DB_DIR but you could define
    ‘%store-database-directory’ similarly.

  • It would probably make sense to use the (guix store …) name space
    for modules that implement the store.  So we could have (guix store
    database) for the subset of (guix sql) that deals with
    /var/guix/db/db.sqlite, (guix store deduplication), and so on.

  • You get bonus points if you can squash “fixup” commits with the
    commit you’re conceptually amending.  :-)  Having a single patch
    that implements something makes it easier for others to review.  If
    you’re not familiar with ‘git rebase -i’ & co., that’s OK though,
    don’t bother.

  • Extra bonus points if you follow our commit log conventions as
    discussed at
    <https://www.gnu.org/software/guix/manual/html_node/Submitting-Patches.html>.
    :-)

What about pushing your changes to a WIP branch on Savannah or
elsewhere?  (If you have an account on Savannah we can give you access.)

Thank you, and thumbs up for the quality work so far!

Ludo’.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]