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
