* [PATCH v5 05/10] rebase: fix incompatiblity checks for --[no-]reapply-cherry-picks
From: Elijah Newren via GitGitGadget @ 2023-01-25 4:03 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.v5.git.1674619434.gitgitgadget@gmail.com>
From: Elijah Newren <newren@gmail.com>
--[no-]reapply-cherry-picks was traditionally only supported by the
sequencer. Support was added for the apply backend, when --keep-base is
also specified, in commit ce5238a690 ("rebase --keep-base: imply
--reapply-cherry-picks", 2022-10-17). Make the code error out when
--[no-]reapply-cherry-picks is specified AND the apply backend is used
AND --keep-base is not specified. Also, clarify a number of comments
surrounding the interaction of these flags.
Helped-by: Phillip Wood <phillip.wood@dunelm.org.uk>
Signed-off-by: Elijah Newren <newren@gmail.com>
---
Documentation/git-rebase.txt | 2 +-
builtin/rebase.c | 34 ++++++++++++++++----------
t/t3422-rebase-incompatible-options.sh | 10 ++++++++
3 files changed, 32 insertions(+), 14 deletions(-)
diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
index 846aeed1b69..8a09f12d897 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 when used without --keep-base
* --edit-todo
* --update-refs
* --root when used without --onto
diff --git a/builtin/rebase.c b/builtin/rebase.c
index b742cc8fb5c..05b130bfae5 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -1224,13 +1224,6 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
if (options.fork_point < 0)
options.fork_point = 0;
}
- /*
- * --keep-base defaults to --reapply-cherry-picks to avoid losing
- * commits when using this option.
- */
- if (options.reapply_cherry_picks < 0)
- options.reapply_cherry_picks = keep_base;
-
if (options.root && options.fork_point > 0)
die(_("options '%s' and '%s' cannot be used together"), "--root", "--fork-point");
@@ -1406,12 +1399,27 @@ 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 (options.reapply_cherry_picks < 0)
+ /*
+ * We default to --no-reapply-cherry-picks unless
+ * --keep-base is given; when --keep-base is given, we want
+ * to default to --reapply-cherry-picks.
+ */
+ options.reapply_cherry_picks = keep_base;
+ else if (!keep_base)
+ /*
+ * The apply backend always searches for and drops cherry
+ * picks. This is often not wanted with --keep-base, so
+ * --keep-base allows --reapply-cherry-picks to be
+ * simulated by altering the upstream such that
+ * cherry-picks cannot be detected and thus all commits are
+ * reapplied. Thus, --[no-]reapply-cherry-picks is
+ * supported when --keep-base is specified, but not when
+ * --keep-base is left out.
+ */
+ imply_merge(&options, options.reapply_cherry_picks ?
+ "--reapply-cherry-picks" :
+ "--no-reapply-cherry-picks");
if (gpg_sign)
options.gpg_sign_opt = xstrfmt("-S%s", gpg_sign);
diff --git a/t/t3422-rebase-incompatible-options.sh b/t/t3422-rebase-incompatible-options.sh
index f86274990b0..826f3b94ae6 100755
--- a/t/t3422-rebase-incompatible-options.sh
+++ b/t/t3422-rebase-incompatible-options.sh
@@ -60,6 +60,16 @@ test_rebase_am_only () {
test_must_fail git rebase $opt --exec 'true' 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 v5 08/10] rebase: fix formatting of rebase --reapply-cherry-picks option in docs
From: Elijah Newren via GitGitGadget @ 2023-01-25 4:03 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.v5.git.1674619434.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 d2b731bd605..99aadac28ef 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 v5 06/10] rebase: add coverage of other incompatible options
From: Elijah Newren via GitGitGadget @ 2023-01-25 4:03 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.v5.git.1674619434.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 one 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>
---
builtin/rebase.c | 3 +++
t/t3422-rebase-incompatible-options.sh | 15 +++++++++++++++
2 files changed, 18 insertions(+)
diff --git a/builtin/rebase.c b/builtin/rebase.c
index 05b130bfae5..d6b20a6a536 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -1511,6 +1511,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 826f3b94ae6..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,16 @@ 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
--
gitgitgadget
^ permalink raw reply related
* [PATCH v5 07/10] rebase: clarify the OPT_CMDMODE incompatibilities
From: Elijah Newren via GitGitGadget @ 2023-01-25 4:03 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.v5.git.1674619434.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 8a09f12d897..d2b731bd605 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 when used without --keep-base
- * --edit-todo
* --update-refs
* --root when used without --onto
--
gitgitgadget
^ permalink raw reply related
* [PATCH v5 10/10] rebase: provide better error message for apply options vs. merge config
From: Elijah Newren via GitGitGadget @ 2023-01-25 4:03 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.v5.git.1674619434.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 99aadac28ef..9a295bcee45 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 ad5ebecbbdb..7171be40eeb 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;
}
@@ -1366,7 +1372,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)
@@ -1499,20 +1506,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 v5 09/10] rebase: put rebase_options initialization in single place
From: Elijah Newren via GitGitGadget @ 2023-01-25 4:03 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.v5.git.1674619434.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 d6b20a6a536..ad5ebecbbdb 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
* Re: [PATCH v2] ls-tree: add --sparse-filter-oid argument
From: Elijah Newren @ 2023-01-25 5:11 UTC (permalink / raw)
To: William Sprent via GitGitGadget
Cc: git, Eric Sunshine, Ævar Arnfjörð Bjarmason,
Derrick Stolee, Victoria Dye, William Sprent
In-Reply-To: <pull.1459.v2.git.1674474371817.gitgitgadget@gmail.com>
It looks like Ævar and Victoria have both given really good reviews
already, but I think I spotted some additional things to comment on.
On Mon, Jan 23, 2023 at 3:46 AM William Sprent via GitGitGadget
<gitgitgadget@gmail.com> wrote:
>
> From: William Sprent <williams@unity3d.com>
>
> There is currently no way to ask git the question "which files would be
> part of a sparse checkout of commit X with sparse checkout patterns Y".
> One use-case would be that tooling may want know whether sparse checkouts
> of two commits contain the same content even if the full trees differ.
Could you say more about this usecase? Why does tooling need or want
to know this; won't a checkout of the new commit end up being quick
and simple? (I'm not saying your usecase is bad, just curious that it
had never occurred to me, and I'm afraid I'm still not sure what your
purpose might be.)
> Another interesting use-case would be for tooling to use in conjunction
> with 'git update-index --index-info'.
Sorry, I'm not following. Could you expound on this a bit?
> 'rev-list --objects --filter=sparse:oid' comes close, but as rev-list is
> concerned with objects rather than directory trees, it leaves files out
> when the same blob occurs in at two different paths.
s/in at/at/ ?
> It is possible to ask git about the sparse status of files currently in
> the index with 'ls-files -t'. However, this does not work well when the
> caller is interested in another commit, intererested in sparsity
s/intererested/interested/
> patterns that aren't currently in '.git/info/sparse-checkout', or when
> working in with bare repo.
s/in with bare/with a bare/ or s/in with bare/in a bare/?
> To fill this gap, add a new argument to ls-tree '--sparse-filter-oid'
> which takes the object id of a blob containing sparse checkout patterns
> that filters the output of 'ls-tree'. When filtering with given sparsity
> patterns, 'ls-tree' only outputs blobs and commit objects that
> match the given patterns.
This seems slightly unfortunate in that it makes things difficult for
cone mode users to take advantage of. They will have to figure out
how to translate their directory list into sparse checkout patterns
before passing it along, and currently the only way to do that is via
`git sparse-checkout set <patterns>` and reading the patterns from
$GIT_DIR/info/sparse-checkout, but that toggles the sparsity of the
current working tree and avoiding changing the current sparse-checkout
was something you listed in your commit message as something you
wanted to avoid.
> While it may be valid in some situations to output a tree object -- e.g.
> when a cone pattern matches all blobs recursively contained in a tree --
> it is less unclear what should be output if a sparse pattern matches
> parts of a tree.
>
> To allow for reusing the pattern matching logic found in
> 'path_in_sparse_checkout_1()' in 'dir.c' with arbitrary patterns,
> extract the pattern matching part of the function into its own new
> function 'recursively_match_path_with_sparse_patterns()'.
>
> Signed-off-by: William Sprent <williams@unity3d.com>
> ---
> ls-tree: add --sparse-filter-oid argument
>
> I'm resubmitting this change as rebased on top of 'master', as it
> conflicted with the topic 'ls-tree.c: clean-up works' 1
> [https://public-inbox.org/git/20230112091135.20050-1-tenglong.tl@alibaba-inc.com],
> which was merged to 'master' recently.
>
> This versions also incorporates changes based on the comments made in 2
> [https://public-inbox.org/git/CAPig+cRgZ0CrkqY7mufuWmhf6BC8yXjXXuOTEQjuz+Y0NA+N7Q@mail.gmail.com/].
>
> I'm also looping in contributors that have touched ls-tree and/or
> sparse-checkouts recently. I hope that's okay.
Of course! It's encouraged, even.
[...]
> diff --git a/builtin/ls-tree.c b/builtin/ls-tree.c
[...]
> +static void init_sparse_filter_data(struct sparse_filter_data **d, struct ls_tree_options *options,
> + const char *sparse_oid_name, read_tree_fn_t fn)
> +{
> + struct object_id sparse_oid;
> + struct object_context oc;
> +
> + (*d) = xcalloc(1, sizeof(**d));
> + (*d)->fn = fn;
> + (*d)->pl.use_cone_patterns = core_sparse_checkout_cone;
Hmm, so the behavior still depends upon the current sparse-checkout
(or lack thereof), despite the documentation and rationale of your
feature as being there to check how a different sparse checkout would
behave?
I would hate to unconditionally turn cone_patterns off, since that
would come with a huge performance penalty for the biggest repos. But
turning it unconditionally on wouldn't be good for the non-cone users.
This probably suggests we need something like another flag, or perhaps
separate flags for each mode. Separate flags might provide the
benefit of allowing cone mode users to specify directories rather than
patterns, which would make it much easier for them to use.
[...]
> +static int path_matches_sparse_checkout_patterns(struct strbuf *full_path, struct pattern_list *pl, int dtype)
> +{
> + enum pattern_match_result match = recursively_match_path_with_sparse_patterns(full_path->buf, the_repository->index, dtype, pl);
> + return match > 0;
So your new caller doesn't care about the pattern_match_result, it
just wants to know if it got MATCHED or MATCHED_RECURSIVELY...
[...]
> diff --git a/dir.c b/dir.c
> index 4e99f0c868f..122ebced08e 100644
> --- a/dir.c
> +++ b/dir.c
> @@ -1457,45 +1457,50 @@ int init_sparse_checkout_patterns(struct index_state *istate)
> return 0;
> }
>
> -static int path_in_sparse_checkout_1(const char *path,
> - struct index_state *istate,
> - int require_cone_mode)
> +int recursively_match_path_with_sparse_patterns(const char *path,
You claim it returns an int here, but previously you presumed an enum
pattern_match_result from the new caller.
> + struct index_state *istate,
> + int dtype,
> + struct pattern_list *pl)
> {
> - int dtype = DT_REG;
> enum pattern_match_result match = UNDECIDED;
> const char *end, *slash;
> -
> - /*
> - * We default to accepting a path if the path is empty, there are no
> - * patterns, or the patterns are of the wrong type.
> - */
> - if (!*path ||
> - init_sparse_checkout_patterns(istate) ||
> - (require_cone_mode &&
> - !istate->sparse_checkout_patterns->use_cone_patterns))
> - return 1;
> -
> /*
> * If UNDECIDED, use the match from the parent dir (recursively), or
> * fall back to NOT_MATCHED at the topmost level. Note that cone mode
> * never returns UNDECIDED, so we will execute only one iteration in
> * this case.
> */
> - for (end = path + strlen(path);
> - end > path && match == UNDECIDED;
> + for (end = path + strlen(path); end > path && match == UNDECIDED;
> end = slash) {
> -
> for (slash = end - 1; slash > path && *slash != '/'; slash--)
> ; /* do nothing */
>
> match = path_matches_pattern_list(path, end - path,
> slash > path ? slash + 1 : path, &dtype,
> - istate->sparse_checkout_patterns, istate);
> + pl, istate);
>
> /* We are going to match the parent dir now */
> dtype = DT_DIR;
> }
> - return match > 0;
> +
> + return match;
Um, this last line seems like a potentially scary change in behavior.
Why should UNDECIDED return a non-zero value? Previously, we returned
a 0 value for both UNDECIDED and NOT_MATCHED, but you've changed that
here. If the change in this last line is actually correct, it should
be split out into its own commit and explained in detail in the commit
message.
> +}
> +
> +static int path_in_sparse_checkout_1(const char *path,
> + struct index_state *istate,
> + int require_cone_mode)
> +{
> + /*
> + * We default to accepting a path if the path is empty, there are no
> + * patterns, or the patterns are of the wrong type.
> + */
> + if (!*path ||
> + init_sparse_checkout_patterns(istate) ||
> + (require_cone_mode &&
> + !istate->sparse_checkout_patterns->use_cone_patterns))
> + return 1;
> +
> + return recursively_match_path_with_sparse_patterns(path, istate, DT_REG, istate->sparse_checkout_patterns) > 0;
Oh, you compare to > 0 here...and digging around your only other
caller just immediately compares to > 0 as well.
Why not just have recursively_match_path_with_sparse_patterns() do the
> 0 check? If it does, returning int is fine. If it doesn't, it
should be declared to return enum pattern_match_result.
^ permalink raw reply
* [PATCH v3] ssh signing: better error message when key not in agent
From: Adam Szkoda via GitGitGadget @ 2023-01-25 12:40 UTC (permalink / raw)
To: git; +Cc: Phillip Wood, Adam Szkoda, Fabian Stelzer, Adam Szkoda,
Adam Szkoda
In-Reply-To: <pull.1270.v2.git.git.1674573972087.gitgitgadget@gmail.com>
From: Adam Szkoda <adaszko@gmail.com>
When signing a commit with a SSH key, with the private key missing from
ssh-agent, a confusing error message is produced:
error: Load key
"/var/folders/t5/cscwwl_n3n1_8_5j_00x_3t40000gn/T//.git_signing_key_tmpkArSj7":
invalid format? fatal: failed to write commit object
The temporary file .git_signing_key_tmpkArSj7 created by git contains a
valid *public* key. The error message comes from `ssh-keygen -Y sign' and
is caused by a fallback mechanism in ssh-keygen whereby it tries to
interpret .git_signing_key_tmpkArSj7 as a *private* key if it can't find in
the agent [1]. A fix is scheduled to be released in OpenSSH 9.1. All that
needs to be done is to pass an additional backward-compatible option -U to
'ssh-keygen -Y sign' call. With '-U', ssh-keygen always interprets the file
as public key and expects to find the private key in the agent.
As a result, when the private key is missing from the agent, a more accurate
error message gets produced:
error: Couldn't find key in agent
[1] https://bugzilla.mindrot.org/show_bug.cgi?id=3429
Signed-off-by: Adam Szkoda <adaszko@gmail.com>
---
ssh signing: better error message when key not in agent
When signing a commit with a SSH key, with the private key missing from
ssh-agent, a confusing error message is produced:
error: Load key "/var/folders/t5/cscwwl_n3n1_8_5j_00x_3t40000gn/T//.git_signing_key_tmpkArSj7": invalid format?
fatal: failed to write commit object
The temporary file .git_signing_key_tmpkArSj7 created by git contains a
valid public key. The error message comes from `ssh-keygen -Y sign' and
is caused by a fallback mechanism in ssh-keygen whereby it tries to
interpret .git_signing_key_tmpkArSj7 as a private key if it can't find
in the agent [1]. A fix is scheduled to be released in OpenSSH 9.1. All
that needs to be done is to pass an additional backward-compatible
option -U to 'ssh-keygen -Y sign' call. With '-U', ssh-keygen always
interprets the file as public key and expects to find the private key in
the agent.
As a result, when the private key is missing from the agent, a more
accurate error message gets produced:
error: Couldn't find key in agent
[1] https://bugzilla.mindrot.org/show_bug.cgi?id=3429
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1270%2Fradicle-dev%2Fmaint-v3
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1270/radicle-dev/maint-v3
Pull-Request: https://github.com/git/git/pull/1270
Range-diff vs v2:
1: 03dfca79387 ! 1: dc7acff3b95 ssh signing: better error message when key not in agent
@@ gpg-interface.c: static int sign_buffer_ssh(struct strbuf *buffer, struct strbuf
if (!key_file)
return error_errno(
@@ gpg-interface.c: static int sign_buffer_ssh(struct strbuf *buffer, struct strbuf *signature,
- }
-
- strvec_pushl(&signer.args, use_format->program,
-- "-Y", "sign",
-- "-n", "git",
-- "-f", ssh_signing_key_file,
+ "-Y", "sign",
+ "-n", "git",
+ "-f", ssh_signing_key_file,
- buffer_file->filename.buf,
-- NULL);
-+ "-Y", "sign",
-+ "-n", "git",
-+ "-f", ssh_signing_key_file,
-+ NULL);
-+ if (literal_ssh_key) {
+ NULL);
++ if (literal_ssh_key)
+ strvec_push(&signer.args, "-U");
-+ }
+ strvec_push(&signer.args, buffer_file->filename.buf);
sigchain_push(SIGPIPE, SIG_IGN);
gpg-interface.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/gpg-interface.c b/gpg-interface.c
index f877a1ea564..687236430bf 100644
--- a/gpg-interface.c
+++ b/gpg-interface.c
@@ -998,6 +998,7 @@ static int sign_buffer_ssh(struct strbuf *buffer, struct strbuf *signature,
char *ssh_signing_key_file = NULL;
struct strbuf ssh_signature_filename = STRBUF_INIT;
const char *literal_key = NULL;
+ int literal_ssh_key = 0;
if (!signing_key || signing_key[0] == '\0')
return error(
@@ -1005,6 +1006,7 @@ static int sign_buffer_ssh(struct strbuf *buffer, struct strbuf *signature,
if (is_literal_ssh_key(signing_key, &literal_key)) {
/* A literal ssh key */
+ literal_ssh_key = 1;
key_file = mks_tempfile_t(".git_signing_key_tmpXXXXXX");
if (!key_file)
return error_errno(
@@ -1039,8 +1041,10 @@ static int sign_buffer_ssh(struct strbuf *buffer, struct strbuf *signature,
"-Y", "sign",
"-n", "git",
"-f", ssh_signing_key_file,
- buffer_file->filename.buf,
NULL);
+ if (literal_ssh_key)
+ strvec_push(&signer.args, "-U");
+ strvec_push(&signer.args, buffer_file->filename.buf);
sigchain_push(SIGPIPE, SIG_IGN);
ret = pipe_command(&signer, NULL, 0, NULL, 0, &signer_stderr, 0);
base-commit: 844ede312b4e988881b6e27e352f469d8ab80b2a
--
gitgitgadget
^ permalink raw reply related
* Re: [PATCH v2] ssh signing: better error message when key not in agent
From: Adam Szkoda @ 2023-01-25 12:46 UTC (permalink / raw)
To: Junio C Hamano
Cc: Adam Szkoda via GitGitGadget, git, Phillip Wood, Fabian Stelzer
In-Reply-To: <xmqq1qnjhlbf.fsf@gitster.g>
On Tue, Jan 24, 2023 at 6:52 PM Junio C Hamano <gitster@pobox.com> wrote:
> > Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1270/radicle-dev/maint-v2
> > Pull-Request: https://github.com/git/git/pull/1270
> >
> > Range-diff vs v1:
> >
> > 1: 0ce06076242 < -: ----------- ssh signing: better error message when key not in agent
> > -: ----------- > 1: 03dfca79387 ssh signing: better error message when key not in agent
>
> This is a fairly useless range-diff.
>
> Even when a range-diff shows the differences in the patches,
> mechanically generated range-diff can only show _what_ changed. It
> is helpful to explain the changes in your own words to highlight
> _why_ such changes are done, and this place after the "---" line
> and the diffstat we see below is the place to do so.
>
> Does GitGitGadget allow its users to describe the differences since
> the previous iteration yourself?
No, I don't think it does. It got generated automatically without
giving me an opportunity to edit.
> > gpg-interface.c | 15 ++++++++++-----
> > 1 file changed, 10 insertions(+), 5 deletions(-)
> >
> > diff --git a/gpg-interface.c b/gpg-interface.c
> > index f877a1ea564..33899a450eb 100644
> > --- a/gpg-interface.c
> > +++ b/gpg-interface.c
> > @@ -998,6 +998,7 @@ static int sign_buffer_ssh(struct strbuf *buffer, struct strbuf *signature,
> > char *ssh_signing_key_file = NULL;
> > struct strbuf ssh_signature_filename = STRBUF_INIT;
> > const char *literal_key = NULL;
> > + int literal_ssh_key = 0;
> >
> > if (!signing_key || signing_key[0] == '\0')
> > return error(
> > @@ -1005,6 +1006,7 @@ static int sign_buffer_ssh(struct strbuf *buffer, struct strbuf *signature,
> >
> > if (is_literal_ssh_key(signing_key, &literal_key)) {
> > /* A literal ssh key */
> > + literal_ssh_key = 1;
> > key_file = mks_tempfile_t(".git_signing_key_tmpXXXXXX");
> > if (!key_file)
> > return error_errno(
> > @@ -1036,11 +1038,14 @@ static int sign_buffer_ssh(struct strbuf *buffer, struct strbuf *signature,
> > }
> >
> > strvec_pushl(&signer.args, use_format->program,
> > - "-Y", "sign",
> > - "-n", "git",
> > - "-f", ssh_signing_key_file,
> > - buffer_file->filename.buf,
> > - NULL);
> > + "-Y", "sign",
> > + "-n", "git",
> > + "-f", ssh_signing_key_file,
> > + NULL);
>
> Please avoid making a pointless indentation change like this.
Yep, removed. It was largely accidental.
> We do
> not pass filename yet with this pushl(), because ...
>
> > + if (literal_ssh_key) {
> > + strvec_push(&signer.args, "-U");
> > + }
>
> ... when we give a literal key, we want to insert "-U" in front, and then
>
> > + strvec_push(&signer.args, buffer_file->filename.buf);
>
> ... the filename. Which makes sense.
I'm not sure what you mean in this paragraph. If there's something
more that needs to be done, I'd appreciate it if you could rephrase
it.
> The insertion of "-U" is a single statement as the body of a if()
> statement. We do not want {} around it, by the way.
Removed the superfluous {}.
Thanks
— Adam
^ permalink raw reply
* Re: [PATCH v2] ls-tree: add --sparse-filter-oid argument
From: William Sprent @ 2023-01-25 13:47 UTC (permalink / raw)
To: Victoria Dye, William Sprent via GitGitGadget, git
Cc: Eric Sunshine, Ævar Arnfjörð Bjarmason,
Derrick Stolee, Elijah Newren
In-Reply-To: <7ccf7b17-4448-5ef4-63b1-9073a400e486@github.com>
On 24/01/2023 21.11, Victoria Dye wrote:
> William Sprent via GitGitGadget wrote:
>> From: William Sprent <williams@unity3d.com>
>>
>> There is currently no way to ask git the question "which files would be
>> part of a sparse checkout of commit X with sparse checkout patterns Y".
>> One use-case would be that tooling may want know whether sparse checkouts
>> of two commits contain the same content even if the full trees differ.
>> Another interesting use-case would be for tooling to use in conjunction
>> with 'git update-index --index-info'.
>
> These types of use cases align nicely with "Behavior A" as described in
> 'Documentation/technical/sparse-checkout.txt' [1]. I think adding a
> '--scope=(sparse|all)' option to 'ls-trees' would be a good way to make
> progress on the goals of that design document as well as serve the needs
> described above.
>
> [1] https://lore.kernel.org/git/pull.1367.v4.git.1667714666810.gitgitgadget@gmail.com/
> >>
>> 'rev-list --objects --filter=sparse:oid' comes close, but as rev-list is
>> concerned with objects rather than directory trees, it leaves files out
>> when the same blob occurs in at two different paths.
>>
>> It is possible to ask git about the sparse status of files currently in
>> the index with 'ls-files -t'. However, this does not work well when the
>> caller is interested in another commit, intererested in sparsity
>> patterns that aren't currently in '.git/info/sparse-checkout', or when
>> working in with bare repo.
>
> I'm a bit confused by your described use cases here, though. Right now,
> 'sparse-checkout' is a local repo-only optimization (for performance and/or
> scoping user workspaces), but you seem to be implying a need for
> "sparse-checkout patterns" as a general-purpose data format (like a config
> file or 'rebase-todo') that can be used to manually configure command
> behavior.
>
> If that is what you're getting at, it seems like a pretty substantial
> expansion to the scope of what we consider "sparse checkout". That's not to
> say it isn't a good idea - I can definitely imagine tooling where this type
> of functionality is useful - just that it warrants careful consideration so
> we don't over-complicate the typical sparse-checkout user experience.
>
I guess it is sort of what I'm getting at. I want to enable tooling (in
particular) to be able to interrogate git about sparse checkouts without
having to checkout a commit and load a set of patterns. There's already
a deterministic relationship between a (commit * sparse checkout
patterns) and a list of included files, so I think it makes sense to be
able to ask git about it.
I agree with your concerns about not over complicating things for the
average user. I think the cone/non-cone distinctions are already a bit
hard to grasp (at least it was for me). FWIW, it isn't my intention that
an average user should use the "explicitly pass patterns to ls-tree"
functionality. But of course we should still get it right.
>>
>> To fill this gap, add a new argument to ls-tree '--sparse-filter-oid'
>> which takes the object id of a blob containing sparse checkout patterns
>> that filters the output of 'ls-tree'. When filtering with given sparsity
>> patterns, 'ls-tree' only outputs blobs and commit objects that
>> match the given patterns.
>
> I don't think a blob OID is the best way to specify an arbitrary pattern set
> in this case. OIDs will only be created for blobs that are tracked in Git;
> it's possible that your project tracks potential sparse-checkout patterns in
> Git, but it seems overly restrictive. You could instead specify the file
> with a path on-disk (like the '--file' options in various commands, e.g.
> 'git commit'); if you did need to get that file from the object store, you
> could first get its contents with 'git cat-file'.
>
I'm fine with changing it to be a file. I picked it to align with
'git rev-list --filter=sparse:<blob-oid>'
which I think is the only other place in the code base where you can
query about arbitrary filters. But I agree, it is a bit awkward to use.
>>
>> While it may be valid in some situations to output a tree object -- e.g.
>> when a cone pattern matches all blobs recursively contained in a tree --
>> it is less unclear what should be output if a sparse pattern matches
>> parts of a tree.
>>
>> To allow for reusing the pattern matching logic found in
>> 'path_in_sparse_checkout_1()' in 'dir.c' with arbitrary patterns,
>> extract the pattern matching part of the function into its own new
>> function 'recursively_match_path_with_sparse_patterns()'.
>>
>> Signed-off-by: William Sprent <williams@unity3d.com>
>> ---
>> ls-tree: add --sparse-filter-oid argument
>>
>> I'm resubmitting this change as rebased on top of 'master', as it
>> conflicted with the topic 'ls-tree.c: clean-up works' 1
>> [https://public-inbox.org/git/20230112091135.20050-1-tenglong.tl@alibaba-inc.com],
>> which was merged to 'master' recently.
>>
>> This versions also incorporates changes based on the comments made in 2
>> [https://public-inbox.org/git/CAPig+cRgZ0CrkqY7mufuWmhf6BC8yXjXXuOTEQjuz+Y0NA+N7Q@mail.gmail.com/].
>>
>> I'm also looping in contributors that have touched ls-tree and/or
>> sparse-checkouts recently. I hope that's okay.
>
> Definitely okay, thanks for CC-ing!
>
> Overall, this is an interesting extension to 'sparse-checkout', and opens up
> some opportunities for expanded tooling. However, at an implementation
> level, I think it's addressing two separate needs ("constrain 'ls-files' to
> display files matching patterns" and "specify sparse patterns not in
> '.git/info/sparse-checkout'") in one option, which makes for a somewhat
> confusing and inflexible user experience. What about instead breaking this
> into two options:
>
> * --scope=(sparse|all): limits the scope of the files output by ("Behavior
> A" vs. "Behavior B" from the document linked earlier).
> * --sparse-patterns=<file>: specifies "override" patterns to use instead of
> those in '.git/info/sparse-checkout' (whether 'sparse-checkout' is enabled
> for the repository or not).
>
That makes sense to me. I'll look into making those changes.
> I haven't looked at your implementation in detail yet, but I did want to
> offer a recommendation in case you hadn't considered it: if you want to
> allow the use of patterns from a user-specified specific file, it would be
> nice to do it in a way that fully replaces the "default" sparse-checkout
> settings at the lowest level (i.e., override the values of
> 'core_apply_sparse_checkout', 'core_sparse_checkout_cone', and
> 'get_sparse_checkout_filename()'). Doing it that way would both make it
> easier for other commands to add a '--sparse-patterns' option, and avoid the
> separate code path ('path_in_sparse_checkout_1()' vs.
> 'recursively_match_path_with_sparse_patterns()', for example) when dealing
> with '.git/info/sparse-checkout' patterns vs. manually-specified patterns.
>
Thanks for the pointers. I'll see what I can do. Do you mean something
along the line of the following?
set_sparse_checkout_file(filename);
init_sparse_checkout_patterns(istate);
_ = path_in_sparse_checkout_1(some_path, istate, ...);
I do I think I like the explicitness of first loading the pattern_list
and then passing it to the matching function, though. But maybe it is
too cumbersome.
>>
>> William
>>
>> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1459%2Fwilliams-unity%2Fls-tree-sparse-v2
>> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1459/williams-unity/ls-tree-sparse-v2
>> Pull-Request: https://github.com/gitgitgadget/git/pull/1459
>
^ permalink raw reply
* Re: [PATCH v5 05/10] rebase: fix incompatiblity checks for --[no-]reapply-cherry-picks
From: Phillip Wood @ 2023-01-25 14:14 UTC (permalink / raw)
To: Elijah Newren via GitGitGadget, git
Cc: Derrick Stolee, Elijah Newren, Eric Sunshine, Martin Ågren,
Phillip Wood
In-Reply-To: <3a8429f3d2b72d59d0a071d83f6fd9effe6824ed.1674619434.git.gitgitgadget@gmail.com>
Hi Elijah
On 25/01/2023 04:03, Elijah Newren via GitGitGadget wrote:
> From: Elijah Newren <newren@gmail.com>
>
> --[no-]reapply-cherry-picks was traditionally only supported by the
> sequencer. Support was added for the apply backend, when --keep-base is
> also specified, in commit ce5238a690 ("rebase --keep-base: imply
> --reapply-cherry-picks", 2022-10-17). Make the code error out when
> --[no-]reapply-cherry-picks is specified AND the apply backend is used
> AND --keep-base is not specified. Also, clarify a number of comments
> surrounding the interaction of these flags.
This looks great to me. Sorry it took me so long to realize that the
apply backend should be erroring out on --no-reapply-cherry-picks
without --keep-base. Thanks for taking the time to update the comments,
hopefully they'll be more helpful to future readers that the current
ones have proved to be.
Best Wishes
Phillip
> Helped-by: Phillip Wood <phillip.wood@dunelm.org.uk>
> Signed-off-by: Elijah Newren <newren@gmail.com>
> ---
> Documentation/git-rebase.txt | 2 +-
> builtin/rebase.c | 34 ++++++++++++++++----------
> t/t3422-rebase-incompatible-options.sh | 10 ++++++++
> 3 files changed, 32 insertions(+), 14 deletions(-)
>
> diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
> index 846aeed1b69..8a09f12d897 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 when used without --keep-base
> * --edit-todo
> * --update-refs
> * --root when used without --onto
> diff --git a/builtin/rebase.c b/builtin/rebase.c
> index b742cc8fb5c..05b130bfae5 100644
> --- a/builtin/rebase.c
> +++ b/builtin/rebase.c
> @@ -1224,13 +1224,6 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
> if (options.fork_point < 0)
> options.fork_point = 0;
> }
> - /*
> - * --keep-base defaults to --reapply-cherry-picks to avoid losing
> - * commits when using this option.
> - */
> - if (options.reapply_cherry_picks < 0)
> - options.reapply_cherry_picks = keep_base;
> -
> if (options.root && options.fork_point > 0)
> die(_("options '%s' and '%s' cannot be used together"), "--root", "--fork-point");
>
> @@ -1406,12 +1399,27 @@ 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 (options.reapply_cherry_picks < 0)
> + /*
> + * We default to --no-reapply-cherry-picks unless
> + * --keep-base is given; when --keep-base is given, we want
> + * to default to --reapply-cherry-picks.
> + */
> + options.reapply_cherry_picks = keep_base;
> + else if (!keep_base)
> + /*
> + * The apply backend always searches for and drops cherry
> + * picks. This is often not wanted with --keep-base, so
> + * --keep-base allows --reapply-cherry-picks to be
> + * simulated by altering the upstream such that
> + * cherry-picks cannot be detected and thus all commits are
> + * reapplied. Thus, --[no-]reapply-cherry-picks is
> + * supported when --keep-base is specified, but not when
> + * --keep-base is left out.
> + */
> + imply_merge(&options, options.reapply_cherry_picks ?
> + "--reapply-cherry-picks" :
> + "--no-reapply-cherry-picks");
>
> if (gpg_sign)
> options.gpg_sign_opt = xstrfmt("-S%s", gpg_sign);
> diff --git a/t/t3422-rebase-incompatible-options.sh b/t/t3422-rebase-incompatible-options.sh
> index f86274990b0..826f3b94ae6 100755
> --- a/t/t3422-rebase-incompatible-options.sh
> +++ b/t/t3422-rebase-incompatible-options.sh
> @@ -60,6 +60,16 @@ test_rebase_am_only () {
> test_must_fail git rebase $opt --exec 'true' 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
^ permalink raw reply
* Re: [PATCH v5 00/10] rebase: fix several code/testing/documentation issues around flag incompatibilities
From: Phillip Wood @ 2023-01-25 14:17 UTC (permalink / raw)
To: Elijah Newren via GitGitGadget, git
Cc: Derrick Stolee, Elijah Newren, Eric Sunshine, Martin Ågren,
Phillip Wood
In-Reply-To: <pull.1466.v5.git.1674619434.gitgitgadget@gmail.com>
Hi Elijah
On 25/01/2023 04:03, Elijah Newren via GitGitGadget wrote:
> 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.
This version looks good to me. Thanks for working on it - it turned out
to be quite a bit of work in the end but it is nice improvement,
especially the last patch.
Best Wishes
Phillip
> Changes since v4:
>
> * Pulled out the changes regarding incompatibility detection for
> --[no-]reapply-cherry-picks into a separate patch. Phillip helped a lot
> with understanding the behavior, suggesting changes, and getting the
> wording right, and I think it deserves its own patch with its own
> explanation.
>
> 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 (10):
> 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: fix incompatiblity checks for --[no-]reapply-cherry-picks
> 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 | 79 +++++++++++++++++++-------
> t/t3422-rebase-incompatible-options.sh | 71 +++++++++++++++++++++--
> 3 files changed, 163 insertions(+), 64 deletions(-)
>
>
> base-commit: 56c8fb1e95377900ec9d53c07886022af0a5d3c2
> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1466%2Fnewren%2Frebase-update-refs-imply-merge-v5
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1466/newren/rebase-update-refs-imply-merge-v5
> Pull-Request: https://github.com/gitgitgadget/git/pull/1466
>
> Range-diff vs v4:
>
> 1: 8a676e6ec1a = 1: 8a676e6ec1a rebase: mark --update-refs as requiring the merge backend
> 2: cc129b87185 = 2: cc129b87185 rebase: flag --apply and --merge as incompatible
> 3: 9464bbbe9ba = 3: 9464bbbe9ba rebase: remove --allow-empty-message from incompatible opts
> 4: b702f15ed7c = 4: b702f15ed7c rebase: fix docs about incompatibilities with --root
> -: ----------- > 5: 3a8429f3d2b rebase: fix incompatiblity checks for --[no-]reapply-cherry-picks
> 5: 5e4851e611e ! 6: 2777dd2788a rebase: add coverage of other incompatible options
> @@ Commit message
>
> 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
> + code checks for one of these, which could result in command line
> options being silently ignored.
>
> Also, note that adding a check for autosquash means that using
> @@ Commit 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;
> - }
> -+ /*
> -+ * 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.
> -@@ builtin/rebase.c: 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);
> -
> @@ builtin/rebase.c: int cmd_rebase(int argc, const char **argv, const char *prefix)
> if (options.update_refs)
> imply_merge(&options, "--update-refs");
> @@ t/t3422-rebase-incompatible-options.sh: test_rebase_am_only () {
> + 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" "
> + test_expect_success "$opt incompatible with --no-reapply-cherry-picks" "
> git checkout B^0 &&
> - test_must_fail git rebase $opt --update-refs A
> + test_must_fail git rebase $opt --no-reapply-cherry-picks A
> 6: 21ae9e05313 ! 7: 0d0541ea243 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=
> - * --[no-]reapply-cherry-picks
> + * --[no-]reapply-cherry-picks when used without --keep-base
> - * --edit-todo
> * --update-refs
> * --root when used without --onto
> 7: 74a216bf0c0 = 8: 01808cf84a8 rebase: fix formatting of rebase --reapply-cherry-picks option in docs
> 8: a8adf7fda61 = 9: f646abee524 rebase: put rebase_options initialization in single place
> 9: 5cb00e5103b = 10: 328f5a1d534 rebase: provide better error message for apply options vs. merge config
>
^ permalink raw reply
* Re: A summary of in-flight topics
From: Elijah Newren @ 2023-01-25 16:05 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <xmqq7cxbftvu.fsf@gitster.g>
On Tue, Jan 24, 2023 at 2:37 PM Junio C Hamano <gitster@pobox.com> wrote:
[...]
> - ab/sequencer-unleak 01-18 #8
Perhaps move to "Expecting a hopefully minor and final reroll"? See
e.g. https://lore.kernel.org/git/xmqqedry17r4.fsf@gitster.g/ where you
commented on Phillip's approving the code changes but wanting a number
of commit message cleanups and fixes. Ævar did send a v3, and Phillip
commented on two of those patches, suggesting one could still use some
commit message cleanups.
> - ab/various-leak-fixes 01-18 #19
Isn't this already merged as 9ea1378d04 ("Merge branch
'ab/various-leak-fixes'", 2022-12-14) ? Why does it appear under
"needing review"? (I started reviewing, and while looking at the
existing code noted it already had the relevant changes, then went
digging and found it was merged...)
> Will merge to 'master'.
> + en/rebase-update-refs-needs-merge-backend 01-22/01-23 #9
I sent out a v5 since you sent this email, to include some final
changes Phillip was suggesting. He seems happy with those
(https://lore.kernel.org/git/94deff87-a4a0-e937-7549-3e5348361a12@dunelm.org.uk/),
so I agree it can merge down, but please use v5 instead of v4.
> Will merge to 'next'?
> - en/ls-files-doc-update 01-13 #4
I'm in favor, but self votes don't count so...
^ permalink raw reply
* Re: [PATCH v2] ls-tree: add --sparse-filter-oid argument
From: William Sprent @ 2023-01-25 16:16 UTC (permalink / raw)
To: Elijah Newren, William Sprent via GitGitGadget
Cc: git, Eric Sunshine, Ævar Arnfjörð Bjarmason,
Derrick Stolee, Victoria Dye
In-Reply-To: <CABPp-BExS8UGfGzT+w9R_p0sY+_=A0-nRzU5QTOKwfBSmX6c3A@mail.gmail.com>
On 25/01/2023 06.11, Elijah Newren wrote:
> It looks like Ævar and Victoria have both given really good reviews
> already, but I think I spotted some additional things to comment on.
>
> On Mon, Jan 23, 2023 at 3:46 AM William Sprent via GitGitGadget
> <gitgitgadget@gmail.com> wrote:
>>
>> From: William Sprent <williams@unity3d.com>
>>
>> There is currently no way to ask git the question "which files would be
>> part of a sparse checkout of commit X with sparse checkout patterns Y".
>> One use-case would be that tooling may want know whether sparse checkouts
>> of two commits contain the same content even if the full trees differ.
>
> Could you say more about this usecase? Why does tooling need or want
> to know this; won't a checkout of the new commit end up being quick
> and simple? (I'm not saying your usecase is bad, just curious that it
> had never occurred to me, and I'm afraid I'm still not sure what your
> purpose might be.)
>
I'm thinking mainly about a monorepo context where there are a number of
distinct 'units' that can be described with sparse checkout patterns.
And perhaps there's some tooling that only wants to perform an action if
the content of a 'unit' changes.
Depending on the repo, it won't necessarily be quick to check out the
commit with the given patterns. However, it is more about it being
inconvenient to have to have a working directory, especially so if you
want use the tooling in some kind of service or query rapidly about
different revisions/patterns.
>> Another interesting use-case would be for tooling to use in conjunction
>> with 'git update-index --index-info'.
>
> Sorry, I'm not following. Could you expound on this a bit?
>
I was imagining something along the lines of being able to generate new
tree objects based on what matches the given sparse checkout patterns.
Not that I have a specific use case for it right now.
I think what I'm trying to evoke with that paragraph is that this
enables integrations with git that seem interesting and weren't possible
before.
>> 'rev-list --objects --filter=sparse:oid' comes close, but as rev-list is
>> concerned with objects rather than directory trees, it leaves files out
>> when the same blob occurs in at two different paths.
>
> s/in at/at/ ?
> >> It is possible to ask git about the sparse status of files currently in
>> the index with 'ls-files -t'. However, this does not work well when the
>> caller is interested in another commit, intererested in sparsity
>
> s/intererested/interested/
>
>> patterns that aren't currently in '.git/info/sparse-checkout', or when
>> working in with bare repo.
>
> s/in with bare/with a bare/ or s/in with bare/in a bare/?
>
Ah. Thanks for catching those.
>> To fill this gap, add a new argument to ls-tree '--sparse-filter-oid'
>> which takes the object id of a blob containing sparse checkout patterns
>> that filters the output of 'ls-tree'. When filtering with given sparsity
>> patterns, 'ls-tree' only outputs blobs and commit objects that
>> match the given patterns.
>
> This seems slightly unfortunate in that it makes things difficult for
> cone mode users to take advantage of. They will have to figure out
> how to translate their directory list into sparse checkout patterns
> before passing it along, and currently the only way to do that is via
> `git sparse-checkout set <patterns>` and reading the patterns from
> $GIT_DIR/info/sparse-checkout, but that toggles the sparsity of the
> current working tree and avoiding changing the current sparse-checkout
> was something you listed in your commit message as something you
> wanted to avoid.
> >> While it may be valid in some situations to output a tree object -- e.g.
>> when a cone pattern matches all blobs recursively contained in a tree --
>> it is less unclear what should be output if a sparse pattern matches
>> parts of a tree.
>>
>> To allow for reusing the pattern matching logic found in
>> 'path_in_sparse_checkout_1()' in 'dir.c' with arbitrary patterns,
>> extract the pattern matching part of the function into its own new
>> function 'recursively_match_path_with_sparse_patterns()'.
>>
>> Signed-off-by: William Sprent <williams@unity3d.com>
>> ---
>> ls-tree: add --sparse-filter-oid argument
>>
>> I'm resubmitting this change as rebased on top of 'master', as it
>> conflicted with the topic 'ls-tree.c: clean-up works' 1
>> [https://public-inbox.org/git/20230112091135.20050-1-tenglong.tl@alibaba-inc.com],
>> which was merged to 'master' recently.
>>
>> This versions also incorporates changes based on the comments made in 2
>> [https://public-inbox.org/git/CAPig+cRgZ0CrkqY7mufuWmhf6BC8yXjXXuOTEQjuz+Y0NA+N7Q@mail.gmail.com/].
>>
>> I'm also looping in contributors that have touched ls-tree and/or
>> sparse-checkouts recently. I hope that's okay.
>
> Of course! It's encouraged, even.
>
> [...]
>> diff --git a/builtin/ls-tree.c b/builtin/ls-tree.c
> [...]
>> +static void init_sparse_filter_data(struct sparse_filter_data **d, struct ls_tree_options *options,
>> + const char *sparse_oid_name, read_tree_fn_t fn)
>> +{
>> + struct object_id sparse_oid;
>> + struct object_context oc;
>> +
>> + (*d) = xcalloc(1, sizeof(**d));
>> + (*d)->fn = fn;
>> + (*d)->pl.use_cone_patterns = core_sparse_checkout_cone;
>
> Hmm, so the behavior still depends upon the current sparse-checkout
> (or lack thereof), despite the documentation and rationale of your
> feature as being there to check how a different sparse checkout would
> behave?
>
> I would hate to unconditionally turn cone_patterns off, since that
> would come with a huge performance penalty for the biggest repos. But
> turning it unconditionally on wouldn't be good for the non-cone users.
> This probably suggests we need something like another flag, or perhaps
> separate flags for each mode. Separate flags might provide the
> benefit of allowing cone mode users to specify directories rather than
> patterns, which would make it much easier for them to use.
>
I used 'core_sparse_checkout_cone' because I wanted to allow for the
cone mode optimisations, but I also figured that I should respect the
configuration. It doesn't change how the patterns are parsed in this case.
I agree that it is a bit awkward to have to "translate" the directories
into patterns when wanting to use cone mode. I can try adding
'--[no]-cone' flags and see how that feels. Together with Victoria's
suggestions that would result in having the following flags:
* --scope=(sparse|all)
* --sparse-patterns-file=<path>
* --[no]-cone: used together with --sparse-patterns-file to tell git
whether to interpret the patterns given as directories (cone) or
patterns (no-cone).
Which seems like a lot at first glance. But it allows for passing
directories instead of patterns for cone mode, and is similar to the
behaviour of 'sparse-checkout set'.
Does that seem like something that would make sense?
> [...]
>> +static int path_matches_sparse_checkout_patterns(struct strbuf *full_path, struct pattern_list *pl, int dtype)
>> +{
>> + enum pattern_match_result match = recursively_match_path_with_sparse_patterns(full_path->buf, the_repository->index, dtype, pl);
>> + return match > 0;
>
> So your new caller doesn't care about the pattern_match_result, it
> just wants to know if it got MATCHED or MATCHED_RECURSIVELY...
>
> [...]
>> diff --git a/dir.c b/dir.c
>> index 4e99f0c868f..122ebced08e 100644
>> --- a/dir.c
>> +++ b/dir.c
>> @@ -1457,45 +1457,50 @@ int init_sparse_checkout_patterns(struct index_state *istate)
>> return 0;
>> }
>>
>> -static int path_in_sparse_checkout_1(const char *path,
>> - struct index_state *istate,
>> - int require_cone_mode)
>> +int recursively_match_path_with_sparse_patterns(const char *path,
>
> You claim it returns an int here, but previously you presumed an enum
> pattern_match_result from the new caller.
>
>> + struct index_state *istate,
>> + int dtype,
>> + struct pattern_list *pl)
>> {
>> - int dtype = DT_REG;
>> enum pattern_match_result match = UNDECIDED;
>> const char *end, *slash;
>> -
>> - /*
>> - * We default to accepting a path if the path is empty, there are no
>> - * patterns, or the patterns are of the wrong type.
>> - */
>> - if (!*path ||
>> - init_sparse_checkout_patterns(istate) ||
>> - (require_cone_mode &&
>> - !istate->sparse_checkout_patterns->use_cone_patterns))
>> - return 1;
>> -
>> /*
>> * If UNDECIDED, use the match from the parent dir (recursively), or
>> * fall back to NOT_MATCHED at the topmost level. Note that cone mode
>> * never returns UNDECIDED, so we will execute only one iteration in
>> * this case.
>> */
>> - for (end = path + strlen(path);
>> - end > path && match == UNDECIDED;
>> + for (end = path + strlen(path); end > path && match == UNDECIDED;
>> end = slash) {
>> -
>> for (slash = end - 1; slash > path && *slash != '/'; slash--)
>> ; /* do nothing */
>>
>> match = path_matches_pattern_list(path, end - path,
>> slash > path ? slash + 1 : path, &dtype,
>> - istate->sparse_checkout_patterns, istate);
>> + pl, istate);
>>
>> /* We are going to match the parent dir now */
>> dtype = DT_DIR;
>> }
>> - return match > 0;
>> +
>> + return match;
>
> Um, this last line seems like a potentially scary change in behavior.
> Why should UNDECIDED return a non-zero value? Previously, we returned
> a 0 value for both UNDECIDED and NOT_MATCHED, but you've changed that
> here. If the change in this last line is actually correct, it should
> be split out into its own commit and explained in detail in the commit
> message.
>
>> +}
>> +
>> +static int path_in_sparse_checkout_1(const char *path,
>> + struct index_state *istate,
>> + int require_cone_mode)
>> +{
>> + /*
>> + * We default to accepting a path if the path is empty, there are no
>> + * patterns, or the patterns are of the wrong type.
>> + */
>> + if (!*path ||
>> + init_sparse_checkout_patterns(istate) ||
>> + (require_cone_mode &&
>> + !istate->sparse_checkout_patterns->use_cone_patterns))
>> + return 1;
>> +
>> + return recursively_match_path_with_sparse_patterns(path, istate, DT_REG, istate->sparse_checkout_patterns) > 0;
>
> Oh, you compare to > 0 here...and digging around your only other
> caller just immediately compares to > 0 as well.
>
> Why not just have recursively_match_path_with_sparse_patterns() do the
>> 0 check? If it does, returning int is fine. If it doesn't, it
> should be declared to return enum pattern_match_result.
I think my thinking was that it made sense in general for
'recursively_match_path_with_sparse_patterns()' to expose the match
result, but then failed to declare it that way. Thanks for catching it.
^ permalink raw reply
* Re: [PATCH v5 00/10] rebase: fix several code/testing/documentation issues around flag incompatibilities
From: Junio C Hamano @ 2023-01-25 16:39 UTC (permalink / raw)
To: Elijah Newren via GitGitGadget
Cc: git, Derrick Stolee, Elijah Newren, Eric Sunshine,
Martin Ågren, Phillip Wood
In-Reply-To: <pull.1466.v5.git.1674619434.gitgitgadget@gmail.com>
"Elijah Newren via GitGitGadget" <gitgitgadget@gmail.com> writes:
> Changes since v4:
>
> * Pulled out the changes regarding incompatibility detection for
> --[no-]reapply-cherry-picks into a separate patch. Phillip helped a lot
> with understanding the behavior, suggesting changes, and getting the
> wording right, and I think it deserves its own patch with its own
> explanation.
Hmph, does this replace the previous round that has already been in
'next' for two days? I do not mind reverting the merge and requeuing
it, but just wanted ot make sure before doing anything.
Thanks.
^ permalink raw reply
* Re: [PATCH v5 00/10] rebase: fix several code/testing/documentation issues around flag incompatibilities
From: Elijah Newren @ 2023-01-25 16:48 UTC (permalink / raw)
To: Junio C Hamano
Cc: Elijah Newren via GitGitGadget, git, Derrick Stolee,
Eric Sunshine, Martin Ågren, Phillip Wood
In-Reply-To: <xmqqa626fu0x.fsf@gitster.g>
On Wed, Jan 25, 2023 at 8:39 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> "Elijah Newren via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
> > Changes since v4:
> >
> > * Pulled out the changes regarding incompatibility detection for
> > --[no-]reapply-cherry-picks into a separate patch. Phillip helped a lot
> > with understanding the behavior, suggesting changes, and getting the
> > wording right, and I think it deserves its own patch with its own
> > explanation.
>
> Hmph, does this replace the previous round that has already been in
> 'next' for two days? I do not mind reverting the merge and requeuing
> it, but just wanted ot make sure before doing anything.
Sorry, I didn't realize it had merged to next. With Phillip's ongoing
reviews and requests for changes, which you had even commented on
(https://lore.kernel.org/git/xmqqpmb4j8e9.fsf@gitster.g/ and the
thread that was in the middle of), I assumed it was still only in
seen.
But yes, if you could revert from next and replace, that would be
great. Now that v5 has Phillip's blessing (and multiple of his
corrections), I think it's good to merge down.
^ permalink raw reply
* Re: [PATCH v2] ssh signing: better error message when key not in agent
From: Junio C Hamano @ 2023-01-25 17:04 UTC (permalink / raw)
To: Adam Szkoda
Cc: Adam Szkoda via GitGitGadget, git, Phillip Wood, Fabian Stelzer
In-Reply-To: <CAEroKagUY5PfuC2CDn=pTJ=brPsjPy6MVz54mH1tvN8E-Pvk7g@mail.gmail.com>
Adam Szkoda <adaszko@gmail.com> writes:
> On Tue, Jan 24, 2023 at 6:52 PM Junio C Hamano <gitster@pobox.com> wrote:
>> > Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1270/radicle-dev/maint-v2
>> > Pull-Request: https://github.com/git/git/pull/1270
>> >
>> > Range-diff vs v1:
>> >
>> > 1: 0ce06076242 < -: ----------- ssh signing: better error message when key not in agent
>> > -: ----------- > 1: 03dfca79387 ssh signing: better error message when key not in agent
>>
>> This is a fairly useless range-diff.
>>
>> Even when a range-diff shows the differences in the patches,
>> mechanically generated range-diff can only show _what_ changed. It
>> is helpful to explain the changes in your own words to highlight
>> _why_ such changes are done, and this place after the "---" line
>> and the diffstat we see below is the place to do so.
>>
>> Does GitGitGadget allow its users to describe the differences since
>> the previous iteration yourself?
>
> No, I don't think it does. It got generated automatically without
> giving me an opportunity to edit.
Hmph.
The text after "---" and before "Fetch-it-via:" does look like
something a human wrote. The part often is byte-for-byte identical
duplicate of the proposed log message, but I think I have seen
patches via GitGitGadget that have different text there, and was
hoping perhaps the authors can use it to describe commentary for the
range-diff.
> Yep, removed. It was largely accidental.
> ...
> Removed the superfluous {}.
>
> Thanks
Thanks. Looking good. Will queue and merge down to 'next'.
^ permalink raw reply
* Re: A summary of in-flight topics
From: Junio C Hamano @ 2023-01-25 17:12 UTC (permalink / raw)
To: Elijah Newren; +Cc: git
In-Reply-To: <CABPp-BH0LwHvN_bXp0BpekQ+f+15KNXQRzf5LCn3_BhcPzB-3A@mail.gmail.com>
Elijah Newren <newren@gmail.com> writes:
> On Tue, Jan 24, 2023 at 2:37 PM Junio C Hamano <gitster@pobox.com> wrote:
> [...]
>> - ab/sequencer-unleak 01-18 #8
>
> Perhaps move to "Expecting a hopefully minor and final reroll"? See
> e.g. https://lore.kernel.org/git/xmqqedry17r4.fsf@gitster.g/ where you
> commented on Phillip's approving the code changes but wanting a number
> of commit message cleanups and fixes. Ævar did send a v3, and Phillip
> commented on two of those patches, suggesting one could still use some
> commit message cleanups.
Thanks, you're right.
>> - ab/various-leak-fixes 01-18 #19
>
> Isn't this already merged as 9ea1378d04 ("Merge branch
> 'ab/various-leak-fixes'", 2022-12-14)?
The cover letter of its first iteration says
This is a follow-up to the ab/various-leak-fixes topic merged in
9ea1378d046 (Merge branch 'ab/various-leak-fixes', 2022-12-14). Like
that topic this is mixed collection of various leak fixes, all of
which should be simple to review & reason about.
Unfortunately the text does not appear in the cover for the
latest iteration, but that is how the topic name was reused.
THanks.
^ permalink raw reply
* Re: [PATCH v2] ssh signing: better error message when key not in agent
From: Junio C Hamano @ 2023-01-25 17:17 UTC (permalink / raw)
To: Adam Szkoda
Cc: Adam Szkoda via GitGitGadget, git, Phillip Wood, Fabian Stelzer
In-Reply-To: <CAEroKagUY5PfuC2CDn=pTJ=brPsjPy6MVz54mH1tvN8E-Pvk7g@mail.gmail.com>
Adam Szkoda <adaszko@gmail.com> writes:
>> We do
>> not pass filename yet with this pushl(), because ...
>>
>> > + if (literal_ssh_key) {
>> > + strvec_push(&signer.args, "-U");
>> > + }
>>
>> ... when we give a literal key, we want to insert "-U" in front, and then
>>
>> > + strvec_push(&signer.args, buffer_file->filename.buf);
>>
>> ... the filename. Which makes sense.
>
> I'm not sure what you mean in this paragraph. If there's something
> more that needs to be done, I'd appreciate it if you could rephrase
> it.
"Which makes sense." is the key conclusion. Instead of saying "This
part of the patch looks good" without explaining why I say so (it
could be that I am saying so without really reading or understanding
the changes or thinking the ramifications of the change through),
the earlier parts that lead to the conclusion is a way to give
weight to the conclusion.
In other words, it is meant to show that the reviewer did read the
patch well enough to understand the reasoning behind it.
Thanks.
^ permalink raw reply
* [PATCH] fix italian expression
From: Paolo Benvenuto via GitGitGadget @ 2023-01-25 18:17 UTC (permalink / raw)
To: git; +Cc: Paolo Benvenuto, Paolo Benvenuto
From: Paolo Benvenuto <paolobenve@gmail.com>
In italian "aggiornato rispetto a" means "ahead of".
"Up to date with something" is to be translated
"aggiornato a qualcosa".
Signed-off-by: Paolo Benvenuto <paolobenve@gmail.com>
---
fix italian expression
In italian "aggiornato rispetto a" means "ahead of".
"Up to date with something" is to be translated "aggiornato a qualcosa".
Signed-off-by: Paolo Benvenuto paolobenve@gmail.com
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1442%2Fpaolobenve%2Fpaolobenve-italian-fix-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1442/paolobenve/paolobenve-italian-fix-v1
Pull-Request: https://github.com/git/git/pull/1442
po/it.po | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/po/it.po b/po/it.po
index c31560834d8..47a0ab0f8bf 100644
--- a/po/it.po
+++ b/po/it.po
@@ -6545,7 +6545,7 @@ msgstr " (usa \"git branch --unset-upstream\" per correggere la situazione)\n"
#: remote.c:2105
#, c-format
msgid "Your branch is up to date with '%s'.\n"
-msgstr "Il tuo branch è aggiornato rispetto a '%s'.\n"
+msgstr "Il tuo branch è aggiornato a '%s'.\n"
#: remote.c:2109
#, c-format
@@ -24606,7 +24606,7 @@ msgstr "Impossibile trovare un commit comune con $pretty_name"
#: git-merge-octopus.sh:77
#, sh-format
msgid "Already up to date with $pretty_name"
-msgstr "Già aggiornato rispetto a $pretty_name"
+msgstr "Già aggiornato a $pretty_name"
#: git-merge-octopus.sh:89
#, sh-format
base-commit: 5dec958dcf965fc75e0f459f8e8ccf9c9f495b15
--
gitgitgadget
^ permalink raw reply related
* Re: [PATCH v2] ls-tree: add --sparse-filter-oid argument
From: Victoria Dye @ 2023-01-25 18:32 UTC (permalink / raw)
To: William Sprent, William Sprent via GitGitGadget, git
Cc: Eric Sunshine, Ævar Arnfjörð Bjarmason,
Derrick Stolee, Elijah Newren
In-Reply-To: <569043fb-9766-037e-c587-1545c2978e7d@unity3d.com>
William Sprent wrote:
> On 24/01/2023 21.11, Victoria Dye wrote>> I haven't looked at your implementation in detail yet, but I did want to
>> offer a recommendation in case you hadn't considered it: if you want to
>> allow the use of patterns from a user-specified specific file, it would be
>> nice to do it in a way that fully replaces the "default" sparse-checkout
>> settings at the lowest level (i.e., override the values of
>> 'core_apply_sparse_checkout', 'core_sparse_checkout_cone', and
>> 'get_sparse_checkout_filename()'). Doing it that way would both make it
>> easier for other commands to add a '--sparse-patterns' option, and avoid the
>> separate code path ('path_in_sparse_checkout_1()' vs.
>> 'recursively_match_path_with_sparse_patterns()', for example) when dealing
>> with '.git/info/sparse-checkout' patterns vs. manually-specified patterns.
>>
>
> Thanks for the pointers. I'll see what I can do. Do you mean something
> along the line of the following?
>
> set_sparse_checkout_file(filename);
> init_sparse_checkout_patterns(istate);
> _ = path_in_sparse_checkout_1(some_path, istate, ...);
>
Sort of. I mentioned separating the options into "specify the sparse pattern
file" and "restrict the displayed files to the active pattern set, if there
is one". For the former, you might add an option like:
OPT_FILENAME(0, "sparse-patterns", &sparse_pattern_file,
N_("override sparse-checkout behavior using patterns from <file>"))
Then do something like what you have, right after option parsing:
if (sparse_pattern_file) {
core_apply_sparse_checkout = 1;
core_sparse_checkout_cone = <???>;
set_sparse_checkout_file(filename);
}
If this option is specified, but the repo already has sparse
patterns/settings of its own, you'll need to (carefully) override the repo's
existing configuration:
* 'core_apply_sparse_checkout' & 'core_sparse_checkout_cone' are set based
on the repo config. You'll need to make sure those values are overridden
before loading the sparse-checkout patterns, and also that they're set
*after* loading the config.
* Speaking of 'core_sparse_checkout_cone', there are a bunch of ways you
might configure "cone-" vs. "non-cone" for your patterns (user-specified
with yet another option, always assume one or the other, try deriving from
the patterns). My preference would be to always assume "cone mode" - it's
the direction Git has been moving with sparse-checkout over the past year,
and still automatically "falls back" on non-cone patterns if the patterns
can't be used in cone mode (with a warning from
'add_pattern_to_hashsets()': "disabling cone pattern matching").
* If the repo is using a sparse index, the existing sparse directories may
not be compatible with the custom patterns. Probably easiest to force use
of a full index, e.g. with 'command_requires_full_index = 1'.
Fair warning: this probably isn't an exhaustive list of things that would
need updating, and it'll need thorough testing to make sure there are no
regressions. But, extending the underlying sparse-checkout infrastructure
will (ideally) avoid duplicating code and make this behavior reusable across
other commands.
For the other desired behavior ("limit the files to the active
sparse-checkout patterns"), you could add an option:
OPT_CALLBACK_F(0, "scope", &sparse_scope, "(sparse|all)",
N_("specify the scope of results with respect to the sparse-checkout"),
PARSE_OPT_NONEG, option_parse_scope),
...whose callback parses the string arg into a 'restrict_scope'
boolean/enum/something. Then, wherever in 'ls-files' a tree or the index are
iterated over, you can gate the per-file operation on:
if (!restrict_scope || path_in_sparse_checkout(<path>, istate)) {
/* do something with <path> */
}
Note that you should use 'path_in_sparse_checkout()', *not* the
internal/private function 'path_in_sparse_checkout_1()'; you also don't need
to explicitly 'init_sparse_checkout_patterns()'. Regardless of whether you
specified custom patterns or are using the ones in
'.git/info/sparse-checkout', 'path_in_sparse_checkout()' will initialize &
use the appropriate patterns.
^ permalink raw reply
* Re: [CI]: Is t7527 known to be flakey?
From: Jeff Hostetler @ 2023-01-25 19:02 UTC (permalink / raw)
To: SZEDER Gábor; +Cc: Junio C Hamano, Jeff Hostetler, edecosta, git
In-Reply-To: <20230123181216.GB2155@szeder.dev>
On 1/23/23 1:12 PM, SZEDER Gábor wrote:
> On Mon, Jan 23, 2023 at 11:56:53AM -0500, Jeff Hostetler wrote:
>> Was this on Linux or MacOS ?
>
> On an average-ish Linux (an Ubuntu LTS variant). So the issue is not
> specific to musl.
>
OK, thanks. I wasn't worried about "musl", but rather whether
you were running the stress test on a Linux or Mac.
Since they have different backends (inotify vs FSEvent) and all
the code that touches the filesystem is different, it would best
to start on the correct OS when trying to repro it.
Can you tell from your stess test whether the fsmonitor-daemon
is crashing? (It might be subtle since the daemon is auto-started
if necessary, so it might be crashing and silently getting restarted
by the next command.)
I ask because a SIGPIPE in the client would make me think that the
server suddenly closed the connection unexpectedly, like if it had
SIGSEGV'd or something.
I won't have time to spin up a Linux VM until next week, so I
won't be able to investigate this for a bit.
Jeff
^ permalink raw reply
* Re: [CI]: Is t7527 known to be flakey?
From: SZEDER Gábor @ 2023-01-25 21:12 UTC (permalink / raw)
To: Jeff Hostetler; +Cc: Junio C Hamano, Jeff Hostetler, edecosta, git
In-Reply-To: <7fa84280-db1d-8618-571a-ce0ac7a26135@jeffhostetler.com>
On Wed, Jan 25, 2023 at 02:02:40PM -0500, Jeff Hostetler wrote:
> Can you tell from your stess test whether the fsmonitor-daemon
> is crashing? (It might be subtle since the daemon is auto-started
> if necessary, so it might be crashing and silently getting restarted
> by the next command.)
>
> I ask because a SIGPIPE in the client would make me think that the
> server suddenly closed the connection unexpectedly, like if it had
> SIGSEGV'd or something.
Last time around I only looked at the failing test case, and didn't
notice anything that might have indicated the cause of the SIGPIPE.
This time I chanced to look a bit further up in the test log, and:
expecting success of 7527.55 'Matrix[uc:true][fsm:true] move_directory_contents_deeper':
matrix_clean_up_repo &&
$fn &&
if test $uc = false && test $fsm = false
then
git status --porcelain=v1 >.git/expect.$fn
else
git status --porcelain=v1 >.git/actual.$fn &&
test_cmp .git/expect.$fn .git/actual.$fn
fi
+ matrix_clean_up_repo
+ git reset --hard HEAD
HEAD is now at 1d1edcb initial
+ git clean -fd
+ move_directory_contents_deeper
+ mkdir T1/_new_
+ mv T1/F1 T1/F2 T1/T2 T1/_new_
+ test true = false
+ git status --porcelain=v1
error: read error: Connection reset by peer
error: could not read IPC response
+ test_cmp .git/expect.move_directory_contents_deeper .git/actual.move_directory_contents_deeper
+ test 2 -ne 2
+ eval diff -u "$@"
+ diff -u .git/expect.move_directory_contents_deeper .git/actual.move_directory_contents_deeper
ok 55 - Matrix[uc:true][fsm:true] move_directory_contents_deeper
expecting success of 7527.56 'Matrix[uc:true][fsm:true] move_directory_up':
matrix_clean_up_repo &&
$fn &&
if test $uc = false && test $fsm = false
then
git status --porcelain=v1 >.git/expect.$fn
else
git status --porcelain=v1 >.git/actual.$fn &&
test_cmp .git/expect.$fn .git/actual.$fn
fi
+ matrix_clean_up_repo
+ git reset --hard HEAD
HEAD is now at 1d1edcb initial
+ git clean -fd
Removing T1/_new_/
+ move_directory_up
+ mv T1/T2/T3 T1
+ test true = false
+ git status --porcelain=v1
error: last command exited with $?=141
not ok 56 - Matrix[uc:true][fsm:true] move_directory_up
Notice that "error: read error: Connection reset by peer" in the
previous, still successful test case! I ran it a couple of times, and
saw the same error message in the still successful '42 -
Matrix[uc:false][fsm:true] move_directory_contents_deeper' followed by
a SIGPIPE caused failure in the next test case. And now that I knew
what to look for, I noticed this error message in the very first test
failure I reported the other day, which didn't fail because of
SIGPIPE, and in that case the error message was printed in the failed
test case.
And there were a few cases that failed because of SIGPIPE but there
were no error messages at all.
I can't say what caused these errors, but I doubt that anything
segfaulted, because segfaults are logged in syslog, and I haven't
found any such syslog entries coinciding with stress testing.
^ permalink raw reply
* Re: [PATCH v2] ssh signing: better error message when key not in agent
From: Eric Sunshine @ 2023-01-25 21:42 UTC (permalink / raw)
To: Adam Szkoda
Cc: Junio C Hamano, Adam Szkoda via GitGitGadget, git, Phillip Wood,
Fabian Stelzer
In-Reply-To: <CAEroKagUY5PfuC2CDn=pTJ=brPsjPy6MVz54mH1tvN8E-Pvk7g@mail.gmail.com>
On Wed, Jan 25, 2023 at 8:05 AM Adam Szkoda <adaszko@gmail.com> wrote:
> On Tue, Jan 24, 2023 at 6:52 PM Junio C Hamano <gitster@pobox.com> wrote:
> > > Range-diff vs v1:
> > >
> > > 1: 0ce06076242 < -: ----------- ssh signing: better error message when key not in agent
> > > -: ----------- > 1: 03dfca79387 ssh signing: better error message when key not in agent
> >
> > This is a fairly useless range-diff.
> >
> > Even when a range-diff shows the differences in the patches,
> > mechanically generated range-diff can only show _what_ changed. It
> > is helpful to explain the changes in your own words to highlight
> > _why_ such changes are done, and this place after the "---" line
> > and the diffstat we see below is the place to do so.
> >
> > Does GitGitGadget allow its users to describe the differences since
> > the previous iteration yourself?
>
> No, I don't think it does. It got generated automatically without
> giving me an opportunity to edit.
Yes, the user can describe the differences since the previous
iteration by editing the pull-request's description. Specifically,
when ready to send a new iteration:
(1) force push the new iteration to the same branch on GitHub
(2) edit the pull-request description; this is the very first
"comment" on the pull-request page; press the "..." button on that
comment and choose the "Edit" menu item; revise the text to describe
the changes since the previous revision, then press the "Update
comment" button to save
(3) post a "/submit" comment to the pull-request to tell GitGitGadget
to send the new revision to the Git mailing list
^ permalink raw reply
* Re: [PATCH v2] ssh signing: better error message when key not in agent
From: Junio C Hamano @ 2023-01-25 22:22 UTC (permalink / raw)
To: Eric Sunshine
Cc: Adam Szkoda, Adam Szkoda via GitGitGadget, git, Phillip Wood,
Fabian Stelzer
In-Reply-To: <CAPig+cTNk1RvfdFembKcxTOUs0UsiXmz8rsEnab-0fQp-QE3Lg@mail.gmail.com>
Eric Sunshine <sunshine@sunshineco.com> writes:
> On Wed, Jan 25, 2023 at 8:05 AM Adam Szkoda <adaszko@gmail.com> wrote:
>> On Tue, Jan 24, 2023 at 6:52 PM Junio C Hamano <gitster@pobox.com> wrote:
>> > > Range-diff vs v1:
>> > >
>> > > 1: 0ce06076242 < -: ----------- ssh signing: better error message when key not in agent
>> > > -: ----------- > 1: 03dfca79387 ssh signing: better error message when key not in agent
>> >
>> > This is a fairly useless range-diff.
>> >
>> > Even when a range-diff shows the differences in the patches,
>> > mechanically generated range-diff can only show _what_ changed. It
>> > is helpful to explain the changes in your own words to highlight
>> > _why_ such changes are done, and this place after the "---" line
>> > and the diffstat we see below is the place to do so.
>> >
>> > Does GitGitGadget allow its users to describe the differences since
>> > the previous iteration yourself?
>>
>> No, I don't think it does. It got generated automatically without
>> giving me an opportunity to edit.
>
> Yes, the user can describe the differences since the previous
> iteration by editing the pull-request's description. Specifically,
> when ready to send a new iteration:
>
> (1) force push the new iteration to the same branch on GitHub
>
> (2) edit the pull-request description; this is the very first
> "comment" on the pull-request page; press the "..." button on that
> comment and choose the "Edit" menu item; revise the text to describe
> the changes since the previous revision, then press the "Update
> comment" button to save
>
> (3) post a "/submit" comment to the pull-request to tell GitGitGadget
> to send the new revision to the Git mailing list
Thanks. I thought the above would make a good addition to our
documentation set. Documentation/MyFirstContribution.txt does have
this to say:
Once you have your branch again in the shape you want following all review
comments, you can submit again:
----
$ git push -f remotename psuh
----
Next, go look at your pull request against GitGitGadget; you should see the CI
has been kicked off again. Now while the CI is running is a good time for you
to modify your description at the top of the pull request thread; it will be
used again as the cover letter. You should use this space to describe what
has changed since your previous version, so that your reviewers have some idea
of what they're looking at. When the CI is done running, you can comment once
more with `/submit` - GitGitGadget will automatically add a v2 mark to your
changes.
before it talks about doing the "/submit" again. Expanding the
above into a bulletted list form like you did might make it easier
to follow through, perhaps? I dunno.
^ 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