[PATCH 0/5] "best effort" checkout

I consider the first one is not part of the series but a bugfix.

The remainder is to teach "git checkout" not to punt in the middle once it
has started touching the work tree.

It does _not_ attempt to autorename a file whose name is NUL to something
else.  Partly because I personally think that sort of magic (or "a cute
hack") makes the system unnecessarily complex and fragile while not adding
much to the usability, but more importantly because I do not think we are
ready to adopt that kind of complexity yet, before fixing more basic issue
like this series addresses.

[PATCH 1/5] "git checkout -- paths..." should error out when paths cannot be written
[PATCH 2/5] checkout: make reset_clean_to_new() not die by itself
[PATCH 3/5] checkout: consolidate reset_{to_new,clean_to_new|()
[PATCH 4/5] unpack_trees(): allow callers to differentiate worktree errors from merge errors
[PATCH 5/5] checkout: "best effort" checkout
[PATCH 6/5] NUL hack to create_file()

When "git checkout -- paths..." cannot update work tree for whatever
reason, checkout_entry() correctly issued an error message for the path to
the end user, but the command ignored the error, causing the entire
command to succeed.  This fixes it.

Signed-off-by: Junio C Hamano <redacted>
---
 builtin-checkout.c |    7 +++++--
 1 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/builtin-checkout.c b/builtin-checkout.c
index 1ea017f..00dc8ca 100644
--- a/builtin-checkout.c
+++ b/builtin-checkout.c

@@ -84,6 +84,7 @@ static int checkout_paths(struct tree *source_tree, const char **pathspec)
 	unsigned char rev[20];
 	int flag;
 	struct commit *head;
+	int errs = 0;
 
 	int newfd;
 	struct lock_file *lock_file = xcalloc(1, sizeof(struct lock_file));

@@ -106,13 +107,14 @@ static int checkout_paths(struct tree *source_tree, const char **pathspec)
 	if (report_path_error(ps_matched, pathspec, 0))
 		return 1;
 
+	/* Now we are committed to check them out */
 	memset(&state, 0, sizeof(state));
 	state.force = 1;
 	state.refresh_cache = 1;
 	for (pos = 0; pos < active_nr; pos++) {
 		struct cache_entry *ce = active_cache[pos];
 		if (pathspec_match(pathspec, NULL, ce->name, 0)) {
-			checkout_entry(ce, &state, NULL);
+			errs |= checkout_entry(ce, &state, NULL);
 		}
 	}
 

@@ -123,7 +125,8 @@ static int checkout_paths(struct tree *source_tree, const char **pathspec)
 	resolve_ref("HEAD", rev, 0, &flag);
 	head = lookup_commit_reference_gently(rev, 1);
 
-	return post_checkout_hook(head, head, 0);
+	errs |= post_checkout_hook(head, head, 0);
+	return errs;
 }
 
 static void show_local_changes(struct object *head)

-- 
1.5.6.rc0.43.g823ea

These two were very similar functions with only tiny bit of difference.

Signed-off-by: Junio C Hamano <redacted>
---
 * This may be a bit hard to read but "struct checkout_opts" is moved up
   at the same time as it is passed to the consolidated function as its
   parameter.

 builtin-checkout.c |   50 +++++++++++++++-----------------------------------
 1 files changed, 15 insertions(+), 35 deletions(-)

diff --git a/builtin-checkout.c b/builtin-checkout.c
index cc97724..9af5197 100644
--- a/builtin-checkout.c
+++ b/builtin-checkout.c

@@ -151,39 +151,29 @@ static void describe_detached_head(char *msg, struct commit *commit)
 	strbuf_release(&sb);
 }
 
-static int reset_to_new(struct tree *tree, int quiet)
-{
-	struct unpack_trees_options opts;
-	struct tree_desc tree_desc;
-
-	memset(&opts, 0, sizeof(opts));
-	opts.head_idx = -1;
-	opts.update = 1;
-	opts.reset = 1;
-	opts.merge = 1;
-	opts.fn = oneway_merge;
-	opts.verbose_update = !quiet;
-	opts.src_index = &the_index;
-	opts.dst_index = &the_index;
-	parse_tree(tree);
-	init_tree_desc(&tree_desc, tree->buffer, tree->size);
-	if (unpack_trees(1, &tree_desc, &opts))
-		return 128;
-	return 0;
-}
+struct checkout_opts {
+	int quiet;
+	int merge;
+	int force;
+
+	char *new_branch;
+	int new_branch_log;
+	enum branch_track track;
+};
 
-static int reset_clean_to_new(struct tree *tree, int quiet)
+static int reset_tree(struct tree *tree, struct checkout_opts *o, int worktree)
 {
 	struct unpack_trees_options opts;
 	struct tree_desc tree_desc;
 
 	memset(&opts, 0, sizeof(opts));
 	opts.head_idx = -1;
-	opts.skip_unmerged = 1;
+	opts.update = worktree;
+	opts.skip_unmerged = !worktree;
 	opts.reset = 1;
 	opts.merge = 1;
 	opts.fn = oneway_merge;
-	opts.verbose_update = !quiet;
+	opts.verbose_update = !o->quiet;
 	opts.src_index = &the_index;
 	opts.dst_index = &the_index;
 	parse_tree(tree);

@@ -193,16 +183,6 @@ static int reset_clean_to_new(struct tree *tree, int quiet)
 	return 0;
 }
 
-struct checkout_opts {
-	int quiet;
-	int merge;
-	int force;
-
-	char *new_branch;
-	int new_branch_log;
-	enum branch_track track;
-};
-
 struct branch_info {
 	const char *name; /* The short name used */
 	const char *path; /* The full name of a real branch */

@@ -227,7 +207,7 @@ static int merge_working_tree(struct checkout_opts *opts,
 	read_cache();
 
 	if (opts->force) {
-		ret = reset_to_new(new->commit->tree, opts->quiet);
+		ret = reset_tree(new->commit->tree, opts, 1);
 		if (ret)
 			return ret;
 	} else {

@@ -291,12 +271,12 @@ static int merge_working_tree(struct checkout_opts *opts,
 			add_files_to_cache(NULL, NULL, 0);
 			work = write_tree_from_memory();
 
-			ret = reset_to_new(new->commit->tree, opts->quiet);
+			ret = reset_tree(new->commit->tree, opts, 1);
 			if (ret)
 				return ret;
 			merge_trees(new->commit->tree, work, old->commit->tree,
 				    new->name, "local", &result);
-			ret = reset_clean_to_new(new->commit->tree, opts->quiet);
+			ret = reset_tree(new->commit->tree, opts, 0);
 			if (ret)
 				return ret;
 		}

-- 
1.5.6.rc0.43.g823ea

When unpack_trees() returned an error while switching branches, we used to
stop right there, exiting without writing the index out or switching HEAD.

This is Ok when unpack_trees() detected a locally modified paths or
untracked files that could be overwritten by branch switching, but it is
undesirable if unpack_trees() already committed to update the work tree
and a failure is returned because some but not all paths are updated
(perhaps a directory that some files need to go in was made read-only by
mistake, or a file that will be overwritten by branch switching had a
mandatory lock on it and we could not unlink).

This changes the behaviour upon such an error to complete the branch
switching; the files updated in the work tree will hopefully much more
consistent with the index and HEAD derived from the switched-to branch.

We still issue error messages, and exit the command with non-zero status,
so scripted callers need to notice it.

Signed-off-by: Junio C Hamano <redacted>
---
 builtin-checkout.c |   22 ++++++++++++++++++----
 1 files changed, 18 insertions(+), 4 deletions(-)

diff --git a/builtin-checkout.c b/builtin-checkout.c
index 9af5197..93ea69b 100644
--- a/builtin-checkout.c
+++ b/builtin-checkout.c

@@ -155,6 +155,7 @@ struct checkout_opts {
 	int quiet;
 	int merge;
 	int force;
+	int writeout_error;
 
 	char *new_branch;
 	int new_branch_log;

@@ -178,9 +179,20 @@ static int reset_tree(struct tree *tree, struct checkout_opts *o, int worktree)
 	opts.dst_index = &the_index;
 	parse_tree(tree);
 	init_tree_desc(&tree_desc, tree->buffer, tree->size);
-	if (unpack_trees(1, &tree_desc, &opts))
+	switch (unpack_trees(1, &tree_desc, &opts)) {
+	case -2:
+		o->writeout_error = 1;
+		/*
+		 * We return 0 nevertheless, as the index is all right
+		 * and more importantly we have made best efforts to
+		 * update paths in the work tree, and we cannot revert
+		 * them.
+		 */
+	case 0:
+		return 0;
+	default:
 		return 128;
-	return 0;
+	}
 }
 
 struct branch_info {

@@ -243,7 +255,8 @@ static int merge_working_tree(struct checkout_opts *opts,
 		tree = parse_tree_indirect(new->commit->object.sha1);
 		init_tree_desc(&trees[1], tree->buffer, tree->size);
 
-		if (unpack_trees(2, trees, &topts)) {
+		ret = unpack_trees(2, trees, &topts);
+		if (ret == -1) {
 			/*
 			 * Unpack couldn't do a trivial merge; either
 			 * give up or do a real merge, depending on

@@ -478,7 +491,8 @@ static int switch_branches(struct checkout_opts *opts, struct branch_info *new)
 
 	update_refs_for_switch(opts, &old, new);
 
-	return post_checkout_hook(old.commit, new->commit, 1);
+	ret = post_checkout_hook(old.commit, new->commit, 1);
+	return ret || opts->writeout_error;
 }
 
 int cmd_checkout(int argc, const char **argv, const char *prefix)

-- 
1.5.6.rc0.43.g823ea

Instead of uniformly returning -1 on any error, this teaches
unpack_trees() to return -2 when the merge itself is Ok but worktree
refuses to get updated.

Signed-off-by: Junio C Hamano <redacted>
---
 unpack-trees.c |   10 +++++++---
 1 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/unpack-trees.c b/unpack-trees.c
index 0de5a31..cba0aca 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c

@@ -358,8 +358,13 @@ static int unpack_failed(struct unpack_trees_options *o, const char *message)
 	return -1;
 }
 
+/*
+ * N-way merge "len" trees.  Returns 0 on success, -1 on failure to manipulate the
+ * resulting index, -2 on failure to reflect the changes to the work tree.
+ */
 int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options *o)
 {
+	int ret;
 	static struct cache_entry *dfc;
 
 	if (len > MAX_UNPACK_TREES)

@@ -404,11 +409,10 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options
 		return unpack_failed(o, "Merge requires file-level merging");
 
 	o->src_index = NULL;
-	if (check_updates(o))
-		return -1;
+	ret = check_updates(o) ? (-2) : 0;
 	if (o->dst_index)
 		*o->dst_index = o->result;
-	return 0;
+	return ret;
 }
 
 /* Here come the merge functions */

-- 
1.5.6.rc0.43.g823ea

This is not meant for application to the mainline.  It allows your git to
refuse to create a blob whose name is "nul".

---
 entry.c |    8 ++++++++
 1 files changed, 8 insertions(+), 0 deletions(-)

diff --git a/entry.c b/entry.c
index 222aaa3..d24b803 100644
--- a/entry.c
+++ b/entry.c

@@ -81,6 +81,14 @@ static void remove_subtree(const char *path)
 
 static int create_file(const char *path, unsigned int mode)
 {
+	if (1) {
+		size_t len = strlen(path);
+		if (3 <= len && !strcmp(path + len - 3, "nul") &&
+		    (3 == len || path[len - 4] == '/')) {
+			errno = EPERM;
+			return -1;
+		}
+	}
 	mode = (mode & 0100) ? 0777 : 0666;
 	return open(path, O_WRONLY | O_CREAT | O_EXCL, mode);
 }

-- 
1.5.6.rc0.43.g823ea

Instead, have its error percolate up through the callchain and let it be
the exit status of the main command.  No semantic changes yet.

Signed-off-by: Junio C Hamano <redacted>
---
 builtin-checkout.c |    9 ++++++---
 1 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/builtin-checkout.c b/builtin-checkout.c
index 00dc8ca..cc97724 100644
--- a/builtin-checkout.c
+++ b/builtin-checkout.c

@@ -172,7 +172,7 @@ static int reset_to_new(struct tree *tree, int quiet)
 	return 0;
 }
 
-static void reset_clean_to_new(struct tree *tree, int quiet)
+static int reset_clean_to_new(struct tree *tree, int quiet)
 {
 	struct unpack_trees_options opts;
 	struct tree_desc tree_desc;

@@ -189,7 +189,8 @@ static void reset_clean_to_new(struct tree *tree, int quiet)
 	parse_tree(tree);
 	init_tree_desc(&tree_desc, tree->buffer, tree->size);
 	if (unpack_trees(1, &tree_desc, &opts))
-		exit(128);
+		return 128;
+	return 0;
 }
 
 struct checkout_opts {

@@ -295,7 +296,9 @@ static int merge_working_tree(struct checkout_opts *opts,
 				return ret;
 			merge_trees(new->commit->tree, work, old->commit->tree,
 				    new->name, "local", &result);
-			reset_clean_to_new(new->commit->tree, opts->quiet);
+			ret = reset_clean_to_new(new->commit->tree, opts->quiet);
+			if (ret)
+				return ret;
 		}
 	}
 

-- 
1.5.6.rc0.43.g823ea

Junio C Hamano schrieb:

This is not meant for application to the mainline.  It allows your git to
refuse to create a blob whose name is "nul".

It's not just about "nul"; these won't work either: "aux", "prn", "con",
"com\d+", "lpt\d+", neither do "$one_of_these.$some_extension". And all of
that regardless of the case!

See http://msdn.microsoft.com/en-us/library/aa365247(VS.85).aspx

Definitely, we don't ever want to have such special-casing somewhere in git.

-- Hannes

Johannes Sixt said the following on 29.05.2008 08:33:

Junio C Hamano schrieb:

quoted

This is not meant for application to the mainline.  It allows your git to
refuse to create a blob whose name is "nul".

It's not just about "nul"; these won't work either: "aux", "prn", "con",
"com\d+", "lpt\d+", neither do "$one_of_these.$some_extension". And all of
that regardless of the case!

See http://msdn.microsoft.com/en-us/library/aa365247(VS.85).aspx

Definitely, we don't ever want to have such special-casing somewhere in git.

They _can_ be used by using the UNC notation:
     \\?\<drive letter>:\<path>\nul
Do you think we should special-case that, or simply fail?

( 9:04:57 - D:\home\marius\source\hg\test)
 > hg checkout
abort: The parameter is incorrect: D:\home\marius\source\hg\test\nul

'Nuff said? :-)

-- 
.marius [@trolltech.com]
'if you know what you're doing, it's not research'

Marius Storm-Olsen schrieb:

Johannes Sixt said the following on 29.05.2008 08:33:

quoted

Junio C Hamano schrieb:

quoted

This is not meant for application to the mainline.  It allows your
git to
refuse to create a blob whose name is "nul".

It's not just about "nul"; these won't work either: "aux", "prn", "con",
"com\d+", "lpt\d+", neither do "$one_of_these.$some_extension". And
all of
that regardless of the case!

See http://msdn.microsoft.com/en-us/library/aa365247(VS.85).aspx

Definitely, we don't ever want to have such special-casing somewhere
in git.

They _can_ be used by using the UNC notation:
    \\?\<drive letter>:\<path>\nul
Do you think we should special-case that, or simply fail?

Rhetoric question: What's so special about those files?

"foo/nul" is a file you don't have permissions to write to. Period. We
should fail the same way as if you had 'chmod a-w foo/nul foo', or as if
there's a bad sector on the disk. Junio's patch series is the way to go
(without 6/5, of course).

-- Hannes

Hi,

On Thu, 29 May 2008, Johannes Sixt wrote:

Junio C Hamano schrieb:

quoted

This is not meant for application to the mainline.  It allows your git 
to refuse to create a blob whose name is "nul".

It's not just about "nul"; these won't work either: "aux", "prn", "con", 
"com\d+", "lpt\d+", neither do "$one_of_these.$some_extension". And all 
of that regardless of the case!

See http://msdn.microsoft.com/en-us/library/aa365247(VS.85).aspx

Definitely, we don't ever want to have such special-casing somewhere in 
git.

I think that the standard methods, namely checking by hook, should be good 
enough.

Ciao,
Dscho

On Wed, 28 May 2008, Junio C Hamano wrote:

This is not meant for application to the mainline.  It allows your git to
refuse to create a blob whose name is "nul".

I assume this is so you can test git's response to a defective filesystem 
without actually having a defective filesystem?

Shouldn't this be EEXIST? I think the issue is that the first exists for 
the purpose of open() but not for anything else we've done up to this 
point.

+			return -1;
+		}
+	}
 	mode = (mode & 0100) ? 0777 : 0666;
 	return open(path, O_WRONLY | O_CREAT | O_EXCL, mode);
 }
-- 
1.5.6.rc0.43.g823ea

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Thu, 29 May 2008, Marius Storm-Olsen wrote:

Johannes Sixt said the following on 29.05.2008 08:33:

quoted

Junio C Hamano schrieb:

quoted

This is not meant for application to the mainline.  It allows your git to
refuse to create a blob whose name is "nul".

It's not just about "nul"; these won't work either: "aux", "prn", "con",
"com\d+", "lpt\d+", neither do "$one_of_these.$some_extension". And all of
that regardless of the case!

See http://msdn.microsoft.com/en-us/library/aa365247(VS.85).aspx

Definitely, we don't ever want to have such special-casing somewhere in git.

They _can_ be used by using the UNC notation:
    \\?\<drive letter>:\<path>\nul
Do you think we should special-case that, or simply fail?

Perhaps we should see if we can get an open() that always uses that 
notation for the actual system call? I doubt we want to support the 
DOS-ish meanings even if the user provides them as input sources. If it's 
not actually a problem with the underlying storage mechanism, but rather a 
flaw in the POSIX implementation for Windows, we should fix that (or do 
something in compat to work around it) instead of failing in any way to 
support it in git. Of course, people on Windows using projects with these 
filenames will probably run into problems with other tools, but at least 
git will behave properly.

On the other hand, I bet there are going to be real issues with filenames 
with backslashes in them.

	-Daniel
*This .sig left intentionally blank*

Junio C Hamano, Thu, May 29, 2008 02:17:26 +0200:

This is not meant for application to the mainline. ...

That's good. No one besides Windows has such a stupid filesystem.

Daniel Barkalow wrote:

support it in git. Of course, people on Windows using projects with these
filenames will probably run into problems with other tools, but at least
git will behave properly.

I don't see how it would help to have core git using the Native syntax
to bypass the Win32 layer's restrictions but none of the accompanying
suite of tools, i.e. the dozens of various MSYS sh.exe, perl.exe,
cat.exe, etc.  None of those would be able to open or even delete those
files with the reserved filenames.

Users tend to get upset when software creates files that cannot be
removed through conventional methods, e.g. Explorer is completely
powerless to remove it.  Cygwin shipped with a bug several years ago
that unintentionally allowed to create (but not unlink) reserved
filenames.  Unless you knew the magical incantation of "del
\\.\c:\path\to\nul" the file was immutable.

Brian

On Thu, 29 May 2008, Brian Dessent wrote:

Daniel Barkalow wrote:

quoted

support it in git. Of course, people on Windows using projects with these
filenames will probably run into problems with other tools, but at least
git will behave properly.

I don't see how it would help to have core git using the Native syntax
to bypass the Win32 layer's restrictions but none of the accompanying
suite of tools, i.e. the dozens of various MSYS sh.exe, perl.exe,
cat.exe, etc.  None of those would be able to open or even delete those
files with the reserved filenames.

Users tend to get upset when software creates files that cannot be
removed through conventional methods, e.g. Explorer is completely
powerless to remove it.  Cygwin shipped with a bug several years ago
that unintentionally allowed to create (but not unlink) reserved
filenames.  Unless you knew the magical incantation of "del
\\.\c:\path\to\nul" the file was immutable.

Well, "git rm <filename>" would work. Or "git mv <weird> <okay>", which is 
possibly more productive. Or checking out a version that doesn't contain 
it. It's a lot worse if the tool that created it can't remove it than if 
tools other than the one that created it can't deal with it.

	-Daniel
*This .sig left intentionally blank*

Junio C Hamano wrote:

	
[PATCH 1/5] "git checkout -- paths..." should error out when paths cannot be written
[PATCH 2/5] checkout: make reset_clean_to_new() not die by itself
[PATCH 3/5] checkout: consolidate reset_{to_new,clean_to_new|()
[PATCH 4/5] unpack_trees(): allow callers to differentiate worktree errors from merge errors
[PATCH 5/5] checkout: "best effort" checkout
[PATCH 6/5] NUL hack to create_file()

This works! I've added these patches (pulled from pu) to my tree and rebuilt. 
The current results on Cygwin...

git>git checkout -f b71ce7f3f13ebd0e
Previous HEAD position was 952538f... checkout: "best effort" checkout
error: git-checkout-index: unable to create file t/t5100/nul (File exists)
HEAD is now at b71ce7f... Merge 1.5.5.3 in
git>git status
# Not currently on any branch.
# Changed but not updated:
#   (use "git add <file>..." to update what will be committed)
#
#   modified:   t/t5100/nul
#
no changes added to commit (use "git add" and/or "git commit -a")
git>git mv  t/t5100/nul t/t5100/nul-plain
fatal: renaming t/t5100/nul failed: Invalid argument
git>git rm -f  --cached t/t5100/nul
rm 't/t5100/nul'
git>git show HEAD:t/t5100/nul
 From nobody Mon Sep 17 00:00:00 2001

---

diff --git a/foo b/foo

^Some strange test^^
^@

So, for posterity, git-mv cannot rename the offending file in the index, but the 
file can be removed, and its contents piped into a file of non-offending name, 
so a reasonable solution for this case exists.

Many thanks to all, especially to Junio for actually creating the fix.

Mark