[RFC/PATCH 0/9] add options to ref-filter

[RFC/PATCH 0/9] add options to ref-filter

[Karthik Nayak]

This is a follow up series to the one posted here
http://thread.gmane.org/gmane.comp.version-control.git/270922

This patch series adds '--ponints-at', '--merged', '--no-merged' and
'--contain' options to 'ref-filter' and uses these options in 
'for-each-ref'.

-- 
Regards,
Karthik

[RFC/PATCH 1/9] tag: libify parse_opt_points_at()

[Karthik Nayak]

Moving 'parse_opt_points_at()' from 'tag.c' to a common location,
in preparation for using it in 'ref-filter'.

Based-on-patch-by: Jeff King <peff@peff.net>
Mentored-by: Christian Couder <christian.couder@gmail.com>
Mentored-by: Matthieu Moy <matthieu.moy@grenoble-inp.fr>
Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
 builtin/tag.c      | 19 +------------------
 parse-options-cb.c | 17 +++++++++++++++++
 parse-options.h    |  1 +
 3 files changed, 19 insertions(+), 18 deletions(-)

diff --git a/builtin/tag.c b/builtin/tag.c
index 5f6cdc5..eda76ba 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -546,23 +546,6 @@ static int strbuf_check_tag_ref(struct strbuf *sb, const char *name)
 	return check_refname_format(sb->buf, 0);
 }
 
-static int parse_opt_points_at(const struct option *opt __attribute__((unused)),
-			const char *arg, int unset)
-{
-	unsigned char sha1[20];
-
-	if (unset) {
-		sha1_array_clear(&points_at);
-		return 0;
-	}
-	if (!arg)
-		return error(_("switch 'points-at' requires an object"));
-	if (get_sha1(arg, sha1))
-		return error(_("malformed object name '%s'"), arg);
-	sha1_array_append(&points_at, sha1);
-	return 0;
-}
-
 static int parse_opt_sort(const struct option *opt, const char *arg, int unset)
 {
 	int *sort = opt->value;
@@ -625,7 +608,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
 			parse_opt_with_commit, (intptr_t)"HEAD",
 		},
 		{
-			OPTION_CALLBACK, 0, "points-at", NULL, N_("object"),
+			OPTION_CALLBACK, 0, "points-at", &points_at, N_("object"),
 			N_("print only tags of the object"), 0, parse_opt_points_at
 		},
 		OPT_END()
diff --git a/parse-options-cb.c b/parse-options-cb.c
index be8c413..a4d294d 100644
--- a/parse-options-cb.c
+++ b/parse-options-cb.c
@@ -4,6 +4,7 @@
 #include "commit.h"
 #include "color.h"
 #include "string-list.h"
+#include "sha1-array.h"
 
 /*----- some often used options -----*/
 
@@ -92,6 +93,22 @@ int parse_opt_with_commit(const struct option *opt, const char *arg, int unset)
 	return 0;
 }
 
+int parse_opt_points_at(const struct option *opt, const char *arg, int unset)
+{
+	unsigned char sha1[20];
+
+	if (unset) {
+		sha1_array_clear(opt->value);
+		return 0;
+	}
+	if (!arg)
+		return -1;
+	if (get_sha1(arg, sha1))
+		return error(_("malformed object name '%s'"), arg);
+	sha1_array_append(opt->value, sha1);
+	return 0;
+}
+
 int parse_opt_tertiary(const struct option *opt, const char *arg, int unset)
 {
 	int *target = opt->value;
diff --git a/parse-options.h b/parse-options.h
index c71e9da..3ae16a1 100644
--- a/parse-options.h
+++ b/parse-options.h
@@ -220,6 +220,7 @@ extern int parse_opt_approxidate_cb(const struct option *, const char *, int);
 extern int parse_opt_expiry_date_cb(const struct option *, const char *, int);
 extern int parse_opt_color_flag_cb(const struct option *, const char *, int);
 extern int parse_opt_verbosity_cb(const struct option *, const char *, int);
+extern int parse_opt_points_at(const struct option *, const char *, int);
 extern int parse_opt_with_commit(const struct option *, const char *, int);
 extern int parse_opt_tertiary(const struct option *, const char *, int);
 extern int parse_opt_string_list(const struct option *, const char *, int);
-- 
2.4.2

[RFC/PATCH 2/9] ref-filter: implement '--points-at' option

[Karthik Nayak]

In 'tag -l' we have '--points-at' option which lets users
list only tags which point to a particular commit, Implement
this option in 'ref-filter.{c,h}' so that the other commands
can benefit from this.

Based-on-patch-by: Jeff King <peff@peff.net>
Mentored-by: Christian Couder <christian.couder@gmail.com>
Mentored-by: Matthieu Moy <matthieu.moy@grenoble-inp.fr>
Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
 ref-filter.c | 26 ++++++++++++++++++++++++++
 ref-filter.h |  1 +
 2 files changed, 27 insertions(+)

diff --git a/ref-filter.c b/ref-filter.c
index 745d3b3..456b0fa 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -841,6 +841,29 @@ static int match_name_as_path(const char **pattern, const char *refname)
 	return 0;
 }
 
+/*
+ * Given a ref (sha1, refname) see if it points to a one of the sha1s
+ * in a sha1_array.
+ */
+static int match_points_at(struct sha1_array *points_at, const unsigned char *sha1,
+			   const char *refname)
+{
+	struct object *obj;
+
+	if (!points_at || !points_at->nr)
+		return 1;
+
+	if (sha1_array_lookup(points_at, sha1) >= 0)
+		return 1;
+
+	obj = parse_object_or_die(sha1, refname);
+	if (obj->type == OBJ_TAG &&
+	    sha1_array_lookup(points_at, ((struct tag *)obj)->tagged->sha1) >= 0)
+		return 1;
+
+	return 0;
+}
+
 /* Allocate space for a new ref_array_item and copy the objectname and flag to it */
 static struct ref_array_item *new_ref_array_item(const char *refname,
 						 const unsigned char *objectname,
@@ -874,6 +897,9 @@ int ref_filter_handler(const char *refname, const struct object_id *oid, int fla
 	if (*filter->name_patterns && !match_name_as_path(filter->name_patterns, refname))
 		return 0;
 
+	if (!match_points_at(&filter->points_at, oid->hash, refname))
+		return 0;
+
 	/*
 	 * We do not open the object yet; sort may only need refname
 	 * to do its job and the resulting list may yet to be pruned
diff --git a/ref-filter.h b/ref-filter.h
index 041a39a..a8980e7 100644
--- a/ref-filter.h
+++ b/ref-filter.h
@@ -39,6 +39,7 @@ struct ref_array {
 
 struct ref_filter {
 	const char **name_patterns;
+	struct sha1_array points_at;
 };
 
 struct ref_filter_cbdata {
-- 
2.4.2

Re: [RFC/PATCH 2/9] ref-filter: implement '--points-at' option

[Matthieu Moy]

Karthik Nayak <karthik.188@gmail.com> writes:

> +/*
> + * Given a ref (sha1, refname) see if it points to a one of the sha1s
> + * in a sha1_array.
> + */
> +static int match_points_at(struct sha1_array *points_at, const unsigned char *sha1,
> +			   const char *refname)
> +{
> +	struct object *obj;
> +
> +	if (!points_at || !points_at->nr)
> +		return 1;
> +
> +	if (sha1_array_lookup(points_at, sha1) >= 0)
> +		return 1;
> +
> +	obj = parse_object_or_die(sha1, refname);
> +	if (obj->type == OBJ_TAG &&
> +	    sha1_array_lookup(points_at, ((struct tag *)obj)->tagged->sha1) >= 0)
> +		return 1;
> +
> +	return 0;
> +}

There's a similar function in builtin/tag.c that you are not removing.
You should justify why you are doing this code duplication in the commit
message (or not do code duplication). It might make sense to add a
comment next to match_points_at in tag.c saying stg like "this is
duplicated from ..., will be removed later".

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

Re: [RFC/PATCH 2/9] ref-filter: implement '--points-at' option

[Karthik Nayak]

On 06/08/2015 11:01 PM, Matthieu Moy wrote:
> Karthik Nayak <karthik.188@gmail.com> writes:
>
>> +/*
>> + * Given a ref (sha1, refname) see if it points to a one of the sha1s
>> + * in a sha1_array.
>> + */
>> +static int match_points_at(struct sha1_array *points_at, const unsigned char *sha1,
>> +			   const char *refname)
>> +{
>> +	struct object *obj;
>> +
>> +	if (!points_at || !points_at->nr)
>> +		return 1;
>> +
>> +	if (sha1_array_lookup(points_at, sha1) >= 0)
>> +		return 1;
>> +
>> +	obj = parse_object_or_die(sha1, refname);
>> +	if (obj->type == OBJ_TAG &&
>> +	    sha1_array_lookup(points_at, ((struct tag *)obj)->tagged->sha1) >= 0)
>> +		return 1;
>> +
>> +	return 0;
>> +}
>
> There's a similar function in builtin/tag.c that you are not removing.
> You should justify why you are doing this code duplication in the commit
> message (or not do code duplication). It might make sense to add a
> comment next to match_points_at in tag.c saying stg like "this is
> duplicated from ..., will be removed later".
>

Ok will add this :)

-- 
Regards,
Karthik

Re: [RFC/PATCH 2/9] ref-filter: implement '--points-at' option

[Matthieu Moy]

Karthik Nayak <karthik.188@gmail.com> writes:

> In 'tag -l' we have '--points-at' option which lets users
> list only tags which point to a particular commit, Implement

s/,/./ ?

> this option in 'ref-filter.{c,h}' so that the other commands

s/the//

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

Re: [RFC/PATCH 2/9] ref-filter: implement '--points-at' option

[Karthik Nayak]

On 06/08/2015 11:30 PM, Matthieu Moy wrote:
> Karthik Nayak <karthik.188@gmail.com> writes:
>
>> In 'tag -l' we have '--points-at' option which lets users
>> list only tags which point to a particular commit, Implement
>
> s/,/./ ?
>
>> this option in 'ref-filter.{c,h}' so that the other commands
>
> s/the//
>

Noted! thanks

-- 
Regards,
Karthik

[RFC/PATCH 3/9] for-each-ref: add '--points-at' option

[Karthik Nayak]

Add the '--points-at' option provided by 'ref-filter'. The
option lets the user to pick only refs which point to a particular
commit.

Add Documentation for the same.

Based-on-patch-by: Jeff King <peff@peff.net>
Mentored-by: Christian Couder <christian.couder@gmail.com>
Mentored-by: Matthieu Moy <matthieu.moy@grenoble-inp.fr>
Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
 Documentation/git-for-each-ref.txt | 3 +++
 builtin/for-each-ref.c             | 6 +++++-
 2 files changed, 8 insertions(+), 1 deletion(-)

diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt
index 7f8d9a5..e9f6a8a 100644
--- a/Documentation/git-for-each-ref.txt
+++ b/Documentation/git-for-each-ref.txt
@@ -10,6 +10,7 @@ SYNOPSIS
 [verse]
 'git for-each-ref' [--count=<count>] [--shell|--perl|--python|--tcl]
 		   [(--sort=<key>)...] [--format=<format>] [<pattern>...]
+		   [--points-at <object>]
 
 DESCRIPTION
 -----------
@@ -62,6 +63,8 @@ OPTIONS
 	the specified host language.  This is meant to produce
 	a scriptlet that can directly be `eval`ed.
 
+--points-at <object>::
+	Only list tags of the given object.
 
 FIELD NAMES
 -----------
diff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c
index 4d2d024..b9d180a 100644
--- a/builtin/for-each-ref.c
+++ b/builtin/for-each-ref.c
@@ -7,6 +7,7 @@
 
 static char const * const for_each_ref_usage[] = {
 	N_("git for-each-ref [<options>] [<pattern>]"),
+	N_("git for-each-ref [--points-at <object>]"),
 	NULL
 };
 
@@ -17,6 +18,7 @@ int cmd_for_each_ref(int argc, const char **argv, const char *prefix)
 	struct ref_sorting *sorting = NULL, **sorting_tail = &sorting;
 	int maxcount = 0, quote_style = 0;
 	struct ref_filter_cbdata ref_cbdata;
+	memset(&ref_cbdata, 0, sizeof(ref_cbdata));
 
 	struct option opts[] = {
 		OPT_BIT('s', "shell", &quote_style,
@@ -33,6 +35,9 @@ int cmd_for_each_ref(int argc, const char **argv, const char *prefix)
 		OPT_STRING(  0 , "format", &format, N_("format"), N_("format to use for the output")),
 		OPT_CALLBACK(0 , "sort", sorting_tail, N_("key"),
 			    N_("field name to sort on"), &parse_opt_ref_sorting),
+		OPT_CALLBACK(0, "points-at", &ref_cbdata.filter.points_at,
+			     N_("object"), N_("print only tags of the object"),
+			     parse_opt_points_at),
 		OPT_END(),
 	};
 
@@ -54,7 +59,6 @@ int cmd_for_each_ref(int argc, const char **argv, const char *prefix)
 	/* for warn_ambiguous_refs */
 	git_config(git_default_config, NULL);
 
-	memset(&ref_cbdata, 0, sizeof(ref_cbdata));
 	ref_cbdata.filter.name_patterns = argv;
 	filter_refs(for_each_rawref, &ref_cbdata);
 
-- 
2.4.2

Re: [RFC/PATCH 3/9] for-each-ref: add '--points-at' option

[Matthieu Moy]

Karthik Nayak <karthik.188@gmail.com> writes:

> Add the '--points-at' option provided by 'ref-filter'. The
> option lets the user to pick only refs which point to a particular
> commit.
>
> Add Documentation for the same.

... but no test?

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

Re: [RFC/PATCH 3/9] for-each-ref: add '--points-at' option

[Karthik Nayak]

On 06/08/2015 11:05 PM, Matthieu Moy wrote:
> Karthik Nayak <karthik.188@gmail.com> writes:
>
>> Add the '--points-at' option provided by 'ref-filter'. The
>> option lets the user to pick only refs which point to a particular
>> commit.
>>
>> Add Documentation for the same.
>
> ... but no test?
>

No haven't written tests, this was just to ensure if the way this is 
moving forth is correct.

-- 
Regards,
Karthik

Re: [RFC/PATCH 3/9] for-each-ref: add '--points-at' option

[Junio C Hamano]

Karthik Nayak <karthik.188@gmail.com> writes:

> Add the '--points-at' option provided by 'ref-filter'. The
> option lets the user to pick only refs which point to a particular
> commit.
>
> Add Documentation for the same.
>
> Based-on-patch-by: Jeff King <peff@peff.net>
> Mentored-by: Christian Couder <christian.couder@gmail.com>
> Mentored-by: Matthieu Moy <matthieu.moy@grenoble-inp.fr>
> Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
> ---
>  Documentation/git-for-each-ref.txt | 3 +++
>  builtin/for-each-ref.c             | 6 +++++-
>  2 files changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt
> index 7f8d9a5..e9f6a8a 100644
> --- a/Documentation/git-for-each-ref.txt
> +++ b/Documentation/git-for-each-ref.txt
> @@ -10,6 +10,7 @@ SYNOPSIS
>  [verse]
>  'git for-each-ref' [--count=<count>] [--shell|--perl|--python|--tcl]
>  		   [(--sort=<key>)...] [--format=<format>] [<pattern>...]
> +		   [--points-at <object>]
>  
>  DESCRIPTION
>  -----------
> @@ -62,6 +63,8 @@ OPTIONS
>  	the specified host language.  This is meant to produce
>  	a scriptlet that can directly be `eval`ed.
>  
> +--points-at <object>::
> +	Only list tags of the given object.

Is this intended?  I would have expected if I did

	git for-each-ref --points-at master

I would get refs/heads/master and any other refs that exactly points
at that commit.

>  
>  FIELD NAMES
>  -----------
> diff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c
> index 4d2d024..b9d180a 100644
> --- a/builtin/for-each-ref.c
> +++ b/builtin/for-each-ref.c
> @@ -7,6 +7,7 @@
>  
>  static char const * const for_each_ref_usage[] = {
>  	N_("git for-each-ref [<options>] [<pattern>]"),
> +	N_("git for-each-ref [--points-at <object>]"),
>  	NULL
>  };
>  
> @@ -17,6 +18,7 @@ int cmd_for_each_ref(int argc, const char **argv, const char *prefix)
>  	struct ref_sorting *sorting = NULL, **sorting_tail = &sorting;
>  	int maxcount = 0, quote_style = 0;
>  	struct ref_filter_cbdata ref_cbdata;
> +	memset(&ref_cbdata, 0, sizeof(ref_cbdata));
>  
>  	struct option opts[] = {
>  		OPT_BIT('s', "shell", &quote_style,
> @@ -33,6 +35,9 @@ int cmd_for_each_ref(int argc, const char **argv, const char *prefix)
>  		OPT_STRING(  0 , "format", &format, N_("format"), N_("format to use for the output")),
>  		OPT_CALLBACK(0 , "sort", sorting_tail, N_("key"),
>  			    N_("field name to sort on"), &parse_opt_ref_sorting),
> +		OPT_CALLBACK(0, "points-at", &ref_cbdata.filter.points_at,
> +			     N_("object"), N_("print only tags of the object"),
> +			     parse_opt_points_at),
>  		OPT_END(),
>  	};
>  
> @@ -54,7 +59,6 @@ int cmd_for_each_ref(int argc, const char **argv, const char *prefix)
>  	/* for warn_ambiguous_refs */
>  	git_config(git_default_config, NULL);
>  
> -	memset(&ref_cbdata, 0, sizeof(ref_cbdata));

I cannot quite see how this change relates to the addition of the
new option.

>  	ref_cbdata.filter.name_patterns = argv;
>  	filter_refs(for_each_rawref, &ref_cbdata);

Re: [RFC/PATCH 3/9] for-each-ref: add '--points-at' option

[Karthik Nayak]

On 06/09/2015 12:42 AM, Junio C Hamano wrote:
>
> Is this intended?  I would have expected if I did
>
> 	git for-each-ref --points-at master
>
> I would get refs/heads/master and any other refs that exactly points
> at that commit.
>

Thats to be changed, thanks!

>>
>>   FIELD NAMES
>>   -----------
>> diff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c
>> index 4d2d024..b9d180a 100644
>> --- a/builtin/for-each-ref.c
>> +++ b/builtin/for-each-ref.c
>> @@ -7,6 +7,7 @@
>>
>>   static char const * const for_each_ref_usage[] = {
>>   	N_("git for-each-ref [<options>] [<pattern>]"),
>> +	N_("git for-each-ref [--points-at <object>]"),
>>   	NULL
>>   };
>>
>> @@ -17,6 +18,7 @@ int cmd_for_each_ref(int argc, const char **argv, const char *prefix)
>>   	struct ref_sorting *sorting = NULL, **sorting_tail = &sorting;
>>   	int maxcount = 0, quote_style = 0;
>>   	struct ref_filter_cbdata ref_cbdata;
>> +	memset(&ref_cbdata, 0, sizeof(ref_cbdata));
>>
>>   	struct option opts[] = {
>>   		OPT_BIT('s', "shell", &quote_style,
>> @@ -33,6 +35,9 @@ int cmd_for_each_ref(int argc, const char **argv, const char *prefix)
>>   		OPT_STRING(  0 , "format", &format, N_("format"), N_("format to use for the output")),
>>   		OPT_CALLBACK(0 , "sort", sorting_tail, N_("key"),
>>   			    N_("field name to sort on"), &parse_opt_ref_sorting),
>> +		OPT_CALLBACK(0, "points-at", &ref_cbdata.filter.points_at,
>> +			     N_("object"), N_("print only tags of the object"),
>> +			     parse_opt_points_at),
>>   		OPT_END(),
>>   	};
>>
>> @@ -54,7 +59,6 @@ int cmd_for_each_ref(int argc, const char **argv, const char *prefix)
>>   	/* for warn_ambiguous_refs */
>>   	git_config(git_default_config, NULL);
>>
>> -	memset(&ref_cbdata, 0, sizeof(ref_cbdata));
>
> I cannot quite see how this change relates to the addition of the
> new option.
>

Well if we memset() after calling parse_opt_points_at(), we loose all 
the information we would have obtained.
So the memset() is moved to an earlier location.

-- 
Regards,
Karthik

Re: [RFC/PATCH 3/9] for-each-ref: add '--points-at' option

[Junio C Hamano]

Karthik Nayak <karthik.188@gmail.com> writes:

>>> @@ -54,7 +59,6 @@ int cmd_for_each_ref(int argc, const char **argv, const char *prefix)
>>>   	/* for warn_ambiguous_refs */
>>>   	git_config(git_default_config, NULL);
>>>
>>> -	memset(&ref_cbdata, 0, sizeof(ref_cbdata));
>>
>> I cannot quite see how this change relates to the addition of the
>> new option.
>>
>
> Well if we memset() after calling parse_opt_points_at(), we loose all
> the information we would have obtained.
> So the memset() is moved to an earlier location.

which I did not see, because I expected the code to follow the usual
"no decl-after-statement" pattern.  IOW

>>   	int maxcount = 0, quote_style = 0;
>>   	struct ref_filter_cbdata ref_cbdata;
>> +	memset(&ref_cbdata, 0, sizeof(ref_cbdata));
>>
>>   	struct option opts[] = {
>>   		OPT_BIT('s', "shell", &quote_style,

Don't do that.  Always start your function like so:

	type funcname(args)
        {
        	declarations;

                first statement;
		...

with no blank line within declarations block and a blank line after
the declarations block.

Re: [RFC/PATCH 3/9] for-each-ref: add '--points-at' option

[Karthik Nayak]

On 06/10/2015 12:37 AM, Junio C Hamano wrote:
> Karthik Nayak <karthik.188@gmail.com> writes:
>
> >>> @@ -54,7 +59,6 @@ int cmd_for_each_ref(int argc, const char **argv, const char *prefix)
> >>>        /* for warn_ambiguous_refs */
> >>>        git_config(git_default_config, NULL);
> >>>
> >>> -    memset(&ref_cbdata, 0, sizeof(ref_cbdata));
> >>
> >> I cannot quite see how this change relates to the addition of the
> >> new option.
> >>
> >
> > Well if we memset() after calling parse_opt_points_at(), we loose all
> > the information we would have obtained.
> > So the memset() is moved to an earlier location.
>
> which I did not see, because I expected the code to follow the usual
> "no decl-after-statement" pattern.  IOW
>
> >>        int maxcount = 0, quote_style = 0;
> >>        struct ref_filter_cbdata ref_cbdata;
> >> +    memset(&ref_cbdata, 0, sizeof(ref_cbdata));
> >>
> >>        struct option opts[] = {
> >>            OPT_BIT('s', "shell", &quote_style,
>
> Don't do that.  Always start your function like so:
>
>     type funcname(args)
>          {
>              declarations;
>
>                  first statement;
>         ...
>
> with no blank line within declarations block and a blank line after
> the declarations block.
>
Will do, thanks!

-- 
Regards,
Karthik

Re: [RFC/PATCH 3/9] for-each-ref: add '--points-at' option

[Matthieu Moy]

Junio C Hamano <gitster@pobox.com> writes:

> Don't do that.  Always start your function like so:
>
> 	type funcname(args)
>         {
>         	declarations;
>
>                 first statement;
> 		...

Hint: create a file config.mak with this content:

$ cat config.mak
CFLAGS += -Wdeclaration-after-statement -Wall -Werror

and gcc will prevent you from doing this mistake again.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

Re: [RFC/PATCH 3/9] for-each-ref: add '--points-at' option

[Karthik Nayak]

On 06/10/2015 01:09 PM, Matthieu Moy wrote:
> Junio C Hamano <gitster@pobox.com> writes:
>
> > Don't do that.  Always start your function like so:
> >
> >     type funcname(args)
> >          {
> >              declarations;
> >
> >                  first statement;
> >         ...
>
> Hint: create a file config.mak with this content:
>
> $ cat config.mak
> CFLAGS += -Wdeclaration-after-statement -Wall -Werror
>
> and gcc will prevent you from doing this mistake again.
>

Thanks a lot!
Your tips are brilliant.

-- 
Regards,
Karthik

[RFC/PATCH 4/9] parse-options: add parse_opt_merge_filter()

[Karthik Nayak]

Add 'parse_opt_merge_filter()' to parse '--merged' and '--no-merged'
options and write MACROS for the same.

Based-on-patch-by: Jeff King <peff@peff.net>
Mentored-by: Christian Couder <christian.couder@gmail.com>
Mentored-by: Matthieu Moy <matthieu.moy@grenoble-inp.fr>
Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
 parse-options-cb.c | 22 ++++++++++++++++++++++
 parse-options.h    | 11 +++++++++++
 2 files changed, 33 insertions(+)

diff --git a/parse-options-cb.c b/parse-options-cb.c
index a4d294d..1969803 100644
--- a/parse-options-cb.c
+++ b/parse-options-cb.c
@@ -4,6 +4,7 @@
 #include "commit.h"
 #include "color.h"
 #include "string-list.h"
+#include "ref-filter.h"
 #include "sha1-array.h"
 
 /*----- some often used options -----*/
@@ -109,6 +110,27 @@ int parse_opt_points_at(const struct option *opt, const char *arg, int unset)
 	return 0;
 }
 
+int parse_opt_merge_filter(const struct option *opt, const char *arg, int unset)
+{
+	struct ref_filter *rf = opt->value;
+	unsigned char sha1[20];
+
+	rf->merge = opt->long_name[0] == 'n'
+		? REF_FILTER_MERGED_OMIT
+		: REF_FILTER_MERGED_INCLUDE;
+
+	if (!arg)
+		arg = "HEAD";
+	if (get_sha1(arg, sha1))
+		die(_("malformed object name %s"), arg);
+
+	rf->merge_commit = lookup_commit_reference_gently(sha1, 0);
+	if (!rf->merge_commit)
+		return opterror(opt, "must point to a commit", 0);
+
+	return 0;
+}
+
 int parse_opt_tertiary(const struct option *opt, const char *arg, int unset)
 {
 	int *target = opt->value;
diff --git a/parse-options.h b/parse-options.h
index 3ae16a1..7bcf0f3 100644
--- a/parse-options.h
+++ b/parse-options.h
@@ -221,6 +221,7 @@ extern int parse_opt_expiry_date_cb(const struct option *, const char *, int);
 extern int parse_opt_color_flag_cb(const struct option *, const char *, int);
 extern int parse_opt_verbosity_cb(const struct option *, const char *, int);
 extern int parse_opt_points_at(const struct option *, const char *, int);
+extern int parse_opt_merge_filter(const struct option *, const char *, int);
 extern int parse_opt_with_commit(const struct option *, const char *, int);
 extern int parse_opt_tertiary(const struct option *, const char *, int);
 extern int parse_opt_string_list(const struct option *, const char *, int);
@@ -243,5 +244,15 @@ extern int parse_opt_noop_cb(const struct option *, const char *, int);
 	OPT_COLOR_FLAG(0, "color", (var), (h))
 #define OPT_COLUMN(s, l, v, h) \
 	{ OPTION_CALLBACK, (s), (l), (v), N_("style"), (h), PARSE_OPT_OPTARG, parseopt_column_callback }
+#define OPT_NO_MERGED(filter, h) \
+	{ OPTION_CALLBACK, 0, "no-merged", (filter), N_("commit"), (h), \
+	  PARSE_OPT_LASTARG_DEFAULT | PARSE_OPT_NONEG, \
+	  parse_opt_merge_filter, (intptr_t) "HEAD" \
+	}
+#define OPT_MERGED(filter, h) \
+	{ OPTION_CALLBACK, 0, "merged", (filter), N_("commit"), (h), \
+	  PARSE_OPT_LASTARG_DEFAULT | PARSE_OPT_NONEG, \
+	  parse_opt_merge_filter, (intptr_t) "HEAD" \
+	}
 
 #endif
-- 
2.4.2

Re: [RFC/PATCH 4/9] parse-options: add parse_opt_merge_filter()

[Matthieu Moy]

Karthik Nayak <karthik.188@gmail.com> writes:

> +int parse_opt_merge_filter(const struct option *opt, const char *arg, int unset)
> +{
> +	struct ref_filter *rf = opt->value;
> +	unsigned char sha1[20];
> +
> +	rf->merge = opt->long_name[0] == 'n'
> +		? REF_FILTER_MERGED_OMIT
> +		: REF_FILTER_MERGED_INCLUDE;

I would use starts_with("no-", opt->long_name) instead. I had a hard
time understanding why the letter 'n' was special while the
starts_with() version is self-explanatory.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

Re: [RFC/PATCH 4/9] parse-options: add parse_opt_merge_filter()

[Karthik Nayak]

On 06/08/2015 11:28 PM, Matthieu Moy wrote:
> Karthik Nayak <karthik.188@gmail.com> writes:
>
>> +int parse_opt_merge_filter(const struct option *opt, const char *arg, int unset)
>> +{
>> +	struct ref_filter *rf = opt->value;
>> +	unsigned char sha1[20];
>> +
>> +	rf->merge = opt->long_name[0] == 'n'
>> +		? REF_FILTER_MERGED_OMIT
>> +		: REF_FILTER_MERGED_INCLUDE;
>
> I would use starts_with("no-", opt->long_name) instead. I had a hard
> time understanding why the letter 'n' was special while the
> starts_with() version is self-explanatory.
>

Can do that also :)

-- 
Regards,
Karthik

Re: [RFC/PATCH 4/9] parse-options: add parse_opt_merge_filter()

[Junio C Hamano]

Karthik Nayak <karthik.188@gmail.com> writes:

> +int parse_opt_merge_filter(const struct option *opt, const char *arg, int unset)
> +{
> +	struct ref_filter *rf = opt->value;
> +	unsigned char sha1[20];
> +
> +	rf->merge = opt->long_name[0] == 'n'
> +		? REF_FILTER_MERGED_OMIT
> +		: REF_FILTER_MERGED_INCLUDE;
> +
> +	if (!arg)
> +		arg = "HEAD";
> +	if (get_sha1(arg, sha1))
> +		die(_("malformed object name %s"), arg);
> +
> +	rf->merge_commit = lookup_commit_reference_gently(sha1, 0);
> +	if (!rf->merge_commit)
> +		return opterror(opt, "must point to a commit", 0);
> +
> +	return 0;
> +}

Again, this smells too specific to live as a part of parse-options
infrastructure.  If we want to have two helper callbacks, one that
gives the results in an sha1-array (because there is no guarantee
that you want only commits) and in a commit-list, I am fine with
having parse_opt_object_name() and parse_opt_with_commit().  Perhaps
rename the latter which was named too specifically to something more
sensible (e.g. parse_opt_commit_object_name()) and use it from the
caller you wanted to use parse_opt_merge_filter()?  The caller, if
it is not prepared to see more than one commits specified, may have
to check if (!list || !list->next) { die("I want one and only one") }
or something, though.

Having it in ref-filter.h as parse_opt_merge_filter() is fine,
though.  After all, you would be sharing it with for-each-ref,
branch and tag and nobody else anyway.

> diff --git a/parse-options.h b/parse-options.h
> index 3ae16a1..7bcf0f3 100644
> --- a/parse-options.h
> +++ b/parse-options.h
> @@ -221,6 +221,7 @@ extern int parse_opt_expiry_date_cb(const struct option *, const char *, int);
>  extern int parse_opt_color_flag_cb(const struct option *, const char *, int);
>  extern int parse_opt_verbosity_cb(const struct option *, const char *, int);
>  extern int parse_opt_points_at(const struct option *, const char *, int);
> +extern int parse_opt_merge_filter(const struct option *, const char *, int);
>  extern int parse_opt_with_commit(const struct option *, const char *, int);
>  extern int parse_opt_tertiary(const struct option *, const char *, int);
>  extern int parse_opt_string_list(const struct option *, const char *, int);
> @@ -243,5 +244,15 @@ extern int parse_opt_noop_cb(const struct option *, const char *, int);
>  	OPT_COLOR_FLAG(0, "color", (var), (h))
>  #define OPT_COLUMN(s, l, v, h) \
>  	{ OPTION_CALLBACK, (s), (l), (v), N_("style"), (h), PARSE_OPT_OPTARG, parseopt_column_callback }
> +#define OPT_NO_MERGED(filter, h) \
> +	{ OPTION_CALLBACK, 0, "no-merged", (filter), N_("commit"), (h), \
> +	  PARSE_OPT_LASTARG_DEFAULT | PARSE_OPT_NONEG, \
> +	  parse_opt_merge_filter, (intptr_t) "HEAD" \
> +	}
> +#define OPT_MERGED(filter, h) \
> +	{ OPTION_CALLBACK, 0, "merged", (filter), N_("commit"), (h), \
> +	  PARSE_OPT_LASTARG_DEFAULT | PARSE_OPT_NONEG, \
> +	  parse_opt_merge_filter, (intptr_t) "HEAD" \
> +	}

Likewise.

Re: [RFC/PATCH 4/9] parse-options: add parse_opt_merge_filter()

[Karthik Nayak]

On 06/09/2015 12:50 AM, Junio C Hamano wrote:
> Karthik Nayak <karthik.188@gmail.com> writes:
>
>> +int parse_opt_merge_filter(const struct option *opt, const char *arg, int unset)
>> +{
>> +	struct ref_filter *rf = opt->value;
>> +	unsigned char sha1[20];
>> +
>> +	rf->merge = opt->long_name[0] == 'n'
>> +		? REF_FILTER_MERGED_OMIT
>> +		: REF_FILTER_MERGED_INCLUDE;
>> +
>> +	if (!arg)
>> +		arg = "HEAD";
>> +	if (get_sha1(arg, sha1))
>> +		die(_("malformed object name %s"), arg);
>> +
>> +	rf->merge_commit = lookup_commit_reference_gently(sha1, 0);
>> +	if (!rf->merge_commit)
>> +		return opterror(opt, "must point to a commit", 0);
>> +
>> +	return 0;
>> +}
>
> Again, this smells too specific to live as a part of parse-options
> infrastructure.  If we want to have two helper callbacks, one that
> gives the results in an sha1-array (because there is no guarantee
> that you want only commits) and in a commit-list, I am fine with
> having parse_opt_object_name() and parse_opt_with_commit().  Perhaps
> rename the latter which was named too specifically to something more
> sensible (e.g. parse_opt_commit_object_name()) and use it from the
> caller you wanted to use parse_opt_merge_filter()?  The caller, if
> it is not prepared to see more than one commits specified, may have
> to check if (!list || !list->next) { die("I want one and only one") }
> or something, though.
>
> Having it in ref-filter.h as parse_opt_merge_filter() is fine,
> though.  After all, you would be sharing it with for-each-ref,
> branch and tag and nobody else anyway.
 >

I guess it's better left in ref-filter.h. We could like you said make
it depend on parse_opt_with_commit() but that again means we need to 
check if more than one commits are specified. So I think it would be 
better to have it in ref-filter.h
>
>> diff --git a/parse-options.h b/parse-options.h
>> index 3ae16a1..7bcf0f3 100644
>> --- a/parse-options.h
>> +++ b/parse-options.h
>> @@ -221,6 +221,7 @@ extern int parse_opt_expiry_date_cb(const struct option *, const char *, int);
>>   extern int parse_opt_color_flag_cb(const struct option *, const char *, int);
>>   extern int parse_opt_verbosity_cb(const struct option *, const char *, int);
>>   extern int parse_opt_points_at(const struct option *, const char *, int);
>> +extern int parse_opt_merge_filter(const struct option *, const char *, int);
>>   extern int parse_opt_with_commit(const struct option *, const char *, int);
>>   extern int parse_opt_tertiary(const struct option *, const char *, int);
>>   extern int parse_opt_string_list(const struct option *, const char *, int);
>> @@ -243,5 +244,15 @@ extern int parse_opt_noop_cb(const struct option *, const char *, int);
>>   	OPT_COLOR_FLAG(0, "color", (var), (h))
>>   #define OPT_COLUMN(s, l, v, h) \
>>   	{ OPTION_CALLBACK, (s), (l), (v), N_("style"), (h), PARSE_OPT_OPTARG, parseopt_column_callback }
>> +#define OPT_NO_MERGED(filter, h) \
>> +	{ OPTION_CALLBACK, 0, "no-merged", (filter), N_("commit"), (h), \
>> +	  PARSE_OPT_LASTARG_DEFAULT | PARSE_OPT_NONEG, \
>> +	  parse_opt_merge_filter, (intptr_t) "HEAD" \
>> +	}
>> +#define OPT_MERGED(filter, h) \
>> +	{ OPTION_CALLBACK, 0, "merged", (filter), N_("commit"), (h), \
>> +	  PARSE_OPT_LASTARG_DEFAULT | PARSE_OPT_NONEG, \
>> +	  parse_opt_merge_filter, (intptr_t) "HEAD" \
>> +	}
>
> Likewise.
>

This too would be better off in ref-filter.h

-- 
Regards,
Karthik

[RFC/PATCH 5/9] ref-filter: implement '--merged' and '--no-merged' options

[Karthik Nayak]

In 'branch -l' we have '--merged' option which only lists refs (branches)
merged into the named commit and '--no-merged' option which only lists
refs (branches) not merged into the named commit will be listed. Implement
these two options in ref-filter.{c,h} so that other commands can benefit
from this.

Based-on-patch-by: Jeff King <peff@peff.net>
Mentored-by: Christian Couder <christian.couder@gmail.com>
Mentored-by: Matthieu Moy <matthieu.moy@grenoble-inp.fr>
Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
 ref-filter.c | 61 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 ref-filter.h |  8 ++++++++
 2 files changed, 68 insertions(+), 1 deletion(-)

diff --git a/ref-filter.c b/ref-filter.c
index 456b0fa..2be9df2 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -9,6 +9,7 @@
 #include "tag.h"
 #include "quote.h"
 #include "ref-filter.h"
+#include "revision.h"
 
 typedef enum { FIELD_STR, FIELD_ULONG, FIELD_TIME } cmp_type;
 
@@ -888,6 +889,7 @@ int ref_filter_handler(const char *refname, const struct object_id *oid, int fla
 	struct ref_filter_cbdata *ref_cbdata = cb_data;
 	struct ref_filter *filter = &ref_cbdata->filter;
 	struct ref_array_item *ref;
+	struct commit *commit = NULL;
 
 	if (flag & REF_BAD_NAME) {
 		  warning("ignoring ref with broken name %s", refname);
@@ -900,12 +902,19 @@ int ref_filter_handler(const char *refname, const struct object_id *oid, int fla
 	if (!match_points_at(&filter->points_at, oid->hash, refname))
 		return 0;
 
+	if(filter->merge_commit) {
+		commit = lookup_commit_reference_gently(sha1, 1);
+		if (!commit)
+			return 0;
+	}
+
 	/*
 	 * We do not open the object yet; sort may only need refname
 	 * to do its job and the resulting list may yet to be pruned
 	 * by maxcount logic.
 	 */
 	ref = new_ref_array_item(refname, oid->hash, flag);
+	ref->commit = commit;
 
 	REALLOC_ARRAY(ref_cbdata->array.items, ref_cbdata->array.nr + 1);
 	ref_cbdata->array.items[ref_cbdata->array.nr++] = ref;
@@ -931,6 +940,50 @@ void ref_array_clear(struct ref_array *array)
 	array->nr = array->alloc = 0;
 }
 
+static void do_merge_filter(struct ref_filter_cbdata *ref_cbdata)
+{
+	struct rev_info revs;
+	int i, old_nr;
+	struct ref_filter *filter = &ref_cbdata->filter;
+	struct ref_array *array = &ref_cbdata->array;
+	struct commit_list *p, *to_clear = NULL;
+
+	init_revisions(&revs, NULL);
+
+	for (i = 0; i < array->nr; i++) {
+		struct ref_array_item *item = array->items[i];
+		add_pending_object(&revs, &item->commit->object, item->refname);
+		commit_list_insert(item->commit, &to_clear);
+	}
+
+	filter->merge_commit->object.flags |= UNINTERESTING;
+	add_pending_object(&revs, &filter->merge_commit->object, "");
+
+	revs.limited = 1;
+	if (prepare_revision_walk(&revs))
+		die(_("revision walk setup failed"));
+
+	old_nr = array->nr;
+	array->nr = 0;
+
+	for (i = 0; i < old_nr; i++) {
+		struct ref_array_item *item = array->items[i];
+		struct commit *commit = item->commit;
+
+		int is_merged = !!(commit->object.flags & UNINTERESTING);
+
+		if (is_merged == (filter->merge == REF_FILTER_MERGED_INCLUDE))
+			array->items[array->nr++] = array->items[i];
+		else
+			free_array_item(item);
+	}
+
+	for (p = to_clear; p; p = p->next)
+		clear_commit_marks(p->item, ALL_REV_FLAGS);
+	clear_commit_marks(filter->merge_commit, ALL_REV_FLAGS);
+	free_commit_list(to_clear);
+}
+
 /*
  * API for filtering a set of refs. The refs are provided and iterated
  * over using the for_each_ref_fn(). The refs are stored into and filtered
@@ -938,7 +991,13 @@ void ref_array_clear(struct ref_array *array)
  */
 int filter_refs(int (for_each_ref_fn)(each_ref_fn, void *), struct ref_filter_cbdata *data)
 {
-	return for_each_ref_fn(ref_filter_handler, data);
+	int ret;
+
+	ret = for_each_ref_fn(ref_filter_handler, data);
+	if (data->filter.merge_commit)
+		do_merge_filter(data);
+
+	return ret;
 }
 
 static int cmp_ref_sorting(struct ref_sorting *s, struct ref_array_item *a, struct ref_array_item *b)
diff --git a/ref-filter.h b/ref-filter.h
index a8980e7..622b942 100644
--- a/ref-filter.h
+++ b/ref-filter.h
@@ -28,6 +28,7 @@ struct ref_array_item {
 	unsigned char objectname[20];
 	int flag;
 	char *symref;
+	struct commit *commit;
 	struct atom_value *value;
 	char refname[FLEX_ARRAY];
 };
@@ -40,6 +41,13 @@ struct ref_array {
 struct ref_filter {
 	const char **name_patterns;
 	struct sha1_array points_at;
+
+	enum {
+		REF_FILTER_MERGED_NONE = 0,
+		REF_FILTER_MERGED_INCLUDE,
+		REF_FILTER_MERGED_OMIT
+	} merge;
+	struct commit *merge_commit;
 };
 
 struct ref_filter_cbdata {
-- 
2.4.2

Re: [RFC/PATCH 5/9] ref-filter: implement '--merged' and '--no-merged' options

[Matthieu Moy]

Karthik Nayak <karthik.188@gmail.com> writes:

> +	if(filter->merge_commit) {

space after if.

> @@ -938,7 +991,13 @@ void ref_array_clear(struct ref_array *array)
>   */
>  int filter_refs(int (for_each_ref_fn)(each_ref_fn, void *), struct ref_filter_cbdata *data)
>  {
> -	return for_each_ref_fn(ref_filter_handler, data);
> +	int ret;
> +
> +	ret = for_each_ref_fn(ref_filter_handler, data);
> +	if (data->filter.merge_commit)
> +		do_merge_filter(data);

Reading this, I first wondered why you did not do the merge_filter as
part of ref_filter_handler. It seems weird to fill-in an array and then
re-filter it. I think it would make sense to add a few comments, like

/* Simple per-ref filtering */
ret = for_each_ref_fn(ref_filter_handler, data);

/* Filters that need revision walking */
if (data->filter.merge_commit)
...

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

Re: [RFC/PATCH 5/9] ref-filter: implement '--merged' and '--no-merged' options

[Karthik Nayak]

On 06/08/2015 11:21 PM, Matthieu Moy wrote:
> Karthik Nayak <karthik.188@gmail.com> writes:
>
>> +	if(filter->merge_commit) {
>
> space after if.
>
>> @@ -938,7 +991,13 @@ void ref_array_clear(struct ref_array *array)
>>    */
>>   int filter_refs(int (for_each_ref_fn)(each_ref_fn, void *), struct ref_filter_cbdata *data)
>>   {
>> -	return for_each_ref_fn(ref_filter_handler, data);
>> +	int ret;
>> +
>> +	ret = for_each_ref_fn(ref_filter_handler, data);
>> +	if (data->filter.merge_commit)
>> +		do_merge_filter(data);
>
> Reading this, I first wondered why you did not do the merge_filter as
> part of ref_filter_handler. It seems weird to fill-in an array and then
> re-filter it. I think it would make sense to add a few comments, like
>
> /* Simple per-ref filtering */
> ret = for_each_ref_fn(ref_filter_handler, data);
>
> /* Filters that need revision walking */
> if (data->filter.merge_commit)
> ...
>

Thanks! will add the space and comments as suggested.

-- 
Regards,
Karthik

[RFC/PATCH 6/9] for-each-ref: add '--merged' and '--no-merged' options

[Karthik Nayak]

Add the '--merged' and '--no-merged' options provided by 'ref-filter'.
The '--merged' option lets the user to only list refs merged into the
named commit. The '--no-merged' option lets the user to only list refs
not merged into the named commit.

Add documentation for the same.

Based-on-patch-by: Jeff King <peff@peff.net>
Mentored-by: Christian Couder <christian.couder@gmail.com>
Mentored-by: Matthieu Moy <matthieu.moy@grenoble-inp.fr>
Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
 Documentation/git-for-each-ref.txt | 10 +++++++++-
 builtin/for-each-ref.c             |  3 +++
 2 files changed, 12 insertions(+), 1 deletion(-)

diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt
index e9f6a8a..74f24f4 100644
--- a/Documentation/git-for-each-ref.txt
+++ b/Documentation/git-for-each-ref.txt
@@ -10,7 +10,7 @@ SYNOPSIS
 [verse]
 'git for-each-ref' [--count=<count>] [--shell|--perl|--python|--tcl]
 		   [(--sort=<key>)...] [--format=<format>] [<pattern>...]
-		   [--points-at <object>]
+		   [--points-at <object>] [(--merged | --no-merged) <object>]
 
 DESCRIPTION
 -----------
@@ -66,6 +66,14 @@ OPTIONS
 --points-at <object>::
 	Only list tags of the given object.
 
+--merged [<commit>]::
+	Only list refs whose tips are reachable from the
+	specified commit (HEAD if not specified).
+
+--no-merged [<commit>]::
+	Only list refs whose tips are not reachable from the
+	specified commit (HEAD if not specified).
+
 FIELD NAMES
 -----------
 
diff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c
index b9d180a..82605ed 100644
--- a/builtin/for-each-ref.c
+++ b/builtin/for-each-ref.c
@@ -8,6 +8,7 @@
 static char const * const for_each_ref_usage[] = {
 	N_("git for-each-ref [<options>] [<pattern>]"),
 	N_("git for-each-ref [--points-at <object>]"),
+	N_("git for-each-ref [(--merged | --no-merged) <object>]"),
 	NULL
 };
 
@@ -38,6 +39,8 @@ int cmd_for_each_ref(int argc, const char **argv, const char *prefix)
 		OPT_CALLBACK(0, "points-at", &ref_cbdata.filter.points_at,
 			     N_("object"), N_("print only tags of the object"),
 			     parse_opt_points_at),
+		OPT_MERGED(&ref_cbdata.filter, N_("print only merged refs")),
+		OPT_NO_MERGED(&ref_cbdata.filter, N_("print only not merged refs")),
 		OPT_END(),
 	};
 
-- 
2.4.2

Re: [RFC/PATCH 6/9] for-each-ref: add '--merged' and '--no-merged' options

[Matthieu Moy]

Karthik Nayak <karthik.188@gmail.com> writes:

> Add the '--merged' and '--no-merged' options provided by 'ref-filter'.
> The '--merged' option lets the user to only list refs merged into the
> named commit. The '--no-merged' option lets the user to only list refs
> not merged into the named commit.
>
> Add documentation for the same.

This also requires some tests. The algorithmic part will be validated by
'git tag --merged' using this library, but you need to check the
codepath from the command-line to the lib in for-each-ref.

> @@ -38,6 +39,8 @@ int cmd_for_each_ref(int argc, const char **argv, const char *prefix)
>  		OPT_CALLBACK(0, "points-at", &ref_cbdata.filter.points_at,
>  			     N_("object"), N_("print only tags of the object"),
>  			     parse_opt_points_at),
> +		OPT_MERGED(&ref_cbdata.filter, N_("print only merged refs")),
> +		OPT_NO_MERGED(&ref_cbdata.filter, N_("print only not merged refs")),

I'd spell that "only refs that are not merged".

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

Re: [RFC/PATCH 6/9] for-each-ref: add '--merged' and '--no-merged' options

[Karthik Nayak]

On 06/08/2015 11:23 PM, Matthieu Moy wrote:
> Karthik Nayak <karthik.188@gmail.com> writes:
>
>> Add the '--merged' and '--no-merged' options provided by 'ref-filter'.
>> The '--merged' option lets the user to only list refs merged into the
>> named commit. The '--no-merged' option lets the user to only list refs
>> not merged into the named commit.
>>
>> Add documentation for the same.
>
> This also requires some tests. The algorithmic part will be validated by
> 'git tag --merged' using this library, but you need to check the
> codepath from the command-line to the lib in for-each-ref.
>
>> @@ -38,6 +39,8 @@ int cmd_for_each_ref(int argc, const char **argv, const char *prefix)
>>   		OPT_CALLBACK(0, "points-at", &ref_cbdata.filter.points_at,
>>   			     N_("object"), N_("print only tags of the object"),
>>   			     parse_opt_points_at),
>> +		OPT_MERGED(&ref_cbdata.filter, N_("print only merged refs")),
>> +		OPT_NO_MERGED(&ref_cbdata.filter, N_("print only not merged refs")),
>
> I'd spell that "only refs that are not merged".
>

Like I mentioned, no tests were written.
Will change that.

-- 
Regards,
Karthik

[RFC/PATCH 7/9] parse-options.h: add macros for '--contains' option

[Karthik Nayak]

Add a macro for using the '--contains' option in parse-options.h
also include an optional '--with' option macro which performs the
same action as '--contains'.

Make tag.c use this new macro

Mentored-by: Christian Couder <christian.couder@gmail.com>
Mentored-by: Matthieu Moy <matthieu.moy@grenoble-inp.fr>
Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
 builtin/tag.c   | 14 ++------------
 parse-options.h | 10 ++++++++++
 2 files changed, 12 insertions(+), 12 deletions(-)

diff --git a/builtin/tag.c b/builtin/tag.c
index eda76ba..e16668b 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -591,23 +591,13 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
 
 		OPT_GROUP(N_("Tag listing options")),
 		OPT_COLUMN(0, "column", &colopts, N_("show tag list in columns")),
+		OPT_CONTAINS(&with_commit, N_("print only tags that contain the commit")),
+		OPT_WITH(&with_commit, N_("print only tags that contain the commit")),
 		{
 			OPTION_CALLBACK, 0, "sort", &tag_sort, N_("type"), N_("sort tags"),
 			PARSE_OPT_NONEG, parse_opt_sort
 		},
 		{
-			OPTION_CALLBACK, 0, "contains", &with_commit, N_("commit"),
-			N_("print only tags that contain the commit"),
-			PARSE_OPT_LASTARG_DEFAULT,
-			parse_opt_with_commit, (intptr_t)"HEAD",
-		},
-		{
-			OPTION_CALLBACK, 0, "with", &with_commit, N_("commit"),
-			N_("print only tags that contain the commit"),
-			PARSE_OPT_HIDDEN | PARSE_OPT_LASTARG_DEFAULT,
-			parse_opt_with_commit, (intptr_t)"HEAD",
-		},
-		{
 			OPTION_CALLBACK, 0, "points-at", &points_at, N_("object"),
 			N_("print only tags of the object"), 0, parse_opt_points_at
 		},
diff --git a/parse-options.h b/parse-options.h
index 7bcf0f3..d8389ad 100644
--- a/parse-options.h
+++ b/parse-options.h
@@ -254,5 +254,15 @@ extern int parse_opt_noop_cb(const struct option *, const char *, int);
 	  PARSE_OPT_LASTARG_DEFAULT | PARSE_OPT_NONEG, \
 	  parse_opt_merge_filter, (intptr_t) "HEAD" \
 	}
+#define OPT_CONTAINS(filter, h) \
+	{ OPTION_CALLBACK, 0, "contains", (filter), N_("commit"), (h), \
+	  PARSE_OPT_LASTARG_DEFAULT, \
+	  parse_opt_with_commit, (intptr_t) "HEAD" \
+	}
+#define OPT_WITH(filter, h) \
+	{ OPTION_CALLBACK, 0, "with", (filter), N_("commit"), (h), \
+	  PARSE_OPT_LASTARG_DEFAULT, \
+	  parse_opt_with_commit, (intptr_t) "HEAD" \
+	}
 
 #endif
-- 
2.4.2

Re: [RFC/PATCH 7/9] parse-options.h: add macros for '--contains' option

[Junio C Hamano]

Karthik Nayak <karthik.188@gmail.com> writes:

> +#define OPT_CONTAINS(filter, h) \
> +	{ OPTION_CALLBACK, 0, "contains", (filter), N_("commit"), (h), \
> +	  PARSE_OPT_LASTARG_DEFAULT, \
> +	  parse_opt_with_commit, (intptr_t) "HEAD" \
> +	}
> +#define OPT_WITH(filter, h) \
> +	{ OPTION_CALLBACK, 0, "with", (filter), N_("commit"), (h), \
> +	  PARSE_OPT_LASTARG_DEFAULT, \
> +	  parse_opt_with_commit, (intptr_t) "HEAD" \
> +	}

The redundancy bothers me.  Can't we do a bit better than that,
perhaps like this?

#define _OPT_CONTAINS_OR_WITH(name, variable, help) \
	{ OPTION_CALLBACK, 0, name, (variable), N_("commit"), (help), \
	  PARSE_OPT_LASTARG_DEFAULT, \
	  parse_opt_with_commit, (intptr_t) "HEAD" \
	}
#define OPT_CONTAINS(v, h) _OPT_CONTAINS_OR_WITH("contains", v, h)

Re: [RFC/PATCH 7/9] parse-options.h: add macros for '--contains' option

[Karthik Nayak]

On 06/09/2015 01:02 AM, Junio C Hamano wrote:
> Karthik Nayak <karthik.188@gmail.com> writes:
>
>> +#define OPT_CONTAINS(filter, h) \
>> +	{ OPTION_CALLBACK, 0, "contains", (filter), N_("commit"), (h), \
>> +	  PARSE_OPT_LASTARG_DEFAULT, \
>> +	  parse_opt_with_commit, (intptr_t) "HEAD" \
>> +	}
>> +#define OPT_WITH(filter, h) \
>> +	{ OPTION_CALLBACK, 0, "with", (filter), N_("commit"), (h), \
>> +	  PARSE_OPT_LASTARG_DEFAULT, \
>> +	  parse_opt_with_commit, (intptr_t) "HEAD" \
>> +	}
>
> The redundancy bothers me.  Can't we do a bit better than that,
> perhaps like this?
>
> #define _OPT_CONTAINS_OR_WITH(name, variable, help) \
> 	{ OPTION_CALLBACK, 0, name, (variable), N_("commit"), (help), \
> 	  PARSE_OPT_LASTARG_DEFAULT, \
> 	  parse_opt_with_commit, (intptr_t) "HEAD" \
> 	}
> #define OPT_CONTAINS(v, h) _OPT_CONTAINS_OR_WITH("contains", v, h)
>

This seems better, thanks!

-- 
Regards,
Karthik

[RFC/PATCH 8/9] ref-filter: add '--contains' option

[Karthik Nayak]

'tag -l' and 'branch -l' have two different ways of finding
out if a certain ref contains a commit. Implement both these
methods in ref-filter and give the caller of ref-filter API
the option to pick which implementation to be used.

'branch -l' uses 'is_descendant_of()' from commit.c which is
left as the default implementation to be used.

'tag -l' uses a more specific algorithm since ffc4b80. This
implementation is used whenever the 'with_commit_tag_algo' bit
is set in 'struct ref_filter'.

Mentored-by: Christian Couder <christian.couder@gmail.com>
Mentored-by: Matthieu Moy <matthieu.moy@grenoble-inp.fr>
Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
 ref-filter.c | 114 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++--
 ref-filter.h |   3 ++
 2 files changed, 115 insertions(+), 2 deletions(-)

diff --git a/ref-filter.c b/ref-filter.c
index 2be9df2..6c5f5c2 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -818,6 +818,113 @@ static void get_ref_atom_value(struct ref_array_item *ref, int atom, struct atom
 	*v = &ref->value[atom];
 }
 
+enum contains_result {
+	CONTAINS_UNKNOWN = -1,
+	CONTAINS_NO = 0,
+	CONTAINS_YES = 1
+};
+
+/*
+ * Mimicking the real stack, this stack lives on the heap, avoiding stack
+ * overflows.
+ *
+ * At each recursion step, the stack items points to the commits whose
+ * ancestors are to be inspected.
+ */
+struct stack {
+	int nr, alloc;
+	struct stack_entry {
+		struct commit *commit;
+		struct commit_list *parents;
+	} *stack;
+};
+
+static int in_commit_list(const struct commit_list *want, struct commit *c)
+{
+	for (; want; want = want->next)
+		if (!hashcmp(want->item->object.sha1, c->object.sha1))
+			return 1;
+	return 0;
+}
+
+/*
+ * Test whether the candidate or one of its parents is contained in the list.
+ * Do not recurse to find out, though, but return -1 if inconclusive.
+ */
+static enum contains_result contains_test(struct commit *candidate,
+			    const struct commit_list *want)
+{
+	/* was it previously marked as containing a want commit? */
+	if (candidate->object.flags & TMP_MARK)
+		return 1;
+	/* or marked as not possibly containing a want commit? */
+	if (candidate->object.flags & UNINTERESTING)
+		return 0;
+	/* or are we it? */
+	if (in_commit_list(want, candidate)) {
+		candidate->object.flags |= TMP_MARK;
+		return 1;
+	}
+
+	if (parse_commit(candidate) < 0)
+		return 0;
+
+	return -1;
+}
+
+static void push_to_stack(struct commit *candidate, struct stack *stack)
+{
+	ALLOC_GROW(stack->stack, stack->nr + 1, stack->alloc);
+	stack->stack[stack->nr].commit = candidate;
+	stack->stack[stack->nr++].parents = candidate->parents;
+}
+
+static enum contains_result contains_tag_algo(struct commit *candidate,
+		const struct commit_list *want)
+{
+	struct stack stack = { 0, 0, NULL };
+	int result = contains_test(candidate, want);
+
+	if (result != CONTAINS_UNKNOWN)
+		return result;
+
+	push_to_stack(candidate, &stack);
+	while (stack.nr) {
+		struct stack_entry *entry = &stack.stack[stack.nr - 1];
+		struct commit *commit = entry->commit;
+		struct commit_list *parents = entry->parents;
+
+		if (!parents) {
+			commit->object.flags |= UNINTERESTING;
+			stack.nr--;
+		}
+		/*
+		 * If we just popped the stack, parents->item has been marked,
+		 * therefore contains_test will return a meaningful 0 or 1.
+		 */
+		else switch (contains_test(parents->item, want)) {
+		case CONTAINS_YES:
+			commit->object.flags |= TMP_MARK;
+			stack.nr--;
+			break;
+		case CONTAINS_NO:
+			entry->parents = parents->next;
+			break;
+		case CONTAINS_UNKNOWN:
+			push_to_stack(parents->item, &stack);
+			break;
+		}
+	}
+	free(stack.stack);
+	return contains_test(candidate, want);
+}
+
+static int commit_contains(struct ref_filter *filter, struct commit *commit)
+{
+	if (filter->with_commit_tag_algo)
+		return contains_tag_algo(commit, filter->with_commit);
+	return is_descendant_of(commit, filter->with_commit);
+}
 /*
  * Given a refname, return 1 if the refname matches with one of the patterns
  * while the pattern is a pathname like 'refs/tags' or 'refs/heads/master'
@@ -902,10 +1009,13 @@ int ref_filter_handler(const char *refname, const struct object_id *oid, int fla
 	if (!match_points_at(&filter->points_at, oid->hash, refname))
 		return 0;
 
-	if(filter->merge_commit) {
-		commit = lookup_commit_reference_gently(sha1, 1);
+	if(filter->merge_commit || filter->with_commit) {
+		commit = lookup_commit_reference_gently(oid->hash, 1);
 		if (!commit)
 			return 0;
+		if(filter->with_commit &&
+		   !commit_contains(filter, commit))
+			return 0;
 	}
 
 	/*
diff --git a/ref-filter.h b/ref-filter.h
index 622b942..c71704e 100644
--- a/ref-filter.h
+++ b/ref-filter.h
@@ -41,6 +41,7 @@ struct ref_array {
 struct ref_filter {
 	const char **name_patterns;
 	struct sha1_array points_at;
+	struct commit_list *with_commit;
 
 	enum {
 		REF_FILTER_MERGED_NONE = 0,
@@ -48,6 +49,8 @@ struct ref_filter {
 		REF_FILTER_MERGED_OMIT
 	} merge;
 	struct commit *merge_commit;
+
+	unsigned int with_commit_tag_algo: 1;
 };
 
 struct ref_filter_cbdata {
-- 
2.4.2

[RFC/PATCH 9/9] for-each-ref: add '--contains' option

[Karthik Nayak]

Add the '--contains' option provided by 'ref-filter'. The '--contains'
lists only refs which are contain the specific commit (HEAD of the
branch if no commit give).

Add documentation for the same.

Mentored-by: Christian Couder <christian.couder@gmail.com>
Mentored-by: Matthieu Moy <matthieu.moy@grenoble-inp.fr>
Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
 Documentation/git-for-each-ref.txt | 5 +++++
 builtin/for-each-ref.c             | 2 ++
 2 files changed, 7 insertions(+)

diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt
index 74f24f4..7ac64ea 100644
--- a/Documentation/git-for-each-ref.txt
+++ b/Documentation/git-for-each-ref.txt
@@ -11,6 +11,7 @@ SYNOPSIS
 'git for-each-ref' [--count=<count>] [--shell|--perl|--python|--tcl]
 		   [(--sort=<key>)...] [--format=<format>] [<pattern>...]
 		   [--points-at <object>] [(--merged | --no-merged) <object>]
+		   [--contains <object>]
 
 DESCRIPTION
 -----------
@@ -74,6 +75,10 @@ OPTIONS
 	Only list refs whose tips are not reachable from the
 	specified commit (HEAD if not specified).
 
+--contains [<commit>]::
+	Only list tags which contain the specified commit (HEAD if not
+	specified).
+
 FIELD NAMES
 -----------
 
diff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c
index 82605ed..44e1467 100644
--- a/builtin/for-each-ref.c
+++ b/builtin/for-each-ref.c
@@ -9,6 +9,7 @@ static char const * const for_each_ref_usage[] = {
 	N_("git for-each-ref [<options>] [<pattern>]"),
 	N_("git for-each-ref [--points-at <object>]"),
 	N_("git for-each-ref [(--merged | --no-merged) <object>]"),
+	N_("git for-each-ref [--contains <object>]"),
 	NULL
 };
 
@@ -41,6 +42,7 @@ int cmd_for_each_ref(int argc, const char **argv, const char *prefix)
 			     parse_opt_points_at),
 		OPT_MERGED(&ref_cbdata.filter, N_("print only merged refs")),
 		OPT_NO_MERGED(&ref_cbdata.filter, N_("print only not merged refs")),
+		OPT_CONTAINS(&ref_cbdata.filter.with_commit, N_("print only refs which contain the commit")),
 		OPT_END(),
 	};
 
-- 
2.4.2

Re: [RFC/PATCH 1/9] tag: libify parse_opt_points_at()

[Junio C Hamano]

Karthik Nayak <karthik.188@gmail.com> writes:

> diff --git a/parse-options-cb.c b/parse-options-cb.c
> index be8c413..a4d294d 100644
> --- a/parse-options-cb.c
> +++ b/parse-options-cb.c
> @@ -4,6 +4,7 @@
>  #include "commit.h"
>  #include "color.h"
>  #include "string-list.h"
> +#include "sha1-array.h"
>  ...
> +int parse_opt_points_at(const struct option *opt, const char *arg, int unset)
> +{
> +	unsigned char sha1[20];
> +
> +	if (unset) {
> +		sha1_array_clear(opt->value);
> +		return 0;
> +	}
> +	if (!arg)
> +		return -1;
> +	if (get_sha1(arg, sha1))
> +		return error(_("malformed object name '%s'"), arg);
> +	sha1_array_append(opt->value, sha1);
> +	return 0;
> +}
> +

This feels way too specialized to live as part of parse_options
infrastructure.

The existing caller(s) may want to use this callback for parsing
"points-at" option they have, but is that the only plausible use of
this callback?  It looks to be usable by any future caller that
wants to take and accumulate any object names into an sha1-array, so
perhaps rename it to be a bit more generic to represent its nature
better?

	parse_opt_object_name()

or something?

I also wonder if we can (and want to) refactor the users of
with-commit callback.  Have them use this to obtain an sha1-array
and then convert what they received into a commit-list themselves.

>  int parse_opt_tertiary(const struct option *opt, const char *arg, int unset)
>  {
>  	int *target = opt->value;
> diff --git a/parse-options.h b/parse-options.h
> index c71e9da..3ae16a1 100644
> --- a/parse-options.h
> +++ b/parse-options.h
> @@ -220,6 +220,7 @@ extern int parse_opt_approxidate_cb(const struct option *, const char *, int);
>  extern int parse_opt_expiry_date_cb(const struct option *, const char *, int);
>  extern int parse_opt_color_flag_cb(const struct option *, const char *, int);
>  extern int parse_opt_verbosity_cb(const struct option *, const char *, int);
> +extern int parse_opt_points_at(const struct option *, const char *, int);
>  extern int parse_opt_with_commit(const struct option *, const char *, int);
>  extern int parse_opt_tertiary(const struct option *, const char *, int);
>  extern int parse_opt_string_list(const struct option *, const char *, int);

Re: [RFC/PATCH 1/9] tag: libify parse_opt_points_at()

[Karthik Nayak]

On 06/09/2015 12:30 AM, Junio C Hamano wrote:
>
> This feels way too specialized to live as part of parse_options
> infrastructure.
>
> The existing caller(s) may want to use this callback for parsing
> "points-at" option they have, but is that the only plausible use of
> this callback?  It looks to be usable by any future caller that
> wants to take and accumulate any object names into an sha1-array, so
> perhaps rename it to be a bit more generic to represent its nature
> better?
>
> 	parse_opt_object_name()
>
> or something?

This makes sense! Will change.

>
> I also wonder if we can (and want to) refactor the users of
> with-commit callback.  Have them use this to obtain an sha1-array
> and then convert what they received into a commit-list themselves.
>

But wouldn't that be too much of an overhead to iterate through the
sha1-array and convert it to a commit-list?

-- 
Regards,
Karthik

Re: [RFC/PATCH 0/9] add options to ref-filter

[Christian Couder]

On Sat, Jun 6, 2015 at 10:03 PM, Karthik Nayak <karthik.188@gmail.com> wrote:
> This is a follow up series to the one posted here
> http://thread.gmane.org/gmane.comp.version-control.git/270922
>
> This patch series adds '--ponints-at', '--merged', '--no-merged' and

s/--ponints-at/--points-at/

> '--contain' options to 'ref-filter' and uses these options in
> 'for-each-ref'.

Re: [RFC/PATCH 0/9] add options to ref-filter

[Karthik Nayak]

On 06/07/2015 06:40 PM, Christian Couder wrote:
> On Sat, Jun 6, 2015 at 10:03 PM, Karthik Nayak <karthik.188@gmail.com> wrote:
> > This is a follow up series to the one posted here
> > http://thread.gmane.org/gmane.comp.version-control.git/270922
> >
> > This patch series adds '--ponints-at', '--merged', '--no-merged' and
>
> s/--ponints-at/--points-at/
>
> > '--contain' options to 'ref-filter' and uses these options in
> > 'for-each-ref'.

Thanks!

-- 
Regards,
Karthik

Re: [RFC/PATCH 0/9] add options to ref-filter

[Junio C Hamano]

Karthik Nayak <karthik.188@gmail.com> writes:

> This is a follow up series to the one posted here
> http://thread.gmane.org/gmane.comp.version-control.git/270922
>
> This patch series adds '--ponints-at', '--merged', '--no-merged' and
> '--contain' options to 'ref-filter' and uses these options in
> for-each-ref'.

The structure of the series looked sensible (I stopped at around
7/9, though, for now); even though a few style violations were
somewhat irritating, overall it was a pleasant read.

Thanks.