{"thread":{"id":"13716","subject":"[PATCH 1/5] \"git checkout -- paths...\" should error out when paths cannot be written","startedAt":"2008-05-29T00:17:20Z","lastAt":"2008-05-30T01:09:44Z","messageCount":20,"participants":["Junio C Hamano","Johannes Sixt","Marius Storm-Olsen","Johannes Schindelin","Daniel Barkalow","Alex Riesen","Brian Dessent","Mark Levedahl"],"isPatch":true,"patchVersion":1,"patchTotal":5},"messages":[{"id":"77999","messageId":"1212020246-26480-1-git-send-email-gitster@pobox.com","threadId":"13716","inReplyTo":null,"subject":"[PATCH 0/5] \"best effort\" checkout","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-05-29T00:17:20Z","receivedAt":"2008-05-29T00:17:20Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"I consider the first one is not part of the series but a bugfix.\n\nThe remainder is to teach \"git checkout\" not to punt in the middle once it\nhas started touching the work tree.\n\nIt does _not_ attempt to autorename a file whose name is NUL to something\nelse.  Partly because I personally think that sort of magic (or \"a cute\nhack\") makes the system unnecessarily complex and fragile while not adding\nmuch to the usability, but more importantly because I do not think we are\nready to adopt that kind of complexity yet, before fixing more basic issue\nlike this series addresses.\n\n[PATCH 1/5] \"git checkout -- paths...\" should error out when paths cannot be written\n[PATCH 2/5] checkout: make reset_clean_to_new() not die by itself\n[PATCH 3/5] checkout: consolidate reset_{to_new,clean_to_new|()\n[PATCH 4/5] unpack_trees(): allow callers to differentiate worktree errors from merge errors\n[PATCH 5/5] checkout: \"best effort\" checkout\n[PATCH 6/5] NUL hack to create_file()\n"},{"id":"77998","messageId":"1212020246-26480-2-git-send-email-gitster@pobox.com","threadId":"13716","inReplyTo":"1212020246-26480-1-git-send-email-gitster@pobox.com","subject":"[PATCH 1/5] \"git checkout -- paths...\" should error out when paths cannot be written","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-05-29T00:17:21Z","receivedAt":"2008-05-29T00:17:21Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"When \"git checkout -- paths...\" cannot update work tree for whatever\nreason, checkout_entry() correctly issued an error message for the path to\nthe end user, but the command ignored the error, causing the entire\ncommand to succeed.  This fixes it.\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n builtin-checkout.c |    7 +++++--\n 1 files changed, 5 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin-checkout.c b/builtin-checkout.c\nindex 1ea017f..00dc8ca 100644\n--- a/builtin-checkout.c\n+++ b/builtin-checkout.c\n@@ -84,6 +84,7 @@ static int checkout_paths(struct tree *source_tree, const char **pathspec)\n \tunsigned char rev[20];\n \tint flag;\n \tstruct commit *head;\n+\tint errs = 0;\n \n \tint newfd;\n \tstruct lock_file *lock_file = xcalloc(1, sizeof(struct lock_file));\n@@ -106,13 +107,14 @@ static int checkout_paths(struct tree *source_tree, const char **pathspec)\n \tif (report_path_error(ps_matched, pathspec, 0))\n \t\treturn 1;\n \n+\t/* Now we are committed to check them out */\n \tmemset(&state, 0, sizeof(state));\n \tstate.force = 1;\n \tstate.refresh_cache = 1;\n \tfor (pos = 0; pos < active_nr; pos++) {\n \t\tstruct cache_entry *ce = active_cache[pos];\n \t\tif (pathspec_match(pathspec, NULL, ce->name, 0)) {\n-\t\t\tcheckout_entry(ce, &state, NULL);\n+\t\t\terrs |= checkout_entry(ce, &state, NULL);\n \t\t}\n \t}\n \n@@ -123,7 +125,8 @@ static int checkout_paths(struct tree *source_tree, const char **pathspec)\n \tresolve_ref(\"HEAD\", rev, 0, &flag);\n \thead = lookup_commit_reference_gently(rev, 1);\n \n-\treturn post_checkout_hook(head, head, 0);\n+\terrs |= post_checkout_hook(head, head, 0);\n+\treturn errs;\n }\n \n static void show_local_changes(struct object *head)\n-- \n1.5.6.rc0.43.g823ea\n"},{"id":"78004","messageId":"1212020246-26480-3-git-send-email-gitster@pobox.com","threadId":"13716","inReplyTo":"1212020246-26480-2-git-send-email-gitster@pobox.com","subject":"[PATCH 2/5] checkout: make reset_clean_to_new() not die by itself","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-05-29T00:17:22Z","receivedAt":"2008-05-29T00:17:22Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Instead, have its error percolate up through the callchain and let it be\nthe exit status of the main command.  No semantic changes yet.\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n builtin-checkout.c |    9 ++++++---\n 1 files changed, 6 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin-checkout.c b/builtin-checkout.c\nindex 00dc8ca..cc97724 100644\n--- a/builtin-checkout.c\n+++ b/builtin-checkout.c\n@@ -172,7 +172,7 @@ static int reset_to_new(struct tree *tree, int quiet)\n \treturn 0;\n }\n \n-static void reset_clean_to_new(struct tree *tree, int quiet)\n+static int reset_clean_to_new(struct tree *tree, int quiet)\n {\n \tstruct unpack_trees_options opts;\n \tstruct tree_desc tree_desc;\n@@ -189,7 +189,8 @@ static void reset_clean_to_new(struct tree *tree, int quiet)\n \tparse_tree(tree);\n \tinit_tree_desc(&tree_desc, tree->buffer, tree->size);\n \tif (unpack_trees(1, &tree_desc, &opts))\n-\t\texit(128);\n+\t\treturn 128;\n+\treturn 0;\n }\n \n struct checkout_opts {\n@@ -295,7 +296,9 @@ static int merge_working_tree(struct checkout_opts *opts,\n \t\t\t\treturn ret;\n \t\t\tmerge_trees(new->commit->tree, work, old->commit->tree,\n \t\t\t\t    new->name, \"local\", &result);\n-\t\t\treset_clean_to_new(new->commit->tree, opts->quiet);\n+\t\t\tret = reset_clean_to_new(new->commit->tree, opts->quiet);\n+\t\t\tif (ret)\n+\t\t\t\treturn ret;\n \t\t}\n \t}\n \n-- \n1.5.6.rc0.43.g823ea\n"},{"id":"78000","messageId":"1212020246-26480-4-git-send-email-gitster@pobox.com","threadId":"13716","inReplyTo":"1212020246-26480-3-git-send-email-gitster@pobox.com","subject":"[PATCH 3/5] checkout: consolidate reset_{to_new,clean_to_new|()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-05-29T00:17:23Z","receivedAt":"2008-05-29T00:17:23Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"These two were very similar functions with only tiny bit of difference.\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n * This may be a bit hard to read but \"struct checkout_opts\" is moved up\n   at the same time as it is passed to the consolidated function as its\n   parameter.\n\n builtin-checkout.c |   50 +++++++++++++++-----------------------------------\n 1 files changed, 15 insertions(+), 35 deletions(-)\n\ndiff --git a/builtin-checkout.c b/builtin-checkout.c\nindex cc97724..9af5197 100644\n--- a/builtin-checkout.c\n+++ b/builtin-checkout.c\n@@ -151,39 +151,29 @@ static void describe_detached_head(char *msg, struct commit *commit)\n \tstrbuf_release(&sb);\n }\n \n-static int reset_to_new(struct tree *tree, int quiet)\n-{\n-\tstruct unpack_trees_options opts;\n-\tstruct tree_desc tree_desc;\n-\n-\tmemset(&opts, 0, sizeof(opts));\n-\topts.head_idx = -1;\n-\topts.update = 1;\n-\topts.reset = 1;\n-\topts.merge = 1;\n-\topts.fn = oneway_merge;\n-\topts.verbose_update = !quiet;\n-\topts.src_index = &the_index;\n-\topts.dst_index = &the_index;\n-\tparse_tree(tree);\n-\tinit_tree_desc(&tree_desc, tree->buffer, tree->size);\n-\tif (unpack_trees(1, &tree_desc, &opts))\n-\t\treturn 128;\n-\treturn 0;\n-}\n+struct checkout_opts {\n+\tint quiet;\n+\tint merge;\n+\tint force;\n+\n+\tchar *new_branch;\n+\tint new_branch_log;\n+\tenum branch_track track;\n+};\n \n-static int reset_clean_to_new(struct tree *tree, int quiet)\n+static int reset_tree(struct tree *tree, struct checkout_opts *o, int worktree)\n {\n \tstruct unpack_trees_options opts;\n \tstruct tree_desc tree_desc;\n \n \tmemset(&opts, 0, sizeof(opts));\n \topts.head_idx = -1;\n-\topts.skip_unmerged = 1;\n+\topts.update = worktree;\n+\topts.skip_unmerged = !worktree;\n \topts.reset = 1;\n \topts.merge = 1;\n \topts.fn = oneway_merge;\n-\topts.verbose_update = !quiet;\n+\topts.verbose_update = !o->quiet;\n \topts.src_index = &the_index;\n \topts.dst_index = &the_index;\n \tparse_tree(tree);\n@@ -193,16 +183,6 @@ static int reset_clean_to_new(struct tree *tree, int quiet)\n \treturn 0;\n }\n \n-struct checkout_opts {\n-\tint quiet;\n-\tint merge;\n-\tint force;\n-\n-\tchar *new_branch;\n-\tint new_branch_log;\n-\tenum branch_track track;\n-};\n-\n struct branch_info {\n \tconst char *name; /* The short name used */\n \tconst char *path; /* The full name of a real branch */\n@@ -227,7 +207,7 @@ static int merge_working_tree(struct checkout_opts *opts,\n \tread_cache();\n \n \tif (opts->force) {\n-\t\tret = reset_to_new(new->commit->tree, opts->quiet);\n+\t\tret = reset_tree(new->commit->tree, opts, 1);\n \t\tif (ret)\n \t\t\treturn ret;\n \t} else {\n@@ -291,12 +271,12 @@ static int merge_working_tree(struct checkout_opts *opts,\n \t\t\tadd_files_to_cache(NULL, NULL, 0);\n \t\t\twork = write_tree_from_memory();\n \n-\t\t\tret = reset_to_new(new->commit->tree, opts->quiet);\n+\t\t\tret = reset_tree(new->commit->tree, opts, 1);\n \t\t\tif (ret)\n \t\t\t\treturn ret;\n \t\t\tmerge_trees(new->commit->tree, work, old->commit->tree,\n \t\t\t\t    new->name, \"local\", &result);\n-\t\t\tret = reset_clean_to_new(new->commit->tree, opts->quiet);\n+\t\t\tret = reset_tree(new->commit->tree, opts, 0);\n \t\t\tif (ret)\n \t\t\t\treturn ret;\n \t\t}\n-- \n1.5.6.rc0.43.g823ea\n"},{"id":"78002","messageId":"1212020246-26480-5-git-send-email-gitster@pobox.com","threadId":"13716","inReplyTo":"1212020246-26480-4-git-send-email-gitster@pobox.com","subject":"[PATCH 4/5] unpack_trees(): allow callers to differentiate worktree errors from merge errors","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-05-29T00:17:24Z","receivedAt":"2008-05-29T00:17:24Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Instead of uniformly returning -1 on any error, this teaches\nunpack_trees() to return -2 when the merge itself is Ok but worktree\nrefuses to get updated.\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n unpack-trees.c |   10 +++++++---\n 1 files changed, 7 insertions(+), 3 deletions(-)\n\ndiff --git a/unpack-trees.c b/unpack-trees.c\nindex 0de5a31..cba0aca 100644\n--- a/unpack-trees.c\n+++ b/unpack-trees.c\n@@ -358,8 +358,13 @@ static int unpack_failed(struct unpack_trees_options *o, const char *message)\n \treturn -1;\n }\n \n+/*\n+ * N-way merge \"len\" trees.  Returns 0 on success, -1 on failure to manipulate the\n+ * resulting index, -2 on failure to reflect the changes to the work tree.\n+ */\n int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options *o)\n {\n+\tint ret;\n \tstatic struct cache_entry *dfc;\n \n \tif (len > MAX_UNPACK_TREES)\n@@ -404,11 +409,10 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options\n \t\treturn unpack_failed(o, \"Merge requires file-level merging\");\n \n \to->src_index = NULL;\n-\tif (check_updates(o))\n-\t\treturn -1;\n+\tret = check_updates(o) ? (-2) : 0;\n \tif (o->dst_index)\n \t\t*o->dst_index = o->result;\n-\treturn 0;\n+\treturn ret;\n }\n \n /* Here come the merge functions */\n-- \n1.5.6.rc0.43.g823ea\n"},{"id":"78001","messageId":"1212020246-26480-6-git-send-email-gitster@pobox.com","threadId":"13716","inReplyTo":"1212020246-26480-5-git-send-email-gitster@pobox.com","subject":"[PATCH 5/5] checkout: \"best effort\" checkout","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-05-29T00:17:25Z","receivedAt":"2008-05-29T00:17:25Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"When unpack_trees() returned an error while switching branches, we used to\nstop right there, exiting without writing the index out or switching HEAD.\n\nThis is Ok when unpack_trees() detected a locally modified paths or\nuntracked files that could be overwritten by branch switching, but it is\nundesirable if unpack_trees() already committed to update the work tree\nand a failure is returned because some but not all paths are updated\n(perhaps a directory that some files need to go in was made read-only by\nmistake, or a file that will be overwritten by branch switching had a\nmandatory lock on it and we could not unlink).\n\nThis changes the behaviour upon such an error to complete the branch\nswitching; the files updated in the work tree will hopefully much more\nconsistent with the index and HEAD derived from the switched-to branch.\n\nWe still issue error messages, and exit the command with non-zero status,\nso scripted callers need to notice it.\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n builtin-checkout.c |   22 ++++++++++++++++++----\n 1 files changed, 18 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin-checkout.c b/builtin-checkout.c\nindex 9af5197..93ea69b 100644\n--- a/builtin-checkout.c\n+++ b/builtin-checkout.c\n@@ -155,6 +155,7 @@ struct checkout_opts {\n \tint quiet;\n \tint merge;\n \tint force;\n+\tint writeout_error;\n \n \tchar *new_branch;\n \tint new_branch_log;\n@@ -178,9 +179,20 @@ static int reset_tree(struct tree *tree, struct checkout_opts *o, int worktree)\n \topts.dst_index = &the_index;\n \tparse_tree(tree);\n \tinit_tree_desc(&tree_desc, tree->buffer, tree->size);\n-\tif (unpack_trees(1, &tree_desc, &opts))\n+\tswitch (unpack_trees(1, &tree_desc, &opts)) {\n+\tcase -2:\n+\t\to->writeout_error = 1;\n+\t\t/*\n+\t\t * We return 0 nevertheless, as the index is all right\n+\t\t * and more importantly we have made best efforts to\n+\t\t * update paths in the work tree, and we cannot revert\n+\t\t * them.\n+\t\t */\n+\tcase 0:\n+\t\treturn 0;\n+\tdefault:\n \t\treturn 128;\n-\treturn 0;\n+\t}\n }\n \n struct branch_info {\n@@ -243,7 +255,8 @@ static int merge_working_tree(struct checkout_opts *opts,\n \t\ttree = parse_tree_indirect(new->commit->object.sha1);\n \t\tinit_tree_desc(&trees[1], tree->buffer, tree->size);\n \n-\t\tif (unpack_trees(2, trees, &topts)) {\n+\t\tret = unpack_trees(2, trees, &topts);\n+\t\tif (ret == -1) {\n \t\t\t/*\n \t\t\t * Unpack couldn't do a trivial merge; either\n \t\t\t * give up or do a real merge, depending on\n@@ -478,7 +491,8 @@ static int switch_branches(struct checkout_opts *opts, struct branch_info *new)\n \n \tupdate_refs_for_switch(opts, &old, new);\n \n-\treturn post_checkout_hook(old.commit, new->commit, 1);\n+\tret = post_checkout_hook(old.commit, new->commit, 1);\n+\treturn ret || opts->writeout_error;\n }\n \n int cmd_checkout(int argc, const char **argv, const char *prefix)\n-- \n1.5.6.rc0.43.g823ea\n"},{"id":"78003","messageId":"1212020246-26480-7-git-send-email-gitster@pobox.com","threadId":"13716","inReplyTo":"1212020246-26480-6-git-send-email-gitster@pobox.com","subject":"[PATCH 6/5] NUL hack to create_file()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-05-29T00:17:26Z","receivedAt":"2008-05-29T00:17:26Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"This is not meant for application to the mainline.  It allows your git to\nrefuse to create a blob whose name is \"nul\".\n\n---\n entry.c |    8 ++++++++\n 1 files changed, 8 insertions(+), 0 deletions(-)\n\ndiff --git a/entry.c b/entry.c\nindex 222aaa3..d24b803 100644\n--- a/entry.c\n+++ b/entry.c\n@@ -81,6 +81,14 @@ static void remove_subtree(const char *path)\n \n static int create_file(const char *path, unsigned int mode)\n {\n+\tif (1) {\n+\t\tsize_t len = strlen(path);\n+\t\tif (3 <= len && !strcmp(path + len - 3, \"nul\") &&\n+\t\t    (3 == len || path[len - 4] == '/')) {\n+\t\t\terrno = EPERM;\n+\t\t\treturn -1;\n+\t\t}\n+\t}\n \tmode = (mode & 0100) ? 0777 : 0666;\n \treturn open(path, O_WRONLY | O_CREAT | O_EXCL, mode);\n }\n-- \n1.5.6.rc0.43.g823ea\n"},{"id":"78020","messageId":"483E4E3C.90805@viscovery.net","threadId":"13716","inReplyTo":"1212020246-26480-7-git-send-email-gitster@pobox.com","subject":"Re: [PATCH 6/5] NUL hack to create_file()","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2008-05-29T06:33:32Z","receivedAt":"2008-05-29T06:33:32Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Junio C Hamano schrieb:\n> This is not meant for application to the mainline.  It allows your git to\n> refuse to create a blob whose name is \"nul\".\n\nIt's not just about \"nul\"; these won't work either: \"aux\", \"prn\", \"con\",\n\"com\\d+\", \"lpt\\d+\", neither do \"$one_of_these.$some_extension\". And all of\nthat regardless of the case!\n\nSee http://msdn.microsoft.com/en-us/library/aa365247(VS.85).aspx\n\nDefinitely, we don't ever want to have such special-casing somewhere in git.\n\n-- Hannes\n"},{"id":"78021","messageId":"483E55C1.1000900@trolltech.com","threadId":"13716","inReplyTo":"483E4E3C.90805@viscovery.net","subject":"Re: [PATCH 6/5] NUL hack to create_file()","fromName":"Marius Storm-Olsen","fromEmail":"marius@trolltech.com","sentAt":"2008-05-29T07:05:37Z","receivedAt":"2008-05-29T07:05:37Z","isPatch":true,"sender":{"key":"marius@trolltech.com","avatar":"https://gravatar.com/avatar/a40071d8f651862c6ab10bd7996f0ad84d94f06c0399de9e3fa4f06beb390a71?d=mp&s=160"},"body":"Johannes Sixt said the following on 29.05.2008 08:33:\n> Junio C Hamano schrieb:\n>> This is not meant for application to the mainline.  It allows your git to\n>> refuse to create a blob whose name is \"nul\".\n> \n> It's not just about \"nul\"; these won't work either: \"aux\", \"prn\", \"con\",\n> \"com\\d+\", \"lpt\\d+\", neither do \"$one_of_these.$some_extension\". And all of\n> that regardless of the case!\n> \n> See http://msdn.microsoft.com/en-us/library/aa365247(VS.85).aspx\n> \n> Definitely, we don't ever want to have such special-casing somewhere in git.\n\nThey _can_ be used by using the UNC notation:\n     \\\\?\\<drive letter>:\\<path>\\nul\nDo you think we should special-case that, or simply fail?\n\n( 9:04:57 - D:\\home\\marius\\source\\hg\\test)\n > hg checkout\nabort: The parameter is incorrect: D:\\home\\marius\\source\\hg\\test\\nul\n\n'Nuff said? :-)\n\n-- \n.marius [@trolltech.com]\n'if you know what you're doing, it's not research'\n\n"},{"id":"78022","messageId":"483E59FE.80707@viscovery.net","threadId":"13716","inReplyTo":"483E55C1.1000900@trolltech.com","subject":"Re: [PATCH 6/5] NUL hack to create_file()","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2008-05-29T07:23:42Z","receivedAt":"2008-05-29T07:23:42Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Marius Storm-Olsen schrieb:\n> Johannes Sixt said the following on 29.05.2008 08:33:\n>> Junio C Hamano schrieb:\n>>> This is not meant for application to the mainline.  It allows your\n>>> git to\n>>> refuse to create a blob whose name is \"nul\".\n>>\n>> It's not just about \"nul\"; these won't work either: \"aux\", \"prn\", \"con\",\n>> \"com\\d+\", \"lpt\\d+\", neither do \"$one_of_these.$some_extension\". And\n>> all of\n>> that regardless of the case!\n>>\n>> See http://msdn.microsoft.com/en-us/library/aa365247(VS.85).aspx\n>>\n>> Definitely, we don't ever want to have such special-casing somewhere\n>> in git.\n> \n> They _can_ be used by using the UNC notation:\n>     \\\\?\\<drive letter>:\\<path>\\nul\n> Do you think we should special-case that, or simply fail?\n\nRhetoric question: What's so special about those files?\n\n\"foo/nul\" is a file you don't have permissions to write to. Period. We\nshould fail the same way as if you had 'chmod a-w foo/nul foo', or as if\nthere's a bad sector on the disk. Junio's patch series is the way to go\n(without 6/5, of course).\n\n-- Hannes\n"},{"id":"78033","messageId":"alpine.DEB.1.00.0805291338370.13507@racer.site.net","threadId":"13716","inReplyTo":"483E4E3C.90805@viscovery.net","subject":"Re: [PATCH 6/5] NUL hack to create_file()","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2008-05-29T12:39:21Z","receivedAt":"2008-05-29T12:39:21Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Thu, 29 May 2008, Johannes Sixt wrote:\n\n> Junio C Hamano schrieb:\n> > This is not meant for application to the mainline.  It allows your git \n> > to refuse to create a blob whose name is \"nul\".\n> \n> It's not just about \"nul\"; these won't work either: \"aux\", \"prn\", \"con\", \n> \"com\\d+\", \"lpt\\d+\", neither do \"$one_of_these.$some_extension\". And all \n> of that regardless of the case!\n> \n> See http://msdn.microsoft.com/en-us/library/aa365247(VS.85).aspx\n> \n> Definitely, we don't ever want to have such special-casing somewhere in \n> git.\n\nI think that the standard methods, namely checking by hook, should be good \nenough.\n\nCiao,\nDscho\n"},{"id":"78054","messageId":"alpine.LNX.1.00.0805291145230.19665@iabervon.org","threadId":"13716","inReplyTo":"1212020246-26480-7-git-send-email-gitster@pobox.com","subject":"Re: [PATCH 6/5] NUL hack to create_file()","fromName":"Daniel Barkalow","fromEmail":"barkalow@iabervon.org","sentAt":"2008-05-29T15:55:56Z","receivedAt":"2008-05-29T15:55:56Z","isPatch":true,"sender":{"key":"barkalow@iabervon.org","avatar":"https://avatars.githubusercontent.com/u/55364219?v=4"},"body":"On Wed, 28 May 2008, Junio C Hamano wrote:\n\n> This is not meant for application to the mainline.  It allows your git to\n> refuse to create a blob whose name is \"nul\".\n\nI assume this is so you can test git's response to a defective filesystem \nwithout actually having a defective filesystem?\n\n> ---\n>  entry.c |    8 ++++++++\n>  1 files changed, 8 insertions(+), 0 deletions(-)\n> \n> diff --git a/entry.c b/entry.c\n> index 222aaa3..d24b803 100644\n> --- a/entry.c\n> +++ b/entry.c\n> @@ -81,6 +81,14 @@ static void remove_subtree(const char *path)\n>  \n>  static int create_file(const char *path, unsigned int mode)\n>  {\n> +\tif (1) {\n> +\t\tsize_t len = strlen(path);\n> +\t\tif (3 <= len && !strcmp(path + len - 3, \"nul\") &&\n> +\t\t    (3 == len || path[len - 4] == '/')) {\n> +\t\t\terrno = EPERM;\n\nShouldn't this be EEXIST? I think the issue is that the first exists for \nthe purpose of open() but not for anything else we've done up to this \npoint.\n\n> +\t\t\treturn -1;\n> +\t\t}\n> +\t}\n>  \tmode = (mode & 0100) ? 0777 : 0666;\n>  \treturn open(path, O_WRONLY | O_CREAT | O_EXCL, mode);\n>  }\n> -- \n> 1.5.6.rc0.43.g823ea\n> \n> --\n> To unsubscribe from this list: send the line \"unsubscribe git\" in\n> the body of a message to majordomo@vger.kernel.org\n> More majordomo info at  http://vger.kernel.org/majordomo-info.html\n> \n"},{"id":"78063","messageId":"alpine.LNX.1.00.0805291157330.19665@iabervon.org","threadId":"13716","inReplyTo":"483E55C1.1000900@trolltech.com","subject":"Re: [PATCH 6/5] NUL hack to create_file()","fromName":"Daniel Barkalow","fromEmail":"barkalow@iabervon.org","sentAt":"2008-05-29T17:19:23Z","receivedAt":"2008-05-29T17:19:23Z","isPatch":true,"sender":{"key":"barkalow@iabervon.org","avatar":"https://avatars.githubusercontent.com/u/55364219?v=4"},"body":"On Thu, 29 May 2008, Marius Storm-Olsen wrote:\n\n> Johannes Sixt said the following on 29.05.2008 08:33:\n> > Junio C Hamano schrieb:\n> > > This is not meant for application to the mainline.  It allows your git to\n> > > refuse to create a blob whose name is \"nul\".\n> > \n> > It's not just about \"nul\"; these won't work either: \"aux\", \"prn\", \"con\",\n> > \"com\\d+\", \"lpt\\d+\", neither do \"$one_of_these.$some_extension\". And all of\n> > that regardless of the case!\n> > \n> > See http://msdn.microsoft.com/en-us/library/aa365247(VS.85).aspx\n> > \n> > Definitely, we don't ever want to have such special-casing somewhere in git.\n> \n> They _can_ be used by using the UNC notation:\n>     \\\\?\\<drive letter>:\\<path>\\nul\n> Do you think we should special-case that, or simply fail?\n\nPerhaps we should see if we can get an open() that always uses that \nnotation for the actual system call? I doubt we want to support the \nDOS-ish meanings even if the user provides them as input sources. If it's \nnot actually a problem with the underlying storage mechanism, but rather a \nflaw in the POSIX implementation for Windows, we should fix that (or do \nsomething in compat to work around it) instead of failing in any way to \nsupport it in git. Of course, people on Windows using projects with these \nfilenames will probably run into problems with other tools, but at least \ngit will behave properly.\n\nOn the other hand, I bet there are going to be real issues with filenames \nwith backslashes in them.\n\n\t-Daniel\n*This .sig left intentionally blank*\n"},{"id":"78068","messageId":"20080529174400.GA5596@steel.home","threadId":"13716","inReplyTo":"1212020246-26480-7-git-send-email-gitster@pobox.com","subject":"Re: [PATCH 6/5] NUL hack to create_file()","fromName":"Alex Riesen","fromEmail":"raa.lkml@gmail.com","sentAt":"2008-05-29T17:44:00Z","receivedAt":"2008-05-29T17:44:00Z","isPatch":true,"sender":{"key":"raa.lkml@gmail.com","avatar":"https://avatars.githubusercontent.com/u/324101?v=4"},"body":"Junio C Hamano, Thu, May 29, 2008 02:17:26 +0200:\n> This is not meant for application to the mainline. ...\n\nThat's good. No one besides Windows has such a stupid filesystem.\n"},{"id":"78072","messageId":"483EED1D.58196FCF@dessent.net","threadId":"13716","inReplyTo":"alpine.LNX.1.00.0805291157330.19665@iabervon.org","subject":"Re: [PATCH 6/5] NUL hack to create_file()","fromName":"Brian Dessent","fromEmail":"brian@dessent.net","sentAt":"2008-05-29T17:51:25Z","receivedAt":"2008-05-29T17:51:25Z","isPatch":true,"sender":{"key":"brian@dessent.net","avatar":null},"body":"Daniel Barkalow wrote:\n\n> support it in git. Of course, people on Windows using projects with these\n> filenames will probably run into problems with other tools, but at least\n> git will behave properly.\n\nI don't see how it would help to have core git using the Native syntax\nto bypass the Win32 layer's restrictions but none of the accompanying\nsuite of tools, i.e. the dozens of various MSYS sh.exe, perl.exe,\ncat.exe, etc.  None of those would be able to open or even delete those\nfiles with the reserved filenames.\n\nUsers tend to get upset when software creates files that cannot be\nremoved through conventional methods, e.g. Explorer is completely\npowerless to remove it.  Cygwin shipped with a bug several years ago\nthat unintentionally allowed to create (but not unlink) reserved\nfilenames.  Unless you knew the magical incantation of \"del\n\\\\.\\c:\\path\\to\\nul\" the file was immutable.\n\nBrian\n"},{"id":"78073","messageId":"7v8wxtotkh.fsf@gitster.siamese.dyndns.org","threadId":"13716","inReplyTo":"alpine.LNX.1.00.0805291145230.19665@iabervon.org","subject":"Re: [PATCH 6/5] NUL hack to create_file()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-05-29T18:26:22Z","receivedAt":"2008-05-29T18:26:22Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Daniel Barkalow <barkalow@iabervon.org> writes:\n\n> On Wed, 28 May 2008, Junio C Hamano wrote:\n>\n>> This is not meant for application to the mainline.  It allows your git to\n>> refuse to create a blob whose name is \"nul\".\n>\n> I assume this is so you can test git's response to a defective filesystem \n> without actually having a defective filesystem?\n\nExactly.  I should have mentioned it so that people did not have to waste\ntheir brain cycles pointing out prn and other stuff.  Sorry.\n"},{"id":"78075","messageId":"alpine.LNX.1.00.0805291428130.19665@iabervon.org","threadId":"13716","inReplyTo":"483EED1D.58196FCF@dessent.net","subject":"Re: [PATCH 6/5] NUL hack to create_file()","fromName":"Daniel Barkalow","fromEmail":"barkalow@iabervon.org","sentAt":"2008-05-29T18:35:03Z","receivedAt":"2008-05-29T18:35:03Z","isPatch":true,"sender":{"key":"barkalow@iabervon.org","avatar":"https://avatars.githubusercontent.com/u/55364219?v=4"},"body":"On Thu, 29 May 2008, Brian Dessent wrote:\n\n> Daniel Barkalow wrote:\n> \n> > support it in git. Of course, people on Windows using projects with these\n> > filenames will probably run into problems with other tools, but at least\n> > git will behave properly.\n> \n> I don't see how it would help to have core git using the Native syntax\n> to bypass the Win32 layer's restrictions but none of the accompanying\n> suite of tools, i.e. the dozens of various MSYS sh.exe, perl.exe,\n> cat.exe, etc.  None of those would be able to open or even delete those\n> files with the reserved filenames.\n> \n> Users tend to get upset when software creates files that cannot be\n> removed through conventional methods, e.g. Explorer is completely\n> powerless to remove it.  Cygwin shipped with a bug several years ago\n> that unintentionally allowed to create (but not unlink) reserved\n> filenames.  Unless you knew the magical incantation of \"del\n> \\\\.\\c:\\path\\to\\nul\" the file was immutable.\n\nWell, \"git rm <filename>\" would work. Or \"git mv <weird> <okay>\", which is \npossibly more productive. Or checking out a version that doesn't contain \nit. It's a lot worse if the tool that created it can't remove it than if \ntools other than the one that created it can't deal with it.\n\n\t-Daniel\n*This .sig left intentionally blank*\n"},{"id":"78094","messageId":"483F3B32.9000907@verizon.net","threadId":"13716","inReplyTo":"1212020246-26480-1-git-send-email-gitster@pobox.com","subject":"Re: [PATCH 0/5] \"best effort\" checkout","fromName":"Mark Levedahl","fromEmail":"mdl123@verizon.net","sentAt":"2008-05-29T23:24:34Z","receivedAt":"2008-05-29T23:24:34Z","isPatch":true,"sender":{"key":"mdl123@verizon.net","avatar":"https://avatars.githubusercontent.com/u/5302462?v=4"},"body":"Junio C Hamano wrote:\n> \t\n> [PATCH 1/5] \"git checkout -- paths...\" should error out when paths cannot be written\n> [PATCH 2/5] checkout: make reset_clean_to_new() not die by itself\n> [PATCH 3/5] checkout: consolidate reset_{to_new,clean_to_new|()\n> [PATCH 4/5] unpack_trees(): allow callers to differentiate worktree errors from merge errors\n> [PATCH 5/5] checkout: \"best effort\" checkout\n> [PATCH 6/5] NUL hack to create_file()\n\nThis works! I've added these patches (pulled from pu) to my tree and rebuilt. \nThe current results on Cygwin...\n\ngit>git checkout -f b71ce7f3f13ebd0e\nPrevious HEAD position was 952538f... checkout: \"best effort\" checkout\nerror: git-checkout-index: unable to create file t/t5100/nul (File exists)\nHEAD is now at b71ce7f... Merge 1.5.5.3 in\ngit>git status\n# Not currently on any branch.\n# Changed but not updated:\n#   (use \"git add <file>...\" to update what will be committed)\n#\n#   modified:   t/t5100/nul\n#\nno changes added to commit (use \"git add\" and/or \"git commit -a\")\ngit>git mv  t/t5100/nul t/t5100/nul-plain\nfatal: renaming t/t5100/nul failed: Invalid argument\ngit>git rm -f  --cached t/t5100/nul\nrm 't/t5100/nul'\ngit>git show HEAD:t/t5100/nul\n From nobody Mon Sep 17 00:00:00 2001\n\n---\ndiff --git a/foo b/foo\n^Some strange test^^\n^@\n\nSo, for posterity, git-mv cannot rename the offending file in the index, but the \nfile can be removed, and its contents piped into a file of non-offending name, \nso a reasonable solution for this case exists.\n\nMany thanks to all, especially to Junio for actually creating the fix.\n\nMark\n"},{"id":"78103","messageId":"7vabi8ocju.fsf@gitster.siamese.dyndns.org","threadId":"13716","inReplyTo":"483F3B32.9000907@verizon.net","subject":"Re: [PATCH 0/5] \"best effort\" checkout","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-05-30T00:33:57Z","receivedAt":"2008-05-30T00:33:57Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Mark Levedahl <mdl123@verizon.net> writes:\n\n> Junio C Hamano wrote:\n>> \t\n>> [PATCH 1/5] \"git checkout -- paths...\" should error out when paths cannot be written\n>> [PATCH 2/5] checkout: make reset_clean_to_new() not die by itself\n>> [PATCH 3/5] checkout: consolidate reset_{to_new,clean_to_new|()\n>> [PATCH 4/5] unpack_trees(): allow callers to differentiate worktree errors from merge errors\n>> [PATCH 5/5] checkout: \"best effort\" checkout\n>> [PATCH 6/5] NUL hack to create_file()\n>\n> This works! I've added these patches (pulled from pu) to my tree and\n> rebuilt. The current results on Cygwin...\n\nI hope you did not use 6/5.  My understanding is that your platform\nnatively supports it without that compatibility layer ;-)\n\n> git>git checkout -f b71ce7f3f13ebd0e\n> Previous HEAD position was 952538f... checkout: \"best effort\" checkout\n> error: git-checkout-index: unable to create file t/t5100/nul (File exists)\n> HEAD is now at b71ce7f... Merge 1.5.5.3 in\n> git>git status\n> # Not currently on any branch.\n> # Changed but not updated:\n> #   (use \"git add <file>...\" to update what will be committed)\n> #\n> #   modified:   t/t5100/nul\n\nInteresting breakage.\n\nI expected to see \"deleted\" here.  I guess lstat(\"anything/nul\") says \"it\nexists\" everywhere, and that probably is why you are getting EEXIST.\n"},{"id":"78104","messageId":"483F53D8.1020201@gmail.com","threadId":"13716","inReplyTo":"7vabi8ocju.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH 0/5] \"best effort\" checkout","fromName":"Mark Levedahl","fromEmail":"mlevedahl@gmail.com","sentAt":"2008-05-30T01:09:44Z","receivedAt":"2008-05-30T01:09:44Z","isPatch":true,"sender":{"key":"mdl123@verizon.net","avatar":"https://avatars.githubusercontent.com/u/5302462?v=4"},"body":"Junio C Hamano wrote:\n> I hope you did not use 6/5.  My understanding is that your platform\n> natively supports it without that compatibility layer ;-)\n>   \nThat is a very polite way to express things, and no I did not apply that \npatch.\n>\n> I expected to see \"deleted\" here.  I guess lstat(\"anything/nul\") says \"it\n> exists\" everywhere, and that probably is why you are getting EEXIST.\n>\n>   \nIndeed:\n\ngit>ls nul\nnul\ngit>ls nul*\nls: cannot access nul*: No such file or directory\n\nSo, any test for existence of <path>NUL will pass, but NUL never appears \nin a directory listing. The same is true of the other special filenames \nunder Windows (aux, ...).\n\nMark\n"}]}