You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@subversion.apache.org by st...@apache.org on 2010/08/20 13:52:05 UTC
svn commit: r987464 - /subversion/trunk/subversion/libsvn_client/patch.c
Author: stsp
Date: Fri Aug 20 11:52:04 2010
New Revision: 987464
URL: http://svn.apache.org/viewvc?rev=987464&view=rev
Log:
* subversion/libsvn_client/patch.c: Clean up various comments and docstrings.
Apart from typo fixes, add a few suggestions regarding questions asked
in comments, and ask a couple of new questions. No functional change.
Modified:
subversion/trunk/subversion/libsvn_client/patch.c
Modified: subversion/trunk/subversion/libsvn_client/patch.c
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_client/patch.c?rev=987464&r1=987463&r2=987464&view=diff
==============================================================================
--- subversion/trunk/subversion/libsvn_client/patch.c (original)
+++ subversion/trunk/subversion/libsvn_client/patch.c Fri Aug 20 11:52:04 2010
@@ -176,7 +176,7 @@ typedef struct patch_target_t {
/* True if at least one hunk was rejected. */
svn_boolean_t had_rejects;
- /* True if atleast one property hunk was rejected. */
+ /* True if at least one property hunk was rejected. */
svn_boolean_t had_prop_rejects;
/* True if the target file had local modifications before the
@@ -256,7 +256,8 @@ strip_path(const char **result, const ch
return SVN_NO_ERROR;
}
-/* Obtain KEYWORDS, EOL_STYLE and EOL_STR for LOCAL_ABSPATH, from WC_CTX.
+/* Obtain KEYWORDS, EOL_STYLE and EOL_STR for LOCAL_ABSPATH.
+ * WC_CTX is a context for the working copy the patch is applied to.
* Use RESULT_POOL for allocations of fields in TARGET.
* Use SCRATCH_POOL for all other allocations. */
static svn_error_t *
@@ -271,7 +272,6 @@ obtain_eol_and_keywords_for_file(apr_has
apr_hash_t *props;
svn_string_t *keywords_val, *eol_style_val;
- /* Handle svn:keyword and svn:eol-style properties. */
SVN_ERR(svn_wc_prop_list2(&props, wc_ctx, local_abspath,
scratch_pool, scratch_pool));
keywords_val = apr_hash_get(props, SVN_PROP_KEYWORDS,
@@ -322,8 +322,9 @@ obtain_eol_and_keywords_for_file(apr_has
* Indicate in TARGET->SKIPPED whether the target should be skipped.
* STRIP_COUNT specifies the number of leading path components
* which should be stripped from target paths in the patch.
- * PROP_CHANGES_ONLY is used for determining if the target path is allowed
- * to be a dir.
+ * PROP_CHANGES_ONLY specifies whether the target path is allowed to have
+ * only property changes, and no content changes (in which case the target
+ * must be a directory).
* Use RESULT_POOL for allocations of fields in TARGET.
* Use SCRATCH_POOL for all other allocations. */
static svn_error_t *
@@ -437,7 +438,7 @@ resolve_target_path(patch_target_t *targ
SVN_ERR(svn_wc_read_kind(&target->db_kind, wc_ctx, target->local_abspath,
FALSE, scratch_pool));
- /* If the target is a version directory not missing from disk,
+ /* If the target is a versioned directory present on disk,
* and there are only property changes in the patch, we accept
* a directory target. Else, we skip directories. */
if (target->db_kind == svn_node_dir && ! prop_changes_only)
@@ -510,14 +511,10 @@ init_prop_target(prop_patch_target_t **p
}
if (value)
- {
- /* We assume that a property is small enough to be kept in memory during
- * the patch process. */
- content_info->stream = svn_stream_from_string(value, result_pool);
- }
+ content_info->stream = svn_stream_from_string(value, result_pool);
/* Create a temporary file to write the patched result to. For properties,
- * we don't have to worrry about different eol-style. */
+ * we don't have to worry about eol-style. ### Why not? */
SVN_ERR(svn_stream_open_unique(&content_info->patched,
&patched_path, NULL,
remove_tempfiles ?
@@ -539,7 +536,8 @@ init_prop_target(prop_patch_target_t **p
* should be skipped, PATCH_TARGET->SKIPPED is set and the target should be
* treated as not fully initialized, e.g. the caller should not not do any
* further operations on the target if it is marked to be skipped.
- * REMOVE_TEMPFILES as in svn_client_patch().
+ * If REMOVE_TEMPFILES is TRUE, set up temporary files to be removed as
+ * soon as they are no longer needed.
* Use SCRATCH_POOL for all other allocations. */
static svn_error_t *
init_patch_target(patch_target_t **patch_target,
@@ -557,7 +555,6 @@ init_patch_target(patch_target_t **patch
{
apr_hash_index_t *hi;
- /* ### Should we use an iterpool here? */
for (hi = apr_hash_first(scratch_pool, patch->prop_patches);
hi;
hi = apr_hash_next(hi))
@@ -602,18 +599,17 @@ init_patch_target(patch_target_t **patch
apr_size_t len;
svn_stream_t *patched_raw;
+ /* Create a temporary file, and associated streams,
+ * to write the patched result to. */
if (target->kind_on_disk == svn_node_file)
{
- /* Open the file. */
SVN_ERR(svn_io_file_open(&target->file, target->local_abspath,
APR_READ | APR_BINARY | APR_BUFFERED,
APR_OS_DEFAULT, result_pool));
- /* Create a stream to read from the target. */
content_info->stream = svn_stream_from_aprfile2(target->file,
FALSE, result_pool);
- /* Also check the file for local mods and the Xbit. */
SVN_ERR(svn_wc_text_modified_p2(&target->local_mods, wc_ctx,
target->local_abspath, FALSE,
scratch_pool));
@@ -630,7 +626,6 @@ init_patch_target(patch_target_t **patch
scratch_pool));
}
- /* Create a temporary file to write the patched result to. */
SVN_ERR(svn_stream_open_unique(&patched_raw,
&target->patched_path, NULL,
remove_tempfiles ?
@@ -638,16 +633,15 @@ init_patch_target(patch_target_t **patch
svn_io_file_del_none,
result_pool, scratch_pool));
- /* Expand keywords in the patched file.
- * Repair newlines if svn:eol-style dictates a particular style. */
+ /* We always expand keywords in the patched file, but repair newlines
+ * only if svn:eol-style dictates a particular style. */
repair_eol = (content_info->eol_style == svn_subst_eol_style_fixed
|| content_info->eol_style == svn_subst_eol_style_native);
content_info->patched = svn_subst_stream_translated(
patched_raw, content_info->eol_str, repair_eol,
content_info->keywords, TRUE, result_pool);
- /* We'll also need a stream to write rejected hunks to.
- * We don't expand keywords, nor normalise line-endings,
+ /* We don't expand keywords, nor normalise line-endings,
* in reject files. */
SVN_ERR(svn_stream_open_unique(&content_info->reject,
&target->reject_path, NULL,
@@ -665,7 +659,7 @@ init_patch_target(patch_target_t **patch
len = strlen(diff_header);
SVN_ERR(svn_stream_write(content_info->reject, diff_header, &len));
- /* Time for the properties */
+ /* Handle properties. */
if (! target->skipped)
{
apr_hash_index_t *hi;
@@ -678,7 +672,6 @@ init_patch_target(patch_target_t **patch
svn_prop_patch_t *prop_patch = svn__apr_hash_index_val(hi);
prop_patch_target_t *prop_target;
- /* Obtain info about this property */
SVN_ERR(init_prop_target(&prop_target,
prop_name,
prop_patch->operation,
@@ -687,7 +680,6 @@ init_patch_target(patch_target_t **patch
wc_ctx, target->local_abspath,
result_pool, scratch_pool));
- /* Store the info */
apr_hash_set(target->prop_targets, prop_name,
APR_HASH_KEY_STRING, prop_target);
}
@@ -1057,7 +1049,9 @@ match_existing_target(svn_boolean_t *mat
* RESULT_POOL. Use fuzz factor FUZZ. Set HI->FUZZ to FUZZ. If no correct
* line can be determined, set HI->REJECTED to TRUE.
* IGNORE_WHITESPACE tells whether whitespace should be considered when
- * matching. When this function returns, neither CONTENT_INFO->CURRENT_LINE nor
+ * matching. IS_PROP_HUNK indicates whether the hunk patches file content
+ * or a property.
+ * When this function returns, neither CONTENT_INFO->CURRENT_LINE nor
* the file offset in the target file will have changed.
* Call cancel CANCEL_FUNC with baton CANCEL_BATON to trigger cancellation.
* Do temporary allocations in POOL. */
@@ -1242,7 +1236,11 @@ get_hunk_info(hunk_info_t **hi, patch_ta
/* Attempt to write LEN bytes of DATA to STREAM, the underlying file
* of which is at ABSPATH. Fail if not all bytes could be written to
- * the stream. Do temporary allocations in POOL. */
+ * the stream. Do temporary allocations in POOL.
+ * ### stsp: Maybe we can remove this? It's only about checking wether
+ * ### all data was written, but we're usually doing buffered I/O
+ * ### anyway so if the disk or filesystem fails we likely won't
+ * ### see the failure right here. I've never seen this error out. */
static svn_error_t *
try_stream_write(svn_stream_t *stream, const char *abspath,
const char *data, apr_size_t len, apr_pool_t *pool)
@@ -1313,6 +1311,8 @@ reject_hunk(patch_target_t *target, targ
SVN_ERR(svn_stream_write(content_info->reject, prop_header, &len));
+ /* ### What about just setting a variable to either "@@" or "##",
+ * ### and merging with the else clause below? */
hunk_header = apr_psprintf(pool, "## -%lu,%lu +%lu,%lu ##%s",
svn_diff_hunk_get_original_start(hi->hunk),
svn_diff_hunk_get_original_length(hi->hunk),
@@ -1362,8 +1362,11 @@ reject_hunk(patch_target_t *target, targ
return SVN_NO_ERROR;
}
-/* Write the modified text of hunk described by HI to the patched
- * stream of CONTENT_INFO. Do temporary allocations in POOL. */
+/* Write the modified text of the hunk described by HI to the patched
+ * stream of CONTENT_INFO. TARGET is the patch target.
+ * If PROP_NAME is not NULL, the hunk is assumed to be targeted for
+ * a property with the given name.
+ * Do temporary allocations in POOL. */
static svn_error_t *
apply_hunk(patch_target_t *target, target_content_info_t *content_info,
hunk_info_t *hi, const char *prop_name, apr_pool_t *pool)
@@ -1449,7 +1452,8 @@ apply_hunk(patch_target_t *target, targe
/* Use client context CTX to send a suitable notification for hunk HI,
* using TARGET to determine the path. If the hunk is a property hunk,
- * PROP_NAME is set, else NULL. Use POOL for temporary allocations. */
+ * PROP_NAME must be the name of the property, else NULL.
+ * Use POOL for temporary allocations. */
static svn_error_t *
send_hunk_notification(const hunk_info_t *hi,
const patch_target_t *target,
@@ -1705,7 +1709,7 @@ apply_one_patch(patch_target_t **patch_t
}
}
- /* Match property hunks. */
+ /* Match property hunks. ### Can we use scratch_pool here? */
for (hash_index = apr_hash_first(result_pool, patch->prop_patches);
hash_index;
hash_index = apr_hash_next(hash_index))
@@ -1715,12 +1719,11 @@ apply_one_patch(patch_target_t **patch_t
prop_patch_target_t *prop_target;
target_content_info_t *prop_content_info;
- /* Fetching the parsed info for one property */
prop_name = svn__apr_hash_index_key(hash_index);
prop_patch = svn__apr_hash_index_val(hash_index);
- /* Fetch the prop_content_info we'll use to store the matched hunks
- * in. */
+ /* We'll store matched hunks in prop_content_info.
+ * ### Just use prop_target->content_info? */
prop_target = apr_hash_get(target->prop_targets, prop_name,
APR_HASH_KEY_STRING);
prop_content_info = prop_target->content_info;
@@ -1765,7 +1768,9 @@ apply_one_patch(patch_target_t **patch_t
const char *prop_patched_path;
prop_target = svn__apr_hash_index_val(hash_index);
+ /* ### Just use prop_target->content_info? */
prop_content_info = prop_target->content_info;
+ /* ### Just use prop_target->patched_path? */
prop_patched_path = prop_target->patched_path;
for (i = 0; i < prop_content_info->hunks->nelts; i++)
@@ -1801,7 +1806,10 @@ apply_one_patch(patch_target_t **patch_t
* ### dannas: Do we really want to skip an entire target
* ### if one of the properties does not apply cleanly,
* ### e.g. both text changes and all prop changes will not be
- * ### installed. */
+ * ### installed.
+ * ### stsp: This is a "should never happen" situation.
+ * ### It means we've run out of disk space or something
+ * ### like that, so skipping is appropriate. */
target->skipped = TRUE;
}
}
@@ -1809,6 +1817,8 @@ apply_one_patch(patch_target_t **patch_t
svn_pool_destroy(iterpool);
+ /* ### Move this a separate function? apply_one_patch() has gotten quite
+ * ### big. We should consider splitting it up into several pieces. */
{
apr_hash_index_t *hi;
target_content_info_t *prop_content_info;
@@ -1966,7 +1976,7 @@ create_missing_parents(patch_target_t *t
}
else
{
- /* It's not a file, it's not a dir..
+ /* It's not a file, it's not a dir...
Let's add a dir */
break;
}
@@ -2149,7 +2159,7 @@ write_out_rejected_hunks(patch_target_t
/* Install the patched properties for TARGET. Use client context CTX to
* retrieve WC_CTX. If DRY_RUN is TRUE, don't modify the working copy.
- * Do tempoary allocations in SCRATCH_POOL. */
+ * Do temporary allocations in SCRATCH_POOL. */
static svn_error_t *
install_patched_prop_targets(patch_target_t *target,
svn_client_ctx_t *ctx, svn_boolean_t dry_run,
@@ -2188,12 +2198,13 @@ install_patched_prop_targets(patch_targe
continue;
}
- /* A property is usually one line long.
- * ### Is this the optimal size to allocate? */
+ /* A property is usually small, at most a couple of bytes.
+ * Start out assuming it won't be larger than a typical line of text. */
prop_content = svn_stringbuf_create_ensure(80, scratch_pool);
/* svn_wc_prop_set4() wants a svn_string_t for input so we need to
- * open the tmp file for reading again. */
+ * open the tmp file for reading again.
+ * ### Just keep it open? */
SVN_ERR(svn_io_file_open(&file, prop_target->patched_path,
APR_READ | APR_BINARY, APR_OS_DEFAULT,
scratch_pool));
@@ -2217,7 +2228,10 @@ install_patched_prop_targets(patch_targe
svn_stream_close(patched_stream);
/* ### How should we handle SVN_ERR_ILLEGAL_TARGET and
- * ### SVN_ERR_BAD_MIME_TYPE? */
+ * ### SVN_ERR_BAD_MIME_TYPE?
+ *
+ * ### stsp: I'd say skip the target, patch is illegal.
+ * ### Revert any changes already made to file content, too! */
SVN_ERR(svn_wc_prop_set4(ctx->wc_ctx, target->local_abspath,
prop_target->name,
svn_string_create_from_buf(prop_content,