You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Bert Huijben <be...@qqmail.nl> on 2014/02/23 00:06:11 UTC
Re: svn commit: r1570936 - /subversion/trunk/subversion/libsvn_client/blame.c
I think your patch doesn't change the behavior (this is more a brain dump based on te earlier related mail thread), but there are property changes that do change how we interpret the content of a file… E.g. some changes to svn:eol-style and svn:keywords.
In these cases the actual file may change while the repository form didn’t.
Blame, especially with whitespace ignores might not be interested in these cases, but there are api users that use the file revs api that are interested.
Bert
Sent from Windows Mail
From: Stefan Fuhrmann
Sent: Saturday, February 22, 2014 11:27 PM
To: commits@subversion.apache.org
Author: stefan2
Date: Sat Feb 22 22:27:06 2014
New Revision: 1570936
URL: http://svn.apache.org/r1570936
Log:
Fix the blame -g implemenation to no longer depend on false positives
in the file contents change detection.
* subversion/libsvn_client/blame.c
(update_blame): Factored out from ...
(window_handler): ... this. Only do the diff application directly here.
(file_rev_handler): For potential merges, trigger the blame processing
even if there has been no change since the last rev
which might have been on a different branch.
Modified:
subversion/trunk/subversion/libsvn_client/blame.c
Modified: subversion/trunk/subversion/libsvn_client/blame.c
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_client/blame.c?rev=1570936&r1=1570935&r2=1570936&view=diff
==============================================================================
--- subversion/trunk/subversion/libsvn_client/blame.c (original)
+++ subversion/trunk/subversion/libsvn_client/blame.c Sat Feb 22 22:27:06 2014
@@ -305,27 +305,15 @@ add_file_blame(const char *last_file,
return SVN_NO_ERROR;
}
-/* The delta window handler for the text delta between the previously seen
- * revision and the revision currently being handled.
- *
- * Record the blame information for this revision in BATON->file_rev_baton.
- *
- * Implements svn_txdelta_window_handler_t.
+/* Record the blame information for the revision in BATON->file_rev_baton.
*/
static svn_error_t *
-window_handler(svn_txdelta_window_t *window, void *baton)
+update_blame(void *baton)
{
struct delta_baton *dbaton = baton;
struct file_rev_baton *frb = dbaton->file_rev_baton;
struct blame_chain *chain;
- /* Call the wrapped handler first. */
- SVN_ERR(dbaton->wrapped_handler(window, dbaton->wrapped_baton));
-
- /* We patiently wait for the NULL window marking the end. */
- if (window)
- return SVN_NO_ERROR;
-
/* Close the source file used for the delta.
It is important to do this early, since otherwise, they will be deleted
before all handles are closed, which leads to failures on some platforms
@@ -382,6 +370,32 @@ window_handler(svn_txdelta_window_t *win
return SVN_NO_ERROR;
}
+/* The delta window handler for the text delta between the previously seen
+ * revision and the revision currently being handled.
+ *
+ * Record the blame information for this revision in BATON->file_rev_baton.
+ *
+ * Implements svn_txdelta_window_handler_t.
+ */
+static svn_error_t *
+window_handler(svn_txdelta_window_t *window, void *baton)
+{
+ struct delta_baton *dbaton = baton;
+
+ /* Call the wrapped handler first. */
+ if (dbaton->wrapped_handler)
+ SVN_ERR(dbaton->wrapped_handler(window, dbaton->wrapped_baton));
+
+ /* We patiently wait for the NULL window marking the end. */
+ if (window)
+ return SVN_NO_ERROR;
+
+ /* Diff and update blame info. */
+ SVN_ERR(update_blame(baton));
+
+ return SVN_NO_ERROR;
+}
+
/* Calculate and record blame information for one revision of the file,
* by comparing the file content against the previously seen revision.
@@ -430,17 +444,18 @@ file_rev_handler(void *baton, const char
if (frb->ctx->cancel_func)
SVN_ERR(frb->ctx->cancel_func(frb->ctx->cancel_baton));
- /* If there were no content changes, we couldn't care less about this
- revision now. Note that we checked the mime type above, so things
- work if the user just changes the mime type in a commit.
+ /* If there were no content changes and no (potential) merges, we couldn't
+ care less about this revision now. Note that we checked the mime type
+ above, so things work if the user just changes the mime type in a commit.
Also note that we don't switch the pools in this case. This is important,
since the tempfile will be removed by the pool and we need the tempfile
from the last revision with content changes. */
- if (!content_delta_handler)
+ if (!content_delta_handler
+ && (!frb->include_merged_revisions || merged_revision))
return SVN_NO_ERROR;
/* Create delta baton. */
- delta_baton = apr_palloc(frb->currpool, sizeof(*delta_baton));
+ delta_baton = apr_pcalloc(frb->currpool, sizeof(*delta_baton));
/* Prepare the text delta window handler. */
if (frb->last_filename)
@@ -460,17 +475,9 @@ file_rev_handler(void *baton, const char
svn_io_file_del_on_pool_cleanup,
filepool, filepool));
- /* Get window handler for applying delta. */
- svn_txdelta_apply(last_stream, cur_stream, NULL, NULL,
- frb->currpool,
- &delta_baton->wrapped_handler,
- &delta_baton->wrapped_baton);
-
/* Wrap the window handler with our own. */
delta_baton->file_rev_baton = frb;
delta_baton->is_merged_revision = merged_revision;
- *content_delta_handler = window_handler;
- *content_delta_baton = delta_baton;
/* Create the rev structure. */
delta_baton->rev = apr_pcalloc(frb->mainpool, sizeof(struct rev));
@@ -502,6 +509,28 @@ file_rev_handler(void *baton, const char
/* Keep last revision for postprocessing after all changes */
frb->last_rev = delta_baton->rev;
+ /* Handle all delta - even if it is empty.
+ We must do the latter to "merge" blame info from other branches. */
+ if (content_delta_handler)
+ {
+ /* Proper delta - get window handler for applying delta.
+ svn_ra_get_file_revs2 will drive the delta editor. */
+ svn_txdelta_apply(last_stream, cur_stream, NULL, NULL,
+ frb->currpool,
+ &delta_baton->wrapped_handler,
+ &delta_baton->wrapped_baton);
+ *content_delta_handler = window_handler;
+ *content_delta_baton = delta_baton;
+ }
+ else
+ {
+ /* Apply an empty delta, i.e. simply copy the old contents.
+ We can't simply use the existing file due to the pool rotation logic.
+ Trigger the blame update magic. */
+ SVN_ERR(svn_stream_copy3(last_stream, cur_stream, NULL, NULL, pool));
+ SVN_ERR(update_blame(delta_baton));
+ }
+
return SVN_NO_ERROR;
}
Re: svn commit: r1570936 - /subversion/trunk/subversion/libsvn_client/blame.c
Posted by Stefan Fuhrmann <st...@wandisco.com>.
On Sun, Feb 23, 2014 at 12:06 AM, Bert Huijben <be...@qqmail.nl> wrote:
> I think your patch doesn't change the behavior (this is more a brain
> dump based on te earlier related mail thread), but there are property
> changes that do change how we interpret the content of a file... E.g. some
> changes to svn:eol-style and svn:keywords.
>
> In these cases the actual file may change while the repository form didn't.
>
> Blame, especially with whitespace ignores might not be interested in these
> cases, but there are api users that use the file revs api that are
> interested.
>
Well, my patch does not fix the fundamental shortcomings
of blame nor do I intend to address them any time soon.
I simply wanted to remove the behavioral dependency on
FS implementation details. That's what this patch does and
tests pass with FSX again.
-- Stefan^2.
Re: svn commit: r1570936 - /subversion/trunk/subversion/libsvn_client/blame.c
Posted by Stefan Fuhrmann <st...@wandisco.com>.
On Sun, Feb 23, 2014 at 12:06 AM, Bert Huijben <be...@qqmail.nl> wrote:
> I think your patch doesn't change the behavior (this is more a brain
> dump based on te earlier related mail thread), but there are property
> changes that do change how we interpret the content of a file... E.g. some
> changes to svn:eol-style and svn:keywords.
>
> In these cases the actual file may change while the repository form didn't.
>
> Blame, especially with whitespace ignores might not be interested in these
> cases, but there are api users that use the file revs api that are
> interested.
>
Well, my patch does not fix the fundamental shortcomings
of blame nor do I intend to address them any time soon.
I simply wanted to remove the behavioral dependency on
FS implementation details. That's what this patch does and
tests pass with FSX again.
-- Stefan^2.