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