Re: [PATCH 11/11] read_cache: convert most calls to repo_read_index_or_die
On Wed, May 16, 2018 at 03:27:33PM -0700, Brandon Williams wrote: > Maybe we can kill read_cache() while at it? I took this out of context. But with the move to repo_* stuff, eventually these macros around the_index should die. Not today though. -- Duy
Re: [PATCH 11/11] read_cache: convert most calls to repo_read_index_or_die
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> --- > 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 >
Re: [PATCH 11/11] read_cache: convert most calls to repo_read_index_or_die
On 05/16, Stefan Beller wrote: > Signed-off-by: Stefan Beller> --- > blame.c | 5 +++-- > builtin/am.c | 3 ++- > builtin/diff.c| 3 ++- > builtin/fsck.c| 3 ++- > builtin/merge-index.c | 3 ++- > check-racy.c | 2 +- > diff.c| 5 +++-- > merge-recursive.c | 3 ++- > revision.c| 5 +++-- > sequencer.c | 5 +++-- > sha1-name.c | 2 +- > 11 files changed, 24 insertions(+), 15 deletions(-) > > diff --git a/blame.c b/blame.c > index 78c9808bd1a..ebfa1c8efcd 100644 > --- a/blame.c > +++ b/blame.c > @@ -5,6 +5,7 @@ > #include "diff.h" > #include "diffcore.h" > #include "tag.h" > +#include "repository.h" > #include "blame.h" > > void blame_origin_decref(struct blame_origin *o) > @@ -159,7 +160,7 @@ static struct commit *fake_working_tree_commit(struct > diff_options *opt, > unsigned mode; > struct strbuf msg = STRBUF_INIT; > > - read_cache(); > + repo_read_index_or_die(the_repository); > time(); > commit = alloc_commit_node(); > commit->object.parsed = 1; > @@ -241,7 +242,7 @@ static struct commit *fake_working_tree_commit(struct > diff_options *opt, >* want to run "diff-index --cached". >*/ > discard_cache(); > - read_cache(); > + repo_read_index_or_die(the_repository); > > len = strlen(path); > if (!mode) { > diff --git a/builtin/am.c b/builtin/am.c > index d834f9e62b6..3c6e77a5369 100644 > --- a/builtin/am.c > +++ b/builtin/am.c > @@ -32,6 +32,7 @@ > #include "apply.h" > #include "string-list.h" > #include "packfile.h" > +#include "repository.h" > > /** > * Returns 1 if the file is empty or does not exist, 0 otherwise. > @@ -1581,7 +1582,7 @@ static int fall_back_threeway(const struct am_state > *state, const char *index_pa > say(state, stdout, _("Falling back to patching base and 3-way > merge...")); > > discard_cache(); > - read_cache(); > + repo_read_index_or_die(the_repository); > > /* >* This is not so wrong. Depending on which base we picked, orig_tree > diff --git a/builtin/diff.c b/builtin/diff.c > index 16bfb22f738..4bba211f1c7 100644 > --- a/builtin/diff.c > +++ b/builtin/diff.c > @@ -17,6 +17,7 @@ > #include "builtin.h" > #include "submodule.h" > #include "sha1-array.h" > +#include "repository.h" > > #define DIFF_NO_INDEX_EXPLICIT 1 > #define DIFF_NO_INDEX_IMPLICIT 2 > @@ -210,7 +211,7 @@ static void refresh_index_quietly(void) > if (fd < 0) > return; > discard_cache(); > - read_cache(); > + repo_read_index(the_repository); /* do not die on error */ > refresh_cache(REFRESH_QUIET|REFRESH_UNMERGED); > update_index_if_able(_index, _file); > } > diff --git a/builtin/fsck.c b/builtin/fsck.c > index 087360a6757..a42e98235da 100644 > --- a/builtin/fsck.c > +++ b/builtin/fsck.c > @@ -18,6 +18,7 @@ > #include "decorate.h" > #include "packfile.h" > #include "object-store.h" > +#include "repository.h" > > #define REACHABLE 0x0001 > #define SEEN 0x0002 > @@ -795,7 +796,7 @@ int cmd_fsck(int argc, const char **argv, const char > *prefix) > if (keep_cache_objects) { > verify_index_checksum = 1; > verify_ce_order = 1; > - read_cache(); > + repo_read_index_or_die(the_repository); > for (i = 0; i < active_nr; i++) { > unsigned int mode; > struct blob *blob; > diff --git a/builtin/merge-index.c b/builtin/merge-index.c > index c99443b095b..2d91c7c3b5e 100644 > --- a/builtin/merge-index.c > +++ b/builtin/merge-index.c > @@ -1,5 +1,6 @@ > #include "builtin.h" > #include "run-command.h" > +#include "repository.h" > > static const char *pgm; > static int one_shot, quiet; > @@ -77,7 +78,7 @@ int cmd_merge_index(int argc, const char **argv, const char > *prefix) > if (argc < 3) > usage("git merge-index [-o] [-q] (-a | [--] > [...])"); > > - read_cache(); > + repo_read_index_or_die(the_repository); > > i = 1; > if (!strcmp(argv[i], "-o")) { > diff --git a/check-racy.c b/check-racy.c > index 24b6542352a..9b884639cf4 100644 > --- a/check-racy.c > +++ b/check-racy.c > @@ -6,7 +6,7 @@ int main(int ac, char **av) > int dirty, clean, racy; > > dirty = clean = racy = 0; > - read_cache(); > + repo_read_index_or_die(the_repository); > for (i = 0; i < active_nr; i++) { > struct cache_entry *ce = active_cache[i]; > struct stat st; > 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
[PATCH 11/11] read_cache: convert most calls to repo_read_index_or_die
Signed-off-by: Stefan Beller--- blame.c | 5 +++-- builtin/am.c | 3 ++- builtin/diff.c| 3 ++- builtin/fsck.c| 3 ++- builtin/merge-index.c | 3 ++- check-racy.c | 2 +- diff.c| 5 +++-- merge-recursive.c | 3 ++- revision.c| 5 +++-- sequencer.c | 5 +++-- sha1-name.c | 2 +- 11 files changed, 24 insertions(+), 15 deletions(-) diff --git a/blame.c b/blame.c index 78c9808bd1a..ebfa1c8efcd 100644 --- a/blame.c +++ b/blame.c @@ -5,6 +5,7 @@ #include "diff.h" #include "diffcore.h" #include "tag.h" +#include "repository.h" #include "blame.h" void blame_origin_decref(struct blame_origin *o) @@ -159,7 +160,7 @@ static struct commit *fake_working_tree_commit(struct diff_options *opt, unsigned mode; struct strbuf msg = STRBUF_INIT; - read_cache(); + repo_read_index_or_die(the_repository); time(); commit = alloc_commit_node(); commit->object.parsed = 1; @@ -241,7 +242,7 @@ static struct commit *fake_working_tree_commit(struct diff_options *opt, * want to run "diff-index --cached". */ discard_cache(); - read_cache(); + repo_read_index_or_die(the_repository); len = strlen(path); if (!mode) { diff --git a/builtin/am.c b/builtin/am.c index d834f9e62b6..3c6e77a5369 100644 --- a/builtin/am.c +++ b/builtin/am.c @@ -32,6 +32,7 @@ #include "apply.h" #include "string-list.h" #include "packfile.h" +#include "repository.h" /** * Returns 1 if the file is empty or does not exist, 0 otherwise. @@ -1581,7 +1582,7 @@ static int fall_back_threeway(const struct am_state *state, const char *index_pa say(state, stdout, _("Falling back to patching base and 3-way merge...")); discard_cache(); - read_cache(); + repo_read_index_or_die(the_repository); /* * This is not so wrong. Depending on which base we picked, orig_tree diff --git a/builtin/diff.c b/builtin/diff.c index 16bfb22f738..4bba211f1c7 100644 --- a/builtin/diff.c +++ b/builtin/diff.c @@ -17,6 +17,7 @@ #include "builtin.h" #include "submodule.h" #include "sha1-array.h" +#include "repository.h" #define DIFF_NO_INDEX_EXPLICIT 1 #define DIFF_NO_INDEX_IMPLICIT 2 @@ -210,7 +211,7 @@ static void refresh_index_quietly(void) if (fd < 0) return; discard_cache(); - read_cache(); + repo_read_index(the_repository); /* do not die on error */ refresh_cache(REFRESH_QUIET|REFRESH_UNMERGED); update_index_if_able(_index, _file); } diff --git a/builtin/fsck.c b/builtin/fsck.c index 087360a6757..a42e98235da 100644 --- a/builtin/fsck.c +++ b/builtin/fsck.c @@ -18,6 +18,7 @@ #include "decorate.h" #include "packfile.h" #include "object-store.h" +#include "repository.h" #define REACHABLE 0x0001 #define SEEN 0x0002 @@ -795,7 +796,7 @@ int cmd_fsck(int argc, const char **argv, const char *prefix) if (keep_cache_objects) { verify_index_checksum = 1; verify_ce_order = 1; - read_cache(); + repo_read_index_or_die(the_repository); for (i = 0; i < active_nr; i++) { unsigned int mode; struct blob *blob; diff --git a/builtin/merge-index.c b/builtin/merge-index.c index c99443b095b..2d91c7c3b5e 100644 --- a/builtin/merge-index.c +++ b/builtin/merge-index.c @@ -1,5 +1,6 @@ #include "builtin.h" #include "run-command.h" +#include "repository.h" static const char *pgm; static int one_shot, quiet; @@ -77,7 +78,7 @@ int cmd_merge_index(int argc, const char **argv, const char *prefix) if (argc < 3) usage("git merge-index [-o] [-q] (-a | [--] [...])"); - read_cache(); + repo_read_index_or_die(the_repository); i = 1; if (!strcmp(argv[i], "-o")) { diff --git a/check-racy.c b/check-racy.c index 24b6542352a..9b884639cf4 100644 --- a/check-racy.c +++ b/check-racy.c @@ -6,7 +6,7 @@ int main(int ac, char **av) int dirty, clean, racy; dirty = clean = racy = 0; - read_cache(); + repo_read_index_or_die(the_repository); for (i = 0; i < active_nr; i++) { struct cache_entry *ce = active_cache[i]; struct stat st; 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 +