[PATCH v8 0/4] tag: configure default tag sort via .gitconfig

[PATCH v8 0/4] tag: configure default tag sort via .gitconfig

[Jacob Keller]

This patch series is hopefully the final version of a series of patches which
updates git-tag to allow the .gitconfig variable 'tag.sort' to be used as the
default sort parameter. This version uses a new set/pop error routine setup
which enables correctly modifying the error routine to handle the config vs the
command line.

In addition, I split the patch such that all the changes to the original
parse_opt_sort are shown, and the following patch which extracts this function
is just a function extraction with no changes, making it easier to review the
new changes. One other minor bug fix is included as well.

Jacob Keller (4):
  usage: make error functions a stack
  tag: fix --sort tests to use cat<<-\EOF format
  tag: update parsing to be more precise regarding errors
  tag: support configuring --sort via .gitconfig

 Documentation/config.txt  |  5 +++
 Documentation/git-tag.txt |  5 ++-
 builtin/tag.c             | 97 ++++++++++++++++++++++++++++++++++++-----------
 git-compat-util.h         |  1 +
 t/t7004-tag.sh            | 76 +++++++++++++++++++++++++++----------
 usage.c                   | 29 ++++++++++++--
 6 files changed, 167 insertions(+), 46 deletions(-)

-- 
2.0.1.475.g9b8d714

[PATCH v8 1/4] usage: make error functions a stack

[Jacob Keller]

Let error routine be a stack of error functions so that callers can
temporarily override the error_routine and then pop their modification
off the stack. This enables customizing error for a small code segment.

Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
---
This is a modification of Peff's original idea for handling multiple error
routines. I simplified it by not having the collect and other routines. I only
modify set_error_routine to be a "push" operation, with pop_error_routine being
its opposite. I don't let pop_error_routine remove all the error routines,
instead only doing one with an assert check that we never call it too many times.

This enables temporarily modifying the error routine and then popping back to
the previous value.

 git-compat-util.h |  1 +
 usage.c           | 29 ++++++++++++++++++++++++++---
 2 files changed, 27 insertions(+), 3 deletions(-)

diff --git a/git-compat-util.h b/git-compat-util.h
index 9de318071083..6d0416c90ad8 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -344,6 +344,7 @@ static inline int const_error(void)
 
 extern void set_die_routine(NORETURN_PTR void (*routine)(const char *err, va_list params));
 extern void set_error_routine(void (*routine)(const char *err, va_list params));
+extern void pop_error_routine(void);
 extern void set_die_is_recursing_routine(int (*routine)(void));
 
 extern int starts_with(const char *str, const char *prefix);
diff --git a/usage.c b/usage.c
index ed146453cabe..fd9126a7ca0b 100644
--- a/usage.c
+++ b/usage.c
@@ -57,18 +57,41 @@ static int die_is_recursing_builtin(void)
  * (ugh), so keep things static. */
 static NORETURN_PTR void (*usage_routine)(const char *err, va_list params) = usage_builtin;
 static NORETURN_PTR void (*die_routine)(const char *err, va_list params) = die_builtin;
-static void (*error_routine)(const char *err, va_list params) = error_builtin;
 static void (*warn_routine)(const char *err, va_list params) = warn_builtin;
 static int (*die_is_recursing)(void) = die_is_recursing_builtin;
 
+struct error_func_list {
+	void (*func)(const char *, va_list);
+	struct error_func_list *next;
+};
+static struct error_func_list default_error_func = { error_builtin };
+static struct error_func_list *error_funcs = &default_error_func;
+
 void set_die_routine(NORETURN_PTR void (*routine)(const char *err, va_list params))
 {
 	die_routine = routine;
 }
 
+/* push error routine onto the error function stack */
 void set_error_routine(void (*routine)(const char *err, va_list params))
 {
-	error_routine = routine;
+	struct error_func_list *efl = xmalloc(sizeof(*efl));
+	efl->func = routine;
+	efl->next = error_funcs;
+	error_funcs = efl;
+}
+
+/* pop a single error routine off of the error function stack, thus reverting
+ * to previous error. Should always be paired with a set_error_routine */
+void pop_error_routine(void)
+{
+	assert(error_funcs != &default_error_func);
+
+	struct error_func_list *efl = error_funcs;
+	if (efl->next) {
+		error_funcs = efl->next;
+		free(efl);
+	}
 }
 
 void set_die_is_recursing_routine(int (*routine)(void))
@@ -144,7 +167,7 @@ int error(const char *err, ...)
 	va_list params;
 
 	va_start(params, err);
-	error_routine(err, params);
+	error_funcs->func(err, params);
 	va_end(params);
 	return -1;
 }
-- 
2.0.1.475.g9b8d714

[PATCH v8 2/4] tag: fix --sort tests to use cat<<-\EOF format

[Jacob Keller]

The --sort tests should use the better format for >expect to maintain
indenting and ensure that no substitution is occurring. This makes
parsing and understanding the tests a bit easier.

Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
---
 t/t7004-tag.sh | 40 ++++++++++++++++++++--------------------
 1 file changed, 20 insertions(+), 20 deletions(-)

diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh
index e4ab0f5b6419..66010b0e7066 100755
--- a/t/t7004-tag.sh
+++ b/t/t7004-tag.sh
@@ -1385,41 +1385,41 @@ test_expect_success 'lexical sort' '
 	git tag foo1.6 &&
 	git tag foo1.10 &&
 	git tag -l --sort=refname "foo*" >actual &&
-	cat >expect <<EOF &&
-foo1.10
-foo1.3
-foo1.6
-EOF
+	cat >expect <<-\EOF &&
+	foo1.10
+	foo1.3
+	foo1.6
+	EOF
 	test_cmp expect actual
 '
 
 test_expect_success 'version sort' '
 	git tag -l --sort=version:refname "foo*" >actual &&
-	cat >expect <<EOF &&
-foo1.3
-foo1.6
-foo1.10
-EOF
+	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 "foo*" >actual &&
-	cat >expect <<EOF &&
-foo1.10
-foo1.6
-foo1.3
-EOF
+	cat >expect <<-\EOF &&
+	foo1.10
+	foo1.6
+	foo1.3
+	EOF
 	test_cmp expect actual
 '
 
 test_expect_success 'reverse lexical sort' '
 	git tag -l --sort=-refname "foo*" >actual &&
-	cat >expect <<EOF &&
-foo1.6
-foo1.3
-foo1.10
-EOF
+	cat >expect <<-\EOF &&
+	foo1.6
+	foo1.3
+	foo1.10
+	EOF
 	test_cmp expect actual
 '
 
-- 
2.0.1.475.g9b8d714

[PATCH v8 3/4] tag: update parsing to be more precise regarding errors

[Jacob Keller]

Update the parsing of sort string specifications so that it is able to
properly detect errors in the function type and allowed atoms.

Cc: Jeff King <peff@peff.net>
Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
---
This function should replace the one I think is already on one of the branches.
This version fully updates the parse_sort_opt to be like what will be the
parse_sort_string function. I have ensured that the next patch is purely a move
of this function, and does not contain modifications to make this easier to
review. Instead of using skip_prefix, I use strchr and check for the separator.

 builtin/tag.c | 55 +++++++++++++++++++++++++++++++++++++++++--------------
 1 file changed, 41 insertions(+), 14 deletions(-)

diff --git a/builtin/tag.c b/builtin/tag.c
index ef765563388c..7d82526e76be 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -522,24 +522,51 @@ static int parse_opt_points_at(const struct option *opt __attribute__((unused)),
 static int parse_opt_sort(const struct option *opt, const char *arg, int unset)
 {
 	int *sort = opt->value;
-	int flags = 0;
+	char *value, *separator, *type, *atom;
+	int flags = 0, function = 0, err = 0;
 
-	if (*arg == '-') {
+	/* skip the '-' prefix for reverse sort order first */
+	if (skip_prefix(arg, "-", &arg))
 		flags |= REVERSE_SORT;
-		arg++;
+
+	/* duplicate string so we can modify it in place */
+	value = xstrdup(arg);
+
+	/* determine the sort function and the sorting atom */
+	separator = strchr(value, ':');
+	if (separator) {
+		/* split the string at the separator with a NULL byte */
+		*separator = '\0';
+		type = value;
+		atom = separator + 1;
+	} else {
+		/* we have no separator, so assume the whole string is the * atom */
+		type = NULL;
+		atom = value;
 	}
-	if (starts_with(arg, "version:")) {
-		*sort = VERCMP_SORT;
-		arg += 8;
-	} else if (starts_with(arg, "v:")) {
-		*sort = VERCMP_SORT;
-		arg += 2;
+
+	if (type) {
+		if (!strcmp(type, "version") || !strcmp(type, "v"))
+			function = VERCMP_SORT;
+		else {
+			err = error(_("unsupported sort function '%s'"), type);
+			goto abort;
+		}
+
 	} else
-		*sort = STRCMP_SORT;
-	if (strcmp(arg, "refname"))
-		die(_("unsupported sort specification %s"), arg);
-	*sort |= flags;
-	return 0;
+		function = STRCMP_SORT;
+
+	/* for now, only the refname is a valid atom */
+	if (atom && strcmp(atom, "refname")) {
+		err = error(_("unsupported sort specification '%s'"), atom);
+		goto abort;
+	}
+
+	*sort = (flags | function);
+
+abort:
+	free(value);
+	return err;
 }
 
 int cmd_tag(int argc, const char **argv, const char *prefix)
-- 
2.0.1.475.g9b8d714

[PATCH v8 4/4] tag: support configuring --sort via .gitconfig

[Jacob Keller]

Add support for configuring default sort specification for git tags.
Command line option will override the configured value. Both methods use
the same syntax. Make use of (set/pop)_error_routine to temporarily
modify error reporting when parsing as a configuration option.

Cc: Jeff King <peff@peff.net>
Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
---
 Documentation/config.txt  |   5 ++
 Documentation/git-tag.txt |   5 +-
 builtin/tag.c             | 124 ++++++++++++++++++++++++++++------------------
 t/t7004-tag.sh            |  36 ++++++++++++++
 4 files changed, 120 insertions(+), 50 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 1d718bdb9662..c55c22ab7be9 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -2354,6 +2354,11 @@ submodule.<name>.ignore::
 	"--ignore-submodules" option. The 'git submodule' commands are not
 	affected by this setting.
 
+tag.sort::
+	This variable controls the sort ordering of tags when displayed by
+	linkgit:git-tag[1]. Without the "--sort=<value>" option provided, the
+	value of this variable will be used as the default.
+
 tar.umask::
 	This variable can be used to restrict the permission bits of
 	tar archive entries.  The default is 0002, which turns off the
diff --git a/Documentation/git-tag.txt b/Documentation/git-tag.txt
index b424a1bc48bb..320908369f06 100644
--- a/Documentation/git-tag.txt
+++ b/Documentation/git-tag.txt
@@ -99,7 +99,9 @@ OPTIONS
 	Sort in a specific order. Supported type is "refname"
 	(lexicographic order), "version:refname" or "v:refname" (tag
 	names are treated as versions). Prepend "-" to reverse sort
-	order.
+	order. When this option is not given, the sort order defaults to the
+	value configured for the 'tag.sort' variable if it exists, or
+	lexicographic order otherwise. See linkgit:git-config[1].
 
 --column[=<options>]::
 --no-column::
@@ -317,6 +319,7 @@ include::date-formats.txt[]
 SEE ALSO
 --------
 linkgit:git-check-ref-format[1].
+linkgit:git-config[1].
 
 GIT
 ---
diff --git a/builtin/tag.c b/builtin/tag.c
index 7d82526e76be..603cf688368c 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -32,6 +32,8 @@ static const char * const git_tag_usage[] = {
 #define SORT_MASK       0x7fff
 #define REVERSE_SORT    0x8000
 
+static int tag_sort;
+
 struct tag_filter {
 	const char **patterns;
 	int lines;
@@ -346,9 +348,76 @@ static const char tag_template_nocleanup[] =
 	"Lines starting with '%c' will be kept; you may remove them"
 	" yourself if you want to.\n");
 
+static int parse_sort_string(const char *arg, int *sort)
+{
+	char *value, *separator, *type, *atom;
+	int flags = 0, function = 0, err = 0;
+
+	/* skip the '-' prefix for reverse sort order first */
+	if (skip_prefix(arg, "-", &arg))
+		flags |= REVERSE_SORT;
+
+	/* duplicate string so we can modify it in place */
+	value = xstrdup(arg);
+
+	/* determine the sort function and the sorting atom */
+	separator = strchr(value, ':');
+	if (separator) {
+		/* split the string at the separator with a NULL byte */
+		*separator = '\0';
+		type = value;
+		atom = separator + 1;
+	} else {
+		/* we have no separator, so assume the whole string is the * atom */
+		type = NULL;
+		atom = value;
+	}
+
+	if (type) {
+		if (!strcmp(type, "version") || !strcmp(type, "v"))
+			function = VERCMP_SORT;
+		else {
+			err = error(_("unsupported sort function '%s'"), type);
+			goto abort;
+		}
+
+	} else
+		function = STRCMP_SORT;
+
+	/* for now, only the refname is a valid atom */
+	if (atom && strcmp(atom, "refname")) {
+		err = error(_("unsupported sort specification '%s'"), atom);
+		goto abort;
+	}
+
+	*sort = (flags | function);
+
+abort:
+	free(value);
+	return err;
+}
+
+static void error_bad_sort_config(const char *err, va_list params)
+{
+	vreportf("warning: tag.sort has ", err, params);
+}
+
 static int git_tag_config(const char *var, const char *value, void *cb)
 {
-	int status = git_gpg_config(var, value, cb);
+	int status;
+
+	if (!strcmp(var, "tag.sort")) {
+		if (!value)
+			return config_error_nonbool(var);
+
+		set_error_routine(error_bad_sort_config);
+		parse_sort_string(value, &tag_sort);
+		pop_error_routine();
+
+		return 0;
+	}
+
+	status = git_gpg_config(var, value, cb);
 	if (status)
 		return status;
 	if (starts_with(var, "column."))
@@ -522,51 +591,8 @@ static int parse_opt_points_at(const struct option *opt __attribute__((unused)),
 static int parse_opt_sort(const struct option *opt, const char *arg, int unset)
 {
 	int *sort = opt->value;
-	char *value, *separator, *type, *atom;
-	int flags = 0, function = 0, err = 0;
 
-	/* skip the '-' prefix for reverse sort order first */
-	if (skip_prefix(arg, "-", &arg))
-		flags |= REVERSE_SORT;
-
-	/* duplicate string so we can modify it in place */
-	value = xstrdup(arg);
-
-	/* determine the sort function and the sorting atom */
-	separator = strchr(value, ':');
-	if (separator) {
-		/* split the string at the separator with a NULL byte */
-		*separator = '\0';
-		type = value;
-		atom = separator + 1;
-	} else {
-		/* we have no separator, so assume the whole string is the * atom */
-		type = NULL;
-		atom = value;
-	}
-
-	if (type) {
-		if (!strcmp(type, "version") || !strcmp(type, "v"))
-			function = VERCMP_SORT;
-		else {
-			err = error(_("unsupported sort function '%s'"), type);
-			goto abort;
-		}
-
-	} else
-		function = STRCMP_SORT;
-
-	/* for now, only the refname is a valid atom */
-	if (atom && strcmp(atom, "refname")) {
-		err = error(_("unsupported sort specification '%s'"), atom);
-		goto abort;
-	}
-
-	*sort = (flags | function);
-
-abort:
-	free(value);
-	return err;
+	return parse_sort_string(arg, sort);
 }
 
 int cmd_tag(int argc, const char **argv, const char *prefix)
@@ -579,7 +605,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
 	struct create_tag_options opt;
 	char *cleanup_arg = NULL;
 	int annotate = 0, force = 0, lines = -1;
-	int cmdmode = 0, sort = 0;
+	int cmdmode = 0;
 	const char *msgfile = NULL, *keyid = NULL;
 	struct msg_arg msg = { 0, STRBUF_INIT };
 	struct commit_list *with_commit = NULL;
@@ -605,7 +631,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
 		OPT__FORCE(&force, N_("replace the tag if exists")),
 		OPT_COLUMN(0, "column", &colopts, N_("show tag list in columns")),
 		{
-			OPTION_CALLBACK, 0, "sort", &sort, N_("type"), N_("sort tags"),
+			OPTION_CALLBACK, 0, "sort", &tag_sort, N_("type"), N_("sort tags"),
 			PARSE_OPT_NONEG, parse_opt_sort
 		},
 
@@ -661,9 +687,9 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
 			copts.padding = 2;
 			run_column_filter(colopts, &copts);
 		}
-		if (lines != -1 && sort)
+		if (lines != -1 && tag_sort)
 			die(_("--sort and -n are incompatible"));
-		ret = list_tags(argv, lines == -1 ? 0 : lines, with_commit, sort);
+		ret = list_tags(argv, lines == -1 ? 0 : lines, with_commit, tag_sort);
 		if (column_active(colopts))
 			stop_column_filter();
 		return ret;
diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh
index 66010b0e7066..036665308841 100755
--- a/t/t7004-tag.sh
+++ b/t/t7004-tag.sh
@@ -1423,6 +1423,42 @@ test_expect_success 'reverse lexical sort' '
 	test_cmp expect actual
 '
 
+test_expect_success 'configured lexical sort' '
+	git config tag.sort "v:refname" &&
+	git tag -l "foo*" >actual &&
+	cat >expect <<-\EOF &&
+	foo1.3
+	foo1.6
+	foo1.10
+	EOF
+	test_cmp expect actual
+'
+
+test_expect_success 'option override configured sort' '
+	git tag -l --sort=-refname "foo*" >actual &&
+	cat >expect <<-\EOF &&
+	foo1.6
+	foo1.3
+	foo1.10
+	EOF
+	test_cmp expect actual
+'
+
+test_expect_success 'invalid sort parameter on command line' '
+	test_must_fail git tag -l --sort=notvalid "foo*" >actual
+'
+
+test_expect_success 'invalid sort parameter in configuratoin' '
+	git config tag.sort "v:notvalid" &&
+	git tag -l "foo*" >actual &&
+	cat >expect <<-\EOF &&
+	foo1.10
+	foo1.3
+	foo1.6
+	EOF
+	test_cmp expect actual
+'
+
 run_with_limited_stack () {
 	(ulimit -s 64 && "$@")
 }
-- 
2.0.1.475.g9b8d714

Re: [PATCH v8 0/4] tag: configure default tag sort via .gitconfig

[Junio C Hamano]

Jacob Keller <jacob.e.keller@intel.com> writes:

> This patch series is hopefully the final version of a series of patches which
> updates git-tag to allow the .gitconfig variable 'tag.sort' to be used as the
> default sort parameter. This version uses a new set/pop error routine setup
> which enables correctly modifying the error routine to handle the config vs the
> command line.
>
> In addition, I split the patch such that all the changes to the original
> parse_opt_sort are shown, and the following patch which extracts this function
> is just a function extraction with no changes, making it easier to review the
> new changes. One other minor bug fix is included as well.
>
> Jacob Keller (4):
>   usage: make error functions a stack
>   tag: fix --sort tests to use cat<<-\EOF format
>   tag: update parsing to be more precise regarding errors
>   tag: support configuring --sort via .gitconfig

The first one looked somewhat tricky, but looked nice overall from a
cursory reading.

Thanks.

Re: [PATCH v8 1/4] usage: make error functions a stack

[Junio C Hamano]

Jacob Keller <jacob.e.keller@intel.com> writes:

>  extern void set_error_routine(void (*routine)(const char *err, va_list params));
> +extern void pop_error_routine(void);

pop that undoes set smells somewhat weird.  Perhaps we should rename
set to push?  That would allow us catch possible topics that add new
calls to set_error_routine() as well by forcing the system not to
link when they are merged without necessary fixes.

> +/* push error routine onto the error function stack */
>  void set_error_routine(void (*routine)(const char *err, va_list params))
>  {
> -	error_routine = routine;
> +	struct error_func_list *efl = xmalloc(sizeof(*efl));
> +	efl->func = routine;
> +	efl->next = error_funcs;
> +	error_funcs = efl;
> +}
> +
> +/* pop a single error routine off of the error function stack, thus reverting
> + * to previous error. Should always be paired with a set_error_routine */
> +void pop_error_routine(void)
> +{
> +	assert(error_funcs != &default_error_func);
> +
> +	struct error_func_list *efl = error_funcs;

decl-after-stmt.  Can be fixed easily by flipping the above two
lines.

> +	if (efl->next) {
> +		error_funcs = efl->next;
> +		free(efl);
> +	}
>  }
>  
>  void set_die_is_recursing_routine(int (*routine)(void))
> @@ -144,7 +167,7 @@ int error(const char *err, ...)
>  	va_list params;
>  
>  	va_start(params, err);
> -	error_routine(err, params);
> +	error_funcs->func(err, params);
>  	va_end(params);
>  	return -1;
>  }

Re: [PATCH v8 1/4] usage: make error functions a stack

[Keller, Jacob E]

On Tue, 2014-07-15 at 15:47 -0700, Junio C Hamano wrote:
> Jacob Keller <jacob.e.keller@intel.com> writes:
> 
> >  extern void set_error_routine(void (*routine)(const char *err, va_list params));
> > +extern void pop_error_routine(void);
> 
> pop that undoes set smells somewhat weird.  Perhaps we should rename
> set to push?  That would allow us catch possible topics that add new
> calls to set_error_routine() as well by forcing the system not to
> link when they are merged without necessary fixes.
> 

I thought about changing set too, but wasn't sure that made sense..?
That does make more sense now though. There *are* valid use cases where
a set_error_routine is used without a pop, (the one current use, I
think).

I'll update this patch with that change.

> > +/* push error routine onto the error function stack */
> >  void set_error_routine(void (*routine)(const char *err, va_list params))
> >  {
> > -	error_routine = routine;
> > +	struct error_func_list *efl = xmalloc(sizeof(*efl));
> > +	efl->func = routine;
> > +	efl->next = error_funcs;
> > +	error_funcs = efl;
> > +}
> > +
> > +/* pop a single error routine off of the error function stack, thus reverting
> > + * to previous error. Should always be paired with a set_error_routine */
> > +void pop_error_routine(void)
> > +{
> > +	assert(error_funcs != &default_error_func);
> > +
> > +	struct error_func_list *efl = error_funcs;
> 
> decl-after-stmt.  Can be fixed easily by flipping the above two
> lines.

Oh, right yes. I'll fix that in a resend as well.

Thanks,
Jake

Re: [PATCH v8 1/4] usage: make error functions a stack

[Keller, Jacob E]

On Tue, 2014-07-15 at 15:47 -0700, Junio C Hamano wrote:
> Jacob Keller <jacob.e.keller@intel.com> writes:
> 
> >  extern void set_error_routine(void (*routine)(const char *err, va_list params));
> > +extern void pop_error_routine(void);
> 
> pop that undoes set smells somewhat weird.  Perhaps we should rename
> set to push?  That would allow us catch possible topics that add new
> calls to set_error_routine() as well by forcing the system not to
> link when they are merged without necessary fixes.
> 

Also curious what your thoughts on making every set_*_routine to be a
stack? For now I was only planning on error but it maybe makes sense to
change them all?

Thanks,
Jake

Re: [PATCH v8 1/4] usage: make error functions a stack

[Junio C Hamano]

I actually am not a big fan of "stack" for a thing like this, to be honest.
Wouldn't it be sufficient for the callers who want specific behaviour from
its callees to

 - save away the current error/warning routines;
 - set error/warning routines to its own custom versions;
 - call the callees;
 - set error/warning routines back to their original; and
 - return from it

at least in the code paths under discussion?


On Tue, Jul 15, 2014 at 4:26 PM, Keller, Jacob E
<jacob.e.keller@intel.com> wrote:
> On Tue, 2014-07-15 at 15:47 -0700, Junio C Hamano wrote:
>> Jacob Keller <jacob.e.keller@intel.com> writes:
>>
>> >  extern void set_error_routine(void (*routine)(const char *err, va_list params));
>> > +extern void pop_error_routine(void);
>>
>> pop that undoes set smells somewhat weird.  Perhaps we should rename
>> set to push?  That would allow us catch possible topics that add new
>> calls to set_error_routine() as well by forcing the system not to
>> link when they are merged without necessary fixes.
>>
>
> Also curious what your thoughts on making every set_*_routine to be a
> stack? For now I was only planning on error but it maybe makes sense to
> change them all?
>
> Thanks,
> Jake
>

Re: [PATCH v8 1/4] usage: make error functions a stack

[Keller, Jacob E]

There was no way to get the current error routine now, and I figured
that a stack was a simple way of saving the old routine.

Essentially these two paths would be the same as a "save/restore" except
we manage it via a stack. I don't really see how that would end up any
different. I mean I don't mind either approach.

Thanks,
Jake

On Tue, 2014-07-15 at 17:49 -0700, Junio C Hamano wrote:
> I actually am not a big fan of "stack" for a thing like this, to be honest.
> Wouldn't it be sufficient for the callers who want specific behaviour from
> its callees to
> 
>  - save away the current error/warning routines;
>  - set error/warning routines to its own custom versions;
>  - call the callees;
>  - set error/warning routines back to their original; and
>  - return from it
> 
> at least in the code paths under discussion?
> 
> 
> On Tue, Jul 15, 2014 at 4:26 PM, Keller, Jacob E
> <jacob.e.keller@intel.com> wrote:
> > On Tue, 2014-07-15 at 15:47 -0700, Junio C Hamano wrote:
> >> Jacob Keller <jacob.e.keller@intel.com> writes:
> >>
> >> >  extern void set_error_routine(void (*routine)(const char *err, va_list params));
> >> > +extern void pop_error_routine(void);
> >>
> >> pop that undoes set smells somewhat weird.  Perhaps we should rename
> >> set to push?  That would allow us catch possible topics that add new
> >> calls to set_error_routine() as well by forcing the system not to
> >> link when they are merged without necessary fixes.
> >>
> >
> > Also curious what your thoughts on making every set_*_routine to be a
> > stack? For now I was only planning on error but it maybe makes sense to
> > change them all?
> >
> > Thanks,
> > Jake
> >