Possible regression: overwriting untracked files in a fresh repo

Possible regression: overwriting untracked files in a fresh repo

[Johannes Schindelin]

Hi,

I _think_ that this used to complain about untracked files being 
overwritten:

	$ git init
	$ git remote add -f origin <url>
	$ git checkout -b blub origin/master

It does not do that here (any longer, IIAC).  Intended?

Ciao,
Dscho

Re: Possible regression: overwriting untracked files in a fresh repo

[Jeff King]

On Mon, Aug 24, 2009 at 06:31:21PM +0200, Johannes Schindelin wrote:

> I _think_ that this used to complain about untracked files being 
> overwritten:
> 
> 	$ git init
> 	$ git remote add -f origin <url>
> 	$ git checkout -b blub origin/master
> 
> It does not do that here (any longer, IIAC).  Intended?

I agree that it probably _should_ complain, but I don't think it ever
did. I tried a handful of released versions as far back as v1.4.4, and
all of them overwrite local files without complaining. My test was:

-- >8 --
#!/bin/sh

rm -rf parent child

mkdir parent && (
  cd parent &&
  git init-db &&
  echo content >file &&
  git add file &&
  git commit -m add
) &&
mkdir child && (
  cd child &&
  git init-db &&
  git fetch ../parent master:origin &&
  echo precious >file &&
  ! git checkout -b foo origin
)
-- >8 --

However, if I then do this:

  (cd parent && echo content >another && git add . && git commit -m more)
  (cd child && git fetch ../parent && git checkout -b new FETCH_HEAD)

then it does complain. I'm guessing there is a different code path for
the case that we have no index at all, and that it is not properly
checking for overwrites.

But now I have a small child waking up so I can't look into it further.
:)

-Peff

Re: Possible regression: overwriting untracked files in a fresh repo

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> However, if I then do this:
>
>   (cd parent && echo content >another && git add . && git commit -m more)
>   (cd child && git fetch ../parent && git checkout -b new FETCH_HEAD)
>
> then it does complain. I'm guessing there is a different code path for
> the case that we have no index at all, and that it is not properly
> checking for overwrites.

I think it is this "opts->force = 1" done when you are on an unborn
branch.

	if (!old.commit && !opts->force) {
		if (!opts->quiet) {
			warning("You appear to be on a branch yet to be born.");
			warning("Forcing checkout of %s.", new->name);
		}
		opts->force = 1;
	}

Re: Possible regression: overwriting untracked files in a fresh repo

[Junio C Hamano]

reset_tree() is used from two places in checkout.

 (1) When --force is given, to reset potentially unmerged state away and
     forcibly switch to the destination branch.  We do an equivalent of
     "reset --hard";

 (2) When switching from a dirty work tree using --merge, we first write
     out the current index + any local changes in the work tree as a tree
     object (thus ensuring that the index is merged at this point), and
     then switch forcibly to the new branch by calling the function.

     After switching to the new branch, we merge the difference between
     the old commit and the tree that represents the dirty work tree,
     but then reset the index to the new branch.

The "checking out a real branch from an unborn branch" codepath was
reusing codepath for (1), essentially doing "reset --hard new".  This of
course allows any work tree cruft that gets in the way removed.

The patch changes it not to force, but adds another call style for the
reset_tree() function that does not do the hard reset, and uses it when
you are switching from an unborn branch (or a broken one).

I do not think that this is the correct fix, but it should be a good start
for other people to take a look at the issue.  With this change, any
leftover work tree files will remain, but it has an interesting effect.

    $ git checkout maint
    $ echo 'ref: refs/heads/nosuch' >.git/HEAD
    $ git checkout -b foo master

You will notice that the index matches master (as expected), but the work
tree mostly matches maint.  Knowing what these files are (i.e. "these are
git.git source files that match 'maint' branch, and are vastly behind what
are in 'master' branch we are switching to"), this result is utterly
counterintuitive and feels wrong, but if you consider a case like what
Dscho brought up originally in the thread of having a freshly initialized
empty repository with some uncommitted files, totally unrelated to what
you are checking out, I think you could argue that it is the right thing.

I dunno.

 builtin-checkout.c |   14 ++++++++------
 1 files changed, 8 insertions(+), 6 deletions(-)

diff --git a/builtin-checkout.c b/builtin-checkout.c
index 8a9a474..2930bd6 100644
--- a/builtin-checkout.c
+++ b/builtin-checkout.c
@@ -309,16 +309,17 @@ static void describe_detached_head(char *msg, struct commit *commit)
 	strbuf_release(&sb);
 }
 
-static int reset_tree(struct tree *tree, struct checkout_opts *o, int worktree)
+static int reset_tree(struct tree *tree, struct checkout_opts *o, int flags)
 {
 	struct unpack_trees_options opts;
 	struct tree_desc tree_desc;
+	int worktree = !!(flags & 01);
 
 	memset(&opts, 0, sizeof(opts));
 	opts.head_idx = -1;
 	opts.update = worktree;
 	opts.skip_unmerged = !worktree;
-	opts.reset = 1;
+	opts.reset = !(flags & 02);
 	opts.merge = 1;
 	opts.fn = oneway_merge;
 	opts.verbose_update = !o->quiet;
@@ -373,6 +374,10 @@ static int merge_working_tree(struct checkout_opts *opts,
 		ret = reset_tree(new->commit->tree, opts, 1);
 		if (ret)
 			return ret;
+	} else if (!old->commit) {
+		ret = reset_tree(new->commit->tree, opts, 2);
+		if (ret)
+			return ret;
 	} else {
 		struct tree_desc trees[2];
 		struct tree *tree;
@@ -542,11 +547,8 @@ static int switch_branches(struct checkout_opts *opts, struct branch_info *new)
 	}
 
 	if (!old.commit && !opts->force) {
-		if (!opts->quiet) {
+		if (!opts->quiet)
 			warning("You appear to be on a branch yet to be born.");
-			warning("Forcing checkout of %s.", new->name);
-		}
-		opts->force = 1;
 	}
 
 	ret = merge_working_tree(opts, &old, new);

Re: Possible regression: overwriting untracked files in a fresh repo

[Jeff King]

On Mon, Aug 24, 2009 at 04:20:12PM -0700, Junio C Hamano wrote:

> I do not think that this is the correct fix, but it should be a good start
> for other people to take a look at the issue.  With this change, any
> leftover work tree files will remain, but it has an interesting effect.
> 
>     $ git checkout maint
>     $ echo 'ref: refs/heads/nosuch' >.git/HEAD
>     $ git checkout -b foo master
> 
> You will notice that the index matches master (as expected), but the work
> tree mostly matches maint.  Knowing what these files are (i.e. "these are
> git.git source files that match 'maint' branch, and are vastly behind what
> are in 'master' branch we are switching to"), this result is utterly
> counterintuitive and feels wrong, but if you consider a case like what
> Dscho brought up originally in the thread of having a freshly initialized
> empty repository with some uncommitted files, totally unrelated to what
> you are checking out, I think you could argue that it is the right thing.

I don't think this is the right thing to do. We have _no_ current
branch, which means that everything in the working tree can be
considered untracked (and therefore precious). So I think the right
thing to do is barf and say "this untracked file would be overwritten".
The user can sort it out as appropriate, either deleting files that are
in the way, or using "-f" themselves (after they make the decision that
what they have can be overwritten).

Something like the patch below seems to work on the test case I posted
earlier, and passes the test suite (but other than that, I am very
unconfident, never having really looked at the checkout code before).

As an aside, if an unborn branch stops implying "-f", should we bother
even mentioning it? It seems superfluous now.

---
diff --git a/builtin-checkout.c b/builtin-checkout.c
index 8a9a474..1f2f84d 100644
--- a/builtin-checkout.c
+++ b/builtin-checkout.c
@@ -402,7 +402,9 @@ static int merge_working_tree(struct checkout_opts *opts,
 		topts.dir = xcalloc(1, sizeof(*topts.dir));
 		topts.dir->flags |= DIR_SHOW_IGNORED;
 		topts.dir->exclude_per_dir = ".gitignore";
-		tree = parse_tree_indirect(old->commit->object.sha1);
+		tree = parse_tree_indirect(old->commit ?
+					   old->commit->object.sha1 :
+					   (unsigned char *)EMPTY_TREE_SHA1_BIN);
 		init_tree_desc(&trees[0], tree->buffer, tree->size);
 		tree = parse_tree_indirect(new->commit->object.sha1);
 		init_tree_desc(&trees[1], tree->buffer, tree->size);
@@ -541,13 +543,8 @@ static int switch_branches(struct checkout_opts *opts, struct branch_info *new)
 		parse_commit(new->commit);
 	}
 
-	if (!old.commit && !opts->force) {
-		if (!opts->quiet) {
-			warning("You appear to be on a branch yet to be born.");
-			warning("Forcing checkout of %s.", new->name);
-		}
-		opts->force = 1;
-	}
+	if (!old.commit && !opts->force && !opts->quiet)
+		warning("You appear to be on a branch yet to be born.");
 
 	ret = merge_working_tree(opts, &old, new);
 	if (ret)

Re: Possible regression: overwriting untracked files in a fresh repo

[Jeff King]

On Mon, Aug 24, 2009 at 09:36:01PM -0400, Jeff King wrote:

> I don't think this is the right thing to do. We have _no_ current
> branch, which means that everything in the working tree can be
> considered untracked (and therefore precious). So I think the right
> thing to do is barf and say "this untracked file would be overwritten".
> The user can sort it out as appropriate, either deleting files that are
> in the way, or using "-f" themselves (after they make the decision that
> what they have can be overwritten).

Actually, let me clarify that a bit. If we have no branch _and_ we have
no index, then everything is untracked. If we do have an index, then we
do a two-way merge from the HEAD. So if we have no branch in that case,
it makes sense to me to treat every element of the index as an addition,
meaning that anything in the new tree that is different should be a
conflict.

And that is what my patch does: it pretends that the HEAD is the empty
tree, which should produce sane output in both cases:

  $ echo content >file
  $ git checkout -b foo origin ;# origin has 'file' in it
  error: Untracked working tree file 'file' would be overwritten by merge.
  $ git add file
  $ git checkout -b foo origin
  error: Entry 'file' would be overwritten by merge. Cannot merge.

and it even handles the matching-index case when the merge is a no-op:

  $ git show origin:file >file ;# match contents
  $ git checkout -b foo origin
  error: Untracked working tree file 'file' would be overwritten by merge.
  $ git add file
  $ git checkout -b foo origin
  Switched to a new branch 'foo'

-Peff

Re: Possible regression: overwriting untracked files in a fresh repo

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> diff --git a/builtin-checkout.c b/builtin-checkout.c
> index 8a9a474..1f2f84d 100644
> --- a/builtin-checkout.c
> +++ b/builtin-checkout.c
> @@ -402,7 +402,9 @@ static int merge_working_tree(struct checkout_opts *opts,
>  		topts.dir = xcalloc(1, sizeof(*topts.dir));
>  		topts.dir->flags |= DIR_SHOW_IGNORED;
>  		topts.dir->exclude_per_dir = ".gitignore";
> -		tree = parse_tree_indirect(old->commit->object.sha1);
> +		tree = parse_tree_indirect(old->commit ?
> +					   old->commit->object.sha1 :
> +					   (unsigned char *)EMPTY_TREE_SHA1_BIN);
>  		init_tree_desc(&trees[0], tree->buffer, tree->size);
>  		tree = parse_tree_indirect(new->commit->object.sha1);
>  		init_tree_desc(&trees[1], tree->buffer, tree->size);

This looks a lot saner; I like it.  Care to wrap it up with the usual
supporting material?

I think the "You appear to be" can just go, but I do not feel very
strongly either way.

Re: Possible regression: overwriting untracked files in a fresh repo

[Jeff King]

On Mon, Aug 24, 2009 at 07:11:43PM -0700, Junio C Hamano wrote:

> This looks a lot saner; I like it.  Care to wrap it up with the usual
> supporting material?

Patch is below. I did a little digging to see the origin of the
current behavior. It looks like invoking "force" was probably the
easiest way to make the shell version work, and then the behavior just
got ported to C (see below for details).

> I think the "You appear to be" can just go, but I do not feel very
> strongly either way.

I don't feel strongly, either. I couldn't think of a reason a user would
care, and it is potentially confusing to new users (though most will
simply clone and never see it), so I ripped it out.

And now you can try your new scissors patch. ;)

-- >8 --
Subject: [PATCH] checkout: do not imply "-f" on unborn branches

When checkout sees that HEAD points to a non-existent ref,
it currently acts as if "-f" was given; this behavior dates
back to 5a03e7f, which enabled checkout from unborn branches
in the shell version of "git-checkout". The reasoning given
is to avoid the code path which tries to merge the tree
contents. When checkout was converted to C, this code
remained intact.

The unfortunate side effect of this strategy is that the
"force" code path will overwrite working tree and index
state that may be precious to the user. Instead of enabling
"force", this patch uses the normal "merge" codepath for an
unborn branch, but substitutes the empty tree for the "old"
commit.

This means that in the absence of an index, any files in the
working tree will be treated as untracked files, and a
checkout which would overwrite them is aborted. Similarly,
any paths in the index will be merged with an empty entry
as the base, meaning that unless the new branch's content is
identical to what's in the index, there will be a conflict
and the checkout will be aborted.

The user is then free to correct the situation or proceed
with "-f" as appropriate.

This patch also removes the "warning: you are on a branch
yet to be born" message. Its function was to warn the user
that we were enabling the "-f" option. Since we are no
longer doing that, there is no reason for the user to care
whether we are switching away from an unborn branch.

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin-checkout.c         |   12 +++---------
 t/t2015-checkout-unborn.sh |   40 ++++++++++++++++++++++++++++++++++++++++
 2 files changed, 43 insertions(+), 9 deletions(-)
 create mode 100755 t/t2015-checkout-unborn.sh

diff --git a/builtin-checkout.c b/builtin-checkout.c
index 8a9a474..c6d6ac9 100644
--- a/builtin-checkout.c
+++ b/builtin-checkout.c
@@ -402,7 +402,9 @@ static int merge_working_tree(struct checkout_opts *opts,
 		topts.dir = xcalloc(1, sizeof(*topts.dir));
 		topts.dir->flags |= DIR_SHOW_IGNORED;
 		topts.dir->exclude_per_dir = ".gitignore";
-		tree = parse_tree_indirect(old->commit->object.sha1);
+		tree = parse_tree_indirect(old->commit ?
+					   old->commit->object.sha1 :
+					   (unsigned char *)EMPTY_TREE_SHA1_BIN);
 		init_tree_desc(&trees[0], tree->buffer, tree->size);
 		tree = parse_tree_indirect(new->commit->object.sha1);
 		init_tree_desc(&trees[1], tree->buffer, tree->size);
@@ -541,14 +543,6 @@ static int switch_branches(struct checkout_opts *opts, struct branch_info *new)
 		parse_commit(new->commit);
 	}
 
-	if (!old.commit && !opts->force) {
-		if (!opts->quiet) {
-			warning("You appear to be on a branch yet to be born.");
-			warning("Forcing checkout of %s.", new->name);
-		}
-		opts->force = 1;
-	}
-
 	ret = merge_working_tree(opts, &old, new);
 	if (ret)
 		return ret;
diff --git a/t/t2015-checkout-unborn.sh b/t/t2015-checkout-unborn.sh
new file mode 100755
index 0000000..c551d39
--- /dev/null
+++ b/t/t2015-checkout-unborn.sh
@@ -0,0 +1,40 @@
+#!/bin/sh
+
+test_description='checkout from unborn branch protects contents'
+. ./test-lib.sh
+
+test_expect_success 'setup' '
+	mkdir parent &&
+	(cd parent &&
+	 git init &&
+	 echo content >file &&
+	 git add file &&
+	 git commit -m base
+	) &&
+	git fetch parent master:origin
+'
+
+test_expect_success 'checkout from unborn preserves untracked files' '
+	echo precious >expect &&
+	echo precious >file &&
+	test_must_fail git checkout -b new origin &&
+	test_cmp expect file
+'
+
+test_expect_success 'checkout from unborn preserves index contents' '
+	echo precious >expect &&
+	echo precious >file &&
+	git add file &&
+	test_must_fail git checkout -b new origin &&
+	test_cmp expect file &&
+	git show :file >file &&
+	test_cmp expect file
+'
+
+test_expect_success 'checkout from unborn merges identical index contents' '
+	echo content >file &&
+	git add file &&
+	git checkout -b new origin
+'
+
+test_done
-- 
1.6.4.1.330.g14cea.dirty

Re: Possible regression: overwriting untracked files in a fresh repo

[Johannes Schindelin]

Hi,

On Mon, 24 Aug 2009, Jeff King wrote:

> Subject: [PATCH] checkout: do not imply "-f" on unborn branches
> 
> [...]

Thanks!

Ciao,
Dscho