Stefan Sperling <stsp@stsp.name> wrote:
> Pass an already open commit object to the blame callback.
> This was written on top of my id_by_path diff I sent earlier.
> 
> The idea is to avoid re-opening the same commit again in the blame callback
> as implemented by got.c and gotweb.c. Otherwise, these callbacks rely on the
> commit cache to avoid doing the same amount of work the caller already did.
> 
> ok?

it also makes the callbacks easier to read :)

ok op

> diff 6fd1a33c932d168a9aef98ca7760e8f63446bea1 43acb5b55df6240c091bc4ec11079eaee47d64a7
> blob - 7ac9d84ea1bc903f4fd7ab569799630bbaa48aaa
> blob + 1b4824dc3a8b72c725ef07b0e5d942bb8ad41589
> --- got/got.c
> +++ got/got.c
> @@ -4876,14 +4876,14 @@ struct blame_cb_args {
>  };
>  
>  static const struct got_error *
> -blame_cb(void *arg, int nlines, int lineno, struct got_object_id *id)
> +blame_cb(void *arg, int nlines, int lineno,
> +    struct got_commit_object *commit, struct got_object_id *id)
>  {
>  	const struct got_error *err = NULL;
>  	struct blame_cb_args *a = arg;
>  	struct blame_line *bline;
>  	char *line = NULL;
>  	size_t linesize = 0;
> -	struct got_commit_object *commit = NULL;
>  	off_t offset;
>  	struct tm tm;
>  	time_t committer_time;
> @@ -4906,10 +4906,6 @@ blame_cb(void *arg, int nlines, int lineno, struct got
>  	if (err)
>  		return err;
>  
> -	err = got_object_open_as_commit(&commit, a->repo, id);
> -	if (err)
> -		goto done;
> -
>  	bline->committer = strdup(got_object_commit_get_committer(commit));
>  	if (bline->committer == NULL) {
>  		err = got_error_from_errno("strdup");
> @@ -4968,8 +4964,6 @@ blame_cb(void *arg, int nlines, int lineno, struct got
>  		bline = &a->lines[a->lineno_cur - 1];
>  	}
>  done:
> -	if (commit)
> -		got_object_commit_close(commit);
>  	free(line);
>  	return err;
>  }
> blob - 70b31ef68737cdd0d46bd944fea3af2c33d3e89a
> blob + 710788181588ea0542eb1b6312476ddaa579fc99
> --- gotweb/gotweb.c
> +++ gotweb/gotweb.c
> @@ -214,6 +214,7 @@ static const struct got_error	*gw_get_commit(struct gw
>  				    struct got_object_id *);
>  static const struct got_error	*gw_apply_unveil(const char *);
>  static const struct got_error	*gw_blame_cb(void *, int, int,
> +				    struct got_commit_object *,
>  				    struct got_object_id *);
>  static const struct got_error	*gw_load_got_paths(struct gw_trans *);
>  static const struct got_error	*gw_load_got_path(struct gw_trans *,
> @@ -3862,14 +3863,14 @@ struct gw_blame_cb_args {
>  };
>  
>  static const struct got_error *
> -gw_blame_cb(void *arg, int nlines, int lineno, struct got_object_id *id)
> +gw_blame_cb(void *arg, int nlines, int lineno,
> +    struct got_commit_object *commit, struct got_object_id *id)
>  {
>  	const struct got_error *err = NULL;
>  	struct gw_blame_cb_args *a = arg;
>  	struct blame_line *bline;
>  	char *line = NULL;
>  	size_t linesize = 0;
> -	struct got_commit_object *commit = NULL;
>  	off_t offset;
>  	struct tm tm;
>  	time_t committer_time;
> @@ -3890,10 +3891,6 @@ gw_blame_cb(void *arg, int nlines, int lineno, struct 
>  	if (err)
>  		return err;
>  
> -	err = got_object_open_as_commit(&commit, a->repo, id);
> -	if (err)
> -		goto done;
> -
>  	bline->committer = strdup(got_object_commit_get_committer(commit));
>  	if (bline->committer == NULL) {
>  		err = got_error_from_errno("strdup");
> @@ -4030,8 +4027,6 @@ err:
>  		free(href_diff);
>  	}
>  done:
> -	if (commit)
> -		got_object_commit_close(commit);
>  	free(line);
>  	if (err == NULL && kerr != KCGI_OK)
>  		err = gw_kcgi_error(kerr);
> blob - c4c15ba2906056d4efdd0378fb21aa0e70cf83f7
> blob + e360955ca287d181a8ff24dbf66100470e96de18
> --- include/got_blame.h
> +++ include/got_blame.h
> @@ -14,6 +14,9 @@
>   * OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
>   */
>  
> +typedef const struct got_error *(*got_blame_cb)(void *, int, int,
> +    struct got_commit_object *, struct got_object_id *);
> +
>  /*
>   * Blame the blob at the specified path in the specified commit and invoke
>   * a callback whenever an annotation has been computed for a line.
> @@ -33,5 +36,4 @@
>   */
>  const struct got_error *got_blame(const char *,
>      struct got_object_id *, struct got_repository *,
> -    const struct got_error *(*cb)(void *, int, int, struct got_object_id *),
> -    void *, got_cancel_cb, void *);
> +    got_blame_cb, void *, got_cancel_cb, void *);
> blob - 4fb57b680144beb72b8bb8898b73e6d032ad15d8
> blob + 19208f0e07660099d682daaab42dfacd9c2c5724
> --- lib/blame.c
> +++ lib/blame.c
> @@ -89,9 +89,9 @@ struct got_blame {
>  };
>  
>  static const struct got_error *
> -annotate_line(struct got_blame *blame, int lineno, struct got_object_id *id,
> -    const struct got_error *(*cb)(void *, int, int, struct got_object_id *),
> -    void *arg)
> +annotate_line(struct got_blame *blame, int lineno,
> +    struct got_commit_object *commit, struct got_object_id *id,
> +    got_blame_cb cb, void *arg)
>  {
>  	const struct got_error *err = NULL;
>  	struct got_blame_line *line;
> @@ -107,15 +107,14 @@ annotate_line(struct got_blame *blame, int lineno, str
>  	line->annotated = 1;
>  	blame->nannotated++;
>  	if (cb)
> -		err = cb(arg, blame->nlines, lineno + 1, id);
> +		err = cb(arg, blame->nlines, lineno + 1, commit, id);
>  	return err;
>  }
>  
>  static const struct got_error *
>  blame_changes(struct got_blame *blame, struct diff_result *diff_result,
> -    struct got_object_id *commit_id,
> -    const struct got_error *(*cb)(void *, int, int, struct got_object_id *),
> -    void *arg)
> +    struct got_commit_object *commit, struct got_object_id *commit_id,
> +    got_blame_cb cb, void *arg)
>  {
>  	const struct got_error *err = NULL;
>  	int i;
> @@ -153,7 +152,8 @@ blame_changes(struct got_blame *blame, struct diff_res
>  
>  		for (j = 0; j < right_count; j++) {
>  			int ln = blame->linemap2[idx2++];
> -			err = annotate_line(blame, ln, commit_id, cb, arg);
> +			err = annotate_line(blame, ln, commit, commit_id,
> +			    cb, arg);
>  			if (err)
>  				return err;
>  			if (blame->nlines == blame->nannotated)
> @@ -196,8 +196,7 @@ blame_prepare_file(FILE *f, unsigned char **p, off_t *
>  static const struct got_error *
>  blame_commit(struct got_blame *blame, struct got_object_id *id,
>      const char *path, struct got_repository *repo,
> -    const struct got_error *(*cb)(void *, int, int, struct got_object_id *),
> -    void *arg)
> +    got_blame_cb cb, void *arg)
>  {
>  	const struct got_error *err = NULL;
>  	struct got_commit_object *commit = NULL, *pcommit = NULL;
> @@ -261,11 +260,11 @@ blame_commit(struct got_blame *blame, struct got_objec
>  				goto done;
>  			}
>  		}
> -		err = blame_changes(blame, diff_result, id, cb, arg);
> +		err = blame_changes(blame, diff_result, commit, id, cb, arg);
>  		if (err)
>  			goto done;
>  	} else if (cb)
> -		err = cb(arg, blame->nlines, -1, id);
> +		err = cb(arg, blame->nlines, -1, commit, id);
>  done:
>  	if (diff_result)
>  		diff_result_free(diff_result);
> @@ -500,11 +499,10 @@ close_file2_and_reuse_file1(struct got_blame *blame)
>  static const struct got_error *
>  blame_open(struct got_blame **blamep, const char *path,
>      struct got_object_id *start_commit_id, struct got_repository *repo,
> -    const struct got_error *(*cb)(void *, int, int, struct got_object_id *),
> -    void *arg, got_cancel_cb cancel_cb, void *cancel_arg)
> +    got_blame_cb cb, void *arg, got_cancel_cb cancel_cb, void *cancel_arg)
>  {
>  	const struct got_error *err = NULL;
> -	struct got_commit_object *start_commit = NULL;
> +	struct got_commit_object *start_commit = NULL, *last_commit = NULL;
>  	struct got_object_id *obj_id = NULL;
>  	struct got_blob_object *blob = NULL;
>  	struct got_blame *blame = NULL;
> @@ -619,8 +617,12 @@ blame_open(struct got_blame **blamep, const char *path
>  
>  	if (id && blame->nannotated < blame->nlines) {
>  		/* Annotate remaining non-annotated lines with last commit. */
> +		err = got_object_open_as_commit(&last_commit, repo, id);
> +		if (err)
> +			goto done;
>  		for (lineno = 0; lineno < blame->nlines; lineno++) {
> -			err = annotate_line(blame, lineno, id, cb, arg);
> +			err = annotate_line(blame, lineno, last_commit, id,
> +			    cb, arg);
>  			if (err)
>  				goto done;
>  		}
> @@ -634,6 +636,8 @@ done:
>  		got_object_blob_close(blob);
>  	if (start_commit)
>  		got_object_commit_close(start_commit);
> +	if (last_commit)
> +		got_object_commit_close(last_commit);
>  	if (err) {
>  		if (blame)
>  			blame_close(blame);
> @@ -645,9 +649,8 @@ done:
>  
>  const struct got_error *
>  got_blame(const char *path, struct got_object_id *commit_id,
> -    struct got_repository *repo,
> -    const struct got_error *(*cb)(void *, int, int, struct got_object_id *),
> -    void *arg, got_cancel_cb cancel_cb, void* cancel_arg)
> +    struct got_repository *repo, got_blame_cb cb, void *arg,
> +    got_cancel_cb cancel_cb, void* cancel_arg)
>  {
>  	const struct got_error *err = NULL, *close_err = NULL;
>  	struct got_blame *blame;
> blob - cf640001b1d644e38bc4909d42362dd3ed7f348e
> blob + 1235f3d15f5c154123999f3791ee35c3119cd238
> --- tog/tog.c
> +++ tog/tog.c
> @@ -4162,7 +4162,8 @@ draw_blame(struct tog_view *view)
>  }
>  
>  static const struct got_error *
> -blame_cb(void *arg, int nlines, int lineno, struct got_object_id *id)
> +blame_cb(void *arg, int nlines, int lineno,
> +    struct got_commit_object *commit, struct got_object_id *id)
>  {
>  	const struct got_error *err = NULL;
>  	struct tog_blame_cb_args *a = arg;