Re: [PATCH v4 1/7] submodule--helper: get remote names from any repository
- From
Glen Choo <chooglen@google.com>
- Date
- Jan 27, 2022, 18:45 UTC
- Message-ID
- <kl6ltudpyq4b.fsf@chooglen-macbookpro.roam.corp.google.com>
- In-Reply-To
- <patch-v4-1.7-1a0b1323cd7-20220127T143552Z-avarab@gmail.com>
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
This wasn't introduced by you (it was introduced in v1 [1]), but I think it's worth pointing out.
Show 5 quoted lines
> Let's instead introduce a function called `repo_get_default_remote()` > which takes any repository object and retrieves the remote accordingly. > > `get_default_remote()` is then defined as a call to > `repo_get_default_remote()` with 'the_repository' passed to it.
We say this, suggesting that repo_get_default_remote()'s signature is just get_default_remote()'s plus a "struct repository *" (like most repo_*). But..
Show 49 quoted lines
> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> index c5d3fc3817f..965260edb22 100644
> --- a/builtin/submodule--helper.c
> +++ b/builtin/submodule--helper.c
> @@ -29,11 +29,10 @@
> typedef void (*each_submodule_fn)(const struct cache_entry *list_item,
> void *cb_data);
>
> -static char *get_default_remote(void)
> +static char *repo_get_default_remote(struct repository *repo, const char *refname)
> {
> char *dest = NULL, *ret;
> struct strbuf sb = STRBUF_INIT;
> - const char *refname = resolve_ref_unsafe("HEAD", 0, NULL, NULL);
>
> if (!refname)
> die(_("No such ref: %s"), "HEAD");
> @@ -46,7 +45,7 @@ static char *get_default_remote(void)
> die(_("Expecting a full ref name, got %s"), refname);
>
> strbuf_addf(&sb, "branch.%s.remote", refname);
> - if (git_config_get_string(sb.buf, &dest))
> + if (repo_config_get_string(repo, sb.buf, &dest))
> ret = xstrdup("origin");
> else
> ret = dest;
> @@ -55,6 +54,25 @@ static char *get_default_remote(void)
> return ret;
> }
>
> +static char *get_default_remote_submodule(const char *module_path)
> +{
> + const char *refname;
> + struct repository subrepo;
> + int ignore_errno;
> +
> + refname = refs_resolve_ref_unsafe(get_submodule_ref_store(module_path),
> + "HEAD", 0, NULL, NULL,
> + &ignore_errno);
> + repo_submodule_init(&subrepo, the_repository, module_path, null_oid());
> + return repo_get_default_remote(&subrepo, refname);
> +}
> +
> +static char *get_default_remote(void)
> +{
> + const char *refname = resolve_ref_unsafe("HEAD", 0, NULL, NULL);
> + return repo_get_default_remote(the_repository, refname);
> +}
> +repo_get_default_remote() actually take yet another argument - refname.
It looks to me that repo_get_default_remote() shouldn't take the refname argument at all and that we should be using refs_resolve_ref_unsafe() instead, like:
+static char *repo_get_default_remote(struct repository *repo)
{
char *dest = NULL, *ret;
struct strbuf sb = STRBUF_INIT;
- const char *refname = resolve_ref_unsafe("HEAD", 0, NULL, NULL);
+ const char *refname = refs_resolve_ref_unsafe(
+ get_main_ref_store(repo), "HEAD", 0, NULL, NULL /*, errno? */);this makes the rest of the code a lot cleaner..
+static char *get_default_remote_submodule(const char *module_path)
+{
+ struct repository subrepo;
+
+ repo_submodule_init(&subrepo, the_repository, module_path, null_oid());
+ return repo_get_default_remote(&subrepo);
+}
+
+static char *get_default_remote(void)
+{
+ return repo_get_default_remote(the_repository);
+}And because it's quite idiomatic to initialize the subrepo struct in order to can call repo_* functions, we could even drop get_default_remote_submodule() altogether.
As for why this wasn't the original approach, the only reason I can think of is that we didn't realize get_main_ref_store(subrepo) was an option.
[1] https://lore.kernel.org/git/20210907115932.36068-3-raykar.ath@gmail.com/