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..
> Signed-off-by: Stefan Beller <sbeller@xxxxxxxxxx>
> ---
> 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.
> 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/
> * 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?
> 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.
> 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 :)
> pos = cache_name_pos(cp, namelen);
> if (pos < 0)
> pos = -pos - 1;
> --
> 2.17.0.582.gccdcbd54c44.dirty
>
