* Re: ERANGE strikes again on my Windows build; RFH
From: Michal Suchánek @ 2020-01-05 15:27 UTC (permalink / raw)
To: Jonathan Nieder; +Cc: Johannes Sixt, Git Mailing List
In-Reply-To: <20191230184948.GC57251@google.com>
On Mon, Dec 30, 2019 at 10:49:48AM -0800, Jonathan Nieder wrote:
> Johannes Sixt wrote:
> > Am 30.12.19 um 19:06 schrieb Jonathan Nieder:
>
> >> when
> >> errno is meaningful for a function for a given return value, the usual
> >> convention is
> >>
> >> (1) it *always* sets errno for errors, not conditionally
> >
> > You seem to understand that errno isn't set somewhere where it should be
> > set.
>
> On the contrary: this caller is using errno as an error *indicator*
> instead of a way of *distinguishing* between errors (or to put it
> another way, this caller is treating `errno == 0` as a meaningful
> condition). This means the calling code is buggy.
That works completely fine if the code in question also sets errno to 0
in case there is some other leftover value from a previous library call.
Thanks
Michal
^ permalink raw reply
* [PATCH 1/1] editorconfig: indent text files with tabs
From: Hans Jerry Illikainen @ 2020-01-05 14:00 UTC (permalink / raw)
To: git; +Cc: Hans Jerry Illikainen
In-Reply-To: <20200105140055.19679-1-hji@dyntopia.com>
Previously, the .editorconfig did not specify an indentation style for
text files. However, a quick look for indentation-like spacing suggest
that tabs are more common for documentation:
$ git grep -Pe '^ {4}' -- '*.txt' |wc -l
2683
$ git grep -Pe '^\t' -- '*.txt' |wc -l
14011
Note that there are a lot of files that indent list continuations (and
other things) with a single space -- if the first search was made
without the fixed quantifier the result would look very different.
However, the result does correspond with my anecdotal experience when
editing git documentation.
This commit adds *.txt to .editorconfig as an extension that should be
indented with tabs.
Signed-off-by: Hans Jerry Illikainen <hji@dyntopia.com>
---
.editorconfig | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/.editorconfig b/.editorconfig
index 42cdc4bbfb..f9d819623d 100644
--- a/.editorconfig
+++ b/.editorconfig
@@ -4,7 +4,7 @@ insert_final_newline = true
# The settings for C (*.c and *.h) files are mirrored in .clang-format. Keep
# them in sync.
-[*.{c,h,sh,perl,pl,pm}]
+[*.{c,h,sh,perl,pl,pm,txt}]
indent_style = tab
tab_width = 8
--
2.20.1
^ permalink raw reply related
* [PATCH 0/1] Add *.txt to .editorconfig
From: Hans Jerry Illikainen @ 2020-01-05 14:00 UTC (permalink / raw)
To: git; +Cc: Hans Jerry Illikainen
Minor patch that adds *.txt to .editorconfig to avoid annoyance when
editing git docs with an editor set to indent with space as a default.
Hans Jerry Illikainen (1):
editorconfig: indent text files with tabs
.editorconfig | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
--
2.20.1
^ permalink raw reply
* [PATCH 5/5] clone: support signature verification
From: Hans Jerry Illikainen @ 2020-01-05 13:56 UTC (permalink / raw)
To: git; +Cc: Hans Jerry Illikainen
In-Reply-To: <20200105135616.19102-1-hji@dyntopia.com>
The merge operation (and as a consequence, pull) has had support for
signature verification for quite some time. However, there were no
support for verifying GPG signatures for the initial clone.
Without signature verification for clone operations, users are forced to
checkout a repository before using verify-commit or verify-tag on the
tree. This is potentially problematic for a few reasons:
- It's possible to forget to verify the tree when there's nothing that
enforce signature verification before a cloned repository is used.
- Software on the users system might process the tree before it has been
successfully verified -- for example, file managers that automagically
creates thumbnails for images, videos and documents.
Now, this could be worked around with a --no-checkout clone followed by
signature verification and checkout. There are also various scripts
floating around that more-or-less re-implements 'git clone' with
signature verification by using a combination of init + remote add +
fetch + verify-commit + merge [1]. But none of these options are
particularly user-friendly.
This patch implements signature verification for the clone builtin to
accommodate use-cases where the tree should be verified before use.
There is one major quirk in this patch; namely, recursive clones of
submodules. It is worked around by passing --no-verify-signatures in
clone_submodule(). The rationale for this approach is that:
- The object ID for submodules are stored in the super repository.
Thus, if the super is trusted and signed (and the hash function is
secure), then it seems reasonable to also trust the submodules
referenced in the super repository.
- Propagating signature verification to recursive clones of submodules
would lessen the use of signature verification for clones, because
then users would need the keys for each submodule author in their
keyring. Also, not all submodule refs may be signed.
[1] https://gist.github.com/tribut/50c0f7d0b8341fa6d1784c317d5275f0
Signed-off-by: Hans Jerry Illikainen <hji@dyntopia.com>
---
Documentation/config.txt | 2 +
Documentation/config/clone.txt | 3 +
Documentation/git-clone.txt | 4 +
builtin/clone.c | 46 ++++
builtin/submodule--helper.c | 6 +
t/t5619-clone-verify-signatures.sh | 411 +++++++++++++++++++++++++++++
tag.c | 10 +-
7 files changed, 478 insertions(+), 4 deletions(-)
create mode 100644 Documentation/config/clone.txt
create mode 100755 t/t5619-clone-verify-signatures.sh
diff --git a/Documentation/config.txt b/Documentation/config.txt
index 83e7bba872..fda69e660e 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -331,6 +331,8 @@ include::config/checkout.txt[]
include::config/clean.txt[]
+include::config/clone.txt[]
+
include::config/color.txt[]
include::config/column.txt[]
diff --git a/Documentation/config/clone.txt b/Documentation/config/clone.txt
new file mode 100644
index 0000000000..9fd2ee3395
--- /dev/null
+++ b/Documentation/config/clone.txt
@@ -0,0 +1,3 @@
+clone.verifySignatures::
+ If true, this is equivalent to the --verify-signatures command
+ line option. See linkgit:git-clone[1] for details.
diff --git a/Documentation/git-clone.txt b/Documentation/git-clone.txt
index bf24f1813a..47a9d1e182 100644
--- a/Documentation/git-clone.txt
+++ b/Documentation/git-clone.txt
@@ -285,6 +285,10 @@ or `--mirror` is given)
The number of submodules fetched at the same time.
Defaults to the `submodule.fetchJobs` option.
+--verify-signatures::
+--no-verify-signatures::
+ Verify that the newly created HEAD is signed with a valid key.
+
<repository>::
The (possibly remote) repository to clone from. See the
<<URLS,GIT URLS>> section below for more information on specifying
diff --git a/builtin/clone.c b/builtin/clone.c
index 0fc89ae2b9..037953bc4b 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -32,6 +32,8 @@
#include "connected.h"
#include "packfile.h"
#include "list-objects-filter-options.h"
+#include "object.h"
+#include "tag.h"
/*
* Overall FIXMEs:
@@ -54,6 +56,7 @@ static int deepen;
static char *option_template, *option_depth, *option_since;
static char *option_origin = NULL;
static char *option_branch = NULL;
+static int option_verify_signatures = -1;
static struct string_list option_not = STRING_LIST_INIT_NODUP;
static const char *real_git_dir;
static char *option_upload_pack = "git-upload-pack";
@@ -120,6 +123,8 @@ static struct option builtin_clone_options[] = {
N_("use <name> instead of 'origin' to track upstream")),
OPT_STRING('b', "branch", &option_branch, N_("branch"),
N_("checkout <branch> instead of the remote's HEAD")),
+ OPT_BOOL(0, "verify-signatures", &option_verify_signatures,
+ N_("verify the GPG signature of the newly created HEAD")),
OPT_STRING('u', "upload-pack", &option_upload_pack, N_("path"),
N_("path to git-upload-pack on the remote")),
OPT_STRING(0, "depth", &option_depth, N_("depth"),
@@ -929,6 +934,40 @@ static int path_exists(const char *path)
return !stat(path, &sb);
}
+static int verify_signature(const struct ref *our, const struct ref *remote)
+{
+ const struct object_id *oid = our ? &our->old_oid : &remote->old_oid;
+ enum object_type type = oid_object_info(the_repository, oid, NULL);
+ int flags = option_verbosity ? GPG_VERIFY_FULL : GPG_VERIFY_SHORT;
+
+ if (type == OBJ_COMMIT)
+ return gpg_verify_commit(oid, NULL, NULL, flags);
+ if (type == OBJ_TAG)
+ return gpg_verify_tag(oid, NULL, flags);
+ return error(_("%s: unknown object type: %s"),
+ find_unique_abbrev(oid, DEFAULT_ABBREV), type_name(type));
+}
+
+static int git_clone_config(const char *var, const char *value, void *cb)
+{
+ int status;
+
+ if (!strcmp(var, "clone.verifysignatures")) {
+ option_verify_signatures = git_config_bool(var, value);
+ return 0;
+ }
+
+ if (!strcmp(var, "gpg.verifysignatures") &&
+ option_verify_signatures < 0)
+ option_verify_signatures = git_config_bool(var, value);
+
+ status = git_gpg_config(var, value, cb);
+ if (status)
+ return status;
+
+ return git_default_config(var, value, cb);
+}
+
int cmd_clone(int argc, const char **argv, const char *prefix)
{
int is_bundle = 0, is_local;
@@ -952,6 +991,9 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
struct argv_array ref_prefixes = ARGV_ARRAY_INIT;
packet_trace_identity("clone");
+
+ git_config(git_clone_config, NULL);
+
argc = parse_options(argc, argv, prefix, builtin_clone_options,
builtin_clone_usage, 0);
@@ -1267,6 +1309,10 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
else if (refs && complete_refs_before_fetch)
transport_fetch_refs(transport, mapped_refs);
+ if (option_verify_signatures > 0)
+ if (verify_signature(our_head_points_at, remote_head))
+ die(_("Signature verification failed"));
+
update_remote_refs(refs, mapped_refs, remote_head_points_at,
branch_top.buf, reflog_msg.buf, transport,
!is_local, filter_options.choice);
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index c72931ecd7..9e69c767c9 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -1248,6 +1248,12 @@ static int clone_submodule(const char *path, const char *gitdir, const char *url
if (gitdir && *gitdir)
argv_array_pushl(&cp.args, "--separate-git-dir", gitdir, NULL);
+ /*
+ * The --no-verify-signatures parameter has to be passed in order to
+ * make verification of super-repositories work on recursive clones.
+ */
+ argv_array_push(&cp.args, "--no-verify-signatures");
+
argv_array_push(&cp.args, "--");
argv_array_push(&cp.args, url);
argv_array_push(&cp.args, path);
diff --git a/t/t5619-clone-verify-signatures.sh b/t/t5619-clone-verify-signatures.sh
new file mode 100755
index 0000000000..7a93d57c2a
--- /dev/null
+++ b/t/t5619-clone-verify-signatures.sh
@@ -0,0 +1,411 @@
+#!/bin/sh
+
+test_description='Test cloning repos with signature verification'
+
+. ./test-lib.sh
+. "$TEST_DIRECTORY/lib-gpg.sh"
+
+test_expect_success GPG 'create repositories with signed commits and tags' '
+ echo 0 >a && git add a &&
+ test_tick && git commit -m "initial-unsigned" &&
+ git tag -a -m "unsigned v0" v0-unsigned &&
+
+ git clone . signed &&
+ (
+ cd signed &&
+ echo 1 >b && git add b &&
+ test_tick && git commit -S -m "signed" &&
+ git branch signed-branch &&
+ git tag -s -m "signed v1" v1-signed
+ ) &&
+
+ git clone . unsigned &&
+ (
+ cd unsigned &&
+ echo 2 >c && git add c &&
+ test_tick && git commit -m "unsigned" &&
+ git tag v2-unsigned-shallow &&
+ git tag -a -m "unsigned and annotated" v2-unsigned-annotated
+ ) &&
+
+ git clone signed unsigned-tip &&
+ (
+ cd unsigned-tip &&
+ echo 3 >d && git add d &&
+ test_tick && git commit -m "unsigned tip" &&
+ git tag -a -m "unsigned v3 tip" v3-unsigned-tip &&
+ git branch signed-branch origin/signed-branch
+ ) &&
+
+ git clone signed unsigned-branch &&
+ (
+ cd unsigned-branch &&
+ git checkout -b unsigned-branch &&
+ git commit --amend --no-edit &&
+ git checkout master
+ ) &&
+
+ git clone . signed-tag-unsigned-commit &&
+ (
+ cd signed-tag-unsigned-commit &&
+ git tag -s -m "signed/unsigned v4" v4-signed-tag-unsigned-commit
+ ) &&
+
+ git clone . bad &&
+ (
+ cd bad &&
+ echo 4 >d && git add d &&
+ test_tick && git commit -S -m "bad" &&
+ git cat-file commit HEAD >raw &&
+ sed -e "s/^bad/forged bad/" raw >forged &&
+ git hash-object -w -t commit forged >forged.commit &&
+ git checkout $(cat forged.commit)
+ ) &&
+
+ git clone . untrusted &&
+ (
+ cd untrusted &&
+ echo 5 >e && git add e &&
+ test_tick && git commit -SB7227189 -m "untrusted"
+ ) &&
+
+ git clone unsigned unsigned-detached &&
+ (
+ cd unsigned-detached &&
+ echo 6 >f && git add f &&
+ test_tick && git commit -S -m "signed" &&
+ git checkout HEAD^
+ ) &&
+
+ git clone signed signed-detached &&
+ (
+ cd signed-detached &&
+ echo 7 >g && git add g &&
+ test_tick && git commit -S -m "signed" &&
+ git checkout HEAD^
+ ) &&
+
+ git clone signed signed-with-unsigned-submodule &&
+ (
+ cd signed-with-unsigned-submodule &&
+ git submodule add "file://$PWD/../unsigned" &&
+ git commit -S -m "add submodule"
+ ) &&
+
+ git clone signed signed-with-signed-submodule &&
+ (
+ cd signed-with-signed-submodule &&
+ git submodule add "file://$PWD/../signed" &&
+ git commit -S -m "add submodule"
+ ) &&
+
+ git clone unsigned unsigned-with-unsigned-submodule &&
+ (
+ cd unsigned-with-unsigned-submodule &&
+ git submodule add "file://$PWD/../unsigned" &&
+ git commit -m "add submodule"
+ ) &&
+
+ git clone unsigned unsigned-with-signed-submodule &&
+ (
+ cd unsigned-with-signed-submodule &&
+ git submodule add "file://$PWD/../signed" &&
+ git commit -m "add submodule"
+ )
+'
+
+test_expect_success GPG 'clone signed with --verify-signatures' '
+ test_when_finished "rm -rf dst" &&
+ git clone --verify-signatures signed dst >out &&
+ test_i18ngrep "Commit [0-9a-f]\+ has a good GPG signature by " out
+'
+
+test_expect_success GPG 'clone signed tag with --verify-signatures' '
+ test_when_finished "rm -rf dst" &&
+ git clone -b v1-signed --verify-signatures signed dst >out &&
+ test_i18ngrep "Tag [0-9a-f]\+ has a good GPG signature by " out
+'
+
+test_expect_success GPG 'clone signed with clone.verifySignatures=true' '
+ test_when_finished "rm -rf dst" &&
+ test_config_global clone.verifySignatures true &&
+ git clone signed dst >out &&
+ test_i18ngrep "Commit [0-9a-f]\+ has a good GPG signature by " out
+'
+
+test_expect_success GPG 'clone signed with --depth=1 and clone.verifySignatures=true' '
+ test_when_finished "rm -rf dst" &&
+ test_config_global clone.verifySignatures true &&
+ git clone --depth=1 signed dst >out &&
+ test_i18ngrep "Commit [0-9a-f]\+ has a good GPG signature by " out
+'
+
+test_expect_success GPG 'clone signed with --no-checkout and clone.verifySignatures=true' '
+ test_when_finished "rm -rf dst" &&
+ test_config_global clone.verifySignatures true &&
+ git clone --no-checkout signed dst >out &&
+ test_i18ngrep "Commit [0-9a-f]\+ has a good GPG signature by " out
+'
+
+test_expect_success GPG 'clone signed with --mirror and clone.verifySignatures=true' '
+ test_when_finished "rm -rf dst" &&
+ test_config_global clone.verifySignatures true &&
+ git clone --mirror signed dst >out &&
+ test_i18ngrep "Commit [0-9a-f]\+ has a good GPG signature by " out
+'
+
+test_expect_success GPG 'clone signed without blobs and clone.verifySignatures=true' '
+ test_when_finished "rm -rf dst" &&
+ test_config_global clone.verifySignatures true &&
+ git clone --filter=blob:none signed dst >out &&
+ test_i18ngrep "Commit [0-9a-f]\+ has a good GPG signature by " out
+'
+
+test_expect_success GPG 'clone signed bare with clone.verifySignatures=true' '
+ test_when_finished "rm -rf dst" &&
+ test_config_global clone.verifySignatures true &&
+ git clone --bare signed dst >out &&
+ test_i18ngrep "Commit [0-9a-f]\+ has a good GPG signature by " out
+'
+
+test_expect_success GPG 'clone signed tag with clone.verifySignatures=true' '
+ test_when_finished "rm -rf dst" &&
+ test_config_global clone.verifySignatures true &&
+ git clone -b v1-signed signed dst >out &&
+ test_i18ngrep "Tag [0-9a-f]\+ has a good GPG signature by " out
+'
+
+test_expect_success GPG 'clone unsigned with defaults' '
+ test_when_finished "rm -rf dst" &&
+ git clone unsigned dst >out 2>&1 &&
+ ! test_i18ngrep "GPG signature" out
+'
+
+test_expect_success GPG 'clone unsigned with clone.verifySignatures=true' '
+ test_config_global clone.verifySignatures true &&
+ test_must_fail git clone unsigned dst 2>out &&
+ test_path_is_missing dst &&
+ test_i18ngrep "Commit [0-9a-f]\+ does not have a GPG signature." out
+'
+
+test_expect_success GPG 'clone unsigned with --depth=1 and clone.verifySignatures=true' '
+ test_config_global clone.verifySignatures true &&
+ test_must_fail git clone --depth=1 unsigned dst 2>out &&
+ test_path_is_missing dst &&
+ test_i18ngrep "Commit [0-9a-f]\+ does not have a GPG signature." out
+'
+
+test_expect_success GPG 'clone unsigned with --no-checkout and clone.verifySignatures=true' '
+ test_config_global clone.verifySignatures true &&
+ test_must_fail git clone --no-checkout unsigned dst 2>out &&
+ test_path_is_missing dst &&
+ test_i18ngrep "Commit [0-9a-f]\+ does not have a GPG signature." out
+'
+
+test_expect_success GPG 'clone unsigned with --mirror and clone.verifySignatures=true' '
+ test_config_global clone.verifySignatures true &&
+ test_must_fail git clone --no-checkout unsigned dst 2>out &&
+ test_path_is_missing dst &&
+ test_i18ngrep "Commit [0-9a-f]\+ does not have a GPG signature." out
+'
+
+test_expect_success GPG 'clone unsigned without blobs and clone.verifySignatures=true' '
+ test_config_global clone.verifySignatures true &&
+ test_must_fail git clone --filter=blob:none unsigned dst 2>out &&
+ test_path_is_missing dst &&
+ test_i18ngrep "Commit [0-9a-f]\+ does not have a GPG signature." out
+'
+
+test_expect_success GPG 'clone unsigned bare with clone.verifySignatures=true' '
+ test_config_global clone.verifySignatures true &&
+ test_must_fail git clone --bare unsigned dst 2>out &&
+ test_path_is_missing dst &&
+ test_i18ngrep "Commit [0-9a-f]\+ does not have a GPG signature." out
+'
+
+test_expect_success GPG 'clone unsigned with --verify-signatures and clone.verifySignatures=false' '
+ test_config_global clone.verifySignatures false &&
+ test_must_fail git clone --verify-signatures unsigned dst 2>out &&
+ test_path_is_missing dst &&
+ test_i18ngrep "Commit [0-9a-f]\+ does not have a GPG signature." out
+'
+
+test_expect_success GPG 'clone unsigned with --no-verify-signatures and clone.verifySignatures=true' '
+ test_when_finished "rm -rf dst" &&
+ test_config_global clone.verifySignatures true &&
+ git clone --no-verify-signatures unsigned dst >out &&
+ ! test_i18ngrep "GPG signature" out
+'
+
+test_expect_success GPG 'clone unsigned with --no-verify-signatures and gpg.verifySignatures=true' '
+ test_when_finished "rm -rf dst" &&
+ test_config_global gpg.verifySignatures true &&
+ git clone --no-verify-signatures unsigned dst >out &&
+ ! test_i18ngrep "GPG signature" out
+'
+
+test_expect_success GPG 'clone unsigned with clone.verifySignatures=false and gpg.verifySignatures=true' '
+ test_when_finished "rm -rf dst" &&
+ test_config_global clone.verifySignatures false &&
+ test_config_global gpg.verifySignatures true &&
+ git clone unsigned dst >out &&
+ ! test_i18ngrep "GPG signature" out
+'
+
+test_expect_success GPG 'clone unsigned with gpg.verifySignatures=true and clone.verifySignatures=false' '
+ test_when_finished "rm -rf dst" &&
+ test_config_global gpg.verifySignatures true &&
+ test_config_global clone.verifySignatures false &&
+ git clone unsigned dst >out &&
+ ! test_i18ngrep "GPG signature" out
+'
+
+test_expect_success GPG 'clone unsigned with gpg.verifySignatures=true' '
+ test_config_global gpg.verifySignatures true &&
+ test_must_fail git clone unsigned dst 2>out &&
+ test_path_is_missing dst &&
+ test_i18ngrep "Commit [0-9a-f]\+ does not have a GPG signature." out
+'
+
+test_expect_success GPG 'clone bad signature with --verbose and clone.verifySignatures=true' '
+ test_config_global clone.verifySignatures true &&
+ test_must_fail git clone --verbose bad dst 2>out &&
+ test_path_is_missing dst &&
+ test_i18ngrep "gpg: BAD signature from " out
+'
+
+test_expect_success GPG 'clone bad signature with clone.verifySignatures=true' '
+ test_config_global clone.verifySignatures true &&
+ test_must_fail git clone bad dst 2>out &&
+ test_path_is_missing dst &&
+ test_i18ngrep "Commit [0-9a-f]\+ has a bad GPG signature " out
+'
+
+test_expect_success GPG 'clone untrusted with clone.verifySignatures=true' '
+ test_when_finished "rm -rf dst" &&
+ test_config_global clone.verifySignatures true &&
+ git clone untrusted dst >out &&
+ test_i18ngrep "Commit [0-9a-f]\+ has a good GPG signature by " out
+'
+
+test_expect_success GPG 'clone untrusted with clone.verifySignatures=true and gpg.minTrustLevel=fully' '
+ test_config_global clone.verifySignatures true &&
+ test_config_global gpg.minTrustLevel fully &&
+ test_must_fail git clone untrusted dst 2>out &&
+ test_path_is_missing dst &&
+ test_i18ngrep "Commit [0-9a-f]\+ has an untrusted GPG signature" out
+'
+
+test_expect_success GPG 'clone unsigned tip with clone.verifySignatures=true' '
+ test_config_global clone.verifySignatures true &&
+ test_must_fail git clone unsigned-tip dst 2>out &&
+ test_path_is_missing dst &&
+ test_i18ngrep "Commit [0-9a-f]\+ does not have a GPG signature." out
+'
+
+test_expect_success GPG 'clone unsigned tip tag with clone.verifySignatures=true' '
+ test_config_global clone.verifySignatures true &&
+ test_must_fail git clone -b v3-unsigned-tip unsigned-tip dst 2>out &&
+ test_path_is_missing dst &&
+ test_i18ngrep "Tag [0-9a-f]\+ does not have a GPG signature." out
+'
+
+test_expect_success GPG 'clone signed tag from unsigned tip tag with clone.verifySignatures=true' '
+ test_when_finished "rm -rf dst" &&
+ test_config_global clone.verifySignatures true &&
+ git clone -b v1-signed unsigned-tip dst >out &&
+ test_i18ngrep "Tag [0-9a-f]\+ has a good GPG signature by " out
+'
+
+test_expect_success GPG 'clone signed branch from unsigned tip tag with clone.verifySignatures=true' '
+ test_when_finished "rm -rf dst" &&
+ test_config_global clone.verifySignatures true &&
+ git clone -b signed-branch unsigned-tip dst >out &&
+ test_i18ngrep "Commit [0-9a-f]\+ has a good GPG signature by " out
+'
+
+test_expect_success GPG 'clone unsigned branch with clone.verifySignatures=true' '
+ test_config_global clone.verifySignatures true &&
+ test_must_fail git clone -b unsigned-branch unsigned-branch dst 2>out &&
+ test_path_is_missing dst &&
+ test_i18ngrep "Commit [0-9a-f]\+ does not have a GPG signature." out
+'
+
+test_expect_success GPG 'clone unsigned shallow tag with clone.verifySignatures=true' '
+ test_config_global clone.verifySignatures true &&
+ test_must_fail git clone -b v2-unsigned-shallow unsigned dst 2>out &&
+ test_path_is_missing dst &&
+ test_i18ngrep "Commit [0-9a-f]\+ does not have a GPG signature." out
+'
+
+test_expect_success GPG 'clone unsigned annotated tag with clone.verifySignatures=true' '
+ test_config_global clone.verifySignatures true &&
+ test_must_fail git clone -b v2-unsigned-annotated unsigned dst 2>out &&
+ test_path_is_missing dst &&
+ test_i18ngrep "Tag [0-9a-f]\+ does not have a GPG signature." out
+'
+
+test_expect_success GPG 'clone signed tag for unsigned commit with clone.verifySignatures=true' '
+ test_when_finished "rm -rf dst" &&
+ test_config_global clone.verifySignatures true &&
+ git clone -b v4-signed-tag-unsigned-commit signed-tag-unsigned-commit dst >out &&
+ test_i18ngrep "Tag [0-9a-f]\+ has a good GPG signature by " out
+'
+
+test_expect_success GPG 'clone unsigned detached HEAD with clone.verifySignatures=true' '
+ test_config_global clone.verifySignatures true &&
+ test_must_fail git clone unsigned-detached dst 2>out &&
+ test_path_is_missing dst &&
+ test_i18ngrep "Commit [0-9a-f]\+ does not have a GPG signature." out
+'
+
+test_expect_success GPG 'clone signed detached HEAD with clone.verifySignatures=true' '
+ test_when_finished "rm -rf dst" &&
+ test_config_global clone.verifySignatures true &&
+ git clone signed-detached dst >out &&
+ test_i18ngrep "Commit [0-9a-f]\+ has a good GPG signature by " out
+'
+
+test_expect_success GPG 'clone signed with unsigned submodules and clone.verifySignatures=true' '
+ test_when_finished "rm -rf dst" &&
+ test_config_global clone.verifySignatures true &&
+ git clone --recurse-submodules signed-with-unsigned-submodule dst >out &&
+ test_i18ngrep "Commit [0-9a-f]\+ has a good GPG signature by " out
+'
+
+test_expect_success GPG 'clone signed with unsigned submodules and --verify-signatures' '
+ test_when_finished "rm -rf dst" &&
+ git clone --recurse-submodules --verify-signatures \
+ signed-with-unsigned-submodule dst >out &&
+ test_i18ngrep "Commit [0-9a-f]\+ has a good GPG signature by " out
+'
+
+test_expect_success GPG 'clone signed with signed submodules and clone.verifySignatures=true' '
+ test_when_finished "rm -rf dst" &&
+ test_config_global clone.verifySignatures true &&
+ git clone --recurse-submodules signed-with-signed-submodule dst >out &&
+ test_i18ngrep "Commit [0-9a-f]\+ has a good GPG signature by " out
+'
+
+test_expect_success GPG 'clone unsigned with signed submodules and clone.verifySignatures=true' '
+ test_config_global clone.verifySignatures true &&
+ test_must_fail git clone --recurse-submodules unsigned-with-signed-submodule dst 2>out &&
+ test_path_is_missing dst &&
+ test_i18ngrep "Commit [0-9a-f]\+ does not have a GPG signature." out
+'
+
+test_expect_success GPG 'clone unsigned with signed submodules and --verify-signatures' '
+ test_must_fail git clone --recurse-submodules --verify-signatures \
+ unsigned-with-signed-submodule dst 2>out &&
+ test_path_is_missing dst &&
+ test_i18ngrep "Commit [0-9a-f]\+ does not have a GPG signature." out
+'
+
+test_expect_success GPG 'clone unsigned with unsigned submodules and clone.verifySignatures=true' '
+ test_config_global clone.verifySignatures true &&
+ test_must_fail git clone --recurse-submodules unsigned-with-unsigned-submodule dst 2>out &&
+ test_path_is_missing dst &&
+ test_i18ngrep "Commit [0-9a-f]\+ does not have a GPG signature." out
+'
+
+test_done
diff --git a/tag.c b/tag.c
index f6ad4171f9..deff55198a 100644
--- a/tag.c
+++ b/tag.c
@@ -13,17 +13,19 @@ const char *tag_type = "tag";
static int run_gpg_verify(const struct object_id *oid, const char *buf,
unsigned long size, unsigned flags)
{
- struct signature_check sigc;
+ struct signature_check sigc = { .result = 'N' };
size_t payload_size;
- int ret;
-
- memset(&sigc, 0, sizeof(sigc));
+ int ret = 1;
payload_size = parse_signature(buf, size);
if (size == payload_size) {
if (flags & GPG_VERIFY_VERBOSE)
write_in_full(1, buf, payload_size);
+
+ /* maybe print a detailed error message */
+ print_signature_buffer(oid, &sigc, ret, flags);
+
return error("no signature found");
}
--
2.25.0.rc1.302.gc71d20beed
^ permalink raw reply related
* [PATCH 3/5] commit: refactor signature verification helpers
From: Hans Jerry Illikainen @ 2020-01-05 13:56 UTC (permalink / raw)
To: git; +Cc: Hans Jerry Illikainen
In-Reply-To: <20200105135616.19102-1-hji@dyntopia.com>
Previously, signature verification for commits had a number of helper
functions with slightly different behaviors:
- builtin/verify-commit.c had a file-local helper, so it wasn't reusable
outside of the verify-commit builtin.
- commit.c had verify_merge_signature() that die()d on errors, making it
difficult to reuse in parts of Git that mustn't (immediately) die on
failure.
- commit.c also had check_commit_signature(). It's flexible enough to
be used anywhere, but it isn't as nice as gpg_verify_tag(). More
specifically, it doesn't take care of printing the result; nor does it
accept and parse arbitrary object IDs.
This commit changes check_commit_signature() to file-local scope. It
also introduces a gpg_verify_commit() function modelled after
gpg_verify_tag(). It is written with the intent of removing the need to
implement local helpers for operations that verifies commit signatures.
This should hopefully make the code paths to the GPG interface easier to
follow.
Signed-off-by: Hans Jerry Illikainen <hji@dyntopia.com>
---
builtin/merge.c | 16 ++++++++------
builtin/pull.c | 18 +++++-----------
builtin/verify-commit.c | 25 +---------------------
commit.c | 47 ++++++++++++++++++++++++++++++-----------
commit.h | 31 ++++++---------------------
gpg-interface.h | 1 +
pretty.c | 3 ++-
7 files changed, 60 insertions(+), 81 deletions(-)
diff --git a/builtin/merge.c b/builtin/merge.c
index d127d2225f..e472f17738 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -62,8 +62,8 @@ 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 unsigned gpg_flags = GPG_VERIFY_SHORT | GPG_VERIFY_COMPAT;
static struct strbuf merge_msg = STRBUF_INIT;
static struct strategy **use_strategies;
static size_t use_strategies_nr, use_strategies_alloc;
@@ -633,7 +633,7 @@ static int git_merge_config(const char *k, const char *v, void *cb)
sign_commit = git_config_bool(k, v) ? "" : NULL;
return 0;
} else if (!strcmp(k, "gpg.mintrustlevel")) {
- check_trust_level = 0;
+ gpg_flags ^= GPG_VERIFY_COMPAT;
}
status = fmt_merge_msg_config(k, v, cb);
@@ -1399,9 +1399,10 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
if (remoteheads->next)
die(_("Can merge only exactly one commit into empty head"));
- if (verify_signatures)
- verify_merge_signature(remoteheads->item, verbosity,
- check_trust_level);
+ if (verify_signatures &&
+ gpg_verify_commit(&remoteheads->item->object.oid, NULL,
+ NULL, gpg_flags))
+ die(_("Signature verification failed"));
remote_head_oid = &remoteheads->item->object.oid;
read_empty(remote_head_oid, 0);
@@ -1424,8 +1425,9 @@ 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,
- check_trust_level);
+ if (gpg_verify_commit(&p->item->object.oid, NULL, NULL,
+ gpg_flags))
+ die(_("Signature verification failed"));
}
}
diff --git a/builtin/pull.c b/builtin/pull.c
index d4e3e77c8e..e41c4032ae 100644
--- a/builtin/pull.c
+++ b/builtin/pull.c
@@ -107,11 +107,11 @@ 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;
static int opt_allow_unrelated_histories;
+static unsigned gpg_flags = GPG_VERIFY_SHORT | GPG_VERIFY_COMPAT;
/* Options passed to git-fetch */
static char *opt_all;
@@ -366,7 +366,7 @@ static int git_pull_config(const char *var, const char *value, void *cb)
RECURSE_SUBMODULES_ON : RECURSE_SUBMODULES_OFF;
return 0;
} else if (!strcmp(var, "gpg.mintrustlevel")) {
- check_trust_level = 0;
+ gpg_flags ^= GPG_VERIFY_COMPAT;
}
status = git_gpg_config(var, value, cb);
@@ -589,17 +589,9 @@ static int run_fetch(const char *repo, const char **refspecs)
static int pull_into_void(const struct object_id *merge_head,
const struct object_id *curr_head)
{
- if (opt_verify_signatures) {
- struct commit *commit;
-
- commit = lookup_commit(the_repository, merge_head);
- if (!commit)
- die(_("unable to access commit %s"),
- oid_to_hex(merge_head));
-
- verify_merge_signature(commit, opt_verbosity,
- check_trust_level);
- }
+ if (opt_verify_signatures)
+ if (gpg_verify_commit(merge_head, NULL, NULL, gpg_flags))
+ return 1;
/*
* Two-way merge: we treat the index as based on an empty tree,
diff --git a/builtin/verify-commit.c b/builtin/verify-commit.c
index acc01a7be9..3f392654dd 100644
--- a/builtin/verify-commit.c
+++ b/builtin/verify-commit.c
@@ -20,37 +20,14 @@ static const char * const verify_commit_usage[] = {
NULL
};
-static int run_gpg_verify(struct commit *commit, unsigned flags)
-{
- struct signature_check signature_check;
- int ret;
-
- memset(&signature_check, 0, sizeof(signature_check));
-
- ret = check_commit_signature(commit, &signature_check);
- print_signature_buffer(&commit->object.oid, &signature_check, ret,
- flags);
-
- signature_check_clear(&signature_check);
- return ret;
-}
-
static int verify_commit(const char *name, unsigned flags)
{
struct object_id oid;
- struct object *obj;
if (get_oid(name, &oid))
return error("commit '%s' not found.", name);
- obj = parse_object(the_repository, &oid);
- if (!obj)
- return error("%s: unable to read file.", name);
- if (obj->type != OBJ_COMMIT)
- return error("%s: cannot verify a non-commit object of type %s.",
- name, type_name(obj->type));
-
- return run_gpg_verify((struct commit *)obj, flags);
+ return gpg_verify_commit(&oid, NULL, NULL, flags);
}
static int git_verify_commit_config(const char *var, const char *value, void *cb)
diff --git a/commit.c b/commit.c
index 519599469b..f8339916a6 100644
--- a/commit.c
+++ b/commit.c
@@ -1116,7 +1116,8 @@ static void handle_signed_tag(struct commit *parent, struct commit_extra_header
free(buf);
}
-int check_commit_signature(const struct commit *commit, struct signature_check *sigc)
+static int check_commit_signature(const struct commit *commit,
+ struct signature_check *sigc)
{
struct strbuf payload = STRBUF_INIT;
struct strbuf signature = STRBUF_INIT;
@@ -1136,21 +1137,43 @@ int check_commit_signature(const struct commit *commit, struct signature_check *
return ret;
}
-void verify_merge_signature(struct commit *commit, int verbosity,
- int check_trust)
+int gpg_verify_commit(const struct object_id *oid, const char *name_to_report,
+ struct signature_check *sigc, unsigned flags)
{
- struct signature_check signature_check;
+ struct object *obj;
+ struct signature_check tmp = { 0 };
int ret;
- memset(&signature_check, 0, sizeof(signature_check));
- ret = check_commit_signature(commit, &signature_check);
- ret |= check_trust && signature_check.trust_level < TRUST_MARGINAL;
- print_signature_buffer(&commit->object.oid, &signature_check, ret,
- GPG_VERIFY_SHORT);
- if (ret)
- die(_("Signature verification failed"));
+ if (!sigc)
+ sigc = &tmp;
- signature_check_clear(&signature_check);
+ obj = parse_object(the_repository, oid);
+ if (!obj)
+ return error("%s: unable to read file.",
+ name_to_report ?
+ name_to_report :
+ find_unique_abbrev(oid, DEFAULT_ABBREV));
+
+ if (obj->type != OBJ_COMMIT)
+ return error("%s: cannot verify a non-commit object of type %s.",
+ name_to_report ?
+ name_to_report :
+ find_unique_abbrev(oid, DEFAULT_ABBREV),
+ type_name(obj->type));
+
+ ret = check_commit_signature((struct commit *)obj, sigc);
+ /*
+ * Merge operations has historically required a trust level of
+ * 'marginal' or higher as a default. Backward-compatibility is
+ * maintained here -- however, new users of this function should
+ * delegate trust level verification to the GPG interface.
+ */
+ ret |= flags & GPG_VERIFY_COMPAT && sigc->trust_level < TRUST_MARGINAL;
+
+ print_signature_buffer(oid, sigc, ret, flags);
+ signature_check_clear(&tmp);
+
+ return ret;
}
void append_merge_tag_headers(struct commit_list *parents,
diff --git a/commit.h b/commit.h
index 008a0fa4a0..a31aaa7304 100644
--- a/commit.h
+++ b/commit.h
@@ -364,13 +364,14 @@ int parse_signed_commit(const struct commit *commit,
int remove_signature(struct strbuf *buf);
/*
- * Check the signature of the given commit. The result of the check is stored
- * in sig->check_result, 'G' for a good signature, 'U' for a good signature
- * from an untrusted signer, 'B' for a bad signature and 'N' for no signature
- * at all. This may allocate memory for sig->gpg_output, sig->gpg_status,
- * sig->signer and sig->key.
+ * Check the signature of the given commit. The result is stored in sigc if it
+ * is non-NULL. However, note that a return code of 0 from this function
+ * should be used to determine if the signature verified successfully, because
+ * multiple members in the signature_check structure are needed to sufficiently
+ * determine the outcome.
*/
-int check_commit_signature(const struct commit *commit, struct signature_check *sigc);
+int gpg_verify_commit(const struct object_id *oid, const char *name_to_report,
+ struct signature_check *sigc, unsigned flags);
/* record author-date for each commit object */
struct author_date_slab;
@@ -378,24 +379,6 @@ void record_author_date(struct author_date_slab *author_date,
struct commit *commit);
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,
- 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.h b/gpg-interface.h
index 2f349f8ed5..3f619cce0e 100644
--- a/gpg-interface.h
+++ b/gpg-interface.h
@@ -7,6 +7,7 @@ struct strbuf;
#define GPG_VERIFY_RAW (1 << 1)
#define GPG_VERIFY_FULL (1 << 2)
#define GPG_VERIFY_SHORT (1 << 3)
+#define GPG_VERIFY_COMPAT (1 << 4)
enum signature_trust_level {
TRUST_UNDEFINED,
diff --git a/pretty.c b/pretty.c
index f5fbbc5ffb..c19620ffe6 100644
--- a/pretty.c
+++ b/pretty.c
@@ -1302,7 +1302,8 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */
if (placeholder[0] == 'G') {
if (!c->signature_check.result)
- check_commit_signature(c->commit, &(c->signature_check));
+ gpg_verify_commit(&c->commit->object.oid, NULL,
+ &(c->signature_check), 0);
switch (placeholder[1]) {
case 'G':
if (c->signature_check.gpg_output)
--
2.25.0.rc1.302.gc71d20beed
^ permalink raw reply related
* [PATCH 2/5] gpg-interface: support one-line summaries in print_signature_buffer()
From: Hans Jerry Illikainen @ 2020-01-05 13:56 UTC (permalink / raw)
To: git; +Cc: Hans Jerry Illikainen
In-Reply-To: <20200105135616.19102-1-hji@dyntopia.com>
Most consumers of the GPG interface use print_signature_buffer() to show
the result from signature verifications. One notable exception was
verify_merge_signature().
Previously, the verify_merge_signature() function processed the
signature_check structure in order to print a one-line summary of the
result.
I think that the one-line summary format has potential use in other
parts of Git; and not only for merges. I also think that it makes sense
to have /one/ summary format in order to avoid confusing users with
different output styles for different operations.
This patch moves the one-line summary from verify_merge_signature() to
print_signature_buffer(). It also introduces a GPG_VERIFY_SHORT flag
that determines whether the summary should be printed.
Signed-off-by: Hans Jerry Illikainen <hji@dyntopia.com>
---
builtin/verify-commit.c | 3 ++-
commit.c | 23 +++++------------------
gpg-interface.c | 41 ++++++++++++++++++++++++++++++++++++++++-
gpg-interface.h | 4 +++-
tag.c | 7 ++++---
5 files changed, 54 insertions(+), 24 deletions(-)
diff --git a/builtin/verify-commit.c b/builtin/verify-commit.c
index 2a099ec6ba..acc01a7be9 100644
--- a/builtin/verify-commit.c
+++ b/builtin/verify-commit.c
@@ -28,7 +28,8 @@ static int run_gpg_verify(struct commit *commit, unsigned flags)
memset(&signature_check, 0, sizeof(signature_check));
ret = check_commit_signature(commit, &signature_check);
- print_signature_buffer(&signature_check, flags);
+ print_signature_buffer(&commit->object.oid, &signature_check, ret,
+ flags);
signature_check_clear(&signature_check);
return ret;
diff --git a/commit.c b/commit.c
index 3f91d3efc5..519599469b 100644
--- a/commit.c
+++ b/commit.c
@@ -1139,29 +1139,16 @@ int check_commit_signature(const struct commit *commit, struct signature_check *
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));
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 'B':
- die(_("Commit %s has a bad GPG signature "
- "allegedly by %s."), hex, signature_check.signer);
- default: /* 'N' */
- die(_("Commit %s does not have a GPG signature."), hex);
- }
- if (verbosity >= 0 && signature_check.result == 'G')
- printf(_("Commit %s has a good GPG signature by %s\n"),
- hex, signature_check.signer);
+ ret |= check_trust && signature_check.trust_level < TRUST_MARGINAL;
+ print_signature_buffer(&commit->object.oid, &signature_check, ret,
+ GPG_VERIFY_SHORT);
+ if (ret)
+ die(_("Signature verification failed"));
signature_check_clear(&signature_check);
}
diff --git a/gpg-interface.c b/gpg-interface.c
index fc182d39be..838ece2b37 100644
--- a/gpg-interface.c
+++ b/gpg-interface.c
@@ -5,6 +5,8 @@
#include "gpg-interface.h"
#include "sigchain.h"
#include "tempfile.h"
+#include "object.h"
+#include "object-store.h"
static char *configured_signing_key;
static enum signature_trust_level configured_min_trust_level = TRUST_UNDEFINED;
@@ -333,16 +335,53 @@ int check_signature(const char *payload, size_t plen, const char *signature,
return !!status;
}
-void print_signature_buffer(const struct signature_check *sigc, unsigned flags)
+void print_signature_buffer(const struct object_id *oid,
+ const struct signature_check *sigc, int status,
+ unsigned flags)
{
const char *output = flags & GPG_VERIFY_RAW ?
sigc->gpg_status : sigc->gpg_output;
+ char hex[GIT_MAX_HEXSZ + 1];
+ char *type;
if (flags & GPG_VERIFY_VERBOSE && sigc->payload)
fputs(sigc->payload, stdout);
if (flags & GPG_VERIFY_FULL && output)
fputs(output, stderr);
+
+ if (flags & GPG_VERIFY_SHORT && oid) {
+ type = xstrdup_or_null(
+ type_name(oid_object_info(the_repository, oid, NULL)));
+ if (!type)
+ type = xstrdup("object");
+ *type = toupper(*type);
+
+ find_unique_abbrev_r(hex, oid, DEFAULT_ABBREV);
+
+ switch (sigc->result) {
+ case 'G':
+ if (status)
+ error(_("%s %s has an untrusted GPG signature, "
+ "allegedly by %s."),
+ type, hex, sigc->signer);
+ else
+ printf(_("%s %s has a good GPG signature by %s\n"),
+ type, hex, sigc->signer);
+ break;
+ case 'N':
+ error(_("%s %s does not have a GPG signature."), type,
+ hex);
+ break;
+ default:
+ error(_("%s %s has a bad GPG signature "
+ "allegedly by %s."),
+ type, hex, sigc->signer);
+ break;
+ }
+
+ free(type);
+ }
}
size_t parse_signature(const char *buf, size_t size)
diff --git a/gpg-interface.h b/gpg-interface.h
index 4631a91330..2f349f8ed5 100644
--- a/gpg-interface.h
+++ b/gpg-interface.h
@@ -6,6 +6,7 @@ struct strbuf;
#define GPG_VERIFY_VERBOSE (1 << 0)
#define GPG_VERIFY_RAW (1 << 1)
#define GPG_VERIFY_FULL (1 << 2)
+#define GPG_VERIFY_SHORT (1 << 3)
enum signature_trust_level {
TRUST_UNDEFINED,
@@ -60,7 +61,8 @@ const char *get_signing_key(void);
int check_signature(const char *payload, size_t plen,
const char *signature, size_t slen,
struct signature_check *sigc);
-void print_signature_buffer(const struct signature_check *sigc,
+void print_signature_buffer(const struct object_id *oid,
+ const struct signature_check *sigc, int status,
unsigned flags);
#endif
diff --git a/tag.c b/tag.c
index b8d6da81eb..f6ad4171f9 100644
--- a/tag.c
+++ b/tag.c
@@ -10,7 +10,8 @@
const char *tag_type = "tag";
-static int run_gpg_verify(const char *buf, unsigned long size, unsigned flags)
+static int run_gpg_verify(const struct object_id *oid, const char *buf,
+ unsigned long size, unsigned flags)
{
struct signature_check sigc;
size_t payload_size;
@@ -28,7 +29,7 @@ static int run_gpg_verify(const char *buf, unsigned long size, unsigned flags)
ret = check_signature(buf, payload_size, buf + payload_size,
size - payload_size, &sigc);
- print_signature_buffer(&sigc, flags);
+ print_signature_buffer(oid, &sigc, ret, flags);
signature_check_clear(&sigc);
return ret;
@@ -57,7 +58,7 @@ int gpg_verify_tag(const struct object_id *oid, const char *name_to_report,
name_to_report :
find_unique_abbrev(oid, DEFAULT_ABBREV));
- ret = run_gpg_verify(buf, size, flags);
+ ret = run_gpg_verify(oid, buf, size, flags);
free(buf);
return ret;
--
2.25.0.rc1.302.gc71d20beed
^ permalink raw reply related
* [PATCH 4/5] merge: verify signatures if gpg.verifySignatures is true
From: Hans Jerry Illikainen @ 2020-01-05 13:56 UTC (permalink / raw)
To: git; +Cc: Hans Jerry Illikainen
In-Reply-To: <20200105135616.19102-1-hji@dyntopia.com>
Merge operations has had support for a merge.verifySignatures config
knob for quite some time. However, there is no global option to enable
signature verification for all operations that support it. This makes
sense because only merges (and, by extent, pulls) has support for
configurable signature verifications.
However, with the upcoming introduction of signature verification for
clones, it seems reasonable to have a global option that enables
verification for all operations that support it. Otherwise, users would
have to track down and enable every *.verifySignatures knob.
This patch adds support for a global gpg.verifySignatures in
git_merge_config(). The global variant is overridden by both
merge.verifySignatures and the --(no-)verify-signatures command-line
parameter.
Signed-off-by: Hans Jerry Illikainen <hji@dyntopia.com>
---
Documentation/config/gpg.txt | 6 ++++++
Documentation/config/merge.txt | 4 +++-
builtin/merge.c | 8 +++++---
t/t7612-merge-verify-signatures.sh | 27 +++++++++++++++++++++++++++
4 files changed, 41 insertions(+), 4 deletions(-)
diff --git a/Documentation/config/gpg.txt b/Documentation/config/gpg.txt
index d94025cb36..7bf64cff24 100644
--- a/Documentation/config/gpg.txt
+++ b/Documentation/config/gpg.txt
@@ -33,3 +33,9 @@ gpg.minTrustLevel::
* `marginal`
* `fully`
* `ultimate`
+
+gpg.verifySignatures::
+ Verify that commits are signed with a valid key for operations
+ that support signature verification. This option act as a
+ global default and can be overridden in sections specific to
+ individual operations.
diff --git a/Documentation/config/merge.txt b/Documentation/config/merge.txt
index 6a313937f8..7ff72fafc2 100644
--- a/Documentation/config/merge.txt
+++ b/Documentation/config/merge.txt
@@ -28,7 +28,9 @@ merge.ff::
merge.verifySignatures::
If true, this is equivalent to the --verify-signatures command
- line option. See linkgit:git-merge[1] for details.
+ line option. See linkgit:git-merge[1] for details. Also see
+ `gpg.verifySignatures` for a global option to enable signature
+ verification.
include::fmt-merge-msg.txt[]
diff --git a/builtin/merge.c b/builtin/merge.c
index e472f17738..539dd1dbfc 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -61,7 +61,7 @@ static const char * const builtin_merge_usage[] = {
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 allow_trivial = 1, have_message, verify_signatures = -1;
static int overwrite_ignore = 1;
static unsigned gpg_flags = GPG_VERIFY_SHORT | GPG_VERIFY_COMPAT;
static struct strbuf merge_msg = STRBUF_INIT;
@@ -610,6 +610,8 @@ static int git_merge_config(const char *k, const char *v, void *cb)
show_diffstat = git_config_bool(k, v);
else if (!strcmp(k, "merge.verifysignatures"))
verify_signatures = git_config_bool(k, v);
+ else if (!strcmp(k, "gpg.verifysignatures") && verify_signatures < 0)
+ verify_signatures = git_config_bool(k, v);
else if (!strcmp(k, "pull.twohead"))
return git_config_string(&pull_twohead, k, v);
else if (!strcmp(k, "pull.octopus"))
@@ -1399,7 +1401,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
if (remoteheads->next)
die(_("Can merge only exactly one commit into empty head"));
- if (verify_signatures &&
+ if (verify_signatures == 1 &&
gpg_verify_commit(&remoteheads->item->object.oid, NULL,
NULL, gpg_flags))
die(_("Signature verification failed"));
@@ -1423,7 +1425,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
usage_with_options(builtin_merge_usage,
builtin_merge_options);
- if (verify_signatures) {
+ if (verify_signatures == 1) {
for (p = remoteheads; p; p = p->next) {
if (gpg_verify_commit(&p->item->object.oid, NULL, NULL,
gpg_flags))
diff --git a/t/t7612-merge-verify-signatures.sh b/t/t7612-merge-verify-signatures.sh
index a426f3a89a..10ab2fa119 100755
--- a/t/t7612-merge-verify-signatures.sh
+++ b/t/t7612-merge-verify-signatures.sh
@@ -125,6 +125,33 @@ test_expect_success GPG 'merge commit with bad signature with merge.verifySignat
git merge --no-verify-signatures $(cat forged.commit)
'
+test_expect_success GPG 'merge commit with bad signature with gpg.verifySignatures=true and --no-verify-signatures' '
+ test_when_finished "git reset --hard && git checkout initial" &&
+ test_config gpg.verifySignatures true &&
+ git merge --no-verify-signatures $(cat forged.commit)
+'
+
+test_expect_success GPG 'merge commit with bad signature with gpg.verifySignatures=true and merge.verifySignatures=false' '
+ test_when_finished "git reset --hard && git checkout initial" &&
+ test_config gpg.verifySignatures true &&
+ test_config merge.verifySignatures false &&
+ git merge $(cat forged.commit)
+'
+
+test_expect_success GPG 'merge commit with bad signature with clone.verifySignatures=false and gpg.verifySignatures=true' '
+ test_when_finished "git reset --hard && git checkout initial" &&
+ test_config merge.verifySignatures false &&
+ test_config gpg.verifySignatures true &&
+ git merge $(cat forged.commit)
+'
+
+test_expect_success GPG 'merge commit with bad signature with gpg.verifySignatures=true' '
+ test_when_finished "git reset --hard && git checkout initial" &&
+ test_config gpg.verifySignatures true &&
+ test_must_fail git merge $(cat forged.commit) 2>mergeerror &&
+ test_i18ngrep "has a bad GPG signature allegedly by" mergeerror
+'
+
test_expect_success GPG 'merge unsigned commit into unborn branch' '
test_when_finished "git checkout initial" &&
git checkout --orphan unborn &&
--
2.25.0.rc1.302.gc71d20beed
^ permalink raw reply related
* [PATCH 1/5] gpg-interface: conditionally show the result in print_signature_buffer()
From: Hans Jerry Illikainen @ 2020-01-05 13:56 UTC (permalink / raw)
To: git; +Cc: Hans Jerry Illikainen
In-Reply-To: <20200105135616.19102-1-hji@dyntopia.com>
The print_signature_buffer() function in gpg-interface.c is used to
print the result of a GPG verified payload. It takes a 'flags'
parameter that determines what to print.
Previously, the 'flags' parameter processed 2 flags:
- GPG_VERIFY_RAW: to print the raw output from GPG instead of the
human(ish)-readable output. One of these outputs were always
shown, irregardless of any other flags.
- GPG_VERIFY_VERBOSE: to print the payload that was verified
Interestingly, there was also a third flag defined in gpg-interface.h;
GPG_VERIFY_OMIT_STATUS. That flag wasn't used by the print function
itself -- instead, callers would check for the presence of
GPG_VERIFY_OMIT_STATUS before invoking print_signature_buffer().
It seems reasonable that the GPG interface should handle all flags
related to how the result should be (or shouldn't be) shown. This patch
implements that behavior by removing GPG_VERIFY_OMIT_STATUS and adding
GPG_VERIFY_FULL. If neither GPG_VERIFY_FULL nor GPG_VERIFY_VERBOSE is
present, then nothing is printed. This allows callers to invoke
print_signature_buffer() unconditionally.
Signed-off-by: Hans Jerry Illikainen <hji@dyntopia.com>
---
builtin/tag.c | 4 ++--
builtin/verify-commit.c | 2 +-
builtin/verify-tag.c | 4 ++--
gpg-interface.c | 2 +-
gpg-interface.h | 6 +++---
tag.c | 4 +---
6 files changed, 10 insertions(+), 12 deletions(-)
diff --git a/builtin/tag.c b/builtin/tag.c
index e0a4c25382..8489e220e8 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -112,10 +112,10 @@ static int verify_tag(const char *name, const char *ref,
{
int flags;
const struct ref_format *format = cb_data;
- flags = GPG_VERIFY_VERBOSE;
+ flags = GPG_VERIFY_FULL | GPG_VERIFY_VERBOSE;
if (format->format)
- flags = GPG_VERIFY_OMIT_STATUS;
+ flags = 0;
if (gpg_verify_tag(oid, name, flags))
return -1;
diff --git a/builtin/verify-commit.c b/builtin/verify-commit.c
index 40c69a0bed..2a099ec6ba 100644
--- a/builtin/verify-commit.c
+++ b/builtin/verify-commit.c
@@ -63,7 +63,7 @@ static int git_verify_commit_config(const char *var, const char *value, void *cb
int cmd_verify_commit(int argc, const char **argv, const char *prefix)
{
int i = 1, verbose = 0, had_error = 0;
- unsigned flags = 0;
+ unsigned flags = GPG_VERIFY_FULL;
const struct option verify_commit_options[] = {
OPT__VERBOSE(&verbose, N_("print commit contents")),
OPT_BIT(0, "raw", &flags, N_("print raw gpg status output"), GPG_VERIFY_RAW),
diff --git a/builtin/verify-tag.c b/builtin/verify-tag.c
index f45136a06b..bd5e99925b 100644
--- a/builtin/verify-tag.c
+++ b/builtin/verify-tag.c
@@ -30,7 +30,7 @@ static int git_verify_tag_config(const char *var, const char *value, void *cb)
int cmd_verify_tag(int argc, const char **argv, const char *prefix)
{
int i = 1, verbose = 0, had_error = 0;
- unsigned flags = 0;
+ unsigned flags = GPG_VERIFY_FULL;
struct ref_format format = REF_FORMAT_INIT;
const struct option verify_tag_options[] = {
OPT__VERBOSE(&verbose, N_("print tag contents")),
@@ -53,7 +53,7 @@ int cmd_verify_tag(int argc, const char **argv, const char *prefix)
if (verify_ref_format(&format))
usage_with_options(verify_tag_usage,
verify_tag_options);
- flags |= GPG_VERIFY_OMIT_STATUS;
+ flags = 0;
}
while (i < argc) {
diff --git a/gpg-interface.c b/gpg-interface.c
index 2d538bcd6e..fc182d39be 100644
--- a/gpg-interface.c
+++ b/gpg-interface.c
@@ -341,7 +341,7 @@ void print_signature_buffer(const struct signature_check *sigc, unsigned flags)
if (flags & GPG_VERIFY_VERBOSE && sigc->payload)
fputs(sigc->payload, stdout);
- if (output)
+ if (flags & GPG_VERIFY_FULL && output)
fputs(output, stderr);
}
diff --git a/gpg-interface.h b/gpg-interface.h
index f4e9b4f371..4631a91330 100644
--- a/gpg-interface.h
+++ b/gpg-interface.h
@@ -3,9 +3,9 @@
struct strbuf;
-#define GPG_VERIFY_VERBOSE 1
-#define GPG_VERIFY_RAW 2
-#define GPG_VERIFY_OMIT_STATUS 4
+#define GPG_VERIFY_VERBOSE (1 << 0)
+#define GPG_VERIFY_RAW (1 << 1)
+#define GPG_VERIFY_FULL (1 << 2)
enum signature_trust_level {
TRUST_UNDEFINED,
diff --git a/tag.c b/tag.c
index 71b544467e..b8d6da81eb 100644
--- a/tag.c
+++ b/tag.c
@@ -28,9 +28,7 @@ static int run_gpg_verify(const char *buf, unsigned long size, unsigned flags)
ret = check_signature(buf, payload_size, buf + payload_size,
size - payload_size, &sigc);
-
- if (!(flags & GPG_VERIFY_OMIT_STATUS))
- print_signature_buffer(&sigc, flags);
+ print_signature_buffer(&sigc, flags);
signature_check_clear(&sigc);
return ret;
--
2.25.0.rc1.302.gc71d20beed
^ permalink raw reply related
* [PATCH 0/5] refactor gpg-interface and add gpg verification for clones
From: Hans Jerry Illikainen @ 2020-01-05 13:56 UTC (permalink / raw)
To: git; +Cc: Hans Jerry Illikainen
This series starts off with refactor of print_signature_buffer() to make
all output conditional based on the 'flags' parameter. The print
function is also extended to optionally show one-line summaries of
signature verifications (previously that functionality existed in
verify_merge_signature()).
The helper functions for signature verification of commits are then
refactored. The new gpg_verify_commit() function is modelled after
gpg_verify_tag(). This allows us to remove verify_merge_signature() and
the file-local run_gpg_verify() (from the verify-commit builtin). It
also allows us to change check_commit_signature() into a local function
in commit.c.
A new configuration option is also introduced, gpg.verifySignatures.
This allows users to enable signature verification for all operations
that support it. Individual operations can then use
<operation>.verifySignatures for finer-grained control.
And finally, signature verification is added to the clone builtin. It
obeys --(no-)verify-signatures, clone.verifySignatures and
gpg.verifySignatures (in decreasing order of significance).
A notable quirk with signature verification for clones is
--recurse-submodules. As mentioned in the commit message, the current
workaround is to disable signature verification for submodules by
passing --no-verify-signatures in submodule--helper.c
I'm very much open to suggestions for a better approach of dealing with
recursive clones. However, I don't think --verify-signatures from the
clone builtin should propagate to submodules, because that would break a
workflow where a user:
1. trust the hash function
2. has audited an unsigned repository at a given point
3. has added the repository at that point as a submodule
4. has signed an object in the super repository where the audited
submodule is referenced
So, I think it'd make more sense to introduce a
submodule.verifySignatures config knob to be used by both
--recurse-submodules and when the 'submodule' command is used directly.
I hope this patch series isn't too confusing/all over the place. I
wasn't sure whether the preparatory patches would have made sense in
isolation, so I opted to send it all in one go.
Hans Jerry Illikainen (5):
gpg-interface: conditionally show the result in
print_signature_buffer()
gpg-interface: support one-line summaries in print_signature_buffer()
commit: refactor signature verification helpers
merge: verify signatures if gpg.verifySignatures is true
clone: support signature verification
Documentation/config.txt | 2 +
Documentation/config/clone.txt | 3 +
Documentation/config/gpg.txt | 6 +
Documentation/config/merge.txt | 4 +-
Documentation/git-clone.txt | 4 +
builtin/clone.c | 46 ++++
builtin/merge.c | 22 +-
builtin/pull.c | 18 +-
builtin/submodule--helper.c | 6 +
builtin/tag.c | 4 +-
builtin/verify-commit.c | 26 +-
builtin/verify-tag.c | 4 +-
commit.c | 58 ++--
commit.h | 31 +--
gpg-interface.c | 43 ++-
gpg-interface.h | 11 +-
pretty.c | 3 +-
t/t5619-clone-verify-signatures.sh | 411 +++++++++++++++++++++++++++++
t/t7612-merge-verify-signatures.sh | 27 ++
tag.c | 19 +-
20 files changed, 633 insertions(+), 115 deletions(-)
create mode 100644 Documentation/config/clone.txt
create mode 100755 t/t5619-clone-verify-signatures.sh
--
2.25.0.rc1.302.gc71d20beed
^ permalink raw reply
* Fwd: My Introduction and a Doubt
From: Nirmal Khedkar @ 2020-01-05 6:01 UTC (permalink / raw)
To: git
In-Reply-To: <CAFFaXszUdyfun1W0TuvJuRihLo7Mz-XYL13Fm5gCv_gWrir-_Q@mail.gmail.com>
Hey everybody!
I'd love to use this email to introduce myself to the community and
ask for help.
I'm Nirmal Khedkar, student from India. I love making end user
applications, and in this FOSS world, there simply aren't many as big
and impactful as Git. I'd love to contribute Git for GSoC 2020 as well
as for long-term.
Now the doubt:
I'm trying to wrap up the issue mentioned here
(https://github.com/gitgitgadget/git/issues/486) : basically allowing
git bisect to work from subdirectories. I do consider myself okay in
C but I'm still kind-of stuck here: hence emailing this on the mailing
list.
When you do "git bisect" in a subdirectory, an error "You need to run
this command from the toplevel of the working tree." is raised. The
error is raised in "git-sh-setup.sh" and the command begins execution
in "bisect--helper.c", but I cant understand how the error appears.
Additionally I'd love to know how the C files linkwith the numerous
shell scripts within Git.
I've searched all I could within the Git project, but if there's any
existing documentation on this, please let me know.
Thanks in advance!
Nirmal Khedkar
^ permalink raw reply
* Re: [PATCH 1/1] worktree: delete branches auto-created by 'worktree add'
From: Eric Sunshine @ 2020-01-05 5:32 UTC (permalink / raw)
To: Pratyush Yadav; +Cc: Git List
In-Reply-To: <CAPig+cRL5w7azdALeBKKisTZwjgU6QhBqJRzQqDENjYiaTT0oA@mail.gmail.com>
On Sat, Jan 4, 2020 at 4:47 PM Pratyush Yadav <me@yadavpratyush.com> wrote:
> On 27/12/19 06:05AM, Eric Sunshine wrote:
> > On Sat, Dec 14, 2019 at 11:16 AM Pratyush Yadav <me@yadavpratyush.com> wrote:
> > > Remove that branch when removing the worktree. To make sure no commits
> > > are lost, the branch won't be deleted if it has moved.
> >
> > My knee-jerk reaction upon reading the first sentence of this
> > paragraph was that this is a significant and undesirable behavior
> > change, however, the second sentence helps to allay my fears about it.
> > It's possible, I suppose, that there is some existing tooling
> > somewhere which relies upon the current behavior, but it's hard to
> > imagine any good reason to do so.
>
> It is possible that some script somewhere does
>
> git worktree add foo
> do_something # doesn't move the branch
> git worktree remove foo
> git branch -d foo
>
> Branch deletion would fail here, which might be considered as an error
> by the script. Not sure how common that would be though.
Good point. That's a quite believable scenario for a scripting case.
Even if the script itself doesn't check for an error from git-branch,
people could get annoyed if their scripts suddenly start complaining
"error: branch 'foo' not found". So, that's a genuine concern.
> > However, there is a rather serious flaw in the implementation. My
> > expectation is that it should only automatically delete a branch if
> > the branch creation was inferred; it should never automatically delete
> > a branch which was created explicitly. You kind of have this covered
> > (and even have a test for it), but it doesn't work correctly when the
> > user explicitly requests branch creation via -b/-B and the branch name
> > matches the worktree name. For instance:
> >
> > git worktree add -b foo foo
> > git worktree remove foo
> >
> > incorrectly automatically removes branch "foo" even though the user
> > requested its creation explicitly.
>
> Thanks for pointing it out. Will fix.
Note that this almost certainly deserves an extra test in t2403 since
the tests added by v1 didn't catch this problem.
> > Subjectively, it seems more natural to fully discuss automatic branch
> > removal here rather than referring to the discussion of "worktree
> > add".
>
> I considered doing this but then left that part in 'add' because the
> conditions in which the branch is auto deleted are described pretty well
> in add's documentation. Will move it to 'remove'.
In retrospect, I don't feel strongly about it one way or the other. It
just surprised me to find it discussed under "add" on my first
read-through. And if you do patch "prune" to also auto-remove an
auto-created branch, then "add" might be the better place for the
discussion anyhow.
> > A bit of bikeshedding regarding the filename: "auto_created" is rather
> > unusual. Most names in the .git hierarchy are short and sweet. Also,
> > with the exception of ORIG_HEAD and FETCH_HEAD, all other multi-word
> > filenames seem to use hyphen rather than underscore, which suggests
> > "auto-created" would be a better choice. However, I'd probably drop
> > the hyphen altogether. Finally, "auto_created", alone, does not
> > necessarily convey that the branch was auto-created; someone could
> > misinterpret it as meaning the worktree itself was auto-created, so I
> > wonder if a better name can be found.
>
> Any suggestions? Does "implicitbranch"/"implicit-branch" sound any
> better? How about "branch-auto-created-at"? It is very clear but is a
> mouthful.
Taking into account the suggestion from my review that you likely also
will need to store in this file the name of the auto-created branch
(not just the original OID of that branch), then the nature of this
file changes a bit, which might help suggest a better name.
"implicitbranch" and "implicit-branch" are not bad, though a bit of a
mouthful. What about "autobranch"?
> > A bigger question, though, is whether we really want to see new files
> > like this springing up in the .git/worktrees/<id>/ directory for each
> > new piece of metadata which belongs to a worktree. I ask because this
> > isn't the first such case in which some additional worktree-specific
> > metadata was proposed (see, for instance, [1]). So, I'm wondering if
> > we should have a more generalized solution, such as introducing a new
> > file which can hold any sort of metadata which comes along in the
> > future. In particular, I'm thinking about a file containing an
> > extensible set of "key: value" tuples, in which case the "auto
> > created" metadata would be just one of possibly many keys.
>
> Do you worry that the number of metadata files might grow to be too
> large? I can't say how worktrees will grow in the future, but right now
> there are 4 metadata files ('commondir', 'gitdir', 'HEAD', 'ORIG_HEAD').
> So, not a lot.
I'm not particularly worried about the number of files. A couple
thoughts I had in mind are: (1) other tools or non-canonical Git
implementations (jgit, libgit, etc.) may be poking around inside the
.git/worktrees/<id>/ directory, and (2) the information represented by
this new file may deserve inclusion in the output of "git worktree
list --porcelain".
It was #1, in particular, I think, which got me thinking of having a
standardized format (i.e. extensible "key: value" list) for worktree
metainfo added in the future. It would require a one-time cost for
each tool/library to implement, and would then effectively be free as
more metainfo is added to the worktree. Compare that with having to
write a reader/parser for each new metainfo file added (not that these
files are terribly difficult to parse).
Similarly, a standardized format simplifies #2, extending "git
worktree list --porcelain" to output additional metainfo.
By the way, I wouldn't mind seeing "git worktree list --porcelain"
extended to output this new information, but I don't insist upon it as
a requirement of this patch; it can easily be done later if the need
arises. (In fact, the --porcelain documentation is so woefully lacking
and under-specified that it needs an overhaul, which I think deserves
a patch series of its own, thus is another reason I don't really
expect/want to see that change made by this patch.)
> I chose to add a new file because from what I have noticed, Git keeps a
> lot of metadata in files like this (HEAD, refs, etc). Do other
> subsystems use a key-value store? What problems did they face?
>
> I'd prefer to not take on this feature (since I expect it to be a lot of
> work), but if there are strong opinions on using a key-value store then
> I guess I'll bite the bullet.
I brought up the idea of a standardized extensible "key: value" store
because, having that older email thread in mind, it was the first
thought which popped into my mind when I saw this patch introducing a
new file in .git/worktrees/<id>/. However, one can make a good
argument that Git already has such a standard, and that that standard
is simply using individual files like "HEAD", "gitdir", "commondir",
etc. So, I think I'm pretty comfortable with the idea of storing this
information in a new file as this patch does. After all, there's
plenty of precedent in Git for doing it that way.
> > > + if (auto_create) {
> > > + fd = xopen(sb.buf, O_WRONLY | O_CREAT | O_TRUNC, 0666);
> > > + get_oid("HEAD", &oid);
> >
> > Unless I'm mistaken, this is just wrong. You're grabbing the OID of
> > HEAD from the worktree in which "worktree add" is being invoked,
> > however, if the new branch name is DWIM'd from an existing
> > tracking-branch, then the OID should be that of the tracking-branch,
> > not HEAD of the current worktree. So, you should be using the OID
> > already looked up earlier in the function, 'commit->object.oid', which
> > should be correct for either case.
>
> Oops! Thanks for pointing it out. Will fix.
This deserves a new test in t2403, as well (or perhaps two new tests
since there are a couple different ways the starting OID can be
DWIM'd, if I'm reading the code correctly).
> > By the way, this suffers from the problem that if "git worktree add
> > foo" fails for some reason, such as because path "foo" already exists,
> > then the new branch will _not_ be cleaned up automatically since that
> > failure will happen before "auto_created" is ever created (among other
> > reasons). But that's not a new issue; it's an existing flaw of
> > "worktree add" not cleaning up a branch it created before it discovers
> > that it can't actually create the target directory for some reason, so
> > I wouldn't expect you to fix that problem with this submission. (I'm
> > just mentioning it for completeness.)
>
> I'll see if I can come up with a fix for this as a follow-up patch.
If fixing this ends up being more involved than a relatively minor
change -- and I think it will be quite a bit more involved due to all
the die()ing going on inside add_worktree() -- then I could easily see
(and probably would prefer) it being a separate patch series (since
the changes made by the patch under discussion are already
sufficiently involved as to eat up a good deal of reviewer time).
> > > + git_path_buf(&sb, "worktrees/%s/auto_created", wt->id);
> > > + if (file_exists(sb.buf)) {
> > > + strbuf_read_file(&hex, sb.buf, 0);
> >
> > You can avoid an unnecessary race condition here by dropping the
> > file_exists() call altogether and just checking the return code of
> > strbuf_read_file() -- which you should probably be doing anyhow. If
> > strbuf_read_file() returns a non-negative value, then you know it
> > exists, so file_exists() is redundant.
>
> Will fix. Though I don't see how it would be a "race condition". Is
> file_exists() asynchronous in some way? Otherwise, how would a race
> happen and between what?
I think I was remembering an earlier[1] issue with someone running a
bunch of git-worktree commands in parallel and encountering races, but
that shouldn't apply here. Still, the code will be cleaner by dropping
file_exists() altogether.
[1]: https://lore.kernel.org/git/cover.1550508544.git.msuchanek@suse.de/T/
> > > + if (strcmp(hex.buf, oid_to_hex(&oid)) == 0)
> > > + delete_auto_created = 1;
> >
> > I was wondering if it would be more semantically correct to parse
> > 'hex' into an 'oid' and compare them with oidcmp() rather than doing a
> > string comparison of the hex values (though I'm not sure it will
> > matter in practice).
>
> Since I haven't spent too much time in the Git internals, the string
> representation feels more natural to me. And that's why I went this way
> subconsciously. While I don't mind either, I wonder if it would make a
> difference in practice. Anyway, if you have a preference for the other
> way round, I'll trust your gut feeling.
It may not matter in practice, but having considered it further, I
really would prefer it to be semantically correct by comparing the
OIDs directly rather than the string representations. With the process
underway of updating Git to be able to work with multiple hash
functions (and moving away from SHA-1), making use of get_oid_hex()
and oideq() to perform this comparison may make the job of auditing
the code for multi-hash-function friendliness easier.
> > However, I don't think it is correct to use 'wt->id' here as the
> > branch name since there is no guarantee that the <id> in
> > .git/worktrees/<id>/ matches the branch name with which the worktree
> > was created. For instance:
> >
> > git worktree add foo/bar existing-branch
> > git worktree add baz/bar
> >
> > will, due to name conflicts, create worktree metadata directories:
> >
> > .git/worktrees/bar
> > .git/worktrees/bar1
> >
> > where the first is associated with branch "existing-branch", and the
> > second is associated with new branch "bar". When you then invoke "git
> > worktree remove baz/bar", it will try removing a branch named "bar1",
> > not "bar" as intended. To fix this, I think you need to record the
> > original auto-created branch name in the "auto_created" metadata file
> > too, not just the OID.
>
> Interesting! Didn't think of a situation like this. Thanks for pointing
> it out. Will fix.
Definitely deserves a test in t2403.
> > > +test_expect_success 'remove auto-created branch' '
> > > + (
> > > + git worktree add to-remove &&
> > > + git worktree remove to-remove &&
> > > + git branch -l to-remove >branch_list &&
> > > + test_line_count = 0 branch_list
> > > + )
> > > +'
> >
> > I don't think there is any need for this test to be run in a subshell,
> > so you can drop the enclosing '(' and ')'.
>
> I was following the pattern in the two tests above. Will drop the
> parentheses.
The existing tests use a subshell (the parentheses) because they 'cd'
around, and use of a subshell ensures that subsequent tests won't
adversely run in the wrong directory if the test fails for some reason
(since the affect of the 'cd' does not last beyond the end of the
subshell). As long as you're not cd'ing around (or doing a few other
questionable things), there's no need for the subshell.
> > > +test_expect_success 'do not remove a branch that was not auto-created' '
> > > + (
> > > + git worktree add -b new_branch to-remove &&
> >
> > Nit: The inconsistent mix of underscore and hyphen in names is odd.
> > Perhaps settle on one or the other (with a slight preference toward
> > hyphen).
>
> I'll change 'new_branch' to 'new-branch'.
As mentioned above, also add a test in which the explicitly-created
new branch has the same name as the worktree itself (since that case
was implemented wrong in v1 but the tests didn't catch the failure).
> I'll send out the v2 as soon as I can. Thanks.
There's no rush. Let's get the details and any lingering questions
worked out before subjecting reviewers to a new version (especially,
as my review time is somewhat limited these days).
^ permalink raw reply
* Re: push --follow-tags not pushing tags ?
From: Matt Rogers @ 2020-01-05 4:24 UTC (permalink / raw)
To: Wes Hurd; +Cc: git
In-Reply-To: <CAOAGZwEfMQ6F8t3a+jVDNrPd6ZWFfiHDU9z1srSHp575DmqcVQ@mail.gmail.com>
I believe --follow-tags only pushes annotated tags. Does the issue
still persist when you use something like:
`git tag -a -m "I'm an annotation"`
^ permalink raw reply
* push --follow-tags not pushing tags ?
From: Wes Hurd @ 2020-01-05 4:04 UTC (permalink / raw)
To: git
Hi,
I'm experiencing an issue where git push --follow-tags is not actually
adding a new tag to the remote, as it's expected
https://git-scm.com/docs/git-push#Documentation/git-push.txt---follow-tags
I did the following sequence:
committed to master (new commit)
git tag v1.0.1
git push --follow-tags origin
The commit pushed to remote but the tag is not pushed.
plain `git push --follow-tags` has the same behaviour.
Output of git tag :
v1.0.0
v1.0.1
git 2.24.1 Ubuntu 18.04
Am I missing something for this to work ?
https://stackoverflow.com/a/3745250/4364036 also indicates it should
work based on the sequence I followed.
Thanks,
^ permalink raw reply
* What's cooking in git.git (Jan 2020, #02; Sat, 4)
From: Junio C Hamano @ 2020-01-05 1:22 UTC (permalink / raw)
To: git
Here are the topics that have been cooking. Commits prefixed with
'-' are only in 'pu' (proposed updates) while commits prefixed with
'+' are in 'next'. The ones marked with '.' do not appear in any of
the integration branches, but I am still holding onto them.
After the topics marked to be merged to 'master' go in, the tip of
the master will hopefully be very close to the final release.
You can find the changes described here in the integration branches
of the repositories listed at
http://git-blame.blogspot.com/p/git-public-repositories.html
--------------------------------------------------
[New Topics]
* do/gitweb-typofix-in-comments (2020-01-04) 1 commit
- gitweb: fix a couple spelling errors in comments
Typofix.
Will merge to 'next' and then to 'master'.
* en/unpack-trees-check-updates-simplify (2020-01-04) 1 commit
- unpack-trees: exit check_updates() early if updates are not wanted
Code simplification.
* jb/doc-multi-pack-idx-fix (2020-01-04) 1 commit
- multi-pack-index: correct configuration in documentation
Typofix.
Will merge to 'next' and then to 'master'.
* pm/am-in-body-header-doc-update (2020-01-04) 1 commit
- am: document that Date: can appear as an in-body header
Doc update.
Will merge to 'next' and then to 'master'.
--------------------------------------------------
[Stalled]
* ag/edit-todo-drop-check (2019-12-06) 2 commits
- rebase-interactive: warn if commit is dropped with `rebase --edit-todo'
- sequencer: move check_todo_list_from_file() to rebase-interactive.c
Allow the rebase.missingCommitsCheck configuration to kick in when
"rebase --edit-todo" and "rebase --continue" restarts the procedure.
Broken.
cf. <64aa4049-ee35-df4c-1e6c-80707f4f9070@gmail.com>
* es/pathspec-f-f-grep (2019-12-13) 1 commit
- grep: support the --pathspec-from-file option
"git grep" learned the "--pathspec-from-file" command line
option.
Waiting for review responses.
cf. <20191204203911.237056-1-emilyshaffer@google.com>
* at/rebase-fork-point-regression-fix (2019-12-09) 1 commit
- rebase: fix --fork-point with short refname
The "--fork-point" mode of "git rebase" regressed when the command
was rewritten in C back in 2.20 era, which has been corrected.
Was waiting for discussion to settle.
cf. <CAPig+cQ-3Ds41hr91fRo_GvuFMTP7zNVJtaSqi-Yccq4Pk-8Qg@mail.gmail.com>
* ma/config-bool-valex (2019-11-14) 8 commits
- builtin/config: die if "value_regex" doesn't canonicalize as boolean
- builtin/config: warn if "value_regex" doesn't canonicalize as boolean
- builtin/config: canonicalize "value_regex" with `--type=bool-or-int`
- builtin/config: canonicalize "value_regex" with `--type=bool`
- builtin/config: collect "value_regexp" data in a struct
- builtin/config: extract `handle_value_regex()` from `get_value()`
- t1300: modernize part of script
- config: make `git_parse_maybe_bool_text()` public
"git config" can be told to affect the existing entries that
"match" the given value via its value_regex argument. It learned
to normalize the value set in the configuration and the value given
from the command line before computing they "match", e.g. "true" in
the configuration file can now match with "yes" given from the
command line.
Needs a bit more work?
cf. <CAN0heSrtwi9V607vBX9PMSfNLQ8iGcno6_iGuR4Fs8ndGxqh8A@mail.gmail.com>
* ds/fsmonitor-testing (2019-12-09) 8 commits
- test-lib: clear watchman watches at test completion
- t7519: disable external GIT_TEST_FSMONITOR variable
- t7063: disable fsmonitor with status cache
- tests: disable fsmonitor in submodule tests
- t3030-merge-recursive.sh: disable fsmonitor when tweaking GIT_WORK_TREE
- t1301-shared-repo.sh: disable FSMONITOR
- fsmonitor: do not output to stderr for tests
- fsmonitor: disable in a bare repo
Updates around testing fsmoitor integration.
cf. <pull.466.v2.git.1575907804.gitgitgadget@gmail.com>
* mt/threaded-grep-in-object-store (2019-10-02) 11 commits
- grep: move driver pre-load out of critical section
- grep: re-enable threads in non-worktree case
- grep: protect packed_git [re-]initialization
- grep: allow submodule functions to run in parallel
- submodule-config: add skip_if_read option to repo_read_gitmodules()
- grep: replace grep_read_mutex by internal obj read lock
- object-store: allow threaded access to object reading
- replace-object: make replace operations thread-safe
- grep: fix racy calls in grep_objects()
- grep: fix race conditions at grep_submodule()
- grep: fix race conditions on userdiff calls
Traditionally, we avoided threaded grep while searching in objects
(as opposed to files in the working tree) as accesses to the object
layer is not thread-safe. This limitation is getting lifted.
Expecting a reroll.
cf. <CAHd-oW7UPSSExyLtfLMCObWogKrBOctYabrFrOdf9-4Q2PZmMg@mail.gmail.com>
* vn/reset-deleted-ita (2019-07-26) 1 commit
- reset: unstage empty deleted ita files
"git reset HEAD [<pathspec>]" did not reset an empty file that was
added with the intent-to-add bit.
Expecting a reroll.
* jn/unknown-index-extensions (2018-11-21) 2 commits
- index: offer advice for unknown index extensions
- index: do not warn about unrecognized extensions
A bit too alarming warning given when unknown index extensions
exist is getting revamped.
Expecting a reroll.
* jc/format-patch-delay-message-id (2019-04-05) 1 commit
- format-patch: move message-id and related headers to the end
The location "git format-patch --thread" adds the Message-Id:
header in the series of header fields has been moved down, which
may help working around a suspected bug in GMail MSA, reported at
<CAHk-=whP1stFZNAaJiMi5eZ9rj0MRt20Y_yHVczZPH+O01d+sA@mail.gmail.com>
Waiting for feedback to see if it truly helps.
Needs tests.
* js/protocol-advertise-multi (2018-12-28) 1 commit
- protocol: advertise multiple supported versions
The transport layer has been updated so that the protocol version
used can be negotiated between the parties, by the initiator
listing the protocol versions it is willing to talk, and the other
side choosing from one of them.
Expecting a reroll.
cf. <CANq=j3u-zdb_FvNJGPCmygNMScseav63GhVvBX3NcVS4f7TejA@mail.gmail.com>
* mk/use-size-t-in-zlib (2018-10-15) 1 commit
- zlib.c: use size_t for size
The wrapper to call into zlib followed our long tradition to use
"unsigned long" for sizes of regions in memory, which have been
updated to use "size_t".
--------------------------------------------------
[Cooking]
* dl/merge-autostash (2019-12-26) 17 commits
- pull: pass --autostash to merge
- t5520: make test_pull_autostash() accept expect_parent_num
- merge: teach --autostash option
- sequencer: unlink autostash in apply_autostash()
- sequencer: extract perform_autostash() from rebase
- rebase: generify create_autostash()
- rebase: extract create_autostash()
- reset: extract reset_head() from rebase
- rebase: generify reset_head()
- rebase: use apply_autostash() from sequencer.c
- sequencer: make apply_rebase() accept a path
- rebase: use read_oneliner()
- sequencer: make read_oneliner() extern
- sequencer: configurably warn on non-existent files
- sequencer: use file strbuf for read_oneliner()
- t7600: use test_write_lines()
- Makefile: alphabetically sort += lists
"git merge" learns the "--autostash" option.
* dl/test-must-fail-fixes-2 (2019-12-27) 16 commits
- t4124: let sed open its own files
- t4124: only mark git command with test_must_fail
- t3507: use test_path_is_missing()
- t3507: fix indentation
- t3504: do check for conflict marker after failed cherry-pick
- t3419: stop losing return code of git command
- t3415: increase granularity of test_auto_{fixup,squash}()
- t3415: stop losing return codes of git commands
- t3310: extract common no_notes_merge_left()
- t3030: use test_path_is_missing()
- t2018: replace "sha" with "oid"
- t2018: don't lose return code of git commands
- t2018: teach do_checkout() to accept `!` arg
- t2018: use test_must_fail for failing git commands
- t2018: add space between function name and ()
- t2018: remove trailing space from test description
Test updates.
Not quite.
cf. <CAPig+cQo1nbRo=n8-XOtycGijj3k1y_Zozu7VW-WTSBB9LncqQ@mail.gmail.com>
cf. <86lfqt36ah.fsf@gmail.com>
* jn/promote-proto2-to-default (2019-12-27) 5 commits
- fetch: default to protocol version 2
- protocol test: let protocol.version override GIT_TEST_PROTOCOL_VERSION
- test: request GIT_TEST_PROTOCOL_VERSION=0 when appropriate
- config doc: protocol.version is not experimental
- fetch test: use more robust test for filtered objects
(this branch uses jn/test-lint-one-shot-export-to-shell-function.)
The transport protocol version 2 becomes the default one.
Will merge to 'next'.
* ds/sparse-list-in-cone-mode (2019-12-30) 2 commits
(merged to 'next' on 2020-01-04 at e1a174647e)
+ sparse-checkout: document interactions with submodules
+ sparse-checkout: list directories in cone mode
"git sparse-checkout list" subcommand learned to give its output in
a more concise form when the "cone" mode is in effect.
Will merge to 'master'.
* am/test-pathspec-f-f-error-cases (2020-01-02) 1 commit
(merged to 'next' on 2020-01-04 at 73ac7e77fb)
+ t: add tests for error conditions with --pathspec-from-file
More tests.
Will cook in 'next'.
* ds/sparse-cone (2020-01-04) 2 commits
(merged to 'next' on 2020-01-04 at cc4b6fbb41)
+ Documentation/git-sparse-checkout.txt: fix a typo
+ sparse-checkout: use extern for global variables
Code cleanup.
Will merge to 'master'.
* en/merge-recursive-oid-eq-simplify (2020-01-02) 1 commit
(merged to 'next' on 2020-01-04 at 623ecf4f16)
+ merge-recursive: remove unnecessary oid_eq function
Code cleanup.
Will merge to 'master'.
* jt/sha1-file-remove-oi-skip-cached (2020-01-02) 1 commit
(merged to 'next' on 2020-01-04 at fab9964f10)
+ sha1-file: remove OBJECT_INFO_SKIP_CACHED
has_object_file() said "no" given an object registered to the
system via pretend_object_file(), making it inconsistent with
read_object_file(), causing lazy fetch to attempt fetching an
empty tree from promisor remotes.
Will cook in 'next'.
* ds/commit-graph-set-size-mult (2020-01-02) 1 commit
(merged to 'next' on 2020-01-04 at 71ea739a6c)
+ commit-graph: prefer default size_mult when given zero
The code to write split commit-graph file(s) upon fetching computed
bogus value for the parameter used in splitting the resulting
files, which has been corrected.
Will merge to 'master'.
* hw/commit-advise-while-rejecting (2019-12-19) 1 commit
(merged to 'next' on 2019-12-30 at e26700d582)
+ commit: honor advice.statusHints when rejecting an empty commit
"git commit" gives output similar to "git status" when there is
nothing to commit, but without honoring the advise.statusHints
configuration variable, which has been corrected.
Will cook in 'next'.
* yz/p4-py3 (2019-12-17) 14 commits
(merged to 'next' on 2019-12-30 at cd67de932d)
+ ci: also run linux-gcc pipeline with python3.5 environment
+ git-p4: use python3's input() everywhere
+ git-p4: simplify regex pattern generation for parsing diff-tree
+ git-p4: use dict.items() iteration for python3 compatibility
+ git-p4: use functools.reduce instead of reduce
+ git-p4: fix freezing while waiting for fast-import progress
+ git-p4: use marshal format version 2 when sending to p4
+ git-p4: open .gitp4-usercache.txt in text mode
+ git-p4: convert path to unicode before processing them
+ git-p4: encode/decode communication with git for python3
+ git-p4: encode/decode communication with p4 for python3
+ git-p4: remove string type aliasing
+ git-p4: change the expansion test from basestring to list
+ git-p4: make python2.7 the oldest supported version
Update "git p4" to work with Python 3.
Will cook in 'next'.
* hi/gpg-mintrustlevel (2019-12-27) 1 commit
(merged to 'next' on 2019-12-30 at 6c790280d2)
+ gpg-interface: add minTrustLevel as a configuration option
gpg.minTrustLevel configuration variable has been introduced to
tell various signature verification codepaths the required minimum
trust level.
Will cook in 'next'.
* sg/completion-worktree (2019-12-19) 6 commits
(merged to 'next' on 2019-12-25 at 42c895ab3b)
+ completion: list paths and refs for 'git worktree add'
+ completion: list existing working trees for 'git worktree' subcommands
+ completion: simplify completing 'git worktree' subcommands and options
+ completion: return the index of found word from __git_find_on_cmdline()
+ completion: clean up the __git_find_on_cmdline() helper function
+ t9902-completion: add tests for the __git_find_on_cmdline() helper
The command line completion (in contrib/) learned to complete
subcommands and arguments to "git worktree".
Will cook in 'next'.
* dl/credential-netrc (2019-12-20) 2 commits
(merged to 'next' on 2019-12-25 at 1fd27f81ac)
+ contrib/credential/netrc: work outside a repo
+ contrib/credential/netrc: make PERL_PATH configurable
Sample credential helper for using .netrc has been updated to work
out of the box.
Will cook in 'next'.
* dl/test-must-fail-fixes (2019-12-20) 15 commits
(merged to 'next' on 2019-12-25 at 3ef7c70bc5)
+ t1507: inline full_name()
+ t1507: run commands within test_expect_success
+ t1507: stop losing return codes of git commands
+ t1501: remove use of `test_might_fail cp`
+ t1409: use test_path_is_missing()
+ t1409: let sed open its own input file
+ t1307: reorder `nongit test_must_fail`
+ t1306: convert `test_might_fail rm` to `rm -f`
+ t0020: use ! check_packed_refs_marked
+ t0020: don't use `test_must_fail has_cr`
+ t0003: don't use `test_must_fail attr_check`
+ t0003: use test_must_be_empty()
+ t0003: use named parameters in attr_check()
+ t0000: replace test_must_fail with run_sub_test_lib_test_err()
+ t/lib-git-p4: use test_path_is_missing()
Test clean-up.
Will cook in 'next'.
* en/rebase-backend (2019-12-26) 15 commits
(merged to 'next' on 2019-12-30 at 5b58e268d6)
+ rebase: change the default backend from "am" to "merge"
+ rebase: make the backend configurable via config setting
+ rebase tests: repeat some tests using the merge backend instead of am
+ rebase tests: mark tests specific to the am-backend with --am
+ git-prompt: change the prompt for interactive-based rebases
+ rebase: add an --am option
+ rebase: move incompatibility checks between backend options a bit earlier
+ git-rebase.txt: add more details about behavioral differences of backends
+ rebase: allow more types of rebases to fast-forward
+ t3432: make these tests work with either am or merge backends
+ rebase: fix handling of restrict_revision
+ rebase: make sure to pass along the quiet flag to the sequencer
+ rebase, sequencer: remove the broken GIT_QUIET handling
+ t3406: simplify an already simple test
+ rebase: extend the options for handling of empty commits
"git rebase" has learned to use the sequencer backend by default,
while allowing "--am" option to go back to the traditional "am"
backend.
Will cook in 'next'.
* bc/hash-independent-tests-part-7 (2019-12-24) 20 commits
(merged to 'next' on 2019-12-30 at 0eedb894ba)
+ t5604: make hash independent
+ t5601: switch into repository to hash object
+ t5562: use $ZERO_OID
+ t5540: make hash size independent
+ t5537: make hash size independent
+ t5530: compute results based on object length
+ t5512: abstract away SHA-1-specific constants
+ t5510: make hash size independent
+ t5504: make hash algorithm independent
+ t5324: make hash size independent
+ t5319: make test work with SHA-256
+ t5319: change invalid offset for SHA-256 compatibility
+ t5318: update for SHA-256
+ t4300: abstract away SHA-1-specific constants
+ t4204: make hash size independent
+ t4202: abstract away SHA-1-specific constants
+ t4200: make hash size independent
+ t4134: compute appropriate length constant
+ t4066: compute index line in diffs
+ t4054: make hash-size independent
Preparation of test scripts for the day when the object names will
use SHA-256 continues.
Will cook in 'next'.
* ew/packfile-syscall-optim (2019-12-26) 2 commits
(merged to 'next' on 2019-12-30 at ada15abf22)
+ packfile: replace lseek+read with pread
+ packfile: remove redundant fcntl F_GETFD/F_SETFD
Code cleanup.
Will merge to 'master'.
* jn/test-lint-one-shot-export-to-shell-function (2019-12-27) 3 commits
(merged to 'next' on 2019-12-30 at d08f039473)
+ fetch test: mark test of "skipping" haves as v0-only
+ t/check-non-portable-shell: detect "FOO= shell_func", too
+ fetch test: avoid use of "VAR= cmd" with a shell function
(this branch is used by jn/promote-proto2-to-default.)
The test-lint machinery knew to check "VAR=VAL shell_function"
construct, but did not check "VAR= shell_funciton", which has been
corrected.
Will cook in 'next'.
* js/add-p-leftover-bits (2019-12-24) 9 commits
- ci: include the built-in `git add -i` in the `linux-gcc` job
- built-in add -p: handle Escape sequences more efficiently
- built-in add -p: handle Escape sequences in interactive.singlekey mode
- built-in add -p: respect the `interactive.singlekey` config setting
- terminal: add a new function to read a single keystroke
- terminal: accommodate Git for Windows' default terminal
- terminal: make the code of disable_echo() reusable
- built-in add -p: handle diff.algorithm
- built-in add -p: support interactive.diffFilter
(this branch uses js/patch-mode-in-others-in-c.)
The final leg of rewriting "add -i/-p" in C.
Will merge to 'next'.
* js/mingw-loosen-overstrict-tree-entry-checks (2020-01-02) 1 commit
(merged to 'next' on 2020-01-02 at 3088a0ccf1)
+ mingw: only test index entries for backslashes, not tree entries
An earlier update to Git for Windows declared that a tree object is
invalid if it has a path component with backslash in it, which was
overly strict, which has been corrected. The only protection the
Windows users need is to prevent such path (or any path that their
filesystem cannot check out) from entering the index.
Will merge to 'master'.
* pb/clarify-line-log-doc (2019-12-26) 2 commits
(merged to 'next' on 2019-12-30 at 7a4e15a436)
+ doc: log, gitk: line-log arguments must exist in starting revision
+ doc: log, gitk: document accepted line-log diff formats
Doc update.
Will merge to 'master'.
* pw/advise-rebase-skip (2019-12-06) 9 commits
- rebase -i: leave CHERRY_PICK_HEAD when there are conflicts
- rebase: fix advice when a fixup creates an empty commit
- commit: give correct advice for empty commit during a rebase
- commit: encapsulate determine_whence() for sequencer
- commit: use enum value for multiple cherry-picks
- sequencer: write CHERRY_PICK_HEAD for reword and edit
- cherry-pick: check commit error messages
- cherry-pick: add test for `--skip` advice in `git commit`
- t3404: use test_cmp_rev
The mechanism to prevent "git commit" from making an empty commit
or amending during an interrupted cherry-pick was broken during the
rewrite of "git rebase" in C, which has been corrected.
What's the status of this one?
The tip two are still RFC.
* js/patch-mode-in-others-in-c (2019-12-21) 7 commits
(merged to 'next' on 2019-12-30 at a767b89288)
+ commit --interactive: make it work with the built-in `add -i`
+ built-in add -p: implement the "worktree" patch modes
+ built-in add -p: implement the "checkout" patch modes
+ built-in stash: use the built-in `git add -p` if so configured
+ legacy stash -p: respect the add.interactive.usebuiltin setting
+ built-in add -p: implement the "stash" and "reset" patch modes
+ built-in add -p: prepare for patch modes other than "stage"
(this branch is used by js/add-p-leftover-bits.)
The effort to move "git-add--interactive" to C continues.
Will cook in 'next'.
* jk/packfile-reuse-cleanup (2019-10-23) 9 commits
(merged to 'next' on 2019-11-19 at 9683853939)
+ pack-objects: improve partial packfile reuse
+ builtin/pack-objects: introduce obj_is_packed()
+ pack-objects: introduce pack.allowPackReuse
+ csum-file: introduce hashfile_total()
+ pack-bitmap: introduce bitmap_walk_contains()
+ pack-bitmap: don't rely on bitmap_git->reuse_objects
+ ewah/bitmap: introduce bitmap_word_alloc()
+ packfile: expose get_delta_base()
+ builtin/pack-objects: report reused packfile objects
The way "git pack-objects" reuses objects stored in existing pack
to generate its result has been improved.
Hold. There is an update to these patches that currently are on 'next'.
cf. <20191115180319.113991-1-jonathantanmy@google.com>
--------------------------------------------------
[Discarded]
* js/advise-rebase-skip (2019-10-23) 3 commits
. commit: give correct advice for empty commit during a rebase
. sequencer: export the function to get the path of `.git/rebase-merge/`
. cherry-pick: add test for `--skip` advice in `git commit`
The logic used in "git commit" to give hints and errors depending
on what operation was in progress learned to distinguish rebase and
cherry-pick better.
Superseded by pw/advise-rebase-skip
* bk/p4-exception-cleanup (2019-12-16) 2 commits
. git-p4: failure because of RCS keywords should show help
. git-p4: wrap patchRCSKeywords test to revert changes on failure
Discarded for now without prejudice.
^ permalink raw reply
* git repo on NTFS mount
From: Lukas Schubert @ 2020-01-05 0:49 UTC (permalink / raw)
To: git
Hi there,
for historical reasons, I keep the data that doesn't belong to any
specific user on a harddisk that is formatted as NTFS. Some git
repositories are there, too. Some time ago, I upgraded from Linux Mint 17
to 19.2. That upgrade brought a change in data partition's mount options.
Old:
UUID=20D0WHATEVER /mnt/DATA ntfs defaults,nls=utf8,umask=000,uid=1000,windows_names
New: UUID=20D0WHATEVER /DATA ntfs defaults,umask=007,gid=46
Now I want to initialize a new git repository
user@xxxx:/DATA/Projects/LearnPython/wxGlade$ git init
error: chmod on /DATA/Projects/LearnPython/wxGlade/.git/config.lock
failed: Operation not permitted
fatal: could not set 'core.filemode' to 'false'
Since there already are repos on that drive, the initialization must have
worked before. But in
https://www.linuxquestions.org/questions/showthread.php?p=6074034#post6074034
I've been told that using git in linux with repositories on NTFS is a
recipe for disaster.
Given I change the mount options to what worked before the update, can I
escape certain doom if I stick to a certain subset of git commands? Or is
the cathastrophe inevitalbe due to subtle errors that culminate but stay
hidden until it's too late?
Thanks
Lukas
^ permalink raw reply
* Re: [PATCH 1/1] unpack-trees: exit check_updates() early if updates are not wanted
From: Elijah Newren @ 2020-01-05 0:49 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Elijah Newren via GitGitGadget, Git Mailing List
In-Reply-To: <xmqqimlqbx61.fsf@gitster-ct.c.googlers.com>
Hi Junio,
On Sat, Jan 4, 2020 at 3:48 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> "Elijah Newren via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
> > + if (!o->update || o->dry_run) {
> > + remove_marked_cache_entries(index, 0);
> > + trace_performance_leave("check_updates");
> > + return 0;
> > + }
>
> OK, so the idea is that bunch of codepaths we see later are
> protected by "o->update && !o->dry_run" in the original and are not
> executed, so we can leave early here.
>
> So the primary task of the reviewers of this patch is to see if the
> remainder of the function has some code that were *not* no-op when
> (!o->update || o->dry_run) is true in the current code, which would
> indicate possible behaviour change, and assess if the change in
> behaviour matters in the real world if there is.
>
> > if (o->clone)
> > setup_collided_checkout_detection(&state, index);
>
> If there were such a thing as dry-run of a clone, we will stop
> calling the report based on the thing we are setting up here?
> Presumably that does not happen in the current code---is that
> something we guarantee in the future evolution of the code, though?
setup_collided_checkout_detection() clears the CE_MATCHED flag for
each cache entry, which can be updated via way of checkout_entry()'s
call to mark_colliding_entries(). Then the trailing
report_collided_checkout() at the end of the function will report any
paths with CE_MATCHED set. However, when we're in a dry-run, all
checkout_entry() calls are skipped, so we won't detect ever detect any
collided entries. As such, there's no point in calling either the
setup_collided_checkout_detection() or report_collided_checkout()
funtions.
Sorry for forgetting to include this in the commit message.
> > progress = get_progress(o);
>
> get_progress() would yield NULL when !o->update, but o->dry_run does
> not influence it, so we would have called the progress API in
> today's code, if o->dry_run and o->update are both true.
>
> Presumably, o->update and o->dry_run are not true at the same time
> in the current code---but even if in the future we start supporting
> the combination, dry-run should be skipping the filesystem update
> (which is the slow part) and lack of progress may not matter, I
> guess?
The point of the progress display was to show how long it takes to do
the updates. For a dry-run, that time is zero because we skip the
updates. And if we're skipping the updates, shouldn't we also skip
measuring how long it takes to do them?
Yeah, I should have included this in the commit message too.
> It seems to me that unpack_trees_options.dry_run can become true
> only in "git read-tree" when the -n (or --dry-run) option is given
> and no other codepath touches it. Am I reading the code correctly?
I hadn't checked previously, but that looks right to me.
> Similarly, unpack_trees_options.clone is turned on only in the
> builtin/clone.c::checkout(). It _might_ occur to future developers
> that "git clone --no-checkout" is better implemented by actually
> going through builtin/clone.c::checkout() with .dry_run turned on,
> instead of leaving that function early. That would for example
> allow collided_checkout() detection to still be done---in such a
> future, is the optimization this patch makes still safe, I wonder?
If it is intended that a report of colliding paths be given when doing
a dry-run using unpack_trees, then the current code is quite buggy
because it'll never report anything. In order to report anything, the
mechanism for detecting them would need to be drastically reworked.
As such, I'd view the optimization as at least making it more obvious
why the code won't report collisions with dry-run rather than
pretending it will. Or are you saying that I've just uncovered a bug
and it should really be fixed rather than exploited for an
optimization?
> > - if (o->update)
> > - git_attr_set_direction(GIT_ATTR_CHECKOUT);
> > + git_attr_set_direction(GIT_ATTR_CHECKOUT);
> >
> > - if (should_update_submodules() && o->update && !o->dry_run)
> > + if (should_update_submodules())
> > load_gitmodules_file(index, NULL);
>
> Good (no behaviour change because this wouldn't have been done under
> the early-exit condition anyway).
> >
> > for (i = 0; i < index->cache_nr; i++) {
> > @@ -388,18 +393,18 @@ static int check_updates(struct unpack_trees_options *o)
> >
> > if (ce->ce_flags & CE_WT_REMOVE) {
> > display_progress(progress, ++cnt);
> > - if (o->update && !o->dry_run)
> > - unlink_entry(ce);
> > + unlink_entry(ce);
>
> Good (no behaviour change because this wouldn't have been done under
> the early-exit condition anyway).
>
> > }
> > }
> > +
> > remove_marked_cache_entries(index, 0);
> > remove_scheduled_dirs();
> >
> > - if (should_update_submodules() && o->update && !o->dry_run)
> > + if (should_update_submodules())
> > load_gitmodules_file(index, &state);
>
> Good (no behaviour change because this wouldn't have been done under
> the early-exit condition anyway).
> >
> > enable_delayed_checkout(&state);
> > - if (has_promisor_remote() && o->update && !o->dry_run) {
> > + if (has_promisor_remote()) {
>
> Good (no behaviour change because this wouldn't have been done under
> the early-exit condition anyway).
>
> > /*
> > * Prefetch the objects that are to be checked out in the loop
> > * below.
> > @@ -431,15 +436,12 @@ static int check_updates(struct unpack_trees_options *o)
> > ce->name);
> > display_progress(progress, ++cnt);
> > ce->ce_flags &= ~CE_UPDATE;
> > - if (o->update && !o->dry_run) {
> > - errs |= checkout_entry(ce, &state, NULL, NULL);
> > - }
> > + errs |= checkout_entry(ce, &state, NULL, NULL);
>
> Good (no behaviour change because this wouldn't have been done under
> the early-exit condition anyway).
>
> > }
> > }
> > stop_progress(&progress);
> > errs |= finish_delayed_checkout(&state, NULL);
> > - if (o->update)
> > - git_attr_set_direction(GIT_ATTR_CHECKIN);
> > + git_attr_set_direction(GIT_ATTR_CHECKIN);
> >
> > if (o->clone)
> > report_collided_checkout(index);
>
> Behaviour around this one (and the corresponding setup) may make a
> difference before and after the patch to future developers (who may
> need to revert this change to achieve what they want to do), but I
> think it is a no-op clean-up for today's code.
^ permalink raw reply
* Re: [PATCH] gitweb: fix a couple spelling errors in comments
From: Junio C Hamano @ 2020-01-04 23:51 UTC (permalink / raw)
To: Denis Ovsienko; +Cc: git
In-Reply-To: <20200104173923.45537050@basepc>
Denis Ovsienko <denis@ovsienko.info> writes:
> Date: Sat, 4 Jan 2020 17:33:55 +0000
>
> Signed-off-by: Denis Ovsienko <denis@ovsienko.info>
> ---
> gitweb/gitweb.perl | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
Both of these typofixes look correct.
By the way, please do not attempt to override the author timestamp
with an in-body header when contributing to this project, as the
time the general public sees the patch on the list is when they
perceive as the time the patch appeared in this world for the first
time, as far as this list is concerned.
Thanks.
>
> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
> index 0f857d790b..65a3a9e62e 100755
> --- a/gitweb/gitweb.perl
> +++ b/gitweb/gitweb.perl
> @@ -741,7 +741,7 @@ sub evaluate_gitweb_config {
> $GITWEB_CONFIG_SYSTEM = "" if ($GITWEB_CONFIG_SYSTEM eq $GITWEB_CONFIG_COMMON);
>
> # Common system-wide settings for convenience.
> - # Those settings can be ovverriden by GITWEB_CONFIG or GITWEB_CONFIG_SYSTEM.
> + # Those settings can be overridden by GITWEB_CONFIG or GITWEB_CONFIG_SYSTEM.
> read_config_file($GITWEB_CONFIG_COMMON);
>
> # Use first config file that exists. This means use the per-instance
> @@ -5285,7 +5285,7 @@ sub format_ctx_rem_add_lines {
> # + c
> # + d
> #
> - # Otherwise the highlightling would be confusing.
> + # Otherwise the highlighting would be confusing.
> if ($is_combined) {
> for (my $i = 0; $i < @$add; $i++) {
> my $prefix_rem = substr($rem->[$i], 0, $num_parents);
^ permalink raw reply
* Re: [PATCH 1/1] unpack-trees: exit check_updates() early if updates are not wanted
From: Junio C Hamano @ 2020-01-04 23:48 UTC (permalink / raw)
To: Elijah Newren via GitGitGadget; +Cc: git, Elijah Newren
In-Reply-To: <de0f381284cfe90c1bd8521abb8d29e3529c981a.1578087730.git.gitgitgadget@gmail.com>
"Elijah Newren via GitGitGadget" <gitgitgadget@gmail.com> writes:
> + if (!o->update || o->dry_run) {
> + remove_marked_cache_entries(index, 0);
> + trace_performance_leave("check_updates");
> + return 0;
> + }
OK, so the idea is that bunch of codepaths we see later are
protected by "o->update && !o->dry_run" in the original and are not
executed, so we can leave early here.
So the primary task of the reviewers of this patch is to see if the
remainder of the function has some code that were *not* no-op when
(!o->update || o->dry_run) is true in the current code, which would
indicate possible behaviour change, and assess if the change in
behaviour matters in the real world if there is.
> if (o->clone)
> setup_collided_checkout_detection(&state, index);
If there were such a thing as dry-run of a clone, we will stop
calling the report based on the thing we are setting up here?
Presumably that does not happen in the current code---is that
something we guarantee in the future evolution of the code, though?
> progress = get_progress(o);
get_progress() would yield NULL when !o->update, but o->dry_run does
not influence it, so we would have called the progress API in
today's code, if o->dry_run and o->update are both true.
Presumably, o->update and o->dry_run are not true at the same time
in the current code---but even if in the future we start supporting
the combination, dry-run should be skipping the filesystem update
(which is the slow part) and lack of progress may not matter, I
guess?
It seems to me that unpack_trees_options.dry_run can become true
only in "git read-tree" when the -n (or --dry-run) option is given
and no other codepath touches it. Am I reading the code correctly?
Similarly, unpack_trees_options.clone is turned on only in the
builtin/clone.c::checkout(). It _might_ occur to future developers
that "git clone --no-checkout" is better implemented by actually
going through builtin/clone.c::checkout() with .dry_run turned on,
instead of leaving that function early. That would for example
allow collided_checkout() detection to still be done---in such a
future, is the optimization this patch makes still safe, I wonder?
> - if (o->update)
> - git_attr_set_direction(GIT_ATTR_CHECKOUT);
> + git_attr_set_direction(GIT_ATTR_CHECKOUT);
>
> - if (should_update_submodules() && o->update && !o->dry_run)
> + if (should_update_submodules())
> load_gitmodules_file(index, NULL);
Good (no behaviour change because this wouldn't have been done under
the early-exit condition anyway).
>
> for (i = 0; i < index->cache_nr; i++) {
> @@ -388,18 +393,18 @@ static int check_updates(struct unpack_trees_options *o)
>
> if (ce->ce_flags & CE_WT_REMOVE) {
> display_progress(progress, ++cnt);
> - if (o->update && !o->dry_run)
> - unlink_entry(ce);
> + unlink_entry(ce);
Good (no behaviour change because this wouldn't have been done under
the early-exit condition anyway).
> }
> }
> +
> remove_marked_cache_entries(index, 0);
> remove_scheduled_dirs();
>
> - if (should_update_submodules() && o->update && !o->dry_run)
> + if (should_update_submodules())
> load_gitmodules_file(index, &state);
Good (no behaviour change because this wouldn't have been done under
the early-exit condition anyway).
>
> enable_delayed_checkout(&state);
> - if (has_promisor_remote() && o->update && !o->dry_run) {
> + if (has_promisor_remote()) {
Good (no behaviour change because this wouldn't have been done under
the early-exit condition anyway).
> /*
> * Prefetch the objects that are to be checked out in the loop
> * below.
> @@ -431,15 +436,12 @@ static int check_updates(struct unpack_trees_options *o)
> ce->name);
> display_progress(progress, ++cnt);
> ce->ce_flags &= ~CE_UPDATE;
> - if (o->update && !o->dry_run) {
> - errs |= checkout_entry(ce, &state, NULL, NULL);
> - }
> + errs |= checkout_entry(ce, &state, NULL, NULL);
Good (no behaviour change because this wouldn't have been done under
the early-exit condition anyway).
> }
> }
> stop_progress(&progress);
> errs |= finish_delayed_checkout(&state, NULL);
> - if (o->update)
> - git_attr_set_direction(GIT_ATTR_CHECKIN);
> + git_attr_set_direction(GIT_ATTR_CHECKIN);
>
> if (o->clone)
> report_collided_checkout(index);
Behaviour around this one (and the corresponding setup) may make a
difference before and after the patch to future developers (who may
need to revert this change to achieve what they want to do), but I
think it is a no-op clean-up for today's code.
^ permalink raw reply
* Re: [ANNOUNCE] Git v2.25.0-rc0
From: Junio C Hamano @ 2020-01-04 23:13 UTC (permalink / raw)
To: Taylor Blau; +Cc: git
In-Reply-To: <20200102221921.GA81596@syl.local>
Taylor Blau <me@ttaylorr.com> writes:
> I'm not sure if this is intentional, or if this was added twice during
> the merge(s) of and f7998d9793 (Merge branch 'js/builtin-add-i',
> 2019-12-05) and 3beff388b2 (Merge branch 'js/builtin-add-i-cmds',
> 2019-12-16).
It's the latter. Thanks.
^ permalink raw reply
* Re: Testsuite failures on ppc64, sparc64 and s390x (64-bit BE)
From: John Paul Adrian Glaubitz @ 2020-01-04 23:13 UTC (permalink / raw)
To: brian m. carlson; +Cc: git
In-Reply-To: <20200104224830.GF6570@camp.crustytoothpaste.net>
Hi Brian!
On 1/4/20 11:48 PM, brian m. carlson wrote:
> Did you build your version of Git with Subversion support, and if so,
> which version of Subversion did you use (version of Debian package or
> other source)? The tests that were failing all require git-svn, which
> in turn require libsvn-perl and subversion (/usr/bin/svn). If you're
> missing those packages, the Subversion tests will automatically be
> skipped.
Good point. I just ran the configure script without any extra options.
I will have another look and make sure git-svn is enabled.
> The latest version on master is 2.25-rc1, and that's failing in the
> Debian package. I haven't checked recently, but last I looked, the
> Debian package wasn't applying any additional patches on top of Git, so
> the version you're getting off GitHub is literally the same version that
> you're getting in the Debian package.
Okay. I wasn't sure whether there were any additional commits after the
2.25-rc1 tag, I hadn't checked yet as I was doing these tests on the
side while watching TV.
Adrian
--
.''`. John Paul Adrian Glaubitz
: :' : Debian Developer - glaubitz@debian.org
`. `' Freie Universitaet Berlin - glaubitz@physik.fu-berlin.de
`- GPG: 62FF 8A75 84E0 2956 9546 0006 7426 3B37 F5B5 F913
^ permalink raw reply
* Re: [PATCH] multi-pack-index: correct configuration in documentation
From: Junio C Hamano @ 2020-01-04 23:03 UTC (permalink / raw)
To: Johannes Berg; +Cc: git, Derrick Stolee
In-Reply-To: <20200104124314.10134-1-johannes@sipsolutions.net>
Johannes Berg <johannes@sipsolutions.net> writes:
> It's core.multiPackIndex, not pack.multiIndex.
>
> Signed-off-by: Johannes Berg <johannes@sipsolutions.net>
> ---
> Documentation/technical/multi-pack-index.txt | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
Thanks, will queue.
> diff --git a/Documentation/technical/multi-pack-index.txt b/Documentation/technical/multi-pack-index.txt
> index 1e312396966c..4e7631437a58 100644
> --- a/Documentation/technical/multi-pack-index.txt
> +++ b/Documentation/technical/multi-pack-index.txt
> @@ -36,7 +36,7 @@ Design Details
> directory of an alternate. It refers only to packfiles in that
> same directory.
>
> -- The pack.multiIndex config setting must be on to consume MIDX files.
> +- The core.multiPackIndex config setting must be on to consume MIDX files.
>
> - The file format includes parameters for the object ID hash
> function, so a future change of hash algorithm does not require
^ permalink raw reply
* Re: [RFC] xl command for visualizing recent history
From: Junio C Hamano @ 2020-01-04 22:56 UTC (permalink / raw)
To: Johannes Schindelin
Cc: Matthew DeVore, Emily Shaffer, Matthew DeVore, git,
Matthew DeVore, jonathantanmy, jrnieder, steadmon
In-Reply-To: <nycvar.QRO.7.76.6.2001042115550.46@tvgsbejvaqbjf.bet>
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> In any case, as stated before, I would like to see this feature be
> implemented as a `git log` (or even `git rev-list`) option before
> implementing a dedicated command.
>
> In other words, this new feature should be treated as a _mode_ rather than
> a new command. The command can come later, just like `git whatchanged`
> is essentially a special-case version of `git log`.
Yup, I agree that we may have plenty of commands that this can
become a feature of, and if there is a good match, we should make it
a mode of an existing command, and "git log" might be a natural
first candidate. If the focus is on the "recent topics in flight",
"git show-branch" might be a good home. There may be some other
candidates.
On the other hand, this thing may be sufficiently different from
everything else and deserves to be a separate command, just like
nobody would think it is a sane design choice to try making "git
shortlog" a mere mode of "git log".
An unrelated tangent, but I wonder if we want to start drafting the
transition plans to deprecate whatchanged. The command was invented
about two weeks before "log", but back then the latter did not know
how to drive diff-tree (iow, it was only about the commit log
messages), so they have both stayed to be "useful" for some time,
until the underlying machinery for "log" matured sufficiently and
made "whatchanged" more or less a special case of "log". It is not
hurting right now to keep it as-is and unmaintained, though.
^ permalink raw reply
* Re: Testsuite failures on ppc64, sparc64 and s390x (64-bit BE)
From: brian m. carlson @ 2020-01-04 22:48 UTC (permalink / raw)
To: John Paul Adrian Glaubitz; +Cc: git
In-Reply-To: <b7565f06-55a2-7087-d46e-94f9e7ada988@physik.fu-berlin.de>
[-- Attachment #1: Type: text/plain, Size: 2207 bytes --]
On 2020-01-04 at 22:14:21, John Paul Adrian Glaubitz wrote:
> Hi!
>
> On 1/4/20 9:14 PM, John Paul Adrian Glaubitz wrote:
> > It seems that git is failing its testsuite on all 64-bit Big-Endian targets,
> > full build log can be found in [1]. There seem to be multiple failures.
>
> Just checked out git with git from github (SCNR ;)) and built and ran the
> testsuite on ppc64, s390x and sparc64.
>
> ppc64:
>
> make aggregate-results
> make[3]: Entering directory '/srv/glaubitz/git-git/t'
> for f in 'test-results'/t*-*.counts; do \
> echo "$f"; \
> done | '/bin/sh' ./aggregate-results.sh
> fixed 0
> success 20683
> failed 0
> broken 239
> total 21161
>
> s390x:
>
> make aggregate-results
> make[3]: Entering directory '/home/glaubitz/git/t'
> for f in 'test-results'/t*-*.counts; do \
> echo "$f"; \
> done | '/bin/sh' ./aggregate-results.sh
> fixed 0
> success 21298
> failed 0
> broken 249
> total 21760
>
> sparc64:
>
> make aggregate-results
> make[3]: Entering directory '/home/glaubitz/git/t'
> for f in 'test-results'/t*-*.counts; do \
> echo "$f"; \
> done | '/bin/sh' ./aggregate-results.sh
> fixed 0
> success 20703
> failed 0
> broken 239
> total 21176
>
> So, it looks like the failures might be specific to the Debian package,
> doesn't it? Where there maybe any recent commits that may have fixed
> those issues?
Did you build your version of Git with Subversion support, and if so,
which version of Subversion did you use (version of Debian package or
other source)? The tests that were failing all require git-svn, which
in turn require libsvn-perl and subversion (/usr/bin/svn). If you're
missing those packages, the Subversion tests will automatically be
skipped.
The latest version on master is 2.25-rc1, and that's failing in the
Debian package. I haven't checked recently, but last I looked, the
Debian package wasn't applying any additional patches on top of Git, so
the version you're getting off GitHub is literally the same version that
you're getting in the Debian package.
--
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: Testsuite failures on ppc64, sparc64 and s390x (64-bit BE)
From: John Paul Adrian Glaubitz @ 2020-01-04 22:14 UTC (permalink / raw)
To: git
In-Reply-To: <34ed7497-643e-5a38-d68c-7c075b647bcd@physik.fu-berlin.de>
Hi!
On 1/4/20 9:14 PM, John Paul Adrian Glaubitz wrote:
> It seems that git is failing its testsuite on all 64-bit Big-Endian targets,
> full build log can be found in [1]. There seem to be multiple failures.
Just checked out git with git from github (SCNR ;)) and built and ran the
testsuite on ppc64, s390x and sparc64.
ppc64:
make aggregate-results
make[3]: Entering directory '/srv/glaubitz/git-git/t'
for f in 'test-results'/t*-*.counts; do \
echo "$f"; \
done | '/bin/sh' ./aggregate-results.sh
fixed 0
success 20683
failed 0
broken 239
total 21161
s390x:
make aggregate-results
make[3]: Entering directory '/home/glaubitz/git/t'
for f in 'test-results'/t*-*.counts; do \
echo "$f"; \
done | '/bin/sh' ./aggregate-results.sh
fixed 0
success 21298
failed 0
broken 249
total 21760
sparc64:
make aggregate-results
make[3]: Entering directory '/home/glaubitz/git/t'
for f in 'test-results'/t*-*.counts; do \
echo "$f"; \
done | '/bin/sh' ./aggregate-results.sh
fixed 0
success 20703
failed 0
broken 239
total 21176
So, it looks like the failures might be specific to the Debian package,
doesn't it? Where there maybe any recent commits that may have fixed
those issues?
Adrian
--
.''`. John Paul Adrian Glaubitz
: :' : Debian Developer - glaubitz@debian.org
`. `' Freie Universitaet Berlin - glaubitz@physik.fu-berlin.de
`- GPG: 62FF 8A75 84E0 2956 9546 0006 7426 3B37 F5B5 F913
^ permalink raw reply
* Re: [PATCH 1/1] worktree: delete branches auto-created by 'worktree add'
From: Pratyush Yadav @ 2020-01-04 21:47 UTC (permalink / raw)
To: Eric Sunshine; +Cc: Git List
In-Reply-To: <CAPig+cRL5w7azdALeBKKisTZwjgU6QhBqJRzQqDENjYiaTT0oA@mail.gmail.com>
Hi Eric,
Thanks for the review.
On 27/12/19 06:05AM, Eric Sunshine wrote:
> (Sorry for taking so long to review this patch; it ended up being a
> quite lengthy review.)
No problem :-). And sorry for being so late to reply.
> On Sat, Dec 14, 2019 at 11:16 AM Pratyush Yadav <me@yadavpratyush.com> wrote:
> > When no branch name is supplied to 'worktree add', it creates a new
> > branch based on the name of the directory the new worktree is located
> > in. But when the worktree is later removed, that created branch is left
> > over.
>
> This is describing the existing (intentional) behavior but doesn't
> explain why this might be annoying or problematic. To help sell the
> patch, it might make sense to say something about how the behavior can
> trip up newcomers to git-worktree, leaving them to wonder why they are
> accumulating so many branches that they weren't aware they created. A
> comment about why you think "git worktree add -d foo" is not a viable
> way to side-step the creation of unwanted branches might also be
> worthwhile. For instance, you might say something about how newcomers
> might not read the documentation thoroughly enough to know about
> --detach or to understand what it means; indeed, some newcomers to Git
> presumably have trouble with the concept of a detached HEAD and may
> find it scary.
Will do.
> > Remove that branch when removing the worktree. To make sure no commits
> > are lost, the branch won't be deleted if it has moved.
>
> My knee-jerk reaction upon reading the first sentence of this
> paragraph was that this is a significant and undesirable behavior
> change, however, the second sentence helps to allay my fears about it.
>
> It's possible, I suppose, that there is some existing tooling
> somewhere which relies upon the current behavior, but it's hard to
> imagine any good reason to do so. (That is, "git worktree add foo &&
> git worktree remove foo" is just a glorified and expensive way to say
> "git branch foo".) So, I don't look upon this change with disfavor; it
> could well be beneficial for newcomers, and perhaps a nice convenience
> in general.
It is possible that some script somewhere does
git worktree add foo
do_something # doesn't move the branch
git worktree remove foo
git branch -d foo
Branch deletion would fail here, which might be considered as an error
by the script. Not sure how common that would be though.
> However, there is a rather serious flaw in the implementation. My
> expectation is that it should only automatically delete a branch if
> the branch creation was inferred; it should never automatically delete
> a branch which was created explicitly. You kind of have this covered
> (and even have a test for it), but it doesn't work correctly when the
> user explicitly requests branch creation via -b/-B and the branch name
> matches the worktree name. For instance:
>
> git worktree add -b foo foo
> git worktree remove foo
>
> incorrectly automatically removes branch "foo" even though the user
> requested its creation explicitly.
Thanks for pointing it out. Will fix.
> Another big question: Should an automatically-created branch be
> deleted automatically when a worktree is pruned? That is, although
> this sequence will remove an automatically-created branch:
>
> git worktree add foo
> git worktree remove foo
>
> the current patch will not clean up the branch given this sequence:
>
> git worktree add foo
> rm -rf foo
> git worktree prune
I see no problem with doing the same thing in 'prune' too.
> Either way, it might be worthwhile to update the documentation to mention this.
I'll see if I can make 'prune' delete the branch too. Otherwise, I'll
mention it in the documentation.
> > An example use case of when something like this is useful is when the
> > user wants to check out a separate worktree to run and test on an older
> > version, but don't want to touch the current worktree. So, they create a
> > worktree, run some tests, and then remove it. But this leaves behind a
> > branch the user never created in the first place.
>
> The last sentence isn't exactly accurate. The user _did_ create the
> branch. It would be more accurate to say "...the user did not
> necessarily _intentionally_ create..." or something like that.
Yes, that was the intention of the sentence. Will fix.
> > So, remove the branch if nothing was done on it.
>
> By the way, the ordering of the commit message paragraphs is a bit
> off; it somewhat tries to justifies the change before explaining what
> the problem is. I'd suggest this order:
>
> - describe current behavior
> - explain why current behavior can be undesirable in some circumstances;
> cite your example use-case here, perhaps
> - describe how this patch improves the situation
>
> The two paragraphs which talk about "remove the branch" are just
> repeating one another. I would drop one of them and keep the other as
> the final bullet point of the suggested commit message order.
Will fix.
> > Signed-off-by: Pratyush Yadav <me@yadavpratyush.com>
> > ---
> > diff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt
> > @@ -73,8 +73,9 @@ If `<commit-ish>` is omitted and neither `-b` nor `-B` nor `--detach` used,
> > doesn't exist, a new branch based on HEAD is automatically created as
> > -if `-b <branch>` was given. If `<branch>` does exist, it will be
> > -checked out in the new worktree, if it's not checked out anywhere
> > +if `-b <branch>` was given. In this case, if `<branch>` is not moved, it is
> > +automatically deleted when the worktree is removed. If `<branch>` does exist,
> > +it will be checked out in the new worktree, if it's not checked out anywhere
>
> I found it confusing to find automatic branch deletion described here
> under the "worktree add" command...
>
> > @@ -108,6 +109,10 @@ Remove a working tree. Only clean working trees (no untracked files
> > +Removing a working tree might lead to its associated branch being deleted if
> > +it was auto-created and has not moved since. See `add` for more information on
> > +when exactly this can happen.
>
> Subjectively, it seems more natural to fully discuss automatic branch
> removal here rather than referring to the discussion of "worktree
> add".
I considered doing this but then left that part in 'add' because the
conditions in which the branch is auto deleted are described pretty well
in add's documentation. Will move it to 'remove'.
> > diff --git a/builtin/worktree.c b/builtin/worktree.c
> > @@ -35,6 +35,7 @@ struct add_opts {
> > +static int auto_create;
>
> I think this variable belongs in the 'add_opts' structure rather than
> being file-global.
Ok.
> > @@ -270,11 +271,13 @@ static int add_worktree(const char *path, const char *refname,
> > - int len, ret;
> > + int len, ret, fd;
> > + struct object_id oid;
> > + char *hex;
>
> Rather than declaring 'fd', 'oid', and 'hex' here, how about declaring
> them in the scope of the "if (auto_create) {" conditional below, which
> is the only place they are used?
Will do. Some other projects I've contributed to in the past insist on
declaring everything up-front, so I played it safe and put them here.
> > @@ -353,6 +356,18 @@ static int add_worktree(const char *path, const char *refname,
> > + strbuf_reset(&sb);
> > + strbuf_addf(&sb, "%s/auto_created", sb_repo.buf);
>
> Why aren't these two lines inside the "if (auto_create) {" conditional
> below? They seem to be used only for that case.
Yes, they should be. Will fix.
> I think this new worktree metadata file warrants a documentation
> update. In particular, gitrepository-layout.txt talks about
> worktree-specific metadata files, and the "Details" section of
> git-worktree.txt may need an update.
Will fix.
> A bit of bikeshedding regarding the filename: "auto_created" is rather
> unusual. Most names in the .git hierarchy are short and sweet. Also,
> with the exception of ORIG_HEAD and FETCH_HEAD, all other multi-word
> filenames seem to use hyphen rather than underscore, which suggests
> "auto-created" would be a better choice. However, I'd probably drop
> the hyphen altogether. Finally, "auto_created", alone, does not
> necessarily convey that the branch was auto-created; someone could
> misinterpret it as meaning the worktree itself was auto-created, so I
> wonder if a better name can be found.
Any suggestions? Does "implicitbranch"/"implicit-branch" sound any
better? How about "branch-auto-created-at"? It is very clear but is a
mouthful.
> A bigger question, though, is whether we really want to see new files
> like this springing up in the .git/worktrees/<id>/ directory for each
> new piece of metadata which belongs to a worktree. I ask because this
> isn't the first such case in which some additional worktree-specific
> metadata was proposed (see, for instance, [1]). So, I'm wondering if
> we should have a more generalized solution, such as introducing a new
> file which can hold any sort of metadata which comes along in the
> future. In particular, I'm thinking about a file containing an
> extensible set of "key: value" tuples, in which case the "auto
> created" metadata would be just one of possibly many keys. For
> instance:
Do you worry that the number of metadata files might grow to be too
large? I can't say how worktrees will grow in the future, but right now
there are 4 metadata files ('commondir', 'gitdir', 'HEAD', 'ORIG_HEAD').
So, not a lot.
I chose to add a new file because from what I have noticed, Git keeps a
lot of metadata in files like this (HEAD, refs, etc). Do other
subsystems use a key-value store? What problems did they face?
> branch-auto-created-at: deadbeef
>
> The above is a genuine question. I'm not demanding that this patch
> implement it, but I think it deserves discussion and thought before
> making a decision.
I'd prefer to not take on this feature (since I expect it to be a lot of
work), but if there are strong opinions on using a key-value store then
I guess I'll bite the bullet.
> [1]: http://public-inbox.org/git/CAPig+cRGMEjVbJZKXOskN6=5zchisx7UuwW9ZKGwoq5GQZQ_rw@mail.gmail.com/
>
> > + /* Mark this branch as an "auto-created" one. */
>
> This comment doesn't really say anything which the code itself isn't
> already saying (especially if you move the strbuf_addf() call inside
> the conditional), so the comment could be dropped.
Ok.
> > + if (auto_create) {
> > + fd = xopen(sb.buf, O_WRONLY | O_CREAT | O_TRUNC, 0666);
> > + get_oid("HEAD", &oid);
>
> Unless I'm mistaken, this is just wrong. You're grabbing the OID of
> HEAD from the worktree in which "worktree add" is being invoked,
> however, if the new branch name is DWIM'd from an existing
> tracking-branch, then the OID should be that of the tracking-branch,
> not HEAD of the current worktree. So, you should be using the OID
> already looked up earlier in the function, 'commit->object.oid', which
> should be correct for either case.
Oops! Thanks for pointing it out. Will fix.
> > + hex = oid_to_hex(&oid);
> > + write_file_buf(sb.buf, hex, strlen(hex));
> > +
> > + if (close(fd))
> > + die(_("could not close '%s'"), sb.buf);
> > + }
>
> Is there a reason you're creating the file in this rather manual
> fashion rather than using write_file() as is already heavily used in
> this code for creating all the other files residing in
> .git/worktrees/<id>/?
No particular reason. I didn't look around enough to catch the pattern
of using write_file() for this. Will fix.
> This code is correctly sandwiched within the "is_junk" scope, which
> means that "auto_created" will be cleaned up automatically, along with
> other .git/worktrees/<id>/ files, if "worktree add" fails for some
> reason. Good.
>
> > argv_array_pushf(&child_env, "%s=%s", GIT_DIR_ENVIRONMENT, sb_git.buf);
> > argv_array_pushf(&child_env, "%s=%s", GIT_WORK_TREE_ENVIRONMENT, path);
> > @@ -576,6 +591,8 @@ static int add(int ac, const char **av, const char *prefix)
> > if (run_command(&cp))
> > return -1;
> > branch = new_branch;
> > +
> > + auto_create = 1;
>
> Drop the unnecessary blank line.
>
> By the way, this suffers from the problem that if "git worktree add
> foo" fails for some reason, such as because path "foo" already exists,
> then the new branch will _not_ be cleaned up automatically since that
> failure will happen before "auto_created" is ever created (among other
> reasons). But that's not a new issue; it's an existing flaw of
> "worktree add" not cleaning up a branch it created before it discovers
> that it can't actually create the target directory for some reason, so
> I wouldn't expect you to fix that problem with this submission. (I'm
> just mentioning it for completeness.)
I'll see if I can come up with a fix for this as a follow-up patch.
> > @@ -912,9 +929,10 @@ static int remove_worktree(int ac, const char **av, const char *prefix)
> > OPT_END()
> > };
> > - struct strbuf errmsg = STRBUF_INIT;
> > + struct strbuf errmsg = STRBUF_INIT, sb = STRBUF_INIT, hex = STRBUF_INIT;
> > - int ret = 0;
> > + int ret = 0, delete_auto_created = 0;
> > + struct object_id oid;
>
> Perhaps move the declarations of 'hex' and 'oid' into the scope where
> they are used rather than making them global to the function.
Will do.
> > @@ -939,6 +957,23 @@ static int remove_worktree(int ac, const char **av, const char *prefix)
> > + /*
> > + * Check if we auto-created a branch for this worktree and it hasn't
> > + * moved since. Do it before the contents of the worktree get wiped.
> > + * Delete the branch later because it is checked out right now.
> > + */
>
> Good useful comment.
Thanks.
> > + git_path_buf(&sb, "worktrees/%s/auto_created", wt->id);
> > + if (file_exists(sb.buf)) {
> > + strbuf_read_file(&hex, sb.buf, 0);
>
> You can avoid an unnecessary race condition here by dropping the
> file_exists() call altogether and just checking the return code of
> strbuf_read_file() -- which you should probably be doing anyhow. If
> strbuf_read_file() returns a non-negative value, then you know it
> exists, so file_exists() is redundant.
Will fix. Though I don't see how it would be a "race condition". Is
file_exists() asynchronous in some way? Otherwise, how would a race
happen and between what?
> > + get_oid(wt->id, &oid);
> > +
>
> Drop the unnecessary blank line.
>
> > + if (strcmp(hex.buf, oid_to_hex(&oid)) == 0)
> > + delete_auto_created = 1;
>
> I was wondering if it would be more semantically correct to parse
> 'hex' into an 'oid' and compare them with oidcmp() rather than doing a
> string comparison of the hex values (though I'm not sure it will
> matter in practice).
Since I haven't spent too much time in the Git internals, the string
representation feels more natural to me. And that's why I went this way
subconsciously. While I don't mind either, I wonder if it would make a
difference in practice. Anyway, if you have a preference for the other
way round, I'll trust your gut feeling.
> > + }
> > +
> > + strbuf_release(&sb);
> > + strbuf_release(&hex);
>
> Drop the unnecessary blank line.
>
> > @@ -952,6 +987,17 @@ static int remove_worktree(int ac, const char **av, const char *prefix)
> > + if (delete_auto_created) {
> > + struct child_process cp = CHILD_PROCESS_INIT;
> > + cp.git_cmd = 1;
> > +
> > + argv_array_push(&cp.args, "branch");
> > + argv_array_push(&cp.args, "-d");
> > + argv_array_push(&cp.args, wt->id);
> > +
> > + ret |= run_command(&cp);
> > + }
>
> Alternately:
>
> argv_array_pushl(&cp.args, "branch", "-d", wt->id, NULL);
Ok.
> However, I don't think it is correct to use 'wt->id' here as the
> branch name since there is no guarantee that the <id> in
> .git/worktrees/<id>/ matches the branch name with which the worktree
> was created. For instance:
>
> git worktree add foo/bar existing-branch
> git worktree add baz/bar
>
> will, due to name conflicts, create worktree metadata directories:
>
> .git/worktrees/bar
> .git/worktrees/bar1
>
> where the first is associated with branch "existing-branch", and the
> second is associated with new branch "bar". When you then invoke "git
> worktree remove baz/bar", it will try removing a branch named "bar1",
> not "bar" as intended. To fix this, I think you need to record the
> original auto-created branch name in the "auto_created" metadata file
> too, not just the OID.
Interesting! Didn't think of a situation like this. Thanks for pointing
it out. Will fix.
> Finally, make this code consistent with other existing similar code in
> this file by dropping the unnecessary blank lines in this hunk.
The blank lines are a personal preference for me mostly. I am not a huge
fan of seeing large chunks of code with do blank lines in between. IMO
it hurts readability. But, I think staying consistent with the code that
already exists is more important. Will remove all them.
> > diff --git a/t/t2403-worktree-move.sh b/t/t2403-worktree-move.sh
> > @@ -222,4 +222,49 @@ test_expect_success 'not remove a repo with initialized submodule' '
> > +test_expect_success 'remove auto-created branch' '
> > + (
> > + git worktree add to-remove &&
> > + git worktree remove to-remove &&
> > + git branch -l to-remove >branch_list &&
> > + test_line_count = 0 branch_list
> > + )
> > +'
>
> I don't think there is any need for this test to be run in a subshell,
> so you can drop the enclosing '(' and ')'.
I was following the pattern in the two tests above. Will drop the
parentheses.
> I worry about using porcelain git-branch to check whether the branch
> has actually been removed. Using git-rev-parse would likely be a more
> direct and safe way to test it. For instance:
>
> git worktree add to-remove &&
> git worktree remove to-remove &&
> test_must_fail git rev-parse --verify -q to-remove
>
> should be sufficient, I think. And, to be really thorough, you might say:
>
> test_might_fail git branch -D to-remove &&
> git worktree add to-remove &&
> git rev-parse --verify -q to-remove &&
> git worktree remove to-remove &&
> test_must_fail git rev-parse --verify -q to-remove
>
> The above comments apply to the other new tests added by this patch, as well.
Will fix.
> > +test_expect_success 'do not remove a branch that was not auto-created' '
> > + (
> > + git worktree add -b new_branch to-remove &&
>
> Nit: The inconsistent mix of underscore and hyphen in names is odd.
> Perhaps settle on one or the other (with a slight preference toward
> hyphen).
I'll change 'new_branch' to 'new-branch'.
> > + git worktree remove to-remove &&
> > + git branch -l new_branch >branch_list &&
> > + test_line_count = 1 branch_list &&
>
> As noted earlier, although this particular case of a branch created
> explicitly with -b works as expected by not deleting the branch, the
> similar case:
>
> git worktree add -b to-remove to-remove &&
>
> will incorrectly automatically delete the branch.
>
> > + git branch -d new_branch &&
> > + git branch foo &&
> > + git worktree add to-remove foo &&
> > + git worktree remove to-remove &&
> > + git branch -l foo >branch_list &&
> > + test_line_count = 1 branch_list &&
> > + git branch -d foo &&
> > + git branch to-remove &&
> > + git worktree add to-remove &&
> > + git worktree remove to-remove &&
> > + git branch -l to-remove >branch_list &&
> > + test_line_count = 1 branch_list &&
> > + git branch -d to-remove
>
> If any code above this "git branch -d" fails, then it will never get
> this far, thus won't remove "to-remove". To perform cleanup whether
> the test succeeds or fails, you should use test_when_finished()
> _early_ in the test:
>
> test_when_finished "git branch -d to-remove || :" &&
>
> However, if you restructure the tests as suggested above, then you
> might be able to get away without bothering with this cleanup.
>
> > + )
> > +'
>
> This test is checking three distinct cases of explicitly-created
> branches. It would make it easier to debug a failing case if you split
> it up into three tests -- one for each case.
I considered doing it, but then I thought maybe I shouldn't add so many
tests. And since there are only 3 rather independent cases, it would not
be that difficult to figure out which one is the culprit. Will split
them.
> > +test_expect_success 'do not remove auto-created branch that was moved' '
> > + (
> > + git worktree add to-remove &&
> > + cd to-remove &&
> > + test_commit foo &&
> > + cd ../ &&
>
> We normally avoid cd'ing around in tests like this because it can
> cause tests following this one to run in the wrong directory if
> something above the "cd ../" fails. In this particular case, it
> doesn't matter since the entire body of this test is within a
> subshell.
>
> However, if you take advantage of test_commits()'s -C argument, then
> you can ditch the cd's and the subshell altogether:
>
> test_commit -C to-remove foo &&
Ok.
> > + git worktree remove to-remove &&
> > + git branch -l to-remove >branch_list &&
> > + test_line_count = 1 branch_list &&
> > + git branch -D to-remove
> > + )
> > +'
I'll send out the v2 as soon as I can. Thanks.
--
Regards,
Pratyush Yadav
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox