git-write-error with cherry-pick -n usages

git-write-error with cherry-pick -n usages

[Björn Steinbrink]

[-- Attachment #1: Type: text/plain, Size: 506 bytes --]

Hi,

when you cherry-pick -n a commit that has changes to a file missing in
the current tree, that file will be added as "Unmerged". A subsequent
cherry-pick that tries to pick a commit that has changes to that file
will then fail with:

fatal: git-write-tree: error building trees

I've attached a small shell script to easily reproduce that.

git version 1.5.4.rc2.84.gf85fd

That specific use of cherry-pick might be an user error, but, if
possible, git should give a less cryptic error message.

Björn

[-- Attachment #2: test.sh --]
[-- Type: application/x-sh, Size: 317 bytes --]

Re: git-write-error with cherry-pick -n usages

[Junio C Hamano]

Björn Steinbrink <B.Steinbrink@gmx.de> writes:

> when you cherry-pick -n a commit that has changes to a file missing in
> the current tree, that file will be added as "Unmerged". A subsequent
> cherry-pick that tries to pick a commit that has changes to that file
> will then fail with:
>
> fatal: git-write-tree: error building trees

Yeah, this was because the original "rewrite in C" did somewhat
a sloppy job while stealing code from git-write-tree.

The caller pretends as if the write_tree() function would return
an error code and being able to issue a sensible error message
itself, but write_tree() function just calls die() and never
returns an error.  Worse yet, the function claims that it was
running git-write-tree (which is no longer true after
cherry-pick stole it).

I think you would need to do something like this patch.  I will
not apply it during the -rc period, but testing and resending
with "Tested-by:" would be helpful after post 1.5.4 cycle opens.

---
 builtin-revert.c     |    5 ++-
 builtin-write-tree.c |   74 +++++++++++---------------------------------------
 builtin.h            |    1 -
 cache-tree.c         |   59 +++++++++++++++++++++++++++++++++++++++
 cache-tree.h         |    5 +++
 5 files changed, 83 insertions(+), 61 deletions(-)

diff --git a/builtin-revert.c b/builtin-revert.c
index 4bf8eb2..d71f414 100644
--- a/builtin-revert.c
+++ b/builtin-revert.c
@@ -8,6 +8,7 @@
 #include "exec_cmd.h"
 #include "utf8.h"
 #include "parse-options.h"
+#include "cache-tree.h"
 
 /*
  * This implements the builtins revert and cherry-pick.
@@ -270,7 +271,7 @@ static int revert_or_cherry_pick(int argc, const char **argv)
 		 * that represents the "current" state for merge-recursive
 		 * to work on.
 		 */
-		if (write_tree(head, 0, NULL))
+		if (write_cache_as_tree(head, 0, NULL))
 			die ("Your index file is unmerged.");
 	} else {
 		struct wt_status s;
@@ -357,7 +358,7 @@ static int revert_or_cherry_pick(int argc, const char **argv)
 	if (merge_recursive(sha1_to_hex(base->object.sha1),
 				sha1_to_hex(head), "HEAD",
 				sha1_to_hex(next->object.sha1), oneline) ||
-			write_tree(head, 0, NULL)) {
+			write_cache_as_tree(head, 0, NULL)) {
 		add_to_msg("\nConflicts:\n\n");
 		read_cache();
 		for (i = 0; i < active_nr;) {
diff --git a/builtin-write-tree.c b/builtin-write-tree.c
index b89d02e..e838d01 100644
--- a/builtin-write-tree.c
+++ b/builtin-write-tree.c
@@ -11,66 +11,12 @@
 static const char write_tree_usage[] =
 "git-write-tree [--missing-ok] [--prefix=<prefix>/]";
 
-int write_tree(unsigned char *sha1, int missing_ok, const char *prefix)
-{
-	int entries, was_valid, newfd;
-
-	/* We can't free this memory, it becomes part of a linked list parsed atexit() */
-	struct lock_file *lock_file = xcalloc(1, sizeof(struct lock_file));
-
-	newfd = hold_locked_index(lock_file, 1);
-
-	entries = read_cache();
-	if (entries < 0)
-		die("git-write-tree: error reading cache");
-
-	if (!active_cache_tree)
-		active_cache_tree = cache_tree();
-
-	was_valid = cache_tree_fully_valid(active_cache_tree);
-
-	if (!was_valid) {
-		if (cache_tree_update(active_cache_tree,
-				      active_cache, active_nr,
-				      missing_ok, 0) < 0)
-			die("git-write-tree: error building trees");
-		if (0 <= newfd) {
-			if (!write_cache(newfd, active_cache, active_nr)
-					&& !close(newfd)) {
-				commit_lock_file(lock_file);
-				newfd = -1;
-			}
-		}
-		/* Not being able to write is fine -- we are only interested
-		 * in updating the cache-tree part, and if the next caller
-		 * ends up using the old index with unupdated cache-tree part
-		 * it misses the work we did here, but that is just a
-		 * performance penalty and not a big deal.
-		 */
-	}
-
-	if (prefix) {
-		struct cache_tree *subtree =
-			cache_tree_find(active_cache_tree, prefix);
-		if (!subtree)
-			die("git-write-tree: prefix %s not found", prefix);
-		hashcpy(sha1, subtree->sha1);
-	}
-	else
-		hashcpy(sha1, active_cache_tree->sha1);
-
-	if (0 <= newfd)
-		close(newfd);
-	rollback_lock_file(lock_file);
-
-	return 0;
-}
-
 int cmd_write_tree(int argc, const char **argv, const char *unused_prefix)
 {
 	int missing_ok = 0, ret;
 	const char *prefix = NULL;
 	unsigned char sha1[20];
+	const char *me = "git-write-tree";
 
 	git_config(git_default_config);
 	while (1 < argc) {
@@ -87,8 +33,20 @@ int cmd_write_tree(int argc, const char **argv, const char *unused_prefix)
 	if (argc > 2)
 		die("too many options");
 
-	ret = write_tree(sha1, missing_ok, prefix);
-	printf("%s\n", sha1_to_hex(sha1));
-
+	ret = write_cache_as_tree(sha1, missing_ok, prefix);
+	switch (ret) {
+	case 0:
+		printf("%s\n", sha1_to_hex(sha1));
+		break;
+	case WRITE_TREE_UNREADABLE_INDEX:
+		die("%s: error reading the index", me);
+		break;
+	case WRITE_TREE_UNMERGED_INDEX:
+		die("%s: error building trees; the index is unmerged?", me);
+		break;
+	case WRITE_TREE_PREFIX_ERROR:
+		die("%s: prefix %s not found", me, prefix);
+		break;
+	}
 	return ret;
 }
diff --git a/builtin.h b/builtin.h
index cb675c4..3d1628c 100644
--- a/builtin.h
+++ b/builtin.h
@@ -8,7 +8,6 @@ extern const char git_usage_string[];
 
 extern void list_common_cmds_help(void);
 extern void help_unknown_cmd(const char *cmd);
-extern int write_tree(unsigned char *sha1, int missing_ok, const char *prefix);
 extern void prune_packed_objects(int);
 
 extern int cmd_add(int argc, const char **argv, const char *prefix);
diff --git a/cache-tree.c b/cache-tree.c
index 50b3526..69d0b46 100644
--- a/cache-tree.c
+++ b/cache-tree.c
@@ -529,3 +529,62 @@ struct cache_tree *cache_tree_find(struct cache_tree *it, const char *path)
 	}
 	return it;
 }
+
+int write_cache_as_tree(unsigned char *sha1, int missing_ok, const char *prefix)
+{
+	int entries, was_valid, newfd;
+
+	/*
+	 * We can't free this memory, it becomes part of a linked list
+	 * parsed atexit()
+	 */
+	struct lock_file *lock_file = xcalloc(1, sizeof(struct lock_file));
+
+	newfd = hold_locked_index(lock_file, 1);
+
+	entries = read_cache();
+	if (entries < 0)
+		return WRITE_TREE_UNREADABLE_INDEX;
+
+	if (!active_cache_tree)
+		active_cache_tree = cache_tree();
+
+	was_valid = cache_tree_fully_valid(active_cache_tree);
+
+	if (!was_valid) {
+		if (cache_tree_update(active_cache_tree,
+				      active_cache, active_nr,
+				      missing_ok, 0) < 0)
+			return WRITE_TREE_UNMERGED_INDEX;
+		if (0 <= newfd) {
+			if (!write_cache(newfd, active_cache, active_nr)
+					&& !close(newfd)) {
+				commit_lock_file(lock_file);
+				newfd = -1;
+			}
+		}
+		/* Not being able to write is fine -- we are only interested
+		 * in updating the cache-tree part, and if the next caller
+		 * ends up using the old index with unupdated cache-tree part
+		 * it misses the work we did here, but that is just a
+		 * performance penalty and not a big deal.
+		 */
+	}
+
+	if (prefix) {
+		struct cache_tree *subtree =
+			cache_tree_find(active_cache_tree, prefix);
+		if (!subtree)
+			return WRITE_TREE_PREFIX_ERROR;
+		hashcpy(sha1, subtree->sha1);
+	}
+	else
+		hashcpy(sha1, active_cache_tree->sha1);
+
+	if (0 <= newfd)
+		close(newfd);
+	rollback_lock_file(lock_file);
+
+	return 0;
+}
+
diff --git a/cache-tree.h b/cache-tree.h
index 8243228..44aad42 100644
--- a/cache-tree.h
+++ b/cache-tree.h
@@ -30,4 +30,9 @@ int cache_tree_update(struct cache_tree *, struct cache_entry **, int, int, int)
 
 struct cache_tree *cache_tree_find(struct cache_tree *, const char *);
 
+#define WRITE_TREE_UNREADABLE_INDEX (-1)
+#define WRITE_TREE_UNMERGED_INDEX (-2)
+#define WRITE_TREE_PREFIX_ERROR (-3)
+
+int write_cache_as_tree(unsigned char *sha1, int missing_ok, const char *prefix);
 #endif

Re: git-write-error with cherry-pick -n usages

[Björn Steinbrink]

On 2008.01.10 22:49:35 -0800, Junio C Hamano wrote:
> Björn Steinbrink <B.Steinbrink@gmx.de> writes:
> 
> > when you cherry-pick -n a commit that has changes to a file missing in
> > the current tree, that file will be added as "Unmerged". A subsequent
> > cherry-pick that tries to pick a commit that has changes to that file
> > will then fail with:
> >
> > fatal: git-write-tree: error building trees
> 
> Yeah, this was because the original "rewrite in C" did somewhat
> a sloppy job while stealing code from git-write-tree.
> 
> The caller pretends as if the write_tree() function would return
> an error code and being able to issue a sensible error message
> itself, but write_tree() function just calls die() and never
> returns an error.  Worse yet, the function claims that it was
> running git-write-tree (which is no longer true after
> cherry-pick stole it).
> 
> I think you would need to do something like this patch.  I will
> not apply it during the -rc period, but testing and resending
> with "Tested-by:" would be helpful after post 1.5.4 cycle opens.

Sorry for the delay, that mail got lost somewhere in the noise of my
inbox and real life. Successfully tested the patch to generate an useful
error message, as well as surviving the test suite and working correctly
on a random set of cherry-picks in some of my repos.

Thanks!

Tested-by: Björn Steinbrink <B.Steinbrink@gmx.de>

> 
> ---
>  builtin-revert.c     |    5 ++-
>  builtin-write-tree.c |   74 +++++++++++---------------------------------------
>  builtin.h            |    1 -
>  cache-tree.c         |   59 +++++++++++++++++++++++++++++++++++++++
>  cache-tree.h         |    5 +++
>  5 files changed, 83 insertions(+), 61 deletions(-)
> 
> diff --git a/builtin-revert.c b/builtin-revert.c
> index 4bf8eb2..d71f414 100644
> --- a/builtin-revert.c
> +++ b/builtin-revert.c
> @@ -8,6 +8,7 @@
>  #include "exec_cmd.h"
>  #include "utf8.h"
>  #include "parse-options.h"
> +#include "cache-tree.h"
>  
>  /*
>   * This implements the builtins revert and cherry-pick.
> @@ -270,7 +271,7 @@ static int revert_or_cherry_pick(int argc, const char **argv)
>  		 * that represents the "current" state for merge-recursive
>  		 * to work on.
>  		 */
> -		if (write_tree(head, 0, NULL))
> +		if (write_cache_as_tree(head, 0, NULL))
>  			die ("Your index file is unmerged.");
>  	} else {
>  		struct wt_status s;
> @@ -357,7 +358,7 @@ static int revert_or_cherry_pick(int argc, const char **argv)
>  	if (merge_recursive(sha1_to_hex(base->object.sha1),
>  				sha1_to_hex(head), "HEAD",
>  				sha1_to_hex(next->object.sha1), oneline) ||
> -			write_tree(head, 0, NULL)) {
> +			write_cache_as_tree(head, 0, NULL)) {
>  		add_to_msg("\nConflicts:\n\n");
>  		read_cache();
>  		for (i = 0; i < active_nr;) {
> diff --git a/builtin-write-tree.c b/builtin-write-tree.c
> index b89d02e..e838d01 100644
> --- a/builtin-write-tree.c
> +++ b/builtin-write-tree.c
> @@ -11,66 +11,12 @@
>  static const char write_tree_usage[] =
>  "git-write-tree [--missing-ok] [--prefix=<prefix>/]";
>  
> -int write_tree(unsigned char *sha1, int missing_ok, const char *prefix)
> -{
> -	int entries, was_valid, newfd;
> -
> -	/* We can't free this memory, it becomes part of a linked list parsed atexit() */
> -	struct lock_file *lock_file = xcalloc(1, sizeof(struct lock_file));
> -
> -	newfd = hold_locked_index(lock_file, 1);
> -
> -	entries = read_cache();
> -	if (entries < 0)
> -		die("git-write-tree: error reading cache");
> -
> -	if (!active_cache_tree)
> -		active_cache_tree = cache_tree();
> -
> -	was_valid = cache_tree_fully_valid(active_cache_tree);
> -
> -	if (!was_valid) {
> -		if (cache_tree_update(active_cache_tree,
> -				      active_cache, active_nr,
> -				      missing_ok, 0) < 0)
> -			die("git-write-tree: error building trees");
> -		if (0 <= newfd) {
> -			if (!write_cache(newfd, active_cache, active_nr)
> -					&& !close(newfd)) {
> -				commit_lock_file(lock_file);
> -				newfd = -1;
> -			}
> -		}
> -		/* Not being able to write is fine -- we are only interested
> -		 * in updating the cache-tree part, and if the next caller
> -		 * ends up using the old index with unupdated cache-tree part
> -		 * it misses the work we did here, but that is just a
> -		 * performance penalty and not a big deal.
> -		 */
> -	}
> -
> -	if (prefix) {
> -		struct cache_tree *subtree =
> -			cache_tree_find(active_cache_tree, prefix);
> -		if (!subtree)
> -			die("git-write-tree: prefix %s not found", prefix);
> -		hashcpy(sha1, subtree->sha1);
> -	}
> -	else
> -		hashcpy(sha1, active_cache_tree->sha1);
> -
> -	if (0 <= newfd)
> -		close(newfd);
> -	rollback_lock_file(lock_file);
> -
> -	return 0;
> -}
> -
>  int cmd_write_tree(int argc, const char **argv, const char *unused_prefix)
>  {
>  	int missing_ok = 0, ret;
>  	const char *prefix = NULL;
>  	unsigned char sha1[20];
> +	const char *me = "git-write-tree";
>  
>  	git_config(git_default_config);
>  	while (1 < argc) {
> @@ -87,8 +33,20 @@ int cmd_write_tree(int argc, const char **argv, const char *unused_prefix)
>  	if (argc > 2)
>  		die("too many options");
>  
> -	ret = write_tree(sha1, missing_ok, prefix);
> -	printf("%s\n", sha1_to_hex(sha1));
> -
> +	ret = write_cache_as_tree(sha1, missing_ok, prefix);
> +	switch (ret) {
> +	case 0:
> +		printf("%s\n", sha1_to_hex(sha1));
> +		break;
> +	case WRITE_TREE_UNREADABLE_INDEX:
> +		die("%s: error reading the index", me);
> +		break;
> +	case WRITE_TREE_UNMERGED_INDEX:
> +		die("%s: error building trees; the index is unmerged?", me);
> +		break;
> +	case WRITE_TREE_PREFIX_ERROR:
> +		die("%s: prefix %s not found", me, prefix);
> +		break;
> +	}
>  	return ret;
>  }
> diff --git a/builtin.h b/builtin.h
> index cb675c4..3d1628c 100644
> --- a/builtin.h
> +++ b/builtin.h
> @@ -8,7 +8,6 @@ extern const char git_usage_string[];
>  
>  extern void list_common_cmds_help(void);
>  extern void help_unknown_cmd(const char *cmd);
> -extern int write_tree(unsigned char *sha1, int missing_ok, const char *prefix);
>  extern void prune_packed_objects(int);
>  
>  extern int cmd_add(int argc, const char **argv, const char *prefix);
> diff --git a/cache-tree.c b/cache-tree.c
> index 50b3526..69d0b46 100644
> --- a/cache-tree.c
> +++ b/cache-tree.c
> @@ -529,3 +529,62 @@ struct cache_tree *cache_tree_find(struct cache_tree *it, const char *path)
>  	}
>  	return it;
>  }
> +
> +int write_cache_as_tree(unsigned char *sha1, int missing_ok, const char *prefix)
> +{
> +	int entries, was_valid, newfd;
> +
> +	/*
> +	 * We can't free this memory, it becomes part of a linked list
> +	 * parsed atexit()
> +	 */
> +	struct lock_file *lock_file = xcalloc(1, sizeof(struct lock_file));
> +
> +	newfd = hold_locked_index(lock_file, 1);
> +
> +	entries = read_cache();
> +	if (entries < 0)
> +		return WRITE_TREE_UNREADABLE_INDEX;
> +
> +	if (!active_cache_tree)
> +		active_cache_tree = cache_tree();
> +
> +	was_valid = cache_tree_fully_valid(active_cache_tree);
> +
> +	if (!was_valid) {
> +		if (cache_tree_update(active_cache_tree,
> +				      active_cache, active_nr,
> +				      missing_ok, 0) < 0)
> +			return WRITE_TREE_UNMERGED_INDEX;
> +		if (0 <= newfd) {
> +			if (!write_cache(newfd, active_cache, active_nr)
> +					&& !close(newfd)) {
> +				commit_lock_file(lock_file);
> +				newfd = -1;
> +			}
> +		}
> +		/* Not being able to write is fine -- we are only interested
> +		 * in updating the cache-tree part, and if the next caller
> +		 * ends up using the old index with unupdated cache-tree part
> +		 * it misses the work we did here, but that is just a
> +		 * performance penalty and not a big deal.
> +		 */
> +	}
> +
> +	if (prefix) {
> +		struct cache_tree *subtree =
> +			cache_tree_find(active_cache_tree, prefix);
> +		if (!subtree)
> +			return WRITE_TREE_PREFIX_ERROR;
> +		hashcpy(sha1, subtree->sha1);
> +	}
> +	else
> +		hashcpy(sha1, active_cache_tree->sha1);
> +
> +	if (0 <= newfd)
> +		close(newfd);
> +	rollback_lock_file(lock_file);
> +
> +	return 0;
> +}
> +
> diff --git a/cache-tree.h b/cache-tree.h
> index 8243228..44aad42 100644
> --- a/cache-tree.h
> +++ b/cache-tree.h
> @@ -30,4 +30,9 @@ int cache_tree_update(struct cache_tree *, struct cache_entry **, int, int, int)
>  
>  struct cache_tree *cache_tree_find(struct cache_tree *, const char *);
>  
> +#define WRITE_TREE_UNREADABLE_INDEX (-1)
> +#define WRITE_TREE_UNMERGED_INDEX (-2)
> +#define WRITE_TREE_PREFIX_ERROR (-3)
> +
> +int write_cache_as_tree(unsigned char *sha1, int missing_ok, const char *prefix);
>  #endif