Web lists-archives.com

Re: [PATCH] Introduce "precious" file concept




Nguyễn Thái Ngọc Duy  <pclouds@xxxxxxxxx> writes:

> 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 one part of Git that are made aware of precious files:

s/are/is/g

> "git clean" will leave precious files alone if --keep-precious is
> specified.
>
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@xxxxxxxxx>
> ---
>  Here's the replacement patch that keeps "git clean" behavior the same
>  as before and only checks 'precious' attribute when --keep-precous is
>  specified.

OK.  So this is even gentler introduction, which we could turn into
the default once more commands start honoring the attribute and
wider adoption proves that it is a good feature, while allowing us a
room to back out by keeping it an optional feature and eventually
deprecate and remove if the experiment did not pan out.

> +--keep-precious::
> +	Do not remove untracked or ignored files if they have
> +	`precious` attribute.

> +Precious files
> +~~~~~~~~~~~~~~
> +
> +`precious`
> +^^^^^^^^^^
> +
> +This attribute is set on files to indicate that their content is
> +valuable. Some commands will behave slightly different on precious
> +files. linkgit:git-clean[1] may leave precious files alone.


When the second thing that cares about the attribute comes along,
the mention of 'git clean' here will become the first item in a
bullet list, while the second and subsequent ones are listed next to
it.  It may not be bad to start that enumerated list of count 1 from
the get-go, but for now this will do.

> @@ -193,9 +203,16 @@ static int remove_dirs(struct strbuf *path, const char *prefix, int force_flag,
>  
>  		strbuf_setlen(path, len);
>  		strbuf_addstr(path, e->d_name);
> -		if (lstat(path->buf, &st))
> +		if (lstat(path->buf, &st)) {
>  			; /* fall thru */
> -		else if (S_ISDIR(st.st_mode)) {
> +		} else if ((!prefix && skip_precious_file(&the_index, path->buf)) ||
> +			   (prefix && skip_prefix(path->buf, prefix, &rel_path) &&
> +			    skip_precious_file(&the_index, rel_path))) {
> +			quote_path_relative(path->buf, prefix, &quoted);
> +			printf(dry_run ? _(msg_would_skip_precious) : _(msg_skip_precious), quoted.buf);
> +			*dir_gone = 0;
> +			continue;

An attribute is given to something that can be tracked, and a
directory would not get an attribute, because Git does not track
directories (there is a reason why skip_precious_file() takes
&the_index that is passed down the callchain to git_check_attr()).

Triggering this logic before excluding S_ISDIR(st.st_mode) feels
iffy.

But let's assume that being able to say "this directory and anything
(recursively) inside are precious" is a good idea and read on.

> +		} else if (S_ISDIR(st.st_mode)) {
>  			if (remove_dirs(path, prefix, force_flag, dry_run, quiet, &gone))
>  				ret = 1;
>  			if (gone) {
> @@ -915,6 +932,8 @@ int cmd_clean(int argc, const char **argv, const char *prefix)
>  		OPT_BOOL('x', NULL, &ignored, N_("remove ignored files, too")),
>  		OPT_BOOL('X', NULL, &ignored_only,
>  				N_("remove only ignored files")),
> +		OPT_BOOL(0, "keep-precious", &keep_precious,
> +			 N_("do not remove files with 'precious' attribute")),
>  		OPT_END()
>  	};

OK.

> @@ -1019,7 +1038,10 @@ int cmd_clean(int argc, const char **argv, const char *prefix)
>  		if (lstat(abs_path.buf, &st))
>  			continue;
>  
> -		if (S_ISDIR(st.st_mode)) {
> +		if (skip_precious_file(&the_index, item->string)) {
> +			qname = quote_path_relative(item->string, NULL, &buf);
> +			printf(dry_run ? _(msg_would_skip_precious) : _(msg_skip_precious), qname);
> +		} else if (S_ISDIR(st.st_mode)) {
>  			if (remove_dirs(&abs_path, prefix, rm_flags, dry_run, quiet, &gone))

Likewise.

> diff --git a/t/t7300-clean.sh b/t/t7300-clean.sh
> index 7b36954d63..ae600dafb5 100755
> --- a/t/t7300-clean.sh
> +++ b/t/t7300-clean.sh
> @@ -669,4 +669,44 @@ test_expect_success 'git clean -d skips untracked dirs containing ignored files'
>  	test_path_is_missing foo/b/bb
>  '
>  
> +test_expect_success 'git clean -xd --keep-precious leaves precious files alone' '
> +	git init precious &&
> +	(
> +		cd precious &&
> +		test_commit one &&
> +		cat >.gitignore <<-\EOF &&
> +		*.o
> +		*.mak
> +		EOF
> +		cat >.gitattributes <<-\EOF &&
> +		*.mak precious
> +		.gitattributes precious
> +		*.precious precious

If the checking with skip_precious() before S_ISDIR() was
intentional (I cannot quite tell if it is), then this should also
have a pattern that matches a directory and mark it as precious.

On the other hand, if it was merely a thinko and the change does not
intend to allow attaching attributes to directories, then this test
is probably OK as-is.  We also _could_ have a pattern that matches a
directory and attempt to make it precious and then make sure the
directory goes away (i.e. any attribute, including 'precious', on
the directory was meaningless).

> +		EOF
> +		mkdir sub &&
> +		touch one.o sub/two.o one.mak sub/two.mak &&
> +		touch one.untracked two.precious sub/also.precious &&
> +		git clean -fdx --keep-precious &&
> +		test_path_is_missing one.o &&
> +		test_path_is_missing sub/two.o &&
> +		test_path_is_missing one.untracked &&
> +		test_path_is_file .gitattributes &&
> +		test_path_is_file one.mak &&
> +		test_path_is_file sub/two.mak &&
> +		test_path_is_file two.precious &&
> +		test_path_is_file sub/also.precious
> +	)
> +'

> +test_expect_success 'git clean -xd still deletes them all' '

OK, so this is exactly the same command as above, but without the
"--keep-precious" option.  It is good to test both positive and
negative.

> +	test_path_is_file precious/one.mak &&
> +	test_path_is_file precious/sub/two.mak &&
> +	test_path_is_file precious/two.precious &&
> +	test_path_is_file precious/sub/also.precious &&
> +	git -C precious clean -fdx &&
> +	test_path_is_missing precious/one.mak &&
> +	test_path_is_missing precious/sub/two.mak &&
> +	test_path_is_missing precious/two.precious &&
> +	test_path_is_missing precious/sub/also.precious
> +'