[PATCH v2 0/2] format-patch: make boolean and mention in diff-options.adoc
From: Kristoffer Haugsbakk <code@khaugsbakk.name>
See: https://lore.kernel.org/git/a9602602-5fea-40c0-a505-34133ed4d58c@app.fastmail.com/
Topic name (applied): kh/format-patch-noprefix-is-boolean
Topic summary: The config `format.noprefix` should act like a boolean. But now it is active no matter what the value is. Change it to act like a boolean like `diff.noprefix`. Also mention it instead of `diff.noprefix` in git-format-patch(1) doc.
§ Changes in v2
Got feedback from Junio.
Copied from note on Patch 1/2:
• Use `advise()` for the hint; you get the `hint:` prefix and color
• Rework the hint, or the advise, to say that any value *used to be*
treated as `true`. That better helps people who maybe set `nope` when
they meant, “no, I don’t want any prefix” (for example)
Also a commit message tweak on Patch 2/2.
§ Link to v1
https://lore.kernel.org/git/CV_format.noprefix_boolean.39c@msgid.xyz/
§ Breaking change (unchanged since v1)
This is a breaking change. But I have opted to die if the config is not a boolean and just hint about the change. See also Peff’s comment on such a breaking change in that link.
I have also asked here about marking breaking changes:
https://lore.kernel.org/git/3124b359-2929-4f3f-9ac6-793277fe422b@jontes.page/T/#ma8856238748d0794a0da6c64e1c0c8a4824b996f
Kristoffer Haugsbakk (2):
format-patch: make format.noprefix a boolean
doc: diff-options.adoc: show format.noprefix for format-patch
Documentation/diff-options.adoc | 4 +++-
builtin/log.c | 14 +++++++++++++-
t/t4014-format-patch.sh | 16 ++++++++++++++++
3 files changed, 32 insertions(+), 2 deletions(-)
Interdiff against v1:
Show changes to 2 files +8 −7
builtin/log.c, t/t4014-format-patch.sh
diff --git a/builtin/log.c b/builtin/log.c
index e56af7465ae..275122b807e 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -40,6 +40,7 @@
#include "mailmap.h"
#include "progress.h"
#include "commit-slab.h"
+#include "advice.h"
#include "commit-reach.h"
#include "range-diff.h"
@@ -1101,11 +1102,11 @@ static int git_format_config(const char *var, const char *value,
int status = die_message(
_("bad boolean config value '%s' for '%s'"),
value, var);
- fprintf(stderr,
- _("hint: '%s' used to accept any value but "
- "now only\n"
- "hint: accepts boolean values, like '%s'\n"),
- var, "diff.noprefix");
+ advise(_("'%s' used to accept any value and "
+ "treat that as 'true'.\n"
+ "Now it only accepts boolean values, "
+ "like what '%s' does.\n"),
+ var, "diff.noprefix");
exit(status);
}
return 0;
diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh
index 645ac402a19..c20091e36fe 100755
--- a/t/t4014-format-patch.sh
+++ b/t/t4014-format-patch.sh
@@ -2555,8 +2555,8 @@ test_expect_success 'format-patch --default-prefix overrides format.noprefix' '
test_expect_success 'errors on format.noprefix which is not boolean' '
cat >expect <<-EOF &&
fatal: bad boolean config value ${SQ}not-a-bool${SQ} for ${SQ}format.noprefix${SQ}
- hint: ${SQ}format.noprefix${SQ} used to accept any value but now only
- hint: accepts boolean values, like ${SQ}diff.noprefix${SQ}
+ hint: ${SQ}format.noprefix${SQ} used to accept any value and treat that as ${SQ}true${SQ}.
+ hint: Now it only accepts boolean values, like what ${SQ}diff.noprefix${SQ} does.
EOF
test_must_fail git -c format.noprefix=not-a-bool \
format-patch -1 --stdout 2>actual &&
Range-diff against v1:
1: 49fa161a392 ! 1: eee61561eb3 format-patch: make format.noprefix a boolean
@@ Commit message
format.noprefix* (current behavior) will now have the opposite
experience. Which is not a reasonable setup.
- Let’s only offer a breaking change fig leaf by hinting about the
+ Let’s only offer a breaking change fig leaf by advising about the
previous behavior before dying.
Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>
## builtin/log.c ##
+@@
+ #include "mailmap.h"
+ #include "progress.h"
+ #include "commit-slab.h"
++#include "advice.h"
+
+ #include "commit-reach.h"
+ #include "range-diff.h"
@@ builtin/log.c: static int git_format_config(const char *var, const char *value,
return 0;
}
@@ builtin/log.c: static int git_format_config(const char *var, const char *value,
+ int status = die_message(
+ _("bad boolean config value '%s' for '%s'"),
+ value, var);
-+ fprintf(stderr,
-+ _("hint: '%s' used to accept any value but "
-+ "now only\n"
-+ "hint: accepts boolean values, like '%s'\n"),
-+ var, "diff.noprefix");
++ advise(_("'%s' used to accept any value and "
++ "treat that as 'true'.\n"
++ "Now it only accepts boolean values, "
++ "like what '%s' does.\n"),
++ var, "diff.noprefix");
+ exit(status);
+ }
return 0;
@@ t/t4014-format-patch.sh: test_expect_success 'format-patch respects format.nopre
+test_expect_success 'errors on format.noprefix which is not boolean' '
+ cat >expect <<-EOF &&
+ fatal: bad boolean config value ${SQ}not-a-bool${SQ} for ${SQ}format.noprefix${SQ}
-+ hint: ${SQ}format.noprefix${SQ} used to accept any value but now only
-+ hint: accepts boolean values, like ${SQ}diff.noprefix${SQ}
++ hint: ${SQ}format.noprefix${SQ} used to accept any value and treat that as ${SQ}true${SQ}.
++ hint: Now it only accepts boolean values, like what ${SQ}diff.noprefix${SQ} does.
+ EOF
+ test_must_fail git -c format.noprefix=not-a-bool \
+ format-patch -1 --stdout 2>actual &&
2: 0cef1915a9c ! 2: b9b583bd007 doc: diff-options.adoc: show format.noprefix for format-patch
@@ Commit message
git-format-patch(1) uses `format.noprefix` and ignores `diff.noprefix`.
The configuration variable `format.prefix` was added as an “escape
- hatch” because “it’s unlikely that anybody really wants format.
+ hatch”, and “it’s unlikely that anybody really wants format.
noprefix=true in the first place.”[1] Based on that there doesn’t
seem to be a need to widely advertise this configuration variable.
base-commit: 67ad42147a7acc2af6074753ebd03d904476118f
--
2.53.0.26.g2afa8602a26
