* Re: [PATCH 1/2] t9902: verify that completion does not print anything
From: Patrick Steinhardt @ 2024-01-15 9:02 UTC (permalink / raw)
To: Jeff King; +Cc: Johannes Schindelin, git, Stan Hu
In-Reply-To: <20240112151655.GA640039@coredump.intra.peff.net>
[-- Attachment #1: Type: text/plain, Size: 1131 bytes --]
On Fri, Jan 12, 2024 at 10:16:55AM -0500, Jeff King wrote:
> On Fri, Jan 12, 2024 at 02:12:43PM +0100, Johannes Schindelin wrote:
>
> > But my main concern is: Why does this happen in the first place? If we are
> > running with Bash, why does `BASH_XTRACEFD` to work as intended here and
> > makes it necessary to filter out the traced commands?
>
> BASH_XTRACEFD was introduced in bash 4.1. macOS ships with the ancient
> bash 3.2.57, which is the last GPLv2 version.
>
> One simple solution is to mark the script with test_untraceable. See
> 5fc98e79fc (t: add means to disable '-x' tracing for individual test
> scripts, 2018-02-24) and 5827506928 (t1510-repo-setup: mark as
> untraceable with '-x', 2018-02-24).
>
> That will disable tracing entirely in the script for older versions of
> bash, which could make debugging harder. But it will still work as
> expected for people on reasonable versions of bash, and doesn't
> introduce any complicated code.
>
> -Peff
Ah, this is indeed the best solution. Thanks for the hints and
investigations everyone, will fix in the next iteration.
Patrick
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH] push: improve consistency of output when "up to date"
From: Patrick Steinhardt @ 2024-01-15 8:43 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Eric Sunshine, Benji Kay via GitGitGadget, git, Benji Kay
In-Reply-To: <xmqqjzofec0e.fsf@gitster.g>
[-- Attachment #1: Type: text/plain, Size: 2334 bytes --]
On Thu, Jan 11, 2024 at 02:33:21PM -0800, Junio C Hamano wrote:
> Eric Sunshine <sunshine@sunshineco.com> writes:
>
> > Thanks. This particular change is proposed periodically...
> >
> >> diff --git a/transport.c b/transport.c
> >> @@ -1467,7 +1467,7 @@ int transport_push(struct repository *r,
> >> else if (!quiet && !ret && !transport_refs_pushed(remote_refs))
> >> - fprintf(stderr, "Everything up-to-date\n");
> >> + fprintf(stderr, "Everything up to date.\n");
> >
> > ... but has not been considered desirable.
> >
> > See, for instance, this email thread explaining the rationale for
> > avoiding such a change:
> > https://lore.kernel.org/git/pull.1298.git.1658908927714.gitgitgadget@gmail.com/T/
>
> Looking at the "grep" hits:
>
> $ git grep -e 'up-to-date.*"' \*.c
> builtin/rm.c: OPT__FORCE(&force, N_("override the up-to-date check"), PARSE_OPT_NOCOMPLETE),
> builtin/send-pack.c: fprintf(stderr, "Everything up-to-date\n");
> http-push.c: fprintf(stderr, "'%s': up-to-date\n", ref->name);
> http-push.c: "Maybe you are not up-to-date and "
> transport.c: fprintf(stderr, "Everything up-to-date\n");
>
> it is true that these are not marked for translation, which should
> be a clue enough that we want them to be exactly the way they are
> spelled. However, they are going to the standard error stream. Is
> it reasonable to expect third-party tools scraping it to find the
> string "up-to-date"?
I would say it's not entirely reasonable:
- These are strings that users see frequently, and if they are not
proficient in the English language I think it actually regresses
their user experience.
- The way this string is written would never lead me, as a script
developer, to think that this is a message that should be parsed by
my script. It's simply too user-focussed to make me think so.
- Last but not least, I think it's not entirely unreasonable to ask
script developers to use e.g. LANG=C when they expect strings to be
stable.
Also, with the introduction of `git push --porcelain`, I think there is
even less reason to keep such user-visible strings intact. Any machine
that wants to parse output of git-push(1) should use `--porcelain`
instead in my opinion.
Patrick
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH] commit-graph: fix memory leak when not writing graph
From: Patrick Steinhardt @ 2024-01-15 7:08 UTC (permalink / raw)
To: Taylor Blau; +Cc: git, Junio C Hamano
In-Reply-To: <ZZhUWu5pgBEYK409@nand.local>
[-- Attachment #1: Type: text/plain, Size: 957 bytes --]
On Fri, Jan 05, 2024 at 02:11:22PM -0500, Taylor Blau wrote:
> On Mon, Dec 18, 2023 at 11:02:28AM +0100, Patrick Steinhardt wrote:
> > When `write_commit_graph()` bails out writing a split commit-graph early
> > then it may happen that we have already gathered the set of existing
> > commit-graph file names without yet determining the new merged set of
> > files. This can result in a memory leak though because we only clear the
> > preimage of files when we have collected the postimage.
> >
> > Fix this issue by dropping the condition altogether so that we always
> > try to free both preimage and postimage filenames. As the context
> > structure is zero-initialized this simplification is safe to do.
>
> Looks obviously good to me, thanks for finding and fixing.
Cc'ing Junio so that this fix doesn't fall off the radar. I thought I
saw the topic in "seen" once, but either I misremember or it got dropped
from there.
Patrick
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* [PATCH] bisect: add --force flag to force checkout
From: Kevin Wang via GitGitGadget @ 2024-01-15 7:05 UTC (permalink / raw)
To: git; +Cc: Kevin Wang, Kevin Wang
From: Kevin Wang <kevmo314@gmail.com>
Adds a `--force`/`-f` flag to `git bisect good/bad` and `git bisect run` to
force a checkout. Currently, if the repository state adds any local changes
the user must manually reset the repository state before moving to the next
bisection step. This can happen with package lock files or log output data,
for example. With this change, a developer can run `git bisect run --force`
to automatically reset the repository state after each evaluation. The flag
is also supported as `git bisect (good|bad) --force` as well.
Signed-off-by: Kevin Wang <kevmo314@gmail.com>
---
bisect: add --force flag to force checkout
Adds a --force/-f flag to git bisect good/bad and git bisect run to
force a checkout. Currently, if the repository state adds any local
changes the user must manually reset the repository state before moving
to the next bisection step. This can happen with package lock files or
log output data, for example. With this change, a developer can run git
bisect run --force to automatically reset the repository state after
each evaluation. The flag is also supported as git bisect (good|bad)
--force as well.
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1641%2Fkevmo314%2Fbisect-force-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1641/kevmo314/bisect-force-v1
Pull-Request: https://github.com/git/git/pull/1641
Documentation/git-bisect.txt | 13 ++++-
bisect.c | 17 ++++--
bisect.h | 4 +-
builtin/bisect.c | 50 +++++++++++-----
t/t6030-bisect-porcelain.sh | 109 +++++++++++++++++++++++++++++++++++
5 files changed, 167 insertions(+), 26 deletions(-)
diff --git a/Documentation/git-bisect.txt b/Documentation/git-bisect.txt
index aa02e462244..57357221718 100644
--- a/Documentation/git-bisect.txt
+++ b/Documentation/git-bisect.txt
@@ -19,14 +19,14 @@ on the subcommand:
git bisect start [--term-(new|bad)=<term-new> --term-(old|good)=<term-old>]
[--no-checkout] [--first-parent] [<bad> [<good>...]] [--] [<paths>...]
git bisect (bad|new|<term-new>) [<rev>]
- git bisect (good|old|<term-old>) [<rev>...]
+ git bisect (good|old|<term-old>) [-f] [<rev>...]
git bisect terms [--term-good | --term-bad]
git bisect skip [(<rev>|<range>)...]
git bisect reset [<commit>]
git bisect (visualize|view)
git bisect replay <logfile>
git bisect log
- git bisect run <cmd> [<arg>...]
+ git bisect run [-f] <cmd> [<arg>...]
git bisect help
This command uses a binary search algorithm to find which commit in
@@ -381,6 +381,15 @@ ignored.
This option is particularly useful in avoiding false positives when a merged
branch contained broken or non-buildable commits, but the merge itself was OK.
+-f::
+--force::
++
+Throw away any local changes and untracked files before moving to the next
+bisection step.
++
+This option may be useful if the repository state changes when testing a
+revision.
+
EXAMPLES
--------
diff --git a/bisect.c b/bisect.c
index 985b96ed132..72ce09f2015 100644
--- a/bisect.c
+++ b/bisect.c
@@ -713,7 +713,7 @@ static int is_expected_rev(const struct object_id *oid)
}
enum bisect_error bisect_checkout(const struct object_id *bisect_rev,
- int no_checkout)
+ int no_checkout, int force_checkout)
{
struct commit *commit;
struct pretty_print_context pp = {0};
@@ -728,8 +728,13 @@ enum bisect_error bisect_checkout(const struct object_id *bisect_rev,
struct child_process cmd = CHILD_PROCESS_INIT;
cmd.git_cmd = 1;
- strvec_pushl(&cmd.args, "checkout", "-q",
- oid_to_hex(bisect_rev), "--", NULL);
+ if (force_checkout) {
+ strvec_pushl(&cmd.args, "checkout", "-f", "-q",
+ oid_to_hex(bisect_rev), "--", NULL);
+ } else {
+ strvec_pushl(&cmd.args, "checkout", "-q",
+ oid_to_hex(bisect_rev), "--", NULL);
+ }
if (run_command(&cmd))
/*
* Errors in `run_command()` itself, signaled by res < 0,
@@ -850,7 +855,7 @@ static enum bisect_error check_merge_bases(int rev_nr, struct commit **rev, int
handle_skipped_merge_base(mb);
} else {
printf(_("Bisecting: a merge base must be tested\n"));
- res = bisect_checkout(mb, no_checkout);
+ res = bisect_checkout(mb, no_checkout, 0);
if (!res)
/* indicate early success */
res = BISECT_INTERNAL_SUCCESS_MERGE_BASE;
@@ -1002,7 +1007,7 @@ void read_bisect_terms(const char **read_bad, const char **read_good)
* the end of bisect_helper::cmd_bisect__helper() helps bypassing
* all the code related to finding a commit to test.
*/
-enum bisect_error bisect_next_all(struct repository *r, const char *prefix)
+enum bisect_error bisect_next_all(struct repository *r, const char *prefix, int force_checkout)
{
struct strvec rev_argv = STRVEC_INIT;
struct rev_info revs = REV_INFO_INIT;
@@ -1104,7 +1109,7 @@ enum bisect_error bisect_next_all(struct repository *r, const char *prefix)
/* Clean up objects used, as they will be reused. */
repo_clear_commit_marks(r, ALL_REV_FLAGS);
- res = bisect_checkout(bisect_rev, no_checkout);
+ res = bisect_checkout(bisect_rev, no_checkout, force_checkout);
cleanup:
release_revisions(&revs);
strvec_clear(&rev_argv);
diff --git a/bisect.h b/bisect.h
index ee3fd65f3bd..6f972365faa 100644
--- a/bisect.h
+++ b/bisect.h
@@ -71,7 +71,7 @@ struct bisect_state {
unsigned int nr_bad;
};
-enum bisect_error bisect_next_all(struct repository *r, const char *prefix);
+enum bisect_error bisect_next_all(struct repository *r, const char *prefix, int force_checkout);
int estimate_bisect_steps(int all);
@@ -80,6 +80,6 @@ void read_bisect_terms(const char **bad, const char **good);
int bisect_clean_state(void);
enum bisect_error bisect_checkout(const struct object_id *bisect_rev,
- int no_checkout);
+ int no_checkout, int force_checkout);
#endif
diff --git a/builtin/bisect.c b/builtin/bisect.c
index 47fcce59ff7..99c16d57942 100644
--- a/builtin/bisect.c
+++ b/builtin/bisect.c
@@ -29,7 +29,7 @@ static GIT_PATH_FUNC(git_path_bisect_run, "BISECT_RUN")
" [--no-checkout] [--first-parent] [<bad> [<good>...]] [--]" \
" [<pathspec>...]")
#define BUILTIN_GIT_BISECT_STATE_USAGE \
- N_("git bisect (good|bad) [<rev>...]")
+ N_("git bisect (good|bad) [-f] [<rev>...]")
#define BUILTIN_GIT_BISECT_TERMS_USAGE \
"git bisect terms [--term-good | --term-bad]"
#define BUILTIN_GIT_BISECT_SKIP_USAGE \
@@ -45,7 +45,7 @@ static GIT_PATH_FUNC(git_path_bisect_run, "BISECT_RUN")
#define BUILTIN_GIT_BISECT_LOG_USAGE \
"git bisect log"
#define BUILTIN_GIT_BISECT_RUN_USAGE \
- N_("git bisect run <cmd> [<arg>...]")
+ N_("git bisect run [-f] <cmd> [<arg>...]")
static const char * const git_bisect_usage[] = {
BUILTIN_GIT_BISECT_START_USAGE,
@@ -651,7 +651,7 @@ static int bisect_successful(struct bisect_terms *terms)
return res;
}
-static enum bisect_error bisect_next(struct bisect_terms *terms, const char *prefix)
+static enum bisect_error bisect_next(struct bisect_terms *terms, const char *prefix, int force_checkout)
{
enum bisect_error res;
@@ -662,7 +662,7 @@ static enum bisect_error bisect_next(struct bisect_terms *terms, const char *pre
return BISECT_FAILED;
/* Perform all bisection computation */
- res = bisect_next_all(the_repository, prefix);
+ res = bisect_next_all(the_repository, prefix, force_checkout);
if (res == BISECT_INTERNAL_SUCCESS_1ST_BAD_FOUND) {
res = bisect_successful(terms);
@@ -674,14 +674,14 @@ static enum bisect_error bisect_next(struct bisect_terms *terms, const char *pre
return res;
}
-static enum bisect_error bisect_auto_next(struct bisect_terms *terms, const char *prefix)
+static enum bisect_error bisect_auto_next(struct bisect_terms *terms, const char *prefix, int force_checkout)
{
if (bisect_next_check(terms, NULL)) {
bisect_print_status(terms);
return BISECT_OK;
}
- return bisect_next(terms, prefix);
+ return bisect_next(terms, prefix, force_checkout);
}
static enum bisect_error bisect_start(struct bisect_terms *terms, int argc,
@@ -875,7 +875,7 @@ static enum bisect_error bisect_start(struct bisect_terms *terms, int argc,
if (res)
return res;
- res = bisect_auto_next(terms, NULL);
+ res = bisect_auto_next(terms, NULL, 0);
if (!is_bisect_success(res))
bisect_clean_state();
return res;
@@ -917,7 +917,7 @@ static enum bisect_error bisect_state(struct bisect_terms *terms, int argc,
const char **argv)
{
const char *state;
- int i, verify_expected = 1;
+ int i, force_checkout = 0, verify_expected = 1;
struct object_id oid, expected;
struct oid_array revs = OID_ARRAY_INIT;
@@ -934,6 +934,13 @@ static enum bisect_error bisect_state(struct bisect_terms *terms, int argc,
argv++;
argc--;
+
+ if (argc > 0 && (!strcmp(argv[0], "--force") || !strcmp(argv[0], "-f"))) {
+ force_checkout = 1;
+ argv++;
+ argc--;
+ }
+
if (argc > 1 && !strcmp(state, terms->term_bad))
return error(_("'git bisect %s' can take only one argument."), terms->term_bad);
@@ -989,7 +996,7 @@ static enum bisect_error bisect_state(struct bisect_terms *terms, int argc,
}
oid_array_clear(&revs);
- return bisect_auto_next(terms, NULL);
+ return bisect_auto_next(terms, NULL, force_checkout);
}
static enum bisect_error bisect_log(void)
@@ -1078,7 +1085,7 @@ static enum bisect_error bisect_replay(struct bisect_terms *terms, const char *f
if (res)
return BISECT_FAILED;
- return bisect_auto_next(terms, NULL);
+ return bisect_auto_next(terms, NULL, 0);
}
static enum bisect_error bisect_skip(struct bisect_terms *terms, int argc,
@@ -1173,7 +1180,7 @@ static int do_bisect_run(const char *command)
return run_command(&cmd);
}
-static int verify_good(const struct bisect_terms *terms, const char *command)
+static int verify_good(const struct bisect_terms *terms, const char *command, int force_checkout)
{
int rc;
enum bisect_error res;
@@ -1189,13 +1196,13 @@ static int verify_good(const struct bisect_terms *terms, const char *command)
if (read_ref(no_checkout ? "BISECT_HEAD" : "HEAD", ¤t_rev))
return -1;
- res = bisect_checkout(&good_rev, no_checkout);
+ res = bisect_checkout(&good_rev, no_checkout, force_checkout);
if (res != BISECT_OK)
return -1;
rc = do_bisect_run(command);
- res = bisect_checkout(¤t_rev, no_checkout);
+ res = bisect_checkout(¤t_rev, no_checkout, force_checkout);
if (res != BISECT_OK)
return -1;
@@ -1209,6 +1216,7 @@ static int bisect_run(struct bisect_terms *terms, int argc, const char **argv)
const char *new_state;
int temporary_stdout_fd, saved_stdout;
int is_first_run = 1;
+ int force_checkout = 0;
if (bisect_next_check(terms, NULL))
return BISECT_FAILED;
@@ -1218,8 +1226,14 @@ static int bisect_run(struct bisect_terms *terms, int argc, const char **argv)
return BISECT_FAILED;
}
+ if (argc > 0 && (!strcmp(argv[0], "--force") || !strcmp(argv[0], "-f"))) {
+ force_checkout = 1;
+ argv++;
+ argc--;
+ }
sq_quote_argv(&command, argv);
strbuf_ltrim(&command);
+
while (1) {
res = do_bisect_run(command.buf);
@@ -1231,7 +1245,7 @@ static int bisect_run(struct bisect_terms *terms, int argc, const char **argv)
* missing or non-executable script.
*/
if (is_first_run && (res == 126 || res == 127)) {
- int rc = verify_good(terms, command.buf);
+ int rc = verify_good(terms, command.buf, force_checkout);
is_first_run = 0;
if (rc < 0 || 128 <= rc) {
error(_("unable to verify %s on good"
@@ -1271,7 +1285,11 @@ static int bisect_run(struct bisect_terms *terms, int argc, const char **argv)
saved_stdout = dup(1);
dup2(temporary_stdout_fd, 1);
- res = bisect_state(terms, 1, &new_state);
+ if (force_checkout) {
+ res = bisect_state(terms, 2, (const char *[]){ new_state, "--force" });
+ } else {
+ res = bisect_state(terms, 1, &new_state);
+ }
fflush(stdout);
dup2(saved_stdout, 1);
@@ -1342,7 +1360,7 @@ static int cmd_bisect__next(int argc, const char **argv UNUSED, const char *pref
return error(_("'%s' requires 0 arguments"),
"git bisect next");
get_terms(&terms);
- res = bisect_next(&terms, prefix);
+ res = bisect_next(&terms, prefix, 0);
free_terms(&terms);
return res;
}
diff --git a/t/t6030-bisect-porcelain.sh b/t/t6030-bisect-porcelain.sh
index 561080bf240..eb17cc58a16 100755
--- a/t/t6030-bisect-porcelain.sh
+++ b/t/t6030-bisect-porcelain.sh
@@ -1259,4 +1259,113 @@ test_expect_success 'verify correct error message' '
grep "git bisect good.*exited with error code" error
'
+test_expect_success 'bisect dirty, explicit ref' '
+ test_when_finished "git bisect reset" &&
+ add_line_into_file "Commit 1" file &&
+ add_line_into_file "Commit 2" file &&
+ add_line_into_file "Commit 3" file &&
+ git bisect start &&
+ git bisect bad HEAD &&
+ echo "modified state" >file &&
+ test_when_finished "git checkout -- file" &&
+ test_must_fail git bisect good HEAD~2
+'
+
+test_expect_success 'bisect dirty good force, explicit ref' '
+ test_when_finished "git bisect reset" &&
+ add_line_into_file "Commit 1" file &&
+ add_line_into_file "Commit 2" file &&
+ add_line_into_file "Commit 3" file &&
+ git bisect start &&
+ git bisect bad HEAD &&
+ echo "modified state" >file &&
+ test_when_finished "git checkout -- file" &&
+ git bisect good --force HEAD~2
+'
+
+test_expect_success 'bisect dirty, implicit ref' '
+ test_when_finished "git bisect reset" &&
+ add_line_into_file "Commit 1" file &&
+ add_line_into_file "Commit 2" file &&
+ add_line_into_file "Commit 3" file &&
+ add_line_into_file "Commit 4" file &&
+ add_line_into_file "Commit 5" file &&
+ git bisect start &&
+ git bisect bad HEAD &&
+ git bisect good HEAD~4 &&
+ echo "modified state" >file &&
+ test_when_finished "git checkout -- file" &&
+ test_must_fail git bisect good &&
+ test_must_fail git bisect bad
+'
+
+test_expect_success 'bisect dirty good force, implicit ref' '
+ test_when_finished "git bisect reset" &&
+ add_line_into_file "Commit 1" file &&
+ add_line_into_file "Commit 2" file &&
+ add_line_into_file "Commit 3" file &&
+ add_line_into_file "Commit 4" file &&
+ add_line_into_file "Commit 5" file &&
+ git bisect start &&
+ git bisect bad HEAD &&
+ git bisect good HEAD~4 &&
+ echo "modified state" >file &&
+ test_when_finished "git checkout -- file" &&
+ git bisect good --force
+'
+
+test_expect_success 'bisect dirty bad force, implicit ref' '
+ test_when_finished "git bisect reset" &&
+ add_line_into_file "Commit 1" file &&
+ add_line_into_file "Commit 2" file &&
+ add_line_into_file "Commit 3" file &&
+ add_line_into_file "Commit 4" file &&
+ add_line_into_file "Commit 5" file &&
+ git bisect start &&
+ git bisect bad HEAD &&
+ git bisect good HEAD~4 &&
+ echo "modified state" >file &&
+ test_when_finished "git checkout -- file" &&
+ git bisect bad --force
+'
+
+test_expect_success 'bisect run dirty' '
+ test_when_finished "git bisect reset" &&
+ test_when_finished "git checkout -- file" &&
+ add_line_into_file "Bisect run 1" file &&
+ add_line_into_file "Bisect run 2" file &&
+ add_line_into_file "Bisect run 3" file &&
+ add_line_into_file "Bisect run 4" file &&
+ add_line_into_file "Bisect run 5" file &&
+ write_script test_script.sh <<-\EOF &&
+ ! echo "modified state" >file
+ ! grep "Bisect run 3" || exit 126 >/dev/null
+ EOF
+ git bisect start &&
+ git bisect bad HEAD &&
+ git bisect good HEAD~4 &&
+ test_must_fail git bisect run ./test_script.sh
+'
+
+test_expect_success 'bisect run dirty force' '
+ test_when_finished "git bisect reset" &&
+ test_when_finished "git checkout -- file" &&
+ add_line_into_file "Bisect run 1" file &&
+ add_line_into_file "Bisect run 2" file &&
+ add_line_into_file "Bisect run 3" file &&
+ add_line_into_file "Bisect run 4" file &&
+ add_line_into_file "Bisect run 5" file &&
+ write_script test_script.sh <<-\EOF &&
+ ! echo "modified state" >file
+ ! grep "Bisect run 3" || exit 126 >/dev/null
+ EOF
+ HASH5=$(git rev-parse --verify HEAD) &&
+ git bisect start &&
+ git bisect bad HEAD &&
+ git bisect good HEAD~4 &&
+ echo "doing run\n" &&
+ git bisect run --force ./test_script.sh >my_bisect_log.txt &&
+ grep "$HASH5 is the first bad commit" my_bisect_log.txt
+'
+
test_done
base-commit: a26002b62827b89a19b1084bd75d9371d565d03c
--
gitgitgadget
^ permalink raw reply related
* Re: [PATCH] clone: support cloning of filtered bundles
From: Junio C Hamano @ 2024-01-15 2:09 UTC (permalink / raw)
To: Nikolay Edigaryev via GitGitGadget; +Cc: git, Derrick Stolee, Nikolay Edigaryev
In-Reply-To: <xmqqcyu35rg9.fsf@gitster.g>
Junio C Hamano <gitster@pobox.com> writes:
> "Nikolay Edigaryev via GitGitGadget" <gitgitgadget@gmail.com>
> writes:
>
>> diff --git a/builtin/clone.c b/builtin/clone.c
>> index c6357af9498..4b3fedf78ed 100644
>> --- a/builtin/clone.c
>> +++ b/builtin/clone.c
>> @@ -1227,9 +1227,18 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
>>
>> if (fd > 0)
>> close(fd);
>> +
>> + if (has_filter) {
>> + strbuf_addf(&key, "remote.%s.promisor", remote_name);
>> + git_config_set(key.buf, "true");
>> + strbuf_reset(&key);
>> +
>> + strbuf_addf(&key, "remote.%s.partialclonefilter", remote_name);
>> + git_config_set(key.buf, expand_list_objects_filter_spec(&header.filter));
>> + strbuf_reset(&key);
>> + }
>> +
>
>> -# NEEDSWORK: 'git clone --bare' should be able to clone from a filtered
>> -# bundle, but that requires a change to promisor/filter config options.
> ...
> But a bundle that were created with objects _omitted_ already?
> ... the source of this clone operation, i.e. the bundle file that is
> pointed at by "remote.$remote_name.url", cannot be that promisor.
Extending the above a bit, one important way a bundle is used is as
a medium for sneaker-net. Instead of making a full clone over the
network, if you can create a bundle that records all objects and all
refs out of the source repository and then unbundle it in a
different place to create a repository, you can tweak the resulting
repository by either adding a separete remote or changing the
remote.origin.url so that your subsequent fetch goes over the
network to the repository you took the initial bundle from.
The "tweak the resulting repository" part however MUST be done
manually with the current system. If we can optionally record the
publically reachable URL of the source repository when we create a
bundle file, and "git clone" on the receiving side can read the URL
out of the bundle and act on it (e.g., show it to the user and offer
to record it as remote.origin.url in the resulting repository---I do
not think it is wise to do this silently without letting the user
know from security's point of view), then the use of bundle files as
a medium for sneaker-netting will become even easier.
And once that is done, perhaps allowing a filtered bundle to act as
a sneaker-net medium to simulate an initial filtered clone would
make sense. The promisor as well as the origin will be the network
reachable URL and subsequent fetches (both deliberate ones via "git
fetch" as well as lazy on-demand ones that backfills missing objects
via the "promisor" access) would become possible.
But without such a change to the bundle file format, allowing
"clone" to finish and pretend the resulting repository is usable is
somewhat irresponsible to the users. The on-demand lazy fetch would
fail after this code cloned from such a filtered bundle, no?
^ permalink raw reply
* Re: [PATCH] clone: support cloning of filtered bundles
From: Junio C Hamano @ 2024-01-15 1:13 UTC (permalink / raw)
To: Nikolay Edigaryev via GitGitGadget; +Cc: git, Derrick Stolee, Nikolay Edigaryev
In-Reply-To: <pull.1644.git.git.1705231010118.gitgitgadget@gmail.com>
"Nikolay Edigaryev via GitGitGadget" <gitgitgadget@gmail.com>
writes:
> diff --git a/builtin/clone.c b/builtin/clone.c
> index c6357af9498..4b3fedf78ed 100644
> --- a/builtin/clone.c
> +++ b/builtin/clone.c
> @@ -1227,9 +1227,18 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
>
> if (fd > 0)
> close(fd);
> +
> + if (has_filter) {
> + strbuf_addf(&key, "remote.%s.promisor", remote_name);
> + git_config_set(key.buf, "true");
> + strbuf_reset(&key);
> +
> + strbuf_addf(&key, "remote.%s.partialclonefilter", remote_name);
> + git_config_set(key.buf, expand_list_objects_filter_spec(&header.filter));
> + strbuf_reset(&key);
> + }
> +
> -# NEEDSWORK: 'git clone --bare' should be able to clone from a filtered
> -# bundle, but that requires a change to promisor/filter config options.
Sorry, but this "should be able to" does not make sense to me in the
first place.
I can understand how an operation to create a narrow clone of an
unfiltered bundle and then prepare the resulting repository for
future "fattening" by naming the unfiltered bundle file a
"promisor", and allow the user to lazily fetch objects that have
initially been filtered out of the bundle lazily.
But a bundle that were created with objects _omitted_ already? It
obviously cannot "promise" to deliber any objects that ought to be
reachable from the objects contained in it, so setting the bundle
file as promisor in the configuration does not help the resulting
repository. Those missing objects must be obtained from somewhere
other than that original "filtered" bundle, and if that source of
objects that can promise all the objects that are ought to be
reachable were specified as the promisor, it would make sense. But
the source of this clone operation, i.e. the bundle file that is
pointed at by "remote.$remote_name.url", cannot be that promisor.
^ permalink raw reply
* [PATCH v2 4/4] maintenance: use XDG config if it exists
From: Kristoffer Haugsbakk @ 2024-01-14 21:43 UTC (permalink / raw)
To: git; +Cc: Kristoffer Haugsbakk, ps, stolee, Eric Sunshine, Taylor Blau
In-Reply-To: <cover.1705267839.git.code@khaugsbakk.name>
`git maintenance register` registers the repository in the user's global
config. `$XDG_CONFIG_HOME/git/config` is supposed to be used if
`~/.gitconfig` does not exist. However, this command creates a
`~/.gitconfig` file and writes to that one even though the XDG variant
exists.
This used to work correctly until 50a044f1e4 (gc: replace config
subprocesses with API calls, 2022-09-27), when the command started calling
the config API instead of git-config(1).
Also change `unregister` accordingly.
Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>
---
Notes (series):
v2:
• Add `unregister` tests
• Use subshell when exporting an env. variable
• Style in tests
• Free variables properly
builtin/gc.c | 27 ++++++++++++-------------
t/t7900-maintenance.sh | 45 ++++++++++++++++++++++++++++++++++++++++++
2 files changed, 58 insertions(+), 14 deletions(-)
diff --git a/builtin/gc.c b/builtin/gc.c
index c078751824c..cb80ced6cb5 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -1543,19 +1543,18 @@ static int maintenance_register(int argc, const char **argv, const char *prefix)
if (!found) {
int rc;
- char *user_config = NULL, *xdg_config = NULL;
+ char *global_config_file = NULL;
if (!config_file) {
- git_global_config_paths(&user_config, &xdg_config);
- config_file = user_config;
- if (!user_config)
- die(_("$HOME not set"));
+ global_config_file = git_global_config();
+ config_file = global_config_file;
}
+ if (!config_file)
+ die(_("$HOME not set"));
rc = git_config_set_multivar_in_file_gently(
config_file, "maintenance.repo", maintpath,
CONFIG_REGEX_NONE, 0);
- free(user_config);
- free(xdg_config);
+ free(global_config_file);
if (rc)
die(_("unable to add '%s' value of '%s'"),
@@ -1612,18 +1611,18 @@ static int maintenance_unregister(int argc, const char **argv, const char *prefi
if (found) {
int rc;
- char *user_config = NULL, *xdg_config = NULL;
+ char *global_config_file = NULL;
+
if (!config_file) {
- git_global_config_paths(&user_config, &xdg_config);
- config_file = user_config;
- if (!user_config)
- die(_("$HOME not set"));
+ global_config_file = git_global_config();
+ config_file = global_config_file;
}
+ if (!config_file)
+ die(_("$HOME not set"));
rc = git_config_set_multivar_in_file_gently(
config_file, key, NULL, maintpath,
CONFIG_FLAGS_MULTI_REPLACE | CONFIG_FLAGS_FIXED_VALUE);
- free(user_config);
- free(xdg_config);
+ free(global_config_file);
if (rc &&
(!force || rc == CONFIG_NOTHING_SET))
diff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh
index 00d29871e65..0943dfa18a3 100755
--- a/t/t7900-maintenance.sh
+++ b/t/t7900-maintenance.sh
@@ -67,6 +67,51 @@ test_expect_success 'maintenance.auto config option' '
test_subcommand ! git maintenance run --auto --quiet <false
'
+test_expect_success 'register uses XDG_CONFIG_HOME config if it exists' '
+ test_when_finished rm -r .config/git/config &&
+ (
+ XDG_CONFIG_HOME=.config &&
+ export XDG_CONFIG_HOME &&
+ mkdir -p $XDG_CONFIG_HOME/git &&
+ >$XDG_CONFIG_HOME/git/config &&
+ git maintenance register &&
+ git config --file=$XDG_CONFIG_HOME/git/config --get maintenance.repo >actual &&
+ pwd >expect &&
+ test_cmp expect actual
+ )
+'
+
+test_expect_success 'register does not need XDG_CONFIG_HOME config to exist' '
+ test_when_finished git maintenance unregister &&
+ test_path_is_missing $XDG_CONFIG_HOME/git/config &&
+ git maintenance register &&
+ git config --global --get maintenance.repo >actual &&
+ pwd >expect &&
+ test_cmp expect actual
+'
+
+test_expect_success 'unregister uses XDG_CONFIG_HOME config if it exists' '
+ test_when_finished rm -r .config/git/config &&
+ (
+ XDG_CONFIG_HOME=.config &&
+ export XDG_CONFIG_HOME &&
+ mkdir -p $XDG_CONFIG_HOME/git &&
+ >$XDG_CONFIG_HOME/git/config &&
+ git maintenance register &&
+ git maintenance unregister &&
+ test_must_fail git config --file=$XDG_CONFIG_HOME/git/config --get maintenance.repo >actual &&
+ test_must_be_empty actual
+ )
+'
+
+test_expect_success 'unregister does not need XDG_CONFIG_HOME config to exist' '
+ test_path_is_missing $XDG_CONFIG_HOME/git/config &&
+ git maintenance register &&
+ git maintenance unregister &&
+ test_must_fail git config --global --get maintenance.repo >actual &&
+ test_must_be_empty actual
+'
+
test_expect_success 'maintenance.<task>.enabled' '
git config maintenance.gc.enabled false &&
git config maintenance.commit-graph.enabled true &&
--
2.43.0
^ permalink raw reply related
* [PATCH v2 3/4] config: factor out global config file retrieval
From: Kristoffer Haugsbakk @ 2024-01-14 21:43 UTC (permalink / raw)
To: git; +Cc: Kristoffer Haugsbakk, ps, stolee, Eric Sunshine, Taylor Blau
In-Reply-To: <cover.1705267839.git.code@khaugsbakk.name>
Factor out code that retrieves the global config file so that we can use
it in `gc.c` as well.
Use the old name from the previous commit since this function acts
functionally the same as `git_system_config` but for “global”.
Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>
---
Notes (series):
v2:
• Don’t die; return `NULL`
builtin/config.c | 25 +++----------------------
config.c | 20 ++++++++++++++++++++
config.h | 4 ++++
3 files changed, 27 insertions(+), 22 deletions(-)
diff --git a/builtin/config.c b/builtin/config.c
index 6fff2655816..08fe36d4997 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -708,30 +708,11 @@ int cmd_config(int argc, const char **argv, const char *prefix)
}
if (use_global_config) {
- char *user_config, *xdg_config;
-
- git_global_config_paths(&user_config, &xdg_config);
- if (!user_config)
- /*
- * It is unknown if HOME/.gitconfig exists, so
- * we do not know if we should write to XDG
- * location; error out even if XDG_CONFIG_HOME
- * is set and points at a sane location.
- */
+ given_config_source.file = git_global_config();
+ if (!given_config_source.file)
die(_("$HOME not set"));
-
given_config_source.scope = CONFIG_SCOPE_GLOBAL;
-
- if (access_or_warn(user_config, R_OK, 0) &&
- xdg_config && !access_or_warn(xdg_config, R_OK, 0)) {
- given_config_source.file = xdg_config;
- free(user_config);
- } else {
- given_config_source.file = user_config;
- free(xdg_config);
- }
- }
- else if (use_system_config) {
+ } else if (use_system_config) {
given_config_source.file = git_system_config();
given_config_source.scope = CONFIG_SCOPE_SYSTEM;
} else if (use_local_config) {
diff --git a/config.c b/config.c
index ebc6a57e1c3..3cfeb3d8bd9 100644
--- a/config.c
+++ b/config.c
@@ -1987,6 +1987,26 @@ char *git_system_config(void)
return system_config;
}
+char *git_global_config(void)
+{
+ char *user_config, *xdg_config;
+
+ git_global_config_paths(&user_config, &xdg_config);
+ if (!user_config) {
+ free(xdg_config);
+ return NULL;
+ }
+
+ if (access_or_warn(user_config, R_OK, 0) && xdg_config &&
+ !access_or_warn(xdg_config, R_OK, 0)) {
+ free(user_config);
+ return xdg_config;
+ } else {
+ free(xdg_config);
+ return user_config;
+ }
+}
+
void git_global_config_paths(char **user_out, char **xdg_out)
{
char *user_config = xstrdup_or_null(getenv("GIT_CONFIG_GLOBAL"));
diff --git a/config.h b/config.h
index e5e523553cc..625e932b993 100644
--- a/config.h
+++ b/config.h
@@ -382,6 +382,10 @@ int config_error_nonbool(const char *);
#endif
char *git_system_config(void);
+/**
+ * Returns `NULL` if is uncertain whether or not `HOME/.gitconfig` exists.
+ */
+char *git_global_config(void);
void git_global_config_paths(char **user, char **xdg);
int git_config_parse_parameter(const char *, config_fn_t fn, void *data);
--
2.43.0
^ permalink raw reply related
* [PATCH v2 2/4] config: rename global config function
From: Kristoffer Haugsbakk @ 2024-01-14 21:43 UTC (permalink / raw)
To: git; +Cc: Kristoffer Haugsbakk, ps, stolee, Eric Sunshine, Taylor Blau
In-Reply-To: <cover.1705267839.git.code@khaugsbakk.name>
Rename this function to a more descriptive name since we want to use the
existing name for a new function.
Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>
---
builtin/config.c | 2 +-
builtin/gc.c | 4 ++--
builtin/var.c | 2 +-
config.c | 4 ++--
config.h | 2 +-
5 files changed, 7 insertions(+), 7 deletions(-)
diff --git a/builtin/config.c b/builtin/config.c
index 87d0dc92d99..6fff2655816 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -710,7 +710,7 @@ int cmd_config(int argc, const char **argv, const char *prefix)
if (use_global_config) {
char *user_config, *xdg_config;
- git_global_config(&user_config, &xdg_config);
+ git_global_config_paths(&user_config, &xdg_config);
if (!user_config)
/*
* It is unknown if HOME/.gitconfig exists, so
diff --git a/builtin/gc.c b/builtin/gc.c
index 7c11d5ebef0..c078751824c 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -1546,7 +1546,7 @@ static int maintenance_register(int argc, const char **argv, const char *prefix)
char *user_config = NULL, *xdg_config = NULL;
if (!config_file) {
- git_global_config(&user_config, &xdg_config);
+ git_global_config_paths(&user_config, &xdg_config);
config_file = user_config;
if (!user_config)
die(_("$HOME not set"));
@@ -1614,7 +1614,7 @@ static int maintenance_unregister(int argc, const char **argv, const char *prefi
int rc;
char *user_config = NULL, *xdg_config = NULL;
if (!config_file) {
- git_global_config(&user_config, &xdg_config);
+ git_global_config_paths(&user_config, &xdg_config);
config_file = user_config;
if (!user_config)
die(_("$HOME not set"));
diff --git a/builtin/var.c b/builtin/var.c
index 8cf7dd9e2e5..cf5567208a2 100644
--- a/builtin/var.c
+++ b/builtin/var.c
@@ -90,7 +90,7 @@ static char *git_config_val_global(int ident_flag UNUSED)
char *user, *xdg;
size_t unused;
- git_global_config(&user, &xdg);
+ git_global_config_paths(&user, &xdg);
if (xdg && *xdg) {
normalize_path_copy(xdg, xdg);
strbuf_addf(&buf, "%s\n", xdg);
diff --git a/config.c b/config.c
index d26e16e3ce3..ebc6a57e1c3 100644
--- a/config.c
+++ b/config.c
@@ -1987,7 +1987,7 @@ char *git_system_config(void)
return system_config;
}
-void git_global_config(char **user_out, char **xdg_out)
+void git_global_config_paths(char **user_out, char **xdg_out)
{
char *user_config = xstrdup_or_null(getenv("GIT_CONFIG_GLOBAL"));
char *xdg_config = NULL;
@@ -2040,7 +2040,7 @@ static int do_git_config_sequence(const struct config_options *opts,
data, CONFIG_SCOPE_SYSTEM,
NULL);
- git_global_config(&user_config, &xdg_config);
+ git_global_config_paths(&user_config, &xdg_config);
if (xdg_config && !access_or_die(xdg_config, R_OK, ACCESS_EACCES_OK))
ret += git_config_from_file_with_options(fn, xdg_config, data,
diff --git a/config.h b/config.h
index 14f881ecfaf..e5e523553cc 100644
--- a/config.h
+++ b/config.h
@@ -382,7 +382,7 @@ int config_error_nonbool(const char *);
#endif
char *git_system_config(void);
-void git_global_config(char **user, char **xdg);
+void git_global_config_paths(char **user, char **xdg);
int git_config_parse_parameter(const char *, config_fn_t fn, void *data);
--
2.43.0
^ permalink raw reply related
* [PATCH v2 1/4] config: format newlines
From: Kristoffer Haugsbakk @ 2024-01-14 21:43 UTC (permalink / raw)
To: git; +Cc: Kristoffer Haugsbakk, ps, stolee, Eric Sunshine, Taylor Blau
In-Reply-To: <cover.1705267839.git.code@khaugsbakk.name>
Remove unneeded newlines according to `clang-format`.
Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>
---
Notes (series):
Honestly the formatter changing these lines over and over again was just
annoying. And we're visiting the file anyway.
builtin/config.c | 1 -
config.c | 2 --
2 files changed, 3 deletions(-)
diff --git a/builtin/config.c b/builtin/config.c
index 11a4d4ef141..87d0dc92d99 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -760,7 +760,6 @@ int cmd_config(int argc, const char **argv, const char *prefix)
given_config_source.scope = CONFIG_SCOPE_COMMAND;
}
-
if (respect_includes_opt == -1)
config_options.respect_includes = !given_config_source.file;
else
diff --git a/config.c b/config.c
index 9ff6ae1cb90..d26e16e3ce3 100644
--- a/config.c
+++ b/config.c
@@ -95,7 +95,6 @@ static long config_file_ftell(struct config_source *conf)
return ftell(conf->u.file);
}
-
static int config_buf_fgetc(struct config_source *conf)
{
if (conf->u.buf.pos < conf->u.buf.len)
@@ -3418,7 +3417,6 @@ int git_config_set_multivar_in_file_gently(const char *config_filename,
write_err_out:
ret = write_error(get_lock_file_path(&lock));
goto out_free;
-
}
void git_config_set_multivar_in_file(const char *config_filename,
--
2.43.0
^ permalink raw reply related
* [PATCH v2 0/4] maintenance: use XDG config if it exists
From: Kristoffer Haugsbakk @ 2024-01-14 21:43 UTC (permalink / raw)
To: git; +Cc: Kristoffer Haugsbakk, ps, stolee, Eric Sunshine, Taylor Blau
In-Reply-To: <cover.1697660181.git.code@khaugsbakk.name>
I use the conventional XDG config path for the global configuration. This
path is always used except for `git maintenance register` and
`unregister`.
§ Changes since v1
• Free `config_file`
• https://lore.kernel.org/git/ZTZDsIcrB0zwHlFR@tanuki/
• Return `NULL` instead of dying
• https://lore.kernel.org/git/ZTZDqToqcsDiS5AP@tanuki/
• Tests
• Test unregister
• Use subshells
• Style
§ Patches
• 1–3: Preparatory
• 4: The desired change
§ CC
• Patrick Steinhardt: `config` changes; v1 feedback
• Derrick Stolee: `maintenance` changes
• Eric Sunshine: v1 feedback
• Taylor Blau: v1 feedback
Kristoffer Haugsbakk (4):
config: format newlines
config: rename global config function
config: factor out global config file retrieval
maintenance: use XDG config if it exists
builtin/config.c | 26 +++---------------------
builtin/gc.c | 27 ++++++++++++-------------
builtin/var.c | 2 +-
config.c | 26 ++++++++++++++++++++----
config.h | 6 +++++-
t/t7900-maintenance.sh | 45 ++++++++++++++++++++++++++++++++++++++++++
6 files changed, 89 insertions(+), 43 deletions(-)
Range-diff against v1:
1: 39934cb7e50 = 1: d5f6c8d62ec config: format newlines
2: 48a5357f97c = 2: cbc5fde0094 config: rename global config function
3: 147c767443c ! 3: 32e5ec7d866 config: factor out global config file retrieval
@@ Commit message
Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>
+
+ ## Notes (series) ##
+ v2:
+ • Don’t die; return `NULL`
+
## builtin/config.c ##
@@ builtin/config.c: int cmd_config(int argc, const char **argv, const char *prefix)
}
@@ builtin/config.c: int cmd_config(int argc, const char **argv, const char *prefix
- * location; error out even if XDG_CONFIG_HOME
- * is set and points at a sane location.
- */
-- die(_("$HOME not set"));
--
+ given_config_source.file = git_global_config();
++ if (!given_config_source.file)
+ die(_("$HOME not set"));
+-
given_config_source.scope = CONFIG_SCOPE_GLOBAL;
-
- if (access_or_warn(user_config, R_OK, 0) &&
@@ config.c: char *git_system_config(void)
+ char *user_config, *xdg_config;
+
+ git_global_config_paths(&user_config, &xdg_config);
-+ if (!user_config)
-+ /*
-+ * It is unknown if HOME/.gitconfig exists, so
-+ * we do not know if we should write to XDG
-+ * location; error out even if XDG_CONFIG_HOME
-+ * is set and points at a sane location.
-+ */
-+ die(_("$HOME not set"));
++ if (!user_config) {
++ free(xdg_config);
++ return NULL;
++ }
+
+ if (access_or_warn(user_config, R_OK, 0) && xdg_config &&
+ !access_or_warn(xdg_config, R_OK, 0)) {
@@ config.h: int config_error_nonbool(const char *);
#endif
char *git_system_config(void);
++/**
++ * Returns `NULL` if is uncertain whether or not `HOME/.gitconfig` exists.
++ */
+char *git_global_config(void);
void git_global_config_paths(char **user, char **xdg);
4: 1e2376a4b99 < -: ----------- maintenance: use XDG config if it exists
-: ----------- > 4: 8bd67c5bf01 maintenance: use XDG config if it exists
--
2.43.0
^ permalink raw reply
* Re: [PATCH] clone: support cloning of filtered bundles
From: Nikolay Edigaryev @ 2024-01-14 21:26 UTC (permalink / raw)
To: phillip.wood; +Cc: Nikolay Edigaryev via GitGitGadget, git, Derrick Stolee
In-Reply-To: <CAFX5hXTCPt-rDrWZ-RN8S84o_FooY3Ck2H1kMYdHQGzuetPBSw@mail.gmail.com>
> Note that currently, when you clone a normal non-filtered bundle with a
> '--filter' argument specified, no filtering will take place and no error
> will be thrown. "promisor = true" and "partialclonefilter = ..." options
> will be set in the repo config, but no .promisor file will be created.
> This is even more confusing IMO, but that's how it currently on
> Git 2.43.0.
I was wrong about this one: the .promisor pack file actually gets created.
And the filtering is not being done because the "uploadpack.allowFilter"
global option is not enabled by default.
Unfortunately the only way to figure this out is to run Git with
GIT_TRACE=2, which shows:
warning: filtering not recognized by server, ignoring
So please feel free to skip this part from the consideration.
On Sun, Jan 14, 2024 at 11:39 PM Nikolay Edigaryev <edigaryev@gmail.com> wrote:
>
> Hello Phillip,
>
> > As I understand it if you're cloning from a bundle file then then there
> > is no remote so how can we set a remote-specific config?
>
> There is a remote, for more details see df61c88979 (clone: also
> configure url for bare clones, 2010-03-29), which has the following
> code:
>
> strbuf_addf(&key, "remote.%s.url", remote_name);
> git_config_set(key.buf, repo);
> strbuf_reset(&key);
>
> You can verify this by creating a bundle on Git 2.43.0 with "git create
> bundle bundle.bundle --all" and then cloning it with "git clone
> --bare /path/to/bundle.bundle", in my case the following repo-wide
> configuration file was created:
>
> [core]
> repositoryformatversion = 0
> filemode = true
> bare = true
> ignorecase = true
> precomposeunicode = true
> [remote "origin"]
> url = /Users/edi/src/cirrus-cli/cli.bundle
>
> > I'm surprised that the proposed change does not require the user to pass
> > "--filter" to "git clone" as I expected that we'd want to check that the
> > filter on the command line was compatible with the filter used to create
> > the bundle. Allowing "git clone" to create a partial clone without the
> > user asking for it by passing the "--filter" option feels like is going
> > to end up confusing users.
>
> Note that currently, when you clone a normal non-filtered bundle with a
> '--filter' argument specified, no filtering will take place and no error
> will be thrown. "promisor = true" and "partialclonefilter = ..." options
> will be set in the repo config, but no .promisor file will be created.
> This is even more confusing IMO, but that's how it currently on
> Git 2.43.0.
>
> You have a good point, but I feel like completely preventing cloning of
> filtered bundles and requiring a '--filter' argument is very taxing. If
> you've already specified a '--filter' when creating a bundle (and thus
> your intent to use partially cloned data), why do it multiple times?
>
> What I propose as an alternative here is to act based on the user's
> intent when cloning:
>
> * when the user specifies no '--filter' argument, do nothing special,
> allow cloning both types of bundles: normal and filtered (with the
> logic from this patch)
>
> * when the user does specify a '--filter' argument, either:
> * throw an error explaining that filtering of filtered bundles is not
> supported
> * or compare the user's filter specification and the one that is
> in the bundle and only throw an error if they mismatch
>
> Let me know what you think about this (and perhaps you have a more
> concrete example in mind where this will have negative consequences)
> and I'll be happy to do a next iteration.
>
>
> On Sun, Jan 14, 2024 at 10:00 PM Phillip Wood <phillip.wood123@gmail.com> wrote:
> >
> > Hi Nikolay
> >
> > On 14/01/2024 11:16, Nikolay Edigaryev via GitGitGadget wrote:
> > > From: Nikolay Edigaryev <edigaryev@gmail.com>
> > >
> > > f18b512bbb (bundle: create filtered bundles, 2022-03-09) introduced an
> > > incredibly useful ability to create filtered bundles, which advances
> > > the partial clone/promisor support in Git and allows for archiving
> > > large repositories to object storages like S3 in bundles that are:
> > >
> > > * easy to manage
> > > * bundle is just a single file, it's easier to guarantee atomic
> > > replacements in object storages like S3 and they are faster to
> > > fetch than a bare repository since there's only a single GET
> > > request involved
> > > * incredibly tiny
> > > * no indexes (which may be more than 10 MB for some repositories)
> > > and other fluff, compared to cloning a bare repository
> > > * bundle can be filtered to only contain the tips of refs neccessary
> > > for e.g. code-analysis purposes
> > >
> > > However, in 86fdd94d72 (clone: fail gracefully when cloning filtered
> > > bundle, 2022-03-09) the cloning of such bundles was disabled, with a
> > > note that this behavior is not desired, and it the long-term this
> > > should be possible.
> > >
> > > The commit above states that it's not possible to have this at the
> > > moment due to lack of remote and a repository-global config that
> > > specifies an object filter, yet it's unclear why a remote-specific
> > > config can't be used instead, which is what this change does.
> >
> > As I understand it if you're cloning from a bundle file then then there
> > is no remote so how can we set a remote-specific config?
> >
> > I'm surprised that the proposed change does not require the user to pass
> > "--filter" to "git clone" as I expected that we'd want to check that the
> > filter on the command line was compatible with the filter used to create
> > the bundle. Allowing "git clone" to create a partial clone without the
> > user asking for it by passing the "--filter" option feels like is going
> > to end up confusing users.
> >
> > > +test_expect_success 'cloning from filtered bundle works' '
> > > + git bundle create partial.bdl --all --filter=blob:none &&
> > > + git clone --bare partial.bdl partial 2>err
> >
> > The redirection hides any error message which will make debugging test
> > failures harder. It would be nice to see this test check any config set
> > when cloning and that git commands can run successfully in the repository.
> >
> > Best Wishes
> >
> > Phillip
^ permalink raw reply
* Re: [PATCH 00/10] Enrich Trailer API
From: Linus Arver @ 2024-01-14 20:05 UTC (permalink / raw)
To: Junio C Hamano, Linus Arver via GitGitGadget
Cc: git, Emily Shaffer, Christian Couder
In-Reply-To: <owlyil3yhv70.fsf@fine.c.googlers.com>
Linus Arver <linusa@google.com> writes:
> interpret-trailers: fail if given unrecognized arguments
> (Summary: E.g., for "--where", only accept recognized WHERE_* enum
> values. If we get something unrecognized, fail with an error
> instead of silently doing nothing. Ditto for "--if-exists" and
> "--if-missing".)
>
> The last one is a different class of bug than the first two, and perhaps
> less interesting.
Actually, upon closer inspection I realize that we already fail if given
unrecognized arguments to --where, --if-exists, and --if-missing. But we
don't explain why to the user because no error message is printed. This
commit has been retitled to "interpret-trailers: print error if given
unrecognized arguments".
Thanks.
^ permalink raw reply
* [PATCH] rev-list-options: fix off-by-one in '--filter=blob:limit=<n>' explainer
From: Nikolay Edigaryev via GitGitGadget @ 2024-01-14 19:50 UTC (permalink / raw)
To: git; +Cc: Nikolay Edigaryev, Nikolay Edigaryev
From: Nikolay Edigaryev <edigaryev@gmail.com>
'--filter=blob:limit=<n>' was introduced in 25ec7bcac0 (list-objects:
filter objects in traverse_commit_list, 2017-11-21) and later expanded
to bitmaps in 84243da129 (pack-bitmap: implement BLOB_LIMIT filtering,
2020-02-14)
The logic that was introduced in these commits (and that still persists
to this day) omits blobs larger than _or equal_ to n bytes or units.
However, the documentation (Documentation/rev-list-options.txt) states:
>The form '--filter=blob:limit=<n>[kmg]' omits blobs larger than n
bytes or units. n may be zero.
Moreover, the t6113-rev-list-bitmap-filters.sh tests for exactly this
logic, so it seems it is the documentation that needs fixing, not the
code.
This changes the explanation to be similar to
Documentation/git-clone.txt, which is correct.
Signed-off-by: Nikolay Edigaryev <edigaryev@gmail.com>
---
rev-list-options: fix off-by-one in '--filter=blob:limit=' explainer
'--filter=blob:limit=' was introduced in 25ec7bcac0 (list-objects:
filter objects in traverse_commit_list, 2017-11-21) and later expanded
to bitmaps in 84243da129 (pack-bitmap: implement BLOB_LIMIT filtering,
2020-02-14)
The logic that was introduced in these commits (and that still persists
to this day) omits blobs larger than or equal to n bytes or units.
However, the documentation (Documentation/rev-list-options.txt) states:
> The form '--filter=blob:limit=[kmg]' omits blobs larger than n bytes
> or units. n may be zero.
Moreover, the t6113-rev-list-bitmap-filters.sh tests for exactly this
logic, so it seems it is the documentation that needs fixing, not the
code.
This changes the explanation to be similar to
Documentation/git-clone.txt, which is correct.
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1645%2Fedigaryev%2Ffix-blob-limit-off-by-one-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1645/edigaryev/fix-blob-limit-off-by-one-v1
Pull-Request: https://github.com/git/git/pull/1645
Documentation/rev-list-options.txt | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/Documentation/rev-list-options.txt b/Documentation/rev-list-options.txt
index 2bf239ff030..a583b52c612 100644
--- a/Documentation/rev-list-options.txt
+++ b/Documentation/rev-list-options.txt
@@ -947,10 +947,10 @@ ifdef::git-rev-list[]
+
The form '--filter=blob:none' omits all blobs.
+
-The form '--filter=blob:limit=<n>[kmg]' omits blobs larger than n bytes
-or units. n may be zero. The suffixes k, m, and g can be used to name
-units in KiB, MiB, or GiB. For example, 'blob:limit=1k' is the same
-as 'blob:limit=1024'.
+The form '--filter=blob:limit=<n>[kmg]' omits blobs of size at least n
+bytes or units. n may be zero. The suffixes k, m, and g can be used
+to name units in KiB, MiB, or GiB. For example, 'blob:limit=1k'
+is the same as 'blob:limit=1024'.
+
The form '--filter=object:type=(tag|commit|tree|blob)' omits all objects
which are not of the requested type.
base-commit: 564d0252ca632e0264ed670534a51d18a689ef5d
--
gitgitgadget
^ permalink raw reply related
* Re: [PATCH] clone: support cloning of filtered bundles
From: Nikolay Edigaryev @ 2024-01-14 19:39 UTC (permalink / raw)
To: phillip.wood; +Cc: Nikolay Edigaryev via GitGitGadget, git, Derrick Stolee
In-Reply-To: <24b82309-34e9-49a0-983b-7e94dad3d06b@gmail.com>
Hello Phillip,
> As I understand it if you're cloning from a bundle file then then there
> is no remote so how can we set a remote-specific config?
There is a remote, for more details see df61c88979 (clone: also
configure url for bare clones, 2010-03-29), which has the following
code:
strbuf_addf(&key, "remote.%s.url", remote_name);
git_config_set(key.buf, repo);
strbuf_reset(&key);
You can verify this by creating a bundle on Git 2.43.0 with "git create
bundle bundle.bundle --all" and then cloning it with "git clone
--bare /path/to/bundle.bundle", in my case the following repo-wide
configuration file was created:
[core]
repositoryformatversion = 0
filemode = true
bare = true
ignorecase = true
precomposeunicode = true
[remote "origin"]
url = /Users/edi/src/cirrus-cli/cli.bundle
> I'm surprised that the proposed change does not require the user to pass
> "--filter" to "git clone" as I expected that we'd want to check that the
> filter on the command line was compatible with the filter used to create
> the bundle. Allowing "git clone" to create a partial clone without the
> user asking for it by passing the "--filter" option feels like is going
> to end up confusing users.
Note that currently, when you clone a normal non-filtered bundle with a
'--filter' argument specified, no filtering will take place and no error
will be thrown. "promisor = true" and "partialclonefilter = ..." options
will be set in the repo config, but no .promisor file will be created.
This is even more confusing IMO, but that's how it currently on
Git 2.43.0.
You have a good point, but I feel like completely preventing cloning of
filtered bundles and requiring a '--filter' argument is very taxing. If
you've already specified a '--filter' when creating a bundle (and thus
your intent to use partially cloned data), why do it multiple times?
What I propose as an alternative here is to act based on the user's
intent when cloning:
* when the user specifies no '--filter' argument, do nothing special,
allow cloning both types of bundles: normal and filtered (with the
logic from this patch)
* when the user does specify a '--filter' argument, either:
* throw an error explaining that filtering of filtered bundles is not
supported
* or compare the user's filter specification and the one that is
in the bundle and only throw an error if they mismatch
Let me know what you think about this (and perhaps you have a more
concrete example in mind where this will have negative consequences)
and I'll be happy to do a next iteration.
On Sun, Jan 14, 2024 at 10:00 PM Phillip Wood <phillip.wood123@gmail.com> wrote:
>
> Hi Nikolay
>
> On 14/01/2024 11:16, Nikolay Edigaryev via GitGitGadget wrote:
> > From: Nikolay Edigaryev <edigaryev@gmail.com>
> >
> > f18b512bbb (bundle: create filtered bundles, 2022-03-09) introduced an
> > incredibly useful ability to create filtered bundles, which advances
> > the partial clone/promisor support in Git and allows for archiving
> > large repositories to object storages like S3 in bundles that are:
> >
> > * easy to manage
> > * bundle is just a single file, it's easier to guarantee atomic
> > replacements in object storages like S3 and they are faster to
> > fetch than a bare repository since there's only a single GET
> > request involved
> > * incredibly tiny
> > * no indexes (which may be more than 10 MB for some repositories)
> > and other fluff, compared to cloning a bare repository
> > * bundle can be filtered to only contain the tips of refs neccessary
> > for e.g. code-analysis purposes
> >
> > However, in 86fdd94d72 (clone: fail gracefully when cloning filtered
> > bundle, 2022-03-09) the cloning of such bundles was disabled, with a
> > note that this behavior is not desired, and it the long-term this
> > should be possible.
> >
> > The commit above states that it's not possible to have this at the
> > moment due to lack of remote and a repository-global config that
> > specifies an object filter, yet it's unclear why a remote-specific
> > config can't be used instead, which is what this change does.
>
> As I understand it if you're cloning from a bundle file then then there
> is no remote so how can we set a remote-specific config?
>
> I'm surprised that the proposed change does not require the user to pass
> "--filter" to "git clone" as I expected that we'd want to check that the
> filter on the command line was compatible with the filter used to create
> the bundle. Allowing "git clone" to create a partial clone without the
> user asking for it by passing the "--filter" option feels like is going
> to end up confusing users.
>
> > +test_expect_success 'cloning from filtered bundle works' '
> > + git bundle create partial.bdl --all --filter=blob:none &&
> > + git clone --bare partial.bdl partial 2>err
>
> The redirection hides any error message which will make debugging test
> failures harder. It would be nice to see this test check any config set
> when cloning and that git commands can run successfully in the repository.
>
> Best Wishes
>
> Phillip
^ permalink raw reply
* Re: [PATCH] strvec: use correct member name in comments
From: Linus Arver @ 2024-01-14 18:20 UTC (permalink / raw)
To: Jeff King; +Cc: Junio C Hamano, Linus Arver via GitGitGadget, git
In-Reply-To: <20240113073131.GA657764@coredump.intra.peff.net>
Jeff King <peff@peff.net> writes:
> On Fri, Jan 12, 2024 at 04:37:46PM -0800, Linus Arver wrote:
>
>> OTOH if we were treating these .h files as something meant for direct
>> external consumption (that is, if strvec.h is libified and external
>> users outside of Git are expected to use it directly as their first
>> point of documentation), at that point it might make sense to name the
>> parameters (akin to the style of manpages for syscalls). But I imagine
>> at that point we would have some other means of developer docs (beyond
>> raw header files) for libified parts of Git, so even in that case it's
>> probably fine to keep things as is.
>
> I think this is mostly orthogonal to libification. Whether the audience
> is other parts of Git or users outside of Git, they need to know how to
> call the function. Our main source of documentation there is comments
> above the declaration (we've marked these with "/**" which would allow a
> parser to pull them into a separate doc file, but AFAIK in the 9 years
> since we started that convention, nobody has bothered to write such a
> script).
>
> Naming the parameters can help when writing those comments, because you
> can then refer to them (e.g., see the comment above strbuf_addftime).
> Even without that, I think they can be helpful, but I don't think I'd
> bother adding them in unless taking a pass over the whole file, looking
> for comments that do not sufficiently explain their matching functions.
So in summary you are saying that the comments are the most important
source of documentation that we have currently, and unless naming the
parameters improves these comments, we shouldn't bother naming these
parameters. I agree.
> I don't doubt that some of that would be necessary for libification,
> just to increase the quality of the documentation. But I think it's
> largely separate from the patch in this thread.
I agree with both statements. Thanks.
^ permalink raw reply
* Re: [PATCH] clone: support cloning of filtered bundles
From: Phillip Wood @ 2024-01-14 18:00 UTC (permalink / raw)
To: Nikolay Edigaryev via GitGitGadget, git; +Cc: Derrick Stolee, Nikolay Edigaryev
In-Reply-To: <pull.1644.git.git.1705231010118.gitgitgadget@gmail.com>
Hi Nikolay
On 14/01/2024 11:16, Nikolay Edigaryev via GitGitGadget wrote:
> From: Nikolay Edigaryev <edigaryev@gmail.com>
>
> f18b512bbb (bundle: create filtered bundles, 2022-03-09) introduced an
> incredibly useful ability to create filtered bundles, which advances
> the partial clone/promisor support in Git and allows for archiving
> large repositories to object storages like S3 in bundles that are:
>
> * easy to manage
> * bundle is just a single file, it's easier to guarantee atomic
> replacements in object storages like S3 and they are faster to
> fetch than a bare repository since there's only a single GET
> request involved
> * incredibly tiny
> * no indexes (which may be more than 10 MB for some repositories)
> and other fluff, compared to cloning a bare repository
> * bundle can be filtered to only contain the tips of refs neccessary
> for e.g. code-analysis purposes
>
> However, in 86fdd94d72 (clone: fail gracefully when cloning filtered
> bundle, 2022-03-09) the cloning of such bundles was disabled, with a
> note that this behavior is not desired, and it the long-term this
> should be possible.
>
> The commit above states that it's not possible to have this at the
> moment due to lack of remote and a repository-global config that
> specifies an object filter, yet it's unclear why a remote-specific
> config can't be used instead, which is what this change does.
As I understand it if you're cloning from a bundle file then then there
is no remote so how can we set a remote-specific config?
I'm surprised that the proposed change does not require the user to pass
"--filter" to "git clone" as I expected that we'd want to check that the
filter on the command line was compatible with the filter used to create
the bundle. Allowing "git clone" to create a partial clone without the
user asking for it by passing the "--filter" option feels like is going
to end up confusing users.
> +test_expect_success 'cloning from filtered bundle works' '
> + git bundle create partial.bdl --all --filter=blob:none &&
> + git clone --bare partial.bdl partial 2>err
The redirection hides any error message which will make debugging test
failures harder. It would be nice to see this test check any config set
when cloning and that git commands can run successfully in the repository.
Best Wishes
Phillip
^ permalink raw reply
* [PATCH] clone: support cloning of filtered bundles
From: Nikolay Edigaryev via GitGitGadget @ 2024-01-14 11:16 UTC (permalink / raw)
To: git; +Cc: Derrick Stolee, Nikolay Edigaryev, Nikolay Edigaryev
From: Nikolay Edigaryev <edigaryev@gmail.com>
f18b512bbb (bundle: create filtered bundles, 2022-03-09) introduced an
incredibly useful ability to create filtered bundles, which advances
the partial clone/promisor support in Git and allows for archiving
large repositories to object storages like S3 in bundles that are:
* easy to manage
* bundle is just a single file, it's easier to guarantee atomic
replacements in object storages like S3 and they are faster to
fetch than a bare repository since there's only a single GET
request involved
* incredibly tiny
* no indexes (which may be more than 10 MB for some repositories)
and other fluff, compared to cloning a bare repository
* bundle can be filtered to only contain the tips of refs neccessary
for e.g. code-analysis purposes
However, in 86fdd94d72 (clone: fail gracefully when cloning filtered
bundle, 2022-03-09) the cloning of such bundles was disabled, with a
note that this behavior is not desired, and it the long-term this
should be possible.
The commit above states that it's not possible to have this at the
moment due to lack of remote and a repository-global config that
specifies an object filter, yet it's unclear why a remote-specific
config can't be used instead, which is what this change does.
Signed-off-by: Nikolay Edigaryev <edigaryev@gmail.com>
---
clone: support cloning of filtered bundles
f18b512bbb (bundle: create filtered bundles, 2022-03-09) introduced an
incredibly useful ability to create filtered bundles, which advances the
partial clone/promisor support in Git and allows for archiving large
repositories to object storages like S3 in bundles that are:
* easy to manage
* bundle is just a single file, it's easier to guarantee atomic
replacements in object storages like S3 and they are faster to
fetch than a bare repository since there's only a single GET
request involved
* incredibly tiny
* no indexes (which may be more than 10 MB for some repositories) and
other fluff, compared to cloning a bare repository
* bundle can be filtered to only contain the tips of refs neccessary
for e.g. code-analysis purposes
However, in 86fdd94d72 (clone: fail gracefully when cloning filtered
bundle, 2022-03-09) the cloning of such bundles was disabled, with a
note that this behavior is not desired, and it the long-term this should
be possible.
The commit above states that it's not possible to have this at the
moment due to lack of remote and a repository-global config that
specifies an object filter, yet it's unclear why a remote-specific
config can't be used instead, which is what this change does.
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1644%2Fedigaryev%2Fclone-filtered-bundles-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1644/edigaryev/clone-filtered-bundles-v1
Pull-Request: https://github.com/git/git/pull/1644
builtin/clone.c | 13 +++++++++++--
t/t6020-bundle-misc.sh | 13 +++----------
2 files changed, 14 insertions(+), 12 deletions(-)
diff --git a/builtin/clone.c b/builtin/clone.c
index c6357af9498..4b3fedf78ed 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -1227,9 +1227,18 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
if (fd > 0)
close(fd);
+
+ if (has_filter) {
+ strbuf_addf(&key, "remote.%s.promisor", remote_name);
+ git_config_set(key.buf, "true");
+ strbuf_reset(&key);
+
+ strbuf_addf(&key, "remote.%s.partialclonefilter", remote_name);
+ git_config_set(key.buf, expand_list_objects_filter_spec(&header.filter));
+ strbuf_reset(&key);
+ }
+
bundle_header_release(&header);
- if (has_filter)
- die(_("cannot clone from filtered bundle"));
}
transport_set_option(transport, TRANS_OPT_KEEP, "yes");
diff --git a/t/t6020-bundle-misc.sh b/t/t6020-bundle-misc.sh
index 3e6bcbf30cd..f449df00642 100755
--- a/t/t6020-bundle-misc.sh
+++ b/t/t6020-bundle-misc.sh
@@ -555,16 +555,9 @@ do
'
done
-# NEEDSWORK: 'git clone --bare' should be able to clone from a filtered
-# bundle, but that requires a change to promisor/filter config options.
-# For now, we fail gracefully with a helpful error. This behavior can be
-# changed in the future to succeed as much as possible.
-test_expect_success 'cloning from filtered bundle has useful error' '
- git bundle create partial.bdl \
- --all \
- --filter=blob:none &&
- test_must_fail git clone --bare partial.bdl partial 2>err &&
- grep "cannot clone from filtered bundle" err
+test_expect_success 'cloning from filtered bundle works' '
+ git bundle create partial.bdl --all --filter=blob:none &&
+ git clone --bare partial.bdl partial 2>err
'
test_expect_success 'verify catches unreachable, broken prerequisites' '
base-commit: 564d0252ca632e0264ed670534a51d18a689ef5d
--
gitgitgadget
^ permalink raw reply related
* Re: [PATCH v2 2/5] reftable/stack: refactor reloading to use file descriptor
From: Jeff King @ 2024-01-14 10:14 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git, Han-Wen Nienhuys, Junio C Hamano
In-Reply-To: <36b9f6b6240686cc5b0a761b889614fc31f01d34.1704966670.git.ps@pks.im>
On Thu, Jan 11, 2024 at 11:06:43AM +0100, Patrick Steinhardt wrote:
> We're about to introduce a stat(3P)-based caching mechanism to reload
> the list of stacks only when it has changed. In order to avoid race
> conditions this requires us to have a file descriptor available that we
> can use to call fstat(3P) on.
>
> Prepare for this by converting the code to use `fd_read_lines()` so that
> we have the file descriptor readily available.
Coverity noted a case with this series where we might feed a negative
value to fstat(). I'm not sure if it's a bug or not.
The issue is that here:
> @@ -329,9 +330,19 @@ static int reftable_stack_reload_maybe_reuse(struct reftable_stack *st,
> if (tries > 3 && tv_cmp(&now, &deadline) >= 0)
> goto out;
>
> - err = read_lines(st->list_file, &names);
> - if (err < 0)
> - goto out;
> + fd = open(st->list_file, O_RDONLY);
> + if (fd < 0) {
> + if (errno != ENOENT) {
> + err = REFTABLE_IO_ERROR;
> + goto out;
> + }
> +
> + names = reftable_calloc(sizeof(char *));
> + } else {
> + err = fd_read_lines(fd, &names);
> + if (err < 0)
> + goto out;
> + }
...we might end up with fd as "-1" after calling open() on the list
file. For most errors we'll jump to "out", which makes sense. But if we
get ENOENT, we keep going with an empty file-list, which makes sense.
But we then do other stuff with "fd". I think this case is OK:
> @@ -356,12 +367,16 @@ static int reftable_stack_reload_maybe_reuse(struct reftable_stack *st,
> names = NULL;
> free_names(names_after);
> names_after = NULL;
> + close(fd);
> + fd = -1;
We only get here if reftable_stack_reload_once() returned an error,
which it won't do since we feed it a blank set of names (and anyway,
close(-1) is a harmless noop).
But if we actually get to the end of the function, it's more
questionable. As of this patch, it's OK:
> delay = delay + (delay * rand()) / RAND_MAX + 1;
> sleep_millisec(delay);
> }
>
> out:
> + if (fd >= 0)
> + close(fd);
> free_names(names);
> free_names(names_after);
> return err;
But in the next patch we have this hunk:
> @@ -374,7 +375,11 @@ static int reftable_stack_reload_maybe_reuse(struct reftable_stack *st,
> sleep_millisec(delay);
> }
>
> + stat_validity_update(&st->list_validity, fd);
> +
> out:
> + if (err)
> + stat_validity_clear(&st->list_validity);
> if (fd >= 0)
> close(fd);
> free_names(names);
which means we'll feed a negative value to stat_validity_update(). I
think this may be OK, because I'd imagine the only sensible thing to do
is call stat_validity_clear() instead. And using a negative fd means
fstat() will fail, which will cause stat_validity_update() to clear the
validity struct anyway. But I thought it was worth double-checking.
-Peff
^ permalink raw reply
* [PATCH v2] tests: move t0009-prio-queue.sh to the new unit testing framework
From: Chandra Pratap via GitGitGadget @ 2024-01-14 8:18 UTC (permalink / raw)
To: git; +Cc: Chandra Pratap, Chandra Pratap
In-Reply-To: <pull.1642.git.1705219829965.gitgitgadget@gmail.com>
From: Chandra Pratap <chandrapratap3519@gmail.com>
t/t0009-prio-queue.sh along with t/helper/test-prio-queue.c unit
tests Git's implementation of a priority queue. Migrate the
test over to the new unit testing framework to simplify debugging
and reduce test run-time. Refactor the required logic and add
a new test case in addition to porting over the original ones in
shell.
Signed-off-by: Chandra Pratap <chandrapratap3519@gmail.com>
---
tests: move t0009-prio-queue.sh to the new unit testing framework
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1642%2FChand-ra%2Fprio-queue-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1642/Chand-ra/prio-queue-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/1642
Range-diff vs v1:
1: ca20abe95ea ! 1: 60ac9b3c259 tests: move t0009-prio-queue.sh to the new unit testing framework
@@ t/unit-tests/t-prio-queue.c (new)
+#define REVERSE_STACK_INPUT "stack", "1", "2", "3", "4", "5", "6", "reverse", "dump"
+#define REVERSE_STACK_EXPECTED 1, 2, 3, 4, 5, 6
+
-+#define TEST_INPUT(INPUT, EXPECTED, name) \
++#define TEST_INPUT(INPUT, EXPECTED, name) \
+ static void test_##name(void) \
-+{ \
-+ const char *input[] = {INPUT}; \
-+ int expected[] = {EXPECTED}; \
-+ int result[INPUT_SIZE]; \
-+ test_prio_queue(input, result); \
++{ \
++ const char *input[] = {INPUT}; \
++ int expected[] = {EXPECTED}; \
++ int result[INPUT_SIZE]; \
++ test_prio_queue(input, result); \
+ check(!memcmp(expected, result, sizeof(expected))); \
+}
+
Makefile | 2 +-
t/helper/test-prio-queue.c | 51 -------------------
t/helper/test-tool.c | 1 -
t/helper/test-tool.h | 1 -
t/t0009-prio-queue.sh | 66 -------------------------
t/unit-tests/t-prio-queue.c | 99 +++++++++++++++++++++++++++++++++++++
6 files changed, 100 insertions(+), 120 deletions(-)
delete mode 100644 t/helper/test-prio-queue.c
delete mode 100755 t/t0009-prio-queue.sh
create mode 100644 t/unit-tests/t-prio-queue.c
diff --git a/Makefile b/Makefile
index 312d95084c1..d5e48e656b3 100644
--- a/Makefile
+++ b/Makefile
@@ -828,7 +828,6 @@ TEST_BUILTINS_OBJS += test-partial-clone.o
TEST_BUILTINS_OBJS += test-path-utils.o
TEST_BUILTINS_OBJS += test-pcre2-config.o
TEST_BUILTINS_OBJS += test-pkt-line.o
-TEST_BUILTINS_OBJS += test-prio-queue.o
TEST_BUILTINS_OBJS += test-proc-receive.o
TEST_BUILTINS_OBJS += test-progress.o
TEST_BUILTINS_OBJS += test-reach.o
@@ -1340,6 +1339,7 @@ THIRD_PARTY_SOURCES += sha1dc/%
UNIT_TEST_PROGRAMS += t-basic
UNIT_TEST_PROGRAMS += t-strbuf
+UNIT_TEST_PROGRAMS += t-prio-queue
UNIT_TEST_PROGS = $(patsubst %,$(UNIT_TEST_BIN)/%$X,$(UNIT_TEST_PROGRAMS))
UNIT_TEST_OBJS = $(patsubst %,$(UNIT_TEST_DIR)/%.o,$(UNIT_TEST_PROGRAMS))
UNIT_TEST_OBJS += $(UNIT_TEST_DIR)/test-lib.o
diff --git a/t/helper/test-prio-queue.c b/t/helper/test-prio-queue.c
deleted file mode 100644
index f0bf255f5f0..00000000000
--- a/t/helper/test-prio-queue.c
+++ /dev/null
@@ -1,51 +0,0 @@
-#include "test-tool.h"
-#include "prio-queue.h"
-
-static int intcmp(const void *va, const void *vb, void *data UNUSED)
-{
- const int *a = va, *b = vb;
- return *a - *b;
-}
-
-static void show(int *v)
-{
- if (!v)
- printf("NULL\n");
- else
- printf("%d\n", *v);
- free(v);
-}
-
-int cmd__prio_queue(int argc UNUSED, const char **argv)
-{
- struct prio_queue pq = { intcmp };
-
- while (*++argv) {
- if (!strcmp(*argv, "get")) {
- void *peek = prio_queue_peek(&pq);
- void *get = prio_queue_get(&pq);
- if (peek != get)
- BUG("peek and get results do not match");
- show(get);
- } else if (!strcmp(*argv, "dump")) {
- void *peek;
- void *get;
- while ((peek = prio_queue_peek(&pq))) {
- get = prio_queue_get(&pq);
- if (peek != get)
- BUG("peek and get results do not match");
- show(get);
- }
- } else if (!strcmp(*argv, "stack")) {
- pq.compare = NULL;
- } else {
- int *v = xmalloc(sizeof(*v));
- *v = atoi(*argv);
- prio_queue_put(&pq, v);
- }
- }
-
- clear_prio_queue(&pq);
-
- return 0;
-}
diff --git a/t/helper/test-tool.c b/t/helper/test-tool.c
index 876cd2dc313..5f591b9718f 100644
--- a/t/helper/test-tool.c
+++ b/t/helper/test-tool.c
@@ -57,7 +57,6 @@ static struct test_cmd cmds[] = {
{ "path-utils", cmd__path_utils },
{ "pcre2-config", cmd__pcre2_config },
{ "pkt-line", cmd__pkt_line },
- { "prio-queue", cmd__prio_queue },
{ "proc-receive", cmd__proc_receive },
{ "progress", cmd__progress },
{ "reach", cmd__reach },
diff --git a/t/helper/test-tool.h b/t/helper/test-tool.h
index 70dd4eba119..b921138d8ec 100644
--- a/t/helper/test-tool.h
+++ b/t/helper/test-tool.h
@@ -50,7 +50,6 @@ int cmd__partial_clone(int argc, const char **argv);
int cmd__path_utils(int argc, const char **argv);
int cmd__pcre2_config(int argc, const char **argv);
int cmd__pkt_line(int argc, const char **argv);
-int cmd__prio_queue(int argc, const char **argv);
int cmd__proc_receive(int argc, const char **argv);
int cmd__progress(int argc, const char **argv);
int cmd__reach(int argc, const char **argv);
diff --git a/t/t0009-prio-queue.sh b/t/t0009-prio-queue.sh
deleted file mode 100755
index eea99107a48..00000000000
--- a/t/t0009-prio-queue.sh
+++ /dev/null
@@ -1,66 +0,0 @@
-#!/bin/sh
-
-test_description='basic tests for priority queue implementation'
-
-TEST_PASSES_SANITIZE_LEAK=true
-. ./test-lib.sh
-
-cat >expect <<'EOF'
-1
-2
-3
-4
-5
-5
-6
-7
-8
-9
-10
-EOF
-test_expect_success 'basic ordering' '
- test-tool prio-queue 2 6 3 10 9 5 7 4 5 8 1 dump >actual &&
- test_cmp expect actual
-'
-
-cat >expect <<'EOF'
-2
-3
-4
-1
-5
-6
-EOF
-test_expect_success 'mixed put and get' '
- test-tool prio-queue 6 2 4 get 5 3 get get 1 dump >actual &&
- test_cmp expect actual
-'
-
-cat >expect <<'EOF'
-1
-2
-NULL
-1
-2
-NULL
-EOF
-test_expect_success 'notice empty queue' '
- test-tool prio-queue 1 2 get get get 1 2 get get get >actual &&
- test_cmp expect actual
-'
-
-cat >expect <<'EOF'
-3
-2
-6
-4
-5
-1
-8
-EOF
-test_expect_success 'stack order' '
- test-tool prio-queue stack 8 1 5 4 6 2 3 dump >actual &&
- test_cmp expect actual
-'
-
-test_done
diff --git a/t/unit-tests/t-prio-queue.c b/t/unit-tests/t-prio-queue.c
new file mode 100644
index 00000000000..07b112f5894
--- /dev/null
+++ b/t/unit-tests/t-prio-queue.c
@@ -0,0 +1,99 @@
+#include "test-lib.h"
+#include "prio-queue.h"
+
+static int intcmp(const void *va, const void *vb, void *data UNUSED)
+{
+ const int *a = va, *b = vb;
+ return *a - *b;
+}
+
+static int show(int *v)
+{
+ int ret = -1;
+ if (v)
+ ret = *v;
+ free(v);
+ return ret;
+}
+
+static int test_prio_queue(const char **input, int *result)
+{
+ struct prio_queue pq = { intcmp };
+ int i = 0;
+
+ while (*input) {
+ if (!strcmp(*input, "get")) {
+ void *peek = prio_queue_peek(&pq);
+ void *get = prio_queue_get(&pq);
+ if (peek != get)
+ BUG("peek and get results do not match");
+ result[i++] = show(get);
+ } else if (!strcmp(*input, "dump")) {
+ void *peek;
+ void *get;
+ while ((peek = prio_queue_peek(&pq))) {
+ get = prio_queue_get(&pq);
+ if (peek != get)
+ BUG("peek and get results do not match");
+ result[i++] = show(get);
+ }
+ } else if (!strcmp(*input, "stack")) {
+ pq.compare = NULL;
+ } else if (!strcmp(*input, "reverse")) {
+ prio_queue_reverse(&pq);
+ } else {
+ int *v = xmalloc(sizeof(*v));
+ *v = atoi(*input);
+ prio_queue_put(&pq, v);
+ }
+ input++;
+ }
+
+ clear_prio_queue(&pq);
+
+ return 0;
+}
+
+#define INPUT_SIZE 6
+
+#define BASIC_INPUT "1", "2", "3", "4", "5", "5", "dump"
+#define BASIC_EXPECTED 1, 2, 3, 4, 5, 5
+
+#define MIXED_PUT_GET_INPUT "6", "2", "4", "get", "5", "3", "get", "get", "1", "dump"
+#define MIXED_PUT_GET_EXPECTED 2, 3, 4, 1, 5, 6
+
+#define EMPTY_QUEUE_INPUT "1", "2", "get", "get", "get", "1", "2", "get", "get", "get"
+#define EMPTY_QUEUE_EXPECTED 1, 2, -1, 1, 2, -1
+
+#define STACK_INPUT "stack", "1", "5", "4", "6", "2", "3", "dump"
+#define STACK_EXPECTED 3, 2, 6, 4, 5, 1
+
+#define REVERSE_STACK_INPUT "stack", "1", "2", "3", "4", "5", "6", "reverse", "dump"
+#define REVERSE_STACK_EXPECTED 1, 2, 3, 4, 5, 6
+
+#define TEST_INPUT(INPUT, EXPECTED, name) \
+ static void test_##name(void) \
+{ \
+ const char *input[] = {INPUT}; \
+ int expected[] = {EXPECTED}; \
+ int result[INPUT_SIZE]; \
+ test_prio_queue(input, result); \
+ check(!memcmp(expected, result, sizeof(expected))); \
+}
+
+TEST_INPUT(BASIC_INPUT, BASIC_EXPECTED, basic)
+TEST_INPUT(MIXED_PUT_GET_INPUT, MIXED_PUT_GET_EXPECTED, mixed)
+TEST_INPUT(EMPTY_QUEUE_INPUT, EMPTY_QUEUE_EXPECTED, empty)
+TEST_INPUT(STACK_INPUT, STACK_EXPECTED, stack)
+TEST_INPUT(REVERSE_STACK_INPUT, REVERSE_STACK_EXPECTED, reverse)
+
+int cmd_main(int argc, const char **argv)
+{
+ TEST(test_basic(), "prio-queue works for basic input");
+ TEST(test_mixed(), "prio-queue works for mixed put & get commands");
+ TEST(test_empty(), "prio-queue works when queue is empty");
+ TEST(test_stack(), "prio-queue works when used as a LIFO stack");
+ TEST(test_reverse(), "prio-queue works when LIFO stack is reversed");
+
+ return test_done();
+}
base-commit: 1a87c842ece327d03d08096395969aca5e0a6996
--
gitgitgadget
^ permalink raw reply related
* Re: [PATCH] tests: move t0009-prio-queue.sh to the new unit testing framework
From: Chandra Pratap @ 2024-01-14 8:15 UTC (permalink / raw)
To: Chandra Pratap via GitGitGadget; +Cc: git, Chandra Pratap
In-Reply-To: <pull.1642.git.1705219829965.gitgitgadget@gmail.com>
Please ignore the patch above, my e-mail client seems to have
messed up the backslash indentation. I will follow up with the
corrected patch shortly.
On Sun, 14 Jan 2024 at 13:40, Chandra Pratap via GitGitGadget
<gitgitgadget@gmail.com> wrote:
>
> From: Chandra Pratap <chandrapratap3519@gmail.com>
>
> t/t0009-prio-queue.sh along with t/helper/test-prio-queue.c unit
> tests Git's implementation of a priority queue. Migrate the
> test over to the new unit testing framework to simplify debugging
> and reduce test run-time. Refactor the required logic and add
> a new test case in addition to porting over the original ones in
> shell.
>
> Signed-off-by: Chandra Pratap <chandrapratap3519@gmail.com>
> ---
> tests: move t0009-prio-queue.sh to the new unit testing framework
>
> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1642%2FChand-ra%2Fprio-queue-v1
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1642/Chand-ra/prio-queue-v1
> Pull-Request: https://github.com/gitgitgadget/git/pull/1642
>
> Makefile | 2 +-
> t/helper/test-prio-queue.c | 51 -------------------
> t/helper/test-tool.c | 1 -
> t/helper/test-tool.h | 1 -
> t/t0009-prio-queue.sh | 66 -------------------------
> t/unit-tests/t-prio-queue.c | 99 +++++++++++++++++++++++++++++++++++++
> 6 files changed, 100 insertions(+), 120 deletions(-)
> delete mode 100644 t/helper/test-prio-queue.c
> delete mode 100755 t/t0009-prio-queue.sh
> create mode 100644 t/unit-tests/t-prio-queue.c
>
> diff --git a/Makefile b/Makefile
> index 312d95084c1..d5e48e656b3 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -828,7 +828,6 @@ TEST_BUILTINS_OBJS += test-partial-clone.o
> TEST_BUILTINS_OBJS += test-path-utils.o
> TEST_BUILTINS_OBJS += test-pcre2-config.o
> TEST_BUILTINS_OBJS += test-pkt-line.o
> -TEST_BUILTINS_OBJS += test-prio-queue.o
> TEST_BUILTINS_OBJS += test-proc-receive.o
> TEST_BUILTINS_OBJS += test-progress.o
> TEST_BUILTINS_OBJS += test-reach.o
> @@ -1340,6 +1339,7 @@ THIRD_PARTY_SOURCES += sha1dc/%
>
> UNIT_TEST_PROGRAMS += t-basic
> UNIT_TEST_PROGRAMS += t-strbuf
> +UNIT_TEST_PROGRAMS += t-prio-queue
> UNIT_TEST_PROGS = $(patsubst %,$(UNIT_TEST_BIN)/%$X,$(UNIT_TEST_PROGRAMS))
> UNIT_TEST_OBJS = $(patsubst %,$(UNIT_TEST_DIR)/%.o,$(UNIT_TEST_PROGRAMS))
> UNIT_TEST_OBJS += $(UNIT_TEST_DIR)/test-lib.o
> diff --git a/t/helper/test-prio-queue.c b/t/helper/test-prio-queue.c
> deleted file mode 100644
> index f0bf255f5f0..00000000000
> --- a/t/helper/test-prio-queue.c
> +++ /dev/null
> @@ -1,51 +0,0 @@
> -#include "test-tool.h"
> -#include "prio-queue.h"
> -
> -static int intcmp(const void *va, const void *vb, void *data UNUSED)
> -{
> - const int *a = va, *b = vb;
> - return *a - *b;
> -}
> -
> -static void show(int *v)
> -{
> - if (!v)
> - printf("NULL\n");
> - else
> - printf("%d\n", *v);
> - free(v);
> -}
> -
> -int cmd__prio_queue(int argc UNUSED, const char **argv)
> -{
> - struct prio_queue pq = { intcmp };
> -
> - while (*++argv) {
> - if (!strcmp(*argv, "get")) {
> - void *peek = prio_queue_peek(&pq);
> - void *get = prio_queue_get(&pq);
> - if (peek != get)
> - BUG("peek and get results do not match");
> - show(get);
> - } else if (!strcmp(*argv, "dump")) {
> - void *peek;
> - void *get;
> - while ((peek = prio_queue_peek(&pq))) {
> - get = prio_queue_get(&pq);
> - if (peek != get)
> - BUG("peek and get results do not match");
> - show(get);
> - }
> - } else if (!strcmp(*argv, "stack")) {
> - pq.compare = NULL;
> - } else {
> - int *v = xmalloc(sizeof(*v));
> - *v = atoi(*argv);
> - prio_queue_put(&pq, v);
> - }
> - }
> -
> - clear_prio_queue(&pq);
> -
> - return 0;
> -}
> diff --git a/t/helper/test-tool.c b/t/helper/test-tool.c
> index 876cd2dc313..5f591b9718f 100644
> --- a/t/helper/test-tool.c
> +++ b/t/helper/test-tool.c
> @@ -57,7 +57,6 @@ static struct test_cmd cmds[] = {
> { "path-utils", cmd__path_utils },
> { "pcre2-config", cmd__pcre2_config },
> { "pkt-line", cmd__pkt_line },
> - { "prio-queue", cmd__prio_queue },
> { "proc-receive", cmd__proc_receive },
> { "progress", cmd__progress },
> { "reach", cmd__reach },
> diff --git a/t/helper/test-tool.h b/t/helper/test-tool.h
> index 70dd4eba119..b921138d8ec 100644
> --- a/t/helper/test-tool.h
> +++ b/t/helper/test-tool.h
> @@ -50,7 +50,6 @@ int cmd__partial_clone(int argc, const char **argv);
> int cmd__path_utils(int argc, const char **argv);
> int cmd__pcre2_config(int argc, const char **argv);
> int cmd__pkt_line(int argc, const char **argv);
> -int cmd__prio_queue(int argc, const char **argv);
> int cmd__proc_receive(int argc, const char **argv);
> int cmd__progress(int argc, const char **argv);
> int cmd__reach(int argc, const char **argv);
> diff --git a/t/t0009-prio-queue.sh b/t/t0009-prio-queue.sh
> deleted file mode 100755
> index eea99107a48..00000000000
> --- a/t/t0009-prio-queue.sh
> +++ /dev/null
> @@ -1,66 +0,0 @@
> -#!/bin/sh
> -
> -test_description='basic tests for priority queue implementation'
> -
> -TEST_PASSES_SANITIZE_LEAK=true
> -. ./test-lib.sh
> -
> -cat >expect <<'EOF'
> -1
> -2
> -3
> -4
> -5
> -5
> -6
> -7
> -8
> -9
> -10
> -EOF
> -test_expect_success 'basic ordering' '
> - test-tool prio-queue 2 6 3 10 9 5 7 4 5 8 1 dump >actual &&
> - test_cmp expect actual
> -'
> -
> -cat >expect <<'EOF'
> -2
> -3
> -4
> -1
> -5
> -6
> -EOF
> -test_expect_success 'mixed put and get' '
> - test-tool prio-queue 6 2 4 get 5 3 get get 1 dump >actual &&
> - test_cmp expect actual
> -'
> -
> -cat >expect <<'EOF'
> -1
> -2
> -NULL
> -1
> -2
> -NULL
> -EOF
> -test_expect_success 'notice empty queue' '
> - test-tool prio-queue 1 2 get get get 1 2 get get get >actual &&
> - test_cmp expect actual
> -'
> -
> -cat >expect <<'EOF'
> -3
> -2
> -6
> -4
> -5
> -1
> -8
> -EOF
> -test_expect_success 'stack order' '
> - test-tool prio-queue stack 8 1 5 4 6 2 3 dump >actual &&
> - test_cmp expect actual
> -'
> -
> -test_done
> diff --git a/t/unit-tests/t-prio-queue.c b/t/unit-tests/t-prio-queue.c
> new file mode 100644
> index 00000000000..98a69790f7e
> --- /dev/null
> +++ b/t/unit-tests/t-prio-queue.c
> @@ -0,0 +1,99 @@
> +#include "test-lib.h"
> +#include "prio-queue.h"
> +
> +static int intcmp(const void *va, const void *vb, void *data UNUSED)
> +{
> + const int *a = va, *b = vb;
> + return *a - *b;
> +}
> +
> +static int show(int *v)
> +{
> + int ret = -1;
> + if (v)
> + ret = *v;
> + free(v);
> + return ret;
> +}
> +
> +static int test_prio_queue(const char **input, int *result)
> +{
> + struct prio_queue pq = { intcmp };
> + int i = 0;
> +
> + while (*input) {
> + if (!strcmp(*input, "get")) {
> + void *peek = prio_queue_peek(&pq);
> + void *get = prio_queue_get(&pq);
> + if (peek != get)
> + BUG("peek and get results do not match");
> + result[i++] = show(get);
> + } else if (!strcmp(*input, "dump")) {
> + void *peek;
> + void *get;
> + while ((peek = prio_queue_peek(&pq))) {
> + get = prio_queue_get(&pq);
> + if (peek != get)
> + BUG("peek and get results do not match");
> + result[i++] = show(get);
> + }
> + } else if (!strcmp(*input, "stack")) {
> + pq.compare = NULL;
> + } else if (!strcmp(*input, "reverse")) {
> + prio_queue_reverse(&pq);
> + } else {
> + int *v = xmalloc(sizeof(*v));
> + *v = atoi(*input);
> + prio_queue_put(&pq, v);
> + }
> + input++;
> + }
> +
> + clear_prio_queue(&pq);
> +
> + return 0;
> +}
> +
> +#define INPUT_SIZE 6
> +
> +#define BASIC_INPUT "1", "2", "3", "4", "5", "5", "dump"
> +#define BASIC_EXPECTED 1, 2, 3, 4, 5, 5
> +
> +#define MIXED_PUT_GET_INPUT "6", "2", "4", "get", "5", "3", "get", "get", "1", "dump"
> +#define MIXED_PUT_GET_EXPECTED 2, 3, 4, 1, 5, 6
> +
> +#define EMPTY_QUEUE_INPUT "1", "2", "get", "get", "get", "1", "2", "get", "get", "get"
> +#define EMPTY_QUEUE_EXPECTED 1, 2, -1, 1, 2, -1
> +
> +#define STACK_INPUT "stack", "1", "5", "4", "6", "2", "3", "dump"
> +#define STACK_EXPECTED 3, 2, 6, 4, 5, 1
> +
> +#define REVERSE_STACK_INPUT "stack", "1", "2", "3", "4", "5", "6", "reverse", "dump"
> +#define REVERSE_STACK_EXPECTED 1, 2, 3, 4, 5, 6
> +
> +#define TEST_INPUT(INPUT, EXPECTED, name) \
> + static void test_##name(void) \
> +{ \
> + const char *input[] = {INPUT}; \
> + int expected[] = {EXPECTED}; \
> + int result[INPUT_SIZE]; \
> + test_prio_queue(input, result); \
> + check(!memcmp(expected, result, sizeof(expected))); \
> +}
> +
> +TEST_INPUT(BASIC_INPUT, BASIC_EXPECTED, basic)
> +TEST_INPUT(MIXED_PUT_GET_INPUT, MIXED_PUT_GET_EXPECTED, mixed)
> +TEST_INPUT(EMPTY_QUEUE_INPUT, EMPTY_QUEUE_EXPECTED, empty)
> +TEST_INPUT(STACK_INPUT, STACK_EXPECTED, stack)
> +TEST_INPUT(REVERSE_STACK_INPUT, REVERSE_STACK_EXPECTED, reverse)
> +
> +int cmd_main(int argc, const char **argv)
> +{
> + TEST(test_basic(), "prio-queue works for basic input");
> + TEST(test_mixed(), "prio-queue works for mixed put & get commands");
> + TEST(test_empty(), "prio-queue works when queue is empty");
> + TEST(test_stack(), "prio-queue works when used as a LIFO stack");
> + TEST(test_reverse(), "prio-queue works when LIFO stack is reversed");
> +
> + return test_done();
> +}
>
> base-commit: 1a87c842ece327d03d08096395969aca5e0a6996
> --
> gitgitgadget
^ permalink raw reply
* [PATCH] tests: move t0009-prio-queue.sh to the new unit testing framework
From: Chandra Pratap via GitGitGadget @ 2024-01-14 8:10 UTC (permalink / raw)
To: git; +Cc: Chandra Pratap, Chandra Pratap
From: Chandra Pratap <chandrapratap3519@gmail.com>
t/t0009-prio-queue.sh along with t/helper/test-prio-queue.c unit
tests Git's implementation of a priority queue. Migrate the
test over to the new unit testing framework to simplify debugging
and reduce test run-time. Refactor the required logic and add
a new test case in addition to porting over the original ones in
shell.
Signed-off-by: Chandra Pratap <chandrapratap3519@gmail.com>
---
tests: move t0009-prio-queue.sh to the new unit testing framework
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1642%2FChand-ra%2Fprio-queue-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1642/Chand-ra/prio-queue-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1642
Makefile | 2 +-
t/helper/test-prio-queue.c | 51 -------------------
t/helper/test-tool.c | 1 -
t/helper/test-tool.h | 1 -
t/t0009-prio-queue.sh | 66 -------------------------
t/unit-tests/t-prio-queue.c | 99 +++++++++++++++++++++++++++++++++++++
6 files changed, 100 insertions(+), 120 deletions(-)
delete mode 100644 t/helper/test-prio-queue.c
delete mode 100755 t/t0009-prio-queue.sh
create mode 100644 t/unit-tests/t-prio-queue.c
diff --git a/Makefile b/Makefile
index 312d95084c1..d5e48e656b3 100644
--- a/Makefile
+++ b/Makefile
@@ -828,7 +828,6 @@ TEST_BUILTINS_OBJS += test-partial-clone.o
TEST_BUILTINS_OBJS += test-path-utils.o
TEST_BUILTINS_OBJS += test-pcre2-config.o
TEST_BUILTINS_OBJS += test-pkt-line.o
-TEST_BUILTINS_OBJS += test-prio-queue.o
TEST_BUILTINS_OBJS += test-proc-receive.o
TEST_BUILTINS_OBJS += test-progress.o
TEST_BUILTINS_OBJS += test-reach.o
@@ -1340,6 +1339,7 @@ THIRD_PARTY_SOURCES += sha1dc/%
UNIT_TEST_PROGRAMS += t-basic
UNIT_TEST_PROGRAMS += t-strbuf
+UNIT_TEST_PROGRAMS += t-prio-queue
UNIT_TEST_PROGS = $(patsubst %,$(UNIT_TEST_BIN)/%$X,$(UNIT_TEST_PROGRAMS))
UNIT_TEST_OBJS = $(patsubst %,$(UNIT_TEST_DIR)/%.o,$(UNIT_TEST_PROGRAMS))
UNIT_TEST_OBJS += $(UNIT_TEST_DIR)/test-lib.o
diff --git a/t/helper/test-prio-queue.c b/t/helper/test-prio-queue.c
deleted file mode 100644
index f0bf255f5f0..00000000000
--- a/t/helper/test-prio-queue.c
+++ /dev/null
@@ -1,51 +0,0 @@
-#include "test-tool.h"
-#include "prio-queue.h"
-
-static int intcmp(const void *va, const void *vb, void *data UNUSED)
-{
- const int *a = va, *b = vb;
- return *a - *b;
-}
-
-static void show(int *v)
-{
- if (!v)
- printf("NULL\n");
- else
- printf("%d\n", *v);
- free(v);
-}
-
-int cmd__prio_queue(int argc UNUSED, const char **argv)
-{
- struct prio_queue pq = { intcmp };
-
- while (*++argv) {
- if (!strcmp(*argv, "get")) {
- void *peek = prio_queue_peek(&pq);
- void *get = prio_queue_get(&pq);
- if (peek != get)
- BUG("peek and get results do not match");
- show(get);
- } else if (!strcmp(*argv, "dump")) {
- void *peek;
- void *get;
- while ((peek = prio_queue_peek(&pq))) {
- get = prio_queue_get(&pq);
- if (peek != get)
- BUG("peek and get results do not match");
- show(get);
- }
- } else if (!strcmp(*argv, "stack")) {
- pq.compare = NULL;
- } else {
- int *v = xmalloc(sizeof(*v));
- *v = atoi(*argv);
- prio_queue_put(&pq, v);
- }
- }
-
- clear_prio_queue(&pq);
-
- return 0;
-}
diff --git a/t/helper/test-tool.c b/t/helper/test-tool.c
index 876cd2dc313..5f591b9718f 100644
--- a/t/helper/test-tool.c
+++ b/t/helper/test-tool.c
@@ -57,7 +57,6 @@ static struct test_cmd cmds[] = {
{ "path-utils", cmd__path_utils },
{ "pcre2-config", cmd__pcre2_config },
{ "pkt-line", cmd__pkt_line },
- { "prio-queue", cmd__prio_queue },
{ "proc-receive", cmd__proc_receive },
{ "progress", cmd__progress },
{ "reach", cmd__reach },
diff --git a/t/helper/test-tool.h b/t/helper/test-tool.h
index 70dd4eba119..b921138d8ec 100644
--- a/t/helper/test-tool.h
+++ b/t/helper/test-tool.h
@@ -50,7 +50,6 @@ int cmd__partial_clone(int argc, const char **argv);
int cmd__path_utils(int argc, const char **argv);
int cmd__pcre2_config(int argc, const char **argv);
int cmd__pkt_line(int argc, const char **argv);
-int cmd__prio_queue(int argc, const char **argv);
int cmd__proc_receive(int argc, const char **argv);
int cmd__progress(int argc, const char **argv);
int cmd__reach(int argc, const char **argv);
diff --git a/t/t0009-prio-queue.sh b/t/t0009-prio-queue.sh
deleted file mode 100755
index eea99107a48..00000000000
--- a/t/t0009-prio-queue.sh
+++ /dev/null
@@ -1,66 +0,0 @@
-#!/bin/sh
-
-test_description='basic tests for priority queue implementation'
-
-TEST_PASSES_SANITIZE_LEAK=true
-. ./test-lib.sh
-
-cat >expect <<'EOF'
-1
-2
-3
-4
-5
-5
-6
-7
-8
-9
-10
-EOF
-test_expect_success 'basic ordering' '
- test-tool prio-queue 2 6 3 10 9 5 7 4 5 8 1 dump >actual &&
- test_cmp expect actual
-'
-
-cat >expect <<'EOF'
-2
-3
-4
-1
-5
-6
-EOF
-test_expect_success 'mixed put and get' '
- test-tool prio-queue 6 2 4 get 5 3 get get 1 dump >actual &&
- test_cmp expect actual
-'
-
-cat >expect <<'EOF'
-1
-2
-NULL
-1
-2
-NULL
-EOF
-test_expect_success 'notice empty queue' '
- test-tool prio-queue 1 2 get get get 1 2 get get get >actual &&
- test_cmp expect actual
-'
-
-cat >expect <<'EOF'
-3
-2
-6
-4
-5
-1
-8
-EOF
-test_expect_success 'stack order' '
- test-tool prio-queue stack 8 1 5 4 6 2 3 dump >actual &&
- test_cmp expect actual
-'
-
-test_done
diff --git a/t/unit-tests/t-prio-queue.c b/t/unit-tests/t-prio-queue.c
new file mode 100644
index 00000000000..98a69790f7e
--- /dev/null
+++ b/t/unit-tests/t-prio-queue.c
@@ -0,0 +1,99 @@
+#include "test-lib.h"
+#include "prio-queue.h"
+
+static int intcmp(const void *va, const void *vb, void *data UNUSED)
+{
+ const int *a = va, *b = vb;
+ return *a - *b;
+}
+
+static int show(int *v)
+{
+ int ret = -1;
+ if (v)
+ ret = *v;
+ free(v);
+ return ret;
+}
+
+static int test_prio_queue(const char **input, int *result)
+{
+ struct prio_queue pq = { intcmp };
+ int i = 0;
+
+ while (*input) {
+ if (!strcmp(*input, "get")) {
+ void *peek = prio_queue_peek(&pq);
+ void *get = prio_queue_get(&pq);
+ if (peek != get)
+ BUG("peek and get results do not match");
+ result[i++] = show(get);
+ } else if (!strcmp(*input, "dump")) {
+ void *peek;
+ void *get;
+ while ((peek = prio_queue_peek(&pq))) {
+ get = prio_queue_get(&pq);
+ if (peek != get)
+ BUG("peek and get results do not match");
+ result[i++] = show(get);
+ }
+ } else if (!strcmp(*input, "stack")) {
+ pq.compare = NULL;
+ } else if (!strcmp(*input, "reverse")) {
+ prio_queue_reverse(&pq);
+ } else {
+ int *v = xmalloc(sizeof(*v));
+ *v = atoi(*input);
+ prio_queue_put(&pq, v);
+ }
+ input++;
+ }
+
+ clear_prio_queue(&pq);
+
+ return 0;
+}
+
+#define INPUT_SIZE 6
+
+#define BASIC_INPUT "1", "2", "3", "4", "5", "5", "dump"
+#define BASIC_EXPECTED 1, 2, 3, 4, 5, 5
+
+#define MIXED_PUT_GET_INPUT "6", "2", "4", "get", "5", "3", "get", "get", "1", "dump"
+#define MIXED_PUT_GET_EXPECTED 2, 3, 4, 1, 5, 6
+
+#define EMPTY_QUEUE_INPUT "1", "2", "get", "get", "get", "1", "2", "get", "get", "get"
+#define EMPTY_QUEUE_EXPECTED 1, 2, -1, 1, 2, -1
+
+#define STACK_INPUT "stack", "1", "5", "4", "6", "2", "3", "dump"
+#define STACK_EXPECTED 3, 2, 6, 4, 5, 1
+
+#define REVERSE_STACK_INPUT "stack", "1", "2", "3", "4", "5", "6", "reverse", "dump"
+#define REVERSE_STACK_EXPECTED 1, 2, 3, 4, 5, 6
+
+#define TEST_INPUT(INPUT, EXPECTED, name) \
+ static void test_##name(void) \
+{ \
+ const char *input[] = {INPUT}; \
+ int expected[] = {EXPECTED}; \
+ int result[INPUT_SIZE]; \
+ test_prio_queue(input, result); \
+ check(!memcmp(expected, result, sizeof(expected))); \
+}
+
+TEST_INPUT(BASIC_INPUT, BASIC_EXPECTED, basic)
+TEST_INPUT(MIXED_PUT_GET_INPUT, MIXED_PUT_GET_EXPECTED, mixed)
+TEST_INPUT(EMPTY_QUEUE_INPUT, EMPTY_QUEUE_EXPECTED, empty)
+TEST_INPUT(STACK_INPUT, STACK_EXPECTED, stack)
+TEST_INPUT(REVERSE_STACK_INPUT, REVERSE_STACK_EXPECTED, reverse)
+
+int cmd_main(int argc, const char **argv)
+{
+ TEST(test_basic(), "prio-queue works for basic input");
+ TEST(test_mixed(), "prio-queue works for mixed put & get commands");
+ TEST(test_empty(), "prio-queue works when queue is empty");
+ TEST(test_stack(), "prio-queue works when used as a LIFO stack");
+ TEST(test_reverse(), "prio-queue works when LIFO stack is reversed");
+
+ return test_done();
+}
base-commit: 1a87c842ece327d03d08096395969aca5e0a6996
--
gitgitgadget
^ permalink raw reply related
* Re: What's cooking in git.git (Jan 2024, #01; Tue, 2)
From: SZEDER Gábor @ 2024-01-13 23:41 UTC (permalink / raw)
To: Taylor Blau; +Cc: Junio C Hamano, git
In-Reply-To: <ZaMJU6MJ5wZxyLeM@nand.local>
On Sat, Jan 13, 2024 at 05:06:11PM -0500, Taylor Blau wrote:
> Hi Gábor,
>
> Thanks very much for your patience while I figure all of this out. I'm
> confident that with the fixup! below that your test passes in the next
> round of this series.
>
> On Sat, Jan 13, 2024 at 07:35:44PM +0100, SZEDER Gábor wrote:
> > On Wed, Jan 03, 2024 at 10:08:18AM -0800, Junio C Hamano wrote:
> > > Taylor Blau <me@ttaylorr.com> writes:
> > >
> > > >> * tb/path-filter-fix (2023-10-18) 17 commits
> > > >> - bloom: introduce `deinit_bloom_filters()`
> > > >> ...
> > > >> - t/t4216-log-bloom.sh: harden `test_bloom_filters_not_used()`
> > > >>
> > > >> The Bloom filter used for path limited history traversal was broken
> > > >> on systems whose "char" is unsigned; update the implementation and
> > > >> bump the format version to 2.
> > > >>
> > > >> Expecting a reroll.
> > > >> cf. <20231023202212.GA5470@szeder.dev>
> > > >> source: <cover.1697653929.git.me@ttaylorr.com>
> > > >
> > > > I was confused by this one, since I couldn't figure out which tests
> > > > Gábor was referring to here. I responded in [1], but haven't heard back
> > > > since the end of October.
> > > > ...
> > > > [1]: https://lore.kernel.org/git/ZUARCJ1MmqgXfS4i@nand.local/
> >
> > I keep referring to the test in:
> >
> > https://public-inbox.org/git/20230830200218.GA5147@szeder.dev/
> >
> > which, rather disappointingly, is still the only test out there
> > exercising the interaction between split commit graphs and different
> > modified path Bloom filter versions. Note that in that message I
> > mentioned that merging layers with differenet Bloom filter versions
> > seemed to work, but that, alas, is no longer the case, because it's
> > now broken in Taylor's recent iterations of the patch series.
>
> Thanks for clarifying. With all of the different versions of this series
> and the couple of tests that you and I were talking about, I think that
> I got confused and thought you were referring to a different test.
>
> Indeed, the current version of this series (v4) does not pass the test
> in <20230830200218.GA5147@szeder.dev>, but the fix in order for it to
> pass is relatively straightforward.
>
> I have this on top of my local branch as a fixup! and I'll squash it
> down on Tuesday[^1] and send a reroll after double-checking that the fix
> is correct and doesn't conflict with any other parts of the series.
>
> Since it's been so long since I've looked at this, I'll re-read the
> series as a whole with fresh eyes to make sure that everything still
> makes sense.
>
> In any case, here's the patch on top (with a lightly modified version of
> the test you wrote in <20230830200218.GA5147@szeder.dev>:
I certainly hope that I'm just misunderstanding, and you don't
actually imply that this one test in its current form would qualify as
thorough testing... :)
> Subject: [PATCH] fixup! commit-graph: ensure Bloom filters are read with
> consistent settings
>
> Signed-off-by: Taylor Blau <me@ttaylorr.com>
> ---
> commit-graph.c | 3 ++-
> t/t4216-log-bloom.sh | 29 ++++++++++++++++++++++++++++-
> 2 files changed, 30 insertions(+), 2 deletions(-)
>
> diff --git a/commit-graph.c b/commit-graph.c
> index 60fa64d956..82a4632c4e 100644
> --- a/commit-graph.c
> +++ b/commit-graph.c
> @@ -507,7 +507,8 @@ static void validate_mixed_bloom_settings(struct commit_graph *g)
> }
>
> if (g->bloom_filter_settings->bits_per_entry != settings->bits_per_entry ||
> - g->bloom_filter_settings->num_hashes != settings->num_hashes) {
> + g->bloom_filter_settings->num_hashes != settings->num_hashes ||
> + g->bloom_filter_settings->hash_version != settings->hash_version) {
> g->chunk_bloom_indexes = NULL;
> g->chunk_bloom_data = NULL;
> FREE_AND_NULL(g->bloom_filter_settings);
> diff --git a/t/t4216-log-bloom.sh b/t/t4216-log-bloom.sh
> index 569f2b6f8b..d8c42e2e27 100755
> --- a/t/t4216-log-bloom.sh
> +++ b/t/t4216-log-bloom.sh
> @@ -438,7 +438,7 @@ test_expect_success 'setup for mixed Bloom setting tests' '
> done
> '
>
> -test_expect_success 'ensure incompatible Bloom filters are ignored' '
> +test_expect_success 'ensure Bloom filters with incompatible settings are ignored' '
> # Compute Bloom filters with "unusual" settings.
> git -C $repo rev-parse one >in &&
> GIT_TEST_BLOOM_SETTINGS_NUM_HASHES=3 git -C $repo commit-graph write \
> @@ -488,6 +488,33 @@ test_expect_success 'merge graph layers with incompatible Bloom settings' '
> test_must_be_empty err
> '
>
> +test_expect_success 'ensure Bloom filter with incompatible versions are ignored' '
> + rm "$repo/$graph" &&
> +
> + git -C $repo log --oneline --no-decorate -- $CENT >expect &&
> +
> + # Compute v1 Bloom filters for commits at the bottom.
> + git -C $repo rev-parse HEAD^ >in &&
> + git -C $repo commit-graph write --stdin-commits --changed-paths \
> + --split <in &&
> +
> + # Compute v2 Bloomfilters for the rest of the commits at the top.
> + git -C $repo rev-parse HEAD >in &&
> + git -C $repo -c commitGraph.changedPathsVersion=2 commit-graph write \
> + --stdin-commits --changed-paths --split=no-merge <in &&
> +
> + test_line_count = 2 $repo/$chain &&
> +
> + git -C $repo log --oneline --no-decorate -- $CENT >actual 2>err &&
> + test_cmp expect actual &&
> +
> + layer="$(head -n 1 $repo/$chain)" &&
> + cat >expect.err <<-EOF &&
> + warning: disabling Bloom filters for commit-graph layer $SQ$layer$SQ due to incompatible settings
> + EOF
> + test_cmp expect.err err
> +'
> +
> get_first_changed_path_filter () {
> test-tool read-graph bloom-filters >filters.dat &&
> head -n 1 filters.dat
>
> --
> 2.38.0.16.g393fd4c6db
>
> (Excuse the old version of Git here, I'm in the middle of a long-running
> process which checks out older versions of the tree to count the number
> of unused-parameters over time.)
>
> Thanks,
> Taylor
>
> [^1]: Monday is a US holiday, so I likely won't be at my computer.
^ permalink raw reply
* Re: What's cooking in git.git (Jan 2024, #01; Tue, 2)
From: SZEDER Gábor @ 2024-01-13 22:51 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Taylor Blau, git
In-Reply-To: <20240113183544.GA3000857@szeder.dev>
On Sat, Jan 13, 2024 at 07:35:44PM +0100, SZEDER Gábor wrote:
> On Wed, Jan 03, 2024 at 10:08:18AM -0800, Junio C Hamano wrote:
> > Taylor Blau <me@ttaylorr.com> writes:
> >
> > >> * tb/path-filter-fix (2023-10-18) 17 commits
> > >> - bloom: introduce `deinit_bloom_filters()`
> > >> ...
> > >> - t/t4216-log-bloom.sh: harden `test_bloom_filters_not_used()`
> > >>
> > >> The Bloom filter used for path limited history traversal was broken
> > >> on systems whose "char" is unsigned; update the implementation and
> > >> bump the format version to 2.
> > >>
> > >> Expecting a reroll.
> > >> cf. <20231023202212.GA5470@szeder.dev>
> > >> source: <cover.1697653929.git.me@ttaylorr.com>
> > >
> > > I was confused by this one, since I couldn't figure out which tests
> > > Gábor was referring to here. I responded in [1], but haven't heard back
> > > since the end of October.
> > > ...
> > > [1]: https://lore.kernel.org/git/ZUARCJ1MmqgXfS4i@nand.local/
>
> I keep referring to the test in:
>
> https://public-inbox.org/git/20230830200218.GA5147@szeder.dev/
>
> which, rather disappointingly, is still the only test out there
> exercising the interaction between split commit graphs and different
> modified path Bloom filter versions. Note that in that message I
> mentioned that merging layers with differenet Bloom filter versions
> seemed to work, but that, alas, is no longer the case, because it's
> now broken in Taylor's recent iterations of the patch series.
>
> At the risk of sounding like a broken record: the interaction of split
> commit graphs and different Bloom filter versions should be thoroughly
> tested.
On a related note, if current git (I tried current master and v2.43.0)
encounters a commit graph layer containing v2 Bloom filters (created
by current seen) while writing a new commit graph, then it segfaults
dereferencing a NULL 'settings' pointer in
get_or_compute_bloom_filter().
The test below demonstrates this, but it's quite hacky using two
different git versions: it has to be run by an old git version not yet
supporting v2 Bloom filters, and a new git version already supporting
them should be installed at /tmp/git-new/.
diff --git a/t/t4216-log-bloom.sh b/t/t4216-log-bloom.sh
index 2ba0324a69..0464dd68d5 100755
--- a/t/t4216-log-bloom.sh
+++ b/t/t4216-log-bloom.sh
@@ -454,4 +454,33 @@ test_expect_success 'Bloom reader notices out-of-order index offsets' '
test_cmp expect.err err
'
+CENT=$(printf "\302\242")
+test_expect_success 'split commit graph vs changed paths Bloom filter v2 vs old git' '
+ git init split-v2-old &&
+ (
+ cd split-v2-old &&
+ git commit --allow-empty -m "Bloom filters are written but still ignored for root commits :(" &&
+ for i in 1 2 3
+ do
+ echo $i >$CENT &&
+ git add $CENT &&
+ git commit -m "$i" || return 1
+ done &&
+ git log --oneline -- $CENT >expect &&
+
+ # Here we write a commit graph layer containing v2 changed
+ # path Bloom filters using a git binary built from current
+ # 'seen' branch.
+ git rev-parse HEAD^ |
+ /tmp/git-new/bin/git -c commitgraph.changedPathsVersion=2 \
+ commit-graph write --stdin-commits --changed-paths --split &&
+
+ # This is current master, and segfaults.
+ git commit-graph write --reachable --changed-paths &&
+
+ git log --oneline -- $CENT >actual &&
+ test_cmp expect actual
+ )
+'
+
test_done
(gdb) r
Starting program: /home/szeder/src/git/git commit-graph write --reachable --changed-paths
[Thread debugging using libthread_db enabled]
Using host libthread_db library "/lib/x86_64-linux-gnu/libthread_db.so.1".
Program received signal SIGSEGV, Segmentation fault.
0x00005555556b8714 in get_or_compute_bloom_filter (r=0x555555a0e5a0 <the_repo>, c=0x555555a1cdd0, compute_if_not_present=1, settings=0x0, computed=0x7fffffffd634) at bloom.c:253
253 diffopt.max_changes = settings->max_changed_paths;
(gdb) l
248 return NULL;
249
250 repo_diff_setup(r, &diffopt);
251 diffopt.flags.recursive = 1;
252 diffopt.detect_rename = 0;
253 diffopt.max_changes = settings->max_changed_paths;
254 diff_setup_done(&diffopt);
255
256 /* ensure commit is parsed so we have parent information */
257 repo_parse_commit(r, c);
(gdb) bt
#0 0x00005555556b8714 in get_or_compute_bloom_filter (
r=0x555555a0e5a0 <the_repo>, c=0x555555a1cdd0, compute_if_not_present=1,
settings=0x0, computed=0x7fffffffd634) at bloom.c:253
#1 0x00005555556d16d5 in compute_bloom_filters (ctx=0x555555a1c200)
at commit-graph.c:1783
#2 0x00005555556d4329 in write_commit_graph (odb=0x555555a19210,
pack_indexes=0x0, commits=0x7fffffffd7c0,
flags=(COMMIT_GRAPH_WRITE_PROGRESS | COMMIT_GRAPH_WRITE_BLOOM_FILTERS),
opts=0x5555559ef8d0 <write_opts>) at commit-graph.c:2603
#3 0x00005555556d19ab in write_commit_graph_reachable (odb=0x555555a19210,
flags=(COMMIT_GRAPH_WRITE_PROGRESS | COMMIT_GRAPH_WRITE_BLOOM_FILTERS),
opts=0x5555559ef8d0 <write_opts>) at commit-graph.c:1849
#4 0x00005555555a63c2 in graph_write (argc=0, argv=0x7fffffffde20, prefix=0x0)
at builtin/commit-graph.c:294
#5 0x00005555555a66f4 in cmd_commit_graph (argc=3, argv=0x7fffffffde20,
prefix=0x0) at builtin/commit-graph.c:353
#6 0x0000555555575b43 in run_builtin (p=0x5555559da260 <commands+576>,
argc=4, argv=0x7fffffffde20) at git.c:469
#7 0x0000555555575fcb in handle_builtin (argc=4, argv=0x7fffffffde20)
at git.c:724
#8 0x0000555555576272 in run_argv (argcp=0x7fffffffdc7c, argv=0x7fffffffdc70)
at git.c:788
#9 0x000055555557685d in cmd_main (argc=4, argv=0x7fffffffde20) at git.c:923
#10 0x000055555568ad55 in main (argc=5, argv=0x7fffffffde18)
at common-main.c:62
^ permalink raw reply related
* Re: [PATCH v2 2/2] t5541: remove lockfile creation
From: Justin Tobler @ 2024-01-13 22:25 UTC (permalink / raw)
To: Junio C Hamano
Cc: Jeff King, Justin Tobler via GitGitGadget, git,
Patrick Steinhardt
In-Reply-To: <xmqqo7dqbfin.fsf@gitster.g>
Great! Thanks everyone for the help!
-Justin
On Fri, Jan 12, 2024 at 11:58 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> Jeff King <peff@peff.net> writes:
>
> > On Thu, Jan 11, 2024 at 08:24:30PM +0000, Justin Tobler via GitGitGadget wrote:
> >
> >> - # the new branch should not have been created upstream
> >> - test_must_fail git -C "$d" show-ref --verify refs/heads/atomic &&
> >> -
> >> - # upstream should still reflect atomic2, the last thing we pushed
> >> - # successfully
> >> - git rev-parse atomic2 >expected &&
> >> - # ...to other.
> >> - git -C "$d" rev-parse refs/heads/other >actual &&
> >> - test_cmp expected actual &&
> >> -
> >> - # the new branch should not have been created upstream
> >> + # The atomic and other branches should be created upstream.
> >> test_must_fail git -C "$d" show-ref --verify refs/heads/atomic &&
> >> + test_must_fail git -C "$d" show-ref --verify refs/heads/other &&
> >
> > This last comment should say "should not be created", I think?
> >
> > Other than that, both patches look good to me.
>
> Thanks. Will queue with the following and "Acked-by: peff".
>
> diff --git c/t/t5541-http-push-smart.sh w/t/t5541-http-push-smart.sh
> index 9a8bed6c32..71428f3d5c 100755
> --- c/t/t5541-http-push-smart.sh
> +++ w/t/t5541-http-push-smart.sh
> @@ -242,7 +242,7 @@ test_expect_success 'push --atomic fails on server-side errors' '
> # --atomic should cause entire push to be rejected
> test_must_fail git push --atomic "$up" atomic other 2>output &&
>
> - # The atomic and other branches should be created upstream.
> + # The atomic and other branches should not be created upstream.
> test_must_fail git -C "$d" show-ref --verify refs/heads/atomic &&
> test_must_fail git -C "$d" show-ref --verify refs/heads/other &&
>
>
^ 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