Re: [PATCH 11/11] read_cache: convert most calls to repo_read_index_or_die

From: Duy Nguyen <pclouds@gmail.com>
Date: May 19, 2018, 06:54 UTC
Message-ID: <20180519065411.GB14755@duynguyen.home>
In-Reply-To: <20180516222118.233868-12-sbeller@google.com>

On Wed, May 16, 2018 at 03:21:18PM -0700, Stefan Beller wrote:

This commit may need some more explanation. It's not always safe to replace "read_cache();" with "repo_read_index_or_die();" and you do sometimes avoid that variant.

While I agree _or_die() is the right thing to do most of the time. You need to consider that we (or some of us) have tried to avoid die() in some library code. So..

Show 7 quoted lines

> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
>  blame.c               | 5 +++--
>  builtin/am.c          | 3 ++-
>  builtin/diff.c        | 3 ++-
>  builtin/fsck.c        | 3 ++-
>  builtin/merge-index.c | 3 ++-

These should be good.

>  check-racy.c          | 2 +-

Meh.. this one is test code.

Show 18 quoted lines

> diff --git a/diff.c b/diff.c
> index 1289df4b1f9..383f52fa118 100644
> --- a/diff.c
> +++ b/diff.c
> @@ -22,6 +22,7 @@
>  #include "argv-array.h"
>  #include "graph.h"
>  #include "packfile.h"
> +#include "repository.h"
>  
>  #ifdef NO_FAST_WORKING_DIRECTORY
>  #define FAST_WORKING_DIRECTORY 0
> @@ -4210,13 +4211,13 @@ void diff_setup_done(struct diff_options *options)
>  		options->rename_limit = diff_rename_limit_default;
>  	if (options->setup & DIFF_SETUP_USE_CACHE) {
>  		if (!active_cache)
> -			/* read-cache does not die even when it fails
> +			/* repo_read_indexe does not die even when it fails

s/indexe/index/

Show 7 quoted lines

>  			 * so it is safe for us to do this here.  Also
>  			 * it does not smudge active_cache or active_nr
>  			 * when it fails, so we do not have to worry about
>  			 * cleaning it up ourselves either.
>  			 */
> -			read_cache();
> +			repo_read_index(the_repository);

Should we write a warning message or something?

Show 18 quoted lines

> diff --git a/revision.c b/revision.c
> index 1cff11833e7..8ad9824143d 100644
> --- a/revision.c
> +++ b/revision.c
> @@ -23,6 +23,7 @@
>  #include "packfile.h"
>  #include "worktree.h"
>  #include "argv-array.h"
> +#include "repository.h"
>  
>  volatile show_early_output_fn_t show_early_output;
>  
> @@ -1344,7 +1345,7 @@ void add_index_objects_to_pending(struct rev_info *revs, unsigned int flags)
>  {
>  	struct worktree **worktrees, **p;
>  
> -	read_cache();
> +	repo_read_index_or_die(the_repository);

I think here we should be able to tolerate a bad index file. If you have bad index file, we ignore object references from it and continue on the rev walk without it. So repo_read_index() may be better, optionally with a warning.

Show 10 quoted lines

> diff --git a/sha1-name.c b/sha1-name.c
> index 5b93bf8da36..83d5f945cf1 100644
> --- a/sha1-name.c
> +++ b/sha1-name.c
> @@ -1639,7 +1639,7 @@ static int get_oid_with_context_1(const char *name,
>  			oc->path = xstrdup(cp);
>  
>  		if (!active_cache)
> -			read_cache();
> +			repo_read_index_or_die(the_repository);

This should return an error code instead. I notice there's a "only_to_die" flag somewhere in here. Something to think about.

BTW you probably want to move away from "active_cache" too. It makes sense to read "if active cache is null then read the cache". But with the move to the repository it now seems disconnected.

This looks clearer but a bit verbose

		if (!the_repository->index_file.cache)
			repo_read_index_or_die(the_repository);

This might be the best

		if (!repo_index_loaded(the_repository))
			repo_read_index_or_die(the_repository);

but obviously more work for you, so your choice :)

Show 6 quoted lines

>  		pos = cache_name_pos(cp, namelen);
>  		if (pos < 0)
>  			pos = -pos - 1;
> -- 
> 2.17.0.582.gccdcbd54c44.dirty
>