* [PATCH 1/1] clean: further clean-up of implementation around "--force"
From: Junio C Hamano @ 2024-03-03 22:06 UTC (permalink / raw)
To: git
In-Reply-To: <20240303220600.2491792-1-gitster@pobox.com>
We clarified how clean.requireForce interacts with the --dry-run
option in the previous commit, both in the implementation and in the
documentation. Even when "git clean" (without other options) is
required to be used with "--force" (i.e. either clean.requireForce
is unset, or explicitly set to true) to protect end-users from
casual invocation of the command by mistake, "--dry-run" does not
require "--force" to be used, because it is already its own
protection mechanism by being a no-op to the working tree files.
The previous commit, however, missed another clean-up opportunity
around the same area. Just like in the "--dry-run" mode, the
command in the "--interactive" mode does not require "--force",
either. This is because by going interactive and giving the end
user one more step to confirm, the mode itself is serving as its own
protection mechanism.
Let's take things one step further, unify the code that defines
interaction between `--force` and these two other options. Just
like we added explanation for the reason why "--dry-run" does not
honor `clean.requireForce`, add the same explanation for
"--interactive". Finally, add some tests to show the interaction
between "--force" and "--interactive" (we already have tests that
show interaction between "--force" and "--dry-run").
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
Documentation/config/clean.txt | 2 +-
Documentation/git-clean.txt | 4 +++-
builtin/clean.c | 9 ++-------
t/t7300-clean.sh | 6 ++++++
4 files changed, 12 insertions(+), 9 deletions(-)
diff --git a/Documentation/config/clean.txt b/Documentation/config/clean.txt
index b19ca210f3..c0188ead4e 100644
--- a/Documentation/config/clean.txt
+++ b/Documentation/config/clean.txt
@@ -1,3 +1,3 @@
clean.requireForce::
A boolean to make git-clean refuse to delete files unless -f
- or -i is given. Defaults to true.
+ is given. Defaults to true.
diff --git a/Documentation/git-clean.txt b/Documentation/git-clean.txt
index 662eebb852..082d033438 100644
--- a/Documentation/git-clean.txt
+++ b/Documentation/git-clean.txt
@@ -37,7 +37,7 @@ OPTIONS
--force::
If the Git configuration variable clean.requireForce is not set
to false, 'git clean' will refuse to delete files or directories
- unless given -f or -i. Git will refuse to modify untracked
+ unless given -f. Git will refuse to modify untracked
nested git repositories (directories with a .git subdirectory)
unless a second -f is given.
@@ -45,6 +45,8 @@ OPTIONS
--interactive::
Show what would be done and clean files interactively. See
``Interactive mode'' for details.
+ Configuration variable clean.requireForce is ignored, as
+ this mode gives its own safety protection by going interactive.
-n::
--dry-run::
diff --git a/builtin/clean.c b/builtin/clean.c
index 41502dcb0d..29efe84153 100644
--- a/builtin/clean.c
+++ b/builtin/clean.c
@@ -950,13 +950,8 @@ int cmd_clean(int argc, const char **argv, const char *prefix)
argc = parse_options(argc, argv, prefix, options, builtin_clean_usage,
0);
- /* Dry run won't remove anything, so requiring force makes no sense */
- if (dry_run)
- require_force = 0;
-
- if (require_force != 0 && !force && !interactive)
- die(_("clean.requireForce is true and neither -f nor -i given:"
- " refusing to clean"));
+ if (require_force != 0 && !force && !interactive && !dry_run)
+ die(_("clean.requireForce is true and -f not given: refusing to clean"));
if (force > 1)
rm_flags = 0;
diff --git a/t/t7300-clean.sh b/t/t7300-clean.sh
index 611b3dd3ae..1f7201eb60 100755
--- a/t/t7300-clean.sh
+++ b/t/t7300-clean.sh
@@ -407,6 +407,12 @@ test_expect_success 'clean.requireForce and -f' '
'
+test_expect_success 'clean.requireForce and --interactive' '
+ git clean --interactive </dev/null >output 2>error &&
+ test_grep ! "requireForce is true and" error &&
+ test_grep "\*\*\* Commands \*\*\*" output
+'
+
test_expect_success 'core.excludesfile' '
echo excludes >excludes &&
--
2.44.0-84-gb387623c12
^ permalink raw reply related
* Re: [PATCH v2] clean: improve -n and -f implementation and documentation
From: Junio C Hamano @ 2024-03-03 22:05 UTC (permalink / raw)
To: git; +Cc: Sergey Organov, Jean-Noël AVILA, Kristoffer Haugsbakk
In-Reply-To: <7le6ziqzb.fsf_-_@osv.gnss.ru>
Sergey Organov <sorganov@gmail.com> writes:
> Changes since v1:
>
> * Fixed style of the if() statement
>
> * Merged two error messages into one
>
> * clean.requireForce description changed accordingly
Excellent.
> diff --git a/builtin/clean.c b/builtin/clean.c
> index d90766cad3a0..41502dcb0dde 100644
> --- a/builtin/clean.c
> +++ b/builtin/clean.c
> @@ -25,7 +25,7 @@
> #include "help.h"
> #include "prompt.h"
>
> -static int force = -1; /* unset */
> +static int require_force = -1; /* unset */
> static int interactive;
> static struct string_list del_list = STRING_LIST_INIT_DUP;
> static unsigned int colopts;
> @@ -128,7 +128,7 @@ static int git_clean_config(const char *var, const char *value,
> }
>
> if (!strcmp(var, "clean.requireforce")) {
> - force = !git_config_bool(var, value);
> + require_force = git_config_bool(var, value);
> return 0;
> }
>
> @@ -920,7 +920,7 @@ int cmd_clean(int argc, const char **argv, const char *prefix)
> {
> int i, res;
> int dry_run = 0, remove_directories = 0, quiet = 0, ignored = 0;
> - int ignored_only = 0, config_set = 0, errors = 0, gone = 1;
> + int ignored_only = 0, force = 0, errors = 0, gone = 1;
> int rm_flags = REMOVE_DIR_KEEP_NESTED_GIT;
> struct strbuf abs_path = STRBUF_INIT;
> struct dir_struct dir = DIR_INIT;
> @@ -946,22 +946,17 @@ int cmd_clean(int argc, const char **argv, const char *prefix)
> };
>
> git_config(git_clean_config, NULL);
> - if (force < 0)
> - force = 0;
> - else
> - config_set = 1;
The above changes are a significant improvement. Instead of a
single "force" variable whose meaning is fuzzy, we now have
"require_force" to track the config setting, and "force" to indicate
the "--force" option. THis makes the code's intent much clearer.
> argc = parse_options(argc, argv, prefix, options, builtin_clean_usage,
> 0);
>
> - if (!interactive && !dry_run && !force) {
> - if (config_set)
> - die(_("clean.requireForce set to true and neither -i, -n, nor -f given; "
> - "refusing to clean"));
> - else
> - die(_("clean.requireForce defaults to true and neither -i, -n, nor -f given;"
And thanks to that, the above trick with an extra variable "config_set",
which smells highly a round-about way, can be simplified.
> + /* Dry run won't remove anything, so requiring force makes no sense */
> + if (dry_run)
> + require_force = 0;
> + if (require_force != 0 && !force && !interactive)
However, the above logic could be improved. The behaviour we have,
for a user who does *not* explicitly disable config.requireForce,
is, that when clean.requireForce is not set to 0, we would fail
unless one of these is in effect: -f, -n, -i. Even though using
either -n or -i makes it unnecessary to use -f *exactly* the same
way, the above treats dry_run and interactive separately with two if
statements, which is suboptimal as a "code/logic clean-up".
The reason for the behaviour can be explained this way:
* "git clean" (with neither -i nor -n. The user wants the default
mode that has no built-in protection will be stopped without -f.
* "git clean -n". The user wants the dry-run mode that has its own
protection, i.e. being always no-op to the files, so there is no
need to fail here for the lack of "-f".
* "git clean --interactive". The user wants the interactive mode
that has its own protection, i.e. giving the end-user a chance to
say "oh, I didn't mean to remove these files, 'q'uit from this
mistake", so there is no need to fail here for the lack of "-f".
> + die(_("clean.requireForce is true and neither -f nor -i given:"
> " refusing to clean"));
The message is certainly cleaner compared to the previous round, but
this also can be improved. Stepping back a bit and thinking who are
the target audience of this message. The only users who see this
message are running "git clean" in its default (unprotected) mode,
and they wanted to "clean" for real. If they wanted to do dry-run,
they would have said "-n" themselves, and that is why we can safely
omit mention of "-n" we had in the original message.
These users did not want to run the interractive clean, either---if
they wanted to go interractive, they would have said "-i"
themselves. So we do not need to mention "-i" either for exactly
the same logic.
Based on the above observation,
I'll send a follow-up patch to clean up the code around here (both
implementation and documentation), taking '-i' into account as well.
^ permalink raw reply
* [PATCH 1/1] rebase: teach `--exec` about `GIT_REBASE_BRANCH`
From: Kristoffer Haugsbakk @ 2024-03-03 20:03 UTC (permalink / raw)
To: git; +Cc: Kristoffer Haugsbakk
In-Reply-To: <cover.1709495964.git.code@khaugsbakk.name>
The command fed to `--exec` might need some contextual information from
the branch name. But there is no convenient access to the branch name
that we were on before starting the rebase; rebase operates in detached
HEAD mode so we cannot ask for it directly. This means that we need to
parse something like this from the first line of `git branch --list`:
(no branch, rebasing <branch>)
This is a moderate amount of effort for something that git-rebase(1) can
store for us.
To that end, teach `--exec` about an env. variable which stores the
branch name for the rebase-in-progress, if applicable.
Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>
---
Documentation/git-rebase.txt | 4 ++++
builtin/rebase.c | 15 ++++++++++++++-
t/t3409-rebase-environ.sh | 19 +++++++++++++++++++
3 files changed, 37 insertions(+), 1 deletion(-)
diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
index 06206521fc3..9b3d6ee8203 100644
--- a/Documentation/git-rebase.txt
+++ b/Documentation/git-rebase.txt
@@ -578,6 +578,10 @@ squash/fixup series.
This uses the `--interactive` machinery internally, but it can be run
without an explicit `--interactive`.
+
+The command has access to the environment variable `GIT_REBASE_BRANCH`
+which stores the branch name that `HEAD` was pointing at when the rebase
+started, if applicable.
++
See also INCOMPATIBLE OPTIONS below.
--root::
diff --git a/builtin/rebase.c b/builtin/rebase.c
index 5b086f651a6..0202130c2d7 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -1044,6 +1044,17 @@ static int check_exec_cmd(const char *cmd)
return 0;
}
+static void try_set_env_git_rebase_branch(void)
+{
+ const char *refname = resolve_ref_unsafe("HEAD", 0, NULL, NULL);
+ const char *shortname = NULL;
+
+ if (refname)
+ skip_prefix(refname, "refs/heads/", &shortname);
+ if (shortname)
+ xsetenv("GIT_REBASE_BRANCH", shortname, true);
+}
+
int cmd_rebase(int argc, const char **argv, const char *prefix)
{
struct rebase_options options = REBASE_OPTIONS_INIT;
@@ -1451,8 +1462,10 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
if (gpg_sign)
options.gpg_sign_opt = xstrfmt("-S%s", gpg_sign);
- if (options.exec.nr)
+ if (options.exec.nr) {
imply_merge(&options, "--exec");
+ try_set_env_git_rebase_branch();
+ }
if (options.type == REBASE_APPLY) {
if (ignore_whitespace)
diff --git a/t/t3409-rebase-environ.sh b/t/t3409-rebase-environ.sh
index acaf5558dbe..5b1d78a255a 100755
--- a/t/t3409-rebase-environ.sh
+++ b/t/t3409-rebase-environ.sh
@@ -21,4 +21,23 @@ test_expect_success 'rebase --exec does not muck with GIT_WORK_TREE' '
test_must_be_empty environ
'
+test_expect_success 'rebase --exec cmd can access GIT_REBASE_BRANCH' '
+ write_script cmd <<-\EOF &&
+printf "%s\n" $GIT_REBASE_BRANCH >actual
+EOF
+ git branch --show-current >expect &&
+ git rebase --exec ./cmd HEAD~1 &&
+ test_cmp expect actual
+'
+
+test_expect_success 'rebase --exec cmd has no GIT_REBASE_BRANCH when on detached HEAD' '
+ test_when_finished git checkout - &&
+ git checkout --detach &&
+ write_script cmd <<-\EOF &&
+printf "%s" $GIT_REBASE_BRANCH >environ
+EOF
+ git rebase --exec ./cmd HEAD~1 &&
+ test_must_be_empty environ
+'
+
test_done
--
2.44.0.64.g52b67adbeb2
^ permalink raw reply related
* [PATCH 0/1] rebase: teach `--exec` about `GIT_REBASE_BRANCH`
From: Kristoffer Haugsbakk @ 2024-03-03 20:03 UTC (permalink / raw)
To: git; +Cc: Kristoffer Haugsbakk
The following patch adds an env. variable for the branch name that we
were on before the rebase operation started. This is for use by `--exec
<cmd>`.
The need for fetching the branch name came up while making a script for
`--exec` and it seemed that parsing the name out of the first line of
`git branch --list` was the best approach.
I thought that was inconvenient. Why not an environment variable set by
git-rebase(1)? (open question)
See: https://stackoverflow.com/a/50124157/1725151
§ Implementation
The implementation is inspired by
`builtin/branch.c:print_current_branch_name`.
Kristoffer Haugsbakk (1):
rebase: teach `--exec` about `GIT_REBASE_BRANCH`
Documentation/git-rebase.txt | 4 ++++
builtin/rebase.c | 15 ++++++++++++++-
t/t3409-rebase-environ.sh | 19 +++++++++++++++++++
3 files changed, 37 insertions(+), 1 deletion(-)
--
2.44.0.64.g52b67adbeb2
^ permalink raw reply
* Re: [PATCH v2 0/1] advise about ref syntax rules
From: Kristoffer Haugsbakk @ 2024-03-03 19:10 UTC (permalink / raw)
To: git
In-Reply-To: <cover.1709491818.git.code@khaugsbakk.name>
I forgot the range-diff. But turns out it’s empty.
--
Kristoffer Haugsbakk
^ permalink raw reply
* [PATCH v2 1/1] branch: advise about ref syntax rules
From: Kristoffer Haugsbakk @ 2024-03-03 18:58 UTC (permalink / raw)
To: git; +Cc: Kristoffer Haugsbakk
In-Reply-To: <cover.1709491818.git.code@khaugsbakk.name>
git-branch(1) will error out if you give it a bad ref name. But the user
might not understand why or what part of the name is illegal.
The user might know that there are some limitations based on the *loose
ref* format (filenames), but there are also further rules for
easier integration with shell-based tools, pathname expansion, and
playing well with reference name expressions.
The man page for git-check-ref-format(1) contains these rules. Let’s
advise about it since that is not a command that you just happen
upon. Also make this advise configurable since you might not want to be
reminded every time you make a little typo.
Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>
---
Notes (series):
v2:
• Make the advise optional via configuration
• Propagate error properly with `die_message(…)` instead of `exit(1)`
• Flesh out commit message a bit
Documentation/config/advice.txt | 3 +++
advice.c | 1 +
advice.h | 1 +
branch.c | 8 ++++++--
builtin/branch.c | 8 ++++++--
t/t3200-branch.sh | 11 +++++++++++
6 files changed, 28 insertions(+), 4 deletions(-)
diff --git a/Documentation/config/advice.txt b/Documentation/config/advice.txt
index c7ea70f2e2e..552cfbcd48c 100644
--- a/Documentation/config/advice.txt
+++ b/Documentation/config/advice.txt
@@ -94,6 +94,9 @@ advice.*::
'pushNonFFCurrent', 'pushNonFFMatching', 'pushAlreadyExists',
'pushFetchFirst', 'pushNeedsForce', and 'pushRefNeedsUpdate'
simultaneously.
+ refSyntax::
+ Point the user towards the ref syntax documentation if
+ they give an invalid ref name.
resetNoRefresh::
Advice to consider using the `--no-refresh` option to
linkgit:git-reset[1] when the command takes more than 2 seconds
diff --git a/advice.c b/advice.c
index 6e9098ff089..550c2968908 100644
--- a/advice.c
+++ b/advice.c
@@ -68,6 +68,7 @@ static struct {
[ADVICE_PUSH_UNQUALIFIED_REF_NAME] = { "pushUnqualifiedRefName" },
[ADVICE_PUSH_UPDATE_REJECTED] = { "pushUpdateRejected" },
[ADVICE_PUSH_UPDATE_REJECTED_ALIAS] = { "pushNonFastForward" }, /* backwards compatibility */
+ [ADVICE_REF_SYNTAX] = { "refSyntax" },
[ADVICE_RESET_NO_REFRESH_WARNING] = { "resetNoRefresh" },
[ADVICE_RESOLVE_CONFLICT] = { "resolveConflict" },
[ADVICE_RM_HINTS] = { "rmHints" },
diff --git a/advice.h b/advice.h
index 9d4f49ae38b..d15fe2351ab 100644
--- a/advice.h
+++ b/advice.h
@@ -36,6 +36,7 @@ enum advice_type {
ADVICE_PUSH_UNQUALIFIED_REF_NAME,
ADVICE_PUSH_UPDATE_REJECTED,
ADVICE_PUSH_UPDATE_REJECTED_ALIAS,
+ ADVICE_REF_SYNTAX,
ADVICE_RESET_NO_REFRESH_WARNING,
ADVICE_RESOLVE_CONFLICT,
ADVICE_RM_HINTS,
diff --git a/branch.c b/branch.c
index 6719a181bd1..621019fcf4b 100644
--- a/branch.c
+++ b/branch.c
@@ -370,8 +370,12 @@ int read_branch_desc(struct strbuf *buf, const char *branch_name)
*/
int validate_branchname(const char *name, struct strbuf *ref)
{
- if (strbuf_check_branch_ref(ref, name))
- die(_("'%s' is not a valid branch name"), name);
+ if (strbuf_check_branch_ref(ref, name)) {
+ int code = die_message(_("'%s' is not a valid branch name"), name);
+ advise_if_enabled(ADVICE_REF_SYNTAX,
+ _("See `man git check-ref-format`"));
+ exit(code);
+ }
return ref_exists(ref->buf);
}
diff --git a/builtin/branch.c b/builtin/branch.c
index cfb63cce5fb..1c122ee8a7b 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -576,8 +576,12 @@ static void copy_or_rename_branch(const char *oldname, const char *newname, int
*/
if (ref_exists(oldref.buf))
recovery = 1;
- else
- die(_("invalid branch name: '%s'"), oldname);
+ else {
+ int code = die_message(_("invalid branch name: '%s'"), oldname);
+ advise_if_enabled(ADVICE_REF_SYNTAX,
+ _("See `man git check-ref-format`"));
+ exit(code);
+ }
}
for (int i = 0; worktrees[i]; i++) {
diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
index de7d3014e4f..d21fdf09c90 100755
--- a/t/t3200-branch.sh
+++ b/t/t3200-branch.sh
@@ -1725,4 +1725,15 @@ test_expect_success '--track overrides branch.autoSetupMerge' '
test_cmp_config "" --default "" branch.foo5.merge
'
+cat <<\EOF >expect
+fatal: 'foo..bar' is not a valid branch name
+hint: See `man git check-ref-format`
+hint: Disable this message with "git config advice.refSyntax false"
+EOF
+
+test_expect_success 'errors if given a bad branch name' '
+ test_must_fail git branch foo..bar >actual 2>&1 &&
+ test_cmp expect actual
+'
+
test_done
--
2.44.0.64.g52b67adbeb2
^ permalink raw reply related
* [PATCH v2 0/1] advise about ref syntax rules
From: Kristoffer Haugsbakk @ 2024-03-03 18:58 UTC (permalink / raw)
To: git; +Cc: Kristoffer Haugsbakk
In-Reply-To: <d275d1d179b90592ddd7b5da2ae4573b3f7a37b7.1709307442.git.code@khaugsbakk.name>
Point the user towards the ref/branch name syntax rules if they give an
invalid name.
§ git-replace(1)
I did not add a hint for a similar message in `builtin/replace.c`.
`builtin/replace.c` has an error message in `check_ref_valid` for an
invalid ref name:
```
return error(_("'%s' is not a valid ref name"), ref->buf);
```
But there doesn’t seem to be a point to placing a hint here.
The preceding calls to `repo_get_oid` will catch both missing refs and
existing refs with invalid names:
```
if (repo_get_oid(r, refname, &object))
return error(_("failed to resolve '%s' as a valid ref"), refname);
```
Like for example this:
```
$ printf $(git rev-parse @~) > .git/refs/heads/hello..goodbye
$ git replace @ hello..goodbye
error: failed to resolve 'hello..goodbye' as a valid ref
[…]
$ git replace @ non-existing
error: failed to resolve 'non-existing' as a valid ref
```
§ Alternatives (to this change)
While working on this I also thought that it might be nice to have a
man page `gitrefsyntax`. That one could use a lot of the content from
`man git check-ref-format` verbatim. Then the hint could point towards
that man page. And it seems that AsciiDoc supports _includes_ which
means that the rules don’t have to be duplicated between the two man
pages.
§ Changes in v2
• Make the advise optional via configuration
• At first I thought that this wasn’t needed but I imagine the advice
could get repetitive for typos and such
• Propagate error properly with `die_message(…)` instead of `exit(1)`
• Flesh out commit message a bit
Kristoffer Haugsbakk (1):
branch: advise about ref syntax rules
Documentation/config/advice.txt | 3 +++
advice.c | 1 +
advice.h | 1 +
branch.c | 8 ++++++--
builtin/branch.c | 8 ++++++--
t/t3200-branch.sh | 11 +++++++++++
6 files changed, 28 insertions(+), 4 deletions(-)
--
2.44.0.64.g52b67adbeb2
^ permalink raw reply
* Re: [PATCH 0/2] Support diff.wordDiff config
From: Junio C Hamano @ 2024-03-03 17:45 UTC (permalink / raw)
To: Chris Torek; +Cc: Kristoffer Haugsbakk, Karthik Nayak, oliver, git
In-Reply-To: <CAPx1GveaNR9ooWqE1VkAuFg5NO4Lwzx7bj-W1mWeHRg-rcg6+w@mail.gmail.com>
Chris Torek <chris.torek@gmail.com> writes:
> This tension is relieved somewhat when there *are* separate
> plumbing commands, such as `git diff-index` and `git diff-tree`
> and so on, or `git rev-list` vs `git log`. Unfortunately there
> are some commands, including `git log` itself, that have options
> that are missing from the roughly-equivalent plumbing command,
> and there are commands (such as `git stash` and `git status`)
> that either do not have, or at one time lacked, plumbing command
> equivalents or options.
Yup. It is my pet peeve that more and more contributors got lazy
and tweaked only Porcelain commands, without bothering to improve
plumbing commands to match, while adding more features during the
last decade. Unfortunately there is no easy remedy after such sins
have been committed. Once people start using `git log` in their
scripts, it is way too late to tell them to update their scripts to
use `git log --porcelain`. The fact that you need to tell them is
an admission that you already broke their scripts.
^ permalink raw reply
* [PATCH v2 3/6] parse-options: factor out register_abbrev() and struct parsed_option
From: René Scharfe @ 2024-03-03 12:19 UTC (permalink / raw)
To: git
In-Reply-To: <20240303121944.20627-1-l.s.r@web.de>
Add a function, register_abbrev(), for storing the necessary details for
remembering an abbreviated and thus potentially ambiguous option. Call
it instead of sharing the code using goto, to make the control flow more
explicit.
Conveniently collect these details in the new struct parsed_option to
reduce the number of necessary function arguments.
Signed-off-by: René Scharfe <l.s.r@web.de>
---
parse-options.c | 83 +++++++++++++++++++++++++++++--------------------
1 file changed, 49 insertions(+), 34 deletions(-)
diff --git a/parse-options.c b/parse-options.c
index 056c6b30e9..398ebaef14 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -350,14 +350,40 @@ static int is_alias(struct parse_opt_ctx_t *ctx,
return 0;
}
+struct parsed_option {
+ const struct option *option;
+ enum opt_parsed flags;
+};
+
+static void register_abbrev(struct parse_opt_ctx_t *p,
+ const struct option *option, enum opt_parsed flags,
+ struct parsed_option *abbrev,
+ struct parsed_option *ambiguous)
+{
+ if (p->flags & PARSE_OPT_KEEP_UNKNOWN_OPT)
+ return;
+ if (abbrev->option &&
+ !is_alias(p, abbrev->option, option)) {
+ /*
+ * If this is abbreviated, it is
+ * ambiguous. So when there is no
+ * exact match later, we need to
+ * error out.
+ */
+ ambiguous->option = abbrev->option;
+ ambiguous->flags = abbrev->flags;
+ }
+ abbrev->option = option;
+ abbrev->flags = flags;
+}
+
static enum parse_opt_result parse_long_opt(
struct parse_opt_ctx_t *p, const char *arg,
const struct option *options)
{
const char *arg_end = strchrnul(arg, '=');
- const struct option *abbrev_option = NULL, *ambiguous_option = NULL;
- enum opt_parsed abbrev_flags = OPT_LONG, ambiguous_flags = OPT_LONG;
- int allow_abbrev = !(p->flags & PARSE_OPT_KEEP_UNKNOWN_OPT);
+ struct parsed_option abbrev = { .option = NULL, .flags = OPT_LONG };
+ struct parsed_option ambiguous = { .option = NULL, .flags = OPT_LONG };
for (; options->type != OPTION_END; options++) {
const char *rest, *long_name = options->long_name;
@@ -377,31 +403,20 @@ static enum parse_opt_result parse_long_opt(
rest = NULL;
if (!rest) {
/* abbreviated? */
- if (allow_abbrev &&
- !strncmp(long_name, arg, arg_end - arg)) {
-is_abbreviated:
- if (abbrev_option &&
- !is_alias(p, abbrev_option, options)) {
- /*
- * If this is abbreviated, it is
- * ambiguous. So when there is no
- * exact match later, we need to
- * error out.
- */
- ambiguous_option = abbrev_option;
- ambiguous_flags = abbrev_flags;
- }
- abbrev_option = options;
- abbrev_flags = flags ^ opt_flags;
+ if (!strncmp(long_name, arg, arg_end - arg)) {
+ register_abbrev(p, options, flags ^ opt_flags,
+ &abbrev, &ambiguous);
continue;
}
/* negation allowed? */
if (options->flags & PARSE_OPT_NONEG)
continue;
/* negated and abbreviated very much? */
- if (allow_abbrev && starts_with("no-", arg)) {
+ if (starts_with("no-", arg)) {
flags |= OPT_UNSET;
- goto is_abbreviated;
+ register_abbrev(p, options, flags ^ opt_flags,
+ &abbrev, &ambiguous);
+ continue;
}
/* negated? */
if (!starts_with(arg, "no-"))
@@ -409,12 +424,12 @@ static enum parse_opt_result parse_long_opt(
flags |= OPT_UNSET;
if (!skip_prefix(arg + 3, long_name, &rest)) {
/* abbreviated and negated? */
- if (allow_abbrev &&
- !strncmp(long_name, arg + 3,
+ if (!strncmp(long_name, arg + 3,
arg_end - arg - 3))
- goto is_abbreviated;
- else
- continue;
+ register_abbrev(p, options,
+ flags ^ opt_flags,
+ &abbrev, &ambiguous);
+ continue;
}
}
if (*rest) {
@@ -425,24 +440,24 @@ static enum parse_opt_result parse_long_opt(
return get_value(p, options, flags ^ opt_flags);
}
- if (disallow_abbreviated_options && (ambiguous_option || abbrev_option))
+ if (disallow_abbreviated_options && (ambiguous.option || abbrev.option))
die("disallowed abbreviated or ambiguous option '%.*s'",
(int)(arg_end - arg), arg);
- if (ambiguous_option) {
+ if (ambiguous.option) {
error(_("ambiguous option: %s "
"(could be --%s%s or --%s%s)"),
arg,
- (ambiguous_flags & OPT_UNSET) ? "no-" : "",
- ambiguous_option->long_name,
- (abbrev_flags & OPT_UNSET) ? "no-" : "",
- abbrev_option->long_name);
+ (ambiguous.flags & OPT_UNSET) ? "no-" : "",
+ ambiguous.option->long_name,
+ (abbrev.flags & OPT_UNSET) ? "no-" : "",
+ abbrev.option->long_name);
return PARSE_OPT_HELP;
}
- if (abbrev_option) {
+ if (abbrev.option) {
if (*arg_end)
p->opt = arg_end + 1;
- return get_value(p, abbrev_option, abbrev_flags);
+ return get_value(p, abbrev.option, abbrev.flags);
}
return PARSE_OPT_UNKNOWN;
}
--
2.44.0
^ permalink raw reply related
* [PATCH v2 4/6] parse-options: detect ambiguous self-negation
From: René Scharfe @ 2024-03-03 12:19 UTC (permalink / raw)
To: git
In-Reply-To: <20240303121944.20627-1-l.s.r@web.de>
Git currently does not detect the ambiguity of an option that starts
with "no" like --notes and its negated form if given just --n or --no.
All Git commands with such options have other negatable options, and
we detect the ambiguity with them, so that's currently only a potential
problem for scripts that use git rev-parse --parseopt.
Let's fix it nevertheless, as there's no need for that confusion. To
detect the ambiguity we have to loosen the check in register_abbrev(),
as an option is considered an alias of itself. Add non-matching
negation flags as a criterion to recognize an option being ambiguous
with its negated form.
And we need to keep going after finding a non-negated option as an
abbreviated candidate and perform the negation checks in the same
loop.
Signed-off-by: René Scharfe <l.s.r@web.de>
---
parse-options.c | 3 +--
t/t1502-rev-parse-parseopt.sh | 11 +++++++++++
2 files changed, 12 insertions(+), 2 deletions(-)
diff --git a/parse-options.c b/parse-options.c
index 398ebaef14..008c0f32cf 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -363,7 +363,7 @@ static void register_abbrev(struct parse_opt_ctx_t *p,
if (p->flags & PARSE_OPT_KEEP_UNKNOWN_OPT)
return;
if (abbrev->option &&
- !is_alias(p, abbrev->option, option)) {
+ !(abbrev->flags == flags && is_alias(p, abbrev->option, option))) {
/*
* If this is abbreviated, it is
* ambiguous. So when there is no
@@ -406,7 +406,6 @@ static enum parse_opt_result parse_long_opt(
if (!strncmp(long_name, arg, arg_end - arg)) {
register_abbrev(p, options, flags ^ opt_flags,
&abbrev, &ambiguous);
- continue;
}
/* negation allowed? */
if (options->flags & PARSE_OPT_NONEG)
diff --git a/t/t1502-rev-parse-parseopt.sh b/t/t1502-rev-parse-parseopt.sh
index f0737593c3..b754b9fd74 100755
--- a/t/t1502-rev-parse-parseopt.sh
+++ b/t/t1502-rev-parse-parseopt.sh
@@ -322,4 +322,15 @@ check_invalid_long_option optionspec-neg --no-positive-only
check_invalid_long_option optionspec-neg --negative
check_invalid_long_option optionspec-neg --no-no-negative
+test_expect_success 'ambiguous: --no matches both --noble and --no-noble' '
+ cat >spec <<-\EOF &&
+ some-command [options]
+ --
+ noble The feudal switch.
+ EOF
+ test_expect_code 129 env GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS=false \
+ git rev-parse --parseopt -- <spec 2>err --no &&
+ grep "error: ambiguous option: no (could be --noble or --no-noble)" err
+'
+
test_done
--
2.44.0
^ permalink raw reply related
* [PATCH v2 0/6] parse-options: long option parsing fixes and cleanup
From: René Scharfe @ 2024-03-03 12:19 UTC (permalink / raw)
To: git
In-Reply-To: <20240224212953.44026-1-l.s.r@web.de>
Fix two corner cases in patches 1 and 4, refactor the code in patches 2
and 3 to prepare the second fix, bonus patches 5 and 6 simplify the code
and improve readability.
Changes since v1:
* Added sign-off.
parse-options: recognize abbreviated negated option with arg
parse-options: set arg of abbreviated option lazily
parse-options: factor out register_abbrev() and struct parsed_option
parse-options: detect ambiguous self-negation
parse-options: normalize arg and long_name before comparison
parse-options: rearrange long_name matching code
parse-options.c | 137 ++++++++++++++++++----------------
t/t0040-parse-options.sh | 16 ++++
t/t1502-rev-parse-parseopt.sh | 11 +++
3 files changed, 100 insertions(+), 64 deletions(-)
--
2.44.0
^ permalink raw reply
* [PATCH v2 1/6] parse-options: recognize abbreviated negated option with arg
From: René Scharfe @ 2024-03-03 12:19 UTC (permalink / raw)
To: git
In-Reply-To: <20240303121944.20627-1-l.s.r@web.de>
Giving an argument to an option that doesn't take one causes Git to
report that error specifically:
$ git rm --dry-run=bogus
error: option `dry-run' takes no value
The same is true when the option is negated or abbreviated:
$ git rm --no-dry-run=bogus
error: option `no-dry-run' takes no value
$ git rm --dry=bogus
error: option `dry-run' takes no value
Not so when doing both, though:
$ git rm --no-dry=bogus
error: unknown option `no-dry=bogus'
usage: git rm [-f | --force] [-n] [-r] [--cached] [--ignore-unmatch]
(Rest of the usage message omitted.)
Improve consistency and usefulness of the error message by recognizing
abbreviated negated options even if they have a (most likely bogus)
argument. With this patch we get:
$ git rm --no-dry=bogus
error: option `no-dry-run' takes no value
Signed-off-by: René Scharfe <l.s.r@web.de>
---
parse-options.c | 5 +++--
t/t0040-parse-options.sh | 16 ++++++++++++++++
2 files changed, 19 insertions(+), 2 deletions(-)
diff --git a/parse-options.c b/parse-options.c
index 63a99dea6e..e4ce33ea48 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -391,7 +391,7 @@ static enum parse_opt_result parse_long_opt(
ambiguous_option = abbrev_option;
ambiguous_flags = abbrev_flags;
}
- if (!(flags & OPT_UNSET) && *arg_end)
+ if (*arg_end)
p->opt = arg_end + 1;
abbrev_option = options;
abbrev_flags = flags ^ opt_flags;
@@ -412,7 +412,8 @@ static enum parse_opt_result parse_long_opt(
if (!skip_prefix(arg + 3, long_name, &rest)) {
/* abbreviated and negated? */
if (allow_abbrev &&
- starts_with(long_name, arg + 3))
+ !strncmp(long_name, arg + 3,
+ arg_end - arg - 3))
goto is_abbreviated;
else
continue;
diff --git a/t/t0040-parse-options.sh b/t/t0040-parse-options.sh
index ec974867e4..8bb2a8b453 100755
--- a/t/t0040-parse-options.sh
+++ b/t/t0040-parse-options.sh
@@ -210,6 +210,22 @@ test_expect_success 'superfluous value provided: boolean' '
test_cmp expect actual
'
+test_expect_success 'superfluous value provided: boolean, abbreviated' '
+ cat >expect <<-\EOF &&
+ error: option `yes'\'' takes no value
+ EOF
+ test_expect_code 129 env GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS=false \
+ test-tool parse-options --ye=hi 2>actual &&
+ test_cmp expect actual &&
+
+ cat >expect <<-\EOF &&
+ error: option `no-yes'\'' takes no value
+ EOF
+ test_expect_code 129 env GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS=false \
+ test-tool parse-options --no-ye=hi 2>actual &&
+ test_cmp expect actual
+'
+
test_expect_success 'superfluous value provided: cmdmode' '
cat >expect <<-\EOF &&
error: option `mode1'\'' takes no value
--
2.44.0
^ permalink raw reply related
* [PATCH v2 2/6] parse-options: set arg of abbreviated option lazily
From: René Scharfe @ 2024-03-03 12:19 UTC (permalink / raw)
To: git
In-Reply-To: <20240303121944.20627-1-l.s.r@web.de>
Postpone setting the opt pointer until we're about to call get_value(),
which uses it. There's no point in setting it eagerly for every
abbreviated candidate option, which may turn out to be ambiguous.
Removing this assignment from the loop doesn't noticeably improve the
performance, but allows further simplification.
Signed-off-by: René Scharfe <l.s.r@web.de>
---
parse-options.c | 7 ++++---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/parse-options.c b/parse-options.c
index e4ce33ea48..056c6b30e9 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -391,8 +391,6 @@ static enum parse_opt_result parse_long_opt(
ambiguous_option = abbrev_option;
ambiguous_flags = abbrev_flags;
}
- if (*arg_end)
- p->opt = arg_end + 1;
abbrev_option = options;
abbrev_flags = flags ^ opt_flags;
continue;
@@ -441,8 +439,11 @@ static enum parse_opt_result parse_long_opt(
abbrev_option->long_name);
return PARSE_OPT_HELP;
}
- if (abbrev_option)
+ if (abbrev_option) {
+ if (*arg_end)
+ p->opt = arg_end + 1;
return get_value(p, abbrev_option, abbrev_flags);
+ }
return PARSE_OPT_UNKNOWN;
}
--
2.44.0
^ permalink raw reply related
* [PATCH v2 5/6] parse-options: normalize arg and long_name before comparison
From: René Scharfe @ 2024-03-03 12:19 UTC (permalink / raw)
To: git
In-Reply-To: <20240303121944.20627-1-l.s.r@web.de>
Strip "no-" from arg and long_name before comparing them. This way we
no longer have to repeat the comparison with an offset of 3 for negated
arguments.
Note that we must not modify the "flags" value, which tracks whether arg
is negated, inside the loop. When registering "--n", "--no" or "--no-"
as abbreviation for any negative option, we used to OR it with OPT_UNSET
and end the loop. We can simply hard-code OPT_UNSET and leave flags
unchanged instead.
Signed-off-by: René Scharfe <l.s.r@web.de>
---
parse-options.c | 44 ++++++++++++++++++++++----------------------
1 file changed, 22 insertions(+), 22 deletions(-)
diff --git a/parse-options.c b/parse-options.c
index 008c0f32cf..d45efa4e5c 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -382,28 +382,42 @@ static enum parse_opt_result parse_long_opt(
const struct option *options)
{
const char *arg_end = strchrnul(arg, '=');
+ const char *arg_start = arg;
+ enum opt_parsed flags = OPT_LONG;
+ int arg_starts_with_no_no = 0;
struct parsed_option abbrev = { .option = NULL, .flags = OPT_LONG };
struct parsed_option ambiguous = { .option = NULL, .flags = OPT_LONG };
+ if (skip_prefix(arg_start, "no-", &arg_start)) {
+ if (skip_prefix(arg_start, "no-", &arg_start))
+ arg_starts_with_no_no = 1;
+ else
+ flags |= OPT_UNSET;
+ }
+
for (; options->type != OPTION_END; options++) {
const char *rest, *long_name = options->long_name;
- enum opt_parsed flags = OPT_LONG, opt_flags = OPT_LONG;
+ enum opt_parsed opt_flags = OPT_LONG;
+ int allow_unset = !(options->flags & PARSE_OPT_NONEG);
if (options->type == OPTION_SUBCOMMAND)
continue;
if (!long_name)
continue;
- if (!starts_with(arg, "no-") &&
- !(options->flags & PARSE_OPT_NONEG) &&
- skip_prefix(long_name, "no-", &long_name))
+ if (skip_prefix(long_name, "no-", &long_name))
opt_flags |= OPT_UNSET;
+ else if (arg_starts_with_no_no)
+ continue;
+
+ if (((flags ^ opt_flags) & OPT_UNSET) && !allow_unset)
+ continue;
- if (!skip_prefix(arg, long_name, &rest))
+ if (!skip_prefix(arg_start, long_name, &rest))
rest = NULL;
if (!rest) {
/* abbreviated? */
- if (!strncmp(long_name, arg, arg_end - arg)) {
+ if (!strncmp(long_name, arg_start, arg_end - arg_start)) {
register_abbrev(p, options, flags ^ opt_flags,
&abbrev, &ambiguous);
}
@@ -412,24 +426,10 @@ static enum parse_opt_result parse_long_opt(
continue;
/* negated and abbreviated very much? */
if (starts_with("no-", arg)) {
- flags |= OPT_UNSET;
- register_abbrev(p, options, flags ^ opt_flags,
+ register_abbrev(p, options, OPT_UNSET ^ opt_flags,
&abbrev, &ambiguous);
- continue;
- }
- /* negated? */
- if (!starts_with(arg, "no-"))
- continue;
- flags |= OPT_UNSET;
- if (!skip_prefix(arg + 3, long_name, &rest)) {
- /* abbreviated and negated? */
- if (!strncmp(long_name, arg + 3,
- arg_end - arg - 3))
- register_abbrev(p, options,
- flags ^ opt_flags,
- &abbrev, &ambiguous);
- continue;
}
+ continue;
}
if (*rest) {
if (*rest != '=')
--
2.44.0
^ permalink raw reply related
* [PATCH v2 6/6] parse-options: rearrange long_name matching code
From: René Scharfe @ 2024-03-03 12:19 UTC (permalink / raw)
To: git
In-Reply-To: <20240303121944.20627-1-l.s.r@web.de>
Move the code for handling a full match of long_name first and get rid
of negations. Reduce the indent of the code for matching abbreviations
and remove unnecessary curly braces. Combine the checks for whether
negation is allowed and whether arg is "n", "no" or "no-" because they
belong together and avoid a continue statement. The result is shorter,
more readable code.
Signed-off-by: René Scharfe <l.s.r@web.de>
---
parse-options.c | 37 +++++++++++++++----------------------
1 file changed, 15 insertions(+), 22 deletions(-)
diff --git a/parse-options.c b/parse-options.c
index d45efa4e5c..30b9e68f8a 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -413,30 +413,23 @@ static enum parse_opt_result parse_long_opt(
if (((flags ^ opt_flags) & OPT_UNSET) && !allow_unset)
continue;
- if (!skip_prefix(arg_start, long_name, &rest))
- rest = NULL;
- if (!rest) {
- /* abbreviated? */
- if (!strncmp(long_name, arg_start, arg_end - arg_start)) {
- register_abbrev(p, options, flags ^ opt_flags,
- &abbrev, &ambiguous);
- }
- /* negation allowed? */
- if (options->flags & PARSE_OPT_NONEG)
+ if (skip_prefix(arg_start, long_name, &rest)) {
+ if (*rest == '=')
+ p->opt = rest + 1;
+ else if (*rest)
continue;
- /* negated and abbreviated very much? */
- if (starts_with("no-", arg)) {
- register_abbrev(p, options, OPT_UNSET ^ opt_flags,
- &abbrev, &ambiguous);
- }
- continue;
+ return get_value(p, options, flags ^ opt_flags);
}
- if (*rest) {
- if (*rest != '=')
- continue;
- p->opt = rest + 1;
- }
- return get_value(p, options, flags ^ opt_flags);
+
+ /* abbreviated? */
+ if (!strncmp(long_name, arg_start, arg_end - arg_start))
+ register_abbrev(p, options, flags ^ opt_flags,
+ &abbrev, &ambiguous);
+
+ /* negated and abbreviated very much? */
+ if (allow_unset && starts_with("no-", arg))
+ register_abbrev(p, options, OPT_UNSET ^ opt_flags,
+ &abbrev, &ambiguous);
}
if (disallow_abbreviated_options && (ambiguous.option || abbrev.option))
--
2.44.0
^ permalink raw reply related
* [PATCH v2 1/4] t-ctype: allow NUL anywhere in the specification string
From: René Scharfe @ 2024-03-03 10:13 UTC (permalink / raw)
To: git; +Cc: Phillip Wood, Josh Steadmon, Achu Luma, Christian Couder
In-Reply-To: <20240303101330.20187-1-l.s.r@web.de>
Replace the custom function is_in() for looking up a character in the
specification string with memchr(3) and sizeof. This is shorter,
simpler and allows NUL anywhere in the string, which may come in handy
if we ever want to support more character classes that contain it.
Getting the string size using sizeof only works in a macro and with a
string constant. Use ARRAY_SIZE and compile-time checks to make sure we
are not passed a string pointer.
Signed-off-by: René Scharfe <l.s.r@web.de>
---
t/unit-tests/t-ctype.c | 18 ++++--------------
1 file changed, 4 insertions(+), 14 deletions(-)
diff --git a/t/unit-tests/t-ctype.c b/t/unit-tests/t-ctype.c
index f315489984..35473c41d8 100644
--- a/t/unit-tests/t-ctype.c
+++ b/t/unit-tests/t-ctype.c
@@ -1,23 +1,13 @@
#include "test-lib.h"
-static int is_in(const char *s, int ch)
-{
- /*
- * We can't find NUL using strchr. Accept it as the first
- * character in the spec -- there are no empty classes.
- */
- if (ch == '\0')
- return ch == *s;
- if (*s == '\0')
- s++;
- return !!strchr(s, ch);
-}
-
/* Macro to test a character type */
#define TEST_CTYPE_FUNC(func, string) \
static void test_ctype_##func(void) { \
+ size_t len = ARRAY_SIZE(string) - 1 + \
+ BUILD_ASSERT_OR_ZERO(ARRAY_SIZE(string) > 0) + \
+ BUILD_ASSERT_OR_ZERO(sizeof(string[0]) == sizeof(char)); \
for (int i = 0; i < 256; i++) { \
- if (!check_int(func(i), ==, is_in(string, i))) \
+ if (!check_int(func(i), ==, !!memchr(string, i, len))) \
test_msg(" i: 0x%02x", i); \
} \
if (!check(!func(EOF))) \
--
2.44.0
^ permalink raw reply related
* [PATCH v2 0/4] t-ctype: simplify unit test definitions
From: René Scharfe @ 2024-03-03 10:13 UTC (permalink / raw)
To: git; +Cc: Phillip Wood, Josh Steadmon, Achu Luma, Christian Couder
In-Reply-To: <20240225112722.89221-1-l.s.r@web.de>
Simplify the ctype unit tests to allow combining specification strings
in any order and no longer require repeating class names.
Changes since v1:
* Added checks to guard string length calculation using sizeof.
* Kept the definition string in the error output.
* Added patches 2 and 3 with further cosmetic changes.
* Dropped the last patch with the huge output for now, as it needs
further thought.
t-ctype: allow NUL anywhere in the specification string
t-ctype: simplify EOF check
t-ctype: align output of i
t-ctype: avoid duplicating class names
t/unit-tests/t-ctype.c | 81 ++++++++++++++----------------------------
1 file changed, 27 insertions(+), 54 deletions(-)
Range-Diff gegen v1:
1: ae9b5468db ! 1: 28fba8bda9 t-ctype: allow NUL anywhere in the specification string
@@ Commit message
if we ever want to support more character classes that contain it.
Getting the string size using sizeof only works in a macro and with a
- string constant, but that's exactly what we have and I don't see it
- changing anytime soon.
+ string constant. Use ARRAY_SIZE and compile-time checks to make sure we
+ are not passed a string pointer.
## t/unit-tests/t-ctype.c ##
@@
@@ t/unit-tests/t-ctype.c
/* Macro to test a character type */
#define TEST_CTYPE_FUNC(func, string) \
static void test_ctype_##func(void) { \
++ size_t len = ARRAY_SIZE(string) - 1 + \
++ BUILD_ASSERT_OR_ZERO(ARRAY_SIZE(string) > 0) + \
++ BUILD_ASSERT_OR_ZERO(sizeof(string[0]) == sizeof(char)); \
for (int i = 0; i < 256; i++) { \
- if (!check_int(func(i), ==, is_in(string, i))) \
-+ int expect = !!memchr(string, i, sizeof(string) - 1); \
-+ if (!check_int(func(i), ==, expect)) \
++ if (!check_int(func(i), ==, !!memchr(string, i, len))) \
test_msg(" i: 0x%02x", i); \
} \
if (!check(!func(EOF))) \
-: ---------- > 2: 1eab0d289c t-ctype: simplify EOF check
-: ---------- > 3: 901312e980 t-ctype: align output of i
2: 688e6b6cb5 ! 4: 5dac51cfeb t-ctype: avoid duplicating class names
@@ t/unit-tests/t-ctype.c
-/* Macro to test a character type */
-#define TEST_CTYPE_FUNC(func, string) \
-static void test_ctype_##func(void) { \
-- for (int i = 0; i < 256; i++) { \
-- int expect = !!memchr(string, i, sizeof(string) - 1); \
-- if (!check_int(func(i), ==, expect)) \
-- test_msg(" i: 0x%02x", i); \
-- } \
-- if (!check(!func(EOF))) \
+#define TEST_CHAR_CLASS(class, string) do { \
+ size_t len = ARRAY_SIZE(string) - 1 + \
+ BUILD_ASSERT_OR_ZERO(ARRAY_SIZE(string) > 0) + \
+ BUILD_ASSERT_OR_ZERO(sizeof(string[0]) == sizeof(char)); \
+- for (int i = 0; i < 256; i++) { \
+- if (!check_int(func(i), ==, !!memchr(string, i, len))) \
+- test_msg(" i: 0x%02x", i); \
+ int skip = test__run_begin(); \
+ if (!skip) { \
+ for (int i = 0; i < 256; i++) { \
-+ int expect = !!memchr(string, i, sizeof(string) - 1); \
-+ if (!check_int(class(i), ==, expect)) \
-+ test_msg(" i: 0x%02x", i); \
++ if (!check_int(class(i), ==, !!memchr(string, i, len)))\
++ test_msg(" i: 0x%02x", i); \
+ } \
-+ if (!check(!class(EOF))) \
- test_msg(" i: 0x%02x (EOF)", EOF); \
++ check(!class(EOF)); \
+ } \
+- check(!func(EOF)); \
-}
-
-#define TEST_CHAR_CLASS(class) TEST(test_ctype_##class(), #class " works")
-+ } \
+ test__run_end(!skip, TEST_LOCATION(), #class " works"); \
+} while (0)
3: caf2acbd09 < -: ---------- t-ctype: do one test per class and char
--
2.44.0
^ permalink raw reply
* [PATCH v2 4/4] t-ctype: avoid duplicating class names
From: René Scharfe @ 2024-03-03 10:13 UTC (permalink / raw)
To: git; +Cc: Phillip Wood, Josh Steadmon, Achu Luma, Christian Couder
In-Reply-To: <20240303101330.20187-1-l.s.r@web.de>
TEST_CTYPE_FUNC defines a function for testing a character classifier,
TEST_CHAR_CLASS calls it, causing the class name to be mentioned twice.
Avoid the need to define a class-specific function by letting
TEST_CHAR_CLASS do all the work. This is done by using the internal
functions test__run_begin() and test__run_end(), but they do exist to be
used in test macros after all.
Alternatively we could unroll the loop to provide a very long expression
that tests all 256 characters and EOF and hand that to TEST, but that
seems awkward and hard to read.
No change of behavior or output intended.
Signed-off-by: René Scharfe <l.s.r@web.de>
---
t/unit-tests/t-ctype.c | 64 ++++++++++++++++--------------------------
1 file changed, 24 insertions(+), 40 deletions(-)
diff --git a/t/unit-tests/t-ctype.c b/t/unit-tests/t-ctype.c
index 02d8569aa3..d6ac1fe678 100644
--- a/t/unit-tests/t-ctype.c
+++ b/t/unit-tests/t-ctype.c
@@ -1,19 +1,19 @@
#include "test-lib.h"
-/* Macro to test a character type */
-#define TEST_CTYPE_FUNC(func, string) \
-static void test_ctype_##func(void) { \
+#define TEST_CHAR_CLASS(class, string) do { \
size_t len = ARRAY_SIZE(string) - 1 + \
BUILD_ASSERT_OR_ZERO(ARRAY_SIZE(string) > 0) + \
BUILD_ASSERT_OR_ZERO(sizeof(string[0]) == sizeof(char)); \
- for (int i = 0; i < 256; i++) { \
- if (!check_int(func(i), ==, !!memchr(string, i, len))) \
- test_msg(" i: 0x%02x", i); \
+ int skip = test__run_begin(); \
+ if (!skip) { \
+ for (int i = 0; i < 256; i++) { \
+ if (!check_int(class(i), ==, !!memchr(string, i, len)))\
+ test_msg(" i: 0x%02x", i); \
+ } \
+ check(!class(EOF)); \
} \
- check(!func(EOF)); \
-}
-
-#define TEST_CHAR_CLASS(class) TEST(test_ctype_##class(), #class " works")
+ test__run_end(!skip, TEST_LOCATION(), #class " works"); \
+} while (0)
#define DIGIT "0123456789"
#define LOWER "abcdefghijklmnopqrstuvwxyz"
@@ -33,37 +33,21 @@ static void test_ctype_##func(void) { \
"\x10\x11\x12\x13\x14\x15\x16\x17\x18\x19\x1a\x1b\x1c\x1d\x1e\x1f" \
"\x7f"
-TEST_CTYPE_FUNC(isdigit, DIGIT)
-TEST_CTYPE_FUNC(isspace, " \n\r\t")
-TEST_CTYPE_FUNC(isalpha, LOWER UPPER)
-TEST_CTYPE_FUNC(isalnum, LOWER UPPER DIGIT)
-TEST_CTYPE_FUNC(is_glob_special, "*?[\\")
-TEST_CTYPE_FUNC(is_regex_special, "$()*+.?[\\^{|")
-TEST_CTYPE_FUNC(is_pathspec_magic, "!\"#%&',-/:;<=>@_`~")
-TEST_CTYPE_FUNC(isascii, ASCII)
-TEST_CTYPE_FUNC(islower, LOWER)
-TEST_CTYPE_FUNC(isupper, UPPER)
-TEST_CTYPE_FUNC(iscntrl, CNTRL)
-TEST_CTYPE_FUNC(ispunct, PUNCT)
-TEST_CTYPE_FUNC(isxdigit, DIGIT "abcdefABCDEF")
-TEST_CTYPE_FUNC(isprint, LOWER UPPER DIGIT PUNCT " ")
-
int cmd_main(int argc, const char **argv) {
- /* Run all character type tests */
- TEST_CHAR_CLASS(isspace);
- TEST_CHAR_CLASS(isdigit);
- TEST_CHAR_CLASS(isalpha);
- TEST_CHAR_CLASS(isalnum);
- TEST_CHAR_CLASS(is_glob_special);
- TEST_CHAR_CLASS(is_regex_special);
- TEST_CHAR_CLASS(is_pathspec_magic);
- TEST_CHAR_CLASS(isascii);
- TEST_CHAR_CLASS(islower);
- TEST_CHAR_CLASS(isupper);
- TEST_CHAR_CLASS(iscntrl);
- TEST_CHAR_CLASS(ispunct);
- TEST_CHAR_CLASS(isxdigit);
- TEST_CHAR_CLASS(isprint);
+ TEST_CHAR_CLASS(isspace, " \n\r\t");
+ TEST_CHAR_CLASS(isdigit, DIGIT);
+ TEST_CHAR_CLASS(isalpha, LOWER UPPER);
+ TEST_CHAR_CLASS(isalnum, LOWER UPPER DIGIT);
+ TEST_CHAR_CLASS(is_glob_special, "*?[\\");
+ TEST_CHAR_CLASS(is_regex_special, "$()*+.?[\\^{|");
+ TEST_CHAR_CLASS(is_pathspec_magic, "!\"#%&',-/:;<=>@_`~");
+ TEST_CHAR_CLASS(isascii, ASCII);
+ TEST_CHAR_CLASS(islower, LOWER);
+ TEST_CHAR_CLASS(isupper, UPPER);
+ TEST_CHAR_CLASS(iscntrl, CNTRL);
+ TEST_CHAR_CLASS(ispunct, PUNCT);
+ TEST_CHAR_CLASS(isxdigit, DIGIT "abcdefABCDEF");
+ TEST_CHAR_CLASS(isprint, LOWER UPPER DIGIT PUNCT " ");
return test_done();
}
--
2.44.0
^ permalink raw reply related
* [PATCH v2 3/4] t-ctype: align output of i
From: René Scharfe @ 2024-03-03 10:13 UTC (permalink / raw)
To: git; +Cc: Phillip Wood, Josh Steadmon, Achu Luma, Christian Couder
In-Reply-To: <20240303101330.20187-1-l.s.r@web.de>
The unit test reports misclassified characters like this:
# check "isdigit(i) == !!memchr("123456789", i, len)" failed at t/unit-tests/t-ctype.c:36
# left: 1
# right: 0
# i: 0x30
Reduce the indent of i to put its colon directly below the ones in the
preceding lines for consistency.
Signed-off-by: René Scharfe <l.s.r@web.de>
---
t/unit-tests/t-ctype.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/t/unit-tests/t-ctype.c b/t/unit-tests/t-ctype.c
index f0d61d6eb2..02d8569aa3 100644
--- a/t/unit-tests/t-ctype.c
+++ b/t/unit-tests/t-ctype.c
@@ -8,7 +8,7 @@ static void test_ctype_##func(void) { \
BUILD_ASSERT_OR_ZERO(sizeof(string[0]) == sizeof(char)); \
for (int i = 0; i < 256; i++) { \
if (!check_int(func(i), ==, !!memchr(string, i, len))) \
- test_msg(" i: 0x%02x", i); \
+ test_msg(" i: 0x%02x", i); \
} \
check(!func(EOF)); \
}
--
2.44.0
^ permalink raw reply related
* [PATCH v2 2/4] t-ctype: simplify EOF check
From: René Scharfe @ 2024-03-03 10:13 UTC (permalink / raw)
To: git; +Cc: Phillip Wood, Josh Steadmon, Achu Luma, Christian Couder
In-Reply-To: <20240303101330.20187-1-l.s.r@web.de>
EOF is not a member of any character class. If a classifier function
returns a non-zero result for it, presumably by mistake, then the unit
test check reports:
# check "!iseof(EOF)" failed at t/unit-tests/t-ctype.c:53
# i: 0xffffffff (EOF)
The numeric value of EOF is not particularly interesting in this
context. Stop printing the second line.
Signed-off-by: René Scharfe <l.s.r@web.de>
---
t/unit-tests/t-ctype.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/t/unit-tests/t-ctype.c b/t/unit-tests/t-ctype.c
index 35473c41d8..f0d61d6eb2 100644
--- a/t/unit-tests/t-ctype.c
+++ b/t/unit-tests/t-ctype.c
@@ -10,8 +10,7 @@ static void test_ctype_##func(void) { \
if (!check_int(func(i), ==, !!memchr(string, i, len))) \
test_msg(" i: 0x%02x", i); \
} \
- if (!check(!func(EOF))) \
- test_msg(" i: 0x%02x (EOF)", EOF); \
+ check(!func(EOF)); \
}
#define TEST_CHAR_CLASS(class) TEST(test_ctype_##class(), #class " works")
--
2.44.0
^ permalink raw reply related
* Re: [PATCH] clean: improve -n and -f implementation and documentation
From: Sergey Organov @ 2024-03-03 9:54 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jean-Noël AVILA, git
In-Reply-To: <87bk7w8aae.fsf@osv.gnss.ru>
Sergey Organov <sorganov@gmail.com> writes:
> Junio C Hamano <gitster@pobox.com> writes:
>
>> Sergey Organov <sorganov@gmail.com> writes:
>>
>>>> Oh, sorry, I misinterpreted the patch. But yet, I'm not sure that
>>>> specifying that this is the default or not is really useful. If the
>>>> configuration was set to true, it is was a no-op. If set to false, no
>>>> message will appear.
>>>
>>> I'm not sure either, and as it's not the topic of this particular patch,
>>> I'd like to delegate the decision on the issue.
>>
>> It is very much spot on the topic of simplifying and clarifying the
>> code to unify these remaining two messages into a single one.
>
> I'm inclined to be more against merging than for it, as for me it'd be
> confusing to be told that a configuration variable is set to true when I
> didn't set it, nor there is any way to figure where it is set, because
> in fact it isn't, and it's rather the default that is in use.
>
> Overall, to me the messages are fine as they are (except -n that doesn't
> belong there), I don't see compelling reason to hide information from
> the user, and thus I won't propose patch that gets rid of one of them.
Nevertheless, as others are in favor of unification, I've merged these
two messages in the v2 version of the patch, which see.
Thanks,
-- Sergey Organov
^ permalink raw reply
* [PATCH v2] clean: improve -n and -f implementation and documentation
From: Sergey Organov @ 2024-03-03 9:50 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Jean-Noël AVILA, Kristoffer Haugsbakk
In-Reply-To: <875xy76qe1.fsf@osv.gnss.ru>
What -n actually does in addition to its documented behavior is
ignoring of configuration variable clean.requireForce, that makes
sense provided -n prevents files removal anyway.
So, first, document this in the manual, and then modify implementation
to make this more explicit in the code.
Improved implementation also stops to share single internal variable
'force' between command-line -f option and configuration variable
clean.requireForce, resulting in more clear logic.
Two error messages with slightly different text depending on if
clean.requireForce was explicitly set or not, are merged into a single
one.
The resulting error message now does not mention -n as well, as it
neither matches intended clean.requireForce usage nor reflects
clarified implementation.
Documentation of clean.requireForce is changed accordingly.
Signed-off-by: Sergey Organov <sorganov@gmail.com>
---
Changes since v1:
* Fixed style of the if() statement
* Merged two error messages into one
* clean.requireForce description changed accordingly
Documentation/config/clean.txt | 4 ++--
Documentation/git-clean.txt | 2 ++
builtin/clean.c | 23 +++++++++--------------
3 files changed, 13 insertions(+), 16 deletions(-)
diff --git a/Documentation/config/clean.txt b/Documentation/config/clean.txt
index f05b9403b5ad..b19ca210f39b 100644
--- a/Documentation/config/clean.txt
+++ b/Documentation/config/clean.txt
@@ -1,3 +1,3 @@
clean.requireForce::
- A boolean to make git-clean do nothing unless given -f,
- -i, or -n. Defaults to true.
+ A boolean to make git-clean refuse to delete files unless -f
+ or -i is given. Defaults to true.
diff --git a/Documentation/git-clean.txt b/Documentation/git-clean.txt
index 69331e3f05a1..662eebb85207 100644
--- a/Documentation/git-clean.txt
+++ b/Documentation/git-clean.txt
@@ -49,6 +49,8 @@ OPTIONS
-n::
--dry-run::
Don't actually remove anything, just show what would be done.
+ Configuration variable clean.requireForce is ignored, as
+ nothing will be deleted anyway.
-q::
--quiet::
diff --git a/builtin/clean.c b/builtin/clean.c
index d90766cad3a0..41502dcb0dde 100644
--- a/builtin/clean.c
+++ b/builtin/clean.c
@@ -25,7 +25,7 @@
#include "help.h"
#include "prompt.h"
-static int force = -1; /* unset */
+static int require_force = -1; /* unset */
static int interactive;
static struct string_list del_list = STRING_LIST_INIT_DUP;
static unsigned int colopts;
@@ -128,7 +128,7 @@ static int git_clean_config(const char *var, const char *value,
}
if (!strcmp(var, "clean.requireforce")) {
- force = !git_config_bool(var, value);
+ require_force = git_config_bool(var, value);
return 0;
}
@@ -920,7 +920,7 @@ int cmd_clean(int argc, const char **argv, const char *prefix)
{
int i, res;
int dry_run = 0, remove_directories = 0, quiet = 0, ignored = 0;
- int ignored_only = 0, config_set = 0, errors = 0, gone = 1;
+ int ignored_only = 0, force = 0, errors = 0, gone = 1;
int rm_flags = REMOVE_DIR_KEEP_NESTED_GIT;
struct strbuf abs_path = STRBUF_INIT;
struct dir_struct dir = DIR_INIT;
@@ -946,22 +946,17 @@ int cmd_clean(int argc, const char **argv, const char *prefix)
};
git_config(git_clean_config, NULL);
- if (force < 0)
- force = 0;
- else
- config_set = 1;
argc = parse_options(argc, argv, prefix, options, builtin_clean_usage,
0);
- if (!interactive && !dry_run && !force) {
- if (config_set)
- die(_("clean.requireForce set to true and neither -i, -n, nor -f given; "
- "refusing to clean"));
- else
- die(_("clean.requireForce defaults to true and neither -i, -n, nor -f given;"
+ /* Dry run won't remove anything, so requiring force makes no sense */
+ if (dry_run)
+ require_force = 0;
+
+ if (require_force != 0 && !force && !interactive)
+ die(_("clean.requireForce is true and neither -f nor -i given:"
" refusing to clean"));
- }
if (force > 1)
rm_flags = 0;
base-commit: 0f9d4d28b7e6021b7e6db192b7bf47bd3a0d0d1d
--
2.25.1
^ permalink raw reply related
* Re: [PATCH 0/2] Support diff.wordDiff config
From: Chris Torek @ 2024-03-03 7:23 UTC (permalink / raw)
To: Kristoffer Haugsbakk; +Cc: Karthik Nayak, oliver, git, Junio C Hamano
In-Reply-To: <34bb249d-4a4d-4cc7-b737-bb18398341d0@app.fastmail.com>
Continuing the digression a bit:
On Sat, Mar 2, 2024 at 11:58 AM Kristoffer Haugsbakk
<code@khaugsbakk.name> wrote:
> This looks similar to the discussion from a [stash] topic:
>
> • Proposed introducing config variables which change how `git stash
> push` and `git stash save` behave (what they save)
> • Concern about how that could break third-party scripts
[snippage]
> 🔗 [stash]: https://lore.kernel.org/git/xmqq34tnyhhf.fsf@gitster.g/
As I see it, the general issue here is the tension between Git
commands that are used for scripting -- which ideally should
always be plumbing commands -- and those used by end-users.
This tension is relieved somewhat when there *are* separate
plumbing commands, such as `git diff-index` and `git diff-tree`
and so on, or `git rev-list` vs `git log`. Unfortunately there
are some commands, including `git log` itself, that have options
that are missing from the roughly-equivalent plumbing command,
and there are commands (such as `git stash` and `git status`)
that either do not have, or at one time lacked, plumbing command
equivalents or options.
The `git status` command shows one way out of this problem:
we can *add* `--porcelain` options. Perhaps every command (or
every non-plumbing-only one) should have `--porcelain[=<version>]`.
This doesn't fix the situation today, but provides an obvious
future-proofing path.
Chris
^ permalink raw reply
* Re: [PATCH] test-lib-functions: simplify `test_file_not_empty` failure message
From: Dirk Gouders @ 2024-03-03 6:42 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Eric Sunshine, Eric Sunshine, git, Aryan Gupta
In-Reply-To: <xmqqbk7wy1di.fsf@gitster.g>
Junio C Hamano <gitster@pobox.com> writes:
> Dirk Gouders <dirk@gouders.net> writes:
>
>> Eric Sunshine <sunshine@sunshineco.com> writes:
>>
>>> On Fri, Mar 1, 2024 at 5:11 PM Junio C Hamano <gitster@pobox.com> wrote:
>>>> Eric Sunshine <ericsunshine@charter.net> writes:
>>>> > A more accurate message might be "'foo' is empty but
>>>> > should not be (or doesn't exist)", but that's unnecessarily long-winded
>>>> > and adds little information that the test author couldn't discover by
>>>> > noticing the file's absence.
>>>>
>>>> The "adds little information" version may be
>>>>
>>>> echo "'$1' is either missing or empty, but should not be"
>>>> ...
>>> I find "'$1' is either missing or empty, but should not be" suggestion
>>> clear and easily understood. I'll reroll with that.
>>
>> This is a view from a position with more distance:
>>
>> I find that not so easily understood -- the "but should not
>> be" part is rather unexpected and I feel, it doesn't provide necessary
>> information, e.g.:
>>
>> test_path_is_executable () {
>> ...
>> echo "$1 is not executable"
>> ...
>>
>> also doesn't state what is wanted and I doubt that message doesn't
>> clearly describe the problem.
>
> I cannot tell if you really meant the double negative involving
> "doubt", but assuming you did, you are saying that
I'm sorry about that double negative which was probably wrong wording of
a non-native speaker.
> With "X is not Y", it is clear enough that we expect X to be Y
> (if it were not clear to somebody who read "X is not Y" that we
> want X to be Y, then "X is not Y, but it should be" may needed,
> but "X is not Y" is clear enough).
>
> So you think "$1 is either missing or empty" is better without "but
> should not be" added to the end? Am I reading you correctly?
>
> I think this takes us back pretty much to square one ;-) but that is
> also fine.
>
> But the above argument depends on an untold assumption. The message
> "X is not Y" must be clearly understood as a complaint, not a mere
> statement of a fact. I am not sure if that is the case.
>
> Instead of "X is not Y, but it should be", the way to clarify these
> messages may be to say "error: X is not Y", perhaps?
That is exactly what came to my mind when I was later re-thinking
what I had written.
>> While I looked at it: there is another `test -s` in test_grep () that
>> perhaps could be fixed the same way:
>>
>> if test -s "$last_arg"
>> then
>> cat >&4 "$last_arg"
>> else
>> echo >&4 "<File '$last_arg' is empty>"
>> fi
>
> If you are worried about "test -s" failing because "$last_arg" does
> not exist, then you are worried too much. We upfront guard the
> test_grep() helper with "test -f" of the same file and diagnoses the
> lack of the file as a bug in the test. And we do not assume gremlins
> removing random files while we are running tests.
Yes, thank you for clarification and sorry for the noise.
Dirk
^ permalink raw reply
* Re: [PATCH] clean: improve -n and -f implementation and documentation
From: Sergey Organov @ 2024-03-02 23:48 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jean-Noël AVILA, git
In-Reply-To: <xmqqwmqkwdef.fsf@gitster.g>
Junio C Hamano <gitster@pobox.com> writes:
> Sergey Organov <sorganov@gmail.com> writes:
>
>>> Oh, sorry, I misinterpreted the patch. But yet, I'm not sure that
>>> specifying that this is the default or not is really useful. If the
>>> configuration was set to true, it is was a no-op. If set to false, no
>>> message will appear.
>>
>> I'm not sure either, and as it's not the topic of this particular patch,
>> I'd like to delegate the decision on the issue.
>
> It is very much spot on the topic of simplifying and clarifying the
> code to unify these remaining two messages into a single one.
I'm inclined to be more against merging than for it, as for me it'd be
confusing to be told that a configuration variable is set to true when I
didn't set it, nor there is any way to figure where it is set, because
in fact it isn't, and it's rather the default that is in use.
Overall, to me the messages are fine as they are (except -n that doesn't
belong there), I don't see compelling reason to hide information from
the user, and thus I won't propose patch that gets rid of one of them.
> And involving the --interactive that allows users a chance to
> rethink and refrain from removing some to the equation would also be
> worth doing in the same topic,
Worth doing what? I'm afraid I lost the plot here, as --interactive
still looks fine to me.
> even though it might not fit your immediate agenda of crusade against
> --dry-run.
I'm hopefully crusading for --dry-run, not against, trying to get rid of
the cause of the original confusion that started -n/-f controversy.
Thanks,
-- Sergey Organov
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox