{"thread":{"id":"20719","subject":"Possible regression: overwriting untracked files in a fresh repo","startedAt":"2009-08-24T16:31:21Z","lastAt":"2009-08-25T11:34:08Z","messageCount":9,"participants":["Johannes Schindelin","Jeff King","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"121648","messageId":"alpine.DEB.1.00.0908241829510.11375@intel-tinevez-2-302","threadId":"20719","inReplyTo":null,"subject":"Possible regression: overwriting untracked files in a fresh repo","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2009-08-24T16:31:21Z","receivedAt":"2009-08-24T16:31:21Z","isPatch":false,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nI _think_ that this used to complain about untracked files being \noverwritten:\n\n\t$ git init\n\t$ git remote add -f origin <url>\n\t$ git checkout -b blub origin/master\n\nIt does not do that here (any longer, IIAC).  Intended?\n\nCiao,\nDscho\n"},{"id":"121663","messageId":"20090824190710.GB25168@coredump.intra.peff.net","threadId":"20719","inReplyTo":"alpine.DEB.1.00.0908241829510.11375@intel-tinevez-2-302","subject":"Re: Possible regression: overwriting untracked files in a fresh repo","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2009-08-24T19:07:10Z","receivedAt":"2009-08-24T19:07:10Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Aug 24, 2009 at 06:31:21PM +0200, Johannes Schindelin wrote:\n\n> I _think_ that this used to complain about untracked files being \n> overwritten:\n> \n> \t$ git init\n> \t$ git remote add -f origin <url>\n> \t$ git checkout -b blub origin/master\n> \n> It does not do that here (any longer, IIAC).  Intended?\n\nI agree that it probably _should_ complain, but I don't think it ever\ndid. I tried a handful of released versions as far back as v1.4.4, and\nall of them overwrite local files without complaining. My test was:\n\n-- >8 --\n#!/bin/sh\n\nrm -rf parent child\n\nmkdir parent && (\n  cd parent &&\n  git init-db &&\n  echo content >file &&\n  git add file &&\n  git commit -m add\n) &&\nmkdir child && (\n  cd child &&\n  git init-db &&\n  git fetch ../parent master:origin &&\n  echo precious >file &&\n  ! git checkout -b foo origin\n)\n-- >8 --\n\nHowever, if I then do this:\n\n  (cd parent && echo content >another && git add . && git commit -m more)\n  (cd child && git fetch ../parent && git checkout -b new FETCH_HEAD)\n\nthen it does complain. I'm guessing there is a different code path for\nthe case that we have no index at all, and that it is not properly\nchecking for overwrites.\n\nBut now I have a small child waking up so I can't look into it further.\n:)\n\n-Peff\n"},{"id":"121684","messageId":"7viqgc3kgx.fsf@alter.siamese.dyndns.org","threadId":"20719","inReplyTo":"20090824190710.GB25168@coredump.intra.peff.net","subject":"Re: Possible regression: overwriting untracked files in a fresh repo","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-08-24T22:39:26Z","receivedAt":"2009-08-24T22:39:26Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> However, if I then do this:\n>\n>   (cd parent && echo content >another && git add . && git commit -m more)\n>   (cd child && git fetch ../parent && git checkout -b new FETCH_HEAD)\n>\n> then it does complain. I'm guessing there is a different code path for\n> the case that we have no index at all, and that it is not properly\n> checking for overwrites.\n\nI think it is this \"opts->force = 1\" done when you are on an unborn\nbranch.\n\n\tif (!old.commit && !opts->force) {\n\t\tif (!opts->quiet) {\n\t\t\twarning(\"You appear to be on a branch yet to be born.\");\n\t\t\twarning(\"Forcing checkout of %s.\", new->name);\n\t\t}\n\t\topts->force = 1;\n\t}\n"},{"id":"121686","messageId":"7vab1o3ikz.fsf@alter.siamese.dyndns.org","threadId":"20719","inReplyTo":"20090824190710.GB25168@coredump.intra.peff.net","subject":"Re: Possible regression: overwriting untracked files in a fresh repo","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-08-24T23:20:12Z","receivedAt":"2009-08-24T23:20:12Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"reset_tree() is used from two places in checkout.\n\n (1) When --force is given, to reset potentially unmerged state away and\n     forcibly switch to the destination branch.  We do an equivalent of\n     \"reset --hard\";\n\n (2) When switching from a dirty work tree using --merge, we first write\n     out the current index + any local changes in the work tree as a tree\n     object (thus ensuring that the index is merged at this point), and\n     then switch forcibly to the new branch by calling the function.\n\n     After switching to the new branch, we merge the difference between\n     the old commit and the tree that represents the dirty work tree,\n     but then reset the index to the new branch.\n\nThe \"checking out a real branch from an unborn branch\" codepath was\nreusing codepath for (1), essentially doing \"reset --hard new\".  This of\ncourse allows any work tree cruft that gets in the way removed.\n\nThe patch changes it not to force, but adds another call style for the\nreset_tree() function that does not do the hard reset, and uses it when\nyou are switching from an unborn branch (or a broken one).\n\nI do not think that this is the correct fix, but it should be a good start\nfor other people to take a look at the issue.  With this change, any\nleftover work tree files will remain, but it has an interesting effect.\n\n    $ git checkout maint\n    $ echo 'ref: refs/heads/nosuch' >.git/HEAD\n    $ git checkout -b foo master\n\nYou will notice that the index matches master (as expected), but the work\ntree mostly matches maint.  Knowing what these files are (i.e. \"these are\ngit.git source files that match 'maint' branch, and are vastly behind what\nare in 'master' branch we are switching to\"), this result is utterly\ncounterintuitive and feels wrong, but if you consider a case like what\nDscho brought up originally in the thread of having a freshly initialized\nempty repository with some uncommitted files, totally unrelated to what\nyou are checking out, I think you could argue that it is the right thing.\n\nI dunno.\n\n builtin-checkout.c |   14 ++++++++------\n 1 files changed, 8 insertions(+), 6 deletions(-)\n\ndiff --git a/builtin-checkout.c b/builtin-checkout.c\nindex 8a9a474..2930bd6 100644\n--- a/builtin-checkout.c\n+++ b/builtin-checkout.c\n@@ -309,16 +309,17 @@ static void describe_detached_head(char *msg, struct commit *commit)\n \tstrbuf_release(&sb);\n }\n \n-static int reset_tree(struct tree *tree, struct checkout_opts *o, int worktree)\n+static int reset_tree(struct tree *tree, struct checkout_opts *o, int flags)\n {\n \tstruct unpack_trees_options opts;\n \tstruct tree_desc tree_desc;\n+\tint worktree = !!(flags & 01);\n \n \tmemset(&opts, 0, sizeof(opts));\n \topts.head_idx = -1;\n \topts.update = worktree;\n \topts.skip_unmerged = !worktree;\n-\topts.reset = 1;\n+\topts.reset = !(flags & 02);\n \topts.merge = 1;\n \topts.fn = oneway_merge;\n \topts.verbose_update = !o->quiet;\n@@ -373,6 +374,10 @@ static int merge_working_tree(struct checkout_opts *opts,\n \t\tret = reset_tree(new->commit->tree, opts, 1);\n \t\tif (ret)\n \t\t\treturn ret;\n+\t} else if (!old->commit) {\n+\t\tret = reset_tree(new->commit->tree, opts, 2);\n+\t\tif (ret)\n+\t\t\treturn ret;\n \t} else {\n \t\tstruct tree_desc trees[2];\n \t\tstruct tree *tree;\n@@ -542,11 +547,8 @@ static int switch_branches(struct checkout_opts *opts, struct branch_info *new)\n \t}\n \n \tif (!old.commit && !opts->force) {\n-\t\tif (!opts->quiet) {\n+\t\tif (!opts->quiet)\n \t\t\twarning(\"You appear to be on a branch yet to be born.\");\n-\t\t\twarning(\"Forcing checkout of %s.\", new->name);\n-\t\t}\n-\t\topts->force = 1;\n \t}\n \n \tret = merge_working_tree(opts, &old, new);\n"},{"id":"121691","messageId":"20090825013601.GA3515@coredump.intra.peff.net","threadId":"20719","inReplyTo":"7vab1o3ikz.fsf@alter.siamese.dyndns.org","subject":"Re: Possible regression: overwriting untracked files in a fresh repo","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2009-08-25T01:36:01Z","receivedAt":"2009-08-25T01:36:01Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Aug 24, 2009 at 04:20:12PM -0700, Junio C Hamano wrote:\n\n> I do not think that this is the correct fix, but it should be a good start\n> for other people to take a look at the issue.  With this change, any\n> leftover work tree files will remain, but it has an interesting effect.\n> \n>     $ git checkout maint\n>     $ echo 'ref: refs/heads/nosuch' >.git/HEAD\n>     $ git checkout -b foo master\n> \n> You will notice that the index matches master (as expected), but the work\n> tree mostly matches maint.  Knowing what these files are (i.e. \"these are\n> git.git source files that match 'maint' branch, and are vastly behind what\n> are in 'master' branch we are switching to\"), this result is utterly\n> counterintuitive and feels wrong, but if you consider a case like what\n> Dscho brought up originally in the thread of having a freshly initialized\n> empty repository with some uncommitted files, totally unrelated to what\n> you are checking out, I think you could argue that it is the right thing.\n\nI don't think this is the right thing to do. We have _no_ current\nbranch, which means that everything in the working tree can be\nconsidered untracked (and therefore precious). So I think the right\nthing to do is barf and say \"this untracked file would be overwritten\".\nThe user can sort it out as appropriate, either deleting files that are\nin the way, or using \"-f\" themselves (after they make the decision that\nwhat they have can be overwritten).\n\nSomething like the patch below seems to work on the test case I posted\nearlier, and passes the test suite (but other than that, I am very\nunconfident, never having really looked at the checkout code before).\n\nAs an aside, if an unborn branch stops implying \"-f\", should we bother\neven mentioning it? It seems superfluous now.\n\n---\ndiff --git a/builtin-checkout.c b/builtin-checkout.c\nindex 8a9a474..1f2f84d 100644\n--- a/builtin-checkout.c\n+++ b/builtin-checkout.c\n@@ -402,7 +402,9 @@ static int merge_working_tree(struct checkout_opts *opts,\n \t\ttopts.dir = xcalloc(1, sizeof(*topts.dir));\n \t\ttopts.dir->flags |= DIR_SHOW_IGNORED;\n \t\ttopts.dir->exclude_per_dir = \".gitignore\";\n-\t\ttree = parse_tree_indirect(old->commit->object.sha1);\n+\t\ttree = parse_tree_indirect(old->commit ?\n+\t\t\t\t\t   old->commit->object.sha1 :\n+\t\t\t\t\t   (unsigned char *)EMPTY_TREE_SHA1_BIN);\n \t\tinit_tree_desc(&trees[0], tree->buffer, tree->size);\n \t\ttree = parse_tree_indirect(new->commit->object.sha1);\n \t\tinit_tree_desc(&trees[1], tree->buffer, tree->size);\n@@ -541,13 +543,8 @@ static int switch_branches(struct checkout_opts *opts, struct branch_info *new)\n \t\tparse_commit(new->commit);\n \t}\n \n-\tif (!old.commit && !opts->force) {\n-\t\tif (!opts->quiet) {\n-\t\t\twarning(\"You appear to be on a branch yet to be born.\");\n-\t\t\twarning(\"Forcing checkout of %s.\", new->name);\n-\t\t}\n-\t\topts->force = 1;\n-\t}\n+\tif (!old.commit && !opts->force && !opts->quiet)\n+\t\twarning(\"You appear to be on a branch yet to be born.\");\n \n \tret = merge_working_tree(opts, &old, new);\n \tif (ret)\n"},{"id":"121692","messageId":"20090825014738.GA7655@coredump.intra.peff.net","threadId":"20719","inReplyTo":"20090825013601.GA3515@coredump.intra.peff.net","subject":"Re: Possible regression: overwriting untracked files in a fresh repo","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2009-08-25T01:47:38Z","receivedAt":"2009-08-25T01:47:38Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Aug 24, 2009 at 09:36:01PM -0400, Jeff King wrote:\n\n> I don't think this is the right thing to do. We have _no_ current\n> branch, which means that everything in the working tree can be\n> considered untracked (and therefore precious). So I think the right\n> thing to do is barf and say \"this untracked file would be overwritten\".\n> The user can sort it out as appropriate, either deleting files that are\n> in the way, or using \"-f\" themselves (after they make the decision that\n> what they have can be overwritten).\n\nActually, let me clarify that a bit. If we have no branch _and_ we have\nno index, then everything is untracked. If we do have an index, then we\ndo a two-way merge from the HEAD. So if we have no branch in that case,\nit makes sense to me to treat every element of the index as an addition,\nmeaning that anything in the new tree that is different should be a\nconflict.\n\nAnd that is what my patch does: it pretends that the HEAD is the empty\ntree, which should produce sane output in both cases:\n\n  $ echo content >file\n  $ git checkout -b foo origin ;# origin has 'file' in it\n  error: Untracked working tree file 'file' would be overwritten by merge.\n  $ git add file\n  $ git checkout -b foo origin\n  error: Entry 'file' would be overwritten by merge. Cannot merge.\n\nand it even handles the matching-index case when the merge is a no-op:\n\n  $ git show origin:file >file ;# match contents\n  $ git checkout -b foo origin\n  error: Untracked working tree file 'file' would be overwritten by merge.\n  $ git add file\n  $ git checkout -b foo origin\n  Switched to a new branch 'foo'\n\n-Peff\n"},{"id":"121695","messageId":"7vk50sve00.fsf@alter.siamese.dyndns.org","threadId":"20719","inReplyTo":"20090825013601.GA3515@coredump.intra.peff.net","subject":"Re: Possible regression: overwriting untracked files in a fresh repo","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-08-25T02:11:43Z","receivedAt":"2009-08-25T02:11:43Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> diff --git a/builtin-checkout.c b/builtin-checkout.c\n> index 8a9a474..1f2f84d 100644\n> --- a/builtin-checkout.c\n> +++ b/builtin-checkout.c\n> @@ -402,7 +402,9 @@ static int merge_working_tree(struct checkout_opts *opts,\n>  \t\ttopts.dir = xcalloc(1, sizeof(*topts.dir));\n>  \t\ttopts.dir->flags |= DIR_SHOW_IGNORED;\n>  \t\ttopts.dir->exclude_per_dir = \".gitignore\";\n> -\t\ttree = parse_tree_indirect(old->commit->object.sha1);\n> +\t\ttree = parse_tree_indirect(old->commit ?\n> +\t\t\t\t\t   old->commit->object.sha1 :\n> +\t\t\t\t\t   (unsigned char *)EMPTY_TREE_SHA1_BIN);\n>  \t\tinit_tree_desc(&trees[0], tree->buffer, tree->size);\n>  \t\ttree = parse_tree_indirect(new->commit->object.sha1);\n>  \t\tinit_tree_desc(&trees[1], tree->buffer, tree->size);\n\nThis looks a lot saner; I like it.  Care to wrap it up with the usual\nsupporting material?\n\nI think the \"You appear to be\" can just go, but I do not feel very\nstrongly either way.\n"},{"id":"121698","messageId":"20090825030316.GA8098@coredump.intra.peff.net","threadId":"20719","inReplyTo":"7vk50sve00.fsf@alter.siamese.dyndns.org","subject":"Re: Possible regression: overwriting untracked files in a fresh repo","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2009-08-25T03:03:16Z","receivedAt":"2009-08-25T03:03:16Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Aug 24, 2009 at 07:11:43PM -0700, Junio C Hamano wrote:\n\n> This looks a lot saner; I like it.  Care to wrap it up with the usual\n> supporting material?\n\nPatch is below. I did a little digging to see the origin of the\ncurrent behavior. It looks like invoking \"force\" was probably the\neasiest way to make the shell version work, and then the behavior just\ngot ported to C (see below for details).\n\n> I think the \"You appear to be\" can just go, but I do not feel very\n> strongly either way.\n\nI don't feel strongly, either. I couldn't think of a reason a user would\ncare, and it is potentially confusing to new users (though most will\nsimply clone and never see it), so I ripped it out.\n\nAnd now you can try your new scissors patch. ;)\n\n-- >8 --\nSubject: [PATCH] checkout: do not imply \"-f\" on unborn branches\n\nWhen checkout sees that HEAD points to a non-existent ref,\nit currently acts as if \"-f\" was given; this behavior dates\nback to 5a03e7f, which enabled checkout from unborn branches\nin the shell version of \"git-checkout\". The reasoning given\nis to avoid the code path which tries to merge the tree\ncontents. When checkout was converted to C, this code\nremained intact.\n\nThe unfortunate side effect of this strategy is that the\n\"force\" code path will overwrite working tree and index\nstate that may be precious to the user. Instead of enabling\n\"force\", this patch uses the normal \"merge\" codepath for an\nunborn branch, but substitutes the empty tree for the \"old\"\ncommit.\n\nThis means that in the absence of an index, any files in the\nworking tree will be treated as untracked files, and a\ncheckout which would overwrite them is aborted. Similarly,\nany paths in the index will be merged with an empty entry\nas the base, meaning that unless the new branch's content is\nidentical to what's in the index, there will be a conflict\nand the checkout will be aborted.\n\nThe user is then free to correct the situation or proceed\nwith \"-f\" as appropriate.\n\nThis patch also removes the \"warning: you are on a branch\nyet to be born\" message. Its function was to warn the user\nthat we were enabling the \"-f\" option. Since we are no\nlonger doing that, there is no reason for the user to care\nwhether we are switching away from an unborn branch.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin-checkout.c         |   12 +++---------\n t/t2015-checkout-unborn.sh |   40 ++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 43 insertions(+), 9 deletions(-)\n create mode 100755 t/t2015-checkout-unborn.sh\n\ndiff --git a/builtin-checkout.c b/builtin-checkout.c\nindex 8a9a474..c6d6ac9 100644\n--- a/builtin-checkout.c\n+++ b/builtin-checkout.c\n@@ -402,7 +402,9 @@ static int merge_working_tree(struct checkout_opts *opts,\n \t\ttopts.dir = xcalloc(1, sizeof(*topts.dir));\n \t\ttopts.dir->flags |= DIR_SHOW_IGNORED;\n \t\ttopts.dir->exclude_per_dir = \".gitignore\";\n-\t\ttree = parse_tree_indirect(old->commit->object.sha1);\n+\t\ttree = parse_tree_indirect(old->commit ?\n+\t\t\t\t\t   old->commit->object.sha1 :\n+\t\t\t\t\t   (unsigned char *)EMPTY_TREE_SHA1_BIN);\n \t\tinit_tree_desc(&trees[0], tree->buffer, tree->size);\n \t\ttree = parse_tree_indirect(new->commit->object.sha1);\n \t\tinit_tree_desc(&trees[1], tree->buffer, tree->size);\n@@ -541,14 +543,6 @@ static int switch_branches(struct checkout_opts *opts, struct branch_info *new)\n \t\tparse_commit(new->commit);\n \t}\n \n-\tif (!old.commit && !opts->force) {\n-\t\tif (!opts->quiet) {\n-\t\t\twarning(\"You appear to be on a branch yet to be born.\");\n-\t\t\twarning(\"Forcing checkout of %s.\", new->name);\n-\t\t}\n-\t\topts->force = 1;\n-\t}\n-\n \tret = merge_working_tree(opts, &old, new);\n \tif (ret)\n \t\treturn ret;\ndiff --git a/t/t2015-checkout-unborn.sh b/t/t2015-checkout-unborn.sh\nnew file mode 100755\nindex 0000000..c551d39\n--- /dev/null\n+++ b/t/t2015-checkout-unborn.sh\n@@ -0,0 +1,40 @@\n+#!/bin/sh\n+\n+test_description='checkout from unborn branch protects contents'\n+. ./test-lib.sh\n+\n+test_expect_success 'setup' '\n+\tmkdir parent &&\n+\t(cd parent &&\n+\t git init &&\n+\t echo content >file &&\n+\t git add file &&\n+\t git commit -m base\n+\t) &&\n+\tgit fetch parent master:origin\n+'\n+\n+test_expect_success 'checkout from unborn preserves untracked files' '\n+\techo precious >expect &&\n+\techo precious >file &&\n+\ttest_must_fail git checkout -b new origin &&\n+\ttest_cmp expect file\n+'\n+\n+test_expect_success 'checkout from unborn preserves index contents' '\n+\techo precious >expect &&\n+\techo precious >file &&\n+\tgit add file &&\n+\ttest_must_fail git checkout -b new origin &&\n+\ttest_cmp expect file &&\n+\tgit show :file >file &&\n+\ttest_cmp expect file\n+'\n+\n+test_expect_success 'checkout from unborn merges identical index contents' '\n+\techo content >file &&\n+\tgit add file &&\n+\tgit checkout -b new origin\n+'\n+\n+test_done\n-- \n1.6.4.1.330.g14cea.dirty\n"},{"id":"121721","messageId":"alpine.DEB.1.00.0908251333450.8306@pacific.mpi-cbg.de","threadId":"20719","inReplyTo":"20090825030316.GA8098@coredump.intra.peff.net","subject":"Re: Possible regression: overwriting untracked files in a fresh repo","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2009-08-25T11:34:08Z","receivedAt":"2009-08-25T11:34:08Z","isPatch":false,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Mon, 24 Aug 2009, Jeff King wrote:\n\n> Subject: [PATCH] checkout: do not imply \"-f\" on unborn branches\n> \n> [...]\n\nThanks!\n\nCiao,\nDscho\n"}]}