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

From: Junio C Hamano <gitster@pobox.com>
Date: Apr 13, 2022, 18:18 UTC
Message-ID: <xmqqfsmg97ac.fsf@gitster.g>
In-Reply-To: <20220412191556.21135-2-Jonathan.bressat@etu.univ-lyon1.fr>

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

Show 14 quoted lines

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