Re: [BUG]: Testsuite failures on big-endian targets
Hello, [+cc: Ævar] John Paul Adrian Glaubitz wrote: > The testsuite is failing again on s390x and all other big-endian targets in > Debian. For a full build log on s390x see [1]. > > Adrian > >> [1] >> https://buildd.debian.org/status/fetch.php?pkg=git&arch=s390x&ver=1%3A2.24.0%7Erc0-1&stamp=1571440098&raw=0 With t0500 resolved by <20191019233706.gm29...@szeder.dev>, that just leaves the one failure in t7812. Test Summary Report --- t7812-grep-icase-non-ascii.sh(Wstat: 256 Tests: 11 Failed: 1) Failed test: 11 Non-zero exit status: 1 Files=879, Tests=21880, 404 wallclock secs ( 3.38 usr 1.15 sys + 440.87 cusr 729.29 csys = 1174.69 CPU) Result: FAIL The failing test output: expecting success of 7812.11 'PCRE v2: grep non-ASCII from invalid UTF-8 data with -i': test_might_fail git grep -hi "Æ" invalid-0x80 >actual && test_cmp expected actual && test_must_fail git grep -hi "(*NO_JIT)Æ" invalid-0x80 && test_cmp expected actual ++ test_might_fail git grep -hi Æ invalid-0x80 ++ test_must_fail ok=success git grep -hi Æ invalid-0x80 ++ case "$1" in ++ _test_ok=success ++ shift ++ git grep -hi Æ invalid-0x80 fatal: pcre2_match failed with error code -22: UTF-8 error: isolated byte with 0x80 bit set ++ exit_code=128 ++ test 128 -eq 0 ++ test_match_signal 13 128 ++ test 128 = 141 ++ test 128 = 269 ++ return 1 ++ test 128 -gt 129 ++ test 128 -eq 127 ++ test 128 -eq 126 ++ return 0 ++ test_cmp expected actual ++ diff -u expected actual --- expected2019-10-19 21:56:08.634252012 + +++ actual 2019-10-19 21:56:08.714252012 + @@ -1 +0,0 @@ -ævar error: last command exited with $?=1 not ok 11 - PCRE v2: grep non-ASCII from invalid UTF-8 data with -i # # test_might_fail git grep -hi "Æ" invalid-0x80 >actual && # test_cmp expected actual && # test_must_fail git grep -hi "(*NO_JIT)Æ" invalid-0x80 && # test_cmp expected actual # # failed 1 among 11 test(s) I'm not flush on time to even try to look much; but I'd be kidding myself if I said I was likely to find the issue quickly. ;) But I'm pretty sure it will be obvious to someone here. -- Todd
Re: [PATCH] test-progress: fix test failures on big-endian systems
Hi, John Paul Adrian Glaubitz wrote: > Hi Gábor! > > On 10/20/19 1:37 AM, SZEDER Gábor wrote: >> On Sat, Oct 19, 2019 at 11:38:40PM +0200, John Paul Adrian Glaubitz wrote: >>> The testsuite is failing again on s390x and all other big-endian targets in >>> Debian. For a full build log on s390x see [1]. >> >> Gah, my progress display fixes strike again... >> >> I think the patch below should fix it, but I could only test it on >> little-endian systems. Could you please confirm that it indeed works >> on big-endian as well? [...] > I can confirm that your patch fixes the testsuite for me on Debian > unstable/ppc64 (big-endian). > > Tested-By: John Paul Adrian Glaubitz Yep, that worked well on the Fedora s390x builders as well (unsurprisingly). Thanks! -- Todd
Re: Git for Windows v2.23.0-rc0, was Re: [ANNOUNCE] Git v2.23.0-rc0
Junio C Hamano wrote: > Junio C Hamano writes: > >> Jeff King writes: >> + if (mailmap < 0) mailmap = 0; - } >>> >>> This should be "mailmap = 1" to match the commit message, no? (Which >>> also implies we may want a new test). >> [...] > +test_expect_success 'log.mailmap is true by default these days' ' > + git log --author Santa | grep Author >actual && > + test_cmp expect actual > +' > + > test_expect_success 'Only grep replaced author with --use-mailmap' ' > git log --use-mailmap --author "" >actual && > test_must_be_empty actual With log.mailmap true by default, should we also have some tests to ensure that --no-use-mailmap and log.mailmap=False do the right thing? (I mean eventually, not necessarily with this patch as extra work for you Junio.) (If I was certain the answer is "yes" and more familiar with t4203, I would have sent this in diff format.) -- Todd
Re: [BUG]: Testsuite failures on big-endian targets
Hi, John Paul Adrian Glaubitz wrote: > Recent versions of git are failing the testsuite on big-endian targets > such as s390x in Debian. > > Build logs are: > >> https://buildd.debian.org/status/fetch.php?pkg=git&arch=s390x&ver=1%3A2.23.0%7Erc0-1&stamp=1564449102&raw=0 >> https://buildd.debian.org/status/fetch.php?pkg=git&arch=s390x&ver=1%3A2.23.0%7Erc0%2Bnext.20190729-1&stamp=1564449397&raw=0 > > Unfortunately, I cannot really read from the build logs which test in > particular is actually failing as I see a lot of lines starting with > the string "error". The test I see failing is test 6 in t0016-oidmap. Grepping for '^not ok ' is helpful in this case, though it's even better when the test summary is provided, as it points to the failing tests by name and number. The t0016-oidmap failure is discussed in the thread starting here: https://public-inbox.org/git/04b301d54715$3b371a90$b1a54fb0$@nexbridge.com/T/ Peff posted a patch which resolves the test failure here: https://public-inbox.org/git/20190731012336.ga13...@sigill.intra.peff.net/ Cheers, -- Todd
Re: [ANNOUNCE] Git v2.23.0-rc0 - Initial test failures on NonStop
Jeff King wrote:
> On Tue, Jul 30, 2019 at 09:59:17PM -0400, Todd Zullinger wrote:
>> At the risk of showing my complete lack of knowledge about
>> these tests, I was wondering if the order mattered for the
>> other tests in t0011 and t0016.
[...]
>> You've got a more comprehensive patch and a proper commit
>> message, so this is really just a matter of curiosity.
>
> I think the order does matter for those ones. E.g., the ones that run
> "get" want to make sure they're seeing the values in the same order in
> which they were requested.
Ahh, thanks for clarifying that (and saving me from sending
the version I had which would have incorrectly sorted all
the test_{hashmap,oidmap} output. :)
FWIW, I applied your patch for sorting hashmap iterations
(<20190731012336.ga13...@sigill.intra.peff.net>¹) and ran it
through the Fedora build system. All architectures passed,
as expected.
¹ https://public-inbox.org/git/20190731012336.ga13...@sigill.intra.peff.net/
--
Todd
Re: [ANNOUNCE] Git v2.23.0-rc0 - Initial test failures on NonStop
Jeff King wrote:
> On Tue, Jul 30, 2019 at 08:59:33PM -0400, Jeff King wrote:
>
>>> OTOH, this is not just any hashmap, but an oidmap, and I could imagine
>>> that there might be use cases where it would be beneficial if the
>>> iteration order were to match the oid order (but don't know whether we
>>> actually have such a use case).
>>
>> I don't think we can promise anything about iteration order. This test
>> is relying on the order at least being deterministic between runs, but
>> if we added a new entry and had to grow the table, all bets are off.
>>
>> So regardless of the endian thing above, it probably does make sense for
>> any hashmap iteration output to be sorted before comparing. That goes
>> for t0011, too; it doesn't have this endian thing, but it looks to be
>> relying on hash order that could change if we swapped out hash
>> functions.
>
> So here's an actual patch.
At the risk of showing my complete lack of knowledge about
these tests, I was wondering if the order mattered for the
other tests in t0011 and t0016. I had assumed it didn't and
had something like this for testing (and a similar change to
test_oidmap() in t0016):
diff --git i/t/t0011-hashmap.sh w/t/t0011-hashmap.sh
index 9c96b3e3b1..9ed1c4f14d 100755
--- i/t/t0011-hashmap.sh
+++ w/t/t0011-hashmap.sh
@@ -4,8 +4,8 @@ test_description='test hashmap and string hash functions'
. ./test-lib.sh
test_hashmap() {
- echo "$1" | test-tool hashmap $3 > actual &&
- echo "$2" > expect &&
+ echo "$1" | test-tool hashmap $3 | sort >actual &&
+ echo "$2" | sort >expect &&
test_cmp expect actual
}
You've got a more comprehensive patch and a proper commit
message, so this is really just a matter of curiosity.
--
Todd
Re: [ANNOUNCE] Git v2.23.0-rc0 - Initial test failures on NonStop
Hi, [added Christian, SZEDER, and Jeff to Cc as author and helpers on the newly-added t0016-oidmap] Randall S. Becker wrote: > A preview of the situation with testing 2.23.0.rc0 on > NonStop is not great. We have had some new failures right > off the bat on our NonStop platforms. This is a preview of > what we find within the first bit of testing. The tests > run a long time, so more to come. > > t0016: oidmap > > Subtest 6 had an ordering issue. We do not know whether > the problem is the code or the test result not keeping up > with the code changes. > > --- expect 2019-07-30 16:56:36 + > +++ actual 2019-07-30 16:56:36 + > @@ -1,6 +1,6 @@ > NULL > NULL > NULL > +7c7cd714e262561f73f3079dfca4e8724682ac21 3 > 139b20d8e6c5b496de61f033f642d0e3dbff528d 2 > d79ce1670bdcb76e6d1da2ae095e890ccb326ae9 1 > -7c7cd714e262561f73f3079dfca4e8724682ac21 3 I hit the same failure while building for Fedora on the s390x architecture. I have not dug into it much yet, but perhaps this is an endianess issue? -- Todd
Re: [PATCH v3] tag: add tag.gpgSign config option to force all tags be GPG-signed
Hi, Tigran Mkrtchyan wrote: > diff --git a/Documentation/config/tag.txt b/Documentation/config/tag.txt > index 663663bdec..675483c3c3 100644 > --- a/Documentation/config/tag.txt > +++ b/Documentation/config/tag.txt > @@ -8,6 +8,13 @@ tag.sort:: > linkgit:git-tag[1]. Without the "--sort=" option provided, the > value of this variable will be used as the default. > > +tag.gpgsign:: > + A boolean to specify whether all tags should be GPG signed. > + Use of this option when running in an automated script can > + result in a large number of tags being signed. It is therefore > + convenient to use an agent to avoid typing your gpg passphrase > + several times. I think the variable should be camelCased, i.e. tag.gpgSign, for consistency with other documentation. -- Todd
[PATCH] RelNotes: minor typo fixes in 2.22.0 draft
Signed-off-by: Todd Zullinger --- These are all just minor typos I noticed while reading the release notes. I did find the first entry on the checkout --no-overlay read a bit strangely with the multiple "out's" in '... checking out paths out ...'. Is it any easier to read with: * "git checkout --no-overlay" can be used to trigger a new mode of - checking out paths out of the tree-ish, that allows paths that + checking out paths from the tree-ish, which allows paths that match the pathspec that are in the current index and working tree and are not in the tree-ish. ? (There's any number of other ways to drop one of the "out's" [and similarly replaces a "that" with "which"] in the sentence, of course. Whether it's worth doing at all is really the question.) Documentation/RelNotes/2.22.0.txt | 12 ++-- 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/Documentation/RelNotes/2.22.0.txt b/Documentation/RelNotes/2.22.0.txt index 84e1ba6106..5aa4a256ae 100644 --- a/Documentation/RelNotes/2.22.0.txt +++ b/Documentation/RelNotes/2.22.0.txt @@ -181,7 +181,7 @@ Performance, Internal Implementation, Development Support etc. been optimized out. * Mechanically and systematically drop "extern" from function - declarlation. + declaration. * The script to aggregate perf result unconditionally depended on libjson-perl even though it did not have to, which has been @@ -270,7 +270,7 @@ Fixes since v2.21 * On platforms where "git fetch" is killed with SIGPIPE (e.g. OSX), the upload-pack that runs on the other end that hangs up after detecting an error could cause "git fetch" to die with a signal, - which led to a flakey test. "git fetch" now ignores SIGPIPE during + which led to a flaky test. "git fetch" now ignores SIGPIPE during the network portion of its operation (this is not a problem as we check the return status from our write(2)s). (merge 143588949c jk/no-sigpipe-during-network-transport later to maint). @@ -358,7 +358,7 @@ Fixes since v2.21 (merge b5a0bd694c nd/read-tree-reset-doc later to maint). * Code clean-up around a much-less-important-than-it-used-to-be - update_server_info() funtion. + update_server_info() function. (merge b3223761c8 jk/server-info-rabbit-hole later to maint). * The message given when "git commit -a " errors out has been @@ -450,7 +450,7 @@ Fixes since v2.21 * When given a tag that points at a commit-ish, "git replace --graft" failed to peel the tag before writing a replace ref, which did not make sense because the old graft mechanism the feature wants to - mimick only allowed to replace one commit object with another. + mimic only allowed to replace one commit object with another. This has been fixed. (merge ee521ec4cb cc/replace-graft-peel-tags later to maint). @@ -500,7 +500,7 @@ Fixes since v2.21 conflicts are resolved in working tree *.h files but before the resolved results are added to the index. This has been corrected. - * "git chery-pick" (and "revert" that shares the same runtime engine) + * "git cherry-pick" (and "revert" that shares the same runtime engine) that deals with multiple commits got confused when the final step gets stopped with a conflict and the user concluded the sequence with "git commit". Attempt to fix it by cleaning up the state @@ -535,7 +535,7 @@ Fixes since v2.21 todo-list "rebase -i -r" uses should not be shown as a hex object name. - * A prerequiste check in the test suite to see if a working jgit is + * A prerequisite check in the test suite to see if a working jgit is available was made more robust. (merge abd0f28983 tz/test-lib-check-working-jgit later to maint). -- Todd
Re: [PATCH v2 1/3] diff-parseopt: correct variable types that are used by parseopt
Nguyễn Thái Ngọc Duy wrote:
> Most number-related OPT_ macros store the value in an 'int'
> variable. Many of the variables in 'struct diff_options' have a
> different type, but during the conversion to using parse_options() I
> failed to notice and correct.
>
> The problem was reported on s360x which is a big-endian
> architechture. The variable to store '-w' option in this case is
> xdl_opts, 'long' type, 8 bytes. But since parse_options() assumes
> 'int' (4 bytes), it will store bits in the wrong part of xdl_opts. The
> problem was found on little-endian platforms because parse_options()
> will accidentally store at the right part of xdl_opts.
>
> There aren't much to say about the type change (except that 'int' for
> xdl_opts should still be big enough, since Windows' long is the same
> size as 'int' and nobody has complained so far). Some safety checks may
> be implemented in the future to prevent class of bugs.
>
> Reported-by: Todd Zullinger
> Signed-off-by: Nguyễn Thái Ngọc Duy
> ---
> diff.h | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/diff.h b/diff.h
> index b20cbcc091..d5e44baa96 100644
> --- a/diff.h
> +++ b/diff.h
> @@ -169,7 +169,7 @@ struct diff_options {
> const char *prefix;
> int prefix_length;
> const char *stat_sep;
> - long xdl_opts;
> + int xdl_opts;
>
> /* see Documentation/diff-options.txt */
> char **anchors;
FWIW, I ran this versions of the series through the fedora
buildsystem and noticed no issues on s390x or any other
architectures.
Thanks,
--
Todd
Re: [PATCH] send-email: remove documented requirement for Net::SMTP::SSL
Hi,
Sorry I missed your earlier reply which also mentioned using
$obj->can() Ævar. That's what I get for typing a reply and
then walking away for a few hours before hitting send. ;)
Ævar Arnfjörð Bjarmason wrote:
> Same, but to bikeshed a bit, at this point we can just do:
>
> diff --git a/git-send-email.perl b/git-send-email.perl
> index 24859a7bc3..4ad2091a49 100755
> --- a/git-send-email.perl
> +++ b/git-send-email.perl
> @@ -1468 +1467,0 @@ sub send_message {
> - my $use_net_smtp_ssl =
> version->parse($Net::SMTP::VERSION) < version->parse("2.34");
> @@ -1485 +1484 @@ sub send_message {
> - if ($use_net_smtp_ssl) {
> + if (Net::SMTP->can('starttls')) {
> @@ -1507 +1506 @@ sub send_message {
> - if ($use_net_smtp_ssl) {
> + if (Net::SMTP->can('starttls')) {
>
I think we'd need to use 'if ! ...' there, or more likely,
switch the blocks which follow because the code following
'if ($use_net_smtp_ssl)' is for Net::SMTP::SSL with the
'else' block handling the case where Net::SMTP has ssl/tls
support. Right?
I know I read the $use_net_smtp_ssl bit backwards the first
time or two as well.
--
Todd
Re: [PATCH] send-email: remove documented requirement for Net::SMTP::SSL
Eric Wong wrote:
> Chris Mayo wrote:
>> git-send-email uses the TLS support in the Net::SMTP core module from
>> recent versions of Perl. Documenting the minimum version is complex
>> because of separate numbering for Perl (5.21.5~169), Net:SMTP (2.34)
>> and libnet (3.01). Version numbers from commit:
>> bfbfc9a953 ("send-email: Net::SMTP::starttls was introduced in v2.34",
>> 2017-05-31)
>
> No disagreement for removing the doc requirement for Net::SMTP::SSL.
>
> But core modules can be split out by OS packagers. For
> Fedora/RH-based systems, the trend tends to be increasing
> granularity and having more optional packages.
>
> So I think documenting Net::SMTP (and Net::Domain) as
> requirements would still be good, perhaps with a note stating
> they're typically installed with Perl.
I didn't know that git-send-email.perl could take advantage
of Net::SMTP::starttls until I read this.
[Adding Dennis and Jonathan as the authors of 0ead000c3a
("send-email: Net::SMTP::SSL is obsolete, use only when
necessary", 2017-03-24) bfbfc9a953 ("send-email:
Net::SMTP::starttls was introduced in v2.34", 2017-05-31),
respectively.]
The current Fedora and Red Hat package have a requirement on
Net::SMTP::SSL from long, long ago¹. As I looked at whether
I could remove that (or more accurately, replace it with
IO::Socket::SSL which is needed for Net::SMTP to handle
starttls), I noticed that on RHEL7 the Net::SMTP version was
2.31, but starttls support has been backported².
I wonder if it's (separately from this change) worth
adjusting the conditional which sets $use_net_smtp_ssl to
use "Net::SMTP->can('starttls')" rather than a strict
version check? (It might not be if using 'can' is too
fragile or would only benefit the Red Hat 7 packages which
likely won't officially be updated to a newer git with such
a change.)
Something like:
diff --git i/git-send-email.perl w/git-send-email.perl
index 24859a7bc3..84ac03994d 100755
--- i/git-send-email.perl
+++ w/git-send-email.perl
@@ -1465,7 +1465,7 @@ sub send_message {
}
require Net::SMTP;
- my $use_net_smtp_ssl = version->parse($Net::SMTP::VERSION) <
version->parse("2.34");
+ my $use_net_smtp_ssl = Net::SMTP->can('starttls') ? 0 : 1;
$smtp_domain ||= maildomain();
if ($smtp_encryption eq 'ssl') {
¹ https://bugzilla.redhat.com/443615
² https://bugzilla.redhat.com/1557574
https://git.centos.org/rpms/perl/c/13dfe3?branch=c7
--
Todd
Re: [PATCH 0/3] fix diff-parseopt regressions
Duy Nguyen wrote: > On Sat, May 25, 2019 at 12:36 AM Todd Zullinger wrote: >> I applied this on top of master/2.22.0-rc1 and see a number >> of compiler errors using gcc-9.1.1 with fedora's standard >> compiler options for rpm builds. > > That last patch 4/3 is not meant to be applied. Yes I've seen similar > compiler errors too. We have some cleaning up to do in order to build > with the last one. D'oh! Sorry for missing that rather obvious part of your previous message. The tests do indeed all pass on all the architectures in the Fedora build system. I'll go dig out my dunce cap from back in grade school. ;) Thanks again, -- Todd
Re: [PATCH 0/3] fix diff-parseopt regressions
I wrote: > Below are the compiler errors. Well, to be precise, all but imap-send are warnings rather than errors. -- Todd
Re: [PATCH 0/3] fix diff-parseopt regressions
Hi,
Nguyễn Thái Ngọc Duy wrote:
> This should fix the diff tests failure on s360x. It's a serious problem
> and I plan to do something to prevent it from happening again.
Thanks for looking at this!
I applied this on top of master/2.22.0-rc1 and see a number
of compiler errors using gcc-9.1.1 with fedora's standard
compiler options for rpm builds.
Below are the compiler errors. This was from an s390x
build, but other arches had the same errors. The complete
build log is available here for a few weeks:
https://kojipkgs.fedoraproject.org//work/tasks/3166/35033166/build.log
cc -o credential-store.o -c -MF ./.depend/credential-store.o.d -MQ
credential-store.o -MMD -MP -O2 -g -pipe -Wall -Werror=format-security
-Wp,-D_FORTIFY_SOURCE=2 -Wp,-D_GLIBCXX_ASSERTIONS -fexceptions
-fstack-protector-strong -grecord-gcc-switches
-specs=/usr/lib/rpm/redhat/redhat-hardened-cc1
-specs=/usr/lib/rpm/redhat/redhat-annobin-cc1 -m64 -march=zEC12 -mtune=z13
-fasynchronous-unwind-tables -fstack-clash-protection -I. -DHAVE_SYSINFO
-DGIT_HOST_CPU="\"s390x\"" -DUSE_LIBPCRE2 -DHAVE_ALLOCA_H
-DUSE_CURL_FOR_IMAP_SEND -DSHA1_DC -DSHA1DC_NO_STANDARD_INCLUDES
-DSHA1DC_INIT_SAFE_HASH_DEFAULT=0 -DSHA1DC_CUSTOM_INCLUDE_SHA1_C="\"cache.h\""
-DSHA1DC_CUSTOM_INCLUDE_UBC_CHECK_C="\"git-compat-util.h\"" -DSHA256_BLK
-DHAVE_PATHS_H -DHAVE_DEV_TTY -DHAVE_CLOCK_GETTIME -DHAVE_CLOCK_MONOTONIC
-DHAVE_GETDELIM '-DPROCFS_EXECUTABLE_PATH="/proc/self/exe"'
-DFREAD_READS_DIRECTORIES -DNO_STRLCPY -DSHELL_PATH='"/bin/sh"'
-DPAGER_ENV='"LESS=FRX LV=-c"' credential-store.c
In file included from credential-store.c:5:
credential-store.c: In function 'cmd_main':
credential-store.c:156:25: warning: passing argument 1 of '_opt_string' from
incompatible pointer type [-Wincompatible-pointer-types]
156 | OPT_STRING(0, "file", &file, "path",
| ^
| |
| char **
parse-options.h:152:82: note: in definition of macro 'OPT_STRING_F'
152 | #define OPT_STRING_F(s, l, v, a, h, f) { OPTION_STRING, (s), (l),
_opt_string(v), (a), (h), (f) }
|
^
credential-store.c:156:3: note: in expansion of macro 'OPT_STRING'
156 | OPT_STRING(0, "file", &file, "path",
| ^~
parse-options.h:132:42: note: expected 'const char **' but argument is of type
'char **'
132 | static inline void *_opt_ ## name(type *p) \
| ^
parse-options.h:139:1: note: in expansion of macro 'DEFINE_OPT_TYPE_CHECK'
139 | DEFINE_OPT_TYPE_CHECK(string, const char *)
| ^
* new link flags
cc -o apply.o -c -MF ./.depend/apply.o.d -MQ apply.o -MMD -MP -O2 -g -pipe
-Wall -Werror=format-security -Wp,-D_FORTIFY_SOURCE=2 -Wp,-D_GLIBCXX_ASSERTIONS
-fexceptions -fstack-protector-strong -grecord-gcc-switches
-specs=/usr/lib/rpm/redhat/redhat-hardened-cc1
-specs=/usr/lib/rpm/redhat/redhat-annobin-cc1 -m64 -march=zEC12 -mtune=z13
-fasynchronous-unwind-tables -fstack-clash-protection -I. -DHAVE_SYSINFO
-DGIT_HOST_CPU="\"s390x\"" -DUSE_LIBPCRE2 -DHAVE_ALLOCA_H
-DUSE_CURL_FOR_IMAP_SEND -DSHA1_DC -DSHA1DC_NO_STANDARD_INCLUDES
-DSHA1DC_INIT_SAFE_HASH_DEFAULT=0 -DSHA1DC_CUSTOM_INCLUDE_SHA1_C="\"cache.h\""
-DSHA1DC_CUSTOM_INCLUDE_UBC_CHECK_C="\"git-compat-util.h\"" -DSHA256_BLK
-DHAVE_PATHS_H -DHAVE_DEV_TTY -DHAVE_CLOCK_GETTIME -DHAVE_CLOCK_MONOTONIC
-DHAVE_GETDELIM '-DPROCFS_EXECUTABLE_PATH="/proc/self/exe"'
-DFREAD_READS_DIRECTORIES -DNO_STRLCPY -DSHELL_PATH='"/bin/sh"'
-DPAGER_ENV='"LESS=FRX LV=-c"' apply.c
In file included from apply.c:20:
apply.c: In function 'apply_parse_options':
apply.c:5002:26: warning: pointer targets in passing argument 1 of '_opt_int'
differ in signedness [-Wpointer-sign]
5002 | OPT_INTEGER('C', NULL, &state->p_context,
| ^
| |
| unsigned int *
parse-options.h:153:79: note: in definition of macro 'OPT_INTEGER_F'
153 | #define OPT_INTEGER_F(s, l, v, h, f) { OPTION_INTEGER, (s), (l),
_opt_int(v), N_("n"), (h), (f) }
|
^
apply.c:5002:3: note: in expansion of macro 'OPT_INTEGER'
5002 | OPT_INTEGER('C', NULL, &state->p_context,
| ^~~
parse-options.h:132:42: note: expected 'int *' but argument is of type
'unsigned int *'
132 | static inline void *_opt_ ## name(type *p) \
| ^
parse-options.h:137:1: note: in expansion of macro 'DEFINE_OPT_TYPE_CHECK'
137 | DEFINE_OPT_TYPE_CHECK(int, int)
| ^
cc -o diff.o -c -MF ./.depend/diff.o.d -MQ diff.o -MMD -MP -O2 -g -pipe -Wall
-Werror=format-security -Wp,-D_FORTIFY_SOURCE=2 -Wp,-D_GLIBCXX_ASSERTIONS
-fexceptions -fstack-protector-strong -grecord-g
Re: New diff test failures on s390x architecture (was: [ANNOUNCE] Git v2.22.0-rc1)
Hi, Duy Nguyen wrote: > On Fri, May 24, 2019 at 2:14 AM Todd Zullinger wrote: >> I am guessing it's no coincidence that this only fails on >> s390x and it is the only big endian architecture in the >> fedora build system. > > I see a problem with -w and wrong type casting. sizeof(int) and > sizeof(long) on s390x are not the same, are they? I don't believe so. I tested a small program which output sizeof(int) and sizeof(long). On s390x it's 4 bytes and 8 bytes, respectively (same as on x86_64). I hope that's the right way to test. Thanks, -- Todd
Re: New diff test failures on s390x architecture (was: [ANNOUNCE] Git v2.22.0-rc1)
I wrote: > While running the 2.22.0-rc1 tests on Fedora, I hit a few > new test failures since 2.21.0 -- but only on the s390x > architecture. > > I haven't had time to dig into these the past few days, so I > thought I would send what I do have in case the problem is > obvious to someone else. I think all of the failing tests > are due to `git diff` commands. [...] > I don't have direct access to these s390x builders. I may > be able to arrange shell access (or even reproduce this with > qemu's s390x emulation). I poked around a little with a qemu s390x instance and see the same failures. One simple failure in t4015 is with: git diff -w >out && test_must_be_empty out Using git-2.21.0 this succeeds, while git-2.22.0-rc1 fails and produces: diff --git a/x b/x index d99af23..22d9f73 100644 --- a/x +++ b/x @@ -1,6 +1,6 @@ -whitespace at beginning -whitespace change -whitespace in the middle -whitespace at end + whitespace at beginning +whitespace change +white space in the middle +whitespace at end unchanged line -CR at end +CR at end I am guessing it's no coincidence that this only fails on s390x and it is the only big endian architecture in the fedora build system. -- Todd
New diff test failures on s390x architecture (was: [ANNOUNCE] Git v2.22.0-rc1)
[-cc: lkml, +cc: Duy as author of a good number of diff-related commits in 2.22.0 :) ] Hi, While running the 2.22.0-rc1 tests on Fedora, I hit a few new test failures since 2.21.0 -- but only on the s390x architecture. I haven't had time to dig into these the past few days, so I thought I would send what I do have in case the problem is obvious to someone else. I think all of the failing tests are due to `git diff` commands. Test Summary Report --- t4017-diff-retval.sh (Wstat: 256 Tests: 24 Failed: 1) Failed test: 2 Non-zero exit status: 1 t4015-diff-whitespace.sh (Wstat: 256 Tests: 83 Failed: 11) Failed tests: 2-7, 9, 11, 52-53, 79 Non-zero exit status: 1 t4035-diff-quiet.sh (Wstat: 256 Tests: 22 Failed: 4) Failed tests: 17-20 Non-zero exit status: 1 t4040-whitespace-status.sh (Wstat: 256 Tests: 11 Failed: 4) Failed tests: 3, 5, 7, 9 Non-zero exit status: 1 t4038-diff-combined.sh (Wstat: 256 Tests: 24 Failed: 3) Failed tests: 10-12 Non-zero exit status: 1 t4050-diff-histogram.sh (Wstat: 256 Tests: 3 Failed: 1) Failed test: 1 Non-zero exit status: 1 t4061-diff-indent.sh (Wstat: 256 Tests: 33 Failed: 19) Failed tests: 2-4, 8, 12, 14, 18, 20-21, 24-33 Non-zero exit status: 1 t4065-diff-anchored.sh (Wstat: 256 Tests: 7 Failed: 1) Failed test: 6 Non-zero exit status: 1 t4253-am-keep-cr-dos.sh (Wstat: 256 Tests: 7 Failed: 1) Failed test: 7 Non-zero exit status: 1 The test output from '-x --verbose-log' is quite verbose and the issues are often in whitespace/end-of-line which might be mangled by some MTA/MUA. So the output is at: https://tmz.fedorapeople.org/git-2.22.0-rc1-s390x-failures (327K) I don't have direct access to these s390x builders. I may be able to arrange shell access (or even reproduce this with qemu's s390x emulation). Before I go further down that road, I wanted to see if this output might be enough to point to the problem. If not, I can run the failing tests with the --debug option and save the trash directory (unless I can get more direct access to the s390x builder to be able to bisect). I rebuilt git-2.21.0 on the same s390x builder successfully, so I don't think this issue is a bug in another of the packages in the current Fedora tree. But it's certainly possible that git-2.22.0-rc1 is just tickling a real bug in some other tool/library on s390x. Thanks for any help in narrowing this down. -- Todd
[PATCH v2] test-lib: try harder to ensure a working jgit
The JGIT prereq uses `type jgit` to determine whether jgit is present. While this is usually sufficient, it won't help if the jgit found is badly broken. This wastes time running tests which fail due to no fault of our own. Use `jgit --version` instead, to guard against cases where jgit is present on the system, but will fail to run, e.g. because of some JRE issue, or missing Java dependencies. Checking that it gets far enough to process the '--version' argument isn't perfect, but seems to be good enough in practice. It's also consistent with how we detect some other dependencies, see e.g. the CURL and UNZIP prerequisites. Signed-off-by: Todd Zullinger --- As promised, I stole the second paragraph from Ævar nearly verbatim. :) t/test-lib.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/t/test-lib.sh b/t/test-lib.sh index 908ddb9c46..599fd70e14 100644 --- a/t/test-lib.sh +++ b/t/test-lib.sh @@ -1522,7 +1522,7 @@ test_lazy_prereq NOT_ROOT ' ' test_lazy_prereq JGIT ' - type jgit + jgit --version ' # SANITY is about "can you correctly predict what the filesystem would -- Todd
Re: [PATCH] test-lib: try harder to ensure a working jgit
Hi,
Jeff King wrote:
> On Tue, May 14, 2019 at 02:14:19AM +, brian m. carlson wrote:
>
>> On Mon, May 13, 2019 at 10:05:20PM -0400, Todd Zullinger wrote:
>>> diff --git a/t/test-lib.sh b/t/test-lib.sh
>>> index 908ddb9c46..599fd70e14 100644
>>> --- a/t/test-lib.sh
>>> +++ b/t/test-lib.sh
>>> @@ -1522,7 +1522,7 @@ test_lazy_prereq NOT_ROOT '
>>> '
>>>
>>> test_lazy_prereq JGIT '
>>> - type jgit
>>> + jgit --version
>>> '
>>
>> I think this is an improvement, not only because of the reasons you
>> mentioned, but because we remove the use of "type", which is not
>> guaranteed to be present in a POSIX shell.
>
> Isn't it?
I wondered the same thing, but I know I am not nearly as
familiar with the POSIX rules as any of you.
> I have always treated it as the most-portable option for this
> (compared to, say, `which`). It is in POSIX as a utility (albeit marked
> with XSI), which even says (in APPLICATION USAGE):
>
> Since type must be aware of the contents of the current shell
> execution environment (such as the lists of commands, functions, and
> built-ins processed by hash), it is always provided as a shell regular
> built-in.
>
> All that said, I think Todd's patch makes perfect sense even without
> wanting to avoid "type".
Yeah, `which` surely isn't a portable option. I presumed
`type` must be fairly widely available since it was in the
test suite since you added it way back in 212f2ffbf0 ("t:
add basic bitmap functionality tests", 2013-12-21).
I usually make use of `command -p -v $foo` in scripts that
need to be portable across systems. But I don't have access
to many esoteric systems.
Based on Junio's follow-up, I think we can avoid adding
anything to the commit message about the use of `type` here.
That way no one will take it as a sign that we should remove
other uses of it just for conformance. (I will send a
follow-up with an update based on Jonathan and Ævar's
comments.)
Thanks to all of you.
--
Todd
Re: [PATCH] test-lib: try harder to ensure a working jgit
Hi, Ævar Arnfjörð Bjarmason wrote: > > On Tue, May 14 2019, Jonathan Nieder wrote: > >> Todd Zullinger wrote: >> >>> The JGIT prereq uses 'type jgit' to determine whether jgit is present. >>> While this should be sufficient, if the jgit found is broken we'll waste >>> time running tests which fail due to no fault of our own. >>> >>> Use 'jgit --version' instead, to catch some badly broken jgit >>> installations. >>> >>> Signed-off-by: Todd Zullinger >>> --- >>> I ran into such a broken jgit on Fedora >= 30¹. This is clearly a >>> problem in the Fedora jgit package which will hopefully be resolved >>> soon. But it may be good to avoid wasting time debugging tests which >>> fail due to a broken tool outside of our control. >>> >>> ¹ https://bugzilla.redhat.com/1709624 >> >> Reviewed-by: Jonathan Nieder >> >> It would be nice to describe that bug in the commit message, to save >> readers some head scratching. > > FWIW the jgit in Debian testing/unstable is similarly broken right now: [...] Hah, small world. :) > So rather than describe specific bugs on RedHat/Debian maybe just say: > > This guards against cases where jgit is present on the system, but > will fail to run, e.g. because of some JRE issue, or missing Java > dependencies. Seeing if it gets far enough to process the > "--version" argument isn't perfect, but seems to be good enough in > practice. It's also consistent with how we detect some other > dependencies, see e.g. the CURL and UNZIP prerequisites. Well said. I indeed avoided putting the detail into the commit message because it was such a Fedora-specific bug. I'll update the commit message to add more details though, borrowing liberally from^W^W^Wperhaps stealing your suggested wording. Thanks! -- Todd
[PATCH] test-lib: try harder to ensure a working jgit
The JGIT prereq uses 'type jgit' to determine whether jgit is present. While this should be sufficient, if the jgit found is broken we'll waste time running tests which fail due to no fault of our own. Use 'jgit --version' instead, to catch some badly broken jgit installations. Signed-off-by: Todd Zullinger --- I ran into such a broken jgit on Fedora >= 30¹. This is clearly a problem in the Fedora jgit package which will hopefully be resolved soon. But it may be good to avoid wasting time debugging tests which fail due to a broken tool outside of our control. ¹ https://bugzilla.redhat.com/1709624 t/test-lib.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/t/test-lib.sh b/t/test-lib.sh index 908ddb9c46..599fd70e14 100644 --- a/t/test-lib.sh +++ b/t/test-lib.sh @@ -1522,7 +1522,7 @@ test_lazy_prereq NOT_ROOT ' ' test_lazy_prereq JGIT ' - type jgit + jgit --version ' # SANITY is about "can you correctly predict what the filesystem would -- Todd
Re: [PATCH] doc/ls-files: put nested list for "-t" option into block
Hi, Jeff King wrote: > The description for the "-t" option contains a sub-list of all of the > possible file status outputs. But because of the newline separating that > list from the description paragraph, asciidoc treats the sub-list > entries as a continuation of the overall options list, rather than as > children of the "-t" description. > > We could fix it by adding a "+" before the sub-list to connect it to the > rest of the "-t" text. But using a pair of "--" to delimit the block is > perhaps more readable, and may have better compatibility with > asciidoctor, as in 39a869b2f2 (Documentation/rev-list-options: wrap > --date= block with "--", 2019-03-30). > > The extra blank line comes from 5bc0e247c4 (Document ls-files -t as > semi-obsolete., 2010-07-28), but the problem actually seems older than > that. Before then, we did: > > -t:: some text... > H:: cached > M:: unmerged > etc... > > but asciidoc also treats that as one big list. So this problem seems to > have been around forever. > > Signed-off-by: Jeff King > --- > Junio: I happened to notice this while hunting for "ls-files" options >that could make your makefile de-dup patch unnecessary (but >didn't find anything). > > Todd: Just an FYI that your "--" strategy is spreading. :) Heh, cool. This is an obviously simple fix, but for good measure I checked the results with asciidoc 8.6.10 as well as asciidoctor 1.5.6 and 2.0.8. The output from each of them looks good. -- Todd
Re: What's cooking in git.git (Apr 2019, #02; Wed, 10)
Hi,
Junio C Hamano wrote:
> * tz/doc-apostrophe-no-longer-needed (2019-04-08) 1 commit
> - Documentation/git-show-branch: drop last use of {apostrophe}
>
> Doc formatting fix.
>
> Will merge to 'next'.
>
>
> * tz/git-svn-doc-markup-fix (2019-04-08) 1 commit
> - Documentation/git-svn: improve asciidoctor compatibility
>
> Doc formatting fix.
>
> Needs a better description.
> cf.
I sent a v2 as <20190410003734.17124-1-...@pobox.com> which
(I hope) improves upon both commit messages.
Thanks,
--
Todd
Re: [PATCH 2/2] Documentation/git-svn: improve asciidoctor compatibility
Martin Ågren wrote: > I think what's happening could be related to the fix in the first patch. > There, it's ok to explicitly escape only the first '. The second one is > matched to it and gets escaped implicitly. Something like that could be > happening here, too, just that we don't want it to. (Should we escape > the implicit escaping? ...) Just speculation, though. It could well be a similar issue. I think I tried adding an escape to the existing \* and/or to elsewhere in the block of text, without success. But I didn't try very hard though, as adjusting the test to use `*` seemed like an improvement. Thanks, -- Todd
[PATCH v2 0/2] a few more minor asciidoc/tor formatting fixes
Hi,
Martin Ågren wrote:
> On Sat, 6 Apr 2019 at 00:51, Todd Zullinger wrote:
>> Here's what I have currently. I haven't tested this much with
>> asciidoctor-1.5.x releases (or maybe not at all -- it's been a
>> week or so since I worked on this).
>
> Tested with Asciidoctor 1.5.5. For both patches, AsciiDoctor stumbles
> before the patch, but not after it. Great. :-) For AsciiDoc 8.6.10, the
> first patch is a no-op, while the second patch makes the difference it
> intends to do. I'll follow up with comments on the individual patches.
Thanks for testing these against older Asciidoctor and for the
helpful feedback on the commit messages.
I reworded the message in the first commit to make it clearer
that Asciidoctor renders the {apostrophe} literally. I updated
the body of the second commit using your suggested wording. It
was much better than the original. :)
The contents of each commit remain unchanged.
Todd Zullinger (2):
Documentation/git-show-branch: avoid literal {apostrophe}
Documentation/git-svn: improve asciidoctor compatibility
Documentation/git-show-branch.txt | 2 +-
Documentation/git-svn.txt | 6 +++---
2 files changed, 4 insertions(+), 4 deletions(-)
Range-diff against v1:
1: 70e3339859 ! 1: da553a596c Documentation/git-show-branch: drop last use of
{apostrophe}
@@ -1,14 +1,17 @@
Author: Todd Zullinger
-Documentation/git-show-branch: drop last use of {apostrophe}
+Documentation/git-show-branch: avoid literal {apostrophe}
The {apostrophe} was needed at the time of a521845800 ("Documentation:
remove stray backslash in show-branch discussion", 2010-08-20). All
other uses of {apostrophe} were removed in 6cf378f0cb ("docs: stop
using
asciidoc no-inline-literal", 2012-04-26).
-Escape only the leading single-quote. This renders properly in
asciidoc
-and asciidoctor.
+Unfortunately, the {apostrophe} is rendered literally with Asciidoctor
+(at least with 1.5.5-2.0.3). Avoid this by using single-quotes.
+
+Escaping the leading single-quote allows the content to render properly
+ in AsciiDoc and Asciidoctor.
Signed-off-by: Todd Zullinger
2: e8f6f873bc ! 2: 6fd412bd97 Documentation/git-svn: improve asciidoctor
compatibility
@@ -3,8 +3,18 @@
Documentation/git-svn: improve asciidoctor compatibility
The second paragraph in the CONFIGURATION section intends to emphasize
-the word 'must' with bold type. Adjust the formatting slightly to
-provide similar results between asciidoc and asciidoctor.
+the word 'must' with bold type. It does so by writing it as *must*, and
+this works fine with AsciiDoc. It usually works great with Asciidoctor,
+too, but in this particular instance, we have another "*" earlier in
the
+paragraph. We do escape it, and it is rendered literally just like we
+want it to, but Asciidoctor then ends up tripping on the second (or
+third) of the asterisks in this paragraph.
+
+Since that asterisk is (part of) a literal example, we can set it in
+monospace, by giving it as `*`. Adjust the whole paragraph in this way.
+There's lots more monospacing to be done in this document, but since
our
+main motivation is addressing AsciiDoc/Asciidoctor discrepancies like
+this one, let's just convert this one paragraph.
Signed-off-by: Todd Zullinger
--
Todd
[PATCH v2 1/2] Documentation/git-show-branch: avoid literal {apostrophe}
The {apostrophe} was needed at the time of a521845800 ("Documentation:
remove stray backslash in show-branch discussion", 2010-08-20). All
other uses of {apostrophe} were removed in 6cf378f0cb ("docs: stop using
asciidoc no-inline-literal", 2012-04-26).
Unfortunately, the {apostrophe} is rendered literally with Asciidoctor
(at least with 1.5.5-2.0.3). Avoid this by using single-quotes.
Escaping the leading single-quote allows the content to render properly
in AsciiDoc and Asciidoctor.
Signed-off-by: Todd Zullinger
---
Documentation/git-show-branch.txt | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/Documentation/git-show-branch.txt
b/Documentation/git-show-branch.txt
index 4a01371227..5cc2fcefba 100644
--- a/Documentation/git-show-branch.txt
+++ b/Documentation/git-show-branch.txt
@@ -167,7 +167,7 @@ $ git show-branch master fixes mhf
These three branches all forked from a common commit, [master],
-whose commit message is "Add {apostrophe}git show-branch{apostrophe}".
+whose commit message is "Add \'git show-branch'".
The "fixes" branch adds one commit "Introduce "reset type" flag to
"git reset"". The "mhf" branch adds many other commits.
The current branch is "master".
[PATCH v2 2/2] Documentation/git-svn: improve asciidoctor compatibility
The second paragraph in the CONFIGURATION section intends to emphasize the word 'must' with bold type. It does so by writing it as *must*, and this works fine with AsciiDoc. It usually works great with Asciidoctor, too, but in this particular instance, we have another "*" earlier in the paragraph. We do escape it, and it is rendered literally just like we want it to, but Asciidoctor then ends up tripping on the second (or third) of the asterisks in this paragraph. Since that asterisk is (part of) a literal example, we can set it in monospace, by giving it as `*`. Adjust the whole paragraph in this way. There's lots more monospacing to be done in this document, but since our main motivation is addressing AsciiDoc/Asciidoctor discrepancies like this one, let's just convert this one paragraph. Signed-off-by: Todd Zullinger --- Documentation/git-svn.txt | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/Documentation/git-svn.txt b/Documentation/git-svn.txt index b99029520d..81aaef8e4e 100644 --- a/Documentation/git-svn.txt +++ b/Documentation/git-svn.txt @@ -1100,10 +1100,10 @@ listed below are allowed: tags = tags/*/project-a:refs/remotes/project-a/tags/* -Keep in mind that the '\*' (asterisk) wildcard of the local ref -(right of the ':') *must* be the farthest right path component; +Keep in mind that the `*` (asterisk) wildcard of the local ref +(right of the `:`) *must* be the farthest right path component; however the remote wildcard may be anywhere as long as it's an -independent path component (surrounded by '/' or EOL). This +independent path component (surrounded by `/` or EOL). This type of configuration is not automatically created by 'init' and should be manually entered with a text-editor or using 'git config'.
[PATCH 2/2] Documentation/git-svn: improve asciidoctor compatibility
The second paragraph in the CONFIGURATION section intends to emphasize the word 'must' with bold type. Adjust the formatting slightly to provide similar results between asciidoc and asciidoctor. Signed-off-by: Todd Zullinger --- I debated changing 'must' from bold to italic in this hunk. There are two other emphasized uses of 'must' in git-svn.txt. One is bold and one is italic. So I left this one bold and simply adjusted the "'" quotes to "`" around the "*" and ":" characters. This is the more minimal fix to keep the output rendered the same in asciidoc and asciidoctor. Documentation/git-svn.txt | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/Documentation/git-svn.txt b/Documentation/git-svn.txt index b99029520d..81aaef8e4e 100644 --- a/Documentation/git-svn.txt +++ b/Documentation/git-svn.txt @@ -1100,10 +1100,10 @@ listed below are allowed: tags = tags/*/project-a:refs/remotes/project-a/tags/* -Keep in mind that the '\*' (asterisk) wildcard of the local ref -(right of the ':') *must* be the farthest right path component; +Keep in mind that the `*` (asterisk) wildcard of the local ref +(right of the `:`) *must* be the farthest right path component; however the remote wildcard may be anywhere as long as it's an -independent path component (surrounded by '/' or EOL). This +independent path component (surrounded by `/` or EOL). This type of configuration is not automatically created by 'init' and should be manually entered with a text-editor or using 'git config'.
[PATCH 0/2] a few more minor asciidoc/tor formatting fixes
Hi,
Martin Ågren wrote:
> On Fri, 5 Apr 2019 at 03:40, Todd Zullinger wrote:
>> There are two other changes I've got queued locally. One in
>> git-show-branch.txt removes the last use of {apostrophe}.
>> Another in git-svn.txt is a bit of a work-around for a
>> difference in the way asciidoc and asciidoctor parse the
>> second paragraph in the CONFIGURATION section. That may
>> well be an asiidoctor bug, but it seems like one we can
>> adjust for without much effort.
>
> The second one looks like it can be fixed by using `*` instead of '\*',
> which I think is more correct anyway. I don't know what your local
> workaround looks like, but I think a patch like "use backticks
> consistently" (both change to them, in a number of places, and add them,
> where we currently have nothing) would be a good change by itself, and
> we could note that "BTW, this fixes ...". How does that compare to what
> you have?
That's exactly what I have -- except that I only changed the
parts needed to improve compatibility between asciidoc and
asciidoctor. So my commit message justifies it differently.
You're probably right that a more general cleanup could be done
and then we get better asciidoctor compatibilty as a consequence.
I was trying to keep from taking too many tangents to avoid
making more work (both for myself and reviewers). :)
Here's what I have currently. I haven't tested this much with
asciidoctor-1.5.x releases (or maybe not at all -- it's been a
week or so since I worked on this).
Todd Zullinger (2):
Documentation/git-show-branch: drop last use of {apostrophe}
Documentation/git-svn: improve asciidoctor compatibility
Documentation/git-show-branch.txt | 2 +-
Documentation/git-svn.txt | 6 +++---
2 files changed, 4 insertions(+), 4 deletions(-)
[PATCH 1/2] Documentation/git-show-branch: drop last use of {apostrophe}
The {apostrophe} was needed at the time of a521845800 ("Documentation:
remove stray backslash in show-branch discussion", 2010-08-20). All
other uses of {apostrophe} were removed in 6cf378f0cb ("docs: stop using
asciidoc no-inline-literal", 2012-04-26).
Escape only the leading single-quote. This renders properly in asciidoc
and asciidoctor.
Signed-off-by: Todd Zullinger
---
Maybe it would be easier to change the example commit messages
and avoid having to nest single quotes within double quotes? I
don't know if that's much preferable to escaping only the opening
single quote.
I went with the more minimal change to avoid having to rewrite
other bits of the example (and risk making them not match what
users would see if they ran similar commands).
This is another potential parsing bug in asciidoctor. Of course,
distros will have versions of asciidoctor in place for some time
which have trouble parsing this doc. Since it's not much work
for us to adjust the text to work around it, that seemed
reasonable.
Documentation/git-show-branch.txt | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/Documentation/git-show-branch.txt
b/Documentation/git-show-branch.txt
index 4a01371227..5cc2fcefba 100644
--- a/Documentation/git-show-branch.txt
+++ b/Documentation/git-show-branch.txt
@@ -167,7 +167,7 @@ $ git show-branch master fixes mhf
These three branches all forked from a common commit, [master],
-whose commit message is "Add {apostrophe}git show-branch{apostrophe}".
+whose commit message is "Add \'git show-branch'".
The "fixes" branch adds one commit "Introduce "reset type" flag to
"git reset"". The "mhf" branch adds many other commits.
The current branch is "master".
Re: [PATCH] asciidoctor-extensions: provide ``
brian m. carlson wrote: > On Sat, Mar 30, 2019 at 02:00:14PM -0400, Todd Zullinger wrote: >> Thanks for the very useful docbook5/xmlto details! >> >> The hard-coded use of the non-namespaced stylesheets in >> xmlto is unfortunate. I hadn't gotten far enough along to >> run into that limitation of xmlto, so many thanks for saving >> me from finding it myself. I'm sure it would have taken me >> far longer. >> >> If it turns out that docbook5 causes us more pain than it's >> worth, I suppose we could choose to use the builtin manpage >> backend when using asciidoctor >= 2. > > I suspect this will be the easiest way forward. If we produce > semantically equivalent manpages, then this is also a lot nicer for > people who would prefer not to have a full XML toolchain installed. It's a good end goal. There are a number of differences in the output when using the manpage backend directly verus docbook and then xmlto. The way links to external html documents are presented is the main one which comes to mind. Maybe we can adjust that via asciidoctor-extensions.rb or something, I don't know. Elsewhere in this thread, Jeff made the very valid point that we're probably wise to keep using the docbook/xmlto chain as long as we're supporting both asciidoc and asciidoctor. Unless it turns out that it's more work to coax asciidoctor (and the various 1.5 and 2.0 releases in common use) to work with that same docbook/xmlto chain than it is to do it directly, that is. >> Or we could see about adding a docbook45 converter, which >> upstream noted as a possibility in the ticket¹ which dropped >> docbook45 by default. > > Another possibility, depending on how responsive the xmlto upstream is, > is to add some sort of DocBook 5 support to it. This shouldn't be > terribly difficult, although we'd have to disable validation. DocBook 5 > uses RELAX NG, and libxml2/xmllint's support for this has some bugs, > such that it will mark some valid documents using complex interleaves as > invalid. Still, if this is the direction we want to go, I have no > problem adding this support. > > Since we'd have a quite new Asciidoctor and a new xmlto, distros should > have both around the same time. Good point. It can't hurt to push for improvements across the tools. (Well, other than time being a limited resource and time you may spend on doc tools being time you can't spend on the hash conversion, which I imagine is a more interesting project.) Thanks, -- Todd
Re: [PATCH v1 0/2] minor asciidoc/tor formatting fixes
Hi,
Martin Ågren wrote:
> On Sat, 30 Mar 2019 at 19:30, Todd Zullinger wrote:
>>
>> Just chipping away at the remaining differences between asciidoc and
>> asciidoctor.
>>
>> Todd Zullinger (2):
>> Documentation/rev-list-options: wrap --date= block with "--"
>> Documentation/git-status: fix titles in porcelain v2 section
>
> Nice. I've tested and diffed across various dimensions. Looks good to me.
Thanks for testing! On pu we're down to a fairly small
amount of differences now. Most of what remains are
whitespace changes; some of which I would like to bring up
to the Asciidoctor team to see if they're intentional.
There are two other changes I've got queued locally. One in
git-show-branch.txt removes the last use of {apostrophe}.
Another in git-svn.txt is a bit of a work-around for a
difference in the way asciidoc and asciidoctor parse the
second paragraph in the CONFIGURATION section. That may
well be an asiidoctor bug, but it seems like one we can
adjust for without much effort.
--
Todd
Re: [PATCH v1 2/2] Documentation/git-status: fix titles in porcelain v2 section
Hi,
Jeff Hostetler wrote:
> On 3/30/2019 2:30 PM, Todd Zullinger wrote:
>> The '^### ' lines were added in 1cd828ddc8 ("git-status.txt: describe
>> --porcelain=v2 format", 2016-08-11). I'm _presuming_ they were made
>> with markdown syntax in mind, but if not I can drop that bit from the
>> commit message. Jeff H, do you happen to recall?
>
> Yes, I was probably had markdown on the brain that day.
Cool, thanks. Lucky guess on my part then. It's not like
I've ever done something similar in other repos. ;)
--
Todd
Re: [PATCH v1 2/2] Documentation/git-status: fix titles in porcelain v2 section
Jeff King wrote: > On Sat, Mar 30, 2019 at 02:30:01PM -0400, Todd Zullinger wrote: > >> Asciidoc uses either one-line or two-line syntax for document/section >> titles[1]. The two-line form is used in git-status. Fix a few section >> titles in the porcelain v2 section which were inadvertently using >> markdown syntax. > > Yep, makes sense. One observation: > >> -### Branch Headers >> +Branch Headers >> +^^ > > The one-line equivalent in asciidoc would be something like: > > === Branch Headers > > but that's actually a "level 2" header (because it counts from zero), > whereas "^" underlining is a "level 3" header. But I think "^" is right > here, because this is under level 2 "~" header. Yeah, since there were a number of existing two-line headers in the document, I thought it would be better to simply update these to that form than convert the others. We have far more of the two-line form too, so it's more consistent with the existing docs. >> As an aside, while I was reading the Asciidoc/tor manuals, I notice the >> two-line title syntax was not mentioned in Asciidoctor. That seems to >> be because Asciidoctor has suggested the two-line title format should be >> deprecated, as discussed at: >> >> https://github.com/asciidoctor/asciidoctor/issues/418 >> >> I'm not sure how likely that is to occur. With the 2.0 release, >> asciidoctor plans to use semantic versioning, so I would not expect any >> deprecation to happen before at least 2.1. It would only affect use >> without compat-mode. > > I think it's probably fine to punt on this until we see some actual > movement upstream on the deprecation / removal. No doubt. I'm sure that would be a long deprecation period. > One side note. The original asciidoc user guide says one-line headers > have equals on either side, like: > > === Branch Headers === > > but that one can omit the trailing delimiter. The asciidoctor reference > just suggests using the one-sided: > > === Branch Headers Interesting. I didn't notice the matching right hand side while I was looking at the original asciidoc manual. > So presumably if we do want to convert, we would just go with the > one-sided version. Seems like a good rule. I presume that when in doubt, we should look to the Asciidoctor reference for the current best practice. Thanks, -- Todd
Re: [PATCH v1 1/2] Documentation/rev-list-options: wrap --date= block with "--"
Hi, Jeff King wrote: > On Sat, Mar 30, 2019 at 02:30:00PM -0400, Todd Zullinger wrote: > >> Using "+" to continue multiple list items is more tedious and >> error-prone than wrapping the entire block with "--" block markers. >> >> When using asciidoctor, the list items after the --date=iso list items >> are incorrectly formatted when using "+" continuation. Use "--" block >> markers to correctly format the block. >> >> When using asciidoc there is no change in how the content is rendered. > > This seems like an asciidoctor bug, though I think this kind of > list-within-a-list stuff is inherently a bit heuristic-driven just due > to the syntax. Indeed. There's certainly a limit to the changes we want to make solely to work-around issues in either asciidoc or asciidoctor. When the work-around is (at least arguably) an improvement, then it's probably worthwhile. That's how I thought about it, anyway. :) > I do agree that the result after your patch is more readable, so I think > I prefer it even if the asciidoctor bug were fixed. I suspect we could > be using "--" blocks for readability in more places (I don't think it's > worth going on a hunt to convert old spots, but something to keep in > mind as we write new documentation). Agreed, that sounds perfectly reasonable to me. The Asciidoctor user manual says: If you’re attaching more than one block to a list item, you’re strongly encouraged to wrap the content inside an open block. That way, you only need a single list continuation line to attach the open block to the list item. Within the open block, you write like you normally would, no longer having to worry about adding list continuations between the blocks to keep them attached to the list item. https://asciidoctor.org/docs/user-manual/#list-continuation I imagine it's "strongly encouraged" both to help consumers avoid these sort of oddly-parsed continuation issues, as well as the Asciidoctor devs from having to field as many bug reports. -- Todd
Re: What's cooking in git.git (Apr 2019, #01; Thu, 4)
Hi Junio,
Junio C Hamano wrote:
> * sg/asciidoctor-in-ci (2019-04-01) 6 commits
> - ci: fix AsciiDoc/Asciidoctor stderr check in the documentation build job
> - ci: stick with Asciidoctor v1.5.8 for now
> - ci: install Asciidoctor in 'ci/install-dependencies.sh'
> - Documentation/technical/protocol-v2.txt: fix formatting
> - Documentation/technical/api-config.txt: fix formatting
> - Documentation/git-diff-tree.txt: fix formatting
>
> Update our support to format documentation in the CI environment,
> either with AsciiDoc ro Asciidoctor.
>
> Will merge to 'next'.
Martin mentioned this in reply to the patch thread¹ but it
looks like it slipped by unnoticed. There's some extraneous
comments in 28216d13f4 ("ci: stick with Asciidoctor v1.5.8
for now", 2019-03-29) which would be good to trim before
this hits next.
commit 28216d13f43b07e41bdd83b786ae31c00c657e06
Author: SZEDER Gábor
Date: Fri Mar 29 20:52:46 2019 +0100
ci: stick with Asciidoctor v1.5.8 for now
On Fri, Mar 29, 2019 at 01:35:19PM +0100, SZEDER Gábor wrote:
> The release of Asciidoctor v2.0.0 two days ago broke our documentation
Well, what happened "two days ago" when I sent v2 is now seven days
ago... Let's just say "recent" instead.
--- >8 ---
Subject: ci: stick with Asciidoctor v1.5.8 for now
The recent release of Asciidoctor v2.0.0 broke our documentation
...
¹
Thanks,
--
Todd
[PATCH v1 1/2] Documentation/rev-list-options: wrap --date= block with "--"
Using "+" to continue multiple list items is more tedious and
error-prone than wrapping the entire block with "--" block markers.
When using asciidoctor, the list items after the --date=iso list items
are incorrectly formatted when using "+" continuation. Use "--" block
markers to correctly format the block.
When using asciidoc there is no change in how the content is rendered.
Signed-off-by: Todd Zullinger
---
The issue this fixes can be seen in the git-log and git-rev-list docs on
git-scm.com:
https://git-scm.com/docs/git-log#Documentation/git-log.txt---dateltformatgt
https://git-scm.com/docs/git-rev-list#Documentation/git-rev-list.txt---dateltformatgt
Documentation/rev-list-options.txt | 22 +++---
1 file changed, 11 insertions(+), 11 deletions(-)
diff --git a/Documentation/rev-list-options.txt
b/Documentation/rev-list-options.txt
index ca959a7286..7b415dff82 100644
--- a/Documentation/rev-list-options.txt
+++ b/Documentation/rev-list-options.txt
@@ -805,12 +805,13 @@ include::pretty-options.txt[]
author's). If `-local` is appended to the format (e.g.,
`iso-local`), the user's local time zone is used instead.
+
+--
`--date=relative` shows dates relative to the current time,
e.g. ``2 hours ago''. The `-local` option has no effect for
`--date=relative`.
-+
+
`--date=local` is an alias for `--date=default-local`.
-+
+
`--date=iso` (or `--date=iso8601`) shows timestamps in a ISO 8601-like format.
The differences to the strict ISO 8601 format are:
@@ -818,15 +819,14 @@ The differences to the strict ISO 8601 format are:
- a space between time and time zone
- no colon between hours and minutes of the time zone
-+
`--date=iso-strict` (or `--date=iso8601-strict`) shows timestamps in strict
ISO 8601 format.
-+
+
`--date=rfc` (or `--date=rfc2822`) shows timestamps in RFC 2822
format, often found in email messages.
-+
+
`--date=short` shows only the date, but not the time, in `-MM-DD` format.
-+
+
`--date=raw` shows the date as seconds since the epoch (1970-01-01
00:00:00 UTC), followed by a space, and then the timezone as an offset
from UTC (a `+` or `-` with four digits; the first two are hours, and
@@ -835,28 +835,28 @@ with `strftime("%s %z")`).
Note that the `-local` option does not affect the seconds-since-epoch
value (which is always measured in UTC), but does switch the accompanying
timezone value.
-+
+
`--date=human` shows the timezone if the timezone does not match the
current time-zone, and doesn't print the whole date if that matches
(ie skip printing year for dates that are "this year", but also skip
the whole date itself if it's in the last few days and we can just say
what weekday it was). For older dates the hour and minute is also
omitted.
-+
+
`--date=unix` shows the date as a Unix epoch timestamp (seconds since
1970). As with `--raw`, this is always in UTC and therefore `-local`
has no effect.
-+
+
`--date=format:...` feeds the format `...` to your system `strftime`,
except for %z and %Z, which are handled internally.
Use `--date=format:%c` to show the date in your system locale's
preferred format. See the `strftime` manual for a complete list of
format placeholders. When using `-local`, the correct syntax is
`--date=format-local:...`.
-+
+
`--date=default` is the default format, and is similar to
`--date=rfc2822`, with a few exceptions:
-
+--
- there is no comma after the day-of-week
- the time zone is omitted when the local time zone is used
[PATCH v1 0/2] minor asciidoc/tor formatting fixes
Just chipping away at the remaining differences between asciidoc and asciidoctor. Todd Zullinger (2): Documentation/rev-list-options: wrap --date= block with "--" Documentation/git-status: fix titles in porcelain v2 section Documentation/git-status.txt | 12 Documentation/rev-list-options.txt | 22 +++--- 2 files changed, 19 insertions(+), 15 deletions(-)
[PATCH v1 2/2] Documentation/git-status: fix titles in porcelain v2 section
Asciidoc uses either one-line or two-line syntax for document/section
titles[1]. The two-line form is used in git-status. Fix a few section
titles in the porcelain v2 section which were inadvertently using
markdown syntax.
[1] http://asciidoc.org/userguide.html#X17
Signed-off-by: Todd Zullinger
---
The '^### ' lines were added in 1cd828ddc8 ("git-status.txt: describe
--porcelain=v2 format", 2016-08-11). I'm _presuming_ they were made
with markdown syntax in mind, but if not I can drop that bit from the
commit message. Jeff H, do you happen to recall?
As an aside, while I was reading the Asciidoc/tor manuals, I notice the
two-line title syntax was not mentioned in Asciidoctor. That seems to
be because Asciidoctor has suggested the two-line title format should be
deprecated, as discussed at:
https://github.com/asciidoctor/asciidoctor/issues/418
I'm not sure how likely that is to occur. With the 2.0 release,
asciidoctor plans to use semantic versioning, so I would not expect any
deprecation to happen before at least 2.1. It would only affect use
without compat-mode.
Documentation/git-status.txt | 12
1 file changed, 8 insertions(+), 4 deletions(-)
diff --git a/Documentation/git-status.txt b/Documentation/git-status.txt
index 861d821d7f..d4e8f24f0c 100644
--- a/Documentation/git-status.txt
+++ b/Documentation/git-status.txt
@@ -278,7 +278,8 @@ Header lines start with "#" and are added in response to
specific
command line arguments. Parsers should ignore headers they
don't recognize.
-### Branch Headers
+Branch Headers
+^^
If `--branch` is given, a series of header lines are printed with
information about the current branch.
@@ -294,7 +295,8 @@ Line Notes
-### Changed Tracked Entries
+Changed Tracked Entries
+^^^
Following the headers, a series of lines are printed for tracked
entries. One of three different line formats may be used to describe
@@ -365,7 +367,8 @@ Field Meaning
-### Other Items
+Other Items
+^^^
Following the tracked entries (and if requested), a series of
lines will be printed for untracked and then ignored items
@@ -379,7 +382,8 @@ Ignored items have the following format:
!
-### Pathname Format Notes and -z
+Pathname Format Notes and -z
+
When the `-z` option is given, pathnames are printed as is and
without any quoting and lines are terminated with a NUL (ASCII 0x00)
Re: [PATCH v3 04/11] gc docs: include the "gc.*" section from "config" in "gc"
Hi Ævar, Ævar Arnfjörð Bjarmason wrote: > diff --git a/Documentation/git-gc.txt b/Documentation/git-gc.txt > index 66386439b7..c037a46b09 100644 > --- a/Documentation/git-gc.txt > +++ b/Documentation/git-gc.txt > @@ -45,28 +45,12 @@ OPTIONS > --auto:: > With this option, 'git gc' checks whether any housekeeping is > required; if not, it exits without performing any work. > - Some git commands run `git gc --auto` after performing > - operations that could create many loose objects. Housekeeping > - is required if there are too many loose objects or too many > - packs in the repository. > + > -If the number of loose objects exceeds the value of the `gc.auto` > -configuration variable, then all loose objects are combined into a > -single pack. Setting the value of `gc.auto` > -to 0 disables automatic packing of loose objects. > +See the `gc.auto' option in the "CONFIGURATION" section below for how > +this heuristic works. Did you want this "gc.auto" to use the differing left and right accent/quote characters (which asciidoc renders as single-quotes and asciidoctor as double-quotes) or should the closing "'" instead be "`" to render "gc.auto" as monospaced text? I suspect it's the latter, as that matches most of the other variable names in the docs. I noticed this while comparing the output from asciidoc and asciidoctor. I've got a few similar changes queued up as minor fixes to lower the diff between asciidoc/tor but I wanted to check whether you intended this one before I sent a patch to correct it. :) Thanks, -- Todd
Re: [PATCH] asciidoctor-extensions: provide ``
Hi, brian m. carlson wrote: > On Tue, Mar 26, 2019 at 09:06:03PM -0400, Todd Zullinger wrote: >> There's still the matter of 2.0 dropping docbook45. I'll >> try to get around to testing 1.5.x releases with docbook5 to >> see if they work reasonably well. If not, we can add a >> version check and set ASCIIDOC_DOCBOOK appropriately. >> >> One other issue that crops up with docbook5 is that the >> xmlto toolchain now outputs: >> >> Note: namesp. cut : stripped namespace before processing >> >> as documented at >> >> https://docbook.org/docs/howto/howto.html#dbxsl >> >> It's mostly an annoyance which we either want to strip out, >> or pass an alternate docbook5 xsl, I think. But I'm not >> very familiar with the guts of the xml/docbook toolchains. > > I'm quite familiar with this area, because I used DocBook and its > stylesheets for my college papers. There are two sets of stylesheets, > the namespaced ones (for DocBook 5) and the non-namespaced ones (for > DocBook 4). They can be distinguished because the URLs (and typically > the paths) use "xsl" (for non-namespaced) or "xsl-ns" (for namespaced). > > xmlto by default uses the older ones, and assuming you have a reasonably > capable XSLT processor, either set of stylesheets can be used, since > they simply transform the document into the right type (although with a > warning, as you've noticed). > > Unfortunately, xmlto is hard-coded to use the non-namespaced stylesheets > with no option to specify which to use, and it doesn't appear to be > actively developed. There's an option to specify the stylesheet (-x), > but it can't take a URL, since xmlto "helpfully" fully qualifies the > path. That means we'd need to know the location on disk of those > stylesheets in order to use them, since we can't rely on catalog > resolution. Thanks for the very useful docbook5/xmlto details! The hard-coded use of the non-namespaced stylesheets in xmlto is unfortunate. I hadn't gotten far enough along to run into that limitation of xmlto, so many thanks for saving me from finding it myself. I'm sure it would have taken me far longer. If it turns out that docbook5 causes us more pain than it's worth, I suppose we could choose to use the builtin manpage backend when using asciidoctor >= 2. Or we could see about adding a docbook45 converter, which upstream noted as a possibility in the ticket¹ which dropped docbook45 by default. It'll be some time before we can reasonably expect to only support asciidoctor-2.x. We'll have to see what method involves the least ugly hacks to support asciidoc and the various 1.5.x and 2.x versions of asciidoctor. ¹ https://github.com/asciidoctor/asciidoctor/issues/3005 -- Todd
Re: [PATCH] asciidoctor-extensions: provide ``
Hi, I wrote: > Jeff King wrote: >> That seems like a bug in asciidoctor, which ought to be quoting the "<". >> We certainly can't quote it ourselves (we don't even know that our >> backend output is going to a format that needs angle brackets quoted). > > Yep, it seems so. I filed this upstream: > > https://github.com/asciidoctor/asciidoctor/issues/3205 > > I updated to asciidoctor-2.0.1 this morning to test, in case > it was one of the issues fixed since the 2.0.0 release. > Alas, we're the first to hit it and report it. Dan Allen fixed this upstream and released 2.0.2 today. It's very good to know that asciidoctor upstream is incredibly responsive. If anyone runs into Dan at a conference, please buy him a beer. ;) There's still the matter of 2.0 dropping docbook45. I'll try to get around to testing 1.5.x releases with docbook5 to see if they work reasonably well. If not, we can add a version check and set ASCIIDOC_DOCBOOK appropriately. One other issue that crops up with docbook5 is that the xmlto toolchain now outputs: Note: namesp. cut : stripped namespace before processing as documented at https://docbook.org/docs/howto/howto.html#dbxsl It's mostly an annoyance which we either want to strip out, or pass an alternate docbook5 xsl, I think. But I'm not very familiar with the guts of the xml/docbook toolchains. -- Todd
Re: [PATCH] asciidoctor-extensions: provide ``
Hi, Jeff King wrote: > On Sun, Mar 24, 2019 at 12:21:31PM -0400, Todd Zullinger wrote: >> I'm curious why manpage builds work for you and not for me. > > I think it's because I'm an idiot. I must have only been using 2.0.0 > when I was looking at the XML output manually (for the refmiscinfo > lines), and never actually rendered it to roff. I get the same problem > when I try a full build. Ahh. I was hoping you'd tell me that I was a fool. :) >> On my fedora 29 test system, ASCIIDOC_DOCBOOK=docbook5 leads >> to a validation failure from xmlto, since docbook5 doesn't >> use a DTD¹. I added XMLTO_EXTRA = --skip-validation to the >> USE_ASCIIDOCTOR block, which can build many of the man >> pages, but fails when it gets to git-blame due to use of >> literal < > characters in the xml: >> >> git-blame.xml:423: parser error : StartTag: invalid element name >> <40-byte hex sha1> >> < >>^ > > That seems like a bug in asciidoctor, which ought to be quoting the "<". > We certainly can't quote it ourselves (we don't even know that our > backend output is going to a format that needs angle brackets quoted). Yep, it seems so. I filed this upstream: https://github.com/asciidoctor/asciidoctor/issues/3205 I updated to asciidoctor-2.0.1 this morning to test, in case it was one of the issues fixed since the 2.0.0 release. Alas, we're the first to hit it and report it. -- Todd
Re: [PATCH] asciidoctor-extensions: provide ``
Hi, Jeff King wrote: > On Sat, Mar 23, 2019 at 03:27:56PM -0400, Todd Zullinger wrote: > >> I updated my system to asciidoctor-2.0.0 last night and now >> I can't even generate the man pages properly, because the >> docbook45 converter was removed. I'll have to see if I >> missed some other required update. :/ > > I ran into that, too. Using ASCIIDOC_DOCBOOK=docbook5 worked. The output > looked OK to me, but I only eyeballed it briefly. It's possible there > are subtle problems. Interesting. I did that last night as well, but it only led to more errors. I'm curious why manpage builds work for you and not for me. On my fedora 29 test system, ASCIIDOC_DOCBOOK=docbook5 leads to a validation failure from xmlto, since docbook5 doesn't use a DTD¹. I added XMLTO_EXTRA = --skip-validation to the USE_ASCIIDOCTOR block, which can build many of the man pages, but fails when it gets to git-blame due to use of literal < > characters in the xml: git-blame.xml:423: parser error : StartTag: invalid element name <40-byte hex sha1> < ^ While this may be a real issue in the formatting of git-blame.txt which we should fix, I'm suspicious of that due to the number of other files similarly affected: git-blame.txt git-diff-files.txt git-diff-index.txt git-diff-tree.txt git-diff.txt git-format-patch.txt git-http-fetch.txt git-log.txt git-rebase.txt git-rev-list.txt git-show.txt git-svn.txt ¹ Here's the failure I get without --skip-validation: [tmz@f29 Documentation]$ make USE_ASCIIDOCTOR=1 git.1 SUBDIR ../ make[1]: 'GIT-VERSION-FILE' is up to date. ASCIIDOC git.xml XMLTO git.1 xmlto: /home/tmz/src/git/Documentation/git.xml does not validate (status 3) xmlto: Fix document syntax or use --skip-validation option validity error : no DTD found! Document /home/tmz/src/git/Documentation/git.xml does not validate make: *** [Makefile:359: git.1] Error 13 Thanks, -- Todd
Re: [PATCH] asciidoctor-extensions: provide ``
Hi,
Martin Ågren wrote:
> On Wed, 20 Mar 2019 at 19:17, Todd Zullinger wrote:
>> Martin Ågren wrote:
>>> {litdd} now renders as --. We should find some other way to
>>> produce '--'. This should then be a simple change, as we're already
>>> providing this attribute inside an `ifdef USE_ASCIIDOCTOR`.
>>
>> I noticed that one and didn't work out a good fix, but it
>> sounds like you have one in mind. That's great.
>>
>>> "+" becomes "+". I didn't immediately find where we do that.
>>
>> For this one, I was working on replacing "{plus}" with `+`
>> (along with " " and "-"). That's probably not ideal though.
>
> The "plus" and "litdd" issues seem like they can be solved by doing:
>
> ASCIIDOC_EXTRA += -aplus='+'
> ASCIIDOC_EXTRA += -alitdd='\--'
Hmm, setting litdd makes the html generate an em dash
followed by a zero width space (in 1.5.8 and 2.0.0)
I updated my system to asciidoctor-2.0.0 last night and now
I can't even generate the man pages properly, because the
docbook45 converter was removed. I'll have to see if I
missed some other required update. :/
> It would probably be worthwhile to try 1.5.7+ to see how much that
> improves things. Seems like you're already underway there.
Yeah, before I knew how soon 2.0.0 was coming, I updated to
1.5.8 and built the various Fedora packages which require it
to see how they handled it. Almost all of the changes were
fixes to bugs in previous versions. And the one which was
not is likely to be fixed in 2.0.0 according to asciidoctor
maintainer Dan Allen.
Have you looked at diffing the html output as well? It
seems like we'll need to check it as well to be sure any
fixes to the manpage output don't have a negative impact on
the html output, and vice versa.
I used 'links -dump' output for comparison of the various
Fedora packages which currently build with asciidoctor.
It's not perfect. It could miss visual changes which might
be important when viewed in a graphical browser. But it was
better than trying to diff the html files directly. :)
We probably also want to check the validity of links within
the docs, as one of the changes in 1.5.8 caused breakage of
cross interdocument references. (This is the change I noted
above which should be fixed in 2.0.0.)
It'll be quite a while until most systems with asciidoctor
1.5.x are gone. I doubt that upgrading to even 1.5.8 is
suitable for the currently released Fedora versions due to
incompatible changes. I am going to try and get 2.0.0 into
Fedora 30, but the deadline for changes has just passed, so
I may not be able to do so. If not, it'll be 6-8 months
until a released version of Fedora has an asciidoctor newer
than 1.5.6.1.
All that is just to say that even if newer asciidoctor fixes
many of the issues we've seen it will still be a long time
before we can reasonable default to asciidoctor or drop
asciidoc support.
For what it's worth, the Fedora asciidoc packages moved to
python3 using https://github.com/asciidoc/asciidoc-py3.
That's not very active, but it should at least keep asciidoc
alive beyond the approaching python2 EOL.
--
Todd
Re: [PATCH v3 0/4] completion.commands: fix multiple command removals
Junio C Hamano wrote: > Todd Zullinger writes: > >> Other than a follow-up to update the commit reference in 4/4 >> after 1/4 is in the final form on pu, I think this might be good. >> If it's easier, we can skip 4/4 and I'll resend it after the >> others are on pu. > > A series that makes a single topic should expect to be read by a > reader who understand the context of individual pieces in the topic, > so it is more common to refer to an earlier step of a series from a > later step without the object name. I would have written the log > message like so instead: > > completion: use __git when calling --list-cmds > > As we made --list-cmds read the local configuration file in an > earlier step, the completion.commands variable respects repo-level > configuration. Use __git which ensures that the proper repo config > is consulted if the command line contains 'git -C /some/other/repo'. > > The whole series looks good to me. Thanks for working on it. Thank you for amending the commit message, and to Jeff, Duy, Eric, and Gábor for all the help. -- Todd
Re: [PATCH] asciidoctor-extensions: provide ``
Jeff King wrote: > On Tue, Mar 19, 2019 at 08:12:54AM +0100, Martin Ågren wrote: > >>> I just tried with asciidoc 2.0.0.rc.2, which came out last week. It does >>> seem to work from the command line: >>> >>> $ make USE_ASCIIDOCTOR=Yes \ >>> ASCIIDOC_DOCBOOK=docbook5 \ >>> ASCIIDOC='asciidoctor -amansource=Git -amanmanual="Git Manual"' \ >>> git-add.xml >>> $ sed -n '/refmeta/,/refmeta/p' git-add.xml >>> >>> git-add >>> 1 >>> Git >>> Git Manual >>> >> >> No such luck with asciidoctor 1.5.5. Seems like it really wants >> "manpage" before it considers these attributes. >> >> (That's still me holding the tool, so factor that into it.) > > The refmiscinfo stuff didn't arrive until asciidoctor v1.5.7. Now I don't feel as bad that I didn't find any good way to handle refmiscinfo when I first tried to use asciidoctor in November 2017 at least. This made me look closer at the fedora asciidoctor packages. They have been stuck on 1.5.6.1. So all my recent testing has been with 1.5.6.1. I spent some time yesterday working toward getting the fedora packages updated to 1.5.8. With luck that will reach the stable updates channels before too long. (I'm guessing the upcoming 2.0 release won't be suitable as an update for released fedora versions, being a major release with potentially backwards-incompatible changes.) There are differences in the output from 1.5.6.1 and 1.5.8, but I haven't looked closely yet. -- Todd
Re: [PATCH] asciidoctor-extensions: provide ``
Hi,
Martin Ågren wrote:
> On Sun, 17 Mar 2019 at 20:44, Todd Zullinger wrote:
>> Martin Ågren wrote:
>> +ASCIIDOC_EXTRA += -a mansource="Git $(GIT_VERSION)" -a manmanual="Git
>> Manual"
>
> So to be honest, I still don't understand how this works, but it does,
> great. I really need to improve my documentation-reading skills.
Let me know if you find any good methods for perfect
retention. I've re-read enough documentation for a
lifetime. ;)
> I had some more time to look at this. Thanks for getting started on this
> switch. A few things I noticed:
>
> {litdd} now renders as --. We should find some other way to
> produce '--'. This should then be a simple change, as we're already
> providing this attribute inside an `ifdef USE_ASCIIDOCTOR`.
I noticed that one and didn't work out a good fix, but it
sounds like you have one in mind. That's great.
> "+" becomes "+". I didn't immediately find where we do that.
For this one, I was working on replacing "{plus}" with `+`
(along with " " and "-"). That's probably not ideal though.
This is what I had to test:
-- 8< --
Subject: [PATCH] WIP: wrap "[ +-]" in backticks (NOT FOR SUBMISSION)
asciidoctor's manpage output html-encodes "{plus}" which seems like a
bug. At the least it needs some option I've yet to learn.
---
Documentation/git-add.txt | 26 +-
Documentation/gitweb.txt | 6 +++---
2 files changed, 16 insertions(+), 16 deletions(-)
diff --git a/Documentation/git-add.txt b/Documentation/git-add.txt
index 37bcab94d5..dc1dd3a91b 100644
--- a/Documentation/git-add.txt
+++ b/Documentation/git-add.txt
@@ -363,20 +363,20 @@ may see in a patch, and which editing operations make
sense on them.
--
added content::
-Added content is represented by lines beginning with "{plus}". You can
+Added content is represented by lines beginning with `"+"`. You can
prevent staging any addition lines by deleting them.
removed content::
-Removed content is represented by lines beginning with "-". You can
-prevent staging their removal by converting the "-" to a " " (space).
+Removed content is represented by lines beginning with `"-"`. You can
+prevent staging their removal by converting the `"-"` to a `" "` (space).
modified content::
-Modified content is represented by "-" lines (removing the old content)
-followed by "{plus}" lines (adding the replacement content). You can
-prevent staging the modification by converting "-" lines to " ", and
-removing "{plus}" lines. Beware that modifying only half of the pair is
+Modified content is represented by `"-"` lines (removing the old content)
+followed by `"+"` lines (adding the replacement content). You can
+prevent staging the modification by converting `"-"` lines to `" "`, and
+removing `"+"` lines. Beware that modifying only half of the pair is
likely to introduce confusing changes to the index.
--
@@ -393,29 +393,29 @@ Avoid using these constructs, or do so with extreme
caution.
removing untouched content::
Content which does not differ between the index and working tree may be
-shown on context lines, beginning with a " " (space). You can stage
-context lines for removal by converting the space to a "-". The
+shown on context lines, beginning with a `" "` (space). You can stage
+context lines for removal by converting the space to a `"-"`. The
resulting working tree file will appear to re-add the content.
modifying existing content::
One can also modify context lines by staging them for removal (by
-converting " " to "-") and adding a "{plus}" line with the new content.
-Similarly, one can modify "{plus}" lines for existing additions or
+converting `" "` to `"-"`) and adding a `"+"` line with the new content.
+Similarly, one can modify `"+"` lines for existing additions or
modifications. In all cases, the new modification will appear reverted
in the working tree.
new content::
You may also add new content that does not exist in the patch; simply
-add new lines, each starting with "{plus}". The addition will appear
+add new lines, each starting with `"+"`. The addition will appear
reverted in the working tree.
--
There are also several operations which should be avoided entirely, as
they will make the patch impossible to apply:
-* adding context (" ") or removal ("-") lines
+* adding context (`" "`) or removal (`"-"`) lines
* deleting context or removal lines
* modifying the contents o
[PATCH v3 1/4] git: read local config in --list-cmds
From: Jeff King
Normally code that is checking config before we've decided to do
setup_git_directory() would use read_early_config(), which uses
discover_git_directory() to tentatively see if we're in a repo,
and if so to add it to the config sequence.
But list_cmds() uses the caching configset mechanism which
rightly does not use read_early_config(), because it has no
idea if it's being called early.
Call setup_git_directory_gently() so we can pick up repo-level
config (like completion.commands).
Signed-off-by: Jeff King
---
git.c | 7 +++
help.c | 7 ---
2 files changed, 7 insertions(+), 7 deletions(-)
diff --git a/git.c b/git.c
index 2dd588674f..10e49d79f6 100644
--- a/git.c
+++ b/git.c
@@ -62,6 +62,13 @@ static int list_cmds(const char *spec)
{
struct string_list list = STRING_LIST_INIT_DUP;
int i;
+ int nongit;
+
+ /*
+ * Set up the repository so we can pick up any repo-level config (like
+ * completion.commands).
+ */
+ setup_git_directory_gently(&nongit);
while (*spec) {
const char *sep = strchrnul(spec, ',');
diff --git a/help.c b/help.c
index 520c9080e8..fac7e421d0 100644
--- a/help.c
+++ b/help.c
@@ -375,13 +375,6 @@ void list_cmds_by_config(struct string_list *list)
{
const char *cmd_list;
- /*
-* There's no actual repository setup at this point (and even
-* if there is, we don't really care; only global config
-* matters). If we accidentally set up a repository, it's ok
-* too since the caller (git --list-cmds=) should exit shortly
-* anyway.
-*/
if (git_config_get_string_const("completion.commands", &cmd_list))
return;
[PATCH v3 4/4] completion: use __git when calling --list-cmds
Since e51bdea6d3 ("git: read local config in --list-cmds", 2019-03-01),
the completion.commands variable respects repo-level configuration. Use
__git which ensures that the proper repo config is consulted if the
command line contains 'git -C /some/other/repo'.
Suggested-by: SZEDER Gábor
Signed-off-by: Todd Zullinger
---
contrib/completion/git-completion.bash | 8
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/contrib/completion/git-completion.bash
b/contrib/completion/git-completion.bash
index 499e56f83d..e505f04ff7 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -1010,7 +1010,7 @@ __git_all_commands=
__git_compute_all_commands ()
{
test -n "$__git_all_commands" ||
- __git_all_commands=$(git --list-cmds=main,others,alias,nohelpers)
+ __git_all_commands=$(__git --list-cmds=main,others,alias,nohelpers)
}
# Lists all set config variables starting with the given section prefix,
@@ -1620,9 +1620,9 @@ _git_help ()
esac
if test -n "$GIT_TESTING_ALL_COMMAND_LIST"
then
- __gitcomp "$GIT_TESTING_ALL_COMMAND_LIST $(git
--list-cmds=alias,list-guide) gitk"
+ __gitcomp "$GIT_TESTING_ALL_COMMAND_LIST $(__git
--list-cmds=alias,list-guide) gitk"
else
- __gitcomp "$(git --list-cmds=main,nohelpers,alias,list-guide)
gitk"
+ __gitcomp "$(__git --list-cmds=main,nohelpers,alias,list-guide)
gitk"
fi
}
@@ -2888,7 +2888,7 @@ __git_main ()
then
__gitcomp "$GIT_TESTING_PORCELAIN_COMMAND_LIST"
else
- __gitcomp "$(git
--list-cmds=list-mainporcelain,others,nohelpers,alias,list-complete,config)"
+ __gitcomp "$(__git
--list-cmds=list-mainporcelain,others,nohelpers,alias,list-complete,config)"
fi
;;
esac
[PATCH v3 3/4] completion: fix multiple command removals
From: Jeff King
Commit 6532f3740b ("completion: allow to customize the completable
command list", 2018-05-20) tried to allow multiple space-separated
entries in completion.commands. To do this, it copies each parsed token
into a strbuf so that the result is NUL-terminated.
However, for tokens starting with "-", it accidentally passes the
original non-terminated string, meaning that only the final one worked.
Switch to using the strbuf.
Reported-by: Todd Zullinger
Signed-off-by: Jeff King
---
help.c| 4 ++--
t/t9902-completion.sh | 2 +-
2 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/help.c b/help.c
index fac7e421d0..a9e451f2ee 100644
--- a/help.c
+++ b/help.c
@@ -386,8 +386,8 @@ void list_cmds_by_config(struct string_list *list)
const char *p = strchrnul(cmd_list, ' ');
strbuf_add(&sb, cmd_list, p - cmd_list);
- if (*cmd_list == '-')
- string_list_remove(list, cmd_list + 1, 0);
+ if (sb.buf[0] == '-')
+ string_list_remove(list, sb.buf + 1, 0);
else
string_list_insert(list, sb.buf);
strbuf_release(&sb);
diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
index 3f5b420bf8..050fac4fff 100755
--- a/t/t9902-completion.sh
+++ b/t/t9902-completion.sh
@@ -1483,7 +1483,7 @@ test_expect_success 'git --help completion' '
test_completion "git --help core" "core-tutorial "
'
-test_expect_failure 'completion.commands removes multiple commands' '
+test_expect_success 'completion.commands removes multiple commands' '
test_config completion.commands "-cherry -mergetool" &&
git --list-cmds=list-mainporcelain,list-complete,config >out &&
! grep -E "^(cherry|mergetool)$" out
[PATCH v3 2/4] t9902: test multiple removals via completion.commands
6532f3740b ("completion: allow to customize the completable command
list", 2018-05-20) added the completion.commands config variable.
Multiple commands may be added or removed, separated by a space.
Demonstrate the failure of multiple removals.
Signed-off-by: Todd Zullinger
---
t/t9902-completion.sh | 6 ++
1 file changed, 6 insertions(+)
diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
index 3a2c6326d8..3f5b420bf8 100755
--- a/t/t9902-completion.sh
+++ b/t/t9902-completion.sh
@@ -1483,6 +1483,12 @@ test_expect_success 'git --help completion' '
test_completion "git --help core" "core-tutorial "
'
+test_expect_failure 'completion.commands removes multiple commands' '
+ test_config completion.commands "-cherry -mergetool" &&
+ git --list-cmds=list-mainporcelain,list-complete,config >out &&
+ ! grep -E "^(cherry|mergetool)$" out
+'
+
test_expect_success 'setup for integration tests' '
echo content >file1 &&
echo more >file2 &&
[PATCH v3 0/4] completion.commands: fix multiple command removals
Hi,
Duy Nguyen wrote:
> You probably want to drop the comment block about repository setup
> inside list_cmds_by_config() too.
You're right, of course. Thanks Duy. :)
That's the only change since v2.
Other than a follow-up to update the commit reference in 4/4
after 1/4 is in the final form on pu, I think this might be good.
If it's easier, we can skip 4/4 and I'll resend it after the
others are on pu.
Jeff King (2):
git: read local config in --list-cmds
completion: fix multiple command removals
Todd Zullinger (2):
t9902: test multiple removals via completion.commands
completion: use __git when calling --list-cmds
contrib/completion/git-completion.bash | 8
git.c | 7 +++
help.c | 11 ++-
t/t9902-completion.sh | 6 ++
4 files changed, 19 insertions(+), 13 deletions(-)
Range-diff against v2:
1: e51bdea6d3 ! 1: 6e9872b0e3 git: read local config in --list-cmds
@@ -33,3 +33,21 @@
while (*spec) {
const char *sep = strchrnul(spec, ',');
+
+ diff --git a/help.c b/help.c
+ --- a/help.c
+ +++ b/help.c
+@@
+ {
+ const char *cmd_list;
+
+- /*
+- * There's no actual repository setup at this point (and even
+- * if there is, we don't really care; only global config
+- * matters). If we accidentally set up a repository, it's ok
+- * too since the caller (git --list-cmds=) should exit shortly
+- * anyway.
+- */
+ if (git_config_get_string_const("completion.commands", &cmd_list))
+ return;
+
2: 2f5e9da9de = 2: 6873ae3868 t9902: test multiple removals via
completion.commands
3: 7548dcc23f = 3: f66bbc0b55 completion: fix multiple command removals
4: 26bef0b2af = 4: 197b176483 completion: use __git when calling --list-cmds
[PATCH] t4038-diff-combined: quote paths with whitespace
d76ce4f734 ("log,diff-tree: add --combined-all-paths option",
2019-02-07) added tests for files containing tabs.
When the tests are run with bash, the lack of quoting during the file
setup causes 'ambiguous redirect' errors.
Signed-off-by: Todd Zullinger
---
Hi,
I noticed these failures while running in a repo where I happened
to have TEST_SHELL_PATH=/bin/bash set. I wonder if we should
have a test matrix which uses bash to catch these sort of things?
t/t4038-diff-combined.sh | 8
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/t/t4038-diff-combined.sh b/t/t4038-diff-combined.sh
index 07b49f6d6d..d4afe12554 100755
--- a/t/t4038-diff-combined.sh
+++ b/t/t4038-diff-combined.sh
@@ -480,18 +480,18 @@ test_expect_success FUNNYNAMES 'setup for
--combined-all-paths with funny names'
git branch side1d &&
git branch side2d &&
git checkout side1d &&
- test_seq 1 10 >$(printf "file\twith\ttabs") &&
+ test_seq 1 10 >"$(printf "file\twith\ttabs")" &&
git add file* &&
git commit -m with &&
git checkout side2d &&
- test_seq 1 9 >$(printf "i\tam\ttabbed") &&
- echo ten >>$(printf "i\tam\ttabbed") &&
+ test_seq 1 9 >"$(printf "i\tam\ttabbed")" &&
+ echo ten >>"$(printf "i\tam\ttabbed")" &&
git add *tabbed &&
git commit -m iam &&
git checkout -b funny-names-mergery side1d &&
git merge --no-commit side2d &&
git rm *tabs &&
- echo eleven >>$(printf "i\tam\ttabbed") &&
+ echo eleven >>"$(printf "i\tam\ttabbed")" &&
git mv "$(printf "i\tam\ttabbed")" "$(printf "fickle\tnaming")" &&
git add fickle* &&
git commit
--
Todd
Re: [PATCH] asciidoctor-extensions: provide ``
Hi Martin,
Martin Ågren wrote:
> When we build with AsciiDoc, asciidoc.conf ensures that each xml-file we
> generate contains some meta-information which `xmlto` can act on, based
> on the following template:
>
>
> {mantitle}
> {manvolnum}
> Git
> {git_version}
> Git Manual
>
>
> When we build with Asciidoctor, it does not honor this configuration file
> and we end up with only this (for a hypothetical git-foo.xml):
>
>
> git-foo
> 1
>
>
> That is, we miss out on the `` tags. As a result, the
> header of each man page doesn't say "Git Manual", but "git-foo(1)"
> instead. Worse, the footers don't give the Git version number and
> instead provide the fairly ugly "[FIXME: source]".
>
> That Asciidoctor ignores asciidoc.conf is nothing new. This is why we
> implement the `linkgit:` macro in asciidoc.conf *and* in
> asciidoctor-extensions.rb. Follow suit and provide these tags in
> asciidoctor-extensions.rb, using a "postprocessor" extension.
Nice! I looked at this a long time ago and didn't figure
out how to use a postprocessor extension. From my notes, I
found discussions about using ruby's tilt for templating and
it all seemed way too convoluted.
Your method looks quite simple and elegant.
> We may consider a few alternatives:
>
> * Provide the `mansource` attribute to Asciidoctor. This attribute
> looks promising until one realizes that it can only be given inside
> the source file (the .txt file in our case), *not* on the command
> line using `-a mansource=foobar`. I toyed with the idea of injecting
> this attribute while feeding Asciidoctor the input on stdin, but it
> didn't feel like it was worth the complexity in the Makefile.
I played with this direction before. Using Asciidoctor we
can convert directly from .txt to man without docbook
and xmlto. That does have some other issues which need to
be worked out though. Here's what I had as a start:
-- 8< --
Subject: [PATCH] WIP: doc: improve asciidoctor manpage generation
Avoid 'FIXME: Source' by setting mansource. Skip xmlto step and render
manpages directly with asciidoctor.
TODO:
- apply to all man pages
- fix links to html docs, like user-manual.html in git.1 (currently
it is listed in brackets inline rather than as a footnote)
Reference:
https://lore.kernel.org/lkml/20180424150456.17353-1-ti...@suse.de/
Signed-off-by: Todd Zullinger
---
Documentation/Makefile | 8
Documentation/asciidoctor-extensions.rb | 2 ++
2 files changed, 10 insertions(+)
diff --git a/Documentation/Makefile b/Documentation/Makefile
index a9697f5146..494f8c9464 100644
--- a/Documentation/Makefile
+++ b/Documentation/Makefile
@@ -197,6 +197,7 @@ ASCIIDOC_DOCBOOK = docbook45
ASCIIDOC_EXTRA += -acompat-mode -atabsize=8
ASCIIDOC_EXTRA += -I. -rasciidoctor-extensions
ASCIIDOC_EXTRA += -alitdd='&\#x2d;&\#x2d;'
+ASCIIDOC_EXTRA += -a mansource="Git $(GIT_VERSION)" -a manmanual="Git Manual"
DBLATEX_COMMON =
endif
@@ -354,9 +355,16 @@ $(OBSOLETE_HTML): %.html : %.txto asciidoc.conf
manpage-base-url.xsl: manpage-base-url.xsl.in
$(QUIET_GEN)sed "s|@@MAN_BASE_URL@@|$(MAN_BASE_URL)|" $< > $@
+ifndef USE_ASCIIDOCTOR
%.1 %.5 %.7 : %.xml manpage-base-url.xsl $(wildcard manpage*.xsl)
$(QUIET_XMLTO)$(RM) $@ && \
$(XMLTO) -m $(MANPAGE_XSL) $(XMLTO_EXTRA) man $<
+else
+%.1 %.5 %.7 : %.txt
+ $(QUIET_ASCIIDOC)$(RM) $@+ $@ && \
+ $(ASCIIDOC_COMMON) -b manpage -d manpage -o $@+ $< && \
+ mv $@+ $@
+endif
%.xml : %.txt asciidoc.conf asciidoctor-extensions.rb
$(QUIET_ASCIIDOC)$(RM) $@+ $@ && \
diff --git a/Documentation/asciidoctor-extensions.rb
b/Documentation/asciidoctor-extensions.rb
index f7a5982f8b..ebb078807a 100644
--- a/Documentation/asciidoctor-extensions.rb
+++ b/Documentation/asciidoctor-extensions.rb
@@ -12,6 +12,8 @@ module Git
if parent.document.basebackend? 'html'
prefix = parent.document.attr('git-relative-html-prefix')
%(#{target}(#{attrs[1]})\n)
+elsif parent.document.basebackend? 'manpage'
+ "#{target}(#{attrs[1]})"
elsif parent.document.basebackend? 'docbook'
"\n" \
"#{target}" \
-- 8< --
That was based on ma/asciidoctor-extensions, but it may be
missing other recent improvements you've made to the make
targets. It's been a month or so since I worked on it.
I munged up doc-diff to set MANDWIDTH=1000 and set one
branch to default to asciidoctor to compare. (Your other
recent series looks like it'll make doing asciidoc and
asciidoctor comparisons easier.)
There were a number of differences that I didn't work
through though. Most importantly was the change in the
links noted in the commit message.
Thanks for working on asciidoctor. I've been trying to poke
it off and on to help ensure the docs can be built if
asciidoc ever gets dropped from Fedora.
--
Todd
[PATCH v2 3/4] completion: fix multiple command removals
From: Jeff King
Commit 6532f3740b ("completion: allow to customize the completable
command list", 2018-05-20) tried to allow multiple space-separated
entries in completion.commands. To do this, it copies each parsed token
into a strbuf so that the result is NUL-terminated.
However, for tokens starting with "-", it accidentally passes the
original non-terminated string, meaning that only the final one worked.
Switch to using the strbuf.
Reported-by: Todd Zullinger
Signed-off-by: Jeff King
---
help.c| 4 ++--
t/t9902-completion.sh | 2 +-
2 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/help.c b/help.c
index 520c9080e8..026f881715 100644
--- a/help.c
+++ b/help.c
@@ -393,8 +393,8 @@ void list_cmds_by_config(struct string_list *list)
const char *p = strchrnul(cmd_list, ' ');
strbuf_add(&sb, cmd_list, p - cmd_list);
- if (*cmd_list == '-')
- string_list_remove(list, cmd_list + 1, 0);
+ if (sb.buf[0] == '-')
+ string_list_remove(list, sb.buf + 1, 0);
else
string_list_insert(list, sb.buf);
strbuf_release(&sb);
diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
index 3f5b420bf8..050fac4fff 100755
--- a/t/t9902-completion.sh
+++ b/t/t9902-completion.sh
@@ -1483,7 +1483,7 @@ test_expect_success 'git --help completion' '
test_completion "git --help core" "core-tutorial "
'
-test_expect_failure 'completion.commands removes multiple commands' '
+test_expect_success 'completion.commands removes multiple commands' '
test_config completion.commands "-cherry -mergetool" &&
git --list-cmds=list-mainporcelain,list-complete,config >out &&
! grep -E "^(cherry|mergetool)$" out
[PATCH v2 0/4] completion.commands: fix multiple command removals
Hi,
Duy Nguyen wrote:
> Poking this thread before it goes completely dead...
I've been meaning to send out a re-rolled version. I didn't
realize 2 weeks had slipped by already. :/
> On Sat, Mar 2, 2019 at 12:34 AM Todd Zullinger wrote:
>>
>> The completion.commands config var must be set in either the system-wide
>> or global config file. The completion script does not read a local
>> repository config.
>
> After the last discussion, I think the consensus was it's ok to allow
> per-repo config so we can just drop this and update the code to read
> from any config file, right?
Yeah. Here's an updated series which I hope sums up the changes
from the discussion.
Changes since v1:
- Change --list-commands to respect local config and use
test_config rather than test_config_global
- Avoid git upstream of pipes
- Check that both cherry and mergetool are removed
- Use __git in completion calls which use --list-cmds, to
ensure the proper git repo config is checked with git -C
/some/other/repo
Jeff King (2):
git: read local config in --list-cmds
completion: fix multiple command removals
Todd Zullinger (2):
t9902: test multiple removals via completion.commands
completion: use __git when calling --list-cmds
contrib/completion/git-completion.bash | 8
git.c | 7 +++
help.c | 4 ++--
t/t9902-completion.sh | 6 ++
4 files changed, 19 insertions(+), 6 deletions(-)
Range-diff against v1:
1: 385c596510 < -: -- doc: note config file restrictions for
completion.commands
-: -- > 1: e51bdea6d3 git: read local config in --list-cmds
2: ffa5ed9780 ! 2: 2f5e9da9de t9902: test multiple removals via
completion.commands
@@ -18,11 +18,9 @@
'
+test_expect_failure 'completion.commands removes multiple commands' '
-+ echo cherry-pick >expected &&
-+ test_config_global completion.commands "-cherry -mergetool" &&
-+ git --list-cmds=list-mainporcelain,list-complete,config |
-+ grep ^cherry >actual &&
-+ test_cmp expected actual
++ test_config completion.commands "-cherry -mergetool" &&
++ git --list-cmds=list-mainporcelain,list-complete,config >out &&
++ ! grep -E "^(cherry|mergetool)$" out
+'
+
test_expect_success 'setup for integration tests' '
3: af029e908d ! 3: 7548dcc23f completion: fix multiple command removals
@@ -2,16 +2,16 @@
completion: fix multiple command removals
-6532f3740b ("completion: allow to customize the completable
-command list", 2018-05-20) added the completion.commands config
-variable.
+Commit 6532f3740b ("completion: allow to customize the completable
+command list", 2018-05-20) tried to allow multiple space-separated
+entries in completion.commands. To do this, it copies each parsed token
+into a strbuf so that the result is NUL-terminated.
-The documentation states multiple commands may be added,
-separated by spaces. Adding multiple commands to remove fails,
-only removing the last command in the config.
-
-Fix multiple command removals.
+However, for tokens starting with "-", it accidentally passes the
+original non-terminated string, meaning that only the final one worked.
+Switch to using the strbuf.
+Reported-by: Todd Zullinger
Signed-off-by: Jeff King
diff --git a/help.c b/help.c
@@ -38,6 +38,6 @@
-test_expect_failure 'completion.commands removes multiple commands' '
+test_expect_success 'completion.commands removes multiple commands' '
- echo cherry-pick >expected &&
- test_config_global completion.commands "-cherry -mergetool" &&
- git --list-cmds=list-mainporcelain,list-complete,config |
+ test_config completion.commands "-cherry -mergetool" &&
+ git --list-cmds=list-mainporcelain,list-complete,config >out &&
+ ! grep -E "^(cherry|mergetool)$" out
-: -- > 4: 26bef0b2af completion: use __git when calling --list-cmds
[PATCH v2 1/4] git: read local config in --list-cmds
From: Jeff King
Normally code that is checking config before we've decided to do
setup_git_directory() would use read_early_config(), which uses
discover_git_directory() to tentatively see if we're in a repo,
and if so to add it to the config sequence.
But list_cmds() uses the caching configset mechanism which
rightly does not use read_early_config(), because it has no
idea if it's being called early.
Call setup_git_directory_gently() so we can pick up repo-level
config (like completion.commands).
Signed-off-by: Jeff King
---
git.c | 7 +++
1 file changed, 7 insertions(+)
diff --git a/git.c b/git.c
index 2dd588674f..10e49d79f6 100644
--- a/git.c
+++ b/git.c
@@ -62,6 +62,13 @@ static int list_cmds(const char *spec)
{
struct string_list list = STRING_LIST_INIT_DUP;
int i;
+ int nongit;
+
+ /*
+ * Set up the repository so we can pick up any repo-level config (like
+ * completion.commands).
+ */
+ setup_git_directory_gently(&nongit);
while (*spec) {
const char *sep = strchrnul(spec, ',');
[PATCH v2 2/4] t9902: test multiple removals via completion.commands
6532f3740b ("completion: allow to customize the completable command
list", 2018-05-20) added the completion.commands config variable.
Multiple commands may be added or removed, separated by a space.
Demonstrate the failure of multiple removals.
Signed-off-by: Todd Zullinger
---
t/t9902-completion.sh | 6 ++
1 file changed, 6 insertions(+)
diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
index 3a2c6326d8..3f5b420bf8 100755
--- a/t/t9902-completion.sh
+++ b/t/t9902-completion.sh
@@ -1483,6 +1483,12 @@ test_expect_success 'git --help completion' '
test_completion "git --help core" "core-tutorial "
'
+test_expect_failure 'completion.commands removes multiple commands' '
+ test_config completion.commands "-cherry -mergetool" &&
+ git --list-cmds=list-mainporcelain,list-complete,config >out &&
+ ! grep -E "^(cherry|mergetool)$" out
+'
+
test_expect_success 'setup for integration tests' '
echo content >file1 &&
echo more >file2 &&
[PATCH v2 4/4] completion: use __git when calling --list-cmds
Since e51bdea6d3 ("git: read local config in --list-cmds", 2019-03-01),
the completion.commands variable respects repo-level configuration. Use
__git which ensures that the proper repo config is consulted if the
command line contains 'git -C /some/other/repo'.
Suggested-by: SZEDER Gábor
Signed-off-by: Todd Zullinger
---
Junio, I referenc the commit id for "git: read local config in
--list-cmds" which is earlier in this series, so the id will, of
course, differ when you apply it. Let me know if you'd prefer
this commit to be dropped and resent once the others in the
series are applied or if it's easy for you to adjust when it's
queued.
Also, as I wrote in an earlier reply, at the moment, I think
using __git only matters for calls where config is in the
--list-cmds list. But since Jeff's fix affects all --list-cmds
calls, it seems prudent to adjust the 3 other uses of --list-cmds
in the completion script.
contrib/completion/git-completion.bash | 8
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/contrib/completion/git-completion.bash
b/contrib/completion/git-completion.bash
index 499e56f83d..e505f04ff7 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -1010,7 +1010,7 @@ __git_all_commands=
__git_compute_all_commands ()
{
test -n "$__git_all_commands" ||
- __git_all_commands=$(git --list-cmds=main,others,alias,nohelpers)
+ __git_all_commands=$(__git --list-cmds=main,others,alias,nohelpers)
}
# Lists all set config variables starting with the given section prefix,
@@ -1620,9 +1620,9 @@ _git_help ()
esac
if test -n "$GIT_TESTING_ALL_COMMAND_LIST"
then
- __gitcomp "$GIT_TESTING_ALL_COMMAND_LIST $(git
--list-cmds=alias,list-guide) gitk"
+ __gitcomp "$GIT_TESTING_ALL_COMMAND_LIST $(__git
--list-cmds=alias,list-guide) gitk"
else
- __gitcomp "$(git --list-cmds=main,nohelpers,alias,list-guide)
gitk"
+ __gitcomp "$(__git --list-cmds=main,nohelpers,alias,list-guide)
gitk"
fi
}
@@ -2888,7 +2888,7 @@ __git_main ()
then
__gitcomp "$GIT_TESTING_PORCELAIN_COMMAND_LIST"
else
- __gitcomp "$(git
--list-cmds=list-mainporcelain,others,nohelpers,alias,list-complete,config)"
+ __gitcomp "$(__git
--list-cmds=list-mainporcelain,others,nohelpers,alias,list-complete,config)"
fi
;;
esac
Re: One failed self test on Fedora 29
Hi, Jeffrey Walton wrote: > Fedora 29, x86_64. One failed self test: > > *** t0021-conversion.sh *** [...] > not ok 13 - disable filter with empty override > # > # test_config_global filter.disable.smudge false && > # test_config_global filter.disable.clean false && > # test_config filter.disable.smudge false && > # test_config filter.disable.clean false && > # > # echo "*.disable filter=disable" >.gitattributes && > # > # echo test >test.disable && > # git -c filter.disable.clean= add test.disable 2>err && > # test_must_be_empty err && > # rm -f test.disable && > # git -c filter.disable.smudge= checkout -- test.disable 2>err > && > # test_must_be_empty err > # [...] > # failed 1 among 26 test(s) > 1..26 > gmake[2]: *** [Makefile:56: t0021-conversion.sh] Error 1 > > Does anyone need a config.log or other test data? It would probably help to know what commit you're building. The verbose test output would also be useful, e.g.: cd t && ./t0021-conversion.sh -v -i If it's not reliably reproducible, the --stress* options might help catch a failing run. FWIW, I just built and ran the tests on a Fedora 29 container for master, next, and pu a few times (some with various --stress options) without any test failures. I did this with and without a config.mak from the fedora git packages. I've never used the configure script, it seems like unnecessary overhead. $ git branch -v master 6e0cc67761 Start 2.22 cycle next 541d9dca55 Merge branch 'yb/utf-16le-bom-spellfix' into next * pu 7eadd8ba98 Merge branch 'js/remote-curl-i18n' into pu -- Todd
Re: [BUG] completion.commands does not remove multiple commands
SZEDER Gábor wrote: [... lots of good history ...] Thanks for an excellent summary! > Note, however, that for completeness sake, if we choose to update > list_cmds_by_config() to read the repo's config as well, then we > should also update the completion script to run $(__git > --list-cmds=...), note the two leading underscores, so in case of 'git > -C over/there ' it would read 'completion.commands' from the right > repository. Thanks for pointing this out. I'll add that to my local branch for the "respect local config" case. At the moment, I think it only matters for calls where config is in the --list-cmds list. But since the fix Jeff suggested affects all --list-cmds calls, it seems prudent to adjust the 3-4 other uses of --list-cmds in the completion script. Let me know if you see a reason not to do that. Thanks again for such a nice summary, -- Todd
Re: [BUG] completion.commands does not remove multiple commands
Hi, Junio C Hamano wrote: > Todd Zullinger writes: > >> Hmm. The comments in list_cmds_by_config() made me wonder >> if not using a local repo config was intentional: >> >> /* >> * There's no actual repository setup at this point (and even >> * if there is, we don't really care; only global config >> * matters). If we accidentally set up a repository, it's ok >> * too since the caller (git --list-cmds=) should exit shortly >> * anyway. >> */ > > Doesn't the output from list-cmds-by-config get cached at the > completion script layer in $__git_blah variables, and the cached > values are not cleared when you chdir around? In testing, I didn't find any evidence of caching. Setting commands to be added and removed in the global and local configs worked reasonably. Duy's reply suggests that was considered but not implemented. I there were caching (and if it were tedious for the completion code to keep fresh between repos), then it would a bad plan to allow per-repo config. If there was a goal of adding such caching it might also make sense to avoid "fixing" the code here to allow per-repo config before it's known how that might affect such caching. It sounds like that's not something Duy is planning on for the near term though, so perhaps we're fine to allow local repo config here? As Duy mentioned, maybe some users with local aliases want to add them to the completion locally as well. If we choose to avoid local repo config then we can add a comment to the documetation like I had in 2/3. Maybe also update the comment in list_cmds_by_config() to note that we intentionally don't setup a repo -- or a similar comment in list_cmds(), where Jeff's 1/3 was adding setup_git_directory_gently(). I don't have a strong opinion either way. I more or less have the minor patches for either direction at this point. Thanks, -- Todd
Re: [BUG] completion.commands does not remove multiple commands
Jeff King wrote:
> On Fri, Mar 01, 2019 at 12:34:40PM -0500, Todd Zullinger wrote:
>
>> Jeff King wrote:
>>> I can reproduce your problem, though the test you included passes for me
>>> even with the current tip of master.
>>
>> Oh, hrm. I think the issue is that completion.commands needs to be
>> set in the global (or system-wide) config, via test_config_global
>> rather than the local repo config which test_config sets.
>>
>> In hindsight, that seems obvious. But it's probably worth noting
>> that where completion.commands is documented, for anyone who might
>> spend a few cycles trying to configure it on a per-repo basis before
>> realizing it doesn't work.
>
> I think this is actually a bug. Normally code that is checking config
> before we've decide to do setup_git_directory() would use
> read_early_config(), which uses discover_git_directory() to tentatively
> see if we're in a repo, and if so to add it to the config sequence.
>
> But this code uses the caching configset mechanism. And that code
> (rightly) does not use read_early_config(), because it has no idea if
> it's being called early or what.
>
> I think we probably ought to be doing something like this:
>
> diff --git a/git.c b/git.c
> index 2dd588674f..ba3690245e 100644
> --- a/git.c
> +++ b/git.c
> @@ -62,6 +62,13 @@ static int list_cmds(const char *spec)
> {
> struct string_list list = STRING_LIST_INIT_DUP;
> int i;
> + int nongit;
> +
> + /*
> + * Set up the repository so we can pick up any repo-level config (like
> + * completion.commands).
> + */
> + setup_git_directory_gently(&nongit);
>
> while (*spec) {
> const char *sep = strchrnul(spec, ',');
Hmm. The comments in list_cmds_by_config() made me wonder
if not using a local repo config was intentional:
/*
* There's no actual repository setup at this point (and even
* if there is, we don't really care; only global config
* matters). If we accidentally set up a repository, it's ok
* too since the caller (git --list-cmds=) should exit shortly
* anyway.
*/
Is the cost of setting up a repository something which might
noticeably slow down interactive completion? In my testing
today I haven't felt it, but I have loads of memory on this
system.
I did apply your change and that allows the test to use
test_config() rather than test_config_global(). The full
test suite passes, so the change doesn't trigger any new
issues we have covered by a test, at least.
If we wanted to respect local configs, how does this look?
-- 8< --
From: Jeff King
Subject: [PATCH] git: read local config in --list-cmds
Normally code that is checking config before we've decide to do
setup_git_directory() would use read_early_config(), which uses
discover_git_directory() to tentatively see if we're in a repo,
and if so to add it to the config sequence.
But list_cmds() uses the caching configset mechanism and
(rightly) does not use read_early_config(), because it has no
idea if it's being called early.
Call setup_git_directory_gently() so we can pick up repo-level
config (like completion.commands).
Signed-off-by: Jeff King
---
git.c | 7 +++
1 file changed, 7 insertions(+)
diff --git a/git.c b/git.c
index 2dd588674f..10e49d79f6 100644
--- a/git.c
+++ b/git.c
@@ -62,6 +62,13 @@ static int list_cmds(const char *spec)
{
struct string_list list = STRING_LIST_INIT_DUP;
int i;
+ int nongit;
+
+ /*
+ * Set up the repository so we can pick up any repo-level config (like
+ * completion.commands).
+ */
+ setup_git_directory_gently(&nongit);
while (*spec) {
const char *sep = strchrnul(spec, ',');
-- 8< --
--
Todd
Re: [PATCH 2/3] t9902: test multiple removals via completion.commands
SZEDER Gábor wrote:
> On Fri, Mar 01, 2019 at 01:22:52PM -0500, Eric Sunshine wrote:
>> On Fri, Mar 1, 2019 at 12:35 PM Todd Zullinger wrote:
>>> 6532f3740b ("completion: allow to customize the completable command
>>> list", 2018-05-20) added the completion.commands config variable.
>>> Multiple commands may be added or removed, separated by a space.
>>>
>>> Demonstrate the failure of multiple removals.
>>>
>>> Signed-off-by: Todd Zullinger
>>> ---
>>> diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
>>> @@ -1483,6 +1483,14 @@ test_expect_success 'git --help completion' '
>>> +test_expect_failure 'completion.commands removes multiple commands' '
>>> + echo cherry-pick >expected &&
>>> + test_config_global completion.commands "-cherry -mergetool" &&
>>> + git --list-cmds=list-mainporcelain,list-complete,config |
>>> + grep ^cherry >actual &&
>>> + test_cmp expected actual
>>> +'
>>
>> We normally avoid placing a Git command upstream of a pipe. Instead,
>> the Git command output would be redirected to a file and then the file
>> grep'd.
>
> Indeed.
Yes, that's a good point. And one I should have known from
reading it here more than once. Thanks to both of you.
>> However, in this case, you might consider simplifying the test
>> like this:
>>
>> test_expect_failure 'completion.commands removes multiple commands' '
>> test_config_global completion.commands "-cherry -mergetool" &&
>> git --list-cmds=list-mainporcelain,list-complete,config >actual &&
>> grep cherry-pick actual
>
> This wouldn't check what we want. The point is that the command
> 'cherry' is not listed, so it should be '! grep cherry$ actual'.
Heh, I had written a similar reply already, but then I got
side-tracked for a bit...
I think the grep needs to be negated, e.g.:
! grep ^cherry$ actual
Otherwise it succeeds without the fix, as cherry-pick is
expected to be in the --list-cmds output. (If we remove
the 'expected' file, it might also make sense to rename
'actual' to 'out' perhaps.)
> Furthermore, I think it would be prudent to check that 'mergetool' is
> not listed, either.
True. With the simplified test, that's easy enough to add
and makes the test description more accurate and the test
more thorough. I'll adjust it like so when I re-send:
test_expect_failure 'completion.commands removes multiple commands' '
test_config completion.commands "-cherry -mergetool" &&
git --list-cmds=list-mainporcelain,list-complete,config >out &&
! grep -E "^(cherry|mergetool)$" out
(Using test_config depends on setup_git_directory_gently() in
list_cmds(), which Jeff suggested elsewhere in the thread.)
Thanks!
--
Todd
[PATCH 3/3] completion: fix multiple command removals
From: Jeff King
6532f3740b ("completion: allow to customize the completable
command list", 2018-05-20) added the completion.commands config
variable.
The documentation states multiple commands may be added,
separated by spaces. Adding multiple commands to remove fails,
only removing the last command in the config.
Fix multiple command removals.
Signed-off-by: Jeff King
---
Jeff,
The commit message could probably be worded better, particularly since it's
forged in your name.
help.c| 4 ++--
t/t9902-completion.sh | 2 +-
2 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/help.c b/help.c
index 520c9080e8..026f881715 100644
--- a/help.c
+++ b/help.c
@@ -393,8 +393,8 @@ void list_cmds_by_config(struct string_list *list)
const char *p = strchrnul(cmd_list, ' ');
strbuf_add(&sb, cmd_list, p - cmd_list);
- if (*cmd_list == '-')
- string_list_remove(list, cmd_list + 1, 0);
+ if (sb.buf[0] == '-')
+ string_list_remove(list, sb.buf + 1, 0);
else
string_list_insert(list, sb.buf);
strbuf_release(&sb);
diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
index dd11bb660d..d7daa1ca92 100755
--- a/t/t9902-completion.sh
+++ b/t/t9902-completion.sh
@@ -1483,7 +1483,7 @@ test_expect_success 'git --help completion' '
test_completion "git --help core" "core-tutorial "
'
-test_expect_failure 'completion.commands removes multiple commands' '
+test_expect_success 'completion.commands removes multiple commands' '
echo cherry-pick >expected &&
test_config_global completion.commands "-cherry -mergetool" &&
git --list-cmds=list-mainporcelain,list-complete,config |
Re: [BUG] completion.commands does not remove multiple commands
Jeff King wrote: > I can reproduce your problem, though the test you included passes for me > even with the current tip of master. Oh, hrm. I think the issue is that completion.commands needs to be set in the global (or system-wide) config, via test_config_global rather than the local repo config which test_config sets. In hindsight, that seems obvious. But it's probably worth noting that where completion.commands is documented, for anyone who might spend a few cycles trying to configure it on a per-repo basis before realizing it doesn't work. > I suspect this is the problem: > > diff --git a/help.c b/help.c > index 520c9080e8..026f881715 100644 > --- a/help.c > +++ b/help.c > @@ -393,8 +393,8 @@ void list_cmds_by_config(struct string_list *list) > const char *p = strchrnul(cmd_list, ' '); > > strbuf_add(&sb, cmd_list, p - cmd_list); > - if (*cmd_list == '-') > - string_list_remove(list, cmd_list + 1, 0); > + if (sb.buf[0] == '-') > + string_list_remove(list, sb.buf + 1, 0); > else > string_list_insert(list, sb.buf); > strbuf_release(&sb); > > though I can't help but wonder if the whole thing would be simpler using > string_list_split(). That does indeed fix things (as does SZEDER's similar version, which arrived only a few seconds later). Thanks to both of you for the very quick replies! I'll leave it to those who know better to follow up with a change to use string_list_split(). Here's a first stab at improving the docs and fixing the bug. Jeff King (1): completion: fix multiple command removals Todd Zullinger (2): doc: note config file restrictions for completion.commands t9902: test multiple removals via completion.commands Documentation/config/completion.txt | 3 ++- Documentation/git.txt | 3 ++- help.c | 4 ++-- t/t9902-completion.sh | 8 4 files changed, 14 insertions(+), 4 deletions(-) Published-as: https://github.com/tmzullinger/git/releases/tag/completion.commands-v1 -- Todd
[PATCH 2/3] t9902: test multiple removals via completion.commands
6532f3740b ("completion: allow to customize the completable command
list", 2018-05-20) added the completion.commands config variable.
Multiple commands may be added or removed, separated by a space.
Demonstrate the failure of multiple removals.
Signed-off-by: Todd Zullinger
---
t/t9902-completion.sh | 8
1 file changed, 8 insertions(+)
diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
index 3a2c6326d8..dd11bb660d 100755
--- a/t/t9902-completion.sh
+++ b/t/t9902-completion.sh
@@ -1483,6 +1483,14 @@ test_expect_success 'git --help completion' '
test_completion "git --help core" "core-tutorial "
'
+test_expect_failure 'completion.commands removes multiple commands' '
+ echo cherry-pick >expected &&
+ test_config_global completion.commands "-cherry -mergetool" &&
+ git --list-cmds=list-mainporcelain,list-complete,config |
+ grep ^cherry >actual &&
+ test_cmp expected actual
+'
+
test_expect_success 'setup for integration tests' '
echo content >file1 &&
echo more >file2 &&
[PATCH 1/3] doc: note config file restrictions for completion.commands
The completion.commands config var must be set in either the system-wide or global config file. The completion script does not read a local repository config. Signed-off-by: Todd Zullinger --- Documentation/config/completion.txt | 3 ++- Documentation/git.txt | 3 ++- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/Documentation/config/completion.txt b/Documentation/config/completion.txt index 4d99bf33c9..4859d18e86 100644 --- a/Documentation/config/completion.txt +++ b/Documentation/config/completion.txt @@ -4,4 +4,5 @@ completion.commands:: porcelain commands and a few select others are completed. You can add more commands, separated by space, in this variable. Prefixing the command with '-' will remove it from - the existing list. + the existing list. The variable must be set in either the + system-wide or global config. diff --git a/Documentation/git.txt b/Documentation/git.txt index 00156d64aa..638f4d6cc9 100644 --- a/Documentation/git.txt +++ b/Documentation/git.txt @@ -172,7 +172,8 @@ foo.bar= ...`) sets `foo.bar` to the empty string which `git config others (all other commands in `$PATH` that have git- prefix), list- (see categories in command-list.txt), nohelpers (exclude helper commands), alias and config - (retrieve command list from config variable completion.commands) + (retrieve command list from config variable completion.commands, + set in the global or system-wide config file) GIT COMMANDS
[BUG] completion.commands does not remove multiple commands
Hi,
I was playing with the completion.commands config added in
6532f3740b ("completion: allow to customize the completable
command list", 2018-05-20) and noticed an issue removing
multiple commands.
I wanted to remove completion for cherry and mergetool, so I
added them both to the config:
git config completion.commands "-cherry -mergetool"
But git still completes cherry in this case, only removing
mergetool. Swapping the items in the config yields the
opposite result (cherry is removed and mergetool is not).
With luck this will be a clear and easily resolved issue in
list_cmds_by_config() (in help.c).
Here's an example in test form:
-- 8< --
diff --git c/t/t9902-completion.sh w/t/t9902-completion.sh
index 3a2c6326d8..06cee36ae5 100755
--- c/t/t9902-completion.sh
+++ w/t/t9902-completion.sh
@@ -1483,6 +1483,14 @@ test_expect_success 'git --help completion' '
test_completion "git --help core" "core-tutorial "
'
+test_expect_failure 'completion.commands removes multiple commands' '
+ test_config completion.commands "-cherry -mergetool" &&
+ git --list-cmds=list-mainporcelain,list-complete,config |
+ grep ^cherry >actual &&
+ echo cherry-pick >expected &&
+ test_cmp expected actual
+'
+
test_expect_success 'setup for integration tests' '
echo content >file1 &&
echo more >file2 &&
-- 8< --
That's not quite normal form for t9902, but I was undable to
create a working test using the test_completion functions.
The tests set GIT_TESTING_PORCELAIN_COMMAND_LIST and
GIT_TESTING_ALL_COMMAND_LIST which override the settings in
the completion script. I played a bit with adjusting those
to add cherry{,-pick} to the command lists, unsetting those
vars for the test, and such, to no avail.
In the end, I'm not really sure that calling --list-cmds
directly is such a bad way to go for testing this issue.
--
Todd
Re: on fedora, "man gitweb" exists but actual gitweb command is missing
Hi, Robert P. J. Day wrote: > > not so much a git issue as what looks like a fedora packaging issue. Yeah, it's just a minor packaging issue. The gitweb manpages are included in the main git package rather than in the gitweb package with the rest of the gitweb files. I'll fix that for future releases and when f29 is updated to 2.21 it will pick that up¹. Since gitweb requires git, you'd be sure to have the documentation after installing gitweb. If we made it possible to install gitweb without getting the documentation, that would be more annoying. :) ¹ https://src.fedoraproject.org/fork/tmz/rpms/git/c/0d9ad786 > it took only a few seconds to determine that fedora bundles that > functionality in two separate packages which are not dependencies of > "git": "gitweb" and "git-instaweb" (output abbreviated): > > $ sudo dnf install git-instaweb > ... > Installing: >git-instaweb > Installing dependencies: >gitweb >perl-CGI > > and now "git-instaweb" works fine. > > the question is, is it not inconsistent for fedora's basic "git" > package to include the man page for gitweb, without including the > corresponding functionality? is this something i should submit a > fedora bugzilla report for? or am i misunderstanding something? If I hadn't seen this thread, then a report in the fedora bugzilla would be the way to go to ensure one of the fedora git maintainers noticed it. Thanks, -- Todd
Re: [PATCH 2/2] commit-graph tests: fix cryptic unportable "dd" invocation
Junio C Hamano wrote:
> Ævar Arnfjörð Bjarmason writes:
>
>> It was my reading of the seek=* section ("the implementation shall seek
>> to the specified offset"). I didn't spot that bit covered in of=*. Yeah,
>> I see that's defined & safe after reading that.
>
> OK, so...
>
> -- >8 --
> From: Ævar Arnfjörð Bjarmason
> Date: Thu, 21 Feb 2019 20:28:49 +0100
> Subject: [PATCH] commit-graph tests: fix cryptic unportable "dd" invocation
>
> Change an unportable invocation of "dd" with count=0, that wanted to
> truncate the commit-graph file. In POSIX it is unspecified what
> happens when count=0 is provided[1]. The NetBSD "dd" behavior
> differs from GNU (and seemingly other BSDs), which as left this test
s/ as/ has/ ?
> broken since d2b86fbaa1 ("commit-graph: fix buffer read-overflow",
> 2019-01-15).
>
> Copying from /dev/null would seek/truncate to seek=$zero_pos and
> stop immediately after that (without being able to copy anything),
> which is the right way to truncate the file.
>
> 1. http://pubs.opengroup.org/onlinepubs/9699919799/utilities/dd.html
>
> Signed-off-by: Ævar Arnfjörð Bjarmason
> Helped-by: SZEDER Gábor
> Signed-off-by: Junio C Hamano
> ---
> t/t5318-commit-graph.sh | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh
> index d4bd1522fe..561796f280 100755
> --- a/t/t5318-commit-graph.sh
> +++ b/t/t5318-commit-graph.sh
> @@ -382,7 +382,7 @@ corrupt_graph_and_verify() {
> test_when_finished mv commit-graph-backup $objdir/info/commit-graph &&
> cp $objdir/info/commit-graph commit-graph-backup &&
> printf "$data" | dd of="$objdir/info/commit-graph" bs=1 seek="$pos"
> conv=notrunc &&
> - dd of="$objdir/info/commit-graph" bs=1 seek="$zero_pos" count=0 &&
> + dd of="$objdir/info/commit-graph" bs=1 seek="$zero_pos" if=/dev/null &&
> generate_zero_bytes $(($orig_size - $zero_pos))
> >>"$objdir/info/commit-graph" &&
> test_must_fail git commit-graph verify 2>test_err &&
> grep -v "^+" test_err >err &&
--
Todd
Re: [Suggestion] Add Skip for t9020
Hi, Randall S. Becker wrote: > On February 21, 2019 15:00, I wrote: >> t9020 subtests 1,2,5,6 failed - Not new. unsurprising as there is no SVN or >> perl with SVN module on platform. It might be useful to have a detection to >> skip of Perl SVN is not present. > > While this is a bit of a hack, it might be useful for skipping t9020 in > environments where the svn.remote package is not installed. I can make this > into a patch if this style is reasonable - guessing probably not and that > the REMOTE_SVN test should go elsewhere if it is called that. Jeff King sent an RFC patch which would remove this test and the rest of the vcs-svn experiment in August[1]. Jonathan Nieder replied as one user who would rather see it moved to contrib/, so it was held off. Whether that has any impact on adding a way to skip all the tests here, I don't know. Maybe it's a gentle nudge in favor of moving them to contrib? [1] https://public-inbox.org/git/20180817190310.ga5...@sigill.intra.peff.net/ -- Todd
Re: [PATCH] t/lib-httpd: pass GIT_TEST_SIDEBAND_ALL through Apache
Jonathan Tan wrote:
>> 07c3c2aa16 ("tests: define GIT_TEST_SIDEBAND_ALL", 2019-01-16) added
>> GIT_TEST_SIDEBAND_ALL to the apache.conf PassEnv list. Avoid warnings
>> from Apache when the variable is unset, as we do for GIT_VALGRIND* and
>> GIT_TRACE, from f628825481 ("t/lib-httpd: handle running under
>> --valgrind", 2012-07-24) and 89c57ab3f0 ("t: pass GIT_TRACE through
>> Apache", 2015-03-13), respectively.
>>
>> Signed-off-by: Todd Zullinger
>> ---
>> I missed this with rc0, but poking through build logs I noticed a number
>> of 'AH01506: PassEnv variable GIT_TEST_SIDEBAND_ALL was undefined'
>> warnings.
>>
>> I think exporting this in lib-httpd.sh like we do for GIT_VALGRIND* and
>> GIT_TRACE is the way to go, as opposed to in test-lib.sh, as we do for
>> things like GNUPGHOME. But I could easily be wrong about that.
>
> Thanks for looking into this. I think this is the right way to do it
> too.
>
> Previous discussion here [1] but I don't think any patches came out of
> that.
>
> [1] https://public-inbox.org/git/20190129232732.gb218...@google.com/
Hah. Somehow I missed that thread and Jeff's reply barely
24 hours before I sent this. Hopefully this saves Jonathan
Nieder a few minutes of patch prep & testing.
Thanks,
--
Todd
Re: [PATCH] t/lib-httpd: pass GIT_TEST_SIDEBAND_ALL through Apache
Jeff King wrote:
> On Thu, Feb 14, 2019 at 01:35:13AM -0500, Todd Zullinger wrote:
>
>> 07c3c2aa16 ("tests: define GIT_TEST_SIDEBAND_ALL", 2019-01-16) added
>> GIT_TEST_SIDEBAND_ALL to the apache.conf PassEnv list. Avoid warnings
>> from Apache when the variable is unset, as we do for GIT_VALGRIND* and
>> GIT_TRACE, from f628825481 ("t/lib-httpd: handle running under
>> --valgrind", 2012-07-24) and 89c57ab3f0 ("t: pass GIT_TRACE through
>> Apache", 2015-03-13), respectively.
>>
>> Signed-off-by: Todd Zullinger
>> ---
>> I missed this with rc0, but poking through build logs I noticed a number
>> of 'AH01506: PassEnv variable GIT_TEST_SIDEBAND_ALL was undefined'
>> warnings.
>>
>> I think exporting this in lib-httpd.sh like we do for GIT_VALGRIND* and
>> GIT_TRACE is the way to go, as opposed to in test-lib.sh, as we do for
>> things like GNUPGHOME. But I could easily be wrong about that.
>
> Yeah, I think this is the right place to put it (and this approach is
> the right thing to do).
Excellent, thanks.
>> diff --git a/t/lib-httpd.sh b/t/lib-httpd.sh
>> index 216281eabc..0dfb48c2f6 100644
>> --- a/t/lib-httpd.sh
>> +++ b/t/lib-httpd.sh
>> @@ -91,6 +91,7 @@ HTTPD_DOCUMENT_ROOT_PATH=$HTTPD_ROOT_PATH/www
>> # hack to suppress apache PassEnv warnings
>> GIT_VALGRIND=$GIT_VALGRIND; export GIT_VALGRIND
>> GIT_VALGRIND_OPTIONS=$GIT_VALGRIND_OPTIONS; export GIT_VALGRIND_OPTIONS
>> +GIT_TEST_SIDEBAND_ALL=$GIT_TEST_SIDEBAND_ALL; export GIT_TEST_SIDEBAND_ALL
>> GIT_TRACE=$GIT_TRACE; export GIT_TRACE
>
> I applaud your attempt to alphabetize, but the existing list is already
> out of order. ;) I don't think it really matters much either way,
> though.
It's like a tar pit for catching people with a little OCD.
I debated whether to add it at the end, sort them all in a
prep patch, or just add it after GIT_TRACE. I'm not sure if
I should even admit to spending as much time debating it
with myself as I did. ;)
--
Todd
Re: What's cooking in git.git (Feb 2019, #03; Wed, 13)
Jeff King wrote: > Yeah, I do have the feeling that not many people really exercise our -rc > candidates. I'm not sure how to improve that. We could try to push > packagers to make them available (and I think Jonathan already does for > Debian's "experimental" track). Similarly, I try to make them available in Fedora's rawhide channel (as well as less officially as builds for current stable Fedora and RHEL/CentOS releases¹). I don't have a good sense of how many git users that reaches, but I haven't had many (or perhaps any?) bug reports from the -rc packages. I typically wait for -rc1 to push to rawhide, just to give a little time to weed out the early issues. I'd make the Fedora QA folks mad if I pushed a git update that caused them too much grief. Thankfully, I can't recall ever having such a problem (certainly nothing that most users of git would run into). > But ultimately I think it is not a packaging/availability question, but > just that most people do not bother until there is a real release. Indeed. ¹ Those "less official" packages are: https://copr.fedorainfracloud.org/coprs/g/git-maint/git/ & https://copr.fedorainfracloud.org/coprs/tmz/git/ -- Todd
[PATCH] t/lib-httpd: pass GIT_TEST_SIDEBAND_ALL through Apache
07c3c2aa16 ("tests: define GIT_TEST_SIDEBAND_ALL", 2019-01-16) added
GIT_TEST_SIDEBAND_ALL to the apache.conf PassEnv list. Avoid warnings
from Apache when the variable is unset, as we do for GIT_VALGRIND* and
GIT_TRACE, from f628825481 ("t/lib-httpd: handle running under
--valgrind", 2012-07-24) and 89c57ab3f0 ("t: pass GIT_TRACE through
Apache", 2015-03-13), respectively.
Signed-off-by: Todd Zullinger
---
I missed this with rc0, but poking through build logs I noticed a number
of 'AH01506: PassEnv variable GIT_TEST_SIDEBAND_ALL was undefined'
warnings.
I think exporting this in lib-httpd.sh like we do for GIT_VALGRIND* and
GIT_TRACE is the way to go, as opposed to in test-lib.sh, as we do for
things like GNUPGHOME. But I could easily be wrong about that.
t/lib-httpd.sh | 1 +
1 file changed, 1 insertion(+)
diff --git a/t/lib-httpd.sh b/t/lib-httpd.sh
index 216281eabc..0dfb48c2f6 100644
--- a/t/lib-httpd.sh
+++ b/t/lib-httpd.sh
@@ -91,6 +91,7 @@ HTTPD_DOCUMENT_ROOT_PATH=$HTTPD_ROOT_PATH/www
# hack to suppress apache PassEnv warnings
GIT_VALGRIND=$GIT_VALGRIND; export GIT_VALGRIND
GIT_VALGRIND_OPTIONS=$GIT_VALGRIND_OPTIONS; export GIT_VALGRIND_OPTIONS
+GIT_TEST_SIDEBAND_ALL=$GIT_TEST_SIDEBAND_ALL; export GIT_TEST_SIDEBAND_ALL
GIT_TRACE=$GIT_TRACE; export GIT_TRACE
if ! test -x "$LIB_HTTPD_PATH"
--
2.21.0.rc1
Re: [PATCH 0/2] t/lib-gpg: a gpgsm fix, a minor improvement, and a question
SZEDER Gábor wrote: > On Sat, Feb 09, 2019 at 06:05:53PM -0500, Todd Zullinger wrote: >> SZEDER Gábor wrote: >>> Just curious: how did you noticed the missing GPGSM prereq? >> >> I just grep the build output for '# SKIP|skipped:' and then >> filter out those which I expect (thing like MINGW and >> NATIVE_CRLF that aren't likely to be in a Fedora build). >> >> Far more manual than the slick method you have below. :) > > Yeah, but yours show the SKIP cases, too, i.e. when the whole test is > skipped by: > > if check-something > then > skip_all="no can do" > test_done > fi > > I didn't bother with that because in those cases the prereq is not > denoted by a single word, but rather by a human-readable phrase, and > becase 'prove' runs those skipped test scripts at last when running > slowest first, so I could already see them anyway. Ahh, good points. >>> I'm asking because I use a patch for a good couple of months now that >>> collects the prereqs missed by test cases and prints them at the end >>> of 'make test'. Its output looks like this: >>> >>> https://travis-ci.org/szeder/git/jobs/490944032#L2358 >>> >>> Since you seem to be interested in that sort of thing as well, perhaps >>> it would be worth to have something like this in git.git? It's just >>> that I have been too wary of potentially annoying other contributors >>> by adding (what might be perceived as) clutter to their 'make test' >>> output :) >> >> Indeed, I think that would be useful. At the very least, >> the .missing_prereqs files look quite handy. I wouldn't >> mind the output from 'make test' either, but building >> packages surely shifts my perspective toward more verbose >> build logs than someone hacking on git regularly and reading >> the 'make test' output. > > The problem with those files is that a successful 'make test' > automatically and unconditionally removes the whole 'test-results' > directory at the end. So a separate and optional 'make test ; make > show-missed-prereqs' wouldn't have worked, that's why I did it this > way. > > I think it would be better if we kept the 'test-results' directory > even after a successful 'make test', there are some interesting things > to be found there: > > > https://public-inbox.org/git/CAM0VKjkVreBKQsvMZ=pee0nn5gg0mm+xj0mzcbw1rxi_pr+...@mail.gmail.com/ Maybe that's something which could be controlled with a make var, to allow folks like us to keep the tests but clean them up by default for everyone else? Though even in the fedora package builds, I don't have access to poke around in test-results and likely wouldn't want to make the effort to extract the results and dump them into the build logs (like ci/util/extract-trash-dirs.sh does for the trash dirs). >> I can certainly live with setting '--root' to a shorter path >> and waiting to see if GnuPG upstream will come up with >> something a little more friendly to users like us - running >> gpg in a test suite. > > Are they aware of the issue? Yeah, it was filed as https://dev.gnupg.org/T2964, as a result of the gnupg-users thread you mention below. There hasn't been any progress on it since 2017 though, so it's doubtful that upstream will fix it anytime soon. If (or when) it's resolved, I wouldn't be surprised if only gnupg >= 2.3 included the fix. So we'll probably have numerous systems with this issue for quite some time. > https://lists.gnupg.org/pipermail/gnupg-users/2017-January/057451.html > > suggests to put the socket in '/var/run/user/$(id -u)', but that's for > the "regular" use case, and we should take extra steps to prevent the > tests' gpg from interfering with the gpg of the user running the > tests. Not sure it would work on macOS. And ultimately it's not much > different from your GIT_TEST_GNUPGHOME_ROOT suggestion. > > Then I stumbled on these patches patches: > > https://lists.zx2c4.com/pipermail/password-store/2017-May/002932.html > > suggesting that at least one other project is working around this > issue instead of waiting for upstream to come up with something. Heh, the gnupg ticket mentions that the notmuch project similarly had to work around gpg2's socket handling for their tests: https://notmuchmail.org/pipermail/notmuch/2017/024148.html >> Though if we do just wait it out, >> maybe we could/should add a note in t/README or t/lib-gpg.sh >> a
Re: [PATCH] Add support for 'git remote rm' in Bash completion script
Hi,
Keith Smiley wrote:
>> On Feb 8, 2019, at 22:27, Todd Zullinger wrote:
>>
>> Hi Sergey,
>>
>> There was a previous discussion of this in December 2017,
>> which might be useful:
>>
>> https://public-inbox.org/git/01020160a0004473-277c3d7c-4e3b-4c50-9d44-4a106f37f1d9-000...@eu-west-1.amazonses.com/
>>
>> It didn't end with a patch applied, but it's likely worth
>> reading to help you make a case for a similar patch.
>>
>> One of the points in that thread is that the rm subcommand
>> is not shown in completion intentionally, as the preferred
>> subcommand is remove. But it should be possible to offer
>> completion of the remotes after a user types rm, which I
>> imagine is the more helpful part of the completion.
>>
>> Also, you'll want to add a signoff to the patch if/when you
>> resend it (refer to Documentation/SubmittingPatches, if you
>> haven't already).
>>
>> Sergey Zolotarev wrote:
>>> ---
>>> contrib/completion/git-completion.bash | 5 -
>>> 1 file changed, 4 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/contrib/completion/git-completion.bash
>>> b/contrib/completion/git-completion.bash
>>> index 499e56f83d..fa25d689e2 100644
>>> --- a/contrib/completion/git-completion.bash
>>> +++ b/contrib/completion/git-completion.bash
>>> @@ -2334,7 +2334,7 @@ _git_remote ()
>>> {
>>>local subcommands="
>>>add rename remove set-head set-branches
>>> -get-url set-url show prune update
>>> +get-url set-url show prune rm update
>>>"
>>>local subcommand="$(__git_find_on_cmdline "$subcommands")"
>>
>> So instead of this change you could adjust the subcommand
>> line, something like:
>>
>> -local subcommand="$(__git_find_on_cmdline "$subcommands")"
>> +# Don't advertise rm by including it in subcommands, but complete
>> +# remotes if it is used.
>> +local subcommand="$(__git_find_on_cmdline "$subcommands rm")"
>>
>> I haven't test that, but the code looks like it hasn't
>> changed since the last time we talked about this -- when I
>> did test the suggestion :).
>>
>>>if [ -z "$subcommand" ]; then
>>> @@ -2379,6 +2379,9 @@ _git_remote ()
>>>prune,--*)
>>>__gitcomp_builtin remote_prune
>>>;;
>>> +rm,--*)
>>> +__gitcomp_builtin remote_rm
>>> +;;
>>>*)
>>>__gitcomp_nl "$(__git_remotes)"
>>>;;
>>
>> I don't think you need this chunk, do you? I think that's
>> only useful for completing options to the subcommand, which
>> 'git remote rm' lacks.
>>
>> I believe you can simply skip it and let the case fall
>> through to the last item which simply completes the
>> available remotes, just as 'git remote remove' does.
>>
>> Hope that helps,
>>
> It would be great if we could land this. Non of the other
> solutions since I proposed my patch have happened, so in
> the meantime this would be nice to have.
Yeah, it seems like we should either complete the remotes
for 'git remote rm ' or follow up with the removal of
the rm subcommand as Duy and Junio fleshed out in the thread
from July. If the command were to be removed, doing it
right after the 2.22 cycle begins would be as good a time as
any.
If the command remains, here's a suggestion for how it might
be submitted (perhaps with more details in the commit to
justify why we're completing arguments to a deprecated
subcommand?).
It's really not an itch of mine, so I'm just tossing this
out in case someone that wants it more will push it forward.
>8
Subject: [PATCH] completion: complete remotes for 'remote rm'
The remote 'rm' subcommand was removed from the completed subcommands
list in e17dba8fe1 ("remote: prefer subcommand name 'remove' to 'rm'",
2012-09-06). While we don't advertise it, we can still complete the
list of remotes for users who type 'git remote rm ' as a courtesy.
Reported-by: Keith Smiley
Reported-by: Sergey Zolotarev
Helped-by: Todd Zullinger
Signed-off-by:
---
contrib/completion/git-completion.bash | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/contrib/completion/git-completion.bash
b/contrib/completion/git-completion.bash
index 499e56f83d..5d0f8a2077 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -2336,7 +2336,9 @@ _git_remote ()
add rename remove set-head set-branches
get-url set-url show prune update
"
- local subcommand="$(__git_find_on_cmdline "$subcommands")"
+ # We don't advertise rm by including it in subcommands, but if it is
+ # used we want to complete remotes.
+ local subcommand="$(__git_find_on_cmdline "$subcommands rm")"
if [ -z "$subcommand" ]; then
case "$cur" in
--*)
--
Todd
Re: [PATCH 0/2] t/lib-gpg: a gpgsm fix, a minor improvement, and a question
SZEDER Gábor wrote: > On Thu, Feb 07, 2019 at 10:17:44PM -0500, Todd Zullinger wrote: >> Looking through the build logs for the fedora git packages, I noticed it >> was missing the GPGSM prereq. > > Just curious: how did you noticed the missing GPGSM prereq? I just grep the build output for '# SKIP|skipped:' and then filter out those which I expect (thing like MINGW and NATIVE_CRLF that aren't likely to be in a Fedora build). Far more manual than the slick method you have below. :) > I'm asking because I use a patch for a good couple of months now that > collects the prereqs missed by test cases and prints them at the end > of 'make test'. Its output looks like this: > > https://travis-ci.org/szeder/git/jobs/490944032#L2358 > > Since you seem to be interested in that sort of thing as well, perhaps > it would be worth to have something like this in git.git? It's just > that I have been too wary of potentially annoying other contributors > by adding (what might be perceived as) clutter to their 'make test' > output :) Indeed, I think that would be useful. At the very least, the .missing_prereqs files look quite handy. I wouldn't mind the output from 'make test' either, but building packages surely shifts my perspective toward more verbose build logs than someone hacking on git regularly and reading the 'make test' output. >> I don't know if there are other packagers or builders who run into this, >> so maybe it's not worth much effort to try and have the test suite cope >> better. It took me longer than I would have liked to track it down, so >> I thought I'd mention it in case anyone else has run into this or has >> thoughts on how to improve lib-gpg.sh while waiting for GnuPG to improve >> this area. > > I stumbled upon this when Dscho inadvertently broke a test script on > setups without gpg last year; sorry for not speaking about it. I > noticed it in our Travis CI builds on macOS, because it (macOS itself > or Homebrew? I don't know) defaulted to gpg2 already back then, and to > make matters worse its sun_path is on the shorter end, and the path > to the build dir on Travis CI includes the GitHub user/repo as well. Heh, I figured it was a rather specific group of folks that might run into this. >> A GIT_TEST_GNUPGHOME_ROOT var to set the root path for the GNUPGHOME >> dirs in the tests is one thought I had, but didn't try to put it into >> patch form. Setting the --root test option is probably enough control >> for most cases. > > A potential issue I see with GIT_TEST_GNUPGHOME_ROOT is that there are > several test scripts involving gpg, and if GIT_TEST_GNUPGHOME_ROOT is > set for the whole 'make test', then they might interfere with each > other when they happen to be run at the same time. Yeah, I was envisioning that var as something which set the base dir, under which the normal test directories would live. Basically, like setting --root, but only for the GnuPG bits. I'm not impressed by that idea (and I'm even less so after realizing how it would most likely make it harder to gather up the results in the CI scripts). I mainly tossed it out in the hope someone would reply with a better method. ;) > In the meantime I came up with a '--short-trash-dir' option to > test-lib, which turns 'trash directory.t7612-merge-verify-signatures' > into 'trash dir.t7612'. It works, but I don't really like it, and it > required various adjustments to the CI build scripts, notably to the > part in 'ci/print-test-failures.sh' that includes the trash dir of > failed test scripts in the build log. I can certainly live with setting '--root' to a shorter path and waiting to see if GnuPG upstream will come up with something a little more friendly to users like us - running gpg in a test suite. Though if we do just wait it out, maybe we could/should add a note in t/README or t/lib-gpg.sh about this to warn others? -- Todd
Re: [Breakage] Git v2.21.0-rc0 - t5403 (NonStop)
[-cc: linux-kernel & git-packagers] SZEDER Gábor wrote: > On Fri, Feb 08, 2019 at 03:11:29PM -0500, Todd Zullinger wrote: >> It made me wonder how I had missed it in my own testing. >> This one requires SHELL_PATH to be bash, while I only set >> TEST_SHELL_PATH to bash for the improved -x tracing in the >> fedora builds. > > Note that you don't need Bash to use '-x' tracing since a5bf824f3b (t: > prevent '-x' tracing from interfering with test helpers' stderr, > 2018-02-25) and the followup commits, except for 't1510-repo-setup.sh' > (and even t1510 just falls back to ignore '-x' instead of causing > failures). Huh, cool. I somehow missed that nice improvement. Thanks for all your fine work on the test suite! It makes my life as a packager much better having a robust test suite running with each build. -- Todd
Re: [PATCH] Add support for 'git remote rm' in Bash completion script
Hi Sergey,
There was a previous discussion of this in December 2017,
which might be useful:
https://public-inbox.org/git/01020160a0004473-277c3d7c-4e3b-4c50-9d44-4a106f37f1d9-000...@eu-west-1.amazonses.com/
It didn't end with a patch applied, but it's likely worth
reading to help you make a case for a similar patch.
One of the points in that thread is that the rm subcommand
is not shown in completion intentionally, as the preferred
subcommand is remove. But it should be possible to offer
completion of the remotes after a user types rm, which I
imagine is the more helpful part of the completion.
Also, you'll want to add a signoff to the patch if/when you
resend it (refer to Documentation/SubmittingPatches, if you
haven't already).
Sergey Zolotarev wrote:
> ---
> contrib/completion/git-completion.bash | 5 -
> 1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/contrib/completion/git-completion.bash
> b/contrib/completion/git-completion.bash
> index 499e56f83d..fa25d689e2 100644
> --- a/contrib/completion/git-completion.bash
> +++ b/contrib/completion/git-completion.bash
> @@ -2334,7 +2334,7 @@ _git_remote ()
> {
> local subcommands="
> add rename remove set-head set-branches
> - get-url set-url show prune update
> + get-url set-url show prune rm update
> "
> local subcommand="$(__git_find_on_cmdline "$subcommands")"
So instead of this change you could adjust the subcommand
line, something like:
- local subcommand="$(__git_find_on_cmdline "$subcommands")"
+ # Don't advertise rm by including it in subcommands, but complete
+ # remotes if it is used.
+ local subcommand="$(__git_find_on_cmdline "$subcommands rm")"
I haven't test that, but the code looks like it hasn't
changed since the last time we talked about this -- when I
did test the suggestion :).
> if [ -z "$subcommand" ]; then
> @@ -2379,6 +2379,9 @@ _git_remote ()
> prune,--*)
> __gitcomp_builtin remote_prune
> ;;
> + rm,--*)
> + __gitcomp_builtin remote_rm
> + ;;
> *)
> __gitcomp_nl "$(__git_remotes)"
> ;;
I don't think you need this chunk, do you? I think that's
only useful for completing options to the subcommand, which
'git remote rm' lacks.
I believe you can simply skip it and let the case fall
through to the last item which simply completes the
available remotes, just as 'git remote remove' does.
Hope that helps,
--
Todd
Re: [PATCH 1/2] t/lib-gpg: quote path to ${GNUPGHOME}/trustlist.txt
SZEDER Gábor wrote:
> On Thu, Feb 07, 2019 at 10:17:45PM -0500, Todd Zullinger wrote:
>> When gpgsm is installed, lib-gpg.sh attempts to update trustlist.txt to
>> relax the checking of some root certificate requirements. The path to
>> "${GNUPGHOME}" contains spaces which cause an "ambiguous redirect"
>> warning when bash is used to run the tests:
>
> s/error/warning/
Did you mean s/warning/error/ so the sentence reads:
The path to "${GNUPGHOME}" contains spaces which cause
an "ambiguous redirect" error when bash is used to run
the tests
?
Is it worth a resend before Junio queues it?
>> $ bash t7030-verify-tag.sh
>> /git/t/lib-gpg.sh: line 66: ${GNUPGHOME}/trustlist.txt: ambiguous redirect
>> ok 1 - create signed tags
>> ok 2 # skip create signed tags x509 (missing GPGSM)
>> ...
>>
>> No warning is issued when using bash called as /bin/sh, dash, or mksh.
>
> Likewise.
>
> POSIX says that no field splitting should be performed on the result
> of a parameter expansion that is used as the target of a redirection,
> but Bash doesn't conform in this respect (unless in POSIX mode).
I wish I'd remembered reading your detailed explanation of
this¹ when I was testing and writing the commit message. :)
¹ https://public-inbox.org/git/20180926121107.GH27036@localhost/
--
Todd
Re: [Breakage] Git v2.21.0-rc0 - t5403 (NonStop)
SZEDER Gábor wrote: > On Fri, Feb 08, 2019 at 05:48:27AM -0500, Randall S. Becker wrote: >> We have a few new breakages on the NonStop port in 2.21.0-rc0. The first is >> in t5403, as below: [...] >> The post-checkout hook is: >> #!/usr/local/bin/bash >> echo "$@" >$GIT_DIR/post-checkout.args >> >> This looks like it is a "bash thing" and $GIT_DIR might have to be in >> quotes, and is not be specific to the platform. If I replace >> >> echo "$@" >$GIT_DIR/post-checkout.args >> >> with >> >> echo "$@" >"$GIT_DIR/post-checkout.args" >> >> The test passes. > > Wow, this is the second time this "redirection to a filename with > spaces under Bash" issue pops up today, see the other one here: > > https://public-inbox.org/git/20190208031746.22683-2-...@pobox.com/T/#u Indeed, I was surprised to see another one today. It made me wonder how I had missed it in my own testing. This one requires SHELL_PATH to be bash, while I only set TEST_SHELL_PATH to bash for the improved -x tracing in the fedora builds. I ran the tests again with SHELL_PATH as bash on fedora and this was the only failure I saw (other than the one from my earlier message, that is). -- Todd
[PATCH 1/2] t/lib-gpg: quote path to ${GNUPGHOME}/trustlist.txt
When gpgsm is installed, lib-gpg.sh attempts to update trustlist.txt to
relax the checking of some root certificate requirements. The path to
"${GNUPGHOME}" contains spaces which cause an "ambiguous redirect"
warning when bash is used to run the tests:
$ bash t7030-verify-tag.sh
/git/t/lib-gpg.sh: line 66: ${GNUPGHOME}/trustlist.txt: ambiguous redirect
ok 1 - create signed tags
ok 2 # skip create signed tags x509 (missing GPGSM)
...
No warning is issued when using bash called as /bin/sh, dash, or mksh.
Quote the path to ensure the redirect works as intended and sets the
GPGSM prereq. While we're here, drop the space after ">>".
Signed-off-by: Todd Zullinger
---
t/lib-gpg.sh | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/t/lib-gpg.sh b/t/lib-gpg.sh
index f1277bef4f..207009793b 100755
--- a/t/lib-gpg.sh
+++ b/t/lib-gpg.sh
@@ -63,7 +63,7 @@ then
cut -d" " -f4 |
tr -d '\n' >"${GNUPGHOME}/trustlist.txt" &&
- echo " S relax" >> ${GNUPGHOME}/trustlist.txt &&
+ echo " S relax" >>"${GNUPGHOME}/trustlist.txt" &&
(gpgconf --kill gpg-agent >/dev/null 2>&1 || : ) &&
echo hello | gpgsm --homedir "${GNUPGHOME}" >/dev/null \
-u commit...@example.com -o /dev/null --sign - 2>&1 &&
--
Todd
[PATCH 0/2] t/lib-gpg: a gpgsm fix, a minor improvement, and a question
Hi,
Looking through the build logs for the fedora git packages, I noticed it
was missing the GPGSM prereq. I added the necessary package to the
build requirements but GPGSM was still failing to be set. This turned
out to be due to a use of ${GNUPGHOME} without quoting, which leads to a
non-zero exit from echo and the end of the happy && chain when using
bash as the test shell. Fixing this allows the GPGSM test prereq to be
set.
While I was poking around I also saw an extra gpgconf call to kill
gpg-agent. This was copied from the GPG block earlier in lib-gpg.sh,
but should not be needed (as far as I can tell). I don't think it can
cause any real harm apart from causing gpg and gpgsm to start the agent
more often than necessary. But I didn't run the tests with the --stress
option to look for potential issues that could be more serious.
Lastly, the GPG test prereq was failing in two of the tests where it was
used, t5573-pull-verify-signatures and t7612-merge-verify-signatures. I
tracked this down to an annoying issue with gnugp-2¹, which recently
became the default /bin/gpg in fedora².
Using gnupg2 as /bin/gpg means using gpg-agent by default. When using a
non-standard GNUPGHOME, gpg-agent defaults to putting its socket files
in GNUPGHOME and fails if the path for any of them is longer than
sun_path (108 chars on linux, 104 on OpenBSD and FreeBSD, and likely
similar on other unices).
When building in the typical fedora build tool (mock), the path to the
git test dir is "/builddir/build/BUILD/git-2.20.1/t." That path then
has "trash directory.$TEST_NAME$TEST_STRESS_JOB_SFX" appended and a
"gpghome" directory within. For t5573 and t7612, the gpg-agent socket
path for S.gpg-agent.browser exceeds the sun_path limit and gpg-agent
fails to start. Sadly, this is handled poorly by gpg and makes the
tests fail to set either the GPG or GPGSM prereqs.
For the fedora packages, I decided to pass --root=/tmp/git-t. (via
mktemp, of course) to the test suite which ensures a path short enough
to keep gpg-agent happy.
I don't know if there are other packagers or builders who run into this,
so maybe it's not worth much effort to try and have the test suite cope
better. It took me longer than I would have liked to track it down, so
I thought I'd mention it in case anyone else has run into this or has
thoughts on how to improve lib-gpg.sh while waiting for GnuPG to improve
this area.
A GIT_TEST_GNUPGHOME_ROOT var to set the root path for the GNUPGHOME
dirs in the tests is one thought I had, but didn't try to put it into
patch form. Setting the --root test option is probably enough control
for most cases.
¹ https://dev.gnupg.org/T2964
² https://fedoraproject.org/wiki/Changes/GnuPG2_as_default_GPG_implementation
Todd Zullinger (2):
t/lib-gpg: quote path to ${GNUPGHOME}/trustlist.txt
t/lib-gpg: drop redundant killing of gpg-agent
t/lib-gpg.sh | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
--
Todd
[PATCH 2/2] t/lib-gpg: drop redundant killing of gpg-agent
In 53fc999306 ("gpg-interface t: extend the existing GPG tests with
GPGSM", 2018-07-20), the gpgconf call which kills gpg-agent was copied
from the existing gpg setup code.
The reason for killing gpg-agent is given in 29ff1f8f74 ("t: lib-gpg:
flush gpg agent on startup", 2017-07-20):
When running gpg-relevant tests, a gpg-daemon is spawned for each
GNUPGHOME used. This daemon may stay running after the test and cache
file descriptors for the trash directories, even after the trash
directory is removed. This leads to ENOENT errors when attempting to
create files if tests are run multiple times.
Add a cleanup script to force flushing the gpg-agent for that GNUPGHOME
(if any) before setting up the GPG relevant-environment.
Killing gpg-agent once per test is sufficient.
Signed-off-by: Todd Zullinger
---
t/lib-gpg.sh | 1 -
1 file changed, 1 deletion(-)
diff --git a/t/lib-gpg.sh b/t/lib-gpg.sh
index 207009793b..8d28652b72 100755
--- a/t/lib-gpg.sh
+++ b/t/lib-gpg.sh
@@ -64,7 +64,6 @@ then
tr -d '\n' >"${GNUPGHOME}/trustlist.txt" &&
echo " S relax" >>"${GNUPGHOME}/trustlist.txt" &&
- (gpgconf --kill gpg-agent >/dev/null 2>&1 || : ) &&
echo hello | gpgsm --homedir "${GNUPGHOME}" >/dev/null \
-u commit...@example.com -o /dev/null --sign - 2>&1 &&
test_set_prereq GPGSM
--
Todd
[PATCH] Documentation: build technical/multi-pack-index
The git-multi-pack-index doc links to technical/multi-pack-index.html. Ensure it is built to prevent a broken link. Signed-off-by: Todd Zullinger --- While building 2.20.0-rc0 I noticed the broken link from git-multi-pack-index to technical/multi-pack-index.html. Documentation/Makefile | 1 + 1 file changed, 1 insertion(+) diff --git a/Documentation/Makefile b/Documentation/Makefile index 48d261dc2c..d5d936e6a7 100644 --- a/Documentation/Makefile +++ b/Documentation/Makefile @@ -73,6 +73,7 @@ TECH_DOCS += technical/hash-function-transition TECH_DOCS += technical/http-protocol TECH_DOCS += technical/index-format TECH_DOCS += technical/long-running-process-protocol +TECH_DOCS += technical/multi-pack-index TECH_DOCS += technical/pack-format TECH_DOCS += technical/pack-heuristics TECH_DOCS += technical/pack-protocol -- 2.19.1
Re: [PATCH] t5551-http-fetch-smart.sh: sort cookies before comparing
Jeff King wrote:
> On Fri, Sep 07, 2018 at 07:22:05PM -0400, Todd Zullinger wrote:
>
>> With curl-7.61.1 cookies are sorted by creation-time¹. Sort the output
>> used in the 'cookies stored in http.cookiefile when http.savecookies
>> set' test before comparing it to the expected cookies.
>>
>> ¹ https://github.com/curl/curl/commit/e2ef8d6fa ("cookies: support
>> creation-time attribute for cookies", 2018-08-28)
>
> According to that commit message, the creation-time sort is only for
> cookies of the same length. But it's not clear to me if that just means
> on-the-wire, and curl always stores by creation-time in the cookie file.
Yeah, I didn't dig into the curl code deeply to try and
understand it. I did test with the only the curl package
downgraded to 7.61.0 to confirm the test worked without
sorting. And I saw that the curl commit updated existing
tests to sort the test data.
> Either way, though, I guess it wouldn't matter for us as long as we
> choose some arbitrary re-ordering for what curl produces (i.e., the
> output of `sort`) and then make sure our "expect" output is in the same
> order. Which is basically what your patch does. One question, though:
>
>> diff --git a/t/t5551-http-fetch-smart.sh b/t/t5551-http-fetch-smart.sh
>> index 771f36f9ff..538656bfef 100755
>> --- a/t/t5551-http-fetch-smart.sh
>> +++ b/t/t5551-http-fetch-smart.sh
>> @@ -215,7 +215,7 @@ test_expect_success 'cookies stored in http.cookiefile
>> when http.savecookies set
>> git config http.cookiefile cookies.txt &&
>> git config http.savecookies true &&
>> git ls-remote $HTTPD_URL/smart_cookies/repo.git master &&
>> -tail -3 cookies.txt >cookies_tail.txt &&
>> +tail -3 cookies.txt | sort >cookies_tail.txt &&
>> test_cmp expect_cookies.txt cookies_tail.txt
>> '
>
> We pick the bottom 3 before sorting. How do we know those are the three
> we want to see?
>
> ...Ah, OK. The lines we are skipping are not actually cookies at all,
> but just header cruft. I wonder if:
>
> grep "^[^#]" cookies.txt
>
> would be a better way of doing that, but that is certainly not something
> new.
>
> So this fix looks fine. It might be worth a comment above the creation
> of expect_cookies.txt to mention it must be in sorted order (of course
> anybody modifying it would see a test failure).
I thought about running the expect_cookies.txt file through
sort as well. That would ensure that both files were using
the same sorting. Whether that's needed on any platform
now, I don't know. Maybe that would be a useful way to
protect against future edits to the expect_cookies.txt file
catching the editor?
I thought there might be a test function to sort the output,
but I was (incorrectly) thinking of check_access_log() which
Gábor added in e8b3b2e275 ("t/lib-httpd: avoid occasional
failures when checking access.log", 2018-07-12).
Perhaps it would be useful to have a test_cmp_sorted() to do
the simple dance of sorting the actual & expected. I
haven't looked through the tests to see how often such a
function might be useful.
>> The in-development version of Fedora updated to the recently
>> released curl-7.61.1 in the past few days. This isn't
>> breakage from the 2.19.0 cycle, but if the fix looks good to
>> everyone it would be nice to include it. That way other
>> distributions and users who update git and curl to the most
>> recent releases won't run into this test failure.
>>
>> I tested this against Fedora 30 (curl-7.61.1) as well as
>> previous releases from RHEL/CentOS 6/7 (7.19.7/7.29.0) and
>> Fedora 27/28/29 (7.55.1/7.59.0/7.61.0).
>
> You're pretty late in the 2.19 cycle, since the release is tentatively
> scheduled for Sunday. Though since this is just touching the test
> script, and since it looks Obviously Correct, I'm not opposed.
Yep, I knew the final was coming very soon. I would not
have been surprised to see it land tonight while I was
finishing my testing of this patch. :)
I'm certainly covered for the Fedora packages. It's hard to
say whether there are many other users/packagers who might
upgrade both git and curl and run into this. So it may not
be worth even a small risk of making the change at this
point.
On the other hand, the change only affects one test and may
be safe enough to apply. I'll leave that choice in the
capable hands of our maintainer and the good folks here.
Thanks for a thoughtful review, as always.
--
Todd
~~
That which seems the height of absurdity in one generation often
becomes the height of wisdom in the next.
-- John Stuart Mill (1806-1873)
[PATCH] t5551-http-fetch-smart.sh: sort cookies before comparing
With curl-7.61.1 cookies are sorted by creation-time¹. Sort the output
used in the 'cookies stored in http.cookiefile when http.savecookies
set' test before comparing it to the expected cookies.
¹ https://github.com/curl/curl/commit/e2ef8d6fa ("cookies: support
creation-time attribute for cookies", 2018-08-28)
Signed-off-by: Todd Zullinger
---
[Resending with the list in Cc; sorry for spamming you,
Junio, Jeff, and Gábor.]
The in-development version of Fedora updated to the recently
released curl-7.61.1 in the past few days. This isn't
breakage from the 2.19.0 cycle, but if the fix looks good to
everyone it would be nice to include it. That way other
distributions and users who update git and curl to the most
recent releases won't run into this test failure.
I tested this against Fedora 30 (curl-7.61.1) as well as
previous releases from RHEL/CentOS 6/7 (7.19.7/7.29.0) and
Fedora 27/28/29 (7.55.1/7.59.0/7.61.0).
The verbose output is:
expecting success:
git config http.cookiefile cookies.txt &&
git config http.savecookies true &&
git ls-remote $HTTPD_URL/smart_cookies/repo.git master &&
tail -3 cookies.txt >cookies_tail.txt &&
test_cmp expect_cookies.txt cookies_tail.txt
++ git config http.cookiefile cookies.txt
++ git config http.savecookies true
++ git ls-remote http://127.0.0.1:5551/smart_cookies/repo.git master
7ae89caac6c721f16555e981eaeed64abc165c5drefs/heads/master
263207bb5fbfbefbdf1c9c3fa4ae5d9663323217
refs/namespaces/ns/refs/heads/master
++ tail -3 cookies.txt
++ test_cmp expect_cookies.txt cookies_tail.txt
++ diff -u expect_cookies.txt cookies_tail.txt
--- expect_cookies.txt 2018-09-07 07:29:05.231532462 +
+++ cookies_tail.txt2018-09-07 07:29:05.306532366 +
@@ -1,3 +1,3 @@
-127.0.0.1 FALSE /smart_cookies/ FALSE 0 othername
othervalue
127.0.0.1 FALSE /smart_cookies/repo.git/info/ FALSE 0 name
value
+127.0.0.1 FALSE /smart_cookies/ FALSE 0 othername
othervalue
error: last command exited with $?=1
not ok 22 - cookies stored in http.cookiefile when http.savecookies set
t/t5551-http-fetch-smart.sh | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/t/t5551-http-fetch-smart.sh b/t/t5551-http-fetch-smart.sh
index 771f36f9ff..538656bfef 100755
--- a/t/t5551-http-fetch-smart.sh
+++ b/t/t5551-http-fetch-smart.sh
@@ -215,7 +215,7 @@ test_expect_success 'cookies stored in http.cookiefile when
http.savecookies set
git config http.cookiefile cookies.txt &&
git config http.savecookies true &&
git ls-remote $HTTPD_URL/smart_cookies/repo.git master &&
- tail -3 cookies.txt >cookies_tail.txt &&
+ tail -3 cookies.txt | sort >cookies_tail.txt &&
test_cmp expect_cookies.txt cookies_tail.txt
'
--
2.19.0.rc2
Re: [RFC/PATCH] drop vcs-svn experiment
Hi Jeff, Jeff King wrote: > .gitignore | 1 - > Makefile | 22 -- > contrib/svn-fe/.gitignore | 4 - > contrib/svn-fe/Makefile| 105 --- > contrib/svn-fe/svn-fe.c| 18 -- > contrib/svn-fe/svn-fe.txt | 71 - > contrib/svn-fe/svnrdump_sim.py | 68 - > remote-testsvn.c | 337 > t/helper/test-line-buffer.c| 81 - > t/helper/test-svn-fe.c | 52 > t/t9020-remote-svn.sh | 89 -- Doesn't t/t9010-svn-fe.sh also need to be removed? It uses the test-svn-fe helper which is removed. The Fedora git-svn package has included git-remote-testsvn for years now but no one has ever filed any bug reports about it. I looked at whether it should be packaged last year. I came to the conclusion that while it could be used outside of the test suite it was doubtful it actually was. -- Todd ~~ No one ever went broke underestimating the taste of the American public. -- H. L. Mencken
Re: [RFC PATCH 1/1] recover: restoration of deleted worktree files
Hi, Robert P. J. Day wrote: > On Sat, 4 Aug 2018, Junio C Hamano wrote: >> In other words, I think this patch can be a fine addition to >> somebody else's project (i.e. random collection of scripts that may >> help Git users), so let's see how I can offer comments/inputs to >> help you improve it. So I won't comment on lang, log message, or >> shell scripting style---these are project convention and the >> git-core convention won't be relevant to this patch. > > not sure how relevant this is, but fedora bundles a bunch of neat > utilities into two packages: git-tools and git-extras. i have no idea > what relationship those packages have to official git, or who decides > what goes into them. For anyone curious, those packages (git-extras and git-tools) are both entirely separate projects upstream and in the fedora packaging. A git-recover script may well be a good fit in one of those upstream projects. The git-(extras|tools) package names are a bit confusing IMO. But it's probably more confusing that they each add a number of git-* commands in the default PATH the way they're packaged. We do package some bits from contrib/ (e.g. completion, subtree, etc.) in the fedora git packages. We don't add scripts and commands from outside of the git tarballs as part of the fedora git package, though. So far, I don't recall anyone filing a bug report about commands from git-extras or git-tools against git. So it seems that users of those additional packages aren't being confused, thankfully. -- Todd ~~ Between two evils, I always pick the one I never tried before. -- Mae West
Re: [PATCH] doc: substitute ETC_GIT(CONFIG|ATTRIBUTES) in generated docs
Jeff King wrote:
> We could get rid around that by using $(DOC_ETC_GITCONFIG) or something,
> with:
>
> DOC_ETC_GITCONFIG ?= $(ETC_GITCONFIG)
>
> in the Makefile. But that still leaves the choice of which generic text
> to use up to the caller. Maybe we should provide more guidance.
>
> I.e., provide a knob like DOC_GENERIC that fills in generic values for
> these values (and maybe some others; it sounds like we have some
> existing problem cases).
That sounds pretty reasonable. I have something
implementing this below.
>> It might be enough if the default values are relatively sane
>> and consistent. That would be a slight improvement over the
>> current situation still.
>
> Yeah. Taking a step back from the implementation questions, I think
> "$(prefix)/etc/gitconfig" is not very helpful text. I'd be happy to see
> us come up with a generic way of saying that which is more
> comprehensible to end-users. Your patches side-step that by filling in
> the real value, but unfortunately we can't do that everywhere. :)
>
> There may not be a good "token" value, though. I.e., we may need to have
> two sets of verbiage: the specific one, and the generic one.
Yeah. About the best generic term I can come up with is
using '$sysconfdir'. But I have no idea if that's something
most readers will recognize as a placeholder for something
like /etc.
A number of the path references are in the FILES sections of
the man pages. It might not be much of an improvement if we
try to stuff too much text in those references. Perhaps if
those used '$sysconfdir/gitconfig' a subsequent note could
expand on that? It could even be wrapped in an ifdef
similar to that used for the default editor/pager.
I don't want to make the plain .txt files significantly less
readable in the process, of course.
>>> It's a shame we have to repeat this logic from the Makefile, though I
>>> guess we already do so for prefix, bindir, etc, so far.
>>>
>>> Should we factor the path logic from the top-level Makefile into an
>>> include that can be used from either?
>>
>> Yeah, maybe. I didn't like the duplication either, but as
>> you noticed, we do it already for many of the other
>> variables. I suspect we could put these defaults into
>> config.mak.uname which Documentation/Makefile includes and
>> then you could run 'make -C Documentation' in a fresh clone
>> or tarball and get docs built with the defaults set for each
>> platform.
>
> I think it shouldn't go into config.mak.uname, since the idea there was
> to keep the long list of platform defaults separate from other logic
> (the Makefile is already long enough!). So I'm basically proposing the
> same thing but in its own file. :)
Ahh, something that the top-level Makefile would create and
then subdir Makefiles would include, perhaps similar to the
way GIT-VERSION-FILE is updated and included? That could
prove useful to some of the tools in contrib which duplicate
logic.
Skipping that larger de-duplication cleanup, here's a stab
at implementing the DOC_GENERIC knob (and the DOC_ overrides
for ETC_GIT(CONFIG|ATTRIBUTES). The defaults with
'/GENERIC-SYSCONFDIR' in them are just placeholders waiting
for a better name. :)
Similarly, the use of {etc-git(config|attributes)} as the
attribute for the replacements could likely be improved for
readers of the .txt files. {system-wide-gitconfig} is
likely better. Maybe the default for the generic paths
could be /system-wide/git(config|attributes) too (or in
CAPS to make it more obviously a placeholder)?
Thanks for thinking this through and providing some good
directions to work on!
-- >8 --
Subject: [PATCH] doc: substitute ETC_GIT(CONFIG|ATTRIBUTES) in generated docs
Replace `$(prefix)/etc/gitconfig` and `$(prefix)/etc/gitattributes` in
generated documentation with the paths chosen when building. Readers of
the documentation should not need to know how `$(prefix)` was defined.
It's also more consistent than sometimes using `$(prefix)/etc/gitconfig`
and other times using `/etc/gitconfig` to refer to the system-wide
config file.
Update the SYNOPSIS of gitattributes(5) to include the system-wide
config file as well.
Support building generic documentation with a DOC_GENERIC Makefile knob.
The default generic paths may be further customized via the
DOC_ETC_GITCONFIG and DOC_ETC_GITATTRIBUTES variables.
Define DOC_GENERIC in dist-doc make target to ensure generic paths are
used in the generated html and manpage tarballs.
Helped-by: Jeff King
Signed-off-by: Todd Zullinger
---
Documentation/Makefile | 22 ++
Documentation/config.txt| 4 ++--
Documentation/git-config.txt| 10 +-
Do
Re: [PATCH] doc: substitute ETC_GIT(CONFIG|ATTRIBUTES) in generated docs
Junio C Hamano wrote: > Jeff King writes: > >> Specifically, I'm thinking of: >> >> 1. The pre-built documentation that Junio builds for >> quick-install-doc. This _could_ be customized during the "quick" >> step, but it's probably not worth the effort. However, we'd want >> some kind of generic fill-in then, and hopefully not >> "/home/jch/etc" or something. > > That is very likely to happen, actually X-<. Obviously, we don't want the end result to cause regressions in the common case or any burden on you. Would setting the ETC_GIT(CONFIG|ATTRIBUTES) variables in the dist-doc target alleviate that concern? Alternately, we can make the default use generic paths and require some other knob to enable substituting the actual paths when building documentation. I tend to think that the default should be to build documentation that is accurate for that build, but since it's something I'll set once for my package builds it's not a big deal either way to me. -- Todd ~~ Einstein argued that there must be simplified explanations of nature, because God is not capricious or arbitrary. No such faith comforts the software engineer. -- Fred Brooks
Re: [PATCH 1/2] gitignore.txt: clarify default core.excludesfile path
Junio C Hamano wrote: > Todd Zullinger writes: > >> The default core.excludesfile path is $XDG_CONFIG_HOME/git/ignore. >> $HOME/.config/git/ignore is used if XDG_CONFIG_HOME is empty or unset, > > ... because $HOME/.config is the default value for XDG_CONFIG_HOME > when it is unset, that is? If so, the change makes sense. Indeed, that's the fallback path. Thanks, -- Todd
Re: [PATCH] doc: substitute ETC_GIT(CONFIG|ATTRIBUTES) in generated docs
I wrote: > Jeff King wrote: >> (Related, there's a build target in the local Makefile for using >> asciidoctor; does it need updated, too?) > > I didn't test asciidoctor specficially, but it also respects > the ASCIIDOC_EXTRA parameters, so I think it will work just > as well. I'll try to confirm that later today. Testing confirmed that asciidoctor works fine with this as well. Somewhat tangentially, I looked at using asciidoctor for the Fedora packages last year and one issue that kept me from using it then was the '[FIXME: source]' it includes in the footer of the manpage. When I dug into it at the time, it appeared this was due to no declaration (similarly missing for manual, and version). It wasn't clear whether it was possible to include a custom header template in plain asciidoctor. I got the impression that it would require using a custom backend, which in turn required the rubygem 'tilt' for processing. I spent about an hour poking around with it and decided that I'd put off building with asciidoctor until that was fixed. I felt that displaying '[FIXME: source]' wass worse than simply not including the version. It's always possible that I was doing something wrong in my use of asciidoctor (I just set USE_ASCIIDOCTOR). Or maybe the Fedora packages are missing some dependency which I missed. It might also be that we need some adjustments similar to https://patchwork.kernel.org/patch/10360207/ to get the mansource attribute passed on to asciidoctor. I only just ran across that patch and haven't had a chance to test sometime similar in the git manpage build. That looks promising though. -- Todd ~~ Why is it that we rejoice at a birth and grieve at a funeral? It is because we are not the person involved. -- Mark Twain
Re: [PATCH] doc: substitute ETC_GIT(CONFIG|ATTRIBUTES) in generated docs
Jeff King wrote:
> On Wed, Jun 27, 2018 at 12:56:37AM -0400, Todd Zullinger wrote:
>
>> Replace `$(prefix)/etc/gitconfig` and `$(prefix)/etc/gitattributes` in
>> generated documentation with the paths chosen when building. Readers of
>> the documentation should not need to know how `$(prefix)` was defined.
>
> Yes, I was just complaining about this yesterday.
So what you're saying is that if I had procrastinated a
little, you may have written such a patch for me? :)
> Besides readers not
> having any clue what $(prefix) means here, $(prefix)/etc is not even
> correct for builds with prefix=/usr.
>
> So I like the overall direction here, but it leaves me with one
> question: what happens for documentation outside of customized builds?
>
> Specifically, I'm thinking of:
>
> 1. The pre-built documentation that Junio builds for
> quick-install-doc. This _could_ be customized during the "quick"
> step, but it's probably not worth the effort. However, we'd want
> some kind of generic fill-in then, and hopefully not
> "/home/jch/etc" or something.
If building docs separately for such a use, the values can
be overridden by setting ETC_GITCONFIG and ETC_GITATTRIBUTES
(or prefix or sysconfig). To keep the same values as we
currently use, for example:
make -C Documentation V=1 \
ETC_GITCONFIG='$$(prefix)/etc/gitconfig' \
ETC_GITATTRIBUTES='$$(prefix)/etc/gitattributes'
I don't know if that's sufficient for folks who build
documentation to share with a general audience or not.
It might be enough if the default values are relatively sane
and consistent. That would be a slight improvement over the
current situation still.
> 2. The manpages on git-scm.com, which are built with asciidoctor. I
> think we'd want the same generic value there. Ideally it would be
> embedded in the asciidoc source as "if this attribute isn't
> defined, then use this", but it's not the end of the world to
> require a patch to the site to handle this.
We have that for the DEFAULT_(EDITOR|PAGER). I didn't know
if we'd want that here, but maybe it's worth the effort if
it helps when building docs destined for the website and
such. It might make the plain text files a bit less
readable though.
The EDITOR/PAGER bits are in git-var.txt, like this:
GIT_EDITOR::
Text editor for use by Git commands. The value is meant to be
interpreted by the shell when it is used. Examples: `~/bin/vi`,
`$SOME_ENVIRONMENT_VARIABLE`, `"C:\Program Files\Vim\gvim.exe"
--nofork`. The order of preference is the `$GIT_EDITOR`
environment variable, then `core.editor` configuration, then
`$VISUAL`, then `$EDITOR`, and then the default chosen at compile
time, which is usually 'vi'.
ifdef::git-default-editor[]
The build you are using chose '{git-default-editor}' as the default.
endif::git-default-editor[]
The ifdef would be a little different to set the var if it
was not set, of course.
I think if we ensure that ETC_GITCONFIG / ETC_GITATTRIBUTES
are set sanely by default (and easily overridden) then we
can probably avoid the need to handle it within the asciidoc
file though. (There's more on that though below.)
> (Related, there's a build target in the local Makefile for using
> asciidoctor; does it need updated, too?)
I didn't test asciidoctor specficially, but it also respects
the ASCIIDOC_EXTRA parameters, so I think it will work just
as well. I'll try to confirm that later today.
>> diff --git a/Documentation/Makefile b/Documentation/Makefile
>> index d079d7c73a..75af671798 100644
>> --- a/Documentation/Makefile
>> +++ b/Documentation/Makefile
>> @@ -95,6 +95,7 @@ DOC_MAN7 = $(patsubst %.txt,%.7,$(MAN7_TXT))
>>
>> prefix ?= $(HOME)
>> bindir ?= $(prefix)/bin
>> +sysconfdir ?= $(prefix)/etc
>> htmldir ?= $(prefix)/share/doc/git-doc
>> infodir ?= $(prefix)/share/info
>> pdfdir ?= $(prefix)/share/doc/git-doc
>> @@ -205,6 +206,18 @@ DEFAULT_EDITOR_SQ = $(subst ','\'',$(DEFAULT_EDITOR))
>> ASCIIDOC_EXTRA += -a 'git-default-editor=$(DEFAULT_EDITOR_SQ)'
>> endif
>>
>> +ifndef ETC_GITCONFIG
>> +ETC_GITCONFIG = $(sysconfdir)/gitconfig
>> +endif
>> +ETC_GITCONFIG_SQ = $(subst ','\'',$(ETC_GITCONFIG))
>> +ASCIIDOC_EXTRA += -a 'etc-gitconfig=$(ETC_GITCONFIG_SQ)'
>> +
>> +ifndef ETC_GITATTRIBUTES
>> +ETC_GITATTRIBUTES = $(sysconfdir)/gitattributes
>> +endif
>> +ETC_GITATTRIBUTES_SQ =
[PATCH] doc: substitute ETC_GIT(CONFIG|ATTRIBUTES) in generated docs
Replace `$(prefix)/etc/gitconfig` and `$(prefix)/etc/gitattributes` in
generated documentation with the paths chosen when building. Readers of
the documentation should not need to know how `$(prefix)` was defined.
It's also more consistent than sometimes using `$(prefix)/etc/gitconfig`
and other times using `/etc/gitconfig` to refer to the system-wide
config file.
Update the SYNOPSIS of gitattributes(5) to include the system-wide
config file as well.
Signed-off-by: Todd Zullinger
---
I noticed this while looking to update gitattributes.txt to
note the system-wide config file. I tested with and without
RUNTIME_PREFIX as well as make gitattributes.5 from within
Documentation. I believe all methods do the right thing.
I couldn't figure out a good way to have asciidoc expand the
attributes inside of a "`" literal, so I changed to the "+"
form. There might be a better way to do this, passing subs=
in asciidoc.conf, but I wasn't clear on where that would
fit. I tested with asciidoc and not asciidoctor.
Documentation/Makefile | 13 +
Documentation/config.txt| 4 ++--
Documentation/git-config.txt| 10 +-
Documentation/git.txt | 4 ++--
Documentation/gitattributes.txt | 4 ++--
5 files changed, 24 insertions(+), 11 deletions(-)
diff --git a/Documentation/Makefile b/Documentation/Makefile
index d079d7c73a..75af671798 100644
--- a/Documentation/Makefile
+++ b/Documentation/Makefile
@@ -95,6 +95,7 @@ DOC_MAN7 = $(patsubst %.txt,%.7,$(MAN7_TXT))
prefix ?= $(HOME)
bindir ?= $(prefix)/bin
+sysconfdir ?= $(prefix)/etc
htmldir ?= $(prefix)/share/doc/git-doc
infodir ?= $(prefix)/share/info
pdfdir ?= $(prefix)/share/doc/git-doc
@@ -205,6 +206,18 @@ DEFAULT_EDITOR_SQ = $(subst ','\'',$(DEFAULT_EDITOR))
ASCIIDOC_EXTRA += -a 'git-default-editor=$(DEFAULT_EDITOR_SQ)'
endif
+ifndef ETC_GITCONFIG
+ETC_GITCONFIG = $(sysconfdir)/gitconfig
+endif
+ETC_GITCONFIG_SQ = $(subst ','\'',$(ETC_GITCONFIG))
+ASCIIDOC_EXTRA += -a 'etc-gitconfig=$(ETC_GITCONFIG_SQ)'
+
+ifndef ETC_GITATTRIBUTES
+ETC_GITATTRIBUTES = $(sysconfdir)/gitattributes
+endif
+ETC_GITATTRIBUTES_SQ = $(subst ','\'',$(ETC_GITATTRIBUTES))
+ASCIIDOC_EXTRA += -a 'etc-gitattributes=$(ETC_GITATTRIBUTES_SQ)'
+
QUIET_SUBDIR0 = +$(MAKE) -C # space to separate -C and subdir
QUIET_SUBDIR1 =
diff --git a/Documentation/config.txt b/Documentation/config.txt
index 1cc18a828c..ed903b60bd 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -5,7 +5,7 @@ The Git configuration file contains a number of variables that
affect
the Git commands' behavior. The `.git/config` file in each repository
is used to store the configuration for that repository, and
`$HOME/.gitconfig` is used to store a per-user configuration as
-fallback values for the `.git/config` file. The file `/etc/gitconfig`
+fallback values for the `.git/config` file. The file +{etc-gitconfig}+
can be used to store a system-wide default configuration.
The configuration variables are used by both the Git plumbing
@@ -2815,7 +2815,7 @@ configuration files (e.g. `$HOME/.gitconfig`).
Example:
-/etc/gitconfig
+{etc-gitconfig}
push.pushoption = a
push.pushoption = b
diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt
index 18ddc78f42..0d5ea5b58e 100644
--- a/Documentation/git-config.txt
+++ b/Documentation/git-config.txt
@@ -114,10 +114,10 @@ See also <>.
--system::
For writing options: write to system-wide
- `$(prefix)/etc/gitconfig` rather than the repository
+ +{etc-gitconfig}+ rather than the repository
`.git/config`.
+
-For reading options: read only from system-wide `$(prefix)/etc/gitconfig`
+For reading options: read only from system-wide +{etc-gitconfig}+
rather than from all available files.
+
See also <>.
@@ -263,7 +263,7 @@ FILES
If not set explicitly with `--file`, there are four files where
'git config' will search for configuration options:
-$(prefix)/etc/gitconfig::
+{etc-gitconfig}::
System-wide configuration file.
$XDG_CONFIG_HOME/git/config::
@@ -310,11 +310,11 @@ ENVIRONMENT
GIT_CONFIG::
Take the configuration from the given file instead of .git/config.
Using the "--global" option forces this to ~/.gitconfig. Using the
- "--system" option forces this to $(prefix)/etc/gitconfig.
+ "--system" option forces this to {etc-gitconfig}.
GIT_CONFIG_NOSYSTEM::
Whether to skip reading settings from the system-wide
- $(prefix)/etc/gitconfig file. See linkgit:git[1] for details.
+ {etc-gitconfig} file. See linkgit:git[1] for details.
See also <>.
diff --git a/Documentation/git.txt b/Documentation/git.txt
index dba7f0c18e..6a4646f991 100644
--- a/Documentation/git.txt
+++ b/Documentation/git.txt
@
[PATCH 1/2] gitignore.txt: clarify default core.excludesfile path
The default core.excludesfile path is $XDG_CONFIG_HOME/git/ignore. $HOME/.config/git/ignore is used if XDG_CONFIG_HOME is empty or unset, as described later in the document. Signed-off-by: Todd Zullinger --- Documentation/gitignore.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Documentation/gitignore.txt b/Documentation/gitignore.txt index ff5d7f9ed6..d107daaffd 100644 --- a/Documentation/gitignore.txt +++ b/Documentation/gitignore.txt @@ -7,7 +7,7 @@ gitignore - Specifies intentionally untracked files to ignore SYNOPSIS -$HOME/.config/git/ignore, $GIT_DIR/info/exclude, .gitignore +$XDG_CONFIG_HOME/git/ignore, $GIT_DIR/info/exclude, .gitignore DESCRIPTION --- -- 2.18.0
