Re: quiltimport mode detection oddity

From: Jeff King <peff@peff.net>
Date: Aug 2, 2024, 03:51 UTC
Message-ID: <20240802035121.GB1246312@coredump.intra.peff.net>
In-Reply-To: <20240801180706.933d797b0ae5744fdcdf47d2@linux-foundation.org>

On Thu, Aug 01, 2024 at 06:07:06PM -0700, Andrew Morton wrote:

Show 9 quoted lines

> > So, the patch removes autoconf.h file from that directory.  The
> > "extended header" part between "diff --git" and "--- a/..." has
> > "deleted file mode 100664" and that is where the warning comes.
> 
> yup yup.  The patch says "remove this file which has mode 100664".
> 
> The file has mode 100664.
> 
> quiltimport says it had mode 100644.  Incorrectly, I suggest.

It's definitely a weird case. Git does not record full modes, but just cares about the execute bit. So it normalizes modes for regular files to 100644 or 100755. You can see that with a simple example:

  git init
  echo foo >file
  chmod 664 file
  git add file
  git commit -m 'add file'

  git ls-files -s

  cat >patch <<\EOF
  diff --git a/file b/file
  deleted file mode 100664
  --- a/file
  +++ /dev/null
  @@ -1 +0,0 @@
  -foo
  EOF
  ls -l file
  git apply patch

Even though the filesystem has 100664, the index records 100644 (which you can see from the "ls-files" output). And then when we apply the patch, we get the "file has type 100644, expected 100664" message. AFAICT, it has been that way forever (I tried as far back as git 1.6.6). So this is nothing new, and I don't think Git would ever produce a patch that said "file mode 100664" itself (I'm assuming in your case the patch is coming from quilt).

Given that, I think it is reasonable for git to also normalize the mode of the patches it reads, so that we are consistently working in the world of simplified modes. I.e., this:

diff --git a/apply.c b/apply.c
index 142e3d913c..3d50fade78 100644
--- a/apply.c
+++ b/apply.c
@@ -995,6 +995,7 @@ static int parse_mode_line(const char *line, int linenr, unsigned int *mode)
 	*mode = strtoul(line, &end, 8);
 	if (end == line || !isspace(*end))
 		return error(_("invalid mode on line %d: %s"), linenr, line);
+	*mode = canon_mode(*mode);
 	return 0;
 }
 

which makes the warning go away in the example above. But I'm not sure
if there could be other fallout. E.g., is there a mode for git-apply to
just touch the working tree and not the index, where we'd perhaps want
to retain the original to compare against the filesystem mode? I don't
think so.

Alternatively (or maybe in addition), I wonder if quilt should similarly
canonicalize the mode. git-apply is certainly meant to work with patches
generated elsewhere, but normal patches don't have modes in them at all.
The "deleted file mode" line is git-ism, so here we have something which
is implementing the git line in a (slightly) incompatible way.

-Peff