[PATCH/RFC 0/3] merge-recursive: Avoid diagnostic on empty work tree

[PATCH/RFC 0/3] merge-recursive: Avoid diagnostic on empty work tree

[Brad King]

On 01/23/2014 07:24 PM, Elijah Newren wrote:
> Two options are just doing a stat to determine whether the file
> is present (which means we'll be stat'ing the file multiple times
> in these cases, which feels wasteful), or perhaps writing a
> modified make_cache_entry() with the behavior we want
> (seems like ugly code duplication).  Suggestions?

Perhaps we can thread enough information through the make_cache_entry
signature to allow the caller to know when lstat reported ENOENT.
Here is a series that takes such an approach.

* Patch 1 is the original test case from $gmane/240853.

* Patch 2 extends the make_cache_entry signature to return lstat errno.

* Patch 3 uses this information to silence the add_cacheinfo diagnostic

-Brad

Brad King (3):
  t3030-merge-recursive: Test known breakage with empty work tree
  read-cache.c: Thread lstat error through make_cache_entry signature
  merge-recursive: Tolerate missing file when HEAD is up to date

 builtin/apply.c            |  2 +-
 builtin/checkout.c         |  2 +-
 builtin/reset.c            |  2 +-
 cache.h                    |  2 +-
 merge-recursive.c          | 22 ++++++++++++++--------
 read-cache.c               | 12 +++++++-----
 resolve-undo.c             |  2 +-
 t/t3030-merge-recursive.sh | 47 ++++++++++++++++++++++++++++++++++++++++++++++
 8 files changed, 73 insertions(+), 18 deletions(-)

-- 
1.8.5.2

[PATCH/RFC 1/3] t3030-merge-recursive: Test known breakage with empty work tree

[Brad King]

Add test cases that use 'merge-recursive' plumbing with a temporary
index and empty work tree.  Populate the index using 'read-tree' and
'update-index --ignore-missing --refresh' to prepare for merge without
actually checking all files out to disk.  Verify that each merge
produces its expected tree while displaying no error diagnostics.

This approach can be used to compute tree merges while checking out only
conflicting files to disk (which is useful for server-side scripts).
Prior to commit 5b448b85 (merge-recursive: When we detect we can skip an
update, actually skip it, 2011-08-11) this worked cleanly in all cases.
Since that commit, merge-recursive displays a diagnostic such as

 error: addinfo_cache failed for path 'e'

when "our" side has a rename (to 'e').  The diagnostic does not
influence the return code and the merge appears to succeed, but it
causes this test case to fail.

Signed-off-by: Brad King <brad.king@kitware.com>
---
 t/t3030-merge-recursive.sh | 47 ++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 47 insertions(+)

diff --git a/t/t3030-merge-recursive.sh b/t/t3030-merge-recursive.sh
index 2f96100..b6d3ed0 100755
--- a/t/t3030-merge-recursive.sh
+++ b/t/t3030-merge-recursive.sh
@@ -257,6 +257,7 @@ test_expect_success 'setup 8' '
 	git add e &&
 	test_tick &&
 	git commit -m "rename a->e" &&
+	c7=$(git rev-parse --verify HEAD) &&
 	git checkout rename-ln &&
 	git mv a e &&
 	test_ln_s_add e a &&
@@ -517,6 +518,52 @@ test_expect_success 'reset and bind merge' '
 
 '
 
+test_expect_failure 'merge-recursive w/ empty work tree - ours has rename' '
+	(
+	 GIT_WORK_TREE="$PWD/ours-has-rename-work" &&
+	 export GIT_WORK_TREE &&
+	 GIT_INDEX_FILE="$PWD/ours-has-rename-index" &&
+	 export GIT_INDEX_FILE &&
+	 mkdir "$GIT_WORK_TREE" &&
+	 git read-tree -i -m $c7 &&
+	 git update-index --ignore-missing --refresh &&
+	 git merge-recursive $c0 -- $c7 $c3 &&
+	 git ls-files -s >actual-files
+	) 2>actual-err &&
+	>expected-err &&
+	cat >expected-files <<-EOF &&
+	100644 $o3 0	b/c
+	100644 $o0 0	c
+	100644 $o0 0	d/e
+	100644 $o0 0	e
+	EOF
+	test_cmp expected-files actual-files &&
+	test_cmp expected-err actual-err
+'
+
+test_expect_success 'merge-recursive w/ empty work tree - theirs has rename' '
+	(
+	 GIT_WORK_TREE="$PWD/theirs-has-rename-work" &&
+	 export GIT_WORK_TREE &&
+	 GIT_INDEX_FILE="$PWD/theirs-has-rename-index" &&
+	 export GIT_INDEX_FILE &&
+	 mkdir "$GIT_WORK_TREE" &&
+	 git read-tree -i -m $c3 &&
+	 git update-index --ignore-missing --refresh &&
+	 git merge-recursive $c0 -- $c3 $c7 &&
+	 git ls-files -s >actual-files
+	) 2>actual-err &&
+	>expected-err &&
+	cat >expected-files <<-EOF &&
+	100644 $o3 0	b/c
+	100644 $o0 0	c
+	100644 $o0 0	d/e
+	100644 $o0 0	e
+	EOF
+	test_cmp expected-files actual-files &&
+	test_cmp expected-err actual-err
+'
+
 test_expect_success 'merge removes empty directories' '
 
 	git reset --hard master &&
-- 
1.8.5.2

Re: [PATCH/RFC 1/3] t3030-merge-recursive: Test known breakage with empty work tree

[Jonathan Nieder]

Hi,

Brad King wrote:

> Add test cases that use 'merge-recursive' plumbing with a temporary
> index and empty work tree.  Populate the index using 'read-tree' and
> 'update-index --ignore-missing --refresh' to prepare for merge without
> actually checking all files out to disk.  Verify that each merge
> produces its expected tree while displaying no error diagnostics.

Following my usual review practice of lazy reading for the sake of
readers in the future who might be in a hurry, it's not clear what
problem these tests are solving or trying to detect.  Could you start
with a quick summary of the symptoms and when it came up?

The commit message doesn't need to paraphrase the actual code, since
anyone curious about the details can always look at the code.  It's
more important to explain the motivation and intended effect so people
can understand what went wrong if something ends up being broken by a
later patch.

> This approach can be used to compute tree merges while checking out only
> conflicting files to disk (which is useful for server-side scripts).
> Prior to commit 5b448b85 (merge-recursive: When we detect we can skip an
> update, actually skip it, 2011-08-11) this worked cleanly in all cases.

Do you mean something like the following?

	Sometimes when working with a large repository it can be useful to
	try out a merge and only check out conflicting files to disk (for
	example as a speed optimization on a server).  Until v1.7.7-rc1~28^2~20
	(merge-recursive: When we detect we can skip an update, actually
	skip it, 2011-08-11), it was possible to do so with the following
	idiom:

		... summary of commands here ...

	Nowadays, that still works and the exit status is the same,
	but merge-recursive produces a diagnostic if "our" side renamed
	a file:

		error: addinfo_cache failed for path 'dst'

	Add a test to document this regression.

[...]
> +++ b/t/t3030-merge-recursive.sh
[...]
> @@ -517,6 +518,52 @@ test_expect_success 'reset and bind merge' '
>  
>  '
>  
> +test_expect_failure 'merge-recursive w/ empty work tree - ours has rename' '
> +	(
> +	 GIT_WORK_TREE="$PWD/ours-has-rename-work" &&

Elsewhere in the test, commands in a subshell are indented by another
tab, so these new tests should probably follow suit.  As a side
effect, that makes the indentation easier to see.

Hope that helps,
Jonathan

Re: [PATCH/RFC 1/3] t3030-merge-recursive: Test known breakage with empty work tree

[Brad King]

On 01/24/2014 11:51 AM, Jonathan Nieder wrote:
> a quick summary of the symptoms and when it came up?

You're suggested commit message correctly explains it:

> Do you mean something like the following?
> 
> 	Sometimes when working with a large repository it can be useful to
> 	try out a merge and only check out conflicting files to disk (for
> 	example as a speed optimization on a server).  Until v1.7.7-rc1~28^2~20
> 	(merge-recursive: When we detect we can skip an update, actually
> 	skip it, 2011-08-11), it was possible to do so with the following
> 	idiom:
> 
> 		... summary of commands here ...
> 
> 	Nowadays, that still works and the exit status is the same,
> 	but merge-recursive produces a diagnostic if "our" side renamed
> 	a file:
> 
> 		error: addinfo_cache failed for path 'dst'
> 
> 	Add a test to document this regression.

Yes, thanks.

> Elsewhere in the test, commands in a subshell are indented by another
> tab, so these new tests should probably follow suit.

Great.  I'll fold both of the above into the next revision of the series.

Thanks,
-Brad

[PATCH/RFC 2/3] read-cache.c: Thread lstat error through make_cache_entry signature

[Brad King]

Add an 'int *err' argument to make_cache_entry to receive any error
that occurred when matching stat information for a file on disk.
Thread it through to the same argument of refresh_cache_ent.
This will allow callers of make_cache_entry to determine whether
failure was due to a missing file on disk.

Signed-off-by: Brad King <brad.king@kitware.com>
---
 builtin/apply.c    |  2 +-
 builtin/checkout.c |  2 +-
 builtin/reset.c    |  2 +-
 cache.h            |  2 +-
 merge-recursive.c  |  3 ++-
 read-cache.c       | 12 +++++++-----
 resolve-undo.c     |  2 +-
 7 files changed, 14 insertions(+), 11 deletions(-)

diff --git a/builtin/apply.c b/builtin/apply.c
index b0d0986..15e14ce 100644
--- a/builtin/apply.c
+++ b/builtin/apply.c
@@ -3675,7 +3675,7 @@ static void build_fake_ancestor(struct patch *list, const char *filename)
 			die("sha1 information is lacking or useless "
 			    "(%s).", name);
 
-		ce = make_cache_entry(patch->old_mode, sha1, name, 0, 0);
+		ce = make_cache_entry(patch->old_mode, sha1, name, 0, 0, NULL);
 		if (!ce)
 			die(_("make_cache_entry failed for path '%s'"), name);
 		if (add_index_entry(&result, ce, ADD_CACHE_OK_TO_ADD))
diff --git a/builtin/checkout.c b/builtin/checkout.c
index 5df3837..c7338bb 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -208,7 +208,7 @@ static int checkout_merged(int pos, struct checkout *state)
 	if (write_sha1_file(result_buf.ptr, result_buf.size,
 			    blob_type, sha1))
 		die(_("Unable to add merge result for '%s'"), path);
-	ce = make_cache_entry(mode, sha1, path, 2, 0);
+	ce = make_cache_entry(mode, sha1, path, 2, 0, NULL);
 	if (!ce)
 		die(_("make_cache_entry failed for path '%s'"), path);
 	status = checkout_entry(ce, state, NULL);
diff --git a/builtin/reset.c b/builtin/reset.c
index 6004803..8e0375d 100644
--- a/builtin/reset.c
+++ b/builtin/reset.c
@@ -122,7 +122,7 @@ static void update_index_from_diff(struct diff_queue_struct *q,
 		if (one->mode && !is_null_sha1(one->sha1)) {
 			struct cache_entry *ce;
 			ce = make_cache_entry(one->mode, one->sha1, one->path,
-				0, 0);
+					      0, 0, NULL);
 			if (!ce)
 				die(_("make_cache_entry failed for path '%s'"),
 				    one->path);
diff --git a/cache.h b/cache.h
index c9efe88..8e4f17d 100644
--- a/cache.h
+++ b/cache.h
@@ -487,7 +487,7 @@ extern int remove_file_from_index(struct index_state *, const char *path);
 #define ADD_CACHE_IMPLICIT_DOT 32	/* internal to "git add -u/-A" */
 extern int add_to_index(struct index_state *, const char *path, struct stat *, int flags);
 extern int add_file_to_index(struct index_state *, const char *path, int flags);
-extern struct cache_entry *make_cache_entry(unsigned int mode, const unsigned char *sha1, const char *path, int stage, int refresh);
+extern struct cache_entry *make_cache_entry(unsigned int mode, const unsigned char *sha1, const char *path, int stage, int refresh, int *err);
 extern int ce_same_name(const struct cache_entry *a, const struct cache_entry *b);
 extern int index_name_is_other(const struct index_state *, const char *, int);
 extern void *read_blob_data_from_index(struct index_state *, const char *, unsigned long *);
diff --git a/merge-recursive.c b/merge-recursive.c
index a18bd15..4394c44 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -201,7 +201,8 @@ static int 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(mode, sha1 ? sha1 : null_sha1, path, stage, refresh);
+	ce = make_cache_entry(mode, sha1 ? sha1 : null_sha1, path,
+			      stage, refresh, NULL);
 	if (!ce)
 		return error(_("addinfo_cache failed for path '%s'"), path);
 	return add_cache_entry(ce, options);
diff --git a/read-cache.c b/read-cache.c
index 33dd676..8f16cee 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -15,7 +15,8 @@
 #include "strbuf.h"
 #include "varint.h"
 
-static struct cache_entry *refresh_cache_entry(struct cache_entry *ce, int really);
+static struct cache_entry *refresh_cache_entry(struct cache_entry *ce,
+					       int really, int* err);
 
 /* Mask for the name length in ce_flags in the on-disk index */
 
@@ -696,7 +697,7 @@ int add_file_to_index(struct index_state *istate, const char *path, int flags)
 
 struct cache_entry *make_cache_entry(unsigned int mode,
 		const unsigned char *sha1, const char *path, int stage,
-		int refresh)
+		int refresh, int* err)
 {
 	int size, len;
 	struct cache_entry *ce;
@@ -717,7 +718,7 @@ struct cache_entry *make_cache_entry(unsigned int mode,
 	ce->ce_mode = create_ce_mode(mode);
 
 	if (refresh)
-		return refresh_cache_entry(ce, 0);
+		return refresh_cache_entry(ce, 0, err);
 
 	return ce;
 }
@@ -1207,9 +1208,10 @@ int refresh_index(struct index_state *istate, unsigned int flags,
 	return has_errors;
 }
 
-static struct cache_entry *refresh_cache_entry(struct cache_entry *ce, int really)
+static struct cache_entry *refresh_cache_entry(struct cache_entry *ce,
+					       int really, int* err)
 {
-	return refresh_cache_ent(&the_index, ce, really, NULL, NULL);
+	return refresh_cache_ent(&the_index, ce, really, err, NULL);
 }
 
 
diff --git a/resolve-undo.c b/resolve-undo.c
index c09b006..2b7a937 100644
--- a/resolve-undo.c
+++ b/resolve-undo.c
@@ -144,7 +144,7 @@ int unmerge_index_entry_at(struct index_state *istate, int pos)
 		if (!ru->mode[i])
 			continue;
 		nce = make_cache_entry(ru->mode[i], ru->sha1[i],
-				       ce->name, i + 1, 0);
+				       ce->name, i + 1, 0, NULL);
 		if (matched)
 			nce->ce_flags |= CE_MATCHED;
 		if (add_index_entry(istate, nce, ADD_CACHE_OK_TO_ADD)) {
-- 
1.8.5.2

[PATCH/RFC 3/3] merge-recursive: Tolerate missing file when HEAD is up to date

[Brad King]

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.

Signed-off-by: Brad King <brad.king@kitware.com>
---
 merge-recursive.c          | 21 +++++++++++++--------
 t/t3030-merge-recursive.sh |  2 +-
 2 files changed, 14 insertions(+), 9 deletions(-)

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);
 }
 
@@ -552,13 +557,13 @@ static int update_stages(const char *path, const struct diff_filespec *o,
 		if (remove_file_from_cache(path))
 			return -1;
 	if (o)
-		if (add_cacheinfo(o->mode, o->sha1, path, 1, 0, options))
+		if (add_cacheinfo(o->mode, o->sha1, path, 1, 0, options, 0))
 			return -1;
 	if (a)
-		if (add_cacheinfo(a->mode, a->sha1, path, 2, 0, options))
+		if (add_cacheinfo(a->mode, a->sha1, path, 2, 0, options, 0))
 			return -1;
 	if (b)
-		if (add_cacheinfo(b->mode, b->sha1, path, 3, 0, options))
+		if (add_cacheinfo(b->mode, b->sha1, path, 3, 0, options, 0))
 			return -1;
 	return 0;
 }
@@ -789,7 +794,7 @@ static void update_file_flags(struct merge_options *o,
 	}
  update_index:
 	if (update_cache)
-		add_cacheinfo(mode, sha, path, 0, update_wd, ADD_CACHE_OK_TO_ADD);
+		add_cacheinfo(mode, sha, path, 0, update_wd, ADD_CACHE_OK_TO_ADD, 0);
 }
 
 static void update_file(struct merge_options *o,
@@ -1624,7 +1629,7 @@ static int merge_content(struct merge_options *o,
 		path_renamed_outside_HEAD = !path2 || !strcmp(path, path2);
 		if (!path_renamed_outside_HEAD) {
 			add_cacheinfo(mfi.mode, mfi.sha, path,
-				      0, (!o->call_depth), 0);
+				      0, (!o->call_depth), 0, 1);
 			return mfi.clean;
 		}
 	} else
diff --git a/t/t3030-merge-recursive.sh b/t/t3030-merge-recursive.sh
index b6d3ed0..c8ba895 100755
--- a/t/t3030-merge-recursive.sh
+++ b/t/t3030-merge-recursive.sh
@@ -518,7 +518,7 @@ test_expect_success 'reset and bind merge' '
 
 '
 
-test_expect_failure 'merge-recursive w/ empty work tree - ours has rename' '
+test_expect_success 'merge-recursive w/ empty work tree - ours has rename' '
 	(
 	 GIT_WORK_TREE="$PWD/ours-has-rename-work" &&
 	 export GIT_WORK_TREE &&
-- 
1.8.5.2

Fwd: [PATCH/RFC 3/3] merge-recursive: Tolerate missing file when HEAD is up to date

[newren]

[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@kitware.com> 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

Re: [PATCH/RFC 3/3] merge-recursive: Tolerate missing file when HEAD is up to date

[Brad King]

On 01/24/2014 01:42 PM, Elijah Newren wrote:
> While this change does work for the particular new testcase you provided,
> there's a more complex case where merge-recursive is failing

I'm not surprised.  The change felt much like covering a symptom.

> it's just that we want the stat information refreshed if and only
> if the file happens to exist in the working tree.

We can add a refresh_flags argument to make_cache_entry to request
this behavior.  I'll send an updated series soon.

Thanks,
-Brad

Re: [PATCH/RFC 3/3] merge-recursive: Tolerate missing file when HEAD is up to date

[Junio C Hamano]

Brad King <brad.king@kitware.com> writes:

> 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.

It somehow feels wrong to force callers of make_cache_entry() to be
so intimate with the implementation details of refresh_cache_ent()
by having them inspect the errno from lstat(2) so deep in the
callchain, and to force callers of make_cache_entry() that says
refresh=NoThanks to pass a useless NULL.

Looking at refresh_cache_ent(), I notice that we already have cases
where we do not bother to lstat and instead say "Yeah, the cache
entry you have is good", and have to wonder if this new feature
should be modeled after them instead, namely, by introducing a new
option bit CE_MATCH_MISSING_OK that asks it to treat a path that is
missing from the working tree as if it is checked out unmodified.

Re: [PATCH/RFC 3/3] merge-recursive: Tolerate missing file when HEAD is up to date

[Brad King]

On Fri, Jan 24, 2014 at 2:50 PM, Junio C Hamano <gitster@pobox.com> wrote:
> It somehow feels wrong to force callers of make_cache_entry() to be
> so intimate with the implementation details of refresh_cache_ent()
[snip]
> option bit CE_MATCH_MISSING_OK that asks it to treat a path that is
> missing from the working tree as if it is checked out unmodified.

I came to the same conclusion after reading Elijah's last response.
My next series revision adds an argument to make_cache_entry to
specify the refresh flags and honors REFRESH_IGNORE_MISSING.

Thanks,
-Brad

[PATCH v2 0/3] merge-recursive: Avoid diagnostic on empty work tree

[Brad King]

Hi Folks,

Here is the second revision of this series.  The previous
revision can be found at $gmane/241009.

Updates since the previous revision of the series:

* Patch 1 test indentation and commit message updated thanks to
  comments from Jonathan.

* Patch 2 now adds a different new argument to make_cache_entry.
  This one is to request certain refresh behavior instead of just
  to get an error value back.

* Patch 3 uses the new make_cache_entry feature in patch 2
  to fix the test case.  This approach is based on suggestions
  from Elijah and Junio.

Thanks,
-Brad

Brad King (3):
  t3030-merge-recursive: Test known breakage with empty work tree
  read-cache.c: Optionally tolerate missing files in make_cache_entry
  merge-recursive.c: Tolerate missing files while refreshing index

 builtin/apply.c            |  2 +-
 builtin/checkout.c         |  2 +-
 builtin/reset.c            |  2 +-
 cache.h                    |  2 +-
 merge-recursive.c          |  3 ++-
 read-cache.c               | 21 ++++++++++++++++-----
 resolve-undo.c             |  2 +-
 t/t3030-merge-recursive.sh | 47 ++++++++++++++++++++++++++++++++++++++++++++++
 8 files changed, 70 insertions(+), 11 deletions(-)

-- 
1.8.5.2

[PATCH v2 1/3] t3030-merge-recursive: Test known breakage with empty work tree

[Brad King]

Sometimes when working with a large repository it can be useful to try
out a merge and only check out conflicting files to disk (for example as
a speed optimization on a server).  Until v1.7.7-rc1~28^2~20
(merge-recursive: When we detect we can skip an update, actually skip
it, 2011-08-11), it was possible to do so with the following idiom:

	# Prepare a temporary index and empty work tree.
	GIT_INDEX_FILE="$PWD/tmp-$$-index" &&
	export GIT_INDEX_FILE &&
	GIT_WORK_TREE="$PWD/tmp-$$-work" &&
	export GIT_WORK_TREE &&
	mkdir "$GIT_WORK_TREE" &&

	# Convince the index that our side is on disk.
	git read-tree -i -m $ours &&
	git update-index --ignore-missing --refresh &&

	# Merge their side into our side.
	bases=$(git merge-base --all $ours $theirs) &&
	git merge-recursive $bases -- $ours $theirs &&
	tree=$(git write-tree)

Nowadays, that still works and the exit status is the same, but
merge-recursive produces a diagnostic if "our" side renamed a file:

	error: addinfo_cache failed for path 'dst'

Add a test to document this regression.

Signed-off-by: Brad King <brad.king@kitware.com>
---
 t/t3030-merge-recursive.sh | 47 ++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 47 insertions(+)

diff --git a/t/t3030-merge-recursive.sh b/t/t3030-merge-recursive.sh
index 2f96100..3db3bf6 100755
--- a/t/t3030-merge-recursive.sh
+++ b/t/t3030-merge-recursive.sh
@@ -257,6 +257,7 @@ test_expect_success 'setup 8' '
 	git add e &&
 	test_tick &&
 	git commit -m "rename a->e" &&
+	c7=$(git rev-parse --verify HEAD) &&
 	git checkout rename-ln &&
 	git mv a e &&
 	test_ln_s_add e a &&
@@ -517,6 +518,52 @@ test_expect_success 'reset and bind merge' '
 
 '
 
+test_expect_failure 'merge-recursive w/ empty work tree - ours has rename' '
+	(
+		GIT_WORK_TREE="$PWD/ours-has-rename-work" &&
+		export GIT_WORK_TREE &&
+		GIT_INDEX_FILE="$PWD/ours-has-rename-index" &&
+		export GIT_INDEX_FILE &&
+		mkdir "$GIT_WORK_TREE" &&
+		git read-tree -i -m $c7 &&
+		git update-index --ignore-missing --refresh &&
+		git merge-recursive $c0 -- $c7 $c3 &&
+		git ls-files -s >actual-files
+	) 2>actual-err &&
+	>expected-err &&
+	cat >expected-files <<-EOF &&
+	100644 $o3 0	b/c
+	100644 $o0 0	c
+	100644 $o0 0	d/e
+	100644 $o0 0	e
+	EOF
+	test_cmp expected-files actual-files &&
+	test_cmp expected-err actual-err
+'
+
+test_expect_success 'merge-recursive w/ empty work tree - theirs has rename' '
+	(
+		GIT_WORK_TREE="$PWD/theirs-has-rename-work" &&
+		export GIT_WORK_TREE &&
+		GIT_INDEX_FILE="$PWD/theirs-has-rename-index" &&
+		export GIT_INDEX_FILE &&
+		mkdir "$GIT_WORK_TREE" &&
+		git read-tree -i -m $c3 &&
+		git update-index --ignore-missing --refresh &&
+		git merge-recursive $c0 -- $c3 $c7 &&
+		git ls-files -s >actual-files
+	) 2>actual-err &&
+	>expected-err &&
+	cat >expected-files <<-EOF &&
+	100644 $o3 0	b/c
+	100644 $o0 0	c
+	100644 $o0 0	d/e
+	100644 $o0 0	e
+	EOF
+	test_cmp expected-files actual-files &&
+	test_cmp expected-err actual-err
+'
+
 test_expect_success 'merge removes empty directories' '
 
 	git reset --hard master &&
-- 
1.8.5.2

[PATCH v2 2/3] read-cache.c: Optionally tolerate missing files in make_cache_entry

[Brad King]

Add an 'int refresh_flags' argument to make_cache_entry to tell the
refresh step about caller preferences.  Teach it to honor the
REFRESH_IGNORE_MISSING flag to skip refreshing stat information when a
file is missing from the work tree on disk.

Signed-off-by: Brad King <brad.king@kitware.com>
---
 builtin/apply.c    |  2 +-
 builtin/checkout.c |  2 +-
 builtin/reset.c    |  2 +-
 cache.h            |  2 +-
 merge-recursive.c  |  2 +-
 read-cache.c       | 21 ++++++++++++++++-----
 resolve-undo.c     |  2 +-
 7 files changed, 22 insertions(+), 11 deletions(-)

diff --git a/builtin/apply.c b/builtin/apply.c
index b0d0986..64c04ec 100644
--- a/builtin/apply.c
+++ b/builtin/apply.c
@@ -3675,7 +3675,7 @@ static void build_fake_ancestor(struct patch *list, const char *filename)
 			die("sha1 information is lacking or useless "
 			    "(%s).", name);
 
-		ce = make_cache_entry(patch->old_mode, sha1, name, 0, 0);
+		ce = make_cache_entry(patch->old_mode, sha1, name, 0, 0, 0);
 		if (!ce)
 			die(_("make_cache_entry failed for path '%s'"), name);
 		if (add_index_entry(&result, ce, ADD_CACHE_OK_TO_ADD))
diff --git a/builtin/checkout.c b/builtin/checkout.c
index 5df3837..d3d8640 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -208,7 +208,7 @@ static int checkout_merged(int pos, struct checkout *state)
 	if (write_sha1_file(result_buf.ptr, result_buf.size,
 			    blob_type, sha1))
 		die(_("Unable to add merge result for '%s'"), path);
-	ce = make_cache_entry(mode, sha1, path, 2, 0);
+	ce = make_cache_entry(mode, sha1, path, 2, 0, 0);
 	if (!ce)
 		die(_("make_cache_entry failed for path '%s'"), path);
 	status = checkout_entry(ce, state, NULL);
diff --git a/builtin/reset.c b/builtin/reset.c
index 6004803..ac45056 100644
--- a/builtin/reset.c
+++ b/builtin/reset.c
@@ -122,7 +122,7 @@ static void update_index_from_diff(struct diff_queue_struct *q,
 		if (one->mode && !is_null_sha1(one->sha1)) {
 			struct cache_entry *ce;
 			ce = make_cache_entry(one->mode, one->sha1, one->path,
-				0, 0);
+					      0, 0, 0);
 			if (!ce)
 				die(_("make_cache_entry failed for path '%s'"),
 				    one->path);
diff --git a/cache.h b/cache.h
index c9efe88..653ede4 100644
--- a/cache.h
+++ b/cache.h
@@ -487,7 +487,7 @@ extern int remove_file_from_index(struct index_state *, const char *path);
 #define ADD_CACHE_IMPLICIT_DOT 32	/* internal to "git add -u/-A" */
 extern int add_to_index(struct index_state *, const char *path, struct stat *, int flags);
 extern int add_file_to_index(struct index_state *, const char *path, int flags);
-extern struct cache_entry *make_cache_entry(unsigned int mode, const unsigned char *sha1, const char *path, int stage, int refresh);
+extern struct cache_entry *make_cache_entry(unsigned int mode, const unsigned char *sha1, const char *path, int stage, int refresh, int refresh_flags);
 extern int ce_same_name(const struct cache_entry *a, const struct cache_entry *b);
 extern int index_name_is_other(const struct index_state *, const char *, int);
 extern void *read_blob_data_from_index(struct index_state *, const char *, unsigned long *);
diff --git a/merge-recursive.c b/merge-recursive.c
index a18bd15..a6fe7f9 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -201,7 +201,7 @@ static int 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(mode, sha1 ? sha1 : null_sha1, path, stage, refresh);
+	ce = make_cache_entry(mode, sha1 ? sha1 : null_sha1, path, stage, refresh, 0);
 	if (!ce)
 		return error(_("addinfo_cache failed for path '%s'"), path);
 	return add_cache_entry(ce, options);
diff --git a/read-cache.c b/read-cache.c
index 33dd676..9ce7a9f 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -15,7 +15,8 @@
 #include "strbuf.h"
 #include "varint.h"
 
-static struct cache_entry *refresh_cache_entry(struct cache_entry *ce, int really);
+static struct cache_entry *refresh_cache_entry(struct cache_entry *ce, int really,
+					       int flags);
 
 /* Mask for the name length in ce_flags in the on-disk index */
 
@@ -696,7 +697,7 @@ int add_file_to_index(struct index_state *istate, const char *path, int flags)
 
 struct cache_entry *make_cache_entry(unsigned int mode,
 		const unsigned char *sha1, const char *path, int stage,
-		int refresh)
+		int refresh, int refresh_flags)
 {
 	int size, len;
 	struct cache_entry *ce;
@@ -717,7 +718,7 @@ struct cache_entry *make_cache_entry(unsigned int mode,
 	ce->ce_mode = create_ce_mode(mode);
 
 	if (refresh)
-		return refresh_cache_entry(ce, 0);
+		return refresh_cache_entry(ce, 0, refresh_flags);
 
 	return ce;
 }
@@ -1207,9 +1208,19 @@ int refresh_index(struct index_state *istate, unsigned int flags,
 	return has_errors;
 }
 
-static struct cache_entry *refresh_cache_entry(struct cache_entry *ce, int really)
+static struct cache_entry *refresh_cache_entry(struct cache_entry *ce, int really,
+					       int flags)
 {
-	return refresh_cache_ent(&the_index, ce, really, NULL, NULL);
+	int not_new = (flags & REFRESH_IGNORE_MISSING) != 0;
+	int cache_errno = 0;
+	struct cache_entry *new;
+
+	new = refresh_cache_ent(&the_index, ce, really, &cache_errno, NULL);
+
+	if(!new && not_new && cache_errno == ENOENT)
+		return ce;
+
+	return new;
 }
 
 
diff --git a/resolve-undo.c b/resolve-undo.c
index c09b006..d4faff0 100644
--- a/resolve-undo.c
+++ b/resolve-undo.c
@@ -144,7 +144,7 @@ int unmerge_index_entry_at(struct index_state *istate, int pos)
 		if (!ru->mode[i])
 			continue;
 		nce = make_cache_entry(ru->mode[i], ru->sha1[i],
-				       ce->name, i + 1, 0);
+				       ce->name, i + 1, 0, 0);
 		if (matched)
 			nce->ce_flags |= CE_MATCHED;
 		if (add_index_entry(istate, nce, ADD_CACHE_OK_TO_ADD)) {
-- 
1.8.5.2

Re: [PATCH v2 2/3] read-cache.c: Optionally tolerate missing files in make_cache_entry

[Junio C Hamano]

Brad King <brad.king@kitware.com> writes:

> +extern struct cache_entry *make_cache_entry(unsigned int mode, const unsigned char *sha1, const char *path, int stage, int refresh, int refresh_flags);

Why a new parameter?  If refresh_flags can be ANY when refresh=NoThanks,
shouldn't they be a single variable that tells the callee how the entry
should be refreshed (e.g. "not at all", "normally", "missing is ok", etc.)?

> +static struct cache_entry *refresh_cache_entry(struct cache_entry *ce, int really,
> +					       int flags)
>  {
> -	return refresh_cache_ent(&the_index, ce, really, NULL, NULL);
> +	int not_new = (flags & REFRESH_IGNORE_MISSING) != 0;
> +	int cache_errno = 0;
> +	struct cache_entry *new;
> +
> +	new = refresh_cache_ent(&the_index, ce, really, &cache_errno, NULL);
> +
> +	if(!new && not_new && cache_errno == ENOENT)
> +		return ce;

I think this is still one level too high in the abstraction chain.
"int really" might be of type signed int by historical accidents,
but it is "unsigned int options" for the underlying
refresh_cache_ent().  I'd suggest renaming this to "unsigned int
refresh_options" or something, and then define a new constatnt
similar to the existing CE_MATCH_IGNORE_*.

[PATCH v2 3/3] merge-recursive.c: Tolerate missing files while refreshing index

[Brad King]

Teach add_cacheinfo to tell make_cache_entry to skip refreshing stat
information when a file is missing from the work tree.  We do not want
the index to be stat-dirty after the merge but also do not want to fail
when a file happens to be missing.

This fixes the 'merge-recursive w/ empty work tree - ours has rename'
case in t3030-merge-recursive.

Suggested-by: Elijah Newren <newren@gmail.com>
Signed-off-by: Brad King <brad.king@kitware.com>
---
 merge-recursive.c          | 3 ++-
 t/t3030-merge-recursive.sh | 2 +-
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/merge-recursive.c b/merge-recursive.c
index a6fe7f9..35935c5 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -201,7 +201,8 @@ static int 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(mode, sha1 ? sha1 : null_sha1, path, stage, refresh, 0);
+	ce = make_cache_entry(mode, sha1 ? sha1 : null_sha1, path, stage,
+			      refresh, REFRESH_IGNORE_MISSING);
 	if (!ce)
 		return error(_("addinfo_cache failed for path '%s'"), path);
 	return add_cache_entry(ce, options);
diff --git a/t/t3030-merge-recursive.sh b/t/t3030-merge-recursive.sh
index 3db3bf6..82e1854 100755
--- a/t/t3030-merge-recursive.sh
+++ b/t/t3030-merge-recursive.sh
@@ -518,7 +518,7 @@ test_expect_success 'reset and bind merge' '
 
 '
 
-test_expect_failure 'merge-recursive w/ empty work tree - ours has rename' '
+test_expect_success 'merge-recursive w/ empty work tree - ours has rename' '
 	(
 		GIT_WORK_TREE="$PWD/ours-has-rename-work" &&
 		export GIT_WORK_TREE &&
-- 
1.8.5.2

[PATCH v3 0/3] merge-recursive: Avoid diagnostic on empty work tree

[Brad King]

Hi Folks,

Here is the third revision of this series.  The previous
revisions can be found at $gmane/241009 and $gmane/241030.

Updates since the previous revision of the series:

* Handling of lstat ENOENT has been moved down into refresh_cache_ent
  and activated by a new CE_MATCH_IGNORE_MISSING option.

* Rather than adding a new argument to make_cache_entry, the existing
  'refresh' boolean argument has been generalized to a set of options.
  This required the addition of a new CE_MATCH_REFRESH option to
  enable refresh with no other options.

Thanks,
-Brad

Brad King (4):
  t3030-merge-recursive: Test known breakage with empty work tree
  read-cache.c: Refactor --ignore-missing implementation
  read-cache.c: Extend make_cache_entry refresh flag with options
  merge-recursive.c: Tolerate missing files while refreshing index

 cache.h                    |  6 +++++-
 merge-recursive.c          |  4 +++-
 read-cache.c               | 27 ++++++++++++++------------
 t/t3030-merge-recursive.sh | 47 ++++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 70 insertions(+), 14 deletions(-)

-- 
1.8.5.2

[PATCH v3 1/4] t3030-merge-recursive: Test known breakage with empty work tree

[Brad King]

Sometimes when working with a large repository it can be useful to try
out a merge and only check out conflicting files to disk (for example as
a speed optimization on a server).  Until v1.7.7-rc1~28^2~20
(merge-recursive: When we detect we can skip an update, actually skip
it, 2011-08-11), it was possible to do so with the following idiom:

	# Prepare a temporary index and empty work tree.
	GIT_INDEX_FILE="$PWD/tmp-$$-index" &&
	export GIT_INDEX_FILE &&
	GIT_WORK_TREE="$PWD/tmp-$$-work" &&
	export GIT_WORK_TREE &&
	mkdir "$GIT_WORK_TREE" &&

	# Convince the index that our side is on disk.
	git read-tree -i -m $ours &&
	git update-index --ignore-missing --refresh &&

	# Merge their side into our side.
	bases=$(git merge-base --all $ours $theirs) &&
	git merge-recursive $bases -- $ours $theirs &&
	tree=$(git write-tree)

Nowadays, that still works and the exit status is the same, but
merge-recursive produces a diagnostic if "our" side renamed a file:

	error: addinfo_cache failed for path 'dst'

Add a test to document this regression.

Signed-off-by: Brad King <brad.king@kitware.com>
---
 t/t3030-merge-recursive.sh | 47 ++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 47 insertions(+)

diff --git a/t/t3030-merge-recursive.sh b/t/t3030-merge-recursive.sh
index 2f96100..3db3bf6 100755
--- a/t/t3030-merge-recursive.sh
+++ b/t/t3030-merge-recursive.sh
@@ -257,6 +257,7 @@ test_expect_success 'setup 8' '
 	git add e &&
 	test_tick &&
 	git commit -m "rename a->e" &&
+	c7=$(git rev-parse --verify HEAD) &&
 	git checkout rename-ln &&
 	git mv a e &&
 	test_ln_s_add e a &&
@@ -517,6 +518,52 @@ test_expect_success 'reset and bind merge' '
 
 '
 
+test_expect_failure 'merge-recursive w/ empty work tree - ours has rename' '
+	(
+		GIT_WORK_TREE="$PWD/ours-has-rename-work" &&
+		export GIT_WORK_TREE &&
+		GIT_INDEX_FILE="$PWD/ours-has-rename-index" &&
+		export GIT_INDEX_FILE &&
+		mkdir "$GIT_WORK_TREE" &&
+		git read-tree -i -m $c7 &&
+		git update-index --ignore-missing --refresh &&
+		git merge-recursive $c0 -- $c7 $c3 &&
+		git ls-files -s >actual-files
+	) 2>actual-err &&
+	>expected-err &&
+	cat >expected-files <<-EOF &&
+	100644 $o3 0	b/c
+	100644 $o0 0	c
+	100644 $o0 0	d/e
+	100644 $o0 0	e
+	EOF
+	test_cmp expected-files actual-files &&
+	test_cmp expected-err actual-err
+'
+
+test_expect_success 'merge-recursive w/ empty work tree - theirs has rename' '
+	(
+		GIT_WORK_TREE="$PWD/theirs-has-rename-work" &&
+		export GIT_WORK_TREE &&
+		GIT_INDEX_FILE="$PWD/theirs-has-rename-index" &&
+		export GIT_INDEX_FILE &&
+		mkdir "$GIT_WORK_TREE" &&
+		git read-tree -i -m $c3 &&
+		git update-index --ignore-missing --refresh &&
+		git merge-recursive $c0 -- $c3 $c7 &&
+		git ls-files -s >actual-files
+	) 2>actual-err &&
+	>expected-err &&
+	cat >expected-files <<-EOF &&
+	100644 $o3 0	b/c
+	100644 $o0 0	c
+	100644 $o0 0	d/e
+	100644 $o0 0	e
+	EOF
+	test_cmp expected-files actual-files &&
+	test_cmp expected-err actual-err
+'
+
 test_expect_success 'merge removes empty directories' '
 
 	git reset --hard master &&
-- 
1.8.5.2

[PATCH v3 2/4] read-cache.c: Refactor --ignore-missing implementation

[Brad King]

Move lstat ENOENT handling from refresh_index to refresh_cache_ent and
activate it with a new CE_MATCH_IGNORE_MISSING option.  This will allow
other call paths into refresh_cache_ent to use the feature.

Signed-off-by: Brad King <brad.king@kitware.com>
---
 cache.h      | 2 ++
 read-cache.c | 8 +++++---
 2 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/cache.h b/cache.h
index c9efe88..c96ada7 100644
--- a/cache.h
+++ b/cache.h
@@ -498,6 +498,8 @@ extern void *read_blob_data_from_index(struct index_state *, const char *, unsig
 #define CE_MATCH_RACY_IS_DIRTY		02
 /* do stat comparison even if CE_SKIP_WORKTREE is true */
 #define CE_MATCH_IGNORE_SKIP_WORKTREE	04
+/* ignore non-existent files during stat update  */
+#define CE_MATCH_IGNORE_MISSING		0x08
 extern int ie_match_stat(const struct index_state *, const struct cache_entry *, struct stat *, unsigned int);
 extern int ie_modified(const struct index_state *, const struct cache_entry *, struct stat *, unsigned int);
 
diff --git a/read-cache.c b/read-cache.c
index 33dd676..d61846c 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -1031,6 +1031,7 @@ static struct cache_entry *refresh_cache_ent(struct index_state *istate,
 	int changed, size;
 	int ignore_valid = options & CE_MATCH_IGNORE_VALID;
 	int ignore_skip_worktree = options & CE_MATCH_IGNORE_SKIP_WORKTREE;
+	int ignore_missing = options & CE_MATCH_IGNORE_MISSING;
 
 	if (ce_uptodate(ce))
 		return ce;
@@ -1050,6 +1051,8 @@ static struct cache_entry *refresh_cache_ent(struct index_state *istate,
 	}
 
 	if (lstat(ce->name, &st) < 0) {
+		if (ignore_missing && errno == ENOENT)
+			return ce;
 		if (err)
 			*err = errno;
 		return NULL;
@@ -1127,7 +1130,8 @@ int refresh_index(struct index_state *istate, unsigned int flags,
 	int ignore_submodules = (flags & REFRESH_IGNORE_SUBMODULES) != 0;
 	int first = 1;
 	int in_porcelain = (flags & REFRESH_IN_PORCELAIN);
-	unsigned int options = really ? CE_MATCH_IGNORE_VALID : 0;
+	unsigned int options = ((really ? CE_MATCH_IGNORE_VALID : 0) |
+				(not_new ? CE_MATCH_IGNORE_MISSING : 0));
 	const char *modified_fmt;
 	const char *deleted_fmt;
 	const char *typechange_fmt;
@@ -1176,8 +1180,6 @@ int refresh_index(struct index_state *istate, unsigned int flags,
 		if (!new) {
 			const char *fmt;
 
-			if (not_new && cache_errno == ENOENT)
-				continue;
 			if (really && cache_errno == EINVAL) {
 				/* If we are doing --really-refresh that
 				 * means the index is not valid anymore.
-- 
1.8.5.2

Re: [PATCH v3 2/4] read-cache.c: Refactor --ignore-missing implementation

[Junio C Hamano]

Brad King <brad.king@kitware.com> writes:

> Move lstat ENOENT handling from refresh_index to refresh_cache_ent and
> activate it with a new CE_MATCH_IGNORE_MISSING option.  This will allow
> other call paths into refresh_cache_ent to use the feature.
>
> Signed-off-by: Brad King <brad.king@kitware.com>
> ---

Good!

I forgot that we had "update-index --ignore-missing --refresh", and
that is conceptually the thing you want to use in your "perform
merge-recursive in an empty tree while pretending that the working
tree is fully populated and up-to-date" scenario.

>  cache.h      | 2 ++
>  read-cache.c | 8 +++++---
>  2 files changed, 7 insertions(+), 3 deletions(-)
>
> diff --git a/cache.h b/cache.h
> index c9efe88..c96ada7 100644
> --- a/cache.h
> +++ b/cache.h
> @@ -498,6 +498,8 @@ extern void *read_blob_data_from_index(struct index_state *, const char *, unsig
>  #define CE_MATCH_RACY_IS_DIRTY		02
>  /* do stat comparison even if CE_SKIP_WORKTREE is true */
>  #define CE_MATCH_IGNORE_SKIP_WORKTREE	04
> +/* ignore non-existent files during stat update  */
> +#define CE_MATCH_IGNORE_MISSING		0x08
>  extern int ie_match_stat(const struct index_state *, const struct cache_entry *, struct stat *, unsigned int);
>  extern int ie_modified(const struct index_state *, const struct cache_entry *, struct stat *, unsigned int);
>  
> diff --git a/read-cache.c b/read-cache.c
> index 33dd676..d61846c 100644
> --- a/read-cache.c
> +++ b/read-cache.c
> @@ -1031,6 +1031,7 @@ static struct cache_entry *refresh_cache_ent(struct index_state *istate,
>  	int changed, size;
>  	int ignore_valid = options & CE_MATCH_IGNORE_VALID;
>  	int ignore_skip_worktree = options & CE_MATCH_IGNORE_SKIP_WORKTREE;
> +	int ignore_missing = options & CE_MATCH_IGNORE_MISSING;
>  
>  	if (ce_uptodate(ce))
>  		return ce;
> @@ -1050,6 +1051,8 @@ static struct cache_entry *refresh_cache_ent(struct index_state *istate,
>  	}
>  
>  	if (lstat(ce->name, &st) < 0) {
> +		if (ignore_missing && errno == ENOENT)
> +			return ce;
>  		if (err)
>  			*err = errno;
>  		return NULL;
> @@ -1127,7 +1130,8 @@ int refresh_index(struct index_state *istate, unsigned int flags,
>  	int ignore_submodules = (flags & REFRESH_IGNORE_SUBMODULES) != 0;
>  	int first = 1;
>  	int in_porcelain = (flags & REFRESH_IN_PORCELAIN);
> -	unsigned int options = really ? CE_MATCH_IGNORE_VALID : 0;
> +	unsigned int options = ((really ? CE_MATCH_IGNORE_VALID : 0) |
> +				(not_new ? CE_MATCH_IGNORE_MISSING : 0));
>  	const char *modified_fmt;
>  	const char *deleted_fmt;
>  	const char *typechange_fmt;
> @@ -1176,8 +1180,6 @@ int refresh_index(struct index_state *istate, unsigned int flags,
>  		if (!new) {
>  			const char *fmt;
>  
> -			if (not_new && cache_errno == ENOENT)
> -				continue;
>  			if (really && cache_errno == EINVAL) {
>  				/* If we are doing --really-refresh that
>  				 * means the index is not valid anymore.

[PATCH v3 3/4] read-cache.c: Extend make_cache_entry refresh flag with options

[Brad King]

Convert the make_cache_entry boolean 'refresh' argument to a more
general 'refresh_options' argument.  Pass the value through to the
underlying refresh_cache_ent call.  Add option CE_MATCH_REFRESH to
enable stat refresh.  Update call sites to use the new signature.

Signed-off-by: Brad King <brad.king@kitware.com>
---
 cache.h           |  4 +++-
 merge-recursive.c |  3 ++-
 read-cache.c      | 21 +++++++++++----------
 3 files changed, 16 insertions(+), 12 deletions(-)

diff --git a/cache.h b/cache.h
index c96ada7..e8820e1 100644
--- a/cache.h
+++ b/cache.h
@@ -487,7 +487,7 @@ extern int remove_file_from_index(struct index_state *, const char *path);
 #define ADD_CACHE_IMPLICIT_DOT 32	/* internal to "git add -u/-A" */
 extern int add_to_index(struct index_state *, const char *path, struct stat *, int flags);
 extern int add_file_to_index(struct index_state *, const char *path, int flags);
-extern struct cache_entry *make_cache_entry(unsigned int mode, const unsigned char *sha1, const char *path, int stage, int refresh);
+extern struct cache_entry *make_cache_entry(unsigned int mode, const unsigned char *sha1, const char *path, int stage, unsigned int refresh_options);
 extern int ce_same_name(const struct cache_entry *a, const struct cache_entry *b);
 extern int index_name_is_other(const struct index_state *, const char *, int);
 extern void *read_blob_data_from_index(struct index_state *, const char *, unsigned long *);
@@ -500,6 +500,8 @@ extern void *read_blob_data_from_index(struct index_state *, const char *, unsig
 #define CE_MATCH_IGNORE_SKIP_WORKTREE	04
 /* ignore non-existent files during stat update  */
 #define CE_MATCH_IGNORE_MISSING		0x08
+/* enable stat refresh */
+#define CE_MATCH_REFRESH		0x10
 extern int ie_match_stat(const struct index_state *, const struct cache_entry *, struct stat *, unsigned int);
 extern int ie_modified(const struct index_state *, const struct cache_entry *, struct stat *, unsigned int);
 
diff --git a/merge-recursive.c b/merge-recursive.c
index a18bd15..c3753c8 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -201,7 +201,8 @@ static int 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(mode, sha1 ? sha1 : null_sha1, path, stage, refresh);
+	ce = make_cache_entry(mode, sha1 ? sha1 : null_sha1, path, stage,
+			      (refresh ? CE_MATCH_REFRESH : 0 ));
 	if (!ce)
 		return error(_("addinfo_cache failed for path '%s'"), path);
 	return add_cache_entry(ce, options);
diff --git a/read-cache.c b/read-cache.c
index d61846c..db3902e 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -15,7 +15,8 @@
 #include "strbuf.h"
 #include "varint.h"
 
-static struct cache_entry *refresh_cache_entry(struct cache_entry *ce, int really);
+static struct cache_entry *refresh_cache_entry(struct cache_entry *ce,
+					       unsigned int options);
 
 /* Mask for the name length in ce_flags in the on-disk index */
 
@@ -696,7 +697,7 @@ int add_file_to_index(struct index_state *istate, const char *path, int flags)
 
 struct cache_entry *make_cache_entry(unsigned int mode,
 		const unsigned char *sha1, const char *path, int stage,
-		int refresh)
+		unsigned int refresh_options)
 {
 	int size, len;
 	struct cache_entry *ce;
@@ -716,10 +717,7 @@ struct cache_entry *make_cache_entry(unsigned int mode,
 	ce->ce_namelen = len;
 	ce->ce_mode = create_ce_mode(mode);
 
-	if (refresh)
-		return refresh_cache_entry(ce, 0);
-
-	return ce;
+	return refresh_cache_entry(ce, refresh_options);
 }
 
 int ce_same_name(const struct cache_entry *a, const struct cache_entry *b)
@@ -1029,11 +1027,12 @@ static struct cache_entry *refresh_cache_ent(struct index_state *istate,
 	struct stat st;
 	struct cache_entry *updated;
 	int changed, size;
+	int refresh = options & CE_MATCH_REFRESH;
 	int ignore_valid = options & CE_MATCH_IGNORE_VALID;
 	int ignore_skip_worktree = options & CE_MATCH_IGNORE_SKIP_WORKTREE;
 	int ignore_missing = options & CE_MATCH_IGNORE_MISSING;
 
-	if (ce_uptodate(ce))
+	if (!refresh || ce_uptodate(ce))
 		return ce;
 
 	/*
@@ -1130,7 +1129,8 @@ int refresh_index(struct index_state *istate, unsigned int flags,
 	int ignore_submodules = (flags & REFRESH_IGNORE_SUBMODULES) != 0;
 	int first = 1;
 	int in_porcelain = (flags & REFRESH_IN_PORCELAIN);
-	unsigned int options = ((really ? CE_MATCH_IGNORE_VALID : 0) |
+	unsigned int options = (CE_MATCH_REFRESH |
+				(really ? CE_MATCH_IGNORE_VALID : 0) |
 				(not_new ? CE_MATCH_IGNORE_MISSING : 0));
 	const char *modified_fmt;
 	const char *deleted_fmt;
@@ -1209,9 +1209,10 @@ int refresh_index(struct index_state *istate, unsigned int flags,
 	return has_errors;
 }
 
-static struct cache_entry *refresh_cache_entry(struct cache_entry *ce, int really)
+static struct cache_entry *refresh_cache_entry(struct cache_entry *ce,
+					       unsigned int options)
 {
-	return refresh_cache_ent(&the_index, ce, really, NULL, NULL);
+	return refresh_cache_ent(&the_index, ce, options, NULL, NULL);
 }
 
 
-- 
1.8.5.2

[PATCH v3 4/4] merge-recursive.c: Tolerate missing files while refreshing index

[Brad King]

Teach add_cacheinfo to tell make_cache_entry to skip refreshing stat
information when a file is missing from the work tree.  We do not want
the index to be stat-dirty after the merge but also do not want to fail
when a file happens to be missing.

This fixes the 'merge-recursive w/ empty work tree - ours has rename'
case in t3030-merge-recursive.

Suggested-by: Elijah Newren <newren@gmail.com>
Signed-off-by: Brad King <brad.king@kitware.com>
---
 merge-recursive.c          | 3 ++-
 t/t3030-merge-recursive.sh | 2 +-
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/merge-recursive.c b/merge-recursive.c
index c3753c8..b8ea172 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -202,7 +202,8 @@ static int add_cacheinfo(unsigned int mode, const unsigned char *sha1,
 {
 	struct cache_entry *ce;
 	ce = make_cache_entry(mode, sha1 ? sha1 : null_sha1, path, stage,
-			      (refresh ? CE_MATCH_REFRESH : 0 ));
+			      (refresh ? (CE_MATCH_REFRESH |
+					  CE_MATCH_IGNORE_MISSING) : 0 ));
 	if (!ce)
 		return error(_("addinfo_cache failed for path '%s'"), path);
 	return add_cache_entry(ce, options);
diff --git a/t/t3030-merge-recursive.sh b/t/t3030-merge-recursive.sh
index 3db3bf6..82e1854 100755
--- a/t/t3030-merge-recursive.sh
+++ b/t/t3030-merge-recursive.sh
@@ -518,7 +518,7 @@ test_expect_success 'reset and bind merge' '
 
 '
 
-test_expect_failure 'merge-recursive w/ empty work tree - ours has rename' '
+test_expect_success 'merge-recursive w/ empty work tree - ours has rename' '
 	(
 		GIT_WORK_TREE="$PWD/ours-has-rename-work" &&
 		export GIT_WORK_TREE &&
-- 
1.8.5.2