Re: Display diffs between generations.

[Roel Janssen <roel@gnu.org>]

Ludovic Courtès writes:

> Hi Roel,
>
> I’ve just tried the patch and I think it’s awesome!  I’ve also always
> been dissatisfied with what ‘--list-generations’ provides—it’s
> inconvenient as soon as you have more than a handful packages.
>
> Perhaps we could add an optional argument to --list-generations where:
>
>   --list-generations
>
> would be equivalent to:
>
>   --list-generations=diff
>
> and:
>
>   --list-generations=full
>
> would restore the previous behavior.  It doesn’t cost much to do that.
>
> WDYT?

That would be great.  Currently, the --list-generations already takes a
generation number.  How would we deal with the situation where we want
to view the differences between generation 2 and 3 only?

  --list-generations=3,diff

That doesn't look appealing to me.

Maybe we could do this instead?:

  --list-generations-diff

> I have only minor stylistic comments:
>
> Roel Janssen <roel@gnu.org> skribis:
>
>> +(define (display-profile-content-diff profile number)
>
> Or just ‘display-profile-diff’?
>
>> +  "Display the changed packages in PROFILE with generation specified by NUMBER."
>
> “… compared to generation NUMBER”?
>
>> +  (define (equal-entry? first second)
>> +    (string= (manifest-entry-item first)
>> +             (manifest-entry-item second)))
>> +
>> +  (define* (display-entries entries #:optional (prefix " "))
>> +    (for-each
>> +     (match-lambda
>> +       (($ <manifest-entry> name version output location _)
>> +        (format #t "  ~a ~a\t~a\t~a\t~a~%"
>> +                prefix name version output location)))
>> +     entries))
>
> In general, I find it clearer to define the singular form and inline the
> ‘for-each’:
>
>   (define display-entry
>     (match-lambda …))
>
>   …
>
>   (for-each display-entry entries)
>
> Otherwise LGTM!

Right.  That does look clearer.  I refactored the function further to
the following:

------- BEGIN -------
(define (display-profile-content-diff profile number)
  "Display the changed packages in PROFILE compared to generation NUMBER."

  (define (equal-entry? first second)
    (string= (manifest-entry-item first)
             (manifest-entry-item second)))

  (define display-entry
    (match-lambda
       (($ <manifest-entry> name version output location _)
        (format #f "~a\t~a\t~a\t~a~%" name version output location))))

  (define (display-entries entries prefix)
    (for-each (lambda (entry)
                (format #t "  ~a ~a" prefix (display-entry entry))) entries))

  (define (list-entries input)
    (manifest-entries (profile-manifest (generation-file-name profile input))))

  (define (display-diff profile older newer)
    (if (= older 0)
        (display-profile-content profile newer)
        (begin
          ;; List newly installed packages.
          (display-entries (lset-difference equal-entry?
                                            (list-entries newer)
                                            (list-entries older)) "+")
          ;; List newly removed packages.
          (display-entries (lset-difference equal-entry?
                                            (list-entries older)
                                            (list-entries newer)) "-"))))

  (display-diff profile (previous-generation-number profile number) number))
------- END -------

The thing with `display-entries' is because I cannot pass two arguments
in the function used in the `for-each', and the prefix for `installed'
or `removed' differs, I cannot remove it that easily :-).

Kind regards,
Roel Janssen