Re: [PATCH v4 05/10] ref-filter: add support to sort by version
Karthik Nayak karthik@gmail.com writes:
On Sun, Jul 26, 2015 at 4:10 AM, Junio C Hamano gits...@pobox.com wrote:
Without looking at the callers, s-version looks like a misdesign
that should be updated to use the same cmp_type mechanism? That
would lead to even more obvious construct that is easy to enhance,
i.e.
switch (cmp_type) {
case CMP_VERSION:
...
case CMP_STRING:
...
case CMP_NUMBER:
...
}
I dunno.
Other than that (and the structure of that format-state stuff we
discussed separately), the series was a pleasant read.
Thanks.
That was the previous design, but Duy asked me to do this so
that we could support all atoms. And I agree with him on this.
http://article.gmane.org/gmane.comp.version-control.git/273888
I am not objecting, but $gmane/273888 does not immediately read, at
least to me, as suggesting using a mechanism different from cmp_type
but a dedicated field s-version. Puzzled...
--
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 v4 05/10] ref-filter: add support to sort by version
On Mon, Jul 27, 2015 at 8:54 PM, Junio C Hamano gits...@pobox.com wrote:
Karthik Nayak karthik@gmail.com writes:
On Sun, Jul 26, 2015 at 4:10 AM, Junio C Hamano gits...@pobox.com wrote:
Without looking at the callers, s-version looks like a misdesign
that should be updated to use the same cmp_type mechanism? That
would lead to even more obvious construct that is easy to enhance,
i.e.
switch (cmp_type) {
case CMP_VERSION:
...
case CMP_STRING:
...
case CMP_NUMBER:
...
}
I dunno.
Other than that (and the structure of that format-state stuff we
discussed separately), the series was a pleasant read.
Thanks.
That was the previous design, but Duy asked me to do this so
that we could support all atoms. And I agree with him on this.
http://article.gmane.org/gmane.comp.version-control.git/273888
I am not objecting, but $gmane/273888 does not immediately read, at
least to me, as suggesting using a mechanism different from cmp_type
but a dedicated field s-version. Puzzled...
What I mean was since version/v aren't atoms as such, their processing is
done before we parse through atoms and fill used_atoms and used_atom_type.
cmp_type is obtained from the used_atom_type, which isn't filled by
version/v.
Hence the dedicated field, just like reverse.
--
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 v4 05/10] ref-filter: add support to sort by version
On Sun, Jul 26, 2015 at 4:10 AM, Junio C Hamano gits...@pobox.com wrote:
Karthik Nayak karthik@gmail.com writes:
@@ -1180,19 +1181,17 @@ static int cmp_ref_sorting(struct ref_sorting *s,
struct ref_array_item *a, stru
get_ref_atom_value(state, a, s-atom, va);
get_ref_atom_value(state, b, s-atom, vb);
- switch (cmp_type) {
- case FIELD_STR:
+ if (s-version)
+ cmp = versioncmp(va-s, vb-s);
+ else if (cmp_type == FIELD_STR)
cmp = strcmp(va-s, vb-s);
- break;
- default:
- if (va-ul vb-ul)
- cmp = -1;
- else if (va-ul == vb-ul)
- cmp = 0;
- else
- cmp = 1;
- break;
- }
+ else if (va-ul vb-ul)
+ cmp = -1;
+ else if (va-ul == vb-ul)
+ cmp = 0;
+ else
+ cmp = 1;
+
So there are generally three kinds of comparison possible:
- if it is to be compared as versions, do versioncmp
- if it is to be compared as strings, do strcmp
- if it is to be compared as numbers, do = but because
we are writing in C, not in Perl, do so as if/else/else
Having understood that, the above is not really easy to read and
extend. We should structure the above more like this:
if (s-version)
... versioncmp
else if (... FIELD_STR)
... strcmp
else {
if (a b)
...
else if (a == b)
...
else
...
}
so that it would be obvious how this code need to be updated
when we need to add yet another kind of comparison.
I find the current version more pleasing to read, The way you've explained
it though, it seems that its better to structure it the way you've
mentioned as this
actually shows the code flow of the three kinds of comparison possible.
Without looking at the callers, s-version looks like a misdesign
that should be updated to use the same cmp_type mechanism? That
would lead to even more obvious construct that is easy to enhance,
i.e.
switch (cmp_type) {
case CMP_VERSION:
...
case CMP_STRING:
...
case CMP_NUMBER:
...
}
I dunno.
Other than that (and the structure of that format-state stuff we
discussed separately), the series was a pleasant read.
Thanks.
That was the previous design, but Duy asked me to do this so
that we could support all atoms. And I agree with him on this.
http://article.gmane.org/gmane.comp.version-control.git/273888
--
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 v4 05/10] ref-filter: add support to sort by version
Karthik Nayak karthik@gmail.com writes:
@@ -1180,19 +1181,17 @@ static int cmp_ref_sorting(struct ref_sorting *s,
struct ref_array_item *a, stru
get_ref_atom_value(state, a, s-atom, va);
get_ref_atom_value(state, b, s-atom, vb);
- switch (cmp_type) {
- case FIELD_STR:
+ if (s-version)
+ cmp = versioncmp(va-s, vb-s);
+ else if (cmp_type == FIELD_STR)
cmp = strcmp(va-s, vb-s);
- break;
- default:
- if (va-ul vb-ul)
- cmp = -1;
- else if (va-ul == vb-ul)
- cmp = 0;
- else
- cmp = 1;
- break;
- }
+ else if (va-ul vb-ul)
+ cmp = -1;
+ else if (va-ul == vb-ul)
+ cmp = 0;
+ else
+ cmp = 1;
+
So there are generally three kinds of comparison possible:
- if it is to be compared as versions, do versioncmp
- if it is to be compared as strings, do strcmp
- if it is to be compared as numbers, do = but because
we are writing in C, not in Perl, do so as if/else/else
Having understood that, the above is not really easy to read and
extend. We should structure the above more like this:
if (s-version)
... versioncmp
else if (... FIELD_STR)
... strcmp
else {
if (a b)
...
else if (a == b)
...
else
...
}
so that it would be obvious how this code need to be updated
when we need to add yet another kind of comparison.
Without looking at the callers, s-version looks like a misdesign
that should be updated to use the same cmp_type mechanism? That
would lead to even more obvious construct that is easy to enhance,
i.e.
switch (cmp_type) {
case CMP_VERSION:
...
case CMP_STRING:
...
case CMP_NUMBER:
...
}
I dunno.
Other than that (and the structure of that format-state stuff we
discussed separately), the series was a pleasant read.
Thanks.
--
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 v4 05/10] ref-filter: add support to sort by version
From: Karthik Nayak karthik@gmail.com
Add support to sort by version using the v:refname and
version:refname option. This is achieved by using the 'versioncmp()'
function as the comparing function for qsort.
This option is included to support sorting by versions in `git tag -l`
which will eventaully be ported to use ref-filter APIs.
Add documentation and tests for the same.
Mentored-by: Christian Couder christian.cou...@gmail.com
Mentored-by: Matthieu Moy matthieu@grenoble-inp.fr
Signed-off-by: Karthik Nayak karthik@gmail.com
---
Documentation/git-for-each-ref.txt | 3 +++
ref-filter.c | 26 ++
ref-filter.h | 3 ++-
t/t6302-for-each-ref-filter.sh | 36
4 files changed, 55 insertions(+), 13 deletions(-)
diff --git a/Documentation/git-for-each-ref.txt
b/Documentation/git-for-each-ref.txt
index e49d578..224dc8c 100644
--- a/Documentation/git-for-each-ref.txt
+++ b/Documentation/git-for-each-ref.txt
@@ -145,6 +145,9 @@ For sorting purposes, fields with numeric values sort in
numeric
order (`objectsize`, `authordate`, `committerdate`, `taggerdate`).
All other fields are used to sort in their byte-value order.
+There is also an option to sort by versions, this can be done by using
+the fieldname `version:refname` or in short `v:refname`.
+
In any case, a field name that refers to a field inapplicable to
the object referred by the ref does not cause an error. It
returns an empty string instead.
diff --git a/ref-filter.c b/ref-filter.c
index 08ecce5..8f2148f 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -12,6 +12,7 @@
#include revision.h
#include utf8.h
#include git-compat-util.h
+#include version.h
typedef enum { FIELD_STR, FIELD_ULONG, FIELD_TIME } cmp_type;
@@ -1180,19 +1181,17 @@ static int cmp_ref_sorting(struct ref_sorting *s,
struct ref_array_item *a, stru
get_ref_atom_value(state, a, s-atom, va);
get_ref_atom_value(state, b, s-atom, vb);
- switch (cmp_type) {
- case FIELD_STR:
+ if (s-version)
+ cmp = versioncmp(va-s, vb-s);
+ else if (cmp_type == FIELD_STR)
cmp = strcmp(va-s, vb-s);
- break;
- default:
- if (va-ul vb-ul)
- cmp = -1;
- else if (va-ul == vb-ul)
- cmp = 0;
- else
- cmp = 1;
- break;
- }
+ else if (va-ul vb-ul)
+ cmp = -1;
+ else if (va-ul == vb-ul)
+ cmp = 0;
+ else
+ cmp = 1;
+
return (s-reverse) ? -cmp : cmp;
}
@@ -1420,6 +1419,9 @@ int parse_opt_ref_sorting(const struct option *opt, const
char *arg, int unset)
s-reverse = 1;
arg++;
}
+ if (skip_prefix(arg, version:, arg) ||
+ skip_prefix(arg, v:, arg))
+ s-version = 1;
len = strlen(arg);
s-atom = parse_ref_filter_atom(arg, arg+len);
return 0;
diff --git a/ref-filter.h b/ref-filter.h
index 1e2ee65..a12fe0c 100644
--- a/ref-filter.h
+++ b/ref-filter.h
@@ -26,7 +26,8 @@ struct atom_value {
struct ref_sorting {
struct ref_sorting *next;
int atom; /* index into used_atom array (internal) */
- unsigned reverse : 1;
+ unsigned reverse : 1,
+ version : 1;
};
struct ref_formatting_state {
diff --git a/t/t6302-for-each-ref-filter.sh b/t/t6302-for-each-ref-filter.sh
index 505a360..5017032 100755
--- a/t/t6302-for-each-ref-filter.sh
+++ b/t/t6302-for-each-ref-filter.sh
@@ -81,4 +81,40 @@ test_expect_success 'filtering with --contains' '
test_cmp expect actual
'
+test_expect_success 'setup for version sort' '
+ test_commit foo1.3
+ test_commit foo1.6
+ test_commit foo1.10
+'
+
+test_expect_success 'version sort' '
+ git tag -l --sort=version:refname | grep foo actual
+ cat expect -\EOF
+ foo1.3
+ foo1.6
+ foo1.10
+ EOF
+ test_cmp expect actual
+'
+
+test_expect_success 'version sort (shortened)' '
+ git tag -l --sort=v:refname | grep foo actual
+ cat expect -\EOF
+ foo1.3
+ foo1.6
+ foo1.10
+ EOF
+ test_cmp expect actual
+'
+
+test_expect_success 'reverse version sort' '
+ git tag -l --sort=-version:refname | grep foo actual
+ cat expect -\EOF
+ foo1.10
+ foo1.6
+ foo1.3
+ EOF
+ test_cmp expect actual
+'
+
test_done
--
2.4.6
--
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
