* [PATCH v4 6/9] rebase: clarify the OPT_CMDMODE incompatibilities
From: Elijah Newren via GitGitGadget @ 2023-01-22 6:12 UTC (permalink / raw)
To: git
Cc: Derrick Stolee, Elijah Newren, Eric Sunshine, Martin Ågren,
Phillip Wood, Elijah Newren, Elijah Newren
In-Reply-To: <pull.1466.v4.git.1674367961.gitgitgadget@gmail.com>
From: Elijah Newren <newren@gmail.com>
--edit-todo was documented as being incompatible with any of the options
for the apply backend. However, it is also incompatible with any of the
options for the merge backend, and is incompatible with any options that
are not backend specific as well. The same can be said for --continue,
--skip, --abort, --quit, etc.
This is already somewhat implicitly covered by the synopsis, but since
"[<options>]" in the first two variants are vague it might be easy to
miss this. That might not be a big deal, but since the rebase manpage
has to spend so much verbiage about incompatibility of options, making
a separate section for these options that are incompatible with
everything else seems clearer. Do that, and remove the needless
inclusion of --edit-todo in the explicit incompatibility list.
Signed-off-by: Elijah Newren <newren@gmail.com>
---
Documentation/git-rebase.txt | 61 +++++++++++++++++++-----------------
1 file changed, 33 insertions(+), 28 deletions(-)
diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
index 8cb075b2bdb..1ba691e4c5f 100644
--- a/Documentation/git-rebase.txt
+++ b/Documentation/git-rebase.txt
@@ -208,6 +208,39 @@ Alternatively, you can undo the 'git rebase' with
git rebase --abort
+MODE OPTIONS
+------------
+
+The options in this section cannot be used with any other option,
+including not with each other:
+
+--continue::
+ Restart the rebasing process after having resolved a merge conflict.
+
+--skip::
+ Restart the rebasing process by skipping the current patch.
+
+--abort::
+ Abort the rebase operation and reset HEAD to the original
+ branch. If `<branch>` was provided when the rebase operation was
+ started, then `HEAD` will be reset to `<branch>`. Otherwise `HEAD`
+ will be reset to where it was when the rebase operation was
+ started.
+
+--quit::
+ Abort the rebase operation but `HEAD` is not reset back to the
+ original branch. The index and working tree are also left
+ unchanged as a result. If a temporary stash entry was created
+ using `--autostash`, it will be saved to the stash list.
+
+--edit-todo::
+ Edit the todo list during an interactive rebase.
+
+--show-current-patch::
+ Show the current patch in an interactive rebase or when rebase
+ is stopped because of conflicts. This is the equivalent of
+ `git show REBASE_HEAD`.
+
OPTIONS
-------
--onto <newbase>::
@@ -249,22 +282,6 @@ See also INCOMPATIBLE OPTIONS below.
<branch>::
Working branch; defaults to `HEAD`.
---continue::
- Restart the rebasing process after having resolved a merge conflict.
-
---abort::
- Abort the rebase operation and reset HEAD to the original
- branch. If `<branch>` was provided when the rebase operation was
- started, then `HEAD` will be reset to `<branch>`. Otherwise `HEAD`
- will be reset to where it was when the rebase operation was
- started.
-
---quit::
- Abort the rebase operation but `HEAD` is not reset back to the
- original branch. The index and working tree are also left
- unchanged as a result. If a temporary stash entry was created
- using `--autostash`, it will be saved to the stash list.
-
--apply::
Use applying strategies to rebase (calling `git-am`
internally). This option may become a no-op in the future
@@ -345,17 +362,6 @@ See also INCOMPATIBLE OPTIONS below.
+
See also INCOMPATIBLE OPTIONS below.
---skip::
- Restart the rebasing process by skipping the current patch.
-
---edit-todo::
- Edit the todo list during an interactive rebase.
-
---show-current-patch::
- Show the current patch in an interactive rebase or when rebase
- is stopped because of conflicts. This is the equivalent of
- `git show REBASE_HEAD`.
-
-m::
--merge::
Using merging strategies to rebase (default).
@@ -651,7 +657,6 @@ are incompatible with the following options:
* --no-keep-empty
* --empty=
* --[no-]reapply-cherry-picks
- * --edit-todo
* --update-refs
* --root when used without --onto
--
gitgitgadget
^ permalink raw reply related
* [PATCH v4 7/9] rebase: fix formatting of rebase --reapply-cherry-picks option in docs
From: Elijah Newren via GitGitGadget @ 2023-01-22 6:12 UTC (permalink / raw)
To: git
Cc: Derrick Stolee, Elijah Newren, Eric Sunshine, Martin Ågren,
Phillip Wood, Elijah Newren, Elijah Newren
In-Reply-To: <pull.1466.v4.git.1674367961.gitgitgadget@gmail.com>
From: Elijah Newren <newren@gmail.com>
Commit ce5238a690 ("rebase --keep-base: imply --reapply-cherry-picks",
2022-10-17) accidentally added some blank lines that cause extra
paragraphs about --reapply-cherry-picks to be considered not part of
the documentation of that option. Remove the blank lines to make it
clear we are still discussing --reapply-cherry-picks.
Signed-off-by: Elijah Newren <newren@gmail.com>
---
Documentation/git-rebase.txt | 2 --
1 file changed, 2 deletions(-)
diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
index 1ba691e4c5f..9f794f5bdcc 100644
--- a/Documentation/git-rebase.txt
+++ b/Documentation/git-rebase.txt
@@ -338,7 +338,6 @@ See also INCOMPATIBLE OPTIONS below.
upstream changes, the behavior towards them is controlled by
the `--empty` flag.)
+
-
In the absence of `--keep-base` (or if `--no-reapply-cherry-picks` is
given), these commits will be automatically dropped. Because this
necessitates reading all upstream commits, this can be expensive in
@@ -347,7 +346,6 @@ read. When using the 'merge' backend, warnings will be issued for each
dropped commit (unless `--quiet` is given). Advice will also be issued
unless `advice.skippedCherryPicks` is set to false (see
linkgit:git-config[1]).
-
+
`--reapply-cherry-picks` allows rebase to forgo reading all upstream
commits, potentially improving performance.
--
gitgitgadget
^ permalink raw reply related
* [PATCH v4 4/9] rebase: fix docs about incompatibilities with --root
From: Elijah Newren via GitGitGadget @ 2023-01-22 6:12 UTC (permalink / raw)
To: git
Cc: Derrick Stolee, Elijah Newren, Eric Sunshine, Martin Ågren,
Phillip Wood, Elijah Newren, Elijah Newren
In-Reply-To: <pull.1466.v4.git.1674367961.gitgitgadget@gmail.com>
From: Elijah Newren <newren@gmail.com>
In commit 5dacd4abdd ("git-rebase.txt: document incompatible options",
2018-06-25), I added notes about incompatibilities between options for
the apply and merge backends. Unfortunately, I inverted the condition
when --root was incompatible with the apply backend. Fix the
documentation, and add a testcase that verifies the documentation
matches the code.
While at it, the documentation for --root also tried to cover some of
the backend differences between the apply and merge backends in relation
to reapplying cherry picks. The information:
* assumed that the apply backend was the default (it isn't anymore)
* was written before --reapply-cherry-picks became an option
* was written before the detailed information on backend differences
All of these factors make the sentence under --root about reapplying
cherry picks contradict information that is now available elsewhere in
the manual, and the other references are correct. So just strike this
sentence.
Signed-off-by: Elijah Newren <newren@gmail.com>
---
Documentation/git-rebase.txt | 7 ++-----
t/t3422-rebase-incompatible-options.sh | 4 ++++
2 files changed, 6 insertions(+), 5 deletions(-)
diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
index 7d01d1412d1..846aeed1b69 100644
--- a/Documentation/git-rebase.txt
+++ b/Documentation/git-rebase.txt
@@ -574,10 +574,7 @@ See also INCOMPATIBLE OPTIONS below.
--root::
Rebase all commits reachable from `<branch>`, instead of
limiting them with an `<upstream>`. This allows you to rebase
- the root commit(s) on a branch. When used with `--onto`, it
- will skip changes already contained in `<newbase>` (instead of
- `<upstream>`) whereas without `--onto` it will operate on every
- change.
+ the root commit(s) on a branch.
+
See also INCOMPATIBLE OPTIONS below.
@@ -656,7 +653,7 @@ are incompatible with the following options:
* --reapply-cherry-picks
* --edit-todo
* --update-refs
- * --root when used in combination with --onto
+ * --root when used without --onto
In addition, the following pairs of options are incompatible:
diff --git a/t/t3422-rebase-incompatible-options.sh b/t/t3422-rebase-incompatible-options.sh
index 9b9e78479f6..f86274990b0 100755
--- a/t/t3422-rebase-incompatible-options.sh
+++ b/t/t3422-rebase-incompatible-options.sh
@@ -65,6 +65,10 @@ test_rebase_am_only () {
test_must_fail git rebase $opt --update-refs A
"
+ test_expect_success "$opt incompatible with --root without --onto" "
+ git checkout B^0 &&
+ test_must_fail git rebase $opt --root A
+ "
}
# Check options which imply --apply
--
gitgitgadget
^ permalink raw reply related
* [PATCH v4 9/9] rebase: provide better error message for apply options vs. merge config
From: Elijah Newren via GitGitGadget @ 2023-01-22 6:12 UTC (permalink / raw)
To: git
Cc: Derrick Stolee, Elijah Newren, Eric Sunshine, Martin Ågren,
Phillip Wood, Elijah Newren, Elijah Newren
In-Reply-To: <pull.1466.v4.git.1674367961.gitgitgadget@gmail.com>
From: Elijah Newren <newren@gmail.com>
When config which selects the merge backend (currently,
rebase.autosquash=true or rebase.updateRefs=true) conflicts with other
options on the command line (such as --whitespace=fix), make the error
message specifically call out the config option and specify how to
override that config option on the command line.
Signed-off-by: Elijah Newren <newren@gmail.com>
---
Documentation/git-rebase.txt | 2 +-
builtin/rebase.c | 27 ++++++++++++++++++++------
t/t3422-rebase-incompatible-options.sh | 24 +++++++++++++++++++++++
3 files changed, 46 insertions(+), 7 deletions(-)
diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
index 9f794f5bdcc..c357f6c3d5c 100644
--- a/Documentation/git-rebase.txt
+++ b/Documentation/git-rebase.txt
@@ -648,7 +648,7 @@ are incompatible with the following options:
* --merge
* --strategy
* --strategy-option
- * --[no-]autosquash
+ * --autosquash
* --rebase-merges
* --interactive
* --exec
diff --git a/builtin/rebase.c b/builtin/rebase.c
index 2358605b254..dd7e2c788f5 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -122,6 +122,8 @@ struct rebase_options {
int reapply_cherry_picks;
int fork_point;
int update_refs;
+ int config_autosquash;
+ int config_update_refs;
};
#define REBASE_OPTIONS_INIT { \
@@ -136,6 +138,10 @@ struct rebase_options {
.fork_point = -1, \
.reapply_cherry_picks = -1, \
.allow_empty_message = 1, \
+ .autosquash = -1, \
+ .config_autosquash = -1, \
+ .update_refs = -1, \
+ .config_update_refs = -1, \
}
static struct replay_opts get_replay_opts(const struct rebase_options *opts)
@@ -778,7 +784,7 @@ static int rebase_config(const char *var, const char *value, void *data)
}
if (!strcmp(var, "rebase.autosquash")) {
- opts->autosquash = git_config_bool(var, value);
+ opts->config_autosquash = git_config_bool(var, value);
return 0;
}
@@ -795,7 +801,7 @@ static int rebase_config(const char *var, const char *value, void *data)
}
if (!strcmp(var, "rebase.updaterefs")) {
- opts->update_refs = git_config_bool(var, value);
+ opts->config_update_refs = git_config_bool(var, value);
return 0;
}
@@ -1393,7 +1399,8 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
if ((options.flags & REBASE_INTERACTIVE_EXPLICIT) ||
(options.action != ACTION_NONE) ||
(options.exec.nr > 0) ||
- options.autosquash) {
+ (options.autosquash == -1 && options.config_autosquash == 1) ||
+ options.autosquash == 1) {
allow_preemptive_ff = 0;
}
if (options.committer_date_is_author_date || options.ignore_date)
@@ -1504,20 +1511,28 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
if (strcmp(options.git_am_opts.v[i], "-q"))
break;
- if (i >= 0) {
+ if (i >= 0 || options.type == REBASE_APPLY) {
if (is_merge(&options))
die(_("apply options and merge options "
"cannot be used together"));
+ else if (options.autosquash == -1 && options.config_autosquash == 1)
+ die(_("apply options are incompatible with rebase.autosquash. Consider adding --no-autosquash"));
+ else if (options.update_refs == -1 && options.config_update_refs == 1)
+ die(_("apply options are incompatible with rebase.updateRefs. Consider adding --no-update-refs"));
else
options.type = REBASE_APPLY;
}
}
- if (options.update_refs)
+ if (options.update_refs == 1)
imply_merge(&options, "--update-refs");
+ options.update_refs = (options.update_refs >= 0) ? options.update_refs :
+ ((options.config_update_refs >= 0) ? options.config_update_refs : 0);
- if (options.autosquash)
+ if (options.autosquash == 1)
imply_merge(&options, "--autosquash");
+ options.autosquash = (options.autosquash >= 0) ? options.autosquash :
+ ((options.config_autosquash >= 0) ? options.config_autosquash : 0);
if (options.type == REBASE_UNSPECIFIED) {
if (!strcmp(options.default_backend, "merge"))
diff --git a/t/t3422-rebase-incompatible-options.sh b/t/t3422-rebase-incompatible-options.sh
index 6a17b571ec7..4711b37a288 100755
--- a/t/t3422-rebase-incompatible-options.sh
+++ b/t/t3422-rebase-incompatible-options.sh
@@ -94,6 +94,30 @@ test_rebase_am_only () {
git checkout B^0 &&
test_must_fail git rebase $opt --root A
"
+
+ test_expect_success "$opt incompatible with rebase.autosquash" "
+ git checkout B^0 &&
+ test_must_fail git -c rebase.autosquash=true rebase $opt A 2>err &&
+ grep -e --no-autosquash err
+ "
+
+ test_expect_success "$opt incompatible with rebase.updateRefs" "
+ git checkout B^0 &&
+ test_must_fail git -c rebase.updateRefs=true rebase $opt A 2>err &&
+ grep -e --no-update-refs err
+ "
+
+ test_expect_success "$opt okay with overridden rebase.autosquash" "
+ test_when_finished \"git reset --hard B^0\" &&
+ git checkout B^0 &&
+ git -c rebase.autosquash=true rebase --no-autosquash $opt A
+ "
+
+ test_expect_success "$opt okay with overridden rebase.updateRefs" "
+ test_when_finished \"git reset --hard B^0\" &&
+ git checkout B^0 &&
+ git -c rebase.updateRefs=true rebase --no-update-refs $opt A
+ "
}
# Check options which imply --apply
--
gitgitgadget
^ permalink raw reply related
* [PATCH v4 5/9] rebase: add coverage of other incompatible options
From: Elijah Newren via GitGitGadget @ 2023-01-22 6:12 UTC (permalink / raw)
To: git
Cc: Derrick Stolee, Elijah Newren, Eric Sunshine, Martin Ågren,
Phillip Wood, Elijah Newren, Elijah Newren
In-Reply-To: <pull.1466.v4.git.1674367961.gitgitgadget@gmail.com>
From: Elijah Newren <newren@gmail.com>
The git-rebase manual noted several sets of incompatible options, but
we were missing tests for a few of these. Further, we were missing
code checks for some of these, which could result in command line
options being silently ignored.
Also, note that adding a check for autosquash means that using
--whitespace=fix together with the config setting rebase.autosquash=true
will trigger an error. A subsequent commit will improve the error
message.
Signed-off-by: Elijah Newren <newren@gmail.com>
---
Documentation/git-rebase.txt | 2 +-
builtin/rebase.c | 30 ++++++++++++++++++++------
t/t3422-rebase-incompatible-options.sh | 25 +++++++++++++++++++++
3 files changed, 49 insertions(+), 8 deletions(-)
diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
index 846aeed1b69..8cb075b2bdb 100644
--- a/Documentation/git-rebase.txt
+++ b/Documentation/git-rebase.txt
@@ -650,7 +650,7 @@ are incompatible with the following options:
* --exec
* --no-keep-empty
* --empty=
- * --reapply-cherry-picks
+ * --[no-]reapply-cherry-picks
* --edit-todo
* --update-refs
* --root when used without --onto
diff --git a/builtin/rebase.c b/builtin/rebase.c
index b742cc8fb5c..ed7475804cb 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -1224,6 +1224,26 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
if (options.fork_point < 0)
options.fork_point = 0;
}
+ /*
+ * The apply backend does not support --[no-]reapply-cherry-picks.
+ * The behavior it implements by default is equivalent to
+ * --no-reapply-cherry-picks (due to passing --cherry-picks to
+ * format-patch), but --keep-base alters the upstream such that no
+ * cherry-picks can be found (effectively making it act like
+ * --reapply-cherry-picks).
+ *
+ * Now, if the user does specify --[no-]reapply-cherry-picks, but
+ * does so in such a way that options.reapply_cherry_picks ==
+ * keep_base, then the behavior they get will match what they
+ * expect despite options.reapply_cherry_picks being ignored. We
+ * could just allow the flag in that case, but it seems better to
+ * just alert the user that they've specified a flag that the
+ * backend ignores.
+ */
+ if (options.reapply_cherry_picks >= 0)
+ imply_merge(&options, options.reapply_cherry_picks ? "--reapply-cherry-picks" :
+ "--no-reapply-cherry-picks");
+
/*
* --keep-base defaults to --reapply-cherry-picks to avoid losing
* commits when using this option.
@@ -1406,13 +1426,6 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
if (options.empty != EMPTY_UNSPECIFIED)
imply_merge(&options, "--empty");
- /*
- * --keep-base implements --reapply-cherry-picks by altering upstream so
- * it works with both backends.
- */
- if (options.reapply_cherry_picks && !keep_base)
- imply_merge(&options, "--reapply-cherry-picks");
-
if (gpg_sign)
options.gpg_sign_opt = xstrfmt("-S%s", gpg_sign);
@@ -1503,6 +1516,9 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
if (options.update_refs)
imply_merge(&options, "--update-refs");
+ if (options.autosquash)
+ imply_merge(&options, "--autosquash");
+
if (options.type == REBASE_UNSPECIFIED) {
if (!strcmp(options.default_backend, "merge"))
imply_merge(&options, "--merge");
diff --git a/t/t3422-rebase-incompatible-options.sh b/t/t3422-rebase-incompatible-options.sh
index f86274990b0..6a17b571ec7 100755
--- a/t/t3422-rebase-incompatible-options.sh
+++ b/t/t3422-rebase-incompatible-options.sh
@@ -50,6 +50,11 @@ test_rebase_am_only () {
test_must_fail git rebase $opt --strategy-option=ours A
"
+ test_expect_success "$opt incompatible with --autosquash" "
+ git checkout B^0 &&
+ test_must_fail git rebase $opt --autosquash A
+ "
+
test_expect_success "$opt incompatible with --interactive" "
git checkout B^0 &&
test_must_fail git rebase $opt --interactive A
@@ -60,6 +65,26 @@ test_rebase_am_only () {
test_must_fail git rebase $opt --exec 'true' A
"
+ test_expect_success "$opt incompatible with --keep-empty" "
+ git checkout B^0 &&
+ test_must_fail git rebase $opt --keep-empty A
+ "
+
+ test_expect_success "$opt incompatible with --empty=..." "
+ git checkout B^0 &&
+ test_must_fail git rebase $opt --empty=ask A
+ "
+
+ test_expect_success "$opt incompatible with --no-reapply-cherry-picks" "
+ git checkout B^0 &&
+ test_must_fail git rebase $opt --no-reapply-cherry-picks A
+ "
+
+ test_expect_success "$opt incompatible with --reapply-cherry-picks" "
+ git checkout B^0 &&
+ test_must_fail git rebase $opt --reapply-cherry-picks A
+ "
+
test_expect_success "$opt incompatible with --update-refs" "
git checkout B^0 &&
test_must_fail git rebase $opt --update-refs A
--
gitgitgadget
^ permalink raw reply related
* [PATCH v4 8/9] rebase: put rebase_options initialization in single place
From: Elijah Newren via GitGitGadget @ 2023-01-22 6:12 UTC (permalink / raw)
To: git
Cc: Derrick Stolee, Elijah Newren, Eric Sunshine, Martin Ågren,
Phillip Wood, Elijah Newren, Elijah Newren
In-Reply-To: <pull.1466.v4.git.1674367961.gitgitgadget@gmail.com>
From: Elijah Newren <newren@gmail.com>
Signed-off-by: Elijah Newren <newren@gmail.com>
---
builtin/rebase.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/builtin/rebase.c b/builtin/rebase.c
index ed7475804cb..2358605b254 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -134,6 +134,8 @@ struct rebase_options {
.exec = STRING_LIST_INIT_NODUP, \
.git_format_patch_opt = STRBUF_INIT, \
.fork_point = -1, \
+ .reapply_cherry_picks = -1, \
+ .allow_empty_message = 1, \
}
static struct replay_opts get_replay_opts(const struct rebase_options *opts)
@@ -1158,8 +1160,6 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
prepare_repo_settings(the_repository);
the_repository->settings.command_requires_full_index = 0;
- options.reapply_cherry_picks = -1;
- options.allow_empty_message = 1;
git_config(rebase_config, &options);
/* options.gpg_sign_opt will be either "-S" or NULL */
gpg_sign = options.gpg_sign_opt ? "" : NULL;
--
gitgitgadget
^ permalink raw reply related
* [PATCH v4 3/9] rebase: remove --allow-empty-message from incompatible opts
From: Elijah Newren via GitGitGadget @ 2023-01-22 6:12 UTC (permalink / raw)
To: git
Cc: Derrick Stolee, Elijah Newren, Eric Sunshine, Martin Ågren,
Phillip Wood, Elijah Newren, Elijah Newren
In-Reply-To: <pull.1466.v4.git.1674367961.gitgitgadget@gmail.com>
From: Elijah Newren <newren@gmail.com>
--allow-empty-message was turned into a no-op and even documented
as such; the flag is simply ignored. Since the flag is ignored, it
shouldn't be documented as being incompatible with other flags.
Signed-off-by: Elijah Newren <newren@gmail.com>
---
Documentation/git-rebase.txt | 1 -
1 file changed, 1 deletion(-)
diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
index 6490bc96a15..7d01d1412d1 100644
--- a/Documentation/git-rebase.txt
+++ b/Documentation/git-rebase.txt
@@ -647,7 +647,6 @@ are incompatible with the following options:
* --merge
* --strategy
* --strategy-option
- * --allow-empty-message
* --[no-]autosquash
* --rebase-merges
* --interactive
--
gitgitgadget
^ permalink raw reply related
* [PATCH v4 1/9] rebase: mark --update-refs as requiring the merge backend
From: Elijah Newren via GitGitGadget @ 2023-01-22 6:12 UTC (permalink / raw)
To: git
Cc: Derrick Stolee, Elijah Newren, Eric Sunshine, Martin Ågren,
Phillip Wood, Elijah Newren, Elijah Newren
In-Reply-To: <pull.1466.v4.git.1674367961.gitgitgadget@gmail.com>
From: Elijah Newren <newren@gmail.com>
--update-refs is built in terms of the sequencer, which requires the
merge backend. It was already marked as incompatible with the apply
backend in the git-rebase manual, but the code didn't check for this
incompatibility and warn the user. Check and error now.
While at it, fix a typo in t3422...and fix some misleading wording
(most options which used to be am-specific have since been implemented
in the merge backend as well).
Signed-off-by: Elijah Newren <newren@gmail.com>
---
Documentation/git-rebase.txt | 2 ++
builtin/rebase.c | 3 +++
t/t3422-rebase-incompatible-options.sh | 15 ++++++++++-----
3 files changed, 15 insertions(+), 5 deletions(-)
diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
index d811c1cf443..6490bc96a15 100644
--- a/Documentation/git-rebase.txt
+++ b/Documentation/git-rebase.txt
@@ -630,6 +630,8 @@ start would be overridden by the presence of
+
If the configuration variable `rebase.updateRefs` is set, then this option
can be used to override and disable this setting.
++
+See also INCOMPATIBLE OPTIONS below.
INCOMPATIBLE OPTIONS
--------------------
diff --git a/builtin/rebase.c b/builtin/rebase.c
index a26cc0cfdb5..c111b89e137 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -1492,6 +1492,9 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
}
}
+ if (options.update_refs)
+ imply_merge(&options, "--update-refs");
+
if (options.type == REBASE_UNSPECIFIED) {
if (!strcmp(options.default_backend, "merge"))
imply_merge(&options, "--merge");
diff --git a/t/t3422-rebase-incompatible-options.sh b/t/t3422-rebase-incompatible-options.sh
index 6dabb05a2ad..9da39cd91c2 100755
--- a/t/t3422-rebase-incompatible-options.sh
+++ b/t/t3422-rebase-incompatible-options.sh
@@ -25,11 +25,11 @@ test_expect_success 'setup' '
'
#
-# Rebase has lots of useful options like --whitepsace=fix, which are
-# actually all built in terms of flags to git-am. Since neither
-# --merge nor --interactive (nor any options that imply those two) use
-# git-am, using them together will result in flags like --whitespace=fix
-# being ignored. Make sure rebase warns the user and aborts instead.
+# Rebase has a couple options which are specific to the apply backend,
+# and several options which are specific to the merge backend. Flags
+# from the different sets cannot work together, and we do not want to
+# just ignore one of the sets of flags. Make sure rebase warns the
+# user and aborts instead.
#
test_rebase_am_only () {
@@ -60,6 +60,11 @@ test_rebase_am_only () {
test_must_fail git rebase $opt --exec 'true' A
"
+ test_expect_success "$opt incompatible with --update-refs" "
+ git checkout B^0 &&
+ test_must_fail git rebase $opt --update-refs A
+ "
+
}
test_rebase_am_only --whitespace=fix
--
gitgitgadget
^ permalink raw reply related
* [PATCH v4 0/9] rebase: fix several code/testing/documentation issues around flag incompatibilities
From: Elijah Newren via GitGitGadget @ 2023-01-22 6:12 UTC (permalink / raw)
To: git
Cc: Derrick Stolee, Elijah Newren, Eric Sunshine, Martin Ågren,
Phillip Wood, Elijah Newren
In-Reply-To: <pull.1466.v3.git.1674266126.gitgitgadget@gmail.com>
We had a report about --update-refs being ignored when --whitespace=fix was
passed, confusing an end user. These were already marked as incompatible in
the manual, but the code didn't check for the incompatibility and provide an
error to the user.
Folks brought up other flags tangentially when reviewing an earlier round of
this series, and I found we had more that were missing checks, and that we
were missing some testcases, and that the documentation was wrong on some of
the relevant options. So, I ended up getting lots of little fixes to
straighten these all out.
Changes since v3:
* Corrected the code surrounding --[no-]reapply-cherry-picks, and extended
the testcases (Thanks to Phillip for pointing out my error)
* I went ahead and implemented the better error message when the merge
backend is triggered solely via config options.
Changes since v2:
* Remove the extra patch and reword the comment in t3422 more thoroughly.
* Add tests for other flag incompatibilities that were missing
* Add code that was missing for checking various flag incompatibilities
* Fix incorrect claims in the documentation around incompatible options
* A few other miscellaneous fixups noticed while doing all the above
Changes since v1:
* Add a patch nuking the -C option to rebase (fixes confusion around the
comment in t3422 and acknowledges the fact that the option is totally and
utterly useless and always has been. It literally never affects the
results of a rebase.)
Signed-off-by: Elijah Newren newren@gmail.com
Elijah Newren (9):
rebase: mark --update-refs as requiring the merge backend
rebase: flag --apply and --merge as incompatible
rebase: remove --allow-empty-message from incompatible opts
rebase: fix docs about incompatibilities with --root
rebase: add coverage of other incompatible options
rebase: clarify the OPT_CMDMODE incompatibilities
rebase: fix formatting of rebase --reapply-cherry-picks option in docs
rebase: put rebase_options initialization in single place
rebase: provide better error message for apply options vs. merge
config
Documentation/git-rebase.txt | 77 +++++++++++++-------------
builtin/rebase.c | 72 +++++++++++++++++++-----
t/t3422-rebase-incompatible-options.sh | 71 ++++++++++++++++++++++--
3 files changed, 162 insertions(+), 58 deletions(-)
base-commit: 56c8fb1e95377900ec9d53c07886022af0a5d3c2
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1466%2Fnewren%2Frebase-update-refs-imply-merge-v4
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1466/newren/rebase-update-refs-imply-merge-v4
Pull-Request: https://github.com/gitgitgadget/git/pull/1466
Range-diff vs v3:
1: 9089834adea = 1: 8a676e6ec1a rebase: mark --update-refs as requiring the merge backend
2: a8b5a0e4fb0 = 2: cc129b87185 rebase: flag --apply and --merge as incompatible
3: f4fbfd40d45 = 3: 9464bbbe9ba rebase: remove --allow-empty-message from incompatible opts
4: a1e61ac8f21 = 4: b702f15ed7c rebase: fix docs about incompatibilities with --root
5: 48c40c0dda0 ! 5: 5e4851e611e rebase: add coverage of other incompatible options
@@ Commit message
code checks for some of these, which could result in command line
options being silently ignored.
+ Also, note that adding a check for autosquash means that using
+ --whitespace=fix together with the config setting rebase.autosquash=true
+ will trigger an error. A subsequent commit will improve the error
+ message.
+
Signed-off-by: Elijah Newren <newren@gmail.com>
+ ## Documentation/git-rebase.txt ##
+@@ Documentation/git-rebase.txt: are incompatible with the following options:
+ * --exec
+ * --no-keep-empty
+ * --empty=
+- * --reapply-cherry-picks
++ * --[no-]reapply-cherry-picks
+ * --edit-todo
+ * --update-refs
+ * --root when used without --onto
+
## builtin/rebase.c ##
@@ builtin/rebase.c: int cmd_rebase(int argc, const char **argv, const char *prefix)
if (options.fork_point < 0)
options.fork_point = 0;
}
+ /*
-+ * reapply_cherry_picks is slightly weird. It starts out with a
-+ * value of -1. It will be assigned a value of keep_base below and
-+ * then handled specially. The apply backend is hardcoded to
-+ * behave like reapply_cherry_picks==1, so if it has that value, we
-+ * can just ignore the flag with the apply backend. Thus, we only
-+ * really need to throw an error and require the merge backend if
-+ * reapply_cherry_picks==0.
++ * The apply backend does not support --[no-]reapply-cherry-picks.
++ * The behavior it implements by default is equivalent to
++ * --no-reapply-cherry-picks (due to passing --cherry-picks to
++ * format-patch), but --keep-base alters the upstream such that no
++ * cherry-picks can be found (effectively making it act like
++ * --reapply-cherry-picks).
++ *
++ * Now, if the user does specify --[no-]reapply-cherry-picks, but
++ * does so in such a way that options.reapply_cherry_picks ==
++ * keep_base, then the behavior they get will match what they
++ * expect despite options.reapply_cherry_picks being ignored. We
++ * could just allow the flag in that case, but it seems better to
++ * just alert the user that they've specified a flag that the
++ * backend ignores.
+ */
-+ if (options.reapply_cherry_picks == 0)
-+ imply_merge(&options, "--no-reapply-cherry-picks");
++ if (options.reapply_cherry_picks >= 0)
++ imply_merge(&options, options.reapply_cherry_picks ? "--reapply-cherry-picks" :
++ "--no-reapply-cherry-picks");
++
/*
* --keep-base defaults to --reapply-cherry-picks to avoid losing
* commits when using this option.
@@ t/t3422-rebase-incompatible-options.sh: test_rebase_am_only () {
+ git checkout B^0 &&
+ test_must_fail git rebase $opt --no-reapply-cherry-picks A
+ "
++
++ test_expect_success "$opt incompatible with --reapply-cherry-picks" "
++ git checkout B^0 &&
++ test_must_fail git rebase $opt --reapply-cherry-picks A
++ "
+
test_expect_success "$opt incompatible with --update-refs" "
git checkout B^0 &&
6: 8664cce6cf7 ! 6: 21ae9e05313 rebase: clarify the OPT_CMDMODE incompatibilities
@@ Documentation/git-rebase.txt: See also INCOMPATIBLE OPTIONS below.
@@ Documentation/git-rebase.txt: are incompatible with the following options:
* --no-keep-empty
* --empty=
- * --reapply-cherry-picks
+ * --[no-]reapply-cherry-picks
- * --edit-todo
* --update-refs
* --root when used without --onto
7: 0e8b06163f2 = 7: 74a216bf0c0 rebase: fix formatting of rebase --reapply-cherry-picks option in docs
-: ----------- > 8: a8adf7fda61 rebase: put rebase_options initialization in single place
-: ----------- > 9: 5cb00e5103b rebase: provide better error message for apply options vs. merge config
--
gitgitgadget
^ permalink raw reply
* [PATCH v4 2/9] rebase: flag --apply and --merge as incompatible
From: Elijah Newren via GitGitGadget @ 2023-01-22 6:12 UTC (permalink / raw)
To: git
Cc: Derrick Stolee, Elijah Newren, Eric Sunshine, Martin Ågren,
Phillip Wood, Elijah Newren, Elijah Newren
In-Reply-To: <pull.1466.v4.git.1674367961.gitgitgadget@gmail.com>
From: Elijah Newren <newren@gmail.com>
Previously, we flagged options which implied --apply as being
incompatible with options which implied --merge. But if both options
were given explicitly, then we didn't flag the incompatibility. The
same is true with --apply and --interactive. Add the check, and add
some testcases to verify these are also caught.
Signed-off-by: Elijah Newren <newren@gmail.com>
---
builtin/rebase.c | 12 ++++++++++--
t/t3422-rebase-incompatible-options.sh | 3 +++
2 files changed, 13 insertions(+), 2 deletions(-)
diff --git a/builtin/rebase.c b/builtin/rebase.c
index c111b89e137..b742cc8fb5c 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -907,6 +907,9 @@ static int parse_opt_am(const struct option *opt, const char *arg, int unset)
BUG_ON_OPT_NEG(unset);
BUG_ON_OPT_ARG(arg);
+ if (opts->type != REBASE_UNSPECIFIED && opts->type != REBASE_APPLY)
+ die(_("apply options and merge options cannot be used together"));
+
opts->type = REBASE_APPLY;
return 0;
@@ -920,8 +923,10 @@ static int parse_opt_merge(const struct option *opt, const char *arg, int unset)
BUG_ON_OPT_NEG(unset);
BUG_ON_OPT_ARG(arg);
- if (!is_merge(opts))
- opts->type = REBASE_MERGE;
+ if (opts->type != REBASE_UNSPECIFIED && opts->type != REBASE_MERGE)
+ die(_("apply options and merge options cannot be used together"));
+
+ opts->type = REBASE_MERGE;
return 0;
}
@@ -935,6 +940,9 @@ static int parse_opt_interactive(const struct option *opt, const char *arg,
BUG_ON_OPT_NEG(unset);
BUG_ON_OPT_ARG(arg);
+ if (opts->type != REBASE_UNSPECIFIED && opts->type != REBASE_MERGE)
+ die(_("apply options and merge options cannot be used together"));
+
opts->type = REBASE_MERGE;
opts->flags |= REBASE_INTERACTIVE_EXPLICIT;
diff --git a/t/t3422-rebase-incompatible-options.sh b/t/t3422-rebase-incompatible-options.sh
index 9da39cd91c2..9b9e78479f6 100755
--- a/t/t3422-rebase-incompatible-options.sh
+++ b/t/t3422-rebase-incompatible-options.sh
@@ -67,7 +67,10 @@ test_rebase_am_only () {
}
+# Check options which imply --apply
test_rebase_am_only --whitespace=fix
test_rebase_am_only -C4
+# Also check an explicit --apply
+test_rebase_am_only --apply
test_done
--
gitgitgadget
^ permalink raw reply related
* Re: [PATCH v3 5/7] rebase: add coverage of other incompatible options
From: Elijah Newren @ 2023-01-22 5:11 UTC (permalink / raw)
To: phillip.wood
Cc: Elijah Newren via GitGitGadget, git, Derrick Stolee,
Eric Sunshine, Martin Ågren, Phillip Wood
In-Reply-To: <20e759bd-7d13-b7ba-c0df-69b4f827c5bd@dunelm.org.uk>
On Sat, Jan 21, 2023 at 11:25 AM Phillip Wood <phillip.wood123@gmail.com> wrote:
>
> On 21/01/2023 15:20, Phillip Wood wrote:
> >> @@ -1238,6 +1238,17 @@ int cmd_rebase(int argc, const char **argv,
> >> const char *prefix)
> >> if (options.fork_point < 0)
> >> options.fork_point = 0;
> >> }
> >> + /*
> >> + * reapply_cherry_picks is slightly weird. It starts out with a
> >> + * value of -1. It will be assigned a value of keep_base below and
> >> + * then handled specially. The apply backend is hardcoded to
> >> + * behave like reapply_cherry_picks==1,
> >
> > I think it is hard coded to 0 not 1. We generate the patches with
> >
> > git format-patch --right-only $upstream...$head
>
> Sorry I somhow managed to omit --cherry-pick from that, it should have been
>
> git format-patch --right-only --cherry-pick $upstream...$head
>
> Phillip
Oh, indeed, I was reading wrong. Thanks for the correction. I still
think there's something to fix up here, which I'll include in my
re-roll.
> > so cherry-picks will not be reapplied. I'm hardly an impartial observer
> > but I think the current check is correct. We support
> >
> > --whitespace=fix --no-reapply-cherry-picks
> > --whitespace=fix --keep-base --reapply-cherry-picks
> >
> > but not
> >
> > --whitespace=fix --reapply-cherry-picks
Right, nor do we support
--whitespace=fix --keep-base --no-reapply-cherry-picks
(If you try it, you'll notice that the code accepts those flags and
starts the rebase pretending everything is fine, but it silently
ignores the --no-reapply-cherry-picks flag.)
Stepping back a bit, though, the apply backend doesn't support
toggling --[no-]reapply-cherry-picks at all. It hard codes the
behavior (in a way dependent upon --keep-base). So, if the user
passes --[no-]reapply-cherry-picks and we don't error out, we are just
going to ignore whatever they passed and do whatever hardcoded thing
is baked into the algorithm.
While the user can pass the flag in a way that happens to match what
is baked into the apply backend, I'd still say it's wrong for them to
specify it since we will just ignore it. Doing so is likely to result
in the user later toggling the flag and being surprised that they get
an error when it used to be accepted, or seeing that it only sometimes
gets accepted.
Anyway, I'll update this patch to document this a bit more clearly in
a code comment and add an improved check.
^ permalink raw reply
* [PATCH] attr: fix instructions on how to check attrs
From: John Cai via GitGitGadget @ 2023-01-22 3:06 UTC (permalink / raw)
To: git; +Cc: John Cai, John Cai
From: John Cai <johncai86@gmail.com>
The instructions in attr.h describing what functions to call to check
attributes is missing the index as the first argument to git_check_attr.
Fix this to make it consistent with the actual function signature.
Signed-off-by: John Cai <johncai86@gmail.com>
---
attr: fix instructions on how to check attrs
The instructions in attr.h describing what functions to call to check
attributes is missing the index as the first argument to git_check_attr.
Fix this to make it consistent with the actual function signature.
Signed-off-by: John Cai johncai86@gmail.com
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1441%2Fjohn-cai%2Fjc%2Ffix-attr-docs-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1441/john-cai/jc/fix-attr-docs-v1
Pull-Request: https://github.com/git/git/pull/1441
attr.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/attr.h b/attr.h
index 2f22dffadb3..47f1111f391 100644
--- a/attr.h
+++ b/attr.h
@@ -45,7 +45,7 @@
* const char *path;
*
* setup_check();
- * git_check_attr(path, check);
+ * git_check_attr(&the_index, path, check);
* ------------
*
* - Act on `.value` member of the result, left in `check->items[]`:
base-commit: 904d404274fef6695c78a6b055edd184b72e2f9b
--
gitgitgadget
^ permalink raw reply related
* [PATCH] MyFirstContribution: refrain from self-iterating too much
From: Junio C Hamano @ 2023-01-22 1:51 UTC (permalink / raw)
To: git
Finding mistakes in and improving your own patches is a good idea,
but doing so too quickly is being inconsiderate to reviewers who
have just seen the initial iteration and taking their time to review
it. Encourage new developers to perform such a self review before
they send out their patches, not after.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
Documentation/MyFirstContribution.txt | 20 ++++++++++++++++++++
1 file changed, 20 insertions(+)
diff --git c/Documentation/MyFirstContribution.txt w/Documentation/MyFirstContribution.txt
index 7c9a037cc2..81dcaedf0c 100644
--- c/Documentation/MyFirstContribution.txt
+++ w/Documentation/MyFirstContribution.txt
@@ -1136,6 +1136,26 @@ index 88f126184c..38da593a60 100644
[[now-what]]
== My Patch Got Emailed - Now What?
+You should wait for your patch to be reviewed by other people in the
+development community. While you are waiting, you may want to
+re-read what you wrote in the patch you already have sent, as if you
+are a reviewer who is helping you to improve your patch. But resist
+the temptation to update the patch and send a new one, until other
+people had enough time to digest your original patch and give you
+their reviews. They may be taking time to give you a carefully
+written review responses and haven't finished it yet. Bombarding
+them with new versions before they have a chance to react to the
+first iteration is being rude to them.
+
+Of course, you still may spot mistakes and rooms for improvements
+after you sent your initial patch. Learn from that experience to
+make sure that you will do such a self-review _before_ sending your
+patches next time. You do not have to send your patches immediately
+once you finished writing them. It is not a race. Take your time
+to self-review them, sleep on them, improve them before sending them
+out, and do not allow you to send them before you are reasonably
+sure that you won't find more mistakes in them yourself.
+
[[reviewing]]
=== Responding to Reviews
^ permalink raw reply related
* Re: [PATCH v2 1/3] branch: fix die_if_checked_out() when ignore_current_worktree
From: Junio C Hamano @ 2023-01-22 1:50 UTC (permalink / raw)
To: Rubén Justo; +Cc: Git List, Phillip Wood
In-Reply-To: <17f267b1-7f5e-2fb6-fb14-1c37ec355e65@gmail.com>
Rubén Justo <rjusto@gmail.com> writes:
> Let's stop using find_shared_symref() in die_if_checked_out(), to handle
> correctly ignore_current_worktree.
This says what the code stops using, but does not say what it uses
instead.
Factoring is_shared_symref() out of find_shared_symref() is probably
a good idea that can stand alone without any other change in this
patch as a single step, and then a second step can update
die_if_checked_out() using the new function.
As the proposed log message explained, updating die_if_checked_out()
with this patch would fix a bug---can we demonstrate the existing
breakage and protect the fix from future breakages by adding a test
or two?
Other than that, it looks like it is going in the right direction.
Thanks for working on the topic.
> Signed-off-by: Rubén Justo <rjusto@gmail.com>
> ---
> branch.c | 16 +++++++++++-----
> worktree.c | 54 +++++++++++++++++++++++++++++-------------------------
> worktree.h | 6 ++++++
> 3 files changed, 46 insertions(+), 30 deletions(-)
>
> diff --git a/branch.c b/branch.c
> index d182756827..2378368415 100644
> --- a/branch.c
> +++ b/branch.c
> @@ -820,12 +820,18 @@ void remove_branch_state(struct repository *r, int verbose)
> void die_if_checked_out(const char *branch, int ignore_current_worktree)
> {
> struct worktree **worktrees = get_worktrees();
> - const struct worktree *wt;
> + int i;
> +
> + for (i = 0; worktrees[i]; i++)
> + {
Style. WRite the above on a single line, i.e.
for (i = 0; worktrees[i]; i++) {
Optionally, we can lose the separate declaration of "i" by using C99
variable declaration, i.e.
for (int i = 0; worktrees[i]; i++) {
> diff --git a/worktree.c b/worktree.c
> index aa43c64119..d500d69e4c 100644
> --- a/worktree.c
> +++ b/worktree.c
> @@ -403,6 +403,33 @@ int is_worktree_being_bisected(const struct worktree *wt,
> * bisect). New commands that do similar things should update this
> * function as well.
> */
The above comment is about find_shared_symref() which iterates over
worktrees and find the one that uses the named symref. Now the
comment appears to apply to is_shared_symref() which does not
iterate but takes one specific worktree instance. Do their
differences necessitate some updates to the comment?
> +int is_shared_symref(const struct worktree *wt, const char *symref,
> + const char *target)
> +{
What this function does sound more like "is target in use in this
particular worktree by being pointed at by the symref?" IOW, I do
not see where "shared" comes into its name from.
"HEAD" that is tentatively detached while bisecting or rebasing the
"target" branch is still considered to point at the "target", so
perhaps symref_points_at_target() or something?
> const struct worktree *find_shared_symref(struct worktree **worktrees,
> const char *symref,
> const char *target)
> @@ -411,31 +438,8 @@ const struct worktree *find_shared_symref(struct worktree **worktrees,
> int i = 0;
>
> for (i = 0; worktrees[i]; i++) {
Not a new problem, but the initialization on the declaration of "i"
is redundant and unnecessary. Again, we can use the C99 style, i.e.
const struct worktree *existing = NULL;
- int i = 0;
-
- for (i = 0; worktrees[i]; i++) {
+ for (int i = 0; worktrees[i]; i++) {
> + if (is_shared_symref(worktrees[i], symref, target)) {
> + existing = worktrees[i];
> break;
> }
> }
> diff --git a/worktree.h b/worktree.h
> index 9dcea6fc8c..7889c4761d 100644
> --- a/worktree.h
> +++ b/worktree.h
> @@ -149,6 +149,12 @@ const struct worktree *find_shared_symref(struct worktree **worktrees,
> const char *symref,
> const char *target);
>
> +/*
> + * Returns true if a symref points to a ref in a worktree.
> + */
Make it clear that what you called "a ref" in the above is what is
called "target" below.
> +int is_shared_symref(const struct worktree *wt,
> + const char *symref, const char *target);
> +
> /*
> * Similar to head_ref() for all HEADs _except_ one from the current
> * worktree, which is covered by head_ref().
^ permalink raw reply
* [PATCH] bisect: fix "reset" when branch is checked out elsewhere
From: Rubén Justo @ 2023-01-22 1:38 UTC (permalink / raw)
To: Git List; +Cc: Junio C Hamano, Phillip Wood
Since 1d0fa89 (checkout: add --ignore-other-wortrees, 2015-01-03) we
have a safety valve in checkout/switch to prevent the same branch from
being checked out simultaneously in multiple worktrees.
If a branch is bisected in a worktree while also being checked out in
another worktree; when the bisection is finished, checking out the
branch back in the current worktree may fail.
Let's teach bisect to use the "--ignore-other-worktrees" flag.
Signed-off-by: Rubén Justo <rjusto@gmail.com>
---
builtin/bisect.c | 3 ++-
t/t6030-bisect-porcelain.sh | 23 +++++++++++++++++++++++
2 files changed, 25 insertions(+), 1 deletion(-)
diff --git a/builtin/bisect.c b/builtin/bisect.c
index cc9483e851..56da34648b 100644
--- a/builtin/bisect.c
+++ b/builtin/bisect.c
@@ -245,7 +245,8 @@ static int bisect_reset(const char *commit)
struct child_process cmd = CHILD_PROCESS_INIT;
cmd.git_cmd = 1;
- strvec_pushl(&cmd.args, "checkout", branch.buf, "--", NULL);
+ strvec_pushl(&cmd.args, "checkout", "--ignore-other-worktrees",
+ branch.buf, "--", NULL);
if (run_command(&cmd)) {
error(_("could not check out original"
" HEAD '%s'. Try 'git bisect"
diff --git a/t/t6030-bisect-porcelain.sh b/t/t6030-bisect-porcelain.sh
index 98a72ff78a..cc8acbab18 100755
--- a/t/t6030-bisect-porcelain.sh
+++ b/t/t6030-bisect-porcelain.sh
@@ -122,6 +122,29 @@ test_expect_success 'bisect start without -- takes unknown arg as pathspec' '
grep bar ".git/BISECT_NAMES"
'
+test_expect_success 'bisect reset: back in a branch checked out also elsewhere' '
+ echo "shared" > branch.expect &&
+ test_bisect_reset() {
+ git -C $1 bisect start &&
+ git -C $1 bisect good $HASH1 &&
+ git -C $1 bisect bad $HASH3 &&
+ git -C $1 bisect reset &&
+ git -C $1 branch --show-current > branch.output &&
+ cmp branch.expect branch.output
+ } &&
+ test_when_finished "
+ git worktree remove wt1 &&
+ git worktree remove wt2 &&
+ git branch -d shared
+ " &&
+ git worktree add wt1 -b shared &&
+ git worktree add wt2 -f shared &&
+ # we test in both worktrees to ensure that works
+ # as expected with "first" and "next" worktrees
+ test_bisect_reset wt1 &&
+ test_bisect_reset wt2
+'
+
test_expect_success 'bisect reset: back in the main branch' '
git bisect reset &&
echo "* main" > branch.expect &&
--
2.39.0
^ permalink raw reply related
* [PATCH v2 3/3] switch: reject if the branch is already checked out elsewhere (test)
From: Rubén Justo @ 2023-01-22 1:28 UTC (permalink / raw)
To: Git List; +Cc: Junio C Hamano, Phillip Wood
In-Reply-To: <f7f45f54-9261-45ea-3399-8ba8dee6832b@gmail.com>
Since 5883034 (checkout: reject if the branch is already checked out
elsewhere) in normal use, we do not allow multiple worktrees having the
same checked out branch.
A bug has recently been fixed that caused this to not work as expected.
Let's add a test to notice if this changes in the future.
Signed-off-by: Rubén Justo <rjusto@gmail.com>
---
t/t2060-switch.sh | 29 +++++++++++++++++++++++++++++
1 file changed, 29 insertions(+)
diff --git a/t/t2060-switch.sh b/t/t2060-switch.sh
index 5a7caf958c..7bea95dba2 100755
--- a/t/t2060-switch.sh
+++ b/t/t2060-switch.sh
@@ -146,4 +146,33 @@ test_expect_success 'tracking info copied with autoSetupMerge=inherit' '
test_cmp_config "" --default "" branch.main2.merge
'
+test_expect_success 'switch back when temporarily detached and checked out elsewhere ' '
+ test_when_finished "
+ git worktree remove wt1 &&
+ git worktree remove wt2 &&
+ git branch -d shared
+ git checkout -
+ " &&
+ git checkout -b shared &&
+ test_commit shared-first &&
+ HASH1=$(git rev-parse --verify HEAD) &&
+ test_commit shared-second &&
+ test_commit shared-third &&
+ HASH2=$(git rev-parse --verify HEAD) &&
+ git worktree add wt1 -f shared &&
+ git -C wt1 bisect start &&
+ git -C wt1 bisect good $HASH1 &&
+ git -C wt1 bisect bad $HASH2 &&
+ git worktree add wt2 -f shared &&
+ git -C wt2 bisect start &&
+ git -C wt2 bisect good $HASH1 &&
+ git -C wt2 bisect bad $HASH2 &&
+ # we test in both worktrees to ensure that works
+ # as expected with "first" and "next" worktrees
+ test_must_fail git -C wt1 switch shared &&
+ git -C wt1 switch --ignore-other-worktrees shared &&
+ test_must_fail git -C wt2 switch shared &&
+ git -C wt2 switch --ignore-other-worktrees shared
+'
+
test_done
--
2.39.0
^ permalink raw reply related
* [PATCH v2 2/3] rebase: refuse to switch to a branch already checked out elsewhere (test)
From: Rubén Justo @ 2023-01-22 1:28 UTC (permalink / raw)
To: Git List; +Cc: Junio C Hamano, Phillip Wood
In-Reply-To: <f7f45f54-9261-45ea-3399-8ba8dee6832b@gmail.com>
In b5cabb4a9 (rebase: refuse to switch to branch already checked out
elsewhere, 2020-02-23) we add a condition to prevent a rebase operation
involving a switch to a branch that is already checked out in another
worktree.
A bug has recently been fixed that caused this to not work as expected.
Let's add a test to notice if this changes in the future.
Signed-off-by: Rubén Justo <rjusto@gmail.com>
---
t/t3400-rebase.sh | 14 ++++++++++++++
1 file changed, 14 insertions(+)
diff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh
index d5a8ee39fc..3ce918fdb8 100755
--- a/t/t3400-rebase.sh
+++ b/t/t3400-rebase.sh
@@ -388,6 +388,20 @@ test_expect_success 'switch to branch checked out here' '
git rebase main main
'
+test_expect_success 'switch to branch checked out elsewhere fails' '
+ test_when_finished "
+ git worktree remove wt1 &&
+ git worktree remove wt2 &&
+ git branch -d shared
+ " &&
+ git worktree add wt1 -b shared &&
+ git worktree add wt2 -f shared &&
+ # we test in both worktrees to ensure that works
+ # as expected with "first" and "next" worktrees
+ test_must_fail git -C wt1 rebase shared shared &&
+ test_must_fail git -C wt2 rebase shared shared
+'
+
test_expect_success 'switch to branch not checked out' '
git checkout main &&
git branch other &&
--
2.39.0
^ permalink raw reply related
* [PATCH v2 1/3] branch: fix die_if_checked_out() when ignore_current_worktree
From: Rubén Justo @ 2023-01-22 1:23 UTC (permalink / raw)
To: Git List; +Cc: Junio C Hamano, Phillip Wood
In-Reply-To: <f7f45f54-9261-45ea-3399-8ba8dee6832b@gmail.com>
In 8d9fdd7 (worktree.c: check whether branch is rebased in another
worktree, 2016-04-22) die_if_checked_out() learned a new
option ignore_current_worktree, to modify the operation from "die() if
the branch is checked out in any worktree" to "die() if the branch
is checked out in any worktree other than the current one".
Unfortunately we implemented it by checking the flag is_current in the
worktree that find_shared_symref() returns.
When the same branch is checked out in several worktrees simultaneously,
find_shared_symref() will return the first matching worktree in the list
composed by get_worktrees(). If one of the worktrees with the checked
out branch is the current worktree, find_shared_symref() may or may not
return it, depending on the order of the list.
Let's stop using find_shared_symref() in die_if_checked_out(), to handle
correctly ignore_current_worktree.
Signed-off-by: Rubén Justo <rjusto@gmail.com>
---
branch.c | 16 +++++++++++-----
worktree.c | 54 +++++++++++++++++++++++++++++-------------------------
worktree.h | 6 ++++++
3 files changed, 46 insertions(+), 30 deletions(-)
diff --git a/branch.c b/branch.c
index d182756827..2378368415 100644
--- a/branch.c
+++ b/branch.c
@@ -820,12 +820,18 @@ void remove_branch_state(struct repository *r, int verbose)
void die_if_checked_out(const char *branch, int ignore_current_worktree)
{
struct worktree **worktrees = get_worktrees();
- const struct worktree *wt;
+ int i;
+
+ for (i = 0; worktrees[i]; i++)
+ {
+ if (worktrees[i]->is_current && ignore_current_worktree)
+ continue;
- wt = find_shared_symref(worktrees, "HEAD", branch);
- if (wt && (!ignore_current_worktree || !wt->is_current)) {
- skip_prefix(branch, "refs/heads/", &branch);
- die(_("'%s' is already checked out at '%s'"), branch, wt->path);
+ if (is_shared_symref(worktrees[i], "HEAD", branch)) {
+ skip_prefix(branch, "refs/heads/", &branch);
+ die(_("'%s' is already checked out at '%s'"),
+ branch, worktrees[i]->path);
+ }
}
free_worktrees(worktrees);
diff --git a/worktree.c b/worktree.c
index aa43c64119..d500d69e4c 100644
--- a/worktree.c
+++ b/worktree.c
@@ -403,6 +403,33 @@ int is_worktree_being_bisected(const struct worktree *wt,
* bisect). New commands that do similar things should update this
* function as well.
*/
+int is_shared_symref(const struct worktree *wt, const char *symref,
+ const char *target)
+{
+ const char *symref_target;
+ struct ref_store *refs;
+ int flags;
+
+ if (wt->is_bare)
+ return 0;
+
+ if (wt->is_detached && !strcmp(symref, "HEAD")) {
+ if (is_worktree_being_rebased(wt, target))
+ return 1;
+ if (is_worktree_being_bisected(wt, target))
+ return 1;
+ }
+
+ refs = get_worktree_ref_store(wt);
+ symref_target = refs_resolve_ref_unsafe(refs, symref, 0,
+ NULL, &flags);
+ if ((flags & REF_ISSYMREF) &&
+ symref_target && !strcmp(symref_target, target))
+ return 1;
+
+ return 0;
+}
+
const struct worktree *find_shared_symref(struct worktree **worktrees,
const char *symref,
const char *target)
@@ -411,31 +438,8 @@ const struct worktree *find_shared_symref(struct worktree **worktrees,
int i = 0;
for (i = 0; worktrees[i]; i++) {
- struct worktree *wt = worktrees[i];
- const char *symref_target;
- struct ref_store *refs;
- int flags;
-
- if (wt->is_bare)
- continue;
-
- if (wt->is_detached && !strcmp(symref, "HEAD")) {
- if (is_worktree_being_rebased(wt, target)) {
- existing = wt;
- break;
- }
- if (is_worktree_being_bisected(wt, target)) {
- existing = wt;
- break;
- }
- }
-
- refs = get_worktree_ref_store(wt);
- symref_target = refs_resolve_ref_unsafe(refs, symref, 0,
- NULL, &flags);
- if ((flags & REF_ISSYMREF) &&
- symref_target && !strcmp(symref_target, target)) {
- existing = wt;
+ if (is_shared_symref(worktrees[i], symref, target)) {
+ existing = worktrees[i];
break;
}
}
diff --git a/worktree.h b/worktree.h
index 9dcea6fc8c..7889c4761d 100644
--- a/worktree.h
+++ b/worktree.h
@@ -149,6 +149,12 @@ const struct worktree *find_shared_symref(struct worktree **worktrees,
const char *symref,
const char *target);
+/*
+ * Returns true if a symref points to a ref in a worktree.
+ */
+int is_shared_symref(const struct worktree *wt,
+ const char *symref, const char *target);
+
/*
* Similar to head_ref() for all HEADs _except_ one from the current
* worktree, which is covered by head_ref().
--
2.39.0
^ permalink raw reply related
* [PATCH v2 0/3] fix die_if_checked_out() when ignore_current_worktree
From: Rubén Justo @ 2023-01-22 1:20 UTC (permalink / raw)
To: Git List; +Cc: Junio C Hamano, Phillip Wood
In-Reply-To: <eeb0c778-af0a-235c-f009-bca3abafdb15@gmail.com>
Changes:
- The message explains the change as a bug fix.
- Introduced a new helper is_shared_symref() to be used from
find_shared_symref() and die_if_checked_out().
- Tests for rebase and switch.
Rubén Justo (3):
branch: fix die_if_checked_out() when ignore_current_worktree
rebase: refuse to switch to a branch already checked out elsewhere (test)
switch: reject if the branch is already checked out elsewhere (test)
branch.c | 16 +++++++++-----
t/t2060-switch.sh | 29 +++++++++++++++++++++++++
t/t3400-rebase.sh | 14 ++++++++++++
worktree.c | 54 +++++++++++++++++++++++++----------------------
worktree.h | 6 ++++++
5 files changed, 89 insertions(+), 30 deletions(-)
--
2.39.0
^ permalink raw reply
* Re: [PATCH v1 1/1] t0003: Call dd with portable blocksize
From: Junio C Hamano @ 2023-01-22 0:08 UTC (permalink / raw)
To: tboegi; +Cc: git
In-Reply-To: <20230121110505.21362-1-tboegi@web.de>
tboegi@web.de writes:
> From: Torsten Bögershausen <tboegi@web.de>
>
> The command `dd -bs=101M count=1` is not portable.
No need for '-'; the UI of dd was meant as a joke and deliberately
deviates from UNIX norm to use '-' as an option introducer.
> Use `bs=1048576 count=101`, which does the same, instead.
Thanks for catching this. It always is hard to catch these mistakes
made in code that was cooked behind embargo, as there aren't many
eyeballs on the changes.
Strictly speaking, "bs=1048576 count=101" does not do the same thing
(unlike the original that does a single write(2)system call of a
huge buffer, it issues 101 smaller write(2)).
It definitely is an improvement, independently from the portability
issues, to rewrite it like you did. Unnecessarily large an I/O
should be avoided.
Will queue. Thanks.
> Signed-off-by: Torsten Bögershausen <tboegi@web.de>
> ---
> t/t0003-attributes.sh | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/t/t0003-attributes.sh b/t/t0003-attributes.sh
> index d0284fe2d7..394a08e6d6 100755
> --- a/t/t0003-attributes.sh
> +++ b/t/t0003-attributes.sh
> @@ -400,7 +400,7 @@ test_expect_success 'large attributes line ignores trailing content in tree' '
>
> test_expect_success EXPENSIVE 'large attributes file ignored in tree' '
> test_when_finished "rm .gitattributes" &&
> - dd if=/dev/zero of=.gitattributes bs=101M count=1 2>/dev/null &&
> + dd if=/dev/zero of=.gitattributes bs=1048576 count=101 2>/dev/null &&
> git check-attr --all path >/dev/null 2>err &&
> echo "warning: ignoring overly large gitattributes file ${SQ}.gitattributes${SQ}" >expect &&
> test_cmp expect err
> @@ -428,7 +428,7 @@ test_expect_success 'large attributes line ignores trailing content in index' '
>
> test_expect_success EXPENSIVE 'large attributes file ignored in index' '
> test_when_finished "git update-index --remove .gitattributes" &&
> - blob=$(dd if=/dev/zero bs=101M count=1 2>/dev/null | git hash-object -w --stdin) &&
> + blob=$(dd if=/dev/zero bs=1048576 count=101 2>/dev/null | git hash-object -w --stdin) &&
> git update-index --add --cacheinfo 100644,$blob,.gitattributes &&
> git check-attr --cached --all path >/dev/null 2>err &&
> echo "warning: ignoring overly large gitattributes blob ${SQ}.gitattributes${SQ}" >expect &&
> --
> 2.39.1.254.g904d404274
^ permalink raw reply
* Re: [PATCH v2 1/2] rebase: remove completely useless -C option
From: Junio C Hamano @ 2023-01-22 0:02 UTC (permalink / raw)
To: Elijah Newren; +Cc: Elijah Newren via GitGitGadget, git, Derrick Stolee
In-Reply-To: <CABPp-BFLUf7htEMXOLGOTNhUP=K11TfKrXQuC-FO4W6mHsjWQg@mail.gmail.com>
Elijah Newren <newren@gmail.com> writes:
> I'd probably go for that, but since in my mind the plan is still to
> deprecate and remove the apply backend as you suggested at [1], I'm
> not particularly motivated to improve/extend options specific to the
> apply backend of rebase.
I still consider that deprecating and removing the "am|format-patch
pipeline" mode is a good longer term goal, but it seems we still
have some features that are only available with the mode and not the
other "sequence of cherry-pick" mode? We'd need to port that over
to the other backend before we can wean ourselves off of the apply
backend. Until then, ...
^ permalink raw reply
* Re: [PATCH] win32: fix thread usage for win32
From: Johannes Sixt @ 2023-01-21 22:53 UTC (permalink / raw)
To: Rose via GitGitGadget; +Cc: Seija Kijin, git
In-Reply-To: <pull.1440.git.git.1674334159116.gitgitgadget@gmail.com>
Am 21.01.23 um 21:49 schrieb Rose via GitGitGadget:
> From: Seija Kijin <doremylover123@gmail.com>
>
> Use pthread_exit instead of async_exit.
>
> This means we do not have
> to deal with Windows's implementation
> requiring an unsigned exit coded
> despite the POSIX exit code requiring
> a signed exit code.
>
> Use _beginthreadex instead of CreateThread
> since we use the Windows CRT.
>
> Finally, check for NULL handles, not "INVALID_HANDLE,"
> as _beginthreadex guarantees a valid handle in most cases
This explains *what* the patch does, but not *why*. You replace
CreateThread() in winansi.c, but what has this to do with async_exit and
why must it be changed?
Please take the time to explain this story more thoroughly.
>
> Signed-off-by: Seija Kijin <doremylover123@gmail.com>
> ---
> win32: fix thread usage for win32
>
> Use pthread_exit instead of async_exit.
>
> This means we do not have to deal with Windows's implementation
> requiring an unsigned exit coded despite the POSIX exit code requiring a
> signed exit code.
>
> Use _beginthreadex instead of CreateThread since we use the Windows CRT.
>
> Finally, check for NULL handles, not "INVALID_HANDLE," as _beginthreadex
> guarantees a valid handle in most cases
>
> Signed-off-by: Seija Kijin doremylover123@gmail.com
>
> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1440%2FAtariDreams%2FCreateThread-v1
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1440/AtariDreams/CreateThread-v1
> Pull-Request: https://github.com/git/git/pull/1440
>
> compat/mingw.c | 2 +-
> compat/winansi.c | 8 ++++----
> run-command.c | 33 ++++++++++++++-------------------
> 3 files changed, 19 insertions(+), 24 deletions(-)
>
> diff --git a/compat/mingw.c b/compat/mingw.c
> index af397e68a1d..c41d821b382 100644
> --- a/compat/mingw.c
> +++ b/compat/mingw.c
> @@ -2295,7 +2295,7 @@ static int start_timer_thread(void)
> timer_event = CreateEvent(NULL, FALSE, FALSE, NULL);
> if (timer_event) {
> timer_thread = (HANDLE) _beginthreadex(NULL, 0, ticktack, NULL, 0, NULL);
> - if (!timer_thread )
> + if (!timer_thread)
> return errno = ENOMEM,
> error("cannot start timer thread");
> } else
> diff --git a/compat/winansi.c b/compat/winansi.c
> index 3abe8dd5a27..be65b27bd75 100644
> --- a/compat/winansi.c
> +++ b/compat/winansi.c
> @@ -340,7 +340,7 @@ enum {
> TEXT = 0, ESCAPE = 033, BRACKET = '['
> };
>
> -static DWORD WINAPI console_thread(LPVOID unused)
> +static unsigned int WINAPI console_thread(LPVOID unused)
> {
> unsigned char buffer[BUFFER_SIZE];
> DWORD bytes;
> @@ -643,9 +643,9 @@ void winansi_init(void)
> die_lasterr("CreateFile for named pipe failed");
>
> /* start console spool thread on the pipe's read end */
> - hthread = CreateThread(NULL, 0, console_thread, NULL, 0, NULL);
> - if (hthread == INVALID_HANDLE_VALUE)
> - die_lasterr("CreateThread(console_thread) failed");
> + hthread = (HANDLE)_beginthreadex(NULL, 0, console_thread, NULL, 0, NULL);
> + if (!hthread)
> + die_lasterr("_beginthreadex(console_thread) failed");
>
> /* schedule cleanup routine */
> if (atexit(winansi_exit))
> diff --git a/run-command.c b/run-command.c
> index 50cc011654e..93fd0d22d4f 100644
> --- a/run-command.c
> +++ b/run-command.c
> @@ -1030,6 +1030,13 @@ static void *run_thread(void *data)
> return (void *)ret;
> }
>
> +int in_async(void)
> +{
> + if (!main_thread_set)
> + return 0; /* no asyncs started yet */
> + return !pthread_equal(main_thread, pthread_self());
> +}
> +
> static NORETURN void die_async(const char *err, va_list params)
> {
> report_fn die_message_fn = get_die_message_routine();
> @@ -1055,18 +1062,6 @@ static int async_die_is_recursing(void)
> return ret != NULL;
> }
>
> -int in_async(void)
> -{
> - if (!main_thread_set)
> - return 0; /* no asyncs started yet */
> - return !pthread_equal(main_thread, pthread_self());
> -}
> -
> -static void NORETURN async_exit(int code)
> -{
> - pthread_exit((void *)(intptr_t)code);
> -}
> -
> #else
>
> static struct {
> @@ -1112,18 +1107,18 @@ int in_async(void)
> return process_is_async;
> }
>
> -static void NORETURN async_exit(int code)
> -{
> - exit(code);
> -}
> -
> #endif
>
> void check_pipe(int err)
> {
> if (err == EPIPE) {
> - if (in_async())
> - async_exit(141);
> + if (in_async()) {
> +#ifdef NO_PTHREADS
> + exit(141);
> +#else
> + pthread_exit((void *)141);
> +#endif
> + }
>
> signal(SIGPIPE, SIG_DFL);
> raise(SIGPIPE);
>
> base-commit: 904d404274fef6695c78a6b055edd184b72e2f9b
^ permalink raw reply
* [PATCH] win32: fix thread usage for win32
From: Rose via GitGitGadget @ 2023-01-21 20:49 UTC (permalink / raw)
To: git; +Cc: Rose, Seija Kijin
From: Seija Kijin <doremylover123@gmail.com>
Use pthread_exit instead of async_exit.
This means we do not have
to deal with Windows's implementation
requiring an unsigned exit coded
despite the POSIX exit code requiring
a signed exit code.
Use _beginthreadex instead of CreateThread
since we use the Windows CRT.
Finally, check for NULL handles, not "INVALID_HANDLE,"
as _beginthreadex guarantees a valid handle in most cases
Signed-off-by: Seija Kijin <doremylover123@gmail.com>
---
win32: fix thread usage for win32
Use pthread_exit instead of async_exit.
This means we do not have to deal with Windows's implementation
requiring an unsigned exit coded despite the POSIX exit code requiring a
signed exit code.
Use _beginthreadex instead of CreateThread since we use the Windows CRT.
Finally, check for NULL handles, not "INVALID_HANDLE," as _beginthreadex
guarantees a valid handle in most cases
Signed-off-by: Seija Kijin doremylover123@gmail.com
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1440%2FAtariDreams%2FCreateThread-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1440/AtariDreams/CreateThread-v1
Pull-Request: https://github.com/git/git/pull/1440
compat/mingw.c | 2 +-
compat/winansi.c | 8 ++++----
run-command.c | 33 ++++++++++++++-------------------
3 files changed, 19 insertions(+), 24 deletions(-)
diff --git a/compat/mingw.c b/compat/mingw.c
index af397e68a1d..c41d821b382 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -2295,7 +2295,7 @@ static int start_timer_thread(void)
timer_event = CreateEvent(NULL, FALSE, FALSE, NULL);
if (timer_event) {
timer_thread = (HANDLE) _beginthreadex(NULL, 0, ticktack, NULL, 0, NULL);
- if (!timer_thread )
+ if (!timer_thread)
return errno = ENOMEM,
error("cannot start timer thread");
} else
diff --git a/compat/winansi.c b/compat/winansi.c
index 3abe8dd5a27..be65b27bd75 100644
--- a/compat/winansi.c
+++ b/compat/winansi.c
@@ -340,7 +340,7 @@ enum {
TEXT = 0, ESCAPE = 033, BRACKET = '['
};
-static DWORD WINAPI console_thread(LPVOID unused)
+static unsigned int WINAPI console_thread(LPVOID unused)
{
unsigned char buffer[BUFFER_SIZE];
DWORD bytes;
@@ -643,9 +643,9 @@ void winansi_init(void)
die_lasterr("CreateFile for named pipe failed");
/* start console spool thread on the pipe's read end */
- hthread = CreateThread(NULL, 0, console_thread, NULL, 0, NULL);
- if (hthread == INVALID_HANDLE_VALUE)
- die_lasterr("CreateThread(console_thread) failed");
+ hthread = (HANDLE)_beginthreadex(NULL, 0, console_thread, NULL, 0, NULL);
+ if (!hthread)
+ die_lasterr("_beginthreadex(console_thread) failed");
/* schedule cleanup routine */
if (atexit(winansi_exit))
diff --git a/run-command.c b/run-command.c
index 50cc011654e..93fd0d22d4f 100644
--- a/run-command.c
+++ b/run-command.c
@@ -1030,6 +1030,13 @@ static void *run_thread(void *data)
return (void *)ret;
}
+int in_async(void)
+{
+ if (!main_thread_set)
+ return 0; /* no asyncs started yet */
+ return !pthread_equal(main_thread, pthread_self());
+}
+
static NORETURN void die_async(const char *err, va_list params)
{
report_fn die_message_fn = get_die_message_routine();
@@ -1055,18 +1062,6 @@ static int async_die_is_recursing(void)
return ret != NULL;
}
-int in_async(void)
-{
- if (!main_thread_set)
- return 0; /* no asyncs started yet */
- return !pthread_equal(main_thread, pthread_self());
-}
-
-static void NORETURN async_exit(int code)
-{
- pthread_exit((void *)(intptr_t)code);
-}
-
#else
static struct {
@@ -1112,18 +1107,18 @@ int in_async(void)
return process_is_async;
}
-static void NORETURN async_exit(int code)
-{
- exit(code);
-}
-
#endif
void check_pipe(int err)
{
if (err == EPIPE) {
- if (in_async())
- async_exit(141);
+ if (in_async()) {
+#ifdef NO_PTHREADS
+ exit(141);
+#else
+ pthread_exit((void *)141);
+#endif
+ }
signal(SIGPIPE, SIG_DFL);
raise(SIGPIPE);
base-commit: 904d404274fef6695c78a6b055edd184b72e2f9b
--
gitgitgadget
^ permalink raw reply related
* [PATCH v4] mingw: prefer RtlGetVersion over GetVersion
From: Rose via GitGitGadget @ 2023-01-21 20:04 UTC (permalink / raw)
To: git; +Cc: Rose, Seija Kijin
In-Reply-To: <pull.1438.v3.git.git.1674330625069.gitgitgadget@gmail.com>
From: Seija Kijin <doremylover123@gmail.com>
The previous way is deprecated and returns
the wrong value in Windows 8 and up,
returning the manifest Windows data
as opposed to the actual Windows data.
RtlGetVersion is the correct way
to get the Windows version now.
Note: ntdll does not need to be
manually loaded into the runtime,
as this is the one special library
that is automatically loaded upon launch.
Signed-off-by: Seija Kijin <doremylover123@gmail.com>
---
mingw: prefer RtlGetVersion over GetVersion
GetVersion has its behavior changed in Windows 8 and above anyway, so
this is the right way to do it now.
The previous way returns the wrong value in Windows 8 and up, returning
the manifest Windows data as opposed to the actual Windows data.
Signed-off-by: Seija Kijin doremylover123@gmail.com
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1438%2FAtariDreams%2Fmingw-v4
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1438/AtariDreams/mingw-v4
Pull-Request: https://github.com/git/git/pull/1438
Range-diff vs v3:
1: 31f778a6b34 ! 1: 8293e868970 mingw: replace deprecated GetVersion with RtlGetVersion
@@ Metadata
Author: Seija Kijin <doremylover123@gmail.com>
## Commit message ##
- mingw: replace deprecated GetVersion with RtlGetVersion
+ mingw: prefer RtlGetVersion over GetVersion
The previous way is deprecated and returns
the wrong value in Windows 8 and up,
compat/mingw.c | 39 +++++++++++++++++++++++++++++++++------
1 file changed, 33 insertions(+), 6 deletions(-)
diff --git a/compat/mingw.c b/compat/mingw.c
index af397e68a1d..b1d75c93cfe 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -3081,15 +3081,42 @@ int wmain(int argc, const wchar_t **wargv)
return exit_status;
}
+/*
+ * for RtlGetVersion in uname
+ */
+
+typedef NTSTATUS(WINAPI *RtlGetVersionPtr)(PRTL_OSVERSIONINFOW);
+union winprocaddr {
+ FARPROC procaddr;
+ RtlGetVersionPtr procGetVersion;
+};
+
int uname(struct utsname *buf)
{
- unsigned v = (unsigned)GetVersion();
+ union winprocaddr RtlGetVersionInternal;
+ OSVERSIONINFOW version;
+
+ memset(&version, 0, sizeof(version));
+ version.dwOSVersionInfoSize = sizeof(version);
+
+ /* RtlGetVersion always gets the true Windows version, even when running
+ * under Windows's compatibility mode*/
+ RtlGetVersionInternal.procaddr =
+ GetProcAddress(GetModuleHandleW(L"ntdll.dll"), "RtlGetVersion");
+
+ if (RtlGetVersionInternal.procaddr) {
+ RtlGetVersionInternal.procGetVersion((PRTL_OSVERSIONINFOW)&version);
+ } else {
+ /* Should not happen, but just in case, fallback to deprecated
+ * GetVersionExW */
+ GetVersionExW(&version);
+ }
+
memset(buf, 0, sizeof(*buf));
xsnprintf(buf->sysname, sizeof(buf->sysname), "Windows");
- xsnprintf(buf->release, sizeof(buf->release),
- "%u.%u", v & 0xff, (v >> 8) & 0xff);
- /* assuming NT variants only.. */
- xsnprintf(buf->version, sizeof(buf->version),
- "%u", (v >> 16) & 0x7fff);
+ xsnprintf(buf->release, sizeof(buf->release), "%lu.%lu",
+ version.dwMajorVersion, version.dwMinorVersion);
+ xsnprintf(buf->version, sizeof(buf->version), "%lu",
+ version.dwBuildNumber);
return 0;
}
base-commit: 904d404274fef6695c78a6b055edd184b72e2f9b
--
gitgitgadget
^ permalink raw reply related
* [PATCH v3] mingw: replace deprecated GetVersion with RtlGetVersion
From: Rose via GitGitGadget @ 2023-01-21 19:50 UTC (permalink / raw)
To: git; +Cc: Rose, Seija Kijin
In-Reply-To: <pull.1438.v2.git.git.1674149773693.gitgitgadget@gmail.com>
From: Seija Kijin <doremylover123@gmail.com>
The previous way is deprecated and returns
the wrong value in Windows 8 and up,
returning the manifest Windows data
as opposed to the actual Windows data.
RtlGetVersion is the correct way
to get the Windows version now.
Note: ntdll does not need to be
manually loaded into the runtime,
as this is the one special library
that is automatically loaded upon launch.
Signed-off-by: Seija Kijin <doremylover123@gmail.com>
---
mingw: replace deprecated GetVersion with RtlGetVersion
GetVersion has its behavior changed in Windows 8 and above anyway, so
this is the right way to do it now.
The previous way returns the wrong value in Windows 8 and up, returning
the manifest Windows data as opposed to the actual Windows data.
Signed-off-by: Seija Kijin doremylover123@gmail.com
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1438%2FAtariDreams%2Fmingw-v3
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1438/AtariDreams/mingw-v3
Pull-Request: https://github.com/git/git/pull/1438
Range-diff vs v2:
1: e5457905028 ! 1: 31f778a6b34 mingw: replace deprecated GetVersion with RtlGetVersion
@@ Commit message
returning the manifest Windows data
as opposed to the actual Windows data.
- RtlGetVersion is the correct way to get the Windows version now.
+ RtlGetVersion is the correct way
+ to get the Windows version now.
+
+ Note: ntdll does not need to be
+ manually loaded into the runtime,
+ as this is the one special library
+ that is automatically loaded upon launch.
Signed-off-by: Seija Kijin <doremylover123@gmail.com>
@@ compat/mingw.c: int wmain(int argc, const wchar_t **wargv)
{
- unsigned v = (unsigned)GetVersion();
+ union winprocaddr RtlGetVersionInternal;
-+ OSVERSIONINFOA version;
++ OSVERSIONINFOW version;
++
++ memset(&version, 0, sizeof(version));
++ version.dwOSVersionInfoSize = sizeof(version);
+
++ /* RtlGetVersion always gets the true Windows version, even when running
++ * under Windows's compatibility mode*/
+ RtlGetVersionInternal.procaddr =
+ GetProcAddress(GetModuleHandleW(L"ntdll.dll"), "RtlGetVersion");
-+ if (!RtlGetVersionInternal.procaddr) {
-+ /* if this is reached, something is seriously, seriously wrong
-+ */
-+ perror("Could not call RtlGetVersion in ntdll.dll");
-+ abort();
-+ }
+
-+ version.dwOSVersionInfoSize = sizeof(OSVERSIONINFO);
-+ RtlGetVersionInternal.procGetVersion((PRTL_OSVERSIONINFOW)&version);
++ if (RtlGetVersionInternal.procaddr) {
++ RtlGetVersionInternal.procGetVersion((PRTL_OSVERSIONINFOW)&version);
++ } else {
++ /* Should not happen, but just in case, fallback to deprecated
++ * GetVersionExW */
++ GetVersionExW(&version);
++ }
+
memset(buf, 0, sizeof(*buf));
xsnprintf(buf->sysname, sizeof(buf->sysname), "Windows");
compat/mingw.c | 39 +++++++++++++++++++++++++++++++++------
1 file changed, 33 insertions(+), 6 deletions(-)
diff --git a/compat/mingw.c b/compat/mingw.c
index af397e68a1d..b1d75c93cfe 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -3081,15 +3081,42 @@ int wmain(int argc, const wchar_t **wargv)
return exit_status;
}
+/*
+ * for RtlGetVersion in uname
+ */
+
+typedef NTSTATUS(WINAPI *RtlGetVersionPtr)(PRTL_OSVERSIONINFOW);
+union winprocaddr {
+ FARPROC procaddr;
+ RtlGetVersionPtr procGetVersion;
+};
+
int uname(struct utsname *buf)
{
- unsigned v = (unsigned)GetVersion();
+ union winprocaddr RtlGetVersionInternal;
+ OSVERSIONINFOW version;
+
+ memset(&version, 0, sizeof(version));
+ version.dwOSVersionInfoSize = sizeof(version);
+
+ /* RtlGetVersion always gets the true Windows version, even when running
+ * under Windows's compatibility mode*/
+ RtlGetVersionInternal.procaddr =
+ GetProcAddress(GetModuleHandleW(L"ntdll.dll"), "RtlGetVersion");
+
+ if (RtlGetVersionInternal.procaddr) {
+ RtlGetVersionInternal.procGetVersion((PRTL_OSVERSIONINFOW)&version);
+ } else {
+ /* Should not happen, but just in case, fallback to deprecated
+ * GetVersionExW */
+ GetVersionExW(&version);
+ }
+
memset(buf, 0, sizeof(*buf));
xsnprintf(buf->sysname, sizeof(buf->sysname), "Windows");
- xsnprintf(buf->release, sizeof(buf->release),
- "%u.%u", v & 0xff, (v >> 8) & 0xff);
- /* assuming NT variants only.. */
- xsnprintf(buf->version, sizeof(buf->version),
- "%u", (v >> 16) & 0x7fff);
+ xsnprintf(buf->release, sizeof(buf->release), "%lu.%lu",
+ version.dwMajorVersion, version.dwMinorVersion);
+ xsnprintf(buf->version, sizeof(buf->version), "%lu",
+ version.dwBuildNumber);
return 0;
}
base-commit: 904d404274fef6695c78a6b055edd184b72e2f9b
--
gitgitgadget
^ permalink raw reply related
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