* Re: [PATCH 2/2] pretty: add '%aA' to show domain-part of email addresses
From: Andy Koppe @ 2023-11-04 9:51 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Kousik Sanagavarapu, Liam Beguin, git
In-Reply-To: <xmqqy1fegu5i.fsf@gitster.g>
On 04/11/2023 01:54, Junio C Hamano wrote:
> Andy Koppe <andy.koppe@gmail.com> writes:
>
>> 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.
>
> We could expand "%am" to \C-h (\010) so that "%al@%am" would end up
> displaying the same as "%al" but that would be way too cute for its
> own worth ;-)
:)
Unfortunately it also wouldn't always work, because ^H only moves the
cursor, so if the next thing is a newline, the '@' wouldn't actually get
deleted.
> It is unfortunate that "%al@%am" cannot be the same as "%ae" for
> local-only address, but giving an empty string for "%am" if "%ae" is
> local-only would be the best we could do for our users, and certainly
> much better than giving the same as "%ae", as you said above.
I suppose "%@am" could mean prepending an '@' when a domain is present,
similar to how "% am" would mean prepending a space and "%+am" would
mean prepending a newline. With that, "%al%@am" would be equivalent to
"%ae".
But that then raises the question whether it should be implemented just
for "%@[ac][mM]", or for all placeholders. In any case, I don't think it
needs to be part of the changes at hand.
Andy
^ permalink raw reply
* Re: Request for Help - Too many perl arguments as of 2.43.0-rc0
From: Junio C Hamano @ 2023-11-04 7:59 UTC (permalink / raw)
To: Eric Sunshine; +Cc: rsbecker, Git List
In-Reply-To: <CAPig+cQpxRQnhffR8EWWRhqJPmOeOuCE3qsuMcnDyuMLLbTH8Q@mail.gmail.com>
Eric Sunshine <sunshine@sunshineco.com> writes:
> Also, in the longer term, as you suggested, `xargs` is likely a more
> fruitful solution.
Hmph, the list of our test scripts exceed command line limit? That
sounds a bit nasty, as we somehow need to prepare a pipe and feed
them into it, in order to drive xargs downstream of the pipe.
Ideally if there were a GNUMake function that slices a list into
sublists of "reasonable" lengths, we could use it to directly drive
N invocations of check-non-portable-shell script instead of xargs,
but I didn't find one. Here is I came up with, using foreach that
is "slice the list into many sublists of 1 element", but it made me
feel dirty.
t/Makefile | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git c/t/Makefile w/t/Makefile
index 3e00cdd801..f9ac6bb4eb 100644
--- c/t/Makefile
+++ w/t/Makefile
@@ -122,7 +122,8 @@ test-lint-executable:
echo >&2 "non-executable tests:" $$bad; exit 1; }
test-lint-shell-syntax:
- @'$(PERL_PATH_SQ)' check-non-portable-shell.pl $(T) $(THELPERS) $(TPERF)
+ @{ $(foreach t,$(T) $(THELPERS) $(TPERF),echo "$t";) } | \
+ xargs '$(PERL_PATH_SQ)' check-non-portable-shell.pl
test-lint-filenames:
@# We do *not* pass a glob to ls-files but use grep instead, to catch
^ permalink raw reply related
* Re: [PATCH 1/1] attr: enable attr pathspec magic for git-add and git-stash
From: Junio C Hamano @ 2023-11-04 7:14 UTC (permalink / raw)
To: Eric Sunshine; +Cc: Joanna Wang, git
In-Reply-To: <CAPig+cRR0PfWP0_uoevQnctSosb-k9cp3b08-sQNV5OPuQ2tAg@mail.gmail.com>
Eric Sunshine <sunshine@sunshineco.com> writes:
> On Sat, Nov 4, 2023 at 1:12 AM Junio C Hamano <gitster@pobox.com> wrote:
>> > + if ((magic & pathspec_magic[i].bit) && (pathspec_magic[i].mnemonic != '\0')) {
>>
>> Overly long, with a stylo.
>>
>> Here is what I ended up queuing.
>>
>> + for (int i = 0; i < ARRAY_SIZE(pathspec_magic); i++) {
>> + if ((magic & pathspec_magic[i].bit) &&
>> + (pathspec_magic[i].mnemonic)) {
>
> Nit: The no-value-added parentheses around
> `pathspec_magic[i].mnemonic` can also be dropped.
Yeah, thanks for spotting.
^ permalink raw reply
* Re: [RFC PATCH] status: avoid reporting worktrees as "Untracked files"
From: Junio C Hamano @ 2023-11-04 6:58 UTC (permalink / raw)
To: Edmundo Carmona Antoranz; +Cc: git
In-Reply-To: <20231104000209.916189-1-eantoranz@gmail.com>
Edmundo Carmona Antoranz <eantoranz@gmail.com> writes:
> Given that worktrees are tracked in their own special fashion separately,
> it makes sense to _not_ report them as "untracked".
My gut feeling is that a much better solution to the unstated
problem you are solving is to make sure that "git worktree add" will
complain and not allow adding a subdirectory of any existing working
tree of a repository as a new worktree.
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.
^ permalink raw reply
* Re: Request for Help - Too many perl arguments as of 2.43.0-rc0
From: Eric Sunshine @ 2023-11-04 6:36 UTC (permalink / raw)
To: rsbecker; +Cc: Git List
In-Reply-To: <00c401da0ea1$b61899c0$2249cd40$@nexbridge.com>
On Fri, Nov 3, 2023 at 6:04 PM <rsbecker@nexbridge.com> wrote:
> On November 3, 2023 5:51 PM, Eric Sunshine wrote:
> >On Fri, Nov 3, 2023 at 5:31 PM <rsbecker@nexbridge.com> wrote:
> >> /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.
I don't see an urgent need for it. Unlike the actual tests themselves
run by `make test` which may catch platform-specific problems in Git
itself, the purpose of the "linting" checks is not to catch
platform-specific problems, but rather to help test authors by
identifying mistakes in the tests which might make them fragile. So,
disabling linting on a particular platform isn't going to cause `make
test` to miss some important Git problem specific to that platform.
Also, in the longer term, as you suggested, `xargs` is likely a more
fruitful solution.
^ permalink raw reply
* Re: [PATCH 1/1] attr: enable attr pathspec magic for git-add and git-stash
From: Eric Sunshine @ 2023-11-04 6:28 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Joanna Wang, git
In-Reply-To: <xmqqsf5mgl0r.fsf@gitster.g>
On Sat, Nov 4, 2023 at 1:12 AM Junio C Hamano <gitster@pobox.com> wrote:
> > + if ((magic & pathspec_magic[i].bit) && (pathspec_magic[i].mnemonic != '\0')) {
>
> Overly long, with a stylo.
>
> Here is what I ended up queuing.
>
> + for (int i = 0; i < ARRAY_SIZE(pathspec_magic); i++) {
> + if ((magic & pathspec_magic[i].bit) &&
> + (pathspec_magic[i].mnemonic)) {
Nit: The no-value-added parentheses around
`pathspec_magic[i].mnemonic` can also be dropped.
^ permalink raw reply
* Re: [RFC PATCH] status: avoid reporting worktrees as "Untracked files"
From: Eric Sunshine @ 2023-11-04 6:15 UTC (permalink / raw)
To: Edmundo Carmona Antoranz; +Cc: git
In-Reply-To: <20231104000209.916189-1-eantoranz@gmail.com>
On Fri, Nov 3, 2023 at 8:03 PM Edmundo Carmona Antoranz
<eantoranz@gmail.com> wrote:
> Given that worktrees are tracked in their own special fashion separately,
> it makes sense to _not_ report them as "untracked". Also, when seeing the
> directory of a worktree listed as Untracked, it might be tempting to try
> to do operations (like 'git add') on them from the parent worktree which,
> at the moment, will silently do nothing.
>
> With this patch, we check items against the list of worktrees to add
> them into the untracked items list effectively hiding them.
>
> END OF PATCH
>
> Here are a few questions more inline with the "RFC" part of the patch.
>
> About UI
> - Would it make more sense to separate them from Untracked files instead
> of hiding them (perhaps add a --worktrees option to display them)?
> - Follow-up if the previous answer is 'yes': List a worktree only if it
> is not clean?
>
> About code:
> - If keeping the idea/patch, Would it make more sense (performance-wise) to
> fist check an item in the list of worktrees before checking it in the
> index? In other words, reverse the conditions to add an item to the
> untracked list?
I have slightly mixed feelings about this idea since I'm sympathetic
to the motivation, however, my knee-jerk reaction is that these really
_are_ untracked considering that Git is a "content tracker" and
worktrees are not project content. Git already has general mechanisms
such as .git/info/exclude and .gitignore for suppressing certain
untracked items, so introducing special-purpose code to suppress
worktrees from being considered untracked may be a case of adding
complexity for little gain.
Moreover, although your personal workflow may be to create worktrees
within your main directory:
git worktree add new-feature
other people use a workflow in which worktrees are created at other
locations, such as making them siblings:
git worktree add ../new-feature
For the former case, it's easy enough to mention worktrees in
.git/info/exclude, especially if you use a standard naming convention
for your worktrees, in which case a single wildcard pattern may allow
you to set it once and forget about it. For the latter workflow, the
extra "is untracked" checking is simply wasteful.
Having said all that, I think that someone may have recently floated
the idea on the mailing list about suppressing dirty submodules from
showing up as "dirty" in git-status. Although the underlying concepts
and mechanisms are quite distinct (especially since a submodule _is_
content), perhaps there is some sort of analogy between worktrees and
dirty submodules which invalidates my knee-jerk reaction. Also, I'm
just one person responding without having put all that much thought
into it. Others may feel differently.
Regarding the patch itself...
> diff --git a/wt-status.c b/wt-status.c
> @@ -795,9 +796,12 @@ static void wt_status_collect_untracked(struct wt_status *s)
> + worktrees = get_worktrees();
> +
> for (i = 0; i < dir.nr; i++) {
> struct dir_entry *ent = dir.entries[i];
> - if (index_name_is_other(istate, ent->name, ent->len))
> + if (index_name_is_other(istate, ent->name, ent->len) &&
> + !find_worktree_by_path(worktrees, ent->name))
> string_list_insert(&s->untracked, ent->name);
> }
This first-stab implementation unfortunately has worse than quadratic
complexity, perhaps even cubic complexity since
find_worktree_by_path() performs a linear scan through the worktree
list. So, for each path in `dir`, it's performing a
character-by-character string comparison with each path in
`worktrees`. Worse, find_worktree_by_path() calls strbuf_realpath()
which hits the filesystem for each path in `worktrees` each time it's
called.
So, a real (non-RFC) implementation would probably need to perform a
preparatory step of creating a hash-table/set in which the keys are
the realpath'd elements from `worktrees`, and then simply consult the
hash-table/set for each path in `dir`.
> @@ -809,6 +813,9 @@ static void wt_status_collect_untracked(struct wt_status *s)
> + if (worktrees)
> + free_worktrees(worktrees);
Nit: At this point, we _know_ that `worktrees` is non-NULL, so
free_worktrees() can be called unconditionally.
^ permalink raw reply
* Re: [PATCH 1/1] attr: enable attr pathspec magic for git-add and git-stash
From: Junio C Hamano @ 2023-11-04 5:11 UTC (permalink / raw)
To: Joanna Wang; +Cc: git
In-Reply-To: <20231103163449.1578841-1-jojwang@google.com>
Thanks. The compiled result from this version looks quite good. As
you started a new round with typofix, let me start the final
nitpicking.
Joanna Wang <jojwang@google.com> writes:
> 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.
I would say
Allow users to limit or exclude files based on file attributes
during git-add and git-stash.
For example, the chromium project would like to use
$ git add . ':(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.
except that I would prefer to see "in a unique way" rephrased.
Would "are managed outside Git using external tools" be a more
accurate description?
It may be true that the chromium project handles submodules unlike
all the other projects, but you guys being alone is not an important
part of the reason why we might want this new feature to help users.
When another project adopts the way how chromium manages the
submodules, forbidding its end-users from running "git add" to
record the changes to their submodules, you guys will no longer be
"unique", but the need for this feature will still exist (and the
demand may even be stronger). The reason why this feature helps
should be stated here.
> 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()).
Add blank line here between paragraphs.
> 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:
Wrap overlong lines here and in the rest of the proposed log message.
> 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)
> {
This is overly long.
> + /* No magic was found in element, just add prefix magic */
> + if (magic == 0) {
Style:
if (!magic) {
> + 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
> + */
Overly long, with a typo. "At this point, we know that ...".
> + 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')) {
Overly long, with a stylo.
> + 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);
> }
Here is what I ended up queuing.
Thanks.
---- >8 -------- >8 -------- >8 -------- >8 ----
From: Joanna Wang <jojwang@google.com>
Date: Fri, 3 Nov 2023 16:34:48 +0000
Subject: [PATCH] attr: enable attr pathspec magic for git-add and git-stash
Allow users to limit or exclude files based on file attributes
during git-add and git-stash.
For example, the chromium project would like to use
$ git add . ':(exclude,attr:submodule)'
as submodules are managed by an external tool, forbidding end users
to record changes with "git add". Allowing "git add" to often
records changes that users do not want in their commits.
This commit 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, we fix a bug in prefix_magic() where attr values were
unintentionally removed. This was triggerable when parse_pathspec()
is called with PATHSPEC_PREFIX_ORIGIN as a flag, which was the case
for git-stash (Bug originally filed here [*])
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.
[Reference]
* https://lore.kernel.org/git/CAMmZTi-0QKtj7Q=sbC5qhipGsQxJFOY-Qkk1jfkRYwfF5FcUVg@mail.gmail.com/)
Signed-off-by: Joanna Wang <jojwang@google.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
builtin/add.c | 7 ++-
dir.c | 3 +-
pathspec.c | 39 +++++++++---
t/t6135-pathspec-with-attrs.sh | 108 +++++++++++++++++++++++++++++++--
4 files changed, 139 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..d132eefac2 100644
--- a/pathspec.c
+++ b/pathspec.c
@@ -109,16 +109,37 @@ 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) {
+ strbuf_addf(sb, ":(prefix:%d)", prefixlen);
+ return;
+ }
+
+ /*
+ * At this point, we know 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)) {
+ 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 +514,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.43.0-rc0
^ permalink raw reply related
* Re: [PATCH] t: avoid perl's pack/unpack "Q" specifier
From: Jeff King @ 2023-11-04 4:59 UTC (permalink / raw)
To: Junio C Hamano; +Cc: rsbecker, git
In-Reply-To: <xmqq5y2ii91p.fsf@gitster.g>
On Sat, Nov 04, 2023 at 10:47:30AM +0900, Junio C Hamano wrote:
> > +sub unpack_quad {
> > + my $bytes = shift;
> > + my ($n1, $n2) = unpack("NN", $bytes);
> > + die "quad value exceeds 32 bits" if $n1;
> > + return $n2;
> > +};
>
> Is this an unnecessary ';' at the end?
Oops, yes. I'm not sure how that snuck in there. (It is not breaking
anything, but if you were to remove it while applying, I would be very
happy).
-Peff
^ permalink raw reply
* Re: [PATCH 2/2] pretty: add '%aA' to show domain-part of email addresses
From: Junio C Hamano @ 2023-11-04 1:54 UTC (permalink / raw)
To: Andy Koppe; +Cc: Kousik Sanagavarapu, Liam Beguin, git
In-Reply-To: <4e3a5661-cba0-4849-8340-06d8d4094f48@gmail.com>
Andy Koppe <andy.koppe@gmail.com> writes:
> 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.
We could expand "%am" to \C-h (\010) so that "%al@%am" would end up
displaying the same as "%al" but that would be way too cute for its
own worth ;-)
It is unfortunate that "%al@%am" cannot be the same as "%ae" for
local-only address, but giving an empty string for "%am" if "%ae" is
local-only would be the best we could do for our users, and certainly
much better than giving the same as "%ae", as you said above.
Thanks.
^ permalink raw reply
* Re: [PATCH] t: avoid perl's pack/unpack "Q" specifier
From: Junio C Hamano @ 2023-11-04 1:47 UTC (permalink / raw)
To: Jeff King; +Cc: rsbecker, git
In-Reply-To: <20231103162019.GB1470570@coredump.intra.peff.net>
Jeff King <peff@peff.net> writes:
> +# 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;
> +};
Is this an unnecessary ';' at the end?
> +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;
> +}
Nice. Both sub are done carefully.
> # 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));
> }
^ permalink raw reply
* Re: [PATCH 1/2] rebase: support non-interactive autosquash
From: Junio C Hamano @ 2023-11-04 1:19 UTC (permalink / raw)
To: Andy Koppe; +Cc: git, newren
In-Reply-To: <20231103212958.18472-1-andy.koppe@gmail.com>
Andy Koppe <andy.koppe@gmail.com> writes:
> 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'.
I think a better and more extensible way to coax the new feature
into the configuration system is to arrange it more like so:
false - synonym for "".
true - synonym for "interactive".
anything else - comman separated list of rebase methods, e.g.,
"interactive,noninteractive"
possible rebase method names might include other
stuff like "apply" or "merge", but I haven't
thought it through, so take this part with a
grain of salt.
That way, the Boolean versions can be considered historical spelling
of a more general system where you can exactly tell when autosquash
takes place. When we add to a new variant on top of 'interactive'
and 'non-interactive' variants the current rebase has, we do not
know if it makes sense to allow it to also handle autosquash without
knowing how that new variant's behavior appears to the end user, so
'always' that blindly enables autosquash for any unforseen future
variants of 'rebase' is probably not what you want.
^ permalink raw reply
* [RFC PATCH] status: avoid reporting worktrees as "Untracked files"
From: Edmundo Carmona Antoranz @ 2023-11-04 0:02 UTC (permalink / raw)
To: git; +Cc: Edmundo Carmona Antoranz
Given that worktrees are tracked in their own special fashion separately,
it makes sense to _not_ report them as "untracked". Also, when seeing the
directory of a worktree listed as Untracked, it might be tempting to try
to do operations (like 'git add') on them from the parent worktree which,
at the moment, will silently do nothing.
With this patch, we check items against the list of worktrees to add
them into the untracked items list effectively hiding them.
END OF PATCH
Here are a few questions more inline with the "RFC" part of the patch.
About UI
- Would it make more sense to separate them from Untracked files instead
of hiding them (perhaps add a --worktrees option to display them)?
- Follow-up if the previous answer is 'yes': List a worktree only if it
is not clean?
About code:
- If keeping the idea/patch, Would it make more sense (performance-wise) to
fist check an item in the list of worktrees before checking it in the
index? In other words, reverse the conditions to add an item to the
untracked list?
---
wt-status.c | 9 ++++++++-
1 file changed, 8 insertions(+), 1 deletion(-)
diff --git a/wt-status.c b/wt-status.c
index 9f45bf6949..5fd1e6007a 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -775,6 +775,7 @@ static void wt_status_collect_untracked(struct wt_status *s)
struct dir_struct dir = DIR_INIT;
uint64_t t_begin = getnanotime();
struct index_state *istate = s->repo->index;
+ struct worktree **worktrees;
if (!s->show_untracked_files)
return;
@@ -795,9 +796,12 @@ static void wt_status_collect_untracked(struct wt_status *s)
fill_directory(&dir, istate, &s->pathspec);
+ worktrees = get_worktrees();
+
for (i = 0; i < dir.nr; i++) {
struct dir_entry *ent = dir.entries[i];
- if (index_name_is_other(istate, ent->name, ent->len))
+ if (index_name_is_other(istate, ent->name, ent->len) &&
+ !find_worktree_by_path(worktrees, ent->name))
string_list_insert(&s->untracked, ent->name);
}
@@ -809,6 +813,9 @@ static void wt_status_collect_untracked(struct wt_status *s)
dir_clear(&dir);
+ if (worktrees)
+ free_worktrees(worktrees);
+
if (advice_enabled(ADVICE_STATUS_U_OPTION))
s->untracked_in_ms = (getnanotime() - t_begin) / 1000000;
}
--
2.42.0
^ permalink raw reply related
* 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
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