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

[igor@9lab.org]

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