Skip to content

Commit

Permalink
read-tree, merge-recursive: overwrite ignored files by default
Browse files Browse the repository at this point in the history
This fixes a long-standing patchwork of ignored files handling in
read-tree and merge-recursive, called out and suggested by Junio long
ago.  Quoting from commit dcf0c16 ("core.excludesfile clean-up"
2007-11-16):

    git-read-tree takes --exclude-per-directory=<gitignore>,
    not because the flexibility was needed.  Again, this was
    because the option predates the standardization of the ignore
    files.

    ...

    On the other hand, I think it makes perfect sense to fix
    git-read-tree, git-merge-recursive and git-clean to follow the
    same rule as other commands.  I do not think of a valid use case
    to give an exclude-per-directory that is nonstandard to
    read-tree command, outside a "negative" test in the t1004 test
    script.

    This patch is the first step to untangle this mess.

    The next step would be to teach read-tree, merge-recursive and
    clean (in C) to use setup_standard_excludes().

History shows each of these were partially or fully fixed:

  * clean was taught the new trick in 1617adc ("Teach git clean to
    use setup_standard_excludes()", 2007-11-14).

  * read-tree was primarily used by checkout & merge scripts.  checkout
    and merge later became builtins and were both fixed to use the new
    setup_standard_excludes() handling in fc001b5 ("checkout,merge:
    loosen overwriting untracked file check based on info/exclude",
    2011-11-27).  So the primary users were fixed, though read-tree
    itself was not.

  * merge-recursive has now been replaced as the default merge backend
    by merge-ort.  merge-ort fixed this by using
    setup_standard_excludes() starting early in its implementation; see
    commit 6681ce5 ("merge-ort: add implementation of checkout()",
    2020-12-13), largely due to its design depending on checkout() and
    thus being influenced by the checkout code.  However,
    merge-recursive itself was not fixed here, in part because its
    design meant it had difficulty differentiating between untracked
    files, ignored files, leftover tracked files that haven't been
    removed yet due to order of processing files, and files written by
    itself due to collisions).

Make the conversion more complete by now handling read-tree and
handling at least the unpack_trees() portion of merge-recursive.  While
merge-recursive is on its way out, fixing the unpack_trees() portion is
easy and facilitates some of the later changes in this series.  Note
that fixing read-tree makes the --exclude-per-directory option to
read-tree useless, so we remove it from the documentation (though we
continue to accept it if passed).

The read-tree changes happen to fix a bug in t1013.

Signed-off-by: Elijah Newren <newren@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
  • Loading branch information
newren authored and gitster committed Sep 27, 2021
1 parent c512d27 commit 491a757
Show file tree
Hide file tree
Showing 4 changed files with 21 additions and 34 deletions.
18 changes: 1 addition & 17 deletions Documentation/git-read-tree.txt
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,7 @@ SYNOPSIS
--------
[verse]
'git read-tree' [[-m [--trivial] [--aggressive] | --reset | --prefix=<prefix>]
[-u [--exclude-per-directory=<gitignore>] | -i]]
[--index-output=<file>] [--no-sparse-checkout]
[-u | -i]] [--index-output=<file>] [--no-sparse-checkout]
(--empty | <tree-ish1> [<tree-ish2> [<tree-ish3>]])


Expand Down Expand Up @@ -88,21 +87,6 @@ OPTIONS
The command will refuse to overwrite entries that already
existed in the original index file.

--exclude-per-directory=<gitignore>::
When running the command with `-u` and `-m` options, the
merge result may need to overwrite paths that are not
tracked in the current branch. The command usually
refuses to proceed with the merge to avoid losing such a
path. However this safety valve sometimes gets in the
way. For example, it often happens that the other
branch added a file that used to be a generated file in
your branch, and the safety valve triggers when you try
to switch to that branch after you ran `make` but before
running `make clean` to remove the generated file. This
option tells the command to read per-directory exclude
file (usually '.gitignore') and allows such an untracked
but explicitly ignored file to be overwritten.

--index-output=<file>::
Instead of writing the results out to `$GIT_INDEX_FILE`,
write the resulting index in the named file. While the
Expand Down
25 changes: 10 additions & 15 deletions builtin/read-tree.c
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ static int list_tree(struct object_id *oid)
}

static const char * const read_tree_usage[] = {
N_("git read-tree [(-m [--trivial] [--aggressive] | --reset | --prefix=<prefix>) [-u [--exclude-per-directory=<gitignore>] | -i]] [--no-sparse-checkout] [--index-output=<file>] (--empty | <tree-ish1> [<tree-ish2> [<tree-ish3>]])"),
N_("git read-tree [(-m [--trivial] [--aggressive] | --reset | --prefix=<prefix>) [-u | -i]] [--no-sparse-checkout] [--index-output=<file>] (--empty | <tree-ish1> [<tree-ish2> [<tree-ish3>]])"),
NULL
};

Expand All @@ -53,24 +53,16 @@ static int index_output_cb(const struct option *opt, const char *arg,
static int exclude_per_directory_cb(const struct option *opt, const char *arg,
int unset)
{
struct dir_struct *dir;
struct unpack_trees_options *opts;

BUG_ON_OPT_NEG(unset);

opts = (struct unpack_trees_options *)opt->value;

if (opts->dir)
die("more than one --exclude-per-directory given.");

dir = xcalloc(1, sizeof(*opts->dir));
dir->flags |= DIR_SHOW_IGNORED;
dir->exclude_per_dir = arg;
opts->dir = dir;
/* We do not need to nor want to do read-directory
* here; we are merely interested in reusing the
* per directory ignore stack mechanism.
*/
if (!opts->update)
die("--exclude-per-directory is meaningless unless -u");
if (strcmp(arg, ".gitignore"))
die("--exclude-per-directory argument must be .gitignore");
return 0;
}

Expand Down Expand Up @@ -209,8 +201,11 @@ int cmd_read_tree(int argc, const char **argv, const char *cmd_prefix)
if ((opts.update || opts.index_only) && !opts.merge)
die("%s is meaningless without -m, --reset, or --prefix",
opts.update ? "-u" : "-i");
if ((opts.dir && !opts.update))
die("--exclude-per-directory is meaningless unless -u");
if (opts.update && !opts.reset) {
CALLOC_ARRAY(opts.dir, 1);
opts.dir->flags |= DIR_SHOW_IGNORED;
setup_standard_excludes(opts.dir);
}
if (opts.merge && !opts.index_only)
setup_work_tree();

Expand Down
11 changes: 10 additions & 1 deletion merge-recursive.c
Original file line number Diff line number Diff line change
Expand Up @@ -408,8 +408,13 @@ static int unpack_trees_start(struct merge_options *opt,
memset(&opt->priv->unpack_opts, 0, sizeof(opt->priv->unpack_opts));
if (opt->priv->call_depth)
opt->priv->unpack_opts.index_only = 1;
else
else {
opt->priv->unpack_opts.update = 1;
/* FIXME: should only do this if !overwrite_ignore */
CALLOC_ARRAY(opt->priv->unpack_opts.dir, 1);
opt->priv->unpack_opts.dir->flags |= DIR_SHOW_IGNORED;
setup_standard_excludes(opt->priv->unpack_opts.dir);
}
opt->priv->unpack_opts.merge = 1;
opt->priv->unpack_opts.head_idx = 2;
opt->priv->unpack_opts.fn = threeway_merge;
Expand All @@ -423,6 +428,10 @@ static int unpack_trees_start(struct merge_options *opt,
init_tree_desc_from_tree(t+2, merge);

rc = unpack_trees(3, t, &opt->priv->unpack_opts);
if (opt->priv->unpack_opts.dir) {
dir_clear(opt->priv->unpack_opts.dir);
FREE_AND_NULL(opt->priv->unpack_opts.dir);
}
cache_tree_free(&opt->repo->index->cache_tree);

/*
Expand Down
1 change: 0 additions & 1 deletion t/t1013-read-tree-submodule.sh
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@ test_description='read-tree can handle submodules'
. "$TEST_DIRECTORY"/lib-submodule-update.sh

KNOWN_FAILURE_DIRECTORY_SUBMODULE_CONFLICTS=1
KNOWN_FAILURE_SUBMODULE_OVERWRITE_IGNORED_UNTRACKED=1

test_submodule_switch_recursing_with_args "read-tree -u -m"

Expand Down

4 comments on commit 491a757

@oconnor663
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@newren this change has broken a test in a project of mine that was relying on git read-tree to not respect .gitignore files. (Obligatory XKCD.) That peru tool is using git plumbing commands to manage trees of files, but it tries to keep this implementation detail internal, and behaving differently in the presence of a .gitignore file belonging to the user would leak this internal implementation detail. I've been trying to figure out a way to reproduce the Git 1.33 behavior in Git 1.34, but so far I haven't found any flags or configs to do that. (For example, putting !* in .git/info/exclude doesn't seem to help, I think because a .gitignore files in the working tree take precedence.) Can you suggest any other workaround? Thanks a million for your help, and please feel free to ignore this if you're busy.

@dscho
Copy link
Member

@dscho dscho commented on 491a757 Dec 12, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@oconnor663 you seem to have a point, but you did not find the correct place to raise the issue.

You will want to reply to this mail: https://lore.kernel.org/git/2501a0c552ad5147f61a96b9ebe45c5199e1dbfd.1632760428.git.gitgitgadget@gmail.com/ (you can use GitGitGadget's guidance how to do that).

@oconnor663
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the heads up @dscho. I've emailed a reply here: https://lore.kernel.org/git/CA+6di1m91q70PfaFq0DKMsmd_Tb6XBB7H9AYPhwawX12cZgtGQ@mail.gmail.com/T/#u. Please let me know if I got anything wrong with the recipients/subject/etc.

@dscho
Copy link
Member

@dscho dscho commented on 491a757 Dec 13, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good!

Please sign in to comment.