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

[Junio C Hamano <gitster@pobox.com>]

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

> 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()?