Re: [PATCH v5 7/8] branch.c: use 'ref-filter' APIs

[Karthik Nayak]

On Mon, Sep 21, 2015 at 12:54 AM, Matthieu Moy
 wrote:
> Karthik Nayak  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
>>apply.
>>
>> +--sort=::
>> + Sort based on the key given. Prefix `-` to sort in descending
>> + order of the value. You may use the --sort= 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 majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v5 7/8] branch.c: use 'ref-filter' APIs

[Matthieu Moy]

Karthik Nayak  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
>apply.
>  
> +--sort=::
> + Sort based on the key given. Prefix `-` to sort in descending
> + order of the value. You may use the --sort= 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.

> -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).

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v5 7/8] branch.c: use 'ref-filter' APIs

[Karthik Nayak]

Make 'branch.c' use 'ref-filter' APIs for iterating through refs
sorting. This removes most of the code used in 'branch.c' replacing it
with calls to the 'ref-filter' library.

Make 'branch.c' use the 'filter_refs()' function provided by 'ref-filter'
to filter out tags based on the options set.

We provide a sorting option provided for 'branch.c' by using the sorting
options provided by 'ref-filter'.

Also remove the 'ignore' variable from ref_array_item as it was
previously used for the '--merged' option and now that is handled by
ref-filter.

Modify some of the tests in t1430 to check the stderr for a warning
regarding the broken ref. This is done as ref-filter throws a warning
for broken refs rather than directly printing them.

Add tests and documentation for the same.

Mentored-by: Christian Couder 
Mentored-by: Matthieu Moy 
Signed-off-by: Karthik Nayak 
---
 Documentation/git-branch.txt |   9 +-
 builtin/branch.c | 212 +++
 ref-filter.c |   2 +-
 ref-filter.h |   1 -
 t/t1430-bad-ref-name.sh  |  31 ---
 t/t3203-branch-output.sh |  11 +++
 6 files changed, 74 insertions(+), 192 deletions(-)

diff --git a/Documentation/git-branch.txt b/Documentation/git-branch.txt
index bbbade4..b87bd7b 100644
--- a/Documentation/git-branch.txt
+++ b/Documentation/git-branch.txt
@@ -11,7 +11,7 @@ SYNOPSIS
 'git branch' [--color[=] | --no-color] [-r | -a]
[--list] [-v [--abbrev= | --no-abbrev]]
[--column[=] | --no-column]
-   [(--merged | --no-merged | --contains) []] [...]
+   [(--merged | --no-merged | --contains) []] [--sort=] 
[...]
 'git branch' [--set-upstream | --track | --no-track] [-l] [-f]  
[]
 'git branch' (--set-upstream-to= | -u ) []
 'git branch' --unset-upstream []
@@ -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
 apply.
 
+--sort=::
+   Sort based on the key given. Prefix `-` to sort in descending
+   order of the value. You may use the --sort= 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.
 
 Examples
 
diff --git a/builtin/branch.c b/builtin/branch.c
index 22557ec..c31d1c4 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -270,125 +270,6 @@ static int delete_branches(int argc, const char **argv, 
int force, int kinds,
return(ret);
 }
 
-static char *resolve_symref(const char *src, const char *prefix)
-{
-   unsigned char sha1[20];
-   int flag;
-   const char *dst;
-
-   dst = resolve_ref_unsafe(src, 0, sha1, );
-   if (!(dst && (flag & REF_ISSYMREF)))
-   return NULL;
-   if (prefix)
-   skip_prefix(dst, prefix, );
-   return xstrdup(dst);
-}
-
-static int match_patterns(const char **pattern, const char *refname)
-{
-   if (!*pattern)
-   return 1; /* no pattern always matches */
-   while (*pattern) {
-   if (!wildmatch(*pattern, refname, 0, NULL))
-   return 1;
-   pattern++;
-   }
-   return 0;
-}
-
-/*
- * Allocate memory for a new ref_array_item and insert that into the
- * given ref_array. Doesn't take the objectname unlike
- * new_ref_array_item(). This is a temporary function which will be
- * removed when we port branch.c to use ref-filter APIs.
- */
-static struct ref_array_item *ref_array_append(struct ref_array *array, const 
char *refname)
-{
-   size_t len = strlen(refname);
-   struct ref_array_item *ref = xcalloc(1, sizeof(struct ref_array_item) + 
len + 1);
-   memcpy(ref->refname, refname, len);
-   ref->refname[len] = '\0';
-   REALLOC_ARRAY(array->items, array->nr + 1);
-   array->items[array->nr++] = ref;
-   return ref;
-}
-
-static int append_ref(const char *refname, const struct object_id *oid, int 
flags, void *cb_data)
-{
-   struct ref_filter_cbdata *cb = (struct ref_filter_cbdata *)(cb_data);
-   struct ref_filter *filter = cb->filter;
-   struct ref_array *array = cb->array;
-   struct ref_array_item *item;
-   struct commit *commit;
-   int kind, i;
-   const char *prefix, *orig_refname = refname;
-
-   static struct {
-   int kind;
-   const char *prefix;
-   } ref_kind[] = {
-   { FILTER_REFS_BRANCHES, "refs/heads/" },
-   { FILTER_REFS_REMOTES, "refs/remotes/" },
-   };
-
-   /* Detect kind */
-   for (i = 0; i < ARRAY_SIZE(ref_kind); i++) {
-   prefix = ref_kind[i].prefix;
-   if (skip_prefix(refname, prefix, )) {
-   kind = ref_kind[i].kind;
-