Re: [9front] PATCH walk(1) to (optionally) quote name and path

[unobe@cpan.org]

Thanks for the feedback, Ori.  I'm not a C programmer, and it shows.
See my replies below.

Quoth ori@eigenstate.org:
> Quoth Romano <unobe@cpan.org>:
> > I'm working with files on a backup drive that have spaces and
> > quotation marks in their names, using walk(1).  I can pipe to sed
> > s/''''/''''''''''/g and do similarly for other characters requiring
> > escaping for rc(1), but figured patching walk(1) would be better.
> 
> generally no objections, though it's usually relatively easy to
> structure scripts such that there's relatively little that is
> interpreted by rc; is this for interactive use?
> 
> > I don't like how I just copied case 'p': the quoting "works", but isn't
> > as clean as the output for ls(1).
> 
> I'll need to look at the code to see if there's a way to
> improve this; regardless, a few comments inline:
> 
> > But it's better than what happens now, where there's no quoting.
> > 
> > From: Romano <unobe@cpan.org>
> > Date: Sat, 12 Aug 2023 03:18:22 +0000
> > Subject: [PATCH] walk: quote name and paths using N and P
> > 
> > ---
> > diff deb39a43ae5ed09c7971726cedeb06a9f65ccc6d 2da52094832662c533f60629813dbd5ac805e3ad
> > --- a/sys/man/1/walk
> > +++ b/sys/man/1/walk
> > @@ -81,8 +81,14 @@
> >  .B n
> >  final path element (name)
> >  .TP
> > +.B N
> > +final path element (name), rc (1) quoted when necessary
> > +.TP
> >  .B p
> >  path
> > +.TP
> > +.B P
> > +path, rc (1) quoted when necessary
> >  .TP
> >  .B q
> >  qid path.version.type (see
> > --- a/sys/src/cmd/walk.c
> > +++ b/sys/src/cmd/walk.c
> > @@ -43,7 +43,7 @@
> >  void
> >  dofile(char *path, Dir *f, int pathonly)
> >  {
> > -	char *p;
> > +	char *p, *quoted;
> 
> nitpicking on style; perhaps '*q' instead of '*quoted'

Got it--be terse.

> 
> >  
> >  	if(
> >  		(f == dotdir)
> > @@ -60,6 +60,10 @@
> >  		case 'a': Bprint(bout, "%uld", f->atime); break;
> >  		case 'm': Bprint(bout, "%uld", f->mtime); break;
> >  		case 'n': Bwrite(bout, f->name, strlen(f->name)); break;
> > +		case 'N':
> > +			quoted = quotestrdup(f->name);
> > +			Bwrite(bout, quoted, strlen(quoted));
> 
> looks like this leaks 'quoted'; same with all other quotestrdup calls.

Okay, so if I'm dup'ing a string, there's memory allocated that needs
to be free()'d.

> 
> > +			break;
> >  		case 'p':
> >  			if(path != dotpath)
> >  				Bwrite(bout, path, strlen(path));
> > @@ -69,6 +73,18 @@
> >  				Bwrite(bout, f->name, strlen(f->name));
> >  			}
> >  			break;
> > +		case 'P':
> 
> this quoting could be done by concating the path and the
> dotpath, possibly even with libstring. Not sure if it'd
> be cleaner to dedup the 'p' and 'P' cases or not yet.

Yeah, I was thinking about consolidating the code for 'p' and 'P', but
figured the path of least resistance is to first just copy-and-modify.
Why not use sprint() instead of concat, since I need to interpose '/'
for the path? Anyway, see below for a patch to my patch: does it look
better?

> 
> > +			if(path != dotpath) {
> > +				quoted = quotestrdup(path);
> > +				Bwrite(bout, quoted, strlen(quoted));
> > +			}
> > +			if(! (f->qid.type & QTDIR) && !pathonly){
> > +				if(path != dotpath)
> > +					Bputc(bout, '/');
> > +				quoted = quotestrdup(f->name);
> > +				Bwrite(bout, quoted, strlen(quoted));
> > +			}
> > +			break;
> >  		case 'q': Bprint(bout, "%ullx.%uld.%.2uhhx", f->qid.path, f->qid.vers, f->qid.type); break;
> >  		case 's': Bprint(bout, "%lld", f->length); break;
> >  		case 'x': Bprint(bout, "%M", f->mode); break;
> > @@ -243,7 +259,7 @@
> >  		if((stfmt = s_reset(stfmt)) == nil)
> >  			sysfatal("s_reset: %r");
> >  		s_append(stfmt, EARGF(usage()));
> > -		i = strspn(s_to_c(stfmt), "UGMamnpqsxDT");
> > +		i = strspn(s_to_c(stfmt), "UGMamnNpPqsxDT");
> >  		if(i != s_len(stfmt))
> >  			sysfatal("bad stfmt: %s", s_to_c(stfmt));
> >  		break;
> > 
> 

From: Romano <unobe@cpan.org>
Date: Sat, 12 Aug 2023 06:32:08 +0000
Subject: [PATCH] walk: use quotefmt and free()

---
diff 2da52094832662c533f60629813dbd5ac805e3ad c371afbc8bcf6882fb1e77b5ddfefb198e331a8f
--- a/sys/src/cmd/walk.c
+++ b/sys/src/cmd/walk.c
@@ -43,7 +43,7 @@
 void
 dofile(char *path, Dir *f, int pathonly)
 {
-	char *p, *quoted;
+	char *p, *q;
 
 	if(
 		(f == dotdir)
@@ -61,8 +61,7 @@
 		case 'm': Bprint(bout, "%uld", f->mtime); break;
 		case 'n': Bwrite(bout, f->name, strlen(f->name)); break;
 		case 'N':
-			quoted = quotestrdup(f->name);
-			Bwrite(bout, quoted, strlen(quoted));
+			Bprint(bout, "%q", f->name);
 			break;
 		case 'p':
 			if(path != dotpath)
@@ -74,16 +73,13 @@
 			}
 			break;
 		case 'P':
-			if(path != dotpath) {
-				quoted = quotestrdup(path);
-				Bwrite(bout, quoted, strlen(quoted));
-			}
-			if(! (f->qid.type & QTDIR) && !pathonly){
-				if(path != dotpath)
-					Bputc(bout, '/');
-				quoted = quotestrdup(f->name);
-				Bwrite(bout, quoted, strlen(quoted));
-			}
+			if(!pathonly && path != dotpath && ! (f->qid.type & QTDIR)) {
+				q = smprint("%s/%s", path, f->name);
+				Bprint(bout, "%q", q);
+				free(q);
+}
+			else if (path != dotpath)
+				Bprint(bout, "%q", path);
 			break;
 		case 'q': Bprint(bout, "%ullx.%uld.%.2uhhx", f->qid.path, f->qid.vers, f->qid.type); break;
 		case 's': Bprint(bout, "%lld", f->length); break;
@@ -268,6 +264,7 @@
 	}ARGEND;
 
 	fmtinstall('M', dirmodefmt);
+	quotefmtinstall();
 
 	if((bout = Bfdopen(1, OWRITE)) == nil)
 		sysfatal("Bfdopen: %r");