On 03/18/2017 03:03 AM, Nguyễn Thái Ngọc Duy wrote:
> Keep repo-related path handling in one place. This will make it easier
> to add submodule/multiworktree support later.
>
> This automatically adds the "if submodule then use the submodule version
> of git_path" to other call sites too. But it does not mean those
> operations are submodule-ready. Not yet.
Maybe `files_loose_ref_path()` would be a more descriptive name for the
new function. But I can live with it either way.
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@xxxxxxxxx>
> ---
> refs/files-backend.c | 47 +++++++++++++++++++++++------------------------
> 1 file changed, 23 insertions(+), 24 deletions(-)
>
> diff --git a/refs/files-backend.c b/refs/files-backend.c
> index 64d1ab3fe8..1a13fb5e2b 100644
> --- a/refs/files-backend.c
> +++ b/refs/files-backend.c
> @@ -1180,6 +1180,18 @@ static void files_reflog_path(struct files_ref_store *refs,
> strbuf_git_path(sb, "logs/%s", refname);
> }
>
> +static void files_refname_path(struct files_ref_store *refs,
> + struct strbuf *sb,
> + const char *refname)
> +{
> + if (refs->submodule) {
> + strbuf_git_path_submodule(sb, refs->submodule, "%s", refname);
> + return;
> + }
> +
> + strbuf_git_path(sb, "%s", refname);
> +}
> +
> /*
> * Get the packed_ref_cache for the specified files_ref_store,
> * creating it if necessary.
> @@ -1249,19 +1261,10 @@ static void read_loose_refs(const char *dirname, struct ref_dir *dir)
> struct strbuf refname;
> struct strbuf path = STRBUF_INIT;
> size_t path_baselen;
> - int err = 0;
>
> - if (refs->submodule)
> - err = strbuf_git_path_submodule(&path, refs->submodule, "%s", dirname);
> - else
> - strbuf_git_path(&path, "%s", dirname);
> + files_refname_path(refs, &path, dirname);
> path_baselen = path.len;
>
> - if (err) {
> - strbuf_release(&path);
> - return;
> - }
> -
> d = opendir(path.buf);
> if (!d) {
> strbuf_release(&path);
The old code in the hunk above went to the trouble of handling errors
from `strbuf_git_path_submodule()`, which I think can happen if the
submodule path doesn't actually point at a directory that looks like a
Git repository. Your new code doesn't handle such an error.
It seems clear that `read_loose_refs()` is to late a place to be dealing
with such errors, and indeed it seems like the check
`is_nonbare_repository_dir()` in `get_ref_store()` should make such
errors impossible, so I think your change is OK. If you agree, it might
be appropriate to mention that reasoning in the commit message.
> [...]
Michael