Re: [PATCH 2/2] apply: add --intent-to-add

From: Junio C Hamano <gitster@pobox.com>
Date: May 14, 2018, 02:33 UTC
Message-ID: <xmqqo9hjc6er.fsf@gitster-ct.c.googlers.com>
In-Reply-To: <20180513175438.32152-2-pclouds@gmail.com>

Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:

Show 35 quoted lines

> Similar to 'git reset -N', this option makes 'git apply' automatically
> mark new files as intent-to-add so they are visible in the following
> 'git diff' command and could also be committed with 'git commit -a'.
>
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
>  Documentation/git-apply.txt |  9 ++++++++-
>  apply.c                     | 38 +++++++++++++++++++++++++++++++------
>  apply.h                     |  1 +
>  t/t2203-add-intent.sh       | 12 ++++++++++++
>  4 files changed, 53 insertions(+), 7 deletions(-)
>
> diff --git a/Documentation/git-apply.txt b/Documentation/git-apply.txt
> index 4ebc3d3271..2374f64b51 100644
> --- a/Documentation/git-apply.txt
> +++ b/Documentation/git-apply.txt
> @@ -9,7 +9,7 @@ git-apply - Apply a patch to files and/or to the index
>  SYNOPSIS
>  --------
>  [verse]
> -'git apply' [--stat] [--numstat] [--summary] [--check] [--index] [--3way]
> +'git apply' [--stat] [--numstat] [--summary] [--check] [--index | --intent-to-add] [--3way]
>  	  [--apply] [--no-add] [--build-fake-ancestor=<file>] [-R | --reverse]
>  	  [--allow-binary-replacement | --binary] [--reject] [-z]
>  	  [-p<n>] [-C<n>] [--inaccurate-eof] [--recount] [--cached]
> @@ -74,6 +74,13 @@ OPTIONS
>  	cached data, apply the patch, and store the result in the index
>  	without using the working tree. This implies `--index`.
>  
> +--intent-to-add::
> +	When applying the patch only to the working tree, mark new
> +	files to be added to the index later (see `--intent-to-add`
> +	option in linkgit:git-add[1]). This option is ignored if
> +	`--index` is present or the command is not run in a Git
> +	repository.

It may make sense to make it incompatible with "--index" like you did, but how does this interact with "--cached" or "--3way"? It is unclear from the above documentation.

Show 10 quoted lines

> diff --git a/apply.c b/apply.c
> index 7e5792c996..31d3e50401 100644
> --- a/apply.c
> +++ b/apply.c
> @@ -136,6 +136,8 @@ int check_apply_state(struct apply_state *state, int force_apply)
>  		state->apply = 0;
>  	if (state->check_index && is_not_gitdir)
>  		return error(_("--index outside a repository"));
> +	if (state->set_ita && is_not_gitdir)
> +		state->set_ita = 0;

I think this should error out, just like one line above does. "I-t-a" is impossible without having the index, just like "--index" is impossible without having the index.

Show 7 quoted lines

> @@ -4265,9 +4267,6 @@ static int add_index_file(struct apply_state *state,
>  	int namelen = strlen(path);
>  	unsigned ce_size = cache_entry_size(namelen);
>  
> -	if (!state->update_index)
> -		return 0;
> -

OK, with this change, only "index-affecting" mode will call into the function, in the first place. The current code was wasteful in that the caller always called and forced this function to do a few useless computation before it realized that it is not going to touch the index at all.

Show 24 quoted lines

> @@ -4305,6 +4304,27 @@ static int add_index_file(struct apply_state *state,
>  	return 0;
>  }
>  
> +static int add_ita_file(struct apply_state *state,
> +			const char *path, unsigned mode)
> +{
> +	struct cache_entry *ce;
> +	int namelen = strlen(path);
> +	unsigned ce_size = cache_entry_size(namelen);
> +
> +	ce = xcalloc(1, ce_size);
> +	memcpy(ce->name, path, namelen);
> +	ce->ce_mode = create_ce_mode(mode);
> +	ce->ce_flags = create_ce_flags(0) | CE_INTENT_TO_ADD;
> +	ce->ce_namelen = namelen;
> +	set_object_name_for_intent_to_add_entry(ce);
> +	if (add_cache_entry(ce, ADD_CACHE_OK_TO_ADD) < 0) {
> +		free(ce);
> +		return error(_("unable to add cache entry for %s"), path);
> +	}
> +
> +	return 0;
> +}

It is somewhat unsatisfactory that we need a whole new function to record a new path in the index. IOW, I have a feeling that a bit of refactoring of add_index_file() should allow us to share more code between it and this new function.

Show 10 quoted lines

> @@ -4465,8 +4485,11 @@ static int create_file(struct apply_state *state, struct patch *patch)
>  
>  	if (patch->conflicted_threeway)
>  		return add_conflicted_stages_file(state, patch);
> -	else
> +	else if (state->update_index)
>  		return add_index_file(state, path, mode, buf, size);
> +	else if (state->set_ita)
> +		return add_ita_file(state, path, mode);
> +	return 0;

It is very unfortunate that you need to do (update_index||set_ita) everywhere else only bevause you want to do this else/if cascade. I'd rather redefine the bits to mean

    - update-index: we will do something that touches the index
      (hence we need to have the repository, we need to lock the
      index, etc.).

    - ita-only: changes to the existing paths are only reflected to
      the working tree, but new paths are added to the index as
      i-t-a entries.

and make add_index_file() more intelligent, without having to add a new add_ita_file().

Of course, setting only ita-only without setting update-index is an inconsistent state. "--index" would set only update-index, "--ita" would set both, "--ita --index" would either be an error, or set only update-index and clear ita-only if we adopt "last one wins" semantics.