"sed -i" not renaming/moving temp-file with "q" command

classic Classic list List threaded Threaded
10 messages Options
Reply | Threaded
Open this post in threaded view
|

"sed -i" not renaming/moving temp-file with "q" command

Tim Chase-5
>Synopsis: "sed -i" not renaming/moving temp-file
>Category: user
>Environment:
        System      : OpenBSD 6.3
        Details     : OpenBSD 6.3 (GENERIC) #1: Sat Apr 21 13:57:54
CEST 2018
[hidden email]:/usr/src/sys/arch/i386/compile/GENERIC

        Architecture: OpenBSD.i386
        Machine     : i386
>Description:

"sed -i.bak '/2/d;q' file.txt" leaves temp file instead of renaming.

>How-To-Repeat:

 $ mkdir sedbug
 $ cd sedbug
 $ jot 5 > file.txt
 $ sed -e '/2/d;q' file.txt # expected behavior: 1 line
 1
 $ sed -i.bak -e '/2/d;q' file.txt # edit it in-place
 $ ls
 file.txt sedpK7uniHbMN
 $ cat file.txt # still has original contents
 1
 2
 3
 4
 5
 $ cat sedpK7uniHbMN
 1

Expected results:

 the resulting temp-file (which contains the correct data) should be
moved back atop the original source file(s).

>Fix:

 unknown

dmesg:
OpenBSD 6.3 (GENERIC) #1: Sat Apr 21 13:57:54 CEST 2018
    [hidden email]:/usr/src/sys/arch/i386/compile/GENERIC
cpu0: Intel Pentium III ("GenuineIntel" 686-class, 128KB L2 cache)
798 MHz cpu0:
FPU,V86,DE,PSE,TSC,MSR,PAE,MCE,CX8,SEP,MTRR,PGE,MCA,CMOV,PSE36,MMX,FXSR,SSE,PERF
real mem  = 326582272 (311MB) avail mem = 306700288 (292MB)
mpath0 at root
scsibus0 at mpath0: 256 targets
mainbus0 at root
bios0 at mainbus0: date 02/06/02, BIOS32 rev. 0 @ 0xe8a10, SMBIOS
rev. 2.3 @ 0xeb8b0 (47 entries) bios0: vendor Insyde Software version
"Version 27.02" date 02/04/2002 bios0: Gateway, Inc. Solo 1200
acpi0 at bios0: rev 0
acpi0: sleep states S0 S1 S3 S4 S5
acpi0: tables DSDT FACP BOOT DBGP
acpi0: wakeup devices LID_(S4) BAT0(S4) PCI0(S3) MODM(S3) CARD(S4)
USB0(S1) USB1(S1) LAN0(S4) acpitimer0 at acpi0: 3579545 Hz, 24 bits
acpiprt0 at acpi0: bus 0 (PCI0)
acpiec0 at acpi0
acpipwrres0 at acpi0: PFAN, resource for FAN0
acpitz0 at acpi0: critical temperature is 89 degC
acpibtn0 at acpi0: LID_
acpibtn1 at acpi0: SLPB
acpiac0 at acpi0: AC unit online
acpibat0 at acpi0: BAT0 model "QT07" serial SANYOQT07 type LiON oem
"SANYO" "PNP0C0B" at acpi0 not configured
acpivideo0 at acpi0: VGA0
bios0: ROM list: 0xc0000/0xc000 0xe0000/0x1800 0xe5000/0x1000!
0xea000/0x5000! cpu0 at mainbus0: (uniprocessor)
mtrr: Pentium Pro MTRR support, 8 var ranges, 88 fixed ranges
pci0 at mainbus0 bus 0: configuration mode 1 (bios)
0:10:0: mem address conflict 0xc000000/0x1000
pchb0 at pci0 dev 0 function 0 "VIA VT8605 PCI" rev 0x00
viaagp0 at pchb0: v2
agp0 at viaagp0: aperture at 0xa0000000, size 0x10000000
ppb0 at pci0 dev 1 function 0 "VIA VT8605 AGP" rev 0x00
pci1 at ppb0 bus 1
1:0:0: mem address conflict 0xc0000/0x10000
vga1 at pci1 dev 0 function 0 "S3 Twister" rev 0x02
wsdisplay0 at vga1 mux 1: console (80x25, vt100 emulation)
wsdisplay0: screen 1-5 added (80x25, vt100 emulation)
cbb0 at pci0 dev 10 function 0 "O2 Micro OZ69[17]2 CardBus" rev 0x00:
irq 10 cbb1 at pci0 dev 11 function 0 "TI PCI1410 CardBus" rev 0x01:
irq 9 pcib0 at pci0 dev 17 function 0 "VIA VT8231 ISA" rev 0x10
pciide0 at pci0 dev 17 function 1 "VIA VT82C571 IDE" rev 0x06:
ATA100, channel 0 configured to compatibility, channel 1 configured
to compatibility wd0 at pciide0 channel 0 drive 0: <FUJITSU
MHU2100AT> wd0: 16-sector PIO, LBA, 95396MB, 195371568 sectors
MHU2100AT> atapiscsi0 at pciide0 channel 0 drive 1
scsibus1 at atapiscsi0: 2 targets
cd0 at scsibus1 targ 0 lun 0: <QSI, CD-ROM SCR-242, CHC7> ATAPI
5/cdrom removable wd0(pciide0:0:0): using PIO mode 4, Ultra-DMA mode 2
cd0(pciide0:0:1): using PIO mode 4, Ultra-DMA mode 2
pciide0: channel 1 ignored (disabled)
uhci0 at pci0 dev 17 function 2 "VIA VT83C572 USB" rev 0x19: irq 9
uhci1 at pci0 dev 17 function 3 "VIA VT83C572 USB" rev 0x19: irq 9
viapm0 at pci0 dev 17 function 4 "VIA VT8231 PMG" rev 0x10: SMI
iic0 at viapm0
spdmem0 at iic0 addr 0x50: 256MB SDRAM non-parity PC133CL2
viapm0: HWM disabled
auvia0 at pci0 dev 17 function 5 "VIA VT82C686 AC97" rev 0x40: irq 10
ac97: codec id 0x43585429 (Conexant CX20468 rev 1)
ac97: codec features reserved, headphone, 18 bit DAC, 18 bit ADC, No
3D Stereo audio0 at auvia0
"VIA VT82C686 Modem" rev 0x20 at pci0 dev 17 function 6 not configured
vr0 at pci0 dev 18 function 0 "VIA RhineII-2" rev 0x51: irq 7,
address 00:e0:b8:37:e1:50 sqphy0 at vr0 phy 1: 84220 10/100 PHY, rev.
0 cardslot0 at cbb0 slot 0 flags 0
cardbus0 at cardslot0: bus 2 device 0 cacheline 0x0, lattimer 0x40
pcmcia0 at cardslot0
cardslot1 at cbb1 slot 1 flags 0
cardbus1 at cardslot1: bus 2 device 0 cacheline 0x0, lattimer 0x20
pcmcia1 at cardslot1
isa0 at pcib0
isadma0 at isa0
fdc0 at isa0 port 0x3f0/6 irq 6 drq 2
pckbc0 at isa0 port 0x60/5 irq 1 irq 12
pckbd0 at pckbc0 (kbd slot)
wskbd0 at pckbd0: console keyboard, using wsdisplay0
pms0 at pckbc0 (aux slot)
wsmouse0 at pms0 mux 0
pms0: ALPS Glidepoint, version 0x7322
pcppi0 at isa0 port 0x61
spkr0 at pcppi0
npx0 at isa0 port 0xf0/16: reported by CPUID; using exception 16
usb0 at uhci0: USB revision 1.0
uhub0 at usb0 configuration 1 interface 0 "VIA UHCI root hub" rev
1.00/1.00 addr 1 usb1 at uhci1: USB revision 1.0
uhub1 at usb1 configuration 1 interface 0 "VIA UHCI root hub" rev
1.00/1.00 addr 1 vscsi0 at root
scsibus2 at vscsi0: 256 targets
softraid0 at root
scsibus3 at softraid0: 256 targets
wi0 at pcmcia1 function 0 "Lucent Technologies, WaveLAN/IEEE, Version
01.01" port 0xa000/64 wi0: Firmware 8.10 variant 1, address
00:02:2d:69:f5:9a root on wd0a (9643cb28c07c0164.a) swap on wd0b dump
on wd0b fd0 at fdc0 drive 0: 1.44MB 80 cyl, 2 head, 18 sec

usbdevs:
Controller /dev/usb0:
addr 1: full speed, self powered, config 1, UHCI root hub(0x0000),
VIA(0x1106), rev 1.00 port 1 powered
 port 2 powered
Controller /dev/usb1:
addr 1: full speed, self powered, config 1, UHCI root hub(0x0000),
VIA(0x1106), rev 1.00 port 1 powered
 port 2 powered

Reply | Threaded
Open this post in threaded view
|

Re: "sed -i" not renaming/moving temp-file with "q" command

Loopw
On 2018-08-13 06:40, Tim Chase wrote:
>> Synopsis: "sed -i" not renaming/moving temp-file

sed -i works just fine for me.  It's really the use of ';q' that seems
to cause the behavior.

This seems like expected behavior for sed when ;q is present.


>> Description:
>
> "sed -i.bak '/2/d;q' file.txt" leaves temp file instead of renaming.
>
>> How-To-Repeat:
>
>  $ mkdir sedbug
>  $ cd sedbug
>  $ jot 5 > file.txt
>  $ sed -e '/2/d;q' file.txt # expected behavior: 1 line
>  1
>  $ sed -i.bak -e '/2/d;q' file.txt # edit it in-place
>  $ ls
>  file.txt sedpK7uniHbMN
>  $ cat file.txt # still has original contents
>  1
>  2
>  3
>  4
>  5
>  $ cat sedpK7uniHbMN
>  1
>
> Expected results:
>
>  the resulting temp-file (which contains the correct data) should be
> moved back atop the original source file(s).
>


What does "q" do?  Its not commonly used.  From SED(1):
     [1addr]q
             Branch to the end of the script and quit without starting a
new
             cycle.


This is probably the command line you actually want, no '...;q' needed -
will delete any line with "2" in it in file.txt, and will save the old
one to file.txt.bak:

sed -i.bak -e '/2/d' file.txt


By putting ";q" in the sed command string, sed is told to cut out at the
end of a loop.  In OpenBSD, this will mean your particular sed command
will exit out after it makes a transformation in a temp file.  MacOS
does something similar.  Linux cuts the temp file out mid-way, leaving a
partial result.  Its not totally surprising to me that sed is leaving
the temp file, its only half way done, so to speak.


Reply | Threaded
Open this post in threaded view
|

Re: "sed -i" not renaming/moving temp-file with "q" command

Martijn van Duren-8
This does indeed seems like a bug to me. The -i flag should produce
similar output for in place as it does without it.

$ printf '1\n2\n3\n' | tee /tmp/test > /tmp/test1
$ sed 's/2/4/;2q' /tmp/test
1
4
$ sed -i 's/2/4/;2q' /tmp/test
$ cat /tmp/test
1
2
3
$ cat /tmp/sedYwXsFr7jxg
1
4

So the output is there, but it's not stored after the quit command.

The diff below fixes this. Note that I took special care to make a
distinction between in place and normal for the 'q' command.
When running normally the files are concatenated, so we should quit
immediately. When running in in place mode I reckon the script
should be treated as if it were to run for every file individually,
since the line numbers are also reset in between files.

The following diff fixes that.

Note that this differs from what gnu sed does, since they decide
to not apply the script on the second file, but do apply it on the
first file:
$ gsed -i 's/2/4/;2q' /tmp/test /tmp/test1
$ cat /tmp/test
1
4
$ cat /tmp/test1
1
2
3

OK?

martijn@

Index: extern.h
===================================================================
RCS file: /cvs/src/usr.bin/sed/extern.h,v
retrieving revision 1.13
diff -u -p -r1.13 extern.h
--- extern.h 1 Aug 2017 18:05:53 -0000 1.13
+++ extern.h 14 Aug 2018 08:40:57 -0000
@@ -42,7 +42,9 @@ extern u_long linenum;
 extern size_t appendnum;
 extern int Eflag, aflag, eflag, nflag;
 extern int pledge_wpath, pledge_rpath;
+extern int nextfile;
 extern const char *fname, *outfname;
+extern char *inplace;
 extern FILE *infile, *outfile;
 
 void cfclose(struct s_command *, struct s_command *);
Index: main.c
===================================================================
RCS file: /cvs/src/usr.bin/sed/main.c,v
retrieving revision 1.37
diff -u -p -r1.37 main.c
--- main.c 11 Jul 2018 06:57:18 -0000 1.37
+++ main.c 14 Aug 2018 08:40:57 -0000
@@ -98,6 +98,7 @@ static char oldfname[PATH_MAX]; /* Old f
 static char tmpfname[PATH_MAX]; /* Temporary file name (for in-place editing) */
 char *inplace; /* Inplace edit file extension */
 u_long linenum;
+int nextfile = 0;
 
 static void add_compunit(enum e_cut, char *);
 static void add_file(char *);
@@ -338,10 +339,11 @@ mf_fgets(SPACE *sp, enum e_spflag spflag
  }
 
  for (;;) {
- if (infile != NULL && (c = getc(infile)) != EOF) {
+ if (!nextfile && infile != NULL && (c = getc(infile)) != EOF) {
  (void)ungetc(c, infile);
  break;
  }
+ nextfile = 0;
  /* If we are here then either eof or no files are open yet */
  if (infile == stdin) {
  sp->len = 0;
Index: process.c
===================================================================
RCS file: /cvs/src/usr.bin/sed/process.c,v
retrieving revision 1.33
diff -u -p -r1.33 process.c
--- process.c 13 Dec 2017 16:06:34 -0000 1.33
+++ process.c 14 Aug 2018 08:40:57 -0000
@@ -193,10 +193,14 @@ redirect:
  }
  break;
  case 'q':
- if (!nflag && !pd)
- OUT();
- flush_appends();
- exit(0);
+ if (inplace == NULL) {
+ if (!nflag && !pd)
+ OUT();
+ flush_appends();
+ exit(0);
+ }
+ nextfile = 1;
+ break;
  case 'r':
  if (appendx >= appendnum) {
  appends = xreallocarray(appends,



On 08/13/18 23:50, Loopw wrote:

> On 2018-08-13 06:40, Tim Chase wrote:
>>> Synopsis:    "sed -i" not renaming/moving temp-file
>
> sed -i works just fine for me.  It's really the use of ';q' that seems to cause the behavior.
>
> This seems like expected behavior for sed when ;q is present.
>
>
>>> Description:
>>
>> "sed -i.bak '/2/d;q' file.txt" leaves temp file instead of renaming.
>>
>>> How-To-Repeat:
>>
>>  $ mkdir sedbug
>>  $ cd sedbug
>>  $ jot 5 > file.txt
>>  $ sed -e '/2/d;q' file.txt # expected behavior: 1 line
>>  1
>>  $ sed -i.bak -e '/2/d;q' file.txt # edit it in-place
>>  $ ls
>>  file.txt sedpK7uniHbMN
>>  $ cat file.txt # still has original contents
>>  1
>>  2
>>  3
>>  4
>>  5
>>  $ cat sedpK7uniHbMN
>>  1
>>
>> Expected results:
>>
>>  the resulting temp-file (which contains the correct data) should be
>> moved back atop the original source file(s).
>>
>
>
> What does "q" do?  Its not commonly used.  From SED(1):
>     [1addr]q
>             Branch to the end of the script and quit without starting a new
>             cycle.
>
>
> This is probably the command line you actually want, no '...;q' needed - will delete any line with "2" in it in file.txt, and will save the old one to file.txt.bak:
>
> sed -i.bak -e '/2/d' file.txt
>
>
> By putting ";q" in the sed command string, sed is told to cut out at the end of a loop.  In OpenBSD, this will mean your particular sed command will exit out after it makes a transformation in a temp file.  MacOS does something similar.  Linux cuts the temp file out mid-way, leaving a partial result.  Its not totally surprising to me that sed is leaving the temp file, its only half way done, so to speak.
>
>

Reply | Threaded
Open this post in threaded view
|

Re: "sed -i" not renaming/moving temp-file with "q" command

Todd C. Miller-2
On Tue, 14 Aug 2018 10:43:30 +0200, Martijn van Duren wrote:

> The diff below fixes this. Note that I took special care to make a
> distinction between in place and normal for the 'q' command.
> When running normally the files are concatenated, so we should quit
> immediately. When running in in place mode I reckon the script
> should be treated as if it were to run for every file individually,
> since the line numbers are also reset in between files.

I'm not convinced this is the right approach.  If there is a quit
command, the script should quit without further processing.

We could do something like this instead which matches what GNU sed
does more closely.  Since the -i flag is a GNU invention I think
it is worthwhile trying to behave similarly.

 - todd

Index: usr.bin/sed/extern.h
===================================================================
RCS file: /cvs/src/usr.bin/sed/extern.h,v
retrieving revision 1.13
diff -u -p -u -r1.13 extern.h
--- usr.bin/sed/extern.h 1 Aug 2017 18:05:53 -0000 1.13
+++ usr.bin/sed/extern.h 14 Aug 2018 12:01:59 -0000
@@ -53,6 +53,7 @@ __dead void error(int, const char *, ...
 void warning(const char *, ...);
 int mf_fgets(SPACE *, enum e_spflag);
 int lastline(void);
+void finish_file(void);
 void process(void);
 void resetranges(void);
 char *strregerror(int, regex_t *);
Index: usr.bin/sed/main.c
===================================================================
RCS file: /cvs/src/usr.bin/sed/main.c,v
retrieving revision 1.37
diff -u -p -u -r1.37 main.c
--- usr.bin/sed/main.c 11 Jul 2018 06:57:18 -0000 1.37
+++ usr.bin/sed/main.c 14 Aug 2018 12:01:10 -0000
@@ -310,6 +310,30 @@ again:
  return (NULL);
 }
 
+void
+finish_file(void)
+{
+ if (infile != NULL) {
+ fclose(infile);
+ if (*oldfname != '\0') {
+ if (rename(fname, oldfname) != 0) {
+ warning("rename()");
+ unlink(tmpfname);
+ exit(1);
+ }
+ *oldfname = '\0';
+ }
+ if (*tmpfname != '\0') {
+ if (outfile != NULL && outfile != stdout)
+ fclose(outfile);
+ outfile = NULL;
+ rename(tmpfname, fname);
+ *tmpfname = '\0';
+ }
+ outfname = NULL;
+ }
+}
+
 /*
  * Like fgets, but go through the list of files chaining them together.
  * Set len to the length of the line.
@@ -347,25 +371,7 @@ mf_fgets(SPACE *sp, enum e_spflag spflag
  sp->len = 0;
  return (0);
  }
- if (infile != NULL) {
- fclose(infile);
- if (*oldfname != '\0') {
- if (rename(fname, oldfname) != 0) {
- warning("rename()");
- unlink(tmpfname);
- exit(1);
- }
- *oldfname = '\0';
- }
- if (*tmpfname != '\0') {
- if (outfile != NULL && outfile != stdout)
- fclose(outfile);
- outfile = NULL;
- rename(tmpfname, fname);
- *tmpfname = '\0';
- }
- outfname = NULL;
- }
+ finish_file();
  if (firstfile == 0)
  files = files->next;
  else
Index: usr.bin/sed/process.c
===================================================================
RCS file: /cvs/src/usr.bin/sed/process.c,v
retrieving revision 1.33
diff -u -p -u -r1.33 process.c
--- usr.bin/sed/process.c 13 Dec 2017 16:06:34 -0000 1.33
+++ usr.bin/sed/process.c 14 Aug 2018 12:03:24 -0000
@@ -196,6 +196,7 @@ redirect:
  if (!nflag && !pd)
  OUT();
  flush_appends();
+ finish_file();
  exit(0);
  case 'r':
  if (appendx >= appendnum) {

Reply | Threaded
Open this post in threaded view
|

Re: "sed -i" not renaming/moving temp-file with "q" command

Martijn van Duren-8
On 08/14/18 14:07, Todd C. Miller wrote:

> On Tue, 14 Aug 2018 10:43:30 +0200, Martijn van Duren wrote:
>
>> The diff below fixes this. Note that I took special care to make a
>> distinction between in place and normal for the 'q' command.
>> When running normally the files are concatenated, so we should quit
>> immediately. When running in in place mode I reckon the script
>> should be treated as if it were to run for every file individually,
>> since the line numbers are also reset in between files.
>
> I'm not convinced this is the right approach.  If there is a quit
> command, the script should quit without further processing.
>
> We could do something like this instead which matches what GNU sed
> does more closely.  Since the -i flag is a GNU invention I think
> it is worthwhile trying to behave similarly.

If we want to mimic what gnu sed does we also need to reset the
hold space:
$ printf '1\n2\n3\n' | tee /tmp/test > /tmp/test1
$ sed -i 'x' /tmp/test{,1}
$ cat /tmp/test

1
2
$ cat /tmp/test1
3
1
2
$ printf '1\n2\n3\n' | tee /tmp/test > /tmp/test1
$ gsed -i 'x' /tmp/test{,1}
$ cat /tmp/test                                  

1
2
$ cat /tmp/test1

1
2
Do note the difference with using it without -i:
$ printf '1\n2\n3\n' | tee /tmp/test > /tmp/test1
$ gsed 'x' /tmp/test{,1}

1
2
3
1
2

So gnu is somewhat contradictory here. On the one hand it treats
every file as a new script, based on the hold-space, but the 'q'
command exits the editor all together.

So the question is how close do we want to follow gnu sed and
how consistent do we want to be in our definitions.
- Do we treat "-i" as a single script passing, then I'd expect
sed to quit, similar to what gnu sed does and keep our hold space
over multiple files (unlike what gnu sed does, but we currently do)
- Do we treat "-i" as multiple executions of the same script
(similar to "for file in *; do sed -i ...; done"), then I'd expect
sed to reset the hold space, similar to what gnu sed does and
not quit the editor itself, but merely the execution on the
current file.
- Do we treat "-i" as quirk by quirk compatible with gnu sed, then
the diff also fixes the hold space case.

martijn@

Index: extern.h
===================================================================
RCS file: /cvs/src/usr.bin/sed/extern.h,v
retrieving revision 1.13
diff -u -p -r1.13 extern.h
--- extern.h 1 Aug 2017 18:05:53 -0000 1.13
+++ extern.h 14 Aug 2018 13:12:35 -0000
@@ -44,6 +44,7 @@ extern int Eflag, aflag, eflag, nflag;
 extern int pledge_wpath, pledge_rpath;
 extern const char *fname, *outfname;
 extern FILE *infile, *outfile;
+extern SPACE HS;
 
 void cfclose(struct s_command *, struct s_command *);
 void compile(void);
@@ -53,6 +54,7 @@ __dead void error(int, const char *, ...
 void warning(const char *, ...);
 int mf_fgets(SPACE *, enum e_spflag);
 int lastline(void);
+void finish_file(void);
 void process(void);
 void resetranges(void);
 char *strregerror(int, regex_t *);
Index: main.c
===================================================================
RCS file: /cvs/src/usr.bin/sed/main.c,v
retrieving revision 1.37
diff -u -p -r1.37 main.c
--- main.c 11 Jul 2018 06:57:18 -0000 1.37
+++ main.c 14 Aug 2018 13:12:35 -0000
@@ -310,6 +310,30 @@ again:
  return (NULL);
 }
 
+void
+finish_file(void)
+{
+ if (infile != NULL) {
+ fclose(infile);
+ if (*oldfname != '\0') {
+ if (rename(fname, oldfname) != 0) {
+ warning("rename()");
+ unlink(tmpfname);
+ exit(1);
+ }
+ *oldfname = '\0';
+ }
+ if (*tmpfname != '\0') {
+ if (outfile != NULL && outfile != stdout)
+ fclose(outfile);
+ outfile = NULL;
+ rename(tmpfname, fname);
+ *tmpfname = '\0';
+ }
+ outfname = NULL;
+ }
+}
+
 /*
  * Like fgets, but go through the list of files chaining them together.
  * Set len to the length of the line.
@@ -347,25 +371,7 @@ mf_fgets(SPACE *sp, enum e_spflag spflag
  sp->len = 0;
  return (0);
  }
- if (infile != NULL) {
- fclose(infile);
- if (*oldfname != '\0') {
- if (rename(fname, oldfname) != 0) {
- warning("rename()");
- unlink(tmpfname);
- exit(1);
- }
- *oldfname = '\0';
- }
- if (*tmpfname != '\0') {
- if (outfile != NULL && outfile != stdout)
- fclose(outfile);
- outfile = NULL;
- rename(tmpfname, fname);
- *tmpfname = '\0';
- }
- outfname = NULL;
- }
+ finish_file();
  if (firstfile == 0)
  files = files->next;
  else
@@ -376,6 +382,8 @@ mf_fgets(SPACE *sp, enum e_spflag spflag
  }
  fname = files->fname;
  if (inplace != NULL) {
+ free(HS.back);
+ memset(&HS, 0, sizeof(HS));
  if (lstat(fname, &sb) != 0)
  error(FATAL, "%s: %s", fname,
     strerror(errno ? errno : EIO));
Index: process.c
===================================================================
RCS file: /cvs/src/usr.bin/sed/process.c,v
retrieving revision 1.33
diff -u -p -r1.33 process.c
--- process.c 13 Dec 2017 16:06:34 -0000 1.33
+++ process.c 14 Aug 2018 13:12:35 -0000
@@ -50,7 +50,8 @@
 #include "defs.h"
 #include "extern.h"
 
-static SPACE HS, PS, SS;
+static SPACE PS, SS;
+SPACE HS;
 #define pd PS.deleted
 #define ps PS.space
 #define psl PS.len
@@ -196,6 +197,7 @@ redirect:
  if (!nflag && !pd)
  OUT();
  flush_appends();
+ finish_file();
  exit(0);
  case 'r':
  if (appendx >= appendnum) {
Index: sed.1
===================================================================
RCS file: /cvs/src/usr.bin/sed/sed.1,v
retrieving revision 1.56
diff -u -p -r1.56 sed.1
--- sed.1 11 Jul 2018 06:47:38 -0000 1.56
+++ sed.1 14 Aug 2018 13:12:35 -0000
@@ -106,6 +106,7 @@ It is not recommended to give a zero len
 .Ar extension
 when in place editing files, as it risks corruption or partial content
 in situations where disk space is exhausted, etc.
+Editing multiple files will reset the hold space in between files.
 .It Fl r
 An alias for
 .Fl E ,

Reply | Threaded
Open this post in threaded view
|

Re: "sed -i" not renaming/moving temp-file with "q" command

Todd C. Miller-2
On Tue, 14 Aug 2018 15:16:47 +0200, Martijn van Duren wrote:

> So gnu is somewhat contradictory here. On the one hand it treats
> every file as a new script, based on the hold-space, but the 'q'
> command exits the editor all together.

The 'q' command should quit the editor, not just the current file.
Resetting the hold space for each file seems reasonable.

> So the question is how close do we want to follow gnu sed and
> how consistent do we want to be in our definitions.
> - Do we treat "-i" as a single script passing, then I'd expect
> sed to quit, similar to what gnu sed does and keep our hold space
> over multiple files (unlike what gnu sed does, but we currently do)
> - Do we treat "-i" as multiple executions of the same script
> (similar to "for file in *; do sed -i ...; done"), then I'd expect
> sed to reset the hold space, similar to what gnu sed does and
> not quit the editor itself, but merely the execution on the
> current file.
> - Do we treat "-i" as quirk by quirk compatible with gnu sed, then
> the diff also fixes the hold space case.

We should try for quirk by quirk compatible with gnu sed with respect
to the -i flag since that's where the feature comes from.

Your updated diff looks good to me.

 - todd

Reply | Threaded
Open this post in threaded view
|

Re: "sed -i" not renaming/moving temp-file with "q" command

Ingo Schwarze
In reply to this post by Martijn van Duren-8
Hi Martijn,

i agree with Todd that in implementing an option invented by GNU,
we should follow GNU semantics unless there are very strong reasons
to diverge.  In this case, GNU -i semantics is maybe not the only
possibility that could be regarded as reasonable, but it certainly
isn't unreasonable:  Execute the script independently for each file
in turn, but let the 'q' command still exit the editor completely,
rather than just moving to the next file.

So, i agree with resetting the hold space when moving from one file
to the next in -i mode.  But i think the variable HS ought to remain
in process.c and not be moved to extern.h and not be manipulated from
main.c.

Instead, there already is a function to reset internal processor
state when moving to the next file with -i: resetranges().
Can you add the two lines to clear the hold space into that
function and resend the diff?

If you want, you can make the name of the function more generic,
for example reset_processor().

Also, in the manual page, while saying that the hold space is reset,
also mention what happens to line numbers and ranges, be even more
explicit that all this applies *only* for -i, and avoid the future
tense, for example like this:

  In
  .Fl i
  mode, line numbers and the hold space are reset between files,
  and no range extends from one file into the next.

Finally, in the description of the 'q' command, we can also be
more explicit:

  Branch to the end of the script and quit
  .Nm
  without starting a new cycle or a new file.

Yours,
  Ingo

Reply | Threaded
Open this post in threaded view
|

Re: "sed -i" not renaming/moving temp-file with "q" command

Martijn van Duren-8
On 8/15/18 8:58 PM, Ingo Schwarze wrote:

> Hi Martijn,
>
> i agree with Todd that in implementing an option invented by GNU,
> we should follow GNU semantics unless there are very strong reasons
> to diverge.  In this case, GNU -i semantics is maybe not the only
> possibility that could be regarded as reasonable, but it certainly
> isn't unreasonable:  Execute the script independently for each file
> in turn, but let the 'q' command still exit the editor completely,
> rather than just moving to the next file.
>
> So, i agree with resetting the hold space when moving from one file
> to the next in -i mode.  But i think the variable HS ought to remain
> in process.c and not be moved to extern.h and not be manipulated from
> main.c.
>
> Instead, there already is a function to reset internal processor
> state when moving to the next file with -i: resetranges().
> Can you add the two lines to clear the hold space into that
> function and resend the diff?
>
> If you want, you can make the name of the function more generic,
> for example reset_processor().
>
> Also, in the manual page, while saying that the hold space is reset,
> also mention what happens to line numbers and ranges, be even more
> explicit that all this applies *only* for -i, and avoid the future
> tense, for example like this:
>
>   In
>   .Fl i
>   mode, line numbers and the hold space are reset between files,
>   and no range extends from one file into the next.
>
> Finally, in the description of the 'q' command, we can also be
> more explicit:
>
>   Branch to the end of the script and quit
>   .Nm
>   without starting a new cycle or a new file.
>
> Yours,
>   Ingo
>
I send the diff below to Ingo some time ago, but never got a reply.
Maybe someone else is willing to look at it?

Index: extern.h
===================================================================
RCS file: /cvs/src/usr.bin/sed/extern.h,v
retrieving revision 1.13
diff -u -p -r1.13 extern.h
--- extern.h 1 Aug 2017 18:05:53 -0000 1.13
+++ extern.h 23 Aug 2018 08:11:48 -0000
@@ -53,8 +53,9 @@ __dead void error(int, const char *, ...
 void warning(const char *, ...);
 int mf_fgets(SPACE *, enum e_spflag);
 int lastline(void);
+void finish_file(void);
 void process(void);
-void resetranges(void);
+void resetstate(void);
 char *strregerror(int, regex_t *);
 void *xmalloc(size_t);
 void *xreallocarray(void *, size_t, size_t);
Index: main.c
===================================================================
RCS file: /cvs/src/usr.bin/sed/main.c,v
retrieving revision 1.37
diff -u -p -r1.37 main.c
--- main.c 11 Jul 2018 06:57:18 -0000 1.37
+++ main.c 23 Aug 2018 08:11:48 -0000
@@ -310,6 +310,30 @@ again:
  return (NULL);
 }
 
+void
+finish_file(void)
+{
+ if (infile != NULL) {
+ fclose(infile);
+ if (*oldfname != '\0') {
+ if (rename(fname, oldfname) != 0) {
+ warning("rename()");
+ unlink(tmpfname);
+ exit(1);
+ }
+ *oldfname = '\0';
+ }
+ if (*tmpfname != '\0') {
+ if (outfile != NULL && outfile != stdout)
+ fclose(outfile);
+ outfile = NULL;
+ rename(tmpfname, fname);
+ *tmpfname = '\0';
+ }
+ outfname = NULL;
+ }
+}
+
 /*
  * Like fgets, but go through the list of files chaining them together.
  * Set len to the length of the line.
@@ -347,25 +371,7 @@ mf_fgets(SPACE *sp, enum e_spflag spflag
  sp->len = 0;
  return (0);
  }
- if (infile != NULL) {
- fclose(infile);
- if (*oldfname != '\0') {
- if (rename(fname, oldfname) != 0) {
- warning("rename()");
- unlink(tmpfname);
- exit(1);
- }
- *oldfname = '\0';
- }
- if (*tmpfname != '\0') {
- if (outfile != NULL && outfile != stdout)
- fclose(outfile);
- outfile = NULL;
- rename(tmpfname, fname);
- *tmpfname = '\0';
- }
- outfname = NULL;
- }
+ finish_file();
  if (firstfile == 0)
  files = files->next;
  else
@@ -405,7 +411,7 @@ mf_fgets(SPACE *sp, enum e_spflag spflag
  fchmod(fileno(outfile), sb.st_mode & ALLPERMS);
  outfname = tmpfname;
  linenum = 0;
- resetranges();
+ resetstate();
  } else {
  outfile = stdout;
  outfname = "stdout";
Index: process.c
===================================================================
RCS file: /cvs/src/usr.bin/sed/process.c,v
retrieving revision 1.33
diff -u -p -r1.33 process.c
--- process.c 13 Dec 2017 16:06:34 -0000 1.33
+++ process.c 23 Aug 2018 08:11:48 -0000
@@ -196,6 +196,7 @@ redirect:
  if (!nflag && !pd)
  OUT();
  flush_appends();
+ finish_file();
  exit(0);
  case 'r':
  if (appendx >= appendnum) {
@@ -312,9 +313,12 @@ applies(struct s_command *cp)
  * Reset all inrange markers.
  */
 void
-resetranges(void)
+resetstate(void)
 {
  struct s_command *cp;
+
+ free(HS.back);
+ memset(&HS, 0, sizeof(HS));
 
  for (cp = prog; cp; cp = cp->code == '{' ? cp->u.c : cp->next)
  if (cp->a2)
Index: sed.1
===================================================================
RCS file: /cvs/src/usr.bin/sed/sed.1,v
retrieving revision 1.56
diff -u -p -r1.56 sed.1
--- sed.1 11 Jul 2018 06:47:38 -0000 1.56
+++ sed.1 23 Aug 2018 08:11:48 -0000
@@ -106,6 +106,9 @@ It is not recommended to give a zero len
 .Ar extension
 when in place editing files, as it risks corruption or partial content
 in situations where disk space is exhausted, etc.
+In
+.Fl i
+mode, the hold space, line numbers, and ranges are reset between files.
 .It Fl r
 An alias for
 .Fl E ,
@@ -392,7 +395,7 @@ Write the pattern space to standard outp
 Write the pattern space, up to the first newline character,
 to the standard output.
 .It [1addr] Ns Ic q
-Branch to the end of the script and quit without starting a new cycle.
+Branch to the end of the script and quit without starting a new cycle or file.
 .It [1addr] Ns Ic r Ar file
 Copy the contents of
 .Ar file

Reply | Threaded
Open this post in threaded view
|

Re: "sed -i" not renaming/moving temp-file with "q" command

Martijn van Duren-8
ping

On 10/31/18 7:28 AM, Martijn van Duren wrote:

> On 8/15/18 8:58 PM, Ingo Schwarze wrote:
>> Hi Martijn,
>>
>> i agree with Todd that in implementing an option invented by GNU,
>> we should follow GNU semantics unless there are very strong reasons
>> to diverge.  In this case, GNU -i semantics is maybe not the only
>> possibility that could be regarded as reasonable, but it certainly
>> isn't unreasonable:  Execute the script independently for each file
>> in turn, but let the 'q' command still exit the editor completely,
>> rather than just moving to the next file.
>>
>> So, i agree with resetting the hold space when moving from one file
>> to the next in -i mode.  But i think the variable HS ought to remain
>> in process.c and not be moved to extern.h and not be manipulated from
>> main.c.
>>
>> Instead, there already is a function to reset internal processor
>> state when moving to the next file with -i: resetranges().
>> Can you add the two lines to clear the hold space into that
>> function and resend the diff?
>>
>> If you want, you can make the name of the function more generic,
>> for example reset_processor().
>>
>> Also, in the manual page, while saying that the hold space is reset,
>> also mention what happens to line numbers and ranges, be even more
>> explicit that all this applies *only* for -i, and avoid the future
>> tense, for example like this:
>>
>>   In
>>   .Fl i
>>   mode, line numbers and the hold space are reset between files,
>>   and no range extends from one file into the next.
>>
>> Finally, in the description of the 'q' command, we can also be
>> more explicit:
>>
>>   Branch to the end of the script and quit
>>   .Nm
>>   without starting a new cycle or a new file.
>>
>> Yours,
>>   Ingo
>>
> I send the diff below to Ingo some time ago, but never got a reply.
> Maybe someone else is willing to look at it?
>
> Index: extern.h
> ===================================================================
> RCS file: /cvs/src/usr.bin/sed/extern.h,v
> retrieving revision 1.13
> diff -u -p -r1.13 extern.h
> --- extern.h 1 Aug 2017 18:05:53 -0000 1.13
> +++ extern.h 23 Aug 2018 08:11:48 -0000
> @@ -53,8 +53,9 @@ __dead void error(int, const char *, ...
>  void warning(const char *, ...);
>  int mf_fgets(SPACE *, enum e_spflag);
>  int lastline(void);
> +void finish_file(void);
>  void process(void);
> -void resetranges(void);
> +void resetstate(void);
>  char *strregerror(int, regex_t *);
>  void *xmalloc(size_t);
>  void *xreallocarray(void *, size_t, size_t);
> Index: main.c
> ===================================================================
> RCS file: /cvs/src/usr.bin/sed/main.c,v
> retrieving revision 1.37
> diff -u -p -r1.37 main.c
> --- main.c 11 Jul 2018 06:57:18 -0000 1.37
> +++ main.c 23 Aug 2018 08:11:48 -0000
> @@ -310,6 +310,30 @@ again:
>   return (NULL);
>  }
>  
> +void
> +finish_file(void)
> +{
> + if (infile != NULL) {
> + fclose(infile);
> + if (*oldfname != '\0') {
> + if (rename(fname, oldfname) != 0) {
> + warning("rename()");
> + unlink(tmpfname);
> + exit(1);
> + }
> + *oldfname = '\0';
> + }
> + if (*tmpfname != '\0') {
> + if (outfile != NULL && outfile != stdout)
> + fclose(outfile);
> + outfile = NULL;
> + rename(tmpfname, fname);
> + *tmpfname = '\0';
> + }
> + outfname = NULL;
> + }
> +}
> +
>  /*
>   * Like fgets, but go through the list of files chaining them together.
>   * Set len to the length of the line.
> @@ -347,25 +371,7 @@ mf_fgets(SPACE *sp, enum e_spflag spflag
>   sp->len = 0;
>   return (0);
>   }
> - if (infile != NULL) {
> - fclose(infile);
> - if (*oldfname != '\0') {
> - if (rename(fname, oldfname) != 0) {
> - warning("rename()");
> - unlink(tmpfname);
> - exit(1);
> - }
> - *oldfname = '\0';
> - }
> - if (*tmpfname != '\0') {
> - if (outfile != NULL && outfile != stdout)
> - fclose(outfile);
> - outfile = NULL;
> - rename(tmpfname, fname);
> - *tmpfname = '\0';
> - }
> - outfname = NULL;
> - }
> + finish_file();
>   if (firstfile == 0)
>   files = files->next;
>   else
> @@ -405,7 +411,7 @@ mf_fgets(SPACE *sp, enum e_spflag spflag
>   fchmod(fileno(outfile), sb.st_mode & ALLPERMS);
>   outfname = tmpfname;
>   linenum = 0;
> - resetranges();
> + resetstate();
>   } else {
>   outfile = stdout;
>   outfname = "stdout";
> Index: process.c
> ===================================================================
> RCS file: /cvs/src/usr.bin/sed/process.c,v
> retrieving revision 1.33
> diff -u -p -r1.33 process.c
> --- process.c 13 Dec 2017 16:06:34 -0000 1.33
> +++ process.c 23 Aug 2018 08:11:48 -0000
> @@ -196,6 +196,7 @@ redirect:
>   if (!nflag && !pd)
>   OUT();
>   flush_appends();
> + finish_file();
>   exit(0);
>   case 'r':
>   if (appendx >= appendnum) {
> @@ -312,9 +313,12 @@ applies(struct s_command *cp)
>   * Reset all inrange markers.
>   */
>  void
> -resetranges(void)
> +resetstate(void)
>  {
>   struct s_command *cp;
> +
> + free(HS.back);
> + memset(&HS, 0, sizeof(HS));
>  
>   for (cp = prog; cp; cp = cp->code == '{' ? cp->u.c : cp->next)
>   if (cp->a2)
> Index: sed.1
> ===================================================================
> RCS file: /cvs/src/usr.bin/sed/sed.1,v
> retrieving revision 1.56
> diff -u -p -r1.56 sed.1
> --- sed.1 11 Jul 2018 06:47:38 -0000 1.56
> +++ sed.1 23 Aug 2018 08:11:48 -0000
> @@ -106,6 +106,9 @@ It is not recommended to give a zero len
>  .Ar extension
>  when in place editing files, as it risks corruption or partial content
>  in situations where disk space is exhausted, etc.
> +In
> +.Fl i
> +mode, the hold space, line numbers, and ranges are reset between files.
>  .It Fl r
>  An alias for
>  .Fl E ,
> @@ -392,7 +395,7 @@ Write the pattern space to standard outp
>  Write the pattern space, up to the first newline character,
>  to the standard output.
>  .It [1addr] Ns Ic q
> -Branch to the end of the script and quit without starting a new cycle.
> +Branch to the end of the script and quit without starting a new cycle or file.
>  .It [1addr] Ns Ic r Ar file
>  Copy the contents of
>  .Ar file
>

Reply | Threaded
Open this post in threaded view
|

Re: "sed -i" not renaming/moving temp-file with "q" command

Todd C. Miller-2
OK millert@

 - todd