On 23-05-30 10:40AM, Omar Polo wrote:
> On 2023/05/30 17:45:50 +1000, Mark Jamsek <mark@jamsek.com> wrote:
> > On 23-05-29 06:27PM, Stefan Sperling wrote:
> > > On Mon, May 29, 2023 at 04:56:25PM +0200, Stefan Sperling wrote:
> > > > Like this?
> > > 
> > > Omar pointed out off-list that in this diff I made the mistake of
> > > assigning -1 to a size_t (which is unsigned). So I would prefer to
> > > keep this code as-is to avoid falling into such traps.
> > 
> > Yes, I shouldn't have been so vague in my suggestion and shared a diff
> > instead. I was thinking of using the `if (len == (size_t)-1)` idiom like
> > below to check for asprintf() failure. But now I'm not so sure if
> > keeping its return value just to drop the strlen() calls are
> > a worthwhile optimisation. More often than not 'got rm' takes few
> > arguments so in the rare cases someone is dropping a large directory,
> > walking the string length for each path is fine. And from a cursory
> > look, I don't see this patten used elsewhere in got so for the sake of
> > consistency, it's probably better to keep it as-is.
> 
> just for the sake of the discussion; I'm not a C-lawyer but I think
> just using an `int' (which is what asprintf returns anyway) is cleaner
> and more readable: we're guaranteed it returns the number of bytes or
> -1 if an error occurs, and it fits inside a size_t.

Yes, this is even nicer than what I had in mind!

ok for me. Thanks, op :)

btw, when I had a quick look before, there are a few more instances in
lib/worktree.c that could make this change, too, if you feel like
pulling them into this diff.

> diff /home/op/w/got
> commit - fb307946174c95e32d2048584c6ab1ce24f3ea00
> path + /home/op/w/got
> blob - 9d45bcc89fba03349491b41c260b4fc0c9606531
> file + lib/worktree.c
> --- lib/worktree.c
> +++ lib/worktree.c
> @@ -4477,15 +4477,17 @@ got_worktree_schedule_delete(struct got_worktree *work
>  
>  	TAILQ_FOREACH(pe, paths, entry) {
>  		char *ondisk_status_path;
> +		int len;
>  
> -		if (asprintf(&ondisk_status_path, "%s%s%s",
> +		len = asprintf(&ondisk_status_path, "%s%s%s",
>  		    got_worktree_get_root_path(worktree),
> -		    pe->path[0] == '\0' ? "" : "/", pe->path) == -1) {
> +		    pe->path[0] == '\0' ? "" : "/", pe->path);
> +		if (len == -1) {
>  			err = got_error_from_errno("asprintf");
>  			goto done;
>  		}
>  		sda.status_path = ondisk_status_path;
> -		sda.status_path_len = strlen(ondisk_status_path);
> +		sda.status_path_len = len;
>  		err = worktree_status(worktree, pe->path, fileindex, repo,
>  			schedule_for_deletion, &sda, NULL, NULL, 1, 1);
>  		free(ondisk_status_path);
> 
> 
> > diff /home/mark/src/got
> > commit - fb307946174c95e32d2048584c6ab1ce24f3ea00
> > path + /home/mark/src/got
> > blob - 9d45bcc89fba03349491b41c260b4fc0c9606531
> > file + lib/worktree.c
> > --- lib/worktree.c
> > +++ lib/worktree.c
> > @@ -4477,15 +4477,17 @@ got_worktree_schedule_delete(struct got_worktree *work
> >  
> >  	TAILQ_FOREACH(pe, paths, entry) {
> >  		char *ondisk_status_path;
> > +		size_t len;
> >  
> > -		if (asprintf(&ondisk_status_path, "%s%s%s",
> > +		len = asprintf(&ondisk_status_path, "%s%s%s",
> >  		    got_worktree_get_root_path(worktree),
> > -		    pe->path[0] == '\0' ? "" : "/", pe->path) == -1) {
> > +		    pe->path[0] == '\0' ? "" : "/", pe->path);
> > +		if (len == (size_t)-1) {
> >  			err = got_error_from_errno("asprintf");
> >  			goto done;
> >  		}
> >  		sda.status_path = ondisk_status_path;
> > -		sda.status_path_len = strlen(ondisk_status_path);
> > +		sda.status_path_len = len;
> >  		err = worktree_status(worktree, pe->path, fileindex, repo,
> >  			schedule_for_deletion, &sda, NULL, NULL, 1, 1);
> >  		free(ondisk_status_path);
> > @@ -9860,6 +9862,7 @@ got_worktree_patch_schedule_rm(const char *path, struc
> >  	const struct got_error *err;
> >  	struct schedule_deletion_args sda;
> >  	char *ondisk_status_path;
> > +	size_t len;
> >  
> >  	memset(&sda, 0, sizeof(sda));
> >  	sda.worktree = worktree;
> > @@ -9871,11 +9874,12 @@ got_worktree_patch_schedule_rm(const char *path, struc
> >  	sda.keep_on_disk = 0;
> >  	sda.ignore_missing_paths = 0;
> >  	sda.status_codes = NULL;
> > -	if (asprintf(&ondisk_status_path, "%s/%s",
> > -	    got_worktree_get_root_path(worktree), path) == -1)
> > +	len = asprintf(&ondisk_status_path, "%s/%s",
> > +	    got_worktree_get_root_path(worktree), path);
> > +	if (len == (size_t)-1)
> >  		return got_error_from_errno("asprintf");
> >  	sda.status_path = ondisk_status_path;
> > -	sda.status_path_len = strlen(ondisk_status_path);
> > +	sda.status_path_len = len;
> >  
> >  	err = worktree_status(worktree, path, fileindex, repo,
> >  	    schedule_for_deletion, &sda, NULL, NULL, 1, 1);
> 
> 

-- 
Mark Jamsek <fnc.bsdbox.org|got.bsdbox.org>
GPG: F2FF 13DE 6A06 C471 CA80  E6E2 2930 DC66 86EE CF68