On Thu, Nov 21, 2024 at 01:13:12AM +0000, Kyle Ackerman wrote:
> @@ -780,11 +781,12 @@ got_privsep_recv_fetch_progress(int *done, struct got_
>  			}
>  			off += s->target_len;
>  			remain -= s->target_len;
> -			err = got_pathlist_append(symrefs, name, target);
> +			err = got_pathlist_insert(&new, symrefs, name, target);
>  			if (err) {

Missing new == NULL condition here?

>  				free(name);
>  				free(target);
> -				goto done;
> +				if (err->code != GOT_ERR_REF_DUP_ENTRY)
> +					goto done;
>  			}
>  		}
>  		break;
> @@ -938,6 +939,7 @@ main(int argc, char **argv)
>  		struct got_object_id *id;
>  		char *refname;
>  
> +
>  		err = got_privsep_recv_imsg(&imsg, &ibuf, 0);
>  		if (err) {
>  			if (err->code == GOT_ERR_PRIVSEP_PIPE)


Needless whitespace change above.

> --- regress/cmdline/diff.sh
> +++ regress/cmdline/diff.sh
> @@ -1941,8 +1941,8 @@ date: $d
>   
>   changed epsilon into file
>   
> - D  epsilon/zeta
>   A  epsilon
> + D  epsilon/zeta
>  

The above chunk of the patch did not apply with 'got patch', probably
because leading spaces are present on these lines. Not sure why, and
not a deal-breaker of course.

> Thoughts/comments/suggestions?

I'm ok with this. Would you mind me addressing the above nits
before comitting your diff?

As I recall, the reason for having the _append function was to allow for
keeping paths sorted in the order given on the command line, such that
the output of a command would reflect the order in which paths were
specified by the user.
However, this was never used effectively, and at least in the case of
'got diff' it was later decided to sort specified paths to provide
consistent output regardless of how the path arguments are provided.

Today, I cannot think of a reason why paths shouldn't always be
sorted, and with an RB tree we cannot have it any other way.