All of lore.kernel.org
 help / color / mirror / Atom feed
From: Junio C Hamano <gitster@pobox.com>
To: Jeff King <peff@peff.net>
Cc: Andrew Morton <akpm@linux-foundation.org>,  git@vger.kernel.org
Subject: Re: quiltimport mode detection oddity
Date: Fri, 02 Aug 2024 07:57:54 -0700	[thread overview]
Message-ID: <xmqq7cczgefh.fsf@gitster.g> (raw)
In-Reply-To: <20240802035121.GB1246312@coredump.intra.peff.net> (Jeff King's message of "Thu, 1 Aug 2024 23:51:21 -0400")

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.



  parent reply	other threads:[~2024-08-02 14:58 UTC|newest]

Thread overview: 10+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2024-08-01 22:57 quiltimport mode detection oddity Andrew Morton
2024-08-02  0:33 ` Junio C Hamano
2024-08-02  1:07   ` Andrew Morton
2024-08-02  3:51     ` Jeff King
2024-08-02  5:33       ` Andrew Morton
2024-08-02 14:57       ` Junio C Hamano [this message]
2024-08-05  6:00         ` [PATCH] apply: canonicalize modes read from patches Jeff King
2024-08-15 14:52           ` Junio C Hamano
2024-08-15 15:30             ` [PATCH] t4129: fix racy index when calling chmod after git-add Jeff King
2024-08-15 16:41               ` Junio C Hamano

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=xmqq7cczgefh.fsf@gitster.g \
    --to=gitster@pobox.com \
    --cc=akpm@linux-foundation.org \
    --cc=git@vger.kernel.org \
    --cc=peff@peff.net \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.