* Re: [PATCH 1/1] git-gui: add possibility to open currently selected file
From: Junio C Hamano @ 2019-12-27 22:32 UTC (permalink / raw)
To: Pratyush Yadav; +Cc: Zoli Szabó via GitGitGadget, git, Zoli Szabó
In-Reply-To: <20191227193418.36uzeizs37nv7ywb@yadavpratyush.com>
Pratyush Yadav <me@yadavpratyush.com> writes:
> Have you tested it on Windows? Does 'explorer.exe' do the correct thing?
>
> Looking at MacOS's 'open' man page, I think it should also work like
> xdg-open and shouldn't be a problem.
> ...
> Tested on Linux. Works fine. Looking forward to the re-roll.
> Subject: Re: [PATCH 1/1] git-gui: add possibility to open currently selected file
The phrasing on the title is a bit awkward. "add possibility to do
X" is better spelled "allow doing X", no?
^ permalink raw reply
* Re: [ANNOUNCE] Git v2.25.0-rc0
From: Junio C Hamano @ 2019-12-27 22:28 UTC (permalink / raw)
To: Danh Doan; +Cc: git
In-Reply-To: <20191227113858.GB24268@danh.dev>
Danh Doan <congdanhqx@gmail.com> writes:
> On 2019-12-26 09:50:49-0800, Junio C Hamano <gitster@pobox.com> wrote:
>> Danh Doan <congdanhqx@gmail.com> writes:
>>
>> > My name should be moved down to next paragraph,
>> > since I was lazy to type my name with all accents.
>>
>> Your first contribution was back in v2.20 days, and then the recent
>> ones are all within this cycle for v2.25.
>>
>> I am a bit curious why you need to avoid being lazy just to give the
>> correct name to your commits, though. Isn't
>
> Some of my projects requires ASCII-only user.name,
> instead of doing the right thing
>
> git config user.name <simplified-name>
>
> I decided to set it globally instead.
> I rarely need to type in my native language,
> hence I don't have the IME software start with Xorg.
Hmph, but back in v2.20 days, you did have IME?
In any case, if I were in such a situation to need my name spelled
differently depending on the project I work on, I would probably use
$ git config --global user.name <simplified-name>
$ cd <repository of git>
$ git config user.name <name-with-accents>
or the other way around (depends on which projects your focus is on).
But perhaps that is not so useful to your situation?
^ permalink raw reply
* Re: [PATCH v2 1/1] gpg-interface: add minTrustLevel as a configuration option
From: Junio C Hamano @ 2019-12-27 22:21 UTC (permalink / raw)
To: Hans Jerry Illikainen; +Cc: git
In-Reply-To: <87r20pkhir.hji@dyntopia.com>
Hans Jerry Illikainen <hji@dyntopia.com> writes:
>> I wonder if the code becomes less misleading if we either (1)
>> renamed 'next' to a name that hints more strongly that it is not the
>> 'next' line but the end of the current token we are interested in,
>> or (2) get rid of the pointer and instead counted size of the
>> current token we are interested in, or perhaps both?
>
> Yeah the name 'next' does seem a bit counter-intuitive when used in
> relation to 'line'. Looking through the function it seems that both (1)
> and (2) would work.
Thanks for thinking the code a bit more than necessary for the
purpose of this topic. Let's leave such a clean-up outside the
scope of this topic, but perhaps a #leftoverbits marker may help us
remember it as something we could do when we have nothing else
better to do ;-)
^ permalink raw reply
* Re: [PATCH 0/1] sparse-checkout: list directories in cone mode
From: Elijah Newren @ 2019-12-27 21:47 UTC (permalink / raw)
To: Derrick Stolee via GitGitGadget
Cc: Git Mailing List, SZEDER Gábor, jon, Derrick Stolee,
Junio C Hamano
In-Reply-To: <pull.500.git.1577393347.gitgitgadget@gmail.com>
On Thu, Dec 26, 2019 at 12:49 PM Derrick Stolee via GitGitGadget
<gitgitgadget@gmail.com> wrote:
>
> When in cone mode, "git sparse-checkout set" takes a list of folders and
> constructs an ordered list of patterns for the sparse-checkout file. The
> "git sparse-checkout list" subcommand outputs the contents of the
> sparse-checkout file in a very basic way.
>
> This patch changes the behavior of "git sparse-checkout list" when
> core.sparseCheckoutCone=true. It will output the folders that were used in
> "git sparse-checkout set" to create the patterns, instead of the patterns
> themselves.
>
> I believe this was requested in the initial review, but I cannot find that
> message now.
Yeah, I think I mentioned it, but couldn't remember for sure. Just
did a little digging and found
https://lore.kernel.org/git/CABPp-BH13XbNR3MQKE7cHO5e=pMY7kLtGhkX1SQg_o9it=uUug@mail.gmail.com/:
"Should the list mode in cone mode be modified to just show the
directories the user added? It seems a little weird to show the
internal details of the implementation (all the parent directories and
negated entries and whatnot). That's also not in a form that users
can pass along to future `sparse-checkout add` invocations."
Though I then went into other tangents, which may have been what
caused it to be forgotten or overlooked.
> I was going to include this as part of a longer follow-up series, but I
> think this may be worth considering for the 2.25.0 release. Hence, it is
> included by itself.
Yeah, I think this is a good and small fixup to a new feature in
2.25.0, so it'd be nice if it could be included. Not sure if it's too
late given that we're at -rc0, but here's a thumbs up from me if it
makes any difference.
^ permalink raw reply
* Re: [PATCH v2 2/2] sparse-checkout: document interactions with submodules
From: Elijah Newren @ 2019-12-27 21:46 UTC (permalink / raw)
To: Derrick Stolee via GitGitGadget
Cc: Git Mailing List, SZEDER Gábor, jon, Derrick Stolee,
Junio C Hamano
In-Reply-To: <331bb7d6fbec6f2f429feb36cf32e0931307ae0b.1577472469.git.gitgitgadget@gmail.com>
On Fri, Dec 27, 2019 at 10:47 AM Derrick Stolee via GitGitGadget
<gitgitgadget@gmail.com> wrote:
>
> From: Derrick Stolee <dstolee@microsoft.com>
>
> Junio asked what the behavior is between the sparse-checkout feature
> and the submodule feature.
Does this first sentence help future readers? It is what spurred you
to write the documentation, but it seems like something that could
just be left out.
> The sparse-checkout builtin has not changed
> the way these features interact, but we may as well document it in
> the builtin docs.
>
> Using 'git submodule (init|deinit)' a user can select a subset of
> submodules to populate. This behaves very similar to the sparse-checkout
> feature, but those directories contain their own .git directory
> including an object database and ref space. To have the sparse-checkout
> file also determine if those files should exist would easily cause
> problems. Therefore, keeping these features independent in this way
> is the best way forward.
>
> Also create a test that demonstrates this behavior to make sure
> it doesn't change as the sparse-checkout feature evolves.
>
> Reported-by: Junio C Hamano <gitster@pobox.com>
> Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
> ---
> Documentation/git-sparse-checkout.txt | 10 ++++++++++
> t/t1091-sparse-checkout-builtin.sh | 28 +++++++++++++++++++++++++++
> 2 files changed, 38 insertions(+)
>
> diff --git a/Documentation/git-sparse-checkout.txt b/Documentation/git-sparse-checkout.txt
> index dcca9ee826..2b7aaa0310 100644
> --- a/Documentation/git-sparse-checkout.txt
> +++ b/Documentation/git-sparse-checkout.txt
> @@ -164,6 +164,16 @@ case-insensitive check. This corrects for case mismatched filenames in the
> 'git sparse-checkout set' command to reflect the expected cone in the working
> directory.
>
> +
> +SUBMODULES
> +----------
> +
> +If your repository contains one or more submodules, then those submodules will
> +appear based on which you initialized with the `git submodule` command. If
> +your sparse-checkout patterns exclude an initialized submodule, then that
> +submodule will still appear in your working directory.
> +
> +
> SEE ALSO
> --------
>
> diff --git a/t/t1091-sparse-checkout-builtin.sh b/t/t1091-sparse-checkout-builtin.sh
> index 37f6d8fa90..5572beeeca 100755
> --- a/t/t1091-sparse-checkout-builtin.sh
> +++ b/t/t1091-sparse-checkout-builtin.sh
> @@ -340,4 +340,32 @@ test_expect_success 'cone mode: set with core.ignoreCase=true' '
> test_cmp expect dir
> '
>
> +test_expect_success 'interaction with submodules' '
> + git clone repo super &&
> + (
> + cd super &&
> + mkdir modules &&
> + git submodule add ../repo modules/child &&
> + git add . &&
> + git commit -m "add submodule" &&
> + git sparse-checkout init --cone &&
> + git sparse-checkout set folder1
> + ) &&
> + list_files super >dir &&
> + cat >expect <<-EOF &&
> + a
> + folder1
> + modules
> + EOF
> + test_cmp expect dir &&
> + list_files super/modules/child >dir &&
> + cat >expect <<-EOF &&
> + a
> + deep
> + folder1
> + folder2
> + EOF
> + test_cmp expect dir
> +'
> +
> test_done
> --
I read over the rest, and not being a submodule user I'm not sure what
I'd expect. But it certainly seems reasonable to document how these
features interact and that you haven't made any modifications in the
area.
^ permalink raw reply
* Re: [PATCH 05/16] t2018: don't lose return code of git commands
From: Eric Sunshine @ 2019-12-27 21:42 UTC (permalink / raw)
To: Denton Liu; +Cc: Git Mailing List
In-Reply-To: <4fe247c09df89ebe908f366ee7c2a4ec1c209d86.1577454401.git.liu.denton@gmail.com>
On Fri, Dec 27, 2019 at 8:48 AM Denton Liu <liu.denton@gmail.com> wrote:
> We had some git commands wrapped in non-assignment command substitutions
> which would result in their return codes to be lost. Rewrite these
> instances so that their return codes are now checked.
Try writing your commit messages in imperative mood:
Fix invocations of Git commands so their exit codes are not lost
within command substitutions...
or something. Same comment about several other commit messages in this series.
More below...
> Signed-off-by: Denton Liu <liu.denton@gmail.com>
> ---
> diff --git a/t/t2018-checkout-branch.sh b/t/t2018-checkout-branch.sh
> @@ -23,7 +23,8 @@ do_checkout () {
> # if <sha> is not specified, use HEAD.
> - exp_sha=${2:-$(git rev-parse --verify HEAD)} &&
> + head=$(git rev-parse --verify HEAD) &&
> + exp_sha=${2:-$head} &&
Are you sure this transformation is needed? In my tests, the exit code
of the Git command is not lost.
^ permalink raw reply
* Re: [PATCH v2 1/2] sparse-checkout: list folders in cone mode
From: Elijah Newren @ 2019-12-27 21:37 UTC (permalink / raw)
To: Derrick Stolee via GitGitGadget
Cc: Git Mailing List, SZEDER Gábor, jon, Derrick Stolee,
Junio C Hamano
In-Reply-To: <d6f4f404866e30e9de89991bb39f2908facb30ae.1577472469.git.gitgitgadget@gmail.com>
On Fri, Dec 27, 2019 at 10:47 AM Derrick Stolee via GitGitGadget
<gitgitgadget@gmail.com> wrote:
>
> From: Derrick Stolee <dstolee@microsoft.com>
>
> When core.sparseCheckoutCone is enabled, the 'git sparse-checkout set'
> command takes a list of folders as input, then creates an ordered
s/folders/directories/
> list of sparse-checkout patterns such that those folders are
s/folders/directories/
> recursively included and all sibling entries along the parent folders
again here.
> are also included. Listing the patterns is less user-friendly than the
> folders themselves.
and here.
>
> In cone mode, and as long as the patterns match the expected cone-mode
> pattern types, change the output of 'git sparse-checkout list' to only
> show the folders that created the patterns.
and here.
>
> With this change, the following piped commands would not change the
> working directory:
>
> git sparse-checkout list | git sparse-checkout set --stdin
>
> The only time this would not work is if core.sparseCheckoutCone is
> true, but the sparse-checkout file contains patterns that do not
> match the expected pattern types for cone mode.
>
> Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
> ---
> Documentation/git-sparse-checkout.txt | 11 ++++++++++-
> builtin/sparse-checkout.c | 21 +++++++++++++++++++++
> t/t1091-sparse-checkout-builtin.sh | 11 +++++++++++
> 3 files changed, 42 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/git-sparse-checkout.txt b/Documentation/git-sparse-checkout.txt
> index 9c3c66cc37..dcca9ee826 100644
> --- a/Documentation/git-sparse-checkout.txt
> +++ b/Documentation/git-sparse-checkout.txt
> @@ -28,7 +28,7 @@ THE FUTURE.
> COMMANDS
> --------
> 'list'::
> - Provide a list of the contents in the sparse-checkout file.
> + Describe the patterns in the sparse-checkout file.
>
> 'init'::
> Enable the `core.sparseCheckout` setting. If the
> @@ -150,6 +150,15 @@ expecting patterns of these types. Git will warn if the patterns do not match.
> If the patterns do match the expected format, then Git will use faster hash-
> based algorithms to compute inclusion in the sparse-checkout.
>
> +In the cone mode case, the `git sparse-checkout list` subcommand will list the
> +folders that define the recursive patterns. For the example sparse-checkout file
and here.
> +above, the output is as follows:
> +
> +--------------------------
> +$ git sparse-checkout list
> +A/B/C
> +--------------------------
> +
> If `core.ignoreCase=true`, then the pattern-matching algorithm will use a
> case-insensitive check. This corrects for case mismatched filenames in the
> 'git sparse-checkout set' command to reflect the expected cone in the working
> diff --git a/builtin/sparse-checkout.c b/builtin/sparse-checkout.c
> index 5d62f7a66d..b3bed891cb 100644
> --- a/builtin/sparse-checkout.c
> +++ b/builtin/sparse-checkout.c
> @@ -53,6 +53,8 @@ static int sparse_checkout_list(int argc, const char **argv)
>
> memset(&pl, 0, sizeof(pl));
>
> + pl.use_cone_patterns = core_sparse_checkout_cone;
> +
> sparse_filename = get_sparse_checkout_filename();
> res = add_patterns_from_file_to_list(sparse_filename, "", 0, &pl, NULL);
> free(sparse_filename);
> @@ -62,6 +64,25 @@ static int sparse_checkout_list(int argc, const char **argv)
> return 0;
> }
>
> + if (pl.use_cone_patterns) {
> + int i;
> + struct pattern_entry *pe;
> + struct hashmap_iter iter;
> + struct string_list sl = STRING_LIST_INIT_DUP;
> +
> + hashmap_for_each_entry(&pl.recursive_hashmap, &iter, pe, ent) {
> + /* pe->pattern starts with "/", skip it */
> + string_list_insert(&sl, pe->pattern + 1);
> + }
> +
> + string_list_sort(&sl);
> +
> + for (i = 0; i < sl.nr; i++)
> + printf("%s\n", sl.items[i].string);
> +
> + return 0;
> + }
> +
> write_patterns_to_file(stdout, &pl);
> clear_pattern_list(&pl);
>
> diff --git a/t/t1091-sparse-checkout-builtin.sh b/t/t1091-sparse-checkout-builtin.sh
> index 6f7e2d0c9e..37f6d8fa90 100755
> --- a/t/t1091-sparse-checkout-builtin.sh
> +++ b/t/t1091-sparse-checkout-builtin.sh
> @@ -246,6 +246,17 @@ test_expect_success 'cone mode: init and set' '
> test_cmp expect dir
> '
>
> +test_expect_success 'cone mode: list' '
> + cat >expect <<-EOF &&
> + folder1
> + folder2
> + EOF
> + git -C repo sparse-checkout set --stdin <expect &&
> + git -C repo sparse-checkout list >actual 2>err &&
> + test_must_be_empty err &&
> + test_cmp expect actual
> +'
> +
> test_expect_success 'cone mode: set with nested folders' '
There's another one here but that's just the context area, so it'd be
a separate patch in a separate cleanup series...
> git -C repo sparse-checkout set deep deep/deeper1/deepest 2>err &&
> test_line_count = 0 err &&
> --
The rest looks good
^ permalink raw reply
* Re: [PATCH 10/20] t5319: make test work with SHA-256
From: brian m. carlson @ 2019-12-27 21:35 UTC (permalink / raw)
To: Derrick Stolee; +Cc: git, Derrick Stolee, Junio C Hamano
In-Reply-To: <54d80d36-f9b9-1c92-faf0-41bbcbe0a133@gmail.com>
[-- Attachment #1: Type: text/plain, Size: 787 bytes --]
On 2019-12-26 at 14:50:09, Derrick Stolee wrote:
> On 12/21/2019 2:49 PM, brian m. carlson wrote:
> > @@ -387,7 +401,7 @@ test_expect_success 'force some 64-bit offsets with pack-objects' '
> > pack64=$(git pack-objects --index-version=2,0x40 objects64/pack/test-64 <obj-list) &&
> > idx64=objects64/pack/test-64-$pack64.idx &&
> > chmod u+w $idx64 &&
> > - corrupt_data $idx64 2999 "\02" &&
> > + corrupt_data $idx64 $(test_oid idxoff) "\02" &&
>
> Sorry that this was not a better-calculated value, but
> your approach works well here.
That's okay. I appreciate that you attempted to make things robust, and
the documentary value of the variables was helpful in fixing these
tests.
--
brian m. carlson: Houston, Texas, US
OpenPGP: https://keybase.io/bk2204
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 868 bytes --]
^ permalink raw reply
* Re: Comparing rebase --am with --interactive via p3400
From: Alban Gruin @ 2019-12-27 21:11 UTC (permalink / raw)
To: Johannes Schindelin, Elijah Newren; +Cc: git
In-Reply-To: <nycvar.QRO.7.76.6.1901312310280.41@tvgsbejvaqbjf.bet>
[-- Attachment #1: Type: text/plain, Size: 5067 bytes --]
Hi Johannes & Elijah,
Le 01/02/2019 à 07:04, Johannes Schindelin a écrit :
> Hi Elijah,
>
> as discussed at the Contributors' Summit, I ran p3400 as-is (i.e. with the
> --am backend) and then with --keep-empty to force the interactive backend
> to be used. Here are the best of 10, on my relatively powerful Windows 10
> laptop, with current `master`.
>
> With regular rebase --am:
>
> 3400.2: rebase on top of a lot of unrelated changes 5.32(0.06+0.15)
> 3400.4: rebase a lot of unrelated changes without split-index 33.08(0.04+0.18)
> 3400.6: rebase a lot of unrelated changes with split-index 30.29(0.03+0.18)
>
> with --keep-empty to force the interactive backend:
>
> 3400.2: rebase on top of a lot of unrelated changes 3.92(0.03+0.18)
> 3400.4: rebase a lot of unrelated changes without split-index 33.92(0.03+0.22)
> 3400.6: rebase a lot of unrelated changes with split-index 38.82(0.03+0.16)
>
> I then changed it to -m to test the current scripted version, trying to
> let it run overnight, but my laptop eventually went to sleep and the tests
> were not even done. I'll let them continue and report back.
>
> My conclusion after seeing these numbers is: the interactive rebase is
> really close to the performance of the --am backend. So to me, it makes a
> total lot of sense to switch --merge over to it, and to make --merge the
> default. We still should investigate why the split-index performance is so
> significantly worse, though.
>
> Ciao,
> Dscho
>
I investigated a bit on this. From a quick glance at a callgrind trace,
I can see that ce_write_entry() is called 20 601[1] times with `git am',
but 739 802 times with the sequencer when the split-index is enabled.
For reference, here are the timings, measured on my Linux machine, on a
tmpfs, with git.git as the repo:
`rebase --am':
> 3400.2: rebase on top of a lot of unrelated changes 0.29(0.24+0.03)
> 3400.4: rebase a lot of unrelated changes without split-index 6.77(6.51+0.22)
> 3400.6: rebase a lot of unrelated changes with split-index 4.43(4.29+0.13)
`rebase --quiet':
> 3400.2: rebase on top of a lot of unrelated changes 0.24(0.21+0.02)
> 3400.4: rebase a lot of unrelated changes without split-index 5.60(5.32+0.27)
> 3400.6: rebase a lot of unrelated changes with split-index 5.67(5.40+0.26)
This comes from two things:
1. There is not enough shared entries in the index with the sequencer.
do_write_index() is called only by do_write_locked_index() with `--am',
but is also called by write_shared_index() with the sequencer once for
every other commit. As the latter is only called by
write_locked_index(), which means that too_many_not_shared_entries()
returns true for the sequencer, but never for `--am'.
Removing the call to discard_index() in do_pick_commit() (as in the
first attached patch) solve this particular issue, but this would
require a more thorough analysis to see if it is actually safe to do.
After this, ce_write() is still called much more by the sequencer.
Here are the results of `rebase --quiet' without discarding the index:
> 3400.2: rebase on top of a lot of unrelated changes 0.23(0.19+0.04)
> 3400.4: rebase a lot of unrelated changes without split-index 5.14(4.95+0.18)
> 3400.6: rebase a lot of unrelated changes with split-index 5.02(4.87+0.15)
The performance of the rebase is better in the two cases.
2. The base index is dropped by unpack_trees_start() and unpack_trees().
Now, write_shared_index() is no longer called and write_locked_index()
is less expensive than before according to callgrind. But
ce_write_entry() is still called 749 302 times (which is even more than
before.)
The only place where ce_write_entry() is called is in a loop in
do_write_index(). The number of iterations is dictated by the size of
the cache, and there is a trace2 probe dumping this value.
For `--am', the value goes like this: 1, 2, 2, 2, 2, 3, 3, 3, 3, 4, 4,
4, 4, 5, 5, 5, 5, … up until 101.
For the sequencer, it goes like this: 1, 1, 3697, 3697, 3698, 3698,
3699, 3699, … up until 3796.
The size of the cache is set in prepare_to_write_split_index(). It
grows if a cache entry has no index (most of them should have one by
now), or if the split index has no base index (with `--am', the split
index always has a base.) This comes from unpack_trees_start() -- it
creates a new index, and unpack_trees() does not carry the base index,
hence the size of the cache.
The second attached patch (which is broken for the non-interactive
rebase case) demonstrates what we could expect for the split-index case
if we fix this:
> 3400.2: rebase on top of a lot of unrelated changes 0.24(0.21+0.03)
> 3400.4: rebase a lot of unrelated changes without split-index 5.81(5.62+0.17)
> 3400.6: rebase a lot of unrelated changes with split-index 4.76(4.54+0.20)
So, for everything related to the index, I think that’s it.
[1] Numbers may vary, but they should remain in the same order of magnitude.
Cheers,
Alban
[-- Attachment #2: sequencer-rebase-si.patch --]
[-- Type: text/x-patch, Size: 317 bytes --]
diff --git a/sequencer.c b/sequencer.c
index 1bee26ebd5..2831abd0fa 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -1863,7 +1863,6 @@ static int do_pick_commit(struct repository *r,
NULL, 0))
return error_dirty_index(r, opts);
}
- discard_index(r->index);
if (!commit->parents)
parent = NULL;
[-- Attachment #3: merge-recursive-rebase-si.patch --]
[-- Type: text/x-patch, Size: 1367 bytes --]
diff --git a/merge-recursive.c b/merge-recursive.c
index 11869ad81c..47f67079f3 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -421,7 +421,7 @@ static int unpack_trees_start(struct merge_options *opt,
{
int rc;
struct tree_desc t[3];
- struct index_state tmp_index = { NULL };
+ /* struct index_state tmp_index = { NULL }; */
memset(&opt->priv->unpack_opts, 0, sizeof(opt->priv->unpack_opts));
if (opt->priv->call_depth)
@@ -432,7 +432,7 @@ static int unpack_trees_start(struct merge_options *opt,
opt->priv->unpack_opts.head_idx = 2;
opt->priv->unpack_opts.fn = threeway_merge;
opt->priv->unpack_opts.src_index = opt->repo->index;
- opt->priv->unpack_opts.dst_index = &tmp_index;
+ opt->priv->unpack_opts.dst_index = opt->repo->index;
opt->priv->unpack_opts.aggressive = !merge_detect_rename(opt);
setup_unpack_trees_porcelain(&opt->priv->unpack_opts, "merge");
@@ -449,8 +449,8 @@ static int unpack_trees_start(struct merge_options *opt,
* saved copy. (verify_uptodate() checks src_index, and the original
* index is the one that had the necessary modification timestamps.)
*/
- opt->priv->orig_index = *opt->repo->index;
- *opt->repo->index = tmp_index;
+ /* opt->priv->orig_index = *opt->repo->index; */
+ /* *opt->repo->index = tmp_index; */
opt->priv->unpack_opts.src_index = &opt->priv->orig_index;
return rc;
^ permalink raw reply related
* Re: [PATCH 02/16] t2018: add space between function name and ()
From: Eric Sunshine @ 2019-12-27 21:03 UTC (permalink / raw)
To: Denton Liu; +Cc: Git Mailing List
In-Reply-To: <9558d2491fa8dc68603426c1528154b4fe352650.1577454401.git.liu.denton@gmail.com>
On Fri, Dec 27, 2019 at 8:47 AM Denton Liu <liu.denton@gmail.com> wrote:
> t2018: add space between function name and ()
Patch 1/16, which removed an entirely unnecessary space from the end
of the description text, made sense without any additional
explanation. However, this patch's change could be puzzling to readers
who are not thoroughly familiar with the finer details of test coding
guidelines. At a minimum, it would be nice to add a sentence to the
commit message stating that this change brings the style in line with
the coding guidelines.
> Signed-off-by: Denton Liu <liu.denton@gmail.com>
^ permalink raw reply
* Re: [PATCH 12/16] t3504: don't use `test_must_fail test_cmp`
From: Johannes Sixt @ 2019-12-27 20:39 UTC (permalink / raw)
To: Denton Liu; +Cc: Git Mailing List
In-Reply-To: <b3e4f1698fd86f75f650c7928f2107ea0edd3600.1577454401.git.liu.denton@gmail.com>
Am 27.12.19 um 14:47 schrieb Denton Liu:
> The test_must_fail function should only be used for git commands since
> we should assume that external commands work sanely. Since test_cmp() just
> wraps an external command, replace `test_must_fail test_cmp` with
> `! test_cmp`.
>
> Signed-off-by: Denton Liu <liu.denton@gmail.com>
> ---
> t/t3504-cherry-pick-rerere.sh | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/t/t3504-cherry-pick-rerere.sh b/t/t3504-cherry-pick-rerere.sh
> index a267b2d144..c31141c471 100755
> --- a/t/t3504-cherry-pick-rerere.sh
> +++ b/t/t3504-cherry-pick-rerere.sh
> @@ -95,7 +95,7 @@ test_expect_success 'cherry-pick --rerere-autoupdate more than once' '
> test_expect_success 'cherry-pick conflict without rerere' '
> test_config rerere.enabled false &&
> test_must_fail git cherry-pick master &&
> - test_must_fail test_cmp expect foo
> + ! test_cmp expect foo
This is VERY suspicious. It is not reliable to check that something
is not exactly equal to something else.
Feel free to replace this patch by the following.
----- 8< -----
t3504: do check for conflict marker after failed cherry-pick
The test with disabled rerere should make sure that the cherry-picked
result does not have the conflict replaced with a recorded resolution.
It attempts to do so by ensuring that the file content is _not_ equal
to some other file. That by itself is a very dubious check because just
about every random result of an incomplete cherry-pick would satisfy
the condition.
In this case, the intent was to check that the conflicting file does
_not_ contain the resolved content. But the check actually uses the
wrong reference file, but not the resolved content. Needless to say
that the non-equality is satisfied. And, on top of it, it uses a commit
that does not even touch the file that is checked.
Do check for the expected result, which is content from both sides of
the merge and merge conflicts. (The latter we check for just the
middle separator for brevity.)
As a side-effect, this also removes an incorrect use of test_must_fail.
Signed-off-by: Johannes Sixt <j6t@kdbg.org>
---
t/t3504-cherry-pick-rerere.sh | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/t/t3504-cherry-pick-rerere.sh b/t/t3504-cherry-pick-rerere.sh
index a267b2d144..80a0d08706 100755
--- a/t/t3504-cherry-pick-rerere.sh
+++ b/t/t3504-cherry-pick-rerere.sh
@@ -94,8 +94,10 @@ test_expect_success 'cherry-pick --rerere-autoupdate more than once' '
test_expect_success 'cherry-pick conflict without rerere' '
test_config rerere.enabled false &&
- test_must_fail git cherry-pick master &&
- test_must_fail test_cmp expect foo
+ test_must_fail git cherry-pick foo-master &&
+ grep ===== foo &&
+ grep foo-dev foo &&
+ grep foo-master foo
'
test_done
--
2.24.1.524.gae569673ff
^ permalink raw reply related
* Re: [PATCH v2 2/2] sparse-checkout: document interactions with submodules
From: Eric Sunshine @ 2019-12-27 20:20 UTC (permalink / raw)
To: Derrick Stolee via GitGitGadget
Cc: Git List, SZEDER Gábor, Elijah Newren, Jon Simons,
Derrick Stolee, Junio C Hamano
In-Reply-To: <331bb7d6fbec6f2f429feb36cf32e0931307ae0b.1577472469.git.gitgitgadget@gmail.com>
On Fri, Dec 27, 2019 at 1:48 PM Derrick Stolee via GitGitGadget
<gitgitgadget@gmail.com> wrote:
> diff --git a/t/t1091-sparse-checkout-builtin.sh b/t/t1091-sparse-checkout-builtin.sh
> @@ -340,4 +340,32 @@ test_expect_success 'cone mode: set with core.ignoreCase=true' '
> +test_expect_success 'interaction with submodules' '
> + ...
> + cat >expect <<-EOF &&
> + a
> + folder1
> + modules
> + EOF
You would normally use \-EOF rather than -EOF to make it clear that no
interpolation is needed/expected within the here-doc body. However,
this script is already full of -EOF when \-EOF ought to be used, so
being consistent with existing tests may override an objection.
Likewise, please note for future reference that the usual way
here-docs are formatted in Git test scripts is to indent the body of
the here-doc to the same level as the command which opens it. That is:
cat >expect <<\-EOF &&
a
folder1
modules
EOF
But, again, this script is already full of these malformatted
here-docs, so maintaining consistency with the existing test in the
script is probably okay.
(Of course, a preparatory patch fixing these here-doc issues would be
welcome, but might also be outside the scope of this submission. And,
fixing these very minor issues is quite low-priority, so I wouldn't
expect or demand it.)
^ permalink raw reply
* Re: [PATCH 1/1] git-gui: add possibility to open currently selected file
From: Pratyush Yadav @ 2019-12-27 19:34 UTC (permalink / raw)
To: Zoli Szabó via GitGitGadget; +Cc: git, Zoli Szabó
In-Reply-To: <fce80f1b95f83915076640ca0be01aa473744777.1577386915.git.gitgitgadget@gmail.com>
Hi Zoli,
Thanks for the patch.
On 26/12/19 07:01PM, Zoli Szabó via GitGitGadget wrote:
> From: =?UTF-8?q?Zoli=20Szab=C3=B3?= <zoli.szabo@gmail.com>
>
> ...in the default associated app (e.g. in a text editor / IDE).
>
> Many times there's the need to quickly open a source file (the one you're
> looking at in Git GUI) in the predefined text editor / IDE. Of course,
> the file can be searched for in your preferred file manager or directly
> in the text editor, but having the option to directly open the current
> file from Git GUI would be just faster. This change enables just that by:
> - Diff header path context menu -> Open;
Would it be a better idea to have this option in the diff body context
menu (.vpane.lower.diff.body.ctxm) instead? The problem I see with the
way its currently done is visibility/discovery. It is not very likely
for a user to try and click the file name which doesn't give any
indication that it is clickable. So how will someone who hasn't read
this commit message know that they can use this neat feature. The diff
body context menu is much more "visible" IMO.
> - or double-clicking the diff header path.
An alternative to the above suggestion would be to make this path
underlined and blue in color (like a hyperlink in a web browser). This
will give the indication that this is not just plain text.
I like the latter idea more, but I don't mind either.
> One "downside" of the approach is that executable files will be run
> and not opened for editing.
FWIW, I do not see it as a downside at all. The menu option is called
"open" not "edit". So if you click it, you should expect the file to
open. In case its a binary file, executing it is the correct outcome. In
case its a text file, opening it in the editor is the correct outcome.
> Signed-off-by: Zoli Szabó <zoli.szabo@gmail.com>
> ---
> git-gui.sh | 26 ++++++++++++++++++++++++--
> 1 file changed, 24 insertions(+), 2 deletions(-)
>
> diff --git a/git-gui.sh b/git-gui.sh
> index c1be733e3e..705b97f7ab 100755
> --- a/git-gui.sh
> +++ b/git-gui.sh
> @@ -2248,8 +2248,8 @@ proc do_git_gui {} {
> }
> }
>
> -proc do_explore {} {
> - global _gitworktree
> +# Get the system-specific explorer app/command.
> +proc get_explorer {} {
> set explorer {}
Not exactly related to this patch, but this line is redundant. No point
in clearing 'explorer' only to set it in the very next if-else chain.
> if {[is_Cygwin] || [is_Windows]} {
> set explorer "explorer.exe"
> @@ -2259,9 +2259,25 @@ proc do_explore {} {
> # freedesktop.org-conforming system is our best shot
> set explorer "xdg-open"
> }
> + return $explorer
> +}
> +
> +proc do_explore {} {
> + global _gitworktree
> + set explorer [get_explorer]
> eval exec $explorer [list [file nativename $_gitworktree]] &
> }
>
> +# Trigger opening a file (relative to the working tree) by the default
> +# associated app of the OS (e.g. a text editor or IDE).
Nitpick: This comment doesn't really add a whole lot of information. The
procedure name already make it pretty clear that it will open a file.
And it is pretty easy to understand from reading the body of the
procedure that it will be relative to the working tree. So, I think it
can be removed altogether.
But even if you think it should stay, please at least make it more
concise. Maybe something like:
Open file relative to the working tree by the default associated app.
> +# FIXME: What about executables (will be run, not opened for editing)?
Again, I think running the executables is the correct behaviour. So I
don't think this needs any fixing.
> +proc do_file_open {file} {
> + global _gitworktree
> + set explorer [get_explorer]
> + set full_file_path [file join $_gitworktree $file]
> + eval exec $explorer [list [file nativename $full_file_path]] &
This executes $explorer, which is 'explorer.exe' on Windows. I'm not a
heavy Windows user but AFAIK it is a file manager. This makes it quite
different from 'xdg-open' which is used to open _any_ file/URL in the
user's default application. So it also happens to open _directories_ in
the default file explorer which was the original intention of this
procedure.
Have you tested it on Windows? Does 'explorer.exe' do the correct thing?
Looking at MacOS's 'open' man page, I think it should also work like
xdg-open and shouldn't be a problem.
> +}
> +
> set is_quitting 0
> set ret_code 1
>
> @@ -3530,8 +3546,14 @@ $ctxm add command \
> -type STRING \
> -- $current_diff_path
> }
> +$ctxm add command \
> + -label [mc Open] \
> + -command {
> + do_file_open $current_diff_path
> + }
Nitpick: no need to make the command multi-line. So, change it to
something like:
-command {do_file_open $current_diff_path}
> lappend diff_actions [list $ctxm entryconf [$ctxm index last] -state]
> bind_button3 .vpane.lower.diff.header.path "tk_popup $ctxm %X %Y"
> +bind .vpane.lower.diff.header.path <Double-1> {do_file_open $current_diff_path}
>
> # -- Diff Body
> #
Tested on Linux. Works fine. Looking forward to the re-roll.
--
Regards,
Pratyush Yadav
^ permalink raw reply
* [PATCH v2 2/2] sparse-checkout: document interactions with submodules
From: Derrick Stolee via GitGitGadget @ 2019-12-27 18:47 UTC (permalink / raw)
To: git; +Cc: szeder.dev, newren, jon, Derrick Stolee, Junio C Hamano,
Derrick Stolee
In-Reply-To: <pull.500.v2.git.1577472469.gitgitgadget@gmail.com>
From: Derrick Stolee <dstolee@microsoft.com>
Junio asked what the behavior is between the sparse-checkout feature
and the submodule feature. The sparse-checkout builtin has not changed
the way these features interact, but we may as well document it in
the builtin docs.
Using 'git submodule (init|deinit)' a user can select a subset of
submodules to populate. This behaves very similar to the sparse-checkout
feature, but those directories contain their own .git directory
including an object database and ref space. To have the sparse-checkout
file also determine if those files should exist would easily cause
problems. Therefore, keeping these features independent in this way
is the best way forward.
Also create a test that demonstrates this behavior to make sure
it doesn't change as the sparse-checkout feature evolves.
Reported-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
---
Documentation/git-sparse-checkout.txt | 10 ++++++++++
t/t1091-sparse-checkout-builtin.sh | 28 +++++++++++++++++++++++++++
2 files changed, 38 insertions(+)
diff --git a/Documentation/git-sparse-checkout.txt b/Documentation/git-sparse-checkout.txt
index dcca9ee826..2b7aaa0310 100644
--- a/Documentation/git-sparse-checkout.txt
+++ b/Documentation/git-sparse-checkout.txt
@@ -164,6 +164,16 @@ case-insensitive check. This corrects for case mismatched filenames in the
'git sparse-checkout set' command to reflect the expected cone in the working
directory.
+
+SUBMODULES
+----------
+
+If your repository contains one or more submodules, then those submodules will
+appear based on which you initialized with the `git submodule` command. If
+your sparse-checkout patterns exclude an initialized submodule, then that
+submodule will still appear in your working directory.
+
+
SEE ALSO
--------
diff --git a/t/t1091-sparse-checkout-builtin.sh b/t/t1091-sparse-checkout-builtin.sh
index 37f6d8fa90..5572beeeca 100755
--- a/t/t1091-sparse-checkout-builtin.sh
+++ b/t/t1091-sparse-checkout-builtin.sh
@@ -340,4 +340,32 @@ test_expect_success 'cone mode: set with core.ignoreCase=true' '
test_cmp expect dir
'
+test_expect_success 'interaction with submodules' '
+ git clone repo super &&
+ (
+ cd super &&
+ mkdir modules &&
+ git submodule add ../repo modules/child &&
+ git add . &&
+ git commit -m "add submodule" &&
+ git sparse-checkout init --cone &&
+ git sparse-checkout set folder1
+ ) &&
+ list_files super >dir &&
+ cat >expect <<-EOF &&
+ a
+ folder1
+ modules
+ EOF
+ test_cmp expect dir &&
+ list_files super/modules/child >dir &&
+ cat >expect <<-EOF &&
+ a
+ deep
+ folder1
+ folder2
+ EOF
+ test_cmp expect dir
+'
+
test_done
--
gitgitgadget
^ permalink raw reply related
* [PATCH v2 1/2] sparse-checkout: list folders in cone mode
From: Derrick Stolee via GitGitGadget @ 2019-12-27 18:47 UTC (permalink / raw)
To: git; +Cc: szeder.dev, newren, jon, Derrick Stolee, Junio C Hamano,
Derrick Stolee
In-Reply-To: <pull.500.v2.git.1577472469.gitgitgadget@gmail.com>
From: Derrick Stolee <dstolee@microsoft.com>
When core.sparseCheckoutCone is enabled, the 'git sparse-checkout set'
command takes a list of folders as input, then creates an ordered
list of sparse-checkout patterns such that those folders are
recursively included and all sibling entries along the parent folders
are also included. Listing the patterns is less user-friendly than the
folders themselves.
In cone mode, and as long as the patterns match the expected cone-mode
pattern types, change the output of 'git sparse-checkout list' to only
show the folders that created the patterns.
With this change, the following piped commands would not change the
working directory:
git sparse-checkout list | git sparse-checkout set --stdin
The only time this would not work is if core.sparseCheckoutCone is
true, but the sparse-checkout file contains patterns that do not
match the expected pattern types for cone mode.
Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
---
Documentation/git-sparse-checkout.txt | 11 ++++++++++-
builtin/sparse-checkout.c | 21 +++++++++++++++++++++
t/t1091-sparse-checkout-builtin.sh | 11 +++++++++++
3 files changed, 42 insertions(+), 1 deletion(-)
diff --git a/Documentation/git-sparse-checkout.txt b/Documentation/git-sparse-checkout.txt
index 9c3c66cc37..dcca9ee826 100644
--- a/Documentation/git-sparse-checkout.txt
+++ b/Documentation/git-sparse-checkout.txt
@@ -28,7 +28,7 @@ THE FUTURE.
COMMANDS
--------
'list'::
- Provide a list of the contents in the sparse-checkout file.
+ Describe the patterns in the sparse-checkout file.
'init'::
Enable the `core.sparseCheckout` setting. If the
@@ -150,6 +150,15 @@ expecting patterns of these types. Git will warn if the patterns do not match.
If the patterns do match the expected format, then Git will use faster hash-
based algorithms to compute inclusion in the sparse-checkout.
+In the cone mode case, the `git sparse-checkout list` subcommand will list the
+folders that define the recursive patterns. For the example sparse-checkout file
+above, the output is as follows:
+
+--------------------------
+$ git sparse-checkout list
+A/B/C
+--------------------------
+
If `core.ignoreCase=true`, then the pattern-matching algorithm will use a
case-insensitive check. This corrects for case mismatched filenames in the
'git sparse-checkout set' command to reflect the expected cone in the working
diff --git a/builtin/sparse-checkout.c b/builtin/sparse-checkout.c
index 5d62f7a66d..b3bed891cb 100644
--- a/builtin/sparse-checkout.c
+++ b/builtin/sparse-checkout.c
@@ -53,6 +53,8 @@ static int sparse_checkout_list(int argc, const char **argv)
memset(&pl, 0, sizeof(pl));
+ pl.use_cone_patterns = core_sparse_checkout_cone;
+
sparse_filename = get_sparse_checkout_filename();
res = add_patterns_from_file_to_list(sparse_filename, "", 0, &pl, NULL);
free(sparse_filename);
@@ -62,6 +64,25 @@ static int sparse_checkout_list(int argc, const char **argv)
return 0;
}
+ if (pl.use_cone_patterns) {
+ int i;
+ struct pattern_entry *pe;
+ struct hashmap_iter iter;
+ struct string_list sl = STRING_LIST_INIT_DUP;
+
+ hashmap_for_each_entry(&pl.recursive_hashmap, &iter, pe, ent) {
+ /* pe->pattern starts with "/", skip it */
+ string_list_insert(&sl, pe->pattern + 1);
+ }
+
+ string_list_sort(&sl);
+
+ for (i = 0; i < sl.nr; i++)
+ printf("%s\n", sl.items[i].string);
+
+ return 0;
+ }
+
write_patterns_to_file(stdout, &pl);
clear_pattern_list(&pl);
diff --git a/t/t1091-sparse-checkout-builtin.sh b/t/t1091-sparse-checkout-builtin.sh
index 6f7e2d0c9e..37f6d8fa90 100755
--- a/t/t1091-sparse-checkout-builtin.sh
+++ b/t/t1091-sparse-checkout-builtin.sh
@@ -246,6 +246,17 @@ test_expect_success 'cone mode: init and set' '
test_cmp expect dir
'
+test_expect_success 'cone mode: list' '
+ cat >expect <<-EOF &&
+ folder1
+ folder2
+ EOF
+ git -C repo sparse-checkout set --stdin <expect &&
+ git -C repo sparse-checkout list >actual 2>err &&
+ test_must_be_empty err &&
+ test_cmp expect actual
+'
+
test_expect_success 'cone mode: set with nested folders' '
git -C repo sparse-checkout set deep deep/deeper1/deepest 2>err &&
test_line_count = 0 err &&
--
gitgitgadget
^ permalink raw reply related
* [PATCH v2 0/2] sparse-checkout: list directories in cone mode
From: Derrick Stolee via GitGitGadget @ 2019-12-27 18:47 UTC (permalink / raw)
To: git; +Cc: szeder.dev, newren, jon, Derrick Stolee, Junio C Hamano
In-Reply-To: <pull.500.git.1577393347.gitgitgadget@gmail.com>
When in cone mode, "git sparse-checkout set" takes a list of folders and
constructs an ordered list of patterns for the sparse-checkout file. The
"git sparse-checkout list" subcommand outputs the contents of the
sparse-checkout file in a very basic way.
This patch changes the behavior of "git sparse-checkout list" when
core.sparseCheckoutCone=true. It will output the folders that were used in
"git sparse-checkout set" to create the patterns, instead of the patterns
themselves.
I believe this was requested in the initial review, but I cannot find that
message now.
I was going to include this as part of a longer follow-up series, but I
think this may be worth considering for the 2.25.0 release. Hence, it is
included by itself.
Update in V2:
* Fixed typos/word choice in commit message.
* Added a second commit including clarification on interactions with
submodules.
Thanks, -Stolee
Derrick Stolee (2):
sparse-checkout: list folders in cone mode
sparse-checkout: document interactions with submodules
Documentation/git-sparse-checkout.txt | 21 ++++++++++++++-
builtin/sparse-checkout.c | 21 +++++++++++++++
t/t1091-sparse-checkout-builtin.sh | 39 +++++++++++++++++++++++++++
3 files changed, 80 insertions(+), 1 deletion(-)
base-commit: 761e3d26bbe44c51f83c4f1ad198461f57029ebd
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-500%2Fderrickstolee%2Fsparse-checkout-list-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-500/derrickstolee/sparse-checkout-list-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/500
Range-diff vs v1:
1: 07be7b8dda ! 1: d6f4f40486 sparse-checkout: list folders in cone mode
@@ -3,9 +3,9 @@
sparse-checkout: list folders in cone mode
When core.sparseCheckoutCone is enabled, the 'git sparse-checkout set'
- command taks a list of folders as input, then creates an ordered
+ command takes a list of folders as input, then creates an ordered
list of sparse-checkout patterns such that those folders are
- recursively included and all sibling blobs along the parent folders
+ recursively included and all sibling entries along the parent folders
are also included. Listing the patterns is less user-friendly than the
folders themselves.
-: ---------- > 2: 331bb7d6fb sparse-checkout: document interactions with submodules
--
gitgitgadget
^ permalink raw reply
* Git Test Coverage Report (Dec. 27)
From: Derrick Stolee @ 2019-12-27 16:18 UTC (permalink / raw)
To: Git List
Here is today's test coverage report [1][2][3]. I also got the
single-branch build working with the commit view, so I have the
commit view available for the v2.25.0-rc0 report [4].
Thanks,
-Stolee
[1] https://derrickstolee.github.io/git-test-coverage/reports/2019-12-27-commits.txt
[2] https://derrickstolee.github.io/git-test-coverage/reports/2019-12-27.txt
[3] https://derrickstolee.github.io/git-test-coverage/reports/2019-12-27.htm
[4] https://derrickstolee.github.io/git-test-coverage/reports/v2.25.0-rc0-commits.txt
---
pu 4da9d1ed5116f89b015c733206b4b7e61883f8bb
jch 0001df5574e7f7a096d568fb64aeb26e5fe2757a
next ca9f1ec99ff3ed584eec58c085a342c9c37d2919
master 0a76bd7381ec0dbb7c43776eb6d1ac906bca29e6
master@{1} b02fd2accad4d48078671adf38fe5b5976d77304
Uncovered code in 'pu' not in 'jch'
--------------------------------------------------------
Commits introducing uncovered code:
Denton Liu daf6b9e4 sequencer: use file strbuf for read_oneliner()
sequencer.c
daf6b9e4 439) goto done;
daf6b9e4 445) goto done;
Denton Liu f2096173 reset: extract reset_head() from rebase
reset.c
f2096173 37) ret = -1;
f2096173 38) goto leave_reset_head;
f2096173 43) goto leave_reset_head;
f2096173 65) goto leave_reset_head;
f2096173 71) goto leave_reset_head;
f2096173 76) goto leave_reset_head;
f2096173 80) ret = -1;
f2096173 81) goto leave_reset_head;
f2096173 89) goto leave_reset_head;
f2096173 108) } else if (old_orig)
f2096173 109) delete_ref(NULL, "ORIG_HEAD", old_orig, 0);
Denton Liu afc27b82 rebase: use read_oneliner()
builtin/rebase.c
afc27b82 648) } else if (!read_oneliner(&buf, state_dir_path("head", opts), 0, 1))
Denton Liu d4e8a655 rebase: generify reset_head()
builtin/rebase.c
d4e8a655 904) reset_head(the_repository, &opts->orig_head, "checkout",
d4e8a655 905) opts->head_name, 0,
Denton Liu 82c638ef rebase: use apply_autostash() from sequencer.c
builtin/rebase.c
82c638ef 1065) apply_autostash(state_dir_path("autostash", opts));
Johannes Schindelin 53fa2153 built-in add -p: handle Escape sequences in interactive.singlekey mode
compat/terminal.c
53fa2153 330) if (ch == '\033' /* ESC */) {
53fa2153 338) strbuf_splice(buf, buf->len - 1, 1, "^[", 2);
53fa2153 346) struct pollfd pfd = { .fd = 0, .events = POLLIN };
53fa2153 348) if (poll(&pfd, 1, 500) < 1)
53fa2153 349) break;
53fa2153 351) ch = getchar();
53fa2153 352) if (ch == EOF)
53fa2153 353) return 0;
53fa2153 354) strbuf_addch(buf, ch);
Johannes Schindelin 61131685 built-in add -p: handle Escape sequences more efficiently
compat/terminal.c
61131685 255) static int sequence_entry_cmp(const void *hashmap_cmp_fn_data,
61131685 260) return strcmp(e1->sequence, keydata ? keydata : e2->sequence);
61131685 263) static int is_known_escape_sequence(const char *sequence)
61131685 268) if (!initialized) {
61131685 269) struct child_process cp = CHILD_PROCESS_INIT;
61131685 270) struct strbuf buf = STRBUF_INIT;
61131685 273) hashmap_init(&sequences, (hashmap_cmp_fn)sequence_entry_cmp,
61131685 276) argv_array_pushl(&cp.args, "infocmp", "-L", "-1", NULL);
61131685 277) if (pipe_command(&cp, NULL, 0, &buf, 0, NULL, 0))
61131685 278) strbuf_setlen(&buf, 0);
61131685 280) for (eol = p = buf.buf; *p; p = eol + 1) {
61131685 281) p = strchr(p, '=');
61131685 282) if (!p)
61131685 283) break;
61131685 284) p++;
61131685 285) eol = strchrnul(p, '\n');
61131685 287) if (starts_with(p, "\\E")) {
61131685 288) char *comma = memchr(p, ',', eol - p);
61131685 291) p[0] = '^';
61131685 292) p[1] = '[';
61131685 293) FLEX_ALLOC_MEM(e, sequence, p, comma - p);
61131685 294) hashmap_entry_init(&e->entry,
61131685 295) strhash(e->sequence));
61131685 296) hashmap_add(&sequences, &e->entry);
61131685 298) if (!*eol)
61131685 299) break;
61131685 301) initialized = 1;
61131685 304) return !!hashmap_get_from_hash(&sequences, strhash(sequence), sequence);
61131685 345) while (!is_known_escape_sequence(buf->buf)) {
Johannes Schindelin 74e42899 terminal: add a new function to read a single keystroke
compat/terminal.c
74e42899 64) static int enable_non_canonical(void)
74e42899 66) return disable_bits(ICANON | ECHO);
74e42899 307) int read_key_without_echo(struct strbuf *buf)
74e42899 312) if (warning_displayed || enable_non_canonical() < 0) {
74e42899 313) if (!warning_displayed) {
74e42899 316) warning_displayed = 1;
74e42899 319) return strbuf_getline(buf, stdin);
74e42899 322) strbuf_reset(buf);
74e42899 323) ch = getchar();
74e42899 324) if (ch == EOF) {
74e42899 325) restore_term();
74e42899 326) return EOF;
74e42899 328) strbuf_addch(buf, ch);
74e42899 358) restore_term();
74e42899 359) return 0;
Johannes Schindelin aaafd603 terminal: make the code of disable_echo() reusable
compat/terminal.c
aaafd603 38) static int disable_bits(tcflag_t bits)
aaafd603 49) t.c_lflag &= ~bits;
aaafd603 59) static int disable_echo(void)
aaafd603 61) return disable_bits(ECHO);
Johannes Schindelin 6610e462 built-in stash: use the built-in `git add -p` if so configured
builtin/stash.c
6610e462 1026) setenv(INDEX_ENVIRONMENT, old_index_env, 1);
Johannes Schindelin 90a6bb98 legacy stash -p: respect the add.interactive.usebuiltin setting
builtin/add.c
90a6bb98 450) parse_pathspec(&pathspec, 0,
90a6bb98 456) return run_add_interactive(NULL, "--patch=stash", &pathspec);
Johannes Schindelin d2a233cb built-in add -p: prepare for patch modes other than "stage"
add-patch.c
d2a233cb 1567) _(s->mode->help_patch_text));
Johannes Schindelin 52628f94 built-in add -p: implement the "checkout" patch modes
add-patch.c
52628f94 1272) fwrite(diff->buf, diff->len, 1, stderr);
Johannes Schindelin 60b7e674 built-in add -p: respect the `interactive.singlekey` config setting
add-patch.c
60b7e674 1156) int res = read_key_without_echo(&s->answer);
60b7e674 1157) printf("%s\n", res == EOF ? "" : s->answer.buf);
60b7e674 1158) return res;
Jonathan Nieder ee70c128 index: offer advice for unknown index extensions
read-cache.c
ee70c128 1761) if (advice_unknown_index_extension) {
Josh Steadmon 6da1f1a9 protocol: advertise multiple supported versions
remote-curl.c
6da1f1a9 354) return 0;
Matheus Tavares beebb9d2 object-store: allow threaded access to object reading
packfile.c
beebb9d2 1453) return;
sha1-file.c
beebb9d2 1431) return;
beebb9d2 1440) return;
Phillip Wood 430b75f7 commit: give correct advice for empty commit during a rebase
sequencer.c
430b75f7 1580) return -1;
Uncovered code in 'jch' not in 'next'
--------------------------------------------------------
Commits introducing uncovered code:
Alban Gruin 0d50cf5e sequencer: move check_todo_list_from_file() to rebase-interactive.c
rebase-interactive.c
0d50cf5e 210) goto out;
0d50cf5e 215) goto out;
0d50cf5e 224) fprintf(stderr, _(edit_todo_list_advice));
Alex Torok fed842f0 rebase: fix --fork-point with short refname
builtin/merge-base.c
fed842f0 128) return 1;
Elijah Newren 4d861fad rebase, sequencer: remove the broken GIT_QUIET handling
builtin/rebase.c
4d861fad 725) write_file(state_dir_path("quiet", opts), "%s", "");
Elijah Newren 7ee11336 rebase: extend the options for handling of empty commits
sequencer.c
7ee11336 2055) res = allow;
7ee11336 2056) goto leave;
7ee11336 2515) opts->drop_redundant_commits =
7ee11336 2516) git_config_bool_or_int(key, value, &error_flag);
7ee11336 2521) opts->ask_on_initially_empty =
7ee11336 2522) git_config_bool_or_int(key, value, &error_flag);
7ee11336 3066) res |= git_config_set_in_file_gently(opts_file,
7ee11336 3072) res |= git_config_set_in_file_gently(opts_file,
7ee11336 4798) continue;
Hans Jerry Illikainen 917ddb16 gpg-interface: add minTrustLevel as a configuration option
builtin/pull.c
917ddb16 374) return status;
gpg-interface.c
917ddb16 143) return 1;
917ddb16 209) free(trust);
917ddb16 210) goto error;
917ddb16 399) return config_error_nonbool(var);
pretty.c
917ddb16 1355) strbuf_addstr(sb, "never");
917ddb16 1356) break;
917ddb16 1358) strbuf_addstr(sb, "marginal");
917ddb16 1359) break;
917ddb16 1361) strbuf_addstr(sb, "fully");
917ddb16 1362) break;
Martin Ågren 3bf986d6 builtin/config: collect "value_regexp" data in a struct
builtin/config.c
3bf986d6 329) FREE_AND_NULL(cmd_line_value.regexp);
Martin Ågren 3bcdd852 builtin/config: extract `handle_value_regex()` from `get_value()`
builtin/config.c
3bcdd852 330) return CONFIG_INVALID_PATTERN;
3bcdd852 375) goto free_strings;
Uncovered code in 'next' not in 'master'
--------------------------------------------------------
Commits introducing uncovered code:
Elijah Newren 4fe7e43c rebase: fix saving of --signoff state for am-based rebases
builtin/rebase.c
4fe7e43c 709) write_file(state_dir_path("signoff", opts), "--signoff");
Jeff King 4f0bd8b9 pack-objects: improve partial packfile reuse
builtin/pack-objects.c
4f0bd8b9 1124) return 1;
4f0bd8b9 2681) (reuse_packfile_bitmap &&
4f0bd8b9 2682) bitmap_walk_contains(bitmap_git, reuse_packfile_bitmap, oid));
pack-bitmap.c
4f0bd8b9 808) return;
4f0bd8b9 811) return;
4f0bd8b9 823) return;
4f0bd8b9 861) i = bitmap_git->pack->num_objects / BITS_IN_EWORD;
Jeff King 7cb9754e pack-objects: introduce pack.allowPackReuse
builtin/pack-objects.c
7cb9754e 2834) allow_pack_reuse = git_config_bool(k, v);
7cb9754e 2835) return 0;
Jeff King 7b143c16 pack-bitmap: introduce bitmap_walk_contains()
pack-bitmap.c
7b143c16 903) return 0;
Uncovered code in 'master' not in 'master@{1}'
--------------------------------------------------------
Commits introducing uncovered code:
Derrick Stolee 96cc8ab5 sparse-checkout: use hashmaps for cone patterns
dir.c
96cc8ab5 663) pl->use_cone_patterns = 0;
96cc8ab5 665) goto clear_hashmaps;
96cc8ab5 689) hashmap_add(&pl->parent_hashmap, &translated->ent);
96cc8ab5 690) hashmap_remove(&pl->recursive_hashmap, &translated->ent, &data);
96cc8ab5 691) free(data);
96cc8ab5 692) return;
96cc8ab5 698) goto clear_hashmaps;
96cc8ab5 716) hashmap_remove(&pl->parent_hashmap, &translated->ent, &data);
96cc8ab5 717) free(data);
96cc8ab5 718) free(translated);
96cc8ab5 1280) return MATCHED;
Derrick Stolee af09ce24 sparse-checkout: init and set in cone mode
builtin/sparse-checkout.c
af09ce24 352) return;
Derrick Stolee 190a65f9 sparse-checkout: respect core.ignoreCase in cone mode
builtin/sparse-checkout.c
190a65f9 337) strihash(e->pattern) :
dir.c
190a65f9 676) strihash(translated->pattern) :
190a65f9 707) strihash(translated->pattern) :
Derrick Stolee bab3c359 sparse-checkout: create 'init' subcommand
builtin/sparse-checkout.c
bab3c359 221) return 1;
bab3c359 275) return 1;
Derrick Stolee e091228e sparse-checkout: update working directory in-process
builtin/sparse-checkout.c
e091228e 85) return 0;
Derrick Stolee d89f09c8 clone: add --sparse mode
builtin/clone.c
d89f09c8 753) result = 1;
d89f09c8 1132) return 1;
Johannes Schindelin 5906d5de built-in app -p: allow selecting a mode change as a "hunk"
add-patch.c
5906d5de 146) return 0;
5906d5de 150) return 0;
5906d5de 277) file_diff->hunk->colored_start =
5906d5de 278) colored_p - colored->buf;
5906d5de 425) const char *p = s->colored.buf;
5906d5de 427) strbuf_add(out, p + head->colored_start,
5906d5de 428) first->colored_start - head->colored_start);
5906d5de 429) strbuf_add(out, p + first->colored_end,
5906d5de 430) head->colored_end - first->colored_end);
Johannes Schindelin 25ea47af built-in add -p: adjust hunk headers as needed
add-patch.c
25ea47af 87) return -1;
25ea47af 104) eol = s->plain.buf + s->plain.len;
25ea47af 127) eol = s->colored.buf + s->colored.len;
25ea47af 250) return -1;
25ea47af 393) strbuf_addf(out, "%s\n", GIT_COLOR_RESET);
25ea47af 1296) repo_refresh_and_write_index(s->s.r, REFRESH_QUIET, 0,
Johannes Schindelin 7584dd3c built-in add -p: offer a helpful error message when hunk navigation failed
add-patch.c
7584dd3c 52) static void err(struct add_p_state *s, const char *fmt, ...)
7584dd3c 56) va_start(args, fmt);
7584dd3c 57) fputs(s->s.error_color, stderr);
7584dd3c 58) vfprintf(stderr, fmt, args);
7584dd3c 59) fputs(s->s.reset_color, stderr);
7584dd3c 60) fputc('\n', stderr);
7584dd3c 61) va_end(args);
7584dd3c 62) }
7584dd3c 1134) if (hunk_index)
7584dd3c 1135) hunk_index--;
7584dd3c 1137) err(s, _("No previous hunk"));
7584dd3c 1140) hunk_index++;
7584dd3c 1142) err(s, _("No next hunk"));
7584dd3c 1144) if (undecided_previous >= 0)
7584dd3c 1145) hunk_index = undecided_previous;
7584dd3c 1147) err(s, _("No previous hunk"));
7584dd3c 1149) if (undecided_next >= 0)
7584dd3c 1150) hunk_index = undecided_next;
7584dd3c 1152) err(s, _("No next hunk"));
Johannes Schindelin 54d9d9b2 built-in add -p: only show the applicable parts of the help text
add-patch.c
54d9d9b2 1254) const char *p = _(help_patch_remainder), *eol = p;
54d9d9b2 1256) color_fprintf(stdout, s->s.help_color, "%s",
54d9d9b2 1263) for (; *p; p = eol + (*eol == '\n')) {
54d9d9b2 1264) eol = strchrnul(p, '\n');
54d9d9b2 1271) if (*p != '?' && !strchr(s->buf.buf, *p))
54d9d9b2 1272) continue;
54d9d9b2 1274) color_fprintf_ln(stdout, s->s.help_color,
54d9d9b2 1275) "%.*s", (int)(eol - p), p);
Johannes Schindelin d6cf8733 built-in add -p: implement the '/' ("search regex") command
add-patch.c
d6cf8733 1195) err(s, _("No other hunks to search"));
d6cf8733 1196) continue;
d6cf8733 1201) printf("%s", _("search for regex? "));
d6cf8733 1202) fflush(stdout);
d6cf8733 1203) if (strbuf_getline(&s->answer,
d6cf8733 1205) break;
d6cf8733 1206) strbuf_trim_trailing_newline(&s->answer);
d6cf8733 1207) if (s->answer.len == 0)
d6cf8733 1208) continue;
d6cf8733 1216) err(s, _("Malformed search regexp %s: %s"),
d6cf8733 1218) continue;
d6cf8733 1233) err(s, _("No hunk matches the given pattern"));
d6cf8733 1234) break;
Johannes Schindelin 80399aec built-in add -p: support multi-file diffs
add-patch.c
80399aec 1118) for (; hunk_index < file_diff->hunk_nr; hunk_index++) {
80399aec 1119) hunk = file_diff->hunk + hunk_index;
80399aec 1139) if (hunk_index + 1 < file_diff->hunk_nr)
Johannes Schindelin 9254bdfb built-in add -p: implement the 'g' ("goto") command
add-patch.c
9254bdfb 981) strbuf_setlen(out, len + SUMMARY_LINE_WIDTH);
9254bdfb 1158) err(s, _("No other hunks to goto"));
9254bdfb 1159) continue;
9254bdfb 1174) break;
9254bdfb 1181) err(s, _("Invalid number: '%s'"),
9254bdfb 1189) (int)file_diff->hunk_nr);
Johannes Schindelin bcdd297b built-in add -p: implement hunk editing
add-patch.c
bcdd297b 770) hunk->colored_start = s->colored.len;
bcdd297b 771) for (current = hunk->start; current < hunk->end; ) {
bcdd297b 772) for (eol = current; eol < hunk->end; eol++)
bcdd297b 773) if (plain[eol] == '\n')
bcdd297b 774) break;
bcdd297b 775) next = eol + (eol < hunk->end);
bcdd297b 776) if (eol > current && plain[eol - 1] == '\r')
bcdd297b 777) eol--;
bcdd297b 779) strbuf_addstr(&s->colored,
bcdd297b 780) plain[current] == '-' ?
bcdd297b 782) plain[current] == '+' ?
bcdd297b 785) strbuf_add(&s->colored, plain + current, eol - current);
bcdd297b 786) strbuf_addstr(&s->colored, GIT_COLOR_RESET);
bcdd297b 787) if (next > eol)
bcdd297b 788) strbuf_add(&s->colored, plain + eol, next - eol);
bcdd297b 789) current = next;
bcdd297b 791) hunk->colored_end = s->colored.len;
bcdd297b 825) return -1;
bcdd297b 840) return 0;
bcdd297b 906) return -1;
bcdd297b 910) case 'y': return 1;
bcdd297b 912) }
bcdd297b 927) *hunk = backup;
bcdd297b 928) return -1;
bcdd297b 1248) err(s, _("Sorry, cannot edit this hunk"));
Johannes Schindelin 11f2c0da built-in add -p: coalesce hunks after splitting them
add-patch.c
11f2c0da 449) return 0;
Johannes Schindelin 510aeca1 built-in add -p: implement the hunk splitting feature
add-patch.c
510aeca1 235) hunk->splittable_into++;
510aeca1 356) return sb->len;
510aeca1 618) return 0;
510aeca1 629) memmove(file_diff->hunk + hunk_index + splittable_into,
510aeca1 630) file_diff->hunk + hunk_index + 1,
510aeca1 631) (file_diff->hunk_nr - hunk_index - splittable_into)
510aeca1 642) colored_current = hunk->colored_start;
510aeca1 660) hunk[1].colored_start = colored_current;
510aeca1 675) ch = marker ? marker : ' ';
510aeca1 689) colored_current =
510aeca1 690) find_next_line(&s->colored,
510aeca1 732) hunk->colored_end = colored_current;
510aeca1 757) hunk->colored_end = colored_end;
510aeca1 1240) err(s, _("Sorry, cannot split this hunk"));
Johannes Schindelin f6aa7ecc built-in add -i: start implementing the `patch` functionality in C
add-patch.c
f6aa7ecc 209) eol = pend;
f6aa7ecc 1051) undecided_previous = i;
f6aa7ecc 1052) break;
f6aa7ecc 1073) strbuf_addstr(&s->buf, ",k");
f6aa7ecc 1107) continue;
f6aa7ecc 1120) if (hunk->use == UNDECIDED_HUNK)
f6aa7ecc 1121) hunk->use = USE_HUNK;
f6aa7ecc 1317) strbuf_release(&s.plain);
f6aa7ecc 1319) return -1;
Johannes Schindelin e3bd11b4 built-in add -p: show colored hunks by default
add-patch.c
e3bd11b4 175) argv_array_clear(&args);
e3bd11b4 320) colored_p = colored_pend;
e3bd11b4 1318) strbuf_release(&s.colored);
Johannes Schindelin b38dd9e7 strbuf: add a helper function to call the editor "on an strbuf"
strbuf.c
b38dd9e7 1140) res = error_errno(_("could not open '%s' for writing"), path);
b38dd9e7 1142) res = error_errno(_("could not write to '%s'"), path);
b38dd9e7 1143) close(fd);
b38dd9e7 1145) res = error_errno(_("could not close '%s'"), path);
b38dd9e7 1149) res = error_errno(_("could not edit '%s'"), path);
Tanushree Tumane 51a0a4ed bisect--helper: avoid use-after-free
builtin/bisect--helper.c
51a0a4ed 177) return -1;
^ permalink raw reply
* Re: [PATCH 0/9] [RFC] Changed Paths Bloom Filters
From: Derrick Stolee @ 2019-12-27 16:11 UTC (permalink / raw)
To: Jeff King, Garima Singh via GitGitGadget
Cc: git, szeder.dev, jonathantanmy, jeffhost, me, Junio C Hamano
In-Reply-To: <20191222093036.GA3449072@coredump.intra.peff.net>
On 12/22/2019 4:30 AM, Jeff King wrote:
> On Fri, Dec 20, 2019 at 10:05:11PM +0000, Garima Singh via GitGitGadget wrote:
>
>> Adopting changed path bloom filters has been discussed on the list before,
>> and a prototype version was worked on by SZEDER Gábor, Jonathan Tan and Dr.
>> Derrick Stolee [1]. This series is based on Dr. Stolee's approach [2] and
>> presents an updated and more polished RFC version of the feature.
>
> Great to see progress here. I probably won't have time to review this
> carefully before the new year, but I did notice some low-hanging fruit
> on the generation side.
>
> So here are a few patches to reduce the CPU and memory usage. They could
> be squashed in at the appropriate spots, or perhaps taken as inspiration
> if there are better solutions (especially for the first one).
I tested these patches with the Linux kernel repo and reported my results
on each patch. However, I wanted to also test on a larger internal repo
(the AzureDevOps repo), which has ~500 commits with more than 512 changes,
and generally has larger diffs than the Linux kernel repo.
| Version | Time | Memory |
|----------|--------|--------|
| Garima | 16m36s | 27.0gb |
| Peff 1 | 6m32s | 28.0gb |
| Peff 2 | 6m48s | 5.6gb |
| Peff 3 | 6m14s | 4.5gb |
| Shortcut | 3m47s | 4.5gb |
For reference, I found the time and memory information using
"/usr/bin/time --verbose" in a bash script.
> I think we could go further still, by actually doing a non-recursive
> diff_tree_oid(), and then recursing into sub-trees ourselves. That would
> save us having to split apart each path to add the leading paths to the
> hashmap (most of which will be duplicates if the commit touched "a/b/c"
> and "a/b/d", etc). I doubt it would be that huge a speedup though. We
> have to keep a list of the touched paths anyway (since the bloom key
> parameters depend on the number of entries), and most of the time is
> almost certainly spent inflating the trees in the first place. However
> it might be easier to follow the code, and it would make it simpler to
> stop traversing at the 512-entry limit, rather than generating a huge
> diff only to throw it away.
By "Shortcut" in the table above, I mean the following patch on top of
Garima's and Peff's changes. It inserts a max_changes option into struct
diff_options to halt the diff early. This seemed like an easier change
than creating a new tree diff algorithm wholesale.
Thanks,
-Stolee
-->8--
From: Derrick Stolee <dstolee@microsoft.com>
Date: Fri, 27 Dec 2019 10:13:48 -0500
Subject: [PATCH] diff: halt tree-diff early after max_changes
When computing the changed-paths bloom filters for the commit-graph,
we limit the size of the filter by restricting the number of paths
in the diff. Instead of computing a large diff and then ignoring the
result, it is better to halt the diff computation early.
Create a new "max_changes" option in struct diff_options. If non-zero,
then halt the diff computation after discovering strictly more changed
paths. This includes paths corresponding to trees that change.
Use this max_changes option in the bloom filter calculations. This
reduces the time taken to compute the filters for the Linux kernel
repo from 2m50s to 2m35s. For a larger repo with more commits changing
many paths, the time reduces from 6 minutes to under 4 minutes.
Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
---
bloom.c | 4 +++-
diff.h | 5 +++++
tree-diff.c | 5 +++++
3 files changed, 13 insertions(+), 1 deletion(-)
diff --git a/bloom.c b/bloom.c
index ea77631cc2..83dde2378b 100644
--- a/bloom.c
+++ b/bloom.c
@@ -155,6 +155,7 @@ struct bloom_filter *get_bloom_filter(struct repository *r,
struct bloom_filter_settings settings = DEFAULT_BLOOM_FILTER_SETTINGS;
int i;
struct diff_options diffopt;
+ int max_changes = 512;
filter = bloom_filter_slab_at(&bloom_filters, c);
@@ -171,6 +172,7 @@ struct bloom_filter *get_bloom_filter(struct repository *r,
repo_diff_setup(r, &diffopt);
diffopt.flags.recursive = 1;
+ diffopt.max_changes = max_changes;
diff_setup_done(&diffopt);
if (c->parents)
@@ -179,7 +181,7 @@ struct bloom_filter *get_bloom_filter(struct repository *r,
diff_tree_oid(NULL, &c->object.oid, "", &diffopt);
diffcore_std(&diffopt);
- if (diff_queued_diff.nr <= 512) {
+ if (diff_queued_diff.nr <= max_changes) {
struct hashmap pathmap;
struct pathmap_hash_entry* e;
struct hashmap_iter iter;
diff --git a/diff.h b/diff.h
index 6febe7e365..9443dc1b00 100644
--- a/diff.h
+++ b/diff.h
@@ -285,6 +285,11 @@ struct diff_options {
/* Number of hexdigits to abbreviate raw format output to. */
int abbrev;
+ /* If non-zero, then stop computing after this many changes. */
+ int max_changes;
+ /* For internal use only. */
+ int num_changes;
+
int ita_invisible_in_index;
/* white-space error highlighting */
#define WSEH_NEW (1<<12)
diff --git a/tree-diff.c b/tree-diff.c
index 33ded7f8b3..16a21d9f34 100644
--- a/tree-diff.c
+++ b/tree-diff.c
@@ -434,6 +434,9 @@ static struct combine_diff_path *ll_diff_tree_paths(
if (diff_can_quit_early(opt))
break;
+ if (opt->max_changes && opt->num_changes > opt->max_changes)
+ break;
+
if (opt->pathspec.nr) {
skip_uninteresting(&t, base, opt);
for (i = 0; i < nparent; i++)
@@ -518,6 +521,7 @@ static struct combine_diff_path *ll_diff_tree_paths(
/* t↓ */
update_tree_entry(&t);
+ opt->num_changes++;
}
/* t > p[imin] */
@@ -535,6 +539,7 @@ static struct combine_diff_path *ll_diff_tree_paths(
skip_emit_tp:
/* ∀ pi=p[imin] pi↓ */
update_tp_entries(tp, nparent);
+ opt->num_changes++;
}
}
--
2.25.0.rc0
^ permalink raw reply related
* Re: [PATCH 1/1] sparse-checkout: list folders in cone mode
From: Elijah Newren @ 2019-12-27 15:52 UTC (permalink / raw)
To: Derrick Stolee via GitGitGadget
Cc: Git Mailing List, SZEDER Gábor, jon, Derrick Stolee,
Junio C Hamano
In-Reply-To: <07be7b8dda679d79ac9b218b2a9b08e47d7762fd.1577393347.git.gitgitgadget@gmail.com>
On Thu, Dec 26, 2019 at 12:49 PM Derrick Stolee via GitGitGadget
<gitgitgadget@gmail.com> wrote:
>
> From: Derrick Stolee <dstolee@microsoft.com>
>
> When core.sparseCheckoutCone is enabled, the 'git sparse-checkout set'
> command taks a list of folders as input, then creates an ordered
> list of sparse-checkout patterns such that those folders are
> recursively included and all sibling blobs along the parent folders
> are also included. Listing the patterns is less user-friendly than the
> folders themselves.
I didn't spot anything that Junio hasn't already mentioned, but I'm
just chiming in to say that this sounds good and thanks so much for
working on the sparse-checkout command. :-)
^ permalink raw reply
* Re: [PATCH 3/3] commit-graph: stop using full rev_info for diffs
From: Derrick Stolee @ 2019-12-27 14:53 UTC (permalink / raw)
To: Jeff King, Garima Singh via GitGitGadget
Cc: git, szeder.dev, jonathantanmy, jeffhost, me, Junio C Hamano
In-Reply-To: <20191222093222.GC3460818@coredump.intra.peff.net>
On 12/22/2019 4:32 AM, Jeff King wrote:
> When we perform a diff to get the set of changed paths for a commit,
> we initialize a full "struct rev_info" with setup_revisions(). But the
> only part of it we use is the diff_options struct. Besides being overly
> complex, this also leaks memory, as we use the fake argv to
> setup_revisions() create a pending array which is never cleared.
>
> Let's just use diff_options directly. This reduces the peak heap usage
> of "git commit-graph write --changed-paths" on linux.git from ~4GB to
> ~1.2GB.
In my testing, this went from ~6gb to ~4gb.
I'm guessing that my memory difference is related to how poorly my
packs are repacked/redeltified.
Thanks,
-Stolee
^ permalink raw reply
* Re: [PATCH 2/3] commit-graph: free large diffs, too
From: Derrick Stolee @ 2019-12-27 14:52 UTC (permalink / raw)
To: Jeff King, Garima Singh via GitGitGadget
Cc: git, szeder.dev, jonathantanmy, jeffhost, me, Junio C Hamano
In-Reply-To: <20191222093216.GB3460818@coredump.intra.peff.net>
On 12/22/2019 4:32 AM, Jeff King wrote:
> If a diff we compute for --changed-path has more than 512 entries, we
> don't bother generating a bloom filter for it. But since we don't
> iterate over diff_queued_diff, we also don't free the filepairs and
> filespecs from the diff before clearing the queue. Let's make sure we do
> so.
>
> This drops the peak heap usage of "commit-graph write --changed-paths"
> on linux.git from ~8GB to ~4GB.
In my testing, the heap size went from ~10gb to ~6gb.
Thanks,
-Stolee
^ permalink raw reply
* Re: [PATCH 1/3] commit-graph: examine changed-path objects in pack order
From: Derrick Stolee @ 2019-12-27 14:51 UTC (permalink / raw)
To: Jeff King, Garima Singh via GitGitGadget
Cc: git, szeder.dev, jonathantanmy, jeffhost, me, Junio C Hamano
In-Reply-To: <20191222093206.GA3460818@coredump.intra.peff.net>
On 12/22/2019 4:32 AM, Jeff King wrote:
> Looking at the diff of commit objects in pack order is much faster than
> in sha1 order, as it gives locality to the access of tree deltas
> (whereas sha1 order is effectively random). Unfortunately the
> commit-graph code sorts the commits (several times, sometimes as an oid
> and sometimes a pointer-to-commit), and we ultimately traverse in sha1
> order.
>
> Instead, let's remember the position at which we see each commit, and
> traverse in that order when looking at bloom filters. This drops my time
> for "git commit-graph write --changed-paths" in linux.git from ~4
> minutes to ~1.5 minutes.
I'm doing my own perf tests on these patches, and my copy of linux.git
has four packs of varying sizes (corresponding with my rare fetches and
lack of repacks). My time goes from 3m50s to 3m00s. I was confused at
first, but then realized that I used the "--reachable" flag. In that
case, we never run set_commit_pos(), so all positions are equal and the
sort is not helpful.
I thought that inserting some set_commit_pos() calls into close_reachable()
and add_missing_parents() would give some amount of time-order to the
commits as we compute the filters. However, the time did not change at
all.
I've included the patch below for reference, anyway.
Thanks,
-Stolee
-->8--
From e7c63d8db09be81ce213ba7f112bb3d2f537bf4a Mon Sep 17 00:00:00 2001
From: Derrick Stolee <dstolee@microsoft.com>
Date: Fri, 27 Dec 2019 09:47:49 -0500
Subject: [PATCH] commit-graph: set commit positions for --reachable
When running 'git commit-graph write --changed-paths', we sort the
commits by pack-order to save time when computing the changed-paths
bloom filters. This does not help when finding the commits via the
--reachable flag.
Add calls to set_commit_pos() when walking the reachable commits,
which provides an ordering similar to a topological ordering.
Unfortunately, the performance did not improve with this change.
Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
---
commit-graph.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/commit-graph.c b/commit-graph.c
index bf6c663772..a6c4ab401e 100644
--- a/commit-graph.c
+++ b/commit-graph.c
@@ -1126,6 +1126,8 @@ static void add_missing_parents(struct write_commit_graph_context *ctx, struct c
oidcpy(&ctx->oids.list[ctx->oids.nr], &(parent->item->object.oid));
ctx->oids.nr++;
parent->item->object.flags |= REACHABLE;
+
+ set_commit_pos(ctx->r, &parent->item->object.oid);
}
}
}
@@ -1142,8 +1144,10 @@ static void close_reachable(struct write_commit_graph_context *ctx)
for (i = 0; i < ctx->oids.nr; i++) {
display_progress(ctx->progress, i + 1);
commit = lookup_commit(ctx->r, &ctx->oids.list[i]);
- if (commit)
+ if (commit) {
commit->object.flags |= REACHABLE;
+ set_commit_pos(ctx->r, &commit->object.oid);
+ }
}
stop_progress(&ctx->progress);
--
2.25.0.rc0
^ permalink raw reply related
* Re: [PATCH 1/1] sparse-checkout: list folders in cone mode
From: Derrick Stolee @ 2019-12-27 14:05 UTC (permalink / raw)
To: Junio C Hamano, Derrick Stolee via GitGitGadget
Cc: git, szeder.dev, newren, jon, Derrick Stolee
In-Reply-To: <xmqqo8vukcqv.fsf@gitster-ct.c.googlers.com>
On 12/26/2019 4:17 PM, Junio C Hamano wrote:
> "Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
>> From: Derrick Stolee <dstolee@microsoft.com>
>
>> Subject: Re: [PATCH 1/1] sparse-checkout: list folders in cone mode
> s/folder/directory/ everywhere as the rest of Git.
>
>> When core.sparseCheckoutCone is enabled, the 'git sparse-checkout set'
>> command taks a list of folders as input, then creates an ordered
>
> "takes"
Good catch.
>> list of sparse-checkout patterns such that those folders are
>> recursively included and all sibling blobs along the parent folders
>
> In this sentence, what does a "blob" really mean? Do you mean a
> filesystem entity, that is not a folder, that is immediately
> contained in the "parent folder" (in other words, regular files
> and symbolic links)?
You're right, I'm using strange wording here. How about "sibling
entries"?
> How would this interact with a submodule by the way?
I just checked with the Git repo by running:
git submodule init
git submodule update
git sparse-checkout init --cone
The working directory then contains all blobs at root AND the
sha1collisiondetection submodule. Interesting that the sparse-
checkout feature ignores submodules. That seems like the best
approach since the user can already enlist in a subset of the
submodules.
>> are also included. Listing the patterns is less user-friendly than the
>> folders themselves.
>>
>> In cone mode, and as long as the patterns match the expected cone-mode
>> pattern types, change the output of 'git sparse-checkout list' to only
>> show the folders that created the patterns.
>> ...
>> +In the cone mode case, the `git sparse-checkout list` subcommand will list the
>> +folders that define the recursive patterns. For the example sparse-checkout file
>> +above, the output is as follows:
>> +
>> +--------------------------
>> +$ git sparse-checkout list
>> +A/B/C
>> +--------------------------
>> +
>
> Sounds like a worthwhile usability improvement.
Thanks,
-Stolee
^ permalink raw reply
* Re: [PATCH v2 1/1] gpg-interface: add minTrustLevel as a configuration option
From: Hans Jerry Illikainen @ 2019-12-27 13:46 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <xmqqblrx5yxu.fsf@gitster-ct.c.googlers.com>
On Tue, Dec 24 2019, Junio C Hamano wrote:
> Hans Jerry Illikainen <hji@dyntopia.com> writes:
>
>> + /* Do we have trust level? */
>> + if (sigcheck_gpg_status[i].flags & GPG_STATUS_TRUST_LEVEL) {
>> + /*
>> + * GPG v1 and v2 differs in how the
>> + * TRUST_ lines are written. Some
>> + * trust lines contain no additional
>> + * space-separated information for v1.
>> + */
>> + next = strchr(line, ' ');
>> + if (!next)
>> + next = strchrnul(line, '\n');
>> + trust = xmemdupz(line, next - line);
>
> I wonder if telling strcspn() to stop at either SP or LF is more in
> line with the existing codebase [*1*] and/or more readable. It
> would make this part to:
>
> size_t trust_size = strcspn(line, " \n");
> trust = xmemdupz(line, trust_size);
>
> without the need to use or update the 'next' variable, if I am not
> mistaken?
I agree; fixed in v3.
> By the way, while we are looking at this patch, I notice that,
> throughout the function, the use of variable 'next' feels rather
> misleading, at least to me.
>
> [...]
>
> I wonder if the code becomes less misleading if we either (1)
> renamed 'next' to a name that hints more strongly that it is not the
> 'next' line but the end of the current token we are interested in,
> or (2) get rid of the pointer and instead counted size of the
> current token we are interested in, or perhaps both?
Yeah the name 'next' does seem a bit counter-intuitive when used in
relation to 'line'. Looking through the function it seems that both (1)
and (2) would work.
> This is not the fault of this patch, but I just mention it before I
> forget.
>
> Thanks.
Thanks for the review!
--
hji
^ permalink raw reply
* [PATCH v3 1/1] gpg-interface: add minTrustLevel as a configuration option
From: Hans Jerry Illikainen @ 2019-12-27 13:55 UTC (permalink / raw)
To: git; +Cc: Hans Jerry Illikainen
In-Reply-To: <20191227135557.31437-1-hji@dyntopia.com>
Previously, signature verification for merge and pull operations checked
if the key had a trust-level of either TRUST_NEVER or TRUST_UNDEFINED in
verify_merge_signature(). If that was the case, the process die()d.
The other code paths that did signature verification relied entirely on
the return code from check_commit_signature(). And signatures made with
a good key, irregardless of its trust level, was considered valid by
check_commit_signature().
This difference in behavior might induce users to erroneously assume
that the trust level of a key in their keyring is always considered by
Git, even for operations where it is not (e.g. during a verify-commit or
verify-tag).
The way it worked was by gpg-interface.c storing the result from the
key/signature status *and* the lowest-two trust levels in the `result`
member of the signature_check structure (the last of these status lines
that were encountered got written to `result`). These are documented in
GPG under the subsection `General status codes` and `Key related`,
respectively [1].
The GPG documentation says the following on the TRUST_ status codes [1]:
"""
These are several similar status codes:
- TRUST_UNDEFINED <error_token>
- TRUST_NEVER <error_token>
- TRUST_MARGINAL [0 [<validation_model>]]
- TRUST_FULLY [0 [<validation_model>]]
- TRUST_ULTIMATE [0 [<validation_model>]]
For good signatures one of these status lines are emitted to
indicate the validity of the key used to create the signature.
The error token values are currently only emitted by gpgsm.
"""
My interpretation is that the trust level is conceptionally different
from the validity of the key and/or signature. That seems to also have
been the assumption of the old code in check_signature() where a result
of 'G' (as in GOODSIG) and 'U' (as in TRUST_NEVER or TRUST_UNDEFINED)
were both considered a success.
The two cases where a result of 'U' had special meaning were in
verify_merge_signature() (where this caused git to die()) and in
format_commit_one() (where it affected the output of the %G? format
specifier).
I think it makes sense to refactor the processing of TRUST_ status lines
such that users can configure a minimum trust level that is enforced
globally, rather than have individual parts of git (e.g. merge) do it
themselves (except for a grace period with backward compatibility).
I also think it makes sense to not store the trust level in the same
struct member as the key/signature status. While the presence of a
TRUST_ status code does imply that the signature is good (see the first
paragraph in the included snippet above), as far as I can tell, the
order of the status lines from GPG isn't well-defined; thus it would
seem plausible that the trust level could be overwritten with the
key/signature status if they were stored in the same member of the
signature_check structure.
This patch introduces a new configuration option: gpg.minTrustLevel. It
consolidates trust-level verification to gpg-interface.c and adds a new
`trust_level` member to the signature_check structure.
Backward-compatibility is maintained by introducing a special case in
verify_merge_signature() such that if no user-configurable
gpg.minTrustLevel is set, then the old behavior of rejecting
TRUST_UNDEFINED and TRUST_NEVER is enforced. If, on the other hand,
gpg.minTrustLevel is set, then that value overrides the old behavior.
Similarly, the %G? format specifier will continue show 'U' for
signatures made with a key that has a trust level of TRUST_UNDEFINED or
TRUST_NEVER, even though the 'U' character no longer exist in the
`result` member of the signature_check structure. A new format
specifier, %GT, is also introduced for users that want to show all
possible trust levels for a signature.
Another approach would have been to simply drop the trust-level
requirement in verify_merge_signature(). This would also have made the
behavior consistent with other parts of git that perform signature
verification. However, requiring a minimum trust level for signing keys
does seem to have a real-world use-case. For example, the build system
used by the Qubes OS project currently parses the raw output from
verify-tag in order to assert a minimum trust level for keys used to
sign git tags [2].
[1] https://git.gnupg.org/cgi-bin/gitweb.cgi?p=gnupg.git;a=blob;f=doc/doc/DETAILS;h=bd00006e933ac56719b1edd2478ecd79273eae72;hb=refs/heads/master
[2] https://github.com/QubesOS/qubes-builder/blob/9674c1991deef45b1a1b1c71fddfab14ba50dccf/scripts/verify-git-tag#L43
Signed-off-by: Hans Jerry Illikainen <hji@dyntopia.com>
---
Documentation/config/gpg.txt | 15 +++++
Documentation/pretty-formats.txt | 1 +
builtin/merge.c | 9 ++-
builtin/pull.c | 13 ++++-
commit.c | 12 ++--
commit.h | 12 +++-
gpg-interface.c | 91 ++++++++++++++++++++++++++----
gpg-interface.h | 10 +++-
pretty.c | 30 +++++++++-
t/t5573-pull-verify-signatures.sh | 64 +++++++++++++++++++++
t/t7030-verify-tag.sh | 24 ++++++++
t/t7510-signed-commit.sh | 39 +++++++++++++
t/t7612-merge-verify-signatures.sh | 22 ++++++++
13 files changed, 319 insertions(+), 23 deletions(-)
diff --git a/Documentation/config/gpg.txt b/Documentation/config/gpg.txt
index cce2c89245..d94025cb36 100644
--- a/Documentation/config/gpg.txt
+++ b/Documentation/config/gpg.txt
@@ -18,3 +18,18 @@ gpg.<format>.program::
chose. (see `gpg.program` and `gpg.format`) `gpg.program` can still
be used as a legacy synonym for `gpg.openpgp.program`. The default
value for `gpg.x509.program` is "gpgsm".
+
+gpg.minTrustLevel::
+ Specifies a minimum trust level for signature verification. If
+ this option is unset, then signature verification for merge
+ operations require a key with at least `marginal` trust. Other
+ operations that perform signature verification require a key
+ with at least `undefined` trust. Setting this option overrides
+ the required trust-level for all operations. Supported values,
+ in increasing order of significance:
++
+* `undefined`
+* `never`
+* `marginal`
+* `fully`
+* `ultimate`
diff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt
index 1a7212ce5a..a4b6f49186 100644
--- a/Documentation/pretty-formats.txt
+++ b/Documentation/pretty-formats.txt
@@ -226,6 +226,7 @@ endif::git-rev-list[]
'%GF':: show the fingerprint of the key used to sign a signed commit
'%GP':: show the fingerprint of the primary key whose subkey was used
to sign a signed commit
+'%GT':: show the trust level for the key used to sign a signed commit
'%gD':: reflog selector, e.g., `refs/stash@{1}` or `refs/stash@{2
minutes ago}`; the format follows the rules described for the
`-g` option. The portion before the `@` is the refname as
diff --git a/builtin/merge.c b/builtin/merge.c
index 062e911441..d127d2225f 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -62,6 +62,7 @@ static int show_diffstat = 1, shortlog_len = -1, squash;
static int option_commit = -1;
static int option_edit = -1;
static int allow_trivial = 1, have_message, verify_signatures;
+static int check_trust_level = 1;
static int overwrite_ignore = 1;
static struct strbuf merge_msg = STRBUF_INIT;
static struct strategy **use_strategies;
@@ -631,6 +632,8 @@ static int git_merge_config(const char *k, const char *v, void *cb)
} else if (!strcmp(k, "commit.gpgsign")) {
sign_commit = git_config_bool(k, v) ? "" : NULL;
return 0;
+ } else if (!strcmp(k, "gpg.mintrustlevel")) {
+ check_trust_level = 0;
}
status = fmt_merge_msg_config(k, v, cb);
@@ -1397,7 +1400,8 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
die(_("Can merge only exactly one commit into empty head"));
if (verify_signatures)
- verify_merge_signature(remoteheads->item, verbosity);
+ verify_merge_signature(remoteheads->item, verbosity,
+ check_trust_level);
remote_head_oid = &remoteheads->item->object.oid;
read_empty(remote_head_oid, 0);
@@ -1420,7 +1424,8 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
if (verify_signatures) {
for (p = remoteheads; p; p = p->next) {
- verify_merge_signature(p->item, verbosity);
+ verify_merge_signature(p->item, verbosity,
+ check_trust_level);
}
}
diff --git a/builtin/pull.c b/builtin/pull.c
index d25ff13a60..d4e3e77c8e 100644
--- a/builtin/pull.c
+++ b/builtin/pull.c
@@ -107,6 +107,7 @@ static char *opt_ff;
static char *opt_verify_signatures;
static int opt_autostash = -1;
static int config_autostash;
+static int check_trust_level = 1;
static struct argv_array opt_strategies = ARGV_ARRAY_INIT;
static struct argv_array opt_strategy_opts = ARGV_ARRAY_INIT;
static char *opt_gpg_sign;
@@ -355,6 +356,8 @@ static enum rebase_type config_get_rebase(void)
*/
static int git_pull_config(const char *var, const char *value, void *cb)
{
+ int status;
+
if (!strcmp(var, "rebase.autostash")) {
config_autostash = git_config_bool(var, value);
return 0;
@@ -362,7 +365,14 @@ static int git_pull_config(const char *var, const char *value, void *cb)
recurse_submodules = git_config_bool(var, value) ?
RECURSE_SUBMODULES_ON : RECURSE_SUBMODULES_OFF;
return 0;
+ } else if (!strcmp(var, "gpg.mintrustlevel")) {
+ check_trust_level = 0;
}
+
+ status = git_gpg_config(var, value, cb);
+ if (status)
+ return status;
+
return git_default_config(var, value, cb);
}
@@ -587,7 +597,8 @@ static int pull_into_void(const struct object_id *merge_head,
die(_("unable to access commit %s"),
oid_to_hex(merge_head));
- verify_merge_signature(commit, opt_verbosity);
+ verify_merge_signature(commit, opt_verbosity,
+ check_trust_level);
}
/*
diff --git a/commit.c b/commit.c
index 434ec030d6..3f91d3efc5 100644
--- a/commit.c
+++ b/commit.c
@@ -1136,21 +1136,23 @@ int check_commit_signature(const struct commit *commit, struct signature_check *
return ret;
}
-void verify_merge_signature(struct commit *commit, int verbosity)
+void verify_merge_signature(struct commit *commit, int verbosity,
+ int check_trust)
{
char hex[GIT_MAX_HEXSZ + 1];
struct signature_check signature_check;
+ int ret;
memset(&signature_check, 0, sizeof(signature_check));
- check_commit_signature(commit, &signature_check);
+ ret = check_commit_signature(commit, &signature_check);
find_unique_abbrev_r(hex, &commit->object.oid, DEFAULT_ABBREV);
switch (signature_check.result) {
case 'G':
+ if (ret || (check_trust && signature_check.trust_level < TRUST_MARGINAL))
+ die(_("Commit %s has an untrusted GPG signature, "
+ "allegedly by %s."), hex, signature_check.signer);
break;
- case 'U':
- die(_("Commit %s has an untrusted GPG signature, "
- "allegedly by %s."), hex, signature_check.signer);
case 'B':
die(_("Commit %s has a bad GPG signature "
"allegedly by %s."), hex, signature_check.signer);
diff --git a/commit.h b/commit.h
index 221cdaa34b..008a0fa4a0 100644
--- a/commit.h
+++ b/commit.h
@@ -383,8 +383,18 @@ int compare_commits_by_author_date(const void *a_, const void *b_, void *unused)
* Verify a single commit with check_commit_signature() and die() if it is not
* a good signature. This isn't really suitable for general use, but is a
* helper to implement consistent logic for pull/merge --verify-signatures.
+ *
+ * The check_trust parameter is meant for backward-compatibility. The GPG
+ * interface verifies key trust with a default trust level that is below the
+ * default trust level for merge operations. Its value should be non-zero if
+ * the user hasn't set a minimum trust level explicitly in their configuration.
+ *
+ * If the user has set a minimum trust level, then that value should be obeyed
+ * and check_trust should be zero, even if the configured trust level is below
+ * the default trust level for merges.
*/
-void verify_merge_signature(struct commit *commit, int verbose);
+void verify_merge_signature(struct commit *commit, int verbose,
+ int check_trust);
int compare_commits_by_commit_date(const void *a_, const void *b_, void *unused);
int compare_commits_by_gen_then_commit_date(const void *a_, const void *b_, void *unused);
diff --git a/gpg-interface.c b/gpg-interface.c
index 5134ce2780..2d538bcd6e 100644
--- a/gpg-interface.c
+++ b/gpg-interface.c
@@ -7,6 +7,8 @@
#include "tempfile.h"
static char *configured_signing_key;
+static enum signature_trust_level configured_min_trust_level = TRUST_UNDEFINED;
+
struct gpg_format {
const char *name;
const char *program;
@@ -85,6 +87,8 @@ void signature_check_clear(struct signature_check *sigc)
#define GPG_STATUS_UID (1<<2)
/* The status includes key fingerprints */
#define GPG_STATUS_FINGERPRINT (1<<3)
+/* The status includes trust level */
+#define GPG_STATUS_TRUST_LEVEL (1<<4)
/* Short-hand for standard exclusive *SIG status with keyid & UID */
#define GPG_STATUS_STDSIG (GPG_STATUS_EXCLUSIVE|GPG_STATUS_KEYID|GPG_STATUS_UID)
@@ -96,13 +100,23 @@ static struct {
} sigcheck_gpg_status[] = {
{ 'G', "GOODSIG ", GPG_STATUS_STDSIG },
{ 'B', "BADSIG ", GPG_STATUS_STDSIG },
- { 'U', "TRUST_NEVER", 0 },
- { 'U', "TRUST_UNDEFINED", 0 },
{ 'E', "ERRSIG ", GPG_STATUS_EXCLUSIVE|GPG_STATUS_KEYID },
{ 'X', "EXPSIG ", GPG_STATUS_STDSIG },
{ 'Y', "EXPKEYSIG ", GPG_STATUS_STDSIG },
{ 'R', "REVKEYSIG ", GPG_STATUS_STDSIG },
{ 0, "VALIDSIG ", GPG_STATUS_FINGERPRINT },
+ { 0, "TRUST_", GPG_STATUS_TRUST_LEVEL },
+};
+
+static struct {
+ const char *key;
+ enum signature_trust_level value;
+} sigcheck_gpg_trust_level[] = {
+ { "UNDEFINED", TRUST_UNDEFINED },
+ { "NEVER", TRUST_NEVER },
+ { "MARGINAL", TRUST_MARGINAL },
+ { "FULLY", TRUST_FULLY },
+ { "ULTIMATE", TRUST_ULTIMATE },
};
static void replace_cstring(char **field, const char *line, const char *next)
@@ -115,6 +129,20 @@ static void replace_cstring(char **field, const char *line, const char *next)
*field = NULL;
}
+static int parse_gpg_trust_level(const char *level,
+ enum signature_trust_level *res)
+{
+ size_t i;
+
+ for (i = 0; i < ARRAY_SIZE(sigcheck_gpg_trust_level); i++) {
+ if (!strcmp(sigcheck_gpg_trust_level[i].key, level)) {
+ *res = sigcheck_gpg_trust_level[i].value;
+ return 0;
+ }
+ }
+ return 1;
+}
+
static void parse_gpg_output(struct signature_check *sigc)
{
const char *buf = sigc->gpg_status;
@@ -136,9 +164,18 @@ static void parse_gpg_output(struct signature_check *sigc)
/* Iterate over all search strings */
for (i = 0; i < ARRAY_SIZE(sigcheck_gpg_status); i++) {
if (skip_prefix(line, sigcheck_gpg_status[i].check, &line)) {
+ /*
+ * GOODSIG, BADSIG etc. can occur only once for
+ * each signature. Therefore, if we had more
+ * than one then we're dealing with multiple
+ * signatures. We don't support them
+ * currently, and they're rather hard to
+ * create, so something is likely fishy and we
+ * should reject them altogether.
+ */
if (sigcheck_gpg_status[i].flags & GPG_STATUS_EXCLUSIVE) {
if (seen_exclusive_status++)
- goto found_duplicate_status;
+ goto error;
}
if (sigcheck_gpg_status[i].result)
@@ -154,6 +191,25 @@ static void parse_gpg_output(struct signature_check *sigc)
replace_cstring(&sigc->signer, line, next);
}
}
+
+ /* Do we have trust level? */
+ if (sigcheck_gpg_status[i].flags & GPG_STATUS_TRUST_LEVEL) {
+ /*
+ * GPG v1 and v2 differs in how the
+ * TRUST_ lines are written. Some
+ * trust lines contain no additional
+ * space-separated information for v1.
+ */
+ size_t trust_size = strcspn(line, " \n");
+ char *trust = xmemdupz(line, trust_size);
+
+ if (parse_gpg_trust_level(trust, &sigc->trust_level)) {
+ free(trust);
+ goto error;
+ }
+ free(trust);
+ }
+
/* Do we have fingerprint? */
if (sigcheck_gpg_status[i].flags & GPG_STATUS_FINGERPRINT) {
const char *limit;
@@ -191,14 +247,7 @@ static void parse_gpg_output(struct signature_check *sigc)
}
return;
-found_duplicate_status:
- /*
- * GOODSIG, BADSIG etc. can occur only once for each signature.
- * Therefore, if we had more than one then we're dealing with multiple
- * signatures. We don't support them currently, and they're rather
- * hard to create, so something is likely fishy and we should reject
- * them altogether.
- */
+error:
sigc->result = 'E';
/* Clear partial data to avoid confusion */
FREE_AND_NULL(sigc->primary_key_fingerprint);
@@ -264,6 +313,7 @@ int check_signature(const char *payload, size_t plen, const char *signature,
int status;
sigc->result = 'N';
+ sigc->trust_level = -1;
status = verify_signed_buffer(payload, plen, signature, slen,
&gpg_output, &gpg_status);
@@ -273,7 +323,8 @@ int check_signature(const char *payload, size_t plen, const char *signature,
sigc->gpg_output = strbuf_detach(&gpg_output, NULL);
sigc->gpg_status = strbuf_detach(&gpg_status, NULL);
parse_gpg_output(sigc);
- status |= sigc->result != 'G' && sigc->result != 'U';
+ status |= sigc->result != 'G';
+ status |= sigc->trust_level < configured_min_trust_level;
out:
strbuf_release(&gpg_status);
@@ -320,6 +371,8 @@ int git_gpg_config(const char *var, const char *value, void *cb)
{
struct gpg_format *fmt = NULL;
char *fmtname = NULL;
+ char *trust;
+ int ret;
if (!strcmp(var, "user.signingkey")) {
if (!value)
@@ -339,6 +392,20 @@ int git_gpg_config(const char *var, const char *value, void *cb)
return 0;
}
+ if (!strcmp(var, "gpg.mintrustlevel")) {
+ if (!value)
+ return config_error_nonbool(var);
+
+ trust = xstrdup_toupper(value);
+ ret = parse_gpg_trust_level(trust, &configured_min_trust_level);
+ free(trust);
+
+ if (ret)
+ return error("unsupported value for %s: %s", var,
+ value);
+ return 0;
+ }
+
if (!strcmp(var, "gpg.program") || !strcmp(var, "gpg.openpgp.program"))
fmtname = "openpgp";
diff --git a/gpg-interface.h b/gpg-interface.h
index 93cc3aff5c..f4e9b4f371 100644
--- a/gpg-interface.h
+++ b/gpg-interface.h
@@ -7,6 +7,14 @@ struct strbuf;
#define GPG_VERIFY_RAW 2
#define GPG_VERIFY_OMIT_STATUS 4
+enum signature_trust_level {
+ TRUST_UNDEFINED,
+ TRUST_NEVER,
+ TRUST_MARGINAL,
+ TRUST_FULLY,
+ TRUST_ULTIMATE,
+};
+
struct signature_check {
char *payload;
char *gpg_output;
@@ -16,7 +24,6 @@ struct signature_check {
* possible "result":
* 0 (not checked)
* N (checked but no further result)
- * U (untrusted good)
* G (good)
* B (bad)
*/
@@ -25,6 +32,7 @@ struct signature_check {
char *key;
char *fingerprint;
char *primary_key_fingerprint;
+ enum signature_trust_level trust_level;
};
void signature_check_clear(struct signature_check *sigc);
diff --git a/pretty.c b/pretty.c
index 305e903192..f5fbbc5ffb 100644
--- a/pretty.c
+++ b/pretty.c
@@ -1311,9 +1311,18 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */
case '?':
switch (c->signature_check.result) {
case 'G':
+ switch (c->signature_check.trust_level) {
+ case TRUST_UNDEFINED:
+ case TRUST_NEVER:
+ strbuf_addch(sb, 'U');
+ break;
+ default:
+ strbuf_addch(sb, 'G');
+ break;
+ }
+ break;
case 'B':
case 'E':
- case 'U':
case 'N':
case 'X':
case 'Y':
@@ -1337,6 +1346,25 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */
if (c->signature_check.primary_key_fingerprint)
strbuf_addstr(sb, c->signature_check.primary_key_fingerprint);
break;
+ case 'T':
+ switch (c->signature_check.trust_level) {
+ case TRUST_UNDEFINED:
+ strbuf_addstr(sb, "undefined");
+ break;
+ case TRUST_NEVER:
+ strbuf_addstr(sb, "never");
+ break;
+ case TRUST_MARGINAL:
+ strbuf_addstr(sb, "marginal");
+ break;
+ case TRUST_FULLY:
+ strbuf_addstr(sb, "fully");
+ break;
+ case TRUST_ULTIMATE:
+ strbuf_addstr(sb, "ultimate");
+ break;
+ }
+ break;
default:
return 0;
}
diff --git a/t/t5573-pull-verify-signatures.sh b/t/t5573-pull-verify-signatures.sh
index 3e9876e197..a53dd8550d 100755
--- a/t/t5573-pull-verify-signatures.sh
+++ b/t/t5573-pull-verify-signatures.sh
@@ -60,6 +60,27 @@ test_expect_success GPG 'pull commit with untrusted signature with --verify-sign
test_i18ngrep "has an untrusted GPG signature" pullerror
'
+test_expect_success GPG 'pull commit with untrusted signature with --verify-signatures and minTrustLevel=ultimate' '
+ test_when_finished "git reset --hard && git checkout initial" &&
+ test_config gpg.minTrustLevel ultimate &&
+ test_must_fail git pull --ff-only --verify-signatures untrusted 2>pullerror &&
+ test_i18ngrep "has an untrusted GPG signature" pullerror
+'
+
+test_expect_success GPG 'pull commit with untrusted signature with --verify-signatures and minTrustLevel=marginal' '
+ test_when_finished "git reset --hard && git checkout initial" &&
+ test_config gpg.minTrustLevel marginal &&
+ test_must_fail git pull --ff-only --verify-signatures untrusted 2>pullerror &&
+ test_i18ngrep "has an untrusted GPG signature" pullerror
+'
+
+test_expect_success GPG 'pull commit with untrusted signature with --verify-signatures and minTrustLevel=undefined' '
+ test_when_finished "git reset --hard && git checkout initial" &&
+ test_config gpg.minTrustLevel undefined &&
+ git pull --ff-only --verify-signatures untrusted >pulloutput &&
+ test_i18ngrep "has a good GPG signature" pulloutput
+'
+
test_expect_success GPG 'pull signed commit with --verify-signatures' '
test_when_finished "git reset --hard && git checkout initial" &&
git pull --verify-signatures signed >pulloutput &&
@@ -79,10 +100,53 @@ test_expect_success GPG 'pull commit with bad signature with --no-verify-signatu
'
test_expect_success GPG 'pull unsigned commit into unborn branch' '
+ test_when_finished "rm -rf empty-repo" &&
git init empty-repo &&
test_must_fail \
git -C empty-repo pull --verify-signatures .. 2>pullerror &&
test_i18ngrep "does not have a GPG signature" pullerror
'
+test_expect_success GPG 'pull commit into unborn branch with bad signature and --verify-signatures' '
+ test_when_finished "rm -rf empty-repo" &&
+ git init empty-repo &&
+ test_must_fail \
+ git -C empty-repo pull --ff-only --verify-signatures ../bad 2>pullerror &&
+ test_i18ngrep "has a bad GPG signature" pullerror
+'
+
+test_expect_success GPG 'pull commit into unborn branch with untrusted signature and --verify-signatures' '
+ test_when_finished "rm -rf empty-repo" &&
+ git init empty-repo &&
+ test_must_fail \
+ git -C empty-repo pull --ff-only --verify-signatures ../untrusted 2>pullerror &&
+ test_i18ngrep "has an untrusted GPG signature" pullerror
+'
+
+test_expect_success GPG 'pull commit into unborn branch with untrusted signature and --verify-signatures and minTrustLevel=ultimate' '
+ test_when_finished "rm -rf empty-repo" &&
+ git init empty-repo &&
+ test_config_global gpg.minTrustLevel ultimate &&
+ test_must_fail \
+ git -C empty-repo pull --ff-only --verify-signatures ../untrusted 2>pullerror &&
+ test_i18ngrep "has an untrusted GPG signature" pullerror
+'
+
+test_expect_success GPG 'pull commit into unborn branch with untrusted signature and --verify-signatures and minTrustLevel=marginal' '
+ test_when_finished "rm -rf empty-repo" &&
+ git init empty-repo &&
+ test_config_global gpg.minTrustLevel marginal &&
+ test_must_fail \
+ git -C empty-repo pull --ff-only --verify-signatures ../untrusted 2>pullerror &&
+ test_i18ngrep "has an untrusted GPG signature" pullerror
+'
+
+test_expect_success GPG 'pull commit into unborn branch with untrusted signature and --verify-signatures and minTrustLevel=undefined' '
+ test_when_finished "rm -rf empty-repo" &&
+ git init empty-repo &&
+ test_config_global gpg.minTrustLevel undefined &&
+ git -C empty-repo pull --ff-only --verify-signatures ../untrusted >pulloutput &&
+ test_i18ngrep "has a good GPG signature" pulloutput
+'
+
test_done
diff --git a/t/t7030-verify-tag.sh b/t/t7030-verify-tag.sh
index 8f077bea60..5c5bc32ccb 100755
--- a/t/t7030-verify-tag.sh
+++ b/t/t7030-verify-tag.sh
@@ -86,6 +86,30 @@ test_expect_success GPGSM 'verify and show signatures x509' '
echo ninth-signed-x509 OK
'
+test_expect_success GPGSM 'verify and show signatures x509 with low minTrustLevel' '
+ test_config gpg.minTrustLevel undefined &&
+ git verify-tag ninth-signed-x509 2>actual &&
+ grep "Good signature from" actual &&
+ ! grep "BAD signature from" actual &&
+ echo ninth-signed-x509 OK
+'
+
+test_expect_success GPGSM 'verify and show signatures x509 with matching minTrustLevel' '
+ test_config gpg.minTrustLevel fully &&
+ git verify-tag ninth-signed-x509 2>actual &&
+ grep "Good signature from" actual &&
+ ! grep "BAD signature from" actual &&
+ echo ninth-signed-x509 OK
+'
+
+test_expect_success GPGSM 'verify and show signatures x509 with high minTrustLevel' '
+ test_config gpg.minTrustLevel ultimate &&
+ test_must_fail git verify-tag ninth-signed-x509 2>actual &&
+ grep "Good signature from" actual &&
+ ! grep "BAD signature from" actual &&
+ echo ninth-signed-x509 OK
+'
+
test_expect_success GPG 'detect fudged signature' '
git cat-file tag seventh-signed >raw &&
sed -e "/^tag / s/seventh/7th forged/" raw >forged1 &&
diff --git a/t/t7510-signed-commit.sh b/t/t7510-signed-commit.sh
index 682b23a068..0c06d22a00 100755
--- a/t/t7510-signed-commit.sh
+++ b/t/t7510-signed-commit.sh
@@ -109,6 +109,21 @@ test_expect_success GPG 'verify-commit exits success on untrusted signature' '
grep "not certified" actual
'
+test_expect_success GPG 'verify-commit exits success with matching minTrustLevel' '
+ test_config gpg.minTrustLevel ultimate &&
+ git verify-commit sixth-signed
+'
+
+test_expect_success GPG 'verify-commit exits success with low minTrustLevel' '
+ test_config gpg.minTrustLevel fully &&
+ git verify-commit sixth-signed
+'
+
+test_expect_success GPG 'verify-commit exits failure with high minTrustLevel' '
+ test_config gpg.minTrustLevel ultimate &&
+ test_must_fail git verify-commit eighth-signed-alt
+'
+
test_expect_success GPG 'verify signatures with --raw' '
(
for commit in initial second merge fourth-signed fifth-signed sixth-signed seventh-signed
@@ -219,6 +234,30 @@ test_expect_success GPG 'show untrusted signature with custom format' '
test_cmp expect actual
'
+test_expect_success GPG 'show untrusted signature with undefined trust level' '
+ cat >expect <<-\EOF &&
+ undefined
+ 65A0EEA02E30CAD7
+ Eris Discordia <discord@example.net>
+ F8364A59E07FFE9F4D63005A65A0EEA02E30CAD7
+ D4BE22311AD3131E5EDA29A461092E85B7227189
+ EOF
+ git log -1 --format="%GT%n%GK%n%GS%n%GF%n%GP" eighth-signed-alt >actual &&
+ test_cmp expect actual
+'
+
+test_expect_success GPG 'show untrusted signature with ultimate trust level' '
+ cat >expect <<-\EOF &&
+ ultimate
+ 13B6F51ECDDE430D
+ C O Mitter <committer@example.com>
+ 73D758744BE721698EC54E8713B6F51ECDDE430D
+ 73D758744BE721698EC54E8713B6F51ECDDE430D
+ EOF
+ git log -1 --format="%GT%n%GK%n%GS%n%GF%n%GP" sixth-signed >actual &&
+ test_cmp expect actual
+'
+
test_expect_success GPG 'show unknown signature with custom format' '
cat >expect <<-\EOF &&
E
diff --git a/t/t7612-merge-verify-signatures.sh b/t/t7612-merge-verify-signatures.sh
index d99218a725..a426f3a89a 100755
--- a/t/t7612-merge-verify-signatures.sh
+++ b/t/t7612-merge-verify-signatures.sh
@@ -66,6 +66,20 @@ test_expect_success GPG 'merge commit with untrusted signature with verification
test_i18ngrep "has an untrusted GPG signature" mergeerror
'
+test_expect_success GPG 'merge commit with untrusted signature with verification and high minTrustLevel' '
+ test_when_finished "git reset --hard && git checkout initial" &&
+ test_config gpg.minTrustLevel marginal &&
+ test_must_fail git merge --ff-only --verify-signatures side-untrusted 2>mergeerror &&
+ test_i18ngrep "has an untrusted GPG signature" mergeerror
+'
+
+test_expect_success GPG 'merge commit with untrusted signature with verification and low minTrustLevel' '
+ test_when_finished "git reset --hard && git checkout initial" &&
+ test_config gpg.minTrustLevel undefined &&
+ git merge --ff-only --verify-signatures side-untrusted >mergeoutput &&
+ test_i18ngrep "has a good GPG signature" mergeoutput
+'
+
test_expect_success GPG 'merge commit with untrusted signature with merge.verifySignatures=true' '
test_when_finished "git reset --hard && git checkout initial" &&
test_config merge.verifySignatures true &&
@@ -73,6 +87,14 @@ test_expect_success GPG 'merge commit with untrusted signature with merge.verify
test_i18ngrep "has an untrusted GPG signature" mergeerror
'
+test_expect_success GPG 'merge commit with untrusted signature with merge.verifySignatures=true and minTrustLevel' '
+ test_when_finished "git reset --hard && git checkout initial" &&
+ test_config merge.verifySignatures true &&
+ test_config gpg.minTrustLevel marginal &&
+ test_must_fail git merge --ff-only side-untrusted 2>mergeerror &&
+ test_i18ngrep "has an untrusted GPG signature" mergeerror
+'
+
test_expect_success GPG 'merge signed commit with verification' '
test_when_finished "git reset --hard && git checkout initial" &&
git merge --verbose --ff-only --verify-signatures side-signed >mergeoutput &&
--
2.24.1.591.g64816733a6
^ 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