From: Torsten Bögershausen Date: Fri, 08 Jul 2016 07:52:30 GMT Subject: Re: [PATCH v3 3/3] correct ce_compare_data() in a middle of a merge Message-ID: <2cbf12a6-2dca-8180-323b-f79638aa03bd@web.de> In-Reply-To: On 07/07/16 20:43, Junio C Hamano wrote: > Torsten Bögershausen 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. It seems at least that this is the correct way to do it. > 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()? I didn't mean too much - the whole call chain touches code where I am not able to comment on details. I'm happy to test other implementations, if someone suggests a path, so to say.