From: Omar Polo Subject: Re: gotwebd: linkables lines in blame page To: Tracey Emery Cc: gameoftrees@openbsd.org Date: Tue, 09 Aug 2022 22:46:23 +0200 Omar Polo wrote: > Tracey Emery wrote: > > On Tue, Aug 09, 2022 at 04:48:04PM +0200, Omar Polo wrote: > > > P.S.: what do you think if we introduce something a printf-like > > > fcgi_gen_responsef? > > > > > > > Can you expound a bit here? What would it be used for? If it is used for > > something like this, I think it would be a good idea and reduce lines: > > > > fcgi_gen_responsef(c, "%s", out_buff, out_buff); > > > > Is that what you mean? > > Yep, that's exactly what i had in mind. It could get rid of long > sequences of code and make it easier to follow. > > I'll try to add it and see how it goes. > > > Thanks, Here's a draft for it. The first `fcgi_printf' version i wrote tried to snprintf in the output buffer first and falled back to asprintf as a second choice only when the produced string was bigger than the buffer. It wasn't worth the complexity thought, so here's a simpler version that just builds a string with asprintf(3) and then feeds it to fcgi_gen_binary_response. I doubt this will become a bottleneck. I've started also to simplify some bits in got_operations.c to show how it's like to have this function available; I've searched for the bits that generates the links as they're one of the most pleasing part to simplify. (truth to be told, we could also refactor the code to generate a link into a separate function.) I'm not 100% sure if it's correct, so please take a careful look. The main difference is that fcgi_gen_response is a no-op when the given string is NULL or "" but fcgi_printf, since relies on vasprintf, turns NULL into "(null)". I'm not sure of exactly which fields may be NULL and which will never be. Thanks, Omar Polo diff /home/op/w/got commit - bf80b15220f51490025e916633cdd70816113604 path + /home/op/w/got blob - f6cb81448f5778d7872b537fd6b90838285c99ee file + gotwebd/fcgi.c --- gotwebd/fcgi.c +++ gotwebd/fcgi.c @@ -25,6 +25,7 @@ #include #include #include +#include #include #include #include @@ -308,6 +309,27 @@ fcgi_timeout(int fd, short events, void *arg) } int +fcgi_printf(struct request *c, const char *fmt, ...) +{ + va_list ap; + char *str; + int r; + + va_start(ap, fmt); + r = vasprintf(&str, fmt, ap); + va_end(ap); + + if (r == -1) { + log_warn("%s: asprintf", __func__); + return -1; + } + + r = fcgi_gen_binary_response(c, str, r); + free(str); + return r; +} + +int fcgi_gen_binary_response(struct request *c, const uint8_t *data, int len) { int r; blob - 1c274b225d0e710d16528c4b9970348bb3bec74e file + gotwebd/got_operations.c --- gotwebd/got_operations.c +++ gotwebd/got_operations.c @@ -829,10 +829,11 @@ got_output_repo_tree(struct request *c) struct got_reflist_head refs; struct got_tree_object *tree = NULL; struct repo_dir *repo_dir = t->repo_dir; + const char *name, *index_page_str, *folder; char *id_str = NULL; - char *path = NULL, *in_repo_path = NULL, *build_folder = NULL; - char *modestr = NULL, *name = NULL; - int nentries, i; + char *path = NULL, *in_repo_path = NULL; + char *modestr = NULL; + int nentries, i, r; TAILQ_INIT(&refs); @@ -871,6 +872,9 @@ got_output_repo_tree(struct request *c) nentries = got_object_tree_get_nentries(tree); + index_page_str = qs->index_page_str ? qs->index_page_str : ""; + folder = qs->folder ? qs->folder : ""; + for (i = 0; i < nentries; i++) { struct got_tree_entry *te; mode_t mode; @@ -917,53 +921,23 @@ got_output_repo_tree(struct request *c) } } - name = strdup(got_tree_entry_get_name(te)); - if (name == NULL) { - error = got_error_from_errno("strdup"); - goto done; - } if (S_ISDIR(mode)) { - if (asprintf(&build_folder, "%s/%s", - qs->folder ? qs->folder : "", - got_tree_entry_get_name(te)) == -1) { - error = got_error_from_errno("asprintf"); - goto done; - } - if (fcgi_gen_response(c, "
\n") == -1) - goto done; + goto done; if (fcgi_gen_response(c, "
") == -1) goto done; - if (fcgi_gen_response(c, "%s%s", + index_page_str, qs->path, rc->commit_id, + folder, name, name, modestr); + if (r == -1) goto done; - if (fcgi_gen_response(c, qs->index_page_str) == -1) - goto done; - if (fcgi_gen_response(c, "&path=") == -1) - goto done; - if (fcgi_gen_response(c, qs->path) == -1) - goto done; - if (fcgi_gen_response(c, "&action=tree") == -1) - goto done; - if (fcgi_gen_response(c, "&commit=") == -1) - goto done; - if (fcgi_gen_response(c, rc->commit_id) == -1) - goto done; - if (fcgi_gen_response(c, "&folder=") == -1) - goto done; - if (fcgi_gen_response(c, build_folder) == -1) - goto done; - if (fcgi_gen_response(c, "'>") == -1) - goto done; - if (fcgi_gen_response(c, name) == -1) - goto done; - if (fcgi_gen_response(c, modestr) == -1) - goto done; - if (fcgi_gen_response(c, "") == -1) - goto done; if (fcgi_gen_response(c, "
\n") == -1) goto done; @@ -979,144 +953,44 @@ got_output_repo_tree(struct request *c) goto done; } else { - free(name); - name = strdup(got_tree_entry_get_name(te)); - if (name == NULL) { - error = got_error_from_errno("strdup"); - goto done; - } - if (fcgi_gen_response(c, "
\n") == -1) goto done; if (fcgi_gen_response(c, "
") == -1) goto done; - if (fcgi_gen_response(c, - "%s%s", + index_page_str, qs->path, rc->commit_id, + folder, name, name, modestr); + if (r == -1) goto done; - if (fcgi_gen_response(c, qs->index_page_str) == -1) - goto done; - - if (fcgi_gen_response(c, "&path=") == -1) - goto done; - if (fcgi_gen_response(c, qs->path) == -1) - goto done; - - if (fcgi_gen_response(c, "&action=blob") == -1) - goto done; - - if (fcgi_gen_response(c, "&commit=") == -1) - goto done; - if (fcgi_gen_response(c, rc->commit_id) == -1) - goto done; - - if (fcgi_gen_response(c, "&folder=") == -1) - goto done; - if (fcgi_gen_response(c, qs->folder) == -1) - goto done; - - if (fcgi_gen_response(c, "&file=") == -1) - goto done; - if (fcgi_gen_response(c, name) == -1) - goto done; - - if (fcgi_gen_response(c, "'>") == -1) - goto done; - if (fcgi_gen_response(c, name) == -1) - goto done; - if (fcgi_gen_response(c, modestr) == -1) - goto done; - - if (fcgi_gen_response(c, "") == -1) - goto done; - if (fcgi_gen_response(c, "
\n") == -1) goto done; if (fcgi_gen_response(c, "
") == -1) goto done; - if (fcgi_gen_response(c, - "commits\n", + index_page_str, qs->path, rc->commit_id, + folder, name); + if (r == -1) goto done; - if (fcgi_gen_response(c, qs->index_page_str) == -1) - goto done; - - if (fcgi_gen_response(c, "&path=") == -1) - goto done; - if (fcgi_gen_response(c, qs->path) == -1) - goto done; - - if (fcgi_gen_response(c, "&action=commits") == -1) - goto done; - - if (fcgi_gen_response(c, "&commit=") == -1) - goto done; - if (fcgi_gen_response(c, rc->commit_id) == -1) - goto done; - - if (fcgi_gen_response(c, "&folder=") == -1) - goto done; - if (fcgi_gen_response(c, qs->folder) == -1) - goto done; - - if (fcgi_gen_response(c, "&file=") == -1) - goto done; - if (fcgi_gen_response(c, name) == -1) - goto done; - - if (fcgi_gen_response(c, "'>") == -1) - goto done; - - if (fcgi_gen_response(c, "commits") == -1) - goto done; - if (fcgi_gen_response(c, "\n") == -1) - goto done; - if (fcgi_gen_response(c, " | \n") == -1) goto done; - if (fcgi_gen_response(c, - "blame\n", + index_page_str, qs->path, rc->commit_id, + folder, name); - if (fcgi_gen_response(c, qs->index_page_str) == -1) - goto done; - - if (fcgi_gen_response(c, "&path=") == -1) - goto done; - if (fcgi_gen_response(c, qs->path) == -1) - goto done; - - if (fcgi_gen_response(c, "&action=blame") == -1) - goto done; - - if (fcgi_gen_response(c, "&commit=") == -1) - goto done; - if (fcgi_gen_response(c, rc->commit_id) == -1) - goto done; - - if (fcgi_gen_response(c, "&folder=") == -1) - goto done; - if (fcgi_gen_response(c, qs->folder) == -1) - goto done; - - if (fcgi_gen_response(c, "&file=") == -1) - goto done; - if (fcgi_gen_response(c, name) == -1) - goto done; - - if (fcgi_gen_response(c, "'>") == -1) - goto done; - - if (fcgi_gen_response(c, "blame") == -1) - goto done; - if (fcgi_gen_response(c, "\n") == -1) - goto done; - if (fcgi_gen_response(c, "
\n") == -1) goto done; if (fcgi_gen_response(c, "
\n") == -1) @@ -1124,19 +998,13 @@ got_output_repo_tree(struct request *c) } free(id_str); id_str = NULL; - free(build_folder); - build_folder = NULL; - free(name); - name = NULL; free(modestr); modestr = NULL; } done: free(id_str); - free(build_folder); free(modestr); free(path); - free(name); got_ref_list_free(&refs); if (commit) got_object_commit_close(commit); @@ -1286,11 +1154,13 @@ got_gotweb_blame_cb(void *arg, int nlines, int lineno, struct transport *t = c->t; struct querystring *qs = t->qs; struct repo_dir *repo_dir = t->repo_dir; + const char *index_page_str; char *line = NULL, *eline = NULL; size_t linesize = 0; off_t offset; struct tm tm; time_t committer_time; + int r; if (nlines != a->nlines || (lineno != -1 && lineno < 1) || lineno > a->nlines) @@ -1334,10 +1204,10 @@ got_gotweb_blame_cb(void *arg, int nlines, int lineno, goto done; } + index_page_str = qs->index_page_str ? qs->index_page_str : ""; + while (bline->annotated) { - int out_buff_size = 100; char *smallerthan, *at, *nl, *committer; - char out_buff[out_buff_size]; size_t len; if (getline(&line, &linesize, a->f) == -1) { @@ -1363,53 +1233,32 @@ got_gotweb_blame_cb(void *arg, int nlines, int lineno, if (fcgi_gen_response(c, "
") == -1) goto done; - if (fcgi_gen_response(c, "
") == -1) + + r = fcgi_printf(c, "
%.*d
", + a->nlines_prec, a->lineno_cur); + if (r == -1) goto done; - if (snprintf(out_buff, strlen(out_buff), "%.*d", a->nlines_prec, - a->lineno_cur) < 0) - goto done; - if (fcgi_gen_response(c, out_buff) == -1) - goto done; - if (fcgi_gen_response(c, "
") == -1) - goto done; if (fcgi_gen_response(c, "
") == -1) goto done; - if (fcgi_gen_response(c, "" + "%.8s
", + index_page_str, repo_dir->name, bline->id_str, + bline->id_str); + if (r == -1) goto done; - if (fcgi_gen_response(c, qs->index_page_str) == -1) - goto done; - if (fcgi_gen_response(c, "&path=") == -1) - goto done; - if (fcgi_gen_response(c, repo_dir->name) == -1) - goto done; - if (fcgi_gen_response(c, "&action=diff&commit=") == -1) - goto done; - if (fcgi_gen_response(c, bline->id_str) == -1) - goto done; - if (fcgi_gen_response(c, "'>") == -1) - goto done; - if (snprintf(out_buff, 10, "%.8s", bline->id_str) < 0) - goto done; - if (fcgi_gen_response(c, out_buff) == -1) - goto done; - if (fcgi_gen_response(c, "
") == -1) - goto done; - if (fcgi_gen_response(c, "
") == -1) + r = fcgi_printf(c, "
%s
", + bline->datebuf); + if (r == -1) goto done; - if (fcgi_gen_response(c, bline->datebuf) == -1) - goto done; - if (fcgi_gen_response(c, "
") == -1) - goto done; - if (fcgi_gen_response(c, "
") == -1) + r = fcgi_printf(c, "
%s
", + committer); + if (r == -1) goto done; - if (fcgi_gen_response(c, committer) == -1) - goto done; - if (fcgi_gen_response(c, "
") == -1) - goto done; if (fcgi_gen_response(c, "
") == -1) goto done; blob - b58b1e7be0421dac9ff6c9717bacb59208ad1bbb file + gotwebd/gotwebd.h --- gotwebd/gotwebd.h +++ gotwebd/gotwebd.h @@ -438,6 +438,9 @@ void fcgi_timeout(int, short, void *); void fcgi_cleanup_request(struct request *); void fcgi_create_end_record(struct request *); void dump_fcgi_record(const char *, struct fcgi_record_header *); +int fcgi_printf(struct request *, const char *, ...) + __attribute__((__format__(printf, 2, 3))) + __attribute__((__nonnull__(2))); int fcgi_gen_response(struct request *, const char *); int fcgi_gen_binary_response(struct request *, const uint8_t *, int);