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();
> + init_merge_options(, 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();
> + init_merge_options(, the_repository);
> o.verbosity = 0;
> work = write_tree_from_memory();
>
> 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();
> + init_merge_options(, 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();
> + init_merge_options(, the_repository);
> if (!strcmp(strategy, "subtree"))
> o.subtree_shift = "";
>
> diff --git a/merge-recursive.c b/merge-recursive.c
> index a296ba3874..fd6556b6de 100644
> ---
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();
+ init_merge_options(, 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();
+ init_merge_options(, the_repository);
o.verbosity = 0;
work = write_tree_from_memory();
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();
+ init_merge_options(, 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();
+ init_merge_options(, 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(_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(_index, ce, options);
+ ret = add_index_entry(istate, ce, options);
if (refresh) {
struct cache_entry *nce;
- nce = refresh_index_entry(_index, ce,
+ nce = refresh_index_entry(istate, ce,
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 _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 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 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 _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: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 _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 _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
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 _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 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 _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
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 _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 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.
