Re: [PATCH 1/1] Merge with untracked file that are the same without failure and test

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

Jonathan <git.jonathan.bressat@gmail.com> writes:

> diff --git a/unpack-trees.c b/unpack-trees.c
> index 360844bda3..834aca0da9 100644
> --- a/unpack-trees.c
> +++ b/unpack-trees.c
> @@ -2259,6 +2259,10 @@ static int check_ok_to_remove(const char *name, int len, int dtype,
>  			return 0;
>  	}
>  
> +	if (!ie_modified(&o->result, ce, st, 0))
> +		return 0;
> +
> +
>  	return add_rejected_path(o, error_type, name);
>  }

It probably is better to step back a bit and take a wider look at
the original to assess this change.

The only two callers of this function appears in this function.

        /*
         * We do not want to remove or overwrite a working tree file that
         * is not tracked, unless it is ignored.
         */
        static int verify_absent_1(const struct cache_entry *ce,
                                   enum unpack_trees_error_types error_type,
                                   struct unpack_trees_options *o)
        {
		...

Notice what the comment in front says?  Does this patch change the
behaviour from what the comment tells us it does?  We should adjust
the comment to the new world order if it does.

The existing code before the pre-context of the hunk reads like
this:

            /*
             * The previous round may already have decided to
             * delete this path, which is in a subdirectory that
             * is being replaced with a blob.
             */
            result = index_file_exists(&o->result, name, len, 0);
            if (result) {
                    if (result->ce_flags & CE_REMOVE)
                            return 0;
            }

We've called index_file_exists(), and the new code added here does
not take the outcome into account.

If we truly care the case "we have _UNTRACKED_ path and it happens
to be identical to what we are going to resolve to anyway",
shouldn't we be making sure that the <name,len> refers to an
untracked path by checking if result is NULL here?  If so,

	if (result) {
		...
-	}
+	} else if (!ie_modified(...)) {
+		return 0;
+	}
	return add_rejected_path(...);

is what you want, perhaps?

Or if we have cases where we have a tracked path and ask this
function if it is OK to remove, should the same reasoning you
invented to deal with untracked paths equally apply?  If that is the
case, then the code may be OK but the proposed log message to
explain and justify the change needs to be updated to explain what
happens in such a case (or how such a case will not happen).

Thanks.