From: Omar Polo <op@omarpolo.com>
Subject: Re: avoid allocating too much errors in cmd_info
To: Stefan Sperling <stsp@stsp.name>
Cc: gameoftrees@openbsd.org
Date: Fri, 05 Aug 2022 12:49:54 +0200

Stefan Sperling <stsp@stsp.name> wrote:
> On Fri, Aug 05, 2022 at 12:07:59PM +0200, Omar Polo wrote:
> > functions like got_error_path use `get_custom_err' to get a statically
> > allocated struct got_custom_error that's on static array of errors (used
> > as a ring.)  it's nice and simple, but the drawbacks is that custom
> > errors shouldn't be used "for too long" because they can be overwritten.
> > 
> > One example of such is `got info':
> > 
> > 	% got info $(jot 30)
> > 	work tree: /home/op/w/got
> > 	work tree base commit: 6adac3028e24cdb048ee573633ce056b74a5b360
> > 	work tree path prefix: /
> > 	work tree branch reference: refs/heads/main
> > 	work tree UUID: e873031f-1f7e-42b7-96b4-04afb872b83a
> > 	repository: /home/op/git/got.git
> > 	got: 24: bad path
> > 
> > where i would have expected "got: 1: bad path".  it's a nitpick, but
> > it's also easy to fix.  ok?
> > 
> > diff /home/op/w/got
> > commit - 575dd20f850659c3caa159936f3fa7e88867b8a7
> > path + /home/op/w/got
> > blob - 77ab232b3e3dd99d8500f2e4792976396ecafda9
> > file + got/got.c
> > --- got/got.c
> > +++ got/got.c
> > @@ -13095,8 +13095,7 @@ cmd_info(int argc, char *argv[])
> >  			 * Assume this path will fail. This will be corrected
> >  			 * in print_path_info() in case the path does suceeed.
> >  			 */
> > -			pe->data = (void *)got_error_path(pe->path,
> > -			    GOT_ERR_BAD_PATH);
> > +			pe->data = (void *)got_error(GOT_ERR_BAD_PATH);
> >  		}
> >  		error = got_worktree_path_info(worktree, &paths,
> >  		    print_path_info, &paths, check_cancelled, NULL);
> > @@ -13104,7 +13103,8 @@ cmd_info(int argc, char *argv[])
> >  			goto done;
> >  		TAILQ_FOREACH(pe, &paths, entry) {
> >  			if (pe->data != NULL) {
> > -				error = pe->data; /* bad path */
> > +				error = got_error_path(pe->path,
> > +				    GOT_ERR_BAD_PATH);
> >  				break;
> >  			}
> >  		}
> 
> It's a bit strange that pe->data is now effectively just used as a flag.
> Do we have a way of treating pe->data as an error pointer, and then
> append path information? Maybe something like this would work?
> 
> 	if (pe->data != NULL) {
> 		int code = ((const struct got_error *)pe->data)->code;
> 		error = got_error_fmt(code, "%s", pe->path)
> 		break;
> 	}

sure, but it still smells like a flag to me -- just a bit less than
with my diff tho :)

diff /home/op/w/got
commit - 575dd20f850659c3caa159936f3fa7e88867b8a7
path + /home/op/w/got
blob - 77ab232b3e3dd99d8500f2e4792976396ecafda9
file + got/got.c
--- got/got.c
+++ got/got.c
@@ -13095,8 +13095,7 @@ cmd_info(int argc, char *argv[])
 			 * Assume this path will fail. This will be corrected
 			 * in print_path_info() in case the path does suceeed.
 			 */
-			pe->data = (void *)got_error_path(pe->path,
-			    GOT_ERR_BAD_PATH);
+			pe->data = (void *)got_error(GOT_ERR_BAD_PATH);
 		}
 		error = got_worktree_path_info(worktree, &paths,
 		    print_path_info, &paths, check_cancelled, NULL);
@@ -13104,7 +13103,11 @@ cmd_info(int argc, char *argv[])
 			goto done;
 		TAILQ_FOREACH(pe, &paths, entry) {
 			if (pe->data != NULL) {
-				error = pe->data; /* bad path */
+				const struct got_error *perr;
+
+				perr = (const struct got_error *)pe->data;
+				error = got_error_fmt(perr->code, "%s",
+				    pe->path);
 				break;
 			}
 		}