| Message ID | 20240801155702.70242c31d476c46c84ee11a3@linux-foundation.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | quiltimport mode detection oddity | expand |
Andrew Morton <akpm@linux-foundation.org> writes: > hp2:/usr/src/mm> git quiltimport --series series > tools-separate-out-shared-radix-tree-components.patch > warning: tools/testing/radix-tree/generated/autoconf.h has type 100644, expected 100664 > > > > > That patch has > > diff --git a/tools/testing/radix-tree/generated/autoconf.h a/tools/testing/radix-tree/generated/autoconf.h > deleted file mode 100664 > --- a/tools/testing/radix-tree/generated/autoconf.h > +++ /dev/null > @@ -1,2 +0,0 @@ > -#include "bit-length.h" > -#define CONFIG_XARRAY_MULTI 1 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. I do not quite recall at which point "git quiltimport" calls "git apply", but the "has type 100644, expected 100664" does ring a bell. > after quiltimport: > > hp2:/usr/src/mm> ls -l tools/testing/radix-tree/generated/autoconf.h > ls: cannot access 'tools/testing/radix-tree/generated/autoconf.h': No such file or directory That is to be expected, if that patch was successfully applied, no? After all, the patch you quoted above seems to be a removal of autoconf.h from that path. > I can't figure what I've done to make quiltimport (git-apply?) think that the file > had 100644 permissions. Maybe the lack of an index line tripped it up. You said "That patch has", and I take it to mean that the input material before "git quiltimport" touched it already had the extended header that records the removal of a file whose mode is 100664? And lack of the index line is probably a red herring. EVen if there were an index line, it would just have recorded the two object names (the blob object name of the original contents before removal, followed by a double-dot "..", followed by all-0 to signal removal). We do not read mode bits out of that line. > (btw, "has type" should be "has permissions" in that message, no?) If leading prefix 100 did not exist, yes, permissions would be more appropriate, but if the prefix changed from 100644 to say 120000, that would be a type change from plain blob to a symlink. So "type" is not quite wrong, either.
On Thu, 01 Aug 2024 17:33:48 -0700 Junio C Hamano <gitster@pobox.com> wrote: > > hp2:/usr/src/mm> git quiltimport --series series > > tools-separate-out-shared-radix-tree-components.patch > > warning: tools/testing/radix-tree/generated/autoconf.h has type 100644, expected 100664 > > > > > > > > > > That patch has > > > > diff --git a/tools/testing/radix-tree/generated/autoconf.h a/tools/testing/radix-tree/generated/autoconf.h > > deleted file mode 100664 > > --- a/tools/testing/radix-tree/generated/autoconf.h > > +++ /dev/null > > @@ -1,2 +0,0 @@ > > -#include "bit-length.h" > > -#define CONFIG_XARRAY_MULTI 1 > > 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.
On Thu, Aug 01, 2024 at 06:07:06PM -0700, Andrew Morton wrote: > > 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
On Thu, 1 Aug 2024 23:51:21 -0400 Jeff King <peff@peff.net> wrote: > On Thu, Aug 01, 2024 at 06:07:06PM -0700, Andrew Morton wrote: > > > > 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. OK. > 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). yup. > Alternatively (or maybe in addition), I wonder if quilt should similarly > canonicalize the mode. I'll hard code 100644 ;) > 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. yup, thanks.
Jeff King <peff@peff.net> writes: > 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. Makes sense. The above is consistent with what we do for the permission bits; only the execute bit matters, and the patch recording 100664 should mean the same thing to us as permission bits 100644---we should warn if the on-disk file is executable while applying such a patch, and we should not warn otherwise. > 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. It's an orthogonal fix and probably worth doing. If a third-party tool adds git-ism mode lines, we should be lenient when we see a wrong mode, as long as the leniency does not affect our normal mode of operation negatively. It is OK if they record a non-executable regular file with 100666. Using 664 (no type bits) or 100755, however, crosses the line and they must stop producing such a bogus mode line, if they do not want to see a warning.
diff --git a/tools/testing/radix-tree/generated/autoconf.h a/tools/testing/radix-tree/generated/autoconf.h deleted file mode 100664 --- a/tools/testing/radix-tree/generated/autoconf.h +++ /dev/null @@ -1,2 +0,0 @@ -#include "bit-length.h" -#define CONFIG_XARRAY_MULTI 1