* Re: [PATCH] ci: avoid running the test suite _twice_
From: Jeff King @ 2023-11-16 20:02 UTC (permalink / raw)
To: Josh Steadmon
Cc: Johannes Schindelin via GitGitGadget, Phillip Wood, git,
Johannes Schindelin
In-Reply-To: <ZVU4EVcj0MDrSNcG@google.com>
On Wed, Nov 15, 2023 at 01:28:49PM -0800, Josh Steadmon wrote:
> The first part is easy, but I don't see a good way to get both shell
> tests and unit tests executing under the same `prove` process. For shell
> tests, we pass `--exec '$(TEST_SHELL_PATH_SQ)'` to prove, meaning that
> we use the specified shell as an interpreter for the test files. That
> will not work for unit test executables.
Yes, it's unfortunate that you can't set the "exec" flag per-script
(especially because without --exec it will auto-detect the right thing,
but then of course it won't use TEST_SHELL_PATH). But we can intercept
and do it ourselves, like:
diff --git a/t/Makefile b/t/Makefile
index 225aaf78ed..0b7c028eea 100644
--- a/t/Makefile
+++ b/t/Makefile
@@ -61,7 +61,7 @@ failed:
test -z "$$failed" || $(MAKE) $$failed
prove: pre-clean check-chainlint $(TEST_LINT)
- @echo "*** prove ***"; $(CHAINLINTSUPPRESS) $(PROVE) --exec '$(TEST_SHELL_PATH_SQ)' $(GIT_PROVE_OPTS) $(T) :: $(GIT_TEST_OPTS)
+ @echo "*** prove ***"; TEST_SHELL_PATH='$(TEST_SHELL_PATH_SQ)' $(CHAINLINTSUPPRESS) $(PROVE) --exec ./run-test.sh $(GIT_PROVE_OPTS) $(T) $(UNIT_TESTS) :: $(GIT_TEST_OPTS)
$(MAKE) clean-except-prove-cache
$(T):
diff --git a/t/run-test.sh b/t/run-test.sh
new file mode 100755
index 0000000000..69944029c8
--- /dev/null
+++ b/t/run-test.sh
@@ -0,0 +1,10 @@
+#!/bin/sh
+
+case "$1" in
+*.sh)
+ exec ${TEST_SHELL_PATH:-/bin/sh} "$@"
+ ;;
+*)
+ exec "$@"
+ ;;
+esac
You can actually do this inside the prove script using their plugin
interface, but the necessary bits are somewhat arcane.
> We could bundle all the unit tests into a single shell script, but then
> we lose parallelization and add hoops to jump through to determine what
> breaks. Or we could autogenerate a corresponding shell script to run
> each individual unit test, but that seems gross. Of course, these are
> hypothetical concerns for now, since we only have a single unit test at
> the moment.
We can't just stick them all in a single script; there must be exactly
one "plan" line in the TAP output from a given source. I had imagined
just manually adding a thin wrapper for each ("t9970-unit-strbuf" or
something). But it would also be easy to autogenerate them while
compiling. (Although all of that is moot with the wrapper I showed
above).
> There's also the issue that the shell test arguments we pass on from
> prove would be shared with the unit tests. That's fine for now, as
> t-strbuf doesn't accept any runtime arguments, but it's possible that
> either the framework or individual unit tests might grow to need
> arguments, and it might not be convenient to stay compatible with the
> shell tests.
Sharing the options between the two seems like a benefit to me. I'd
think that "-v" and "-i" would be useful, at least. Options which don't
apply (e.g., "--root") could be quietly ignored.
-Peff
^ permalink raw reply related
* [PATCH] send-email: remove stray characters from usage
From: Todd Zullinger @ 2023-11-16 19:36 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <xmqqzfzes37f.fsf@gitster.g>
A few stray single quotes crept into the usage string in a2ce608244
(send-email docs: add format-patch options, 2021-10-25). Remove them.
Signed-off-by: Todd Zullinger <tmz@pobox.com>
---
[I trimmed the Cc: list]
Junio C Hamano wrote:
> Thanks. Let's split this out as a docfix patch and handle it
> separately.
Done. :)
git-send-email.perl | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/git-send-email.perl b/git-send-email.perl
index cacdbd6bb2..d24e981d61 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -28,8 +28,8 @@
sub usage {
print <<EOT;
-git send-email' [<options>] <file|directory>
-git send-email' [<options>] <format-patch options>
+git send-email [<options>] <file|directory>
+git send-email [<options>] <format-patch options>
git send-email --dump-aliases
Composing:
--
2.43.0.rc2
^ permalink raw reply related
* [PATCH v3 2/2] send-email: avoid duplicate specification warnings
From: Todd Zullinger @ 2023-11-16 19:30 UTC (permalink / raw)
To: Junio C Hamano
Cc: git, Jeff King, Ævar Arnfjörð Bjarmason,
Ondřej Pohořelský
In-Reply-To: <20231116193014.470420-1-tmz@pobox.com>
A warning is issued for options which are specified more than once
beginning with perl-Getopt-Long >= 2.55. In addition to causing users
to see warnings, this results in test failures which compare the output.
An example, from t9001-send-email.37:
| +++ diff -u expect actual
| --- expect 2023-11-14 10:38:23.854346488 +0000
| +++ actual 2023-11-14 10:38:23.848346466 +0000
| @@ -1,2 +1,7 @@
| +Duplicate specification "no-chain-reply-to" for option "no-chain-reply-to"
| +Duplicate specification "to-cover|to-cover!" for option "to-cover"
| +Duplicate specification "cc-cover|cc-cover!" for option "cc-cover"
| +Duplicate specification "no-thread" for option "no-thread"
| +Duplicate specification "no-to-cover" for option "no-to-cover"
| fatal: longline.patch:35 is longer than 998 characters
| warning: no patches were sent
| error: last command exited with $?=1
| not ok 37 - reject long lines
Remove the duplicate option specs. These are primarily the explicit
'--no-' prefix opts which were added in f471494303 (git-send-email.perl:
support no- prefix with older GetOptions, 2015-01-30). This was done
specifically to support perl-5.8.0 which includes Getopt::Long 2.32[1].
Getopt::Long 2.33 added support for the '--no-' prefix natively by
appending '!' to the option specification string, which was included in
perl-5.8.1 and is not present in perl-5.8.0. The previous commit bumped
the minimum supported Perl version to 5.8.1 so we no longer need to
provide the '--no-' variants for negatable options manually.
Teach `--git-completion-helper` to output the '--no-' options. They are
not included in the options hash and would otherwise be lost.
Signed-off-by: Todd Zullinger <tmz@pobox.com>
---
git-send-email.perl | 19 ++++++-------------
1 file changed, 6 insertions(+), 13 deletions(-)
diff --git a/git-send-email.perl b/git-send-email.perl
index d75a4a33dd..f214bd4521 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -119,13 +119,16 @@ sub completion_helper {
foreach my $key (keys %$original_opts) {
unless (exists $not_for_completion{$key}) {
- $key =~ s/!$//;
+ my $negatable = ($key =~ s/!$//);
if ($key =~ /[:=][si]$/) {
$key =~ s/[:=][si]$//;
push (@send_email_opts, "--$_=") foreach (split (/\|/, $key));
} else {
push (@send_email_opts, "--$_") foreach (split (/\|/, $key));
+ if ($negatable) {
+ push (@send_email_opts, "--no-$_") foreach (split (/\|/, $key));
+ }
}
}
}
@@ -491,7 +494,6 @@ sub config_regexp {
"bcc=s" => \@getopt_bcc,
"no-bcc" => \$no_bcc,
"chain-reply-to!" => \$chain_reply_to,
- "no-chain-reply-to" => sub {$chain_reply_to = 0},
"sendmail-cmd=s" => \$sendmail_cmd,
"smtp-server=s" => \$smtp_server,
"smtp-server-option=s" => \@smtp_server_options,
@@ -506,36 +508,27 @@ sub config_regexp {
"smtp-auth=s" => \$smtp_auth,
"no-smtp-auth" => sub {$smtp_auth = 'none'},
"annotate!" => \$annotate,
- "no-annotate" => sub {$annotate = 0},
"compose" => \$compose,
"quiet" => \$quiet,
"cc-cmd=s" => \$cc_cmd,
"header-cmd=s" => \$header_cmd,
"no-header-cmd" => \$no_header_cmd,
"suppress-from!" => \$suppress_from,
- "no-suppress-from" => sub {$suppress_from = 0},
"suppress-cc=s" => \@suppress_cc,
"signed-off-cc|signed-off-by-cc!" => \$signed_off_by_cc,
- "no-signed-off-cc|no-signed-off-by-cc" => sub {$signed_off_by_cc = 0},
- "cc-cover|cc-cover!" => \$cover_cc,
- "no-cc-cover" => sub {$cover_cc = 0},
- "to-cover|to-cover!" => \$cover_to,
- "no-to-cover" => sub {$cover_to = 0},
+ "cc-cover!" => \$cover_cc,
+ "to-cover!" => \$cover_to,
"confirm=s" => \$confirm,
"dry-run" => \$dry_run,
"envelope-sender=s" => \$envelope_sender,
"thread!" => \$thread,
- "no-thread" => sub {$thread = 0},
"validate!" => \$validate,
- "no-validate" => sub {$validate = 0},
"transfer-encoding=s" => \$target_xfer_encoding,
"format-patch!" => \$format_patch,
- "no-format-patch" => sub {$format_patch = 0},
"8bit-encoding=s" => \$auto_8bit_encoding,
"compose-encoding=s" => \$compose_encoding,
"force" => \$force,
"xmailer!" => \$use_xmailer,
- "no-xmailer" => sub {$use_xmailer = 0},
"batch-size=i" => \$batch_size,
"relogin-delay=i" => \$relogin_delay,
"git-completion-helper" => \$git_completion_helper,
--
2.43.0.rc2
^ permalink raw reply related
* [PATCH v3 1/2] perl: bump the required Perl version to 5.8.1 from 5.8.0
From: Todd Zullinger @ 2023-11-16 19:30 UTC (permalink / raw)
To: Junio C Hamano
Cc: git, Jeff King, Ævar Arnfjörð Bjarmason,
Ondřej Pohořelský
In-Reply-To: <20231116193014.470420-1-tmz@pobox.com>
The following commit will make use of a Getopt::Long feature which is
only present in Perl >= 5.8.1. Document that as the minimum version we
support.
Many of our Perl scripts will continue to run with 5.8.0 but this change
allows us to adjust them as needed without breaking any promises to our
users.
The Perl requirement was last changed in d48b284183 (perl: bump the
required Perl version to 5.8 from 5.6.[21], 2010-09-24). At that time,
5.8.0 was 8 years old. It is now over 21 years old.
Signed-off-by: Todd Zullinger <tmz@pobox.com>
---
I debated changing all the 'use 5.008;' lines here, as most don't
actually require a newer Perl, but the previous bump did the same.
I can see the merit in either direction.
Changing it allows future contributors to be confident in relying on
5.8.1 features.
Not changing it allows anyone stuck on 5.8.0 to continue using the perl
scripts which don't actually require 5.8.1.
Tangentially, the Perl docs for 'use' function recommend against the
5.008001 form[1]:
Specifying VERSION as a numeric argument of the form 5.024001 should
generally be avoided as older less readable syntax compared to
v5.24.1. Before perl 5.8.0 released in 2002 the more verbose numeric
form was the only supported syntax, which is why you might see it in
older code.
use v5.24.1; # compile time version check
use 5.24.1; # ditto
use 5.024_001; # ditto; older syntax compatible with perl 5.6
I'm not enough of a Perl coder to have a strong preference or desire to
push for such a change, but I thought it was worth mentioning in case
others wonder why we're using the 5.008001 form.
[1] https://perldoc.perl.org/functions/use#use-VERSION
Documentation/CodingGuidelines | 2 +-
INSTALL | 2 +-
contrib/diff-highlight/DiffHighlight.pm | 2 +-
contrib/mw-to-git/Git/Mediawiki.pm | 2 +-
git-archimport.perl | 2 +-
git-cvsexportcommit.perl | 2 +-
git-cvsimport.perl | 2 +-
git-cvsserver.perl | 2 +-
git-send-email.perl | 4 ++--
git-svn.perl | 2 +-
gitweb/INSTALL | 2 +-
gitweb/gitweb.perl | 2 +-
perl/Git.pm | 2 +-
perl/Git/I18N.pm | 2 +-
perl/Git/LoadCPAN.pm | 2 +-
perl/Git/LoadCPAN/Error.pm | 2 +-
perl/Git/LoadCPAN/Mail/Address.pm | 2 +-
perl/Git/Packet.pm | 2 +-
t/t0202/test.pl | 2 +-
t/t5562/invoke-with-content-length.pl | 2 +-
t/t9700/test.pl | 2 +-
t/test-terminal.perl | 2 +-
22 files changed, 23 insertions(+), 23 deletions(-)
diff --git a/Documentation/CodingGuidelines b/Documentation/CodingGuidelines
index 8d3a467c01..39b9b7260f 100644
--- a/Documentation/CodingGuidelines
+++ b/Documentation/CodingGuidelines
@@ -490,7 +490,7 @@ For Perl programs:
- Most of the C guidelines above apply.
- - We try to support Perl 5.8 and later ("use Perl 5.008").
+ - We try to support Perl 5.8.1 and later ("use Perl 5.008001").
- use strict and use warnings are strongly preferred.
diff --git a/INSTALL b/INSTALL
index 4b42288882..06f29a8ae7 100644
--- a/INSTALL
+++ b/INSTALL
@@ -119,7 +119,7 @@ Issues of note:
- A POSIX-compliant shell is required to run some scripts needed
for everyday use (e.g. "bisect", "request-pull").
- - "Perl" version 5.8 or later is needed to use some of the
+ - "Perl" version 5.8.1 or later is needed to use some of the
features (e.g. sending patches using "git send-email",
interacting with svn repositories with "git svn"). If you can
live without these, use NO_PERL. Note that recent releases of
diff --git a/contrib/diff-highlight/DiffHighlight.pm b/contrib/diff-highlight/DiffHighlight.pm
index 376f577737..636add6968 100644
--- a/contrib/diff-highlight/DiffHighlight.pm
+++ b/contrib/diff-highlight/DiffHighlight.pm
@@ -1,6 +1,6 @@
package DiffHighlight;
-use 5.008;
+use 5.008001;
use warnings FATAL => 'all';
use strict;
diff --git a/contrib/mw-to-git/Git/Mediawiki.pm b/contrib/mw-to-git/Git/Mediawiki.pm
index 917d9e2d32..ff7811225e 100644
--- a/contrib/mw-to-git/Git/Mediawiki.pm
+++ b/contrib/mw-to-git/Git/Mediawiki.pm
@@ -1,6 +1,6 @@
package Git::Mediawiki;
-use 5.008;
+use 5.008001;
use strict;
use POSIX;
use Git;
diff --git a/git-archimport.perl b/git-archimport.perl
index b7c173c345..f5a317b899 100755
--- a/git-archimport.perl
+++ b/git-archimport.perl
@@ -54,7 +54,7 @@ =head1 Devel Notes
=cut
-use 5.008;
+use 5.008001;
use strict;
use warnings;
use Getopt::Std;
diff --git a/git-cvsexportcommit.perl b/git-cvsexportcommit.perl
index 289d4bc684..1e03ba94d1 100755
--- a/git-cvsexportcommit.perl
+++ b/git-cvsexportcommit.perl
@@ -1,6 +1,6 @@
#!/usr/bin/perl
-use 5.008;
+use 5.008001;
use strict;
use warnings;
use Getopt::Std;
diff --git a/git-cvsimport.perl b/git-cvsimport.perl
index 7bf3c12d67..07ea3443f7 100755
--- a/git-cvsimport.perl
+++ b/git-cvsimport.perl
@@ -13,7 +13,7 @@
# The head revision is on branch "origin" by default.
# You can change that with the '-o' option.
-use 5.008;
+use 5.008001;
use strict;
use warnings;
use Getopt::Long;
diff --git a/git-cvsserver.perl b/git-cvsserver.perl
index 7b757360e2..124f598bdc 100755
--- a/git-cvsserver.perl
+++ b/git-cvsserver.perl
@@ -15,7 +15,7 @@
####
####
-use 5.008;
+use 5.008001;
use strict;
use warnings;
use bytes;
diff --git a/git-send-email.perl b/git-send-email.perl
index cacdbd6bb2..d75a4a33dd 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -16,7 +16,7 @@
# and second line is the subject of the message.
#
-use 5.008;
+use 5.008001;
use strict;
use warnings $ENV{GIT_PERL_FATAL_WARNINGS} ? qw(FATAL all) : ();
use Getopt::Long;
@@ -228,7 +228,7 @@ sub system_or_msg {
my @sprintf_args = ($cmd_name ? $cmd_name : $args->[0], $exit_code);
if (defined $msg) {
# Quiet the 'redundant' warning category, except we
- # need to support down to Perl 5.8, so we can't do a
+ # need to support down to Perl 5.8.1, so we can't do a
# "no warnings 'redundant'", since that category was
# introduced in perl 5.22, and asking for it will die
# on older perls.
diff --git a/git-svn.perl b/git-svn.perl
index 4e8878f035..b0d0a50984 100755
--- a/git-svn.perl
+++ b/git-svn.perl
@@ -1,7 +1,7 @@
#!/usr/bin/perl
# Copyright (C) 2006, Eric Wong <normalperson@yhbt.net>
# License: GPL v2 or later
-use 5.008;
+use 5.008001;
use warnings $ENV{GIT_PERL_FATAL_WARNINGS} ? qw(FATAL all) : ();
use strict;
use vars qw/ $AUTHOR $VERSION
diff --git a/gitweb/INSTALL b/gitweb/INSTALL
index a58e6b3c44..dadc6efa81 100644
--- a/gitweb/INSTALL
+++ b/gitweb/INSTALL
@@ -29,7 +29,7 @@ Requirements
------------
- Core git tools
- - Perl 5.8
+ - Perl 5.8.1
- Perl modules: CGI, Encode, Fcntl, File::Find, File::Basename.
- web server
diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index e66eb3d9ba..55e7c6567e 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -7,7 +7,7 @@
#
# This program is licensed under the GPLv2
-use 5.008;
+use 5.008001;
use strict;
use warnings;
# handle ACL in file access tests
diff --git a/perl/Git.pm b/perl/Git.pm
index 117765dc73..03bf570bf4 100644
--- a/perl/Git.pm
+++ b/perl/Git.pm
@@ -7,7 +7,7 @@ =head1 NAME
package Git;
-use 5.008;
+use 5.008001;
use strict;
use warnings $ENV{GIT_PERL_FATAL_WARNINGS} ? qw(FATAL all) : ();
diff --git a/perl/Git/I18N.pm b/perl/Git/I18N.pm
index 895e759c57..5454c3a6d2 100644
--- a/perl/Git/I18N.pm
+++ b/perl/Git/I18N.pm
@@ -1,5 +1,5 @@
package Git::I18N;
-use 5.008;
+use 5.008001;
use strict;
use warnings $ENV{GIT_PERL_FATAL_WARNINGS} ? qw(FATAL all) : ();
BEGIN {
diff --git a/perl/Git/LoadCPAN.pm b/perl/Git/LoadCPAN.pm
index 0c360bc799..8c7fa805f9 100644
--- a/perl/Git/LoadCPAN.pm
+++ b/perl/Git/LoadCPAN.pm
@@ -1,5 +1,5 @@
package Git::LoadCPAN;
-use 5.008;
+use 5.008001;
use strict;
use warnings $ENV{GIT_PERL_FATAL_WARNINGS} ? qw(FATAL all) : ();
diff --git a/perl/Git/LoadCPAN/Error.pm b/perl/Git/LoadCPAN/Error.pm
index 5d84c20288..5cecb0fcd6 100644
--- a/perl/Git/LoadCPAN/Error.pm
+++ b/perl/Git/LoadCPAN/Error.pm
@@ -1,5 +1,5 @@
package Git::LoadCPAN::Error;
-use 5.008;
+use 5.008001;
use strict;
use warnings $ENV{GIT_PERL_FATAL_WARNINGS} ? qw(FATAL all) : ();
use Git::LoadCPAN (
diff --git a/perl/Git/LoadCPAN/Mail/Address.pm b/perl/Git/LoadCPAN/Mail/Address.pm
index 340e88a7a5..9f808090a6 100644
--- a/perl/Git/LoadCPAN/Mail/Address.pm
+++ b/perl/Git/LoadCPAN/Mail/Address.pm
@@ -1,5 +1,5 @@
package Git::LoadCPAN::Mail::Address;
-use 5.008;
+use 5.008001;
use strict;
use warnings $ENV{GIT_PERL_FATAL_WARNINGS} ? qw(FATAL all) : ();
use Git::LoadCPAN (
diff --git a/perl/Git/Packet.pm b/perl/Git/Packet.pm
index d144f5168f..d896e69523 100644
--- a/perl/Git/Packet.pm
+++ b/perl/Git/Packet.pm
@@ -1,5 +1,5 @@
package Git::Packet;
-use 5.008;
+use 5.008001;
use strict;
use warnings $ENV{GIT_PERL_FATAL_WARNINGS} ? qw(FATAL all) : ();
BEGIN {
diff --git a/t/t0202/test.pl b/t/t0202/test.pl
index 2cbf7b9590..47d96a2a13 100755
--- a/t/t0202/test.pl
+++ b/t/t0202/test.pl
@@ -1,5 +1,5 @@
#!/usr/bin/perl
-use 5.008;
+use 5.008001;
use lib (split(/:/, $ENV{GITPERLLIB}));
use strict;
use warnings;
diff --git a/t/t5562/invoke-with-content-length.pl b/t/t5562/invoke-with-content-length.pl
index 718dd9b49d..9babb9a375 100644
--- a/t/t5562/invoke-with-content-length.pl
+++ b/t/t5562/invoke-with-content-length.pl
@@ -1,4 +1,4 @@
-use 5.008;
+use 5.008001;
use strict;
use warnings;
diff --git a/t/t9700/test.pl b/t/t9700/test.pl
index 6d753708d2..d8e85482ab 100755
--- a/t/t9700/test.pl
+++ b/t/t9700/test.pl
@@ -1,7 +1,7 @@
#!/usr/bin/perl
use lib (split(/:/, $ENV{GITPERLLIB}));
-use 5.008;
+use 5.008001;
use warnings;
use strict;
diff --git a/t/test-terminal.perl b/t/test-terminal.perl
index 1bcf01a9a4..3810e9bb43 100755
--- a/t/test-terminal.perl
+++ b/t/test-terminal.perl
@@ -1,5 +1,5 @@
#!/usr/bin/perl
-use 5.008;
+use 5.008001;
use strict;
use warnings;
use IO::Pty;
--
2.43.0.rc2
^ permalink raw reply related
* [PATCH v3 0/2] send-email: avoid duplicate specification warnings
From: Todd Zullinger @ 2023-11-16 19:30 UTC (permalink / raw)
To: Junio C Hamano
Cc: git, Jeff King, Ævar Arnfjörð Bjarmason,
Ondřej Pohořelský
In-Reply-To: <xmqq4jhmthtg.fsf@gitster.g>
Junio C Hamano wrote:
> Todd Zullinger <tmz@pobox.com> writes:
[...]
>> A little history:
>>
>> Support for the '--no-' prefix was added in Getopt::Long >= 2.33, in
>> commit 8ca8b48 (Negatable options (with "!") now also support the
>> "no-" prefix., 2003-04-04). Getopt::Long 2.34 was included in
>> perl-5.8.1 (2003-09-25), per Module::CoreList[1].
>>
>> We list perl-5.8 as the minimum version in INSTALL. This would leave
>> users with perl-5.8.0 (2002-07-18) with non-working arguments for
>> options where we're removing the explicit 'no-' variant.
>>
>> The explicit 'no-' opts were added in f471494303 (git-send-email.perl:
>> support no- prefix with older GetOptions, 2015-01-30), specifically to
>> support perl-5.8.0 which includes the older Getopt::Long.
>
> These are all very much relevant and deserve to be in the log
> message, not hidden under the three-dash line, I would think.
> Thanks for digging the history. The first paragraph was a bit hard
> to read as it wasn't clear "support" on which side is being
> discussed, though. If it were written perhaps like so:
>
> Getopt::Long >= 2.33 started supporting the '--no-' prefix
> natively by appending '!' to the option specification string,
> which was shipped with perl-5.8.1 and not present in perl-5.8.0
>
> it would have been clear that it was talking about the support
> given by Getopt module, not on our side.
That is much better. I've adjusted the commit message similarly and
hopefully kept your improved wording largely intact.
>> It may be time to bump the Perl requirement to 5.8.1 (2003-09-25) or
>> even 5.10.0 (2007-12-18). We last bumped the requirement from 5.6 to
>> 5.8 in d48b284183 (perl: bump the required Perl version to 5.8 from
>> 5.6.[21], 2010-09-24).
>
> Isn't the position this patch takes a lot stronger than "It may be
> time"? If we applied this patch, it drops the support for folks
> with Perl 5.8.0 (which I do not think is a bad thing, by the way).
Indeed it is. I should have mentioned that more explicitly. I added
the RFC tag to this round because I was unsure whether we'd want to go
the route of bumping the Perl requirement. But I managed to not
actually say as much.
> This sounds like something that is worth describing in the log
> message (and Release Notes).
I think the new commit messages describe the changes better. I didn't
include anything in RelNotes as I was presuming we'd leave this for
2.44 rather than risk causing any problems this late in the 2.43 cycle.
If you think the risk is low and/or the benefit is high, I can add it to
the 2.43.0 RelNotes.
>> diff --git a/git-send-email.perl b/git-send-email.perl
>> index cacdbd6bb2..94046e0fb7 100755
>> --- a/git-send-email.perl
>> +++ b/git-send-email.perl
>> @@ -119,13 +119,16 @@ sub completion_helper {
>>
>> foreach my $key (keys %$original_opts) {
>> unless (exists $not_for_completion{$key}) {
>> - $key =~ s/!$//;
>> + my $negate = ($key =~ s/!$//);
>
> A very minor nit, but I'd call this $negatable if I were doing this
> patch.
Sounds good.
> Just to make sure I did not misunderstand what you said below the
> three-dash line, if we were to take the other option that allows us
> to live with 5.8.0, we would make this hunk ...
>
>> "chain-reply-to!" => \$chain_reply_to,
>> - "no-chain-reply-to" => sub {$chain_reply_to = 0},
>
> ... look more like this?
>
>> - "chain-reply-to!" => \$chain_reply_to,
>> + "chain-reply-to" => \$chain_reply_to,
>> "no-chain-reply-to" => sub {$chain_reply_to = 0},
>> + "nochain-reply-to" => sub {$chain_reply_to = 0},
>
> That is, by removing the "!" suffix, we reject the native support of
> "--no-*" offered by Getopt::Long, and implement the negated variants
> ourselves?
Exactly. We could bundle the two no* options together, but that's a
trivial style issue, i.e.:
> - "chain-reply-to!" => \$chain_reply_to,
> - "no-chain-reply-to" => sub {$chain_reply_to = 0},
> + "chain-reply-to" => \$chain_reply_to,
> + "no-chain-reply-to|nochain-reply-to" => sub {$chain_reply_to = 0},
Thanks for a very helpful review, as always.
Todd Zullinger (2):
perl: bump the required Perl version to 5.8.1 from 5.8.0
send-email: avoid duplicate specification warnings
Documentation/CodingGuidelines | 2 +-
INSTALL | 2 +-
contrib/diff-highlight/DiffHighlight.pm | 2 +-
contrib/mw-to-git/Git/Mediawiki.pm | 2 +-
git-archimport.perl | 2 +-
git-cvsexportcommit.perl | 2 +-
git-cvsimport.perl | 2 +-
git-cvsserver.perl | 2 +-
git-send-email.perl | 23 ++++++++---------------
git-svn.perl | 2 +-
gitweb/INSTALL | 2 +-
gitweb/gitweb.perl | 2 +-
perl/Git.pm | 2 +-
perl/Git/I18N.pm | 2 +-
perl/Git/LoadCPAN.pm | 2 +-
perl/Git/LoadCPAN/Error.pm | 2 +-
perl/Git/LoadCPAN/Mail/Address.pm | 2 +-
perl/Git/Packet.pm | 2 +-
t/t0202/test.pl | 2 +-
t/t5562/invoke-with-content-length.pl | 2 +-
t/t9700/test.pl | 2 +-
t/test-terminal.perl | 2 +-
22 files changed, 29 insertions(+), 36 deletions(-)
Range-diff against v2:
-: ---------- > 1: b276216a53 perl: bump the required Perl version to 5.8.1 from 5.8.0
1: 59e2c79085 ! 2: e076a2ede5 send-email: avoid duplicate specification warnings
@@ Metadata
## Commit message ##
send-email: avoid duplicate specification warnings
- With perl-Getopt-Long >= 2.55 a warning is issued for options which are
- specified more than once. In addition to causing users to see warnings,
- this results in test failures which compare the output. An example,
- from t9001-send-email.37:
+ A warning is issued for options which are specified more than once
+ beginning with perl-Getopt-Long >= 2.55. In addition to causing users
+ to see warnings, this results in test failures which compare the output.
+ An example, from t9001-send-email.37:
| +++ diff -u expect actual
| --- expect 2023-11-14 10:38:23.854346488 +0000
@@ Commit message
| error: last command exited with $?=1
| not ok 37 - reject long lines
- Remove the duplicate option specs.
+ Remove the duplicate option specs. These are primarily the explicit
+ '--no-' prefix opts which were added in f471494303 (git-send-email.perl:
+ support no- prefix with older GetOptions, 2015-01-30). This was done
+ specifically to support perl-5.8.0 which includes Getopt::Long 2.32[1].
+
+ Getopt::Long 2.33 added support for the '--no-' prefix natively by
+ appending '!' to the option specification string, which was included in
+ perl-5.8.1 and is not present in perl-5.8.0. The previous commit bumped
+ the minimum supported Perl version to 5.8.1 so we no longer need to
+ provide the '--no-' variants for negatable options manually.
Teach `--git-completion-helper` to output the '--no-' options. They are
not included in the options hash and would otherwise be lost.
Signed-off-by: Todd Zullinger <tmz@pobox.com>
## git-send-email.perl ##
@@ git-send-email.perl: sub completion_helper {
foreach my $key (keys %$original_opts) {
unless (exists $not_for_completion{$key}) {
- $key =~ s/!$//;
-+ my $negate = ($key =~ s/!$//);
++ my $negatable = ($key =~ s/!$//);
if ($key =~ /[:=][si]$/) {
$key =~ s/[:=][si]$//;
push (@send_email_opts, "--$_=") foreach (split (/\|/, $key));
} else {
push (@send_email_opts, "--$_") foreach (split (/\|/, $key));
-+ if ($negate) {
++ if ($negatable) {
+ push (@send_email_opts, "--no-$_") foreach (split (/\|/, $key));
+ }
}
2: c1f37d4395 < -: ---------- send-email: remove stray characters from usage
--
2.43.0.rc2
^ permalink raw reply
* Re: [PATCH 1/1] attr: add builtin objectmode values support
From: Joanna Wang @ 2023-11-16 17:54 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, sunshine, tboegi
In-Reply-To: <xmqqil62qfvr.fsf@gitster.g>
yes sorry, t/#t1700 was a mistake.
Thank you for the review and further fixes
On Thu, Nov 16, 2023 at 12:08 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> Junio C Hamano <gitster@pobox.com> writes:
>
> > Other than the removal of that file, I locally applied the following
> > fix-up while checking the difference relative to the previous
> > iteration.
>
> Cumulatively, aside from the removal of the t/#t* file, here is what
> I ended up with so far.
>
> Subject: [PATCH] SQUASH???
>
> ---
> Documentation/gitattributes.txt | 2 +-
> neue | 0
> t/t0003-attributes.sh | 5 +++--
> t/t6135-pathspec-with-attrs.sh | 10 ++++++----
> 4 files changed, 10 insertions(+), 7 deletions(-)
> create mode 100644 neue
>
> diff --git a/Documentation/gitattributes.txt b/Documentation/gitattributes.txt
> index 784aa9d4de..201bdf5edb 100644
> --- a/Documentation/gitattributes.txt
> +++ b/Documentation/gitattributes.txt
> @@ -108,7 +108,7 @@ user defined attributes under this namespace will be ignored and
> trigger a warning.
>
> `builtin_objectmode`
> -^^^^^^^^^^^^^^^^^^^^
> +~~~~~~~~~~~~~~~~~~~~
> This attribute is for filtering files by their file bit modes (40000,
> 120000, 160000, 100755, 100644). e.g. ':(attr:builtin_objectmode=160000)'.
> You may also check these values with `git check-attr builtin_objectmode -- <file>`.
> diff --git a/neue b/neue
> new file mode 100644
> index 0000000000..e69de29bb2
> diff --git a/t/t0003-attributes.sh b/t/t0003-attributes.sh
> index 86f8681570..774b52c298 100755
> --- a/t/t0003-attributes.sh
> +++ b/t/t0003-attributes.sh
> @@ -580,12 +580,13 @@ test_expect_success 'builtin object mode attributes work (dir and regular paths)
> '
>
> test_expect_success POSIXPERM 'builtin object mode attributes work (executable)' '
> - >exec && chmod +x exec &&
> + >exec &&
> + chmod +x exec &&
> attr_check_object_mode exec 100755
> '
>
> test_expect_success SYMLINKS 'builtin object mode attributes work (symlinks)' '
> - >to_sym ln -s to_sym sym &&
> + ln -s to_sym sym &&
> attr_check_object_mode sym 120000
> '
>
> diff --git a/t/t6135-pathspec-with-attrs.sh b/t/t6135-pathspec-with-attrs.sh
> index b08a32ea68..f6403ebbda 100755
> --- a/t/t6135-pathspec-with-attrs.sh
> +++ b/t/t6135-pathspec-with-attrs.sh
> @@ -295,22 +295,24 @@ test_expect_success 'reading from .gitattributes in a subdirectory (3)' '
> test_cmp expect actual
> '
>
> -test_expect_success 'pathspec with builtin_objectmode attr can be used' '
> +test_expect_success POSIXPERM 'pathspec with builtin_objectmode attr can be used' '
> >mode_exec_file_1 &&
>
> git status -s ":(attr:builtin_objectmode=100644)mode_exec_*" >actual &&
> echo ?? mode_exec_file_1 >expect &&
> test_cmp expect actual &&
>
> - git add mode_exec_file_1 && chmod +x mode_exec_file_1 &&
> + git add mode_exec_file_1 &&
> + chmod +x mode_exec_file_1 &&
> git status -s ":(attr:builtin_objectmode=100755)mode_exec_*" >actual &&
> echo AM mode_exec_file_1 >expect &&
> test_cmp expect actual
> '
>
> -test_expect_success 'builtin_objectmode attr can be excluded' '
> +test_expect_success POSIXPERM 'builtin_objectmode attr can be excluded' '
> >mode_1_regular &&
> - >mode_1_exec && chmod +x mode_1_exec &&
> + >mode_1_exec &&
> + chmod +x mode_1_exec &&
> git status -s ":(exclude,attr:builtin_objectmode=100644)" "mode_1_*" >actual &&
> echo ?? mode_1_exec >expect &&
> test_cmp expect actual &&
> --
> 2.43.0-rc2
>
^ permalink raw reply
* Re: Feature request: git status --branch-only
From: phillip.wood123 @ 2023-11-16 16:09 UTC (permalink / raw)
To: Jeff King, phillip.wood; +Cc: Ondra Medek, git
In-Reply-To: <20231114201808.GE2092538@coredump.intra.peff.net>
Hi Peff
On 14/11/2023 20:18, Jeff King wrote:
> On Tue, Nov 14, 2023 at 03:02:04PM +0000, Phillip Wood wrote:
>
>> Hi Ondra
>>
>> On 14/11/2023 12:40, Ondra Medek wrote:
>>> Hi Phillip,
>>>
>>> it does not work for a fresh clone of an empty repository
>>>
>>> git for-each-ref --format="%(upstream:short)" refs/heads/master
>>>
>>> outputs nothing, while
>>
>> Oh dear, that's a shame. I wonder if it is a bug because the documentation
>> says that
>>
>> --format="%(upstream:track)"
>>
>> should print "[gone]" whenever an unknown upstream ref is encountered but
>> trying that on a clone of an empty repository gives no output.
>
> I think it would print "gone" if the upstream branch went missing. But
> in this case the actual local branch is missing. And for-each-ref will
> not show an entry at all for a ref that does not exist. The
> "refs/heads/master" on your command line is not a ref, but a pattern,
> and that pattern does not match anything. So it's working as intended.
Oh of course, I'd somehow forgotten that "refs/heads/master" did not
exist so it makes sense that for-each-ref does not print anything.
> I think a more direct tool would be:
>
> git rev-parse --symbolic-full-name master@{upstream}
>
> That convinces branch_get_upstream() to return the value we want, but
> sadly it seems to get lost somewhere in the resolution process, and we
> spit out an error. Arguably that is a bug (with --symbolic or
> --symbolic-full-name, I think it would be OK to resolve names even if
> they don't point to something, but it's possible that would have other
> unexpected side effects).
Yeah, maybe we should look at fixing that - I didn't suggest it because
I knew it did not work on an unborn branch but as you say there is no
obvious reason why it shouldn't
Best Wishes
Phillip
> -Peff
^ permalink raw reply
* Re: Feasibility of folding `unit-tests` into `make test`, was Re: [PATCH] ci: avoid running the test suite _twice_
From: phillip.wood123 @ 2023-11-16 15:05 UTC (permalink / raw)
To: Johannes Schindelin, Josh Steadmon
Cc: Jeff King, Johannes Schindelin via GitGitGadget, Phillip Wood,
git
In-Reply-To: <850ea42c-f103-68d5-896b-9120e2628686@gmx.de>
On 16/11/2023 08:42, Johannes Schindelin wrote:
> On Wed, 15 Nov 2023, Josh Steadmon wrote:
>> On 2023.11.13 13:49, Jeff King wrote:
>> We could bundle all the unit tests into a single shell script, but then
>> we lose parallelization and add hoops to jump through to determine what
>> breaks. Or we could autogenerate a corresponding shell script to run
>> each individual unit test, but that seems gross. Of course, these are
>> hypothetical concerns for now, since we only have a single unit test at
>> the moment.
>
> I totally agree with you, Josh, that it makes little sense to
> try to contort the unit tests to be run in the same `prove` run as the
> regression tests that need to be invoked so totally differently.
FWIW that's my feeling too. It makes sense for "make test" to run the
unit tests, but wrapping the unit tests in one or more shell scripts
adds unnecessary complexity.
Best Wishes
Phillip
^ permalink raw reply
* Re: [PATCH v2 02/10] ref-filter.h: add max_count and omit_empty to ref_format
From: Øystein Walle @ 2023-11-16 12:06 UTC (permalink / raw)
To: gitgitgadget; +Cc: code, git, oystwa, ps, vdye
In-Reply-To: <adac101bc6022d5477371d6a94225f38da7fffee.1699991638.git.gitgitgadget@gmail.com>
Victoria Dye <vdye@github.com> writes:
> diff --git a/ref-filter.h b/ref-filter.h
> index 1524bc463a5..d87d61238b7 100644
> --- a/ref-filter.h
> +++ b/ref-filter.h
> @@ -92,6 +92,11 @@ struct ref_format {
>
> /* List of bases for ahead-behind counts. */
> struct string_list bases;
> +
> + struct {
> + int max_count;
> + int omit_empty;
> + } array_opts;
> };
What the benefit of having them in a nested struct is compared to just
two distinct members?
Regardless this is the kind of deduplication I wanted to achieve when I
added --omit-empty, but never did. Either way, I meant to ack this in
the last round never got around to it. Nice work.
Acked-by: Øystein Walle <oystwa@gmail.com>
Øsse
^ permalink raw reply
* Re: [PATCH] commit-graph: disable GIT_COMMIT_GRAPH_PARANOIA by default
From: Patrick Steinhardt @ 2023-11-16 11:19 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jeff King, git, Karthik Nayak
In-Reply-To: <xmqqh6lmwogq.fsf@gitster.g>
[-- Attachment #1: Type: text/plain, Size: 2474 bytes --]
On Thu, Nov 16, 2023 at 09:07:01AM +0900, Junio C Hamano wrote:
> Patrick Steinhardt <ps@pks.im> writes:
>
> >> Yeah. Just like we auto-enabled GIT_REF_PARANOIA for git-gc, etc, I
> >> think we should do the same here.
> >
> > I'm honestly still torn on this one. There are two cases that I can
> > think of where missing objects would be benign and where one wants to
> > use `git rev-list --missing`:
> >
> > - Repositories with promisor remotes, to find the boundary of where
> > we need to fetch new objects.
> >
> > - Quarantine directories where you only intend to list new objects
> > or find the boundary.
> >
> > And in neither of those cases I can see a path for how the commit-graph
> > would contain such missing commits when using regular tooling to perform
> > repository maintenance.
>
> I can buy the promisor remotes use case---we may expect boundary
> objects missing without any repository corruption. I do not know
> about the other one---where does our "rev-list --missing" start
> traversing in a quarantine directory is unclear. Objects that are
> still in quarantine are not (yet) made reachable from any refs, so
> even "rev-list --missing --all" would not make a useful traversal,
> no?
You typically know about the new tips when having a quarantine
directory. So you can discover the boundary between objects in your
quarantine directory and your main object database by executing `git
rev-list $NEWREVS --missing=print` and execute that command with
`GIT_OBJECT_DIRECTORY=$quarantine`. The benefit is that this scales with
the number of objects in the quarantine, and not with the size of the
overall repository.
As mention, this is really niche, but we do plan to use this in Gitaly
eventually.
> In any case, it sounds like you are not torn but are convinced that
> we should leave this loose by default ;-) and after thinking it over
> again, I tend to agree that it would be a better choice, as long as
> the feature "rev-list --missing" has any good use case other than
> corruption in repository.
>
> So,... thanks for pushing back.
Okay, glad to hear that I'm not totally bonkers then. I'm going to wait
another few days for additional feedback before sending a v2. But if
what I'm saying also makes sense to others then v2 would only squash in
the diff I sent to run the subset of tests that is now failing with
`GIT_COMMIT_GRAPH_PARANOIA=true`.
Patrick
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH v7 00/14] Introduce new `git replay` command
From: Johannes Schindelin @ 2023-11-16 8:53 UTC (permalink / raw)
To: Christian Couder
Cc: git, Junio C Hamano, Patrick Steinhardt, Elijah Newren, John Cai,
Derrick Stolee, Phillip Wood, Calvin Wan, Toon Claes,
Dragan Simic, Linus Arver
In-Reply-To: <20231115143327.2441397-1-christian.couder@gmail.com>
Hi Christian,
[focusing exclusively on the `range-diff` because I lack the capacity for
anything beyond that]
On Wed, 15 Nov 2023, Christian Couder wrote:
> # Range-diff between v6 and v7
>
> (Sorry it looks like patch 6/14 in v7 is considered to be completely
> different from what it was in v6, so the range-diff is not showing
> differences between them.)
>
> 1: fac0a9dff4 = 1: cddcd967b2 t6429: remove switching aspects of fast-rebase
> 2: bec2eb8928 ! 2: c8476fb093 replay: introduce new builtin
> @@ Documentation/git-replay.txt (new)
> +DESCRIPTION
> +-----------
> +
> -+Takes a range of commits and replays them onto a new location.
> ++Takes a range of commits, specified by <oldbase> and <branch>, and
> ++replays them onto a new location (see `--onto` option below).
> +
> +THIS COMMAND IS EXPERIMENTAL. THE BEHAVIOR MAY CHANGE.
> +
Thank you.
> 3: b0cdfdc0c3 = 3: 43322abd1e replay: start using parse_options API
> 4: c3403f0b9d = 4: 6524c7f045 replay: die() instead of failing assert()
> 5: 4188eeac30 = 5: 05d0efa3cb replay: introduce pick_regular_commit()
> 6: b7b4d9001e < -: ---------- replay: change rev walking options
> -: ---------- > 6: c7a5aad3d6 replay: change rev walking options
The actual range-diff for the sixth patch looks like this:
-- snip --
6: b7b4d9001e9 ! 6: c7a5aad3d62 replay: change rev walking options
@@ Metadata
## Commit message ##
replay: change rev walking options
- Let's set the rev walking options we need after calling
- setup_revisions() instead of before. This enforces options we always
- want for now.
+ Let's force the rev walking options we need after calling
+ setup_revisions() instead of before.
+
+ This might override some user supplied rev walking command line options
+ though. So let's detect that and warn users by:
+
+ a) setting the desired values, before setup_revisions(),
+ b) checking after setup_revisions() whether these values differ from
+ the desired values,
+ c) if so throwing a warning and setting the desired values again.
We want the command to work from older commits to newer ones by default.
Also we don't want history simplification, as we want to deal with all
the commits in the affected range.
- When we see an option that we are going to override, we emit a warning
- to avoid confusion as much as possible though.
-
Helped-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>
Co-authored-by: Christian Couder <chriscool@tuxfamily.org>
Signed-off-by: Elijah Newren <newren@gmail.com>
@@ Commit message
## builtin/replay.c ##
@@ builtin/replay.c: int cmd_replay(int argc, const char **argv, const char *prefix)
- struct merge_result result;
- struct strbuf reflog_msg = STRBUF_INIT;
- struct strbuf branch_name = STRBUF_INIT;
-- int ret = 0;
-+ int i, ret = 0;
-
- const char * const replay_usage[] = {
- N_("git replay --onto <newbase> <oldbase> <branch> # EXPERIMENTAL"),
-@@ builtin/replay.c: int cmd_replay(int argc, const char **argv, const char *prefix)
repo_init_revisions(the_repository, &revs, prefix);
@@ builtin/replay.c: int cmd_replay(int argc, const char **argv, const char *prefix
- revs.max_parents = 1;
- revs.cherry_mark = 1;
- revs.limited = 1;
-- revs.reverse = 1;
++ strvec_pushl(&rev_walk_args, "", argv[2], "--not", argv[1], NULL);
++
++ /*
++ * Set desired values for rev walking options here. If they
++ * are changed by some user specified option in setup_revisions()
++ * below, we will detect that below and then warn.
++ *
++ * TODO: In the future we might want to either die(), or allow
++ * some options changing these values if we think they could
++ * be useful.
++ */
+ revs.reverse = 1;
- revs.right_only = 1;
-- revs.sort_order = REV_SORT_IN_GRAPH_ORDER;
-- revs.topo_order = 1;
+ revs.sort_order = REV_SORT_IN_GRAPH_ORDER;
+ revs.topo_order = 1;
-
- strvec_pushl(&rev_walk_args, "", argv[2], "--not", argv[1], NULL);
+- strvec_pushl(&rev_walk_args, "", argv[2], "--not", argv[1], NULL);
++ revs.simplify_history = 0;
-+ /*
-+ * TODO: For now, let's warn when we see an option that we are
-+ * going to override after setup_revisions() below. In the
-+ * future we might want to either die() or allow them if we
-+ * think they could be useful though.
-+ */
-+ for (i = 0; i < argc; i++) {
-+ if (!strcmp(argv[i], "--reverse") || !strcmp(argv[i], "--date-order") ||
-+ !strcmp(argv[i], "--topo-order") || !strcmp(argv[i], "--author-date-order") ||
-+ !strcmp(argv[i], "--full-history"))
-+ warning(_("option '%s' will be overridden"), argv[i]);
-+ }
-+
if (setup_revisions(rev_walk_args.nr, rev_walk_args.v, &revs, NULL) > 1) {
ret = error(_("unhandled options"));
goto cleanup;
}
-+ /* requirements/overrides for revs */
-+ revs.reverse = 1;
-+ revs.sort_order = REV_SORT_IN_GRAPH_ORDER;
-+ revs.topo_order = 1;
-+ revs.simplify_history = 0;
++ /*
++ * Detect and warn if we override some user specified rev
++ * walking options.
++ */
++ if (revs.reverse != 1) {
++ warning(_("some rev walking options will be overridden as "
++ "'%s' bit in 'struct rev_info' will be forced"),
++ "reverse");
++ revs.reverse = 1;
++ }
++ if (revs.sort_order != REV_SORT_IN_GRAPH_ORDER) {
++ warning(_("some rev walking options will be overridden as "
++ "'%s' bit in 'struct rev_info' will be forced"),
++ "sort_order");
++ revs.sort_order = REV_SORT_IN_GRAPH_ORDER;
++ }
++ if (revs.topo_order != 1) {
++ warning(_("some rev walking options will be overridden as "
++ "'%s' bit in 'struct rev_info' will be forced"),
++ "topo_order");
++ revs.topo_order = 1;
++ }
++ if (revs.simplify_history != 0) {
++ warning(_("some rev walking options will be overridden as "
++ "'%s' bit in 'struct rev_info' will be forced"),
++ "simplify_history");
++ revs.simplify_history = 0;
++ }
+
strvec_clear(&rev_walk_args);
-- snap --
This looks really good. Thank you for going the extra step to make this
patch so much better.
> 7: c57577a9b8 = 7: 01f35f924b replay: add an important FIXME comment about gpg signing
> 8: e78be50f3d = 8: 1498b24bad replay: remove progress and info output
> 9: e4c79b676f = 9: 6786fc147b replay: remove HEAD related sanity check
> 10: 8d89f1b733 ! 10: 9a24dbb530 replay: make it a minimal server side command
> @@ Commit message
> Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
>
> ## Documentation/git-replay.txt ##
> -@@ Documentation/git-replay.txt: SYNOPSIS
> - DESCRIPTION
> +@@ Documentation/git-replay.txt: DESCRIPTION
> -----------
>
> --Takes a range of commits and replays them onto a new location.
> -+Takes a range of commits and replays them onto a new location. Leaves
> -+the working tree and the index untouched, and updates no
> -+references. The output of this command is meant to be used as input to
> + Takes a range of commits, specified by <oldbase> and <branch>, and
> +-replays them onto a new location (see `--onto` option below).
> ++replays them onto a new location (see `--onto` option below). Leaves
> ++the working tree and the index untouched, and updates no references.
> ++The output of this command is meant to be used as input to
> +`git update-ref --stdin`, which would update the relevant branches.
>
> THIS COMMAND IS EXPERIMENTAL. THE BEHAVIOR MAY CHANGE.
> @@ builtin/replay.c: int cmd_replay(int argc, const char **argv, const char *prefix
> struct merge_result result;
> - struct strbuf reflog_msg = STRBUF_INIT;
> struct strbuf branch_name = STRBUF_INIT;
> - int i, ret = 0;
> + int ret = 0;
>
> @@ builtin/replay.c: int cmd_replay(int argc, const char **argv, const char *prefix)
> onto = peel_committish(onto_name);
Looks good to me.
> 11: 3d433a1322 ! 11: ad6ca2fbef replay: use standard revision ranges
> @@ Documentation/git-replay.txt: git-replay - EXPERIMENTAL: Replay commits on a new
>
> DESCRIPTION
> -----------
> -@@ Documentation/git-replay.txt: DESCRIPTION
> - Takes a range of commits and replays them onto a new location. Leaves
> - the working tree and the index untouched, and updates no
> - references. The output of this command is meant to be used as input to
> +
> +-Takes a range of commits, specified by <oldbase> and <branch>, and
> +-replays them onto a new location (see `--onto` option below). Leaves
> ++Takes ranges of commits and replays them onto a new location. Leaves
> + the working tree and the index untouched, and updates no references.
> + The output of this command is meant to be used as input to
> -`git update-ref --stdin`, which would update the relevant branches.
> +`git update-ref --stdin`, which would update the relevant branches
> +(see the OUTPUT section below).
> @@ builtin/replay.c: int cmd_replay(int argc, const char **argv, const char *prefix
> struct merge_options merge_opt;
> struct merge_result result;
> - struct strbuf branch_name = STRBUF_INIT;
> - int i, ret = 0;
> + int ret = 0;
>
> const char * const replay_usage[] = {
> - N_("git replay --onto <newbase> <oldbase> <branch> # EXPERIMENTAL"),
> @@ builtin/replay.c: int cmd_replay(int argc, const char **argv, const char *prefix
> - strvec_pushl(&rev_walk_args, "", argv[2], "--not", argv[1], NULL);
> -
> /*
> - * TODO: For now, let's warn when we see an option that we are
> - * going to override after setup_revisions() below. In the
> + * Set desired values for rev walking options here. If they
> + * are changed by some user specified option in setup_revisions()
> @@ builtin/replay.c: int cmd_replay(int argc, const char **argv, const char *prefix)
> - warning(_("option '%s' will be overridden"), argv[i]);
> - }
> + revs.topo_order = 1;
> + revs.simplify_history = 0;
>
> - if (setup_revisions(rev_walk_args.nr, rev_walk_args.v, &revs, NULL) > 1) {
> - ret = error(_("unhandled options"));
> @@ builtin/replay.c: int cmd_replay(int argc, const char **argv, const char *prefix
> }
>
> @@ builtin/replay.c: int cmd_replay(int argc, const char **argv, const char *prefix)
> - revs.topo_order = 1;
> - revs.simplify_history = 0;
> + revs.simplify_history = 0;
> + }
>
> - strvec_clear(&rev_walk_args);
> -
This is the correct spot for that documentation change. Good.
> 12: cca8105382 ! 12: 081864ed5f replay: add --advance or 'cherry-pick' mode
> @@ builtin/replay.c: static struct commit *pick_regular_commit(struct commit *pickm
> struct merge_options merge_opt;
> struct merge_result result;
> + struct strset *update_refs = NULL;
> - int i, ret = 0;
> + int ret = 0;
>
> const char * const replay_usage[] = {
> - N_("git replay --onto <newbase> <revision-range>... # EXPERIMENTAL"),
> @@ builtin/replay.c: int cmd_replay(int argc, const char **argv, const char *prefix
>
> /*
> @@ builtin/replay.c: int cmd_replay(int argc, const char **argv, const char *prefix)
> - revs.topo_order = 1;
> - revs.simplify_history = 0;
> + revs.simplify_history = 0;
> + }
>
> + determine_replay_mode(&revs.cmdline, onto_name, &advance_name,
> + &onto, &update_refs);
> 13: 92287a2cc8 ! 13: 19c4016c7c replay: add --contained to rebase contained branches
> @@ builtin/replay.c: int cmd_replay(int argc, const char **argv, const char *prefix
> struct rev_info revs;
> struct commit *last_commit = NULL;
> @@ builtin/replay.c: int cmd_replay(int argc, const char **argv, const char *prefix)
> - int i, ret = 0;
> + int ret = 0;
>
> const char * const replay_usage[] = {
> - N_("git replay (--onto <newbase> | --advance <branch>) <revision-range>... # EXPERIMENTAL"),
> 14: 529a7fda40 ! 14: 29556bcc86 replay: stop assuming replayed branches do not diverge
> @@ builtin/replay.c: int cmd_replay(int argc, const char **argv, const char *prefix
> struct merge_result result;
> struct strset *update_refs = NULL;
> + kh_oid_map_t *replayed_commits;
> - int i, ret = 0;
> + int ret = 0;
>
> const char * const replay_usage[] = {
> @@ builtin/replay.c: int cmd_replay(int argc, const char **argv, const char *prefix)
The last three only have context-only changes. Obviously good.
Apart from the one little outstanding nit where I would love to see
`(EXPERIMENTAL!)` as the first word of the synopsis both in the manual
page and in the output of `git replay -h`, you have addressed all of my
concerns.
Thank you!
Johannes
^ permalink raw reply
* Re: [PATCH v6 00/14] Introduce new `git replay` command
From: Christian Couder @ 2023-11-16 8:52 UTC (permalink / raw)
To: Johannes Schindelin
Cc: git, Junio C Hamano, Patrick Steinhardt, Elijah Newren, John Cai,
Derrick Stolee, Phillip Wood, Calvin Wan, Toon Claes,
Dragan Simic, Linus Arver
In-Reply-To: <0ddca907-6e64-b684-2e08-c7e95e737a3c@gmx.de>
Hi Dscho,
On Thu, Nov 16, 2023 at 9:45 AM Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
> On Wed, 15 Nov 2023, Christian Couder wrote:
> > I have fixed that in the v7 I just sent with the following:
> >
> > +SYNOPSIS
> > +--------
> > +[verse]
> > +'git replay' --onto <newbase> <oldbase> <branch> # EXPERIMENTAL
>
> I still think that the following would serve us better:
>
> [verse]
> (EXPERIMENTAL!) 'git replay' --onto <newbase> <oldbase> <branch>
>
> But if nobody else feels as strongly, I won't bring this up again.
For the tests to pass, the SYNOPSIS should be the same as the first
line of the `git replay -h` output. After this series it is:
usage: git replay ([--contained] --onto <newbase> | --advance
<branch>) <revision-range>... # EXPERIMENTAL
As the usage is supposed to describe how the command should be used, I
think it makes sense for "EXPERIMENTAL" to be in a shell comment after
the command.
Thanks,
Christian.
^ permalink raw reply
* Re: [PATCH v6 00/14] Introduce new `git replay` command
From: Johannes Schindelin @ 2023-11-16 8:45 UTC (permalink / raw)
To: Christian Couder
Cc: git, Junio C Hamano, Patrick Steinhardt, Elijah Newren, John Cai,
Derrick Stolee, Phillip Wood, Calvin Wan, Toon Claes,
Dragan Simic, Linus Arver
In-Reply-To: <CAP8UFD0Es4qai98WB6bpykisBT628JndPXG8jg1=_uUbn4zogA@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 2090 bytes --]
Hi Christian,
On Wed, 15 Nov 2023, Christian Couder wrote:
> On Wed, Nov 8, 2023 at 1:47 PM Johannes Schindelin
> <Johannes.Schindelin@gmx.de> wrote:
> > On Thu, 2 Nov 2023, Christian Couder wrote:
>
> > > + ## Documentation/git-replay.txt (new) ##
> > > +@@
> > > ++git-replay(1)
> > > ++=============
> > > ++
> > > ++NAME
> > > ++----
> > > ++git-replay - EXPERIMENTAL: Replay commits on a new base, works with bare repos too
> > > ++
> > > ++
> > > ++SYNOPSIS
> > > ++--------
> > > ++[verse]
> > > ++'git replay' --onto <newbase> <revision-range>... # EXPERIMENTAL
> >
> > Technically, at this stage `git replay` requires precisely 5 arguments, so
> > the `<revision>...` is incorrect and should be `<upstream> <branch>`
> > instead. But it's not worth a new iteration to fix this.
>
> It was actually:
>
> 'git replay' --onto <newbase> <oldbase> <branch> # EXPERIMENTAL
Right.
> > > ++
> > > ++DESCRIPTION
> > > ++-----------
> > > ++
> > > ++Takes a range of commits and replays them onto a new location.
> > > ++
> > > ++THIS COMMAND IS EXPERIMENTAL. THE BEHAVIOR MAY CHANGE.
> > > ++
> > > ++OPTIONS
> > > ++-------
> > > ++
> > > ++--onto <newbase>::
> > > ++ Starting point at which to create the new commits. May be any
> > > ++ valid commit, and not just an existing branch name.
> > > ++
> >
> > Traditionally, this would be a place to describe the `<revision>` argument
> > (or, in this patch, to reflect the current state of `builtin/replay.c`,
> > the `<upstream> <branch>` arguments instead).
>
> I have fixed that in the v7 I just sent with the following:
>
> +SYNOPSIS
> +--------
> +[verse]
> +'git replay' --onto <newbase> <oldbase> <branch> # EXPERIMENTAL
I still think that the following would serve us better:
[verse]
(EXPERIMENTAL!) 'git replay' --onto <newbase> <oldbase> <branch>
But if nobody else feels as strongly, I won't bring this up again.
Ciao,
Johannes
^ permalink raw reply
* Feasibility of folding `unit-tests` into `make test`, was Re: [PATCH] ci: avoid running the test suite _twice_
From: Johannes Schindelin @ 2023-11-16 8:42 UTC (permalink / raw)
To: Josh Steadmon
Cc: Jeff King, Johannes Schindelin via GitGitGadget, Phillip Wood,
git
In-Reply-To: <ZVU4EVcj0MDrSNcG@google.com>
[-- Attachment #1: Type: text/plain, Size: 3298 bytes --]
Hi Josh,
On Wed, 15 Nov 2023, Josh Steadmon wrote:
> On 2023.11.13 13:49, Jeff King wrote:
> >
> > why are the unit tests totally separate from the rest of the suite? I
> > would think we'd want them run from one or more t/t*.sh scripts. That
> > would make bugs like this impossible, but also:
> >
> > 1. They'd be run via "make test", so developers don't have to remember
> > to run them separately.
> >
> > 2. They can be run in parallel with all of the other tests when using
> > "prove -j", etc.
>
> The first part is easy, but I don't see a good way to get both shell
> tests and unit tests executing under the same `prove` process. For shell
> tests, we pass `--exec '$(TEST_SHELL_PATH_SQ)'` to prove, meaning that
> we use the specified shell as an interpreter for the test files. That
> will not work for unit test executables.
Probably my favorite aspect about the new unit tests is that they avoid
using the error-prone, unintuitive and slow shell scripts and stay within
the programming language of the code that is to be tested: C.
> We could bundle all the unit tests into a single shell script, but then
> we lose parallelization and add hoops to jump through to determine what
> breaks. Or we could autogenerate a corresponding shell script to run
> each individual unit test, but that seems gross. Of course, these are
> hypothetical concerns for now, since we only have a single unit test at
> the moment.
I totally agree with you, Josh, that it makes little sense to
try to contort the unit tests to be run in the same `prove` run as the
regression tests that need to be invoked so totally differently.
> There's also the issue that the shell test arguments we pass on from
> prove would be shared with the unit tests. That's fine for now, as
> t-strbuf doesn't accept any runtime arguments, but it's possible that
> either the framework or individual unit tests might grow to need
> arguments, and it might not be convenient to stay compatible with the
> shell tests.
>
> Personally, I lean towards keeping things simple and just running a
> second `prove` process as part of `make test`.
Agreed.
> If I was forced to pick a way to get everything under one process, I'd
> lean towards autogenerating individual shell script wrappers for each
> unit test. But I'm open to discussion, especially if people have other
> approaches I haven't thought of.
One alternative would be to avoid running the unit tests via `prove` in
the first place.
For example, we could use the helper from be5d88e11280 (test-tool
run-command: learn to run (parts of) the testsuite, 2019-10-04) [*1*]. It
would probably need a few improvements, but certainly no wizardry nor
witchcraft would be required. It would also help on Windows, where running
a simple test helper written in C is vastly faster than running a complex
Perl script (which `prove` is).
Ciao,
Johannes
Footnote *1*: I had always wanted to improve that test helper to the point
where it could replace our use of `prove`, at least on Windows. It seems,
however, that as of 4c2c38e800f3 (ci: modification of main.yml to use
cmake for vs-build job, 2020-06-26) we do not use the helper at all
anymore. Hopefully it can still be useful. 🤞
^ permalink raw reply
* Re: [PATCH 1/1] attr: add builtin objectmode values support
From: Junio C Hamano @ 2023-11-16 8:08 UTC (permalink / raw)
To: Joanna Wang; +Cc: git, sunshine, tboegi
In-Reply-To: <xmqqsf56ql14.fsf@gitster.g>
Junio C Hamano <gitster@pobox.com> writes:
> Other than the removal of that file, I locally applied the following
> fix-up while checking the difference relative to the previous
> iteration.
Cumulatively, aside from the removal of the t/#t* file, here is what
I ended up with so far.
Subject: [PATCH] SQUASH???
---
Documentation/gitattributes.txt | 2 +-
neue | 0
t/t0003-attributes.sh | 5 +++--
t/t6135-pathspec-with-attrs.sh | 10 ++++++----
4 files changed, 10 insertions(+), 7 deletions(-)
create mode 100644 neue
diff --git a/Documentation/gitattributes.txt b/Documentation/gitattributes.txt
index 784aa9d4de..201bdf5edb 100644
--- a/Documentation/gitattributes.txt
+++ b/Documentation/gitattributes.txt
@@ -108,7 +108,7 @@ user defined attributes under this namespace will be ignored and
trigger a warning.
`builtin_objectmode`
-^^^^^^^^^^^^^^^^^^^^
+~~~~~~~~~~~~~~~~~~~~
This attribute is for filtering files by their file bit modes (40000,
120000, 160000, 100755, 100644). e.g. ':(attr:builtin_objectmode=160000)'.
You may also check these values with `git check-attr builtin_objectmode -- <file>`.
diff --git a/neue b/neue
new file mode 100644
index 0000000000..e69de29bb2
diff --git a/t/t0003-attributes.sh b/t/t0003-attributes.sh
index 86f8681570..774b52c298 100755
--- a/t/t0003-attributes.sh
+++ b/t/t0003-attributes.sh
@@ -580,12 +580,13 @@ test_expect_success 'builtin object mode attributes work (dir and regular paths)
'
test_expect_success POSIXPERM 'builtin object mode attributes work (executable)' '
- >exec && chmod +x exec &&
+ >exec &&
+ chmod +x exec &&
attr_check_object_mode exec 100755
'
test_expect_success SYMLINKS 'builtin object mode attributes work (symlinks)' '
- >to_sym ln -s to_sym sym &&
+ ln -s to_sym sym &&
attr_check_object_mode sym 120000
'
diff --git a/t/t6135-pathspec-with-attrs.sh b/t/t6135-pathspec-with-attrs.sh
index b08a32ea68..f6403ebbda 100755
--- a/t/t6135-pathspec-with-attrs.sh
+++ b/t/t6135-pathspec-with-attrs.sh
@@ -295,22 +295,24 @@ test_expect_success 'reading from .gitattributes in a subdirectory (3)' '
test_cmp expect actual
'
-test_expect_success 'pathspec with builtin_objectmode attr can be used' '
+test_expect_success POSIXPERM 'pathspec with builtin_objectmode attr can be used' '
>mode_exec_file_1 &&
git status -s ":(attr:builtin_objectmode=100644)mode_exec_*" >actual &&
echo ?? mode_exec_file_1 >expect &&
test_cmp expect actual &&
- git add mode_exec_file_1 && chmod +x mode_exec_file_1 &&
+ git add mode_exec_file_1 &&
+ chmod +x mode_exec_file_1 &&
git status -s ":(attr:builtin_objectmode=100755)mode_exec_*" >actual &&
echo AM mode_exec_file_1 >expect &&
test_cmp expect actual
'
-test_expect_success 'builtin_objectmode attr can be excluded' '
+test_expect_success POSIXPERM 'builtin_objectmode attr can be excluded' '
>mode_1_regular &&
- >mode_1_exec && chmod +x mode_1_exec &&
+ >mode_1_exec &&
+ chmod +x mode_1_exec &&
git status -s ":(exclude,attr:builtin_objectmode=100644)" "mode_1_*" >actual &&
echo ?? mode_1_exec >expect &&
test_cmp expect actual &&
--
2.43.0-rc2
^ permalink raw reply related
* Re: [PATCH 1/1] attr: add builtin objectmode values support
From: Junio C Hamano @ 2023-11-16 7:57 UTC (permalink / raw)
To: Joanna Wang; +Cc: git, sunshine, tboegi
In-Reply-To: <20231116054437.2343549-1-jojwang@google.com>
Joanna Wang <jojwang@google.com> writes:
> diff --git a/t/t6135-pathspec-with-attrs.sh b/t/t6135-pathspec-with-attrs.sh
> index a9c1e4e0ec..b08a32ea68 100755
> --- a/t/t6135-pathspec-with-attrs.sh
> +++ b/t/t6135-pathspec-with-attrs.sh
> @@ -295,4 +295,29 @@ test_expect_success 'reading from .gitattributes in a subdirectory (3)' '
> test_cmp expect actual
> '
>
> +test_expect_success 'pathspec with builtin_objectmode attr can be used' '
> + >mode_exec_file_1 &&
> +
> + git status -s ":(attr:builtin_objectmode=100644)mode_exec_*" >actual &&
> + echo ?? mode_exec_file_1 >expect &&
> + test_cmp expect actual &&
> +
> + git add mode_exec_file_1 && chmod +x mode_exec_file_1 &&
This again would break with filesystems that are incapable of
supporting executable bit natively. Writing one command per line,
doing something like this may help
git add mode_exec_file_1 &&
git update-index --chmod=+x mode_exec_file_1 &&
> + git status -s ":(attr:builtin_objectmode=100755)mode_exec_*" >actual &&
> + echo AM mode_exec_file_1 >expect &&
> + test_cmp expect actual
> +'
> +
> +test_expect_success 'builtin_objectmode attr can be excluded' '
> + >mode_1_regular &&
> + >mode_1_exec && chmod +x mode_1_exec &&
Ditto.
> + git status -s ":(exclude,attr:builtin_objectmode=100644)" "mode_1_*" >actual &&
> + echo ?? mode_1_exec >expect &&
> + test_cmp expect actual &&
> +
> + git status -s ":(exclude,attr:builtin_objectmode=100755)" "mode_1_*" >actual &&
> + echo ?? mode_1_regular >expect &&
> + test_cmp expect actual
> +'
> +
> test_done
^ permalink raw reply
* Re: Git Rename Detection Bug
From: Elijah Newren @ 2023-11-16 6:26 UTC (permalink / raw)
To: Philip Oakley; +Cc: Jeremy Pridmore, git@vger.kernel.org, Paul Baumgartner
In-Reply-To: <781fc667-6597-4327-80d5-721fb273d2e2@iee.email>
Hi Philip,
On Wed, Nov 15, 2023 at 6:36 AM Philip Oakley <philipoakley@iee.email> wrote:
>
> Hi Elijah,
> sorry for the delay in replying.
It was only a few days; no need to apologize.
>
> On 11/11/2023 15:13, Elijah Newren wrote:
> > Hi,
> >
> > On Sat, Nov 11, 2023 at 3:08 AM Philip Oakley <philipoakley@iee.email> wrote:
> >>
> >> Hi all,
> >>
> >> On 11/11/2023 05:46, Elijah Newren wrote:
> >>> The fact that you were trying to "undo" renames and "redo the correct
> >>> ones" suggested there's something you still didn't understand about
> >>> rename detection, though.
> >>
> >>
> >> Could I suggest that we are missing a piece of terminology, to wit,
> >> BLOBSAME. It's a compatriot to TREESAME, as used in `git log` for
> >> history simplification (based on a tree's pathspec, most commonly a
> >> commit's top level path).
> >
> > We could add it, but I'm not sure how it helps. We already had 'exact
> > rename' which seems to fit the bill as well,
>
> My point was that we already had the confusion of mental models, with
> both sides essentially thinking they had an "exact rename", hence my
> thought was to add a rather distinct technical name which reflected the
> Git mind-shift. Without something to bring folks up short they'll
> continue, erroneously, with their prior mental models.
Maybe I'm missing the other mental model you're alluding to? The
mental model problems I suspected Jeremy had did not appear to hinge
on exact renames; the fact that Jeremy was trying to "undo" renames
and "redo the correct ones" suggested to me that it's an issue with
understanding detecting vs. tracking renames, something for which it
doesn't matter whether the renames are exact or inexact. (Also, the
fact that he thought it was a bug that the detected renames could
change in `git status` output by merely editing files, suggested also
a detected vs. tracked rename misunderstanding -- and in that case,
there's virtually no chance that an explanation of exact renames or
BLOBSAME or whatever could help since the editing of files in question
means that they aren't going to have the same contents anymore.)
> and 'blob' is something
> > someone new to Git is unlikely to know.
>
> I'd agree that BLOBSAME is new, but we should be proactive in ensuring
> folk do have the mind shift from the old centralised VCS misunderstandings.
I agree it's good to help folks make the mind shift, I'm just not
following how BLOBSAME or any other alternative for "exact rename"
could help in this particular case.
Maybe it'd help to understand why I brought up "exact renames" in the
first place? When most people see "rename detection bug" (as in the
subject of this thread), they're likely to assume some heuristic went
haywire. When Jeremy pointed out that the content of the "mismatched"
file was identical to the source file of interest, I thought it worth
pointing out so anyone else watching the thread would notice. It
means all the normal heuristics involved in inexact renames cannot be
the source of this particular problem. And, in particular, that the
changes that ort brought aren't relevant to this particular problem
either.
> > Perhaps it's useful in some other context, though?
> >
> >> File rename, at it's most basic, is when the blob associated with that
> >> changed path is identical, i.e. BLOBSAME. There is no need to 'record'
> >> the action of renaming, moving or whatever, the content sameness is
> >> right there, in plain sight, as an identical blob name. After that
> >> (files with slight variations) it is a load of heuristics, but starting
> >> with BLOBSAME we see how easy the basic rename detection is, and why
> >> renames (and de-dup) don't need recording.
> >
> > This is incorrect. Let's say you have a file foo:
> > * base version: foo has hash A
> > * our version: foo has been renamed to bar, but bar still has hash A
> > * their version: foo has been modified; it now has hash B
> >
> > The foo->bar is an exact rename (or they are BLOBSAME if you prefer),
> > but the renaming/moving/whatever is a critical piece of information
> > because the changes to foo in 'their' version need to be applied to
> > bar to get the correct end results.
>
> Isn't that what I thought I'd said?
> Hash A = Hash A => identical content;
> Hash A != B => different content.
My apologies, I misread what you had written. I had somehow read you
as saying that the rename wasn't needed, but you didn't say that at
all.
Now that I've re-read what you wrote, and hopefully understand better,
I did notice something else that might be worth responding to, though.
Let me re-quote a piece of what you said and respond to it:
> >> ...the content sameness is
> >> right there, in plain sight, as an identical blob name. After that
> >> (files with slight variations) it is a load of heuristics, ...
This suggests there aren't heuristics involved when we have identical
blob names. That's not quite accurate, though. When there are
multiple identical blob names that a given source file could be paired
with (e.g. old/foo.txt deleted, and all three of A/foo.txt and
B/bar.txt and C/foo.txt all added on the same side of history and all
with identical contents to the old/foo.txt), we give a higher score to
files that share the same basename (so A/foo.txt and C/foo.txt would
be picked as rename targets over B/bar.txt). If we have multiple
files with an identical blob name and the same file basename (i.e.
A/foo.txt and C/foo.txt in this case), then we pick one, basically at
random as far as the user is concerned (so old/foo.txt could be a
rename to either A/foo.txt or C/foo.txt, just depending on processing
order).
> > I do not know if in Jeremy's case foo has been modified on the
> > unrenamed side. But the following hypothetical is exactly the type of
> > problem Jeremy is hitting: what should happen when 'our' version has
> > both a new 'bar' and a new 'baz' file that each have hash A? In that
> > case, to which one was foo renamed? It's inherently ambiguous.
>
> true, the terminology hasn't kept up with the methodology for blob
> content, and the independent meta-data. In previous 'ort' discussions I
> didn't really understand what the '1/2' renames (and other
> nomenclatures) really meant with respect to paths, filenames, content
> and the ours / theirs / base distinctions.
None of that other nomenclature in 'ort' really matters; the problem
presented is almost certainly unrelated to any changes that came with
'ort'. If he had switched to git anytime in the last 15 years he'd
probably see exactly the same problem.
I'm guessing by "'1/2' renames" that you're referring to what I termed
"rename/rename(1to2)" cases, which exist in both merge-recursive and
merge-ort. Those are cases where the base version had a file named A,
and one side of history renamed A->B, while the other side of history
renamed A->C. That kind of situation is not relevant to the current
problem; for the current problem we have a file named D that was
renamed on only one side, but there are both files E and F each with
contents identical to D and rename detection has to decide whether D
was renamed to E or renamed to F on that side.
(This contrasts with "rename/rename(2to1)" cases, where the base
version has both files G & H, and then one side of history renames
G->I and the other side renames H->I. Or in short, 2to1 means two
files are renamed to one file, and 1to2 means one file is renamed to
two.)
> >> The heuristics of 'rename with small change' is trickier, but for a
> >> basic understanding, starting at BLOBSAME (and TREESAME for directory
> >> renames) should make it easier to grasp the concepts.
> >
> > Interesting; TREESAME isn't used within directory rename detection
> > currently; it is only used currently when two (or three) trees with
> > the same name are TREESAME, in order to potentially avoid recursing
> > into the tree. But even then, having two trees with the same name be
> > TREESAME isn't enough on its own to avoid recursing into that tree,
> > because the other side could have added files within the same-named
> > tree and we need to know about those added files because they could be
> > part of renames involving other files outside that tree.
>
> definitely easy to get confused on these cases...
yeah, and apparently I shouldn't have used TREESAME this way as per
Junio's comment elsewhere in the thread. Oops.
> Thanks for the insights.
Thanks for your comments, and again, my apologies for misreading what
you wrote the first time. I hope I didn't do it again.
^ permalink raw reply
* Re: [PATCH 1/1] attr: add builtin objectmode values support
From: Junio C Hamano @ 2023-11-16 6:17 UTC (permalink / raw)
To: Joanna Wang; +Cc: git, sunshine, tboegi
In-Reply-To: <20231116054437.2343549-1-jojwang@google.com>
Joanna Wang <jojwang@google.com> writes:
> t/#t1700-split-index.sh# | 547 ++++++++++++++++++++++++++++++++
This was added by mistake and we can safely ignore it? IOW, is
everything else in the patch, other than the addition of this path,
what you intended to send out?
Other than the removal of that file, I locally applied the following
fix-up while checking the difference relative to the previous
iteration. Other than these, the updated version looked very clear
and nicely done.
Thanks.
t/t0003-attributes.sh | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/t/t0003-attributes.sh b/t/t0003-attributes.sh
index 86f8681570..774b52c298 100755
--- a/t/t0003-attributes.sh
+++ b/t/t0003-attributes.sh
@@ -580,12 +580,13 @@ test_expect_success 'builtin object mode attributes work (dir and regular paths)
'
test_expect_success POSIXPERM 'builtin object mode attributes work (executable)' '
- >exec && chmod +x exec &&
+ >exec &&
+ chmod +x exec &&
attr_check_object_mode exec 100755
'
test_expect_success SYMLINKS 'builtin object mode attributes work (symlinks)' '
- >to_sym ln -s to_sym sym &&
+ ln -s to_sym sym &&
attr_check_object_mode sym 120000
'
^ permalink raw reply related
* Re: [PATCH v2 09/10] ref-filter.c: use peeled tag for '*' format fields
From: Junio C Hamano @ 2023-11-16 5:48 UTC (permalink / raw)
To: Victoria Dye via GitGitGadget
Cc: git, Patrick Steinhardt, Øystein Walle, Kristoffer Haugsbakk,
Victoria Dye
In-Reply-To: <48254d8e161de7f0e165510c06801195f9b0a8fd.1699991638.git.gitgitgadget@gmail.com>
"Victoria Dye via GitGitGadget" <gitgitgadget@gmail.com> writes:
> From: Victoria Dye <vdye@github.com>
>
> In most builtins ('rev-parse <revision>^{}', 'show-ref --dereference'),
> "dereferencing" a tag refers to a recursive peel of the tag object. Unlike
> these cases, the dereferencing prefix ('*') in 'for-each-ref' format
> specifiers triggers only a single, non-recursive dereference of a given tag
> object. For most annotated tags, a single dereference is all that is needed
> to access the tag's associated commit or tree; "recursive" and
> "non-recursive" dereferencing are functionally equivalent in these cases.
> However, nested tags (annotated tags whose target is another annotated tag)
> dereferenced once return another tag, where a recursive dereference would
> return the commit or tree.
This may be the only potentially controversial step in the series.
> - /*
> - * NEEDSWORK: This derefs tag only once, which
> - * is good to deal with chains of trust, but
> - * is not consistent with what deref_tag() does
> - * which peels the onion to the core.
> - */
> return get_object(ref, 1, &obj, &oi_deref, err);
> }
Very nice to see an ancient comment I added at 9f613ddd (Add
git-for-each-ref: helper for language bindings, 2006-09-15) finally
go.
Thanks.
^ permalink raw reply
* [PATCH 1/1] attr: add builtin objectmode values support
From: Joanna Wang @ 2023-11-16 5:44 UTC (permalink / raw)
To: gitster; +Cc: git, jojwang, sunshine, tboegi
In-Reply-To: <xmqqttpmtnn5.fsf@gitster.g>
Gives all paths builtin objectmode values based on the paths' modes
(one of 100644, 100755, 120000, 040000, 160000). Users may use
this feature to filter by file types. For example a pathspec such as
':(attr:builtin_objectmode=160000)' could filter for submodules without
needing to have `builtin_objectmode=160000` to be set in .gitattributes
for every submodule path.
These values are also reflected in `git check-attr` results.
If the git_attr_direction is set to GIT_ATTR_INDEX or GIT_ATTR_CHECKIN
and a path is not found in the index, the value will be unspecified.
This patch also reserves the builtin_* attribute namespace for objectmode
and any future builtin attributes. Any user defined attributes using this
reserved namespace will result in a warning. This is a breaking change for
any existing builtin_* attributes.
Pathspecs with some builtin_* attribute name (excluding builtin_objectmode)
will behave like any attribute where there are no user specified values.
Signed-off-by: Joanna Wang <jojwang@google.com>
---
>The idiom is to see if it is still NULL and then call git_attr().
Got it, thank you for explaining again.
>We need a precondition here:
>test_expect_success SYMLINKS
Torsten pointed this out in a previous version,
sorry i missed this until Junio pointed it out again.
I've addressed the all the latest feedback in this update.
Documentation/gitattributes.txt | 15 +
attr.c | 71 ++++-
t/#t1700-split-index.sh# | 547 ++++++++++++++++++++++++++++++++
t/t0003-attributes.sh | 75 +++++
t/t6135-pathspec-with-attrs.sh | 25 ++
5 files changed, 730 insertions(+), 3 deletions(-)
create mode 100755 t/#t1700-split-index.sh#
diff --git a/Documentation/gitattributes.txt b/Documentation/gitattributes.txt
index 8c1793c148..784aa9d4de 100644
--- a/Documentation/gitattributes.txt
+++ b/Documentation/gitattributes.txt
@@ -100,6 +100,21 @@ for a path to `Unspecified` state. This can be done by listing
the name of the attribute prefixed with an exclamation point `!`.
+RESERVED BUILTIN_* ATTRIBUTES
+-----------------------------
+
+builtin_* is a reserved namespace for builtin attribute values. Any
+user defined attributes under this namespace will be ignored and
+trigger a warning.
+
+`builtin_objectmode`
+^^^^^^^^^^^^^^^^^^^^
+This attribute is for filtering files by their file bit modes (40000,
+120000, 160000, 100755, 100644). e.g. ':(attr:builtin_objectmode=160000)'.
+You may also check these values with `git check-attr builtin_objectmode -- <file>`.
+If the object is not in the index `git check-attr --cached` will return unspecified.
+
+
EFFECTS
-------
diff --git a/attr.c b/attr.c
index e62876dfd3..f8a94aeb23 100644
--- a/attr.c
+++ b/attr.c
@@ -17,6 +17,7 @@
#include "utf8.h"
#include "quote.h"
#include "read-cache-ll.h"
+#include "refs.h"
#include "revision.h"
#include "object-store-ll.h"
#include "setup.h"
@@ -183,6 +184,15 @@ static void all_attrs_init(struct attr_hashmap *map, struct attr_check *check)
}
}
+/*
+ * Atribute name cannot begin with "builtin_" which
+ * is a reserved namespace for built in attributes values.
+ */
+static int attr_name_reserved(const char *name)
+{
+ return starts_with(name, "builtin_");
+}
+
static int attr_name_valid(const char *name, size_t namelen)
{
/*
@@ -315,7 +325,7 @@ static const char *parse_attr(const char *src, int lineno, const char *cp,
cp++;
len--;
}
- if (!attr_name_valid(cp, len)) {
+ if (!attr_name_valid(cp, len) || attr_name_reserved(cp)) {
report_invalid_attr(cp, len, src, lineno);
return NULL;
}
@@ -379,7 +389,7 @@ static struct match_attr *parse_attr_line(const char *line, const char *src,
name += strlen(ATTRIBUTE_MACRO_PREFIX);
name += strspn(name, blank);
namelen = strcspn(name, blank);
- if (!attr_name_valid(name, namelen)) {
+ if (!attr_name_valid(name, namelen) || attr_name_reserved(name)) {
report_invalid_attr(name, namelen, src, lineno);
goto fail_return;
}
@@ -1240,6 +1250,61 @@ static struct object_id *default_attr_source(void)
return &attr_source;
}
+static const char *builtin_object_mode_attr(struct index_state *istate, const char *path)
+{
+ unsigned int mode;
+ struct strbuf sb = STRBUF_INIT;
+
+ if (direction == GIT_ATTR_CHECKIN) {
+ struct object_id oid;
+ struct stat st;
+ if (lstat(path, &st))
+ die_errno(_("unable to stat '%s'"), path);
+ mode = canon_mode(st.st_mode);
+ if (S_ISDIR(mode)) {
+ /*
+ *`path` is either a directory or it is a submodule,
+ * in which case it is already indexed as submodule
+ * or it does not exist in the index yet and we need to
+ * check if we can resolve to a ref.
+ */
+ int pos = index_name_pos(istate, path, strlen(path));
+ if (pos >= 0) {
+ if (S_ISGITLINK(istate->cache[pos]->ce_mode))
+ mode = istate->cache[pos]->ce_mode;
+ } else if (resolve_gitlink_ref(path, "HEAD", &oid) == 0) {
+ mode = S_IFGITLINK;
+ }
+ }
+ } else {
+ /*
+ * For GIT_ATTR_CHECKOUT and GIT_ATTR_INDEX we only check
+ * for mode in the index.
+ */
+ int pos = index_name_pos(istate, path, strlen(path));
+ if (pos >= 0)
+ mode = istate->cache[pos]->ce_mode;
+ else
+ return ATTR__UNSET;
+ }
+ strbuf_addf(&sb, "%06o", mode);
+ return strbuf_detach(&sb, NULL);
+}
+
+
+static const char *compute_builtin_attr(struct index_state *istate,
+ const char *path,
+ const struct git_attr *attr) {
+ static const struct git_attr *object_mode_attr;
+
+ if (!object_mode_attr)
+ object_mode_attr = git_attr("builtin_objectmode");
+
+ if (attr == object_mode_attr)
+ return builtin_object_mode_attr(istate, path);
+ return ATTR__UNSET;
+}
+
void git_check_attr(struct index_state *istate,
const char *path,
struct attr_check *check)
@@ -1253,7 +1318,7 @@ void git_check_attr(struct index_state *istate,
unsigned int n = check->items[i].attr->attr_nr;
const char *value = check->all_attrs[n].value;
if (value == ATTR__UNKNOWN)
- value = ATTR__UNSET;
+ value = compute_builtin_attr(istate, path, check->all_attrs[n].attr);
check->items[i].value = value;
}
}
diff --git a/t/#t1700-split-index.sh# b/t/#t1700-split-index.sh#
new file mode 100755
index 0000000000..a7b7263b35
--- /dev/null
+++ b/t/#t1700-split-index.sh#
@@ -0,0 +1,547 @@
+#!/bin/sh
+
+test_description='split index mode tests'
+
+GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
+export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
+
+. ./test-lib.sh
+
+# We need total control of index splitting here
+sane_unset GIT_TEST_SPLIT_INDEX
+
+# Testing a hard coded SHA against an index with an extension
+# that can vary from run to run is problematic so we disable
+# those extensions.
+sane_unset GIT_TEST_FSMONITOR
+sane_unset GIT_TEST_INDEX_THREADS
+
+# Create a file named as $1 with content read from stdin.
+# Set the file's mtime to a few seconds in the past to avoid racy situations.
+create_non_racy_file () {
+ cat >"$1" &&
+ test-tool chmtime =-5 "$1"
+}
+
+test_expect_success 'setup' '
+ test_oid_cache <<-EOF
+ own_v3 sha1:8299b0bcd1ac364e5f1d7768efb62fa2da79a339
+ own_v3 sha256:38a6d2925e3eceec33ad7b34cbff4e0086caa0daf28f31e51f5bd94b4a7af86b
+
+ base_v3 sha1:39d890139ee5356c7ef572216cebcd27aa41f9df
+ base_v3 sha256:c9baeadf905112bf6c17aefbd7d02267afd70ded613c30cafed2d40cb506e1ed
+
+ own_v4 sha1:432ef4b63f32193984f339431fd50ca796493569
+ own_v4 sha256:6738ac6319c25b694afa7bcc313deb182d1a59b68bf7a47b4296de83478c0420
+
+ base_v4 sha1:508851a7f0dfa8691e9f69c7f055865389012491
+ base_v4 sha256:3177d4adfdd4b6904f7e921d91d715a471c0dde7cf6a4bba574927f02b699508
+ EOF
+'
+
+test_expect_success 'enable split index' '
+ git config splitIndex.maxPercentChange 100 &&
+ git update-index --split-index &&
+ test-tool dump-split-index .git/index >actual &&
+ indexversion=$(git update-index --show-index-version) &&
+
+ # NEEDSWORK: Stop hard-coding checksums.
+ if test "$indexversion" = "4"
+ then
+ own=$(test_oid own_v4) &&
+ base=$(test_oid base_v4)
+ else
+ own=$(test_oid own_v3) &&
+ base=$(test_oid base_v3)
+ fi &&
+
+ cat >expect <<-EOF &&
+ own $own
+ base $base
+ replacements:
+ deletions:
+ EOF
+ test_cmp expect actual
+'
+
+test_expect_success 'add one file' '
+ create_non_racy_file one &&
+ git update-index --add one &&
+ git ls-files --stage >ls-files.actual &&
+ cat >ls-files.expect <<-EOF &&
+ 100644 $EMPTY_BLOB 0 one
+ EOF
+ test_cmp ls-files.expect ls-files.actual &&
+
+ test-tool dump-split-index .git/index | sed "/^own/d" >actual &&
+ cat >expect <<-EOF &&
+ base $base
+ 100644 $EMPTY_BLOB 0 one
+ replacements:
+ deletions:
+ EOF
+ test_cmp expect actual
+'
+
+test_expect_success 'disable split index' '
+ git update-index --no-split-index &&
+ git ls-files --stage >ls-files.actual &&
+ cat >ls-files.expect <<-EOF &&
+ 100644 $EMPTY_BLOB 0 one
+ EOF
+ test_cmp ls-files.expect ls-files.actual &&
+
+ BASE=$(test-tool dump-split-index .git/index | sed -n "s/^own/base/p") &&
+ test-tool dump-split-index .git/index | sed "/^own/d" >actual &&
+ cat >expect <<-EOF &&
+ not a split index
+ EOF
+ test_cmp expect actual
+'
+
+test_expect_success 'enable split index again, "one" now belongs to base index"' '
+ git update-index --split-index &&
+ git ls-files --stage >ls-files.actual &&
+ cat >ls-files.expect <<-EOF &&
+ 100644 $EMPTY_BLOB 0 one
+ EOF
+ test_cmp ls-files.expect ls-files.actual &&
+
+ test-tool dump-split-index .git/index | sed "/^own/d" >actual &&
+ cat >expect <<-EOF &&
+ $BASE
+ replacements:
+ deletions:
+ EOF
+ test_cmp expect actual
+'
+
+test_expect_success 'modify original file, base index untouched' '
+ echo modified | create_non_racy_file one &&
+ file1_blob=$(git hash-object one) &&
+ git update-index one &&
+ git ls-files --stage >ls-files.actual &&
+ cat >ls-files.expect <<-EOF &&
+ 100644 $file1_blob 0 one
+ EOF
+ test_cmp ls-files.expect ls-files.actual &&
+
+ test-tool dump-split-index .git/index | sed "/^own/d" >actual &&
+ q_to_tab >expect <<-EOF &&
+ $BASE
+ 100644 $file1_blob 0Q
+ replacements: 0
+ deletions:
+ EOF
+ test_cmp expect actual
+'
+
+test_expect_success 'add another file, which stays index' '
+ create_non_racy_file two &&
+ git update-index --add two &&
+ git ls-files --stage >ls-files.actual &&
+ cat >ls-files.expect <<-EOF &&
+ 100644 $file1_blob 0 one
+ 100644 $EMPTY_BLOB 0 two
+ EOF
+ test_cmp ls-files.expect ls-files.actual &&
+
+ test-tool dump-split-index .git/index | sed "/^own/d" >actual &&
+ q_to_tab >expect <<-EOF &&
+ $BASE
+ 100644 $file1_blob 0Q
+ 100644 $EMPTY_BLOB 0 two
+ replacements: 0
+ deletions:
+ EOF
+ test_cmp expect actual
+'
+
+test_expect_success 'remove file not in base index' '
+ git update-index --force-remove two &&
+ git ls-files --stage >ls-files.actual &&
+ cat >ls-files.expect <<-EOF &&
+ 100644 $file1_blob 0 one
+ EOF
+ test_cmp ls-files.expect ls-files.actual &&
+
+ test-tool dump-split-index .git/index | sed "/^own/d" >actual &&
+ q_to_tab >expect <<-EOF &&
+ $BASE
+ 100644 $file1_blob 0Q
+ replacements: 0
+ deletions:
+ EOF
+ test_cmp expect actual
+'
+
+test_expect_success 'remove file in base index' '
+ git update-index --force-remove one &&
+ git ls-files --stage >ls-files.actual &&
+ test_must_be_empty ls-files.actual &&
+
+ test-tool dump-split-index .git/index | sed "/^own/d" >actual &&
+ cat >expect <<-EOF &&
+ $BASE
+ replacements:
+ deletions: 0
+ EOF
+ test_cmp expect actual
+'
+
+test_expect_success 'add original file back' '
+ create_non_racy_file one &&
+ git update-index --add one &&
+ git ls-files --stage >ls-files.actual &&
+ cat >ls-files.expect <<-EOF &&
+ 100644 $EMPTY_BLOB 0 one
+ EOF
+ test_cmp ls-files.expect ls-files.actual &&
+
+ test-tool dump-split-index .git/index | sed "/^own/d" >actual &&
+ cat >expect <<-EOF &&
+ $BASE
+ 100644 $EMPTY_BLOB 0 one
+ replacements:
+ deletions: 0
+ EOF
+ test_cmp expect actual
+'
+
+test_expect_success 'add new file' '
+ create_non_racy_file two &&
+ git update-index --add two &&
+ git ls-files --stage >actual &&
+ cat >expect <<-EOF &&
+ 100644 $EMPTY_BLOB 0 one
+ 100644 $EMPTY_BLOB 0 two
+ EOF
+ test_cmp expect actual
+'
+
+test_expect_success 'unify index, two files remain' '
+ git update-index --no-split-index &&
+ git ls-files --stage >ls-files.actual &&
+ cat >ls-files.expect <<-EOF &&
+ 100644 $EMPTY_BLOB 0 one
+ 100644 $EMPTY_BLOB 0 two
+ EOF
+ test_cmp ls-files.expect ls-files.actual &&
+
+ test-tool dump-split-index .git/index | sed "/^own/d" >actual &&
+ cat >expect <<-EOF &&
+ not a split index
+ EOF
+ test_cmp expect actual
+'
+
+test_expect_success 'rev-parse --shared-index-path' '
+ test_create_repo split-index &&
+ (
+ cd split-index &&
+ git update-index --split-index &&
+ echo .git/sharedindex* >expect &&
+ git rev-parse --shared-index-path >actual &&
+ test_cmp expect actual &&
+ mkdir subdirectory &&
+ cd subdirectory &&
+ echo ../.git/sharedindex* >expect &&
+ git rev-parse --shared-index-path >actual &&
+ test_cmp expect actual
+ )
+'
+
+test_expect_success 'set core.splitIndex config variable to true' '
+ git config core.splitIndex true &&
+ create_non_racy_file three &&
+ git update-index --add three &&
+ git ls-files --stage >ls-files.actual &&
+ cat >ls-files.expect <<-EOF &&
+ 100644 $EMPTY_BLOB 0 one
+ 100644 $EMPTY_BLOB 0 three
+ 100644 $EMPTY_BLOB 0 two
+ EOF
+ test_cmp ls-files.expect ls-files.actual &&
+ BASE=$(test-tool dump-split-index .git/index | grep "^base") &&
+ test-tool dump-split-index .git/index | sed "/^own/d" >actual &&
+ cat >expect <<-EOF &&
+ $BASE
+ replacements:
+ deletions:
+ EOF
+ test_cmp expect actual
+'
+
+test_expect_success 'set core.splitIndex config variable to false' '
+ git config core.splitIndex false &&
+ git update-index --force-remove three &&
+ git ls-files --stage >ls-files.actual &&
+ cat >ls-files.expect <<-EOF &&
+ 100644 $EMPTY_BLOB 0 one
+ 100644 $EMPTY_BLOB 0 two
+ EOF
+ test_cmp ls-files.expect ls-files.actual &&
+ test-tool dump-split-index .git/index | sed "/^own/d" >actual &&
+ cat >expect <<-EOF &&
+ not a split index
+ EOF
+ test_cmp expect actual
+'
+
+test_expect_success 'set core.splitIndex config variable back to true' '
+ git config core.splitIndex true &&
+ create_non_racy_file three &&
+ git update-index --add three &&
+ BASE=$(test-tool dump-split-index .git/index | grep "^base") &&
+ test-tool dump-split-index .git/index | sed "/^own/d" >actual &&
+ cat >expect <<-EOF &&
+ $BASE
+ replacements:
+ deletions:
+ EOF
+ test_cmp expect actual &&
+ create_non_racy_file four &&
+ git update-index --add four &&
+ test-tool dump-split-index .git/index | sed "/^own/d" >actual &&
+ cat >expect <<-EOF &&
+ $BASE
+ 100644 $EMPTY_BLOB 0 four
+ replacements:
+ deletions:
+ EOF
+ test_cmp expect actual
+'
+
+test_expect_success 'check behavior with splitIndex.maxPercentChange unset' '
+ git config --unset splitIndex.maxPercentChange &&
+ create_non_racy_file five &&
+ git update-index --add five &&
+ BASE=$(test-tool dump-split-index .git/index | grep "^base") &&
+ test-tool dump-split-index .git/index | sed "/^own/d" >actual &&
+ cat >expect <<-EOF &&
+ $BASE
+ replacements:
+ deletions:
+ EOF
+ test_cmp expect actual &&
+ create_non_racy_file six &&
+ git update-index --add six &&
+ test-tool dump-split-index .git/index | sed "/^own/d" >actual &&
+ cat >expect <<-EOF &&
+ $BASE
+ 100644 $EMPTY_BLOB 0 six
+ replacements:
+ deletions:
+ EOF
+ test_cmp expect actual
+'
+
+test_expect_success 'check splitIndex.maxPercentChange set to 0' '
+ git config splitIndex.maxPercentChange 0 &&
+ create_non_racy_file seven &&
+ git update-index --add seven &&
+ BASE=$(test-tool dump-split-index .git/index | grep "^base") &&
+ test-tool dump-split-index .git/index | sed "/^own/d" >actual &&
+ cat >expect <<-EOF &&
+ $BASE
+ replacements:
+ deletions:
+ EOF
+ test_cmp expect actual &&
+ create_non_racy_file eight &&
+ git update-index --add eight &&
+ BASE=$(test-tool dump-split-index .git/index | grep "^base") &&
+ test-tool dump-split-index .git/index | sed "/^own/d" >actual &&
+ cat >expect <<-EOF &&
+ $BASE
+ replacements:
+ deletions:
+ EOF
+ test_cmp expect actual
+'
+
+test_expect_success 'shared index files expire after 2 weeks by default' '
+ create_non_racy_file ten &&
+ git update-index --add ten &&
+ test $(ls .git/sharedindex.* | wc -l) -gt 2 &&
+ just_under_2_weeks_ago=$((5-14*86400)) &&
+ test-tool chmtime =$just_under_2_weeks_ago .git/sharedindex.* &&
+ create_non_racy_file eleven &&
+ git update-index --add eleven &&
+ test $(ls .git/sharedindex.* | wc -l) -gt 2 &&
+ just_over_2_weeks_ago=$((-1-14*86400)) &&
+ test-tool chmtime =$just_over_2_weeks_ago .git/sharedindex.* &&
+ create_non_racy_file twelve &&
+ git update-index --add twelve &&
+ test $(ls .git/sharedindex.* | wc -l) -le 2
+'
+
+test_expect_success 'check splitIndex.sharedIndexExpire set to 16 days' '
+ git config splitIndex.sharedIndexExpire "16.days.ago" &&
+ test-tool chmtime =$just_over_2_weeks_ago .git/sharedindex.* &&
+ create_non_racy_file thirteen &&
+ git update-index --add thirteen &&
+ test $(ls .git/sharedindex.* | wc -l) -gt 2 &&
+ just_over_16_days_ago=$((-1-16*86400)) &&
+ test-tool chmtime =$just_over_16_days_ago .git/sharedindex.* &&
+ create_non_racy_file fourteen &&
+ git update-index --add fourteen &&
+ test $(ls .git/sharedindex.* | wc -l) -le 2
+'
+
+test_expect_success 'check splitIndex.sharedIndexExpire set to "never" and "now"' '
+ git config splitIndex.sharedIndexExpire never &&
+ just_10_years_ago=$((-365*10*86400)) &&
+ test-tool chmtime =$just_10_years_ago .git/sharedindex.* &&
+ create_non_racy_file fifteen &&
+ git update-index --add fifteen &&
+ test $(ls .git/sharedindex.* | wc -l) -gt 2 &&
+ git config splitIndex.sharedIndexExpire now &&
+ just_1_second_ago=-1 &&
+ test-tool chmtime =$just_1_second_ago .git/sharedindex.* &&
+ create_non_racy_file sixteen &&
+ git update-index --add sixteen &&
+ test $(ls .git/sharedindex.* | wc -l) -le 2
+'
+
+test_expect_success POSIXPERM 'same mode for index & split index' '
+ git init same-mode &&
+ (
+ cd same-mode &&
+ test_commit A &&
+ test_modebits .git/index >index_mode &&
+ test_must_fail git config core.sharedRepository &&
+ git -c core.splitIndex=true status &&
+ shared=$(ls .git/sharedindex.*) &&
+ case "$shared" in
+ *" "*)
+ # we have more than one???
+ false ;;
+ *)
+ test_modebits "$shared" >split_index_mode &&
+ test_cmp index_mode split_index_mode ;;
+ esac
+ )
+'
+
+while read -r mode modebits
+do
+ test_expect_success POSIXPERM "split index respects core.sharedrepository $mode" '
+ # Remove existing shared index files
+ git config core.splitIndex false &&
+ git update-index --force-remove one &&
+ rm -f .git/sharedindex.* &&
+ # Create one new shared index file
+ git config core.sharedrepository "$mode" &&
+ git config core.splitIndex true &&
+ create_non_racy_file one &&
+ git update-index --add one &&
+ echo "$modebits" >expect &&
+ test_modebits .git/index >actual &&
+ test_cmp expect actual &&
+ shared=$(ls .git/sharedindex.*) &&
+ case "$shared" in
+ *" "*)
+ # we have more than one???
+ false ;;
+ *)
+ test_modebits "$shared" >actual &&
+ test_cmp expect actual ;;
+ esac
+ '
+done <<\EOF
+0666 -rw-rw-rw-
+0642 -rw-r---w-
+EOF
+
+test_expect_success POSIXPERM,SANITY 'graceful handling when splitting index is not allowed' '
+ test_create_repo ro &&
+ (
+ cd ro &&
+ test_commit initial &&
+ git update-index --split-index &&
+ test -f .git/sharedindex.*
+ ) &&
+ cp ro/.git/index new-index &&
+ test_when_finished "chmod u+w ro/.git" &&
+ chmod u-w ro/.git &&
+ GIT_INDEX_FILE="$(pwd)/new-index" git -C ro update-index --split-index &&
+ chmod u+w ro/.git &&
+ rm ro/.git/sharedindex.* &&
+ GIT_INDEX_FILE=new-index git ls-files >actual &&
+ echo initial.t >expected &&
+ test_cmp expected actual
+'
+
+test_expect_success 'writing split index with null sha1 does not write cache tree' '
+ git config core.splitIndex true &&
+ git config splitIndex.maxPercentChange 0 &&
+ git commit -m "commit" &&
+ {
+ git ls-tree HEAD &&
+ printf "160000 commit $ZERO_OID\\tbroken\\n"
+ } >broken-tree &&
+ echo "add broken entry" >msg &&
+
+ tree=$(git mktree <broken-tree) &&
+ test_tick &&
+ commit=$(git commit-tree $tree -p HEAD <msg) &&
+ git update-ref HEAD "$commit" &&
+ GIT_ALLOW_NULL_SHA1=1 git reset --hard &&
+ test_might_fail test-tool dump-cache-tree >cache-tree.out &&
+ test_line_count = 0 cache-tree.out
+'
+
+test_expect_success 'do not refresh null base index' '
+ test_create_repo merge &&
+ (
+ cd merge &&
+ test_commit initial &&
+ git checkout -b side-branch &&
+ test_commit extra &&
+ git checkout main &&
+ git update-index --split-index &&
+ test_commit more &&
+ # must not write a new shareindex, or we wont catch the problem
+ git -c splitIndex.maxPercentChange=100 merge --no-edit side-branch 2>err &&
+ # i.e. do not expect warnings like
+ # could not freshen shared index .../shareindex.00000...
+ test_must_be_empty err
+ )
+'
+
+test_expect_success 'reading split index at alternate location' '
+ git init reading-alternate-location &&
+ (
+ cd reading-alternate-location &&
+ >file-in-alternate &&
+ git update-index --split-index --add file-in-alternate
+ ) &&
+ echo file-in-alternate >expect &&
+
+ # Should be able to find the shared index both right next to
+ # the specified split index file ...
+ GIT_INDEX_FILE=./reading-alternate-location/.git/index \
+ git ls-files --cached >actual &&
+ test_cmp expect actual &&
+
+ # ... and, for backwards compatibility, in the current GIT_DIR
+ # as well.
+ mv -v ./reading-alternate-location/.git/sharedindex.* .git &&
+ GIT_INDEX_FILE=./reading-alternate-location/.git/index \
+ git ls-files --cached >actual &&
+ test_cmp expect actual
+'
+
+test_expect_success 'GIT_TEST_SPLIT_INDEX works' '
+ git init git-test-split-index &&
+ (
+ cd git-test-split-index &&
+ >file &&
+ GIT_TEST_SPLIT_INDEX=1 git update-index --add file &&
+ ls -l .git/sharedindex.* >actual &&
+ test_line_count = 1 actual
+ )
+'
+
+test_done
diff --git a/t/t0003-attributes.sh b/t/t0003-attributes.sh
index aee2298f01..86f8681570 100755
--- a/t/t0003-attributes.sh
+++ b/t/t0003-attributes.sh
@@ -19,6 +19,20 @@ attr_check () {
test_must_be_empty err
}
+attr_check_object_mode_basic () {
+ path="$1" &&
+ expect="$2" &&
+ check_opts="$3" &&
+ git check-attr $check_opts builtin_objectmode -- "$path" >actual 2>err &&
+ echo "$path: builtin_objectmode: $expect" >expect &&
+ test_cmp expect actual
+}
+
+attr_check_object_mode () {
+ attr_check_object_mode_basic "$@" &&
+ test_must_be_empty err
+}
+
attr_check_quote () {
path="$1" quoted_path="$2" expect="$3" &&
@@ -558,4 +572,65 @@ test_expect_success EXPENSIVE 'large attributes file ignored in index' '
test_cmp expect err
'
+test_expect_success 'builtin object mode attributes work (dir and regular paths)' '
+ >normal &&
+ attr_check_object_mode normal 100644 &&
+ mkdir dir &&
+ attr_check_object_mode dir 040000
+'
+
+test_expect_success POSIXPERM 'builtin object mode attributes work (executable)' '
+ >exec && chmod +x exec &&
+ attr_check_object_mode exec 100755
+'
+
+test_expect_success SYMLINKS 'builtin object mode attributes work (symlinks)' '
+ >to_sym ln -s to_sym sym &&
+ attr_check_object_mode sym 120000
+'
+
+test_expect_success 'native object mode attributes work with --cached' '
+ >normal &&
+ git add normal &&
+ empty_blob=$(git rev-parse :normal) &&
+ git update-index --index-info <<-EOF &&
+ 100755 $empty_blob 0 exec
+ 120000 $empty_blob 0 symlink
+ EOF
+ attr_check_object_mode normal 100644 --cached &&
+ attr_check_object_mode exec 100755 --cached &&
+ attr_check_object_mode symlink 120000 --cached
+'
+
+test_expect_success 'check object mode attributes work for submodules' '
+ mkdir sub &&
+ (
+ cd sub &&
+ git init &&
+ mv .git .real &&
+ echo "gitdir: .real" >.git &&
+ test_commit first
+ ) &&
+ attr_check_object_mode sub 160000 &&
+ attr_check_object_mode sub unspecified --cached &&
+ git add sub &&
+ attr_check_object_mode sub 160000 --cached
+'
+
+test_expect_success 'we do not allow user defined builtin_* attributes' '
+ echo "foo* builtin_foo" >.gitattributes &&
+ git add .gitattributes 2>actual &&
+ echo "builtin_foo is not a valid attribute name: .gitattributes:1" >expect &&
+ test_cmp expect actual
+'
+
+test_expect_success 'user defined builtin_objectmode values are ignored' '
+ echo "foo* builtin_objectmode=12345" >.gitattributes &&
+ git add .gitattributes &&
+ >foo_1 &&
+ attr_check_object_mode_basic foo_1 100644 &&
+ echo "builtin_objectmode is not a valid attribute name: .gitattributes:1" >expect &&
+ test_cmp expect err
+'
+
test_done
diff --git a/t/t6135-pathspec-with-attrs.sh b/t/t6135-pathspec-with-attrs.sh
index a9c1e4e0ec..b08a32ea68 100755
--- a/t/t6135-pathspec-with-attrs.sh
+++ b/t/t6135-pathspec-with-attrs.sh
@@ -295,4 +295,29 @@ test_expect_success 'reading from .gitattributes in a subdirectory (3)' '
test_cmp expect actual
'
+test_expect_success 'pathspec with builtin_objectmode attr can be used' '
+ >mode_exec_file_1 &&
+
+ git status -s ":(attr:builtin_objectmode=100644)mode_exec_*" >actual &&
+ echo ?? mode_exec_file_1 >expect &&
+ test_cmp expect actual &&
+
+ git add mode_exec_file_1 && chmod +x mode_exec_file_1 &&
+ git status -s ":(attr:builtin_objectmode=100755)mode_exec_*" >actual &&
+ echo AM mode_exec_file_1 >expect &&
+ test_cmp expect actual
+'
+
+test_expect_success 'builtin_objectmode attr can be excluded' '
+ >mode_1_regular &&
+ >mode_1_exec && chmod +x mode_1_exec &&
+ git status -s ":(exclude,attr:builtin_objectmode=100644)" "mode_1_*" >actual &&
+ echo ?? mode_1_exec >expect &&
+ test_cmp expect actual &&
+
+ git status -s ":(exclude,attr:builtin_objectmode=100755)" "mode_1_*" >actual &&
+ echo ?? mode_1_regular >expect &&
+ test_cmp expect actual
+'
+
test_done
--
2.43.0.rc0.421.g78406f8d94-goog
^ permalink raw reply related
* Re: [PATCH v2 04/10] ref-filter.h: add functions for filter/format & format-only
From: Junio C Hamano @ 2023-11-16 5:39 UTC (permalink / raw)
To: Victoria Dye via GitGitGadget
Cc: git, Patrick Steinhardt, Øystein Walle, Kristoffer Haugsbakk,
Victoria Dye
In-Reply-To: <187b1d6610f96ba16bb7e1ff80d1c994a67b8753.1699991638.git.gitgitgadget@gmail.com>
"Victoria Dye via GitGitGadget" <gitgitgadget@gmail.com> writes:
> This consolidates much of the code used to filter and format refs in
> 'builtin/for-each-ref.c', 'builtin/tag.c', and 'builtin/branch.c', reducing
> duplication and simplifying the future changes needed to optimize the filter
> & format process.
>
> Signed-off-by: Victoria Dye <vdye@github.com>
> ---
> builtin/branch.c | 33 +++++++++++++++++----------------
> builtin/for-each-ref.c | 27 +--------------------------
> builtin/tag.c | 23 +----------------------
> ref-filter.c | 35 +++++++++++++++++++++++++++++++++++
> ref-filter.h | 14 ++++++++++++++
> 5 files changed, 68 insertions(+), 64 deletions(-)
The amount of existing duplication of code is rather surprising, and
this patch nicely refactors to improve. Good.
Thanks.
^ permalink raw reply
* Re: [PATCH v2 01/10] ref-filter.c: really don't sort when using --no-sort
From: Junio C Hamano @ 2023-11-16 5:29 UTC (permalink / raw)
To: Victoria Dye via GitGitGadget
Cc: git, Patrick Steinhardt, Øystein Walle, Kristoffer Haugsbakk,
Victoria Dye
In-Reply-To: <074da1ff3e85927324c42a3fa65e4239f051cd70.1699991638.git.gitgitgadget@gmail.com>
"Victoria Dye via GitGitGadget" <gitgitgadget@gmail.com> writes:
> From: Victoria Dye <vdye@github.com>
>
> When '--no-sort' is passed to 'for-each-ref', 'tag', and 'branch', the
> printed refs are still sorted by ascending refname. Change the handling of
> sort options in these commands so that '--no-sort' to truly disables
> sorting.
"to truly disables" -> "truly disables" I think?
> '--no-sort' does not disable sorting in these commands is because their
"'--no-sort' does not" -> "The reason why '--no-sort' does not", or
"is because" -> "because".
> option parsing does not distinguish between "the absence of '--sort'"
> (and/or values for tag.sort & branch.sort) and '--no-sort'. Both result in
> an empty 'sorting_options' string list, which is parsed by
> 'ref_sorting_options()' to create the 'struct ref_sorting *' for the
> command. If the string list is empty, 'ref_sorting_options()' interprets
> that as "the absence of '--sort'" and returns the default ref sorting
> structure (equivalent to "refname" sort).
>
> To handle '--no-sort' properly while preserving the "refname" sort in the
> "absence of --sort'" case, first explicitly add "refname" to the string list
> *before* parsing options. This alone doesn't actually change any behavior,
> since 'compare_refs()' already falls back on comparing refnames if two refs
> are equal w.r.t all other sort keys.
>
> Now that the string list is populated by default, '--no-sort' is the only
> way to empty the 'sorting_options' string list. Update
> 'ref_sorting_options()' to return a NULL 'struct ref_sorting *' if the
> string list is empty, and add a condition to 'ref_array_sort()' to skip the
> sort altogether if the sort structure is NULL. Note that other functions
> using 'struct ref_sorting *' do not need any changes because they already
> ignore NULL values.
Nice.
> Finally, remove the condition around sorting in 'ls-remote', since it's no
> longer necessary. Unlike 'for-each-ref' et. al., it does *not* do any
> sorting by default. This default is preserved by simply leaving its sort key
> string list empty before parsing options; if no additional sort keys are
> set, 'struct ref_sorting *' is NULL and sorting is skipped.
Doubly nice.
> diff --git a/ref-filter.c b/ref-filter.c
> index e4d3510e28e..7250089b7c6 100644
> --- a/ref-filter.c
> +++ b/ref-filter.c
> @@ -3142,7 +3142,8 @@ void ref_sorting_set_sort_flags_all(struct ref_sorting *sorting,
>
> void ref_array_sort(struct ref_sorting *sorting, struct ref_array *array)
> {
> - QSORT_S(array->items, array->nr, compare_refs, sorting);
> + if (sorting)
> + QSORT_S(array->items, array->nr, compare_refs, sorting);
> }
Nice. We do allow passing NULL to ref_sorting_release(), and we can
return NULL from ref_sorting_options(), and allowing NULL to be
passed to this function makes it easier for the callers to deal with
the case where no sorting is specified.
> diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
> index 3182abde27f..9918ba05dec 100755
> --- a/t/t3200-branch.sh
> +++ b/t/t3200-branch.sh
> @@ -1570,9 +1570,10 @@ test_expect_success 'tracking with unexpected .fetch refspec' '
>
> test_expect_success 'configured committerdate sort' '
> git init -b main sort &&
> + test_config -C sort branch.sort "committerdate" &&
> +
> (
> cd sort &&
> - git config branch.sort committerdate &&
> test_commit initial &&
> git checkout -b a &&
> test_commit a &&
> @@ -1592,9 +1593,10 @@ test_expect_success 'configured committerdate sort' '
> '
>
> test_expect_success 'option override configured sort' '
> + test_config -C sort branch.sort "committerdate" &&
> +
> (
> cd sort &&
> - git config branch.sort committerdate &&
> git branch --sort=refname >actual &&
> cat >expect <<-\EOF &&
> a
The above two are not strictly necessary for the purpose of this
patch, in that the tests that come after these tests do not care if
the branch.sort configuration variable is set in the "sort"
repository, as they set their own value before doing their test.
But of course, cleaning up after yourself with test_config and
friends is a good idea regardless, and a handful of new tests added
after this point follow the same pattern. Good.
> @@ -1606,10 +1608,70 @@ test_expect_success 'option override configured sort' '
> )
> '
>
> +test_expect_success '--no-sort cancels config sort keys' '
> + test_config -C sort branch.sort "-refname" &&
> +
> + (
> + cd sort &&
> +
> + # objecttype is identical for all of them, so sort falls back on
> + # default (ascending refname)
Interesting.
> + git branch \
> + --no-sort \
> + --sort="objecttype" >actual &&
> + cat >expect <<-\EOF &&
> + a
> + * b
> + c
> + main
> + EOF
> + test_cmp expect actual
> + )
> +
> +'
> +
> +test_expect_success '--no-sort cancels command line sort keys' '
> + (
> + cd sort &&
> +
> + # objecttype is identical for all of them, so sort falls back on
> + # default (ascending refname)
> + git branch \
> + --sort="-refname" \
> + --no-sort \
> + --sort="objecttype" >actual &&
OK, this exercises the same "--no-sort cleans the slate" as before,
and for this one it is essential that we lack branch.sort after the
previous step is done, which is ensured thanks to the use of
test_config in the previous one. Nice.
> + cat >expect <<-\EOF &&
> + a
> + * b
> + c
> + main
> + EOF
> + test_cmp expect actual
> + )
> +'
> +
> +test_expect_success '--no-sort without subsequent --sort prints expected branches' '
> + (
> + cd sort &&
> +
> + # Sort the results with `sort` for a consistent comparison
> + # against expected
> + git branch --no-sort | sort >actual &&
> + cat >expect <<-\EOF &&
> + a
> + c
> + main
> + * b
> + EOF
> + test_cmp expect actual
> + )
> +'
> +
> test_expect_success 'invalid sort parameter in configuration' '
> + test_config -C sort branch.sort "v:notvalid" &&
> +
> (
> cd sort &&
> - git config branch.sort "v:notvalid" &&
>
> # this works in the "listing" mode, so bad sort key
> # is a dying offence.
With such an invalid configuration value set, running the command
with "--no-sort" would stop the command from failing? Is that worth
protecting with a new test, I wonder.
Overall very nicely done.
Thanks.
^ permalink raw reply
* Re: [RFC PATCH v2 2/2] send-email: remove stray characters from usage
From: Junio C Hamano @ 2023-11-16 4:59 UTC (permalink / raw)
To: Todd Zullinger
Cc: git, Jeff King, Ævar Arnfjörð Bjarmason,
Ondřej Pohořelský
In-Reply-To: <20231115173952.339303-3-tmz@pobox.com>
Todd Zullinger <tmz@pobox.com> writes:
> A few stray single quotes crept into the usage string in a2ce608244
> (send-email docs: add format-patch options, 2021-10-25). Remove them.
>
> Signed-off-by: Todd Zullinger <tmz@pobox.com>
> ---
> This is not scrictly tied to the previous commit. It just stood out
> while I was reviewing the usage output.
Thanks. Let's split this out as a docfix patch and handle it
separately.
>
> git-send-email.perl | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/git-send-email.perl b/git-send-email.perl
> index 94046e0fb7..cd2f0ae14e 100755
> --- a/git-send-email.perl
> +++ b/git-send-email.perl
> @@ -28,8 +28,8 @@
>
> sub usage {
> print <<EOT;
> -git send-email' [<options>] <file|directory>
> -git send-email' [<options>] <format-patch options>
> +git send-email [<options>] <file|directory>
> +git send-email [<options>] <format-patch options>
> git send-email --dump-aliases
>
> Composing:
^ permalink raw reply
* Re: [RFC PATCH v2 1/2] send-email: avoid duplicate specification warnings
From: Junio C Hamano @ 2023-11-16 4:58 UTC (permalink / raw)
To: Todd Zullinger
Cc: git, Jeff King, Ævar Arnfjörð Bjarmason,
Ondřej Pohořelský
In-Reply-To: <20231115173952.339303-2-tmz@pobox.com>
Todd Zullinger <tmz@pobox.com> writes:
> With perl-Getopt-Long >= 2.55 a warning is issued for options which are
> specified more than once. In addition to causing users to see warnings,
> this results in test failures which compare the output. An example,
> from t9001-send-email.37:
>
> | +++ diff -u expect actual
> | --- expect 2023-11-14 10:38:23.854346488 +0000
> | +++ actual 2023-11-14 10:38:23.848346466 +0000
> | @@ -1,2 +1,7 @@
> | +Duplicate specification "no-chain-reply-to" for option "no-chain-reply-to"
> | +Duplicate specification "to-cover|to-cover!" for option "to-cover"
> | +Duplicate specification "cc-cover|cc-cover!" for option "cc-cover"
> | +Duplicate specification "no-thread" for option "no-thread"
> | +Duplicate specification "no-to-cover" for option "no-to-cover"
> | fatal: longline.patch:35 is longer than 998 characters
> | warning: no patches were sent
> | error: last command exited with $?=1
> | not ok 37 - reject long lines
>
> Remove the duplicate option specs.
>
> Teach `--git-completion-helper` to output the '--no-' options. They are
> not included in the options hash and would otherwise be lost.
Nice to see a careful handling of potential fallouts.
> A little history:
>
> Support for the '--no-' prefix was added in Getopt::Long >= 2.33, in
> commit 8ca8b48 (Negatable options (with "!") now also support the
> "no-" prefix., 2003-04-04). Getopt::Long 2.34 was included in
> perl-5.8.1 (2003-09-25), per Module::CoreList[1].
>
> We list perl-5.8 as the minimum version in INSTALL. This would leave
> users with perl-5.8.0 (2002-07-18) with non-working arguments for
> options where we're removing the explicit 'no-' variant.
>
> The explicit 'no-' opts were added in f471494303 (git-send-email.perl:
> support no- prefix with older GetOptions, 2015-01-30), specifically to
> support perl-5.8.0 which includes the older Getopt::Long.
These are all very much relevant and deserve to be in the log
message, not hidden under the three-dash line, I would think.
Thanks for digging the history. The first paragraph was a bit hard
to read as it wasn't clear "support" on which side is being
discussed, though. If it were written perhaps like so:
Getopt::Long >= 2.33 started supporting the '--no-' prefix
natively by appending '!' to the option specification string,
which was shipped with perl-5.8.1 and not present in perl-5.8.0
it would have been clear that it was talking about the support
given by Getopt module, not on our side.
> It may be time to bump the Perl requirement to 5.8.1 (2003-09-25) or
> even 5.10.0 (2007-12-18). We last bumped the requirement from 5.6 to
> 5.8 in d48b284183 (perl: bump the required Perl version to 5.8 from
> 5.6.[21], 2010-09-24).
Isn't the position this patch takes a lot stronger than "It may be
time"? If we applied this patch, it drops the support for folks
with Perl 5.8.0 (which I do not think is a bad thing, by the way).
This sounds like something that is worth describing in the log
message (and Release Notes).
> If there is a way to have our cake without any consequence, I'm happy to
> hear it. If not, I'll add a commit which bumps the requirement in
> general or notes that some git-send-email requires perl >= 5.8.1 and
> adjusts the 'use' line there to `use 5.008001;`.
Sounds like a plan.
> diff --git a/git-send-email.perl b/git-send-email.perl
> index cacdbd6bb2..94046e0fb7 100755
> --- a/git-send-email.perl
> +++ b/git-send-email.perl
> @@ -119,13 +119,16 @@ sub completion_helper {
>
> foreach my $key (keys %$original_opts) {
> unless (exists $not_for_completion{$key}) {
> - $key =~ s/!$//;
> + my $negate = ($key =~ s/!$//);
A very minor nit, but I'd call this $negatable if I were doing this
patch.
Just to make sure I did not misunderstand what you said below the
three-dash line, if we were to take the other option that allows us
to live with 5.8.0, we would make this hunk ...
> "chain-reply-to!" => \$chain_reply_to,
> - "no-chain-reply-to" => sub {$chain_reply_to = 0},
... look more like this?
> - "chain-reply-to!" => \$chain_reply_to,
> + "chain-reply-to" => \$chain_reply_to,
> "no-chain-reply-to" => sub {$chain_reply_to = 0},
> + "nochain-reply-to" => sub {$chain_reply_to = 0},
That is, by removing the "!" suffix, we reject the native support of
"--no-*" offered by Getopt::Long, and implement the negated variants
ourselves?
Thanks.
^ permalink raw reply
* Re: Bug: "Received" misspelled in remote message
From: Junio C Hamano @ 2023-11-16 3:42 UTC (permalink / raw)
To: git; +Cc: Alan Dove
In-Reply-To: <xmqqmsvetmu3.fsf@gitster.g>
Junio C Hamano <gitster@pobox.com> writes:
>> "remote: Recieved update on checked-out branch, queueing deployment."
>>
>> "Received" is misspelled.
>
> I think it is coming from the "push-to-checkout" hook that is
> installed on your "private server", and not from what we ship as
> part of our software offering.
Googling for the misspelled message seems to indicate that it is
coming from cpanel (whatever it is). A randomly picked example is
https://essgeelabs.com/posts/cpanel-git-1/ which is a recipe that
does not involve typing the typoed message by the end-user, so
somebody (most likely cPanel) must be shipping with a hook with
typo.
Another user of cPanel reports this
https://forums.cpanel.net/threads/git-automatic-deployment-not-working-but-manual-deployment-is.679837/
where they observe post-receive hook with the typo.
Anybody with contact there at cpanel.net may want to report the bug.
Thanks.
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox