Re: [PATCH 3/3] checkout -m: attempt merge when deletion of path was staged

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

On Tue, Aug 12, 2014 at 5:03 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:
> twoway_merge() is missing an o->gently check in the case where a file
> that needs to be modified is missing from the index but present in the
> old and new trees.  As a result, in this case 'git checkout -m' errors
> out instead of trying to perform a merge.
>
> Fix it by checking o->gently.  While at it, inline the o->gently check
> into reject_merge to prevent future call sites from making the same
> mistake.
>
> Noticed by code inspection.  The motivating case hasn't been tested.

That sounds sloppy X-<.  I may comment more after figuring out
what _other_ reject_merge() caller that does not appear in the
patch would change its behaviour with this patch.

  side note: of course, if this were two patches, one that adds the
  same o->gently ? -1 : reject thing to places where they forget to
  do so, and the other that moves the gently thing to reject helper,
  then we can read all the necessary information to judge the change
  in the patch ;-)

Thanks.

>
> Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
> ---
> This is the most iffy of the three patches, mostly because I was too
> lazy to write a test.  I believe it's safe as-is nonetheless.
>
> Thanks for reading.
>
>  unpack-trees.c | 11 ++++++-----
>  1 file changed, 6 insertions(+), 5 deletions(-)
>
> diff --git a/unpack-trees.c b/unpack-trees.c
> index 187b15b..6c45af7 100644
> --- a/unpack-trees.c
> +++ b/unpack-trees.c
> @@ -1178,7 +1178,8 @@ return_failed:
>  static int reject_merge(const struct cache_entry *ce,
>                         struct unpack_trees_options *o)
>  {
> -       return add_rejected_path(o, ERROR_WOULD_OVERWRITE, ce->name);
> +       return o->gently ? -1 :
> +               add_rejected_path(o, ERROR_WOULD_OVERWRITE, ce->name);
>  }
>
>  static int same(const struct cache_entry *a, const struct cache_entry *b)
> @@ -1633,7 +1634,7 @@ int threeway_merge(const struct cache_entry * const *stages,
>         /* #14, #14ALT, #2ALT */
>         if (remote && !df_conflict_head && head_match && !remote_match) {
>                 if (index && !same(index, remote) && !same(index, head))
> -                       return o->gently ? -1 : reject_merge(index, o);
> +                       return reject_merge(index, o);
>                 return merged_entry(remote, index, o);
>         }
>         /*
> @@ -1641,7 +1642,7 @@ int threeway_merge(const struct cache_entry * const *stages,
>          * make sure that it matches head.
>          */
>         if (index && !same(index, head))
> -               return o->gently ? -1 : reject_merge(index, o);
> +               return reject_merge(index, o);
>
>         if (head) {
>                 /* #5ALT, #15 */
> @@ -1770,7 +1771,7 @@ int twoway_merge(const struct cache_entry * const *src,
>                                 else
>                                         return merged_entry(newtree, current, o);
>                         }
> -                       return o->gently ? -1 : reject_merge(current, o);
> +                       return reject_merge(current, o);
>                 } else if ((!oldtree && !newtree) || /* 4 and 5 */
>                          (!oldtree && newtree &&
>                           same(current, newtree)) || /* 6 and 7 */
> @@ -1788,7 +1789,7 @@ int twoway_merge(const struct cache_entry * const *src,
>                         /* 20 or 21 */
>                         return merged_entry(newtree, current, o);
>                 } else
> -                       return o->gently ? -1 : reject_merge(current, o);
> +                       return reject_merge(current, o);
>         }
>         else if (newtree) {
>                 if (oldtree && !o->initial_checkout) {
> --