On Fri, Apr 04, 2025 at 03:22:02AM +0000, James Cook wrote:
> > > The below patch explicitly checks whether the ref is a directory.
> > > Maybe this should go in non-portable got too, at least to keep
> > > the codebases more similar.
> > 
> > Could you change the S_ISDIR check to !S_ISREG? That would also cover
> > device nodes, sockets, fifos, etc.
> > ok stsp@ with that change, provided it still works as expected for
> > you and the tests are passing.
> 
> Done. Should I push the same patch to got-portable too? I'm not
> sure what the process is.
> 
> (got-portable regress is already broken on NetBSD, so I haven't
> tried that, but I'm using got-portable with the change.)

Please ask Thomas first.  He usually handles syncing between got.git
and got-portable.git with a script.
 
> > > (By the way, I noticed that if fstat fails, parse_ref_file doesn't
> > > call get_lockfile_unlock in the cleanup path. Is that an oversight,
> > > or is it that way because we know the program's going to stop soon
> > > anyway, or is it something else? Every other "goto done;" unlocks,
> > > so if it's okay to unlock in that case too, the unlock could be
> > > moved to "done:" to make the code a bit tidier.)
> > 
> > Looks like an oversight to me. Could you write a fix for this, too?
> > 
> > Thanks!
> 
> See patch below.
> 
> Moving it to "done:" is more complicated than I thought, because
> sometimes the lock is kept when there's no error.

ok by me. I agree that this fix seems simpler and good enough.

> diff /home/falsifian/co/got
> path + /home/falsifian/co/got
> commit - e896de0dbd9971fbc7969f0df170b4a495b4cd28
> blob - 90462626d8b4280e7a38aa3e03f64545013c53f6
> file + lib/reference.c
> --- lib/reference.c
> +++ lib/reference.c
> @@ -206,6 +206,8 @@ parse_ref_file(struct got_reference **ref, const char 
>  	}
>  	if (fstat(fileno(f), &sb) == -1) {
>  		err = got_error_from_errno2("fstat", abspath);
> +		if (lock)
> +			got_lockfile_unlock(lf, -1);
>  		goto done;
>  	}
>  	if (!S_ISREG(sb.st_mode)) {
>