add options to ref-filter

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

Quoted 2 lines · Karthik Nayak

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

s/,/./ ?

Quoted 1 line

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

s/the//

ref-filter.c +12 -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;
+

Quoted 20 lines · Karthik Nayak

ref-filter.c +20 -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;
+

ref-filter.c static int match_name_as_path(const char **pattern, const char *refname) · line 865

Quoted 2 lines

ref-filter.c +2 -0

+	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".

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.

Quoted 5 lines · 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.

... but no test?

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 <[email protected]>

Mentored-by Christian Couder <[email protected]>

Mentored-by Matthieu Moy <[email protected]>

Signed-off-by Karthik Nayak <[email protected]>

---
 Documentation/git-for-each-ref.txt | 3 +++
 builtin/for-each-ref.c             | 6 +++++-
 2 files changed, 8 insertions(+), 1 deletion(-)

Documentation/git-for-each-ref.txt +1 -0

index 7f8d9a5..e9f6a8a 100644
@@ -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
 -----------

Documentation/git-for-each-ref.txt +0 -0

 FIELD NAMES
 -----------

builtin/for-each-ref.c +5 -0

index 4d2d024..b9d180a 100644
@@ -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(),
 	};
 

builtin/for-each-ref.c +0 -0

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

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.

Quoted 33 lines

FIELD NAMES
-----------

builtin/for-each-ref.c +5 -0

index 4d2d024..b9d180a 100644
@@ -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(),
  	};
 

builtin/for-each-ref.c int cmd_for_each_ref(int argc, const char **argv, const char *prefix) · line 57

Quoted 5 lines

builtin/for-each-ref.c +0 -1

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

Quoted 9 lines

builtin/for-each-ref.c int cmd_for_each_ref(int argc, const char **argv, const char *prefix) · line 57

Quoted 5 lines

builtin/for-each-ref.c +0 -1

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

builtin/for-each-ref.c int cmd_for_each_ref(int argc, const char **argv, const char *prefix) · line 24

Quoted 6 lines

  	int maxcount = 0, quote_style = 0;
  	struct ref_filter_cbdata ref_cbdata;

builtin/for-each-ref.c +1 -0

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

Quoted 10 lines · Karthik Nayak

Quoted 9 lines

builtin/for-each-ref.c int cmd_for_each_ref(int argc, const char **argv, const char *prefix) · line 57

Quoted 5 lines

builtin/for-each-ref.c +0 -1

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

Quoted 3 lines

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

Quoted 6 lines

Quoted 6 lines

       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.

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

	type funcname(args)
        {
        	declarations;

                first statement;
		...

Quoted 8 lines · Junio C Hamano

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.

parse-options-cb.c int parse_opt_points_at(const struct option *opt, const char *arg, int unset) · line 120

Quoted 8 lines · Karthik Nayak

parse-options-cb.c +8 -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;

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.

parse-options-cb.c +10 -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)

parse-options.h +1 -0

index 3ae16a1..7bcf0f3 100644
@@ -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 }

Quoted 10 lines · Karthik Nayak

parse-options-cb.c +10 -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)

parse-options-cb.c int parse_opt_points_at(const struct option *opt, const char *arg, int unset) · line 132

Quoted 10 lines

parse-options-cb.c +10 -0

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

Quoted 21 lines

parse-options.h +5 -0

index 3ae16a1..7bcf0f3 100644
@@ -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" \

parse-options.h extern int parse_opt_noop_cb(const struct option *, const char *, int); · line 256

Quoted 6 lines

parse-options.h +6 -0

+	}
+#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.

ref-filter.c int ref_filter_handler(const char *refname, const struct object_id *oid, int fla · line 905

Quoted 1 line · Karthik Nayak

ref-filter.c +1 -0

+	if(filter->merge_commit) {

space after if.

ref-filter.c void ref_array_clear(struct ref_array *array) · line 998

Quoted 10 lines

ref-filter.c +5 -1

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

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.

Quoted 6 lines · 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.

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.

builtin/for-each-ref.c int cmd_for_each_ref(int argc, const char **argv, const char *prefix) · line 43

Quoted 6 lines

builtin/for-each-ref.c +2 -0

@@ -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".

parse-options.h extern int parse_opt_noop_cb(const struct option *, const char *, int); · line 266

Quoted 10 lines · Karthik Nayak

parse-options.h +10 -0

+#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)

parse-options-cb.c +7 -0

index be8c413..a4d294d 100644
@@ -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);

int parse_opt_tertiary(const struct option *opt, const char *arg, int unset) {

parse-options-cb.c +0 -0

 	int *target = opt->value;

parse-options.h +1 -0

index c71e9da..3ae16a1 100644
@@ -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);

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.

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

Quoted 4 lines · 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

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

Quoted 2 lines

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

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