* Re: [PATCH 1/2] t9902: verify that completion does not print anything
From: Patrick Steinhardt @ 2024-01-15 9:02 UTC (permalink / raw)
To: Jeff King; +Cc: Johannes Schindelin, git, Stan Hu
In-Reply-To: <20240112151655.GA640039@coredump.intra.peff.net>
[-- Attachment #1: Type: text/plain, Size: 1131 bytes --]
On Fri, Jan 12, 2024 at 10:16:55AM -0500, Jeff King wrote:
> On Fri, Jan 12, 2024 at 02:12:43PM +0100, Johannes Schindelin wrote:
>
> > But my main concern is: Why does this happen in the first place? If we are
> > running with Bash, why does `BASH_XTRACEFD` to work as intended here and
> > makes it necessary to filter out the traced commands?
>
> BASH_XTRACEFD was introduced in bash 4.1. macOS ships with the ancient
> bash 3.2.57, which is the last GPLv2 version.
>
> One simple solution is to mark the script with test_untraceable. See
> 5fc98e79fc (t: add means to disable '-x' tracing for individual test
> scripts, 2018-02-24) and 5827506928 (t1510-repo-setup: mark as
> untraceable with '-x', 2018-02-24).
>
> That will disable tracing entirely in the script for older versions of
> bash, which could make debugging harder. But it will still work as
> expected for people on reasonable versions of bash, and doesn't
> introduce any complicated code.
>
> -Peff
Ah, this is indeed the best solution. Thanks for the hints and
investigations everyone, will fix in the next iteration.
Patrick
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH v2 2/5] reftable/stack: refactor reloading to use file descriptor
From: Patrick Steinhardt @ 2024-01-15 10:03 UTC (permalink / raw)
To: Jeff King; +Cc: git, Han-Wen Nienhuys, Junio C Hamano
In-Reply-To: <20240114101424.GA1196682@coredump.intra.peff.net>
[-- Attachment #1: Type: text/plain, Size: 3799 bytes --]
On Sun, Jan 14, 2024 at 05:14:24AM -0500, Jeff King wrote:
> On Thu, Jan 11, 2024 at 11:06:43AM +0100, Patrick Steinhardt wrote:
>
> > We're about to introduce a stat(3P)-based caching mechanism to reload
> > the list of stacks only when it has changed. In order to avoid race
> > conditions this requires us to have a file descriptor available that we
> > can use to call fstat(3P) on.
> >
> > Prepare for this by converting the code to use `fd_read_lines()` so that
> > we have the file descriptor readily available.
>
> Coverity noted a case with this series where we might feed a negative
> value to fstat(). I'm not sure if it's a bug or not.
>
> The issue is that here:
>
> > @@ -329,9 +330,19 @@ static int reftable_stack_reload_maybe_reuse(struct reftable_stack *st,
> > if (tries > 3 && tv_cmp(&now, &deadline) >= 0)
> > goto out;
> >
> > - err = read_lines(st->list_file, &names);
> > - if (err < 0)
> > - goto out;
> > + fd = open(st->list_file, O_RDONLY);
> > + if (fd < 0) {
> > + if (errno != ENOENT) {
> > + err = REFTABLE_IO_ERROR;
> > + goto out;
> > + }
> > +
> > + names = reftable_calloc(sizeof(char *));
> > + } else {
> > + err = fd_read_lines(fd, &names);
> > + if (err < 0)
> > + goto out;
> > + }
>
> ...we might end up with fd as "-1" after calling open() on the list
> file. For most errors we'll jump to "out", which makes sense. But if we
> get ENOENT, we keep going with an empty file-list, which makes sense.
>
> But we then do other stuff with "fd". I think this case is OK:
>
> > @@ -356,12 +367,16 @@ static int reftable_stack_reload_maybe_reuse(struct reftable_stack *st,
> > names = NULL;
> > free_names(names_after);
> > names_after = NULL;
> > + close(fd);
> > + fd = -1;
>
> We only get here if reftable_stack_reload_once() returned an error,
> which it won't do since we feed it a blank set of names (and anyway,
> close(-1) is a harmless noop).
>
> But if we actually get to the end of the function, it's more
> questionable. As of this patch, it's OK:
>
> > delay = delay + (delay * rand()) / RAND_MAX + 1;
> > sleep_millisec(delay);
> > }
> >
> > out:
> > + if (fd >= 0)
> > + close(fd);
> > free_names(names);
> > free_names(names_after);
> > return err;
>
> But in the next patch we have this hunk:
>
> > @@ -374,7 +375,11 @@ static int reftable_stack_reload_maybe_reuse(struct reftable_stack *st,
> > sleep_millisec(delay);
> > }
> >
> > + stat_validity_update(&st->list_validity, fd);
> > +
> > out:
> > + if (err)
> > + stat_validity_clear(&st->list_validity);
> > if (fd >= 0)
> > close(fd);
> > free_names(names);
>
> which means we'll feed a negative value to stat_validity_update(). I
> think this may be OK, because I'd imagine the only sensible thing to do
> is call stat_validity_clear() instead. And using a negative fd means
> fstat() will fail, which will cause stat_validity_update() to clear the
> validity struct anyway. But I thought it was worth double-checking.
Good catch, and thanks a lot for double-checking. I was briefly
wondering whether this behaviour is actually specified by POSIX. In any
case, fstat(3P) explicitly documents `EBADF` as:
The fildes argument is not a valid file descriptor.
That makes me think that this code is indeed POSIX-compliant, as
implementations are expected to handle invalid file descriptors via this
error code.
So overall this works as intended, even though I would not consider it
to be the cleanest way to handle this. Unless you or others think that
this should be refactored I'll leave it as-is for now though.
Patrick
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH] clone: support cloning of filtered bundles
From: phillip.wood123 @ 2024-01-15 10:18 UTC (permalink / raw)
To: Nikolay Edigaryev, phillip.wood
Cc: Nikolay Edigaryev via GitGitGadget, git, Derrick Stolee
In-Reply-To: <CAFX5hXTCPt-rDrWZ-RN8S84o_FooY3Ck2H1kMYdHQGzuetPBSw@mail.gmail.com>
Hi Nikolay
On 14/01/2024 19:39, Nikolay Edigaryev wrote:
> Hello Phillip,
>
>> As I understand it if you're cloning from a bundle file then then there
>> is no remote so how can we set a remote-specific config?
>
> There is a remote, for more details see df61c88979 (clone: also
> configure url for bare clones, 2010-03-29), which has the following
> code:
>
> strbuf_addf(&key, "remote.%s.url", remote_name);
> git_config_set(key.buf, repo);
> strbuf_reset(&key);
>
> You can verify this by creating a bundle on Git 2.43.0 with "git create
> bundle bundle.bundle --all" and then cloning it with "git clone
> --bare /path/to/bundle.bundle", in my case the following repo-wide
> configuration file was created:
>
> [core]
> repositoryformatversion = 0
> filemode = true
> bare = true
> ignorecase = true
> precomposeunicode = true
> [remote "origin"]
> url = /Users/edi/src/cirrus-cli/cli.bundle
Oh, thanks for clarifying that I didn't realize we set "origin" to point
to the bundle. That means this patch creates a promisor remote config
pointing to a bundle that does not contain the missing objects. As Junio
said that doesn't make much sense to me as the point of the promisor
config is to allow git to lazily fetch the missing objects.
Best Wishes
Phillip
>> I'm surprised that the proposed change does not require the user to pass
>> "--filter" to "git clone" as I expected that we'd want to check that the
>> filter on the command line was compatible with the filter used to create
>> the bundle. Allowing "git clone" to create a partial clone without the
>> user asking for it by passing the "--filter" option feels like is going
>> to end up confusing users.
>
> Note that currently, when you clone a normal non-filtered bundle with a
> '--filter' argument specified, no filtering will take place and no error
> will be thrown. "promisor = true" and "partialclonefilter = ..." options
> will be set in the repo config, but no .promisor file will be created.
> This is even more confusing IMO, but that's how it currently on
> Git 2.43.0.
>
> You have a good point, but I feel like completely preventing cloning of
> filtered bundles and requiring a '--filter' argument is very taxing. If
> you've already specified a '--filter' when creating a bundle (and thus
> your intent to use partially cloned data), why do it multiple times?
>
> What I propose as an alternative here is to act based on the user's
> intent when cloning:
>
> * when the user specifies no '--filter' argument, do nothing special,
> allow cloning both types of bundles: normal and filtered (with the
> logic from this patch)
>
> * when the user does specify a '--filter' argument, either:
> * throw an error explaining that filtering of filtered bundles is not
> supported
> * or compare the user's filter specification and the one that is
> in the bundle and only throw an error if they mismatch
>
> Let me know what you think about this (and perhaps you have a more
> concrete example in mind where this will have negative consequences)
> and I'll be happy to do a next iteration.
>
>
> On Sun, Jan 14, 2024 at 10:00 PM Phillip Wood <phillip.wood123@gmail.com> wrote:
>>
>> Hi Nikolay
>>
>> On 14/01/2024 11:16, Nikolay Edigaryev via GitGitGadget wrote:
>>> From: Nikolay Edigaryev <edigaryev@gmail.com>
>>>
>>> f18b512bbb (bundle: create filtered bundles, 2022-03-09) introduced an
>>> incredibly useful ability to create filtered bundles, which advances
>>> the partial clone/promisor support in Git and allows for archiving
>>> large repositories to object storages like S3 in bundles that are:
>>>
>>> * easy to manage
>>> * bundle is just a single file, it's easier to guarantee atomic
>>> replacements in object storages like S3 and they are faster to
>>> fetch than a bare repository since there's only a single GET
>>> request involved
>>> * incredibly tiny
>>> * no indexes (which may be more than 10 MB for some repositories)
>>> and other fluff, compared to cloning a bare repository
>>> * bundle can be filtered to only contain the tips of refs neccessary
>>> for e.g. code-analysis purposes
>>>
>>> However, in 86fdd94d72 (clone: fail gracefully when cloning filtered
>>> bundle, 2022-03-09) the cloning of such bundles was disabled, with a
>>> note that this behavior is not desired, and it the long-term this
>>> should be possible.
>>>
>>> The commit above states that it's not possible to have this at the
>>> moment due to lack of remote and a repository-global config that
>>> specifies an object filter, yet it's unclear why a remote-specific
>>> config can't be used instead, which is what this change does.
>>
>> As I understand it if you're cloning from a bundle file then then there
>> is no remote so how can we set a remote-specific config?
>>
>> I'm surprised that the proposed change does not require the user to pass
>> "--filter" to "git clone" as I expected that we'd want to check that the
>> filter on the command line was compatible with the filter used to create
>> the bundle. Allowing "git clone" to create a partial clone without the
>> user asking for it by passing the "--filter" option feels like is going
>> to end up confusing users.
>>
>>> +test_expect_success 'cloning from filtered bundle works' '
>>> + git bundle create partial.bdl --all --filter=blob:none &&
>>> + git clone --bare partial.bdl partial 2>err
>>
>> The redirection hides any error message which will make debugging test
>> failures harder. It would be nice to see this test check any config set
>> when cloning and that git commands can run successfully in the repository.
>>
>> Best Wishes
>>
>> Phillip
^ permalink raw reply
* Re: [PATCH] clone: support cloning of filtered bundles
From: phillip.wood123 @ 2024-01-15 10:35 UTC (permalink / raw)
To: Nikolay Edigaryev, phillip.wood
Cc: Nikolay Edigaryev via GitGitGadget, git, Derrick Stolee
In-Reply-To: <CAFX5hXQ291gR4831dtRTdvtffWefCNCFp13ADvOkU7s3SVPczQ@mail.gmail.com>
Hi Nikolay
On 14/01/2024 21:26, Nikolay Edigaryev wrote:
>> Note that currently, when you clone a normal non-filtered bundle with a
>> '--filter' argument specified, no filtering will take place and no error
>> will be thrown. "promisor = true" and "partialclonefilter = ..." options
>> will be set in the repo config, but no .promisor file will be created.
>> This is even more confusing IMO, but that's how it currently on
>> Git 2.43.0.
>
> I was wrong about this one: the .promisor pack file actually gets created.
>
> And the filtering is not being done because the "uploadpack.allowFilter"
> global option is not enabled by default.
That makes sense - if you try to make a partial clone from a remote that
does not support object filtering we fallback to a full clone and print
the warning you mention below.
> Unfortunately the only way to figure this out is to run Git with
> GIT_TRACE=2, which shows:
That seems strange, you should see the warning without having to set
GIT_TRACE. I've certainly seen the warning in the past when trying to
create a partial clone from a remote did not support them without me
setting any special environment variables.
Best Wishes
Phillip
> warning: filtering not recognized by server, ignoring
>
> So please feel free to skip this part from the consideration.
>
>
> On Sun, Jan 14, 2024 at 11:39 PM Nikolay Edigaryev <edigaryev@gmail.com> wrote:
>>
>> Hello Phillip,
>>
>>> As I understand it if you're cloning from a bundle file then then there
>>> is no remote so how can we set a remote-specific config?
>>
>> There is a remote, for more details see df61c88979 (clone: also
>> configure url for bare clones, 2010-03-29), which has the following
>> code:
>>
>> strbuf_addf(&key, "remote.%s.url", remote_name);
>> git_config_set(key.buf, repo);
>> strbuf_reset(&key);
>>
>> You can verify this by creating a bundle on Git 2.43.0 with "git create
>> bundle bundle.bundle --all" and then cloning it with "git clone
>> --bare /path/to/bundle.bundle", in my case the following repo-wide
>> configuration file was created:
>>
>> [core]
>> repositoryformatversion = 0
>> filemode = true
>> bare = true
>> ignorecase = true
>> precomposeunicode = true
>> [remote "origin"]
>> url = /Users/edi/src/cirrus-cli/cli.bundle
>>
>>> I'm surprised that the proposed change does not require the user to pass
>>> "--filter" to "git clone" as I expected that we'd want to check that the
>>> filter on the command line was compatible with the filter used to create
>>> the bundle. Allowing "git clone" to create a partial clone without the
>>> user asking for it by passing the "--filter" option feels like is going
>>> to end up confusing users.
>>
>> Note that currently, when you clone a normal non-filtered bundle with a
>> '--filter' argument specified, no filtering will take place and no error
>> will be thrown. "promisor = true" and "partialclonefilter = ..." options
>> will be set in the repo config, but no .promisor file will be created.
>> This is even more confusing IMO, but that's how it currently on
>> Git 2.43.0.
>>
>> You have a good point, but I feel like completely preventing cloning of
>> filtered bundles and requiring a '--filter' argument is very taxing. If
>> you've already specified a '--filter' when creating a bundle (and thus
>> your intent to use partially cloned data), why do it multiple times?
>>
>> What I propose as an alternative here is to act based on the user's
>> intent when cloning:
>>
>> * when the user specifies no '--filter' argument, do nothing special,
>> allow cloning both types of bundles: normal and filtered (with the
>> logic from this patch)
>>
>> * when the user does specify a '--filter' argument, either:
>> * throw an error explaining that filtering of filtered bundles is not
>> supported
>> * or compare the user's filter specification and the one that is
>> in the bundle and only throw an error if they mismatch
>>
>> Let me know what you think about this (and perhaps you have a more
>> concrete example in mind where this will have negative consequences)
>> and I'll be happy to do a next iteration.
>>
>>
>> On Sun, Jan 14, 2024 at 10:00 PM Phillip Wood <phillip.wood123@gmail.com> wrote:
>>>
>>> Hi Nikolay
>>>
>>> On 14/01/2024 11:16, Nikolay Edigaryev via GitGitGadget wrote:
>>>> From: Nikolay Edigaryev <edigaryev@gmail.com>
>>>>
>>>> f18b512bbb (bundle: create filtered bundles, 2022-03-09) introduced an
>>>> incredibly useful ability to create filtered bundles, which advances
>>>> the partial clone/promisor support in Git and allows for archiving
>>>> large repositories to object storages like S3 in bundles that are:
>>>>
>>>> * easy to manage
>>>> * bundle is just a single file, it's easier to guarantee atomic
>>>> replacements in object storages like S3 and they are faster to
>>>> fetch than a bare repository since there's only a single GET
>>>> request involved
>>>> * incredibly tiny
>>>> * no indexes (which may be more than 10 MB for some repositories)
>>>> and other fluff, compared to cloning a bare repository
>>>> * bundle can be filtered to only contain the tips of refs neccessary
>>>> for e.g. code-analysis purposes
>>>>
>>>> However, in 86fdd94d72 (clone: fail gracefully when cloning filtered
>>>> bundle, 2022-03-09) the cloning of such bundles was disabled, with a
>>>> note that this behavior is not desired, and it the long-term this
>>>> should be possible.
>>>>
>>>> The commit above states that it's not possible to have this at the
>>>> moment due to lack of remote and a repository-global config that
>>>> specifies an object filter, yet it's unclear why a remote-specific
>>>> config can't be used instead, which is what this change does.
>>>
>>> As I understand it if you're cloning from a bundle file then then there
>>> is no remote so how can we set a remote-specific config?
>>>
>>> I'm surprised that the proposed change does not require the user to pass
>>> "--filter" to "git clone" as I expected that we'd want to check that the
>>> filter on the command line was compatible with the filter used to create
>>> the bundle. Allowing "git clone" to create a partial clone without the
>>> user asking for it by passing the "--filter" option feels like is going
>>> to end up confusing users.
>>>
>>>> +test_expect_success 'cloning from filtered bundle works' '
>>>> + git bundle create partial.bdl --all --filter=blob:none &&
>>>> + git clone --bare partial.bdl partial 2>err
>>>
>>> The redirection hides any error message which will make debugging test
>>> failures harder. It would be nice to see this test check any config set
>>> when cloning and that git commands can run successfully in the repository.
>>>
>>> Best Wishes
>>>
>>> Phillip
^ permalink raw reply
* [PATCH v2 0/5] completion: silence pseudo-ref existence check
From: Patrick Steinhardt @ 2024-01-15 10:35 UTC (permalink / raw)
To: git
Cc: Stan Hu, SZEDER Gábor, Jeff King, Johannes Schindelin,
Junio C Hamano
In-Reply-To: <cover.1704969119.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 1428 bytes --]
Hi,
this is the second version of my patch series that refactors the Bash
completion scripts to work better with reftables. Changes compared to
v1:
- I've incorporated SZEDER's feedback that he has sent in reply to the
original patch series that is already in `master`. Most importantly,
we now discover the repo path in `__git_pseudoref_exists ()` itself.
- I've included some refactorings to stop leaking local variables in
`__git_pseudoref_exists ()`.
- I've added tests for `__git_pseudoref_exists ()` directly, which was
also suggested by SZEDER.
- I've marked t9902 with `test_untraceable` now to unbreak macOS CI.
- I've refactored the completion test to use `git show-ref --exists`
to fix an issue with unborn pseudorefs that I didn't notice
previously.
I didn't include a range-diff because quite a lof of things have moved
around.
Patrick
Patrick Steinhardt (5):
completion: discover repo path in `__git_pseudoref_exists ()`
t9902: verify that completion does not print anything
completion: improve existence check for pseudo-refs
completion: silence pseudoref existence check
completion: treat dangling symrefs as existing pseudorefs
contrib/completion/git-completion.bash | 11 +++---
t/t9902-completion.sh | 47 ++++++++++++++++++++++++--
2 files changed, 50 insertions(+), 8 deletions(-)
--
2.43.GIT
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* [PATCH v2 1/5] completion: discover repo path in `__git_pseudoref_exists ()`
From: Patrick Steinhardt @ 2024-01-15 10:35 UTC (permalink / raw)
To: git
Cc: Stan Hu, SZEDER Gábor, Jeff King, Johannes Schindelin,
Junio C Hamano
In-Reply-To: <cover.1705314554.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 1695 bytes --]
The helper function `__git_pseudoref_exists ()` expects that the repo
path has already been discovered by its callers, which makes for a
rather fragile calling convention. Refactor the function to discover the
repo path itself to make it more self-contained, which also removes the
need to discover the path in some of its callers.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
contrib/completion/git-completion.bash | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 8c40ade494..06a9107449 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -138,6 +138,8 @@ __git_pseudoref_exists ()
{
local ref=$1
+ __git_find_repo_path
+
# If the reftable is in use, we have to shell out to 'git rev-parse'
# to determine whether the ref exists instead of looking directly in
# the filesystem to determine whether the ref exists. Otherwise, use
@@ -1656,7 +1658,6 @@ __git_cherry_pick_inprogress_options=$__git_sequencer_inprogress_options
_git_cherry_pick ()
{
- __git_find_repo_path
if __git_pseudoref_exists CHERRY_PICK_HEAD; then
__gitcomp "$__git_cherry_pick_inprogress_options"
return
@@ -2966,7 +2967,6 @@ _git_reset ()
_git_restore ()
{
- __git_find_repo_path
case "$prev" in
-s)
__git_complete_refs
@@ -2995,7 +2995,6 @@ __git_revert_inprogress_options=$__git_sequencer_inprogress_options
_git_revert ()
{
- __git_find_repo_path
if __git_pseudoref_exists REVERT_HEAD; then
__gitcomp "$__git_revert_inprogress_options"
return
--
2.43.GIT
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* [PATCH v2 2/5] t9902: verify that completion does not print anything
From: Patrick Steinhardt @ 2024-01-15 10:36 UTC (permalink / raw)
To: git
Cc: Stan Hu, SZEDER Gábor, Jeff King, Johannes Schindelin,
Junio C Hamano
In-Reply-To: <cover.1705314554.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 1384 bytes --]
The Bash completion script must not print anything to either stdout or
stderr. Instead, it is only expected to populate certain variables.
Tighten our `test_completion ()` test helper to verify this requirement.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
t/t9902-completion.sh | 12 ++++++++++--
1 file changed, 10 insertions(+), 2 deletions(-)
diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
index aa9a614de3..95ec762a74 100755
--- a/t/t9902-completion.sh
+++ b/t/t9902-completion.sh
@@ -5,6 +5,12 @@
test_description='test bash completion'
+# The Bash completion scripts must not print anything to either stdout or
+# stderr, which we try to verify. When tracing is enabled without support for
+# BASH_XTRACEFD this assertion will fail, so we have to mark the test as
+# untraceable with such ancient Bash versions.
+test_untraceable=UnfortunatelyYes
+
. ./lib-bash.sh
complete ()
@@ -87,9 +93,11 @@ test_completion ()
else
sed -e 's/Z$//' |sort >expected
fi &&
- run_completion "$1" &&
+ run_completion "$1" >"$TRASH_DIRECTORY"/bash-completion-output 2>&1 &&
sort out >out_sorted &&
- test_cmp expected out_sorted
+ test_cmp expected out_sorted &&
+ test_must_be_empty "$TRASH_DIRECTORY"/bash-completion-output &&
+ rm "$TRASH_DIRECTORY"/bash-completion-output
}
# Test __gitcomp.
--
2.43.GIT
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* [PATCH v2 3/5] completion: improve existence check for pseudo-refs
From: Patrick Steinhardt @ 2024-01-15 10:36 UTC (permalink / raw)
To: git
Cc: Stan Hu, SZEDER Gábor, Jeff King, Johannes Schindelin,
Junio C Hamano
In-Reply-To: <cover.1705314554.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 1606 bytes --]
Improve the existence check along the following lines:
- Stop stripping the "ref :" prefix and compare to the expected value
directly. This allows us to drop a now-unused variable that was
previously leaking into the user's shell.
- Mark the "head" variable as local so that we don't leak its value
into the user's shell.
- Stop manually handling the `-C $__git_repo_path` option, which the
`__git ()` wrapper aleady does for us.
- In simlar spirit, stop redirecting stderr, which is also handled by
the wrapper already.
Suggested-by: SZEDER Gábor <szeder.dev@gmail.com>
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
contrib/completion/git-completion.bash | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 06a9107449..d703e3e64f 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -137,6 +137,7 @@ __git_eread ()
__git_pseudoref_exists ()
{
local ref=$1
+ local head
__git_find_repo_path
@@ -146,9 +147,8 @@ __git_pseudoref_exists ()
# Bash builtins since executing Git commands are expensive on some
# platforms.
if __git_eread "$__git_repo_path/HEAD" head; then
- b="${head#ref: }"
- if [ "$b" == "refs/heads/.invalid" ]; then
- __git -C "$__git_repo_path" rev-parse --verify --quiet "$ref" 2>/dev/null
+ if [ "$head" == "ref: refs/heads/.invalid" ]; then
+ __git rev-parse --verify --quiet "$ref"
return $?
fi
fi
--
2.43.GIT
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* [PATCH v2 4/5] completion: silence pseudoref existence check
From: Patrick Steinhardt @ 2024-01-15 10:36 UTC (permalink / raw)
To: git
Cc: Stan Hu, SZEDER Gábor, Jeff King, Johannes Schindelin,
Junio C Hamano
In-Reply-To: <cover.1705314554.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 2991 bytes --]
In 44dbb3bf29 (completion: support pseudoref existence checks for
reftables, 2023-12-19), we have extended the Bash completion script to
support future ref backends better by using git-rev-parse(1) to check
for pseudo-ref existence. This conversion has introduced a bug, because
even though we pass `--quiet` to git-rev-parse(1) it would still output
the resolved object ID of the ref in question if it exists.
Fix this by redirecting its stdout to `/dev/null` and add a test that
catches this behaviour. Note that the test passes even without the fix
for the "files" backend because we parse pseudo refs via the filesystem
directly in that case. But the test will fail with the "reftable"
backend.
Helped-by: Jeff King <peff@peff.net>
Helped-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
contrib/completion/git-completion.bash | 2 +-
t/t9902-completion.sh | 31 ++++++++++++++++++++++++++
2 files changed, 32 insertions(+), 1 deletion(-)
diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index d703e3e64f..54ce58f73d 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -148,7 +148,7 @@ __git_pseudoref_exists ()
# platforms.
if __git_eread "$__git_repo_path/HEAD" head; then
if [ "$head" == "ref: refs/heads/.invalid" ]; then
- __git rev-parse --verify --quiet "$ref"
+ __git rev-parse --verify --quiet "$ref" >/dev/null
return $?
fi
fi
diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
index 95ec762a74..56dc7343a2 100755
--- a/t/t9902-completion.sh
+++ b/t/t9902-completion.sh
@@ -1933,6 +1933,14 @@ test_expect_success 'git checkout - --orphan with branch already provided comple
EOF
'
+test_expect_success 'git restore completes modified files' '
+ test_commit A a.file &&
+ echo B >a.file &&
+ test_completion "git restore a." <<-\EOF
+ a.file
+ EOF
+'
+
test_expect_success 'teardown after ref completion' '
git branch -d matching-branch &&
git tag -d matching-tag &&
@@ -2728,4 +2736,27 @@ test_expect_success '__git_complete' '
test_must_fail __git_complete ga missing
'
+test_expect_success '__git_pseudoref_exists' '
+ test_when_finished "rm -rf repo" &&
+ git init repo &&
+ (
+ cd repo &&
+ sane_unset __git_repo_path &&
+
+ # HEAD points to an existing branch, so it should exist.
+ test_commit A &&
+ __git_pseudoref_exists HEAD >output 2>&1 &&
+ test_must_be_empty output &&
+
+ # CHERRY_PICK_HEAD does not exist, so the existence check should fail.
+ ! __git_pseudoref_exists CHERRY_PICK_HEAD >output 2>&1 &&
+ test_must_be_empty output &&
+
+ # CHERRY_PICK_HEAD points to a commit, so it should exist.
+ git update-ref CHERRY_PICK_HEAD A &&
+ __git_pseudoref_exists CHERRY_PICK_HEAD >output 2>&1 &&
+ test_must_be_empty output
+ )
+'
+
test_done
--
2.43.GIT
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* [PATCH v2 5/5] completion: treat dangling symrefs as existing pseudorefs
From: Patrick Steinhardt @ 2024-01-15 10:36 UTC (permalink / raw)
To: git
Cc: Stan Hu, SZEDER Gábor, Jeff King, Johannes Schindelin,
Junio C Hamano
In-Reply-To: <cover.1705314554.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 1927 bytes --]
The `__git_pseudoref_exists ()` helper function back to git-rev-parse(1)
in case the reftable backend is in use. This is not in the same spirit
as the simple existence check that the "files" backend does though,
because there we only check for the pseudo-ref to exist with `test -f`.
With git-rev-parse(1) we not only check for existence, but also verify
that the pseudo-ref resolves to an object, which may not be the case
when the pseudo-ref points to an unborn branch.
Fix this issue by using `git show-ref --exists` instead. Note that we do
not have to silence stdout anymore as git-show-ref(1) will not print
anything.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
contrib/completion/git-completion.bash | 2 +-
t/t9902-completion.sh | 4 ++++
2 files changed, 5 insertions(+), 1 deletion(-)
diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 54ce58f73d..6662db221d 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -148,7 +148,7 @@ __git_pseudoref_exists ()
# platforms.
if __git_eread "$__git_repo_path/HEAD" head; then
if [ "$head" == "ref: refs/heads/.invalid" ]; then
- __git rev-parse --verify --quiet "$ref" >/dev/null
+ __git show-ref --exists "$ref"
return $?
fi
fi
diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
index 56dc7343a2..35eb534fdd 100755
--- a/t/t9902-completion.sh
+++ b/t/t9902-completion.sh
@@ -2743,6 +2743,10 @@ test_expect_success '__git_pseudoref_exists' '
cd repo &&
sane_unset __git_repo_path &&
+ # HEAD should exist, even if it points to an unborn branch.
+ __git_pseudoref_exists HEAD >output 2>&1 &&
+ test_must_be_empty output &&
+
# HEAD points to an existing branch, so it should exist.
test_commit A &&
__git_pseudoref_exists HEAD >output 2>&1 &&
--
2.43.GIT
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* Re: [Outreachy][PATCH v5] Port helper/test-ctype.c to unit-tests/t-ctype.c
From: Phillip Wood @ 2024-01-15 10:39 UTC (permalink / raw)
To: Achu Luma, git
Cc: chriscool, christian.couder, gitster, l.s.r, me, phillip.wood,
steadmon
In-Reply-To: <20240112102743.1440-1-ach.lumap@gmail.com>
Hi Achu
On 12/01/2024 10:27, Achu Luma wrote:
> In the recent codebase update (8bf6fbd00d (Merge branch
> 'js/doc-unit-tests', 2023-12-09)), a new unit testing framework was
> merged, providing a standardized approach for testing C code. Prior to
> this update, some unit tests relied on the test helper mechanism,
> lacking a dedicated unit testing framework. It's more natural to perform
> these unit tests using the new unit test framework.
>
> This commit migrates the unit tests for C character classification
> functions (isdigit(), isspace(), etc) from the legacy approach
> using the test-tool command `test-tool ctype` in t/helper/test-ctype.c
> to the new unit testing framework (t/unit-tests/test-lib.h).
>
> The migration involves refactoring the tests to utilize the testing
> macros provided by the framework (TEST() and check_*()).
>
> Mentored-by: Christian Couder <chriscool@tuxfamily.org>
> Helped-by: René Scharfe <l.s.r@web.de>
> Helped-by: Phillip Wood <phillip.wood123@gmail.com>
> Helped-by: Taylor Blau <me@ttaylorr.com>
> Signed-off-by: Achu Luma <ach.lumap@gmail.com>
> ---
> The change between version 4 and version 5 is:
> - Added tests to handle EOF.
>
> Thanks to Phillip for noticing the missing tests..
> Here is a diff between v4 and v5:
>
> + if (!check(!func(EOF))) \
> + test_msg(" i: 0x%02x (EOF)", EOF); \
Thanks for adding back the test for EOF, this version looks good to me.
Best Wishes
Phillip
> Thanks also to René, Phillip, Junio and Taylor who helped with
> previous versions.
>
> Makefile | 2 +-
> t/helper/test-ctype.c | 70 ------------------------------------
> t/helper/test-tool.c | 1 -
> t/helper/test-tool.h | 1 -
> t/t0070-fundamental.sh | 4 ---
> t/unit-tests/t-ctype.c | 80 ++++++++++++++++++++++++++++++++++++++++++
> 6 files changed, 81 insertions(+), 77 deletions(-)
> delete mode 100644 t/helper/test-ctype.c
> create mode 100644 t/unit-tests/t-ctype.c
>
> diff --git a/Makefile b/Makefile
> index 15990ff312..1a62e48759 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -792,7 +792,6 @@ TEST_BUILTINS_OBJS += test-chmtime.o
> TEST_BUILTINS_OBJS += test-config.o
> TEST_BUILTINS_OBJS += test-crontab.o
> TEST_BUILTINS_OBJS += test-csprng.o
> -TEST_BUILTINS_OBJS += test-ctype.o
> TEST_BUILTINS_OBJS += test-date.o
> TEST_BUILTINS_OBJS += test-delta.o
> TEST_BUILTINS_OBJS += test-dir-iterator.o
> @@ -1342,6 +1341,7 @@ THIRD_PARTY_SOURCES += sha1dc/%
> UNIT_TEST_PROGRAMS += t-basic
> UNIT_TEST_PROGRAMS += t-mem-pool
> UNIT_TEST_PROGRAMS += t-strbuf
> +UNIT_TEST_PROGRAMS += t-ctype
> UNIT_TEST_PROGS = $(patsubst %,$(UNIT_TEST_BIN)/%$X,$(UNIT_TEST_PROGRAMS))
> UNIT_TEST_OBJS = $(patsubst %,$(UNIT_TEST_DIR)/%.o,$(UNIT_TEST_PROGRAMS))
> UNIT_TEST_OBJS += $(UNIT_TEST_DIR)/test-lib.o
> diff --git a/t/helper/test-ctype.c b/t/helper/test-ctype.c
> deleted file mode 100644
> index e5659df40b..0000000000
> --- a/t/helper/test-ctype.c
> +++ /dev/null
> @@ -1,70 +0,0 @@
> -#include "test-tool.h"
> -
> -static int rc;
> -
> -static void report_error(const char *class, int ch)
> -{
> - printf("%s classifies char %d (0x%02x) wrongly\n", class, ch, ch);
> - rc = 1;
> -}
> -
> -static int is_in(const char *s, int ch)
> -{
> - /*
> - * We can't find NUL using strchr. Accept it as the first
> - * character in the spec -- there are no empty classes.
> - */
> - if (ch == '\0')
> - return ch == *s;
> - if (*s == '\0')
> - s++;
> - return !!strchr(s, ch);
> -}
> -
> -#define TEST_CLASS(t,s) { \
> - int i; \
> - for (i = 0; i < 256; i++) { \
> - if (is_in(s, i) != t(i)) \
> - report_error(#t, i); \
> - } \
> - if (t(EOF)) \
> - report_error(#t, EOF); \
> -}
> -
> -#define DIGIT "0123456789"
> -#define LOWER "abcdefghijklmnopqrstuvwxyz"
> -#define UPPER "ABCDEFGHIJKLMNOPQRSTUVWXYZ"
> -#define PUNCT "!\"#$%&'()*+,-./:;<=>?@[\\]^_`{|}~"
> -#define ASCII \
> - "\x00\x01\x02\x03\x04\x05\x06\x07\x08\x09\x0a\x0b\x0c\x0d\x0e\x0f" \
> - "\x10\x11\x12\x13\x14\x15\x16\x17\x18\x19\x1a\x1b\x1c\x1d\x1e\x1f" \
> - "\x20\x21\x22\x23\x24\x25\x26\x27\x28\x29\x2a\x2b\x2c\x2d\x2e\x2f" \
> - "\x30\x31\x32\x33\x34\x35\x36\x37\x38\x39\x3a\x3b\x3c\x3d\x3e\x3f" \
> - "\x40\x41\x42\x43\x44\x45\x46\x47\x48\x49\x4a\x4b\x4c\x4d\x4e\x4f" \
> - "\x50\x51\x52\x53\x54\x55\x56\x57\x58\x59\x5a\x5b\x5c\x5d\x5e\x5f" \
> - "\x60\x61\x62\x63\x64\x65\x66\x67\x68\x69\x6a\x6b\x6c\x6d\x6e\x6f" \
> - "\x70\x71\x72\x73\x74\x75\x76\x77\x78\x79\x7a\x7b\x7c\x7d\x7e\x7f"
> -#define CNTRL \
> - "\x00\x01\x02\x03\x04\x05\x06\x07\x08\x09\x0a\x0b\x0c\x0d\x0e\x0f" \
> - "\x10\x11\x12\x13\x14\x15\x16\x17\x18\x19\x1a\x1b\x1c\x1d\x1e\x1f" \
> - "\x7f"
> -
> -int cmd__ctype(int argc UNUSED, const char **argv UNUSED)
> -{
> - TEST_CLASS(isdigit, DIGIT);
> - TEST_CLASS(isspace, " \n\r\t");
> - TEST_CLASS(isalpha, LOWER UPPER);
> - TEST_CLASS(isalnum, LOWER UPPER DIGIT);
> - TEST_CLASS(is_glob_special, "*?[\\");
> - TEST_CLASS(is_regex_special, "$()*+.?[\\^{|");
> - TEST_CLASS(is_pathspec_magic, "!\"#%&',-/:;<=>@_`~");
> - TEST_CLASS(isascii, ASCII);
> - TEST_CLASS(islower, LOWER);
> - TEST_CLASS(isupper, UPPER);
> - TEST_CLASS(iscntrl, CNTRL);
> - TEST_CLASS(ispunct, PUNCT);
> - TEST_CLASS(isxdigit, DIGIT "abcdefABCDEF");
> - TEST_CLASS(isprint, LOWER UPPER DIGIT PUNCT " ");
> -
> - return rc;
> -}
> diff --git a/t/helper/test-tool.c b/t/helper/test-tool.c
> index 37ba996539..33b9501c21 100644
> --- a/t/helper/test-tool.c
> +++ b/t/helper/test-tool.c
> @@ -19,7 +19,6 @@ static struct test_cmd cmds[] = {
> { "config", cmd__config },
> { "crontab", cmd__crontab },
> { "csprng", cmd__csprng },
> - { "ctype", cmd__ctype },
> { "date", cmd__date },
> { "delta", cmd__delta },
> { "dir-iterator", cmd__dir_iterator },
> diff --git a/t/helper/test-tool.h b/t/helper/test-tool.h
> index 8a1a7c63da..b72f07ded9 100644
> --- a/t/helper/test-tool.h
> +++ b/t/helper/test-tool.h
> @@ -12,7 +12,6 @@ int cmd__chmtime(int argc, const char **argv);
> int cmd__config(int argc, const char **argv);
> int cmd__crontab(int argc, const char **argv);
> int cmd__csprng(int argc, const char **argv);
> -int cmd__ctype(int argc, const char **argv);
> int cmd__date(int argc, const char **argv);
> int cmd__delta(int argc, const char **argv);
> int cmd__dir_iterator(int argc, const char **argv);
> diff --git a/t/t0070-fundamental.sh b/t/t0070-fundamental.sh
> index 487bc8d905..a4756fbab9 100755
> --- a/t/t0070-fundamental.sh
> +++ b/t/t0070-fundamental.sh
> @@ -9,10 +9,6 @@ Verify wrappers and compatibility functions.
> TEST_PASSES_SANITIZE_LEAK=true
> . ./test-lib.sh
>
> -test_expect_success 'character classes (isspace, isalpha etc.)' '
> - test-tool ctype
> -'
> -
> test_expect_success 'mktemp to nonexistent directory prints filename' '
> test_must_fail test-tool mktemp doesnotexist/testXXXXXX 2>err &&
> grep "doesnotexist/test" err
> diff --git a/t/unit-tests/t-ctype.c b/t/unit-tests/t-ctype.c
> new file mode 100644
> index 0000000000..f315489984
> --- /dev/null
> +++ b/t/unit-tests/t-ctype.c
> @@ -0,0 +1,80 @@
> +#include "test-lib.h"
> +
> +static int is_in(const char *s, int ch)
> +{
> + /*
> + * We can't find NUL using strchr. Accept it as the first
> + * character in the spec -- there are no empty classes.
> + */
> + if (ch == '\0')
> + return ch == *s;
> + if (*s == '\0')
> + s++;
> + return !!strchr(s, ch);
> +}
> +
> +/* Macro to test a character type */
> +#define TEST_CTYPE_FUNC(func, string) \
> +static void test_ctype_##func(void) { \
> + for (int i = 0; i < 256; i++) { \
> + if (!check_int(func(i), ==, is_in(string, i))) \
> + test_msg(" i: 0x%02x", i); \
> + } \
> + if (!check(!func(EOF))) \
> + test_msg(" i: 0x%02x (EOF)", EOF); \
> +}
> +
> +#define TEST_CHAR_CLASS(class) TEST(test_ctype_##class(), #class " works")
> +
> +#define DIGIT "0123456789"
> +#define LOWER "abcdefghijklmnopqrstuvwxyz"
> +#define UPPER "ABCDEFGHIJKLMNOPQRSTUVWXYZ"
> +#define PUNCT "!\"#$%&'()*+,-./:;<=>?@[\\]^_`{|}~"
> +#define ASCII \
> + "\x00\x01\x02\x03\x04\x05\x06\x07\x08\x09\x0a\x0b\x0c\x0d\x0e\x0f" \
> + "\x10\x11\x12\x13\x14\x15\x16\x17\x18\x19\x1a\x1b\x1c\x1d\x1e\x1f" \
> + "\x20\x21\x22\x23\x24\x25\x26\x27\x28\x29\x2a\x2b\x2c\x2d\x2e\x2f" \
> + "\x30\x31\x32\x33\x34\x35\x36\x37\x38\x39\x3a\x3b\x3c\x3d\x3e\x3f" \
> + "\x40\x41\x42\x43\x44\x45\x46\x47\x48\x49\x4a\x4b\x4c\x4d\x4e\x4f" \
> + "\x50\x51\x52\x53\x54\x55\x56\x57\x58\x59\x5a\x5b\x5c\x5d\x5e\x5f" \
> + "\x60\x61\x62\x63\x64\x65\x66\x67\x68\x69\x6a\x6b\x6c\x6d\x6e\x6f" \
> + "\x70\x71\x72\x73\x74\x75\x76\x77\x78\x79\x7a\x7b\x7c\x7d\x7e\x7f"
> +#define CNTRL \
> + "\x00\x01\x02\x03\x04\x05\x06\x07\x08\x09\x0a\x0b\x0c\x0d\x0e\x0f" \
> + "\x10\x11\x12\x13\x14\x15\x16\x17\x18\x19\x1a\x1b\x1c\x1d\x1e\x1f" \
> + "\x7f"
> +
> +TEST_CTYPE_FUNC(isdigit, DIGIT)
> +TEST_CTYPE_FUNC(isspace, " \n\r\t")
> +TEST_CTYPE_FUNC(isalpha, LOWER UPPER)
> +TEST_CTYPE_FUNC(isalnum, LOWER UPPER DIGIT)
> +TEST_CTYPE_FUNC(is_glob_special, "*?[\\")
> +TEST_CTYPE_FUNC(is_regex_special, "$()*+.?[\\^{|")
> +TEST_CTYPE_FUNC(is_pathspec_magic, "!\"#%&',-/:;<=>@_`~")
> +TEST_CTYPE_FUNC(isascii, ASCII)
> +TEST_CTYPE_FUNC(islower, LOWER)
> +TEST_CTYPE_FUNC(isupper, UPPER)
> +TEST_CTYPE_FUNC(iscntrl, CNTRL)
> +TEST_CTYPE_FUNC(ispunct, PUNCT)
> +TEST_CTYPE_FUNC(isxdigit, DIGIT "abcdefABCDEF")
> +TEST_CTYPE_FUNC(isprint, LOWER UPPER DIGIT PUNCT " ")
> +
> +int cmd_main(int argc, const char **argv) {
> + /* Run all character type tests */
> + TEST_CHAR_CLASS(isspace);
> + TEST_CHAR_CLASS(isdigit);
> + TEST_CHAR_CLASS(isalpha);
> + TEST_CHAR_CLASS(isalnum);
> + TEST_CHAR_CLASS(is_glob_special);
> + TEST_CHAR_CLASS(is_regex_special);
> + TEST_CHAR_CLASS(is_pathspec_magic);
> + TEST_CHAR_CLASS(isascii);
> + TEST_CHAR_CLASS(islower);
> + TEST_CHAR_CLASS(isupper);
> + TEST_CHAR_CLASS(iscntrl);
> + TEST_CHAR_CLASS(ispunct);
> + TEST_CHAR_CLASS(isxdigit);
> + TEST_CHAR_CLASS(isprint);
> +
> + return test_done();
> +}
> --
> 2.42.0.windows.2
>
>
^ permalink raw reply
* Re: [RFC PATCH] `log --merge` also for rebase/cherry pick/revert
From: Phillip Wood @ 2024-01-15 10:48 UTC (permalink / raw)
To: Michael Lohmann; +Cc: git, phillip.wood, Junio C Hamano
In-Reply-To: <20240112150346.73735-1-mi.al.lohmann@gmail.com>
Hi Michael
On 12/01/2024 15:03, Michael Lohmann wrote:
> Hi Phillip,
>
> On 12. Jan 2024, at 12:01, phillip.wood123@gmail.com wrote:
>> I should start by saying that I didn't know "git log --merge" existed before
>> I saw this message
> I also just found it and it looked very useful...
>
>> so please correct me if I've misunderstood what this patch is doing. If I
>> understand correctly it shows the commits from each side of the merge and is
>> equivalent to
>>
>> git log HEAD MERGE_HEAD ^$(git merge-base HEAD MERGE_HEAD)
>>
>> When a commit is cherry-picked the merge base used is CHERRY_PICK_HEAD^ [*]
>> so I'm not sure what
>>
>> git log HEAD CHERRY_PICK_HEAD ^$(git merge-base HEAD CHERRY_PICK_HEAD)
>
> Almost, but not quite: "git log —merge" only shows the commits touching the
> conflict, so it would be equivalent to (I think):
>
> git log HEAD CHERRY_PICK_HEAD ^$(git merge-base HEAD CHERRY_PICK_HEAD) -- $(git diff --name-only --diff-filter=U --relative)
>
> (or replace CHERRY_PICK with one of the other actions)
Thanks for clarifying that.
>> Indeed there HEAD and CHERRY_PICK_HEAD may not share a common ancestor.
>
> True - but same for MERGE_HEAD ("git merge --allow-unrelated-histories"). I
> have to confess I did not check how it would behave under those circumstances.
> It could either error, or (more helpful) show the log touching the file until
> the root commit.
What I was trying to get at was that with "git merge" "git log --merge"
will show commits that are part of the merge. With "git cherry-pick"
that's not the case because we're selecting the commits to show using
the merge base of HEAD and CHERRY_PICK_HEAD while cherry-pick uses
CHERRY_PICK_HEAD^ as the base of the merge. I think Junio explains why
it is still useful to show those commits in [1] i.e. they help the user
understand the conflicts even though they are not part of the merge. It
might be worth expanding the commit message to explain that.
Best Wishes
Phillip
[1] https://lore.kernel.org/git/xmqqil3y9rvm.fsf@gitster.g/
^ permalink raw reply
* Re: [PATCH] advice: allow disabling the automatic hint in advise_if_enabled()
From: Rubén Justo @ 2024-01-15 11:24 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Git List, Taylor Blau, Jeff King, Dragan Simic
In-Reply-To: <xmqqsf326vpn.fsf@gitster.g>
On 12-ene-2024 14:19:32, Junio C Hamano wrote:
> Rubén Justo <rjusto@gmail.com> writes:
>
> > advice.c | 109 +++++++++++++++++++++++++---------------------
> > t/t0018-advice.sh | 1 -
> > 2 files changed, 59 insertions(+), 51 deletions(-)
> >
> > diff --git a/advice.c b/advice.c
> > index f6e4c2f302..8474966ce1 100644
> > --- a/advice.c
> > +++ b/advice.c
> > @@ -33,50 +33,56 @@ static const char *advise_get_color(enum color_advice ix)
> > return "";
> > }
> >
> > +enum advice_level {
> > + ADVICE_LEVEL_DEFAULT = 0,
>
> We may want to say "_NONE" not "_DEFAULT" to match the convention
> used elsewhere, but I have no strong preference.
The "_DEFAULT" name is rooted in the idea of having a default but
configurable value. I'm not developing that idea in this series because
I'm not sure if it's desirable. But, I'll leave a sketch here:
diff --git a/advice.c b/advice.c
index 8474966ce1..1bb427a8d8 100644
--- a/advice.c
+++ b/advice.c
@@ -39,6 +39,8 @@ enum advice_level {
ADVICE_LEVEL_ENABLED,
};
+static enum advice_level advice_level_default;
+
static struct {
const char *key;
enum advice_level level;
@@ -122,7 +124,9 @@ void advise(const char *advice, ...)
int advice_enabled(enum advice_type type)
{
- int enabled = advice_setting[type].level != ADVICE_LEVEL_DISABLED;
+ int enabled = advice_setting[type].level
+ ? advice_setting[type].level != ADVICE_LEVEL_DISABLED
+ : advice_level_default != ADVICE_LEVEL_DISABLED;
if (type == ADVICE_PUSH_UPDATE_REJECTED)
return enabled &&
@@ -139,7 +143,7 @@ void advise_if_enabled(enum advice_type type, const char *advice, ...)
return;
va_start(params, advice);
- vadvise(advice, advice_setting[type].level == ADVICE_LEVEL_DEFAULT,
+ vadvise(advice, !advice_setting[type].level && !advice_level_default,
advice_setting[type].key, params);
va_end(params);
}
@@ -166,6 +170,13 @@ int git_default_advice_config(const char *var, const char *value)
if (!skip_prefix(var, "advice.", &k))
return 0;
+ if (!strcmp(var, "advice.default")) {
+ advice_level_default = git_config_bool(var, value)
+ ? ADVICE_LEVEL_ENABLED
+ : ADVICE_LEVEL_DISABLED;
+ return 0;
+ }
+
for (i = 0; i < ARRAY_SIZE(advice_setting); i++) {
if (strcasecmp(k, advice_setting[i].key))
continue;
The way it is now, "_NONE" is perfectly fine. I'll use it.
> I do have a
> preference to see that, when we explicitly say "In this enum, there
> is ADVICE_LEVEL_DEFAULT and its value is zero" with "= 0" in the
> definition, the places we use a variable of this enum type, we say
>
> if (!variable)
OK
>
> and not
>
> if (variable == ADVICE_LEVEL_DEFAULT)
>
> > + ADVICE_LEVEL_DISABLED,
> > + ADVICE_LEVEL_ENABLED,
> > +};
> > +
> > static struct {
> > const char *key;
> > - int enabled;
> > + enum advice_level level;
> > } advice_setting[] = {
> > ...
> > - [ADVICE_WORKTREE_ADD_ORPHAN] = { "worktreeAddOrphan", 1 },
> > + [ADVICE_ADD_EMBEDDED_REPO] = { "addEmbeddedRepo" },
> > ...
> > + [ADVICE_WORKTREE_ADD_ORPHAN] = { "worktreeAddOrphan" },
> > };
>
> It certainly becomes nicer to use the "unspecified is left to 0"
> convention like this.
Indeed, that's my feeling too.
>
> > static const char turn_off_instructions[] =
> > @@ -116,13 +122,13 @@ void advise(const char *advice, ...)
> >
> > int advice_enabled(enum advice_type type)
> > {
> > - switch(type) {
> > - case ADVICE_PUSH_UPDATE_REJECTED:
> > - return advice_setting[ADVICE_PUSH_UPDATE_REJECTED].enabled &&
> > - advice_setting[ADVICE_PUSH_UPDATE_REJECTED_ALIAS].enabled;
> > - default:
> > - return advice_setting[type].enabled;
> > - }
> > + int enabled = advice_setting[type].level != ADVICE_LEVEL_DISABLED;
>
> OK.
>
> > + if (type == ADVICE_PUSH_UPDATE_REJECTED)
> > + return enabled &&
> > + advice_enabled(ADVICE_PUSH_UPDATE_REJECTED_ALIAS);
>
> I like the textbook use of a simple recursion here ;-)
>
> > + return enabled;
> > }
>
> > void advise_if_enabled(enum advice_type type, const char *advice, ...)
> > @@ -133,7 +139,8 @@ void advise_if_enabled(enum advice_type type, const char *advice, ...)
> > return;
> >
> > va_start(params, advice);
> > - vadvise(advice, 1, advice_setting[type].key, params);
> > + vadvise(advice, advice_setting[type].level == ADVICE_LEVEL_DEFAULT,
> > + advice_setting[type].key, params);
>
> OK. Once you configure this advice to be always shown, you no
> longer are using the _DEFAULT, so we pass 0 as the second parameter.
> Makes sense.
>
> As I said, if we explicitly document that _DEFAULT's value is zero
> with "= 0" in the enum definition, which we also rely on the
> initialization of the advice_setting[] array, then it may probably
> be better to write it more like so:
>
> vadvice(advice, !advice_setting[type].level,
> advice_setting[type].key, params);
OK
>
> I wonder if it makes this caller simpler to update the return value
> of advice_enabled() to a tristate. Then we can do
>
> int enabled = advice_enabled(type);
>
> if (!enabled)
> return;
> va_start(...);
> vadvice(advice, enabled != ADVICE_LEVEL_ENABLED, ...);
> va_end(...);
>
> but it does not make that much difference.
>
> > va_end(params);
> > }
> >
> > @@ -162,7 +169,9 @@ int git_default_advice_config(const char *var, const char *value)
> > for (i = 0; i < ARRAY_SIZE(advice_setting); i++) {
> > if (strcasecmp(k, advice_setting[i].key))
> > continue;
> > - advice_setting[i].enabled = git_config_bool(var, value);
> > + advice_setting[i].level = git_config_bool(var, value)
> > + ? ADVICE_LEVEL_ENABLED
> > + : ADVICE_LEVEL_DISABLED;
>
> OK. We saw (var, value) in the configuration files we are parsing,
> so we find [i] that corresponds to the advice key and tweak the
> level.
>
> This loop obviously is O(N*M) for the number of "advice.*"
> configuration variables N and the number of advice keys M, but that
> is not new to this patch---our expectation is that N will be small,
> even though M will grow over time.
I wonder if we can drop entirely this loop. Maybe a hashmap can be
helpful here. Of course, this is tangential to this series.
>
> > diff --git a/t/t0018-advice.sh b/t/t0018-advice.sh
> > index c13057a4ca..0dcfb760a2 100755
> > --- a/t/t0018-advice.sh
> > +++ b/t/t0018-advice.sh
> > @@ -17,7 +17,6 @@ test_expect_success 'advice should be printed when config variable is unset' '
> > test_expect_success 'advice should be printed when config variable is set to true' '
> > cat >expect <<-\EOF &&
> > hint: This is a piece of advice
> > - hint: Disable this message with "git config advice.nestedTag false"
> > EOF
> > test_config advice.nestedTag true &&
> > test-tool advise "This is a piece of advice" 2>actual &&
>
> OK. The commit changes behaviour for existing users who explicitly
> said "I want to see this advice" by configuring advice.* key to 'true',
Technically, when the user sets any value.
Maybe in the future we transform the knob to have more than two states,
beyond yes/no. At that point, current rationality would still apply.
> and it probably is a good thing, even though it may be a bit surprising.
>
> One thing that may be missing is a documentation update. Something
> along this line to highlight that now 'true' has a bit different
> meaning from before (and as a consequence, we can no longer say
> "defaults to true").
>
> Documentation/config/advice.txt | 10 +++++++---
> 1 file changed, 7 insertions(+), 3 deletions(-)
>
> diff --git c/Documentation/config/advice.txt w/Documentation/config/advice.txt
> index 2737381a11..364a8268b6 100644
> --- c/Documentation/config/advice.txt
> +++ w/Documentation/config/advice.txt
> @@ -1,7 +1,11 @@
> advice.*::
> - These variables control various optional help messages designed to
> - aid new users. All 'advice.*' variables default to 'true', and you
> - can tell Git that you do not need help by setting these to 'false':
> +
> + These variables control various optional help messages
> + designed to aid new users. When left unconfigured, Git will
> + give you the help message with an instruction on how to
> + squelch it. When set to 'true', the help message is given,
> + but without hint on how to squelch the message. You can
> + tell Git that you do not need help by setting these to 'false':
> +
> --
> ambiguousFetchRefspec::
OK. I'll add this.
Thanks.
^ permalink raw reply related
* Re: [PATCH v2 1/2] revision: ensure MERGE_HEAD is a ref in prepare_show_merge
From: Patrick Steinhardt @ 2024-01-15 11:36 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Michael Lohmann, git, newren, phillip.wood123
In-Reply-To: <xmqqzfxa9usx.fsf@gitster.g>
[-- Attachment #1: Type: text/plain, Size: 2319 bytes --]
On Fri, Jan 12, 2024 at 12:10:54PM -0800, Junio C Hamano wrote:
> Michael Lohmann <mi.al.lohmann@gmail.com> writes:
>
> >> but we may want to tighten the original's use of repo_get
> >> > - if (repo_get_oid(the_repository, "MERGE_HEAD", &oid))
> >> > - die("--merge without MERGE_HEAD?");
> >> > - other = lookup_commit_or_die(&oid, "MERGE_HEAD");
> >>
> >> ... so that we won't be confused in a repository that has a tag
> >> whose name happens to be MERGE_HEAD. We probably should be using
> >> refs.c:refs_resolve_ref_unsafe() instead to _oid() here ...
> >
> > Here I am really at the limit of my understanding of the core functions.
> > Is this roughly what you had in mind? From my testing it seems to do the
> > job, but I don't understand the details "refs_resolve_ref_unsafe"...
>
> Perhaps there are others who are more familiar with the ref API than
> I am, but I was just looking at refs.h today and wonder if something
> along the lines of this ...
>
> if (read_ref_full("MERGE_HEAD",
> RESOLVE_REF_READING | RESOLVE_REF_NO_RECURSE,
> &oid, NULL))
> die("no MERGE_HEAD");
> if (is_null_oid(&oid))
> die("MERGE_HEAD is a symbolic ref???");
>
> ... would be simpler.
>
> With the above, we are discarding the refname read_ref_full()
> obtains from its refs_resolve_ref_unsafe(), but I think that is
> totally fine. We expect it to be "MERGE_HEAD" in the good case.
> The returned value can be different from "MERGE_HEAD" if it is
> a symbolic ref, but if the comment in refs.h on NO_RECURSE is to be
> trusted, we should catch that case with the NULL-ness check on oid.
Yeah, this should be fine.
Even though I have stared at our refs API for extended periods of time
during the last months I still have to look up this part of the API
almost every time. I find it to be hard to use because you not only have
to pay attention to the return value, but also to the flags in some edge
cases. I wouldn't be surprised if there were many callsites that get
this wrong.
> Which would mean that we do not need a separate "other_head"
> variable, and instead can use "MERGE_HEAD".
There is no need for this, is there? We have already resolved the ref to
an object ID, so why not use that via `add_pending_oid()`?
Patrick
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [Outreachy][PATCH] Port helper/test-advise.c to unit-tests/t-advise.c
From: Rubén Justo @ 2024-01-15 11:37 UTC (permalink / raw)
To: Junio C Hamano, Achu Luma; +Cc: git, christian.couder, Christian Couder
In-Reply-To: <xmqqmsta6uju.fsf@gitster.g>
On 12-ene-2024 14:44:37, Junio C Hamano wrote:
> Achu Luma <ach.lumap@gmail.com> writes:
>
> > In the recent codebase update (8bf6fbd00d (Merge branch
> > 'js/doc-unit-tests', 2023-12-09)), a new unit testing framework was
> > merged, providing a standardized approach for testing C code. Prior to
> > this update, some unit tests relied on the test helper mechanism,
> > lacking a dedicated unit testing framework. It's more natural to perform
> > these unit tests using the new unit test framework.
> >
> > This commit migrates the unit tests for advise_if_enabled functionality
> > from the legacy approach using the test-tool command `test-tool advise`
> > in t/helper/test-advise.c to the new unit testing framework
> > (t/unit-tests/test-lib.h).
> >
> > The migration involves refactoring the tests to utilize the testing
> > macros provided by the framework (TEST() and check_*()).
>
> In the light of the presense of work like this one
>
> https://lore.kernel.org/git/c870a0b6-9fa8-4d00-a5a6-661ca175805f@gmail.com/
>
> I am not sure if moving this to unit-test framework is a good thing,
> at least right at this moment.
>
> To test the effect of setting one configuration variable, and ensure
> it results in a slightly different advice message output to the
> standard error stream, "test-tool advice" needs only a single line
> of patch, but if we started with this version, how much work does it
> take to run the equivalent test in the other patch if it were to be
> rebased on top of this change? It won't be just the matter of
> adding a new TEST(check_advise_if_enabled()) call to cmd_main(),
> will it?
Maybe something like this will do the trick:
diff --git a/t/unit-tests/t-advise.c b/t/unit-tests/t-advise.c
index 15df29c955..ac7d2620ef 100644
--- a/t/unit-tests/t-advise.c
+++ b/t/unit-tests/t-advise.c
@@ -6,6 +6,7 @@
static const char expect_advice_msg[] = "hint: This is a piece of advice\n"
"hint: Disable this message with \"git config advice.nestedTag false\"\n";
+static const char expect_advice_msg_without_disable_hint[] = "hint: This is a piece of advice\n";
static const char advice_msg[] = "This is a piece of advice";
static const char out_file[] = "./output.txt";
@@ -44,7 +45,7 @@ int cmd_main(int argc, const char **argv) {
TEST(check_advise_if_enabled(advice_msg, NULL, expect_advice_msg),
"advice should be printed when config variable is unset");
- TEST(check_advise_if_enabled(advice_msg, "true", expect_advice_msg),
+ TEST(check_advise_if_enabled(advice_msg, "true", expect_advice_msg_without_disable_hint),
"advice should be printed when config variable is set to true");
TEST(check_advise_if_enabled(advice_msg, "false", ""),
"advice should not be printed when config variable is set to false");
>
> I doubt the issue is not about "picking the right moment" to
> transition from the test-tool to unit testing framework in this
> particular case. Is "check-advice-if-enabled" a bit too high level
> a feature to be a good match to "unit" testing, I have to wonder?
I don't have a formed opinion on the change, but I don't see it making
things worse. I share your doubts, though.
Thanks.
^ permalink raw reply related
* [PATCH 0/3] ci: add support for macOS to GitLab CI
From: Patrick Steinhardt @ 2024-01-15 11:44 UTC (permalink / raw)
To: git
[-- Attachment #1: Type: text/plain, Size: 989 bytes --]
Hi,
this patch series extends GitLab CI to also support macOS. Besides
extending test coverage for GitLab users, this change also has the added
benefit that the macOS runners at GitLab are based on Apple silicon,
which to the best of my knowledge is not something we're currently
testing on.
This patch series builds on top of ps/gitlab-ci-static-analysis
(currently at cd69c635a1 (ci: add job performing static analysis on
GitLab CI, 2023-12-28)) to avoid a conflict.
Patrick
Patrick Steinhardt (3):
ci: make p4 setup on macOS more robust
Makefile: detect new Homebrew location for ARM-based Macs
ci: add macOS jobs to GitLab CI
.gitlab-ci.yml | 26 +++++++++++++++++++++++++-
ci/install-dependencies.sh | 10 ++++------
ci/lib.sh | 12 +++++++++++-
config.mak.uname | 13 +++++++++++++
4 files changed, 53 insertions(+), 8 deletions(-)
base-commit: cd69c635a1a62b0c8bfdbf221778be8a512ad048
--
2.43.GIT
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* [PATCH 1/3] ci: make p4 setup on macOS more robust
From: Patrick Steinhardt @ 2024-01-15 11:44 UTC (permalink / raw)
To: git
In-Reply-To: <cover.1705318985.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 1725 bytes --]
When setting up Perforce on macOS we put both `p4` and `p4d` into
"$HOME/bin". On GitHub CI this directory is indeed contained in the PATH
environment variable and thus there is no need for additional setup than
to put the binaries there. But GitLab CI does not do this, and thus our
Perforce-based tests would be skipped there even though we download the
binaries.
Refactor the setup code to become more robust by downloading binaries
into a separate directory which we then manually append to our PATH.
This matches what we do on Linux-based jobs.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
ci/install-dependencies.sh | 10 ++++------
ci/lib.sh | 3 +++
2 files changed, 7 insertions(+), 6 deletions(-)
diff --git a/ci/install-dependencies.sh b/ci/install-dependencies.sh
index 4f407530d3..b4e22de3cb 100755
--- a/ci/install-dependencies.sh
+++ b/ci/install-dependencies.sh
@@ -37,15 +37,13 @@ macos-*)
test -z "$BREW_INSTALL_PACKAGES" ||
brew install $BREW_INSTALL_PACKAGES
brew link --force gettext
- mkdir -p $HOME/bin
- (
- cd $HOME/bin
+
+ mkdir -p "$P4_PATH"
+ pushd "$P4_PATH"
wget -q "$P4WHENCE/bin.macosx1015x86_64/helix-core-server.tgz" &&
tar -xf helix-core-server.tgz &&
sudo xattr -d com.apple.quarantine p4 p4d 2>/dev/null || true
- )
- PATH="$PATH:${HOME}/bin"
- export PATH
+ popd
if test -n "$CC_PACKAGE"
then
diff --git a/ci/lib.sh b/ci/lib.sh
index c749b21366..f631206a44 100755
--- a/ci/lib.sh
+++ b/ci/lib.sh
@@ -344,6 +344,9 @@ macos-*)
then
MAKEFLAGS="$MAKEFLAGS APPLE_COMMON_CRYPTO_SHA1=Yes"
fi
+
+ P4_PATH="$HOME/custom/p4"
+ export PATH="$P4_PATH:$PATH"
;;
esac
--
2.43.GIT
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* [PATCH 2/3] Makefile: detect new Homebrew location for ARM-based Macs
From: Patrick Steinhardt @ 2024-01-15 11:45 UTC (permalink / raw)
To: git
In-Reply-To: <cover.1705318985.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 1625 bytes --]
With the introduction of the ARM-based Macs the default location for
Homebrew has changed from "/usr/local" to "/opt/homebrew". We only
handle the former location though, which means that unless the user has
manually configured required search paths we won't be able to locate it.
Improve upon this by adding relevant paths to our CFLAGS and LDFLAGS as
well as detecting the location of msgfmt(1).
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
config.mak.uname | 13 +++++++++++++
1 file changed, 13 insertions(+)
diff --git a/config.mak.uname b/config.mak.uname
index 3bb03f423a..dacc95172d 100644
--- a/config.mak.uname
+++ b/config.mak.uname
@@ -158,6 +158,19 @@ ifeq ($(uname_S),Darwin)
ifeq ($(shell test -x /usr/local/opt/gettext/bin/msgfmt && echo y),y)
MSGFMT = /usr/local/opt/gettext/bin/msgfmt
endif
+ # On newer ARM-based machines the default installation path has changed to
+ # /opt/homebrew. Include it in our search paths so that the user does not
+ # have to configure this manually.
+ #
+ # Note that we do not employ the same workaround as above where we manually
+ # add gettext. The issue was fixed more than three years ago by now, and at
+ # that point there haven't been any ARM-based Macs yet.
+ else ifeq ($(shell test -d /opt/homebrew/ && echo y),y)
+ BASIC_CFLAGS += -I/opt/homebrew/include
+ BASIC_LDFLAGS += -L/opt/homebrew/lib
+ ifeq ($(shell test -x /opt/homebrew/bin/msgfmt && echo y),y)
+ MSGFMT = /opt/homebrew/bin/msgfmt
+ endif
endif
# The builtin FSMonitor on MacOS builds upon Simple-IPC. Both require
--
2.43.GIT
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* [PATCH 3/3] ci: add macOS jobs to GitLab CI
From: Patrick Steinhardt @ 2024-01-15 11:45 UTC (permalink / raw)
To: git
In-Reply-To: <cover.1705318985.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 2672 bytes --]
Add two macOS-based jobs to GitLab CI, one for Clang and one for GCC.
This matches equivalent jobs we have for GitHub Workflows, except that
we use macOS 14 instead of macOS 13.
Note that one test marked as `test_must_fail` is surprisingly passing:
t7815-grep-binary.sh (Wstat: 0 Tests: 22 Failed: 0)
TODO passed: 12
This seems to boil down to an unexpected difference in how regcomp(1)
works when matching NUL bytes. Cross-checking with the respective GitHub
job shows though that this is not an issue unique to the GitLab CI job
as it passes in the same way there.
Further note that we do not include the equivalent for the "osx-gcc" job
that we use with GitHub Workflows. This is because the runner for macOS
on GitLab is running on Apple M1 machines and thus uses the "arm64"
architecture. GCC does not support this platform yet.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
.gitlab-ci.yml | 26 +++++++++++++++++++++++++-
ci/lib.sh | 9 ++++++++-
2 files changed, 33 insertions(+), 2 deletions(-)
diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml
index 793243421c..9748970798 100644
--- a/.gitlab-ci.yml
+++ b/.gitlab-ci.yml
@@ -7,7 +7,7 @@ workflow:
- if: $CI_COMMIT_TAG
- if: $CI_COMMIT_REF_PROTECTED == "true"
-test:
+test:linux:
image: $image
before_script:
- ./ci/install-docker-dependencies.sh
@@ -52,6 +52,30 @@ test:
- t/failed-test-artifacts
when: on_failure
+test:osx:
+ image: $image
+ tags:
+ - saas-macos-medium-m1
+ before_script:
+ - ./ci/install-dependencies.sh
+ script:
+ - ./ci/run-build-and-tests.sh
+ after_script:
+ - |
+ if test "$CI_JOB_STATUS" != 'success'
+ then
+ ./ci/print-test-failures.sh
+ fi
+ parallel:
+ matrix:
+ - jobname: osx-clang
+ image: macos-13-xcode-14
+ CC: clang
+ artifacts:
+ paths:
+ - t/failed-test-artifacts
+ when: on_failure
+
static-analysis:
image: ubuntu:22.04
variables:
diff --git a/ci/lib.sh b/ci/lib.sh
index f631206a44..d5dd2f2697 100755
--- a/ci/lib.sh
+++ b/ci/lib.sh
@@ -252,7 +252,14 @@ then
CI_COMMIT="$CI_COMMIT_SHA"
case "$CI_JOB_IMAGE" in
macos-*)
- CI_OS_NAME=osx;;
+ # GitLab CI has Python installed via multiple package managers,
+ # most notably via asdf and Homebrew. Ensure that our builds
+ # pick up the Homebrew one by prepending it to our PATH as the
+ # asdf one breaks tests.
+ export PATH="$(brew --prefix)/bin:$PATH"
+
+ CI_OS_NAME=osx
+ ;;
alpine:*|fedora:*|ubuntu:*)
CI_OS_NAME=linux;;
*)
--
2.43.GIT
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* [PATCH v2] advice: allow disabling the automatic hint in advise_if_enabled()
From: Rubén Justo @ 2024-01-15 14:28 UTC (permalink / raw)
To: Git List, Junio C Hamano, Taylor Blau, Jeff King, Dragan Simic
In-Reply-To: <c870a0b6-9fa8-4d00-a5a6-661ca175805f@gmail.com>
Using advise_if_enabled() to display an advice will automatically
include instructions on how to disable the advice, alongside the main
advice:
hint: use --reapply-cherry-picks to include skipped commits
hint: Disable this message with "git config advice.skippedCherryPicks false"
To do so, we provide a knob which can be used to disable the advice.
But also to tell us the opposite: to show the advice.
Let's not include the deactivation instructions for an advice if the
user explicitly sets its visibility.
Signed-off-by: Rubén Justo <rjusto@gmail.com>
---
This must have been v3, but previous iteration was erroneously sent as a
v1. Sorry.
Range-diff against v1:
1: d280195c7b ! 1: 0bee5d1bba advice: allow disabling the automatic hint in advise_if_enabled()
@@ Commit message
Signed-off-by: Rubén Justo <rjusto@gmail.com>
+ ## Documentation/config/advice.txt ##
+@@
+ advice.*::
+ These variables control various optional help messages designed to
+- aid new users. All 'advice.*' variables default to 'true', and you
+- can tell Git that you do not need help by setting these to 'false':
++ aid new users. When left unconfigured, Git will give the message
++ alongside instructions on how to squelch it. You can tell Git
++ that you do not need the help message by setting these to 'false':
+ +
+ --
+ addEmbeddedRepo::
+
## advice.c ##
@@ advice.c: static const char *advise_get_color(enum color_advice ix)
return "";
}
+enum advice_level {
-+ ADVICE_LEVEL_DEFAULT = 0,
++ ADVICE_LEVEL_NONE = 0,
+ ADVICE_LEVEL_DISABLED,
+ ADVICE_LEVEL_ENABLED,
+};
@@ advice.c: void advise_if_enabled(enum advice_type type, const char *advice, ...)
va_start(params, advice);
- vadvise(advice, 1, advice_setting[type].key, params);
-+ vadvise(advice, advice_setting[type].level == ADVICE_LEVEL_DEFAULT,
-+ advice_setting[type].key, params);
++ vadvise(advice, !advice_setting[type].level, advice_setting[type].key,
++ params);
va_end(params);
}
Documentation/config/advice.txt | 5 +-
advice.c | 109 +++++++++++++++++---------------
t/t0018-advice.sh | 1 -
3 files changed, 62 insertions(+), 53 deletions(-)
diff --git a/Documentation/config/advice.txt b/Documentation/config/advice.txt
index 25c0917524..c7ea70f2e2 100644
--- a/Documentation/config/advice.txt
+++ b/Documentation/config/advice.txt
@@ -1,7 +1,8 @@
advice.*::
These variables control various optional help messages designed to
- aid new users. All 'advice.*' variables default to 'true', and you
- can tell Git that you do not need help by setting these to 'false':
+ aid new users. When left unconfigured, Git will give the message
+ alongside instructions on how to squelch it. You can tell Git
+ that you do not need the help message by setting these to 'false':
+
--
addEmbeddedRepo::
diff --git a/advice.c b/advice.c
index f6e4c2f302..6e9098ff08 100644
--- a/advice.c
+++ b/advice.c
@@ -33,50 +33,56 @@ static const char *advise_get_color(enum color_advice ix)
return "";
}
+enum advice_level {
+ ADVICE_LEVEL_NONE = 0,
+ ADVICE_LEVEL_DISABLED,
+ ADVICE_LEVEL_ENABLED,
+};
+
static struct {
const char *key;
- int enabled;
+ enum advice_level level;
} advice_setting[] = {
- [ADVICE_ADD_EMBEDDED_REPO] = { "addEmbeddedRepo", 1 },
- [ADVICE_ADD_EMPTY_PATHSPEC] = { "addEmptyPathspec", 1 },
- [ADVICE_ADD_IGNORED_FILE] = { "addIgnoredFile", 1 },
- [ADVICE_AMBIGUOUS_FETCH_REFSPEC] = { "ambiguousFetchRefspec", 1 },
- [ADVICE_AM_WORK_DIR] = { "amWorkDir", 1 },
- [ADVICE_CHECKOUT_AMBIGUOUS_REMOTE_BRANCH_NAME] = { "checkoutAmbiguousRemoteBranchName", 1 },
- [ADVICE_COMMIT_BEFORE_MERGE] = { "commitBeforeMerge", 1 },
- [ADVICE_DETACHED_HEAD] = { "detachedHead", 1 },
- [ADVICE_DIVERGING] = { "diverging", 1 },
- [ADVICE_FETCH_SHOW_FORCED_UPDATES] = { "fetchShowForcedUpdates", 1 },
- [ADVICE_FORCE_DELETE_BRANCH] = { "forceDeleteBranch", 1 },
- [ADVICE_GRAFT_FILE_DEPRECATED] = { "graftFileDeprecated", 1 },
- [ADVICE_IGNORED_HOOK] = { "ignoredHook", 1 },
- [ADVICE_IMPLICIT_IDENTITY] = { "implicitIdentity", 1 },
- [ADVICE_NESTED_TAG] = { "nestedTag", 1 },
- [ADVICE_OBJECT_NAME_WARNING] = { "objectNameWarning", 1 },
- [ADVICE_PUSH_ALREADY_EXISTS] = { "pushAlreadyExists", 1 },
- [ADVICE_PUSH_FETCH_FIRST] = { "pushFetchFirst", 1 },
- [ADVICE_PUSH_NEEDS_FORCE] = { "pushNeedsForce", 1 },
- [ADVICE_PUSH_NON_FF_CURRENT] = { "pushNonFFCurrent", 1 },
- [ADVICE_PUSH_NON_FF_MATCHING] = { "pushNonFFMatching", 1 },
- [ADVICE_PUSH_REF_NEEDS_UPDATE] = { "pushRefNeedsUpdate", 1 },
- [ADVICE_PUSH_UNQUALIFIED_REF_NAME] = { "pushUnqualifiedRefName", 1 },
- [ADVICE_PUSH_UPDATE_REJECTED] = { "pushUpdateRejected", 1 },
- [ADVICE_PUSH_UPDATE_REJECTED_ALIAS] = { "pushNonFastForward", 1 }, /* backwards compatibility */
- [ADVICE_RESET_NO_REFRESH_WARNING] = { "resetNoRefresh", 1 },
- [ADVICE_RESOLVE_CONFLICT] = { "resolveConflict", 1 },
- [ADVICE_RM_HINTS] = { "rmHints", 1 },
- [ADVICE_SEQUENCER_IN_USE] = { "sequencerInUse", 1 },
- [ADVICE_SET_UPSTREAM_FAILURE] = { "setUpstreamFailure", 1 },
- [ADVICE_SKIPPED_CHERRY_PICKS] = { "skippedCherryPicks", 1 },
- [ADVICE_STATUS_AHEAD_BEHIND_WARNING] = { "statusAheadBehindWarning", 1 },
- [ADVICE_STATUS_HINTS] = { "statusHints", 1 },
- [ADVICE_STATUS_U_OPTION] = { "statusUoption", 1 },
- [ADVICE_SUBMODULES_NOT_UPDATED] = { "submodulesNotUpdated", 1 },
- [ADVICE_SUBMODULE_ALTERNATE_ERROR_STRATEGY_DIE] = { "submoduleAlternateErrorStrategyDie", 1 },
- [ADVICE_SUGGEST_DETACHING_HEAD] = { "suggestDetachingHead", 1 },
- [ADVICE_UPDATE_SPARSE_PATH] = { "updateSparsePath", 1 },
- [ADVICE_WAITING_FOR_EDITOR] = { "waitingForEditor", 1 },
- [ADVICE_WORKTREE_ADD_ORPHAN] = { "worktreeAddOrphan", 1 },
+ [ADVICE_ADD_EMBEDDED_REPO] = { "addEmbeddedRepo" },
+ [ADVICE_ADD_EMPTY_PATHSPEC] = { "addEmptyPathspec" },
+ [ADVICE_ADD_IGNORED_FILE] = { "addIgnoredFile" },
+ [ADVICE_AMBIGUOUS_FETCH_REFSPEC] = { "ambiguousFetchRefspec" },
+ [ADVICE_AM_WORK_DIR] = { "amWorkDir" },
+ [ADVICE_CHECKOUT_AMBIGUOUS_REMOTE_BRANCH_NAME] = { "checkoutAmbiguousRemoteBranchName" },
+ [ADVICE_COMMIT_BEFORE_MERGE] = { "commitBeforeMerge" },
+ [ADVICE_DETACHED_HEAD] = { "detachedHead" },
+ [ADVICE_DIVERGING] = { "diverging" },
+ [ADVICE_FETCH_SHOW_FORCED_UPDATES] = { "fetchShowForcedUpdates" },
+ [ADVICE_FORCE_DELETE_BRANCH] = { "forceDeleteBranch" },
+ [ADVICE_GRAFT_FILE_DEPRECATED] = { "graftFileDeprecated" },
+ [ADVICE_IGNORED_HOOK] = { "ignoredHook" },
+ [ADVICE_IMPLICIT_IDENTITY] = { "implicitIdentity" },
+ [ADVICE_NESTED_TAG] = { "nestedTag" },
+ [ADVICE_OBJECT_NAME_WARNING] = { "objectNameWarning" },
+ [ADVICE_PUSH_ALREADY_EXISTS] = { "pushAlreadyExists" },
+ [ADVICE_PUSH_FETCH_FIRST] = { "pushFetchFirst" },
+ [ADVICE_PUSH_NEEDS_FORCE] = { "pushNeedsForce" },
+ [ADVICE_PUSH_NON_FF_CURRENT] = { "pushNonFFCurrent" },
+ [ADVICE_PUSH_NON_FF_MATCHING] = { "pushNonFFMatching" },
+ [ADVICE_PUSH_REF_NEEDS_UPDATE] = { "pushRefNeedsUpdate" },
+ [ADVICE_PUSH_UNQUALIFIED_REF_NAME] = { "pushUnqualifiedRefName" },
+ [ADVICE_PUSH_UPDATE_REJECTED] = { "pushUpdateRejected" },
+ [ADVICE_PUSH_UPDATE_REJECTED_ALIAS] = { "pushNonFastForward" }, /* backwards compatibility */
+ [ADVICE_RESET_NO_REFRESH_WARNING] = { "resetNoRefresh" },
+ [ADVICE_RESOLVE_CONFLICT] = { "resolveConflict" },
+ [ADVICE_RM_HINTS] = { "rmHints" },
+ [ADVICE_SEQUENCER_IN_USE] = { "sequencerInUse" },
+ [ADVICE_SET_UPSTREAM_FAILURE] = { "setUpstreamFailure" },
+ [ADVICE_SKIPPED_CHERRY_PICKS] = { "skippedCherryPicks" },
+ [ADVICE_STATUS_AHEAD_BEHIND_WARNING] = { "statusAheadBehindWarning" },
+ [ADVICE_STATUS_HINTS] = { "statusHints" },
+ [ADVICE_STATUS_U_OPTION] = { "statusUoption" },
+ [ADVICE_SUBMODULES_NOT_UPDATED] = { "submodulesNotUpdated" },
+ [ADVICE_SUBMODULE_ALTERNATE_ERROR_STRATEGY_DIE] = { "submoduleAlternateErrorStrategyDie" },
+ [ADVICE_SUGGEST_DETACHING_HEAD] = { "suggestDetachingHead" },
+ [ADVICE_UPDATE_SPARSE_PATH] = { "updateSparsePath" },
+ [ADVICE_WAITING_FOR_EDITOR] = { "waitingForEditor" },
+ [ADVICE_WORKTREE_ADD_ORPHAN] = { "worktreeAddOrphan" },
};
static const char turn_off_instructions[] =
@@ -116,13 +122,13 @@ void advise(const char *advice, ...)
int advice_enabled(enum advice_type type)
{
- switch(type) {
- case ADVICE_PUSH_UPDATE_REJECTED:
- return advice_setting[ADVICE_PUSH_UPDATE_REJECTED].enabled &&
- advice_setting[ADVICE_PUSH_UPDATE_REJECTED_ALIAS].enabled;
- default:
- return advice_setting[type].enabled;
- }
+ int enabled = advice_setting[type].level != ADVICE_LEVEL_DISABLED;
+
+ if (type == ADVICE_PUSH_UPDATE_REJECTED)
+ return enabled &&
+ advice_enabled(ADVICE_PUSH_UPDATE_REJECTED_ALIAS);
+
+ return enabled;
}
void advise_if_enabled(enum advice_type type, const char *advice, ...)
@@ -133,7 +139,8 @@ void advise_if_enabled(enum advice_type type, const char *advice, ...)
return;
va_start(params, advice);
- vadvise(advice, 1, advice_setting[type].key, params);
+ vadvise(advice, !advice_setting[type].level, advice_setting[type].key,
+ params);
va_end(params);
}
@@ -162,7 +169,9 @@ int git_default_advice_config(const char *var, const char *value)
for (i = 0; i < ARRAY_SIZE(advice_setting); i++) {
if (strcasecmp(k, advice_setting[i].key))
continue;
- advice_setting[i].enabled = git_config_bool(var, value);
+ advice_setting[i].level = git_config_bool(var, value)
+ ? ADVICE_LEVEL_ENABLED
+ : ADVICE_LEVEL_DISABLED;
return 0;
}
diff --git a/t/t0018-advice.sh b/t/t0018-advice.sh
index c13057a4ca..0dcfb760a2 100755
--- a/t/t0018-advice.sh
+++ b/t/t0018-advice.sh
@@ -17,7 +17,6 @@ test_expect_success 'advice should be printed when config variable is unset' '
test_expect_success 'advice should be printed when config variable is set to true' '
cat >expect <<-\EOF &&
hint: This is a piece of advice
- hint: Disable this message with "git config advice.nestedTag false"
EOF
test_config advice.nestedTag true &&
test-tool advise "This is a piece of advice" 2>actual &&
base-commit: bec9bb4b3918d2b3c7b91bbb116a667d5d6d298d
--
2.42.0
^ permalink raw reply related
* git-request-get ?
From: Matěj Cepl @ 2024-01-15 16:24 UTC (permalink / raw)
To: git
[-- Attachment #1: Type: text/plain, Size: 2166 bytes --]
Hello,
I am still haunted by my own ancient comment on
https://gitlab.com/gitlab-org/gitlab/-/issues/14116:
> Well, the minimal solution [to federated merge requests]
> would be a parser for the git-request-pull(1), which would
> check every comment and if recognized the comment as the
> pull request, it would add button for the owner of the repo
> (e.g., [Create Pull Request]). After pressing that (so it is
> constantly under the control of the owner repo), gitlab would
> setup new remote (if doesn't exist still), fetch it, and create
> a merge request.
Of course, the part with a button has to be resolved in a
particular web application, but I am still wondering whether
there isn’t any way how to make consuming git-request-pull(1)
generated emails more easily digestable and thus promote the use
of the tool.
First I created rather complicated bash script
(https://da.gd/pSgdc), but then I have read CodingGuidelines and
found that I need to keep myself to the POSIX shell script, so
I have simplified a lot. Besides, I don’t think the complicated
part (like adding remotes) is necessarily a good thing (???).
Currently I have just this:
#!/bin/sh
set -eu
STR="$(cat)"
URL="$(echo "$STR" | sed -n -e '/^are available in the Git repository at:/,+2 {
s/[[:space:]]\+//
s/\(=[[:digit:]]\{2\}\)\+$//
/^\(http\|git\)/p
}')"
END="$(echo "$STR" | awk '/^for you to fetch changes up to / { print $NF }' | sed -e 's/[=:]*$//')"
git fetch "$URL" "$END"
git checkout -B _4review FETCH_HEAD
Do you think this is a good idea at all? Should we be more
complicated or less? Should we do some fun things like parsing
and adding remotes, parsing email addresses or something to
create individualized branch names for review? Stuff like that.
Looking forward to any feedback.
Best,
Matěj
--
http://matej.ceplovi.cz/blog/, @mcepl@floss.social
GPG Finger: 3C76 A027 CA45 AD70 98B5 BC1D 7920 5802 880B C9D8
Those to whom evil is done \ Do evil in return.
-- W. H. Auden, September 1, 1939
http://www.poets.org/viewmedia.php/prmMID/15545
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 216 bytes --]
^ permalink raw reply
* Re: [PATCH v2 1/2] revision: ensure MERGE_HEAD is a ref in prepare_show_merge
From: Junio C Hamano @ 2024-01-15 17:19 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: Michael Lohmann, git, newren, phillip.wood123
In-Reply-To: <ZaUYyEAdr4oTImL-@tanuki>
Patrick Steinhardt <ps@pks.im> writes:
>> Which would mean that we do not need a separate "other_head"
>> variable, and instead can use "MERGE_HEAD".
>
> There is no need for this, is there? We have already resolved the ref to
> an object ID, so why not use that via `add_pending_oid()`?
add_pending_oid() and its callees use the name only for human
consumption (e.g., in error messages), as they all already have the
object name.
^ permalink raw reply
* Re: [Outreachy][PATCH] Port helper/test-advise.c to unit-tests/t-advise.c
From: Junio C Hamano @ 2024-01-15 17:27 UTC (permalink / raw)
To: Rubén Justo; +Cc: Achu Luma, git, christian.couder, Christian Couder
In-Reply-To: <93468f5c-5f62-4f22-85ce-b60621852430@gmail.com>
Rubén Justo <rjusto@gmail.com> writes:
>> To test the effect of setting one configuration variable, and ensure
>> it results in a slightly different advice message output to the
>> standard error stream, "test-tool advice" needs only a single line
>> of patch, but if we started with this version, how much work does it
>> take to run the equivalent test in the other patch if it were to be
>> rebased on top of this change? It won't be just the matter of
>> adding a new TEST(check_advise_if_enabled()) call to cmd_main(),
>> will it?
>
> Maybe something like this will do the trick:
>
> diff --git a/t/unit-tests/t-advise.c b/t/unit-tests/t-advise.c
> index 15df29c955..ac7d2620ef 100644
> --- a/t/unit-tests/t-advise.c
> +++ b/t/unit-tests/t-advise.c
> @@ -6,6 +6,7 @@
>
> static const char expect_advice_msg[] = "hint: This is a piece of advice\n"
> "hint: Disable this message with \"git config advice.nestedTag false\"\n";
> +static const char expect_advice_msg_without_disable_hint[] = "hint: This is a piece of advice\n";
> static const char advice_msg[] = "This is a piece of advice";
> static const char out_file[] = "./output.txt";
Yup, but ...
> @@ -44,7 +45,7 @@ int cmd_main(int argc, const char **argv) {
>
> TEST(check_advise_if_enabled(advice_msg, NULL, expect_advice_msg),
> "advice should be printed when config variable is unset");
> - TEST(check_advise_if_enabled(advice_msg, "true", expect_advice_msg),
> + TEST(check_advise_if_enabled(advice_msg, "true", expect_advice_msg_without_disable_hint),
> "advice should be printed when config variable is set to true");
> TEST(check_advise_if_enabled(advice_msg, "false", ""),
> "advice should not be printed when config variable is set to false");
... I cannot shake this feeling that the next person who comes to
this code and stares at advice.c would be very tempted to "refactor"
the messages, so that there is only one instance of the same string
in advice.c that is passed to TEST() above. After all, you can
change only one place to update the message and avoid triggering
test failures that way, right? But that line of thinking obviously
reduces the value of having tests.
What if messages from plumbing that should not be modified are being
tested with unit tests? These messages are part of contract with
users, and it is very much worth our time to write and maintain the
tests to ensure they will not be modified by accident. Obviously
such a refactoring of test messages to reuse the production strings
would destroy the value of having such a test in the first place.
So, I dunno.
>> I doubt the issue is not about "picking the right moment" to
>> transition from the test-tool to unit testing framework in this
>> particular case. Is "check-advice-if-enabled" a bit too high level
>> a feature to be a good match to "unit" testing, I have to wonder?
>
> I don't have a formed opinion on the change, but I don't see it making
> things worse. I share your doubts, though.
>
> Thanks.
^ permalink raw reply
* Strange behaviour when pushing a commit object to remote's refs/HEAD
From: Pratyush Yadav @ 2024-01-15 19:08 UTC (permalink / raw)
To: git
Hi,
I ran into a strange Magit bug, where when I ran magit-show-refs on a
particular repo it threw an error. The details of the Magit bug are not
very interesting, but when attempting to reproduce it, I also saw git
misbehaving for such repos.
The strange behaviour happens when you push a commit object to remote's
refs/HEAD instead of pushing a symbolic ref. Such a repository can be
found at https://github.com/prati0100/magit-reproducer. I roughly used
the below steps to create such a repo:
$ git init
$ echo 1 > foo && git add foo && git commit
$ echo 2 > bar && git add bar && git commit
$ git push
$ git checkout 79264c3
$ echo 2.1 > bar && git add bar && git commit
$ git push origin 707a3d5:refs/heads/HEAD
Now with such a repo, if you do `git log --all --oneline` it would look
something like:
707a3d5 (origin/HEAD) 2.1
86e1c97 (HEAD -> main, origin/main) 2
79264c3 1
And running `git for-each-ref --format='%(symref:short),%(refname:short),%(refname),%(subject)' refs/remotes/origin` gives:
,origin,refs/remotes/origin/HEAD,2.1
,origin/main,refs/remotes/origin/main,2
All well and good so far. Now delete the repo and attempt to clone it.
This time `git log --all --oneline` gives:
86e1c97 (HEAD -> main, origin/main, origin/HEAD) 2
79264c3 1
And running `git for-each-ref --format='%(symref:short),%(refname:short),%(refname),%(subject)' refs/remotes/origin` gives:
origin/main,origin,refs/remotes/origin/HEAD,2
,origin/main,refs/remotes/origin/main,2
So suddenly the remote's HEAD becomes origin/main (symbolic ref) and the
commit (707a3d5, "2.1") is nowhere to be found. It neither shows up in
`git rev-list --all` nor in `git log --all`. The files and trees
associated with it also do not show up in `git rev-list --all --object`.
Yet if you do `git show 707a3d5` it shows up. So it does exist and did
get cloned, but git cannot properly see it.
Interestingly enough, even the GitHub UI is confused and it won't show
you the repo correctly. It will show the commit (86e1c97, "2") for both
"branches" main and HEAD. cgit's UI [0] seems to work fine with this,
though cloning from cgit still suffers from this bug.
There _is_ a way to clone the repo correctly. If you do:
$ git init magit-reproducer
$ git remote add origin https://github.com/prati0100/magit-reproducer.git
$ git remote update
Now if you do git log --all or git for-each-ref, you see the correct
result.
I don't really know how to fix this but it certainly is a bug in git
since it can't clone the repo correctly. And at least one major Git host
can't display such a repo properly (I haven't tried others).
I used Git v2.40.1 to do most of this but I did compile the latest
master d4dbce1db5 ("The seventh batch") and attempted to clone using it
and I see the same problem.
[0] https://git.kernel.org/pub/scm/linux/kernel/git/pratyush/magit-reproducer.git/
--
Regards,
Pratyush Yadav
^ permalink raw reply
* Re: [Outreachy][PATCH] Port helper/test-advise.c to unit-tests/t-advise.c
From: Rubén Justo @ 2024-01-15 19:24 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Achu Luma, git, christian.couder, Christian Couder
In-Reply-To: <xmqqy1cq4ide.fsf@gitster.g>
On 15-ene-2024 09:27:25, Junio C Hamano wrote:
> Rubén Justo <rjusto@gmail.com> writes:
>
> >> To test the effect of setting one configuration variable, and ensure
> >> it results in a slightly different advice message output to the
> >> standard error stream, "test-tool advice" needs only a single line
> >> of patch, but if we started with this version, how much work does it
> >> take to run the equivalent test in the other patch if it were to be
> >> rebased on top of this change? It won't be just the matter of
> >> adding a new TEST(check_advise_if_enabled()) call to cmd_main(),
> >> will it?
> >
> > Maybe something like this will do the trick:
> >
> > diff --git a/t/unit-tests/t-advise.c b/t/unit-tests/t-advise.c
> > index 15df29c955..ac7d2620ef 100644
> > --- a/t/unit-tests/t-advise.c
> > +++ b/t/unit-tests/t-advise.c
> > @@ -6,6 +6,7 @@
> >
> > static const char expect_advice_msg[] = "hint: This is a piece of advice\n"
> > "hint: Disable this message with \"git config advice.nestedTag false\"\n";
> > +static const char expect_advice_msg_without_disable_hint[] = "hint: This is a piece of advice\n";
> > static const char advice_msg[] = "This is a piece of advice";
> > static const char out_file[] = "./output.txt";
>
> Yup, but ...
>
> > @@ -44,7 +45,7 @@ int cmd_main(int argc, const char **argv) {
> >
> > TEST(check_advise_if_enabled(advice_msg, NULL, expect_advice_msg),
> > "advice should be printed when config variable is unset");
> > - TEST(check_advise_if_enabled(advice_msg, "true", expect_advice_msg),
> > + TEST(check_advise_if_enabled(advice_msg, "true", expect_advice_msg_without_disable_hint),
> > "advice should be printed when config variable is set to true");
> > TEST(check_advise_if_enabled(advice_msg, "false", ""),
> > "advice should not be printed when config variable is set to false");
>
> ... I cannot shake this feeling that the next person who comes to
> this code and stares at advice.c would be very tempted to "refactor"
> the messages, so that there is only one instance of the same string
> in advice.c that is passed to TEST() above. After all, you can
> change only one place to update the message and avoid triggering
> test failures that way, right?
I see. Maybe you're expecting something like:
diff --git a/t/unit-tests/t-advise.c b/t/unit-tests/t-advise.c
index 15df29c955..15e293fa82 100644
--- a/t/unit-tests/t-advise.c
+++ b/t/unit-tests/t-advise.c
@@ -4,14 +4,15 @@
#include "setup.h"
#include "strbuf.h"
-static const char expect_advice_msg[] = "hint: This is a piece of advice\n"
- "hint: Disable this message with \"git config advice.nestedTag false\"\n";
+static const char expect_advice_msg[] = "hint: This is a piece of advice\n";
+static const char expect_hint_msg[] = "hint: Disable this message with \"git config advice.nestedTag false\"\n";
static const char advice_msg[] = "This is a piece of advice";
static const char out_file[] = "./output.txt";
static void check_advise_if_enabled(const char *argv, const char *conf_val, const char *expect) {
FILE *file;
+ const char *hint;
struct strbuf actual = STRBUF_INIT;
if (conf_val)
@@ -32,7 +33,9 @@ static void check_advise_if_enabled(const char *argv, const char *conf_val, cons
return;
}
- check_str(actual.buf, expect);
+ check_str_len(actual.buf, expect, strlen(expect));
+ if (!conf_val && skip_prefix(actual.buf, expect, &hint))
+ check_str_len(hint, expect_hint_msg, strlen(expect_hint_msg));
strbuf_release(&actual);
if (!check(remove(out_file) == 0))
This implies a new check_str_len() helper, which I'm not including here
but it's a trivial copy of check_str() but using strncmp().
Maybe we can turn the screw a little more.
I'm still not sure of the value in the changes in this series, though.
^ permalink raw reply related
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