unpack-trees.c: do not use "the_index"
unpack-trees code works on multiple indexes specified in unpack_trees_options. Although they normally all refer to the_index at the call site, that is the caller's business. unpack-trees.c should not make any assumption about that and should use the correct index field in unpack_trees_options.
This patch is actually confusing because sometimes the function parameter is also named "the_index" while some other times "the_index" is the global variable because the function just does not have a parameter of the same name! The only subtle difference is that the function parameter is a pointer while the global one is not.
This is more of a bug report than an actual fix because I'm not sure if "o->src_index" is always the correct answer instead of "the_index" here. But this is very similar to 7db118303a (unpack_trees: fix breakage when o->src_index != o->dst_index - 2018-04-23) and could potentially break things again...
--- unpack-trees.c | 45 ++++++++++++++++++++++++++------------------- 1 file changed, 26 insertions(+), 19 deletions(-)
unpack-trees.c +26 -19
@@ -18,6 +18,9 @@#include "fsmonitor.h" #include "fetch-object.h" +/* Do not use the_index here, you probably want o->src_index */ +#define the_index the_index_should_not_be_used here +/* * Error messages expected by scripts out of plumbing commands such as * read-tree. Non-scripted Porcelain is not required to use these messages
@@ -1085,13 +1088,15 @@ static int unpack_callback(int n, unsigned long mask, unsigned long dirmask, strreturn mask; } -static int clear_ce_flags_1 (struct cache_entry **cache,int nr, +static int clear_ce_flags_1 (struct index_state *istate, +struct cache_entry **cache,int nr,struct strbuf *prefix,int select_mask,int clear_mask,struct exclude_list *el,int defval);/* Whole directory matching */ -static int clear_ce_flags_dir(struct cache_entry **cache, int nr, +static int clear_ce_flags_dir(struct index_state *istate, + struct cache_entry **cache, int nr, struct strbuf *prefix, char *basename, int select_mask, int clear_mask,
@@ -1100,7 +1105,7 @@ static int clear_ce_flags_dir(struct cache_entry **cache, int nr,struct cache_entry **cache_end;int dtype =DT_DIR ;int ret =is_excluded_from_list (prefix->buf, prefix->len, - basename, &dtype, el, &the_index); + basename, &dtype, el, istate);int rc;strbuf_addch (prefix,'/' );
@@ -1122,7 +1127,7 @@ static int clear_ce_flags_dir(struct cache_entry **cache, int nr,* calling clear_ce_flags_1(). That function will call * the expensive is_excluded_from_list() on every entry. */ - rc =clear_ce_flags_1 (cache, cache_end - cache, + rc =clear_ce_flags_1 (istate, cache, cache_end - cache, prefix, select_mask, clear_mask, el, ret);
@@ -1145,7 +1150,8 @@ static int clear_ce_flags_dir(struct cache_entry **cache, int nr,* cache[0]->name[0..(prefix_len-1)] * Top level path has prefix_len zero. */ -static int clear_ce_flags_1 (struct cache_entry **cache,int nr, +static int clear_ce_flags_1 (struct index_state *istate, +struct cache_entry **cache,int nr,struct strbuf *prefix,int select_mask,int clear_mask,struct exclude_list *el,int defval)
@@ -1179,7 +1185,7 @@ static int clear_ce_flags_1(struct cache_entry **cache, int nr, len = slash - name;strbuf_add (prefix, name, len); - processed = clear_ce_flags_dir(cache, cache_end - cache, + processed = clear_ce_flags_dir(istate, cache, cache_end - cache, prefix, prefix->buf + prefix->len - len, select_mask, clear_mask,
@@ -1193,7 +1199,7 @@ static int clear_ce_flags_1(struct cache_entry **cache, int nr, }strbuf_addch (prefix,'/' ); - cache +=clear_ce_flags_1 (cache, cache_end - cache, + cache +=clear_ce_flags_1 (istate, cache, cache_end - cache, prefix, select_mask, clear_mask, el, defval);strbuf_setlen (prefix, prefix->len - len -1 );
@@ -1203,7 +1209,7 @@ static int clear_ce_flags_1(struct cache_entry **cache, int nr,/* Non-directory */ dtype =ce_to_dtype (ce); ret =is_excluded_from_list (ce->name,ce_namelen (ce), - name, &dtype, el, &the_index); + name, &dtype, el, istate);if (ret <0 ) ret = defval;if (ret >0 )
@@ -1213,15 +1219,17 @@ static int clear_ce_flags_1(struct cache_entry **cache, int nr,return nr - (cache_end - cache); } -static int clear_ce_flags (struct cache_entry **cache,int nr, -int select_mask,int clear_mask, -struct exclude_list *el) +static int clear_ce_flags (struct index_state *istate, +int select_mask,int clear_mask, +struct exclude_list *el) {static struct strbuf prefix =STRBUF_INIT ;strbuf_reset (&prefix); -return clear_ce_flags_1 (cache, nr, +return clear_ce_flags_1 (istate, + istate->cache, + istate->cache_nr, &prefix, select_mask, clear_mask, el,0 );
@@ -1231,7 +1239,7 @@ static int clear_ce_flags(struct cache_entry **cache, int nr,* Set/Clear CE_NEW_SKIP_WORKTREE according to $GIT_DIR/info/sparse-checkout */ static void mark_new_skip_worktree (struct exclude_list *el, -struct index_state *the_index, +struct index_state *istate,int select_flag,int skip_wt_flag) {int i;
@@ -1240,8 +1248,8 @@ static void mark_new_skip_worktree(struct exclude_list *el,* 1. Pretend the narrowest worktree: only unmerged entries * are checked out */ -for (i =0 ; i < the_index->cache_nr; i++) { -struct cache_entry *ce = the_index->cache[i]; +for (i =0 ; i < istate->cache_nr; i++) { +struct cache_entry *ce = istate->cache[i];if (select_flag && !(ce->ce_flags & select_flag))continue ;
@@ -1256,8 +1264,7 @@ static void mark_new_skip_worktree(struct exclude_list *el,* 2. Widen worktree according to sparse-checkout file. * Matched entries will have skip_wt_flag cleared (i.e. "in") */ -clear_ce_flags (the_index->cache, the_index->cache_nr, - select_flag, skip_wt_flag, el); +clear_ce_flags (istate, select_flag, skip_wt_flag, el); } static int verify_absent(const struct cache_entry *,
@@ -1636,7 +1643,7 @@ static int verify_clean_subdirectory(const struct cache_entry *ce,memset (&d,0 ,sizeof (d));if (o->dir) d.exclude_per_dir = o->dir->exclude_per_dir; - i =read_directory (&d, &the_index, pathbuf, namelen+1 ,NULL ); + i =read_directory (&d, o->src_index, pathbuf, namelen+1 ,NULL );if (i)return o->gently ?-1 :add_rejected_path (o,ERROR_NOT_UPTODATE_DIR , ce->name);
@@ -1678,7 +1685,7 @@ static int check_ok_to_remove(const char *name, int len, int dtype,return 0 ;if (o->dir && -is_excluded (o->dir, &the_index, name, &dtype)) +is_excluded (o->dir, o->src_index, name, &dtype))/* * ce->name is explicitly excluded, so it is Ok to * overwrite it.
Signature · 1 line
2.18.0.rc0.309.g77c7720784
Hi,
Quoted 12 lines · Nguyễn Thái Ngọc Duy
unpack-trees code works on multiple indexes specified in
unpack_trees_options. Although they normally all refer tothe_indexat the call site, that is the caller's business.unpack-trees.cshould not make any assumption about that and should use the correct index field inunpack_trees_options.This patch is actually confusing because sometimes the function parameter is also named "
the_index" while some other times "the_index" is the global variable because the function just does not have a parameter of the same name! The only subtle difference is that the function parameter is a pointer while the global one is not.
Quoted 5 lines
This is more of a bug report than an actual fix because I'm not sure if "
o->src_index" is always the correct answer instead of "the_index" here. But this is very similar to7db118303a(unpack_trees: fix breakage wheno->src_index!=o->dst_index- 2018-04-23) and could potentially break things again...
Actually, I don't think the patch will break anything in the current code. Currently, all callers of unpack_trees() (even merge recursive which uses different src_index and dst_index now) set src_index = &the_index. So, these changes should continue to work as before (with a minor possible exception of merge-recursive calling into other functions from unpack-trees.c after unpack_trees() has finished..). That's not to say that your patch is bug free, just that I think any bugs shouldn't be triggerable from the current codebase.
Also, if any of the changes you made are wrong, what was there before was also clearly wrong. So I think we're at least no worse off.
But, I agree, it's not easy to verify what the correct thing should be in all cases. I'll try to take a closer look in the next couple days.
Quoted 11 lines
Signed-off-by Nguyễn Thái Ngọc Duy <[email protected]>--- unpack-trees.c | 45 ++++++++++++++++++++++++++------------------- 1 file changed, 26 insertions(+), 19 deletions(-)unpack-trees.c +0
-0
unpack-trees.c line 22
Quoted 5 lines
unpack-trees.c +2
-0@@ -18,6 +18,9 @@#include "fsmonitor.h" #include "fetch-object.h" +/* Do not use the_index here, you probably want o->src_index */ +#define the_index the_index_should_not_be_used here
Good call.
Quoted 4 lines · Elijah Newren
unpack-trees.c line 22
Quoted 2 lines
unpack-trees.c +2
-0+/* Do not use the_index here, you probably want o->src_index */ +#define the_index the_index_should_not_be_used hereGood call.
Is the space instead of the underscore between the last two words intentional?
Quoted 8 lines · Stefan Beller
Quoted 4 lines · Elijah Newren
unpack-trees.c line 22
Quoted 2 lines
unpack-trees.c +2
-0+/* Do not use the_index here, you probably want o->src_index */ +#define the_index the_index_should_not_be_used hereGood call.
Is the space instead of the underscore between the last two words intentional?
Unintentional. I wanted gcc to show this phrase when a developer accidentally uses the_index here (it's basically "the_index_should_not_be_used_here is not declared blah blah"). Two words would cause a different syntax error.
Signature · 1 line
Duy
Quoted 21 lines · Elijah Newren
Hi,
Quoted 12 lines · Nguyễn Thái Ngọc Duy
unpack-trees code works on multiple indexes specified in
unpack_trees_options. Although they normally all refer tothe_indexat the call site, that is the caller's business.unpack-trees.cshould not make any assumption about that and should use the correct index field inunpack_trees_options.This patch is actually confusing because sometimes the function parameter is also named "
the_index" while some other times "the_index" is the global variable because the function just does not have a parameter of the same name! The only subtle difference is that the function parameter is a pointer while the global one is not.Quoted 5 lines
This is more of a bug report than an actual fix because I'm not sure if "
o->src_index" is always the correct answer instead of "the_index" here. But this is very similar to7db118303a(unpack_trees: fix breakage wheno->src_index!=o->dst_index- 2018-04-23) and could potentially break things again...
Quoted 8 lines
Actually, I don't think the patch will break anything in the current code. Currently, all callers of
unpack_trees()(even merge recursive which uses differentsrc_indexanddst_indexnow) setsrc_index=&the_index. So, these changes should continue to work as before (with a minor possible exception of merge-recursive calling into other functions fromunpack-trees.cafterunpack_trees()has finished..). That's not to say that your patch is bug free, just that I think any bugs shouldn't be triggerable from the current codebase.
Ah.. I thought merge-recursive would do fancier things and used some temporary index. Good to know.
Quoted 5 lines
Also, if any of the changes you made are wrong, what was there before was also clearly wrong. So I think we're at least no worse off.
But, I agree, it's not easy to verify what the correct thing should be in all cases. I'll try to take a closer look in the next couple days.
Thanks. I will also stare at this code some more in the next couple days trying to remember what these functions do.
Signature · 1 line
Duy
Quoted 7 lines · Duy Nguyen
Quoted 6 lines · Elijah Newren
Quoted 5 lines · Nguyễn Thái Ngọc Duy
This is more of a bug report than an actual fix because I'm not sure if "
o->src_index" is always the correct answer instead of "the_index" here. But this is very similar to7db118303a(unpack_trees: fix breakage wheno->src_index!=o->dst_index- 2018-04-23) and could potentially break things again...
I'm pretty sure your patch is correct. Adding Brandon Williams to the cc for comment since his patches came up in the analysis below...
Quoted 11 lines
Quoted 8 lines
Actually, I don't think the patch will break anything in the current code. Currently, all callers of
unpack_trees()(even merge recursive which uses differentsrc_indexanddst_indexnow) setsrc_index=&the_index. So, these changes should continue to work as before (with a minor possible exception of merge-recursive calling into other functions fromunpack-trees.cafterunpack_trees()has finished..). That's not to say that your patch is bug free, just that I think any bugs shouldn't be triggerable from the current codebase.Ah.. I thought merge-recursive would do fancier things and used some temporary index. Good to know.
Well, it does does use a temporary index, but for dst_index rather than src_index. It then does some fancier things, but not until the call to unpack_trees() is over. In particular, at that point, it swaps the_index and tmp_index, reversing their roles so that now tmp_index is the original index and the_index becomes the result after unpack_trees() is run. That's done because I later want to use the original index for calling verify_uptodate(). verify_uptodate() is then used for was_tracked_and_matches() and was_tracked().
Anyway, the whole upshot of this is:
* merge-recursive uses src_index == &the_index for the unpack_trees() call.
* merge-recursive uses src_index == o->orig_index for subsequent
calls to verify_uptodate(), but verify_uptodate() doesn't call into any of the sites you have modified.
Further:
* Every other existing caller of unpack-trees in the code setssrc_index == &the_index, so this won't break any of them.
* There are only two callers in the codebase that set dst_index to
something other than &the_index -- diff-lib.c (which sets it to NULL) and merge-recursive (which does the stuff described above).
So, having done that analysis, I am now pretty convinced your patch won't break anything. That's one half...
Quoted 8 lines
Quoted 5 lines
Also, if any of the changes you made are wrong, what was there before was also clearly wrong. So I think we're at least no worse off.
But, I agree, it's not easy to verify what the correct thing should be in all cases. I'll try to take a closer look in the next couple days.
Thanks. I will also stare at this code some more in the next couple days trying to remember what these functions do.
Your patch has two divisible parts:
1) Your modifications to
* clear_ce_flags_1()
* clear_ce_flags_dir()
* clear_ce_flags()
* mark_new_skip_worktree()
The clear_ce_flags*() functions are only called by each other and by mark_new_skip_worktree(), which in turn is only called from unpack_trees(). Also, in all of these, your change ends up only modifying what index_state is passed to is_excluded_from_list().
2) Your modifications to
* verify_clean_subdirectory()
* check_ok_to_remove()
In this case, the former is only called by the latter, and the latter ends up only being called (via verify_absent_1()) from verify_absent() and verify_absent_sparse().
I'll address each, in reverse order.
2) The stuff that affects verify_absent()
While verify_absent() is not called from merge-recursive right now, it was something I wanted to use in the future for very similar reasons that verify_uptodate() started being called directly from merge-recursive. In particular, if the rewrite of merge-recursive[A] I want to do sets index_only when calling unpack_trees(), then does the whole merge without touching the worktree, then at the end goes to update the working tree, it will need to do extra checks. verify_absent() will come in handy as one of those extra checks. For that case, using the_index (the new index just created with lots of changes) would be wrong in all the same ways that using the_index caused massive problems for was_tracked() in merge-recursive (e.g. the blow up of when Junio merged the original directory rename detection series into master and subsequently reverted it); we'd instead want src_index (which was the index that existed when merge was called) instead. So, with this patch you've fixed some important bugs that I would have hit later.
[A] sidenote: see
https://public-inbox.org/git/[email protected]/
for more details
1) mark_new_skip_worktree() ... is_excluded_from_list().
Sadly, I'm not very familiar with the skip_worktree and sparse checkout stuff. However, the fact that mark_new_skip_worktree() explicitly takes an index_state (and a different one is passed to it the two different times it is called), and that it is the only caller of the clear_ce_flags*() family of functions, and that those function use the cache entries from the index passed to them in all cases other than the calls to is_excluded_from_list() makes those two look like oversights. In fact, a little more digging turns up commit
fba92be8f7 ("dir: convert is_excluded_from_list to take an index",
2017-05-05)
and before then, those functions didn't use the_index directly. But they did use it indirectly, because they called a function in dir.c that had it hardcoded. So it looks like Brandon fixed part of the bug for us, but moved the incorrect hardcoding from dir.c to unpack-trees.c. Your patch is just fixing it up. In fact, a little more digging turns up:
2c1eb10454 ("dir: convert read_directory to take an index", 2017-05-05) a0bba65b10 ("dir: convert is_excluded to take an index", 2017-05-05)
which appear to be the culprits behind the other two uses of the_index called from verify_absent(). It looks like before these commits that unpack_trees() was carefully using the appropriate index, except that functions in dir.c had use of the_index hardcoded. Brandon fix the functions in dir.c for us, but ended up still hardcoding the_index in unpack-trees.c. Basically, he did most of the necessary lifting, and your patch just finally changes them over to use the correct index.
Brandon: Does anything look off in my analysis above?
Quoted 132 lines · Elijah Newren
Quoted 7 lines · Duy Nguyen
Quoted 6 lines · Elijah Newren
Quoted 5 lines · Nguyễn Thái Ngọc Duy
This is more of a bug report than an actual fix because I'm not sure if "
o->src_index" is always the correct answer instead of "the_index" here. But this is very similar to7db118303a(unpack_trees: fix breakage wheno->src_index!=o->dst_index- 2018-04-23) and could potentially break things again...I'm pretty sure your patch is correct. Adding Brandon Williams to the cc for comment since his patches came up in the analysis below...
Quoted 11 lines
Quoted 8 lines
Actually, I don't think the patch will break anything in the current code. Currently, all callers of
unpack_trees()(even merge recursive which uses differentsrc_indexanddst_indexnow) setsrc_index=&the_index. So, these changes should continue to work as before (with a minor possible exception of merge-recursive calling into other functions fromunpack-trees.cafterunpack_trees()has finished..). That's not to say that your patch is bug free, just that I think any bugs shouldn't be triggerable from the current codebase.Ah.. I thought merge-recursive would do fancier things and used some temporary index. Good to know.
Well, it does does use a temporary index, but for
dst_indexrather thansrc_index. It then does some fancier things, but not until the call tounpack_trees()is over. In particular, at that point, it swapsthe_indexandtmp_index, reversing their roles so that nowtmp_indexis the original index andthe_indexbecomes the result afterunpack_trees()is run. That's done because I later want to use the original index for callingverify_uptodate().verify_uptodate()is then used forwas_tracked_and_matches()andwas_tracked().Anyway, the whole upshot of this is:
* merge-recursive usessrc_index==&the_indexfor theunpack_trees()call.
* merge-recursive usessrc_index==o->orig_indexfor subsequent
calls toverify_uptodate(), butverify_uptodate()doesn't call into any of the sites you have modified.Further:
* Every other existing caller of unpack-trees in the code setssrc_index==&the_index, so this won't break any of them.
* There are only two callers in the codebase that setdst_indexto
something other than&the_index--diff-lib.c(which sets it to NULL) and merge-recursive (which does the stuff described above).So, having done that analysis, I am now pretty convinced your patch won't break anything. That's one half...
Quoted 8 lines
Quoted 5 lines
Also, if any of the changes you made are wrong, what was there before was also clearly wrong. So I think we're at least no worse off.
But, I agree, it's not easy to verify what the correct thing should be in all cases. I'll try to take a closer look in the next couple days.
Thanks. I will also stare at this code some more in the next couple days trying to remember what these functions do.
Your patch has two divisible parts:
1) Your modifications to
*clear_ce_flags_1()
*clear_ce_flags_dir()
*clear_ce_flags()
*mark_new_skip_worktree()
The clear_ce_flags*() functions are only called by each other and bymark_new_skip_worktree(), which in turn is only called fromunpack_trees(). Also, in all of these, your change ends up only modifying whatindex_stateis passed tois_excluded_from_list().2) Your modifications to
*verify_clean_subdirectory()
*check_ok_to_remove()
In this case, the former is only called by the latter, and the latter ends up only being called (viaverify_absent_1()) fromverify_absent()andverify_absent_sparse().I'll address each, in reverse order.
2) The stuff that affects
verify_absent()While
verify_absent()is not called from merge-recursive right now, it was something I wanted to use in the future for very similar reasons thatverify_uptodate()started being called directly from merge-recursive. In particular, if the rewrite of merge-recursive[A] I want to do setsindex_onlywhen callingunpack_trees(), then does the whole merge without touching the worktree, then at the end goes to update the working tree, it will need to do extra checks.verify_absent()will come in handy as one of those extra checks. For that case, usingthe_index(the new index just created with lots of changes) would be wrong in all the same ways that usingthe_indexcaused massive problems forwas_tracked()in merge-recursive (e.g. the blow up of when Junio merged the original directory rename detection series into master and subsequently reverted it); we'd instead wantsrc_index(which was the index that existed when merge was called) instead. So, with this patch you've fixed some important bugs that I would have hit later.[A] sidenote: see
https://public-inbox.org/git/[email protected]/
for more details1)
mark_new_skip_worktree()...is_excluded_from_list().Sadly, I'm not very familiar with the
skip_worktreeand sparse checkout stuff. However, the fact thatmark_new_skip_worktree()explicitly takes anindex_state(and a different one is passed to it the two different times it is called), and that it is the only caller of the clear_ce_flags*() family of functions, and that those function use the cache entries from the index passed to them in all cases other than the calls tois_excluded_from_list()makes those two look like oversights. In fact, a little more digging turns up commit
fba92be8f7("dir: convertis_excluded_from_listto take an index",
2017-05-05)
and before then, those functions didn't usethe_indexdirectly. But they did use it indirectly, because they called a function indir.cthat had it hardcoded. So it looks like Brandon fixed part of the bug for us, but moved the incorrect hardcoding fromdir.ctounpack-trees.c. Your patch is just fixing it up. In fact, a little more digging turns up:
2c1eb10454("dir: convertread_directoryto take an index", 2017-05-05)a0bba65b10("dir: convertis_excludedto take an index", 2017-05-05)which appear to be the culprits behind the other two uses of
the_indexcalled fromverify_absent(). It looks like before these commits thatunpack_trees()was carefully using the appropriate index, except that functions indir.chad use ofthe_indexhardcoded. Brandon fix the functions indir.cfor us, but ended up still hardcodingthe_indexinunpack-trees.c. Basically, he did most of the necessary lifting, and your patch just finally changes them over to use the correct index.
Quoted 1 line
Brandon: Does anything look off in my analysis above?
Nope your analysis is correct with regards to the changes I made to dir. Back when I was converting ls-files and grep to recursively work on submodules I needed to make it so that some of the dir functions explicitly accepted an index parameter instead of implicitly relying on the_index. So I bubbled up the hardcoded the_index to the places I wasn't passing in a specific index (which would be when the_index appeared in unpack-trees.c) and allowed for arbitrary index's to be passed in like in ls-files.
This is one of the larger issues with working in our codebase, implicitly relying on global state. It makes it very difficult to reason if a section of code is correct or not, as is in this case. Is it a bug that we use o->src_index in some places and the_index in others? Well maybe not, but that's because they usually are the same thing in the few cases that it matters (at least now its a bit more explicit to see this, whereas before it was very implicit). Of course I know stefan has done work with making unpack-trees recursive so maybe it can be a bug in the recursive case.
Signature · 1 line
Brandon Williams
cover Fix "the_index" usage in unpack-trees.c
In reply to Nguyễn Thái Ngọc DuyThis is a potential problem that is exposed after Brandon kicked the_index out of dir.c (big thanks!) and could be seen as a continuation of bw/dir-c-stops-relying-on-the-index. Also thanks to Elijah for helping analyze this code.
The last patch may be debatable. If we go this route, we may end up with plenty of "#define NO_THIS" and "#define NO_THAT" until we finally achieve "#define THIS_IS_TRUE_LIB_CODE" many years later.
Nguyễn Thái Ngọc Duy (6):
unpack-trees: remove 'extern' on function declaration
unpack-trees: add a note about path invalidation
unpack-trees: don't shadow global var the_index
unpack-tress: convert clear_ce_flags* to avoid the_index
unpack-trees: avoid the_index in verify_absent() Forbid "the_index" in dir.c and unpack-trees.c
cache.h | 2 ++ dir.c | 2 ++ unpack-trees.c | 55 +++++++++++++++++++++++++++++++++----------------- unpack-trees.h | 4 ++-- 4 files changed, 42 insertions(+), 21 deletions(-)
Signature · 1 line
2.18.0.rc0.333.g22e6ee6cdf
PATCH 1/6 unpack-trees: remove 'extern' on function declaration
--- unpack-trees.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
unpack-trees.h +2 -2
@@ -82,8 +82,8 @@ struct unpack_trees_options {struct exclude_list *el;/* for internal use */ }; -extern int unpack_trees (unsigned n,struct tree_desc *t, -struct unpack_trees_options *options); +int unpack_trees (unsigned n,struct tree_desc *t, +struct unpack_trees_options *options);int verify_uptodate (const struct cache_entry *ce,struct unpack_trees_options *o);
Signature · 1 line
2.18.0.rc0.333.g22e6ee6cdf
PATCH 2/6 unpack-trees: add a note about path invalidation
In reply to Nguyễn Thái Ngọc Duy--- unpack-trees.c | 11 +++++++++++ 1 file changed, 11 insertions(+)
unpack-trees.c +11 -0
@@ -1545,6 +1545,17 @@ static int verify_uptodate_sparse(const struct cache_entry *ce,return verify_uptodate_1 (ce, o,ERROR_SPARSE_NOT_UPTODATE_FILE ); } +/* +* TODO: We should actually invalidate o->result, not src_index [1]. +* But since cache tree and untracked cache both are not copied to +* o->result until unpacking is complete, we invalidate them on +* src_index instead with the assumption that they will be copied to +* dst_index at the end. +* +* [1] src_index->cache_tree is also used in unpack_callback() so if +* we invalidate o->result, we need to update it to use +* o->result.cache_tree as well. +*/ static void invalidate_ce_path (const struct cache_entry *ce,struct unpack_trees_options *o) {
Signature · 1 line
2.18.0.rc0.333.g22e6ee6cdf
PATCH 3/6 unpack-trees: don't shadow global var the_index
In reply to Nguyễn Thái Ngọc DuyThis function mark_new_skip_worktree() has an argument named the_index which is also the name of a global variable. While they have different types (the global the_index is not a pointer) mistakes can easily happen and it's also confusing for readers. Rename the function argument to something other than the_index.
--- unpack-trees.c | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-)
unpack-trees.c +4 -5
@@ -1231,7 +1231,7 @@ static int clear_ce_flags(struct cache_entry **cache, int nr,* Set/Clear CE_NEW_SKIP_WORKTREE according to $GIT_DIR/info/sparse-checkout */ static void mark_new_skip_worktree (struct exclude_list *el, -struct index_state *the_index, +struct index_state *istate,int select_flag,int skip_wt_flag) {int i;
@@ -1240,8 +1240,8 @@ static void mark_new_skip_worktree(struct exclude_list *el,* 1. Pretend the narrowest worktree: only unmerged entries * are checked out */ -for (i =0 ; i < the_index->cache_nr; i++) { -struct cache_entry *ce = the_index->cache[i]; +for (i =0 ; i < istate->cache_nr; i++) { +struct cache_entry *ce = istate->cache[i];if (select_flag && !(ce->ce_flags & select_flag))continue ;
@@ -1256,8 +1256,7 @@ static void mark_new_skip_worktree(struct exclude_list *el,* 2. Widen worktree according to sparse-checkout file. * Matched entries will have skip_wt_flag cleared (i.e. "in") */ -clear_ce_flags (the_index->cache, the_index->cache_nr, - select_flag, skip_wt_flag, el); +clear_ce_flags (istate, select_flag, skip_wt_flag, el); } static int verify_absent(const struct cache_entry *,
Signature · 1 line
2.18.0.rc0.333.g22e6ee6cdf
Re: [PATCH 3/6] unpack-trees: don't shadow global var the_index
Quoted 36 lines · Nguyễn Thái Ngọc Duy
This function
mark_new_skip_worktree()has an argument namedthe_indexwhich is also the name of a global variable. While they have different types (the globalthe_indexis not a pointer) mistakes can easily happen and it's also confusing for readers. Rename the function argument to something other thanthe_index.Signed-off-by Nguyễn Thái Ngọc Duy <[email protected]>--- unpack-trees.c | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-)unpack-trees.c +3
-3@@ -1231,7 +1231,7 @@ static int clear_ce_flags(struct cache_entry **cache, int nr,* Set/Clear CE_NEW_SKIP_WORKTREE according to $GIT_DIR/info/sparse-checkout */ static void mark_new_skip_worktree (struct exclude_list *el, -struct index_state *the_index, +struct index_state *istate,int select_flag,int skip_wt_flag) {int i;@@ -1240,8 +1240,8 @@ static void mark_new_skip_worktree(struct exclude_list *el,* 1. Pretend the narrowest worktree: only unmerged entries * are checked out */ -for (i =0 ; i < the_index->cache_nr; i++) { -struct cache_entry *ce = the_index->cache[i]; +for (i =0 ; i < istate->cache_nr; i++) { +struct cache_entry *ce = istate->cache[i];if (select_flag && !(ce->ce_flags & select_flag))continue ;
unpack-trees.c static void mark_new_skip_worktree(struct exclude_list *el, · line 1259
Quoted 6 lines
unpack-trees.c +1
-2@@ -1256,8 +1256,7 @@ static void mark_new_skip_worktree(struct exclude_list *el,* 2. Widen worktree according to sparse-checkout file. * Matched entries will have skip_wt_flag cleared (i.e. "in") */ -clear_ce_flags (the_index->cache, the_index->cache_nr, - select_flag, skip_wt_flag, el); +clear_ce_flags (istate, select_flag, skip_wt_flag, el);
This looks a bit suspect. The clear_ce_flags() function has not been modified to take a 'struct index_state *' as its first parameter, right? (If you look back at the first hunk header, you will see that it still takes 'struct cache_entry **, int, ...') ;-)
ATB,
Ramsay Jones
PATCH 4/6 unpack-tress: convert clear_ce_flags* to avoid the_index
In reply to Nguyễn Thái Ngọc DuyPrior to fba92be8f7, this code implicitly (and incorrectly) assumes the_index when running the exclude machinery. fba92be8f7 helps show this problem clearer because unpack-trees operation is supposed to work on whatever index the caller specifies... not specifically the_index.
Update the code to use "istate" argument that's originally from mark_new_skip_worktree(). From the call sites, both in unpack_trees(), you can see that this function works on two separate indexes:o->src_index and o->result. The second mark_new_skip_worktree() so far has incorecctly applied exclude rules on o->src_index instead of o->result. It's unclear what is the consequences of this, but it's definitely wrong.
[1] fba92be8f7 (dir: convert is_excluded_from_list to take an index -
2017-05-05)
--- unpack-trees.c | 29 +++++++++++++++++------------ 1 file changed, 17 insertions(+), 12 deletions(-)
unpack-trees.c +17 -12
@@ -1085,13 +1085,15 @@ static int unpack_callback(int n, unsigned long mask, unsigned long dirmask, strreturn mask; } -static int clear_ce_flags_1 (struct cache_entry **cache,int nr, +static int clear_ce_flags_1 (struct index_state *istate, +struct cache_entry **cache,int nr,struct strbuf *prefix,int select_mask,int clear_mask,struct exclude_list *el,int defval);/* Whole directory matching */ -static int clear_ce_flags_dir(struct cache_entry **cache, int nr, +static int clear_ce_flags_dir(struct index_state *istate, + struct cache_entry **cache, int nr, struct strbuf *prefix, char *basename, int select_mask, int clear_mask,
@@ -1100,7 +1102,7 @@ static int clear_ce_flags_dir(struct cache_entry **cache, int nr,struct cache_entry **cache_end;int dtype =DT_DIR ;int ret =is_excluded_from_list (prefix->buf, prefix->len, - basename, &dtype, el, &the_index); + basename, &dtype, el, istate);int rc;strbuf_addch (prefix,'/' );
@@ -1122,7 +1124,7 @@ static int clear_ce_flags_dir(struct cache_entry **cache, int nr,* calling clear_ce_flags_1(). That function will call * the expensive is_excluded_from_list() on every entry. */ - rc =clear_ce_flags_1 (cache, cache_end - cache, + rc =clear_ce_flags_1 (istate, cache, cache_end - cache, prefix, select_mask, clear_mask, el, ret);
@@ -1145,7 +1147,8 @@ static int clear_ce_flags_dir(struct cache_entry **cache, int nr,* cache[0]->name[0..(prefix_len-1)] * Top level path has prefix_len zero. */ -static int clear_ce_flags_1 (struct cache_entry **cache,int nr, +static int clear_ce_flags_1 (struct index_state *istate, +struct cache_entry **cache,int nr,struct strbuf *prefix,int select_mask,int clear_mask,struct exclude_list *el,int defval)
@@ -1179,7 +1182,7 @@ static int clear_ce_flags_1(struct cache_entry **cache, int nr, len = slash - name;strbuf_add (prefix, name, len); - processed = clear_ce_flags_dir(cache, cache_end - cache, + processed = clear_ce_flags_dir(istate, cache, cache_end - cache, prefix, prefix->buf + prefix->len - len, select_mask, clear_mask,
@@ -1193,7 +1196,7 @@ static int clear_ce_flags_1(struct cache_entry **cache, int nr, }strbuf_addch (prefix,'/' ); - cache +=clear_ce_flags_1 (cache, cache_end - cache, + cache +=clear_ce_flags_1 (istate, cache, cache_end - cache, prefix, select_mask, clear_mask, el, defval);strbuf_setlen (prefix, prefix->len - len -1 );
@@ -1203,7 +1206,7 @@ static int clear_ce_flags_1(struct cache_entry **cache, int nr,/* Non-directory */ dtype =ce_to_dtype (ce); ret =is_excluded_from_list (ce->name,ce_namelen (ce), - name, &dtype, el, &the_index); + name, &dtype, el, istate);if (ret <0 ) ret = defval;if (ret >0 )
@@ -1213,15 +1216,17 @@ static int clear_ce_flags_1(struct cache_entry **cache, int nr,return nr - (cache_end - cache); } -static int clear_ce_flags (struct cache_entry **cache,int nr, -int select_mask,int clear_mask, -struct exclude_list *el) +static int clear_ce_flags (struct index_state *istate, +int select_mask,int clear_mask, +struct exclude_list *el) {static struct strbuf prefix =STRBUF_INIT ;strbuf_reset (&prefix); -return clear_ce_flags_1 (cache, nr, +return clear_ce_flags_1 (istate, + istate->cache, + istate->cache_nr, &prefix, select_mask, clear_mask, el,0 );
Signature · 1 line
2.18.0.rc0.333.g22e6ee6cdf
Re: [PATCH 4/6] unpack-tress: convert clear_ce_flags* to avoid the_index
Quoted 100 lines · Nguyễn Thái Ngọc Duy
Prior to
fba92be8f7, this code implicitly (and incorrectly) assumesthe_indexwhen running the exclude machinery.fba92be8f7helps show this problem clearer because unpack-trees operation is supposed to work on whatever index the caller specifies... not specificallythe_index.Update the code to use "istate" argument that's originally from
mark_new_skip_worktree(). From the call sites, both inunpack_trees(), you can see that this function works on two separate indexes:o->src_indexando->result. The secondmark_new_skip_worktree()so far has incorecctly applied exclude rules ono->src_indexinstead ofo->result. It's unclear what is the consequences of this, but it's definitely wrong.[1]
fba92be8f7(dir: convertis_excluded_from_listto take an index -2017-05-05)Signed-off-by Nguyễn Thái Ngọc Duy <[email protected]>--- unpack-trees.c | 29 +++++++++++++++++------------ 1 file changed, 17 insertions(+), 12 deletions(-)unpack-trees.c +11
-8@@ -1085,13 +1085,15 @@ static int unpack_callback(int n, unsigned long mask, unsigned long dirmask, strreturn mask; } -static int clear_ce_flags_1 (struct cache_entry **cache,int nr, +static int clear_ce_flags_1 (struct index_state *istate, +struct cache_entry **cache,int nr,struct strbuf *prefix,int select_mask,int clear_mask,struct exclude_list *el,int defval);/* Whole directory matching */ -static int clear_ce_flags_dir(struct cache_entry **cache, int nr, +static int clear_ce_flags_dir(struct index_state *istate, + struct cache_entry **cache, int nr, struct strbuf *prefix, char *basename, int select_mask, int clear_mask,@@ -1100,7 +1102,7 @@ static int clear_ce_flags_dir(struct cache_entry **cache, int nr,struct cache_entry **cache_end;int dtype =DT_DIR ;int ret =is_excluded_from_list (prefix->buf, prefix->len, - basename, &dtype, el, &the_index); + basename, &dtype, el, istate);int rc;strbuf_addch (prefix,'/' );@@ -1122,7 +1124,7 @@ static int clear_ce_flags_dir(struct cache_entry **cache, int nr,* calling clear_ce_flags_1(). That function will call * the expensive is_excluded_from_list() on every entry. */ - rc =clear_ce_flags_1 (cache, cache_end - cache, + rc =clear_ce_flags_1 (istate, cache, cache_end - cache, prefix, select_mask, clear_mask, el, ret);@@ -1145,7 +1147,8 @@ static int clear_ce_flags_dir(struct cache_entry **cache, int nr,* cache[0]->name[0..(prefix_len-1)] * Top level path has prefix_len zero. */ -static int clear_ce_flags_1 (struct cache_entry **cache,int nr, +static int clear_ce_flags_1 (struct index_state *istate, +struct cache_entry **cache,int nr,struct strbuf *prefix,int select_mask,int clear_mask,struct exclude_list *el,int defval)@@ -1179,7 +1182,7 @@ static int clear_ce_flags_1(struct cache_entry **cache, int nr, len = slash - name;strbuf_add (prefix, name, len); - processed = clear_ce_flags_dir(cache, cache_end - cache, + processed = clear_ce_flags_dir(istate, cache, cache_end - cache, prefix, prefix->buf + prefix->len - len, select_mask, clear_mask,@@ -1193,7 +1196,7 @@ static int clear_ce_flags_1(struct cache_entry **cache, int nr, }strbuf_addch (prefix,'/' ); - cache +=clear_ce_flags_1 (cache, cache_end - cache, + cache +=clear_ce_flags_1 (istate, cache, cache_end - cache, prefix, select_mask, clear_mask, el, defval);strbuf_setlen (prefix, prefix->len - len -1 );@@ -1203,7 +1206,7 @@ static int clear_ce_flags_1(struct cache_entry **cache, int nr,/* Non-directory */ dtype =ce_to_dtype (ce); ret =is_excluded_from_list (ce->name,ce_namelen (ce), - name, &dtype, el, &the_index); + name, &dtype, el, istate);if (ret <0 ) ret = defval;if (ret >0 )
unpack-trees.c static int clear_ce_flags_1(struct cache_entry **cache, int nr, · line 1221
Quoted 9 lines
unpack-trees.c +3
-3@@ -1213,15 +1216,17 @@ static int clear_ce_flags_1(struct cache_entry **cache, int nr,return nr - (cache_end - cache); } -static int clear_ce_flags (struct cache_entry **cache,int nr, -int select_mask,int clear_mask, -struct exclude_list *el) +static int clear_ce_flags (struct index_state *istate, +int select_mask,int clear_mask, +struct exclude_list *el)
OK, so that last hunk of patch #3 should be moved to the end of this patch. (A 'git rebase -i' problem?)
ATB,
Ramsay Jones
PATCH 5/6 unpack-trees: avoid the_index in verify_absent()
In reply to Nguyễn Thái Ngọc DuyBoth functions that are updated in this commit are called by verify_absent(), which is part of the "unpack-trees" operation that is supposed to work on any index file specified by the caller. Thanks to Brandon [1] [2], an implicit dependency on the_index is exposed. This commit fixes it.
In both functions, it makes sense to use src_index to check for exclusion because it's almost unchanged and should give us the same outcome as if running the exclude check before the unpack.
It's "almost unchanged" because we do invalidate cache-tree and untracked cache in the source index. But this should not affect how exclude machinery uses the index: to see if a file is tracked, and to read a blob from the index instead of worktree if it's marked skip-worktree (i.e. it's not available in worktree)
[1] a0bba65b10 (dir: convert is_excluded to take an index - 2017-05-05
[2] 2c1eb10454 (dir: convert read_directory to take an index - 2017-05-05)
--- unpack-trees.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
unpack-trees.c +2 -2
@@ -1651,7 +1651,7 @@ static int verify_clean_subdirectory(const struct cache_entry *ce,memset (&d,0 ,sizeof (d));if (o->dir) d.exclude_per_dir = o->dir->exclude_per_dir; - i =read_directory (&d, &the_index, pathbuf, namelen+1 ,NULL ); + i =read_directory (&d, o->src_index, pathbuf, namelen+1 ,NULL );if (i)return o->gently ?-1 :add_rejected_path (o,ERROR_NOT_UPTODATE_DIR , ce->name);
@@ -1693,7 +1693,7 @@ static int check_ok_to_remove(const char *name, int len, int dtype,return 0 ;if (o->dir && -is_excluded (o->dir, &the_index, name, &dtype)) +is_excluded (o->dir, o->src_index, name, &dtype))/* * ce->name is explicitly excluded, so it is Ok to * overwrite it.
Signature · 1 line
2.18.0.rc0.333.g22e6ee6cdf
PATCH 6/6 Forbid "the_index" in dir.c and unpack-trees.c
In reply to Nguyễn Thái Ngọc DuyUse of global variables like the_index makes it very hard to follow the code flow, especially when it's buried deep in some minor helper function.
We are gradually avoiding global variables, this hopefully will make it one step closer. The end game is, the set of "library" source files will have just one macro set: "LIB_CODE" (or something). This macro will prevent access to most (if not all) global variables in those files. We can't do that now, so we just prevent one thing at a time.
---
Should I keep my trick of defining the_index to the_index_should_not_be_used_here? It may help somewhat when people accidentally use the_index.
cache.h | 2 ++ dir.c | 2 ++ unpack-trees.c | 2 ++ 3 files changed, 6 insertions(+)
cache.h +2 -0
@@ -330,7 +330,9 @@ struct index_state {struct ewah_bitmap *fsmonitor_dirty; }; +#ifndef NO_GLOBAL_INDEX extern struct index_state the_index; +#endif /* Name hashing */ extern int test_lazy_init_name_hash (struct index_state *istate,int try_threaded);
dir.c +2 -0
@@ -8,6 +8,8 @@* Junio Hamano, 2005-2006 */ #define NO_THE_INDEX_COMPATIBILITY_MACROS +/* Do not use the_index. You should have access to it via function arg */ +#define NO_GLOBAL_INDEX #include "cache.h" #include "config.h" #include "dir.h"
unpack-trees.c +2 -0
@@ -1,4 +1,6 @@#define NO_THE_INDEX_COMPATIBILITY_MACROS +/* Do not use the_index here, you probably want o->src_index */ +#define NO_GLOBAL_INDEX #include "cache.h" #include "argv-array.h" #include "repository.h"
Signature · 1 line
2.18.0.rc0.333.g22e6ee6cdf
Re: [PATCH 6/6] Forbid "the_index" in dir.c and unpack-trees.c
Quoted 10 lines · Nguyễn Thái Ngọc Duy
Use of global variables like
the_indexmakes it very hard to follow the code flow, especially when it's buried deep in some minor helper function.We are gradually avoiding global variables, this hopefully will make it one step closer. The end game is, the set of "library" source files will have just one macro set: "
LIB_CODE" (or something). This macro will prevent access to most (if not all) global variables in those files. We can't do that now, so we just prevent one thing at a time.
Quoted 5 lines
Signed-off-by Nguyễn Thái Ngọc Duy <[email protected]>---
Should I keep my trick of definingthe_indextothe_index_should_not_be_used_here? It may help somewhat when people accidentally usethe_index.
We already have a set of index compatibility macros which this could maybe be a part of. Though if we want to go this way I think we should do the reverse of this and instead have GLOBAL_INDEX which must be defined in order to have access to the global. That way new files are already protected by this and it may be easier to reduce the number of these defines through our codebase than to add them to every single file.
Quoted 47 lines
cache.h | 2 ++ dir.c | 2 ++ unpack-trees.c | 2 ++ 3 files changed, 6 insertions(+)cache.h +2
-0@@ -330,7 +330,9 @@ struct index_state {struct ewah_bitmap *fsmonitor_dirty; }; +#ifndef NO_GLOBAL_INDEX extern struct index_state the_index; +#endif /* Name hashing */ extern int test_lazy_init_name_hash (struct index_state *istate,int try_threaded);dir.c +2
-0@@ -8,6 +8,8 @@* Junio Hamano, 2005-2006 */ #define NO_THE_INDEX_COMPATIBILITY_MACROS +/* Do not use the_index. You should have access to it via function arg */ +#define NO_GLOBAL_INDEX #include "cache.h" #include "config.h" #include "dir.h" unpack-trees.c +2
-0@@ -1,4 +1,6 @@#define NO_THE_INDEX_COMPATIBILITY_MACROS +/* Do not use the_index here, you probably want o->src_index */ +#define NO_GLOBAL_INDEX #include "cache.h" #include "argv-array.h" #include "repository.h" Signature · 1 line
2.18.0.rc0.333.g22e6ee6cdf
Signature · 1 line
Brandon Williams
Re: [PATCH 6/6] Forbid "the_index" in dir.c and unpack-trees.c
Quoted 11 lines · Brandon Williams
Quoted 10 lines · Nguyễn Thái Ngọc Duy
Use of global variables like
the_indexmakes it very hard to follow the code flow, especially when it's buried deep in some minor helper function.We are gradually avoiding global variables, this hopefully will make it one step closer. The end game is, the set of "library" source files will have just one macro set: "
LIB_CODE" (or something). This macro will prevent access to most (if not all) global variables in those files. We can't do that now, so we just prevent one thing at a time.
Quoted 8 lines
Quoted 5 lines
Signed-off-by Nguyễn Thái Ngọc Duy <[email protected]>---
Should I keep my trick of definingthe_indextothe_index_should_not_be_used_here? It may help somewhat when people accidentally usethe_index.We already have a set of index compatibility macros which this could maybe be a part of.
I like it. Only attr.c and submodule.c fail to build if I make the_index declaration part of NO_THE_INDEX_COMPATIBILITY_MACROS. So I'm going to drop this patch for now, work on kicking the_index out of submodule.c and attr.c then move the_index decl there in a separate series.
Quoted 57 lines
Though if we want to go this way I think we should
do the reverse of this and instead haveGLOBAL_INDEXwhich must be defined in order to have access to the global. That way new files are already protected by this and it may be easier to reduce the number of these defines through our codebase than to add them to every single file.Quoted 45 lines
cache.h | 2 ++ dir.c | 2 ++ unpack-trees.c | 2 ++ 3 files changed, 6 insertions(+)cache.h +2
-0@@ -330,7 +330,9 @@ struct index_state {struct ewah_bitmap *fsmonitor_dirty; }; +#ifndef NO_GLOBAL_INDEX extern struct index_state the_index; +#endif /* Name hashing */ extern int test_lazy_init_name_hash (struct index_state *istate,int try_threaded);dir.c +2
-0@@ -8,6 +8,8 @@* Junio Hamano, 2005-2006 */ #define NO_THE_INDEX_COMPATIBILITY_MACROS +/* Do not use the_index. You should have access to it via function arg */ +#define NO_GLOBAL_INDEX #include "cache.h" #include "config.h" #include "dir.h" unpack-trees.c +2
-1@@ -1,4 +1,6 @@#define NO_THE_INDEX_COMPATIBILITY_MACROS +/* Do not use the_index here, you probably want o->src_index */ +#define NO_GLOBAL_INDEX #include "cache.h" #include "argv-array.h" #include "repository.h" --Quoted 2 lines
2.18.0.rc0.333.g22e6ee6cdf
--
Brandon Williams
Signature · 1 line
Duy
v2 cover Fix "the_index" usage in unpack-trees.c
In reply to Nguyễn Thái Ngọc Duyv2 fixes an incorrect patch splitting (I should have built one more time :P) between 3/5 and 4/5. v1's 6/6 is dropped. Brandon suggested a better way of doing it which may happen later.
Nguyễn Thái Ngọc Duy (5):
unpack-trees: remove 'extern' on function declaration
unpack-trees: add a note about path invalidation
unpack-trees: don't shadow global var the_index
unpack-tress: convert clear_ce_flags* to avoid the_index
unpack-trees: avoid the_index in verify_absent()
unpack-trees.c | 53 ++++++++++++++++++++++++++++++++------------------ unpack-trees.h | 4 ++-- 2 files changed, 36 insertions(+), 21 deletions(-)
Signature · 1 line
2.18.0.rc0.333.g22e6ee6cdf
PATCH v2 1/5 unpack-trees: remove 'extern' on function declaration
--- unpack-trees.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
unpack-trees.h +2 -2
@@ -82,8 +82,8 @@ struct unpack_trees_options {struct exclude_list *el;/* for internal use */ }; -extern int unpack_trees (unsigned n,struct tree_desc *t, -struct unpack_trees_options *options); +int unpack_trees (unsigned n,struct tree_desc *t, +struct unpack_trees_options *options);int verify_uptodate (const struct cache_entry *ce,struct unpack_trees_options *o);
Signature · 1 line
2.18.0.rc0.333.g22e6ee6cdf
PATCH v2 2/5 unpack-trees: add a note about path invalidation
In reply to Nguyễn Thái Ngọc Duy--- unpack-trees.c | 11 +++++++++++ 1 file changed, 11 insertions(+)
unpack-trees.c +11 -0
@@ -1545,6 +1545,17 @@ static int verify_uptodate_sparse(const struct cache_entry *ce,return verify_uptodate_1 (ce, o,ERROR_SPARSE_NOT_UPTODATE_FILE ); } +/* +* TODO: We should actually invalidate o->result, not src_index [1]. +* But since cache tree and untracked cache both are not copied to +* o->result until unpacking is complete, we invalidate them on +* src_index instead with the assumption that they will be copied to +* dst_index at the end. +* +* [1] src_index->cache_tree is also used in unpack_callback() so if +* we invalidate o->result, we need to update it to use +* o->result.cache_tree as well. +*/ static void invalidate_ce_path (const struct cache_entry *ce,struct unpack_trees_options *o) {
Signature · 1 line
2.18.0.rc0.333.g22e6ee6cdf
PATCH v2 3/5 unpack-trees: don't shadow global var the_index
In reply to Nguyễn Thái Ngọc DuyThis function mark_new_skip_worktree() has an argument named the_index which is also the name of a global variable. While they have different types (the global the_index is not a pointer) mistakes can easily happen and it's also confusing for readers. Rename the function argument to something other than the_index.
--- unpack-trees.c | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-)
unpack-trees.c +4 -5
@@ -1231,7 +1231,7 @@ static int clear_ce_flags(struct cache_entry **cache, int nr,* Set/Clear CE_NEW_SKIP_WORKTREE according to $GIT_DIR/info/sparse-checkout */ static void mark_new_skip_worktree (struct exclude_list *el, -struct index_state *the_index, +struct index_state *istate,int select_flag,int skip_wt_flag) {int i;
@@ -1240,8 +1240,8 @@ static void mark_new_skip_worktree(struct exclude_list *el,* 1. Pretend the narrowest worktree: only unmerged entries * are checked out */ -for (i =0 ; i < the_index->cache_nr; i++) { -struct cache_entry *ce = the_index->cache[i]; +for (i =0 ; i < istate->cache_nr; i++) { +struct cache_entry *ce = istate->cache[i];if (select_flag && !(ce->ce_flags & select_flag))continue ;
@@ -1256,8 +1256,7 @@ static void mark_new_skip_worktree(struct exclude_list *el,* 2. Widen worktree according to sparse-checkout file. * Matched entries will have skip_wt_flag cleared (i.e. "in") */ -clear_ce_flags (the_index->cache, the_index->cache_nr, - select_flag, skip_wt_flag, el); +clear_ce_flags (istate->cache, istate->cache_nr, select_flag, skip_wt_flag, el); } static int verify_absent(const struct cache_entry *,
Signature · 1 line
2.18.0.rc0.333.g22e6ee6cdf
PATCH v2 4/5 unpack-tress: convert clear_ce_flags* to avoid the_index
In reply to Nguyễn Thái Ngọc DuyPrior to fba92be8f7, this code implicitly (and incorrectly) assumes the_index when running the exclude machinery. fba92be8f7 helps show this problem clearer because unpack-trees operation is supposed to work on whatever index the caller specifies... not specifically the_index.
Update the code to use "istate" argument that's originally from mark_new_skip_worktree(). From the call sites, both in unpack_trees(), you can see that this function works on two separate indexes:o->src_index and o->result. The second mark_new_skip_worktree() so far has incorecctly applied exclude rules on o->src_index instead of o->result. It's unclear what is the consequences of this, but it's definitely wrong.
[1] fba92be8f7 (dir: convert is_excluded_from_list to take an index -
2017-05-05)
--- unpack-trees.c | 31 ++++++++++++++++++------------- 1 file changed, 18 insertions(+), 13 deletions(-)
unpack-trees.c +18 -13
@@ -1085,13 +1085,15 @@ static int unpack_callback(int n, unsigned long mask, unsigned long dirmask, strreturn mask; } -static int clear_ce_flags_1 (struct cache_entry **cache,int nr, +static int clear_ce_flags_1 (struct index_state *istate, +struct cache_entry **cache,int nr,struct strbuf *prefix,int select_mask,int clear_mask,struct exclude_list *el,int defval);/* Whole directory matching */ -static int clear_ce_flags_dir(struct cache_entry **cache, int nr, +static int clear_ce_flags_dir(struct index_state *istate, + struct cache_entry **cache, int nr, struct strbuf *prefix, char *basename, int select_mask, int clear_mask,
@@ -1100,7 +1102,7 @@ static int clear_ce_flags_dir(struct cache_entry **cache, int nr,struct cache_entry **cache_end;int dtype =DT_DIR ;int ret =is_excluded_from_list (prefix->buf, prefix->len, - basename, &dtype, el, &the_index); + basename, &dtype, el, istate);int rc;strbuf_addch (prefix,'/' );
@@ -1122,7 +1124,7 @@ static int clear_ce_flags_dir(struct cache_entry **cache, int nr,* calling clear_ce_flags_1(). That function will call * the expensive is_excluded_from_list() on every entry. */ - rc =clear_ce_flags_1 (cache, cache_end - cache, + rc =clear_ce_flags_1 (istate, cache, cache_end - cache, prefix, select_mask, clear_mask, el, ret);
@@ -1145,7 +1147,8 @@ static int clear_ce_flags_dir(struct cache_entry **cache, int nr,* cache[0]->name[0..(prefix_len-1)] * Top level path has prefix_len zero. */ -static int clear_ce_flags_1 (struct cache_entry **cache,int nr, +static int clear_ce_flags_1 (struct index_state *istate, +struct cache_entry **cache,int nr,struct strbuf *prefix,int select_mask,int clear_mask,struct exclude_list *el,int defval)
@@ -1179,7 +1182,7 @@ static int clear_ce_flags_1(struct cache_entry **cache, int nr, len = slash - name;strbuf_add (prefix, name, len); - processed = clear_ce_flags_dir(cache, cache_end - cache, + processed = clear_ce_flags_dir(istate, cache, cache_end - cache, prefix, prefix->buf + prefix->len - len, select_mask, clear_mask,
@@ -1193,7 +1196,7 @@ static int clear_ce_flags_1(struct cache_entry **cache, int nr, }strbuf_addch (prefix,'/' ); - cache +=clear_ce_flags_1 (cache, cache_end - cache, + cache +=clear_ce_flags_1 (istate, cache, cache_end - cache, prefix, select_mask, clear_mask, el, defval);strbuf_setlen (prefix, prefix->len - len -1 );
@@ -1203,7 +1206,7 @@ static int clear_ce_flags_1(struct cache_entry **cache, int nr,/* Non-directory */ dtype =ce_to_dtype (ce); ret =is_excluded_from_list (ce->name,ce_namelen (ce), - name, &dtype, el, &the_index); + name, &dtype, el, istate);if (ret <0 ) ret = defval;if (ret >0 )
@@ -1213,15 +1216,17 @@ static int clear_ce_flags_1(struct cache_entry **cache, int nr,return nr - (cache_end - cache); } -static int clear_ce_flags (struct cache_entry **cache,int nr, -int select_mask,int clear_mask, -struct exclude_list *el) +static int clear_ce_flags (struct index_state *istate, +int select_mask,int clear_mask, +struct exclude_list *el) {static struct strbuf prefix =STRBUF_INIT ;strbuf_reset (&prefix); -return clear_ce_flags_1 (cache, nr, +return clear_ce_flags_1 (istate, + istate->cache, + istate->cache_nr, &prefix, select_mask, clear_mask, el,0 );
@@ -1256,7 +1261,7 @@ static void mark_new_skip_worktree(struct exclude_list *el,* 2. Widen worktree according to sparse-checkout file. * Matched entries will have skip_wt_flag cleared (i.e. "in") */ -clear_ce_flags (istate->cache, istate->cache_nr, select_flag, skip_wt_flag, el); +clear_ce_flags (istate, select_flag, skip_wt_flag, el); } static int verify_absent(const struct cache_entry *,
Signature · 1 line
2.18.0.rc0.333.g22e6ee6cdf
PATCH v2 5/5 unpack-trees: avoid the_index in verify_absent()
In reply to Nguyễn Thái Ngọc DuyBoth functions that are updated in this commit are called by verify_absent(), which is part of the "unpack-trees" operation that is supposed to work on any index file specified by the caller. Thanks to Brandon [1] [2], an implicit dependency on the_index is exposed. This commit fixes it.
In both functions, it makes sense to use src_index to check for exclusion because it's almost unchanged and should give us the same outcome as if running the exclude check before the unpack.
It's "almost unchanged" because we do invalidate cache-tree and untracked cache in the source index. But this should not affect how exclude machinery uses the index: to see if a file is tracked, and to read a blob from the index instead of worktree if it's marked skip-worktree (i.e. it's not available in worktree)
[1] a0bba65b10 (dir: convert is_excluded to take an index - 2017-05-05
[2] 2c1eb10454 (dir: convert read_directory to take an index - 2017-05-05)
--- unpack-trees.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
unpack-trees.c +2 -2
@@ -1651,7 +1651,7 @@ static int verify_clean_subdirectory(const struct cache_entry *ce,memset (&d,0 ,sizeof (d));if (o->dir) d.exclude_per_dir = o->dir->exclude_per_dir; - i =read_directory (&d, &the_index, pathbuf, namelen+1 ,NULL ); + i =read_directory (&d, o->src_index, pathbuf, namelen+1 ,NULL );if (i)return o->gently ?-1 :add_rejected_path (o,ERROR_NOT_UPTODATE_DIR , ce->name);
@@ -1693,7 +1693,7 @@ static int check_ok_to_remove(const char *name, int len, int dtype,return 0 ;if (o->dir && -is_excluded (o->dir, &the_index, name, &dtype)) +is_excluded (o->dir, o->src_index, name, &dtype))/* * ce->name is explicitly excluded, so it is Ok to * overwrite it.
Signature · 1 line
2.18.0.rc0.333.g22e6ee6cdf
v3 cover Fix incorrect use of the_index
In reply to Nguyễn Thái Ngọc DuyQuoted 3 lines · Nguyễn Thái Ngọc Duy
v2 fixes an incorrect patch splitting (I should have built one more time :P) between 3/5 and 4/5. v1's 6/6 is dropped. Brandon suggested a better way of doing it which may happen later.
I started looking into Brandon's suggestion, did some mindless conversion, then found out a couple more incorrect uses of the_index :( So here's the whole thing.
Nguyễn Thái Ngọc Duy (20):
unpack-trees: remove 'extern' on function declaration
unpack-trees: add a note about path invalidation
unpack-trees: don't shadow global var the_index
unpack-tress: convert clear_ce_flags* to avoid the_index
unpack-trees: avoid the_index in verify_absent() attr.h: drop extern from function declaration
attr: remove an implicit dependency on the_index convert.h: drop 'extern' from function declaration convert.c: remove an implicit dependency on the_index dir.c: remove an implicit dependency on the_index in pathspec code
ls-files: correct index argument to get_convert_attr_ascii() pathspec.c: use the right index instead of the_index submodule.c: use the right index instead of the_index entry.c: use the right index instead of the_index
attr: remove index from git_attr_set_direction() preload-index.c: use the right index instead of the_index cache.c: remove an implicit dependency on the_index resolve-undo.c: use the right index instead of the_index
grep: use the right index instead of the_index
cache.h: make the_index part of "compatibility macros"
apply.c | 4 +-- archive.c | 8 ++--- attr.c | 52 ++++++++++++++++++------------- attr.h | 31 ++++++++++--------- builtin/add.c | 6 ++-- builtin/cat-file.c | 2 +- builtin/check-attr.c | 6 ++-- builtin/checkout-index.c | 1 + builtin/checkout.c | 4 +-- builtin/clean.c | 2 +- builtin/commit.c | 2 +- builtin/difftool.c | 4 +-- builtin/grep.c | 6 ++-- builtin/ls-files.c | 17 ++++++----- builtin/pack-objects.c | 2 +- builtin/reset.c | 2 +- builtin/rm.c | 2 +- builtin/submodule--helper.c | 2 +- builtin/update-index.c | 2 +- cache.h | 9 +++--- convert.c | 41 ++++++++++++++----------- convert.h | 61 +++++++++++++++++++++---------------- diff-lib.c | 4 +-- diff.c | 2 +- dir.c | 27 +++++++++------- dir.h | 16 ++++++---- entry.c | 10 +++--- ll-merge.c | 4 +-- merge-recursive.c | 4 +-- pathspec.c | 2 +- preload-index.c | 3 +- read-cache.c | 19 +++++++----- rerere.c | 2 +- resolve-undo.c | 5 +-- revision.c | 2 +- sha1-file.c | 4 +-- submodule.c | 8 ++--- unpack-trees.c | 57 +++++++++++++++++++++------------- unpack-trees.h | 4 +-- userdiff.c | 2 +- ws.c | 2 +- wt-status.c | 6 ++-- 42 files changed, 255 insertions(+), 194 deletions(-)
Signature · 1 line
2.18.0.rc0.333.g22e6ee6cdf
PATCH v3 1/20 unpack-trees: remove 'extern' on function declaration
--- unpack-trees.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
unpack-trees.h +2 -2
@@ -82,8 +82,8 @@ struct unpack_trees_options {struct exclude_list *el;/* for internal use */ }; -extern int unpack_trees (unsigned n,struct tree_desc *t, -struct unpack_trees_options *options); +int unpack_trees (unsigned n,struct tree_desc *t, +struct unpack_trees_options *options);int verify_uptodate (const struct cache_entry *ce,struct unpack_trees_options *o);
Signature · 1 line
2.18.0.rc0.333.g22e6ee6cdf
PATCH v3 2/20 unpack-trees: add a note about path invalidation
In reply to Nguyễn Thái Ngọc Duy--- unpack-trees.c | 11 +++++++++++ 1 file changed, 11 insertions(+)
unpack-trees.c +11 -0
@@ -1545,6 +1545,17 @@ static int verify_uptodate_sparse(const struct cache_entry *ce,return verify_uptodate_1 (ce, o,ERROR_SPARSE_NOT_UPTODATE_FILE ); } +/* +* TODO: We should actually invalidate o->result, not src_index [1]. +* But since cache tree and untracked cache both are not copied to +* o->result until unpacking is complete, we invalidate them on +* src_index instead with the assumption that they will be copied to +* dst_index at the end. +* +* [1] src_index->cache_tree is also used in unpack_callback() so if +* we invalidate o->result, we need to update it to use +* o->result.cache_tree as well. +*/ static void invalidate_ce_path (const struct cache_entry *ce,struct unpack_trees_options *o) {
Signature · 1 line
2.18.0.rc0.333.g22e6ee6cdf
PATCH v3 3/20 unpack-trees: don't shadow global var the_index
In reply to Nguyễn Thái Ngọc DuyThis function mark_new_skip_worktree() has an argument named the_index which is also the name of a global variable. While they have different types (the global the_index is not a pointer) mistakes can easily happen and it's also confusing for readers. Rename the function argument to something other than the_index.
--- unpack-trees.c | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-)
unpack-trees.c +4 -5
@@ -1231,7 +1231,7 @@ static int clear_ce_flags(struct cache_entry **cache, int nr,* Set/Clear CE_NEW_SKIP_WORKTREE according to $GIT_DIR/info/sparse-checkout */ static void mark_new_skip_worktree (struct exclude_list *el, -struct index_state *the_index, +struct index_state *istate,int select_flag,int skip_wt_flag) {int i;
@@ -1240,8 +1240,8 @@ static void mark_new_skip_worktree(struct exclude_list *el,* 1. Pretend the narrowest worktree: only unmerged entries * are checked out */ -for (i =0 ; i < the_index->cache_nr; i++) { -struct cache_entry *ce = the_index->cache[i]; +for (i =0 ; i < istate->cache_nr; i++) { +struct cache_entry *ce = istate->cache[i];if (select_flag && !(ce->ce_flags & select_flag))continue ;
@@ -1256,8 +1256,7 @@ static void mark_new_skip_worktree(struct exclude_list *el,* 2. Widen worktree according to sparse-checkout file. * Matched entries will have skip_wt_flag cleared (i.e. "in") */ -clear_ce_flags (the_index->cache, the_index->cache_nr, - select_flag, skip_wt_flag, el); +clear_ce_flags (istate->cache, istate->cache_nr, select_flag, skip_wt_flag, el); } static int verify_absent(const struct cache_entry *,
Signature · 1 line
2.18.0.rc0.333.g22e6ee6cdf
PATCH v3 4/20 unpack-tress: convert clear_ce_flags* to avoid the_index
In reply to Nguyễn Thái Ngọc DuyPrior to fba92be8f7, this code implicitly (and incorrectly) assumes the_index when running the exclude machinery. fba92be8f7 helps show this problem clearer because unpack-trees operation is supposed to work on whatever index the caller specifies... not specifically the_index.
Update the code to use "istate" argument that's originally from mark_new_skip_worktree(). From the call sites, both in unpack_trees(), you can see that this function works on two separate indexes:o->src_index and o->result. The second mark_new_skip_worktree() so far has incorecctly applied exclude rules on o->src_index instead of o->result. It's unclear what is the consequences of this, but it's definitely wrong.
[1] fba92be8f7 (dir: convert is_excluded_from_list to take an index -
2017-05-05)
--- unpack-trees.c | 31 ++++++++++++++++++------------- 1 file changed, 18 insertions(+), 13 deletions(-)
unpack-trees.c +18 -13
@@ -1085,13 +1085,15 @@ static int unpack_callback(int n, unsigned long mask, unsigned long dirmask, strreturn mask; } -static int clear_ce_flags_1 (struct cache_entry **cache,int nr, +static int clear_ce_flags_1 (struct index_state *istate, +struct cache_entry **cache,int nr,struct strbuf *prefix,int select_mask,int clear_mask,struct exclude_list *el,int defval);/* Whole directory matching */ -static int clear_ce_flags_dir(struct cache_entry **cache, int nr, +static int clear_ce_flags_dir(struct index_state *istate, + struct cache_entry **cache, int nr, struct strbuf *prefix, char *basename, int select_mask, int clear_mask,
@@ -1100,7 +1102,7 @@ static int clear_ce_flags_dir(struct cache_entry **cache, int nr,struct cache_entry **cache_end;int dtype =DT_DIR ;int ret =is_excluded_from_list (prefix->buf, prefix->len, - basename, &dtype, el, &the_index); + basename, &dtype, el, istate);int rc;strbuf_addch (prefix,'/' );
@@ -1122,7 +1124,7 @@ static int clear_ce_flags_dir(struct cache_entry **cache, int nr,* calling clear_ce_flags_1(). That function will call * the expensive is_excluded_from_list() on every entry. */ - rc =clear_ce_flags_1 (cache, cache_end - cache, + rc =clear_ce_flags_1 (istate, cache, cache_end - cache, prefix, select_mask, clear_mask, el, ret);
@@ -1145,7 +1147,8 @@ static int clear_ce_flags_dir(struct cache_entry **cache, int nr,* cache[0]->name[0..(prefix_len-1)] * Top level path has prefix_len zero. */ -static int clear_ce_flags_1 (struct cache_entry **cache,int nr, +static int clear_ce_flags_1 (struct index_state *istate, +struct cache_entry **cache,int nr,struct strbuf *prefix,int select_mask,int clear_mask,struct exclude_list *el,int defval)
@@ -1179,7 +1182,7 @@ static int clear_ce_flags_1(struct cache_entry **cache, int nr, len = slash - name;strbuf_add (prefix, name, len); - processed = clear_ce_flags_dir(cache, cache_end - cache, + processed = clear_ce_flags_dir(istate, cache, cache_end - cache, prefix, prefix->buf + prefix->len - len, select_mask, clear_mask,
@@ -1193,7 +1196,7 @@ static int clear_ce_flags_1(struct cache_entry **cache, int nr, }strbuf_addch (prefix,'/' ); - cache +=clear_ce_flags_1 (cache, cache_end - cache, + cache +=clear_ce_flags_1 (istate, cache, cache_end - cache, prefix, select_mask, clear_mask, el, defval);strbuf_setlen (prefix, prefix->len - len -1 );
@@ -1203,7 +1206,7 @@ static int clear_ce_flags_1(struct cache_entry **cache, int nr,/* Non-directory */ dtype =ce_to_dtype (ce); ret =is_excluded_from_list (ce->name,ce_namelen (ce), - name, &dtype, el, &the_index); + name, &dtype, el, istate);if (ret <0 ) ret = defval;if (ret >0 )
@@ -1213,15 +1216,17 @@ static int clear_ce_flags_1(struct cache_entry **cache, int nr,return nr - (cache_end - cache); } -static int clear_ce_flags (struct cache_entry **cache,int nr, -int select_mask,int clear_mask, -struct exclude_list *el) +static int clear_ce_flags (struct index_state *istate, +int select_mask,int clear_mask, +struct exclude_list *el) {static struct strbuf prefix =STRBUF_INIT ;strbuf_reset (&prefix); -return clear_ce_flags_1 (cache, nr, +return clear_ce_flags_1 (istate, + istate->cache, + istate->cache_nr, &prefix, select_mask, clear_mask, el,0 );
@@ -1256,7 +1261,7 @@ static void mark_new_skip_worktree(struct exclude_list *el,* 2. Widen worktree according to sparse-checkout file. * Matched entries will have skip_wt_flag cleared (i.e. "in") */ -clear_ce_flags (istate->cache, istate->cache_nr, select_flag, skip_wt_flag, el); +clear_ce_flags (istate, select_flag, skip_wt_flag, el); } static int verify_absent(const struct cache_entry *,
Signature · 1 line
2.18.0.rc0.333.g22e6ee6cdf
PATCH v3 5/20 unpack-trees: avoid the_index in verify_absent()
In reply to Nguyễn Thái Ngọc DuyBoth functions that are updated in this commit are called by verify_absent(), which is part of the "unpack-trees" operation that is supposed to work on any index file specified by the caller. Thanks to Brandon [1] [2], an implicit dependency on the_index is exposed. This commit fixes it.
In both functions, it makes sense to use src_index to check for exclusion because it's almost unchanged and should give us the same outcome as if running the exclude check before the unpack.
It's "almost unchanged" because we do invalidate cache-tree and untracked cache in the source index. But this should not affect how exclude machinery uses the index: to see if a file is tracked, and to read a blob from the index instead of worktree if it's marked skip-worktree (i.e. it's not available in worktree)
[1] a0bba65b10 (dir: convert is_excluded to take an index - 2017-05-05
[2] 2c1eb10454 (dir: convert read_directory to take an index - 2017-05-05)
--- unpack-trees.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
unpack-trees.c +2 -2
@@ -1651,7 +1651,7 @@ static int verify_clean_subdirectory(const struct cache_entry *ce,memset (&d,0 ,sizeof (d));if (o->dir) d.exclude_per_dir = o->dir->exclude_per_dir; - i =read_directory (&d, &the_index, pathbuf, namelen+1 ,NULL ); + i =read_directory (&d, o->src_index, pathbuf, namelen+1 ,NULL );if (i)return o->gently ?-1 :add_rejected_path (o,ERROR_NOT_UPTODATE_DIR , ce->name);
@@ -1693,7 +1693,7 @@ static int check_ok_to_remove(const char *name, int len, int dtype,return 0 ;if (o->dir && -is_excluded (o->dir, &the_index, name, &dtype)) +is_excluded (o->dir, o->src_index, name, &dtype))/* * ce->name is explicitly excluded, so it is Ok to * overwrite it.
Signature · 1 line
2.18.0.rc0.333.g22e6ee6cdf
PATCH v3 6/20 attr.h: drop extern from function declaration
In reply to Nguyễn Thái Ngọc Duy--- attr.h | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-)
attr.h +12 -12
@@ -42,31 +42,31 @@ struct attr_check {struct attr_stack *stack; }; -extern struct attr_check *attr_check_alloc (void ); -extern struct attr_check *attr_check_initl (const char *, ...); -extern struct attr_check *attr_check_dup (const struct attr_check *check); +struct attr_check *attr_check_alloc (void ); +struct attr_check *attr_check_initl (const char *, ...); +struct attr_check *attr_check_dup (const struct attr_check *check); -extern struct attr_check_item *attr_check_append (struct attr_check *check, -const struct git_attr *attr); +struct attr_check_item *attr_check_append (struct attr_check *check, +const struct git_attr *attr); -extern void attr_check_reset (struct attr_check *check); -extern void attr_check_clear (struct attr_check *check); -extern void attr_check_free (struct attr_check *check); +void attr_check_reset (struct attr_check *check); +void attr_check_clear (struct attr_check *check); +void attr_check_free (struct attr_check *check);/* * Return the name of the attribute represented by the argument. The * return value is a pointer to a null-delimited string that is part * of the internal data structure; it should not be modified or freed. */ -extern const char *git_attr_name (const struct git_attr *); +const char *git_attr_name (const struct git_attr *); -extern int git_check_attr (const char *path,struct attr_check *check); +int git_check_attr (const char *path,struct attr_check *check);/* * Retrieve all attributes that apply to the specified path. * check holds the attributes and their values. */ -extern void git_all_attrs (const char *path,struct attr_check *check); +void git_all_attrs (const char *path,struct attr_check *check);enum git_attr_direction {GIT_ATTR_CHECKIN ,
@@ -76,6 +76,6 @@ enum git_attr_direction {void git_attr_set_direction (enum git_attr_direction new_direction,struct index_state *istate); -extern void attr_start (void ); +void attr_start (void );#endif /* ATTR_H */
Signature · 1 line
2.18.0.rc0.333.g22e6ee6cdf
PATCH v3 7/20 attr: remove an implicit dependency on the_index
In reply to Nguyễn Thái Ngọc DuyMake the attr API take an index_state instead of assuming the_index in attr code. All call sites are converted blindly to keep the patch simple and retain current behavior. Individual call sites may receive further updates to use the right index instead of the_index.
There is one ugly temporary workaround added in attr.c that needs some more explanation.
Commit c24f3abace (apply: file commited * with CRLF should roundtrip diff and apply - 2017-08-19) forces one convert_to_git() call to NOT read the index at all. But what do you know, we read it anyway by falling back to the_index. When "istate" from convert_to_git is now propagated down to read_attr_from_array() we will hit segfault somewhere inside read_blob_data_from_index.
The right way of dealing with this is to kill "use_index" variable and only follow "istate" but at this stage we are not ready for that:
while most git_attr_set_direction() calls just passes the_index to be assigned to use_index, unpack-trees passes a different one which is used by entry.c code, which has no way to know what index to use if we delete use_index. So this has to be done later.
--- archive.c | 2 +- attr.c | 57 ++++++++++++++++++++++++++++-------------- attr.h | 10 +++++--- builtin/check-attr.c | 4 +-- builtin/pack-objects.c | 2 +- convert.c | 2 +- dir.c | 2 +- ll-merge.c | 4 +-- userdiff.c | 2 +- ws.c | 2 +- 10 files changed, 55 insertions(+), 32 deletions(-)
archive.c +1 -1
@@ -108,7 +108,7 @@ static const struct attr_check *get_archive_attrs(const char *path)static struct attr_check *check;if (!check) check =attr_check_initl ("export-ignore" ,"export-subst" ,NULL ); -return git_check_attr (path, check) ?NULL : check; +return git_check_attr (&the_index, path, check) ?NULL : check; }static int check_attr_export_ignore (const struct attr_check *check)
attr.c +38 -19
155 lines not on this page · raw message
attr.h +7 -3
@@ -1,6 +1,8 @@#ifndef ATTR_H #define ATTR_H +struct index_state ; +/* An attribute is a pointer to this opaque structure */ struct git_attr ;
@@ -60,13 +62,15 @@ void attr_check_free(struct attr_check *check);*/ const char *git_attr_name (const struct git_attr *); -int git_check_attr (const char *path,struct attr_check *check); +int git_check_attr (const struct index_state *istate, +const char *path,struct attr_check *check);/* * Retrieve all attributes that apply to the specified path. * check holds the attributes and their values. */ -void git_all_attrs (const char *path,struct attr_check *check); +void git_all_attrs (const struct index_state *istate, +const char *path,struct attr_check *check);enum git_attr_direction {GIT_ATTR_CHECKIN ,
@@ -74,7 +78,7 @@ enum git_attr_direction { GIT_ATTR_INDEX };void git_attr_set_direction (enum git_attr_direction new_direction, -struct index_state *istate); +const struct index_state *istate);void attr_start (void );
builtin/check-attr.c +2 -2
@@ -63,9 +63,9 @@ static void check_attr(const char *prefix,prefix_path (prefix, prefix ?strlen (prefix) :0 , file);if (collect_all) { -git_all_attrs (full_path, check); +git_all_attrs (&the_index, full_path, check); }else { -if (git_check_attr (full_path, check)) +if (git_check_attr (&the_index, full_path, check))die ("git_check_attr died" ); }output_attr (check, file);
builtin/pack-objects.c +1 -1
@@ -945,7 +945,7 @@ static int no_try_delta(const char *path)if (!check) check =attr_check_initl ("delta" ,NULL ); -if (git_check_attr (path, check)) +if (git_check_attr (&the_index, path, check))return 0 ;if (ATTR_FALSE (check->items[0 ].value))return 1 ;
convert.c +1 -1
@@ -1302,7 +1302,7 @@ static void convert_attrs(struct conv_attrs *ca, const char *path)git_config (read_convert_config,NULL ); } -if (!git_check_attr (path, check)) { +if (!git_check_attr (&the_index, path, check)) {struct attr_check_item *ccheck = check->items; ca->crlf_action =git_path_check_crlf (ccheck +4 );if (ca->crlf_action ==CRLF_UNDEFINED )
dir.c +1 -1
@@ -280,7 +280,7 @@ static int match_attrs(const char *name, int namelen, {int i; -git_check_attr (name, item->attr_check); +git_check_attr (&the_index, name, item->attr_check);for (i =0 ; i < item->attr_match_nr; i++) {const char *value;int matched;
ll-merge.c +2 -2
@@ -371,7 +371,7 @@ int ll_merge(mmbuffer_t *result_buf,if (!check) check =attr_check_initl ("merge" ,"conflict-marker-size" ,NULL ); -if (!git_check_attr (path, check)) { +if (!git_check_attr (&the_index, path, check)) { ll_driver_name = check->items[0 ].value;if (check->items[1 ].value) { marker_size =atoi (check->items[1 ].value);
@@ -398,7 +398,7 @@ int ll_merge_marker_size(const char *path)if (!check) check =attr_check_initl ("conflict-marker-size" ,NULL ); -if (!git_check_attr (path, check) && check->items[0 ].value) { +if (!git_check_attr (&the_index, path, check) && check->items[0 ].value) { marker_size =atoi (check->items[0 ].value);if (marker_size <=0 ) marker_size =DEFAULT_CONFLICT_MARKER_SIZE ;
userdiff.c +1 -1
@@ -278,7 +278,7 @@ struct userdiff_driver *userdiff_find_by_path(const char *path) check =attr_check_initl ("diff" ,NULL );if (!path)return NULL ; -if (git_check_attr (path, check)) +if (git_check_attr (&the_index, path, check))return NULL ;if (ATTR_TRUE (check->items[0 ].value))
ws.c +1 -1
@@ -78,7 +78,7 @@ unsigned whitespace_rule(const char *pathname)if (!attr_whitespace_rule) attr_whitespace_rule =attr_check_initl ("whitespace" ,NULL ); -if (!git_check_attr (pathname, attr_whitespace_rule)) { +if (!git_check_attr (&the_index, pathname, attr_whitespace_rule)) {const char *value; value = attr_whitespace_rule->items[0 ].value;
Signature · 1 line
2.18.0.rc0.333.g22e6ee6cdf
Re: [PATCH v3 07/20] attr: remove an implicit dependency on the_index
Quoted 9 lines · Nguyễn Thái Ngọc Duy
Make the attr API take an
index_stateinstead of assumingthe_indexin attr code. All call sites are converted blindly to keep the patch simple and retain current behavior. Individual call sites may receive further updates to use the right index instead ofthe_index.There is one ugly temporary workaround added in
attr.cthat needs some more explanation.Commit
c24f3abace(apply: file commited * with CRLF should roundtrip
s/commited * with/commited with/
ATB,
Ramsay Jones
Re: [PATCH v3 07/20] attr: remove an implicit dependency on the_index
In reply to Nguyễn Thái Ngọc DuyQuoted 15 lines · Nguyễn Thái Ngọc Duy
Make the attr API take an
index_stateinstead of assumingthe_indexin attr code. All call sites are converted blindly to keep the patch simple and retain current behavior. Individual call sites may receive further updates to use the right index instead ofthe_index.There is one ugly temporary workaround added in
attr.cthat needs some more explanation.Commit
c24f3abace(apply: file commited * with CRLF should roundtrip diff and apply - 2017-08-19) forces oneconvert_to_git()call to NOT read the index at all. But what do you know, we read it anyway by falling back tothe_index. When "istate" fromconvert_to_gitis now propagated down toread_attr_from_array()we will hit segfault somewhere insideread_blob_data_from_index.
Quoted 6 lines
The right way of dealing with this is to kill "
use_index" variable and only follow "istate" but at this stage we are not ready for that:
while mostgit_attr_set_direction()calls just passesthe_indexto be assigned touse_index, unpack-trees passes a different one which is used byentry.ccode, which has no way to know what index to use if we deleteuse_index. So this has to be done later.
Ugh I remember this when I was doing some refactoring to the attr subsystem a year or so ago. I really wanted to get rid of the whole "direction" thing because if I remember correctly its one of the only remaining globals that affects the outcome of an attr check (everything else was converted to use the attr check struct. I always got way to far into the weeds when trying to do that too. I'm not expecting that from this series (since its way out of scope) but maybe it'll be easier afterwards.
Quoted 346 lines
Signed-off-by Nguyễn Thái Ngọc Duy <[email protected]>--- archive.c | 2 +- attr.c | 57 ++++++++++++++++++++++++++++-------------- attr.h | 10 +++++--- builtin/check-attr.c | 4 +-- builtin/pack-objects.c | 2 +- convert.c | 2 +- dir.c | 2 +- ll-merge.c | 4 +-- userdiff.c | 2 +- ws.c | 2 +- 10 files changed, 55 insertions(+), 32 deletions(-)archive.c +1
-1@@ -108,7 +108,7 @@ static const struct attr_check *get_archive_attrs(const char *path)static struct attr_check *check;if (!check) check =attr_check_initl ("export-ignore" ,"export-subst" ,NULL ); -return git_check_attr (path, check) ?NULL : check; +return git_check_attr (&the_index, path, check) ?NULL : check; }static int check_attr_export_ignore (const struct attr_check *check)attr.c +38
-19@@ -708,10 +708,10 @@ static struct attr_stack *read_attr_from_array(const char **list)* another thread could potentially be calling into the attribute system. */ static enum git_attr_direction direction; -static struct index_state *use_index; +static const struct index_state *use_index;void git_attr_set_direction (enum git_attr_direction new_direction, -struct index_state *istate) +const struct index_state *istate) {if (is_bare_repository () && new_direction !=GIT_ATTR_INDEX )BUG ("non-INDEX attr direction in a bare repo" );@@ -743,13 +743,24 @@ static struct attr_stack *read_attr_from_file(const char *path, int macro_ok)return res; } -static struct attr_stack *read_attr_from_index (const char *path,int macro_ok) +static struct attr_stack *read_attr_from_index (const struct index_state *istate, +const char *path, +int macro_ok) {struct attr_stack *res;char *buf, *sp;int lineno =0 ; +const struct index_state *to_read_from; - buf =read_blob_data_from_index (use_index ? use_index : &the_index, path,NULL ); +/* +* Temporary workaround for c24f3abace (apply: file commited +* with CRLF should roundtrip diff and apply - 2017-08-19) +*/ + to_read_from = use_index ? use_index : istate; +if (!to_read_from) +return NULL ; + + buf =read_blob_data_from_index (to_read_from, path,NULL );if (!buf)return NULL ;@@ -768,15 +779,16 @@ static struct attr_stack *read_attr_from_index(const char *path, int macro_ok)return res; } -static struct attr_stack *read_attr (const char *path,int macro_ok) +static struct attr_stack *read_attr (const struct index_state *istate, +const char *path,int macro_ok) {struct attr_stack *res =NULL ;if (direction ==GIT_ATTR_INDEX ) { - res =read_attr_from_index (path, macro_ok); + res =read_attr_from_index (istate, path, macro_ok); }else if (!is_bare_repository ()) {if (direction ==GIT_ATTR_CHECKOUT ) { - res =read_attr_from_index (path, macro_ok); + res =read_attr_from_index (istate, path, macro_ok);if (!res) res =read_attr_from_file (path, macro_ok); }else if (direction ==GIT_ATTR_CHECKIN ) {@@ -788,7 +800,7 @@ static struct attr_stack *read_attr(const char *path, int macro_ok)* We allow operation in a sparsely checked out * work tree, so read from it. */ - res =read_attr_from_index (path, macro_ok); + res =read_attr_from_index (istate, path, macro_ok); } }@@ -859,7 +871,8 @@ static void push_stack(struct attr_stack **attr_stack_p, } } -static void bootstrap_attr_stack (struct attr_stack **stack) +static void bootstrap_attr_stack (const struct index_state *istate, +struct attr_stack **stack) {struct attr_stack *e;@@ -883,7 +896,7 @@ static void bootstrap_attr_stack(struct attr_stack **stack) }/* root directory */ - e =read_attr (GITATTRIBUTES_FILE ,1 ); + e =read_attr (istate,GITATTRIBUTES_FILE ,1 );push_stack (stack, e,xstrdup ("" ),0 );/* info frame */ @@ -896,7 +909,8 @@ static void bootstrap_attr_stack(struct attr_stack **stack)push_stack (stack, e,NULL ,0 ); } -static void prepare_attr_stack (const char *path,int dirlen, +static void prepare_attr_stack (const struct index_state *istate, +const char *path,int dirlen,struct attr_stack **stack) {struct attr_stack *info;@@ -917,7 +931,7 @@ static void prepare_attr_stack(const char *path, int dirlen,* .gitattributes in deeper directories to shallower ones, * and finally use the built-in set as the default. */ -bootstrap_attr_stack (stack); +bootstrap_attr_stack (istate, stack);/* * Pop the "info" one that is always at the top of the stack. @@ -973,7 +987,7 @@ static void prepare_attr_stack(const char *path, int dirlen,strbuf_add (&pathbuf, path + pathbuf.len, (len - pathbuf.len));strbuf_addf (&pathbuf,"/%s" ,GITATTRIBUTES_FILE ); - next =read_attr (pathbuf.buf,0 ); + next =read_attr (istate, pathbuf.buf,0 );/* reset the pathbuf to not include "/.gitattributes" */ strbuf_setlen (&pathbuf, len);@@ -1095,7 +1109,9 @@ static void determine_macros(struct all_attrs_item *all_attrs,* If check->check_nr is non-zero, only attributes in check[] are collected. * Otherwise all attributes are collected. */ -static void collect_some_attrs (const char *path,struct attr_check *check) +static void collect_some_attrs (const struct index_state *istate, +const char *path, +struct attr_check *check) {int i, pathlen, rem, dirlen;const char *cp, *last_slash =NULL ;@@ -1114,7 +1130,7 @@ static void collect_some_attrs(const char *path, struct attr_check *check) dirlen =0 ; } -prepare_attr_stack (path, dirlen, &check->stack); +prepare_attr_stack (istate, path, dirlen, &check->stack);all_attrs_init (&g_attr_hashmap, check);determine_macros (check->all_attrs, check->stack);@@ -1136,11 +1152,13 @@ static void collect_some_attrs(const char *path, struct attr_check *check)fill (path, pathlen, basename_offset, check->stack, check->all_attrs, rem); } -int git_check_attr (const char *path,struct attr_check *check) +int git_check_attr (const struct index_state *istate, +const char *path, +struct attr_check *check) {int i; -collect_some_attrs (path, check); +collect_some_attrs (istate, path, check);for (i =0 ; i < check->nr; i++) {size_t n = check->items[i].attr->attr_nr;@@ -1153,12 +1171,13 @@ int git_check_attr(const char *path, struct attr_check *check)return 0 ; } -void git_all_attrs (const char *path,struct attr_check *check) +void git_all_attrs (const struct index_state *istate, +const char *path,struct attr_check *check) {int i;attr_check_reset (check); -collect_some_attrs (path, check); +collect_some_attrs (istate, path, check);for (i =0 ; i < check->all_attrs_nr; i++) {const char *name = check->all_attrs[i].attr->name;attr.h +7
-3@@ -1,6 +1,8 @@#ifndef ATTR_H #define ATTR_H +struct index_state ; +/* An attribute is a pointer to this opaque structure */ struct git_attr ;@@ -60,13 +62,15 @@ void attr_check_free(struct attr_check *check);*/ const char *git_attr_name (const struct git_attr *); -int git_check_attr (const char *path,struct attr_check *check); +int git_check_attr (const struct index_state *istate, +const char *path,struct attr_check *check);/* * Retrieve all attributes that apply to the specified path. * check holds the attributes and their values. */ -void git_all_attrs (const char *path,struct attr_check *check); +void git_all_attrs (const struct index_state *istate, +const char *path,struct attr_check *check);enum git_attr_direction {GIT_ATTR_CHECKIN ,@@ -74,7 +78,7 @@ enum git_attr_direction { GIT_ATTR_INDEX };void git_attr_set_direction (enum git_attr_direction new_direction, -struct index_state *istate); +const struct index_state *istate);void attr_start (void );builtin/check-attr.c +2
-2@@ -63,9 +63,9 @@ static void check_attr(const char *prefix,prefix_path (prefix, prefix ?strlen (prefix) :0 , file);if (collect_all) { -git_all_attrs (full_path, check); +git_all_attrs (&the_index, full_path, check); }else { -if (git_check_attr (full_path, check)) +if (git_check_attr (&the_index, full_path, check))die ("git_check_attr died" ); }output_attr (check, file);builtin/pack-objects.c +1
-1@@ -945,7 +945,7 @@ static int no_try_delta(const char *path)if (!check) check =attr_check_initl ("delta" ,NULL ); -if (git_check_attr (path, check)) +if (git_check_attr (&the_index, path, check))return 0 ;if (ATTR_FALSE (check->items[0 ].value))return 1 ;convert.c +1
-1@@ -1302,7 +1302,7 @@ static void convert_attrs(struct conv_attrs *ca, const char *path)git_config (read_convert_config,NULL ); } -if (!git_check_attr (path, check)) { +if (!git_check_attr (&the_index, path, check)) {struct attr_check_item *ccheck = check->items; ca->crlf_action =git_path_check_crlf (ccheck +4 );if (ca->crlf_action ==CRLF_UNDEFINED )dir.c +1
-1@@ -280,7 +280,7 @@ static int match_attrs(const char *name, int namelen, {int i; -git_check_attr (name, item->attr_check); +git_check_attr (&the_index, name, item->attr_check);for (i =0 ; i < item->attr_match_nr; i++) {const char *value;int matched;ll-merge.c +2
-2@@ -371,7 +371,7 @@ int ll_merge(mmbuffer_t *result_buf,if (!check) check =attr_check_initl ("merge" ,"conflict-marker-size" ,NULL ); -if (!git_check_attr (path, check)) { +if (!git_check_attr (&the_index, path, check)) { ll_driver_name = check->items[0 ].value;if (check->items[1 ].value) { marker_size =atoi (check->items[1 ].value);@@ -398,7 +398,7 @@ int ll_merge_marker_size(const char *path)if (!check) check =attr_check_initl ("conflict-marker-size" ,NULL ); -if (!git_check_attr (path, check) && check->items[0 ].value) { +if (!git_check_attr (&the_index, path, check) && check->items[0 ].value) { marker_size =atoi (check->items[0 ].value);if (marker_size <=0 ) marker_size =DEFAULT_CONFLICT_MARKER_SIZE ;userdiff.c +1
-1@@ -278,7 +278,7 @@ struct userdiff_driver *userdiff_find_by_path(const char *path) check =attr_check_initl ("diff" ,NULL );if (!path)return NULL ; -if (git_check_attr (path, check)) +if (git_check_attr (&the_index, path, check))return NULL ;if (ATTR_TRUE (check->items[0 ].value))ws.c +1
-1@@ -78,7 +78,7 @@ unsigned whitespace_rule(const char *pathname)if (!attr_whitespace_rule) attr_whitespace_rule =attr_check_initl ("whitespace" ,NULL ); -if (!git_check_attr (pathname, attr_whitespace_rule)) { +if (!git_check_attr (&the_index, pathname, attr_whitespace_rule)) {const char *value; value = attr_whitespace_rule->items[0 ].value;Signature · 1 line
2.18.0.rc0.333.g22e6ee6cdf
Signature · 1 line
Brandon Williams
Re: [PATCH v3 07/20] attr: remove an implicit dependency on the_index
Quoted 23 lines · Brandon Williams
Quoted 15 lines · Nguyễn Thái Ngọc Duy
Make the attr API take an
index_stateinstead of assumingthe_indexin attr code. All call sites are converted blindly to keep the patch simple and retain current behavior. Individual call sites may receive further updates to use the right index instead ofthe_index.There is one ugly temporary workaround added in
attr.cthat needs some more explanation.Commit
c24f3abace(apply: file commited * with CRLF should roundtrip diff and apply - 2017-08-19) forces oneconvert_to_git()call to NOT read the index at all. But what do you know, we read it anyway by falling back tothe_index. When "istate" fromconvert_to_gitis now propagated down toread_attr_from_array()we will hit segfault somewhere insideread_blob_data_from_index.Quoted 6 lines
The right way of dealing with this is to kill "
use_index" variable and only follow "istate" but at this stage we are not ready for that:
while mostgit_attr_set_direction()calls just passesthe_indexto be assigned touse_index, unpack-trees passes a different one which is used byentry.ccode, which has no way to know what index to use if we deleteuse_index. So this has to be done later.
Quoted 8 lines
Ugh I remember this when I was doing some refactoring to the attr subsystem a year or so ago. I really wanted to get rid of the whole "direction" thing because if I remember correctly its one of the only remaining globals that affects the outcome of an attr check (everything else was converted to use the attr check struct. I always got way to far into the weeds when trying to do that too. I'm not expecting that from this series (since its way out of scope) but maybe it'll be easier afterwards.
It's not that much easier. This direction thing is still global by design. It would be great if we have something like Scheme's parameter (aka. dynamic scoping iirc) then we can still scope this nicely. Git does not live in such worlds.
Signature · 1 line
Duy
Re: [PATCH v3 07/20] attr: remove an implicit dependency on the_index
Quoted 33 lines · Duy Nguyen
Quoted 23 lines · Brandon Williams
Quoted 15 lines · Nguyễn Thái Ngọc Duy
Make the attr API take an
index_stateinstead of assumingthe_indexin attr code. All call sites are converted blindly to keep the patch simple and retain current behavior. Individual call sites may receive further updates to use the right index instead ofthe_index.There is one ugly temporary workaround added in
attr.cthat needs some more explanation.Commit
c24f3abace(apply: file commited * with CRLF should roundtrip diff and apply - 2017-08-19) forces oneconvert_to_git()call to NOT read the index at all. But what do you know, we read it anyway by falling back tothe_index. When "istate" fromconvert_to_gitis now propagated down toread_attr_from_array()we will hit segfault somewhere insideread_blob_data_from_index.Quoted 6 lines
The right way of dealing with this is to kill "
use_index" variable and only follow "istate" but at this stage we are not ready for that:
while mostgit_attr_set_direction()calls just passesthe_indexto be assigned touse_index, unpack-trees passes a different one which is used byentry.ccode, which has no way to know what index to use if we deleteuse_index. So this has to be done later.Quoted 8 lines
Ugh I remember this when I was doing some refactoring to the attr subsystem a year or so ago. I really wanted to get rid of the whole "direction" thing because if I remember correctly its one of the only remaining globals that affects the outcome of an attr check (everything else was converted to use the attr check struct. I always got way to far into the weeds when trying to do that too. I'm not expecting that from this series (since its way out of scope) but maybe it'll be easier afterwards.
Quoted 4 lines
It's not that much easier. This direction thing is still global by design. It would be great if we have something like Scheme's parameter (aka. dynamic scoping iirc) then we can still scope this nicely. Git does not live in such worlds.
Yeah i realized this after sending. Either way thanks for simplifying the global state by getting ride of the index global.
Quoted 2 lines
Signature · 1 line
Duy
Signature · 1 line
Brandon Williams
PATCH v3 8/20 convert.h: drop 'extern' from function declaration
In reply to Nguyễn Thái Ngọc Duy--- convert.h | 56 ++++++++++++++++++++++++++++--------------------------- 1 file changed, 29 insertions(+), 27 deletions(-)
convert.h +29 -27
@@ -57,35 +57,36 @@ struct delayed_checkout {extern enum eol core_eol;extern char *check_roundtrip_encoding; -extern const char *get_cached_convert_stats_ascii (const struct index_state *istate, -const char *path); -extern const char *get_wt_convert_stats_ascii (const char *path); -extern const char *get_convert_attr_ascii (const char *path); +const char *get_cached_convert_stats_ascii (const struct index_state *istate, +const char *path); +const char *get_wt_convert_stats_ascii (const char *path); +const char *get_convert_attr_ascii (const char *path);/* returns 1 if *dst was used */ -extern int convert_to_git (const struct index_state *istate, -const char *path,const char *src,size_t len, -struct strbuf *dst,int conv_flags); -extern int convert_to_working_tree (const char *path,const char *src, -size_t len,struct strbuf *dst); -extern int async_convert_to_working_tree (const char *path,const char *src, -size_t len,struct strbuf *dst, -void *dco); -extern int async_query_available_blobs (const char *cmd,struct string_list *available_paths); -extern int renormalize_buffer (const struct index_state *istate, -const char *path,const char *src,size_t len, -struct strbuf *dst); +int convert_to_git (const struct index_state *istate, +const char *path,const char *src,size_t len, +struct strbuf *dst,int conv_flags); +int convert_to_working_tree (const char *path,const char *src, +size_t len,struct strbuf *dst); +int async_convert_to_working_tree (const char *path,const char *src, +size_t len,struct strbuf *dst, +void *dco); +int async_query_available_blobs (const char *cmd, +struct string_list *available_paths); +int renormalize_buffer (const struct index_state *istate, +const char *path,const char *src,size_t len, +struct strbuf *dst);static inline int would_convert_to_git (const struct index_state *istate,const char *path) {return convert_to_git (istate, path,NULL ,0 ,NULL ,0 ); }/* Precondition: would_convert_to_git_filter_fd(path) == true */ -extern void convert_to_git_filter_fd (const struct index_state *istate, -const char *path,int fd, -struct strbuf *dst, -int conv_flags); -extern int would_convert_to_git_filter_fd (const char *path); +void convert_to_git_filter_fd (const struct index_state *istate, +const char *path,int fd, +struct strbuf *dst, +int conv_flags); +int would_convert_to_git_filter_fd (const char *path);/***************************************************************** *
@@ -95,9 +96,10 @@ extern int would_convert_to_git_filter_fd(const char *path);struct stream_filter ;/* opaque */ -extern struct stream_filter *get_stream_filter (const char *path,const struct object_id *); -extern void free_stream_filter (struct stream_filter *); -extern int is_null_stream_filter (struct stream_filter *); +struct stream_filter *get_stream_filter (const char *path, +const struct object_id *); +void free_stream_filter (struct stream_filter *); +int is_null_stream_filter (struct stream_filter *);/* * Use as much input up to *isize_p and fill output up to *osize_p;
@@ -111,8 +113,8 @@ extern int is_null_stream_filter(struct stream_filter *);* such filters know there is no more input coming and it is time for * them to produce the remaining output based on the buffered input. */ -extern int stream_filter (struct stream_filter *, -const char *input,size_t *isize_p, -char *output,size_t *osize_p); +int stream_filter (struct stream_filter *, +const char *input,size_t *isize_p, +char *output,size_t *osize_p);#endif /* CONVERT_H */
Signature · 1 line
2.18.0.rc0.333.g22e6ee6cdf
PATCH v3 9/20 convert.c: remove an implicit dependency on the_index
In reply to Nguyễn Thái Ngọc DuyMake the convert API take an index_state instead of assuming the_index in convert.c. All external call sites are converted blindly to keep the patch simple and retain current behavior. Individual call sites may receive further updates to use the right index instead of the_index.
--- apply.c | 2 +- archive.c | 2 +- builtin/cat-file.c | 2 +- builtin/ls-files.c | 2 +- convert.c | 41 ++++++++++++++++++++++++----------------- convert.h | 15 ++++++++++----- diff.c | 2 +- entry.c | 6 +++--- merge-recursive.c | 2 +- sha1-file.c | 4 ++-- 10 files changed, 45 insertions(+), 33 deletions(-)
apply.c +1 -1
@@ -4333,7 +4333,7 @@ static int try_create_file(const char *path, unsigned int mode, const char *buf,if (fd <0 )return 1 ; -if (convert_to_working_tree (path, buf, size, &nbuf)) { +if (convert_to_working_tree (&the_index, path, buf, size, &nbuf)) { size = nbuf.len; buf = nbuf.buf; }
archive.c +1 -1
@@ -78,7 +78,7 @@ void *object_file_to_archive(const struct archiver_args *args,size_t size =0 ;strbuf_attach (&buf, buffer, *sizep, *sizep +1 ); -convert_to_working_tree (path, buf.buf, buf.len, &buf); +convert_to_working_tree (&the_index, path, buf.buf, buf.len, &buf);if (commit)format_subst (commit, buf.buf, buf.len, &buf); buffer =strbuf_detach (&buf, &size);
builtin/cat-file.c +1 -1
@@ -38,7 +38,7 @@ static int filter_object(const char *path, unsigned mode, oid_to_hex(oid), path);if ((type ==OBJ_BLOB ) &&S_ISREG (mode)) {struct strbuf strbuf =STRBUF_INIT ; -if (convert_to_working_tree (path, *buf, *size, &strbuf)) { +if (convert_to_working_tree (&the_index, path, *buf, *size, &strbuf)) {free (*buf); *size = strbuf.len; *buf =strbuf_detach (&strbuf,NULL );
builtin/ls-files.c +1 -1
@@ -63,7 +63,7 @@ static void write_eolinfo(const struct index_state *istate,struct stat st;const char *i_txt ="" ;const char *w_txt ="" ; -const char *a_txt =get_convert_attr_ascii (path); +const char *a_txt =get_convert_attr_ascii (&the_index, path);if (ce &&S_ISREG (ce->ce_mode)) i_txt =get_cached_convert_stats_ascii (istate, ce->name);
convert.c +24 -17
122 lines not on this page · raw message
convert.h +10 -5
@@ -60,15 +60,18 @@ extern char *check_roundtrip_encoding;const char *get_cached_convert_stats_ascii (const struct index_state *istate,const char *path);const char *get_wt_convert_stats_ascii (const char *path); -const char *get_convert_attr_ascii (const char *path); +const char *get_convert_attr_ascii (const struct index_state *istate, +const char *path);/* returns 1 if *dst was used */ int convert_to_git (const struct index_state *istate,const char *path,const char *src,size_t len,struct strbuf *dst,int conv_flags); -int convert_to_working_tree (const char *path,const char *src, +int convert_to_working_tree (const struct index_state *istate, +const char *path,const char *src,size_t len,struct strbuf *dst); -int async_convert_to_working_tree (const char *path,const char *src, +int async_convert_to_working_tree (const struct index_state *istate, +const char *path,const char *src,size_t len,struct strbuf *dst,void *dco); int async_query_available_blobs(const char *cmd,
@@ -86,7 +89,8 @@ void convert_to_git_filter_fd(const struct index_state *istate, const char *path, int fd, struct strbuf *dst, int conv_flags); -int would_convert_to_git_filter_fd (const char *path); +int would_convert_to_git_filter_fd (const struct index_state *istate, +const char *path);/***************************************************************** *
@@ -96,7 +100,8 @@ int would_convert_to_git_filter_fd(const char *path);struct stream_filter ;/* opaque */ -struct stream_filter *get_stream_filter (const char *path, +struct stream_filter *get_stream_filter (const struct index_state *istate, +const char *path,const struct object_id *);void free_stream_filter (struct stream_filter *);int is_null_stream_filter (struct stream_filter *);
diff.c +1 -1
@@ -3695,7 +3695,7 @@ static void prep_temp_blob(const char *path, struct diff_tempfile *temp, temp->tempfile =mks_tempfile_ts (tempfile.buf,strlen (base) +1 );if (!temp->tempfile)die_errno ("unable to create temp-file" ); -if (convert_to_working_tree (path, +if (convert_to_working_tree (&the_index, path, (const char *)blob, (size_t )size, &buf)) { blob = buf.buf; size = buf.len;
entry.c +3 -3
@@ -265,7 +265,7 @@ static int write_entry(struct cache_entry *ce,const struct submodule *sub; if (ce_mode_s_ifmt == S_IFREG) { - struct stream_filter *filter = get_stream_filter(ce->name, + struct stream_filter *filter = get_stream_filter(&the_index, ce->name, &ce->oid); if (filter && !streaming_write_entry(ce, path, filter,
@@ -313,14 +313,14 @@ static int write_entry(struct cache_entry *ce,* Convert from git internal format to working tree format */ if (dco && dco->state !=CE_NO_DELAY ) { - ret =async_convert_to_working_tree (ce->name, new_blob, + ret =async_convert_to_working_tree (&the_index, ce->name, new_blob, size, &buf, dco);if (ret &&string_list_has_string (&dco->paths, ce->name)) {free (new_blob); goto delayed; } }else - ret =convert_to_working_tree (ce->name, new_blob, size, &buf); + ret =convert_to_working_tree (&the_index, ce->name, new_blob, size, &buf);if (ret) {free (new_blob);
merge-recursive.c +1 -1
@@ -961,7 +961,7 @@ static int update_file_flags(struct merge_options *o, }if (S_ISREG (mode)) {struct strbuf strbuf =STRBUF_INIT ; -if (convert_to_working_tree (path, buf, size, &strbuf)) { +if (convert_to_working_tree (&the_index, path, buf, size, &strbuf)) {free (buf); size = strbuf.len; buf =strbuf_detach (&strbuf,NULL );
sha1-file.c +2 -2
@@ -1860,7 +1860,7 @@ static int index_stream_convert_blob(struct object_id *oid, int fd,struct strbuf sbuf =STRBUF_INIT ;assert (path); -assert (would_convert_to_git_filter_fd (path)); +assert (would_convert_to_git_filter_fd (&the_index, path));convert_to_git_filter_fd (&the_index, path, fd, &sbuf,get_conv_flags (flags));
@@ -1950,7 +1950,7 @@ int index_fd(struct object_id *oid, int fd, struct stat *st,* Call xsize_t() only when needed to avoid potentially unnecessary * die() for large files. */ -if (type ==OBJ_BLOB && path &&would_convert_to_git_filter_fd (path)) +if (type ==OBJ_BLOB && path &&would_convert_to_git_filter_fd (&the_index, path)) ret =index_stream_convert_blob (oid, fd, path, flags);else if (!S_ISREG (st->st_mode)) ret =index_pipe (oid, fd, type, path, flags);
Signature · 1 line
2.18.0.rc0.333.g22e6ee6cdf
PATCH v3 10/20 dir.c: remove an implicit dependency on the_index in pathspec code
In reply to Nguyễn Thái Ngọc DuyMake the match_patchspec API and friends take an index_state instead of assuming the_index in dir.c. All external call sites are converted blindly to keep the patch simple and retain current behavior. Individual call sites may receive further updates to use the right index instead of the_index.
--- archive.c | 2 +- builtin/add.c | 6 +++--- builtin/checkout.c | 2 +- builtin/clean.c | 2 +- builtin/commit.c | 2 +- builtin/grep.c | 6 +++--- builtin/ls-files.c | 6 +++--- builtin/rm.c | 2 +- builtin/submodule--helper.c | 2 +- builtin/update-index.c | 2 +- diff-lib.c | 4 ++-- dir.c | 27 ++++++++++++++++----------- dir.h | 16 ++++++++++------ pathspec.c | 2 +- preload-index.c | 2 +- read-cache.c | 2 +- rerere.c | 2 +- resolve-undo.c | 2 +- revision.c | 2 +- submodule.c | 2 +- wt-status.c | 6 +++--- 21 files changed, 54 insertions(+), 45 deletions(-)
archive.c +1 -1
@@ -312,7 +312,7 @@ static int reject_entry(const struct object_id *oid, struct strbuf *base,struct strbuf sb =STRBUF_INIT ;strbuf_addbuf (&sb, base);strbuf_addstr (&sb, filename); -if (!match_pathspec (context, sb.buf, sb.len,0 ,NULL ,1 )) +if (!match_pathspec (&the_index, context, sb.buf, sb.len,0 ,NULL ,1 )) ret =READ_TREE_RECURSIVE ;strbuf_release (&sb); }
builtin/add.c +3 -3
@@ -40,7 +40,7 @@ static void chmod_pathspec(struct pathspec *pathspec, char flip)for (i =0 ; i < active_nr; i++) {struct cache_entry *ce = active_cache[i]; -if (pathspec && !ce_path_match (ce, pathspec,NULL )) +if (pathspec && !ce_path_match (&the_index, ce, pathspec,NULL ))continue ;if (chmod_cache_entry (ce, flip) <0 )
@@ -135,7 +135,7 @@ static int renormalize_tracked_files(const struct pathspec *pathspec, int flags)continue ;/* do not touch unmerged paths */ if (!S_ISREG (ce->ce_mode) && !S_ISLNK (ce->ce_mode))continue ;/* do not touch non blobs */ -if (pathspec && !ce_path_match (ce, pathspec,NULL )) +if (pathspec && !ce_path_match (&the_index, ce, pathspec,NULL ))continue ; retval |=add_file_to_cache (ce->name, flags |HASH_RENORMALIZE ); }
@@ -155,7 +155,7 @@ static char *prune_directory(struct dir_struct *dir, struct pathspec *pathspec, i = dir->nr;while (--i >=0 ) {struct dir_entry *entry = *src++; -if (dir_path_match (entry, pathspec, prefix, seen)) +if (dir_path_match (&the_index, entry, pathspec, prefix, seen)) *dst++ = entry; } dir->nr = dst - dir->entries;
builtin/checkout.c +1 -1
@@ -316,7 +316,7 @@ static int checkout_paths(const struct checkout_opts *opts,* match_pathspec() for _all_ entries when * opts->source_tree != NULL. */ -if (ce_path_match (ce, &opts->pathspec, ps_matched)) +if (ce_path_match (&the_index, ce, &opts->pathspec, ps_matched)) ce->ce_flags |=CE_MATCHED ; }
builtin/clean.c +1 -1
@@ -981,7 +981,7 @@ int cmd_clean(int argc, const char **argv, const char *prefix)continue ;if (pathspec.nr) - matches =dir_path_match (ent, &pathspec,0 ,NULL ); + matches =dir_path_match (&the_index, ent, &pathspec,0 ,NULL );if (pathspec.nr && !matches)continue ;
builtin/commit.c +1 -1
@@ -238,7 +238,7 @@ static int list_paths(struct string_list *list, const char *with_tree,if (ce->ce_flags &CE_UPDATE )continue ; -if (!ce_path_match (ce, pattern, m)) +if (!ce_path_match (&the_index, ce, pattern, m))continue ; item =string_list_insert (list, ce->name);if (ce_skip_worktree (ce))
builtin/grep.c +3 -3
@@ -497,7 +497,7 @@ static int grep_cache(struct grep_opt *opt, struct repository *repo,strbuf_addstr (&name, ce->name);if (S_ISREG (ce->ce_mode) && -match_pathspec (pathspec, name.buf, name.len,0 ,NULL , +match_pathspec (&the_index, pathspec, name.buf, name.len,0 ,NULL ,S_ISDIR (ce->ce_mode) ||S_ISGITLINK (ce->ce_mode))) {/*
@@ -515,7 +515,7 @@ static int grep_cache(struct grep_opt *opt, struct repository *repo, hit |=grep_file (opt, name.buf); } }else if (recurse_submodules &&S_ISGITLINK (ce->ce_mode) && -submodule_path_match (pathspec, name.buf,NULL )) { +submodule_path_match (&the_index, pathspec, name.buf,NULL )) { hit |=grep_submodule (opt, repo, pathspec,NULL , ce->name, ce->name); }else {continue ;
@@ -678,7 +678,7 @@ static int grep_directory(struct grep_opt *opt, const struct pathspec *pathspec,fill_directory (&dir, &the_index, pathspec);for (i =0 ; i < dir.nr; i++) { -if (!dir_path_match (dir.entries[i], pathspec,0 ,NULL )) +if (!dir_path_match (&the_index, dir.entries[i], pathspec,0 ,NULL ))continue ; hit |=grep_file (opt, dir.entries[i]->name);if (hit && opt->status_only)
builtin/ls-files.c +3 -3
@@ -128,7 +128,7 @@ static void show_dir_entry(const char *tag, struct dir_entry *ent)if (len > ent->len)die ("git ls-files: internal error - directory entry not superset of prefix" ); -if (!dir_path_match (ent, &pathspec, len, ps_matched)) +if (!dir_path_match (&the_index, ent, &pathspec, len, ps_matched))return ;fputs (tag, stdout);
@@ -228,7 +228,7 @@ static void show_ce(struct repository *repo, struct dir_struct *dir,if (recurse_submodules &&S_ISGITLINK (ce->ce_mode) &&is_submodule_active (repo, ce->name)) {show_submodule (repo, dir, ce->name); - }else if (match_pathspec (&pathspec, fullname,strlen (fullname), + }else if (match_pathspec (&the_index, &pathspec, fullname,strlen (fullname), max_prefix_len, ps_matched,S_ISDIR (ce->ce_mode) ||S_ISGITLINK (ce->ce_mode))) {
@@ -264,7 +264,7 @@ static void show_ru_info(const struct index_state *istate) len =strlen (path);if (len < max_prefix_len)continue ;/* outside of the prefix */ -if (!match_pathspec (&pathspec, path, len, +if (!match_pathspec (&the_index, &pathspec, path, len, max_prefix_len, ps_matched,0 ))continue ;/* uninterested */ for (i =0 ; i <3 ; i++) {
builtin/rm.c +1 -1
@@ -278,7 +278,7 @@ int cmd_rm(int argc, const char **argv, const char *prefix)for (i =0 ; i < active_nr; i++) {const struct cache_entry *ce = active_cache[i]; -if (!ce_path_match (ce, &pathspec, seen)) +if (!ce_path_match (&the_index, ce, &pathspec, seen))continue ;ALLOC_GROW (list.entry, list.nr +1 , list.alloc); list.entry[list.nr].name =xstrdup (ce->name);
builtin/submodule--helper.c +1 -1
@@ -330,7 +330,7 @@ static int module_list_compute(int argc, const char **argv,for (i =0 ; i < active_nr; i++) {const struct cache_entry *ce = active_cache[i]; -if (!match_pathspec (pathspec, ce->name,ce_namelen (ce), +if (!match_pathspec (&the_index, pathspec, ce->name,ce_namelen (ce),0 , ps_matched,1 ) || !S_ISGITLINK (ce->ce_mode))continue ;
builtin/update-index.c +1 -1
@@ -751,7 +751,7 @@ static int do_reupdate(int ac, const char **av,int save_nr;char *path; -if (ce_stage (ce) || !ce_path_match (ce, &pathspec,NULL )) +if (ce_stage (ce) || !ce_path_match (&the_index, ce, &pathspec,NULL ))continue ;if (has_head) old = read_one_ent(NULL, &head_oid,
diff-lib.c +2 -2
@@ -109,7 +109,7 @@ int run_diff_files(struct rev_info *revs, unsigned int option)if (diff_can_quit_early (&revs->diffopt))break ; -if (!ce_path_match (ce, &revs->prune_data,NULL )) +if (!ce_path_match (&the_index, ce, &revs->prune_data,NULL ))continue ;if (ce_stage (ce)) {
@@ -470,7 +470,7 @@ static int oneway_diff(const struct cache_entry * const *src,if (tree == o->df_conflict_entry) tree =NULL ; -if (ce_path_match (idx ? idx : tree, &revs->prune_data,NULL )) { +if (ce_path_match (&the_index, idx ? idx : tree, &revs->prune_data,NULL )) {do_oneway_diff (o, idx, tree);if (diff_can_quit_early (&revs->diffopt)) { o->exiting_early =1 ;
dir.c +16 -11
@@ -275,12 +275,13 @@ static int do_read_blob(const struct object_id *oid, struct oid_stat *oid_stat,#define DO_MATCH_DIRECTORY (1<<1)#define DO_MATCH_SUBMODULE (1<<2) -static int match_attrs (const char *name,int namelen, +static int match_attrs (const struct index_state *istate, +const char *name,int namelen,const struct pathspec_item *item) {int i; -git_check_attr (&the_index, name, item->attr_check); +git_check_attr (istate, name, item->attr_check);for (i =0 ; i < item->attr_match_nr; i++) {const char *value;int matched;
@@ -317,7 +318,8 @@ static int match_attrs(const char *name, int namelen,* * It returns 0 when there is no match. */ -static int match_pathspec_item (const struct pathspec_item *item,int prefix, +static int match_pathspec_item (const struct index_state *istate, +const struct pathspec_item *item,int prefix,const char *name,int namelen,unsigned flags) {/* name/namelen has prefix cut off by caller */
@@ -357,7 +359,7 @@ static int match_pathspec_item(const struct pathspec_item *item, int prefix, strncmp(item->match, name - prefix, item->prefix))return 0 ; -if (item->attr_match_nr && !match_attrs (name, namelen, item)) +if (item->attr_match_nr && !match_attrs (istate, name, namelen, item))return 0 ;/* If the match was just the prefix, we matched */
@@ -425,7 +427,8 @@ static int match_pathspec_item(const struct pathspec_item *item, int prefix,* pathspec did not match any names, which could indicate that the * user mistyped the nth pathspec. */ -static int do_match_pathspec (const struct pathspec *ps, +static int do_match_pathspec (const struct index_state *istate, +const struct pathspec *ps,const char *name,int namelen,int prefix,char *seen,unsigned flags)
@@ -471,7 +474,7 @@ static int do_match_pathspec(const struct pathspec *ps,*/ if (seen && ps->items[i].magic &PATHSPEC_EXCLUDE ) seen[i] =MATCHED_FNMATCH ; - how =match_pathspec_item (ps->items+i, prefix, name, + how =match_pathspec_item (istate, ps->items+i, prefix, name, namelen, flags); if (ps->recursive && (ps->magic & PATHSPEC_MAXDEPTH) &&
@@ -495,17 +498,18 @@ static int do_match_pathspec(const struct pathspec *ps,return retval; } -int match_pathspec (const struct pathspec *ps, +int match_pathspec (const struct index_state *istate, +const struct pathspec *ps,const char *name,int namelen,int prefix,char *seen,int is_dir) {int positive, negative;unsigned flags = is_dir ?DO_MATCH_DIRECTORY :0 ; - positive =do_match_pathspec (ps, name, namelen, + positive =do_match_pathspec (istate, ps, name, namelen, prefix, seen, flags);if (!(ps->magic &PATHSPEC_EXCLUDE ) || !positive)return positive; - negative =do_match_pathspec (ps, name, namelen, + negative =do_match_pathspec (istate, ps, name, namelen, prefix, seen, flags |DO_MATCH_EXCLUDE );return negative ?0 : positive;
@@ -514,11 +518,12 @@ int match_pathspec(const struct pathspec *ps,/** * Check if a submodule is a superset of the pathspec */ -int submodule_path_match (const struct pathspec *ps, +int submodule_path_match (const struct index_state *istate, +const struct pathspec *ps,const char *submodule_name,char *seen) { - int matched = do_match_pathspec(ps, submodule_name, + int matched = do_match_pathspec(istate, ps, submodule_name, strlen(submodule_name), 0, seen, DO_MATCH_DIRECTORY |
dir.h +10 -6
@@ -216,7 +216,8 @@ extern int count_slashes(const char *s);extern int simple_length (const char *match);extern int no_wildcard (const char *string);extern char *common_prefix (const struct pathspec *pathspec); -extern int match_pathspec (const struct pathspec *pathspec, +extern int match_pathspec (const struct index_state *istate, +const struct pathspec *pathspec,const char *name,int namelen,int prefix,char *seen,int is_dir);extern int report_path_error (const char *ps_matched,const struct pathspec *pathspec,const char *prefix);
@@ -326,25 +327,28 @@ extern int git_fnmatch(const struct pathspec_item *item, const char *pattern, const char *string, int prefix); -extern int submodule_path_match (const struct pathspec *ps, +extern int submodule_path_match (const struct index_state *istate, +const struct pathspec *ps,const char *submodule_name,char *seen); -static inline int ce_path_match (const struct cache_entry *ce, +static inline int ce_path_match (const struct index_state *istate, +const struct cache_entry *ce,const struct pathspec *pathspec,char *seen) { -return match_pathspec (pathspec, ce->name,ce_namelen (ce),0 , seen, +return match_pathspec (istate, pathspec, ce->name,ce_namelen (ce),0 , seen,S_ISDIR (ce->ce_mode) ||S_ISGITLINK (ce->ce_mode)); } -static inline int dir_path_match (const struct dir_entry *ent, +static inline int dir_path_match (const struct index_state *istate, +const struct dir_entry *ent,const struct pathspec *pathspec,int prefix,char *seen) {int has_trailing_dir = ent->len && ent->name[ent->len -1 ] =='/' ;int len = has_trailing_dir ? ent->len -1 : ent->len; -return match_pathspec (pathspec, ent->name, len, prefix, seen, +return match_pathspec (istate, pathspec, ent->name, len, prefix, seen, has_trailing_dir); }
pathspec.c +1 -1
@@ -37,7 +37,7 @@ void add_pathspec_matches_against_index(const struct pathspec *pathspec,return ;for (i =0 ; i < istate->cache_nr; i++) {const struct cache_entry *ce = istate->cache[i]; -ce_path_match (ce, pathspec, seen); +ce_path_match (&the_index, ce, pathspec, seen); } }
preload-index.c +1 -1
@@ -58,7 +58,7 @@ static void *preload_thread(void *_data)continue ;if (ce->ce_flags &CE_FSMONITOR_VALID )continue ; -if (!ce_path_match (ce, &p->pathspec,NULL )) +if (!ce_path_match (&the_index, ce, &p->pathspec,NULL ))continue ;if (threaded_has_symlink_leading_path (&cache, ce->name,ce_namelen (ce)))continue ;
read-cache.c +1 -1
@@ -1415,7 +1415,7 @@ int refresh_index(struct index_state *istate, unsigned int flags,if (ignore_submodules &&S_ISGITLINK (ce->ce_mode))continue ; -if (pathspec && !ce_path_match (ce, pathspec, seen)) +if (pathspec && !ce_path_match (&the_index, ce, pathspec, seen)) filtered =1 ;if (ce_stage (ce)) {
rerere.c +1 -1
@@ -1118,7 +1118,7 @@ int rerere_forget(struct pathspec *pathspec)find_conflict (&conflict);for (i =0 ; i < conflict.nr; i++) {struct string_list_item *it = &conflict.items[i]; -if (!match_pathspec (pathspec, it->string, +if (!match_pathspec (&the_index, pathspec, it->string,strlen (it->string),0 ,NULL ,0 ))continue ;rerere_forget_one_path (it->string, &merge_rr);
resolve-undo.c +1 -1
@@ -186,7 +186,7 @@ void unmerge_index(struct index_state *istate, const struct pathspec *pathspec)for (i =0 ; i < istate->cache_nr; i++) {const struct cache_entry *ce = istate->cache[i]; -if (!ce_path_match (ce, pathspec,NULL )) +if (!ce_path_match (&the_index, ce, pathspec,NULL ))continue ; i =unmerge_index_entry_at (istate, i); }
revision.c +1 -1
@@ -1501,7 +1501,7 @@ static void prepare_show_merge(struct rev_info *revs)const struct cache_entry *ce = active_cache[i];if (!ce_stage (ce))continue ; -if (ce_path_match (ce, &revs->prune_data,NULL )) { +if (ce_path_match (&the_index, ce, &revs->prune_data,NULL )) { prune_num++;REALLOC_ARRAY (prune, prune_num); prune[prune_num-2 ] = ce->name;
submodule.c +1 -1
@@ -258,7 +258,7 @@ int is_submodule_active(struct repository *repo, const char *path) }parse_pathspec (&ps,0 ,0 ,NULL , args.argv); - ret =match_pathspec (&ps, path,strlen (path),0 ,NULL ,1 ); + ret =match_pathspec (&the_index, &ps, path,strlen (path),0 ,NULL ,1 );argv_array_clear (&args);clear_pathspec (&ps);
wt-status.c +3 -3
@@ -647,7 +647,7 @@ static void wt_status_collect_changes_initial(struct wt_status *s)struct wt_status_change_data *d;const struct cache_entry *ce = active_cache[i]; -if (!ce_path_match (ce, &s->pathspec,NULL )) +if (!ce_path_match (&the_index, ce, &s->pathspec,NULL ))continue ;if (ce_intent_to_add (ce))continue ;
@@ -703,7 +703,7 @@ static void wt_status_collect_untracked(struct wt_status *s)for (i =0 ; i < dir.nr; i++) {struct dir_entry *ent = dir.entries[i];if (cache_name_is_other (ent->name, ent->len) && -dir_path_match (ent, &s->pathspec,0 ,NULL )) +dir_path_match (&the_index, ent, &s->pathspec,0 ,NULL ))string_list_insert (&s->untracked, ent->name);free (ent); }
@@ -711,7 +711,7 @@ static void wt_status_collect_untracked(struct wt_status *s)for (i =0 ; i < dir.ignored_nr; i++) {struct dir_entry *ent = dir.ignored[i];if (cache_name_is_other (ent->name, ent->len) && -dir_path_match (ent, &s->pathspec,0 ,NULL )) +dir_path_match (&the_index, ent, &s->pathspec,0 ,NULL ))string_list_insert (&s->ignored, ent->name);free (ent); }
Signature · 1 line
2.18.0.rc0.333.g22e6ee6cdf
PATCH v3 11/20 ls-files: correct index argument to get_convert_attr_ascii()
In reply to Nguyễn Thái Ngọc Duywrite_eolinfo() does take an istate as function argument and it should be used instead of the_index.
--- builtin/ls-files.c | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-)
builtin/ls-files.c +9 -8
@@ -63,7 +63,7 @@ static void write_eolinfo(const struct index_state *istate,struct stat st;const char *i_txt ="" ;const char *w_txt ="" ; -const char *a_txt =get_convert_attr_ascii (&the_index, path); +const char *a_txt =get_convert_attr_ascii (istate, path);if (ce &&S_ISREG (ce->ce_mode)) i_txt =get_cached_convert_stats_ascii (istate, ce->name);
@@ -121,18 +121,19 @@ static void print_debug(const struct cache_entry *ce) } } -static void show_dir_entry (const char *tag,struct dir_entry *ent) +static void show_dir_entry (const struct index_state *istate, +const char *tag,struct dir_entry *ent) {int len = max_prefix_len;if (len > ent->len)die ("git ls-files: internal error - directory entry not superset of prefix" ); -if (!dir_path_match (&the_index, ent, &pathspec, len, ps_matched)) +if (!dir_path_match (istate, ent, &pathspec, len, ps_matched))return ;fputs (tag, stdout); -write_eolinfo (NULL ,NULL , ent->name); +write_eolinfo (istate,NULL , ent->name);write_name (ent->name); }
@@ -145,7 +146,7 @@ static void show_other_files(const struct index_state *istate,struct dir_entry *ent = dir->entries[i];if (!index_name_is_other (istate, ent->name, ent->len))continue ; -show_dir_entry (tag_other, ent); +show_dir_entry (istate, tag_other, ent); } }
@@ -196,7 +197,7 @@ static void show_killed_files(const struct index_state *istate, } }if (killed) -show_dir_entry (tag_killed, dir->entries[i]); +show_dir_entry (istate, tag_killed, dir->entries[i]); } }
@@ -228,7 +229,7 @@ static void show_ce(struct repository *repo, struct dir_struct *dir,if (recurse_submodules &&S_ISGITLINK (ce->ce_mode) &&is_submodule_active (repo, ce->name)) {show_submodule (repo, dir, ce->name); - }else if (match_pathspec (&the_index, &pathspec, fullname,strlen (fullname), + }else if (match_pathspec (repo->index, &pathspec, fullname,strlen (fullname), max_prefix_len, ps_matched,S_ISDIR (ce->ce_mode) ||S_ISGITLINK (ce->ce_mode))) {
@@ -264,7 +265,7 @@ static void show_ru_info(const struct index_state *istate) len =strlen (path);if (len < max_prefix_len)continue ;/* outside of the prefix */ -if (!match_pathspec (&the_index, &pathspec, path, len, +if (!match_pathspec (istate, &pathspec, path, len, max_prefix_len, ps_matched,0 ))continue ;/* uninterested */ for (i =0 ; i <3 ; i++) {
Signature · 1 line
2.18.0.rc0.333.g22e6ee6cdf
PATCH v3 12/20 pathspec.c: use the right index instead of the_index
In reply to Nguyễn Thái Ngọc Duy--- pathspec.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
pathspec.c +1 -1
@@ -37,7 +37,7 @@ void add_pathspec_matches_against_index(const struct pathspec *pathspec,return ;for (i =0 ; i < istate->cache_nr; i++) {const struct cache_entry *ce = istate->cache[i]; -ce_path_match (&the_index, ce, pathspec, seen); +ce_path_match (istate, ce, pathspec, seen); } }
Signature · 1 line
2.18.0.rc0.333.g22e6ee6cdf
PATCH v3 13/20 submodule.c: use the right index instead of the_index
In reply to Nguyễn Thái Ngọc Duy--- submodule.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)
submodule.c +4 -4
@@ -93,7 +93,7 @@ int update_path_in_gitmodules(const char *oldpath, const char *newpath)if (!file_exists (GITMODULES_FILE ))/* Do nothing without .gitmodules */ return -1 ; -if (is_gitmodules_unmerged (&the_index)) +if (is_gitmodules_unmerged (the_repository->index))die (_ ("Cannot change unmerged .gitmodules, resolve merge conflicts first" )); submodule =submodule_from_path (the_repository, &null_oid, oldpath);
@@ -127,7 +127,7 @@ int remove_path_from_gitmodules(const char *path)if (!file_exists (GITMODULES_FILE ))/* Do nothing without .gitmodules */ return -1 ; -if (is_gitmodules_unmerged (&the_index)) +if (is_gitmodules_unmerged (the_repository->index))die (_ ("Cannot change unmerged .gitmodules, resolve merge conflicts first" )); submodule =submodule_from_path (the_repository, &null_oid, path);
@@ -188,7 +188,7 @@ void set_diffopt_flags_from_submodule_config(struct diff_options *diffopt,if (ignore)handle_ignore_submodules_arg (diffopt, ignore); -else if (is_gitmodules_unmerged (&the_index)) +else if (is_gitmodules_unmerged (the_repository->index)) diffopt->flags.ignore_submodules =1 ; } }
@@ -258,7 +258,7 @@ int is_submodule_active(struct repository *repo, const char *path) }parse_pathspec (&ps,0 ,0 ,NULL , args.argv); - ret =match_pathspec (&the_index, &ps, path,strlen (path),0 ,NULL ,1 ); + ret =match_pathspec (repo->index, &ps, path,strlen (path),0 ,NULL ,1 );argv_array_clear (&args);clear_pathspec (&ps);
Signature · 1 line
2.18.0.rc0.333.g22e6ee6cdf
PATCH v3 14/20 entry.c: use the right index instead of the_index
In reply to Nguyễn Thái Ngọc Duycheckout-index.c needs update because if checkout->istate is NULL, ie_match_stat() will crash. Previously this is ie_match_stat(&the_index, ..) so it will not crash, but it is not technically correct either.
--- builtin/checkout-index.c | 1 + entry.c | 10 ++++++---- 2 files changed, 7 insertions(+), 4 deletions(-)
builtin/checkout-index.c +1 -0
@@ -190,6 +190,7 @@ int cmd_checkout_index(int argc, const char **argv, const char *prefix) argc =parse_options (argc, argv, prefix, builtin_checkout_index_options, builtin_checkout_index_usage,0 ); + state.istate = &the_index; state.force = force; state.quiet = quiet; state.not_new = not_new;
entry.c +6 -4
@@ -1,3 +1,4 @@ +#define NO_THE_INDEX_COMPATIBILITY_MACROS #include "cache.h" #include "blob.h" #include "dir.h"
@@ -265,7 +266,7 @@ static int write_entry(struct cache_entry *ce,const struct submodule *sub; if (ce_mode_s_ifmt == S_IFREG) { - struct stream_filter *filter = get_stream_filter(&the_index, ce->name, + struct stream_filter *filter = get_stream_filter(state->istate, ce->name, &ce->oid); if (filter && !streaming_write_entry(ce, path, filter,
@@ -313,14 +314,14 @@ static int write_entry(struct cache_entry *ce,* Convert from git internal format to working tree format */ if (dco && dco->state !=CE_NO_DELAY ) { - ret =async_convert_to_working_tree (&the_index, ce->name, new_blob, + ret =async_convert_to_working_tree (state->istate, ce->name, new_blob, size, &buf, dco);if (ret &&string_list_has_string (&dco->paths, ce->name)) {free (new_blob); goto delayed; } }else - ret =convert_to_working_tree (&the_index, ce->name, new_blob, size, &buf); + ret =convert_to_working_tree (state->istate, ce->name, new_blob, size, &buf);if (ret) {free (new_blob);
@@ -421,7 +422,8 @@ int checkout_entry(struct cache_entry *ce,if (!check_path (path.buf, path.len, &st, state->base_dir_len)) {const struct submodule *sub; -unsigned changed =ce_match_stat (ce, &st,CE_MATCH_IGNORE_VALID |CE_MATCH_IGNORE_SKIP_WORKTREE ); +unsigned changed =ie_match_stat (state->istate, ce, &st, +CE_MATCH_IGNORE_VALID |CE_MATCH_IGNORE_SKIP_WORKTREE );/* * Needs to be checked before !changed returns early, * as the possibly empty directory was not changed
Signature · 1 line
2.18.0.rc0.333.g22e6ee6cdf
PATCH v3 15/20 attr: remove index from git_attr_set_direction()
In reply to Nguyễn Thái Ngọc DuySince attr checking API now take the index, there's no need to set an index in advance with this call. Most call sites are straightforward because they either pass the_index or NULL (which defaults back to the_index previously). There's only one suspicious call site in unpack-trees.c where it sets a different index.
This code in unpack-trees is about to checking out entries from the new/temporary index after merging is done in it. The attributes will be used by entry.c code to do crlf conversion if needed. entry.c now respects struct checkout's istate field, and this field is correctly set in unpack-trees.c, there should be no regression from this change.
--- archive.c | 2 +- attr.c | 15 +++------------ attr.h | 3 +-- builtin/check-attr.c | 2 +- unpack-trees.c | 4 ++-- 5 files changed, 8 insertions(+), 18 deletions(-)
archive.c +1 -1
@@ -273,7 +273,7 @@ int write_archive_entries(struct archiver_args *args,init_tree_desc (&t, args->tree->buffer, args->tree->size);if (unpack_trees (1 , &t, &opts))return -1 ; -git_attr_set_direction (GIT_ATTR_INDEX , &the_index); +git_attr_set_direction (GIT_ATTR_INDEX ); } err = read_tree_recursive(args->tree, "", 0, 0, &args->pathspec,
attr.c +3 -12
@@ -708,10 +708,8 @@ static struct attr_stack *read_attr_from_array(const char **list)* another thread could potentially be calling into the attribute system. */ static enum git_attr_direction direction; -static const struct index_state *use_index; -void git_attr_set_direction (enum git_attr_direction new_direction, -const struct index_state *istate) +void git_attr_set_direction (enum git_attr_direction new_direction) {if (is_bare_repository () && new_direction !=GIT_ATTR_INDEX )BUG ("non-INDEX attr direction in a bare repo" );
@@ -720,7 +718,6 @@ void git_attr_set_direction(enum git_attr_direction new_direction,drop_all_attr_stacks (); direction = new_direction; - use_index = istate; }static struct attr_stack *read_attr_from_file (const char *path,int macro_ok)
@@ -750,17 +747,11 @@ static struct attr_stack *read_attr_from_index(const struct index_state *istate,struct attr_stack *res;char *buf, *sp;int lineno =0 ; -const struct index_state *to_read_from; -/* -* Temporary workaround for c24f3abace (apply: file commited -* with CRLF should roundtrip diff and apply - 2017-08-19) -*/ - to_read_from = use_index ? use_index : istate; -if (!to_read_from) +if (!istate)return NULL ; - buf =read_blob_data_from_index (to_read_from, path,NULL ); + buf =read_blob_data_from_index (istate, path,NULL );if (!buf)return NULL ;
attr.h +1 -2
@@ -77,8 +77,7 @@ enum git_attr_direction {GIT_ATTR_CHECKOUT , GIT_ATTR_INDEX }; -void git_attr_set_direction (enum git_attr_direction new_direction, -const struct index_state *istate); +void git_attr_set_direction (enum git_attr_direction new_direction);void attr_start (void );
builtin/check-attr.c +1 -1
@@ -120,7 +120,7 @@ int cmd_check_attr(int argc, const char **argv, const char *prefix) }if (cached_attrs) -git_attr_set_direction (GIT_ATTR_INDEX ,NULL ); +git_attr_set_direction (GIT_ATTR_INDEX ); doubledash =-1 ;for (i =0 ; doubledash <0 && i < argc; i++) {
unpack-trees.c +2 -2
@@ -361,7 +361,7 @@ static int check_updates(struct unpack_trees_options *o) progress =get_progress (o);if (o->update) -git_attr_set_direction (GIT_ATTR_CHECKOUT , index); +git_attr_set_direction (GIT_ATTR_CHECKOUT );if (should_update_submodules () && o->update && !o->dry_run)load_gitmodules_file (index,NULL );
@@ -421,7 +421,7 @@ static int check_updates(struct unpack_trees_options *o)stop_progress (&progress); errs |=finish_delayed_checkout (&state);if (o->update) -git_attr_set_direction (GIT_ATTR_CHECKIN ,NULL ); +git_attr_set_direction (GIT_ATTR_CHECKIN );return errs !=0 ; }
Signature · 1 line
2.18.0.rc0.333.g22e6ee6cdf
Re: [PATCH v3 15/20] attr: remove index from git_attr_set_direction()
Quoted 11 lines · Nguyễn Thái Ngọc Duy
Since attr checking API now take the index, there's no need to set an index in advance with this call. Most call sites are straightforward because they either pass
the_indexor NULL (which defaults back tothe_indexpreviously). There's only one suspicious call site inunpack-trees.cwhere it sets a different index.This code in unpack-trees is about to checking out entries from the new/temporary index after merging is done in it. The attributes will be used by
entry.ccode to do crlf conversion if needed.entry.cnow respects struct checkout's istate field, and this field is correctly set inunpack-trees.c, there should be no regression from this change.
Nice! Much cleaner.
Quoted 119 lines
Signed-off-by Nguyễn Thái Ngọc Duy <[email protected]>--- archive.c | 2 +- attr.c | 15 +++------------ attr.h | 3 +-- builtin/check-attr.c | 2 +- unpack-trees.c | 4 ++-- 5 files changed, 8 insertions(+), 18 deletions(-)archive.c +1
-1@@ -273,7 +273,7 @@ int write_archive_entries(struct archiver_args *args,init_tree_desc (&t, args->tree->buffer, args->tree->size);if (unpack_trees (1 , &t, &opts))return -1 ; -git_attr_set_direction (GIT_ATTR_INDEX , &the_index); +git_attr_set_direction (GIT_ATTR_INDEX ); } err = read_tree_recursive(args->tree, "", 0, 0, &args->pathspec,attr.c +3
-12@@ -708,10 +708,8 @@ static struct attr_stack *read_attr_from_array(const char **list)* another thread could potentially be calling into the attribute system. */ static enum git_attr_direction direction; -static const struct index_state *use_index; -void git_attr_set_direction (enum git_attr_direction new_direction, -const struct index_state *istate) +void git_attr_set_direction (enum git_attr_direction new_direction) {if (is_bare_repository () && new_direction !=GIT_ATTR_INDEX )BUG ("non-INDEX attr direction in a bare repo" );@@ -720,7 +718,6 @@ void git_attr_set_direction(enum git_attr_direction new_direction,drop_all_attr_stacks (); direction = new_direction; - use_index = istate; }static struct attr_stack *read_attr_from_file (const char *path,int macro_ok)@@ -750,17 +747,11 @@ static struct attr_stack *read_attr_from_index(const struct index_state *istate,struct attr_stack *res;char *buf, *sp;int lineno =0 ; -const struct index_state *to_read_from; -/* -* Temporary workaround for c24f3abace (apply: file commited -* with CRLF should roundtrip diff and apply - 2017-08-19) -*/ - to_read_from = use_index ? use_index : istate; -if (!to_read_from) +if (!istate)return NULL ; - buf =read_blob_data_from_index (to_read_from, path,NULL ); + buf =read_blob_data_from_index (istate, path,NULL );if (!buf)return NULL ;attr.h +1
-2@@ -77,8 +77,7 @@ enum git_attr_direction {GIT_ATTR_CHECKOUT , GIT_ATTR_INDEX }; -void git_attr_set_direction (enum git_attr_direction new_direction, -const struct index_state *istate); +void git_attr_set_direction (enum git_attr_direction new_direction);void attr_start (void );builtin/check-attr.c +1
-1@@ -120,7 +120,7 @@ int cmd_check_attr(int argc, const char **argv, const char *prefix) }if (cached_attrs) -git_attr_set_direction (GIT_ATTR_INDEX ,NULL ); +git_attr_set_direction (GIT_ATTR_INDEX ); doubledash =-1 ;for (i =0 ; doubledash <0 && i < argc; i++) {unpack-trees.c +2
-2@@ -361,7 +361,7 @@ static int check_updates(struct unpack_trees_options *o) progress =get_progress (o);if (o->update) -git_attr_set_direction (GIT_ATTR_CHECKOUT , index); +git_attr_set_direction (GIT_ATTR_CHECKOUT );if (should_update_submodules () && o->update && !o->dry_run)load_gitmodules_file (index,NULL );@@ -421,7 +421,7 @@ static int check_updates(struct unpack_trees_options *o)stop_progress (&progress); errs |=finish_delayed_checkout (&state);if (o->update) -git_attr_set_direction (GIT_ATTR_CHECKIN ,NULL ); +git_attr_set_direction (GIT_ATTR_CHECKIN );return errs !=0 ; }Signature · 1 line
2.18.0.rc0.333.g22e6ee6cdf
Signature · 1 line
Brandon Williams
PATCH v3 16/20 preload-index.c: use the right index instead of the_index
In reply to Nguyễn Thái Ngọc Duy--- preload-index.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
preload-index.c +2 -1
@@ -1,6 +1,7 @@/* * Copyright (C) 2008 Linus Torvalds */ +#define NO_THE_INDEX_COMPATIBILITY_MACROS #include "cache.h" #include "pathspec.h" #include "dir.h"
@@ -58,7 +59,7 @@ static void *preload_thread(void *_data)continue ;if (ce->ce_flags &CE_FSMONITOR_VALID )continue ; -if (!ce_path_match (&the_index, ce, &p->pathspec,NULL )) +if (!ce_path_match (index, ce, &p->pathspec,NULL ))continue ;if (threaded_has_symlink_leading_path (&cache, ce->name,ce_namelen (ce)))continue ;
Signature · 1 line
2.18.0.rc0.333.g22e6ee6cdf
PATCH v3 17/20 cache.c: remove an implicit dependency on the_index
In reply to Nguyễn Thái Ngọc DuyMake some index API take an index_state instead of assuming the_index in read-cache.c. All external call sites are converted blindly to keep the patch simple and retain current behavior. Individual call sites may receive further updates to use the right index instead of the_index.
--- apply.c | 2 +- builtin/checkout.c | 2 +- builtin/difftool.c | 4 ++-- builtin/reset.c | 2 +- cache.h | 5 +++-- merge-recursive.c | 2 +- read-cache.c | 19 +++++++++++-------- resolve-undo.c | 2 +- 8 files changed, 21 insertions(+), 17 deletions(-)
apply.c +1 -1
@@ -4090,7 +4090,7 @@ 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 (patch->old_mode, oid.hash, name,0 ,0 ); + ce =make_cache_entry (&the_index, patch->old_mode, oid.hash, name,0 ,0 );if (!ce)return error (_ ("make_cache_entry failed for path '%s'" ), name);
builtin/checkout.c +1 -1
@@ -230,7 +230,7 @@ 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 (mode, oid.hash, path,2 ,0 ); + ce =make_cache_entry (&the_index, mode, oid.hash, path,2 ,0 );if (!ce)die (_ ("make_cache_entry failed for path '%s'" ), path); status =checkout_entry (ce, state,NULL );
builtin/difftool.c +2 -2
@@ -321,7 +321,7 @@ static int checkout_path(unsigned mode, struct object_id *oid,struct cache_entry *ce;int ret; - ce =make_cache_entry (mode, oid->hash, path,0 ,0 ); + ce =make_cache_entry (&the_index, 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 (rmode, roid.hash, +make_cache_entry (&the_index, rmode, roid.hash, dst_path,0 ,0 ); add_index_entry(&wtindex, ce2,
builtin/reset.c +1 -1
@@ -134,7 +134,7 @@ static void update_index_from_diff(struct diff_queue_struct *q,continue ; } - ce =make_cache_entry (one->mode, one->oid.hash, one->path, + ce =make_cache_entry (&the_index, one->mode, one->oid.hash, one->path,0 ,0 );if (!ce) die(_("make_cache_entry failed for path '%s'"),
cache.h +3 -2
@@ -355,6 +355,7 @@ extern void free_name_hash(struct index_state *istate);#define unmerged_cache () unmerged_index(&the_index)#define cache_name_pos (name, namelen) index_name_pos(&the_index,(name),(namelen))#define add_cache_entry (ce, option) add_index_entry(&the_index, (ce), (option)) +#define refresh_cache_entry (ce, flags) refresh_index_entry(&the_index, ce, flags)#define rename_cache_entry_at (pos, new_name) rename_index_entry_at(&the_index, (pos), (new_name))#define remove_cache_entry_at (pos) remove_index_entry_at(&the_index, (pos))#define remove_file_from_cache (path) remove_file_from_index(&the_index, (path))
@@ -698,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 (unsigned int mode,const unsigned char *sha1,const char *path,int stage,unsigned int refresh_options); +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 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);
@@ -751,7 +752,7 @@ extern void fill_stat_cache_info(struct cache_entry *ce, struct stat *st);#define REFRESH_IGNORE_SUBMODULES 0x0010/* ignore submodules */ #define REFRESH_IN_PORCELAIN 0x0020/* user friendly output, not "needs update" */ extern int refresh_index (struct index_state *,unsigned int flags,const struct pathspec *pathspec,char *seen,const char *header_msg); -extern struct cache_entry *refresh_cache_entry (struct cache_entry *,unsigned int ); +extern struct cache_entry *refresh_index_entry (struct index_state *istate,struct cache_entry *,unsigned int );/* * Opportunistically update the index but do not complain if we can't.
merge-recursive.c +1 -1
@@ -315,7 +315,7 @@ static int add_cacheinfo(struct merge_options *o,struct cache_entry *ce;int ret; - ce =make_cache_entry (mode, oid ? oid->hash : null_sha1, path, stage,0 ); + ce =make_cache_entry (&the_index, mode, oid ? oid->hash : null_sha1, path, stage,0 );if (!ce)return err (o,_ ("add_cacheinfo failed for path '%s'; merge aborting." ), path);
read-cache.c +11 -8
@@ -745,9 +745,11 @@ int add_file_to_index(struct index_state *istate, const char *path, int flags)return add_to_index (istate, path, &st, flags); } -struct cache_entry *make_cache_entry (unsigned int mode, -const unsigned char *sha1,const char *path,int stage, -unsigned int refresh_options) +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) {int size, len;struct cache_entry *ce, *ret;
@@ -767,7 +769,7 @@ struct cache_entry *make_cache_entry(unsigned int mode, ce->ce_namelen = len; ce->ce_mode =create_ce_mode (mode); - ret =refresh_cache_entry (ce, refresh_options); + ret =refresh_index_entry (istate, ce, refresh_options);if (ret != ce)free (ce);return ret;
@@ -1415,7 +1417,7 @@ int refresh_index(struct index_state *istate, unsigned int flags,if (ignore_submodules &&S_ISGITLINK (ce->ce_mode))continue ; -if (pathspec && !ce_path_match (&the_index, ce, pathspec, seen)) +if (pathspec && !ce_path_match (istate, ce, pathspec, seen)) filtered =1 ;if (ce_stage (ce)) {
@@ -1473,10 +1475,11 @@ int refresh_index(struct index_state *istate, unsigned int flags,return has_errors; } -struct cache_entry *refresh_cache_entry (struct cache_entry *ce, -unsigned int options) +struct cache_entry *refresh_index_entry (struct index_state *istate, +struct cache_entry *ce, +unsigned int options) { -return refresh_cache_ent (&the_index, ce, options,NULL ,NULL ); +return refresh_cache_ent (istate, ce, options,NULL ,NULL ); }
resolve-undo.c +1 -1
@@ -146,7 +146,7 @@ int unmerge_index_entry_at(struct index_state *istate, int pos)struct cache_entry *nce;if (!ru->mode[i])continue ; - nce =make_cache_entry (ru->mode[i], ru->oid[i].hash, + nce =make_cache_entry (&the_index, ru->mode[i], ru->oid[i].hash, name, i +1 ,0 );if (matched) nce->ce_flags |=CE_MATCHED ;
Signature · 1 line
2.18.0.rc0.333.g22e6ee6cdf
Re: [PATCH v3 17/20] cache.c: remove an implicit dependency on the_index
Quoted 91 lines · Nguyễn Thái Ngọc Duy
Make some index API take an
index_stateinstead of assumingthe_indexinread-cache.c. All external call sites are converted blindly to keep the patch simple and retain current behavior. Individual call sites may receive further updates to use the right index instead ofthe_index.Signed-off-by Nguyễn Thái Ngọc Duy <[email protected]>--- apply.c | 2 +- builtin/checkout.c | 2 +- builtin/difftool.c | 4 ++-- builtin/reset.c | 2 +- cache.h | 5 +++-- merge-recursive.c | 2 +- read-cache.c | 19 +++++++++++-------- resolve-undo.c | 2 +- 8 files changed, 21 insertions(+), 17 deletions(-)apply.c +1
-1@@ -4090,7 +4090,7 @@ 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 (patch->old_mode, oid.hash, name,0 ,0 ); + ce =make_cache_entry (&the_index, patch->old_mode, oid.hash, name,0 ,0 );if (!ce)return error (_ ("make_cache_entry failed for path '%s'" ), name);builtin/checkout.c +1
-1@@ -230,7 +230,7 @@ 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 (mode, oid.hash, path,2 ,0 ); + ce =make_cache_entry (&the_index, mode, oid.hash, path,2 ,0 );if (!ce)die (_ ("make_cache_entry failed for path '%s'" ), path); status =checkout_entry (ce, state,NULL );builtin/difftool.c +2
-2@@ -321,7 +321,7 @@ static int checkout_path(unsigned mode, struct object_id *oid,struct cache_entry *ce;int ret; - ce =make_cache_entry (mode, oid->hash, path,0 ,0 ); + ce =make_cache_entry (&the_index, 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 (rmode, roid.hash, +make_cache_entry (&the_index, rmode, roid.hash, dst_path,0 ,0 ); add_index_entry(&wtindex, ce2,builtin/reset.c +1
-1@@ -134,7 +134,7 @@ static void update_index_from_diff(struct diff_queue_struct *q,continue ; } - ce =make_cache_entry (one->mode, one->oid.hash, one->path, + ce =make_cache_entry (&the_index, one->mode, one->oid.hash, one->path,0 ,0 );if (!ce) die(_("make_cache_entry failed for path '%s'"),cache.h +1
-0@@ -355,6 +355,7 @@ extern void free_name_hash(struct index_state *istate);#define unmerged_cache () unmerged_index(&the_index)#define cache_name_pos (name, namelen) index_name_pos(&the_index,(name),(namelen))#define add_cache_entry (ce, option) add_index_entry(&the_index, (ce), (option)) +#define refresh_cache_entry (ce, flags) refresh_index_entry(&the_index, ce, flags)#define rename_cache_entry_at (pos, new_name) rename_index_entry_at(&the_index, (pos), (new_name))#define remove_cache_entry_at (pos) remove_index_entry_at(&the_index, (pos))#define remove_file_from_cache (path) remove_file_from_index(&the_index, (path))
cache.h extern int remove_file_from_index(struct index_state *, const char *path); · line 702
Quoted 5 lines
cache.h +1
-1@@ -698,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 (unsigned int mode,const unsigned char *sha1,const char *path,int stage,unsigned int refresh_options); +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);
minor nit: s/istate,unsigned/istate, unsigned/
Quoted 93 lines
cache.h +1
-1extern 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);@@ -751,7 +752,7 @@ extern void fill_stat_cache_info(struct cache_entry *ce, struct stat *st);#define REFRESH_IGNORE_SUBMODULES 0x0010/* ignore submodules */ #define REFRESH_IN_PORCELAIN 0x0020/* user friendly output, not "needs update" */ extern int refresh_index (struct index_state *,unsigned int flags,const struct pathspec *pathspec,char *seen,const char *header_msg); -extern struct cache_entry *refresh_cache_entry (struct cache_entry *,unsigned int ); +extern struct cache_entry *refresh_index_entry (struct index_state *istate,struct cache_entry *,unsigned int );/* * Opportunistically update the index but do not complain if we can't. merge-recursive.c +1
-1@@ -315,7 +315,7 @@ static int add_cacheinfo(struct merge_options *o,struct cache_entry *ce;int ret; - ce =make_cache_entry (mode, oid ? oid->hash : null_sha1, path, stage,0 ); + ce =make_cache_entry (&the_index, mode, oid ? oid->hash : null_sha1, path, stage,0 );if (!ce)return err (o,_ ("add_cacheinfo failed for path '%s'; merge aborting." ), path);read-cache.c +11
-8@@ -745,9 +745,11 @@ int add_file_to_index(struct index_state *istate, const char *path, int flags)return add_to_index (istate, path, &st, flags); } -struct cache_entry *make_cache_entry (unsigned int mode, -const unsigned char *sha1,const char *path,int stage, -unsigned int refresh_options) +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) {int size, len;struct cache_entry *ce, *ret;@@ -767,7 +769,7 @@ struct cache_entry *make_cache_entry(unsigned int mode, ce->ce_namelen = len; ce->ce_mode =create_ce_mode (mode); - ret =refresh_cache_entry (ce, refresh_options); + ret =refresh_index_entry (istate, ce, refresh_options);if (ret != ce)free (ce);return ret;@@ -1415,7 +1417,7 @@ int refresh_index(struct index_state *istate, unsigned int flags,if (ignore_submodules &&S_ISGITLINK (ce->ce_mode))continue ; -if (pathspec && !ce_path_match (&the_index, ce, pathspec, seen)) +if (pathspec && !ce_path_match (istate, ce, pathspec, seen)) filtered =1 ;if (ce_stage (ce)) {@@ -1473,10 +1475,11 @@ int refresh_index(struct index_state *istate, unsigned int flags,return has_errors; } -struct cache_entry *refresh_cache_entry (struct cache_entry *ce, -unsigned int options) +struct cache_entry *refresh_index_entry (struct index_state *istate, +struct cache_entry *ce, +unsigned int options) { -return refresh_cache_ent (&the_index, ce, options,NULL ,NULL ); +return refresh_cache_ent (istate, ce, options,NULL ,NULL ); }resolve-undo.c +1
-1@@ -146,7 +146,7 @@ int unmerge_index_entry_at(struct index_state *istate, int pos)struct cache_entry *nce;if (!ru->mode[i])continue ; - nce =make_cache_entry (ru->mode[i], ru->oid[i].hash, + nce =make_cache_entry (&the_index, ru->mode[i], ru->oid[i].hash, name, i +1 ,0 );if (matched) nce->ce_flags |=CE_MATCHED ;Signature · 1 line
2.18.0.rc0.333.g22e6ee6cdf
Signature · 1 line
Brandon Williams
PATCH v3 18/20 resolve-undo.c: use the right index instead of the_index
In reply to Nguyễn Thái Ngọc Duy--- resolve-undo.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)
resolve-undo.c +3 -2
@@ -1,3 +1,4 @@ +#define NO_THE_INDEX_COMPATIBILITY_MACROS #include "cache.h" #include "dir.h" #include "resolve-undo.h"
@@ -146,7 +147,7 @@ int unmerge_index_entry_at(struct index_state *istate, int pos)struct cache_entry *nce;if (!ru->mode[i])continue ; - nce =make_cache_entry (&the_index, ru->mode[i], ru->oid[i].hash, + nce =make_cache_entry (istate, ru->mode[i], ru->oid[i].hash, name, i +1 ,0 );if (matched) nce->ce_flags |=CE_MATCHED ;
@@ -186,7 +187,7 @@ void unmerge_index(struct index_state *istate, const struct pathspec *pathspec)for (i =0 ; i < istate->cache_nr; i++) {const struct cache_entry *ce = istate->cache[i]; -if (!ce_path_match (&the_index, ce, pathspec,NULL )) +if (!ce_path_match (istate, ce, pathspec,NULL ))continue ; i =unmerge_index_entry_at (istate, i); }
Signature · 1 line
2.18.0.rc0.333.g22e6ee6cdf
PATCH v3 19/20 grep: use the right index instead of the_index
In reply to Nguyễn Thái Ngọc Duy--- builtin/grep.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
builtin/grep.c +2 -2
@@ -497,7 +497,7 @@ static int grep_cache(struct grep_opt *opt, struct repository *repo,strbuf_addstr (&name, ce->name);if (S_ISREG (ce->ce_mode) && -match_pathspec (&the_index, pathspec, name.buf, name.len,0 ,NULL , +match_pathspec (repo->index, pathspec, name.buf, name.len,0 ,NULL ,S_ISDIR (ce->ce_mode) ||S_ISGITLINK (ce->ce_mode))) {/*
@@ -515,7 +515,7 @@ static int grep_cache(struct grep_opt *opt, struct repository *repo, hit |=grep_file (opt, name.buf); } }else if (recurse_submodules &&S_ISGITLINK (ce->ce_mode) && -submodule_path_match (&the_index, pathspec, name.buf,NULL )) { +submodule_path_match (repo->index, pathspec, name.buf,NULL )) { hit |=grep_submodule (opt, repo, pathspec,NULL , ce->name, ce->name); }else {continue ;
Signature · 1 line
2.18.0.rc0.333.g22e6ee6cdf
PATCH v3 20/20 cache.h: make the_index part of "compatibility macros"
In reply to Nguyễn Thái Ngọc DuyWhile the_index is not actually a macro, its use throughout the code base is dangerous because developers sometimes may not see that some function is using the_index (instead of some other index that the devs are interested in).
By keeping the_index part of this NO_ macro, we try to reduce its use more and more until it's completely gone.
--- cache.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
cache.h +2 -2
@@ -330,8 +330,6 @@ struct index_state {struct ewah_bitmap *fsmonitor_dirty; }; -extern struct index_state the_index; -/* Name hashing */ extern int test_lazy_init_name_hash (struct index_state *istate,int try_threaded);extern void add_name_hash (struct index_state *istate,struct cache_entry *ce);
@@ -340,6 +338,8 @@ extern void free_name_hash(struct index_state *istate);#ifndef NO_THE_INDEX_COMPATIBILITY_MACROS +extern struct index_state the_index; +#define active_cache (the_index.cache)#define active_nr (the_index.cache_nr)#define active_alloc (the_index.cache_alloc)
Signature · 1 line
2.18.0.rc0.333.g22e6ee6cdf
v4 cover Fix incorrect use of the_index
In reply to Nguyễn Thái Ngọc Duyv4 fixes some commit messages and killed a couple more the_index references after I tried to merge this with 'pu'
apply.c +2 -2
@@ -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);
builtin/checkout.c +2 -2
@@ -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;
builtin/difftool.c +2 -2
@@ -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,
builtin/reset.c +2 -2
@@ -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 ;
cache.h +1 -1
@@ -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);
merge-recursive.c +1 -1
@@ -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_sha1, path, stage,0 );if (!ce)return err (o,_ ("add_cacheinfo failed for path '%s'; merge aborting." ), path);
read-cache.c +1 -1
@@ -745,7 +745,7 @@ int add_file_to_index(struct index_state *istate, const char *path, int flags)return add_to_index (istate, path, &st, flags); } -struct cache_entry *make_cache_entry(struct index_state *istate, +struct cache_entry *make_index_entry(struct index_state *istate, unsigned int mode, const unsigned char *sha1, const char *path, int stage,
resolve-undo.c +1 -1
@@ -147,7 +147,7 @@ int unmerge_index_entry_at(struct index_state *istate, int pos)struct cache_entry *nce;if (!ru->mode[i])continue ; - nce =make_cache_entry (istate, ru->mode[i], ru->oid[i].hash, + nce =make_index_entry (istate, ru->mode[i], ru->oid[i].hash, name, i +1 ,0 );if (matched) nce->ce_flags |=CE_MATCHED ;
Nguyễn Thái Ngọc Duy (23):
unpack-trees: remove 'extern' on function declaration
unpack-trees: add a note about path invalidation
unpack-trees: don't shadow global var the_index
unpack-tress: convert clear_ce_flags* to avoid the_index
unpack-trees: avoid the_index in verify_absent() attr.h: drop extern from function declaration
attr: remove an implicit dependency on the_index convert.h: drop 'extern' from function declaration convert.c: remove an implicit dependency on the_index dir.c: remove an implicit dependency on the_index in pathspec code
ls-files: correct index argument to get_convert_attr_ascii() pathspec.c: use the right index instead of the_index submodule.c: use the right index instead of the_index entry.c: use the right index instead of the_index
attr: remove index from git_attr_set_direction() preload-index.c: use the right index instead of the_index read-cache.c: remove an implicit dependency on the_index apply.c: use the right index instead of the_index
difftool: use the right index instead of the_index
checkout: avoid the_index when possible
resolve-undo.c: use the right index instead of the_index
grep: use the right index instead of the_index
cache.h: make the_index part of "compatibility macros"
apply.c | 6 ++-- archive.c | 8 ++--- attr.c | 52 ++++++++++++++++++------------- attr.h | 31 ++++++++++--------- builtin/add.c | 6 ++-- builtin/cat-file.c | 2 +- builtin/check-attr.c | 6 ++-- builtin/checkout-index.c | 1 + builtin/checkout.c | 6 ++-- builtin/clean.c | 2 +- builtin/commit.c | 2 +- builtin/difftool.c | 4 +-- builtin/grep.c | 6 ++-- builtin/ls-files.c | 17 ++++++----- builtin/pack-objects.c | 2 +- builtin/reset.c | 4 +-- builtin/rm.c | 2 +- builtin/submodule--helper.c | 2 +- builtin/update-index.c | 2 +- cache.h | 9 +++--- convert.c | 41 ++++++++++++++----------- convert.h | 61 +++++++++++++++++++++---------------- diff-lib.c | 4 +-- diff.c | 2 +- dir.c | 27 +++++++++------- dir.h | 16 ++++++---- entry.c | 10 +++--- ll-merge.c | 4 +-- merge-recursive.c | 4 +-- pathspec.c | 2 +- preload-index.c | 3 +- read-cache.c | 19 +++++++----- rerere.c | 2 +- resolve-undo.c | 5 +-- revision.c | 2 +- sha1-file.c | 4 +-- submodule.c | 8 ++--- unpack-trees.c | 57 +++++++++++++++++++++------------- unpack-trees.h | 4 +-- userdiff.c | 2 +- ws.c | 2 +- wt-status.c | 6 ++-- 42 files changed, 258 insertions(+), 197 deletions(-)
Signature · 1 line
2.18.0.rc0.333.g22e6ee6cdf
PATCH v4 1/23 unpack-trees: remove 'extern' on function declaration
--- unpack-trees.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
unpack-trees.h +2 -2
@@ -82,8 +82,8 @@ struct unpack_trees_options {struct exclude_list *el;/* for internal use */ }; -extern int unpack_trees (unsigned n,struct tree_desc *t, -struct unpack_trees_options *options); +int unpack_trees (unsigned n,struct tree_desc *t, +struct unpack_trees_options *options);int verify_uptodate (const struct cache_entry *ce,struct unpack_trees_options *o);
Signature · 1 line
2.18.0.rc0.333.g22e6ee6cdf
PATCH v4 2/23 unpack-trees: add a note about path invalidation
In reply to Nguyễn Thái Ngọc Duy--- unpack-trees.c | 11 +++++++++++ 1 file changed, 11 insertions(+)
unpack-trees.c +11 -0
@@ -1545,6 +1545,17 @@ static int verify_uptodate_sparse(const struct cache_entry *ce,return verify_uptodate_1 (ce, o,ERROR_SPARSE_NOT_UPTODATE_FILE ); } +/* +* TODO: We should actually invalidate o->result, not src_index [1]. +* But since cache tree and untracked cache both are not copied to +* o->result until unpacking is complete, we invalidate them on +* src_index instead with the assumption that they will be copied to +* dst_index at the end. +* +* [1] src_index->cache_tree is also used in unpack_callback() so if +* we invalidate o->result, we need to update it to use +* o->result.cache_tree as well. +*/ static void invalidate_ce_path (const struct cache_entry *ce,struct unpack_trees_options *o) {
Signature · 1 line
2.18.0.rc0.333.g22e6ee6cdf
PATCH v4 3/23 unpack-trees: don't shadow global var the_index
In reply to Nguyễn Thái Ngọc DuyThis function mark_new_skip_worktree() has an argument named the_index which is also the name of a global variable. While they have different types (the global the_index is not a pointer) mistakes can easily happen and it's also confusing for readers. Rename the function argument to something other than the_index.
--- unpack-trees.c | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-)
unpack-trees.c +4 -5
@@ -1231,7 +1231,7 @@ static int clear_ce_flags(struct cache_entry **cache, int nr,* Set/Clear CE_NEW_SKIP_WORKTREE according to $GIT_DIR/info/sparse-checkout */ static void mark_new_skip_worktree (struct exclude_list *el, -struct index_state *the_index, +struct index_state *istate,int select_flag,int skip_wt_flag) {int i;
@@ -1240,8 +1240,8 @@ static void mark_new_skip_worktree(struct exclude_list *el,* 1. Pretend the narrowest worktree: only unmerged entries * are checked out */ -for (i =0 ; i < the_index->cache_nr; i++) { -struct cache_entry *ce = the_index->cache[i]; +for (i =0 ; i < istate->cache_nr; i++) { +struct cache_entry *ce = istate->cache[i];if (select_flag && !(ce->ce_flags & select_flag))continue ;
@@ -1256,8 +1256,7 @@ static void mark_new_skip_worktree(struct exclude_list *el,* 2. Widen worktree according to sparse-checkout file. * Matched entries will have skip_wt_flag cleared (i.e. "in") */ -clear_ce_flags (the_index->cache, the_index->cache_nr, - select_flag, skip_wt_flag, el); +clear_ce_flags (istate->cache, istate->cache_nr, select_flag, skip_wt_flag, el); } static int verify_absent(const struct cache_entry *,
Signature · 1 line
2.18.0.rc0.333.g22e6ee6cdf
PATCH v4 4/23 unpack-tress: convert clear_ce_flags* to avoid the_index
In reply to Nguyễn Thái Ngọc DuyPrior to fba92be8f7, this code implicitly (and incorrectly) assumes the_index when running the exclude machinery. fba92be8f7 helps show this problem clearer because unpack-trees operation is supposed to work on whatever index the caller specifies... not specifically the_index.
Update the code to use "istate" argument that's originally from mark_new_skip_worktree(). From the call sites, both in unpack_trees(), you can see that this function works on two separate indexes:o->src_index and o->result. The second mark_new_skip_worktree() so far has incorecctly applied exclude rules on o->src_index instead of o->result. It's unclear what is the consequences of this, but it's definitely wrong.
[1] fba92be8f7 (dir: convert is_excluded_from_list to take an index -
2017-05-05)
--- unpack-trees.c | 31 ++++++++++++++++++------------- 1 file changed, 18 insertions(+), 13 deletions(-)
unpack-trees.c +18 -13
@@ -1085,13 +1085,15 @@ static int unpack_callback(int n, unsigned long mask, unsigned long dirmask, strreturn mask; } -static int clear_ce_flags_1 (struct cache_entry **cache,int nr, +static int clear_ce_flags_1 (struct index_state *istate, +struct cache_entry **cache,int nr,struct strbuf *prefix,int select_mask,int clear_mask,struct exclude_list *el,int defval);/* Whole directory matching */ -static int clear_ce_flags_dir(struct cache_entry **cache, int nr, +static int clear_ce_flags_dir(struct index_state *istate, + struct cache_entry **cache, int nr, struct strbuf *prefix, char *basename, int select_mask, int clear_mask,
@@ -1100,7 +1102,7 @@ static int clear_ce_flags_dir(struct cache_entry **cache, int nr,struct cache_entry **cache_end;int dtype =DT_DIR ;int ret =is_excluded_from_list (prefix->buf, prefix->len, - basename, &dtype, el, &the_index); + basename, &dtype, el, istate);int rc;strbuf_addch (prefix,'/' );
@@ -1122,7 +1124,7 @@ static int clear_ce_flags_dir(struct cache_entry **cache, int nr,* calling clear_ce_flags_1(). That function will call * the expensive is_excluded_from_list() on every entry. */ - rc =clear_ce_flags_1 (cache, cache_end - cache, + rc =clear_ce_flags_1 (istate, cache, cache_end - cache, prefix, select_mask, clear_mask, el, ret);
@@ -1145,7 +1147,8 @@ static int clear_ce_flags_dir(struct cache_entry **cache, int nr,* cache[0]->name[0..(prefix_len-1)] * Top level path has prefix_len zero. */ -static int clear_ce_flags_1 (struct cache_entry **cache,int nr, +static int clear_ce_flags_1 (struct index_state *istate, +struct cache_entry **cache,int nr,struct strbuf *prefix,int select_mask,int clear_mask,struct exclude_list *el,int defval)
@@ -1179,7 +1182,7 @@ static int clear_ce_flags_1(struct cache_entry **cache, int nr, len = slash - name;strbuf_add (prefix, name, len); - processed = clear_ce_flags_dir(cache, cache_end - cache, + processed = clear_ce_flags_dir(istate, cache, cache_end - cache, prefix, prefix->buf + prefix->len - len, select_mask, clear_mask,
@@ -1193,7 +1196,7 @@ static int clear_ce_flags_1(struct cache_entry **cache, int nr, }strbuf_addch (prefix,'/' ); - cache +=clear_ce_flags_1 (cache, cache_end - cache, + cache +=clear_ce_flags_1 (istate, cache, cache_end - cache, prefix, select_mask, clear_mask, el, defval);strbuf_setlen (prefix, prefix->len - len -1 );
@@ -1203,7 +1206,7 @@ static int clear_ce_flags_1(struct cache_entry **cache, int nr,/* Non-directory */ dtype =ce_to_dtype (ce); ret =is_excluded_from_list (ce->name,ce_namelen (ce), - name, &dtype, el, &the_index); + name, &dtype, el, istate);if (ret <0 ) ret = defval;if (ret >0 )
@@ -1213,15 +1216,17 @@ static int clear_ce_flags_1(struct cache_entry **cache, int nr,return nr - (cache_end - cache); } -static int clear_ce_flags (struct cache_entry **cache,int nr, -int select_mask,int clear_mask, -struct exclude_list *el) +static int clear_ce_flags (struct index_state *istate, +int select_mask,int clear_mask, +struct exclude_list *el) {static struct strbuf prefix =STRBUF_INIT ;strbuf_reset (&prefix); -return clear_ce_flags_1 (cache, nr, +return clear_ce_flags_1 (istate, + istate->cache, + istate->cache_nr, &prefix, select_mask, clear_mask, el,0 );
@@ -1256,7 +1261,7 @@ static void mark_new_skip_worktree(struct exclude_list *el,* 2. Widen worktree according to sparse-checkout file. * Matched entries will have skip_wt_flag cleared (i.e. "in") */ -clear_ce_flags (istate->cache, istate->cache_nr, select_flag, skip_wt_flag, el); +clear_ce_flags (istate, select_flag, skip_wt_flag, el); } static int verify_absent(const struct cache_entry *,
Signature · 1 line
2.18.0.rc0.333.g22e6ee6cdf
Re: [PATCH v4 04/23] unpack-tress: convert clear_ce_flags* to avoid the_index
Subject line: unpack-trees rather than unpack-tress.
Quoted 14 lines · Nguyễn Thái Ngọc Duy
Prior to
fba92be8f7, this code implicitly (and incorrectly) assumesthe_indexwhen running the exclude machinery.fba92be8f7helps show this problem clearer because unpack-trees operation is supposed to work on whatever index the caller specifies... not specificallythe_index.Update the code to use "istate" argument that's originally from
mark_new_skip_worktree(). From the call sites, both inunpack_trees(), you can see that this function works on two separate indexes:o->src_indexando->result. The secondmark_new_skip_worktree()so far has incorecctly applied exclude rules ono->src_indexinstead ofo->result. It's unclear what is the consequences of this, but it's definitely wrong.
Quoted 4 lines
[1]
fba92be8f7(dir: convertis_excluded_from_listto take an index -2017-05-05)Signed-off-by Nguyễn Thái Ngọc Duy <[email protected]>
A somewhat curious finding: when I was rebuilding and re-testing all 23 patches, I got a failure on this patch in test 31 of t7063-status-untracked-cache.sh. I did not get any test failures with any of the other patches. However, after re-running that test or the whole suite half a dozen times with just up to this patch applied, I was not able to trigger the failure again. Is there a rare race in that testcase? I certainly don't see anything in this patch that appears problematic, and the fact that I couldn't readily reproduce suggests it could well have been there before any of these patches.
Everything else in the patch looks fine to me.
Re: [PATCH v4 04/23] unpack-tress: convert clear_ce_flags* to avoid the_index
Quoted 24 lines · Elijah Newren
Subject line: unpack-trees rather than unpack-tress.
Quoted 14 lines · Nguyễn Thái Ngọc Duy
Prior to
fba92be8f7, this code implicitly (and incorrectly) assumesthe_indexwhen running the exclude machinery.fba92be8f7helps show this problem clearer because unpack-trees operation is supposed to work on whatever index the caller specifies... not specificallythe_index.Update the code to use "istate" argument that's originally from
mark_new_skip_worktree(). From the call sites, both inunpack_trees(), you can see that this function works on two separate indexes:o->src_indexando->result. The secondmark_new_skip_worktree()so far has incorecctly applied exclude rules ono->src_indexinstead ofo->result. It's unclear what is the consequences of this, but it's definitely wrong.Quoted 4 lines
[1]
fba92be8f7(dir: convertis_excluded_from_listto take an index -2017-05-05)Signed-off-by Nguyễn Thái Ngọc Duy <[email protected]>
Quoted 7 lines
A somewhat curious finding: when I was rebuilding and re-testing all 23 patches, I got a failure on this patch in test 31 of
t7063-status-untracked-cache.sh. I did not get any test failures with any of the other patches. However, after re-running that test or the whole suite half a dozen times with just up to this patch applied, I was not able to trigger the failure again. Is there a rare race in that testcase?
Untracked cache tests are very time-sensitive. I'll try to run and re-run them a couple times to understand more. Thanks for pointing it out.
Signature · 1 line
Duy
Re: [PATCH v4 04/23] unpack-tress: convert clear_ce_flags* to avoid the_index
Quoted 34 lines · Duy Nguyen
Quoted 24 lines · Elijah Newren
Subject line: unpack-trees rather than unpack-tress.
Quoted 14 lines · Nguyễn Thái Ngọc Duy
Prior to
fba92be8f7, this code implicitly (and incorrectly) assumesthe_indexwhen running the exclude machinery.fba92be8f7helps show this problem clearer because unpack-trees operation is supposed to work on whatever index the caller specifies... not specificallythe_index.Update the code to use "istate" argument that's originally from
mark_new_skip_worktree(). From the call sites, both inunpack_trees(), you can see that this function works on two separate indexes:o->src_indexando->result. The secondmark_new_skip_worktree()so far has incorecctly applied exclude rules ono->src_indexinstead ofo->result. It's unclear what is the consequences of this, but it's definitely wrong.Quoted 4 lines
[1]
fba92be8f7(dir: convertis_excluded_from_listto take an index -2017-05-05)Signed-off-by Nguyễn Thái Ngọc Duy <[email protected]>Quoted 7 lines
A somewhat curious finding: when I was rebuilding and re-testing all 23 patches, I got a failure on this patch in test 31 of
t7063-status-untracked-cache.sh. I did not get any test failures with any of the other patches. However, after re-running that test or the whole suite half a dozen times with just up to this patch applied, I was not able to trigger the failure again. Is there a rare race in that testcase?
Quoted 3 lines
Untracked cache tests are very time-sensitive. I'll try to run and re-run them a couple times to understand more. Thanks for pointing it out.
after hours of running tests, either with full 23 patches or just the first 4, and failing to catch the failure, i declare (or more like, pray) that you ran into a crack of time that led to a race. I'll take no action on this, but I'll remember this and watch out for untracked cache related mails for some time.
Signature · 1 line
Duy
PATCH v4 5/23 unpack-trees: avoid the_index in verify_absent()
In reply to Nguyễn Thái Ngọc DuyBoth functions that are updated in this commit are called by verify_absent(), which is part of the "unpack-trees" operation that is supposed to work on any index file specified by the caller. Thanks to Brandon [1] [2], an implicit dependency on the_index is exposed. This commit fixes it.
In both functions, it makes sense to use src_index to check for exclusion because it's almost unchanged and should give us the same outcome as if running the exclude check before the unpack.
It's "almost unchanged" because we do invalidate cache-tree and untracked cache in the source index. But this should not affect how exclude machinery uses the index: to see if a file is tracked, and to read a blob from the index instead of worktree if it's marked skip-worktree (i.e. it's not available in worktree)
[1] a0bba65b10 (dir: convert is_excluded to take an index - 2017-05-05
[2] 2c1eb10454 (dir: convert read_directory to take an index - 2017-05-05)
--- unpack-trees.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
unpack-trees.c +2 -2
@@ -1651,7 +1651,7 @@ static int verify_clean_subdirectory(const struct cache_entry *ce,memset (&d,0 ,sizeof (d));if (o->dir) d.exclude_per_dir = o->dir->exclude_per_dir; - i =read_directory (&d, &the_index, pathbuf, namelen+1 ,NULL ); + i =read_directory (&d, o->src_index, pathbuf, namelen+1 ,NULL );if (i)return o->gently ?-1 :add_rejected_path (o,ERROR_NOT_UPTODATE_DIR , ce->name);
@@ -1693,7 +1693,7 @@ static int check_ok_to_remove(const char *name, int len, int dtype,return 0 ;if (o->dir && -is_excluded (o->dir, &the_index, name, &dtype)) +is_excluded (o->dir, o->src_index, name, &dtype))/* * ce->name is explicitly excluded, so it is Ok to * overwrite it.
Signature · 1 line
2.18.0.rc0.333.g22e6ee6cdf
PATCH v4 6/23 attr.h: drop extern from function declaration
In reply to Nguyễn Thái Ngọc Duy--- attr.h | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-)
attr.h +12 -12
@@ -42,31 +42,31 @@ struct attr_check {struct attr_stack *stack; }; -extern struct attr_check *attr_check_alloc (void ); -extern struct attr_check *attr_check_initl (const char *, ...); -extern struct attr_check *attr_check_dup (const struct attr_check *check); +struct attr_check *attr_check_alloc (void ); +struct attr_check *attr_check_initl (const char *, ...); +struct attr_check *attr_check_dup (const struct attr_check *check); -extern struct attr_check_item *attr_check_append (struct attr_check *check, -const struct git_attr *attr); +struct attr_check_item *attr_check_append (struct attr_check *check, +const struct git_attr *attr); -extern void attr_check_reset (struct attr_check *check); -extern void attr_check_clear (struct attr_check *check); -extern void attr_check_free (struct attr_check *check); +void attr_check_reset (struct attr_check *check); +void attr_check_clear (struct attr_check *check); +void attr_check_free (struct attr_check *check);/* * Return the name of the attribute represented by the argument. The * return value is a pointer to a null-delimited string that is part * of the internal data structure; it should not be modified or freed. */ -extern const char *git_attr_name (const struct git_attr *); +const char *git_attr_name (const struct git_attr *); -extern int git_check_attr (const char *path,struct attr_check *check); +int git_check_attr (const char *path,struct attr_check *check);/* * Retrieve all attributes that apply to the specified path. * check holds the attributes and their values. */ -extern void git_all_attrs (const char *path,struct attr_check *check); +void git_all_attrs (const char *path,struct attr_check *check);enum git_attr_direction {GIT_ATTR_CHECKIN ,
@@ -76,6 +76,6 @@ enum git_attr_direction {void git_attr_set_direction (enum git_attr_direction new_direction,struct index_state *istate); -extern void attr_start (void ); +void attr_start (void );#endif /* ATTR_H */
Signature · 1 line
2.18.0.rc0.333.g22e6ee6cdf
PATCH v4 7/23 attr: remove an implicit dependency on the_index
Make the attr API take an index_state instead of assuming the_index in attr code. All call sites are converted blindly to keep the patch simple and retain current behavior. Individual call sites may receive further updates to use the right index instead of the_index.
There is one ugly temporary workaround added in attr.c that needs some more explanation.
Commit c24f3abace (apply: file commited with CRLF should roundtrip diff and apply - 2017-08-19) forces one convert_to_git() call to NOT read the index at all. But what do you know, we read it anyway by falling back to the_index. When "istate" from convert_to_git is now propagated down to read_attr_from_array() we will hit segfault somewhere inside read_blob_data_from_index.
The right way of dealing with this is to kill "use_index" variable and only follow "istate" but at this stage we are not ready for that:
while most git_attr_set_direction() calls just passes the_index to be assigned to use_index, unpack-trees passes a different one which is used by entry.c code, which has no way to know what index to use if we delete use_index. So this has to be done later.
--- archive.c | 2 +- attr.c | 57 ++++++++++++++++++++++++++++-------------- attr.h | 10 +++++--- builtin/check-attr.c | 4 +-- builtin/pack-objects.c | 2 +- convert.c | 2 +- dir.c | 2 +- ll-merge.c | 4 +-- userdiff.c | 2 +- ws.c | 2 +- 10 files changed, 55 insertions(+), 32 deletions(-)
archive.c +1 -1
@@ -108,7 +108,7 @@ static const struct attr_check *get_archive_attrs(const char *path)static struct attr_check *check;if (!check) check =attr_check_initl ("export-ignore" ,"export-subst" ,NULL ); -return git_check_attr (path, check) ?NULL : check; +return git_check_attr (&the_index, path, check) ?NULL : check; }static int check_attr_export_ignore (const struct attr_check *check)
attr.c +38 -19
155 lines not on this page · raw message
attr.h +7 -3
@@ -1,6 +1,8 @@#ifndef ATTR_H #define ATTR_H +struct index_state ; +/* An attribute is a pointer to this opaque structure */ struct git_attr ;
@@ -60,13 +62,15 @@ void attr_check_free(struct attr_check *check);*/ const char *git_attr_name (const struct git_attr *); -int git_check_attr (const char *path,struct attr_check *check); +int git_check_attr (const struct index_state *istate, +const char *path,struct attr_check *check);/* * Retrieve all attributes that apply to the specified path. * check holds the attributes and their values. */ -void git_all_attrs (const char *path,struct attr_check *check); +void git_all_attrs (const struct index_state *istate, +const char *path,struct attr_check *check);enum git_attr_direction {GIT_ATTR_CHECKIN ,
@@ -74,7 +78,7 @@ enum git_attr_direction { GIT_ATTR_INDEX };void git_attr_set_direction (enum git_attr_direction new_direction, -struct index_state *istate); +const struct index_state *istate);void attr_start (void );
builtin/check-attr.c +2 -2
@@ -63,9 +63,9 @@ static void check_attr(const char *prefix,prefix_path (prefix, prefix ?strlen (prefix) :0 , file);if (collect_all) { -git_all_attrs (full_path, check); +git_all_attrs (&the_index, full_path, check); }else { -if (git_check_attr (full_path, check)) +if (git_check_attr (&the_index, full_path, check))die ("git_check_attr died" ); }output_attr (check, file);
builtin/pack-objects.c +1 -1
@@ -945,7 +945,7 @@ static int no_try_delta(const char *path)if (!check) check =attr_check_initl ("delta" ,NULL ); -if (git_check_attr (path, check)) +if (git_check_attr (&the_index, path, check))return 0 ;if (ATTR_FALSE (check->items[0 ].value))return 1 ;
convert.c +1 -1
@@ -1302,7 +1302,7 @@ static void convert_attrs(struct conv_attrs *ca, const char *path)git_config (read_convert_config,NULL ); } -if (!git_check_attr (path, check)) { +if (!git_check_attr (&the_index, path, check)) {struct attr_check_item *ccheck = check->items; ca->crlf_action =git_path_check_crlf (ccheck +4 );if (ca->crlf_action ==CRLF_UNDEFINED )
dir.c +1 -1
@@ -280,7 +280,7 @@ static int match_attrs(const char *name, int namelen, {int i; -git_check_attr (name, item->attr_check); +git_check_attr (&the_index, name, item->attr_check);for (i =0 ; i < item->attr_match_nr; i++) {const char *value;int matched;
ll-merge.c +2 -2
@@ -371,7 +371,7 @@ int ll_merge(mmbuffer_t *result_buf,if (!check) check =attr_check_initl ("merge" ,"conflict-marker-size" ,NULL ); -if (!git_check_attr (path, check)) { +if (!git_check_attr (&the_index, path, check)) { ll_driver_name = check->items[0 ].value;if (check->items[1 ].value) { marker_size =atoi (check->items[1 ].value);
@@ -398,7 +398,7 @@ int ll_merge_marker_size(const char *path)if (!check) check =attr_check_initl ("conflict-marker-size" ,NULL ); -if (!git_check_attr (path, check) && check->items[0 ].value) { +if (!git_check_attr (&the_index, path, check) && check->items[0 ].value) { marker_size =atoi (check->items[0 ].value);if (marker_size <=0 ) marker_size =DEFAULT_CONFLICT_MARKER_SIZE ;
userdiff.c +1 -1
@@ -278,7 +278,7 @@ struct userdiff_driver *userdiff_find_by_path(const char *path) check =attr_check_initl ("diff" ,NULL );if (!path)return NULL ; -if (git_check_attr (path, check)) +if (git_check_attr (&the_index, path, check))return NULL ;if (ATTR_TRUE (check->items[0 ].value))
ws.c +1 -1
@@ -78,7 +78,7 @@ unsigned whitespace_rule(const char *pathname)if (!attr_whitespace_rule) attr_whitespace_rule =attr_check_initl ("whitespace" ,NULL ); -if (!git_check_attr (pathname, attr_whitespace_rule)) { +if (!git_check_attr (&the_index, pathname, attr_whitespace_rule)) {const char *value; value = attr_whitespace_rule->items[0 ].value;
Signature · 1 line
2.18.0.rc0.333.g22e6ee6cdf
PATCH v4 8/23 convert.h: drop 'extern' from function declaration
In reply to Nguyễn Thái Ngọc Duy--- convert.h | 56 ++++++++++++++++++++++++++++--------------------------- 1 file changed, 29 insertions(+), 27 deletions(-)
convert.h +29 -27
@@ -57,35 +57,36 @@ struct delayed_checkout {extern enum eol core_eol;extern char *check_roundtrip_encoding; -extern const char *get_cached_convert_stats_ascii (const struct index_state *istate, -const char *path); -extern const char *get_wt_convert_stats_ascii (const char *path); -extern const char *get_convert_attr_ascii (const char *path); +const char *get_cached_convert_stats_ascii (const struct index_state *istate, +const char *path); +const char *get_wt_convert_stats_ascii (const char *path); +const char *get_convert_attr_ascii (const char *path);/* returns 1 if *dst was used */ -extern int convert_to_git (const struct index_state *istate, -const char *path,const char *src,size_t len, -struct strbuf *dst,int conv_flags); -extern int convert_to_working_tree (const char *path,const char *src, -size_t len,struct strbuf *dst); -extern int async_convert_to_working_tree (const char *path,const char *src, -size_t len,struct strbuf *dst, -void *dco); -extern int async_query_available_blobs (const char *cmd,struct string_list *available_paths); -extern int renormalize_buffer (const struct index_state *istate, -const char *path,const char *src,size_t len, -struct strbuf *dst); +int convert_to_git (const struct index_state *istate, +const char *path,const char *src,size_t len, +struct strbuf *dst,int conv_flags); +int convert_to_working_tree (const char *path,const char *src, +size_t len,struct strbuf *dst); +int async_convert_to_working_tree (const char *path,const char *src, +size_t len,struct strbuf *dst, +void *dco); +int async_query_available_blobs (const char *cmd, +struct string_list *available_paths); +int renormalize_buffer (const struct index_state *istate, +const char *path,const char *src,size_t len, +struct strbuf *dst);static inline int would_convert_to_git (const struct index_state *istate,const char *path) {return convert_to_git (istate, path,NULL ,0 ,NULL ,0 ); }/* Precondition: would_convert_to_git_filter_fd(path) == true */ -extern void convert_to_git_filter_fd (const struct index_state *istate, -const char *path,int fd, -struct strbuf *dst, -int conv_flags); -extern int would_convert_to_git_filter_fd (const char *path); +void convert_to_git_filter_fd (const struct index_state *istate, +const char *path,int fd, +struct strbuf *dst, +int conv_flags); +int would_convert_to_git_filter_fd (const char *path);/***************************************************************** *
@@ -95,9 +96,10 @@ extern int would_convert_to_git_filter_fd(const char *path);struct stream_filter ;/* opaque */ -extern struct stream_filter *get_stream_filter (const char *path,const struct object_id *); -extern void free_stream_filter (struct stream_filter *); -extern int is_null_stream_filter (struct stream_filter *); +struct stream_filter *get_stream_filter (const char *path, +const struct object_id *); +void free_stream_filter (struct stream_filter *); +int is_null_stream_filter (struct stream_filter *);/* * Use as much input up to *isize_p and fill output up to *osize_p;
@@ -111,8 +113,8 @@ extern int is_null_stream_filter(struct stream_filter *);* such filters know there is no more input coming and it is time for * them to produce the remaining output based on the buffered input. */ -extern int stream_filter (struct stream_filter *, -const char *input,size_t *isize_p, -char *output,size_t *osize_p); +int stream_filter (struct stream_filter *, +const char *input,size_t *isize_p, +char *output,size_t *osize_p);#endif /* CONVERT_H */
Signature · 1 line
2.18.0.rc0.333.g22e6ee6cdf
PATCH v4 9/23 convert.c: remove an implicit dependency on the_index
In reply to Nguyễn Thái Ngọc DuyMake the convert API take an index_state instead of assuming the_index in convert.c. All external call sites are converted blindly to keep the patch simple and retain current behavior. Individual call sites may receive further updates to use the right index instead of the_index.
--- apply.c | 2 +- archive.c | 2 +- builtin/cat-file.c | 2 +- builtin/ls-files.c | 2 +- convert.c | 41 ++++++++++++++++++++++++----------------- convert.h | 15 ++++++++++----- diff.c | 2 +- entry.c | 6 +++--- merge-recursive.c | 2 +- sha1-file.c | 4 ++-- 10 files changed, 45 insertions(+), 33 deletions(-)
apply.c +1 -1
@@ -4333,7 +4333,7 @@ static int try_create_file(const char *path, unsigned int mode, const char *buf,if (fd <0 )return 1 ; -if (convert_to_working_tree (path, buf, size, &nbuf)) { +if (convert_to_working_tree (&the_index, path, buf, size, &nbuf)) { size = nbuf.len; buf = nbuf.buf; }
archive.c +1 -1
@@ -78,7 +78,7 @@ void *object_file_to_archive(const struct archiver_args *args,size_t size =0 ;strbuf_attach (&buf, buffer, *sizep, *sizep +1 ); -convert_to_working_tree (path, buf.buf, buf.len, &buf); +convert_to_working_tree (&the_index, path, buf.buf, buf.len, &buf);if (commit)format_subst (commit, buf.buf, buf.len, &buf); buffer =strbuf_detach (&buf, &size);
builtin/cat-file.c +1 -1
@@ -38,7 +38,7 @@ static int filter_object(const char *path, unsigned mode, oid_to_hex(oid), path);if ((type ==OBJ_BLOB ) &&S_ISREG (mode)) {struct strbuf strbuf =STRBUF_INIT ; -if (convert_to_working_tree (path, *buf, *size, &strbuf)) { +if (convert_to_working_tree (&the_index, path, *buf, *size, &strbuf)) {free (*buf); *size = strbuf.len; *buf =strbuf_detach (&strbuf,NULL );
builtin/ls-files.c +1 -1
@@ -63,7 +63,7 @@ static void write_eolinfo(const struct index_state *istate,struct stat st;const char *i_txt ="" ;const char *w_txt ="" ; -const char *a_txt =get_convert_attr_ascii (path); +const char *a_txt =get_convert_attr_ascii (&the_index, path);if (ce &&S_ISREG (ce->ce_mode)) i_txt =get_cached_convert_stats_ascii (istate, ce->name);
convert.c +24 -17
122 lines not on this page · raw message
convert.h +10 -5
@@ -60,15 +60,18 @@ extern char *check_roundtrip_encoding;const char *get_cached_convert_stats_ascii (const struct index_state *istate,const char *path);const char *get_wt_convert_stats_ascii (const char *path); -const char *get_convert_attr_ascii (const char *path); +const char *get_convert_attr_ascii (const struct index_state *istate, +const char *path);/* returns 1 if *dst was used */ int convert_to_git (const struct index_state *istate,const char *path,const char *src,size_t len,struct strbuf *dst,int conv_flags); -int convert_to_working_tree (const char *path,const char *src, +int convert_to_working_tree (const struct index_state *istate, +const char *path,const char *src,size_t len,struct strbuf *dst); -int async_convert_to_working_tree (const char *path,const char *src, +int async_convert_to_working_tree (const struct index_state *istate, +const char *path,const char *src,size_t len,struct strbuf *dst,void *dco); int async_query_available_blobs(const char *cmd,
@@ -86,7 +89,8 @@ void convert_to_git_filter_fd(const struct index_state *istate, const char *path, int fd, struct strbuf *dst, int conv_flags); -int would_convert_to_git_filter_fd (const char *path); +int would_convert_to_git_filter_fd (const struct index_state *istate, +const char *path);/***************************************************************** *
@@ -96,7 +100,8 @@ int would_convert_to_git_filter_fd(const char *path);struct stream_filter ;/* opaque */ -struct stream_filter *get_stream_filter (const char *path, +struct stream_filter *get_stream_filter (const struct index_state *istate, +const char *path,const struct object_id *);void free_stream_filter (struct stream_filter *);int is_null_stream_filter (struct stream_filter *);
diff.c +1 -1
@@ -3695,7 +3695,7 @@ static void prep_temp_blob(const char *path, struct diff_tempfile *temp, temp->tempfile =mks_tempfile_ts (tempfile.buf,strlen (base) +1 );if (!temp->tempfile)die_errno ("unable to create temp-file" ); -if (convert_to_working_tree (path, +if (convert_to_working_tree (&the_index, path, (const char *)blob, (size_t )size, &buf)) { blob = buf.buf; size = buf.len;
entry.c +3 -3
@@ -265,7 +265,7 @@ static int write_entry(struct cache_entry *ce,const struct submodule *sub; if (ce_mode_s_ifmt == S_IFREG) { - struct stream_filter *filter = get_stream_filter(ce->name, + struct stream_filter *filter = get_stream_filter(&the_index, ce->name, &ce->oid); if (filter && !streaming_write_entry(ce, path, filter,
@@ -313,14 +313,14 @@ static int write_entry(struct cache_entry *ce,* Convert from git internal format to working tree format */ if (dco && dco->state !=CE_NO_DELAY ) { - ret =async_convert_to_working_tree (ce->name, new_blob, + ret =async_convert_to_working_tree (&the_index, ce->name, new_blob, size, &buf, dco);if (ret &&string_list_has_string (&dco->paths, ce->name)) {free (new_blob); goto delayed; } }else - ret =convert_to_working_tree (ce->name, new_blob, size, &buf); + ret =convert_to_working_tree (&the_index, ce->name, new_blob, size, &buf);if (ret) {free (new_blob);
merge-recursive.c +1 -1
@@ -961,7 +961,7 @@ static int update_file_flags(struct merge_options *o, }if (S_ISREG (mode)) {struct strbuf strbuf =STRBUF_INIT ; -if (convert_to_working_tree (path, buf, size, &strbuf)) { +if (convert_to_working_tree (&the_index, path, buf, size, &strbuf)) {free (buf); size = strbuf.len; buf =strbuf_detach (&strbuf,NULL );
sha1-file.c +2 -2
@@ -1860,7 +1860,7 @@ static int index_stream_convert_blob(struct object_id *oid, int fd,struct strbuf sbuf =STRBUF_INIT ;assert (path); -assert (would_convert_to_git_filter_fd (path)); +assert (would_convert_to_git_filter_fd (&the_index, path));convert_to_git_filter_fd (&the_index, path, fd, &sbuf,get_conv_flags (flags));
@@ -1950,7 +1950,7 @@ int index_fd(struct object_id *oid, int fd, struct stat *st,* Call xsize_t() only when needed to avoid potentially unnecessary * die() for large files. */ -if (type ==OBJ_BLOB && path &&would_convert_to_git_filter_fd (path)) +if (type ==OBJ_BLOB && path &&would_convert_to_git_filter_fd (&the_index, path)) ret =index_stream_convert_blob (oid, fd, path, flags);else if (!S_ISREG (st->st_mode)) ret =index_pipe (oid, fd, type, path, flags);
Signature · 1 line
2.18.0.rc0.333.g22e6ee6cdf
PATCH v4 10/23 dir.c: remove an implicit dependency on the_index in pathspec code
In reply to Nguyễn Thái Ngọc DuyMake the match_patchspec API and friends take an index_state instead of assuming the_index in dir.c. All external call sites are converted blindly to keep the patch simple and retain current behavior. Individual call sites may receive further updates to use the right index instead of the_index.
--- archive.c | 2 +- builtin/add.c | 6 +++--- builtin/checkout.c | 2 +- builtin/clean.c | 2 +- builtin/commit.c | 2 +- builtin/grep.c | 6 +++--- builtin/ls-files.c | 6 +++--- builtin/rm.c | 2 +- builtin/submodule--helper.c | 2 +- builtin/update-index.c | 2 +- diff-lib.c | 4 ++-- dir.c | 27 ++++++++++++++++----------- dir.h | 16 ++++++++++------ pathspec.c | 2 +- preload-index.c | 2 +- read-cache.c | 2 +- rerere.c | 2 +- resolve-undo.c | 2 +- revision.c | 2 +- submodule.c | 2 +- wt-status.c | 6 +++--- 21 files changed, 54 insertions(+), 45 deletions(-)
archive.c +1 -1
@@ -312,7 +312,7 @@ static int reject_entry(const struct object_id *oid, struct strbuf *base,struct strbuf sb =STRBUF_INIT ;strbuf_addbuf (&sb, base);strbuf_addstr (&sb, filename); -if (!match_pathspec (context, sb.buf, sb.len,0 ,NULL ,1 )) +if (!match_pathspec (&the_index, context, sb.buf, sb.len,0 ,NULL ,1 )) ret =READ_TREE_RECURSIVE ;strbuf_release (&sb); }
builtin/add.c +3 -3
@@ -40,7 +40,7 @@ static void chmod_pathspec(struct pathspec *pathspec, char flip)for (i =0 ; i < active_nr; i++) {struct cache_entry *ce = active_cache[i]; -if (pathspec && !ce_path_match (ce, pathspec,NULL )) +if (pathspec && !ce_path_match (&the_index, ce, pathspec,NULL ))continue ;if (chmod_cache_entry (ce, flip) <0 )
@@ -135,7 +135,7 @@ static int renormalize_tracked_files(const struct pathspec *pathspec, int flags)continue ;/* do not touch unmerged paths */ if (!S_ISREG (ce->ce_mode) && !S_ISLNK (ce->ce_mode))continue ;/* do not touch non blobs */ -if (pathspec && !ce_path_match (ce, pathspec,NULL )) +if (pathspec && !ce_path_match (&the_index, ce, pathspec,NULL ))continue ; retval |=add_file_to_cache (ce->name, flags |HASH_RENORMALIZE ); }
@@ -155,7 +155,7 @@ static char *prune_directory(struct dir_struct *dir, struct pathspec *pathspec, i = dir->nr;while (--i >=0 ) {struct dir_entry *entry = *src++; -if (dir_path_match (entry, pathspec, prefix, seen)) +if (dir_path_match (&the_index, entry, pathspec, prefix, seen)) *dst++ = entry; } dir->nr = dst - dir->entries;
builtin/checkout.c +1 -1
@@ -316,7 +316,7 @@ static int checkout_paths(const struct checkout_opts *opts,* match_pathspec() for _all_ entries when * opts->source_tree != NULL. */ -if (ce_path_match (ce, &opts->pathspec, ps_matched)) +if (ce_path_match (&the_index, ce, &opts->pathspec, ps_matched)) ce->ce_flags |=CE_MATCHED ; }
builtin/clean.c +1 -1
@@ -981,7 +981,7 @@ int cmd_clean(int argc, const char **argv, const char *prefix)continue ;if (pathspec.nr) - matches =dir_path_match (ent, &pathspec,0 ,NULL ); + matches =dir_path_match (&the_index, ent, &pathspec,0 ,NULL );if (pathspec.nr && !matches)continue ;
builtin/commit.c +1 -1
@@ -238,7 +238,7 @@ static int list_paths(struct string_list *list, const char *with_tree,if (ce->ce_flags &CE_UPDATE )continue ; -if (!ce_path_match (ce, pattern, m)) +if (!ce_path_match (&the_index, ce, pattern, m))continue ; item =string_list_insert (list, ce->name);if (ce_skip_worktree (ce))
builtin/grep.c +3 -3
@@ -497,7 +497,7 @@ static int grep_cache(struct grep_opt *opt, struct repository *repo,strbuf_addstr (&name, ce->name);if (S_ISREG (ce->ce_mode) && -match_pathspec (pathspec, name.buf, name.len,0 ,NULL , +match_pathspec (&the_index, pathspec, name.buf, name.len,0 ,NULL ,S_ISDIR (ce->ce_mode) ||S_ISGITLINK (ce->ce_mode))) {/*
@@ -515,7 +515,7 @@ static int grep_cache(struct grep_opt *opt, struct repository *repo, hit |=grep_file (opt, name.buf); } }else if (recurse_submodules &&S_ISGITLINK (ce->ce_mode) && -submodule_path_match (pathspec, name.buf,NULL )) { +submodule_path_match (&the_index, pathspec, name.buf,NULL )) { hit |=grep_submodule (opt, repo, pathspec,NULL , ce->name, ce->name); }else {continue ;
@@ -678,7 +678,7 @@ static int grep_directory(struct grep_opt *opt, const struct pathspec *pathspec,fill_directory (&dir, &the_index, pathspec);for (i =0 ; i < dir.nr; i++) { -if (!dir_path_match (dir.entries[i], pathspec,0 ,NULL )) +if (!dir_path_match (&the_index, dir.entries[i], pathspec,0 ,NULL ))continue ; hit |=grep_file (opt, dir.entries[i]->name);if (hit && opt->status_only)
builtin/ls-files.c +3 -3
@@ -128,7 +128,7 @@ static void show_dir_entry(const char *tag, struct dir_entry *ent)if (len > ent->len)die ("git ls-files: internal error - directory entry not superset of prefix" ); -if (!dir_path_match (ent, &pathspec, len, ps_matched)) +if (!dir_path_match (&the_index, ent, &pathspec, len, ps_matched))return ;fputs (tag, stdout);
@@ -228,7 +228,7 @@ static void show_ce(struct repository *repo, struct dir_struct *dir,if (recurse_submodules &&S_ISGITLINK (ce->ce_mode) &&is_submodule_active (repo, ce->name)) {show_submodule (repo, dir, ce->name); - }else if (match_pathspec (&pathspec, fullname,strlen (fullname), + }else if (match_pathspec (&the_index, &pathspec, fullname,strlen (fullname), max_prefix_len, ps_matched,S_ISDIR (ce->ce_mode) ||S_ISGITLINK (ce->ce_mode))) {
@@ -264,7 +264,7 @@ static void show_ru_info(const struct index_state *istate) len =strlen (path);if (len < max_prefix_len)continue ;/* outside of the prefix */ -if (!match_pathspec (&pathspec, path, len, +if (!match_pathspec (&the_index, &pathspec, path, len, max_prefix_len, ps_matched,0 ))continue ;/* uninterested */ for (i =0 ; i <3 ; i++) {
builtin/rm.c +1 -1
@@ -278,7 +278,7 @@ int cmd_rm(int argc, const char **argv, const char *prefix)for (i =0 ; i < active_nr; i++) {const struct cache_entry *ce = active_cache[i]; -if (!ce_path_match (ce, &pathspec, seen)) +if (!ce_path_match (&the_index, ce, &pathspec, seen))continue ;ALLOC_GROW (list.entry, list.nr +1 , list.alloc); list.entry[list.nr].name =xstrdup (ce->name);
builtin/submodule--helper.c +1 -1
@@ -330,7 +330,7 @@ static int module_list_compute(int argc, const char **argv,for (i =0 ; i < active_nr; i++) {const struct cache_entry *ce = active_cache[i]; -if (!match_pathspec (pathspec, ce->name,ce_namelen (ce), +if (!match_pathspec (&the_index, pathspec, ce->name,ce_namelen (ce),0 , ps_matched,1 ) || !S_ISGITLINK (ce->ce_mode))continue ;
builtin/update-index.c +1 -1
@@ -751,7 +751,7 @@ static int do_reupdate(int ac, const char **av,int save_nr;char *path; -if (ce_stage (ce) || !ce_path_match (ce, &pathspec,NULL )) +if (ce_stage (ce) || !ce_path_match (&the_index, ce, &pathspec,NULL ))continue ;if (has_head) old = read_one_ent(NULL, &head_oid,
diff-lib.c +2 -2
@@ -109,7 +109,7 @@ int run_diff_files(struct rev_info *revs, unsigned int option)if (diff_can_quit_early (&revs->diffopt))break ; -if (!ce_path_match (ce, &revs->prune_data,NULL )) +if (!ce_path_match (&the_index, ce, &revs->prune_data,NULL ))continue ;if (ce_stage (ce)) {
@@ -470,7 +470,7 @@ static int oneway_diff(const struct cache_entry * const *src,if (tree == o->df_conflict_entry) tree =NULL ; -if (ce_path_match (idx ? idx : tree, &revs->prune_data,NULL )) { +if (ce_path_match (&the_index, idx ? idx : tree, &revs->prune_data,NULL )) {do_oneway_diff (o, idx, tree);if (diff_can_quit_early (&revs->diffopt)) { o->exiting_early =1 ;
dir.c +16 -11
@@ -275,12 +275,13 @@ static int do_read_blob(const struct object_id *oid, struct oid_stat *oid_stat,#define DO_MATCH_DIRECTORY (1<<1)#define DO_MATCH_SUBMODULE (1<<2) -static int match_attrs (const char *name,int namelen, +static int match_attrs (const struct index_state *istate, +const char *name,int namelen,const struct pathspec_item *item) {int i; -git_check_attr (&the_index, name, item->attr_check); +git_check_attr (istate, name, item->attr_check);for (i =0 ; i < item->attr_match_nr; i++) {const char *value;int matched;
@@ -317,7 +318,8 @@ static int match_attrs(const char *name, int namelen,* * It returns 0 when there is no match. */ -static int match_pathspec_item (const struct pathspec_item *item,int prefix, +static int match_pathspec_item (const struct index_state *istate, +const struct pathspec_item *item,int prefix,const char *name,int namelen,unsigned flags) {/* name/namelen has prefix cut off by caller */
@@ -357,7 +359,7 @@ static int match_pathspec_item(const struct pathspec_item *item, int prefix, strncmp(item->match, name - prefix, item->prefix))return 0 ; -if (item->attr_match_nr && !match_attrs (name, namelen, item)) +if (item->attr_match_nr && !match_attrs (istate, name, namelen, item))return 0 ;/* If the match was just the prefix, we matched */
@@ -425,7 +427,8 @@ static int match_pathspec_item(const struct pathspec_item *item, int prefix,* pathspec did not match any names, which could indicate that the * user mistyped the nth pathspec. */ -static int do_match_pathspec (const struct pathspec *ps, +static int do_match_pathspec (const struct index_state *istate, +const struct pathspec *ps,const char *name,int namelen,int prefix,char *seen,unsigned flags)
@@ -471,7 +474,7 @@ static int do_match_pathspec(const struct pathspec *ps,*/ if (seen && ps->items[i].magic &PATHSPEC_EXCLUDE ) seen[i] =MATCHED_FNMATCH ; - how =match_pathspec_item (ps->items+i, prefix, name, + how =match_pathspec_item (istate, ps->items+i, prefix, name, namelen, flags); if (ps->recursive && (ps->magic & PATHSPEC_MAXDEPTH) &&
@@ -495,17 +498,18 @@ static int do_match_pathspec(const struct pathspec *ps,return retval; } -int match_pathspec (const struct pathspec *ps, +int match_pathspec (const struct index_state *istate, +const struct pathspec *ps,const char *name,int namelen,int prefix,char *seen,int is_dir) {int positive, negative;unsigned flags = is_dir ?DO_MATCH_DIRECTORY :0 ; - positive =do_match_pathspec (ps, name, namelen, + positive =do_match_pathspec (istate, ps, name, namelen, prefix, seen, flags);if (!(ps->magic &PATHSPEC_EXCLUDE ) || !positive)return positive; - negative =do_match_pathspec (ps, name, namelen, + negative =do_match_pathspec (istate, ps, name, namelen, prefix, seen, flags |DO_MATCH_EXCLUDE );return negative ?0 : positive;
@@ -514,11 +518,12 @@ int match_pathspec(const struct pathspec *ps,/** * Check if a submodule is a superset of the pathspec */ -int submodule_path_match (const struct pathspec *ps, +int submodule_path_match (const struct index_state *istate, +const struct pathspec *ps,const char *submodule_name,char *seen) { - int matched = do_match_pathspec(ps, submodule_name, + int matched = do_match_pathspec(istate, ps, submodule_name, strlen(submodule_name), 0, seen, DO_MATCH_DIRECTORY |
dir.h +10 -6
@@ -216,7 +216,8 @@ extern int count_slashes(const char *s);extern int simple_length (const char *match);extern int no_wildcard (const char *string);extern char *common_prefix (const struct pathspec *pathspec); -extern int match_pathspec (const struct pathspec *pathspec, +extern int match_pathspec (const struct index_state *istate, +const struct pathspec *pathspec,const char *name,int namelen,int prefix,char *seen,int is_dir);extern int report_path_error (const char *ps_matched,const struct pathspec *pathspec,const char *prefix);
@@ -326,25 +327,28 @@ extern int git_fnmatch(const struct pathspec_item *item, const char *pattern, const char *string, int prefix); -extern int submodule_path_match (const struct pathspec *ps, +extern int submodule_path_match (const struct index_state *istate, +const struct pathspec *ps,const char *submodule_name,char *seen); -static inline int ce_path_match (const struct cache_entry *ce, +static inline int ce_path_match (const struct index_state *istate, +const struct cache_entry *ce,const struct pathspec *pathspec,char *seen) { -return match_pathspec (pathspec, ce->name,ce_namelen (ce),0 , seen, +return match_pathspec (istate, pathspec, ce->name,ce_namelen (ce),0 , seen,S_ISDIR (ce->ce_mode) ||S_ISGITLINK (ce->ce_mode)); } -static inline int dir_path_match (const struct dir_entry *ent, +static inline int dir_path_match (const struct index_state *istate, +const struct dir_entry *ent,const struct pathspec *pathspec,int prefix,char *seen) {int has_trailing_dir = ent->len && ent->name[ent->len -1 ] =='/' ;int len = has_trailing_dir ? ent->len -1 : ent->len; -return match_pathspec (pathspec, ent->name, len, prefix, seen, +return match_pathspec (istate, pathspec, ent->name, len, prefix, seen, has_trailing_dir); }
pathspec.c +1 -1
@@ -37,7 +37,7 @@ void add_pathspec_matches_against_index(const struct pathspec *pathspec,return ;for (i =0 ; i < istate->cache_nr; i++) {const struct cache_entry *ce = istate->cache[i]; -ce_path_match (ce, pathspec, seen); +ce_path_match (&the_index, ce, pathspec, seen); } }
preload-index.c +1 -1
@@ -58,7 +58,7 @@ static void *preload_thread(void *_data)continue ;if (ce->ce_flags &CE_FSMONITOR_VALID )continue ; -if (!ce_path_match (ce, &p->pathspec,NULL )) +if (!ce_path_match (&the_index, ce, &p->pathspec,NULL ))continue ;if (threaded_has_symlink_leading_path (&cache, ce->name,ce_namelen (ce)))continue ;
read-cache.c +1 -1
@@ -1415,7 +1415,7 @@ int refresh_index(struct index_state *istate, unsigned int flags,if (ignore_submodules &&S_ISGITLINK (ce->ce_mode))continue ; -if (pathspec && !ce_path_match (ce, pathspec, seen)) +if (pathspec && !ce_path_match (&the_index, ce, pathspec, seen)) filtered =1 ;if (ce_stage (ce)) {
rerere.c +1 -1
@@ -1118,7 +1118,7 @@ int rerere_forget(struct pathspec *pathspec)find_conflict (&conflict);for (i =0 ; i < conflict.nr; i++) {struct string_list_item *it = &conflict.items[i]; -if (!match_pathspec (pathspec, it->string, +if (!match_pathspec (&the_index, pathspec, it->string,strlen (it->string),0 ,NULL ,0 ))continue ;rerere_forget_one_path (it->string, &merge_rr);
resolve-undo.c +1 -1
@@ -186,7 +186,7 @@ void unmerge_index(struct index_state *istate, const struct pathspec *pathspec)for (i =0 ; i < istate->cache_nr; i++) {const struct cache_entry *ce = istate->cache[i]; -if (!ce_path_match (ce, pathspec,NULL )) +if (!ce_path_match (&the_index, ce, pathspec,NULL ))continue ; i =unmerge_index_entry_at (istate, i); }
revision.c +1 -1
@@ -1501,7 +1501,7 @@ static void prepare_show_merge(struct rev_info *revs)const struct cache_entry *ce = active_cache[i];if (!ce_stage (ce))continue ; -if (ce_path_match (ce, &revs->prune_data,NULL )) { +if (ce_path_match (&the_index, ce, &revs->prune_data,NULL )) { prune_num++;REALLOC_ARRAY (prune, prune_num); prune[prune_num-2 ] = ce->name;
submodule.c +1 -1
@@ -258,7 +258,7 @@ int is_submodule_active(struct repository *repo, const char *path) }parse_pathspec (&ps,0 ,0 ,NULL , args.argv); - ret =match_pathspec (&ps, path,strlen (path),0 ,NULL ,1 ); + ret =match_pathspec (&the_index, &ps, path,strlen (path),0 ,NULL ,1 );argv_array_clear (&args);clear_pathspec (&ps);
wt-status.c +3 -3
@@ -647,7 +647,7 @@ static void wt_status_collect_changes_initial(struct wt_status *s)struct wt_status_change_data *d;const struct cache_entry *ce = active_cache[i]; -if (!ce_path_match (ce, &s->pathspec,NULL )) +if (!ce_path_match (&the_index, ce, &s->pathspec,NULL ))continue ;if (ce_intent_to_add (ce))continue ;
@@ -703,7 +703,7 @@ static void wt_status_collect_untracked(struct wt_status *s)for (i =0 ; i < dir.nr; i++) {struct dir_entry *ent = dir.entries[i];if (cache_name_is_other (ent->name, ent->len) && -dir_path_match (ent, &s->pathspec,0 ,NULL )) +dir_path_match (&the_index, ent, &s->pathspec,0 ,NULL ))string_list_insert (&s->untracked, ent->name);free (ent); }
@@ -711,7 +711,7 @@ static void wt_status_collect_untracked(struct wt_status *s)for (i =0 ; i < dir.ignored_nr; i++) {struct dir_entry *ent = dir.ignored[i];if (cache_name_is_other (ent->name, ent->len) && -dir_path_match (ent, &s->pathspec,0 ,NULL )) +dir_path_match (&the_index, ent, &s->pathspec,0 ,NULL ))string_list_insert (&s->ignored, ent->name);free (ent); }
Signature · 1 line
2.18.0.rc0.333.g22e6ee6cdf
PATCH v4 11/23 ls-files: correct index argument to get_convert_attr_ascii()
In reply to Nguyễn Thái Ngọc Duywrite_eolinfo() does take an istate as function argument and it should be used instead of the_index.
--- builtin/ls-files.c | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-)
builtin/ls-files.c +9 -8
@@ -63,7 +63,7 @@ static void write_eolinfo(const struct index_state *istate,struct stat st;const char *i_txt ="" ;const char *w_txt ="" ; -const char *a_txt =get_convert_attr_ascii (&the_index, path); +const char *a_txt =get_convert_attr_ascii (istate, path);if (ce &&S_ISREG (ce->ce_mode)) i_txt =get_cached_convert_stats_ascii (istate, ce->name);
@@ -121,18 +121,19 @@ static void print_debug(const struct cache_entry *ce) } } -static void show_dir_entry (const char *tag,struct dir_entry *ent) +static void show_dir_entry (const struct index_state *istate, +const char *tag,struct dir_entry *ent) {int len = max_prefix_len;if (len > ent->len)die ("git ls-files: internal error - directory entry not superset of prefix" ); -if (!dir_path_match (&the_index, ent, &pathspec, len, ps_matched)) +if (!dir_path_match (istate, ent, &pathspec, len, ps_matched))return ;fputs (tag, stdout); -write_eolinfo (NULL ,NULL , ent->name); +write_eolinfo (istate,NULL , ent->name);write_name (ent->name); }
@@ -145,7 +146,7 @@ static void show_other_files(const struct index_state *istate,struct dir_entry *ent = dir->entries[i];if (!index_name_is_other (istate, ent->name, ent->len))continue ; -show_dir_entry (tag_other, ent); +show_dir_entry (istate, tag_other, ent); } }
@@ -196,7 +197,7 @@ static void show_killed_files(const struct index_state *istate, } }if (killed) -show_dir_entry (tag_killed, dir->entries[i]); +show_dir_entry (istate, tag_killed, dir->entries[i]); } }
@@ -228,7 +229,7 @@ static void show_ce(struct repository *repo, struct dir_struct *dir,if (recurse_submodules &&S_ISGITLINK (ce->ce_mode) &&is_submodule_active (repo, ce->name)) {show_submodule (repo, dir, ce->name); - }else if (match_pathspec (&the_index, &pathspec, fullname,strlen (fullname), + }else if (match_pathspec (repo->index, &pathspec, fullname,strlen (fullname), max_prefix_len, ps_matched,S_ISDIR (ce->ce_mode) ||S_ISGITLINK (ce->ce_mode))) {
@@ -264,7 +265,7 @@ static void show_ru_info(const struct index_state *istate) len =strlen (path);if (len < max_prefix_len)continue ;/* outside of the prefix */ -if (!match_pathspec (&the_index, &pathspec, path, len, +if (!match_pathspec (istate, &pathspec, path, len, max_prefix_len, ps_matched,0 ))continue ;/* uninterested */ for (i =0 ; i <3 ; i++) {
Signature · 1 line
2.18.0.rc0.333.g22e6ee6cdf
PATCH v4 12/23 pathspec.c: use the right index instead of the_index
In reply to Nguyễn Thái Ngọc Duy--- pathspec.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
pathspec.c +1 -1
@@ -37,7 +37,7 @@ void add_pathspec_matches_against_index(const struct pathspec *pathspec,return ;for (i =0 ; i < istate->cache_nr; i++) {const struct cache_entry *ce = istate->cache[i]; -ce_path_match (&the_index, ce, pathspec, seen); +ce_path_match (istate, ce, pathspec, seen); } }
Signature · 1 line
2.18.0.rc0.333.g22e6ee6cdf
PATCH v4 13/23 submodule.c: use the right index instead of the_index
In reply to Nguyễn Thái Ngọc Duy--- submodule.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)
submodule.c +4 -4
@@ -93,7 +93,7 @@ int update_path_in_gitmodules(const char *oldpath, const char *newpath)if (!file_exists (GITMODULES_FILE ))/* Do nothing without .gitmodules */ return -1 ; -if (is_gitmodules_unmerged (&the_index)) +if (is_gitmodules_unmerged (the_repository->index))die (_ ("Cannot change unmerged .gitmodules, resolve merge conflicts first" )); submodule =submodule_from_path (the_repository, &null_oid, oldpath);
@@ -127,7 +127,7 @@ int remove_path_from_gitmodules(const char *path)if (!file_exists (GITMODULES_FILE ))/* Do nothing without .gitmodules */ return -1 ; -if (is_gitmodules_unmerged (&the_index)) +if (is_gitmodules_unmerged (the_repository->index))die (_ ("Cannot change unmerged .gitmodules, resolve merge conflicts first" )); submodule =submodule_from_path (the_repository, &null_oid, path);
@@ -188,7 +188,7 @@ void set_diffopt_flags_from_submodule_config(struct diff_options *diffopt,if (ignore)handle_ignore_submodules_arg (diffopt, ignore); -else if (is_gitmodules_unmerged (&the_index)) +else if (is_gitmodules_unmerged (the_repository->index)) diffopt->flags.ignore_submodules =1 ; } }
@@ -258,7 +258,7 @@ int is_submodule_active(struct repository *repo, const char *path) }parse_pathspec (&ps,0 ,0 ,NULL , args.argv); - ret =match_pathspec (&the_index, &ps, path,strlen (path),0 ,NULL ,1 ); + ret =match_pathspec (repo->index, &ps, path,strlen (path),0 ,NULL ,1 );argv_array_clear (&args);clear_pathspec (&ps);
Signature · 1 line
2.18.0.rc0.333.g22e6ee6cdf
PATCH v4 14/23 entry.c: use the right index instead of the_index
In reply to Nguyễn Thái Ngọc Duycheckout-index.c needs update because if checkout->istate is NULL, ie_match_stat() will crash. Previously this is ie_match_stat(&the_index, ..) so it will not crash, but it is not technically correct either.
--- builtin/checkout-index.c | 1 + entry.c | 10 ++++++---- 2 files changed, 7 insertions(+), 4 deletions(-)
builtin/checkout-index.c +1 -0
@@ -190,6 +190,7 @@ int cmd_checkout_index(int argc, const char **argv, const char *prefix) argc =parse_options (argc, argv, prefix, builtin_checkout_index_options, builtin_checkout_index_usage,0 ); + state.istate = &the_index; state.force = force; state.quiet = quiet; state.not_new = not_new;
entry.c +6 -4
@@ -1,3 +1,4 @@ +#define NO_THE_INDEX_COMPATIBILITY_MACROS #include "cache.h" #include "blob.h" #include "dir.h"
@@ -265,7 +266,7 @@ static int write_entry(struct cache_entry *ce,const struct submodule *sub; if (ce_mode_s_ifmt == S_IFREG) { - struct stream_filter *filter = get_stream_filter(&the_index, ce->name, + struct stream_filter *filter = get_stream_filter(state->istate, ce->name, &ce->oid); if (filter && !streaming_write_entry(ce, path, filter,
@@ -313,14 +314,14 @@ static int write_entry(struct cache_entry *ce,* Convert from git internal format to working tree format */ if (dco && dco->state !=CE_NO_DELAY ) { - ret =async_convert_to_working_tree (&the_index, ce->name, new_blob, + ret =async_convert_to_working_tree (state->istate, ce->name, new_blob, size, &buf, dco);if (ret &&string_list_has_string (&dco->paths, ce->name)) {free (new_blob); goto delayed; } }else - ret =convert_to_working_tree (&the_index, ce->name, new_blob, size, &buf); + ret =convert_to_working_tree (state->istate, ce->name, new_blob, size, &buf);if (ret) {free (new_blob);
@@ -421,7 +422,8 @@ int checkout_entry(struct cache_entry *ce,if (!check_path (path.buf, path.len, &st, state->base_dir_len)) {const struct submodule *sub; -unsigned changed =ce_match_stat (ce, &st,CE_MATCH_IGNORE_VALID |CE_MATCH_IGNORE_SKIP_WORKTREE ); +unsigned changed =ie_match_stat (state->istate, ce, &st, +CE_MATCH_IGNORE_VALID |CE_MATCH_IGNORE_SKIP_WORKTREE );/* * Needs to be checked before !changed returns early, * as the possibly empty directory was not changed
Signature · 1 line
2.18.0.rc0.333.g22e6ee6cdf
PATCH v4 15/23 attr: remove index from git_attr_set_direction()
In reply to Nguyễn Thái Ngọc DuySince attr checking API now take the index, there's no need to set an index in advance with this call. Most call sites are straightforward because they either pass the_index or NULL (which defaults back to the_index previously). There's only one suspicious call site in unpack-trees.c where it sets a different index.
This code in unpack-trees is about to checking out entries from the new/temporary index after merging is done in it. The attributes will be used by entry.c code to do crlf conversion if needed. entry.c now respects struct checkout's istate field, and this field is correctly set in unpack-trees.c, there should be no regression from this change.
--- archive.c | 2 +- attr.c | 15 +++------------ attr.h | 3 +-- builtin/check-attr.c | 2 +- unpack-trees.c | 4 ++-- 5 files changed, 8 insertions(+), 18 deletions(-)
archive.c +1 -1
@@ -273,7 +273,7 @@ int write_archive_entries(struct archiver_args *args,init_tree_desc (&t, args->tree->buffer, args->tree->size);if (unpack_trees (1 , &t, &opts))return -1 ; -git_attr_set_direction (GIT_ATTR_INDEX , &the_index); +git_attr_set_direction (GIT_ATTR_INDEX ); } err = read_tree_recursive(args->tree, "", 0, 0, &args->pathspec,
attr.c +3 -12
@@ -708,10 +708,8 @@ static struct attr_stack *read_attr_from_array(const char **list)* another thread could potentially be calling into the attribute system. */ static enum git_attr_direction direction; -static const struct index_state *use_index; -void git_attr_set_direction (enum git_attr_direction new_direction, -const struct index_state *istate) +void git_attr_set_direction (enum git_attr_direction new_direction) {if (is_bare_repository () && new_direction !=GIT_ATTR_INDEX )BUG ("non-INDEX attr direction in a bare repo" );
@@ -720,7 +718,6 @@ void git_attr_set_direction(enum git_attr_direction new_direction,drop_all_attr_stacks (); direction = new_direction; - use_index = istate; }static struct attr_stack *read_attr_from_file (const char *path,int macro_ok)
@@ -750,17 +747,11 @@ static struct attr_stack *read_attr_from_index(const struct index_state *istate,struct attr_stack *res;char *buf, *sp;int lineno =0 ; -const struct index_state *to_read_from; -/* -* Temporary workaround for c24f3abace (apply: file commited -* with CRLF should roundtrip diff and apply - 2017-08-19) -*/ - to_read_from = use_index ? use_index : istate; -if (!to_read_from) +if (!istate)return NULL ; - buf =read_blob_data_from_index (to_read_from, path,NULL ); + buf =read_blob_data_from_index (istate, path,NULL );if (!buf)return NULL ;
attr.h +1 -2
@@ -77,8 +77,7 @@ enum git_attr_direction {GIT_ATTR_CHECKOUT , GIT_ATTR_INDEX }; -void git_attr_set_direction (enum git_attr_direction new_direction, -const struct index_state *istate); +void git_attr_set_direction (enum git_attr_direction new_direction);void attr_start (void );
builtin/check-attr.c +1 -1
@@ -120,7 +120,7 @@ int cmd_check_attr(int argc, const char **argv, const char *prefix) }if (cached_attrs) -git_attr_set_direction (GIT_ATTR_INDEX ,NULL ); +git_attr_set_direction (GIT_ATTR_INDEX ); doubledash =-1 ;for (i =0 ; doubledash <0 && i < argc; i++) {
unpack-trees.c +2 -2
@@ -361,7 +361,7 @@ static int check_updates(struct unpack_trees_options *o) progress =get_progress (o);if (o->update) -git_attr_set_direction (GIT_ATTR_CHECKOUT , index); +git_attr_set_direction (GIT_ATTR_CHECKOUT );if (should_update_submodules () && o->update && !o->dry_run)load_gitmodules_file (index,NULL );
@@ -421,7 +421,7 @@ static int check_updates(struct unpack_trees_options *o)stop_progress (&progress); errs |=finish_delayed_checkout (&state);if (o->update) -git_attr_set_direction (GIT_ATTR_CHECKIN ,NULL ); +git_attr_set_direction (GIT_ATTR_CHECKIN );return errs !=0 ; }
Signature · 1 line
2.18.0.rc0.333.g22e6ee6cdf
Re: [PATCH v4 15/23] attr: remove index from git_attr_set_direction()
Quoted 7 lines · Nguyễn Thái Ngọc Duy
Since attr checking API now take the index, there's no need to set an index in advance with this call. Most call sites are straightforward because they either pass
the_indexor NULL (which defaults back tothe_indexpreviously). There's only one suspicious call site inunpack-trees.cwhere it sets a different index.This code in unpack-trees is about to checking out entries from the
minor nit: s/checking/check/ ?
...
Quoted 31 lines
attr.c +1
-6@@ -708,10 +708,8 @@ static struct attr_stack *read_attr_from_array(const char **list)* another thread could potentially be calling into the attribute system. */ static enum git_attr_direction direction; -static const struct index_state *use_index; -void git_attr_set_direction (enum git_attr_direction new_direction, -const struct index_state *istate) +void git_attr_set_direction (enum git_attr_direction new_direction) {if (is_bare_repository () && new_direction !=GIT_ATTR_INDEX )BUG ("non-INDEX attr direction in a bare repo" );@@ -720,7 +718,6 @@ void git_attr_set_direction(enum git_attr_direction new_direction,drop_all_attr_stacks (); direction = new_direction; - use_index = istate; }static struct attr_stack *read_attr_from_file (const char *path,int macro_ok)@@ -750,17 +747,11 @@ static struct attr_stack *read_attr_from_index(const struct index_state *istate,struct attr_stack *res;char *buf, *sp;int lineno =0 ; -const struct index_state *to_read_from; -/*
attr.c static struct attr_stack *read_attr_from_index(const struct index_state *istate, · line 754
Quoted 10 lines
attr.c +2
-6-* Temporary workaround for c24f3abace (apply: file commited -* with CRLF should roundtrip diff and apply - 2017-08-19) -*/ - to_read_from = use_index ? use_index : istate; -if (!to_read_from) +if (!istate)return NULL ; - buf =read_blob_data_from_index (to_read_from, path,NULL ); + buf =read_blob_data_from_index (istate, path,NULL );
This code is much clearer and easier to reason about than stashing off use_index. I had to dig for a bit through history to try to understand the old code and why it was written that way, but the new code just makes sense.
PATCH v4 16/23 preload-index.c: use the right index instead of the_index
In reply to Nguyễn Thái Ngọc Duy--- preload-index.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
preload-index.c +2 -1
@@ -1,6 +1,7 @@/* * Copyright (C) 2008 Linus Torvalds */ +#define NO_THE_INDEX_COMPATIBILITY_MACROS #include "cache.h" #include "pathspec.h" #include "dir.h"
@@ -58,7 +59,7 @@ static void *preload_thread(void *_data)continue ;if (ce->ce_flags &CE_FSMONITOR_VALID )continue ; -if (!ce_path_match (&the_index, ce, &p->pathspec,NULL )) +if (!ce_path_match (index, ce, &p->pathspec,NULL ))continue ;if (threaded_has_symlink_leading_path (&cache, ce->name,ce_namelen (ce)))continue ;
Signature · 1 line
2.18.0.rc0.333.g22e6ee6cdf
PATCH v4 17/23 read-cache.c: remove an implicit dependency on the_index
In reply to Nguyễn Thái Ngọc DuyMake some index API take an index_state instead of assuming the_index in read-cache.c. All external call sites are converted blindly to keep the patch simple and retain current behavior. Individual call sites may receive further updates to use the right index instead of the_index.
--- apply.c | 4 ++-- builtin/checkout.c | 4 ++-- builtin/difftool.c | 4 ++-- builtin/reset.c | 4 ++-- cache.h | 5 +++-- merge-recursive.c | 2 +- read-cache.c | 19 +++++++++++-------- resolve-undo.c | 2 +- 8 files changed, 24 insertions(+), 20 deletions(-)
apply.c +2 -2
@@ -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 (patch->old_mode, oid.hash, name,0 ,0 ); + ce =make_index_entry (&the_index, 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);
builtin/checkout.c +2 -2
@@ -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 (mode, oid.hash, path,2 ,0 ); + ce =make_index_entry (&the_index, 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;
builtin/difftool.c +2 -2
@@ -321,7 +321,7 @@ static int checkout_path(unsigned mode, struct object_id *oid,struct cache_entry *ce;int ret; - ce =make_cache_entry (mode, oid->hash, path,0 ,0 ); + ce =make_index_entry (&the_index, 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 (rmode, roid.hash, +make_index_entry (&the_index, rmode, roid.hash, dst_path,0 ,0 ); add_index_entry(&wtindex, ce2,
builtin/reset.c +2 -2
@@ -134,10 +134,10 @@ static void update_index_from_diff(struct diff_queue_struct *q,continue ; } - ce =make_cache_entry (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 ;
cache.h +3 -2
@@ -355,6 +355,7 @@ extern void free_name_hash(struct index_state *istate);#define unmerged_cache () unmerged_index(&the_index)#define cache_name_pos (name, namelen) index_name_pos(&the_index,(name),(namelen))#define add_cache_entry (ce, option) add_index_entry(&the_index, (ce), (option)) +#define refresh_cache_entry (ce, flags) refresh_index_entry(&the_index, ce, flags)#define rename_cache_entry_at (pos, new_name) rename_index_entry_at(&the_index, (pos), (new_name))#define remove_cache_entry_at (pos) remove_index_entry_at(&the_index, (pos))#define remove_file_from_cache (path) remove_file_from_index(&the_index, (path))
@@ -698,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 (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);
@@ -751,7 +752,7 @@ extern void fill_stat_cache_info(struct cache_entry *ce, struct stat *st);#define REFRESH_IGNORE_SUBMODULES 0x0010/* ignore submodules */ #define REFRESH_IN_PORCELAIN 0x0020/* user friendly output, not "needs update" */ extern int refresh_index (struct index_state *,unsigned int flags,const struct pathspec *pathspec,char *seen,const char *header_msg); -extern struct cache_entry *refresh_cache_entry (struct cache_entry *,unsigned int ); +extern struct cache_entry *refresh_index_entry (struct index_state *istate,struct cache_entry *,unsigned int );/* * Opportunistically update the index but do not complain if we can't.
merge-recursive.c +1 -1
@@ -315,7 +315,7 @@ static int add_cacheinfo(struct merge_options *o,struct cache_entry *ce;int ret; - ce =make_cache_entry (mode, oid ? oid->hash : null_sha1, path, stage,0 ); + ce =make_index_entry (&the_index, mode, oid ? oid->hash : null_sha1, path, stage,0 );if (!ce)return err (o,_ ("add_cacheinfo failed for path '%s'; merge aborting." ), path);
read-cache.c +11 -8
@@ -745,9 +745,11 @@ int add_file_to_index(struct index_state *istate, const char *path, int flags)return add_to_index (istate, path, &st, flags); } -struct cache_entry *make_cache_entry (unsigned int mode, -const unsigned char *sha1,const char *path,int stage, -unsigned int refresh_options) +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) {int size, len;struct cache_entry *ce, *ret;
@@ -767,7 +769,7 @@ struct cache_entry *make_cache_entry(unsigned int mode, ce->ce_namelen = len; ce->ce_mode =create_ce_mode (mode); - ret =refresh_cache_entry (ce, refresh_options); + ret =refresh_index_entry (istate, ce, refresh_options);if (ret != ce)free (ce);return ret;
@@ -1415,7 +1417,7 @@ int refresh_index(struct index_state *istate, unsigned int flags,if (ignore_submodules &&S_ISGITLINK (ce->ce_mode))continue ; -if (pathspec && !ce_path_match (&the_index, ce, pathspec, seen)) +if (pathspec && !ce_path_match (istate, ce, pathspec, seen)) filtered =1 ;if (ce_stage (ce)) {
@@ -1473,10 +1475,11 @@ int refresh_index(struct index_state *istate, unsigned int flags,return has_errors; } -struct cache_entry *refresh_cache_entry (struct cache_entry *ce, -unsigned int options) +struct cache_entry *refresh_index_entry (struct index_state *istate, +struct cache_entry *ce, +unsigned int options) { -return refresh_cache_ent (&the_index, ce, options,NULL ,NULL ); +return refresh_cache_ent (istate, ce, options,NULL ,NULL ); }
resolve-undo.c +1 -1
@@ -146,7 +146,7 @@ int unmerge_index_entry_at(struct index_state *istate, int pos)struct cache_entry *nce;if (!ru->mode[i])continue ; - nce =make_cache_entry (ru->mode[i], ru->oid[i].hash, + nce =make_index_entry (&the_index, ru->mode[i], ru->oid[i].hash, name, i +1 ,0 );if (matched) nce->ce_flags |=CE_MATCHED ;
Signature · 1 line
2.18.0.rc0.333.g22e6ee6cdf
Re: [PATCH v4 17/23] read-cache.c: remove an implicit dependency on the_index
merge-recursive.c static int add_cacheinfo(struct merge_options *o, · line 320
Quoted 12 lines · Nguyễn Thái Ngọc Duy
merge-recursive.c +1
-1@@ -315,7 +315,7 @@ static int add_cacheinfo(struct merge_options *o,struct cache_entry *ce;int ret; - ce =make_cache_entry (mode, oid ? oid->hash : null_sha1, path, stage,0 ); + ce =make_index_entry (&the_index, mode, oid ? oid->hash : null_sha1, path, stage,0 );if (!ce)return err (o,_ ("add_cacheinfo failed for path '%s'; merge aborting." ), path);
There's also a refresh_cache_entry() call about ten lines after this; since you converted all other make_cache_entry() and refresh_cache_entry() calls in this patch, I'm curious if that one was left out for a reason or was just an oversight.
There are also a lot of add_cache_entry() calls in this function. I'm guessing we should either convert all of those too, or just change back this particular make_index_entry to make_cache_entry() as it was; it seems weird to have a mix of explicit the_index and implicit the_index usages in the same function. If we convert them all, perhaps we should consider having merge_options store the index we're working on? If you want to punt this until later or leave it for me while I make all my ongoing merge-recursive changes, that's fine. Just thought I'd point it out.
Re: [PATCH v4 17/23] read-cache.c: remove an implicit dependency on the_index
Quoted 10 lines · Elijah Newren
Quoted 8 lines · Nguyễn Thái Ngọc Duy
merge-recursive.c +0
-0@@ -315,7 +315,7 @@ static int add_cacheinfo(struct merge_options *o,struct cache_entry *ce;int ret;
Quoted 9 lines
merge-recursive.c static int add_cacheinfo(struct merge_options *o, · line 320
Quoted 4 lines
merge-recursive.c +1
-1- ce =make_cache_entry (mode, oid ? oid->hash : null_sha1, path, stage,0 ); + ce =make_index_entry (&the_index, mode, oid ? oid->hash : null_sha1, path, stage,0 );if (!ce)return err (o,_ ("add_cacheinfo failed for path '%s'; merge aborting." ), path);There's also a
refresh_cache_entry()call about ten lines after this; since you converted all othermake_cache_entry()andrefresh_cache_entry()calls in this patch, I'm curious if that one was left out for a reason or was just an oversight.
Ah I didn't mean to convert or kill refresh_cache_entry(), not outside read-cache.c. I rely on NO_THE_INDEX_COMPATIBILITY_MACROS to catch *cache* functions and if we set it in this file, we're going to have a lot more work to do and plenty of the_index will show up.
Quoted 5 lines
There are also a lot of
add_cache_entry()calls in this function. I'm guessing we should either convert all of those too, or just change back this particularmake_index_entrytomake_cache_entry()as it was; it seems weird to have a mix of explicitthe_indexand implicitthe_indexusages in the same function.
Yes some files still have the mix of the_index and *cache*(). This one and apply.c come to mind. There's more work to do to kill all the_index outside builtin/
Quoted 5 lines
If we convert them all,
perhaps we should consider havingmerge_optionsstore the index we're working on? If you want to punt this until later or leave it for me while I make all my ongoing merge-recursive changes, that's fine. Just thought I'd point it out.
Right you're updating merge-recursive.c, I'd love it if you could define NO_THE_INDEX_COMPATIBILITY_MACROS here. Yes merge_options sounds like a good place to tell merge-recursive where to get a struct index_state.
Signature · 1 line
Duy
Re: [PATCH v4 17/23] read-cache.c: remove an implicit dependency on the_index
Quoted 32 lines · Duy Nguyen
Quoted 10 lines · Elijah Newren
Quoted 8 lines · Nguyễn Thái Ngọc Duy
merge-recursive.c +0
-0@@ -315,7 +315,7 @@ static int add_cacheinfo(struct merge_options *o,struct cache_entry *ce;int ret;Quoted 9 lines
merge-recursive.c static int add_cacheinfo(struct merge_options *o, · line 320
Quoted 4 lines
merge-recursive.c +1
-1- ce =make_cache_entry (mode, oid ? oid->hash : null_sha1, path, stage,0 ); + ce =make_index_entry (&the_index, mode, oid ? oid->hash : null_sha1, path, stage,0 );if (!ce)return err (o,_ ("add_cacheinfo failed for path '%s'; merge aborting." ), path);There's also a
refresh_cache_entry()call about ten lines after this; since you converted all othermake_cache_entry()andrefresh_cache_entry()calls in this patch, I'm curious if that one was left out for a reason or was just an oversight.Ah I didn't mean to convert or kill
refresh_cache_entry(), not outsideread-cache.c. I rely onNO_THE_INDEX_COMPATIBILITY_MACROSto catch *cache* functions and if we set it in this file, we're going to have a lot more work to do and plenty ofthe_indexwill show up.Quoted 5 lines
There are also a lot of
add_cache_entry()calls in this function. I'm guessing we should either convert all of those too, or just change back this particularmake_index_entrytomake_cache_entry()as it was; it seems weird to have a mix of explicitthe_indexand implicitthe_indexusages in the same function.
Quoted 3 lines
Yes some files still have the mix of
the_indexand *cache*(). This one andapply.ccome to mind. There's more work to do to kill allthe_indexoutside builtin/
Yeah, there's also low-level common files like sha1-file.c or ll-merge.c which now have &the_index in them (or more references to it). Clearly those need to be propagated up, but as you say, there's more work to do. Your patch series just moves things in the right direction.
Quoted 10 lines
Quoted 5 lines
If we convert them all,
perhaps we should consider havingmerge_optionsstore the index we're working on? If you want to punt this until later or leave it for me while I make all my ongoing merge-recursive changes, that's fine. Just thought I'd point it out.Right you're updating
merge-recursive.c, I'd love it if you could defineNO_THE_INDEX_COMPATIBILITY_MACROShere. Yesmerge_optionssounds like a good place to tell merge-recursive where to get a structindex_state.
Sure, I can tackle that.
PATCH v4 18/23 apply.c: use the right index instead of the_index
In reply to Nguyễn Thái Ngọc Duy--- apply.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
apply.c +1 -1
@@ -4090,7 +4090,7 @@ static int build_fake_ancestor(struct apply_state *state, struct patch *list)return error (_ ("sha1 information is lacking or useless " "(%s)." ), name); - ce =make_index_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_index_entry failed for path '%s'" ), name);
Signature · 1 line
2.18.0.rc0.333.g22e6ee6cdf
PATCH v4 19/23 difftool: use the right index instead of the_index
In reply to Nguyễn Thái Ngọc Duy--- builtin/difftool.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
builtin/difftool.c +2 -2
@@ -321,7 +321,7 @@ static int checkout_path(unsigned mode, struct object_id *oid,struct cache_entry *ce;int ret; - ce =make_index_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_index_entry (&the_index, rmode, roid.hash, +make_index_entry (&wtindex, rmode, roid.hash, dst_path,0 ,0 ); add_index_entry(&wtindex, ce2,
Signature · 1 line
2.18.0.rc0.333.g22e6ee6cdf
PATCH v4 20/23 checkout: avoid the_index when possible
In reply to Nguyễn Thái Ngọc Duy--- builtin/checkout.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
builtin/checkout.c +1 -1
@@ -230,7 +230,7 @@ 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_index_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_index_entry failed for path '%s'" ), path); status =checkout_entry (ce, state,NULL );
Signature · 1 line
2.18.0.rc0.333.g22e6ee6cdf
PATCH v4 21/23 resolve-undo.c: use the right index instead of the_index
In reply to Nguyễn Thái Ngọc Duy--- resolve-undo.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)
resolve-undo.c +3 -2
@@ -1,3 +1,4 @@ +#define NO_THE_INDEX_COMPATIBILITY_MACROS #include "cache.h" #include "dir.h" #include "resolve-undo.h"
@@ -146,7 +147,7 @@ int unmerge_index_entry_at(struct index_state *istate, int pos)struct cache_entry *nce;if (!ru->mode[i])continue ; - nce =make_index_entry (&the_index, ru->mode[i], ru->oid[i].hash, + nce =make_index_entry (istate, ru->mode[i], ru->oid[i].hash, name, i +1 ,0 );if (matched) nce->ce_flags |=CE_MATCHED ;
@@ -186,7 +187,7 @@ void unmerge_index(struct index_state *istate, const struct pathspec *pathspec)for (i =0 ; i < istate->cache_nr; i++) {const struct cache_entry *ce = istate->cache[i]; -if (!ce_path_match (&the_index, ce, pathspec,NULL )) +if (!ce_path_match (istate, ce, pathspec,NULL ))continue ; i =unmerge_index_entry_at (istate, i); }
Signature · 1 line
2.18.0.rc0.333.g22e6ee6cdf
PATCH v4 22/23 grep: use the right index instead of the_index
In reply to Nguyễn Thái Ngọc Duy--- builtin/grep.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
builtin/grep.c +2 -2
@@ -497,7 +497,7 @@ static int grep_cache(struct grep_opt *opt, struct repository *repo,strbuf_addstr (&name, ce->name);if (S_ISREG (ce->ce_mode) && -match_pathspec (&the_index, pathspec, name.buf, name.len,0 ,NULL , +match_pathspec (repo->index, pathspec, name.buf, name.len,0 ,NULL ,S_ISDIR (ce->ce_mode) ||S_ISGITLINK (ce->ce_mode))) {/*
@@ -515,7 +515,7 @@ static int grep_cache(struct grep_opt *opt, struct repository *repo, hit |=grep_file (opt, name.buf); } }else if (recurse_submodules &&S_ISGITLINK (ce->ce_mode) && -submodule_path_match (&the_index, pathspec, name.buf,NULL )) { +submodule_path_match (repo->index, pathspec, name.buf,NULL )) { hit |=grep_submodule (opt, repo, pathspec,NULL , ce->name, ce->name); }else {continue ;
Signature · 1 line
2.18.0.rc0.333.g22e6ee6cdf
PATCH v4 23/23 cache.h: make the_index part of "compatibility macros"
In reply to Nguyễn Thái Ngọc DuyWhile the_index is not actually a macro, its use throughout the code base is dangerous because developers sometimes may not see that some function is using the_index (instead of some other index that the devs are interested in).
By keeping the_index part of this NO_ macro, we try to reduce its use more and more until it's completely gone.
--- cache.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
cache.h +2 -2
@@ -330,8 +330,6 @@ struct index_state {struct ewah_bitmap *fsmonitor_dirty; }; -extern struct index_state the_index; -/* Name hashing */ extern int test_lazy_init_name_hash (struct index_state *istate,int try_threaded);extern void add_name_hash (struct index_state *istate,struct cache_entry *ce);
@@ -340,6 +338,8 @@ extern void free_name_hash(struct index_state *istate);#ifndef NO_THE_INDEX_COMPATIBILITY_MACROS +extern struct index_state the_index; +#define active_cache (the_index.cache)#define active_nr (the_index.cache_nr)#define active_alloc (the_index.cache_alloc)
Signature · 1 line
2.18.0.rc0.333.g22e6ee6cdf
Re: [PATCH v4 00/23] Fix incorrect use of the_index
In reply to Nguyễn Thái Ngọc DuyQuoted 2 lines · Nguyễn Thái Ngọc Duy
v4 fixes some commit messages and killed a couple more
the_indexreferences 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
Quoted 9 lines · Elijah Newren
Quoted 2 lines · Nguyễn Thái Ngọc Duy
v4 fixes some commit messages and killed a couple more
the_indexreferences 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.cstuff 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.
Quoted 2 lines
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
Quoted 6 lines · Elijah Newren
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_indexusages to callers ofll-merge.candsha1-file.cinstead of having them in those files, to mixes of _cache_ and _index_ functions as inapply.candmerge-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.
Signature · 1 line
Duy
Re: [PATCH v4 00/23] Fix incorrect use of the_index
Hi Duy,
Quoted 17 lines · Duy Nguyen
Quoted 6 lines · Elijah Newren
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_indexusages to callers ofll-merge.candsha1-file.cinstead of having them in those files, to mixes of _cache_ and _index_ functions as inapply.candmerge-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_indexin three files outside builtin/: merge-recursive,notes-merge.candtransport.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 NULLindex_stateto some functions, but fixing them up to tolerate NULLindex_state. So it's going to be a few patch series untilthe_indexis gone for good [1].
Quoted 4 lines
[1] but like cheap horror movies, there's always a sequel:
the_repositoryis 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/[email protected]/.
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
Quoted 20 lines · Elijah Newren
Hi Duy,
Quoted 16 lines · Duy Nguyen
Quoted 6 lines · Elijah Newren
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_indexusages to callers ofll-merge.candsha1-file.cinstead of having them in those files, to mixes of _cache_ and _index_ functions as inapply.candmerge-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_indexin three files outside builtin/: merge-recursive,notes-merge.candtransport.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 NULLindex_stateto some functions, but fixing them up to tolerate NULLindex_state. So it's going to be a few patch series untilthe_indexis 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.
Quoted 6 lines
Quoted 5 lines
[1] but like cheap horror movies, there's always a sequel:
the_repositoryis 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.
Quoted 6 lines
I was just about to send you an email to ask if you were continuing on with the series. I need
diff-lib.cconverted in order to make the changes Junio suggested toindex_has_changesat
https://public-inbox.org/git/[email protected]/.
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 ;-)
Signature · 1 line
Duy
Re: [PATCH v4 00/23] Fix incorrect use of the_index
Quoted 7 lines · Duy Nguyen
Quoted 6 lines
I was just about to send you an email to ask if you were continuing on with the series. I need
diff-lib.cconverted in order to make the changes Junio suggested toindex_has_changesat
https://public-inbox.org/git/[email protected]/.
Since you're already working on that, I won't duplicate your effort. Thanks for tackling all of this. :-)
Quoted 8 lines
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 'structindex_state*' and just pass&the_indexin 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.)
Quoted 5 lines
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 keepmerge-recursive.cthe_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
Quoted 6 lines · Elijah Newren
Quoted 5 lines
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 keepmerge-recursive.cthe_index-free now, I will not stop you ;-)
Quoted 6 lines
I was just worried that since I was making changes in
merge-recursive.cthat 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
Signature · 1 line
Duy
Re: [PATCH v4 00/23] Fix incorrect use of the_index
Quoted 14 lines · Duy Nguyen
Quoted 6 lines · Elijah Newren
Quoted 5 lines
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 keepmerge-recursive.cthe_index-free now, I will not stop you ;-)Quoted 6 lines
I was just worried that since I was making changes in
merge-recursive.cthat 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.
Quoted 3 lines
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< --
builtin/am.c +1 -1
@@ -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);
builtin/checkout.c +1 -1
@@ -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);
builtin/merge-recursive.c +1 -1
@@ -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 ="" ;
builtin/merge.c +1 -1
@@ -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 ="" ;
merge-recursive.c +85 -70
475 lines not on this page · raw message
merge-recursive.h +5 -1
@@ -4,6 +4,8 @@#include "unpack-trees.h" #include "string-list.h" +struct repository ; +struct merge_options {const char *ancestor;const char *branch1;
@@ -31,6 +33,7 @@ struct merge_options {struct string_list df_conflict_file_set;struct unpack_trees_options unpack_opts;struct index_state orig_index; +struct repository *repo; };/*
@@ -89,7 +92,8 @@ int merge_recursive_generic(struct merge_options *o,const struct object_id **ca, struct commit **result); -void init_merge_options (struct merge_options *o); +void init_merge_options (struct merge_options *o, +struct repository *repo);struct tree *write_tree_from_memory (struct merge_options *o);int parse_merge_opt (struct merge_options *out,const char *s);
sequencer.c +2 -2
@@ -532,7 +532,7 @@ static int do_recursive_merge(struct repository *repo,repo_read_index (repo); -init_merge_options (&o); +init_merge_options (&o, repo); o.ancestor = base ? base_label :"(empty tree)" ; o.branch1 ="HEAD" ; o.branch2 = next ? next_label :"(empty tree)" ;
@@ -3022,7 +3022,7 @@ static int do_merge(struct repository *repo,free_commit_list (bases);repo_read_index (repo); -init_merge_options (&o); +init_merge_options (&o, repo); o.branch1 ="HEAD" ; o.branch2 = ref_name.buf; o.buffer_output =2 ;
-- 8< --
Quoted 2 lines
Signature · 1 line
Duy
Re: [PATCH v4 00/23] Fix incorrect use of the_index
Quoted 25 lines · Duy Nguyen
Quoted 14 lines · Duy Nguyen
Quoted 6 lines · Elijah Newren
Quoted 5 lines
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 keepmerge-recursive.cthe_index-free now, I will not stop you ;-)Quoted 6 lines
I was just worried that since I was making changes in
merge-recursive.cthat 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.Quoted 3 lines
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". Sometimeso->repo->indexis not the correct answer.
Quoted 6 lines
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
Quoted 632 lines
-- 8< --
builtin/am.c +1
-1@@ -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);builtin/checkout.c +1
-1@@ -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);builtin/merge-recursive.c +1
-1@@ -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 ="" ;builtin/merge.c +1
-1@@ -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 ="" ;merge-recursive.c +85
-70@@ -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,CE_MATCH_REFRESH |CE_MATCH_IGNORE_MISSING );if (!nce)return err (o,_ ("add_cacheinfo failed to refresh for path '%s'; merge aborting." ), path);if (nce != ce) - ret =add_index_entry (&the_index, nce, options); + ret =add_index_entry (istate, nce, options); }return ret; }@@ -356,7 +359,7 @@ static int unpack_trees_start(struct merge_options *o, o->unpack_opts.merge =1 ; o->unpack_opts.head_idx =2 ; o->unpack_opts.fn = threeway_merge; - o->unpack_opts.src_index = &the_index; + o->unpack_opts.src_index = o->repo->index; o->unpack_opts.dst_index = &tmp_index; o->unpack_opts.aggressive = !merge_detect_rename (o);setup_unpack_trees_porcelain (&o->unpack_opts,"merge" );@@ -366,16 +369,16 @@ static int unpack_trees_start(struct merge_options *o,init_tree_desc_from_tree (t+2 , merge); rc =unpack_trees (3 , t, &o->unpack_opts); -cache_tree_free (&the_index.cache_tree); +cache_tree_free (&o->repo->index->cache_tree);/* -* Update the_index to match the new results, AFTER saving a copy +* Update o->repo->index to match the new results, AFTER saving a copy * in o->orig_index. Update src_index to point to the saved copy. * (verify_uptodate() checks src_index, and the original index is * the one that had the necessary modification timestamps.) */ - o->orig_index = the_index; - the_index = tmp_index; + o->orig_index = *o->repo->index; + *o->repo->index = tmp_index; o->unpack_opts.src_index = &o->orig_index;return rc;@@ -390,12 +393,13 @@ static void unpack_trees_finish(struct merge_options *o)struct tree *write_tree_from_memory (struct merge_options *o) {struct tree *result =NULL ; +struct index_state *istate = o->repo->index; -if (unmerged_index (&the_index)) { +if (unmerged_index (istate)) {int i;fprintf (stderr,"BUG: There are unmerged index entries:\n" ); -for (i =0 ; i < the_index.cache_nr; i++) { -const struct cache_entry *ce = the_index.cache[i]; +for (i =0 ; i < istate->cache_nr; i++) { +const struct cache_entry *ce = istate->cache[i];if (ce_stage (ce))fprintf (stderr,"BUG: %d %.*s\n" ,ce_stage (ce), (int )ce_namelen (ce), ce->name);@@ -403,16 +407,16 @@ struct tree *write_tree_from_memory(struct merge_options *o)BUG ("unmerged index entries in merge-recursive.c" ); } -if (!the_index.cache_tree) - the_index.cache_tree =cache_tree (); +if (!istate->cache_tree) + istate->cache_tree =cache_tree (); -if (!cache_tree_fully_valid (the_index.cache_tree) && -cache_tree_update (&the_index,0 ) <0 ) { +if (!cache_tree_fully_valid (istate->cache_tree) && +cache_tree_update (istate,0 ) <0 ) {err (o,_ ("error building trees" ));return NULL ; } - result =lookup_tree (&the_index.cache_tree->oid); + result =lookup_tree (&istate->cache_tree->oid);return result; }@@ -482,17 +486,17 @@ static struct stage_data *insert_stage_data(const char *path,* Create a dictionary mapping file names to stage_data objects. The * dictionary contains one entry for every path with a non-zero stage entry. */ -static struct string_list *get_unmerged (void ) +static struct string_list *get_unmerged (struct index_state *istate) {struct string_list *unmerged =xcalloc (1 ,sizeof (struct string_list ));int i; unmerged->strdup_strings =1 ; -for (i =0 ; i < the_index.cache_nr; i++) { +for (i =0 ; i < istate->cache_nr; i++) {struct string_list_item *item;struct stage_data *e; -const struct cache_entry *ce = the_index.cache[i]; +const struct cache_entry *ce = istate->cache[i];if (!ce_stage (ce))continue ;@@ -652,7 +656,7 @@ static int update_stages(struct merge_options *opt, const char *path,int clear =1 ;int options =ADD_CACHE_OK_TO_ADD |ADD_CACHE_SKIP_DFCHECK ;if (clear) -if (remove_file_from_index (&the_index, path)) +if (remove_file_from_index (opt->repo->index, path))return -1 ;if (o)if (add_cacheinfo (opt, o->mode, &o->oid, path,1 ,0 , options))@@ -708,13 +712,13 @@ static int remove_file(struct merge_options *o, int clean,int update_working_directory = !o->call_depth && !no_wd;if (update_cache) { -if (remove_file_from_index (&the_index, path)) +if (remove_file_from_index (o->repo->index, path))return -1 ; }if (update_working_directory) {if (ignore_case) {struct cache_entry *ce; - ce =index_file_exists (&the_index, path,strlen (path), + ce =index_file_exists (o->repo->index, path,strlen (path), ignore_case);if (ce &&ce_stage (ce) ==0 &&strcmp (path, ce->name))return 0 ;@@ -765,7 +769,8 @@ static char *unique_path(struct merge_options *o, const char *path, const char ** check the working directory. If empty_ok is non-zero, also return * 0 in the case where the working-tree dir exists but is empty. */ -static int dir_in_way (const char *path,int check_working_copy,int empty_ok) +static int dir_in_way (struct index_state *istate,const char *path, +int check_working_copy,int empty_ok) {int pos;struct strbuf dirpath =STRBUF_INIT ;@@ -774,12 +779,12 @@ static int dir_in_way(const char *path, int check_working_copy, int empty_ok)strbuf_addstr (&dirpath, path);strbuf_addch (&dirpath,'/' ); - pos =index_name_pos (&the_index, dirpath.buf, dirpath.len); + pos =index_name_pos (istate, dirpath.buf, dirpath.len);if (pos <0 ) pos =-1 - pos; -if (pos < the_index.cache_nr && - !strncmp (dirpath.buf, the_index.cache[pos]->name, dirpath.len)) { +if (pos < istate->cache_nr && + !strncmp (dirpath.buf, istate->cache[pos]->name, dirpath.len)) {strbuf_release (&dirpath);return 1 ; }@@ -822,7 +827,7 @@ static int was_tracked(struct merge_options *o, const char *path)return 0 ; } -static int would_lose_untracked (const char *path) +static int would_lose_untracked (struct index_state *istate,const char *path) {/* * This may look like it can be simplified to: @@ -841,19 +846,19 @@ static int would_lose_untracked(const char *path)* update_file()/would_lose_untracked(); see every comment in this * file which mentions "update_stages". */ -int pos =index_name_pos (&the_index, path,strlen (path)); +int pos =index_name_pos (istate, path,strlen (path));if (pos <0 ) pos =-1 - pos; -while (pos < the_index.cache_nr && - !strcmp (path, the_index.cache[pos]->name)) { +while (pos < istate->cache_nr && + !strcmp (path, istate->cache[pos]->name)) {/* * If stage #0, it is definitely tracked. * If it has stage #2 then it was tracked * before this merge started. All other * cases the path was not tracked. */ -switch (ce_stage (the_index.cache[pos])) { +switch (ce_stage (istate->cache[pos])) {case 0 :case 2 :return 0 ;@@ -913,7 +918,7 @@ static int make_room_for_path(struct merge_options *o, const char *path)* Do not unlink a file in the work tree if we are not * tracking it. */ -if (would_lose_untracked (path)) +if (would_lose_untracked (o->repo->index, path))return err (o,_ ("refusing to lose untracked file at '%s'" ), path);@@ -963,7 +968,7 @@ static int update_file_flags(struct merge_options *o, }if (S_ISREG (mode)) {struct strbuf strbuf =STRBUF_INIT ; -if (convert_to_working_tree (&the_index, path, buf, size, &strbuf)) { +if (convert_to_working_tree (o->repo->index, path, buf, size, &strbuf)) {free (buf); size = strbuf.len; buf =strbuf_detach (&strbuf,NULL );@@ -1081,7 +1086,7 @@ static int merge_3way(struct merge_options *o, merge_status =ll_merge (result_buf, a->path, &orig, base_name, &src1, name1, &src2, name2, - &the_index, &ll_opts); + o->repo->index, &ll_opts);free (base_name);free (name1);@@ -1092,8 +1097,9 @@ static int merge_3way(struct merge_options *o,return merge_status; } -static int find_first_merges (struct object_array *result,const char *path, -struct commit *a,struct commit *b) +static int find_first_merges (struct repository *repo, +struct object_array *result,const char *path, +struct commit *a,struct commit *b) {int i, j;struct object_array merges =OBJECT_ARRAY_INIT ;@@ -1112,7 +1118,7 @@ static int find_first_merges(struct object_array *result, const char *path,/* get all revisions that merge commit a */ xsnprintf (merged_revision,sizeof (merged_revision),"^%s" ,oid_to_hex (&a->object.oid)); -init_revisions (&revs, the_repository,NULL ); +init_revisions (&revs, repo,NULL ); rev_opts.submodule = path;/* FIXME: can't handle linked worktrees in submodules yet */ revs.single_worktree = path !=NULL ;@@ -1242,7 +1248,8 @@ static int merge_submodule(struct merge_options *o,return 0 ;/* find commit which merges them */ - parent_count =find_first_merges (&merges, path, commit_a, commit_b); + parent_count =find_first_merges (o->repo, &merges, path, + commit_a, commit_b);switch (parent_count) {case 0 :output (o,1 ,_ ("Failed to merge submodule %s (merge following commits not found)" ), path);@@ -1420,7 +1427,7 @@ static int conflict_rename_dir(struct merge_options *o, {const struct diff_filespec *dest = pair->two; -if (!o->call_depth &&would_lose_untracked (dest->path)) { +if (!o->call_depth &&would_lose_untracked (o->repo->index, dest->path)) {char *alt_path =unique_path (o, dest->path, rename_branch); output(o, 1, _("Error: Refusing to lose untracked file at %s; "@@ -1458,8 +1465,8 @@ static int handle_change_delete(struct merge_options *o,const char *update_path = path;int ret =0 ; -if (dir_in_way (path, !o->call_depth,0 ) || - (!o->call_depth &&would_lose_untracked (path))) { +if (dir_in_way (o->repo->index, path, !o->call_depth,0 ) || + (!o->call_depth &&would_lose_untracked (o->repo->index, path))) { update_path = alt_path =unique_path (o, path, change_branch); }@@ -1469,7 +1476,7 @@ static int handle_change_delete(struct merge_options *o,* correct; since there is no true "middle point" between * them, simply reuse the base version for virtual merge base. */ - ret =remove_file_from_index (&the_index, path); + ret =remove_file_from_index (o->repo->index, path);if (!ret) ret =update_file (o,0 , o_oid, o_mode, update_path); }else {@@ -1530,7 +1537,7 @@ static int conflict_rename_delete(struct merge_options *o,return -1 ;if (o->call_depth) -return remove_file_from_index (&the_index, dest->path); +return remove_file_from_index (o->repo->index, dest->path);else return update_stages(o, dest->path, NULL, rename_branch == o->branch1 ? dest : NULL,@@ -1592,12 +1599,12 @@ static int handle_file(struct merge_options *o,remove_file (o,0 , rename->path, ren_src_was_dirty); dst_name =unique_path (o, rename->path, cur_branch); }else { -if (dir_in_way (rename->path, !o->call_depth,0 )) { +if (dir_in_way (o->repo->index, rename->path, !o->call_depth,0 )) { dst_name =unique_path (o, rename->path, cur_branch);output (o,1 ,_ ("%s is a directory in %s adding as %s instead" ), rename->path, other_branch, dst_name); }else if (!o->call_depth && -would_lose_untracked (rename->path)) { +would_lose_untracked (o->repo->index, rename->path)) { dst_name =unique_path (o, rename->path, cur_branch); output(o, 1, _("Refusing to lose untracked file at %s; " "adding as %s instead"),@@ -1665,14 +1672,14 @@ static int conflict_rename_rename_1to2(struct merge_options *o,return -1 ; }else -remove_file_from_index (&the_index, a->path); +remove_file_from_index (o->repo->index, a->path); add =filespec_from_entry (&other, ci->dst_entry2,3 ^1 );if (add) {if (update_file (o,0 , &add->oid, add->mode, b->path))return -1 ; }else -remove_file_from_index (&the_index, b->path); +remove_file_from_index (o->repo->index, b->path); }else if (handle_file (o, a,2 , ci) ||handle_file (o, b,3 , ci))return -1 ;@@ -1680,8 +1687,9 @@ static int conflict_rename_rename_1to2(struct merge_options *o, }static int conflict_rename_rename_2to1 (struct merge_options *o, -struct rename_conflict_info *ci) +struct rename_conflict_info *ci) { +struct index_state *istate = o->repo->index;/* Two files, a & b, were renamed to the same thing, c. */ struct diff_filespec *a = ci->pair1->one;struct diff_filespec *b = ci->pair2->one;@@ -1700,8 +1708,12 @@ static int conflict_rename_rename_2to1(struct merge_options *o, a->path, c1->path, ci->branch1, b->path, c2->path, ci->branch2); -remove_file (o,1 , a->path, o->call_depth ||would_lose_untracked (a->path)); -remove_file (o,1 , b->path, o->call_depth ||would_lose_untracked (b->path)); +remove_file (o,1 , a->path, + (o->call_depth || +would_lose_untracked (istate, a->path))); +remove_file (o,1 , b->path, + (o->call_depth || +would_lose_untracked (istate, b->path))); path_side_1_desc =xstrfmt ("%s (was %s)" , path, a->path); path_side_2_desc =xstrfmt ("%s (was %s)" , path, b->path);@@ -1738,7 +1750,7 @@ static int conflict_rename_rename_2to1(struct merge_options *o,if (was_dirty (o, path))output (o,1 ,_ ("Refusing to lose dirty file at %s" ), path); -else if (would_lose_untracked (path)) +else if (would_lose_untracked (istate, path))/* * Only way we get here is if both renames were from * a directory rename AND user had an untracked file @@ -1789,7 +1801,7 @@ static struct diff_queue_struct *get_diffpairs(struct merge_options *o,struct diff_queue_struct *ret;struct diff_options opts; -diff_setup (&opts, &the_index); +diff_setup (&opts, o->repo->index); opts.flags.recursive =1 ; opts.flags.rename_empty =0 ; opts.detect_rename =merge_detect_rename (o);@@ -2949,8 +2961,8 @@ static int blob_unchanged(struct merge_options *opt,* performed. Comparison can be skipped if both files are * unchanged since their sha1s have already been compared. */ -if (renormalize_buffer (&the_index, path, o.buf, o.len, &o) | -renormalize_buffer (&the_index, path, a.buf, a.len, &a)) +if (renormalize_buffer (opt->repo->index, path, o.buf, o.len, &o) | +renormalize_buffer (opt->repo->index, path, a.buf, a.len, &a)) ret = (o.len == a.len && !memcmp (o.buf, a.buf, o.len)); error_return:@@ -3028,7 +3040,7 @@ static int merge_content(struct merge_options *o, o->branch2 == rename_conflict_info->branch1) ? pair1->two->path : pair1->one->path; -if (dir_in_way (path, !o->call_depth, +if (dir_in_way (o->repo->index, path, !o->call_depth,S_ISGITLINK (pair1->two->mode))) df_conflict_remains =1 ; }@@ -3066,7 +3078,7 @@ static int merge_content(struct merge_options *o,if (df_conflict_remains || is_dirty) {char *new_path;if (o->call_depth) { -remove_file_from_index (&the_index, path); +remove_file_from_index (o->repo->index, path); }else {if (!mfi.clean) {if (update_stages (o, path, &one, &a, &b))@@ -3212,7 +3224,7 @@ static int process_entry(struct merge_options *o, oid = b_oid; conf =_ ("directory/file" ); } -if (dir_in_way (path, +if (dir_in_way (o->repo->index, path, !o->call_depth && !S_ISGITLINK (a_mode),0 )) {char *new_path =unique_path (o, path, add_branch);@@ -3223,7 +3235,7 @@ static int process_entry(struct merge_options *o,if (update_file (o,0 , oid, mode, new_path)) clean_merge =-1 ;else if (o->call_depth) -remove_file_from_index (&the_index, path); +remove_file_from_index (o->repo->index, path);free (new_path); }else {output (o,2 ,_ ("Adding %s" ), path);@@ -3256,6 +3268,7 @@ int merge_trees(struct merge_options *o,struct tree *common, struct tree **result) { + struct index_state *istate = o->repo->index;int code, clean;if (o->subtree_shift) {@@ -3266,7 +3279,7 @@ int merge_trees(struct merge_options *o,if (oid_eq (&common->object.oid, &merge->object.oid)) {struct strbuf sb =STRBUF_INIT ; -if (!o->call_depth &&index_has_changes (&the_index, &sb)) { +if (!o->call_depth &&index_has_changes (istate, &sb)) {err (o,_ ("Dirty index: cannot merge (dirty: %s)" ), sb.buf);return 0 ;@@ -3287,7 +3300,7 @@ int merge_trees(struct merge_options *o,return -1 ; } -if (unmerged_index (&the_index)) { +if (unmerged_index (istate)) {struct string_list *entries;struct rename_info re_info;int i;@@ -3302,7 +3315,7 @@ int merge_trees(struct merge_options *o,get_files_dirs (o, head);get_files_dirs (o, merge); - entries =get_unmerged (); + entries =get_unmerged (o->repo->index); clean =handle_renames (o, common, head, merge, entries, &re_info);record_df_conflict_files (o, entries);@@ -3418,7 +3431,7 @@ int merge_recursive(struct merge_options *o,* overwritten it: the committed "conflicts" were * already resolved. */ -discard_index (&the_index); +discard_index (o->repo->index); saved_b1 = o->branch1; saved_b2 = o->branch2; o->branch1 ="Temporary merge branch 1" ;@@ -3434,9 +3447,9 @@ int merge_recursive(struct merge_options *o,return err (o,_ ("merge returned no commit" )); } -discard_index (&the_index); +discard_index (o->repo->index);if (!o->call_depth) -repo_read_index (the_repository); +repo_read_index (o->repo); o->ancestor ="merged common ancestors" ; clean = merge_trees(o, get_commit_tree(h1), get_commit_tree(h2),@@ -3501,7 +3514,7 @@ int merge_recursive_generic(struct merge_options *o, } } -repo_hold_locked_index (the_repository, &lock,LOCK_DIE_ON_ERROR ); +repo_hold_locked_index (o->repo, &lock,LOCK_DIE_ON_ERROR ); clean =merge_recursive (o, head_commit, next_commit, ca, result);if (clean <0 ) {@@ -3509,7 +3522,7 @@ int merge_recursive_generic(struct merge_options *o,return clean; } -if (write_locked_index (&the_index, &lock, +if (write_locked_index (o->repo->index, &lock,COMMIT_LOCK |SKIP_IF_UNCHANGED ))return err (o,_ ("Unable to write index." ));@@ -3533,10 +3546,12 @@ static void merge_recursive_config(struct merge_options *o)git_config (git_xmerge_config,NULL ); } -void init_merge_options (struct merge_options *o) +void init_merge_options (struct merge_options *o, +struct repository *repo) {const char *merge_verbosity;memset (o,0 ,sizeof (struct merge_options )); + o->repo = repo; o->verbosity =2 ; o->buffer_output =1 ; o->diff_rename_limit =-1 ;merge-recursive.h +5
-1@@ -4,6 +4,8 @@#include "unpack-trees.h" #include "string-list.h" +struct repository ; +struct merge_options {const char *ancestor;const char *branch1;@@ -31,6 +33,7 @@ struct merge_options {struct string_list df_conflict_file_set;struct unpack_trees_options unpack_opts;struct index_state orig_index; +struct repository *repo; };/* @@ -89,7 +92,8 @@ int merge_recursive_generic(struct merge_options *o,const struct object_id **ca, struct commit **result); -void init_merge_options (struct merge_options *o); +void init_merge_options (struct merge_options *o, +struct repository *repo);struct tree *write_tree_from_memory (struct merge_options *o);int parse_merge_opt (struct merge_options *out,const char *s);sequencer.c +2
-3@@ -532,7 +532,7 @@ static int do_recursive_merge(struct repository *repo,repo_read_index (repo); -init_merge_options (&o); +init_merge_options (&o, repo); o.ancestor = base ? base_label :"(empty tree)" ; o.branch1 ="HEAD" ; o.branch2 = next ? next_label :"(empty tree)" ;@@ -3022,7 +3022,7 @@ static int do_merge(struct repository *repo,free_commit_list (bases);repo_read_index (repo); -init_merge_options (&o); +init_merge_options (&o, repo); o.branch1 ="HEAD" ; o.branch2 = ref_name.buf; o.buffer_output =2 ; -- 8< --Quoted 3 lines
--
Duy
Re: [PATCH v4 00/23] Fix incorrect use of the_index
In reply to Duy NguyenQuoted 10 lines · Duy Nguyen
Quoted 6 lines · Elijah Newren
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_indexusages to callers ofll-merge.candsha1-file.cinstead of having them in those files, to mixes of _cache_ and _index_ functions as inapply.candmerge-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)
Quoted 15 lines
i'm down to leaving
the_index
in three files outside builtin/: merge-recursive,notes-merge.candtransport.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 NULLindex_stateto some functions, but fixing them up to tolerate NULLindex_state. So it's going to be a few patch series untilthe_indexis gone for good [1].[1] but like cheap horror movies, there's always a sequel:
the_repositoryis 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
Signature · 1 line
Duy