Re: [PATCH 00/15] Kill the_index part 1, expose it
On 6/18/2018 2:41 PM, Brandon Williams wrote: On 06/17, Duy Nguyen wrote: On Sun, Jun 17, 2018 at 9:02 AM Elijah Newren wrote: On Fri, Jun 15, 2018 at 10:41 PM, Nguyễn Thái Ngọc Duy wrote: This is the beginning of the end of the_index. The problem with the_index is it lets library code anywhere access it freely. This is not good because from high level you may not realize that the_index is being used while you don't want to touch index at all, or you want to use a different index instead. This is a long series, 86 patches [1], so I'm going to split and submit it in 15-20 patches at a time. The first two parts are trivial though and could be safely fast tracked if needed. You post this small little patch about unpack-trees.c, mentioning you don't know if it's even correct, and bait me into reviewing it and then spring on me that it's actually nearly 100 patches that need review... Very sneaky. ;-) To be fair, it's all Brandon's fault. If he didn't kick the_index out of dir.c, it would not conflict with one of my out-of-tree patches in unpack-trees.c, catch my attention and make me go down this rabbit hole :D Haha well this is something I've wanted to do for over a year now, glad you've decided to run with it :) I guess I've gotten pretty good at getting people to go down rabbit holes. First Stefan with the object store refactoring and now you with the index stuff. All because I wanted git to be more object oriented and have less global state ;) Let me join the chorus of voices excited to see someone finally taking the plunge to do this! I'm very happy about seeing this taken through to "the end of the_index."
Re: [PATCH 00/15] Kill the_index part 1, expose it
On Tue, Jun 19, 2018 at 1:48 PM Derrick Stolee wrote: > Personally, > I find it difficult to base a patch off of multiple in-progress branches > and would rather work off of a "known good" point like the tip of master. You should always base your patches on 'master' (or even 'maint' if it's bug fixes that have a chance of entering 'maint'). We all we face conflicts at some point (mostly when we rebase after something gets merged to master; or things are merged on 'pu'). But git-rerere should keep conflict resolution easy (most of the time). I also expect Junio to kick and scream when patch series conflict badly on 'pu' and he will decide which one goes first and kick others out temporarily. -- Duy
Re: [PATCH 00/15] Kill the_index part 1, expose it
On 6/16/2018 1:41 AM, Nguyễn Thái Ngọc Duy wrote: This is the beginning of the end of the_index. The problem with the_index is it lets library code anywhere access it freely. This is not good because from high level you may not realize that the_index is being used while you don't want to touch index at all, or you want to use a different index instead. This is a long series, 86 patches [1], so I'm going to split and submit it in 15-20 patches at a time. The first two parts are trivial though and could be safely fast tracked if needed. This is the first part, which kills the use of index compat macros outside builtin/ and expose the_index in all library code. Later on we will ban the_index from one file each time until it's gone for good. "struct index_state *" will be passed from builtin/ through the call chain to the function that needs it. In some cases, "struct repository *" will be passed instead when the whole operation spans more than just the index. By the end, the_index becomes part of "index compat macros" and cannot be used outside builtin/ Part one is mechanical conversion with the help of coccinelle. The only real patches are the first and the last one. [1] https://gitlab.com/pclouds/git/commits/really-kill-the-index This is a good series, and a good goal! Outside of dropping [PATCH 01/15] until all the changes are applied, this patch looks like a good, mechanical change. There are a lot of cross-cutting changes happening right now, between this series of series and Stefan's series of series. Hopefully after 2.18 is cut, a lot of these can graduate to master quickly. Personally, I find it difficult to base a patch off of multiple in-progress branches and would rather work off of a "known good" point like the tip of master. Thanks, -Stolee
Re: [PATCH 00/15] Kill the_index part 1, expose it
On 06/17, Duy Nguyen wrote: > On Sun, Jun 17, 2018 at 9:02 AM Elijah Newren wrote: > > > > On Fri, Jun 15, 2018 at 10:41 PM, Nguyễn Thái Ngọc Duy > > wrote: > > > This is the beginning of the end of the_index. The problem with > > > the_index is it lets library code anywhere access it freely. This is > > > not good because from high level you may not realize that the_index is > > > being used while you don't want to touch index at all, or you want to > > > use a different index instead. > > > > > > This is a long series, 86 patches [1], so I'm going to split and > > > submit it in 15-20 patches at a time. The first two parts are trivial > > > though and could be safely fast tracked if needed. > > > > You post this small little patch about unpack-trees.c, mentioning you > > don't know if it's even correct, and bait me into reviewing it and > > then spring on me that it's actually nearly 100 patches that need > > review... Very sneaky. ;-) > > To be fair, it's all Brandon's fault. If he didn't kick the_index out > of dir.c, it would not conflict with one of my out-of-tree patches in > unpack-trees.c, catch my attention and make me go down this rabbit > hole :D Haha well this is something I've wanted to do for over a year now, glad you've decided to run with it :) I guess I've gotten pretty good at getting people to go down rabbit holes. First Stefan with the object store refactoring and now you with the index stuff. All because I wanted git to be more object oriented and have less global state ;) -- Brandon Williams
Re: [PATCH 00/15] Kill the_index part 1, expose it
On Sun, Jun 17, 2018 at 9:02 AM Elijah Newren wrote: > > On Fri, Jun 15, 2018 at 10:41 PM, Nguyễn Thái Ngọc Duy > wrote: > > This is the beginning of the end of the_index. The problem with > > the_index is it lets library code anywhere access it freely. This is > > not good because from high level you may not realize that the_index is > > being used while you don't want to touch index at all, or you want to > > use a different index instead. > > > > This is a long series, 86 patches [1], so I'm going to split and > > submit it in 15-20 patches at a time. The first two parts are trivial > > though and could be safely fast tracked if needed. > > You post this small little patch about unpack-trees.c, mentioning you > don't know if it's even correct, and bait me into reviewing it and > then spring on me that it's actually nearly 100 patches that need > review... Very sneaky. ;-) To be fair, it's all Brandon's fault. If he didn't kick the_index out of dir.c, it would not conflict with one of my out-of-tree patches in unpack-trees.c, catch my attention and make me go down this rabbit hole :D -- Duy
Re: [PATCH 00/15] Kill the_index part 1, expose it
On Fri, Jun 15, 2018 at 10:41 PM, Nguyễn Thái Ngọc Duy wrote: > This is the beginning of the end of the_index. The problem with > the_index is it lets library code anywhere access it freely. This is > not good because from high level you may not realize that the_index is > being used while you don't want to touch index at all, or you want to > use a different index instead. > > This is a long series, 86 patches [1], so I'm going to split and > submit it in 15-20 patches at a time. The first two parts are trivial > though and could be safely fast tracked if needed. You post this small little patch about unpack-trees.c, mentioning you don't know if it's even correct, and bait me into reviewing it and then spring on me that it's actually nearly 100 patches that need review... Very sneaky. ;-) > This is the first part, which kills the use of index compat macros > outside builtin/ and expose the_index in all library code. Later on we > will ban the_index from one file each time until it's gone for good. > > "struct index_state *" will be passed from builtin/ through the call > chain to the function that needs it. In some cases, "struct > repository *" will be passed instead when the whole operation spans > more than just the index. By the end, the_index becomes part of > "index compat macros" and cannot be used outside builtin/ > > Part one is mechanical conversion with the help of coccinelle. The > only real patches are the first and the last one. Thanks for the nice division. I read through all 15 patches, though I looked at the first and the last a bit closer. I'm not familiar with coccinelle yet, but it at least looked relatively straightforward; would be good to have someone else double check that patch. Other than that, the changes look good to me. Thanks for working on this!
[PATCH 00/15] Kill the_index part 1, expose it
This is the beginning of the end of the_index. The problem with the_index is it lets library code anywhere access it freely. This is not good because from high level you may not realize that the_index is being used while you don't want to touch index at all, or you want to use a different index instead. This is a long series, 86 patches [1], so I'm going to split and submit it in 15-20 patches at a time. The first two parts are trivial though and could be safely fast tracked if needed. This is the first part, which kills the use of index compat macros outside builtin/ and expose the_index in all library code. Later on we will ban the_index from one file each time until it's gone for good. "struct index_state *" will be passed from builtin/ through the call chain to the function that needs it. In some cases, "struct repository *" will be passed instead when the whole operation spans more than just the index. By the end, the_index becomes part of "index compat macros" and cannot be used outside builtin/ Part one is mechanical conversion with the help of coccinelle. The only real patches are the first and the last one. [1] https://gitlab.com/pclouds/git/commits/really-kill-the-index Nguyễn Thái Ngọc Duy (15): contrib: add cocci script to replace index compat macros apply.c: stop using index compat macros blame.c: stop using index compat macros check-racy.c: stop using index compat macros diff-lib.c: stop using index compat macros diff.c: stop using index compat macros entry.c: stop using index compat macros merge-recursive.c: stop using index compat macros merge.c: stop using index compat macros rerere.c: stop using index compat macros revision.c: stop using index compat macros sequencer.c: stop using index compat macros sha1-name.c: stop using index compat macros wt-status.c: stop using index compat macros cache.h: flip NO_THE_INDEX_COMPATIBILITY_MACROS switch apply.c | 34 ++--- attr.c| 1 - blame.c | 19 +-- builtin/add.c | 1 + builtin/am.c | 1 + builtin/check-attr.c | 1 + builtin/check-ignore.c| 1 + builtin/checkout-index.c | 1 + builtin/checkout.c| 1 + builtin/clean.c | 1 + builtin/commit.c | 1 + builtin/describe.c| 1 + builtin/diff-files.c | 1 + builtin/diff-index.c | 1 + builtin/diff-tree.c | 1 + builtin/diff.c| 1 + builtin/fsck.c| 1 + builtin/ls-files.c| 1 - builtin/merge-index.c | 1 + builtin/merge-ours.c | 1 + builtin/merge.c | 1 + builtin/mv.c | 1 + builtin/pull.c| 1 + builtin/read-tree.c | 1 + builtin/reset.c | 1 + builtin/rev-parse.c | 1 + builtin/rm.c | 1 + builtin/submodule--helper.c | 1 + builtin/update-index.c| 1 + cache.h | 2 +- check-racy.c | 10 +- contrib/coccinelle/index-compat.cocci | 184 ++ convert.c | 1 - diff-lib.c| 8 +- diff.c| 12 +- dir.c | 1 - entry.c | 3 +- merge-recursive.c | 65 - merge.c | 14 +- name-hash.c | 1 - pathspec.c| 1 - read-cache.c | 1 - rerere.c | 36 ++--- revision.c| 14 +- sequencer.c | 32 ++--- sha1-name.c | 22 +-- submodule.c | 1 - t/helper/test-dump-untracked-cache.c | 1 + t/helper/test-tool.h | 2 + tree.c| 1 - unpack-trees.c| 1 - wt-status.c | 24 ++-- 52 files changed, 363 insertions(+), 154 deletions(-) create mode 100644 contrib/coccinelle/index-compat.cocci -- 2.18.0.rc0.333.g22e6ee6cdf
