* [PATCH 0/5] "best effort" checkout
@ 2008-05-29 0:17 Junio C Hamano
2008-05-29 0:17 ` [PATCH 1/5] "git checkout -- paths..." should error out when paths cannot be written Junio C Hamano
2008-05-29 23:24 ` [PATCH 0/5] "best effort" checkout Mark Levedahl
0 siblings, 2 replies; 20+ messages in thread
From: Junio C Hamano @ 2008-05-29 0:17 UTC (permalink / raw)
To: git
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()
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH 1/5] "git checkout -- paths..." should error out when paths cannot be written
2008-05-29 0:17 [PATCH 0/5] "best effort" checkout Junio C Hamano
@ 2008-05-29 0:17 ` Junio C Hamano
2008-05-29 0:17 ` [PATCH 2/5] checkout: make reset_clean_to_new() not die by itself Junio C Hamano
2008-05-29 23:24 ` [PATCH 0/5] "best effort" checkout Mark Levedahl
1 sibling, 1 reply; 20+ messages in thread
From: Junio C Hamano @ 2008-05-29 0:17 UTC (permalink / raw)
To: git
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 <gitster@pobox.com>
---
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
^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH 2/5] checkout: make reset_clean_to_new() not die by itself
2008-05-29 0:17 ` [PATCH 1/5] "git checkout -- paths..." should error out when paths cannot be written Junio C Hamano
@ 2008-05-29 0:17 ` Junio C Hamano
2008-05-29 0:17 ` [PATCH 3/5] checkout: consolidate reset_{to_new,clean_to_new|() Junio C Hamano
0 siblings, 1 reply; 20+ messages in thread
From: Junio C Hamano @ 2008-05-29 0:17 UTC (permalink / raw)
To: git
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 <gitster@pobox.com>
---
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
^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH 3/5] checkout: consolidate reset_{to_new,clean_to_new|()
2008-05-29 0:17 ` [PATCH 2/5] checkout: make reset_clean_to_new() not die by itself Junio C Hamano
@ 2008-05-29 0:17 ` Junio C Hamano
2008-05-29 0:17 ` [PATCH 4/5] unpack_trees(): allow callers to differentiate worktree errors from merge errors Junio C Hamano
0 siblings, 1 reply; 20+ messages in thread
From: Junio C Hamano @ 2008-05-29 0:17 UTC (permalink / raw)
To: git
These two were very similar functions with only tiny bit of difference.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
* 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
^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH 4/5] unpack_trees(): allow callers to differentiate worktree errors from merge errors
2008-05-29 0:17 ` [PATCH 3/5] checkout: consolidate reset_{to_new,clean_to_new|() Junio C Hamano
@ 2008-05-29 0:17 ` Junio C Hamano
2008-05-29 0:17 ` [PATCH 5/5] checkout: "best effort" checkout Junio C Hamano
0 siblings, 1 reply; 20+ messages in thread
From: Junio C Hamano @ 2008-05-29 0:17 UTC (permalink / raw)
To: git
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 <gitster@pobox.com>
---
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
^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH 5/5] checkout: "best effort" checkout
2008-05-29 0:17 ` [PATCH 4/5] unpack_trees(): allow callers to differentiate worktree errors from merge errors Junio C Hamano
@ 2008-05-29 0:17 ` Junio C Hamano
2008-05-29 0:17 ` [PATCH 6/5] NUL hack to create_file() Junio C Hamano
0 siblings, 1 reply; 20+ messages in thread
From: Junio C Hamano @ 2008-05-29 0:17 UTC (permalink / raw)
To: git
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 <gitster@pobox.com>
---
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
^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH 6/5] NUL hack to create_file()
2008-05-29 0:17 ` [PATCH 5/5] checkout: "best effort" checkout Junio C Hamano
@ 2008-05-29 0:17 ` Junio C Hamano
2008-05-29 6:33 ` Johannes Sixt
` (2 more replies)
0 siblings, 3 replies; 20+ messages in thread
From: Junio C Hamano @ 2008-05-29 0:17 UTC (permalink / raw)
To: git
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
^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [PATCH 6/5] NUL hack to create_file()
2008-05-29 0:17 ` [PATCH 6/5] NUL hack to create_file() Junio C Hamano
@ 2008-05-29 6:33 ` Johannes Sixt
2008-05-29 7:05 ` Marius Storm-Olsen
2008-05-29 12:39 ` Johannes Schindelin
2008-05-29 15:55 ` Daniel Barkalow
2008-05-29 17:44 ` Alex Riesen
2 siblings, 2 replies; 20+ messages in thread
From: Johannes Sixt @ 2008-05-29 6:33 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
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
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 6/5] NUL hack to create_file()
2008-05-29 6:33 ` Johannes Sixt
@ 2008-05-29 7:05 ` Marius Storm-Olsen
2008-05-29 7:23 ` Johannes Sixt
2008-05-29 17:19 ` Daniel Barkalow
2008-05-29 12:39 ` Johannes Schindelin
1 sibling, 2 replies; 20+ messages in thread
From: Marius Storm-Olsen @ 2008-05-29 7:05 UTC (permalink / raw)
To: Johannes Sixt; +Cc: Junio C Hamano, git
[-- Attachment #1: Type: text/plain, Size: 938 bytes --]
Johannes Sixt said the following on 29.05.2008 08:33:
> 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.
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'
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 187 bytes --]
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 6/5] NUL hack to create_file()
2008-05-29 7:05 ` Marius Storm-Olsen
@ 2008-05-29 7:23 ` Johannes Sixt
2008-05-29 17:19 ` Daniel Barkalow
1 sibling, 0 replies; 20+ messages in thread
From: Johannes Sixt @ 2008-05-29 7:23 UTC (permalink / raw)
To: Marius Storm-Olsen; +Cc: Junio C Hamano, git
Marius Storm-Olsen schrieb:
> Johannes Sixt said the following on 29.05.2008 08:33:
>> 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.
>
> 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
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 6/5] NUL hack to create_file()
2008-05-29 6:33 ` Johannes Sixt
2008-05-29 7:05 ` Marius Storm-Olsen
@ 2008-05-29 12:39 ` Johannes Schindelin
1 sibling, 0 replies; 20+ messages in thread
From: Johannes Schindelin @ 2008-05-29 12:39 UTC (permalink / raw)
To: Johannes Sixt; +Cc: Junio C Hamano, git
Hi,
On Thu, 29 May 2008, Johannes Sixt wrote:
> 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.
I think that the standard methods, namely checking by hook, should be good
enough.
Ciao,
Dscho
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 6/5] NUL hack to create_file()
2008-05-29 0:17 ` [PATCH 6/5] NUL hack to create_file() Junio C Hamano
2008-05-29 6:33 ` Johannes Sixt
@ 2008-05-29 15:55 ` Daniel Barkalow
2008-05-29 18:26 ` Junio C Hamano
2008-05-29 17:44 ` Alex Riesen
2 siblings, 1 reply; 20+ messages in thread
From: Daniel Barkalow @ 2008-05-29 15:55 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
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?
> ---
> 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;
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
>
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 6/5] NUL hack to create_file()
2008-05-29 7:05 ` Marius Storm-Olsen
2008-05-29 7:23 ` Johannes Sixt
@ 2008-05-29 17:19 ` Daniel Barkalow
2008-05-29 17:51 ` Brian Dessent
1 sibling, 1 reply; 20+ messages in thread
From: Daniel Barkalow @ 2008-05-29 17:19 UTC (permalink / raw)
To: Marius Storm-Olsen; +Cc: Johannes Sixt, Junio C Hamano, git
On Thu, 29 May 2008, Marius Storm-Olsen wrote:
> Johannes Sixt said the following on 29.05.2008 08:33:
> > 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.
>
> 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*
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 6/5] NUL hack to create_file()
2008-05-29 0:17 ` [PATCH 6/5] NUL hack to create_file() Junio C Hamano
2008-05-29 6:33 ` Johannes Sixt
2008-05-29 15:55 ` Daniel Barkalow
@ 2008-05-29 17:44 ` Alex Riesen
2 siblings, 0 replies; 20+ messages in thread
From: Alex Riesen @ 2008-05-29 17:44 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
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.
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 6/5] NUL hack to create_file()
2008-05-29 17:19 ` Daniel Barkalow
@ 2008-05-29 17:51 ` Brian Dessent
2008-05-29 18:35 ` Daniel Barkalow
0 siblings, 1 reply; 20+ messages in thread
From: Brian Dessent @ 2008-05-29 17:51 UTC (permalink / raw)
To: Daniel Barkalow; +Cc: Marius Storm-Olsen, Johannes Sixt, Junio C Hamano, git
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
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 6/5] NUL hack to create_file()
2008-05-29 15:55 ` Daniel Barkalow
@ 2008-05-29 18:26 ` Junio C Hamano
0 siblings, 0 replies; 20+ messages in thread
From: Junio C Hamano @ 2008-05-29 18:26 UTC (permalink / raw)
To: Daniel Barkalow; +Cc: git
Daniel Barkalow <barkalow@iabervon.org> writes:
> 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?
Exactly. I should have mentioned it so that people did not have to waste
their brain cycles pointing out prn and other stuff. Sorry.
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 6/5] NUL hack to create_file()
2008-05-29 17:51 ` Brian Dessent
@ 2008-05-29 18:35 ` Daniel Barkalow
0 siblings, 0 replies; 20+ messages in thread
From: Daniel Barkalow @ 2008-05-29 18:35 UTC (permalink / raw)
To: git; +Cc: Marius Storm-Olsen, Johannes Sixt, Junio C Hamano
On Thu, 29 May 2008, Brian Dessent wrote:
> 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.
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*
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 0/5] "best effort" checkout
2008-05-29 0:17 [PATCH 0/5] "best effort" checkout Junio C Hamano
2008-05-29 0:17 ` [PATCH 1/5] "git checkout -- paths..." should error out when paths cannot be written Junio C Hamano
@ 2008-05-29 23:24 ` Mark Levedahl
2008-05-30 0:33 ` Junio C Hamano
1 sibling, 1 reply; 20+ messages in thread
From: Mark Levedahl @ 2008-05-29 23:24 UTC (permalink / raw)
To: git; +Cc: git
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
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 0/5] "best effort" checkout
2008-05-29 23:24 ` [PATCH 0/5] "best effort" checkout Mark Levedahl
@ 2008-05-30 0:33 ` Junio C Hamano
2008-05-30 1:09 ` Mark Levedahl
0 siblings, 1 reply; 20+ messages in thread
From: Junio C Hamano @ 2008-05-30 0:33 UTC (permalink / raw)
To: Mark Levedahl; +Cc: git
Mark Levedahl <mdl123@verizon.net> writes:
> 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...
I hope you did not use 6/5. My understanding is that your platform
natively supports it without that compatibility layer ;-)
> 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
Interesting breakage.
I expected to see "deleted" here. I guess lstat("anything/nul") says "it
exists" everywhere, and that probably is why you are getting EEXIST.
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 0/5] "best effort" checkout
2008-05-30 0:33 ` Junio C Hamano
@ 2008-05-30 1:09 ` Mark Levedahl
0 siblings, 0 replies; 20+ messages in thread
From: Mark Levedahl @ 2008-05-30 1:09 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
Junio C Hamano wrote:
> I hope you did not use 6/5. My understanding is that your platform
> natively supports it without that compatibility layer ;-)
>
That is a very polite way to express things, and no I did not apply that
patch.
>
> I expected to see "deleted" here. I guess lstat("anything/nul") says "it
> exists" everywhere, and that probably is why you are getting EEXIST.
>
>
Indeed:
git>ls nul
nul
git>ls nul*
ls: cannot access nul*: No such file or directory
So, any test for existence of <path>NUL will pass, but NUL never appears
in a directory listing. The same is true of the other special filenames
under Windows (aux, ...).
Mark
^ permalink raw reply [flat|nested] 20+ messages in thread
end of thread, other threads:[~2008-05-30 1:10 UTC | newest]
Thread overview: 20+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2008-05-29 0:17 [PATCH 0/5] "best effort" checkout Junio C Hamano
2008-05-29 0:17 ` [PATCH 1/5] "git checkout -- paths..." should error out when paths cannot be written Junio C Hamano
2008-05-29 0:17 ` [PATCH 2/5] checkout: make reset_clean_to_new() not die by itself Junio C Hamano
2008-05-29 0:17 ` [PATCH 3/5] checkout: consolidate reset_{to_new,clean_to_new|() Junio C Hamano
2008-05-29 0:17 ` [PATCH 4/5] unpack_trees(): allow callers to differentiate worktree errors from merge errors Junio C Hamano
2008-05-29 0:17 ` [PATCH 5/5] checkout: "best effort" checkout Junio C Hamano
2008-05-29 0:17 ` [PATCH 6/5] NUL hack to create_file() Junio C Hamano
2008-05-29 6:33 ` Johannes Sixt
2008-05-29 7:05 ` Marius Storm-Olsen
2008-05-29 7:23 ` Johannes Sixt
2008-05-29 17:19 ` Daniel Barkalow
2008-05-29 17:51 ` Brian Dessent
2008-05-29 18:35 ` Daniel Barkalow
2008-05-29 12:39 ` Johannes Schindelin
2008-05-29 15:55 ` Daniel Barkalow
2008-05-29 18:26 ` Junio C Hamano
2008-05-29 17:44 ` Alex Riesen
2008-05-29 23:24 ` [PATCH 0/5] "best effort" checkout Mark Levedahl
2008-05-30 0:33 ` Junio C Hamano
2008-05-30 1:09 ` Mark Levedahl
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).