* [9front] git/merge: preserve exec bit correctly
@ 2022-04-16 21:13 ori
2022-04-16 21:46 ` ori
0 siblings, 1 reply; 5+ messages in thread
From: ori @ 2022-04-16 21:13 UTC (permalink / raw)
To: 9front
A while ago, qwx noticed that we clobbered the exec
bit when merging files. This is not what we want, so
we changed the operator precedence to avoid merging
dirty files implicitly.
But we do want to merge, because it's convenient for
maintaining permissions. So, instead, we should do a
3 way merge of the exec bit.
This patch does that, as well as reverting the rollback
of that change.
While we're here, we adjust the timestamps correctly
in git/branch.
This requires changes to git/fs, because without an open
handler, lib9p allows opening any file with any mode,
which confuses 'test -x'.
diff 061ec57021a7c813844582f6f1973dafae6e668b uncommitted
--- a//sys/lib/git/common.rc
+++ b//sys/lib/git/common.rc
@@ -29,13 +29,18 @@
' $*
}
-fn present {
+fn mergeperm {
if(~ $1 /dev/null && cmp $2 $3>/dev/null)
status=gone
if not if (~ $3 /dev/null && cmp $1 $2>/dev/null)
status=gone
- if not
+ if not {
+ if(test -x $2 && {! test -x $1 || ! test -x $3})
+ mergedperms='-x'
+ if not
+ mergedperms='+x'
status=()
+ }
}
fn whoami{
@@ -76,9 +81,10 @@
if(! ape/diff3 -3 -m $ours $base $theirs > $tmp)
echo merge needed: $out >[1=2]
- if(present $ours $base $theirs){
+ if(mergeperm $ours $base $theirs){
mv $tmp $out
git/add $out
+ chmod $mergedperms $out
}
if not {
rm -f $tmp $out
--- a/sys/src/cmd/git/branch
+++ b/sys/src/cmd/git/branch
@@ -48,9 +48,12 @@
modified=`$nl{git/query -c HEAD $base | grep '^[^-]' | subst '^..'}
deleted=`$nl{git/query -c HEAD $base | grep '^-' | subst '^..'}
-if(! ~ $#modified 0 || {! ~ $#deleted 0 && ~ $#merge 0}){
- git/walk -fRMA $modified $deleted ||
- die 'uncommitted changes would be clobbered'
+# if we're not merging, don't clobber existing changes.
+if(~ $#merge 0){
+ if(! ~ $#modified 0 || ! ~ $#deleted 0){
+ git/walk -fRMA $modified $deleted ||
+ die 'uncommitted changes would be clobbered'
+ }
}
if(~ $delete 1){
rm -f .git/$new
@@ -97,10 +100,9 @@
rm -rf .git/index9/tracked/$m
}
if(~ $b file){
- if(cp -x -- $basedir/tree/$m $m)
- walk -eq $m > .git/index9/tracked/$m
- if not
- echo -n > .git/index9/tracked/$m
+ cp -x -- $basedir/tree/$m $m)
+ walk -eq $m > .git/index9/tracked/$m
+ touch $m
}
}
--- a/sys/src/cmd/git/fs.c
+++ b/sys/src/cmd/git/fs.c
@@ -70,13 +70,14 @@
"ctl",
};
-#define Eperm "permission denied";
-#define Eexist "does not exist";
-#define E2long "path too long";
-#define Enodir "not a directory";
-#define Erepo "unable to read repo";
-#define Egreg "wat";
-#define Ebadobj "invalid object";
+#define Eperm "permission denied"
+#define Eexist "does not exist"
+#define E2long "path too long"
+#define Enodir "not a directory"
+#define Erepo "unable to read repo"
+#define Eobject "invalid object"
+#define Egreg "wat"
+#define Ebadobj "invalid object"
char gitdir[512];
char *username;
@@ -624,9 +625,9 @@
e = objwalk1(q, o->obj, o, c, name, Qobject, aux);
}else{
if(hparse(&h, name) == -1)
- return "invalid object name";
+ return Eobject;
if((c->obj = readobject(h)) == nil)
- return "could not read object";
+ return Eobject;
if(c->obj->type == GBlob || c->obj->type == GTag){
c->mode = 0644;
q->type = 0;
@@ -805,6 +806,34 @@
}
static void
+gitopen(Req *r)
+{
+ Gitaux *aux;
+ Crumb *c;
+
+ aux = r->fid->aux;
+ c = crumb(aux, 0);
+ switch(r->ifcall.mode&3){
+ default:
+ respond(r, "botched mode");
+ break;
+ case OWRITE:
+ respond(r, Eperm);
+ break;
+ case OREAD:
+ case ORDWR:
+ respond(r, nil);
+ break;
+ case OEXEC:
+ if((c->mode & 0111) == 0)
+ respond(r, Eperm);
+ else
+ respond(r, nil);
+ break;
+ }
+}
+
+static void
gitstat(Req *r)
{
Gitaux *aux;
@@ -830,6 +859,7 @@
.attach=gitattach,
.walk1=gitwalk1,
.clone=gitclone,
+ .open=gitopen,
.read=gitread,
.stat=gitstat,
.destroyfid=gitdestroyfid,
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [9front] git/merge: preserve exec bit correctly
2022-04-16 21:13 [9front] git/merge: preserve exec bit correctly ori
@ 2022-04-16 21:46 ` ori
2022-04-19 8:46 ` igor
0 siblings, 1 reply; 5+ messages in thread
From: ori @ 2022-04-16 21:46 UTC (permalink / raw)
To: 9front
Quoth ori@eigenstate.org:
> A while ago, qwx noticed that we clobbered the exec
> bit when merging files. This is not what we want, so
> we changed the operator precedence to avoid merging
> dirty files implicitly.
>
> But we do want to merge, because it's convenient for
> maintaining permissions. So, instead, we should do a
> 3 way merge of the exec bit.
>
> This patch does that, as well as reverting the rollback
> of that change.
>
> While we're here, we adjust the timestamps correctly
> in git/branch.
>
> This requires changes to git/fs, because without an open
> handler, lib9p allows opening any file with any mode,
> which confuses 'test -x'.
diff 061ec57021a7c813844582f6f1973dafae6e668b uncommitted
--- a//sys/lib/git/common.rc
+++ b//sys/lib/git/common.rc
@@ -29,13 +29,23 @@
' $*
}
-fn present {
+fn mergep\x02erm {
if(~ $1 /dev/null && cmp $2 $3>/dev/null)
status=gone
if not if (~ $3 /dev/null && cmp $1 $2>/dev/null)
status=gone
- if not
+ if not {
+ mergedperms='-x'
+ if(test -x $2){
+ if(test -x $1 -a -x $3)
+ mergedperms='+x'
+ }
+ if not{
+ if(test -x $1 -o -x $3)
+ mergedperms='+x'
+ }
status=()
+ }
}
fn whoami{
@@ -76,9 +86,10 @@
if(! ape/diff3 -3 -m $ours $base $theirs > $tmp)
echo merge needed: $out >[1=2]
- if(present $ours $base $theirs){
+ if(mergeperm $ours $base $theirs){
mv $tmp $out
git/add $out
+ chmod $mergedperms $out
}
if not {
rm -f $tmp $out
--- a/sys/src/cmd/git/branch
+++ b/sys/src/cmd/git/branch
@@ -48,9 +48,12 @@
modified=`$nl{git/query -c HEAD $base | grep '^[^-]' | subst '^..'}
deleted=`$nl{git/query -c HEAD $base | grep '^-' | subst '^..'}
-if(! ~ $#modified 0 || {! ~ $#deleted 0 && ~ $#merge 0}){
- git/walk -fRMA $modified $deleted ||
- die 'uncommitted changes would be clobbered'
+# if we're not merging, don't clobber existing changes.
+if(~ $#merge 0){
+ if(! ~ $#modified 0 || ! ~ $#deleted 0){
+ git/walk -fRMA $modified $deleted ||
+ die 'uncommitted changes would be clobbered'
+ }
}
if(~ $delete 1){
rm -f .git/$new
@@ -97,10 +100,9 @@
rm -rf .git/index9/tracked/$m
}
if(~ $b file){
- if(cp -x -- $basedir/tree/$m $m)
- walk -eq $m > .git/index9/tracked/$m
- if not
- echo -n > .git/index9/tracked/$m
+ cp -x -- $basedir/tree/$m $m
+ walk -eq $m > .git/index9/tracked/$m
+ touch $m
}
}
--- a/sys/src/cmd/git/fs.c
+++ b/sys/src/cmd/git/fs.c
@@ -70,13 +70,14 @@
"ctl",
};
-#define Eperm "permission denied";
-#define Eexist "does not exist";
-#define E2long "path too long";
-#define Enodir "not a directory";
-#define Erepo "unable to read repo";
-#define Egreg "wat";
-#define Ebadobj "invalid object";
+#define Eperm "permission denied"
+#define Eexist "does not exist"
+#define E2long "path too long"
+#define Enodir "not a directory"
+#define Erepo "unable to read repo"
+#define Eobject "invalid object"
+#define Egreg "wat"
+#define Ebadobj "invalid object"
char gitdir[512];
char *username;
@@ -624,9 +625,9 @@
e = objwalk1(q, o->obj, o, c, name, Qobject, aux);
}else{
if(hparse(&h, name) == -1)
- return "invalid object name";
+ return Eobject;
if((c->obj = readobject(h)) == nil)
- return "could not read object";
+ return Eobject;
if(c->obj->type == GBlob || c->obj->type == GTag){
c->mode = 0644;
q->type = 0;
@@ -805,6 +806,34 @@
}
static void
+gitopen(Req *r)
+{
+ Gitaux *aux;
+ Crumb *c;
+
+ aux = r->fid->aux;
+ c = crumb(aux, 0);
+ switch(r->ifcall.mode&3){
+ default:
+ respond(r, "botched mode");
+ break;
+ case OWRITE:
+ respond(r, Eperm);
+ break;
+ case OREAD:
+ case ORDWR:
+ respond(r, nil);
+ break;
+ case OEXEC:
+ if((c->mode & 0111) == 0)
+ respond(r, Eperm);
+ else
+ respond(r, nil);
+ break;
+ }
+}
+
+static void
gitstat(Req *r)
{
Gitaux *aux;
@@ -830,6 +859,7 @@
.attach=gitattach,
.walk1=gitwalk1,
.clone=gitclone,
+ .open=gitopen,
.read=gitread,
.stat=gitstat,
.destroyfid=gitdestroyfid,
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [9front] git/merge: preserve exec bit correctly
2022-04-16 21:46 ` ori
@ 2022-04-19 8:46 ` igor
2022-04-19 14:18 ` ori
0 siblings, 1 reply; 5+ messages in thread
From: igor @ 2022-04-19 8:46 UTC (permalink / raw)
To: 9front; +Cc: ori, igor
Hi Ori,
I was testing the combination of two patches you sent via the list:
* git/merge: preserve exec bit correctly
* git/send: fill in 'theirs' object, even if we miss it
To exercise the code I used a very large repo with many branches
cloned from an on-site GitLab instance.
When trying to switch between branches using git/branch things fall
over with the below error.
I have done no further debugging so far. Hence, the issue could still
be between keyboard and chair. More debug info to follow off list...
<snip>
term% git/clone git+ssh://some-gitlab-repo.xyz/repo
term% cd repo
term% git/pull
git-upload-pack: Command not found.
up to date
term% git/branch
heads/develop
term% git/branch xxx/yyyREDACTEDyyy
git/query: resolve: invalid ref refs/heads/xxx/yyyREDACTEDyyy
refs/heads/xxx/yyyREDACTEDyyy: 89d2917afd764b48d6fcf5a8b9c25e28da956eb2
term% git/branch
heads/xxx/yyyREDACTEDyyy
term% git/branch develop
assert failed: o->flag & Cloaded
fs 2459926: suicide: sys: trap: fault read addr=0x0 pc=0x2125d9
cp: can't stat /REDACTED/.git/fs/object/31423d28b3eb695f5cb9df954113bfef30f53fe9/tree/REDACTED-PATH/some.file: '/REDACTED/.git/fs/object' mount rpc error
<snap>
Here the stack trace:
<snip>
acid: lstk()
abort()+0x0 /sys/src/libc/9sys/abort.c:6
_assert(s=0x4003a8)+0x42 /sys/src/libc/port/_assert.c:12
clear(o=0x11626fd0)+0x108 /sys/src/cmd/git/pack.c:83
unref()+0x1f /sys/src/cmd/git/pack.c:118
gitdestroyfid()+0x53 /sys/src/cmd/git/fs.c:702
i=0x1162ea300000000c
aux=0x1162ea30
closefid(f=0x11622710)+0x4f /sys/src/lib9p/fid.c:69
closereq(r=0x11626c70)+0x84 /sys/src/lib9p/req.c:79
i=0x21994e00000000
respond(error=0x0,r=0x11626c70)+0x31d /sys/src/lib9p/srv.c:924
srv=0x407b18
errbuf=0x21a36e00000000
n=0x11626c7000000007
i=0x218af200000000
sclunk(r=0x11626c70)+0x56 /sys/src/lib9p/srv.c:590
srvwork()+0x1d2 /sys/src/lib9p/srv.c:764
srv=0x407b18
r=0x11626c70
srv(srv=0x407b18)+0x15b /sys/src/lib9p/srv.c:847
postproc()+0x30 /sys/src/lib9p/post.c:12
s=0x407b18
srvforker(fn=0x21713f,arg=0x407b18,flag=0x9)+0x30 /sys/src/lib9p/rfork.c:17
postsrv(s=0x407b18,name=0x0)+0x112 /sys/src/lib9p/post.c:41
fd=0x400000003
buf=0x4003a800000000
cfd=0x3ffffffff
postmountsrv(name=0x0,mtpt=0x407397,flag=0x4)+0x13 /sys/src/lib9p/mount.c:13
sfd=0x202a3600000000
main(argc=0x0,argv=0x7fffffffefa0)+0xf1 /sys/src/cmd/git/fs.c:905
_argc=0x0
_args=0x0
d=0x40d9e8
_main+0x40 /sys/src/libc/amd64/main9.s:15
acid:
<snap>
HTH
Cheers,
Igor
Quoth ori@eigenstate.org:
> Quoth ori@eigenstate.org:
> > A while ago, qwx noticed that we clobbered the exec
> > bit when merging files. This is not what we want, so
> > we changed the operator precedence to avoid merging
> > dirty files implicitly.
> >
> > But we do want to merge, because it's convenient for
> > maintaining permissions. So, instead, we should do a
> > 3 way merge of the exec bit.
> >
> > This patch does that, as well as reverting the rollback
> > of that change.
> >
> > While we're here, we adjust the timestamps correctly
> > in git/branch.
> >
> > This requires changes to git/fs, because without an open
> > handler, lib9p allows opening any file with any mode,
> > which confuses 'test -x'.
>
> diff 061ec57021a7c813844582f6f1973dafae6e668b uncommitted
> --- a//sys/lib/git/common.rc
> +++ b//sys/lib/git/common.rc
> @@ -29,13 +29,23 @@
> ' $*
> }
>
> -fn present {
> +fn mergep\x02erm {
> if(~ $1 /dev/null && cmp $2 $3>/dev/null)
> status=gone
> if not if (~ $3 /dev/null && cmp $1 $2>/dev/null)
> status=gone
> - if not
> + if not {
> + mergedperms='-x'
> + if(test -x $2){
> + if(test -x $1 -a -x $3)
> + mergedperms='+x'
> + }
> + if not{
> + if(test -x $1 -o -x $3)
> + mergedperms='+x'
> + }
> status=()
> + }
> }
>
> fn whoami{
> @@ -76,9 +86,10 @@
> if(! ape/diff3 -3 -m $ours $base $theirs > $tmp)
> echo merge needed: $out >[1=2]
>
> - if(present $ours $base $theirs){
> + if(mergeperm $ours $base $theirs){
> mv $tmp $out
> git/add $out
> + chmod $mergedperms $out
> }
> if not {
> rm -f $tmp $out
> --- a/sys/src/cmd/git/branch
> +++ b/sys/src/cmd/git/branch
> @@ -48,9 +48,12 @@
> modified=`$nl{git/query -c HEAD $base | grep '^[^-]' | subst '^..'}
> deleted=`$nl{git/query -c HEAD $base | grep '^-' | subst '^..'}
>
> -if(! ~ $#modified 0 || {! ~ $#deleted 0 && ~ $#merge 0}){
> - git/walk -fRMA $modified $deleted ||
> - die 'uncommitted changes would be clobbered'
> +# if we're not merging, don't clobber existing changes.
> +if(~ $#merge 0){
> + if(! ~ $#modified 0 || ! ~ $#deleted 0){
> + git/walk -fRMA $modified $deleted ||
> + die 'uncommitted changes would be clobbered'
> + }
> }
> if(~ $delete 1){
> rm -f .git/$new
> @@ -97,10 +100,9 @@
> rm -rf .git/index9/tracked/$m
> }
> if(~ $b file){
> - if(cp -x -- $basedir/tree/$m $m)
> - walk -eq $m > .git/index9/tracked/$m
> - if not
> - echo -n > .git/index9/tracked/$m
> + cp -x -- $basedir/tree/$m $m
> + walk -eq $m > .git/index9/tracked/$m
> + touch $m
> }
> }
>
> --- a/sys/src/cmd/git/fs.c
> +++ b/sys/src/cmd/git/fs.c
> @@ -70,13 +70,14 @@
> "ctl",
> };
>
> -#define Eperm "permission denied";
> -#define Eexist "does not exist";
> -#define E2long "path too long";
> -#define Enodir "not a directory";
> -#define Erepo "unable to read repo";
> -#define Egreg "wat";
> -#define Ebadobj "invalid object";
> +#define Eperm "permission denied"
> +#define Eexist "does not exist"
> +#define E2long "path too long"
> +#define Enodir "not a directory"
> +#define Erepo "unable to read repo"
> +#define Eobject "invalid object"
> +#define Egreg "wat"
> +#define Ebadobj "invalid object"
>
> char gitdir[512];
> char *username;
> @@ -624,9 +625,9 @@
> e = objwalk1(q, o->obj, o, c, name, Qobject, aux);
> }else{
> if(hparse(&h, name) == -1)
> - return "invalid object name";
> + return Eobject;
> if((c->obj = readobject(h)) == nil)
> - return "could not read object";
> + return Eobject;
> if(c->obj->type == GBlob || c->obj->type == GTag){
> c->mode = 0644;
> q->type = 0;
> @@ -805,6 +806,34 @@
> }
>
> static void
> +gitopen(Req *r)
> +{
> + Gitaux *aux;
> + Crumb *c;
> +
> + aux = r->fid->aux;
> + c = crumb(aux, 0);
> + switch(r->ifcall.mode&3){
> + default:
> + respond(r, "botched mode");
> + break;
> + case OWRITE:
> + respond(r, Eperm);
> + break;
> + case OREAD:
> + case ORDWR:
> + respond(r, nil);
> + break;
> + case OEXEC:
> + if((c->mode & 0111) == 0)
> + respond(r, Eperm);
> + else
> + respond(r, nil);
> + break;
> + }
> +}
> +
> +static void
> gitstat(Req *r)
> {
> Gitaux *aux;
> @@ -830,6 +859,7 @@
> .attach=gitattach,
> .walk1=gitwalk1,
> .clone=gitclone,
> + .open=gitopen,
> .read=gitread,
> .stat=gitstat,
> .destroyfid=gitdestroyfid,
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [9front] git/merge: preserve exec bit correctly
2022-04-19 8:46 ` igor
@ 2022-04-19 14:18 ` ori
2022-04-26 6:10 ` igor
0 siblings, 1 reply; 5+ messages in thread
From: ori @ 2022-04-19 14:18 UTC (permalink / raw)
To: 9front; +Cc: igor, ori
Quoth igor@9lab.org:
> When trying to switch between branches using git/branch things fall
> over with the below error.
>
> I have done no further debugging so far. Hence, the issue could still
> be between keyboard and chair. More debug info to follow off list...
A suicide is definitely not between keyboard and chair :)
What would be useful would be a 9p trace of the messages;
I'd like to know what we cloned the fid from, and what we
tried to do with it before clunking.
git/fs -d
will print a 9p trace.
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [9front] git/merge: preserve exec bit correctly
2022-04-19 14:18 ` ori
@ 2022-04-26 6:10 ` igor
0 siblings, 0 replies; 5+ messages in thread
From: igor @ 2022-04-26 6:10 UTC (permalink / raw)
To: 9front; +Cc: igor, ori
Quoth ori@eigenstate.org:
> Quoth igor@9lab.org:
> > When trying to switch between branches using git/branch things fall
> > over with the below error.
> >
> > I have done no further debugging so far. Hence, the issue could still
> > be between keyboard and chair. More debug info to follow off list...
>
> A suicide is definitely not between keyboard and chair :)
>
> What would be useful would be a 9p trace of the messages;
> I'd like to know what we cloned the fid from, and what we
> tried to do with it before clunking.
>
> git/fs -d
>
> will print a 9p trace.
Confirmed that the issue exists without applying the patches. Hence
this is an existing issue and not related to the changes you asked
to be tested.
Will try to debug further in the course of next week.
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2022-04-26 6:12 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-04-16 21:13 [9front] git/merge: preserve exec bit correctly ori
2022-04-16 21:46 ` ori
2022-04-19 8:46 ` igor
2022-04-19 14:18 ` ori
2022-04-26 6:10 ` igor
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).