Re: [PATCH v5 07/24] files-backend: add and use files_refname_path()

On Wed, Mar 1, 2017 at 12:41 AM, Michael Haggerty [off-list ref] wrote:

On 02/22/2017 03:04 PM, Nguyễn Thái Ngọc Duy wrote:

quoted

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 sumodule-ready. Not yet.

s/sumodule/submodule/

quoted

Signed-off-by: Nguyễn Thái Ngọc Duy <redacted>
---
 refs/files-backend.c | 45 +++++++++++++++++++++++++--------------------
 1 file changed, 25 insertions(+), 20 deletions(-)

diff --git a/refs/files-backend.c b/refs/files-backend.c
index 7b4ea4c56..72f4e1746 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);
+}

Maybe it's just me, but I find it odd to exit early here when the first
exit isn't due to an error. For me, structuring this like `if ()
call1(); else call2();` would make it clearer that the two code paths
are equally-valid alternatives, and either one or the other will be
executed.

Its original version probably looked better. This is another case of
future patches influencing back the past ones: I structure the patch
so that in future we mostly add lines, or delete whole (in this case,
I believe), not modify a lot of lines. I think the readability does
not degrade too much though, so it's probably ok.
-- 
Duy