[PATCH 1/2] praise: a culturally sensitive wrapper for 'blame'

[PATCH 1/2] praise: a culturally sensitive wrapper for 'blame'

[Ævar Arnfjörð Bjarmason]

Add a wrapper for "blame" for use in blameless cultural
settings. Users in such cultures have hitherto had to fall back on the
legacy "annotate" command and its inferior output if they wanted to
avoid culturally inappropriate behavior.

Now such users have the option of using this wrapper. Organizations
that wish to engage in cultural enforcement can set
"blame.culture=blameless" in their configuration to make any direct
use of the "blame" command an error. Those wishing to centrally report
such transgressions can do so with the trace2 logging facility[1].

1. See Documentation/technical/api-trace2.txt --
   https://github.com/git/git/blob/master/Documentation/technical/api-trace2.txt

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 Documentation/config/blame.txt | 10 ++++++++++
 Documentation/git-blame.txt    |  3 ++-
 Documentation/git-praise.txt   | 20 ++++++++++++++++++++
 builtin/blame.c                |  9 +++++++++
 git.c                          |  1 +
 t/t8002-blame.sh               | 15 +++++++++++++++
 6 files changed, 57 insertions(+), 1 deletion(-)
 create mode 100644 Documentation/git-praise.txt

diff --git a/Documentation/config/blame.txt b/Documentation/config/blame.txt
index 67b5c1d1e0..c85b35de17 100644
--- a/Documentation/config/blame.txt
+++ b/Documentation/config/blame.txt
@@ -1,3 +1,13 @@
+blame.culture::
+	Set to 'blameless' to enforce the use of the functionality of
+	linkgit:git-blame[1] in blameless cultural settings. This will
+	make invocations of linkgit:git-blame[1] an error, forcing the
+	use of linkgit:git-praise[1]. Defaults to 'blameful', which is
+	the default.
++
+Note that the `--porcelain` format for machine consumption is exempt
+from this enforcement to avoid breaking existing scripts.
+
 blame.blankBoundary::
 	Show blank commit object name for boundary commits in
 	linkgit:git-blame[1]. This option defaults to false.
diff --git a/Documentation/git-blame.txt b/Documentation/git-blame.txt
index 16323eb80e..9c1bff7389 100644
--- a/Documentation/git-blame.txt
+++ b/Documentation/git-blame.txt
@@ -228,7 +228,8 @@ include::mailmap.txt[]
 
 SEE ALSO
 --------
-linkgit:git-annotate[1]
+linkgit:git-annotate[1],
+linkgit:git-praise[1]
 
 GIT
 ---
diff --git a/Documentation/git-praise.txt b/Documentation/git-praise.txt
new file mode 100644
index 0000000000..d34b9dbc61
--- /dev/null
+++ b/Documentation/git-praise.txt
@@ -0,0 +1,20 @@
+git-praise(1)
+============
+
+NAME
+----
+git-praise - A culturally sensitive alias for git-blame
+
+DESCRIPTION
+-----------
+
+Annotates each line in the given file without compromising the
+cultural integrity of blameless cultures.
+
+The functionality of this command is equivalent to
+linkgit:git-blame[1]. Its use can be enforced by setting
+`blame.culture=blameless` in the config. See linkgit:git-config[1].
+
+GIT
+---
+Part of the linkgit:git[1] suite
diff --git a/builtin/blame.c b/builtin/blame.c
index 177c1022a0..238b19db48 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -37,6 +37,7 @@ static const char *blame_opt_usage[] = {
 	NULL
 };
 
+static int blameless_culture;
 static int longest_file;
 static int longest_author;
 static int max_orig_digits;
@@ -681,6 +682,10 @@ static int git_blame_config(const char *var, const char *value, void *cb)
 		blank_boundary = git_config_bool(var, value);
 		return 0;
 	}
+	if (!strcmp(var, "blame.culture")) {
+		blameless_culture = !strcmp(value, "blameless");
+		return 0;
+	}
 	if (!strcmp(var, "blame.showemail")) {
 		int *output_option = cb;
 		if (git_config_bool(var, value))
@@ -828,6 +833,7 @@ int cmd_blame(int argc, const char **argv, const char *prefix)
 
 	struct parse_opt_ctx_t ctx;
 	int cmd_is_annotate = !strcmp(argv[0], "annotate");
+	int cmd_is_praise = !strcmp(argv[0], "praise");
 	struct range_set ranges;
 	unsigned int range_i;
 	long anchor;
@@ -889,6 +895,9 @@ int cmd_blame(int argc, const char **argv, const char *prefix)
 	if (cmd_is_annotate) {
 		output_option |= OUTPUT_ANNOTATE_COMPAT;
 		blame_date_mode.type = DATE_ISO8601;
+	} else if (!cmd_is_praise && blameless_culture &&
+		   !(output_option & OUTPUT_PORCELAIN)) {
+		die(_("must be invoked as 'git praise' with 'blame.culture=blameless' set!"));
 	} else {
 		blame_date_mode = revs.date_mode;
 	}
diff --git a/git.c b/git.c
index 2014aab6b8..77640f8cd9 100644
--- a/git.c
+++ b/git.c
@@ -536,6 +536,7 @@ static struct cmd_struct commands[] = {
 	{ "pack-refs", cmd_pack_refs, RUN_SETUP },
 	{ "patch-id", cmd_patch_id, RUN_SETUP_GENTLY | NO_PARSEOPT },
 	{ "pickaxe", cmd_blame, RUN_SETUP },
+	{ "praise", cmd_blame, RUN_SETUP },
 	{ "prune", cmd_prune, RUN_SETUP },
 	{ "prune-packed", cmd_prune_packed, RUN_SETUP },
 	{ "pull", cmd_pull, RUN_SETUP | NEED_WORK_TREE },
diff --git a/t/t8002-blame.sh b/t/t8002-blame.sh
index eea048e52c..2d59b856d1 100755
--- a/t/t8002-blame.sh
+++ b/t/t8002-blame.sh
@@ -48,6 +48,21 @@ test_expect_success 'blame with no options and no config' '
 	test_cmp expected_n result
 '
 
+test_expect_success 'praise' '
+	git praise --show-email one >praise1 &&
+	find_blame <praise1 >result &&
+	test_cmp expected_e result &&
+
+	git praise --no-show-email one >praise2 &&
+	find_blame <praise2 >result &&
+	test_cmp expected_n result
+'
+
+test_expect_success 'enforced praise' '
+	test_must_fail git -c blame.culture=blameless blame one 2>err &&
+	test_i18ngrep "must be.*git praise" err
+'
+
 test_expect_success 'blame with showemail options' '
 	git blame --show-email one >blame1 &&
 	find_blame <blame1 >result &&
-- 
2.21.0.392.gf8f6787159e

[PATCH 2/2] praise: make 'blameless' cultural enforcement configurable

[Ævar Arnfjörð Bjarmason]

The culture shock of having a 'blameless' culture from day one might
be too much for some, so let's allow for setting
"blame.culture.enforcement=warning" to allow for easing into the
default of "error".

Also allow for excluding non-interactive users of "blame". There are
some automated users who use "blame" but don't use the "--porcelain"
format (which was already excluded). Those can set
e.g. "error:interactive" to only emit errors when "blame" is
interacting with a TTY.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 Documentation/config/blame.txt | 12 ++++++++++++
 builtin/blame.c                | 27 ++++++++++++++++++++++++++-
 t/t8002-blame.sh               | 28 ++++++++++++++++++++++++++++
 3 files changed, 66 insertions(+), 1 deletion(-)

diff --git a/Documentation/config/blame.txt b/Documentation/config/blame.txt
index c85b35de17..13570192cf 100644
--- a/Documentation/config/blame.txt
+++ b/Documentation/config/blame.txt
@@ -7,6 +7,18 @@ blame.culture::
 +
 Note that the `--porcelain` format for machine consumption is exempt
 from this enforcement to avoid breaking existing scripts.
++
+See `blame.culture.enforcement` below for tweaking the error behavior.
+
+blame.culture.enforcement::
+	When `blame.culture=blameless` is set invoking
+	linkgit:git-blame[1] becomes an `error` This variable can also
+	be set to `warning` to only warn, and to either
+	`error:interactive` or `warning:interactive` to only error out
+	or warn if stderr is connected to a TTY.
++
+This allows for enforcing a blameless culture on interactive users,
+while leaving any automated use alone.
 
 blame.blankBoundary::
 	Show blank commit object name for boundary commits in
diff --git a/builtin/blame.c b/builtin/blame.c
index 238b19db48..9f62950559 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -59,6 +59,12 @@ static size_t blame_date_width;
 
 static struct string_list mailmap = STRING_LIST_INIT_NODUP;
 
+static enum {
+	BLAME_ENFORCE_ERROR		= 1<<0,
+	BLAME_ENFORCE_WARNING		= 1<<1,
+	BLAME_ENFORCE_INTERACTIVE	= 1<<2
+} blame_culture_enforcement = BLAME_ENFORCE_ERROR;
+
 #ifndef DEBUG
 #define DEBUG 0
 #endif
@@ -686,6 +692,19 @@ static int git_blame_config(const char *var, const char *value, void *cb)
 		blameless_culture = !strcmp(value, "blameless");
 		return 0;
 	}
+	if (!strcmp(var, "blame.culture.enforcement")) {
+		if (!strcmp(value, "error"))
+			blame_culture_enforcement = BLAME_ENFORCE_ERROR;
+		else if (!strcmp(value, "error:interactive"))
+			blame_culture_enforcement = (BLAME_ENFORCE_ERROR |
+						     BLAME_ENFORCE_INTERACTIVE);
+		else if (!strcmp(value, "warning"))
+			blame_culture_enforcement = BLAME_ENFORCE_WARNING;
+		else if (!strcmp(value, "warning:interactive"))
+			blame_culture_enforcement = (BLAME_ENFORCE_WARNING |
+						     BLAME_ENFORCE_INTERACTIVE);
+		return 0;
+	}
 	if (!strcmp(var, "blame.showemail")) {
 		int *output_option = cb;
 		if (git_config_bool(var, value))
@@ -897,7 +916,13 @@ int cmd_blame(int argc, const char **argv, const char *prefix)
 		blame_date_mode.type = DATE_ISO8601;
 	} else if (!cmd_is_praise && blameless_culture &&
 		   !(output_option & OUTPUT_PORCELAIN)) {
-		die(_("must be invoked as 'git praise' with 'blame.culture=blameless' set!"));
+		if (!(blame_culture_enforcement & BLAME_ENFORCE_INTERACTIVE) ||
+		    isatty(2)) {
+			if (blame_culture_enforcement & BLAME_ENFORCE_ERROR)
+				die(_("must be invoked as 'git praise' with 'blame.culture=blameless' set!"));
+			else if (blame_culture_enforcement & BLAME_ENFORCE_WARNING)
+				warning(_("should be invoked as 'git praise' with 'blame.culture=blameless' set!"));
+		}
 	} else {
 		blame_date_mode = revs.date_mode;
 	}
diff --git a/t/t8002-blame.sh b/t/t8002-blame.sh
index 2d59b856d1..09ef0bc440 100755
--- a/t/t8002-blame.sh
+++ b/t/t8002-blame.sh
@@ -2,6 +2,7 @@
 
 test_description='git blame'
 . ./test-lib.sh
+. "$TEST_DIRECTORY/lib-terminal.sh"
 
 PROG='git blame -c'
 . "$TEST_DIRECTORY"/annotate-tests.sh
@@ -60,9 +61,36 @@ test_expect_success 'praise' '
 
 test_expect_success 'enforced praise' '
 	test_must_fail git -c blame.culture=blameless blame one 2>err &&
+	test_i18ngrep "must be.*git praise" err &&
+	test_must_fail git -c blame.culture=blameless \
+		-c blame.culture.enforcement=error blame one 2>err &&
 	test_i18ngrep "must be.*git praise" err
 '
 
+test_expect_success 'recommended praise' '
+	git -c blame.culture=blameless \
+		-c blame.culture.enforcement=warning blame one 2>err &&
+	test_i18ngrep "should be.*git praise" err
+'
+
+test_expect_success TTY 'interactive: praise blame.culture.enforcement=*:interactive' '
+	test_must_fail test_terminal git -c blame.culture=blameless \
+		-c blame.culture.enforcement=error:interactive blame one 2>err &&
+	test_i18ngrep "must be.*git praise" err &&
+	test_terminal git -c blame.culture=blameless \
+		-c blame.culture.enforcement=warning:interactive blame one 2>err &&
+	test_i18ngrep "should be.*git praise" err
+'
+
+test_expect_success TTY 'non-interactive: praise blame.culture.enforcement=*:interactive' '
+	git -c blame.culture=blameless \
+		-c blame.culture.enforcement=error:interactive blame one 2>err &&
+	test_i18ngrep ! "must be.*git praise" err &&
+	git -c blame.culture=blameless \
+		-c blame.culture.enforcement=warning:interactive blame one 2>err &&
+	test_i18ngrep ! "should be.*git praise" err
+'
+
 test_expect_success 'blame with showemail options' '
 	git blame --show-email one >blame1 &&
 	find_blame <blame1 >result &&
-- 
2.21.0.392.gf8f6787159e

Re: [PATCH 2/2] praise: make 'blameless' cultural enforcement configurable

[Johannes Schindelin]

[-- Attachment #1: Type: text/plain, Size: 6155 bytes --]

Hi,

On Mon, 1 Apr 2019, Ævar Arnfjörð Bjarmason wrote:

> The culture shock of having a 'blameless' culture from day one might
> be too much for some, so let's allow for setting
> "blame.culture.enforcement=warning" to allow for easing into the
> default of "error".
>
> Also allow for excluding non-interactive users of "blame". There are
> some automated users who use "blame" but don't use the "--porcelain"
> format (which was already excluded). Those can set
> e.g. "error:interactive" to only emit errors when "blame" is
> interacting with a TTY.
>
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>

I reviewed both patches, and they look fine to me. So they are

Blessed-by: Johannes Schindelin <johannes.schindelin@gmx.de>

:-D

> ---
>  Documentation/config/blame.txt | 12 ++++++++++++
>  builtin/blame.c                | 27 ++++++++++++++++++++++++++-
>  t/t8002-blame.sh               | 28 ++++++++++++++++++++++++++++
>  3 files changed, 66 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/config/blame.txt b/Documentation/config/blame.txt
> index c85b35de17..13570192cf 100644
> --- a/Documentation/config/blame.txt
> +++ b/Documentation/config/blame.txt
> @@ -7,6 +7,18 @@ blame.culture::
>  +
>  Note that the `--porcelain` format for machine consumption is exempt
>  from this enforcement to avoid breaking existing scripts.
> ++
> +See `blame.culture.enforcement` below for tweaking the error behavior.
> +
> +blame.culture.enforcement::
> +	When `blame.culture=blameless` is set invoking
> +	linkgit:git-blame[1] becomes an `error` This variable can also
> +	be set to `warning` to only warn, and to either
> +	`error:interactive` or `warning:interactive` to only error out
> +	or warn if stderr is connected to a TTY.
> ++
> +This allows for enforcing a blameless culture on interactive users,
> +while leaving any automated use alone.
>
>  blame.blankBoundary::
>  	Show blank commit object name for boundary commits in
> diff --git a/builtin/blame.c b/builtin/blame.c
> index 238b19db48..9f62950559 100644
> --- a/builtin/blame.c
> +++ b/builtin/blame.c
> @@ -59,6 +59,12 @@ static size_t blame_date_width;
>
>  static struct string_list mailmap = STRING_LIST_INIT_NODUP;
>
> +static enum {
> +	BLAME_ENFORCE_ERROR		= 1<<0,
> +	BLAME_ENFORCE_WARNING		= 1<<1,
> +	BLAME_ENFORCE_INTERACTIVE	= 1<<2
> +} blame_culture_enforcement = BLAME_ENFORCE_ERROR;
> +
>  #ifndef DEBUG
>  #define DEBUG 0
>  #endif
> @@ -686,6 +692,19 @@ static int git_blame_config(const char *var, const char *value, void *cb)
>  		blameless_culture = !strcmp(value, "blameless");
>  		return 0;
>  	}
> +	if (!strcmp(var, "blame.culture.enforcement")) {
> +		if (!strcmp(value, "error"))
> +			blame_culture_enforcement = BLAME_ENFORCE_ERROR;
> +		else if (!strcmp(value, "error:interactive"))
> +			blame_culture_enforcement = (BLAME_ENFORCE_ERROR |
> +						     BLAME_ENFORCE_INTERACTIVE);
> +		else if (!strcmp(value, "warning"))
> +			blame_culture_enforcement = BLAME_ENFORCE_WARNING;
> +		else if (!strcmp(value, "warning:interactive"))
> +			blame_culture_enforcement = (BLAME_ENFORCE_WARNING |
> +						     BLAME_ENFORCE_INTERACTIVE);
> +		return 0;
> +	}
>  	if (!strcmp(var, "blame.showemail")) {
>  		int *output_option = cb;
>  		if (git_config_bool(var, value))
> @@ -897,7 +916,13 @@ int cmd_blame(int argc, const char **argv, const char *prefix)
>  		blame_date_mode.type = DATE_ISO8601;
>  	} else if (!cmd_is_praise && blameless_culture &&
>  		   !(output_option & OUTPUT_PORCELAIN)) {
> -		die(_("must be invoked as 'git praise' with 'blame.culture=blameless' set!"));
> +		if (!(blame_culture_enforcement & BLAME_ENFORCE_INTERACTIVE) ||
> +		    isatty(2)) {
> +			if (blame_culture_enforcement & BLAME_ENFORCE_ERROR)
> +				die(_("must be invoked as 'git praise' with 'blame.culture=blameless' set!"));
> +			else if (blame_culture_enforcement & BLAME_ENFORCE_WARNING)
> +				warning(_("should be invoked as 'git praise' with 'blame.culture=blameless' set!"));
> +		}
>  	} else {
>  		blame_date_mode = revs.date_mode;
>  	}
> diff --git a/t/t8002-blame.sh b/t/t8002-blame.sh
> index 2d59b856d1..09ef0bc440 100755
> --- a/t/t8002-blame.sh
> +++ b/t/t8002-blame.sh
> @@ -2,6 +2,7 @@
>
>  test_description='git blame'
>  . ./test-lib.sh
> +. "$TEST_DIRECTORY/lib-terminal.sh"
>
>  PROG='git blame -c'
>  . "$TEST_DIRECTORY"/annotate-tests.sh
> @@ -60,9 +61,36 @@ test_expect_success 'praise' '
>
>  test_expect_success 'enforced praise' '
>  	test_must_fail git -c blame.culture=blameless blame one 2>err &&
> +	test_i18ngrep "must be.*git praise" err &&
> +	test_must_fail git -c blame.culture=blameless \
> +		-c blame.culture.enforcement=error blame one 2>err &&
>  	test_i18ngrep "must be.*git praise" err
>  '
>
> +test_expect_success 'recommended praise' '
> +	git -c blame.culture=blameless \
> +		-c blame.culture.enforcement=warning blame one 2>err &&
> +	test_i18ngrep "should be.*git praise" err
> +'
> +
> +test_expect_success TTY 'interactive: praise blame.culture.enforcement=*:interactive' '
> +	test_must_fail test_terminal git -c blame.culture=blameless \
> +		-c blame.culture.enforcement=error:interactive blame one 2>err &&
> +	test_i18ngrep "must be.*git praise" err &&
> +	test_terminal git -c blame.culture=blameless \
> +		-c blame.culture.enforcement=warning:interactive blame one 2>err &&
> +	test_i18ngrep "should be.*git praise" err
> +'
> +
> +test_expect_success TTY 'non-interactive: praise blame.culture.enforcement=*:interactive' '
> +	git -c blame.culture=blameless \
> +		-c blame.culture.enforcement=error:interactive blame one 2>err &&
> +	test_i18ngrep ! "must be.*git praise" err &&
> +	git -c blame.culture=blameless \
> +		-c blame.culture.enforcement=warning:interactive blame one 2>err &&
> +	test_i18ngrep ! "should be.*git praise" err
> +'
> +
>  test_expect_success 'blame with showemail options' '
>  	git blame --show-email one >blame1 &&
>  	find_blame <blame1 >result &&
> --
> 2.21.0.392.gf8f6787159e
>
>