* [PATCH] unit-tests: do show relative file paths on non-Windows, too
From: Junio C Hamano @ 2024-02-11 8:57 UTC (permalink / raw)
To: git; +Cc: Phillip Wood, Johannes Schindelin, Randall S. Becker
There are compilers other than Visual C that want to show absolute
paths. Generalize the helper introduced by a2c5e294 (unit-tests: do
show relative file paths, 2023-09-25) so that it can also work with
a path that uses slash as the directory separator, and becomes
almost no-op once one-time preparation finds out that we are using a
compiler that already gives relative paths. Incidentally, this also
should do the right thing on Windows with a compiler that shows
relative paths but with backslash as the directory separator (if
such a thing exists and is used to build git).
Reported-by: Randall S. Becker <rsbecker@nexbridge.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
* Another change I made, which is not described in the proposed
commit log message, is that we now use fspathcmp() instead of
strcmp() to precompute the prefix length using a known needle[]
string, to be consistent with the runtime check done for each and
every path.
This is a belated follow-up on <f0b804129e8a21449cbb6f346473d3570182ddfa.1695640837.git.gitgitgadget@gmail.com>
t/unit-tests/test-lib.c | 60 ++++++++++++++++++++++++++++++++---------
1 file changed, 47 insertions(+), 13 deletions(-)
diff --git a/t/unit-tests/test-lib.c b/t/unit-tests/test-lib.c
index 7bf9dfdb95..83c9eb8c59 100644
--- a/t/unit-tests/test-lib.c
+++ b/t/unit-tests/test-lib.c
@@ -21,12 +21,11 @@ static struct {
.result = RESULT_NONE,
};
-#ifndef _MSC_VER
-#define make_relative(location) location
-#else
/*
* Visual C interpolates the absolute Windows path for `__FILE__`,
* but we want to see relative paths, as verified by t0080.
+ * There are other compilers that do the same, and are not for
+ * Windows.
*/
#include "dir.h"
@@ -34,32 +33,67 @@ static const char *make_relative(const char *location)
{
static char prefix[] = __FILE__, buf[PATH_MAX], *p;
static size_t prefix_len;
+ static int need_bs_to_fs = -1;
- if (!prefix_len) {
+ /* one-time preparation */
+ if (need_bs_to_fs < 0) {
size_t len = strlen(prefix);
- const char *needle = "\\t\\unit-tests\\test-lib.c";
+ char needle[] = "t\\unit-tests\\test-lib.c";
size_t needle_len = strlen(needle);
- if (len < needle_len || strcmp(needle, prefix + len - needle_len))
+ if (len < needle_len)
+ die("unexpected prefix '%s'", prefix);
+
+ /*
+ * The path could be relative (t/unit-tests/test-lib.c)
+ * or full (/home/user/git/t/unit-tests/test-lib.c).
+ * Check the slash between "t" and "unit-tests".
+ */
+ prefix_len = len - needle_len;
+ if (prefix[prefix_len + 1] == '/') {
+ /* Oh, we're not Windows */
+ for (size_t i = 0; i < needle_len; i++)
+ if (needle[i] == '\\')
+ needle[i] = '/';
+ need_bs_to_fs = 0;
+ } else {
+ need_bs_to_fs = 1;
+ }
+
+ /*
+ * prefix_len == 0 if the compiler gives paths relative
+ * to the root of the working tree. Otherwise, we want
+ * to see that we did find the needle[] at a directory
+ * boundary.
+ */
+ if (fspathcmp(needle, prefix + prefix_len) ||
+ (prefix_len &&
+ prefix[prefix_len - 1] != '/' &&
+ prefix[prefix_len - 1] != '\\'))
die("unexpected suffix of '%s'", prefix);
- /* let it end in a directory separator */
- prefix_len = len - needle_len + 1;
}
+ /*
+ * If our compiler gives relative paths and we do not need
+ * to munge directory separator, we can return location as-is.
+ */
+ if (!prefix_len && !need_bs_to_fs)
+ return location;
+
/* Does it not start with the expected prefix? */
if (fspathncmp(location, prefix, prefix_len))
return location;
strlcpy(buf, location + prefix_len, sizeof(buf));
/* convert backslashes to forward slashes */
- for (p = buf; *p; p++)
- if (*p == '\\')
- *p = '/';
-
+ if (need_bs_to_fs) {
+ for (p = buf; *p; p++)
+ if (*p == '\\')
+ *p = '/';
+ }
return buf;
}
-#endif
static void msg_with_prefix(const char *prefix, const char *format, va_list ap)
{
--
2.44.0-rc0
^ permalink raw reply related
* Re: [PATCH] unit-tests: do show relative file paths on non-Windows, too
From: Phillip Wood @ 2024-02-11 11:03 UTC (permalink / raw)
To: Junio C Hamano, git; +Cc: Johannes Schindelin, Randall S. Becker
In-Reply-To: <xmqqttmf9y46.fsf@gitster.g>
Hi Junio
On 11/02/2024 08:57, Junio C Hamano wrote:
> There are compilers other than Visual C that want to show absolute
> paths. Generalize the helper introduced by a2c5e294 (unit-tests: do
> show relative file paths, 2023-09-25) so that it can also work with
> a path that uses slash as the directory separator, and becomes
> almost no-op once one-time preparation finds out that we are using a
> compiler that already gives relative paths. Incidentally, this also
> should do the right thing on Windows with a compiler that shows
> relative paths but with backslash as the directory separator (if
> such a thing exists and is used to build git).
>
> Reported-by: Randall S. Becker <rsbecker@nexbridge.com>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
>
> * Another change I made, which is not described in the proposed
> commit log message, is that we now use fspathcmp() instead of
> strcmp() to precompute the prefix length using a known needle[]
> string, to be consistent with the runtime check done for each and
> every path.
>
> This is a belated follow-up on <f0b804129e8a21449cbb6f346473d3570182ddfa.1695640837.git.gitgitgadget@gmail.com>
Thanks for putting this together - I was about to sit down and write
something similar when I saw your patch. I've left one comment below but
I don't think it is worth a re-roll, this looks good to me.
> + /*
> + * prefix_len == 0 if the compiler gives paths relative
> + * to the root of the working tree. Otherwise, we want
> + * to see that we did find the needle[] at a directory
> + * boundary.
> + */
> + if (fspathcmp(needle, prefix + prefix_len) ||
> + (prefix_len &&
> + prefix[prefix_len - 1] != '/' &&
> + prefix[prefix_len - 1] != '\\'))
We know which separator we're expecting so we could replace the last
two comparisons with
prefix[prefix_len -1] != needle[1]
but as I say I'm not sure that is worth re-rolling for
Best Wishes
Phillip
^ permalink raw reply
* Re: [PATCH 2/2] sequencer: unset GIT_CHERRY_PICK_HELP for 'exec' commands
From: Phillip Wood @ 2024-02-11 11:11 UTC (permalink / raw)
To: Junio C Hamano, Vegard Nossum; +Cc: Kristoffer Haugsbakk, git, Jonathan Nieder
In-Reply-To: <xmqqv86yoot3.fsf@gitster.g>
Hi Junio
On 08/02/2024 17:20, Junio C Hamano wrote:
> Phillip Wood <phillip.wood123@gmail.com> writes:
>
>> I think that typically for small suggestions like that we just add a
>> Helped-by: trailer but feel free to add my SOB if you want.
>
> Thanks, both. Here is what I assembled from the pieces.
>
> ----- >8 --------- >8 --------- >8 --------- >8 -----
> From: Vegard Nossum <vegard.nossum@oracle.com>
> Date: Fri, 2 Feb 2024 10:18:50 +0100
> Subject: [PATCH] sequencer: unset GIT_CHERRY_PICK_HELP for 'exec' commands
>
> Running "git cherry-pick" as an x-command in the rebase plan loses
> the original authorship information.
It might be worth explaining why this happens
This is because rebase sets the GIT_CHERRY_PICK_HELP environment
variable to customize the advice given to users when there are conflicts
which causes the sequencer to remove CHERRY_PICK_HEAD.
> To fix this, unset GIT_CHERRY_PICK_HELP for 'exec' commands.
The patch itself looks fine
Best Wishes
Phillip
> Helped-by: Phillip Wood <phillip.wood123@gmail.com>
> Signed-off-by: Vegard Nossum <vegard.nossum@oracle.com>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
> sequencer.c | 1 +
> t/t3404-rebase-interactive.sh | 12 ++++++++++++
> 2 files changed, 13 insertions(+)
>
> diff --git a/sequencer.c b/sequencer.c
> index d584cac8ed..ed30ceaf8b 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -3647,6 +3647,7 @@ static int do_exec(struct repository *r, const char *command_line)
> fprintf(stderr, _("Executing: %s\n"), command_line);
> cmd.use_shell = 1;
> strvec_push(&cmd.args, command_line);
> + strvec_push(&cmd.env, "GIT_CHERRY_PICK_HELP");
> status = run_command(&cmd);
>
> /* force re-reading of the cache */
> diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
> index c5f30554c6..84a92d6da0 100755
> --- a/t/t3404-rebase-interactive.sh
> +++ b/t/t3404-rebase-interactive.sh
> @@ -153,6 +153,18 @@ test_expect_success 'rebase -i with the exec command checks tree cleanness' '
> git rebase --continue
> '
>
> +test_expect_success 'cherry-pick works with rebase --exec' '
> + test_when_finished "git cherry-pick --abort; \
> + git rebase --abort; \
> + git checkout primary" &&
> + echo "exec git cherry-pick G" >todo &&
> + (
> + set_replace_editor todo &&
> + test_must_fail git rebase -i D D
> + ) &&
> + test_cmp_rev G CHERRY_PICK_HEAD
> +'
> +
> test_expect_success 'rebase -x with empty command fails' '
> test_when_finished "git rebase --abort ||:" &&
> test_must_fail env git rebase -x "" @ 2>actual &&
^ permalink raw reply
* [PATCH] ci: bump remaining outdated Actions versions
From: Johannes Schindelin via GitGitGadget @ 2024-02-11 11:52 UTC (permalink / raw)
To: git; +Cc: Johannes Schindelin, Johannes Schindelin
From: Johannes Schindelin <Johannes.Schindelin@gmx.de>
This avoids "Node.js 16 Actions" deprecation messages by bumping the
following Actions' versions:
- actions/upload-artifact from 3 to 4
- actions/download-artifact from 3 to 4
- actions/cache from 3 to 4
Helped-by: Matthias Aßhauer <mha1993@live.de>
Original-commits-by: dependabot[bot] <support@github.com>
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
ci: bump remaining outdated Actions versions
I noticed that Junio recently bumped a couple of these Actions versions,
and incidentally I also activated automatic Dependabot updates of those
in git-for-windows/git. Dependabot noticed a couple of yet-unaddressed
updates, which I accumulated into a single patch.
This patch is based on jc/github-actions-update.
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1660%2Fdscho%2Fmoar-github-actions-updates-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1660/dscho/moar-github-actions-updates-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1660
.github/workflows/coverity.yml | 4 ++--
.github/workflows/main.yml | 18 +++++++++---------
2 files changed, 11 insertions(+), 11 deletions(-)
diff --git a/.github/workflows/coverity.yml b/.github/workflows/coverity.yml
index a81a7566d10..53cf12fe044 100644
--- a/.github/workflows/coverity.yml
+++ b/.github/workflows/coverity.yml
@@ -98,7 +98,7 @@ jobs:
# A cache miss will add ~30s to create, but a cache hit will save minutes.
- name: restore the Coverity Build Tool
id: cache
- uses: actions/cache/restore@v3
+ uses: actions/cache/restore@v4
with:
path: ${{ runner.temp }}/cov-analysis
key: cov-build-${{ env.COVERITY_LANGUAGE }}-${{ env.COVERITY_PLATFORM }}-${{ steps.lookup.outputs.hash }}
@@ -141,7 +141,7 @@ jobs:
esac
- name: cache the Coverity Build Tool
if: steps.cache.outputs.cache-hit != 'true'
- uses: actions/cache/save@v3
+ uses: actions/cache/save@v4
with:
path: ${{ runner.temp }}/cov-analysis
key: cov-build-${{ env.COVERITY_LANGUAGE }}-${{ env.COVERITY_PLATFORM }}-${{ steps.lookup.outputs.hash }}
diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml
index bb857bdaf08..4eea49fc85c 100644
--- a/.github/workflows/main.yml
+++ b/.github/workflows/main.yml
@@ -123,7 +123,7 @@ jobs:
- name: zip up tracked files
run: git archive -o artifacts/tracked.tar.gz HEAD
- name: upload tracked files and build artifacts
- uses: actions/upload-artifact@v3
+ uses: actions/upload-artifact@v4
with:
name: windows-artifacts
path: artifacts
@@ -140,7 +140,7 @@ jobs:
cancel-in-progress: ${{ needs.ci-config.outputs.skip_concurrent == 'yes' }}
steps:
- name: download tracked files and build artifacts
- uses: actions/download-artifact@v3
+ uses: actions/download-artifact@v4
with:
name: windows-artifacts
path: ${{github.workspace}}
@@ -157,7 +157,7 @@ jobs:
run: ci/print-test-failures.sh
- name: Upload failed tests' directories
if: failure() && env.FAILED_TEST_ARTIFACTS != ''
- uses: actions/upload-artifact@v3
+ uses: actions/upload-artifact@v4
with:
name: failed-tests-windows
path: ${{env.FAILED_TEST_ARTIFACTS}}
@@ -212,7 +212,7 @@ jobs:
- name: zip up tracked files
run: git archive -o artifacts/tracked.tar.gz HEAD
- name: upload tracked files and build artifacts
- uses: actions/upload-artifact@v3
+ uses: actions/upload-artifact@v4
with:
name: vs-artifacts
path: artifacts
@@ -230,7 +230,7 @@ jobs:
steps:
- uses: git-for-windows/setup-git-for-windows-sdk@v1
- name: download tracked files and build artifacts
- uses: actions/download-artifact@v3
+ uses: actions/download-artifact@v4
with:
name: vs-artifacts
path: ${{github.workspace}}
@@ -248,7 +248,7 @@ jobs:
run: ci/print-test-failures.sh
- name: Upload failed tests' directories
if: failure() && env.FAILED_TEST_ARTIFACTS != ''
- uses: actions/upload-artifact@v3
+ uses: actions/upload-artifact@v4
with:
name: failed-tests-windows
path: ${{env.FAILED_TEST_ARTIFACTS}}
@@ -305,7 +305,7 @@ jobs:
run: ci/print-test-failures.sh
- name: Upload failed tests' directories
if: failure() && env.FAILED_TEST_ARTIFACTS != ''
- uses: actions/upload-artifact@v3
+ uses: actions/upload-artifact@v4
with:
name: failed-tests-${{matrix.vector.jobname}}
path: ${{env.FAILED_TEST_ARTIFACTS}}
@@ -353,13 +353,13 @@ jobs:
run: ci/print-test-failures.sh
- name: Upload failed tests' directories
if: failure() && env.FAILED_TEST_ARTIFACTS != '' && matrix.vector.jobname != 'linux32'
- uses: actions/upload-artifact@v3
+ uses: actions/upload-artifact@v4
with:
name: failed-tests-${{matrix.vector.jobname}}
path: ${{env.FAILED_TEST_ARTIFACTS}}
- name: Upload failed tests' directories
if: failure() && env.FAILED_TEST_ARTIFACTS != '' && matrix.vector.jobname == 'linux32'
- uses: actions/upload-artifact@v1
+ uses: actions/upload-artifact@v4
with:
name: failed-tests-${{matrix.vector.jobname}}
path: ${{env.FAILED_TEST_ARTIFACTS}}
base-commit: dcce2bda214ac4c838f4b85f2c550816df3a6ac9
--
gitgitgadget
^ permalink raw reply related
* [PATCH v2 0/2] ci: bump remaining outdated Actions versions
From: Johannes Schindelin via GitGitGadget @ 2024-02-11 12:11 UTC (permalink / raw)
To: git; +Cc: Johannes Schindelin
In-Reply-To: <pull.1660.git.1707652357696.gitgitgadget@gmail.com>
I noticed that Junio recently bumped a couple of these Actions versions, and
incidentally I also activated automatic Dependabot updates of those in
git-for-windows/git. Dependabot noticed a couple of yet-unaddressed updates,
which I accumulated into a single patch.
This patch is based on jc/github-actions-update.
Changes since v1 (sorry for the quick succession):
* The linux32 job cannot handle Node.js Actions, and therefore would fail
to run the latest actions/upload-artifact version. I only noticed this
after submitting v1 because CI does not fail (since this step is only in
use when something in Git's test suite breaks).
* To avoid making the same mistake even one more time, I added a commit
that puts big fat warnings next to the Actions that must not be updated
to newer (i.e. Node.js) versions.
Johannes Schindelin (2):
ci: bump remaining outdated Actions versions
ci(linux32): add a note about Actions that must not be updated
.github/workflows/coverity.yml | 4 ++--
.github/workflows/main.yml | 20 ++++++++++----------
2 files changed, 12 insertions(+), 12 deletions(-)
base-commit: dcce2bda214ac4c838f4b85f2c550816df3a6ac9
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1660%2Fdscho%2Fmoar-github-actions-updates-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1660/dscho/moar-github-actions-updates-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/1660
Range-diff vs v1:
1: 77c0810bf1a ! 1: 2b35b9878a7 ci: bump remaining outdated Actions versions
@@ .github/workflows/main.yml: jobs:
- name: Upload failed tests' directories
if: failure() && env.FAILED_TEST_ARTIFACTS != '' && matrix.vector.jobname != 'linux32'
- uses: actions/upload-artifact@v3
-+ uses: actions/upload-artifact@v4
- with:
- name: failed-tests-${{matrix.vector.jobname}}
- path: ${{env.FAILED_TEST_ARTIFACTS}}
- - name: Upload failed tests' directories
- if: failure() && env.FAILED_TEST_ARTIFACTS != '' && matrix.vector.jobname == 'linux32'
-- uses: actions/upload-artifact@v1
+ uses: actions/upload-artifact@v4
with:
name: failed-tests-${{matrix.vector.jobname}}
-: ----------- > 2: 9088cc60bda ci(linux32): add a note about Actions that must not be updated
--
gitgitgadget
^ permalink raw reply
* [PATCH v2 1/2] ci: bump remaining outdated Actions versions
From: Johannes Schindelin via GitGitGadget @ 2024-02-11 12:11 UTC (permalink / raw)
To: git; +Cc: Johannes Schindelin, Johannes Schindelin
In-Reply-To: <pull.1660.v2.git.1707653489.gitgitgadget@gmail.com>
From: Johannes Schindelin <Johannes.Schindelin@gmx.de>
This avoids "Node.js 16 Actions" deprecation messages by bumping the
following Actions' versions:
- actions/upload-artifact from 3 to 4
- actions/download-artifact from 3 to 4
- actions/cache from 3 to 4
Helped-by: Matthias Aßhauer <mha1993@live.de>
Original-commits-by: dependabot[bot] <support@github.com>
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
.github/workflows/coverity.yml | 4 ++--
.github/workflows/main.yml | 16 ++++++++--------
2 files changed, 10 insertions(+), 10 deletions(-)
diff --git a/.github/workflows/coverity.yml b/.github/workflows/coverity.yml
index a81a7566d10..53cf12fe044 100644
--- a/.github/workflows/coverity.yml
+++ b/.github/workflows/coverity.yml
@@ -98,7 +98,7 @@ jobs:
# A cache miss will add ~30s to create, but a cache hit will save minutes.
- name: restore the Coverity Build Tool
id: cache
- uses: actions/cache/restore@v3
+ uses: actions/cache/restore@v4
with:
path: ${{ runner.temp }}/cov-analysis
key: cov-build-${{ env.COVERITY_LANGUAGE }}-${{ env.COVERITY_PLATFORM }}-${{ steps.lookup.outputs.hash }}
@@ -141,7 +141,7 @@ jobs:
esac
- name: cache the Coverity Build Tool
if: steps.cache.outputs.cache-hit != 'true'
- uses: actions/cache/save@v3
+ uses: actions/cache/save@v4
with:
path: ${{ runner.temp }}/cov-analysis
key: cov-build-${{ env.COVERITY_LANGUAGE }}-${{ env.COVERITY_PLATFORM }}-${{ steps.lookup.outputs.hash }}
diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml
index bb857bdaf08..ec25f6f99de 100644
--- a/.github/workflows/main.yml
+++ b/.github/workflows/main.yml
@@ -123,7 +123,7 @@ jobs:
- name: zip up tracked files
run: git archive -o artifacts/tracked.tar.gz HEAD
- name: upload tracked files and build artifacts
- uses: actions/upload-artifact@v3
+ uses: actions/upload-artifact@v4
with:
name: windows-artifacts
path: artifacts
@@ -140,7 +140,7 @@ jobs:
cancel-in-progress: ${{ needs.ci-config.outputs.skip_concurrent == 'yes' }}
steps:
- name: download tracked files and build artifacts
- uses: actions/download-artifact@v3
+ uses: actions/download-artifact@v4
with:
name: windows-artifacts
path: ${{github.workspace}}
@@ -157,7 +157,7 @@ jobs:
run: ci/print-test-failures.sh
- name: Upload failed tests' directories
if: failure() && env.FAILED_TEST_ARTIFACTS != ''
- uses: actions/upload-artifact@v3
+ uses: actions/upload-artifact@v4
with:
name: failed-tests-windows
path: ${{env.FAILED_TEST_ARTIFACTS}}
@@ -212,7 +212,7 @@ jobs:
- name: zip up tracked files
run: git archive -o artifacts/tracked.tar.gz HEAD
- name: upload tracked files and build artifacts
- uses: actions/upload-artifact@v3
+ uses: actions/upload-artifact@v4
with:
name: vs-artifacts
path: artifacts
@@ -230,7 +230,7 @@ jobs:
steps:
- uses: git-for-windows/setup-git-for-windows-sdk@v1
- name: download tracked files and build artifacts
- uses: actions/download-artifact@v3
+ uses: actions/download-artifact@v4
with:
name: vs-artifacts
path: ${{github.workspace}}
@@ -248,7 +248,7 @@ jobs:
run: ci/print-test-failures.sh
- name: Upload failed tests' directories
if: failure() && env.FAILED_TEST_ARTIFACTS != ''
- uses: actions/upload-artifact@v3
+ uses: actions/upload-artifact@v4
with:
name: failed-tests-windows
path: ${{env.FAILED_TEST_ARTIFACTS}}
@@ -305,7 +305,7 @@ jobs:
run: ci/print-test-failures.sh
- name: Upload failed tests' directories
if: failure() && env.FAILED_TEST_ARTIFACTS != ''
- uses: actions/upload-artifact@v3
+ uses: actions/upload-artifact@v4
with:
name: failed-tests-${{matrix.vector.jobname}}
path: ${{env.FAILED_TEST_ARTIFACTS}}
@@ -353,7 +353,7 @@ jobs:
run: ci/print-test-failures.sh
- name: Upload failed tests' directories
if: failure() && env.FAILED_TEST_ARTIFACTS != '' && matrix.vector.jobname != 'linux32'
- uses: actions/upload-artifact@v3
+ uses: actions/upload-artifact@v4
with:
name: failed-tests-${{matrix.vector.jobname}}
path: ${{env.FAILED_TEST_ARTIFACTS}}
--
gitgitgadget
^ permalink raw reply related
* [PATCH v2 2/2] ci(linux32): add a note about Actions that must not be updated
From: Johannes Schindelin via GitGitGadget @ 2024-02-11 12:11 UTC (permalink / raw)
To: git; +Cc: Johannes Schindelin, Johannes Schindelin
In-Reply-To: <pull.1660.v2.git.1707653489.gitgitgadget@gmail.com>
From: Johannes Schindelin <johannes.schindelin@gmx.de>
The Docker container used by the `linux32` job comes without Node.js,
and therefore the `actions/checkout` and `actions/upload-artifact`
Actions cannot be upgraded to the latest versions (because they use
Node.js).
One time too many, I accidentally tried to update them, where
`actions/checkout` at least fails immediately, but the
`actions/upload-artifact` step is only used when any test fails, and
therefore the CI run usually passes even though that Action was updated
to a version that is incompatible with the Docker container in which
this job runs.
So let's add a big fat warning, mainly for my own benefit, to avoid
running into the very same issue over and over again.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
.github/workflows/main.yml | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml
index ec25f6f99de..7bacb322e4f 100644
--- a/.github/workflows/main.yml
+++ b/.github/workflows/main.yml
@@ -344,7 +344,7 @@ jobs:
steps:
- uses: actions/checkout@v4
if: matrix.vector.jobname != 'linux32'
- - uses: actions/checkout@v1
+ - uses: actions/checkout@v1 # cannot be upgraded because Node.js Actions aren't supported in this container
if: matrix.vector.jobname == 'linux32'
- run: ci/install-docker-dependencies.sh
- run: ci/run-build-and-tests.sh
@@ -359,7 +359,7 @@ jobs:
path: ${{env.FAILED_TEST_ARTIFACTS}}
- name: Upload failed tests' directories
if: failure() && env.FAILED_TEST_ARTIFACTS != '' && matrix.vector.jobname == 'linux32'
- uses: actions/upload-artifact@v1
+ uses: actions/upload-artifact@v1 # cannot be upgraded because Node.js Actions aren't supported in this container
with:
name: failed-tests-${{matrix.vector.jobname}}
path: ${{env.FAILED_TEST_ARTIFACTS}}
--
gitgitgadget
^ permalink raw reply related
* Re: [PATCH 0/7] t: drop more REFFILES prereqs
From: Karthik Nayak @ 2024-02-11 14:00 UTC (permalink / raw)
To: Patrick Steinhardt, git
In-Reply-To: <cover.1707463221.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 472 bytes --]
Hello,
Patrick Steinhardt <ps@pks.im> writes:
> Hi,
>
> this patch series is another step towards less tests with the REFFILES
> prerequisite. Some of the tests are simply moved into t0600 because they
> are inherently exercising low-level behaviour of the "files" backend.
> Other tests are adapted so that they work across both the "files" and
> the "reftable" backends.
>
> Patrick
>
I had a look at this series and have nothing to add. Looks great!
Thanks!
Karthik
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]
^ permalink raw reply
* [PATCH] t9146: replace test -d/-f with appropriate test_path_is_* function
From: Chandra Pratap via GitGitGadget @ 2024-02-11 14:53 UTC (permalink / raw)
To: git; +Cc: Chandra Pratap, Chandra Pratap
From: Chandra Pratap <chandrapratap3519@gmail.com>
The helper functions test_path_is_* provide better debugging
information than test -d/-e/-f.
Replace "! test -d" with "test_path_is_missing" at places where
we check for non-existent directories.
Replace "test -f" with "test_path_is_file" and "test -d" with
"test_path_is_dir" at places where we expect files or directories
to exist.
Signed-off-by: Chandra Pratap <chandrapratap3519@gmail.com>
---
t9146: replace test -d/-f with appropriate test_path_is_* function
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1661%2FChand-ra%2Ftestfix-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1661/Chand-ra/testfix-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1661
t/t9146-git-svn-empty-dirs.sh | 24 ++++++++++++------------
1 file changed, 12 insertions(+), 12 deletions(-)
diff --git a/t/t9146-git-svn-empty-dirs.sh b/t/t9146-git-svn-empty-dirs.sh
index 09606f1b3cf..532f5baa208 100755
--- a/t/t9146-git-svn-empty-dirs.sh
+++ b/t/t9146-git-svn-empty-dirs.sh
@@ -20,7 +20,7 @@ test_expect_success 'empty directories exist' '
cd cloned &&
for i in a b c d d/e d/e/f "weird file name"
do
- if ! test -d "$i"
+ if test_path_is_missing "$i"
then
echo >&2 "$i does not exist" &&
exit 1
@@ -37,7 +37,7 @@ test_expect_success 'option automkdirs set to false' '
git svn fetch &&
for i in a b c d d/e d/e/f "weird file name"
do
- if test -d "$i"
+ if test_path_is_dir "$i"
then
echo >&2 "$i exists" &&
exit 1
@@ -52,7 +52,7 @@ test_expect_success 'more emptiness' '
test_expect_success 'git svn rebase creates empty directory' '
( cd cloned && git svn rebase ) &&
- test -d cloned/"! !"
+ test_path_is_dir cloned/"! !"
'
test_expect_success 'git svn mkdirs recreates empty directories' '
@@ -62,7 +62,7 @@ test_expect_success 'git svn mkdirs recreates empty directories' '
git svn mkdirs &&
for i in a b c d d/e d/e/f "weird file name" "! !"
do
- if ! test -d "$i"
+ if test_path_is_missing "$i"
then
echo >&2 "$i does not exist" &&
exit 1
@@ -78,21 +78,21 @@ test_expect_success 'git svn mkdirs -r works' '
git svn mkdirs -r7 &&
for i in a b c d d/e d/e/f "weird file name"
do
- if ! test -d "$i"
+ if test_path_is_missing "$i"
then
echo >&2 "$i does not exist" &&
exit 1
fi
done &&
- if test -d "! !"
+ if test_path_is_dir "! !"
then
echo >&2 "$i should not exist" &&
exit 1
fi &&
git svn mkdirs -r8 &&
- if ! test -d "! !"
+ if test_path_is_missing "! !"
then
echo >&2 "$i not exist" &&
exit 1
@@ -114,7 +114,7 @@ test_expect_success 'empty directories in trunk exist' '
cd trunk &&
for i in a "weird file name"
do
- if ! test -d "$i"
+ if test_path_is_missing "$i"
then
echo >&2 "$i does not exist" &&
exit 1
@@ -138,16 +138,16 @@ test_expect_success 'git svn gc-ed files work' '
cd removed &&
git svn gc &&
: Compress::Zlib may not be available &&
- if test -f "$unhandled".gz
+ if test_path_is_file "$unhandled".gz
then
svn_cmd mkdir -m gz "$svnrepo"/gz &&
git reset --hard $(git rev-list HEAD | tail -1) &&
git svn rebase &&
- test -f "$unhandled".gz &&
- test -f "$unhandled" &&
+ test_path_is_file "$unhandled".gz &&
+ test_path_is_file "$unhandled" &&
for i in a b c "weird file name" gz "! !"
do
- if ! test -d "$i"
+ if test_path_is_missing "$i"
then
echo >&2 "$i does not exist" &&
exit 1
base-commit: 235986be822c9f8689be2e9a0b7804d0b1b6d821
--
gitgitgadget
^ permalink raw reply related
* Re: git gc changes ownerships of files linux
From: Torsten Bögershausen @ 2024-02-11 15:14 UTC (permalink / raw)
To: K_V; +Cc: git
In-Reply-To: <CABkRduQNdgdF8WhZadP5hyYvpEWgP_AE8=qzxNiRNA71bdJcYQ@mail.gmail.com>
On Sat, Feb 10, 2024 at 08:52:22PM +0100, K_V wrote:
> Hi team git
>
> Running 'git -C /srv/mssioncontrol/.git gc' on linux from a user which
> has access to another users files will occasionally change the
> ownership of the file 'packed-refs'. I believe git actually overwrites
> the file with a new one at the end af the gc command. But details
> about this is not writen in the help text. Could it be a bug?
>
> Use case: I'm using ansible to cleanup .git directories across
> multible servers and this issue is starting to cause problems.
>
> My solution is to make a variable containing the user and group id
> before running gc command and then reapply it afterwards:
> current_owner_uid_gid=$(stat -c "%u:%g" /srv/mssioncontrol/.git)
> git -C /srv/mssioncontrol/.git gc
> chown -R $current_owner_uid_gid "/srv/mssioncontrol/.git"
>
> Details:
> Debian GNU/Linux 12 (bookworm)
> git version 2.39.2
Thanks for reporting this -
I think that you have a working workaround ?
However, Git has a feature called "shared repository".
You need to define a (unix) group that is shared between
your user(s) and the ansible user.
The basic trick is to do
git config core.sharedRepository true
(And then change the ownership of all files/directories to the new group)
There is a docu here, please search for core.sharedRepository
https://git-scm.com/docs/git-config
I wonder, which solution is easier to maintain ?
>
> Best:
> zinen
>
^ permalink raw reply
* Re: git gc changes ownerships of files linux
From: brian m. carlson @ 2024-02-11 15:28 UTC (permalink / raw)
To: Torsten Bögershausen; +Cc: K_V, git
In-Reply-To: <20240211151455.GA27103@tb-raspi4>
[-- Attachment #1: Type: text/plain, Size: 917 bytes --]
On 2024-02-11 at 15:14:55, Torsten Bögershausen wrote:
> Thanks for reporting this -
> I think that you have a working workaround ?
>
> However, Git has a feature called "shared repository".
> You need to define a (unix) group that is shared between
> your user(s) and the ansible user.
>
> The basic trick is to do
> git config core.sharedRepository true
>
> (And then change the ownership of all files/directories to the new group)
On Linux, you also should set each directory to be setgid. That's
because by default, Linux uses the user's current group ID as the group
to create new directories. However, you'll want the group to be
inherited from the parent directory, which is the behaviour when the
parent directory is setgid.
This behaviour is the default on the BSDs, and no such configuration is
required there.
--
brian m. carlson (he/him or they/them)
Toronto, Ontario, CA
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 262 bytes --]
^ permalink raw reply
* Re: git gc changes ownerships of files linux
From: Torsten Bögershausen @ 2024-02-11 15:43 UTC (permalink / raw)
To: brian m. carlson, K_V, git
In-Reply-To: <ZcjnuSAZiNHvA5h1@tapette.crustytoothpaste.net>
On Sun, Feb 11, 2024 at 03:28:57PM +0000, brian m. carlson wrote:
> On 2024-02-11 at 15:14:55, Torsten Bögershausen wrote:
> > Thanks for reporting this -
> > I think that you have a working workaround ?
> >
> > However, Git has a feature called "shared repository".
> > You need to define a (unix) group that is shared between
> > your user(s) and the ansible user.
> >
> > The basic trick is to do
> > git config core.sharedRepository true
> >
> > (And then change the ownership of all files/directories to the new group)
>
> On Linux, you also should set each directory to be setgid. That's
> because by default, Linux uses the user's current group ID as the group
> to create new directories. However, you'll want the group to be
> inherited from the parent directory, which is the behaviour when the
> parent directory is setgid.
>
> This behaviour is the default on the BSDs, and no such configuration is
> required there.
Briam, Hm, I wonder what this function (in path.c) does:
int adjust_shared_perm(const char *path)
According to my understanding, it was included into the Git codebase
to work around the missing "setgid" feature in Linux (and probably cygwin).
And then we have t/t1301-shared-repo.sh,
so I think we are in a good shape: for all systems that have posix permissions.
and do not overwrite them with facl or other features.
^ permalink raw reply
* [PATCH v2] unit-tests: do show relative file paths on non-Windows, too
From: Junio C Hamano @ 2024-02-11 15:58 UTC (permalink / raw)
To: Phillip Wood; +Cc: git, Johannes Schindelin, Randall S. Becker
In-Reply-To: <6872b42d-8763-44dc-9502-2362d1ed80a7@gmail.com>
>
> We know which separator we're expecting so we could replace the last
> two comparisons with
>
> prefix[prefix_len -1] != needle[1]
>
> but as I say I'm not sure that is worth re-rolling for
There is a larger clean-up opportunity to drop the need for making a
copy, which probably is worth doing, so I folded the above into this
version.
------- >8 ------------- >8 ------------- >8 ------------- >8 -------
There are compilers other than Visual C that want to show absolute
paths. Generalize the helper introduced by a2c5e294 (unit-tests: do
show relative file paths, 2023-09-25) so that it can also work with
a path that uses slash as the directory separator, and becomes
almost no-op once one-time preparation finds out that we are using a
compiler that already gives relative paths. Incidentally, this also
should do the right thing on Windows with a compiler that shows
relative paths but with backslash as the directory separator (if
such a thing exists and is used to build git).
Reported-by: Randall S. Becker <rsbecker@nexbridge.com>
Helped-by: Phillip Wood <phillip.wood@dunelm.org.uk>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
* I found that the diff relative to the result of applying v1 was
easier to follow than the range-diff, so here it is.
diff --git c/t/unit-tests/test-lib.c w/t/unit-tests/test-lib.c
index 83c9eb8c59..66d6980ffb 100644
--- c/t/unit-tests/test-lib.c
+++ w/t/unit-tests/test-lib.c
@@ -64,34 +64,33 @@ static const char *make_relative(const char *location)
* prefix_len == 0 if the compiler gives paths relative
* to the root of the working tree. Otherwise, we want
* to see that we did find the needle[] at a directory
- * boundary.
+ * boundary. Again we rely on that needle[] begins with
+ * "t" followed by the directory separator.
*/
if (fspathcmp(needle, prefix + prefix_len) ||
- (prefix_len &&
- prefix[prefix_len - 1] != '/' &&
- prefix[prefix_len - 1] != '\\'))
+ (prefix_len && prefix[prefix_len - 1] != needle[1]))
die("unexpected suffix of '%s'", prefix);
-
}
/*
- * If our compiler gives relative paths and we do not need
- * to munge directory separator, we can return location as-is.
+ * Does it not start with the expected prefix?
+ * Return it as-is without making it worse.
*/
- if (!prefix_len && !need_bs_to_fs)
+ if (prefix_len && fspathncmp(location, prefix, prefix_len))
return location;
- /* Does it not start with the expected prefix? */
- if (fspathncmp(location, prefix, prefix_len))
- return location;
+ /*
+ * If we do not need to munge directory separator, we can return
+ * the substring at the tail of the location.
+ */
+ if (!need_bs_to_fs)
+ return location + prefix_len;
- strlcpy(buf, location + prefix_len, sizeof(buf));
/* convert backslashes to forward slashes */
- if (need_bs_to_fs) {
- for (p = buf; *p; p++)
- if (*p == '\\')
- *p = '/';
- }
+ strlcpy(buf, location + prefix_len, sizeof(buf));
+ for (p = buf; *p; p++)
+ if (*p == '\\')
+ *p = '/';
return buf;
}
t/unit-tests/test-lib.c | 61 +++++++++++++++++++++++++++++++----------
1 file changed, 47 insertions(+), 14 deletions(-)
diff --git a/t/unit-tests/test-lib.c b/t/unit-tests/test-lib.c
index 7bf9dfdb95..66d6980ffb 100644
--- a/t/unit-tests/test-lib.c
+++ b/t/unit-tests/test-lib.c
@@ -21,12 +21,11 @@ static struct {
.result = RESULT_NONE,
};
-#ifndef _MSC_VER
-#define make_relative(location) location
-#else
/*
* Visual C interpolates the absolute Windows path for `__FILE__`,
* but we want to see relative paths, as verified by t0080.
+ * There are other compilers that do the same, and are not for
+ * Windows.
*/
#include "dir.h"
@@ -34,32 +33,66 @@ static const char *make_relative(const char *location)
{
static char prefix[] = __FILE__, buf[PATH_MAX], *p;
static size_t prefix_len;
+ static int need_bs_to_fs = -1;
- if (!prefix_len) {
+ /* one-time preparation */
+ if (need_bs_to_fs < 0) {
size_t len = strlen(prefix);
- const char *needle = "\\t\\unit-tests\\test-lib.c";
+ char needle[] = "t\\unit-tests\\test-lib.c";
size_t needle_len = strlen(needle);
- if (len < needle_len || strcmp(needle, prefix + len - needle_len))
- die("unexpected suffix of '%s'", prefix);
+ if (len < needle_len)
+ die("unexpected prefix '%s'", prefix);
+
+ /*
+ * The path could be relative (t/unit-tests/test-lib.c)
+ * or full (/home/user/git/t/unit-tests/test-lib.c).
+ * Check the slash between "t" and "unit-tests".
+ */
+ prefix_len = len - needle_len;
+ if (prefix[prefix_len + 1] == '/') {
+ /* Oh, we're not Windows */
+ for (size_t i = 0; i < needle_len; i++)
+ if (needle[i] == '\\')
+ needle[i] = '/';
+ need_bs_to_fs = 0;
+ } else {
+ need_bs_to_fs = 1;
+ }
- /* let it end in a directory separator */
- prefix_len = len - needle_len + 1;
+ /*
+ * prefix_len == 0 if the compiler gives paths relative
+ * to the root of the working tree. Otherwise, we want
+ * to see that we did find the needle[] at a directory
+ * boundary. Again we rely on that needle[] begins with
+ * "t" followed by the directory separator.
+ */
+ if (fspathcmp(needle, prefix + prefix_len) ||
+ (prefix_len && prefix[prefix_len - 1] != needle[1]))
+ die("unexpected suffix of '%s'", prefix);
}
- /* Does it not start with the expected prefix? */
- if (fspathncmp(location, prefix, prefix_len))
+ /*
+ * Does it not start with the expected prefix?
+ * Return it as-is without making it worse.
+ */
+ if (prefix_len && fspathncmp(location, prefix, prefix_len))
return location;
- strlcpy(buf, location + prefix_len, sizeof(buf));
+ /*
+ * If we do not need to munge directory separator, we can return
+ * the substring at the tail of the location.
+ */
+ if (!need_bs_to_fs)
+ return location + prefix_len;
+
/* convert backslashes to forward slashes */
+ strlcpy(buf, location + prefix_len, sizeof(buf));
for (p = buf; *p; p++)
if (*p == '\\')
*p = '/';
-
return buf;
}
-#endif
static void msg_with_prefix(const char *prefix, const char *format, va_list ap)
{
--
2.44.0-rc0
^ permalink raw reply related
* Re: git gc changes ownerships of files linux
From: Junio C Hamano @ 2024-02-11 16:43 UTC (permalink / raw)
To: Torsten Bögershausen; +Cc: brian m. carlson, K_V, git
In-Reply-To: <20240211154345.GA28699@tb-raspi4>
Torsten Bögershausen <tboegi@web.de> writes:
> Briam, Hm, I wonder what this function (in path.c) does:
>
> int adjust_shared_perm(const char *path)
>
> According to my understanding, it was included into the Git codebase
> to work around the missing "setgid" feature in Linux (and probably cygwin).
No. "g+s on directory" is required and depended upon for getting
the correct group ownership. We do not do anything to chown(2) a
filesystem entry to force what group it is owned by there.
What adjust_shared_perm() does is to counter what the screwed-up
umask settings the user may have causes. If you are a member of a
group and working in a directory owned by the group with other
members, you want to make sure others in the group can access the
files and the directories in the project. Their umask should be
loosened to at least 027 and preferrably to 007 to give group
members the same access as you do. Yet people do not loosen their
umask when starting work in such a group owned directory that is
supposed to be shared, as it would be extremely cumbersome to do
[*]. These users end up creating files with overly tight permission
bits, e.g. 0644 or 0700, and we go in with adjust_shared_perm() to
fix these modes.
You definitely must set up your initial directory with g+s if you
are usihng the group-writable shared directory model (which I would
actually be surprised to see in 2020---is a shared machine with more
than one user-account still a thing???); adjust_shared_perm() will
not help you there.
[Footnote]
* Unless, of course, you use some sort of "hook" in the shell to
notice you switched to a certain directory and run command
there---some people achieve this by aliasing their "cd", "pushd",
and "popd".
^ permalink raw reply
* Re: [PATCH v4 2/2] revision: implement `git log --merge` also for rebase/cherry-pick/revert
From: Philippe Blain @ 2024-02-11 16:43 UTC (permalink / raw)
To: Johannes Sixt
Cc: Elijah Newren, Michael Lohmann, Phillip Wood, Patrick Steinhardt,
Michael Lohmann, Junio C Hamano, git
In-Reply-To: <2cf557e9-bf48-4bf3-be24-c1eeaa887418@kdbg.org>
Hi Johannes,
Le 2024-02-11 à 03:34, Johannes Sixt a écrit :
>> Adjust the documentation of this option accordingly.
>>
>> Co-authored-by: Philippe Blain <levraiphilippeblain@gmail.com>
>> Co-authored-by: Johannes Sixt <j6t@kdbg.org>
>> Signed-off-by: Philippe Blain <levraiphilippeblain@gmail.com>
>> Signed-off-by: Michael Lohmann <mi.al.lohmann@gmail.com>
>> [jc: tweaked in j6t's precedence fix that tries REBASE_HEAD last]
>> Signed-off-by: Junio C Hamano <gitster@pobox.com>
>
> Signed-off-by trailers should occur in temporal order. Therefore, when
> you pick up a commit and resend it, you should keep existing
> Signed-off-by and add yours last.
Thank you, I did not know that. I guess Junio should be kept last though ?
Or maybe I should remove Junio's sign-off if I send a new version of the
patch ?
I'll resend with corrected order.
By the way, Michael put you as co-author but did not add your signed-off-by...
>> ---
>> Documentation/gitk.txt | 8 ++++----
>> Documentation/rev-list-options.txt | 6 ++++--
>> revision.c | 31 +++++++++++++++++++++++--------
>> 3 files changed, 31 insertions(+), 14 deletions(-)
>>
>> diff --git a/Documentation/gitk.txt b/Documentation/gitk.txt
>> index c2213bb77b..80ff4e149a 100644
>> --- a/Documentation/gitk.txt
>> +++ b/Documentation/gitk.txt
>> @@ -63,10 +63,10 @@ linkgit:git-rev-list[1] for a complete list.
>>
>> --merge::
>>
>> - After an attempt to merge stops with conflicts, show the commits on
>> - the history between two branches (i.e. the HEAD and the MERGE_HEAD)
>> - that modify the conflicted files and do not exist on all the heads
>> - being merged.
>> + Show commits touching conflicted paths in the range `HEAD...$OTHER`,
>> + where `$OTHER` is the first existing pseudoref in `MERGE_HEAD`,
>> + `CHERRY_PICK_HEAD`, `REVERT_HEAD` or `REBASE_HEAD`. Only works
>> + when the index has unmerged entries.
>
> Unfortunately, this patch does not help gitk. Gitk has its own logic to
> treat --merge and needs its own patch. This hunk should not be part of
> this patch.
Ah, you are right. I assumed it just used rev-list under the hood, but it's
not the case for this flag. I'll remove that hunk.
Thanks,
Philippe.
^ permalink raw reply
* Re: git gc changes ownerships of files linux
From: Torsten Bögershausen @ 2024-02-11 16:59 UTC (permalink / raw)
To: Junio C Hamano; +Cc: brian m. carlson, K_V, git
In-Reply-To: <xmqqcyt39cju.fsf@gitster.g>
On Sun, Feb 11, 2024 at 08:43:33AM -0800, Junio C Hamano wrote:
> Torsten Bögershausen <tboegi@web.de> writes:
>
> > Briam, Hm, I wonder what this function (in path.c) does:
> >
> > int adjust_shared_perm(const char *path)
> >
> > According to my understanding, it was included into the Git codebase
> > to work around the missing "setgid" feature in Linux (and probably cygwin).
>
> No. "g+s on directory" is required and depended upon for getting
> the correct group ownership. We do not do anything to chown(2) a
> filesystem entry to force what group it is owned by there.
>
> What adjust_shared_perm() does is to counter what the screwed-up
> umask settings the user may have causes. If you are a member of a
> group and working in a directory owned by the group with other
> members, you want to make sure others in the group can access the
> files and the directories in the project. Their umask should be
> loosened to at least 027 and preferrably to 007 to give group
> members the same access as you do. Yet people do not loosen their
> umask when starting work in such a group owned directory that is
> supposed to be shared, as it would be extremely cumbersome to do
> [*]. These users end up creating files with overly tight permission
> bits, e.g. 0644 or 0700, and we go in with adjust_shared_perm() to
> fix these modes.
>
> You definitely must set up your initial directory with g+s if you
> are usihng the group-writable shared directory model (which I would
> actually be surprised to see in 2020---is a shared machine with more
> than one user-account still a thing???); adjust_shared_perm() will
> not help you there.
>
Oh - I learned something today and sorry for the noise.
>
> [Footnote]
>
> * Unless, of course, you use some sort of "hook" in the shell to
> notice you switched to a certain directory and run command
> there---some people achieve this by aliasing their "cd", "pushd",
> and "popd".
>
>
^ permalink raw reply
* Re: [PATCH 2/2] sequencer: unset GIT_CHERRY_PICK_HELP for 'exec' commands
From: Junio C Hamano @ 2024-02-11 17:05 UTC (permalink / raw)
To: Phillip Wood; +Cc: Vegard Nossum, Kristoffer Haugsbakk, git, Jonathan Nieder
In-Reply-To: <4073b764-ab6a-4b4b-a8a3-2e898620b2f5@gmail.com>
Phillip Wood <phillip.wood123@gmail.com> writes:
> Hi Junio
>
> On 08/02/2024 17:20, Junio C Hamano wrote:
>> Phillip Wood <phillip.wood123@gmail.com> writes:
>>
>>> I think that typically for small suggestions like that we just add a
>>> Helped-by: trailer but feel free to add my SOB if you want.
>> Thanks, both. Here is what I assembled from the pieces.
>> ----- >8 --------- >8 --------- >8 --------- >8 -----
>> From: Vegard Nossum <vegard.nossum@oracle.com>
>> Date: Fri, 2 Feb 2024 10:18:50 +0100
>> Subject: [PATCH] sequencer: unset GIT_CHERRY_PICK_HELP for 'exec' commands
>> Running "git cherry-pick" as an x-command in the rebase plan loses
>> the original authorship information.
>
> It might be worth explaining why this happens
>
> This is because rebase sets the GIT_CHERRY_PICK_HELP environment
> variable to customize the advice given to users when there are
> conflicts which causes the sequencer to remove CHERRY_PICK_HEAD.
True. I'd prefer to see the original submitter assemble the pieces
and come up with the final version, rather than me doing so.
Thanks.
^ permalink raw reply
* Re: git column fails (or crashes) if padding is negative
From: Kristoffer Haugsbakk @ 2024-02-11 17:08 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Tiago Pascoal, git@vger.kernel.org
In-Reply-To: <xmqqttmhfrko.fsf@gitster.g>
On Fri, Feb 9, 2024, at 18:57, Junio C Hamano wrote:
> builtin/column.c | 2 ++
> column.c | 4 ++--
> 2 files changed, 4 insertions(+), 2 deletions(-)
>
> […]
> diff --git c/column.c w/column.c
> index ff2f0abf39..9cc703832a 100644
> --- c/column.c
> +++ w/column.c
> @@ -189,7 +189,7 @@ void print_columns(const struct string_list *list,
> unsigned int colopts,
> memset(&nopts, 0, sizeof(nopts));
> nopts.indent = opts && opts->indent ? opts->indent : "";
> nopts.nl = opts && opts->nl ? opts->nl : "\n";
> - nopts.padding = opts ? opts->padding : 1;
> + nopts.padding = (opts && 0 < opts->padding) ? opts->padding : 1;
If these two are meant to check the same condition as in
`builtin/column.c`, shouldn’t it be `0 <= opts->padding`?
> nopts.width = opts && opts->width ? opts->width : term_columns() - 1;
> if (!column_active(colopts)) {
> display_plain(list, "", "\n");
> @@ -373,7 +373,7 @@ int run_column_filter(int colopts, const struct
> column_options *opts)
> strvec_pushf(argv, "--width=%d", opts->width);
> if (opts && opts->indent)
> strvec_pushf(argv, "--indent=%s", opts->indent);
> - if (opts && opts->padding)
> + if (opts && 0 < opts->padding)
> strvec_pushf(argv, "--padding=%d", opts->padding);
>
> fflush(stdout);
--
Kristoffer Haugsbakk
^ permalink raw reply
* Re: [PATCH] column: disallow negative padding
From: Kristoffer Haugsbakk @ 2024-02-11 17:10 UTC (permalink / raw)
To: Chris Torek; +Cc: git, Tiago Pascoal, Junio C Hamano
In-Reply-To: <CAPx1GvdDvmBmvoktd7onB4mSzikKf4eWVWnrzrn8c8Y1RcRgsA@mail.gmail.com>
On Sat, Feb 10, 2024, at 10:48, Chris Torek wrote:
> On Sat, Feb 10, 2024 at 1:46 AM Kristoffer Haugsbakk
> <code@khaugsbakk.name> wrote:
>> I forgot tests.
>
> You presumably also wanted the `_` here for gettext-ing:
>
>> + die("%s: argument must be a non-negative integer", "padding");
>
> Chris
Yeah, thanks. You probably saved me a v3. :)
Although I failed to notice that the string I stole was just a plain
string, not a translation string. And apparently there are no generic
“non-negative” translation strings. So I’ll just make a new one.
Cheers
--
Kristoffer Haugsbakk
^ permalink raw reply
* Re: [PATCH] ci: bump remaining outdated Actions versions
From: Junio C Hamano @ 2024-02-11 17:17 UTC (permalink / raw)
To: Johannes Schindelin via GitGitGadget; +Cc: git, Johannes Schindelin
In-Reply-To: <pull.1660.git.1707652357696.gitgitgadget@gmail.com>
"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
writes:
> From: Johannes Schindelin <Johannes.Schindelin@gmx.de>
>
> This avoids "Node.js 16 Actions" deprecation messages by bumping the
> following Actions' versions:
>
> - actions/upload-artifact from 3 to 4
> - actions/download-artifact from 3 to 4
> - actions/cache from 3 to 4
>
> Helped-by: Matthias Aßhauer <mha1993@live.de>
> Original-commits-by: dependabot[bot] <support@github.com>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
> - name: Upload failed tests' directories
> if: failure() && env.FAILED_TEST_ARTIFACTS != '' && matrix.vector.jobname == 'linux32'
> - uses: actions/upload-artifact@v1
> + uses: actions/upload-artifact@v4
> with:
> name: failed-tests-${{matrix.vector.jobname}}
> path: ${{env.FAILED_TEST_ARTIFACTS}}
Curious that, among all other uses of actions/upload-artifact@v3,
only this one has been using @v1, which may deserve explanation.
The proposed commit log message pretends that this never existed.
Please drop a non-standard "Original-commits-by:" trailer, and
instead mention what you wrote under three-dash line about the
dependabot in the log message. Perhaps something like...
After activating automatic Dependabot updates in the
git-for-windows/git repository, Dependabot noticed a couple
of yet-unaddressed updates. They avoid "Node.js 16 Actions"
deprecation messages by bumping the following Actions'
versions:
- actions/upload-artifact from 1 or 3 to 4
- actions/download-artifact from 3 to 4
- actions/cache from 3 to 4
Note that one actions/upload-artifact@v1 was used in one of
the rules because ...
Helped-by: Matthias Aßhauer <mha1993@live.de>
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Thanks.
^ permalink raw reply
* Re: [PATCH] column: disallow negative padding
From: Junio C Hamano @ 2024-02-11 17:55 UTC (permalink / raw)
To: Kristoffer Haugsbakk; +Cc: Chris Torek, git, Tiago Pascoal
In-Reply-To: <9c00311d-e31c-428b-9c66-fef7ac8bfc76@app.fastmail.com>
"Kristoffer Haugsbakk" <code@khaugsbakk.name> writes:
> On Sat, Feb 10, 2024, at 10:48, Chris Torek wrote:
>> On Sat, Feb 10, 2024 at 1:46 AM Kristoffer Haugsbakk
>> <code@khaugsbakk.name> wrote:
>>> I forgot tests.
>>
>> You presumably also wanted the `_` here for gettext-ing:
>>
>>> + die("%s: argument must be a non-negative integer", "padding");
>>
>> Chris
>
> Yeah, thanks. You probably saved me a v3. :)
>
> Although I failed to notice that the string I stole was just a plain
> string, not a translation string. And apparently there are no generic
> “non-negative” translation strings. So I’ll just make a new one.
The last time I took a look, I thought there were more than just the
single entry point you patched that can feed negative padding into
the machinery? Don't you need to cover them as well?
Thanks.
^ permalink raw reply
* Re: [PATCH] t9146: replace test -d/-f with appropriate test_path_is_* function
From: Eric Sunshine @ 2024-02-11 17:58 UTC (permalink / raw)
To: Chandra Pratap via GitGitGadget; +Cc: git, Chandra Pratap, Chandra Pratap
In-Reply-To: <pull.1661.git.1707663197543.gitgitgadget@gmail.com>
On Sun, Feb 11, 2024 at 9:53 AM Chandra Pratap via GitGitGadget
<gitgitgadget@gmail.com> wrote:
> The helper functions test_path_is_* provide better debugging
> information than test -d/-e/-f.
>
> Replace "! test -d" with "test_path_is_missing" at places where
> we check for non-existent directories.
>
> Replace "test -f" with "test_path_is_file" and "test -d" with
> "test_path_is_dir" at places where we expect files or directories
> to exist.
The aim of this patch makes sense, but the implementation needs some
refinement...
> diff --git a/t/t9146-git-svn-empty-dirs.sh b/t/t9146-git-svn-empty-dirs.sh
> @@ -20,7 +20,7 @@ test_expect_success 'empty directories exist' '
> for i in a b c d d/e d/e/f "weird file name"
> do
> - if ! test -d "$i"
> + if test_path_is_missing "$i"
> then
> echo >&2 "$i does not exist" &&
> exit 1
The point of functions such as test_path_is_missing() is to _assert_
that some condition is true, thus allowing the test to succeed; if the
condition is not true, then the function prints an error message and
the test aborts and fails.
test_path_is_missing () {
if test -e "$1"
then
echo "Path exists:"
ls -ld "$1"
false
fi
}
It is meant to replace noisy code such as:
if ! test -f bloop
then
echo >&2 "error message" &&
exit 1
fi &&
other-code
with much simpler:
test_path_exists bloop &&
other-code
So, the changes made by this patch are incorrect in two ways...
First, the patch retains code which prints an error message even
though this code becomes redundant since the test_path_foo() functions
already take care of printing the error message.
Second, and more problematic, the patch incorrectly inverts the sense
of what is being tested. For instance, the title of this test is
"empty directories exist", and the body of the test asserts that the
empty directories _do_ exist, but the patch changes the condition to
assert that the directories do _not_ exist, which is wrong.
Taking these observations into account, this test should become:
test_expect_success 'empty directories exist' '
(
cd cloned &&
for i in a b c d d/e d/e/f "weird file name"
do
test_path_exists "$i" || exit 1
done
)
'
Many of the other changes made by this patch suffer similar problems
> @@ -138,16 +138,16 @@ test_expect_success 'git svn gc-ed files work' '
> : Compress::Zlib may not be available &&
> - if test -f "$unhandled".gz
> + if test_path_is_file "$unhandled".gz
> then
> svn_cmd mkdir -m gz "$svnrepo"/gz &&
> git reset --hard $(git rev-list HEAD | tail -1) &&
This change is wrong/undesirable for a different reason. Taking into
account what was said above about test_path_is_file() being an
_assertion_ that some condition is true, coupled with the comment
above this `if` statement which says "Compress::Zlib may not be
available", then this `test -f` is legitimately part of the
control-flow of the function. It is not a mere assertion. Thus,
replacing it with the assertion function test_path_is_file() breaks
the test for the case when Compress::Zlib is not available.
> - test -f "$unhandled".gz &&
> - test -f "$unhandled" &&
> + test_path_is_file "$unhandled".gz &&
> + test_path_is_file "$unhandled" &&
These replacements are correct in that they replace the _assertion_
`test -f` with the equivalent assertion `test_path_is_file`.
^ permalink raw reply
* Re: [PATCH v4 2/2] revision: implement `git log --merge` also for rebase/cherry-pick/revert
From: Johannes Sixt @ 2024-02-11 17:59 UTC (permalink / raw)
To: Philippe Blain
Cc: Elijah Newren, Michael Lohmann, Phillip Wood, Patrick Steinhardt,
Michael Lohmann, Junio C Hamano, git
In-Reply-To: <1c258037-cb08-5fbc-d473-743a60cd8eab@gmail.com>
Am 11.02.24 um 17:43 schrieb Philippe Blain:
> Hi Johannes,
>
> Le 2024-02-11 à 03:34, Johannes Sixt a écrit :
>
>>> Adjust the documentation of this option accordingly.
>>>
>>> Co-authored-by: Philippe Blain <levraiphilippeblain@gmail.com>
>>> Co-authored-by: Johannes Sixt <j6t@kdbg.org>
>>> Signed-off-by: Philippe Blain <levraiphilippeblain@gmail.com>
>>> Signed-off-by: Michael Lohmann <mi.al.lohmann@gmail.com>
>>> [jc: tweaked in j6t's precedence fix that tries REBASE_HEAD last]
>>> Signed-off-by: Junio C Hamano <gitster@pobox.com>
>>
>> Signed-off-by trailers should occur in temporal order. Therefore, when
>> you pick up a commit and resend it, you should keep existing
>> Signed-off-by and add yours last.
>
> Thank you, I did not know that. I guess Junio should be kept last though ?
> Or maybe I should remove Junio's sign-off if I send a new version of the
> patch ?
You should *not* remove Junio's Signed-off-by, because the patch went
through his hands before you picked it up. Then you add your own
sign-off below. Later, Junio will sign it off again.
> I'll resend with corrected order.
>
> By the way, Michael put you as co-author but did not add your signed-off-by...
This is fine and sufficient. Micheal used some of my ideas, but I didn't
take part in the patch submission process.
-- Hannes
^ permalink raw reply
* Re: Bug: Commit fails when no global email address is set even though --author is used
From: Kristoffer Haugsbakk @ 2024-02-11 18:16 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Marcus Tillmanns, git@vger.kernel.org, Phillip Wood
In-Reply-To: <xmqqfry1fm2e.fsf@gitster.g>
On Fri, Feb 9, 2024, at 20:56, Junio C Hamano wrote:
>> I think a three-way switch looks good. With the amendment that it steers
>> you towards `user.*` instead of setting both `author.*` and
>> `committer.*`.
>>
>> Something like
>>
>> • Author is set, not committer
>> • Message: author is set but not committer: you might want to set
>> *user* instead (prints suggested config)
>>
>> I can try to make a patch later.
>
> Wait. I didn't realize this when I wrote the message you are
> responding to, but we *do* already suggest settig user.* variables.
>
> If the user chose to ignore that, then there isn't much we can do to
> help, is there?
>
> Puzzled, but I'll stop here.
Aye, good point. Maybe I misremembered and/or didn’t look carefully
enough at the error message back when I set `author.*` instead of
`user.*`.
Maybe the error could say (back to the multi-way switch):
```
Author identity known, but committer identity unknown
*** Please tell me who you are.
Run
git config --global user.email "code@khaugsbakk.name"
git config --global user.name "Kristoffer Haugsbakk"
to set your account's default identity.
Omit --global to set the identity only in this repository.
fatal: empty ident name (for <>) not allowed
```
And then (see above) fill in the real information in the proposed config
commands if the other identity is known. Maybe that could be
accomplished with one more parameter to `ident_env_hint`:
```
static void ident_env_hint(enum want_ident whose_ident, char *other_ident)
```
(turned out to be a bit more tricky than I thought)
But that’s a slightly longer error message, which (again) can cause the
already-discussed glaze-over effect caused by the “wall of text”.
--
Kristoffer Haugsbakk
^ permalink raw reply
* Re: [PATCH] column: disallow negative padding
From: Kristoffer Haugsbakk @ 2024-02-11 18:18 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Chris Torek, git, Tiago Pascoal
In-Reply-To: <xmqqle7q997e.fsf@gitster.g>
On Sun, Feb 11, 2024, at 18:55, Junio C Hamano wrote:
> "Kristoffer Haugsbakk" <code@khaugsbakk.name> writes:
>> Yeah, thanks. You probably saved me a v3. :)
>>
>> Although I failed to notice that the string I stole was just a plain
>> string, not a translation string. And apparently there are no generic
>> “non-negative” translation strings. So I’ll just make a new one.
>
> The last time I took a look, I thought there were more than just the
> single entry point you patched that can feed negative padding into
> the machinery? Don't you need to cover them as well?
>
> Thanks.
I’ve incorporated the `column.c` patch you posted in my
not-yet-published v2. Hopefully that was it.(? :) ) I’ll take another
look.
v2 is finished now so maybe I’ll send it out soon.
--
Kristoffer Haugsbakk
^ 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