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

> 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 <pclo...@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.

> 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.

> @@ -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.

> @@ -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.

> @@ -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.

Reply via email to