Daniel Shahaf wrote:
> In a nutshell, the purpose of the branch is to make 'svn patch' set
> svn:exeutable when processing a patch such as:
>
> diff --git a/iota b/iota
> old mode 100644
> new mode 100755
> The unit tests changes (in parse-diff-test.c and patch_tests.py) give
> a full example of the new functionality.
> It seems pretty straightforward so far, but I wanted to see if anyone
> had comments on the idea or the implementation before I took it any
> further.
>
> Basically, does this sound like a good idea, and if so is the
> implementation going in the right direction, etc.
Basically it does sound good. But what behaviour are you planning,
exactly? (Your tests don't tell me.)
First, I would like 'svn patch' in general to be able to apply non-svn
changes to unversioned files, and setting the executable bit(s) from
the 'mode' line should qualify there.
>> +- [TODO] Write tests for adding/remove svn:executable using the normal
>> + 'svn diff' property add/removal syntax
How will this mode metadata interact with the svn:executable property?
What should happen if a patch adds the 'executable' bit on the file
mode but also removes the svn:executable property, or vice-versa, or
adds a new file with them mismatching?
>> +- [TODO] parser: Review handling of modes other than 0644/0755
>> + Consider checking "mode & 0111"
I wouldn't look only for the two particular magic values '100644' and
'100755' unless there's some very good reason why we should, even if
that's what git does. Instead I'd go for a bit more flexibility, on
the basis that some other users of this git patch format may do so
sooner or later. At least look for any valid octal string of three or
more digits, and extract only the three 'executable' bits. Then what?
Require that the three remaining bits are identical (warn or error if
not), or look only at the 'user' executable bit, or use all three bits
as they are if we're on a Unixy file system, or ...? It depends partly
on whether we think the purpose of these lines is to convey versioned
information consisting of a single 'executable' bit or of three bits.
One thing I can suggest is that we should be designing for the
conveyance of *versioned information* rather than for the *potential*
use of these lines to convey actual Unixy permissions of local files.
If you can find any useful documentation about this on line, please link to it.
>> +Optional:
>> +
>> +- [TODO] Should 'svn diff --git' be taught to emit the new form?
>> + (possibly in addition to the stadard propchange form used for all
>> + user properties, for compatibility with released 'svn patch' versions)
It's not a question of "should it?" but "in what cases should it?"
When diffing repository files, the only metadata available is the
svn:executable property, and we're asking whether to convert that to
'mode' lines, and if so whether to also keep the property diff. (Do we
currently have any way to output a property diff in --git mode at all?
If not, or when we're not doing so, then certainly we should emit the
'mode' lines instead.)
When diffing WC files, then potentially we can generate 'mode' lines
as well as an svn:executable property diff (potentially, if/when we
have a way to do so).
When diffing unversioned local files, then certainly we should emit
'mode' lines.
But a diff is between two files. They might be (a: repo, b: WC) or (a:
repo, b: unversioned) or (a: WC, b: unversioned). Then what? We need a
simple high-level rule to tell us what we should expect in all cases.
Perhaps: we always output a mode line, and its value comes from the
presence or absence of the svn:executable property if it's a versioned
file, else from the OS filesystem.
- Julian
Received on 2015-07-29 17:22:51 CEST