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/04/09 16:28:43 UTC
Re: [PATCH] Flush stdout more often
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>.
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