From: Stefan Sperling <stsp@stsp.name>
Subject: Re: Loss of original errors
To: Christian Weisgerber <naddy@mips.inka.de>
Cc: gameoftrees@openbsd.org
Date: Wed, 12 Jul 2023 23:02:37 +0200

On Wed, Jul 12, 2023 at 09:21:18PM +0200, Christian Weisgerber wrote:
> Stefan Sperling:
> 
> >  	err = got_path_mkdir(abspath);
> > -	if (err && err->code == GOT_ERR_ERRNO && errno == EEXIST) {
> > -		struct stat sb;
> > -		err = NULL;
> > -		if (lstat(abspath, &sb) == -1) {
> > -			err = got_error_from_errno2("lstat", abspath);
> > -		} else if (!S_ISDIR(sb.st_mode)) {
> > -			/* TODO directory is obstructed; do something */
> > -			err = got_error_path(abspath, GOT_ERR_FILE_OBSTRUCTED);
> > -		}
> > +	if (err)
> > +		goto done;
> > +
> > +	if (lstat(abspath, &sb) == -1) {
> > +		err = got_error_from_errno2("lstat", abspath);
> > +	} else if (!S_ISDIR(sb.st_mode)) {
> > +		/* TODO directory is obstructed; do something */
> > +		err = got_error_path(abspath, GOT_ERR_FILE_OBSTRUCTED);
> >  	}
> 
> That doesn't look right.  If we successfully create a new directory,
> we'll return an error that the path was obstructed.

But we only return such an error if the path is not a directory...?

Anyway, here is a better fix we can use. The root cause of all these
mkdir calls is work tree code which uses a diff walker to compare
the repository to the file index. It attempts to create every directory
that exists in the repository tree, paying no consideration to their
existence on disk.

Instead, let functions which add files worry about creating any missing
parent directories. This shakes out two other bugs in broken symlink
installation code which ended up passing an absolute path to
add_dir_on_disk() and forgot to report progress in an unreachable code
path that is now getting used.

Ok?

diff /home/stsp/src/got
commit - ad6dd0bb6c5ebeafaa57204a04330df6658c3861
path + /home/stsp/src/got
blob - 20e319355137e3ec594f1a28760412c19ab58905
file + lib/worktree.c
--- lib/worktree.c
+++ lib/worktree.c
@@ -397,6 +397,13 @@ add_dir_on_disk(struct got_worktree *worktree, const c
 	const struct got_error *err = NULL;
 	char *abspath;
 
+	/* We only accept worktree-relative paths here. */
+	if (got_path_is_absolute(path)) {
+		return got_error_fmt(GOT_ERR_BAD_PATH,
+		    "%s does not accept absolute paths: %s",
+		    __func__, path);
+	}
+
 	if (asprintf(&abspath, "%s/%s", worktree->root_path, path) == -1)
 		return got_error_from_errno("asprintf");
 
@@ -1350,7 +1357,7 @@ install_symlink(int *is_bad_symlink, struct got_worktr
 			err = got_path_dirname(&parent, ondisk_path);
 			if (err)
 				return err;
-			err = add_dir_on_disk(worktree, parent);
+			err = got_path_mkdir(parent);
 			free(parent);
 			if (err)
 				return err;
@@ -1360,6 +1367,12 @@ install_symlink(int *is_bad_symlink, struct got_worktr
 			 */
 			if (symlink(target_path, ondisk_path) != -1) {
 				err = NULL; /* success */
+				if (progress_cb) {
+					err = (*progress_cb)(progress_arg,
+					    reverting_versioned_file ?
+					    GOT_STATUS_REVERT : GOT_STATUS_ADD,
+					    path);
+				}
 				return err;
 			}
 		}
@@ -2294,16 +2307,16 @@ diff_new(void *arg, struct got_tree_entry *te, const c
 	if (got_object_tree_entry_is_submodule(te))
 		return NULL;
 
+	if (!S_ISREG(te->mode) && !S_ISLNK(te->mode))
+		return NULL;
+
 	if (asprintf(&path, "%s%s%s", parent_path,
 	    parent_path[0] ? "/" : "", te->name)
 	    == -1)
 		return got_error_from_errno("asprintf");
 
-	if (S_ISDIR(te->mode))
-		err = add_dir_on_disk(a->worktree, path);
-	else
-		err = update_blob(a->worktree, a->fileindex, NULL, te, path,
-		    a->repo, a->progress_cb, a->progress_arg);
+	err = update_blob(a->worktree, a->fileindex, NULL, te, path,
+	    a->repo, a->progress_cb, a->progress_arg);
 
 	free(path);
 	return err;