On Mon, Feb 20, 2017 at 6:23 PM, Michael Haggerty <mhagger@xxxxxxxxxxxx> wrote: > On 02/16/2017 12:48 PM, Nguyễn Thái Ngọc Duy wrote: >> 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@xxxxxxxxx/T/#u -- Duy