threads / patch / 20637

patchread-tree: Fix regression with creation of a new index file.

Subject: [PATCH] read-tree: Fix regression with creation of a new index file.

## tl;dr

3 messages between Aug 17, 2009 and Aug 18, 2009. Diffs are folded; open one to read it.

replies: 2people: 3as markdown or json

Alexandre Julliard· Aug 17, 2009, 15:35 UTC · lore

Reading the index into an empty file has been broken by 5a56da58060e50980fab0f4c38203a25440d1530, since it causes the existing index to always be loaded first, and dies if it's an empty file:

$ GIT_INDEX_FILE=`mktemp` git read-tree master fatal: index file smaller than expected

It breaks for instance committing from git.el. This patch reverts to the previous behavior of only loading the index when merging it.

Signed-off-by: Alexandre Julliard <julliard@winehq.org>
---
 builtin-read-tree.c            |   10 ++++++----
 t/t1009-read-tree-new-index.sh |   25 +++++++++++++++++++++++++
 2 files changed, 31 insertions(+), 4 deletions(-)
 create mode 100755 t/t1009-read-tree-new-index.sh

Show changes to 2 files +31 −4

builtin-read-tree.c, t/t1009-read-tree-new-index.sh

diff --git a/builtin-read-tree.c b/builtin-read-tree.c
index 9c2d634..14c836b 100644
--- a/builtin-read-tree.c
+++ b/builtin-read-tree.c
@@ -113,13 +113,15 @@ int cmd_read_tree(int argc, const char **argv, const char *unused_prefix)
 	argc = parse_options(argc, argv, unused_prefix, read_tree_options,
 			     read_tree_usage, 0);
 
-	if (read_cache_unmerged() && (opts.prefix || opts.merge))
-		die("You need to resolve your current index first");
-
 	prefix_set = opts.prefix ? 1 : 0;
 	if (1 < opts.merge + opts.reset + prefix_set)
 		die("Which one? -m, --reset, or --prefix?");
-	stage = opts.merge = (opts.reset || opts.merge || prefix_set);
+
+	if (opts.reset || opts.merge || opts.prefix) {
+		if (read_cache_unmerged() && (opts.prefix || opts.merge))
+			die("You need to resolve your current index first");
+		stage = opts.merge = 1;
+	}
 
 	for (i = 0; i < argc; i++) {
 		const char *arg = argv[i];
diff --git a/t/t1009-read-tree-new-index.sh b/t/t1009-read-tree-new-index.sh
new file mode 100755
index 0000000..59b3aa4
--- /dev/null
+++ b/t/t1009-read-tree-new-index.sh
@@ -0,0 +1,25 @@
+#!/bin/sh
+
+test_description='test read-tree into a fresh index file'
+
+. ./test-lib.sh
+
+test_expect_success setup '
+	echo one >a &&
+	git add a &&
+	git commit -m initial
+'
+
+test_expect_success 'non-existent index file' '
+	rm -f new-index &&
+	GIT_INDEX_FILE=new-index git read-tree master
+'
+
+test_expect_success 'empty index file' '
+	rm -f new-index &&
+	> new-index &&
+	GIT_INDEX_FILE=new-index git read-tree master
+'
+
+test_done
+

-- 
1.6.4.181.g3f2ea.dirty

-- 
Alexandre Julliard
julliard@winehq.org

Johannes Schindelin· Aug 17, 2009, 22:19 UTC · re: Alexandre Julliard · lore

Re: [PATCH] read-tree: Fix regression with creation of a new index file.

Hi,

On Mon, 17 Aug 2009, Alexandre Julliard wrote:

Show 21 quoted lines

> diff --git a/builtin-read-tree.c b/builtin-read-tree.c
> index 9c2d634..14c836b 100644
> --- a/builtin-read-tree.c
> +++ b/builtin-read-tree.c
> @@ -113,13 +113,15 @@ int cmd_read_tree(int argc, const char **argv, const char *unused_prefix)
>  	argc = parse_options(argc, argv, unused_prefix, read_tree_options,
>  			     read_tree_usage, 0);
>  
> -	if (read_cache_unmerged() && (opts.prefix || opts.merge))
> -		die("You need to resolve your current index first");
> -
>  	prefix_set = opts.prefix ? 1 : 0;
>  	if (1 < opts.merge + opts.reset + prefix_set)
>  		die("Which one? -m, --reset, or --prefix?");
> -	stage = opts.merge = (opts.reset || opts.merge || prefix_set);
> +
> +	if (opts.reset || opts.merge || opts.prefix) {
> +		if (read_cache_unmerged() && (opts.prefix || opts.merge))
> +			die("You need to resolve your current index first");
> +		stage = opts.merge = 1;
> +	}

Actually, this should be enough:

-- snipsnap --

Show changes to builtin-read-tree.c +3 −3

diff --git a/builtin-read-tree.c b/builtin-read-tree.c
index 9c2d634..d649c56 100644
--- a/builtin-read-tree.c
+++ b/builtin-read-tree.c
@@ -113,14 +113,14 @@ int cmd_read_tree(int argc, const char **argv, const char *unused_prefix)
 	argc = parse_options(argc, argv, unused_prefix, read_tree_options,
 			     read_tree_usage, 0);
 
-	if (read_cache_unmerged() && (opts.prefix || opts.merge))
-		die("You need to resolve your current index first");
-
 	prefix_set = opts.prefix ? 1 : 0;
 	if (1 < opts.merge + opts.reset + prefix_set)
 		die("Which one? -m, --reset, or --prefix?");
 	stage = opts.merge = (opts.reset || opts.merge || prefix_set);
 
+	if (opts.merge && (read_cache_unmerged() && !prefix_set && !opts.reset))
+		die("You need to resolve your current index first");
+
 	for (i = 0; i < argc; i++) {
 		const char *arg = argv[i];

-- 
1.6.4.313.g3d9e3

Stephen Boyd· Aug 18, 2009, 03:37 UTC · re: Johannes Schindelin · lore

Re: [PATCH] read-tree: Fix regression with creation of a new index file.

Johannes Schindelin wrote:

Show 18 quoted lines

> diff --git a/builtin-read-tree.c b/builtin-read-tree.c
> index 9c2d634..d649c56 100644
> --- a/builtin-read-tree.c
> +++ b/builtin-read-tree.c
> @@ -113,14 +113,14 @@ int cmd_read_tree(int argc, const char **argv, const char *unused_prefix)
>  	argc = parse_options(argc, argv, unused_prefix, read_tree_options,
>  			     read_tree_usage, 0);
>  
> -	if (read_cache_unmerged() && (opts.prefix || opts.merge))
> -		die("You need to resolve your current index first");
> -
>  	prefix_set = opts.prefix ? 1 : 0;
>  	if (1 < opts.merge + opts.reset + prefix_set)
>  		die("Which one? -m, --reset, or --prefix?");
>  	stage = opts.merge = (opts.reset || opts.merge || prefix_set);
>  
> +	if (opts.merge && (read_cache_unmerged() && !prefix_set && !opts.reset))
>

This looks more compact but I think the !prefix_set check is wrong.

Yes, we want to do read_cache_unmerged() if we're doing some sort of merging operation. But we want to die() when either -m or --prefix is used. Therefore, die() if we're not doing a --reset. So we might as well just check that case and nothing else.

The original patch from Alexandre is correct, but if you want to avoid extra nesting I suppose you could do something like the patch below.

Thanks.

---

Show changes to builtin-read-tree.c +2 −3

diff --git a/builtin-read-tree.c b/builtin-read-tree.c
index 9c2d634..c6d5b49 100644
--- a/builtin-read-tree.c
+++ b/builtin-read-tree.c
@@ -113,14 +113,14 @@ int cmd_read_tree(int argc, const char **argv, const char *unused_prefix)
        argc = parse_options(argc, argv, unused_prefix, read_tree_options,
                             read_tree_usage, 0);
 
-       if (read_cache_unmerged() && (opts.prefix || opts.merge))
-               die("You need to resolve your current index first");
-
        prefix_set = opts.prefix ? 1 : 0;
        if (1 < opts.merge + opts.reset + prefix_set)
                die("Which one? -m, --reset, or --prefix?");
        stage = opts.merge = (opts.reset || opts.merge || prefix_set);
 
+       if (opts.merge && read_cache_unmerged() && !opts.reset)
+               die("You need to resolve your current index first");