You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by kf...@collab.net on 2005/08/04 20:14:56 UTC
Re: svn status enhancement
Scott Palmer <sc...@2connected.org> writes:
> Interesting, but I fear that issue is going too far for what I want.
>
> It seems that issue 1935 proposes to suppress ALL info about
> externals unless the "-v" option is given. In which case the output
> would be the same as it is now with a bunch of "empty" status info.
>
> I want to get status output for externals that actually have some
> status to report. But I don't want the "Performing status on
> external item at..." message when the status for that external item
> would be empty.
>
> Am I clearly communicating the difference?
Yes; sorry for not realizing that before.
I think your enhancement is basically different from what issue #1935
discusses, though if your enhancement were implemented, it would
reduce the urgency of #1935 by a lot. You might want to make a note
of this thread in #1935, therefore.
If you want to file an enhancement issue, pointing to this thread,
that would be great, but without a patch I fear it might linger for a
long time. Tell you what: if you have time to come up with a patch,
and send it to the dev@ list, I'll at least do a full review. I can't
promise to commit it the first time obviously, but it shouldn't be a
huge patch, and once we get it ship-shape (however many turnarounds
that takes) I see no reason it can't be committed. I've CC'd the dev@
list in case anyone has objections.
Don't forget to read
http://subversion.tigris.org/mailing-list-guidelines.html#patches
and the relevant parts of
http://subversion.tigris.org/hacking.html
-Karl
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn status enhancement
Posted by Julian Foad <ju...@btopenworld.com>.
Scott Palmer wrote:
>
> But my solution has given me an idea for implementing what is proposed
> in issue #1935. I will likely have something later tonight.
Cool.
> BTW, why isn't the existing patch attached to that issue acceptable? I
> see that it changes a lot more than the status command.
Hmm. I haven't looked at it to see whether it contains useful code. The fact
that it changes a lot more than the "status" command is enough reason to reject
it as a solution for that particular issue.
> I want to add a new header file "notify.h" so I can share the
> notify_baton struct definition (with my changes) with status_cmd.c. Is
> that going to be a problem? (I've currently just pasted a duplicate
> definition at the top of status_cmd.c.)
Just put it in "cl.h". No need for a new header file for a little bit of
private stuff. Prefix it with "svn_cl__".
- Julian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn status enhancement
Posted by Scott Palmer <sc...@2connected.org>.
On 5-Aug-05, at 1:58 PM, kfogel@collab.net wrote:
> "C. Michael Pilato" <cm...@collab.net> writes:
>
>> Scott Palmer <sc...@2connected.org> writes:
>>
>>> Am I correct then in thinking (what I originally thought, but didn't
>>> fully grok) that if I think I need to change anything outside of the
>>> subversion/clients/cmdline folder that I need to slap my self
>>> back to
>>> reality?
>>
>> Sounds correct to me. Though, be gentle when slapping. Maybe "love
>> taps" are more in order, considering this is your first trip into our
>> codebase and all.
>
> I don't know about this. I looked at this problem in some depth, and
> I doubt it can be solved just under subversion/clients/cmdline/. At
> any rate I don't think you should be surprised if changes start being
> required elsewhere...
I managed to get something hacked together does what I wanted
originally. That is, it only prints the "Performing status on..."
message when there is going to be output from the external folder.
And all the changes are contained within the cmdline folder.
But based, on issue #1935 and Julian's comments, this isn't really a
solution to issue #1935, since in that case the "Performing
status..." message shouldn't print either, unless the -v flag was
specified.
But my solution has given me an idea for implementing what is
proposed in issue #1935. I will likely have something later tonight.
BTW, why isn't the existing patch attached to that issue acceptable?
I see that it changes a lot more than the status command.
I want to add a new header file "notify.h" so I can share the
notify_baton struct definition (with my changes) with status_cmd.c.
Is that going to be a problem? (I've currently just pasted a
duplicate definition at the top of status_cmd.c.)
Scott
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn status enhancement
Posted by kf...@collab.net.
"C. Michael Pilato" <cm...@collab.net> writes:
> Scott Palmer <sc...@2connected.org> writes:
> > Am I correct then in thinking (what I originally thought, but didn't
> > fully grok) that if I think I need to change anything outside of the
> > subversion/clients/cmdline folder that I need to slap my self back to
> > reality?
>
> Sounds correct to me. Though, be gentle when slapping. Maybe "love
> taps" are more in order, considering this is your first trip into our
> codebase and all.
I don't know about this. I looked at this problem in some depth, and
I doubt it can be solved just under subversion/clients/cmdline/. At
any rate I don't think you should be surprised if changes start being
required elsewhere...
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn status enhancement
Posted by "C. Michael Pilato" <cm...@collab.net>.
Scott Palmer <sc...@2connected.org> writes:
> Am I correct then in thinking (what I originally thought, but didn't
> fully grok) that if I think I need to change anything outside of the
> subversion/clients/cmdline folder that I need to slap my self back to
> reality?
Sounds correct to me. Though, be gentle when slapping. Maybe "love
taps" are more in order, considering this is your first trip into our
codebase and all.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn status enhancement
Posted by Julian Foad <ju...@btopenworld.com>.
Scott Palmer wrote:
>
> On 5-Aug-05, at 1:51 PM, Julian Foad wrote:
>
>> I'm fairly sure that you and the people who raised issue #1935 ("svn
>> status too verbose with svn:externals definitions") want the same
>> thing, ...
>
> If your example #2 is what is proposed by issue #1935, then yes, I want
> the same thing.
It was my best understanding of what the participants in the issue agreed they
wanted.
>> Do you want that message when the status for that item is non-empty?
>
> I was undecided.
>
>> Do you really want it, or would you be happy either way?
>
> But I think I will be happy either way.
>
>> (3) What do you want? Copy (2) and see if any field that you care
>> strongly about should differ from it. If not, then we're all happy.
>
> We are all happy, or will be when issue #1935 is closed.
Fantastic. I look forward to your patch.
> My primary problem with coming up with a patch is that I am so
> unfamiliar with the code and your style/conventions. I have never used
> the APR lib before, etc.. But I'm making progress quickly.
I hope you'll quickly grow to like it. It opened my eyes to how badly the
software was written in my day job, and how well it could be done. I love it.
- Julian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn status enhancement
Posted by Scott Palmer <sc...@2connected.org>.
On 5-Aug-05, at 1:51 PM, Julian Foad wrote:
> I'm fairly sure that you and the people who raised issue #1935
> ("svn status too verbose with svn:externals definitions") want the
> same thing, ...
If your example #2 is what is proposed by issue #1935, then yes, I
want the same thing.
> Do you want that message when the status for that item is non-empty?
I was undecided.
> Do you really want it, or would you be happy either way?
But I think I will be happy either way.
> (3) What do you want? Copy (2) and see if any field that you care
> strongly about should differ from it. If not, then we're all happy.
We are all happy, or will be when issue #1935 is closed.
My primary problem with coming up with a patch is that I am so
unfamiliar with the code and your style/conventions. I have never
used the APR lib before, etc.. But I'm making progress quickly.
Scott
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn status enhancement
Posted by Julian Foad <ju...@btopenworld.com>.
I'm fairly sure that you and the people who raised issue #1935 ("svn status too
verbose with svn:externals definitions") want the same thing, really, though
maybe it was expressed with slightly different ideas of how it would behave in
detail. I think you should bear in mind what's said in that issue, maybe
compromise a bit, and aim for your solution to be also an acceptable solution
to that.
Note that your mail thread was brought to this "dev" list after you had started
discussing it, and we haven't actually heard your proposed behaviour stated
first-hand here yet. You were quoted as saying,
> It seems that issue 1935 proposes to suppress ALL info about
> externals unless the "-v" option is given.
No. It wanted to suppress the "Performing ..." lines, and the "X" lines if
"-q" given, but not the statuses of items within an external directory. (The
"--ignore-externals" switch prevents looking into the external directories, so
should remove the "Performing ..." lines and the statuses of items within an
external directory.)
> In which case the output
> would be the same as it is now with a bunch of "empty" status info.
I don't know what that sentence means exactly.
> I want to get status output for externals that actually have some
> status to report. But I don't want the "Performing status on
> external item at..." message when the status for that external item
> would be empty.
OK, but that's incomplete. Do you want that message when the status for that
item is non-empty? Do you really want it, or would you be happy either way?
For what combinations of "-q" and "-v" do you want it?
Issue #1935 also doesn't say explicitly whether the "Performing ..." message is
wanted when there are statuses to display, but it (the July 2004 comment from
Scott Lawrence) strongly implies that the presence or absence of the message
should be controlled only by the switches and independent of whether changes exist.
** Fixed-width font required for viewing this next bit **
Here are tables showing which of the three kinds of information are output in
various circumstances.
(1) As I believe it is implemented at present:
Options (none) --ignore-externals
------------------------------- -------------------------------
-q (none) -v -q (none) -v
-------- --------- --------- --------- --------- --------- ---------
Changes X dir X dir X dir X dir
Perfor... Perfor... Perfor...
S dir/foo S dir/foo S dir/foo
-------- --------- --------- --------- --------- --------- ---------
No change X dir X dir X dir X dir
Perfor... Perfor... Perfor...
-------- --------- --------- --------- --------- --------- ---------
(2) As I believe is proposed in issue #1935. The difference from (1) is that
"Performing..." is not printed unless "-v" is specified.
Options (none) --ignore-externals
------------------------------- -------------------------------
-q (none) -v -q (none) -v
-------- --------- --------- --------- --------- --------- ---------
Changes X dir X dir X dir X dir
Perfor...
S dir/foo S dir/foo S dir/foo
-------- --------- --------- --------- --------- --------- ---------
No change X dir X dir X dir X dir
Perfor...
-------- --------- --------- --------- --------- --------- ---------
(3) What do you want? Copy (2) and see if any field that you care strongly
about should differ from it. If not, then we're all happy.
I've still been incomplete, omitting the "-q -v" case which is apparently used
by the test suite, but tough, that'll just have to be changed to cope with
whatever "-q -v" does. I don't think we need to specify a publically defined
behaviour for "status -q -v" even though we do for "log".
More recently, Scott Palmer wrote:
> Am I correct then in thinking (what I originally thought, but didn't
> fully grok) that if I think I need to change anything outside of the
> subversion/clients/cmdline folder that I need to slap my self back to
> reality?
Although you ought to be able to solve this within subversion/clients/cmdline,
as C-Mike Pilato says, there's no guarantee that the client library interface
is beautifully designed. It's best not to try changing it in your first patch
but if, after working with it and getting familiar with how it fits into the
software, you have found it really cumbersome, do consider discussing it.
- Julian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn status enhancement
Posted by Scott Palmer <sc...@2connected.org>.
On 5-Aug-05, at 1:26 AM, C. Michael Pilato wrote:
> Scott Palmer <sc...@2connected.org> writes:
>
>
>> I identified two places that seemed to be where the modifications are
>> required. In externals.c the method svn_client__do_external_status
>> (), and in status.c: svn_client_status2()
>>
>
> First, sorry for not mailing earlier and saving you the investigative
> trip into libsvn_wc (from whence many never return). But
> unfortunately, I think neither of these is correct. Your goal is to
> eliminate some output, but you need to do this without eliminating
> data other users of libsvn_client might actually (continue to) want.
>
> To do this correctly, you need to get your status callback and your
> notification callback talking to each other. I'm thinking you could
> do this by making the notification baton semi-public and putting a
> pointer to it in the status callback baton. With creative use of
> booleans and logic, you should be able to choreograph the exotic dance
> needed to solve the problem you aim to solve.
There is so much of the design that I am unfamiliar with... I had a
feeling I was in trouble :-).
After reading your message and taking a fresh look I think I have a
better idea of how things work.
Am I correct then in thinking (what I originally thought, but didn't
fully grok) that if I think I need to change anything outside of the
subversion/clients/cmdline folder that I need to slap my self back to
reality?
Scott
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn status enhancement
Posted by "C. Michael Pilato" <cm...@collab.net>.
Scott Palmer <sc...@2connected.org> writes:
> I identified two places that seemed to be where the modifications are
> required. In externals.c the method svn_client__do_external_status
> (), and in status.c: svn_client_status2()
First, sorry for not mailing earlier and saving you the investigative
trip into libsvn_wc (from whence many never return). But
unfortunately, I think neither of these is correct. Your goal is to
eliminate some output, but you need to do this without eliminating
data other users of libsvn_client might actually (continue to) want.
To do this correctly, you need to get your status callback and your
notification callback talking to each other. I'm thinking you could
do this by making the notification baton semi-public and putting a
pointer to it in the status callback baton. With creative use of
booleans and logic, you should be able to choreograph the exotic dance
needed to solve the problem you aim to solve.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn status enhancement
Posted by Scott Palmer <sc...@2connected.org>.
On 4-Aug-05, at 4:14 PM, kfogel@collab.net wrote:
> Tell you what: if you have time to come up with a patch,
> and send it to the dev@ list, I'll at least do a full review. I can't
> promise to commit it the first time obviously, but it shouldn't be a
> huge patch, and once we get it ship-shape (however many turnarounds
> that takes) I see no reason it can't be committed. I've CC'd the dev@
> list in case anyone has objections.
I took a look at the code just to see how simple this might be. I've
never actually looked at the Subversion code before.
I'm lost.
Well not entirely. I pulled down r15583 of http://svn.collab.net/
repos/svn/branches/1.2.x, that's the branch I occasionally compile
svn from. I figured for now I would stay out of trunk, though I
think you would want a patch against the trunk.
I identified two places that seemed to be where the modifications are
required. In externals.c the method svn_client__do_external_status
(), and in status.c: svn_client_status2()
Now somehow I have to suppress the message printed in
svn_client__do_external_status(), but only if svn_client_status2()
isn't going to output anything. That's basically not possible
(unless I'm missing something big) so the message printing in
svn_client__do_external_status() has to be relocated to
svn_client_status2(). svn_client_status2() needs to be made aware of
when it is processing status for an external item and trigger the
printing of the message, just once, if it finds that it has something
to say.
So far, so good. but not knowing a thing about the structure or
conventions used in the code I'm not sure what the right way would be
to pass the needed information into svn_client_status2(). It looks
like the options are: via svn_client_ctx_t or add another parameter
to svn_client_status2(). But it seems those things are not to be
changed lightly, e.g. the '2' is there so the original remains for
backward compatibility. I don't know how quickly I should jump to
make a svn_client_status3()...
I'm afraid I'm going to take way too much time to come up to speed.
I have time to make a quick patch. I don't think I have time to
learn enough (about the Subversion code) to actually do it though :(
I haven't given up entirely yet though...
Scott
P.S. I'm not subscribed to the dev list, so please copy my on any
replies.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org