[PATCH 2/9] Add flag to make unpack_trees() not print errors.

[PATCH 2/9] Add flag to make unpack_trees() not print errors.

[Daniel Barkalow]

This will allow builtin-checkout to suppress merge errors if it's
going to try more merging methods.

Additionally, if unpack_trees() returns with an error, but without
printing anything, it will roll back any changes to the index (by
rereading the index, currently). This obviously could be done by the
caller, but chances are that the caller would forget and debugging
this is difficult.

Signed-off-by: Daniel Barkalow <barkalow@iabervon.org>
---
 unpack-trees.c |   43 +++++++++++++++++++++++++++++--------------
 unpack-trees.h |    1 +
 2 files changed, 30 insertions(+), 14 deletions(-)

diff --git a/unpack-trees.c b/unpack-trees.c
index 9e6587f..45f40c2 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -356,12 +356,23 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options
 			posns[i] = create_tree_entry_list(t+i);
 
 		if (unpack_trees_rec(posns, len, o->prefix ? o->prefix : "",
-				     o, &df_conflict_list))
+				     o, &df_conflict_list)) {
+			if (o->gently) {
+				discard_cache();
+				read_cache();
+			}
 			return -1;
+		}
 	}
 
-	if (o->trivial_merges_only && o->nontrivial_merge)
-		return error("Merge requires file-level merging");
+	if (o->trivial_merges_only && o->nontrivial_merge) {
+		if (o->gently) {
+			discard_cache();
+			read_cache();
+		}
+		return o->gently ? -1 :
+			error("Merge requires file-level merging");
+	}
 
 	check_updates(active_cache, active_nr, o);
 	return 0;
@@ -415,7 +426,8 @@ static int verify_uptodate(struct cache_entry *ce,
 	}
 	if (errno == ENOENT)
 		return 0;
-	return error("Entry '%s' not uptodate. Cannot merge.", ce->name);
+	return o->gently ? -1 :
+		error("Entry '%s' not uptodate. Cannot merge.", ce->name);
 }
 
 static void invalidate_ce_path(struct cache_entry *ce)
@@ -501,8 +513,9 @@ static int verify_clean_subdirectory(struct cache_entry *ce, const char *action,
 		d.exclude_per_dir = o->dir->exclude_per_dir;
 	i = read_directory(&d, ce->name, pathbuf, namelen+1, NULL);
 	if (i)
-		return error("Updating '%s' would lose untracked files in it",
-			     ce->name);
+		return o->gently ? -1 :
+			error("Updating '%s' would lose untracked files in it",
+			      ce->name);
 	free(pathbuf);
 	return cnt;
 }
@@ -574,8 +587,9 @@ static int verify_absent(struct cache_entry *ce, const char *action,
 				return 0;
 		}
 
-		return error("Untracked working tree file '%s' "
-			     "would be %s by merge.", ce->name, action);
+		return o->gently ? -1 :
+			error("Untracked working tree file '%s' "
+			      "would be %s by merge.", ce->name, action);
 	}
 	return 0;
 }
@@ -707,7 +721,7 @@ int threeway_merge(struct cache_entry **stages,
 	/* #14, #14ALT, #2ALT */
 	if (remote && !df_conflict_head && head_match && !remote_match) {
 		if (index && !same(index, remote) && !same(index, head))
-			return reject_merge(index);
+			return o->gently ? -1 : reject_merge(index);
 		return merged_entry(remote, index, o);
 	}
 	/*
@@ -715,7 +729,7 @@ int threeway_merge(struct cache_entry **stages,
 	 * make sure that it matches head.
 	 */
 	if (index && !same(index, head)) {
-		return reject_merge(index);
+		return o->gently ? -1 : reject_merge(index);
 	}
 
 	if (head) {
@@ -866,11 +880,11 @@ int twoway_merge(struct cache_entry **src,
 			/* all other failures */
 			remove_entry(remove);
 			if (oldtree)
-				return reject_merge(oldtree);
+				return o->gently ? -1 : reject_merge(oldtree);
 			if (current)
-				return reject_merge(current);
+				return o->gently ? -1 : reject_merge(current);
 			if (newtree)
-				return reject_merge(newtree);
+				return o->gently ? -1 : reject_merge(newtree);
 			return -1;
 		}
 	}
@@ -897,7 +911,8 @@ int bind_merge(struct cache_entry **src,
 		return error("Cannot do a bind merge of %d trees\n",
 			     o->merge_size);
 	if (a && old)
-		return error("Entry '%s' overlaps.  Cannot bind.", a->name);
+		return o->gently ? -1 :
+			error("Entry '%s' overlaps.  Cannot bind.", a->name);
 	if (!a)
 		return keep_entry(old, o);
 	else
diff --git a/unpack-trees.h b/unpack-trees.h
index 197a004..83d1229 100644
--- a/unpack-trees.h
+++ b/unpack-trees.h
@@ -16,6 +16,7 @@ struct unpack_trees_options {
 	int trivial_merges_only;
 	int verbose_update;
 	int aggressive;
+	int gently;
 	const char *prefix;
 	int pos;
 	struct dir_struct *dir;
-- 
1.5.4

Re: [PATCH 2/9] Add flag to make unpack_trees() not print errors.

[Johannes Schindelin]

Hi,

On Mon, 4 Feb 2008, Daniel Barkalow wrote:

> This will allow builtin-checkout to suppress merge errors if it's going 
> to try more merging methods.

I saw one error() in twoway_merge(), one in bind_merge(), and one in 
onway_merge() that were not guarded by o->gently.

Also, I'd call it "silent", not "gently".

> Additionally, if unpack_trees() returns with an error, but without 
> printing anything, it will roll back any changes to the index (by 
> rereading the index, currently). This obviously could be done by the 
> caller, but chances are that the caller would forget and debugging this 
> is difficult.

Granted, it is easy to forget.  But maybe the caller does not need the 
index?  Or maybe it wants a different one?  I'd prefer the caller to clean 
up, if necessary.

Ciao,
Dscho

Re: [PATCH 2/9] Add flag to make unpack_trees() not print errors.

[Daniel Barkalow]

On Tue, 5 Feb 2008, Johannes Schindelin wrote:

> Hi,
> 
> On Mon, 4 Feb 2008, Daniel Barkalow wrote:
> 
> > This will allow builtin-checkout to suppress merge errors if it's going 
> > to try more merging methods.
> 
> I saw one error() in twoway_merge(), one in bind_merge(), and one in 
> onway_merge() that were not guarded by o->gently.
> 
> Also, I'd call it "silent", not "gently".
> 
> > Additionally, if unpack_trees() returns with an error, but without 
> > printing anything, it will roll back any changes to the index (by 
> > rereading the index, currently). This obviously could be done by the 
> > caller, but chances are that the caller would forget and debugging this 
> > is difficult.
> 
> Granted, it is easy to forget.  But maybe the caller does not need the 
> index?  Or maybe it wants a different one?  I'd prefer the caller to clean 
> up, if necessary.

That's what makes it "gently" instead of just "silent"; it has no effect 
if it doesn't succeed. Longer term, I'd like to have unpack_trees() unpack 
into a separate index, which should actually be faster (since it doesn't 
have to keep shifting the entries in the index it's working on) and make 
this moot. In any case, it only rolls it back with the option that's only 
used by a caller that wants the index unchanged on failure, currently. If 
a caller turns out to just want a return code and not care about the index 
or the error message, and the code hasn't been reworked, we can add a 
separate flag then.

I'd done some analysis at the time that suggested that, if you didn't want 
to give a message on failure, you must want to do something else to the 
index to replace what hadn't worked, so you must want the index reset, but 
I've forgotten why I was so sure at the time, aside from that nobody's 
wanted it before now.

	-Daniel
*This .sig left intentionally blank*

Re: [PATCH 2/9] Add flag to make unpack_trees() not print errors.

[Junio C Hamano]

Daniel Barkalow <barkalow@iabervon.org> writes:

> On Tue, 5 Feb 2008, Johannes Schindelin wrote:
> ...
>> > Additionally, if unpack_trees() returns with an error, but without 
>> > printing anything, it will roll back any changes to the index (by 
>> > rereading the index, currently). This obviously could be done by the 
>> > caller, but chances are that the caller would forget and debugging this 
>> > is difficult.
>> 
>> Granted, it is easy to forget.  But maybe the caller does not need the 
>> index?  Or maybe it wants a different one?  I'd prefer the caller to clean 
>> up, if necessary.
>
> That's what makes it "gently" instead of just "silent"; it has no effect 
> if it doesn't succeed. Longer term, I'd like to have unpack_trees() unpack 
> into a separate index, which should actually be faster (since it doesn't 
> have to keep shifting the entries in the index it's working on) and make 
> this moot.

Absolutely.  That is the original motivation I did the_index
thing for.

But "re-reading" may not be quite nice.  It would defeat the
optimization introduced by the change to use CE_UPTODATE flag to
avoid unnecessary lstat(2) calls.

Re: [PATCH 2/9] Add flag to make unpack_trees() not print errors.

[Daniel Barkalow]

On Tue, 5 Feb 2008, Junio C Hamano wrote:

> Daniel Barkalow <barkalow@iabervon.org> writes:
> 
> > On Tue, 5 Feb 2008, Johannes Schindelin wrote:
> > ...
> >> > Additionally, if unpack_trees() returns with an error, but without 
> >> > printing anything, it will roll back any changes to the index (by 
> >> > rereading the index, currently). This obviously could be done by the 
> >> > caller, but chances are that the caller would forget and debugging this 
> >> > is difficult.
> >> 
> >> Granted, it is easy to forget.  But maybe the caller does not need the 
> >> index?  Or maybe it wants a different one?  I'd prefer the caller to clean 
> >> up, if necessary.
> >
> > That's what makes it "gently" instead of just "silent"; it has no effect 
> > if it doesn't succeed. Longer term, I'd like to have unpack_trees() unpack 
> > into a separate index, which should actually be faster (since it doesn't 
> > have to keep shifting the entries in the index it's working on) and make 
> > this moot.
> 
> Absolutely.  That is the original motivation I did the_index
> thing for.
> 
> But "re-reading" may not be quite nice.  It would defeat the
> optimization introduced by the change to use CE_UPTODATE flag to
> avoid unnecessary lstat(2) calls.

For now, I want to re-read the index, because I need the changes undone 
and I don't have any way of reverting them. That's another reason to have 
unpack_trees return with the index unchanged: it may have a better way of 
getting there than the caller does, such as having never changed that 
memory in the first place. I'll add doing it efficiently to my list of 
things to do, but I want to keep it from being necessary for correctness 
of this series.

	-Daniel
*This .sig left intentionally blank*

[PATCH 2/9] Add flag to make unpack_trees() not print errors.

[Daniel Barkalow]

This will allow builtin-checkout to suppress merge errors if it's
going to try more merging methods.

Additionally, if unpack_trees() returns with an error, but without
printing anything, it will roll back any changes to the index (by
rereading the index, currently). This obviously could be done by the
caller, but chances are that the caller would forget and debugging
this is difficult.

Signed-off-by: Daniel Barkalow <barkalow@iabervon.org>
---
 unpack-trees.c |   43 +++++++++++++++++++++++++++++--------------
 unpack-trees.h |    1 +
 2 files changed, 30 insertions(+), 14 deletions(-)

diff --git a/unpack-trees.c b/unpack-trees.c
index a56e1c9..fc0d2c7 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -357,12 +357,23 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options
 			posns[i] = create_tree_entry_list(t+i);
 
 		if (unpack_trees_rec(posns, len, o->prefix ? o->prefix : "",
-				     o, &df_conflict_list))
+				     o, &df_conflict_list)) {
+			if (o->gently) {
+				discard_cache();
+				read_cache();
+			}
 			return -1;
+		}
 	}
 
-	if (o->trivial_merges_only && o->nontrivial_merge)
-		return error("Merge requires file-level merging");
+	if (o->trivial_merges_only && o->nontrivial_merge) {
+		if (o->gently) {
+			discard_cache();
+			read_cache();
+		}
+		return o->gently ? -1 :
+			error("Merge requires file-level merging");
+	}
 
 	check_updates(active_cache, active_nr, o);
 	return 0;
@@ -416,7 +427,8 @@ static int verify_uptodate(struct cache_entry *ce,
 	}
 	if (errno == ENOENT)
 		return 0;
-	return error("Entry '%s' not uptodate. Cannot merge.", ce->name);
+	return o->gently ? -1 :
+		error("Entry '%s' not uptodate. Cannot merge.", ce->name);
 }
 
 static void invalidate_ce_path(struct cache_entry *ce)
@@ -502,8 +514,9 @@ static int verify_clean_subdirectory(struct cache_entry *ce, const char *action,
 		d.exclude_per_dir = o->dir->exclude_per_dir;
 	i = read_directory(&d, ce->name, pathbuf, namelen+1, NULL);
 	if (i)
-		return error("Updating '%s' would lose untracked files in it",
-			     ce->name);
+		return o->gently ? -1 :
+			error("Updating '%s' would lose untracked files in it",
+			      ce->name);
 	free(pathbuf);
 	return cnt;
 }
@@ -575,8 +588,9 @@ static int verify_absent(struct cache_entry *ce, const char *action,
 				return 0;
 		}
 
-		return error("Untracked working tree file '%s' "
-			     "would be %s by merge.", ce->name, action);
+		return o->gently ? -1 :
+			error("Untracked working tree file '%s' "
+			      "would be %s by merge.", ce->name, action);
 	}
 	return 0;
 }
@@ -708,7 +722,7 @@ int threeway_merge(struct cache_entry **stages,
 	/* #14, #14ALT, #2ALT */
 	if (remote && !df_conflict_head && head_match && !remote_match) {
 		if (index && !same(index, remote) && !same(index, head))
-			return reject_merge(index);
+			return o->gently ? -1 : reject_merge(index);
 		return merged_entry(remote, index, o);
 	}
 	/*
@@ -716,7 +730,7 @@ int threeway_merge(struct cache_entry **stages,
 	 * make sure that it matches head.
 	 */
 	if (index && !same(index, head)) {
-		return reject_merge(index);
+		return o->gently ? -1 : reject_merge(index);
 	}
 
 	if (head) {
@@ -867,11 +881,11 @@ int twoway_merge(struct cache_entry **src,
 			/* all other failures */
 			remove_entry(remove);
 			if (oldtree)
-				return reject_merge(oldtree);
+				return o->gently ? -1 : reject_merge(oldtree);
 			if (current)
-				return reject_merge(current);
+				return o->gently ? -1 : reject_merge(current);
 			if (newtree)
-				return reject_merge(newtree);
+				return o->gently ? -1 : reject_merge(newtree);
 			return -1;
 		}
 	}
@@ -898,7 +912,8 @@ int bind_merge(struct cache_entry **src,
 		return error("Cannot do a bind merge of %d trees\n",
 			     o->merge_size);
 	if (a && old)
-		return error("Entry '%s' overlaps.  Cannot bind.", a->name);
+		return o->gently ? -1 :
+			error("Entry '%s' overlaps.  Cannot bind.", a->name);
 	if (!a)
 		return keep_entry(old, o);
 	else
diff --git a/unpack-trees.h b/unpack-trees.h
index 5517faa..619950d 100644
--- a/unpack-trees.h
+++ b/unpack-trees.h
@@ -16,6 +16,7 @@ struct unpack_trees_options {
 	int trivial_merges_only;
 	int verbose_update;
 	int aggressive;
+	int gently;
 	const char *prefix;
 	int pos;
 	struct dir_struct *dir;
-- 
1.5.4.rc3.4.g16335