[v3,5/8] submodule: return target of submodule symref

Commit Message

Glen Choo Oct. 28, 2022, 8:14 p.m. UTC

From: Glen Choo <[e-mail]>

resolve_gitlink_ref() can tell us which oid the submodule ref is
pointing to, but in a future commit, we would also like to know the
symbolic ref target if we are checking a symbolic ref. Teach
resolve_gitlink_ref() to "return" the symbolic ref's target via an "out"
parameter.

This changes resolve_gitlink_ref()'s signature so that new callers
trying to use the old signature will be stopped by the compiler. If we
returned the target instead (just like refs_resolve_ref_unsafe()), we
would be more consistent with refs_resolve_ref_unsafe(), but callers
expecting the old signature will get the opposite return value from what
they expect (since exit code 0 means success, but NULL pointer means
failure). We should do this refactor once we think that nobody will try
to use the old signature.

Signed-off-by: Glen Choo <[e-mail]>
---
 builtin/submodule--helper.c |  8 +++++---
 builtin/update-index.c      |  4 ++--
 combine-diff.c              |  3 ++-
 diff-lib.c                  |  2 +-
 dir.c                       |  2 +-
 object-file.c               |  2 +-
 read-cache.c                |  4 ++--
 refs.c                      | 10 ++++++----
 refs.h                      |  5 ++++-
 unpack-trees.c              |  3 ++-
 10 files changed, 26 insertions(+), 17 deletions(-)

Comments

Junio C Hamano Oct. 28, 2022, 9:49 p.m. UTC | #1

"Glen Choo via GitGitGadget" <[e-mail]> writes:

>  int resolve_gitlink_ref(const char *submodule, const char *refname,
> -			struct object_id *oid)
> +			struct object_id *oid, const char **target_out)
>  {
>  	struct ref_store *refs;
>  	int flags;
> +	const char *target;
>  
>  	refs = get_submodule_ref_store(submodule);
>  
>  	if (!refs)
>  		return -1;
> -
> -	if (!refs_resolve_ref_unsafe(refs, refname, 0, oid, &flags) ||
> -	    is_null_oid(oid))
> +	target = refs_resolve_ref_unsafe(refs, refname, 0, oid, &flags);
> +	if (!target || is_null_oid(oid))
>  		return -1;
> +	if (target_out)
> +		*target_out = target;
>  	return 0;
>  }

Please remind me why we call this underlying helper _unsafe()?

Isn't it because we return a temporary buffer  (static strbuf),
whose contents is not permanent?

I am wondering if we should force the callers who care enough to
pass the optional **target_out to xstrdup() the result.

Glen Choo Oct. 28, 2022, 11:11 p.m. UTC | #2

Junio C Hamano <[e-mail]> writes:

> "Glen Choo via GitGitGadget" <[e-mail]> writes:
>
>>  int resolve_gitlink_ref(const char *submodule, const char *refname,
>> -			struct object_id *oid)
>> +			struct object_id *oid, const char **target_out)
>>  {
>>  	struct ref_store *refs;
>>  	int flags;
>> +	const char *target;
>>  
>>  	refs = get_submodule_ref_store(submodule);
>>  
>>  	if (!refs)
>>  		return -1;
>> -
>> -	if (!refs_resolve_ref_unsafe(refs, refname, 0, oid, &flags) ||
>> -	    is_null_oid(oid))
>> +	target = refs_resolve_ref_unsafe(refs, refname, 0, oid, &flags);
>> +	if (!target || is_null_oid(oid))
>>  		return -1;
>> +	if (target_out)
>> +		*target_out = target;
>>  	return 0;
>>  }
>
> Please remind me why we call this underlying helper _unsafe()?
>
> Isn't it because we return a temporary buffer  (static strbuf),
> whose contents is not permanent?

8cad4744ee (Rename resolve_ref() to resolve_ref_unsafe(), 2011-12-12)
seems to suggest so.

(For some reason, I thought it was *_unsafe() because it would die(),
but it obviously doesn't.)

> I am wondering if we should force the callers who care enough to
> pass the optional **target_out to xstrdup() the result.

Yes, makes sense. We even have the *_refdup() helper for that.

Patch

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index c974206cad4..9ca138374b7 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -2533,7 +2533,8 @@  static int update_submodule(struct update_data *update_data)
 
 	if (update_data->just_cloned)
 		oidcpy(&update_data->suboid, null_oid());
-	else if (resolve_gitlink_ref(update_data->sm_path, "HEAD", &update_data->suboid))
+	else if (resolve_gitlink_ref(update_data->sm_path, "HEAD",
+				     &update_data->suboid, NULL))
 		return die_message(_("Unable to find current revision in submodule path '%s'"),
 				   update_data->displaypath);
 
@@ -2560,7 +2561,8 @@  static int update_submodule(struct update_data *update_data)
 						   update_data->sm_path);
 		}
 
-		if (resolve_gitlink_ref(update_data->sm_path, remote_ref, &update_data->oid))
+		if (resolve_gitlink_ref(update_data->sm_path, remote_ref,
+					&update_data->oid, NULL))
 			return die_message(_("Unable to find %s revision in submodule path '%s'"),
 					   remote_ref, update_data->sm_path);
 
@@ -3305,7 +3307,7 @@  static void die_on_repo_without_commits(const char *path)
 	strbuf_addstr(&sb, path);
 	if (is_nonbare_repository_dir(&sb)) {
 		struct object_id oid;
-		if (resolve_gitlink_ref(path, "HEAD", &oid) < 0)
+		if (resolve_gitlink_ref(path, "HEAD", &oid, NULL) < 0)
 			die(_("'%s' does not have a commit checked out"), path);
 	}
 	strbuf_release(&sb);
diff --git a/builtin/update-index.c b/builtin/update-index.c
index b62249905f1..19a21a4586c 100644
--- a/builtin/update-index.c
+++ b/builtin/update-index.c
@@ -339,7 +339,7 @@  static int process_directory(const char *path, int len, struct stat *st)
 		if (S_ISGITLINK(ce->ce_mode)) {
 
 			/* Do nothing to the index if there is no HEAD! */
-			if (resolve_gitlink_ref(path, "HEAD", &oid) < 0)
+			if (resolve_gitlink_ref(path, "HEAD", &oid, NULL) < 0)
 				return 0;
 
 			return add_one_path(ce, path, len, st);
@@ -365,7 +365,7 @@  static int process_directory(const char *path, int len, struct stat *st)
 	}
 
 	/* No match - should we add it as a gitlink? */
-	if (!resolve_gitlink_ref(path, "HEAD", &oid))
+	if (!resolve_gitlink_ref(path, "HEAD", &oid, NULL))
 		return add_one_path(NULL, path, len, st);
 
 	/* Error out. */
diff --git a/combine-diff.c b/combine-diff.c
index b0ece954808..88efcaeefa7 100644
--- a/combine-diff.c
+++ b/combine-diff.c
@@ -1060,7 +1060,8 @@  static void show_patch_diff(struct combine_diff_path *elem, int num_parent,
 			elem->mode = canon_mode(st.st_mode);
 		} else if (S_ISDIR(st.st_mode)) {
 			struct object_id oid;
-			if (resolve_gitlink_ref(elem->path, "HEAD", &oid) < 0)
+			if (resolve_gitlink_ref(elem->path, "HEAD", &oid,
+						NULL) < 0)
 				result = grab_blob(opt->repo, &elem->oid,
 						   elem->mode, &result_size,
 						   NULL, NULL);
diff --git a/diff-lib.c b/diff-lib.c
index 2edea41a234..ac94b6234ca 100644
--- a/diff-lib.c
+++ b/diff-lib.c
@@ -53,7 +53,7 @@  static int check_removed(const struct index_state *istate, const struct cache_en
 		 * a directory --- the blob was removed!
 		 */
 		if (!S_ISGITLINK(ce->ce_mode) &&
-		    resolve_gitlink_ref(ce->name, "HEAD", &sub))
+		    resolve_gitlink_ref(ce->name, "HEAD", &sub, NULL))
 			return 1;
 	}
 	return 0;
diff --git a/dir.c b/dir.c
index d604d1bab98..81d232424c1 100644
--- a/dir.c
+++ b/dir.c
@@ -3251,7 +3251,7 @@  static int remove_dir_recurse(struct strbuf *path, int flag, int *kept_up)
 	struct object_id submodule_head;
 
 	if ((flag & REMOVE_DIR_KEEP_NESTED_GIT) &&
-	    !resolve_gitlink_ref(path->buf, "HEAD", &submodule_head)) {
+	    !resolve_gitlink_ref(path->buf, "HEAD", &submodule_head, NULL)) {
 		/* Do not descend and nuke a nested git work tree. */
 		if (kept_up)
 			*kept_up = 1;
diff --git a/object-file.c b/object-file.c
index 5e309602346..a3c6580c16e 100644
--- a/object-file.c
+++ b/object-file.c
@@ -2522,7 +2522,7 @@  int index_path(struct index_state *istate, struct object_id *oid,
 		strbuf_release(&sb);
 		break;
 	case S_IFDIR:
-		return resolve_gitlink_ref(path, "HEAD", oid);
+		return resolve_gitlink_ref(path, "HEAD", oid, NULL);
 	default:
 		return error(_("%s: unsupported file type"), path);
 	}
diff --git a/read-cache.c b/read-cache.c
index 32024029274..4c1bf33ef48 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -285,7 +285,7 @@  static int ce_compare_gitlink(const struct cache_entry *ce)
 	 *
 	 * If so, we consider it always to match.
 	 */
-	if (resolve_gitlink_ref(ce->name, "HEAD", &oid) < 0)
+	if (resolve_gitlink_ref(ce->name, "HEAD", &oid, NULL) < 0)
 		return 0;
 	return !oideq(&oid, &ce->oid);
 }
@@ -781,7 +781,7 @@  int add_to_index(struct index_state *istate, const char *path, struct stat *st,
 
 	namelen = strlen(path);
 	if (S_ISDIR(st_mode)) {
-		if (resolve_gitlink_ref(path, "HEAD", &oid) < 0)
+		if (resolve_gitlink_ref(path, "HEAD", &oid, NULL) < 0)
 			return error(_("'%s' does not have a commit checked out"), path);
 		while (namelen && path[namelen-1] == '/')
 			namelen--;
diff --git a/refs.c b/refs.c
index 1491ae937eb..a32a25ccb69 100644
--- a/refs.c
+++ b/refs.c
@@ -1904,19 +1904,21 @@  const char *resolve_ref_unsafe(const char *refname, int resolve_flags,
 }
 
 int resolve_gitlink_ref(const char *submodule, const char *refname,
-			struct object_id *oid)
+			struct object_id *oid, const char **target_out)
 {
 	struct ref_store *refs;
 	int flags;
+	const char *target;
 
 	refs = get_submodule_ref_store(submodule);
 
 	if (!refs)
 		return -1;
-
-	if (!refs_resolve_ref_unsafe(refs, refname, 0, oid, &flags) ||
-	    is_null_oid(oid))
+	target = refs_resolve_ref_unsafe(refs, refname, 0, oid, &flags);
+	if (!target || is_null_oid(oid))
 		return -1;
+	if (target_out)
+		*target_out = target;
 	return 0;
 }
 
diff --git a/refs.h b/refs.h
index 8958717a17d..d5c32fac000 100644
--- a/refs.h
+++ b/refs.h
@@ -137,9 +137,12 @@  int peel_iterated_oid(const struct object_id *base, struct object_id *peeled);
  * submodule (which must be non-NULL). If the resolution is
  * successful, return 0 and set oid to the name of the object;
  * otherwise, return a non-zero value.
+ *
+ * FIXME: Return "target" just like refs_resolve_ref_unsafe(). This will be
+ * safe to do once we merge resolve_gitlink_ref() into master.
  */
 int resolve_gitlink_ref(const char *submodule, const char *refname,
-			struct object_id *oid);
+			struct object_id *oid, const char **target);
 
 /*
  * Return true iff abbrev_name is a possible abbreviation for
diff --git a/unpack-trees.c b/unpack-trees.c
index bae812156c4..db12bfcaffd 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -2288,7 +2288,8 @@  static int verify_clean_subdirectory(const struct cache_entry *ce,
 
 	if (S_ISGITLINK(ce->ce_mode)) {
 		struct object_id oid;
-		int sub_head = resolve_gitlink_ref(ce->name, "HEAD", &oid);
+		int sub_head =
+			resolve_gitlink_ref(ce->name, "HEAD", &oid, NULL);
 		/*
 		 * If we are not going to update the submodule, then
 		 * we don't care.