You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@subversion.apache.org by da...@apache.org on 2015/09/26 02:46:52 UTC
svn commit: r1705391 - in /subversion/trunk: ./ subversion/include/
subversion/include/private/ subversion/libsvn_client/
subversion/libsvn_diff/ subversion/libsvn_fs_x/ subversion/tests/cmdline/
subversion/tests/libsvn_diff/
Author: danielsh
Date: Sat Sep 26 00:46:52 2015
New Revision: 1705391
URL: http://svn.apache.org/viewvc?rev=1705391&view=rev
Log:
patch: Parse 'old mode' / 'new mode' line from git diffs and translate them
to svn:executable propchanges.
This merges the 'patch-exec' branch into trunk.
---
The following are the major changes. See the branch for detailed changes.
* subversion/include/svn_diff.h
(svn_patch_t.old_executable_p, svn_patch_t.new_executable_p): New members.
* subversion/libsvn_client/patch.c
(contradictory_executability): New function.
(init_patch_target, apply_one_patch):
Translate mode changes to svn:executable changes.
* subversion/include/private/svn_diff_private.h
(svn_diff_hunk__create_adds_single_line,
svn_diff_hunk__create_deletes_single_line): New functions.
* subversion/libsvn_diff/parse-diff.c
(add_or_delete_single_line,
svn_diff_hunk__create_adds_single_line,
svn_diff_hunk__create_deletes_single_line): New functions.
(parse_state::state_old_mode_seen,
parse_state::state_git_mode_seen): New enumerators.
(parse_bits_into_executability, git_old_mode, git_new_mode): New functions.
(git_deleted_file, git_new_file): Parse file mode.
(transitions): Parse "old mode" and "new mode" lines.
* subversion/tests/cmdline/patch_tests.py,
* subversion/tests/libsvn_diff/parse-diff-test.c:
Add unit tests.
Modified:
subversion/trunk/ (props changed)
subversion/trunk/subversion/include/private/svn_diff_private.h
subversion/trunk/subversion/include/svn_diff.h
subversion/trunk/subversion/libsvn_client/patch.c
subversion/trunk/subversion/libsvn_diff/parse-diff.c
subversion/trunk/subversion/libsvn_fs_x/ (props changed)
subversion/trunk/subversion/tests/cmdline/patch_tests.py
subversion/trunk/subversion/tests/libsvn_diff/parse-diff-test.c
Propchange: subversion/trunk/
------------------------------------------------------------------------------
--- svn:mergeinfo (original)
+++ svn:mergeinfo Sat Sep 26 00:46:52 2015
@@ -62,6 +62,7 @@
/subversion/branches/multi-layer-moves:1239019-1300930
/subversion/branches/nfc-nfd-aware-client:870276,870376
/subversion/branches/node_pool:1304828-1305388
+/subversion/branches/patch-exec:1692717-1705390
/subversion/branches/performance:979193,980118,981087,981090,981189,981194,981287,981684,981827,982043,982355,983398,983406,983430,983474,983488,983490,983760,983764,983766,983770,984927,984973,984984,985014,985037,985046,985472,985477,985482,985487-985488,985493,985497,985500,985514,985601,985603,985606,985669,985673,985695,985697,986453,986465,986485,986491-986492,986517,986521,986605,986608,986817,986832,987865,987868-987869,987872,987886-987888,987893,988319,988898,990330,990533,990535-990537,990541,990568,990572,990574-990575,990600,990759,992899,992904,992911,993127,993141,994956,995478,995507,995603,998012,998858,999098,1001413,1001417,1004291,1022668,1022670,1022676,1022715,1022719,1025660,1025672,1027193,1027203,1027206,1027214,1027227,1028077,1028092,1028094,1028104,1028107,1028111,1028354,1029038,1029042-1029043,1029054-1029055,1029062-1029063,1029078,1029080,1029090,1029092-1029093,1029111,1029151,1029158,1029229-1029230,1029232,1029335-1029336,1029339-1029340,1029342,10
29344,1030763,1030827,1031203,1031235,1032285,1032333,1033040,1033057,1033294,1035869,1035882,1039511,1043705,1053735,1056015,1066452,1067683,1067697-1078365
/subversion/branches/pin-externals:1643757-1659392
/subversion/branches/py-tests-as-modules:956579-1033052
Modified: subversion/trunk/subversion/include/private/svn_diff_private.h
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/include/private/svn_diff_private.h?rev=1705391&r1=1705390&r2=1705391&view=diff
==============================================================================
--- subversion/trunk/subversion/include/private/svn_diff_private.h (original)
+++ subversion/trunk/subversion/include/private/svn_diff_private.h Sat Sep 26 00:46:52 2015
@@ -113,6 +113,33 @@ svn_diff__display_prop_diffs(svn_stream_
void *cancel_baton,
apr_pool_t *scratch_pool);
+/** Create a hunk object that adds a single line. Return the new object
+ * in @a *hunk.
+ *
+ * @a line is the added text, without a trailing newline.
+ *
+ * The hunk will be associated with @a patch.
+ */
+svn_error_t *
+svn_diff_hunk__create_adds_single_line(svn_diff_hunk_t **hunk,
+ const char *line,
+ svn_patch_t *patch,
+ apr_pool_t *result_pool,
+ apr_pool_t *scratch_pool);
+
+/** Create a hunk object that deletes a single line. Return the new object
+ * in @a *hunk.
+ *
+ * @a line is the deleted text, without a trailing newline.
+ *
+ * The hunk will be associated with @a patch.
+ */
+svn_error_t *
+svn_diff_hunk__create_deletes_single_line(svn_diff_hunk_t **hunk,
+ const char *line,
+ svn_patch_t *patch,
+ apr_pool_t *result_pool,
+ apr_pool_t *scratch_pool);
#ifdef __cplusplus
}
Modified: subversion/trunk/subversion/include/svn_diff.h
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/include/svn_diff.h?rev=1705391&r1=1705390&r2=1705391&view=diff
==============================================================================
--- subversion/trunk/subversion/include/svn_diff.h (original)
+++ subversion/trunk/subversion/include/svn_diff.h Sat Sep 26 00:46:52 2015
@@ -1288,6 +1288,24 @@ typedef struct svn_patch_t {
* to apply it as parsed from the file.
* @since New in 1.10. */
svn_diff_binary_patch_t *binary_patch;
+
+ /** The old and new executability bits, as retrieved from the patch file.
+ *
+ * A patch may specify an executability change via @a old_executable_p and
+ * / @a new_executable_p, via a #SVN_PROP_EXECUTABLE propchange hunk, or both
+ * ways; however, if both ways are used, they must specify the same semantic
+ * change.
+ *
+ * #svn_tristate_unknown indicates the patch does not specify the
+ * corresponding bit.
+ *
+ * @since New in 1.10.
+ */
+ /* ### This is currently not parsed out of "index" lines (where it
+ * ### serves as an assertion of the executability state, without
+ * ### changing it). */
+ svn_tristate_t old_executable_p;
+ svn_tristate_t new_executable_p;
} svn_patch_t;
/** An opaque type representing an open patch file.
Modified: subversion/trunk/subversion/libsvn_client/patch.c
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_client/patch.c?rev=1705391&r1=1705390&r2=1705391&view=diff
==============================================================================
--- subversion/trunk/subversion/libsvn_client/patch.c (original)
+++ subversion/trunk/subversion/libsvn_client/patch.c Sat Sep 26 00:46:52 2015
@@ -46,6 +46,7 @@
#include "private/svn_eol_private.h"
#include "private/svn_wc_private.h"
#include "private/svn_dep_compat.h"
+#include "private/svn_diff_private.h"
#include "private/svn_string_private.h"
#include "private/svn_subr_private.h"
#include "private/svn_sorts_private.h"
@@ -914,6 +915,41 @@ choose_target_filename(const svn_patch_t
return (old < new) ? patch->old_filename : patch->new_filename;
}
+/* Return whether the svn:executable proppatch and the out-of-band
+ * executability metadata contradict each other, assuming both are present.
+ */
+static svn_boolean_t
+contradictory_executability(const svn_patch_t *patch,
+ const prop_patch_target_t *target)
+{
+ switch (target->operation)
+ {
+ case svn_diff_op_added:
+ return patch->new_executable_p == svn_tristate_false;
+
+ case svn_diff_op_deleted:
+ return patch->new_executable_p == svn_tristate_true;
+
+ case svn_diff_op_unchanged:
+ /* ### Can this happen? */
+ return (patch->old_executable_p != svn_tristate_unknown
+ && patch->new_executable_p != svn_tristate_unknown
+ && patch->old_executable_p != patch->new_executable_p);
+
+ case svn_diff_op_modified:
+ /* Can't happen: the property should only ever be added or deleted,
+ * but never modified from one valid value to another. */
+ return (patch->old_executable_p != svn_tristate_unknown
+ && patch->new_executable_p != svn_tristate_unknown
+ && patch->old_executable_p == patch->new_executable_p);
+
+ default:
+ /* Can't happen: the proppatch parser never generates other values. */
+ SVN_ERR_MALFUNCTION_NO_RETURN();
+ }
+}
+
+
/* Attempt to initialize a *PATCH_TARGET structure for a target file
* described by PATCH. Use working copy context WC_CTX.
* STRIP_COUNT specifies the number of leading path components
@@ -956,6 +992,9 @@ init_patch_target(patch_target_t **patch
target->kind_on_disk = svn_node_none;
target->content = content;
target->prop_targets = apr_hash_make(result_pool);
+ if (patch->new_executable_p != svn_tristate_unknown)
+ /* May also be set by apply_hunk(). */
+ target->has_prop_changes = TRUE;
SVN_ERR(resolve_target_path(target, choose_target_filename(patch),
root_abspath, strip_count, has_text_changes,
@@ -1129,6 +1168,7 @@ init_patch_target(patch_target_t **patch
if (! target->skipped)
{
apr_hash_index_t *hi;
+ prop_patch_target_t *prop_executable_target;
for (hi = apr_hash_first(result_pool, patch->prop_patches);
hi;
@@ -1145,6 +1185,99 @@ init_patch_target(patch_target_t **patch
result_pool, scratch_pool));
svn_hash_sets(target->prop_targets, prop_name, prop_target);
}
+
+ /* Now, check for an out-of-band mode change and convert it to
+ * an svn:executable property patch. */
+ prop_executable_target = svn_hash_gets(target->prop_targets,
+ SVN_PROP_EXECUTABLE);
+ if (patch->new_executable_p != svn_tristate_unknown
+ && prop_executable_target)
+ {
+ if (contradictory_executability(patch, prop_executable_target))
+ /* Invalid input: specifies both git-like "new mode" lines and
+ * svn-like addition/removal of svn:executable.
+ *
+ * If this were merely a hunk that didn't apply, we'd reject it
+ * and move on. However, this is a self-contradictory hunk;
+ * it has no unambiguous interpretation. Therefore: */
+ return svn_error_createf(SVN_ERR_INVALID_INPUT, NULL,
+ _("Invalid patch: specifies "
+ "contradicting mode changes and "
+ "%s changes (for '%s')"),
+ SVN_PROP_EXECUTABLE,
+ target->local_abspath);
+ else
+ /* We have two representations of the same change.
+ *
+ * In the caller, there will be two hunk_info_t's for the
+ * patch: one generated from the property diff and one
+ * generated from the out-of-band mode change. Both hunks will
+ * be processed, but the output will be as though there was
+ * just one hunk.
+ *
+ * The reason there will be only a single notification is not
+ * specific to SVN_PROP_EXECUTABLE but generic to all property
+ * patches: if a patch file contains two identical property
+ * hunks (e.g.,
+ * svn ps k v iota; svn diff iota >patch; svn diff iota >>patch
+ * ), applying the patch file will only produce a single ' U'
+ * notification.
+ *
+ * In contrast, the same for file-content hunks will result in
+ * a 'U' notification followed by a 'G' notification. (The 'U'
+ * for the first copy of the hunk; the 'G' for the second.)
+ */
+ ;
+ }
+ else if (patch->new_executable_p != svn_tristate_unknown
+ && !prop_executable_target)
+ {
+ svn_diff_operation_kind_t operation;
+ svn_boolean_t nothing_to_do = FALSE;
+ prop_patch_target_t *prop_target;
+
+ if (patch->old_executable_p == patch->new_executable_p)
+ {
+ /* Noop change. */
+ operation = svn_diff_op_unchanged;
+ }
+ else switch (patch->old_executable_p)
+ {
+ case svn_tristate_false:
+ /* Made executable. */
+ operation = svn_diff_op_added;
+ break;
+
+ case svn_tristate_true:
+ /* Made non-executable. */
+ operation = svn_diff_op_deleted;
+ break;
+
+ case svn_tristate_unknown:
+ if (patch->new_executable_p == svn_tristate_true)
+ /* New, executable file. */
+ operation = svn_diff_op_added;
+ else
+ /* New, non-executable file. That's not a change. */
+ nothing_to_do = TRUE;
+ break;
+
+ default:
+ /* NOTREACHED */
+ SVN_ERR_MALFUNCTION();
+ }
+
+ if (! nothing_to_do)
+ {
+ SVN_ERR(init_prop_target(&prop_target,
+ SVN_PROP_EXECUTABLE,
+ operation,
+ wc_ctx, target->local_abspath,
+ result_pool, scratch_pool));
+ svn_hash_sets(target->prop_targets, SVN_PROP_EXECUTABLE,
+ prop_target);
+ }
+ }
}
}
@@ -2425,6 +2558,50 @@ apply_one_patch(patch_target_t **patch_t
}
}
+ /* Match implied property hunks. */
+ if (patch->new_executable_p != svn_tristate_unknown
+ && svn_hash_gets(target->prop_targets, SVN_PROP_EXECUTABLE))
+ {
+ hunk_info_t *hi;
+ svn_diff_hunk_t *hunk;
+ prop_patch_target_t *prop_target = svn_hash_gets(target->prop_targets,
+ SVN_PROP_EXECUTABLE);
+ const char *const value = SVN_PROP_EXECUTABLE_VALUE;
+
+ switch (prop_target->operation)
+ {
+ case svn_diff_op_added:
+ SVN_ERR(svn_diff_hunk__create_adds_single_line(&hunk, value, patch,
+ result_pool,
+ iterpool));
+ break;
+
+ case svn_diff_op_deleted:
+ SVN_ERR(svn_diff_hunk__create_deletes_single_line(&hunk, value,
+ patch,
+ result_pool,
+ iterpool));
+ break;
+
+ case svn_diff_op_unchanged:
+ /* ### What to do? */
+ break;
+
+ default:
+ SVN_ERR_MALFUNCTION();
+ }
+
+ /* Derive a hunk_info from hunk. */
+ SVN_ERR(get_hunk_info(&hi, target, prop_target->content,
+ hunk, 0 /* fuzz */, 0 /* previous_offset */,
+ ignore_whitespace,
+ TRUE /* is_prop_hunk */,
+ cancel_func, cancel_baton,
+ result_pool, iterpool));
+ if (! hi->already_applied)
+ APR_ARRAY_PUSH(prop_target->content->hunks, hunk_info_t *) = hi;
+ }
+
/* Apply or reject property hunks. */
for (hash_index = apr_hash_first(scratch_pool, target->prop_targets);
hash_index;
Modified: subversion/trunk/subversion/libsvn_diff/parse-diff.c
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_diff/parse-diff.c?rev=1705391&r1=1705390&r2=1705391&view=diff
==============================================================================
--- subversion/trunk/subversion/libsvn_diff/parse-diff.c (original)
+++ subversion/trunk/subversion/libsvn_diff/parse-diff.c Sat Sep 26 00:46:52 2015
@@ -40,6 +40,7 @@
#include "private/svn_eol_private.h"
#include "private/svn_dep_compat.h"
+#include "private/svn_diff_private.h"
#include "private/svn_sorts_private.h"
#include "diff.h"
@@ -108,6 +109,116 @@ struct svn_diff_binary_patch_t {
svn_filesize_t dst_filesize; /* Expanded/final size */
};
+/* Common guts of svn_diff_hunk__create_adds_single_line() and
+ * svn_diff_hunk__create_deletes_single_line().
+ *
+ * ADD is TRUE if adding and FALSE if deleting.
+ */
+static svn_error_t *
+add_or_delete_single_line(svn_diff_hunk_t **hunk_out,
+ const char *line,
+ svn_patch_t *patch,
+ svn_boolean_t add,
+ apr_pool_t *result_pool,
+ apr_pool_t *scratch_pool)
+{
+ svn_diff_hunk_t *hunk = apr_palloc(result_pool, sizeof(*hunk));
+ static const char *hunk_header[] = { "@@ -1 +0,0 @@\n", "@@ -0,0 +1 @@\n" };
+ const unsigned len = strlen(line);
+ /* The +1 is for the 'plus' start-of-line character. */
+ const apr_off_t end = STRLEN_LITERAL(hunk_header[add]) + (1 + len);
+ /* The +1 is for the second \n. */
+ svn_stringbuf_t *buf = svn_stringbuf_create_ensure(end + 1, scratch_pool);
+
+ hunk->patch = patch;
+
+ /* hunk->apr_file is created below. */
+
+ hunk->diff_text_range.start = STRLEN_LITERAL(hunk_header[add]);
+ hunk->diff_text_range.current = STRLEN_LITERAL(hunk_header[add]);
+ hunk->diff_text_range.end = end;
+
+ if (add)
+ {
+ hunk->original_text_range.start = 0; /* There's no "original" text. */
+ hunk->original_text_range.current = 0;
+ hunk->original_text_range.end = 0;
+
+ hunk->modified_text_range.start = STRLEN_LITERAL(hunk_header[add]);
+ hunk->modified_text_range.current = STRLEN_LITERAL(hunk_header[add]);
+ hunk->modified_text_range.end = end;
+
+ hunk->original_start = 0;
+ hunk->original_length = 0;
+
+ hunk->modified_start = 1;
+ hunk->modified_length = 1;
+ }
+ else /* delete */
+ {
+ hunk->original_text_range.start = STRLEN_LITERAL(hunk_header[add]);
+ hunk->original_text_range.current = STRLEN_LITERAL(hunk_header[add]);
+ hunk->original_text_range.end = end;
+
+ hunk->modified_text_range.start = 0; /* There's no "original" text. */
+ hunk->modified_text_range.current = 0;
+ hunk->modified_text_range.end = 0;
+
+ hunk->original_start = 1;
+ hunk->original_length = 1;
+
+ hunk->modified_start = 0;
+ hunk->modified_length = 0; /* setting to '1' works too */
+ }
+
+ hunk->leading_context = 0;
+ hunk->trailing_context = 0;
+
+ /* Create APR_FILE and put just a hunk in it (without a diff header).
+ * Save the offset of the last byte of the diff line. */
+ svn_stringbuf_appendbytes(buf, hunk_header[add],
+ STRLEN_LITERAL(hunk_header[add]));
+ svn_stringbuf_appendbyte(buf, add ? '+' : '-');
+ svn_stringbuf_appendbytes(buf, line, len);
+ svn_stringbuf_appendbyte(buf, '\n');
+
+ SVN_ERR(svn_io_open_unique_file3(&hunk->apr_file, NULL /* filename */,
+ NULL /* system tempdir */,
+ svn_io_file_del_on_pool_cleanup,
+ result_pool, scratch_pool));
+ SVN_ERR(svn_io_file_write_full(hunk->apr_file,
+ buf->data, buf->len,
+ NULL, scratch_pool));
+ /* No need to seek. */
+
+ *hunk_out = hunk;
+ return SVN_NO_ERROR;
+}
+
+svn_error_t *
+svn_diff_hunk__create_adds_single_line(svn_diff_hunk_t **hunk_out,
+ const char *line,
+ svn_patch_t *patch,
+ apr_pool_t *result_pool,
+ apr_pool_t *scratch_pool)
+{
+ SVN_ERR(add_or_delete_single_line(hunk_out, line, patch, TRUE,
+ result_pool, scratch_pool));
+ return SVN_NO_ERROR;
+}
+
+svn_error_t *
+svn_diff_hunk__create_deletes_single_line(svn_diff_hunk_t **hunk_out,
+ const char *line,
+ svn_patch_t *patch,
+ apr_pool_t *result_pool,
+ apr_pool_t *scratch_pool)
+{
+ SVN_ERR(add_or_delete_single_line(hunk_out, line, patch, FALSE,
+ result_pool, scratch_pool));
+ return SVN_NO_ERROR;
+}
+
void
svn_diff_hunk_reset_diff_text(svn_diff_hunk_t *hunk)
{
@@ -1240,6 +1351,8 @@ enum parse_state
state_git_tree_seen, /* a tree operation, rather than content change */
state_git_minus_seen, /* --- /dev/null; or --- a/ */
state_git_plus_seen, /* +++ /dev/null; or +++ a/ */
+ state_old_mode_seen, /* old mode 100644 */
+ state_git_mode_seen, /* new mode 100644 */
state_move_from_seen, /* rename from foo.c */
state_copy_from_seen, /* copy from foo.c */
state_minus_seen, /* --- foo.c */
@@ -1472,6 +1585,85 @@ git_plus(enum parse_state *new_state, ch
return SVN_NO_ERROR;
}
+/* Helper for git_old_mode() and git_new_mode(). Translate the git
+ * file mode MODE_STR into a binary "executable?" notion EXECUTABLE_P. */
+static svn_error_t *
+parse_bits_into_executability(svn_tristate_t *executable_p,
+ const char *mode_str)
+{
+ apr_uint64_t mode;
+ SVN_ERR(svn_cstring_strtoui64(&mode, mode_str,
+ 0 /* min */,
+ 0777777 /* max: six octal digits */,
+ 010 /* radix (octal) */));
+
+ /* Note: 0644 and 0755 are the only modes that can occur for plain files.
+ * We deliberately choose to parse only those values: we are strict in what
+ * we accept _and_ in what we produce.
+ *
+ * (Having said that, though, we could consider relaxing the parser to also
+ * map
+ * (mode & 0111) == 0000 -> svn_tristate_false
+ * (mode & 0111) == 0111 -> svn_tristate_true
+ * [anything else] -> svn_tristate_unknown
+ * .)
+ */
+
+ switch (mode & 0777)
+ {
+ case 0644:
+ *executable_p = svn_tristate_false;
+ break;
+
+ case 0755:
+ *executable_p = svn_tristate_true;
+ break;
+
+ default:
+ /* Ignore unknown values. */
+ *executable_p = svn_tristate_unknown;
+ break;
+ }
+
+ return SVN_NO_ERROR;
+}
+
+/* Parse the 'old mode ' line of a git extended unidiff. */
+static svn_error_t *
+git_old_mode(enum parse_state *new_state, char *line, svn_patch_t *patch,
+ apr_pool_t *result_pool, apr_pool_t *scratch_pool)
+{
+ SVN_ERR(parse_bits_into_executability(&patch->old_executable_p,
+ line + STRLEN_LITERAL("old mode ")));
+
+#ifdef SVN_DEBUG
+ /* If this assert trips, the "old mode" is neither ...644 nor ...755 . */
+ SVN_ERR_ASSERT(patch->old_executable_p != svn_tristate_unknown);
+#endif
+
+ *new_state = state_old_mode_seen;
+ return SVN_NO_ERROR;
+}
+
+/* Parse the 'new mode ' line of a git extended unidiff. */
+static svn_error_t *
+git_new_mode(enum parse_state *new_state, char *line, svn_patch_t *patch,
+ apr_pool_t *result_pool, apr_pool_t *scratch_pool)
+{
+ SVN_ERR(parse_bits_into_executability(&patch->new_executable_p,
+ line + STRLEN_LITERAL("new mode ")));
+
+#ifdef SVN_DEBUG
+ /* If this assert trips, the "old mode" is neither ...644 nor ...755 . */
+ SVN_ERR_ASSERT(patch->new_executable_p != svn_tristate_unknown);
+#endif
+
+ /* Don't touch patch->operation. */
+
+ *new_state = state_git_mode_seen;
+ return SVN_NO_ERROR;
+}
+
/* Parse the 'rename from ' line of a git extended unidiff. */
static svn_error_t *
git_move_from(enum parse_state *new_state, char *line, svn_patch_t *patch,
@@ -1532,6 +1724,10 @@ static svn_error_t *
git_new_file(enum parse_state *new_state, char *line, svn_patch_t *patch,
apr_pool_t *result_pool, apr_pool_t *scratch_pool)
{
+ SVN_ERR(
+ parse_bits_into_executability(&patch->new_executable_p,
+ line + STRLEN_LITERAL("new file mode ")));
+
patch->operation = svn_diff_op_added;
/* Filename already retrieved from diff --git header. */
@@ -1545,6 +1741,10 @@ static svn_error_t *
git_deleted_file(enum parse_state *new_state, char *line, svn_patch_t *patch,
apr_pool_t *result_pool, apr_pool_t *scratch_pool)
{
+ SVN_ERR(
+ parse_bits_into_executability(&patch->old_executable_p,
+ line + STRLEN_LITERAL("deleted file mode ")));
+
patch->operation = svn_diff_op_deleted;
/* Filename already retrieved from diff --git header. */
@@ -1808,15 +2008,22 @@ static struct transition transitions[] =
{"diff --git", state_start, git_start},
{"--- a/", state_git_diff_seen, git_minus},
+ {"--- a/", state_git_mode_seen, git_minus},
{"--- a/", state_git_tree_seen, git_minus},
+ {"--- /dev/null", state_git_mode_seen, git_minus},
{"--- /dev/null", state_git_tree_seen, git_minus},
{"+++ b/", state_git_minus_seen, git_plus},
{"+++ /dev/null", state_git_minus_seen, git_plus},
+ {"old mode ", state_git_diff_seen, git_old_mode},
+ {"new mode ", state_old_mode_seen, git_new_mode},
+
{"rename from ", state_git_diff_seen, git_move_from},
+ {"rename from ", state_git_mode_seen, git_move_from},
{"rename to ", state_move_from_seen, git_move_to},
{"copy from ", state_git_diff_seen, git_copy_from},
+ {"copy from ", state_git_mode_seen, git_copy_from},
{"copy to ", state_copy_from_seen, git_copy_to},
{"new file ", state_git_diff_seen, git_new_file},
@@ -1850,6 +2057,8 @@ svn_diff_parse_next_patch(svn_patch_t **
}
patch = apr_pcalloc(result_pool, sizeof(*patch));
+ patch->old_executable_p = svn_tristate_unknown;
+ patch->new_executable_p = svn_tristate_unknown;
pos = patch_file->next_patch_offset;
SVN_ERR(svn_io_file_seek(patch_file->apr_file, APR_SET, &pos, scratch_pool));
@@ -1896,7 +2105,8 @@ svn_diff_parse_next_patch(svn_patch_t **
/* We have a valid diff header, yay! */
break;
}
- else if (state == state_git_tree_seen && line_after_tree_header_read
+ else if ((state == state_git_tree_seen || state == state_git_mode_seen)
+ && line_after_tree_header_read
&& !valid_header_line)
{
/* git patches can contain an index line after the file mode line */
@@ -1911,7 +2121,8 @@ svn_diff_parse_next_patch(svn_patch_t **
break;
}
}
- else if (state == state_git_tree_seen)
+ else if (state == state_git_tree_seen
+ || state == state_git_mode_seen)
{
line_after_tree_header_read = TRUE;
}
Propchange: subversion/trunk/subversion/libsvn_fs_x/
------------------------------------------------------------------------------
--- svn:mergeinfo (original)
+++ svn:mergeinfo Sat Sep 26 00:46:52 2015
@@ -61,6 +61,7 @@
/subversion/branches/multi-layer-moves/subversion/libsvn_fs_x:1239019-1300930
/subversion/branches/nfc-nfd-aware-client/subversion/libsvn_fs_x:870276,870376
/subversion/branches/node_pool/subversion/libsvn_fs_x:1304828-1305388
+/subversion/branches/patch-exec/subversion/libsvn_fs_x:1692717-1705390
/subversion/branches/performance/subversion/libsvn_fs_x:979193,980118,981087,981090,981189,981194,981287,981684,981827,982043,982355,983398,983406,983430,983474,983488,983490,983760,983764,983766,983770,984927,984973,984984,985014,985037,985046,985472,985477,985482,985487-985488,985493,985497,985500,985514,985601,985603,985606,985669,985673,985695,985697,986453,986465,986485,986491-986492,986517,986521,986605,986608,986817,986832,987865,987868-987869,987872,987886-987888,987893,988319,988898,990330,990533,990535-990537,990541,990568,990572,990574-990575,990600,990759,992899,992904,992911,993127,993141,994956,995478,995507,995603,998012,998858,999098,1001413,1001417,1004291,1022668,1022670,1022676,1022715,1022719,1025660,1025672,1027193,1027203,1027206,1027214,1027227,1028077,1028092,1028094,1028104,1028107,1028111,1028354,1029038,1029042-1029043,1029054-1029055,1029062-1029063,1029078,1029080,1029090,1029092-1029093,1029111,1029151,1029158,1029229-1029230,1029232,1029335-1029336,102
9339-1029340,1029342,1029344,1030763,1030827,1031203,1031235,1032285,1032333,1033040,1033057,1033294,1035869,1035882,1039511,1043705,1053735,1056015,1066452,1067683,1067697-1078365
/subversion/branches/pin-externals/subversion/libsvn_fs_x:1643757-1659392
/subversion/branches/py-tests-as-modules/subversion/libsvn_fs_x:956579-1033052
Modified: subversion/trunk/subversion/tests/cmdline/patch_tests.py
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/tests/cmdline/patch_tests.py?rev=1705391&r1=1705390&r2=1705391&view=diff
==============================================================================
--- subversion/trunk/subversion/tests/cmdline/patch_tests.py (original)
+++ subversion/trunk/subversion/tests/cmdline/patch_tests.py Sat Sep 26 00:46:52 2015
@@ -5962,6 +5962,189 @@ def patch_final_eol(sbox):
expected_status, expected_skip,
[], False, True, '--reverse-diff')
+def patch_adds_executability_nocontents(sbox):
+ """patch adds svn:executable, without contents"""
+
+ sbox.build(read_only=True)
+ wc_dir = sbox.wc_dir
+
+ unidiff_patch = (
+ "diff --git a/iota b/iota\n"
+ "old mode 100644\n"
+ "new mode 100755\n"
+ )
+ patch_file_path = make_patch_path(sbox)
+ svntest.main.file_write(patch_file_path, unidiff_patch)
+
+ expected_output = [
+ ' U %s\n' % sbox.ospath('iota'),
+ ]
+ expected_disk = svntest.main.greek_state.copy()
+ # "*" is SVN_PROP_EXECUTABLE_VALUE aka SVN_PROP_BOOLEAN_TRUE
+ expected_disk.tweak('iota', props={'svn:executable': '*'})
+
+ expected_status = svntest.actions.get_virginal_state(wc_dir, 1)
+ expected_status.tweak('iota', status=' M')
+
+ expected_skip = wc.State('', { })
+
+ svntest.actions.run_and_verify_patch(wc_dir, os.path.abspath(patch_file_path),
+ expected_output, expected_disk,
+ expected_status, expected_skip,
+ check_props=True)
+
+def patch_adds_executability_yescontents(sbox):
+ """patch adds svn:executable, with contents"""
+
+ sbox.build(read_only=True)
+ wc_dir = sbox.wc_dir
+
+ mu_new_contents = (
+ "This is the file 'mu'.\n"
+ "with text mods too\n"
+ )
+
+ unidiff_patch = (
+ "diff --git a/A/mu b/A/mu\n"
+ "old mode 100644\n"
+ "new mode 100755\n"
+ "index 8a0f01c..dfad3ac\n"
+ "--- a/A/mu\n"
+ "+++ b/A/mu\n"
+ "@@ -1 +1,2 @@\n"
+ " This is the file 'mu'.\n"
+ "+with text mods too\n"
+ )
+ patch_file_path = make_patch_path(sbox)
+ svntest.main.file_write(patch_file_path, unidiff_patch)
+
+ expected_output = [
+ 'UU %s\n' % sbox.ospath('A/mu'),
+ ]
+ expected_disk = svntest.main.greek_state.copy()
+ # "*" is SVN_PROP_EXECUTABLE_VALUE aka SVN_PROP_BOOLEAN_TRUE
+ expected_disk.tweak('A/mu', props={'svn:executable': '*'})
+ expected_disk.tweak('A/mu', contents=mu_new_contents)
+
+ expected_status = svntest.actions.get_virginal_state(wc_dir, 1)
+ expected_status.tweak('A/mu', status='MM')
+
+ expected_skip = wc.State('', { })
+
+ svntest.actions.run_and_verify_patch(wc_dir, os.path.abspath(patch_file_path),
+ expected_output, expected_disk,
+ expected_status, expected_skip,
+ check_props=True)
+
+def patch_deletes_executability(sbox):
+ """patch deletes svn:executable"""
+
+ sbox.build(read_only=True)
+ wc_dir = sbox.wc_dir
+
+ ## Set up the basic state.
+ sbox.simple_propset('svn:executable', 'yes', 'iota')
+ #sbox.simple_commit(target='iota', message="Make 'iota' executable.")
+
+ unidiff_patch = (
+ "diff --git a/iota b/iota\n"
+ "old mode 100755\n"
+ "new mode 100644\n"
+ )
+ patch_file_path = make_patch_path(sbox)
+ svntest.main.file_write(patch_file_path, unidiff_patch)
+
+ expected_output = [
+ ' U %s\n' % sbox.ospath('iota'),
+ ]
+ expected_disk = svntest.main.greek_state.copy()
+ expected_disk.tweak('iota') # props=None by default
+
+ expected_status = svntest.actions.get_virginal_state(wc_dir, 1)
+ expected_status.tweak('iota', status=' ')
+
+ expected_skip = wc.State('', { })
+
+ svntest.actions.run_and_verify_patch(wc_dir, os.path.abspath(patch_file_path),
+ expected_output, expected_disk,
+ expected_status, expected_skip,
+ check_props=True)
+
+def patch_ambiguous_executability_contradiction(sbox):
+ """patch ambiguous svn:executable, bad"""
+
+ sbox.build(read_only=True)
+ wc_dir = sbox.wc_dir
+
+ unidiff_patch = (
+ "Index: iota\n"
+ "===================================================================\n"
+ "diff --git a/iota b/iota\n"
+ "old mode 100755\n"
+ "new mode 100644\n"
+ "Property changes on: iota\n"
+ "-------------------------------------------------------------------\n"
+ "Added: svn:executable\n"
+ "## -0,0 +1 ##\n"
+ "+*\n"
+ )
+ patch_file_path = make_patch_path(sbox)
+ svntest.main.file_write(patch_file_path, unidiff_patch)
+
+ expected_output = []
+
+ expected_disk = svntest.main.greek_state.copy()
+
+ expected_status = svntest.actions.get_virginal_state(wc_dir, 1)
+
+ expected_skip = wc.State('', { })
+
+ error_re_string = r'.*Invalid patch:.*contradicting.*mode.*svn:executable'
+ svntest.actions.run_and_verify_patch(wc_dir, os.path.abspath(patch_file_path),
+ expected_output, expected_disk,
+ expected_status, expected_skip,
+ error_re_string=error_re_string,
+ check_props=True)
+
+def patch_ambiguous_executability_consistent(sbox):
+ """patch ambiguous svn:executable, good"""
+
+ sbox.build(read_only=True)
+ wc_dir = sbox.wc_dir
+
+ unidiff_patch = (
+ "Index: iota\n"
+ "===================================================================\n"
+ "diff --git a/iota b/iota\n"
+ "old mode 100644\n"
+ "new mode 100755\n"
+ "Property changes on: iota\n"
+ "-------------------------------------------------------------------\n"
+ "Added: svn:executable\n"
+ "## -0,0 +1 ##\n"
+ "+*\n"
+ )
+ patch_file_path = make_patch_path(sbox)
+ svntest.main.file_write(patch_file_path, unidiff_patch)
+
+ expected_output = [
+ ' U %s\n' % sbox.ospath('iota'),
+ ]
+
+ expected_disk = svntest.main.greek_state.copy()
+ expected_disk.tweak('iota', props={'svn:executable': '*'})
+
+ expected_status = svntest.actions.get_virginal_state(wc_dir, 1)
+ expected_status.tweak('iota', status=' M')
+
+ expected_skip = wc.State('', { })
+
+ svntest.actions.run_and_verify_patch(wc_dir, os.path.abspath(patch_file_path),
+ expected_output, expected_disk,
+ expected_status, expected_skip,
+ error_re_string=None,
+ check_props=True)
+
########################################################################
#Run the tests
@@ -6027,6 +6210,11 @@ test_list = [ None,
patch_delete_nodes,
patch_delete_missing_eol,
patch_final_eol,
+ patch_adds_executability_nocontents,
+ patch_adds_executability_yescontents,
+ patch_deletes_executability,
+ patch_ambiguous_executability_contradiction,
+ patch_ambiguous_executability_consistent,
]
if __name__ == '__main__':
Modified: subversion/trunk/subversion/tests/libsvn_diff/parse-diff-test.c
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/tests/libsvn_diff/parse-diff-test.c?rev=1705391&r1=1705390&r2=1705391&view=diff
==============================================================================
--- subversion/trunk/subversion/tests/libsvn_diff/parse-diff-test.c (original)
+++ subversion/trunk/subversion/tests/libsvn_diff/parse-diff-test.c Sat Sep 26 00:46:52 2015
@@ -66,6 +66,8 @@ static const char *git_unidiff =
"Index: A/C/gamma" NL
"===================================================================" NL
"diff --git a/A/C/gamma b/A/C/gamma" NL
+ "old mode 100644" NL
+ "new mode 100755" NL
"--- a/A/C/gamma\t(revision 2)" NL
"+++ b/A/C/gamma\t(working copy)" NL
"@@ -1 +1,2 @@" NL
@@ -86,6 +88,8 @@ static const char *git_tree_and_text_uni
"Index: iota.copied" NL
"===================================================================" NL
"diff --git a/iota b/iota.copied" NL
+ "old mode 100644" NL
+ "new mode 100755" NL
"copy from iota" NL
"copy to iota.copied" NL
"--- a/iota\t(revision 2)" NL
@@ -96,6 +100,8 @@ static const char *git_tree_and_text_uni
"Index: A/mu.moved" NL
"===================================================================" NL
"diff --git a/A/mu b/A/mu.moved" NL
+ "old mode 100644" NL
+ "new mode 100755" NL
"rename from A/mu" NL
"rename to A/mu.moved" NL
"--- a/A/mu\t(revision 2)" NL
@@ -114,7 +120,7 @@ static const char *git_tree_and_text_uni
"Index: A/B/lambda" NL
"===================================================================" NL
"diff --git a/A/B/lambda b/A/B/lambda" NL
- "deleted file mode 100644" NL
+ "deleted file mode 100755" NL
"--- a/A/B/lambda\t(revision 2)" NL
"+++ /dev/null\t(working copy)" NL
"@@ -1 +0,0 @@" NL
@@ -465,6 +471,8 @@ test_parse_git_diff(apr_pool_t *pool)
SVN_TEST_STRING_ASSERT(patch->new_filename, "A/C/gamma");
SVN_TEST_ASSERT(patch->operation == svn_diff_op_modified);
SVN_TEST_ASSERT(patch->hunks->nelts == 1);
+ SVN_TEST_ASSERT(patch->old_executable_p = svn_tristate_false);
+ SVN_TEST_ASSERT(patch->new_executable_p = svn_tristate_true);
hunk = APR_ARRAY_IDX(patch->hunks, 0, svn_diff_hunk_t *);
@@ -500,6 +508,8 @@ test_parse_git_diff(apr_pool_t *pool)
SVN_TEST_STRING_ASSERT(patch->new_filename, "new");
SVN_TEST_ASSERT(patch->operation == svn_diff_op_added);
SVN_TEST_ASSERT(patch->hunks->nelts == 0);
+ SVN_TEST_ASSERT(patch->old_executable_p = svn_tristate_unknown);
+ SVN_TEST_ASSERT(patch->new_executable_p = svn_tristate_false);
SVN_ERR(svn_diff_close_patch_file(patch_file, pool));
@@ -525,6 +535,8 @@ test_parse_git_tree_and_text_diff(apr_po
SVN_TEST_ASSERT(patch);
SVN_TEST_STRING_ASSERT(patch->old_filename, "iota");
SVN_TEST_STRING_ASSERT(patch->new_filename, "iota.copied");
+ SVN_TEST_ASSERT(patch->old_executable_p = svn_tristate_false);
+ SVN_TEST_ASSERT(patch->new_executable_p = svn_tristate_true);
SVN_TEST_ASSERT(patch->operation == svn_diff_op_copied);
SVN_TEST_ASSERT(patch->hunks->nelts == 1);
@@ -547,6 +559,8 @@ test_parse_git_tree_and_text_diff(apr_po
SVN_TEST_ASSERT(patch);
SVN_TEST_STRING_ASSERT(patch->old_filename, "A/mu");
SVN_TEST_STRING_ASSERT(patch->new_filename, "A/mu.moved");
+ SVN_TEST_ASSERT(patch->old_executable_p = svn_tristate_false);
+ SVN_TEST_ASSERT(patch->new_executable_p = svn_tristate_true);
SVN_TEST_ASSERT(patch->operation == svn_diff_op_moved);
SVN_TEST_ASSERT(patch->hunks->nelts == 1);
@@ -590,6 +604,8 @@ test_parse_git_tree_and_text_diff(apr_po
SVN_TEST_STRING_ASSERT(patch->new_filename, "/dev/null");
SVN_TEST_ASSERT(patch->operation == svn_diff_op_deleted);
SVN_TEST_ASSERT(patch->hunks->nelts == 1);
+ SVN_TEST_ASSERT(patch->old_executable_p = svn_tristate_true);
+ SVN_TEST_ASSERT(patch->new_executable_p = svn_tristate_unknown);
hunk = APR_ARRAY_IDX(patch->hunks, 0, svn_diff_hunk_t *);
Re: svn commit: r1705391 - in /subversion/trunk: ./
subversion/include/ subversion/include/private/ subversion/libsvn_client/
subversion/libsvn_diff/ subversion/libsvn_fs_x/ subversion/tests/cmdline/
subversion/tests/libsvn_diff/
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Bert Huijben wrote on Sat, Sep 26, 2015 at 10:06:15 +0200:
> A few comments inline.
>
Thanks for the review.
> > + /* ### This is currently not parsed out of "index" lines (where it
> > + * ### serves as an assertion of the executability state, without
> > + * ### changing it). */
> > + svn_tristate_t old_executable_p;
> > + svn_tristate_t new_executable_p;
> > } svn_patch_t;
>
> I'm not sure if we should leave this comment here in the long term...
*shrug* We could put it elsewhere.
> > +/* Return whether the svn:executable proppatch and the out-of-band
> > + * executability metadata contradict each other, assuming both are present.
> > + */
> > +static svn_boolean_t
> > +contradictory_executability(const svn_patch_t *patch,
> > + const prop_patch_target_t *target)
> > +{
> > + switch (target->operation)
> > + {
> > + case svn_diff_op_added:
> > + return patch->new_executable_p == svn_tristate_false;
> > +
> > + case svn_diff_op_deleted:
> > + return patch->new_executable_p == svn_tristate_true;
> > +
> > + case svn_diff_op_unchanged:
> > + /* ### Can this happen? */
> > + return (patch->old_executable_p != svn_tristate_unknown
> > + && patch->new_executable_p != svn_tristate_unknown
> > + && patch->old_executable_p != patch->new_executable_p);
> > +
> > + case svn_diff_op_modified:
> > + /* Can't happen: the property should only ever be added or deleted,
> > + * but never modified from one valid value to another. */
> > + return (patch->old_executable_p != svn_tristate_unknown
> > + && patch->new_executable_p != svn_tristate_unknown
> > + && patch->old_executable_p == patch->new_executable_p);
>
> These can happen when the svn:executable property just changes value (E.g. from '*' to 'x'). That should not happen, but it can.
Right. So _if_ somebody had broken the "value is SVN_PROP_BOOLEAN_VALUE"
invariant, _and_ generated a diff that contains a 'old mode 0755\n new
mode 0755\n' sequence — which, although syntactically valid, wouldn't
be output by any diff program — then they will get a false positive
"Contradictory executability" hard error.
The place to handle broken invariants is at the entry point, which for
patches is parse-diff.c. Would make sense, then, for parse-diff.c to
normalize the 'old value' and 'new value' of svn_prop_is_boolean()
properties to SVN_PROP_BOOLEAN_VALUE, and for patch.c to canonicalize
the ACTUAL/WORKING property value to SVN_PROP_BOOLEAN_VALUE before
trying to apply the patch? (Example: if the property value in the
wc is "yes", and the patch says 'change the property value from "sí" to
unset', parse-diff.c will fold "yes" to "*", patch.c will fold "sí"
to "*", and the patch will apply successfully.)
> > +add_or_delete_single_line(svn_diff_hunk_t **hunk_out,
> > + const char *line,
> > + svn_patch_t *patch,
> > + svn_boolean_t add,
> > + apr_pool_t *result_pool,
> > + apr_pool_t *scratch_pool)
> > +{
> > + svn_diff_hunk_t *hunk = apr_palloc(result_pool, sizeof(*hunk));
>
> As pcalloc() would be safer here...
...
> > + hunk->leading_context = 0;
> > + hunk->trailing_context = 0;
>
> As this patch forgets to set the new booleans added on trunk to signal no EOL markers here.
>
> You might need to set one of them to true though, as I think you are trying to add a property with no end of line.
It's just a merge artifact. I'd rather just initialize the newly-added
members and leave the non-initializing allocation so cc/valgrind can
catch bugs.
I'll initialize them to FALSE for now, to plug the uninitialized memory
access. I'm not sure we need to change that to TRUE; see discussion
under the patch_tests.py hunks, below.
> > +/* Parse the 'old mode ' line of a git extended unidiff. */
> > +static svn_error_t *
> > +git_old_mode(enum parse_state *new_state, char *line, svn_patch_t
> > *patch,
> > + apr_pool_t *result_pool, apr_pool_t *scratch_pool)
> > +{
> > + SVN_ERR(parse_bits_into_executability(&patch->old_executable_p,
> > + line + STRLEN_LITERAL("old mode ")));
> > +
> > +#ifdef SVN_DEBUG
> > + /* If this assert trips, the "old mode" is neither ...644 nor ...755 . */
> > + SVN_ERR_ASSERT(patch->old_executable_p != svn_tristate_unknown);
> > +#endif
>
> I don't think we should leave these enabled even in maintainer mode.
> This will break abilities to apply diffs generated from other tools.
The purpose of the warning is to alert us that there is a syntax we
aren't handling. That's why it's for maintainers only. The assumption
is that maintainers can switch to a release build if they run into this
warning unexpectedly.
I don't think we can convert this to a warning, can we? This API
doesn't have a way to report warnings to its caller.
> (Perhaps even from older Subversion versions as I think you recently
> changed what Subversion generates itself on trunk)
>
You mean r1695384?
Personally, I think rejecting pre-r1695384 patches is _good_, since
thoes patches were simply invalid and would have gotten in the way of
assigning a meaning to the 010000 bit in the future. If we ever find
out that somebody is generating such patches, we can then relax our
parser to accept them.
> > +def patch_ambiguous_executability_contradiction(sbox):
> > + """patch ambiguous svn:executable, bad"""
> > +
> > + sbox.build(read_only=True)
> > + wc_dir = sbox.wc_dir
> > +
> > + unidiff_patch = (
> > + "Index: iota\n"
> > +
> > "===============================================================
> > ====\n"
> > + "diff --git a/iota b/iota\n"
> > + "old mode 100755\n"
> > + "new mode 100644\n"
> > + "Property changes on: iota\n"
> > + "-------------------------------------------------------------------\n"
> > + "Added: svn:executable\n"
> > + "## -0,0 +1 ##\n"
> > + "+*\n"
> > + )
>
> This specifies the addition of the svn:executable property with the non canonical "*\n" value. Is this really what you want to test?
> I think the actual property set code will change it to the proper value, but I would say you need the " \ No newline at end of property" marker here.
>
> The generated patchfile doesn't need it, but it does need the boolean to note the same thing.
>
I don't understand. If the generated patchfile doesn't need the \ line,
then that is a great reason not to have it here, so that we test parsing
the output svn currently generates.
In any case, patch_dir_properties() and patch_add_symlink() are written
without a \ line. I'll make one of them use a \ line so both variants
are tested.
Cheers,
Daniel
RE: svn commit: r1705391 - in /subversion/trunk: ./ subversion/include/ subversion/include/private/ subversion/libsvn_client/ subversion/libsvn_diff/ subversion/libsvn_fs_x/ subversion/tests/cmdline/ subversion/tests/libsvn_diff/
Posted by Bert Huijben <be...@qqmail.nl>.
> -----Original Message-----
> From: Bert Huijben [mailto:bert@qqmail.nl]
> Sent: zaterdag 26 september 2015 10:06
> To: dev@subversion.apache.org; commits@subversion.apache.org
> Subject: RE: svn commit: r1705391 - in /subversion/trunk: ./
> subversion/include/ subversion/include/private/ subversion/libsvn_client/
> subversion/libsvn_diff/ subversion/libsvn_fs_x/ subversion/tests/cmdline/
> subversion/tests/libsvn_diff/
>
> A few comments inline.
> "===============================================================
> > ====\n"
> > + "diff --git a/iota b/iota\n"
> > + "old mode 100755\n"
> > + "new mode 100644\n"
> > + "Property changes on: iota\n"
> > + "-------------------------------------------------------------------\n"
> > + "Added: svn:executable\n"
> > + "## -0,0 +1 ##\n"
> > + "+*\n"
> > + )
>
> This specifies the addition of the svn:executable property with the non
> canonical "*\n" value. Is this really what you want to test?
> I think the actual property set code will change it to the proper value, but I
> would say you need the " \ No newline at end of property" marker here.
>
> The generated patchfile doesn't need it, but it does need the boolean to note
> the same thing.
The generated patch file might still need it to record a failed patch in the reject file.
In that case the hunk is copied there and it should have the 'has no eol' marker.
>
> Bert
Re: svn commit: r1705391 - in /subversion/trunk: ./
subversion/include/ subversion/include/private/ subversion/libsvn_client/
subversion/libsvn_diff/ subversion/libsvn_fs_x/ subversion/tests/cmdline/
subversion/tests/libsvn_diff/
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Bert Huijben wrote on Sat, Sep 26, 2015 at 10:06:15 +0200:
> A few comments inline.
>
Thanks for the review.
> > + /* ### This is currently not parsed out of "index" lines (where it
> > + * ### serves as an assertion of the executability state, without
> > + * ### changing it). */
> > + svn_tristate_t old_executable_p;
> > + svn_tristate_t new_executable_p;
> > } svn_patch_t;
>
> I'm not sure if we should leave this comment here in the long term...
*shrug* We could put it elsewhere.
> > +/* Return whether the svn:executable proppatch and the out-of-band
> > + * executability metadata contradict each other, assuming both are present.
> > + */
> > +static svn_boolean_t
> > +contradictory_executability(const svn_patch_t *patch,
> > + const prop_patch_target_t *target)
> > +{
> > + switch (target->operation)
> > + {
> > + case svn_diff_op_added:
> > + return patch->new_executable_p == svn_tristate_false;
> > +
> > + case svn_diff_op_deleted:
> > + return patch->new_executable_p == svn_tristate_true;
> > +
> > + case svn_diff_op_unchanged:
> > + /* ### Can this happen? */
> > + return (patch->old_executable_p != svn_tristate_unknown
> > + && patch->new_executable_p != svn_tristate_unknown
> > + && patch->old_executable_p != patch->new_executable_p);
> > +
> > + case svn_diff_op_modified:
> > + /* Can't happen: the property should only ever be added or deleted,
> > + * but never modified from one valid value to another. */
> > + return (patch->old_executable_p != svn_tristate_unknown
> > + && patch->new_executable_p != svn_tristate_unknown
> > + && patch->old_executable_p == patch->new_executable_p);
>
> These can happen when the svn:executable property just changes value (E.g. from '*' to 'x'). That should not happen, but it can.
Right. So _if_ somebody had broken the "value is SVN_PROP_BOOLEAN_VALUE"
invariant, _and_ generated a diff that contains a 'old mode 0755\n new
mode 0755\n' sequence — which, although syntactically valid, wouldn't
be output by any diff program — then they will get a false positive
"Contradictory executability" hard error.
The place to handle broken invariants is at the entry point, which for
patches is parse-diff.c. Would make sense, then, for parse-diff.c to
normalize the 'old value' and 'new value' of svn_prop_is_boolean()
properties to SVN_PROP_BOOLEAN_VALUE, and for patch.c to canonicalize
the ACTUAL/WORKING property value to SVN_PROP_BOOLEAN_VALUE before
trying to apply the patch? (Example: if the property value in the
wc is "yes", and the patch says 'change the property value from "sí" to
unset', parse-diff.c will fold "yes" to "*", patch.c will fold "sí"
to "*", and the patch will apply successfully.)
> > +add_or_delete_single_line(svn_diff_hunk_t **hunk_out,
> > + const char *line,
> > + svn_patch_t *patch,
> > + svn_boolean_t add,
> > + apr_pool_t *result_pool,
> > + apr_pool_t *scratch_pool)
> > +{
> > + svn_diff_hunk_t *hunk = apr_palloc(result_pool, sizeof(*hunk));
>
> As pcalloc() would be safer here...
...
> > + hunk->leading_context = 0;
> > + hunk->trailing_context = 0;
>
> As this patch forgets to set the new booleans added on trunk to signal no EOL markers here.
>
> You might need to set one of them to true though, as I think you are trying to add a property with no end of line.
It's just a merge artifact. I'd rather just initialize the newly-added
members and leave the non-initializing allocation so cc/valgrind can
catch bugs.
I'll initialize them to FALSE for now, to plug the uninitialized memory
access. I'm not sure we need to change that to TRUE; see discussion
under the patch_tests.py hunks, below.
> > +/* Parse the 'old mode ' line of a git extended unidiff. */
> > +static svn_error_t *
> > +git_old_mode(enum parse_state *new_state, char *line, svn_patch_t
> > *patch,
> > + apr_pool_t *result_pool, apr_pool_t *scratch_pool)
> > +{
> > + SVN_ERR(parse_bits_into_executability(&patch->old_executable_p,
> > + line + STRLEN_LITERAL("old mode ")));
> > +
> > +#ifdef SVN_DEBUG
> > + /* If this assert trips, the "old mode" is neither ...644 nor ...755 . */
> > + SVN_ERR_ASSERT(patch->old_executable_p != svn_tristate_unknown);
> > +#endif
>
> I don't think we should leave these enabled even in maintainer mode.
> This will break abilities to apply diffs generated from other tools.
The purpose of the warning is to alert us that there is a syntax we
aren't handling. That's why it's for maintainers only. The assumption
is that maintainers can switch to a release build if they run into this
warning unexpectedly.
I don't think we can convert this to a warning, can we? This API
doesn't have a way to report warnings to its caller.
> (Perhaps even from older Subversion versions as I think you recently
> changed what Subversion generates itself on trunk)
>
You mean r1695384?
Personally, I think rejecting pre-r1695384 patches is _good_, since
thoes patches were simply invalid and would have gotten in the way of
assigning a meaning to the 010000 bit in the future. If we ever find
out that somebody is generating such patches, we can then relax our
parser to accept them.
> > +def patch_ambiguous_executability_contradiction(sbox):
> > + """patch ambiguous svn:executable, bad"""
> > +
> > + sbox.build(read_only=True)
> > + wc_dir = sbox.wc_dir
> > +
> > + unidiff_patch = (
> > + "Index: iota\n"
> > +
> > "===============================================================
> > ====\n"
> > + "diff --git a/iota b/iota\n"
> > + "old mode 100755\n"
> > + "new mode 100644\n"
> > + "Property changes on: iota\n"
> > + "-------------------------------------------------------------------\n"
> > + "Added: svn:executable\n"
> > + "## -0,0 +1 ##\n"
> > + "+*\n"
> > + )
>
> This specifies the addition of the svn:executable property with the non canonical "*\n" value. Is this really what you want to test?
> I think the actual property set code will change it to the proper value, but I would say you need the " \ No newline at end of property" marker here.
>
> The generated patchfile doesn't need it, but it does need the boolean to note the same thing.
>
I don't understand. If the generated patchfile doesn't need the \ line,
then that is a great reason not to have it here, so that we test parsing
the output svn currently generates.
In any case, patch_dir_properties() and patch_add_symlink() are written
without a \ line. I'll make one of them use a \ line so both variants
are tested.
Cheers,
Daniel
RE: svn commit: r1705391 - in /subversion/trunk: ./ subversion/include/ subversion/include/private/ subversion/libsvn_client/ subversion/libsvn_diff/ subversion/libsvn_fs_x/ subversion/tests/cmdline/ subversion/tests/libsvn_diff/
Posted by Bert Huijben <be...@qqmail.nl>.
A few comments inline.
> -----Oorspronkelijk bericht-----
> Van: danielsh@apache.org [mailto:danielsh@apache.org]
> Verzonden: zaterdag 26 september 2015 02:47
> Aan: commits@subversion.apache.org
> Onderwerp: svn commit: r1705391 - in /subversion/trunk: ./
> subversion/include/ subversion/include/private/ subversion/libsvn_client/
> subversion/libsvn_diff/ subversion/libsvn_fs_x/ subversion/tests/cmdline/
> subversion/tests/libsvn_diff/
>
> Author: danielsh
> Date: Sat Sep 26 00:46:52 2015
> New Revision: 1705391
>
> URL: http://svn.apache.org/viewvc?rev=1705391&view=rev
> Log:
> patch: Parse 'old mode' / 'new mode' line from git diffs and translate them to
> svn:executable propchanges.
>
> This merges the 'patch-exec' branch into trunk.
>
> ---
>
> The following are the major changes. See the branch for detailed changes.
>
> * subversion/include/svn_diff.h
> (svn_patch_t.old_executable_p, svn_patch_t.new_executable_p): New
> members.
>
> * subversion/libsvn_client/patch.c
> (contradictory_executability): New function.
> (init_patch_target, apply_one_patch):
> Translate mode changes to svn:executable changes.
>
> * subversion/include/private/svn_diff_private.h
> (svn_diff_hunk__create_adds_single_line,
> svn_diff_hunk__create_deletes_single_line): New functions.
>
> * subversion/libsvn_diff/parse-diff.c
> (add_or_delete_single_line,
> svn_diff_hunk__create_adds_single_line,
> svn_diff_hunk__create_deletes_single_line): New functions.
> (parse_state::state_old_mode_seen,
> parse_state::state_git_mode_seen): New enumerators.
> (parse_bits_into_executability, git_old_mode, git_new_mode): New
> functions.
> (git_deleted_file, git_new_file): Parse file mode.
> (transitions): Parse "old mode" and "new mode" lines.
>
> * subversion/tests/cmdline/patch_tests.py,
> * subversion/tests/libsvn_diff/parse-diff-test.c:
> Add unit tests.
>
> Modified:
> subversion/trunk/ (props changed)
> subversion/trunk/subversion/include/private/svn_diff_private.h
> subversion/trunk/subversion/include/svn_diff.h
> subversion/trunk/subversion/libsvn_client/patch.c
> subversion/trunk/subversion/libsvn_diff/parse-diff.c
> subversion/trunk/subversion/libsvn_fs_x/ (props changed)
> subversion/trunk/subversion/tests/cmdline/patch_tests.py
> subversion/trunk/subversion/tests/libsvn_diff/parse-diff-test.c
>
> Propchange: subversion/trunk/
> ------------------------------------------------------------------------------
> --- svn:mergeinfo (original)
> +++ svn:mergeinfo Sat Sep 26 00:46:52 2015
> @@ -62,6 +62,7 @@
> /subversion/branches/multi-layer-moves:1239019-1300930
> /subversion/branches/nfc-nfd-aware-client:870276,870376
> /subversion/branches/node_pool:1304828-1305388
> +/subversion/branches/patch-exec:1692717-1705390
>
> /subversion/branches/performance:979193,980118,981087,981090,981189,
> 981194,981287,981684,981827,982043,982355,983398,983406,983430,98347
> 4,983488,983490,983760,983764,983766,983770,984927,984973,984984,985
> 014,985037,985046,985472,985477,985482,985487-
> 985488,985493,985497,985500,985514,985601,985603,985606,985669,98567
> 3,985695,985697,986453,986465,986485,986491-
> 986492,986517,986521,986605,986608,986817,986832,987865,987868-
> 987869,987872,987886-
> 987888,987893,988319,988898,990330,990533,990535-
> 990537,990541,990568,990572,990574-
> 990575,990600,990759,992899,992904,992911,993127,993141,994956,99547
> 8,995507,995603,998012,998858,999098,1001413,1001417,1004291,1022668
> ,1022670,1022676,1022715,1022719,1025660,1025672,1027193,1027203,102
> 7206,1027214,1027227,1028077,1028092,1028094,1028104,1028107,102811
> 1,1028354,1029038,1029042-1029043,1029054-1029055,1029062-
> 1029063,1029078,1029080,1029090,1029092-
> 1029093,1029111,1029151,1029158,1029229-1029230,1029232,1029335-
> 1029336,1029339-1029340,1029342,10
>
> 29344,1030763,1030827,1031203,1031235,1032285,1032333,1033040,10330
> 57,1033294,1035869,1035882,1039511,1043705,1053735,1056015,1066452,1
> 067683,1067697-1078365
> /subversion/branches/pin-externals:1643757-1659392
> /subversion/branches/py-tests-as-modules:956579-1033052
>
<snip>
> Modified: subversion/trunk/subversion/include/svn_diff.h
> URL:
> http://svn.apache.org/viewvc/subversion/trunk/subversion/include/svn_diff
> .h?rev=1705391&r1=1705390&r2=1705391&view=diff
> ================================================================
> ==============
> --- subversion/trunk/subversion/include/svn_diff.h (original)
> +++ subversion/trunk/subversion/include/svn_diff.h Sat Sep 26 00:46:52
> +++ 2015
> @@ -1288,6 +1288,24 @@ typedef struct svn_patch_t {
> * to apply it as parsed from the file.
> * @since New in 1.10. */
> svn_diff_binary_patch_t *binary_patch;
> +
> + /** The old and new executability bits, as retrieved from the patch file.
> + *
> + * A patch may specify an executability change via @a old_executable_p
> and
> + * / @a new_executable_p, via a #SVN_PROP_EXECUTABLE propchange
> hunk, or both
> + * ways; however, if both ways are used, they must specify the same
> semantic
> + * change.
> + *
> + * #svn_tristate_unknown indicates the patch does not specify the
> + * corresponding bit.
> + *
> + * @since New in 1.10.
> + */
> + /* ### This is currently not parsed out of "index" lines (where it
> + * ### serves as an assertion of the executability state, without
> + * ### changing it). */
> + svn_tristate_t old_executable_p;
> + svn_tristate_t new_executable_p;
> } svn_patch_t;
I'm not sure if we should leave this comment here in the long term...
>
> /** An opaque type representing an open patch file.
>
> Modified: subversion/trunk/subversion/libsvn_client/patch.c
> URL:
> http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_client/pa
> tch.c?rev=1705391&r1=1705390&r2=1705391&view=diff
> ================================================================
> ==============
> --- subversion/trunk/subversion/libsvn_client/patch.c (original)
> +++ subversion/trunk/subversion/libsvn_client/patch.c Sat Sep 26 00:46:52
> 2015
> @@ -46,6 +46,7 @@
> #include "private/svn_eol_private.h"
> #include "private/svn_wc_private.h"
> #include "private/svn_dep_compat.h"
> +#include "private/svn_diff_private.h"
> #include "private/svn_string_private.h"
> #include "private/svn_subr_private.h"
> #include "private/svn_sorts_private.h"
> @@ -914,6 +915,41 @@ choose_target_filename(const svn_patch_t
> return (old < new) ? patch->old_filename : patch->new_filename;
> }
>
> +/* Return whether the svn:executable proppatch and the out-of-band
> + * executability metadata contradict each other, assuming both are present.
> + */
> +static svn_boolean_t
> +contradictory_executability(const svn_patch_t *patch,
> + const prop_patch_target_t *target)
> +{
> + switch (target->operation)
> + {
> + case svn_diff_op_added:
> + return patch->new_executable_p == svn_tristate_false;
> +
> + case svn_diff_op_deleted:
> + return patch->new_executable_p == svn_tristate_true;
> +
> + case svn_diff_op_unchanged:
> + /* ### Can this happen? */
> + return (patch->old_executable_p != svn_tristate_unknown
> + && patch->new_executable_p != svn_tristate_unknown
> + && patch->old_executable_p != patch->new_executable_p);
> +
> + case svn_diff_op_modified:
> + /* Can't happen: the property should only ever be added or deleted,
> + * but never modified from one valid value to another. */
> + return (patch->old_executable_p != svn_tristate_unknown
> + && patch->new_executable_p != svn_tristate_unknown
> + && patch->old_executable_p == patch->new_executable_p);
These can happen when the svn:executable property just changes value (E.g. from '*' to 'x'). That should not happen, but it can.
> +
> + default:
> + /* Can't happen: the proppatch parser never generates other values. */
> + SVN_ERR_MALFUNCTION_NO_RETURN();
> + }
> +}
> +
> +
> /* Attempt to initialize a *PATCH_TARGET structure for a target file
> * described by PATCH. Use working copy context WC_CTX.
> * STRIP_COUNT specifies the number of leading path components
> @@ -956,6 +992,9 @@ init_patch_target(patch_target_t **patch
> target->kind_on_disk = svn_node_none;
> target->content = content;
> target->prop_targets = apr_hash_make(result_pool);
> + if (patch->new_executable_p != svn_tristate_unknown)
> + /* May also be set by apply_hunk(). */
> + target->has_prop_changes = TRUE;
>
> SVN_ERR(resolve_target_path(target, choose_target_filename(patch),
> root_abspath, strip_count, has_text_changes,
> @@ -1129,6 +1168,7 @@ init_patch_target(patch_target_t **patch
> if (! target->skipped)
> {
> apr_hash_index_t *hi;
> + prop_patch_target_t *prop_executable_target;
>
> for (hi = apr_hash_first(result_pool, patch->prop_patches);
> hi;
> @@ -1145,6 +1185,99 @@ init_patch_target(patch_target_t **patch
> result_pool, scratch_pool));
> svn_hash_sets(target->prop_targets, prop_name, prop_target);
> }
> +
> + /* Now, check for an out-of-band mode change and convert it to
> + * an svn:executable property patch. */
> + prop_executable_target = svn_hash_gets(target->prop_targets,
> + SVN_PROP_EXECUTABLE);
> + if (patch->new_executable_p != svn_tristate_unknown
> + && prop_executable_target)
> + {
> + if (contradictory_executability(patch, prop_executable_target))
> + /* Invalid input: specifies both git-like "new mode" lines and
> + * svn-like addition/removal of svn:executable.
> + *
> + * If this were merely a hunk that didn't apply, we'd reject it
> + * and move on. However, this is a self-contradictory hunk;
> + * it has no unambiguous interpretation. Therefore: */
> + return svn_error_createf(SVN_ERR_INVALID_INPUT, NULL,
> + _("Invalid patch: specifies "
> + "contradicting mode changes and "
> + "%s changes (for '%s')"),
> + SVN_PROP_EXECUTABLE,
> + target->local_abspath);
> + else
> + /* We have two representations of the same change.
> + *
> + * In the caller, there will be two hunk_info_t's for the
> + * patch: one generated from the property diff and one
> + * generated from the out-of-band mode change. Both hunks will
> + * be processed, but the output will be as though there was
> + * just one hunk.
> + *
> + * The reason there will be only a single notification is not
> + * specific to SVN_PROP_EXECUTABLE but generic to all property
> + * patches: if a patch file contains two identical property
> + * hunks (e.g.,
> + * svn ps k v iota; svn diff iota >patch; svn diff iota >>patch
> + * ), applying the patch file will only produce a single ' U'
> + * notification.
> + *
> + * In contrast, the same for file-content hunks will result in
> + * a 'U' notification followed by a 'G' notification. (The 'U'
> + * for the first copy of the hunk; the 'G' for the second.)
> + */
> + ;
> + }
> + else if (patch->new_executable_p != svn_tristate_unknown
> + && !prop_executable_target)
> + {
> + svn_diff_operation_kind_t operation;
> + svn_boolean_t nothing_to_do = FALSE;
> + prop_patch_target_t *prop_target;
> +
> + if (patch->old_executable_p == patch->new_executable_p)
> + {
> + /* Noop change. */
> + operation = svn_diff_op_unchanged;
> + }
> + else switch (patch->old_executable_p)
> + {
> + case svn_tristate_false:
> + /* Made executable. */
> + operation = svn_diff_op_added;
> + break;
> +
> + case svn_tristate_true:
> + /* Made non-executable. */
> + operation = svn_diff_op_deleted;
> + break;
> +
> + case svn_tristate_unknown:
> + if (patch->new_executable_p == svn_tristate_true)
> + /* New, executable file. */
> + operation = svn_diff_op_added;
> + else
> + /* New, non-executable file. That's not a change. */
> + nothing_to_do = TRUE;
> + break;
> +
> + default:
> + /* NOTREACHED */
> + SVN_ERR_MALFUNCTION();
> + }
> +
> + if (! nothing_to_do)
> + {
> + SVN_ERR(init_prop_target(&prop_target,
> + SVN_PROP_EXECUTABLE,
> + operation,
> + wc_ctx, target->local_abspath,
> + result_pool, scratch_pool));
> + svn_hash_sets(target->prop_targets, SVN_PROP_EXECUTABLE,
> + prop_target);
> + }
> + }
> }
> }
>
> @@ -2425,6 +2558,50 @@ apply_one_patch(patch_target_t **patch_t
> }
> }
>
> + /* Match implied property hunks. */
> + if (patch->new_executable_p != svn_tristate_unknown
> + && svn_hash_gets(target->prop_targets, SVN_PROP_EXECUTABLE))
> + {
> + hunk_info_t *hi;
> + svn_diff_hunk_t *hunk;
> + prop_patch_target_t *prop_target = svn_hash_gets(target-
> >prop_targets,
> + SVN_PROP_EXECUTABLE);
> + const char *const value = SVN_PROP_EXECUTABLE_VALUE;
> +
> + switch (prop_target->operation)
> + {
> + case svn_diff_op_added:
> + SVN_ERR(svn_diff_hunk__create_adds_single_line(&hunk, value,
> patch,
> + result_pool,
> + iterpool));
> + break;
> +
> + case svn_diff_op_deleted:
> + SVN_ERR(svn_diff_hunk__create_deletes_single_line(&hunk, value,
> + patch,
> + result_pool,
> + iterpool));
> + break;
> +
> + case svn_diff_op_unchanged:
> + /* ### What to do? */
> + break;
> +
> + default:
> + SVN_ERR_MALFUNCTION();
> + }
> +
> + /* Derive a hunk_info from hunk. */
> + SVN_ERR(get_hunk_info(&hi, target, prop_target->content,
> + hunk, 0 /* fuzz */, 0 /* previous_offset */,
> + ignore_whitespace,
> + TRUE /* is_prop_hunk */,
> + cancel_func, cancel_baton,
> + result_pool, iterpool));
> + if (! hi->already_applied)
> + APR_ARRAY_PUSH(prop_target->content->hunks, hunk_info_t *) = hi;
> + }
> +
> /* Apply or reject property hunks. */
> for (hash_index = apr_hash_first(scratch_pool, target->prop_targets);
> hash_index;
>
> Modified: subversion/trunk/subversion/libsvn_diff/parse-diff.c
> URL:
> http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_diff/pars
> e-diff.c?rev=1705391&r1=1705390&r2=1705391&view=diff
> ================================================================
> ==============
> --- subversion/trunk/subversion/libsvn_diff/parse-diff.c (original)
> +++ subversion/trunk/subversion/libsvn_diff/parse-diff.c Sat Sep 26 00:46:52
> 2015
> @@ -40,6 +40,7 @@
>
> #include "private/svn_eol_private.h"
> #include "private/svn_dep_compat.h"
> +#include "private/svn_diff_private.h"
> #include "private/svn_sorts_private.h"
>
> #include "diff.h"
> @@ -108,6 +109,116 @@ struct svn_diff_binary_patch_t {
> svn_filesize_t dst_filesize; /* Expanded/final size */
> };
>
> +/* Common guts of svn_diff_hunk__create_adds_single_line() and
> + * svn_diff_hunk__create_deletes_single_line().
> + *
> + * ADD is TRUE if adding and FALSE if deleting.
> + */
> +static svn_error_t *
> +add_or_delete_single_line(svn_diff_hunk_t **hunk_out,
> + const char *line,
> + svn_patch_t *patch,
> + svn_boolean_t add,
> + apr_pool_t *result_pool,
> + apr_pool_t *scratch_pool)
> +{
> + svn_diff_hunk_t *hunk = apr_palloc(result_pool, sizeof(*hunk));
As pcalloc() would be safer here...
> + static const char *hunk_header[] = { "@@ -1 +0,0 @@\n", "@@ -0,0 +1
> @@\n" };
> + const unsigned len = strlen(line);
> + /* The +1 is for the 'plus' start-of-line character. */
> + const apr_off_t end = STRLEN_LITERAL(hunk_header[add]) + (1 + len);
> + /* The +1 is for the second \n. */
> + svn_stringbuf_t *buf = svn_stringbuf_create_ensure(end + 1,
> scratch_pool);
> +
> + hunk->patch = patch;
> +
> + /* hunk->apr_file is created below. */
> +
> + hunk->diff_text_range.start = STRLEN_LITERAL(hunk_header[add]);
> + hunk->diff_text_range.current = STRLEN_LITERAL(hunk_header[add]);
> + hunk->diff_text_range.end = end;
> +
> + if (add)
> + {
> + hunk->original_text_range.start = 0; /* There's no "original" text. */
> + hunk->original_text_range.current = 0;
> + hunk->original_text_range.end = 0;
> +
> + hunk->modified_text_range.start = STRLEN_LITERAL(hunk_header[add]);
> + hunk->modified_text_range.current =
> STRLEN_LITERAL(hunk_header[add]);
> + hunk->modified_text_range.end = end;
> +
> + hunk->original_start = 0;
> + hunk->original_length = 0;
> +
> + hunk->modified_start = 1;
> + hunk->modified_length = 1;
> + }
> + else /* delete */
> + {
> + hunk->original_text_range.start = STRLEN_LITERAL(hunk_header[add]);
> + hunk->original_text_range.current =
> STRLEN_LITERAL(hunk_header[add]);
> + hunk->original_text_range.end = end;
> +
> + hunk->modified_text_range.start = 0; /* There's no "original" text. */
> + hunk->modified_text_range.current = 0;
> + hunk->modified_text_range.end = 0;
> +
> + hunk->original_start = 1;
> + hunk->original_length = 1;
> +
> + hunk->modified_start = 0;
> + hunk->modified_length = 0; /* setting to '1' works too */
> + }
> +
> + hunk->leading_context = 0;
> + hunk->trailing_context = 0;
As this patch forgets to set the new booleans added on trunk to signal no EOL markers here.
You might need to set one of them to true though, as I think you are trying to add a property with no end of line.
> +
> + /* Create APR_FILE and put just a hunk in it (without a diff header).
> + * Save the offset of the last byte of the diff line. */
> + svn_stringbuf_appendbytes(buf, hunk_header[add],
> + STRLEN_LITERAL(hunk_header[add]));
> + svn_stringbuf_appendbyte(buf, add ? '+' : '-');
> + svn_stringbuf_appendbytes(buf, line, len);
> + svn_stringbuf_appendbyte(buf, '\n');
> +
> + SVN_ERR(svn_io_open_unique_file3(&hunk->apr_file, NULL /* filename
> */,
> + NULL /* system tempdir */,
> + svn_io_file_del_on_pool_cleanup,
> + result_pool, scratch_pool));
> + SVN_ERR(svn_io_file_write_full(hunk->apr_file,
> + buf->data, buf->len,
> + NULL, scratch_pool));
> + /* No need to seek. */
> +
> + *hunk_out = hunk;
> + return SVN_NO_ERROR;
> +}
> +
> +svn_error_t *
> +svn_diff_hunk__create_adds_single_line(svn_diff_hunk_t **hunk_out,
> + const char *line,
> + svn_patch_t *patch,
> + apr_pool_t *result_pool,
> + apr_pool_t *scratch_pool)
> +{
> + SVN_ERR(add_or_delete_single_line(hunk_out, line, patch, TRUE,
> + result_pool, scratch_pool));
> + return SVN_NO_ERROR;
> +}
> +
> +svn_error_t *
> +svn_diff_hunk__create_deletes_single_line(svn_diff_hunk_t **hunk_out,
> + const char *line,
> + svn_patch_t *patch,
> + apr_pool_t *result_pool,
> + apr_pool_t *scratch_pool)
> +{
> + SVN_ERR(add_or_delete_single_line(hunk_out, line, patch, FALSE,
> + result_pool, scratch_pool));
> + return SVN_NO_ERROR;
> +}
> +
> void
> svn_diff_hunk_reset_diff_text(svn_diff_hunk_t *hunk)
> {
> @@ -1240,6 +1351,8 @@ enum parse_state
> state_git_tree_seen, /* a tree operation, rather than content change */
> state_git_minus_seen, /* --- /dev/null; or --- a/ */
> state_git_plus_seen, /* +++ /dev/null; or +++ a/ */
> + state_old_mode_seen, /* old mode 100644 */
> + state_git_mode_seen, /* new mode 100644 */
> state_move_from_seen, /* rename from foo.c */
> state_copy_from_seen, /* copy from foo.c */
> state_minus_seen, /* --- foo.c */
> @@ -1472,6 +1585,85 @@ git_plus(enum parse_state *new_state, ch
> return SVN_NO_ERROR;
> }
>
> +/* Helper for git_old_mode() and git_new_mode(). Translate the git
> + * file mode MODE_STR into a binary "executable?" notion EXECUTABLE_P.
> */
> +static svn_error_t *
> +parse_bits_into_executability(svn_tristate_t *executable_p,
> + const char *mode_str)
> +{
> + apr_uint64_t mode;
> + SVN_ERR(svn_cstring_strtoui64(&mode, mode_str,
> + 0 /* min */,
> + 0777777 /* max: six octal digits */,
> + 010 /* radix (octal) */));
> +
> + /* Note: 0644 and 0755 are the only modes that can occur for plain files.
> + * We deliberately choose to parse only those values: we are strict in what
> + * we accept _and_ in what we produce.
> + *
> + * (Having said that, though, we could consider relaxing the parser to also
> + * map
> + * (mode & 0111) == 0000 -> svn_tristate_false
> + * (mode & 0111) == 0111 -> svn_tristate_true
> + * [anything else] -> svn_tristate_unknown
> + * .)
> + */
> +
> + switch (mode & 0777)
> + {
> + case 0644:
> + *executable_p = svn_tristate_false;
> + break;
> +
> + case 0755:
> + *executable_p = svn_tristate_true;
> + break;
> +
> + default:
> + /* Ignore unknown values. */
> + *executable_p = svn_tristate_unknown;
> + break;
> + }
> +
> + return SVN_NO_ERROR;
> +}
> +
> +/* Parse the 'old mode ' line of a git extended unidiff. */
> +static svn_error_t *
> +git_old_mode(enum parse_state *new_state, char *line, svn_patch_t
> *patch,
> + apr_pool_t *result_pool, apr_pool_t *scratch_pool)
> +{
> + SVN_ERR(parse_bits_into_executability(&patch->old_executable_p,
> + line + STRLEN_LITERAL("old mode ")));
> +
> +#ifdef SVN_DEBUG
> + /* If this assert trips, the "old mode" is neither ...644 nor ...755 . */
> + SVN_ERR_ASSERT(patch->old_executable_p != svn_tristate_unknown);
> +#endif
I don't think we should leave these enabled even in maintainer mode. This will break abilities to apply diffs generated from other tools. (Perhaps even from older Subversion versions as I think you recently changed what Subversion generates itself on trunk)
> +
> + *new_state = state_old_mode_seen;
> + return SVN_NO_ERROR;
> +}
> +
> +/* Parse the 'new mode ' line of a git extended unidiff. */
> +static svn_error_t *
> +git_new_mode(enum parse_state *new_state, char *line, svn_patch_t
> *patch,
> + apr_pool_t *result_pool, apr_pool_t *scratch_pool)
> +{
> + SVN_ERR(parse_bits_into_executability(&patch->new_executable_p,
> + line + STRLEN_LITERAL("new mode ")));
> +
> +#ifdef SVN_DEBUG
> + /* If this assert trips, the "old mode" is neither ...644 nor ...755 . */
> + SVN_ERR_ASSERT(patch->new_executable_p != svn_tristate_unknown);
> +#endif
Same here.
> +
> + /* Don't touch patch->operation. */
> +
> + *new_state = state_git_mode_seen;
> + return SVN_NO_ERROR;
> +}
> +
> /* Parse the 'rename from ' line of a git extended unidiff. */
> static svn_error_t *
> git_move_from(enum parse_state *new_state, char *line, svn_patch_t
> *patch,
> @@ -1532,6 +1724,10 @@ static svn_error_t *
> git_new_file(enum parse_state *new_state, char *line, svn_patch_t *patch,
> apr_pool_t *result_pool, apr_pool_t *scratch_pool)
> {
> + SVN_ERR(
> + parse_bits_into_executability(&patch->new_executable_p,
> + line + STRLEN_LITERAL("new file mode ")));
> +
> patch->operation = svn_diff_op_added;
>
> /* Filename already retrieved from diff --git header. */
> @@ -1545,6 +1741,10 @@ static svn_error_t *
> git_deleted_file(enum parse_state *new_state, char *line, svn_patch_t
> *patch,
> apr_pool_t *result_pool, apr_pool_t *scratch_pool)
> {
> + SVN_ERR(
> + parse_bits_into_executability(&patch->old_executable_p,
> + line + STRLEN_LITERAL("deleted file mode ")));
> +
> patch->operation = svn_diff_op_deleted;
>
> /* Filename already retrieved from diff --git header. */
> @@ -1808,15 +2008,22 @@ static struct transition transitions[] =
>
> {"diff --git", state_start, git_start},
> {"--- a/", state_git_diff_seen, git_minus},
> + {"--- a/", state_git_mode_seen, git_minus},
> {"--- a/", state_git_tree_seen, git_minus},
> + {"--- /dev/null", state_git_mode_seen, git_minus},
> {"--- /dev/null", state_git_tree_seen, git_minus},
> {"+++ b/", state_git_minus_seen, git_plus},
> {"+++ /dev/null", state_git_minus_seen, git_plus},
>
> + {"old mode ", state_git_diff_seen, git_old_mode},
> + {"new mode ", state_old_mode_seen, git_new_mode},
> +
> {"rename from ", state_git_diff_seen, git_move_from},
> + {"rename from ", state_git_mode_seen, git_move_from},
> {"rename to ", state_move_from_seen, git_move_to},
>
> {"copy from ", state_git_diff_seen, git_copy_from},
> + {"copy from ", state_git_mode_seen, git_copy_from},
> {"copy to ", state_copy_from_seen, git_copy_to},
>
> {"new file ", state_git_diff_seen, git_new_file},
> @@ -1850,6 +2057,8 @@ svn_diff_parse_next_patch(svn_patch_t **
> }
>
> patch = apr_pcalloc(result_pool, sizeof(*patch));
> + patch->old_executable_p = svn_tristate_unknown;
> + patch->new_executable_p = svn_tristate_unknown;
>
> pos = patch_file->next_patch_offset;
> SVN_ERR(svn_io_file_seek(patch_file->apr_file, APR_SET, &pos,
> scratch_pool));
> @@ -1896,7 +2105,8 @@ svn_diff_parse_next_patch(svn_patch_t **
> /* We have a valid diff header, yay! */
> break;
> }
> - else if (state == state_git_tree_seen && line_after_tree_header_read
> + else if ((state == state_git_tree_seen || state == state_git_mode_seen)
> + && line_after_tree_header_read
> && !valid_header_line)
> {
> /* git patches can contain an index line after the file mode line */
> @@ -1911,7 +2121,8 @@ svn_diff_parse_next_patch(svn_patch_t **
> break;
> }
> }
> - else if (state == state_git_tree_seen)
> + else if (state == state_git_tree_seen
> + || state == state_git_mode_seen)
> {
> line_after_tree_header_read = TRUE;
> }
>
> Propchange: subversion/trunk/subversion/libsvn_fs_x/
> ------------------------------------------------------------------------------
> --- svn:mergeinfo (original)
> +++ svn:mergeinfo Sat Sep 26 00:46:52 2015
> @@ -61,6 +61,7 @@
> /subversion/branches/multi-layer-moves/subversion/libsvn_fs_x:1239019-
> 1300930
> /subversion/branches/nfc-nfd-aware-
> client/subversion/libsvn_fs_x:870276,870376
> /subversion/branches/node_pool/subversion/libsvn_fs_x:1304828-1305388
> +/subversion/branches/patch-exec/subversion/libsvn_fs_x:1692717-
> 1705390
>
> /subversion/branches/performance/subversion/libsvn_fs_x:979193,980118,
> 981087,981090,981189,981194,981287,981684,981827,982043,982355,98339
> 8,983406,983430,983474,983488,983490,983760,983764,983766,983770,984
> 927,984973,984984,985014,985037,985046,985472,985477,985482,985487-
> 985488,985493,985497,985500,985514,985601,985603,985606,985669,98567
> 3,985695,985697,986453,986465,986485,986491-
> 986492,986517,986521,986605,986608,986817,986832,987865,987868-
> 987869,987872,987886-
> 987888,987893,988319,988898,990330,990533,990535-
> 990537,990541,990568,990572,990574-
> 990575,990600,990759,992899,992904,992911,993127,993141,994956,99547
> 8,995507,995603,998012,998858,999098,1001413,1001417,1004291,1022668
> ,1022670,1022676,1022715,1022719,1025660,1025672,1027193,1027203,102
> 7206,1027214,1027227,1028077,1028092,1028094,1028104,1028107,102811
> 1,1028354,1029038,1029042-1029043,1029054-1029055,1029062-
> 1029063,1029078,1029080,1029090,1029092-
> 1029093,1029111,1029151,1029158,1029229-1029230,1029232,1029335-
> 1029336,102
> 9339-
> 1029340,1029342,1029344,1030763,1030827,1031203,1031235,1032285,103
> 2333,1033040,1033057,1033294,1035869,1035882,1039511,1043705,105373
> 5,1056015,1066452,1067683,1067697-1078365
> /subversion/branches/pin-externals/subversion/libsvn_fs_x:1643757-
> 1659392
> /subversion/branches/py-tests-as-modules/subversion/libsvn_fs_x:956579-
> 1033052
>
> Modified: subversion/trunk/subversion/tests/cmdline/patch_tests.py
> URL:
> http://svn.apache.org/viewvc/subversion/trunk/subversion/tests/cmdline/p
> atch_tests.py?rev=1705391&r1=1705390&r2=1705391&view=diff
> ================================================================
> ==============
> --- subversion/trunk/subversion/tests/cmdline/patch_tests.py (original)
> +++ subversion/trunk/subversion/tests/cmdline/patch_tests.py Sat Sep 26
> 00:46:52 2015
> @@ -5962,6 +5962,189 @@ def patch_final_eol(sbox):
> expected_status, expected_skip,
> [], False, True, '--reverse-diff')
>
> +def patch_adds_executability_nocontents(sbox):
> + """patch adds svn:executable, without contents"""
> +
> + sbox.build(read_only=True)
> + wc_dir = sbox.wc_dir
> +
> + unidiff_patch = (
> + "diff --git a/iota b/iota\n"
> + "old mode 100644\n"
> + "new mode 100755\n"
> + )
> + patch_file_path = make_patch_path(sbox)
> + svntest.main.file_write(patch_file_path, unidiff_patch)
> +
> + expected_output = [
> + ' U %s\n' % sbox.ospath('iota'),
> + ]
> + expected_disk = svntest.main.greek_state.copy()
> + # "*" is SVN_PROP_EXECUTABLE_VALUE aka SVN_PROP_BOOLEAN_TRUE
> + expected_disk.tweak('iota', props={'svn:executable': '*'})
> +
> + expected_status = svntest.actions.get_virginal_state(wc_dir, 1)
> + expected_status.tweak('iota', status=' M')
> +
> + expected_skip = wc.State('', { })
> +
> + svntest.actions.run_and_verify_patch(wc_dir,
> os.path.abspath(patch_file_path),
> + expected_output, expected_disk,
> + expected_status, expected_skip,
> + check_props=True)
> +
> +def patch_adds_executability_yescontents(sbox):
> + """patch adds svn:executable, with contents"""
> +
> + sbox.build(read_only=True)
> + wc_dir = sbox.wc_dir
> +
> + mu_new_contents = (
> + "This is the file 'mu'.\n"
> + "with text mods too\n"
> + )
> +
> + unidiff_patch = (
> + "diff --git a/A/mu b/A/mu\n"
> + "old mode 100644\n"
> + "new mode 100755\n"
> + "index 8a0f01c..dfad3ac\n"
> + "--- a/A/mu\n"
> + "+++ b/A/mu\n"
> + "@@ -1 +1,2 @@\n"
> + " This is the file 'mu'.\n"
> + "+with text mods too\n"
> + )
> + patch_file_path = make_patch_path(sbox)
> + svntest.main.file_write(patch_file_path, unidiff_patch)
> +
> + expected_output = [
> + 'UU %s\n' % sbox.ospath('A/mu'),
> + ]
> + expected_disk = svntest.main.greek_state.copy()
> + # "*" is SVN_PROP_EXECUTABLE_VALUE aka SVN_PROP_BOOLEAN_TRUE
> + expected_disk.tweak('A/mu', props={'svn:executable': '*'})
> + expected_disk.tweak('A/mu', contents=mu_new_contents)
> +
> + expected_status = svntest.actions.get_virginal_state(wc_dir, 1)
> + expected_status.tweak('A/mu', status='MM')
> +
> + expected_skip = wc.State('', { })
> +
> + svntest.actions.run_and_verify_patch(wc_dir,
> os.path.abspath(patch_file_path),
> + expected_output, expected_disk,
> + expected_status, expected_skip,
> + check_props=True)
> +
> +def patch_deletes_executability(sbox):
> + """patch deletes svn:executable"""
> +
> + sbox.build(read_only=True)
> + wc_dir = sbox.wc_dir
> +
> + ## Set up the basic state.
> + sbox.simple_propset('svn:executable', 'yes', 'iota')
> + #sbox.simple_commit(target='iota', message="Make 'iota' executable.")
> +
> + unidiff_patch = (
> + "diff --git a/iota b/iota\n"
> + "old mode 100755\n"
> + "new mode 100644\n"
> + )
> + patch_file_path = make_patch_path(sbox)
> + svntest.main.file_write(patch_file_path, unidiff_patch)
> +
> + expected_output = [
> + ' U %s\n' % sbox.ospath('iota'),
> + ]
> + expected_disk = svntest.main.greek_state.copy()
> + expected_disk.tweak('iota') # props=None by default
> +
> + expected_status = svntest.actions.get_virginal_state(wc_dir, 1)
> + expected_status.tweak('iota', status=' ')
> +
> + expected_skip = wc.State('', { })
> +
> + svntest.actions.run_and_verify_patch(wc_dir,
> os.path.abspath(patch_file_path),
> + expected_output, expected_disk,
> + expected_status, expected_skip,
> + check_props=True)
> +
> +def patch_ambiguous_executability_contradiction(sbox):
> + """patch ambiguous svn:executable, bad"""
> +
> + sbox.build(read_only=True)
> + wc_dir = sbox.wc_dir
> +
> + unidiff_patch = (
> + "Index: iota\n"
> +
> "===============================================================
> ====\n"
> + "diff --git a/iota b/iota\n"
> + "old mode 100755\n"
> + "new mode 100644\n"
> + "Property changes on: iota\n"
> + "-------------------------------------------------------------------\n"
> + "Added: svn:executable\n"
> + "## -0,0 +1 ##\n"
> + "+*\n"
> + )
This specifies the addition of the svn:executable property with the non canonical "*\n" value. Is this really what you want to test?
I think the actual property set code will change it to the proper value, but I would say you need the " \ No newline at end of property" marker here.
The generated patchfile doesn't need it, but it does need the boolean to note the same thing.
> + patch_file_path = make_patch_path(sbox)
> + svntest.main.file_write(patch_file_path, unidiff_patch)
> +
> + expected_output = []
> +
> + expected_disk = svntest.main.greek_state.copy()
> +
> + expected_status = svntest.actions.get_virginal_state(wc_dir, 1)
> +
> + expected_skip = wc.State('', { })
> +
> + error_re_string = r'.*Invalid patch:.*contradicting.*mode.*svn:executable'
> + svntest.actions.run_and_verify_patch(wc_dir,
> os.path.abspath(patch_file_path),
> + expected_output, expected_disk,
> + expected_status, expected_skip,
> + error_re_string=error_re_string,
> + check_props=True)
> +
> +def patch_ambiguous_executability_consistent(sbox):
> + """patch ambiguous svn:executable, good"""
> +
> + sbox.build(read_only=True)
> + wc_dir = sbox.wc_dir
> +
> + unidiff_patch = (
> + "Index: iota\n"
> +
> "===============================================================
> ====\n"
> + "diff --git a/iota b/iota\n"
> + "old mode 100644\n"
> + "new mode 100755\n"
> + "Property changes on: iota\n"
> + "-------------------------------------------------------------------\n"
> + "Added: svn:executable\n"
> + "## -0,0 +1 ##\n"
> + "+*\n"
> + )
Same in this test.
> + patch_file_path = make_patch_path(sbox)
> + svntest.main.file_write(patch_file_path, unidiff_patch)
> +
> + expected_output = [
> + ' U %s\n' % sbox.ospath('iota'),
> + ]
> +
> + expected_disk = svntest.main.greek_state.copy()
> + expected_disk.tweak('iota', props={'svn:executable': '*'})
> +
> + expected_status = svntest.actions.get_virginal_state(wc_dir, 1)
> + expected_status.tweak('iota', status=' M')
> +
> + expected_skip = wc.State('', { })
> +
> + svntest.actions.run_and_verify_patch(wc_dir,
> os.path.abspath(patch_file_path),
> + expected_output, expected_disk,
> + expected_status, expected_skip,
> + error_re_string=None,
> + check_props=True)
> +
>
> ################################################################
> ########
> #Run the tests
>
> @@ -6027,6 +6210,11 @@ test_list = [ None,
> patch_delete_nodes,
> patch_delete_missing_eol,
> patch_final_eol,
> + patch_adds_executability_nocontents,
> + patch_adds_executability_yescontents,
> + patch_deletes_executability,
> + patch_ambiguous_executability_contradiction,
> + patch_ambiguous_executability_consistent,
> ]
>
> if __name__ == '__main__':
>
<snip>
Bert
RE: svn commit: r1705391 - in /subversion/trunk: ./ subversion/include/ subversion/include/private/ subversion/libsvn_client/ subversion/libsvn_diff/ subversion/libsvn_fs_x/ subversion/tests/cmdline/ subversion/tests/libsvn_diff/
Posted by Bert Huijben <be...@qqmail.nl>.
A few comments inline.
> -----Oorspronkelijk bericht-----
> Van: danielsh@apache.org [mailto:danielsh@apache.org]
> Verzonden: zaterdag 26 september 2015 02:47
> Aan: commits@subversion.apache.org
> Onderwerp: svn commit: r1705391 - in /subversion/trunk: ./
> subversion/include/ subversion/include/private/ subversion/libsvn_client/
> subversion/libsvn_diff/ subversion/libsvn_fs_x/ subversion/tests/cmdline/
> subversion/tests/libsvn_diff/
>
> Author: danielsh
> Date: Sat Sep 26 00:46:52 2015
> New Revision: 1705391
>
> URL: http://svn.apache.org/viewvc?rev=1705391&view=rev
> Log:
> patch: Parse 'old mode' / 'new mode' line from git diffs and translate them to
> svn:executable propchanges.
>
> This merges the 'patch-exec' branch into trunk.
>
> ---
>
> The following are the major changes. See the branch for detailed changes.
>
> * subversion/include/svn_diff.h
> (svn_patch_t.old_executable_p, svn_patch_t.new_executable_p): New
> members.
>
> * subversion/libsvn_client/patch.c
> (contradictory_executability): New function.
> (init_patch_target, apply_one_patch):
> Translate mode changes to svn:executable changes.
>
> * subversion/include/private/svn_diff_private.h
> (svn_diff_hunk__create_adds_single_line,
> svn_diff_hunk__create_deletes_single_line): New functions.
>
> * subversion/libsvn_diff/parse-diff.c
> (add_or_delete_single_line,
> svn_diff_hunk__create_adds_single_line,
> svn_diff_hunk__create_deletes_single_line): New functions.
> (parse_state::state_old_mode_seen,
> parse_state::state_git_mode_seen): New enumerators.
> (parse_bits_into_executability, git_old_mode, git_new_mode): New
> functions.
> (git_deleted_file, git_new_file): Parse file mode.
> (transitions): Parse "old mode" and "new mode" lines.
>
> * subversion/tests/cmdline/patch_tests.py,
> * subversion/tests/libsvn_diff/parse-diff-test.c:
> Add unit tests.
>
> Modified:
> subversion/trunk/ (props changed)
> subversion/trunk/subversion/include/private/svn_diff_private.h
> subversion/trunk/subversion/include/svn_diff.h
> subversion/trunk/subversion/libsvn_client/patch.c
> subversion/trunk/subversion/libsvn_diff/parse-diff.c
> subversion/trunk/subversion/libsvn_fs_x/ (props changed)
> subversion/trunk/subversion/tests/cmdline/patch_tests.py
> subversion/trunk/subversion/tests/libsvn_diff/parse-diff-test.c
>
> Propchange: subversion/trunk/
> ------------------------------------------------------------------------------
> --- svn:mergeinfo (original)
> +++ svn:mergeinfo Sat Sep 26 00:46:52 2015
> @@ -62,6 +62,7 @@
> /subversion/branches/multi-layer-moves:1239019-1300930
> /subversion/branches/nfc-nfd-aware-client:870276,870376
> /subversion/branches/node_pool:1304828-1305388
> +/subversion/branches/patch-exec:1692717-1705390
>
> /subversion/branches/performance:979193,980118,981087,981090,981189,
> 981194,981287,981684,981827,982043,982355,983398,983406,983430,98347
> 4,983488,983490,983760,983764,983766,983770,984927,984973,984984,985
> 014,985037,985046,985472,985477,985482,985487-
> 985488,985493,985497,985500,985514,985601,985603,985606,985669,98567
> 3,985695,985697,986453,986465,986485,986491-
> 986492,986517,986521,986605,986608,986817,986832,987865,987868-
> 987869,987872,987886-
> 987888,987893,988319,988898,990330,990533,990535-
> 990537,990541,990568,990572,990574-
> 990575,990600,990759,992899,992904,992911,993127,993141,994956,99547
> 8,995507,995603,998012,998858,999098,1001413,1001417,1004291,1022668
> ,1022670,1022676,1022715,1022719,1025660,1025672,1027193,1027203,102
> 7206,1027214,1027227,1028077,1028092,1028094,1028104,1028107,102811
> 1,1028354,1029038,1029042-1029043,1029054-1029055,1029062-
> 1029063,1029078,1029080,1029090,1029092-
> 1029093,1029111,1029151,1029158,1029229-1029230,1029232,1029335-
> 1029336,1029339-1029340,1029342,10
>
> 29344,1030763,1030827,1031203,1031235,1032285,1032333,1033040,10330
> 57,1033294,1035869,1035882,1039511,1043705,1053735,1056015,1066452,1
> 067683,1067697-1078365
> /subversion/branches/pin-externals:1643757-1659392
> /subversion/branches/py-tests-as-modules:956579-1033052
>
<snip>
> Modified: subversion/trunk/subversion/include/svn_diff.h
> URL:
> http://svn.apache.org/viewvc/subversion/trunk/subversion/include/svn_diff
> .h?rev=1705391&r1=1705390&r2=1705391&view=diff
> ================================================================
> ==============
> --- subversion/trunk/subversion/include/svn_diff.h (original)
> +++ subversion/trunk/subversion/include/svn_diff.h Sat Sep 26 00:46:52
> +++ 2015
> @@ -1288,6 +1288,24 @@ typedef struct svn_patch_t {
> * to apply it as parsed from the file.
> * @since New in 1.10. */
> svn_diff_binary_patch_t *binary_patch;
> +
> + /** The old and new executability bits, as retrieved from the patch file.
> + *
> + * A patch may specify an executability change via @a old_executable_p
> and
> + * / @a new_executable_p, via a #SVN_PROP_EXECUTABLE propchange
> hunk, or both
> + * ways; however, if both ways are used, they must specify the same
> semantic
> + * change.
> + *
> + * #svn_tristate_unknown indicates the patch does not specify the
> + * corresponding bit.
> + *
> + * @since New in 1.10.
> + */
> + /* ### This is currently not parsed out of "index" lines (where it
> + * ### serves as an assertion of the executability state, without
> + * ### changing it). */
> + svn_tristate_t old_executable_p;
> + svn_tristate_t new_executable_p;
> } svn_patch_t;
I'm not sure if we should leave this comment here in the long term...
>
> /** An opaque type representing an open patch file.
>
> Modified: subversion/trunk/subversion/libsvn_client/patch.c
> URL:
> http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_client/pa
> tch.c?rev=1705391&r1=1705390&r2=1705391&view=diff
> ================================================================
> ==============
> --- subversion/trunk/subversion/libsvn_client/patch.c (original)
> +++ subversion/trunk/subversion/libsvn_client/patch.c Sat Sep 26 00:46:52
> 2015
> @@ -46,6 +46,7 @@
> #include "private/svn_eol_private.h"
> #include "private/svn_wc_private.h"
> #include "private/svn_dep_compat.h"
> +#include "private/svn_diff_private.h"
> #include "private/svn_string_private.h"
> #include "private/svn_subr_private.h"
> #include "private/svn_sorts_private.h"
> @@ -914,6 +915,41 @@ choose_target_filename(const svn_patch_t
> return (old < new) ? patch->old_filename : patch->new_filename;
> }
>
> +/* Return whether the svn:executable proppatch and the out-of-band
> + * executability metadata contradict each other, assuming both are present.
> + */
> +static svn_boolean_t
> +contradictory_executability(const svn_patch_t *patch,
> + const prop_patch_target_t *target)
> +{
> + switch (target->operation)
> + {
> + case svn_diff_op_added:
> + return patch->new_executable_p == svn_tristate_false;
> +
> + case svn_diff_op_deleted:
> + return patch->new_executable_p == svn_tristate_true;
> +
> + case svn_diff_op_unchanged:
> + /* ### Can this happen? */
> + return (patch->old_executable_p != svn_tristate_unknown
> + && patch->new_executable_p != svn_tristate_unknown
> + && patch->old_executable_p != patch->new_executable_p);
> +
> + case svn_diff_op_modified:
> + /* Can't happen: the property should only ever be added or deleted,
> + * but never modified from one valid value to another. */
> + return (patch->old_executable_p != svn_tristate_unknown
> + && patch->new_executable_p != svn_tristate_unknown
> + && patch->old_executable_p == patch->new_executable_p);
These can happen when the svn:executable property just changes value (E.g. from '*' to 'x'). That should not happen, but it can.
> +
> + default:
> + /* Can't happen: the proppatch parser never generates other values. */
> + SVN_ERR_MALFUNCTION_NO_RETURN();
> + }
> +}
> +
> +
> /* Attempt to initialize a *PATCH_TARGET structure for a target file
> * described by PATCH. Use working copy context WC_CTX.
> * STRIP_COUNT specifies the number of leading path components
> @@ -956,6 +992,9 @@ init_patch_target(patch_target_t **patch
> target->kind_on_disk = svn_node_none;
> target->content = content;
> target->prop_targets = apr_hash_make(result_pool);
> + if (patch->new_executable_p != svn_tristate_unknown)
> + /* May also be set by apply_hunk(). */
> + target->has_prop_changes = TRUE;
>
> SVN_ERR(resolve_target_path(target, choose_target_filename(patch),
> root_abspath, strip_count, has_text_changes,
> @@ -1129,6 +1168,7 @@ init_patch_target(patch_target_t **patch
> if (! target->skipped)
> {
> apr_hash_index_t *hi;
> + prop_patch_target_t *prop_executable_target;
>
> for (hi = apr_hash_first(result_pool, patch->prop_patches);
> hi;
> @@ -1145,6 +1185,99 @@ init_patch_target(patch_target_t **patch
> result_pool, scratch_pool));
> svn_hash_sets(target->prop_targets, prop_name, prop_target);
> }
> +
> + /* Now, check for an out-of-band mode change and convert it to
> + * an svn:executable property patch. */
> + prop_executable_target = svn_hash_gets(target->prop_targets,
> + SVN_PROP_EXECUTABLE);
> + if (patch->new_executable_p != svn_tristate_unknown
> + && prop_executable_target)
> + {
> + if (contradictory_executability(patch, prop_executable_target))
> + /* Invalid input: specifies both git-like "new mode" lines and
> + * svn-like addition/removal of svn:executable.
> + *
> + * If this were merely a hunk that didn't apply, we'd reject it
> + * and move on. However, this is a self-contradictory hunk;
> + * it has no unambiguous interpretation. Therefore: */
> + return svn_error_createf(SVN_ERR_INVALID_INPUT, NULL,
> + _("Invalid patch: specifies "
> + "contradicting mode changes and "
> + "%s changes (for '%s')"),
> + SVN_PROP_EXECUTABLE,
> + target->local_abspath);
> + else
> + /* We have two representations of the same change.
> + *
> + * In the caller, there will be two hunk_info_t's for the
> + * patch: one generated from the property diff and one
> + * generated from the out-of-band mode change. Both hunks will
> + * be processed, but the output will be as though there was
> + * just one hunk.
> + *
> + * The reason there will be only a single notification is not
> + * specific to SVN_PROP_EXECUTABLE but generic to all property
> + * patches: if a patch file contains two identical property
> + * hunks (e.g.,
> + * svn ps k v iota; svn diff iota >patch; svn diff iota >>patch
> + * ), applying the patch file will only produce a single ' U'
> + * notification.
> + *
> + * In contrast, the same for file-content hunks will result in
> + * a 'U' notification followed by a 'G' notification. (The 'U'
> + * for the first copy of the hunk; the 'G' for the second.)
> + */
> + ;
> + }
> + else if (patch->new_executable_p != svn_tristate_unknown
> + && !prop_executable_target)
> + {
> + svn_diff_operation_kind_t operation;
> + svn_boolean_t nothing_to_do = FALSE;
> + prop_patch_target_t *prop_target;
> +
> + if (patch->old_executable_p == patch->new_executable_p)
> + {
> + /* Noop change. */
> + operation = svn_diff_op_unchanged;
> + }
> + else switch (patch->old_executable_p)
> + {
> + case svn_tristate_false:
> + /* Made executable. */
> + operation = svn_diff_op_added;
> + break;
> +
> + case svn_tristate_true:
> + /* Made non-executable. */
> + operation = svn_diff_op_deleted;
> + break;
> +
> + case svn_tristate_unknown:
> + if (patch->new_executable_p == svn_tristate_true)
> + /* New, executable file. */
> + operation = svn_diff_op_added;
> + else
> + /* New, non-executable file. That's not a change. */
> + nothing_to_do = TRUE;
> + break;
> +
> + default:
> + /* NOTREACHED */
> + SVN_ERR_MALFUNCTION();
> + }
> +
> + if (! nothing_to_do)
> + {
> + SVN_ERR(init_prop_target(&prop_target,
> + SVN_PROP_EXECUTABLE,
> + operation,
> + wc_ctx, target->local_abspath,
> + result_pool, scratch_pool));
> + svn_hash_sets(target->prop_targets, SVN_PROP_EXECUTABLE,
> + prop_target);
> + }
> + }
> }
> }
>
> @@ -2425,6 +2558,50 @@ apply_one_patch(patch_target_t **patch_t
> }
> }
>
> + /* Match implied property hunks. */
> + if (patch->new_executable_p != svn_tristate_unknown
> + && svn_hash_gets(target->prop_targets, SVN_PROP_EXECUTABLE))
> + {
> + hunk_info_t *hi;
> + svn_diff_hunk_t *hunk;
> + prop_patch_target_t *prop_target = svn_hash_gets(target-
> >prop_targets,
> + SVN_PROP_EXECUTABLE);
> + const char *const value = SVN_PROP_EXECUTABLE_VALUE;
> +
> + switch (prop_target->operation)
> + {
> + case svn_diff_op_added:
> + SVN_ERR(svn_diff_hunk__create_adds_single_line(&hunk, value,
> patch,
> + result_pool,
> + iterpool));
> + break;
> +
> + case svn_diff_op_deleted:
> + SVN_ERR(svn_diff_hunk__create_deletes_single_line(&hunk, value,
> + patch,
> + result_pool,
> + iterpool));
> + break;
> +
> + case svn_diff_op_unchanged:
> + /* ### What to do? */
> + break;
> +
> + default:
> + SVN_ERR_MALFUNCTION();
> + }
> +
> + /* Derive a hunk_info from hunk. */
> + SVN_ERR(get_hunk_info(&hi, target, prop_target->content,
> + hunk, 0 /* fuzz */, 0 /* previous_offset */,
> + ignore_whitespace,
> + TRUE /* is_prop_hunk */,
> + cancel_func, cancel_baton,
> + result_pool, iterpool));
> + if (! hi->already_applied)
> + APR_ARRAY_PUSH(prop_target->content->hunks, hunk_info_t *) = hi;
> + }
> +
> /* Apply or reject property hunks. */
> for (hash_index = apr_hash_first(scratch_pool, target->prop_targets);
> hash_index;
>
> Modified: subversion/trunk/subversion/libsvn_diff/parse-diff.c
> URL:
> http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_diff/pars
> e-diff.c?rev=1705391&r1=1705390&r2=1705391&view=diff
> ================================================================
> ==============
> --- subversion/trunk/subversion/libsvn_diff/parse-diff.c (original)
> +++ subversion/trunk/subversion/libsvn_diff/parse-diff.c Sat Sep 26 00:46:52
> 2015
> @@ -40,6 +40,7 @@
>
> #include "private/svn_eol_private.h"
> #include "private/svn_dep_compat.h"
> +#include "private/svn_diff_private.h"
> #include "private/svn_sorts_private.h"
>
> #include "diff.h"
> @@ -108,6 +109,116 @@ struct svn_diff_binary_patch_t {
> svn_filesize_t dst_filesize; /* Expanded/final size */
> };
>
> +/* Common guts of svn_diff_hunk__create_adds_single_line() and
> + * svn_diff_hunk__create_deletes_single_line().
> + *
> + * ADD is TRUE if adding and FALSE if deleting.
> + */
> +static svn_error_t *
> +add_or_delete_single_line(svn_diff_hunk_t **hunk_out,
> + const char *line,
> + svn_patch_t *patch,
> + svn_boolean_t add,
> + apr_pool_t *result_pool,
> + apr_pool_t *scratch_pool)
> +{
> + svn_diff_hunk_t *hunk = apr_palloc(result_pool, sizeof(*hunk));
As pcalloc() would be safer here...
> + static const char *hunk_header[] = { "@@ -1 +0,0 @@\n", "@@ -0,0 +1
> @@\n" };
> + const unsigned len = strlen(line);
> + /* The +1 is for the 'plus' start-of-line character. */
> + const apr_off_t end = STRLEN_LITERAL(hunk_header[add]) + (1 + len);
> + /* The +1 is for the second \n. */
> + svn_stringbuf_t *buf = svn_stringbuf_create_ensure(end + 1,
> scratch_pool);
> +
> + hunk->patch = patch;
> +
> + /* hunk->apr_file is created below. */
> +
> + hunk->diff_text_range.start = STRLEN_LITERAL(hunk_header[add]);
> + hunk->diff_text_range.current = STRLEN_LITERAL(hunk_header[add]);
> + hunk->diff_text_range.end = end;
> +
> + if (add)
> + {
> + hunk->original_text_range.start = 0; /* There's no "original" text. */
> + hunk->original_text_range.current = 0;
> + hunk->original_text_range.end = 0;
> +
> + hunk->modified_text_range.start = STRLEN_LITERAL(hunk_header[add]);
> + hunk->modified_text_range.current =
> STRLEN_LITERAL(hunk_header[add]);
> + hunk->modified_text_range.end = end;
> +
> + hunk->original_start = 0;
> + hunk->original_length = 0;
> +
> + hunk->modified_start = 1;
> + hunk->modified_length = 1;
> + }
> + else /* delete */
> + {
> + hunk->original_text_range.start = STRLEN_LITERAL(hunk_header[add]);
> + hunk->original_text_range.current =
> STRLEN_LITERAL(hunk_header[add]);
> + hunk->original_text_range.end = end;
> +
> + hunk->modified_text_range.start = 0; /* There's no "original" text. */
> + hunk->modified_text_range.current = 0;
> + hunk->modified_text_range.end = 0;
> +
> + hunk->original_start = 1;
> + hunk->original_length = 1;
> +
> + hunk->modified_start = 0;
> + hunk->modified_length = 0; /* setting to '1' works too */
> + }
> +
> + hunk->leading_context = 0;
> + hunk->trailing_context = 0;
As this patch forgets to set the new booleans added on trunk to signal no EOL markers here.
You might need to set one of them to true though, as I think you are trying to add a property with no end of line.
> +
> + /* Create APR_FILE and put just a hunk in it (without a diff header).
> + * Save the offset of the last byte of the diff line. */
> + svn_stringbuf_appendbytes(buf, hunk_header[add],
> + STRLEN_LITERAL(hunk_header[add]));
> + svn_stringbuf_appendbyte(buf, add ? '+' : '-');
> + svn_stringbuf_appendbytes(buf, line, len);
> + svn_stringbuf_appendbyte(buf, '\n');
> +
> + SVN_ERR(svn_io_open_unique_file3(&hunk->apr_file, NULL /* filename
> */,
> + NULL /* system tempdir */,
> + svn_io_file_del_on_pool_cleanup,
> + result_pool, scratch_pool));
> + SVN_ERR(svn_io_file_write_full(hunk->apr_file,
> + buf->data, buf->len,
> + NULL, scratch_pool));
> + /* No need to seek. */
> +
> + *hunk_out = hunk;
> + return SVN_NO_ERROR;
> +}
> +
> +svn_error_t *
> +svn_diff_hunk__create_adds_single_line(svn_diff_hunk_t **hunk_out,
> + const char *line,
> + svn_patch_t *patch,
> + apr_pool_t *result_pool,
> + apr_pool_t *scratch_pool)
> +{
> + SVN_ERR(add_or_delete_single_line(hunk_out, line, patch, TRUE,
> + result_pool, scratch_pool));
> + return SVN_NO_ERROR;
> +}
> +
> +svn_error_t *
> +svn_diff_hunk__create_deletes_single_line(svn_diff_hunk_t **hunk_out,
> + const char *line,
> + svn_patch_t *patch,
> + apr_pool_t *result_pool,
> + apr_pool_t *scratch_pool)
> +{
> + SVN_ERR(add_or_delete_single_line(hunk_out, line, patch, FALSE,
> + result_pool, scratch_pool));
> + return SVN_NO_ERROR;
> +}
> +
> void
> svn_diff_hunk_reset_diff_text(svn_diff_hunk_t *hunk)
> {
> @@ -1240,6 +1351,8 @@ enum parse_state
> state_git_tree_seen, /* a tree operation, rather than content change */
> state_git_minus_seen, /* --- /dev/null; or --- a/ */
> state_git_plus_seen, /* +++ /dev/null; or +++ a/ */
> + state_old_mode_seen, /* old mode 100644 */
> + state_git_mode_seen, /* new mode 100644 */
> state_move_from_seen, /* rename from foo.c */
> state_copy_from_seen, /* copy from foo.c */
> state_minus_seen, /* --- foo.c */
> @@ -1472,6 +1585,85 @@ git_plus(enum parse_state *new_state, ch
> return SVN_NO_ERROR;
> }
>
> +/* Helper for git_old_mode() and git_new_mode(). Translate the git
> + * file mode MODE_STR into a binary "executable?" notion EXECUTABLE_P.
> */
> +static svn_error_t *
> +parse_bits_into_executability(svn_tristate_t *executable_p,
> + const char *mode_str)
> +{
> + apr_uint64_t mode;
> + SVN_ERR(svn_cstring_strtoui64(&mode, mode_str,
> + 0 /* min */,
> + 0777777 /* max: six octal digits */,
> + 010 /* radix (octal) */));
> +
> + /* Note: 0644 and 0755 are the only modes that can occur for plain files.
> + * We deliberately choose to parse only those values: we are strict in what
> + * we accept _and_ in what we produce.
> + *
> + * (Having said that, though, we could consider relaxing the parser to also
> + * map
> + * (mode & 0111) == 0000 -> svn_tristate_false
> + * (mode & 0111) == 0111 -> svn_tristate_true
> + * [anything else] -> svn_tristate_unknown
> + * .)
> + */
> +
> + switch (mode & 0777)
> + {
> + case 0644:
> + *executable_p = svn_tristate_false;
> + break;
> +
> + case 0755:
> + *executable_p = svn_tristate_true;
> + break;
> +
> + default:
> + /* Ignore unknown values. */
> + *executable_p = svn_tristate_unknown;
> + break;
> + }
> +
> + return SVN_NO_ERROR;
> +}
> +
> +/* Parse the 'old mode ' line of a git extended unidiff. */
> +static svn_error_t *
> +git_old_mode(enum parse_state *new_state, char *line, svn_patch_t
> *patch,
> + apr_pool_t *result_pool, apr_pool_t *scratch_pool)
> +{
> + SVN_ERR(parse_bits_into_executability(&patch->old_executable_p,
> + line + STRLEN_LITERAL("old mode ")));
> +
> +#ifdef SVN_DEBUG
> + /* If this assert trips, the "old mode" is neither ...644 nor ...755 . */
> + SVN_ERR_ASSERT(patch->old_executable_p != svn_tristate_unknown);
> +#endif
I don't think we should leave these enabled even in maintainer mode. This will break abilities to apply diffs generated from other tools. (Perhaps even from older Subversion versions as I think you recently changed what Subversion generates itself on trunk)
> +
> + *new_state = state_old_mode_seen;
> + return SVN_NO_ERROR;
> +}
> +
> +/* Parse the 'new mode ' line of a git extended unidiff. */
> +static svn_error_t *
> +git_new_mode(enum parse_state *new_state, char *line, svn_patch_t
> *patch,
> + apr_pool_t *result_pool, apr_pool_t *scratch_pool)
> +{
> + SVN_ERR(parse_bits_into_executability(&patch->new_executable_p,
> + line + STRLEN_LITERAL("new mode ")));
> +
> +#ifdef SVN_DEBUG
> + /* If this assert trips, the "old mode" is neither ...644 nor ...755 . */
> + SVN_ERR_ASSERT(patch->new_executable_p != svn_tristate_unknown);
> +#endif
Same here.
> +
> + /* Don't touch patch->operation. */
> +
> + *new_state = state_git_mode_seen;
> + return SVN_NO_ERROR;
> +}
> +
> /* Parse the 'rename from ' line of a git extended unidiff. */
> static svn_error_t *
> git_move_from(enum parse_state *new_state, char *line, svn_patch_t
> *patch,
> @@ -1532,6 +1724,10 @@ static svn_error_t *
> git_new_file(enum parse_state *new_state, char *line, svn_patch_t *patch,
> apr_pool_t *result_pool, apr_pool_t *scratch_pool)
> {
> + SVN_ERR(
> + parse_bits_into_executability(&patch->new_executable_p,
> + line + STRLEN_LITERAL("new file mode ")));
> +
> patch->operation = svn_diff_op_added;
>
> /* Filename already retrieved from diff --git header. */
> @@ -1545,6 +1741,10 @@ static svn_error_t *
> git_deleted_file(enum parse_state *new_state, char *line, svn_patch_t
> *patch,
> apr_pool_t *result_pool, apr_pool_t *scratch_pool)
> {
> + SVN_ERR(
> + parse_bits_into_executability(&patch->old_executable_p,
> + line + STRLEN_LITERAL("deleted file mode ")));
> +
> patch->operation = svn_diff_op_deleted;
>
> /* Filename already retrieved from diff --git header. */
> @@ -1808,15 +2008,22 @@ static struct transition transitions[] =
>
> {"diff --git", state_start, git_start},
> {"--- a/", state_git_diff_seen, git_minus},
> + {"--- a/", state_git_mode_seen, git_minus},
> {"--- a/", state_git_tree_seen, git_minus},
> + {"--- /dev/null", state_git_mode_seen, git_minus},
> {"--- /dev/null", state_git_tree_seen, git_minus},
> {"+++ b/", state_git_minus_seen, git_plus},
> {"+++ /dev/null", state_git_minus_seen, git_plus},
>
> + {"old mode ", state_git_diff_seen, git_old_mode},
> + {"new mode ", state_old_mode_seen, git_new_mode},
> +
> {"rename from ", state_git_diff_seen, git_move_from},
> + {"rename from ", state_git_mode_seen, git_move_from},
> {"rename to ", state_move_from_seen, git_move_to},
>
> {"copy from ", state_git_diff_seen, git_copy_from},
> + {"copy from ", state_git_mode_seen, git_copy_from},
> {"copy to ", state_copy_from_seen, git_copy_to},
>
> {"new file ", state_git_diff_seen, git_new_file},
> @@ -1850,6 +2057,8 @@ svn_diff_parse_next_patch(svn_patch_t **
> }
>
> patch = apr_pcalloc(result_pool, sizeof(*patch));
> + patch->old_executable_p = svn_tristate_unknown;
> + patch->new_executable_p = svn_tristate_unknown;
>
> pos = patch_file->next_patch_offset;
> SVN_ERR(svn_io_file_seek(patch_file->apr_file, APR_SET, &pos,
> scratch_pool));
> @@ -1896,7 +2105,8 @@ svn_diff_parse_next_patch(svn_patch_t **
> /* We have a valid diff header, yay! */
> break;
> }
> - else if (state == state_git_tree_seen && line_after_tree_header_read
> + else if ((state == state_git_tree_seen || state == state_git_mode_seen)
> + && line_after_tree_header_read
> && !valid_header_line)
> {
> /* git patches can contain an index line after the file mode line */
> @@ -1911,7 +2121,8 @@ svn_diff_parse_next_patch(svn_patch_t **
> break;
> }
> }
> - else if (state == state_git_tree_seen)
> + else if (state == state_git_tree_seen
> + || state == state_git_mode_seen)
> {
> line_after_tree_header_read = TRUE;
> }
>
> Propchange: subversion/trunk/subversion/libsvn_fs_x/
> ------------------------------------------------------------------------------
> --- svn:mergeinfo (original)
> +++ svn:mergeinfo Sat Sep 26 00:46:52 2015
> @@ -61,6 +61,7 @@
> /subversion/branches/multi-layer-moves/subversion/libsvn_fs_x:1239019-
> 1300930
> /subversion/branches/nfc-nfd-aware-
> client/subversion/libsvn_fs_x:870276,870376
> /subversion/branches/node_pool/subversion/libsvn_fs_x:1304828-1305388
> +/subversion/branches/patch-exec/subversion/libsvn_fs_x:1692717-
> 1705390
>
> /subversion/branches/performance/subversion/libsvn_fs_x:979193,980118,
> 981087,981090,981189,981194,981287,981684,981827,982043,982355,98339
> 8,983406,983430,983474,983488,983490,983760,983764,983766,983770,984
> 927,984973,984984,985014,985037,985046,985472,985477,985482,985487-
> 985488,985493,985497,985500,985514,985601,985603,985606,985669,98567
> 3,985695,985697,986453,986465,986485,986491-
> 986492,986517,986521,986605,986608,986817,986832,987865,987868-
> 987869,987872,987886-
> 987888,987893,988319,988898,990330,990533,990535-
> 990537,990541,990568,990572,990574-
> 990575,990600,990759,992899,992904,992911,993127,993141,994956,99547
> 8,995507,995603,998012,998858,999098,1001413,1001417,1004291,1022668
> ,1022670,1022676,1022715,1022719,1025660,1025672,1027193,1027203,102
> 7206,1027214,1027227,1028077,1028092,1028094,1028104,1028107,102811
> 1,1028354,1029038,1029042-1029043,1029054-1029055,1029062-
> 1029063,1029078,1029080,1029090,1029092-
> 1029093,1029111,1029151,1029158,1029229-1029230,1029232,1029335-
> 1029336,102
> 9339-
> 1029340,1029342,1029344,1030763,1030827,1031203,1031235,1032285,103
> 2333,1033040,1033057,1033294,1035869,1035882,1039511,1043705,105373
> 5,1056015,1066452,1067683,1067697-1078365
> /subversion/branches/pin-externals/subversion/libsvn_fs_x:1643757-
> 1659392
> /subversion/branches/py-tests-as-modules/subversion/libsvn_fs_x:956579-
> 1033052
>
> Modified: subversion/trunk/subversion/tests/cmdline/patch_tests.py
> URL:
> http://svn.apache.org/viewvc/subversion/trunk/subversion/tests/cmdline/p
> atch_tests.py?rev=1705391&r1=1705390&r2=1705391&view=diff
> ================================================================
> ==============
> --- subversion/trunk/subversion/tests/cmdline/patch_tests.py (original)
> +++ subversion/trunk/subversion/tests/cmdline/patch_tests.py Sat Sep 26
> 00:46:52 2015
> @@ -5962,6 +5962,189 @@ def patch_final_eol(sbox):
> expected_status, expected_skip,
> [], False, True, '--reverse-diff')
>
> +def patch_adds_executability_nocontents(sbox):
> + """patch adds svn:executable, without contents"""
> +
> + sbox.build(read_only=True)
> + wc_dir = sbox.wc_dir
> +
> + unidiff_patch = (
> + "diff --git a/iota b/iota\n"
> + "old mode 100644\n"
> + "new mode 100755\n"
> + )
> + patch_file_path = make_patch_path(sbox)
> + svntest.main.file_write(patch_file_path, unidiff_patch)
> +
> + expected_output = [
> + ' U %s\n' % sbox.ospath('iota'),
> + ]
> + expected_disk = svntest.main.greek_state.copy()
> + # "*" is SVN_PROP_EXECUTABLE_VALUE aka SVN_PROP_BOOLEAN_TRUE
> + expected_disk.tweak('iota', props={'svn:executable': '*'})
> +
> + expected_status = svntest.actions.get_virginal_state(wc_dir, 1)
> + expected_status.tweak('iota', status=' M')
> +
> + expected_skip = wc.State('', { })
> +
> + svntest.actions.run_and_verify_patch(wc_dir,
> os.path.abspath(patch_file_path),
> + expected_output, expected_disk,
> + expected_status, expected_skip,
> + check_props=True)
> +
> +def patch_adds_executability_yescontents(sbox):
> + """patch adds svn:executable, with contents"""
> +
> + sbox.build(read_only=True)
> + wc_dir = sbox.wc_dir
> +
> + mu_new_contents = (
> + "This is the file 'mu'.\n"
> + "with text mods too\n"
> + )
> +
> + unidiff_patch = (
> + "diff --git a/A/mu b/A/mu\n"
> + "old mode 100644\n"
> + "new mode 100755\n"
> + "index 8a0f01c..dfad3ac\n"
> + "--- a/A/mu\n"
> + "+++ b/A/mu\n"
> + "@@ -1 +1,2 @@\n"
> + " This is the file 'mu'.\n"
> + "+with text mods too\n"
> + )
> + patch_file_path = make_patch_path(sbox)
> + svntest.main.file_write(patch_file_path, unidiff_patch)
> +
> + expected_output = [
> + 'UU %s\n' % sbox.ospath('A/mu'),
> + ]
> + expected_disk = svntest.main.greek_state.copy()
> + # "*" is SVN_PROP_EXECUTABLE_VALUE aka SVN_PROP_BOOLEAN_TRUE
> + expected_disk.tweak('A/mu', props={'svn:executable': '*'})
> + expected_disk.tweak('A/mu', contents=mu_new_contents)
> +
> + expected_status = svntest.actions.get_virginal_state(wc_dir, 1)
> + expected_status.tweak('A/mu', status='MM')
> +
> + expected_skip = wc.State('', { })
> +
> + svntest.actions.run_and_verify_patch(wc_dir,
> os.path.abspath(patch_file_path),
> + expected_output, expected_disk,
> + expected_status, expected_skip,
> + check_props=True)
> +
> +def patch_deletes_executability(sbox):
> + """patch deletes svn:executable"""
> +
> + sbox.build(read_only=True)
> + wc_dir = sbox.wc_dir
> +
> + ## Set up the basic state.
> + sbox.simple_propset('svn:executable', 'yes', 'iota')
> + #sbox.simple_commit(target='iota', message="Make 'iota' executable.")
> +
> + unidiff_patch = (
> + "diff --git a/iota b/iota\n"
> + "old mode 100755\n"
> + "new mode 100644\n"
> + )
> + patch_file_path = make_patch_path(sbox)
> + svntest.main.file_write(patch_file_path, unidiff_patch)
> +
> + expected_output = [
> + ' U %s\n' % sbox.ospath('iota'),
> + ]
> + expected_disk = svntest.main.greek_state.copy()
> + expected_disk.tweak('iota') # props=None by default
> +
> + expected_status = svntest.actions.get_virginal_state(wc_dir, 1)
> + expected_status.tweak('iota', status=' ')
> +
> + expected_skip = wc.State('', { })
> +
> + svntest.actions.run_and_verify_patch(wc_dir,
> os.path.abspath(patch_file_path),
> + expected_output, expected_disk,
> + expected_status, expected_skip,
> + check_props=True)
> +
> +def patch_ambiguous_executability_contradiction(sbox):
> + """patch ambiguous svn:executable, bad"""
> +
> + sbox.build(read_only=True)
> + wc_dir = sbox.wc_dir
> +
> + unidiff_patch = (
> + "Index: iota\n"
> +
> "===============================================================
> ====\n"
> + "diff --git a/iota b/iota\n"
> + "old mode 100755\n"
> + "new mode 100644\n"
> + "Property changes on: iota\n"
> + "-------------------------------------------------------------------\n"
> + "Added: svn:executable\n"
> + "## -0,0 +1 ##\n"
> + "+*\n"
> + )
This specifies the addition of the svn:executable property with the non canonical "*\n" value. Is this really what you want to test?
I think the actual property set code will change it to the proper value, but I would say you need the " \ No newline at end of property" marker here.
The generated patchfile doesn't need it, but it does need the boolean to note the same thing.
> + patch_file_path = make_patch_path(sbox)
> + svntest.main.file_write(patch_file_path, unidiff_patch)
> +
> + expected_output = []
> +
> + expected_disk = svntest.main.greek_state.copy()
> +
> + expected_status = svntest.actions.get_virginal_state(wc_dir, 1)
> +
> + expected_skip = wc.State('', { })
> +
> + error_re_string = r'.*Invalid patch:.*contradicting.*mode.*svn:executable'
> + svntest.actions.run_and_verify_patch(wc_dir,
> os.path.abspath(patch_file_path),
> + expected_output, expected_disk,
> + expected_status, expected_skip,
> + error_re_string=error_re_string,
> + check_props=True)
> +
> +def patch_ambiguous_executability_consistent(sbox):
> + """patch ambiguous svn:executable, good"""
> +
> + sbox.build(read_only=True)
> + wc_dir = sbox.wc_dir
> +
> + unidiff_patch = (
> + "Index: iota\n"
> +
> "===============================================================
> ====\n"
> + "diff --git a/iota b/iota\n"
> + "old mode 100644\n"
> + "new mode 100755\n"
> + "Property changes on: iota\n"
> + "-------------------------------------------------------------------\n"
> + "Added: svn:executable\n"
> + "## -0,0 +1 ##\n"
> + "+*\n"
> + )
Same in this test.
> + patch_file_path = make_patch_path(sbox)
> + svntest.main.file_write(patch_file_path, unidiff_patch)
> +
> + expected_output = [
> + ' U %s\n' % sbox.ospath('iota'),
> + ]
> +
> + expected_disk = svntest.main.greek_state.copy()
> + expected_disk.tweak('iota', props={'svn:executable': '*'})
> +
> + expected_status = svntest.actions.get_virginal_state(wc_dir, 1)
> + expected_status.tweak('iota', status=' M')
> +
> + expected_skip = wc.State('', { })
> +
> + svntest.actions.run_and_verify_patch(wc_dir,
> os.path.abspath(patch_file_path),
> + expected_output, expected_disk,
> + expected_status, expected_skip,
> + error_re_string=None,
> + check_props=True)
> +
>
> ################################################################
> ########
> #Run the tests
>
> @@ -6027,6 +6210,11 @@ test_list = [ None,
> patch_delete_nodes,
> patch_delete_missing_eol,
> patch_final_eol,
> + patch_adds_executability_nocontents,
> + patch_adds_executability_yescontents,
> + patch_deletes_executability,
> + patch_ambiguous_executability_contradiction,
> + patch_ambiguous_executability_consistent,
> ]
>
> if __name__ == '__main__':
>
<snip>
Bert