* Re: [PATCH v2 5/5] completion: add an use __git_compute_second_level_config_vars_for_section
From: Patrick Steinhardt @ 2024-02-08 7:42 UTC (permalink / raw)
To: Philippe Blain via GitGitGadget; +Cc: git, Philippe Blain
In-Reply-To: <a2e792c911e1b9fa77d27ec327f6a9dfe06d4de4.1706534882.git.gitgitgadget@gmail.com>
[-- Attachment #1: Type: text/plain, Size: 2477 bytes --]
On Mon, Jan 29, 2024 at 01:28:01PM +0000, Philippe Blain via GitGitGadget wrote:
> From: Philippe Blain <levraiphilippeblain@gmail.com>
[snip]
> diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
> index 2934ceb7637..0e8fd63bfdb 100644
> --- a/contrib/completion/git-completion.bash
> +++ b/contrib/completion/git-completion.bash
> @@ -2605,6 +2612,15 @@ __git_compute_first_level_config_vars_for_section ()
> printf -v "__git_first_level_config_vars_for_section_${section}" %s "$(echo "$__git_config_vars" | grep -E "^${section}\.[a-z]" | awk -F. '{print $2}')"
> }
>
> +__git_compute_second_level_config_vars_for_section ()
> +{
> + section="$1"
This should be `local section`, as well.
> + __git_compute_config_vars_all
> + local this_section="__git_second_level_config_vars_for_section_${section}"
> + test -n "${!this_section}" ||
> + printf -v "__git_second_level_config_vars_for_section_${section}" %s "$(echo "$__git_config_vars_all" | grep -E "^${section}\.<" | awk -F. '{print $3}')"
> +}
> +
> __git_config_sections=
> __git_compute_config_sections ()
> {
> @@ -2749,10 +2765,13 @@ __git_complete_config_variable_name ()
> done
>
> case "$cur_" in
> - branch.*.*)
> + branch.*.*|guitool.*.*|difftool.*.*|man.*.*|mergetool.*.*|remote.*.*|submodule.*.*|url.*.*)
> local pfx="${cur_%.*}."
> cur_="${cur_##*.}"
> - __gitcomp "remote pushRemote merge mergeOptions rebase" "$pfx" "$cur_" "$sfx"
> + local section="${pfx%.*.}"
> + __git_compute_second_level_config_vars_for_section "${section}"
> + local this_section="__git_second_level_config_vars_for_section_${section}"
> + __gitcomp "${!this_section}" "$pfx" "$cur_" "$sfx"
> return
> ;;
Nice.
[snip]
> diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
> index f28d8f531b7..24ff786b273 100755
> --- a/t/t9902-completion.sh
> +++ b/t/t9902-completion.sh
> @@ -2593,6 +2593,16 @@ test_expect_success 'git config - variable name - __git_compute_first_level_conf
> submodule.recurse Z
> EOF
> '
Missing a newline.
> +test_expect_success 'git config - variable name - __git_compute_second_level_config_vars_for_section' '
> + test_completion "git config branch.main." <<-\EOF
> + branch.main.description Z
> + branch.main.remote Z
> + branch.main.pushRemote Z
> + branch.main.merge Z
> + branch.main.mergeOptions Z
> + branch.main.rebase Z
> + EOF
> +'
Patrick
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH v2 4/5] builtin/help: add --config-all-for-completion
From: Patrick Steinhardt @ 2024-02-08 7:42 UTC (permalink / raw)
To: Philippe Blain via GitGitGadget; +Cc: git, Philippe Blain
In-Reply-To: <d442a039b27820dbd44e604df75ec026b8243d47.1706534882.git.gitgitgadget@gmail.com>
[-- Attachment #1: Type: text/plain, Size: 2709 bytes --]
On Mon, Jan 29, 2024 at 01:28:00PM +0000, Philippe Blain via GitGitGadget wrote:
> From: Philippe Blain <levraiphilippeblain@gmail.com>
>
> There is currently no machine-friendly way to show _all_ configuration
> variables from the command line. 'git help --config' does show them all,
> but it also sets up the pager. 'git help --config-for-completion' omits
> some variables (those containing wildcards, for example) and 'git help
> --config-section-for-completion' shows only top-level section names.
You can invoke `git --no-pager help --config` so that Git does not set
up the pager. Is there a reason why we can't use that?
> In a following commit we will want to have access to a list of all
> configuration variables from the Bash completion script. As such, add a
> new mode for the command, HELP_ACTION_CONFIG_ALL_FOR_COMPLETION,
> triggered by the new option '--config-all-for-completion'. In this mode,
> show all variables, just as HELP_ACTION_CONFIG, but do not set up the
> pager.
>
> Signed-off-by: Philippe Blain <levraiphilippeblain@gmail.com>
> ---
> builtin/help.c | 7 +++++++
> 1 file changed, 7 insertions(+)
>
> diff --git a/builtin/help.c b/builtin/help.c
> index dc1fbe2b986..dacaeb10bf4 100644
> --- a/builtin/help.c
> +++ b/builtin/help.c
> @@ -50,6 +50,7 @@ static enum help_action {
> HELP_ACTION_DEVELOPER_INTERFACES,
> HELP_ACTION_CONFIG_FOR_COMPLETION,
> HELP_ACTION_CONFIG_SECTIONS_FOR_COMPLETION,
> + HELP_ACTION_CONFIG_ALL_FOR_COMPLETION,
> } cmd_mode;
>
> static const char *html_path;
> @@ -86,6 +87,8 @@ static struct option builtin_help_options[] = {
> HELP_ACTION_CONFIG_FOR_COMPLETION, PARSE_OPT_HIDDEN),
> OPT_CMDMODE_F(0, "config-sections-for-completion", &cmd_mode, "",
> HELP_ACTION_CONFIG_SECTIONS_FOR_COMPLETION, PARSE_OPT_HIDDEN),
> + OPT_CMDMODE_F(0, "config-all-for-completion", &cmd_mode, "",
> + HELP_ACTION_CONFIG_ALL_FOR_COMPLETION, PARSE_OPT_HIDDEN),
>
> OPT_END(),
> };
> @@ -670,6 +673,10 @@ int cmd_help(int argc, const char **argv, const char *prefix)
> opt_mode_usage(argc, "--config-for-completion", help_format);
> list_config_help(SHOW_CONFIG_VARS);
> return 0;
> + case HELP_ACTION_CONFIG_ALL_FOR_COMPLETION:
> + opt_mode_usage(argc, "--config-all-for-completion", help_format);
> + list_config_help(SHOW_CONFIG_HUMAN);
> + return 0;
> case HELP_ACTION_USER_INTERFACES:
> opt_mode_usage(argc, "--user-interfaces", help_format);
> list_user_interfaces_help();
We should add a testcase to "t0012-help.sh" to exercise this new
feature. That would also help show the reviewer what exactly it will end
up printing.
Patrick
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH v2 3/5] completion: add and use __git_compute_first_level_config_vars_for_section
From: Patrick Steinhardt @ 2024-02-08 7:42 UTC (permalink / raw)
To: Philippe Blain via GitGitGadget; +Cc: git, Philippe Blain
In-Reply-To: <838aabf2858b73361be8e8579bc80826e1cfd4c3.1706534882.git.gitgitgadget@gmail.com>
[-- Attachment #1: Type: text/plain, Size: 6400 bytes --]
On Mon, Jan 29, 2024 at 01:27:59PM +0000, Philippe Blain via GitGitGadget wrote:
> From: Philippe Blain <levraiphilippeblain@gmail.com>
>
> The function __git_complete_config_variable_name in the Bash completion
> script hardcodes several config variable names. These variables are
> those in config section where user-defined names can appear, such as
> "branch.<name>". These sections are treated first by the case statement,
> and the two last "catch all" cases are used for other sections, making
> use of the __git_compute_config_vars and __git_compute_config_sections
> function, which omit listing any variables containing wildcards or
> placeholders. Having hardcoded config variables introduces the risk of
> the completion code becoming out of sync with the actual config
> variables accepted by Git.
>
> To avoid these hardcoded config variables, introduce a new function,
> __git_compute_first_level_config_vars_for_section, making use of the
> existing __git_config_vars variable. This function takes as argument a
> config section name and computes the matching "first level" config
> variables for that section, i.e. those _not_ containing any placeholder,
> like 'branch.autoSetupMerge, 'remote.pushDefault', etc. Use this
> function and the variables it defines in the 'branch.*', 'remote.*' and
> 'submodule.*' switches of the case statement instead of hardcoding the
> corresponding config variables. Note that we use indirect expansion
> instead of associative arrays because those are not supported in Bash 3,
> on which macOS is stuck for licensing reasons.
>
> Add a test to make sure the new function works correctly by verfying it
> lists all 'submodule' config variables. This has the downside that this
> test must be updated when new 'submodule' configuration are added, but
> this should be a small burden since it happens infrequently.
>
> Signed-off-by: Philippe Blain <levraiphilippeblain@gmail.com>
> ---
> contrib/completion/git-completion.bash | 24 +++++++++++++++++++++---
> t/t9902-completion.sh | 11 +++++++++++
> 2 files changed, 32 insertions(+), 3 deletions(-)
>
> diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
> index 8af9bc3f4e1..2934ceb7637 100644
> --- a/contrib/completion/git-completion.bash
> +++ b/contrib/completion/git-completion.bash
> @@ -2596,6 +2596,15 @@ __git_compute_config_vars ()
> __git_config_vars="$(git help --config-for-completion)"
> }
>
> +__git_compute_first_level_config_vars_for_section ()
> +{
> + section="$1"
Section needs to be `local`, right?
> + __git_compute_config_vars
> + local this_section="__git_first_level_config_vars_for_section_${section}"
> + test -n "${!this_section}" ||
> + printf -v "__git_first_level_config_vars_for_section_${section}" %s "$(echo "$__git_config_vars" | grep -E "^${section}\.[a-z]" | awk -F. '{print $2}')"
> +}
I've been wondering a bit why we store the result in a global variable.
The value certainly isn't reused in the completion scripts here. It took
me quite some time to realize though that it's going to end up in the
user's shell environment even after completion finishes so that it can
be reused the next time we invoke the completion function.
While this does feel a tad weird to me to be stateful like this across
completion calls, we use the same pattern for `__git_config_vars` and
`__git_config_sections`. So I guess it should be fine given that there
is precedent.
> __git_config_sections=
> __git_compute_config_sections ()
> {
> @@ -2749,8 +2758,11 @@ __git_complete_config_variable_name ()
> branch.*)
> local pfx="${cur_%.*}."
> cur_="${cur_#*.}"
> + local section="${pfx%.}"
> __gitcomp_direct "$(__git_heads "$pfx" "$cur_" ".")"
> - __gitcomp_nl_append $'autoSetupMerge\nautoSetupRebase\n' "$pfx" "$cur_" "${sfx:- }"
> + __git_compute_first_level_config_vars_for_section "${section}"
> + local this_section="__git_first_level_config_vars_for_section_${section}"
> + __gitcomp_nl_append "${!this_section}" "$pfx" "$cur_" "${sfx:- }"
> return
> ;;
> guitool.*.*)
> @@ -2799,8 +2811,11 @@ __git_complete_config_variable_name ()
> remote.*)
> local pfx="${cur_%.*}."
> cur_="${cur_#*.}"
> + local section="${pfx%.}"
> __gitcomp_nl "$(__git_remotes)" "$pfx" "$cur_" "."
> - __gitcomp_nl_append "pushDefault" "$pfx" "$cur_" "${sfx:- }"
> + __git_compute_first_level_config_vars_for_section "${section}"
> + local this_section="__git_first_level_config_vars_for_section_${section}"
> + __gitcomp_nl_append "${!this_section}" "$pfx" "$cur_" "${sfx:- }"
> return
> ;;
> submodule.*.*)
> @@ -2812,8 +2827,11 @@ __git_complete_config_variable_name ()
> submodule.*)
> local pfx="${cur_%.*}."
> cur_="${cur_#*.}"
> + local section="${pfx%.}"
> __gitcomp_nl "$(__git config -f "$(__git rev-parse --show-toplevel)/.gitmodules" --get-regexp 'submodule.*.path' | awk -F. '{print $2}')" "$pfx" "$cur_" "."
> - __gitcomp_nl_append $'alternateErrorStrategy\nfetchJobs\nactive\nalternateLocation\nrecurse\npropagateBranches' "$pfx" "$cur_" "${sfx:- }"
> + __git_compute_first_level_config_vars_for_section "${section}"
> + local this_section="__git_first_level_config_vars_for_section_${section}"
> + __gitcomp_nl_append "${!this_section}" "$pfx" "$cur_" "${sfx:- }"
> return
> ;;
> url.*.*)
> diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
> index 35eb534fdda..f28d8f531b7 100755
> --- a/t/t9902-completion.sh
> +++ b/t/t9902-completion.sh
> @@ -2583,6 +2583,17 @@ test_expect_success 'git config - variable name include' '
> EOF
> '
>
> +test_expect_success 'git config - variable name - __git_compute_first_level_config_vars_for_section' '
> + test_completion "git config submodule." <<-\EOF
> + submodule.active Z
> + submodule.alternateErrorStrategy Z
> + submodule.alternateLocation Z
> + submodule.fetchJobs Z
> + submodule.propagateBranches Z
> + submodule.recurse Z
> + EOF
> +'
> +
Shouldn't we verify that we know to complete both first-level config
vars as well as the user-specified submodule names here?
Patrick
> test_expect_success 'git config - value' '
> test_completion "git config color.pager " <<-\EOF
> false Z
> --
> gitgitgadget
>
>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH v2 2/5] completion: complete 'submodule.*' config variables
From: Patrick Steinhardt @ 2024-02-08 7:42 UTC (permalink / raw)
To: Philippe Blain via GitGitGadget; +Cc: git, Philippe Blain
In-Reply-To: <426374ff9b3820512f73ef094f9533e6a1ea5cad.1706534882.git.gitgitgadget@gmail.com>
[-- Attachment #1: Type: text/plain, Size: 2466 bytes --]
On Mon, Jan 29, 2024 at 01:27:58PM +0000, Philippe Blain via GitGitGadget wrote:
> From: Philippe Blain <levraiphilippeblain@gmail.com>
>
> In the Bash completion script, function
> __git_complete_config_variable_name completes config variables and has
> special logic to deal with config variables involving user-defined
> names, like branch.<name>.* and remote.<name>.*.
>
> This special logic is missing for submodule-related config variables.
> Add the appropriate branches to the case statement, making use of the
> in-tree '.gitmodules' to list relevant submodules.
>
> Signed-off-by: Philippe Blain <levraiphilippeblain@gmail.com>
> ---
> contrib/completion/git-completion.bash | 13 +++++++++++++
> 1 file changed, 13 insertions(+)
>
> diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
> index 159a4fd8add..8af9bc3f4e1 100644
> --- a/contrib/completion/git-completion.bash
> +++ b/contrib/completion/git-completion.bash
> @@ -2803,6 +2803,19 @@ __git_complete_config_variable_name ()
> __gitcomp_nl_append "pushDefault" "$pfx" "$cur_" "${sfx:- }"
> return
> ;;
> + submodule.*.*)
> + local pfx="${cur_%.*}."
> + cur_="${cur_##*.}"
> + __gitcomp "url update branch fetchRecurseSubmodules ignore active" "$pfx" "$cur_" "$sfx"
> + return
> + ;;
> + submodule.*)
> + local pfx="${cur_%.*}."
> + cur_="${cur_#*.}"
> + __gitcomp_nl "$(__git config -f "$(__git rev-parse --show-toplevel)/.gitmodules" --get-regexp 'submodule.*.path' | awk -F. '{print $2}')" "$pfx" "$cur_" "."
> + __gitcomp_nl_append $'alternateErrorStrategy\nfetchJobs\nactive\nalternateLocation\nrecurse\npropagateBranches' "$pfx" "$cur_" "${sfx:- }"
> + return
> + ;;
Hm, it feels quite awkward that we have to manually massage the
gitmodules config like this. But the closest tool I could find is
`git submodule status`, which would also end up describing commits in
each of the submodules and thus do needless work. And second, it prints
submodule paths and not submodule names, so it surfaces the wrong info
in the first place.
Ideally, we would create such a tool that makes the information more
accessible to us. But that certainly seems out of scope of this patch
series.
In any case though it would be nice to add some tests for these new
completions.
Patrick
> url.*.*)
> local pfx="${cur_%.*}."
> cur_="${cur_##*.}"
> --
> gitgitgadget
>
>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH v2 0/5] completion: remove hardcoded config variable names
From: Patrick Steinhardt @ 2024-02-08 7:42 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Philippe Blain via GitGitGadget, Philippe Blain
In-Reply-To: <xmqq8r3w53nc.fsf@gitster.g>
[-- Attachment #1: Type: text/plain, Size: 1172 bytes --]
On Wed, Feb 07, 2024 at 02:08:07PM -0800, Junio C Hamano wrote:
> "Philippe Blain via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
> > Changes since v1:
> >
> > * Corrected my email in PATCH 2/5 (sorry for the noise)
> >
> > v1: This series removes hardcoded config variable names in the
> > __git_complete_config_variable_name function, partly by adding a new mode to
> > 'git help'. It also adds completion for 'submodule.*' config variables,
> > which were previously missing.
> >
> > I think it makes sense to do that in the same series since it's closely
> > related, and splitting it would result in textual conflicts between both
> > series if one does not build on top of the other, but I'm open to other
> > suggestions.
> >
> > Thanks,
>
> Neither rounds of this series unfortunately got any review.
> Comments from anybody interested in helping to improve completion
> scripts?
Well, I've spent some time with Bash completion recently, so let me give
it a go. I was trying to avoid the dense Bash logic and thus shied away
from reviewing it. But the end result of having less hardcoded values is
quite nice indeed.
Patrick
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH 1/2] bisect: document "terms" subcommand more fully
From: Kristoffer Haugsbakk @ 2024-02-08 6:36 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Matthieu Moy, git
In-Reply-To: <20240207214436.538586-2-gitster@pobox.com>
On Wed, Feb 7, 2024, at 22:44, Junio C Hamano wrote:
> The documentation for "git bisect terms", although it did not hide
> any information, was a bit incomplete and forced readers to fill in
> the blanks to get the complete picture.
>
> Acked-by: Matthieu Moy <git@matthieu-moy.fr>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
Past tense? How about:
The documentation for "git bisect terms"---although it does not hide
any information---is a bit incomplete and forces readers to fill in
the blanks to get the complete picture.
--
Kristoffer Haugsbakk
^ permalink raw reply
* Re: [PATCH v2 00/30] initial support for multiple hash functions
From: Patrick Steinhardt @ 2024-02-08 6:11 UTC (permalink / raw)
To: Linus Arver
Cc: Junio C Hamano, git, Eric W. Biederman, brian m. carlson,
Eric Sunshine, Eric W. Biederman
In-Reply-To: <owlymssbn6qa.fsf@fine.c.googlers.com>
[-- Attachment #1: Type: text/plain, Size: 1423 bytes --]
On Wed, Feb 07, 2024 at 04:24:13PM -0800, Linus Arver wrote:
> Junio C Hamano <gitster@pobox.com> writes:
>
> > "Eric W. Biederman" <ebiederm@gmail.com> writes:
> >
> >> This addresses all of the known test failures from v1 of this set of
> >> changes. In particular I have reworked commit_tree_extended which
> >> was flagged by smatch, -Werror=array-bounds, and the leak detector.
> >>
> >> One functional bug was fixed in repo_for_each_abbrev where it was
> >> mistakenly displaying too many ambiguous oids.
> >>
> >> I am posting this so that people review and testing of this patchset
> >> won't be distracted by the known and fixed issues.
> >
> > We haven't seen any reviews on this second round, and have had it
> > outside 'next' for too long. I am tempted to say that we merge it
> > to 'next' and see if anybody screams at this point.
>
> FWIW out of all the "Needs review" topics this one seemed like the most
> deserving of another pair of eyes, and I was planning to review some of
> the patches here this week + the weekend. If my review takes too long
> (taking longer than this weekend) I can give another update next week
> saying "too hard for me, please don't wait for me" to unblock you from
> merging to next.
I completely lost track of this patch series. So same for me: I don't
want to hold it up, but would be happy to give it a pair of eyes next
week.
Patrick
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [RFH] "git -C there add foo" completes, s/add/diff/ does not
From: Junio C Hamano @ 2024-02-08 6:04 UTC (permalink / raw)
To: Kyle Lippincott; +Cc: git
In-Reply-To: <CAO_smVjnknv1ePTHhDNKK=C_iEg6+T0nNwaXqA67QuPd6tBkxw@mail.gmail.com>
Kyle Lippincott <spectral@google.com> writes:
>> ...
>> But the same does not work for the step before I can decide to
>> actually "add" the contents, which is to "diff", i.e.
>>
>> $ git -C Meta diff whats-<TAB>
>>
>> does not complete.
>
> I'm not a completions expert, but I think what's happening is that the
> completions for `git diff` aren't producing anything, so it (where
> "it" here might be the shell?) falls back to just doing normal path
> completion.
Yes, that seems to be the case.
> For `git add`, it's checking the `git status` output to
> filter the list to things that need to be added,...
Not exactly, but a close enough description, I think.
__git_complete_index_file does not run `git status` but asks
ls-files the paths it knows about (including "--others" so that a
path that is untracked can become a candidate to be added), then
massages the list of paths with a custom awk script.
For "git diff", depending on what two sets of contents are being
compared, the source of possible paths may differ, but the list of
paths obtained from ls-files (without --others) would be appropriate
when comparing the index and the working tree files, or comparing a
tree-ish and the working tree files. The necessary ingredient to do
so may be pretty much the same as what is used by _git_add completion.
When comparing two tree-ishes, the candidate would ideally come from
union of paths in these two tree-ishes, but I offhand do not know if
there already is a support for choosing from such a set.
Thanks.
^ permalink raw reply
* Re: [PATCH] tag: fix sign_buffer() call to create a signed tag
From: Junio C Hamano @ 2024-02-08 5:29 UTC (permalink / raw)
To: Jeff King; +Cc: git, Sergey Kosukhin
In-Reply-To: <xmqq5xyzr6tm.fsf@gitster.g>
Junio C Hamano <gitster@pobox.com> writes:
> We could do belt and suspenders by tightening the other callers to
> only expect negative for errors (but then what should they do when
> they receive non-zero positive? Should they BUG() out???) while
> teaching sign_buffer_ssh() that our convention is to return negative
> for an error, of course, but I am not sure if it that is worth it.
Actually, we could loosen the caller(s) while tightening the
callee(s), which is the more usual approach we would take in a
situation like this. Here is what I am tempted to pile on top of
the patch.
----- >8 --------- >8 --------- >8 --------- >8 --------- >8 -----
Subject: [PATCH] ssh signing: signal an error with a negative return value
The other backend for the sign_buffer() function followed our usual
"an error is signalled with a negative return" convention, but the
SSH signer did not. Even though we already fixed the caller that
assumed only a negative return value is an error, tighten the callee
to signal an error with a negative return as well. This way, the
callees will be strict on what they produce, while the callers will
be lenient in what they accept.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
gpg-interface.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/gpg-interface.c b/gpg-interface.c
index 48f43c5a21..e19a69c400 100644
--- a/gpg-interface.c
+++ b/gpg-interface.c
@@ -1088,7 +1088,7 @@ static int sign_buffer_ssh(struct strbuf *buffer, struct strbuf *signature,
if (strstr(signer_stderr.buf, "usage:"))
error(_("ssh-keygen -Y sign is needed for ssh signing (available in openssh version 8.2p1+)"));
- error("%s", signer_stderr.buf);
+ ret = error("%s", signer_stderr.buf);
goto out;
}
--
2.43.0-561-g235986be82
^ permalink raw reply related
* [PATCH] refs/reftable: fix leak when copying reflog fails
From: Patrick Steinhardt @ 2024-02-08 5:26 UTC (permalink / raw)
To: git; +Cc: Jeff King
[-- Attachment #1: Type: text/plain, Size: 998 bytes --]
When copying a ref with the reftable backend we also copy the
corresponding log records. When seeking the first log record that we're
about to copy fails though we directly return from `write_copy_table()`
without doing any cleanup, leaking several allocated data structures.
Fix this by exiting via our common cleanup logic instead.
Reported-by: Jeff King <peff@peff.net> via Coverity
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
refs/reftable-backend.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/refs/reftable-backend.c b/refs/reftable-backend.c
index 85214baa60..a14f2ad7f4 100644
--- a/refs/reftable-backend.c
+++ b/refs/reftable-backend.c
@@ -1503,7 +1503,7 @@ static int write_copy_table(struct reftable_writer *writer, void *cb_data)
*/
ret = reftable_merged_table_seek_log(mt, &it, arg->oldname);
if (ret < 0)
- return ret;
+ goto done;
while (1) {
ret = reftable_iterator_next_log(&it, &old_log);
--
2.43.GIT
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* Re: [PATCH v4 1/2] refs: introduce reftable backend
From: Patrick Steinhardt @ 2024-02-08 5:11 UTC (permalink / raw)
To: Jeff King; +Cc: git, Junio C Hamano, Han-Wen Nienhuys, Karthik Nayak
In-Reply-To: <20240207223120.GA537741@coredump.intra.peff.net>
[-- Attachment #1: Type: text/plain, Size: 2495 bytes --]
On Wed, Feb 07, 2024 at 05:31:20PM -0500, Jeff King wrote:
> On Wed, Feb 07, 2024 at 08:20:31AM +0100, Patrick Steinhardt wrote:
>
> > +static int write_copy_table(struct reftable_writer *writer, void *cb_data)
> > +{
> > [...]
> > + /*
> > + * Create the reflog entry for the newly created branch.
> > + */
> > + ALLOC_GROW(logs, logs_nr + 1, logs_alloc);
> > + memset(&logs[logs_nr], 0, sizeof(logs[logs_nr]));
> > + fill_reftable_log_record(&logs[logs_nr]);
> > + logs[logs_nr].refname = (char *)arg->newname;
> > + logs[logs_nr].update_index = creation_ts;
> > + logs[logs_nr].value.update.message =
> > + xstrndup(arg->logmsg, arg->refs->write_options.block_size / 2);
> > + logs[logs_nr].value.update.new_hash = old_ref.value.val1;
> > + logs_nr++;
> > +
> > + /*
> > + * In addition to writing the reflog entry for the new branch, we also
> > + * copy over all log entries from the old reflog. Last but not least,
> > + * when renaming we also have to delete all the old reflog entries.
> > + */
> > + ret = reftable_merged_table_seek_log(mt, &it, arg->oldname);
> > + if (ret < 0)
> > + return ret;
>
> Should this last line be "goto done" as is used elsewhere in the
> function? Otherwise we are at least leaking the "logs" array (and
> possibly some of the other cleanup is important, too).
Indeed we should.
> > + while (1) {
> > + ret = reftable_iterator_next_log(&it, &old_log);
> > + if (ret < 0)
> > + goto done;
> > + if (ret > 0 || strcmp(old_log.refname, arg->oldname)) {
> > + ret = 0;
> > + break;
> > + }
>
> This "ret = 0" doesn't have any effect. We break out of the loop, and
> then...
>
> > + }
> > +
> > + ret = reftable_writer_add_logs(writer, logs, logs_nr);
> > + if (ret < 0)
> > + goto done;
>
> ...the first thing we do is write over it. I dunno if it's worth keeping
> as a maintenance precaution, though (if the code after the loop changed
> to omit that assignment, then setting "ret" would become important).
Yeah, I think this one we should keep. It's also a repeating pattern
that we have in many other places, so it helps lower the mental burden
when it looks similar to all the others.
> Both were noticed by Coverity (along with several other false
> positives).
Is the Coverity instance publicly accessible? If not, can you maybe
invite me so that I get a better feeling for them?
I'll send a patch to fix the missing `goto done` above, thanks!
Patrick
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH] ref-filter.c: sort formatted dates by byte value
From: Junio C Hamano @ 2024-02-08 3:11 UTC (permalink / raw)
To: Victoria Dye via GitGitGadget; +Cc: git, Victoria Dye
In-Reply-To: <pull.1655.git.1707357439586.gitgitgadget@gmail.com>
"Victoria Dye via GitGitGadget" <gitgitgadget@gmail.com> writes:
> Leaving the default (unformatted) date sorting unchanged, sorting by the
> formatted date string adds some flexibility to 'for-each-ref' by allowing
> for behavior like "sort by year, then by refname within each year" or "sort
> by time of day".
Hmph, what a strange use case, but understandable.
> I came across a use case for 'git for-each-ref' at $DAYJOB in which I'd
> want to sort by a portion of a formatted 'creatordate' (e.g., only the
> time of day, sans date). When I tried to run something like 'git
> for-each-ref --sort=creatordate:format:%H:%M:%S',
Hmph, this indeed is interesting ;-)
I wonder if there are other "sort by numeric but the thing could be
stringified by the end-user" atoms offered by for-each-ref
machinery. IOW, is the timestamp the only thing that needs this
fix?
Thanks.
^ permalink raw reply
* Re: [PATCH] tag: fix sign_buffer() call to create a signed tag
From: Junio C Hamano @ 2024-02-08 3:08 UTC (permalink / raw)
To: Jeff King; +Cc: git, Sergey Kosukhin
In-Reply-To: <20240208004757.GA1059751@coredump.intra.peff.net>
Jeff King <peff@peff.net> writes:
>> * We alternatively could fix individual sign_buffer() backend that
>> signals an error with a positive value (sign_buffer_ssh() in this
>> case) to return a negative value, but this would hopefully be
>> more future-proof.
>
> FWIW, I would have gone the other way, and fixed sign_buffer_ssh(). Your
> solution here is future-proofing the tag code against other
> sign_buffer_*() functions behaving like ssh. But it is also leaving
> other sign_buffer() callers to introduce the same bug.
>
> Your documentation change at least makes that less likely. But given how
> much of our code uses the "negative is error" convention, I wouldn't be
> surprised to see it happen anyway.
Yeah, but other callers are prepared to honor the current return
value convention used by gpg-interface, so "fixing" sign_buffer_ssh()
would not give us any future-proofing.
We could do belt and suspenders by tightening the other callers to
only expect negative for errors (but then what should they do when
they receive non-zero positive? Should they BUG() out???) while
teaching sign_buffer_ssh() that our convention is to return negative
for an error, of course, but I am not sure if it that is worth it.
Thanks.
^ permalink raw reply
* Re: [RFH] "git -C there add foo" completes, s/add/diff/ does not
From: Kyle Lippincott @ 2024-02-08 2:50 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <xmqqcyt89l7z.fsf@gitster.g>
On Wed, Feb 7, 2024 at 10:35 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> As some of you may already know, I keep an untracked directory
> called "Meta" at the top-level of the working tree of the Git
> source tree. This "Meta" directory is actually a single-branch
> clone of the git.kernel.org/pub/scm/git/git.git that checks out
> its "todo" branch, where files like whats-cooking.txt lives.
>
> So, what I often would do is
>
> $ git -C Meta add whats-cooking.txt
>
> after updating the draft of the next issue of the "What's cooking"
> report. The command line completion support for "git add" knows how
> to complete this when I stopped typing the above after whats-" and
> hit <TAB>. It seems that __git_find_repo_path helper function that
> notices "-C there" and discovers the $GIT_DIR, and _git_add helper
> uses __git_complete_index_file that honors the discovered $GIT_DIR
> to find paths in the correct index, which is wonderful.
>
> But the same does not work for the step before I can decide to
> actually "add" the contents, which is to "diff", i.e.
>
> $ git -C Meta diff whats-<TAB>
>
> does not complete.
I'm not a completions expert, but I think what's happening is that the
completions for `git diff` aren't producing anything, so it (where
"it" here might be the shell?) falls back to just doing normal path
completion. For `git add`, it's checking the `git status` output to
filter the list to things that need to be added, so it respects the
`-C` option when calling into git to get that list, but there's no
such logic for `git diff` (the git-specific logic treats the
[optional] positional argument as a ref, not a file).
>
> Anybody wants to take a crack at it?
>
> Thanks.
>
>
^ permalink raw reply
* [PATCH] ref-filter.c: sort formatted dates by byte value
From: Victoria Dye via GitGitGadget @ 2024-02-08 1:57 UTC (permalink / raw)
To: git; +Cc: Victoria Dye, Victoria Dye
From: Victoria Dye <vdye@github.com>
Update the ref sorting functions of 'ref-filter.c' so that when date fields
are specified with a format string (such as in 'git for-each-ref
--sort=creatordate:<something>'), they are sorted by their formatted string
value rather than by the underlying numeric timestamp. Currently, date
fields are always sorted by timestamp, regardless of whether formatting
information is included in the '--sort' key.
Leaving the default (unformatted) date sorting unchanged, sorting by the
formatted date string adds some flexibility to 'for-each-ref' by allowing
for behavior like "sort by year, then by refname within each year" or "sort
by time of day". Because the inclusion of a format string previously had no
effect on sort behavior, this change likely will not affect existing usage
of 'for-each-ref' or other ref listing commands.
Additionally, update documentation & tests to document the new sorting
mechanism.
Signed-off-by: Victoria Dye <vdye@github.com>
---
ref-filter.c: sort formatted dates by byte value
I came across a use case for 'git for-each-ref' at $DAYJOB in which I'd
want to sort by a portion of a formatted 'creatordate' (e.g., only the
time of day, sans date). When I tried to run something like 'git
for-each-ref --sort=creatordate:format:%H:%M:%S', though, I was
surprised to find that the refs were still sorted according to the full
date/time value (as they would be with '--sort=creatordate').
This patch attempts to make date-based sorting a bit more flexible by
ordering based on the formatted date string if and only if a custom
format is specified. The implementation is fairly simple (manually set
the comparison type to 'FIELD_STR' if the format string is not null),
but I'm interested in hearing from reviewers whether this seems like a
reasonable extension to 'git for-each-ref --sort', or if there's another
(better) way to add this kind of functionality.
Thanks!
* Victoria
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1655%2Fvdye%2Fvdye%2Ffor-each-ref-date-sorting-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1655/vdye/vdye/for-each-ref-date-sorting-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1655
Documentation/git-for-each-ref.txt | 8 ++++--
ref-filter.c | 6 ++++
t/t6300-for-each-ref.sh | 46 ++++++++++++++++++++++++++++++
3 files changed, 57 insertions(+), 3 deletions(-)
diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt
index be9543f6840..3a9ad91b7af 100644
--- a/Documentation/git-for-each-ref.txt
+++ b/Documentation/git-for-each-ref.txt
@@ -359,9 +359,11 @@ In any case, a field name that refers to a field inapplicable to
the object referred by the ref does not cause an error. It
returns an empty string instead.
-As a special case for the date-type fields, you may specify a format for
-the date by adding `:` followed by date format name (see the
-values the `--date` option to linkgit:git-rev-list[1] takes).
+As a special case for the date-type fields, you may specify a format for the
+date by adding `:` followed by date format name (see the values the `--date`
+option to linkgit:git-rev-list[1] takes). If this formatting is provided in
+a `--sort` key, references will be sorted according to the byte-value of the
+formatted string rather than the numeric value of the underlying timestamp.
Some atoms like %(align) and %(if) always require a matching %(end).
We call them "opening atoms" and sometimes denote them as %($open).
diff --git a/ref-filter.c b/ref-filter.c
index 35b989e1dfe..be14b56e324 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -1611,6 +1611,12 @@ static void grab_date(const char *buf, struct atom_value *v, const char *atomnam
if (formatp) {
formatp++;
parse_date_format(formatp, &date_mode);
+
+ /*
+ * If this is a sort field and a format was specified, we'll
+ * want to compare formatted date by string value.
+ */
+ v->atom->type = FIELD_STR;
}
if (!eoemail)
diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh
index 843a7fe1431..eb6c8204e8b 100755
--- a/t/t6300-for-each-ref.sh
+++ b/t/t6300-for-each-ref.sh
@@ -1356,6 +1356,52 @@ test_expect_success '--no-sort without subsequent --sort prints expected refs' '
test_cmp expected actual
'
+test_expect_success 'set up custom date sorting' '
+ # Dates:
+ # - Wed Feb 07 2024 21:34:20 +0000
+ # - Tue Dec 14 1999 00:05:22 +0000
+ # - Fri Jun 04 2021 11:26:51 +0000
+ # - Mon Jan 22 2007 16:44:01 GMT+0000
+ i=1 &&
+ for when in 1707341660 945129922 1622806011 1169484241
+ do
+ GIT_COMMITTER_DATE="@$when +0000" \
+ GIT_COMMITTER_EMAIL="user@example.com" \
+ git tag -m "tag $when" custom-dates-$i &&
+ i=$(($i+1)) || return 1
+ done
+'
+
+test_expect_success 'sort by date defaults to full timestamp' '
+ cat >expected <<-\EOF &&
+ 945129922 refs/tags/custom-dates-2
+ 1169484241 refs/tags/custom-dates-4
+ 1622806011 refs/tags/custom-dates-3
+ 1707341660 refs/tags/custom-dates-1
+ EOF
+
+ git for-each-ref \
+ --format="%(creatordate:unix) %(refname)" \
+ --sort=creatordate \
+ "refs/tags/custom-dates-*" >actual &&
+ test_cmp expected actual
+'
+
+test_expect_success 'sort by custom date format' '
+ cat >expected <<-\EOF &&
+ 00:05:22 refs/tags/custom-dates-2
+ 11:26:51 refs/tags/custom-dates-3
+ 16:44:01 refs/tags/custom-dates-4
+ 21:34:20 refs/tags/custom-dates-1
+ EOF
+
+ git for-each-ref \
+ --format="%(creatordate:format:%H:%M:%S) %(refname)" \
+ --sort="creatordate:format:%H:%M:%S" \
+ "refs/tags/custom-dates-*" >actual &&
+ test_cmp expected actual
+'
+
test_expect_success 'do not dereference NULL upon %(HEAD) on unborn branch' '
test_when_finished "git checkout main" &&
git for-each-ref --format="%(HEAD) %(refname:short)" refs/heads/ >actual &&
base-commit: 235986be822c9f8689be2e9a0b7804d0b1b6d821
--
gitgitgadget
^ permalink raw reply related
* Re: [PATCH] commit.c: ensure strchrnul() doesn't scan beyond range
From: Jeff King @ 2024-02-08 1:00 UTC (permalink / raw)
To: René Scharfe
Cc: Chandra Pratap via GitGitGadget, git, Chandra Pratap,
Chandra Pratap
In-Reply-To: <ce83bd09-dbd2-4c9e-8197-6e4800935523@web.de>
On Mon, Feb 05, 2024 at 08:57:46PM +0100, René Scharfe wrote:
> If you want to make the code work with buffers that lack a terminating
> NUL then you need to replace the strchrnul() call with something that
> respects buffer lengths. You could e.g. call memchr(). Don't forget
> to check for NUL to preserve the original behavior. Or you could roll
> your own custom replacement, perhaps like this:
I'm not sure it is worth retaining the check for NUL. The original
function added by me in fe6eb7f2c5 (commit: provide a function to find a
header in a buffer, 2014-08-27) just took a NUL-terminated string, so
we certainly were not expecting embedded NULs.
In cfc5cf428b (receive-pack.c: consolidate find header logic,
2022-01-06) we switched to taking the "len" parameter, but the new
caller just passes strlen(msg) anyway.
I guess you could argue that before that commit, receive-pack.c's
find_header() which took a length was buggy to use strchrnul(). It gets
fed with a push-cert buffer. I guess it's possible for there to be an
embedded NUL there, but in practice there shouldn't be. If we are
thinking of malformed or malicious input, it's not clear which behavior
(finding or not finding a header past a NUL) is more harmful. So all
things being equal, I would try to reduce the number of special cases
here by not worrying about NULs.
(Though if somebody really wants to dig, it's possible there's a clever
dual-parser attack here where "\nfoo\0bar baz" finds the header "bar
baz" in one parser but not in another).
-Peff
^ permalink raw reply
* Re: [PATCH v2 3/5] t4301: verify that merge-tree fails on missing blob objects
From: Eric Sunshine @ 2024-02-08 0:52 UTC (permalink / raw)
To: Johannes Schindelin via GitGitGadget
Cc: git, Patrick Steinhardt, Johannes Schindelin
In-Reply-To: <CAPig+cSJz3U+vT==NhX5hcrTjsCggnAzhzQOvZcSXbcEGuYaKQ@mail.gmail.com>
On Wed, Feb 7, 2024 at 12:24 PM Eric Sunshine <sunshine@sunshineco.com> wrote:
> Interpolating the $seqN variable directly into the string rather than
> using %s would make it even clearer that only a single line is being
> generated as input to git-mktree:
>
> tree1=$(printf "100644 blob $seq1\tsequence\n" | git mktree) &&
> tree2=$(printf "100644 blob $seq2\tsequence\n" | git mktree) &&
> tree3=$(printf "100644 blob $seq3\tsequence\n" | git mktree) &&
>
> Alternatively `echo` could be used, though it's not necessarily any nicer:
>
> tree1=$(echo "100644 blob $seq1Qsequence" | q_to_tab | git mktree) &&
> tree2=$(echo "100644 blob $seq2Qsequence" | q_to_tab | git mktree) &&
> tree3=$(echo "100644 blob $seq3Qsequence" | q_to_tab | git mktree) &&
The `printf` example is probably cleaner, thus preferable. For
completeness, though, I should mention that the `echo` example is, of
course, not quite correct. For the interpolation to work as intended,
it would need ${...}:
tree1=$(echo "100644 blob ${seq1}Qsequence" | q_to_tab | git mktree) &&
tree2=$(echo "100644 blob ${seq2}Qsequence" | q_to_tab | git mktree) &&
tree3=$(echo "100644 blob ${seq3}Qsequence" | q_to_tab | git mktree) &&
^ permalink raw reply
* Re: [PATCH] tag: fix sign_buffer() call to create a signed tag
From: Jeff King @ 2024-02-08 0:47 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Sergey Kosukhin
In-Reply-To: <xmqq4jek9ko1.fsf@gitster.g>
On Wed, Feb 07, 2024 at 10:46:54AM -0800, Junio C Hamano wrote:
> The command "git tag -s" internally calls sign_buffer() to make a
> cryptographic signature using the chosen backend like GPG and SSH.
> The internal helper functions used by "git tag" implementation seem
> to use a "negative return values are errors, zero or positive return
> values are not" convention, and there are places (e.g., verify_tag()
> that calls gpg_verify_tag()) that these internal helper functions
> translate return values that signal errors to conform to this
> convention, but do_sign() that calls sign_buffer() forgets to do so.
>
> Fix it, so that a failed call to sign_buffer() that can return the
> exit status from pipe_command() will not be overlooked.
>
> Reported-by: Sergey Kosukhin <skosukhin@gmail.com>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
>
> * We alternatively could fix individual sign_buffer() backend that
> signals an error with a positive value (sign_buffer_ssh() in this
> case) to return a negative value, but this would hopefully be
> more future-proof.
FWIW, I would have gone the other way, and fixed sign_buffer_ssh(). Your
solution here is future-proofing the tag code against other
sign_buffer_*() functions behaving like ssh. But it is also leaving
other sign_buffer() callers to introduce the same bug.
Your documentation change at least makes that less likely. But given how
much of our code uses the "negative is error" convention, I wouldn't be
surprised to see it happen anyway.
-Peff
^ permalink raw reply
* Re: [PATCH v2 00/30] initial support for multiple hash functions
From: Linus Arver @ 2024-02-08 0:24 UTC (permalink / raw)
To: Junio C Hamano, git
Cc: Eric W. Biederman, brian m. carlson, Eric Sunshine,
Eric W. Biederman
In-Reply-To: <xmqqv86z5359.fsf@gitster.g>
Junio C Hamano <gitster@pobox.com> writes:
> "Eric W. Biederman" <ebiederm@gmail.com> writes:
>
>> This addresses all of the known test failures from v1 of this set of
>> changes. In particular I have reworked commit_tree_extended which
>> was flagged by smatch, -Werror=array-bounds, and the leak detector.
>>
>> One functional bug was fixed in repo_for_each_abbrev where it was
>> mistakenly displaying too many ambiguous oids.
>>
>> I am posting this so that people review and testing of this patchset
>> won't be distracted by the known and fixed issues.
>
> We haven't seen any reviews on this second round, and have had it
> outside 'next' for too long. I am tempted to say that we merge it
> to 'next' and see if anybody screams at this point.
FWIW out of all the "Needs review" topics this one seemed like the most
deserving of another pair of eyes, and I was planning to review some of
the patches here this week + the weekend. If my review takes too long
(taking longer than this weekend) I can give another update next week
saying "too hard for me, please don't wait for me" to unblock you from
merging to next.
Thanks.
^ permalink raw reply
* Re: [RFC PATCH v2 1/6] t0080: turn t-basic unit test into a helper
From: Junio C Hamano @ 2024-02-08 0:09 UTC (permalink / raw)
To: Jeff King; +Cc: Josh Steadmon, git, johannes.schindelin, phillip.wood
In-Reply-To: <20240207225802.GA538110@coredump.intra.peff.net>
Jeff King <peff@peff.net> writes:
> So I think we probably want to keep it (or possibly move it onto the
> UNIT_TEST_SOURCES line, which keeps it close to the wildcard call).
Sensible.
^ permalink raw reply
* Re: [RFC PATCH v2 6/6] t/Makefile: run unit tests alongside shell tests
From: Junio C Hamano @ 2024-02-07 23:26 UTC (permalink / raw)
To: Jeff King; +Cc: Josh Steadmon, git, johannes.schindelin, phillip.wood
In-Reply-To: <20240207224350.GA537799@coredump.intra.peff.net>
Jeff King <peff@peff.net> writes:
> Not if they share the same command-line options. If you use something
> like "--state=slow,save", then the first run will write the list of all
> tests to ".prove", and then the second will run every test mentioned in
> .prove (in addition to the unit-tests provided on the command-line).
>
> You should be able to work around it by passing "--statefile". I _think_
> it might be OK to just do that unconditionally. Something like:
>
> prove --exec $(TEST_SHELL_PATH $(GIT_PROVE_OPTS) $(T) :: $(GIT_TEST_OPTS)
> prove --statefile=.prove-unit-tests $(GIT_PROVE_OPTS) $(UNIT_TESTS) :: $(GIT_TEST_OPTS)
>
> and then it's just a noop if GIT_PROVE_OPTS doesn't use --state. But I
> haven't played with it myself.
I do not think it warrants such complexity. The wrapper script is
fine.
^ permalink raw reply
* Re: [RFC PATCH v2 1/6] t0080: turn t-basic unit test into a helper
From: Jeff King @ 2024-02-07 22:58 UTC (permalink / raw)
To: Josh Steadmon; +Cc: git, johannes.schindelin, phillip.wood, gitster
In-Reply-To: <da756b4bfb9d1ce0d1213d585e72acfbf667e2a2.1706921262.git.steadmon@google.com>
On Fri, Feb 02, 2024 at 04:50:26PM -0800, Josh Steadmon wrote:
> This has the additional benefit that test harnesses seeking to run all
> unit tests can find them with a simple glob of "t/unit-tests/bin/t-*",
> with no exceptions needed. This will be important in a later patch where
> we add support for running the unit tests via a test-tool subcommand.
Is this last paragraph still accurate? I think in this rebased version
of the series, we'll continue to use $(UNIT_TESTS) derived from the
source list rather than a glob in bin/.
> --- a/t/Makefile
> +++ b/t/Makefile
> @@ -44,8 +44,7 @@ TINTEROP = $(sort $(wildcard interop/i[0-9][0-9][0-9][0-9]-*.sh))
> CHAINLINTTESTS = $(sort $(patsubst chainlint/%.test,%,$(wildcard chainlint/*.test)))
> CHAINLINT = '$(PERL_PATH_SQ)' chainlint.pl
> UNIT_TEST_SOURCES = $(wildcard unit-tests/t-*.c)
> -UNIT_TEST_PROGRAMS = $(patsubst unit-tests/%.c,unit-tests/bin/%$(X),$(UNIT_TEST_SOURCES))
> -UNIT_TESTS = $(sort $(filter-out unit-tests/bin/t-basic%,$(UNIT_TEST_PROGRAMS)))
> +UNIT_TESTS = $(patsubst unit-tests/%.c,unit-tests/bin/%$(X),$(UNIT_TEST_SOURCES))
This drops the intermediate UNIT_TEST_PROGRAMS, which makes sense. It
was only used to keep the long lines a bit more readable. But it also
drops the $(sort) call. Do we need to keep it?
Certainly I'd think we want the contents of $(UNIT_TESTS) to be in a
deterministic order. Does the $(wildcard) function already return things
in sorted order? I can't find any mention in the documention. It seems
to do so for me in a simple test, but aae5239be2 (t/Makefile: Use $(sort
...) explicitly where needed, 2011-09-04) argues otherwise.
So I think we probably want to keep it (or possibly move it onto the
UNIT_TEST_SOURCES line, which keeps it close to the wildcard call).
-Peff
^ permalink raw reply
* Re: [RFC PATCH v2 6/6] t/Makefile: run unit tests alongside shell tests
From: Jeff King @ 2024-02-07 22:43 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Josh Steadmon, git, johannes.schindelin, phillip.wood
In-Reply-To: <xmqqzfwc6lle.fsf@gitster.g>
On Wed, Feb 07, 2024 at 12:55:09PM -0800, Junio C Hamano wrote:
> > +# A simple wrapper to run shell tests via TEST_SHELL_PATH,
> > +# or exec unit tests directly.
> > +
> > +case "$1" in
> > +*.sh)
> > + exec ${TEST_SHELL_PATH:-/bin/sh} "$@"
> > + ;;
> > +*)
> > + exec "$@"
> > + ;;
> > +esac
>
> Hmph. This penalizes the non-unit tests by doing an extra "exec",
> once per program?
It does, but IMHO that is not likely to be a problem. It's once per
top-level script (so ~1000), and each of those scripts spawns hundreds
or thousands of sub-commands. I didn't do any measurements, though.
You can extend "prove" with extra perl modules so that it makes the
distinction internally without the extra shell invocation. But when I
tried to do it, I found it rather baroque and complicated (I can't
remember if I succeeded but found it too gross, or just gave up halfway
through trying).
> Of course we cannot run two $(PROVE) invocations serially, one for
> doing $(T) and the other for doing $(UNIT_TESTS)?
Not if they share the same command-line options. If you use something
like "--state=slow,save", then the first run will write the list of all
tests to ".prove", and then the second will run every test mentioned in
.prove (in addition to the unit-tests provided on the command-line).
You should be able to work around it by passing "--statefile". I _think_
it might be OK to just do that unconditionally. Something like:
prove --exec $(TEST_SHELL_PATH $(GIT_PROVE_OPTS) $(T) :: $(GIT_TEST_OPTS)
prove --statefile=.prove-unit-tests $(GIT_PROVE_OPTS) $(UNIT_TESTS) :: $(GIT_TEST_OPTS)
and then it's just a noop if GIT_PROVE_OPTS doesn't use --state. But I
haven't played with it myself.
-Peff
^ permalink raw reply
* Re: [PATCH v4 1/2] refs: introduce reftable backend
From: Jeff King @ 2024-02-07 22:31 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git, Junio C Hamano, Han-Wen Nienhuys, Karthik Nayak
In-Reply-To: <5de60d46bdccbfbf0a923abc2f45eda07f30c110.1707288261.git.ps@pks.im>
On Wed, Feb 07, 2024 at 08:20:31AM +0100, Patrick Steinhardt wrote:
> +static int write_copy_table(struct reftable_writer *writer, void *cb_data)
> +{
> [...]
> + /*
> + * Create the reflog entry for the newly created branch.
> + */
> + ALLOC_GROW(logs, logs_nr + 1, logs_alloc);
> + memset(&logs[logs_nr], 0, sizeof(logs[logs_nr]));
> + fill_reftable_log_record(&logs[logs_nr]);
> + logs[logs_nr].refname = (char *)arg->newname;
> + logs[logs_nr].update_index = creation_ts;
> + logs[logs_nr].value.update.message =
> + xstrndup(arg->logmsg, arg->refs->write_options.block_size / 2);
> + logs[logs_nr].value.update.new_hash = old_ref.value.val1;
> + logs_nr++;
> +
> + /*
> + * In addition to writing the reflog entry for the new branch, we also
> + * copy over all log entries from the old reflog. Last but not least,
> + * when renaming we also have to delete all the old reflog entries.
> + */
> + ret = reftable_merged_table_seek_log(mt, &it, arg->oldname);
> + if (ret < 0)
> + return ret;
Should this last line be "goto done" as is used elsewhere in the
function? Otherwise we are at least leaking the "logs" array (and
possibly some of the other cleanup is important, too).
> + while (1) {
> + ret = reftable_iterator_next_log(&it, &old_log);
> + if (ret < 0)
> + goto done;
> + if (ret > 0 || strcmp(old_log.refname, arg->oldname)) {
> + ret = 0;
> + break;
> + }
This "ret = 0" doesn't have any effect. We break out of the loop, and
then...
> + }
> +
> + ret = reftable_writer_add_logs(writer, logs, logs_nr);
> + if (ret < 0)
> + goto done;
...the first thing we do is write over it. I dunno if it's worth keeping
as a maintenance precaution, though (if the code after the loop changed
to omit that assignment, then setting "ret" would become important).
Both were noticed by Coverity (along with several other false
positives).
-Peff
^ permalink raw reply
* Re: [PATCH v2 00/30] initial support for multiple hash functions
From: Junio C Hamano @ 2024-02-07 22:18 UTC (permalink / raw)
To: git; +Cc: Eric W. Biederman, brian m. carlson, Eric Sunshine,
Eric W. Biederman
In-Reply-To: <878r8l929e.fsf@gmail.froward.int.ebiederm.org>
"Eric W. Biederman" <ebiederm@gmail.com> writes:
> This addresses all of the known test failures from v1 of this set of
> changes. In particular I have reworked commit_tree_extended which
> was flagged by smatch, -Werror=array-bounds, and the leak detector.
>
> One functional bug was fixed in repo_for_each_abbrev where it was
> mistakenly displaying too many ambiguous oids.
>
> I am posting this so that people review and testing of this patchset
> won't be distracted by the known and fixed issues.
We haven't seen any reviews on this second round, and have had it
outside 'next' for too long. I am tempted to say that we merge it
to 'next' and see if anybody screams at this point.
Thanks.
^ 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