From: Stefan Sperling Subject: Re: got add recursion To: Tracey Emery Cc: gameoftrees@openbsd.org Date: Fri, 22 Nov 2019 11:23:46 +0100 On Thu, Nov 21, 2019 at 04:28:41PM -0700, Tracey Emery wrote: > Hello, > > Below is a diff to add recursion to got add. Regress passes. Could you also add a test for new add -R behaviour? > Ok? Better way to do this? Anything I missed? Some comments below: > diff refs/heads/master refs/heads/recurse > blob - 9320a0971ee4f04fb9c3ff5cda31e6ceae91d2ea > blob + 9c0c3772a7c0a876c6e1fc5ae868fe928503cb90 > --- got/got.1 > +++ got/got.1 > @@ -619,9 +619,22 @@ If a tag must be deleted, the > command may be used to delete a tag's reference. > This should only be done if the tag has not already been copied to > another repository. > -.It Cm add Ar file-path ... > +.It Cm add Oo Fl R Oc Ar path ... > Schedule unversioned files in a work tree for addition to the > repository in the next commit. > +.Pp > +The options for > +.Cm got add > +are as follows: > +.Bl -tag -width Ds > +.It Fl R > +Permit recursion into directories. > +If this option is not specified, > +.Cm got add > +will refuse to run if a specified > +.Ar path > +is a directory. > +.El > .It Cm remove Ar file-path ... > Remove versioned files from a work tree and schedule them for deletion > from the repository in the next commit. > blob - 3b8935895b310dbf4148cef5f6f4b5722e0369ba > blob + 776fb4c2ed06b1f5eaa6573a02ff26a086f3701b > --- got/got.c > +++ got/got.c > @@ -4086,6 +4086,15 @@ usage_add(void) > } > > static const struct got_error * > +add_progress(void *arg, unsigned char status, const char *path) > +{ > + while (path[0] == '/') > + path++; > + printf("%c %s\n", status, path); > + return NULL; > +} > + > +static const struct got_error * > cmd_add(int argc, char *argv[]) > { > const struct got_error *error = NULL; > @@ -4094,12 +4103,15 @@ cmd_add(int argc, char *argv[]) > char *cwd = NULL; > struct got_pathlist_head paths; > struct got_pathlist_entry *pe; > - int ch; > + int ch, can_recurse = 0; > > TAILQ_INIT(&paths); > > - while ((ch = getopt(argc, argv, "")) != -1) { > + while ((ch = getopt(argc, argv, "R")) != -1) { > switch (ch) { > + case 'R': > + can_recurse = 1; > + break; > default: > usage_add(); > /* NOTREACHED */ > @@ -4141,7 +4153,35 @@ cmd_add(int argc, char *argv[]) > if (error) > goto done; > > - error = got_worktree_schedule_add(worktree, &paths, print_status, > + if (!can_recurse) { > + char *ondisk_path; > + struct stat sb; > + TAILQ_FOREACH(pe, &paths, entry) { > + if (asprintf(&ondisk_path, "%s/%s", > + got_worktree_get_root_path(worktree), > + pe->path) == -1) { > + error = got_error_from_errno("asprintf"); > + goto done; > + } > + if (lstat(ondisk_path, &sb) == -1) { > + if (errno == ENOENT) { > + free(ondisk_path); > + continue; > + } > + error = got_error_from_errno2("lstat", > + ondisk_path); > + free(ondisk_path); > + goto done; > + } > + free(ondisk_path); > + if (S_ISDIR(sb.st_mode)) { > + error = got_error_msg(GOT_ERR_BAD_PATH, > + "adding directories requires -R option"); > + goto done; > + } > + } > + } > + error = got_worktree_schedule_add(worktree, &paths, add_progress, > NULL, repo); > done: > if (repo) > blob - 336553780903efcf81551b9db8c4a76a0ab2f148 > blob + a18fc5a59d6c2c165d6f449c5accb262eb450485 > --- include/got_worktree.h > +++ include/got_worktree.h > @@ -158,7 +158,7 @@ const struct got_error *got_worktree_resolve_path(char > > /* Schedule files at on-disk paths for addition in the next commit. */ > const struct got_error *got_worktree_schedule_add(struct got_worktree *, > - struct got_pathlist_head *, got_worktree_status_cb, void *, > + struct got_pathlist_head *, got_worktree_checkout_cb, void *, > struct got_repository *); > > /* > blob - e841520c6fbc877de575ee04d69018206ea0391f > blob + 7d7f8b1c558ff0f39077411e8d0fd396ebae72cd > --- lib/worktree.c > +++ lib/worktree.c > @@ -2563,7 +2563,7 @@ add_ignores(struct got_pathlist_head *ignores, const c > err = read_ignores(ignores, path, ignoresfile); > > if (ignoresfile && fclose(ignoresfile) == EOF && err == NULL) > - err = got_error_from_errno2("flose", path); > + err = got_error_from_errno2("fclose", path); > free(ignorespath); > return err; > } > @@ -2771,50 +2771,83 @@ done: > return err; > } > > +struct schedule_addition_args { > + struct got_worktree *worktree; > + struct got_fileindex *fileindex; > + const char *ondisk_path; > + got_worktree_checkout_cb progress_cb; > + void *progress_arg; > + struct got_repository *repo; > +}; > + > static const struct got_error * > -schedule_addition(const char *ondisk_path, struct got_fileindex *fileindex, > - const char *relpath, got_worktree_status_cb status_cb, void *status_arg, > - struct got_repository *repo) > +schedule_addition(void *arg, unsigned char status, unsigned char staged_status, > + const char *relpath, struct got_object_id *blob_id, > + struct got_object_id *staged_blob_id, struct got_object_id *commit_id) > { > + struct schedule_addition_args *a = arg; > const struct got_error *err = NULL; > struct got_fileindex_entry *ie; > - unsigned char status; > struct stat sb; > + char *path = NULL; > > - ie = got_fileindex_entry_get(fileindex, relpath, strlen(relpath)); > + ie = got_fileindex_entry_get(a->fileindex, relpath, strlen(relpath)); > if (ie) { > - err = get_file_status(&status, &sb, ie, ondisk_path, repo); > + err = get_file_status(&status, &sb, ie, a->ondisk_path, > + a->repo); > if (err) > - return err; > + goto done; This can return directly because 'path' is not allocated yet, so free(path) will be a free(NULL) no-op. Also, the code below is in the same block and still returns directly: > /* Re-adding an existing entry is a no-op. */ > if (status == GOT_STATUS_ADD) > return NULL; > return got_error_path(relpath, GOT_ERR_FILE_STATUS); > } > > - err = got_fileindex_entry_alloc(&ie, ondisk_path, relpath, NULL, NULL); > + if (status == GOT_STATUS_UNVERSIONED) { > + if (lstat(a->ondisk_path, &sb) != 0) { > + err = got_error_from_errno2("lstat", a->ondisk_path); > + goto done; Could also return got_error_from_errno2 directly. > + } > + if (S_ISDIR(sb.st_mode)) { > + if (asprintf(&path, "%s/%s", > + got_worktree_get_root_path(a->worktree), > + relpath) == -1) { > + err = got_error_from_errno("asprintf"); > + goto done; Same. > + } > + } else > + path = strdup(a->ondisk_path); > + } How path has been allocated so we need 'goto done' in case of errors to prevent a memory leak. > + > + err = got_fileindex_entry_alloc(&ie, path, relpath, NULL, NULL); > if (err) > - return err; > + goto done; > > - err = got_fileindex_entry_add(fileindex, ie); > + err = got_fileindex_entry_add(a->fileindex, ie); > if (err) { > got_fileindex_entry_free(ie); > - return err; > + goto done; > } > > - return report_file_status(ie, ondisk_path, status_cb, status_arg, repo); > + free(path); > + return (*a->progress_cb)(a->progress_arg, > + GOT_STATUS_ADD, relpath); > +done: > + free(path); > + return err; > } > > const struct got_error * > got_worktree_schedule_add(struct got_worktree *worktree, > struct got_pathlist_head *paths, > - got_worktree_status_cb status_cb, void *status_arg, > + got_worktree_checkout_cb progress_cb, void *progress_arg, > struct got_repository *repo) > { > struct got_fileindex *fileindex = NULL; > char *fileindex_path = NULL; > const struct got_error *err = NULL, *sync_err, *unlockerr; > struct got_pathlist_entry *pe; > + struct schedule_addition_args saa; > > err = lock_worktree(worktree, LOCK_EX); > if (err) > @@ -2824,13 +2857,20 @@ got_worktree_schedule_add(struct got_worktree *worktre > if (err) > goto done; > > + saa.worktree = worktree; > + saa.fileindex = fileindex; > + saa.progress_cb = progress_cb; > + saa.progress_arg = progress_arg; > + saa.repo = repo; > + > TAILQ_FOREACH(pe, paths, entry) { > char *ondisk_path; > if (asprintf(&ondisk_path, "%s/%s", worktree->root_path, > pe->path) == -1) > return got_error_from_errno("asprintf"); > - err = schedule_addition(ondisk_path, fileindex, pe->path, > - status_cb, status_arg, repo); > + saa.ondisk_path = ondisk_path; > + err = worktree_status(worktree, pe->path, fileindex, repo, > + schedule_addition, &saa, NULL, NULL); > free(ondisk_path); > if (err) > break; > @@ -2838,6 +2878,7 @@ got_worktree_schedule_add(struct got_worktree *worktre > sync_err = sync_fileindex(fileindex, fileindex_path); > if (sync_err && err == NULL) > err = sync_err; The goto below seems pointless, it just jumps one line of code ahead: > + goto done; > done: > free(fileindex_path); > if (fileindex) > >