* Re: [PATCH v3 7/8] ci: install test dependencies for linux-musl
From: Patrick Steinhardt @ 2023-10-30 12:47 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Phillip Wood, Oswald Buddenhagen
In-Reply-To: <f150d61a1ceec5dd6cf370c3f4421a3e2d9f11a3.1698667545.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 3326 bytes --]
On Mon, Oct 30, 2023 at 01:15:10PM +0100, Patrick Steinhardt wrote:
> The linux-musl CI job executes tests on Alpine Linux, which is based on
> musl libc instead of glibc. We're missing some test dependencies though,
> which causes us to skip a subset of tests.
>
> Install these test dependencies to increase our test coverage on this
> platform. There are still some missing test dependecies, but these do
> not have a corresponding package in the Alpine repositories:
>
> - p4 and p4d, both parts of the Perforce version control system.
>
> - cvsps, which generates patch sets for CVS.
>
> - Subversion and the SVN::Core Perl library, the latter of which is
> not available in the Alpine repositories. While the tool itself is
> available, all Subversion-related tests are skipped without the
> SVN::Core Perl library anyway.
>
> Furthermore, in order to make the Apache-based tests work, this commit
> also adds the Alpine-specific modules path of it to the list of known
> paths.
>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
> ci/install-docker-dependencies.sh | 3 ++-
> t/lib-httpd.sh | 3 ++-
> 2 files changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/ci/install-docker-dependencies.sh b/ci/install-docker-dependencies.sh
> index d0bc19d3bb3..05dde5c5d40 100755
> --- a/ci/install-docker-dependencies.sh
> +++ b/ci/install-docker-dependencies.sh
> @@ -17,7 +17,8 @@ linux32)
> ;;
> linux-musl)
> apk add --update build-base curl-dev openssl-dev expat-dev gettext \
> - pcre2-dev python3 musl-libintl perl-utils ncurses >/dev/null
> + pcre2-dev python3 musl-libintl perl-utils ncurses \
> + apache2 bash cvs gnupg perl-cgi perl-dbd-sqlite >/dev/null
Meh. I just noticed that I missed a few other dependencies to make
Apache2 work:
diff --git a/ci/install-docker-dependencies.sh b/ci/install-docker-dependencies.sh
index 5e28adf55b..ce910e3f3c 100755
--- a/ci/install-docker-dependencies.sh
+++ b/ci/install-docker-dependencies.sh
@@ -21,7 +21,8 @@ linux32)
linux-musl)
apk add --update shadow sudo build-base curl-dev openssl-dev expat-dev gettext \
pcre2-dev python3 musl-libintl perl-utils ncurses \
- apache2 bash cvs gnupg perl-cgi perl-dbd-sqlite >/dev/null
+ apache2 apache2-http2 apache2-proxy apache2-ssl apache2-webdav \
+ bash cvs gnupg perl-cgi perl-dbd-sqlite >/dev/null
;;
linux-*)
apt update -q &&
But once fixed, tests do indeed start to fail:
t5540-http-push-webdav.sh (Wstat: 256 (exited 1) Tests: 20 Failed: 11)
Failed tests: 5-11, 13, 15-16, 18
Non-zero exit status: 1
Seems like another thing to fix in a separate patch series.
Patrick
> ;;
> pedantic)
> dnf -yq update >/dev/null &&
> diff --git a/t/lib-httpd.sh b/t/lib-httpd.sh
> index 2fb1b2ae561..9791f94b16f 100644
> --- a/t/lib-httpd.sh
> +++ b/t/lib-httpd.sh
> @@ -67,7 +67,8 @@ for DEFAULT_HTTPD_MODULE_PATH in '/usr/libexec/apache2' \
> '/usr/lib/apache2/modules' \
> '/usr/lib64/httpd/modules' \
> '/usr/lib/httpd/modules' \
> - '/usr/libexec/httpd'
> + '/usr/libexec/httpd' \
> + '/usr/lib/apache2'
> do
> if test -d "$DEFAULT_HTTPD_MODULE_PATH"
> then
> --
> 2.42.0
>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* Re: [PATCH v3 7/8] ci: install test dependencies for linux-musl
From: Patrick Steinhardt @ 2023-10-30 13:22 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Phillip Wood, Oswald Buddenhagen
In-Reply-To: <ZT-l4qDDNloizjxl@tanuki>
[-- Attachment #1: Type: text/plain, Size: 5019 bytes --]
On Mon, Oct 30, 2023 at 01:47:30PM +0100, Patrick Steinhardt wrote:
> On Mon, Oct 30, 2023 at 01:15:10PM +0100, Patrick Steinhardt wrote:
> > The linux-musl CI job executes tests on Alpine Linux, which is based on
> > musl libc instead of glibc. We're missing some test dependencies though,
> > which causes us to skip a subset of tests.
> >
> > Install these test dependencies to increase our test coverage on this
> > platform. There are still some missing test dependecies, but these do
> > not have a corresponding package in the Alpine repositories:
> >
> > - p4 and p4d, both parts of the Perforce version control system.
> >
> > - cvsps, which generates patch sets for CVS.
> >
> > - Subversion and the SVN::Core Perl library, the latter of which is
> > not available in the Alpine repositories. While the tool itself is
> > available, all Subversion-related tests are skipped without the
> > SVN::Core Perl library anyway.
> >
> > Furthermore, in order to make the Apache-based tests work, this commit
> > also adds the Alpine-specific modules path of it to the list of known
> > paths.
> >
> > Signed-off-by: Patrick Steinhardt <ps@pks.im>
> > ---
> > ci/install-docker-dependencies.sh | 3 ++-
> > t/lib-httpd.sh | 3 ++-
> > 2 files changed, 4 insertions(+), 2 deletions(-)
> >
> > diff --git a/ci/install-docker-dependencies.sh b/ci/install-docker-dependencies.sh
> > index d0bc19d3bb3..05dde5c5d40 100755
> > --- a/ci/install-docker-dependencies.sh
> > +++ b/ci/install-docker-dependencies.sh
> > @@ -17,7 +17,8 @@ linux32)
> > ;;
> > linux-musl)
> > apk add --update build-base curl-dev openssl-dev expat-dev gettext \
> > - pcre2-dev python3 musl-libintl perl-utils ncurses >/dev/null
> > + pcre2-dev python3 musl-libintl perl-utils ncurses \
> > + apache2 bash cvs gnupg perl-cgi perl-dbd-sqlite >/dev/null
>
> Meh. I just noticed that I missed a few other dependencies to make
> Apache2 work:
>
> diff --git a/ci/install-docker-dependencies.sh b/ci/install-docker-dependencies.sh
> index 5e28adf55b..ce910e3f3c 100755
> --- a/ci/install-docker-dependencies.sh
> +++ b/ci/install-docker-dependencies.sh
> @@ -21,7 +21,8 @@ linux32)
> linux-musl)
> apk add --update shadow sudo build-base curl-dev openssl-dev expat-dev gettext \
> pcre2-dev python3 musl-libintl perl-utils ncurses \
> - apache2 bash cvs gnupg perl-cgi perl-dbd-sqlite >/dev/null
> + apache2 apache2-http2 apache2-proxy apache2-ssl apache2-webdav \
> + bash cvs gnupg perl-cgi perl-dbd-sqlite >/dev/null
> ;;
> linux-*)
> apt update -q &&
>
> But once fixed, tests do indeed start to fail:
>
> t5540-http-push-webdav.sh (Wstat: 256 (exited 1) Tests: 20 Failed: 11)
> Failed tests: 5-11, 13, 15-16, 18
> Non-zero exit status: 1
>
> Seems like another thing to fix in a separate patch series.
>
> Patrick
I've been digging a bit, and the issue comes from the the DAV module
indeed:
```
[Mon Oct 30 12:36:19.776149 2023] [dav_fs:crit] [pid 275752] (20019)DSO load failed: AH00576: The DBM driver could not be loaded
[Mon Oct 30 12:36:19.776168 2023] [dav:error] [pid 275752] [client 127.0.0.1:51388] Could not LOCK /dumb/test_repo.git/info/refs due to a failed precondition (e.g. other locks). [500, #0]
[Mon Oct 30 12:36:19.776174 2023] [dav:error] [pid 275752] [client 127.0.0.1:51388] The locks could not be queried for verification against a possible "If:" header. [500, #0]
[Mon Oct 30 12:36:19.776177 2023] [dav:error] [pid 275752] [client 127.0.0.1:51388] Could not open the lock database. [500, #400]
[Mon Oct 30 12:36:19.776181 2023] [dav:error] [pid 275752] (20019)DSO load failed: [client 127.0.0.1:51388] Could not open property database. [500, #1]
```
This seems to be a known limitation in Alpine Linux as they do not
package apr-util-dbm_db anymore due to license incompatibilities with
with Berkely DB [1], and the WebDAV module does rely on it to provide
locking.
In the best case we'd be able to detect this limitation and skip those
tests automatically so that we can at least execute all the other Apache
tests. But again, this rather feels like something we should do as a
follow up rather than as part of this series.
Patrick
[1]: https://gitlab.alpinelinux.org/alpine/aports/-/issues/12534
> > ;;
> > pedantic)
> > dnf -yq update >/dev/null &&
> > diff --git a/t/lib-httpd.sh b/t/lib-httpd.sh
> > index 2fb1b2ae561..9791f94b16f 100644
> > --- a/t/lib-httpd.sh
> > +++ b/t/lib-httpd.sh
> > @@ -67,7 +67,8 @@ for DEFAULT_HTTPD_MODULE_PATH in '/usr/libexec/apache2' \
> > '/usr/lib/apache2/modules' \
> > '/usr/lib64/httpd/modules' \
> > '/usr/lib/httpd/modules' \
> > - '/usr/libexec/httpd'
> > + '/usr/libexec/httpd' \
> > + '/usr/lib/apache2'
> > do
> > if test -d "$DEFAULT_HTTPD_MODULE_PATH"
> > then
> > --
> > 2.42.0
> >
>
>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH 5/5] ci: add support for GitLab CI
From: Dragan Simic @ 2023-10-30 14:04 UTC (permalink / raw)
To: phillip.wood; +Cc: Oswald Buddenhagen, Patrick Steinhardt, git
In-Reply-To: <68c4684b-0ab0-427a-90e5-fca602c9d993@gmail.com>
On 2023-10-30 10:49, Phillip Wood wrote:
> On 27/10/2023 18:47, Oswald Buddenhagen wrote:
>> On Fri, Oct 27, 2023 at 03:32:48PM +0100, Phillip Wood wrote:
>>> On 27/10/2023 11:43, Oswald Buddenhagen wrote:
>>>> On Fri, Oct 27, 2023 at 11:22:35AM +0100, Phillip Wood wrote:
>>>>>>>> + CI_BRANCH="$CI_COMMIT_REF_NAME"
>>>>>>>> + CI_COMMIT="$CI_COMMIT_SHA"
>>>>>>>>
>>>>>>> assignments need no quoting to prevent word splitting.
>>>>>>> repeats below.
>>>>>>>
>>>>> I think it is quite common for us to quote variables when it isn't
>>>>> strictly necessary as it makes it clear to anyone reading the
>>>>> script that there is no word splitting going on
>>>>
>>>>> and ensures that we don't start splitting the variable if the
>>>>> contents changes in the future.
>>>>>
>>>> the point was that it *isn't* content-dependent; it's simply the
>>>> shell rules.
>>>
>>> Oh, I'd misunderstood what you were saying which was that assignment
>>> and case statements are not subject to field splitting.
>>>
>>>> of course, many people (apparently you included) don't know these
>>>> subtleties
>>>
>>> I find this comment to be condescending, needlessly antagonistic and
>>> completely at odds with the norms of constructive discussion on this
>>> list.
>>>
>> the observation was necessary for the point i subsequently made (which
>> was basically agreeing with the first part of your response).
>
> It was not necessary to phrase it as you did though. Before replying
> on Friday I showed your comment to someone else and their reaction was
> "That's rude". You could have made your point by saying something like
>
> It is hard to remember all the shell's word splitting rules so
> quoting everywhere is not a bad idea.
>
> This is not the first time I've found your comments unnecessarily
> adversarial and at odds with the norms of constructive discussion and
> respectful disagreement on this list. I don't think I'm the only one
> either - in [1] Junio points out an ad-hominem remark and in [2] Marc
> comments on the unreceptive tone of you review responses.
>
> I would urge you to try and strike a more conciliatory tone in your
> messages - it is perfectly possible to correct or disagree with
> someone without alienating them in the process.
Yeah, I've also noticed a not so great tone is some of Oswald's
messages. It's perfectly fine to disagree on something, but it isn't
that great to put the other party down while doing that, even to the
point of insulting them.
We're all humans, and we should treat each other with respect.
Furthermore, disagreeing in a friendly and polite way can many times
lead to finding a better solution together.
> Best Wishes
>
> Phillip
>
> [1] https://lore.kernel.org/git/xmqqleeihok5.fsf@gitster.g/
> [2]
> https://lore.kernel.org/git/e33f919d-1b6a-4944-ab5d-93ad0d323b68@xiplink.com/
^ permalink raw reply
* Re: [PATCH v3] bugreport: reject positional arguments
From: Phillip Wood @ 2023-10-30 14:11 UTC (permalink / raw)
To: Junio C Hamano
Cc: Eric Sunshine, emilyshaffer, git, Emily Shaffer, Sheik,
Dragan Simic
In-Reply-To: <xmqqcywwg9am.fsf@gitster.g>
On 30/10/2023 01:59, Junio C Hamano wrote:
> Junio C Hamano <gitster@pobox.com> writes:
>
>> Junio C Hamano <gitster@pobox.com> writes:
>>
>>> Phillip Wood <phillip.wood123@gmail.com> writes:
>>>
>>>> It is rather unfortunate that test_i18ngrep was deprecated without
>>>> providing an alternative that offers the same debugging
>>>> experience.
>>> ...
>>> We could rename test_i18ngrep to test_grep (and make test_i18ngrep
>>> into a thin wrapper with warnings).
>>>
>>> test_grep -e must-exist file &&
>>> test_grep ! -e must-not-exist file
>>
>> ... as the only remaining part in test_18ngrep has no hack to work
>> around the tainted localization tests, so "was deprecated without"
>> is a bit too strong. There is nothing we have lost yet.
>
> Having said all that, when re-reading the test_i18ngrep with a fresh
> pair of eyes, I somehow doubt there was much upside in "debugging
> experience" with test_i18ngrep in the first place, and I doubt if
> retaining it with a new name test_grep has much value.
>
> Given that test_i18ngrep (hence test_grep) requires you to have the
> haystack in a file, between
>
> test_i18ngrep must-exist file &&
> test_i18ngrep ! must-not-exist file
>
> and
>
> grep must-exist file &&
> ! grep must-not-exist file
>
> I do not see any difference in "debugging experience" when you run
> the test with "-i [-v] -d". The two cases you care about are
>
> (1) the test expects the string "must-exist" in the file "file" but
> the string is not there.
>
> (2) the test expects the string "must-not-exist" missing from the
> file "file", but the string is there.
>
> The latter can clearly be seen in output from "-i -v -d" (the "grep"
> outputs a line with "must-not-exist" on it). The former will show
> silence but since you are debugging with "-d", and your haystack is
> in a file, after such a step fails, the test stops, and without
> removing the "file" even if the test piece had test_when_finished
> to remove it (i.e. running tests in debugging mode "-d" and
> immediately stopping upon failure "-i" behaves this way exactly to
> help you debugging), so you can go there to the TRASH_DIRECTORY
> yourself and inspect "file" to see what is going on anyway.
>
> So, I dunno. Surely with a long &&-chain of steps, where a grep
> that expects lack of something is in the middle, it is hard to see
> if the lack of hit is because an earlier step failed (and the
> control did not reach "grep must-exist file") or because the
> haystack lacked the "must-exist" needle, so from that point of view,
> it may be nicer that "did not find an expected match" is explicitly
> stated.
It is this latter point that I had in mind. I find it much easier to
debug a test that says "This command failed" rather than looking at the
output to try and figure out which was the last successful command. I
take your point above that one can go and inspect the file when the test
is run with "-i -d" but it is determining that grep failed in the first
place that I find annoying. I've also found the output from
test_i18ngrep is helpful when debugging CI test failures.
Best Wishes
Phillip
^ permalink raw reply
* Re: [PATCH v3 5/8] ci: unify setup of some environment variables
From: Phillip Wood @ 2023-10-30 15:09 UTC (permalink / raw)
To: Patrick Steinhardt, git; +Cc: Junio C Hamano, Oswald Buddenhagen
In-Reply-To: <6af0075fd875f176e7fdf6c219e7117dac5cd71c.1698667545.git.ps@pks.im>
Hi Patrick
On 30/10/2023 12:15, Patrick Steinhardt wrote:
> Both GitHub Actions and Azue Pipelines set up the environment variables
> GIT_TEST_OPTS, GIT_PROVE_OPTS and MAKEFLAGS. And while most values are
> actually the same, the setup is completely duplicate. With the upcoming
> support for GitLab CI this duplication would only extend even further.
>
> Unify the setup of those environment variables so that only the uncommon
> parts are separated. While at it, we also perform some additional small
> improvements:
>
> - We use nproc instead of a hardcoded count of jobs for make and
> prove. This ensures that the number of concurrent processes adapts
> to the host automatically.
Sadly this makes the Windows and MacOS jobs fail on GitHub Actions as
nproc is not installed[1]. Perhaps we could do
--jobs="$(nproc || echo 2)"
instead. (Maybe 2 is a bit low but the current value of 10 seems pretty
high for the number of cores on the runners that we use)
> - We now always pass `--state=failed,slow,save` via GIT_PROVE_OPTS.
> It doesn't hurt on platforms where we don't persist the state, so
> this further reduces boilerplate.
>
> - When running on Windows systems we set `--no-chain-lint` and
> `--no-bin-wrappers`. Interestingly though, we did so _after_
> already having exported the respective environment variables.
> > - We stop using `export VAR=value` syntax, which is a Bashism.
It's
> not quite worth it as we still use this syntax all over the place,
> but it doesn't hurt readability either.
I don't mind this change, but the 'export VAR=value' syntax is in POSIX[2]
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
> ci/lib.sh | 24 ++++++++++++++----------
> 1 file changed, 14 insertions(+), 10 deletions(-)
>
> diff --git a/ci/lib.sh b/ci/lib.sh
> index 9ffdf743903..c7a716a6e3f 100755
> --- a/ci/lib.sh
> +++ b/ci/lib.sh
> @@ -175,11 +175,7 @@ then
> # among *all* phases)
> cache_dir="$HOME/test-cache/$SYSTEM_PHASENAME"
>
> - export GIT_PROVE_OPTS="--timer --jobs 10 --state=failed,slow,save"
> - export GIT_TEST_OPTS="--verbose-log -x --write-junit-xml"
> - MAKEFLAGS="$MAKEFLAGS --jobs=10"
> - test windows_nt != "$CI_OS_NAME" ||
> - GIT_TEST_OPTS="--no-chain-lint --no-bin-wrappers $GIT_TEST_OPTS"
> + GIT_TEST_OPTS="--write-junit-xml"
> elif test true = "$GITHUB_ACTIONS"
> then
> CI_TYPE=github-actions
> @@ -198,17 +194,25 @@ then
>
> cache_dir="$HOME/none"
>
> - export GIT_PROVE_OPTS="--timer --jobs 10"
> - export GIT_TEST_OPTS="--verbose-log -x --github-workflow-markup"
> - MAKEFLAGS="$MAKEFLAGS --jobs=10"
> - test windows != "$CI_OS_NAME" ||
> - GIT_TEST_OPTS="--no-chain-lint --no-bin-wrappers $GIT_TEST_OPTS"
> + GIT_TEST_OPTS="--github-workflow-markup"
> else
> echo "Could not identify CI type" >&2
> env >&2
> exit 1
> fi
>
> +MAKEFLAGS="$MAKEFLAGS --jobs=$(nproc)"
> +GIT_PROVE_OPTS="--timer --jobs $(nproc) --state=failed,slow,save"
> +
> +GIT_TEST_OPTS="$GIT_TEST_OPTS --verbose-log -x"
> +if test windows = "$CI_OS_NAME"
> +then
> + GIT_TEST_OPTS="$GIT_TEST_OPTS --no-chain-lint --no-bin-wrappers"
> +fi
>
> +export GIT_TEST_OPTS
> +export GIT_PROVE_OPTS
I was wondering why we don't export MAKEFLAGS here but it is exported
earlier on before we set it. Apart from the nproc issue this looks like
a nice improvement
Best Wishes
Phillip
[1] https://github.com/phillipwood/git/actions/runs/6694263874
[2]
https://pubs.opengroup.org/onlinepubs/9699919799/utilities/V3_chap02.html#export
^ permalink raw reply
* Re: [PATCH v3 7/8] ci: install test dependencies for linux-musl
From: Phillip Wood @ 2023-10-30 15:13 UTC (permalink / raw)
To: Patrick Steinhardt, git; +Cc: Junio C Hamano, Oswald Buddenhagen
In-Reply-To: <ZT-l4qDDNloizjxl@tanuki>
Hi Patrick
On 30/10/2023 12:47, Patrick Steinhardt wrote:
> On Mon, Oct 30, 2023 at 01:15:10PM +0100, Patrick Steinhardt wrote:
> But once fixed, tests do indeed start to fail:
>
> t5540-http-push-webdav.sh (Wstat: 256 (exited 1) Tests: 20 Failed: 11)
> Failed tests: 5-11, 13, 15-16, 18
> Non-zero exit status: 1
>
> Seems like another thing to fix in a separate patch series.
Yes, or we could just leave it - I had not realized before that it was
only the musl job that was not running the httpd tests (I thought
install-docker-dependencies.sh was missing the packages for ubuntu as
well). Given that, the status quo does not seem so bad.
Best Wishes
Phillip
^ permalink raw reply
* Re: [PATCH v3 5/8] ci: unify setup of some environment variables
From: Patrick Steinhardt @ 2023-10-30 15:19 UTC (permalink / raw)
To: phillip.wood; +Cc: git, Junio C Hamano, Oswald Buddenhagen
In-Reply-To: <87430c6c-91c0-4be1-b89d-bf442b3f018b@gmail.com>
[-- Attachment #1: Type: text/plain, Size: 4391 bytes --]
On Mon, Oct 30, 2023 at 03:09:01PM +0000, Phillip Wood wrote:
> Hi Patrick
>
> On 30/10/2023 12:15, Patrick Steinhardt wrote:
> > Both GitHub Actions and Azue Pipelines set up the environment variables
> > GIT_TEST_OPTS, GIT_PROVE_OPTS and MAKEFLAGS. And while most values are
> > actually the same, the setup is completely duplicate. With the upcoming
> > support for GitLab CI this duplication would only extend even further.
> >
> > Unify the setup of those environment variables so that only the uncommon
> > parts are separated. While at it, we also perform some additional small
> > improvements:
> >
> > - We use nproc instead of a hardcoded count of jobs for make and
> > prove. This ensures that the number of concurrent processes adapts
> > to the host automatically.
>
> Sadly this makes the Windows and MacOS jobs fail on GitHub Actions as nproc
> is not installed[1]. Perhaps we could do
>
> --jobs="$(nproc || echo 2)"
>
> instead. (Maybe 2 is a bit low but the current value of 10 seems pretty high
> for the number of cores on the runners that we use)
Ugh, thanks. I'll update it to keep the hardcoded 10 jobs in place for
now for the other pipelines. Trying to do too many things at once is
only going to make it harder to get this landed, doubly so when you have
no easy way to verify what you're doing :)
> > - We now always pass `--state=failed,slow,save` via GIT_PROVE_OPTS.
> > It doesn't hurt on platforms where we don't persist the state, so
> > this further reduces boilerplate.
> >
> > - When running on Windows systems we set `--no-chain-lint` and
> > `--no-bin-wrappers`. Interestingly though, we did so _after_
> > already having exported the respective environment variables.
> > > - We stop using `export VAR=value` syntax, which is a Bashism. It's
> > not quite worth it as we still use this syntax all over the place,
> > but it doesn't hurt readability either.
>
> I don't mind this change, but the 'export VAR=value' syntax is in POSIX[2]
I don't quite mind it, either. The reason why I chose to use it is that
there was indeed a bug already with the order of exports and
modification of the vars after the export. So with that in mind it made
sense to me to adapt it accordingly.
> > Signed-off-by: Patrick Steinhardt <ps@pks.im>
> > ---
> > ci/lib.sh | 24 ++++++++++++++----------
> > 1 file changed, 14 insertions(+), 10 deletions(-)
> >
> > diff --git a/ci/lib.sh b/ci/lib.sh
> > index 9ffdf743903..c7a716a6e3f 100755
> > --- a/ci/lib.sh
> > +++ b/ci/lib.sh
> > @@ -175,11 +175,7 @@ then
> > # among *all* phases)
> > cache_dir="$HOME/test-cache/$SYSTEM_PHASENAME"
> > - export GIT_PROVE_OPTS="--timer --jobs 10 --state=failed,slow,save"
> > - export GIT_TEST_OPTS="--verbose-log -x --write-junit-xml"
> > - MAKEFLAGS="$MAKEFLAGS --jobs=10"
> > - test windows_nt != "$CI_OS_NAME" ||
> > - GIT_TEST_OPTS="--no-chain-lint --no-bin-wrappers $GIT_TEST_OPTS"
> > + GIT_TEST_OPTS="--write-junit-xml"
> > elif test true = "$GITHUB_ACTIONS"
> > then
> > CI_TYPE=github-actions
> > @@ -198,17 +194,25 @@ then
> > cache_dir="$HOME/none"
> > - export GIT_PROVE_OPTS="--timer --jobs 10"
> > - export GIT_TEST_OPTS="--verbose-log -x --github-workflow-markup"
> > - MAKEFLAGS="$MAKEFLAGS --jobs=10"
> > - test windows != "$CI_OS_NAME" ||
> > - GIT_TEST_OPTS="--no-chain-lint --no-bin-wrappers $GIT_TEST_OPTS"
> > + GIT_TEST_OPTS="--github-workflow-markup"
> > else
> > echo "Could not identify CI type" >&2
> > env >&2
> > exit 1
> > fi
> > +MAKEFLAGS="$MAKEFLAGS --jobs=$(nproc)"
> > +GIT_PROVE_OPTS="--timer --jobs $(nproc) --state=failed,slow,save"
> > +
> > +GIT_TEST_OPTS="$GIT_TEST_OPTS --verbose-log -x"
> > +if test windows = "$CI_OS_NAME"
> > +then
> > + GIT_TEST_OPTS="$GIT_TEST_OPTS --no-chain-lint --no-bin-wrappers"
> > +fi
> >
> > +export GIT_TEST_OPTS
> > +export GIT_PROVE_OPTS
>
> I was wondering why we don't export MAKEFLAGS here but it is exported
> earlier on before we set it. Apart from the nproc issue this looks like a
> nice improvement
In fact, I also noticed later today that we adapt MAKEFLAGS at a later
point again without reexporting it, so it's got the same issue. I'll
adapt it in the same way.
Patrick
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH v3 7/8] ci: install test dependencies for linux-musl
From: Patrick Steinhardt @ 2023-10-30 15:23 UTC (permalink / raw)
To: phillip.wood; +Cc: git, Junio C Hamano, Oswald Buddenhagen
In-Reply-To: <d675b51b-c50b-43e4-b127-650c0e24d3d0@gmail.com>
[-- Attachment #1: Type: text/plain, Size: 2737 bytes --]
On Mon, Oct 30, 2023 at 03:13:35PM +0000, Phillip Wood wrote:
> Hi Patrick
>
> On 30/10/2023 12:47, Patrick Steinhardt wrote:
> > On Mon, Oct 30, 2023 at 01:15:10PM +0100, Patrick Steinhardt wrote:
> > But once fixed, tests do indeed start to fail:
> >
> > t5540-http-push-webdav.sh (Wstat: 256 (exited 1) Tests: 20 Failed: 11)
> > Failed tests: 5-11, 13, 15-16, 18
> > Non-zero exit status: 1
> >
> > Seems like another thing to fix in a separate patch series.
>
> Yes, or we could just leave it - I had not realized before that it was only
> the musl job that was not running the httpd tests (I thought
> install-docker-dependencies.sh was missing the packages for ubuntu as well).
> Given that, the status quo does not seem so bad.
I of course couldn't let go. The following would fix this:
diff --git a/ci/install-docker-dependencies.sh b/ci/install-docker-dependencies.sh
index 5e28adf55b6..48cb2e735b5 100755
--- a/ci/install-docker-dependencies.sh
+++ b/ci/install-docker-dependencies.sh
@@ -21,7 +21,8 @@ linux32)
linux-musl)
apk add --update shadow sudo build-base curl-dev openssl-dev expat-dev gettext \
pcre2-dev python3 musl-libintl perl-utils ncurses \
- apache2 bash cvs gnupg perl-cgi perl-dbd-sqlite >/dev/null
+ apache2 apache2-http2 apache2-proxy apache2-ssl apache2-webdav apr-util-dbd_sqlite3 \
+ bash cvs gnupg perl-cgi perl-dbd-sqlite >/dev/null
;;
linux-*)
apt update -q &&
diff --git a/t/lib-httpd.sh b/t/lib-httpd.sh
index 9791f94b16f..9ea74927c40 100644
--- a/t/lib-httpd.sh
+++ b/t/lib-httpd.sh
@@ -128,6 +128,20 @@ else
"Could not identify web server at '$LIB_HTTPD_PATH'"
fi
+if test -n "$LIB_HTTPD_DAV" && test -f /etc/os-release
+then
+ case "$(grep "^ID=" /etc/os-release | cut -d= -f2-)" in
+ alpine)
+ # The WebDAV module in Alpine Linux is broken at least up to
+ # Alpine v3.16 as the default DBM driver is missing.
+ #
+ # https://gitlab.alpinelinux.org/alpine/aports/-/issues/13112
+ test_skip_or_die GIT_TEST_HTTPD \
+ "Apache WebDAV module does not have default DBM backend driver"
+ ;;
+ esac
+fi
+
install_script () {
write_script "$HTTPD_ROOT_PATH/$1" <"$TEST_PATH/$1"
}
I might as well roll it into this patch series now. It increases test
coverage on musl libc and doesn't have any significant downsides. In
fact, it uncovers that tests on Alpine Linux don't work right now, so it
fixes real issues to have the test coverage in our pipelines.
Patrick
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* Re: Repository cloned using SSH does not respect bare repository initial branch
From: Sheik @ 2023-10-30 15:24 UTC (permalink / raw)
To: Jeff King; +Cc: git
In-Reply-To: <20231030093605.GE84866@coredump.intra.peff.net>
Server version is same as client (v2.42.0) as I ran these commands all
on the same machine.
Test on ssh server:
$ git -v --build-options
#Output
git version 2.42.0
cpu: x86_64
no commit associated with this build
sizeof-long: 8
sizeof-size_t: 8
shell-path: /bin/sh
Thanks
Sheik
On 30/10/23 20:36, Jeff King wrote:
> On Thu, Oct 26, 2023 at 07:36:36AM +1100, Sheik wrote:
>
>> Repository cloned using SSH does not use the branch configured in the bare
>> repository however repository cloned using filesystem does as expected.
>> Shouldn't they both behave the same?
> What version of Git is running on the ssh server?
>
> Your example seems to show that the parent repository has an unborn
> branch (i.e., HEAD points to "refs/heads/test", but there are no commits
> yet). I think the server-side bits you need for that to work showed up
> in 59e1205d16 (ls-refs: report unborn targets of symrefs, 2021-02-05),
> which is in v2.31.
>
> So even though your client seems to be v2.42 (from the output you gave),
> if the server is older it may not be sending sufficient information.
> There were also some other fixes on top of that, but I _think_ they were
> all client-side (so your v2.42 clone command should be doing the right
> thing).
>
> -Peff
^ permalink raw reply
* Re: Repository cloned using SSH does not respect bare repository initial branch
From: Sheik @ 2023-10-30 15:33 UTC (permalink / raw)
To: Jeff King; +Cc: git
In-Reply-To: <20231030093605.GE84866@coredump.intra.peff.net>
On 30/10/23 20:36, Jeff King wrote:
> On Thu, Oct 26, 2023 at 07:36:36AM +1100, Sheik wrote:
>
>> Repository cloned using SSH does not use the branch configured in the bare
>> repository however repository cloned using filesystem does as expected.
>> Shouldn't they both behave the same?
> What version of Git is running on the ssh server?
>
> Your example seems to show that the parent repository has an unborn
> branch (i.e., HEAD points to "refs/heads/test", but there are no commits
> yet). I think the server-side bits you need for that to work showed up
> in 59e1205d16 (ls-refs: report unborn targets of symrefs, 2021-02-05),
> which is in v2.31.
>
> So even though your client seems to be v2.42 (from the output you gave),
> if the server is older it may not be sending sufficient information.
> There were also some other fixes on top of that, but I _think_ they were
> all client-side (so your v2.42 clone command should be doing the right
> thing).
>
> -Peff
Confirming your observation above, yes this is a purely new bare
repository which has no commits yet before cloning.
Thanks
Sheik
^ permalink raw reply
* [PATCH] tests: handle "funny" exit code 127 produced by MSVC-compiled exes
From: Johannes Schindelin via GitGitGadget @ 2023-10-30 15:45 UTC (permalink / raw)
To: git; +Cc: Johannes Schindelin, Johannes Schindelin
From: Johannes Schindelin <johannes.schindelin@gmx.de>
The exit code 127 is well-documented to mean: command not found.
Unfortunately, it is also used as fall-back in Cygwin's
`pinfo::status_exit()` method (which maps things like Windows'
`STATUS_ACCESS_VIOLATION` to `128 | SIGSEGV`).
This is particularly unfortunate because there is no explicit mapping
for `STATUS_STACK_OVERFLOW`. Meaning: when MSVC-compiled executables
produce a stack overflow the exit code in the Cygwin Bash will be 127.
Consequently, the same will be true for the MSYS2 Bash that is used by
Git for Windows.
Now, `jk/tree-name-and-depth-limit` introduces a pair of test cases that
expect a command that produces a stack overflow to fail, which it
typically does with exit code 139 (which means SIGSEGV).
But since MSVC-compiled `git.exe` exits with `STATUS_STACK_OVERFLOW`
which the MSYS2 runtime maps to 127, and since 127 is taken to mean
"command not found" by `test_must_fail`, even though everything works as
planned the two new test cases fail when run in `win+VS test`.
Let's work around this by:
1) recording which C compiler was used, and
2) adding an MSVC-only exception to `test_must_fail` to treat 127 as a
regular failure.
There is a slight downside of this approach in that a real missing
command could be mistaken for a failure. However, this would be caught
on other platforms, and besides, we use `test_must_fail` only for `git`
and `scalar` anymore, and we can be pretty certain that both are there.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
Fix t6700.[45] in win+VS test
These two test cases have been failing for a while in Git for Windows'
shears/* branches. Took a good while to figure out, too.
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1604%2Fdscho%2Ffix-vs-win-test-with-new-depth-limit-test-cases-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1604/dscho/fix-vs-win-test-with-new-depth-limit-test-cases-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1604
contrib/buildsystems/CMakeLists.txt | 3 ++-
t/test-lib-functions.sh | 3 +++
2 files changed, 5 insertions(+), 1 deletion(-)
diff --git a/contrib/buildsystems/CMakeLists.txt b/contrib/buildsystems/CMakeLists.txt
index 6b819e2fbdf..e164484be98 100644
--- a/contrib/buildsystems/CMakeLists.txt
+++ b/contrib/buildsystems/CMakeLists.txt
@@ -1057,7 +1057,8 @@ if(NOT PYTHON_TESTS)
set(NO_PYTHON 1)
endif()
-file(WRITE ${CMAKE_BINARY_DIR}/GIT-BUILD-OPTIONS "SHELL_PATH='${SHELL_PATH}'\n")
+file(WRITE ${CMAKE_BINARY_DIR}/GIT-BUILD-OPTIONS "CMAKE_C_COMPILER='${CMAKE_C_COMPILER}'\n")
+file(APPEND ${CMAKE_BINARY_DIR}/GIT-BUILD-OPTIONS "SHELL_PATH='${SHELL_PATH}'\n")
file(APPEND ${CMAKE_BINARY_DIR}/GIT-BUILD-OPTIONS "TEST_SHELL_PATH='${TEST_SHELL_PATH}'\n")
file(APPEND ${CMAKE_BINARY_DIR}/GIT-BUILD-OPTIONS "PERL_PATH='${PERL_PATH}'\n")
file(APPEND ${CMAKE_BINARY_DIR}/GIT-BUILD-OPTIONS "DIFF='${DIFF}'\n")
diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index 2f8868caa17..ee19c748973 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -1112,6 +1112,9 @@ test_must_fail () {
return 1
elif test $exit_code -eq 127
then
+ # Work-around for MSVC-compiled executables
+ case "$CMAKE_C_COMPILER" in *MSVC*) return 0;; esac
+
echo >&4 "test_must_fail: command not found: $*"
return 1
elif test $exit_code -eq 126
base-commit: 3130c155df9a65ebccf128b4af5a19af49532580
--
gitgitgadget
^ permalink raw reply related
* Re: [PATCH 0/5] ci: add GitLab CI definition
From: Taylor Blau @ 2023-10-30 15:46 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git
In-Reply-To: <cover.1698305961.git.ps@pks.im>
On Thu, Oct 26, 2023 at 09:59:59AM +0200, Patrick Steinhardt wrote:
> And this is exactly what this patch series does: it adds GitLab-specific
> knowledge to our CI scripts and adds a CI definition that builds on top
> of those scripts. This is rather straight forward, as the scripts
> already know to discern Azure Pipelines and GitHub Actions, and adding
> a third item to this list feels quite natural. And by building on top of
> the preexisting infra, the actual ".gitlab-ci.yml" is really quite
> small.
>
> I acknowledge that the Git project may not be willing to fully support
> GitLab CI, and that's fine with me. If we want to further stress that
> point then I'd also be perfectly happy to move the definitions into the
> "contrib/" directory -- it would still be a huge win for our workflow.
> In any case, I'm happy to keep on maintaining the intgeration with
> GitLab CI, and if things break I'll do my best to fix them fast.
I don't have any strong opinions here, but my preference would probably
be to keep any GitLab-specific CI configuration limited to "contrib", if
it lands in the tree at all.
We already have a rather complicated CI setup on GitHub, which I think
we generally consider authoritative in terms of determining whether "CI"
is green. I know we have some Azure remnants in "ci", but I'm not aware
of any of the details there.
So I have some hesitation about trying to mirror this rather complicated
set of build rules in another CI environment. My primary concern would
be that the two might fall out of sync and a series that is green on
GitHub would be red on GitLab, or vice-versa. Importantly, this can
happen even without changes to the build definitions, since (AFAICT)
both forges distribute new images automatically, so the set of packages
installed in GitHub may not exactly match what's in GitLab (and
vice-versa).
My other concern is that we're doubling the cost of any new changes to
our CI definition. Perhaps this is more of an academic concern, but I
think my fear would be that one or the other would fall behind on in
implementation leading to further divergence between the two.
I think having the new CI definition live in "contrib" somewhat
addresses the "which CI is authoritative?" problem, but that it doesn't
address the "we have two of these" problem.
So my preference would probably to have this live out of Junio's tree,
but I'm curious to hear what others think.
Thanks,
Taylor
^ permalink raw reply
* Re: [PATCH 0/1] Object ID support for git merge-file
From: Taylor Blau @ 2023-10-30 15:54 UTC (permalink / raw)
To: Elijah Newren; +Cc: brian m. carlson, git, Junio C Hamano, Phillip Wood
In-Reply-To: <CABPp-BFJn5NsSaaxEMpicqND_-8CBw370kyBQuKHGOX-3PRyMw@mail.gmail.com>
On Sat, Oct 28, 2023 at 11:24:06PM -0700, Elijah Newren wrote:
> On Tue, Oct 24, 2023 at 12:58 PM brian m. carlson
> <sandals@crustytoothpaste.net> wrote:
> >
> > This series introduces an --object-id option to git merge-file such
> > that, instead of reading and writing from files on the system, it reads
> > from and writes to the object store using blobs.
>
> This seems like a reasonable capability to want from such a plumbing command.
Agreed.
> > This is in use at
> > GitHub to produce conflict diffs when a merge fails, and it seems
> > generally useful, so I'm sending it here.
>
> But...wouldn't you already have the conflicts generated when doing the
> merge and learning that it fails? Why would you need to generate them
> again?
brian would know better than I do, but I believe the reason is because
the "attempt this merge" RPC is handled separately from the "show me the
merge conflict(s) at xyz path". Those probably could be combined
(obviating the need for this patch), but doing so is probably rather
complicated.
Since this feature is generally useful for callers that haven't already
completed a tree-level merge and really just care about the result of
merging a single path, I don't have any objections here.
Thanks,
Taylor
^ permalink raw reply
* Re: please add link / url to remote - when - git push
From: Taylor Blau @ 2023-10-30 15:55 UTC (permalink / raw)
To: Alexander Mills; +Cc: git
In-Reply-To: <CA+KyZp5mwGJ6YOvjKtfnDMDb9ci3vSq5KNUep6-8EfkHNaxREg@mail.gmail.com>
Hi Alexander,
On Sun, Oct 29, 2023 at 06:15:35PM -0500, Alexander Mills wrote:
> Having the link in the console saves me tremendous time and is
> extremely effective/efficient. Can we get links in the console plz?
That link is generated at the remote end (in your case, this is on
GitHub) and then sent over the wire before being printed out with the
"remote:" prefix at your terminal.
The Git project does not itself generate these messages, so you may want
to relay your request to GitHub's support address at <support@github.com>.
Thanks,
Taylor
^ permalink raw reply
* Re: [PATCH v3 7/8] ci: install test dependencies for linux-musl
From: Phillip Wood @ 2023-10-30 16:09 UTC (permalink / raw)
To: Patrick Steinhardt, phillip.wood; +Cc: git, Junio C Hamano, Oswald Buddenhagen
In-Reply-To: <ZT_KWjxkPEIXHEeH@tanuki>
On 30/10/2023 15:23, Patrick Steinhardt wrote:
> On Mon, Oct 30, 2023 at 03:13:35PM +0000, Phillip Wood wrote:
>> Hi Patrick
>>
>> On 30/10/2023 12:47, Patrick Steinhardt wrote:
>>> On Mon, Oct 30, 2023 at 01:15:10PM +0100, Patrick Steinhardt wrote:
>>> But once fixed, tests do indeed start to fail:
>>>
>>> t5540-http-push-webdav.sh (Wstat: 256 (exited 1) Tests: 20 Failed: 11)
>>> Failed tests: 5-11, 13, 15-16, 18
>>> Non-zero exit status: 1
>>>
>>> Seems like another thing to fix in a separate patch series.
>>
>> Yes, or we could just leave it - I had not realized before that it was only
>> the musl job that was not running the httpd tests (I thought
>> install-docker-dependencies.sh was missing the packages for ubuntu as well).
>> Given that, the status quo does not seem so bad.
>
> I of course couldn't let go. The following would fix this:
Great, that was fast! As you've got a fix I agree it makes sense to
include it in this series.
Best Wishes
Phillip
> diff --git a/ci/install-docker-dependencies.sh b/ci/install-docker-dependencies.sh
> index 5e28adf55b6..48cb2e735b5 100755
> --- a/ci/install-docker-dependencies.sh
> +++ b/ci/install-docker-dependencies.sh
> @@ -21,7 +21,8 @@ linux32)
> linux-musl)
> apk add --update shadow sudo build-base curl-dev openssl-dev expat-dev gettext \
> pcre2-dev python3 musl-libintl perl-utils ncurses \
> - apache2 bash cvs gnupg perl-cgi perl-dbd-sqlite >/dev/null
> + apache2 apache2-http2 apache2-proxy apache2-ssl apache2-webdav apr-util-dbd_sqlite3 \
> + bash cvs gnupg perl-cgi perl-dbd-sqlite >/dev/null
> ;;
> linux-*)
> apt update -q &&
> diff --git a/t/lib-httpd.sh b/t/lib-httpd.sh
> index 9791f94b16f..9ea74927c40 100644
> --- a/t/lib-httpd.sh
> +++ b/t/lib-httpd.sh
> @@ -128,6 +128,20 @@ else
> "Could not identify web server at '$LIB_HTTPD_PATH'"
> fi
>
> +if test -n "$LIB_HTTPD_DAV" && test -f /etc/os-release
> +then
> + case "$(grep "^ID=" /etc/os-release | cut -d= -f2-)" in
> + alpine)
> + # The WebDAV module in Alpine Linux is broken at least up to
> + # Alpine v3.16 as the default DBM driver is missing.
> + #
> + # https://gitlab.alpinelinux.org/alpine/aports/-/issues/13112
> + test_skip_or_die GIT_TEST_HTTPD \
> + "Apache WebDAV module does not have default DBM backend driver"
> + ;;
> + esac
> +fi
> +
> install_script () {
> write_script "$HTTPD_ROOT_PATH/$1" <"$TEST_PATH/$1"
> }
>
> I might as well roll it into this patch series now. It increases test
> coverage on musl libc and doesn't have any significant downsides. In
> fact, it uncovers that tests on Alpine Linux don't work right now, so it
> fixes real issues to have the test coverage in our pipelines.
>
> Patrick
^ permalink raw reply
* Re: [PATCH] reflog: fix expire --single-worktree
From: René Scharfe @ 2023-10-30 16:12 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Git List, John Cai
In-Reply-To: <xmqqa5s1hxhh.fsf@gitster.g>
Am 29.10.23 um 23:31 schrieb Junio C Hamano:
> René Scharfe <l.s.r@web.de> writes:
>
>> ... and added a non-printable short flag for it, presumably by
>> accident.
>
> Very well spotted.
>
> FWIW, with the following patch on top of this patch, all tests pass
> (and without your fix, of course this notices the "\001" and breaks
> numerous tests that use "git reflog"). So you seem to have found
> the only one broken instance (among those that are tested, anyway).
>
> parse-options.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git i/parse-options.c w/parse-options.c
> index 093eaf2db8..be8bedba29 100644
> --- i/parse-options.c
> +++ w/parse-options.c
> @@ -469,7 +469,8 @@ static void parse_options_check(const struct option *opts)
> optbug(opts, "uses incompatible flags "
> "LASTARG_DEFAULT and OPTARG");
> if (opts->short_name) {
> - if (0x7F <= opts->short_name)
> + if (opts->short_name &&
> + (opts->short_name < 0x21 || 0x7F <= opts->short_name))
Good idea. This is equivalent to !isprint(opts->short_name), which I
find to be more readable here. Seeing why "char short_opts[128];" a
few lines up is big enough would become a bit harder, though.
> optbug(opts, "invalid short name");
> else if (short_opts[opts->short_name]++)
> optbug(opts, "short name already used");
^ permalink raw reply
* Re: [PATCH 1/1] merge-file: add an option to process object IDs
From: brian m. carlson @ 2023-10-30 16:14 UTC (permalink / raw)
To: phillip.wood; +Cc: Elijah Newren, git, Junio C Hamano
In-Reply-To: <fec21bbe-46da-4f1c-a9b8-6be44403d68f@gmail.com>
[-- Attachment #1: Type: text/plain, Size: 468 bytes --]
On 2023-10-29 at 10:12:57, Phillip Wood wrote:
> It would be nice to print an error message here
>
> ret = error("object '%s' does not exist", argv[i]);
Good point. I'll include that in my reroll.
> none of the existing error messages are marked for translation so I've left
> this untranslated as well.
I've opted to make them translatable here because I think that's for the
best.
--
brian m. carlson (he/him or they/them)
Toronto, Ontario, CA
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 263 bytes --]
^ permalink raw reply
* Re: [PATCH 0/1] Object ID support for git merge-file
From: brian m. carlson @ 2023-10-30 16:24 UTC (permalink / raw)
To: Taylor Blau; +Cc: Elijah Newren, git, Junio C Hamano, Phillip Wood
In-Reply-To: <ZT/RpqvfQyx+uzxa@nand.local>
[-- Attachment #1: Type: text/plain, Size: 756 bytes --]
On 2023-10-30 at 15:54:14, Taylor Blau wrote:
> On Sat, Oct 28, 2023 at 11:24:06PM -0700, Elijah Newren wrote:
> > But...wouldn't you already have the conflicts generated when doing the
> > merge and learning that it fails? Why would you need to generate them
> > again?
>
> brian would know better than I do, but I believe the reason is because
> the "attempt this merge" RPC is handled separately from the "show me the
> merge conflict(s) at xyz path". Those probably could be combined
> (obviating the need for this patch), but doing so is probably rather
> complicated.
That's correct. They could in theory happen at different times, which
is why they're not linked.
--
brian m. carlson (he/him or they/them)
Toronto, Ontario, CA
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 263 bytes --]
^ permalink raw reply
* [PATCH v2 0/1] Object ID support for git merge-file
From: brian m. carlson @ 2023-10-30 16:26 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Elijah Newren, Phillip Wood, Eric Sunshine,
Taylor Blau
In-Reply-To: <20231024195655.2413191-1-sandals@crustytoothpaste.net>
This series introduces an --object-id option to git merge-file such
that, instead of reading and writing from files on the system, it reads
from and writes to the object store using blobs.
Changes from v1:
* Improve error handling
* Re-add `-p` argument for documentation
brian m. carlson (1):
merge-file: add an option to process object IDs
Documentation/git-merge-file.txt | 20 +++++++++++
builtin/merge-file.c | 62 +++++++++++++++++++++++---------
t/t6403-merge-file.sh | 58 ++++++++++++++++++++++++++++++
3 files changed, 124 insertions(+), 16 deletions(-)
^ permalink raw reply
* [PATCH v2 1/1] merge-file: add an option to process object IDs
From: brian m. carlson @ 2023-10-30 16:26 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Elijah Newren, Phillip Wood, Eric Sunshine,
Taylor Blau
In-Reply-To: <20231030162658.567523-1-sandals@crustytoothpaste.net>
From: "brian m. carlson" <bk2204@github.com>
git merge-file knows how to merge files on the file system already. It
would be helpful, however, to allow it to also merge single blobs.
Teach it an `--object-id` option which means that its arguments are
object IDs and not files to allow it to do so.
Since we obviously won't be writing the data to the first argument,
imply the -p option so we write to standard output.
We handle the empty blob specially since read_mmblob doesn't read it
directly and otherwise users cannot specify an empty ancestor.
Signed-off-by: brian m. carlson <bk2204@github.com>
---
Documentation/git-merge-file.txt | 20 +++++++++++
builtin/merge-file.c | 62 +++++++++++++++++++++++---------
t/t6403-merge-file.sh | 58 ++++++++++++++++++++++++++++++
3 files changed, 124 insertions(+), 16 deletions(-)
diff --git a/Documentation/git-merge-file.txt b/Documentation/git-merge-file.txt
index 7e9093fab6..a4de2bd297 100644
--- a/Documentation/git-merge-file.txt
+++ b/Documentation/git-merge-file.txt
@@ -12,6 +12,9 @@ SYNOPSIS
'git merge-file' [-L <current-name> [-L <base-name> [-L <other-name>]]]
[--ours|--theirs|--union] [-p|--stdout] [-q|--quiet] [--marker-size=<n>]
[--[no-]diff3] <current-file> <base-file> <other-file>
+'git merge-file' --object-id [-L <current-name> [-L <base-name> [-L <other-name>]]]
+ [--ours|--theirs|--union] [-p|--stdout] [-q|--quiet] [--marker-size=<n>]
+ [--[no-]diff3] <current-oid> <base-oid> <other-oid>
DESCRIPTION
@@ -40,6 +43,10 @@ however, these conflicts are resolved favouring lines from `<current-file>`,
lines from `<other-file>`, or lines from both respectively. The length of the
conflict markers can be given with the `--marker-size` option.
+If `--object-id` is specified, exactly the same behavior occurs, except that
+instead of specifying what to merge as files, it is specified as a list of
+object IDs referring to blobs.
+
The exit value of this program is negative on error, and the number of
conflicts otherwise (truncated to 127 if there are more than that many
conflicts). If the merge was clean, the exit value is 0.
@@ -52,6 +59,14 @@ linkgit:git[1].
OPTIONS
-------
+--object-id::
+ Specify the contents to merge as blobs in the current repository instead of
+ files. In this case, the operation must take place within a valid repository.
++
+If the `-p` option is specified, the merged file (including conflicts, if any)
+goes to standard output as normal; otherwise, the merged file is written to the
+object store and the object ID of its blob is written to standard output.
+
-L <label>::
This option may be given up to three times, and
specifies labels to be used in place of the
@@ -93,6 +108,11 @@ EXAMPLES
merges tmp/a123 and tmp/c345 with the base tmp/b234, but uses labels
`a` and `c` instead of `tmp/a123` and `tmp/c345`.
+`git merge-file -p --object-id abc1234 def567 890abcd`::
+
+ combines the changes of the blob abc1234 and 890abcd since def567,
+ tries to merge them and writes the result to standard output
+
GIT
---
Part of the linkgit:git[1] suite
diff --git a/builtin/merge-file.c b/builtin/merge-file.c
index d7eb4c6540..832c93d8d5 100644
--- a/builtin/merge-file.c
+++ b/builtin/merge-file.c
@@ -1,5 +1,8 @@
#include "builtin.h"
#include "abspath.h"
+#include "hex.h"
+#include "object-name.h"
+#include "object-store.h"
#include "config.h"
#include "gettext.h"
#include "setup.h"
@@ -31,10 +34,11 @@ int cmd_merge_file(int argc, const char **argv, const char *prefix)
mmfile_t mmfs[3] = { 0 };
mmbuffer_t result = { 0 };
xmparam_t xmp = { 0 };
- int ret = 0, i = 0, to_stdout = 0;
+ int ret = 0, i = 0, to_stdout = 0, object_id = 0;
int quiet = 0;
struct option options[] = {
OPT_BOOL('p', "stdout", &to_stdout, N_("send results to standard output")),
+ OPT_BOOL(0, "object-id", &object_id, N_("use object IDs instead of filenames")),
OPT_SET_INT(0, "diff3", &xmp.style, N_("use a diff3 based merge"), XDL_MERGE_DIFF3),
OPT_SET_INT(0, "zdiff3", &xmp.style, N_("use a zealous diff3 based merge"),
XDL_MERGE_ZEALOUS_DIFF3),
@@ -71,8 +75,12 @@ int cmd_merge_file(int argc, const char **argv, const char *prefix)
return error_errno("failed to redirect stderr to /dev/null");
}
+ if (object_id)
+ setup_git_directory();
+
for (i = 0; i < 3; i++) {
char *fname;
+ struct object_id oid;
mmfile_t *mmf = mmfs + i;
if (!names[i])
@@ -80,12 +88,22 @@ int cmd_merge_file(int argc, const char **argv, const char *prefix)
fname = prefix_filename(prefix, argv[i]);
- if (read_mmfile(mmf, fname))
+ if (object_id) {
+ if (repo_get_oid(the_repository, argv[i], &oid))
+ ret = error(_("object '%s' does not exist"),
+ argv[i]);
+ else if (!oideq(&oid, the_hash_algo->empty_blob))
+ read_mmblob(mmf, &oid);
+ else
+ read_mmfile(mmf, "/dev/null");
+ } else if (read_mmfile(mmf, fname)) {
ret = -1;
- else if (mmf->size > MAX_XDIFF_SIZE ||
- buffer_is_binary(mmf->ptr, mmf->size))
+ }
+ if (ret != -1 && (mmf->size > MAX_XDIFF_SIZE ||
+ buffer_is_binary(mmf->ptr, mmf->size))) {
ret = error("Cannot merge binary files: %s",
argv[i]);
+ }
free(fname);
if (ret)
@@ -99,20 +117,32 @@ int cmd_merge_file(int argc, const char **argv, const char *prefix)
ret = xdl_merge(mmfs + 1, mmfs + 0, mmfs + 2, &xmp, &result);
if (ret >= 0) {
- const char *filename = argv[0];
- char *fpath = prefix_filename(prefix, argv[0]);
- FILE *f = to_stdout ? stdout : fopen(fpath, "wb");
+ if (object_id && !to_stdout) {
+ struct object_id oid;
+ if (result.size) {
+ if (write_object_file(result.ptr, result.size, OBJ_BLOB, &oid) < 0)
+ ret = error(_("Could not write object file"));
+ } else {
+ oidcpy(&oid, the_hash_algo->empty_blob);
+ }
+ if (ret >= 0)
+ printf("%s\n", oid_to_hex(&oid));
+ } else {
+ const char *filename = argv[0];
+ char *fpath = prefix_filename(prefix, argv[0]);
+ FILE *f = to_stdout ? stdout : fopen(fpath, "wb");
- if (!f)
- ret = error_errno("Could not open %s for writing",
- filename);
- else if (result.size &&
- fwrite(result.ptr, result.size, 1, f) != 1)
- ret = error_errno("Could not write to %s", filename);
- else if (fclose(f))
- ret = error_errno("Could not close %s", filename);
+ if (!f)
+ ret = error_errno("Could not open %s for writing",
+ filename);
+ else if (result.size &&
+ fwrite(result.ptr, result.size, 1, f) != 1)
+ ret = error_errno("Could not write to %s", filename);
+ else if (fclose(f))
+ ret = error_errno("Could not close %s", filename);
+ free(fpath);
+ }
free(result.ptr);
- free(fpath);
}
if (ret > 127)
diff --git a/t/t6403-merge-file.sh b/t/t6403-merge-file.sh
index 1a7082323d..2c92209eca 100755
--- a/t/t6403-merge-file.sh
+++ b/t/t6403-merge-file.sh
@@ -65,11 +65,30 @@ test_expect_success 'merge with no changes' '
test_cmp test.txt orig.txt
'
+test_expect_success 'merge with no changes with --object-id' '
+ git add orig.txt &&
+ git merge-file -p --object-id :orig.txt :orig.txt :orig.txt >actual &&
+ test_cmp actual orig.txt
+'
+
test_expect_success "merge without conflict" '
cp new1.txt test.txt &&
git merge-file test.txt orig.txt new2.txt
'
+test_expect_success 'merge without conflict with --object-id' '
+ git add orig.txt new2.txt &&
+ git merge-file --object-id :orig.txt :orig.txt :new2.txt >actual &&
+ git rev-parse :new2.txt >expected &&
+ test_cmp actual expected
+'
+
+test_expect_success 'can accept object ID with --object-id' '
+ git merge-file --object-id $(test_oid empty_blob) $(test_oid empty_blob) :new2.txt >actual &&
+ git rev-parse :new2.txt >expected &&
+ test_cmp actual expected
+'
+
test_expect_success 'works in subdirectory' '
mkdir dir &&
cp new1.txt dir/a.txt &&
@@ -138,6 +157,31 @@ test_expect_success "expected conflict markers" '
test_cmp expect.txt test.txt
'
+test_expect_success "merge with conflicts with --object-id" '
+ git add backup.txt orig.txt new3.txt &&
+ test_must_fail git merge-file -p --object-id :backup.txt :orig.txt :new3.txt >actual &&
+ sed -e "s/<< test.txt/<< :backup.txt/" \
+ -e "s/>> new3.txt/>> :new3.txt/" \
+ expect.txt >expect &&
+ test_cmp expect actual &&
+ test_must_fail git merge-file --object-id :backup.txt :orig.txt :new3.txt >oid &&
+ git cat-file blob "$(cat oid)" >actual &&
+ test_cmp expect actual
+'
+
+test_expect_success "merge with conflicts with --object-id with labels" '
+ git add backup.txt orig.txt new3.txt &&
+ test_must_fail git merge-file -p --object-id \
+ -L test.txt -L orig.txt -L new3.txt \
+ :backup.txt :orig.txt :new3.txt >actual &&
+ test_cmp expect.txt actual &&
+ test_must_fail git merge-file --object-id \
+ -L test.txt -L orig.txt -L new3.txt \
+ :backup.txt :orig.txt :new3.txt >oid &&
+ git cat-file blob "$(cat oid)" >actual &&
+ test_cmp expect.txt actual
+'
+
test_expect_success "merge conflicting with --ours" '
cp backup.txt test.txt &&
@@ -256,6 +300,14 @@ test_expect_success 'binary files cannot be merged' '
grep "Cannot merge binary files" merge.err
'
+test_expect_success 'binary files cannot be merged with --object-id' '
+ cp "$TEST_DIRECTORY"/test-binary-1.png . &&
+ git add orig.txt new1.txt test-binary-1.png &&
+ test_must_fail git merge-file --object-id \
+ :orig.txt :test-binary-1.png :new1.txt 2> merge.err &&
+ grep "Cannot merge binary files" merge.err
+'
+
test_expect_success 'MERGE_ZEALOUS simplifies non-conflicts' '
sed -e "s/deerit.\$/deerit;/" -e "s/me;\$/me./" <new5.txt >new6.txt &&
sed -e "s/deerit.\$/deerit,/" -e "s/me;\$/me,/" <new5.txt >new7.txt &&
@@ -389,4 +441,10 @@ test_expect_success 'conflict sections match existing line endings' '
test $(tr "\015" Q <nolf.txt | grep "^[<=>].*Q$" | wc -l) = 0
'
+test_expect_success '--object-id fails without repository' '
+ empty="$(test_oid empty_blob)" &&
+ nongit test_must_fail git merge-file --object-id $empty $empty $empty 2>err &&
+ grep "not a git repository" err
+'
+
test_done
^ permalink raw reply related
* Re: [PATCH 1/1] merge-file: add an option to process object IDs
From: Elijah Newren @ 2023-10-30 16:39 UTC (permalink / raw)
To: brian m. carlson, Elijah Newren, git, Junio C Hamano,
Phillip Wood
In-Reply-To: <ZT5p0WUw7EeaY8vW@tapette.crustytoothpaste.net>
Hi,
On Sun, Oct 29, 2023 at 7:18 AM brian m. carlson
<sandals@crustytoothpaste.net> wrote:
>
> On 2023-10-29 at 06:17:09, Elijah Newren wrote:
> > Hi,
> >
> > Overall, looks good. Just a couple questions...
> >
> > On Tue, Oct 24, 2023 at 12:58 PM brian m. carlson
> > <sandals@crustytoothpaste.net> wrote:
> > >
> > > From: "brian m. carlson" <bk2204@github.com>
> > >
> > [...]
> > > --- a/Documentation/git-merge-file.txt
> > > +++ b/Documentation/git-merge-file.txt
> > > @@ -12,6 +12,9 @@ SYNOPSIS
> > > 'git merge-file' [-L <current-name> [-L <base-name> [-L <other-name>]]]
> > > [--ours|--theirs|--union] [-p|--stdout] [-q|--quiet] [--marker-size=<n>]
> > > [--[no-]diff3] <current-file> <base-file> <other-file>
> > > +'git merge-file' --object-id [-L <current-name> [-L <base-name> [-L <other-name>]]]
> > > + [--ours|--theirs|--union] [-q|--quiet] [--marker-size=<n>]
> > > + [--[no-]diff3] <current-oid> <base-oid> <other-oid>
> >
> > Why was the `[-p|--stdout]` option removed in the second synopsis?
> > Elsewhere you explicitly call it out as a possibility to be used with
> > --object-id.
>
> Originally because it implied `-p`, but I changed that to write into the
> object store. I'll restore it.
>
> > Also, why the extra synopsis instead of just adding a `[--object-id]`
> > option to the previous one?
>
> Because there's a relevant difference: the former has <current-file>,
> <base-file>, and <other-file>, and the latter has the -oid versions.
Ah, I looked over it a couple times and just kept missing that
difference. Thanks for pointing it out.
>
> > Does "/dev/null" have any portability considerations? (I really don't
> > know; just curious.)
>
> We already use it elsewhere in the codebase, so I assume it works. We
> also have a test for that case and it worked in CI, so it's probably
> fine.
Seems reasonable to me; thanks.
^ permalink raw reply
* Re: [PATCH] chore: fix typo in .clang-format comment
From: Taylor Blau @ 2023-10-30 16:56 UTC (permalink / raw)
To: Aditya Neelamraju via GitGitGadget; +Cc: git, Aditya Neelamraju
In-Reply-To: <pull.1602.git.git.1698610987926.gitgitgadget@gmail.com>
On Sun, Oct 29, 2023 at 08:23:07PM +0000, Aditya Neelamraju via GitGitGadget wrote:
> From: Aditya Neelamraju <adityanv97@gmail.com>
We typically prefix commit messages with the subject area they're
working in, not with "chore", or "feat" like some Git workflows
recommend.
That said, the contents of this patch look obviously correct to me.
Thanks for noticing and fixing!
Thanks,
Taylor
^ permalink raw reply
* Re: [PATCH 0/1] Object ID support for git merge-file
From: Elijah Newren @ 2023-10-30 17:14 UTC (permalink / raw)
To: brian m. carlson, Taylor Blau, Elijah Newren, git, Junio C Hamano,
Phillip Wood
In-Reply-To: <ZT_YuF4g-8P9fc4t@tapette.crustytoothpaste.net>
On Mon, Oct 30, 2023 at 9:24 AM brian m. carlson
<sandals@crustytoothpaste.net> wrote:
>
> On 2023-10-30 at 15:54:14, Taylor Blau wrote:
> > On Sat, Oct 28, 2023 at 11:24:06PM -0700, Elijah Newren wrote:
> > > But...wouldn't you already have the conflicts generated when doing the
> > > merge and learning that it fails? Why would you need to generate them
> > > again?
> >
> > brian would know better than I do, but I believe the reason is because
> > the "attempt this merge" RPC is handled separately from the "show me the
> > merge conflict(s) at xyz path". Those probably could be combined
> > (obviating the need for this patch), but doing so is probably rather
> > complicated.
>
> That's correct. They could in theory happen at different times, which
> is why they're not linked.
Maybe this is digging a little into "historical reasons" too much, but
this still seems a little funny. If they happen at different times,
you still need multiple pieces of information remembered from the
merge operation in order for git-merge-file to be able to regenerate
the conflict correctly in general. In particular, you need the OIDs
and the filenames. Trying to regenerate a conflict without
remembering those from the merge step would only work for common
cases, but would be problematic in the face of either renames being
involved or recursive merges or both. And if you need to remember
information from the merge step, then why not remember the actual
conflicts (or at least the tree OID generated by the merge operation,
which has the conflicts embedded within it)?
I know, I know, there's probably just historical cruft that needs
cleaning up, and I don't think any of this matters to the patch at
hand since it's independently useful. It just sounds like a system
has been set up that has some rough edge cases caused by a poor
splitting.
> --
> brian m. carlson (he/him or they/them)
> Toronto, Ontario, CA
^ permalink raw reply
* Re: [PATCH v2 0/1] Object ID support for git merge-file
From: Elijah Newren @ 2023-10-30 17:15 UTC (permalink / raw)
To: brian m. carlson
Cc: git, Junio C Hamano, Phillip Wood, Eric Sunshine, Taylor Blau
In-Reply-To: <20231030162658.567523-1-sandals@crustytoothpaste.net>
On Mon, Oct 30, 2023 at 9:27 AM brian m. carlson
<sandals@crustytoothpaste.net> wrote:
>
> This series introduces an --object-id option to git merge-file such
> that, instead of reading and writing from files on the system, it reads
> from and writes to the object store using blobs.
>
> Changes from v1:
> * Improve error handling
> * Re-add `-p` argument for documentation
>
> brian m. carlson (1):
> merge-file: add an option to process object IDs
>
> Documentation/git-merge-file.txt | 20 +++++++++++
> builtin/merge-file.c | 62 +++++++++++++++++++++++---------
> t/t6403-merge-file.sh | 58 ++++++++++++++++++++++++++++++
> 3 files changed, 124 insertions(+), 16 deletions(-)
Thanks, this version looks good to me.
^ permalink raw reply
* Re: [PATCH v5 00/14] Introduce new `git replay` command
From: Elijah Newren @ 2023-10-30 17:18 UTC (permalink / raw)
To: Johannes Schindelin
Cc: Christian Couder, git, Junio C Hamano, Patrick Steinhardt,
John Cai, Derrick Stolee, Phillip Wood, Calvin Wan, Toon Claes,
Dragan Simic, Linus Arver
In-Reply-To: <e04cfbdc-fd28-c645-8f5d-132f7ceec6be@gmx.de>
Hi Johannes,
On Sun, Oct 29, 2023 at 7:14 AM Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
>
> Hi Elijah,
>
> On Sat, 28 Oct 2023, Elijah Newren wrote:
>
> > On Tue, Oct 10, 2023 at 5:39 AM Christian Couder
> > <christian.couder@gmail.com> wrote:
> > > * Patch 12/15 (replay: disallow revision specific options and
> > > pathspecs) in version 4 has been removed, so there are now only 14
> > > patches instead of 15 in the series. This follows a suggestion by
> > > Dscho, and goes in the direction Elijah initially wanted before
> > > Derrick Stolee argued for disallowing revision specific options and
> > > pathspecs.
> >
> > [... snipping many parts that I agree with...]
> >
> > > Also instead of forcing reverse order we use the reverse order by
> > > default but allow it to be changed using `--reverse`. Thanks to
> > > Dscho.
> >
> > I can see why this might sometimes be useful for exclusively linear
> > history, but it seems to open a can of worms and possibly unfixable
> > corner cases for non-linear history. I'd rather not do this, or at
> > least pull it out of this series and let us discuss it in some follow
> > up series. There are some other alternatives that might handle such
> > usecases better.
>
> I find myself wishing for an easy way to reverse commits, if only to
> switch around the latest two commits while stopped during a rebase.
>
> So it would have been nice for me if there had been an easy, worktree-less
> way to make that happen.
Seems reasonable; we'll definitely want to keep this in mind.
> I guess this would be going in the direction of reordering commits,
> though, something we deliberately left for later?
Yes, I think that's a good framing for it.
Thanks,
Elijah
^ 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