Re: [PATCH 1/3] sparse-checkout: remove use of the_repository

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

"Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Derrick Stolee <stolee@gmail.com>
>
> The logic for the 'git sparse-checkout' builtin uses the_repository all
> over the place, despite some use of a repository struct in different
> method parameters. Complete this removal of the_repository by using
> 'repo' when possible.
>
> In one place, there was already a local variable 'r' that was set to
> the_repository, so move that to a method parameter.
>
> We cannot remove the USE_THE_REPOSITORY_VARIABLE declaration as we are
> still using global constants for the state of the sparse-checkout.
>
> Signed-off-by: Derrick Stolee <stolee@gmail.com>
> ---
>  builtin/sparse-checkout.c | 119 ++++++++++++++++++++------------------
>  1 file changed, 63 insertions(+), 56 deletions(-)

OK.  The damage is not too bad for a partial update ;-).

As the file-scope static functions in builtin/sparse-checkout.c are
not going to be called by anybody else, it does not really matter if
they internally pass an extra parameter around or use the_repository
since the end result is the same.  But doing this may hopefully help
those that may want to move some of these functions to a more
library-ish part of the system outside builtin/ hierarchy.

> diff --git a/builtin/sparse-checkout.c b/builtin/sparse-checkout.c
> index 1bf01591b275..8b70d0c6a441 100644
> --- a/builtin/sparse-checkout.c
> +++ b/builtin/sparse-checkout.c
> @@ -204,12 +204,12 @@ static void clean_tracked_sparse_directories(struct repository *r)
>  		ensure_full_index(r->index);
>  }
>  
> -static int update_working_directory(struct pattern_list *pl)
> +static int update_working_directory(struct repository *r,
> +				    struct pattern_list *pl)
>  {
>  	enum update_sparsity_result result;
>  	struct unpack_trees_options o;
>  	struct lock_file lock_file = LOCK_INIT;
> -	struct repository *r = the_repository;
>  	struct pattern_list *old_pl;

As this already used short-and-sweet 'r', we just follow suit to
minimize the damage, which is fine.

> @@ -327,7 +327,8 @@ static void write_cone_to_file(FILE *fp, struct pattern_list *pl)
>  	string_list_clear(&sl, 0);
>  }
>  
> -static int write_patterns_and_update(struct pattern_list *pl)
> +static int write_patterns_and_update(struct repository *repo,
> +				     struct pattern_list *pl)
>  {
>  	char *sparse_filename;
>  	FILE *fp;
> @@ -336,15 +337,15 @@ static int write_patterns_and_update(struct pattern_list *pl)
>  
>  	sparse_filename = get_sparse_checkout_filename();
>  
> -	if (safe_create_leading_directories(the_repository, sparse_filename))
> +	if (safe_create_leading_directories(repo, sparse_filename))
>  		die(_("failed to create directory for sparse-checkout file"));
>  
>  	hold_lock_file_for_update(&lk, sparse_filename, LOCK_DIE_ON_ERROR);
>  
> -	result = update_working_directory(pl);
> +	result = update_working_directory(repo, pl);
>  	if (result) {
>  		rollback_lock_file(&lk);
> -		update_working_directory(NULL);
> +		update_working_directory(repo, NULL);
>  		goto out;
>  	}

But this introduces a new parameter.  Both of two instances of
repository struct used in the existing code in this function, other
than references to struct repository *UNUSED, use "r", and with this
patch, the name "repo" becomes more prevanent.

We would probably want to rename "r" to "repo" for consistency in
clean_tracked_sparse_repositories() and update_working_directory(),
but that is better done later after the dust settles and the code
around here becomes quiescent again, not as part of this topic as an
extra churn.

Looking good.  Thanks.  Will queue.