On Thu, Jul 21, 2022 at 07:09:23PM +0200, Florian Obser wrote:
> scan-build reports two more dead stores in tog/tog.c where I don't know
> what to do about.
> 
> No idea what this does, should there be only one call to
> offset_selection_down()? or are there simply two
> 	if (err)
> 		return err;
> missing?
> 
>   1206		if (v->mode == TOG_VIEW_SPLIT_HRZN) {
>   1207			err = offset_selection_down(v);
>   1208			err = offset_selection_down(v->child);
>   1209		} else {

To me this looks like two missing error checks.
In the else clause, we run the corresponding _up functions for
both v and v->child as well.

Errors here could be related to a log view failing to load additional
commits to fill the scrolled display, for example. Which is definitely
something we'd want to alert the user about.
 
> Here I'd just ignore close_err because there is nothing we can do about
> it. But we have all the information that we need, so we might as well
> just chug along.
> 
> It's mainly bubbling up errors from close(2) which tbh should have a
> return type of void.

I've found some double-close errors by adding such checks, which would
otherwise have kept lingering undetected. And if I pass an invalid value
like -1 by accident (as I've done sometimes), close(2) will complain and
make me go look for the issue.
Those are the reasons I started checking for close errors. It's not that I
would expect close(2) itself to fail when given an open file descriptor.

> But maybe that's not the style all y'all prefer, then I guess it should
> do a goto done;

So, yes, this code should take the error path if close_err != NULL.

Thank you for spotting these mistakes.

>   8272		close_err = got_repo_close(repo);
>   8273		if (error == NULL)
>   8274			error = close_err;
>   8275		repo = NULL;
>   8276
>   8277		error = got_object_id_str(&commit_id_str, commit_id);
>   8278		if (error)
>   8279			goto done;