Bug#817982: ucf: diff screen is ugly, unreadable and counterintuitive

[Paul Gevers]

Hi Robert

[Please, next time, don't use CC directly, but use reportbug CC option
to handle the CC (something with X-Debug-CC or similar). Now I had to
figure out what the bug number was.]

Looking at your changelog and patch description, it seems that you found
a regression. Why did you not make that more explicit by filling a
separate bug?

+++ b/debian/changelog
> As a side effect, fix a bug introduced in
> 3.0034 that caused ucf to fail on not yet existing destination files.

I think I already saw issues with this in Launchpad bug 1553757¹,
although I am not sure under which conditions this actually happens
(just deleting the file didn't seem to do the trick, or did I not test
correctly).

Paul

¹ https://bugs.launchpad.net/ubuntu/+source/dbconfig-common/+bug/1553757



signature.asc
Description: OpenPGP digital signature

Bug#817982: ucf: diff screen is ugly, unreadable and counterintuitive

[Robert Luberda]

tags 817982 patch
thanks

Manoj Srivastava pisze:
> severity 817982 wishlist

To be honest I was even considering filling my initial bug as serious,
but most probably the "least surprising principle" is not mentioned in
our policy in any way.

> 
> The line by line differences are still present, and always had
>  metadata (file, line numbers). The metadata has been expanded, and
>  clearly delineated from the content differences.

But now the metadata takes more space than the actual differences, and
consists of information that is:
- pretty much useless (devices, inodes,  or everything when
ownership/permissions are same),
- duplicated (timestamp is printed also in the ---/+++ lines of diff).


> I don’t see extra information as a bug; and clearly, our
>  opinions on the  aesthetics differ.

Because of its size and the way it is presented, the metadata is no
longer an extra information, but it is the main piece of information,
that hides the actual file differences somewhere below - for that reason
I would consider this as a bug.

> 
> As long as the information is present I have no objection to a
>  more compact display. Patches welcome.

Patch attached together with three screenshots showing the new output.

Regards,
robert

From 4d49cb249b7905c028d13e0c029e77a9400972a7 Mon Sep 17 00:00:00 2001
From: Robert Luberda 
Date: Sun, 13 Mar 2016 14:42:55 +0100
Subject: [PATCH] Use compact format for metadata differences

Display the file permissions and ownerships information either in diff
label lines or in additional lines prepended to sdiff output not to
hide the actuall file differences by the whole output of /usr/bin/stat
command (Closes: #817982). As a side effect, fix a bug introduced in
3.0034 that caused ucf to fail on not yet existing destination files.
---
 debian/changelog | 10 ++
 ucf  | 92 +---
 2 files changed, 64 insertions(+), 38 deletions(-)

diff --git a/debian/changelog b/debian/changelog
index 5538fcf..3274f77 100644
--- a/debian/changelog
+++ b/debian/changelog
@@ -1,3 +1,13 @@
+ucf (3.0035+local1) UNRELEASED; urgency=medium
+
+  * Display the file permissions and ownerships information either in diff
+label lines or in additional lines prepended to sdiff output not to
+hide the actuall file differences by the whole output of /usr/bin/stat
+command (Closes: #817982). As a side effect, fix a bug introduced in
+3.0034 that caused ucf to fail on not yet existing destination files.
+
+ -- Robert Luberda   Sun, 13 Mar 2016 14:38:46 +0100
+
 ucf (3.0035) unstable; urgency=low
 
   * Minor typo fixing release.
diff --git a/ucf b/ucf
index bfb558a..cbbeada 100755
--- a/ucf
+++ b/ucf
@@ -57,22 +57,52 @@ setq() {
 fi
 }
 
-# Use debconf to show the differences
-sep='
-##
-The existing file stats
-'
-sep2='
---
-New file stats
-'
-sep3='
-##
-'
-endline='
+# Usage: get_file_metadate file_name
+get_file_metadata()
+{
+if [ -e "$1" ]; then
+# get file modification date without the nanoseconds and timezone info
+local moddate="$(date +"%F %T" --date $(stat --format '@%Y' "$1"))"
+# print file_name user.group permissions above_date
+stat --format "%n %U.%G 0%a $moddate" "$1"
+else
+echo "/dev/null"
+fi
+}
+
+# Runs the diff command with approrpiate arguments
+# Usage run_diff diff|sdiff diff_opts old_file new_file
+run_diff()
+{
+local diff_cmd="$1"
+local diff_opt="$2"
+local old_file="$3"
+local new_file="$4"
+
+# Note: get_file_metadata not in quotes to ignore "\n" characters
+local old_file_label=$(get_file_metadata "$old_file")
+local new_file_label=$(get_file_metadata "$new_file")
+
+[ -e "$old_file" ] || old_file=/dev/null
+[ -e "$new_file" ] || new_file=/dev/null
+
+if [ "$diff_cmd" = "diff" ] ; then
+  diff "$diff_opt" --label "$old_file_label" "$old_file" \
+--label "$new_file_label" "$new_file" || true
+elif [ "$diff_cmd" = "sdiff" ] ; then
+  # unfortunatelly the sdiff command does not support --label option
+  local out="$(sdiff "$diff_opt" "$old_file" "$new_file")" || true
+  [ -z "$out" ] || printf "Old file: %s\nNew file: %s\n\n%s" \
+   "$old_file_label" "$new_file_label" "$out"
+else
+  echo "Unknown diff command: $diff_cmd" >&2
+  exit 1
+fi
+}
 
-The actual differences between the files follow
-'
+
+# Use debconf to show the differences
+# Usage: show_diff actual_file_differences file_stat_differences
 show_diff() {
 if [ -z "$1" ]; then
 	DIFF="There are no non-white space differences in the files."
@@ -86,7 +116,7 @@ show_diff() {
 if [ "$DEBCONF_OK" = "YES" ] && [ "$DEBIAN_HAS_FRONTEND" ]; then
 	t

Bug#817982: ucf: diff screen is ugly, unreadable and counterintuitive

[Manoj Srivastava]

severity 817982 wishlist
thanks

On Sat, Mar 12 2016, Robert Luberda wrote:

> Package: ucf
> Version: 3.0035
> Severity: important
>
> The `show differences between versions' screen shows some stat
> informations that take the whole first screen of output - see 
> the attached screenshot.png - which is very strange.

I can see why the extra information can be surprising. 

> I've already  noticed that the actual file differences are shown 
> after scrolling down the screen, but my first impression when I 
> saw the initial screen was `What the ... is this??? Why it shows 
> some devices, inodes, sizes, timestamps, etc on a screen called
> "Line by line differences between versions"?' 

The line by line differences are still present, and always had
 metadata (file, line numbers). The metadata has been expanded, and
 clearly delineated from the content differences.

I don’t see extra information as a bug; and clearly, our
 opinions on the  aesthetics differ.

> I guess this has been done because of #812321, whose submitter asked for 
> `differences between the old and new file for permissions and ownership',
> (which actually would makes sense to me as well), but I don't think he
> expected to get 20 lines of some strange output to analyze to spot any 
> actual differences by hand.


> In my opinion the feature requested in #812321 could be better implemented 
> in several ways, for example:
>
> 1. By showing two lines only, something like:
>  Old file mode: root.root 0644
>  New file mode: root.root 0755
> if there is actual difference.
>
> 2. In a diff --git way, e.g.:
> diff --git a/makefile b/makefile
>  old mode 100644
>  new mode 100755
>  index 5479b48..fe2a8ae
>  --- a/makefile
>  +++ b/makefile
>  @@ -1,3 +1,4 @@
>  +# test
>   MKDIR = mkdir -p
> The "old/new mode" lines can be extended to display ownerships, or
> alternatively it could be displayed in additional "old/new owner/group"
> lines. Obviously as in the point 1, the lines should be displayed only
> if they are different.
>
> 3. Or owner/permissions could  be shown instead of timestamps in diff 
> headers, 
> i.e. replace
>
>  --- /etc/apt/listchanges.conf 2016-03-12 10:57:25.0 +0100
>  +++ /etc/apt/listchanges.conf.new 2016-03-12 11:01:58.0 +0100
> with
>  --- /etc/apt/listchanges.conf root.root 0644
>  +++ /etc/apt/listchanges.conf.new root.root 0644
>
> (In my opinion this would be the best approach, however the drawback of
> it is that some people might consider timestamps, especially the old one, 
> as a valuable piece of information in some cases).

As long as the information is present I have no objection to a
 more compact display. Patches welcome.

manoj
-- 
Life is like a tin of sardines.  We're, all of us, looking for the
key. Beyond the Fringe
Manoj Srivastava    
4096R/C5779A1C E37E 5EC5 2A01 DA25 AD20  05B6 CF48 9438 C577 9A1C


smime.p7s
Description: S/MIME cryptographic signature

Bug#817982: ucf: diff screen is ugly, unreadable and counterintuitive

[Robert Luberda]

Package: ucf
Version: 3.0035
Severity: important

The `show differences between versions' screen shows some stat
informations that take the whole first screen of output - see 
the attached screenshot.png - which is very strange.

I've already  noticed that the actual file differences are shown 
after scrolling down the screen, but my first impression when I 
saw the initial screen was `What the ... is this??? Why it shows 
some devices, inodes, sizes, timestamps, etc on a screen called
"Line by line differences between versions"?' 


I guess this has been done because of #812321, whose submitter asked for 
`differences between the old and new file for permissions and ownership',
(which actually would makes sense to me as well), but I don't think he
expected to get 20 lines of some strange output to analyze to spot any 
actual differences by hand.


In my opinion the feature requested in #812321 could be better implemented 
in several ways, for example:

1. By showing two lines only, something like:
 Old file mode: root.root 0644
 New file mode: root.root 0755
if there is actual difference.

2. In a diff --git way, e.g.:
diff --git a/makefile b/makefile
 old mode 100644
 new mode 100755
 index 5479b48..fe2a8ae
 --- a/makefile
 +++ b/makefile
 @@ -1,3 +1,4 @@
 +# test
  MKDIR = mkdir -p
The "old/new mode" lines can be extended to display ownerships, or
alternatively it could be displayed in additional "old/new owner/group"
lines. Obviously as in the point 1, the lines should be displayed only
if they are different.

3. Or owner/permissions could  be shown instead of timestamps in diff headers, 
i.e. replace

 --- /etc/apt/listchanges.conf 2016-03-12 10:57:25.0 +0100
 +++ /etc/apt/listchanges.conf.new 2016-03-12 11:01:58.0 +0100
with
 --- /etc/apt/listchanges.conf root.root 0644
 +++ /etc/apt/listchanges.conf.new root.root 0644

(In my opinion this would be the best approach, however the drawback of
it is that some people might consider timestamps, especially the old one, 
as a valuable piece of information in some cases).



Regards,
robert

-- System Information:
Debian Release: stretch/sid
  APT prefers unstable
  APT policy: (990, 'unstable'), (200, 'testing')
Architecture: i386 (i686)

Kernel: Linux 4.4.0-1-686-pae (SMP w/1 CPU core)
Locale: LANG=pl_PL.UTF-8, LC_CTYPE=pl_PL.UTF-8 (charmap=UTF-8)
Shell: /bin/sh linked to /bin/dash
Init: sysvinit (via /sbin/init)

Versions of packages ucf depends on:
ii  coreutils  8.25-2
ii  debconf1.5.59

ucf recommends no packages.

ucf suggests no packages.

-- Configuration Files:
/etc/ucf.conf changed:


-- debconf information:
* ucf/changeprompt: install_new
* ucf/show_diff:
* ucf/changeprompt_threeway: install_new
* ucf/conflicts_found:
  ucf/title: