You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Julian Foad <ju...@btopenworld.com> on 2004/03/08 23:30:09 UTC
Issue #1756 -- import doesn't handle svn:eol-style or svn:keywords
Candidate for 1.0.1:
* r8839,r8932
Finish Issue #1756 -- import doesn't handle svn:eol-style or svn:keywords
Justification: Allows malformed data into the repository.
Votes:
+1: kfogel, sussman
Old votes (for r8839 alone):
+1: cmpilato, jszakmeister
-1: kfogel (doc string bug, fixed in r8932)
I am voting -1 for this, due to the "repair" of line endings. For the other reasons mentioned below I would have voted "-0".
A normal commit, via svn_wc_transmit_text_deltas, does not force repair of inconsistent line endings, but fails with an error. This patch to "import", on the other hand, silently "repairs" inconsistent line endings. I think that is the wrong thing for it to do, as I don't see any reason for it to behave differently from "commit".
This patch is on the fuzzy line between fix and enhancement. Lack of this patch only "allows malformed data" in the same way that any commit does if "svn:eol-style" and "svn:keywords" are not as intended. Behaviour without this patch is to preserve the file contents exactly. For these reasons I don't think there is a strong need to put this patch in 1.0.1.
I have reviewed the code but have not tested it. Of course, it would be nice to see regression tests for it, but I can't insist on that. I hope I can assume the "+1" reviewers tested it :-)
One minor thing I found in review is that, while the patch uses svn_string_create generally to create strings, in one place it was done long-hand. This additional patch tidies it up. but there is certainly no need to back-port this:
Index: subversion/libsvn_client/add.c
===================================================================
--- subversion/libsvn_client/add.c (revision 8939)
+++ subversion/libsvn_client/add.c (working copy)
@@ -121,12 +121,8 @@
len = strlen (property);
if (len > 0)
{
- svn_string_t *propval = apr_pcalloc (autoprops->pool,
- sizeof (*propval));
- propval->data = this_value;
- propval->len = strlen (this_value);
-
- apr_hash_set (autoprops->properties, property, len, propval);
+ apr_hash_set (autoprops->properties, property, len,
+ svn_string_create (this_value, autoprops->pool));
if (strcmp (property, SVN_PROP_MIME_TYPE) == 0)
autoprops->mimetype = this_value;
else if (strcmp (property, SVN_PROP_EXECUTABLE) == 0)
I will commit that tidy-up separately. (You know how much I like tidying up. I also know how dangerous it can be.)
- Julian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Issue #1756 -- import doesn't handle svn:eol-style or svn:keywords
Posted by Julian Foad <ju...@btopenworld.com>.
kfogel@collab.net wrote:
> Julian Foad <ju...@btopenworld.com> writes:
>
>>I am voting -1 for this, due to the "repair" of line endings. For
>>the other reasons mentioned below I would have voted "-0".
>>
>>A normal commit, via svn_wc_transmit_text_deltas, does not force
>>repair of inconsistent line endings, but fails with an error. This
>>patch to "import", on the other hand, silently "repairs"
>>inconsistent line endings. I think that is the wrong thing for it
>>to do, as I don't see any reason for it to behave differently from
>>"commit".
>>
>>This patch is on the fuzzy line between fix and enhancement. Lack
>>of this patch only "allows malformed data" in the same way that any
>>commit does if "svn:eol-style" and "svn:keywords" are not as
>>intended. Behaviour without this patch is to preserve the file
>>contents exactly. For these reasons I don't think there is a strong
>>need to put this patch in 1.0.1.
>
> I voted +1 for the change, but still have to say your argument is
> persuasive...
>
> Next question is: does the change belong in 1.1? Do you consider the
> behavior undesirable overall, or just not appropriate for 1.0.1?
I consider the behaviour to be desirable overall, and better than the status quo, but just lacking a simple fix to make it consistent with "commit". Given that fix, it definitely belongs in 1.1, and maybe even in 1.0.1 if other people think it more important than I do. (This fixes one aspect of "import", but I don't like "import" anyway, as there are so many other ways in which it can do what I don't want: add files that I don't want, wrongly guess the binariness of files, etc. I nearly always copy the project into an empty WC and recursive-add it; since I am going to want a WC anyway, this saves the checkout, and gives me the chance to remove unwanted files and adjust the properties before making the first commit.)
- Julian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Issue #1756 -- import doesn't handle svn:eol-style or
svn:keywords
Posted by Philip Martin <ph...@codematters.co.uk>.
kfogel@collab.net writes:
> - commit w/ svn:eol-style==native does not repair, thus fails
> - add+commit w/ svn:eol-style==native does not repair, thus fails
> - import w/ svn:eol-style==native does not repair, then ???
>
> The "???" means "Does it fail, or does it just get committed as-is,
> with inconsistent line endings?"
It fails.
--
Philip Martin
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Issue #1756 -- import doesn't handle svn:eol-style or svn:keywords
Posted by kf...@collab.net.
Greg Hudson <gh...@MIT.EDU> writes:
> As always, I am -1 on any Subversion default which destroys data in
> svn:eol-style=native files. So, this change does not belong on the
> trunk as I understand it.
Following Julian's most recent tweak on trunk, let's try to summarize.
My understanding is that with trunk code now, a file with inconsistent
line endings behaves like so:
- commit w/ svn:eol-style==native does not repair, thus fails
- add+commit w/ svn:eol-style==native does not repair, thus fails
- import w/ svn:eol-style==native does not repair, then ???
The "???" means "Does it fail, or does it just get committed as-is,
with inconsistent line endings?"
If it fails, then we have consistency, and no danger of committing
something the user didn't want committed (either they should turn off
the prop, or fix the file contents).
That seems like a fine situation, and is in keeping with the idea of
not destroying data.
Assuming this summary is accurate, does anyone think the current trunk
situation is undesirable?
> (If people want to introduce a --repair-newlines option which performs
> the data-destroying transformation for such files, I'm fine with that.)
Agreed, though not in 1.0.x of course.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Issue #1756 -- import doesn't handle svn:eol-style or
svn:keywords
Posted by Greg Hudson <gh...@MIT.EDU>.
On Mon, 2004-03-08 at 21:24, kfogel@collab.net wrote:
> Next question is: does the change belong in 1.1? Do you consider the
> behavior undesirable overall, or just not appropriate for 1.0.1?
As always, I am -1 on any Subversion default which destroys data in
svn:eol-style=native files. So, this change does not belong on the
trunk as I understand it.
(If people want to introduce a --repair-newlines option which performs
the data-destroying transformation for such files, I'm fine with that.)
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Issue #1756 -- import doesn't handle svn:eol-style or svn:keywords
Posted by kf...@collab.net.
Julian Foad <ju...@btopenworld.com> writes:
> I am voting -1 for this, due to the "repair" of line endings. For
> the other reasons mentioned below I would have voted "-0".
>
> A normal commit, via svn_wc_transmit_text_deltas, does not force
> repair of inconsistent line endings, but fails with an error. This
> patch to "import", on the other hand, silently "repairs"
> inconsistent line endings. I think that is the wrong thing for it
> to do, as I don't see any reason for it to behave differently from
> "commit".
>
> This patch is on the fuzzy line between fix and enhancement. Lack
> of this patch only "allows malformed data" in the same way that any
> commit does if "svn:eol-style" and "svn:keywords" are not as
> intended. Behaviour without this patch is to preserve the file
> contents exactly. For these reasons I don't think there is a strong
> need to put this patch in 1.0.1.
I voted +1 for the change, but still have to say your argument is
persuasive...
Next question is: does the change belong in 1.1? Do you consider the
behavior undesirable overall, or just not appropriate for 1.0.1?
-K
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Issue #1756 -- import doesn't handle svn:eol-style or svn:keywords
Posted by Edmund Horner <ch...@chrysophylax.cjb.net>.
Julian Foad wrote:
> A normal commit, via svn_wc_transmit_text_deltas, does not force repair
> of inconsistent line endings, but fails with an error. This patch to
> "import", on the other hand, silently "repairs" inconsistent line
> endings. I think that is the wrong thing for it to do, as I don't see
> any reason for it to behave differently from "commit".
Speaking as a user on a nameless tool-impaired operating system, I'd
like it if there was an option to automatically repair the line-endings.
Often I've wanted to add text files with svn:eol-style = native, but
which had harmless inconsistent endings. There is no standard, easy way
to fix that on my OS, so it would be nice is Subversion could do it for
me. :-)
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Issue #1756 -- import doesn't handle svn:eol-style or svn:keywords
Posted by Julian Foad <ju...@btopenworld.com>.
Philip Martin wrote:
> Julian Foad <ju...@btopenworld.com> writes:
>
>>OK - committed in r8950. Perhaps people could review and vote again.
>
> You need to vote again :)
... because you found another bug and fixed it in r8952. Yikes. Is that because - perish the thought - you actually tested it? That would be cheeky: it would give you an unfair advantage over armchair reviewers like me :-) Ahem, I jest of course. Well spotted, whether by diligent testing or sharp-eyed review.
- Julian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Issue #1756 -- import doesn't handle svn:eol-style or
svn:keywords
Posted by Philip Martin <ph...@codematters.co.uk>.
Julian Foad <ju...@btopenworld.com> writes:
> OK - committed in r8950. Perhaps people could review and vote again.
You need to vote again :)
--
Philip Martin
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Issue #1756 -- import doesn't handle svn:eol-style or svn:keywords
Posted by Julian Foad <ju...@btopenworld.com>.
C. Michael Pilato wrote:
> Julian Foad <ju...@btopenworld.com> writes:
>
>>What I didn't say explicitly is that I am wholly in favour of what I
>>believe was the intent of this patch, and was merely pointing out
>>that the patch is not quite right: it should not request repair
>>(which is just a matter of changing a "TRUE" to "FALSE").
>
> The decision to not repair line-endings was admittedly somewhat
> arbitrary. In retrospect, I think repairing the line endings is
> fine.
OK - committed in r8950. Perhaps people could review and vote again.
- Julian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Issue #1756 -- import doesn't handle svn:eol-style or svn:keywords
Posted by Julian Foad <ju...@btopenworld.com>.
Erik Huelsmann wrote:
>
> In what way does the import behaviour relate to 'add'? Does 'add' do
> translation? If so (and I'll be trying to find out too), then I think import should
> follow the behaviour of 'add' not of commit.
"import" is like the combination of "add" and "commit". When I said "commit" I was effectively referring to that combination, I think.
- Julian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Issue #1756 -- import doesn't handle svn:eol-style or svn:keywords
Posted by Erik Huelsmann <e....@gmx.net>.
Oh. ok you noticed....
> Why do you think repairing the line endings is fine? It is inconsistent
> with "commit". Do you think commit should repair them too, or do you
think
> it is right for them to behave differently? Or did you just mean that it
> is fine for now, because the patch is an improvement over not having the
> patch, and the "repairing" issue is just a further, minor issue that can
be
> addressed later?
In what way does the import behaviour relate to 'add'? Does 'add' do
translation? If so (and I'll be trying to find out too), then I think import should
follow the behaviour of 'add' not of commit.
bye,
Erik.
--
+++ NEU bei GMX und erstmalig in Deutschland: TÜV-geprüfter Virenschutz +++
100% Virenerkennung nach Wildlist. Infos: http://www.gmx.net/virenschutz
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Issue #1756 -- import doesn't handle svn:eol-style or svn:keywords
Posted by Julian Foad <ju...@btopenworld.com>.
C. Michael Pilato wrote:
>
> It is perfectly okay for them to behave differently, in my opinion.
> In fact, it is perfectly okay for commit itself to behave differently
> depending on what's being committed -- we could make commits of "adds"
> do repairs (which would be consistent with import), but not commits of
> modifications to already-versioned things -- as long as we have a
> legitimate reasonings for our behavior.
OK.
> I have two main reasons for why I think it could be okay for import to
> behave differently than commit:
>
> (1) Auto-props are something each user turns on for him or herself.
> So if you go through the effort of enabling those badboys, an
> import that goes awry eol-style-wise can reasonably be
> considered the user's own fault.
[...]
OK if our behaviour is documented. (Presently, it isn't in the book: the section on svn:eol-style doesn't mention consistency, and auto-props aren't mentioned at all.)
> (2) Import never touches the original file. So if something goes
> wrong with the import, you still have your original data. IIRC,
[...]
Good point.
Thank you for explaining your thoughts. I accept your reasoning that it could be OK for Subversion to repair line endings in some situations and complain in others, if we wanted that. I had not thought of those arguments. Yet, as fas as I know, we still have no reason to want that difference, so we should keep things simple by having the same behaviour in all cases. If the documentation can be simple and yet accurate, that tends to be a good sign.
> All of that said, I don't have strong opinions about the matter. I
For the record, I am interpreting this as meaning that you do not object to r8950 (which causes inconsistencies to be rejected).
> think the majority of the use cases which are interesting to this
> discussion could be considered edge cases in the grand scheme of things.
Indeed they could. Still, the fewer edge case surprises we have, the better.
- Julian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Issue #1756 -- import doesn't handle svn:eol-style or svn:keywords
Posted by "C. Michael Pilato" <cm...@collab.net>.
Julian Foad <ju...@btopenworld.com> writes:
> Oops. I had first read your reply as "I think refusing to repair
> the line endings is fine", and therefore thought you were saying you
> would be happy for me to modify it, and so I did. I'm sorry. I was
> just replying to this mail when I read your words and noticed my
> misunderstanding. Grrr. Now what?
>
> Why do you think repairing the line endings is fine? It is
> inconsistent with "commit". Do you think commit should repair them
> too, or do you think it is right for them to behave differently?
It is perfectly okay for them to behave differently, in my opinion.
In fact, it is perfectly okay for commit itself to behave differently
depending on what's being committed -- we could make commits of "adds"
do repairs (which would be consistent with import), but not commits of
modifications to already-versioned things -- as long as we have a
legitimate reasonings for our behavior.
I have two main reasons for why I think it could be okay for import to
behave differently than commit:
(1) Auto-props are something each user turns on for him or herself.
So if you go through the effort of enabling those badboys, an
import that goes awry eol-style-wise can reasonably be
considered the user's own fault.
Commits of already-versioned files, however, don't advertise
that they have an eol-style setting. Some other user might have
set that property on the files, and the committing user could be
completely unaware of this fact. An automatic repair might
actually damage purposefully-inconsistent line endings, and the
committing user won't know what hit him.
(2) Import never touches the original file. So if something goes
wrong with the import, you still have your original data. IIRC,
Commit, on the other hand, actually replaces the working file
with a de-and-re-translated file upon completion of the commit.
So if it repaired something it wasn't supposed to, you'd have to
retrace your steps to correct the problem.
All of that said, I don't have strong opinions about the matter. I
think the majority of the use cases which are interesting to this
discussion could be considered edge cases in the grand scheme of things.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Issue #1756 -- import doesn't handle svn:eol-style or svn:keywords
Posted by Julian Foad <ju...@btopenworld.com>.
C. Michael Pilato wrote:
> Julian Foad <ju...@btopenworld.com> writes:
>
>>What I didn't say explicitly is that I am wholly in favour of what I
>>believe was the intent of this patch, and was merely pointing out
>>that the patch is not quite right: it should not request repair
>>(which is just a matter of changing a "TRUE" to "FALSE").
>
> The decision to not repair line-endings was admittedly somewhat
> arbitrary. In retrospect, I think repairing the line endings is
> fine.
Oops. I had first read your reply as "I think refusing to repair the line endings is fine", and therefore thought you were saying you would be happy for me to modify it, and so I did. I'm sorry. I was just replying to this mail when I read your words and noticed my misunderstanding. Grrr. Now what?
Why do you think repairing the line endings is fine? It is inconsistent with "commit". Do you think commit should repair them too, or do you think it is right for them to behave differently? Or did you just mean that it is fine for now, because the patch is an improvement over not having the patch, and the "repairing" issue is just a further, minor issue that can be addressed later?
- Julian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Issue #1756 -- import doesn't handle svn:eol-style or svn:keywords
Posted by "C. Michael Pilato" <cm...@collab.net>.
Julian Foad <ju...@btopenworld.com> writes:
> What I didn't say explicitly is that I am wholly in favour of what I
> believe was the intent of this patch, and was merely pointing out
> that the patch is not quite right: it should not request repair
> (which is just a matter of changing a "TRUE" to "FALSE").
The decision to not repair line-endings was admittedly somewhat
arbitrary. In retrospect, I think repairing the line endings is
fine.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Issue #1756 -- import doesn't handle svn:eol-style or svn:keywords
Posted by Julian Foad <ju...@btopenworld.com>.
kfogel@collab.net wrote:
> Philip Martin <ph...@codematters.co.uk> writes:
>
>>Thus the patch fixes the "normal" use case and makes commit and import
>>have the same behaviour. In the "abnormal" case commit and import are
>>different whether or not the patch is applied, the patch merely
>>changes the nature of the difference.
>
> I think Julian's point is that the import now imports something other
> than the file's contents, in such a way that the original contents
> can't be reliably reproduced -- and that 'import' is the operation for
> which such behavior is least appropriate.
>
> I'm not sure I have a strong opinion one way or the other, just trying
> to summarize his position for clarity (or possibly I've got it wrong,
> in which case he'll correct me).
Thanks Karl. That summary is not inconsistent with my reasoning, but is not my main line of argument :-)
What I didn't say explicitly is that I am wholly in favour of what I believe was the intent of this patch, and was merely pointing out that the patch is not quite right: it should not request repair (which is just a matter of changing a "TRUE" to "FALSE"). With that modification, I would be happy with the patch and happy for it to be ported into 1.0.1 if other people think it is appropriate. I won't vote +1 because I haven't tested it, and I still think it has a bit of a flavour of "enhancement" to it, but am now being persuaded that it is pretty much a bug fix and quite an important one, so would vote at least +0.
The reason I voted -1 is because it appeared that nobody had noticed the inconsistency and it looked as if three reviewers were going to agree that the patch is perfect, and then we would merge it and never look at it again.
If someone will change that "TRUE" to "FALSE", I'll remove my veto. I'll do it if you like. Any reason not to?
- Julian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Issue #1756 -- import doesn't handle svn:eol-style or svn:keywords
Posted by kf...@collab.net.
Philip Martin <ph...@codematters.co.uk> writes:
> Thus the patch fixes the "normal" use case and makes commit and import
> have the same behaviour. In the "abnormal" case commit and import are
> different whether or not the patch is applied, the patch merely
> changes the nature of the difference.
I think Julian's point is that the import now imports something other
than the file's contents, in such a way that the original contents
can't be reliably reproduced -- and that 'import' is the operation for
which such behavior is least appropriate.
I'm not sure I have a strong opinion one way or the other, just trying
to summarize his position for clarity (or possibly I've got it wrong,
in which case he'll correct me).
-K
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Issue #1756 -- import doesn't handle svn:eol-style or
svn:keywords
Posted by Philip Martin <ph...@codematters.co.uk>.
Julian Foad <ju...@btopenworld.com> writes:
> I am voting -1 for this, due to the "repair" of line endings. For the
> other reasons mentioned below I would have voted "-0".
>
> A normal commit, via svn_wc_transmit_text_deltas, does not force
> repair of inconsistent line endings, but fails with an error. This
> patch to "import", on the other hand, silently "repairs" inconsistent
> line endings. I think that is the wrong thing for it to do, as I
> don't see any reason for it to behave differently from "commit".
First consider a file with consistent line endings, one that can be
committed. With the patch it can be imported and the result will be
the same as the commit, without the patch the import and commit will
be different.
Next consider a file with inconsistent line endings, one that cannot
be committed. The file can be imported with or without the patch, so
in both cases import is different from commit.
Thus the patch fixes the "normal" use case and makes commit and import
have the same behaviour. In the "abnormal" case commit and import are
different whether or not the patch is applied, the patch merely
changes the nature of the difference.
> This patch is on the fuzzy line between fix and enhancement. Lack of
> this patch only "allows malformed data" in the same way that any
> commit does if "svn:eol-style" and "svn:keywords" are not as intended.
> Behaviour without this patch is to preserve the file contents exactly.
> For these reasons I don't think there is a strong need to put this
> patch in 1.0.1.
Without the patch a Windows user who sets svn:eol-style set to native
via autoprops will get the wrong result when importing files with
consistent native line endings. I'd say this patch is quite clearly a
bug fix, with the side effect of modifying the already buggy behaviour
in a corner case.
--
Philip Martin
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org