[PATCH] for-each-ref: fix setup of option-parsing for --sort

[PATCH] for-each-ref: fix setup of option-parsing for --sort

[Lars Hjemli]

The option value for --sort is already a pointer to a pointer to struct
ref_sort, so just use it.

Signed-off-by: Lars Hjemli <hjemli@gmail.com>
---

On Nov 10, 2007 5:25 PM, Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:
> Could you add a test for that too, please?

Is this ok?


 builtin-for-each-ref.c  |    2 +-
 t/t6300-for-each-ref.sh |   22 ++++++++++++++++++++++
 2 files changed, 23 insertions(+), 1 deletions(-)

diff --git a/builtin-for-each-ref.c b/builtin-for-each-ref.c
index da8c794..e909e66 100644
--- a/builtin-for-each-ref.c
+++ b/builtin-for-each-ref.c
@@ -847,7 +847,7 @@ int cmd_for_each_ref(int argc, const char **argv, const char *prefix)
 		OPT_GROUP(""),
 		OPT_INTEGER( 0 , "count", &maxcount, "show only <n> matched refs"),
 		OPT_STRING(  0 , "format", &format, "format", "format to use for the output"),
-		OPT_CALLBACK(0 , "sort", &sort_tail, "key",
+		OPT_CALLBACK(0 , "sort", sort_tail, "key",
 		            "field name to sort on", &opt_parse_sort),
 		OPT_END(),
 	};
diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh
index d0809eb..c722635 100755
--- a/t/t6300-for-each-ref.sh
+++ b/t/t6300-for-each-ref.sh
@@ -148,4 +148,26 @@ test_expect_success 'Check format "rfc2822" date fields output' '
 	git diff expected actual
 '
 
+cat >expected <<\EOF
+refs/heads/master
+refs/tags/testtag
+EOF
+
+test_expect_success 'Verify ascending sort' '
+	git-for-each-ref --format="%(refname)" --sort=refname >actual &&
+	git diff expected actual
+'
+
+
+cat >expected <<\EOF
+refs/tags/testtag
+refs/heads/master
+EOF
+
+test_expect_success 'Verify descending sort' '
+	git-for-each-ref --format="%(refname)" --sort=-refname >actual &&
+	git diff expected actual
+'
+
+
 test_done
-- 
1.5.3.5.623.g0a1d

Re: [PATCH] for-each-ref: fix setup of option-parsing for --sort

[Junio C Hamano]

Lars Hjemli <hjemli@gmail.com> writes:

> The option value for --sort is already a pointer to a pointer to struct
> ref_sort, so just use it.
>
> Signed-off-by: Lars Hjemli <hjemli@gmail.com>
> ---
>
> On Nov 10, 2007 5:25 PM, Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:
>> Could you add a test for that too, please?
>
> Is this ok?
>

Testing "for that" would be kind of hard and semi pointless,
isn't it?

If it's mismatch of the expected number of times a pointer is
dereferenced between the caller and the callee, I'd imagine that
it will read from and write to random place in memory and would
lead to unpredictable behaviour.  If you are lucky you would not
get expected results but if you are unlucky who knows what would
happen.

But the new test makes sure --sort takes intended effect, which
is a good thing.

Thanks.

>  builtin-for-each-ref.c  |    2 +-
>  t/t6300-for-each-ref.sh |   22 ++++++++++++++++++++++
>  2 files changed, 23 insertions(+), 1 deletions(-)
>
> diff --git a/builtin-for-each-ref.c b/builtin-for-each-ref.c
> index da8c794..e909e66 100644
> --- a/builtin-for-each-ref.c
> +++ b/builtin-for-each-ref.c
> @@ -847,7 +847,7 @@ int cmd_for_each_ref(int argc, const char **argv, const char *prefix)
>  		OPT_GROUP(""),
>  		OPT_INTEGER( 0 , "count", &maxcount, "show only <n> matched refs"),
>  		OPT_STRING(  0 , "format", &format, "format", "format to use for the output"),
> -		OPT_CALLBACK(0 , "sort", &sort_tail, "key",
> +		OPT_CALLBACK(0 , "sort", sort_tail, "key",
>  		            "field name to sort on", &opt_parse_sort),
>  		OPT_END(),
>  	};
> diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh
> index d0809eb..c722635 100755
> --- a/t/t6300-for-each-ref.sh
> +++ b/t/t6300-for-each-ref.sh
> @@ -148,4 +148,26 @@ test_expect_success 'Check format "rfc2822" date fields output' '
>  	git diff expected actual
>  '
>  
> +cat >expected <<\EOF
> +refs/heads/master
> +refs/tags/testtag
> +EOF
> +
> +test_expect_success 'Verify ascending sort' '
> +	git-for-each-ref --format="%(refname)" --sort=refname >actual &&
> +	git diff expected actual
> +'
> +
> +
> +cat >expected <<\EOF
> +refs/tags/testtag
> +refs/heads/master
> +EOF
> +
> +test_expect_success 'Verify descending sort' '
> +	git-for-each-ref --format="%(refname)" --sort=-refname >actual &&
> +	git diff expected actual
> +'
> +
> +
>  test_done
> -- 
> 1.5.3.5.623.g0a1d

Re: [PATCH] for-each-ref: fix setup of option-parsing for --sort

[Johannes Schindelin]

Hi,

On Sat, 10 Nov 2007, Lars Hjemli wrote:

> The option value for --sort is already a pointer to a pointer to struct
> ref_sort, so just use it.
> 
> Signed-off-by: Lars Hjemli <hjemli@gmail.com>
> ---
> 
> On Nov 10, 2007 5:25 PM, Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:
> > Could you add a test for that too, please?
> 
> Is this ok?

That's exactly what I had in mind.

Thank you,
Dscho

Re: gitweb, updating 'last changed' column on the project page

[Jon Smirl]

On 11/10/07, Jakub Narebski <jnareb@gmail.com> wrote:
> > It is sorted by committerdate, the sort is ascending. Did you expect
> > it to be descending, pick off the last entry instead of the first?
>
> Excerpts from git-for-each-ref(1):
>
>   git-for-each-ref [--count=<count>]* (...) [--sort=<key>]* (...)
>
>   <count>
>           By  default  the  command  shows all refs that match <pattern>. This
>           option makes it stop after showing that many refs.
>
>    <key>  A field name to sort on. Prefix - to sort in descending order of the
>           value.  When  unspecified,  refname is used. More than one sort keys
>           can be given.
>
> So I expect --sort=-committerdate to sort by date of committing,
> descending, and --count=1 pick first one, which means most recent.

git has a bug, it is not implementing the - prefix. I am using git head.

jonsmirl@terra:~$ cd mpc5200b
jonsmirl@terra:~/mpc5200b$ git for-each-ref
--format="%(refname):%09%(committer)" --sort=-committerdate refs/heads
refs/heads/m24: Jon Smirl <jonsmirl@gmail.com> 1191362799 -0400
refs/heads/m25: Jon Smirl <jonsmirl@gmail.com> 1191472422 -0400
refs/heads/m26: Jon Smirl <jonsmirl@gmail.com> 1194382038 -0500
refs/heads/m28: Jon Smirl <jonsmirl@gmail.com> 1194385071 -0500
refs/heads/m29: Jon Smirl <jonsmirl@gmail.com> 1194674673 -0500
jonsmirl@terra:~/mpc5200b$ git for-each-ref
--format="%(refname):%09%(committer)" --sort=committerdate refs/heads
refs/heads/m24: Jon Smirl <jonsmirl@gmail.com> 1191362799 -0400
refs/heads/m25: Jon Smirl <jonsmirl@gmail.com> 1191472422 -0400
refs/heads/m26: Jon Smirl <jonsmirl@gmail.com> 1194382038 -0500
refs/heads/m28: Jon Smirl <jonsmirl@gmail.com> 1194385071 -0500
refs/heads/m29: Jon Smirl <jonsmirl@gmail.com> 1194674673 -0500
jonsmirl@terra:~/mpc5200b$ git --version
git version 1.5.3.5.1651.g30bf
jonsmirl@terra:~/mpc5200b$


>
> It looks like "your" gitweb sorts ascending instead... strange...
>
>
> How does git_get_last_activity subroutine in your gitweb.cgi looks like?
> Does it have '--sort=-commiterdate'? If it has, then I think it is some
> strange bug in git, if it doesn't it is strange modification of gitweb.
>
> HTH
> --
> Jakub Narebski
> Poland
>


-- 
Jon Smirl
jonsmirl@gmail.com

[PATCH] for-each-ref: fix setup of option-parsing for --sort

[Lars Hjemli]

The option value for --sort is already a pointer to a pointer to struct
ref_sort, so just use it.

Signed-off-by: Lars Hjemli <hjemli@gmail.com>
---
 builtin-for-each-ref.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/builtin-for-each-ref.c b/builtin-for-each-ref.c
index da8c794..e909e66 100644
--- a/builtin-for-each-ref.c
+++ b/builtin-for-each-ref.c
@@ -847,7 +847,7 @@ int cmd_for_each_ref(int argc, const char **argv, const char *prefix)
 		OPT_GROUP(""),
 		OPT_INTEGER( 0 , "count", &maxcount, "show only <n> matched refs"),
 		OPT_STRING(  0 , "format", &format, "format", "format to use for the output"),
-		OPT_CALLBACK(0 , "sort", &sort_tail, "key",
+		OPT_CALLBACK(0 , "sort", sort_tail, "key",
 		            "field name to sort on", &opt_parse_sort),
 		OPT_END(),
 	};
-- 
1.5.3.5.623.g0a1d

Re: [PATCH] for-each-ref: fix setup of option-parsing for --sort

[Johannes Schindelin]

Hi,

On Sat, 10 Nov 2007, Lars Hjemli wrote:

> The option value for --sort is already a pointer to a pointer to struct
> ref_sort, so just use it.

Could you add a test for that too, please?

Thanks,
Dscho