Skip to content

Conversation

@dscho
Copy link
Member

@dscho dscho commented Dec 11, 2018

The code added in 26c7d06 (help -a: improve and make --verbose default, 2018-09-29) that intends to print out aliases in addition to commands failed to adjust for the length of the aliases. As a consequence, if there was any alias whose name is longer than 18 characters, git help -a tried to print an insanely large number of spaces, one at a time, causing what appeared to be a "hang".

Let's fix this, and while at it fix a style issue that I saw on the way as well.

Original report at git-for-windows#1975

We want a space after the `while` keyword.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
We take pains to determine the longest command beforehand, so that we
can align the category column after printing the command names.

However, then we re-use that value when printing the aliases. If any
alias name is longer than the longest command name, we consequently try
to add a negative number of spaces (but `mput_char()` does not expect
any negative values and simply decrements until the value is 0, i.e.
it tries to add close to 2**31 spaces).

Let's fix this by adjusting the `longest` variable before printing the
aliases.

This fixes git-for-windows#1975.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
@dscho
Copy link
Member Author

dscho commented Dec 11, 2018

/submit

@gitgitgadget
Copy link

gitgitgadget bot commented Dec 11, 2018

Submitted as pull.97.git.gitgitgadget@gmail.com

@gitgitgadget
Copy link

gitgitgadget bot commented Dec 15, 2018

This branch is now known as js/help-commands-verbose-by-default-fix.

@gitgitgadget
Copy link

gitgitgadget bot commented Dec 15, 2018

This patch series was integrated into pu via git@916f56d.

@gitgitgadget
Copy link

gitgitgadget bot commented Dec 15, 2018

This patch series was integrated into next via git@916f56d.

@gitgitgadget
Copy link

gitgitgadget bot commented Dec 15, 2018

This patch series was integrated into master via git@916f56d.

@gitgitgadget
Copy link

gitgitgadget bot commented Dec 15, 2018

This patch series was integrated into maint via git@916f56d.

@gitgitgadget
Copy link

gitgitgadget bot commented Dec 15, 2018

Closed via 916f56d.

@gitgitgadget gitgitgadget bot closed this Dec 15, 2018
@dscho dscho deleted the fix-help-a branch December 15, 2018 21:52
gitgitgadget bot pushed a commit that referenced this pull request Aug 16, 2024
Import the clar unit testing framework at commit 1516124 (Merge pull
request #97 from pks-t/pks-whitespace-fixes, 2024-08-15). The framework
will be wired up in subsequent commits.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
gitgitgadget bot pushed a commit that referenced this pull request Aug 20, 2024
Import the clar unit testing framework at commit 1516124 (Merge pull
request #97 from pks-t/pks-whitespace-fixes, 2024-08-15). The framework
will be wired up in subsequent commits.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
gitgitgadget bot pushed a commit that referenced this pull request Sep 3, 2024
Our unit testing framework is a homegrown solution. While it supports
most of our needs, it is likely that the volume of unit tests will grow
quite a bit in the future such that we can exercise low-level subsystems
directly. This surfaces several shortcomings that the current solution
has:

  - There is no way to run only one specific tests. While some of our
    unit tests wire this up manually, others don't. In general, it
    requires quite a bit of boilerplate to get this set up correctly.

  - Failures do not cause a test to stop execution directly. Instead,
    the test author needs to return manually whenever an assertion
    fails. This is rather verbose and is not done correctly in most of
    our unit tests.

  - Wiring up a new testcase requires both implementing the test
    function and calling it in the respective test suite's main
    function, which is creating code duplication.

We can of course fix all of these issues ourselves, but that feels
rather pointless when there are already so many unit testing frameworks
out there that have those features.

We line out some requirements for any unit testing framework in
"Documentation/technical/unit-tests.txt". The "clar" unit testing
framework, which isn't listed in that table yet, ticks many of the
boxes:

  - It is licensed under ISC, which is compatible.

  - It is easily vendorable because it is rather tiny at around 1200
    lines of code.

  - It is easily hackable due to the same reason.

  - It has TAP support.

  - It has skippable tests.

  - It preprocesses test files in order to extract test functions, which
    then get wired up automatically.

While it's not perfect, the fact that clar originates from the libgit2
project means that it should be rather easy for us to collaborate with
upstream to plug any gaps.

Import the clar unit testing framework at commit 1516124 (Merge pull
request #97 from pks-t/pks-whitespace-fixes, 2024-08-15). The framework
will be wired up in subsequent commits.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
gitgitgadget bot pushed a commit that referenced this pull request Sep 4, 2024
Our unit testing framework is a homegrown solution. While it supports
most of our needs, it is likely that the volume of unit tests will grow
quite a bit in the future such that we can exercise low-level subsystems
directly. This surfaces several shortcomings that the current solution
has:

  - There is no way to run only one specific tests. While some of our
    unit tests wire this up manually, others don't. In general, it
    requires quite a bit of boilerplate to get this set up correctly.

  - Failures do not cause a test to stop execution directly. Instead,
    the test author needs to return manually whenever an assertion
    fails. This is rather verbose and is not done correctly in most of
    our unit tests.

  - Wiring up a new testcase requires both implementing the test
    function and calling it in the respective test suite's main
    function, which is creating code duplication.

We can of course fix all of these issues ourselves, but that feels
rather pointless when there are already so many unit testing frameworks
out there that have those features.

We line out some requirements for any unit testing framework in
"Documentation/technical/unit-tests.txt". The "clar" unit testing
framework, which isn't listed in that table yet, ticks many of the
boxes:

  - It is licensed under ISC, which is compatible.

  - It is easily vendorable because it is rather tiny at around 1200
    lines of code.

  - It is easily hackable due to the same reason.

  - It has TAP support.

  - It has skippable tests.

  - It preprocesses test files in order to extract test functions, which
    then get wired up automatically.

While it's not perfect, the fact that clar originates from the libgit2
project means that it should be rather easy for us to collaborate with
upstream to plug any gaps.

Import the clar unit testing framework at commit 1516124 (Merge pull
request #97 from pks-t/pks-whitespace-fixes, 2024-08-15). The framework
will be wired up in subsequent commits.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
gitgitgadget bot pushed a commit that referenced this pull request Oct 14, 2024
Update clar from:

    - 1516124 (Merge pull request #97 from pks-t/pks-whitespace-fixes, 2024-08-15).

To:

    - 0810a36 (Merge pull request #107 from pks-t/pks-sunos-compatibility, 2024-10-14)

This update includes a bunch of fixes and improvements that we have
discussed in Git when initial support for clar was merged:

  - There is a ".editorconfig" file now.

  - Compatibility with Windows has been improved so that the clar
    compiles on this platform without an issue. This has been tested
    with Cygwin, MinGW and Microsoft Visual Studio.

  - clar now uses CMake. This does not impact us at all as we wire up
    the clar into our own build infrastructure anyway. This conversion
    was done such that we can easily run CI jobs against Windows.

  - Allocation failures are now checked for consistently.

  - We now define feature test macros in "clar.c", which fixes
    compilation on some platforms that didn't previously pull in
    non-standard functions like lstat(3p) or strdup(3p). This was
    reported by a user of OpenSUSE Leap.

  - We stop using `struct timezone`, which is undefined behaviour
    nowadays and results in a compilation error on some platforms.

  - We now use the combination of mktemp(3) and mkdir(3) on SunOS, same
    as we do on NonStop.

The most important bits here are the improved platform compatibility
with Windows, OpenSUSE and SunOS.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Taylor Blau <me@ttaylorr.com>
gitgitgadget bot pushed a commit that referenced this pull request Oct 21, 2024
Update clar from:

    - 1516124 (Merge pull request #97 from pks-t/pks-whitespace-fixes, 2024-08-15).

To:

    - 206accb (Merge pull request #108 from pks-t/pks-uclibc-without-wchar, 2024-10-21)

This update includes a bunch of fixes and improvements that we have
discussed in Git when initial support for clar was merged:

  - There is a ".editorconfig" file now.

  - Compatibility with Windows has been improved so that the clar
    compiles on this platform without an issue. This has been tested
    with Cygwin, MinGW and Microsoft Visual Studio.

  - clar now uses CMake. This does not impact us at all as we wire up
    the clar into our own build infrastructure anyway. This conversion
    was done such that we can easily run CI jobs against Windows.

  - Allocation failures are now checked for consistently.

  - We now define feature test macros in "clar.c", which fixes
    compilation on some platforms that didn't previously pull in
    non-standard functions like lstat(3p) or strdup(3p). This was
    reported by a user of OpenSUSE Leap.

  - We stop using `struct timezone`, which is undefined behaviour
    nowadays and results in a compilation error on some platforms.

  - We now use the combination of mktemp(3) and mkdir(3) on SunOS, same
    as we do on NonStop.

  - We now support uClibc without support for <wchar.h>.

The most important bits here are the improved platform compatibility
with Windows, OpenSUSE, SunOS and uClibc.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Taylor Blau <me@ttaylorr.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant