Re: [PATCH v2 04/16] files-backend: replace *git_path*() with files_path()

On Mon, Feb 20, 2017 at 6:23 PM, Michael Haggerty [off-list ref] wrote:

On 02/16/2017 12:48 PM, Nguyễn Thái Ngọc Duy wrote:

quoted

This centralizes all path rewriting of files-backend.c in one place so
we have easier time removing the path rewriting later. There could be
some hidden indirect git_path() though, I didn't audit the code to the
bottom.

Almost all of the calls to `files_path()` [1] take one of the following
forms:

* `files_path(refs, &sb, "packed-refs")`
* `files_path(refs, &sb, "%s", refname)`
* `files_path(refs, &sb, "logs/%s", refname)`

(though sometimes `refname` is not the name of a reference but rather
the name of a directory containing references, like "refs/heads").

This suggests to me that it would be more natural to have three
functions, `files_packed_refs_path()`, `files_loose_ref_path()`, and
`files_reflog_path()`, with no `fmt` arguments. Aside from making the
callers a bit simpler and the implementation of each of the three
functions simpler (they wouldn't have to deal with variable argument
lists), at the cost of needing three similar functions.

But I think the split would also be advantageous from a design point of
view. The relative path locations of loose reference files, reflog
files, and the packed-refs file is kind of a coincidence, and it would
be advantageous to encode that policy in three functions rather than
continuing to spread knowledge of those assumptions around.

It would also make it easier to switch to a new system of encoding
reference names, for example so that reference names that differ only in
case can be stored on a case-insensitive filesystem, or to store reflogs
using a naming scheme that is not susceptible to D/F conflicts so that
we can retain reflogs for deleted references.

I agree. I didn't see it clearly at the beginning (and made several
mistakes in earlier iterations) but as you have seen files_path() the
separation is pretty clear there. I was going to do it when
introducing the "linked worktree" backend. But since you pointed it
out, I'll update it in this series too.

Michael

[1] The only exception I see is one call `files_path(refs, &sb,
"logs")`, which is a prelude to iterating over the reflog files. This is
actually an example of the code giving us a hint that the design is
wrong: if you iterate only over the directories under `git_path(logs)`,
you miss the reflogs for worktree references.

Oh yes, I had to fix the reflog iterator [1] exactly for that :)

[1] https://public-inbox.org/git/20170217141908.18012-14-pclouds@gmail.com/T/#u
-- 
Duy