[The copy of my message to the list bounced; trying to resend...]
Hi,
Thanks for flagging this problem, providing a clear testcase, and working on it.
On Fri, Jan 24, 2014 at 7:01 AM, Brad King <brad.king@xxxxxxxxxxx> wrote:
>
> Teach add_cacheinfo to optionally tolerate make_cache_entry failure when
> the reason is ENOENT from lstat. Tell it to do so in the call path when
> the entry from HEAD is known to be up to date.
>
> This fixes the 'merge-recursive w/ empty work tree - ours has rename'
> case in t3030-merge-recursive.
While this change does work for the particular new testcase you
provided, there's a more complex case where merge-recursive is failing
that is not yet found in the testsuite and not fully reflected with
your new test. In particular, if you combine your special case of an
empty work tree with other special cases such as renames across a D/F
conflict, then git merge will fail and your change would merely
suppress part of the error messages.
To make this concrete, try modifying the 'merging with triple rename
across D/F conflict' testcase in t6031-merge-recursive.sh (an example
that should merge cleanly) to remove all files from the working tree
right before the merge (which shoudln't affect whether the merge is
clean). Currently, git merge will fail with:
error: addinfo_cache failed for path 'sub1/file3'
error: addinfo_cache failed for path 'sub1/file2'
error: addinfo_cache failed for path 'sub1/file1'
sub1/file1: unmerged (ac3e272b72bbf89def8657766b855d0656630ed4)
sub1/file2: unmerged (637f0347d31dad180d6fc7f6720c187b05a8754c)
sub1/file3: unmerged (27d10cc8d0f10540c1fce1aa6de5e8f3e6b655ba)
fatal: git write-tree failed to write a tree
Your patch would remove the first 3 error messages, but leave the
deeper problem.
> diff --git a/merge-recursive.c b/merge-recursive.c
> index 4394c44..6a2b962 100644
> --- a/merge-recursive.c
> +++ b/merge-recursive.c
> @@ -198,13 +198,18 @@ static void output_commit_title(struct merge_options *o, struct commit *commit)
> }
>
> static int add_cacheinfo(unsigned int mode, const unsigned char *sha1,
> - const char *path, int stage, int refresh, int options)
> + const char *path, int stage, int refresh,
> + int options, int noent_okay)
> {
> struct cache_entry *ce;
> + int cache_errno = 0;
> ce = make_cache_entry(mode, sha1 ? sha1 : null_sha1, path,
> - stage, refresh, NULL);
> - if (!ce)
> + stage, refresh, &cache_errno);
> + if (!ce) {
> + if(cache_errno == ENOENT && noent_okay)
> + return 0;
> return error(_("addinfo_cache failed for path '%s'"), path);
> + }
> return add_cache_entry(ce, options);
> }
This is the crux of the change and the one you referred to in the
commit message. However, we don't really want add_cacheinfo to
tolerate failure to create a cache entry; we need one. We just want
add_cacheinfo to be tolerant of failure to refresh the stat-timestamp
for the new cache entry if there is no associated file on disk. Said
another way, we need a new cache entry back from make_cache_entry() in
all cases, it's just that we want the stat information refreshed if
and only if the file happens to exist in the working tree. (We could
just stat the file in the working tree, but that seems a waste since
make_cache_entry() will stat it again when it exists. In fact, the
stat in make_cache_entry() is also a bit of a waste because this is
the case when we know that before the merge started the file already
had the right contents and thus we ought to be able to get the right
timestamp for that particular file from the cache entry of the index
from before the merge even began. But I don't know how to access
that.)
Elijah
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html