Karl,
Fixed the log message.
Thanks for the review.
With regards
Kamesh Jayachandran
-----Original Message-----
From: Karl Fogel [mailto:kfogel_at_red-bean.com]
Sent: Sat 8/23/2008 5:08 AM
To: dev_at_subversion.tigris.org
Cc: kameshj_at_tigris.org
Subject: Re: svn commit: r32618 - branches/issue-2897-take2/subversion/libsvn_fs_base
kameshj_at_tigris.org writes:
> On issue-2897-take2 branch:
>
> When 'svn:mergeinfo' property is added freshly in a commit
> mergeinfo_delta is svn:mergeinfo itself. This fixes few segfaults
> triggered by our testsuite.
>
> * subversion/libsvn_fs_base/tree.c
> (txn_body_change_node_prop): When 'svn:mergeinfo' property is added freshly
> in a commit mergeinfo_delta is svn:mergeinfo itself.
I wasn't able to understand what this meant (also, it has the same
exact-repetition problem as that other log message I just posted about).
It sounds like you're trying to describe the problem in terms of the
implementation details that led to the problem (or led to its solution).
But it's often clearer to describe a problem at a higher level. For
example, this change could be described thusly:
"When the 'svn:mergeinfo' property is being added as part of a
commit, then there is no need to take a delta against old mergeinfo;
instead, just mark all the new mergeinfo as added."
...or something like that (you may have a better way to describe this
change, since you know more about the context).
Note that the main thing I did was remove the references to
'mergeinfo_delta', which looks like a symbol name but isn't anywhere in
the diff, and anyway assumes a level of detailed knowledge that a reader
of the log message is unlikely to have the first time they read the log.
The ideal log message leads the reader from high-level to low-level (the
diff itself being the lower level, of course). If the log message
starts out at a low-level, then the person reading it has to keep more
"unresolved symbols" in their head as they move on to the diff (think of
the reader as a human linker :-) ).
Hope this helps,
-Karl
> --- branches/issue-2897-take2/subversion/libsvn_fs_base/tree.c
> +++ branches/issue-2897-take2/subversion/libsvn_fs_base/tree.c
> @@ -1315,19 +1315,28 @@ txn_body_change_node_prop(void *baton,
> }
> else
> {
> - svn_mergeinfo_t deleted, orig_mergeinfo, new_mergeinfo;
> if (args->value && (strcmp(args->name, SVN_PROP_MERGEINFO) == 0))
> {
> svn_string_t *orig_mergeinfo_str = apr_hash_get(proplist,
> SVN_PROP_MERGEINFO,
> APR_HASH_KEY_STRING);
> - SVN_ERR(svn_mergeinfo_parse(&orig_mergeinfo,
> - orig_mergeinfo_str->data, trail->pool));
> - SVN_ERR(svn_mergeinfo_parse(&new_mergeinfo,
> - args->value->data, trail->pool));
> - SVN_ERR(svn_mergeinfo_diff(&deleted, &mergeinfo_added,
> - orig_mergeinfo, new_mergeinfo,
> - TRUE, trail->pool));
> + if (orig_mergeinfo_str)
> + {
> + svn_mergeinfo_t deleted, orig_mergeinfo, new_mergeinfo;
> + SVN_ERR(svn_mergeinfo_parse(&orig_mergeinfo,
> + orig_mergeinfo_str->data,
> + trail->pool));
> + SVN_ERR(svn_mergeinfo_parse(&new_mergeinfo,
> + args->value->data, trail->pool));
> + SVN_ERR(svn_mergeinfo_diff(&deleted, &mergeinfo_added,
> + orig_mergeinfo, new_mergeinfo,
> + TRUE, trail->pool));
> + }
> + else
> + {
> + SVN_ERR(svn_mergeinfo_parse(&mergeinfo_added,
> + args->value->data, trail->pool));
> + }
> }
> }
Received on 2008-08-23 07:08:45 CEST