Re: [PATCH v2] add: support pre-add hook

[Junio C Hamano <gitster@pobox.com>]

"Chandra Kethi-Reddy via GitGitGadget" <gitgitgadget@gmail.com>
writes:

> diff --git a/Documentation/git-add.adoc b/Documentation/git-add.adoc
> index 6192daeb03..c864ce272d 100644
> --- a/Documentation/git-add.adoc
> +++ b/Documentation/git-add.adoc
> @@ -10,7 +10,7 @@ SYNOPSIS
>  [synopsis]
>  git add [--verbose | -v] [--dry-run | -n] [--force | -f] [--interactive | -i] [--patch | -p]
>  	[--edit | -e] [--[no-]all | -A | --[no-]ignore-removal | [--update | -u]] [--sparse]
> -	[--intent-to-add | -N] [--refresh] [--ignore-errors] [--ignore-missing] [--renormalize]
> +	[--intent-to-add | -N] [--refresh] [--ignore-errors] [--ignore-missing] [--renormalize] [--no-verify]
>  	[--chmod=(+|-)x] [--pathspec-from-file=<file> [--pathspec-file-nul]]
>  	[--] [<pathspec>...]
>  
> @@ -42,6 +42,10 @@ use the `--force` option to add ignored files. If you specify the exact
>  filename of an ignored file, `git add` will fail with a list of ignored
>  files. Otherwise it will silently ignore the file.
>  
> +A pre-add hook can be run to inspect or reject the proposed index update
> +after `git add` computes staging and writes it to the index lockfile,
> +but before writing it to the final index. See linkgit:githooks[5].
>
> +`--no-verify`::
> +	Bypass the pre-add hook if it exists. See linkgit:githooks[5] for
> +	more information about hooks.

I'll leave it up to others to comment on and make concrete
suggestions for the formatting and markups, but the word pre-add the
users must use verbatim that is not marked up in any way would not
look good in the documentation.

Is it and will it always be only the pre-add hook that this option
will bypass, or if we ever add another hook that decides to interfere,
will that hook also be turned off with this option?  This reads like
the former, but the intent would be the latter, no?

I'll also leve it up to others (including the original author of the
patch) to propose a better wording here, as I am not good at naming
things ;-)


> +pre-add
> +~~~~~~~
> +
> +This hook is invoked by linkgit:git-add[1], and can be bypassed with the
> +`--no-verify` option. It is not invoked for `--interactive`, `--patch`,
> +`--edit`, or `--dry-run`.
> +
> +It takes two parameters: the path to a copy of the index before this
> +invocation of `git add`, and the path to the lockfile containing the
> +proposed index after staging. It does not read from standard input.
> +If no index exists yet, the first parameter names a path that does not
> +exist and should be treated as an empty index. No special environment
> +variables are set. The hook is invoked after the index has been updated

What are "special environment variables"?  What happens, for
example, if the end user has an "special environment variable" set
and exported when running "git add"---are you unexporting them?
E.g., Does GIT_INDEX_FILE environment variable visible to the hook
when you do this ...

    $ GIT_INDEX_FILE=.git/alt-index git add .

... and if so, what value does it have?

In other words, is it worth spelling this "special environment
variables" thing out?

> +	if (!show_only && !no_verify && find_hook(repo, "pre-add")) {
> +		int fd_in, status;
> +		const char *index_file = repo_get_index_file(repo);
> +		char *template;
> +
> +		run_pre_add = 1;
> +		template = xstrfmt("%s.pre-add.XXXXXX", index_file);
> +		orig_index = xmks_tempfile(template);
> +		free(template);
> +
> +		fd_in = open(index_file, O_RDONLY);
> +		if (fd_in >= 0) {
> +			status = copy_fd(fd_in, get_tempfile_fd(orig_index));
> +			if (close(fd_in))
> +				die_errno(_("unable to close index for pre-add hook"));
> +			if (close_tempfile_gently(orig_index))
> +				die_errno(_("unable to close temporary index copy"));
> +			if (status < 0)
> +				die(_("failed to copy index for pre-add hook"));
> +		} else if (errno == ENOENT) {
> +			orig_index_path = xstrdup(get_tempfile_path(orig_index));
> +			if (delete_tempfile(&orig_index))
> +				die_errno(_("unable to remove temporary index copy"));
> +		} else {
> +			die_errno(_("unable to open index for pre-add hook"));
> +		}
> +	}

Do we really need to create a copy of the file?  I am just asking
without knowing the answer myself, but given that the general
architecture of file writing used in our codebase, which is to (1)
prepare a new temporary file, (2) write new contents to that
temporary file, and then finally (3) rename the temporary file to
the final location, I would expect that between the time the control
passes this point and the latter half of write_locked_index() calls
commit_locked_index(), the original index file would not be touched
by anybody, and can be readable by the hook.

> +	if (run_pre_add && !exit_status && repo->index->cache_changed) {
> +		struct run_hooks_opt opt = RUN_HOOKS_OPT_INIT;
> +
> +		if (write_locked_index(repo->index, &lock_file, 0))
> +			die(_("unable to write new index file"));

This mimics the pattern used in builtin/commit.c:prepare_index()
that populates the index file (the real one, when making a
non-partial commit, or the temporary one when making a partial
commit), closes it, and let us later commit or roll back depending
on what happens in between.  Looks sensible (but I have to admit
that I may have missed resource leakage etc., as I didn't seriously
look for such flaws).

Shouldn't the die() message mirror the wording used there, i.e.,
"unable to create temporary index" or something, or is this fine, as
it will become the new index file once the hook approves?  I dunno.

Thanks.

> +		strvec_push(&opt.args, orig_index ? get_tempfile_path(orig_index) :
> +					     orig_index_path);
> +		strvec_push(&opt.args, get_lock_file_path(&lock_file));
> +		if (run_hooks_opt(repo, "pre-add", &opt)) {
> +			rollback_lock_file(&lock_file); /* hook rejected */
> +			exit_status = 1;
> +		} else {
> +			if (commit_lock_file(&lock_file)) /* hook approved */
> +				die(_("unable to write new index file"));
> +		}
> +	} else {
> +		if (write_locked_index(repo->index, &lock_file,
> +				       COMMIT_LOCK | SKIP_IF_UNCHANGED))
> +			die(_("unable to write new index file"));
> +	}