Re: [PATCH 1/1] git-apply: Allow simultaneous --cached and --3way options

[Elijah Newren <newren@gmail.com>]

I'm not that familiar with apply.c, but let me attempt to take a look...

On Fri, Apr 2, 2021 at 6:36 PM Jerry Zhang <jerry@skydio.com> wrote:
>
> Previously, --cached and --3way were not
> allowed to be used together, since --3way
> wrote conflict markers into the working tree.
>
> These changes change semantics so that if
> these flags are given together and there is
> a conflict, the conflict markers are added
> directly to cache. If there is no conflict,
> the patch is applied directly to cache as
> expected.
>
> Signed-off-by: Jerry Zhang <jerry@skydio.com>
> Signed-off-by: Jerry Zhang <jerryxzha@googlemail.com>
> ---
>  Documentation/git-apply.txt |  4 +++-
>  apply.c                     | 13 +++++++------
>  2 files changed, 10 insertions(+), 7 deletions(-)
>
> diff --git a/Documentation/git-apply.txt b/Documentation/git-apply.txt
> index 91d9a8601c..3dc0085066 100644
> --- a/Documentation/git-apply.txt
> +++ b/Documentation/git-apply.txt
> @@ -89,7 +89,9 @@ OPTIONS
>         and we have those blobs available locally, possibly leaving the
>         conflict markers in the files in the working tree for the user to
>         resolve.  This option implies the `--index` option, and is incompatible
> -       with the `--reject` and the `--cached` options.
> +       with the `--reject` option. When used with the --cached option, any
> +       conflict markers are added directly to the cache rather than the
> +       working tree.
>
>  --build-fake-ancestor=<file>::
>         Newer 'git diff' output has embedded 'index information'
> diff --git a/apply.c b/apply.c
> index 6695a931e9..fc94ca0e99 100644
> --- a/apply.c
> +++ b/apply.c
> @@ -133,8 +133,6 @@ int check_apply_state(struct apply_state *state, int force_apply)
>
>         if (state->apply_with_reject && state->threeway)
>                 return error(_("--reject and --3way cannot be used together."));
> -       if (state->cached && state->threeway)
> -               return error(_("--cached and --3way cannot be used together."));
>         if (state->threeway) {
>                 if (is_not_gitdir)
>                         return error(_("--3way outside a repository"));
> @@ -4490,13 +4488,16 @@ static int create_file(struct apply_state *state, struct patch *patch)
>
>         if (!mode)
>                 mode = S_IFREG | 0644;
> -       if (create_one_file(state, path, mode, buf, size))
> -               return -1;
> +       if (!state->cached) {

Why add this check?  create_one_file() already has an early return if
state->cached is true.

> +               if (create_one_file(state, path, mode, buf, size))
> +                       return -1;
> +       }
>
> -       if (patch->conflicted_threeway)
> +       if (patch->conflicted_threeway && !state->cached)
>                 return add_conflicted_stages_file(state, patch);
> -       else if (state->update_index)
> +       else if (state->update_index) {
>                 return add_index_file(state, path, mode, buf, size);

So if something had conflicts, you ignore the various conflicted
modes, and just add it to the index as it stands.  What if it was
deleted upstream and modified locally?  Doesn't that just ignore the
conflict, make it invisible to the user, and add the locally modified
version?  Similarly if it was renamed upstream and modified locally,
doesn't that end up in both files being present?  And if there's a
directory/file conflict, due to the lack of ADD_CACHE_SKIP_DFCHECK (or
whatever it's called), the add is just going to fail, but perhaps
that's the most reasonable case as it'd print an error message and
return -1, I think.

Again, I didn't test any of this out and I'm not so familiar with this
code, so I'm guessing at these scenarios.  If I'm wrong about how this
works, the commit message probably deserves an explanation about why
they work, and we'd definitely need a few testcases for these types of
scenarios.  If I'm right, the current implementation is problematic at
least if not the idea of using these options together.