On Mon, Sep 21, 2015 at 12:54 AM, Matthieu Moy
<Matthieu.Moy@xxxxxxxxxxxxxxx> wrote:
> Karthik Nayak <karthik.188@xxxxxxxxx> writes:
>
>> --- a/Documentation/git-branch.txt
>> +++ b/Documentation/git-branch.txt
>> @@ -231,6 +231,13 @@ start-point is either a local or remote-tracking branch.
>> The new name for an existing branch. The same restrictions as for
>> <branchname> apply.
>>
>> +--sort=<key>::
>> + Sort based on the key given. Prefix `-` to sort in descending
>> + order of the value. You may use the --sort=<key> option
>> + multiple times, in which case the last key becomes the primary
>> + key. The keys supported are the same as those in `git
>> + for-each-ref`. Sort order defaults to sorting based on branch
>> + type.
>
> The last sentence is no longer true. Perhaps something like:
>
> Sort order defaults to sorting based on the full refname (including
> `refs/...` prefix). This lists detached HEAD (if present) first, then
> local branches and finally remote-tracking branches.
>
Sounds good will follow.
>> -static void print_ref_list(struct ref_filter *filter)
>> +static void print_ref_list(struct ref_filter *filter, struct ref_sorting *sorting)
>> {
>> int i;
>> struct ref_array array;
>> - struct ref_filter_cbdata data;
>> int maxwidth = 0;
>> const char *remote_prefix = "";
>> - struct rev_info revs;
>> + struct ref_sorting def_sorting;
>> + const char *sort_type = "refname";
>
> You are using refname without special-casing HEAD at all. So, this is
> relying on the fact that HEAD comes alphabetically before refs/..., and
> that we are only listing refs/... and HEAD.
>
> As I mentionned earlyer, if we ever allow branch to list e.g.
> FETCH_HEAD, then the detached HEAD will come in the middle. I first
> thought that this was too fragile, but after thinking about it, I think
> this is actually sensible. After all, if we ever allow FETCH_HEAD, then
> keeping the alphabetical order still makes sense.
>
> So, your code is OK, but a bit too subtle to my taste: you should add a
> comment explaining why sorting according to refname is sufficient (I
> would use a comment, but a better commit message may be OK too).
Since I'm re-rolling, I'll do both :)
--
Regards,
Karthik Nayak
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html