[PATCH v4 00/23] Fix incorrect use of the_index
v4 fixes some commit messages and killed a couple more the_index references after I tried to merge this with 'pu' diff --git a/apply.c b/apply.c index 811ff2ad5e..82f681972f 100644 --- a/apply.c +++ b/apply.c @@ -4090,9 +4090,9 @@ static int build_fake_ancestor(struct apply_state *state, struct patch *list) return error(_("sha1 information is lacking or useless " "(%s)."), name); - ce = make_cache_entry(&the_index, patch->old_mode, oid.hash, name, 0, 0); + ce = make_index_entry(&result, patch->old_mode, oid.hash, name, 0, 0); if (!ce) - return error(_("make_cache_entry failed for path '%s'"), + return error(_("make_index_entry failed for path '%s'"), name); if (add_index_entry(&result, ce, ADD_CACHE_OK_TO_ADD)) { free(ce); diff --git a/builtin/checkout.c b/builtin/checkout.c index 3c8218304e..4dbcab3727 100644 --- a/builtin/checkout.c +++ b/builtin/checkout.c @@ -230,9 +230,9 @@ static int checkout_merged(int pos, const struct checkout *state) if (write_object_file(result_buf.ptr, result_buf.size, blob_type, &oid)) die(_("Unable to add merge result for '%s'"), path); free(result_buf.ptr); - ce = make_cache_entry(&the_index, mode, oid.hash, path, 2, 0); + ce = make_index_entry(state->istate, mode, oid.hash, path, 2, 0); if (!ce) - die(_("make_cache_entry failed for path '%s'"), path); + die(_("make_index_entry failed for path '%s'"), path); status = checkout_entry(ce, state, NULL); free(ce); return status; diff --git a/builtin/difftool.c b/builtin/difftool.c index e34e75a42d..c7d6296762 100644 --- a/builtin/difftool.c +++ b/builtin/difftool.c @@ -321,7 +321,7 @@ static int checkout_path(unsigned mode, struct object_id *oid, struct cache_entry *ce; int ret; - ce = make_cache_entry(&the_index, mode, oid->hash, path, 0, 0); + ce = make_index_entry(state->istate, mode, oid->hash, path, 0, 0); ret = checkout_entry(ce, state, NULL); free(ce); @@ -488,7 +488,7 @@ static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix, * index. */ struct cache_entry *ce2 = - make_cache_entry(&the_index, rmode, roid.hash, + make_index_entry(&wtindex, rmode, roid.hash, dst_path, 0, 0); add_index_entry(&wtindex, ce2, diff --git a/builtin/reset.c b/builtin/reset.c index 0ea0a19d5e..067f535031 100644 --- a/builtin/reset.c +++ b/builtin/reset.c @@ -134,10 +134,10 @@ static void update_index_from_diff(struct diff_queue_struct *q, continue; } - ce = make_cache_entry(&the_index, one->mode, one->oid.hash, one->path, + ce = make_index_entry(&the_index, one->mode, one->oid.hash, one->path, 0, 0); if (!ce) - die(_("make_cache_entry failed for path '%s'"), + die(_("make_index_entry failed for path '%s'"), one->path); if (is_missing) { ce->ce_flags |= CE_INTENT_TO_ADD; diff --git a/cache.h b/cache.h index 242aaa5498..eb8c79b8a1 100644 --- a/cache.h +++ b/cache.h @@ -699,7 +699,7 @@ extern int remove_file_from_index(struct index_state *, const char *path); extern int add_to_index(struct index_state *, const char *path, struct stat *, int flags); extern int add_file_to_index(struct index_state *, const char *path, int flags); -extern struct cache_entry *make_cache_entry(struct index_state *istate,unsigned int mode, const unsigned char *sha1, const char *path, int stage, unsigned int refresh_options); +extern struct cache_entry *make_index_entry(struct index_state *istate, unsigned int mode, const unsigned char *sha1, const char *path, int stage, unsigned int refresh_options); extern int chmod_index_entry(struct index_state *, struct cache_entry *ce, char flip); extern int ce_same_name(const struct cache_entry *a, const struct cache_entry *b); extern void set_object_name_for_intent_to_add_entry(struct cache_entry *ce); diff --git a/merge-recursive.c b/merge-recursive.c index 9280deb6a1..4f054d6dbb 100644 --- a/merge-recursive.c +++ b/merge-recursive.c @@ -315,7 +315,7 @@ static int add_cacheinfo(struct merge_options *o, struct cache_entry *ce; int ret; - ce = make_cache_entry(&the_index, mode, oid ? oid->hash : null_sha1, path, stage, 0); + ce = make_index_entry(&the_index, mode, oid ? oid->hash : null_s
Re: [PATCH v4 00/23] Fix incorrect use of the_index
On Wed, Jun 6, 2018 at 9:49 AM, Nguyễn Thái Ngọc Duy wrote: > v4 fixes some commit messages and killed a couple more the_index > references after I tried to merge this with 'pu' Thanks for tackling this. The first 8 patches look good to me other than the typo I pointed out on patch 4. However, my eyes glazed over a bit on the attr.c stuff in patch 7 that you specifically mentioned in the commit message, so my "looks good" doesn't count for as much on that one. I'm getting sleepy, but I'll try to circle back and look over the rest of the patches in the next few days.
Re: [PATCH v4 00/23] Fix incorrect use of the_index
On Thu, Jun 7, 2018 at 12:44 AM, Elijah Newren wrote: > On Wed, Jun 6, 2018 at 9:49 AM, Nguyễn Thái Ngọc Duy > wrote: >> v4 fixes some commit messages and killed a couple more the_index >> references after I tried to merge this with 'pu' > > Thanks for tackling this. The first 8 patches look good to me other > than the typo I pointed out on patch 4. However, my eyes glazed over > a bit on the attr.c stuff in patch 7 that you specifically mentioned > in the commit message, so my "looks good" doesn't count for as much on > that one. I re-read the attr.c stuff in patch 7; looks good to me and the new series makes the code much more readable in the end. > I'm getting sleepy, but I'll try to circle back and look over the rest > of the patches in the next few days. I read over the rest. Found a small grammatical error in a commit message. Found multiple places that still need conversion, from pushing up &the_index usages to callers of ll-merge.c and sha1-file.c instead of having them in those files, to mixes of _cache_ and _index_ functions as in apply.c and merge-recursive.c. However, Duy pointed out there was more work to do, and this series is a step in the right direction and a good start.
Re: [PATCH v4 00/23] Fix incorrect use of the_index
On Sat, Jun 9, 2018 at 9:58 PM Elijah Newren wrote: > I read over the rest. Found a small grammatical error in a commit > message. Found multiple places that still need conversion, from > pushing up &the_index usages to callers of ll-merge.c and sha1-file.c > instead of having them in those files, to mixes of _cache_ and _index_ > functions as in apply.c and merge-recursive.c. However, Duy pointed > out there was more work to do, Yes. This is just fyi, 40 patches later, i'm down to leaving the_index in three files outside builtin/: merge-recursive, notes-merge.c and transport.c. Even after the conversion we may need some more follow-up patches because it now shows places where we should _not_ touch the index at all, which may involve not simply passing NULL index_state to some functions, but fixing them up to tolerate NULL index_state. So it's going to be a few patch series until the_index is gone for good [1]. [1] but like cheap horror movies, there's always a sequel: the_repository is still spread in many places and hides dependencies in the same way. We can't do anything about it though until struct repository conversion is more or less complete. -- Duy
Re: [PATCH v4 00/23] Fix incorrect use of the_index
Hi Duy, On Mon, Jun 11, 2018 at 9:05 AM, Duy Nguyen wrote: > On Sat, Jun 9, 2018 at 9:58 PM Elijah Newren wrote: >> I read over the rest. Found a small grammatical error in a commit >> message. Found multiple places that still need conversion, from >> pushing up &the_index usages to callers of ll-merge.c and sha1-file.c >> instead of having them in those files, to mixes of _cache_ and _index_ >> functions as in apply.c and merge-recursive.c. However, Duy pointed >> out there was more work to do, > > Yes. This is just fyi, 40 patches later, i'm down to leaving the_index > in three files outside builtin/: merge-recursive, notes-merge.c and > transport.c. Even after the conversion we may need some more follow-up > patches because it now shows places where we should _not_ touch the > index at all, which may involve not simply passing NULL index_state to > some functions, but fixing them up to tolerate NULL index_state. So > it's going to be a few patch series until the_index is gone for good > [1]. > > [1] but like cheap horror movies, there's always a sequel: > the_repository is still spread in many places and hides dependencies > in the same way. We can't do anything about it though until struct > repository conversion is more or less complete. I was just about to send you an email to ask if you were continuing on with the series. I need diff-lib.c converted in order to make the changes Junio suggested to index_has_changes at https://public-inbox.org/git/xmqqvaaz5jcv@gitster-ct.c.googlers.com/. Since you're already working on that, I won't duplicate your effort. Thanks for tackling all of this. :-)
Re: [PATCH v4 00/23] Fix incorrect use of the_index
On Mon, Jun 11, 2018 at 6:11 PM Elijah Newren wrote: > > Hi Duy, > > On Mon, Jun 11, 2018 at 9:05 AM, Duy Nguyen wrote: > > On Sat, Jun 9, 2018 at 9:58 PM Elijah Newren wrote: > >> I read over the rest. Found a small grammatical error in a commit > >> message. Found multiple places that still need conversion, from > >> pushing up &the_index usages to callers of ll-merge.c and sha1-file.c > >> instead of having them in those files, to mixes of _cache_ and _index_ > >> functions as in apply.c and merge-recursive.c. However, Duy pointed > >> out there was more work to do, > > > > Yes. This is just fyi, 40 patches later, i'm down to leaving the_index > > in three files outside builtin/: merge-recursive, notes-merge.c and > > transport.c. Even after the conversion we may need some more follow-up > > patches because it now shows places where we should _not_ touch the > > index at all, which may involve not simply passing NULL index_state to > > some functions, but fixing them up to tolerate NULL index_state. So > > it's going to be a few patch series until the_index is gone for good > > [1]. And I forgot one thing. There are other hidden dependencies as well. Like hold_locked_index() will assume $GIT_DIR/index, but when you take an arbitrary 'struct index_state *' I don't think you can make that kind of assumption. This is mostly a note to myself in case I forget it again. > > > > [1] but like cheap horror movies, there's always a sequel: > > the_repository is still spread in many places and hides dependencies > > in the same way. We can't do anything about it though until struct > > repository conversion is more or less complete. > > I was just about to send you an email to ask if you were continuing on > with the series. I need diff-lib.c converted in order to make the > changes Junio suggested to index_has_changes at > https://public-inbox.org/git/xmqqvaaz5jcv@gitster-ct.c.googlers.com/. > Since you're already working on that, I won't duplicate your effort. > Thanks for tackling all of this. :-) I'm not sure if it's possible to cherry pick this patch to continue your work (because of dependencies between patches) but it's https://gitlab.com/pclouds/git/commits/really-kill-the-index, commit "merge.c: remove...". Or you just leave it to me, update has_index_changes() to always take 'struct index_state *' and just pass &the_index in all uninteresting places. (Or not update at all if it's really not needed for your work) What about merge-recursive.c? Given that this whole thing will take many release cycles to finish, your work may get merged before mine and I could do the conversion now (and resolve conflicts and resubmit later). Of course if you like to keep merge-recursive.c the_index-free now, I will not stop you ;-) -- Duy
Re: [PATCH v4 00/23] Fix incorrect use of the_index
On Mon, Jun 11, 2018 at 9:24 AM, Duy Nguyen wrote: >> I was just about to send you an email to ask if you were continuing on >> with the series. I need diff-lib.c converted in order to make the >> changes Junio suggested to index_has_changes at >> https://public-inbox.org/git/xmqqvaaz5jcv@gitster-ct.c.googlers.com/. >> Since you're already working on that, I won't duplicate your effort. >> Thanks for tackling all of this. :-) > > I'm not sure if it's possible to cherry pick this patch to continue > your work (because of dependencies between patches) but it's > https://gitlab.com/pclouds/git/commits/really-kill-the-index, commit > "merge.c: remove...". > > Or you just leave it to me, update has_index_changes() to always take > 'struct index_state *' and just pass &the_index in all uninteresting > places. (Or not update at all if it's really not needed for your work) Actually, I'm thinking maybe I should just push that series forward without changing index_has_changes() to take an istate (since it's not actually important to that series yet), and then once your changes land, then update the function appropriately. (Or maybe your series will update that function too.) > What about merge-recursive.c? Given that this whole thing will take > many release cycles to finish, your work may get merged before mine > and I could do the conversion now (and resolve conflicts and resubmit > later). Of course if you like to keep merge-recursive.c the_index-free > now, I will not stop you ;-) I was just worried that since I was making changes in merge-recursive.c that it'd cause you conflict pain, so I offered to convert it. If that pain doesn't bother you, feel free to do the conversion and we'll just work through the minor conflicts as we go. Besides, sounds like you've converted nearly all of git, so it may make sense to just keep it together in one big series.
Re: [PATCH v4 00/23] Fix incorrect use of the_index
On Mon, Jun 11, 2018 at 6:44 PM Elijah Newren wrote: > > What about merge-recursive.c? Given that this whole thing will take > > many release cycles to finish, your work may get merged before mine > > and I could do the conversion now (and resolve conflicts and resubmit > > later). Of course if you like to keep merge-recursive.c the_index-free > > now, I will not stop you ;-) > > I was just worried that since I was making changes in > merge-recursive.c that it'd cause you conflict pain, so I offered to > convert it. If that pain doesn't bother you, feel free to do the > conversion and we'll just work through the minor conflicts as we go. > Besides, sounds like you've converted nearly all of git, so it may > make sense to just keep it together in one big series. OK let's just "quickly" get your series in then. I still have a few files to go and a couple more places to look back and ponder. I'm in no hurry to convert merge-recursive now :D -- Duy
Re: [PATCH v4 00/23] Fix incorrect use of the_index
On Mon, Jun 11, 2018 at 6:05 PM Duy Nguyen wrote: > > On Sat, Jun 9, 2018 at 9:58 PM Elijah Newren wrote: > > I read over the rest. Found a small grammatical error in a commit > > message. Found multiple places that still need conversion, from > > pushing up &the_index usages to callers of ll-merge.c and sha1-file.c > > instead of having them in those files, to mixes of _cache_ and _index_ > > functions as in apply.c and merge-recursive.c. However, Duy pointed > > out there was more work to do, > > Yes. This is just fyi, 40 patches later, ... Junio, just to be clear, I think I'll withdraw this 23-patch series (reviews are still welcome though). It does fix some potential bugs but it's not that critical. This will let me merge it with the other 40+ patches and reorganize better (after all this started out as a single rfc patch, I didn't realize what I got myself into) > i'm down to leaving the_index > in three files outside builtin/: merge-recursive, notes-merge.c and > transport.c. Even after the conversion we may need some more follow-up > patches because it now shows places where we should _not_ touch the > index at all, which may involve not simply passing NULL index_state to > some functions, but fixing them up to tolerate NULL index_state. So > it's going to be a few patch series until the_index is gone for good > [1]. > > [1] but like cheap horror movies, there's always a sequel: > the_repository is still spread in many places and hides dependencies > in the same way. We can't do anything about it though until struct > repository conversion is more or less complete. > -- > Duy -- Duy
Re: [PATCH v4 00/23] Fix incorrect use of the_index
On Mon, Jun 11, 2018 at 06:49:00PM +0200, Duy Nguyen wrote: > On Mon, Jun 11, 2018 at 6:44 PM Elijah Newren wrote: > > > What about merge-recursive.c? Given that this whole thing will take > > > many release cycles to finish, your work may get merged before mine > > > and I could do the conversion now (and resolve conflicts and resubmit > > > later). Of course if you like to keep merge-recursive.c the_index-free > > > now, I will not stop you ;-) > > > > I was just worried that since I was making changes in > > merge-recursive.c that it'd cause you conflict pain, so I offered to > > convert it. If that pain doesn't bother you, feel free to do the > > conversion and we'll just work through the minor conflicts as we go. > > Besides, sounds like you've converted nearly all of git, so it may > > make sense to just keep it together in one big series. > > OK let's just "quickly" get your series in then. I still have a few > files to go and a couple more places to look back and ponder. I'm in > no hurry to convert merge-recursive now :D Actually could you quickly check if I pass the right index in the following patch? I realize you only have one extra index, but I don't know if it gets passed elsewhere and in some function I'm supposed to use another index than the default "o->repo->index". Sometimes o->repo->index is not the correct answer. It takes many patches to get to this stage, so if you diff context looks so strange to you, just ignore this for now. I'll eventually send this one out in a series. PS. I put a 'struct repository *' in 'struct merge_options' instead because it turns out we do need full repo struct in some function. -- 8< -- diff --git a/builtin/am.c b/builtin/am.c index 3961878871..5d2ff9aa8c 100644 --- a/builtin/am.c +++ b/builtin/am.c @@ -1592,7 +1592,7 @@ static int fall_back_threeway(const struct am_state *state, const char *index_pa * changes. */ - init_merge_options(&o); + init_merge_options(&o, the_repository); o.branch1 = "HEAD"; their_tree_name = xstrfmt("%.*s", linelen(state->msg), state->msg); diff --git a/builtin/checkout.c b/builtin/checkout.c index 528c6de7e1..4fd53cec6e 100644 --- a/builtin/checkout.c +++ b/builtin/checkout.c @@ -570,7 +570,7 @@ static int merge_working_tree(const struct checkout_opts *opts, * a pain; plumb in an option to set * o.renormalize? */ - init_merge_options(&o); + init_merge_options(&o, the_repository); o.verbosity = 0; work = write_tree_from_memory(&o); diff --git a/builtin/merge-recursive.c b/builtin/merge-recursive.c index 0dd9021958..c4a9e575d6 100644 --- a/builtin/merge-recursive.c +++ b/builtin/merge-recursive.c @@ -28,7 +28,7 @@ int cmd_merge_recursive(int argc, const char **argv, const char *prefix) struct merge_options o; struct commit *result; - init_merge_options(&o); + init_merge_options(&o, the_repository); if (argv[0] && ends_with(argv[0], "-subtree")) o.subtree_shift = ""; diff --git a/builtin/merge.c b/builtin/merge.c index db460a35cf..2ebd939b76 100644 --- a/builtin/merge.c +++ b/builtin/merge.c @@ -671,7 +671,7 @@ static int try_merge_strategy(const char *strategy, struct commit_list *common, return 2; } - init_merge_options(&o); + init_merge_options(&o, the_repository); if (!strcmp(strategy, "subtree")) o.subtree_shift = ""; diff --git a/merge-recursive.c b/merge-recursive.c index a296ba3874..fd6556b6de 100644 --- a/merge-recursive.c +++ b/merge-recursive.c @@ -309,26 +309,29 @@ static void output_commit_title(struct merge_options *o, struct commit *commit) } static int add_cacheinfo(struct merge_options *o, - unsigned int mode, const struct object_id *oid, - const char *path, int stage, int refresh, int options) +unsigned int mode, +const struct object_id *oid, +const char *path, int stage, +int refresh, int options) { + struct index_state *istate = o->repo->index; struct cache_entry *ce; int ret; - ce = make_index_entry(&the_index, mode, oid ? oid->hash : null_sha1, path, stage, 0); + ce = make_index_entry(istate, mode, oid ? oid->hash : null_sha1, path, stage, 0); if (!ce) return err(o, _("add_cacheinfo failed for path '%s'; merge aborting."), path); - ret = add_index_entry(&the_index, ce, options); + ret = add_index_entry(istate, ce, options); if (refresh) { struct cache_entry *nce; - nce = refresh_index_entry(&the_index, ce, + nce = refresh_index_entry(istate, ce,
Re: [PATCH v4 00/23] Fix incorrect use of the_index
On Thu, Jun 14, 2018 at 10:18 AM, Duy Nguyen wrote: > On Mon, Jun 11, 2018 at 06:49:00PM +0200, Duy Nguyen wrote: >> On Mon, Jun 11, 2018 at 6:44 PM Elijah Newren wrote: >> > > What about merge-recursive.c? Given that this whole thing will take >> > > many release cycles to finish, your work may get merged before mine >> > > and I could do the conversion now (and resolve conflicts and resubmit >> > > later). Of course if you like to keep merge-recursive.c the_index-free >> > > now, I will not stop you ;-) >> > >> > I was just worried that since I was making changes in >> > merge-recursive.c that it'd cause you conflict pain, so I offered to >> > convert it. If that pain doesn't bother you, feel free to do the >> > conversion and we'll just work through the minor conflicts as we go. >> > Besides, sounds like you've converted nearly all of git, so it may >> > make sense to just keep it together in one big series. >> >> OK let's just "quickly" get your series in then. I still have a few >> files to go and a couple more places to look back and ponder. I'm in >> no hurry to convert merge-recursive now :D > > Actually could you quickly check if I pass the right index in the > following patch? I realize you only have one extra index, but I don't > know if it gets passed elsewhere and in some function I'm supposed to > use another index than the default "o->repo->index". Sometimes > o->repo->index is not the correct answer. > > It takes many patches to get to this stage, so if you diff context > looks so strange to you, just ignore this for now. I'll eventually > send this one out in a series. > > PS. I put a 'struct repository *' in 'struct merge_options' instead > because it turns out we do need full repo struct in some function. Yes, this looks right. It'd be nice to be able to apply the patch locally so I could also look at the --color-words version, but it's not a real big deal. In general, this is how the index stuff works in merge-recursive: * In unpack_trees_start(), we call unpack_trees() using the_index as src, and tmp_index as dest. * As soon as unpack_trees() finishes, we swap: (orig_index, the_index) = (the_index, tmp_index). * We ONLY use orig_index (also called unpack_opts.src_index) in the was_*() family of functions, which query about what the index was like before the merge started. (To name these functions explicitly: was_tracked_and_matches(), was_tracked(), and was_dirty()). ALL other callsites within merge-recursive.c should be operating on the_index, or, after your patch, o->repo->index. (Also, if you want to avoid merge conflicts, you might want to avoid unrelated changes like the whitespace and alignment fixes. You'll be happy to know, though, that I think I fixed them all up as part of the en/merge-recursive-cleanup series in pu.) Elijah > > -- 8< -- > diff --git a/builtin/am.c b/builtin/am.c > index 3961878871..5d2ff9aa8c 100644 > --- a/builtin/am.c > +++ b/builtin/am.c > @@ -1592,7 +1592,7 @@ static int fall_back_threeway(const struct am_state > *state, const char *index_pa > * changes. > */ > > - init_merge_options(&o); > + init_merge_options(&o, the_repository); > > o.branch1 = "HEAD"; > their_tree_name = xstrfmt("%.*s", linelen(state->msg), state->msg); > diff --git a/builtin/checkout.c b/builtin/checkout.c > index 528c6de7e1..4fd53cec6e 100644 > --- a/builtin/checkout.c > +++ b/builtin/checkout.c > @@ -570,7 +570,7 @@ static int merge_working_tree(const struct checkout_opts > *opts, > * a pain; plumb in an option to set > * o.renormalize? > */ > - init_merge_options(&o); > + init_merge_options(&o, the_repository); > o.verbosity = 0; > work = write_tree_from_memory(&o); > > diff --git a/builtin/merge-recursive.c b/builtin/merge-recursive.c > index 0dd9021958..c4a9e575d6 100644 > --- a/builtin/merge-recursive.c > +++ b/builtin/merge-recursive.c > @@ -28,7 +28,7 @@ int cmd_merge_recursive(int argc, const char **argv, const > char *prefix) > struct merge_options o; > struct commit *result; > > - init_merge_options(&o); > + init_merge_options(&o, the_repository); > if (argv[0] && ends_with(argv[0], "-subtree")) > o.subtree_shift = ""; > > diff --git a/builtin/merge.c b/builtin/merge.c > index db460a35cf..2ebd939b76 100644 > --- a/builtin/merge.c > +++ b/builtin/merge.c > @@ -671,7 +671,7 @@ static int try_merge_strategy(const char *strategy, > struct commit_list *common, > return 2; > } > > - init_merge_options(&o); > + init_merge_options(&o, the_repository); > if (!strcmp(strategy, "subtree")) > o.subtree_shift = ""; > > diff --git a/merge-recursive.c b/merge-recursive.c > index a296ba3874..fd6556b