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. 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.