Re: [PATCH] Flush stdout more often

OK, so here's a new patch that adds flushing for diff too.

Personally I don't like it though; this is taking it to a point where I
would agree with Justin's word "cruft". I only added it because Justin
said he would reject any patch that is not complete.

My personal opinion is that adding flush calls where it can be done with
minimal effort is very valuable and worthwhile. It doesn't have to be
complete to be useful; so I'm still in favour of the previous patch
(four one-liners). And I don't understand the concerns about
maintenance burdens: if someone adds a new subcommand and forgets to
add flush calls, this is not a serious problem. It won't crash or
corrupt anyone's data; it's just an inconvenience that's easily fixed if
somebody reports it.

A while ago, Brane said:

> This whole discussion seems to me a bit overblown for such a trivial
> (maintenance-wise) and generally useful change.

I absolutely agree. Why can't we just accept this as a useful
improvement and get over it?

-- 
Stefan Haller
Ableton
http://www.ableton.com/
[[[
Flush stdout after every line of status output.  This is useful when
piping svn output into post-processor tools, e.g. to colorize the
output, or simply into tee or a pager.
* subversion/include/svn_wc.h
  (svn_wc_notify_action_t): Added new constant svn_wc_notify_diff_path.
* subversion/libsvn_client/diff.c
  (diff_cmd_baton): Removed config hash, added client context instead.
  (diff_content_changed): Changed diff_cmd_baton->config to
  diff_cmd_baton->ctx->config.
  (diff_file_changed): Send svn_wc_notify_diff_path callback after doing
  the work.
  (svn_client_diff3, svn_client_diff_peg3): Initialize baton with ctx
  instead of config.
* subversion/svn/diff-cmd.c
  (summarize_func): Flush output after every line printed.
  (svn_cl__diff): Get notifier.
* subversion/svn/log-cmd.c
  (log_message_receiver): Flush output after every log entry.
* subversion/svn/status.c
  (print_status): Flush output after every line printed.
* subversion/svn/notify.c
  (notify): Flush output after every line printed; remove explicit
  flush from the svn_wc_notify_commit_postfix_txdelta case; mention
  the new svn_wc_notify_diff_path to make compilers happy.
]]]
Index: subversion/include/svn_wc.h
===================================================================
--- subversion/include/svn_wc.h (revision 19285)
+++ subversion/include/svn_wc.h (working copy)
@@ -640,7 +640,10 @@
   svn_wc_notify_failed_lock,
 
   /** Failed to unlock a path. @since New in 1.2. */
-  svn_wc_notify_failed_unlock
+  svn_wc_notify_failed_unlock,
+
+  /** Sent after generating a diff for a file. @since New in 1.4. */
+  svn_wc_notify_diff_path
 } svn_wc_notify_action_t;
 
 
Index: subversion/libsvn_client/diff.c
===================================================================
--- subversion/libsvn_client/diff.c (revision 19285)
+++ subversion/libsvn_client/diff.c (working copy)
@@ -259,8 +259,8 @@
   svn_revnum_t revnum1;
   svn_revnum_t revnum2;
 
-  /* Client config hash (may be NULL). */
-  apr_hash_t *config;
+  /* The client context. */
+  svn_client_ctx_t *ctx;
 
   /* Set this if you want diff output even for binary files. */
   svn_boolean_t force_binary;
@@ -461,9 +461,9 @@
 
 
   /* Find out if we need to run an external diff */
-  if (diff_cmd_baton->config)
+  if (diff_cmd_baton->ctx->config)
     {
-      svn_config_t *cfg = apr_hash_get(diff_cmd_baton->config,
+      svn_config_t *cfg = apr_hash_get(diff_cmd_baton->ctx->config,
                                        SVN_CONFIG_CATEGORY_CONFIG,
                                        APR_HASH_KEY_STRING);
       svn_config_get(cfg, &diff_cmd, SVN_CONFIG_SECTION_HELPERS,
@@ -537,6 +537,8 @@
                   apr_hash_t *original_props,
                   void *diff_baton)
 {
+  struct diff_cmd_baton *diff_cmd_baton = diff_baton;
+
   if (tmpfile1)
     SVN_ERR(diff_content_changed(path,
                                  tmpfile1, tmpfile2, rev1, rev2,
@@ -548,6 +550,16 @@
     *content_state = svn_wc_notify_state_unknown;
   if (prop_state)
     *prop_state = svn_wc_notify_state_unknown;
+
+  if (diff_cmd_baton->ctx->notify_func2)
+    {
+      svn_wc_notify_t *notify
+        = svn_wc_create_notify(path, svn_wc_notify_diff_path,
+                               diff_cmd_baton->pool);
+      (*diff_cmd_baton->ctx->notify_func2)(diff_cmd_baton->ctx->notify_baton2,
+                                           notify, diff_cmd_baton->pool);
+    }
+
   return SVN_NO_ERROR;
 }
 
@@ -2416,7 +2428,7 @@
   diff_cmd_baton.revnum1 = SVN_INVALID_REVNUM;
   diff_cmd_baton.revnum2 = SVN_INVALID_REVNUM;
 
-  diff_cmd_baton.config = ctx->config;
+  diff_cmd_baton.ctx = ctx;
   diff_cmd_baton.force_empty = FALSE;
   diff_cmd_baton.force_binary = ignore_content_type;
 
@@ -2515,7 +2527,7 @@
   diff_cmd_baton.revnum1 = SVN_INVALID_REVNUM;
   diff_cmd_baton.revnum2 = SVN_INVALID_REVNUM;
 
-  diff_cmd_baton.config = ctx->config;
+  diff_cmd_baton.ctx = ctx;
   diff_cmd_baton.force_empty = FALSE;
   diff_cmd_baton.force_binary = ignore_content_type;
 
Index: subversion/svn/diff-cmd.c
===================================================================
--- subversion/svn/diff-cmd.c   (revision 19285)
+++ subversion/svn/diff-cmd.c   (working copy)
@@ -83,6 +83,8 @@
                              summary->prop_changed ? 'M' : ' ',
                              path));
 
+  SVN_ERR(svn_cmdline_fflush(stdout));
+
   return SVN_NO_ERROR;
 }
 
@@ -94,6 +96,7 @@
              apr_pool_t *pool)
 {
   svn_cl__opt_state_t *opt_state = ((svn_cl__cmd_baton_t *) baton)->opt_state;
+  svn_client_ctx_t *ctx = ((svn_cl__cmd_baton_t *) baton)->ctx;
   apr_array_header_t *options;
   apr_array_header_t *targets;
   apr_file_t *outfile, *errfile;
@@ -231,6 +234,10 @@
   svn_opt_push_implicit_dot_target(targets, pool);
 
   iterpool = svn_pool_create(pool);
+
+  svn_cl__get_notifier(&ctx->notify_func2, &ctx->notify_baton2,
+                       FALSE, FALSE, FALSE, pool);
+
   for (i = 0; i < targets->nelts; ++i)
     {
       const char *path = APR_ARRAY_IDX(targets, i, const char *);
@@ -252,7 +259,7 @@
                      opt_state->notice_ancestry ? FALSE : TRUE,
                      summarize_func,
                      (void *) target1,
-                     ((svn_cl__cmd_baton_t *)baton)->ctx,
+                     ctx,
                      iterpool));
           else          
             SVN_ERR(svn_client_diff3
@@ -268,7 +275,7 @@
                      svn_cmdline_output_encoding(pool),
                      outfile,
                      errfile,
-                     ((svn_cl__cmd_baton_t *)baton)->ctx,
+                     ctx,
                      iterpool));
         }
       else
@@ -295,7 +302,7 @@
                      opt_state->notice_ancestry ? FALSE : TRUE,
                      summarize_func,
                      (void *) truepath,
-                     ((svn_cl__cmd_baton_t *)baton)->ctx,
+                     ctx,
                      iterpool));
           else
             SVN_ERR(svn_client_diff_peg3
@@ -311,7 +318,7 @@
                      svn_cmdline_output_encoding(pool),
                      outfile,
                      errfile,
-                     ((svn_cl__cmd_baton_t *)baton)->ctx,
+                     ctx,
                      iterpool));
         }
     }
Index: subversion/svn/log-cmd.c
===================================================================
--- subversion/svn/log-cmd.c    (revision 19285)
+++ subversion/svn/log-cmd.c    (working copy)
@@ -231,6 +231,8 @@
       SVN_ERR(svn_cmdline_printf(pool, "\n%s\n", msg));
     }
 
+  SVN_ERR(svn_cmdline_fflush(stdout));
+
   return SVN_NO_ERROR;
 }
 
Index: subversion/svn/status.c
===================================================================
--- subversion/svn/status.c (revision 19285)
+++ subversion/svn/status.c (working copy)
@@ -191,6 +191,8 @@
                            ? 'K' : ' '),
                           path));
 
+  SVN_ERR(svn_cmdline_fflush(stdout));
+
   return SVN_NO_ERROR;
 }
 
Index: subversion/svn/notify.c
===================================================================
--- subversion/svn/notify.c (revision 19285)
+++ subversion/svn/notify.c (working copy)
@@ -344,7 +344,6 @@
 
       if ((err = svn_cmdline_printf(pool, ".")))
         goto print_error;
-      fflush(stdout);
       break;
 
     case svn_wc_notify_locked:
@@ -364,10 +363,20 @@
       svn_handle_warning(stderr, n->err);
       break;
 
+    case svn_wc_notify_diff_path:
+      /* Do nothing special here.  We want this notification so that we can flush
+         stdout; this is done for every notification anyway, below.  We only mention
+         this case explicitely here so that the compiler doesn't warn that not all
+         enum values were handled. */
+      break;
+
     default:
       break;
     }
 
+  if ((err = svn_cmdline_fflush(stdout)))
+    goto print_error;
+
   return;
 
  print_error:
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org