* [PATCH v4 2/4] rebase: support --autosquash without -i
From: Andy Koppe @ 2023-11-11 13:27 UTC (permalink / raw)
To: git; +Cc: gitster, newren, Andy Koppe
In-Reply-To: <20231111132720.78877-1-andy.koppe@gmail.com>
The --autosquash option prevents preemptive fast-forwarding and triggers
conflicts with amend backend options, yet it only actually performs
auto-squashing when combined with the --interactive (or -i) option.
Remove the latter restriction and tweak the --autosquash description
accordingly.
Signed-off-by: Andy Koppe <andy.koppe@gmail.com>
---
Documentation/git-rebase.txt | 2 +-
builtin/rebase.c | 4 +---
2 files changed, 2 insertions(+), 4 deletions(-)
diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
index b4526ca246..10548e715c 100644
--- a/Documentation/git-rebase.txt
+++ b/Documentation/git-rebase.txt
@@ -592,7 +592,7 @@ See also INCOMPATIBLE OPTIONS below.
When the commit log message begins with "squash! ..." or "fixup! ..."
or "amend! ...", and there is already a commit in the todo list that
matches the same `...`, automatically modify the todo list of
- `rebase -i`, so that the commit marked for squashing comes right after
+ `rebase`, so that the commit marked for squashing comes right after
the commit to be modified, and change the action of the moved commit
from `pick` to `squash` or `fixup` or `fixup -C` respectively. A commit
matches the `...` if the commit subject matches, or if the `...` refers
diff --git a/builtin/rebase.c b/builtin/rebase.c
index a73de7892b..9f8192e0a5 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -710,10 +710,8 @@ static int run_specific_rebase(struct rebase_options *opts)
if (opts->type == REBASE_MERGE) {
/* Run sequencer-based rebase */
setenv("GIT_CHERRY_PICK_HELP", resolvemsg, 1);
- if (!(opts->flags & REBASE_INTERACTIVE_EXPLICIT)) {
+ if (!(opts->flags & REBASE_INTERACTIVE_EXPLICIT))
setenv("GIT_SEQUENCE_EDITOR", ":", 1);
- opts->autosquash = 0;
- }
if (opts->gpg_sign_opt) {
/* remove the leading "-S" */
char *tmp = xstrdup(opts->gpg_sign_opt + 2);
--
2.43.0-rc1
^ permalink raw reply related
* [PATCH v4 1/4] rebase: fully ignore rebase.autoSquash without -i
From: Andy Koppe @ 2023-11-11 13:27 UTC (permalink / raw)
To: git; +Cc: gitster, newren, Andy Koppe
In-Reply-To: <20231111132720.78877-1-andy.koppe@gmail.com>
Setting the rebase.autoSquash config variable to true implies a couple
of restrictions: it prevents preemptive fast-forwarding and it triggers
conflicts with amend backend options. However, it only actually results
in auto-squashing when combined with the --interactive (or -i) option,
due to code in run_specific_rebase() that disables auto-squashing unless
the REBASE_INTERACTIVE_EXPLICIT flag is set.
Doing autosquashing for rebase.autoSquash without --interactive would be
problematic in terms of backward compatibility, but conversely, there is
no need for the aforementioned restrictions without --interactive.
So drop the options.config_autosquash check from the conditions for
clearing allow_preemptive_ff, as the case where it is combined with
--interactive is already covered by the REBASE_INTERACTIVE_EXPLICIT
flag check above it.
Also drop the "apply options are incompatible with rebase.autoSquash"
error, because it is unreachable if it is restricted to --interactive,
as apply options already cause an error when used with --interactive.
Drop the tests for the error from t3422-rebase-incompatible-options.sh,
which has separate tests for the conflicts of --interactive with apply
options.
When neither --autosquash nor --no-autosquash are given, only set
options.autosquash to true if rebase.autosquash is combined with
--interactive.
Don't initialize options.config_autosquash to -1, as there is no need to
distinguish between rebase.autoSquash being unset or explicitly set to
false.
Finally, amend the rebase.autoSquash documentation to say it only
affects interactive mode.
Signed-off-by: Andy Koppe <andy.koppe@gmail.com>
---
Documentation/config/rebase.txt | 4 +++-
builtin/rebase.c | 13 ++++++-------
t/t3422-rebase-incompatible-options.sh | 12 ------------
3 files changed, 9 insertions(+), 20 deletions(-)
diff --git a/Documentation/config/rebase.txt b/Documentation/config/rebase.txt
index 9c248accec..d59576dbb2 100644
--- a/Documentation/config/rebase.txt
+++ b/Documentation/config/rebase.txt
@@ -9,7 +9,9 @@ rebase.stat::
rebase. False by default.
rebase.autoSquash::
- If set to true enable `--autosquash` option by default.
+ If set to true, enable the `--autosquash` option of
+ linkgit:git-rebase[1] by default for interactive mode.
+ This can be overridden with the `--no-autosquash` option.
rebase.autoStash::
When set to true, automatically create a temporary stash entry
diff --git a/builtin/rebase.c b/builtin/rebase.c
index 043c65dccd..a73de7892b 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -149,7 +149,6 @@ struct rebase_options {
.reapply_cherry_picks = -1, \
.allow_empty_message = 1, \
.autosquash = -1, \
- .config_autosquash = -1, \
.rebase_merges = -1, \
.config_rebase_merges = -1, \
.update_refs = -1, \
@@ -1405,7 +1404,6 @@ 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 == -1 && options.config_autosquash == 1) ||
options.autosquash == 1) {
allow_preemptive_ff = 0;
}
@@ -1508,8 +1506,6 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
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.rebase_merges == -1 && options.config_rebase_merges == 1)
die(_("apply options are incompatible with rebase.rebaseMerges. Consider adding --no-rebase-merges"));
else if (options.update_refs == -1 && options.config_update_refs == 1)
@@ -1529,10 +1525,13 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
options.rebase_merges = (options.rebase_merges >= 0) ? options.rebase_merges :
((options.config_rebase_merges >= 0) ? options.config_rebase_merges : 0);
- if (options.autosquash == 1)
+ if (options.autosquash == 1) {
imply_merge(&options, "--autosquash");
- options.autosquash = (options.autosquash >= 0) ? options.autosquash :
- ((options.config_autosquash >= 0) ? options.config_autosquash : 0);
+ } else if (options.autosquash == -1) {
+ options.autosquash =
+ options.config_autosquash &&
+ (options.flags & REBASE_INTERACTIVE_EXPLICIT);
+ }
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 2eba00bdf5..b40f26250b 100755
--- a/t/t3422-rebase-incompatible-options.sh
+++ b/t/t3422-rebase-incompatible-options.sh
@@ -100,12 +100,6 @@ test_rebase_am_only () {
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.rebaseMerges" "
git checkout B^0 &&
test_must_fail git -c rebase.rebaseMerges=true rebase $opt A 2>err &&
@@ -118,12 +112,6 @@ test_rebase_am_only () {
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.rebaseMerges" "
test_when_finished \"git reset --hard B^0\" &&
git checkout B^0 &&
--
2.43.0-rc1
^ permalink raw reply related
* [PATCH v4 4/4] docs: rewrite rebase --(no-)autosquash description
From: Andy Koppe @ 2023-11-11 13:27 UTC (permalink / raw)
To: git; +Cc: gitster, newren, Andy Koppe
In-Reply-To: <20231111132720.78877-1-andy.koppe@gmail.com>
Rewrite the description of the rebase --(no-)autosquash options to try
to make it a bit clearer. Don't use "the '...'" to refer to part of a
commit message, mention how --interactive can be used to review the
todo list, and add a bit more detail on commit --squash/amend.
Signed-off-by: Andy Koppe <andy.koppe@gmail.com>
---
Documentation/git-rebase.txt | 34 ++++++++++++++++++++--------------
1 file changed, 20 insertions(+), 14 deletions(-)
diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
index 10548e715c..1dd6555f66 100644
--- a/Documentation/git-rebase.txt
+++ b/Documentation/git-rebase.txt
@@ -589,21 +589,27 @@ See also INCOMPATIBLE OPTIONS below.
--autosquash::
--no-autosquash::
- When the commit log message begins with "squash! ..." or "fixup! ..."
- or "amend! ...", and there is already a commit in the todo list that
- matches the same `...`, automatically modify the todo list of
- `rebase`, so that the commit marked for squashing comes right after
- the commit to be modified, and change the action of the moved commit
- from `pick` to `squash` or `fixup` or `fixup -C` respectively. A commit
- matches the `...` if the commit subject matches, or if the `...` refers
- to the commit's hash. As a fall-back, partial matches of the commit
- subject work, too. The recommended way to create fixup/amend/squash
- commits is by using the `--fixup`, `--fixup=amend:` or `--fixup=reword:`
- and `--squash` options respectively of linkgit:git-commit[1].
+ Automatically squash commits with specially formatted messages into
+ previous commits being rebased. If a commit message starts with
+ "squash! ", "fixup! " or "amend! ", the remainder of the subject line
+ is taken as a commit specifier, which matches a previous commit if it
+ matches the subject line or the hash of that commit. If no commit
+ matches fully, matches of the specifier with the start of commit
+ subjects are considered.
+
-If the `--autosquash` option is enabled by default using the
-configuration variable `rebase.autoSquash`, this option can be
-used to override and disable this setting.
+In the rebase todo list, the actions of squash, fixup and amend commits are
+changed from `pick` to `squash`, `fixup` or `fixup -C`, respectively, and they
+are moved right after the commit they modify. The `--interactive` option can
+be used to review and edit the todo list before proceeding.
++
+The recommended way to create commits with squash markers is by using the
+`--squash`, `--fixup`, `--fixup=amend:` or `--fixup=reword:` options of
+linkgit:git-commit[1], which take the target commit as an argument and
+automatically fill in the subject line of the new commit from that.
++
+Settting configuration variable `rebase.autoSquash` to true enables
+auto-squashing by default for interactive rebase. The `--no-autosquash`
+option can be used to override that setting.
+
See also INCOMPATIBLE OPTIONS below.
--
2.43.0-rc1
^ permalink raw reply related
* [PATCH v4 3/4] rebase: test autosquash with and without -i
From: Andy Koppe @ 2023-11-11 13:27 UTC (permalink / raw)
To: git; +Cc: gitster, newren, Andy Koppe
In-Reply-To: <20231111132720.78877-1-andy.koppe@gmail.com>
Amend t3415-rebase-autosquash.sh to test the --autosquash option and
rebase.autoSquash config with and without -i.
Signed-off-by: Andy Koppe <andy.koppe@gmail.com>
---
t/t3415-rebase-autosquash.sh | 38 ++++++++++++++++++++++++++----------
1 file changed, 28 insertions(+), 10 deletions(-)
diff --git a/t/t3415-rebase-autosquash.sh b/t/t3415-rebase-autosquash.sh
index a364530d76..fcc40d6fe1 100755
--- a/t/t3415-rebase-autosquash.sh
+++ b/t/t3415-rebase-autosquash.sh
@@ -43,7 +43,7 @@ test_auto_fixup () {
git tag $1 &&
test_tick &&
- git rebase $2 -i HEAD^^^ &&
+ git rebase $2 HEAD^^^ &&
git log --oneline >actual &&
if test -n "$no_squash"
then
@@ -61,15 +61,24 @@ test_auto_fixup () {
}
test_expect_success 'auto fixup (option)' '
- test_auto_fixup final-fixup-option --autosquash
+ test_auto_fixup fixup-option --autosquash &&
+ test_auto_fixup fixup-option-i "--autosquash -i"
'
-test_expect_success 'auto fixup (config)' '
+test_expect_success 'auto fixup (config true)' '
git config rebase.autosquash true &&
- test_auto_fixup final-fixup-config-true &&
+ test_auto_fixup ! fixup-config-true &&
+ test_auto_fixup fixup-config-true-i -i &&
test_auto_fixup ! fixup-config-true-no --no-autosquash &&
+ test_auto_fixup ! fixup-config-true-i-no "-i --no-autosquash"
+'
+
+test_expect_success 'auto fixup (config false)' '
git config rebase.autosquash false &&
- test_auto_fixup ! final-fixup-config-false
+ test_auto_fixup ! fixup-config-false &&
+ test_auto_fixup ! fixup-config-false-i -i &&
+ test_auto_fixup fixup-config-false-yes --autosquash &&
+ test_auto_fixup fixup-config-false-i-yes "-i --autosquash"
'
test_auto_squash () {
@@ -87,7 +96,7 @@ test_auto_squash () {
git commit -m "squash! first" -m "extra para for first" &&
git tag $1 &&
test_tick &&
- git rebase $2 -i HEAD^^^ &&
+ git rebase $2 HEAD^^^ &&
git log --oneline >actual &&
if test -n "$no_squash"
then
@@ -105,15 +114,24 @@ test_auto_squash () {
}
test_expect_success 'auto squash (option)' '
- test_auto_squash final-squash --autosquash
+ test_auto_squash squash-option --autosquash &&
+ test_auto_squash squash-option-i "--autosquash -i"
'
-test_expect_success 'auto squash (config)' '
+test_expect_success 'auto squash (config true)' '
git config rebase.autosquash true &&
- test_auto_squash final-squash-config-true &&
+ test_auto_squash ! squash-config-true &&
+ test_auto_squash squash-config-true-i -i &&
test_auto_squash ! squash-config-true-no --no-autosquash &&
+ test_auto_squash ! squash-config-true-i-no "-i --no-autosquash"
+'
+
+test_expect_success 'auto squash (config false)' '
git config rebase.autosquash false &&
- test_auto_squash ! final-squash-config-false
+ test_auto_squash ! squash-config-false &&
+ test_auto_squash ! squash-config-false-i -i &&
+ test_auto_squash squash-config-false-yes --autosquash &&
+ test_auto_squash squash-config-false-i-yes "-i --autosquash"
'
test_expect_success 'misspelled auto squash' '
--
2.43.0-rc1
^ permalink raw reply related
* [PATCH v4 4/4] rebase: rewrite --(no-)autosquash documentation
From: Andy Koppe @ 2023-11-11 13:27 UTC (permalink / raw)
To: git; +Cc: gitster, newren, Andy Koppe
In-Reply-To: <20231111132720.78877-1-andy.koppe@gmail.com>
Rewrite the description of the rebase --(no-)autosquash options to try
to make it a bit clearer. Don't use "the '...'" to refer to part of a
commit message, mention how --interactive can be used to review the
todo list, and add a bit more detail on commit --squash/amend.
Signed-off-by: Andy Koppe <andy.koppe@gmail.com>
---
Documentation/git-rebase.txt | 34 ++++++++++++++++++++--------------
1 file changed, 20 insertions(+), 14 deletions(-)
diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
index 10548e715c..1dd6555f66 100644
--- a/Documentation/git-rebase.txt
+++ b/Documentation/git-rebase.txt
@@ -589,21 +589,27 @@ See also INCOMPATIBLE OPTIONS below.
--autosquash::
--no-autosquash::
- When the commit log message begins with "squash! ..." or "fixup! ..."
- or "amend! ...", and there is already a commit in the todo list that
- matches the same `...`, automatically modify the todo list of
- `rebase`, so that the commit marked for squashing comes right after
- the commit to be modified, and change the action of the moved commit
- from `pick` to `squash` or `fixup` or `fixup -C` respectively. A commit
- matches the `...` if the commit subject matches, or if the `...` refers
- to the commit's hash. As a fall-back, partial matches of the commit
- subject work, too. The recommended way to create fixup/amend/squash
- commits is by using the `--fixup`, `--fixup=amend:` or `--fixup=reword:`
- and `--squash` options respectively of linkgit:git-commit[1].
+ Automatically squash commits with specially formatted messages into
+ previous commits being rebased. If a commit message starts with
+ "squash! ", "fixup! " or "amend! ", the remainder of the subject line
+ is taken as a commit specifier, which matches a previous commit if it
+ matches the subject line or the hash of that commit. If no commit
+ matches fully, matches of the specifier with the start of commit
+ subjects are considered.
+
-If the `--autosquash` option is enabled by default using the
-configuration variable `rebase.autoSquash`, this option can be
-used to override and disable this setting.
+In the rebase todo list, the actions of squash, fixup and amend commits are
+changed from `pick` to `squash`, `fixup` or `fixup -C`, respectively, and they
+are moved right after the commit they modify. The `--interactive` option can
+be used to review and edit the todo list before proceeding.
++
+The recommended way to create commits with squash markers is by using the
+`--squash`, `--fixup`, `--fixup=amend:` or `--fixup=reword:` options of
+linkgit:git-commit[1], which take the target commit as an argument and
+automatically fill in the subject line of the new commit from that.
++
+Settting configuration variable `rebase.autoSquash` to true enables
+auto-squashing by default for interactive rebase. The `--no-autosquash`
+option can be used to override that setting.
+
See also INCOMPATIBLE OPTIONS below.
--
2.43.0-rc1
^ permalink raw reply related
* Re: [PATCH v4 4/4] docs: rewrite rebase --(no-)autosquash description
From: Andy Koppe @ 2023-11-11 13:33 UTC (permalink / raw)
To: git; +Cc: gitster, newren
In-Reply-To: <20231111132720.78877-5-andy.koppe@gmail.com>
Disregard this one in favour of the other patch 4/4 please:
"[PATCH v4 4/4] rebase: rewrite --(no-)autosquash documentation"
Sorry I failed to notice that I still had this lying around from the
previous iteration.
On 11/11/2023 13:27, Andy Koppe wrote:
> Rewrite the description of the rebase --(no-)autosquash options to try
> to make it a bit clearer. Don't use "the '...'" to refer to part of a
> commit message, mention how --interactive can be used to review the
> todo list, and add a bit more detail on commit --squash/amend.
>
> Signed-off-by: Andy Koppe <andy.koppe@gmail.com>
> ---
> Documentation/git-rebase.txt | 34 ++++++++++++++++++++--------------
> 1 file changed, 20 insertions(+), 14 deletions(-)
>
> diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
> index 10548e715c..1dd6555f66 100644
> --- a/Documentation/git-rebase.txt
> +++ b/Documentation/git-rebase.txt
> @@ -589,21 +589,27 @@ See also INCOMPATIBLE OPTIONS below.
>
> --autosquash::
> --no-autosquash::
> - When the commit log message begins with "squash! ..." or "fixup! ..."
> - or "amend! ...", and there is already a commit in the todo list that
> - matches the same `...`, automatically modify the todo list of
> - `rebase`, so that the commit marked for squashing comes right after
> - the commit to be modified, and change the action of the moved commit
> - from `pick` to `squash` or `fixup` or `fixup -C` respectively. A commit
> - matches the `...` if the commit subject matches, or if the `...` refers
> - to the commit's hash. As a fall-back, partial matches of the commit
> - subject work, too. The recommended way to create fixup/amend/squash
> - commits is by using the `--fixup`, `--fixup=amend:` or `--fixup=reword:`
> - and `--squash` options respectively of linkgit:git-commit[1].
> + Automatically squash commits with specially formatted messages into
> + previous commits being rebased. If a commit message starts with
> + "squash! ", "fixup! " or "amend! ", the remainder of the subject line
> + is taken as a commit specifier, which matches a previous commit if it
> + matches the subject line or the hash of that commit. If no commit
> + matches fully, matches of the specifier with the start of commit
> + subjects are considered.
> +
> -If the `--autosquash` option is enabled by default using the
> -configuration variable `rebase.autoSquash`, this option can be
> -used to override and disable this setting.
> +In the rebase todo list, the actions of squash, fixup and amend commits are
> +changed from `pick` to `squash`, `fixup` or `fixup -C`, respectively, and they
> +are moved right after the commit they modify. The `--interactive` option can
> +be used to review and edit the todo list before proceeding.
> ++
> +The recommended way to create commits with squash markers is by using the
> +`--squash`, `--fixup`, `--fixup=amend:` or `--fixup=reword:` options of
> +linkgit:git-commit[1], which take the target commit as an argument and
> +automatically fill in the subject line of the new commit from that.
> ++
> +Settting configuration variable `rebase.autoSquash` to true enables
> +auto-squashing by default for interactive rebase. The `--no-autosquash`
> +option can be used to override that setting.
> +
> See also INCOMPATIBLE OPTIONS below.
>
^ permalink raw reply
* Re: [PATCH v3 1/2] rebase: support non-interactive autosquash
From: Andy Koppe @ 2023-11-11 14:08 UTC (permalink / raw)
To: phillip.wood, git; +Cc: gitster, newren
In-Reply-To: <8c2bb219-127c-4128-99ed-158bc64b1dab@gmail.com>
On 06/11/2023 11:06, Phillip Wood wrote:
> On 05/11/2023 00:08, Andy Koppe wrote:
>> So far, the rebase --autosquash option and rebase.autoSquash=true
>> config setting are quietly ignored when used without --interactive,
>
> Thanks for working on this, I agree that "--autosquash" being ignored
> without "--interactive" is something that we should address. I think
> there are several possible solutions
>
> 1 - make "--autosquash" imply "--interactive". This has the advantage
> that the user gets to check the commits are going to be reordered as
> they expect when they edit the todo list. It is hard to see how to
> accommodate the config setting though - I don't think we want
> "rebase.autosquash=true" to imply "--interactive".
>
> 2 - make "--autosquash" without "--interactive" an error. This would
> prevent the user being surprised that their commits are not squashed
> by a non-interactive rebase. Users who have set
> "rebase.autosquash=true" would have to pass "--no-autosquash" to
> perform any form of non-interactive rebase. This is similar to the
> current behavior where the user has to pass "--no-autosquash" if
> they want to use the apply backend with "rebase.autosquash=true".
>
> 3 - make "--autosquash" rearrange and squash commits without
> "--interactive". This is convenient but there is a risk in that the
> user does not get a chance to check the todo list before the commits
> are reordered and squashed. I think that risk is fairly small with
> an explicit "--autosquash" on the commandline. This is the approach
> taken by this patch. I do have some concerns about extending the
> config setting to non-interactive rebases though. If the user has
> commits that look like
>
> fixup! foo (HEAD)
> foo bar
> foo
>
> and runs "git -c rebase.autosquash=non-interactive rebase HEAD~2"
> then we'll silently squash the fixup into the wrong commit due to a
> prefix subject match.
Good analysis. My order of preference is 3 (obviously), 1, 2.
>> except that they prevent fast-forward and that they trigger conflicts
>> with --apply and relatives, which is less than helpful particularly for
>> the config setting.
>
> The behavior to make the config setting incompatible with the apply
> backend was implemented to avoid users being surprised that their
> commits are not squashed by that backend even when they have set
> "rebase.autosquash=true"[1]. I think one could consider "--autosquash"
> being silently ignored without "--interactive" to be an oversight in
> 796abac7e1 (rebase: add coverage of other incompatible options,
> 2023-01-25) that introduced that change.
>
> [1]
> https://lore.kernel.org/git/pull.1466.v5.git.1674619434.gitgitgadget@gmail.com/
>
>> Since the "merge" backend used for interactive rebase also is the
>> default for non-interactive rebase, there doesn't appear to be a
>> reason not to do --autosquash without --interactive, so support that.
>
> I think making "--autosquash" on the commandline work for
> non-interactive rebases is reasonable but I would be open to the
> argument that it would be better to make it an error and require
> "--interactive" to allow the user to check that the commits are going to
> be reordered as they expect.
I found that once I got used to and started trusting the feature,
particularly in connection with the corresponding git-commit support, I
no longer felt the need to check the todo list as I'd inspect the log
afterwards anyway. And of course there's always resetting to ORIG_HEAD
when things do go wrong.
So I think users should be trusted with this, especially as it's not a
particularly dangerous feature, given it requires the squash markers to
be present in the first place to do anything.
>> Turn rebase.autoSquash into a comma-separated list of flags, with
>> "interactive" or "i" enabling auto-squashing with --interactive, and
>> "no-interactive" or "no-i" enabling it without. Make boolean true mean
>> "interactive" for backward compatibility.
>
> Please, please, please don't introduce abbreviated config settings, it
> just makes the interface more complicated. The user only has to set this
> once so I think the short names just add confusion.
Duly noted.
> I also think
> "non-interactive" would be a better name for the config setting
> corresponding to non-interactive rebases. Does this mean the user can
> request that commits are only rearranged when they do not pass
> "--interactive"?
Yes. That doesn't seem useful.
> As I said above I do have some concerns that the
> "rebase.autosquash=non-interactive" setting will catch people out.
I think you're right, so I've gone back to interpreting it as a boolean,
but officially make it affect interactive mode only.
> Having said that ignoring "rebase.autosquash=true" without
> "--interactive" as we do now is inconsistent with the behavior of
> "rebase.autosquash=true" with "--apply". One possibility would be to
> introduce "rebase.autosquash=interactive" which would not cause an error
> with "--apply" and always require an explicit "--autosquash" on the
> commandline to squash fixups without "--interactive"
I don't think different error behaviour is worth a separate setting, as
we can't change rebase.autosquash=true to do auto-squashing without
--interactive without surprising people.
>> Don't prevent fast-forwards or report conflicts with --apply options
>> when auto-squashing is not active.
>
> I think this change deserves to be in a separate commit (which probably
> means separating out the config changes into that commit) as it is not
> directly related to fixing "--autosquash" without "--interactive" on the
> commandline.
Done in v4.
> In summary I like "--autosquash" working without "--interactive" but I'm
> unsure about the config changes.
Thanks very much for the thoughtful review!
Regards,
Andy
^ permalink raw reply
* Re: [PATCH v2 1/2] rebase: support non-interactive autosquash
From: Andy Koppe @ 2023-11-11 14:26 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, newren
In-Reply-To: <xmqqcywng0wu.fsf@gitster.g>
On 06/11/2023 00:50, Junio C Hamano wrote:
> Andy Koppe <andy.koppe@gmail.com> writes:
> Our log
> message convention is to first describe what happens in the system
> in the present tense to illustrate why it is suboptimal, to prepare
> readers' minds to anticipate the solution, which is described next.
>
> When asking reviews on a new iteration [PATCH v(N+1)], please
> summarize the differences relative to [PATCH vN]. For explaining
> such incremental changes for individual patches, here between the
> three-dash line and the diffstat is the place to do so. When you
> have a cover letter [PATCH 0/X], it can be done in that messaage.
> Either way is OK. Doing both is also helpful as long as the
> explanation done in two places do not contradict with each other.
> If you tried to format the documentation before sending this patch,
> you'd have seen the second paragraph formatted as if it were a code
> snippet. Dedent the second paragraph (and later ones if you had
> more than one extra paragraphs), and turn the blank line between the
> paragraphs into a line with "+" (and nothing else) on it. See the
> description of `--autosquash` option in Documentation/git-rebase.txt
> for an example.
Sad thing is that I knew most of that from reading the contribution
guidelines and previous experience, but obviously I don't always
remember. So thanks for you patience in re-explaining that.
> OK, by clearing opts->config_autosquash in this function, you keep
> the rebase.autosquash to be "the last one wins" as a whole. If a
> configuration file with lower precedence (e.g., /etc/gitconfig) says
> "[rebase] autosquash" to set it to "interactive,no-interactive", a
> separate setting in your ~/.gitconfig "[rebase] autosquash = false"
> would override both bits.
>
> A more involved design may let the users override these bits
> independently by allowing something like "!no-i" (take whatever the
> lower precedence configuration file says for the interactive case,
> but disable autosquash when running a non-interactive rebase) as the
> value, but I think the approach taken by this patch to allow replacing
> as a whole is OK. It is simpler to explain.
>
> Giving short-hands for often used command line options is one thing,
> but I do not think a short-hand is warranted here, especially when
> the other one needs to be a less-than-half legible "no-i" that does
> not allow "no-int" and friends, for configuration variable values.
> I'd strongly suggest dropping them.
Dropped in v4, along with the attempt to expand rebase.autoSquash,
following Phillip's review.
Regards,
Andy
^ permalink raw reply
* Re: Git Rename Detection Bug
From: Elijah Newren @ 2023-11-11 15:13 UTC (permalink / raw)
To: Philip Oakley; +Cc: Jeremy Pridmore, git@vger.kernel.org, Paul Baumgartner
In-Reply-To: <9baca4af-a570-4b7a-a1ee-de91b809e79c@iee.email>
Hi,
On Sat, Nov 11, 2023 at 3:08 AM Philip Oakley <philipoakley@iee.email> wrote:
>
> Hi all,
>
> On 11/11/2023 05:46, Elijah Newren wrote:
> > The fact that you were trying to "undo" renames and "redo the correct
> > ones" suggested there's something you still didn't understand about
> > rename detection, though.
>
>
> Could I suggest that we are missing a piece of terminology, to wit,
> BLOBSAME. It's a compatriot to TREESAME, as used in `git log` for
> history simplification (based on a tree's pathspec, most commonly a
> commit's top level path).
We could add it, but I'm not sure how it helps. We already had 'exact
rename' which seems to fit the bill as well, and 'blob' is something
someone new to Git is unlikely to know.
Perhaps it's useful in some other context, though?
> File rename, at it's most basic, is when the blob associated with that
> changed path is identical, i.e. BLOBSAME. There is no need to 'record'
> the action of renaming, moving or whatever, the content sameness is
> right there, in plain sight, as an identical blob name. After that
> (files with slight variations) it is a load of heuristics, but starting
> with BLOBSAME we see how easy the basic rename detection is, and why
> renames (and de-dup) don't need recording.
This is incorrect. Let's say you have a file foo:
* base version: foo has hash A
* our version: foo has been renamed to bar, but bar still has hash A
* their version: foo has been modified; it now has hash B
The foo->bar is an exact rename (or they are BLOBSAME if you prefer),
but the renaming/moving/whatever is a critical piece of information
because the changes to foo in 'their' version need to be applied to
bar to get the correct end results.
I do not know if in Jeremy's case foo has been modified on the
unrenamed side. But the following hypothetical is exactly the type of
problem Jeremy is hitting: what should happen when 'our' version has
both a new 'bar' and a new 'baz' file that each have hash A? In that
case, to which one was foo renamed? It's inherently ambiguous.
> The heuristics of 'rename with small change' is trickier, but for a
> basic understanding, starting at BLOBSAME (and TREESAME for directory
> renames) should make it easier to grasp the concepts.
Interesting; TREESAME isn't used within directory rename detection
currently; it is only used currently when two (or three) trees with
the same name are TREESAME, in order to potentially avoid recursing
into the tree. But even then, having two trees with the same name be
TREESAME isn't enough on its own to avoid recursing into that tree,
because the other side could have added files within the same-named
tree and we need to know about those added files because they could be
part of renames involving other files outside that tree. There would
probably be similar challenges to attempting to apply the concept of
TREESAME to directory rename detection to two trees of different
names, but it's at least an interesting idea. Hmm....
^ permalink raw reply
* rev-list default order sometimes very slow (size and metadata dependent)
From: Kevin Lyles @ 2023-11-11 16:17 UTC (permalink / raw)
To: git@vger.kernel.org; +Cc: allred.sean@gmail.com
[-- Attachment #1.1: Type: text/plain, Size: 5028 bytes --]
Hello,
As part of my company's migration from SVN to git, we discovered a
performance issue in rev-list with large repositories. The issue appears to
be metadata-dependent; we were able to work around it (completely avoiding
any performance penalty) by changing the date of certain commits.
The general structure of our repository is I think fairly normal (if large
-- we have >5.5 million commits total). We have a handful of trunk
branches, and ~10k total refs. To reduce the ref count (we hit other
performance issues when we had significantly more refs), we remove refs as
we're done with them. Any code that doesn't make it into a trunk is
preserved in an archive branch. The archive branch has no content, and
consists entirely of octopus merges with 50-500 parents.
If the archive branch is created with author/commit dates older than the
rest of the repository, we're able to run:
$ git rev-list --count --all
in ~9-10 seconds on a mirror clone with a commit-graph. However, if the
archive branch is instead created with author/commit dates newer than the
rest of the repository, it takes 4-5 minutes.
Using any order other than the default or --reverse removes the disparity.
All orders except --author-date-order bring things much closer to the ~9-10
seconds we see with the workaround, and --author-date-order is still under a
minute (though not by much).
System info from git bugreport:
[System Info]
git version:
git version 2.42.0.windows.2
cpu: x86_64
built from commit: 2f819d1670fff9a1818f63b6722e9959405378e3
sizeof-long: 4
sizeof-size_t: 8
shell-path: /bin/sh
feature: fsmonitor--daemon
uname: Windows 10.0 19044
compiler info: gnuc: 13.2
libc info: no libc information available
$SHELL (typically, interactive shell): C:\Program Files\Git\usr\bin\bash.exe
(no enabled hooks)
Note that we first realized this was an issue on our GitLab instance, which
runs on Linux, so this is not a Windows-specific bug.
I created a bash script to create very similar repositories that are/are not
affected by the issue; it follows. The issue starts to become visible at 1
million commits (the default), where the difference is ~2x. 5 million
commits is roughly equivalent performance-wise to what we saw in our
repository, with a difference of ~33x. Note that with 5 million commits,
each repository is ~1.2 GB and takes 7-8 minutes to create on an i9-9900
with NVMe storage.
Once you create a fast and a slow repo with the script, try the following
commands in each one:
# Shows the performance difference
$ time git rev-list --count –all
# Shows very similar performance across both repos
$ time git rev-list --count --all --topo-order
Thank you,
Kevin Lyles
klyles@epic.com <mailto:klyles@epic.com>
----------------------------------------
#!/bin/bash
usage="Usage: $0 <destination folder> <--fast|--slow> [Number of commits (default: 1000000)]"
destinationFolder=${1:?$usage}
oldTimestamp=315554400 # 1980-01-01 midnight
newTimestamp=1672552800 # 2023-01-01 midnight
if [ "$2" == "--fast" ]
then
archiveTimestamp=$oldTimestamp
elif [ "$2" == "--slow" ]
then
archiveTimestamp=$newTimestamp
else
echo "$usage" >&2
exit 1
fi
numberOfCommits=${3:-1000000}
if ! [[ "$numberOfCommits" =~ ^[0-9]+$ ]]
then
echo "$usage" >&2
exit 1
fi
increment=$(( (newTimestamp - oldTimestamp) / (numberOfCommits + 2) ))
timestamp=$oldTimestamp
rm -rf "$destinationFolder"
git init "$destinationFolder"
echo "Fast-importing repo, please wait..."
{
echo "feature done"
echo "reset refs/heads/main"
echo ""
for count in $(seq "$numberOfCommits")
do
timestamp=$(( timestamp + increment ))
echo "commit refs/heads/main"
echo "mark :$count"
echo "committer Test Test <test@test.com <mailto:test@test.com> > $timestamp -0500"
echo "data <<|"
echo "Main branch commit #$count"
echo "|"
echo ""
done
parentMark=0
echo "reset refs/archive"
for count in $(seq $(( numberOfCommits / 1000 )))
do
echo "commit refs/archive"
echo "committer Test Test <test@test.com <mailto:test@test.com> > $archiveTimestamp -0500"
echo "data <<|"
echo "Archive branch commit #$count"
echo "|"
for parentCount in {1..50}
do
parentMark=$(( (parentMark + 99991) % numberOfCommits + 1 ))
echo "merge :$parentMark"
done
echo ""
done
echo "done"
} | git -C "$destinationFolder" fast-import
git -C "$destinationFolder" commit-graph write
[-- Attachment #1.2: Type: text/html, Size: 12590 bytes --]
[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 6034 bytes --]
^ permalink raw reply
* Re: Git Rename Detection Bug
From: Philip Oakley @ 2023-11-11 11:08 UTC (permalink / raw)
To: Elijah Newren, Jeremy Pridmore; +Cc: git@vger.kernel.org, Paul Baumgartner
In-Reply-To: <CABPp-BHEX+SyophEfgRqDbNdrAS3=bptt_cKzHLBSutnBAxexw@mail.gmail.com>
Hi all,
On 11/11/2023 05:46, Elijah Newren wrote:
> The fact that you were trying to "undo" renames and "redo the correct
> ones" suggested there's something you still didn't understand about
> rename detection, though.
Could I suggest that we are missing a piece of terminology, to wit,
BLOBSAME. It's a compatriot to TREESAME, as used in `git log` for
history simplification (based on a tree's pathspec, most commonly a
commit's top level path).
File rename, at it's most basic, is when the blob associated with that
changed path is identical, i.e. BLOBSAME. There is no need to 'record'
the action of renaming, moving or whatever, the content sameness is
right there, in plain sight, as an identical blob name. After that
(files with slight variations) it is a load of heuristics, but starting
with BLOBSAME we see how easy the basic rename detection is, and why
renames (and de-dup) don't need recording.
The heuristics of 'rename with small change' is trickier, but for a
basic understanding, starting at BLOBSAME (and TREESAME for directory
renames) should make it easier to grasp the concepts.
--
Philip
^ permalink raw reply
* [PATCH] fuzz: add new oss-fuzz fuzzer for date.c / date.h
From: Arthur Chan via GitGitGadget @ 2023-11-11 17:39 UTC (permalink / raw)
To: git; +Cc: Arthur Chan, Arthur Chan
From: Arthur Chan <arthur.chan@adalogics.com>
Signed-off-by: Arthur Chan <arthur.chan@adalogics.com>
---
fuzz: add new oss-fuzz fuzzer for date.c / date.h
This patch is aimed to add a new oss-fuzz fuzzer to the oss-fuzz
directory for fuzzing date.c / date.h in the base directory.
The .gitignore of the oss-fuzz directory and the Makefile have been
modified to accommodate the new fuzzer fuzz-date.c.
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1612%2Farthurscchan%2Fnew-fuzzer-date-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1612/arthurscchan/new-fuzzer-date-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1612
Makefile | 1 +
oss-fuzz/.gitignore | 1 +
oss-fuzz/fuzz-date.c | 75 ++++++++++++++++++++++++++++++++++++++++++++
3 files changed, 77 insertions(+)
create mode 100644 oss-fuzz/fuzz-date.c
diff --git a/Makefile b/Makefile
index 03adcb5a480..c9fe99a8c88 100644
--- a/Makefile
+++ b/Makefile
@@ -752,6 +752,7 @@ ETAGS_TARGET = TAGS
FUZZ_OBJS += oss-fuzz/fuzz-commit-graph.o
FUZZ_OBJS += oss-fuzz/fuzz-pack-headers.o
FUZZ_OBJS += oss-fuzz/fuzz-pack-idx.o
+FUZZ_OBJS += oss-fuzz/fuzz-date.o
.PHONY: fuzz-objs
fuzz-objs: $(FUZZ_OBJS)
diff --git a/oss-fuzz/.gitignore b/oss-fuzz/.gitignore
index 9acb74412ef..2375e77b108 100644
--- a/oss-fuzz/.gitignore
+++ b/oss-fuzz/.gitignore
@@ -1,3 +1,4 @@
fuzz-commit-graph
fuzz-pack-headers
fuzz-pack-idx
+fuzz-date
diff --git a/oss-fuzz/fuzz-date.c b/oss-fuzz/fuzz-date.c
new file mode 100644
index 00000000000..29bcaf595e4
--- /dev/null
+++ b/oss-fuzz/fuzz-date.c
@@ -0,0 +1,75 @@
+#include "git-compat-util.h"
+#include "date.h"
+
+int LLVMFuzzerTestOneInput(const uint8_t *data, size_t size);
+
+int LLVMFuzzerTestOneInput(const uint8_t *data, size_t size)
+{
+ int type;
+ int time;
+ int num;
+ char *str;
+ timestamp_t ts;
+ enum date_mode_type dmtype;
+ struct date_mode *dm;
+
+ if (size <= 8)
+ {
+ return 0;
+ }
+
+ type = (*((int *)data)) % 8;
+ data += 4;
+ size -= 4;
+
+ time = abs(*((int *)data));
+ data += 4;
+ size -= 4;
+
+ str = (char *)malloc(size+1);
+ if (!str)
+ {
+ return 0;
+ }
+ memcpy(str, data, size);
+ str[size] = '\0';
+
+ ts = approxidate_careful(str, &num);
+ free(str);
+
+ switch(type)
+ {
+ case 0: default:
+ dmtype = DATE_NORMAL;
+ break;
+ case 1:
+ dmtype = DATE_HUMAN;
+ break;
+ case 2:
+ dmtype = DATE_SHORT;
+ break;
+ case 3:
+ dmtype = DATE_ISO8601;
+ break;
+ case 4:
+ dmtype = DATE_ISO8601_STRICT;
+ break;
+ case 5:
+ dmtype = DATE_RFC2822;
+ break;
+ case 6:
+ dmtype = DATE_RAW;
+ break;
+ case 7:
+ dmtype = DATE_UNIX;
+ break;
+ }
+
+ dm = date_mode_from_type(dmtype);
+ dm->local = 1;
+ show_date(ts, time, dm);
+
+ date_mode_release(dm);
+
+ return 0;
+}
base-commit: dadef801b365989099a9929e995589e455c51fed
--
gitgitgadget
^ permalink raw reply related
* [PATCH] checkout: add config variable checkout.autoDetach
From: Andy Koppe @ 2023-11-11 22:42 UTC (permalink / raw)
To: git; +Cc: pclouds, Andy Koppe
The git-checkout command without pathspecs automatically detaches HEAD
when switching to something other than a branch, whereas git-switch
requires the --detach option to do so.
Add configuration variable checkout.autoDetach to choose the behavior
for both: true for automatic detaching, false for requiring --detach.
Amend their documentation and tests accordingly.
Signed-off-by: Andy Koppe <andy.koppe@gmail.com>
---
I like to use git-switch and git-restore instead of git-checkout, and
also recommend them when new users ask how to switch branches or undo
local changes. But I do miss git-checkout's auto-detaching, as I like
to avoid creating local versions of remote branches when I don't intend
to make changes to them.
Hence this patch, which turned out to be very simple in terms of the
actual source change, as the checkout_opts.implicit_detach field
already controls the necessary functionality. That was added in commit
7968bef06b by Nguyễn Thái Ngọc Duy, with the following explanation:
> "git checkout <commit>" will checkout the commit in question and
> detach HEAD from the current branch. It is naturally a right thing to
> do once you get git references. But detached HEAD is a scary concept
> to new users because we show a lot of warnings and stuff, and it could
> be hard to get out of (until you know better).
>
> To keep switch a bit more friendly to new users, we only allow
> entering detached HEAD mode when --detach is given.
I think that makes plenty of sense, and I don't think it conflicts with
the config setting proposed here, as new users are unlikely to mess with
such settings. In fact, the point where "you get git references" would
probably be a good time to enable checkout.autoDetach.
Conversely, admins might want to set checkout.autoDetach to false in the
system-wide config to prevent accidental decapitation by git-checkout
and the resulting support requests.
Documentation/config/checkout.txt | 8 ++++++++
Documentation/git-checkout.txt | 3 +++
Documentation/git-switch.txt | 3 +++
builtin/checkout.c | 4 ++++
t/t2020-checkout-detach.sh | 14 ++++++++++++++
t/t2060-switch.sh | 7 +++++++
6 files changed, 39 insertions(+)
diff --git a/Documentation/config/checkout.txt b/Documentation/config/checkout.txt
index a323022993..6827ee74d5 100644
--- a/Documentation/config/checkout.txt
+++ b/Documentation/config/checkout.txt
@@ -17,6 +17,14 @@ and by linkgit:git-worktree[1] when `git worktree add` refers to a
remote branch. This setting might be used for other checkout-like
commands or functionality in the future.
+checkout.autoDetach::
+ If set to true, `git checkout` and `git switch` automatically detach
+ HEAD when switching to something other than a branch. If set to false,
+ they require the `--detach` option to detach HEAD.
++
+If this setting is not specified, `git checkout` defaults to automatic
+detaching, whereas `git switch` defaults to requiring `--detach`.
+
checkout.guess::
Provides the default value for the `--guess` or `--no-guess`
option in `git checkout` and `git switch`. See
diff --git a/Documentation/git-checkout.txt b/Documentation/git-checkout.txt
index 240c54639e..23f90c15ac 100644
--- a/Documentation/git-checkout.txt
+++ b/Documentation/git-checkout.txt
@@ -79,6 +79,9 @@ When the `<commit>` argument is a branch name, the `--detach` option can
be used to detach `HEAD` at the tip of the branch (`git checkout
<branch>` would check out that branch without detaching `HEAD`).
+
+If the `checkout.autoDetach` config variable is set to false, the `--detach`
+option is required even if the `<commit>` argument is not a branch name.
++
Omitting `<branch>` detaches `HEAD` at the tip of the current branch.
'git checkout' [-f|--ours|--theirs|-m|--conflict=<style>] [<tree-ish>] [--] <pathspec>...::
diff --git a/Documentation/git-switch.txt b/Documentation/git-switch.txt
index c60fc9c138..f6b925c43b 100644
--- a/Documentation/git-switch.txt
+++ b/Documentation/git-switch.txt
@@ -83,6 +83,9 @@ $ git switch <new-branch>
Switch to a commit for inspection and discardable
experiments. See the "DETACHED HEAD" section in
linkgit:git-checkout[1] for details.
++
+If the `checkout.autoDetach` configuration variable is set to true,
+`--detach` can be omitted when the target is not a branch.
--guess::
--no-guess::
diff --git a/builtin/checkout.c b/builtin/checkout.c
index f02434bc15..d042638bb0 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -1205,6 +1205,10 @@ static int git_checkout_config(const char *var, const char *value,
handle_ignore_submodules_arg(&opts->diff_options, value);
return 0;
}
+ if (!strcmp(var, "checkout.autodetach")) {
+ opts->implicit_detach = git_config_bool(var, value);
+ return 0;
+ }
if (!strcmp(var, "checkout.guess")) {
opts->dwim_new_local_branch = git_config_bool(var, value);
return 0;
diff --git a/t/t2020-checkout-detach.sh b/t/t2020-checkout-detach.sh
index 8202ef8c74..b842b6cc89 100755
--- a/t/t2020-checkout-detach.sh
+++ b/t/t2020-checkout-detach.sh
@@ -69,12 +69,26 @@ test_expect_success 'checkout ref^0 detaches' '
check_detached
'
+test_expect_success 'checkout of tag with autoDetach=false fails' '
+ reset &&
+ test_config checkout.autoDetach false &&
+ test_must_fail git checkout tag &&
+ check_not_detached
+'
+
test_expect_success 'checkout --detach detaches' '
reset &&
git checkout --detach branch &&
check_detached
'
+test_expect_success 'checkout --detach with autoDetach=false detaches' '
+ reset &&
+ test_config checkout.autoDetach false &&
+ git checkout --detach branch &&
+ check_detached
+'
+
test_expect_success 'checkout --detach without branch name' '
reset &&
git checkout --detach &&
diff --git a/t/t2060-switch.sh b/t/t2060-switch.sh
index e247a4735b..69ff197d11 100755
--- a/t/t2060-switch.sh
+++ b/t/t2060-switch.sh
@@ -50,6 +50,13 @@ test_expect_success 'switch and detach current branch' '
test_must_fail git symbolic-ref HEAD
'
+test_expect_success 'switch with checkout.autoDetach=true' '
+ test_when_finished git switch main &&
+ test_config checkout.autoDetach true &&
+ git switch main^{commit} &&
+ test_must_fail git symbolic-ref HEAD
+'
+
test_expect_success 'switch and create branch' '
test_when_finished git switch main &&
git switch -c temp main^ &&
--
2.43.0-rc1
^ permalink raw reply related
* Re: [PATCH] RelNotes: minor wording fixes in 2.43.0 release notes
From: Junio C Hamano @ 2023-11-12 0:59 UTC (permalink / raw)
To: Elijah Newren via GitGitGadget; +Cc: git, Elijah Newren
In-Reply-To: <pull.1611.git.1699675340400.gitgitgadget@gmail.com>
"Elijah Newren via GitGitGadget" <gitgitgadget@gmail.com> writes:
> From: Elijah Newren <newren@gmail.com>
>
> Signed-off-by: Elijah Newren <newren@gmail.com>
> ---
> RelNotes: minor wording fixes in 2.43.0 release notes
Thanks, will directly apply on 'master'.
^ permalink raw reply
* Re: [PATCH] fuzz: add new oss-fuzz fuzzer for date.c / date.h
From: Junio C Hamano @ 2023-11-12 5:59 UTC (permalink / raw)
To: Arthur Chan via GitGitGadget; +Cc: git, Arthur Chan
In-Reply-To: <pull.1612.git.1699724379458.gitgitgadget@gmail.com>
"Arthur Chan via GitGitGadget" <gitgitgadget@gmail.com> writes:
> diff --git a/Makefile b/Makefile
> index 03adcb5a480..c9fe99a8c88 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -752,6 +752,7 @@ ETAGS_TARGET = TAGS
> FUZZ_OBJS += oss-fuzz/fuzz-commit-graph.o
> FUZZ_OBJS += oss-fuzz/fuzz-pack-headers.o
> FUZZ_OBJS += oss-fuzz/fuzz-pack-idx.o
> +FUZZ_OBJS += oss-fuzz/fuzz-date.o
The same comment applies to .gitignore but I think the existing
entries are sorted and fuzz-date should be added between
fuzz-commit-graph and fuzz-pack-headers.
> diff --git a/oss-fuzz/fuzz-date.c b/oss-fuzz/fuzz-date.c
> new file mode 100644
> index 00000000000..29bcaf595e4
> --- /dev/null
> +++ b/oss-fuzz/fuzz-date.c
> @@ -0,0 +1,75 @@
> +#include "git-compat-util.h"
> +#include "date.h"
> +
> +int LLVMFuzzerTestOneInput(const uint8_t *data, size_t size);
> +
> +int LLVMFuzzerTestOneInput(const uint8_t *data, size_t size)
> +{
> + int type;
> + int time;
> + int num;
> + char *str;
> + timestamp_t ts;
> + enum date_mode_type dmtype;
> + struct date_mode *dm;
> +
> + if (size <= 8)
> + {
> + return 0;
> + }
How much do we care about sticking to our coding style for source
files in this directory? If we do (and I do not see a strong reason
not to), let's lose the {unneeded braces} around a single statement
block.
> + type = (*((int *)data)) % 8;
> + data += 4;
> + size -= 4;
I'd prefer to avoid these hardcoded and unexplained constants. I
think "8" relates to the number of case arms below? I am not sure
if "4" is justifiable without "we assume everybody's int is four
bytes long", but if that is what is going on, perhaps use uint32_t
or something?
Also, is data[] guaranteed to be aligned well to allow us to do the
above? As we only need to spread to DATE_UNIX-1 types (because we
exclude DATE_STRFTIME), it is sufficient to look at the lower nibble
of a single byte. The upper nibble could be used to fuzz the .local
bit if you wanted to, e.g. so I wonder
local_bit = !!(*data & 0x10);
dmtype = (enum date_mode_type)(*data % DATE_UNIX);
if (dmtype == DATE_STRFTIME)
return 0;
data++;
size--;
> + time = abs(*((int *)data));
> + data += 4;
> + size -= 4;
Ditto. Rename "time" because the second parameter to show_date() is
*not* "time" but "tz". The valid range of "tz" comfortably fits in
16-bit signed int, but note that there are valid negative values in
the range.
Are we assuming that the "tz" is attacker controlled? Why are you
limiting its value to non-negative, yet you are not rejecting absurd
timezone offsets? Good values lie in a range much narrower than
between -2400 and 2400. Subjecting "tz" to fuzzer is perfectly
fine, but then limiting its value to non-negative contradicts with
it, so I am not sure what your intention is.
As I used the first byte to fuzz dmtype and .local, let's use the
next three bytes to allow feeding overly wild timezone values to the
machinery and see what breaks, perhaps like so:
tz = *data++; /* int tz; */
tz = (tz << 8) | *data++;
tz = (tz << 8) | *data++;
size -= 3;
Now the upfront length check needs to reject any input shorter than
4 bytes, so do so with a comment accordingly, perhaps like
if (size < 4)
/*
* we use the first byte to fuzz dmtype and local,
* then the next three bytes to fuzz tz offset,
* and the remainder is fed as end-user input to
* approxidate().
*/
return 0;
before everything I wrote so far.
> + str = (char *)malloc(size+1);
(char *)malloc(size + 1);
> + if (!str)
> + {
> + return 0;
> + }
Ditto on {unnecessary braces}.
> + memcpy(str, data, size);
> + str[size] = '\0';
> +
> + ts = approxidate_careful(str, &num);
> + free(str);
> +
> + switch(type)
> + {
> + case 0: default:
> + dmtype = DATE_NORMAL;
> + break;
Style. In our codebase, "switch" and "case" align at the same
column, and case arms are written one per line, i.e.,
switch (type) {
case 0:
default:
...
The way dmtype is handled in a switch() below tells me that you do
not consider it is a potential attack vector (e.g., an attacker
cannot force us to use dmtype==DATE_STRFTIME without the format and
cause us to die). Am I reading your intention correctly?
If so, I'd just do the "use the lower nibble of the first byte" as I
shown earlier, and this large switch statement will go away.
> + case 1:
> + dmtype = DATE_HUMAN;
> + break;
> + case 2:
> + dmtype = DATE_SHORT;
> + break;
> + case 3:
> + dmtype = DATE_ISO8601;
> + break;
> + case 4:
> + dmtype = DATE_ISO8601_STRICT;
> + break;
> + case 5:
> + dmtype = DATE_RFC2822;
> + break;
> + case 6:
> + dmtype = DATE_RAW;
> + break;
> + case 7:
> + dmtype = DATE_UNIX;
> + break;
> + }
> +
> + dm = date_mode_from_type(dmtype);
> + dm->local = 1;
Don't we want to allow the incoming data to fuzz the local bit, too?
> + show_date(ts, time, dm);
> +
> + date_mode_release(dm);
> +
> + return 0;
> +}
>
> base-commit: dadef801b365989099a9929e995589e455c51fed
^ permalink raw reply
* Re: [PATCH] checkout: add config variable checkout.autoDetach
From: Junio C Hamano @ 2023-11-12 6:04 UTC (permalink / raw)
To: Andy Koppe; +Cc: git, pclouds
In-Reply-To: <20231111224253.1923-1-andy.koppe@gmail.com>
Andy Koppe <andy.koppe@gmail.com> writes:
> The git-checkout command without pathspecs automatically detaches HEAD
> when switching to something other than a branch, whereas git-switch
> requires the --detach option to do so.
>
> Add configuration variable checkout.autoDetach to choose the behavior
> for both: true for automatic detaching, false for requiring --detach.
>
> Amend their documentation and tests accordingly.
>
> Signed-off-by: Andy Koppe <andy.koppe@gmail.com>
> ---
"switch" was meant to be an experimental command to sort out this
kind of UI ideas, and I think the fact that it requires a more
explicit "--detach", where experienced users might just say "git
checkout that-branch^0", has established itself as a more friendly
and good thing to help new users. I do not know how others react to
this kind of proliferation of configuration variables, but I do not
mind this particular variable existing.
^ permalink raw reply
* Re: first-class conflicts?
From: Martin von Zweigbergk @ 2023-11-12 7:05 UTC (permalink / raw)
To: Elijah Newren; +Cc: phillip.wood, Sandra Snan, git, Randall S. Becker
In-Reply-To: <CABPp-BF35JvbXcjLxJkQtKeVhQ2qYaBXBoN4P07BEWS8mxTaMA@mail.gmail.com>
On Fri, Nov 10, 2023 at 1:41 PM Elijah Newren <newren@gmail.com> wrote:
>
> Hi Martin,
>
> On Wed, Nov 8, 2023 at 10:23 AM Martin von Zweigbergk
> <martinvonz@google.com> wrote:
> > On Tue, Nov 7, 2023 at 11:31 PM Elijah Newren <newren@gmail.com> wrote:
> > > On Tue, Nov 7, 2023 at 9:38 AM Martin von Zweigbergk
> > > <martinvonz@google.com> wrote:
> > > >
> [...]
> > > I am curious more about the data you do store. My fuzzy memory is
> > > that you store a commit header involving something of the form "A + B
> > > - C", where those are all commit IDs. Is that correct?
> >
> > We actually store it outside the Git repo (together with the "change
> > id"). We have avoided using commit headers because I wasn't sure how
> > well different tools deal with unexpected commit headers, and because
> > I wanted commits to be indistinguishable from commits created by a
> > regular Git binary. The latter argument doesn't apply to commits with
> > conflicts since those are clearly not from a regular Git binary
> > anyway, and we don't allow pushing them to a remote.
> >
> > > Is this in
> > > addition to a normal "tree" header as in Git, or are one of A or B
> > > found in the tree header?
> >
> > It's in addition. For the tree, we actually write a tree object with
> > three subtrees:
> >
> > .jjconflict-base-0: C
> > .jjconflict-side-0: A
> > .jjconflict-side-1: B
> >
> > The tree is not authoritative - we use the Git-external storage for
> > that. The reason we write the trees is mostly to prevent them from
> > getting GC'd.
>
> Oh, that seems like a clever way to handle reachability and make sure
> the relevant trees are automatically included in any pushes or pulls.
>
> > Also, if a user does `git checkout <conflicted commit>`,
> > they'll see those subdirectories and will hopefully be reminded that
> > they did something odd (perhaps we should drop the leading `.` so `ls`
> > will show them...). They can also diff the directories in a diff tool
> > if they like.
>
> Oh, so they don't get a regular top-level looking tree with
> possibly-conflicted-files present? Or is this in addition to the
> regular repository contents?
They get a regular tree with conflict markers if they use `jj
checkout`, but not if they use `git checkout`.
> If in addition, are you worried about
> users ever creating real entries named ".jjconflict-base-<N>" in their
> repository?
I'm not worried about that since it's not the source of truth, so at
most they waste some time.
By the way, if the user did use `git checkout` and got those
`.jjconflict-*` directories in the working copy, and then ran a `jj`
command afterwards, then jj would think that the conflict was resolved
by replacing the conflicted paths (and all other paths!) by those
`.jjconflict-*` directories :) The user would probably realize their
mistake pretty quickly and run `jj abandon` to discard those changes.
>
> > > I think you said there was also the
> > > possibility for more than three terms. Are those for when a
> > > conflicted commit is merged with another branch that adds more
> > > conflicts, or are there other cases too? (Octopus merges?)
> >
> > Yes, they can happen in both of those cases you mention. More
> > generally, whenever you apply a diff between two trees onto another
> > tree, you might end up with a higher-arity conflict. So merging in
> > another branch can do that, or doing an octopus merge (which is the
> > same thing at the tree level, just different at the commit level), or
> > rebasing or reverting a commit.
> >
> > We simplify conflicts algebraically, so rebasing a commit multiple
> > times does not increase the arity - the intermediate parents were both
> > added and removed and thus cancel out. These simple algorithms for
> > simplifying conflicts are encapsulated in
> > https://github.com/martinvonz/jj/blob/main/lib/src/merge.rs. Most of
> > them are independent of the type of values being merged; they can be
> > used for doing algebra on tree ids, content hunks, refs, etc. (in the
> > test cases, we mostly merge integers because integer literals are
> > compact).
>
> It's done on content hunks as well? That's interesting.
Yes, when merging trees, we start at the root tree and try to resolve
conflicts at the tree entry level (i.e. without reading file
contents). I think git does the same. If that's not enough we need to
recurse into subtrees or file contents. When merging files, we find
matching regions of the inputs and use the same algorithm on the
individual chunks between the matching regions.
>
> When exactly would it be done on refs, though? I'm not following that one.
First of all, note that jj allows refs to be in a conflicted state
similar to how trees can be in a conflicted state. We merge refs for a
few different reasons. If you run two concurrent operations on a repo,
we merge any changes to the refs. We do the same thing when you fetch
branches from a remote. For example, if you've fetched branch "main"
from a remote, then moved it locally, and then you fetch again from
the remote, we'll attempt to merge those refs. We use the same
function for merging there, but if it fails, we then also
automatically resolve two operations moving the branch forward
different amounts (e.g. one operation moves a ref from X~10 to X~5
while the other moves it forward to X, we resolve to X).
https://github.com/martinvonz/jj/blob/main/docs/technical/concurrency.md
talks a bit more about that.
>
> And what else is in that "etc."?
I think it's only individual file ids (blob ids) and the executable
bit. If a file's content changed and its executable bit changed, we
use the same algorithm for each of those pieces of information.
>
> > > What about recursive merges, i.e. merges where the two sides do not
> > > have a unique merge base. What is the form of those? (Would "- C" be
> > > replaced by "- C1 - C2 - ... - Cn"? Or would we create the virtual
> > > merge base V and then do a " - V"? Or do we only have "A + B"?)
> >
> > We do that by recursively creating a virtual tree just like Git does,
> > I think (https://github.com/martinvonz/jj/blob/084b99e1e2c42c40f2d52038cdc97687b76fed89/lib/src/rewrite.rs#L56-L71).
> > I think the main difference is that by modeling conflicts, we can
> > avoid recursive conflict markers (if that's what Git does), and we can
> > even automatically resolve some cases where the virtual tree has a
> > conflict.
>
> Okay, but that talks about the mechanics of creating a recursive
> merge, omitting all the details about how the conflict header is
> written when you record the merge. Is the virtual merge base
> represented in the algebraic "A + B - C" expressions, or is the "- C"
> part omitted? If it is represented, and the virtual merge base had
> conflicts which you could not automatically resolve, what exactly does
> the conflicted header for the outer merge get populated with?
I think we're talking about the state in F below, right?
F
/ \
/ \
D E
|\ /|
| X |
|/ \|
B C
\ /
\ /
A
The virtual commit/tree, which we can think of as sitting where the X
is in the graph, would have state V=B+C-A. The state at F would have
D+E-V=D+E-(B+C-A)=D+(E-C)+(A-B). This is encoded in `Merge::flatten()`
here: https://github.com/martinvonz/jj/blob/e3a1e5b80ed9124091baa4d920cc9e8124c1f559/lib/src/merge.rs#L421-L451.
It's not specific to recursive merge; we run into the same kind of
higher-arity conflicts on regular octopus merges or repeated merges
(if you don't resolve conflicts in between).
Oh, I should also say that we don't store the unmodified trees in
these expressions. Instead, for anything we can automatically resolve,
we replace those parts of the trees. So even if A, B, and C differ at
paths X, Y, and Z, the trees we associate with V might only differ at
path Y if that's the only path we couldn't resolve. IIRC, I did it
that way because it seemed wasteful to re-attempt the merge at paths X
and Z every time we rewrite the commit. I *think* it rarely matters in
practice, but it feels like it could in some cases (maybe where two
sides make the same changes).
>
> [...]
>
> > Great questions! We don't have support for renames, so we haven't had
> > to worry about these things. We have talked a little about divergent
> > renames and the need for recording that in the commit so we can tell
> > the user about it and maybe ask them which name they want to keep. I
> > had not considered the interaction with partial conflict resolution,
> > so thanks for bringing that up. I don't have any answers now, but
> > we'll probably need to start thinking about this soon.
>
> I was wondering if that might be the answer. When you do tackle this,
> I'd be interested to hear your thoughts. I'm wondering if we just
> need to augment the data in the conflict header to handle such cases
> (though I guess this could risk having commit objects that are
> significantly bigger than normal in theoretical cases where many such
> paths are involved?)
Yes, that's what I've been thinking, but I think the only thing I had
been thinking of storing was for "divergent renames" (A->B on one
side, A->C on the other). Will let you know when we start thinking
about this for real. Thanks again for your input!
>
> > > I'm curious to hear what happens when you do start dogfooding, on
> > > projects with many developers and which are jj-only. Do commits with
> > > conflicts accidentally end up in mainline branches, or are there good
> > > ways to make sure they don't hit anything considered stable?
> >
> > That won't happen at Google because our source of truth for "merged
> > PRs" (in GitHub-speak) is in our existing VCS. We will necessarily
> > have to translate from jj's data model to its data model before a
> > commit can even be sent for review.
>
> That makes sense, but I was just hoping we'd have an example to look
> to for how to keep things safe if we were to implement this. Sadly, I
> don't think we have the benefit of relying on folks to first push
> their commits into some other VCS which lacks this feature. ;-)
It might be best to disallow pushing conflicts to start with. It
should also be easy to add a hook on the server to disallow it only to
certain branches.
^ permalink raw reply
* Re: [PATCH] checkout: add config variable checkout.autoDetach
From: Andy Koppe @ 2023-11-12 9:15 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, pclouds
In-Reply-To: <xmqqbkbzo6ba.fsf@gitster.g>
On 12/11/2023 06:04, Junio C Hamano wrote:
> Andy Koppe <andy.koppe@gmail.com> writes:
>
>> The git-checkout command without pathspecs automatically detaches HEAD
>> when switching to something other than a branch, whereas git-switch
>> requires the --detach option to do so.
>>
>> Add configuration variable checkout.autoDetach to choose the behavior
>> for both: true for automatic detaching, false for requiring --detach.
>>
>> Amend their documentation and tests accordingly.
>>
>> Signed-off-by: Andy Koppe <andy.koppe@gmail.com>
>> ---
>
> "switch" was meant to be an experimental command to sort out this
> kind of UI ideas, and I think the fact that it requires a more
> explicit "--detach", where experienced users might just say "git
> checkout that-branch^0", has established itself as a more friendly
> and good thing to help new users.
I agree, but as an experienced user, I nevertheless prefer switch and
restore over checkout, because those are rather different tasks, and
with checkout you're only ever a small thinko and errant dot away from
losing your local changes. If switch and restore had existed first, I
don't think anyone would be asking for mashing them together.
Incidentally, as reset is similarly overloaded, and restore can also
replace the forms of reset that take pathspec arguments, was there a
similar plan to factor the head-moving forms of reset out into a
separate command? (I realise there'd be little appetite for that after
the switch/restore experiment.)
> I do not know how others react to
> this kind of proliferation of configuration variables, but I do not
> mind this particular variable existing.
Thanks. There's also the checkout.guess variable as a closely related
precedent.
Regards,
Andy
^ permalink raw reply
* Re: [PATCH] fuzz: add new oss-fuzz fuzzer for date.c / date.h
From: Junio C Hamano @ 2023-11-12 12:39 UTC (permalink / raw)
To: Arthur Chan via GitGitGadget; +Cc: git, Arthur Chan
In-Reply-To: <xmqqfs1bo6l4.fsf@gitster.g>
Junio C Hamano <gitster@pobox.com> writes:
> As I used the first byte to fuzz dmtype and .local, let's use the
> next three bytes to allow feeding overly wild timezone values to the
> machinery and see what breaks, perhaps like so:
>
> tz = *data++; /* int tz; */
> tz = (tz << 8) | *data++;
> tz = (tz << 8) | *data++;
> size -= 3;
Just this part. As data points at unsigned char, the above would
not give us any negative number. We'd need to sign-extend the
24-bit resulting value if we are going to adopt the above approach.
^ permalink raw reply
* Declutter the home directory
From: Félix Piédallu @ 2023-11-12 12:10 UTC (permalink / raw)
To: git
Hi,
I'm new on this mailing list so please tell me if the subject was
already discussed.
The Freedesktop XDG Base Directory specification
(https://specifications.freedesktop.org/basedir-spec/basedir-spec-latest.html)
aims to declutter the home directory by moving files where they should
be more logically located. Here i'll be talking about some directories:
* $XDG_CACHE_HOME, that defaults to the well-known $HOME/.cache
* $XDG_CONFIG_HOME, that defaults to the also well known $HOME/.config
* $XDG_RUNTIME_DIR, that should contain non-essential runtime files
such as socket files
For now 4 kind of files are put in the $HOME directory by git:
* .gitconfig | .config/git/config
it defaults to .gitconfig
* .git-credentials | .config/git/credentials from the credential-store
it also defaults to .git-credentials
* .cache/git/credentials | .git-credential-cache/ from credential-cache
this one defaults to .cache/git/credentials/
* .git-fsmonitor-%s for Darwin file system monitor.
I have 3 propositions:
## Move runtimes files to $XDG_RUNTIME_DIR
* .cache/git/credential/ -> $XDG_RUNTIME_DIR/git/credential/
* .git-fsmonitor-%s -> $XDG_RUNTIME_DIR/git/fsmonitor/%s
Those file shall be deleted at the end of the user session, so this is
the good place to store them.
## Move ~/.config/git/credentials to ~/.cache/git/credentials
Indeed this might contain sensitive information that some users don't
want to be backed up or synchronized.
A possibility would be to read ~/.cache/git/credentials,
~/.config/git/credentials, ~/.git-credentials, and to use the first
existing one.
## Default to XDG paths instead of home directory
It would just swap the priority of the two possible paths. It would also
only impact new installations as, if ~/.gitconfig exists, it would keep
being used.
The goal of those propositions is to declutter the home directory, and
ease the cleanup, backup and synchronisation of user settings.
Thanks for reading me.
Félix Piédallu
^ permalink raw reply
* Re: Declutter the home directory
From: Dragan Simic @ 2023-11-12 13:39 UTC (permalink / raw)
To: Félix Piédallu; +Cc: git
In-Reply-To: <cb89d863-12d4-412b-a25b-9eedce996708@piedallu.me>
On 2023-11-12 13:10, Félix Piédallu wrote:
> ## Default to XDG paths instead of home directory
>
> It would just swap the priority of the two possible paths. It would
> also only impact new installations as, if ~/.gitconfig exists, it
> would keep being used.
Quite frankly, some configuration files should remain in $HOME, and
~/.gitconfig is IMHO one of those. It's used very frequently, not only
directly, but also while discussing or explaining various configuration
features or issues. Thus, moving it somewhere else would only make
things more complicated and worse.
IOW, I think that the order/priority shouldn't be changed.
^ permalink raw reply
* Re: first-class conflicts?
From: Theodore Ts'o @ 2023-11-12 15:21 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Sandra Snan, git, Dragan Simic, rsbecker
In-Reply-To: <xmqqh6ltrs6t.fsf@gitster.g>
On Sat, Nov 11, 2023 at 10:31:54AM +0900, Junio C Hamano wrote:
> Correct but with a caveat: it is too easy for lazy folks to
> circumvent the safety by mistake with "commit -a".
>
> I wonder if it would help users to add a new configuration option
> for those who want to live safer that tells "commit -a" to leave
> unmerged paths alone and require the unmerged paths to be added
> explicitly (which may have to extend to cover things like "add -u"
> and "add .").
>
> Perhaps not. I often find myself doing "git add -u" after resolving
> conflicts and re-reading the result, without an explicit pathspec.
Maybe the configuration option would also forbit "git add -u" from
adding diffs with conflict markers unless --force is added?
I dunno. I personally wouldn't use it myself, because I've always
made a point of running "git diff", or "git status", and almost
always, a command like "make -j16 && make -j16 check" (or an aliased
equivalent) before commiting a merge.
But that's because I'm a paranoid s.o.b. and in my long career, I've
learned is that "you can't be paranoid enough", and "hope is not a
strategy". :-)
- Ted
^ permalink raw reply
* Re: [RFC PATCH] status: avoid reporting worktrees as "Untracked files"
From: Eric Sunshine @ 2023-11-12 17:13 UTC (permalink / raw)
To: Edmundo Carmona Antoranz; +Cc: Junio C Hamano, git
In-Reply-To: <CAOc6etbowajhHsctFJN4ZQ0gND0jzZUrhEkep_pLYtE9y9RBCQ@mail.gmail.com>
On Sat, Nov 11, 2023 at 4:22 AM Edmundo Carmona Antoranz
<eantoranz@gmail.com> wrote:
> On Sat, Nov 4, 2023 at 7:58 AM Junio C Hamano <gitster@pobox.com> wrote:
> > What problem are you trying to solve? "git add foo" where "foo" is
> > actually a different worktree of the repository would add it as a
> > submodule that causes confusion? If that is the case, I think the
> > right solution is not to get into such a state, i.e. not create a
> > worktree of the repository inside a different worktree in the first
> > place.
> >
> Hey, guys! Thanks Junio and Eric for sharing your thoughts.
>
> I am not against the idea of creating worktrees outside of the
> repository... however, I like them to be _inside_ the repository. Am I
> the only one? IDK. I might be! It feels completely natural, if you ask
> me.... but that's just my opinion, I acknowledge that.
I doubt you're the only one, but, based upon, list emails over the
years, it seems that both in-main-tree and outside-main-tree (often
sibling) worktrees are common. More recently, we've also heard from
people who don't even have a main-worktree; instead, they hang their
multiple worktrees off of a bare repository (which is an
explicitly-supported use-case); i.e.:
git clone --bare https://.../foobar.git
git -C foobar.git worktree add worktree1
git -C foobar.git worktree add worktree2
...
^ permalink raw reply
* Re: [PATCH] format-patch: fix ignored encode_email_headers for cover letter
From: René Scharfe @ 2023-11-12 18:38 UTC (permalink / raw)
To: Jeff King; +Cc: git, Junio C Hamano, Simon Ser
In-Reply-To: <20231109183506.GB2711684@coredump.intra.peff.net>
Am 09.11.23 um 19:35 schrieb Jeff King:
> On Thu, Nov 09, 2023 at 11:19:56AM +0000, Simon Ser wrote:
>
>> When writing the cover letter, the encode_email_headers option was
>> ignored. That is, UTF-8 subject lines and email addresses were
>> written out as-is, without any Q-encoding, even if
>> --encode-email-headers was passed on the command line.
>>
>> This is due to encode_email_headers not being copied over from
>> struct rev_info to struct pretty_print_context. Fix that and add
>> a test.
>
> That makes sense, and your patch looks the right thing to do as an
> immediate fix. But I have to wonder:
>
> 1. Are there other bits that need to be copied?
Good question.
> Grepping for other
> code that does the same thing, I see that show_log() and
> cmd_format_patch() copy a lot more.
show_log() copies almost half of the struct 6d167fd7cc members
from struct rev_info!
cmd_format_patch() doesn't seem have a struct pretty_print_context,
though...
> (For that matter, why doesn't
> make_cover_letter() just use the context set up by
> cmd_format_patch()?).
... which answers this question, but did you perhaps mean a different
function?
> 2. Why are we copying this stuff at all? When we introduced the
> pretty-print context back in 6bf139440c (clean up calling
> conventions for pretty.c functions, 2011-05-26), the idea was just
> to keep all of the format options together. But later, 6d167fd7cc
> (pretty: use fmt_output_email_subject(), 2017-03-01) added a
> pointer to the rev_info directly.
Hmm. Was sticking the rev_info pointer in unwise? Does it tangle up
things that should be kept separate? It uses force_in_body_from,
grep_filter, sources, nr, total and subject_prefix from struct rev_info.
That seems a lot, but is just a small fraction of its total members and
we could just copy those we need. Or prepare the subject string and
pass it in, as before 6d167fd7cc.
> So could/should we just be using
> pp->rev->encode_email_headers here?
Perhaps. If we want struct pretty_print_context to be a collection of
named parameters, though, then it makes sense to avoid using
complicated types to provide a nice interface to its callers, and
struct rev_info is one of our largest structs we have.
> Or if that field is not always set (or we want to override some
> elements), should there be a single helper function to initialize
> the pretty_print_context from a rev_info, that could be shared
> between spots like show_log() and make_cover_letter()?
That would help avoid forgetting to copy something. But copying is
questionable in general, as you mentioned. Given the extent of the
overlap, would it make sense to embed struct pretty_print_context in
struct rev_info instead? Or a subset thereof?
René
^ permalink raw reply
* Re: Git Rename Detection Bug
From: Junio C Hamano @ 2023-11-12 23:09 UTC (permalink / raw)
To: Elijah Newren
Cc: Philip Oakley, Jeremy Pridmore, git@vger.kernel.org,
Paul Baumgartner
In-Reply-To: <CABPp-BEtva2WTGQG3Qs4EbZLK_RJC9vuA-2OYxkTPExgowwvqQ@mail.gmail.com>
Elijah Newren <newren@gmail.com> writes:
>> Could I suggest that we are missing a piece of terminology, to wit,
>> BLOBSAME. It's a compatriot to TREESAME, as used in `git log` for
>> history simplification (based on a tree's pathspec, most commonly a
>> commit's top level path).
>
> We could add it, but I'm not sure how it helps. We already had 'exact
> rename' which seems to fit the bill as well, and 'blob' is something
> someone new to Git is unlikely to know.
Also, as Philip said, TREESAME is a concept foreign to rename
detection codepath. It is a property of a commit (not a tree) and
tells us if it has the same tree object as its relevant parents (in
which case it can be simplified away if it is a merge). I do not
mind rename codepath using a jargon (or two) to express "in trees A
and B, this subtree of A records the same tree object as a subtree
of B at a different path (i.e., the contents of these two subtrees
at different paths are the same)" but the word used to express that
should not be TREESAME to avoid confusion. And the other word to
express "this path in tree A records a blob object that is identical
to this other path in tree B" should not be BLOBSAME, as the word
strongly hints it is somehow related to TREESAME.
Thanks.
^ 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