Re: [PATCH 3/3] worktree list: keep the list sorted

From: Junio C Hamano <gitster@pobox.com>
Date: Nov 23, 2016, 17:16 UTC
Message-ID: <xmqqwpfum801.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <20161122100046.8341-4-pclouds@gmail.com>

Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:

Show 9 quoted lines

> +		for (i = nr = 0; worktrees[i]; i++)
> +			nr++;
> +
> +		/*
> +		 * don't sort the first item (main worktree), which will
> +		 * always be the first
> +		 */
> +		QSORT(worktrees + 1, nr - 1, compare_worktree);
> +

This is somewhat curious.

	for (i = 0; worktrees[i]; i++)
		; /* just counting */
	QSORT(worktrees + 1, i - 1, compare_worktree);

would have been a lot more idiomatic (you do not use nr after sorting).

More importantly, perhaps get_worktrees() should learn to take an optional pointer to int that returns how many items are in the list?

Alternatively, other existing callers of the function do not care about the order, so it may not be such a good idea to always sort the result, but it may be a good idea to teach it to take a boolean that signals that the list should be sorted in a "natural order", which is how "worktree list" would show them to the user?

This should be easily protectable with a new test.  Please do.