patchwdiff - Patches: patch #7121, New, per-character diff, mode

 
 

You are not allowed to post comments on this tracker with your current authentication level.

patch #7121: New, per-character diff, mode

Submitter:  Georgios Zarkadas <gzarkadas>
Submitted:  Sun 14 Mar 2010 02:05:07 PM UTC
   
 
Category:  None Priority:  5 - Normal
Status:  In Progress Privacy:  Public
Assigned to:  gagern Open/Closed:  Open

Tue 30 Mar 2010 08:01:42 AM UTC, comment #4: 

-1- see personal e-mail.

-3-

> I have started studying other encodings, such as UTF-16,
> the unicode routines available by glibc


I assume that the most GNUish way to handle different encodings would be using libiconv <http://www.gnu.org/software/libiconv/>, which is also included in glibc. That way, you can formulate your app to deal with unicode only, and rely on libiconv for input processing. Downside is that there might be encodings which have different possible encodings for the same unicode string, in which case wdiff might loose some details about the binary representation. Shouldn't worry us, though, as long as things look the same. wdiff is more about looks than bytes, I think.

> Calling `setlocale(LC_CTYPE, NULL)' at program's startup
> to get the default encoding (plus a commandline option to
> supply it explicitly)


Most apps don't provide special command line switches, but instead accept customization of encodings via environment variables. I'd stick with this to avoid excessive numbers of command line switches. Unless you want to control encoding for both sides of a diff independently...

> Splitting of words/chars will need some thought


By the way: I believe you want to split glyphs, not chars, i.e. you want to keep combining characters together with their base characters. This requires even more understanding of unicode, and applying it to other encodings manually would probably be very painful, so conversions to unicode feel ever more sensible.

> Most probably it will require to change all getc/putc calls
> to the apropriate multi-byte versions.


libiconv at least works on byte buffers, not I/O streams. And it's probably slow if you pass one char at a time. Hmmm... I wonder whether we should switch from the one-char-at-a-time implementation to something processing larger buffers of bytes one at a time.

> Doing more elaborate things such as trully supporting the
> -i, --ignore-case option in all encodings will most probably
> require quite a lot of code (actually, I do not know yet how
> much).


The ICU library has some documents that might be useful here:
http://userguide.icu-project.org/transforms/casemappings
http://userguide.icu-project.org/collation/concepts

> Thus, a pass-through implementation (just to break words/chars
> right in any encoding) is feasible IMO in a few months;
> I will try it.


That sounds great. I guess it would be useful if you could share a bzr branch of your progress. You could do so either by applying for group membership here on savannah, or using launchpad, or any other publicly accessible bzr or http server.

> If you are aware of other GNU software that handles unicode
> point me to it; there may be suitable ready code to use for
> this purpose.


The only GNU thing I know about is libiconv. It does conversion, but knows little more about unicode.

The most powerful unicode library I've worked with so far is ICU, the International Components for Unicode, <http://site.icu-project.org/>. It seems pretty powerful, and also has some good documentation, to which I've already pointed you up there. One more pointer: the White_Space and Word_Break properties mentioned in http://userguide.icu-project.org/strings/properties might be useful.

ICU is no GNU project, and has over 17MiB of libraries on my setup. So it might be a useful and powerful support library, but wdiff should not depend on it, maybe not even for simple char splitting.

Looking at the ICU docs made me realize just how deep the waters are getting here. For example, if you do this correctly in a partially right-to-left script, you should ensure that unicode direction modifiers do nest properly. I assume things can become really complicated. I wouldn't expect you to address all this complexity before I consider your patch for inclusion, the word-wise diff doesn't do so either. But it might pay to think about these things, so the implementation is designed in such a way it can deal with them in the long run. Just a thought.

Martin von Gagern <gagern>
Group administrator
Mon 29 Mar 2010 11:04:41 PM UTC, comment #3: 

Hi,
My answers to the remarks follow, in the same order.

-1- Yes, to both questions.

-2- Ok, it fits with (-3-)'s time frame; we could also keep just the long option to avoid any ambiguity.

-3- Yes, it is; it was a quick hack in order to drive development of a tool for the trans-coord project, which uses wdiff for showing changes in fuzzy translations (see http://lists.gnu.org/archive/html/trans-coord-devel/2010-03/msg00014.html).
   However, it is fast, resets after an error to normal and stream preserving. If, as I chose, one wants to keep all bytes of the stream, inevitably on an input error it will spit something non-printable; but it is not wdiff's responsibility to validate the stream IMHO.
   This is not my final word, however; in order to arrive in a more general solution I have started studying other encodings, such as UTF-16, the unicode routines available by glibc and also I had a quick look in Coreutils' sources.
   I believe that the following apply:
   --- Calling `setlocale(LC_CTYPE, NULL)' at program's startup to get the default encoding (plus a commandline option to supply it explicitly) and branching to either single-byte or the appropriate multi-byte mode will be easy.
   --- Splitting of words/chars will need some thought in order not to loose too much in speed and handle errors gracefully, but it is rather straightforward; you just follow the selected encoding's rules. Most probably it will require to change all getc/putc calls to the apropriate multi-byte versions.
   --- Doing more elaborate things such as trully supporting the -i, --ignore-case option in all encodings will most probably require quite a lot of code (actually, I do not know yet how much).
   Thus, a pass-through implementation (just to break words/chars right in any encoding) is feasible IMO in a few months; I will try it. If you are aware of other GNU software that handles unicode point me to it; there may be suitable ready code to use for this purpose.

-4- It is better to postpone any such activity at this moment (cf. -3-)

Georgios Zarkadas <gzarkadas>
Mon 29 Mar 2010 04:16:17 PM UTC, comment #2: 

OK, I've now had a second look at your patch. The general idea seems sane enough. A few remarks, in order of diff lines:

  • Would it be all right to move your name from the source file to the AUTHORS file in the long run? Generally, the change is a significant one. Would you be willing to assign the copyright for your modifications to the Free Software Foundation, so that we can officially include it in future releases? If so, I'll send you suitable forms by e-mail.


  • The "c" short option is used by mdiff for context format, but as wdiff doesn't support context format, that should be all right. The introduction of a new flag means we'll need additional translations. Therefore I won't include this in any 0.6.x release, but rather target this towards 0.7.0.


  • The UTF-8 treatment is very simplistic. I'd either want it to check UTF-8 format more thoroughly, i.e. calculate number of bytes to expect and ensure that's what we get, or I'd want it enabled only in UTF-8 locales. The former has benefits when file encoding doesn't agree with default locale encoding, but prevents users from overriding this using environment variables. I guess getting things right in non-UTF multibyte encodings might be a lot harder, but I'd like to know just how hard.


  • Maybe we can avoid some duplication, e.g. between get_char and skip_char, or between copy_char and copy_word. Haven't tried to yet, so I'm not sure it would make sense.
Martin von Gagern <gagern>
Group administrator
Mon 29 Mar 2010 02:08:48 PM UTC, comment #1: 

Thank you for your patch, Georgios!

Unfortunately I havn't been informed by your submission immediately. That is fixed now, so future patches should get replies sooner. At least so I hope...

The wdiff code is accessible through bzr by specifying the branch name "trunk" as an additional directory within the shared repository, like this:
$ bzr branch http://bzr.savannah.gnu.org/r/wdiff/trunk

I've just applied your 0.5.95 patch (patch #7122) and merged it with the 0.6.0 release code. The result is already accessible here: http://bzr.savannah.gnu.org/r/wdiff/contrib/gzarkadas/single_char_mode

I haven't had a closer look at your patch yet, but hope I'll find time to examine it soon.

By the way, if -a doesn't work for you, as indicated in patch #7122, and it doesn't work in the 0.6.0 release either, please file a bug report, so we can have a closer look at that.

Martin von Gagern <gagern>
Group administrator
Sun 14 Mar 2010 02:05:07 PM UTC, original submission:  

I attach a diff from the 0.5 version of wdiff.c (latest stable release) that implements an additional per-character diff mode, taking into account only non-whitespace characters (just as the per-word diff mode does).

The code can handle ascii and utf-8 characters and (I believe, though it has to be better tested) can withstand encoding errors (ie the presence of invalid characters in the stream) in the sense that it will re-adjust to proper character separation after them.

I compiled it with CFLAGS: -std=c89 -Wall ; some extra pair of braces are to correct some of the warnings appeared.

I used this specific version because I didn't manage to get the latest sources with bazaar. I also wanted something that it could work immediately, to use it in a gettext-related tool under development for the trans-coord / www / www-xx projects.

I will start now to prepare a merge with the code of the latest alpha release (0.5.95), in order to submit a patch to the latest sources.



Georgios Zarkadas <gzarkadas>

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

Attached Files
file #19942:  wdiff_v0-5.diff added by gzarkadas (10KiB - text/x-patch - diff taken as: diff --normal new_file old_file)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by gagern (Posted a comment)
  • -email is unavailable- added by gzarkadas (Submitted the item)
  •  

    There are 0 votes so far. Votes easily highlight which items people would like to see resolved in priority, independently of the priority of the item set by tracker managers.

     

    Follow 4 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2010-03-30 gagern StatusNeed Info In Progress
    2010-03-29 gagern StatusNone Need Info
    2010-03-29 gagern Assigned toNone gagern
    2010-03-14 gzarkadas Attached File- Added wdiff_v0-5.diff, #19942

    Back to the top

    Powered by Savane 3.13-54b4.
    Corresponding source code