Re: [PATCH 1/2] refs.c: add resolve_ref_submodule()
From: Michael Haggerty <hidden>
Date: 2017-02-09 05:26:12
On 02/08/2017 12:31 PM, Nguyễn Thái Ngọc Duy wrote:
quoted hunk ↗ jump to hunk
This is basically the extended version of resolve_gitlink_ref() where we have access to more info from the underlying resolve_ref_recursively() call. Signed-off-by: Nguyễn Thái Ngọc Duy <redacted> --- refs.c | 20 ++++++++++++++------ refs.h | 3 +++ 2 files changed, 17 insertions(+), 6 deletions(-)diff --git a/refs.c b/refs.c index cd36b64ed9..02e35d83f3 100644 --- a/refs.c +++ b/refs.c@@ -1325,18 +1325,18 @@ const char *resolve_ref_unsafe(const char *refname, int resolve_flags, resolve_flags, sha1, flags); } -int resolve_gitlink_ref(const char *submodule, const char *refname, - unsigned char *sha1) +const char *resolve_ref_submodule(const char *submodule, const char *refname, + int resolve_flags, unsigned char *sha1, + int *flags) { size_t len = strlen(submodule); struct ref_store *refs; - int flags; while (len && submodule[len - 1] == '/') len--; if (!len) - return -1; + return NULL; if (submodule[len]) { /* We need to strip off one or more trailing slashes */@@ -1349,9 +1349,17 @@ int resolve_gitlink_ref(const char *submodule, const char *refname, } if (!refs) - return -1; + return NULL; + + return resolve_ref_recursively(refs, refname, resolve_flags, sha1, flags); +} + +int resolve_gitlink_ref(const char *submodule, const char *refname, + unsigned char *sha1) +{ + int flags; - if (!resolve_ref_recursively(refs, refname, 0, sha1, &flags) || + if (!resolve_ref_submodule(submodule, refname, 0, sha1, &flags) || is_null_sha1(sha1)) return -1; return 0;diff --git a/refs.h b/refs.h index 9fbff90e79..74542468d8 100644 --- a/refs.h +++ b/refs.h@@ -88,6 +88,9 @@ int peel_ref(const char *refname, unsigned char *sha1); */ int resolve_gitlink_ref(const char *submodule, const char *refname, unsigned char *sha1); +const char *resolve_ref_submodule(const char *submodule, const char *refname, + int resolve_flags, unsigned char *sha1, + int *flags);
This function is the analog of resolve_ref_unsafe(); i.e., it returns a pointer to either a static buffer or a pointer into the refname argument. Therefore, I think it should have "unsafe" in its name. And/or maybe there should be a safe version of the function analogous to resolve_refdup(). Moreover, this function has inherited the code for stripping trailing slashes from the submodule name. I have the feeling that this is a wart, not a feature, and that it would be sad to see it spread. How about moving the slash-stripping code to resolve_gitlink_ref() and making resolve_ref_submodule() assume that its submodule name is already clean? It would be nice to have a docstring here. I also have some higher-level concerns about the approach of this patch series, which I'll write about in a comment to patch 2/2. Michael