Re: [RFC PATCH] git-apply: Permit change of file mode when filename does not change

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 25, 2020, 06:52 UTC
Message-ID: <xmqqeethx6ww.fsf@gitster.c.googlers.com>
In-Reply-To: <20200325061102.GD651138@coredump.intra.peff.net>

Jeff King <peff@peff.net> writes:

Show 13 quoted lines

> On Tue, Mar 24, 2020 at 09:00:54AM -0700, Daniel Sommermann wrote:
>
>> The documentation for git's diff format does not expressly disallow
>> changing the mode of a file without splitting it into a delete and
>> create. Mercurial's `hg diff --git` in fact produces git diffs with such
>> format. When applying such patches in Git, this assert can be hit. The check
>> preventing this type of diff has been around since 2005 in
>> 3cca928d4aae691572ef9a73dcc29a04f66900a1.
>
> This description confused me for a moment, because in Git we generally
> refer to "mode changes" as flipping the executable bit. And anything
> further is a "type change" (and this isn't just academic; options like
> --diff-filter distinguish the two).

I too found that file "mode" made me confused and thought we reject a patch that flips executable bits of files with or without touching their contents. You are talking about a patch that changes file "type" between regular files and symbolic links, or worse yet, regular files or symbolic links and submodules.

It sounds more like a documentation bug, or a bug in the reader of the documentation. It's not like what is not explicitly disallowed are all allowed.

Show 9 quoted lines

> And we do indeed allow a simple mode change like:
>
>   $ git show c9d4999155700651a37f4eb577cec1f4b5b8d6be --format=
>   diff --git a/t/perf/p0004-lazy-init-name-hash.sh b/t/perf/p0004-lazy-init-name-hash.sh
>   old mode 100644
>   new mode 100755
>
> But you're talking about typechanges here, and we do always represent
> those as a deletion/addition pair:

While I, like you said below, do not offhand think of a reason why it may hurt if we allowed to express a replacement of a symbolic link with a regular file that holds the result of readlink(2) as pure mode bits change without (or even with) content change at the mechanical level, I strongly suspect that we chose not to emit such a patch on the "diff" side because it would be easily missed by human readers, and that is why such a change is always shown as a deletion followed by a creation.

And this safety was there in the oldest "still not yet fully but started working barely" version, so we can safely say that we never allowed such a patch.

So, I am not sure if we want to lose it. As you found out, the removed segment is not only about regular file becoming symlink, and I do not think we would want to allow other typechanges by just simply removing the check like the proposed patch did.

Thanks.