* Re: [PATCH 6/6] coverity: detect and report when the token or project is incorrect
From: Jeff King @ 2023-09-23 7:07 UTC (permalink / raw)
To: Johannes Schindelin via GitGitGadget; +Cc: git, Johannes Schindelin
In-Reply-To: <458bc2ea91faf88a3e1d21945f12f314d1a7b78e.1695379323.git.gitgitgadget@gmail.com>
On Fri, Sep 22, 2023 at 10:42:03AM +0000, Johannes Schindelin via GitGitGadget wrote:
> From: Johannes Schindelin <johannes.schindelin@gmx.de>
>
> When trying to obtain the MD5 of the Coverity Scan Tool (in order to
> decide whether a cached version can be used or a new version has to be
> downloaded), it is possible to get a 401 (Authorization required) due to
> either an incorrect token, or even more likely due to an incorrect
> Coverity project name.
>
> Let's detect that scenario and provide a helpful error message instead
> of trying to go forward with an empty string instead of the correct MD5.
Ah. :) I think using "curl --fail" is probably a simpler solution here.
-Peff
^ permalink raw reply
* Re: [PATCH 5/6] coverity: allow running on macOS
From: Jeff King @ 2023-09-23 7:06 UTC (permalink / raw)
To: Johannes Schindelin via GitGitGadget; +Cc: git, Johannes Schindelin
In-Reply-To: <782cf2b440313fcad0492ee352fa66d1a3534ac4.1695379323.git.gitgitgadget@gmail.com>
On Fri, Sep 22, 2023 at 10:42:02AM +0000, Johannes Schindelin via GitGitGadget wrote:
> From: Johannes Schindelin <johannes.schindelin@gmx.de>
>
> For completeness' sake, let's add support for submitting macOS builds to
> Coverity Scan.
I don't have any real problem with this, and it will check a few extra
bits of platform-specific code not covered elsewhere. My big question
would be: how much extra does this cost to run each time?
I guess it is not too much (compared to regular CI); my coverity build
job ran in 7 minutes, and that is including the download of the tool.
-Peff
^ permalink raw reply
* Re: [PATCH 4/6] coverity: support building on Windows
From: Jeff King @ 2023-09-23 7:03 UTC (permalink / raw)
To: Johannes Schindelin via GitGitGadget; +Cc: git, Johannes Schindelin
In-Reply-To: <14cdefff08244f9b5e2ee1fa2a78cfd3e58c14d2.1695379323.git.gitgitgadget@gmail.com>
On Fri, Sep 22, 2023 at 10:42:01AM +0000, Johannes Schindelin via GitGitGadget wrote:
> From: Johannes Schindelin <johannes.schindelin@gmx.de>
>
> By adding the repository variable `ENABLE_COVERITY_SCAN_ON_OS` with a
> value, say, `["windows-latest"]`, this GitHub workflow now runs on
> Windows, allowing to analyze Windows-specific issues.
Makes sense. I figured we'd key this on COVERITY_PLATFORM itself, but I
guess we need to map Actions environments to Coverity platform names,
so starting with the Actions names makes sense.
> +# The workflow runs on `ubuntu-latest` by default. This can be overridden by setting
> +# the repository variable `ENABLE_COVERITY_SCAN_ON_OS` to a JSON string array specifying
> +# the operating systems, e.g. `["ubuntu-latest", "windows-latest"]`.
OK. I was envisioning that we'd just run on one platform, and maybe
git-for-windows would run on another. But it does not hurt to be able to
do both from one repo. I'm not sure how Coverity presents that, but it
should be able to figure out based on "version" and "platform" fields
that they are two builds of the same version (and not, say, one
overriding the other as the "latest").
-Peff
^ permalink raw reply
* Re: [PATCH 3/6] coverity: allow overriding the Coverity project
From: Jeff King @ 2023-09-23 7:00 UTC (permalink / raw)
To: Johannes Schindelin via GitGitGadget; +Cc: git, Johannes Schindelin
In-Reply-To: <6c1c82862814f40a408231cb249fb4b653276b52.1695379323.git.gitgitgadget@gmail.com>
On Fri, Sep 22, 2023 at 10:42:00AM +0000, Johannes Schindelin via GitGitGadget wrote:
> +# By default, the builds are submitted to the Coverity project `git`. To override this,
> +# set the repository variable `COVERITY_PROJECT`.
This may not matter all that much, because I don't expect most people to
set this up for their forks (and if we have git/git results that I have
access to, I will probably even stop building my peff/git one). But I
just wondered if a better default would be the GitHub project name
(i.e., $user/git).
It has been long enough that I do not remember all of the setup on the
Coverity side, but I assumed there was some "set it up for this GitHub
project" button. But maybe I just picked that name myself.
-Peff
^ permalink raw reply
* Re: [PATCH 2/6] coverity: cache the Coverity Build Tool
From: Jeff King @ 2023-09-23 6:58 UTC (permalink / raw)
To: Johannes Schindelin via GitGitGadget; +Cc: git, Johannes Schindelin
In-Reply-To: <8420a76eba3eba3afdc7747af6d609ad8dbd8cb6.1695379323.git.gitgitgadget@gmail.com>
On Fri, Sep 22, 2023 at 10:41:59AM +0000, Johannes Schindelin via GitGitGadget wrote:
> From: Johannes Schindelin <johannes.schindelin@gmx.de>
>
> It would add a 1GB+ download for every run, better cache it.
>
> This is inspired by the GitHub Action `vapier/coverity-scan-action`,
> however, it uses the finer-grained `restore`/`save` method to be able to
> cache the Coverity Build Tool even if an unrelated step in the GitHub
> workflow fails later on.
Nice. This is the big thing that I think the vapier action was providing
us, and it does not look too bad. I have never used actions/cache
before, but it all looks plausibly correct to me (and I assume you did a
few test-runs to check it).
One note:
> + # The Coverity site says the tool is usually updated twice yearly, so the
> + # MD5 of download can be used to determine whether there's been an update.
> + - name: get the Coverity Build Tool hash
> + id: lookup
> + run: |
> + MD5=$(curl https://scan.coverity.com/download/$COVERITY_LANGUAGE/$COVERITY_PLATFORM \
> + --data "token=${{ secrets.COVERITY_SCAN_TOKEN }}&project=$COVERITY_PROJECT&md5=1")
> + echo "hash=$MD5" >>$GITHUB_OUTPUT
We probably want --fail here, too (and presumably &&-chaining) so that
we don't accidentally write a bogus cache entry. Possibly even check
that $MD5 isn't blank if we want to be double-paranoid.
That made me wonder: if we do end up with a bogus cache entry, how does
one clear it? And it looks like it can be managed directly via
https://github.com/$user/$project/actions/caches. Nice.
-Peff
^ permalink raw reply
* Re: [PATCH 1/6] ci: add a GitHub workflow to submit Coverity scans
From: Jeff King @ 2023-09-23 6:49 UTC (permalink / raw)
To: Johannes Schindelin via GitGitGadget; +Cc: git, Johannes Schindelin
In-Reply-To: <8cb92968c5ebd38f328ed325ddf7f2e531dc9190.1695379323.git.gitgitgadget@gmail.com>
On Fri, Sep 22, 2023 at 10:41:58AM +0000, Johannes Schindelin via GitGitGadget wrote:
> Note: The initial version of this patch used
> `vapier/coverity-scan-action` to benefit from that Action's caching of
> the Coverity tool, which is rather large. Sadly, that Action only
> supports Linux, and we want to have the option of building on Windows,
> too. Besides, in the meantime Coverity requires `cov-configure` to be
> runantime, and that Action was not adjusted accordingly, i.e. it seems
> not to be maintained actively. Therefore it would seem prudent to
> implement the steps manually instead of using that Action.
I'm still unsure of the cov-configure thing, as I have never needed it
(and the "vapier" Action worked fine for me). But the lack of Windows
support is obviously a deal-breaker. I wondered if it might be worth
trying to submit a PR to that project to fix it for everybody, but I saw
that you commented on their "Windows support" issue, which has been
sitting unanswered since it was opened in May. It's possible they might
be more responsive to an actual PR, but I agree that it may be simpler
to just go our own way here.
> +jobs:
> + coverity:
> + if: contains(fromJSON(vars.ENABLE_COVERITY_SCAN_FOR_BRANCHES || '[""]'), github.ref_name)
> + runs-on: ubuntu-latest
> + env:
> + COVERITY_PROJECT: git
> + COVERITY_LANGUAGE: cxx
> + COVERITY_PLATFORM: linux64
Ah, now I see why you were bothered by using "git/git" at the project
name earlier. That is what I assumed we would use (and certainly I use
"peff/git" on the Coverity side), but I forgot that we already have the
general "git" name on the Coverity side (which isn't to say we couldn't
switch to using the "git/git" name, but I am happy for us to be just
"git" there).
> + steps:
> + - uses: actions/checkout@v3
> + - run: ci/install-dependencies.sh
> + env:
> + runs_on_pool: ubuntu-latest
> +
> + - name: download the Coverity Build Tool (${{ env.COVERITY_LANGUAGE }} / ${{ env.COVERITY_PLATFORM}})
> + run: |
> + curl https://scan.coverity.com/download/$COVERITY_LANGUAGE/$COVERITY_PLATFORM \
> + --no-progress-meter \
> + --output $RUNNER_TEMP/cov-analysis.tgz \
> + --data "token=${{ secrets.COVERITY_SCAN_TOKEN }}&project=$COVERITY_PROJECT"
You might want "--fail" or "--fail-with-body" here. I think any
server-side errors (like a missing or invalid token or project name)
will result in a 401. Having curl reported that as a non-zero exit
should stop the Action with a failure, rather than silently continuing.
This is mostly a style suggestion, but I think you can use:
--form token="${{ secrets.COVERITY_SCAN_TOKEN }}" \
--form project="$COVERITY_PROJECT"
here, which IMHO is a little more readable than "data". It probably
doesn't matter in practice, but I think it would also would handle any
encoding for us (though note that if we wanted to be really careful, the
TOKEN secret would need shell quoting).
Using --form will use multipart/form-data instead of
application/x-www-form-url-encoded, but coverity seems happy with
either.
> + - name: extract the Coverity Build Tool
> + run: |
> + mkdir $RUNNER_TEMP/cov-analysis &&
> + tar -xzf $RUNNER_TEMP/cov-analysis.tgz --strip 1 -C $RUNNER_TEMP/cov-analysis
OK, we are starting without Windows support yet. :)
> + - name: build with cov-build
> + run: |
> + export PATH="$RUNNER_TEMP/cov-analysis/bin:$PATH" &&
> + cov-configure --gcc &&
> + cov-build --dir cov-int make -j$(nproc)
> + - name: package the build
> + run: tar -czvf cov-int.tgz cov-int
OK, this looks a lot like what my custom rule does (no surprise, since
we are all adapting Coverity's instructions).
> + - name: submit the build to Coverity Scan
> + run: |
> + curl \
> + --form token="${{ secrets.COVERITY_SCAN_TOKEN }}" \
> + --form email="${{ secrets.COVERITY_SCAN_EMAIL }}" \
> + --form file=@cov-int.tgz \
> + --form version="${{ github.sha }}" \
> + "https://scan.coverity.com/builds?project=$COVERITY_PROJECT"
Likewise. I add:
--form description="$(./git version)"
to mine, but I am not even sure where that ends up (the "version" is
probably the most interesting bit, as that is shown on the Coverity
project page).
I notice you put the "project" variable in the query string. Can it be
a --form, too, for symmetry? (In mine, I seem to have it as _both_,
which is probably just a mistake). Not a huge deal either way, but just
a small readability thing.
As with above, we'd probably want --fail or --fail-with-body here to
detect errors (since otherwise a failed upload goes completely
unreported).
-Peff
^ permalink raw reply
* Re: [PATCH v2] test-lib: fix GIT_TEST_SANITIZE_LEAK_LOG
From: Jeff King @ 2023-09-23 6:24 UTC (permalink / raw)
To: Rubén Justo
Cc: Git List, Ævar Arnfjörð Bjarmason, Eric Sunshine,
Junio C Hamano
In-Reply-To: <54253e98-10d5-55ef-a3ac-1f1a8cfcdec9@gmail.com>
On Fri, Sep 22, 2023 at 10:38:06PM +0200, Rubén Justo wrote:
> [...]
> Let's add the missing conditions in the if-elses chain to make it work
> as expected.
Thanks, I think this explains the situation better than the original.
> + elif test "$test_failure" = 0
> + then
> say "With GIT_TEST_SANITIZE_LEAK_LOG=true our logs revealed a memory leak, exit non-zero!" &&
> invert_exit_code=t
> + else
> + say "With GIT_TEST_SANITIZE_LEAK_LOG=true our logs revealed a memory leak..."
> fi
OK, so you did add in the "else" here. :) I am obviously fine with that,
and the patch overall looks good to me.
-Peff
^ permalink raw reply
* Re: [PATCH v2] .github/workflows: add coverity action
From: Jeff King @ 2023-09-23 6:21 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: Taylor Blau, git, Junio C Hamano
In-Reply-To: <d29b5fd3-3b44-89ca-62b4-cf93b23031fd@gmx.de>
On Fri, Sep 22, 2023 at 01:09:26PM +0200, Johannes Schindelin wrote:
> While it is true that that Action caches the Coverity Tool, this GitHub
> Action comes with a few non-niceties, too, one of them being that it
> actually fails to do its job. Looking at
> https://github.com/vapier/coverity-scan-action/blob/main/action.yml it can
> be very easily verified that `cov-configure` is not called. But as
> https://github.com/git-for-windows/git/actions/runs/6267593979/job/17020975444#step:9:12
> demonstrates, this command now _needs_ to be called:
Hmm. I have been running Coverity for years without running
cov-configure (using my own workflow), and I just tested Taylor's patch,
which worked fine. I wonder what is different about our setups; maybe it
is only an issue on the Windows version of Coverity?
Or maybe it has to do with the project setup on Coverity's server side,
since we have to provide the project name as part of the tool download
(though I don't know if that is purely for logging/auditing on their
side or if the delivered download is customized in any way).
> Another issue I ran into: That Action defaults to using the URL-encoded
> name of the repository (in our case, `git%2Fgit`) as Coverity project
> name. That project does not exist, though, and the GitHub Action therefore
> silently assumes an empty MD5 and attempts to extract a tar file that is
> actually a text file instead, with the contents "Not found".
It took me a minute to understand what you meant here, since the main
use of the project name is for uploading our tarball. But we also must
provide it (along with the token) just to download their scan software,
so that step failed.
And yes, I agree that the error checking in the Action could be better
(I also mentioned elsewhere in the thread that it did not detect or
report a failed upload). I'm not sure if it is easier to submit a patch
there, or if we are better off just doing it all inline (it is not that
much code either way).
> Further, when I tried to specify `win64` as this GitHub Action's
> `platform` input variable (whose existence suggests that platforms other
> than `linux64` are allowed), it totally fell over, trying to untar a
> `.zip` file.
Hmm, yes, I hadn't really considered running Coverity with different
build configurations. But I guess it needs to run code through the
compiler to do its magic, so you'd want to be able to run it on a
Windows build to hit any platform-specific code.
For general git/git use, I'm not sure if we'd want to run it on multiple
platforms, but I guess your plan would be to share the workflow code
between git/ and git-for-windows/, and use a repo variable to select the
platform for the latter. And for that case we can't rely on an Action
which doesn't support Windows.
I'll take a look at the patches you posted.
-Peff
^ permalink raw reply
* Re: [PATCH v2] .github/workflows: add coverity action
From: Jeff King @ 2023-09-23 6:10 UTC (permalink / raw)
To: Taylor Blau; +Cc: git, Junio C Hamano
In-Reply-To: <b23951c569660e1891a7fb3ad2c2ea1952897bd7.1695332105.git.me@ttaylorr.com>
On Thu, Sep 21, 2023 at 05:53:31PM -0400, Taylor Blau wrote:
> diff --git a/.github/workflows/coverity.yml b/.github/workflows/coverity.yml
> new file mode 100644
> index 0000000000..3ba00b3929
> --- /dev/null
> +++ b/.github/workflows/coverity.yml
> @@ -0,0 +1,22 @@
> +name: Coverity
> +
> +on: [push, pull_request]
> +
> +jobs:
> + coverity:
> + if: (vars.ENABLE_COVERITY == 'true') &&
> + (vars.COVERITY_BRANCHES == '' ||
> + contains(vars.COVERITY_BRANCHES, github.ref_name) ||
> + contains(vars.COVERITY_BRANCHES, '*'))
I wonder if we really need this ENABLE_COVERITY flag. It's not _too_
hard to set up, but it feels like we can eliminate a setup step and just
infer it from other variables. Either:
1. Treat unset COVERITY_BRANCHES as "do not run". Unlike the CI job,
there's not much useful signal in running this for every PR. A
human has to go periodically look at the coverity output and make
sense of it. So I think it's something that would get run
periodically (say, on updates of "next") just for one or two
branches.
2. Use COVERITY_SCAN_EMAIL as a clue that the feature it is enabled. I
don't think we can do that as-is because it's a secret, not a var.
But is there a reason for the EMAIL to be a secret? I don't think
repository vars are fully public; they're just not hidden as deeply
as secrets. It seems like the right level of privacy for an email.
> + runs-on: ubuntu-latest
> + steps:
> + - uses: actions/checkout@v3
> + - run: ci/install-dependencies.sh
> + env:
> + jobname: coverity
> + - uses: vapier/coverity-scan-action@cae3c096a2eb21c431961a49375ac17aea2670ce
> + with:
> + email: ${{ secrets.COVERITY_SCAN_EMAIL }}
> + token: ${{ secrets.COVERITY_SCAN_TOKEN }}
> + command: make -j8
I ran the action and it worked out of the box for me (I have everything
set up on the coverity side already from my custom workflow), modulo one
hiccup. I initially had COVERITY_SCAN_EMAIL as a var, so the upload
failed, complaining of an empty email.
But much to my surprise, the Action still succeeded. It didn't record
the HTTP result code, so I'm not sure if we could detect this with "curl
--fail-with-body" or if we'd have to scrape the output. It may not be
that big a deal either way, though, since the coverity output really is
useless until a human periodically scans over it (but it would be nice
to know if it was routinely failing; I might not notice if it didn't).
-Peff
^ permalink raw reply
* [PATCH v2] diff --stat: set the width defaults in a helper function
From: Dragan Simic @ 2023-09-23 4:01 UTC (permalink / raw)
To: git
Extract the commonly used initialization of the --stat-width=<width>,
--stat-name-width=<width> and --stat-graph-with=<width> parameters to their
internal default values into a helper function, to avoid repeating the same
initialization code in a few places.
Add a couple of tests to additionally cover existing configuration options
diff.statNameWidth=<width> and diff.statGraphWidth=<width> when used by
git-merge to generate --stat outputs. This closes the gap that existed
previously in the --stat tests, and reduces the chances for having any
regressions introduced by this commit.
While there, perform a small bunch of minor wording tweaks in the improved
unit test, to improve its test-level consistency a bit.
Signed-off-by: Dragan Simic <dsimic@manjaro.org>
---
builtin/diff.c | 4 +--
builtin/log.c | 6 ++---
builtin/merge.c | 4 +--
builtin/rebase.c | 4 +--
diff.c | 7 +++++
diff.h | 1 +
t/t4052-stat-output.sh | 60 ++++++++++++++++++++++++++----------------
7 files changed, 51 insertions(+), 35 deletions(-)
diff --git a/builtin/diff.c b/builtin/diff.c
index c0f564273a..55e7d21755 100644
--- a/builtin/diff.c
+++ b/builtin/diff.c
@@ -474,9 +474,7 @@ int cmd_diff(int argc, const char **argv, const char *prefix)
repo_init_revisions(the_repository, &rev, prefix);
/* Set up defaults that will apply to both no-index and regular diffs. */
- rev.diffopt.stat_width = -1;
- rev.diffopt.stat_name_width = -1;
- rev.diffopt.stat_graph_width = -1;
+ init_diffstat_widths(&rev.diffopt);
rev.diffopt.flags.allow_external = 1;
rev.diffopt.flags.allow_textconv = 1;
diff --git a/builtin/log.c b/builtin/log.c
index 80e1be1645..ba775d7b5c 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -176,17 +176,15 @@ static void cmd_log_init_defaults(struct rev_info *rev)
if (default_follow)
rev->diffopt.flags.default_follow_renames = 1;
rev->verbose_header = 1;
+ init_diffstat_widths(&rev->diffopt);
rev->diffopt.flags.recursive = 1;
- rev->diffopt.stat_width = -1; /* use full terminal width */
- rev->diffopt.stat_name_width = -1; /* respect statNameWidth config */
- rev->diffopt.stat_graph_width = -1; /* respect statGraphWidth config */
+ rev->diffopt.flags.allow_textconv = 1;
rev->abbrev_commit = default_abbrev_commit;
rev->show_root_diff = default_show_root;
rev->subject_prefix = fmt_patch_subject_prefix;
rev->patch_name_max = fmt_patch_name_max;
rev->show_signature = default_show_signature;
rev->encode_email_headers = default_encode_email_headers;
- rev->diffopt.flags.allow_textconv = 1;
if (default_date_mode)
parse_date_format(default_date_mode, &rev->date_mode);
diff --git a/builtin/merge.c b/builtin/merge.c
index fd21c0d4f4..8f397cbeff 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -466,9 +466,7 @@ static void finish(struct commit *head_commit,
if (new_head && show_diffstat) {
struct diff_options opts;
repo_diff_setup(the_repository, &opts);
- opts.stat_width = -1; /* use full terminal width */
- opts.stat_name_width = -1; /* respect statNameWidth config */
- opts.stat_graph_width = -1; /* respect statGraphWidth config */
+ init_diffstat_widths(&opts);
opts.output_format |=
DIFF_FORMAT_SUMMARY | DIFF_FORMAT_DIFFSTAT;
opts.detect_rename = DIFF_DETECT_RENAME;
diff --git a/builtin/rebase.c b/builtin/rebase.c
index b93dca95a6..4783f90ac5 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -1806,9 +1806,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
/* We want color (if set), but no pager */
repo_diff_setup(the_repository, &opts);
- opts.stat_width = -1; /* use full terminal width */
- opts.stat_name_width = -1; /* respect statNameWidth config */
- opts.stat_graph_width = -1; /* respect statGraphWidth config */
+ init_diffstat_widths(&opts);
opts.output_format |=
DIFF_FORMAT_SUMMARY | DIFF_FORMAT_DIFFSTAT;
opts.detect_rename = DIFF_DETECT_RENAME;
diff --git a/diff.c b/diff.c
index 353e3b2cc9..2c602df10a 100644
--- a/diff.c
+++ b/diff.c
@@ -6936,6 +6936,13 @@ void diff_queued_diff_prefetch(void *repository)
oid_array_clear(&to_fetch);
}
+void init_diffstat_widths(struct diff_options *options)
+{
+ options->stat_width = -1; /* use full terminal width */
+ options->stat_name_width = -1; /* respect diff.statNameWidth config */
+ options->stat_graph_width = -1; /* respect diff.statGraphWidth config */
+}
+
void diffcore_std(struct diff_options *options)
{
int output_formats_to_prefetch = DIFF_FORMAT_DIFFSTAT |
diff --git a/diff.h b/diff.h
index caf1528bf0..66bd8aeb29 100644
--- a/diff.h
+++ b/diff.h
@@ -573,6 +573,7 @@ int git_config_rename(const char *var, const char *value);
#define DIFF_PICKAXE_IGNORE_CASE 32
+void init_diffstat_widths(struct diff_options *);
void diffcore_std(struct diff_options *);
void diffcore_fix_diff_index(void);
diff --git a/t/t4052-stat-output.sh b/t/t4052-stat-output.sh
index beb2ec2a55..7badd72488 100755
--- a/t/t4052-stat-output.sh
+++ b/t/t4052-stat-output.sh
@@ -12,7 +12,7 @@ TEST_PASSES_SANITIZE_LEAK=true
. ./test-lib.sh
. "$TEST_DIRECTORY"/lib-terminal.sh
-# 120 character name
+# 120-character name
name=aaaaaaaaaa
name=$name$name$name$name$name$name$name$name$name$name$name$name
test_expect_success 'preparation' '
@@ -58,15 +58,15 @@ while read verb expect cmd args
do
# No width limit applied when statNameWidth is ignored
case "$expect" in expect72|expect.6030)
- test_expect_success "$cmd $verb statNameWidth config with long name" '
+ test_expect_success "$cmd $verb diff.statNameWidth with long name" '
git -c diff.statNameWidth=30 $cmd $args >output &&
grep " | " output >actual &&
test_cmp $expect actual
';;
esac
# Maximum width limit still applied when statNameWidth is ignored
case "$expect" in expect.60|expect.6030)
- test_expect_success "$cmd --stat=width $verb statNameWidth config with long name" '
+ test_expect_success "$cmd --stat=width $verb diff.statNameWidth with long name" '
git -c diff.statNameWidth=30 $cmd $args --stat=60 >output &&
grep " | " output >actual &&
test_cmp $expect actual
@@ -111,19 +111,19 @@ do
test_cmp $expect.6030 actual
'
- test_expect_success "$cmd --stat-name-width with long name" '
+ test_expect_success "$cmd --stat-name-width=width with long name" '
git $cmd $args --stat-name-width=30 >output &&
grep " | " output >actual &&
test_cmp $expect.6030 actual
'
done <<\EOF
expect2 format-patch --cover-letter -1 --stdout
expect diff HEAD^ HEAD --stat
expect show --stat
expect log -1 --stat
EOF
-test_expect_success 'preparation for big change tests' '
+test_expect_success 'preparation for big-change tests' '
>abcd &&
git add abcd &&
git commit -m message &&
@@ -139,7 +139,7 @@ cat >expect72 <<'EOF'
abcd | 1000 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
abcd | 1000 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
EOF
-test_expect_success "format-patch --cover-letter ignores COLUMNS (big change)" '
+test_expect_success "format-patch --cover-letter ignores COLUMNS with big change" '
COLUMNS=200 git format-patch -1 --stdout --cover-letter >output &&
grep " | " output >actual &&
test_cmp expect72 actual
@@ -159,15 +159,15 @@ cat >expect200-graph <<'EOF'
EOF
while read verb expect cmd args
do
- test_expect_success "$cmd $verb COLUMNS (big change)" '
+ test_expect_success "$cmd $verb COLUMNS with big change" '
COLUMNS=200 git $cmd $args >output &&
grep " | " output >actual &&
test_cmp "$expect" actual
'
case "$cmd" in diff|show) continue;; esac
- test_expect_success "$cmd --graph $verb COLUMNS (big change)" '
+ test_expect_success "$cmd --graph $verb COLUMNS with big change" '
COLUMNS=200 git $cmd $args --graph >output &&
grep " | " output >actual &&
test_cmp "$expect-graph" actual
@@ -187,15 +187,15 @@ cat >expect40-graph <<'EOF'
EOF
while read verb expect cmd args
do
- test_expect_success "$cmd $verb not enough COLUMNS (big change)" '
+ test_expect_success "$cmd $verb not enough COLUMNS with big change" '
COLUMNS=40 git $cmd $args >output &&
grep " | " output >actual &&
test_cmp "$expect" actual
'
case "$cmd" in diff|show) continue;; esac
- test_expect_success "$cmd --graph $verb not enough COLUMNS (big change)" '
+ test_expect_success "$cmd --graph $verb not enough COLUMNS with big change" '
COLUMNS=40 git $cmd $args --graph >output &&
grep " | " output >actual &&
test_cmp "$expect-graph" actual
@@ -215,15 +215,15 @@ cat >expect40-graph <<'EOF'
EOF
while read verb expect cmd args
do
- test_expect_success "$cmd $verb statGraphWidth config" '
+ test_expect_success "$cmd $verb diff.statGraphWidth" '
git -c diff.statGraphWidth=26 $cmd $args >output &&
grep " | " output >actual &&
test_cmp "$expect" actual
'
case "$cmd" in diff|show) continue;; esac
- test_expect_success "$cmd --graph $verb statGraphWidth config" '
+ test_expect_success "$cmd --graph $verb diff.statGraphWidth" '
git -c diff.statGraphWidth=26 $cmd $args --graph >output &&
grep " | " output >actual &&
test_cmp "$expect-graph" actual
@@ -255,33 +255,33 @@ do
test_cmp expect actual
'
- test_expect_success "$cmd --stat-graph-width with big change" '
+ test_expect_success "$cmd --stat-graph-width=width with big change" '
git $cmd $args --stat-graph-width=26 >output &&
grep " | " output >actual &&
test_cmp expect actual
'
case "$cmd" in diff|show) continue;; esac
test_expect_success "$cmd --stat-width=width --graph with big change" '
git $cmd $args --stat-width=40 --graph >output &&
grep " | " output >actual &&
test_cmp expect-graph actual
'
- test_expect_success "$cmd --stat-graph-width --graph with big change" '
+ test_expect_success "$cmd --stat-graph-width=width --graph with big change" '
git $cmd $args --stat-graph-width=26 --graph >output &&
grep " | " output >actual &&
test_cmp expect-graph actual
'
done <<\EOF
format-patch -1 --stdout
diff HEAD^ HEAD --stat
show --stat
log -1 --stat
EOF
-test_expect_success 'preparation for long filename tests' '
+test_expect_success 'preparation for long-name tests' '
cp abcd aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa &&
git add aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa &&
git commit -m message
@@ -329,15 +329,15 @@ cat >expect200-graph <<'EOF'
EOF
while read verb expect cmd args
do
- test_expect_success "$cmd $verb COLUMNS (long filename)" '
+ test_expect_success "$cmd $verb COLUMNS with long name" '
COLUMNS=200 git $cmd $args >output &&
grep " | " output >actual &&
test_cmp "$expect" actual
'
case "$cmd" in diff|show) continue;; esac
- test_expect_success "$cmd --graph $verb COLUMNS (long filename)" '
+ test_expect_success "$cmd --graph $verb COLUMNS with long name" '
COLUMNS=200 git $cmd $args --graph >output &&
grep " | " output >actual &&
test_cmp "$expect-graph" actual
@@ -358,41 +358,57 @@ EOF
while read verb expect cmd args
do
test_expect_success COLUMNS_CAN_BE_1 \
- "$cmd $verb prefix greater than COLUMNS (big change)" '
+ "$cmd $verb prefix greater than COLUMNS with big change" '
COLUMNS=1 git $cmd $args >output &&
grep " | " output >actual &&
test_cmp "$expect" actual
'
case "$cmd" in diff|show) continue;; esac
test_expect_success COLUMNS_CAN_BE_1 \
- "$cmd --graph $verb prefix greater than COLUMNS (big change)" '
+ "$cmd --graph $verb prefix greater than COLUMNS with big change" '
COLUMNS=1 git $cmd $args --graph >output &&
grep " | " output >actual &&
test_cmp "$expect-graph" actual
'
done <<\EOF
ignores expect72 format-patch -1 --stdout
respects expect1 diff HEAD^ HEAD --stat
respects expect1 show --stat
respects expect1 log -1 --stat
EOF
cat >expect <<'EOF'
abcd | 1000 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
EOF
-test_expect_success 'merge --stat respects COLUMNS (big change)' '
- git checkout -b branch HEAD^^ &&
+test_expect_success 'merge --stat respects diff.statGraphWidth with big change' '
+ git checkout -b branch1 HEAD^^ &&
+ git -c diff.statGraphWidth=26 merge --stat --no-ff main^ >output &&
+ grep " | " output >actual &&
+ test_cmp expect40 actual
+'
+test_expect_success 'merge --stat respects COLUMNS with big change' '
+ git checkout -b branch2 HEAD^^ &&
COLUMNS=100 git merge --stat --no-ff main^ >output &&
grep " | " output >actual &&
test_cmp expect actual
'
cat >expect <<'EOF'
aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa | 1000 +++++++++++++++++++++++++++++++++++++++
EOF
-test_expect_success 'merge --stat respects COLUMNS (long filename)' '
+cat >expect.30 <<'EOF'
+ ...aaaaaaaaaaaaaaaaaaaaaaaaaaa | 1000 ++++++++++++++++++++++++++++++++++++++++
+EOF
+test_expect_success 'merge --stat respects diff.statNameWidth with long name' '
+ git switch branch1 &&
+ git -c diff.statNameWidth=30 merge --stat --no-ff main >output &&
+ grep " | " output >actual &&
+ test_cmp expect.30 actual
+'
+test_expect_success 'merge --stat respects COLUMNS with long name' '
+ git switch branch2 &&
COLUMNS=100 git merge --stat --no-ff main >output &&
grep " | " output >actual &&
test_cmp expect actual
^ permalink raw reply related
* What's cooking in git.git (Sep 2023, #07; Fri, 22)
From: Junio C Hamano @ 2023-09-23 2:50 UTC (permalink / raw)
To: git
Here are the topics that have been cooking in my tree. Commits
prefixed with '+' are in 'next' (being in 'next' is a sign that a
topic is stable enough to be used and are candidate to be in a
future release). Commits prefixed with '-' are only in 'seen', and
aren't considered "accepted" at all and may be annotated with an URL
to a message that raises issues but they are no means exhaustive. A
topic without enough support may be discarded after a long period of
no activity (of course they can be resubmit when new interests
arise).
Copies of the source code to Git live in many repositories, and the
following is a list of the ones I push into or their mirrors. Some
repositories have only a subset of branches.
With maint, master, next, seen, todo:
git://git.kernel.org/pub/scm/git/git.git/
git://repo.or.cz/alt-git.git/
https://kernel.googlesource.com/pub/scm/git/git/
https://github.com/git/git/
https://gitlab.com/git-vcs/git/
With all the integration branches and topics broken out:
https://github.com/gitster/git/
Even though the preformatted documentation in HTML and man format
are not sources, they are published in these repositories for
convenience (replace "htmldocs" with "manpages" for the manual
pages):
git://git.kernel.org/pub/scm/git/git-htmldocs.git/
https://github.com/gitster/git-htmldocs.git/
Release tarballs are available at:
https://www.kernel.org/pub/software/scm/git/
--------------------------------------------------
[Graduated to 'master']
* ch/clean-docfix (2023-09-15) 1 commit
(merged to 'next' on 2023-09-19 at 1e16b715bb)
+ git-clean doc: fix "without do cleaning" typo
Typofix.
source: <pull.1572.git.git.1694800409471.gitgitgadget@gmail.com>
* eg/config-type-path-docfix (2023-09-15) 1 commit
(merged to 'next' on 2023-09-19 at 2c8ad3e731)
+ git-config: fix misworded --type=path explanation
Typofix.
source: <20230915202610.21206-1-evan.gates@gmail.com>
* jc/update-index-show-index-version (2023-09-12) 3 commits
(merged to 'next' on 2023-09-13 at b754554df8)
+ test-tool: retire "index-version"
+ update-index: add --show-index-version
+ update-index doc: v4 is OK with JGit and libgit2
"git update-index" learns "--show-index-version" to inspect
the index format version used by the on-disk index file.
source: <20230912193235.776292-1-gitster@pobox.com>
* jk/ort-unused-parameter-cleanups (2023-09-16) 5 commits
(merged to 'next' on 2023-09-18 at 25a5dfc38e)
+ merge-ort: lowercase a few error messages
+ merge-ort: drop unused "opt" parameter from merge_check_renames_reusable()
+ merge-ort: drop unused parameters from detect_and_process_renames()
+ merge-ort: stop passing "opt" to read_oid_strbuf()
+ merge-ort: drop custom err() function
Code clean-up.
source: <20230914093409.GA2254811@coredump.intra.peff.net>
* jk/redact-h2h3-headers-fix (2023-09-15) 2 commits
(merged to 'next' on 2023-09-19 at d40278ef9e)
+ http: update curl http/2 info matching for curl 8.3.0
+ http: factor out matching of curl http/2 trace lines
HTTP Header redaction code has been adjusted for a newer version of
cURL library that shows its traces differently from earlier
versions.
source: <20230915113237.GA3531328@coredump.intra.peff.net>
* js/diff-cached-fsmonitor-fix (2023-09-11) 1 commit
(merged to 'next' on 2023-09-12 at 7479278da0)
+ diff-lib: fix check_removed when fsmonitor is on
(this branch is used by jc/diff-cached-fsmonitor-fix.)
"git diff --cached" codepath did not fill the necessary stat
information for a file when fsmonitor knows it is clean and ended
up behaving as if it is not clean, which has been corrected.
source: <20230911170901.49050-2-sokcevic@google.com>
* js/systemd-timers-wsl-fix (2023-09-11) 1 commit
(merged to 'next' on 2023-09-12 at 71c4cbb1df)
+ maintenance(systemd): support the Windows Subsystem for Linux
Update "git maintainance" timers' implementation based on systemd
timers to work with WSL.
source: <pull.1586.git.1694334620260.gitgitgadget@gmail.com>
* la/trailer-cleanups (2023-09-11) 6 commits
(merged to 'next' on 2023-09-12 at 779c4a097a)
+ trailer: split process_command_line_args into separate functions
+ trailer: split process_input_file into separate pieces
+ trailer: separate public from internal portion of trailer_iterator
Code clean-up.
Keep only the first three clean-ups, and discard the rest to be replaced later.
cf. <owly1qetjqo1.fsf@fine.c.googlers.com>
cf. <owlyzg1dsswr.fsf@fine.c.googlers.com>
source: <pull.1563.v2.git.1694240177.gitgitgadget@gmail.com>
* ob/sequencer-remove-dead-code (2023-09-12) 1 commit
(merged to 'next' on 2023-09-13 at 1f6c2b336b)
+ sequencer: remove unreachable exit condition in pick_commits()
Code clean-up.
source: <20230912105541.272917-1-oswald.buddenhagen@gmx.de>
* ob/t3404-typofix (2023-09-12) 1 commit
(merged to 'next' on 2023-09-13 at b17fa78c3f)
+ t3404-rebase-interactive.sh: fix typos in title of a rewording test
Code clean-up.
source: <20230912104237.271616-1-oswald.buddenhagen@gmx.de>
* pb/complete-commit-trailers (2023-09-12) 2 commits
(merged to 'next' on 2023-09-13 at 9a0ec17606)
+ completion: commit: complete trailers tokens more robustly
(merged to 'next' on 2023-09-08 at 842587016d)
+ completion: commit: complete configured trailer tokens
The command-line complation support (in contrib/) learned to
complete "git commit --trailer=" for possible trailer keys.
source: <pull.1583.v2.git.1694539827.gitgitgadget@gmail.com>
* pb/completion-aliases-doc (2023-09-12) 1 commit
(merged to 'next' on 2023-09-13 at b248a5bc26)
+ completion: improve doc for complex aliases
Clarify how "alias.foo = : git cmd ; aliased-command-string" should
be spelled with necessary whitespaces around punctuation marks to
work.
source: <pull.1585.v2.git.1694538135853.gitgitgadget@gmail.com>
* pw/diff-no-index-from-named-pipes (2023-09-11) 1 commit
(merged to 'next' on 2023-09-12 at 135ecd136f)
+ diff --no-index: fix -R with stdin
"git diff --no-index -R <(one) <(two)" did not work correctly,
which has been corrected.
source: <22fdfa3b-f90e-afcc-667c-705fb7670245@web.de>
* tb/repack-existing-packs-cleanup (2023-09-13) 8 commits
(merged to 'next' on 2023-09-14 at bb8065e89c)
+ builtin/repack.c: extract common cruft pack loop
+ builtin/repack.c: avoid directly inspecting "util"
+ builtin/repack.c: store existing cruft packs separately
+ builtin/repack.c: extract `has_existing_non_kept_packs()`
+ builtin/repack.c: extract redundant pack cleanup for existing packs
+ builtin/repack.c: extract redundant pack cleanup for --geometric
+ builtin/repack.c: extract marking packs for deletion
+ builtin/repack.c: extract structure to store existing packs
The code to keep track of existing packs in the repository while
repacking has been refactored.
source: <cover.1694632644.git.me@ttaylorr.com>
* tb/send-email-extract-valid-address-error-message-fix (2023-09-18) 1 commit
(merged to 'next' on 2023-09-19 at d037282391)
+ git-send-email.perl: avoid printing undef when validating addresses
An error message given by "git send-email" when given a malformed
address did not give correct information, which has been corrected.
source: <545729b619308c6f3397b9aa1747f26ddc58f461.1695054945.git.me@ttaylorr.com>
--------------------------------------------------
[New Topics]
* hy/doc-show-is-like-log-not-diff-tree (2023-09-20) 1 commit
(merged to 'next' on 2023-09-22 at 5492c03eae)
+ show doc: redirect user to git log manual instead of git diff-tree
Doc update.
Will merge to 'master'.
source: <20230920132731.1259-1-hanyang.tony@bytedance.com>
* jc/alias-completion (2023-09-20) 1 commit
(merged to 'next' on 2023-09-22 at 1d069e900b)
+ completion: loosen and document the requirement around completing alias
The command line completion script (in contrib/) can be told to
complete aliases by including ": git <cmd> ;" in the alias to tell
it that the alias should be completed similar to how "git <cmd>" is
completed. The parsing code for the alias as been loosened to
allow ';' without an extra space before it.
Will merge to 'master'.
cf. <owlyjzssjro2.fsf@fine.c.googlers.com>
source: <xmqqy1h08zsp.fsf_-_@gitster.g>
* jk/test-pass-ubsan-options-to-http-test (2023-09-21) 1 commit
(merged to 'next' on 2023-09-22 at bbe2f75937)
+ test-lib: set UBSAN_OPTIONS to match ASan
UBSAN options were not propagated through the test framework to git
run via the httpd, unlike ASAN options, which has been corrected.
Will merge to 'master'.
source: <20230921041825.GA2814583@coredump.intra.peff.net>
* ob/am-msgfix (2023-09-21) 1 commit
(merged to 'next' on 2023-09-22 at 7f7589a06a)
+ am: fix error message in parse_opt_show_current_patch()
The parameters to generate an error message have been corrected.
Will merge to 'master'.
source: <20230921110727.789156-1-oswald.buddenhagen@gmx.de>
* js/ci-coverity (2023-09-22) 6 commits
- coverity: detect and report when the token or project is incorrect
- coverity: allow running on macOS
- coverity: support building on Windows
- coverity: allow overriding the Coverity project
- coverity: cache the Coverity Build Tool
- ci: add a GitHub workflow to submit Coverity scans
GitHub CI workflow has learned to trigger Coverity check.
source: <pull.1588.git.1695379323.gitgitgadget@gmail.com>
* js/doc-status-with-submodules-mark-up-fix (2023-09-22) 1 commit
- Documentation/git-status: add missing line breaks
Docfix.
Will merge to 'next'.
source: <pull.1590.git.1695392082207.gitgitgadget@gmail.com>
--------------------------------------------------
[Stalled]
* tk/cherry-pick-sequence-requires-clean-worktree (2023-06-01) 1 commit
- cherry-pick: refuse cherry-pick sequence if index is dirty
"git cherry-pick A" that replays a single commit stopped before
clobbering local modification, but "git cherry-pick A..B" did not,
which has been corrected.
Expecting a reroll.
cf. <999f12b2-38d6-f446-e763-4985116ad37d@gmail.com>
source: <pull.1535.v2.git.1685264889088.gitgitgadget@gmail.com>
* jc/diff-cached-fsmonitor-fix (2023-09-15) 3 commits
- diff-lib: fix check_removed() when fsmonitor is active
- Merge branch 'jc/fake-lstat' into jc/diff-cached-fsmonitor-fix
- Merge branch 'js/diff-cached-fsmonitor-fix' into jc/diff-cached-fsmonitor-fix
(this branch uses jc/fake-lstat.)
The optimization based on fsmonitor in the "diff --cached"
codepath is resurrected with the "fake-lstat" introduced earlier.
It is unknown if the optimization is worth resurrecting, but in case...
source: <xmqqr0n0h0tw.fsf@gitster.g>
--------------------------------------------------
[Cooking]
* js/config-parse (2023-09-21) 5 commits
- config-parse: split library out of config.[c|h]
- config.c: accept config_parse_options in git_config_from_stdin
- config: report config parse errors using cb
- config: split do_event() into start and flush operations
- config: split out config_parse_options
The parsing routines for the configuration files have been split
into a separate file.
source: <cover.1695330852.git.steadmon@google.com>
* ml/git-gui-exec-path-fix (2023-09-18) 3 commits
(merged to 'next' on 2023-09-19 at 0565b0b14b)
+ Merge git-gui into ml/git-gui-exec-path-fix
+ git-gui - use git-hook, honor core.hooksPath
+ git-gui - re-enable use of hook scripts
Fix recent regression in Git-GUI that fails to run hook scripts at
all.
Will merge to 'master'.
* ds/stat-name-width-configuration (2023-09-18) 1 commit
(merged to 'next' on 2023-09-22 at dbf5bd96e8)
+ diff --stat: add config option to limit filename width
"git diff" learned diff.statNameWidth configuration variable, to
give the default width for the name part in the "--stat" output.
Will merge to 'master'.
source: <87badb12f040d1c66cd9b89074d3de5015a45983.1694446743.git.dsimic@manjaro.org>
* jk/fsmonitor-unused-parameter (2023-09-18) 8 commits
(merged to 'next' on 2023-09-19 at bd06505f9e)
+ run-command: mark unused parameters in start_bg_wait callbacks
+ fsmonitor: mark unused hashmap callback parameters
+ fsmonitor/darwin: mark unused parameters in system callback
+ fsmonitor: mark unused parameters in stub functions
+ fsmonitor/win32: mark unused parameter in fsm_os__incompatible()
+ fsmonitor: mark some maybe-unused parameters
+ fsmonitor/win32: drop unused parameters
+ fsmonitor: prefer repo_git_path() to git_pathdup()
Unused parameters in fsmonitor related code paths have been marked
as such.
Will merge to 'master'.
source: <20230918222908.GA2659096@coredump.intra.peff.net>
* jc/fake-lstat (2023-09-15) 1 commit
- cache: add fake_lstat()
(this branch is used by jc/diff-cached-fsmonitor-fix.)
A new helper to let us pretend that we called lstat() when we know
our cache_entry is up-to-date via fsmonitor.
Needs review.
source: <xmqqcyykig1l.fsf@gitster.g>
* kn/rev-list-ignore-missing-links (2023-09-20) 1 commit
- revision: add `--ignore-missing-links` user option
Surface the .ignore_missing_links bit that stops the revision
traversal from stopping and dying when encountering a missing
object to a new command line option of "git rev-list", so that the
objects that are required but are missing can be enumerated.
Waiting for review response.
source: <20230920104507.21664-1-karthik.188@gmail.com>
* kh/range-diff-notes (2023-09-19) 1 commit
(merged to 'next' on 2023-09-22 at ac04978b4b)
+ range-diff: treat notes like `log`
"git range-diff --notes=foo" compared "log --notes=foo --notes" of
the two ranges, instead of using just the specified notes tree.
Will merge to 'master'.
source: <6e114271a2e7d2323193bd58bb307f60101942ce.1695154855.git.code@khaugsbakk.name>
* rs/parse-options-value-int (2023-09-18) 2 commits
- parse-options: use and require int pointer for OPT_CMDMODE
- parse-options: add int value pointer to struct option
A bit of type safety for the "value" pointer used in the
parse-options API.
Comments?
source: <e6d8a291-03de-cfd3-3813-747fc2cad145@web.de>
* so/diff-merges-d (2023-09-11) 2 commits
- diff-merges: introduce '-d' option
- diff-merges: improve --diff-merges documentation
Teach a new "-d" option that shows the patch against the first
parent for merge commits (which is "--diff-merges=first-parent -p").
Letting a less useful combination of options squat on short-and-sweet "-d" feels dubious.
source: <20230909125446.142715-1-sorganov@gmail.com>
* cc/repack-sift-filtered-objects-to-separate-pack (2023-09-11) 9 commits
. gc: add `gc.repackFilterTo` config option
. repack: implement `--filter-to` for storing filtered out objects
. gc: add `gc.repackFilter` config option
. repack: add `--filter=<filter-spec>` option
. pack-bitmap-write: rebuild using new bitmap when remapping
. repack: refactor finding pack prefix
. repack: refactor finishing pack-objects command
. t/helper: add 'find-pack' test-tool
. pack-objects: allow `--filter` without `--stdout`
"git repack" machinery learns to pay attention to the "--filter="
option.
May need to wait until tb/repack-existing-packs-cleanup stablizes.
source: <20230911150618.129737-1-christian.couder@gmail.com>
* pw/rebase-sigint (2023-09-07) 1 commit
- rebase -i: ignore signals when forking subprocesses
If the commit log editor or other external programs (spawned via
"exec" insn in the todo list) receive internactive signal during
"git rebase -i", it caused not just the spawned program but the
"Git" process that spawned them, which is often not what the end
user intended. "git" learned to ignore SIGINT and SIGQUIT while
waiting for these subprocesses.
Expecting a reroll.
cf. <12c956ea-330d-4441-937f-7885ab519e26@gmail.com>
source: <pull.1581.git.1694080982621.gitgitgadget@gmail.com>
* cw/git-std-lib (2023-09-11) 7 commits
- SQUASH???
- git-std-lib: add test file to call git-std-lib.a functions
- git-std-lib: introduce git standard library
- parse: create new library for parsing strings and env values
- config: correct bad boolean env value error message
- wrapper: remove dependency to Git-specific internal file
- hex-ll: split out functionality from hex
Another libification effort.
Needs more work.
cf. <xmqqy1hfrk6p.fsf@gitster.g>
cf. <20230915183927.1597414-1-jonathantanmy@google.com>
source: <20230908174134.1026823-1-calvinwan@google.com>
* cc/git-replay (2023-09-07) 15 commits
- replay: stop assuming replayed branches do not diverge
- replay: add --contained to rebase contained branches
- replay: add --advance or 'cherry-pick' mode
- replay: disallow revision specific options and pathspecs
- replay: use standard revision ranges
- replay: make it a minimal server side command
- replay: remove HEAD related sanity check
- replay: remove progress and info output
- replay: add an important FIXME comment about gpg signing
- replay: don't simplify history
- replay: introduce pick_regular_commit()
- replay: die() instead of failing assert()
- replay: start using parse_options API
- replay: introduce new builtin
- t6429: remove switching aspects of fast-rebase
Waiting for review response.
cf. <52277471-4ddd-b2e0-62ca-c2a5b59ae418@gmx.de>
cf. <58daa706-7efb-51dd-9061-202ef650b96a@gmx.de>
cf. <f0e75d47-c277-9fbb-7bcd-53e4e5686f3c@gmx.de>
May want to wait until tb/repack-existing-packs-cleanup stabilizes.
source: <20230907092521.733746-1-christian.couder@gmail.com>
* la/trailer-test-and-doc-updates (2023-09-07) 13 commits
- trailer doc: <token> is a <key> or <keyAlias>, not both
- trailer doc: separator within key suppresses default separator
- trailer doc: emphasize the effect of configuration variables
- trailer --unfold help: prefer "reformat" over "join"
- trailer --parse docs: add explanation for its usefulness
- trailer --only-input: prefer "configuration variables" over "rules"
- trailer --parse help: expose aliased options
- trailer --no-divider help: describe usual "---" meaning
- trailer: trailer location is a place, not an action
- trailer doc: narrow down scope of --where and related flags
- trailer: add tests to check defaulting behavior with --no-* flags
- trailer test description: this tests --where=after, not --where=before
- trailer tests: make test cases self-contained
Test coverage for trailers has been improved.
source: <pull.1564.v3.git.1694125209.gitgitgadget@gmail.com>
* js/doc-unit-tests (2023-08-17) 3 commits
- ci: run unit tests in CI
- unit tests: add TAP unit test framework
- unit tests: Add a project plan document
(this branch is used by js/doc-unit-tests-with-cmake.)
Process to add some form of low-level unit tests has started.
Waiting for review response.
cf. <xmqq350hw6n7.fsf@gitster.g>
source: <cover.1692297001.git.steadmon@google.com>
* js/doc-unit-tests-with-cmake (2023-09-18) 7 commits
- cmake: handle also unit tests
- cmake: use test names instead of full paths
- cmake: fix typo in variable name
- artifacts-tar: when including `.dll` files, don't forget the unit-tests
- unit-tests: do show relative file paths
- unit-tests: do not mistake `.pdb` files for being executable
- cmake: also build unit tests
(this branch uses js/doc-unit-tests.)
Update the base topic to work with CMake builds.
Waiting for the base topic to settle.
source: <pull.1579.v2.git.1695070468.gitgitgadget@gmail.com>
* tb/path-filter-fix (2023-08-30) 15 commits
- bloom: introduce `deinit_bloom_filters()`
- commit-graph: reuse existing Bloom filters where possible
- object.h: fix mis-aligned flag bits table
- commit-graph: drop unnecessary `graph_read_bloom_data_context`
- commit-graph.c: unconditionally load Bloom filters
- t/t4216-log-bloom.sh: harden `test_bloom_filters_not_used()`
- bloom: prepare to discard incompatible Bloom filters
- bloom: annotate filters with hash version
- commit-graph: new filter ver. that fixes murmur3
- repo-settings: introduce commitgraph.changedPathsVersion
- t4216: test changed path filters with high bit paths
- t/helper/test-read-graph: implement `bloom-filters` mode
- bloom.h: make `load_bloom_filter_from_graph()` public
- t/helper/test-read-graph.c: extract `dump_graph_info()`
- gitformat-commit-graph: describe version 2 of BDAT
The Bloom filter used for path limited history traversal was broken
on systems whose "char" is unsigned; update the implementation and
bump the format version to 2.
Almost there.
cf. <20230830200218.GA5147@szeder.dev>
cf. <20230901205616.3572722-1-jonathantanmy@google.com>
source: <cover.1693413637.git.jonathantanmy@google.com>
* jc/rerere-cleanup (2023-08-25) 4 commits
- rerere: modernize use of empty strbuf
- rerere: try_merge() should use LL_MERGE_ERROR when it means an error
- rerere: fix comment on handle_file() helper
- rerere: simplify check_one_conflict() helper function
(this branch uses jc/unresolve-removal.)
Code clean-up.
Not ready to be reviewed yet.
source: <20230731224409.4181277-1-gitster@pobox.com>
* jc/unresolve-removal (2023-07-31) 7 commits
- checkout: allow "checkout -m path" to unmerge removed paths
- checkout/restore: add basic tests for --merge
- checkout/restore: refuse unmerging paths unless checking out of the index
- update-index: remove stale fallback code for "--unresolve"
- update-index: use unmerge_index_entry() to support removal
- resolve-undo: allow resurrecting conflicted state that resolved to deletion
- update-index: do not read HEAD and MERGE_HEAD unconditionally
(this branch is used by jc/rerere-cleanup.)
"checkout --merge -- path" and "update-index --unresolve path" did
not resurrect conflicted state that was resolved to remove path,
but now they do.
Will merge to 'next'.
source: <20230731224409.4181277-1-gitster@pobox.com>
* rj/status-bisect-while-rebase (2023-08-01) 1 commit
- status: fix branch shown when not only bisecting
"git status" is taught to show both the branch being bisected and
being rebased when both are in effect at the same time.
Needs review.
cf. <xmqqtttia3vn.fsf@gitster.g>
source: <48745298-f12b-8efb-4e48-90d2c22a8349@gmail.com>
--------------------------------------------------
[Discarded]
* tb/ci-coverity (2023-09-21) 1 commit
. .github/workflows: add coverity action
GitHub CI workflow has learned to trigger Coverity check.
Superseded by the js/ci-coverity topic.
source: <b23951c569660e1891a7fb3ad2c2ea1952897bd7.1695332105.git.me@ttaylorr.com>
^ permalink raw reply
* Re: [PATCH v3 0/9] Trailer readability cleanups
From: Junio C Hamano @ 2023-09-23 0:48 UTC (permalink / raw)
To: Linus Arver
Cc: Linus Arver via GitGitGadget, git, Glen Choo, Christian Couder,
Phillip Wood
In-Reply-To: <owlyzg1dsswr.fsf@fine.c.googlers.com>
Linus Arver <linusa@google.com> writes:
>> I could revert and discard [4-6/6] of the previous iteration out of
>> 'next' and have only the first three (which I thought have been
>> adequately reviewed without remaining issues) graduate to 'master',
>> if it makes it easier to fix this update on top, but I'd rather not
>> to encourage people to form a habit of reverting changes out of
>> 'next'.
>>
>> Thanks.
>
> I totally agree that reverting changes out of next is undesirable. I
> will do a reroll on top of 'next' with only those incremental (new)
> patches.
OK, so the first 3 patches are now in 'master', and the remainder of
the previous series have been discarded.
Thanks.
^ permalink raw reply
* Re: [PATCH v3 0/9] Trailer readability cleanups
From: Linus Arver @ 2023-09-22 23:13 UTC (permalink / raw)
To: Junio C Hamano, Linus Arver via GitGitGadget
Cc: git, Glen Choo, Christian Couder, Phillip Wood
In-Reply-To: <xmqq8r8xyge6.fsf@gitster.g>
Junio C Hamano <gitster@pobox.com> writes:
> "Linus Arver via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
>> These patches were created while digging into the trailer code to better
>> understand how it works, in preparation for making the trailer.{c,h} files
>> as small as possible to make them available as a library for external users.
>> This series was originally created as part of [1], but are sent here
>> separately because the changes here are arguably more subjective in nature.
>> I think Patch 1 is the most important in this series. The others can wait,
>> if folks are opposed to adding them on their own merits at this point in
>> time.
>
> Hmph, as we discussed, these changes have already been cooking in
> 'next' for some time:
>
> 13211ae23f trailer: separate public from internal portion of trailer_iterator
> c2a8edf997 trailer: split process_input_file into separate pieces
> 94430d03df trailer: split process_command_line_args into separate functions
> ee8c5ee08c trailer: teach find_patch_start about --no-divider
> d2be104085 trailer: rename *_DEFAULT enums to *_UNSPECIFIED
> b5e75f87b5 trailer: use offsets for trailer_start/trailer_end
>
> and I thought we agreed that we'll park them in 'next' and do
> whatever necessary fix-up on top as incremental patches?
Ahhh yes! I completely forgot. So sorry for the noise...
> The first
> three patches in this iteration seems to be identical to the
> previous round, so I can ignore them and keep the old iteration, but
> the remainder of this series are replacements that are suitable when
> the series was still out of 'next', but they are no longer usable
> once the series is in 'next'.
Right.
> I could revert and discard [4-6/6] of the previous iteration out of
> 'next' and have only the first three (which I thought have been
> adequately reviewed without remaining issues) graduate to 'master',
> if it makes it easier to fix this update on top, but I'd rather not
> to encourage people to form a habit of reverting changes out of
> 'next'.
>
> Thanks.
I totally agree that reverting changes out of next is undesirable. I
will do a reroll on top of 'next' with only those incremental (new)
patches.
^ permalink raw reply
* Re: [PATCH v3 0/9] Trailer readability cleanups
From: Junio C Hamano @ 2023-09-22 22:47 UTC (permalink / raw)
To: Linus Arver via GitGitGadget
Cc: git, Glen Choo, Christian Couder, Phillip Wood, Linus Arver
In-Reply-To: <pull.1563.v3.git.1695412245.gitgitgadget@gmail.com>
"Linus Arver via GitGitGadget" <gitgitgadget@gmail.com> writes:
> These patches were created while digging into the trailer code to better
> understand how it works, in preparation for making the trailer.{c,h} files
> as small as possible to make them available as a library for external users.
> This series was originally created as part of [1], but are sent here
> separately because the changes here are arguably more subjective in nature.
> I think Patch 1 is the most important in this series. The others can wait,
> if folks are opposed to adding them on their own merits at this point in
> time.
Hmph, as we discussed, these changes have already been cooking in
'next' for some time:
13211ae23f trailer: separate public from internal portion of trailer_iterator
c2a8edf997 trailer: split process_input_file into separate pieces
94430d03df trailer: split process_command_line_args into separate functions
ee8c5ee08c trailer: teach find_patch_start about --no-divider
d2be104085 trailer: rename *_DEFAULT enums to *_UNSPECIFIED
b5e75f87b5 trailer: use offsets for trailer_start/trailer_end
and I thought we agreed that we'll park them in 'next' and do
whatever necessary fix-up on top as incremental patches? The first
three patches in this iteration seems to be identical to the
previous round, so I can ignore them and keep the old iteration, but
the remainder of this series are replacements that are suitable when
the series was still out of 'next', but they are no longer usable
once the series is in 'next'.
I could revert and discard [4-6/6] of the previous iteration out of
'next' and have only the first three (which I thought have been
adequately reviewed without remaining issues) graduate to 'master',
if it makes it easier to fix this update on top, but I'd rather not
to encourage people to form a habit of reverting changes out of
'next'.
Thanks.
^ permalink raw reply
* Re: [PATCH] Documentation/git-status: add missing line breaks
From: Junio C Hamano @ 2023-09-22 22:29 UTC (permalink / raw)
To: Josh Soref via GitGitGadget; +Cc: git, Josh Soref
In-Reply-To: <pull.1590.git.1695392082207.gitgitgadget@gmail.com>
"Josh Soref via GitGitGadget" <gitgitgadget@gmail.com> writes:
> From: Josh Soref <jsoref@gmail.com>
>
> Signed-off-by: Josh Soref <jsoref@gmail.com>
> ---
> @@ -245,10 +245,12 @@ U U unmerged, both modified
> ....
>
> Submodules have more state and instead report
> +
> M the submodule has a different HEAD than
> recorded in the index
> m the submodule has modified content
> ? the submodule has untracked files
> +
> since modified content or untracked files in a submodule cannot be added
> via `git add` in the superproject to prepare a commit.
Ah, good find.
Thanks, will queue.
^ permalink raw reply
* Re: [PATCH 2/4] doc: update links to current pages
From: Junio C Hamano @ 2023-09-22 22:23 UTC (permalink / raw)
To: Josh Soref via GitGitGadget; +Cc: git, Josh Soref
In-Reply-To: <f9f3f4af1c8889eb69f777a322348afc53feeca2.1695392028.git.gitgitgadget@gmail.com>
"Josh Soref via GitGitGadget" <gitgitgadget@gmail.com> writes:
> From: Josh Soref <jsoref@gmail.com>
>
> Signed-off-by: Josh Soref <jsoref@gmail.com>
> ---
> Documentation/CodingGuidelines | 2 +-
> Documentation/RelNotes/1.6.2.txt | 2 +-
> Documentation/RelNotes/1.6.3.txt | 2 +-
> Documentation/RelNotes/1.6.4.txt | 2 +-
> Documentation/RelNotes/1.6.5.txt | 2 +-
> Documentation/RelNotes/1.6.6.txt | 2 +-
> Documentation/git-cvsimport.txt | 2 +-
> Documentation/git-format-patch.txt | 2 +-
> Documentation/git-ls-remote.txt | 4 ++--
> Documentation/git.txt | 2 +-
> Documentation/gitcore-tutorial.txt | 4 ++--
> compat/nedmalloc/malloc.c.h | 2 +-
> contrib/persistent-https/README | 2 +-
> git-gui/git-gui.sh | 2 +-
> gitk-git/gitk | 2 +-
> gitweb/static/js/lib/common-lib.js | 2 +-
> http.c | 2 +-
> imap-send.c | 2 +-
> json-writer.h | 2 +-
> 19 files changed, 21 insertions(+), 21 deletions(-)
Broken links are annoying while reading documentation. Thank you
for doing the necessary research.
I am not sure if direct replacement to Release Notes (historical
document) is the best way to present these updates, but let's assume
it is OK for now.
> diff --git a/Documentation/git-ls-remote.txt b/Documentation/git-ls-remote.txt
> index 1c4f696ab57..b9c8478a62a 100644
> --- a/Documentation/git-ls-remote.txt
> +++ b/Documentation/git-ls-remote.txt
> @@ -128,7 +128,7 @@ d4ca2e3147b409459955613c152220f4db848ee1 refs/tags/v2.40.0
> * List all references matching given patterns:
> +
> ----
> -$ git ls-remote http://www.kernel.org/pub/scm/git/git.git master seen rc
> +$ git ls-remote https://git.kernel.org/pub/scm/git/git.git master seen rc
> 5fe978a5381f1fbad26a80e682ddd2a401966740 refs/heads/master
> c781a84b5204fb294c9ccc79f8b3baceeb32c061 refs/heads/seen
> ----
I am not sure if this change is warranted. The old URL still works
just fine, even though there are no refs that match "rc" and the
"master" branch is no longer at 5fe978a5, but these are not problem
caused by using the http:// address. The point of the example is
not about preferred use of HTTP:// over HTTPS://, so this again
falls into "if we were writing this anew, we may have used the other
address, but the original still works, so it is not worth the patch
noise" category, I would think.
> diff --git a/Documentation/gitcore-tutorial.txt b/Documentation/gitcore-tutorial.txt
> index 2122aeb9769..924a9a97807 100644
> --- a/Documentation/gitcore-tutorial.txt
> +++ b/Documentation/gitcore-tutorial.txt
> @@ -1101,8 +1101,8 @@ Examples.
>
> the above are equivalent to:
>
> -. `git pull http://www.kernel.org/pub/scm/git/git.git/ HEAD`
> -. `git pull http://www.kernel.org/pub/scm/git/git.git/ tag v0.99.1`
> +. `git pull https://git.kernel.org/pub/scm/git/git.git/ HEAD`
> +. `git pull https://git.kernel.org/pub/scm/git/git.git/ tag v0.99.1`
Likewise.
> diff --git a/Documentation/git.txt b/Documentation/git.txt
> index 11228956cd5..c7292eb25d0 100644
> --- a/Documentation/git.txt
> +++ b/Documentation/git.txt
> @@ -1061,7 +1061,7 @@ Authors
> -------
> Git was started by Linus Torvalds, and is currently maintained by Junio
> C Hamano. Numerous contributions have come from the Git mailing list
> -<git@vger.kernel.org>. http://www.openhub.net/p/git/contributors/summary
> +<git@vger.kernel.org>. https://openhub.net/p/git/contributors/summary
> gives you a more complete list of contributors.
OK, even though the former seems to redirect to the latter, so in
that sense it is still "current".
> diff --git a/compat/nedmalloc/malloc.c.h b/compat/nedmalloc/malloc.c.h
> index 5c5357a379f..4b711c6b9ca 100644
> --- a/compat/nedmalloc/malloc.c.h
> +++ b/compat/nedmalloc/malloc.c.h
> @@ -1359,7 +1359,7 @@ LONG __cdecl _InterlockedExchange(LONG volatile *Target, LONG Value);
> /* --[ start GCC compatibility ]----------------------------------------------
> * Compatibility <intrin_x86.h> header for GCC -- GCC equivalents of intrinsic
> * Microsoft Visual C++ functions. Originally developed for the ReactOS
> - * (<http://www.reactos.org/>) and TinyKrnl (<http://www.tinykrnl.org/>)
> + * (<https://reactos.org/>) and TinyKrnl (<http://www.tinykrnl.org/>)
Likewise for reactos; the tinykrnl one does not seem to be current.
In any case, this is borrowed code, so I'd rather not touch it and
risk giving a wrong page that is not a replacement.
> diff --git a/contrib/persistent-https/README b/contrib/persistent-https/README
> index 2ad95893c27..2c9bec91066 100644
> --- a/contrib/persistent-https/README
> +++ b/contrib/persistent-https/README
> @@ -60,7 +60,7 @@ https://kernel.googlesource.com/pub/scm/git/git
>
> PREREQUISITES
>
> -The code is written in Go (http://golang.org/) and the Go compiler is
> +The code is written in Go (https://go.dev/) and the Go compiler is
Likewise.
> required. Currently, the compiler must be built and installed from tip
> of source, in order to include a fix in the reverse http proxy:
> https://code.google.com/p/go/source/detail?r=a615b796570a2cd8591884767a7d67ede74f6648
This is an interesting one. code.google.com is no longer there and
the updated URL seems to be at https://github.com/golang/go somewhere.
^ permalink raw reply
* Re: [PATCH 2/4] doc: update links to current pages
From: Junio C Hamano @ 2023-09-22 22:21 UTC (permalink / raw)
To: Josh Soref via GitGitGadget; +Cc: git, Josh Soref
In-Reply-To: <f9f3f4af1c8889eb69f777a322348afc53feeca2.1695392028.git.gitgitgadget@gmail.com>
"Josh Soref via GitGitGadget" <gitgitgadget@gmail.com> writes:
> From: Josh Soref <jsoref@gmail.com>
>
> Signed-off-by: Josh Soref <jsoref@gmail.com>
> ---
> Documentation/CodingGuidelines | 2 +-
> Documentation/RelNotes/1.6.2.txt | 2 +-
> Documentation/RelNotes/1.6.3.txt | 2 +-
> Documentation/RelNotes/1.6.4.txt | 2 +-
> Documentation/RelNotes/1.6.5.txt | 2 +-
> Documentation/RelNotes/1.6.6.txt | 2 +-
> Documentation/git-cvsimport.txt | 2 +-
> Documentation/git-format-patch.txt | 2 +-
> Documentation/git-ls-remote.txt | 4 ++--
> Documentation/git.txt | 2 +-
> Documentation/gitcore-tutorial.txt | 4 ++--
> compat/nedmalloc/malloc.c.h | 2 +-
> contrib/persistent-https/README | 2 +-
> git-gui/git-gui.sh | 2 +-
> gitk-git/gitk | 2 +-
> gitweb/static/js/lib/common-lib.js | 2 +-
> http.c | 2 +-
> imap-send.c | 2 +-
> json-writer.h | 2 +-
> 19 files changed, 21 insertions(+), 21 deletions(-)
Broken links are annoying while reading documentation. Thank you
for doing the necessary research.
I am not sure if direct replacement to Release Notes (historical
document) is the best way to present these updates, but let's assume
it is OK for now.
> diff --git a/Documentation/git-ls-remote.txt b/Documentation/git-ls-remote.txt
> index 1c4f696ab57..b9c8478a62a 100644
> --- a/Documentation/git-ls-remote.txt
> +++ b/Documentation/git-ls-remote.txt
> @@ -128,7 +128,7 @@ d4ca2e3147b409459955613c152220f4db848ee1 refs/tags/v2.40.0
> * List all references matching given patterns:
> +
> ----
> -$ git ls-remote http://www.kernel.org/pub/scm/git/git.git master seen rc
> +$ git ls-remote https://git.kernel.org/pub/scm/git/git.git master seen rc
> 5fe978a5381f1fbad26a80e682ddd2a401966740 refs/heads/master
> c781a84b5204fb294c9ccc79f8b3baceeb32c061 refs/heads/seen
> ----
I am not sure if this change is warranted. The old URL still works
just fine, even though there are no refs that match "rc" and the
"master" branch is no longer at 5fe978a5, but these are not problem
caused by using the http:// address. The point of the example is
not about preferred use of HTTP:// over HTTPS://, so this again
falls into "if we were writing this anew, we may have used the other
address, but the original still works, so it is not worth the patch
noise" category, I would think.
> diff --git a/Documentation/gitcore-tutorial.txt b/Documentation/gitcore-tutorial.txt
> index 2122aeb9769..924a9a97807 100644
> --- a/Documentation/gitcore-tutorial.txt
> +++ b/Documentation/gitcore-tutorial.txt
> @@ -1101,8 +1101,8 @@ Examples.
>
> the above are equivalent to:
>
> -. `git pull http://www.kernel.org/pub/scm/git/git.git/ HEAD`
> -. `git pull http://www.kernel.org/pub/scm/git/git.git/ tag v0.99.1`
> +. `git pull https://git.kernel.org/pub/scm/git/git.git/ HEAD`
> +. `git pull https://git.kernel.org/pub/scm/git/git.git/ tag v0.99.1`
Likewise.
> diff --git a/Documentation/git.txt b/Documentation/git.txt
> index 11228956cd5..c7292eb25d0 100644
> --- a/Documentation/git.txt
> +++ b/Documentation/git.txt
> @@ -1061,7 +1061,7 @@ Authors
> -------
> Git was started by Linus Torvalds, and is currently maintained by Junio
> C Hamano. Numerous contributions have come from the Git mailing list
> -<git@vger.kernel.org>. http://www.openhub.net/p/git/contributors/summary
> +<git@vger.kernel.org>. https://openhub.net/p/git/contributors/summary
> gives you a more complete list of contributors.
OK, even though the former seems to redirect to the latter, so in
that sense it is still "current".
> diff --git a/compat/nedmalloc/malloc.c.h b/compat/nedmalloc/malloc.c.h
> index 5c5357a379f..4b711c6b9ca 100644
> --- a/compat/nedmalloc/malloc.c.h
> +++ b/compat/nedmalloc/malloc.c.h
> @@ -1359,7 +1359,7 @@ LONG __cdecl _InterlockedExchange(LONG volatile *Target, LONG Value);
> /* --[ start GCC compatibility ]----------------------------------------------
> * Compatibility <intrin_x86.h> header for GCC -- GCC equivalents of intrinsic
> * Microsoft Visual C++ functions. Originally developed for the ReactOS
> - * (<http://www.reactos.org/>) and TinyKrnl (<http://www.tinykrnl.org/>)
> + * (<https://reactos.org/>) and TinyKrnl (<http://www.tinykrnl.org/>)
Likewise for reactos; the tinykrnl one does not seem to be current.
In any case, this is borrowed code, so I'd rather not touch it and
risk giving a wrong page that is not a replacement.
> diff --git a/contrib/persistent-https/README b/contrib/persistent-https/README
> index 2ad95893c27..2c9bec91066 100644
> --- a/contrib/persistent-https/README
> +++ b/contrib/persistent-https/README
> @@ -60,7 +60,7 @@ https://kernel.googlesource.com/pub/scm/git/git
>
> PREREQUISITES
>
> -The code is written in Go (http://golang.org/) and the Go compiler is
> +The code is written in Go (https://go.dev/) and the Go compiler is
Likewise.
> required. Currently, the compiler must be built and installed from tip
> of source, in order to include a fix in the reverse http proxy:
> https://code.google.com/p/go/source/detail?r=a615b796570a2cd8591884767a7d67ede74f6648
This is an interesting one. code.google.com is no longer tehre
> diff --git a/git-gui/git-gui.sh b/git-gui/git-gui.sh
> index 1d6102e0674..4a11d590b68 100755
> --- a/git-gui/git-gui.sh
> +++ b/git-gui/git-gui.sh
> @@ -2390,7 +2390,7 @@ proc do_quit {{rc {1}}} {
> set ret_code $rc
>
> # Briefly enable send again, working around Tk bug
> - # http://sourceforge.net/tracker/?func=detail&atid=112997&aid=1821174&group_id=12997
> + # https://sourceforge.net/p/tktoolkit/bugs/2343/
> tk appname [appname]
>
> destroy .
> diff --git a/gitk-git/gitk b/gitk-git/gitk
> index 1db46977df0..7a087f123d7 100755
> --- a/gitk-git/gitk
> +++ b/gitk-git/gitk
> @@ -12472,7 +12472,7 @@ if {[tk windowingsystem] eq "aqua"} {
>
> catch {
> # follow the XDG base directory specification by default. See
> - # http://standards.freedesktop.org/basedir-spec/basedir-spec-latest.html
> + # https://specifications.freedesktop.org/basedir-spec/basedir-spec-latest.html
> if {[info exists env(XDG_CONFIG_HOME)] && $env(XDG_CONFIG_HOME) ne ""} {
> # XDG_CONFIG_HOME environment variable is set
> set config_file [file join $env(XDG_CONFIG_HOME) git gitk]
> diff --git a/gitweb/static/js/lib/common-lib.js b/gitweb/static/js/lib/common-lib.js
> index 17b1796496d..99e3eb8c3d9 100644
> --- a/gitweb/static/js/lib/common-lib.js
> +++ b/gitweb/static/js/lib/common-lib.js
> @@ -137,7 +137,7 @@ function addCssRule(selector, style) {
> * http://www.dustindiaz.com/getelementsbyclass/
> * https://stackoverflow.com/questions/1818865/do-we-have-getelementsbyclassname-in-javascript
> *
> - * See also http://ejohn.org/blog/getelementsbyclassname-speed-comparison/
> + * See also https://johnresig.com/blog/getelementsbyclassname-speed-comparison/
> *
> * @param {String} class: name of _single_ class to find
> * @param {String} [taghint] limit search to given tags
> diff --git a/http.c b/http.c
> index e138b4b96fb..33c6011e8d8 100644
> --- a/http.c
> +++ b/http.c
> @@ -1877,7 +1877,7 @@ static void write_accept_language(struct strbuf *buf)
> * MAX_DECIMAL_PLACES must not be larger than 3. If it is larger than
> * that, q-value will be smaller than 0.001, the minimum q-value the
> * HTTP specification allows. See
> - * http://tools.ietf.org/html/rfc7231#section-5.3.1 for q-value.
> + * https://datatracker.ietf.org/doc/html/rfc7231#section-5.3.1 for q-value.
> */
> const int MAX_DECIMAL_PLACES = 3;
> const int MAX_LANGUAGE_TAGS = 1000;
> diff --git a/imap-send.c b/imap-send.c
> index 3b2077e39b2..448ca64c052 100644
> --- a/imap-send.c
> +++ b/imap-send.c
> @@ -860,7 +860,7 @@ static void imap_close_store(struct imap_store *ctx)
>
> /*
> * hexchar() and cram() functions are based on the code from the isync
> - * project (http://isync.sf.net/).
> + * project (https://isync.sourceforge.io/).
> */
> static char hexchar(unsigned int b)
> {
> diff --git a/json-writer.h b/json-writer.h
> index de140e54c98..04413bd1afd 100644
> --- a/json-writer.h
> +++ b/json-writer.h
> @@ -4,7 +4,7 @@
> /*
> * JSON data structures are defined at:
> * [1] https://www.ietf.org/rfc/rfc7159.txt
> - * [2] http://json.org/
> + * [2] https://www.json.org/
> *
> * The JSON-writer API allows one to build JSON data structures using a
> * simple wrapper around a "struct strbuf" buffer. It is intended as a
^ permalink raw reply
* Re: [PATCH 1/4] doc: switch links to https
From: Junio C Hamano @ 2023-09-22 22:05 UTC (permalink / raw)
To: Josh Soref via GitGitGadget; +Cc: git, Josh Soref
In-Reply-To: <b80c6089653bffc59faaa585e29d8c46a0611e83.1695392028.git.gitgitgadget@gmail.com>
"Josh Soref via GitGitGadget" <gitgitgadget@gmail.com> writes:
> From: Josh Soref <jsoref@gmail.com>
>
> Signed-off-by: Josh Soref <jsoref@gmail.com>
> ---
> Documentation/CodingGuidelines | 2 +-
> Documentation/MyFirstContribution.txt | 2 +-
> Documentation/git-cvsimport.txt | 2 +-
> Documentation/git-format-patch.txt | 2 +-
> Documentation/git-imap-send.txt | 2 +-
> Documentation/git-send-email.txt | 2 +-
> Documentation/gitcore-tutorial.txt | 2 +-
> Documentation/gitprotocol-http.txt | 4 ++--
> Documentation/gitweb.conf.txt | 2 +-
> Documentation/gitweb.txt | 2 +-
> Documentation/howto/keep-canonical-history-correct.txt | 2 +-
> Documentation/signoff-option.txt | 2 +-
> INSTALL | 2 +-
> Makefile | 4 ++--
> README.md | 2 +-
These are end-user facing and most of the changes looked sensible,
except for URLs that had comments like "at the time of this writing
the URL is at ...", which we should not bother touching. There may
be other reasons not to touch some of them, but I didn't look very
closely.
> compat/nedmalloc/malloc.c.h | 10 +++++-----
> compat/obstack.c | 2 +-
> compat/obstack.h | 2 +-
> compat/poll/poll.c | 2 +-
> compat/poll/poll.h | 2 +-
> compat/precompose_utf8.h | 2 +-
> compat/regex/regcomp.c | 2 +-
> compat/regex/regex.c | 2 +-
> compat/regex/regex.h | 2 +-
> compat/regex/regex_internal.c | 2 +-
> compat/regex/regex_internal.h | 2 +-
> compat/regex/regexec.c | 2 +-
> compat/vcbuild/README | 10 +++++-----
> contrib/completion/git-completion.bash | 2 +-
> .../credential/libsecret/git-credential-libsecret.c | 2 +-
> contrib/fast-import/import-directories.perl | 2 +-
> contrib/hg-to-git/hg-to-git.py | 2 +-
> contrib/mw-to-git/t/test-gitmw-lib.sh | 4 ++--
> contrib/persistent-https/LICENSE | 2 +-
> contrib/persistent-https/README | 2 +-
> contrib/thunderbird-patch-inline/appp.sh | 2 +-
> contrib/update-unicode/update_unicode.sh | 6 +++---
> convert.c | 2 +-
> ewah/bitmap.c | 2 +-
> ewah/ewah_bitmap.c | 2 +-
> ewah/ewah_io.c | 2 +-
> ewah/ewah_rlw.c | 2 +-
> ewah/ewok.h | 4 ++--
> ewah/ewok_rlw.h | 2 +-
> xdiff/xdiff.h | 2 +-
> xdiff/xdiffi.c | 2 +-
> xdiff/xdiffi.h | 2 +-
> xdiff/xemit.c | 2 +-
> xdiff/xemit.h | 2 +-
> xdiff/xinclude.h | 2 +-
> xdiff/xmacros.h | 2 +-
> xdiff/xmerge.c | 2 +-
> xdiff/xpatience.c | 2 +-
> xdiff/xprepare.c | 2 +-
> xdiff/xprepare.h | 2 +-
> xdiff/xtypes.h | 2 +-
> xdiff/xutils.c | 2 +-
> xdiff/xutils.h | 2 +-
There are mostly borrowed code. I would not touch them, if I were
doing this patch (unless the HTTP:// variant stopped to work and
only HTTPS:// variant works now, that is).
Thanks.
^ permalink raw reply
* Re: [PATCH] pretty-formats.txt: fix whitespace
From: Junio C Hamano @ 2023-09-22 21:56 UTC (permalink / raw)
To: Josh Soref via GitGitGadget; +Cc: git, Josh Soref
In-Reply-To: <pull.950.git.1695391818917.gitgitgadget@gmail.com>
"Josh Soref via GitGitGadget" <gitgitgadget@gmail.com> writes:
> From: Josh Soref <jsoref@gmail.com>
>
> * two spaces after periods for sentences
Although I admit I tend to hit my spacebar twice after a sentence, I
think more people tend to just do with a single space and let the
formatting software (like AsciiDoc) take care of the inter-sentence
spacing. To me, this falls into "it would be nicer if we did this
when we write a new sentence, but it is not worth the patch noise if
somebody already wrote with a single space to change them" category.
The title of the patch may have been written after you did the
above, but now it includes other changes listed below, so it may
want to be revised.
> * comma (before/)after `i.e.` and `e.g.`
That's modern American English. I think our documentation is
mixture so this may also fall into "if it is already written that
way, it is not worth the patch noise to change" category.
> * comma after `etc.` when not ending a sentence
There is one instance that says "A, B, C, D, etc., are all accepted."
without the comma after 'etc.' and the patch corrects it. OK.
> * space before `(`
If you mean by the above that we used to say
... as described below(see linkgit:git-config[1]))
and you added a SP before "(see", that is a definite improvement. I
however didn't find an example of a line that lacks SP before '('
that got corrected to have a SP there.
> Signed-off-by: Josh Soref <jsoref@gmail.com>
> ---
Thanks.
> pretty-formats.txt: fix whitespace
>
> follow-up-to:
> https://github.com/gitgitgadget/git/pull/939#discussion_r624652778
>
> * two spaces after periods for sentences
> * comma (before/)after i.e. and e.g.
> * space before (
>
> Signed-off-by: Josh Soref jsoref@gmail.com
>
> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-950%2Fjsoref%2Fwhitespace-v1
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-950/jsoref/whitespace-v1
> Pull-Request: https://github.com/gitgitgadget/git/pull/950
>
> Documentation/pretty-formats.txt | 68 ++++++++++++++++----------------
> 1 file changed, 34 insertions(+), 34 deletions(-)
>
> diff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt
> index d38b4ab5666..f64c0a2d92e 100644
> --- a/Documentation/pretty-formats.txt
> +++ b/Documentation/pretty-formats.txt
> @@ -15,7 +15,7 @@ There are several built-in formats, and you can define
> additional formats by setting a pretty.<name>
> config option to either another format name, or a
> 'format:' string, as described below (see
> -linkgit:git-config[1]). Here are the details of the
> +linkgit:git-config[1]). Here are the details of the
> built-in formats:
>
> * 'oneline'
> @@ -96,15 +96,15 @@ stored in the commit object. Notably, the hashes are
> displayed in full, regardless of whether --abbrev or
> --no-abbrev are used, and 'parents' information show the
> true parent commits, without taking grafts or history
> -simplification into account. Note that this format affects the way
> -commits are displayed, but not the way the diff is shown e.g. with
> -`git log --raw`. To get full object names in a raw diff format,
> +simplification into account. Note that this format affects the way
> +commits are displayed, but not the way the diff is shown, e.g., with
> +`git log --raw`. To get full object names in a raw diff format,
> use `--no-abbrev`.
>
> * 'format:<format-string>'
> +
> The 'format:<format-string>' format allows you to specify which information
> -you want to show. It works a little bit like printf format,
> +you want to show. It works a little bit like printf format,
> with the notable exception that you get a newline with '%n'
> instead of '\n'.
> +
> @@ -136,13 +136,13 @@ The placeholders are:
> default, colors are shown only when enabled for log output
> (by `color.diff`, `color.ui`, or `--color`, and respecting
> the `auto` settings of the former if we are going to a
> - terminal). `%C(auto,...)` is accepted as a historical
> - synonym for the default (e.g., `%C(auto,red)`). Specifying
> + terminal). `%C(auto,...)` is accepted as a historical
> + synonym for the default (e.g., `%C(auto,red)`). Specifying
> `%C(always,...)` will show the colors even when color is
> not otherwise enabled (though consider just using
> `--color=always` to enable color for the whole output,
> including this format and anything else git might color).
> - `auto` alone (i.e. `%C(auto)`) will turn on auto coloring
> + `auto` alone (i.e., `%C(auto)`) will turn on auto coloring
> on the next placeholders until the color is switched
> again.
> '%m':: left (`<`), right (`>`) or boundary (`-`) mark
> @@ -177,7 +177,7 @@ The placeholders are:
> spaces
> '%><( <N> )', '%><|( <M> )':: similar to '%<( <N> )', '%<|( <M> )'
> respectively, but padding both sides
> - (i.e. the text is centered)
> + (i.e., the text is centered)
>
> - Placeholders that expand to information extracted from the commit:
> '%H':: commit hash
> @@ -225,9 +225,9 @@ The placeholders are:
> '%d':: ref names, like the --decorate option of linkgit:git-log[1]
> '%D':: ref names without the " (", ")" wrapping.
> '%(decorate[:<options>])'::
> -ref names with custom decorations. The `decorate` string may be followed by a
> -colon and zero or more comma-separated options. Option values may contain
> -literal formatting codes. These must be used for commas (`%x2C`) and closing
> +ref names with custom decorations. The `decorate` string may be followed by a
> +colon and zero or more comma-separated options. Option values may contain
> +literal formatting codes. These must be used for commas (`%x2C`) and closing
> parentheses (`%x29`), due to their role in the option syntax.
> +
> ** 'prefix=<value>': Shown before the list of ref names. Defaults to "{nbsp}`(`".
> @@ -235,7 +235,7 @@ parentheses (`%x29`), due to their role in the option syntax.
> ** 'separator=<value>': Shown between ref names. Defaults to "`,`{nbsp}".
> ** 'pointer=<value>': Shown between HEAD and the branch it points to, if any.
> Defaults to "{nbsp}`->`{nbsp}".
> -** 'tag=<value>': Shown before tag names. Defaults to "`tag:`{nbsp}".
> +** 'tag=<value>': Shown before tag names. Defaults to "`tag:`{nbsp}".
>
> +
> For example, to produce decorations with no wrapping
> @@ -277,7 +277,7 @@ endif::git-rev-list[]
> "X" for a good signature that has expired,
> "Y" for a good signature made by an expired key,
> "R" for a good signature made by a revoked key,
> - "E" if the signature cannot be checked (e.g. missing key)
> + "E" if the signature cannot be checked (e.g., missing key)
> and "N" for no signature
> '%GS':: show the name of the signer for a signed commit
> '%GK':: show the key used to sign a signed commit
> @@ -287,7 +287,7 @@ endif::git-rev-list[]
> '%GT':: show the trust level for the key used to sign a signed commit
> '%gD':: reflog selector, e.g., `refs/stash@{1}` or `refs/stash@{2
> minutes ago}`; the format follows the rules described for the
> - `-g` option. The portion before the `@` is the refname as
> + `-g` option. The portion before the `@` is the refname as
> given on the command line (so `git log -g refs/heads/master`
> would yield `refs/heads/master@{0}`).
> '%gd':: shortened reflog selector; same as `%gD`, but the refname
> @@ -302,48 +302,48 @@ endif::git-rev-list[]
> '%gs':: reflog subject
> '%(trailers[:<options>])'::
> display the trailers of the body as interpreted by
> -linkgit:git-interpret-trailers[1]. The `trailers` string may be followed by
> -a colon and zero or more comma-separated options. If any option is provided
> +linkgit:git-interpret-trailers[1]. The `trailers` string may be followed by
> +a colon and zero or more comma-separated options. If any option is provided
> multiple times, the last occurrence wins.
> +
> -** 'key=<key>': only show trailers with specified <key>. Matching is done
> - case-insensitively and trailing colon is optional. If option is
> +** 'key=<key>': only show trailers with specified <key>. Matching is done
> + case-insensitively and trailing colon is optional. If option is
> given multiple times trailer lines matching any of the keys are
> - shown. This option automatically enables the `only` option so that
> - non-trailer lines in the trailer block are hidden. If that is not
> + shown. This option automatically enables the `only` option so that
> + non-trailer lines in the trailer block are hidden. If that is not
> desired it can be disabled with `only=false`. E.g.,
> `%(trailers:key=Reviewed-by)` shows trailer lines with key
> `Reviewed-by`.
> ** 'only[=<bool>]': select whether non-trailer lines from the trailer
> block should be included.
> ** 'separator=<sep>': specify a separator inserted between trailer
> - lines. When this option is not given each trailer line is
> - terminated with a line feed character. The string <sep> may contain
> - the literal formatting codes described above. To use comma as
> + lines. When this option is not given each trailer line is
> + terminated with a line feed character. The string <sep> may contain
> + the literal formatting codes described above. To use comma as
> separator one must use `%x2C` as it would otherwise be parsed as
> - next option. E.g., `%(trailers:key=Ticket,separator=%x2C )`
> + next option. E.g., `%(trailers:key=Ticket,separator=%x2C )`
> shows all trailer lines whose key is "Ticket" separated by a comma
> and a space.
> ** 'unfold[=<bool>]': make it behave as if interpret-trailer's `--unfold`
> - option was given. E.g.,
> + option was given. E.g.,
> `%(trailers:only,unfold=true)` unfolds and shows all trailer lines.
> ** 'keyonly[=<bool>]': only show the key part of the trailer.
> ** 'valueonly[=<bool>]': only show the value part of the trailer.
> ** 'key_value_separator=<sep>': specify a separator inserted between
> - trailer lines. When this option is not given each trailer key-value
> - pair is separated by ": ". Otherwise it shares the same semantics
> + trailer lines. When this option is not given each trailer key-value
> + pair is separated by ": ". Otherwise it shares the same semantics
> as 'separator=<sep>' above.
>
> NOTE: Some placeholders may depend on other options given to the
> -revision traversal engine. For example, the `%g*` reflog options will
> +revision traversal engine. For example, the `%g*` reflog options will
> insert an empty string unless we are traversing reflog entries (e.g., by
> -`git log -g`). The `%d` and `%D` placeholders will use the "short"
> +`git log -g`). The `%d` and `%D` placeholders will use the "short"
> decoration format if `--decorate` was not already provided on the command
> line.
>
> -The boolean options accept an optional value `[=<bool-value>]`. The values
> -`true`, `false`, `on`, `off` etc. are all accepted. See the "boolean"
> -sub-section in "EXAMPLES" in linkgit:git-config[1]. If a boolean
> +The boolean options accept an optional value `[=<bool-value>]`. The values
> +`true`, `false`, `on`, `off`, etc., are all accepted. See the "boolean"
> +sub-section in "EXAMPLES" in linkgit:git-config[1]. If a boolean
> option is given with no value, it's enabled.
>
> If you add a `+` (plus sign) after '%' of a placeholder, a line-feed
> @@ -361,7 +361,7 @@ placeholder expands to a non-empty string.
> * 'tformat:'
> +
> The 'tformat:' format works exactly like 'format:', except that it
> -provides "terminator" semantics instead of "separator" semantics. In
> +provides "terminator" semantics instead of "separator" semantics. In
> other words, each commit has the message terminator character (usually a
> newline) appended, rather than a separator placed between entries.
> This means that the final entry of a single-line format will be properly
>
> base-commit: bda494f4043963b9ec9a1ecd4b19b7d1cd9a0518
^ permalink raw reply
* Re: [PATCH 2/4] doc: update links to current pages
From: Eric Sunshine @ 2023-09-22 21:34 UTC (permalink / raw)
To: Josh Soref; +Cc: Josh Soref via GitGitGadget, git
In-Reply-To: <CACZqfqC_UwSPfn0f9L4TfkYDGU64t_en6=7wYMsnxA9fB2rSjw@mail.gmail.com>
On Fri, Sep 22, 2023 at 5:31 PM Josh Soref <jsoref@gmail.com> wrote:
> Eric Sunshine wrote:
> > > - * [2] http://json.org/
> > > + * [2] https://www.json.org/
> >
> > Not sure why this change is needed considering that the simpler
> > s/http/https/ seems sufficient.
>
> This is because that's their preference:
>
> ```sh
> % curl -s http://json.org/|grep REFRESH
> <meta HTTP-EQUIV="REFRESH" content="0;
> url=https://www.JSON.org/json-en.html"></head>
> ```
>
> It's somewhat traditional to respect sites' self-identification.
It might be worth calling that out in the commit message of both
patches if you happen to reroll so other reviewers understand the
motivation. (That said, on its own, it's probably not worth a reroll.)
^ permalink raw reply
* Re: [PATCH 2/4] doc: update links to current pages
From: Josh Soref @ 2023-09-22 21:30 UTC (permalink / raw)
To: Eric Sunshine; +Cc: Josh Soref via GitGitGadget, git
In-Reply-To: <CAPig+cRc49GCr+z+LA65VFS8RTaOEkKe8KszQOUhFPxZGQ_QmQ@mail.gmail.com>
Eric Sunshine wrote:
> > doc: update links to current pages
> > @@ -4,7 +4,7 @@
> > - * [2] http://json.org/
> > + * [2] https://www.json.org/
>
> Not sure why this change is needed considering that the simpler
> s/http/https/ seems sufficient.
This is because that's their preference:
```sh
% curl -s http://json.org/|grep REFRESH
<meta HTTP-EQUIV="REFRESH" content="0;
url=https://www.JSON.org/json-en.html"></head>
```
It's somewhat traditional to respect sites' self-identification.
^ permalink raw reply
* Re: [PATCH 2/4] doc: update links to current pages
From: Eric Sunshine @ 2023-09-22 21:25 UTC (permalink / raw)
To: Josh Soref via GitGitGadget; +Cc: git, Josh Soref
In-Reply-To: <f9f3f4af1c8889eb69f777a322348afc53feeca2.1695392028.git.gitgitgadget@gmail.com>
On Fri, Sep 22, 2023 at 10:14 AM Josh Soref via GitGitGadget
<gitgitgadget@gmail.com> wrote:
> doc: update links to current pages
>
> Signed-off-by: Josh Soref <jsoref@gmail.com>
> ---
> diff --git a/json-writer.h b/json-writer.h
> @@ -4,7 +4,7 @@
> /*
> * JSON data structures are defined at:
> * [1] https://www.ietf.org/rfc/rfc7159.txt
> - * [2] http://json.org/
> + * [2] https://www.json.org/
Not sure why this change is needed considering that the simpler
s/http/https/ seems sufficient.
^ permalink raw reply
* Re: [PATCH 1/4] doc: switch links to https
From: Eric Sunshine @ 2023-09-22 21:22 UTC (permalink / raw)
To: Josh Soref via GitGitGadget; +Cc: git, Josh Soref
In-Reply-To: <b80c6089653bffc59faaa585e29d8c46a0611e83.1695392028.git.gitgitgadget@gmail.com>
On Fri, Sep 22, 2023 at 10:14 AM Josh Soref via GitGitGadget
<gitgitgadget@gmail.com> wrote:
> doc: switch links to https
>
> Signed-off-by: Josh Soref <jsoref@gmail.com>
> ---
> #include "git-compat-util.h"
> --- a/json-writer.h
> +++ b/json-writer.h
> @@ -3,7 +3,7 @@
> /*
> * JSON data structures are defined at:
> - * [1] http://www.ietf.org/rfc/rfc7159.txt
> + * [1] https://www.ietf.org/rfc/rfc7159.txt
> * [2] http://json.org/
Shouldn't "json.org" receive the same s/http/https/ treatment?
^ permalink raw reply
* [PATCH v2] test-lib: fix GIT_TEST_SANITIZE_LEAK_LOG
From: Rubén Justo @ 2023-09-22 20:38 UTC (permalink / raw)
To: Git List
Cc: Ævar Arnfjörð Bjarmason, Eric Sunshine, Jeff King,
Junio C Hamano
In-Reply-To: <68522960-edda-26d3-ddca-cee63f2d859e@gmail.com>
In the if-elses chain we have in "check_test_results_san_file_", we
consider three variables: $passes_sanitize_leak, $sanitize_leak_check
and lastly, implicitly, GIT_TEST_SANITIZE_LEAK_LOG (always set to "true"
at that point).
For the first two variables we have different considerations depending
on the value of $test_failure, which make sense. However, for the
third, GIT_TEST_SANITIZE_LEAK_LOG, we don't; regardless of
$test_failure, we use "invert_exit_code=t" to produce a non-zero
return value.
That assumes "$test_failure = 0" is always true at that point. But it
is not:
$ git checkout v2.40.1
$ make SANITIZE=leak
$ make -C t GIT_TEST_SANITIZE_LEAK_LOG=true t3200-branch.sh
...
With GIT_TEST_SANITIZE_LEAK_LOG=true, our logs revealed a memory leak, exiting with a non-zero status!
# Simulated failures as TODO & now exiting with 0 due to --invert-exit-code
We need to use "invert_exit_code=t" only when "$test_failure = 0".
Probably we have missed this because GIT_TEST_SANITIZE_LEAK_LOG is
commonly used in combination with GIT_TEST_PASSING_SANITIZE_LEAK=check,
the second variable noted above, for which we have, as already said,
different considerations depending on the value of $test_failure.
However GIT_TEST_SANITIZE_LEAK_LOG has value used on its own, as it was
documented in faececa53f (test-lib: have the "check" mode for
SANITIZE=leak consider leak logs, 2022-07-28).
Let's add the missing conditions in the if-elses chain to make it work
as expected.
Note that we're not resetting the value of "invert_exit_code" when
$test_failure is non-zero. Doing that is only needed when
GIT_TEST_PASSING_SANITIZE_LEAK=check, the second variable noted above.
Helped-by: Eric Sunshine <sunshine@sunshineco.com>
Helped-by: Jeff King <peff@peff.net>
Signed-off-by: Rubén Justo <rjusto@gmail.com>
---
Changes since v1:
- dropped 1/1 as it was erroneously included
- reworded the message to better capture the situation and the goal
- show a message also when $test_failure is non-zero
It is probably worth revisiting the use of "invert_exit_code=t"
in this SANITIZE_LEAK machinery. But I'll stop here.
---
t/test-lib.sh | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/t/test-lib.sh b/t/test-lib.sh
index 5ea5d1d62a..945ede50ac 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -1264,9 +1264,12 @@ check_test_results_san_file_ () {
then
say "As TEST_PASSES_SANITIZE_LEAK=true isn't set the above leak is 'ok' with GIT_TEST_PASSING_SANITIZE_LEAK=check" &&
invert_exit_code=t
- else
+ elif test "$test_failure" = 0
+ then
say "With GIT_TEST_SANITIZE_LEAK_LOG=true our logs revealed a memory leak, exit non-zero!" &&
invert_exit_code=t
+ else
+ say "With GIT_TEST_SANITIZE_LEAK_LOG=true our logs revealed a memory leak..."
fi
}
--
2.34.1
^ permalink raw reply related
* Re: [PATCH v7 2/3] unit tests: add TAP unit test framework
From: Junio C Hamano @ 2023-09-22 20:05 UTC (permalink / raw)
To: phillip.wood123; +Cc: Josh Steadmon, git, linusa, calvinwan, rsbecker
In-Reply-To: <xmqq350hw6n7.fsf@gitster.g>
It seems this got stuck during Josh's absense and I didn't ping it
further, but I should have noticed that you are the author of this
patch, and pinged you in the meantime.
Any thought on the "polarity" of the return values from the
assertion? I still find it confusing and hard to follow.
Thanks.
> Josh Steadmon <steadmon@google.com> writes:
>
>> +test_expect_success 'TAP output from unit tests' '
>> + cat >expect <<-EOF &&
>> + ok 1 - passing test
>> + ok 2 - passing test and assertion return 0
>> + # check "1 == 2" failed at t/unit-tests/t-basic.c:76
>> + # left: 1
>> + # right: 2
>> + not ok 3 - failing test
>> + ok 4 - failing test and assertion return -1
>> + not ok 5 - passing TEST_TODO() # TODO
>> + ok 6 - passing TEST_TODO() returns 0
>> + # todo check ${SQ}check(x)${SQ} succeeded at t/unit-tests/t-basic.c:25
>> + not ok 7 - failing TEST_TODO()
>> + ok 8 - failing TEST_TODO() returns -1
>> + # check "0" failed at t/unit-tests/t-basic.c:30
>> + # skipping test - missing prerequisite
>> + # skipping check ${SQ}1${SQ} at t/unit-tests/t-basic.c:32
>> + ok 9 - test_skip() # SKIP
>> + ok 10 - skipped test returns 0
>> + # skipping test - missing prerequisite
>> + ok 11 - test_skip() inside TEST_TODO() # SKIP
>> + ok 12 - test_skip() inside TEST_TODO() returns 0
>> + # check "0" failed at t/unit-tests/t-basic.c:48
>> + not ok 13 - TEST_TODO() after failing check
>> + ok 14 - TEST_TODO() after failing check returns -1
>> + # check "0" failed at t/unit-tests/t-basic.c:56
>> + not ok 15 - failing check after TEST_TODO()
>> + ok 16 - failing check after TEST_TODO() returns -1
>> + # check "!strcmp("\thello\\\\", "there\"\n")" failed at t/unit-tests/t-basic.c:61
>> + # left: "\011hello\\\\"
>> + # right: "there\"\012"
>> + # check "!strcmp("NULL", NULL)" failed at t/unit-tests/t-basic.c:62
>> + # left: "NULL"
>> + # right: NULL
>> + # check "${SQ}a${SQ} == ${SQ}\n${SQ}" failed at t/unit-tests/t-basic.c:63
>> + # left: ${SQ}a${SQ}
>> + # right: ${SQ}\012${SQ}
>> + # check "${SQ}\\\\${SQ} == ${SQ}\\${SQ}${SQ}" failed at t/unit-tests/t-basic.c:64
>> + # left: ${SQ}\\\\${SQ}
>> + # right: ${SQ}\\${SQ}${SQ}
>> + not ok 17 - messages from failing string and char comparison
>> + # BUG: test has no checks at t/unit-tests/t-basic.c:91
>> + not ok 18 - test with no checks
>> + ok 19 - test with no checks returns -1
>> + 1..19
>> + EOF
>
> Presumably t-basic will serve as a catalog of check_* functions and
> the test binary, together with this test piece, will keep growing as
> we gain features in the unit tests infrastructure. I wonder how
> maintainable the above is, though. When we acquire new test, we
> would need to renumber. What if multiple developers add new
> features to the catalog at the same time?
>
>> diff --git a/t/unit-tests/.gitignore b/t/unit-tests/.gitignore
>> new file mode 100644
>> index 0000000000..e292d58348
>> --- /dev/null
>> +++ b/t/unit-tests/.gitignore
>> @@ -0,0 +1,2 @@
>> +/t-basic
>> +/t-strbuf
>
> Also, can we come up with some naming convention so that we do not
> have to keep adding to this file every time we add a new test
> script?
>
>> diff --git a/t/unit-tests/t-strbuf.c b/t/unit-tests/t-strbuf.c
>> new file mode 100644
>> index 0000000000..561611e242
>> --- /dev/null
>> +++ b/t/unit-tests/t-strbuf.c
>> @@ -0,0 +1,75 @@
>> +#include "test-lib.h"
>> +#include "strbuf.h"
>> +
>> +/* wrapper that supplies tests with an initialized strbuf */
>> +static void setup(void (*f)(struct strbuf*, void*), void *data)
>> +{
>> + struct strbuf buf = STRBUF_INIT;
>> +
>> + f(&buf, data);
>> + strbuf_release(&buf);
>> + check_uint(buf.len, ==, 0);
>> + check_uint(buf.alloc, ==, 0);
>> + check(buf.buf == strbuf_slopbuf);
>> + check_char(buf.buf[0], ==, '\0');
>> +}
>
> What I am going to utter from here on are not complaints but purely
> meant as questions.
>
> Would the resulting output and maintainability of the tests change
> (improve, or worsen) if we introduce
>
> static void assert_empty_strbuf(struct strbuf *buf)
> {
> check_uint(buf->len, ==, 0);
> check_uint(buf->alloc, ==, 0);
> check(buf.buf == strbuf_slopbuf);
> check_char(buf.buf[0], ==, '\0');
> }
>
> and call it from the setup() function to ensure that
> strbuf_release(&buf) it calls after running customer test f() brings
> the buffer in a reasonably initialized state? The t_static_init()
> test should be able to say
>
> static void t_static_init(void)
> {
> struct strbuf buf = STRBUF_INIT;
> assert_empty_strbuf(&buf);
> }
>
> if we did so, but is that a good thing or a bad thing (e.g. it may
> make it harder to figure out where the real error came from, because
> of the "line number" thing will not easily capture the caller of the
> caller, perhaps)?
>
>> +static void t_static_init(void)
>> +{
>> + struct strbuf buf = STRBUF_INIT;
>> +
>> + check_uint(buf.len, ==, 0);
>> + check_uint(buf.alloc, ==, 0);
>> + if (check(buf.buf == strbuf_slopbuf))
>> + return; /* avoid de-referencing buf.buf */
>
> strbuf_slopbuf[0] is designed to be readable. Do check() assertions
> return their parameter negated?
>
> In other words, if "we expect buf.buf to point at the slopbuf, but
> if that expectation does not hold, check() returns true and we
> refrain from doing check_char() on the next line because we cannot
> trust what buf.buf points at" is what is going on here, I find it
> very confusing. Perhaps my intuition is failing me, but somehow I
> would have expected that passing check_foo() would return true while
> failing ones would return false.
>
> IOW I would expect
>
> if (check(buf.buf == strbuf_slopbuf))
> return;
>
> to work very similarly to
>
> if (buf.buf == strbuf_slopbuf)
> return;
>
> in expressing the control flow, simply because they are visually
> similar. But of course, if we early-return because buf.buf that
> does not point at strbuf_slopbuf is a sign of trouble, then the
> control flow we want is
>
> if (buf.buf != strbuf_slopbuf)
> return;
>
> or
>
> if (!(buf.buf == strbuf_slopbuf))
> return;
>
> The latter is easier to translate to check_foo(), because what is
> inside the inner parentheses is the condition we expect, and we
> would like check_foo() to complain when the condition does not hold.
>
> For the "check_foo()" thing to work in a similar way, while having
> the side effect of reporting any failed expectations, we would want
> to write
>
> if (!check(buf.buf == strbuf_slopbuf))
> return;
>
> And for that similarity to work, check_foo() must return false when
> its expectation fails, and return true when its expectation holds.
>
> I think that is where my "I find it very confusing" comes from.
>
>> + check_char(buf.buf[0], ==, '\0');
>> +}
>
>> +static void t_dynamic_init(void)
>> +{
>> + struct strbuf buf;
>> +
>> + strbuf_init(&buf, 1024);
>> + check_uint(buf.len, ==, 0);
>> + check_uint(buf.alloc, >=, 1024);
>> + check_char(buf.buf[0], ==, '\0');
>
> Is it sensible to check buf.buf is not slopbuf at this point, or
> does it make the test TOO intimate with the current implementation
> detail?
>
>> + strbuf_release(&buf);
>> +}
>> +
>> +static void t_addch(struct strbuf *buf, void *data)
>> +{
>> + const char *p_ch = data;
>> + const char ch = *p_ch;
>> +
>> + strbuf_addch(buf, ch);
>> + if (check_uint(buf->len, ==, 1) ||
>> + check_uint(buf->alloc, >, 1))
>> + return; /* avoid de-referencing buf->buf */
>
> Again, I find the return values from these check_uint() calls highly
> confusing, if this is saying "if len is 1 and alloc is more than 1,
> then we are in an expected state and can further validate that buf[0]
> is ch and buf[1] is NULL, but otherwise we should punt". The polarity
> looks screwy. Perhaps it is just me?
>
>> + check_char(buf->buf[0], ==, ch);
>> + check_char(buf->buf[1], ==, '\0');
>> +}
>
> In any case, this t_addch() REQUIRES that incoming buf is empty,
> doesn't it? I do not think it is sensible. I would have expected
> that it would be more like
>
> t_addch(struct strbuf *buf, void *data)
> {
> char ch = *(char *)data;
> size_t orig_alloc = buf->alloc;
> size_t orig_len = buf->len;
>
> if (!assert_sane_strbuf(buf))
> return;
> strbuf_addch(buf, ch);
> if (!assert_sane_strbuf(buf))
> return;
> check_uint(buf->len, ==, orig_len + 1);
> check_uint(buf->alloc, >=, orig_alloc);
> check_char(buf->buf[buf->len - 1], ==, ch);
> check_char(buf->buf[buf->len], ==, '\0');
> }
>
> to ensure that we can add a ch to a strbuf with any existing
> contents and get a one-byte longer contents than before, with the
> last byte of the buffer becoming 'ch' and still NUL terminated.
>
> And we protect ourselves with a helper that checks if the given
> strbuf looks *sane*.
>
> static int assert_sane_strbuf(struct strbuf *buf)
> {
> /* can use slopbuf only when the length is 0 */
> if (buf->buf == strbuf_slopbuf)
> return (buf->len == 0);
> /* everybody else must have non-NULL buffer */
> if (buf->buf == NULL)
> return 0;
> /*
> * alloc must be at least 1 byte larger than len
> * for the terminating NUL at the end.
> */
> return ((buf->len + 1 <= buf->alloc) &&
> (buf->buf[buf->len] == '\0'));
> }
>
> You can obviously use your check_foo() for the individual checks
> done in this function to get a more detailed diagnosis, but because
> I have confused myself enough by thinking about their polarity, I
> wrote this in barebones comparison instead.
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox