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

Nguyen Thai Ngoc Duy [off-list ref] 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.

2011/11/21 Junio C Hamano [off-list ref]:

Nguyen Thai Ngoc Duy [off-list ref] writes:

quoted

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'.

On Mon, Nov 21, 2011 at 10:18 PM, Junio C Hamano [off-list ref] 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

On Mon, Nov 21, 2011 at 10:18 PM, Junio C Hamano [off-list ref] 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

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 <redacted>
---
 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

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 <redacted>
---
 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

Nguyễn Thái Ngọc Duy [off-list ref] 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.