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

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

Jeff King <peff@peff.net> writes:

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

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