git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Junio C Hamano <gitster@pobox.com>
To: "Nguyễn Thái Ngọc Duy" <pclouds@gmail.com>
Cc: avarab@gmail.com, git@vger.kernel.org
Subject: Re: [PATCH] Introduce "precious" file concept
Date: Tue, 09 Apr 2019 20:31:50 +0900	[thread overview]
Message-ID: <xmqqh8b75s9l.fsf@gitster-ct.c.googlers.com> (raw)
In-Reply-To: <20190409102649.22115-1-pclouds@gmail.com> ("Nguyễn Thái Ngọc Duy"'s message of "Tue, 9 Apr 2019 17:26:49 +0700")

Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> 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@gmail.com>
> ---
>  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
> +'

  reply	other threads:[~2019-04-09 11:31 UTC|newest]

Thread overview: 19+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-04-04 10:28 What's cooking in git.git (Apr 2019, #01; Thu, 4) Junio C Hamano
2019-04-04 11:08 ` Duy Nguyen
2019-04-04 21:29   ` Junio C Hamano
2019-04-06 20:28     ` Ævar Arnfjörð Bjarmason
2019-04-08  4:14       ` Junio C Hamano
2019-04-09 10:26         ` [PATCH] Introduce "precious" file concept Nguyễn Thái Ngọc Duy
2019-04-09 11:31           ` Junio C Hamano [this message]
2019-04-10  9:36             ` Duy Nguyen
2019-04-12  1:28               ` Junio C Hamano
2019-04-09 17:44           ` Eric Sunshine
2019-04-12 21:54           ` Ævar Arnfjörð Bjarmason
2019-04-13 10:19             ` Duy Nguyen
2019-04-05  1:05 ` What's cooking in git.git (Apr 2019, #01; Thu, 4) Todd Zullinger
2019-04-05  5:41   ` Junio C Hamano
2019-04-06 19:28 ` Ævar Arnfjörð Bjarmason
2019-04-08  4:18   ` Junio C Hamano
2019-04-06 19:57 ` Ævar Arnfjörð Bjarmason
2019-04-08  4:28   ` Junio C Hamano
2019-04-08 21:18     ` Josh Steadmon

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=xmqqh8b75s9l.fsf@gitster-ct.c.googlers.com \
    --to=gitster@pobox.com \
    --cc=avarab@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=pclouds@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).