subversion-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Bert Huijben" <b...@qqmail.nl>
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/
Date Sat, 26 Sep 2015 08:06:15 GMT
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



Mime
View raw message