* [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 04/10] rebase: fix docs about incompatibilities with --root
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>
In commit 5dacd4abdd ("git-rebase.txt: document incompatible options",
2018-06-25), I added notes about incompatibilities between options for
the apply and merge backends. Unfortunately, I inverted the condition
when --root was incompatible with the apply backend. Fix the
documentation, and add a testcase that verifies the documentation
matches the code.
While at it, the documentation for --root also tried to cover some of
the backend differences between the apply and merge backends in relation
to reapplying cherry picks. The information:
* assumed that the apply backend was the default (it isn't anymore)
* was written before --reapply-cherry-picks became an option
* was written before the detailed information on backend differences
All of these factors make the sentence under --root about reapplying
cherry picks contradict information that is now available elsewhere in
the manual, and the other references are correct. So just strike this
sentence.
Signed-off-by: Elijah Newren <newren@gmail.com>
---
Documentation/git-rebase.txt | 7 ++-----
t/t3422-rebase-incompatible-options.sh | 4 ++++
2 files changed, 6 insertions(+), 5 deletions(-)
diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
index 7d01d1412d1..846aeed1b69 100644
--- a/Documentation/git-rebase.txt
+++ b/Documentation/git-rebase.txt
@@ -574,10 +574,7 @@ See also INCOMPATIBLE OPTIONS below.
--root::
Rebase all commits reachable from `<branch>`, instead of
limiting them with an `<upstream>`. This allows you to rebase
- the root commit(s) on a branch. When used with `--onto`, it
- will skip changes already contained in `<newbase>` (instead of
- `<upstream>`) whereas without `--onto` it will operate on every
- change.
+ the root commit(s) on a branch.
+
See also INCOMPATIBLE OPTIONS below.
@@ -656,7 +653,7 @@ are incompatible with the following options:
* --reapply-cherry-picks
* --edit-todo
* --update-refs
- * --root when used in combination with --onto
+ * --root when used without --onto
In addition, the following pairs of options are incompatible:
diff --git a/t/t3422-rebase-incompatible-options.sh b/t/t3422-rebase-incompatible-options.sh
index 9b9e78479f6..f86274990b0 100755
--- a/t/t3422-rebase-incompatible-options.sh
+++ b/t/t3422-rebase-incompatible-options.sh
@@ -65,6 +65,10 @@ test_rebase_am_only () {
test_must_fail git rebase $opt --update-refs A
"
+ test_expect_success "$opt incompatible with --root without --onto" "
+ git checkout B^0 &&
+ test_must_fail git rebase $opt --root A
+ "
}
# Check options which imply --apply
--
gitgitgadget
^ permalink raw reply related
* [PATCH v5 03/10] rebase: remove --allow-empty-message from incompatible opts
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>
--allow-empty-message was turned into a no-op and even documented
as such; the flag is simply ignored. Since the flag is ignored, it
shouldn't be documented as being incompatible with other flags.
Signed-off-by: Elijah Newren <newren@gmail.com>
---
Documentation/git-rebase.txt | 1 -
1 file changed, 1 deletion(-)
diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
index 6490bc96a15..7d01d1412d1 100644
--- a/Documentation/git-rebase.txt
+++ b/Documentation/git-rebase.txt
@@ -647,7 +647,6 @@ are incompatible with the following options:
* --merge
* --strategy
* --strategy-option
- * --allow-empty-message
* --[no-]autosquash
* --rebase-merges
* --interactive
--
gitgitgadget
^ permalink raw reply related
* [PATCH v5 02/10] rebase: flag --apply and --merge as incompatible
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>
Previously, we flagged options which implied --apply as being
incompatible with options which implied --merge. But if both options
were given explicitly, then we didn't flag the incompatibility. The
same is true with --apply and --interactive. Add the check, and add
some testcases to verify these are also caught.
Signed-off-by: Elijah Newren <newren@gmail.com>
---
builtin/rebase.c | 12 ++++++++++--
t/t3422-rebase-incompatible-options.sh | 3 +++
2 files changed, 13 insertions(+), 2 deletions(-)
diff --git a/builtin/rebase.c b/builtin/rebase.c
index c111b89e137..b742cc8fb5c 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -907,6 +907,9 @@ static int parse_opt_am(const struct option *opt, const char *arg, int unset)
BUG_ON_OPT_NEG(unset);
BUG_ON_OPT_ARG(arg);
+ if (opts->type != REBASE_UNSPECIFIED && opts->type != REBASE_APPLY)
+ die(_("apply options and merge options cannot be used together"));
+
opts->type = REBASE_APPLY;
return 0;
@@ -920,8 +923,10 @@ static int parse_opt_merge(const struct option *opt, const char *arg, int unset)
BUG_ON_OPT_NEG(unset);
BUG_ON_OPT_ARG(arg);
- if (!is_merge(opts))
- opts->type = REBASE_MERGE;
+ if (opts->type != REBASE_UNSPECIFIED && opts->type != REBASE_MERGE)
+ die(_("apply options and merge options cannot be used together"));
+
+ opts->type = REBASE_MERGE;
return 0;
}
@@ -935,6 +940,9 @@ static int parse_opt_interactive(const struct option *opt, const char *arg,
BUG_ON_OPT_NEG(unset);
BUG_ON_OPT_ARG(arg);
+ if (opts->type != REBASE_UNSPECIFIED && opts->type != REBASE_MERGE)
+ die(_("apply options and merge options cannot be used together"));
+
opts->type = REBASE_MERGE;
opts->flags |= REBASE_INTERACTIVE_EXPLICIT;
diff --git a/t/t3422-rebase-incompatible-options.sh b/t/t3422-rebase-incompatible-options.sh
index 9da39cd91c2..9b9e78479f6 100755
--- a/t/t3422-rebase-incompatible-options.sh
+++ b/t/t3422-rebase-incompatible-options.sh
@@ -67,7 +67,10 @@ test_rebase_am_only () {
}
+# Check options which imply --apply
test_rebase_am_only --whitespace=fix
test_rebase_am_only -C4
+# Also check an explicit --apply
+test_rebase_am_only --apply
test_done
--
gitgitgadget
^ permalink raw reply related
* [PATCH v5 01/10] rebase: mark --update-refs as requiring the merge backend
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>
--update-refs is built in terms of the sequencer, which requires the
merge backend. It was already marked as incompatible with the apply
backend in the git-rebase manual, but the code didn't check for this
incompatibility and warn the user. Check and error now.
While at it, fix a typo in t3422...and fix some misleading wording
(most options which used to be am-specific have since been implemented
in the merge backend as well).
Signed-off-by: Elijah Newren <newren@gmail.com>
---
Documentation/git-rebase.txt | 2 ++
builtin/rebase.c | 3 +++
t/t3422-rebase-incompatible-options.sh | 15 ++++++++++-----
3 files changed, 15 insertions(+), 5 deletions(-)
diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
index d811c1cf443..6490bc96a15 100644
--- a/Documentation/git-rebase.txt
+++ b/Documentation/git-rebase.txt
@@ -630,6 +630,8 @@ start would be overridden by the presence of
+
If the configuration variable `rebase.updateRefs` is set, then this option
can be used to override and disable this setting.
++
+See also INCOMPATIBLE OPTIONS below.
INCOMPATIBLE OPTIONS
--------------------
diff --git a/builtin/rebase.c b/builtin/rebase.c
index a26cc0cfdb5..c111b89e137 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -1492,6 +1492,9 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
}
}
+ if (options.update_refs)
+ imply_merge(&options, "--update-refs");
+
if (options.type == REBASE_UNSPECIFIED) {
if (!strcmp(options.default_backend, "merge"))
imply_merge(&options, "--merge");
diff --git a/t/t3422-rebase-incompatible-options.sh b/t/t3422-rebase-incompatible-options.sh
index 6dabb05a2ad..9da39cd91c2 100755
--- a/t/t3422-rebase-incompatible-options.sh
+++ b/t/t3422-rebase-incompatible-options.sh
@@ -25,11 +25,11 @@ test_expect_success 'setup' '
'
#
-# Rebase has lots of useful options like --whitepsace=fix, which are
-# actually all built in terms of flags to git-am. Since neither
-# --merge nor --interactive (nor any options that imply those two) use
-# git-am, using them together will result in flags like --whitespace=fix
-# being ignored. Make sure rebase warns the user and aborts instead.
+# Rebase has a couple options which are specific to the apply backend,
+# and several options which are specific to the merge backend. Flags
+# from the different sets cannot work together, and we do not want to
+# just ignore one of the sets of flags. Make sure rebase warns the
+# user and aborts instead.
#
test_rebase_am_only () {
@@ -60,6 +60,11 @@ test_rebase_am_only () {
test_must_fail git rebase $opt --exec 'true' A
"
+ test_expect_success "$opt incompatible with --update-refs" "
+ git checkout B^0 &&
+ test_must_fail git rebase $opt --update-refs A
+ "
+
}
test_rebase_am_only --whitespace=fix
--
gitgitgadget
^ permalink raw reply related
* [PATCH v5 00/10] rebase: fix several code/testing/documentation issues around flag 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
In-Reply-To: <pull.1466.v4.git.1674367961.gitgitgadget@gmail.com>
We had a report about --update-refs being ignored when --whitespace=fix was
passed, confusing an end user. These were already marked as incompatible in
the manual, but the code didn't check for the incompatibility and provide an
error to the user.
Folks brought up other flags tangentially when reviewing an earlier round of
this series, and I found we had more that were missing checks, and that we
were missing some testcases, and that the documentation was wrong on some of
the relevant options. So, I ended up getting lots of little fixes to
straighten these all out.
Changes since 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
--
gitgitgadget
^ permalink raw reply
* Re: [PATCH 4/5] pretty: Add name_and_address_only parameter
From: Junio C Hamano @ 2023-01-25 2:23 UTC (permalink / raw)
To: Zev Weiss; +Cc: git, Emma Brooks, Hamza Mahfooz
In-Reply-To: <20230119223858.29262-5-zev@bewilderbeest.net>
Zev Weiss <zev@bewilderbeest.net> writes:
> This is meant to be used with pp_user_info() when using it to format
> email recipients generated by --to-cmd/--cc-cmd. When set it omits
> the leading 'From: ', trailing linefeed, and the date suffix, and
> additionally will return the input string unmodified if
> split_ident_line() can't parse it (e.g. for a bare email address).
It is somewhat disturbing to see that this only takes effect when
cmit_fmt_is_mail(pp->fmt) and completely ignored otherwise. Seeing
pp->fmt and pp->name_and_address_only sitting next to each other, it
looks like a layering error.
I wonder if you instead want a new value for pp->fmt that
cmit_fmt_is_mail() considers an e-mail format but is different from
what we used for usual e-mail format?
> diff --git a/pretty.c b/pretty.c
> index 1e1e21878c83..e6798fadc107 100644
> --- a/pretty.c
> +++ b/pretty.c
> @@ -509,8 +509,11 @@ void pp_user_info(struct pretty_print_context *pp,
> return;
>
> line_end = strchrnul(line, '\n');
> - if (split_ident_line(&ident, line, line_end - line))
> + if (split_ident_line(&ident, line, line_end - line)) {
> + if (pp->name_and_address_only)
> + strbuf_addstr(sb, line);
> return;
> + }
This error handling is also disturbing. What makes it correct to
parrot the original input that does not parse correctly as an ident
line to the output, only when name_and_address_only bit is on? It
does not make any sense to do so when cmit_fmt_is_mail(pp->fmt) is
false, especially.
^ permalink raw reply
* Re: [PATCH 2/5] log: Refactor duplicated code to headerize recipient lists
From: Junio C Hamano @ 2023-01-25 2:11 UTC (permalink / raw)
To: Zev Weiss
Cc: git, brian m. carlson, Ævar Arnfjörð Bjarmason,
Patrick Hemmer
In-Reply-To: <20230119223858.29262-3-zev@bewilderbeest.net>
Zev Weiss <zev@bewilderbeest.net> writes:
> Subject: Re: [PATCH 2/5] log: Refactor duplicated code to headerize recipient lists
Style: "log: Refactor" -> "log: refactor"
cf. Documentation/SubmittingPatches[[summary-section]]
> The To and Cc headers are handled identically (the only difference is
> the header name tag), so we might as well reuse the same code for both
> instead of duplicating it.
Makes tons of sense. But seeing this one ...
> + recipients_to_header_buf("To", &buf, &extra_to);
> + recipients_to_header_buf("Cc", &buf, &extra_cc);
... the parameters to the function probably should be ...
> +void recipients_to_header_buf(const char *hdr, struct strbuf *buf,
> + const struct string_list *recipients);
... in this order, instead:
format_recipients(&buf, "To", &extra_to);
That is, "To" and &extra_to are much closely related to each other
than they are to &buf in the sense that they are both input to the
helper function to work on, while &buf is an output buffer, and we
tend to place closer things together, next to each other.
Other than that, removal of repetition is very good.
Thanks.
^ permalink raw reply
* A summary of in-flight topics
From: Junio C Hamano @ 2023-01-24 22:30 UTC (permalink / raw)
To: git
I hate to send out new issues of the "What's cooking" report too
often, but we have too many in-flight topics that are not getting
traction they may deserve, so let me send out a summary of the
topics listed in the draft I have for the next issue of it.
Those that are marked as "will merge to 'master'" won't be merged
immediately; they usually spend at least 1 calendar week in 'next'
(the second date on the line for each topic is the date they were
merged to 'next').
--------------------------------------------------
Expecting a (hopefully final) reroll.
- ms/send-email-feed-header-to-validate-hook 01-19 #2
- cb/checkout-same-branch-twice 01-20 #1
- rj/avoid-switching-to-already-used-branch 01-22 #3
Expecting a hopefully minor and final reroll.
- ab/avoid-losing-exit-codes-in-tests 12-20 #6
Expecting a reroll.
- tl/notes--blankline 11-09 #5
- ab/tag-object-type-errors 11-22 #5
- ab/config-multi-and-nonbool 11-27 #9
- cb/grep-fallback-failing-jit 12-17 #1
- ja/worktree-orphan 01-13 #4
- tc/cat-file-z-use-cquote 01-16 #1
- rj/branch-unborn-in-other-worktrees 01-19 #3
May want to discard. Breaking compatibility does not seem worth it.
- so/diff-merges-more 12-18 #5
May want to discard. Its jaggy edges may be a bit too sharp.
- cc/filtered-repack 12-25 #3
Needs review on the updated round.
- ed/fsmonitor-inotify 12-13 #6
Needs review.
- jc/spell-id-in-both-caps-in-message-id 12-17 #1
- ad/test-record-count-when-harness-is-in-use 12-25 #1
- cw/submodule-status-in-parallel 01-17 #6
- ab/sequencer-unleak 01-18 #8
- ab/various-leak-fixes 01-18 #19
Undecided
- rs/tree-parse-mode-overflow-check 01-21 #1
- rj/bisect-already-used-branch 01-22 #1
- ab/hook-api-with-stdin 01-23 #5
- ds/bundle-uri-5 01-23 #10
Waiting for review response.
- mc/switch-advice 11-09 #1
- js/range-diff-mbox 11-23 #1
Will merge to 'master'.
+ ab/cache-api-cleanup-users 01-17/01-18 #3
+ sa/cat-file-mailmap--batch-check 01-18/01-18 #1
+ km/send-email-with-v-reroll-count 11-27/01-19 #1
+ pb/branch-advice-recurse-submodules 01-18/01-19 #1
+ jc/doc-branch-update-checked-out-branch 01-18/01-19 #1
+ cb/grep-pcre-ucp 01-18/01-19 #1
+ jk/hash-object-literally-fd-leak 01-19/01-19 #1
+ cw/fetch-remote-group-with-duplication 01-19/01-20 #1
+ jc/doc-checkout-b 01-19/01-23 #1
+ po/pretty-format-columns-doc 01-19/01-23 #5
+ jk/hash-object-fsck 01-19/01-23 #7
+ en/rebase-update-refs-needs-merge-backend 01-22/01-23 #9
+ tb/t0003-invoke-dd-more-portably 01-22/01-23 #1
+ jc/attr-doc-fix 01-22/01-24 #1
+ ar/markup-em-dash 01-23/01-24 #1
Will merge to 'next'.
- mc/credential-helper-auth-headers 01-20 #12
Will merge to 'next'?
- en/ls-files-doc-update 01-13 #4
- as/ssh-signing-improve-key-missing-error 01-24 #1
^ permalink raw reply
* Re: git not allowing 744 as permissions for a file
From: brian m. carlson @ 2023-01-24 22:04 UTC (permalink / raw)
To: Auriane Reverdell; +Cc: git
In-Reply-To: <CAFC92SFZgQtstEzV5pgT_tPSs=6fRJ=rE6ad_DENnn_UoobxFQ@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 886 bytes --]
On 2023-01-24 at 15:48:36, Auriane Reverdell wrote:
> Hi,
>
> git doesn't allow to add the execution permission on a file only for
> the user. A chmod 744 on a file will transform into 755 when added to
> git. This can potentially lead to security problems on certain
> systems. Is there a way to fix that? I'll be happy to do so if
> somebody shows me where to do it.
No, there isn't. Git tracks only whether the executable bit is set.
All file modes are either 644 or 755.
If you need the permissions or ownership on the file to be different,
you can do that by using a script to copy the files into another
location with the proper permissions or ownership instead of using the
copies in the repository. For example, I do this with my dotfiles such
that the files have the correct permissions.
--
brian m. carlson (he/him or they/them)
Toronto, Ontario, CA
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 263 bytes --]
^ permalink raw reply
* Re: [PATCH] tree-walk: disallow overflowing modes
From: Junio C Hamano @ 2023-01-24 20:44 UTC (permalink / raw)
To: René Scharfe
Cc: Ævar Arnfjörð Bjarmason, Git List, Jeff King
In-Reply-To: <b4b48877-5b80-e96f-d09f-2fe275f42950@web.de>
René Scharfe <l.s.r@web.de> writes:
> ... It's basically harmless. Perhaps
> we just need a comment stating that, to contain the urge to "fix" this.
Yeah, especially since fsck finds and warns about bad mode with
FSCK_MSG_BAD_FILEMODE and also FSCK_MSG_ZERO_PADDED_FILEMODE in a
separate codepath, adding another piece of code that checks a
slightly different condition does not sound like a good idea.
Thanks.
^ permalink raw reply
* Re: [PATCH v2.5 02/11] bundle: verify using connected()
From: Junio C Hamano @ 2023-01-24 20:41 UTC (permalink / raw)
To: Derrick Stolee
Cc: Derrick Stolee via GitGitGadget, git, me, vdye, avarab, steadmon,
chooglen
In-Reply-To: <bad5d998-b34a-15dd-4f9d-9bf1cb74dda0@github.com>
Derrick Stolee <derrickstolee@github.com> writes:
>> I do not see the need to say "even" here. In what other situation
>> do we make connectivity checks, and is there a need to be more
>> strict than others when checking fetched packfiles?
>
> I suppose that I was implying that fetches are the more common
> operation, and the scrutiny applied to an arbitrary pack-file from
> a remote is probably higher there. However, who knows where a
> bundle came from, so the scrutiny should be the same.
Ah, I see that is where that "even" came from. And yes, I agree
that unbundling and fetch should be suspicious of their input to the
same degree.
^ permalink raw reply
* Re: [PATCH v2] ls-tree: add --sparse-filter-oid argument
From: Victoria Dye @ 2023-01-24 20:11 UTC (permalink / raw)
To: William Sprent via GitGitGadget, git
Cc: Eric Sunshine, Ævar Arnfjörð Bjarmason,
Derrick Stolee, Elijah Newren, William Sprent
In-Reply-To: <pull.1459.v2.git.1674474371817.gitgitgadget@gmail.com>
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.
>
> 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'.
>
> 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).
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.
>
> 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 v4 5/9] rebase: add coverage of other incompatible options
From: Phillip Wood @ 2023-01-24 19:21 UTC (permalink / raw)
To: Elijah Newren
Cc: Elijah Newren via GitGitGadget, git, Derrick Stolee,
Eric Sunshine, Martin Ågren, Phillip Wood
In-Reply-To: <CABPp-BEwv+cRMOR_-kz_UhfQt1+SGRdhictLmwmq=122LYZaDw@mail.gmail.com>
On 24/01/2023 17:12, Elijah Newren wrote:
> On Tue, Jan 24, 2023 at 8:48 AM Phillip Wood <phillip.wood123@gmail.com> wrote:
>>>> + options.reapply_cherry_picks = keep_base;
>>>> + else if (!keep_base)
>>>> + /*
>>>> + * --keep-base implements --reapply-cherry-picks by
>>>
>>> Should this be --[no-]reapply-cherry-picks, to clarify that it handles
>>> both cases? Especially given how many times I missed it?
>>
>> This has obviously proved to be confusing. The aim was to explain that
>> in order to work with the apply backend "[--reapply-cherry-picks]
>> --keep-base" was doing something unusual with `upstream` to reapply
>> cherry picks. "--no-reapply-cherry-picks --keep-base" does not do
>> anything unusual with `upstream`. I don't think changing it to
>> --[no-]reapply-cherry-picks quite captures that. I came up with
>>
>> To support --reapply-cherry-picks (which is not supported by the apply
>> backend) --keep-base alters upstream to prevent cherry picked commits
>> from being dropped.
>>
>> but it really needs to mention that --keep-base also supports
>> --no-reapply-cherry-picks in the usual way
>
> Somewhat wordy, but perhaps:
>
> /*
> * 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.
> */
That sounds good to me, it is definitely an improvement on the current
comment which I think is too terse.
Best Wishes
Phillip
^ permalink raw reply
* Re: GSoC 2023
From: Taylor Blau @ 2023-01-24 19:18 UTC (permalink / raw)
To: Christian Couder
Cc: Kaartic Sivaraam, git, Derrick Stolee, Victoria Dye, Hariom verma
In-Reply-To: <CAP8UFD32nDLR8BrhmeTpyraX3QBrc=U1ody+qgyMVY+_-HrASA@mail.gmail.com>
On Tue, Jan 24, 2023 at 05:38:31PM +0100, Christian Couder wrote:
> Actually you were already an Org Admin last year and it looks like
> they didn't remove people from the roles they had last year, so you
> are still an Org Admin.
I think that puts me in the same boat, though I can't remember if I am
actually an org admin or not. If I'm not, and you folks need some extra
help, I'm happy to participate.
I am going to pass on mentoring this year, though. It was a fun
experience for me last time, but took more time than I had planned for.
If someone needs or wants a co-mentor, though, I'm happy to help there
as I think it will end up taking less time.
Thanks,
Taylor
^ permalink raw reply
* Re: [PATCH] tree-walk: disallow overflowing modes
From: René Scharfe @ 2023-01-24 18:53 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason
Cc: Git List, Junio C Hamano, Jeff King
In-Reply-To: <230123.86a629tzgc.gmgdl@evledraar.gmail.com>
Am 23.01.23 um 09:33 schrieb Ævar Arnfjörð Bjarmason:
>
> On Sat, Jan 21 2023, René Scharfe wrote:
>
>> When parsing tree entries, reject mode values that don't fit into an
>> unsigned int.
>>
>> Signed-off-by: René Scharfe <l.s.r@web.de>
>> ---
>> tree-walk.c | 2 ++
>> 1 file changed, 2 insertions(+)
>>
>> diff --git a/tree-walk.c b/tree-walk.c
>> index 74f4d710e8..5e7bc38600 100644
>> --- a/tree-walk.c
>> +++ b/tree-walk.c
>> @@ -17,6 +17,8 @@ static const char *get_mode(const char *str, unsigned int *modep)
>> while ((c = *str++) != ' ') {
>> if (c < '0' || c > '7')
>> return NULL;
>> + if ((mode << 3) >> 3 != mode)
>> + return NULL;
>> mode = (mode << 3) + (c - '0');
>> }
>> *modep = mode;
>
> There was a discussion about this on git-security last August, in a
> report that turned out to be uninteresting for the security aspects.
>
> I'll just quote my own reply here out of context
> (<220811.86mtcbqd5x.gmgdl@evledraar.gmail.com> for those with access).
> On the other hand this edge case is also a golden opportunity we're not
> likely to ever have again. We can't really change the git object format
> at this point without *major* headaches, but in this case we have the
> ability to encode arbitrary data into tree entries (e.g file metadata)
> as long as the writer makes sure they overflow back to the valid
> filemode :)
Patch v1 cited above still keeps bits beyond S_IFMT (0xF000), so that's
at 16 bits on a platform with 32-bit unsigned int for future object
types or other metadata.
One bit would suffice to switch the path field into an URL and encode
additional metadata there. We could do that even with the stricter
patch v2 by using one of the bits between S_IFMT and normal permissions
(0777) for that, e.g. the sticky bit.
That all said, the longer I think about mode overflow the less I
understand why I sent these patches. It's basically harmless. Perhaps
we just need a comment stating that, to contain the urge to "fix" this.
Anyway, I'd like to retract my overflowing modes patches (unless someone
else really, really wants one of them applied).
René
^ permalink raw reply
* Re: [PATCH v2.5 02/11] bundle: verify using connected()
From: Derrick Stolee @ 2023-01-24 18:46 UTC (permalink / raw)
To: Junio C Hamano
Cc: Derrick Stolee via GitGitGadget, git, me, vdye, avarab, steadmon,
chooglen
In-Reply-To: <xmqqfsbzhm7h.fsf@gitster.g>
On 1/24/2023 12:33 PM, Junio C Hamano wrote:
> Derrick Stolee <derrickstolee@github.com> writes:
>
>> When Git verifies a bundle to see if it is safe for unbundling, it first
>> looks to see if the prerequisite commits are in the object store. This
>> is usually a sufficient filter, and those missing commits are indicated
>> clearly in the error messages.
>
> I am not sure if our early check is because "does the prerequisite
> even exist?" is sufficient. It is a short-cut that is cheap and can
> be done without preparing the commit traversal.
I suppose I should say "Usually, existence in the object store is
correlated with having all reachable objects, but this is not
guaranteed." I'll also mention that it is a short-cut that can fail
faster than the reachability check.
>> This check is more strict than what Git applies even to fetched
>> pack-files.
>
> I do not see the need to say "even" here. In what other situation
> do we make connectivity checks, and is there a need to be more
> strict than others when checking fetched packfiles?
I suppose that I was implying that fetches are the more common
operation, and the scrutiny applied to an arbitrary pack-file from
a remote is probably higher there. However, who knows where a
bundle came from, so the scrutiny should be the same.
>> To better align with the restrictions required by 'git fetch',
>> reimplement this check in verify_bundle() to use check_connected(). This
>> also simplifies the code significantly.
>
> Wonderful. I never liked the custom check done in unbundle code,
> which I am reasonably sure came from scripted hack to unbundle I
> wrote eons ago.
Excellent. Thanks for your feedback.
-Stolee
^ permalink raw reply
* Re: [PATCH v7 00/12] Enhance credential helper protocol to include auth headers
From: Junio C Hamano @ 2023-01-24 18:03 UTC (permalink / raw)
To: Victoria Dye
Cc: Matthew John Cheetham via GitGitGadget, git, Derrick Stolee,
Lessley Dennington, Matthew John Cheetham, M Hickford,
Jeff Hostetler, Glen Choo, Ævar Arnfjörð Bjarmason
In-Reply-To: <e57c1ca3-c21c-db41-a386-e5887f46055c@github.com>
Victoria Dye <vdye@github.com> writes:
> Matthew John Cheetham via GitGitGadget wrote:
>> Updates in v6
>> =============
>> ...
> I've re-read the patches in this version; all of my comments from v5 have
> been addressed, and the additional updates w.r.t. other reviewer feedback
> all look good as well. At this point, I think the series is ready for
> 'next'.
>
> Thanks!
Thanks, both. Let's merge it down.
^ permalink raw reply
* Re: [PATCH v2] ssh signing: better error message when key not in agent
From: Junio C Hamano @ 2023-01-24 17:52 UTC (permalink / raw)
To: Adam Szkoda via GitGitGadget
Cc: git, Phillip Wood, Adam Szkoda, Fabian Stelzer
In-Reply-To: <pull.1270.v2.git.git.1674573972087.gitgitgadget@gmail.com>
"Adam Szkoda via GitGitGadget" <gitgitgadget@gmail.com> writes:
> 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>
> ---
Well explained.
> 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?
> 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. 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.
The insertion of "-U" is a single statement as the body of a if()
statement. We do not want {} around it, by the way.
Other than that, nicely done. Thanks.
> sigchain_push(SIGPIPE, SIG_IGN);
> ret = pipe_command(&signer, NULL, 0, NULL, 0, &signer_stderr, 0);
>
> base-commit: 844ede312b4e988881b6e27e352f469d8ab80b2a
^ permalink raw reply
* Re: [PATCH v2.5 02/11] bundle: verify using connected()
From: Junio C Hamano @ 2023-01-24 17:33 UTC (permalink / raw)
To: Derrick Stolee
Cc: Derrick Stolee via GitGitGadget, git, me, vdye, avarab, steadmon,
chooglen
In-Reply-To: <ecc6b167-f5c4-48ce-3973-461d1659ed40@github.com>
Derrick Stolee <derrickstolee@github.com> writes:
> When Git verifies a bundle to see if it is safe for unbundling, it first
> looks to see if the prerequisite commits are in the object store. This
> is usually a sufficient filter, and those missing commits are indicated
> clearly in the error messages.
I am not sure if our early check is because "does the prerequisite
even exist?" is sufficient. It is a short-cut that is cheap and can
be done without preparing the commit traversal.
> However, if the commits are present in
> the object store, then there could still be issues if those commits are
> not reachable from the repository's references. The repository only has
> guarantees that its object store is closed under reachability for the
> objects that are reachable from references.
Correct.
> Thus, the code in verify_bundle() has previously had the additional
> check that all prerequisite commits are reachable from repository
> references. This is done via a revision walk from all references,
> stopping only if all prerequisite commits are discovered or all commits
> are walked. This uses a custom walk to verify_bundle().
Correct.
> This check is more strict than what Git applies even to fetched
> pack-files.
I do not see the need to say "even" here. In what other situation
do we make connectivity checks, and is there a need to be more
strict than others when checking fetched packfiles?
> In the fetch case, Git guarantees that the new references
> are closed under reachability by walking from the new references until
> walking commits that are reachable from repository refs. This is done
> through the well-used check_connected() method.
Correct and is a good point to make.
> To better align with the restrictions required by 'git fetch',
> reimplement this check in verify_bundle() to use check_connected(). This
> also simplifies the code significantly.
Wonderful. I never liked the custom check done in unbundle code,
which I am reasonably sure came from scripted hack to unbundle I
wrote eons ago.
^ permalink raw reply
* Re: [PATCH v7 00/12] Enhance credential helper protocol to include auth headers
From: Victoria Dye @ 2023-01-24 17:30 UTC (permalink / raw)
To: Matthew John Cheetham via GitGitGadget, git
Cc: Derrick Stolee, Lessley Dennington, Matthew John Cheetham,
M Hickford, Jeff Hostetler, Glen Choo,
Ævar Arnfjörð Bjarmason
In-Reply-To: <pull.1352.v7.git.1674252530.gitgitgadget@gmail.com>
Matthew John Cheetham via GitGitGadget wrote:
> Updates in v6
> =============
>
> * Clarify the change to make logging optional in the check_dead_children()
> function during libification of daemon.c.
>
> * Fix missing pointer dereference bugs identified in libification of child
> process handling functions for daemon.c.
>
> * Add doc comments to child process handling function declarations in the
> daemon-utils.h header.
>
> * Align function parameter names with variable names at callsites for
> libified daemon functions.
>
> * Re-split out the test-http-server test helper commits in to smaller
> patches: error response handling, request parsing, http-backend
> pass-through, simple authentication, arbitrary header support.
>
> * Call out auth configuration file format for test-http-server test helper
> and supported options in commit messages, as well as a test to exercise
> and demonstrate these options.
>
> * Permit auth.token and auth.challenge to appear in any order; create the
> struct auth_module just-in-time as options for that scheme are read. This
> simplifies the configuration authoring of the test-http-server test
> helper.
>
> * Update tests to use auth.allowAnoymous in the patch that introduces the
> new test helper option.
>
> * Drop the strvec_push_nodup() commit and update the implementation of HTTP
> request header line folding to use xstrdup and strvec_pop and _pushf.
>
> * Use size_t instead of int in credential.c when iterating over the struct
> strvec credential members. Also drop the not required const and cast from
> the full_key definition and free.
>
> * Replace in-tree test-credential-helper-reply.sh test cred helper script
> with the lib-credential-helper.sh reusable 'lib' test script and shell
> functions to configure the helper behaviour.
>
> * Leverage sed over the while read $line loop in the test credential helper
> script.
>
>
> Updates in v7
> =============
>
> * Address several whitespace and arg/param list alignment issues.
>
> * Rethink the test-http-helper worker-mode error and result enum to be more
> simple and more informative to the nature of the error.
>
> * Use uintmax_t to store the Content-Length of a request in the helper
> test-http-server. Maintain a bit flag to store if we received such a
> header.
>
> * Return a "400 Bad Request" HTTP response if we fail to parse the request
> in the test-http-server.
>
> * Add test case to cover request message parsing in test-http-server.
>
> * Use size_t and ALLOC_ARRAY over int and CALLOC_ARRAY respectively in
> get_auth_module.
>
> * Correctly free the split strbufs created in the header parsing loop in
> test-http-server.
>
> * Avoid needless comparison > 0 for unsigned types.
>
> * Always set optional outputs to NULL if not present in test helper config
> value handling.
>
> * Remove an accidentally commented-out test cleanup line for one test case
> in t5556.
I've re-read the patches in this version; all of my comments from v5 have
been addressed, and the additional updates w.r.t. other reviewer feedback
all look good as well. At this point, I think the series is ready for
'next'.
Thanks!
^ permalink raw reply
* Re: [PATCH v2.5 01/11] bundle: test unbundling with incomplete history
From: Junio C Hamano @ 2023-01-24 17:16 UTC (permalink / raw)
To: Derrick Stolee
Cc: Derrick Stolee via GitGitGadget, git, me, vdye, avarab, steadmon,
chooglen
In-Reply-To: <01f97aff-58a1-ef2c-e668-d37ea513c64e@github.com>
Derrick Stolee <derrickstolee@github.com> writes:
> In order to construct a broken history, perform a shallow clone of a
> repository with a linear history, but whose default branch ('base') has
> a single commit, so dropping the shallow markers leaves a complete
> history from that reference. However, the 'tip' reference adds a
> shallow commit whose parent is missing in the cloned repository. Trying
> to unbundle a bundle with the 'tip' as a prerequisite will succeed past
> the object store check and move into the reachability check.
It makes it sound convoluted set-up for tests, but I guess it is the
most direct way to get to the state you want to test, which is good.
In practice, the problem would appear when you create a multi-commit
branch, which then is discarded. GC then decides to expire the
older part of the commit chain while leaving the commits near the
tip still in the object store. So the problem can happen without
users doing anything esoteric, and is very much worth testing.
> +test_expect_success 'verify catches unreachable, broken prerequisites' '
> + test_when_finished rm -rf clone-from clone-to &&
OK, so my understanding of what happens is ...
> + git init clone-from &&
> + (
> + cd clone-from &&
> + git checkout -b base &&
> + test_commit A &&
> + git checkout -b tip &&
> + git commit --allow-empty -m "will drop by shallow" &&
> + git commit --allow-empty -m "will keep by shallow" &&
> + git commit --allow-empty -m "for bundle, not clone" &&
> + git bundle create tip.bundle tip~1..tip &&
... there is a single strand of pearls
A---D---K---B tip
where D is with "will drop by shallow" message. The bundle
is prepared to give a history leading to B while requiring K.
> + git reset --hard HEAD~1 &&
> + git checkout base
Then B is thrown away before the history is cloned.
> + ) &&
> + BAD_OID=$(git -C clone-from rev-parse tip~1) &&
> + TIP_OID=$(git -C clone-from rev-parse tip) &&
> + git clone --depth=1 --no-single-branch \
> + "file://$(pwd)/clone-from" clone-to &&
> + (
> + cd clone-to &&
The cloned repository should have
A---d---K
where D is missing behind the shallow boundary, origin/tip pointing
at K.
> + # Set up broken history by removing shallow markers
> + git update-ref -d refs/remotes/origin/tip &&
But we remove origin/tip, so K (and its trees and blobs) is totally
disconnected.
> + rm .git/shallow &&
And then this removes the shallow info that makes us to pretend that
K does not have D (missing) as its parent. Now we lack the required
parent D if we start traversing from K.
> + # Verify should fail
> + test_must_fail git bundle verify \
> + ../clone-from/tip.bundle 2>err &&
verify_bundle() wants to see traversal from "--all" to hit the
prerequisite objects and K certainly cannot be reached by any ref.
OK. So we ended up with a repository where we are on 'base' branch,
and origin/HEAD and origin/base remote-tracking refs exist, all of
these refs pointing at A. Plus K exists but not D, but it is fine
because K is not referenced by any ref.
This is perfectly constructed test case that checks a very
interesting scenario. It is as if the commit chain D---K was
discarded (via "git branch -D") and then D got expired for being too
old but K is not old enough.
We want to ensure "git bundle verify" and "git fetch ./bundle.file"
in this healthy repository, where its refs do honor the promise, but
its object store has unconnected commits (like "K") that are not
complete, behaves sensibly. If we loosen "prerequisites must be
reachable from refs" to "prerequisites must exist", it will lead to
repository corruption if we allow the bundle to be unbundled and its
tips made into our refs, because these new refs point at incomplete
objects.
Excellent.
> + # Unbundling should fail
> + test_must_fail git bundle unbundle \
> + ../clone-from/tip.bundle 2>err &&
> + grep "Could not read $BAD_OID" err &&
> + grep "Failed to traverse parents of commit $TIP_OID" err
> + )
> +'
^ permalink raw reply
* Re: [PATCH v4 5/9] rebase: add coverage of other incompatible options
From: Elijah Newren @ 2023-01-24 17:12 UTC (permalink / raw)
To: phillip.wood
Cc: Elijah Newren via GitGitGadget, git, Derrick Stolee,
Eric Sunshine, Martin Ågren, Phillip Wood
In-Reply-To: <0418e5b6-8cbd-9dc9-085e-31380beda89b@dunelm.org.uk>
On Tue, Jan 24, 2023 at 8:48 AM Phillip Wood <phillip.wood123@gmail.com> wrote:
>
> Hi Elijah
>
> On 24/01/2023 15:41, Elijah Newren wrote:
[...]
> >> - /*
> >> - * --keep-base implements --reapply-cherry-picks by altering
> >> upstream so
> >> - * it works with both backends.
> >> - */
> >> - if (options.reapply_cherry_picks && !keep_base)
> >> + if (options.reapply_cherry_picks < 0)
> >> + /*
> >> + * --keep-base defaults to --reapply-cherry-picks to
> >> + * avoid losing commits when using this option.
> >> + */
> >
> > I know you were copying the previous comment, but this comment is
> > really confusing to me. Shouldn't it read "--reapply-cherry-picks
> > defaults to --keep-base" instead of vice-versa?
>
> Clearly this comment has not been as helpful as I indented it to be. I
> think maybe we should spell out the defaults with and without
> --keep-base. Perhaps something like
>
> default to --no-reapply-cherry-picks unless --keep-base is given in
> which case default to --reapply-cherry-picks
I like that; sounds good to me.
> >> + options.reapply_cherry_picks = keep_base;
> >> + else if (!keep_base)
> >> + /*
> >> + * --keep-base implements --reapply-cherry-picks by
> >
> > Should this be --[no-]reapply-cherry-picks, to clarify that it handles
> > both cases? Especially given how many times I missed it?
>
> This has obviously proved to be confusing. The aim was to explain that
> in order to work with the apply backend "[--reapply-cherry-picks]
> --keep-base" was doing something unusual with `upstream` to reapply
> cherry picks. "--no-reapply-cherry-picks --keep-base" does not do
> anything unusual with `upstream`. I don't think changing it to
> --[no-]reapply-cherry-picks quite captures that. I came up with
>
> To support --reapply-cherry-picks (which is not supported by the apply
> backend) --keep-base alters upstream to prevent cherry picked commits
> from being dropped.
>
> but it really needs to mention that --keep-base also supports
> --no-reapply-cherry-picks in the usual way
Somewhat wordy, but perhaps:
/*
* 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.
*/
?
^ permalink raw reply
* Re: [PATCH v4 5/9] rebase: add coverage of other incompatible options
From: Phillip Wood @ 2023-01-24 16:48 UTC (permalink / raw)
To: Elijah Newren
Cc: Elijah Newren via GitGitGadget, git, Derrick Stolee,
Eric Sunshine, Martin Ågren, Phillip Wood
In-Reply-To: <CABPp-BHVUc7EdY9z_TPcHspCak6Yc3mfDXUkivj4zq_fJem3SQ@mail.gmail.com>
Hi Elijah
On 24/01/2023 15:41, Elijah Newren wrote:
> Ah, despite looking over the code multiple times to check my
> statements, I somehow kept missing this:
>
> if (keep_base && options.reapply_cherry_picks)
> options.upstream = options.onto;
>
> which is how --[no-]reapply-cherry-picks is supported in conjunction
> with --keep-base. Thanks.
>
>>>> For all four cases (A)-(D), the apply backend will ignore whatever
>>>> --[no-]reapply-cherry-picks flag is provided.
>>>
>>> For (D) the flag is respected, (C) errors out, the other cases
>>> correspond to the default so it's like saying
>>>
>>> git rebase --merge --no-reapply-cherry-picks
>>>
>>> ignores the flag.
>>
>> On reflection that is only true for (B). I agree that we should error
>> out on (A) which we don't at the moment.
>>
>> I'd support a change that errors out on (A) and (C) but continues to
>> allow (B) and (D). I think we can do that with the diff below
>>
>>
>> diff --git a/builtin/rebase.c b/builtin/rebase.c
>> index 1481c5b6a5..66aef356b8 100644
>> --- a/builtin/rebase.c
>> +++ b/builtin/rebase.c
>> @@ -1230,12 +1230,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");
>> @@ -1412,11 +1406,17 @@ 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)
>> + if (options.reapply_cherry_picks < 0)
>> + /*
>> + * --keep-base defaults to --reapply-cherry-picks to
>> + * avoid losing commits when using this option.
>> + */
>
> I know you were copying the previous comment, but this comment is
> really confusing to me. Shouldn't it read "--reapply-cherry-picks
> defaults to --keep-base" instead of vice-versa?
Clearly this comment has not been as helpful as I indented it to be. I
think maybe we should spell out the defaults with and without
--keep-base. Perhaps something like
default to --no-reapply-cherry-picks unless --keep-base is given in
which case default to --reapply-cherry-picks
>> + options.reapply_cherry_picks = keep_base;
>> + else if (!keep_base)
>> + /*
>> + * --keep-base implements --reapply-cherry-picks by
>
> Should this be --[no-]reapply-cherry-picks, to clarify that it handles
> both cases? Especially given how many times I missed it?
This has obviously proved to be confusing. The aim was to explain that
in order to work with the apply backend "[--reapply-cherry-picks]
--keep-base" was doing something unusual with `upstream` to reapply
cherry picks. "--no-reapply-cherry-picks --keep-base" does not do
anything unusual with `upstream`. I don't think changing it to
--[no-]reapply-cherry-picks quite captures that. I came up with
To support --reapply-cherry-picks (which is not supported by the apply
backend) --keep-base alters upstream to prevent cherry picked commits
from being dropped.
but it really needs to mention that --keep-base also supports
--no-reapply-cherry-picks in the usual way
>> + * altering upstream so it works with both backends.
>> + */
>> imply_merge(&options, "--reapply-cherry-picks");
>
> And perhaps this should be
>
> imply_merge(&options, options.reapply_cherry_picks ?
> "--reapply-cherry-picks" :
> "--no-reapply-cherry-picks");
That's a good idea
> Also, the comment in git-rebase.txt about incompatibilities would become
>
> * --[no-]reapply-cherry-picks, when --keep-base is not provided
Yes, that's good
Thanks again for working on this
Phillip
^ permalink raw reply
* Re: GSoC 2023
From: Christian Couder @ 2023-01-24 16:38 UTC (permalink / raw)
To: Kaartic Sivaraam; +Cc: git, Derrick Stolee, Victoria Dye, Hariom verma
In-Reply-To: <d8ce0159-c9dc-25c2-4180-70518bb31bfc@gmail.com>
Hi,
On Mon, Jan 23, 2023 at 6:41 PM Kaartic Sivaraam
<kaartic.sivaraam@gmail.com> wrote:
> On 17/01/23 15:04, Christian Couder wrote:
> >
> > GSoC Org Applications open next week on Monday, January 23rd at 1800
> > UTC and close on Tuesday, February 7th at 1800 UTC.
> >
> > I am interested in mentoring and being an org admin for Git again this
> > year, so I plan to apply for Git soon.
>
> I would be glad to help as an Org admin this year. I don't suppose I
> have the capacity to mentor / co-mentor this year, though. I could very
> well help as much as I can passively where needed.
You are welcome to be an Org Admin, thanks!
Actually you were already an Org Admin last year and it looks like
they didn't remove people from the roles they had last year, so you
are still an Org Admin.
> One thing to note about the Org application. As per Google's claim we
> should be able to complete this year's application quickly since the new
> webapp allows us to reuse last year's application details.
Yeah, I just did it and reusing last year's application details was
quite quick. One needs to read, agree on and accept 3 long documents,
and they still ask a few questions about last year's GSoC though.
They also want an URL with an idea list, so I quickly copied and
edited last year's idea list into this one:
https://git.github.io/SoC-2023-Ideas/
I removed the "Reachability bitmap improvements" idea but left the 2 others:
- More Sparse Index Integrations (I removed `git mv` in the list of
commands that need to be improved though)
- Unify ref-filter formats with other pretty formats
On both of them I removed all possible mentors except me though. They
are welcome to tell me that they should be added back.
> We certainly are in need of ideas and mentors for this year. Do chime in
> with your thoughts. :-)
Yeah, sure.
^ 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