threads / patch / 32669

patchunpack-trees: do not abort when overwriting an existing file with the same content

Subject: [PATCH] unpack-trees: do not abort when overwriting an existing file with the same content

## tl;dr

9 messages between Jan 19, 2013 and Jan 22, 2013. Diffs are folded; open one to read it.

replies: 8people: 3as markdown or json

Nguyễn Thái Ngọc Duy· Jan 19, 2013, 11:24 UTC · lore
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 The "git reset" as illustrated in the test case is one case people
 may be hit by this. We can also do the same for "uptodate" check, but
 I'm not sure how that could happen.
 t/t1011-read-tree-sparse-checkout.sh |  3 ++-
 t/t2021-checkout-overwrite.sh        |  8 ++++++++
 unpack-trees.c                       | 27 +++++++++++++++++++++++++++
 3 files changed, 37 insertions(+), 1 deletion(-)
Show changes to 3 files +37 −1

t/t1011-read-tree-sparse-checkout.sh, t/t2021-checkout-overwrite.sh, unpack-trees.c

diff --git a/t/t1011-read-tree-sparse-checkout.sh b/t/t1011-read-tree-sparse-checkout.sh
index 5c0053a..38f9899 100755
--- a/t/t1011-read-tree-sparse-checkout.sh
+++ b/t/t1011-read-tree-sparse-checkout.sh
@@ -238,7 +238,8 @@ test_expect_success 'print errors when failed to update worktree' '
 	echo sub >.git/info/sparse-checkout &&
 	git checkout -f init &&
 	mkdir sub &&
-	touch sub/added sub/addedtoo &&
+	echo modified >sub/added &&
+	echo modified >sub/addedtoo &&
 	test_must_fail git checkout top 2>actual &&
 	cat >expected <<\EOF &&
 error: The following untracked working tree files would be overwritten by checkout:
diff --git a/t/t2021-checkout-overwrite.sh b/t/t2021-checkout-overwrite.sh
index 5da63e9..4163449 100755
--- a/t/t2021-checkout-overwrite.sh
+++ b/t/t2021-checkout-overwrite.sh
@@ -47,4 +47,12 @@ test_expect_success SYMLINKS 'checkout commit with dir must not remove untracked
 	test -h a/b
 '
 
+test_expect_success 'do not abort on overwriting an existing file with the same content' '
+	echo abc >bar &&
+	git add bar &&
+	git commit -m "new file" &&
+	git reset HEAD^ &&
+	git checkout HEAD@{1}
+'
+
 test_done
diff --git a/unpack-trees.c b/unpack-trees.c
index 0e1a196..16adc03 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -1363,6 +1363,7 @@ static int check_ok_to_remove(const char *name, int len, int dtype,
 			      struct unpack_trees_options *o)
 {
 	struct cache_entry *result;
+	unsigned long ce_size;
 
 	/*
 	 * It may be that the 'lstat()' succeeded even though
@@ -1405,6 +1406,32 @@ static int check_ok_to_remove(const char *name, int len, int dtype,
 			return 0;
 	}
 
+	/*
+	 * If it has the same content that we are going to write down,
+	 * there's no point in complaining. We still overwrite it in the
+	 * end though. Permission is not checked so it may be lost.
+	 */
+	if (ce &&
+	    S_ISREG(st->st_mode) && S_ISREG(ce->ce_mode) &&
+	    st->st_size < 1024 * 1024 && /* should be configurable */
+	    sha1_object_info(ce->sha1, &ce_size) == OBJ_BLOB &&
+	    ce_size == st->st_size) {
+		void *buffer = NULL;
+		unsigned long size;
+		enum object_type type;
+		struct strbuf sb = STRBUF_INIT;
+		int matched =
+			strbuf_read_file(&sb, ce->name, ce_size) == ce_size &&
+			(buffer = read_sha1_file(ce->sha1, &type, &size)) != NULL &&
+			type == OBJ_BLOB &&
+			size == ce_size &&
+			!memcmp(buffer, sb.buf, size);
+		free(buffer);
+		strbuf_release(&sb);
+		if (matched)
+			return 0;
+	}
+
 	return o->gently ? -1 :
 		add_rejected_path(o, error_type, name);
 }
-- 
1.8.0.rc2.23.g1fb49df
Junio C Hamano· Jan 20, 2013, 18:35 UTC · re: Nguyễn Thái Ngọc Duy · lore

Re: [PATCH] unpack-trees: do not abort when overwriting an existing file with the same content

Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:
> +	/*
> +	 * If it has the same content that we are going to write down,
write down???
> +	 * there's no point in complaining. We still overwrite it in the
> +	 * end though. Permission is not checked so it may be lost.
> +	 */

That is a regression, isn't it? Is it too cumbersome to avoid losing the permission bits by stopping in that case?

Show 24 quoted lines
> +	if (ce &&
> +	    S_ISREG(st->st_mode) && S_ISREG(ce->ce_mode) &&
> +	    st->st_size < 1024 * 1024 && /* should be configurable */
> +	    sha1_object_info(ce->sha1, &ce_size) == OBJ_BLOB &&
> +	    ce_size == st->st_size) {
> +		void *buffer = NULL;
> +		unsigned long size;
> +		enum object_type type;
> +		struct strbuf sb = STRBUF_INIT;
> +		int matched =
> +			strbuf_read_file(&sb, ce->name, ce_size) == ce_size &&
> +			(buffer = read_sha1_file(ce->sha1, &type, &size)) != NULL &&
> +			type == OBJ_BLOB &&
> +			size == ce_size &&
> +			!memcmp(buffer, sb.buf, size);
> +		free(buffer);
> +		strbuf_release(&sb);
> +		if (matched)
> +			return 0;
> +	}
> +
>  	return o->gently ? -1 :
>  		add_rejected_path(o, error_type, name);
>  }
Duy Nguyen· Jan 21, 2013, 01:35 UTC · re: Junio C Hamano · lore

Re: [PATCH] unpack-trees: do not abort when overwriting an existing file with the same content

On Mon, Jan 21, 2013 at 1:35 AM, Junio C Hamano <gitster@pobox.com> wrote:
Show 6 quoted lines
> Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:
>
>> +     /*
>> +      * If it has the same content that we are going to write down,
>
> write down???
hmm.. "overwrite" then.
Show 6 quoted lines
>> +      * there's no point in complaining. We still overwrite it in the
>> +      * end though. Permission is not checked so it may be lost.
>> +      */
>
> That is a regression, isn't it?  Is it too cumbersome to avoid
> losing the permission bits by stopping in that case?

I'm not sure how to deal with this case. (Lack of) Executable bit can be easily restored (unlike file content) if we give the user the list of changed files. On the other hand, not everybody runs git with a huge scrollback buffer and warnings can be lost. I guess abort is a safe choice.

-- 
Duy
Nguyễn Thái Ngọc Duy· Jan 21, 2013, 11:40 UTC · re: Nguyễn Thái Ngọc Duy · lore

[PATCH v2] unpack-trees: do not abort when overwriting an existing file with the same content

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 Changes: do not lose worktree's executable permission.
 t/t1011-read-tree-sparse-checkout.sh |  3 ++-
 t/t2021-checkout-overwrite.sh        | 18 ++++++++++++++++++
 unpack-trees.c                       | 31 +++++++++++++++++++++++++++++++
 3 files changed, 51 insertions(+), 1 deletion(-)
Show changes to 3 files +51 −1

t/t1011-read-tree-sparse-checkout.sh, t/t2021-checkout-overwrite.sh, unpack-trees.c

diff --git a/t/t1011-read-tree-sparse-checkout.sh b/t/t1011-read-tree-sparse-checkout.sh
index 5c0053a..38f9899 100755
--- a/t/t1011-read-tree-sparse-checkout.sh
+++ b/t/t1011-read-tree-sparse-checkout.sh
@@ -238,7 +238,8 @@ test_expect_success 'print errors when failed to update worktree' '
 	echo sub >.git/info/sparse-checkout &&
 	git checkout -f init &&
 	mkdir sub &&
-	touch sub/added sub/addedtoo &&
+	echo modified >sub/added &&
+	echo modified >sub/addedtoo &&
 	test_must_fail git checkout top 2>actual &&
 	cat >expected <<\EOF &&
 error: The following untracked working tree files would be overwritten by checkout:
diff --git a/t/t2021-checkout-overwrite.sh b/t/t2021-checkout-overwrite.sh
index 5da63e9..bb1696d 100755
--- a/t/t2021-checkout-overwrite.sh
+++ b/t/t2021-checkout-overwrite.sh
@@ -47,4 +47,22 @@ test_expect_success SYMLINKS 'checkout commit with dir must not remove untracked
 	test -h a/b
 '
 
+test_expect_success 'do not abort on overwriting an existing file with the same content' '
+	echo abc >bar &&
+	git add bar &&
+	git commit -m "new file" &&
+	git reset HEAD^ &&
+	git checkout HEAD@{1}
+'
+
+test_expect_success POSIXPERM 'do abort on an existing file, same content but different permission' '
+	git checkout -f HEAD^ &&
+	echo abc >bar &&
+	git add bar &&
+	git commit -m "new file" &&
+	git reset HEAD^ &&
+	chmod a+x bar &&
+	test_must_fail git checkout HEAD@{1}
+'
+
 test_done
diff --git a/unpack-trees.c b/unpack-trees.c
index 0e1a196..ea204ae 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -9,6 +9,8 @@
 #include "refs.h"
 #include "attr.h"
 
+#define SAME_CONTENT_SIZE_LIMIT (1024 * 1024)
+
 /*
  * Error messages expected by scripts out of plumbing commands such as
  * read-tree.  Non-scripted Porcelain is not required to use these messages
@@ -1363,6 +1365,7 @@ static int check_ok_to_remove(const char *name, int len, int dtype,
 			      struct unpack_trees_options *o)
 {
 	struct cache_entry *result;
+	unsigned long ce_size;
 
 	/*
 	 * It may be that the 'lstat()' succeeded even though
@@ -1405,6 +1408,34 @@ static int check_ok_to_remove(const char *name, int len, int dtype,
 			return 0;
 	}
 
+	/*
+	 * If it has the same content that we are going to overwrite,
+	 * there's no point in complaining. We still overwrite it in
+	 * the end though.
+	 */
+	if (ce &&
+	    S_ISREG(st->st_mode) && S_ISREG(ce->ce_mode) &&
+	    (!trust_executable_bit ||
+	     (0100 & (ce->ce_mode ^ st->st_mode)) == 0) &&
+	    st->st_size < SAME_CONTENT_SIZE_LIMIT &&
+	    sha1_object_info(ce->sha1, &ce_size) == OBJ_BLOB &&
+	    ce_size == st->st_size) {
+		void *buffer = NULL;
+		unsigned long size;
+		enum object_type type;
+		struct strbuf sb = STRBUF_INIT;
+		int matched =
+			strbuf_read_file(&sb, ce->name, ce_size) == ce_size &&
+			(buffer = read_sha1_file(ce->sha1, &type, &size)) != NULL &&
+			type == OBJ_BLOB &&
+			size == ce_size &&
+			!memcmp(buffer, sb.buf, size);
+		free(buffer);
+		strbuf_release(&sb);
+		if (matched)
+			return 0;
+	}
+
 	return o->gently ? -1 :
 		add_rejected_path(o, error_type, name);
 }
-- 
1.8.0.rc2.23.g1fb49df
Jeff King· Jan 21, 2013, 23:15 UTC · re: Nguyễn Thái Ngọc Duy · lore

Re: [PATCH v2] unpack-trees: do not abort when overwriting an existing file with the same content

On Mon, Jan 21, 2013 at 06:40:33PM +0700, Nguyen Thai Ngoc Duy wrote:
Show 27 quoted lines
> +	/*
> +	 * If it has the same content that we are going to overwrite,
> +	 * there's no point in complaining. We still overwrite it in
> +	 * the end though.
> +	 */
> +	if (ce &&
> +	    S_ISREG(st->st_mode) && S_ISREG(ce->ce_mode) &&
> +	    (!trust_executable_bit ||
> +	     (0100 & (ce->ce_mode ^ st->st_mode)) == 0) &&
> +	    st->st_size < SAME_CONTENT_SIZE_LIMIT &&
> +	    sha1_object_info(ce->sha1, &ce_size) == OBJ_BLOB &&
> +	    ce_size == st->st_size) {
> +		void *buffer = NULL;
> +		unsigned long size;
> +		enum object_type type;
> +		struct strbuf sb = STRBUF_INIT;
> +		int matched =
> +			strbuf_read_file(&sb, ce->name, ce_size) == ce_size &&
> +			(buffer = read_sha1_file(ce->sha1, &type, &size)) != NULL &&
> +			type == OBJ_BLOB &&
> +			size == ce_size &&
> +			!memcmp(buffer, sb.buf, size);
> +		free(buffer);
> +		strbuf_release(&sb);
> +		if (matched)
> +			return 0;
> +	}
Can you elaborate on when this code is triggered?

In the general case, shouldn't we already know the sha1 of what's on disk in the index, and be able to just compare the hashes? And if we don't, because the entry is start-dirty, should we be updating it (possibly earlier, so we do not even get into the "need to write" code path) instead of doing this ad-hoc byte comparison?

Confused...
-Peff
Duy Nguyen· Jan 22, 2013, 00:59 UTC · re: Jeff King · lore

Re: [PATCH v2] unpack-trees: do not abort when overwriting an existing file with the same content

On Tue, Jan 22, 2013 at 6:15 AM, Jeff King <peff@peff.net> wrote:
Show 9 quoted lines
> Can you elaborate on when this code is triggered?
>
> In the general case, shouldn't we already know the sha1 of what's on
> disk in the index, and be able to just compare the hashes? And if we
> don't, because the entry is start-dirty, should we be updating it
> (possibly earlier, so we do not even get into the "need to write" code
> path) instead of doing this ad-hoc byte comparison?
>
> Confused...

git reset HEAD~10 # blah that was a mistake, undo it git checkout HEAD@{1}

I hit it a few times, probably not with the exact recipe above though.
-- 
Duy
Junio C Hamano· Jan 22, 2013, 01:45 UTC · re: Duy Nguyen · lore

Re: [PATCH v2] unpack-trees: do not abort when overwriting an existing file with the same content

Duy Nguyen <pclouds@gmail.com> writes:
Show 8 quoted lines
> On Tue, Jan 22, 2013 at 6:15 AM, Jeff King <peff@peff.net> wrote:
>> Can you elaborate on when this code is triggered?
>>
>> In the general case, shouldn't we already know the sha1 of what's on
>> disk in the index, and be able to just compare the hashes? And if we
>> don't, because the entry is start-dirty, should we be updating it
>> (possibly earlier, so we do not even get into the "need to write" code
>> path) instead of doing this ad-hoc byte comparison?
If the index knows what is in the working tree, I think I agree.
Show 7 quoted lines
>> Confused...
>
> git reset HEAD~10
> # blah that was a mistake, undo it
> git checkout HEAD@{1}
>
> I hit it a few times, probably not with the exact recipe above though.

I've seen myself doing "git reset HEAD~10" by mistake (I wanted "--hard"), but the recovery is to do "git reset --hard @{1}" and then come back with "git reset --hard HEAD~10", so it hasn't been a real problem for me.

A case similar to this is already covered by a special case of two-tree read-tree where the index already matches the tree we are checking out even though it is different from HEAD; in other words, if you did this:

        git init
        date >file
        git add file; git commit -m 'has a file'
        git checkout -b side
        git rm file; git commit -m 'does not have the file'
        git checkout master file
	: now index has the file from 'master' and worktree is clean
        git checkout master
you shouldn't get any complaint, I think.

If you did "git rm --cached file" to lose it from the index, immediately after "git checkout master file", then we wouldn't know what we may be losing. If the file in the working tree happens to match the index and the HEAD after checking out the other branch ('master' in this case), it is _not_ losing information, so we might be able to treat it as an extension of the existing special case.

I haven't thought things through to see if in a more general case that this codepath triggers we might be losing a local changes that we should be carrying forward while checking out a new branch, though.

Duy Nguyen· Jan 22, 2013, 01:55 UTC · re: Junio C Hamano · lore

Re: [PATCH v2] unpack-trees: do not abort when overwriting an existing file with the same content

On Tue, Jan 22, 2013 at 8:45 AM, Junio C Hamano <gitster@pobox.com> wrote:
Show 25 quoted lines
> Duy Nguyen <pclouds@gmail.com> writes:
>
>> On Tue, Jan 22, 2013 at 6:15 AM, Jeff King <peff@peff.net> wrote:
>>> Can you elaborate on when this code is triggered?
>>>
>>> In the general case, shouldn't we already know the sha1 of what's on
>>> disk in the index, and be able to just compare the hashes? And if we
>>> don't, because the entry is start-dirty, should we be updating it
>>> (possibly earlier, so we do not even get into the "need to write" code
>>> path) instead of doing this ad-hoc byte comparison?
>
> If the index knows what is in the working tree, I think I agree.
>
>>> Confused...
>>
>> git reset HEAD~10
>> # blah that was a mistake, undo it
>> git checkout HEAD@{1}
>>
>> I hit it a few times, probably not with the exact recipe above though.
>
> I've seen myself doing "git reset HEAD~10" by mistake (I wanted "--hard"),
> but the recovery is to do "git reset --hard @{1}" and then come back
> with "git reset --hard HEAD~10", so it hasn't been a real problem
> for me.
Except when you already make some changes elsewhere and you don't want --hard.
-- 
Duy
Jeff King· Jan 22, 2013, 20:19 UTC · re: Junio C Hamano · lore

Re: [PATCH v2] unpack-trees: do not abort when overwriting an existing file with the same content

On Mon, Jan 21, 2013 at 05:45:05PM -0800, Junio C Hamano wrote:
Show 12 quoted lines
> Duy Nguyen <pclouds@gmail.com> writes:
> 
> > On Tue, Jan 22, 2013 at 6:15 AM, Jeff King <peff@peff.net> wrote:
> >> Can you elaborate on when this code is triggered?
> >>
> >> In the general case, shouldn't we already know the sha1 of what's on
> >> disk in the index, and be able to just compare the hashes? And if we
> >> don't, because the entry is start-dirty, should we be updating it
> >> (possibly earlier, so we do not even get into the "need to write" code
> >> path) instead of doing this ad-hoc byte comparison?
> 
> If the index knows what is in the working tree, I think I agree.

Right. I was wondering why it didn't (and if it doesn't, why we are not saving the information there).

But I think I was letting my inaccurate mental model of the index get in the way. I tend to think of the stat information as "if the file matches this stat info, then it has sha1 X". But that is not true. The sha1 we store is the actual index entry, and if it does not match what is in the working tree, we do not know or store the sha1 of what is in the working tree. We cannot just "refresh" that value and compare it, which is what I was implying.

So I think I was just confused. That is what I get for not actually doing low-level index stuff enough.

> > git reset HEAD~10
> > # blah that was a mistake, undo it
> > git checkout HEAD@{1}
It seems like
  git reset HEAD@{1}

would be the correct undo, as the original never touched the working tree.

-Peff

← back to recent threads