Re: [PATCH 1/4] remote: minor code cleanups in preparation for changing "show" output

From: Junio C Hamano <gitster@pobox.com>
Date: Feb 20, 2009, 07:19 UTC
Message-ID: <7vbpsxh8xa.fsf@gitster.siamese.dyndns.org>
In-Reply-To: <1235020471-59982-2-git-send-email-jaysoffian@gmail.com>

Jay Soffian <jaysoffian@gmail.com> writes:

Show 17 quoted lines

> * Rename char *remote to remote_name to distinguish it clearly from the
>   struct remote pointer, also named remote.
>
> * There is no need to call sort_string_list() on branch_list, as its
>   items are added to it via string_list_insert() which maintains its
>   order.
>
> * Sort states->new and states->tracked so that we can use binary search
>   string_list_has_string() on them instead of less efficient linear
>   unsorted_string_list_has_string. This alters the output of "remote
>   show" slightly, so update the tests to match.
>
> * Simplify get_ref_states(); nothing is using the pointer to states that
>   is being copied into util.
>
> * Have get_remote_ref_states() populate states->tracked even when it is
>   not querying the remote so that this need not be done by the caller.

This does too many things in a single patch.

Ideally this would have been four patches for reviewability:

 - one "trivial and obviously correct bits" (s/remote/remote_name/ and
   removal of sort_string_list(&branch_list)) patch;

 - the change for states->{new,tracked}, should stand on its own; I think
   the reordering of the output should be described much better and
   defended independently.  "Earlier it was sorted by this order, which
   did not make sense for such and such reasons; this fixes the logic to
   sort the list by the name of the tracked branch, which makes it easier
   to read", or something like that.

 - change to the states->tracked population rule; and

 - get_ref_states() to lose the util bit.

It probably is Ok to squash the last two, though.