Nikita Karetnikov <nik...@karetnikov.org> skribis:
> Can I push this patch to ‘master’? Do you see any problems?
Looks good! Minor issues discussed below.
> I had noticed that ‘--roll-back’ doesn’t output anything with
> ‘--dry-run’, so I implemented ‘--delete-generations’ similarly. Maybe
> it would be better to print something. WDYT?
Agreed (in a separate patch.)
> +@item --delete-generations[=@var{pattern}]
> +@itemx -d [@var{pattern}]
> +Delete generations.
“Delete the generations matching @var{patterns} or ... when omitted.”
Or what actually? I would expect it to delete all the generations but
the current one when PATTERN is omitted, right?
> +(define (link-to-empty-environment generation)
> + "Link GENERATION, a string, to the empty environment."
s/environment/profile/ maybe? (I know there are other inconsistencies
in these files.)
Ideally this factorization would go in a patch of its own, before the
one that adds --delete-generations. Is that doable for you?
> + (define (apply-to-generations function profile pattern)
s/function/proc/ to follow the convention.
> + (cond ((zero? number)) ; do not delete generation 0
> + ((and (current-generation? profile generation)
> + (not (file-exists? previous-generation)))
> + (begin (link-to-empty-environment previous-generation)
> + (switch-to-previous-generation profile)
> + (display-and-delete generation)))
> + ((current-generation? profile generation)
> + (begin (roll-back profile)
> + (display-and-delete generation)))
No need for ‘begin’ in the body of a ‘cond’ clause.
> ;; First roll back if asked to.
> - (if (and (assoc-ref opts 'roll-back?) (not dry-run?))
> - (begin
> - (roll-back profile)
> - (process-actions (alist-delete 'roll-back? opts)))
> - (let* ((installed (manifest-packages (profile-manifest profile)))
> - (upgrade-regexps (filter-map (match-lambda
> - (('upgrade . regexp)
> - (make-regexp (or regexp "")))
> - (_ #f))
[...]
Why is there this big hunk? If it’s just reindenting, could you arrange
to remove this hunk?
Thanks!
Ludo’.
