[9front] git/merge: preserve exec bit correctly

[9front] git/merge: preserve exec bit correctly

[ori]

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,

Re: [9front] git/merge: preserve exec bit correctly

[ori]

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,

Re: [9front] git/merge: preserve exec bit correctly

[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,
> 

Re: [9front] git/merge: preserve exec bit correctly

[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.

Re: [9front] git/merge: preserve exec bit correctly

[igor]

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.