diff mbox

Display diffs between generations.

Message ID 87eg59izmw.fsf@gnu.org
State New
Headers show

Commit Message

Roel Janssen Aug. 27, 2016, 11:02 p.m. UTC
Dear Guix,

After looking at a terribly long output of `guix package
--list-generations', I thought it may be a good idea to only display the
difference between profile versions.

I came up with the following patch.  What do you think about changing
from displaying the full profile contents for each generation to only
showing the differences between the profiles?

And, I am probably missing something, because some of my generation
entries are empty..  So how could I improve this patch for it to be
complete?

Thanks in advance for your feedback!

Kind regards,
Roel Janssen

Comments

Ludovic Courtès Aug. 29, 2016, 4:25 p.m. UTC | #1
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?

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!

Thank you for addressing this!

Ludo’.
Roel Janssen Aug. 29, 2016, 7:08 p.m. UTC | #2
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
Ludovic Courtès Aug. 31, 2016, 8:52 p.m. UTC | #3
Roel Janssen <roel@gnu.org> skribis:

> 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.

Ah, good point.  Well, forget my suggestion then; I’d say, let’s just do
diffs, and that’s it.  If someone wants to list the contents of one
generation, they can always do “--list-generations=42”, which does
exactly that.

Anyone against ‘--list-generations’ always displaying diffs?  Time to
speak up!  :-)

>> 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))

[...]

> 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 :-).

Maybe like this:

  (define (display-entry entry prefix)
    (match entry …))

  …

  (for-each (cut display-entry <> "+") added)
  (for-each (cut display-entry <> "-") removed)

Let’s wait a bit to see what people think.

Thanks!

Ludo’.
Vincent Legoll Aug. 31, 2016, 9:11 p.m. UTC | #4
Hello

>>>   --list-generations=full

I think keeping a way to see all generations in one go would be cool
Ludovic Courtès Sept. 1, 2016, 12:12 p.m. UTC | #5
Vincent Legoll <vincent.legoll@gmail.com> skribis:

>>>>   --list-generations=full
>
> I think keeping a way to see all generations in one go would be cool

--list-generations would still display all the generations.  Or did you
mean “a way to see *the content* of each generation”?

Ludo’.
Vincent Legoll Sept. 3, 2016, 1:03 p.m. UTC | #6
On Thu, Sep 1, 2016 at 2:12 PM, Ludovic Courtès <ludo@gnu.org> wrote:
>>>>>   --list-generations=full
>>
>> I think keeping a way to see all generations in one go would be cool
>
> --list-generations would still display all the generations.  Or did you
> mean “a way to see *the content* of each generation”?

I'm not sure to understand what you mean by "the content", what I meant
was to keep what current guix package -l shows us, i.e. do not alter current
UI, only add new things, but do not break existing practices when not mandated.
Roel Janssen Sept. 4, 2016, 4:53 p.m. UTC | #7
Vincent Legoll writes:

> On Thu, Sep 1, 2016 at 2:12 PM, Ludovic Courtès <ludo@gnu.org> wrote:
>>>>>>   --list-generations=full
>>>
>>> I think keeping a way to see all generations in one go would be cool
>>
>> --list-generations would still display all the generations.  Or did you
>> mean “a way to see *the content* of each generation”?
>
> I'm not sure to understand what you mean by "the content", what I meant
> was to keep what current guix package -l shows us, i.e. do not alter current
> UI, only add new things, but do not break existing practices when not mandated.

What about adding a `--no-diff-format' or a `--diff-format' argument to
`--list-generations'?

We could then make two choices:  Default to the proposed diff format, or
default to the full format (what we have now).  Depending on what we
choose as default, we can implement the appropriate negating argument.

I would prefer defaulting to the diff format..

Thanks for your feedback!

Kind regards,
Roel Janssen
Ludovic Courtès Sept. 4, 2016, 10:12 p.m. UTC | #8
Vincent Legoll <vincent.legoll@gmail.com> skribis:

> On Thu, Sep 1, 2016 at 2:12 PM, Ludovic Courtès <ludo@gnu.org> wrote:
>>>>>>   --list-generations=full
>>>
>>> I think keeping a way to see all generations in one go would be cool
>>
>> --list-generations would still display all the generations.  Or did you
>> mean “a way to see *the content* of each generation”?
>
> I'm not sure to understand what you mean by "the content", what I meant
> was to keep what current guix package -l shows us, i.e. do not alter current
> UI, only add new things, but do not break existing practices when not mandated.

I agree with this as a general principle, of course.  However, my
experience is that currently -l is hardly usable in practice, because it
spits out way too many lines.

For example, I have 200+ packages in my user profile, so it prints out
several times 200 lines, which is useless for me.  Conversely, the diff
format that Roel proposes would be more immediately readable.

Does that make sense?

Ludo’.
Hartmut Goebel Sept. 5, 2016, 7:52 a.m. UTC | #9
Am 05.09.2016 um 00:12 schrieb Ludovic Courtès:
> I agree with this as a general principle, of course.  However, my
> experience is that currently -l is hardly usable in practice, because it
> spits out way too many lines.

+1
Vincent Legoll Sept. 5, 2016, 10:38 a.m. UTC | #10
Hello,

On Mon, Sep 5, 2016 at 9:52 AM, Hartmut Goebel
<h.goebel@goebel-consult.de> wrote:
> I agree with this as a general principle, of course.  However, my
> experience is that currently -l is hardly usable in practice, because it
> spits out way too many lines.
>
> +1

Ah, I don't have a lot of packages in my profiles because I'm not using it as my
distro, only experimenting, so I didn't see the problem... So maybe the default
needs to change.
Ludovic Courtès Oct. 10, 2016, 8:30 p.m. UTC | #11
Hi Roel!

I realized we have not applied this --list-generations patch of yours,
which is a pity.  So…

Roel Janssen <roel@gnu.org> skribis:

> Vincent Legoll writes:
>
>> On Thu, Sep 1, 2016 at 2:12 PM, Ludovic Courtès <ludo@gnu.org> wrote:
>>>>>>>   --list-generations=full
>>>>
>>>> I think keeping a way to see all generations in one go would be cool
>>>
>>> --list-generations would still display all the generations.  Or did you
>>> mean “a way to see *the content* of each generation”?
>>
>> I'm not sure to understand what you mean by "the content", what I meant
>> was to keep what current guix package -l shows us, i.e. do not alter current
>> UI, only add new things, but do not break existing practices when not mandated.
>
> What about adding a `--no-diff-format' or a `--diff-format' argument to
> `--list-generations'?
>
> We could then make two choices:  Default to the proposed diff format, or
> default to the full format (what we have now).  Depending on what we
> choose as default, we can implement the appropriate negating argument.
>
> I would prefer defaulting to the diff format..

In the discussion that ensued, it seems there was a consensus to provide
only the diff format:

  https://lists.gnu.org/archive/html/guix-devel/2016-09/msg00385.html

So it seems all the lights are green.  :-)  Let me know if there’s
anything we should do to help!

Thanks,
Ludo’.
Roel Janssen Oct. 11, 2016, 7:19 a.m. UTC | #12
Ludovic Courtès writes:

> Hi Roel!
>
> I realized we have not applied this --list-generations patch of yours,
> which is a pity.  So…
>
> Roel Janssen <roel@gnu.org> skribis:
>
>> Vincent Legoll writes:
>>
>>> On Thu, Sep 1, 2016 at 2:12 PM, Ludovic Courtès <ludo@gnu.org> wrote:
>>>>>>>>   --list-generations=full
>>>>>
>>>>> I think keeping a way to see all generations in one go would be cool
>>>>
>>>> --list-generations would still display all the generations.  Or did you
>>>> mean “a way to see *the content* of each generation”?
>>>
>>> I'm not sure to understand what you mean by "the content", what I meant
>>> was to keep what current guix package -l shows us, i.e. do not alter current
>>> UI, only add new things, but do not break existing practices when not mandated.
>>
>> What about adding a `--no-diff-format' or a `--diff-format' argument to
>> `--list-generations'?
>>
>> We could then make two choices:  Default to the proposed diff format, or
>> default to the full format (what we have now).  Depending on what we
>> choose as default, we can implement the appropriate negating argument.
>>
>> I would prefer defaulting to the diff format..
>
> In the discussion that ensued, it seems there was a consensus to provide
> only the diff format:
>
>   https://lists.gnu.org/archive/html/guix-devel/2016-09/msg00385.html
>
> So it seems all the lights are green.  :-)  Let me know if there’s
> anything we should do to help!

Thanks for the reminder.  I seem to have a lot to finish up :-).  I will
work out the arguments and post a new version of the patch for final review.

Kind regards,
Roel Janssen
Roel Janssen Oct. 18, 2016, 12:43 p.m. UTC | #13
Roel Janssen writes:

> Ludovic Courtès writes:
>
>> Hi Roel!
>>
>> I realized we have not applied this --list-generations patch of yours,
>> which is a pity.  So…
>>
>> Roel Janssen <roel@gnu.org> skribis:
>>
>>> Vincent Legoll writes:
>>>
>>>> On Thu, Sep 1, 2016 at 2:12 PM, Ludovic Courtès <ludo@gnu.org> wrote:
>>>>>>>>>   --list-generations=full
>>>>>>
>>>>>> I think keeping a way to see all generations in one go would be cool
>>>>>
>>>>> --list-generations would still display all the generations.  Or did you
>>>>> mean “a way to see *the content* of each generation”?
>>>>
>>>> I'm not sure to understand what you mean by "the content", what I meant
>>>> was to keep what current guix package -l shows us, i.e. do not alter current
>>>> UI, only add new things, but do not break existing practices when not mandated.
>>>
>>> What about adding a `--no-diff-format' or a `--diff-format' argument to
>>> `--list-generations'?
>>>
>>> We could then make two choices:  Default to the proposed diff format, or
>>> default to the full format (what we have now).  Depending on what we
>>> choose as default, we can implement the appropriate negating argument.
>>>
>>> I would prefer defaulting to the diff format..
>>
>> In the discussion that ensued, it seems there was a consensus to provide
>> only the diff format:
>>
>>   https://lists.gnu.org/archive/html/guix-devel/2016-09/msg00385.html
>>
>> So it seems all the lights are green.  :-)  Let me know if there’s
>> anything we should do to help!
>
> Thanks for the reminder.  I seem to have a lot to finish up :-).  I will
> work out the arguments and post a new version of the patch for final review.

My GNU mail hasn't been working for a couple of days, but now that it
does, I wonder if the initial patch I sent is fine as-is, since we agree
upon only showing the diff format.

Should I implement the --no-diff-format option to change to the
old behavior?

Kind regards,
Roel Janssen
Ludovic Courtès Oct. 19, 2016, 8:22 p.m. UTC | #14
Roel Janssen <roel@gnu.org> skribis:

> Roel Janssen writes:
>
>> Ludovic Courtès writes:

[...]

>>> In the discussion that ensued, it seems there was a consensus to provide
>>> only the diff format:
>>>
>>>   https://lists.gnu.org/archive/html/guix-devel/2016-09/msg00385.html
>>>
>>> So it seems all the lights are green.  :-)  Let me know if there’s
>>> anything we should do to help!
>>
>> Thanks for the reminder.  I seem to have a lot to finish up :-).  I will
>> work out the arguments and post a new version of the patch for final review.
>
> My GNU mail hasn't been working for a couple of days, but now that it
> does, I wonder if the initial patch I sent is fine as-is, since we agree
> upon only showing the diff format.
>
> Should I implement the --no-diff-format option to change to the
> old behavior?

I don’t think so.  The old behavior will always be available by
specifying a single generation anyway:

  guix package --list-generations=42

It may be worth mentioning this in the manual.

My understanding of the discussion referred above is that people were
fine with that (or indifferent ;-)).

Thanks,
Ludo’.
diff mbox

Patch

From 1ea5eaae4b492c82358c7394c22cd46497388449 Mon Sep 17 00:00:00 2001
From: Roel Janssen <roel@gnu.org>
Date: Sun, 28 Aug 2016 00:54:06 +0200
Subject: [PATCH] guix package: Display generation diffs.

---
 guix/scripts/package.scm |  2 +-
 guix/ui.scm              | 34 ++++++++++++++++++++++++++++++++++
 2 files changed, 35 insertions(+), 1 deletion(-)

diff --git a/guix/scripts/package.scm b/guix/scripts/package.scm
index 2a751a4..32cbcdc 100644
--- a/guix/scripts/package.scm
+++ b/guix/scripts/package.scm
@@ -640,7 +640,7 @@  processed, #f otherwise."
        (define (list-generation number)
          (unless (zero? number)
            (display-generation profile number)
-           (display-profile-content profile number)
+           (display-profile-content-diff profile number)
            (newline)))
 
        (cond ((not (file-exists? profile))      ; XXX: race condition
diff --git a/guix/ui.scm b/guix/ui.scm
index 906b349..cb056a0 100644
--- a/guix/ui.scm
+++ b/guix/ui.scm
@@ -87,6 +87,7 @@ 
             matching-generations
             display-generation
             display-profile-content
+            display-profile-content-diff
             roll-back*
             switch-to-generation*
             delete-generation*
@@ -1070,6 +1071,39 @@  DURATION-RELATION with the current time."
           (format #t (_ "~a\t(current)~%") header)
           (format #t "~a~%" header)))))
 
+(define (display-profile-content-diff profile number)
+  "Display the changed packages in PROFILE with generation specified by 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))
+  
+  (define (display-profile-content-diff-between profile older newer)
+    (if (= older 0)
+        (display-profile-content profile newer)
+        (let* ((old (profile-manifest (generation-file-name profile older)))
+               (new (profile-manifest (generation-file-name profile newer)))
+               (installed (lset-difference equal-entry?
+                                           (manifest-entries new)
+                                           (manifest-entries old)))
+               (removed (lset-difference equal-entry?
+                                         (manifest-entries old)
+                                         (manifest-entries new))))
+
+          (display-entries installed "+")
+          (display-entries removed "-"))))
+
+  (let ((previous (previous-generation-number profile number)))
+    (display-profile-content-diff-between profile previous number)))
+
 (define (display-profile-content profile number)
   "Display the packages in PROFILE, generation NUMBER, in a human-readable
 way."
-- 
2.9.3