Hi Guillem, Thank you for the review.
On Fri, Jul 09, 2021 at 02:23:31PM +0200, Guillem Jover wrote: > I'm wondering whether tying the pathname to a Debian specific package > name might make a possible "standardization" or adoption by say other > distributions or upstream harder? Perhaps a more generic name would > leave the door open to such adoption, so that upstreams could ship > those files directly f.ex.? I'm not opposed. My intention here was to avoid stepping on other peoples' toes. > > + realshell=$(dpkg-realpath --root "$ROOT" "$line") > > Not sure how desirable this would be, but if DPKG_ROOT is set in the > caller environment, but the caller did not specify --root for this > program, then DPKG_ROOT will not be taken into account by > dpkg-realpath, as this unconditionally overrides it. Perhaps this > program should be defaulting ROOT to DPKG_ROOT? This actually is intentional. I figured that there was only one legitimate caller of this tool in maintainer scripts and that one caller could easily pass --root explicitly. Therefore, I intentionally did not add DPKG_ROOT support to the tool itself. DPKG_ROOT in tools always feels a little magic to me, so I err on being explicit when feasible. > > + if [ "$line" != "$realshell" ]; then > > + PKG_SHELLS="$PKG_SHELLS$realshell#" > > + fi > > + done < "$f" > > +done > > Doesn't seem to handle an interrupted invocation by removing staged > files for in-between steps for the shells and state files. Leaving tempfiles around when interrupted seems fair enough to me. Existing tempfiles are truncated. What kind of practical issue do you see here? > Isn't this missing the filesystem existence check for the shell to > handle local admin additions and the default fragment default cases, > as described in the algorithm? I didn't understand the need for the existence check, so I left it out. Do you envision partially unpacked packages here? I ruled that out as too unrealistic. If an admin adds a non-existent shell, who am I to remove it? > I've just skimmed over this algo (and run out of time now), but it does > not match directly to the one proposed on the list so I'll have to think > about this a bit more. :) Yeah, what I sold as your proposal is actually slightly different. Seems to work anyhow. :) > I think this is missing sync'ing the containing directories too. Agreed. Though given that other tools don't sync maybe it doesn't really matter that much. Also sync should do the right thing. > Perhaps move or add the file descriptions to a FILES section? Reasonable request. Helmut
