threads / patch / 11875

patch, 9 partsAdd flag to make unpack_trees() not print errors.

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

## tl;dr

5 messages between Feb 4, 2008 and Feb 6, 2008. Diffs are folded; open one to read it.

replies: 4people: 3as markdown or json

Daniel Barkalow· Feb 4, 2008, 18:35 UTC · lore

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(-)
Show changes to 2 files +30 −14

unpack-trees.c, unpack-trees.h

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
Johannes Schindelin· Feb 5, 2008, 01:16 UTC · re: Daniel Barkalow · lore

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

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".
Show 5 quoted lines
> 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

Daniel Barkalow· Feb 5, 2008, 20:38 UTC · re: Johannes Schindelin · lore

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

On Tue, 5 Feb 2008, Johannes Schindelin wrote:
Show 21 quoted lines
> 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*
Junio C Hamano· Feb 5, 2008, 23:43 UTC · re: Daniel Barkalow · lore

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

Daniel Barkalow <barkalow@iabervon.org> writes:
Show 17 quoted lines
> 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.

Daniel Barkalow· Feb 6, 2008, 01:11 UTC · re: Junio C Hamano · lore

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

On Tue, 5 Feb 2008, Junio C Hamano wrote:
Show 26 quoted lines
> 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*

← back to recent threads