[2/2] alpm version diff coloring

Message ID 20220301050212.37576-2-david.e.gustavsson@gmail.com
State Under Review
Headers show
Series [1/2] Diff color version strings | expand

Commit Message

David Gustavsson March 1, 2022, 5:02 a.m. UTC
Use non-alphanumerics as branching points for diff coloring.

Signed-off-by: David Gustavsson <david.e.gustavsson@gmail.com>
---
 src/pacman/util.c | 83 +++++++++++++++++++++++++++++++++--------------
 src/pacman/util.h |  1 +
 2 files changed, 60 insertions(+), 24 deletions(-)

Comments

Allan McRae July 31, 2022, 5:05 a.m. UTC | #1
On 1/3/22 15:02, David Gustavsson wrote:
> Use non-alphanumerics as branching points for diff coloring.
> 
> Signed-off-by: David Gustavsson <david.e.gustavsson@gmail.com>
> ---

There is a memory leak somewhere in here:

$ sudo ./build/pacman -Su
:: Starting full system upgrade...
resolving dependencies...
looking for conflicting packages...
free(): invalid next size (fast)
Aborted


I have not looked for it yet, because I am still deciding the utility of 
these patches.


For discussion, consider these versions:

5.18.14.arch1-1
5.18.15.arch1-1

With the first patch in this series applied (have not looked to see if 
this changes with the second), this starts highlighting after "5.18.1". 
  But if we were going from 5.18.9 to 5.18.10, it would highlight after 
the "5.18.".  These upgrades are no different in that versioning scheme. 
  This should definitely always highlight from the version level separator.


Also, I personally do not find this colouring useful, but I also do not 
find the verbose package lists before update useful...  So I am open to 
opinions on this.  Here is an example of the output after the patch 1/2 
applied:
http://allanmcrae.com/tmp/pacman-version-change-highlight.png

I'll wait for more feedback before looking into this further.

Allan
David Gustavsson Aug. 18, 2022, 2:35 p.m. UTC | #2
The memory leak was an off-by-one-error, I can't reproduce it after
removing some needless pointer arithmetic.
Yes, I had some constructive pointers about semantic diffing between the
two previous patches.
The attached patch looks like this:
[image: 2022-08-18T155817.png]
Your examples would go
5.18.*14.arch1-1*       5.18.*15arch1-1*
5.18.*9*                      5.18.*10*

To me, this is very useful. Especially when there's many rows, it helps
when figuring out which release notes to read after an upgrade.
Disabling `color` or `verbose` of course removes this.



Den sön 31 juli 2022 kl 07:05 skrev Allan McRae <allan@archlinux.org>:

> On 1/3/22 15:02, David Gustavsson wrote:
> > Use non-alphanumerics as branching points for diff coloring.
> >
> > Signed-off-by: David Gustavsson <david.e.gustavsson@gmail.com>
> > ---
>
> There is a memory leak somewhere in here:
>
> $ sudo ./build/pacman -Su
> :: Starting full system upgrade...
> resolving dependencies...
> looking for conflicting packages...
> free(): invalid next size (fast)
> Aborted
>
>
> I have not looked for it yet, because I am still deciding the utility of
> these patches.
>
>
> For discussion, consider these versions:
>
> 5.18.14.arch1-1
> 5.18.15.arch1-1
>
> With the first patch in this series applied (have not looked to see if
> this changes with the second), this starts highlighting after "5.18.1".
>   But if we were going from 5.18.9 to 5.18.10, it would highlight after
> the "5.18.".  These upgrades are no different in that versioning scheme.
>   This should definitely always highlight from the version level separator.
>
>
> Also, I personally do not find this colouring useful, but I also do not
> find the verbose package lists before update useful...  So I am open to
> opinions on this.  Here is an example of the output after the patch 1/2
> applied:
> http://allanmcrae.com/tmp/pacman-version-change-highlight.png
>
> I'll wait for more feedback before looking into this further.
>
> Allan
>
>
>
>
>

Patch

diff --git a/src/pacman/util.c b/src/pacman/util.c
index 10d7c573..242af8b2 100644
--- a/src/pacman/util.c
+++ b/src/pacman/util.c
@@ -34,6 +34,7 @@ 
 #include <limits.h>
 #include <wchar.h>
 #include <wctype.h>
+#include <ctype.h>
 #ifdef HAVE_TERMIOS_H
 #include <termios.h> /* tcflush */
 #endif
@@ -840,10 +841,13 @@  static alpm_list_t *create_verbose_row(pm_target_t *target)
 {
 	char *str;
 	off_t size = 0;
-  size_t i = 0;
+	int split;
 	double human_size;
 	const char *label;
-  const char *version;
+  const char *old_version_in;
+	const char *new_version_in;
+	char *old_version;
+	char *new_version;
 	alpm_list_t *ret = NULL;
 
 	/* a row consists of the package name, */
@@ -859,24 +863,29 @@  static alpm_list_t *create_verbose_row(pm_target_t *target)
 	}
 	add_table_cell(&ret, str, CELL_NORMAL | CELL_FREE);
 
+
 	/* old and new versions */
-  version = alpm_pkg_get_version(target->remove);
-  char old_version[strlen(version) + strlen(config->colstr.diffrem) + strlen(config->colstr.nocolor) + 1];
-  memcpy(old_version, (char *)version, strlen(version) + 1);
-  version = alpm_pkg_get_version(target->install);
-  char new_version[strlen(version) + strlen(config->colstr.diffadd) + strlen(config->colstr.nocolor) + 1];
-  memcpy(new_version, (char *)version, strlen(version) + 1);
-
-  i = 0;
-  while (old_version[i] != '\0' && new_version[i] != '\0' && old_version[i] == new_version[i]) {
-    i++;
-  }
-  insertstring(old_version, config->colstr.diffrem, i);
-  insertstring(new_version, config->colstr.diffadd, i);
-  snprintf(old_version + i + strlen(old_version + i),
-      strlen(config->colstr.nocolor) + 1, "%s", config->colstr.nocolor);
-  snprintf(new_version + i + strlen(new_version + i),
-      strlen(config->colstr.nocolor) + 1, "%s", config->colstr.nocolor);
+	if (target->remove) {
+		old_version_in = alpm_pkg_get_version(target->remove);
+	} else {
+		old_version_in = "";
+	}
+  old_version = malloc((strlen(old_version_in) + strlen(config->colstr.diffrem) + strlen(config->colstr.nocolor) + 1)*sizeof(char));
+  memcpy(old_version, (char *)old_version_in, strlen(old_version_in) + 1);
+
+	if (target->install) {
+		new_version_in = alpm_pkg_get_version(target->install);
+	} else {
+		new_version_in = "";
+	}
+  new_version = malloc((strlen(new_version_in) + strlen(config->colstr.diffadd) + strlen(config->colstr.nocolor) + 1)*sizeof(char));
+  memcpy(new_version, (char *)new_version_in, strlen(new_version_in) + 1);
+
+	split = alpm_diff(old_version_in, new_version_in);
+  insertstring(old_version, config->colstr.diffrem, split);
+	insertstring(old_version, config->colstr.nocolor, strlen(old_version));
+  insertstring(new_version, config->colstr.diffadd, split);
+	insertstring(new_version, config->colstr.nocolor, strlen(new_version));
 
   pm_asprintf(&str, "%s", old_version);
 	add_table_cell(&ret, str, CELL_NORMAL | CELL_FREE);
@@ -884,6 +893,9 @@  static alpm_list_t *create_verbose_row(pm_target_t *target)
   pm_asprintf(&str, "%s", new_version);
 	add_table_cell(&ret, str, CELL_NORMAL | CELL_FREE);
 
+	free(old_version);
+	free(new_version);
+
 	/* and size */
 	size -= target->remove ? alpm_pkg_get_isize(target->remove) : 0;
 	size += target->install ? alpm_pkg_get_isize(target->install) : 0;
@@ -1878,11 +1890,34 @@  void console_erase_line(void)
 	printf("\x1B[K");
 }
 
-/* inserts string insert into string at index i, modifying string */
+/* inserts string insert into string after index i, modifying string */
 void insertstring(char *string, const char *insert, size_t i)
 {
-  char temp[strlen(string) - i + 1];
-  memcpy(temp, string + i, strlen(string + i) + 1);
-  memcpy(string + i, insert, strlen(insert) + 1);
-  memcpy(string + i + strlen(insert), temp, strlen(temp) + 1);
+  char *temp = malloc((strlen(string) - i)*sizeof(char));
+  memcpy(temp, string + i + 1, strlen(string + i));
+  memcpy(string + i + 1, insert, strlen(insert) + 1);
+  memcpy(string + i + strlen(insert) + 1, temp, strlen(temp) + 1);
+	free(temp);
+}
+
+int alpm_diff(const char *a, const char *b)
+{
+	/*
+	 * Find the index of the last non-alphanumeric character
+	 * before `a` and `b` differ
+	 */
+	int i = 0;
+	int split = -1;
+	while(a[i] == b[i]) {
+		if(a[i] == '\0') {
+			split = i;
+			return split;
+		}
+		if(isalnum(a[i]) == 0) {
+			/* Non-alphanumeric character */
+			split = i;
+		}
+		i++;
+	}
+	return split;
 }
diff --git a/src/pacman/util.h b/src/pacman/util.h
index 003961eb..afac74c1 100644
--- a/src/pacman/util.h
+++ b/src/pacman/util.h
@@ -89,6 +89,7 @@  void console_cursor_move_end(void);
 /* Erases line from the current cursor position till the end of the line */
 void console_erase_line(void);
 void insertstring(char *string, const char *insert, size_t i);
+int alpm_diff(const char *a, const char *b);
 
 int pm_printf(alpm_loglevel_t level, const char *format, ...) __attribute__((format(printf,2,3)));
 int pm_asprintf(char **string, const char *format, ...) __attribute__((format(printf,2,3)));