Re: [RFC PATCH 6/7] autostash.c: undefine USE_THE_INDEX_COMPATIBILITY_MACROS

From: Phillip Wood <phillip.wood123@gmail.com>
Date: Oct 18, 2019, 09:41 UTC
Message-ID: <836db3ce-2c2a-1d4f-a131-6005fc961f45@gmail.com>
In-Reply-To: <73607c2e00a87b05da0e374734eb5c4d7d377d84.1571246693.git.liu.denton@gmail.com>

Hi Denton

On 16/10/2019 18:26, Denton Liu wrote:

Show 26 quoted lines

> Since autostash.c was recently introduced, we should avoid
> USE_THE_INDEX_COMPATIBILITY_MACROS since we are trying to move away from
> this in the rest of the codebase. Rewrite the autostash code to not need
> it and remove its definition.
> 
> Signed-off-by: Denton Liu <liu.denton@gmail.com>
> ---
>   autostash.c | 8 +++-----
>   1 file changed, 3 insertions(+), 5 deletions(-)
> 
> diff --git a/autostash.c b/autostash.c
> index 722cf78b12..0a1f00d2e5 100644
> --- a/autostash.c
> +++ b/autostash.c
> @@ -1,5 +1,3 @@
> -#define USE_THE_INDEX_COMPATIBILITY_MACROS
> -
>   #include "git-compat-util.h"
>   #include "autostash.h"
>   #include "cache-tree.h"
> @@ -46,7 +44,7 @@ int reset_head(struct object_id *oid, const char *action,
>   	if (switch_to_branch && !starts_with(switch_to_branch, "refs/"))
>   		BUG("Not a fully qualified branch: '%s'", switch_to_branch);
>   
> -	if (!refs_only && hold_locked_index(&lock, LOCK_REPORT_ON_ERROR) < 0) {
> +	if (!refs_only && repo_hold_locked_index(the_repository, &lock, LOCK_REPORT_ON_ERROR) < 0) {

As I understand it the reason for moving away from hold_locked_index() is that it relies on a global variable. While replacing it with an explicit reference the the_repository removes the implicit dependency on global state, it does not move us away from depending on a global variable. I think it would be nicer to change these functions to take a `struct repository*` before moving them.

Best Wishes

Phillip

Show 15 quoted lines

>   		ret = -1;
>   		goto leave_reset_head;
>   	}
> @@ -157,8 +155,8 @@ void perform_autostash(const char *path)
>   	struct lock_file lock_file = LOCK_INIT;
>   	int fd;
>   
> -	fd = hold_locked_index(&lock_file, 0);
> -	refresh_cache(REFRESH_QUIET);
> +	fd = repo_hold_locked_index(the_repository, &lock_file, 0);
> +	refresh_index(the_repository->index, REFRESH_QUIET, NULL, NULL, NULL);
>   	if (0 <= fd)
>   		repo_update_index_if_able(the_repository, &lock_file);
>   	rollback_lock_file(&lock_file);
>