Comment important codepaths regarding nuking untracked files/dirs

In the last few commits we focused on code in unpack-trees.c that
mistakenly removed untracked files or directories.  There may be more of
those, but in this commit we change our focus: callers of toplevel
commands that are expected to remove untracked files or directories.

As noted previously, we have toplevel commands that are expected to
delete untracked files such as 'read-tree --reset', 'reset --hard', and
'checkout --force'.  However, that does not mean that other highlevel
commands that happen to call these other commands thought about or
conveyed to users the possibility that untracked files could be removed.
Audit the code for such callsites, and add comments near existing
callsites to mention whether these are safe or not.

My auditing is somewhat incomplete, though; it skipped several cases:
  * git-rebase--preserve-merges.sh: is in the process of being
    deprecated/removed, so I won't leave a note that there are
    likely more bugs in that script.
  * contrib/git-new-workdir: why is the -f flag being used in a new
    empty directory??  It shouldn't hurt, but it seems useless.
  * git-p4.py: Don't see why -f is needed for a new dir (maybe it's
    not and is just superfluous), but I'm not at all familiar with
    the p4 stuff
  * git-archimport.perl: Don't care; arch is long since dead
  * git-cvs*.perl: Don't care; cvs is long since dead

Also, the reset --hard in builtin/worktree.c looks safe, due to only
running in an empty directory.

Signed-off-by: Elijah Newren <newren@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

diff --git a/builtin/stash.c b/builtin/stash.c
index 0e3662a230c3abfa8cb4121f978040d37b8d761b..aa31163a5a17e897893815009f27feb739b2471c 100644 (file)
--- a/builtin/stash.c
+++ b/builtin/stash.c
@@ -1521,6 +1521,7 @@ static int do_push_stash(const struct pathspec *ps, const char *stash_msg, int q
                } else {
                        struct child_process cp = CHILD_PROCESS_INIT;
                        cp.git_cmd = 1;
+                       /* BUG: this nukes untracked files in the way */
                        strvec_pushl(&cp.args, "reset", "--hard", "-q",
                                     "--no-recurse-submodules", NULL);
                        if (run_command(&cp)) {

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 4da9781b991286614bd7070b5fa76221513e8f44..549129bc1bfbd0b0d382f13e585f9a74b2f389dc 100644 (file)
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -2864,6 +2864,10 @@ static int add_submodule(const struct add_data *add_data)
                prepare_submodule_repo_env(&cp.env_array);
                cp.git_cmd = 1;
                cp.dir = add_data->sm_path;
+               /*
+                * NOTE: we only get here if add_data->force is true, so
+                * passing --force to checkout is reasonable.
+                */
                strvec_pushl(&cp.args, "checkout", "-f", "-q", NULL);
 
                if (add_data->branch) {

diff --git a/contrib/rerere-train.sh b/contrib/rerere-train.sh
index eeee45dd341b25d51b7497c4def555c4133c4267..75125d6ae003fcd213a32a0f893ae0ae48b48a1a 100755 (executable)
--- a/contrib/rerere-train.sh
+++ b/contrib/rerere-train.sh
@@ -91,7 +91,7 @@ do
                git checkout -q $commit -- .
                git rerere
        fi
-       git reset -q --hard
+       git reset -q --hard  # Might nuke untracked files...
 done
 
 if test -z "$branch"

diff --git a/submodule.c b/submodule.c
index 8e611fe1dbf1f7616040f8359ee5d9b9892ad191..a9b71d585cf636269e71919eda365345c34561c9 100644 (file)
--- a/submodule.c
+++ b/submodule.c
@@ -1866,6 +1866,7 @@ static void submodule_reset_index(const char *path)
 
        strvec_pushf(&cp.args, "--super-prefix=%s%s/",
                     get_super_prefix_or_empty(), path);
+       /* TODO: determine if this might overwright untracked files */
        strvec_pushl(&cp.args, "read-tree", "-u", "--reset", NULL);
 
        strvec_push(&cp.args, empty_tree_oid_hex());