* [PATCH v4 03/15] replay: start using parse_options API
From: Christian Couder @ 2023-09-07 9:25 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Patrick Steinhardt, Johannes Schindelin,
Elijah Newren, John Cai, Derrick Stolee, Phillip Wood, Calvin Wan,
Toon Claes, Christian Couder
In-Reply-To: <20230907092521.733746-1-christian.couder@gmail.com>
From: Elijah Newren <newren@gmail.com>
Instead of manually parsing arguments, let's start using the parse_options
API. This way this new builtin will look more standard, and in some
upcoming commits will more easily be able to handle more command line
options.
Note that we plan to later use standard revision ranges instead of
hardcoded "<oldbase> <branch>" arguments. When we will use standard
revision ranges, it will be easier to check if there are no spurious
arguments if we keep ARGV[0], so let's call parse_options() with
PARSE_OPT_KEEP_ARGV0 even if we don't need ARGV[0] right now to avoid
some useless code churn.
Co-authored-by: Christian Couder <chriscool@tuxfamily.org>
Signed-off-by: Elijah Newren <newren@gmail.com>
Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
builtin/replay.c | 45 ++++++++++++++++++++++++++++++++-------------
1 file changed, 32 insertions(+), 13 deletions(-)
diff --git a/builtin/replay.c b/builtin/replay.c
index e102749ab6..d6dec7c866 100644
--- a/builtin/replay.c
+++ b/builtin/replay.c
@@ -15,7 +15,7 @@
#include "lockfile.h"
#include "merge-ort.h"
#include "object-name.h"
-#include "read-cache-ll.h"
+#include "parse-options.h"
#include "refs.h"
#include "revision.h"
#include "sequencer.h"
@@ -92,6 +92,7 @@ static struct commit *create_commit(struct tree *tree,
int cmd_replay(int argc, const char **argv, const char *prefix)
{
struct commit *onto;
+ const char *onto_name = NULL;
struct commit *last_commit = NULL, *last_picked_commit = NULL;
struct object_id head;
struct lock_file lock = LOCK_INIT;
@@ -105,16 +106,32 @@ int cmd_replay(int argc, const char **argv, const char *prefix)
struct strbuf branch_name = STRBUF_INIT;
int ret = 0;
- if (argc == 2 && !strcmp(argv[1], "-h")) {
- printf("Sorry, I am not a psychiatrist; I can not give you the help you need. Oh, you meant usage...\n");
- exit(129);
+ const char * const replay_usage[] = {
+ N_("git replay --onto <newbase> <oldbase> <branch>"),
+ NULL
+ };
+ struct option replay_options[] = {
+ OPT_STRING(0, "onto", &onto_name,
+ N_("revision"),
+ N_("replay onto given commit")),
+ OPT_END()
+ };
+
+ argc = parse_options(argc, argv, prefix, replay_options, replay_usage,
+ PARSE_OPT_KEEP_ARGV0 | PARSE_OPT_KEEP_UNKNOWN_OPT);
+
+ if (!onto_name) {
+ error(_("option --onto is mandatory"));
+ usage_with_options(replay_usage, replay_options);
}
- if (argc != 5 || strcmp(argv[1], "--onto"))
- die("usage: read the code, figure out how to use it, then do so");
+ if (argc != 3) {
+ error(_("bad number of arguments"));
+ usage_with_options(replay_usage, replay_options);
+ }
- onto = peel_committish(argv[2]);
- strbuf_addf(&branch_name, "refs/heads/%s", argv[4]);
+ onto = peel_committish(onto_name);
+ strbuf_addf(&branch_name, "refs/heads/%s", argv[2]);
/* Sanity check */
if (repo_get_oid(the_repository, "HEAD", &head))
@@ -126,6 +143,7 @@ int cmd_replay(int argc, const char **argv, const char *prefix)
BUG("Could not read index");
repo_init_revisions(the_repository, &revs, prefix);
+
revs.verbose_header = 1;
revs.max_parents = 1;
revs.cherry_mark = 1;
@@ -134,7 +152,8 @@ int cmd_replay(int argc, const char **argv, const char *prefix)
revs.right_only = 1;
revs.sort_order = REV_SORT_IN_GRAPH_ORDER;
revs.topo_order = 1;
- strvec_pushl(&rev_walk_args, "", argv[4], "--not", argv[3], NULL);
+
+ strvec_pushl(&rev_walk_args, "", argv[2], "--not", argv[1], NULL);
if (setup_revisions(rev_walk_args.nr, rev_walk_args.v, &revs, NULL) > 1) {
ret = error(_("unhandled options"));
@@ -197,8 +216,8 @@ int cmd_replay(int argc, const char **argv, const char *prefix)
&last_commit->object.oid,
&last_picked_commit->object.oid,
REF_NO_DEREF, UPDATE_REFS_MSG_ON_ERR)) {
- error(_("could not update %s"), argv[4]);
- die("Failed to update %s", argv[4]);
+ error(_("could not update %s"), argv[2]);
+ die("Failed to update %s", argv[2]);
}
if (create_symref("HEAD", branch_name.buf, reflog_msg.buf) < 0)
die(_("unable to update HEAD"));
@@ -210,8 +229,8 @@ int cmd_replay(int argc, const char **argv, const char *prefix)
&last_commit->object.oid,
&head,
REF_NO_DEREF, UPDATE_REFS_MSG_ON_ERR)) {
- error(_("could not update %s"), argv[4]);
- die("Failed to update %s", argv[4]);
+ error(_("could not update %s"), argv[2]);
+ die("Failed to update %s", argv[2]);
}
}
ret = (result.clean == 0);
--
2.42.0.126.gcf8c984877
^ permalink raw reply related
* Re: [PATCH v2] diff-lib: Fix check_removed when fsmonitor is on
From: Jonathan Tan @ 2023-09-07 17:22 UTC (permalink / raw)
To: Josip Sokcevic; +Cc: Jonathan Tan, git, git
In-Reply-To: <20230907170119.1536694-1-sokcevic@google.com>
Josip Sokcevic <sokcevic@google.com> writes:
> git-diff-index may return incorrect deleted entries when fsmonitor is used in a
> repository with git submodules. This can be observed on Mac machines, but it
> can affect all other supported platforms too.
>
> If fsmonitor is used, `stat *st` may not be initialized. Since `lstat` calls
> aren't not desired when fsmonitor is on, skip the entire gitlink check using
> the same condition used to initialize `stat *st`.
I think this paragraph is outdated - you'll need to update it to match
the code in this version.
> diff --git a/diff-lib.c b/diff-lib.c
> index d8aa777a73..664613bb1b 100644
> --- a/diff-lib.c
> +++ b/diff-lib.c
> @@ -39,11 +39,22 @@
> static int check_removed(const struct index_state *istate, const struct cache_entry *ce, struct stat *st)
> {
> assert(is_fsmonitor_refreshed(istate));
> - if (!(ce->ce_flags & CE_FSMONITOR_VALID) && lstat(ce->name, st) < 0) {
> - if (!is_missing_file_error(errno))
> - return -1;
> - return 1;
> + if (ce->ce_flags & CE_FSMONITOR_VALID) {
> + /*
> + * Both check_removed() and its callers expect lstat() to have
> + * happened and, in particular, the st_mode field to be set.
> + * Simulate this with the contents of ce.
> + */
> + memset(st, 0, sizeof(*st));
> + st->st_mode = ce->ce_mode;
> + } else {
> + if (lstat(ce->name, st) < 0) {
> + if (!is_missing_file_error(errno))
> + return -1;
> + return 1;
> + }
> }
> +
> if (has_symlink_leading_path(ce->name, ce_namelen(ce)))
> return 1;
> if (S_ISDIR(st->st_mode)) {
I'm on the fence about whether the extra newline is necessary - the "if"
did get bigger, but the code is clear enough without the newline. I lean
towards removing it, to avoid cluttering the diff.
^ permalink raw reply
* [PATCH v4 09/15] replay: remove HEAD related sanity check
From: Christian Couder @ 2023-09-07 9:25 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Patrick Steinhardt, Johannes Schindelin,
Elijah Newren, John Cai, Derrick Stolee, Phillip Wood, Calvin Wan,
Toon Claes, Christian Couder
In-Reply-To: <20230907092521.733746-1-christian.couder@gmail.com>
From: Elijah Newren <newren@gmail.com>
We want replay to be a command that can be used on the server side on
any branch, not just the current one, so we are going to stop updating
HEAD in a future commit.
A "sanity check" that makes sure we are replaying the current branch
doesn't make sense anymore. Let's remove it.
Co-authored-by: Christian Couder <chriscool@tuxfamily.org>
Signed-off-by: Elijah Newren <newren@gmail.com>
Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
builtin/replay.c | 8 +-------
1 file changed, 1 insertion(+), 7 deletions(-)
diff --git a/builtin/replay.c b/builtin/replay.c
index b5c854c686..a2636fbdcc 100644
--- a/builtin/replay.c
+++ b/builtin/replay.c
@@ -123,7 +123,6 @@ int cmd_replay(int argc, const char **argv, const char *prefix)
struct commit *onto;
const char *onto_name = NULL;
struct commit *last_commit = NULL, *last_picked_commit = NULL;
- struct object_id head;
struct lock_file lock = LOCK_INIT;
struct strvec rev_walk_args = STRVEC_INIT;
struct rev_info revs;
@@ -162,11 +161,6 @@ int cmd_replay(int argc, const char **argv, const char *prefix)
onto = peel_committish(onto_name);
strbuf_addf(&branch_name, "refs/heads/%s", argv[2]);
- /* Sanity check */
- if (repo_get_oid(the_repository, "HEAD", &head))
- die(_("Cannot read HEAD"));
- assert(oideq(&onto->object.oid, &head));
-
repo_hold_locked_index(the_repository, &lock, LOCK_DIE_ON_ERROR);
if (repo_read_index(the_repository) < 0)
BUG("Could not read index");
@@ -238,7 +232,7 @@ int cmd_replay(int argc, const char **argv, const char *prefix)
oid_to_hex(&last_picked_commit->object.oid));
if (update_ref(reflog_msg.buf, "HEAD",
&last_commit->object.oid,
- &head,
+ &onto->object.oid,
REF_NO_DEREF, UPDATE_REFS_MSG_ON_ERR)) {
error(_("could not update %s"), argv[2]);
die("Failed to update %s", argv[2]);
--
2.42.0.126.gcf8c984877
^ permalink raw reply related
* Re: [PATCH 11/14] replay: use standard revision ranges
From: Christian Couder @ 2023-09-07 8:39 UTC (permalink / raw)
To: Johannes Schindelin
Cc: Elijah Newren, Derrick Stolee, git, Junio C Hamano,
Patrick Steinhardt, John Cai, Christian Couder
In-Reply-To: <f74fb509-0e1a-9542-d80c-0bec2a1e6740@gmx.de>
Hi Dscho,
On Sun, Sep 3, 2023 at 5:47 PM Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
>
> Hi Elijah & Stolee,
>
> On Sat, 29 Apr 2023, Elijah Newren wrote:
>
> > On Mon, Apr 24, 2023 at 8:23 AM Derrick Stolee <derrickstolee@github.com> wrote:
> > > Basically, I'm not super thrilled about exposing options that are
> > > unlikely to be valuable to users and instead are more likely to cause
> > > confusion due to changes that won't successfully apply.
> >
> > Oh, I got thrown by the "right now" portion of your comment; I
> > couldn't see how time or future changes would affect anything to make
> > it less (or more) confusing for users.
> >
> > Quick clarification, though: while you correctly point out the type of
> > confusion the user would experience without my overriding, my
> > overriding of rev.reverse (after setup_revisions() returns, not before
> > it is called) precludes that experience. The override means none of
> > the above happens, and they would instead just wonder why their option
> > is being ignored.
>
> FWIW here is my view on the matter: `git replay`, at least in its current
> incarnation, is a really low-level tool. As such, I actually do not want
> to worry much about protecting users from nonsensical invocations.
>
> In that light, I would like to see that code rejecting all revision
> options except `--diff-algorithm` be dropped. Should we ever decide to add
> a non-low-level mode to `git replay`, we can easily add some user-friendly
> sanity check of the options then, and only for that non-low-level code.
> For now, I feel that it's just complicating things, and `git replay` is in
> the experimental phase anyway.
I would be Ok with removing the patch (called "replay: disallow
revision specific options and pathspecs")
that rejects all revision options and pathspecs if there is a
consensus for that. It might not simplify things too much if there is
still an exception for `--diff-algorithm` though. Also it's not clear
if you are Ok with allowing pathspecs or not.
The idea with disallowing all of them was to later add back those that
make sense along with tests and maybe docs to explain them in the
context of this command. It was not to disallow them permanently. So I
would think the best path forward would be a patch series on top of
this one that would revert the patch disallowing these options and
maybe pathspecs, and instead allow most of them and document and test
things a bit.
> And further, I would even like to see that `--reverse` override go, and
> turn it into `revs.reverse = !revs.reverse` instead. (And yes, I can
> easily think of instances where I would have wanted to reverse a series of
> patches...).
I think this might deserve docs and tests too, so it might want to be
part of a separate patch series once the existing one has graduated.
At this point I don't think it's worth delaying this patch series for
relatively small issues like this. There are many different ways this
new command can be polished and improved. The important thing is that
it looks like we all agree that the new command makes sense and should
have roughly the basic set of features that Elijah originally
implemented, so let's go with this, and then we can improve and
iterate on top of this.
^ permalink raw reply
* Re: Is "bare"ness in the context of multiple worktrees weird? Bitmap error in git gc.
From: Sergey Organov @ 2023-09-07 6:33 UTC (permalink / raw)
To: Tao Klerks
Cc: Junio C Hamano, Kristoffer Haugsbakk, Eric Sunshine,
Johannes Schindelin, Taylor Blau, Patrick Steinhardt, git
In-Reply-To: <CAPMMpojTLswqubRk0Ly3RQqkrnpx_9Hiu_TRK1=ASPbPNz4ApQ@mail.gmail.com>
Tao Klerks <tao@klerks.biz> writes:
> On Wed, Sep 6, 2023 at 10:26 PM Junio C Hamano <gitster@pobox.com> wrote:
>>
>> Tao Klerks <tao@klerks.biz> writes:
>>
>> > I like the nomenclature, I like the simple "zero (i.e. bare) or one
>> > inline worktree, zero or more attached worktrees" explanation.
>>
>> We have used "main worktree" to refer to the working tree part (plus
>> the repository) of a non-bare repository. And it makes sense to
>> explain it together with the concept of "worktree", as the primary
>> one is very much special in that it cannot be removed. You can see
>> that "git worktree remove" would stop you from removing it with an
>> error message:
>>
>> fatal: '../there' is a main working tree.
>>
>> It probably does not add much value to introduce a new term
>> "inline". Here is what "git worktree --help" has to say about it.
>>
>> A repository has one main worktree (if it's not a bare repository) and
>> zero or more linked worktrees.
>
> I've definitely changed my mind about "inline", I agree "main" is
> better. I'm not convinced it's the best word we could come up with,
> but if it's well-established, I'm happy with it.
>
> The problem I (now) see with "inline" is that it seems to imply a
> spatial proximity that doesn't necessarily hold true, with
> "--separate-git-dir" or other ways to separate the main worktree from
> its usual "just above the .git directory" location. "Inline" is still
> a reasonable qualification of the main worktree's *metadata* in that
> situation (index, etc), but I think the word would not be sufficiently
> clear/representative overall.
It's not to argue in favor of "inline", just to clarify: I took it from
inline-vs-attached as used in e-mail, where "inline" means that you see
attachment right here, inline with the rest of text.
I also admit I didn't happen to consider --separate-git-dir at the
time.
^ permalink raw reply
* [PATCH] completion: commit: complete configured trailer tokens
From: Philippe Blain via GitGitGadget @ 2023-09-07 17:42 UTC (permalink / raw)
To: git; +Cc: ZheNing Hu, Philippe Blain, Philippe Blain
From: Philippe Blain <levraiphilippeblain@gmail.com>
Since 2daae3d1d1 (commit: add --trailer option, 2021-03-23), 'git
commit' can add trailers to commit messages. To make that feature more
pleasant to use at the command line, update the Bash completion code to
offer configured trailer tokens.
Add a __git_trailer_tokens function to list the configured trailers
tokens, and use it in _git_commit to suggest the configured tokens,
suffixing the completion words with ':' so that the user only has to add
the trailer value.
Signed-off-by: Philippe Blain <levraiphilippeblain@gmail.com>
---
completion: commit: complete configured trailer tokens
Since 2daae3d1d1 (commit: add --trailer option, 2021-03-23), 'git
commit' can add trailers to commit messages. To make that feature more
pleasant to use at the command line, update the Bash completion code to
offer configured trailer tokens.
Add a __git_trailer_tokens function to list the configured trailers
tokens, and use it in _git_commit to suggest the configured tokens,
suffixing the completion words with ':' so that the user only has to add
the trailer value.
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1583%2Fphil-blain%2Fcompletion-commit-trailers-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1583/phil-blain/completion-commit-trailers-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1583
contrib/completion/git-completion.bash | 9 +++++++++
1 file changed, 9 insertions(+)
diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 133ec92bfae..b5eb75aadc5 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -1677,6 +1677,11 @@ _git_clone ()
__git_untracked_file_modes="all no normal"
+__git_trailer_tokens ()
+{
+ git config --name-only --get-regexp trailer.\*.key | awk -F. '{print $2}'
+}
+
_git_commit ()
{
case "$prev" in
@@ -1701,6 +1706,10 @@ _git_commit ()
__gitcomp "$__git_untracked_file_modes" "" "${cur##--untracked-files=}"
return
;;
+ --trailer=*)
+ __gitcomp_nl "$(__git_trailer_tokens)" "" "${cur##--trailer=}" ":"
+ return
+ ;;
--*)
__gitcomp_builtin commit
return
base-commit: 1fc548b2d6a3596f3e1c1f8b1930d8dbd1e30bf3
--
gitgitgadget
^ permalink raw reply related
* Re: [PATCH v4 00/15] Introduce new `git replay` command
From: Johannes Schindelin @ 2023-09-07 10:25 UTC (permalink / raw)
To: Christian Couder
Cc: git, Junio C Hamano, Patrick Steinhardt, Elijah Newren, John Cai,
Derrick Stolee, Phillip Wood, Calvin Wan, Toon Claes
In-Reply-To: <20230907092521.733746-1-christian.couder@gmail.com>
Hi Christian,
hope you had a restful vacation!
On Thu, 7 Sep 2023, Christian Couder wrote:
> # Changes between v3 and v4
>
> Thanks to Toon, Junio and Dscho for their suggestions on the previous
> version! The very few and minor changes compared to v3 are:
>
> * The patch series was rebased onto master at d814540bb7 (The fifth
> batch, 2023-09-01). This is to fix a few header related conflicts as
> can be seen in the range-diff.
>
> * In patch 10/15 (replay: make it a minimal server side command) a /*
> Cleanup */ code comment has been removed as suggested by Toon.
>
> * In patch 11/15 (replay: use standard revision ranges) the git-replay
> documentation related to --onto has been improved to better explain
> which branches will be updated by the update-ref command(s) in the
> output as suggested by Junio.
>
> * In patch 12/15 (replay: disallow revision specific options and
> pathspecs) an error message has been improved as suggested by Junio.
>
> * In patch 13/15 (replay: add --advance or 'cherry-pick' mode) the
> commit message and the git-replay documentation have been improved
> to better explain that --advance only works when the revision range
> passed has a single tip as suggested by Junio.
>
> * Also in patch 13/15 (replay: add --advance or 'cherry-pick' mode) an
> error message has been improved, and a few tests have been added to
> check that `git replay` fails when it's passed both --advance and
> --onto and when it's passed none of these options, as suggested by
> Toon.
I left a bit of feedback and think that once my concerns are addressed, a
v5 will be ready for `next`.
Ciao,
Johannes
^ permalink raw reply
* Re: [PATCH v3 12/15] replay: disallow revision specific options and pathspecs
From: Christian Couder @ 2023-09-07 8:33 UTC (permalink / raw)
To: Junio C Hamano
Cc: git, Patrick Steinhardt, Johannes Schindelin, Elijah Newren,
John Cai, Derrick Stolee, Phillip Wood, Felipe Contreras,
Calvin Wan, Christian Couder
In-Reply-To: <xmqqy1j3itdw.fsf@gitster.g>
On Tue, Jul 25, 2023 at 11:16 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Christian Couder <christian.couder@gmail.com> writes:
>
> > + /*
> > + * Reject any pathspec. (They are allowed and eaten by
> > + * setup_revisions() above.) In the future we might accept
> > + * them, after adding related tests and doc though.
> > + */
> > + if (revs.prune_data.nr) {
> > + error(_("invalid pathspec: %s"), revs.prune_data.items[0].match);
>
> This made me waste a few minutes wondering if and how I misspelt my
> pathspec elements. If we mean "no pathspec is allowed", we should
> say so instead.
Yeah, right. I have changed this to:
error(_("no pathspec is allowed: '%s'"),
revs.prune_data.items[0].match);
> > + usage_with_options(replay_usage, replay_options);
> > + }
^ permalink raw reply
* Re: [PATCH v4 11/15] replay: use standard revision ranges
From: Johannes Schindelin @ 2023-09-07 10:24 UTC (permalink / raw)
To: Christian Couder
Cc: git, Junio C Hamano, Patrick Steinhardt, Elijah Newren, John Cai,
Derrick Stolee, Phillip Wood, Calvin Wan, Toon Claes,
Christian Couder
In-Reply-To: <20230907092521.733746-12-christian.couder@gmail.com>
Hi Christian,
It is a bit surprising to see the manual page added in _this_ patch, in
the middle of the series... I can live with it, though.
On Thu, 7 Sep 2023, Christian Couder wrote:
> diff --git a/Documentation/git-replay.txt b/Documentation/git-replay.txt
> new file mode 100644
> index 0000000000..9a2087b01a
> --- /dev/null
> +++ b/Documentation/git-replay.txt
> @@ -0,0 +1,90 @@
> +git-replay(1)
> +=============
> +
> +NAME
> +----
> +git-replay - Replay commits on a different base, without touching working tree
> +
> +
> +SYNOPSIS
> +--------
> +[verse]
> +'git replay' --onto <newbase> <revision-range>...
We need to make it clear here, already in the SYNOPSIS, that this is
experimental. Let's add an `(EXPERIMENTAL!)` prefix here.
> [...]
> diff --git a/t/t3650-replay-basics.sh b/t/t3650-replay-basics.sh
> new file mode 100755
Just like the manual page, I would have expected this test to be
introduced earlier, and not piggy-backed onto one of the handful "let's
turn fast-rebase into replay" patches.
Ciao,
Johannes
^ permalink raw reply
* Re: [PATCH v2] diff-lib: Fix check_removed when fsmonitor is on
From: Junio C Hamano @ 2023-09-07 18:07 UTC (permalink / raw)
To: Josip Sokcevic; +Cc: jonathantanmy, git, git
In-Reply-To: <20230907170119.1536694-1-sokcevic@google.com>
Josip Sokcevic <sokcevic@google.com> writes:
> diff --git a/diff-lib.c b/diff-lib.c
> index d8aa777a73..664613bb1b 100644
> --- a/diff-lib.c
> +++ b/diff-lib.c
> @@ -39,11 +39,22 @@
> static int check_removed(const struct index_state *istate, const struct cache_entry *ce, struct stat *st)
> {
> assert(is_fsmonitor_refreshed(istate));
Not a problem this patch introduces, but doesn't this call path
diff_cache()
-> unpack_trees()
-> oneway_diff()
-> do_oneway_diff()
-> show_new_file(), show_modified()
-> get_stat_data()
-> check_removed()
violate the assertion? If so, perhaps we should rewrite it into a
more explicit "if (...) BUG(...)" that is not compiled away.
> - if (!(ce->ce_flags & CE_FSMONITOR_VALID) && lstat(ce->name, st) < 0) {
> - if (!is_missing_file_error(errno))
> - return -1;
> - return 1;
> + if (ce->ce_flags & CE_FSMONITOR_VALID) {
> + /*
> + * Both check_removed() and its callers expect lstat() to have
> + * happened and, in particular, the st_mode field to be set.
> + * Simulate this with the contents of ce.
> + */
> + memset(st, 0, sizeof(*st));
It is true that the original, when CE_FSMONITOR_VALID bit is set,
bypasses lstat() altogether and leaves the contents of st completely
uninitialized, but this is still way too insufficient, isn't it?
There are three call sites of the check_removed() function.
* The first one in run_diff_files() only cares about st.st_mode and
other members of the structure are not looked at. This makes
readers wonder if the "st" parameter to check_removed() should
become "mode_t *st_mode" to clarify this point, but the primary
thing I want to say is that this caller will not mind if we leave
other members of st bogus (like 0-bit filled) as long as the mode
is set correctly.
* The second one in run_diff_files() passes the resulting &st to
match_stat_with_submodule(), which in turn passes it to
ie_match_stat(), which cares about "struct stat" members that are
used for quick change detection, like owner, group, mtime.
Giving it a bogus st will most likely cause it to report a
change.
* The third one is in get_stat_data(). This also uses the &st to
call match_stat_with_submodule(), so it is still totally broken
to give it a bogus st, the same way as the second caller above.
> + st->st_mode = ce->ce_mode;
Does this work correctly when the cache entry points at a gitlink,
which uses 0160000 that is not a valid st_mode? I think you'd want
to use a reverse function of create_ce_mode().
> + } else {
> + if (lstat(ce->name, st) < 0) {
> + if (!is_missing_file_error(errno))
> + return -1;
> + return 1;
> + }
> }
At this point, if FSMONITOR_VALID bit is not set, we will always
perform lstat() and get all the members of st populated properly,
which is a definite improvement.
While I think this does not make it worse (it is an existing bug
that the code is broken for a ce with the CE_FSMONITOR_VALID bit
set), we may want to leave a note that we _know_ the code after this
patch is still broken. "Simulate this with ..." -> "Just setting
st_mode is still insufficient and will break majority of callers".
It may make sense, until we clean it up, to disable the check for
the FSMONITOR_VALID bit in this codepath and always perform lstat().
Optimization matters, but computing quickly in order to return an
incorrect result is optimizing for a wrong thing. I dunno.
Thanks.
^ permalink raw reply
* [PATCH v4 05/15] replay: introduce pick_regular_commit()
From: Christian Couder @ 2023-09-07 9:25 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Patrick Steinhardt, Johannes Schindelin,
Elijah Newren, John Cai, Derrick Stolee, Phillip Wood, Calvin Wan,
Toon Claes, Christian Couder
In-Reply-To: <20230907092521.733746-1-christian.couder@gmail.com>
From: Elijah Newren <newren@gmail.com>
Let's refactor the code to handle a regular commit (a commit that is
neither a root commit nor a merge commit) into a single function instead
of keeping it inside cmd_replay().
This is good for separation of concerns, and this will help further work
in the future to replay merge commits.
Co-authored-by: Christian Couder <chriscool@tuxfamily.org>
Signed-off-by: Elijah Newren <newren@gmail.com>
Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
builtin/replay.c | 54 ++++++++++++++++++++++++++++++------------------
1 file changed, 34 insertions(+), 20 deletions(-)
diff --git a/builtin/replay.c b/builtin/replay.c
index f3fdbe48c9..c66888679b 100644
--- a/builtin/replay.c
+++ b/builtin/replay.c
@@ -89,6 +89,35 @@ static struct commit *create_commit(struct tree *tree,
return (struct commit *)obj;
}
+static struct commit *pick_regular_commit(struct commit *pickme,
+ struct commit *last_commit,
+ struct merge_options *merge_opt,
+ struct merge_result *result)
+{
+ struct commit *base;
+ struct tree *pickme_tree, *base_tree;
+
+ base = pickme->parents->item;
+
+ pickme_tree = repo_get_commit_tree(the_repository, pickme);
+ base_tree = repo_get_commit_tree(the_repository, base);
+
+ merge_opt->branch2 = short_commit_name(pickme);
+ merge_opt->ancestor = xstrfmt("parent of %s", merge_opt->branch2);
+
+ merge_incore_nonrecursive(merge_opt,
+ base_tree,
+ result->tree,
+ pickme_tree,
+ result);
+
+ free((char*)merge_opt->ancestor);
+ merge_opt->ancestor = NULL;
+ if (!result->clean)
+ return NULL;
+ return create_commit(result->tree, pickme, last_commit);
+}
+
int cmd_replay(int argc, const char **argv, const char *prefix)
{
struct commit *onto;
@@ -100,7 +129,7 @@ int cmd_replay(int argc, const char **argv, const char *prefix)
struct rev_info revs;
struct commit *commit;
struct merge_options merge_opt;
- struct tree *next_tree, *base_tree, *head_tree;
+ struct tree *head_tree;
struct merge_result result;
struct strbuf reflog_msg = STRBUF_INIT;
struct strbuf branch_name = STRBUF_INIT;
@@ -175,7 +204,7 @@ int cmd_replay(int argc, const char **argv, const char *prefix)
result.tree = head_tree;
last_commit = onto;
while ((commit = get_revision(&revs))) {
- struct commit *base;
+ struct commit *pick;
fprintf(stderr, "Rebasing %s...\r",
oid_to_hex(&commit->object.oid));
@@ -185,26 +214,11 @@ int cmd_replay(int argc, const char **argv, const char *prefix)
if (commit->parents->next)
die(_("replaying merge commits is not supported yet!"));
- base = commit->parents->item;
-
- next_tree = repo_get_commit_tree(the_repository, commit);
- base_tree = repo_get_commit_tree(the_repository, base);
-
- merge_opt.branch2 = short_commit_name(commit);
- merge_opt.ancestor = xstrfmt("parent of %s", merge_opt.branch2);
-
- merge_incore_nonrecursive(&merge_opt,
- base_tree,
- result.tree,
- next_tree,
- &result);
-
- free((char*)merge_opt.ancestor);
- merge_opt.ancestor = NULL;
- if (!result.clean)
+ pick = pick_regular_commit(commit, last_commit, &merge_opt, &result);
+ if (!pick)
break;
+ last_commit = pick;
last_picked_commit = commit;
- last_commit = create_commit(result.tree, commit, last_commit);
}
merge_finalize(&merge_opt, &result);
--
2.42.0.126.gcf8c984877
^ permalink raw reply related
* [PATCH v4 02/15] replay: introduce new builtin
From: Christian Couder @ 2023-09-07 9:25 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Patrick Steinhardt, Johannes Schindelin,
Elijah Newren, John Cai, Derrick Stolee, Phillip Wood, Calvin Wan,
Toon Claes, Christian Couder
In-Reply-To: <20230907092521.733746-1-christian.couder@gmail.com>
From: Elijah Newren <newren@gmail.com>
For now, this is just a rename from `t/helper/test-fast-rebase.c` into
`builtin/replay.c` with minimal changes to make it build appropriately.
Subsequent commits will flesh out its capabilities and make it a more
standard regular builtin.
Co-authored-by: Christian Couder <chriscool@tuxfamily.org>
Signed-off-by: Elijah Newren <newren@gmail.com>
Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
.gitignore | 1 +
Makefile | 2 +-
builtin.h | 1 +
.../test-fast-rebase.c => builtin/replay.c | 27 ++++++-------------
command-list.txt | 1 +
git.c | 1 +
t/helper/test-tool.c | 1 -
t/helper/test-tool.h | 1 -
t/t6429-merge-sequence-rename-caching.sh | 27 +++++++------------
9 files changed, 22 insertions(+), 40 deletions(-)
rename t/helper/test-fast-rebase.c => builtin/replay.c (89%)
diff --git a/.gitignore b/.gitignore
index 5e56e471b3..612c0f6a0f 100644
--- a/.gitignore
+++ b/.gitignore
@@ -135,6 +135,7 @@
/git-remote-ext
/git-repack
/git-replace
+/git-replay
/git-request-pull
/git-rerere
/git-reset
diff --git a/Makefile b/Makefile
index 5776309365..05a504dc28 100644
--- a/Makefile
+++ b/Makefile
@@ -799,7 +799,6 @@ TEST_BUILTINS_OBJS += test-dump-split-index.o
TEST_BUILTINS_OBJS += test-dump-untracked-cache.o
TEST_BUILTINS_OBJS += test-env-helper.o
TEST_BUILTINS_OBJS += test-example-decorate.o
-TEST_BUILTINS_OBJS += test-fast-rebase.o
TEST_BUILTINS_OBJS += test-fsmonitor-client.o
TEST_BUILTINS_OBJS += test-genrandom.o
TEST_BUILTINS_OBJS += test-genzeros.o
@@ -1287,6 +1286,7 @@ BUILTIN_OBJS += builtin/remote-fd.o
BUILTIN_OBJS += builtin/remote.o
BUILTIN_OBJS += builtin/repack.o
BUILTIN_OBJS += builtin/replace.o
+BUILTIN_OBJS += builtin/replay.o
BUILTIN_OBJS += builtin/rerere.o
BUILTIN_OBJS += builtin/reset.o
BUILTIN_OBJS += builtin/rev-list.o
diff --git a/builtin.h b/builtin.h
index d560baa661..28280636da 100644
--- a/builtin.h
+++ b/builtin.h
@@ -211,6 +211,7 @@ int cmd_remote(int argc, const char **argv, const char *prefix);
int cmd_remote_ext(int argc, const char **argv, const char *prefix);
int cmd_remote_fd(int argc, const char **argv, const char *prefix);
int cmd_repack(int argc, const char **argv, const char *prefix);
+int cmd_replay(int argc, const char **argv, const char *prefix);
int cmd_rerere(int argc, const char **argv, const char *prefix);
int cmd_reset(int argc, const char **argv, const char *prefix);
int cmd_restore(int argc, const char **argv, const char *prefix);
diff --git a/t/helper/test-fast-rebase.c b/builtin/replay.c
similarity index 89%
rename from t/helper/test-fast-rebase.c
rename to builtin/replay.c
index 2bfab66b1b..e102749ab6 100644
--- a/t/helper/test-fast-rebase.c
+++ b/builtin/replay.c
@@ -1,17 +1,11 @@
/*
- * "git fast-rebase" builtin command
- *
- * FAST: Forking Any Subprocesses (is) Taboo
- *
- * This is meant SOLELY as a demo of what is possible. sequencer.c and
- * rebase.c should be refactored to use the ideas here, rather than attempting
- * to extend this file to replace those (unless Phillip or Dscho say that
- * refactoring is too hard and we need a clean slate, but I'm guessing that
- * refactoring is the better route).
+ * "git replay" builtin command
*/
#define USE_THE_INDEX_VARIABLE
-#include "test-tool.h"
+#include "git-compat-util.h"
+
+#include "builtin.h"
#include "cache-tree.h"
#include "commit.h"
#include "environment.h"
@@ -27,7 +21,8 @@
#include "sequencer.h"
#include "setup.h"
#include "strvec.h"
-#include "tree.h"
+#include <oidset.h>
+#include <tree.h>
static const char *short_commit_name(struct commit *commit)
{
@@ -94,7 +89,7 @@ static struct commit *create_commit(struct tree *tree,
return (struct commit *)obj;
}
-int cmd__fast_rebase(int argc, const char **argv)
+int cmd_replay(int argc, const char **argv, const char *prefix)
{
struct commit *onto;
struct commit *last_commit = NULL, *last_picked_commit = NULL;
@@ -110,12 +105,6 @@ int cmd__fast_rebase(int argc, const char **argv)
struct strbuf branch_name = STRBUF_INIT;
int ret = 0;
- /*
- * test-tool stuff doesn't set up the git directory by default; need to
- * do that manually.
- */
- setup_git_directory();
-
if (argc == 2 && !strcmp(argv[1], "-h")) {
printf("Sorry, I am not a psychiatrist; I can not give you the help you need. Oh, you meant usage...\n");
exit(129);
@@ -136,7 +125,7 @@ int cmd__fast_rebase(int argc, const char **argv)
if (repo_read_index(the_repository) < 0)
BUG("Could not read index");
- repo_init_revisions(the_repository, &revs, NULL);
+ repo_init_revisions(the_repository, &revs, prefix);
revs.verbose_header = 1;
revs.max_parents = 1;
revs.cherry_mark = 1;
diff --git a/command-list.txt b/command-list.txt
index 54b2a50f5f..d74836ab21 100644
--- a/command-list.txt
+++ b/command-list.txt
@@ -160,6 +160,7 @@ git-reflog ancillarymanipulators complete
git-remote ancillarymanipulators complete
git-repack ancillarymanipulators complete
git-replace ancillarymanipulators complete
+git-replay mainporcelain history
git-request-pull foreignscminterface complete
git-rerere ancillaryinterrogators
git-reset mainporcelain history
diff --git a/git.c b/git.c
index c67e44dd82..7068a184b0 100644
--- a/git.c
+++ b/git.c
@@ -594,6 +594,7 @@ static struct cmd_struct commands[] = {
{ "remote-fd", cmd_remote_fd, NO_PARSEOPT },
{ "repack", cmd_repack, RUN_SETUP },
{ "replace", cmd_replace, RUN_SETUP },
+ { "replay", cmd_replay, RUN_SETUP },
{ "rerere", cmd_rerere, RUN_SETUP },
{ "reset", cmd_reset, RUN_SETUP },
{ "restore", cmd_restore, RUN_SETUP | NEED_WORK_TREE },
diff --git a/t/helper/test-tool.c b/t/helper/test-tool.c
index abe8a785eb..9ca1586de7 100644
--- a/t/helper/test-tool.c
+++ b/t/helper/test-tool.c
@@ -30,7 +30,6 @@ static struct test_cmd cmds[] = {
{ "dump-untracked-cache", cmd__dump_untracked_cache },
{ "env-helper", cmd__env_helper },
{ "example-decorate", cmd__example_decorate },
- { "fast-rebase", cmd__fast_rebase },
{ "fsmonitor-client", cmd__fsmonitor_client },
{ "genrandom", cmd__genrandom },
{ "genzeros", cmd__genzeros },
diff --git a/t/helper/test-tool.h b/t/helper/test-tool.h
index ea2672436c..a03bbfc6b2 100644
--- a/t/helper/test-tool.h
+++ b/t/helper/test-tool.h
@@ -24,7 +24,6 @@ int cmd__dump_untracked_cache(int argc, const char **argv);
int cmd__dump_reftable(int argc, const char **argv);
int cmd__env_helper(int argc, const char **argv);
int cmd__example_decorate(int argc, const char **argv);
-int cmd__fast_rebase(int argc, const char **argv);
int cmd__fsmonitor_client(int argc, const char **argv);
int cmd__genrandom(int argc, const char **argv);
int cmd__genzeros(int argc, const char **argv);
diff --git a/t/t6429-merge-sequence-rename-caching.sh b/t/t6429-merge-sequence-rename-caching.sh
index 75d3fd2dba..7670b72008 100755
--- a/t/t6429-merge-sequence-rename-caching.sh
+++ b/t/t6429-merge-sequence-rename-caching.sh
@@ -71,9 +71,8 @@ test_expect_success 'caching renames does not preclude finding new ones' '
git switch upstream &&
- test-tool fast-rebase --onto HEAD upstream~1 topic &&
+ git replay --onto HEAD upstream~1 topic &&
git reset --hard topic &&
- #git cherry-pick upstream~1..topic
git ls-files >tracked-files &&
test_line_count = 2 tracked-files &&
@@ -141,8 +140,7 @@ test_expect_success 'cherry-pick both a commit and its immediate revert' '
GIT_TRACE2_PERF="$(pwd)/trace.output" &&
export GIT_TRACE2_PERF &&
- test-tool fast-rebase --onto HEAD upstream~1 topic &&
- #git cherry-pick upstream~1..topic &&
+ git replay --onto HEAD upstream~1 topic &&
grep region_enter.*diffcore_rename trace.output >calls &&
test_line_count = 1 calls
@@ -200,9 +198,8 @@ test_expect_success 'rename same file identically, then reintroduce it' '
GIT_TRACE2_PERF="$(pwd)/trace.output" &&
export GIT_TRACE2_PERF &&
- test-tool fast-rebase --onto HEAD upstream~1 topic &&
+ git replay --onto HEAD upstream~1 topic &&
git reset --hard topic &&
- #git cherry-pick upstream~1..topic &&
git ls-files >tracked &&
test_line_count = 2 tracked &&
@@ -278,9 +275,8 @@ test_expect_success 'rename same file identically, then add file to old dir' '
GIT_TRACE2_PERF="$(pwd)/trace.output" &&
export GIT_TRACE2_PERF &&
- test-tool fast-rebase --onto HEAD upstream~1 topic &&
+ git replay --onto HEAD upstream~1 topic &&
git reset --hard topic &&
- #git cherry-pick upstream~1..topic &&
git ls-files >tracked &&
test_line_count = 4 tracked &&
@@ -356,8 +352,7 @@ test_expect_success 'cached dir rename does not prevent noticing later conflict'
GIT_TRACE2_PERF="$(pwd)/trace.output" &&
export GIT_TRACE2_PERF &&
- test_must_fail test-tool fast-rebase --onto HEAD upstream~1 topic >output &&
- #git cherry-pick upstream..topic &&
+ test_must_fail git replay --onto HEAD upstream~1 topic >output &&
grep region_enter.*diffcore_rename trace.output >calls &&
test_line_count = 2 calls
@@ -456,9 +451,8 @@ test_expect_success 'dir rename unneeded, then add new file to old dir' '
GIT_TRACE2_PERF="$(pwd)/trace.output" &&
export GIT_TRACE2_PERF &&
- test-tool fast-rebase --onto HEAD upstream~1 topic &&
+ git replay --onto HEAD upstream~1 topic &&
git reset --hard topic &&
- #git cherry-pick upstream..topic &&
grep region_enter.*diffcore_rename trace.output >calls &&
test_line_count = 2 calls &&
@@ -523,9 +517,8 @@ test_expect_success 'dir rename unneeded, then rename existing file into old dir
GIT_TRACE2_PERF="$(pwd)/trace.output" &&
export GIT_TRACE2_PERF &&
- test-tool fast-rebase --onto HEAD upstream~1 topic &&
+ git replay --onto HEAD upstream~1 topic &&
git reset --hard topic &&
- #git cherry-pick upstream..topic &&
grep region_enter.*diffcore_rename trace.output >calls &&
test_line_count = 3 calls &&
@@ -626,9 +619,8 @@ test_expect_success 'caching renames only on upstream side, part 1' '
GIT_TRACE2_PERF="$(pwd)/trace.output" &&
export GIT_TRACE2_PERF &&
- test-tool fast-rebase --onto HEAD upstream~1 topic &&
+ git replay --onto HEAD upstream~1 topic &&
git reset --hard topic &&
- #git cherry-pick upstream..topic &&
grep region_enter.*diffcore_rename trace.output >calls &&
test_line_count = 1 calls &&
@@ -685,9 +677,8 @@ test_expect_success 'caching renames only on upstream side, part 2' '
GIT_TRACE2_PERF="$(pwd)/trace.output" &&
export GIT_TRACE2_PERF &&
- test-tool fast-rebase --onto HEAD upstream~1 topic &&
+ git replay --onto HEAD upstream~1 topic &&
git reset --hard topic &&
- #git cherry-pick upstream..topic &&
grep region_enter.*diffcore_rename trace.output >calls &&
test_line_count = 2 calls &&
--
2.42.0.126.gcf8c984877
^ permalink raw reply related
* Re: [PATCH 7/8] builtin/repack.c: drop `DELETE_PACK` macro
From: Junio C Hamano @ 2023-09-07 18:16 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: Taylor Blau, git, Jeff King
In-Reply-To: <ZPmHpqHNzXF0Jbu6@tanuki>
Patrick Steinhardt <ps@pks.im> writes:
> If the intent is to make this check a bit prettier, how about we instead
> introduce a helper function like the following:
>
> ```
> static inline int pack_marked_for_deletion(const struct string_list_item *item)
> {
> return (uintptr) item->util & DELETE_PACK;
> }
> ```
Good suggestion.
Or just check if it is NULL (with the new code that only cares about
the NULL-ness). Regardless of the implementation, having such a
helper would document the intent of the code better, especially if
there are multiple places that make that check.
> Other than that the rest of this series looks good to me, thanks.
>
> Patrick
Thanks.
^ permalink raw reply
* [PATCH v4 15/15] replay: stop assuming replayed branches do not diverge
From: Christian Couder @ 2023-09-07 9:25 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Patrick Steinhardt, Johannes Schindelin,
Elijah Newren, John Cai, Derrick Stolee, Phillip Wood, Calvin Wan,
Toon Claes, Christian Couder
In-Reply-To: <20230907092521.733746-1-christian.couder@gmail.com>
From: Elijah Newren <newren@gmail.com>
The replay command is able to replay multiple branches but when some of
them are based on other replayed branches, their commit should be
replayed onto already replayed commits.
For this purpose, let's store the replayed commit and its original
commit in a key value store, so that we can easily find and reuse a
replayed commit instead of the original one.
Co-authored-by: Christian Couder <chriscool@tuxfamily.org>
Signed-off-by: Elijah Newren <newren@gmail.com>
Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
builtin/replay.c | 44 ++++++++++++++++++++++++++--------
t/t3650-replay-basics.sh | 52 ++++++++++++++++++++++++++++++++++++++++
2 files changed, 86 insertions(+), 10 deletions(-)
diff --git a/builtin/replay.c b/builtin/replay.c
index a7d36a639c..5479406863 100644
--- a/builtin/replay.c
+++ b/builtin/replay.c
@@ -223,20 +223,33 @@ static void determine_replay_mode(struct rev_cmdline_info *cmd_info,
strset_clear(&rinfo.positive_refs);
}
+static struct commit *mapped_commit(kh_oid_map_t *replayed_commits,
+ struct commit *commit,
+ struct commit *fallback)
+{
+ khint_t pos = kh_get_oid_map(replayed_commits, commit->object.oid);
+ if (pos == kh_end(replayed_commits))
+ return fallback;
+ return kh_value(replayed_commits, pos);
+}
+
static struct commit *pick_regular_commit(struct commit *pickme,
- struct commit *last_commit,
+ kh_oid_map_t *replayed_commits,
+ struct commit *onto,
struct merge_options *merge_opt,
struct merge_result *result)
{
- struct commit *base;
+ struct commit *base, *replayed_base;
struct tree *pickme_tree, *base_tree;
base = pickme->parents->item;
+ replayed_base = mapped_commit(replayed_commits, base, onto);
+ result->tree = repo_get_commit_tree(the_repository, replayed_base);
pickme_tree = repo_get_commit_tree(the_repository, pickme);
base_tree = repo_get_commit_tree(the_repository, base);
- merge_opt->branch1 = short_commit_name(last_commit);
+ merge_opt->branch1 = short_commit_name(replayed_base);
merge_opt->branch2 = short_commit_name(pickme);
merge_opt->ancestor = xstrfmt("parent of %s", merge_opt->branch2);
@@ -250,7 +263,7 @@ static struct commit *pick_regular_commit(struct commit *pickme,
merge_opt->ancestor = NULL;
if (!result->clean)
return NULL;
- return create_commit(result->tree, pickme, last_commit);
+ return create_commit(result->tree, pickme, replayed_base);
}
int cmd_replay(int argc, const char **argv, const char *prefix)
@@ -266,6 +279,7 @@ int cmd_replay(int argc, const char **argv, const char *prefix)
struct merge_options merge_opt;
struct merge_result result;
struct strset *update_refs = NULL;
+ kh_oid_map_t *replayed_commits;
int ret = 0, i;
const char * const replay_usage[] = {
@@ -348,21 +362,30 @@ int cmd_replay(int argc, const char **argv, const char *prefix)
init_merge_options(&merge_opt, the_repository);
memset(&result, 0, sizeof(result));
merge_opt.show_rename_progress = 0;
-
- result.tree = repo_get_commit_tree(the_repository, onto);
last_commit = onto;
+ replayed_commits = kh_init_oid_map();
while ((commit = get_revision(&revs))) {
const struct name_decoration *decoration;
+ khint_t pos;
+ int hr;
if (!commit->parents)
die(_("replaying down to root commit is not supported yet!"));
if (commit->parents->next)
die(_("replaying merge commits is not supported yet!"));
- last_commit = pick_regular_commit(commit, last_commit, &merge_opt, &result);
+ last_commit = pick_regular_commit(commit, replayed_commits, onto,
+ &merge_opt, &result);
if (!last_commit)
break;
+ /* Record commit -> last_commit mapping */
+ pos = kh_put_oid_map(replayed_commits, commit->object.oid, &hr);
+ if (hr == 0)
+ BUG("Duplicate rewritten commit: %s\n",
+ oid_to_hex(&commit->object.oid));
+ kh_value(replayed_commits, pos) = last_commit;
+
/* Update any necessary branches */
if (advance_name)
continue;
@@ -391,13 +414,14 @@ int cmd_replay(int argc, const char **argv, const char *prefix)
}
merge_finalize(&merge_opt, &result);
- ret = result.clean;
-
-cleanup:
+ kh_destroy_oid_map(replayed_commits);
if (update_refs) {
strset_clear(update_refs);
free(update_refs);
}
+ ret = result.clean;
+
+cleanup:
release_revisions(&revs);
/* Return */
diff --git a/t/t3650-replay-basics.sh b/t/t3650-replay-basics.sh
index 57d2ef9ea4..fe5ae0391d 100755
--- a/t/t3650-replay-basics.sh
+++ b/t/t3650-replay-basics.sh
@@ -159,4 +159,56 @@ test_expect_success 'using replay on bare repo to also rebase a contained branch
test_cmp expect result-bare
'
+test_expect_success 'using replay to rebase multiple divergent branches' '
+ git replay --onto main ^topic1 topic2 topic4 >result &&
+
+ test_line_count = 2 result &&
+ cut -f 3 -d " " result >new-branch-tips &&
+
+ git log --format=%s $(head -n 1 new-branch-tips) >actual &&
+ test_write_lines E D M L B A >expect &&
+ test_cmp expect actual &&
+
+ git log --format=%s $(tail -n 1 new-branch-tips) >actual &&
+ test_write_lines J I M L B A >expect &&
+ test_cmp expect actual &&
+
+ printf "update refs/heads/topic2 " >expect &&
+ printf "%s " $(head -n 1 new-branch-tips) >>expect &&
+ git rev-parse topic2 >>expect &&
+ printf "update refs/heads/topic4 " >>expect &&
+ printf "%s " $(tail -n 1 new-branch-tips) >>expect &&
+ git rev-parse topic4 >>expect &&
+
+ test_cmp expect result
+'
+
+test_expect_success 'using replay on bare repo to rebase multiple divergent branches, including contained ones' '
+ git -C bare replay --contained --onto main ^main topic2 topic3 topic4 >result &&
+
+ test_line_count = 4 result &&
+ cut -f 3 -d " " result >new-branch-tips &&
+
+ >expect &&
+ for i in 2 1 3 4
+ do
+ printf "update refs/heads/topic$i " >>expect &&
+ printf "%s " $(grep topic$i result | cut -f 3 -d " ") >>expect &&
+ git -C bare rev-parse topic$i >>expect || return 1
+ done &&
+
+ test_cmp expect result &&
+
+ test_write_lines F C M L B A >expect1 &&
+ test_write_lines E D C M L B A >expect2 &&
+ test_write_lines H G F C M L B A >expect3 &&
+ test_write_lines J I M L B A >expect4 &&
+
+ for i in 1 2 3 4
+ do
+ git -C bare log --format=%s $(grep topic$i result | cut -f 3 -d " ") >actual &&
+ test_cmp expect$i actual || return 1
+ done
+'
+
test_done
--
2.42.0.126.gcf8c984877
^ permalink raw reply related
* Re: Is "bare"ness in the context of multiple worktrees weird? Bitmap error in git gc.
From: Junio C Hamano @ 2023-09-07 18:23 UTC (permalink / raw)
To: Kristoffer Haugsbakk
Cc: Sergey Organov, Eric Sunshine, Johannes Schindelin, Taylor Blau,
Patrick Steinhardt, git, Tao Klerks
In-Reply-To: <d59a97e7-81fd-472b-9a18-32d993f8c1c8@app.fastmail.com>
"Kristoffer Haugsbakk" <code@khaugsbakk.name> writes:
>> fatal: '../there' is a main working tree.
>
> This gives the same error if `there` is a bare repository. Is that
> intended?
I do not think so. The above is from "git worktree remove" and the
candidates to be removed is listed by "git worktree list". I do not
know if it is sensible to include the bare repository that the
worktrees are attached to in the "list" output, and I do not think
it makes sense to accept the path to the directory that is such a
bare repository and let the code proceed that far. It should just
reject it saying it is *not* a worktree.
> PS: Is it correct that the error message says “main working tree” instead
> of “main worktree”? (See cc73385cf6 (worktree remove: new command,
> 2018-02-12.) I was thinking of spelunking the history further but thought
> that I would quickly ask in case I'm missing something obvious.
My understanding is that "working tree" refers to what "git
checkout" would give you to your "make" and compilers. The
"worktree" is a mechanism to allow you to have multiple "working
tree"s that are connected to a single repository (be it a bare or a
non-bare one).
> Certainly. But although this looks like it completely describes everything
> that you want, I still think it is good to explicitly mention something
> like:
>
> “ Note that a bare repository may have ...
>
> Since although this can certainly be inferred from the text, it's good to
> have some redundancy when it comes to non-obvious cases.
That is fine.
I think I've already said everything that I think should be in the
final text, and I do not mind if there are anything extra for
helping new readers that may be more than absolute minimum.
Thanks.
^ permalink raw reply
* Re: [PATCH 1/2] ci: allow branch selection through "vars"
From: Jeff King @ 2023-09-07 7:47 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git
In-Reply-To: <44a5d2d4-a6f1-7259-504e-269ee389c8ea@gmx.de>
On Tue, Sep 05, 2023 at 12:51:14PM +0200, Johannes Schindelin wrote:
> Thank you for asking my opinion. The `[no ci]` support described in
> https://github.blog/changelog/2021-02-08-github-actions-skip-pull-request-and-push-workflows-with-skip-ci/
> solves the problem adequately and with a lot less complexity than the
> current or the `vars.`-based solution. In my opinion.
Unfortunately that doesn't work well for the uses cases allow-ref was
meant to support, for the reasons given in e76eec3554 (ci: allow
per-branch config for GitHub Actions, 2020-05-07).
-Peff
^ permalink raw reply
* Re: What's cooking in git.git (Sep 2023, #02; Tue, 5)
From: Jeff King @ 2023-09-07 7:51 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <xmqqtts8nl67.fsf@gitster.g>
On Tue, Sep 05, 2023 at 06:29:52PM -0700, Junio C Hamano wrote:
> * jk/ci-retire-allow-ref (2023-08-30) 2 commits
> (merged to 'next' on 2023-08-31 at 5fe4861f16)
> + ci: deprecate ci/config/allow-ref script
> + ci: allow branch selection through "vars"
>
> CI update.
>
> Will merge to 'master'.
> source: <20230830194919.GA1709446@coredump.intra.peff.net>
I think we should pause on this one. While I am excited to see it master
(since after all it really does nothing until branches actually
incorporate it), the comments from Phillip and Johannes make me think we
may be better off re-designing it as a JSON-formatted CI_CONFIG
variable.
I don't think we'd have to care about backwards compatibility with
CI_BRANCHES that made it into master, since our target audience is Git
developers. But it's nicer not to litter failed experiments into the
history.
I'll see if I can work up a CI_CONFIG patch to replace it, but in the
meantime let's not advance the topic any further.
-Peff
^ permalink raw reply
* Re: [PATCH 1/8] builtin/repack.c: extract structure to store existing packs
From: Jeff King @ 2023-09-07 7:54 UTC (permalink / raw)
To: Taylor Blau; +Cc: git, Junio C Hamano, Patrick Steinhardt
In-Reply-To: <5b48b7e3cc03c83465a3dcecaa98b9d2e9667084.1693946195.git.me@ttaylorr.com>
On Tue, Sep 05, 2023 at 04:36:40PM -0400, Taylor Blau wrote:
> The repack machinery needs to keep track of which packfiles were present
> in the repository at the beginning of a repack, segmented by whether or
> not each pack is marked as kept.
>
> The names of these packs are stored in two `string_list`s, corresponding
> to kept- and non-kept packs, respectively. As a consequence, many
> functions within the repack code need to take both `string_list`s as
> arguments, leading to code like this:
>
> ret = write_cruft_pack(&cruft_po_args, packtmp, pack_prefix,
> cruft_expiration, &names,
> &existing_nonkept_packs, /* <- */
> &existing_kept_packs); /* <- */
>
> Wrap up this pair of `string_list`s into a single structure that stores
> both. This saves us from having to pass both string lists separately,
> and prepares for adding additional fields to this structure.
Makes sense. Even without any additional fields, the grouping makes the
code a bit easier to follow.
Patch is noisy, but looks correct. :)
-Peff
^ permalink raw reply
* Re: [PATCH 2/8] builtin/repack.c: extract marking packs for deletion
From: Jeff King @ 2023-09-07 7:59 UTC (permalink / raw)
To: Taylor Blau; +Cc: git, Junio C Hamano, Patrick Steinhardt
In-Reply-To: <313537ef68892b15e772eaad8937a4a8c7ebbe61.1693946195.git.me@ttaylorr.com>
On Tue, Sep 05, 2023 at 04:36:43PM -0400, Taylor Blau wrote:
> At the end of a repack (when given `-d`), Git attempts to remove any
> packs which have been made "redundant" as a result of the repacking
> operation. For example, an all-into-one (`-A` or `-a`) repack makes
> every pre-existing pack which is not marked as kept redundant. Geometric
> repacks (with `--geometric=<n>`) make any packs which were rolled up
> redundant, and so on.
>
> But before deleting the set of packs we think are redundant, we first
> check to see whether or not we just wrote a pack which is identical to
> any one of the packs we were going to delete. When this is the case, Git
> must avoid deleting that pack, since it matches a pack we just wrote
> (so deleting it may cause the repository to become corrupt).
>
> Right now we only process the list of non-kept packs in a single pass.
> But a future change will split the existing non-kept packs further into
> two lists: one for cruft packs, and another for non-cruft packs.
>
> Factor out this routine to prepare for calling it twice on two separate
> lists in a future patch.
There are really two "factor outs" here: we pull the code from
cmd_repack() into a helper, and then the helper is also just a thin
wrapper around its "_1" variant. That latter part isn't needed yet, but
I can guess from your description that we'll eventually have the main
function dispatch to the "_1" helper for lists.
The main caller in cmd_repack() could do that double-dispatch, but then
this code:
> + if (delete_redundant && pack_everything & ALL_INTO_ONE)
> + mark_packs_for_deletion(&existing, &names);
would know more about how "existing_packs" work than it needs to. So
this seems like a good split (and the two-liner above is making
cmd_repack() much more readable than the big loop it had in the
pre-image).
-Peff
^ permalink raw reply
* Re: [PATCH 3/8] builtin/repack.c: extract redundant pack cleanup for --geometric
From: Jeff King @ 2023-09-07 8:00 UTC (permalink / raw)
To: Taylor Blau; +Cc: git, Junio C Hamano, Patrick Steinhardt
In-Reply-To: <5c25ef87c1430e012a2e48b738b3b5aa760b4b0f.1693946195.git.me@ttaylorr.com>
On Tue, Sep 05, 2023 at 04:36:46PM -0400, Taylor Blau wrote:
> To reduce the complexity of the already quite-long `cmd_repack()`
> implementation, extract out the parts responsible for deleting redundant
> packs from a geometric repack out into its own sub-routine.
Makes sense. Again, I'm happy to see some of these large functional
pieces of cmd_repack() moved into sub-functions. Even if we never call
them twice, IMHO it is a readability improvement to give them meaningful
names.
-Peff
^ permalink raw reply
* Re: [PATCH 4/8] builtin/repack.c: extract redundant pack cleanup for existing packs
From: Jeff King @ 2023-09-07 8:02 UTC (permalink / raw)
To: Taylor Blau; +Cc: git, Junio C Hamano, Patrick Steinhardt
In-Reply-To: <7bb543fef8b140726b6b3efc2bb2cb1a6384fcd9.1693946195.git.me@ttaylorr.com>
On Tue, Sep 05, 2023 at 04:36:48PM -0400, Taylor Blau wrote:
> To remove redundant packs at the end of a repacking operation, Git uses
> its `remove_redundant_pack()` function in a loop over the set of
> pre-existing, non-kept packs.
>
> In a later commit, we will split this list into two, one for
> pre-existing cruft pack(s), and another for non-cruft pack(s). Prepare
> for this by factoring out the routine to loop over and delete redundant
> packs into its own function.
>
> Instead of calling `remove_redundant_pack()` directly, we now will call
> `remove_redundant_existing_packs()`, which itself dispatches a call to
> `remove_redundant_packs_1()`. Note that the geometric repacking code
> will still call `remove_redundant_pack()` directly, but see the previous
> commit for more details.
>
> Having `remove_redundant_packs_1()` exist as a separate function may
> seem like overkill in this patch. However, a later patch will call
> `remove_redundant_packs_1()` once over two separate lists, so this
> refactoring sets us up for that.
Heh, so this is basically the same "_1" case discussed in the earlier
patch. This commit message explains the split a bit better, IMHO. :)
(Not worth re-rolling; just thinking out loud).
-Peff
^ permalink raw reply
* Re: [PATCH 5/8] builtin/repack.c: extract `has_existing_non_kept_packs()`
From: Jeff King @ 2023-09-07 8:04 UTC (permalink / raw)
To: Taylor Blau; +Cc: git, Junio C Hamano, Patrick Steinhardt
In-Reply-To: <e2cf87bb94da1a777f6b87ae3f32e036832839a6.1693946195.git.me@ttaylorr.com>
On Tue, Sep 05, 2023 at 04:36:51PM -0400, Taylor Blau wrote:
> But a following change will store cruft- and non-cruft packs separately,
> meaning this check would break as a result. Prepare for this by
> extracting this part of the check into a new helper function called
> `has_existing_non_kept_packs()`.
>
> This patch does not introduce any functional changes, but prepares us to
> make a more isolated change in a subsequent patch.
OK, that makes sense.
-Peff
^ permalink raw reply
* Re: [PATCH 6/8] builtin/repack.c: store existing cruft packs separately
From: Jeff King @ 2023-09-07 8:09 UTC (permalink / raw)
To: Taylor Blau; +Cc: git, Junio C Hamano, Patrick Steinhardt
In-Reply-To: <414a558883830a29924710126960074b37ab97fc.1693946195.git.me@ttaylorr.com>
On Tue, Sep 05, 2023 at 04:36:54PM -0400, Taylor Blau wrote:
> When repacking with the `--write-midx` option, we invoke the function
> `midx_included_packs()` in order to produce the list of packs we want to
> include in the resulting MIDX.
>
> This list is comprised of:
>
> - existing .keep packs
> - any pack(s) which were written earlier in the same process
> - any unchanged packs when doing a `--geometric` repack
> - any cruft packs
>
> Prior to this patch, we stored pre-existing cruft and non-cruft packs
> together (provided those packs are non-kept). This meant we needed an
> additional bit to indicate which non-kept pack(s) were cruft versus
> those that aren't.
>
> But alternatively we can store cruft packs in a separate list, avoiding
> the need for this extra bit, and simplifying the code below.
OK. Getting rid of the extra bit is nice. We shorten code that only
cares about cruft packs like:
for each pack
if (pack is cruft)
...
to just:
for each cruft_pack
...
which is good. But the flip side is that any existing code which looks
at the combined list now has to do:
for each pack
...
for each cruft_pack
...
I think there's just one case of that, here:
> @@ -707,6 +706,12 @@ static void midx_included_packs(struct string_list *include,
> continue;
> string_list_insert(include, xstrfmt("%s.idx", item->string));
> }
> +
> + for_each_string_list_item(item, &existing->cruft_packs) {
> + if ((uintptr_t)item->util & DELETE_PACK)
> + continue;
> + string_list_insert(include, xstrfmt("%s.idx", item->string));
> + }
> }
> }
It may be an OK price to pay if this lets us keep cleaning things up
(especially if we could get rid of that util casting entirely!). Let's
read on...
-Peff
^ permalink raw reply
* Re: [PATCH] rebase -i: ignore signals when forking subprocesses
From: Johannes Schindelin @ 2023-09-07 12:57 UTC (permalink / raw)
To: Phillip Wood via GitGitGadget; +Cc: git, Jeff King, Phillip Wood, Phillip Wood
In-Reply-To: <pull.1581.git.1694080982621.gitgitgadget@gmail.com>
Hi Phillip,
On Thu, 7 Sep 2023, Phillip Wood via GitGitGadget wrote:
> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>
> If the user presses Ctrl-C to interrupt a program run by a rebase "exec"
> command then SIGINT will also be sent to the git process running the
> rebase resulting in it being killed. Fortunately the consequences of
> this are not severe as all the state necessary to continue the rebase is
> saved to disc but it would be better to avoid killing git and instead
> report that the command failed. A similar situation occurs when the
> sequencer runs "git commit" or "git merge". If the user generates SIGINT
> while editing the commit message then the git processes creating the
> commit will ignore it but the git process running the rebase will be
> killed.
>
> Fix this by ignoring SIGINT and SIGQUIT when forking "exec" commands,
> "git commit" and "git merge". This matches what git already does when
> running the user's editor and matches the behavior of the standard
> library's system() function.
ACK
>
> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
> ---
> rebase -i: ignore signals when forking subprocesses
>
> Having written this I started thinking about what happens when we fork
> hooks, merge strategies and merge drivers. I now wonder if it would be
> better to change run_command() instead - are there any cases where we
> actually want git to be killed when the user interrupts a child process?
I am not sure that we can rely on arbitrary hooks to do the right thing
upon Ctrl+C, which is to wrap up and leave. So I _guess_ that we will have
to leave it an opt-in.
However, we could easily make it an option that `run_command()` handles,
much like `no_stdin`.
Ciao,
Johannes
>
> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1581%2Fphillipwood%2Fsequencer-subprocesses-ignore-sigint-v1
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1581/phillipwood/sequencer-subprocesses-ignore-sigint-v1
> Pull-Request: https://github.com/gitgitgadget/git/pull/1581
>
> sequencer.c | 19 +++++++++++++++++--
> 1 file changed, 17 insertions(+), 2 deletions(-)
>
> diff --git a/sequencer.c b/sequencer.c
> index a66dcf8ab26..26d70f68454 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -1059,6 +1059,7 @@ static int run_git_commit(const char *defmsg,
> unsigned int flags)
> {
> struct child_process cmd = CHILD_PROCESS_INIT;
> + int res;
>
> if ((flags & CLEANUP_MSG) && (flags & VERBATIM_MSG))
> BUG("CLEANUP_MSG and VERBATIM_MSG are mutually exclusive");
> @@ -1116,10 +1117,16 @@ static int run_git_commit(const char *defmsg,
> if (!(flags & EDIT_MSG))
> strvec_push(&cmd.args, "--allow-empty-message");
>
> + sigchain_push(SIGINT, SIG_IGN);
> + sigchain_push(SIGQUIT, SIG_IGN);
> if (is_rebase_i(opts) && !(flags & EDIT_MSG))
> - return run_command_silent_on_success(&cmd);
> + res = run_command_silent_on_success(&cmd);
> else
> - return run_command(&cmd);
> + res = run_command(&cmd);
> + sigchain_pop(SIGINT);
> + sigchain_pop(SIGQUIT);
> +
> + return res;
> }
>
> static int rest_is_empty(const struct strbuf *sb, int start)
> @@ -3628,10 +3635,14 @@ static int do_exec(struct repository *r, const char *command_line)
> struct child_process cmd = CHILD_PROCESS_INIT;
> int dirty, status;
>
> + sigchain_push(SIGINT, SIG_IGN);
> + sigchain_push(SIGQUIT, SIG_IGN);
> fprintf(stderr, _("Executing: %s\n"), command_line);
> cmd.use_shell = 1;
> strvec_push(&cmd.args, command_line);
> status = run_command(&cmd);
> + sigchain_pop(SIGINT);
> + sigchain_pop(SIGQUIT);
>
> /* force re-reading of the cache */
> discard_index(r->index);
> @@ -4111,7 +4122,11 @@ static int do_merge(struct repository *r,
> NULL, 0);
> rollback_lock_file(&lock);
>
> + sigchain_push(SIGINT, SIG_IGN);
> + sigchain_push(SIGQUIT, SIG_IGN);
> ret = run_command(&cmd);
> + sigchain_pop(SIGINT);
> + sigchain_pop(SIGQUIT);
>
> /* force re-reading of the cache */
> if (!ret) {
>
> base-commit: 1fc548b2d6a3596f3e1c1f8b1930d8dbd1e30bf3
> --
> gitgitgadget
>
^ permalink raw reply
* Re: [PATCH 7/8] builtin/repack.c: drop `DELETE_PACK` macro
From: Jeff King @ 2023-09-07 8:58 UTC (permalink / raw)
To: Taylor Blau; +Cc: Junio C Hamano, git, Patrick Steinhardt
In-Reply-To: <ZPi1c98o2fKB/U+e@nand.local>
On Wed, Sep 06, 2023 at 01:22:59PM -0400, Taylor Blau wrote:
> --- 8< ---
> Subject: [PATCH] builtin/repack.c: treat string_list_item util as booleans
>
> The `->util` field corresponding to each string_list_item used to track
> the existence of some pack at the beginning of a repack operation was
> originally intended to be used as a bitfield.
>
> This bitfield tracked:
>
> - (1 << 0): whether or not the pack should be deleted
> - (1 << 1): whether or not the pack is cruft
>
> The previous commit removed the use of the second bit, meaning that we
> can treat this field as a boolean instead of a bitset.
I do think the boolean is syntactically a little nicer than the bit-set,
just because of the casting we have to with the void pointer). After
reading the earlier parts, I was hoping the culmination of this series
would be dropping the use of util entirely (presumably in favor of
separate lists). But maybe that would be too disruptive; I didn't try
it.
> -#define DELETE_PACK 1
> +#define DELETE_PACK ((void*)(uintptr_t)1)
> [...]
> @@ -130,7 +134,7 @@ static void mark_packs_for_deletion_1(struct string_list *names,
> * (if `-d` was given).
> */
> if (!string_list_has_string(names, sha1))
> - item->util = (void*)(uintptr_t)((size_t)item->util | DELETE_PACK);
> + item->util = DELETE_PACK;
> }
> }
I do like the use of the macro here to make the meaning of the boolean
more plain, since the name "util" is totally meaningless (but we are
stuck with it).
But on the other side, things get more mysterious:
> @@ -158,7 +162,7 @@ static void remove_redundant_packs_1(struct string_list *packs)
> {
> struct string_list_item *item;
> for_each_string_list_item(item, packs) {
> - if (!((uintptr_t)item->util & DELETE_PACK))
> + if (!item->util)
> continue;
This is syntactically much nicer, but the meaning of "util" as a boolean
for "we should delete this" is lost.
So I dunno. The end result of this patch is more readable syntactically,
but arguably less so semantically. Unless we have a good reason to ditch
the bit-set entirely, I wonder if we could have the best of both with
some macro helpers. Or even a set of matching helper functions like:
void pack_mark_for_deletion(struct string_list_item *item)
{
(uintptr_t)item->util = 1;
}
int pack_should_deleted(const struct string_list_item *item)
{
return item->util;
}
which could be a bool or a bit-set; the callers no longer need to care
and get to use human-readable names.
I dunno. It is a file-local convention and there aren't that many spots,
so maybe it is not worth worrying too much about. I'm pretty sure that I
got confused by the use of "util" here when looking at the code before,
as it did not have the DELETE_PACK name until your 72263ffc32
(builtin/repack.c: use named flags for existing_packs, 2022-05-20). But
maybe the comment that you added is sufficient.
If we had generic pointer-bitset macros, then perhaps other string-list
users could benefit, too. I thought maybe fast-export could use this for
its mark_to_ptr() stuff, but that is storing a whole 32-bit value, not a
bitset. So maybe this is just a weird localized thing.
A more radical idea is that we don't care very much about the data
structure as long as it is ordered. So we could just do:
struct existing_pack {
struct list_head list;
int to_delete;
char name[FLEX_ARRAY];
};
and ditch string-list entirely. That lets us use "to_delete" in a
natural way. Though I suspect it makes all the _other_ code unreadable,
as we have to allocate them, deal with list_entry(), and so on.
So I guess I'm fine with any path (including the one in this patch).
-Peff
^ 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