* [PATCH v2 08/12] builtin/show-ref: refactor options for patterns subcommand
From: Patrick Steinhardt @ 2023-10-26 9:56 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Eric Sunshine, Han-Wen Nienhuys
In-Reply-To: <cover.1698314128.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 3956 bytes --]
The patterns subcommand is the last command that still uses global
variables to track its options. Convert it to use a structure instead
with the same motivation as preceding commits.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
builtin/show-ref.c | 35 ++++++++++++++++++++++-------------
1 file changed, 22 insertions(+), 13 deletions(-)
diff --git a/builtin/show-ref.c b/builtin/show-ref.c
index 3e6ad6b6a84..87bc45d2d13 100644
--- a/builtin/show-ref.c
+++ b/builtin/show-ref.c
@@ -18,8 +18,6 @@ static const char * const show_ref_usage[] = {
NULL
};
-static int show_head, tags_only, heads_only, verify;
-
struct show_one_options {
int quiet;
int hash_only;
@@ -59,6 +57,7 @@ struct show_ref_data {
const struct show_one_options *show_one_opts;
const char **patterns;
int found_match;
+ int show_head;
};
static int show_ref(const char *refname, const struct object_id *oid,
@@ -66,7 +65,7 @@ static int show_ref(const char *refname, const struct object_id *oid,
{
struct show_ref_data *data = cbdata;
- if (show_head && !strcmp(refname, "HEAD"))
+ if (data->show_head && !strcmp(refname, "HEAD"))
goto match;
if (data->patterns) {
@@ -180,22 +179,30 @@ static int cmd_show_ref__verify(const struct show_one_options *show_one_opts,
return 0;
}
-static int cmd_show_ref__patterns(const struct show_one_options *show_one_opts,
+struct patterns_options {
+ int show_head;
+ int heads_only;
+ int tags_only;
+};
+
+static int cmd_show_ref__patterns(const struct patterns_options *opts,
+ const struct show_one_options *show_one_opts,
const char **patterns)
{
struct show_ref_data show_ref_data = {
.show_one_opts = show_one_opts,
+ .show_head = opts->show_head,
};
if (patterns && *patterns)
show_ref_data.patterns = patterns;
- if (show_head)
+ if (opts->show_head)
head_ref(show_ref, &show_ref_data);
- if (heads_only || tags_only) {
- if (heads_only)
+ if (opts->heads_only || opts->tags_only) {
+ if (opts->heads_only)
for_each_fullref_in("refs/heads/", show_ref, &show_ref_data);
- if (tags_only)
+ if (opts->tags_only)
for_each_fullref_in("refs/tags/", show_ref, &show_ref_data);
} else {
for_each_ref(show_ref, &show_ref_data);
@@ -233,15 +240,17 @@ static int exclude_existing_callback(const struct option *opt, const char *arg,
int cmd_show_ref(int argc, const char **argv, const char *prefix)
{
struct exclude_existing_options exclude_existing_opts = {0};
+ struct patterns_options patterns_opts = {0};
struct show_one_options show_one_opts = {0};
+ int verify = 0;
const struct option show_ref_options[] = {
- OPT_BOOL(0, "tags", &tags_only, N_("only show tags (can be combined with heads)")),
- OPT_BOOL(0, "heads", &heads_only, N_("only show heads (can be combined with tags)")),
+ OPT_BOOL(0, "tags", &patterns_opts.tags_only, N_("only show tags (can be combined with heads)")),
+ OPT_BOOL(0, "heads", &patterns_opts.heads_only, N_("only show heads (can be combined with tags)")),
OPT_BOOL(0, "verify", &verify, N_("stricter reference checking, "
"requires exact ref path")),
- OPT_HIDDEN_BOOL('h', NULL, &show_head,
+ OPT_HIDDEN_BOOL('h', NULL, &patterns_opts.show_head,
N_("show the HEAD reference, even if it would be filtered out")),
- OPT_BOOL(0, "head", &show_head,
+ OPT_BOOL(0, "head", &patterns_opts.show_head,
N_("show the HEAD reference, even if it would be filtered out")),
OPT_BOOL('d', "dereference", &show_one_opts.deref_tags,
N_("dereference tags into object IDs")),
@@ -267,5 +276,5 @@ int cmd_show_ref(int argc, const char **argv, const char *prefix)
else if (verify)
return cmd_show_ref__verify(&show_one_opts, argv);
else
- return cmd_show_ref__patterns(&show_one_opts, argv);
+ return cmd_show_ref__patterns(&patterns_opts, &show_one_opts, argv);
}
--
2.42.0
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* [PATCH v2 09/12] builtin/show-ref: ensure mutual exclusiveness of subcommands
From: Patrick Steinhardt @ 2023-10-26 9:56 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Eric Sunshine, Han-Wen Nienhuys
In-Reply-To: <cover.1698314128.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 1823 bytes --]
The git-show-ref(1) command has three different modes, of which one is
implicit and the other two can be chosen explicitly by passing a flag.
But while these modes are standalone and cause us to execute completely
separate code paths, we gladly accept the case where a user asks for
both `--exclude-existing` and `--verify` at the same time even though it
is not obvious what will happen. Spoiler: we ignore `--verify` and
execute the `--exclude-existing` mode.
Let's explicitly detect this invalid usage and die in case both modes
were requested.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
builtin/show-ref.c | 4 ++++
t/t1403-show-ref.sh | 5 +++++
2 files changed, 9 insertions(+)
diff --git a/builtin/show-ref.c b/builtin/show-ref.c
index 87bc45d2d13..1768aef77b3 100644
--- a/builtin/show-ref.c
+++ b/builtin/show-ref.c
@@ -271,6 +271,10 @@ int cmd_show_ref(int argc, const char **argv, const char *prefix)
argc = parse_options(argc, argv, prefix, show_ref_options,
show_ref_usage, 0);
+ if ((!!exclude_existing_opts.enabled + !!verify) > 1)
+ die(_("only one of '%s' or '%s' can be given"),
+ "--exclude-existing", "--verify");
+
if (exclude_existing_opts.enabled)
return cmd_show_ref__exclude_existing(&exclude_existing_opts);
else if (verify)
diff --git a/t/t1403-show-ref.sh b/t/t1403-show-ref.sh
index 9252a581abf..4a90a88e05d 100755
--- a/t/t1403-show-ref.sh
+++ b/t/t1403-show-ref.sh
@@ -196,4 +196,9 @@ test_expect_success 'show-ref --verify with dangling ref' '
)
'
+test_expect_success 'show-ref sub-modes are mutually exclusive' '
+ test_must_fail git show-ref --verify --exclude-existing 2>err &&
+ grep "only one of ${SQ}--exclude-existing${SQ} or ${SQ}--verify${SQ} can be given" err
+'
+
test_done
--
2.42.0
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* [PATCH v2 10/12] builtin/show-ref: explicitly spell out different modes in synopsis
From: Patrick Steinhardt @ 2023-10-26 9:57 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Eric Sunshine, Han-Wen Nienhuys
In-Reply-To: <cover.1698314128.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 2536 bytes --]
The synopsis treats the `--verify` and the implicit mode the same. They
are slightly different though:
- They accept different sets of flags.
- The implicit mode accepts patterns while the `--verify` mode
accepts references.
Split up the synopsis for these two modes such that we can disambiguate
those differences.
While at it, drop "--quiet" from the pattern mode's synopsis. It does
not make a lot of sense to list patterns, but squelch the listing output
itself. The description for "--quiet" is adapted accordingly.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
Documentation/git-show-ref.txt | 9 ++++++---
builtin/show-ref.c | 5 ++++-
2 files changed, 10 insertions(+), 4 deletions(-)
diff --git a/Documentation/git-show-ref.txt b/Documentation/git-show-ref.txt
index 2fe274b8faa..22f5ebc6a92 100644
--- a/Documentation/git-show-ref.txt
+++ b/Documentation/git-show-ref.txt
@@ -8,9 +8,12 @@ git-show-ref - List references in a local repository
SYNOPSIS
--------
[verse]
-'git show-ref' [-q | --quiet] [--verify] [--head] [-d | --dereference]
+'git show-ref' [--head] [-d | --dereference]
[-s | --hash[=<n>]] [--abbrev[=<n>]] [--tags]
[--heads] [--] [<pattern>...]
+'git show-ref' --verify [-q | --quiet] [-d | --dereference]
+ [-s | --hash[=<n>]] [--abbrev[=<n>]]
+ [--] [<ref>...]
'git show-ref' --exclude-existing[=<pattern>]
DESCRIPTION
@@ -70,8 +73,8 @@ OPTIONS
-q::
--quiet::
- Do not print any results to stdout. When combined with `--verify`, this
- can be used to silently check if a reference exists.
+ Do not print any results to stdout. Can be used with `--verify` to
+ silently check if a reference exists.
--exclude-existing[=<pattern>]::
diff --git a/builtin/show-ref.c b/builtin/show-ref.c
index 1768aef77b3..d4561d7ce1f 100644
--- a/builtin/show-ref.c
+++ b/builtin/show-ref.c
@@ -11,9 +11,12 @@
#include "parse-options.h"
static const char * const show_ref_usage[] = {
- N_("git show-ref [-q | --quiet] [--verify] [--head] [-d | --dereference]\n"
+ N_("git show-ref [--head] [-d | --dereference]\n"
" [-s | --hash[=<n>]] [--abbrev[=<n>]] [--tags]\n"
" [--heads] [--] [<pattern>...]"),
+ N_("git show-ref --verify [-q | --quiet] [-d | --dereference]\n"
+ " [-s | --hash[=<n>]] [--abbrev[=<n>]]\n"
+ " [--] [<ref>...]"),
N_("git show-ref --exclude-existing[=<pattern>]"),
NULL
};
--
2.42.0
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* [PATCH v2 11/12] builtin/show-ref: add new mode to check for reference existence
From: Patrick Steinhardt @ 2023-10-26 9:57 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Eric Sunshine, Han-Wen Nienhuys
In-Reply-To: <cover.1698314128.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 9759 bytes --]
While we have multiple ways to show the value of a given reference, we
do not have any way to check whether a reference exists at all. While
commands like git-rev-parse(1) or git-show-ref(1) can be used to check
for reference existence in case the reference resolves to something
sane, neither of them can be used to check for existence in some other
scenarios where the reference does not resolve cleanly:
- References which have an invalid name cannot be resolved.
- References to nonexistent objects cannot be resolved.
- Dangling symrefs can be resolved via git-symbolic-ref(1), but this
requires the caller to special case existence checks depending on
whether or not a reference is symbolic or direct.
Furthermore, git-rev-list(1) and other commands do not let the caller
distinguish easily between an actually missing reference and a generic
error.
Taken together, this seems like sufficient motivation to introduce a
separate plumbing command to explicitly check for the existence of a
reference without trying to resolve its contents.
This new command comes in the form of `git show-ref --exists`. This
new mode will exit successfully when the reference exists, with a
specific exit code of 2 when it does not exist, or with 1 when there
has been a generic error.
Note that the only way to properly implement this command is by using
the internal `refs_read_raw_ref()` function. While the public function
`refs_resolve_ref_unsafe()` can be made to behave in the same way by
passing various flags, it does not provide any way to obtain the errno
with which the reference backend failed when reading the reference. As
such, it becomes impossible for us to distinguish generic errors from
the explicit case where the reference wasn't found.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
Documentation/git-show-ref.txt | 11 ++++++
builtin/show-ref.c | 49 ++++++++++++++++++++++---
t/t1403-show-ref.sh | 67 +++++++++++++++++++++++++++++++++-
3 files changed, 121 insertions(+), 6 deletions(-)
diff --git a/Documentation/git-show-ref.txt b/Documentation/git-show-ref.txt
index 22f5ebc6a92..8fecc9e80f6 100644
--- a/Documentation/git-show-ref.txt
+++ b/Documentation/git-show-ref.txt
@@ -15,6 +15,7 @@ SYNOPSIS
[-s | --hash[=<n>]] [--abbrev[=<n>]]
[--] [<ref>...]
'git show-ref' --exclude-existing[=<pattern>]
+'git show-ref' --exists <ref>
DESCRIPTION
-----------
@@ -30,6 +31,10 @@ The `--exclude-existing` form is a filter that does the inverse. It reads
refs from stdin, one ref per line, and shows those that don't exist in
the local repository.
+The `--exists` form can be used to check for the existence of a single
+references. This form does not verify whether the reference resolves to an
+actual object.
+
Use of this utility is encouraged in favor of directly accessing files under
the `.git` directory.
@@ -65,6 +70,12 @@ OPTIONS
Aside from returning an error code of 1, it will also print an error
message if `--quiet` was not specified.
+--exists::
+
+ Check whether the given reference exists. Returns an exit code of 0 if
+ it does, 2 if it is missing, and 1 in case looking up the reference
+ failed with an error other than the reference being missing.
+
--abbrev[=<n>]::
Abbreviate the object name. When using `--hash`, you do
diff --git a/builtin/show-ref.c b/builtin/show-ref.c
index d4561d7ce1f..2cbe4e3f721 100644
--- a/builtin/show-ref.c
+++ b/builtin/show-ref.c
@@ -2,7 +2,7 @@
#include "config.h"
#include "gettext.h"
#include "hex.h"
-#include "refs.h"
+#include "refs/refs-internal.h"
#include "object-name.h"
#include "object-store-ll.h"
#include "object.h"
@@ -18,6 +18,7 @@ static const char * const show_ref_usage[] = {
" [-s | --hash[=<n>]] [--abbrev[=<n>]]\n"
" [--] [<ref>...]"),
N_("git show-ref --exclude-existing[=<pattern>]"),
+ N_("git show-ref --exists <ref>"),
NULL
};
@@ -216,6 +217,41 @@ static int cmd_show_ref__patterns(const struct patterns_options *opts,
return 0;
}
+static int cmd_show_ref__exists(const char **refs)
+{
+ struct strbuf unused_referent = STRBUF_INIT;
+ struct object_id unused_oid;
+ unsigned int unused_type;
+ int failure_errno = 0;
+ const char *ref;
+ int ret = 0;
+
+ if (!refs || !*refs)
+ die("--exists requires a reference");
+ ref = *refs++;
+ if (*refs)
+ die("--exists requires exactly one reference");
+
+ if (refs_read_raw_ref(get_main_ref_store(the_repository), ref,
+ &unused_oid, &unused_referent, &unused_type,
+ &failure_errno)) {
+ if (failure_errno == ENOENT) {
+ error(_("reference does not exist"));
+ ret = 2;
+ } else {
+ errno = failure_errno;
+ error_errno(_("failed to look up reference"));
+ ret = 1;
+ }
+
+ goto out;
+ }
+
+out:
+ strbuf_release(&unused_referent);
+ return ret;
+}
+
static int hash_callback(const struct option *opt, const char *arg, int unset)
{
struct show_one_options *opts = opt->value;
@@ -245,10 +281,11 @@ int cmd_show_ref(int argc, const char **argv, const char *prefix)
struct exclude_existing_options exclude_existing_opts = {0};
struct patterns_options patterns_opts = {0};
struct show_one_options show_one_opts = {0};
- int verify = 0;
+ int verify = 0, exists = 0;
const struct option show_ref_options[] = {
OPT_BOOL(0, "tags", &patterns_opts.tags_only, N_("only show tags (can be combined with heads)")),
OPT_BOOL(0, "heads", &patterns_opts.heads_only, N_("only show heads (can be combined with tags)")),
+ OPT_BOOL(0, "exists", &exists, N_("check for reference existence without resolving")),
OPT_BOOL(0, "verify", &verify, N_("stricter reference checking, "
"requires exact ref path")),
OPT_HIDDEN_BOOL('h', NULL, &patterns_opts.show_head,
@@ -274,14 +311,16 @@ int cmd_show_ref(int argc, const char **argv, const char *prefix)
argc = parse_options(argc, argv, prefix, show_ref_options,
show_ref_usage, 0);
- if ((!!exclude_existing_opts.enabled + !!verify) > 1)
- die(_("only one of '%s' or '%s' can be given"),
- "--exclude-existing", "--verify");
+ if ((!!exclude_existing_opts.enabled + !!verify + !!exists) > 1)
+ die(_("only one of '%s', '%s' or '%s' can be given"),
+ "--exclude-existing", "--verify", "--exists");
if (exclude_existing_opts.enabled)
return cmd_show_ref__exclude_existing(&exclude_existing_opts);
else if (verify)
return cmd_show_ref__verify(&show_one_opts, argv);
+ else if (exists)
+ return cmd_show_ref__exists(argv);
else
return cmd_show_ref__patterns(&patterns_opts, &show_one_opts, argv);
}
diff --git a/t/t1403-show-ref.sh b/t/t1403-show-ref.sh
index 4a90a88e05d..b50ae6fcf11 100755
--- a/t/t1403-show-ref.sh
+++ b/t/t1403-show-ref.sh
@@ -197,8 +197,73 @@ test_expect_success 'show-ref --verify with dangling ref' '
'
test_expect_success 'show-ref sub-modes are mutually exclusive' '
+ cat >expect <<-EOF &&
+ fatal: only one of ${SQ}--exclude-existing${SQ}, ${SQ}--verify${SQ} or ${SQ}--exists${SQ} can be given
+ EOF
+
test_must_fail git show-ref --verify --exclude-existing 2>err &&
- grep "only one of ${SQ}--exclude-existing${SQ} or ${SQ}--verify${SQ} can be given" err
+ test_cmp expect err &&
+
+ test_must_fail git show-ref --verify --exists 2>err &&
+ test_cmp expect err &&
+
+ test_must_fail git show-ref --exclude-existing --exists 2>err &&
+ test_cmp expect err
+'
+
+test_expect_success '--exists with existing reference' '
+ git show-ref --exists refs/heads/$GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
+'
+
+test_expect_success '--exists with missing reference' '
+ test_expect_code 2 git show-ref --exists refs/heads/does-not-exist
+'
+
+test_expect_success '--exists does not use DWIM' '
+ test_expect_code 2 git show-ref --exists $GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME 2>err &&
+ grep "reference does not exist" err
+'
+
+test_expect_success '--exists with HEAD' '
+ git show-ref --exists HEAD
+'
+
+test_expect_success '--exists with bad reference name' '
+ test_when_finished "git update-ref -d refs/heads/bad...name" &&
+ new_oid=$(git rev-parse HEAD) &&
+ test-tool ref-store main update-ref msg refs/heads/bad...name $new_oid $ZERO_OID REF_SKIP_REFNAME_VERIFICATION &&
+ git show-ref --exists refs/heads/bad...name
+'
+
+test_expect_success '--exists with arbitrary symref' '
+ test_when_finished "git symbolic-ref -d refs/symref" &&
+ git symbolic-ref refs/symref refs/heads/$GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME &&
+ git show-ref --exists refs/symref
+'
+
+test_expect_success '--exists with dangling symref' '
+ test_when_finished "git symbolic-ref -d refs/heads/dangling" &&
+ git symbolic-ref refs/heads/dangling refs/heads/does-not-exist &&
+ git show-ref --exists refs/heads/dangling
+'
+
+test_expect_success '--exists with nonexistent object ID' '
+ test-tool ref-store main update-ref msg refs/heads/missing-oid $(test_oid 001) $ZERO_OID REF_SKIP_OID_VERIFICATION &&
+ git show-ref --exists refs/heads/missing-oid
+'
+
+test_expect_success '--exists with non-commit object' '
+ tree_oid=$(git rev-parse HEAD^{tree}) &&
+ test-tool ref-store main update-ref msg refs/heads/tree ${tree_oid} $ZERO_OID REF_SKIP_OID_VERIFICATION &&
+ git show-ref --exists refs/heads/tree
+'
+
+test_expect_success '--exists with directory fails with generic error' '
+ cat >expect <<-EOF &&
+ error: failed to look up reference: Is a directory
+ EOF
+ test_expect_code 1 git show-ref --exists refs/heads 2>err &&
+ test_cmp expect err
'
test_done
--
2.42.0
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* [PATCH v2 12/12] t: use git-show-ref(1) to check for ref existence
From: Patrick Steinhardt @ 2023-10-26 9:57 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Eric Sunshine, Han-Wen Nienhuys
In-Reply-To: <cover.1698314128.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 14354 bytes --]
Convert tests that use `test_path_is_file` and `test_path_is_missing` to
instead use a set of helpers `test_ref_exists` and `test_ref_missing`.
These helpers are implemented via the newly introduced `git show-ref
--exists` command. Thus, we can avoid intimate knowledge of how the ref
backend stores references on disk.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
t/t1430-bad-ref-name.sh | 27 +++++++++++++-------
t/t3200-branch.sh | 33 ++++++++++++++-----------
t/t5521-pull-options.sh | 4 +--
t/t5605-clone-local.sh | 2 +-
t/test-lib-functions.sh | 55 +++++++++++++++++++++++++++++++++++++++++
5 files changed, 94 insertions(+), 27 deletions(-)
diff --git a/t/t1430-bad-ref-name.sh b/t/t1430-bad-ref-name.sh
index ff1c967d550..7b7d6953c62 100755
--- a/t/t1430-bad-ref-name.sh
+++ b/t/t1430-bad-ref-name.sh
@@ -205,8 +205,9 @@ test_expect_success 'update-ref --no-deref -d can delete symref to broken name'
test_when_finished "test-tool ref-store main delete-refs REF_NO_DEREF msg refs/heads/broken...ref" &&
test-tool ref-store main create-symref refs/heads/badname refs/heads/broken...ref msg &&
test_when_finished "test-tool ref-store main delete-refs REF_NO_DEREF msg refs/heads/badname" &&
+ test_ref_exists refs/heads/badname &&
git update-ref --no-deref -d refs/heads/badname >output 2>error &&
- test_path_is_missing .git/refs/heads/badname &&
+ test_ref_missing refs/heads/badname &&
test_must_be_empty output &&
test_must_be_empty error
'
@@ -216,8 +217,9 @@ test_expect_success 'branch -d can delete symref to broken name' '
test_when_finished "test-tool ref-store main delete-refs REF_NO_DEREF msg refs/heads/broken...ref" &&
test-tool ref-store main create-symref refs/heads/badname refs/heads/broken...ref msg &&
test_when_finished "test-tool ref-store main delete-refs REF_NO_DEREF msg refs/heads/badname" &&
+ test_ref_exists refs/heads/badname &&
git branch -d badname >output 2>error &&
- test_path_is_missing .git/refs/heads/badname &&
+ test_ref_missing refs/heads/badname &&
test_i18ngrep "Deleted branch badname (was refs/heads/broken\.\.\.ref)" output &&
test_must_be_empty error
'
@@ -225,8 +227,9 @@ test_expect_success 'branch -d can delete symref to broken name' '
test_expect_success 'update-ref --no-deref -d can delete dangling symref to broken name' '
test-tool ref-store main create-symref refs/heads/badname refs/heads/broken...ref msg &&
test_when_finished "test-tool ref-store main delete-refs REF_NO_DEREF msg refs/heads/badname" &&
+ test_ref_exists refs/heads/badname &&
git update-ref --no-deref -d refs/heads/badname >output 2>error &&
- test_path_is_missing .git/refs/heads/badname &&
+ test_ref_missing refs/heads/badname &&
test_must_be_empty output &&
test_must_be_empty error
'
@@ -234,8 +237,9 @@ test_expect_success 'update-ref --no-deref -d can delete dangling symref to brok
test_expect_success 'branch -d can delete dangling symref to broken name' '
test-tool ref-store main create-symref refs/heads/badname refs/heads/broken...ref msg &&
test_when_finished "test-tool ref-store main delete-refs REF_NO_DEREF msg refs/heads/badname" &&
+ test_ref_exists refs/heads/badname &&
git branch -d badname >output 2>error &&
- test_path_is_missing .git/refs/heads/badname &&
+ test_ref_missing refs/heads/badname &&
test_i18ngrep "Deleted branch badname (was refs/heads/broken\.\.\.ref)" output &&
test_must_be_empty error
'
@@ -245,8 +249,9 @@ test_expect_success 'update-ref -d can delete broken name through symref' '
test_when_finished "test-tool ref-store main delete-refs REF_NO_DEREF msg refs/heads/broken...ref" &&
test-tool ref-store main create-symref refs/heads/badname refs/heads/broken...ref msg &&
test_when_finished "test-tool ref-store main delete-refs REF_NO_DEREF msg refs/heads/badname" &&
+ test_ref_exists refs/heads/broken...ref &&
git update-ref -d refs/heads/badname >output 2>error &&
- test_path_is_missing .git/refs/heads/broken...ref &&
+ test_ref_missing refs/heads/broken...ref &&
test_must_be_empty output &&
test_must_be_empty error
'
@@ -254,8 +259,9 @@ test_expect_success 'update-ref -d can delete broken name through symref' '
test_expect_success 'update-ref --no-deref -d can delete symref with broken name' '
printf "ref: refs/heads/main\n" >.git/refs/heads/broken...symref &&
test_when_finished "test-tool ref-store main delete-refs REF_NO_DEREF msg refs/heads/broken...symref" &&
+ test_ref_exists refs/heads/broken...symref &&
git update-ref --no-deref -d refs/heads/broken...symref >output 2>error &&
- test_path_is_missing .git/refs/heads/broken...symref &&
+ test_ref_missing refs/heads/broken...symref &&
test_must_be_empty output &&
test_must_be_empty error
'
@@ -263,8 +269,9 @@ test_expect_success 'update-ref --no-deref -d can delete symref with broken name
test_expect_success 'branch -d can delete symref with broken name' '
printf "ref: refs/heads/main\n" >.git/refs/heads/broken...symref &&
test_when_finished "test-tool ref-store main delete-refs REF_NO_DEREF msg refs/heads/broken...symref" &&
+ test_ref_exists refs/heads/broken...symref &&
git branch -d broken...symref >output 2>error &&
- test_path_is_missing .git/refs/heads/broken...symref &&
+ test_ref_missing refs/heads/broken...symref &&
test_i18ngrep "Deleted branch broken...symref (was refs/heads/main)" output &&
test_must_be_empty error
'
@@ -272,8 +279,9 @@ test_expect_success 'branch -d can delete symref with broken name' '
test_expect_success 'update-ref --no-deref -d can delete dangling symref with broken name' '
printf "ref: refs/heads/idonotexist\n" >.git/refs/heads/broken...symref &&
test_when_finished "test-tool ref-store main delete-refs REF_NO_DEREF msg refs/heads/broken...symref" &&
+ test_ref_exists refs/heads/broken...symref &&
git update-ref --no-deref -d refs/heads/broken...symref >output 2>error &&
- test_path_is_missing .git/refs/heads/broken...symref &&
+ test_ref_missing refs/heads/broken...symref &&
test_must_be_empty output &&
test_must_be_empty error
'
@@ -281,8 +289,9 @@ test_expect_success 'update-ref --no-deref -d can delete dangling symref with br
test_expect_success 'branch -d can delete dangling symref with broken name' '
printf "ref: refs/heads/idonotexist\n" >.git/refs/heads/broken...symref &&
test_when_finished "test-tool ref-store main delete-refs REF_NO_DEREF msg refs/heads/broken...symref" &&
+ test_ref_exists refs/heads/broken...symref &&
git branch -d broken...symref >output 2>error &&
- test_path_is_missing .git/refs/heads/broken...symref &&
+ test_ref_missing refs/heads/broken...symref &&
test_i18ngrep "Deleted branch broken...symref (was refs/heads/idonotexist)" output &&
test_must_be_empty error
'
diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
index 080e4f24a6e..bde4f1485b7 100755
--- a/t/t3200-branch.sh
+++ b/t/t3200-branch.sh
@@ -25,7 +25,7 @@ test_expect_success 'prepare a trivial repository' '
test_expect_success 'git branch --help should not have created a bogus branch' '
test_might_fail git branch --man --help </dev/null >/dev/null 2>&1 &&
- test_path_is_missing .git/refs/heads/--help
+ test_ref_missing refs/heads/--help
'
test_expect_success 'branch -h in broken repository' '
@@ -40,7 +40,8 @@ test_expect_success 'branch -h in broken repository' '
'
test_expect_success 'git branch abc should create a branch' '
- git branch abc && test_path_is_file .git/refs/heads/abc
+ git branch abc &&
+ test_ref_exists refs/heads/abc
'
test_expect_success 'git branch abc should fail when abc exists' '
@@ -61,11 +62,13 @@ test_expect_success 'git branch --force abc should succeed when abc exists' '
'
test_expect_success 'git branch a/b/c should create a branch' '
- git branch a/b/c && test_path_is_file .git/refs/heads/a/b/c
+ git branch a/b/c &&
+ test_ref_exists refs/heads/a/b/c
'
test_expect_success 'git branch mb main... should create a branch' '
- git branch mb main... && test_path_is_file .git/refs/heads/mb
+ git branch mb main... &&
+ test_ref_exists refs/heads/mb
'
test_expect_success 'git branch HEAD should fail' '
@@ -78,14 +81,14 @@ EOF
test_expect_success 'git branch --create-reflog d/e/f should create a branch and a log' '
GIT_COMMITTER_DATE="2005-05-26 23:30" \
git -c core.logallrefupdates=false branch --create-reflog d/e/f &&
- test_path_is_file .git/refs/heads/d/e/f &&
+ test_ref_exists refs/heads/d/e/f &&
test_path_is_file .git/logs/refs/heads/d/e/f &&
test_cmp expect .git/logs/refs/heads/d/e/f
'
test_expect_success 'git branch -d d/e/f should delete a branch and a log' '
git branch -d d/e/f &&
- test_path_is_missing .git/refs/heads/d/e/f &&
+ test_ref_missing refs/heads/d/e/f &&
test_must_fail git reflog exists refs/heads/d/e/f
'
@@ -213,7 +216,7 @@ test_expect_success 'git branch -M should leave orphaned HEAD alone' '
test_commit initial &&
git checkout --orphan lonely &&
grep lonely .git/HEAD &&
- test_path_is_missing .git/refs/head/lonely &&
+ test_ref_missing refs/head/lonely &&
git branch -M main mistress &&
grep lonely .git/HEAD
)
@@ -799,8 +802,8 @@ test_expect_success 'deleting a symref' '
git symbolic-ref refs/heads/symref refs/heads/target &&
echo "Deleted branch symref (was refs/heads/target)." >expect &&
git branch -d symref >actual &&
- test_path_is_file .git/refs/heads/target &&
- test_path_is_missing .git/refs/heads/symref &&
+ test_ref_exists refs/heads/target &&
+ test_ref_missing refs/heads/symref &&
test_cmp expect actual
'
@@ -809,16 +812,16 @@ test_expect_success 'deleting a dangling symref' '
test_path_is_file .git/refs/heads/dangling-symref &&
echo "Deleted branch dangling-symref (was nowhere)." >expect &&
git branch -d dangling-symref >actual &&
- test_path_is_missing .git/refs/heads/dangling-symref &&
+ test_ref_missing refs/heads/dangling-symref &&
test_cmp expect actual
'
test_expect_success 'deleting a self-referential symref' '
git symbolic-ref refs/heads/self-reference refs/heads/self-reference &&
- test_path_is_file .git/refs/heads/self-reference &&
+ test_ref_exists refs/heads/self-reference &&
echo "Deleted branch self-reference (was refs/heads/self-reference)." >expect &&
git branch -d self-reference >actual &&
- test_path_is_missing .git/refs/heads/self-reference &&
+ test_ref_missing refs/heads/self-reference &&
test_cmp expect actual
'
@@ -826,8 +829,8 @@ test_expect_success 'renaming a symref is not allowed' '
git symbolic-ref refs/heads/topic refs/heads/main &&
test_must_fail git branch -m topic new-topic &&
git symbolic-ref refs/heads/topic &&
- test_path_is_file .git/refs/heads/main &&
- test_path_is_missing .git/refs/heads/new-topic
+ test_ref_exists refs/heads/main &&
+ test_ref_missing refs/heads/new-topic
'
test_expect_success SYMLINKS 'git branch -m u v should fail when the reflog for u is a symlink' '
@@ -1142,7 +1145,7 @@ EOF
test_expect_success 'git checkout -b g/h/i -l should create a branch and a log' '
GIT_COMMITTER_DATE="2005-05-26 23:30" \
git checkout -b g/h/i -l main &&
- test_path_is_file .git/refs/heads/g/h/i &&
+ test_ref_exists refs/heads/g/h/i &&
test_path_is_file .git/logs/refs/heads/g/h/i &&
test_cmp expect .git/logs/refs/heads/g/h/i
'
diff --git a/t/t5521-pull-options.sh b/t/t5521-pull-options.sh
index 079b2f2536e..3681859f983 100755
--- a/t/t5521-pull-options.sh
+++ b/t/t5521-pull-options.sh
@@ -143,7 +143,7 @@ test_expect_success 'git pull --dry-run' '
cd clonedry &&
git pull --dry-run ../parent &&
test_path_is_missing .git/FETCH_HEAD &&
- test_path_is_missing .git/refs/heads/main &&
+ test_ref_missing refs/heads/main &&
test_path_is_missing .git/index &&
test_path_is_missing file
)
@@ -157,7 +157,7 @@ test_expect_success 'git pull --all --dry-run' '
git remote add origin ../parent &&
git pull --all --dry-run &&
test_path_is_missing .git/FETCH_HEAD &&
- test_path_is_missing .git/refs/remotes/origin/main &&
+ test_ref_missing refs/remotes/origin/main &&
test_path_is_missing .git/index &&
test_path_is_missing file
)
diff --git a/t/t5605-clone-local.sh b/t/t5605-clone-local.sh
index 1d7b1abda1a..946c5751885 100755
--- a/t/t5605-clone-local.sh
+++ b/t/t5605-clone-local.sh
@@ -69,7 +69,7 @@ test_expect_success 'local clone of repo with nonexistent ref in HEAD' '
git clone a d &&
(cd d &&
git fetch &&
- test ! -e .git/refs/remotes/origin/HEAD)
+ test_ref_missing refs/remotes/origin/HEAD)
'
test_expect_success 'bundle clone without .bundle suffix' '
diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index 2f8868caa17..56b33536ed1 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -251,6 +251,61 @@ debug () {
done
}
+# Usage: test_ref_exists [options] <ref>
+#
+# -C <dir>:
+# Run all git commands in directory <dir>
+#
+# This helper function checks whether a reference exists. Symrefs or object IDs
+# will not be resolved. Can be used to check references with bad names.
+test_ref_exists () {
+ local indir=
+
+ while test $# != 0
+ do
+ case "$1" in
+ -C)
+ indir="$2"
+ shift
+ ;;
+ *)
+ break
+ ;;
+ esac
+ shift
+ done &&
+
+ indir=${indir:+"$indir"/} &&
+
+ if test "$#" != 1
+ then
+ BUG "expected exactly one reference"
+ fi &&
+
+ git ${indir:+ -C "$indir"} show-ref --exists "$1"
+}
+
+# Behaves the same as test_ref_exists, except that it checks for the absence of
+# a reference. This is preferable to `! test_ref_exists` as this function is
+# able to distinguish actually-missing references from other, generic errors.
+test_ref_missing () {
+ test_ref_exists "$@"
+ case "$?" in
+ 2)
+ # This is the good case.
+ return 0
+ ;;
+ 0)
+ echo >&4 "test_ref_missing: reference exists"
+ return 1
+ ;;
+ *)
+ echo >&4 "test_ref_missing: generic error"
+ return 1
+ ;;
+ esac
+}
+
# Usage: test_commit [options] <message> [<file> [<contents> [<tag>]]]
# -C <dir>:
# Run all git commands in directory <dir>
--
2.42.0
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* [PATCH v5 0/3] rev-list: add support for commits in `--missing`
From: Karthik Nayak @ 2023-10-26 10:11 UTC (permalink / raw)
To: karthik.188; +Cc: git, gitster, ps
In-Reply-To: <20231024122631.158415-1-karthik.188@gmail.com>
The `--missing` option in git-rev-list(1) was introduced intitally
to deal with missing blobs in the context of promissory notes.
Eventually the option was extended to also support tree objects in
7c0fe330d5 (rev-list: handle missing tree objects properly,2018-10-05).
This patch series extends the `--missing` option to also add support for
commit objects. We do this by introducing a new flag `MISSING` which is
added whenever we encounter a missing commit during traversal. Then in
`builtin/rev-list` we check for this flag and take the appropriate
action based on the `--missing=*` option used.
This series is an alternate to the patch series I had posted earlier:
https://lore.kernel.org/git/20230908174208.249184-1-karthik.188@gmail.com/.
In that patch, we introduced an option `--ignore-missing-links` which
was added to expose the `ignore_missing_links` bit to the user. The
issue in that patch was that, the option `--ignore-missing-links` didn't
play well the pre-existing `--missing` option. This series avoids that
route and just extends the `--missing` option for commits to solve the
same problem.
V4 of the series can be found here: https://lore.kernel.org/git/20231024122631.158415-1-karthik.188@gmail.com/T/#ma7f07bc637e20e2a9558b23e8081957af61f63ce
Changes from v4:
- Rename `missing_objects` to `missing_commits` since we only deal
with commits.
- Fix incorrect indentation
- clear oidset after traversal in `release_revisions()`
Range diff:
1: 8c469cf479 = 1: 8c469cf479 revision: rename bit to `do_not_die_on_missing_objects`
2: 76ce43d973 = 2: 76ce43d973 rev-list: move `show_commit()` to the bottom
3: d892f0b82d ! 3: 6f0c74f888 rev-list: add commit object support in `--missing` option
@@ Commit message
between the main and alternate object directory.
Helped-by: Patrick Steinhardt <ps@pks.im>
+ Helped-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
## builtin/rev-list.c ##
@@ builtin/rev-list.c: static void show_commit(struct commit *commit, void *data)
display_progress(progress, ++progress_counter);
+ if (revs->do_not_die_on_missing_objects &&
-+ oidset_contains(&revs->missing_objects, &commit->object.oid)) {
++ oidset_contains(&revs->missing_commits, &commit->object.oid)) {
+ finish_object__ma(&commit->object);
+ return;
+ }
@@ list-objects.c: static void do_traverse(struct traversal_context *ctx)
if (!ctx->revs->tree_objects)
; /* do not bother loading tree */
+ else if (ctx->revs->do_not_die_on_missing_objects &&
-+ oidset_contains(&ctx->revs->missing_objects, &commit->object.oid))
++ oidset_contains(&ctx->revs->missing_commits, &commit->object.oid))
+ ;
else if (repo_get_commit_tree(the_repository, commit)) {
struct tree *tree = repo_get_commit_tree(the_repository,
@@ revision.c: static int process_parents(struct rev_info *revs, struct commit *com
if (commit->object.flags & ADDED)
return 0;
+ if (revs->do_not_die_on_missing_objects &&
-+ oidset_contains(&revs->missing_objects, &commit->object.oid))
++ oidset_contains(&revs->missing_commits, &commit->object.oid))
+ return 0;
commit->object.flags |= ADDED;
@@ revision.c: static int process_parents(struct rev_info *revs, struct commit *com
}
- return -1;
+
-+ if (!revs->do_not_die_on_missing_objects)
-+ return -1;
++ if (revs->do_not_die_on_missing_objects)
++ oidset_insert(&revs->missing_commits, &p->object.oid);
+ else
-+ oidset_insert(&revs->missing_objects, &p->object.oid);
++ return -1; /* corrupt repository */
}
if (revs->sources) {
char **slot = revision_sources_at(revs->sources, p);
+@@ revision.c: void release_revisions(struct rev_info *revs)
+ clear_decoration(&revs->merge_simplification, free);
+ clear_decoration(&revs->treesame, free);
+ line_log_free(revs);
++ oidset_clear(&revs->missing_commits);
+ }
+
+ static void add_child(struct rev_info *revs, struct commit *parent, struct commit *child)
@@ revision.c: int prepare_revision_walk(struct rev_info *revs)
FOR_EACH_OBJECT_PROMISOR_ONLY);
}
+ if (revs->do_not_die_on_missing_objects)
-+ oidset_init(&revs->missing_objects, 0);
++ oidset_init(&revs->missing_commits, 0);
+
if (!revs->reflog_info)
prepare_to_use_bloom_filter(revs);
@@ revision.h: struct rev_info {
/* Location where temporary objects for remerge-diff are written. */
struct tmp_objdir *remerge_objdir;
+
-+ /* Missing objects to be tracked without failing traversal. */
-+ struct oidset missing_objects;
++ /* Missing commits to be tracked without failing traversal. */
++ struct oidset missing_commits;
};
/**
Karthik Nayak (3):
revision: rename bit to `do_not_die_on_missing_objects`
rev-list: move `show_commit()` to the bottom
rev-list: add commit object support in `--missing` option
builtin/reflog.c | 2 +-
builtin/rev-list.c | 93 +++++++++++++++++++------------------
list-objects.c | 5 +-
revision.c | 17 ++++++-
revision.h | 21 +++++----
t/t6022-rev-list-missing.sh | 74 +++++++++++++++++++++++++++++
6 files changed, 156 insertions(+), 56 deletions(-)
create mode 100755 t/t6022-rev-list-missing.sh
--
2.42.0
^ permalink raw reply
* [PATCH v5 1/3] revision: rename bit to `do_not_die_on_missing_objects`
From: Karthik Nayak @ 2023-10-26 10:11 UTC (permalink / raw)
To: karthik.188; +Cc: git, gitster, ps
In-Reply-To: <20231026101109.43110-1-karthik.188@gmail.com>
The bit `do_not_die_on_missing_tree` is used in revision.h to ensure the
revision walker does not die when encountering a missing tree. This is
currently exclusively set within `builtin/rev-list.c` to ensure the
`--missing` option works with missing trees.
In the upcoming commits, we will extend `--missing` to also support
missing commits. So let's rename the bit to
`do_not_die_on_missing_objects`, which is object type agnostic and can
be used for both trees/commits.
Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
builtin/reflog.c | 2 +-
builtin/rev-list.c | 2 +-
list-objects.c | 2 +-
revision.h | 17 +++++++++--------
4 files changed, 12 insertions(+), 11 deletions(-)
diff --git a/builtin/reflog.c b/builtin/reflog.c
index df63a5892e..9e369a5977 100644
--- a/builtin/reflog.c
+++ b/builtin/reflog.c
@@ -298,7 +298,7 @@ static int cmd_reflog_expire(int argc, const char **argv, const char *prefix)
struct rev_info revs;
repo_init_revisions(the_repository, &revs, prefix);
- revs.do_not_die_on_missing_tree = 1;
+ revs.do_not_die_on_missing_objects = 1;
revs.ignore_missing = 1;
revs.ignore_missing_links = 1;
if (verbose)
diff --git a/builtin/rev-list.c b/builtin/rev-list.c
index ff715d6918..ea77489c38 100644
--- a/builtin/rev-list.c
+++ b/builtin/rev-list.c
@@ -561,7 +561,7 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix)
}
if (arg_missing_action)
- revs.do_not_die_on_missing_tree = 1;
+ revs.do_not_die_on_missing_objects = 1;
argc = setup_revisions(argc, argv, &revs, &s_r_opt);
diff --git a/list-objects.c b/list-objects.c
index c25c72b32c..47296dff2f 100644
--- a/list-objects.c
+++ b/list-objects.c
@@ -177,7 +177,7 @@ static void process_tree(struct traversal_context *ctx,
is_promisor_object(&obj->oid))
return;
- if (!revs->do_not_die_on_missing_tree)
+ if (!revs->do_not_die_on_missing_objects)
die("bad tree object %s", oid_to_hex(&obj->oid));
}
diff --git a/revision.h b/revision.h
index 50091bbd13..c73c92ef40 100644
--- a/revision.h
+++ b/revision.h
@@ -212,18 +212,19 @@ struct rev_info {
/*
* Blobs are shown without regard for their existence.
- * But not so for trees: unless exclude_promisor_objects
+ * But not so for trees/commits: unless exclude_promisor_objects
* is set and the tree in question is a promisor object;
* OR ignore_missing_links is set, the revision walker
- * dies with a "bad tree object HASH" message when
- * encountering a missing tree. For callers that can
- * handle missing trees and want them to be filterable
+ * dies with a "bad <type> object HASH" message when
+ * encountering a missing object. For callers that can
+ * handle missing trees/commits and want them to be filterable
* and showable, set this to true. The revision walker
- * will filter and show such a missing tree as usual,
- * but will not attempt to recurse into this tree
- * object.
+ * will filter and show such a missing object as usual,
+ * but will not attempt to recurse into this tree/commit
+ * object. The revision walker will also set the MISSING
+ * flag for such objects.
*/
- do_not_die_on_missing_tree:1,
+ do_not_die_on_missing_objects:1,
/* for internal use only */
exclude_promisor_objects:1;
--
2.42.0
^ permalink raw reply related
* [PATCH v5 2/3] rev-list: move `show_commit()` to the bottom
From: Karthik Nayak @ 2023-10-26 10:11 UTC (permalink / raw)
To: karthik.188; +Cc: git, gitster, ps
In-Reply-To: <20231026101109.43110-1-karthik.188@gmail.com>
The `show_commit()` function already depends on `finish_commit()`, and
in the upcoming commit, we'll also add a dependency on
`finish_object__ma()`. Since in C symbols must be declared before
they're used, let's move `show_commit()` below both `finish_commit()`
and `finish_object__ma()`, so the code is cleaner as a whole without the
need for declarations.
Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
builtin/rev-list.c | 85 +++++++++++++++++++++++-----------------------
1 file changed, 42 insertions(+), 43 deletions(-)
diff --git a/builtin/rev-list.c b/builtin/rev-list.c
index ea77489c38..98542e8b3c 100644
--- a/builtin/rev-list.c
+++ b/builtin/rev-list.c
@@ -100,7 +100,48 @@ static off_t get_object_disk_usage(struct object *obj)
return size;
}
-static void finish_commit(struct commit *commit);
+static inline void finish_object__ma(struct object *obj)
+{
+ /*
+ * Whether or not we try to dynamically fetch missing objects
+ * from the server, we currently DO NOT have the object. We
+ * can either print, allow (ignore), or conditionally allow
+ * (ignore) them.
+ */
+ switch (arg_missing_action) {
+ case MA_ERROR:
+ die("missing %s object '%s'",
+ type_name(obj->type), oid_to_hex(&obj->oid));
+ return;
+
+ case MA_ALLOW_ANY:
+ return;
+
+ case MA_PRINT:
+ oidset_insert(&missing_objects, &obj->oid);
+ return;
+
+ case MA_ALLOW_PROMISOR:
+ if (is_promisor_object(&obj->oid))
+ return;
+ die("unexpected missing %s object '%s'",
+ type_name(obj->type), oid_to_hex(&obj->oid));
+ return;
+
+ default:
+ BUG("unhandled missing_action");
+ return;
+ }
+}
+
+static void finish_commit(struct commit *commit)
+{
+ free_commit_list(commit->parents);
+ commit->parents = NULL;
+ free_commit_buffer(the_repository->parsed_objects,
+ commit);
+}
+
static void show_commit(struct commit *commit, void *data)
{
struct rev_list_info *info = data;
@@ -219,48 +260,6 @@ static void show_commit(struct commit *commit, void *data)
finish_commit(commit);
}
-static void finish_commit(struct commit *commit)
-{
- free_commit_list(commit->parents);
- commit->parents = NULL;
- free_commit_buffer(the_repository->parsed_objects,
- commit);
-}
-
-static inline void finish_object__ma(struct object *obj)
-{
- /*
- * Whether or not we try to dynamically fetch missing objects
- * from the server, we currently DO NOT have the object. We
- * can either print, allow (ignore), or conditionally allow
- * (ignore) them.
- */
- switch (arg_missing_action) {
- case MA_ERROR:
- die("missing %s object '%s'",
- type_name(obj->type), oid_to_hex(&obj->oid));
- return;
-
- case MA_ALLOW_ANY:
- return;
-
- case MA_PRINT:
- oidset_insert(&missing_objects, &obj->oid);
- return;
-
- case MA_ALLOW_PROMISOR:
- if (is_promisor_object(&obj->oid))
- return;
- die("unexpected missing %s object '%s'",
- type_name(obj->type), oid_to_hex(&obj->oid));
- return;
-
- default:
- BUG("unhandled missing_action");
- return;
- }
-}
-
static int finish_object(struct object *obj, const char *name UNUSED,
void *cb_data)
{
--
2.42.0
^ permalink raw reply related
* [PATCH v5 3/3] rev-list: add commit object support in `--missing` option
From: Karthik Nayak @ 2023-10-26 10:11 UTC (permalink / raw)
To: karthik.188; +Cc: git, gitster, ps
In-Reply-To: <20231026101109.43110-1-karthik.188@gmail.com>
The `--missing` object option in rev-list currently works only with
missing blobs/trees. For missing commits the revision walker fails with
a fatal error.
Let's extend the functionality of `--missing` option to also support
commit objects. This is done by adding a `missing_objects` field to
`rev_info`. This field is an `oidset` to which we'll add the missing
commits as we encounter them. The revision walker will now continue the
traversal and call `show_commit()` even for missing commits. In rev-list
we can then check if the commit is a missing commit and call the
existing code for parsing `--missing` objects.
A scenario where this option would be used is to find the boundary
objects between different object directories. Consider a repository with
a main object directory (GIT_OBJECT_DIRECTORY) and one or more alternate
object directories (GIT_ALTERNATE_OBJECT_DIRECTORIES). In such a
repository, using the `--missing=print` option while disabling the
alternate object directory allows us to find the boundary objects
between the main and alternate object directory.
Helped-by: Patrick Steinhardt <ps@pks.im>
Helped-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
builtin/rev-list.c | 6 +++
list-objects.c | 3 ++
revision.c | 17 ++++++++-
revision.h | 4 ++
t/t6022-rev-list-missing.sh | 74 +++++++++++++++++++++++++++++++++++++
5 files changed, 102 insertions(+), 2 deletions(-)
create mode 100755 t/t6022-rev-list-missing.sh
diff --git a/builtin/rev-list.c b/builtin/rev-list.c
index 98542e8b3c..181353dcf5 100644
--- a/builtin/rev-list.c
+++ b/builtin/rev-list.c
@@ -149,6 +149,12 @@ static void show_commit(struct commit *commit, void *data)
display_progress(progress, ++progress_counter);
+ if (revs->do_not_die_on_missing_objects &&
+ oidset_contains(&revs->missing_commits, &commit->object.oid)) {
+ finish_object__ma(&commit->object);
+ return;
+ }
+
if (show_disk_usage)
total_disk_usage += get_object_disk_usage(&commit->object);
diff --git a/list-objects.c b/list-objects.c
index 47296dff2f..f4e1104b56 100644
--- a/list-objects.c
+++ b/list-objects.c
@@ -389,6 +389,9 @@ static void do_traverse(struct traversal_context *ctx)
*/
if (!ctx->revs->tree_objects)
; /* do not bother loading tree */
+ else if (ctx->revs->do_not_die_on_missing_objects &&
+ oidset_contains(&ctx->revs->missing_commits, &commit->object.oid))
+ ;
else if (repo_get_commit_tree(the_repository, commit)) {
struct tree *tree = repo_get_commit_tree(the_repository,
commit);
diff --git a/revision.c b/revision.c
index 219dc76716..738bacad08 100644
--- a/revision.c
+++ b/revision.c
@@ -6,6 +6,7 @@
#include "object-name.h"
#include "object-file.h"
#include "object-store-ll.h"
+#include "oidset.h"
#include "tag.h"
#include "blob.h"
#include "tree.h"
@@ -1112,6 +1113,9 @@ static int process_parents(struct rev_info *revs, struct commit *commit,
if (commit->object.flags & ADDED)
return 0;
+ if (revs->do_not_die_on_missing_objects &&
+ oidset_contains(&revs->missing_commits, &commit->object.oid))
+ return 0;
commit->object.flags |= ADDED;
if (revs->include_check &&
@@ -1168,7 +1172,8 @@ static int process_parents(struct rev_info *revs, struct commit *commit,
for (parent = commit->parents; parent; parent = parent->next) {
struct commit *p = parent->item;
int gently = revs->ignore_missing_links ||
- revs->exclude_promisor_objects;
+ revs->exclude_promisor_objects ||
+ revs->do_not_die_on_missing_objects;
if (repo_parse_commit_gently(revs->repo, p, gently) < 0) {
if (revs->exclude_promisor_objects &&
is_promisor_object(&p->object.oid)) {
@@ -1176,7 +1181,11 @@ static int process_parents(struct rev_info *revs, struct commit *commit,
break;
continue;
}
- return -1;
+
+ if (revs->do_not_die_on_missing_objects)
+ oidset_insert(&revs->missing_commits, &p->object.oid);
+ else
+ return -1; /* corrupt repository */
}
if (revs->sources) {
char **slot = revision_sources_at(revs->sources, p);
@@ -3109,6 +3118,7 @@ void release_revisions(struct rev_info *revs)
clear_decoration(&revs->merge_simplification, free);
clear_decoration(&revs->treesame, free);
line_log_free(revs);
+ oidset_clear(&revs->missing_commits);
}
static void add_child(struct rev_info *revs, struct commit *parent, struct commit *child)
@@ -3800,6 +3810,9 @@ int prepare_revision_walk(struct rev_info *revs)
FOR_EACH_OBJECT_PROMISOR_ONLY);
}
+ if (revs->do_not_die_on_missing_objects)
+ oidset_init(&revs->missing_commits, 0);
+
if (!revs->reflog_info)
prepare_to_use_bloom_filter(revs);
if (!revs->unsorted_input)
diff --git a/revision.h b/revision.h
index c73c92ef40..94c43138bc 100644
--- a/revision.h
+++ b/revision.h
@@ -4,6 +4,7 @@
#include "commit.h"
#include "grep.h"
#include "notes.h"
+#include "oidset.h"
#include "pretty.h"
#include "diff.h"
#include "commit-slab-decl.h"
@@ -373,6 +374,9 @@ struct rev_info {
/* Location where temporary objects for remerge-diff are written. */
struct tmp_objdir *remerge_objdir;
+
+ /* Missing commits to be tracked without failing traversal. */
+ struct oidset missing_commits;
};
/**
diff --git a/t/t6022-rev-list-missing.sh b/t/t6022-rev-list-missing.sh
new file mode 100755
index 0000000000..40265a4f66
--- /dev/null
+++ b/t/t6022-rev-list-missing.sh
@@ -0,0 +1,74 @@
+#!/bin/sh
+
+test_description='handling of missing objects in rev-list'
+
+TEST_PASSES_SANITIZE_LEAK=true
+. ./test-lib.sh
+
+# We setup the repository with two commits, this way HEAD is always
+# available and we can hide commit 1.
+test_expect_success 'create repository and alternate directory' '
+ test_commit 1 &&
+ test_commit 2 &&
+ test_commit 3
+'
+
+for obj in "HEAD~1" "HEAD~1^{tree}" "HEAD:1.t"
+do
+ test_expect_success "rev-list --missing=error fails with missing object $obj" '
+ oid="$(git rev-parse $obj)" &&
+ path=".git/objects/$(test_oid_to_path $oid)" &&
+
+ mv "$path" "$path.hidden" &&
+ test_when_finished "mv $path.hidden $path" &&
+
+ test_must_fail git rev-list --missing=error --objects \
+ --no-object-names HEAD
+ '
+done
+
+for obj in "HEAD~1" "HEAD~1^{tree}" "HEAD:1.t"
+do
+ for action in "allow-any" "print"
+ do
+ test_expect_success "rev-list --missing=$action with missing $obj" '
+ oid="$(git rev-parse $obj)" &&
+ path=".git/objects/$(test_oid_to_path $oid)" &&
+
+ # Before the object is made missing, we use rev-list to
+ # get the expected oids.
+ git rev-list --objects --no-object-names \
+ HEAD ^$obj >expect.raw &&
+
+ # Blobs are shared by all commits, so evethough a commit/tree
+ # might be skipped, its blob must be accounted for.
+ if [ $obj != "HEAD:1.t" ]; then
+ echo $(git rev-parse HEAD:1.t) >>expect.raw &&
+ echo $(git rev-parse HEAD:2.t) >>expect.raw
+ fi &&
+
+ mv "$path" "$path.hidden" &&
+ test_when_finished "mv $path.hidden $path" &&
+
+ git rev-list --missing=$action --objects --no-object-names \
+ HEAD >actual.raw &&
+
+ # When the action is to print, we should also add the missing
+ # oid to the expect list.
+ case $action in
+ allow-any)
+ ;;
+ print)
+ grep ?$oid actual.raw &&
+ echo ?$oid >>expect.raw
+ ;;
+ esac &&
+
+ sort actual.raw >actual &&
+ sort expect.raw >expect &&
+ test_cmp expect actual
+ '
+ done
+done
+
+test_done
--
2.42.0
^ permalink raw reply related
* git grep --no-index fails when given an absolute path
From: triallax @ 2023-10-26 10:46 UTC (permalink / raw)
To: Git
Thank you for filling out a Git bug report!
Please answer the following questions to help us understand your issue.
What did you do before the bug happened? (Steps to reproduce your issue)
- Run `git grep --no-index -r -- -- /usr/share/fish/completions/`
What did you expect to happen? (Expected behavior)
The command executes the search successfully.
What happened instead? (Actual behavior)
This happens:
triallax@satoru ~> git grep --no-index -r -- -- /usr/share/fish/completions/
BUG: environment.c:213: git environment hasn't been setup
fish: Job 1, 'git grep --no-index -r -- -- /u…' terminated by signal SIGABRT (Abort)
What's different between what you expected and what actually happened?
One of them is an error, and the other is a successful execution.
Anything else you want to add:
Interestingly enough, this seems to be happening only with absolute paths from
my testing, and it happens even when the directory that's passed is a Git repo.
Please review the rest of the bug report below.
You can delete any lines you don't wish to share.
[System Info]
git version:
git version 2.42.0
cpu: x86_64
no commit associated with this build
sizeof-long: 8
sizeof-size_t: 8
shell-path: /bin/sh
uname: Linux 6.5.8_1 #1 SMP PREEMPT_DYNAMIC Fri Oct 20 13:35:41 UTC 2023 x86_64
compiler info: gnuc: 12.2
libc info: glibc: 2.36
$SHELL (typically, interactive shell): /bin/dash
[Enabled Hooks]
not run from a git repository - no hooks to show
^ permalink raw reply
* Re: git diagnose with invalid CLI argument does not report error
From: Bagas Sanjaya @ 2023-10-26 11:10 UTC (permalink / raw)
To: Sheik, Git Mailing List
Cc: Elijah Newren, Calvin Wan, Junio C Hamano,
Ævar Arnfjörð Bjarmason, Alex Henrie,
Derrick Stolee, Victoria Dye, Kristoffer Haugsbakk
In-Reply-To: <849b6ee2-99f3-4aaa-835f-44d3e13befc3@gmail.com>
[-- Attachment #1: Type: text/plain, Size: 772 bytes --]
On Thu, Oct 26, 2023 at 07:49:58AM +1100, Sheik wrote:
> Hi Maintainers,
>
>
> Running git diagnose with an invalid CLI argument in a valid Git directory
> does not report error. Expected behaviour would be that it reports an error.
>
> #Example shell commands which should have reported an error but continues to
> succeed
>
> cd $ToAGitDirectory
>
> git diagnose mod
>
> git diagnose mode
>
> git diagnose mode=all
>
I can reproduce this only when the invalid parameter is a normal word:
```
$ git diagnose huh
```
But the command errors out on invalid flag:
```
$ git diagnose -m
```
Cc:'ing people who recently worked on builtin/diagnose.c for help.
Thanks.
--
An old man doll... just what I always wanted! - Clara
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply
* Re: git grep --no-index fails when given an absolute path
From: Kristoffer Haugsbakk @ 2023-10-26 12:28 UTC (permalink / raw)
To: triallax; +Cc: Git
In-Reply-To: <Nhfa-Qv--F-9@tutanota.com>
Hi
On Thu, Oct 26, 2023, at 12:46, triallax@tutanota.com wrote:
> Thank you for filling out a Git bug report!
> Please answer the following questions to help us understand your issue.
>
> What did you do before the bug happened? (Steps to reproduce your issue)
> - Run `git grep --no-index -r -- -- /usr/share/fish/completions/`
See this thread: https://lore.kernel.org/git/CAKFQ_Q_P4HvCMHsg4=6ycb8r44qprhRCGSmLQf7B3_-zy28_oQ@mail.gmail.com/
> What did you expect to happen? (Expected behavior)
>
> The command executes the search successfully.
>
> What happened instead? (Actual behavior)
>
> This happens:
>
> triallax@satoru ~> git grep --no-index -r -- --
> /usr/share/fish/completions/
> BUG: environment.c:213: git environment hasn't been setup
> fish: Job 1, 'git grep --no-index -r -- -- /u…' terminated by signal
> SIGABRT (Abort)
>
> What's different between what you expected and what actually happened?
>
> One of them is an error, and the other is a successful execution.
>
> Anything else you want to add:
>
> Interestingly enough, this seems to be happening only with absolute paths from
> my testing, and it happens even when the directory that's passed is a Git repo.
What is the working directory that you are in? I’m guessing somewhere in
your home directory?
I’ll try to reproduce your command later with the proposed fix.
Kristoffer
^ permalink raw reply
* Re: [PATCH v4 3/3] rev-list: add commit object support in `--missing` option
From: Junio C Hamano @ 2023-10-26 12:37 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: Karthik Nayak, git
In-Reply-To: <ZTi4Zd1by53q5gtM@tanuki>
Patrick Steinhardt <ps@pks.im> writes:
>> + if (revs->do_not_die_on_missing_objects)
>> + oidset_init(&revs->missing_objects, 0);
>> +
>
> While we're initializing the new oidset, we never clear it. We should
> probably call `oidset_clear()` in `release_revisions()`. And if we
> unconditionally initialized the oidset here then we can also call
> `oiadset_clear()` unconditionally there, which should be preferable
> given that `oidset_init()` does not allocate memory when no initial size
> was given.
Yup, I used the conditional one to match the above, but initializing
unused oidset is cheap and frees us from having to worry about
mistakes. I like your idea much better.
>> +
>> + /* Missing objects to be tracked without failing traversal. */
>> + struct oidset missing_objects;
>
> As far as I can see we only use this set to track missing commits, but
> none of the other objects. The name thus feels a bit misleading to me,
> as a reader might rightfully assume that it contains _all_ missing
> objects after the revwalk. Should we rename it to `missing_commits` to
> clarify?
Again, very good suggestion.
Thanks.
^ permalink raw reply
* Re: Regression: git send-email fails with "Use of uninitialized value $address" + "unable to extract a valid address"
From: Junio C Hamano @ 2023-10-26 12:41 UTC (permalink / raw)
To: Jeff King
Cc: Uwe Kleine-König, Michael Strawbridge, Luben Tuikov, git,
entwicklung
In-Reply-To: <20231025072104.GA2145145@coredump.intra.peff.net>
Jeff King <peff@peff.net> writes:
> Note that the bug will only trigger if Email::Valid is installed.
I recall we chased a different bug that depends on the use/non-use
of this package a few years ago. Is the difference significant
enough that we may want to install on one but not in another CI
environment, like we have a separate CI jobs with exotic settings, I
wonder.
^ permalink raw reply
* Re: [PATCH v2] send-email: move validation code below process_address_list
From: Junio C Hamano @ 2023-10-26 12:44 UTC (permalink / raw)
To: Michael Strawbridge; +Cc: Jeff King, Bagas Sanjaya, Git Mailing List
In-Reply-To: <ddd4bfdd-ed14-44f4-89d3-192332bbc1c4@amd.com>
Michael Strawbridge <michael.strawbridge@amd.com> writes:
> From 67223238d9b1977d20b1286055d7f197e4d746e9 Mon Sep 17 00:00:00 2001
> From: Michael Strawbridge <michael.strawbridge@amd.com>
> Date: Wed, 11 Oct 2023 16:13:13 -0400
> Subject: [PATCH v2] send-email: move validation code below
> process_address_list
Why do these in-body headers to lie about the author date?
By the way, the in-body header does seem to support the header line
folding (see the "subject" one here).
^ permalink raw reply
* Re: Regression: git send-email fails with "Use of uninitialized value $address" + "unable to extract a valid address"
From: Michael Strawbridge @ 2023-10-26 13:07 UTC (permalink / raw)
To: Junio C Hamano, Jeff King
Cc: Uwe Kleine-König, Luben Tuikov, git, entwicklung
In-Reply-To: <xmqqsf5xr1xk.fsf@gitster.g>
On 10/26/23 08:41, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
>
>> Note that the bug will only trigger if Email::Valid is installed.
>
> I recall we chased a different bug that depends on the use/non-use
> of this package a few years ago. Is the difference significant
> enough that we may want to install on one but not in another CI
> environment, like we have a separate CI jobs with exotic settings, I
> wonder.
That would make sense to me. We have had 3 regressions threads
recently for git send email where Email::Valid was important.
- [REGRESSION] uninitialized value $address in git send-email when given multiple recipients separated by commas - (this thread)
- [REGRESSION] uninitialized value $address in git send-email - https://public-inbox.org/git/20230918212004.GC2163162@coredump.intra.peff.net/T/#m9e0211a8ad387adbbadf31dcfcd7982d4046633d
- Regression: git send-email fails with "Use of uninitialized value $address" + "unable to extract a valid address" - https://public-inbox.org/git/68d7e5c3-6b4a-4d0d-9885-f3d4e2199f26@amd.com/T/#m1411c155e11ad9c5d913d22d1d11180ed56eabc7
^ permalink raw reply
* Re: [PATCH v2] send-email: move validation code below process_address_list
From: Michael Strawbridge @ 2023-10-26 13:11 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jeff King, Bagas Sanjaya, Git Mailing List
In-Reply-To: <xmqqlebpr1se.fsf@gitster.g>
On 10/26/23 08:44, Junio C Hamano wrote:
> Michael Strawbridge <michael.strawbridge@amd.com> writes:
>
>> From 67223238d9b1977d20b1286055d7f197e4d746e9 Mon Sep 17 00:00:00 2001
>> From: Michael Strawbridge <michael.strawbridge@amd.com>
>> Date: Wed, 11 Oct 2023 16:13:13 -0400
>> Subject: [PATCH v2] send-email: move validation code below
>> process_address_list
>
> Why do these in-body headers to lie about the author date?
Sorry. They weren't meant to. I have been amending my local commit
rather than making new ones for every version. It seems that when
I do that, the author date stays at the first time the commit was
created. I wasn't aware of that unintended side effect.
>
> By the way, the in-body header does seem to support the header line
> folding (see the "subject" one here).
^ permalink raw reply
* Re: [PATCH] merge: --ff-one-only to apply FF if commit is one
From: Ruslan Yakauleu @ 2023-10-26 13:40 UTC (permalink / raw)
To: git
In-Reply-To: <c37ba153-7239-49ff-b40f-370bc695986e@gmail.com>
> Squash and rebase are functionally identical in this case.
Sorry, but for me `git merge --squash` doesn't work.
Currently, I have a global option --no-ff for master
$ git config branch.master.mergeoptions --no-ff
In this way `git merge --squash` crashes with message
> fatal: options '--squash' and '--no-ff.' cannot be used together
In other way it writes something like
> Fast-forward
> Squash commit -- not updating HEAD
and... not updates parent branch.
For example, GitHub propose for PR's:
- Create a merge commit - the same as `git merge --no-ff`
- Rebase and merge - the same as `git rebase ...; git merge --ff-only`
- Squash AND commit - like two different operations. So after squash we
still have to merge our commit properly into parent branch.
The new option just dynamically selects between --ff and --no-ff for
`git merge`. Nothing else.
--
Ruslan
^ permalink raw reply
* Re: [PATCH v5 00/14] Introduce new `git replay` command
From: Johannes Schindelin @ 2023-10-26 13:44 UTC (permalink / raw)
To: Christian Couder
Cc: git, Junio C Hamano, Patrick Steinhardt, Elijah Newren, John Cai,
Derrick Stolee, Phillip Wood, Calvin Wan, Toon Claes,
Dragan Simic, Linus Arver
In-Reply-To: <20231010123847.2777056-1-christian.couder@gmail.com>
Hi Christian,
On Tue, 10 Oct 2023, Christian Couder wrote:
> 6: ec51351889 ! 6: 37d545d5d6 replay: don't simplify history
> @@ Metadata
> Author: Elijah Newren <newren@gmail.com>
>
> ## Commit message ##
> - replay: don't simplify history
> + replay: change rev walking options
>
> Let's set the rev walking options we need after calling
> - setup_revisions() instead of before. This makes it clearer which options
> - we need.
> + setup_revisions() instead of before. This enforces options we always
> + want.
> +
> + We want the command to work from older commits to newer ones by default,
> + but we are Ok with letting users reverse that, using --reverse, if that's
> + what they really want.
>
> Also we don't want history simplification, as we want to deal with all
> the commits in the affected range.
>
> + Helped-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>
> Co-authored-by: Christian Couder <chriscool@tuxfamily.org>
> Signed-off-by: Elijah Newren <newren@gmail.com>
> Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
> @@ builtin/replay.c: int cmd_replay(int argc, const char **argv, const char *prefix
> }
>
> + /* requirements/overrides for revs */
> -+ revs.reverse = 1;
> ++ revs.reverse = !revs.reverse;
> + revs.sort_order = REV_SORT_IN_GRAPH_ORDER;
> + revs.topo_order = 1;
> + revs.simplify_history = 0;
This still overrides a couple of command-line options, _silently_. I would
prefer those three assignments to be moved just before the
`setup_revisions()` call.
Letting users override these settings may not make much sense, but it
makes even less sense to pretend to let them override the settings and
then just ignore them without warning. (See also
https://en.wikipedia.org/wiki/Principle_of_least_astonishment.)
Moving these three assignments before the `setup_revisions()` call would
neatly remedy that.
> 7: cd4ed07d2d = 7: 2943f08926 replay: add an important FIXME comment about gpg signing
> 8: e45a55917c = 8: f81962ba41 replay: remove progress and info output
> 9: 0587a76cbb = 9: 236747497e replay: remove HEAD related sanity check
> 10: d10368e87a = 10: 3374d5be23 replay: make it a minimal server side command
> 11: 4e09572c43 ! 11: 197d076a93 replay: use standard revision ranges
> @@ Commit message
> way as many other Git commands. This makes its interface more
> standard and more flexible.
>
> + This also enables many revision related options accepted and
> + eaten by setup_revisions(). If the replay command was a high level
> + one or had a high level mode, it would make sense to restrict some
> + of the possible options, like those generating non-contiguous
> + history, as they could be confusing for most users.
> +
> Also as the interface of the command is now mostly finalized,
> we can add some documentation as well as testcases to make sure
> the command will continue to work as designed in the future.
>
> + We only document the rev-list related options among all the
> + revision related options that are now accepted, as the rev-list
> + related ones are probably the most useful for now.
> +
> + Helped-by: Dragan Simic <dsimic@manjaro.org>
> + Helped-by: Linus Arver <linusa@google.com>
> Co-authored-by: Christian Couder <chriscool@tuxfamily.org>
> Signed-off-by: Elijah Newren <newren@gmail.com>
> Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
> @@ Documentation/git-replay.txt (new)
> +
> +NAME
> +----
> -+git-replay - Replay commits on a different base, without touching working tree
> ++git-replay - Replay commits on a new base, works on bare repos too
> +
> +
> +SYNOPSIS
As mentioned in
https://lore.kernel.org/git/03460733-0219-c648-5757-db1958f8042e@gmx.de/,
I would like the `EXPERIMENTAL` label to be shown prominently here.
Probably not only the `SYNOPSIS` as I had originally suggested but also in
the `NAME`.
Otherwise we may end up with the same situation as with the (from my
perspective, failed) `git switch`/`git restore` experiment, where we
wanted to explore a better user experience than the overloaded `git
checkout` command, only to now be stuck with having to maintain
backward-compatibility for `git switch`/`git restore` command-line options
that were not meant to be set in stone but to be iterated on, instead. A
real-life demonstration of [Hyrum's Law](hyrumslaw.com/), if you like. Or,
from a different angle, we re-enacted https://xkcd.com/927/ in a way.
I'd like to suggest to learn from history and avoid this by tacking on a
warning label right at the top of the documentation. We may eventually
manage to iterate `git replay` to a point where it is totally capable to
supersede `git rebase`, by doing everything the latter does, except
better, who knows? But we _do_ need the liberty to make sweeping changes
to this new builtin if we want to have a prayer of doing that. And I fear
that not even mentioning the EXPERIMENTAL nature right at the top of the
manual page would just render us into that undesirable corner.
In addition, I am still a bit uneasy with introducing both the manual page
and the test script in this commit (see my comments in
https://lore.kernel.org/git/03460733-0219-c648-5757-db1958f8042e@gmx.de/).
It would be better to uphold our high standard and introduce scaffolds for
both files in the first commit, then populate the file contents
incrementally in the same the patches that introduce the corresponding
options/features/changes.
The rest of the interdiff consists mostly of context changes intermixed
with a couple of changes that I like.
Ciao,
Johannes
^ permalink raw reply
* Re: Regression: git send-email fails with "Use of uninitialized value $address" + "unable to extract a valid address"
From: Todd Zullinger @ 2023-10-26 14:46 UTC (permalink / raw)
To: Michael Strawbridge
Cc: Junio C Hamano, Jeff King, Uwe Kleine-König, Luben Tuikov,
git, entwicklung
In-Reply-To: <a71f2f1f-b5f0-4628-a4f3-6fd1319062a3@amd.com>
Michael Strawbridge wrote:
> On 10/26/23 08:41, Junio C Hamano wrote:
>> Jeff King <peff@peff.net> writes:
>>
>>> Note that the bug will only trigger if Email::Valid is installed.
>>
>> I recall we chased a different bug that depends on the use/non-use
>> of this package a few years ago. Is the difference significant
>> enough that we may want to install on one but not in another CI
>> environment, like we have a separate CI jobs with exotic settings, I
>> wonder.
>
> That would make sense to me. We have had 3 regressions threads
> recently for git send email where Email::Valid was important.
>
> - [REGRESSION] uninitialized value $address in git send-email when given multiple recipients separated by commas - (this thread)
> - [REGRESSION] uninitialized value $address in git send-email - https://public-inbox.org/git/20230918212004.GC2163162@coredump.intra.peff.net/T/#m9e0211a8ad387adbbadf31dcfcd7982d4046633d
> - Regression: git send-email fails with "Use of uninitialized value $address" + "unable to extract a valid address" - https://public-inbox.org/git/68d7e5c3-6b4a-4d0d-9885-f3d4e2199f26@amd.com/T/#m1411c155e11ad9c5d913d22d1d11180ed56eabc7
Alternately, perhaps having Email::Valid as an optional
dependency is worth reconsidering. If it's truly important
to validation, make it a requirement. If it's not, then
drop it to simplify the code and avoid these sort of issues.
As a (former) distribution packager, having these optional
dependencies which change the behavior is always a tough
position to be in.
If I make the git package require it to ensure consistent
behavior then some folks will -quite rightly- complain that
it should not be a requirement. If I keep it an optional
dependency, then debugging becomes more difficult for the
reasons we've seen in these recent (and not-so-recent)
threads.
I'd lean toward dropping the dependency entirely and leave
the more basic validation of git-send-email in place. That
may not catch every type of address error, but I would argue
that what we do without Email::Valid is perfectly reasonable
for checking basic email address syntax sanity.
Further validation will happen along the path of mail
transfer agents and failures should be reported to the
sender in the same way as any other invalid email address.
On a related note, one issue¹ we had reported in Fedora
after making Email::Valid a requirement was that it rejected
messages where the local part was too long, per the relevant
RFC's. But these were generated addresses from GitLab. The
addresses worked in practice. While Email::Valid was
technically correct in rejecting such addresses, it didn't
improve the experience of git send-email users.
¹ https://bugzilla.redhat.com/2046203
--
Todd
^ permalink raw reply
* [PATCH v3] bugreport: reject positional arguments
From: emilyshaffer @ 2023-10-26 15:54 UTC (permalink / raw)
To: git; +Cc: Emily Shaffer, Eric Sunshine, Sheik, Dragan Simic
In-Reply-To: <20231026005542.872301-1-nasamuffin@google.com>
From: Emily Shaffer <nasamuffin@google.com>
git-bugreport already rejected unrecognized flag arguments, like
`--diaggnose`, but this doesn't help if the user's mistake was to forget
the `--` in front of the argument. This can result in a user's intended
argument not being parsed with no indication to the user that something
went wrong. Since git-bugreport presently doesn't take any positionals
at all, let's reject all positionals and give the user a usage hint.
Signed-off-by: Emily Shaffer <nasamuffin@google.com>
---
Per Eric's and Dragan's comments, dropped the null checking for argv[0].
No point in being too paranoid, I suppose :)
Note that after this morning it's not likely that I'll be able to find
time to update this again so quickly, so if there are other nits,
reviewers can feel free to send their own rerolls rather than waiting
for me to see it and turn the patch around.
- Emily
builtin/bugreport.c | 5 +++++
t/t0091-bugreport.sh | 7 +++++++
2 files changed, 12 insertions(+)
diff --git a/builtin/bugreport.c b/builtin/bugreport.c
index d2ae5c305d..3106e56a13 100644
--- a/builtin/bugreport.c
+++ b/builtin/bugreport.c
@@ -126,6 +126,11 @@ int cmd_bugreport(int argc, const char **argv, const char *prefix)
argc = parse_options(argc, argv, prefix, bugreport_options,
bugreport_usage, 0);
+ if (argc) {
+ error(_("unknown argument `%s'"), argv[0]);
+ usage(bugreport_usage[0]);
+ }
+
/* Prepare the path to put the result */
prefixed_filename = prefix_filename(prefix,
option_output ? option_output : "");
diff --git a/t/t0091-bugreport.sh b/t/t0091-bugreport.sh
index f6998269be..5b1b3e8d07 100755
--- a/t/t0091-bugreport.sh
+++ b/t/t0091-bugreport.sh
@@ -69,6 +69,13 @@ test_expect_success 'incorrect arguments abort with usage' '
test_path_is_missing git-bugreport-*
'
+test_expect_success 'incorrect positional arguments abort with usage and hint' '
+ test_must_fail git bugreport false 2>output &&
+ test_i18ngrep usage output &&
+ test_i18ngrep false output &&
+ test_path_is_missing git-bugreport-*
+'
+
test_expect_success 'runs outside of a git dir' '
test_when_finished rm non-repo/git-bugreport-* &&
nongit git bugreport
--
2.42.0.758.gaed0368e0e-goog
^ permalink raw reply related
* Re: [PATCH v3] bugreport: reject positional arguments
From: Eric Sunshine @ 2023-10-26 17:18 UTC (permalink / raw)
To: emilyshaffer; +Cc: git, Emily Shaffer, Sheik, Dragan Simic
In-Reply-To: <20231026155459.2234929-1-nasamuffin@google.com>
On Thu, Oct 26, 2023 at 11:55 AM <emilyshaffer@google.com> wrote:
> git-bugreport already rejected unrecognized flag arguments, like
> `--diaggnose`, but this doesn't help if the user's mistake was to forget
> the `--` in front of the argument. This can result in a user's intended
> argument not being parsed with no indication to the user that something
> went wrong. Since git-bugreport presently doesn't take any positionals
> at all, let's reject all positionals and give the user a usage hint.
>
> Signed-off-by: Emily Shaffer <nasamuffin@google.com>
> ---
> Per Eric's and Dragan's comments, dropped the null checking for argv[0].
> No point in being too paranoid, I suppose :)
>
> Note that after this morning it's not likely that I'll be able to find
> time to update this again so quickly, so if there are other nits,
> reviewers can feel free to send their own rerolls rather than waiting
> for me to see it and turn the patch around.
Thanks. This version looks good enough to me. Just one minor comment below...
> diff --git a/t/t0091-bugreport.sh b/t/t0091-bugreport.sh
> @@ -69,6 +69,13 @@ test_expect_success 'incorrect arguments abort with usage' '
> +test_expect_success 'incorrect positional arguments abort with usage and hint' '
> + test_must_fail git bugreport false 2>output &&
> + test_i18ngrep usage output &&
> + test_i18ngrep false output &&
> + test_path_is_missing git-bugreport-*
> +'
I didn't really pay attention to the test in earlier rounds so didn't
notice this, but these days we just use 'grep' rather than
'test_i18ngrep'. (Indeed, the existing tests in this script use
'grep'.)
^ permalink raw reply
* [PATCH v4 0/2] bugreport: reject positional arguments
From: emilyshaffer @ 2023-10-26 18:22 UTC (permalink / raw)
To: git; +Cc: Emily Shaffer, Eric Sunshine, Sheik, Dragan Simic
In-Reply-To: <20231026155459.2234929-1-nasamuffin@google.com>
From: Emily Shaffer <nasamuffin@google.com>
The test I cribbed from for the newly added one in patch 2 was still
using test_i18ngrep, and Eric mentioned us not wanting i18ngrep at all
anymore, so I went ahead and cleaned that up as well.
- Emily
Emily Shaffer (2):
t0091-bugreport: stop using i18ngrep
bugreport: reject positional arguments
builtin/bugreport.c | 5 +++++
t/t0091-bugreport.sh | 9 ++++++++-
2 files changed, 13 insertions(+), 1 deletion(-)
--
2.42.0.820.g83a721a137-goog
^ permalink raw reply
* [PATCH v4 1/2] t0091-bugreport: stop using i18ngrep
From: emilyshaffer @ 2023-10-26 18:22 UTC (permalink / raw)
To: git; +Cc: Emily Shaffer
In-Reply-To: <20231026155459.2234929-1-nasamuffin@google.com>
From: Emily Shaffer <nasamuffin@google.com>
Since e6545201ad (Merge branch 'ab/detox-config-gettext', 2021-04-13),
test_i18ngrep is no longer required. Quit using it in the bugreport
tests, since it's setting a bad example for tests added later.
Signed-off-by: Emily Shaffer <nasamuffin@google.com>
---
t/t0091-bugreport.sh | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/t/t0091-bugreport.sh b/t/t0091-bugreport.sh
index f6998269be..e1588f71b7 100755
--- a/t/t0091-bugreport.sh
+++ b/t/t0091-bugreport.sh
@@ -65,7 +65,7 @@ test_expect_success '--output-directory puts the report in the provided dir' '
test_expect_success 'incorrect arguments abort with usage' '
test_must_fail git bugreport --false 2>output &&
- test_i18ngrep usage output &&
+ grep usage output &&
test_path_is_missing git-bugreport-*
'
--
2.42.0.820.g83a721a137-goog
^ permalink raw reply related
* [PATCH v4 2/2] bugreport: reject positional arguments
From: emilyshaffer @ 2023-10-26 18:22 UTC (permalink / raw)
To: git; +Cc: Emily Shaffer
In-Reply-To: <20231026155459.2234929-1-nasamuffin@google.com>
From: Emily Shaffer <nasamuffin@google.com>
git-bugreport already rejected unrecognized flag arguments, like
`--diaggnose`, but this doesn't help if the user's mistake was to forget
the `--` in front of the argument. This can result in a user's intended
argument not being parsed with no indication to the user that something
went wrong. Since git-bugreport presently doesn't take any positionals
at all, let's reject all positionals and give the user a usage hint.
Signed-off-by: Emily Shaffer <nasamuffin@google.com>
---
builtin/bugreport.c | 5 +++++
t/t0091-bugreport.sh | 7 +++++++
2 files changed, 12 insertions(+)
diff --git a/builtin/bugreport.c b/builtin/bugreport.c
index d2ae5c305d..3106e56a13 100644
--- a/builtin/bugreport.c
+++ b/builtin/bugreport.c
@@ -126,6 +126,11 @@ int cmd_bugreport(int argc, const char **argv, const char *prefix)
argc = parse_options(argc, argv, prefix, bugreport_options,
bugreport_usage, 0);
+ if (argc) {
+ error(_("unknown argument `%s'"), argv[0]);
+ usage(bugreport_usage[0]);
+ }
+
/* Prepare the path to put the result */
prefixed_filename = prefix_filename(prefix,
option_output ? option_output : "");
diff --git a/t/t0091-bugreport.sh b/t/t0091-bugreport.sh
index e1588f71b7..ae5b7dc31f 100755
--- a/t/t0091-bugreport.sh
+++ b/t/t0091-bugreport.sh
@@ -69,6 +69,13 @@ test_expect_success 'incorrect arguments abort with usage' '
test_path_is_missing git-bugreport-*
'
+test_expect_success 'incorrect positional arguments abort with usage and hint' '
+ test_must_fail git bugreport false 2>output &&
+ grep usage output &&
+ grep false output &&
+ test_path_is_missing git-bugreport-*
+'
+
test_expect_success 'runs outside of a git dir' '
test_when_finished rm non-repo/git-bugreport-* &&
nongit git bugreport
--
2.42.0.820.g83a721a137-goog
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox