Re: [Outreachy PATCH v2] checkout: pass the repository to unique_tracking_name()

[Junio C Hamano <gitster@pobox.com>]

Okhuomon Ajayi <okhuomonajayi54@gmail.com> writes:

> unique_tracking_name() uses the_repository to read the
> "checkout.defaultRemote" configuration and look up the object
> ID of a remote-tracking branch.
>
> Pass the repository to unique_tracking_name() and through
> tracking_name_data so check_tracking_name() can use it for
> the object lookup. Update the three callers to pass
> the_repository.
>
> This removes the need for USE_THE_REPOSITORY_VARIABLE in
> checkout.c. for_each_remote() still uses the global repository;
> leave that for a separate change.

The mention of for_each_remote() feels weirdly abrupt.  Many other
things still rely on the the_repository global variable, but
there must be a reason why for_each_remote() is singled out in the
context of this patch.  What is it?

Replace the last sentence with something like:

    The callers still cannot call unique_tracking_name() with an
    arbitrary repository parameter other than the_repository, until
    the call in unique_tracking_name() to for_each_remote(), which
    only works with the_repository, is updated to take an arbitrary
    repository instance.

perhaps?

And explained that way, it becomes dubious if we chose a good place
to stop.  The updated unique_tracking_name() gives an illusion that
callers now can pass an arbitrary repository to it and expect it to
work correctly with the given repository, but it does not.

So, I dunno.


> Signed-off-by: Okhuomon Ajayi <okhuomonajayi54@gmail.com>
> ---
> Changes since v1:
> - Rewrote the commit message to name the function and say what was
>   there, what changed and why, as suggested by Usman. No code changes.
>
>  builtin/checkout.c |  3 ++-
>  builtin/worktree.c |  6 ++++--
>  checkout.c         | 11 ++++++-----
>  checkout.h         |  4 +++-
>  4 files changed, 15 insertions(+), 9 deletions(-)
>
> diff --git a/builtin/checkout.c b/builtin/checkout.c
> index 2d575a4f57..bad49368b9 100644
> --- a/builtin/checkout.c
> +++ b/builtin/checkout.c
> @@ -1389,7 +1389,8 @@ static char *parse_remote_branch(const char *arg,
>  	int num_matches = 0;
>  	struct string_list matched_remote_names = STRING_LIST_INIT_DUP;
>  
> -	char *remote = unique_tracking_name(arg, rev, &num_matches,
> +	char *remote = unique_tracking_name(the_repository, arg, rev,
> +					    &num_matches,
>  					    &matched_remote_names);
>  
>  	if (remote && could_be_checkout_paths) {
> diff --git a/builtin/worktree.c b/builtin/worktree.c
> index 77ecd0f71f..203456bd23 100644
> --- a/builtin/worktree.c
> +++ b/builtin/worktree.c
> @@ -802,7 +802,8 @@ static char *dwim_branch(const struct add_opts *opts, const char *path, char **n
>  		int num_matches = 0;
>  		struct string_list matched_remote_names = STRING_LIST_INIT_DUP;
>  
> -		remote = unique_tracking_name(*new_branch, &oid, &num_matches,
> +		remote = unique_tracking_name(the_repository, *new_branch,
> +					      &oid, &num_matches,
>  					      &matched_remote_names);
>  		if (!remote && num_matches > 1) {
>  			if (!opts->quiet &&
> @@ -938,7 +939,8 @@ static int add(int ac, const char **av, const char *prefix,
>  			int num_matches = 0;
>  			struct string_list matched_remote_names = STRING_LIST_INIT_DUP;
>  
> -			remote = unique_tracking_name(branch, &oid, &num_matches,
> +			remote = unique_tracking_name(the_repository, branch,
> +						      &oid, &num_matches,
>  						      &matched_remote_names);
>  			if (remote) {
>  				new_branch = branch;
> diff --git a/checkout.c b/checkout.c
> index a0d0229435..f0fb40d48e 100644
> --- a/checkout.c
> +++ b/checkout.c
> @@ -1,5 +1,3 @@
> -#define USE_THE_REPOSITORY_VARIABLE
> -
>  #include "git-compat-util.h"
>  #include "object-name.h"
>  #include "remote.h"
> @@ -11,6 +9,7 @@
>  #include "string-list.h"
>  
>  struct tracking_name_data {
> +	struct repository *repo;
>  	/* const */ char *src_ref;
>  	char *dst_ref;
>  	struct object_id *dst_oid;
> @@ -30,7 +29,7 @@ static int check_tracking_name(struct remote *remote, void *cb_data)
>  	memset(&query, 0, sizeof(struct refspec_item));
>  	query.src = cb->src_ref;
>  	if (remote_find_tracking(remote, &query) ||
> -	    repo_get_oid(the_repository, query.dst, cb->dst_oid)) {
> +	    repo_get_oid(cb->repo, query.dst, cb->dst_oid)) {
>  		free(query.dst);
>  		return 0;
>  	}
> @@ -51,17 +50,19 @@ static int check_tracking_name(struct remote *remote, void *cb_data)
>  	return 0;
>  }
>  
> -char *unique_tracking_name(const char *name, struct object_id *oid,
> +char *unique_tracking_name(struct repository *repo,
> +			   const char *name, struct object_id *oid,
>  			   int *dwim_remotes_matched,
>  			   struct string_list *dwim_remote_names)
>  {
>  	struct tracking_name_data cb_data = TRACKING_NAME_DATA_INIT;
>  	const char *default_remote = NULL;
>  
> -	if (!repo_config_get_string_tmp(the_repository,
> +	if (!repo_config_get_string_tmp(repo,
>  					"checkout.defaultremote",
>  					&default_remote))
>  		cb_data.default_remote = default_remote;
> +	cb_data.repo = repo;
>  	cb_data.src_ref = xstrfmt("refs/heads/%s", name);
>  	cb_data.dst_oid = oid;
>  	cb_data.remote_names = dwim_remote_names;
> diff --git a/checkout.h b/checkout.h
> index 0b185a0fc9..25fe7dfb7c 100644
> --- a/checkout.h
> +++ b/checkout.h
> @@ -3,6 +3,7 @@
>  
>  #include "hash.h"
>  
> +struct repository;
>  struct string_list;
>  
>  /*
> @@ -10,7 +11,8 @@ struct string_list;
>   * tracking branch.  Return the name of the remote if such a branch
>   * exists, NULL otherwise.
>   */
> -char *unique_tracking_name(const char *name,
> +char *unique_tracking_name(struct repository *repo,
> +			   const char *name,
>  			   struct object_id *oid,
>  			   int *dwim_remotes_matched,
>  			   struct string_list *dwim_remote_names);
>
> base-commit: c51408889c3b372814bc20b9431c3b74d0d268eb