Roland Mainz writes: > James Carlson wrote: > > John Sonnenschein writes: > > > code at: > > > http://cr.opensolaris.org/~error404/6733971/ > > > > Instead of introducing spaces -- which I think makes the code just a > > little more obscure -- why not get rid of the superfluous parenthesis? > > Good question... the answer is that we do not know exactly _why_ this > stuff was originally added with subshells. It seems to be done > intentionally but noone knows _why_.
At least for me, that's just not a good enough answer. There shouldn't be any mystery here, particularly for a change that's going to be made in a whole pile of files at once and that is small enough to have reasonably well-understood results. As for the origin, I went back to the SCCS 1.1 version of $SRC/uts/common/Makefile.rules (from 1991), and the puzzling extra parenthesis are there. That seems to be the oldest version. It's not in SysV or old SunOS. Given that, and the fact that the code does exactly the same thing with or without the redundant parenthesis, I'm claiming that it's just an ancient mistake and shouldn't be embellished. > > That way, there's no question for future maintainers, and both forms > > appear to be equivalent for ksh93. > > > > Are all these subshells doing us any good? > > As said above I tried to get an answer and so far the investigation was > for /dev/null. My gut feeling says we should keep them. Why? I refer to the question above: what good do the subshells do? > Options so far are: > 1. Keep the patch in the current state and forward the investigation to > the ToDo list of the OS/Net Janitor project > 2. Keep the patch and ignore the extra brackets (they cause an extra > |fork()| for the original Bourne shell but ksh93 does not |fork()| for > subshells which will make this efectively a no-op) > 3. Make a new patch and remove the extra brackets > > I would prefer [1] in this case since [3] would turn this patch IMO into > a high-risk patch and [2] is IMO just reckless. I don't understand [1]. I can't tell which of those options (if any) actually leads to integrating something into the ON gate, and, if so, what gets integrated. I don't know what "keep the patch" means in this context, because I don't know where you're saying it should be kept. If [1] means "don't integrate, but forward along to someone else who can look at it more deeply," then I'd reluctantly go along. I think it's fairly obvious that the subshells do nothing here, but if you (or the actual submitter?) aren't comfortable with that, then doing nothing is a bit safer. If [1] means "integrate as-is now, but then think about it a bit, and maybe rewhack these things again in the future," then I'm opposed to the plan. They should be fixed, and fixed just once. Don't force everyone else through needless merges. I'd be opposed to [2] for essentially the same reason. It seems like a half-baked response to a review comment. If [3] means "make a new patch without the extra parenthesis *and* integrate into ON," then I'm in agreement with that. If it means "make a new patch and then just stop," then I don't understand what the point is. -- James Carlson, Solaris Networking <james.d.carlson at sun.com> Sun Microsystems / 35 Network Drive 71.232W Vox +1 781 442 2084 MS UBUR02-212 / Burlington MA 01803-2757 42.496N Fax +1 781 442 1677
