* Re: What's cooking in git.git (Nov 2023, #01; Thu, 2)
From: Christian Couder @ 2023-11-03 22:48 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <xmqqa5rwo7u7.fsf@gitster.g>
On Thu, Nov 2, 2023 at 9:53 AM Junio C Hamano <gitster@pobox.com> wrote:
> * ps/ci-gitlab (2023-11-02) 8 commits
> - ci: add support for GitLab CI
> - ci: install test dependencies for linux-musl
> - ci: squelch warnings when testing with unusable Git repo
> - ci: unify setup of some environment variables
> - ci: split out logic to set up failed test artifacts
> - ci: group installation of Docker dependencies
> - ci: make grouping setup more generic
> - ci: reorder definitions for grouping functions
>
> Add support for GitLab CI.
>
> Comments?
> source: <cover.1698843660.git.ps@pks.im>
I just took a look at this series and I like both the idea of having a
GitLab CI configuration in the code codebase and the implementation
which makes things as easy as possible to maintain.
> * cc/git-replay (2023-10-10) 14 commits
> - replay: stop assuming replayed branches do not diverge
> - replay: add --contained to rebase contained branches
> - replay: add --advance or 'cherry-pick' mode
> - replay: use standard revision ranges
> - replay: make it a minimal server side command
> - replay: remove HEAD related sanity check
> - replay: remove progress and info output
> - replay: add an important FIXME comment about gpg signing
> - replay: change rev walking options
> - replay: introduce pick_regular_commit()
> - replay: die() instead of failing assert()
> - replay: start using parse_options API
> - replay: introduce new builtin
> - t6429: remove switching aspects of fast-rebase
>
> Introduce "git replay", a tool meant on the server side without
> working tree to recreate a history.
>
> Expectting a (hopefully final and quick) reroll.
> cf. <bd872b81-80a9-5e4e-dcb6-faebc9671848@gmx.de>
> source: <20231010123847.2777056-1-christian.couder@gmail.com>
Reroll sent yesterday
(https://lore.kernel.org/git/20231102135151.843758-1-christian.couder@gmail.com/).
A quick review of the latest changes would be nice.
> * js/doc-unit-tests (2023-11-02) 3 commits
> - ci: run unit tests in CI
> - unit tests: add TAP unit test framework
> - unit tests: add a project plan document
> (this branch is used by js/doc-unit-tests-with-cmake.)
>
> Process to add some form of low-level unit tests has started.
>
> Will merge to 'next'?
> source: <cover.1698881249.git.steadmon@google.com>
I just took a look at the v9 of the series and it looks good despite a
small nit. Also I like the idea and implementation, so I agree with
merging it to 'next' and then 'master'.
^ permalink raw reply
* Re: [PATCH 4/5] ci: split out logic to set up failed test artifacts
From: Christian Couder @ 2023-11-03 22:35 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git
In-Reply-To: <4a864a1d174f7d4d36202a375302d450fbe9f2a3.1698305961.git.ps@pks.im>
On Thu, Oct 26, 2023 at 10:01 AM Patrick Steinhardt <ps@pks.im> wrote:
>
> We have some logic in place to create a directory with the output from
> failed tests, which will then subsequently be uploaded as CI artifact.
> We're about to add support for GitLab CI, which will want to reuse the
> logic.
>
> Split the logic into a separate function so that it is reusable.
>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
> ci/lib.sh | 40 ++++++++++++++++++++++------------------
> 1 file changed, 22 insertions(+), 18 deletions(-)
>
> diff --git a/ci/lib.sh b/ci/lib.sh
> index 957fd152d9c..33005854520 100755
> --- a/ci/lib.sh
> +++ b/ci/lib.sh
> @@ -137,6 +137,27 @@ handle_failed_tests () {
> return 1
> }
>
> +create_failed_test_artifacts () {
> + mkdir -p t/failed-test-artifacts
> +
> + for test_exit in t/test-results/*.exit
> + do
> + test 0 != "$(cat "$test_exit")" || continue
> +
> + test_name="${test_exit%.exit}"
> + test_name="${test_name##*/}"
> + printf "\\e[33m\\e[1m=== Failed test: ${test_name} ===\\e[m\\n"
> + echo "The full logs are in the 'print test failures' step below."
> + echo "See also the 'failed-tests-*' artifacts attached to this run."
> + cat "t/test-results/$test_name.markup"
> +
> + trash_dir="t/trash directory.$test_name"
> + cp "t/test-results/$test_name.out" t/failed-test-artifacts/
> + tar czf t/failed-test-artifacts/"$test_name".trash.tar.gz "$trash_dir"
> + done
> + return 1
Nit: I think it's more the responsibility of handle_failed_tests() to
`return 1` than of create_failed_test_artifacts(). I understand that
putting it into this new function means one more line that is common
to several CI tools though.
Otherwise everything in this series LGTM. Thanks!
^ permalink raw reply
* RE: Request for Help - Too many perl arguments as of 2.43.0-rc0
From: rsbecker @ 2023-11-03 22:04 UTC (permalink / raw)
To: 'Eric Sunshine'; +Cc: 'Git List'
In-Reply-To: <CAPig+cRG3VWgnwOPWvbW9boUooa6sD9d667wbSUe4_RkcR60vg@mail.gmail.com>
On November 3, 2023 5:51 PM, Eric Sunshine wrote:
>On Fri, Nov 3, 2023 at 5:31 PM <rsbecker@nexbridge.com> wrote:
>> On November 3, 2023 4:52 PM, Eric Sunshine wrote:
>> >On Fri, Nov 3, 2023 at 3:03 PM <rsbecker@nexbridge.com> wrote:
>> >> I can no longer test git as the number of arguments (and size) sent
>> >> to perl from the make infrastructure is too big for my platform. Is
>> >> it possible to use xargs instead?
>> >
>> >Presumably you're talking about the invocation of chainlint.pl?
>> >Considering that that's quite an unimportant part of `make test` for
>> >those just running the test suite (as opposed to those authoring new
>> >tests), the simplest workaround is to disable chainlint when you run
>> >the tests by setting environment variable GIT_TEST_EXT_CHAIN_LINT to 0.
>>
>> I tried both ways to specify this variable with no success:
>>
>> /usr/coreutils/bin/bash: /usr/bin/perl: Argument list too long
>> Makefile:125: recipe for target 'test-lint-shell-syntax' failed
>
>This is a separate issue from chainlint (though, it too will likely suffer the same
>problem). In this case, it's trying to run the check-non-portable-shell.pl script and
>the command-line is too long.
>Unfortunately, unlike chainlint for which there is a knob to disble it, there is no direct
>knob to disable `test-lint-shell-syntax`. You may be able to skip all the shell-script
>linting by setting TEST_LINT to an empty value.
That worked. I wonder whether it might be useful to set up a knob for this.
^ permalink raw reply
* Re: [PATCH v9 2/3] unit tests: add TAP unit test framework
From: Christian Couder @ 2023-11-03 21:54 UTC (permalink / raw)
To: Josh Steadmon
Cc: git, Junio C Hamano, Phillip Wood, Randall S. Becker,
Oswald Buddenhagen
In-Reply-To: <8b831f4937a167519b77c724fcbc36884bbfe43d.1698881249.git.steadmon@google.com>
On Thu, Nov 2, 2023 at 12:31 AM Josh Steadmon <steadmon@google.com> wrote:
>
> From: Phillip Wood <phillip.wood@dunelm.org.uk>
> +int test_assert(const char *location, const char *check, int ok)
> +{
> + assert(ctx.running);
> +
> + if (ctx.result == RESULT_SKIP) {
> + test_msg("skipping check '%s' at %s", check, location);
> + return 1;
> + } else if (!ctx.todo) {
I suggested removing the "else" and moving the "if (!ctx.todo) {" to
its own line in the previous round and thought you agreed with that,
but maybe it fell through the cracks somehow.
Anyway I think this is a minor nit, and the series looks good to me.
> + if (ok) {
> + test_pass();
> + } else {
> + test_msg("check \"%s\" failed at %s", check, location);
> + test_fail();
> + }
> + }
> +
> + return !!ok;
> +}
^ permalink raw reply
* Re: Request for Help - Too many perl arguments as of 2.43.0-rc0
From: Eric Sunshine @ 2023-11-03 21:50 UTC (permalink / raw)
To: rsbecker; +Cc: Git List
In-Reply-To: <00c301da0e9d$0d2cb8e0$27862aa0$@nexbridge.com>
On Fri, Nov 3, 2023 at 5:31 PM <rsbecker@nexbridge.com> wrote:
> On November 3, 2023 4:52 PM, Eric Sunshine wrote:
> >On Fri, Nov 3, 2023 at 3:03 PM <rsbecker@nexbridge.com> wrote:
> >> I can no longer test git as the number of arguments (and size) sent to
> >> perl from the make infrastructure is too big for my platform. Is it
> >> possible to use xargs instead?
> >
> >Presumably you're talking about the invocation of chainlint.pl?
> >Considering that that's quite an unimportant part of `make test` for those just
> >running the test suite (as opposed to those authoring new tests), the simplest
> >workaround is to disable chainlint when you run the tests by setting environment
> >variable GIT_TEST_EXT_CHAIN_LINT to 0.
>
> I tried both ways to specify this variable with no success:
>
> /usr/coreutils/bin/bash: /usr/bin/perl: Argument list too long
> Makefile:125: recipe for target 'test-lint-shell-syntax' failed
This is a separate issue from chainlint (though, it too will likely
suffer the same problem). In this case, it's trying to run the
check-non-portable-shell.pl script and the command-line is too long.
Unfortunately, unlike chainlint for which there is a knob to disble
it, there is no direct knob to disable `test-lint-shell-syntax`. You
may be able to skip all the shell-script linting by setting TEST_LINT
to an empty value.
^ permalink raw reply
* RE: Request for Help - Too many perl arguments as of 2.43.0-rc0
From: rsbecker @ 2023-11-03 21:30 UTC (permalink / raw)
To: 'Eric Sunshine'; +Cc: 'Git List'
In-Reply-To: <CAPig+cTzWCpmksJm-0M1MfUSeYpWbgMaFNHRSOtAhCsNQigV-g@mail.gmail.com>
On November 3, 2023 4:52 PM, Eric Sunshine wrote:
>On Fri, Nov 3, 2023 at 3:03 PM <rsbecker@nexbridge.com> wrote:
>> I can no longer test git as the number of arguments (and size) sent to
>> perl from the make infrastructure is too big for my platform. Is it
>> possible to use xargs instead?
>
>Presumably you're talking about the invocation of chainlint.pl?
>Considering that that's quite an unimportant part of `make test` for those just
>running the test suite (as opposed to those authoring new tests), the simplest
>workaround is to disable chainlint when you run the tests by setting environment
>variable GIT_TEST_EXT_CHAIN_LINT to 0.
I tried both ways to specify this variable with no success:
/home/randall/git: make -C t/ all SHELL=/usr/coreutils/bin/bash GIT_TEST_EXT_CHAIN_LINT=0
make: Entering directory '/home/randall/git/t'
rm -f -r 'test-results'
/usr/coreutils/bin/bash: /usr/bin/perl: Argument list too long
Makefile:125: recipe for target 'test-lint-shell-syntax' failed
make: *** [test-lint-shell-syntax] Error 126
make: Leaving directory '/home/randall/git/t'
/home/randall/git: GIT_TEST_EXT_CHAIN_LINT=0 make -C t/ all SHELL=/usr/coreutils/bin/bash
make: Entering directory '/home/randall/git/t'
rm -f -r 'test-results'
/usr/coreutils/bin/bash: /usr/bin/perl: Argument list too long
Makefile:125: recipe for target 'test-lint-shell-syntax' failed
make: *** [test-lint-shell-syntax] Error 126
make: Leaving directory '/home/randall/git/t'
^ permalink raw reply
* [PATCH 1/2] rebase: support non-interactive autosquash
From: Andy Koppe @ 2023-11-03 21:29 UTC (permalink / raw)
To: git; +Cc: newren, Andy Koppe
So far, the rebase --autosquash option and rebase.autoSquash=true
config setting are quietly ignored when used without --interactive,
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.
Since the "merge" backend used for interactive rebase also is the
default for non-interactive rebase, there does not appear to be a
reason not to do --autosquash without --interactive, so support that.
Turn rebase.autoSquash into a multi-choice setting, with "interactive"
enabling autosquashing for --interactive, and "always" enabling it
without --interactive as well. Make boolean true values synonyms for
"interactive" for backward compatibility.
Don't prevent fast-forwards or report conflicts with --apply options
for rebase.autoSquash=interactive mode when rebase is invoked without
--interactive.
Change the git-rebase and config/rebase documentation accordingly, and
extend t3415-rebase-autosquash.sh to test the new rebase.autosquash
values and combinations with and without --interactive.
Signed-off-by: Andy Koppe <andy.koppe@gmail.com>
---
Documentation/config/rebase.txt | 4 +-
Documentation/git-rebase.txt | 2 +-
builtin/rebase.c | 45 +++++++++++------
t/t3415-rebase-autosquash.sh | 70 ++++++++++++++++++++------
t/t3422-rebase-incompatible-options.sh | 2 +-
5 files changed, 92 insertions(+), 31 deletions(-)
diff --git a/Documentation/config/rebase.txt b/Documentation/config/rebase.txt
index 9c248accec..7e5ec74e00 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.
+ When set to 'interactive' or 'true', enable the `--autosquash` option
+ for interactive rebase. When set to 'always', enable it for
+ non-interactive rebase as well. Defaults to 'false'.
rebase.autoStash::
When set to true, automatically create a temporary stash entry
diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
index e7b39ad244..102ff91493 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 261a9a61fc..35792744fc 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -85,6 +85,12 @@ static const char *action_names[] = {
"show_current_patch"
};
+enum config_autosquash {
+ AUTOSQUASH_NO = 0,
+ AUTOSQUASH_INTERACTIVE = 1,
+ AUTOSQUASH_ALWAYS
+};
+
struct rebase_options {
enum rebase_type type;
enum empty_type empty;
@@ -131,7 +137,7 @@ struct rebase_options {
int reapply_cherry_picks;
int fork_point;
int update_refs;
- int config_autosquash;
+ enum config_autosquash config_autosquash;
int config_rebase_merges;
int config_update_refs;
};
@@ -149,7 +155,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, \
@@ -711,10 +716,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);
@@ -772,7 +775,16 @@ static int rebase_config(const char *var, const char *value,
}
if (!strcmp(var, "rebase.autosquash")) {
- opts->config_autosquash = git_config_bool(var, value);
+ int b = git_parse_maybe_bool(value);
+
+ if (b == 0)
+ opts->config_autosquash = AUTOSQUASH_NO;
+ else if (b == 1 || !strcmp(value, "interactive"))
+ opts->config_autosquash = AUTOSQUASH_INTERACTIVE;
+ else if (!strcmp(value, "always"))
+ opts->config_autosquash = AUTOSQUASH_ALWAYS;
+ else
+ die(_("invalid value for '%s': '%s'"), var, value);
return 0;
}
@@ -1402,13 +1414,22 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
state_dir_base, cmd_live_rebase, buf.buf);
}
+ if (options.autosquash == 1) {
+ imply_merge(&options, "--autosquash");
+ } else if (options.autosquash == -1) {
+ options.autosquash =
+ options.config_autosquash == AUTOSQUASH_ALWAYS ||
+ (options.config_autosquash == AUTOSQUASH_INTERACTIVE &&
+ options.flags & REBASE_INTERACTIVE_EXPLICIT);
+ }
+
if ((options.flags & REBASE_INTERACTIVE_EXPLICIT) ||
(options.action != ACTION_NONE) ||
(options.exec.nr > 0) ||
- (options.autosquash == -1 && options.config_autosquash == 1) ||
- options.autosquash == 1) {
+ options.autosquash) {
allow_preemptive_ff = 0;
}
+
if (options.committer_date_is_author_date || options.ignore_date)
options.flags |= REBASE_FORCE;
@@ -1508,7 +1529,7 @@ 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)
+ else if (options.autosquash && options.config_autosquash)
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"));
@@ -1529,11 +1550,6 @@ 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)
- imply_merge(&options, "--autosquash");
- options.autosquash = (options.autosquash >= 0) ? options.autosquash :
- ((options.config_autosquash >= 0) ? options.config_autosquash : 0);
-
if (options.type == REBASE_UNSPECIFIED) {
if (!strcmp(options.default_backend, "merge"))
options.type = REBASE_MERGE;
@@ -1858,3 +1874,4 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
free(keep_base_onto_name);
return !!ret;
}
+
diff --git a/t/t3415-rebase-autosquash.sh b/t/t3415-rebase-autosquash.sh
index a364530d76..4b48414b3d 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,36 @@ 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 "-i --autosquash"
'
-test_expect_success 'auto fixup (config)' '
- git config rebase.autosquash true &&
- test_auto_fixup final-fixup-config-true &&
- test_auto_fixup ! fixup-config-true-no --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_expect_success 'auto fixup (config true)' '
+ git config rebase.autosquash true &&
+ test_auto_fixup ! fixup-config-true &&
+ test_auto_fixup fixup-config-true-i -i &&
+ test_auto_fixup ! fixup-config-true-i-no "-i --no-autosquash"
+'
+
+test_expect_success 'auto fixup (config interactive)' '
+ git config rebase.autosquash interactive &&
+ test_auto_fixup ! fixup-config-interactive &&
+ test_auto_fixup fixup-config-interactive-i -i &&
+ test_auto_fixup ! fixup-config-interactive-i-no "-i --no-autosquash"
+'
+
+test_expect_success 'auto fixup (config always)' '
+ git config rebase.autosquash always &&
+ test_auto_fixup fixup-config-always &&
+ test_auto_fixup fixup-config-always-i -i &&
+ test_auto_fixup ! fixup-config-always-no --no-autosquash &&
+ test_auto_fixup ! fixup-config-always-i-no "-i --no-autosquash"
'
test_auto_squash () {
@@ -87,7 +108,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 +126,36 @@ 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 "-i --autosquash"
'
-test_expect_success 'auto squash (config)' '
- git config rebase.autosquash true &&
- test_auto_squash final-squash-config-true &&
- test_auto_squash ! squash-config-true-no --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_expect_success 'auto squash (config true)' '
+ git config rebase.autosquash true &&
+ test_auto_squash ! squash-config-true &&
+ test_auto_squash squash-config-true-i -i &&
+ test_auto_squash ! squash-config-true-i-no "-i --no-autosquash"
+'
+
+test_expect_success 'auto squash (config interactive)' '
+ git config rebase.autosquash interactive &&
+ test_auto_squash ! squash-config-interactive &&
+ test_auto_squash squash-config-interactive-i -i &&
+ test_auto_squash ! squash-config-interactive-i-no "-i --no-autosquash"
+'
+
+test_expect_success 'auto squash (config always)' '
+ git config rebase.autosquash always &&
+ test_auto_squash squash-config-always &&
+ test_auto_squash squash-config-always-i -i &&
+ test_auto_squash ! squash-config-always-no --no-autosquash &&
+ test_auto_squash ! squash-config-always-i-no "-i --no-autosquash"
'
test_expect_success 'misspelled auto squash' '
diff --git a/t/t3422-rebase-incompatible-options.sh b/t/t3422-rebase-incompatible-options.sh
index 2eba00bdf5..d9feb3a89d 100755
--- a/t/t3422-rebase-incompatible-options.sh
+++ b/t/t3422-rebase-incompatible-options.sh
@@ -102,7 +102,7 @@ test_rebase_am_only () {
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 &&
+ test_must_fail git -c rebase.autosquash=always rebase $opt A 2>err &&
grep -e --no-autosquash err
"
--
2.43.0-rc0
^ permalink raw reply related
* [PATCH 2/2] docs: rewrite rebase --(no-)autosquash description
From: Andy Koppe @ 2023-11-03 21:29 UTC (permalink / raw)
To: git; +Cc: newren, Andy Koppe
In-Reply-To: <20231103212958.18472-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 | 32 ++++++++++++++++++--------------
1 file changed, 18 insertions(+), 14 deletions(-)
diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
index 102ff91493..594158fcbc 100644
--- a/Documentation/git-rebase.txt
+++ b/Documentation/git-rebase.txt
@@ -589,21 +589,25 @@ 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. 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 start of the subject line or the hash of that commit.
+
-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, commits marked for squashing are moved right after
+the commits they modify, and their action is changed from `pick` to `squash`,
+`fixup` or `fixup -C`, depending on the squash marker. 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.
++
+The configuration variable `rebase.autoSquash` can be used to enable
+`--autosquash` by default. See the CONFIGURATION section below for details.
+The `--no-autosquash` option overrides that setting.
+
See also INCOMPATIBLE OPTIONS below.
--
2.43.0-rc0
^ permalink raw reply related
* Re: Request for Help - Too many perl arguments as of 2.43.0-rc0
From: Eric Sunshine @ 2023-11-03 20:52 UTC (permalink / raw)
To: rsbecker; +Cc: Git List
In-Reply-To: <00a101da0e88$778cd3b0$66a67b10$@nexbridge.com>
On Fri, Nov 3, 2023 at 3:03 PM <rsbecker@nexbridge.com> wrote:
> I can no longer test git as the number of arguments (and size) sent to perl
> from the make infrastructure is too big for my platform. Is it possible to
> use xargs instead?
Presumably you're talking about the invocation of chainlint.pl?
Considering that that's quite an unimportant part of `make test` for
those just running the test suite (as opposed to those authoring new
tests), the simplest workaround is to disable chainlint when you run
the tests by setting environment variable GIT_TEST_EXT_CHAIN_LINT to
0.
^ permalink raw reply
* RE: [BUG] Git 2.43.0-rc0 - t4216 unpack(Q) invalid type
From: rsbecker @ 2023-11-03 19:18 UTC (permalink / raw)
To: 'Jeff King'; +Cc: git
In-Reply-To: <20231103162119.GC1470570@coredump.intra.peff.net>
On November 3, 2023 12:21 PM Jeff King wrote:
>On Fri, Nov 03, 2023 at 12:07:17PM -0400, rsbecker@nexbridge.com wrote:
>
>> What I get from Perl is
>> $ perl -e '
>> > my $bytes = "\1\2\3\4\5\6\7\8";
>> > my $q = eval { unpack("Q>", $bytes) };
>> > print "Q = ", defined($q) ? $q : "($@)", "\n";
>> > my ($n1, $n2) = unpack("NN", $bytes);
>> > print "n1 = $n1\n";
>> > print "n2 = $n2\n";
>> > print "computed quad = ", ($n1 << 32) | $n2, "\n"; '
>> Q = (Invalid type 'Q' in unpack at -e line 3.
>> )
>> n1 = 16909060
>> n2 = 84281144
>> computed quad = 84281144
>
>OK, that matches what I expected. Hopefully the patch I just sent (our mails just
>crossed) will fix it for you.
>
>> Because perl itself is 32-bit, not 64-bit on this platform. So even
>> moving git to 64-bit will not correct the issue.
>
>Yep, exactly.
Your patch worked! Thanks.
^ permalink raw reply
* Request for Help - Too many perl arguments as of 2.43.0-rc0
From: rsbecker @ 2023-11-03 19:03 UTC (permalink / raw)
To: 'Git List'
I can no longer test git as the number of arguments (and size) sent to perl
from the make infrastructure is too big for my platform. Is it possible to
use xargs instead?
Thanks,
Randall
--
Brief whoami: NonStop&UNIX developer since approximately
UNIX(421664400)
NonStop(211288444200000000)
-- In real life, I talk too much.
^ permalink raw reply
* Re: why does git set X in LESS env var?
From: Dragan Simic @ 2023-11-03 18:38 UTC (permalink / raw)
To: Andy Koppe
Cc: Thomas Guyot, Junio C Hamano, Jeff King, Christoph Anton Mitterer,
git
In-Reply-To: <CAHWeT-aftfawZ9DhqC1NkQrJAuArJZC65ZDr4FyPwEc7aXGVuA@mail.gmail.com>
On 2023-11-03 16:28, Andy Koppe wrote:
> Thomas Guyot wrote:
>> >> I actually have one major issue with it, it's that displaying anything
>> >> less than a full page will fill the screen with ~ on the bottom, just
>> >> like when scrolling up on a partial page without -F.
>
> 'less' has the '-~' (or --tilde) option to suppress that.
Good to know, thanks. However, with the "-F" option in place, the
situation in which less than a full page is displayed within less(1)
shouldn't be encountered.
> For what it's worth, the 'mintty' terminal used by default for Git for
> Windows as well as MSYS and Cygwin has another approach to the whole
> problem. Its rather flippantly named 'Flip Screen' context menu
> command with Alt+F12 or Ctrl+Shift+S shortcut lets users temporarily
> look at the alternate screen buffer while the main screen buffer is
> active, and vice versa.
This is a rather neat feature. Though, I wonder how many users are
actually aware of this feature, and how frequently is it used. I wasn't
aware of it, and I used Git CLI on Windows for some time.
> If 'less' is invoked without the -X option, it will switch to the
> alternate screen, where mousewheel scrolling works by sending cursor
> up/down keycodes. While in 'less', you can temporarily flip to the
> main screen to look up something in the shell session there or copy
> something for searching in 'less'. While looking at the main screen,
> the mousewheel will scroll the scrollback buffer. Keyboard input
> that's sent to 'less' will flip back to the alternate screen.
To me, this is another confirmation that (ab)using the "-X" option is
something that we need to get rid of, as I already described earlier in
this email thread.
> Quitting 'less' switches back to the main screen, so the 'less' output
> disappears and you're back in the shell session with the command that
> invoked 'less' as the last thing shown. But again, the 'Flip Screen'
> command or shortcuts can be used to temporarily look at or copy from
> the alternate screen, which will contain the last page displayed by
> 'less'. (The alternate screen does not have a scrollback buffer.)
>
> The 'Flip Screen' feature of course also works with other
> alternate-screen applications, for example editors.
>
> Apparently the Mac terminal has such a feature as well:
> https://support.apple.com/en-ie/guide/terminal/trmld1f46097/mac
Maybe there is some data available about how frequently this neat
feature is used? It would be really good to know how much is it
actually used.
> (Full disclosure: I originally made mintty, from PuTTY.)
Thanks for your work!
^ permalink raw reply
* Re: why does git set X in LESS env var?
From: Dragan Simic @ 2023-11-03 18:22 UTC (permalink / raw)
To: Thomas Guyot; +Cc: Junio C Hamano, Jeff King, Christoph Anton Mitterer, git
In-Reply-To: <24c46f25-6b31-437d-9f89-1e8eb74136c8@gmail.com>
On 2023-11-03 12:47, Thomas Guyot wrote:
> On 2023-11-02 10:19, Dragan Simic wrote:
>> On 2023-11-02 14:19, Thomas Guyot wrote:
>>> That's correct, you need both and also -y0
>>
>> Hmm, I tried the following:
>>
>> GIT_PAGER='less -R -F -X -c -y0'
>>
>> In my environment (Xfce), the result after scrolling the output of
>> "git
>> log -p" up and down a bit was about 20 copies of the same screen
>> "page"
>> in the scrollback, plus a couple of blank "pages". Not good,
>> unfortunately, and actually much worse than having just "-R -F -X".
>
> Indeed, I did notice it with xfce4-term - I suspect different controls
> are used to clear the screen, with rxvt-unicode the initial one
> scrolls anything in the current display to the scrollback (which is
> important to avoid clearing the last commands/output form the
> scrollback), but afterward screen updates do not update the
> scrollback, it remains within the screen.
>
> These options may actually affect the behavior (I have put my current
> values there)
>
> secondaryScreen: True
> Turn on/off secondary screen (default enabled).
>
> secondaryScroll: True
> Turn on/off secondary screen scroll (default enabled). If this
> option is enabled, scrolls on the secondary screen will change
> the
> scrollback buffer and, when secondaryScreen is off, switching
> to/from the secondary screen will instead scroll the screen up.
>
> So it appears I could disable secondaryScroll to avoid getting
> scrolled lines into the scrollback buffer. It's also noteworthy that
> with secondaryScreen disabled, rxvt-unicode instead scroll things up
> to avoid the current screen form being wiped out.
This is quite interesting. However, we can't expect that the users
perform such adjustments to their environments, if you agree. Tweaking
one's own environment is fine, but the shipped solution should "just
work" in most environments, or ideally in all environments in their
original form.
>>> Indeed, but when less update from the bottom, it can add new lines
>>> and
>>> let the overflow lines scroll up into the scrollback buffer.
>>>
>>> Then updating it from the top, it draws the whole page, top to
>>> bottom.
>>> That's fine for a full page but not desired for a partial one. Also
>>> note that on my terminal (rxvt-unicode) when less clears the screen
>>> to
>>> draw the first page the current screen is rolled up into scrollback -
>>> iirc that's a configurable option, it would be worth testing other
>>> terminal's behavior on that. IIRC it may also erase it when using the
>>> wrong termcap file.
>>>
>>> I haven't looked at the code, but I think it could be possibly to
>>> start the -c behavior only after a full page is drawn, after exiting
>>> on partial pages, which would give us the best of both worlds.
>>
>> Does the GIT_PAGER setup, as I described it above, work for you
>> without
>> the described artifacts, in any of the environments you have access
>> to?
>
> Yes, rxvt-unicode, with the settings mentioned above.
Good to know, thanks. Looking forward to see will the
"--redraw-on-quit" option work as expected in your rxvt-unicode
terminal.
>> One of the solutions is to edit the terminfo entry manually and remove
>> the escape codes that cause the switching to and from alternate
>> screen,
>> which I tested, but that also introduced another issue -- the screen
>> contents was always present after less(1) exited, which isn't always
>> the
>> desired behavior.
>
> But when less scrolls down (line by line, not page by page), it always
> append lines and let them scroll up. Won't you see these?
> (page-by-page otoh will redraw the full screen without scrolling).
>
> Also won't it wipe the *current* screen so that you won't see the
> commands/output you had *before* running less?
>
> This is why rxvt-unicode has an option to disable secondary screen,
> and scroll contents all the way to the buffer on switch to/from
> secondary screen.
IIRC, when I tested by removing the escape codes manually from the
terminfo entry, it all worked fine, but the screen contents always
remained displayed after less(1) exited. That isn't always the desired
behavior.
>>> Actually I just tested my
>>> solution on xfce4-terminal and it doesn't work, the terminal still
>>> push up stuff above on redraw (noteworthy is with rxvt-unicode the
>>> first draw pushes the current screen contents up but no other redraw
>>> does, which is what makes it work so well - I haven't tried to find
>>> out what is being done exactly... OTOH the redraw on scroll down is
>>> slightly noticeable there, while impossible to see on xfce4-terminal.
>>> I'll install the latest less and see what happens with --redraw on
>>
>> Please test the "--redraw-on-quit" option, so far it's the best
>> available solution, IMHO.
>
> I will, not now though - need it compile less form source I guess...
Great, thanks. Looking forward to the results of your testing. As a
reminder, this is what I use:
GIT_PAGER='less -R -F --redraw-on-quit'
My LESS environment variable contains only some coloring-related
options, which don't matter in this case.
>>> Now, if all other terms misbehave here, that's an issue, making this
>>> suggestion mostly useless. And considering the number of Windows
>>> users
>>> we absolutely need to test Windows Terminal, and should probably test
>>> MacOS's term too (whatever that is).
>>
>> Quite frankly, I think that such a solution would be like "fixing the
>> fix, which is actually an abuse", as I described it above, eventually
>> introducing even more issues, instead of solving the original issue.
>
> I'll let you know my findings... I'm not convinced --redraw-on-quit is
> actually going to fix it for all unless this does a lot more than the
> option name implies (but quite happy if it does).
Actually, it's more about not (ab)using the "-X" option, because
skipping the terminal initialization may cause various issues. The
"--redraw-on-quit" option is there just to have the screen contents
preserved after less(1) exits.
^ permalink raw reply
* Re: [PATCH 2/2] pretty: add '%aA' to show domain-part of email addresses
From: Kousik Sanagavarapu @ 2023-11-03 17:20 UTC (permalink / raw)
To: Andy Koppe; +Cc: Liam Beguin, git
In-Reply-To: <4e3a5661-cba0-4849-8340-06d8d4094f48@gmail.com>
On Fri, Nov 03, 2023 at 08:22:05AM +0000, Andy Koppe wrote:
>
> On 27/10/2023 19:40, Kousik Sanagavarapu wrote:
> > So, if we have a domain-name, we grab it, else (the case where we don't
> > have '@') we grab it as-is. Looks good.
>
> I'm not sure that this is the right way to handle a missing '@' here
> actually, because %al already returns the whole email field in that case,
> which makes sense as the likes of the 'mail' command would interpret it as a
> local username.
>
> And if someone was going to use %al and the new specifier together to format
> the parts of the email field differently, they probably wouldn't want the
> field to appear twice.
>
> Therefore I think it would be more appropriate to expand to nothing in that
> case. Tools that consume this output would already need to be able to deal
> with the empty case, as it could also happen if there's a single '@' at the
> end of the email field, or if the field is empty.
I originally thought since localpart and the new domainpart are like
counterparts (or are symmetrical, like Peff mentioned), falling back
like in the case of localpart was the correct way here (again, symmetry).
Having read your reasoning though I think it makes sense to not fall back the
same way as localpart (%al) and return empty instead.
This goes in favor of what Liam said in the original commit message of this
feature being used to keep track of commits from different organizations (as
in from GitHub or GitLab) or at least domains that make sense. As, if we
return the whole email (where the email is with no domain-part or '@' is at
the end of the email), the whole purpose of our new feature is lost.
Thanks
^ permalink raw reply
* [PATCH 1/1] attr: enable attr pathspec magic for git-add and git-stash
From: Joanna Wang @ 2023-11-03 16:34 UTC (permalink / raw)
To: jojwang; +Cc: git, gitster
In-Reply-To: <20231103143508.896745-1-jojwang@google.com>
This lets users limit files or exclude files based on file
attributes during git-add and git-stash.
For example, the chromium project would like to use this like
"git add --all ':(exclude,attr:submodule)'", as submodules are managed in a
unique way and often results in submodule changes that users do not want in
their commits.
This does not change any attr magic implementation. It is only adding
attr as an allowed pathspec in git-add and git-stash, which was previously
blocked by GUARD_PATHSPEC and a pathspec mask in parse_pathspec()).
However, this does fix a bug in prefix_magic() where attr values were unintentionally removed.
This was hit whenever parse_pathspec() is called with PATHSPEC_PREFIX_ORIGIN as a flag,
which was the case for git-stash.
(Bug originally filed here:
https://lore.kernel.org/git/CAMmZTi-0QKtj7Q=sbC5qhipGsQxJFOY-Qkk1jfkRYwfF5FcUVg@mail.gmail.com/)
Furthermore, while other commands hit this code path it did not result in unexpected
behavior because this bug only impacts the pathspec->items->original field which is
NOT used to filter paths. However, git-stash does use pathspec->items->original when
building args used to call other git commands.
(See add_pathspecs() usage and implementation in stash.c)
It is possible that when the attr pathspec feature was first added in
b0db704652 (pathspec: allow querying for attributes, 2017-03-13),
"PATHSPEC_ATTR" was just unintentionally left out of a few GUARD_PATHSPEC() invocations.
Later, to get a more user-friendly error message when attr was used with git-add,
PATHSPEC_ATTR was added as a mask to git-add's invocation of parse_pathspec()
84d938b732 (add: do not accept pathspec magic 'attr', 2018-09-18).
However, this user-friendly error message was never added for git-stash.
Signed-off-by: Joanna Wang <jojwang@google.com>
---
I fixed a typo above. "(But originally" -> "(Bug originally"
> So if element magic is not 0, we know the body is in the expected
> form (either long or short).
Also while this is still true AFAICT, I found an existing bug in parse_short_magic()
where it does not handle paths that start with ":" where ":" is part of the
file name. e.g. "git stash push -- ':chicken'" is not handled correcly. But I feel
this is a seperate topic from this patch. So I'll file a bug to address it seperately.
builtin/add.c | 7 ++-
dir.c | 3 +-
pathspec.c | 36 ++++++++---
t/t6135-pathspec-with-attrs.sh | 108 +++++++++++++++++++++++++++++++--
4 files changed, 136 insertions(+), 18 deletions(-)
diff --git a/builtin/add.c b/builtin/add.c
index 5126d2ede3..d46e4d10e9 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -424,7 +424,7 @@ int cmd_add(int argc, const char **argv, const char *prefix)
* Check the "pathspec '%s' did not match any files" block
* below before enabling new magic.
*/
- parse_pathspec(&pathspec, PATHSPEC_ATTR,
+ parse_pathspec(&pathspec, 0,
PATHSPEC_PREFER_FULL |
PATHSPEC_SYMLINK_LEADING_PATH,
prefix, argv);
@@ -433,7 +433,7 @@ int cmd_add(int argc, const char **argv, const char *prefix)
if (pathspec.nr)
die(_("'%s' and pathspec arguments cannot be used together"), "--pathspec-from-file");
- parse_pathspec_file(&pathspec, PATHSPEC_ATTR,
+ parse_pathspec_file(&pathspec, 0,
PATHSPEC_PREFER_FULL |
PATHSPEC_SYMLINK_LEADING_PATH,
prefix, pathspec_from_file, pathspec_file_nul);
@@ -504,7 +504,8 @@ int cmd_add(int argc, const char **argv, const char *prefix)
PATHSPEC_LITERAL |
PATHSPEC_GLOB |
PATHSPEC_ICASE |
- PATHSPEC_EXCLUDE);
+ PATHSPEC_EXCLUDE |
+ PATHSPEC_ATTR);
for (i = 0; i < pathspec.nr; i++) {
const char *path = pathspec.items[i].match;
diff --git a/dir.c b/dir.c
index 16fdb03f2a..4d1cd039be 100644
--- a/dir.c
+++ b/dir.c
@@ -2179,7 +2179,8 @@ static int exclude_matches_pathspec(const char *path, int pathlen,
PATHSPEC_LITERAL |
PATHSPEC_GLOB |
PATHSPEC_ICASE |
- PATHSPEC_EXCLUDE);
+ PATHSPEC_EXCLUDE |
+ PATHSPEC_ATTR);
for (i = 0; i < pathspec->nr; i++) {
const struct pathspec_item *item = &pathspec->items[i];
diff --git a/pathspec.c b/pathspec.c
index bb1efe1f39..2f8721cc15 100644
--- a/pathspec.c
+++ b/pathspec.c
@@ -109,16 +109,34 @@ static struct pathspec_magic {
{ PATHSPEC_ATTR, '\0', "attr" },
};
-static void prefix_magic(struct strbuf *sb, int prefixlen, unsigned magic)
+static void prefix_magic(struct strbuf *sb, int prefixlen, unsigned magic, const char *element)
{
- int i;
- strbuf_addstr(sb, ":(");
- for (i = 0; i < ARRAY_SIZE(pathspec_magic); i++)
- if (magic & pathspec_magic[i].bit) {
- if (sb->buf[sb->len - 1] != '(')
- strbuf_addch(sb, ',');
- strbuf_addstr(sb, pathspec_magic[i].name);
+ /* No magic was found in element, just add prefix magic */
+ if (magic == 0) {
+ strbuf_addf(sb, ":(prefix:%d)", prefixlen);
+ return;
+ }
+
+ /*
+ * At this point we known that parse_element_magic() was able to extract some pathspec
+ * magic from element. So we know element is correctly formatted in either shorthand
+ * or longhand form
+ */
+ if (element[1] != '(') {
+ /* Process an element in shorthand form (e.g. ":!/<match>") */
+ strbuf_addstr(sb, ":(");
+ for (int i = 0; i < ARRAY_SIZE(pathspec_magic); i++) {
+ if ((magic & pathspec_magic[i].bit) && (pathspec_magic[i].mnemonic != '\0')) {
+ if (sb->buf[sb->len - 1] != '(')
+ strbuf_addch(sb, ',');
+ strbuf_addstr(sb, pathspec_magic[i].name);
+ }
}
+ } else {
+ /* For the longhand form, we copy everything up to the final ')' */
+ size_t len = strchr(element, ')') - element;
+ strbuf_add(sb, element, len);
+ }
strbuf_addf(sb, ",prefix:%d)", prefixlen);
}
@@ -493,7 +511,7 @@ static void init_pathspec_item(struct pathspec_item *item, unsigned flags,
struct strbuf sb = STRBUF_INIT;
/* Preserve the actual prefix length of each pattern */
- prefix_magic(&sb, prefixlen, element_magic);
+ prefix_magic(&sb, prefixlen, element_magic, elt);
strbuf_addstr(&sb, match);
item->original = strbuf_detach(&sb, NULL);
diff --git a/t/t6135-pathspec-with-attrs.sh b/t/t6135-pathspec-with-attrs.sh
index f70c395e75..e46fa176ed 100755
--- a/t/t6135-pathspec-with-attrs.sh
+++ b/t/t6135-pathspec-with-attrs.sh
@@ -64,12 +64,24 @@ test_expect_success 'setup .gitattributes' '
fileSetLabel label
fileValue label=foo
fileWrongLabel label☺
+ newFileA* labelA
+ newFileB* labelB
EOF
echo fileSetLabel label1 >sub/.gitattributes &&
git add .gitattributes sub/.gitattributes &&
git commit -m "add attributes"
'
+test_expect_success 'setup .gitignore' '
+ cat <<-\EOF >.gitignore &&
+ actual
+ expect
+ pathspec_file
+ EOF
+ git add .gitignore &&
+ git commit -m "add gitignore"
+'
+
test_expect_success 'check specific set attr' '
cat <<-\EOF >expect &&
fileSetLabel
@@ -150,6 +162,7 @@ test_expect_success 'check specific value attr (2)' '
test_expect_success 'check unspecified attr' '
cat <<-\EOF >expect &&
.gitattributes
+ .gitignore
fileA
fileAB
fileAC
@@ -175,6 +188,7 @@ test_expect_success 'check unspecified attr' '
test_expect_success 'check unspecified attr (2)' '
cat <<-\EOF >expect &&
HEAD:.gitattributes
+ HEAD:.gitignore
HEAD:fileA
HEAD:fileAB
HEAD:fileAC
@@ -200,6 +214,7 @@ test_expect_success 'check unspecified attr (2)' '
test_expect_success 'check multiple unspecified attr' '
cat <<-\EOF >expect &&
.gitattributes
+ .gitignore
fileC
fileNoLabel
fileWrongLabel
@@ -239,16 +254,99 @@ test_expect_success 'fail on multiple attr specifiers in one pathspec item' '
test_i18ngrep "Only one" actual
'
-test_expect_success 'fail if attr magic is used places not implemented' '
+test_expect_success 'fail if attr magic is used in places not implemented' '
# The main purpose of this test is to check that we actually fail
# when you attempt to use attr magic in commands that do not implement
- # attr magic. This test does not advocate git-add to stay that way,
- # though, but git-add is convenient as it has its own internal pathspec
- # parsing.
- test_must_fail git add ":(attr:labelB)" 2>actual &&
+ # attr magic. This test does not advocate check-ignore to stay that way.
+ # When you teach the command to grok the pathspec, you need to find
+ # another command to replace it for the test.
+ test_must_fail git check-ignore ":(attr:labelB)" 2>actual &&
test_i18ngrep "magic not supported" actual
'
+test_expect_success 'check that attr magic works for git stash push' '
+ cat <<-\EOF >expect &&
+ A sub/newFileA-foo
+ EOF
+ >sub/newFileA-foo &&
+ >sub/newFileB-foo &&
+ git stash push --include-untracked -- ":(exclude,attr:labelB)" &&
+ git stash show --include-untracked --name-status >actual &&
+ test_cmp expect actual
+'
+
+test_expect_success 'check that attr magic works for git add --all' '
+ cat <<-\EOF >expect &&
+ sub/newFileA-foo
+ EOF
+ >sub/newFileA-foo &&
+ >sub/newFileB-foo &&
+ git add --all ":(exclude,attr:labelB)" &&
+ git diff --name-only --cached >actual &&
+ git restore -W -S . &&
+ test_cmp expect actual
+'
+
+test_expect_success 'check that attr magic works for git add -u' '
+ cat <<-\EOF >expect &&
+ sub/fileA
+ EOF
+ >sub/newFileA-foo &&
+ >sub/newFileB-foo &&
+ >sub/fileA &&
+ >sub/fileB &&
+ git add -u ":(exclude,attr:labelB)" &&
+ git diff --name-only --cached >actual &&
+ git restore -S -W . && rm sub/new* &&
+ test_cmp expect actual
+'
+
+test_expect_success 'check that attr magic works for git add <path>' '
+ cat <<-\EOF >expect &&
+ fileA
+ fileB
+ sub/fileA
+ EOF
+ >fileA &&
+ >fileB &&
+ >sub/fileA &&
+ >sub/fileB &&
+ git add ":(exclude,attr:labelB)sub/*" &&
+ git diff --name-only --cached >actual &&
+ git restore -S -W . &&
+ test_cmp expect actual
+'
+
+test_expect_success 'check that attr magic works for git -add .' '
+ cat <<-\EOF >expect &&
+ sub/fileA
+ EOF
+ >fileA &&
+ >fileB &&
+ >sub/fileA &&
+ >sub/fileB &&
+ cd sub &&
+ git add . ":(exclude,attr:labelB)" &&
+ cd .. &&
+ git diff --name-only --cached >actual &&
+ git restore -S -W . &&
+ test_cmp expect actual
+'
+
+test_expect_success 'check that attr magic works for git add --pathspec-from-file' '
+ cat <<-\EOF >pathspec_file &&
+ :(exclude,attr:labelB)
+ EOF
+ cat <<-\EOF >expect &&
+ sub/newFileA-foo
+ EOF
+ >sub/newFileA-foo &&
+ >sub/newFileB-foo &&
+ git add --all --pathspec-from-file=pathspec_file &&
+ git diff --name-only --cached >actual &&
+ test_cmp expect actual
+'
+
test_expect_success 'abort on giving invalid label on the command line' '
test_must_fail git ls-files . ":(attr:☺)"
'
--
2.42.0.869.gea05f2083d-goog
^ permalink raw reply related
* Re: [BUG] Git 2.43.0-rc0 - t4216 unpack(Q) invalid type
From: Jeff King @ 2023-11-03 16:21 UTC (permalink / raw)
To: rsbecker; +Cc: git
In-Reply-To: <007f01da0e6f$d5031a40$7f094ec0$@nexbridge.com>
On Fri, Nov 03, 2023 at 12:07:17PM -0400, rsbecker@nexbridge.com wrote:
> What I get from Perl is
> $ perl -e '
> > my $bytes = "\1\2\3\4\5\6\7\8";
> > my $q = eval { unpack("Q>", $bytes) };
> > print "Q = ", defined($q) ? $q : "($@)", "\n";
> > my ($n1, $n2) = unpack("NN", $bytes);
> > print "n1 = $n1\n";
> > print "n2 = $n2\n";
> > print "computed quad = ", ($n1 << 32) | $n2, "\n"; '
> Q = (Invalid type 'Q' in unpack at -e line 3.
> )
> n1 = 16909060
> n2 = 84281144
> computed quad = 84281144
OK, that matches what I expected. Hopefully the patch I just sent (our
mails just crossed) will fix it for you.
> Because perl itself is 32-bit, not 64-bit on this platform. So even
> moving git to 64-bit will not correct the issue.
Yep, exactly.
-Peff
^ permalink raw reply
* [PATCH] t: avoid perl's pack/unpack "Q" specifier
From: Jeff King @ 2023-11-03 16:20 UTC (permalink / raw)
To: rsbecker; +Cc: git
In-Reply-To: <007c01da0e6f$014e2c30$03ea8490$@nexbridge.com>
On Fri, Nov 03, 2023 at 12:01:22PM -0400, rsbecker@nexbridge.com wrote:
> Explains a lot. We are only able to build git in 32-bit mode because
> of OS dependencies (only available in 32-bit). I did not know that
> 64-bit was now required for git. We will get there, but it will
> probably take years.
It's not required for Git. And even on 32-bit systems there is usually
some compiler support for 64-bit types via "uint64_t", etc. And likewise
perl can probably be built for 64-bit types even on a 32-bit system (the
tests do pass on our Linux 32-bit build), but your perl simply wasn't.
I think we can accommodate it, though. Did you try the snippet I showed?
If it behaves as I think it does, then this patch should hopefully fix
things for you:
-- >8 --
Subject: [PATCH] t: avoid perl's pack/unpack "Q" specifier
The perl script introduced by 86b008ee61 (t: add library for munging
chunk-format files, 2023-10-09) uses pack("Q") and unpack("Q") to read
and write 64-bit values ("quadwords" in perl parlance) from the on-disk
chunk files. However, some builds of perl may not support 64-bit
integers at all, and throw an exception here. While some 32-bit
platforms may still support 64-bit integers in perl (such as our linux32
CI environment), others reportedly don't (the NonStop 32-bit builds).
We can work around this by treating the 64-bit values as two 32-bit
values. We can't ever combine them into a single 64-bit value, but in
practice this is OK. These are representing file offsets, and our files
are much smaller than 4GB. So the upper half of the 64-bit value will
always be 0.
We can just introduce a few helper functions which perform the
translation and double-check our assumptions.
Reported-by: Randall S. Becker <randall.becker@nexbridge.ca>
Signed-off-by: Jeff King <peff@peff.net>
---
If this does work, it should go on top of jk/chunk-bounds.
t/lib-chunk/corrupt-chunk-file.pl | 30 +++++++++++++++++++++++++++---
1 file changed, 27 insertions(+), 3 deletions(-)
diff --git a/t/lib-chunk/corrupt-chunk-file.pl b/t/lib-chunk/corrupt-chunk-file.pl
index cd6d386fef..b024bbdcb5 100644
--- a/t/lib-chunk/corrupt-chunk-file.pl
+++ b/t/lib-chunk/corrupt-chunk-file.pl
@@ -21,14 +21,38 @@ sub copy {
return $buf;
}
+# Some platforms' perl builds don't support 64-bit integers, and hence do not
+# allow packing/unpacking quadwords with "Q". The chunk format uses 64-bit file
+# offsets to support files of any size, but in practice our test suite will
+# only use small files. So we can fake it by asking for two 32-bit values and
+# discarding the first (most significant) one, which is equivalent as long as
+# it's just zero.
+sub unpack_quad {
+ my $bytes = shift;
+ my ($n1, $n2) = unpack("NN", $bytes);
+ die "quad value exceeds 32 bits" if $n1;
+ return $n2;
+};
+sub pack_quad {
+ my $n = shift;
+ my $ret = pack("NN", 0, $n);
+ # double check that our original $n did not exceed the 32-bit limit.
+ # This is presumably impossible on a 32-bit system (which would have
+ # truncated much earlier), but would still alert us on a 64-bit build
+ # of a new test that would fail on a 32-bit build (though we'd
+ # presumably see the die() from unpack_quad() in such a case).
+ die "quad round-trip failed" if unpack_quad($ret) != $n;
+ return $ret;
+}
+
# read until we find table-of-contents entry for chunk;
# note that we cheat a bit by assuming 4-byte alignment and
# that no ToC entry will accidentally look like a header.
#
# If we don't find the entry, copy() will hit EOF and exit
# (which should cause the caller to fail the test).
while (copy(4) ne $chunk) { }
-my $offset = unpack("Q>", copy(8));
+my $offset = unpack_quad(copy(8));
# In clear mode, our length will change. So figure out
# the length by comparing to the offset of the next chunk, and
@@ -38,11 +62,11 @@ sub copy {
my $id;
do {
$id = copy(4);
- my $next = unpack("Q>", get(8));
+ my $next = unpack_quad(get(8));
if (!defined $len) {
$len = $next - $offset;
}
- print pack("Q>", $next - $len + length($bytes));
+ print pack_quad($next - $len + length($bytes));
} while (unpack("N", $id));
}
--
2.42.0.1037.g1c5f731f9d
^ permalink raw reply related
* RE: [BUG] Git 2.43.0-rc0 - t4216 unpack(Q) invalid type
From: rsbecker @ 2023-11-03 16:07 UTC (permalink / raw)
To: 'Jeff King'; +Cc: git
In-Reply-To: <20231103155220.GA1470570@coredump.intra.peff.net>
On November 3, 2023, Jeff King <peff@peff.net> wrote:
>On Fri, Nov 03, 2023 at 10:50:19AM -0400, rsbecker@nexbridge.com wrote:
>
>> In RC0, the following tests are failing (with verbose). They look like
>> the same root cause. Unpack("Q>".... What version does git now require for perl?
>> I have v5.30.3 available, nothing more recent.
>
>The perl used in the test suite is supposed to be vanilla enough to support any
>ancient version. The perl5 Git import doesn't have version tags that go back that far,
>but the quadwords in pack/unpack go back at least to a commit from 1998.
>
>So I suspect this is not a version issue, but rather a build-time config one. The docs
>say:
>
> Q An unsigned quad value.
> (Quads are available only if your system supports 64-bit integer
> values _and_ if Perl has been compiled to support those. Raises
> an exception otherwise.)
>
>It would probably be possible to rewrite the use of "Q" here to grab two 32-bit
>values instead. But I'd guess that on your system it is not as simple as a shift-and-
>add to then treat them as a 64-bit value, since presumably the problem is that perl's
>ints are all strictly 32-bit.
>
>What does this script produce for you:
>
> perl -e '
> my $bytes = "\1\2\3\4\5\6\7\8";
> my $q = eval { unpack("Q>", $bytes) };
> print "Q = ", defined($q) ? $q : "($@)", "\n";
> my ($n1, $n2) = unpack("NN", $bytes);
> print "n1 = $n1\n";
> print "n2 = $n2\n";
> print "computed quad = ", ($n1 << 32) | $n2, "\n";
> '
>
>I get:
>
> Q = 72623859790382904
> n1 = 16909060
> n2 = 84281144
> computed quad = 72623859790382904
>
>but I'm guessing you get an exception report for Q, and that the computed quad is
>probably equal to n2 (the shift of n1 goes totally off the end).
>
>We may not be without hope, though. These 64-bit values are file offsets we're
>reading from the chunk files. The format naturally uses 64-bit values here to
>accommodate arbitrarily large files. But in our tests, the offsets are all going to be
>relatively small. So our "$n1" in practice will always be 0.
>
>> This same problem also happens in t5318, t5319, t5324
>
>Yep. The offending code is in lib-chunk.sh, so the new tests added in all of those
>scripts which use it will run into the same problem.
What I get from Perl is
$ perl -e '
> my $bytes = "\1\2\3\4\5\6\7\8";
> my $q = eval { unpack("Q>", $bytes) };
> print "Q = ", defined($q) ? $q : "($@)", "\n";
> my ($n1, $n2) = unpack("NN", $bytes);
> print "n1 = $n1\n";
> print "n2 = $n2\n";
> print "computed quad = ", ($n1 << 32) | $n2, "\n"; '
Q = (Invalid type 'Q' in unpack at -e line 3.
)
n1 = 16909060
n2 = 84281144
computed quad = 84281144
Because perl itself is 32-bit, not 64-bit on this platform. So even moving git to 64-bit will not correct the issue.
^ permalink raw reply
* RE: [BUG] Git 2.43.0-rc0 - t4216 unpack(Q) invalid type
From: rsbecker @ 2023-11-03 16:01 UTC (permalink / raw)
To: 'Jeff King'; +Cc: git
In-Reply-To: <20231103155220.GA1470570@coredump.intra.peff.net>
On Jeff King <peff@peff.net> wrote:
>On Fri, Nov 03, 2023 at 10:50:19AM -0400, rsbecker@nexbridge.com wrote:
>> In RC0, the following tests are failing (with verbose). They look like
>> the same root cause. Unpack("Q>".... What version does git now require for perl?
>> I have v5.30.3 available, nothing more recent.
>
>The perl used in the test suite is supposed to be vanilla enough to support any
>ancient version. The perl5 Git import doesn't have version tags that go back that far,
>but the quadwords in pack/unpack go back at least to a commit from 1998.
>
>So I suspect this is not a version issue, but rather a build-time config one. The docs
>say:
>
> Q An unsigned quad value.
> (Quads are available only if your system supports 64-bit integer
> values _and_ if Perl has been compiled to support those. Raises
> an exception otherwise.)
>
>It would probably be possible to rewrite the use of "Q" here to grab two 32-bit
>values instead. But I'd guess that on your system it is not as simple as a shift-and-
>add to then treat them as a 64-bit value, since presumably the problem is that perl's
>ints are all strictly 32-bit.
>
>What does this script produce for you:
>
> perl -e '
> my $bytes = "\1\2\3\4\5\6\7\8";
> my $q = eval { unpack("Q>", $bytes) };
> print "Q = ", defined($q) ? $q : "($@)", "\n";
> my ($n1, $n2) = unpack("NN", $bytes);
> print "n1 = $n1\n";
> print "n2 = $n2\n";
> print "computed quad = ", ($n1 << 32) | $n2, "\n";
> '
>
>I get:
>
> Q = 72623859790382904
> n1 = 16909060
> n2 = 84281144
> computed quad = 72623859790382904
>
>but I'm guessing you get an exception report for Q, and that the computed quad is
>probably equal to n2 (the shift of n1 goes totally off the end).
>
>We may not be without hope, though. These 64-bit values are file offsets we're
>reading from the chunk files. The format naturally uses 64-bit values here to
>accommodate arbitrarily large files. But in our tests, the offsets are all going to be
>relatively small. So our "$n1" in practice will always be 0.
>
>> This same problem also happens in t5318, t5319, t5324
>
>Yep. The offending code is in lib-chunk.sh, so the new tests added in all of those
>scripts which use it will run into the same problem.
Explains a lot. We are only able to build git in 32-bit mode because of OS dependencies (only available in 32-bit). I did not know that 64-bit was now required for git. We will get there, but it will probably take years.
^ permalink raw reply
* Re: [BUG] Git 2.43.0-rc0 - t4216 unpack(Q) invalid type
From: Jeff King @ 2023-11-03 15:52 UTC (permalink / raw)
To: rsbecker; +Cc: git
In-Reply-To: <007101da0e65$13e3c170$3bab4450$@nexbridge.com>
On Fri, Nov 03, 2023 at 10:50:19AM -0400, rsbecker@nexbridge.com wrote:
> In RC0, the following tests are failing (with verbose). They look like the
> same root cause. Unpack("Q>".... What version does git now require for perl?
> I have v5.30.3 available, nothing more recent.
The perl used in the test suite is supposed to be vanilla enough to
support any ancient version. The perl5 Git import doesn't have version
tags that go back that far, but the quadwords in pack/unpack go back at
least to a commit from 1998.
So I suspect this is not a version issue, but rather a build-time config
one. The docs say:
Q An unsigned quad value.
(Quads are available only if your system supports 64-bit integer
values _and_ if Perl has been compiled to support those. Raises
an exception otherwise.)
It would probably be possible to rewrite the use of "Q" here to grab two
32-bit values instead. But I'd guess that on your system it is not as
simple as a shift-and-add to then treat them as a 64-bit value, since
presumably the problem is that perl's ints are all strictly 32-bit.
What does this script produce for you:
perl -e '
my $bytes = "\1\2\3\4\5\6\7\8";
my $q = eval { unpack("Q>", $bytes) };
print "Q = ", defined($q) ? $q : "($@)", "\n";
my ($n1, $n2) = unpack("NN", $bytes);
print "n1 = $n1\n";
print "n2 = $n2\n";
print "computed quad = ", ($n1 << 32) | $n2, "\n";
'
I get:
Q = 72623859790382904
n1 = 16909060
n2 = 84281144
computed quad = 72623859790382904
but I'm guessing you get an exception report for Q, and that the
computed quad is probably equal to n2 (the shift of n1 goes totally off
the end).
We may not be without hope, though. These 64-bit values are file offsets
we're reading from the chunk files. The format naturally uses 64-bit
values here to accommodate arbitrarily large files. But in our tests,
the offsets are all going to be relatively small. So our "$n1" in
practice will always be 0.
> This same problem also happens in t5318, t5319, t5324
Yep. The offending code is in lib-chunk.sh, so the new tests added in
all of those scripts which use it will run into the same problem.
-Peff
^ permalink raw reply
* Re: [PATCH 1/1] attr: enable attr pathspec magic for git-add and git-stash
From: Ruben Safir @ 2023-11-03 15:31 UTC (permalink / raw)
To: Joanna Wang; +Cc: gitster, git
In-Reply-To: <20231103143508.896745-1-jojwang@google.com>
Is that really you name or is it a bad joke?
On Fri, Nov 03, 2023 at 02:35:07PM +0000, Joanna Wang wrote:
> This lets users limit files or exclude files based on file
> attributes during git-add and git-stash.
> For example, the chromium project would like to use this like
> "git add --all ':(exclude,attr:submodule)'", as submodules are managed in a
> unique way and often results in submodule changes that users do not want in
> their commits.
>
> This does not change any attr magic implementation. It is only adding
> attr as an allowed pathspec in git-add and git-stash, which was previously
> blocked by GUARD_PATHSPEC and a pathspec mask in parse_pathspec()).
> However, this does fix a bug in prefix_magic() where attr values were unintentionally removed.
> This was hit whenever parse_pathspec() is called with PATHSPEC_PREFIX_ORIGIN as a flag,
> which was the case for git-stash.
> (But originally filed here:
> https://lore.kernel.org/git/CAMmZTi-0QKtj7Q=sbC5qhipGsQxJFOY-Qkk1jfkRYwfF5FcUVg@mail.gmail.com/)
>
> Furthermore, while other commands hit this code path it did not result in unexpected
> behavior because this bug only impacts the pathspec->items->original field which is
> NOT used to filter paths. However, git-stash does use pathspec->items->original when
> building args used to call other git commands.
> (See add_pathspecs() usage and implementation in stash.c)
>
> It is possible that when the attr pathspec feature was first added in
> b0db704652 (pathspec: allow querying for attributes, 2017-03-13),
> "PATHSPEC_ATTR" was just unintentionally left out of a few GUARD_PATHSPEC() invocations.
>
> Later, to get a more user-friendly error message when attr was used with git-add,
> PATHSPEC_ATTR was added as a mask to git-add's invocation of parse_pathspec()
> 84d938b732 (add: do not accept pathspec magic 'attr', 2018-09-18).
> However, this user-friendly error message was never added for git-stash.
>
> Signed-off-by: Joanna Wang <jojwang@google.com>
>
> ---
>
> > At this point in the code, is it guaranteed that element[0] is ':'
> > and never a NUL? Also is it guaranteed that element has ')'
> > somewhere later if element[1] is '('?
> No sorry, we can only assume this if there was element magic detected
> by parse_element_magic(). So if element magic is not 0, we know the body
> is in the expected form (either long or short).
> I have added comments and a check for magic to guard against this.
>
> > I wonder if this existing bug caused by
> > failing to copy the value of "attr:" is triggerable from a codepath
> > that already allows PATHSPEC_ATTR magic.
> While there are other commands that go through the prefix_magic path (like `git-add -i`),
> AFAICT they are not actually impacted by the bug. The bug only impacts the value of
> pathspec->items->original which is not used during pathspec matching. But git-stash
> uses `original` to pass in as args to other commands it calls. I've included this
> detail in the description above.
>
> > but as the third parameter strbuf_add() takes is of type size_t, it would not
> > hurt to define "len" as the same type as well.
> Thanks for spotting. fixed.
>
> builtin/add.c | 7 ++-
> dir.c | 3 +-
> pathspec.c | 36 ++++++++---
> t/t6135-pathspec-with-attrs.sh | 108 +++++++++++++++++++++++++++++++--
> 4 files changed, 136 insertions(+), 18 deletions(-)
>
> diff --git a/builtin/add.c b/builtin/add.c
> index 5126d2ede3..d46e4d10e9 100644
> --- a/builtin/add.c
> +++ b/builtin/add.c
> @@ -424,7 +424,7 @@ int cmd_add(int argc, const char **argv, const char *prefix)
> * Check the "pathspec '%s' did not match any files" block
> * below before enabling new magic.
> */
> - parse_pathspec(&pathspec, PATHSPEC_ATTR,
> + parse_pathspec(&pathspec, 0,
> PATHSPEC_PREFER_FULL |
> PATHSPEC_SYMLINK_LEADING_PATH,
> prefix, argv);
> @@ -433,7 +433,7 @@ int cmd_add(int argc, const char **argv, const char *prefix)
> if (pathspec.nr)
> die(_("'%s' and pathspec arguments cannot be used together"), "--pathspec-from-file");
>
> - parse_pathspec_file(&pathspec, PATHSPEC_ATTR,
> + parse_pathspec_file(&pathspec, 0,
> PATHSPEC_PREFER_FULL |
> PATHSPEC_SYMLINK_LEADING_PATH,
> prefix, pathspec_from_file, pathspec_file_nul);
> @@ -504,7 +504,8 @@ int cmd_add(int argc, const char **argv, const char *prefix)
> PATHSPEC_LITERAL |
> PATHSPEC_GLOB |
> PATHSPEC_ICASE |
> - PATHSPEC_EXCLUDE);
> + PATHSPEC_EXCLUDE |
> + PATHSPEC_ATTR);
>
> for (i = 0; i < pathspec.nr; i++) {
> const char *path = pathspec.items[i].match;
> diff --git a/dir.c b/dir.c
> index 16fdb03f2a..4d1cd039be 100644
> --- a/dir.c
> +++ b/dir.c
> @@ -2179,7 +2179,8 @@ static int exclude_matches_pathspec(const char *path, int pathlen,
> PATHSPEC_LITERAL |
> PATHSPEC_GLOB |
> PATHSPEC_ICASE |
> - PATHSPEC_EXCLUDE);
> + PATHSPEC_EXCLUDE |
> + PATHSPEC_ATTR);
>
> for (i = 0; i < pathspec->nr; i++) {
> const struct pathspec_item *item = &pathspec->items[i];
> diff --git a/pathspec.c b/pathspec.c
> index bb1efe1f39..2f8721cc15 100644
> --- a/pathspec.c
> +++ b/pathspec.c
> @@ -109,16 +109,34 @@ static struct pathspec_magic {
> { PATHSPEC_ATTR, '\0', "attr" },
> };
>
> -static void prefix_magic(struct strbuf *sb, int prefixlen, unsigned magic)
> +static void prefix_magic(struct strbuf *sb, int prefixlen, unsigned magic, const char *element)
> {
> - int i;
> - strbuf_addstr(sb, ":(");
> - for (i = 0; i < ARRAY_SIZE(pathspec_magic); i++)
> - if (magic & pathspec_magic[i].bit) {
> - if (sb->buf[sb->len - 1] != '(')
> - strbuf_addch(sb, ',');
> - strbuf_addstr(sb, pathspec_magic[i].name);
> + /* No magic was found in element, just add prefix magic */
> + if (magic == 0) {
> + strbuf_addf(sb, ":(prefix:%d)", prefixlen);
> + return;
> + }
> +
> + /*
> + * At this point we known that parse_element_magic() was able to extract some pathspec
> + * magic from element. So we know element is correctly formatted in either shorthand
> + * or longhand form
> + */
> + if (element[1] != '(') {
> + /* Process an element in shorthand form (e.g. ":!/<match>") */
> + strbuf_addstr(sb, ":(");
> + for (int i = 0; i < ARRAY_SIZE(pathspec_magic); i++) {
> + if ((magic & pathspec_magic[i].bit) && (pathspec_magic[i].mnemonic != '\0')) {
> + if (sb->buf[sb->len - 1] != '(')
> + strbuf_addch(sb, ',');
> + strbuf_addstr(sb, pathspec_magic[i].name);
> + }
> }
> + } else {
> + /* For the longhand form, we copy everything up to the final ')' */
> + size_t len = strchr(element, ')') - element;
> + strbuf_add(sb, element, len);
> + }
> strbuf_addf(sb, ",prefix:%d)", prefixlen);
> }
>
> @@ -493,7 +511,7 @@ static void init_pathspec_item(struct pathspec_item *item, unsigned flags,
> struct strbuf sb = STRBUF_INIT;
>
> /* Preserve the actual prefix length of each pattern */
> - prefix_magic(&sb, prefixlen, element_magic);
> + prefix_magic(&sb, prefixlen, element_magic, elt);
>
> strbuf_addstr(&sb, match);
> item->original = strbuf_detach(&sb, NULL);
> diff --git a/t/t6135-pathspec-with-attrs.sh b/t/t6135-pathspec-with-attrs.sh
> index f70c395e75..e46fa176ed 100755
> --- a/t/t6135-pathspec-with-attrs.sh
> +++ b/t/t6135-pathspec-with-attrs.sh
> @@ -64,12 +64,24 @@ test_expect_success 'setup .gitattributes' '
> fileSetLabel label
> fileValue label=foo
> fileWrongLabel label☺
> + newFileA* labelA
> + newFileB* labelB
> EOF
> echo fileSetLabel label1 >sub/.gitattributes &&
> git add .gitattributes sub/.gitattributes &&
> git commit -m "add attributes"
> '
>
> +test_expect_success 'setup .gitignore' '
> + cat <<-\EOF >.gitignore &&
> + actual
> + expect
> + pathspec_file
> + EOF
> + git add .gitignore &&
> + git commit -m "add gitignore"
> +'
> +
> test_expect_success 'check specific set attr' '
> cat <<-\EOF >expect &&
> fileSetLabel
> @@ -150,6 +162,7 @@ test_expect_success 'check specific value attr (2)' '
> test_expect_success 'check unspecified attr' '
> cat <<-\EOF >expect &&
> .gitattributes
> + .gitignore
> fileA
> fileAB
> fileAC
> @@ -175,6 +188,7 @@ test_expect_success 'check unspecified attr' '
> test_expect_success 'check unspecified attr (2)' '
> cat <<-\EOF >expect &&
> HEAD:.gitattributes
> + HEAD:.gitignore
> HEAD:fileA
> HEAD:fileAB
> HEAD:fileAC
> @@ -200,6 +214,7 @@ test_expect_success 'check unspecified attr (2)' '
> test_expect_success 'check multiple unspecified attr' '
> cat <<-\EOF >expect &&
> .gitattributes
> + .gitignore
> fileC
> fileNoLabel
> fileWrongLabel
> @@ -239,16 +254,99 @@ test_expect_success 'fail on multiple attr specifiers in one pathspec item' '
> test_i18ngrep "Only one" actual
> '
>
> -test_expect_success 'fail if attr magic is used places not implemented' '
> +test_expect_success 'fail if attr magic is used in places not implemented' '
> # The main purpose of this test is to check that we actually fail
> # when you attempt to use attr magic in commands that do not implement
> - # attr magic. This test does not advocate git-add to stay that way,
> - # though, but git-add is convenient as it has its own internal pathspec
> - # parsing.
> - test_must_fail git add ":(attr:labelB)" 2>actual &&
> + # attr magic. This test does not advocate check-ignore to stay that way.
> + # When you teach the command to grok the pathspec, you need to find
> + # another command to replace it for the test.
> + test_must_fail git check-ignore ":(attr:labelB)" 2>actual &&
> test_i18ngrep "magic not supported" actual
> '
>
> +test_expect_success 'check that attr magic works for git stash push' '
> + cat <<-\EOF >expect &&
> + A sub/newFileA-foo
> + EOF
> + >sub/newFileA-foo &&
> + >sub/newFileB-foo &&
> + git stash push --include-untracked -- ":(exclude,attr:labelB)" &&
> + git stash show --include-untracked --name-status >actual &&
> + test_cmp expect actual
> +'
> +
> +test_expect_success 'check that attr magic works for git add --all' '
> + cat <<-\EOF >expect &&
> + sub/newFileA-foo
> + EOF
> + >sub/newFileA-foo &&
> + >sub/newFileB-foo &&
> + git add --all ":(exclude,attr:labelB)" &&
> + git diff --name-only --cached >actual &&
> + git restore -W -S . &&
> + test_cmp expect actual
> +'
> +
> +test_expect_success 'check that attr magic works for git add -u' '
> + cat <<-\EOF >expect &&
> + sub/fileA
> + EOF
> + >sub/newFileA-foo &&
> + >sub/newFileB-foo &&
> + >sub/fileA &&
> + >sub/fileB &&
> + git add -u ":(exclude,attr:labelB)" &&
> + git diff --name-only --cached >actual &&
> + git restore -S -W . && rm sub/new* &&
> + test_cmp expect actual
> +'
> +
> +test_expect_success 'check that attr magic works for git add <path>' '
> + cat <<-\EOF >expect &&
> + fileA
> + fileB
> + sub/fileA
> + EOF
> + >fileA &&
> + >fileB &&
> + >sub/fileA &&
> + >sub/fileB &&
> + git add ":(exclude,attr:labelB)sub/*" &&
> + git diff --name-only --cached >actual &&
> + git restore -S -W . &&
> + test_cmp expect actual
> +'
> +
> +test_expect_success 'check that attr magic works for git -add .' '
> + cat <<-\EOF >expect &&
> + sub/fileA
> + EOF
> + >fileA &&
> + >fileB &&
> + >sub/fileA &&
> + >sub/fileB &&
> + cd sub &&
> + git add . ":(exclude,attr:labelB)" &&
> + cd .. &&
> + git diff --name-only --cached >actual &&
> + git restore -S -W . &&
> + test_cmp expect actual
> +'
> +
> +test_expect_success 'check that attr magic works for git add --pathspec-from-file' '
> + cat <<-\EOF >pathspec_file &&
> + :(exclude,attr:labelB)
> + EOF
> + cat <<-\EOF >expect &&
> + sub/newFileA-foo
> + EOF
> + >sub/newFileA-foo &&
> + >sub/newFileB-foo &&
> + git add --all --pathspec-from-file=pathspec_file &&
> + git diff --name-only --cached >actual &&
> + test_cmp expect actual
> +'
> +
> test_expect_success 'abort on giving invalid label on the command line' '
> test_must_fail git ls-files . ":(attr:☺)"
> '
> --
> 2.42.0.869.gea05f2083d-goog
>
--
So many immigrant groups have swept through our town
that Brooklyn, like Atlantis, reaches mythological
proportions in the mind of the world - RI Safir 1998
http://www.mrbrklyn.com
DRM is THEFT - We are the STAKEHOLDERS - RI Safir 2002
http://www.nylxs.com - Leadership Development in Free Software
http://www2.mrbrklyn.com/resources - Unpublished Archive
http://www.coinhangout.com - coins!
http://www.brooklyn-living.com
Being so tracked is for FARM ANIMALS and extermination camps,
but incompatible with living as a free human being. -RI Safir 2013
^ permalink raw reply
* Re: why does git set X in LESS env var?
From: Andy Koppe @ 2023-11-03 15:28 UTC (permalink / raw)
To: Thomas Guyot
Cc: Dragan Simic, Junio C Hamano, Jeff King, Christoph Anton Mitterer,
git
In-Reply-To: <24c46f25-6b31-437d-9f89-1e8eb74136c8@gmail.com>
Thomas Guyot wrote:
> >> I actually have one major issue with it, it's that displaying anything
> >> less than a full page will fill the screen with ~ on the bottom, just
> >> like when scrolling up on a partial page without -F.
'less' has the '-~' (or --tilde) option to suppress that.
> I mentioned it slightly above, to be clear it would mean that:
>
> 1. less starts by just writing lined down as usual, making any lines
> above scroll up and overflow into the scrollback buffer as usual
> 2. If less draws less than a page, exits as before - the effective
> result is as if pager was cat
> 3. If less reaches a full page and still has lines to write, it turns on
> -c's behavior and further updates happen from the top of the screen,
> preventing scroll up (at least on rxvt-unicode)
>
> Now, if all other terms misbehave here, that's an issue, making this
> suggestion mostly useless. And considering the number of Windows users
> we absolutely need to test Windows Terminal, and should probably test
> MacOS's term too (whatever that is).
For what it's worth, the 'mintty' terminal used by default for Git for
Windows as well as MSYS and Cygwin has another approach to the whole
problem. Its rather flippantly named 'Flip Screen' context menu
command with Alt+F12 or Ctrl+Shift+S shortcut lets users temporarily
look at the alternate screen buffer while the main screen buffer is
active, and vice versa.
If 'less' is invoked without the -X option, it will switch to the
alternate screen, where mousewheel scrolling works by sending cursor
up/down keycodes. While in 'less', you can temporarily flip to the
main screen to look up something in the shell session there or copy
something for searching in 'less'. While looking at the main screen,
the mousewheel will scroll the scrollback buffer. Keyboard input
that's sent to 'less' will flip back to the alternate screen.
Quitting 'less' switches back to the main screen, so the 'less' output
disappears and you're back in the shell session with the command that
invoked 'less' as the last thing shown. But again, the 'Flip Screen'
command or shortcuts can be used to temporarily look at or copy from
the alternate screen, which will contain the last page displayed by
'less'. (The alternate screen does not have a scrollback buffer.)
The 'Flip Screen' feature of course also works with other
alternate-screen applications, for example editors.
Apparently the Mac terminal has such a feature as well:
https://support.apple.com/en-ie/guide/terminal/trmld1f46097/mac
(Full disclosure: I originally made mintty, from PuTTY.)
Kind regards,
Andy
^ permalink raw reply
* RE: [BUG] Git 2.43.0-rc0 - t4216 unpack(Q) invalid type
From: rsbecker @ 2023-11-03 15:01 UTC (permalink / raw)
To: git
In-Reply-To: <007101da0e65$13e3c170$3bab4450$@nexbridge.com>
On Friday, November 3, 2023 10:50 AM, I wrote:
>In RC0, the following tests are failing (with verbose). They look like the
same root
>cause. Unpack("Q>".... What version does git now require for perl?
>I have v5.30.3 available, nothing more recent.
>
>expecting success of 4216.141 'Bloom reader notices too-small data chunk':
>check_corrupt_graph BDAT clear 00000000 && echo "warning: ignoring
too-small
>changed-path chunk" \
>"(4 < 12) in commit-graph file" >expect.err && test_cmp expect.err err
>
>Invalid type 'Q' in unpack at
>/home/jenkinsbuild/.jenkins/workspace/Git_Pipeline/t/lib-chunk/corrupt-chun
k
>-file.pl line 31.
>not ok 141 - Bloom reader notices too-small data chunk #
#check_corrupt_graph
>BDAT clear 00000000 && #echo "warning: ignoring too-small changed-path
chunk"
>\
>#"(4 < 12) in commit-graph file" >expect.err && #test_cmp expect.err err #
>
>expecting success of 4216.142 'Bloom reader notices out-of-bounds filter
>offsets':
>check_corrupt_graph BIDX 12 FFFFFFFF &&
># use grep to avoid depending on exact chunk size grep "warning: ignoring
out-of-
>range offset (4294967295) for changed-path filter at pos 3 of
>.git/objects/info/commit-graph" err
>
>Invalid type 'Q' in unpack at
>/home/jenkinsbuild/.jenkins/workspace/Git_Pipeline/t/lib-chunk/corrupt-chun
k
>-file.pl line 31.
>not ok 142 - Bloom reader notices out-of-bounds filter offsets #
>#check_corrupt_graph BIDX 12 FFFFFFFF && ## use grep to avoid depending on
>exact chunk size #grep "warning: ignoring out-of-range offset (4294967295)
for
>changed-path filter at pos 3 of .git/objects/info/commit-graph" err #
>
>expecting success of 4216.143 'Bloom reader notices too-small index chunk':
># replace the index with a single entry, making most # lookups
out-of-bounds
>check_corrupt_graph BIDX clear 00000000 && echo "warning: commit-graph
>changed-path index chunk" \ "is too small" >expect.err && test_cmp
expect.err err
>
>Invalid type 'Q' in unpack at
>/home/jenkinsbuild/.jenkins/workspace/Git_Pipeline/t/lib-chunk/corrupt-chun
k
>-file.pl line 31.
>not ok 143 - Bloom reader notices too-small index chunk # ## replace the
index with
>a single entry, making most ## lookups out-of-bounds #check_corrupt_graph
BIDX
>clear 00000000 && #echo "warning: commit-graph changed-path index chunk" \
>#"is too small" >expect.err && #test_cmp expect.err err #
>
>expecting success of 4216.144 'Bloom reader notices out-of-order index
>offsets':
># we do not know any real offsets, but we can pick # something plausible;
we
>should not get to the point of # actually reading from the bogus offsets
anyway.
>corrupt_graph BIDX 4 0000000c00000005 && echo "warning: ignoring decreasing
>changed-path index offsets" \
>"(12 > 5) for positions 1 and 2 of .git/objects/info/commit-graph"
>>expect.err &&
>git -c core.commitGraph=false log -- A/B/file2 >expect.out && git -c
>core.commitGraph=true log -- A/B/file2 >out 2>err && test_cmp expect.out
out &&
>test_cmp expect.err err
>
>Invalid type 'Q' in unpack at
>/home/jenkinsbuild/.jenkins/workspace/Git_Pipeline/t/lib-chunk/corrupt-chun
k
>-file.pl line 31.
>not ok 144 - Bloom reader notices out-of-order index offsets # ## we do not
know
>any real offsets, but we can pick ## something plausible; we should not get
to the
>point of ## actually reading from the bogus offsets anyway.
>#corrupt_graph BIDX 4 0000000c00000005 && #echo "warning: ignoring
>decreasing changed-path index offsets" \
>#"(12 > 5) for positions 1 and 2 of .git/objects/info/commit-graph"
>>expect.err &&
>#git -c core.commitGraph=false log -- A/B/file2 >expect.out && #git -c
>core.commitGraph=true log -- A/B/file2 >out 2>err && #test_cmp expect.out
out
>&& #test_cmp expect.err err #
>
This same problem also happens in t5318, t5319, t5324
--Randall
^ permalink raw reply
* Re: mintty bug in current git bash (Git-2.42.0.2-64-bit.exe)
From: Andy Koppe @ 2023-11-03 14:58 UTC (permalink / raw)
To: Rudisill, Steven L. (Booz Allen Hamilton); +Cc: git@vger.kernel.org
In-Reply-To: <SJ0PR09MB99693718DDA17BC15532A3099CA5A@SJ0PR09MB9969.namprd09.prod.outlook.com>
On Fri, 3 Nov 2023 at 14:11, Rudisill, Steven L. (Booz Allen Hamilton) wrote:
> Mintty has a bug - when double-clicking on text, the preceding '~' or '.' is not selected.
> https://github.com/mintty/mintty/issues/1237
>
> This really slows down interacting in git bash because this is a very frequent console interaction:
> Double-clicking to select a path (which copies it to the paste buffer since the text is highlighted), then easily paste to manipulate text.
> The pasted text is omitting the preceding '~' or '.' character.
>
> Please release a new git bash version that uses the updated mintty with the fix for this bug. The issue link above indicates that this is a bug and has been fixed.
This is fixed in mintty 3.6.5, which is part of the Git for Windows
2.43.0 release candidate announced earlier today:
https://lore.kernel.org/git/20231103085526.3655-1-johannes.schindelin@gmx.de
Regards,
Andy
^ permalink raw reply
* [BUG] Git 2.43.0-rc0 - t4216 unpack(Q) invalid type
From: rsbecker @ 2023-11-03 14:50 UTC (permalink / raw)
To: git
In RC0, the following tests are failing (with verbose). They look like the
same root cause. Unpack("Q>".... What version does git now require for perl?
I have v5.30.3 available, nothing more recent.
expecting success of 4216.141 'Bloom reader notices too-small data chunk':
check_corrupt_graph BDAT clear 00000000 &&
echo "warning: ignoring too-small changed-path chunk" \
"(4 < 12) in commit-graph file" >expect.err &&
test_cmp expect.err err
Invalid type 'Q' in unpack at
/home/jenkinsbuild/.jenkins/workspace/Git_Pipeline/t/lib-chunk/corrupt-chunk
-file.pl line 31.
not ok 141 - Bloom reader notices too-small data chunk
#
#check_corrupt_graph BDAT clear 00000000 &&
#echo "warning: ignoring too-small changed-path chunk" \
#"(4 < 12) in commit-graph file" >expect.err &&
#test_cmp expect.err err
#
expecting success of 4216.142 'Bloom reader notices out-of-bounds filter
offsets':
check_corrupt_graph BIDX 12 FFFFFFFF &&
# use grep to avoid depending on exact chunk size
grep "warning: ignoring out-of-range offset (4294967295) for changed-path
filter at pos 3 of .git/objects/info/commit-graph" err
Invalid type 'Q' in unpack at
/home/jenkinsbuild/.jenkins/workspace/Git_Pipeline/t/lib-chunk/corrupt-chunk
-file.pl line 31.
not ok 142 - Bloom reader notices out-of-bounds filter offsets
#
#check_corrupt_graph BIDX 12 FFFFFFFF &&
## use grep to avoid depending on exact chunk size
#grep "warning: ignoring out-of-range offset (4294967295) for changed-path
filter at pos 3 of .git/objects/info/commit-graph" err
#
expecting success of 4216.143 'Bloom reader notices too-small index chunk':
# replace the index with a single entry, making most
# lookups out-of-bounds
check_corrupt_graph BIDX clear 00000000 &&
echo "warning: commit-graph changed-path index chunk" \
"is too small" >expect.err &&
test_cmp expect.err err
Invalid type 'Q' in unpack at
/home/jenkinsbuild/.jenkins/workspace/Git_Pipeline/t/lib-chunk/corrupt-chunk
-file.pl line 31.
not ok 143 - Bloom reader notices too-small index chunk
#
## replace the index with a single entry, making most
## lookups out-of-bounds
#check_corrupt_graph BIDX clear 00000000 &&
#echo "warning: commit-graph changed-path index chunk" \
#"is too small" >expect.err &&
#test_cmp expect.err err
#
expecting success of 4216.144 'Bloom reader notices out-of-order index
offsets':
# we do not know any real offsets, but we can pick
# something plausible; we should not get to the point of
# actually reading from the bogus offsets anyway.
corrupt_graph BIDX 4 0000000c00000005 &&
echo "warning: ignoring decreasing changed-path index offsets" \
"(12 > 5) for positions 1 and 2 of .git/objects/info/commit-graph"
>expect.err &&
git -c core.commitGraph=false log -- A/B/file2 >expect.out &&
git -c core.commitGraph=true log -- A/B/file2 >out 2>err &&
test_cmp expect.out out &&
test_cmp expect.err err
Invalid type 'Q' in unpack at
/home/jenkinsbuild/.jenkins/workspace/Git_Pipeline/t/lib-chunk/corrupt-chunk
-file.pl line 31.
not ok 144 - Bloom reader notices out-of-order index offsets
#
## we do not know any real offsets, but we can pick
## something plausible; we should not get to the point of
## actually reading from the bogus offsets anyway.
#corrupt_graph BIDX 4 0000000c00000005 &&
#echo "warning: ignoring decreasing changed-path index offsets" \
#"(12 > 5) for positions 1 and 2 of .git/objects/info/commit-graph"
>expect.err &&
#git -c core.commitGraph=false log -- A/B/file2 >expect.out &&
#git -c core.commitGraph=true log -- A/B/file2 >out 2>err &&
#test_cmp expect.out out &&
#test_cmp expect.err err
#
--
Brief whoami: NonStop&UNIX developer since approximately
UNIX(421664400)
NonStop(211288444200000000)
-- In real life, I talk too much.
^ 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