Unified diff with contextall

[anonymous]

The changeset view (example 3: changeset:16824) allows to "Show the changes in full context" via ?contextall=1 (example 2: changeset:16824?contextall=1).

It also has a link to "Download in other formats: Unified Diff" (example 3: changeset:16824?format=diff&new=16824).

But combining these two features is not currently possible. On the example 2 page the contextall=1 parameter is not included in the Unified Diff link. And even if it is manually added it does not seem to have any effect (example 4: changeset:16824?format=diff&new=16824&contextall=1).

Would it be possible to make this work? This is very useful for people that prefer using an offline diff viewer tool.

[Jun Omae]

Currently, Unified diff link in Download in other formats doesn't respect all options of prefs form in changeset view (e.g. Blank lines, …).

I think we should keep it but it might be a bit nice to show another unified diff link respected the options.

[anonymous]

trac/versioncontrol/web_ui/changeset.py

@@ -349 +349 @@
         self._render_html(req, repos, chgset, restricted, data)
 
         if chgset:
-            diff_params = 'new=%s' % new
+            diff_params = {'new': new}
         else:
-            diff_params = unicode_urlencode({
+            diff_params = {
                 'new_path': full_new_path, 'new': new,
-                'old_path': full_old_path, 'old': old})
-        add_link(req, 'alternate', '?format=diff&' + diff_params,
+                'old_path': full_old_path, 'old': old}
+        diff_params_without_opts = unicode_urlencode(diff_params)
+        diff_params.update(diff_opts)
+        diff_params_with_opts = unicode_urlencode(diff_params)
+        add_link(req, 'alternate', '?format=diff&' + diff_params_with_opts,
                  _('Unified Diff'), 'text/plain', 'diff')
-        add_link(req, 'alternate', '?format=zip&' + diff_params,
+        if diff_params_with_opts != diff_params_without_opts:
+            add_link(req, 'alternate', '?format=diff&' + diff_params_without_opts,
+                     _('Plain Unified Diff'), 'text/plain', 'diff')
+        add_link(req, 'alternate', '?format=zip&' + diff_params_without_opts,
                  _('Zip Archive'), 'application/zip', 'zip')
         add_script(req, 'common/js/diff.js')
         add_stylesheet(req, 'common/css/changeset.css')
@@ -747 +753 @@
                 options = data['diff']['options']
                 context = options.get('contextlines', 3)
                 if context < 0 or options.get('contextall'):
-                    context = 3  # FIXME: unified_diff bugs with context=None
+                    context = None
                 ignore_blank_lines = options.get('ignoreblanklines')
                 ignore_case = options.get('ignorecase')
                 ignore_space = options.get('ignorewhitespace')

This seems to work. Does anyone know what that FIXME comment​ refers to? It is very old: It was first(?) added here and then (via this move) here. This and this may also be related. But I see no hint about what the "bug" is or was. Is it still relevant?

[Jun Omae]

Thanks for the patch and investigation.

I re-investigated the unified diff link.

The context lines, ignore blank lines, ignore case changes and ignore white space changes options are saved in the user session when prefs form is submitted and the unified diff link uses the saved options. However, context all option is not saved in the user session. The behavior is introduced in r9322 and comment:2:ticket:6423.

Then, I consider the unified diff link should respect contextall=1 parameter.

[d87e6dc3c/jomae.git] (I'll add unit tests).

Another thing, I noticed, after submitting negative integer or non-digits in context lines textbox, showing all in the textbox and the changeset view shows contextall diff. I think we should remove the weird behavior or Show the changes in full context radio should be selected.