bug#61326: 30.0.50; Editing fil in zip file without extension save creates new file

bug#61326: 30.0.50; Editing fil in zip file without extension save creates new file

[Adam Sjøgren via Bug reports for GNU Emacs, the Swiss army knife of text editors]

If I open a zip archive in Emacs, I get a listing of files within the
archive, and I can edit the files by pressing RET, and if I then save
the file, Emacs updates the archive.

However, if the zip archive doesn't have ".zip" (or ".jar") at the end
of the filename, Emacs instead of updating the archive opened, creates a
new archive with ".zip" added.

Example:

    $ echo ABBA > a.txt
    $ zip a.zip a.txt
      adding: a.txt (stored 0%)
    $ mv a.zip extless
    $ ls
    a.txt  extless
    $ emacs -Q extless 
    $ # RET on a.txt, add to content, save
    $ ls
    a.txt  extless  extless.zip
    $ 

I was expecting the zip archive I opened, "extless" to be updated,
instead of a new file "extless.zip" appearing.

The expected behaviour is useful when editing e.g. "zipimport" binaries,
such as yt-dlp: https://github.com/yt-dlp/yt-dlp#release-files


In GNU Emacs 30.0.50 (build 1, x86_64-pc-linux-gnu, GTK+ Version
 3.24.36, cairo version 1.16.0) of 2023-01-15 built on tullinup
Repository revision: b017c0453c73798c30b2f046bfa62a57cf6ea72b
Repository branch: master
Windowing system distributor 'The X.Org Foundation', version 11.0.12101006
System Description: Debian GNU/Linux bookworm/sid

Configured using:
 'configure -C --with-tree-sitter --with-librsvg --with-xinput2
 --without-pgtk --with-native-compilation=aot'

Configured features:
ACL CAIRO DBUS FREETYPE GIF GLIB GMP GNUTLS GPM GSETTINGS HARFBUZZ JPEG
JSON LCMS2 LIBOTF LIBSELINUX LIBSYSTEMD LIBXML2 M17N_FLT MODULES
NATIVE_COMP NOTIFY INOTIFY PDUMPER PNG RSVG SECCOMP SOUND SQLITE3
THREADS TIFF TOOLKIT_SCROLL_BARS TREE_SITTER WEBP X11 XDBE XIM XINPUT2
XPM GTK3 ZLIB

Important settings:
  value of $LANG: en_GB.UTF-8
  locale-coding-system: utf-8-unix

Major mode: Group

Minor modes in effect:
  gnus-topic-mode: t
  gnus-undo-mode: t
  age-encryption-mode: t
  pixel-scroll-precision-mode: t
  global-git-commit-mode: t
  magit-auto-revert-mode: t
  dumb-jump-mode: t
  shell-dirtrack-mode: t
  which-function-mode: t
  global-auto-complete-mode: t
  save-place-mode: t
  jabber-activity-mode: t
  winner-mode: t
  server-mode: t
  tooltip-mode: t
  global-eldoc-mode: t
  electric-indent-mode: t
  mouse-wheel-mode: t
  menu-bar-mode: t
  file-name-shadow-mode: t
  global-font-lock-mode: t
  font-lock-mode: t
  buffer-read-only: t
  line-number-mode: t
  auto-composition-mode: t
  auto-encryption-mode: t
  auto-compression-mode: t

Load-path shadows:
~/elisp/let-alist/let-alist hides ~/elisp/extra/let-alist
/home/asjo/elisp/boxquote.el/boxquote hides ~/elisp/extra/boxquote
~/elisp/let-alist/let-alist hides /usr/src/emacs/lisp/emacs-lisp/let-alist

Features:
(shadow bbdb-message emacsbug shr-color executable js-mode-expansions js
c-ts-mode treesit cc-mode-expansions cc-mode cc-fonts cc-guess cc-menus
cc-cmds cc-styles cc-align cc-engine cc-vars cc-defs flow-fill rng-xsd
xsd-regexp rng-cmpct nxml-mode-expansions rng-nxml rng-valid rng-loc
rng-uri rng-parse nxml-parse rng-match rng-dt rng-util rng-pttrn nxml-ns
nxml-mode nxml-outln nxml-rap nxml-util nxml-enc xmltok jka-compr
mm-archive bbdb-gnus-aux qp mule-util url-http url-gw url-auth
gnus-gravatar gravatar sort smiley gnus-cite textsec uni-scripts
idna-mapping ucs-normalize uni-confusable textsec-check gnus-bcklg
gnus-async gnus-dup gnus-ml disp-table gnus-topic utf-7 imap rfc2104
face-remap epa-file network-stream nsm nnml bbdb-gnus bbdb-mua nnnil
gnus-demon gnus-delay gnus-draft gnus-agent gnus-srvr gnus-score
score-mode nnvirtual nntp gnus-cache nndraft nnmh mail-extr spam
spam-stat bbdb-com gnus-uu yenc gnus-msg gnus-html url-queue help-fns
radix-tree url-cache mm-url bbdb-picture gnus-art mm-uu mml2015 mm-view
mml-smime smime dig gnus-sum shr pixel-fill kinsoku url-file svg
gnus-group gnus-undo gnus-fun hashcash gnus-start gnus-dbus gnus-cloud
nnimap nnmail mail-source utf7 nnoo gnus-spec gnus-int gnus-range
gnus-win gnus nnheader range flymake-proc flymake time age pixel-scroll
cua-base litable magithub magithub-ci magithub-issue magithub-cache
magithub-core magit-submodule magit-obsolete magit-popup magit-blame
magit-stash magit-reflog magit-bisect magit-push magit-pull magit-fetch
magit-clone magit-remote magit-commit magit-sequence magit-notes
magit-worktree magit-tag magit-merge magit-branch magit-reset
magit-files magit-refs magit-status magit magit-repos magit-apply
magit-wip magit-log magit-diff smerge-mode diff git-commit log-edit
message sendmail yank-media rfc822 mml mml-sec epa epg rfc6068
epg-config gnus-util mm-decode mm-bodies mm-encode mail-parse rfc2231
rfc2047 rfc2045 mm-util ietf-drums mail-prsvr mailabbrev mail-utils
gmm-utils mailheader pcvs-util magit-core magit-autorevert autorevert
filenotify magit-margin magit-transient magit-process with-editor
magit-mode magit-git magit-section benchmark magit-utils vc-git
diff-mode ido crm markdown-mode color noutline outline rg pcase
rg-info-hack rg-menu transient rg-ibuffer ibuf-macs rg-result wgrep-rg
rg-history rg-header ibuf-ext ibuffer ibuffer-loaddefs wgrep grep
compile text-property-search dumb-jump f dash s etags fileloop generator
xref project tramp tramp-loaddefs trampver tramp-integration cus-edit pp
cus-load files-x tramp-compat shell pcomplete parse-time iso8601 ls-lisp
auto-loads tex-site expand-region subword-mode-expansions
cperl-mode-expansions text-mode-expansions html-mode-expansions
er-basic-expansions thingatpt expand-region-core expand-region-custom
which-func cperl-mode auto-complete-config auto-complete edmacro kmacro
popup debian-changelog-mode imenu add-log dpkg-dev-el saveplace vc
vc-dispatcher bbdb bbdb-site timezone bbdb-loaddefs julia-mode
julia-mode-latexsubs boxquote rect jabber-last-message-correction
jabber-http-file-upload jabber-print-html jabber-otr jabber
jabber-notifications notifications jabber-libnotify dbus jabber-awesome
jabber-osd jabber-wmii jabber-xmessage jabber-festival jabber-sawfish
jabber-ratpoison jabber-tmux jabber-screen jabber-socks5
jabber-ft-server jabber-si-server jabber-ft-client jabber-ft-common
jabber-si-client jabber-si-common jabber-feature-neg jabber-truncate
jabber-time jabber-autoaway time-date jabber-vcard-avatars
jabber-chatstates jabber-events jabber-vcard jabber-avatar
jabber-activity jabber-watch jabber-modeline easy-mmode advice
jabber-ahc-presence jabber-ahc jabber-version jabber-ourversion
jabber-muc-nick-completion hippie-exp comint ansi-osc ansi-color
jabber-browse jabber-search jabber-register jabber-roster format-spec
jabber-presence jabber-muc jabber-bookmarks jabber-private
jabber-muc-nick-coloring hexrgb jabber-widget jabber-disco wid-edit
jabber-chat jabber-history jabber-chatbuffer jabber-alert jabber-iq
jabber-core jabber-console derived sgml-mode facemenu dom ewoc
jabber-keymap jabber-sasl sasl sasl-anonymous sasl-login sasl-plain fsm
jabber-logon jabber-conn comp comp-cstr warnings icons rx cl-extra
help-mode srv dns starttls tls jabber-xml xml jabber-menu jabber-util cl
winner ring gnutls puny find-file-from-selection find-lisp dired
dired-loaddefs cap-words superword subword server finder-inf package
browse-url url url-proxy url-privacy url-expand url-methods url-history
url-cookie generate-lisp-file url-domsuf url-util mailcap url-handlers
url-parse auth-source cl-seq eieio eieio-core cl-macs password-cache
json subr-x map byte-opt gv bytecomp byte-compile url-vars cl-loaddefs
cl-lib rmc iso-transl tooltip cconv eldoc paren electric uniquify
ediff-hook vc-hooks lisp-float-type elisp-mode mwheel term/x-win x-win
term/common-win x-dnd tool-bar dnd fontset image regexp-opt fringe
tabulated-list replace newcomment text-mode lisp-mode prog-mode register
page tab-bar menu-bar rfn-eshadow isearch easymenu timer select
scroll-bar mouse jit-lock font-lock syntax font-core term/tty-colors
frame minibuffer nadvice seq simple cl-generic indonesian philippine
cham georgian utf-8-lang misc-lang vietnamese tibetan thai tai-viet lao
korean japanese eucjp-ms cp51932 hebrew greek romanian slovak czech
european ethiopic indian cyrillic chinese composite emoji-zwj charscript
charprop case-table epa-hook jka-cmpr-hook help abbrev obarray oclosure
cl-preloaded button loaddefs theme-loaddefs faces cus-face macroexp
files window text-properties overlay sha1 md5 base64 format env
code-pages mule custom widget keymap hashtable-print-readable backquote
threads dbusbind inotify lcms2 dynamic-setting system-font-setting
font-render-setting cairo move-toolbar gtk x-toolkit xinput2 x multi-tty
make-network-process native-compile emacs)

Memory information:
((conses 16 1135254 142749)
 (symbols 48 43507 17)
 (strings 32 298866 21178)
 (string-bytes 1 12616608)
 (vectors 16 98259)
 (vector-slots 8 3113739 175347)
 (floats 8 750 1097)
 (intervals 56 795 371)
 (buffers 984 33))

-- 
 "Den ene mands loft                                        Adam Sjøgren
  Er den anden mands gulv"                             asjo@koldfront.dk

bug#61326: 30.0.50; Editing fil in zip file without extension save creates new file

[Eli Zaretskii]

> Date: Mon, 06 Feb 2023 18:00:14 +0100
> From:  Adam Sjøgren via "Bug reports for GNU Emacs,
>  the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>
> 
> 
> If I open a zip archive in Emacs, I get a listing of files within the
> archive, and I can edit the files by pressing RET, and if I then save
> the file, Emacs updates the archive.
> 
> However, if the zip archive doesn't have ".zip" (or ".jar") at the end
> of the filename, Emacs instead of updating the archive opened, creates a
> new archive with ".zip" added.

AFAIR, Emacs uses the 'zip' program to create or update a zip archive,
and I suspect that the fact the archive has .zip appended is because
that's what the 'zip' program does.

Did you try looking at the command that Emacs invokes when you update
a member of the archive?

bug#61326: 30.0.50; Editing fil in zip file without extension save creates new file

[Adam Sjøgren via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Eli writes:

> I suspect that the fact the archive has .zip appended is because
> that's what the 'zip' program does.

Yes - I skimmed the man-page for zip(1), and there doesn't seem to be an
option to turn off "add .zip if there is no '.' in the filename" -
unfortunately, that would have been really handy here.

> Did you try looking at the command that Emacs invokes when you update
> a member of the archive?

I did not.

bug#61326: Adding --no-add-suffix to zip patch

[Adam Sjøgren via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Adding a '--no-add-suffix' option to zip 3.0 is not too bad:

diff -u orig/zip-3.0/globals.c chan/zip-3.0/globals.c
--- orig/zip-3.0/globals.c	2008-05-25 19:26:38.000000000 +0200
+++ chan/zip-3.0/globals.c	2023-02-06 19:42:48.000000000 +0100
@@ -106,6 +106,7 @@
 int noisy = 1;                /* 0=quiet operation */
 int extra_fields = 1;         /* 0=create minimum, 1=don't copy old, 2=keep old */
 int use_descriptors = 0;      /* 1=use data descriptors 12/29/04 */
+int no_add_suffix = 0;        /* 1=do not add suffix .zip to archive names without . */
 int zip_to_stdout = 0;        /* output zipfile to stdout 12/30/04 */
 int allow_empty_archive = 0;  /* if no files, create empty archive anyway 12/28/05 */
 int copy_only = 0;            /* 1=copying archive entries only */
diff -u orig/zip-3.0/zip.c chan/zip-3.0/zip.c
--- orig/zip-3.0/zip.c	2023-02-06 19:49:42.000000000 +0100
+++ chan/zip-3.0/zip.c	2023-02-06 19:47:26.000000000 +0100
@@ -1942,6 +1942,7 @@
 #ifdef UNICODE_TEST
 #define o_sC            0x146
 #endif
+#define o_nas           0x147
 
 
 /* the below is mainly from the old main command line
@@ -2042,6 +2043,7 @@
     {"N",  "notes",       o_NO_VALUE,       o_NOT_NEGATABLE, 'N',  "add notes as entry comments"},
 #endif
     {"o",  "latest-time", o_NO_VALUE,       o_NOT_NEGATABLE, 'o',  "use latest entry time as archive time"},
+    {"",   "no-add-suffix", o_NO_VALUE,     o_NOT_NEGATABLE, o_nas, "do not add .zip suffix to archive name without ."},
     {"O",  "output-file", o_REQUIRED_VALUE, o_NOT_NEGATABLE, 'O',  "set out zipfile different than in zipfile"},
     {"p",  "paths",       o_NO_VALUE,       o_NOT_NEGATABLE, 'p',  "store paths"},
     {"P",  "password",    o_REQUIRED_VALUE, o_NOT_NEGATABLE, 'P',  "encrypt entries, option value is password"},
@@ -2378,6 +2380,7 @@
   before = 0;             /* 0=ignore, else exclude files before this time */
   after = 0;              /* 0=ignore, else exclude files newer than this time */
 
+  no_add_suffix = 0       /* 0=add .zip if no . as usual, else use archive name unchanged */
   special = ".Z:.zip:.zoo:.arc:.lzh:.arj"; /* List of special suffixes */
   key = NULL;             /* Scramble password if scrambling */
   key_needed = 0;         /* Need scramble password */
@@ -3299,6 +3302,11 @@
           break;
 #endif
 
+        case o_nas:
+          no_add_suffix = 1;
+          break;
+
+
         case o_NON_OPTION_ARG:
           /* not an option */
           /* no more options as permuting */
@@ -3340,8 +3348,14 @@
 #endif /* !MACOS && !WINDLL */
               {
                 /* name of zipfile */
-                if ((zipfile = ziptyp(value)) == NULL) {
-                  ZIPERR(ZE_MEM, "was processing arguments");
+                if (no_add_suffix) {
+                  zipfile = value;
+                }
+                else {
+                  if ((zipfile = ziptyp(value)) == NULL) {
+                    ZIPERR(ZE_MEM, "was processing arguments");
+                  }
+                  free(value);
                 }
                 /* read zipfile if exists */
                 /*
@@ -3349,7 +3363,6 @@
                   ZIPERR(r, zipfile);
                 }
                 */
-                free(value);
               }
               if (show_what_doing) {
                 fprintf(mesg, "sd: Zipfile name '%s'\n", zipfile);
diff -u orig/zip-3.0/zip.h chan/zip-3.0/zip.h
--- orig/zip-3.0/zip.h	2008-05-25 19:23:22.000000000 +0200
+++ chan/zip-3.0/zip.h	2023-02-06 19:43:41.000000000 +0100
@@ -442,6 +442,7 @@
  extern int use_privileges;     /* use security privilege overrides */
 #endif
 extern int use_descriptors;     /* use data descriptors (extended headings) */
+extern int no_add_suffix;       /* do not add suffix .zip to archive names without . */
 extern int allow_empty_archive; /* if no files, create empty archive anyway */
 extern int copy_only;           /* 1 = copy archive with no changes */
 extern int zip_to_stdout;       /* output to stdout */

But getting something like that accepted and distributed, and for Emacs
to tell whether the installed zip has that option or not, seems like a
lot of work.

bug#61326: Adding --no-add-suffix to zip patch

[Ruijie Yu via Bug reports for GNU Emacs, the Swiss army knife of text editors]

On Feb 7, 2023, at 02:58, Adam Sjøgren via Bug reports for GNU Emacs, the Swiss army knife of text editors <bug-gnu-emacs@gnu.org> wrote:
> 
> Adding a '--no-add-suffix' option to zip 3.0 is not too bad:
> 
> diff -u orig/zip-3.0/globals.c chan/zip-3.0/globals.c
> --- orig/zip-3.0/globals.c    2008-05-25 19:26:38.000000000 +0200
> +++ chan/zip-3.0/globals.c    2023-02-06 19:42:48.000000000 +0100
> @@ -106,6 +106,7 @@
> int noisy = 1;                /* 0=quiet operation */
> int extra_fields = 1;         /* 0=create minimum, 1=don't copy old, 2=keep old */
> int use_descriptors = 0;      /* 1=use data descriptors 12/29/04 */
> +int no_add_suffix = 0;        /* 1=do not add suffix .zip to archive names without . */
> int zip_to_stdout = 0;        /* output zipfile to stdout 12/30/04 */
> int allow_empty_archive = 0;  /* if no files, create empty archive anyway 12/28/05 */
> int copy_only = 0;            /* 1=copying archive entries only */
> diff -u orig/zip-3.0/zip.c chan/zip-3.0/zip.c
> --- orig/zip-3.0/zip.c    2023-02-06 19:49:42.000000000 +0100
> +++ chan/zip-3.0/zip.c    2023-02-06 19:47:26.000000000 +0100
> @@ -1942,6 +1942,7 @@
> #ifdef UNICODE_TEST
> #define o_sC            0x146
> #endif
> +#define o_nas           0x147
> 
> 
> /* the below is mainly from the old main command line
> @@ -2042,6 +2043,7 @@
>     {"N",  "notes",       o_NO_VALUE,       o_NOT_NEGATABLE, 'N',  "add notes as entry comments"},
> #endif
>     {"o",  "latest-time", o_NO_VALUE,       o_NOT_NEGATABLE, 'o',  "use latest entry time as archive time"},
> +    {"",   "no-add-suffix", o_NO_VALUE,     o_NOT_NEGATABLE, o_nas, "do not add .zip suffix to archive name without ."},
>     {"O",  "output-file", o_REQUIRED_VALUE, o_NOT_NEGATABLE, 'O',  "set out zipfile different than in zipfile"},
>     {"p",  "paths",       o_NO_VALUE,       o_NOT_NEGATABLE, 'p',  "store paths"},
>     {"P",  "password",    o_REQUIRED_VALUE, o_NOT_NEGATABLE, 'P',  "encrypt entries, option value is password"},
> @@ -2378,6 +2380,7 @@
>   before = 0;             /* 0=ignore, else exclude files before this time */
>   after = 0;              /* 0=ignore, else exclude files newer than this time */
> 
> +  no_add_suffix = 0       /* 0=add .zip if no . as usual, else use archive name unchanged */
>   special = ".Z:.zip:.zoo:.arc:.lzh:.arj"; /* List of special suffixes */
>   key = NULL;             /* Scramble password if scrambling */
>   key_needed = 0;         /* Need scramble password */
> @@ -3299,6 +3302,11 @@
>           break;
> #endif
> 
> +        case o_nas:
> +          no_add_suffix = 1;
> +          break;
> +
> +
>         case o_NON_OPTION_ARG:
>           /* not an option */
>           /* no more options as permuting */
> @@ -3340,8 +3348,14 @@
> #endif /* !MACOS && !WINDLL */
>               {
>                 /* name of zipfile */
> -                if ((zipfile = ziptyp(value)) == NULL) {
> -                  ZIPERR(ZE_MEM, "was processing arguments");
> +                if (no_add_suffix) {
> +                  zipfile = value;
> +                }
> +                else {
> +                  if ((zipfile = ziptyp(value)) == NULL) {
> +                    ZIPERR(ZE_MEM, "was processing arguments");
> +                  }
> +                  free(value);
>                 }
>                 /* read zipfile if exists */
>                 /*
> @@ -3349,7 +3363,6 @@
>                   ZIPERR(r, zipfile);
>                 }
>                 */
> -                free(value);
>               }
>               if (show_what_doing) {
>                 fprintf(mesg, "sd: Zipfile name '%s'\n", zipfile);
> diff -u orig/zip-3.0/zip.h chan/zip-3.0/zip.h
> --- orig/zip-3.0/zip.h    2008-05-25 19:23:22.000000000 +0200
> +++ chan/zip-3.0/zip.h    2023-02-06 19:43:41.000000000 +0100
> @@ -442,6 +442,7 @@
>  extern int use_privileges;     /* use security privilege overrides */
> #endif
> extern int use_descriptors;     /* use data descriptors (extended headings) */
> +extern int no_add_suffix;       /* do not add suffix .zip to archive names without . */
> extern int allow_empty_archive; /* if no files, create empty archive anyway */
> extern int copy_only;           /* 1 = copy archive with no changes */
> extern int zip_to_stdout;       /* output to stdout */
> 
> But getting something like that accepted and distributed, and for Emacs
> to tell whether the installed zip has that option or not, seems like a
> lot of work.

Maybe, at least in the meantime, we change it such that all write operations for zip create files in temp, and move to / overwrite the original file when done? Although I don’t have a full understanding on how that would be done and whether there are problems along with it. 

bug#61326: Adding --no-add-suffix to zip patch

[Eli Zaretskii]

> Cc: 61326@debbugs.gnu.org
> Date: Tue, 7 Feb 2023 09:31:11 +0800
> From:  Ruijie Yu via "Bug reports for GNU Emacs,
>  the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>
> 
> Maybe, at least in the meantime, we change it such that all write operations for zip create files in temp, and move to / overwrite the original file when done? Although I don’t have a full understanding on how that would be done and whether there are problems along with it. 

How about submitting a patch to do that, but only when the original
file doesn't have an extension?

bug#61326: Adding --no-add-suffix to zip patch

[Ruijie Yu via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Eli Zaretskii <eliz@gnu.org> writes:

>> Cc: 61326@debbugs.gnu.org
>> Date: Tue, 7 Feb 2023 09:31:11 +0800
>> From:  Ruijie Yu via "Bug reports for GNU Emacs,
>>  the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>
>>
>> Maybe, at least in the meantime, we change it such that all write operations
>> for zip create files in temp, and move to / overwrite the original file when
>> done? Although I don’t have a full understanding on how that would be done and
>> whether there are problems along with it.
>
> How about submitting a patch to do that, but only when the original
> file doesn't have an extension?

Will do.  Will report back in 2-3 days unless someone else gets to it
first.

Best,


RY

bug#61326: Adding --no-add-suffix to zip patch

[Eli Zaretskii]

> From: Ruijie Yu <ruijie@netyu.xyz>
> Cc: asjo@koldfront.dk, 61326@debbugs.gnu.org
> Date: Tue, 07 Feb 2023 21:53:12 +0800
> 
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> >> Cc: 61326@debbugs.gnu.org
> >> Date: Tue, 7 Feb 2023 09:31:11 +0800
> >> From:  Ruijie Yu via "Bug reports for GNU Emacs,
> >>  the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>
> >>
> >> Maybe, at least in the meantime, we change it such that all write operations
> >> for zip create files in temp, and move to / overwrite the original file when
> >> done? Although I don’t have a full understanding on how that would be done and
> >> whether there are problems along with it.
> >
> > How about submitting a patch to do that, but only when the original
> > file doesn't have an extension?
> 
> Will do.  Will report back in 2-3 days unless someone else gets to it
> first.

Thanks in advance.

bug#61326: [DRAFT PATCH] Work around zip's filename extension limitation (was: Adding --no-add-suffix to zip patch)

[Ruijie Yu via Bug reports for GNU Emacs, the Swiss army knife of text editors]

[-- Attachment #1: Type: text/plain, Size: 1539 bytes --]


Ruijie Yu via "Bug reports for GNU Emacs, the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org> writes:

> Eli Zaretskii <eliz@gnu.org> writes:
>
>>> Cc: 61326@debbugs.gnu.org
>>> Date: Tue, 7 Feb 2023 09:31:11 +0800
>>> From:  Ruijie Yu via "Bug reports for GNU Emacs,
>>>  the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>
>>>
>>> Maybe, at least in the meantime, we change it such that all write operations
>>> for zip create files in temp, and move to / overwrite the original file when
>>> done? Although I don’t have a full understanding on how that would be done and
>>> whether there are problems along with it.
>>
>> How about submitting a patch to do that, but only when the original
>> file doesn't have an extension?
>
> Will do.  Will report back in 2-3 days unless someone else gets to it
> first.
>
> Best,
>
>
> RY

Here is a preliminary patch that contains some "REVIEW" comments where I
need inputs.  I have tested with the following recipe and things seem to
work correctly:

$ touch 1 2 && zip z 1 2 && mv z.zip z
$ ls -l
.... 1
.... 2
.... z
$ src/emacs -Q
RET          ; open file "1" from within "z" archive
s C-x C-s    ; insert "s" and save file "1"
C-x C-c      ; exit
$ ls -l      # notice that no "z.zip" exists, and "z" is correctly updated
.... 1
.... 2
.... z

Patch based on 907fd1f7ff402f9d226ebb3b891ea5b54fac1d1c which is ~3 days
old.  I will amend the commit (and rebase if necessary) according to ML
reviews and further progress.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-lisp-arc-mode.el-Work-around-zip-s-filename-limitati.patch --]
[-- Type: text/x-patch, Size: 6335 bytes --]

From bc086395929520a66eb928fd5d3baf6c9fa79bb5 Mon Sep 17 00:00:00 2001
From: Ruijie Yu <ruijie+git@netyu.xyz>
Date: Thu, 9 Feb 2023 00:45:19 +0800
Subject: [PATCH] lisp/arc-mode.el Work around zip's filename limitations on
 extension

[DRAFT PATCH]

Fixes 61326.  The "zip" executable requires that the named archive must have
an extension, else it attaches ".zip" to the supplied file name, causing
incorrect behaviors.  This patch looks for such scenarios and temporarily
rename extension-less archives so that "zip" would function correctly.

TODO:

1. Address all REVIEW points.
2. Make sure other write operations, in addition to zip-write-member, are
fixed.
3. Tests? (I might need some pointers as to where existing tests are and how
to write them.)
---
 lisp/arc-mode.el | 82 +++++++++++++++++++++++++++++++-----------------
 1 file changed, 54 insertions(+), 28 deletions(-)

diff --git a/lisp/arc-mode.el b/lisp/arc-mode.el
index 6f3e922880d..ac8c7cefa89 100644
--- a/lisp/arc-mode.el
+++ b/lisp/arc-mode.el
@@ -1350,42 +1350,61 @@ archive-write-file-member
       (setq last-coding-system-used archive-member-coding-system))
   t)

-(defun archive-*-write-file-member (archive descr command)
+;; REVIEW: is there a better name than AVOID-EXTLESS-P?
+(defun archive-*-write-file-member (archive descr command
+                                            &optional avoid-extless-p)
   (let* ((archive (expand-file-name archive))
          (ename (archive--file-desc-ext-file-name descr))
          (tmpfile (expand-file-name ename archive-tmpdir))
          (top (directory-file-name (file-name-as-directory archive-tmpdir)))
     (default-directory (file-name-as-directory top)))
+    ;; REVIEW: the diff here is because the previous code had TAB's
+    ;; (while assuming each TAB is 4 spaces), and my Emacs replaced
+    ;; them with spaces.  What is the status quo on this kind of diff?
+    ;; I can remove them if we consider this change excessive and/or
+    ;; intrusive.
     (unwind-protect
         (progn
           (make-directory (file-name-directory tmpfile) t)
-     ;; If the member is itself an archive, write it without
-     ;; the dired-like listing we created.
-     (if (eq major-mode 'archive-mode)
-         (archive-write-file tmpfile)
-       (write-region nil nil tmpfile nil 'nomessage))
-     ;; basic-save-buffer needs last-coding-system-used to have
-     ;; the value used to write the file, so save it before any
-     ;; further processing clobbers it (we restore it in
-     ;; archive-write-file-member, above).
-     (setq archive-member-coding-system last-coding-system-used)
-     (if (archive--file-desc-mode descr)
-         ;; Set the file modes, but make sure we can read it.
-         (set-file-modes tmpfile
-                         (logior ?\400 (archive--file-desc-mode descr))))
-     (setq ename
-       (encode-coding-string ename archive-file-name-coding-system))
+          ;; If the member is itself an archive, write it without
+          ;; the dired-like listing we created.
+          (if (eq major-mode 'archive-mode)
+              (archive-write-file tmpfile)
+            (write-region nil nil tmpfile nil 'nomessage))
+          ;; basic-save-buffer needs last-coding-system-used to have
+          ;; the value used to write the file, so save it before any
+          ;; further processing clobbers it (we restore it in
+          ;; archive-write-file-member, above).
+          (setq archive-member-coding-system last-coding-system-used)
+          (if (archive--file-desc-mode descr)
+              ;; Set the file modes, but make sure we can read it.
+              (set-file-modes tmpfile
+                              (logior ?\400 (archive--file-desc-mode descr))))
+          (setq ename
+                (encode-coding-string ename archive-file-name-coding-system))
           (let* ((coding-system-for-write 'no-conversion)
-        (default-directory (file-name-as-directory archive-tmpdir))
-        (exitcode (apply #'call-process
-                 (car command)
-                 nil
-                 nil
-                 nil
-                 (append (cdr command)
-                     (list archive ename)))))
-            (or (zerop exitcode)
-       (error "Updating was unsuccessful (%S)" exitcode))))
+                 (default-directory (file-name-as-directory archive-tmpdir))
+                 (safe-archive
+                  (if avoid-extless-p
+                      (make-temp-name
+                       (expand-file-name (concat archive "_tmp.")))
+                    archive))
+                 (maybe-rename
+                  (lambda (newname)
+                    (when avoid-extless-p
+                      (with-current-buffer archive-superior-buffer
+                        (rename-visited-file newname))))))
+            ;; REVIEW: is `unwind-protect' necessary here?
+            (prog2 (funcall maybe-rename safe-archive)
+                (let ((exitcode
+                       (apply #'call-process (car command)
+                              nil nil nil
+                              (append (cdr command)
+                                      (list safe-archive ename)))))
+                  (or (zerop exitcode)
+                      (error "Updating was unsuccessful (%S)"
+                             exitcode)))
+              (funcall maybe-rename archive))))
       (archive-delete-local tmpfile))))

 (defun archive-write-file (&optional file)
@@ -2048,12 +2067,19 @@ archive--file-desc-case-fiddled
   (not (eq (archive--file-desc-int-file-name fd)
            (archive--file-desc-ext-file-name fd))))

+(defun archive--file-name-zip-extless-p (fname)
+  ;; zip's rule: if the filename contains "." anywhere in the name
+  ;; (including obscure names like ".foo" and "bar."), then this
+  ;; filename is considered to have an extension.
+  (not (seq-contains-p (file-name-nondirectory fname) ?. #'eq)))
+
 (defun archive-zip-write-file-member (archive descr)
   (archive-*-write-file-member
    archive
    descr
    (if (archive--file-desc-case-fiddled descr)
-       archive-zip-update-case archive-zip-update)))
+       archive-zip-update-case archive-zip-update)
+   (archive--file-name-zip-extless-p archive)))

 (defun archive-zip-chmod-entry (newmode files)
   (save-restriction
--
2.39.1


[-- Attachment #3: Type: text/plain, Size: 12 bytes --]


Best,


RY

bug#61326: [DRAFT PATCH] Work around zip's filename extension limitation (was: Adding --no-add-suffix to zip patch)

[Eli Zaretskii]

> From: Ruijie Yu <ruijie@netyu.xyz>
> Cc: Eli Zaretskii <eliz@gnu.org>, asjo@koldfront.dk
> Date: Thu, 09 Feb 2023 00:48:15 +0800
> 
> Here is a preliminary patch that contains some "REVIEW" comments where I
> need inputs.

Thanks, but could you perhaps post diffs disregarding the whitespace
changes?  That would make it easier to review the real changes.

> -(defun archive-*-write-file-member (archive descr command)
> +;; REVIEW: is there a better name than AVOID-EXTLESS-P?
> +(defun archive-*-write-file-member (archive descr command
> +                                            &optional avoid-extless-p)

ensure-extension?

> +    ;; REVIEW: the diff here is because the previous code had TAB's
> +    ;; (while assuming each TAB is 4 spaces), and my Emacs replaced
> +    ;; them with spaces.  What is the status quo on this kind of diff?
> +    ;; I can remove them if we consider this change excessive and/or
> +    ;; intrusive.

TABs in Emacs are by default 8 columns, not 4.

It is OK to convert TABs to spaces when changing the code in Lisp, but
please do that only for the last commit, to make the review process
easier.  For all the draft versions, please use "git diff" options
that cause Git to ignore changes in whitespace.

Thanks.

bug#61326: [DRAFT PATCH v2] Work around zip's filename extension limitation (was: Adding --no-add-suffix to zip patch)

[Ruijie Yu via Bug reports for GNU Emacs, the Swiss army knife of text editors]

[-- Attachment #1: Type: text/plain, Size: 2051 bytes --]


Eli Zaretskii <eliz@gnu.org> writes:

>> From: Ruijie Yu <ruijie@netyu.xyz>
>> Cc: Eli Zaretskii <eliz@gnu.org>, asjo@koldfront.dk
>> Date: Thu, 09 Feb 2023 00:48:15 +0800
>>
>> Here is a preliminary patch that contains some "REVIEW" comments where I
>> need inputs.
>
> Thanks, but could you perhaps post diffs disregarding the whitespace
> changes?  That would make it easier to review the real changes.

Thanks, done.

>> -(defun archive-*-write-file-member (archive descr command)
>> +;; REVIEW: is there a better name than AVOID-EXTLESS-P?
>> +(defun archive-*-write-file-member (archive descr command
>> +                                            &optional avoid-extless-p)
>
> ensure-extension?

That sounds better, and I have renamed the variable in this iteration.

>> +    ;; REVIEW: the diff here is because the previous code had TAB's
>> +    ;; (while assuming each TAB is 4 spaces), and my Emacs replaced
>> +    ;; them with spaces.  What is the status quo on this kind of diff?
>> +    ;; I can remove them if we consider this change excessive and/or
>> +    ;; intrusive.
>
> TABs in Emacs are by default 8 columns, not 4.
>
> It is OK to convert TABs to spaces when changing the code in Lisp, but
> please do that only for the last commit, to make the review process
> easier.  For all the draft versions, please use "git diff" options
> that cause Git to ignore changes in whitespace.
>
> Thanks.

Thanks, I will keep that in mind.  I have also made some rearrangements
in the code to minimize delta.

In addition, I fixed an issue from the first iteration: in the first
iteration, the user would be prompted to revert buffer from disk if the
rename was in action; in this iteration this should no longer be the
case -- that is, the user should expect no difference between filenames
with extensions and without.

As pointed out in the commit message, two things remain: to ensure that
all write operations to extensionless zip archives behave correctly, and
to have some way to test that things continue to work in the future.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-lisp-arc-mode.el-Work-around-zip-s-filename-limitati.patch --]
[-- Type: text/x-patch, Size: 4186 bytes --]

From bc56c17082ea3ff2a76308f14310908853e3a2d1 Mon Sep 17 00:00:00 2001
From: Ruijie Yu <ruijie+git@netyu.xyz>
Date: Thu, 9 Feb 2023 00:45:19 +0800
Subject: [PATCH] lisp/arc-mode.el Work around zip's filename limitations on
 extension

[DRAFT PATCH]

Fixes 61326.  The "zip" executable requires that the named archive must have
an extension, else it attaches ".zip" to the supplied file name, causing
incorrect behaviors.  This patch looks for such scenarios and temporarily
rename extension-less archives so that "zip" would function correctly.

TODO:

1. Make sure other write operations, in addition to zip-write-member, are
fixed.
2. Tests? (I might need some pointers as to where existing tests are and how
to write them.)
---
 lisp/arc-mode.el | 38 +++++++++++++++++++++++++++++---------
 1 file changed, 29 insertions(+), 9 deletions(-)

diff --git a/lisp/arc-mode.el b/lisp/arc-mode.el
index 6f3e922880d..cd7f4ca1134 100644
--- a/lisp/arc-mode.el
+++ b/lisp/arc-mode.el
@@ -1350,14 +1350,23 @@ archive-write-file-member
       (setq last-coding-system-used archive-member-coding-system))
   t)

-(defun archive-*-write-file-member (archive descr command)
+(defun archive-*-write-file-member
+    (archive descr command &optional ensure-extension)
   (let* ((archive (expand-file-name archive))
+         (real-archive archive)
+         (archive (if ensure-extension
+                      (make-temp-name
+                       (expand-file-name (concat archive "_tmp.")))
+                    archive))
          (ename (archive--file-desc-ext-file-name descr))
          (tmpfile (expand-file-name ename archive-tmpdir))
          (top (directory-file-name (file-name-as-directory archive-tmpdir)))
     (default-directory (file-name-as-directory top)))
     (unwind-protect
-        (progn
+        (cl-flet ((maybe-rename (newname)
+                    (when ensure-extension
+                      (with-current-buffer archive-superior-buffer
+                        (rename-visited-file newname)))))
           (make-directory (file-name-directory tmpfile) t)
      ;; If the member is itself an archive, write it without
      ;; the dired-like listing we created.
@@ -1376,16 +1385,20 @@ archive-*-write-file-member
      (setq ename
        (encode-coding-string ename archive-file-name-coding-system))
           (let* ((coding-system-for-write 'no-conversion)
-        (default-directory (file-name-as-directory archive-tmpdir))
-        (exitcode (apply #'call-process
-                 (car command)
-                 nil
-                 nil
-                 nil
+        (default-directory (file-name-as-directory archive-tmpdir)))
+            (unwind-protect
+                (progn
+                  (maybe-rename archive)
+                  (let ((exitcode
+                         (apply #'call-process (car command)
+                                nil nil nil
                  (append (cdr command)
                      (list archive ename)))))
             (or (zerop exitcode)
        (error "Updating was unsuccessful (%S)" exitcode))))
+              (progn (maybe-rename real-archive)
+                     (with-current-buffer archive-superior-buffer
+                       (revert-buffer nil t))))))
       (archive-delete-local tmpfile))))

 (defun archive-write-file (&optional file)
@@ -2048,12 +2061,19 @@ archive--file-desc-case-fiddled
   (not (eq (archive--file-desc-int-file-name fd)
            (archive--file-desc-ext-file-name fd))))

+(defun archive--file-name-zip-extless-p (fname)
+  ;; zip's rule: if the filename contains "." anywhere in the name
+  ;; (including obscure names like ".foo" and "bar."), then this
+  ;; filename is considered to have an extension.
+  (not (seq-contains-p (file-name-nondirectory fname) ?. #'eq)))
+
 (defun archive-zip-write-file-member (archive descr)
   (archive-*-write-file-member
    archive
    descr
    (if (archive--file-desc-case-fiddled descr)
-       archive-zip-update-case archive-zip-update)))
+       archive-zip-update-case archive-zip-update)
+   (archive--file-name-zip-extless-p archive)))

 (defun archive-zip-chmod-entry (newmode files)
   (save-restriction
--
2.39.1


[-- Attachment #3: Type: text/plain, Size: 12 bytes --]


Best,


RY

bug#61326: [DRAFT PATCH v3] Work around zip's filename extension limitation (was: Adding --no-add-suffix to zip patch)

[Ruijie Yu via Bug reports for GNU Emacs, the Swiss army knife of text editors]

[-- Attachment #1: Type: text/plain, Size: 760 bytes --]


I believe the functionalities (both update and deletion) are done in
this patch.  I have not added any tests nor updates on documentation, as
I am not sure what and where they should be.

I have done some basic tests locally to confirm that it works:
- update subfile in a zip archive named "z"
- delete subfile in a zip archive named "z"
- update subfile in a zip archive named "x.zip"
- delete subfile in a zip archive named "x.zip"

FTR, if this is of any importance: the filesystem containing the
archives "z" and "x.zip" is btrfs, and /tmp is ext2 on zram.  Not sure
if any problems would arise in other filesystems / OSes -- hopefully
someone else on those platforms can confirm that it works.

Comments welcome.  Particularly on my choices of naming.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-lisp-arc-mode.el-Work-around-zip-s-filename-limitati.patch --]
[-- Type: text/x-patch, Size: 4444 bytes --]

From 0766bec8071c7c793d6c81211577832d9a7a78f9 Mon Sep 17 00:00:00 2001
From: Ruijie Yu <ruijie+git@netyu.xyz>
Date: Thu, 9 Feb 2023 00:45:19 +0800
Subject: [PATCH] lisp/arc-mode.el Work around zip's filename limitations on
 extension

[DRAFT PATCH]

Fixes 61326.  The "zip" executable requires that the named archive must have
an extension, else it attaches ".zip" to the supplied file name, causing
incorrect behaviors.  This patch looks for such scenarios and temporarily
rename extension-less archives so that "zip" would function correctly.

Currently there are no tests.  I might need some pointers as to where existing
tests are and how to write them.
---
 lisp/arc-mode.el | 64 ++++++++++++++++++++++++++++++++++++------------
 1 file changed, 48 insertions(+), 16 deletions(-)

diff --git a/lisp/arc-mode.el b/lisp/arc-mode.el
index 6f3e922880d..875b3971086 100644
--- a/lisp/arc-mode.el
+++ b/lisp/arc-mode.el
@@ -645,6 +645,49 @@ archive-get-descr
       (if (not noerror)
           (error "Line does not describe a member of the archive")))))
 ;; -------------------------------------------------------------------------
+;;; Section: Helper functions for requiring filename extensions
+
+(defun archive--act-files (command files)
+  (lambda (archive)
+    (apply #'call-process (car command)
+           nil nil nil (append (cdr command) (cons archive files)))))
+
+(defun archive--need-rename-p (&optional archive)
+  (let ((archive
+         (file-name-nondirectory (or archive buffer-file-name))))
+    (cl-case archive-subtype
+      ((zip) (not (seq-contains-p archive ?. #'eq))))))
+
+(defun archive--ensure-extension (archive ensure-extension)
+  (if ensure-extension
+      (make-temp-name (expand-file-name (concat archive "_tmp.")))
+    archive))
+
+(defun archive--maybe-rename (newname need-rename-p)
+  ;; Operating with archive as current buffer, and protect
+  ;; `default-directory' from being modified in `rename-visited-file'.
+  (when need-rename-p
+    (let ((default-directory default-directory))
+      (rename-visited-file newname))))
+
+(defun archive--with-ensure-extension (archive proc-fn)
+  (let ((saved default-directory))
+    (with-current-buffer (find-buffer-visiting archive)
+      (let ((ensure-extension (archive--need-rename-p))
+            (default-directory saved))
+        (unwind-protect
+            ;; Some archive programs (like zip) expect filenames to
+            ;; have an extension, so if necessary, temporarily rename
+            ;; an extensionless file for write accesses.
+            (let ((archive (archive--ensure-extension
+                            archive ensure-extension)))
+              (archive--maybe-rename archive ensure-extension)
+              (let ((exitcode (funcall proc-fn archive)))
+                (or (zerop exitcode)
+                    (error "Updating was unsuccessful (%S)" exitcode))))
+          (progn (archive--maybe-rename archive ensure-extension)
+                 (revert-buffer nil t)))))))
+;; -------------------------------------------------------------------------
 ;;; Section: the mode definition

 ;;;###autoload
@@ -1376,16 +1419,9 @@ archive-*-write-file-member
      (setq ename
        (encode-coding-string ename archive-file-name-coding-system))
           (let* ((coding-system-for-write 'no-conversion)
-        (default-directory (file-name-as-directory archive-tmpdir))
-        (exitcode (apply #'call-process
-                 (car command)
-                 nil
-                 nil
-                 nil
-                 (append (cdr command)
-                     (list archive ename)))))
-            (or (zerop exitcode)
-       (error "Updating was unsuccessful (%S)" exitcode))))
+                 (default-directory (file-name-as-directory archive-tmpdir)))
+            (archive--with-ensure-extension
+             archive (archive--act-files command (list ename)))))
       (archive-delete-local tmpfile))))

 (defun archive-write-file (&optional file)
@@ -1539,12 +1575,8 @@ archive-expunge
         (revert-buffer))))))

 (defun archive-*-expunge (archive files command)
-  (apply #'call-process
-    (car command)
-    nil
-    nil
-    nil
-    (append (cdr command) (cons archive files))))
+  (archive--with-ensure-extension
+   archive (archive--act-files command files)))

 (defun archive-rename-entry (newname)
   "Change the name associated with this entry in the archive file."
--
2.39.1


[-- Attachment #3: Type: text/plain, Size: 15 bytes --]


--
Best,


RY

bug#61326: [DRAFT PATCH v3] Work around zip's filename extension limitation (was: Adding --no-add-suffix to zip patch)

[Eli Zaretskii]

> From: Ruijie Yu <ruijie@netyu.xyz>
> Cc: asjo@koldfront.dk, 61326@debbugs.gnu.org
> Date: Mon, 13 Feb 2023 18:35:17 +0800
> 
> I believe the functionalities (both update and deletion) are done in
> this patch.  I have not added any tests nor updates on documentation, as
> I am not sure what and where they should be.

Thanks.

I don't think we need any updates to the documentation, as this just
fixes a subtle bug.  It would be nice to have a few tests, though.
Please add them to test/lisp/arc-mode-tests.el.

I see that you don't have a copyright assignment on file, so would you
like me to send you the form to fill, which will start the legal
paperwork of assigning the copyright to the FSF?  Once that is
completed, we can install these changes.

Thanks.

bug#61326: [DRAFT PATCH v3] Work around zip's filename extension limitation (was: Adding --no-add-suffix to zip patch)

[Ruijie Yu via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Ruijie Yu <ruijie@netyu.xyz>
>> Cc: asjo@koldfront.dk, 61326@debbugs.gnu.org
>> Date: Mon, 13 Feb 2023 18:35:17 +0800
>>
>> I believe the functionalities (both update and deletion) are done in
>> this patch.  I have not added any tests nor updates on documentation, as
>> I am not sure what and where they should be.
>
> Thanks.
>
> I don't think we need any updates to the documentation, as this just
> fixes a subtle bug.  It would be nice to have a few tests, though.
> Please add them to test/lisp/arc-mode-tests.el.

Sure, will do in a few days.

> I see that you don't have a copyright assignment on file, so would you
> like me to send you the form to fill, which will start the legal
> paperwork of assigning the copyright to the FSF?  Once that is
> completed, we can install these changes.
>
> Thanks.

Please do, thanks.

--
Best,


RY

bug#61326: [DRAFT PATCH v3] Work around zip's filename extension limitation (was: Adding --no-add-suffix to zip patch)

[Eli Zaretskii]

> From: Ruijie Yu <ruijie@netyu.xyz>
> Cc: asjo@koldfront.dk, 61326@debbugs.gnu.org
> Date: Sat, 04 Mar 2023 22:56:21 +0800
> 
> > I see that you don't have a copyright assignment on file, so would you
> > like me to send you the form to fill, which will start the legal
> > paperwork of assigning the copyright to the FSF?  Once that is
> > completed, we can install these changes.
> >
> > Thanks.
> 
> Please do, thanks.

Form sent off-list.

bug#61326: [DRAFT PATCH v3] Work around zip's filename extension limitation (was: Adding --no-add-suffix to zip patch)

[Ruijie Yu via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Ruijie Yu <ruijie@netyu.xyz>
>> Cc: asjo@koldfront.dk, 61326@debbugs.gnu.org
>> Date: Sat, 04 Mar 2023 22:56:21 +0800
>>
>> > I see that you don't have a copyright assignment on file, so would you
>> > like me to send you the form to fill, which will start the legal
>> > paperwork of assigning the copyright to the FSF?  Once that is
>> > completed, we can install these changes.
>> >
>> > Thanks.
>>
>> Please do, thanks.
>
> Form sent off-list.

Response sent to assign@gnu, thanks.

By the way, while writing tests, I see a need to modify
`archive-expunge' to accept an additional optional argument FORCE acting
as the prefix argument.  I suppose if this is favored, I should probably
say something in the etc/NEWS file (as well as the docstring, and maybe
an info page somewhere)?  FTR, previously prefix arguments are ignored
for `archive-expunge'.

The reason for the need of a FORCE argument is so that I want to ensure
that file contents are correct after deleting a member using
archive-mode functionalities without getting prompted for confirmation.
However, `archive-expunge' ATM requires a user prompt via its baked-in
`yes-or-no-p' call.

An alternative to the above is to move the meat of said function into a
new internal helper function which simply skips over prompting the user.
This would keep the `archive-expunge' API untouched.

A new draft iteration should be here in a few days, with the question(s)
above answered.  Thanks.

--
Best,


RY

bug#61326: [DRAFT PATCH v3] Work around zip's filename extension limitation (was: Adding --no-add-suffix to zip patch)

[Eli Zaretskii]

> From: Ruijie Yu <ruijie@netyu.xyz>
> Cc: asjo@koldfront.dk, 61326@debbugs.gnu.org
> Date: Sun, 05 Mar 2023 23:23:59 +0800
> 
> > Form sent off-list.
> 
> Response sent to assign@gnu, thanks.

Thanks.

> By the way, while writing tests, I see a need to modify
> `archive-expunge' to accept an additional optional argument FORCE acting
> as the prefix argument.  I suppose if this is favored, I should probably
> say something in the etc/NEWS file (as well as the docstring, and maybe
> an info page somewhere)?  FTR, previously prefix arguments are ignored
> for `archive-expunge'.
> 
> The reason for the need of a FORCE argument is so that I want to ensure
> that file contents are correct after deleting a member using
> archive-mode functionalities without getting prompted for confirmation.
> However, `archive-expunge' ATM requires a user prompt via its baked-in
> `yes-or-no-p' call.

This is a separate feature.  In Dired-like user interfaces, we always
ask the user for confirmation before deleting items; 'C-u x' in Dired
AFAIR just avoids messages when no files are flagged for deletion, but
doesn't prevent the confirmation.  So if we are going to treat
archive-expunge differently, I think we'll need a very good reason.

bug#61326: [DRAFT PATCH v4] Work around zip's filename extension limitation (was: Adding --no-add-suffix to zip patch)

[Ruijie Yu via Bug reports for GNU Emacs, the Swiss army knife of text editors]

[-- Attachment #1: Type: text/plain, Size: 2422 bytes --]


Eli Zaretskii <eliz@gnu.org> writes:

> [...]
>> By the way, while writing tests, I see a need to modify
>> `archive-expunge' to accept an additional optional argument FORCE acting
>> as the prefix argument.  I suppose if this is favored, I should probably
>> say something in the etc/NEWS file (as well as the docstring, and maybe
>> an info page somewhere)?  FTR, previously prefix arguments are ignored
>> for `archive-expunge'.
>>
>> The reason for the need of a FORCE argument is so that I want to ensure
>> that file contents are correct after deleting a member using
>> archive-mode functionalities without getting prompted for confirmation.
>> However, `archive-expunge' ATM requires a user prompt via its baked-in
>> `yes-or-no-p' call.
>
> This is a separate feature.  [...] So if we are going to treat
> archive-expunge differently, I think we'll need a very good reason.

I see, thanks for explaining.

In the attached patches I followed the alternative approach which I
described in my previous message: take the meat of the function into a
separate helper function, and use that in the `archive-expunge' function
and in my new test.  If someone else has a good reason for using prefix
argument to "force" deletion, they can ask in a new bug report.

Another question regarding this change: when moving `archive-expunge'
into `archive--expunge-maybe-force', I rewrote the portion that
populates the list of files that are marked for deletion.  Originally it
was using `while' + `setq', and seeing that arc-mode.el already requires
`cl-lib', I turned it into a `cl-do' construct.  Do people have a
preference or does it matter?  I can change this portion back to the
original if there's objection.

To ease the review process, I have broken down the changes into two
patch files.  The first one is merely to take out `archive-expunge' into
helper function `archive--expunge-maybe-force', and the second one is
everything else, including the tests.

The goal for the final patch is to combine these two into one, to make
necessary indentation changes around the portions that I touched, and to
only use the commit message of the second patch _verbatim_ -- so please
verify that this commit message is satisfactory in emacs.git, thanks.

FTR, I ran `make check' and ensured that my changes didn't introduce
regressions, while also noticed that some tests in vc and eglot fail
both before and after my changes.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-REVIEW-pull-out-content-of-arc-expunge-for-nonintera.patch --]
[-- Type: text/x-patch, Size: 2229 bytes --]

From 3b9196a095c6843b7c87b867a82fa7a2d930bb0b Mon Sep 17 00:00:00 2001
From: Ruijie Yu <ruijie+git@netyu.xyz>
Date: Mon, 6 Mar 2023 11:03:32 +0800
Subject: [PATCH 1/2] REVIEW: pull out content of arc-expunge for
 noninteractive deletion

---
 lisp/arc-mode.el | 30 +++++++++++++++++++-----------
 1 file changed, 19 insertions(+), 11 deletions(-)

diff --git a/lisp/arc-mode.el b/lisp/arc-mode.el
index 6f3e922880d..f8d7182597b 100644
--- a/lisp/arc-mode.el
+++ b/lisp/arc-mode.el
@@ -1508,23 +1508,26 @@ archive-chgrp-entry
      (archive-resummarize))
       (error "Setting group is not supported for this archive type"))))

-(defun archive-expunge ()
-  "Do the flagged deletions."
-  (interactive)
-  (let (files)
+(defun archive--expunge-maybe-force (force)
+  (let ((files
     (save-excursion
       (goto-char archive-file-list-start)
-      (while (< (point) archive-file-list-end)
-        (if (= (following-char) ?D)
-       (setq files (cons (archive--file-desc-ext-file-name
+           (cl-do ((files
+                    nil
+                    (prog1
+                        (if (eq (following-char) ?D)
+                            (cons (archive--file-desc-ext-file-name
                           (archive-get-descr))
-                         files)))
-        (forward-line 1)))
-    (setq files (nreverse files))
+                                  files)
+                          files)
+                      (forward-line 1))))
+               ((>= (point) archive-file-list-end)
+                (nreverse files))))))
     (and files
     (or (not archive-read-only)
         (error "Archive is read-only"))
-    (or (yes-or-no-p (format "Really delete %d member%s? "
+         (or force
+             (yes-or-no-p (format "Really delete %d member%s? "
                  (length files)
                  (if (null (cdr files)) "" "s")))
         (error "Operation aborted"))
@@ -1538,6 +1541,11 @@ archive-expunge
           (archive-resummarize)
         (revert-buffer))))))

+(defun archive-expunge ()
+  "Do the flagged deletions."
+  (interactive)
+  (archive--expunge-maybe-force nil))
+
 (defun archive-*-expunge (archive files command)
   (apply #'call-process
     (car command)
--
2.39.2


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: 0002-Handle-modifications-on-extensionless-zips-correctly.patch --]
[-- Type: text/x-patch, Size: 7625 bytes --]

From ba5825f9bdd323a460f4a327670d99ed34df3128 Mon Sep 17 00:00:00 2001
From: Ruijie Yu <ruijie+git@netyu.xyz>
Date: Thu, 9 Feb 2023 00:45:19 +0800
Subject: [PATCH 2/2] Handle modifications on extensionless zips correctly
 (bug#61326)

* lisp/arc-mode.el: Refactor to handle extless zips
* test/lisp/arc-mode-tests.el: New test for correctly handling extless zips
---
 lisp/arc-mode.el            | 64 ++++++++++++++++++++++++++---------
 test/lisp/arc-mode-tests.el | 67 +++++++++++++++++++++++++++++++++++++
 2 files changed, 115 insertions(+), 16 deletions(-)

diff --git a/lisp/arc-mode.el b/lisp/arc-mode.el
index f8d7182597b..47611f13b6f 100644
--- a/lisp/arc-mode.el
+++ b/lisp/arc-mode.el
@@ -645,6 +645,49 @@ archive-get-descr
       (if (not noerror)
           (error "Line does not describe a member of the archive")))))
 ;; -------------------------------------------------------------------------
+;;; Section: Helper functions for requiring filename extensions
+
+(defun archive--act-files (command files)
+  (lambda (archive)
+    (apply #'call-process (car command)
+           nil nil nil (append (cdr command) (cons archive files)))))
+
+(defun archive--need-rename-p (&optional archive)
+  (let ((archive
+         (file-name-nondirectory (or archive buffer-file-name))))
+    (cl-case archive-subtype
+      ((zip) (not (seq-contains-p archive ?. #'eq))))))
+
+(defun archive--ensure-extension (archive ensure-extension)
+  (if ensure-extension
+      (make-temp-name (expand-file-name (concat archive "_tmp.")))
+    archive))
+
+(defun archive--maybe-rename (newname need-rename-p)
+  ;; Operating with archive as current buffer, and protect
+  ;; `default-directory' from being modified in `rename-visited-file'.
+  (when need-rename-p
+    (let ((default-directory default-directory))
+      (rename-visited-file newname))))
+
+(defun archive--with-ensure-extension (archive proc-fn)
+  (let ((saved default-directory))
+    (with-current-buffer (find-buffer-visiting archive)
+      (let ((ensure-extension (archive--need-rename-p))
+            (default-directory saved))
+        (unwind-protect
+            ;; Some archive programs (like zip) expect filenames to
+            ;; have an extension, so if necessary, temporarily rename
+            ;; an extensionless file for write accesses.
+            (let ((archive (archive--ensure-extension
+                            archive ensure-extension)))
+              (archive--maybe-rename archive ensure-extension)
+              (let ((exitcode (funcall proc-fn archive)))
+                (or (zerop exitcode)
+                    (error "Updating was unsuccessful (%S)" exitcode))))
+          (progn (archive--maybe-rename archive ensure-extension)
+                 (revert-buffer nil t)))))))
+;; -------------------------------------------------------------------------
 ;;; Section: the mode definition

 ;;;###autoload
@@ -1376,16 +1419,9 @@ archive-*-write-file-member
      (setq ename
        (encode-coding-string ename archive-file-name-coding-system))
           (let* ((coding-system-for-write 'no-conversion)
-        (default-directory (file-name-as-directory archive-tmpdir))
-        (exitcode (apply #'call-process
-                 (car command)
-                 nil
-                 nil
-                 nil
-                 (append (cdr command)
-                     (list archive ename)))))
-            (or (zerop exitcode)
-       (error "Updating was unsuccessful (%S)" exitcode))))
+                 (default-directory (file-name-as-directory archive-tmpdir)))
+            (archive--with-ensure-extension
+             archive (archive--act-files command (list ename)))))
       (archive-delete-local tmpfile))))

 (defun archive-write-file (&optional file)
@@ -1547,12 +1583,8 @@ archive-expunge
   (archive--expunge-maybe-force nil))

 (defun archive-*-expunge (archive files command)
-  (apply #'call-process
-    (car command)
-    nil
-    nil
-    nil
-    (append (cdr command) (cons archive files))))
+  (archive--with-ensure-extension
+   archive (archive--act-files command files)))

 (defun archive-rename-entry (newname)
   "Change the name associated with this entry in the archive file."
diff --git a/test/lisp/arc-mode-tests.el b/test/lisp/arc-mode-tests.el
index 32bce1b71bd..ae44ef3439c 100644
--- a/test/lisp/arc-mode-tests.el
+++ b/test/lisp/arc-mode-tests.el
@@ -46,6 +46,73 @@ arc-mode-test-zip-extract-gz
       (when (buffer-live-p zip-buffer) (kill-buffer zip-buffer))
       (when (buffer-live-p gz-buffer) (kill-buffer gz-buffer)))))

+(ert-deftest arc-mode-test-zip-ensure-ext ()
+  ;; Bug#61326
+  (skip-unless (executable-find "zip"))
+  (let* ((default-directory arc-mode-tests-data-directory)
+         (base-zip-1 "base-1.zip")
+         (base-zip-2 "base-2.zip")
+         (content-1 '("1" "2"))
+         (content-2 '("3" "4"))
+         (make-file (lambda (name)
+                      (with-temp-buffer
+                        (insert name)
+                        (write-file name))))
+         (make-zip
+          (lambda (zip files)
+            (delete-file zip nil)
+            (funcall (archive--act-files '("zip") files) zip)))
+         (update-fn
+          (lambda (zip-nonempty)
+            (with-current-buffer (find-file-noselect zip-nonempty)
+              (save-excursion
+                (goto-char archive-file-list-start)
+                (save-current-buffer
+                  (archive-extract)
+                  (save-excursion
+                    (goto-char (point-max))
+                    (insert ?a)
+                    (save-buffer))
+                  (kill-buffer (current-buffer)))
+                (archive-extract)
+                ;; [2] must be ?a; [3] must be (eobp)
+                (should (eq (char-after 2) ?a))
+                (should (eq (point-max) 3))))))
+         (delete-fn
+          (lambda (zip-nonempty)
+            (with-current-buffer (find-file-noselect zip-nonempty)
+              ;; mark delete and expunge first entry
+              (save-excursion
+                (goto-char archive-file-list-start)
+                (should (length= archive-files 2))
+                (archive-flag-deleted 1)
+                (archive--expunge-maybe-force t)
+                (should (length= archive-files 1))))))
+         (test-modify
+          (lambda (zip mod-fn)
+            (let ((zip-base (concat zip ".zip"))
+                  (tag (gensym)))
+              (copy-file base-zip-1 zip t)
+              (copy-file base-zip-2 zip-base t)
+              (file-has-changed-p zip tag)
+              (file-has-changed-p zip-base tag)
+              (funcall mod-fn zip)
+              (should-not (file-has-changed-p zip-base tag))
+              (should (file-has-changed-p zip tag))))))
+    ;; setup: make two zip files with different contents
+    (mapc make-file (append content-1 content-2))
+    (mapc (lambda (args) (apply make-zip args))
+          (list (list base-zip-1 content-1)
+                (list base-zip-2 content-2)))
+    ;; test 1: with "test-update" and "test-update.zip", update
+    ;; "test-update": (1) ensure only "test-update" is modified, (2)
+    ;; ensure the contents of the new member is expected.
+    (funcall test-modify "test-update" update-fn)
+    ;; test 2: with "test-delete" and "test-delete.zip", delete entry
+    ;; from "test-delete": (1) ensure only "test-delete" is modified,
+    ;; (2) ensure the file list is reduced as expected.
+    (funcall test-modify "test-delete" delete-fn)))
+
 (provide 'arc-mode-tests)

 ;;; arc-mode-tests.el ends here
--
2.39.2


[-- Attachment #4: Type: text/plain, Size: 15 bytes --]


--
Best,


RY

bug#61326: [DRAFT PATCH v4] Work around zip's filename extension limitation (was: Adding --no-add-suffix to zip patch)

[Eli Zaretskii]

> From: Ruijie Yu <ruijie@netyu.xyz>
> Cc: asjo@koldfront.dk, 61326@debbugs.gnu.org
> Date: Mon, 06 Mar 2023 12:05:23 +0800
> 
> In the attached patches I followed the alternative approach which I
> described in my previous message: take the meat of the function into a
> separate helper function, and use that in the `archive-expunge' function
> and in my new test.  If someone else has a good reason for using prefix
> argument to "force" deletion, they can ask in a new bug report.
> 
> Another question regarding this change: when moving `archive-expunge'
> into `archive--expunge-maybe-force', I rewrote the portion that
> populates the list of files that are marked for deletion.  Originally it
> was using `while' + `setq', and seeing that arc-mode.el already requires
> `cl-lib', I turned it into a `cl-do' construct.  Do people have a
> preference or does it matter?  I can change this portion back to the
> original if there's objection.

I don't object in principle, but in this case it looks like the
implementation based on cl-do needs much more complex code than the
original?  If so, I'd prefer the original, simpler and
easier-to-understand code.

> To ease the review process, I have broken down the changes into two
> patch files.  The first one is merely to take out `archive-expunge' into
> helper function `archive--expunge-maybe-force', and the second one is
> everything else, including the tests.
> 
> The goal for the final patch is to combine these two into one, to make
> necessary indentation changes around the portions that I touched, and to
> only use the commit message of the second patch _verbatim_ -- so please
> verify that this commit message is satisfactory in emacs.git, thanks.

The commit log message is not detailed enough: it doesn't mention the
functions you modify.  Please see the conventions we follow for log
messages described in CONTRIBUTE, which also mentions useful Emacs
functions which will help you format the log message according to our
conventions.

Thanks.

bug#61326: [DRAFT PATCH v4] Work around zip's filename extension limitation (was: Adding --no-add-suffix to zip patch)

[Ruijie Yu via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Eli Zaretskii <eliz@gnu.org> writes:

>> [...] I turned it into a `cl-do' construct. [...]
>
> I don't object in principle, but in this case it looks like the
> implementation based on cl-do needs much more complex code than the
> original?  If so, I'd prefer the original, simpler and
> easier-to-understand code.

It's more that everything is buried under the let expression, so it
_looks_ more complex.  But I do agree that this change might introduce
unnecessary cognative load for maintainers and I will revert that change
in my next iteration.

> The commit log message is not detailed enough: it doesn't mention the
> functions you modify.  Please see the conventions we follow for log
> messages described in CONTRIBUTE, which also mentions useful Emacs
> functions which will help you format the log message according to our
> conventions.
>
> Thanks.

Thank you for the review.  I will take a closer look at etc/CONTRIBUTE
-- apparently I didn't read it in enough detail.  I will report back
within the next few days.

--
Best,


RY

bug#61326: [DRAFT PATCH v5] Work around zip's filename extension limitation (was: Adding --no-add-suffix to zip patch)

[Ruijie Yu via Bug reports for GNU Emacs, the Swiss army knife of text editors]

[-- Attachment #1: Type: text/plain, Size: 905 bytes --]


> [...]
> Thank you for the review.  I will take a closer look at etc/CONTRIBUTE
> -- apparently I didn't read it in enough detail.  I will report back
> within the next few days.

New iteration.  As mentioned from my last message, I have reverted the
part where I tried to rewrite part of `archive-expunge' using `cl-do'.

Test cases for arc-mode still pass:
    $ make -C test arc-mode-tests

Note that I mentioned the new internal helper functions in the commit
message, as I saw João too has mentioned updates to internal functions
in 9d3fdf7e0.

If mentioning the internal functions turns out to be unnecessary, simply
removing these lines from the patch should suffice.  Otherwise, if
there's something else that looks wrong in the commit message, please
let me know.

FTR, I am still waiting for the counter signature from FSF before this
can be included into emacs.git.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Handle-modifications-on-extensionless-zips-correctly.patch --]
[-- Type: text/x-patch, Size: 9155 bytes --]

From f3b29596ef3ddfa63d6cd65fcbf8a5210e99989f Mon Sep 17 00:00:00 2001
From: Ruijie Yu <ruijie+git@netyu.xyz>
Date: Mon, 6 Mar 2023 11:03:32 +0800
Subject: [PATCH] Handle modifications on extensionless zips correctly
 (Bug#61326)

* lisp/arc-mode.el
(archive-*-write-file-member) (archive-*-expunge):
Refactor to correctly modify extensionless zip archives.
(archive-expunge):
Move implementation to a separate helper function to facilitate testing.
(archive--act-files): New helper function to wrap around `call-process' calls.
(archive--need-rename-p):
New helper function to check whether a temporary rename is necessary.
(archive--ensure-extension) (archive--maybe-rename):
New helper functions to rename archive if the caller deems it necessary.
(archive--with-ensure-extension):
New helper function to handle writing an archive while ensuring extensionless
archives work correctly by temporarily renaming them.

* test/lisp/arc-mode-tests.el
(arc-mode-test-zip-ensure-ext): New regression test for bug#61326.
---
 lisp/arc-mode.el            | 76 +++++++++++++++++++++++++++----------
 test/lisp/arc-mode-tests.el | 67 ++++++++++++++++++++++++++++++++
 2 files changed, 123 insertions(+), 20 deletions(-)

diff --git a/lisp/arc-mode.el b/lisp/arc-mode.el
index 5e696c091b2..0a971799746 100644
--- a/lisp/arc-mode.el
+++ b/lisp/arc-mode.el
@@ -645,6 +645,49 @@ archive-get-descr
       (if (not noerror)
           (error "Line does not describe a member of the archive")))))
 ;; -------------------------------------------------------------------------
+;;; Section: Helper functions for requiring filename extensions
+
+(defun archive--act-files (command files)
+  (lambda (archive)
+    (apply #'call-process (car command)
+           nil nil nil (append (cdr command) (cons archive files)))))
+
+(defun archive--need-rename-p (&optional archive)
+  (let ((archive
+         (file-name-nondirectory (or archive buffer-file-name))))
+    (cl-case archive-subtype
+      ((zip) (not (seq-contains-p archive ?. #'eq))))))
+
+(defun archive--ensure-extension (archive ensure-extension)
+  (if ensure-extension
+      (make-temp-name (expand-file-name (concat archive "_tmp.")))
+    archive))
+
+(defun archive--maybe-rename (newname need-rename-p)
+  ;; Operating with archive as current buffer, and protect
+  ;; `default-directory' from being modified in `rename-visited-file'.
+  (when need-rename-p
+    (let ((default-directory default-directory))
+      (rename-visited-file newname))))
+
+(defun archive--with-ensure-extension (archive proc-fn)
+  (let ((saved default-directory))
+    (with-current-buffer (find-buffer-visiting archive)
+      (let ((ensure-extension (archive--need-rename-p))
+            (default-directory saved))
+        (unwind-protect
+            ;; Some archive programs (like zip) expect filenames to
+            ;; have an extension, so if necessary, temporarily rename
+            ;; an extensionless file for write accesses.
+            (let ((archive (archive--ensure-extension
+                            archive ensure-extension)))
+              (archive--maybe-rename archive ensure-extension)
+              (let ((exitcode (funcall proc-fn archive)))
+                (or (zerop exitcode)
+                    (error "Updating was unsuccessful (%S)" exitcode))))
+          (progn (archive--maybe-rename archive ensure-extension)
+                 (revert-buffer nil t)))))))
+;; -------------------------------------------------------------------------
 ;;; Section: the mode definition

 ;;;###autoload
@@ -1378,16 +1421,9 @@ archive-*-write-file-member
      (setq ename
        (encode-coding-string ename archive-file-name-coding-system))
           (let* ((coding-system-for-write 'no-conversion)
-        (default-directory (file-name-as-directory archive-tmpdir))
-        (exitcode (apply #'call-process
-                 (car command)
-                 nil
-                 nil
-                 nil
-                 (append (cdr command)
-                     (list archive ename)))))
-            (or (zerop exitcode)
-       (error "Updating was unsuccessful (%S)" exitcode))))
+                 (default-directory (file-name-as-directory archive-tmpdir)))
+            (archive--with-ensure-extension
+             archive (archive--act-files command (list ename)))))
       (archive-delete-local tmpfile))))

 (defun archive-write-file (&optional file)
@@ -1510,9 +1546,7 @@ archive-chgrp-entry
      (archive-resummarize))
       (error "Setting group is not supported for this archive type"))))

-(defun archive-expunge ()
-  "Do the flagged deletions."
-  (interactive)
+(defun archive--expunge-maybe-force (force)
   (let (files)
     (save-excursion
       (goto-char archive-file-list-start)
@@ -1526,7 +1560,8 @@ archive-expunge
     (and files
     (or (not archive-read-only)
         (error "Archive is read-only"))
-    (or (yes-or-no-p (format "Really delete %d member%s? "
+         (or force
+             (yes-or-no-p (format "Really delete %d member%s? "
                  (length files)
                  (if (null (cdr files)) "" "s")))
         (error "Operation aborted"))
@@ -1540,13 +1575,14 @@ archive-expunge
           (archive-resummarize)
         (revert-buffer))))))

+(defun archive-expunge ()
+  "Do the flagged deletions."
+  (interactive)
+  (archive--expunge-maybe-force nil))
+
 (defun archive-*-expunge (archive files command)
-  (apply #'call-process
-    (car command)
-    nil
-    nil
-    nil
-    (append (cdr command) (cons archive files))))
+  (archive--with-ensure-extension
+   archive (archive--act-files command files)))

 (defun archive-rename-entry (newname)
   "Change the name associated with this entry in the archive file."
diff --git a/test/lisp/arc-mode-tests.el b/test/lisp/arc-mode-tests.el
index 32bce1b71bd..ae44ef3439c 100644
--- a/test/lisp/arc-mode-tests.el
+++ b/test/lisp/arc-mode-tests.el
@@ -46,6 +46,73 @@ arc-mode-test-zip-extract-gz
       (when (buffer-live-p zip-buffer) (kill-buffer zip-buffer))
       (when (buffer-live-p gz-buffer) (kill-buffer gz-buffer)))))

+(ert-deftest arc-mode-test-zip-ensure-ext ()
+  ;; Bug#61326
+  (skip-unless (executable-find "zip"))
+  (let* ((default-directory arc-mode-tests-data-directory)
+         (base-zip-1 "base-1.zip")
+         (base-zip-2 "base-2.zip")
+         (content-1 '("1" "2"))
+         (content-2 '("3" "4"))
+         (make-file (lambda (name)
+                      (with-temp-buffer
+                        (insert name)
+                        (write-file name))))
+         (make-zip
+          (lambda (zip files)
+            (delete-file zip nil)
+            (funcall (archive--act-files '("zip") files) zip)))
+         (update-fn
+          (lambda (zip-nonempty)
+            (with-current-buffer (find-file-noselect zip-nonempty)
+              (save-excursion
+                (goto-char archive-file-list-start)
+                (save-current-buffer
+                  (archive-extract)
+                  (save-excursion
+                    (goto-char (point-max))
+                    (insert ?a)
+                    (save-buffer))
+                  (kill-buffer (current-buffer)))
+                (archive-extract)
+                ;; [2] must be ?a; [3] must be (eobp)
+                (should (eq (char-after 2) ?a))
+                (should (eq (point-max) 3))))))
+         (delete-fn
+          (lambda (zip-nonempty)
+            (with-current-buffer (find-file-noselect zip-nonempty)
+              ;; mark delete and expunge first entry
+              (save-excursion
+                (goto-char archive-file-list-start)
+                (should (length= archive-files 2))
+                (archive-flag-deleted 1)
+                (archive--expunge-maybe-force t)
+                (should (length= archive-files 1))))))
+         (test-modify
+          (lambda (zip mod-fn)
+            (let ((zip-base (concat zip ".zip"))
+                  (tag (gensym)))
+              (copy-file base-zip-1 zip t)
+              (copy-file base-zip-2 zip-base t)
+              (file-has-changed-p zip tag)
+              (file-has-changed-p zip-base tag)
+              (funcall mod-fn zip)
+              (should-not (file-has-changed-p zip-base tag))
+              (should (file-has-changed-p zip tag))))))
+    ;; setup: make two zip files with different contents
+    (mapc make-file (append content-1 content-2))
+    (mapc (lambda (args) (apply make-zip args))
+          (list (list base-zip-1 content-1)
+                (list base-zip-2 content-2)))
+    ;; test 1: with "test-update" and "test-update.zip", update
+    ;; "test-update": (1) ensure only "test-update" is modified, (2)
+    ;; ensure the contents of the new member is expected.
+    (funcall test-modify "test-update" update-fn)
+    ;; test 2: with "test-delete" and "test-delete.zip", delete entry
+    ;; from "test-delete": (1) ensure only "test-delete" is modified,
+    ;; (2) ensure the file list is reduced as expected.
+    (funcall test-modify "test-delete" delete-fn)))
+
 (provide 'arc-mode-tests)

 ;;; arc-mode-tests.el ends here
--
2.39.2


[-- Attachment #3: Type: text/plain, Size: 15 bytes --]


--
Best,


RY

bug#61326: [PATCH v5] Work around zip's filename extension limitation (was: Adding --no-add-suffix to zip patch)

[Ruijie Yu via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Ruijie Yu <ruijie@netyu.xyz> writes:

> New iteration.  As mentioned from my last message, I have reverted the
> part where I tried to rewrite part of `archive-expunge' using `cl-do'.
>
> FTR, I am still waiting for the counter signature from FSF before this
> can be included into emacs.git.
>
> [2. text/x-patch; 0001-Handle-modifications-on-extensionless-zips-correctly.patch]...

Ping?

Removing the "DRAFT" from subject, because I think it is probably
complete.  Also, my FSF process should now be complete.

-- 
Best,


RY

bug#61326: [PATCH v5] Work around zip's filename extension limitation (was: Adding --no-add-suffix to zip patch)

[Eli Zaretskii]

> From: Ruijie Yu <ruijie@netyu.xyz>
> Cc: Eli Zaretskii <eliz@gnu.org>, asjo@koldfront.dk, 61326@debbugs.gnu.org
> Date: Mon, 17 Apr 2023 16:48:13 +0800
> 
> 
> Ruijie Yu <ruijie@netyu.xyz> writes:
> 
> > New iteration.  As mentioned from my last message, I have reverted the
> > part where I tried to rewrite part of `archive-expunge' using `cl-do'.
> >
> > FTR, I am still waiting for the counter signature from FSF before this
> > can be included into emacs.git.
> >
> 
> Ping?
> 
> Removing the "DRAFT" from subject, because I think it is probably
> complete.  Also, my FSF process should now be complete.

Great, then, with these two issues out of our way, we can install this
now.

However, the patch as posted back then no longer applies.  Please
rebase on the current master branch and re-post.

Thanks.

bug#61326: [PATCH v5] Work around zip's filename extension limitation (was: Adding --no-add-suffix to zip patch)

[Ruijie Yu via Bug reports for GNU Emacs, the Swiss army knife of text editors]

[-- Attachment #1: Type: text/plain, Size: 1149 bytes --]


Eli Zaretskii <eliz@gnu.org> writes:

>> From: Ruijie Yu <ruijie@netyu.xyz>
>> Cc: Eli Zaretskii <eliz@gnu.org>, asjo@koldfront.dk, 61326@debbugs.gnu.org
>> Date: Mon, 17 Apr 2023 16:48:13 +0800
>> 
>> 
>> Ruijie Yu <ruijie@netyu.xyz> writes:
>> 
>> > New iteration.  As mentioned from my last message, I have reverted the
>> > part where I tried to rewrite part of `archive-expunge' using `cl-do'.
>> >
>> > FTR, I am still waiting for the counter signature from FSF before this
>> > can be included into emacs.git.
>> >
>> 
>> Ping?
>> 
>> Removing the "DRAFT" from subject, because I think it is probably
>> complete.  Also, my FSF process should now be complete.
>
> Great, then, with these two issues out of our way, we can install this
> now.
>
> However, the patch as posted back then no longer applies.  Please
> rebase on the current master branch and re-post.
>
> Thanks.

Thanks.  I have made one change after the previous iteration: I turned
the "bug#xxx" comment in the test/lisp/arc-mode-tests.el into a
docstring.  Also, interestingly, git-rebase finishes cleanly on my end.

Here attached is a patch _without_ indentation changes.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: Patch *without* indentation changes --]
[-- Type: text/x-patch, Size: 9046 bytes --]

From a98a530d7a7e73404c107e77c173d8981db811ce Mon Sep 17 00:00:00 2001
From: Ruijie Yu <ruijie+git@netyu.xyz>
Date: Mon, 6 Mar 2023 11:03:32 +0800
Subject: [PATCH] Handle modifications on extensionless zips correctly
 (Bug#61326)

* lisp/arc-mode.el
(archive-*-write-file-member) (archive-*-expunge):
Refactor to correctly modify extensionless zip archives.
(archive-expunge):
Move implementation to a separate helper function to facilitate testing.
(archive--act-files): New helper function to wrap around `call-process' calls.
(archive--need-rename-p):
New helper function to check whether a temporary rename is necessary.
(archive--ensure-extension) (archive--maybe-rename):
New helper functions to rename archive if the caller deems it necessary.
(archive--with-ensure-extension):
New helper function to handle writing an archive while ensuring extensionless
archives work correctly by temporarily renaming them.

* test/lisp/arc-mode-tests.el
(arc-mode-test-zip-ensure-ext): New regression test for bug#61326.
---
 lisp/arc-mode.el            | 76 +++++++++++++++++++++++++++----------
 test/lisp/arc-mode-tests.el | 67 ++++++++++++++++++++++++++++++++
 2 files changed, 123 insertions(+), 20 deletions(-)

diff --git a/lisp/arc-mode.el b/lisp/arc-mode.el
index 5e696c091b2..0a971799746 100644
--- a/lisp/arc-mode.el
+++ b/lisp/arc-mode.el
@@ -645,6 +645,49 @@ archive-get-descr
       (if (not noerror)
           (error "Line does not describe a member of the archive")))))
 ;; -------------------------------------------------------------------------
+;;; Section: Helper functions for requiring filename extensions
+
+(defun archive--act-files (command files)
+  (lambda (archive)
+    (apply #'call-process (car command)
+           nil nil nil (append (cdr command) (cons archive files)))))
+
+(defun archive--need-rename-p (&optional archive)
+  (let ((archive
+         (file-name-nondirectory (or archive buffer-file-name))))
+    (cl-case archive-subtype
+      ((zip) (not (seq-contains-p archive ?. #'eq))))))
+
+(defun archive--ensure-extension (archive ensure-extension)
+  (if ensure-extension
+      (make-temp-name (expand-file-name (concat archive "_tmp.")))
+    archive))
+
+(defun archive--maybe-rename (newname need-rename-p)
+  ;; Operating with archive as current buffer, and protect
+  ;; `default-directory' from being modified in `rename-visited-file'.
+  (when need-rename-p
+    (let ((default-directory default-directory))
+      (rename-visited-file newname))))
+
+(defun archive--with-ensure-extension (archive proc-fn)
+  (let ((saved default-directory))
+    (with-current-buffer (find-buffer-visiting archive)
+      (let ((ensure-extension (archive--need-rename-p))
+            (default-directory saved))
+        (unwind-protect
+            ;; Some archive programs (like zip) expect filenames to
+            ;; have an extension, so if necessary, temporarily rename
+            ;; an extensionless file for write accesses.
+            (let ((archive (archive--ensure-extension
+                            archive ensure-extension)))
+              (archive--maybe-rename archive ensure-extension)
+              (let ((exitcode (funcall proc-fn archive)))
+                (or (zerop exitcode)
+                    (error "Updating was unsuccessful (%S)" exitcode))))
+          (progn (archive--maybe-rename archive ensure-extension)
+                 (revert-buffer nil t)))))))
+;; -------------------------------------------------------------------------
 ;;; Section: the mode definition
 
 ;;;###autoload
@@ -1378,16 +1421,9 @@ archive-*-write-file-member
 	  (setq ename
 		(encode-coding-string ename archive-file-name-coding-system))
           (let* ((coding-system-for-write 'no-conversion)
-		 (default-directory (file-name-as-directory archive-tmpdir))
-		 (exitcode (apply #'call-process
-				  (car command)
-				  nil
-				  nil
-				  nil
-				  (append (cdr command)
-					  (list archive ename)))))
-            (or (zerop exitcode)
-		(error "Updating was unsuccessful (%S)" exitcode))))
+                 (default-directory (file-name-as-directory archive-tmpdir)))
+            (archive--with-ensure-extension
+             archive (archive--act-files command (list ename)))))
       (archive-delete-local tmpfile))))
 
 (defun archive-write-file (&optional file)
@@ -1510,9 +1546,7 @@ archive-chgrp-entry
 	  (archive-resummarize))
       (error "Setting group is not supported for this archive type"))))
 
-(defun archive-expunge ()
-  "Do the flagged deletions."
-  (interactive)
+(defun archive--expunge-maybe-force (force)
   (let (files)
     (save-excursion
       (goto-char archive-file-list-start)
@@ -1526,7 +1560,8 @@ archive-expunge
     (and files
 	 (or (not archive-read-only)
 	     (error "Archive is read-only"))
-	 (or (yes-or-no-p (format "Really delete %d member%s? "
+         (or force
+             (yes-or-no-p (format "Really delete %d member%s? "
 				  (length files)
 				  (if (null (cdr files)) "" "s")))
 	     (error "Operation aborted"))
@@ -1540,13 +1575,14 @@ archive-expunge
 	       (archive-resummarize)
 	     (revert-buffer))))))
 
+(defun archive-expunge ()
+  "Do the flagged deletions."
+  (interactive)
+  (archive--expunge-maybe-force nil))
+
 (defun archive-*-expunge (archive files command)
-  (apply #'call-process
-	 (car command)
-	 nil
-	 nil
-	 nil
-	 (append (cdr command) (cons archive files))))
+  (archive--with-ensure-extension
+   archive (archive--act-files command files)))
 
 (defun archive-rename-entry (newname)
   "Change the name associated with this entry in the archive file."
diff --git a/test/lisp/arc-mode-tests.el b/test/lisp/arc-mode-tests.el
index 32bce1b71bd..b6e06a563fe 100644
--- a/test/lisp/arc-mode-tests.el
+++ b/test/lisp/arc-mode-tests.el
@@ -46,6 +46,73 @@ arc-mode-test-zip-extract-gz
       (when (buffer-live-p zip-buffer) (kill-buffer zip-buffer))
       (when (buffer-live-p gz-buffer) (kill-buffer gz-buffer)))))
 
+(ert-deftest arc-mode-test-zip-ensure-ext ()
+  "Regression test for bug#61326."
+  (skip-unless (executable-find "zip"))
+  (let* ((default-directory arc-mode-tests-data-directory)
+         (base-zip-1 "base-1.zip")
+         (base-zip-2 "base-2.zip")
+         (content-1 '("1" "2"))
+         (content-2 '("3" "4"))
+         (make-file (lambda (name)
+                      (with-temp-buffer
+                        (insert name)
+                        (write-file name))))
+         (make-zip
+          (lambda (zip files)
+            (delete-file zip nil)
+            (funcall (archive--act-files '("zip") files) zip)))
+         (update-fn
+          (lambda (zip-nonempty)
+            (with-current-buffer (find-file-noselect zip-nonempty)
+              (save-excursion
+                (goto-char archive-file-list-start)
+                (save-current-buffer
+                  (archive-extract)
+                  (save-excursion
+                    (goto-char (point-max))
+                    (insert ?a)
+                    (save-buffer))
+                  (kill-buffer (current-buffer)))
+                (archive-extract)
+                ;; [2] must be ?a; [3] must be (eobp)
+                (should (eq (char-after 2) ?a))
+                (should (eq (point-max) 3))))))
+         (delete-fn
+          (lambda (zip-nonempty)
+            (with-current-buffer (find-file-noselect zip-nonempty)
+              ;; mark delete and expunge first entry
+              (save-excursion
+                (goto-char archive-file-list-start)
+                (should (length= archive-files 2))
+                (archive-flag-deleted 1)
+                (archive--expunge-maybe-force t)
+                (should (length= archive-files 1))))))
+         (test-modify
+          (lambda (zip mod-fn)
+            (let ((zip-base (concat zip ".zip"))
+                  (tag (gensym)))
+              (copy-file base-zip-1 zip t)
+              (copy-file base-zip-2 zip-base t)
+              (file-has-changed-p zip tag)
+              (file-has-changed-p zip-base tag)
+              (funcall mod-fn zip)
+              (should-not (file-has-changed-p zip-base tag))
+              (should (file-has-changed-p zip tag))))))
+    ;; setup: make two zip files with different contents
+    (mapc make-file (append content-1 content-2))
+    (mapc (lambda (args) (apply make-zip args))
+          (list (list base-zip-1 content-1)
+                (list base-zip-2 content-2)))
+    ;; test 1: with "test-update" and "test-update.zip", update
+    ;; "test-update": (1) ensure only "test-update" is modified, (2)
+    ;; ensure the contents of the new member is expected.
+    (funcall test-modify "test-update" update-fn)
+    ;; test 2: with "test-delete" and "test-delete.zip", delete entry
+    ;; from "test-delete": (1) ensure only "test-delete" is modified,
+    ;; (2) ensure the file list is reduced as expected.
+    (funcall test-modify "test-delete" delete-fn)))
+
 (provide 'arc-mode-tests)
 
 ;;; arc-mode-tests.el ends here
-- 
2.40.0


[-- Attachment #3: Type: text/plain, Size: 273 bytes --]


Here attached is a patch _with_ indentation changes (on functions which
I have touched) -- I simply re-indented these functions without tabs.
Not sure if you want this, so I provided the other patch file as an
alternative.  Please choose whichever you prefer to install.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #4: Patch *with* indentation changes --]
[-- Type: text/x-patch, Size: 12416 bytes --]

From 36c5f839042e39b3d49e7939543b988142103033 Mon Sep 17 00:00:00 2001
From: Ruijie Yu <ruijie+git@netyu.xyz>
Date: Mon, 6 Mar 2023 11:03:32 +0800
Subject: [PATCH] Handle modifications on extensionless zips correctly
 (Bug#61326)

* lisp/arc-mode.el
(archive-*-write-file-member) (archive-*-expunge):
Refactor to correctly modify extensionless zip archives.
(archive-expunge):
Move implementation to a separate helper function to facilitate testing.
(archive--act-files): New helper function to wrap around `call-process' calls.
(archive--need-rename-p):
New helper function to check whether a temporary rename is necessary.
(archive--ensure-extension) (archive--maybe-rename):
New helper functions to rename archive if the caller deems it necessary.
(archive--with-ensure-extension):
New helper function to handle writing an archive while ensuring extensionless
archives work correctly by temporarily renaming them.

* test/lisp/arc-mode-tests.el
(arc-mode-test-zip-ensure-ext): New regression test for bug#61326.
---
 lisp/arc-mode.el            | 144 ++++++++++++++++++++++--------------
 test/lisp/arc-mode-tests.el |  67 +++++++++++++++++
 2 files changed, 157 insertions(+), 54 deletions(-)

diff --git a/lisp/arc-mode.el b/lisp/arc-mode.el
index 5e696c091b2..89b3e720ed9 100644
--- a/lisp/arc-mode.el
+++ b/lisp/arc-mode.el
@@ -645,6 +645,49 @@ archive-get-descr
       (if (not noerror)
           (error "Line does not describe a member of the archive")))))
 ;; -------------------------------------------------------------------------
+;;; Section: Helper functions for requiring filename extensions
+
+(defun archive--act-files (command files)
+  (lambda (archive)
+    (apply #'call-process (car command)
+           nil nil nil (append (cdr command) (cons archive files)))))
+
+(defun archive--need-rename-p (&optional archive)
+  (let ((archive
+         (file-name-nondirectory (or archive buffer-file-name))))
+    (cl-case archive-subtype
+      ((zip) (not (seq-contains-p archive ?. #'eq))))))
+
+(defun archive--ensure-extension (archive ensure-extension)
+  (if ensure-extension
+      (make-temp-name (expand-file-name (concat archive "_tmp.")))
+    archive))
+
+(defun archive--maybe-rename (newname need-rename-p)
+  ;; Operating with archive as current buffer, and protect
+  ;; `default-directory' from being modified in `rename-visited-file'.
+  (when need-rename-p
+    (let ((default-directory default-directory))
+      (rename-visited-file newname))))
+
+(defun archive--with-ensure-extension (archive proc-fn)
+  (let ((saved default-directory))
+    (with-current-buffer (find-buffer-visiting archive)
+      (let ((ensure-extension (archive--need-rename-p))
+            (default-directory saved))
+        (unwind-protect
+            ;; Some archive programs (like zip) expect filenames to
+            ;; have an extension, so if necessary, temporarily rename
+            ;; an extensionless file for write accesses.
+            (let ((archive (archive--ensure-extension
+                            archive ensure-extension)))
+              (archive--maybe-rename archive ensure-extension)
+              (let ((exitcode (funcall proc-fn archive)))
+                (or (zerop exitcode)
+                    (error "Updating was unsuccessful (%S)" exitcode))))
+          (progn (archive--maybe-rename archive ensure-extension)
+                 (revert-buffer nil t)))))))
+;; -------------------------------------------------------------------------
 ;;; Section: the mode definition
 
 ;;;###autoload
@@ -1357,37 +1400,30 @@ archive-*-write-file-member
          (ename (archive--file-desc-ext-file-name descr))
          (tmpfile (expand-file-name ename archive-tmpdir))
          (top (directory-file-name (file-name-as-directory archive-tmpdir)))
-	 (default-directory (file-name-as-directory top)))
+         (default-directory (file-name-as-directory top)))
     (unwind-protect
         (progn
           (make-directory (file-name-directory tmpfile) t)
-	  ;; If the member is itself an archive, write it without
-	  ;; the dired-like listing we created.
-	  (if (eq major-mode 'archive-mode)
-	      (archive-write-file tmpfile)
-	    (write-region nil nil tmpfile nil 'nomessage))
-	  ;; basic-save-buffer needs last-coding-system-used to have
-	  ;; the value used to write the file, so save it before any
-	  ;; further processing clobbers it (we restore it in
-	  ;; archive-write-file-member, above).
-	  (setq archive-member-coding-system last-coding-system-used)
-	  (if (archive--file-desc-mode descr)
-	      ;; Set the file modes, but make sure we can read it.
-	      (set-file-modes tmpfile
-	                      (logior ?\400 (archive--file-desc-mode descr))))
-	  (setq ename
-		(encode-coding-string ename archive-file-name-coding-system))
+          ;; If the member is itself an archive, write it without
+          ;; the dired-like listing we created.
+          (if (eq major-mode 'archive-mode)
+              (archive-write-file tmpfile)
+            (write-region nil nil tmpfile nil 'nomessage))
+          ;; basic-save-buffer needs last-coding-system-used to have
+          ;; the value used to write the file, so save it before any
+          ;; further processing clobbers it (we restore it in
+          ;; archive-write-file-member, above).
+          (setq archive-member-coding-system last-coding-system-used)
+          (if (archive--file-desc-mode descr)
+              ;; Set the file modes, but make sure we can read it.
+              (set-file-modes tmpfile
+                              (logior ?\400 (archive--file-desc-mode descr))))
+          (setq ename
+                (encode-coding-string ename archive-file-name-coding-system))
           (let* ((coding-system-for-write 'no-conversion)
-		 (default-directory (file-name-as-directory archive-tmpdir))
-		 (exitcode (apply #'call-process
-				  (car command)
-				  nil
-				  nil
-				  nil
-				  (append (cdr command)
-					  (list archive ename)))))
-            (or (zerop exitcode)
-		(error "Updating was unsuccessful (%S)" exitcode))))
+                 (default-directory (file-name-as-directory archive-tmpdir)))
+            (archive--with-ensure-extension
+             archive (archive--act-files command (list ename)))))
       (archive-delete-local tmpfile))))
 
 (defun archive-write-file (&optional file)
@@ -1510,43 +1546,43 @@ archive-chgrp-entry
 	  (archive-resummarize))
       (error "Setting group is not supported for this archive type"))))
 
-(defun archive-expunge ()
-  "Do the flagged deletions."
-  (interactive)
+(defun archive--expunge-maybe-force (force)
   (let (files)
     (save-excursion
       (goto-char archive-file-list-start)
       (while (< (point) archive-file-list-end)
         (if (= (following-char) ?D)
-	    (setq files (cons (archive--file-desc-ext-file-name
-	                       (archive-get-descr))
-	                      files)))
+            (setq files (cons (archive--file-desc-ext-file-name
+                               (archive-get-descr))
+                              files)))
         (forward-line 1)))
     (setq files (nreverse files))
     (and files
-	 (or (not archive-read-only)
-	     (error "Archive is read-only"))
-	 (or (yes-or-no-p (format "Really delete %d member%s? "
-				  (length files)
-				  (if (null (cdr files)) "" "s")))
-	     (error "Operation aborted"))
-	 (let ((archive (archive-maybe-copy (buffer-file-name)))
-	       (expunger (archive-name "expunge")))
-	   (if (fboundp expunger)
-	       (funcall expunger archive files)
-	     (archive-*-expunge archive files (symbol-value expunger)))
-	   (archive-maybe-update nil)
-	   (if archive-remote
-	       (archive-resummarize)
-	     (revert-buffer))))))
+         (or (not archive-read-only)
+             (error "Archive is read-only"))
+         (or force
+             (yes-or-no-p (format "Really delete %d member%s? "
+                                  (length files)
+                                  (if (null (cdr files)) "" "s")))
+             (error "Operation aborted"))
+         (let ((archive (archive-maybe-copy (buffer-file-name)))
+               (expunger (archive-name "expunge")))
+           (if (fboundp expunger)
+               (funcall expunger archive files)
+             (archive-*-expunge archive files (symbol-value expunger)))
+           (archive-maybe-update nil)
+           (if archive-remote
+               (archive-resummarize)
+             (revert-buffer))))))
+
+(defun archive-expunge ()
+  "Do the flagged deletions."
+  (interactive)
+  (archive--expunge-maybe-force nil))
 
 (defun archive-*-expunge (archive files command)
-  (apply #'call-process
-	 (car command)
-	 nil
-	 nil
-	 nil
-	 (append (cdr command) (cons archive files))))
+  (archive--with-ensure-extension
+   archive (archive--act-files command files)))
 
 (defun archive-rename-entry (newname)
   "Change the name associated with this entry in the archive file."
diff --git a/test/lisp/arc-mode-tests.el b/test/lisp/arc-mode-tests.el
index 32bce1b71bd..b6e06a563fe 100644
--- a/test/lisp/arc-mode-tests.el
+++ b/test/lisp/arc-mode-tests.el
@@ -46,6 +46,73 @@ arc-mode-test-zip-extract-gz
       (when (buffer-live-p zip-buffer) (kill-buffer zip-buffer))
       (when (buffer-live-p gz-buffer) (kill-buffer gz-buffer)))))
 
+(ert-deftest arc-mode-test-zip-ensure-ext ()
+  "Regression test for bug#61326."
+  (skip-unless (executable-find "zip"))
+  (let* ((default-directory arc-mode-tests-data-directory)
+         (base-zip-1 "base-1.zip")
+         (base-zip-2 "base-2.zip")
+         (content-1 '("1" "2"))
+         (content-2 '("3" "4"))
+         (make-file (lambda (name)
+                      (with-temp-buffer
+                        (insert name)
+                        (write-file name))))
+         (make-zip
+          (lambda (zip files)
+            (delete-file zip nil)
+            (funcall (archive--act-files '("zip") files) zip)))
+         (update-fn
+          (lambda (zip-nonempty)
+            (with-current-buffer (find-file-noselect zip-nonempty)
+              (save-excursion
+                (goto-char archive-file-list-start)
+                (save-current-buffer
+                  (archive-extract)
+                  (save-excursion
+                    (goto-char (point-max))
+                    (insert ?a)
+                    (save-buffer))
+                  (kill-buffer (current-buffer)))
+                (archive-extract)
+                ;; [2] must be ?a; [3] must be (eobp)
+                (should (eq (char-after 2) ?a))
+                (should (eq (point-max) 3))))))
+         (delete-fn
+          (lambda (zip-nonempty)
+            (with-current-buffer (find-file-noselect zip-nonempty)
+              ;; mark delete and expunge first entry
+              (save-excursion
+                (goto-char archive-file-list-start)
+                (should (length= archive-files 2))
+                (archive-flag-deleted 1)
+                (archive--expunge-maybe-force t)
+                (should (length= archive-files 1))))))
+         (test-modify
+          (lambda (zip mod-fn)
+            (let ((zip-base (concat zip ".zip"))
+                  (tag (gensym)))
+              (copy-file base-zip-1 zip t)
+              (copy-file base-zip-2 zip-base t)
+              (file-has-changed-p zip tag)
+              (file-has-changed-p zip-base tag)
+              (funcall mod-fn zip)
+              (should-not (file-has-changed-p zip-base tag))
+              (should (file-has-changed-p zip tag))))))
+    ;; setup: make two zip files with different contents
+    (mapc make-file (append content-1 content-2))
+    (mapc (lambda (args) (apply make-zip args))
+          (list (list base-zip-1 content-1)
+                (list base-zip-2 content-2)))
+    ;; test 1: with "test-update" and "test-update.zip", update
+    ;; "test-update": (1) ensure only "test-update" is modified, (2)
+    ;; ensure the contents of the new member is expected.
+    (funcall test-modify "test-update" update-fn)
+    ;; test 2: with "test-delete" and "test-delete.zip", delete entry
+    ;; from "test-delete": (1) ensure only "test-delete" is modified,
+    ;; (2) ensure the file list is reduced as expected.
+    (funcall test-modify "test-delete" delete-fn)))
+
 (provide 'arc-mode-tests)
 
 ;;; arc-mode-tests.el ends here
-- 
2.40.0


[-- Attachment #5: Type: text/plain, Size: 128 bytes --]


-- 
Best,


RY

[Please note that this mail might go to spam due to some
misconfiguration in my mail server -- will fix soon.]

bug#61326: [PATCH v5] Work around zip's filename extension limitation (was: Adding --no-add-suffix to zip patch)

[Eli Zaretskii]

> From: Ruijie Yu <ruijie@netyu.xyz>
> Cc: asjo@koldfront.dk, 61326@debbugs.gnu.org, bug-gnu-emacs@gnu.org
> Date: Thu, 20 Apr 2023 16:49:29 +0800
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > However, the patch as posted back then no longer applies.  Please
> > rebase on the current master branch and re-post.
> >
> > Thanks.
> 
> Thanks.  I have made one change after the previous iteration: I turned
> the "bug#xxx" comment in the test/lisp/arc-mode-tests.el into a
> docstring.  Also, interestingly, git-rebase finishes cleanly on my end.
> 
> Here attached is a patch _without_ indentation changes.

Thanks, the first patch applied cleanly, so I've now pushed this to
the master branch, and I'm closing this bug.

bug#61326: Adding --no-add-suffix to zip patch

[Adam Sjøgren via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Ruijie writes:

> Maybe, at least in the meantime, we change it such that all write
> operations for zip create files in temp, and move to / overwrite the
> original file when done? Although I don’t have a full understanding on
> how that would be done and whether there are problems along with it.

That sounds like a good solution, for archives with no '.' in the filename.

I guess the main problem is if the archive is huge, updating an existing
archive might (?) be more efficient than creating an entirely new one.

Another way to go about it could be to temporarily rename the archive to
have a name with a '.' in it, make the change, and then rename it back;
conceptually:

  mv a a.zip
  # update a.zip rather than a
  mv a.zip a

That has its own set of possible problems.

Maybe you could make a link to the original archive, where the link has
a '.' in the name, have zip update the file via the link and then remove
the link again; conceptually:

  ln a a.zip
  # update a.zip rather than a
  rm a.zip

I think I like this the most, but again...

bug#61326: Adding --no-add-suffix to zip patch

[Ruijie Yu via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Adam Sjøgren via "Bug reports for GNU Emacs, the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org> writes:

> Ruijie writes:
>
>> Maybe, at least in the meantime, we change it such that all write
>> operations for zip create files in temp, and move to / overwrite the
>> original file when done? Although I don’t have a full understanding on
>> how that would be done and whether there are problems along with it.
>
> That sounds like a good solution, for archives with no '.' in the filename.
>
> I guess the main problem is if the archive is huge, updating an existing
> archive might (?) be more efficient than creating an entirely new one.
>
> Another way to go about it could be to temporarily rename the archive to
> have a name with a '.' in it, make the change, and then rename it back;
> conceptually:
>
>   mv a a.zip
>   # update a.zip rather than a
>   mv a.zip a
>
> That has its own set of possible problems.
>
> Maybe you could make a link to the original archive, where the link has
> a '.' in the name, have zip update the file via the link and then remove
> the link again; conceptually:
>
>   ln a a.zip
>   # update a.zip rather than a
>   rm a.zip
>
> I think I like this the most, but again...

I like the link solution, but I don't think we can reliably "link"
without knowing that we actually can do so -- i.e., on MS Windows, IIUC,
one needs admin privilege to create symlinks on cmd unless something in
the registry is changed.  Also, on old MS Windows versions like XP and
earier, I believe it is impossible to create symlinks at all. (Do we
still support them?)

Come to think of it, I think your mv idea might be better and even
easier to implement than my zip-offsite-then-overwrite idea.  I might
lean towards that method if people have no bias when I work on the patch
in a couple hours.

Best,


RY

bug#61326: Adding --no-add-suffix to zip patch

[Eli Zaretskii]

> Cc: 61326@debbugs.gnu.org
> Date: Wed, 08 Feb 2023 09:21:11 +0800
> From:  Ruijie Yu via "Bug reports for GNU Emacs,
>  the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>
> 
> Also, on old MS Windows versions like XP and earier, I believe it is
> impossible to create symlinks at all. (Do we still support them?)

Yes, we do.