You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Stefan Haller <ha...@ableton.com> on 2006/02/09 13:34:03 UTC

Stdout buffering issue

I'm writing a simple wrapper script for the svn command line client that
colorizes the status, update, and merge output.  It is extremely
valuable to easily spot conflicts if they stand out in red.

Trouble is, it doesn't work well with the stock svn client, because
stdout is only line buffered if output goes to a terminal; if it goes to
a pipe, it is block-buffered, which means in practice that first the
entire operation completes and then I see the colorized output all at
once.  This sucks for large updates or merges that can easily take
minutes to complete.

So I think that the output of co, st, up, switch, and merge should be
line-buffered always.  (Diff and blame output shouldn't, of course.)

As a hack for my personal use, I just put

  if (subcommand->cmd_func == svn_cl__checkout
      || subcommand->cmd_func == svn_cl__update
      || subcommand->cmd_func == svn_cl__merge
      || subcommand->cmd_func == svn_cl__switch
      || subcommand->cmd_func == svn_cl__status)
    {
      setlinebuf(stdout);
    }

in subversion/clients/cmdline/main.c, right before cmd_func is called;
for a real solution, one would probably have to insert calls to
svn_cmdline_fflush in all subcommands as appropriate.

Before I go about making a patch, I'd like to know if people think this
is a good idea at all.


-- 
Stefan Haller
Ableton
http://www.ableton.com/

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org

Re: [PATCH] Flush stdout more often

Posted by Vincent Lefevre <vi...@vinc17.org>.
On 2006-03-07 02:14:13 -0800, Justin Erenkrantz wrote:
> The only thing you'd need to implement is the print logic that you
> want.  You'd implement svn_wc_notify_func2_t and set that in your
> svn_client_ctx.  You can call the diff/merge/whatever functions from
> your application directly rather than invoking a binary application

My application is an interactive shell. The interface between
Subversion and the shell is a shell command (currently the
"svn"/"svnadmin" commands).

> and your notifier will get invoked.  You'll also get all of the
> information you want directly from the bindings without having to
> parse anything thing from stdout.  You can display it however you like
> with as much or as little buffering as you'd like.
> 
> The bindings are the way every front-end to Subversion should be written.

So, you suggest to rewrite a command that does the same thing as svn,
except that it uses the bindings to flush the output when needed?

-- 
Vincent Lefèvre <vi...@vinc17.org> - Web: <http://www.vinc17.org/>
100% accessible validated (X)HTML - Blog: <http://www.vinc17.org/blog/>
Work: CR INRIA - computer arithmetic / SPACES project at LORIA

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org

Re: [PATCH] Flush stdout more often

Posted by Vincent Lefevre <vi...@vinc17.org>.
On 2006-03-07 09:12:33 -0800, Justin Erenkrantz wrote:
> But, that patch doesn't even begin to add it to half of the places
> that we need it!

This wouldn't be worse than block-buffering. If you complain, why not
fix block-buffering first?

-- 
Vincent Lefèvre <vi...@vinc17.org> - Web: <http://www.vinc17.org/>
100% accessible validated (X)HTML - Blog: <http://www.vinc17.org/blog/>
Work: CR INRIA - computer arithmetic / SPACES project at LORIA

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org

Re: [PATCH] Flush stdout more often

Posted by "Peter N. Lundblad" <pe...@famlundblad.se>.
Vincent Lefevre writes:
 > On 2006-03-08 08:27:29 +0100, Stefan Haller wrote:
 > > OK, in that case I see two possibilities:
 > > 
 > > - add the flushing to libsvn_client/diff.c in the hope that it is the
 > > appropriate thing for all clients, not just the svn command line client
 > > (which I tend to think is true; at least I can't imagine that it would
 > > hurt anybody); or
 > 
 > Yes, there could even be some benefits. In general, it is better to
 > flush if one knows that the following output won't be sent before
 > some time.
 > 

No, no standard stream manipulation in libsvn_client, please. The
solution here would be to add a notification call (which could be
useful for GUI clients anyway). The cmdline client could just fflush
stdout. (Notification doesn't necesarily mean "print something to the
user").

Regards,
//Peter

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org

Re: [PATCH] Flush stdout more often

Posted by Vincent Lefevre <vi...@vinc17.org>.
On 2006-03-08 08:27:29 +0100, Stefan Haller wrote:
> OK, in that case I see two possibilities:
> 
> - add the flushing to libsvn_client/diff.c in the hope that it is the
> appropriate thing for all clients, not just the svn command line client
> (which I tend to think is true; at least I can't imagine that it would
> hurt anybody); or

Yes, there could even be some benefits. In general, it is better to
flush if one knows that the following output won't be sent before
some time.

> - live with the fact that diff output doesn't have the level of
> streaminess that would be nice to have.  Diff would then be no worse
> than it is now, but all other subcommands would be improved; to me this
> improvement would outweigh the inconsistency.

Yes, and IMHO, since diff often generates more output, the problem
is less important with diff.

Also, stdout and stderr should be synchronized. This is currently not
the case due to the lack of flush. For instance, I currently get:

dixsept:~/wd> svn info -r10000 tex www | grep '^URL'
Connected to ay (from 152.81.9.195)
Connected to ay (from 152.81.9.195)
Connected to ay (from 152.81.9.195)
Connected to ay (from 152.81.9.195)
URL: svn+ssh://home/home/lefevre/svn/tex
URL: svn+ssh://home/home/lefevre/svn/www

instead of

Connected to ay (from 152.81.9.195)
Connected to ay (from 152.81.9.195)
URL: svn+ssh://home/home/lefevre/svn/tex
Connected to ay (from 152.81.9.195)
Connected to ay (from 152.81.9.195)
URL: svn+ssh://home/home/lefevre/svn/www

-- 
Vincent Lefèvre <vi...@vinc17.org> - Web: <http://www.vinc17.org/>
100% accessible validated (X)HTML - Blog: <http://www.vinc17.org/blog/>
Work: CR INRIA - computer arithmetic / SPACES project at LORIA

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org

Re: [PATCH] Flush stdout more often

Posted by Stefan Haller <ha...@ableton.com>.
Branko ?ibej <br...@xbc.nu> wrote:

> Stefan Haller wrote:
> > My idea was to touch cmd-line client code only, no core code, so you
> > don't have to make any assumptions about ViewVC or others.  Nothing will
> > change for them.
> >
> > You are right about diff, I missed that.  However, as far as I can see
> > it is not necessary to touch libsvn_diff for that; it is sufficiant to
> > add fflush calls to libsvn_client/diff.c.
>
> This is inconsistent with the previous paragraph. libsvn_client is *not*
> specific to the command-line client; it is a core library that any 
> client must use.

Ah, sorry, I didn't realize that.

OK, in that case I see two possibilities:

- add the flushing to libsvn_client/diff.c in the hope that it is the
appropriate thing for all clients, not just the svn command line client
(which I tend to think is true; at least I can't imagine that it would
hurt anybody); or

- live with the fact that diff output doesn't have the level of
streaminess that would be nice to have.  Diff would then be no worse
than it is now, but all other subcommands would be improved; to me this
improvement would outweigh the inconsistency.

I could live with either.


-- 
Stefan Haller
Ableton
http://www.ableton.com/

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org

Re: [PATCH] Flush stdout more often

Posted by Branko Čibej <br...@xbc.nu>.
Stefan Haller wrote:
> My idea was to touch cmd-line client code only, no core code, so you
> don't have to make any assumptions about ViewVC or others.  Nothing will
> change for them.
>
> You are right about diff, I missed that.  However, as far as I can see
> it is not necessary to touch libsvn_diff for that; it is sufficiant to
> add fflush calls to libsvn_client/diff.c.
This is inconsistent with the previous paragraph. libsvn_client is *not* 
specific to the command-line client; it is a core library that any 
client must use.

-- Brane


---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org

Re: [PATCH] Flush stdout more often

Posted by kf...@collab.net.
haller@ableton.com (Stefan Haller) writes:
> Justin Erenkrantz <ju...@erenkrantz.com> wrote:
> > For example, one of the claims was that he really wants to support
> > diff.  The change submitted was to the new summarize function - it
> > wouldn't touch diff at all.  The diff printing code is deep down
> > inside our core libraries - libsvn_diff assumes that it has a stream
> > (svn_diff_file_output_unified2).  At the minimum, we *must* add fflush
> > after every header write in diff.  But, is that a valid assumption? 
> > For the command-line - yes.  But, for ViewVC?  I'm not so sure. 
> 
> Thanks for being constructive; now we're getting somewhere.

Urgk.  I'm sorry to chime in with only meta-comments in this thread,
but this is important.

Statements like "thanks for being constructive; now we're getting
somewhere" imply that Justin was being non-constructive before.
That's not fair at all.  He articulated his concerns clearly and
without rancor; he may have been a bit terse, but that was easily
solved by requesting more detailed explanations, which he gave.

I don't necessarily agree with Justin's technical reasoning here, but
he's conducted himself just fine in the discussion.  I don't think you
can really complain about him in that respect.  Let's please limit
ourselves to technical statements on technical issues, and not hint
someone was being non-constructive when they're just disagreeing.  If
you're frustrated that you can't persuade Justin, the solution is to
go find other people to persuade (which you successfully did), not
expand the issue to include Justin's moral fiber :-).  (I know you
probably didn't mean to, it just came out looking like that.)

Thanks,
-Karl

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org

Re: [PATCH] Flush stdout more often

Posted by Stefan Haller <ha...@ableton.com>.
Justin Erenkrantz <ju...@erenkrantz.com> wrote:

> For example, one of the claims was that he really wants to support
> diff.  The change submitted was to the new summarize function - it
> wouldn't touch diff at all.  The diff printing code is deep down
> inside our core libraries - libsvn_diff assumes that it has a stream
> (svn_diff_file_output_unified2).  At the minimum, we *must* add fflush
> after every header write in diff.  But, is that a valid assumption? 
> For the command-line - yes.  But, for ViewVC?  I'm not so sure. 

Thanks for being constructive; now we're getting somewhere.

My idea was to touch cmd-line client code only, no core code, so you
don't have to make any assumptions about ViewVC or others.  Nothing will
change for them.

You are right about diff, I missed that.  However, as far as I can see
it is not necessary to touch libsvn_diff for that; it is sufficiant to
add fflush calls to libsvn_client/diff.c.  I think it makes the most
sense to flush after the output for every file, be it props only, text
changes, status only, whatever; so the appropriate fix would be to
fflush at the end of each of the six diff_callbacks.  Please correct me
if you think this is wrong.

Can you (or anybody else) think of any other places besides this and the
three one-liners in my first patch?


-- 
Stefan Haller
Ableton
http://www.ableton.com/

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org

Re: [PATCH] Flush stdout more often

Posted by Peter Samuelson <pe...@p12n.org>.
[Justin Erenkrantz]
> The problem is that forcing a flush is a hack and doesn't solve
> anyone's actual problems.  It solves a completely contrived problem
> that simply does not matter

If your main objection is that nobody really wants this anyway, I
disagree.  I, as a lowly end-user, think it's very useful to pipe the
svn client output into other things.  A filter to add colors or
boldface to certain status lines.  A pager.  grep.  tee.  Output should
generally be flushed per-file (for diff this means after lots of lines,
for many other commands it would be every line).  Perhaps XML output
need not be buffered, since an XML document is usually not processed
until the whole thing is available to be validated.


> Where do we stop?  In short, we are not going to be able to guarantee
> line-buffering to a *file* across the entire client without a much
> larger and substantial patch.  We now have to audit every place we
> produce any output and add flushing.

I'd be in favor of that too - though I'd call it buffering per file
rather than per line.  This makes a difference for 'diff', as well as
exempting the subcommands such as 'help' and 'info' whose output isn't
worth flushing anyway.

Auditing all the output calls certainly doesn't have to be done all at
once.

Re: [PATCH] Flush stdout more often

Posted by Stefan Haller <ha...@ableton.com>.
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

Re: [PATCH] Flush stdout more often

Posted by "Peter N. Lundblad" <pe...@famlundblad.se>.
Stefan Haller writes:
 > Peter N. Lundblad <pe...@famlundblad.se> wrote:
 > 
 > > As I think I pointed out somewhere, you can solve this by adding a
 > > notification kind so the notify callback gets invoked after each diffed
 > > file (this might be useful for GUIs as well).  The cmdline client would
 > > just fflush the output stream.
 > 
 > Ah, ok.  I misunderstood this the last time you mentioned it, sorry.
 > 
 > Just to make sure I get it right this time: I add a new constant at the
 > end of the svn_wc_notify_action_t enum, right?  Would you just call that
 > svn_wc_notify_diff?

That'd be my suggestion, but maybe call it svn_wc_notify_diff_path or
something in case want to add other diff notifications in the future.

Thanks,
//Peter

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org

Re: [PATCH] Flush stdout more often

Posted by Stefan Haller <ha...@ableton.com>.
Peter N. Lundblad <pe...@famlundblad.se> wrote:

> As I think I pointed out somewhere, you can solve this by adding a
> notification kind so the notify callback gets invoked after each diffed
> file (this might be useful for GUIs as well).  The cmdline client would
> just fflush the output stream.

Ah, ok.  I misunderstood this the last time you mentioned it, sorry.

Just to make sure I get it right this time: I add a new constant at the
end of the svn_wc_notify_action_t enum, right?  Would you just call that
svn_wc_notify_diff?


-- 
Stefan Haller
Ableton
http://www.ableton.com/

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org

Re: [PATCH] Flush stdout more often

Posted by "Peter N. Lundblad" <pe...@famlundblad.se>.
Stefan Haller writes:
 > Looking back through the postings in this thread, quite a lot of people
 > were in favour of applying the patch in this form.  Only Justin
 > Erenkrantz opposed; the objection was that the patch is incomplete, for
 > example it doesn't flush diff output.  That's correct, but fixing this
 > would require much bigger changes than I feel is justified for this; as
 > I see it, it would require adding a callback to svn_client_diff3 and
 > svn_client_diff_peg3, and changing the svn_wc_diff_callbacks2_t vtable
 > to store this callback.  I don't think it's worth that, especially since

As I think I pointed out somewhere, you can solve this by adding a notification
kind so the notify callback gets invoked after each diffed file
(this might be useful for GUIs as well).  The cmdline client would just fflush
the output stream.

 > May I ask for opinions again, please.
 > 

As before, I think this is a good idea, but I'm not going to do anything
with this (if we can convince Justin that is) until after 1.4 is branched.
I have more important things to do, syrro.

Thanks,
//Peter

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org

Re: [PATCH] Flush stdout more often

Posted by Greg Hudson <gh...@MIT.EDU>.
On Mon, 2006-04-10 at 10:32 +0000, Mattias Engdegård wrote:
> Adding an option for setvbuf solves it once and for all.

Using setvbuf instead of explicit flush calls may be the right answer.

Having an option of any kind for this would be beyond ridiculous.


---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org


Re: [PATCH] Flush stdout more often

Posted by Mattias Engdegård <ma...@virtutech.se>.
Peter Samuelson <pe...@p12n.org> writes:

>Ewwww.  That's a much bigger kludge than his patch.  Please don't tell
>me, as an end user, that I should have to wrap the svn client with a
>little script just to get output which it is reasonable to pipe through
>a pager.

It was suggested as a solution of his immediate problem. I assumed he
just wanted to get things done.

>You didn't read his patch, did you?  He added 4 flush calls and removed
>one.  That is not "littering flushes all over the code".

Of course I read his patch; please don't put words in my mouth.
The problem with that approach is that it is open-ended and requires
maintenance indefinitely after it has been applied.
Adding an option for setvbuf solves it once and for all.

>Can such an option be supported using less code than the 4 calls he
>added?  That would, frankly, surprise me.

I claim it's not the absolute line count up-front that matters, but
how it will have to be maintained in the future.

>(Likewise for 'svn diff', though flushing at
>the end of each file might be worthwhile.)

Right, you already found a need for improvement. And then another
svn sub-command is added, which might need flushing. And so on.

Now look what you have done - you dragged me into a bicycle shed.
Shame on me for falling into the trap.


---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org

Re: [PATCH] Flush stdout more often

Posted by Peter Samuelson <pe...@p12n.org>.
[Mattias Engdegård]
> What about the following trivial script:
> 
> #!/usr/bin/env python
> import sys, os
> (pid, master) = os.forkpty()

Ewwww.  That's a much bigger kludge than his patch.  Please don't tell
me, as an end user, that I should have to wrap the svn client with a
little script just to get output which it is reasonable to pipe through
a pager.

Don't forget the Lesson of Arch: if you have to use shell
aliases/functions or custom command completion in order to have a
usable client, something about your CLI has gone very, very wrong.

> Instead of littering your flushes all over the code, just run your
> subversion command through this. No svn changes necessary.

You didn't read his patch, did you?  He added 4 flush calls and removed
one.  That is not "littering flushes all over the code".

> (If you insist in changing the client, why not add a global option
> which just sets the stdout buffering to line mode?)

Can such an option be supported using less code than the 4 calls he
added?  That would, frankly, surprise me.  I also like how his patch
doesn't touch areas like 'svn cat' and 'svn info' for which line
buffering is not needed.  (Likewise for 'svn diff', though flushing at
the end of each file might be worthwhile.)

Re: [PATCH] Flush stdout more often

Posted by Branko Čibej <br...@xbc.nu>.
Stefan Haller wrote:
> Mattias Engdegård <ma...@virtutech.se> wrote:
>
>   
>> haller@ableton.com (Stefan Haller) writes:
>>
>>     
>>> To recap: For operations that can take a long time to complete, such as
>>> status or update, it is nice to flush stdout after every line of status
>>> output, to give nicer behaviour when piping output into, say, a pager,
>>> or tee.
>>>       
>> What about the following trivial script:
>>
>> [ pty wrapper script snipped ]
>>     
>
> This is not an acceptable solution; users shouldn't have to do this.  If
> you type "make | tee log.txt", you expect to see the ouput while it
> happens, right?  If this works well for make, I don't see why some
> people insist it shouldn't also for svn.
>
> Besides, it is probably not very portable.  I'm not sure if it works in
> cygwin, for example (I haven't tried).
>
>   
>> (If you insist in changing the client, why not add a global option
>> which just sets the stdout buffering to line mode?)
>>     
>
> There shouldn't be an option for something like this.  It should "just
> work", without the user having to do anything.
>
> And you don't really want line mode anyway.  You want flushing after
> chunks of information; in many cases this is after every line, but for
> log it is after each entry, and for diff it is after each file.
>
> I really think that judicious flushing in selected places is the right
> thing to do to solve this.
>   
I agree, and +1 to committing your patch and ending this discussion. If 
nobody beats me to it, I'll look into committing this over the week-end.

-- Brane



---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org

Re: [PATCH] Flush stdout more often

Posted by Stefan Haller <st...@snafu.de>.
Mattias Engdegård <ma...@virtutech.se> wrote:

> haller@ableton.com (Stefan Haller) writes:
> 
> >To recap: For operations that can take a long time to complete, such as
> >status or update, it is nice to flush stdout after every line of status
> >output, to give nicer behaviour when piping output into, say, a pager,
> >or tee.
> 
> What about the following trivial script:
> 
> [ pty wrapper script snipped ]

This is not an acceptable solution; users shouldn't have to do this.  If
you type "make | tee log.txt", you expect to see the ouput while it
happens, right?  If this works well for make, I don't see why some
people insist it shouldn't also for svn.

Besides, it is probably not very portable.  I'm not sure if it works in
cygwin, for example (I haven't tried).

> (If you insist in changing the client, why not add a global option
> which just sets the stdout buffering to line mode?)

There shouldn't be an option for something like this.  It should "just
work", without the user having to do anything.

And you don't really want line mode anyway.  You want flushing after
chunks of information; in many cases this is after every line, but for
log it is after each entry, and for diff it is after each file.

I really think that judicious flushing in selected places is the right
thing to do to solve this.


-- 
Stefan Haller
Berlin, Germany
http://home.snafu.de/stk/

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org


Re: [PATCH] Flush stdout more often

Posted by Mattias Engdegård <ma...@virtutech.se>.
haller@ableton.com (Stefan Haller) writes:

>To recap: For operations that can take a long time to complete, such as
>status or update, it is nice to flush stdout after every line of status
>output, to give nicer behaviour when piping output into, say, a pager,
>or tee.

What about the following trivial script:

#!/usr/bin/env python
import sys, os
(pid, master) = os.forkpty()
if pid == 0:
    os.execvp(sys.argv[1], sys.argv[1:])
while 1:
    try:
        s = os.read(master, 0x1000)
    except:
        break
    sys.stdout.write(s)

Instead of littering your flushes all over the code, just run your subversion
command through this. No svn changes necessary.

(If you insist in changing the client, why not add a global option
which just sets the stdout buffering to line mode?)


---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org

Re: [PATCH] Flush stdout more often

Posted by Stefan Haller <ha...@ableton.com>.
Sorry to be a pest, but I'd like to bring this issue up again, hoping it
may be possible to reach some kind of consensus this time.

For those who missed the beginning of this thread (about two months
ago), it started at
<http://subversion.tigris.org/servlets/ReadMsg?list=dev&msgNo=112026>.

To recap: For operations that can take a long time to complete, such as
status or update, it is nice to flush stdout after every line of status
output, to give nicer behaviour when piping output into, say, a pager,
or tee.

Looking back through the postings in this thread, quite a lot of people
were in favour of applying the patch in this form.  Only Justin
Erenkrantz opposed; the objection was that the patch is incomplete, for
example it doesn't flush diff output.  That's correct, but fixing this
would require much bigger changes than I feel is justified for this; as
I see it, it would require adding a callback to svn_client_diff3 and
svn_client_diff_peg3, and changing the svn_wc_diff_callbacks2_t vtable
to store this callback.  I don't think it's worth that, especially since
diff is a subcommand where streamy output is not nearly as important in
practice as it is for status, update, or merge.

Apart from diff, I'm not aware of any other places that need flushing;
please point out any that I might have missed.  (I did add flushing to
svn log since the first version of the patch.)  Personally I think that
even if this is incomplete, the usefulness for those operations where it
does flush now outweighs the inconsistency by far.

May I ask for opinions again, please.


-- 
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.

* subversion/svn/diff-cmd.c
  (summarize_func): Flush output after every line printed.

* 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.
]]]


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;
 }
 
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:
@@ -368,6 +367,9 @@
       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

Re: [PATCH] Flush stdout more often

Posted by Stefan Haller <ha...@ableton.com>.
Kevin Puetz <pu...@puetzk.org> wrote:

> Actually, if it's preferable to do it globally, the solution is a *single*
> one-liner :-), either:
>         setlinebuf(stdout);

That's what I mentioned in the very first mail in this thread.  But no,
I don't think this is a good idea; for something like

$ svn cat svn://host/some-large-file > output

you definitely don't want line-buffering.


-- 
Stefan Haller
Ableton
http://www.ableton.com/

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org

Re: [PATCH] Flush stdout more often

Posted by Kevin Puetz <pu...@puetzk.org>.
Justin Erenkrantz wrote:

> I will veto any patches that attempt to add a guarantee of
> line-buffering but do not so consistently across the board.  I refuse
> to see us take half-hearted approaches - we're not CVS.
> 
> Enough people have said that flushing is a good idea that I'm sure
> lots of people are willing to write the patch.  I'd be willing to
> review them.  I do caution that a complete solution is going to be
> larger than three one-liners.  -- justin

Actually, if it's preferable to do it globally, the solution is a *single*
one-liner :-), either:
        setlinebuf(stdout);
or
        setvbuf(stdout, (char *)NULL, _IOLBF, 0);

This explicitly requests line-buffered stdout, rather than just accepting
whatever the OS chose to give us (typically line-buffered for a pty and
block-buffered for anything else, but not actually guaranteed AFAIK). Since
this changes the actual buffering policy in stdio, it should make the case
where stdout is redirected exactly the same as the current case where it
isn't.

I've usually used setlinebuf (which is 4.2BSD), since it's easier to
remember, but I'm not sure about the portability of it (this issue has
never before come up in an app where I cared about the portability).
setvbuf is C89, which should be portable enough. Either call is documented
as something which must be done "after opening the stream, and before any
other operations have been performed on it", so this would have to be done
early in the svn commandline client's initialization; it doesn't appear to
inherit across exec (at least on linux), so it's apparently not something
that a caller can force.


---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org

Re: [PATCH] Flush stdout more often

Posted by Justin Erenkrantz <ju...@erenkrantz.com>.
On 3/7/06, Peter N. Lundblad <pe...@famlundblad.se> wrote:
> For notifications, yes, since the whole point of notification is to
> give the user instant feedback. For status, also yes, because it will
> often wait for the network (status -u) and there is no point in having
> the next process in the pipe wait when there is more data it could
> process.

*sigh*

The problem is that forcing a flush is a hack and doesn't solve
anyone's actual problems.  It solves a completely contrived problem
that simply does not matter - and to which there are several
alternatives that don't impact everyone else.

That said, everyone else seems to disagree - fine.

> I never understood Justin's argument about cruft.  It is a few
> one-liner changes to add fflush calls. That can't be a maintenance
> burden worth worryihg about.

But, that patch doesn't even begin to add it to half of the places
that we need it!

Where do we stop?  In short, we are not going to be able to guarantee
line-buffering to a *file* across the entire client without a much
larger and substantial patch.  We now have to audit every place we
produce any output and add flushing.

For example, one of the claims was that he really wants to support
diff.  The change submitted was to the new summarize function - it
wouldn't touch diff at all.  The diff printing code is deep down
inside our core libraries - libsvn_diff assumes that it has a stream
(svn_diff_file_output_unified2).  At the minimum, we *must* add fflush
after every header write in diff.  But, is that a valid assumption? 
For the command-line - yes.  But, for ViewVC?  I'm not so sure.  This
doesn't even mention that we'll have to setup a pty ourselves for
external diff to ensure we maintain the line-buffering semantics
regardless of usage.

A custom client can and should use these same code paths through our
bindings - but when we add fflush's, we've now gone and made all of
our bindings and other programs line-buffered too.  Is that really
what we intend?  Are we making the right call for all clients?

I will veto any patches that attempt to add a guarantee of
line-buffering but do not so consistently across the board.  I refuse
to see us take half-hearted approaches - we're not CVS.

Enough people have said that flushing is a good idea that I'm sure
lots of people are willing to write the patch.  I'd be willing to
review them.  I do caution that a complete solution is going to be
larger than three one-liners.  -- justin

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org


Re: [PATCH] Flush stdout more often

Posted by "Peter N. Lundblad" <pe...@famlundblad.se>.
Max Bowsher writes:
 > -----BEGIN PGP SIGNED MESSAGE-----
 > Hash: SHA1
 > 
 > Since our command-line client output is line-structured, it seems to me
 > that it ought to be line-buffered, and the fact that it is not, at the
 > moment, is a bug.
 > 
Not everywhere, as has been pointed out earlier. diff/cat/blame are
examples where flushing after each lines doesn't make sense.

For notifications, yes, since the whole point of notification is to
give the user instant feedback. For status, also yes, because it will
often wait for the network (status -u) and there is no point in having
the next process in the pipe wait when there is more data it could
process.

I never understood Justin's argument about cruft.  It is a few
one-liner changes to add fflush calls. That can't be a maintenance
burden worth worryihg about.

Thanks,
//Peter

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org

Re: [PATCH] Flush stdout more often

Posted by Max Bowsher <ma...@ukf.net>.
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Malcolm Rowe wrote:
> On Tue, Mar 07, 2006 at 02:14:13AM -0800, Justin Erenkrantz wrote:
>> The bindings are the way every front-end to Subversion should be written.
>>
> 
> I disagree, or perhaps I misunderstand your definition of 'front-end'.
> Scriptability of the command-line client is something we've been
> promoting _as well as_ the bindings forever - it's even on the front
> page of subversion.tigris.org:
> 
> # Parseable output
> 
> All output of the Subversion command-line client is carefully designed
> to be both human readable and automatically parseable; scriptability is
> a high priority.
> 
> 
> I frequently write trivial scripts to parse the output of the command-line
> client.  Nothing complex, but I'd be unhappy if we've suddenly decided
> to disapprove of this kind of behaviour.
> 
> As to the issue at hand: what exactly was the objection again? (sorry,
> I've lost track)  As long as we're limiting a change to subversion/svn
> rather than subversion/libsvn_*, I don't really see a problem.

Since our command-line client output is line-structured, it seems to me
that it ought to be line-buffered, and the fact that it is not, at the
moment, is a bug.

Max.
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.1 (Cygwin)

iD8DBQFEDX6WfFNSmcDyxYARAtZTAKCngMawnjDWcXXWFk7bfWaXz8IFpwCgmHiT
DkBACSNICeJRGCl6rul3+FU=
=Nxqy
-----END PGP SIGNATURE-----

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org

Re: [PATCH] Flush stdout more often

Posted by Malcolm Rowe <ma...@farside.org.uk>.
On Tue, Mar 07, 2006 at 02:14:13AM -0800, Justin Erenkrantz wrote:
> The bindings are the way every front-end to Subversion should be written.
> 

I disagree, or perhaps I misunderstand your definition of 'front-end'.
Scriptability of the command-line client is something we've been
promoting _as well as_ the bindings forever - it's even on the front
page of subversion.tigris.org:

# Parseable output

All output of the Subversion command-line client is carefully designed
to be both human readable and automatically parseable; scriptability is
a high priority.


I frequently write trivial scripts to parse the output of the command-line
client.  Nothing complex, but I'd be unhappy if we've suddenly decided
to disapprove of this kind of behaviour.

As to the issue at hand: what exactly was the objection again? (sorry,
I've lost track)  As long as we're limiting a change to subversion/svn
rather than subversion/libsvn_*, I don't really see a problem.

Regards,
Malcolm

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org

Re: [PATCH] Flush stdout more often

Posted by Branko Čibej <br...@xbc.nu>.
Justin Erenkrantz wrote:
> The bindings are the way every front-end to Subversion should be written.
>   
Justin, as others have pointed out before, this is simply not true. I 
can't imagine where you got that idea; scriptability of the command-line 
tools has been a goal from day one. Another goal is that Subversion 
command-line tools should behave like native apps on all supported 
systems; at least on Unix it's natural to expect such tools to interact 
nicely with script wrappers.

The fact that some parts of the code already do line-based flushing is 
also relevant.

Looking back at this thread, the majority opinion is clearly in support 
of this patch. If you want to stop it with the "v"-word, that's your 
prerogative. But before you do, please consider that rejecting this 
patch for the reasons you supplied would imply that we have to rip out 
all existing line-based flushing from the code.

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

-- Brane


---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org

Re: [PATCH] Flush stdout more often

Posted by Justin Erenkrantz <ju...@erenkrantz.com>.
On 3/7/06, Stefan Haller <ha...@ableton.com> wrote:
> Others have pointed out that the patch is not only useful to my
> particular problem, but also when you redirect output to a file and look
> at it with tail -f, or pipe it through tee, things like that.  You
> assert that nobody cares about that, which I don't believe.  Why should
> this be less important for svn than it is for make or other tools that
> produce line-based output?

Because to me no one has yet made a convincing argument that the lack
of flushing is a demonstrable and damaging problem that requires us to
fix our core code.

> As for my specific problem: to me, using a pty feels like much more the
> wrong solution than having svn flush more often.

Well, it'd work with all prior and future releases.

> And a custom notification callback: maybe I don't understand well enough
> what you are suggesting, so could you please briefly outline what you
> think I should do in order to post-process the output of, say,
> "svn merge".  I want it to behave exactly like svn merge, with the same
> option handling (including future enhancements); I don't see how I can
> do that without completely reimplementing merge-cmd.c and notify.c in
> python.

The only thing you'd need to implement is the print logic that you
want.  You'd implement svn_wc_notify_func2_t and set that in your
svn_client_ctx.  You can call the diff/merge/whatever functions from
your application directly rather than invoking a binary application
and your notifier will get invoked.  You'll also get all of the
information you want directly from the bindings without having to
parse anything thing from stdout.  You can display it however you like
with as much or as little buffering as you'd like.

The bindings are the way every front-end to Subversion should be written.

> > > Does it help if I collect some timing data of some sort?  If
> > > so, what exactly would be useful?
> >
> > Any timing data that you could produce would not address my
> > fundamental concerns that the core is the wrong place to address this.
>
> I don't get it.  You have heard several different reasons why the change
> can be useful to some people; and you have heard that the change does no
> harm (actually, makes no difference at all) for everybody else.  Still
> you object to it, "just because"?

Uh, no.  You haven't been paying attention to my concerns then.

Our command-line client is not supposed to serve everyone's
customization needs.  To continually add bloat to support one minor
case when there are far better alternative avenues to support the case
should be against our philosophy.  It's why we have the bindings - if
the command-line client doesn't fit what you personally need, you
should use the bindings and write your own notifier rather than expect
the command-line client to gain a feature that only your app would
ever truly care about.  -- justin

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org


Re: [PATCH] Flush stdout more often

Posted by Justin Erenkrantz <ju...@erenkrantz.com>.
On 3/7/06, Jim Blandy <ji...@red-bean.com> wrote:
> I'm too sleepy to find the right part of the process document to
> quote, but it's my understanding that after someone raises a veto
> (which Justin has clearly done), after a certain period of discussion,

I've been very careful not to mention the 'v' word.  I would like to
see us come to a consensus though - as a matter of a last resort, yes,
I would consider utilizing the 'v' word - but I would rather not have
to make that decision.

> the issue is brought to a vote.  So there is a way for the patch to
> progress; Justin's concerns simply mean we are more deliberate about
> it.

I don't believe we've called out the process for an override.  We've
usually tried to reach consensus before that point.  -- justin

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org


Re: [PATCH] Flush stdout more often

Posted by Jim Blandy <ji...@red-bean.com>.
I'm too sleepy to find the right part of the process document to
quote, but it's my understanding that after someone raises a veto
(which Justin has clearly done), after a certain period of discussion,
the issue is brought to a vote.  So there is a way for the patch to
progress; Justin's concerns simply mean we are more deliberate about
it.

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org


Re: [PATCH] Flush stdout more often

Posted by kf...@collab.net.
"Justin Erenkrantz" <ju...@erenkrantz.com> writes:
> On 3/6/06, Stefan Haller <ha...@ableton.com> wrote:
> > I'm told that this patch won't be committed because no consensus was
> > reached.  Frankly, my impression was that a consensus *was* reached:
> > many people gave good reasons why the patch is useful, and you stopped
> > arguing against them.  But apparently that's not what people call
> > "consensus" here, so I'm unsure how to proceed with this issue.
> 
> Consensus is when everyone agrees.

Yes, sort of.

Consensus and voting are not the same thing; consensus and 100%
agreement are not quite the same thing either.

Consensus is what happens when someone says "Do we have consensus on
XYZ?" and no one claims otherwise (after a reasonable period has
elapsed for people to check their mail, of course.)  Thus consensus
can mean some people want XYZ to happen, and others don't object
enough to stand in the way, even though they might not be enthusiastic
supporters.

This is the working definition we've been using forever, as far as I
know.  I've posted it on this list before, but unfortunately it's hard
to find the right SIP to track it down with a search engine.  So I'm
reduced the embarrassing tactic of referring to my own book:
http://producingoss.com/html-chunk/consensus-democracy.html

(Maybe we should formalize this in hacking.html, but we've never
really needed to before.)

It's academic here, of course.  We don't have consensus in this case,
Justin has clearly articulated why, so the answer is to discuss some
more, until someone thinks of a better solution or (perhaps) until
Justin decides he's convinced by what he hears.  Voting should be very
rare, a last resort, especially on technical issues.  Er, if I may:
http://producingoss.com/html-chunk/consensus-democracy.html#when-to-vote

I'm not citing these sections from my book as some sort of authority,
by the way, just as expanded explanations for what this email says in
shorter form.

-Karl

-- 
www.collab.net  <>  CollabNet  |  Distributed Development On Demand

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org

Re: [PATCH] Flush stdout more often

Posted by Stefan Haller <ha...@ableton.com>.
Justin Erenkrantz <ju...@erenkrantz.com> wrote:

> Yes, I still object.  I pointed out what my objections are to this
> patch and others have also outlined several strategies (custom
> notification callbacks or pty's) that can be undertaken by you to
> resolve your specific problem without cluttering Subversion's core
> code and to do so in such a way that will resolve this issue for prior
> releases too - which your patch can not do.

Others have pointed out that the patch is not only useful to my
particular problem, but also when you redirect output to a file and look
at it with tail -f, or pipe it through tee, things like that.  You
assert that nobody cares about that, which I don't believe.  Why should
this be less important for svn than it is for make or other tools that
produce line-based output?

As for my specific problem: to me, using a pty feels like much more the
wrong solution than having svn flush more often.

And a custom notification callback: maybe I don't understand well enough
what you are suggesting, so could you please briefly outline what you
think I should do in order to post-process the output of, say,
"svn merge".  I want it to behave exactly like svn merge, with the same
option handling (including future enhancements); I don't see how I can
do that without completely reimplementing merge-cmd.c and notify.c in
python.

> > Does it help if I collect some timing data of some sort?  If
> > so, what exactly would be useful?
> 
> Any timing data that you could produce would not address my
> fundamental concerns that the core is the wrong place to address this.

I don't get it.  You have heard several different reasons why the change
can be useful to some people; and you have heard that the change does no
harm (actually, makes no difference at all) for everybody else.  Still
you object to it, "just because"?


-- 
Stefan Haller
Ableton
http://www.ableton.com/

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org

Re: [PATCH] Flush stdout more often

Posted by Justin Erenkrantz <ju...@erenkrantz.com>.
On 3/6/06, Stefan Haller <ha...@ableton.com> wrote:
> I'm told that this patch won't be committed because no consensus was
> reached.  Frankly, my impression was that a consensus *was* reached:
> many people gave good reasons why the patch is useful, and you stopped
> arguing against them.  But apparently that's not what people call
> "consensus" here, so I'm unsure how to proceed with this issue.

Consensus is when everyone agrees.

> Do you still object to the patch after everything that was said in this
> thread?

Yes, I still object.  I pointed out what my objections are to this
patch and others have also outlined several strategies (custom
notification callbacks or pty's) that can be undertaken by you to
resolve your specific problem without cluttering Subversion's core
code and to do so in such a way that will resolve this issue for prior
releases too - which your patch can not do.

> Does it help if I collect some timing data of some sort?  If
> so, what exactly would be useful?

Any timing data that you could produce would not address my
fundamental concerns that the core is the wrong place to address this.
   -- justin

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org


Re: [PATCH] Flush stdout more often

Posted by Bart Robinson <lo...@pobox.com>.
On 2006-2-10 Justin Erenkrantz <ju...@erenkrantz.com> wrote:
 > On 2/10/06, Garrett Rooney <ro...@electricjellyfish.net> wrote:
 > > I can't imagine that the flush is going to even show up in profiling...
 > 
 > It's an extra syscall for every line of output (imagine redirecting
 > stdout to a file!).  We've never needed it before...

The output is line-based, so why not play nice and make it
line-buffered?  (CVS does this FWIW.)

Note that the extra syscall is only for the less-common case of
writing to a pipe or file.  The typical case of writing to a
terminal is already line-buffered by stdio and so doing the
explicit fflush is effectively a no-op (in regards to system
calls).

-- bart

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org

Re: [PATCH] Flush stdout more often

Posted by Branko Čibej <br...@xbc.nu>.
Justin Erenkrantz wrote:
> On 2/10/06, Garrett Rooney <ro...@electricjellyfish.net> wrote:
>   
>> I can't imagine that the flush is going to even show up in profiling...
>>     
>
> It's an extra syscall for every line of output (imagine redirecting
> stdout to a file!).  We've never needed it before...
>   
fflush isnt' a syscall.

-- Brane


---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org

Re: [PATCH] Flush stdout more often

Posted by Vincent Lefevre <vi...@vinc17.org>.
On 2006-02-10 12:07:01 -0800, Justin Erenkrantz wrote:
> There's a point that if a developer wants to write a tool on top of
> SVN, they should be using the bindings not asking us to make changes
> in our command-line program so their tool works better.  -- justin

It's much simpler to write scripts using the svn command-line output
than to use the bindings. This is really the point of a pipe: a very
simple interface between tools.

Are the bindings available in any language anyway?

-- 
Vincent Lefèvre <vi...@vinc17.org> - Web: <http://www.vinc17.org/>
100% accessible validated (X)HTML - Blog: <http://www.vinc17.org/blog/>
Work: CR INRIA - computer arithmetic / SPACES project at LORIA

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org

Re: [PATCH] Flush stdout more often

Posted by Michael Sinz <Mi...@sinz.org>.
Justin Erenkrantz wrote:
> On 2/11/06, Peter N. Lundblad <pe...@famlundblad.se> wrote:
>> I just checked out a copy of trunk from the svn repository. That produced
>> 1367 lines of output and strace reports 123426 syscalls (that's the number
>> of lines output by strace). So, in the case where fflush *is* an extra
>> syscall per line, would you mind estimating the actual overhead in time?
>> (This is a rethorical question, sorry). I think this is microoptimization.
> 
> The addition of this patch is cruft to support a tool that shouldn't
> be dependent upon line-buffering at all.  There are far cleaner ways
> to build applications on top of Subversion that don't require us to
> put in ugly hacks in our code.

I don't fully understand why this is considered an ugly hack.  Nor do I
understand the next point...

>> Note that we already have a fflush (per character) when we output the dots
>> while transfering files, because we want the dots to show up immediately.
> 
> Yes, but that's because we feel we need it for our users.  We don't
> put it there because someone doesn't like our default notification
> callback.

I don't know about others, but let us not forget the history of Unix systems
and the power and versatility of pulling together tools via just the STDIO
command line programs.  This provides major flexibility and loosely coupled
systems that can scale amazingly well.  (And, yes, manytimes that means
less efficient but efficient enough is all that really matters...)

>> As others pointed out, being script friendly is important.  Why do we
>> otherwise have a locale-independent date in the log output (and the number
>> of lines in the log message printed)?
> 
> Parsable?  Sure.  That level of streamy?  Nah.  -- justin

Streamy is a good thing - think of the system scalability by having it more
streamy - more parallelism, which multi-processor systems can really take
advantage of.  (cmd1 | cmd2 | cmd3 with streamy commands scales beautifully
into a 3+ processor setup)

-- 
Michael Sinz                     Technology and Engineering Director/Consultant
"Starting Startups"                                mailto:michael.sinz@sinz.org
My place on the web                            http://www.sinz.org/Michael.Sinz

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org

Re: [PATCH] Flush stdout more often

Posted by Tobias Ringström <to...@ringstrom.mine.nu>.
Justin Erenkrantz wrote:
> On 2/11/06, Peter N. Lundblad <pe...@famlundblad.se> wrote:
>   
>> I just checked out a copy of trunk from the svn repository. That produced
>> 1367 lines of output and strace reports 123426 syscalls (that's the number
>> of lines output by strace). So, in the case where fflush *is* an extra
>> syscall per line, would you mind estimating the actual overhead in time?
>> (This is a rethorical question, sorry). I think this is microoptimization.
>>     
>
> The addition of this patch is cruft to support a tool that shouldn't
> be dependent upon line-buffering at all.  There are far cleaner ways
> to build applications on top of Subversion that don't require us to
> put in ugly hacks in our code.
>   
Forcing line buffering would of course be silly, but it's not what the 
suggested patch does. The point is to flush when there's a good chance 
that there will be a pause in the output. It makes the behavior when 
piping the output into a pager much nicer. It's not uncommon for command 
line applications to behave in the suggested way. As was mentioned 
earlier, CVS does it, and so does make. As Peter demonstrated, the patch 
has no measurable impact on performance, and I don't see how it could 
ever become a maintenance burden, so I'm much in favor of the patch 
being applied.

/Tobias


---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org

Re: [PATCH] Flush stdout more often

Posted by Justin Erenkrantz <ju...@erenkrantz.com>.
On 2/11/06, Peter N. Lundblad <pe...@famlundblad.se> wrote:
> I just checked out a copy of trunk from the svn repository. That produced
> 1367 lines of output and strace reports 123426 syscalls (that's the number
> of lines output by strace). So, in the case where fflush *is* an extra
> syscall per line, would you mind estimating the actual overhead in time?
> (This is a rethorical question, sorry). I think this is microoptimization.

The addition of this patch is cruft to support a tool that shouldn't
be dependent upon line-buffering at all.  There are far cleaner ways
to build applications on top of Subversion that don't require us to
put in ugly hacks in our code.

> Note that we already have a fflush (per character) when we output the dots
> while transfering files, because we want the dots to show up immediately.

Yes, but that's because we feel we need it for our users.  We don't
put it there because someone doesn't like our default notification
callback.

> As others pointed out, being script friendly is important.  Why do we
> otherwise have a locale-independent date in the log output (and the number
> of lines in the log message printed)?

Parsable?  Sure.  That level of streamy?  Nah.  -- justin

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org


Re: [PATCH] Flush stdout more often

Posted by "Peter N. Lundblad" <pe...@famlundblad.se>.
On Fri, 10 Feb 2006, Justin Erenkrantz wrote:

> On 2/10/06, Garrett Rooney <ro...@electricjellyfish.net> wrote:
> > I can't imagine that the flush is going to even show up in profiling...
>
> It's an extra syscall for every line of output (imagine redirecting
> stdout to a file!).  We've never needed it before...
>
Justin,

I just checked out a copy of trunk from the svn repository. That produced
1367 lines of output and strace reports 123426 syscalls (that's the number
of lines output by strace). So, in the case where fflush *is* an extra
syscall per line, would you mind estimating the actual overhead in time?
(This is a rethorical question, sorry). I think this is microoptimization.

Note that we already have a fflush (per character) when we output the dots
while transfering files, because we want the dots to show up immediately.

> > And as for not intending the command line program to produce parseable
> > output, it's the second to last thing on our list of features on
> > subversion.tigris.org.
>
> There's a point that if a developer wants to write a tool on top of
> SVN, they should be using the bindings not asking us to make changes
> in our command-line program so their tool works better.  -- justin
>
As others pointed out, being script friendly is important.  Why do we
otherwise have a locale-independent date in the log output (and the number
of lines in the log message printed)?

Regards,
//Peter

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org

Re: [PATCH] Flush stdout more often

Posted by Greg Hudson <gh...@MIT.EDU>.
On Fri, 2006-02-10 at 12:07 -0800, Justin Erenkrantz wrote:
> On 2/10/06, Garrett Rooney <ro...@electricjellyfish.net> wrote:
> > I can't imagine that the flush is going to even show up in profiling...
> 
> It's an extra syscall for every line of output (imagine redirecting
> stdout to a file!).  We've never needed it before...

When the output is a terminal, that's a syscall which happens anyway.


---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org

Re: [PATCH] Flush stdout more often

Posted by Vincent Lefevre <vi...@vinc17.org>.
On 2006-02-10 22:19:05 -0300, Nicolás Lichtmaier wrote:
> [1] = This flush would only be for status output, not for commands
> like "svn cat" or "svn diff".

I think there would be a benefit for "svn diff" too (not after every
line since it would be useless, but after every file).

-- 
Vincent Lefèvre <vi...@vinc17.org> - Web: <http://www.vinc17.org/>
100% accessible validated (X)HTML - Blog: <http://www.vinc17.org/blog/>
Work: CR INRIA - computer arithmetic / SPACES project at LORIA

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org

Re: [PATCH] Flush stdout more often

Posted by Nicolás Lichtmaier <ni...@reloco.com.ar>.
>> I can't imagine that the flush is going to even show up in profiling...
>>     
>
> It's an extra syscall for every line of output (imagine redirecting
> stdout to a file!).  We've never needed it before...
>   

Even in this case it would be nice to flush every line. Redirecting to a 
file a long running operation is a way to create a log file of the 
operation [1]. And it's nice to be able to do a "tail -f" on that. (Not 
that I can think of any command which would take so long in subversion 
that somebody would want to redirect it to a file and run it in 
background...)

[1] = This flush would only be for status output, not for commands like 
"svn cat" or "svn diff".


---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org

Re: [PATCH] Flush stdout more often

Posted by Mattias Engdeg�rd <ma...@virtutech.se>.
Justin Erenkrantz <ju...@erenkrantz.com> writes:
>There's a point that if a developer wants to write a tool on top of
>SVN, they should be using the bindings not asking us to make changes
>in our command-line program so their tool works better.  -- justin

No bindings required in this case. Just use a pty.


---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org

Re: [PATCH] Flush stdout more often

Posted by Justin Erenkrantz <ju...@erenkrantz.com>.
On 2/10/06, Garrett Rooney <ro...@electricjellyfish.net> wrote:
> I can't imagine that the flush is going to even show up in profiling...

It's an extra syscall for every line of output (imagine redirecting
stdout to a file!).  We've never needed it before...

> And as for not intending the command line program to produce parseable
> output, it's the second to last thing on our list of features on
> subversion.tigris.org.

There's a point that if a developer wants to write a tool on top of
SVN, they should be using the bindings not asking us to make changes
in our command-line program so their tool works better.  -- justin

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org


Re: [PATCH] Flush stdout more often

Posted by Garrett Rooney <ro...@electricjellyfish.net>.
On 2/10/06, Justin Erenkrantz <ju...@erenkrantz.com> wrote:

> Checkout, etc. would have a flush put in after *every* line?  Yikes.
> The proper way to do this is to call the bindings and install your own
> notification callback.  We have never intended for the command-line
> program to produce parsable output - that's why we have an API.

I can't imagine that the flush is going to even show up in profiling...

And as for not intending the command line program to produce parseable
output, it's the second to last thing on our list of features on
subversion.tigris.org.

-garrett

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org


Re: [PATCH] Flush stdout more often

Posted by "C. Michael Pilato" <cm...@collab.net>.
Justin Erenkrantz wrote:

> Checkout, etc. would have a flush put in after *every* line?  Yikes. 
> The proper way to do this is to call the bindings and install your own
> notification callback.  We have never intended for the command-line
> program to produce parsable output - that's why we have an API.  --

Hey, I know!  Let's introduce a new global option space, with an initial
member of a new --per-line-flushitude option!

/me ducks and runs (screaming, I might add).

-- 
C. Michael Pilato <cm...@collab.net>
CollabNet   <>   www.collab.net   <>   Distributed Development On Demand

Re: [PATCH] Flush stdout more often

Posted by Stefan Haller <ha...@ableton.com>.
Justin Erenkrantz <ju...@erenkrantz.com> wrote:

> This introduces unnecessary overhead for those people who don't care about
> such things (which is almost everybody).

No, it doesn't create additional overhead for people who run svn in a
terminal without redirecting stdout, which is almost everybody.  In that
case stdout is already line-buffered anyway.

> The proper way to do this is to call the bindings and install your own
> notification callback.

I really don't want to have to have to reimplement all of
subversion/svn/status.c in python just so that I can add a tiny little
bit of functionality on top of it.  This is so much less effort with a
/bin/sh post-processor.


-- 
Stefan Haller
Ableton
http://www.ableton.com/

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org

Re: [PATCH] Flush stdout more often

Posted by Justin Erenkrantz <ju...@erenkrantz.com>.
On 2/10/06, Julian Foad <ju...@btopenworld.com> wrote:
> Stefan's log message said why we would want to do this.  He first discussed it
> on the list: <http://svn.haxx.se/dev/archive-2006-02/0544.shtml>

Yes, I had seen that.

> Do you think the overhead would be significant?  (Note, this is for commands
> that print approximately one line of status output per item, not for those that
> print big chunks of text like "cat", "diff", "blame".)

Checkout, etc. would have a flush put in after *every* line?  Yikes. 
The proper way to do this is to call the bindings and install your own
notification callback.  We have never intended for the command-line
program to produce parsable output - that's why we have an API.  --
justin

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org


Re: [PATCH] Flush stdout more often

Posted by Julian Foad <ju...@btopenworld.com>.
Justin Erenkrantz wrote:
> On 2/10/06, Stefan Haller <ha...@ableton.com> wrote:
>>Peter N. Lundblad <pe...@famlundblad.se> wrote:
>>
>>>>Should I prepare a patch?
>>>
>>>Please:-)
>>
>>Here it is.  It's my first patch for subversion, so please bear with me
>>if I did something stupid.
> 
> Why would we want to do this?  This introduces unnecessary overhead
> for those people who don't care about such things (which is almost
> everybody).   -- justin

Stefan's log message said why we would want to do this.  He first discussed it 
on the list: <http://svn.haxx.se/dev/archive-2006-02/0544.shtml>

Do you think the overhead would be significant?  (Note, this is for commands 
that print approximately one line of status output per item, not for those that 
print big chunks of text like "cat", "diff", "blame".)

- Julian

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org

Re: [PATCH] Flush stdout more often

Posted by Stefan Haller <ha...@ableton.com>.
Justin Erenkrantz <ju...@erenkrantz.com> wrote:

> On 2/10/06, Stefan Haller <ha...@ableton.com> wrote:
>
> > Here it is.  It's my first patch for subversion, so please bear with me
> > if I did something stupid.
> 
> Why would we want to do this?  This introduces unnecessary overhead
> for those people who don't care about such things (which is almost
> everybody).   -- justin

Justin,

I'm told that this patch won't be committed because no consensus was
reached.  Frankly, my impression was that a consensus *was* reached:
many people gave good reasons why the patch is useful, and you stopped
arguing against them.  But apparently that's not what people call
"consensus" here, so I'm unsure how to proceed with this issue.

Do you still object to the patch after everything that was said in this
thread?  Does it help if I collect some timing data of some sort?  If
so, what exactly would be useful?


-- 
Stefan Haller
Ableton
http://www.ableton.com/

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org

Re: [PATCH] Flush stdout more often

Posted by Justin Erenkrantz <ju...@erenkrantz.com>.
On 2/10/06, Stefan Haller <ha...@ableton.com> wrote:
> Peter N. Lundblad <pe...@famlundblad.se> wrote:
>
> > > Should I prepare a patch?
> >
> > Please:-)
>
> Here it is.  It's my first patch for subversion, so please bear with me
> if I did something stupid.

Why would we want to do this?  This introduces unnecessary overhead
for those people who don't care about such things (which is almost
everybody).   -- justin

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org


Re: [PATCH] Flush stdout more often

Posted by "Peter N. Lundblad" <pe...@famlundblad.se>.
On Fri, 10 Feb 2006, Stefan Haller wrote:

> Peter N. Lundblad <pe...@famlundblad.se> wrote:
>
> > > Should I prepare a patch?
> >
> > Please:-)
>
> Here it is.  It's my first patch for subversion, so please bear with me
> if I did something stupid.
>
Thanks, I put it on my TODO list.  It looks good at a quick glance, but
I've not tested it yet.

Thanks,
//Peter

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org

[PATCH] Flush stdout more often

Posted by Stefan Haller <ha...@ableton.com>.
Peter N. Lundblad <pe...@famlundblad.se> wrote:

> > Should I prepare a patch?
> 
> Please:-)

Here it is.  It's my first patch for subversion, so please bear with me
if I did something stupid.


-- 
Stefan Haller
Ableton
http://www.ableton.com/


[[[
Flush stdout after every line that is printed (for most commands, except
for diff and blame).  This is useful when piping svn output into
post-processor tools, e.g. to colorize the output, or simply into tee.

* subversion/svn/diff-cmd.c
  (summarize_func): Flush output after every line printed.

* subversion/svn/status.c
  (print_status): Flush output after every line printed.

* subversion/svn/notify.c
  (notify): Flush output after every line printed.
]]]


Index: ../trunk/subversion/svn/diff-cmd.c
===================================================================
--- ../trunk/subversion/svn/diff-cmd.c  (revision 18413)
+++ ../trunk/subversion/svn/diff-cmd.c  (working copy)
@@ -84,6 +84,8 @@
                                summary->copyfrom_path ? '+' : ' ',
                                path));
 
+  SVN_ERR (svn_cmdline_fflush(stdout));
+
   return SVN_NO_ERROR;
 }
 
Index: ../trunk/subversion/svn/status.c
===================================================================
--- ../trunk/subversion/svn/status.c    (revision 18413)
+++ ../trunk/subversion/svn/status.c    (working copy)
@@ -191,6 +191,8 @@
                             ? 'K' : ' '),
                            path));
 
+  SVN_ERR (svn_cmdline_fflush(stdout));
+
   return SVN_NO_ERROR;
 }
 
Index: ../trunk/subversion/svn/notify.c
===================================================================
--- ../trunk/subversion/svn/notify.c    (revision 18413)
+++ ../trunk/subversion/svn/notify.c    (working copy)
@@ -368,6 +368,9 @@
       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

Re: Stdout buffering issue

Posted by "Peter N. Lundblad" <pe...@famlundblad.se>.
On Fri, 10 Feb 2006, Stefan Haller wrote:

> Peter N. Lundblad <pe...@famlundblad.se> wrote:
>
> > On Thu, 9 Feb 2006, Stefan Haller wrote:
> >
> > > So I think that the output of co, st, up, switch, and merge should be
> > > line-buffered always.  (Diff and blame output shouldn't, of course.)
> > >
> > I think it would be reasonable to flush the output at each call of
> > notify() in subversion/svn/notify.c.
>
> OK, but this wouldn't help for status.  Do you think there should be a
> flush in print_status() in addition to notify()?
>
Ouch, you're right there.

> Should I prepare a patch?

Please:-)

Thanks,
//Peter

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org

Re: Stdout buffering issue

Posted by Stefan Haller <ha...@ableton.com>.
Peter N. Lundblad <pe...@famlundblad.se> wrote:

> On Thu, 9 Feb 2006, Stefan Haller wrote:
> 
> > So I think that the output of co, st, up, switch, and merge should be
> > line-buffered always.  (Diff and blame output shouldn't, of course.)
> >
> I think it would be reasonable to flush the output at each call of
> notify() in subversion/svn/notify.c.

OK, but this wouldn't help for status.  Do you think there should be a
flush in print_status() in addition to notify()?

Should I prepare a patch?


-- 
Stefan Haller
Ableton
http://www.ableton.com/

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org

Re: Stdout buffering issue

Posted by "Peter N. Lundblad" <pe...@famlundblad.se>.
On Thu, 9 Feb 2006, Stefan Haller wrote:

> So I think that the output of co, st, up, switch, and merge should be
> line-buffered always.  (Diff and blame output shouldn't, of course.)
>
I think it would be reasonable to flush the output at each call of
notify() in subversion/svn/notify.c.

Regards,
//Peter

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org