You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Mark Grosberg <ma...@nolab.conman.org> on 2003/05/09 04:46:52 UTC
[PATCH] Issue #1295 (take 2)
Okay,
Here is my updated patch. I now use a hash table as suggested by Brane to
store the paths we've seen. Also, qsort() is now used to remove
the "UNMARKED" entries.
I also added an error message if no entries are specified. I've tested
this with several combinations and pathalogical cases on my test repos.
Let me know what you think. I would love to see this merged. This nit
bugs me in CVS daily.
L8r,
Mark G.
-----------------------------------------------------------------------
Index: include/svn_error_codes.h
===================================================================
--- include/svn_error_codes.h (revision 5857)
+++ include/svn_error_codes.h (working copy)
@@ -688,6 +688,11 @@
SVN_ERR_CLIENT_CATEGORY_START + 8,
"Revision range is not allowed")
+ SVN_ERRDEF (SVN_ERR_CLIENT_NO_WORK,
+ SVN_ERR_CLIENT_CATEGORY_START + 9,
+ "No work to do")
+
+
/* misc errors */
SVN_ERRDEF (SVN_ERR_BASE,
Index: include/svn_client.h
===================================================================
--- include/svn_client.h (revision 5857)
+++ include/svn_client.h (working copy)
@@ -292,6 +292,7 @@
#define SVN_CLIENT_COMMIT_ITEM_TEXT_MODS 0x04
#define SVN_CLIENT_COMMIT_ITEM_PROP_MODS 0x08
#define SVN_CLIENT_COMMIT_ITEM_IS_COPY 0x10
+#define SVN_CLIENT_COMMIT_ITEM_UNMARKED 0x20
/** @} */
/** The commit candidate structure. */
Index: libsvn_client/commit_util.c
===================================================================
--- libsvn_client/commit_util.c (revision 5857)
+++ libsvn_client/commit_util.c (working copy)
@@ -730,6 +730,14 @@
= *((const svn_client_commit_item_t * const *) a);
const svn_client_commit_item_t *item2
= *((const svn_client_commit_item_t * const *) b);
+
+ if ((item1->state_flags & SVN_CLIENT_COMMIT_ITEM_UNMARKED)
+ && (!(item2->state_flags & SVN_CLIENT_COMMIT_ITEM_UNMARKED)))
+ return 1;
+ if ((!(item1->state_flags & SVN_CLIENT_COMMIT_ITEM_UNMARKED))
+ && (item2->state_flags & SVN_CLIENT_COMMIT_ITEM_UNMARKED))
+ return -1;
+
return svn_path_compare_paths (item1->url, item2->url);
}
@@ -751,6 +759,20 @@
qsort (ci->elts, ci->nelts,
ci->elt_size, svn_client__sort_commit_item_urls);
+ /* Delete unrequested items. */
+ for (i = ci->nelts; i > 0; i--)
+ {
+ item = (((svn_client_commit_item_t **) ci->elts)[i - 1]);
+ if (!(item->state_flags & SVN_CLIENT_COMMIT_ITEM_UNMARKED))
+ break;
+ }
+
+ ci->nelts = i;
+ if (ci->nelts == 0)
+ return svn_error_create (SVN_ERR_CLIENT_NO_WORK, NULL,
+ "No entires submitted for commit.");
+
+
/* Loop through the URLs, finding the longest usable ancestor common
to all of them, and making sure there are no duplicate URLs. */
for (i = 0; i < ci->nelts; i++)
Index: clients/cmdline/util.c
===================================================================
--- clients/cmdline/util.c (revision 5857)
+++ clients/cmdline/util.c (working copy)
@@ -416,6 +416,154 @@
#define EDITOR_EOF_PREFIX "--This line, and those below, will be ignored--"
+/* Get the full path string for the specificed item. */
+static const char *
+get_item_path (svn_client_commit_item_t *item,
+ struct log_msg_baton *lmb,
+ apr_pool_t *pool)
+{
+ const char *item_path = item->path;
+
+ if (! item_path)
+ item_path = item->url;
+ else if (! *item_path)
+ item_path = ".";
+
+ if (item_path && lmb->base_dir)
+ item_path = svn_path_is_child (lmb->base_dir, item_path, pool);
+
+ /* If still no path, then just use current directory. */
+ if (! item_path)
+ item_path = ".";
+
+ return item_path;
+}
+
+/* Parse the edited comitted message and store the named paths
+ in the hash. */
+static void
+process_comitted_list(apr_hash_t *seen,
+ char *commited_list,
+ apr_pool_t *pool)
+{
+ char *end_str, *prev_str = NULL;
+ int i;
+
+ /* Find the generated message. */
+ end_str = commited_list;
+ while (end_str != NULL)
+ {
+ end_str = strstr (end_str, EDITOR_EOF_PREFIX);
+ if (end_str == NULL)
+ break;
+
+ end_str = end_str + (sizeof(EDITOR_EOF_PREFIX) - 1);
+ prev_str = end_str;
+ }
+
+ if (prev_str == NULL)
+ return;
+ while (*prev_str != '\0')
+ {
+ if (!apr_isspace (*prev_str))
+ break;
+ prev_str++;
+ }
+
+ /* Now, parse each line and make sure that the commited item is named. */
+ for (;;)
+ {
+ char *end_of_path;
+
+ /* Skip leading whitespace. */
+ for (;;)
+ {
+ if (*prev_str == '\0')
+ return;
+
+ if (!apr_isspace (*prev_str))
+ break;
+ prev_str++;
+ }
+ if (*prev_str == '\0')
+ break;
+
+ /* The next byte had better be one of the text mod's below. */
+ switch (*prev_str++)
+ {
+ case '_':
+ case 'R':
+ case 'A':
+ case 'D':
+ case 'M':
+ break;
+
+ default:
+ return;
+ }
+
+ /* The next byte had better be one of the prop mod's below. */
+ switch (*prev_str++)
+ {
+ case ' ':
+ case 'M':
+ break;
+
+ default:
+ return;
+ }
+
+ /* Skip more whitespace. */
+ for (;;)
+ {
+ if (*prev_str == '\0')
+ return;
+ if (!apr_isspace(*prev_str))
+ break;
+ prev_str++;
+ }
+
+ /* Chop up the path. */
+ for (end_of_path = prev_str; *end_of_path != '\0'; end_of_path++)
+ {
+ if (*end_of_path == '\n')
+ break;
+ }
+ if (*end_of_path == '\0')
+ return;
+
+ /* Truncate the string. The path is now in prev_str. */
+ *end_of_path++ = '\0';
+
+ /* Note the existence. */
+ apr_hash_set(seen, prev_str, APR_HASH_KEY_STRING, "");
+
+ /* Advance the pointer. */
+ prev_str = end_of_path;
+ }
+}
+
+/* Find the paths that are marked in the seen hash and set their
+ unmarked flags. */
+static void
+flag_unmarked (struct log_msg_baton *lmb,
+ apr_array_header_t *commit_items,
+ apr_hash_t *seen,
+ apr_pool_t *pool)
+{
+ int i;
+
+ for (i = 0; i < commit_items->nelts; i++)
+ {
+ svn_client_commit_item_t *item
+ = ((svn_client_commit_item_t **) commit_items->elts)[i];
+ const char *path = get_item_path (item, lmb, pool);
+
+ if (! apr_hash_get (seen, path, APR_HASH_KEY_STRING))
+ item->state_flags |= SVN_CLIENT_COMMIT_ITEM_UNMARKED;
+ }
+}
+
/* This function is of type svn_client_get_commit_log_t. */
svn_error_t *
svn_cl__get_log_message (const char **log_msg,
@@ -469,21 +617,9 @@
{
svn_client_commit_item_t *item
= ((svn_client_commit_item_t **) commit_items->elts)[i];
- const char *path = item->path;
+ const char *path = get_item_path (item, lmb, pool);
char text_mod = '_', prop_mod = ' ';
- if (! path)
- path = item->url;
- else if (! *path)
- path = ".";
-
- if (path && lmb->base_dir)
- path = svn_path_is_child (lmb->base_dir, path, pool);
-
- /* If still no path, then just use current directory. */
- if (! path)
- path = ".";
-
if ((item->state_flags & SVN_CLIENT_COMMIT_ITEM_DELETE)
&& (item->state_flags & SVN_CLIENT_COMMIT_ITEM_ADD))
text_mod = 'R';
@@ -542,8 +678,16 @@
/* Strip the prefix from the buffer. */
if (message)
+ {
+ char *commited_list = apr_pstrdup (pool, msg2);
+ apr_hash_t *seen = apr_hash_make(pool);
+
+ process_comitted_list (seen, commited_list, pool);
+ flag_unmarked (lmb, commit_items, seen, pool);
+
truncate_buffer_at_prefix (&message->len, message->data,
EDITOR_EOF_PREFIX);
+ }
if (message)
{
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Issue #1295 (take 2, with log)
Posted by kf...@collab.net.
Greg Stein <gs...@lyra.org> writes:
> Ya know... sometimes I get cranky at the bars that are raised. "Oh, come
> on... give yourself two minutes and you can find the log message!" Of
> course, the counter is: multiply that by the number of patches, and it gets
> bothersome.
Yes, you get cranky too often about this :-).
I wasn't raising any bars. I didn't say "No one will look at your
patch until you repost with a log message", I just explained why it
helped to have it, and asked that he repost with it as that helps
everyone else. That is, letting Mark know (earlier rather than later)
what helps people review patches. Giving advice and shutting someone
out are two completely different things; this was the former.
By the way, the more important counterargument -- which I mentioned in
the mail -- is that even after you find the log message, you can't be
sure how well it still applies to the newly posted patch. That
problem is independent of the number of patches posted.
> But! You've been great, Mark. Thanks for the persistence and the
> understanding. The worst thing we can do is frighten off new contributors
> with an arm's length of rules. It's kind of, "come on! give the guy a
> break... apply his patch already, will ya? iterate after that."
(+1 on the "You've been great, Mark" part!)
The patch was not ready for application -- which is no slur on Mark:
considering that it's his first patch, it was pretty amazing.
But there's no "rules for rules' sake" stuff going on here. All the
requests made of Mark were important for the health of the project.
If they happened to coincide with some of our stated policies, well,
that just means those policies aren't completely random :-).
-K
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Issue #1295 (take 2, with log)
Posted by Greg Stein <gs...@lyra.org>.
On Fri, May 09, 2003 at 02:24:35AM -0400, Mark Grosberg wrote:
>
>
> On 9 May 2003 kfogel@collab.net wrote:
>
> > Always include the full log message with every post of a patch. You
> > can also give metadata like "Repost number 3 of the blah blah patch"
>
> Yeah, I really meant to do that. I was fixing my A/C while doing this
> patch and I think the PVC glue fumes got to me. ;-) Down here you can't
> live an hour without the A/C.
>
> > So, please repost this exact same patch, together with its log message
> > :-).
>
> Done. And I also address some formatting fubar.
Ya know... sometimes I get cranky at the bars that are raised. "Oh, come
on... give yourself two minutes and you can find the log message!" Of
course, the counter is: multiply that by the number of patches, and it gets
bothersome.
But! You've been great, Mark. Thanks for the persistence and the
understanding. The worst thing we can do is frighten off new contributors
with an arm's length of rules. It's kind of, "come on! give the guy a
break... apply his patch already, will ya? iterate after that."
Thanks for the patch and the understanding,
-g
--
Greg Stein, http://www.lyra.org/
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Issue #1295 (take 2, with log)
Posted by Michael Wood <mw...@its.uct.ac.za>.
On Fri, May 09, 2003 at 04:12:47AM -0400, Mark Grosberg wrote:
>
>
> On 9 May 2003 cmpilato@collab.net wrote:
>
> > > "error" reports; we want to avoid that, so giving a bit of though to the
> > > failure cases would be good here.
> >
> > Additionally, I think the spelling "committed" is preferred to a
> > mixture of "commited" and "commited". :-)
>
> I was trying to follow the surrounding code, which used commited.
Well then there's still a mistake :)
cmpilato meant '... mixture of "comitted" and "commited".'
--
Michael Wood <mw...@its.uct.ac.za>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Issue #1295 (take 2, with log)
Posted by kf...@collab.net.
"Sander Striker" <st...@apache.org> writes:
> I sincerely hope that the marker will reflect that. Everything below
> that line certainly isn't ignored anymore, but checked to see if any
> of the commit sources are removed.
Ooooh, good point, yah.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
RE: [PATCH] Issue #1295 (take 2, with log)
Posted by Sander Striker <st...@apache.org>.
> From: kfogel@newton.ch.collab.net [mailto:kfogel@newton.ch.collab.net]On
> Behalf Of kfogel@collab.net
> Sent: Friday, May 09, 2003 6:48 PM
> Not only is it confusing, it isn't even the behavior I was proposing
> we have :-).
>
> If the line
>
> "--This line, and those below, will be ignored.--"
>
> is absent, then we shouldn't do *any* special interpretation; instead,
> just send the message as-is. That line is our marker, the signal to
> interpret a portion of the log message as live data.
I sincerely hope that the marker will reflect that. Everything below
that line certainly isn't ignored anymore, but checked to see if any
of the commit sources are removed.
> If the marker's missing, we should just treat the whole message as static data.
As in Marks patch.
Sander
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Issue #1295 (take 2, with log)
Posted by kf...@collab.net.
mark benedetto king <mb...@boredom.org> writes:
> i.e., something like:
>
> --This line, and those below, will be ignored.
> --If the above line is removed, the list after the next line
> --will be interpreted as a list of files to include in the commit.
>
> This may be confusing and/or overkill, I admit.
Not only is it confusing, it isn't even the behavior I was proposing
we have :-).
If the line
"--This line, and those below, will be ignored.--"
is absent, then we shouldn't do *any* special interpretation; instead,
just send the message as-is. That line is our marker, the signal to
interpret a portion of the log message as live data. If the marker's
missing, we should just treat the whole message as static data.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Issue #1295 (take 2, with log)
Posted by mark benedetto king <mb...@boredom.org>.
On Fri, May 09, 2003 at 10:51:36AM -0500, kfogel@collab.net wrote:
> Mark, just a quick sanity check:
>
> If someone removes the line that says
>
> "--This line, and those below, will be ignored--"
>
> then all the files will be committed as usual, right? That's my
> understanding from reading the code in process_comitted_list (er,
> which I guess should be called `process_committed_list')... but I'm
> asking just to be sure.
>
Because of the potentially to irrevocably break an atomic commit
into multiple revisions, should we make it opt-in?
i.e., something like:
--This line, and those below, will be ignored.
--If the above line is removed, the list after the next line
--will be interpreted as a list of files to include in the commit.
This may be confusing and/or overkill, I admit.
--ben
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Issue #1295 (take 2, with log)
Posted by Branko Čibej <br...@xbc.nu>.
Mark Grosberg wrote:
>On 9 May 2003 kfogel@collab.net wrote:
>
>
>>Of course, one way to answer my user interface questions is to supply
>>the documentation patches with the code patches :-). Changes to the
>>.xml files in doc/book/book/ are welcome.
>>
>>
>
>After the patch goes in to the code I'll do the doc updates as a second
>pass. Is that okay?
>
>
I guess so, but the behaviour must be documented in the function's
docstring when the patch goes in.
--
Brane Čibej <br...@xbc.nu> http://www.xbc.nu/brane/
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Issue #1295 (take 2, with log)
Posted by Mark Grosberg <ma...@nolab.conman.org>.
On 9 May 2003 kfogel@collab.net wrote:
> Sure, I think that's fine (but as Brane said, the behavior needs to be
> documented at the API level, even though that isn't user
> documentation).
Take 5 does have the doc strings that talk about the error returns. I'm
pretty poor at writing documentation. So unless there are any suggestions,
take 5 is ready for commit.
L8r,
Mark G.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Issue #1295 (take 2, with log)
Posted by kf...@collab.net.
Mark Grosberg <ma...@nolab.conman.org> writes:
> After the patch goes in to the code I'll do the doc updates as a second
> pass. Is that okay?
Sure, I think that's fine (but as Brane said, the behavior needs to be
documented at the API level, even though that isn't user
documentation).
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Issue #1295 (take 2, with log)
Posted by Mark Grosberg <ma...@nolab.conman.org>.
On 9 May 2003 kfogel@collab.net wrote:
> Mark, just a quick sanity check:
>
> If someone removes the line that says
>
> "--This line, and those below, will be ignored--"
>
> then all the files will be committed as usual, right? That's my
Yup.
> understanding from reading the code in process_comitted_list (er,
> which I guess should be called `process_committed_list')... but I'm
> asking just to be sure.
> Conversely, what are the ways someone could get a target eliminated
> from the commit?:
>
> a) Remove that item's line from the msg?
Yes.
> b) Change the modification letter code at the start of its line?
No. In this case, it would error out. I did that on purpose because I
wanted to make it rather fool-proof.
I can fix the spelling things and do take 5, should I?
> (And Brane already pointed out the problems with the 'silent return'
> mode when it encounters a letter code it doesn't expect.)
Take 4 does not have this problem.
> Of course, one way to answer my user interface questions is to supply
> the documentation patches with the code patches :-). Changes to the
> .xml files in doc/book/book/ are welcome.
After the patch goes in to the code I'll do the doc updates as a second
pass. Is that okay?
L8r,
Mark G.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Issue #1295 (take 2, with log)
Posted by kf...@collab.net.
Mark, just a quick sanity check:
If someone removes the line that says
"--This line, and those below, will be ignored--"
then all the files will be committed as usual, right? That's my
understanding from reading the code in process_comitted_list (er,
which I guess should be called `process_committed_list')... but I'm
asking just to be sure.
Conversely, what are the ways someone could get a target eliminated
from the commit?:
a) Remove that item's line from the msg?
b) Change the modification letter code at the start of its line?
(And Brane already pointed out the problems with the 'silent return'
mode when it encounters a letter code it doesn't expect.)
Of course, one way to answer my user interface questions is to supply
the documentation patches with the code patches :-). Changes to the
.xml files in doc/book/book/ are welcome.
-Karl
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Issue #1295 (take 2, with log)
Posted by Mark Grosberg <ma...@nolab.conman.org>.
On 9 May 2003 cmpilato@collab.net wrote:
> > "error" reports; we want to avoid that, so giving a bit of though to the
> > failure cases would be good here.
>
> Additionally, I think the spelling "committed" is preferred to a
> mixture of "commited" and "commited". :-)
I was trying to follow the surrounding code, which used commited.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Issue #1295 (take 2, with log)
Posted by cm...@collab.net.
=?UTF-8?B?QnJhbmtvIMSMaWJlag==?= <br...@xbc.nu> writes:
> >+/* Parse the edited comitted message and store the named paths
> >+ in the hash. */
> >+static void
> >+process_comitted_list (apr_hash_t *seen,
> >+ char *commited_list,
> >+ apr_pool_t *pool)
> >
>
> The docstring for this function should explain exactly what kind of
> input it expects, what it'll parse and when it'll fail. I'm also a bit
> worried about its silently doing nothing if it encounters something it
> can't parse; I think that's a potential source of many surprises and
> "error" reports; we want to avoid that, so giving a bit of though to the
> failure cases would be good here.
Additionally, I think the spelling "committed" is preferred to a
mixture of "commited" and "commited". :-)
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Issue #1295 (take 2, with log)
Posted by Ben Collins-Sussman <su...@collab.net>.
"Sander Striker" <st...@apache.org> writes:
> > Not quite sure what you mean. I've only used CVS as I use CVS. Do my
> > coding, then do a commit and type the log message in the provided editor.
>
> Right. Some people compose their commit message while coding. When
> done and doing a commit, they do 'svn ci -F /path/to/commit_msg'.
/me raises his hand. :-)
The elisp function 'svn-log-message', in tools/dev/svn-dev.el, is the
best thing that ever happened to me.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
RE: [PATCH] Issue #1295 (take 2, with log)
Posted by Sander Striker <st...@apache.org>.
> From: Mark Grosberg [mailto:mark@nolab.conman.org]
> Sent: Friday, May 09, 2003 9:50 AM
> > > From: Branko Cibej [mailto:brane@xbc.nu]
> > > Sent: Friday, May 09, 2003 9:32 AM
> >
> > [...]
> > > And we really want tests for this feature (in commit-tests.py).
> > > Modifying the commit list in a message passed in with -F should work,
> > right?
> >
> > Hmmm, won't this result in committing nothing when people compose their
> > log messages while coding? Typically these messages won't have the part
> > that the log message generator adds before popping up an editor.
>
> Not quite sure what you mean. I've only used CVS as I use CVS. Do my
> coding, then do a commit and type the log message in the provided editor.
Right. Some people compose their commit message while coding. When
done and doing a commit, they do 'svn ci -F /path/to/commit_msg'.
> What I can do is abort conditionally depending on if I find the pre-canned
> string. If I don't find it, the default behavior (commit all) can be
> performed.
>
> Does that sound right?
That sounds like a good idea.
Sander
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Issue #1295 (take 2, with log)
Posted by Mark Grosberg <ma...@nolab.conman.org>.
> > From: Branko Cibej [mailto:brane@xbc.nu]
> > Sent: Friday, May 09, 2003 9:32 AM
>
> [...]
> > And we really want tests for this feature (in commit-tests.py).
> > Modifying the commit list in a message passed in with -F should work,
> right?
>
> Hmmm, won't this result in committing nothing when people compose their
> log messages while coding? Typically these messages won't have the part
> that the log message generator adds before popping up an editor.
Not quite sure what you mean. I've only used CVS as I use CVS. Do my
coding, then do a commit and type the log message in the provided editor.
What I can do is abort conditionally depending on if I find the pre-canned
string. If I don't find it, the default behavior (commit all) can be
performed.
Does that sound right?
L8r,
Mark G.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Issue #1295 (take 2, with log)
Posted by cm...@collab.net.
"Sander Striker" <st...@apache.org> writes:
> > From: Branko Cibej [mailto:brane@xbc.nu]
> > Sent: Friday, May 09, 2003 9:32 AM
>
> [...]
> > And we really want tests for this feature (in commit-tests.py).
> > Modifying the commit list in a message passed in with -F should work, right?
>
> Hmmm, won't this result in committing nothing when people compose their
> log messages while coding? Typically these messages won't have the part
> that the log message generator adds before popping up an editor.
The current code allows you to pass the tmpfile from a failed commit,
and it goes through the same stripping-stuff-below-this-line code that
a pop-up editor message goes through. So, while this might be useful
in the typical usage of -F for non-failed commits, it certainly
could be leveraged in a testing scenario.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
RE: [PATCH] Issue #1295 (take 2, with log)
Posted by Sander Striker <st...@apache.org>.
> From: Branko Cibej [mailto:brane@xbc.nu]
> Sent: Friday, May 09, 2003 9:32 AM
[...]
> And we really want tests for this feature (in commit-tests.py).
> Modifying the commit list in a message passed in with -F should work, right?
Hmmm, won't this result in committing nothing when people compose their
log messages while coding? Typically these messages won't have the part
that the log message generator adds before popping up an editor.
Sander
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Issue #1295 (take 2, with log)
Posted by Branko Čibej <br...@xbc.nu>.
Mark Grosberg wrote:
>On 9 May 2003 kfogel@collab.net wrote:
>
>
>
>>Always include the full log message with every post of a patch. You
>>can also give metadata like "Repost number 3 of the blah blah patch"
>>
>>
>
>Yeah, I really meant to do that. I was fixing my A/C while doing this
>patch and I think the PVC glue fumes got to me. ;-) Down here you can't
>live an hour without the A/C.
>
>
>
>>So, please repost this exact same patch, together with its log message
>>:-).
>>
>>
>
>Done. And I also address some formatting fubar.
>
This patch looks much, much better to me than the first one. Very nice!
[snip]
> /** The commit candidate structure. */
>Index: libsvn_client/commit_util.c
>===================================================================
>--- libsvn_client/commit_util.c (revision 5857)
>+++ libsvn_client/commit_util.c (working copy)
>@@ -730,6 +730,14 @@
> = *((const svn_client_commit_item_t * const *) a);
> const svn_client_commit_item_t *item2
> = *((const svn_client_commit_item_t * const *) b);
>+
>+ if ((item1->state_flags & SVN_CLIENT_COMMIT_ITEM_UNMARKED)
>+ && (!(item2->state_flags & SVN_CLIENT_COMMIT_ITEM_UNMARKED)))
>+ return 1;
>+ if ((!(item1->state_flags & SVN_CLIENT_COMMIT_ITEM_UNMARKED))
>+ && (item2->state_flags & SVN_CLIENT_COMMIT_ITEM_UNMARKED))
>+ return -1;
>+
> return svn_path_compare_paths (item1->url, item2->url);
> }
>
Heh.
int cmp = ((item1->state_flags & SVN_CLIENT_COMMIT_ITEM_UNMARKED)
- (item2->state_flags & SVN_CLIENT_COMMIT_ITEM_UNMARKED));
if (cmp == 0)
cmp = svn_path_compare_paths (item1->url, item2->url);
return cmp;
[snip]
>+/* Parse the edited comitted message and store the named paths
>+ in the hash. */
>+static void
>+process_comitted_list (apr_hash_t *seen,
>+ char *commited_list,
>+ apr_pool_t *pool)
>
The docstring for this function should explain exactly what kind of
input it expects, what it'll parse and when it'll fail. I'm also a bit
worried about its silently doing nothing if it encounters something it
can't parse; I think that's a potential source of many surprises and
"error" reports; we want to avoid that, so giving a bit of though to the
failure cases would be good here.
And we really want tests for this feature (in commit-tests.py).
Modifying the commit list in a message passed in with -F should work, right?
--
Brane Čibej <br...@xbc.nu> http://www.xbc.nu/brane/
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Issue #1295 (take 2, with log)
Posted by Mark Grosberg <ma...@nolab.conman.org>.
On 9 May 2003 kfogel@collab.net wrote:
> Always include the full log message with every post of a patch. You
> can also give metadata like "Repost number 3 of the blah blah patch"
Yeah, I really meant to do that. I was fixing my A/C while doing this
patch and I think the PVC glue fumes got to me. ;-) Down here you can't
live an hour without the A/C.
> So, please repost this exact same patch, together with its log message
> :-).
Done. And I also address some formatting fubar.
L8r,
Mark G.
========================================================================
Fix for issue #1295.
* subversion/libsvn_client/commit_util.c
(svn_client__condense_commit_items): Skip targets that have the
new SVN_CLIENT_COMMIT_ITEM_UNMARKED flag set.
(svn_client__sort_commit_item_urls): Always sort unmarked items
at the end of the list. This way, they are easy to remove.
* subversion/clients/cmdline.c
(get_item_path): [New] Factored out log path name generator.
(flag_unmarked): [New] Set the SVN_CLIENT_COMMIT_ITEM_UNMARKED flag
for paths not found in the specified hash.
(process_comitted_list): Parse the edited message to see what file
names remain and add them to a hash.
(svn_cl__get_log_message): Mark the entries in the comitted items
list that should not be comitted.
* subversion/include/svn_client.h
Add new flag: SVN_CLIENT_COMMIT_ITEM_UNMARKED
* subversion/include/svn_error_codes.h
Add new client error: SVN_ERR_CLIENT_NO_WORK
-------------------------------------------------------------------------
Index: include/svn_error_codes.h
===================================================================
--- include/svn_error_codes.h (revision 5857)
+++ include/svn_error_codes.h (working copy)
@@ -688,6 +688,11 @@
SVN_ERR_CLIENT_CATEGORY_START + 8,
"Revision range is not allowed")
+ SVN_ERRDEF (SVN_ERR_CLIENT_NO_WORK,
+ SVN_ERR_CLIENT_CATEGORY_START + 9,
+ "No work to do")
+
+
/* misc errors */
SVN_ERRDEF (SVN_ERR_BASE,
Index: include/svn_client.h
===================================================================
--- include/svn_client.h (revision 5857)
+++ include/svn_client.h (working copy)
@@ -292,6 +292,7 @@
#define SVN_CLIENT_COMMIT_ITEM_TEXT_MODS 0x04
#define SVN_CLIENT_COMMIT_ITEM_PROP_MODS 0x08
#define SVN_CLIENT_COMMIT_ITEM_IS_COPY 0x10
+#define SVN_CLIENT_COMMIT_ITEM_UNMARKED 0x20
/** @} */
/** The commit candidate structure. */
Index: libsvn_client/commit_util.c
===================================================================
--- libsvn_client/commit_util.c (revision 5857)
+++ libsvn_client/commit_util.c (working copy)
@@ -730,6 +730,14 @@
= *((const svn_client_commit_item_t * const *) a);
const svn_client_commit_item_t *item2
= *((const svn_client_commit_item_t * const *) b);
+
+ if ((item1->state_flags & SVN_CLIENT_COMMIT_ITEM_UNMARKED)
+ && (!(item2->state_flags & SVN_CLIENT_COMMIT_ITEM_UNMARKED)))
+ return 1;
+ if ((!(item1->state_flags & SVN_CLIENT_COMMIT_ITEM_UNMARKED))
+ && (item2->state_flags & SVN_CLIENT_COMMIT_ITEM_UNMARKED))
+ return -1;
+
return svn_path_compare_paths (item1->url, item2->url);
}
@@ -751,6 +759,20 @@
qsort (ci->elts, ci->nelts,
ci->elt_size, svn_client__sort_commit_item_urls);
+ /* Delete unrequested items. */
+ for (i = ci->nelts; i > 0; i--)
+ {
+ item = (((svn_client_commit_item_t **) ci->elts)[i - 1]);
+ if (!(item->state_flags & SVN_CLIENT_COMMIT_ITEM_UNMARKED))
+ break;
+ }
+
+ ci->nelts = i;
+ if (ci->nelts == 0)
+ return svn_error_create (SVN_ERR_CLIENT_NO_WORK, NULL,
+ "No entires submitted for commit.");
+
+
/* Loop through the URLs, finding the longest usable ancestor common
to all of them, and making sure there are no duplicate URLs. */
for (i = 0; i < ci->nelts; i++)
Index: clients/cmdline/util.c
===================================================================
--- clients/cmdline/util.c (revision 5857)
+++ clients/cmdline/util.c (working copy)
@@ -416,6 +416,147 @@
#define EDITOR_EOF_PREFIX "--This line, and those below, will be ignored--"
+/* Get the full path string for the specificed item. */
+static const char *
+get_item_path (svn_client_commit_item_t *item,
+ struct log_msg_baton *lmb,
+ apr_pool_t *pool)
+{
+ const char *item_path = item->path;
+
+ if (! item_path)
+ item_path = item->url;
+ else if (! *item_path)
+ item_path = ".";
+
+ if (item_path && lmb->base_dir)
+ item_path = svn_path_is_child (lmb->base_dir, item_path, pool);
+
+ /* If still no path, then just use current directory. */
+ if (! item_path)
+ item_path = ".";
+
+ return item_path;
+}
+
+/* Parse the edited comitted message and store the named paths
+ in the hash. */
+static void
+process_comitted_list (apr_hash_t *seen,
+ char *commited_list,
+ apr_pool_t *pool)
+{
+ char *end_str, *prev_str = NULL;
+
+ /* Find the generated message. */
+ end_str = commited_list;
+ while (end_str != NULL)
+ {
+ end_str = strstr (end_str, EDITOR_EOF_PREFIX);
+ if (end_str == NULL)
+ break;
+
+ end_str = end_str + (sizeof(EDITOR_EOF_PREFIX) - 1);
+ prev_str = end_str;
+ }
+
+ if (prev_str == NULL)
+ return;
+ while (*prev_str != '\0')
+ {
+ if (!apr_isspace (*prev_str))
+ break;
+ prev_str++;
+ }
+
+ /* Now, parse each line and make sure that the commited item is named. */
+ for (;;)
+ {
+ char *end_of_path;
+
+ /* Skip leading whitespace. */
+ for (;;)
+ {
+ if (*prev_str == '\0')
+ return;
+
+ if (!apr_isspace (*prev_str))
+ break;
+ prev_str++;
+ }
+ if (*prev_str == '\0')
+ break;
+
+ /* The next byte had better be one of the text mod's below. */
+ switch (*prev_str++)
+ {
+ case '_':
+ case 'R':
+ case 'A':
+ case 'D':
+ case 'M': break;
+ default: return;
+ }
+
+ /* The next byte had better be one of the prop mod's below. */
+ switch (*prev_str++)
+ {
+ case ' ':
+ case 'M': break;
+ default: return;
+ }
+
+ /* Skip more whitespace. */
+ for (;;)
+ {
+ if (*prev_str == '\0')
+ return;
+ if (!apr_isspace(*prev_str))
+ break;
+ prev_str++;
+ }
+
+ /* Chop up the path. */
+ for (end_of_path = prev_str; *end_of_path != '\0'; end_of_path++)
+ {
+ if (*end_of_path == '\n')
+ break;
+ }
+ if (*end_of_path == '\0')
+ return;
+
+ /* Truncate the string. The path is now in prev_str. */
+ *end_of_path++ = '\0';
+
+ /* Note the existence. */
+ apr_hash_set(seen, prev_str, APR_HASH_KEY_STRING, "");
+
+ /* Advance the pointer. */
+ prev_str = end_of_path;
+ }
+}
+
+/* Find the paths that are marked in the seen hash and set their
+ unmarked flags. */
+static void
+flag_unmarked (struct log_msg_baton *lmb,
+ apr_array_header_t *commit_items,
+ apr_hash_t *seen,
+ apr_pool_t *pool)
+{
+ int i;
+
+ for (i = 0; i < commit_items->nelts; i++)
+ {
+ svn_client_commit_item_t *item
+ = ((svn_client_commit_item_t **) commit_items->elts)[i];
+ const char *path = get_item_path (item, lmb, pool);
+
+ if (! apr_hash_get (seen, path, APR_HASH_KEY_STRING))
+ item->state_flags |= SVN_CLIENT_COMMIT_ITEM_UNMARKED;
+ }
+}
+
/* This function is of type svn_client_get_commit_log_t. */
svn_error_t *
svn_cl__get_log_message (const char **log_msg,
@@ -469,21 +610,9 @@
{
svn_client_commit_item_t *item
= ((svn_client_commit_item_t **) commit_items->elts)[i];
- const char *path = item->path;
+ const char *path = get_item_path (item, lmb, pool);
char text_mod = '_', prop_mod = ' ';
- if (! path)
- path = item->url;
- else if (! *path)
- path = ".";
-
- if (path && lmb->base_dir)
- path = svn_path_is_child (lmb->base_dir, path, pool);
-
- /* If still no path, then just use current directory. */
- if (! path)
- path = ".";
-
if ((item->state_flags & SVN_CLIENT_COMMIT_ITEM_DELETE)
&& (item->state_flags & SVN_CLIENT_COMMIT_ITEM_ADD))
text_mod = 'R';
@@ -542,8 +671,16 @@
/* Strip the prefix from the buffer. */
if (message)
+ {
+ char *commited_list = apr_pstrdup (pool, msg2);
+ apr_hash_t *seen = apr_hash_make(pool);
+
+ process_comitted_list (seen, commited_list, pool);
+ flag_unmarked (lmb, commit_items, seen, pool);
+
truncate_buffer_at_prefix (&message->len, message->data,
EDITOR_EOF_PREFIX);
+ }
if (message)
{
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Issue #1295 (take 2)
Posted by kf...@collab.net.
Mark Grosberg <ma...@nolab.conman.org> writes:
> Here is my updated patch. I now use a hash table as suggested by Brane to
> store the paths we've seen. Also, qsort() is now used to remove
> the "UNMARKED" entries.
>
> I also added an error message if no entries are specified. I've tested
> this with several combinations and pathalogical cases on my test repos.
>
> Let me know what you think. I would love to see this merged. This nit
> bugs me in CVS daily.
Hey Mark -- a minor nit (which I want to counterbalance in advance by
praising both the patch itself, and your responsiveness to feedback):
Always include the full log message with every post of a patch. You
can also give metadata like "Repost number 3 of the blah blah patch"
or whatever, but the patch and its log message should be treated as a
self-contained unit, always travelling together. Otherwise, whoever
reviews and/or applies the patch must search back through their old
mail (or worse, visit the mailing list archives) to dig up the log
message. And even then, they can't be *sure* it's still accurate for
the version of the patch just posted...
So, please repost this exact same patch, together with its log message
:-).
Thanks,
-Karl
> -----------------------------------------------------------------------
>
> Index: include/svn_error_codes.h
> ===================================================================
> --- include/svn_error_codes.h (revision 5857)
> +++ include/svn_error_codes.h (working copy)
> @@ -688,6 +688,11 @@
> SVN_ERR_CLIENT_CATEGORY_START + 8,
> "Revision range is not allowed")
>
> + SVN_ERRDEF (SVN_ERR_CLIENT_NO_WORK,
> + SVN_ERR_CLIENT_CATEGORY_START + 9,
> + "No work to do")
> +
> +
> /* misc errors */
>
> SVN_ERRDEF (SVN_ERR_BASE,
> Index: include/svn_client.h
> ===================================================================
> --- include/svn_client.h (revision 5857)
> +++ include/svn_client.h (working copy)
> @@ -292,6 +292,7 @@
> #define SVN_CLIENT_COMMIT_ITEM_TEXT_MODS 0x04
> #define SVN_CLIENT_COMMIT_ITEM_PROP_MODS 0x08
> #define SVN_CLIENT_COMMIT_ITEM_IS_COPY 0x10
> +#define SVN_CLIENT_COMMIT_ITEM_UNMARKED 0x20
> /** @} */
>
> /** The commit candidate structure. */
> Index: libsvn_client/commit_util.c
> ===================================================================
> --- libsvn_client/commit_util.c (revision 5857)
> +++ libsvn_client/commit_util.c (working copy)
> @@ -730,6 +730,14 @@
> = *((const svn_client_commit_item_t * const *) a);
> const svn_client_commit_item_t *item2
> = *((const svn_client_commit_item_t * const *) b);
> +
> + if ((item1->state_flags & SVN_CLIENT_COMMIT_ITEM_UNMARKED)
> + && (!(item2->state_flags & SVN_CLIENT_COMMIT_ITEM_UNMARKED)))
> + return 1;
> + if ((!(item1->state_flags & SVN_CLIENT_COMMIT_ITEM_UNMARKED))
> + && (item2->state_flags & SVN_CLIENT_COMMIT_ITEM_UNMARKED))
> + return -1;
> +
> return svn_path_compare_paths (item1->url, item2->url);
> }
>
> @@ -751,6 +759,20 @@
> qsort (ci->elts, ci->nelts,
> ci->elt_size, svn_client__sort_commit_item_urls);
>
> + /* Delete unrequested items. */
> + for (i = ci->nelts; i > 0; i--)
> + {
> + item = (((svn_client_commit_item_t **) ci->elts)[i - 1]);
> + if (!(item->state_flags & SVN_CLIENT_COMMIT_ITEM_UNMARKED))
> + break;
> + }
> +
> + ci->nelts = i;
> + if (ci->nelts == 0)
> + return svn_error_create (SVN_ERR_CLIENT_NO_WORK, NULL,
> + "No entires submitted for commit.");
> +
> +
> /* Loop through the URLs, finding the longest usable ancestor common
> to all of them, and making sure there are no duplicate URLs. */
> for (i = 0; i < ci->nelts; i++)
> Index: clients/cmdline/util.c
> ===================================================================
> --- clients/cmdline/util.c (revision 5857)
> +++ clients/cmdline/util.c (working copy)
> @@ -416,6 +416,154 @@
>
> #define EDITOR_EOF_PREFIX "--This line, and those below, will be ignored--"
>
> +/* Get the full path string for the specificed item. */
> +static const char *
> +get_item_path (svn_client_commit_item_t *item,
> + struct log_msg_baton *lmb,
> + apr_pool_t *pool)
> +{
> + const char *item_path = item->path;
> +
> + if (! item_path)
> + item_path = item->url;
> + else if (! *item_path)
> + item_path = ".";
> +
> + if (item_path && lmb->base_dir)
> + item_path = svn_path_is_child (lmb->base_dir, item_path, pool);
> +
> + /* If still no path, then just use current directory. */
> + if (! item_path)
> + item_path = ".";
> +
> + return item_path;
> +}
> +
> +/* Parse the edited comitted message and store the named paths
> + in the hash. */
> +static void
> +process_comitted_list(apr_hash_t *seen,
> + char *commited_list,
> + apr_pool_t *pool)
> +{
> + char *end_str, *prev_str = NULL;
> + int i;
> +
> + /* Find the generated message. */
> + end_str = commited_list;
> + while (end_str != NULL)
> + {
> + end_str = strstr (end_str, EDITOR_EOF_PREFIX);
> + if (end_str == NULL)
> + break;
> +
> + end_str = end_str + (sizeof(EDITOR_EOF_PREFIX) - 1);
> + prev_str = end_str;
> + }
> +
> + if (prev_str == NULL)
> + return;
> + while (*prev_str != '\0')
> + {
> + if (!apr_isspace (*prev_str))
> + break;
> + prev_str++;
> + }
> +
> + /* Now, parse each line and make sure that the commited item is named. */
> + for (;;)
> + {
> + char *end_of_path;
> +
> + /* Skip leading whitespace. */
> + for (;;)
> + {
> + if (*prev_str == '\0')
> + return;
> +
> + if (!apr_isspace (*prev_str))
> + break;
> + prev_str++;
> + }
> + if (*prev_str == '\0')
> + break;
> +
> + /* The next byte had better be one of the text mod's below. */
> + switch (*prev_str++)
> + {
> + case '_':
> + case 'R':
> + case 'A':
> + case 'D':
> + case 'M':
> + break;
> +
> + default:
> + return;
> + }
> +
> + /* The next byte had better be one of the prop mod's below. */
> + switch (*prev_str++)
> + {
> + case ' ':
> + case 'M':
> + break;
> +
> + default:
> + return;
> + }
> +
> + /* Skip more whitespace. */
> + for (;;)
> + {
> + if (*prev_str == '\0')
> + return;
> + if (!apr_isspace(*prev_str))
> + break;
> + prev_str++;
> + }
> +
> + /* Chop up the path. */
> + for (end_of_path = prev_str; *end_of_path != '\0'; end_of_path++)
> + {
> + if (*end_of_path == '\n')
> + break;
> + }
> + if (*end_of_path == '\0')
> + return;
> +
> + /* Truncate the string. The path is now in prev_str. */
> + *end_of_path++ = '\0';
> +
> + /* Note the existence. */
> + apr_hash_set(seen, prev_str, APR_HASH_KEY_STRING, "");
> +
> + /* Advance the pointer. */
> + prev_str = end_of_path;
> + }
> +}
> +
> +/* Find the paths that are marked in the seen hash and set their
> + unmarked flags. */
> +static void
> +flag_unmarked (struct log_msg_baton *lmb,
> + apr_array_header_t *commit_items,
> + apr_hash_t *seen,
> + apr_pool_t *pool)
> +{
> + int i;
> +
> + for (i = 0; i < commit_items->nelts; i++)
> + {
> + svn_client_commit_item_t *item
> + = ((svn_client_commit_item_t **) commit_items->elts)[i];
> + const char *path = get_item_path (item, lmb, pool);
> +
> + if (! apr_hash_get (seen, path, APR_HASH_KEY_STRING))
> + item->state_flags |= SVN_CLIENT_COMMIT_ITEM_UNMARKED;
> + }
> +}
> +
> /* This function is of type svn_client_get_commit_log_t. */
> svn_error_t *
> svn_cl__get_log_message (const char **log_msg,
> @@ -469,21 +617,9 @@
> {
> svn_client_commit_item_t *item
> = ((svn_client_commit_item_t **) commit_items->elts)[i];
> - const char *path = item->path;
> + const char *path = get_item_path (item, lmb, pool);
> char text_mod = '_', prop_mod = ' ';
>
> - if (! path)
> - path = item->url;
> - else if (! *path)
> - path = ".";
> -
> - if (path && lmb->base_dir)
> - path = svn_path_is_child (lmb->base_dir, path, pool);
> -
> - /* If still no path, then just use current directory. */
> - if (! path)
> - path = ".";
> -
> if ((item->state_flags & SVN_CLIENT_COMMIT_ITEM_DELETE)
> && (item->state_flags & SVN_CLIENT_COMMIT_ITEM_ADD))
> text_mod = 'R';
> @@ -542,8 +678,16 @@
>
> /* Strip the prefix from the buffer. */
> if (message)
> + {
> + char *commited_list = apr_pstrdup (pool, msg2);
> + apr_hash_t *seen = apr_hash_make(pool);
> +
> + process_comitted_list (seen, commited_list, pool);
> + flag_unmarked (lmb, commit_items, seen, pool);
> +
> truncate_buffer_at_prefix (&message->len, message->data,
> EDITOR_EOF_PREFIX);
> + }
>
> if (message)
> {
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
> For additional commands, e-mail: dev-help@subversion.tigris.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org