On Wed, May 17, 2023 at 10:41:34AM +0200, Omar Polo wrote:
> On 2023/05/16 23:46:26 +0200, Stefan Sperling <stsp@stsp.name> wrote:
> > On Tue, May 16, 2023 at 11:30:45AM +0200, Omar Polo wrote:
> > > I've found that my suggestion could result in a segfault.  This
> > > because the reflist may include uninteresting refs in the refs/got/
> > > namespace that gets filtered out by build_refs_str.
> > > 
> > > If a commit happens to have only uninteresting refs pointing to it
> > > (e.g. when you have worktrees not up-to-date), build_refs_str returns
> > > NULL, whihc is not handled here.
> > > 
> > > I'm attaching a diff with a way to handle it.  Another way would be to
> > > relax build_refs_str to allow a NULL refs being passed.
> > 
> > ok, thanks!
> 
> as a small follow-up, what about allowing to pass a NULL reflist to
> build_refs_str?  Simplifies a bit the callers.

Sure. ok.

> diffstat /home/op/w/gotacl
>  M  tog/tog.c  |  11+  13-
> 
> 1 file changed, 11 insertions(+), 13 deletions(-)
> 
> diff /home/op/w/gotacl
> commit - 29efeeddc18828c36f1a6df50e05297a51a1a396
> path + /home/op/w/gotacl
> blob - 6d3e738e1220743df1fd30ba6a2779ff5b504e3e
> file + tog/tog.c
> --- tog/tog.c
> +++ tog/tog.c
> @@ -2322,6 +2322,9 @@ build_refs_str(char **refs_str, struct got_reflist_hea
>  
>  	*refs_str = NULL;
>  
> +	if (refs == NULL)
> +		return NULL;
> +
>  	TAILQ_FOREACH(re, refs, entry) {
>  		struct got_tag_object *tag = NULL;
>  		struct got_object_id *ref_id;
> @@ -2496,8 +2499,7 @@ draw_commit(struct tog_view *view, struct got_commit_o
>  
>  	/* Prepend reference labels to log message if possible .*/
>  	refs = got_reflist_object_id_map_lookup(tog_refs_idmap, id);
> -	if (refs)
> -		err = build_refs_str(&refs_str, refs, id, s->repo);
> +	err = build_refs_str(&refs_str, refs, id, s->repo);
>  	if (err)
>  		goto done;
>  	if (refs_str) {
> @@ -2777,12 +2779,10 @@ draw_commits(struct tog_view *view)
>  			return err;
>  		refs = got_reflist_object_id_map_lookup(tog_refs_idmap,
>  		    s->selected_entry->id);
> -		if (refs) {
> -			err = build_refs_str(&refs_str, refs,
> -			    s->selected_entry->id, s->repo);
> -			if (err)
> -				goto done;
> -		}
> +		err = build_refs_str(&refs_str, refs, s->selected_entry->id,
> +		    s->repo);
> +		if (err)
> +			goto done;
>  	}
>  
>  	if (s->thread_args.commits_needed == 0 && !using_mock_io)
> @@ -4965,11 +4965,9 @@ write_commit_info(struct got_diff_line **lines, size_t
>  	off_t outoff = 0;
>  	int n;
>  
> -	if (refs) {
> -		err = build_refs_str(&refs_str, refs, commit_id, repo);
> -		if (err)
> -			return err;
> -	}
> +	err = build_refs_str(&refs_str, refs, commit_id, repo);
> +	if (err)
> +		return err;
>  
>  	err = got_object_open_as_commit(&commit, repo, commit_id);
>  	if (err)
>