Re: What's cooking in git.git (Sep 2018, #01; Tue, 4)
On Sun, Nov 25 2018, Torsten Bögershausen wrote:
> On Sun, Nov 25, 2018 at 05:28:35AM +0100, Torsten Bögershausen wrote:
>> On Sat, Nov 24, 2018 at 08:33:37PM +0100, Ævar Arnfjörð Bjarmason wrote:
>> >
>> > On Wed, Sep 05 2018, Ævar Arnfjörð Bjarmason wrote:
>> >
>> > > On Wed, Sep 05 2018, Eric Sunshine wrote:
>>
>> []
>>
>> > > SunCC used to be ahead of GCC & Clang when it came to certain classes of
>> > > warnings, but e.g. now everything it complains about is because it
>> > > doesn't understand C as well, e.g. we have quite a few compile warnings
>> > > due to code like this, which it claims is unreachable (but isn't):
>> > > https://github.com/git/git/blob/v2.19.0-rc2/read-cache.c#L950-L955
>> >
>>
>> Wait a second - even if the compiler claims something (wrong)...
>> there a still 1+1/2 questions from my side:
>>
>>
>> int verify_path(const char *path, unsigned mode)
>> {
>> char c;
>> ^
>> /* Q1: should "c" be initialized like this: */
>> char c = *path;
>>
>> if (has_dos_drive_prefix(path))
>> return 0;
>>
>> goto inside;
>> /* Q2: and why do we need the "goto" here ? */
>> for (;;) {
>> if (!c)
>> return 1;
>> if (is_dir_sep(c)) {
>> inside:
>
> After some re-reading,
> I think that the "goto inside" was just hard to read
>
> Out of interest:
> would the following make the compiler happy ?
>
>
> diff --git a/read-cache.c b/read-cache.c
> index 49add63fe1..d574d58b9d 100644
> --- a/read-cache.c
> +++ b/read-cache.c
> @@ -951,17 +951,15 @@ static int verify_dotfile(const char *rest, unsigned
> mode)
>
> int verify_path(const char *path, unsigned mode)
> {
> - char c;
> + char c = *path ? '/' : '\0';
>
> if (has_dos_drive_prefix(path))
> return 0;
>
> - goto inside;
> for (;;) {
> if (!c)
> return 1;
> if (is_dir_sep(c)) {
> -inside:
> if (protect_hfs) {
> if (is_hfs_dotgit(path))
> return 0;
I haven't tested (it's tedious) but yes, I can tell you SunCC won't
whine about this. I've only seen its unreachability detector get
confused about goto inside a loop, not the the sort of code you've
replaced this with.
We should not be appeasing these old compiler warnings in cases where
they're wrong. You can check out the CI output I linked to to see 10-20
cases in the codebase where SunCC is wrong about unreliability.
Whether we should just fix this for its own sake is another question.
Re: What's cooking in git.git (Sep 2018, #01; Tue, 4)
On Sun, Nov 25, 2018 at 05:28:35AM +0100, Torsten Bögershausen wrote:
> On Sat, Nov 24, 2018 at 08:33:37PM +0100, Ævar Arnfjörð Bjarmason wrote:
> >
> > On Wed, Sep 05 2018, Ævar Arnfjörð Bjarmason wrote:
> >
> > > On Wed, Sep 05 2018, Eric Sunshine wrote:
>
> []
>
> > > SunCC used to be ahead of GCC & Clang when it came to certain classes of
> > > warnings, but e.g. now everything it complains about is because it
> > > doesn't understand C as well, e.g. we have quite a few compile warnings
> > > due to code like this, which it claims is unreachable (but isn't):
> > > https://github.com/git/git/blob/v2.19.0-rc2/read-cache.c#L950-L955
> >
>
> Wait a second - even if the compiler claims something (wrong)...
> there a still 1+1/2 questions from my side:
>
>
> int verify_path(const char *path, unsigned mode)
> {
> char c;
>^
> /* Q1: should "c" be initialized like this: */
> char c = *path;
>
> if (has_dos_drive_prefix(path))
> return 0;
>
> goto inside;
> /* Q2: and why do we need the "goto" here ? */
> for (;;) {
> if (!c)
> return 1;
> if (is_dir_sep(c)) {
> inside:
After some re-reading,
I think that the "goto inside" was just hard to read
Out of interest:
would the following make the compiler happy ?
diff --git a/read-cache.c b/read-cache.c
index 49add63fe1..d574d58b9d 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -951,17 +951,15 @@ static int verify_dotfile(const char *rest, unsigned mode)
int verify_path(const char *path, unsigned mode)
{
- char c;
+ char c = *path ? '/' : '\0';
if (has_dos_drive_prefix(path))
return 0;
- goto inside;
for (;;) {
if (!c)
return 1;
if (is_dir_sep(c)) {
-inside:
if (protect_hfs) {
if (is_hfs_dotgit(path))
return 0;
Re: What's cooking in git.git (Sep 2018, #01; Tue, 4)
On Sat, Nov 24, 2018 at 08:33:37PM +0100, Ævar Arnfjörð Bjarmason wrote:
>
> On Wed, Sep 05 2018, Ævar Arnfjörð Bjarmason wrote:
>
> > On Wed, Sep 05 2018, Eric Sunshine wrote:
[]
> > SunCC used to be ahead of GCC & Clang when it came to certain classes of
> > warnings, but e.g. now everything it complains about is because it
> > doesn't understand C as well, e.g. we have quite a few compile warnings
> > due to code like this, which it claims is unreachable (but isn't):
> > https://github.com/git/git/blob/v2.19.0-rc2/read-cache.c#L950-L955
>
Wait a second - even if the compiler claims something (wrong)...
there a still 1+1/2 questions from my side:
int verify_path(const char *path, unsigned mode)
{
char c;
^
/* Q1: should "c" be initialized like this: */
char c = *path;
if (has_dos_drive_prefix(path))
return 0;
goto inside;
/* Q2: and why do we need the "goto" here ? */
for (;;) {
if (!c)
return 1;
if (is_dir_sep(c)) {
inside:
Re: What's cooking in git.git (Sep 2018, #01; Tue, 4)
On Wed, Sep 05 2018, Ævar Arnfjörð Bjarmason wrote:
> On Wed, Sep 05 2018, Eric Sunshine wrote:
>
>> On Wed, Sep 5, 2018 at 4:29 AM Ævar Arnfjörð Bjarmason
>> wrote:
>>> I recently gained access to a Solaris 10 SPARC (5.10) box and discovered
>>> that the chainlint.sed implementation in 2.19.0 has more sed portability
>>> issues.
>>>
>>> First, whoever implemented the /bin/sed on Solaris apparently read the
>>> POSIX requirements for a max length label of 8 to mean that 8 characters
>>> should include the colon, so a bunch of things fail because of that, but
>>> are fixed with a shorter 7 character label.
>>
>> I'm pretty sure that Solaris 'sed' predates POSIX by a good bit, but
>> that's neither here nor there.
>>
>>> Then GIT_TEST_CHAIN_LINT=1 still fails because 878f988350 ("t/test-lib:
>>> teach --chain-lint to detect broken &&-chains in subshells", 2018-07-11)
>>> added a "grep -q" invocation. The /bin/grep on that version of Solaris
>>> doesn't have -q.
>>
>> I knew that '-q' was potentially problematic on some platforms, so I
>> checked and saw that existing tests were already using it, thus went
>> ahead and used it. Dropping '-q' here and redirecting stderr to
>> /dev/null is a perfectly fine alternative.
>>
>>> We fixed a similar issue way back in 80700fde91
>>> ("t/t1304: make a second colon optional in the mask ACL check",
>>> 2010-03-15) by redirecting to /dev/null instead.
>>>
>>> A bunch of other tests in the test suite rely on "grep -q", but nothing
>>> as central as chainlint, so it makes everything break. Do we want to
>>> away with "grep -q" entirely because of old Solaris /bin/grep?
>>
>> I count 132 instances in existing tests (though, I may have missed some).
>>
>>> At this point those familiar with Solaris are screaming ("why are you
>>> using anything in /bin!"). Okey fine, but it mostly worked before, so
>>> are we OK with breaking it? "Mostly" here is "test suite would fail
>>> 20-30 tests for various reasons, but at least no failures in test-lib.sh
>>> and the like".
>>>
>>> However, if as config.mak.uname does we run the tests with
>>> PATH=/usr/xpg6/bin:/usr/xpg4/bin:$PATH, at this point sed is fine with 8
>>> character labels [...]
>>
>> So, if you run the tests via 'make test' (or whatever), then you get
>> /usr/xpg?/bin in PATH, but if you run an individual test script (and
>> haven't set your PATH appropriately), then you encounter the problems
>> you report above?
>
> You need to manually set the PATH before running the tests, the
> SANE_TOOL_PATH just affects git-sh-setup. We should probably fix that,
> i.e. now if you compile git our shellscripts will use the fixed paths,
> but not the tests.
>
>>> [...] but starts complaining about this (also in
>>> chainlint.sed):
>>>
>>> sed: Too many commands, last: s/\n//
>>
>> Erm. Any additional context to help narrow this down?
>
> I tried playing around with it a bit, but honestly don't understand
> enough of this advanced sed syntax to make any headway, it's complaining
> about e.g. the "check for multi-line double-quoted string" rule, but
> then if you remove the s/\n// from there it complains about the next
> rule in that format.
>
> If you want to dig into this yourself I think the best way forward is
> the last two paragraphs of
> https://public-inbox.org/git/20180824152016.20286-1-ava...@gmail.com/
>
>>> diff --git a/config.mak.uname b/config.mak.uname
>>> @@ -163,6 +163,10 @@ ifeq ($(uname_S),SunOS)
>>> BASIC_CFLAGS += -D__EXTENSIONS__ -D__sun__
>>> + # t/chainlint.sed is hopelessly broken all known (tested
>>> + # Solaris 10 & 11) versions of Solaris, both /bin/sed and
>>> + # /usr/xpg4/bin/sed
>>> + GIT_TEST_CHAIN_LINT = 0
>>> endif
>>>
>>> It slightly sucks to not have chainlint on
>>> Solaris, but it would also suck to revert chainlint.sed back to 2.18.0
>>> (there were some big improvements). So I think the patch above is the
>>> best way forward, especially since we're on rc2. What do you think?
>>
>> Keeping in mind that the main goal of 'chainlint' is to prevent _new_
>> breakage from entering the test suite, disabling 'chainlint' on
>> Solaris is an entirely reasonable way forward. In present day, it
>> seems quite unlikely that we'll see someone develop new tests on
>> Solaris, so having 'chainlint' disabled there isn't likely to be a big
>> deal. Moreover, if someone does write a new test on Solaris which has
>> a broken &&-chain in a subshell, that breakage will be caught very
>> quickly once the test is run by anyone on Linux or MacOS (or Windows
>> or BSD or AIX), so it's not like the broken test will make it into the
>> project undetected.
>
> Since writing my initial message I see that the CSW package packaging
> git on Solaris just uses GNU userland:
> https://sourceforge.net/p/gar/code/HEAD/tree/csw/mgar/pkg/git/trunk/Makefile
>
> I.e. it puts /opt/csw/gnu in its $PATH, so this whole thing is probably
> a non-issue. I.e. if the
Re: What's cooking in git.git (Sep 2018, #01; Tue, 4)
Jeff King writes: >> [1]: >> https://public-inbox.org/git/20180830075431.gf11...@sigill.intra.peff.net/ > > Yeah, I'm not sure which is easier for Junio. I figured by replying > inline, it makes it easy to pick up on top of the others (since it > really does depend on them and should be in the same topic). But it is > easier to overlook as "meh, just more discussion". > > Either way, yes, I'd be happy to see that patch on top. Thanks.
Re: What's cooking in git.git (Sep 2018, #01; Tue, 4)
On Wed, Sep 05, 2018 at 05:50:04AM -0400, Eric Sunshine wrote: > On Tue, Sep 4, 2018 at 6:36 PM Junio C Hamano wrote: > > * es/worktree-forced-ops-fix (2018-08-30) 9 commits > > - worktree: delete .git/worktrees if empty after 'remove' > > - worktree: teach 'remove' to override lock when --force given twice > > - worktree: teach 'move' to override lock when --force given twice > > - worktree: teach 'add' to respect --force for registered but missing path > > - worktree: disallow adding same path multiple times > > - worktree: prepare for more checks of whether path can become worktree > > - worktree: generalize delete_git_dir() to reduce code duplication > > - worktree: move delete_git_dir() earlier in file for upcoming new callers > > - worktree: don't die() in library function find_worktree() > > > > Various subcommands of "git worktree" take '--force' but did not > > behave sensibly, which has been corrected. > > This series is missing a patch[1] from Peff which he wanted placed at > the end of the series. It was probably overlooked since he embedded > it as a reply in that thread rather than sending it as a standalone > patch. > > [1]: > https://public-inbox.org/git/20180830075431.gf11...@sigill.intra.peff.net/ Yeah, I'm not sure which is easier for Junio. I figured by replying inline, it makes it easy to pick up on top of the others (since it really does depend on them and should be in the same topic). But it is easier to overlook as "meh, just more discussion". Either way, yes, I'd be happy to see that patch on top. -Peff
Re: What's cooking in git.git (Sep 2018, #01; Tue, 4)
On Wed, Sep 5, 2018 at 5:14 AM Eric Sunshine wrote: > On Tue, Sep 4, 2018 at 6:36 PM Junio C Hamano wrote: > > * es/worktree-forced-ops-fix (2018-08-30) 9 commits > This description mischaracterizes what these changes are about. [...] > > So, perhaps rewrite this description like this: > > Fix a bug in which the same path could be registered under > multiple worktree entries if the patch was missing (for instance, > was removed manually). Bah, typo: s/patch/path/ > Also, as a convenience, expand the number of cases in which > --force is applicable.
Re: What's cooking in git.git (Sep 2018, #01; Tue, 4)
On Wed, Sep 05, 2018 at 06:48:06PM +0200, Duy Nguyen wrote: > On Wed, Sep 5, 2018 at 6:35 PM Junio C Hamano wrote: > > > > Junio C Hamano writes: > > > > > Here are the topics that have been cooking. Commits prefixed with > > > '-' are only in 'pu' (proposed updates) while commits prefixed with > > > '+' are in 'next'. The ones marked with '.' do not appear in any of > > > the integration branches, but I am still holding onto them. > > > > > > Git 2.19-rc2 is out. Hopefully the tip of 'master' is more or less > > > identical to the final one without needing much updates. > > > > By the way, linux-gcc job of TravisCI seems to have been unhappy > > lately all the way down to 'master'. It fails split-index tests, > > which may or may not be new breakage. > > > > https://travis-ci.org/git/git/jobs/424552273 > > > > If this is a recent regression, we may want to revert a few commits, > > but I do not offhand recall us having touched the spilt-index part > > of the code during this cycle. It's not a regression, it's as old as the split index feature itself. > I can't reproduce it here (with either 64 or 32 bit builds on x86). > Not denying the problem, just a quick update. I'll need more time, > maybe over weekend to have a closer look. I have the patches almost ready, only the commit messages need some touching up.
Re: What's cooking in git.git (Sep 2018, #01; Tue, 4)
Eric Sunshine writes: > This description mischaracterizes what these changes are about. Thanks for a replacement text; very much appreciated.
Re: What's cooking in git.git (Sep 2018, #01; Tue, 4)
On Wed, Sep 5, 2018 at 6:35 PM Junio C Hamano wrote: > > Junio C Hamano writes: > > > Here are the topics that have been cooking. Commits prefixed with > > '-' are only in 'pu' (proposed updates) while commits prefixed with > > '+' are in 'next'. The ones marked with '.' do not appear in any of > > the integration branches, but I am still holding onto them. > > > > Git 2.19-rc2 is out. Hopefully the tip of 'master' is more or less > > identical to the final one without needing much updates. > > By the way, linux-gcc job of TravisCI seems to have been unhappy > lately all the way down to 'master'. It fails split-index tests, > which may or may not be new breakage. > > https://travis-ci.org/git/git/jobs/424552273 > > If this is a recent regression, we may want to revert a few commits, > but I do not offhand recall us having touched the spilt-index part > of the code during this cycle. I can't reproduce it here (with either 64 or 32 bit builds on x86). Not denying the problem, just a quick update. I'll need more time, maybe over weekend to have a closer look. -- Duy
Re: What's cooking in git.git (Sep 2018, #01; Tue, 4)
Eric Sunshine writes: > On Wed, Sep 5, 2018 at 4:29 AM Ævar Arnfjörð Bjarmason > wrote: >> I recently gained access to a Solaris 10 SPARC (5.10) box and discovered >> that the chainlint.sed implementation in 2.19.0 has more sed portability >> issues. >> >> First, whoever implemented the /bin/sed on Solaris apparently read the >> POSIX requirements for a max length label of 8 to mean that 8 characters >> should include the colon, so a bunch of things fail because of that, but >> are fixed with a shorter 7 character label. > > I'm pretty sure that Solaris 'sed' predates POSIX by a good bit, but > that's neither here nor there. I thought that we long time ago declared that it is a lost cause to use Solaris with only tools in /bin and /usr/bin and instead depend on /usr/xpg[46]/bin; did something happen recently to make us reconsider the position? If not, let's not waste time worrying about /bin/sed over there.
Re: jc/rebase-in-c-9-fixes, was Re: What's cooking in git.git (Sep 2018, #01; Tue, 4)
Johannes Schindelin writes: > On Tue, 4 Sep 2018, Junio C Hamano wrote: > >> * jc/rebase-in-c-9-fixes (2018-09-04) 1 commit >> - rebase: re-add forgotten -k that stands for --keep-empty >> (this branch uses ag/rebase-i-in-c, >> js/rebase-in-c-5.5-work-with-rebase-i-in-c, pk/rebase-in-c, >> pk/rebase-in-c-2-basic, pk/rebase-in-c-3-acts, pk/rebase-in-c-4-opts, >> pk/rebase-in-c-5-test and pk/rebase-in-c-6-final.) > > Quite frankly, I'd rather you pick up my two v2 series. I promised to take > over from Pratik, who is busy with exams, and I did. I even announced as > much in that mail where I pointed out the missing short option originally. Yup, I'm planning to replace the large pile of patches with the new ones you'd send once the final is done. The topic above is just a stop-gap measure to keep the tip of 'pu' testable, to be dropped when the series it fixes is replaced with a corrected one.
jc/rebase-in-c-9-fixes, was Re: What's cooking in git.git (Sep 2018, #01; Tue, 4)
Hi Junio, On Tue, 4 Sep 2018, Junio C Hamano wrote: > * jc/rebase-in-c-9-fixes (2018-09-04) 1 commit > - rebase: re-add forgotten -k that stands for --keep-empty > (this branch uses ag/rebase-i-in-c, > js/rebase-in-c-5.5-work-with-rebase-i-in-c, pk/rebase-in-c, > pk/rebase-in-c-2-basic, pk/rebase-in-c-3-acts, pk/rebase-in-c-4-opts, > pk/rebase-in-c-5-test and pk/rebase-in-c-6-final.) Quite frankly, I'd rather you pick up my two v2 series. I promised to take over from Pratik, who is busy with exams, and I did. I even announced as much in that mail where I pointed out the missing short option originally. So there is no need to do add-on patches, as if the `rebase-in-c` patch series had already been accepted into `next`. Ciao, Dscho
Re: What's cooking in git.git (Sep 2018, #01; Tue, 4)
On Wed, Sep 05 2018, Eric Sunshine wrote:
> On Wed, Sep 5, 2018 at 4:29 AM Ævar Arnfjörð Bjarmason
> wrote:
>> I recently gained access to a Solaris 10 SPARC (5.10) box and discovered
>> that the chainlint.sed implementation in 2.19.0 has more sed portability
>> issues.
>>
>> First, whoever implemented the /bin/sed on Solaris apparently read the
>> POSIX requirements for a max length label of 8 to mean that 8 characters
>> should include the colon, so a bunch of things fail because of that, but
>> are fixed with a shorter 7 character label.
>
> I'm pretty sure that Solaris 'sed' predates POSIX by a good bit, but
> that's neither here nor there.
>
>> Then GIT_TEST_CHAIN_LINT=1 still fails because 878f988350 ("t/test-lib:
>> teach --chain-lint to detect broken &&-chains in subshells", 2018-07-11)
>> added a "grep -q" invocation. The /bin/grep on that version of Solaris
>> doesn't have -q.
>
> I knew that '-q' was potentially problematic on some platforms, so I
> checked and saw that existing tests were already using it, thus went
> ahead and used it. Dropping '-q' here and redirecting stderr to
> /dev/null is a perfectly fine alternative.
>
>> We fixed a similar issue way back in 80700fde91
>> ("t/t1304: make a second colon optional in the mask ACL check",
>> 2010-03-15) by redirecting to /dev/null instead.
>>
>> A bunch of other tests in the test suite rely on "grep -q", but nothing
>> as central as chainlint, so it makes everything break. Do we want to
>> away with "grep -q" entirely because of old Solaris /bin/grep?
>
> I count 132 instances in existing tests (though, I may have missed some).
>
>> At this point those familiar with Solaris are screaming ("why are you
>> using anything in /bin!"). Okey fine, but it mostly worked before, so
>> are we OK with breaking it? "Mostly" here is "test suite would fail
>> 20-30 tests for various reasons, but at least no failures in test-lib.sh
>> and the like".
>>
>> However, if as config.mak.uname does we run the tests with
>> PATH=/usr/xpg6/bin:/usr/xpg4/bin:$PATH, at this point sed is fine with 8
>> character labels [...]
>
> So, if you run the tests via 'make test' (or whatever), then you get
> /usr/xpg?/bin in PATH, but if you run an individual test script (and
> haven't set your PATH appropriately), then you encounter the problems
> you report above?
You need to manually set the PATH before running the tests, the
SANE_TOOL_PATH just affects git-sh-setup. We should probably fix that,
i.e. now if you compile git our shellscripts will use the fixed paths,
but not the tests.
>> [...] but starts complaining about this (also in
>> chainlint.sed):
>>
>> sed: Too many commands, last: s/\n//
>
> Erm. Any additional context to help narrow this down?
I tried playing around with it a bit, but honestly don't understand
enough of this advanced sed syntax to make any headway, it's complaining
about e.g. the "check for multi-line double-quoted string" rule, but
then if you remove the s/\n// from there it complains about the next
rule in that format.
If you want to dig into this yourself I think the best way forward is
the last two paragraphs of
https://public-inbox.org/git/20180824152016.20286-1-ava...@gmail.com/
>> diff --git a/config.mak.uname b/config.mak.uname
>> @@ -163,6 +163,10 @@ ifeq ($(uname_S),SunOS)
>> BASIC_CFLAGS += -D__EXTENSIONS__ -D__sun__
>> + # t/chainlint.sed is hopelessly broken all known (tested
>> + # Solaris 10 & 11) versions of Solaris, both /bin/sed and
>> + # /usr/xpg4/bin/sed
>> + GIT_TEST_CHAIN_LINT = 0
>> endif
>>
>> It slightly sucks to not have chainlint on
>> Solaris, but it would also suck to revert chainlint.sed back to 2.18.0
>> (there were some big improvements). So I think the patch above is the
>> best way forward, especially since we're on rc2. What do you think?
>
> Keeping in mind that the main goal of 'chainlint' is to prevent _new_
> breakage from entering the test suite, disabling 'chainlint' on
> Solaris is an entirely reasonable way forward. In present day, it
> seems quite unlikely that we'll see someone develop new tests on
> Solaris, so having 'chainlint' disabled there isn't likely to be a big
> deal. Moreover, if someone does write a new test on Solaris which has
> a broken &&-chain in a subshell, that breakage will be caught very
> quickly once the test is run by anyone on Linux or MacOS (or Windows
> or BSD or AIX), so it's not like the broken test will make it into the
> project undetected.
Since writing my initial message I see that the CSW package packaging
git on Solaris just uses GNU userland:
https://sourceforge.net/p/gar/code/HEAD/tree/csw/mgar/pkg/git/trunk/Makefile
I.e. it puts /opt/csw/gnu in its $PATH, so this whole thing is probably
a non-issue. I.e. if the near-as-you-can-get "official" package just
compiles git against GNU tooling, perhaps we should stop caring about
Solaris-native tooling.
That does also mean that something
Re: What's cooking in git.git (Sep 2018, #01; Tue, 4)
On Tue, Sep 4, 2018 at 6:36 PM Junio C Hamano wrote: > * es/worktree-forced-ops-fix (2018-08-30) 9 commits > - worktree: delete .git/worktrees if empty after 'remove' > - worktree: teach 'remove' to override lock when --force given twice > - worktree: teach 'move' to override lock when --force given twice > - worktree: teach 'add' to respect --force for registered but missing path > - worktree: disallow adding same path multiple times > - worktree: prepare for more checks of whether path can become worktree > - worktree: generalize delete_git_dir() to reduce code duplication > - worktree: move delete_git_dir() earlier in file for upcoming new callers > - worktree: don't die() in library function find_worktree() > > Various subcommands of "git worktree" take '--force' but did not > behave sensibly, which has been corrected. This series is missing a patch[1] from Peff which he wanted placed at the end of the series. It was probably overlooked since he embedded it as a reply in that thread rather than sending it as a standalone patch. [1]: https://public-inbox.org/git/20180830075431.gf11...@sigill.intra.peff.net/
Re: What's cooking in git.git (Sep 2018, #01; Tue, 4)
On Tue, Sep 4, 2018 at 6:36 PM Junio C Hamano wrote: > * es/worktree-forced-ops-fix (2018-08-30) 9 commits > - worktree: delete .git/worktrees if empty after 'remove' > - worktree: teach 'remove' to override lock when --force given twice > - worktree: teach 'move' to override lock when --force given twice > - worktree: teach 'add' to respect --force for registered but missing path > - worktree: disallow adding same path multiple times > - worktree: prepare for more checks of whether path can become worktree > - worktree: generalize delete_git_dir() to reduce code duplication > - worktree: move delete_git_dir() earlier in file for upcoming new callers > - worktree: don't die() in library function find_worktree() > > Various subcommands of "git worktree" take '--force' but did not > behave sensibly, which has been corrected. This description mischaracterizes what these changes are about. The primary intent of this series is to fix a bug in which the same path can be registered under multiple worktree entries. As for --force, it worked perfectly fine for the couple git-worktree subcommands which accepted it. The patches in this series dealing with --force are merely extending it to other subcommands or to other use-cases. So, perhaps rewrite this description like this: Fix a bug in which the same path could be registered under multiple worktree entries if the patch was missing (for instance, was removed manually). Also, as a convenience, expand the number of cases in which --force is applicable.
Re: What's cooking in git.git (Sep 2018, #01; Tue, 4)
On Wed, Sep 5, 2018 at 4:29 AM Ævar Arnfjörð Bjarmason wrote:
> I recently gained access to a Solaris 10 SPARC (5.10) box and discovered
> that the chainlint.sed implementation in 2.19.0 has more sed portability
> issues.
>
> First, whoever implemented the /bin/sed on Solaris apparently read the
> POSIX requirements for a max length label of 8 to mean that 8 characters
> should include the colon, so a bunch of things fail because of that, but
> are fixed with a shorter 7 character label.
I'm pretty sure that Solaris 'sed' predates POSIX by a good bit, but
that's neither here nor there.
> Then GIT_TEST_CHAIN_LINT=1 still fails because 878f988350 ("t/test-lib:
> teach --chain-lint to detect broken &&-chains in subshells", 2018-07-11)
> added a "grep -q" invocation. The /bin/grep on that version of Solaris
> doesn't have -q.
I knew that '-q' was potentially problematic on some platforms, so I
checked and saw that existing tests were already using it, thus went
ahead and used it. Dropping '-q' here and redirecting stderr to
/dev/null is a perfectly fine alternative.
> We fixed a similar issue way back in 80700fde91
> ("t/t1304: make a second colon optional in the mask ACL check",
> 2010-03-15) by redirecting to /dev/null instead.
>
> A bunch of other tests in the test suite rely on "grep -q", but nothing
> as central as chainlint, so it makes everything break. Do we want to
> away with "grep -q" entirely because of old Solaris /bin/grep?
I count 132 instances in existing tests (though, I may have missed some).
> At this point those familiar with Solaris are screaming ("why are you
> using anything in /bin!"). Okey fine, but it mostly worked before, so
> are we OK with breaking it? "Mostly" here is "test suite would fail
> 20-30 tests for various reasons, but at least no failures in test-lib.sh
> and the like".
>
> However, if as config.mak.uname does we run the tests with
> PATH=/usr/xpg6/bin:/usr/xpg4/bin:$PATH, at this point sed is fine with 8
> character labels [...]
So, if you run the tests via 'make test' (or whatever), then you get
/usr/xpg?/bin in PATH, but if you run an individual test script (and
haven't set your PATH appropriately), then you encounter the problems
you report above?
> [...] but starts complaining about this (also in
> chainlint.sed):
>
> sed: Too many commands, last: s/\n//
Erm. Any additional context to help narrow this down?
> diff --git a/config.mak.uname b/config.mak.uname
> @@ -163,6 +163,10 @@ ifeq ($(uname_S),SunOS)
> BASIC_CFLAGS += -D__EXTENSIONS__ -D__sun__
> + # t/chainlint.sed is hopelessly broken all known (tested
> + # Solaris 10 & 11) versions of Solaris, both /bin/sed and
> + # /usr/xpg4/bin/sed
> + GIT_TEST_CHAIN_LINT = 0
> endif
>
> It slightly sucks to not have chainlint on
> Solaris, but it would also suck to revert chainlint.sed back to 2.18.0
> (there were some big improvements). So I think the patch above is the
> best way forward, especially since we're on rc2. What do you think?
Keeping in mind that the main goal of 'chainlint' is to prevent _new_
breakage from entering the test suite, disabling 'chainlint' on
Solaris is an entirely reasonable way forward. In present day, it
seems quite unlikely that we'll see someone develop new tests on
Solaris, so having 'chainlint' disabled there isn't likely to be a big
deal. Moreover, if someone does write a new test on Solaris which has
a broken &&-chain in a subshell, that breakage will be caught very
quickly once the test is run by anyone on Linux or MacOS (or Windows
or BSD or AIX), so it's not like the broken test will make it into the
project undetected.
Re: What's cooking in git.git (Sep 2018, #01; Tue, 4)
On Tue, Sep 04 2018, Junio C Hamano wrote:
> Git 2.19-rc2 is out. Hopefully the tip of 'master' is more or less
> identical to the final one without needing much updates.
>
> You can find the changes described here in the integration branches
> of the repositories listed at
>
> http://git-blame.blogspot.com/p/git-public-repositories.html
>
> --
> [Graduated to "master"]
>
> * ab/portable (2018-08-27) 6 commits
> (merged to 'next' on 2018-08-27 at 37640e66ef)
> + tests: fix and add lint for non-portable grep --file
> + tests: fix version-specific portability issue in Perl JSON
> + tests: use shorter labels in chainlint.sed for AIX sed
> + tests: fix comment syntax in chainlint.sed for AIX sed
> + tests: fix and add lint for non-portable seq
> + tests: fix and add lint for non-portable head -c N
> (this branch is used by ab/portable-more.)
I recently gained access to a Solaris 10 SPARC (5.10) box and discovered
that the chainlint.sed implementation in 2.19.0 has more sed portability
issues.
First, whoever implemented the /bin/sed on Solaris apparently read the
POSIX requirements for a max length label of 8 to mean that 8 characters
should include the colon, so a bunch of things fail because of that, but
are fixed with a shorter 7 character label.
Then GIT_TEST_CHAIN_LINT=1 still fails because 878f988350 ("t/test-lib:
teach --chain-lint to detect broken &&-chains in subshells", 2018-07-11)
added a "grep -q" invocation. The /bin/grep on that version of Solaris
doesn't have -q. We fixed a similar issue way back in 80700fde91
("t/t1304: make a second colon optional in the mask ACL check",
2010-03-15) by redirecting to /dev/null instead.
A bunch of other tests in the test suite rely on "grep -q", but nothing
as central as chainlint, so it makes everything break. Do we want to
away with "grep -q" entirely because of old Solaris /bin/grep?
At this point those familiar with Solaris are screaming ("why are you
using anything in /bin!"). Okey fine, but it mostly worked before, so
are we OK with breaking it? "Mostly" here is "test suite would fail
20-30 tests for various reasons, but at least no failures in test-lib.sh
and the like".
However, if as config.mak.uname does we run the tests with
PATH=/usr/xpg6/bin:/usr/xpg4/bin:$PATH, at this point sed is fine with 8
character labels, but starts complaining about this (also in
chainlint.sed):
sed: Too many commands, last: s/\n//
As with other sed issues I fixed recently in chainlint.sed this one is
just the tip of the iceberg. Once you "fix" one (just remove it, I have
no idea how to rewrite it) others appear.
I was hoping this would just be a Solaris 10 issue, but it's also an
issue in Solaris 11 (5.11 11.3).
So, do we want to chase this down or just do this?:
diff --git a/Makefile b/Makefile
index 5a969f5..f125dc5 100644
--- a/Makefile
+++ b/Makefile
@@ -2602,6 +2602,9 @@ endif
ifdef TEST_GIT_INDEX_VERSION
@echo TEST_GIT_INDEX_VERSION=\''$(subst ','\'',$(subst
','\'',$(TEST_GIT_INDEX_VERSION)))'\' >>$@+
endif
+ifdef GIT_TEST_CHAIN_LINT
+ @echo GIT_TEST_CHAIN_LINT=\''$(subst ','\'',$(subst
','\'',$(GIT_TEST_CHAIN_LINT)))'\' >>$@+
+endif
@if cmp $@+ $@ >/dev/null 2>&1; then $(RM) $@+; else mv $@+ $@; fi
### Detect Python interpreter path changes
diff --git a/config.mak.uname b/config.mak.uname
index e47af72..2b02a2b 100644
--- a/config.mak.uname
+++ b/config.mak.uname
@@ -163,6 +163,10 @@ ifeq ($(uname_S),SunOS)
INSTALL = /usr/ucb/install
TAR = gtar
BASIC_CFLAGS += -D__EXTENSIONS__ -D__sun__
+ # t/chainlint.sed is hopelessly broken all known (tested
+ # Solaris 10 & 11) versions of Solaris, both /bin/sed and
+ # /usr/xpg4/bin/sed
+ GIT_TEST_CHAIN_LINT = 0
endif
ifeq ($(uname_O),Cygwin)
ifeq ($(shell expr "$(uname_R)" : '1\.[1-6]\.'),4)
If I could wave a magic wand I'd say "let's just rewrite chainlint.sed
in perl, at least that's portable", but that's a lot of effort (and I
doubt Eric wants to). It slightly sucks to not have chainlint on
Solaris, but it would also suck to revert chainlint.sed back to 2.18.0
(there were some big improvements). So I think the patch above is the
best way forward, especially since we're on rc2. What do you think?
What's cooking in git.git (Sep 2018, #01; Tue, 4)
Here are the topics that have been cooking. Commits prefixed with
'-' are only in 'pu' (proposed updates) while commits prefixed with
'+' are in 'next'. The ones marked with '.' do not appear in any of
the integration branches, but I am still holding onto them.
Git 2.19-rc2 is out. Hopefully the tip of 'master' is more or less
identical to the final one without needing much updates.
You can find the changes described here in the integration branches
of the repositories listed at
http://git-blame.blogspot.com/p/git-public-repositories.html
--
[Graduated to "master"]
* ab/portable (2018-08-27) 6 commits
(merged to 'next' on 2018-08-27 at 37640e66ef)
+ tests: fix and add lint for non-portable grep --file
+ tests: fix version-specific portability issue in Perl JSON
+ tests: use shorter labels in chainlint.sed for AIX sed
+ tests: fix comment syntax in chainlint.sed for AIX sed
+ tests: fix and add lint for non-portable seq
+ tests: fix and add lint for non-portable head -c N
(this branch is used by ab/portable-more.)
Portability fix.
* ab/portable-more (2018-08-29) 2 commits
(merged to 'next' on 2018-08-31 at d7b44993e4)
+ tests: fix non-portable iconv invocation
+ tests: fix non-portable "${var:-"str"}" construct
(this branch uses ab/portable.)
Portability fix.
* ds/commit-graph-lockfile-fix (2018-08-30) 1 commit
(merged to 'next' on 2018-09-04 at 0876e6ddcc)
+ commit: don't use generation numbers if not needed
"git merge-base" in 2.19-rc1 has performance regression when the
(experimental) commit-graph feature is in use, which has been
mitigated.
* en/directory-renames-nothanks (2018-08-30) 3 commits
(merged to 'next' on 2018-08-31 at 91d663d688)
+ am: avoid directory rename detection when calling recursive merge machinery
+ merge-recursive: add ability to turn off directory rename detection
+ t3401: add another directory rename testcase for rebase and am
Recent addition of "directory rename" heuristics to the
merge-recursive backend makes the command susceptible to false
positives and false negatives. In the context of "git am -3",
which does not know about surrounding unmodified paths and thus
cannot inform the merge machinery about the full trees involved,
this risk is particularly severe. As such, the heuristic is
disabled for "git am -3" to keep the machinery "more stupid but
predictable".
* es/chain-lint-more (2018-08-29) 1 commit
(merged to 'next' on 2018-08-31 at d456090b62)
+ chainlint: match "quoted" here-doc tags
The test linter code has learned that the end of here-doc mark
"EOF" can be quoted in a double-quote pair, not just in a
single-quote pair.
* es/freebsd-iconv-portability (2018-08-31) 1 commit
(merged to 'next' on 2018-09-04 at 52baa37dd7)
+ config.mak.uname: resolve FreeBSD iconv-related compilation warning
Build fix.
* pw/rebase-i-author-script-fix (2018-08-07) 2 commits
(merged to 'next' on 2018-08-31 at 7b9f485407)
+ sequencer: fix quoting in write_author_script
+ sequencer: handle errors from read_author_ident()
Recent "git rebase -i" update started to write bogusly formatted
author-script, with a matching broken reading code. These are
fixed.
--
[New Topics]
* ab/fetch-tags-noclobber (2018-08-31) 9 commits
- fetch: stop clobbering existing tags without --force
- fetch: document local ref updates with/without --force
- push doc: correct lies about how push refspecs work
- push doc: move mention of "tag " later in the prose
- push doc: remove confusing mention of remote merger
- fetch tests: add a test for clobbering tag behavior
- push tests: use spaces in interpolated string
- push tests: make use of unused $1 in test description
- fetch: change "branch" to "reference" in --force -h output
The rules used by "git push" and "git fetch" to determine if a ref
can or cannot be updated were inconsistent; specifically, fetching
to update existing tags were allowed even though tags are supposed
to be unmoving anchoring points. "git fetch" was taught to forbid
updates to existing tags without the "--force" option.
Will merge to and cook in 'next'.
This is a backward incompatible change but in a good way; it may
still need to be treated carefully.
* es/worktree-forced-ops-fix (2018-08-30) 9 commits
- worktree: delete .git/worktrees if empty after 'remove'
- worktree: teach 'remove' to override lock when --force given twice
- worktree: teach 'move' to override lock when --force given twice
- worktree: teach 'add' to respect --force for registered but missing path
- worktree: disallow adding same path multiple times
- worktree: prepare for more checks of whether path can become worktree
- worktree: generalize delete_git_dir() to reduce code duplication
- worktree: move delete_git_dir() earlier in file for upcoming new callers
- worktree: don't die() in library function
