Re: [PATCH v3 3/3] correct ce_compare_data() in a middle of a merge

From: Junio C Hamano <gitster@pobox.com>
Date: Jul 7, 2016, 18:43 UTC
Message-ID: <xmqqr3b5p9v0.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <c36fe487-b8dc-9767-7fae-bee513dac0b2@web.de>

Torsten Bögershausen <tboegi@web.de> writes:

Show 47 quoted lines

> This is the callstack:
>
> merge-recursive.c/add_cacheinfo(unsigned int mode, const unsigned char *sha1,
> 		const char *path, int stage, int refresh, int options)
> {
> 	struct cache_entry *ce;
> 	ce = make_cache_entry
> 	if (!ce)
> 		return error(_("addinfo_cache failed for path '%s'"), path);
> 	return add_cache_entry(ce, options);
> }
> #Calls
> read-cache.c/make_cache_entry(path=file sha1=0x99b633 stage=0)
>
>
> struct cache_entry *make_cache_entry(unsigned int mode,
> 		const unsigned char *sha1, const char *path, int stage,
> 		unsigned int refresh_options)
> {
>         [snip]
> 	ret = refresh_cache_entry(ce, refresh_options);
> 	if (ret != ce)
> 		free(ce);
> 	return ret;
> }
>
> #Calls
> refresh_cache_ent(&the_index, ce, options, NULL, NULL);
>
> #Calls
> ie_modified()
>
> #Calls
> read-cache.c/ie_match_stat()
>
> #Calls
> changed |= ce_modified_check_fs(ce, st);
>
> #Calls
> ce_compare_data(path=file sha1=0x99b633)
>
> #Calls
> index_fd(..., ..., ce->name, ...)
> #Note the sha is lost here, the parameter sha is the output.
>
> Deep down, has_cr_in_index(path) will look at ad55e2 instead,
> which is wrong.

The call to add_cacheinfo() is made with 99b633 to record the 3-way merge result to the index in this callchain:

 merge_trees()
 -> git_merge_trees()
 -> process_renames() # does nothing for this case
 -> process_entry()
    -> merge_content()
       -> merge_file_special_markers()
          -> merge_file_1()
             -> merge_3way()
                -> ll_merge() # correctly handles the renormalization
             -> write_sha1_file() # this is what gives us 99b633
    -> update_file() # this is fed the 99b633 write_sha1_file() computed
       -> update_file_flags()
          -> read_sha1_file() # reads 99b633
          -> convert_to_working_tree()
          -> write_in_full() # updates the working tree
          -> add_cacheinfo() # to record 99b633 at "file" in the index

So refresh() may incorrectly find that 99b633 does not match what is in the filesystem if it looks at _any_ entry in the index. We know we have written the file ourselves, so by definition it should match ;-) Even though I am not sure why that should affect the overall correctness of the program (because we have written the correct result to both working tree and to the index), this needs to be fixed.

I am however not convinced passing the full SHA-1 is a good solution. The make_cache_entry() function may be creating a cache entry to stuff in a non-default index (i.e. not "the_index"), but its caller does not have a way to tell it which index the resulting cache entry will go, and calls refresh_cache_entry(), which always assumes that the caller is interested in "the_index", so what add_cacheinfo() does by calling make_cache_entry() feels wrong in the first place. Also, the call from update_file_flags() knows that the working tree is in sync with the resulting cache entry.

Perhaps update_file_flags() should not even call add_cacheinfo() there in the first place? I wonder if it should just call add_file_to_index()---after all, even though the current code already knows that 99b633 _is_ the result it wants in the index, it still goes to the filesystem and rehashes.

I dunno. I really do not like that extra sha1 argument added all over the callchain by this patch.

Or did you mean other calls to add_cacheinfo()?