Re: [RFC PATCH] Introduce "precious" file concept

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

On Sun, Nov 11 2018, Ævar Arnfjörð Bjarmason wrote:

> [CC-ing some of the people involved in recent threads about this]
>
> On Sun, Nov 11 2018, Nguyễn Thái Ngọc Duy wrote:
>
>> Since this topic has come up twice recently, I revisited this
>> "precious" thingy that I started four years ago and tried to see if I
>> could finally finish it. There are a couple things to be sorted out...
>>
>> A new attribute "precious" is added to indicate that certain files
>> have valuable content and should not be easily discarded even if they
>> are ignored or untracked (*).
>>
>> So far there are two parts of Git that are made aware of precious
>> files: "git clean" will leave precious files alone and unpack-trees.c
>> (i.e. merges and branch switches) will not overwrite
>> ignored-but-precious files.
>>
>> Is there any other parts of Git that should be made aware of this
>> "precious" attribute?
>>
>> Also while "precious" is a fun name, but it does not sound serious.
>> Any suggestions? Perhaps "valuable"?
>>
>> Very lightly tested. The patch is more to have something to discuss
>> than is bug free and ready to use.
>>
>> (*) Note that tracked files could be marked "precious" in the future
>>     too although the exact semantics is not very clear since tracked
>>     files are by default precious.
>>
>>     But something like "index log" could use this to record all
>>     changes to precious files instead of just "git add -p" changes,
>>     for example. So these files are in a sense more precious than
>>     other tracked files.
>>
>> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
>> ---
>>  Documentation/git-clean.txt     |  3 ++-
>>  Documentation/gitattributes.txt | 13 +++++++++++++
>>  attr.c                          |  9 +++++++++
>>  attr.h                          |  2 ++
>>  builtin/clean.c                 | 19 ++++++++++++++++---
>>  unpack-trees.c                  |  3 ++-
>>  6 files changed, 44 insertions(+), 5 deletions(-)
>>
>> diff --git a/Documentation/git-clean.txt b/Documentation/git-clean.txt
>> index 03056dad0d..a9beadfb12 100644
>> --- a/Documentation/git-clean.txt
>> +++ b/Documentation/git-clean.txt
>> @@ -21,7 +21,8 @@ option is specified, ignored files are also removed. This can, for
>>  example, be useful to remove all build products.
>>
>>  If any optional `<path>...` arguments are given, only those paths
>> -are affected.
>> +are affected. Ignored or untracked files with `precious` attributes
>> +are not removed.
>>
>>  OPTIONS
>>  -------
>> diff --git a/Documentation/gitattributes.txt b/Documentation/gitattributes.txt
>> index b8392fc330..c722479bdc 100644
>> --- a/Documentation/gitattributes.txt
>> +++ b/Documentation/gitattributes.txt
>> @@ -1188,6 +1188,19 @@ If this attribute is not set or has an invalid value, the value of the
>>  (See linkgit:git-config[1]).
>>
>>
>> +Precious files
>> +~~~~~~~~~~~~~~~~~~~~~~~~
>> +
>> +`precious`
>> +^^^^^^^^^^
>> +
>> +This attribute is set on files to indicate that their content is
>> +valuable. Many commands will behave slightly different on precious
>> +files. linkgit:git-clean[1] will leave precious files alone. Merging
>> +and branch switching will not silently overwrite ignored files that
>> +are marked "precious".
>> +
>> +
>>  USING MACRO ATTRIBUTES
>>  ----------------------
>>
>> diff --git a/attr.c b/attr.c
>> index 60d284796d..d06ca0ae4b 100644
>> --- a/attr.c
>> +++ b/attr.c
>> @@ -1186,3 +1186,12 @@ void attr_start(void)
>>  	pthread_mutex_init(&check_vector.mutex, NULL);
>>  #endif
>>  }
>> +
>> +int is_precious_file(struct index_state *istate, const char *path)
>> +{
>> +	static struct attr_check *check;
>> +	if (!check)
>> +		check = attr_check_initl("precious", NULL);
>> +	git_check_attr(istate, path, check);
>> +	return check && ATTR_TRUE(check->items[0].value);
>> +}
>
> If we merge two branches is this using the merged post-image of
> .gitattributes as a source?
>
>>  	if (o->dir &&
>> -	    is_excluded(o->dir, o->src_index, name, &dtype))
>> +	    is_excluded(o->dir, o->src_index, name, &dtype) &&
>> +	    !is_precious_file(o->src_index, name))
>>  		/*
>>  		 * ce->name is explicitly excluded, so it is Ok to
>>  		 * overwrite it.
>
> I wonder if instead we should just be reverting c81935348b ("Fix
> switching to a branch with D/F when current branch has file D.",
> 2007-03-15), which these days (haven't dug deeply) would just be this,
> right?:
>
>>    diff --git a/unpack-trees.c b/unpack-trees.c
>     index 7570df481b..b3efaddd4f 100644
>     --- a/unpack-trees.c
>     +++ b/unpack-trees.c
>     @@ -1894,13 +1894,6 @@ static int check_ok_to_remove(const char *name, int len, int dtype,
>      	if (ignore_case && icase_exists(o, name, len, st))
>      		return 0;
>
>     -	if (o->dir &&
>     -	    is_excluded(o->dir, o->src_index, name, &dtype))
>     -		/*
>     -		 * ce->name is explicitly excluded, so it is Ok to
>     -		 * overwrite it.
>     -		 */
>     -		return 0;
>      	if (S_ISDIR(st->st_mode)) {
>      		/*
>      		 * We are checking out path "foo" and
>
> Something like the approach you're taking will absolutely work from a
> technical standpoint, but I fear that it's going to be useless in
> practice.
>
> The users who need protection against git deleting their files the most
> are exactly the sort of users who aren't expert-level enough to
> understand the nuances of how the semantics of .gitignore and "precious"
> are going to interact before git eats their data.
>
> This is pretty apparent from the bug reports we're getting about
> this. None of them are:
>
>     "Hey, I 100% understood .gitignore semantics including this one part
>     of the docs where you say you'll do this, but just forgot one day
>     and deleted my work. Can we get some more safety?"
>
> But rather (with some hyperbole for effect):
>
>     "ZOMG git deleted my file! Is this a bug??"
>
> So I think we should have the inverse of this "precious"
> attribute". Just a change to the docs to say that .gitignore doesn't
> imply these eager deletion semantics on tree unpacking anymore, and if
> users want it back they can define a "garbage" attribute
> (s/precious/garbage/).
>
> That will lose no data, and in the very rare cases where a checkout of
> tracked files would overwrite an ignored pattern, we can just error out
> (as we do with the "Ok to overwrite" branch removed) and tell the user
> to delete the files to proceed.
>
> Three tests in our test suite fail with that patch applied, and they're
> explicitly testing for exactly the sort of scenario where users are likely to lose data. I.e.:
>
>  1. Open a tracked file in an editor
>  2. Save it
>  3. Switch to a topic branch, that has different .gitignore semantics
>     (e.g. let's say a build/ dir exists there)
>  4. Have their work deleted
>
> So actually in writing this out I've become convinced that this
> "precious" approach can't work either, because *even if* you're an
> expert who manages to perfectly define their .gitignore and "precious"
> rules in advance to avoid data deletion, those rules will *also* need to
> take into account switching between branches (or even different
> histories) where you have other sorts of rules.
>
> So really, if there's ambiguity let's just not delete stuff by default
> and ask the user to resolve it.

Here's a patch to implement that (which borrows from some of yours). It
passes all of our tests:

diff --git a/Documentation/gitattributes.txt b/Documentation/gitattributes.txt
index b8392fc330..a6cad17899 100644
--- a/Documentation/gitattributes.txt
+++ b/Documentation/gitattributes.txt
@@ -1188,6 +1188,17 @@ If this attribute is not set or has an invalid value, the value of the
 (See linkgit:git-config[1]).


+Trashable files
+~~~~~~~~~~~~~~~
+
+`trashable`
+^^^^^^^^^^
+
+Provides an escape hatch for re-enabling a potentially data destroying
+feature which was enabled by default between Git versions 1.5.2 and
+2.20. See the `NOTES` section of linkgit:gitignore[5] for details.
+
+
 USING MACRO ATTRIBUTES
 ----------------------

diff --git a/Documentation/gitignore.txt b/Documentation/gitignore.txt
index d107daaffd..39c6d5955a 100644
--- a/Documentation/gitignore.txt
+++ b/Documentation/gitignore.txt
@@ -140,6 +140,13 @@ not tracked by Git remain untracked.
 To stop tracking a file that is currently tracked, use
 'git rm --cached'.

+Between Git versions 1.5.2 and 2.20 untracked files or directories
+which were ignored and conflicted with a file about to be checked out
+(e.g. during linkgit:git-checkout[1] or linkgit:git-merge[1]) would be
+deleted. This could lead to loss of user data and is no longer the
+default, See `trashable` in linkgit:gitattributes[5]. for how to
+selectively enable this behavior.
+
 EXAMPLES
 --------

diff --git a/attr.c b/attr.c
index 60d284796d..930af78650 100644
--- a/attr.c
+++ b/attr.c
@@ -1186,3 +1186,12 @@ void attr_start(void)
 	pthread_mutex_init(&check_vector.mutex, NULL);
 #endif
 }
+
+int is_trashable_file(struct index_state *istate, const char *path)
+{
+	static struct attr_check *check;
+	if (!check)
+		check = attr_check_initl("trashable", NULL);
+	git_check_attr(istate, path, check);
+	return check && ATTR_TRUE(check->items[0].value);
+}
diff --git a/attr.h b/attr.h
index b0378bfe5f..ccf4d4e6b5 100644
--- a/attr.h
+++ b/attr.h
@@ -82,4 +82,6 @@ void git_attr_set_direction(enum git_attr_direction new_direction);

 void attr_start(void);

+int is_trashable_file(struct index_state *istate, const char *path);
+
 #endif /* ATTR_H */
diff --git a/t/lib-submodule-update.sh b/t/lib-submodule-update.sh
index 016391723c..d2ceee33d2 100755
--- a/t/lib-submodule-update.sh
+++ b/t/lib-submodule-update.sh
@@ -844,6 +844,8 @@ test_submodule_switch_recursing_with_args () {
 			git branch -t add_sub1 origin/add_sub1 &&
 			: >sub1 &&
 			echo sub1 >.git/info/exclude &&
+			test_must_fail $command add_sub1 &&
+			echo sub1 trashable >.gitattributes &&
 			$command add_sub1 &&
 			test_superproject_content origin/add_sub1 &&
 			test_submodule_content sub1 origin/add_sub1
diff --git a/t/t1004-read-tree-m-u-wf.sh b/t/t1004-read-tree-m-u-wf.sh
index c13578a635..2243cd955e 100755
--- a/t/t1004-read-tree-m-u-wf.sh
+++ b/t/t1004-read-tree-m-u-wf.sh
@@ -63,8 +63,10 @@ test_expect_success 'two-way with incorrect --exclude-per-directory (2)' '
 	fi
 '

-test_expect_success 'two-way clobbering a ignored file' '
+test_expect_success 'two-way keeping a ignored file, trashing a trashable file' '

+	read_tree_u_must_fail -m -u --exclude-per-directory=.gitignore master side &&
+	echo file2 trashable >.gitattributes &&
 	read_tree_u_must_succeed -m -u --exclude-per-directory=.gitignore master side
 '

@@ -106,7 +108,7 @@ test_expect_success 'three-way not clobbering a working tree file' '

 echo >.gitignore file3

-test_expect_success 'three-way not complaining on an untracked file' '
+test_expect_success 'three-way complaining on an untracked file, trashing a trashable file' '

 	git reset --hard &&
 	rm -f file2 subdir/file2 file3 subdir/file3 &&
@@ -114,6 +116,8 @@ test_expect_success 'three-way not complaining on an untracked file' '
 	echo >file3 file three created in master, untracked &&
 	echo >subdir/file3 file three created in master, untracked &&

+	read_tree_u_must_fail -m -u --exclude-per-directory=.gitignore branch-point master side &&
+	echo file3 trashable >.gitattributes &&
 	read_tree_u_must_succeed -m -u --exclude-per-directory=.gitignore branch-point master side
 '

diff --git a/unpack-trees.c b/unpack-trees.c
index 7570df481b..e9a7fb6583 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -1895,9 +1895,10 @@ static int check_ok_to_remove(const char *name, int len, int dtype,
 		return 0;

 	if (o->dir &&
-	    is_excluded(o->dir, o->src_index, name, &dtype))
+	    is_excluded(o->dir, o->src_index, name, &dtype) &&
+	    is_trashable_file(o->src_index, name))
 		/*
-		 * ce->name is explicitly excluded, so it is Ok to
+		 * ce->name is explicitly trashable, so it is Ok to
 		 * overwrite it.
 		 */
 		return 0;