[PATCH] Add -p to show C function names was Re: [PATCH] Extra options for libsvn_diff

[ Going way way back into the time machine.... ]

On Feb 16, 2006 12:45 PM, Garrett Rooney <rooneg@electricjellyfish.net> wrote:
> On 2/16/06, Julian Foad <julianfoad@btopenworld.com> wrote:
>
> > I don't know. Again, it's a matter of how we look at the situation. I think
> > that if we add this then we should be willing to add "--show-pascal-function",
> > "--show-xml-element", "--show-python-function", "--show-docbook-lite-section",
> > and so on whenever people want them. I'm serious, and maybe that's OK.
>
> I would be perfectly willing to add such features if:
>
> 1) They didn't overly complicate the diff code
> 2) They didn't come with heavyweight external dependencies
> 3) Someone actually volunteered to write them
> 4) They work acceptibly well
>
> I think the -p option fits all of those requirements.
>
> As for why I want to have it built in as opposed to using GNU diff,
> well, it's partially that I like eating our own dogfood, so I use our
> diff impl as the default, and thus when I want -p I have to add
> --diff-cmd diff to my command line, and it's partly that I sometimes
> work on platforms that don't necessarily have GNU diff installed
> (windows, solaris, etc), and it's nice for all the same functionality
> to be there.

I'd like to resurrect this discussion - IOW, I want Subversion to
support -p out-of-the-box to print C-style function names.

Julian and C-Mike's comments almost two years ago was that it was
unnecessary feature bloat. As I (among others) said back then, I
think this is a tremendously useful feature. The entire unified diff
patch is under 250 lines - I view that as outweighed by the process
savings we can incur by supporting this out-of-the-box. We've added
so many other less-universally helpful things since then (hello merge
tracking!) and that continuing to exclude this should be reconsidered.
 It's not going to be 100% perfect in its identification, but let's
not make the perfect the enemy of the far far better.

Patch against trunk below. I plan to commit this unless someone
intends to veto it.

Finally, many kudos to Peter for writing this in the first place. =) -- justin

---
Add -p to match GNU diff's show C function support.
(Use 'svn diff -x -p' to activate.)
Submitted by: lundblad
Updated by: jerenkrantz
* subversion/libsvn_diff/diff_file.c
  (svn_ctype.h): Include.
  (diff_options): Add show-c-function/p.
  (svn_diff_file_options_parse): Support -p.
  (SVN_DIFF__EXTRA_CONTEXT_LENGTH): Define to 50.
  (svn_diff__file_output_baton_t): Add boolean flag and fields to store context.
  (output_unified_line): Collect extra context if needed.
  (output_unified_flush_hunk): Emit extra context if we have it.
  (output_unified_diff_modified): Collect extra context.
  (svn_diff_file_output_unified3): Initialize new fields in baton.
  (svn_diff_file_output_unified2): Pass false to unified3.
* subversion/include/svn_diff.h
  (svn_diff_file_options_t): Add show_c_function boolean.
  (svn_diff_file_output_unified3): Add in new parameter to new 1.5 function.
  (svn_diff_file_output_unified2): Document the hidden parameter.
* subversion/libsvn_client/diff.c
  (diff_content_changed): Pass in new show_c_function boolean.
* subversion/svn/main.c
  (svn_cl__options): Document new -p/show-c-function option.
Index: subversion/include/svn_diff.h
===================================================================
--- subversion/include/svn_diff.h	(revision 28650)
+++ subversion/include/svn_diff.h	(working copy)
@@ -338,6 +338,12 @@ typedef struct svn_diff_file_options_t
   /** Whether to treat all end-of-line markers the same when comparing lines.
    * The default is @c FALSE. */
   svn_boolean_t ignore_eol_style;
+  /** Whether the '@@' lines of the unified diff output should include a prefix
+    * of the nearest preceding line that starts with a character that might be
+    * the initial character of a C language identifier.  The default is
+    * @c FALSE.
+    */
+  svn_boolean_t show_c_function;
 } svn_diff_file_options_t;
 /** Allocate a @c svn_diff_file_options_t structure in @a pool, initializing
@@ -484,10 +490,11 @@ svn_diff_file_output_unified3(svn_stream_t *output
                               const char *modified_header,
                               const char *header_encoding,
                               const char *relative_to_dir,
+                              svn_boolean_t show_c_function,
                               apr_pool_t *pool);
 /** Similar to svn_diff_file_output_unified3(), but with @a relative_to_dir
- * set to NULL.
+ * set to NULL and @a show_c_function to false.
  *
  * @deprecated Provided for backwards compatibility with the 1.3 API.
  */
Index: subversion/libsvn_diff/diff_file.c
===================================================================
--- subversion/libsvn_diff/diff_file.c	(revision 28650)
+++ subversion/libsvn_diff/diff_file.c	(working copy)
@@ -36,6 +36,7 @@
 #include "diff.h"
 #include "svn_private_config.h"
 #include "svn_path.h"
+#include "svn_ctype.h"
 /* A token, i.e. a line read from a file. */
@@ -546,6 +547,7 @@ static const apr_getopt_option_t diff_options[] =
   { "ignore-space-change", 'b', 0, NULL },
   { "ignore-all-space", 'w', 0, NULL },
   { "ignore-eol-style", SVN_DIFF__OPT_IGNORE_EOL_STYLE, 0, NULL },
+  { "show-c-function", 'p', 0, NULL },
   /* ### For compatibility; we don't support the argument to -u, because
    * ### we don't have optional argument support. */
   { "unified", 'u', 0, NULL },
@@ -598,6 +600,9 @@ svn_diff_file_options_parse(svn_diff_file_options_
         case SVN_DIFF__OPT_IGNORE_EOL_STYLE:
           options->ignore_eol_style = TRUE;
           break;
+        case 'p':
+          options->show_c_function = TRUE;
+          break;
         default:
           break;
         }
@@ -716,6 +721,9 @@ svn_diff_file_diff4(svn_diff_t **diff,
 
 /** Display unified context diffs **/
+/* Maximum length of the extra context to show when show_c_function is set.
+ * GNU diff uses 40, let's be brave and use 50 instead. */
+#define SVN_DIFF__EXTRA_CONTEXT_LENGTH 50
 typedef struct svn_diff__file_output_baton_t
 {
   svn_stream_t *output_stream;
@@ -739,6 +747,14 @@ typedef struct svn_diff__file_output_baton_t
   apr_off_t   hunk_length[2];
   svn_stringbuf_t *hunk;
+  /* Should we emit C functions in the unified diff header */
+  svn_boolean_t show_c_function;
+  /* "Context" to append to the @@ line when the show_c_function option
+   * is set. */
+  svn_stringbuf_t *extra_context;
+  /* Extra context for the current hunk. */
+  char hunk_extra_context[SVN_DIFF__EXTRA_CONTEXT_LENGTH + 1];
+
   apr_pool_t *pool;
 } svn_diff__file_output_baton_t;
@@ -761,6 +777,8 @@ output_unified_line(svn_diff__file_output_baton_t
   svn_error_t *err;
   svn_boolean_t bytes_processed = FALSE;
   svn_boolean_t had_cr = FALSE;
+  /* Are we collecting extra context? */
+  svn_boolean_t collect_extra = FALSE;
   length = baton->length[idx];
   curp = baton->curp[idx];
@@ -799,6 +817,15 @@ output_unified_line(svn_diff__file_output_baton_t
                 default:
                   break;
                 }
+
+              if (baton->show_c_function
+                  && (type == svn_diff__file_output_unified_skip
+                      || type == svn_diff__file_output_unified_context)
+                  && svn_ctype_isalnum(*curp))
+                {
+                  svn_stringbuf_setempty(baton->extra_context);
+                  collect_extra = TRUE;
+                }
             }
           eol = find_eol_start(curp, length);
@@ -825,6 +852,11 @@ output_unified_line(svn_diff__file_output_baton_t
                     {
                       svn_stringbuf_appendbytes(baton->hunk, curp, len);
                     }
+                  if (collect_extra)
+                    {
+                      svn_stringbuf_appendbytes(baton->extra_context,
+                                                curp, len);
+                    }
                   baton->curp[idx] = eol;
                   baton->length[idx] = length;
@@ -840,6 +872,11 @@ output_unified_line(svn_diff__file_output_baton_t
               svn_stringbuf_appendbytes(baton->hunk, curp, length);
             }
+          if (collect_extra)
+            {
+              svn_stringbuf_appendbytes(baton->extra_context, curp, length);
+            }
+
           bytes_processed = TRUE;
         }
@@ -858,6 +895,8 @@ output_unified_line(svn_diff__file_output_baton_t
                 {
                   svn_stringbuf_appendbytes(baton->hunk, curp, 1);
                 }
+              /* We don't append the LF to extra_context, since it would
+               * just be stripped anyway. */
               ++curp;
               --length;
             }
@@ -960,7 +999,10 @@ output_unified_flush_hunk(svn_diff__file_output_ba
   SVN_ERR(svn_stream_printf_from_utf8(baton->output_stream,
                                       baton->header_encoding,
-                                      baton->pool, " @@" APR_EOL_STR));
+                                      baton->pool, " @@%s%s" APR_EOL_STR,
+                                      baton->hunk_extra_context[0]
+                                      ? " " : "",
+                                      baton->hunk_extra_context));
   /* Output the hunk content */
   hunk_len = baton->hunk->len;
@@ -1011,6 +1053,27 @@ output_unified_diff_modified(void *baton,
           SVN_ERR(output_unified_line(output_baton,
                                       svn_diff__file_output_unified_skip, 0));
         }
+
+      if (output_baton->show_c_function)
+        {
+          int p;
+
+          /* Save the extra context for later use.
+           * Note that the last byte of the hunk_extra_context array is never
+           * touched after it is zero-initialized, so the array is always
+           * 0-terminated. */
+          strncpy(output_baton->hunk_extra_context,
+                  output_baton->extra_context->data,
+                  SVN_DIFF__EXTRA_CONTEXT_LENGTH);
+          /* Trim whitespace at the end, most notably to get rid of any
+           * newline characters. */
+          p = strlen(output_baton->hunk_extra_context);
+          while (p > 0
+                 && svn_ctype_isspace(output_baton->hunk_extra_context[p - 1]))
+            {
+              output_baton->hunk_extra_context[--p] = '\0';
+            }
+        }
     }
   /* Skip lines until we are at the start of the changed range */
@@ -1084,6 +1147,7 @@ svn_diff_file_output_unified3(svn_stream_t *output
                               const char *modified_header,
                               const char *header_encoding,
                               const char *relative_to_dir,
+                              svn_boolean_t show_c_function,
                               apr_pool_t *pool)
 {
   svn_diff__file_output_baton_t baton;
@@ -1098,6 +1162,8 @@ svn_diff_file_output_unified3(svn_stream_t *output
       baton.path[0] = original_path;
       baton.path[1] = modified_path;
       baton.hunk = svn_stringbuf_create("", pool);
+      baton.show_c_function = show_c_function;
+      baton.extra_context = svn_stringbuf_create("", pool);
       SVN_ERR(svn_utf_cstring_from_utf8_ex2(&baton.context_str, " ",
                                             header_encoding, pool));
@@ -1187,7 +1253,7 @@ svn_diff_file_output_unified2(svn_stream_t *output
   return svn_diff_file_output_unified3(output_stream, diff,
                                        original_path, modified_path,
                                        original_header, modified_header,
-                                       header_encoding, NULL, pool);
+                                       header_encoding, NULL, FALSE, pool);
 }
 svn_error_t *
Index: subversion/libsvn_client/diff.c
===================================================================
--- subversion/libsvn_client/diff.c	(revision 28650)
+++ subversion/libsvn_client/diff.c	(working copy)
@@ -604,7 +604,8 @@ diff_content_changed(const char *path,
           /* Output the actual diff */
           SVN_ERR(svn_diff_file_output_unified3
                   (os, diff, tmpfile1, tmpfile2, label1, label2,
-                   diff_cmd_baton->header_encoding, rel_to_dir, subpool));
+                   diff_cmd_baton->header_encoding, rel_to_dir,
+                   opts->show_c_function, subpool));
         }
     }
Index: subversion/svn/main.c
===================================================================
--- subversion/svn/main.c	(revision 28650)
+++ subversion/svn/main.c	(working copy)
@@ -172,7 +172,11 @@ const apr_getopt_option_t svn_cl__options[] =
                        "                            "
                        "    --ignore-eol-style:\n"
                        "                            "
-                       "       Ignore changes in EOL style")},
+                       "       Ignore changes in EOL style\n"
+                       "                            "
+                       "    -p (--show-c-function):\n"
+                       "                            "
+                       "       Show C function name in diff output.")},
 #endif
   {"targets",       opt_targets, 1,
                     N_("pass contents of file ARG as additional args")},
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org