git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [PATCH 06/22] make sure partially read index is not changed

From
Thomas Gummerer <t.gummerer@gmail.com>
Date
Jul 8, 2013, 18:33 UTC
Message-ID
<877gh0rjfk.fsf@gmail.com>
In-Reply-To
<7vehb910a6.fsf@alter.siamese.dyndns.org>
Junio C Hamano <gitster@pobox.com> writes:
Show 17 quoted lines
> Thomas Gummerer <t.gummerer@gmail.com> writes:
>
>> A partially read index file currently cannot be written to disk.  Make
>> sure that never happens, by re-reading the index file if the index file
>> wasn't read completely before changing the in-memory index.
>
> I am not quite sure what you are trying to do.
>
> In operations that modify the index (replace_index_entry(),
> remove_index_entry_at(), etc.)  you lift the filter_ops and keep
> partially_read flag still on.  In the write-out codepath, you have
> an assert to make sure the caller has cleared the partially_read
> flag.  A natural way to clear the flag is to re-read the index from
> the file, but then you can easily lose the modifications.
>
> Also shouldn't the flag be cleared upon discard_index()?  If it is
> done there, you probably would not need to clear it in read_index().

Hrm, maybe the code isn't quite clear enough here, or maybe the patch should come directly before (16/22) read-cache: read index-v5 to be more clear.

The flag is always set to 0 in read_index_v2, as the whole index is always read and therefore it never needs to be reset. With read_index_v5 on the other hand the flag is set when the filter_opts are different than NULL.

But thinking about it, the flag is actually not necessary at all. The filter_opts should simply be checked for NULL for the assert and they should also be set to NULL on discard_index. Will fix this in the next version. Thanks.

> Should
> there be another safety that says "calling read_index() with the
> partially_read flag on is a bug" or something?

I'm not sure. I think it doesn't hurt, as we discard the index when we change the index_ops. At the moment I can't think of a case where where calling read_index() could be used when it's partially read without discarding the cache first. I'll add it in the next version.

Show 114 quoted lines
>>
>> Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>
>> ---
>>  builtin/update-index.c | 4 ++++
>>  cache.h                | 4 +++-
>>  read-cache-v2.c        | 3 +++
>>  read-cache.c           | 8 ++++++++
>>  4 files changed, 18 insertions(+), 1 deletion(-)
>>
>> diff --git a/builtin/update-index.c b/builtin/update-index.c
>> index 5c7762e..03f6426 100644
>> --- a/builtin/update-index.c
>> +++ b/builtin/update-index.c
>> @@ -49,6 +49,8 @@ static int mark_ce_flags(const char *path, int flag, int mark)
>>  	int namelen = strlen(path);
>>  	int pos = cache_name_pos(path, namelen);
>>  	if (0 <= pos) {
>> +		if (active_cache_partially_read)
>> +			cache_change_filter_opts(NULL);
>>  		if (mark)
>>  			active_cache[pos]->ce_flags |= flag;
>>  		else
>> @@ -253,6 +255,8 @@ static void chmod_path(int flip, const char *path)
>>  	pos = cache_name_pos(path, strlen(path));
>>  	if (pos < 0)
>>  		goto fail;
>> +	if (active_cache_partially_read)
>> +		cache_change_filter_opts(NULL);
>>  	ce = active_cache[pos];
>>  	mode = ce->ce_mode;
>>  	if (!S_ISREG(mode))
>> diff --git a/cache.h b/cache.h
>> index d38dfbd..f6c3407 100644
>> --- a/cache.h
>> +++ b/cache.h
>> @@ -293,7 +293,8 @@ struct index_state {
>>  	struct cache_tree *cache_tree;
>>  	struct cache_time timestamp;
>>  	unsigned name_hash_initialized : 1,
>> -		 initialized : 1;
>> +		 initialized : 1,
>> +		 partially_read : 1;
>>  	struct hash_table name_hash;
>>  	struct hash_table dir_hash;
>>  	struct index_ops *ops;
>> @@ -315,6 +316,7 @@ extern void free_name_hash(struct index_state *istate);
>>  #define active_alloc (the_index.cache_alloc)
>>  #define active_cache_changed (the_index.cache_changed)
>>  #define active_cache_tree (the_index.cache_tree)
>> +#define active_cache_partially_read (the_index.partially_read)
>>
>>  #define read_cache() read_index(&the_index)
>>  #define read_cache_from(path) read_index_from(&the_index, (path))
>> diff --git a/read-cache-v2.c b/read-cache-v2.c
>> index 1ed640d..2cc792d 100644
>> --- a/read-cache-v2.c
>> +++ b/read-cache-v2.c
>> @@ -273,6 +273,7 @@ static int read_index_v2(struct index_state *istate, void *mmap,
>>  		src_offset += 8;
>>  		src_offset += extsize;
>>  	}
>> +	istate->partially_read = 0;
>>  	return 0;
>>  unmap:
>>  	munmap(mmap, mmap_size);
>> @@ -495,6 +496,8 @@ static int write_index_v2(struct index_state *istate, int newfd)
>>  	struct stat st;
>>  	struct strbuf previous_name_buf = STRBUF_INIT, *previous_name;
>>
>> +	if (istate->partially_read)
>> +		die("BUG: index: cannot write a partially read index");
>>  	for (i = removed = extended = 0; i < entries; i++) {
>>  		if (cache[i]->ce_flags & CE_REMOVE)
>>  			removed++;
>> diff --git a/read-cache.c b/read-cache.c
>> index b30ee75..4529fab 100644
>> --- a/read-cache.c
>> +++ b/read-cache.c
>> @@ -30,6 +30,8 @@ static void replace_index_entry(struct index_state *istate, int nr, struct cache
>>  {
>>  	struct cache_entry *old = istate->cache[nr];
>>
>> +	if (istate->partially_read)
>> +		index_change_filter_opts(istate, NULL);
>>  	remove_name_hash(istate, old);
>>  	set_index_entry(istate, nr, ce);
>>  	istate->cache_changed = 1;
>> @@ -467,6 +469,8 @@ int remove_index_entry_at(struct index_state *istate, int pos)
>>  {
>>  	struct cache_entry *ce = istate->cache[pos];
>>
>> +	if (istate->partially_read)
>> +		index_change_filter_opts(istate, NULL);
>>  	record_resolve_undo(istate, ce);
>>  	remove_name_hash(istate, ce);
>>  	istate->cache_changed = 1;
>> @@ -978,6 +982,8 @@ int add_index_entry(struct index_state *istate, struct cache_entry *ce, int opti
>>  {
>>  	int pos;
>>
>> +	if (istate->partially_read)
>> +		index_change_filter_opts(istate, NULL);
>>  	if (option & ADD_CACHE_JUST_APPEND)
>>  		pos = istate->cache_nr;
>>  	else {
>> @@ -1184,6 +1190,8 @@ int refresh_index(struct index_state *istate, unsigned int flags, const char **p
>>  				/* If we are doing --really-refresh that
>>  				 * means the index is not valid anymore.
>>  				 */
>> +				if (istate->partially_read)
>> +					index_change_filter_opts(istate, NULL);
>>  				ce->ce_flags &= ~CE_VALID;
>>  				istate->cache_changed = 1;
>>  			}
Previous: Junio C HamanoNext: Thomas Gummerer
Message 26 of 51 in “Index v5”
  1. 00/22 Index v5Thomas Gummerer, Jul 7, 2013
  2. 01/22 t2104: Don't fail for index versions other than [23]Thomas Gummerer, Jul 7, 2013
  3. 02/22 read-cache: split index file version specific functionalityThomas Gummerer, Jul 7, 2013
  4. 03/22 read-cache: move index v2 specific functions to their own fileThomas Gummerer, Jul 7, 2013
  5. 04/22 read-cache: Re-read index if index file changedThomas Gummerer, Jul 7, 2013
  6. 05/22 read-cache: add index reading apiThomas Gummerer, Jul 7, 2013
  7. Duy NguyenJul 8, 2013
  8. Thomas GummererJul 8, 2013
  9. Duy NguyenJul 8, 2013
  10. Thomas GummererJul 8, 2013
  11. Duy NguyenJul 8, 2013
  12. Thomas GummererJul 8, 2013
  13. 5/22 Add documentation for the index apiThomas Gummerer, Jul 8, 2013
  14. Duy NguyenJul 9, 2013
  15. Thomas GummererJul 9, 2013
  16. Duy NguyenJul 10, 2013
  17. Thomas GummererJul 11, 2013
  18. Duy NguyenJul 11, 2013
  19. Duy NguyenJul 11, 2013
  20. Junio C HamanoJul 8, 2013
  21. Thomas GummererJul 8, 2013
  22. Junio C HamanoJul 8, 2013
  23. Thomas GummererJul 9, 2013
  24. 06/22 make sure partially read index is not changedThomas Gummerer, Jul 7, 2013
  25. Junio C HamanoJul 8, 2013
  26. Thomas GummererJul 8, 2013
  27. 07/22 dir.c: use index apiThomas Gummerer, Jul 7, 2013
  28. 08/22 tree.c: use index apiThomas Gummerer, Jul 7, 2013
  29. 09/22 name-hash.c: use index apiThomas Gummerer, Jul 7, 2013
  30. 10/22 grep.c: Use index apiThomas Gummerer, Jul 7, 2013
  31. 11/22 ls-files.c: use the index apiThomas Gummerer, Jul 7, 2013
  32. 12/22 read-cache: make read_blob_data_from_index use index apiThomas Gummerer, Jul 7, 2013
  33. 13/22 documentation: add documentation of the index-v5 file formatThomas Gummerer, Jul 7, 2013
  34. Duy NguyenJul 11, 2013
  35. Thomas GummererJul 11, 2013
  36. Duy NguyenJul 11, 2013
  37. Thomas GummererJul 11, 2013
  38. Duy NguyenJul 11, 2013
  39. 14/22 read-cache: make in-memory format aware of stat_crcThomas Gummerer, Jul 7, 2013
  40. 15/22 read-cache: read index-v5Thomas Gummerer, Jul 7, 2013
  41. Eric SunshineJul 7, 2013
  42. Thomas GummererJul 8, 2013
  43. 16/22 read-cache: read resolve-undo dataThomas Gummerer, Jul 7, 2013
  44. 17/22 read-cache: read cache-tree in index-v5Thomas Gummerer, Jul 7, 2013
  45. Eric SunshineJul 7, 2013
  46. 18/22 read-cache: write index-v5Thomas Gummerer, Jul 7, 2013
  47. Eric SunshineJul 7, 2013
  48. 19/22 read-cache: write index-v5 cache-tree dataThomas Gummerer, Jul 7, 2013
  49. 20/22 read-cache: write resolve-undo data for index-v5Thomas Gummerer, Jul 7, 2013
  50. 21/22 update-index.c: rewrite index when index-version is givenThomas Gummerer, Jul 7, 2013
  51. 22/22 p0003-index.sh: add perf test for the index formatsThomas Gummerer, Jul 7, 2013

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.