Bug report - local (and git ignored) file silently removed after checkout

Bug report - local (and git ignored) file silently removed after checkout

[Bertrand BENOIT]

Hi,

In a (special ?) situation, I've lost a local (and git ignored) file
silently, and I've failed to get it back.

Context:
  - in previous version of my project, the TODO file was versioned
  - in current development version, this file is NO more versioned AND
it is ignored by a gitignore rule, but still at the same place for
better ease of access -> such a way, this file has been locally
updated several times
  - during worflow process, I've finally performed a checkout on a
previous version (in which the TODO file was versioned) -> after that
the file has been silently updated with the old version
  - after modification, I've performed back a checkout on the current
version and this time my TODO file has been fully and silently removed

Questions:
  - is it a behavior regarded as normal ?
  - is there any way to get back the TODO file, in LOCAL version it
was before the first checkout ?
  - in any case, is there any way to be warned (and to make the
checkout fail) of such situation (local ignored file being overwritten
with 'older' not-ignored version) ?

Thx,
Best regards,
TWK

Re: Bug report - local (and git ignored) file silently removed after checkout

[Junio C Hamano]

Bertrand BENOIT <projettwk@users.sourceforge.net> writes:

> Context:
>   - in previous version of my project, the TODO file was versioned
>   - in current development version, this file is NO more versioned AND
> it is ignored by a gitignore rule,...

What you observed is expected with all the versions of Git since the
gitignore mechanism was introduced. The ignored files are "not tracked,
are cruft that can be removed to make room if it is necessary for checking
out a tracked version, and would not want to track (e.g. build artifacts
like '*.o')" (I am not saying that it is the most sane semantics, I am
just stating the fact).

We do not have "not tracked, but precious (e.g. your TODO)" category; from
time to time we have discussed possibilities of adding such to enrich the
categorization, but nobody stepped up to actually do the work of designing
let alone coding such a beast.

Re: Bug report - local (and git ignored) file silently removed after checkout

[Taylor Hedberg]

[-- Attachment #1: Type: text/plain, Size: 624 bytes --]

Junio C Hamano, Sun 2011-11-20 @ 13:16:27-0800:
> We do not have "not tracked, but precious (e.g. your TODO)" category;
> from time to time we have discussed possibilities of adding such to
> enrich the categorization, but nobody stepped up to actually do the
> work of designing let alone coding such a beast.

If I'm not mistaken, files ignored by .git/info/exclude rather than
.gitignore do exhibit this behavior. That is, Git will refuse a checkout
with an error message if it would overwrite a path listed in that file.

I don't know if that's actually intended behavior, but I've noticed it
before and made use of it.

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

Re: Bug report - local (and git ignored) file silently removed after checkout

[Junio C Hamano]

Taylor Hedberg <tmhedberg@gmail.com> writes:

> Junio C Hamano, Sun 2011-11-20 @ 13:16:27-0800:
>> We do not have "not tracked, but precious (e.g. your TODO)" category;
>> from time to time we have discussed possibilities of adding such to
>> enrich the categorization, but nobody stepped up to actually do the
>> work of designing let alone coding such a beast.
>
> If I'm not mistaken, files ignored by .git/info/exclude rather than
> .gitignore do exhibit this behavior.

As far as I am aware, info/exclude should work exactly the same as having
a .gitignore file at the root level of the working tree. Can you show a
minimum reproduction recipe in a form of a patch to our test scripts in t/
hierarchy?

[PATCH] Add test that checkout does not overwrite entries in .git/info/exclude

[Johannes Sixt]

From: Johannes Sixt <j6t@kdbg.org>

It is an unintended accident that entries matched by .git/info/exclude are
considered precious, but entries matched by .gitignore are not. That is,
'git checkout' will overwrite untracked files matched by .gitignore, but
refuses to overwrite files matched by .git/info/exclude.

It is a lucky accident: it allows the distinction between "untracked but
precious" and "untracked and garbage". And it is a doubly lucky accident:
.gitignore entries are meant for files like build products, which usually
affect all consumers of a repository, whereas .git/info/exclude is
intended for personal files, which frequently are precious (think of a
TODO file).

Add a test that codifies the accident as wanted behavior.

Signed-off-by: Johannes Sixt <j6t@kdbg.org>
---
Am 11/21/2011 4:36, schrieb Junio C Hamano:
> As far as I am aware, info/exclude should work exactly the same as having
> a .gitignore file at the root level of the working tree. Can you show a
> minimum reproduction recipe in a form of a patch to our test scripts in t/
> hierarchy?

Here you are. As you can see from my commit message, IMO, this is
a very useful accident. Therefore, there is no 'test_expect_failure'
in the test script :-)

-- Hannes

 t/t2023-checkout-ignored.sh |   51 +++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 51 insertions(+), 0 deletions(-)
 create mode 100755 t/t2023-checkout-ignored.sh

diff --git a/t/t2023-checkout-ignored.sh b/t/t2023-checkout-ignored.sh
new file mode 100755
index 0000000..03a5a56
--- /dev/null
+++ b/t/t2023-checkout-ignored.sh
@@ -0,0 +1,51 @@
+#!/bin/sh
+
+test_description='checkout overwrites or preserves ignored files
+
+`git checkout` makes a distinction between files mentioned in
+.gitignore and .git/info/exclude in that untracked files matched
+by the latter are considered precious and are not overwritten.
+'
+
+. ./test-lib.sh
+
+test_expect_success setup '
+
+	echo excluded > excluded &&
+	echo ignored > ignored &&
+	git add . &&
+	test_commit initial &&
+	git checkout -b side &&
+	git rm excluded &&
+	git mv ignored .gitignore &&
+	test_commit side &&
+	echo excluded >> .git/info/exclude
+'
+
+test_expect_success 'files are ignored' '
+
+	echo keep > excluded &&
+	echo overwrite > ignored &&
+	list=$(git ls-files --others --exclude-standard) &&
+	test -z "$list"
+'
+
+test_expect_success 'entries in .git/info/exclude are precious' '
+
+	test_must_fail git checkout master 2>errors &&
+	test_i18ngrep "would be overwritten" errors &&
+	grep "	excluded" errors &&
+	! grep "	ignored" errors &&
+	grep keep excluded &&
+	grep overwrite ignored
+'
+
+test_expect_success 'entries in .gitignore are not precious' '
+
+	rm -f excluded &&
+	git checkout master &&
+	grep excluded excluded &&
+	grep ignored ignored
+'
+
+test_done
-- 
1.7.8.rc0.126.gd15506

Re: [PATCH] Add test that checkout does not overwrite entries in .git/info/exclude

[Philip Oakley]

From: "Johannes Sixt" <j.sixt@viscovery.net>
> From: Johannes Sixt <j6t@kdbg.org>
>
> It is an unintended accident that entries matched by .git/info/exclude are
> considered precious, but entries matched by .gitignore are not. That is,
> 'git checkout' will overwrite untracked files matched by .gitignore, but
> refuses to overwrite files matched by .git/info/exclude.
>
> It is a lucky accident: it allows the distinction between "untracked but
> precious" and "untracked and garbage". And it is a doubly lucky accident:
> .gitignore entries are meant for files like build products, which usually
> affect all consumers of a repository, whereas .git/info/exclude is
> intended for personal files, which frequently are precious (think of a
> TODO file).
>
> Add a test that codifies the accident as wanted behavior.
>
> Signed-off-by: Johannes Sixt <j6t@kdbg.org>
> ---
> Am 11/21/2011 4:36, schrieb Junio C Hamano:
>> As far as I am aware, info/exclude should work exactly the same as having
>> a .gitignore file at the root level of the working tree. Can you show a
>> minimum reproduction recipe in a form of a patch to our test scripts in 
>> t/
>> hierarchy?
>
> Here you are. As you can see from my commit message, IMO, this is
> a very useful accident. Therefore, there is no 'test_expect_failure'
> in the test script :-)

Shouldn't there be some documentation changes to support this very useful 
feature. By documenting the existing code functionality  we get the double 
benefit of no code changes and we publish the existance of a desired bit of 
functionality.
Philip

>
> -- Hannes
>
> t/t2023-checkout-ignored.sh |   51 
> +++++++++++++++++++++++++++++++++++++++++++
> 1 files changed, 51 insertions(+), 0 deletions(-)
> create mode 100755 t/t2023-checkout-ignored.sh
>
> diff --git a/t/t2023-checkout-ignored.sh b/t/t2023-checkout-ignored.sh
> new file mode 100755
> index 0000000..03a5a56
> --- /dev/null
> +++ b/t/t2023-checkout-ignored.sh
> @@ -0,0 +1,51 @@
> +#!/bin/sh
> +
> +test_description='checkout overwrites or preserves ignored files
> +
> +`git checkout` makes a distinction between files mentioned in
> +.gitignore and .git/info/exclude in that untracked files matched
> +by the latter are considered precious and are not overwritten.
> +'
> +
> +. ./test-lib.sh
> +
> +test_expect_success setup '
> +
> + echo excluded > excluded &&
> + echo ignored > ignored &&
> + git add . &&
> + test_commit initial &&
> + git checkout -b side &&
> + git rm excluded &&
> + git mv ignored .gitignore &&
> + test_commit side &&
> + echo excluded >> .git/info/exclude
> +'
> +
> +test_expect_success 'files are ignored' '
> +
> + echo keep > excluded &&
> + echo overwrite > ignored &&
> + list=$(git ls-files --others --exclude-standard) &&
> + test -z "$list"
> +'
> +
> +test_expect_success 'entries in .git/info/exclude are precious' '
> +
> + test_must_fail git checkout master 2>errors &&
> + test_i18ngrep "would be overwritten" errors &&
> + grep " excluded" errors &&
> + ! grep " ignored" errors &&
> + grep keep excluded &&
> + grep overwrite ignored
> +'
> +
> +test_expect_success 'entries in .gitignore are not precious' '
> +
> + rm -f excluded &&
> + git checkout master &&
> + grep excluded excluded &&
> + grep ignored ignored
> +'
> +
> +test_done
> -- 
> 1.7.8.rc0.126.gd15506
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
>
> -----
> No virus found in this message.
> Checked by AVG - www.avg.com
> Version: 2012.0.1872 / Virus Database: 2092/4628 - Release Date: 11/20/11
> 

Re: [PATCH] Add test that checkout does not overwrite entries in .git/info/exclude

[Junio C Hamano]

Johannes Sixt <j.sixt@viscovery.net> writes:

> It is a lucky accident: it allows the distinction between "untracked but
> precious" and "untracked and garbage". And it is a doubly lucky accident:
> .gitignore entries are meant for files like build products, which usually
> affect all consumers of a repository, whereas .git/info/exclude is
> intended for personal files, which frequently are precious (think of a
> TODO file).
> ...
> Here you are. As you can see from my commit message, IMO, this is
> a very useful accident. Therefore, there is no 'test_expect_failure'
> in the test script :-)

Heh.

If this is a feature, we would like a patch to Documentation/gitignore.txt
as well, I think.  Also I have a suspicion that this was an unintended
"regression"; do you have a bisection?

Re: [PATCH] Add test that checkout does not overwrite entries in .git/info/exclude

[Junio C Hamano]

"Philip Oakley" <philipoakley@iee.org> writes:

> Shouldn't there be some documentation changes to support this very
> useful feature.

I do not agree it is "useful", if it is info/exclude only, which by design
would not propagate across repositories and histories.

If we were to support "precious" as a feature, we should do that properly
with a properly defined syntax (e.g. just like '!' prefix is "no, this is
not 'ignored' entry", use some prefix to say "well, this is ignored in the
sense that not to be tracked, but that does not mean this can be nuked")
that can be in any exclude sources.

Exclude patterns in info/exclude should work just like ones in the in-tree
.gitignore only at different precedence for uniformity and consistency, so
the behaviour J6t showed in the new test should be fixed.

The entries in info/exclude are at lower priority than the ones in in-tree
.gitignore files, which probably was a design mistake, but that is a
separate issue.

Re: [PATCH] Add test that checkout does not overwrite entries in .git/info/exclude

[Nguyen Thai Ngoc Duy]

On Mon, Nov 21, 2011 at 08:23:45AM +0100, Johannes Sixt wrote:
> From: Johannes Sixt <j6t@kdbg.org>
> 
> It is an unintended accident that entries matched by .git/info/exclude are
> considered precious, but entries matched by .gitignore are not. That is,
> 'git checkout' will overwrite untracked files matched by .gitignore, but
> refuses to overwrite files matched by .git/info/exclude.
> 
> It is a lucky accident: it allows the distinction between "untracked but
> precious" and "untracked and garbage". And it is a doubly lucky accident:
> .gitignore entries are meant for files like build products, which usually
> affect all consumers of a repository, whereas .git/info/exclude is
> intended for personal files, which frequently are precious (think of a
> TODO file).
> 
> Add a test that codifies the accident as wanted behavior.

If you want to keep this accident (which is a bug to me), you may want
to add the reason: callers to unpack_trees() are supposed to also
setup exclude rules in .git/info/exclude before calling
unpack_trees(), which they don't.

So .git/info/exclude is entirely dismissed. This patch makes t2023.3
fail. I haven't dug into history to see if this is on purpose though.

-- 8< --
diff --git a/builtin/checkout.c b/builtin/checkout.c
index 2a80772..c2fc2ba 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -412,6 +412,8 @@ 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";
+		if (!access(git_path("info/exclude"), R_OK))
+			add_excludes_from_file(topts.dir, git_path("info/exclude"));
 		tree = parse_tree_indirect(old->commit ?
 					   old->commit->object.sha1 :
 					   EMPTY_TREE_SHA1_BIN);
-- 8< --
--
Duy

Re: [PATCH] Add test that checkout does not overwrite entries in .git/info/exclude

[Nguyen Thai Ngoc Duy]

On Mon, Nov 21, 2011 at 2:50 PM, Junio C Hamano <gitster@pobox.com> wrote:
> If this is a feature, we would like a patch to Documentation/gitignore.txt
> as well, I think.  Also I have a suspicion that this was an unintended
> "regression"; do you have a bisection?

It dated back to f8a9d42 (read-tree: further loosen "working file will
be lost" check. - 2006-12-04) when you introduced
--exclude-per-directory to read-tree, but not --exclude-from to
explicitly add .git/info/exclude. "read tree --exclude-per-directory"
is used in git-checkout.sh and the same logic is carried over to
builtin-checkout.c until today.

I guess it's time to add "read-tree --exclude-from". Not sure what to
do with git-checkout though. We may add an option to checkout to skip
either .git/info/exclude or .gitignore, or both.
-- 
Duy

Re: [PATCH] Add test that checkout does not overwrite entries in .git/info/exclude

[Junio C Hamano]

Nguyen Thai Ngoc Duy <pclouds@gmail.com> writes:

> If you want to keep this accident (which is a bug to me), you may want
> to add the reason: callers to unpack_trees() are supposed to also
> setup exclude rules in .git/info/exclude before calling
> unpack_trees(), which they don't.
>
> So .git/info/exclude is entirely dismissed.

Ohh, thanks for this and also for digging this through in your other
message.

I think it is the right thing to do to make sure .gitignore and
info/exclude behave the same way regardless of the original issue in this
topic.

In the medium term, I think one reasonable way forward solving the "TODO
that used to be tracked but now untracked and ignored" issue is to
introduce "info/exclude-override" that comes between command line and
in-tree patterns. The info/exclude file is designed as the fallback
definition to be used when all other sources are too lax, and comes near
the precedence stack; the "TODO" situation however calls for an override
that is stronger than the in-tree patterns.

In the longer term, we should carefully determine if we need "precious" in
the first place. The last time this was brought up there were people who
argued they are OK with having to remove the ignored file by hand when
checking out another branch (i.e. we switch the semantics of "ignored" so
that they are "not tracked but all precious").

I think it matters in two cases.

 (1) If you change an untracked "cruft" file on branch A into a directory
     with tracked files in it on another branch B. If you are on branch A,
     have that "cruft" file (perhaps it is a build product after running
     "make"), and try to checkout branch B, such an updated "git checkout"
     will start erroring out telling you that "cruft" will be lost.

 (2) If you have a directory on branch A, underneath of which there are
     untracked "cruft" files (e.g. think "build/" directory that is full
     of "*.o" files and ".gitignore" to mark object files as ignored but
     is otherwise empty), and another branch B that has the same path as a
     file. If you are on branch A, have "cruft" files in that directory,
     and try to checkout branch B, such an updated "git checkout" will
     start erroring out telling you that "cruft" will be lost.

If people are OK with such a behaviour, we can do without "precious".

Otherwise we would need to update excluded() in dir.c to return tristate
(ignored, precious or unspecified) instead of the current boolean (ignored
or unspecified), examine and decide for each caller what they want to do
to "precious" files.

Re: [PATCH] Add test that checkout does not overwrite entries in .git/info/exclude

[Bertrand BENOIT]

2011/11/21 Junio C Hamano <gitster@pobox.com>:
> Nguyen Thai Ngoc Duy <pclouds@gmail.com> writes:
>
>> If you want to keep this accident (which is a bug to me), you may want
>> to add the reason: callers to unpack_trees() are supposed to also
>> setup exclude rules in .git/info/exclude before calling
>> unpack_trees(), which they don't.
>>
>> So .git/info/exclude is entirely dismissed.
>
> Ohh, thanks for this and also for digging this through in your other
> message.
>
> I think it is the right thing to do to make sure .gitignore and
> info/exclude behave the same way regardless of the original issue in this
> topic.
>
> In the medium term, I think one reasonable way forward solving the "TODO
> that used to be tracked but now untracked and ignored" issue is to
> introduce "info/exclude-override" that comes between command line and
> in-tree patterns. The info/exclude file is designed as the fallback
> definition to be used when all other sources are too lax, and comes near
> the precedence stack; the "TODO" situation however calls for an override
> that is stronger than the in-tree patterns.
>
> In the longer term, we should carefully determine if we need "precious" in
> the first place. The last time this was brought up there were people who
> argued they are OK with having to remove the ignored file by hand when
> checking out another branch (i.e. we switch the semantics of "ignored" so
> that they are "not tracked but all precious").
>
> I think it matters in two cases.
>
>  (1) If you change an untracked "cruft" file on branch A into a directory
>     with tracked files in it on another branch B. If you are on branch A,
>     have that "cruft" file (perhaps it is a build product after running
>     "make"), and try to checkout branch B, such an updated "git checkout"
>     will start erroring out telling you that "cruft" will be lost.
>
>  (2) If you have a directory on branch A, underneath of which there are
>     untracked "cruft" files (e.g. think "build/" directory that is full
>     of "*.o" files and ".gitignore" to mark object files as ignored but
>     is otherwise empty), and another branch B that has the same path as a
>     file. If you are on branch A, have "cruft" files in that directory,
>     and try to checkout branch B, such an updated "git checkout" will
>     start erroring out telling you that "cruft" will be lost.
>
> If people are OK with such a behaviour, we can do without "precious".
>
> Otherwise we would need to update excluded() in dir.c to return tristate
> (ignored, precious or unspecified) instead of the current boolean (ignored
> or unspecified), examine and decide for each caller what they want to do
> to "precious" files.

IMO, as user, I think that erroring out telling you that 'cruft' will
be lost, is enough to avoid data loss.
However, in addition the tristate system (with a dedicated syntax in
.gitattributes and/or .gitignore files) would give more freedom,
without having to move 'untracked but precious' files between each
updated 'git checkout'.

Re: [PATCH] Add test that checkout does not overwrite entries in .git/info/exclude

[Junio C Hamano]

Nguyen Thai Ngoc Duy <pclouds@gmail.com> writes:

> It dated back to f8a9d42 (read-tree: further loosen "working file will
> be lost" check. - 2006-12-04) when you introduced --exclude-per-directory
> to read-tree, but not --exclude-from to explicitly add .git/info/exclude.

That one together with 1127148 (Loosen "working file will be lost" check
in Porcelain-ish, 2006-12-04) complets the picture. Thanks for digging
this out.

> I guess it's time to add "read-tree --exclude-from".

Perhaps, also --exclude-standard to match?

> We may add an option to checkout to skip
> either .git/info/exclude or .gitignore, or both.

I do not think we want to go that route, because it is not just "checkout"
but is an issue shared across users of unpack-trees machinery.

When a merge result loses build/ directory and replaces it with a regular
file "build", with the current ignore semantics, build/*.o will go but if
you had an untracked and ignored build/TODO, the file will also go, for
example. If we want to have "precious", we should either make "ignored" to
mean "precious", or introduce a "precious" as a new category
(cf. $gmane/185746)

Re: [PATCH] Add test that checkout does not overwrite entries in .git/info/exclude

[Nguyen Thai Ngoc Duy]

On Tue, Nov 22, 2011 at 12:27 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Nguyen Thai Ngoc Duy <pclouds@gmail.com> writes:
>
>> It dated back to f8a9d42 (read-tree: further loosen "working file will
>> be lost" check. - 2006-12-04) when you introduced --exclude-per-directory
>> to read-tree, but not --exclude-from to explicitly add .git/info/exclude.
>
> That one together with 1127148 (Loosen "working file will be lost" check
> in Porcelain-ish, 2006-12-04) complets the picture. Thanks for digging
> this out.
>
>> I guess it's time to add "read-tree --exclude-from".
>
> Perhaps, also --exclude-standard to match?

Yep, will do.
-- 
Duy

Re: [PATCH] Add test that checkout does not overwrite entries in .git/info/exclude

[Nguyen Thai Ngoc Duy]

On Mon, Nov 21, 2011 at 10:18 PM, Junio C Hamano <gitster@pobox.com> wrote:
> In the medium term, I think one reasonable way forward solving the "TODO
> that used to be tracked but now untracked and ignored" issue is to
> introduce "info/exclude-override" that comes between command line and
> in-tree patterns. The info/exclude file is designed as the fallback
> definition to be used when all other sources are too lax, and comes near
> the precedence stack; the "TODO" situation however calls for an override
> that is stronger than the in-tree patterns.

"info/precious" might be a better name

> In the longer term, we should carefully determine if we need "precious" in
> the first place. The last time this was brought up there were people who
> argued they are OK with having to remove the ignored file by hand when
> checking out another branch (i.e. we switch the semantics of "ignored" so
> that they are "not tracked but all precious").
>
> I think it matters in two cases.
>
>  (1) If you change an untracked "cruft" file on branch A into a directory
>     with tracked files in it on another branch B. If you are on branch A,
>     have that "cruft" file (perhaps it is a build product after running
>     "make"), and try to checkout branch B, such an updated "git checkout"
>     will start erroring out telling you that "cruft" will be lost.
>
>  (2) If you have a directory on branch A, underneath of which there are
>     untracked "cruft" files (e.g. think "build/" directory that is full
>     of "*.o" files and ".gitignore" to mark object files as ignored but
>     is otherwise empty), and another branch B that has the same path as a
>     file. If you are on branch A, have "cruft" files in that directory,
>     and try to checkout branch B, such an updated "git checkout" will
>     start erroring out telling you that "cruft" will be lost.

I think we should do this regardless precious being added or not.

> If people are OK with such a behaviour, we can do without "precious".

What about git-clean to remove ignored but not precious files?

> Otherwise we would need to update excluded() in dir.c to return tristate
> (ignored, precious or unspecified) instead of the current boolean (ignored
> or unspecified), examine and decide for each caller what they want to do
> to "precious" files.

Or do excluded() twice, the first time to check for precious files,
the second for all ignored rules. Callers are changed anyway, but this
way git-add for example will be untouched because it does not care
about precious stuff. Only unpack-trees and maybe git-clean are
changed.
-- 
Duy

Re: [PATCH] Add test that checkout does not overwrite entries in .git/info/exclude

[Junio C Hamano]

Nguyen Thai Ngoc Duy <pclouds@gmail.com> writes:

> On Mon, Nov 21, 2011 at 10:18 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> In the medium term, I think one reasonable way forward solving the "TODO
>> that used to be tracked but now untracked and ignored" issue is to
>> introduce "info/exclude-override" that comes between command line and
>> in-tree patterns. The info/exclude file is designed as the fallback
>> definition to be used when all other sources are too lax, and comes near
>> the precedence stack; the "TODO" situation however calls for an override
>> that is stronger than the in-tree patterns.
>
> "info/precious" might be a better name

The above is only about the precedence order and is not about introducing
the new "precious" class at all.

>> In the longer term, we should carefully determine if we need "precious" in
>> the first place. The last time this was brought up there were people who
>> argued they are OK with having to remove the ignored file by hand when
>> checking out another branch (i.e. we switch the semantics of "ignored" so
>> that they are "not tracked but all precious").
>>
>> I think it matters in two cases.
>>
>>  (1) If you change an untracked "cruft" file on branch A into a directory
>>     with tracked files in it on another branch B. If you are on branch A,
>>     have that "cruft" file (perhaps it is a build product after running
>>     "make"), and try to checkout branch B, such an updated "git checkout"
>>     will start erroring out telling you that "cruft" will be lost.
>>
>>  (2) If you have a directory on branch A, underneath of which there are
>>     untracked "cruft" files (e.g. think "build/" directory that is full
>>     of "*.o" files and ".gitignore" to mark object files as ignored but
>>     is otherwise empty), and another branch B that has the same path as a
>>     file. If you are on branch A, have "cruft" files in that directory,
>>     and try to checkout branch B, such an updated "git checkout" will
>>     start erroring out telling you that "cruft" will be lost.
>
> I think we should do this regardless precious being added or not.

Because (see below)?

>> If people are OK with such a behaviour, we can do without "precious".
>
> What about git-clean to remove ignored but not precious files?

"clean" without -x is a way to remove untracked and not ignored files,
i.e. remove "test.c", "trash-patch", "notes" that are not part of the
sources but were crufts you hand generated during your development process
that you do not need, without removing build products such as "main.o". If
we switch the semantics of "ignored" from "untracked and is expendable for
the purpose of checking out another conflicting branch" to "untracked but
is not expendable", it is clear that it should not remove them.

"clean -x" is more subtle. It has been a way to say "Remove cruft the
usual way, and in addition, remove the expendable build products, just
like 'make clean' _should_ do, but I do not trust my Makefile". If we
introduced "precious", it would be very clear what it should do---even
with "-x" precious files should be kept. But if we don't and just try to
get away by changing the semantics of "ignored", they will still need to
be removed, so we won't really get the "precious".

The conclusion from this is that it is a mistake to change the semantics
of "ignored" from the current "untracked and expendable if needed" if the
purpose of that change is to avoid introducing the new "precious" class.

I don't care too much about it, as I do not use "git clean -x" myself ;-)
but that wouldn't stop others from think about the issue and try to come
up with a good solid design.

> Or do excluded() twice, the first time to check for precious files,
> the second for all ignored rules. Callers are changed anyway, but this
> way git-add for example will be untouched because it does not care
> about precious stuff. Only unpack-trees and maybe git-clean are
> changed.

I don't think we want to go there, as it will encourage different
codepaths doing different things without a good reason. Having to add
ignore source manually in different codepaths was the real cause of the
inconsistency bug around info/exclude vs .gitignore we discussed earlier.

Re: [PATCH] Add test that checkout does not overwrite entries in .git/info/exclude

[Nguyen Thai Ngoc Duy]

On Thu, Nov 24, 2011 at 12:16 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Nguyen Thai Ngoc Duy <pclouds@gmail.com> writes:
>
>> On Mon, Nov 21, 2011 at 10:18 PM, Junio C Hamano <gitster@pobox.com> wrote:
>>> In the medium term, I think one reasonable way forward solving the "TODO
>>> that used to be tracked but now untracked and ignored" issue is to
>>> introduce "info/exclude-override" that comes between command line and
>>> in-tree patterns. The info/exclude file is designed as the fallback
>>> definition to be used when all other sources are too lax, and comes near
>>> the precedence stack; the "TODO" situation however calls for an override
>>> that is stronger than the in-tree patterns.
>>
>> "info/precious" might be a better name
>
> The above is only about the precedence order and is not about introducing
> the new "precious" class at all.

The way I see how info/exclude-override helps keep (ignored but)
precious files is it un-ignores files that are ignored by info/exclude
and .gitignore. But if we go that way, then git-add will also
un-ignore precious files, allowing users to add them in-tree by
mistake.

>>> In the longer term, we should carefully determine if we need "precious" in
>>> the first place. The last time this was brought up there were people who
>>> argued they are OK with having to remove the ignored file by hand when
>>> checking out another branch (i.e. we switch the semantics of "ignored" so
>>> that they are "not tracked but all precious").
>>>
>>> I think it matters in two cases.
>>>
>>>  (1) If you change an untracked "cruft" file on branch A into a directory
>>>     with tracked files in it on another branch B. If you are on branch A,
>>>     have that "cruft" file (perhaps it is a build product after running
>>>     "make"), and try to checkout branch B, such an updated "git checkout"
>>>     will start erroring out telling you that "cruft" will be lost.
>>>
>>>  (2) If you have a directory on branch A, underneath of which there are
>>>     untracked "cruft" files (e.g. think "build/" directory that is full
>>>     of "*.o" files and ".gitignore" to mark object files as ignored but
>>>     is otherwise empty), and another branch B that has the same path as a
>>>     file. If you are on branch A, have "cruft" files in that directory,
>>>     and try to checkout branch B, such an updated "git checkout" will
>>>     start erroring out telling you that "cruft" will be lost.
>>
>> I think we should do this regardless precious being added or not.
>
> Because (see below)?

Because it may potentially lose user's changes. Assume file "A" is
tracked and also ignored. Users may make some changes in A, then move
HEAD away without touching worktree, now HEAD does not see "A" as
tracked any more. If we switch to a branch that has A tracked, git may
override A and wipe our user changes in that file. In short, if a file
is tracked (either in source or target in 2-way merge) then we ignore
excluded() result on that file.

>>> If people are OK with such a behaviour, we can do without "precious".
>>
>> What about git-clean to remove ignored but not precious files?
>
(git clean without -x snipped because I was talking about -x)
>
> "clean -x" is more subtle. It has been a way to say "Remove cruft the
> usual way, and in addition, remove the expendable build products, just
> like 'make clean' _should_ do, but I do not trust my Makefile". If we
> introduced "precious", it would be very clear what it should do---even
> with "-x" precious files should be kept. But if we don't and just try to
> get away by changing the semantics of "ignored", they will still need to
> be removed, so we won't really get the "precious".
>
> The conclusion from this is that it is a mistake to change the semantics
> of "ignored" from the current "untracked and expendable if needed" if the
> purpose of that change is to avoid introducing the new "precious" class.
>
> I don't care too much about it, as I do not use "git clean -x" myself ;-)
> but that wouldn't stop others from think about the issue and try to come
> up with a good solid design.

I don't use "git clean -x" myself either. This is just some input to
the discussion and true users are supposed to speak up. The point I
want to make is, if users really want the ability remove non-precious
ignored files, then we may need to add the new "precious" class that
(and you seem to reach the same conclusion).
-- 
Duy

Re: [PATCH] Add test that checkout does not overwrite entries in .git/info/exclude

[Junio C Hamano]

Nguyen Thai Ngoc Duy <pclouds@gmail.com> writes:

> ...
>>> I think we should do this regardless precious being added or not.
>>
>> Because (see below)?
>
> Because it may potentially lose user's changes. Assume file "A" is
> tracked and also ignored. Users may make some changes in A, then move
> HEAD away without touching worktree, now HEAD does not see "A" as
> tracked any more.

Huh? "A" is initially tracked and the user modifies it. Moving HEAD away
how?  "git checkout" would refuse to check out a branch that has different
contents at "A", or is missing "A". So how can "now HEAD does not see 'A'
as tracked any more" happen?

Re: [PATCH] Add test that checkout does not overwrite entries in .git/info/exclude

[Nguyen Thai Ngoc Duy]

On Thu, Nov 24, 2011 at 12:17 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Nguyen Thai Ngoc Duy <pclouds@gmail.com> writes:
>
>> ...
>>>> I think we should do this regardless precious being added or not.
>>>
>>> Because (see below)?
>>
>> Because it may potentially lose user's changes. Assume file "A" is
>> tracked and also ignored. Users may make some changes in A, then move
>> HEAD away without touching worktree, now HEAD does not see "A" as
>> tracked any more.
>
> Huh? "A" is initially tracked and the user modifies it. Moving HEAD away
> how?  "git checkout" would refuse to check out a branch that has different
> contents at "A", or is missing "A". So how can "now HEAD does not see 'A'
> as tracked any more" happen?

git reset <somewhere else>
-- 
Duy

Re: [PATCH] Add test that checkout does not overwrite entries in .git/info/exclude

[Nguyen Thai Ngoc Duy]

On Mon, Nov 21, 2011 at 10:18 PM, Junio C Hamano <gitster@pobox.com> wrote:
> In the medium term, I think one reasonable way forward solving the "TODO
> that used to be tracked but now untracked and ignored" issue is to
> introduce "info/exclude-override" that comes between command line and
> in-tree patterns. The info/exclude file is designed as the fallback
> definition to be used when all other sources are too lax, and comes near
> the precedence stack; the "TODO" situation however calls for an override
> that is stronger than the in-tree patterns.

Short term, should we allow an option to disregard ignored status
(i.e. all files are precious)? Something like [2/2]

[1/2] checkout,merge: loosen overwriting untracked file check based on
info/exclude
[2/2] checkout,merge: disallow overwriting ignored files with
--no-overwrite-ignore
-- 
Duy

[PATCH 1/2] checkout,merge: loosen overwriting untracked file check based on info/exclude

[Nguyễn Thái Ngọc Duy]

Back in 1127148 (Loosen "working file will be lost" check in
Porcelain-ish - 2006-12-04), git-checkout.sh learned to quietly
overwrite ignored files. Howver the code only took .gitignore files
into account.

Standard ignored files include all specified in .gitignore files in
working directory _and_ $GIT_DIR/info/exclude. This patch makes sure
ignored files in info/exclude can also be overwritten automatically in
the spirit of the original patch.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 builtin/checkout.c |    2 +-
 builtin/merge.c    |    2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/builtin/checkout.c b/builtin/checkout.c
index 2a80772..51840b9 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -411,7 +411,7 @@ static int merge_working_tree(struct checkout_opts *opts,
 		topts.fn = twoway_merge;
 		topts.dir = xcalloc(1, sizeof(*topts.dir));
 		topts.dir->flags |= DIR_SHOW_IGNORED;
-		topts.dir->exclude_per_dir = ".gitignore";
+		setup_standard_excludes(topts.dir);
 		tree = parse_tree_indirect(old->commit ?
 					   old->commit->object.sha1 :
 					   EMPTY_TREE_SHA1_BIN);
diff --git a/builtin/merge.c b/builtin/merge.c
index 2870a6a..1387376 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -775,7 +775,7 @@ int checkout_fast_forward(const unsigned char *head, const unsigned char *remote
 	memset(&t, 0, sizeof(t));
 	memset(&dir, 0, sizeof(dir));
 	dir.flags |= DIR_SHOW_IGNORED;
-	dir.exclude_per_dir = ".gitignore";
+	setup_standard_excludes(&dir);
 	opts.dir = &dir;
 
 	opts.head_idx = 1;
-- 
1.7.4.74.g639db

[PATCH 2/2] checkout,merge: disallow overwriting ignored files with --no-overwrite-ignore

[Nguyễn Thái Ngọc Duy]

Ignored files usually are generated files (e.g. .o files) and can be
safely discarded. However sometimes users may have important files in
working directory, but still want a clean "git status", so they mark
them as ignored files. But in this case, these files should not be
overwritten without asking first.

Enable this use case with --no-overwrite-ignore, where git only sees
tracked and untracked files, no ignored files. Those who mix
discardable ignored files with important ones may have to sort it out
themselves.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 builtin/checkout.c |   11 ++++++++---
 builtin/merge.c    |   12 ++++++++----
 2 files changed, 16 insertions(+), 7 deletions(-)

diff --git a/builtin/checkout.c b/builtin/checkout.c
index 51840b9..5f9474d 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -34,6 +34,7 @@ struct checkout_opts {
 	int force_detach;
 	int writeout_stage;
 	int writeout_error;
+	int overwrite_ignore;
 
 	/* not set by parse_options */
 	int branch_exists;
@@ -409,9 +410,11 @@ static int merge_working_tree(struct checkout_opts *opts,
 		topts.gently = opts->merge && old->commit;
 		topts.verbose_update = !opts->quiet;
 		topts.fn = twoway_merge;
-		topts.dir = xcalloc(1, sizeof(*topts.dir));
-		topts.dir->flags |= DIR_SHOW_IGNORED;
-		setup_standard_excludes(topts.dir);
+		if (opts->overwrite_ignore) {
+			topts.dir = xcalloc(1, sizeof(*topts.dir));
+			topts.dir->flags |= DIR_SHOW_IGNORED;
+			setup_standard_excludes(topts.dir);
+		}
 		tree = parse_tree_indirect(old->commit ?
 					   old->commit->object.sha1 :
 					   EMPTY_TREE_SHA1_BIN);
@@ -926,6 +929,7 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)
 			    3),
 		OPT__FORCE(&opts.force, "force checkout (throw away local modifications)"),
 		OPT_BOOLEAN('m', "merge", &opts.merge, "perform a 3-way merge with the new branch"),
+		OPT_BOOLEAN(0, "overwrite-ignore", &opts.overwrite_ignore, "update ignored files (default)"),
 		OPT_STRING(0, "conflict", &conflict_style, "style",
 			   "conflict style (merge or diff3)"),
 		OPT_BOOLEAN('p', "patch", &patch_mode, "select hunks interactively"),
@@ -937,6 +941,7 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)
 
 	memset(&opts, 0, sizeof(opts));
 	memset(&new, 0, sizeof(new));
+	opts.overwrite_ignore = 1;
 
 	gitmodules_config();
 	git_config(git_checkout_config, &opts);
diff --git a/builtin/merge.c b/builtin/merge.c
index 1387376..07102c4 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -48,6 +48,7 @@ static int show_diffstat = 1, shortlog_len, squash;
 static int option_commit = 1, allow_fast_forward = 1;
 static int fast_forward_only, option_edit;
 static int allow_trivial = 1, have_message;
+static int overwrite_ignore = 1;
 static struct strbuf merge_msg;
 static struct commit_list *remoteheads;
 static struct strategy **use_strategies;
@@ -207,6 +208,7 @@ static struct option builtin_merge_options[] = {
 	OPT_BOOLEAN(0, "abort", &abort_current_merge,
 		"abort the current in-progress merge"),
 	OPT_SET_INT(0, "progress", &show_progress, "force progress reporting", 1),
+	OPT_BOOLEAN(0, "overwrite-ignore", &overwrite_ignore, "update ignored files (default)"),
 	OPT_END()
 };
 
@@ -773,10 +775,12 @@ int checkout_fast_forward(const unsigned char *head, const unsigned char *remote
 	memset(&trees, 0, sizeof(trees));
 	memset(&opts, 0, sizeof(opts));
 	memset(&t, 0, sizeof(t));
-	memset(&dir, 0, sizeof(dir));
-	dir.flags |= DIR_SHOW_IGNORED;
-	setup_standard_excludes(&dir);
-	opts.dir = &dir;
+	if (overwrite_ignore) {
+		memset(&dir, 0, sizeof(dir));
+		dir.flags |= DIR_SHOW_IGNORED;
+		setup_standard_excludes(&dir);
+		opts.dir = &dir;
+	}
 
 	opts.head_idx = 1;
 	opts.src_index = &the_index;
-- 
1.7.4.74.g639db

Re: [PATCH 1/2] checkout,merge: loosen overwriting untracked file check based on info/exclude

[Junio C Hamano]

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

> Back in 1127148 (Loosen "working file will be lost" check in
> Porcelain-ish - 2006-12-04), git-checkout.sh learned to quietly
> overwrite ignored files. Howver the code only took .gitignore files
> into account.

v1.5.3.5-721-g039bc64 also tried to make it harder to make this kind of
mistake, and forgot to spot it in git-checkout.sh which was a bit
unfortunate.

Thanks. I think both patches make sense, and especially 2/2 opens the door
to possibly make ignored ones automatically precious.