Ben Peart <peart...@gmail.com> writes:
> diff --git a/config.c b/config.c
> index 4051e38823..96e05ee0f1 100644
> --- a/config.c
> +++ b/config.c
> ...
> @@ -2307,6 +2311,37 @@ int git_config_get_index_threads(void)
> return 0; /* auto */
> }
>
> +int git_config_get_virtualfilesystem(void)
> +{
> + if (git_config_get_pathname("core.virtualfilesystem",
> &core_virtualfilesystem))
> + core_virtualfilesystem = getenv("GIT_TEST_VIRTUALFILESYSTEM");
> +
> + if (core_virtualfilesystem && !*core_virtualfilesystem)
> + core_virtualfilesystem = NULL;
> +
> + if (core_virtualfilesystem) {
> + /*
> + * Some git commands spawn helpers and redirect the index to a
> different
> + * location. These include "difftool -d" and the sequencer
> + * (i.e. `git rebase -i`, `git cherry-pick` and `git revert`)
> and others.
> + * In those instances we don't want to update their temporary
> index with
> + * our virtualization data.
> + */
> + char *default_index_file = xstrfmt("%s/%s",
> the_repository->gitdir, "index");
> + int should_run_hook = !strcmp(default_index_file,
> the_repository->index_file);
> +
> + free(default_index_file);
> + if (should_run_hook) {
> + /* virtual file system relies on the sparse checkout
> logic so force it on */
> + core_apply_sparse_checkout = 1;
> + return 1;
> + }
> + core_virtualfilesystem = NULL;
It would be a small leak if this came from config_get_pathname(),
but if it came from $GIT_TEST_VFS env, we cannot free it X-<.
A helper function called *_get_X() that does not return X as its
return value or updating the location pointed by its *dst parameter,
and instead only stores its finding in a global variable feels
somewhat odd. It smells more like "find out", "probe", "check",
etc.
> diff --git a/dir.c b/dir.c
> index 47c2fca8dc..3097b0e446 100644
> --- a/dir.c
> +++ b/dir.c
> @@ -21,6 +21,7 @@
> #include "ewah/ewok.h"
> #include "fsmonitor.h"
> #include "submodule-config.h"
> +#include "virtualfilesystem.h"
>
> /*
> * Tells read_directory_recursive how a file or directory should be treated.
> @@ -1109,6 +1110,18 @@ int is_excluded_from_list(const char *pathname,
> struct exclude_list *el, struct index_state *istate)
> {
> struct exclude *exclude;
> +
> + /*
> + * The virtual file system data is used to prevent git from traversing
> + * any part of the tree that is not in the virtual file system. Return
> + * 1 to exclude the entry if it is not found in the virtual file system,
> + * else fall through to the regular excludes logic as it may further
> exclude.
> + */
This comment will sit better immediately in front of the call to "is
excluded from vfs?" helper function.
> + if (*dtype == DT_UNKNOWN)
> + *dtype = get_dtype(NULL, istate, pathname, pathlen);
We try to defer paying cost to determine unknown *dtype as late as
possible by having this call in last_exclude_matching_from_list(),
and not here. If we are doing this, we probably should update the
callpaths that call last_exclude_matching_from_list() to make the
caller responsible for doing get_dtype() and drop the lazy finding
of dtype from the callee. Alternatively, the new "is excluded from
vfs" helper can learn to do the lazy get_dtype() just like the
existing last_exclude_matching_from_list() does. I suspect the
latter may be simpler.
> + if (is_excluded_from_virtualfilesystem(pathname, pathlen, *dtype) > 0)
> + return 1;
> +
> exclude = last_exclude_matching_from_list(pathname, pathlen, basename,
> dtype, el, istate);
> if (exclude)
> @@ -1324,8 +1337,20 @@ struct exclude *last_exclude_matching(struct
> dir_struct *dir,
> int is_excluded(struct dir_struct *dir, struct index_state *istate,
> const char *pathname, int *dtype_p)
> {
> - struct exclude *exclude =
> - last_exclude_matching(dir, istate, pathname, dtype_p);
> + struct exclude *exclude;
> +
> + /*
> + * The virtual file system data is used to prevent git from traversing
> + * any part of the tree that is not in the virtual file system. Return
> + * 1 to exclude the entry if it is not found in the virtual file system,
> + * else fall through to the regular excludes logic as it may further
> exclude.
> + */
> + if (*dtype_p == DT_UNKNOWN)
> + *dtype_p = get_dtype(NULL, istate, pathname, strlen(pathname));
Exactly the same comment as above.
> + if (is_excluded_from_virtualfilesystem(pathname, strlen(pathname),
> *dtype_p) > 0)
> + return 1;
> +
> + exclude = last_exclude_matching(dir, istate, pathname, dtype_p);
> if (exclude)
> return exclude->flags & EXC_FLAG_NEGATIVE ? 0 : 1;
> return 0;
> @@ -1678,6 +1703,9 @@ static enum path_treatment treat_one_path(struct
> dir_struct *dir,
> if (dtype != DT_DIR && has_path_in_index)
> return path_none;
>
> + if (is_excluded_from_virtualfilesystem(path->buf, path->len, dtype) > 0)
> + return path_excluded;
> +
> /*
> * When we are looking at a directory P in the working tree,
> * there are three cases:
> @@ -2018,6 +2046,8 @@ static enum path_treatment
> read_directory_recursive(struct dir_struct *dir,
> /* add the path to the appropriate result list */
> switch (state) {
> case path_excluded:
> + if (is_excluded_from_virtualfilesystem(path.buf,
> path.len, DT_DIR) > 0)
> + break;
> if (dir->flags & DIR_SHOW_IGNORED)
> dir_add_name(dir, istate, path.buf, path.len);
> else if ((dir->flags & DIR_SHOW_IGNORED_TOO) ||
I am kind-of surprised that the "damage" to dir.c need to support
this is so isolated and small ;-)
> ...
> +if test "$1" != 1
> +then
> + echo "Unsupported core.virtualfilesystem hook version." >&2
> + exit 1
> +fi
> +
> +#find -type f -printf '%P\0'
> +find -type d -printf '%P/\0'
I am not reading (hence not commenting on) tests in this review
message yet.
> diff --git a/unpack-trees.c b/unpack-trees.c
> index 7570df481b..ee3cda2e94 100644
> --- a/unpack-trees.c
> +++ b/unpack-trees.c
> @@ -18,6 +18,7 @@
> #include "fsmonitor.h"
> #include "object-store.h"
> #include "fetch-object.h"
> +#include "virtualfilesystem.h"
>
> /*
> * Error messages expected by scripts out of plumbing commands such as
> @@ -1363,6 +1364,14 @@ static int clear_ce_flags_1(struct index_state *istate,
> continue;
> }
>
> + /* if it's not in the virtual file system, exit early */
> + if (core_virtualfilesystem) {
> + if (is_included_in_virtualfilesystem(ce->name,
> ce->ce_namelen) > 0)
> + ce->ce_flags &= ~clear_mask;
> + cache++;
> + continue;
> + }
Earlier we saw "is it excluded?" and now we have "is it included?"
They have different function signature (i.e. "included?" does not
need to know the type of the entry), and I am guessing that for the
purpose of this particular patch that may be sufficient, but I have
to wonder if in the longer term we'd be better off to keep the
interface to these two functions similar. Also, I wonder if we need
both---I see below that these "is included/excluded?" helpers are
allowed to say "I dunno", so that may be the reason why we cannot
simply say "included is the same as !excluded".
