From: Mark Jamsek Subject: Re: got: rm * removes current directory To: Omar Polo Cc: Mikhail , gameoftrees@openbsd.org Date: Tue, 30 May 2023 19:18:51 +1000 On 23-05-30 10:40AM, Omar Polo wrote: > On 2023/05/30 17:45:50 +1000, Mark Jamsek 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 GPG: F2FF 13DE 6A06 C471 CA80 E6E2 2930 DC66 86EE CF68