Julian Foad wrote on Wed, Jul 29, 2015 at 16:22:27 +0100:
> 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.)
>
At a high level, I'd like it to be possible to do port a diff that
represents "add an executable file" from git to svn:
'echo contents > file; chmod +x file; ...; git diff; svn patch'.
hg can produce git diffs too, so it should be possible to use hg instead
of git in that scenario, too.
> 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.
>
Okay, but what are the practical implications of this today? 'svn
patch' does not yet support non-wc targets¹. Does the possibility of
supporting non-wc targets in the future affect the design today? It
certainly doesn't affect parse-diff.c, but do you see any concerns for
the libsvn_client-level code?
¹ According to the first sentence of the help text of 'svn patch'.
> >> +- [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?
>
It should not be possible to generate a patch in the first place.
If we see such a thing on input, I would generate a warning or an error
(or a rejected propchange hunk), and not attempt to choose between the
two conflicting directives.
It is not an error for a patch to have an svn:executable propdiff hunk
but no git mode lines, or vice-versa: svn 1.9 gnerates the former and
git/hg generate the latter.
> >> +- [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.
"Be liberal in what you accept" tends to encourage buggy producers.
I generally prefer being strict in what I accept, and adding relaxations
as necessary later (when compelling reasons arise).
In this case, we have forward compatbility to consider: if svn 1.10
considers 0600 as equivalent to 0644, and subsequently git will define
a distinction between those two values, svn 1.11 will not be able to
interpret the value "0600" in a manner compatible with both svn 1.10 and
then-current git. We can avoid this problem by making svn 1.10 at least
as strict in its parsing as git is.
> 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
For now, let's require that the three bits 0111 are either all set or
all unset. The "git patch format" only defines 0644 and 0755 as valid
values, and we have no need for finer distinctions than that, so let's
accept just those two values and reject everything else. When we need
to make finer distinctions, we can relax the parser to accept additional
values.
> 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.
>
Agreed, I don't think we should attempt to represent what a Unix
filesystem can but what a Subversion filesystem can.
> If you can find any useful documentation about this on line, please link to it.
>
Git internally only distinguishes between 0644 and 0755:
https://github.com/git/git/blob/master/Documentation/git-fast-import.txt#L546
https://github.com/git/git/blob/master/Documentation/user-manual.txt#L3074
I couldn't find docs about whether it checks the S_IXUSR (0100) bit
specifically, or follows the normal rules of access(_, X_OK), or
something else.
>
As those links say, git modes have 18 bits, not nine. The high nine
bits are the setuid/setgid/sticky bits and six additional bits, which
are used to represent file type (eg symlink). Perhaps we should be
doing something with those nine bits.
As you say — I think we need a simple rule of thumb to determine what
kind of changes we are going to support (or not to support). "Support
what a Subversion filesystem can represent" seems like a good candidate.
It implies we should attempt to DTRT with symlinks and directories
(symlinks certainly seem like a nice enhancement). However, should we
be attempting to reject unknown values? For example, should we check
tha the high nine bits are either 0100 (plain file) or 0120 (symlink),
and report a warning/error/rejected hunk if they have any other value?
> >> +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.)
Currently, we print propdiffs in svn notation, git should ignore the
propdiffs as garbage:
% svn ps -q k v README
% svn diff --git -N
Index: README
===================================================================
diff --git a/subversion/site/README b/subversion/site/README
--- a/subversion/site/README (revision 1662576)
+++ b/subversion/site/README (working copy)
Property changes on: subversion/site/README
___________________________________________________________________
Added: k
## -0,0 +1 ##
+v
\ No newline at end of property
So, okay, agreed that we could print the "mode" lines as well.
Consumers who don't recognize "mode" lines will ignore them as garbage,
and we'll have added interoperability with consumers who do. Our own
parser will just have to check that the "mode" lines, if present, and
the propdiffs match.
Properties don't have an equivalent in git (there is gitattributes(5)
which is comparable but not identical).
> 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.
Two questions:
- When one side of the diff is in the OS filesystem, do we still fold
its value to 644/755 for output?
- If yes, how do we choose between 644 and 755? (e.g., do we use
"x & 0111 == 0111", or "x & 0100 == 0100", or access(X_OK), or …)
My answer to the first question is "yes", as discussed above.
> - Julian
>
Thanks for the feedback, Julian!
Daniel
Received on 2015-08-01 01:38:57 CEST