* [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
* [PATCH v4 08/15] replay: remove progress and info output
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 will be changed in a follow up commit, so that it
will not update refs directly, but instead it will print on stdout a
list of commands that can be consumed by `git update-ref --stdin`.
We don't want this output to be polluted by its current low value
output, so let's just remove the latter.
In the future, when the command gets an option to update refs by
itself, it will make a lot of sense to display a progress meter, but
we are not there yet.
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 | 7 +------
1 file changed, 1 insertion(+), 6 deletions(-)
diff --git a/builtin/replay.c b/builtin/replay.c
index 47d695df93..b5c854c686 100644
--- a/builtin/replay.c
+++ b/builtin/replay.c
@@ -195,7 +195,7 @@ 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 = 1;
+ merge_opt.show_rename_progress = 0;
merge_opt.branch1 = "HEAD";
head_tree = repo_get_commit_tree(the_repository, onto);
result.tree = head_tree;
@@ -203,9 +203,6 @@ int cmd_replay(int argc, const char **argv, const char *prefix)
while ((commit = get_revision(&revs))) {
struct commit *pick;
- fprintf(stderr, "Rebasing %s...\r",
- oid_to_hex(&commit->object.oid));
-
if (!commit->parents)
die(_("replaying down to root commit is not supported yet!"));
if (commit->parents->next)
@@ -224,7 +221,6 @@ int cmd_replay(int argc, const char **argv, const char *prefix)
exit(128);
if (result.clean) {
- fprintf(stderr, "\nDone.\n");
strbuf_addf(&reflog_msg, "finish rebase %s onto %s",
oid_to_hex(&last_picked_commit->object.oid),
oid_to_hex(&last_commit->object.oid));
@@ -238,7 +234,6 @@ int cmd_replay(int argc, const char **argv, const char *prefix)
if (create_symref("HEAD", branch_name.buf, reflog_msg.buf) < 0)
die(_("unable to update HEAD"));
} else {
- fprintf(stderr, "\nAborting: Hit a conflict.\n");
strbuf_addf(&reflog_msg, "rebase progress up to %s",
oid_to_hex(&last_picked_commit->object.oid));
if (update_ref(reflog_msg.buf, "HEAD",
--
2.42.0.126.gcf8c984877
^ permalink raw reply related
* [PATCH v4 00/15] Introduce new `git replay` command
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: <20230602102533.876905-1-christian.couder@gmail.com>
# Intro
`git replay` has initially been developed entirely by Elijah Newren
mostly last year (2022) at:
https://github.com/newren/git/commits/replay
I took over this year to polish and upstream it as GitLab is
interested in replacing libgit2, and for that purpose needs a command
to do server side (so without using a worktree) rebases, cherry-picks
and reverts.
I reduced the number of commits and features in this first patch
series, compared to what Elijah already developed. Especially I
stopped short of replaying merge commits and replaying
interactively. These and other features might be upstreamed in the
future after this patch series has graduated.
The focus in this series is to make it a good plumbing command that
can already be used server side and that replaces the "fast-rebase"
test-tool command. So things to make it easier to use on the command
line, and more advanced features (like replaying merges) are left out.
It looks like GitHub has actually already been using version 3 of this
patch series in production with good results. See:
https://github.blog/2023-07-27-scaling-merge-ort-across-github/
https://lore.kernel.org/git/304f2a49-5e05-7655-9f87-2011606df5db@gmx.de/
# Content of this cover letter
The "Quick Overview" and "Reasons for diverging from cherry-pick &
rebase" sections just below are describing the purpose of the new
command in the big scheme of things. They are taken from Elijah's
design notes
(https://github.com/newren/git/blob/replay/replay-design-notes.txt)
and describe what we want this command to become and the reasons for
that, not what the command is after only this patch series. Also these
design notes were written at least one year ago, so parts of those 2
sections are not true anymore. I have added Phillip Wood's or Felipe
Contreras' notes (thanks to them) where that's the case, but some now
flawed parts may have missed.
After these two sections, starting with the "Important limitations"
section, you will find sections describing what is actually in this
patch series.
More interesting material is available in Elijah's design notes like
an "Intro via examples"
(https://github.com/newren/git/blob/replay/replay-design-notes.txt#L37-L132),
a discussion about "Preserving topology, replaying merges"
(https://github.com/newren/git/blob/replay/replay-design-notes.txt#L264-L341)
and a "Current status" section describing Elijah's work
(https://github.com/newren/git/blob/replay/replay-design-notes.txt#L344-L392)
before I started working on upstreaming it.
I have not included this material here though, as the documentation
added by this patch series for the `git replay` command already
includes an "EXAMPLES" section, and other sections of Elijah's design
notes might not be interesting for now. Also this cover letter is
already pretty long. But reviewers can refer to the links above if
they think it can help.
# Quick Overview (from Elijah's design notes)
`git replay`, at a basic level, can perhaps be thought of as a
"default-to-dry-run rebase" -- meaning no updates to the working tree,
or to the index, or to any references. However, it differs from
rebase in that it:
* Works for branches that aren't checked out
* Works in a bare repository
* Can replay multiple branches simultaneously (with or without common
history in the range being replayed)
* Preserves relative topology by default (merges are replayed too in
Elijah's original work, not in this series)
* Focuses on performance
* Has several altered defaults as a result of the above
I sometimes think of `git replay` as "fast-replay", a patch-based
analogue to the snapshot-based fast-export & fast-import tools.
# Reasons for diverging from cherry-pick & rebase (from Elijah's
design notes)
There are multiple reasons to diverge from the defaults in cherry-pick and
rebase.
* Server side needs
* Both cherry-pick and rebase, via the sequencer, are heavily tied
to updating the working tree, index, some refs, and a lot of
control files with every commit replayed, and invoke a mess of
hooks[1] that might be hard to avoid for backward compatibility
reasons (at least, that's been brought up a few times on the
list).
* cherry-pick and rebase both fork various subprocesses
unnecessarily, but somewhat intrinsically in part to ensure the
same hooks are called that old scripted implementations would have
called.
Note: since 356ee4659bb (sequencer: try to commit without forking
'git commit', 2017-11-24) cherry-pick and rebase do not fork
subprocesses other than hooks for the cases covered by this patch
series (i.e. they do not fork "git commit" for simple picks).
* "Dry run" behavior, where there are no updates to worktree, index,
or even refs might be important.
* Should not assume users only want to operate on HEAD (see next
section)
* Decapitate HEAD-centric assumptions
* cherry-pick forces commits to be played on top of HEAD;
inflexible.
* rebase assumes the range of commits to be replayed is
upstream..HEAD by default, though it allows one to replay
upstream..otherbranch -- but it still forcibly and needlessly
checks out 'otherbranch' before starting to replay things.
Note: since 767a9c417eb (rebase -i: stop checking out the tip of
the branch to rebase, 2020-01-24) it's not true that rebase
forcibly and needlessly checks out 'otherbranch'.
* Assuming HEAD is involved severely limits replaying multiple
(possibly divergent) branches.
Note: since 89fc0b53fdb (rebase: update refs from 'update-ref'
commands, 2022-07-19) the sequencer can update multiple
branches. The issue with divergent branch is with command line
arguments and the todo list generation rather than the
capabilities of the sequencer.
* Once you stop assuming HEAD has a certain meaning, there's not
much reason to have two separate commands anymore (except for the
funny extra not-necessarily-compatible options both have gained
over time).
* (Micro issue: Assuming HEAD is involved also makes it harder for
new users to learn what rebase means and does; it makes command
lines hard to parse. Not sure I want to harp on this too much, as
I have a suspicion I might be creating a tool for experts with
complicated use cases, but it's a minor quibble.)
* Performance
* jj is slaughtering us on rebase speed[2]. I would like us to become
competitive. (I dropped a few comments in the link at [2] about why
git is currently so bad.)
* From [3], there was a simple 4-patch series in linux.git that took
53 seconds to rebase. Switching to ort dropped it to 16 seconds.
While that sounds great, only 11 *milliseconds* were needed to do
the actual merges. That means almost *all* the time (>99%) was
overhead! Big offenders:
* --reapply-cherry-picks should be the default
* can_fast_forward() should be ripped out, and perhaps other extraneous
revision walks
Note: d42c9ffa0f (rebase: factor out branch_base calculation,
2022-10-17) might already deal with that (according to Felipe
Contreras).
* avoid updating working tree, index, refs, reflogs, and control
structures except when needed (e.g. hitting a conflict, or operation
finished)
* Other performance ideas (mostly for future work, not in this
series)
* single-file control structures instead of directory of files
(when doing interactive things which is in Elijah's original
work, but not in this series)
* avoid forking subprocesses unless explicitly requested (e.g.
--exec, --strategy, --run-hooks). For example, definitely do not
invoke `git commit` or `git merge`.
* Sanitize hooks:
* dispense with all per-commit hooks for sure (pre-commit,
post-commit, post-checkout).
* pre-rebase also seems to assume exactly 1 ref is written, and
invoking it repeatedly would be stupid. Plus, it's specific
to "rebase". So...ignore? (Stolee's --ref-update option for
rebase probably broke the pre-rebase assumptions already...)
* post-rewrite hook might make sense, but fast-import got
exempted, and I think of replay like a patch-based analogue
to the snapshot-based fast-import.
* When not running server side, resolve conflicts in a sparse-cone
sparse-index worktree to reduce number of files written to a
working tree. (See below as well.)
* [High risk of possible premature optimization] Avoid large
numbers of newly created loose objects, when replaying large
numbers of commits. Two possibilities: (1) Consider using
tmp-objdir and pack objects from the tmp-objdir at end of
exercise, (2) Lift code from git-fast-import to immediately
stuff new objects into a pack?
* Multiple branches and non-checked out branches
* The ability to operate on non-checked out branches also implies
that we should generally be able to replay when in a dirty working
tree (exception being when we expect to update HEAD and any of the
dirty files is one that needs to be updated by the replay).
* Also, if we are operating locally on a non-checked out branch and
hit a conflict, we should have a way to resolve the conflict
without messing with the user's work on their current
branch. (This is not is this patch series though.)
* Idea: new worktree with sparse cone + sparse index checkout,
containing only files in the root directory, and whatever is
necessary to get the conflicts
* Companion to above idea: control structures should be written to
$GIT_COMMON_DIR/replay-${worktree}, so users can have multiple
replay sessions, and so we know which worktrees are associated
with which replay operations.
- [1] https://lore.kernel.org/git/pull.749.v3.git.git.1586044818132.gitgitgadget@gmail.com/
- [2] https://github.com/martinvonz/jj/discussions/49
- [3] https://lore.kernel.org/git/CABPp-BE48=97k_3tnNqXPjSEfA163F8hoE+HY0Zvz1SWB2B8EA@mail.gmail.com/
# Important limitations
* The code exits with code 1 if there are any conflict. No
resumability. No nice output. No interactivity. No special exit code
depending on the reason.
* When a commit becomes empty as it is replayed, it is still replayed
as an empty commit, instead of being dropped.
* No replaying merges, nor root commits. Only regular commits.
* Signed commits are not properly handled. It's not clear what to do
to such commits when replaying on the server side.
* Notes associated with replayed commits are not updated nor
duplicated. (Thanks to Phillip Wood for noticing.)
# Commit overview
* 1/15 t6429: remove switching aspects of fast-rebase
New preparatory commit to make it easier to later replace the
fast-rebase test-tool by `git replay` without breaking existing
tests.
* 2/15 replay: introduce new builtin
This creates a minimal `git replay` command by moving the code
from the `fast-rebase` test helper from `t/helper/` into
`builtin/` and doing some renames and a few other needed changes.
(In v3 some `#include ...` were changed to deal with upstream
changes in this area as suggested by Junio.)
* - 3/15 replay: start using parse_options API
- 4/15 replay: die() instead of failing assert()
- 5/15 replay: introduce pick_regular_commit()
- 6/15 replay: don't simplify history
- 7/15 replay: add an important FIXME comment about gpg signing
- 8/15 replay: remove progress and info output
- 9/15 replay: remove HEAD related sanity check
These slowly change the command to make it behave more like
regular commands and to start cleaning up its output.
* 10/15 replay: make it a minimal server side command
After the cleaning up in previous commits, it's now time to
radically change the way it works by stopping it to do ref
updates, to update the index and worktree, to consider HEAD as
special. Instead just make it output commands that should be
passed to `git update-ref --stdin`.
* 11/15 replay: use standard revision ranges
Start addind new interesting features and also documentation and
tests, as the interface of the command is cristalizing into its
final form.
* 12/15 replay: disallow revision specific options and pathspecs
For now disallow revision specific options and pathspecs that are
allowed and eaten by setup_revisions(), as it's not clear if all
of these extra features are really needed, and anyway they would
require tests and doc. So we leave them for future improvements.
(In v3 a typo was fixed in the commit message and a code comment
has been improved as suggested by Elijah.)
* - 13/15 replay: add --advance or 'cherry-pick' mode
- 14/15 replay: add --contained to rebase contained branches
Add new option and features to the command. (In v3, in commit
14/15, the synopsys of the command was improved and a sentence
about git rebase in its doc was removed as suggested by Elijah.)
* 15/15 replay: stop assuming replayed branches do not diverge
This adds another interesting feature, as well as related
documentation and tests. (In v3 a typo in the commit message was
fixed as suggested by Elijah.)
# Notes about `fast-rebase`, tests and documentation
The `fast-rebase` test-tool helper was developed by Elijah to
experiment with a rebasing tool that would be developed from scratch
based on his merge-ort work, could be used to test that merge-ort
work, and would not have the speed and interface limitations of `git
rebase` or `git cherry-pick`.
This `fast-rebase` helper was used before this series in:
t6429-merge-sequence-rename-caching.sh
So when `git replay` is created from `fast-rebase` in patch 2/15, the
t6429 test script is also converted to use `git replay`. This ensures
that `git replay` doesn't break too badly during the first 10 patches
in this patch series.
Tests and documentation are introduced specifically for `git replay`
only in 11/15 and later patches as it doesn't make much sense to
document and test behavior that we know is going to change soon. So
it's only when the command is crystalizing towards its final form that
we start documenting and testing it.
# Possibly controversial issues
* bare or not bare: this series works towards a plumbing command with
the end goal of it being usable and used first on bare repos,
contrary to existing commands like `git rebase` and `git
cherry-pick`. The tests check that the command works on both bare
and non-bare repo though.
* exit status: a successful, non-conflicted replay exits with code
0. When the replay has conflicts, the exit status is 1. If the
replay is not able to complete (or start) due to some kind of error,
the exit status is something other than 0 or 1. There are a few
tests checking that. It has been suggested in an internal review
that conflicts might want to get a more specific error code as an
error code of 1 might be quite easy to return by accident. It
doesn't seem to me from their docs (which might want to be improved,
I didn't look at the code) that other commands like `git merge` and
`git rebase` exit with a special error code in case of conflict.
* make worktree and index changes optional: commit 10/15 stops
updating the index and worktree, but it might be better especially
for cli users to make that optional. The issue is that this would
make the command more complex while we are developing a number of
important features so that the command can be used on bare repos. It
seems that this should rather be done in an iterative improvement
after the important features have landed.
* when and where to add tests and docs: although t6429 has tests that
are changed to use the new command instead of the fast-rebase
test-tool command as soon as the former is introduced, there is no
specific test script and no doc for the new command until commit
11/15 when standard revision ranges are used. This is done to avoid
churn in tests and docs while the final form of the command hasn't
crystalized enough. Adding tests and doc at this point makes this
commit quite big and possibly more difficult to review than if they
were in separate commits though. On the other hand when tests and
docs are added in specific commits some reviewers say it would be
better to introduce them when the related changes are made.
* --advance and --contained: these two advanced options might not
belong to this first series and could perhaps be added in a followup
series in separate commits. On the other hand the code for
--contained seems involved with the code of --advance and it's nice
to see soon that git replay can indeed do cherry-picking and rebase
many refs at once, and this way fullfil these parts of its promise.
* replaying diverging branches: 15/15 the last patch in the series,
which allow replaying diverging branches, can be seen as a
fundamental fix or alternatively as adding an interesting
feature. So it's debatable if it should be in its own patch along
with its own tests as in this series, or if it should be merged into
a previous patch and which one.
* only 2 patches: this patch series can be seen from a high level
point of view as 1) introducing the new `git replay` command, and 2)
using `git replay` to replace, and get rid of, the fast-rebase
test-tool command. The fact that not much of the original
fast-rebase code and interface is left would agree with that point
of view. On the other hand, fast-rebase can also be seen as a first
iteration towards `git replay`. So it can also make sense to see how
`git replay` evolved from it.
# 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.
# Range-diff between v3 and v4
1: 51fa1c7aea = 1: 1eaca9b788 t6429: remove switching aspects of fast-rebase
2: 19f8cf1b2e ! 2: 5ac4beb1ae replay: introduce new builtin
@@ t/helper/test-fast-rebase.c => builtin/replay.c
#define USE_THE_INDEX_VARIABLE
-#include "test-tool.h"
--#include "cache.h"
+#include "git-compat-util.h"
+
+#include "builtin.h"
3: 295e876db6 ! 3: 299381aa9b replay: start using parse_options API
@@ builtin/replay.c
#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"
4: 6ece7d3751 = 4: 3b825c9be0 replay: die() instead of failing assert()
5: 9b4bc659fb = 5: b1e890745d replay: introduce pick_regular_commit()
6: 9ab68d50ab = 6: ec51351889 replay: don't simplify history
7: 37e93faafa = 7: cd4ed07d2d replay: add an important FIXME comment about gpg signing
8: 03036781ed = 8: e45a55917c replay: remove progress and info output
9: 4ea289952e = 9: 0587a76cbb replay: remove HEAD related sanity check
10: fba98eda07 ! 10: d10368e87a replay: make it a minimal server side command
@@ builtin/replay.c
-#include "commit.h"
#include "environment.h"
-#include "gettext.h"
+-#include "hash.h"
#include "hex.h"
#include "lockfile.h"
#include "merge-ort.h"
@@ builtin/replay.c: int cmd_replay(int argc, const char **argv, const char *prefix
+ }
}
-+ /* Cleanup */
merge_finalize(&merge_opt, &result);
+ ret = result.clean;
11: 03f9f20f6e ! 11: 4e09572c43 replay: use standard revision ranges
@@ Documentation/git-replay.txt (new)
+ Starting point at which to create the new commits. May be any
+ valid commit, and not just an existing branch name.
++
-+The update-ref commands in the output will update the branch(es)
-+in the revision range to point at the new commits (in other
-+words, this mimics a rebase operation).
++The update-ref command(s) in the output will update the branch(es) in
++the revision range to point at the new commits, similar to the way how
++`git rebase --update-refs` updates multiple branches in the affected
++range.
+
+<revision-range>::
+ Range of commits to replay; see "Specifying Ranges" in
@@ Documentation/git-replay.txt (new)
+ update refs/heads/branch2 ${NEW_branch2_HASH} ${OLD_branch2_HASH}
+ update refs/heads/branch3 ${NEW_branch3_HASH} ${OLD_branch3_HASH}
+
-+where the number of refs updated depend on the arguments passed.
++where the number of refs updated depends on the arguments passed and
++the shape of the history being replayed.
+
+EXIT STATUS
+-----------
12: e651250ac7 ! 12: 64b803f1cf replay: disallow revision specific options and pathspecs
@@ builtin/replay.c: int cmd_replay(int argc, const char **argv, const char *prefix
+ * them, after adding related tests and doc though.
+ */
+ if (revs.prune_data.nr) {
-+ error(_("invalid pathspec: %s"), revs.prune_data.items[0].match);
++ error(_("no pathspec is allowed: '%s'"), revs.prune_data.items[0].match);
+ usage_with_options(replay_usage, replay_options);
+ }
+
13: 56e5416dad ! 13: 04f27d81ab replay: add --advance or 'cherry-pick' mode
@@ Commit message
'cherry-pick' mode with `--advance`. This new mode will make the target
branch advance as we replay commits onto it.
+ The replayed commits should have a single tip, so that it's clear where
+ the target branch should be advanced. If they have more than one tip,
+ this new mode will error out.
+
Co-authored-by: Christian Couder <chriscool@tuxfamily.org>
Signed-off-by: Elijah Newren <newren@gmail.com>
Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
@@ Documentation/git-replay.txt: OPTIONS
Starting point at which to create the new commits. May be any
valid commit, and not just an existing branch name.
+
--The update-ref commands in the output will update the branch(es)
--in the revision range to point at the new commits (in other
--words, this mimics a rebase operation).
+-The update-ref command(s) in the output will update the branch(es) in
+-the revision range to point at the new commits, similar to the way how
+-`git rebase --update-refs` updates multiple branches in the affected
+-range.
+When `--onto` is specified, the update-ref command(s) in the output will
+update the branch(es) in the revision range to point at the new
-+commits (in other words, this mimics a rebase operation).
++commits, similar to the way how `git rebase --update-refs` updates
++multiple branches in the affected range.
+
+--advance <branch>::
+ Starting point at which to create the new commits; must be a
@@ Documentation/git-replay.txt: OPTIONS
+the new commits (in other words, this mimics a cherry-pick operation).
<revision-range>::
- Range of commits to replay; see "Specifying Ranges" in
+- Range of commits to replay; see "Specifying Ranges" in
+- linkgit:git-rev-parse.
++ Range of commits to replay. More than one <revision-range> can
++ be passed, but in `--advance <branch>` mode, they should have
++ a single tip, so that it's clear where <branch> should point
++ to. See "Specifying Ranges" in linkgit:git-rev-parse.
+
+ OUTPUT
+ ------
@@ Documentation/git-replay.txt: input to `git update-ref --stdin`. It is basically of the form:
- update refs/heads/branch2 ${NEW_branch2_HASH} ${OLD_branch2_HASH}
update refs/heads/branch3 ${NEW_branch3_HASH} ${OLD_branch3_HASH}
--where the number of refs updated depend on the arguments passed.
-+where the number of refs updated depend on the arguments passed. When
-+using `--advance`, the number of refs updated is always one, but for
-+`--onto`, it can be one or more (rebasing multiple branches
-+simultaneously is supported).
+ where the number of refs updated depends on the arguments passed and
+-the shape of the history being replayed.
++the shape of the history being replayed. When using `--advance`, the
++number of refs updated is always one, but for `--onto`, it can be one
++or more (rebasing multiple branches simultaneously is supported).
EXIT STATUS
-----------
@@ builtin/replay.c: static struct commit *create_commit(struct tree *tree,
+ die(_("argument to --advance must be a reference"));
+ }
+ if (rinfo.positive_refexprs > 1)
-+ die(_("cannot advance target with multiple source branches because ordering would be ill-defined"));
++ die(_("cannot advance target with multiple sources because ordering would be ill-defined"));
+ } else {
+ int positive_refs_complete = (
+ rinfo.positive_refexprs ==
@@ builtin/replay.c: int cmd_replay(int argc, const char **argv, const char *prefix
+ oid_to_hex(&onto->object.oid));
+ }
+
- /* Cleanup */
merge_finalize(&merge_opt, &result);
ret = result.clean;
@@ t/t3650-replay-basics.sh: test_expect_success 'using replay on bare repo with di
+ git -C bare replay --advance main topic1..topic2 >result-bare &&
+ test_cmp expect result-bare
+'
++
++test_expect_success 'replay on bare repo fails with both --advance and --onto' '
++ test_must_fail git -C bare replay --advance main --onto main topic1..topic2 >result-bare
++'
++
++test_expect_success 'replay fails when both --advance and --onto are omitted' '
++ test_must_fail git replay topic1..topic2 >result
++'
+
test_done
14: 2cc17dfdc7 ! 14: 9ed0919ad5 replay: add --contained to rebase contained branches
@@ builtin/replay.c: int cmd_replay(int argc, const char **argv, const char *prefix
oid_to_hex(&last_commit->object.oid),
## t/t3650-replay-basics.sh ##
-@@ t/t3650-replay-basics.sh: test_expect_success 'using replay on bare repo to perform basic cherry-pick' '
- test_cmp expect result-bare
+@@ t/t3650-replay-basics.sh: test_expect_success 'replay fails when both --advance and --onto are omitted' '
+ test_must_fail git replay topic1..topic2 >result
'
+test_expect_success 'using replay to also rebase a contained branch' '
15: a6d88fc8f0 ! 15: cf8c984877 replay: stop assuming replayed branches do not diverge
@@ builtin/replay.c: int cmd_replay(int argc, const char **argv, const char *prefix
if (advance_name)
continue;
@@ builtin/replay.c: int cmd_replay(int argc, const char **argv, const char *prefix)
+ }
- /* Cleanup */
merge_finalize(&merge_opt, &result);
- ret = result.clean;
-
Christian Couder (1):
replay: disallow revision specific options and pathspecs
Elijah Newren (14):
t6429: remove switching aspects of fast-rebase
replay: introduce new builtin
replay: start using parse_options API
replay: die() instead of failing assert()
replay: introduce pick_regular_commit()
replay: don't simplify history
replay: add an important FIXME comment about gpg signing
replay: remove progress and info output
replay: remove HEAD related sanity check
replay: make it a minimal server side command
replay: use standard revision ranges
replay: add --advance or 'cherry-pick' mode
replay: add --contained to rebase contained branches
replay: stop assuming replayed branches do not diverge
.gitignore | 1 +
Documentation/git-replay.txt | 124 +++++++
Makefile | 2 +-
builtin.h | 1 +
builtin/replay.c | 431 +++++++++++++++++++++++
command-list.txt | 1 +
git.c | 1 +
t/helper/test-fast-rebase.c | 241 -------------
t/helper/test-tool.c | 1 -
t/helper/test-tool.h | 1 -
t/t3650-replay-basics.sh | 214 +++++++++++
t/t6429-merge-sequence-rename-caching.sh | 45 +--
12 files changed, 799 insertions(+), 264 deletions(-)
create mode 100644 Documentation/git-replay.txt
create mode 100644 builtin/replay.c
delete mode 100644 t/helper/test-fast-rebase.c
create mode 100755 t/t3650-replay-basics.sh
--
2.42.0.126.gcf8c984877
^ permalink raw reply
* [PATCH v2] diff-lib: Fix check_removed when fsmonitor is on
From: Josip Sokcevic @ 2023-09-07 17:01 UTC (permalink / raw)
To: jonathantanmy; +Cc: git, git, Josip Sokcevic
In-Reply-To: <20230906203726.1526272-1-jonathantanmy@google.com>
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`.
Signed-off-by: Josip Sokcevic <sokcevic@google.com>
---
diff-lib.c | 19 +++++++++++++++----
t/t7527-builtin-fsmonitor.sh | 5 +++++
2 files changed, 20 insertions(+), 4 deletions(-)
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)) {
diff --git a/t/t7527-builtin-fsmonitor.sh b/t/t7527-builtin-fsmonitor.sh
index 0c241d6c14..78503158fd 100755
--- a/t/t7527-builtin-fsmonitor.sh
+++ b/t/t7527-builtin-fsmonitor.sh
@@ -809,6 +809,11 @@ my_match_and_clean () {
status --porcelain=v2 >actual.without &&
test_cmp actual.with actual.without &&
+ git -C super --no-optional-locks diff-index --name-status HEAD >actual.with &&
+ git -C super --no-optional-locks -c core.fsmonitor=false \
+ diff-index --name-status HEAD >actual.without &&
+ test_cmp actual.with actual.without &&
+
git -C super/dir_1/dir_2/sub reset --hard &&
git -C super/dir_1/dir_2/sub clean -d -f
}
--
2.42.0.283.g2d96d420d3-goog
^ permalink raw reply related
* [PATCH v4 11/15] replay: use standard revision ranges
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 the fixed "<oldbase> <branch>" arguments, the replay
command now accepts "<revision-range>..." arguments in a similar
way as many other Git commands. This makes its interface more
standard and more flexible.
Also as the interface of the command is now mostly finalized,
we can add some documentation as well as testcases to make sure
the command will continue to work as designed in the future.
Co-authored-by: Christian Couder <chriscool@tuxfamily.org>
Signed-off-by: Elijah Newren <newren@gmail.com>
Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
Documentation/git-replay.txt | 90 ++++++++++++++++++++++++
builtin/replay.c | 21 ++----
t/t3650-replay-basics.sh | 83 ++++++++++++++++++++++
t/t6429-merge-sequence-rename-caching.sh | 18 ++---
4 files changed, 186 insertions(+), 26 deletions(-)
create mode 100644 Documentation/git-replay.txt
create mode 100755 t/t3650-replay-basics.sh
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>...
+
+DESCRIPTION
+-----------
+
+Takes a range of commits, and replays them onto a new location. Does
+not touch the working tree or index, and does not update any
+references. However, the output of this command is meant to be used
+as input to `git update-ref --stdin`, which would update the relevant
+branches.
+
+THIS COMMAND IS EXPERIMENTAL. THE BEHAVIOR MAY CHANGE.
+
+OPTIONS
+-------
+
+--onto <newbase>::
+ Starting point at which to create the new commits. May be any
+ valid commit, and not just an existing branch name.
++
+The update-ref command(s) in the output will update the branch(es) in
+the revision range to point at the new commits, similar to the way how
+`git rebase --update-refs` updates multiple branches in the affected
+range.
+
+<revision-range>::
+ Range of commits to replay; see "Specifying Ranges" in
+ linkgit:git-rev-parse.
+
+OUTPUT
+------
+
+When there are no conflicts, the output of this command is usable as
+input to `git update-ref --stdin`. It is basically of the form:
+
+ update refs/heads/branch1 ${NEW_branch1_HASH} ${OLD_branch1_HASH}
+ update refs/heads/branch2 ${NEW_branch2_HASH} ${OLD_branch2_HASH}
+ update refs/heads/branch3 ${NEW_branch3_HASH} ${OLD_branch3_HASH}
+
+where the number of refs updated depends on the arguments passed and
+the shape of the history being replayed.
+
+EXIT STATUS
+-----------
+
+For a successful, non-conflicted replay, the exit status is 0. When
+the replay has conflicts, the exit status is 1. If the replay is not
+able to complete (or start) due to some kind of error, the exit status
+is something other than 0 or 1.
+
+EXAMPLES
+--------
+
+To simply rebase mybranch onto target:
+
+------------
+$ git replay --onto target origin/main..mybranch
+update refs/heads/mybranch ${NEW_mybranch_HASH} ${OLD_mybranch_HASH}
+------------
+
+When calling `git replay`, one does not need to specify a range of
+commits to replay using the syntax `A..B`; any range expression will
+do:
+
+------------
+$ git replay --onto origin/main ^base branch1 branch2 branch3
+update refs/heads/branch1 ${NEW_branch1_HASH} ${OLD_branch1_HASH}
+update refs/heads/branch2 ${NEW_branch2_HASH} ${OLD_branch2_HASH}
+update refs/heads/branch3 ${NEW_branch3_HASH} ${OLD_branch3_HASH}
+------------
+
+This will simultaneously rebase branch1, branch2, and branch3 -- all
+commits they have since base, playing them on top of origin/main.
+These three branches may have commits on top of base that they have in
+common, but that does not need to be the case.
+
+GIT
+---
+Part of the linkgit:git[1] suite
diff --git a/builtin/replay.c b/builtin/replay.c
index e45cd59da1..de2ddeae3e 100644
--- a/builtin/replay.c
+++ b/builtin/replay.c
@@ -14,7 +14,6 @@
#include "parse-options.h"
#include "refs.h"
#include "revision.h"
-#include "strvec.h"
#include <oidset.h>
#include <tree.h>
@@ -118,16 +117,14 @@ int cmd_replay(int argc, const char **argv, const char *prefix)
struct commit *onto;
const char *onto_name = NULL;
struct commit *last_commit = NULL;
- struct strvec rev_walk_args = STRVEC_INIT;
struct rev_info revs;
struct commit *commit;
struct merge_options merge_opt;
struct merge_result result;
- struct strbuf branch_name = STRBUF_INIT;
int ret = 0;
const char * const replay_usage[] = {
- N_("git replay --onto <newbase> <oldbase> <branch>"),
+ N_("git replay --onto <newbase> <revision-range>..."),
NULL
};
struct option replay_options[] = {
@@ -145,20 +142,13 @@ int cmd_replay(int argc, const char **argv, const char *prefix)
usage_with_options(replay_usage, replay_options);
}
- if (argc != 3) {
- error(_("bad number of arguments"));
- usage_with_options(replay_usage, replay_options);
- }
-
onto = peel_committish(onto_name);
- strbuf_addf(&branch_name, "refs/heads/%s", argv[2]);
repo_init_revisions(the_repository, &revs, prefix);
- 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"));
+ argc = setup_revisions(argc, argv, &revs, NULL);
+ if (argc > 1) {
+ ret = error(_("unrecognized argument: %s"), argv[1]);
goto cleanup;
}
@@ -168,8 +158,6 @@ int cmd_replay(int argc, const char **argv, const char *prefix)
revs.topo_order = 1;
revs.simplify_history = 0;
- strvec_clear(&rev_walk_args);
-
if (prepare_revision_walk(&revs) < 0) {
ret = error(_("error preparing revisions"));
goto cleanup;
@@ -211,7 +199,6 @@ int cmd_replay(int argc, const char **argv, const char *prefix)
ret = result.clean;
cleanup:
- strbuf_release(&branch_name);
release_revisions(&revs);
/* Return */
diff --git a/t/t3650-replay-basics.sh b/t/t3650-replay-basics.sh
new file mode 100755
index 0000000000..a1da4f9ef9
--- /dev/null
+++ b/t/t3650-replay-basics.sh
@@ -0,0 +1,83 @@
+#!/bin/sh
+
+test_description='basic git replay tests'
+
+GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
+export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
+
+. ./test-lib.sh
+
+GIT_AUTHOR_NAME=author@name
+GIT_AUTHOR_EMAIL=bogus@email@address
+export GIT_AUTHOR_NAME GIT_AUTHOR_EMAIL
+
+test_expect_success 'setup' '
+ test_commit A &&
+ test_commit B &&
+
+ git switch -c topic1 &&
+ test_commit C &&
+ git switch -c topic2 &&
+ test_commit D &&
+ test_commit E &&
+ git switch topic1 &&
+ test_commit F &&
+ git switch -c topic3 &&
+ test_commit G &&
+ test_commit H &&
+ git switch -c topic4 main &&
+ test_commit I &&
+ test_commit J &&
+
+ git switch -c next main &&
+ test_commit K &&
+ git merge -m "Merge topic1" topic1 &&
+ git merge -m "Merge topic2" topic2 &&
+ git merge -m "Merge topic3" topic3 &&
+ >evil &&
+ git add evil &&
+ git commit --amend &&
+ git merge -m "Merge topic4" topic4 &&
+
+ git switch main &&
+ test_commit L &&
+ test_commit M &&
+
+ git switch -c conflict B &&
+ test_commit C.conflict C.t conflict
+'
+
+test_expect_success 'setup bare' '
+ git clone --bare . bare
+'
+
+test_expect_success 'using replay to rebase two branches, one on top of other' '
+ git replay --onto main topic1..topic2 >result &&
+
+ test_line_count = 1 result &&
+
+ git log --format=%s $(cut -f 3 -d " " result) >actual &&
+ test_write_lines E D M L B A >expect &&
+ test_cmp expect actual &&
+
+ printf "update refs/heads/topic2 " >expect &&
+ printf "%s " $(cut -f 3 -d " " result) >>expect &&
+ git rev-parse topic2 >>expect &&
+
+ test_cmp expect result
+'
+
+test_expect_success 'using replay on bare repo to rebase two branches, one on top of other' '
+ git -C bare replay --onto main topic1..topic2 >result-bare &&
+ test_cmp expect result-bare
+'
+
+test_expect_success 'using replay to rebase with a conflict' '
+ test_expect_code 1 git replay --onto topic1 B..conflict
+'
+
+test_expect_success 'using replay on bare repo to rebase with a conflict' '
+ test_expect_code 1 git -C bare replay --onto topic1 B..conflict
+'
+
+test_done
diff --git a/t/t6429-merge-sequence-rename-caching.sh b/t/t6429-merge-sequence-rename-caching.sh
index 099aefeffc..0f39ed0d08 100755
--- a/t/t6429-merge-sequence-rename-caching.sh
+++ b/t/t6429-merge-sequence-rename-caching.sh
@@ -71,7 +71,7 @@ test_expect_success 'caching renames does not preclude finding new ones' '
git switch upstream &&
- git replay --onto HEAD upstream~1 topic >out &&
+ git replay --onto HEAD upstream~1..topic >out &&
git update-ref --stdin <out &&
git checkout topic &&
@@ -141,7 +141,7 @@ test_expect_success 'cherry-pick both a commit and its immediate revert' '
GIT_TRACE2_PERF="$(pwd)/trace.output" &&
export GIT_TRACE2_PERF &&
- git replay --onto HEAD upstream~1 topic >out &&
+ git replay --onto HEAD upstream~1..topic >out &&
git update-ref --stdin <out &&
git checkout topic &&
@@ -201,7 +201,7 @@ test_expect_success 'rename same file identically, then reintroduce it' '
GIT_TRACE2_PERF="$(pwd)/trace.output" &&
export GIT_TRACE2_PERF &&
- git replay --onto HEAD upstream~1 topic >out &&
+ git replay --onto HEAD upstream~1..topic >out &&
git update-ref --stdin <out &&
git checkout topic &&
@@ -279,7 +279,7 @@ test_expect_success 'rename same file identically, then add file to old dir' '
GIT_TRACE2_PERF="$(pwd)/trace.output" &&
export GIT_TRACE2_PERF &&
- git replay --onto HEAD upstream~1 topic >out &&
+ git replay --onto HEAD upstream~1..topic >out &&
git update-ref --stdin <out &&
git checkout topic &&
@@ -357,7 +357,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 git replay --onto HEAD upstream~1 topic >output &&
+ 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,7 +456,7 @@ test_expect_success 'dir rename unneeded, then add new file to old dir' '
GIT_TRACE2_PERF="$(pwd)/trace.output" &&
export GIT_TRACE2_PERF &&
- git replay --onto HEAD upstream~1 topic >out &&
+ git replay --onto HEAD upstream~1..topic >out &&
git update-ref --stdin <out &&
git checkout topic &&
@@ -523,7 +523,7 @@ test_expect_success 'dir rename unneeded, then rename existing file into old dir
GIT_TRACE2_PERF="$(pwd)/trace.output" &&
export GIT_TRACE2_PERF &&
- git replay --onto HEAD upstream~1 topic >out &&
+ git replay --onto HEAD upstream~1..topic >out &&
git update-ref --stdin <out &&
git checkout topic &&
@@ -626,7 +626,7 @@ test_expect_success 'caching renames only on upstream side, part 1' '
GIT_TRACE2_PERF="$(pwd)/trace.output" &&
export GIT_TRACE2_PERF &&
- git replay --onto HEAD upstream~1 topic >out &&
+ git replay --onto HEAD upstream~1..topic >out &&
git update-ref --stdin <out &&
git checkout topic &&
@@ -685,7 +685,7 @@ test_expect_success 'caching renames only on upstream side, part 2' '
GIT_TRACE2_PERF="$(pwd)/trace.output" &&
export GIT_TRACE2_PERF &&
- git replay --onto HEAD upstream~1 topic >out &&
+ git replay --onto HEAD upstream~1..topic >out &&
git update-ref --stdin <out &&
git checkout topic &&
--
2.42.0.126.gcf8c984877
^ permalink raw reply related
* Re: [bug]fatal: fetch-pack: invalid index-pack output
From: Bagas Sanjaya @ 2023-09-07 9:06 UTC (permalink / raw)
To: Junio C Hamano; +Cc: mingli zhang, git, Jonathan Tan, brian m. carlson
In-Reply-To: <xmqqwmx9q1he.fsf@gitster.g>
On 01/09/2023 23:53, Junio C Hamano wrote:
> Bagas Sanjaya <bagasdotme@gmail.com> writes:
>
>> On Sun, Aug 14, 2022 at 05:09:13AM +0800, mingli zhang wrote:
>>> ...
>>> I use git 2.36.1 on MacOS Monterey 12.3.1
>>> ...
>>
>> Related discussion is in [1] with the fix for OpenSSL users is in [2].
>> Please test.
>>
>> [1]: https://lore.kernel.org/git/ZPCL11k38PXTkFga@debian.me/
>> [2]: https://lore.kernel.org/git/20230901020928.M610756@dcvr/
>
> I somehow think they are not related. The EVP thing appeared only
> in Git 2.42 and the report says 2.36.1, which was released in May
> 2022 and predates it by more than a year, no?
Ah! I missed the point.
mingli, can you test v2.42.0?
--
An old man doll... just what I always wanted! - Clara
^ permalink raw reply
* [PATCH v4 13/15] replay: add --advance or 'cherry-pick' mode
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>
There is already a 'rebase' mode with `--onto`. Let's add an 'advance' or
'cherry-pick' mode with `--advance`. This new mode will make the target
branch advance as we replay commits onto it.
The replayed commits should have a single tip, so that it's clear where
the target branch should be advanced. If they have more than one tip,
this new mode will error out.
Co-authored-by: Christian Couder <chriscool@tuxfamily.org>
Signed-off-by: Elijah Newren <newren@gmail.com>
Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
Documentation/git-replay.txt | 40 ++++++--
builtin/replay.c | 185 +++++++++++++++++++++++++++++++++--
t/t3650-replay-basics.sh | 34 +++++++
3 files changed, 242 insertions(+), 17 deletions(-)
diff --git a/Documentation/git-replay.txt b/Documentation/git-replay.txt
index 9a2087b01a..5c5c15237d 100644
--- a/Documentation/git-replay.txt
+++ b/Documentation/git-replay.txt
@@ -9,7 +9,7 @@ git-replay - Replay commits on a different base, without touching working tree
SYNOPSIS
--------
[verse]
-'git replay' --onto <newbase> <revision-range>...
+'git replay' (--onto <newbase> | --advance <branch>) <revision-range>...
DESCRIPTION
-----------
@@ -29,14 +29,24 @@ OPTIONS
Starting point at which to create the new commits. May be any
valid commit, and not just an existing branch name.
+
-The update-ref command(s) in the output will update the branch(es) in
-the revision range to point at the new commits, similar to the way how
-`git rebase --update-refs` updates multiple branches in the affected
-range.
+When `--onto` is specified, the update-ref command(s) in the output will
+update the branch(es) in the revision range to point at the new
+commits, similar to the way how `git rebase --update-refs` updates
+multiple branches in the affected range.
+
+--advance <branch>::
+ Starting point at which to create the new commits; must be a
+ branch name.
++
+When `--advance` is specified, the update-ref command(s) in the output
+will update the branch passed as an argument to `--advance` to point at
+the new commits (in other words, this mimics a cherry-pick operation).
<revision-range>::
- Range of commits to replay; see "Specifying Ranges" in
- linkgit:git-rev-parse.
+ Range of commits to replay. More than one <revision-range> can
+ be passed, but in `--advance <branch>` mode, they should have
+ a single tip, so that it's clear where <branch> should point
+ to. See "Specifying Ranges" in linkgit:git-rev-parse.
OUTPUT
------
@@ -49,7 +59,9 @@ input to `git update-ref --stdin`. It is basically of the form:
update refs/heads/branch3 ${NEW_branch3_HASH} ${OLD_branch3_HASH}
where the number of refs updated depends on the arguments passed and
-the shape of the history being replayed.
+the shape of the history being replayed. When using `--advance`, the
+number of refs updated is always one, but for `--onto`, it can be one
+or more (rebasing multiple branches simultaneously is supported).
EXIT STATUS
-----------
@@ -69,6 +81,18 @@ $ git replay --onto target origin/main..mybranch
update refs/heads/mybranch ${NEW_mybranch_HASH} ${OLD_mybranch_HASH}
------------
+To cherry-pick the commits from mybranch onto target:
+
+------------
+$ git replay --advance target origin/main..mybranch
+update refs/heads/target ${NEW_target_HASH} ${OLD_target_HASH}
+------------
+
+Note that the first two examples replay the exact same commits and on
+top of the exact same new base, they only differ in that the first
+provides instructions to make mybranch point at the new commits and
+the second provides instructions to make target point at them.
+
When calling `git replay`, one does not need to specify a range of
commits to replay using the syntax `A..B`; any range expression will
do:
diff --git a/builtin/replay.c b/builtin/replay.c
index 60abdaee9e..6b89964be9 100644
--- a/builtin/replay.c
+++ b/builtin/replay.c
@@ -14,6 +14,7 @@
#include "parse-options.h"
#include "refs.h"
#include "revision.h"
+#include "strmap.h"
#include <oidset.h>
#include <tree.h>
@@ -82,6 +83,146 @@ static struct commit *create_commit(struct tree *tree,
return (struct commit *)obj;
}
+struct ref_info {
+ struct commit *onto;
+ struct strset positive_refs;
+ struct strset negative_refs;
+ int positive_refexprs;
+ int negative_refexprs;
+};
+
+static void get_ref_information(struct rev_cmdline_info *cmd_info,
+ struct ref_info *ref_info)
+{
+ int i;
+
+ ref_info->onto = NULL;
+ strset_init(&ref_info->positive_refs);
+ strset_init(&ref_info->negative_refs);
+ ref_info->positive_refexprs = 0;
+ ref_info->negative_refexprs = 0;
+
+ /*
+ * When the user specifies e.g.
+ * git replay origin/main..mybranch
+ * git replay ^origin/next mybranch1 mybranch2
+ * we want to be able to determine where to replay the commits. In
+ * these examples, the branches are probably based on an old version
+ * of either origin/main or origin/next, so we want to replay on the
+ * newest version of that branch. In contrast we would want to error
+ * out if they ran
+ * git replay ^origin/master ^origin/next mybranch
+ * git replay mybranch~2..mybranch
+ * the first of those because there's no unique base to choose, and
+ * the second because they'd likely just be replaying commits on top
+ * of the same commit and not making any difference.
+ */
+ for (i = 0; i < cmd_info->nr; i++) {
+ struct rev_cmdline_entry *e = cmd_info->rev + i;
+ struct object_id oid;
+ const char *refexpr = e->name;
+ char *fullname = NULL;
+ int can_uniquely_dwim = 1;
+
+ if (*refexpr == '^')
+ refexpr++;
+ if (repo_dwim_ref(the_repository, refexpr, strlen(refexpr), &oid, &fullname, 0) != 1)
+ can_uniquely_dwim = 0;
+
+ if (e->flags & BOTTOM) {
+ if (can_uniquely_dwim)
+ strset_add(&ref_info->negative_refs, fullname);
+ if (!ref_info->negative_refexprs)
+ ref_info->onto = lookup_commit_reference_gently(the_repository,
+ &e->item->oid, 1);
+ ref_info->negative_refexprs++;
+ } else {
+ if (can_uniquely_dwim)
+ strset_add(&ref_info->positive_refs, fullname);
+ ref_info->positive_refexprs++;
+ }
+
+ free(fullname);
+ }
+}
+
+static void determine_replay_mode(struct rev_cmdline_info *cmd_info,
+ const char *onto_name,
+ const char **advance_name,
+ struct commit **onto,
+ struct strset **update_refs)
+{
+ struct ref_info rinfo;
+
+ get_ref_information(cmd_info, &rinfo);
+ if (!rinfo.positive_refexprs)
+ die(_("need some commits to replay"));
+ if (onto_name && *advance_name)
+ die(_("--onto and --advance are incompatible"));
+ else if (onto_name) {
+ *onto = peel_committish(onto_name);
+ if (rinfo.positive_refexprs <
+ strset_get_size(&rinfo.positive_refs))
+ die(_("all positive revisions given must be references"));
+ } else if (*advance_name) {
+ struct object_id oid;
+ char *fullname = NULL;
+
+ *onto = peel_committish(*advance_name);
+ if (repo_dwim_ref(the_repository, *advance_name, strlen(*advance_name),
+ &oid, &fullname, 0) == 1) {
+ *advance_name = fullname;
+ } else {
+ die(_("argument to --advance must be a reference"));
+ }
+ if (rinfo.positive_refexprs > 1)
+ die(_("cannot advance target with multiple sources because ordering would be ill-defined"));
+ } else {
+ int positive_refs_complete = (
+ rinfo.positive_refexprs ==
+ strset_get_size(&rinfo.positive_refs));
+ int negative_refs_complete = (
+ rinfo.negative_refexprs ==
+ strset_get_size(&rinfo.negative_refs));
+ /*
+ * We need either positive_refs_complete or
+ * negative_refs_complete, but not both.
+ */
+ if (rinfo.negative_refexprs > 0 &&
+ positive_refs_complete == negative_refs_complete)
+ die(_("cannot implicitly determine whether this is an --advance or --onto operation"));
+ if (negative_refs_complete) {
+ struct hashmap_iter iter;
+ struct strmap_entry *entry;
+
+ if (rinfo.negative_refexprs == 0)
+ die(_("all positive revisions given must be references"));
+ else if (rinfo.negative_refexprs > 1)
+ die(_("cannot implicitly determine whether this is an --advance or --onto operation"));
+ else if (rinfo.positive_refexprs > 1)
+ die(_("cannot advance target with multiple source branches because ordering would be ill-defined"));
+
+ /* Only one entry, but we have to loop to get it */
+ strset_for_each_entry(&rinfo.negative_refs,
+ &iter, entry) {
+ *advance_name = entry->key;
+ }
+ } else { /* positive_refs_complete */
+ if (rinfo.negative_refexprs > 1)
+ die(_("cannot implicitly determine correct base for --onto"));
+ if (rinfo.negative_refexprs == 1)
+ *onto = rinfo.onto;
+ }
+ }
+ if (!*advance_name) {
+ *update_refs = xcalloc(1, sizeof(**update_refs));
+ **update_refs = rinfo.positive_refs;
+ memset(&rinfo.positive_refs, 0, sizeof(**update_refs));
+ }
+ strset_clear(&rinfo.negative_refs);
+ strset_clear(&rinfo.positive_refs);
+}
+
static struct commit *pick_regular_commit(struct commit *pickme,
struct commit *last_commit,
struct merge_options *merge_opt,
@@ -114,20 +255,26 @@ static struct commit *pick_regular_commit(struct commit *pickme,
int cmd_replay(int argc, const char **argv, const char *prefix)
{
- struct commit *onto;
+ const char *advance_name = NULL;
+ struct commit *onto = NULL;
const char *onto_name = NULL;
- struct commit *last_commit = NULL;
+
struct rev_info revs;
+ struct commit *last_commit = NULL;
struct commit *commit;
struct merge_options merge_opt;
struct merge_result result;
+ struct strset *update_refs = NULL;
int ret = 0, i;
const char * const replay_usage[] = {
- N_("git replay --onto <newbase> <revision-range>..."),
+ N_("git replay (--onto <newbase> | --advance <branch>) <revision-range>..."),
NULL
};
struct option replay_options[] = {
+ OPT_STRING(0, "advance", &advance_name,
+ N_("branch"),
+ N_("make replay advance given branch")),
OPT_STRING(0, "onto", &onto_name,
N_("revision"),
N_("replay onto given commit")),
@@ -151,13 +298,11 @@ int cmd_replay(int argc, const char **argv, const char *prefix)
usage_with_options(replay_usage, replay_options);
}
- if (!onto_name) {
- error(_("option --onto is mandatory"));
+ if (!onto_name && !advance_name) {
+ error(_("option --onto or --advance is mandatory"));
usage_with_options(replay_usage, replay_options);
}
- onto = peel_committish(onto_name);
-
repo_init_revisions(the_repository, &revs, prefix);
argc = setup_revisions(argc, argv, &revs, NULL);
@@ -182,6 +327,12 @@ int cmd_replay(int argc, const char **argv, const char *prefix)
revs.topo_order = 1;
revs.simplify_history = 0;
+ determine_replay_mode(&revs.cmdline, onto_name, &advance_name,
+ &onto, &update_refs);
+
+ if (!onto) /* FIXME: Should handle replaying down to root commit */
+ die("Replaying down to root commit is not supported yet!");
+
if (prepare_revision_walk(&revs) < 0) {
ret = error(_("error preparing revisions"));
goto cleanup;
@@ -190,6 +341,7 @@ 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;
while ((commit = get_revision(&revs))) {
@@ -204,12 +356,15 @@ int cmd_replay(int argc, const char **argv, const char *prefix)
if (!last_commit)
break;
+ /* Update any necessary branches */
+ if (advance_name)
+ continue;
decoration = get_name_decoration(&commit->object);
if (!decoration)
continue;
-
while (decoration) {
- if (decoration->type == DECORATION_REF_LOCAL) {
+ if (decoration->type == DECORATION_REF_LOCAL &&
+ strset_contains(update_refs, decoration->name)) {
printf("update %s %s %s\n",
decoration->name,
oid_to_hex(&last_commit->object.oid),
@@ -219,10 +374,22 @@ int cmd_replay(int argc, const char **argv, const char *prefix)
}
}
+ /* In --advance mode, advance the target ref */
+ if (result.clean == 1 && advance_name) {
+ printf("update %s %s %s\n",
+ advance_name,
+ oid_to_hex(&last_commit->object.oid),
+ oid_to_hex(&onto->object.oid));
+ }
+
merge_finalize(&merge_opt, &result);
ret = result.clean;
cleanup:
+ if (update_refs) {
+ strset_clear(update_refs);
+ free(update_refs);
+ }
release_revisions(&revs);
/* Return */
diff --git a/t/t3650-replay-basics.sh b/t/t3650-replay-basics.sh
index de6e40950e..1919f7d5d1 100755
--- a/t/t3650-replay-basics.sh
+++ b/t/t3650-replay-basics.sh
@@ -96,4 +96,38 @@ test_expect_success 'using replay on bare repo with disallowed pathspec' '
test_must_fail git -C bare replay --onto main topic1..topic2 -- A.t
'
+test_expect_success 'using replay to perform basic cherry-pick' '
+ # The differences between this test and previous ones are:
+ # --advance vs --onto
+ # 2nd field of result is refs/heads/main vs. refs/heads/topic2
+ # 4th field of result is hash for main instead of hash for topic2
+
+ git replay --advance main topic1..topic2 >result &&
+
+ test_line_count = 1 result &&
+
+ git log --format=%s $(cut -f 3 -d " " result) >actual &&
+ test_write_lines E D M L B A >expect &&
+ test_cmp expect actual &&
+
+ printf "update refs/heads/main " >expect &&
+ printf "%s " $(cut -f 3 -d " " result) >>expect &&
+ git rev-parse main >>expect &&
+
+ test_cmp expect result
+'
+
+test_expect_success 'using replay on bare repo to perform basic cherry-pick' '
+ git -C bare replay --advance main topic1..topic2 >result-bare &&
+ test_cmp expect result-bare
+'
+
+test_expect_success 'replay on bare repo fails with both --advance and --onto' '
+ test_must_fail git -C bare replay --advance main --onto main topic1..topic2 >result-bare
+'
+
+test_expect_success 'replay fails when both --advance and --onto are omitted' '
+ test_must_fail git replay topic1..topic2 >result
+'
+
test_done
--
2.42.0.126.gcf8c984877
^ permalink raw reply related
* [PATCH v4 12/15] replay: disallow revision specific options and pathspecs
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, Christian Couder
In-Reply-To: <20230907092521.733746-1-christian.couder@gmail.com>
A previous commit changed `git replay` to make it accept standard
revision ranges using the setup_revisions() function. While this is a
good thing to make this command more standard and more flexible, it has
the downside of enabling many revision related options accepted and eaten
by setup_revisions().
Some of these options might make sense, but others, like those
generating non-contiguous history, might not. Anyway those we might want
to allow should probably be tested and perhaps documented a bit, which
could be done in future work.
For now it is just simpler and safer to just disallow all of them, so
let's do that.
Other commands, like `git fast-export`, currently allow all these
revision specific options even though some of them might not make sense,
as these commands also use setup_revisions() but do not check the
options that might be passed to this function.
So a way to fix those commands as well as git replay could be to improve
or refactor the setup_revisions() mechanism to let callers allow and
disallow options in a relevant way for them. Such improvements are
outside the scope of this work though.
Pathspecs, which are also accepted and eaten by setup_revisions(), are
likely to result in disconnected history. That could perhaps be useful,
but that would need tests and documentation, which can be added in
future work. So, while at it, let's disallow them too.
Helped-by: Elijah Newren <newren@gmail.com>
Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
builtin/replay.c | 26 +++++++++++++++++++++++++-
t/t3650-replay-basics.sh | 16 ++++++++++++++++
2 files changed, 41 insertions(+), 1 deletion(-)
diff --git a/builtin/replay.c b/builtin/replay.c
index de2ddeae3e..60abdaee9e 100644
--- a/builtin/replay.c
+++ b/builtin/replay.c
@@ -121,7 +121,7 @@ int cmd_replay(int argc, const char **argv, const char *prefix)
struct commit *commit;
struct merge_options merge_opt;
struct merge_result result;
- int ret = 0;
+ int ret = 0, i;
const char * const replay_usage[] = {
N_("git replay --onto <newbase> <revision-range>..."),
@@ -137,6 +137,20 @@ int cmd_replay(int argc, const char **argv, const char *prefix)
argc = parse_options(argc, argv, prefix, replay_options, replay_usage,
PARSE_OPT_KEEP_ARGV0 | PARSE_OPT_KEEP_UNKNOWN_OPT);
+ /*
+ * TODO: For now, we reject any unknown or invalid option,
+ * including revision related ones, like --not,
+ * --first-parent, etc that would be allowed and eaten by
+ * setup_revisions() below. In the future we should definitely
+ * accept those that make sense and add related tests and doc
+ * though.
+ */
+ for (i = 0; i < argc; i++)
+ if (argv[i][0] == '-') {
+ error(_("invalid option: %s"), argv[i]);
+ usage_with_options(replay_usage, replay_options);
+ }
+
if (!onto_name) {
error(_("option --onto is mandatory"));
usage_with_options(replay_usage, replay_options);
@@ -152,6 +166,16 @@ int cmd_replay(int argc, const char **argv, const char *prefix)
goto cleanup;
}
+ /*
+ * 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(_("no pathspec is allowed: '%s'"), revs.prune_data.items[0].match);
+ usage_with_options(replay_usage, replay_options);
+ }
+
/* requirements/overrides for revs */
revs.reverse = 1;
revs.sort_order = REV_SORT_IN_GRAPH_ORDER;
diff --git a/t/t3650-replay-basics.sh b/t/t3650-replay-basics.sh
index a1da4f9ef9..de6e40950e 100755
--- a/t/t3650-replay-basics.sh
+++ b/t/t3650-replay-basics.sh
@@ -80,4 +80,20 @@ test_expect_success 'using replay on bare repo to rebase with a conflict' '
test_expect_code 1 git -C bare replay --onto topic1 B..conflict
'
+test_expect_success 'using replay with (for now) disallowed revision specific option --not' '
+ test_must_fail git replay --onto main topic2 --not topic1
+'
+
+test_expect_success 'using replay on bare repo with (for now) disallowed revision specific option --first-parent' '
+ test_must_fail git -C bare replay --onto main --first-parent topic1..topic2
+'
+
+test_expect_success 'using replay with disallowed pathspec' '
+ test_must_fail git replay --onto main topic1..topic2 A.t
+'
+
+test_expect_success 'using replay on bare repo with disallowed pathspec' '
+ test_must_fail git -C bare replay --onto main topic1..topic2 -- A.t
+'
+
test_done
--
2.42.0.126.gcf8c984877
^ permalink raw reply related
* [PATCH v4 01/15] t6429: remove switching aspects of fast-rebase
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>
At the time t6429 was written, merge-ort was still under development,
did not have quite as many tests, and certainly was not widely deployed.
Since t6429 was exercising some codepaths just a little differently, we
thought having them also test the "merge_switch_to_result()" bits of
merge-ort was useful even though they weren't intrinsic to the real
point of these tests.
However, the value provided by doing extra testing of the
"merge_switch_to_result()" bits has decreased a bit over time, and it's
actively making it harder to refactor `test-tool fast-rebase` into `git
replay`, which we are going to do in following commits. Dispense with
these bits.
Co-authored-by: Christian Couder <chriscool@tuxfamily.org>
Signed-off-by: Elijah Newren <newren@gmail.com>
Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
t/helper/test-fast-rebase.c | 9 +--------
t/t6429-merge-sequence-rename-caching.sh | 9 +++++++--
2 files changed, 8 insertions(+), 10 deletions(-)
diff --git a/t/helper/test-fast-rebase.c b/t/helper/test-fast-rebase.c
index cac20a72b3..2bfab66b1b 100644
--- a/t/helper/test-fast-rebase.c
+++ b/t/helper/test-fast-rebase.c
@@ -194,7 +194,7 @@ int cmd__fast_rebase(int argc, const char **argv)
last_commit = create_commit(result.tree, commit, last_commit);
}
- merge_switch_to_result(&merge_opt, head_tree, &result, 1, !result.clean);
+ merge_finalize(&merge_opt, &result);
if (result.clean < 0)
exit(128);
@@ -213,9 +213,6 @@ int cmd__fast_rebase(int argc, const char **argv)
}
if (create_symref("HEAD", branch_name.buf, reflog_msg.buf) < 0)
die(_("unable to update HEAD"));
-
- prime_cache_tree(the_repository, the_repository->index,
- result.tree);
} else {
fprintf(stderr, "\nAborting: Hit a conflict.\n");
strbuf_addf(&reflog_msg, "rebase progress up to %s",
@@ -228,10 +225,6 @@ int cmd__fast_rebase(int argc, const char **argv)
die("Failed to update %s", argv[4]);
}
}
- if (write_locked_index(&the_index, &lock,
- COMMIT_LOCK | SKIP_IF_UNCHANGED))
- die(_("unable to write %s"), get_index_file());
-
ret = (result.clean == 0);
cleanup:
strbuf_release(&reflog_msg);
diff --git a/t/t6429-merge-sequence-rename-caching.sh b/t/t6429-merge-sequence-rename-caching.sh
index d02fa16614..75d3fd2dba 100755
--- a/t/t6429-merge-sequence-rename-caching.sh
+++ b/t/t6429-merge-sequence-rename-caching.sh
@@ -72,6 +72,7 @@ test_expect_success 'caching renames does not preclude finding new ones' '
git switch upstream &&
test-tool fast-rebase --onto HEAD upstream~1 topic &&
+ git reset --hard topic &&
#git cherry-pick upstream~1..topic
git ls-files >tracked-files &&
@@ -200,6 +201,7 @@ test_expect_success 'rename same file identically, then reintroduce it' '
export GIT_TRACE2_PERF &&
test-tool fast-rebase --onto HEAD upstream~1 topic &&
+ git reset --hard topic &&
#git cherry-pick upstream~1..topic &&
git ls-files >tracked &&
@@ -277,6 +279,7 @@ test_expect_success 'rename same file identically, then add file to old dir' '
export GIT_TRACE2_PERF &&
test-tool fast-rebase --onto HEAD upstream~1 topic &&
+ git reset --hard topic &&
#git cherry-pick upstream~1..topic &&
git ls-files >tracked &&
@@ -356,8 +359,6 @@ test_expect_success 'cached dir rename does not prevent noticing later conflict'
test_must_fail test-tool fast-rebase --onto HEAD upstream~1 topic >output &&
#git cherry-pick upstream..topic &&
- grep CONFLICT..rename/rename output &&
-
grep region_enter.*diffcore_rename trace.output >calls &&
test_line_count = 2 calls
)
@@ -456,6 +457,7 @@ test_expect_success 'dir rename unneeded, then add new file to old dir' '
export GIT_TRACE2_PERF &&
test-tool fast-rebase --onto HEAD upstream~1 topic &&
+ git reset --hard topic &&
#git cherry-pick upstream..topic &&
grep region_enter.*diffcore_rename trace.output >calls &&
@@ -522,6 +524,7 @@ test_expect_success 'dir rename unneeded, then rename existing file into old dir
export GIT_TRACE2_PERF &&
test-tool fast-rebase --onto HEAD upstream~1 topic &&
+ git reset --hard topic &&
#git cherry-pick upstream..topic &&
grep region_enter.*diffcore_rename trace.output >calls &&
@@ -624,6 +627,7 @@ test_expect_success 'caching renames only on upstream side, part 1' '
export GIT_TRACE2_PERF &&
test-tool fast-rebase --onto HEAD upstream~1 topic &&
+ git reset --hard topic &&
#git cherry-pick upstream..topic &&
grep region_enter.*diffcore_rename trace.output >calls &&
@@ -682,6 +686,7 @@ test_expect_success 'caching renames only on upstream side, part 2' '
export GIT_TRACE2_PERF &&
test-tool fast-rebase --onto HEAD upstream~1 topic &&
+ git reset --hard topic &&
#git cherry-pick upstream..topic &&
grep region_enter.*diffcore_rename trace.output >calls &&
--
2.42.0.126.gcf8c984877
^ permalink raw reply related
* Re: [PATCH v3 14/15] replay: add --contained to rebase contained branches
From: Christian Couder @ 2023-09-07 8:37 UTC (permalink / raw)
To: Toon Claes
Cc: git, Junio C Hamano, Patrick Steinhardt, Johannes Schindelin,
Elijah Newren, John Cai, Derrick Stolee, Phillip Wood,
Felipe Contreras, Calvin Wan, Christian Couder
In-Reply-To: <87cz1nst1s.fsf@iotcl.com>
On Thu, Jun 22, 2023 at 12:13 PM Toon Claes <toon@iotcl.com> wrote:
>
>
> Christian Couder <christian.couder@gmail.com> writes:
>
> > diff --git a/Documentation/git-replay.txt b/Documentation/git-replay.txt
> > index 439b2f92e7..6fcaa44ef2 100644
> > --- a/Documentation/git-replay.txt
> > +++ b/Documentation/git-replay.txt
> > @@ -9,7 +9,7 @@ git-replay - Replay commits on a different base, without touching working tree
> > SYNOPSIS
> > --------
> > [verse]
> > -'git replay' (--onto <newbase> | --advance <branch>) <revision-range>...
> > +'git replay' ([--contained] --onto <newbase> | --advance <branch>)
> > <revision-range>...
>
> I'm not sure we need this, or at least not right now.
> I've been testing with a repo having:
>
> * a13d9c4 (another-feature) yet another commit
> * c7afc2e (HEAD -> feature) third commit
> * e95cecc second commit
> * f141e77 first commit
> | * 7bb62ac (main) later commit
> | * 506cb0a another commit
> |/
> * e7acac6 initial commit
>
> I tried both commands:
>
> $ git replay --onto main main..feature main..another-feature
> $ git replay --onto main --contained main..another-feature
>
> and they both give the same result (especially with the commit following
> up this one). What is the upside of having this --contained option?
This is expected. The thing is that:
$ git replay --onto main main..another-feature
will only output something to update 'another-feature'
while:
$ git replay --onto main --contained main..another-feature
will output something to update 'another-feature' and also something
to update 'feature'.
So when you use --contained you don't need to first find the other
branches like 'feature' that point to commits between 'main' and
'another-feature', as --contained will find them for you.
> Maybe it's better to defer this patch to a separate series.
I am not sure why you are proposing this. It's true that there are
other means to achieve the same thing as --contained, but that doesn't
mean that it cannot be useful already. If there were things that
needed to be more polished in this feature, then maybe leaving it for
a separate series later might allow this series to graduate while
--contained is polished, but I don't think we are in this case.
> And another question, in git-rebase(1) a similar option is called
> --update-refs. Would you consider reusing that name here is a good idea
> for consistency?
`git replay` outputs commands that can be passed to `git update-ref
--stdin`, but for now it doesn't run that command itself. So no refs
are actually updated. If we ever add an option for git replay to also
update the refs, it would have a name probably quite similar to
--update-refs so it would be unfortunate if --update-refs is already
used for something else.
--contained also tells about the fact that the branches affected by
the option are "contained" in the revision range that is passed, which
is nice.
In short I think it's just unfortunate that git rebase already uses
--update-refs for the "contained" branches, as it would be likely to
confuse people if we would use it here for that.
^ permalink raw reply
* Re: [PATCH v3 13/15] replay: add --advance or 'cherry-pick' mode
From: Christian Couder @ 2023-09-07 8:35 UTC (permalink / raw)
To: Toon Claes
Cc: git, Junio C Hamano, Patrick Steinhardt, Johannes Schindelin,
Elijah Newren, John Cai, Derrick Stolee, Phillip Wood,
Felipe Contreras, Calvin Wan, Christian Couder
In-Reply-To: <87h6qzst8u.fsf@iotcl.com>
On Thu, Jun 22, 2023 at 12:09 PM Toon Claes <toon@iotcl.com> wrote:
>
>
> Christian Couder <christian.couder@gmail.com> writes:
>
> > + /*
> > + * When the user specifies e.g.
> > + * git replay origin/main..mybranch
> > + * git replay ^origin/next mybranch1 mybranch2
>
> When I'm trying these, I'm getting the error:
> error: option --onto or --advance is mandatory
>
> In what situation can I omit both --onto and --advance?
It was possible with version 1 of this series as one of the patches
allowed the command to guess the base:
https://lore.kernel.org/git/20230407072415.1360068-13-christian.couder@gmail.com/
so --onto wasn't needed to specify it.
Comments on that patch said that it might be better to focus on a
plumbing command first and for that the patch wasn't needed, so I
removed it in version 2.
> > +static void determine_replay_mode(struct rev_cmdline_info *cmd_info,
> > + const char *onto_name,
> > + const char **advance_name,
> > + struct commit **onto,
>
> Would it make sense to call this target?
This is more the new base we are rebasing target branches onto. So if
we want to change the name, `--base` or `--newbase` would make more
sense. `git rebase` already has `--onto` though, so, if we want to be
consistent with it, we should keep `--onto` for this.
> > + struct strset **update_refs)
> > +{
> > + struct ref_info rinfo;
> > +
> > + get_ref_information(cmd_info, &rinfo);
> > + if (!rinfo.positive_refexprs)
> > + die(_("need some commits to replay"));
> > + if (onto_name && *advance_name)
> > + die(_("--onto and --advance are incompatible"));
>
> Do we actually need to disallow this? I mean from git-replay's point of
> view, there's no technical limitation why can cannot support both modes
> at once. The update-ref commands in the output will update both target
> and source branches, but it's not up to us whether that's desired.
I am not sure what you call "target" and "source" branches. Anyway
here is in simple terms the way the command works:
1) it takes either `--onto <newbase>` or `--advance <branch>` and
then one or more <revision-range>,
2) it replays all the commits in the <revision-range> onto either
<newbase> or <branch>,
3) in case of `--advance`, it outputs a single command for `git
update-ref --stdin` to advance <branch> to the last commit that was
just replayed,
4) in case of `--onto`, it outputs a number of commands for `git
update-ref --stdin` to update the branches in <revision-range> to
where the tip commits of these branches have been replayed.
So `--advance` is like a cherry-pick, and `--onto` is like a rebase.
It would be possible to do both a rebase onto a branch and a
cherry-pick of the rebased commits onto that branch, but this is not
very common and you can achieve the same result by just rebasing and
then using `git reset` or `git update-ref` to make the branch point to
the result of the rebase. So I don't see the point of complicating the
command at this point.
> > + else if (onto_name) {
>
> No need to 'else' here IMHO.
>
> > + *onto = peel_committish(onto_name);
> > + if (rinfo.positive_refexprs <
> > + strset_get_size(&rinfo.positive_refs))
> > + die(_("all positive revisions given must be references"));
>
> I tested this locally with the following command:
>
> $ git replay --onto main OID..OID
>
> This command didn't give any errors, neither did it return any
> update-ref lines. I would have expected to hit this die().
Yeah, this might be unexpected.
I tested it too and 'rinfo.positive_refexprs' is 1 while
'strset_get_size(&rinfo.positive_refs)' is 0 with such a command.
The result does not look wrong though. Above that code there is:
if (!rinfo.positive_refexprs)
die(_("need some commits to replay"));
so it looks like there is at least a check that the revision range
passed to the command contains positive commits.
It might be possible that users prefer a command that outputs nothing
when there is nothing to replay instead of erroring out.
> > + } else if (*advance_name) {
> > + struct object_id oid;
> > + char *fullname = NULL;
> > +
> > + *onto = peel_committish(*advance_name);
> > + if (repo_dwim_ref(the_repository, *advance_name, strlen(*advance_name),
> > + &oid, &fullname, 0) == 1) {
> > + *advance_name = fullname;
> > + } else {
> > + die(_("argument to --advance must be a reference"));
> > + }
> > + if (rinfo.positive_refexprs > 1)
> > + die(_("cannot advance target with multiple source branches because ordering would be ill-defined"));
>
> The sources aren't always branches, so I suggest something like:
>
> + die(_("cannot advance target with multiple sources because ordering would be ill-defined"));
Yeah, that looks reasonable. I have made this change in version 4 I
will send very soon.
> > + determine_replay_mode(&revs.cmdline, onto_name, &advance_name,
> > + &onto, &update_refs);
> > +
> > + if (!onto) /* FIXME: Should handle replaying down to root commit */
> > + die("Replaying down to root commit is not supported yet!");
>
> When I was testing locally I tried the following:
>
> $ git replay --onto main feature
>
> I was expecting this command to find the common ancestor automatically,
> but instead I got this error. I'm fine if for now the command does not
> determine the common ancestor yet, but I think we should provide a
> better error for this scenario.
I agree that it isn't very user friendly. We could indeed try to find
if there is a common ancestor, and, if that's the case, suggest
another way to call the command. This is a plumbing command in its
early stage though for now. So I guess it's Ok to postpone working on
nicer error messages.
> > +test_expect_success 'using replay on bare repo to perform basic cherry-pick' '
> > + git -C bare replay --advance main topic1..topic2 >result-bare &&
> > + test_cmp expect result-bare
> > +'
> > +
> > test_done
>
> Shall we add a test case when providing both --onto and --advance? And
> one that omits both?
Ok, I have made this change in version 4.
^ permalink raw reply
* [PATCH v4 14/15] replay: add --contained to rebase contained branches
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 add a `--contained` option that can be used along with
`--onto` to rebase all the branches contained in the <revision-range>
argument.
Co-authored-by: Christian Couder <chriscool@tuxfamily.org>
Signed-off-by: Elijah Newren <newren@gmail.com>
Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
Documentation/git-replay.txt | 12 +++++++++++-
builtin/replay.c | 12 ++++++++++--
t/t3650-replay-basics.sh | 29 +++++++++++++++++++++++++++++
3 files changed, 50 insertions(+), 3 deletions(-)
diff --git a/Documentation/git-replay.txt b/Documentation/git-replay.txt
index 5c5c15237d..b94c39b161 100644
--- a/Documentation/git-replay.txt
+++ b/Documentation/git-replay.txt
@@ -9,7 +9,7 @@ git-replay - Replay commits on a different base, without touching working tree
SYNOPSIS
--------
[verse]
-'git replay' (--onto <newbase> | --advance <branch>) <revision-range>...
+'git replay' ([--contained] --onto <newbase> | --advance <branch>) <revision-range>...
DESCRIPTION
-----------
@@ -93,6 +93,16 @@ top of the exact same new base, they only differ in that the first
provides instructions to make mybranch point at the new commits and
the second provides instructions to make target point at them.
+What if you have a stack of branches, one depending upon another, and
+you'd really like to rebase the whole set?
+
+------------
+$ git replay --contained --onto origin/main origin/main..tipbranch
+update refs/heads/branch1 ${NEW_branch1_HASH} ${OLD_branch1_HASH}
+update refs/heads/branch2 ${NEW_branch2_HASH} ${OLD_branch2_HASH}
+update refs/heads/tipbranch ${NEW_tipbranch_HASH} ${OLD_tipbranch_HASH}
+------------
+
When calling `git replay`, one does not need to specify a range of
commits to replay using the syntax `A..B`; any range expression will
do:
diff --git a/builtin/replay.c b/builtin/replay.c
index 6b89964be9..a7d36a639c 100644
--- a/builtin/replay.c
+++ b/builtin/replay.c
@@ -258,6 +258,7 @@ int cmd_replay(int argc, const char **argv, const char *prefix)
const char *advance_name = NULL;
struct commit *onto = NULL;
const char *onto_name = NULL;
+ int contained = 0;
struct rev_info revs;
struct commit *last_commit = NULL;
@@ -268,7 +269,7 @@ int cmd_replay(int argc, const char **argv, const char *prefix)
int ret = 0, i;
const char * const replay_usage[] = {
- N_("git replay (--onto <newbase> | --advance <branch>) <revision-range>..."),
+ N_("git replay ([--contained] --onto <newbase> | --advance <branch>) <revision-range>..."),
NULL
};
struct option replay_options[] = {
@@ -278,6 +279,8 @@ int cmd_replay(int argc, const char **argv, const char *prefix)
OPT_STRING(0, "onto", &onto_name,
N_("revision"),
N_("replay onto given commit")),
+ OPT_BOOL(0, "contained", &contained,
+ N_("advance all branches contained in revision-range")),
OPT_END()
};
@@ -303,6 +306,10 @@ int cmd_replay(int argc, const char **argv, const char *prefix)
usage_with_options(replay_usage, replay_options);
}
+ if (advance_name && contained)
+ die(_("options '%s' and '%s' cannot be used together"),
+ "--advance", "--contained");
+
repo_init_revisions(the_repository, &revs, prefix);
argc = setup_revisions(argc, argv, &revs, NULL);
@@ -364,7 +371,8 @@ int cmd_replay(int argc, const char **argv, const char *prefix)
continue;
while (decoration) {
if (decoration->type == DECORATION_REF_LOCAL &&
- strset_contains(update_refs, decoration->name)) {
+ (contained || strset_contains(update_refs,
+ decoration->name))) {
printf("update %s %s %s\n",
decoration->name,
oid_to_hex(&last_commit->object.oid),
diff --git a/t/t3650-replay-basics.sh b/t/t3650-replay-basics.sh
index 1919f7d5d1..57d2ef9ea4 100755
--- a/t/t3650-replay-basics.sh
+++ b/t/t3650-replay-basics.sh
@@ -130,4 +130,33 @@ test_expect_success 'replay fails when both --advance and --onto are omitted' '
test_must_fail git replay topic1..topic2 >result
'
+test_expect_success 'using replay to also rebase a contained branch' '
+ git replay --contained --onto main main..topic3 >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 F C M L B A >expect &&
+ test_cmp expect actual &&
+
+ git log --format=%s $(tail -n 1 new-branch-tips) >actual &&
+ test_write_lines H G F C M L B A >expect &&
+ test_cmp expect actual &&
+
+ printf "update refs/heads/topic1 " >expect &&
+ printf "%s " $(head -n 1 new-branch-tips) >>expect &&
+ git rev-parse topic1 >>expect &&
+ printf "update refs/heads/topic3 " >>expect &&
+ printf "%s " $(tail -n 1 new-branch-tips) >>expect &&
+ git rev-parse topic3 >>expect &&
+
+ test_cmp expect result
+'
+
+test_expect_success 'using replay on bare repo to also rebase a contained branch' '
+ git -C bare replay --contained --onto main main..topic3 >result-bare &&
+ test_cmp expect result-bare
+'
+
test_done
--
2.42.0.126.gcf8c984877
^ permalink raw reply related
* [PATCH v4 10/15] replay: make it a minimal server side command
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 this command to be a minimal command that just does server side
picking of commits, displaying the results on stdout for higher level
scripts to consume.
So let's simplify it:
* remove the worktree and index reading/writing,
* remove the ref (and reflog) updating,
* remove the assumptions tying us to HEAD, since (a) this is not a
rebase and (b) we want to be able to pick commits in a bare repo,
i.e. to/from branches that are not checked out and not the main
branch,
* remove unneeded includes,
* handle rebasing multiple branches by printing on stdout the update
ref commands that should be performed.
The output can be piped into `git update-ref --stdin` for the ref
updates to happen.
In the future to make it easier for users to use this command
directly maybe an option can be added to automatically pipe its output
into `git update-ref`.
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 | 78 ++++++++----------------
t/t6429-merge-sequence-rename-caching.sh | 39 +++++++-----
2 files changed, 50 insertions(+), 67 deletions(-)
diff --git a/builtin/replay.c b/builtin/replay.c
index a2636fbdcc..e45cd59da1 100644
--- a/builtin/replay.c
+++ b/builtin/replay.c
@@ -6,11 +6,7 @@
#include "git-compat-util.h"
#include "builtin.h"
-#include "cache-tree.h"
-#include "commit.h"
#include "environment.h"
-#include "gettext.h"
-#include "hash.h"
#include "hex.h"
#include "lockfile.h"
#include "merge-ort.h"
@@ -18,8 +14,6 @@
#include "parse-options.h"
#include "refs.h"
#include "revision.h"
-#include "sequencer.h"
-#include "setup.h"
#include "strvec.h"
#include <oidset.h>
#include <tree.h>
@@ -102,6 +96,7 @@ static struct commit *pick_regular_commit(struct commit *pickme,
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->branch2 = short_commit_name(pickme);
merge_opt->ancestor = xstrfmt("parent of %s", merge_opt->branch2);
@@ -122,15 +117,12 @@ 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 lock_file lock = LOCK_INIT;
+ struct commit *last_commit = NULL;
struct strvec rev_walk_args = STRVEC_INIT;
struct rev_info revs;
struct commit *commit;
struct merge_options merge_opt;
- struct tree *head_tree;
struct merge_result result;
- struct strbuf reflog_msg = STRBUF_INIT;
struct strbuf branch_name = STRBUF_INIT;
int ret = 0;
@@ -161,10 +153,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]);
- repo_hold_locked_index(the_repository, &lock, LOCK_DIE_ON_ERROR);
- if (repo_read_index(the_repository) < 0)
- BUG("Could not read index");
-
repo_init_revisions(the_repository, &revs, prefix);
strvec_pushl(&rev_walk_args, "", argv[2], "--not", argv[1], NULL);
@@ -190,58 +178,44 @@ 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;
- merge_opt.branch1 = "HEAD";
- head_tree = repo_get_commit_tree(the_repository, onto);
- result.tree = head_tree;
+ result.tree = repo_get_commit_tree(the_repository, onto);
last_commit = onto;
while ((commit = get_revision(&revs))) {
- struct commit *pick;
+ const struct name_decoration *decoration;
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!"));
- pick = pick_regular_commit(commit, last_commit, &merge_opt, &result);
- if (!pick)
+ last_commit = pick_regular_commit(commit, last_commit, &merge_opt, &result);
+ if (!last_commit)
break;
- last_commit = pick;
- last_picked_commit = commit;
+
+ decoration = get_name_decoration(&commit->object);
+ if (!decoration)
+ continue;
+
+ while (decoration) {
+ if (decoration->type == DECORATION_REF_LOCAL) {
+ printf("update %s %s %s\n",
+ decoration->name,
+ oid_to_hex(&last_commit->object.oid),
+ oid_to_hex(&commit->object.oid));
+ }
+ decoration = decoration->next;
+ }
}
merge_finalize(&merge_opt, &result);
+ ret = result.clean;
- if (result.clean < 0)
- exit(128);
-
- if (result.clean) {
- strbuf_addf(&reflog_msg, "finish rebase %s onto %s",
- oid_to_hex(&last_picked_commit->object.oid),
- oid_to_hex(&last_commit->object.oid));
- if (update_ref(reflog_msg.buf, branch_name.buf,
- &last_commit->object.oid,
- &last_picked_commit->object.oid,
- REF_NO_DEREF, UPDATE_REFS_MSG_ON_ERR)) {
- 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"));
- } else {
- strbuf_addf(&reflog_msg, "rebase progress up to %s",
- oid_to_hex(&last_picked_commit->object.oid));
- if (update_ref(reflog_msg.buf, "HEAD",
- &last_commit->object.oid,
- &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]);
- }
- }
- ret = (result.clean == 0);
cleanup:
- strbuf_release(&reflog_msg);
strbuf_release(&branch_name);
release_revisions(&revs);
- return ret;
+
+ /* Return */
+ if (ret < 0)
+ exit(128);
+ return ret ? 0 : 1;
}
diff --git a/t/t6429-merge-sequence-rename-caching.sh b/t/t6429-merge-sequence-rename-caching.sh
index 7670b72008..099aefeffc 100755
--- a/t/t6429-merge-sequence-rename-caching.sh
+++ b/t/t6429-merge-sequence-rename-caching.sh
@@ -71,8 +71,9 @@ test_expect_success 'caching renames does not preclude finding new ones' '
git switch upstream &&
- git replay --onto HEAD upstream~1 topic &&
- git reset --hard topic &&
+ git replay --onto HEAD upstream~1 topic >out &&
+ git update-ref --stdin <out &&
+ git checkout topic &&
git ls-files >tracked-files &&
test_line_count = 2 tracked-files &&
@@ -140,7 +141,9 @@ test_expect_success 'cherry-pick both a commit and its immediate revert' '
GIT_TRACE2_PERF="$(pwd)/trace.output" &&
export GIT_TRACE2_PERF &&
- git replay --onto HEAD upstream~1 topic &&
+ git replay --onto HEAD upstream~1 topic >out &&
+ git update-ref --stdin <out &&
+ git checkout topic &&
grep region_enter.*diffcore_rename trace.output >calls &&
test_line_count = 1 calls
@@ -198,8 +201,9 @@ test_expect_success 'rename same file identically, then reintroduce it' '
GIT_TRACE2_PERF="$(pwd)/trace.output" &&
export GIT_TRACE2_PERF &&
- git replay --onto HEAD upstream~1 topic &&
- git reset --hard topic &&
+ git replay --onto HEAD upstream~1 topic >out &&
+ git update-ref --stdin <out &&
+ git checkout topic &&
git ls-files >tracked &&
test_line_count = 2 tracked &&
@@ -275,8 +279,9 @@ test_expect_success 'rename same file identically, then add file to old dir' '
GIT_TRACE2_PERF="$(pwd)/trace.output" &&
export GIT_TRACE2_PERF &&
- git replay --onto HEAD upstream~1 topic &&
- git reset --hard topic &&
+ git replay --onto HEAD upstream~1 topic >out &&
+ git update-ref --stdin <out &&
+ git checkout topic &&
git ls-files >tracked &&
test_line_count = 4 tracked &&
@@ -451,8 +456,9 @@ test_expect_success 'dir rename unneeded, then add new file to old dir' '
GIT_TRACE2_PERF="$(pwd)/trace.output" &&
export GIT_TRACE2_PERF &&
- git replay --onto HEAD upstream~1 topic &&
- git reset --hard topic &&
+ git replay --onto HEAD upstream~1 topic >out &&
+ git update-ref --stdin <out &&
+ git checkout topic &&
grep region_enter.*diffcore_rename trace.output >calls &&
test_line_count = 2 calls &&
@@ -517,8 +523,9 @@ test_expect_success 'dir rename unneeded, then rename existing file into old dir
GIT_TRACE2_PERF="$(pwd)/trace.output" &&
export GIT_TRACE2_PERF &&
- git replay --onto HEAD upstream~1 topic &&
- git reset --hard topic &&
+ git replay --onto HEAD upstream~1 topic >out &&
+ git update-ref --stdin <out &&
+ git checkout topic &&
grep region_enter.*diffcore_rename trace.output >calls &&
test_line_count = 3 calls &&
@@ -619,8 +626,9 @@ test_expect_success 'caching renames only on upstream side, part 1' '
GIT_TRACE2_PERF="$(pwd)/trace.output" &&
export GIT_TRACE2_PERF &&
- git replay --onto HEAD upstream~1 topic &&
- git reset --hard topic &&
+ git replay --onto HEAD upstream~1 topic >out &&
+ git update-ref --stdin <out &&
+ git checkout topic &&
grep region_enter.*diffcore_rename trace.output >calls &&
test_line_count = 1 calls &&
@@ -677,8 +685,9 @@ test_expect_success 'caching renames only on upstream side, part 2' '
GIT_TRACE2_PERF="$(pwd)/trace.output" &&
export GIT_TRACE2_PERF &&
- git replay --onto HEAD upstream~1 topic &&
- git reset --hard topic &&
+ git replay --onto HEAD upstream~1 topic >out &&
+ git update-ref --stdin <out &&
+ git checkout 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 v3 10/15] replay: make it a minimal server side command
From: Christian Couder @ 2023-09-07 8:32 UTC (permalink / raw)
To: Toon Claes
Cc: git, Junio C Hamano, Patrick Steinhardt, Johannes Schindelin,
Elijah Newren, John Cai, Derrick Stolee, Phillip Wood,
Felipe Contreras, Calvin Wan, Christian Couder
In-Reply-To: <87pm5nstli.fsf@iotcl.com>
On Thu, Jun 22, 2023 at 12:01 PM Toon Claes <toon@iotcl.com> wrote:
>
> Christian Couder <christian.couder@gmail.com> writes:
>
> > + decoration = decoration->next;
> > + }
> > }
> >
> > + /* Cleanup */
>
> Nit: I don't think this comment adds much value. I would argue it's more
> confusing to have it, because there's a label cleanup: just a few lines
> down.
Ok, I have removed that comment in the version 4 I will send soon.
^ permalink raw reply
* Re: Is "bare"ness in the context of multiple worktrees weird? Bitmap error in git gc.
From: Kristoffer Haugsbakk @ 2023-09-07 15:07 UTC (permalink / raw)
To: Junio C Hamano
Cc: Sergey Organov, Eric Sunshine, Johannes Schindelin, Taylor Blau,
Patrick Steinhardt, git, Tao Klerks
In-Reply-To: <xmqqledjm4k2.fsf@gitster.g>
Hi again Junio
On Wed, Sep 6, 2023, at 22:26, Junio C Hamano 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.
This gives the same error if `there` is a bare repository. Is that
intended?
This goes back to my point about missing nomenclature: it's weird if the
“main working tree” can be a bare repository.
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.
> It probably does not add much value to introduce a new term
> "inline".
The reason that I like it is because it lets you describe a bare
repository with linked worktrees. Not because it would replace “main
worktree”.
Although in light of Sergey's post about inline/attached, the “main
worktree” term *might* start to look a bit anachronistic. But I'm not
sure.
> 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 applaud whoever wrote this sentence for packing so much good
> information in a concise and easy-to-understand description.
I agree that it is very elegant.
> Perhaps we should borrow it to update the glossary, like so?
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.
Cheers
--
Kristoffer Haugsbakk
^ permalink raw reply
* Re: [PATCH v3 11/15] replay: use standard revision ranges
From: Christian Couder @ 2023-09-07 8:32 UTC (permalink / raw)
To: Toon Claes
Cc: git, Junio C Hamano, Patrick Steinhardt, Johannes Schindelin,
Elijah Newren, John Cai, Derrick Stolee, Phillip Wood,
Felipe Contreras, Calvin Wan, Christian Couder
In-Reply-To: <87legbsths.fsf@iotcl.com>
On Thu, Jun 22, 2023 at 12:03 PM Toon Claes <toon@iotcl.com> wrote:
>
> Christian Couder <christian.couder@gmail.com> writes:
>
> > +DESCRIPTION
> > +-----------
> > +
> > +Takes a range of commits, and replays them onto a new location. Does
> > +not touch the working tree or index, and does not update any
> > +references. However, the output of this command is meant to be used
>
> Small suggestion here:
>
> Takes a range of commits, and replays them onto a new location. Does
> neither touch the working tree nor index, and does not update any
> references.
I am not a native speaker, so I am not sure what's best here. I find
your suggestion a bit less clear though, so until a native speaker
agrees with it or maybe finds something even better, I prefer to leave
it as-is.
^ permalink raw reply
* [PATCH] rebase -i: ignore signals when forking subprocesses
From: Phillip Wood via GitGitGadget @ 2023-09-07 10:03 UTC (permalink / raw)
To: git; +Cc: Johannes Schindelin, Jeff King, Phillip Wood, Phillip Wood
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.
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?
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 related
* [PATCH v4 04/15] replay: die() instead of failing assert()
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>
It's not a good idea for regular Git commands to use an assert() to
check for things that could happen but are not supported.
Let's die() with an explanation of the issue instead.
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 | 7 ++++++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/builtin/replay.c b/builtin/replay.c
index d6dec7c866..f3fdbe48c9 100644
--- a/builtin/replay.c
+++ b/builtin/replay.c
@@ -179,7 +179,12 @@ int cmd_replay(int argc, const char **argv, const char *prefix)
fprintf(stderr, "Rebasing %s...\r",
oid_to_hex(&commit->object.oid));
- assert(commit->parents && !commit->parents->next);
+
+ 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!"));
+
base = commit->parents->item;
next_tree = repo_get_commit_tree(the_repository, commit);
--
2.42.0.126.gcf8c984877
^ permalink raw reply related
* Re: [PATCH 7/8] builtin/repack.c: drop `DELETE_PACK` macro
From: Patrick Steinhardt @ 2023-09-07 8:19 UTC (permalink / raw)
To: Taylor Blau; +Cc: Junio C Hamano, git, Jeff King
In-Reply-To: <ZPi1c98o2fKB/U+e@nand.local>
[-- Attachment #1: Type: text/plain, Size: 4512 bytes --]
On Wed, Sep 06, 2023 at 01:22:59PM -0400, Taylor Blau wrote:
> On Wed, Sep 06, 2023 at 10:05:37AM -0700, Junio C Hamano wrote:
> > Taylor Blau <me@ttaylorr.com> writes:
> >
> > > Signed-off-by: Taylor Blau <me@ttaylorr.com>
> > > ---
> > > builtin/repack.c | 18 ++++++++++--------
> > > 1 file changed, 10 insertions(+), 8 deletions(-)
> >
> > The reason being...?
>
> Wow, I have no idea how this got sent out without a commit message! At
> least it was signed off ;-).
>
> Here's a replacement that I cooked up, with your Helped-by. Let me know
> if you want to replace this patch manually, or if you'd prefer a reroll
> of the series. Either is fine with me! :-)
Personally I think that the original version is still more readable. If
you simply see `if (item->util)` you don't really have any indicator
what this actually means, whereas previously the more verbose check for
`if ((uintptr)item->util & DELETE_PACK)` gives the reader a nice hint
what this may be about.
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;
}
```
Other than that the rest of this series looks good to me, thanks.
Patrick
> --- 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.
>
> Helped-by: Junio C Hamano <gitster@pobox.com>
> Signed-off-by: Taylor Blau <me@ttaylorr.com>
> ---
> builtin/repack.c | 18 +++++++++++-------
> 1 file changed, 11 insertions(+), 7 deletions(-)
>
> diff --git a/builtin/repack.c b/builtin/repack.c
> index 478fab96c9..575e69b020 100644
> --- a/builtin/repack.c
> +++ b/builtin/repack.c
> @@ -26,7 +26,7 @@
> #define LOOSEN_UNREACHABLE 2
> #define PACK_CRUFT 4
>
> -#define DELETE_PACK 1
> +#define DELETE_PACK ((void*)(uintptr_t)1)
>
> static int pack_everything;
> static int delta_base_offset = 1;
> @@ -96,6 +96,10 @@ static int repack_config(const char *var, const char *value,
>
> struct existing_packs {
> struct string_list kept_packs;
> + /*
> + * for both non_kept_packs, and cruft_packs, a non-NULL
> + * 'util' field indicates the pack should be deleted.
> + */
> struct string_list non_kept_packs;
> struct string_list cruft_packs;
> };
> @@ -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;
> }
> }
>
> @@ -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;
> remove_redundant_pack(packdir, item->string);
> }
> @@ -695,20 +699,20 @@ static void midx_included_packs(struct string_list *include,
>
> for_each_string_list_item(item, &existing->cruft_packs) {
> /*
> - * no need to check DELETE_PACK, since we're not
> - * doing an ALL_INTO_ONE repack
> + * no need to check for deleted packs, since we're
> + * not doing an ALL_INTO_ONE repack
> */
> string_list_insert(include, xstrfmt("%s.idx", item->string));
> }
> } else {
> for_each_string_list_item(item, &existing->non_kept_packs) {
> - if ((uintptr_t)item->util & DELETE_PACK)
> + if (item->util)
> 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)
> + if (item->util)
> continue;
> string_list_insert(include, xstrfmt("%s.idx", item->string));
> }
> --
> 2.38.0.16.g393fd4c6db
>
> --- >8 ---
>
> Thanks,
> Taylor
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH 0/2] replacing ci/config/allow-ref with a repo variable
From: Phillip Wood @ 2023-09-07 10:04 UTC (permalink / raw)
To: Jeff King, phillip.wood; +Cc: git, Johannes Schindelin
In-Reply-To: <20230905072444.GH199565@coredump.intra.peff.net>
On 05/09/2023 08:24, Jeff King wrote:
> On Mon, Sep 04, 2023 at 10:56:15AM +0100, Phillip Wood wrote:
>
>>> Yes, I think it would be possible to do something like:
>>>
>>> if: |
>>> (vars.CI_BRANCHES == '' || contains(vars.CI_BRANCHES, github.ref_name)) &&
>>> !contains(vars.CI_BRANCHES_REJECT, github.ref_name)
>>>
>>> It doesn't allow globbing, though. Do you need that?
>>
>> Oh I'd missed that, yes I do. All the globs are prefix matches but I'm not
>> sure that helps.
>
> It does make it easier. There's no globbing function available to us,
> but if we know something is a prefix, there's a startsWith() we can use.
> It does seem we're getting a combinatorial expansion of things to check,
> though:
>
> - full names to accept
> - full names to reject
> - prefixes to accept
> - prefixes to reject
>
> I wrote "prefixes" but I'm actually not sure how feasible that is. That
> implies iterating over the list of prefixes, which I'm not sure we can
> do.
I scanned the github documentation the other day and wondered if it
would be possible to use with fromJson with a json array to do a prefx
match on each element. It all sounds like it is getting a bit
complicated though.
Best Wishes
Phillip
> -Peff
^ permalink raw reply
* Re: [PATCH v3 13/15] replay: add --advance or 'cherry-pick' mode
From: Christian Couder @ 2023-09-07 8:35 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: <xmqq8rb3is8c.fsf@gitster.g>
On Tue, Jul 25, 2023 at 11:41 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Christian Couder <christian.couder@gmail.com> writes:
>
> > There is already a 'rebase' mode with `--onto`. Let's add an 'advance' or
> > 'cherry-pick' mode with `--advance`. This new mode will make the target
> > branch advance as we replay commits onto it.
>
> If I say
>
> $ git replay --(advance|onto) xyzzy frotz..nitfol yomin
>
> where the topology of the cherry-picked range look like this
>
> x---x---Y yomin
> /
> E---F---x----x----N nitfol
> / frotz
> /
> X xyzzy
>
> after transplanting the commits, we would get something like
>
> x---x---Y yomin
> /
> E---F---x----x----N nitfol
> / frotz
> /
> X---x----x----N'
> \
> x---x---Y'
>
> Now, if this was done with --onto, nitfol and yomin would point at
> N' and Y', but with --advance, where would xyzzy go?
>
> Yes, my point is that without --advance, there always is a
> reasonable set of branch tips that will be moved, but with
> "--advance", you cannot guarantee that you have any reasonable
> answer to that question.
>
> The answer could be "when there is no single 'tip of the new
> history', the command with '--advance' errors out", but whatever
> behaviour we choose, it should be documented.
Ok, I have improved the commit message by adding the following:
"The replayed commits should have a single tip, so that it's clear where
the target branch should be advanced. If they have more than one tip,
this new mode will error out."
I have also updated the doc for <revision-range> like this:
"<revision-range>::
Range of commits to replay. More than one <revision-range> can
be passed, but in `--advance <branch>` mode, they should have
a single tip, so that it's clear where <branch> should point
to. See "Specifying Ranges" in linkgit:git-rev-parse."
> > +--advance <branch>::
> > + Starting point at which to create the new commits; must be a
> > + branch name.
> > ++
> > +When `--advance` is specified, the update-ref command(s) in the output
> > +will update the branch passed as an argument to `--advance` to point at
> > +the new commits (in other words, this mimics a cherry-pick operation).
>
> This part is not giving much useful information to determine the
> answer (which might be fine, as long as the answer can easily be
> found in some other parts of this document, but I would have
> expected everything necessary would be found here or one item before
> this one about --onto).
The doc about <revision-range> is just after the above, so I think the
above change in the <revision-range> doc is Ok and enough here.
> > @@ -47,7 +55,10 @@ input to `git update-ref --stdin`. It is basically of the form:
> > update refs/heads/branch2 ${NEW_branch2_HASH} ${OLD_branch2_HASH}
> > update refs/heads/branch3 ${NEW_branch3_HASH} ${OLD_branch3_HASH}
> >
> > -where the number of refs updated depend on the arguments passed.
> > +where the number of refs updated depend on the arguments passed. When
> > +using `--advance`, the number of refs updated is always one, but for
> > +`--onto`, it can be one or more (rebasing multiple branches
> > +simultaneously is supported).
>
> "dependS on the arguments passed" is not incorrect per-se, in the
> sense that if you "replay --onto X E..N" (in the above picture), I
> think you'll move F and N (two), while "F..N" will only move N
> (one). But the important thing to convey is how many branches had
> their tips in the replayed range, no? "depends on the shape of the
> history being replayed" would be a more correct thing to say for the
> "--onto" mode. For "--advance", presumably you would require to
> have a single positive endpoint [*], so "depends on the arguments"
> is still not wrong per-se, because "when --advance is part of the
> arguments, the number becomes one".
Yeah, I agree.
> Side note: even then, I suspect that
>
> git replay --advance X E..F N
>
> should be allowed, simply because there is only one sensible
> interpretation. You'll end up with a single strand of
> pearls F--x--x--N transplanted on top of X, and the range
> happens to contain F and N but it is obvious the end result
> should advance xyzzy to N because F fast-forwards to N.
>
> I'd say "where the number of ..." and everything after these sample
> "update" lines should be removed,
I am not sure it's a good thing to remove that, as I think repeating
how things work in the context of an example output can help people
understand. I have updated these sentences to the following:
"where the number of refs updated depends on the arguments passed and
the shape of the history being replayed. When using `--advance`, the
number of refs updated is always one, but for `--onto`, it can be one
or more (rebasing multiple branches simultaneously is supported)."
> and instead we should add a few
> words to the main description of the options, e.g. for "--onto"
>
> > +When `--onto` is specified, the update-ref command(s) in the output will
> > +update the branch(es) in the revision range to point at the new
> > +commits (in other words, this mimics a rebase operation).
>
> we could update the above to something like
>
> ... will update the branches in the revision range to point at the
> new commits, similar to the way how "rebase --update-refs" updates
> multiple branches in the affected range.
Yeah, I agree. In the version 4 I will send soon, have updated the above to:
"When `--onto` is specified, the update-ref command(s) in the output will
update the branch(es) in the revision range to point at the new
commits, similar to the way how `git rebase --update-refs` updates
multiple branches in the affected range."
^ permalink raw reply
* [PATCH v4 07/15] replay: add an important FIXME comment about gpg signing
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 to be able to handle signed commits in some way in the future,
but we are not ready to do it now. So for the time being let's just add
a FIXME comment to remind us about it.
These are different ways we could handle them:
- in case of a cli user and if there was an interactive mode, we could
perhaps ask if the user wants to sign again
- we could add an option to just fail if there are signed 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 | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/builtin/replay.c b/builtin/replay.c
index 4b1e501595..47d695df93 100644
--- a/builtin/replay.c
+++ b/builtin/replay.c
@@ -62,7 +62,7 @@ static struct commit *create_commit(struct tree *tree,
struct object *obj;
struct commit_list *parents = NULL;
char *author;
- char *sign_commit = NULL;
+ char *sign_commit = NULL; /* FIXME: cli users might want to sign again */
struct commit_extra_header *extra;
struct strbuf msg = STRBUF_INIT;
const char *out_enc = get_commit_output_encoding();
--
2.42.0.126.gcf8c984877
^ permalink raw reply related
* Re: [PATCH v4 06/15] replay: don't simplify history
From: Johannes Schindelin @ 2023-09-07 10:23 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-7-christian.couder@gmail.com>
Hi Christian & Elijah,
On Thu, 7 Sep 2023, Christian Couder wrote:
> From: Elijah Newren <newren@gmail.com>
>
> Let's set the rev walking options we need after calling
> setup_revisions() instead of before. This makes it clearer which options
> we need.
In light of the currently open issue about command-line validation, this
change does more than this paragraph lets on: It hardcodes certain
settings, overriding (silently) any rev-list options the user might have
passed.
Is there any chance that we can avoid this change?
> Also we don't want history simplification, as we want to deal with all
> the commits in the affected range.
This, however, is a good change. It deserves to live in its own commit,
with its own commit message, in particular because it is not obvious from
the attribute names which ones we're talking about (I guess it's `limited`
and `simplify_history`, not just the latter.
Ciao,
Johannes
^ permalink raw reply
* [PATCH v4 06/15] replay: don't simplify history
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 set the rev walking options we need after calling
setup_revisions() instead of before. This makes it clearer which options
we need.
Also we don't want history simplification, as we want to deal with all
the commits in the affected range.
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 | 15 ++++++---------
1 file changed, 6 insertions(+), 9 deletions(-)
diff --git a/builtin/replay.c b/builtin/replay.c
index c66888679b..4b1e501595 100644
--- a/builtin/replay.c
+++ b/builtin/replay.c
@@ -173,15 +173,6 @@ int cmd_replay(int argc, const char **argv, const char *prefix)
repo_init_revisions(the_repository, &revs, prefix);
- revs.verbose_header = 1;
- revs.max_parents = 1;
- revs.cherry_mark = 1;
- revs.limited = 1;
- revs.reverse = 1;
- revs.right_only = 1;
- revs.sort_order = REV_SORT_IN_GRAPH_ORDER;
- revs.topo_order = 1;
-
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) {
@@ -189,6 +180,12 @@ int cmd_replay(int argc, const char **argv, const char *prefix)
goto cleanup;
}
+ /* requirements/overrides for revs */
+ revs.reverse = 1;
+ revs.sort_order = REV_SORT_IN_GRAPH_ORDER;
+ revs.topo_order = 1;
+ revs.simplify_history = 0;
+
strvec_clear(&rev_walk_args);
if (prepare_revision_walk(&revs) < 0) {
--
2.42.0.126.gcf8c984877
^ permalink raw reply related
* Re: [PATCH v4 12/15] replay: disallow revision specific options and pathspecs
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-13-christian.couder@gmail.com>
Hi Christian & Elijah,
On Thu, 7 Sep 2023, Christian Couder wrote:
> A previous commit changed `git replay` to make it accept standard
> revision ranges using the setup_revisions() function. While this is a
> good thing to make this command more standard and more flexible, it has
> the downside of enabling many revision related options accepted and eaten
> by setup_revisions().
>
> Some of these options might make sense, but others, like those
> generating non-contiguous history, might not. Anyway those we might want
> to allow should probably be tested and perhaps documented a bit, which
> could be done in future work.
>
> For now it is just simpler and safer to just disallow all of them, so
> let's do that.
>
> Other commands, like `git fast-export`, currently allow all these
> revision specific options even though some of them might not make sense,
> as these commands also use setup_revisions() but do not check the
> options that might be passed to this function.
>
> So a way to fix those commands as well as git replay could be to improve
> or refactor the setup_revisions() mechanism to let callers allow and
> disallow options in a relevant way for them. Such improvements are
> outside the scope of this work though.
>
> Pathspecs, which are also accepted and eaten by setup_revisions(), are
> likely to result in disconnected history. That could perhaps be useful,
> but that would need tests and documentation, which can be added in
> future work. So, while at it, let's disallow them too.
As pointed out elsewhere in this mail thread, I consider this patch to do
more harm than good. After switching the command to plumbingmanipulators
it should be possible to just forego all command-line validation and leave
that job to the caller.
Therefore I would love to see this patch dropped.
Ciao,
Johannes
^ 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