diff mbox

Display diffs between generations.

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

Commit Message

Roel Janssen Oct. 19, 2016, 9:48 p.m. UTC
Ludovic Courtès writes:

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

Then the following patch should be it.  I applied your genius
suggestions of using @code{display-entry} instead of
@code{display-entries}.  I learned something new again today. :-)

>From 5630a512f16c6677fd5b0b8085e2ef15cb10499b Mon Sep 17 00:00:00 2001
From: Roel Janssen <roel@gnu.org>
Date: Wed, 19 Oct 2016 23:38:11 +0200
Subject: [PATCH] guix package: Display generation diffs.

* guix/ui.scm (display-profile-content-diff): New variable.
* guix/scripts/package.scm (process-query): Use display-profile-content-diff.
---
 guix/scripts/package.scm |  2 +-
 guix/ui.scm              | 27 +++++++++++++++++++++++++++
 2 files changed, 28 insertions(+), 1 deletion(-)

Comments

Ludovic Courtès Oct. 20, 2016, 12:36 p.m. UTC | #1
Roel Janssen <roel@gnu.org> skribis:

> Ludovic Courtès writes:

[...]

>> 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 ;-)).
>
> Then the following patch should be it.  I applied your genius
> suggestions of using @code{display-entry} instead of
> @code{display-entries}.  I learned something new again today. :-)

Heh.  :-)

>>From 5630a512f16c6677fd5b0b8085e2ef15cb10499b Mon Sep 17 00:00:00 2001
> From: Roel Janssen <roel@gnu.org>
> Date: Wed, 19 Oct 2016 23:38:11 +0200
> Subject: [PATCH] guix package: Display generation diffs.
>
> * guix/ui.scm (display-profile-content-diff): New variable.
> * guix/scripts/package.scm (process-query): Use display-profile-content-diff.

The code LGTM, but it doesn’t implement the behavior I suggested above
for --list-generations=42, which I think I didn’t explain correctly.

I think what I’m suggesting is that the first matching generation should
be displayed in full using ‘display-profile-content’, and subsequent
generations would be displayed as a diff.

Thus, when viewing a single generation, as with --list-generations=42,
we’d only display the full contents of that generation.

Does that make sense?

Sorry for not clarifying this in the first place!  I guess that wasn’t
entirely clear to me either.  ;-)

Thanks,
Ludo’.
Roel Janssen Oct. 20, 2016, 12:56 p.m. UTC | #2
Ludovic Courtès writes:

> Roel Janssen <roel@gnu.org> skribis:
>
>> Ludovic Courtès writes:
>
> [...]
>
>>> 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 ;-)).
>>
>> Then the following patch should be it.  I applied your genius
>> suggestions of using @code{display-entry} instead of
>> @code{display-entries}.  I learned something new again today. :-)
>
> Heh.  :-)
>
>>>From 5630a512f16c6677fd5b0b8085e2ef15cb10499b Mon Sep 17 00:00:00 2001
>> From: Roel Janssen <roel@gnu.org>
>> Date: Wed, 19 Oct 2016 23:38:11 +0200
>> Subject: [PATCH] guix package: Display generation diffs.
>>
>> * guix/ui.scm (display-profile-content-diff): New variable.
>> * guix/scripts/package.scm (process-query): Use display-profile-content-diff.
>
> The code LGTM, but it doesn’t implement the behavior I suggested above
> for --list-generations=42, which I think I didn’t explain correctly.
>
> I think what I’m suggesting is that the first matching generation should
> be displayed in full using ‘display-profile-content’, and subsequent
> generations would be displayed as a diff.
>
> Thus, when viewing a single generation, as with --list-generations=42,
> we’d only display the full contents of that generation.
>
> Does that make sense?

Ah, sorry, I forgot about this.  This makes sense.  But then, what should we
see when we do:

  guix package --list-generations=42,46

Should we see:
  Generation 42 ...:
  <full contents>

  Generation 46 ...:
  <diff with 42>

Or should we then see:
  Generation 42 ...:
  <full contents>

  Generation 46 ...:
  <full contents>

Kind regards,
Roel Janssen
Ludovic Courtès Oct. 20, 2016, 7:38 p.m. UTC | #3
Roel Janssen <roel@gnu.org> skribis:

> Ludovic Courtès writes:

[...]

>> I think what I’m suggesting is that the first matching generation should
>> be displayed in full using ‘display-profile-content’, and subsequent
>> generations would be displayed as a diff.
>>
>> Thus, when viewing a single generation, as with --list-generations=42,
>> we’d only display the full contents of that generation.
>>
>> Does that make sense?
>
> Ah, sorry, I forgot about this.  This makes sense.  But then, what should we
> see when we do:
>
>   guix package --list-generations=42,46
>
> Should we see:
>   Generation 42 ...:
>   <full contents>
>
>   Generation 46 ...:
>   <diff with 42>

This one, IMO.

Ludo’.
diff mbox

Patch

diff --git a/guix/scripts/package.scm b/guix/scripts/package.scm
index b87aee0..5d93b7b 100644
--- a/guix/scripts/package.scm
+++ b/guix/scripts/package.scm
@@ -670,7 +670,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 eb85df3..7c835c2 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,32 @@  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 compared to generation NUMBER."
+
+  (define (equal-entry? first second)
+    (string= (manifest-entry-item first) (manifest-entry-item second)))
+
+  (define (display-entry entry prefix)
+    (match entry
+      (($ <manifest-entry> name version output location _)
+       (format #t " ~a ~a\t~a\t~a\t~a~%" prefix name version output location))))
+
+  (define (list-entries input)
+    (manifest-entries (profile-manifest (generation-file-name profile input))))
+
+  (define (display-diff profile old new)
+    (if (= old 0)
+        (display-profile-content profile new)
+        (let ((added (lset-difference
+                      equal-entry? (list-entries new) (list-entries old)))
+              (removed (lset-difference
+                        equal-entry? (list-entries old) (list-entries new))))
+          (for-each (cut display-entry <> "+") added)
+          (for-each (cut display-entry <> "-") removed))))
+
+  (display-diff profile (previous-generation-number profile number) number))
+
 (define (display-profile-content profile number)
   "Display the packages in PROFILE, generation NUMBER, in a human-readable
 way."