* [PATCH v4 5/7] rebase: fix rewritten list for failed pick
From: Phillip Wood via GitGitGadget @ 2023-09-06 15:22 UTC (permalink / raw)
To: git
Cc: Johannes Schindelin, Junio C Hamano, Stefan Haller, Phillip Wood,
Eric Sunshine, Glen Choo, Phillip Wood, Phillip Wood
In-Reply-To: <pull.1492.v4.git.1694013771.gitgitgadget@gmail.com>
From: Phillip Wood <phillip.wood@dunelm.org.uk>
git rebase keeps a list that maps the OID of each commit before it was
rebased to the OID of the equivalent commit after the rebase. This list
is used to drive the "post-rewrite" hook that is called at the end of a
successful rebase. When a rebase stops for the user to resolve merge
conflicts the OID of the commit being picked is written to
".git/rebase-merge/stopped-sha". Then when the rebase is continued that
OID is added to the list of rewritten commits. Unfortunately if a commit
cannot be picked because it would overwrite an untracked file we still
write the "stopped-sha1" file. This means that when the rebase is
continued the commit is added into the list of rewritten commits even
though it has not been picked yet.
Fix this by not calling error_with_patch() for failed commands. The pick
has failed so there is nothing to commit and therefore we do not want to
set up the state files for committing staged changes when the rebase
continues. This change means we no-longer write a patch for the failed
command or display the error message printed by error_with_patch(). As
the command has failed the patch isn't really useful and in any case the
user can inspect the commit associated with the failed command by
inspecting REBASE_HEAD. Unless the user has disabled it we already print
an advice message that is more helpful than the message from
error_with_patch() which the user will still see. Even if the advice is
disabled the user will see the messages from the merge machinery
detailing the problem.
The code to add a failed command back into the todo list is duplicated
between pick_one_commit() and the loop in pick_commits(). Both sites
print advice about the command being rescheduled, decrement the current
item and save the todo list. To avoid duplicating this code
pick_one_commit() is modified to set a flag to indicate that the command
should be rescheduled in the main loop. This simplifies things as only
the remaining copy of the code needs to be modified to set REBASE_HEAD
rather than calling error_with_patch().
Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
sequencer.c | 19 +++++---------
t/t3404-rebase-interactive.sh | 6 +++++
t/t3430-rebase-merges.sh | 4 +--
t/t5407-post-rewrite-hook.sh | 48 +++++++++++++++++++++++++++++++++++
4 files changed, 63 insertions(+), 14 deletions(-)
diff --git a/sequencer.c b/sequencer.c
index b434c5a2570..76932ab7b23 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -4158,6 +4158,7 @@ static int do_merge(struct repository *r,
if (ret < 0) {
error(_("could not even attempt to merge '%.*s'"),
merge_arg_len, arg);
+ unlink(git_path_merge_msg(r));
goto leave_merge;
}
/*
@@ -4648,7 +4649,7 @@ N_("Could not execute the todo command\n"
static int pick_one_commit(struct repository *r,
struct todo_list *todo_list,
struct replay_opts *opts,
- int *check_todo)
+ int *check_todo, int* reschedule)
{
int res;
struct todo_item *item = todo_list->items + todo_list->current;
@@ -4661,12 +4662,8 @@ static int pick_one_commit(struct repository *r,
check_todo);
if (is_rebase_i(opts) && res < 0) {
/* Reschedule */
- advise(_(rescheduled_advice),
- get_item_line_length(todo_list, todo_list->current),
- get_item_line(todo_list, todo_list->current));
- todo_list->current--;
- if (save_todo(todo_list, opts))
- return -1;
+ *reschedule = 1;
+ return -1;
}
if (item->command == TODO_EDIT) {
struct commit *commit = item->commit;
@@ -4766,7 +4763,8 @@ static int pick_commits(struct repository *r,
}
}
if (item->command <= TODO_SQUASH) {
- res = pick_one_commit(r, todo_list, opts, &check_todo);
+ res = pick_one_commit(r, todo_list, opts, &check_todo,
+ &reschedule);
if (!res && item->command == TODO_EDIT)
return 0;
} else if (item->command == TODO_EXEC) {
@@ -4820,10 +4818,7 @@ static int pick_commits(struct repository *r,
if (save_todo(todo_list, opts))
return -1;
if (item->commit)
- return error_with_patch(r,
- item->commit,
- arg, item->arg_len,
- opts, res, 0);
+ write_rebase_head(&item->commit->object.oid);
} else if (is_rebase_i(opts) && check_todo && !res &&
reread_todo_if_changed(r, todo_list, opts)) {
return -1;
diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index ff0afad63e2..6d3788c588b 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -1287,7 +1287,9 @@ test_expect_success 'rebase -i commits that overwrite untracked files (pick)' '
>file6 &&
test_must_fail git rebase --continue &&
test_cmp_rev HEAD F &&
+ test_cmp_rev REBASE_HEAD I &&
rm file6 &&
+ test_path_is_missing .git/rebase-merge/patch &&
git rebase --continue &&
test_cmp_rev HEAD I
'
@@ -1305,7 +1307,9 @@ test_expect_success 'rebase -i commits that overwrite untracked files (squash)'
>file6 &&
test_must_fail git rebase --continue &&
test_cmp_rev HEAD F &&
+ test_cmp_rev REBASE_HEAD I &&
rm file6 &&
+ test_path_is_missing .git/rebase-merge/patch &&
git rebase --continue &&
test $(git cat-file commit HEAD | sed -ne \$p) = I &&
git reset --hard original-branch2
@@ -1323,7 +1327,9 @@ test_expect_success 'rebase -i commits that overwrite untracked files (no ff)' '
>file6 &&
test_must_fail git rebase --continue &&
test $(git cat-file commit HEAD | sed -ne \$p) = F &&
+ test_cmp_rev REBASE_HEAD I &&
rm file6 &&
+ test_path_is_missing .git/rebase-merge/patch &&
git rebase --continue &&
test $(git cat-file commit HEAD | sed -ne \$p) = I
'
diff --git a/t/t3430-rebase-merges.sh b/t/t3430-rebase-merges.sh
index 96ae0edf1e1..4938ebb1c17 100755
--- a/t/t3430-rebase-merges.sh
+++ b/t/t3430-rebase-merges.sh
@@ -165,12 +165,12 @@ test_expect_success 'failed `merge -C` writes patch (may be rescheduled, too)' '
test_config sequence.editor \""$PWD"/replace-editor.sh\" &&
test_tick &&
test_must_fail git rebase -ir HEAD &&
+ test_cmp_rev REBASE_HEAD H^0 &&
grep "^merge -C .* G$" .git/rebase-merge/done &&
grep "^merge -C .* G$" .git/rebase-merge/git-rebase-todo &&
- test_path_is_file .git/rebase-merge/patch &&
+ test_path_is_missing .git/rebase-merge/patch &&
: fail because of merge conflict &&
- rm G.t .git/rebase-merge/patch &&
git reset --hard conflicting-G &&
test_must_fail git rebase --continue &&
! grep "^merge -C .* G$" .git/rebase-merge/git-rebase-todo &&
diff --git a/t/t5407-post-rewrite-hook.sh b/t/t5407-post-rewrite-hook.sh
index 5f3ff051ca2..ad7f8c6f002 100755
--- a/t/t5407-post-rewrite-hook.sh
+++ b/t/t5407-post-rewrite-hook.sh
@@ -17,6 +17,12 @@ test_expect_success 'setup' '
git checkout A^0 &&
test_commit E bar E &&
test_commit F foo F &&
+ git checkout B &&
+ git merge E &&
+ git tag merge-E &&
+ test_commit G G &&
+ test_commit H H &&
+ test_commit I I &&
git checkout main &&
test_hook --setup post-rewrite <<-EOF
@@ -173,6 +179,48 @@ test_fail_interactive_rebase () {
)
}
+test_expect_success 'git rebase with failed pick' '
+ clear_hook_input &&
+ cat >todo <<-\EOF &&
+ exec >bar
+ merge -C merge-E E
+ exec >G
+ pick G
+ exec >H 2>I
+ pick H
+ fixup I
+ EOF
+
+ (
+ set_replace_editor todo &&
+ test_must_fail git rebase -i D D 2>err
+ ) &&
+ grep "would be overwritten" err &&
+ rm bar &&
+
+ test_must_fail git rebase --continue 2>err &&
+ grep "would be overwritten" err &&
+ rm G &&
+
+ test_must_fail git rebase --continue 2>err &&
+ grep "would be overwritten" err &&
+ rm H &&
+
+ test_must_fail git rebase --continue 2>err &&
+ grep "would be overwritten" err &&
+ rm I &&
+
+ git rebase --continue &&
+ echo rebase >expected.args &&
+ cat >expected.data <<-EOF &&
+ $(git rev-parse merge-E) $(git rev-parse HEAD~2)
+ $(git rev-parse G) $(git rev-parse HEAD~1)
+ $(git rev-parse H) $(git rev-parse HEAD)
+ $(git rev-parse I) $(git rev-parse HEAD)
+ EOF
+ verify_hook_input
+'
+
test_expect_success 'git rebase -i (unchanged)' '
git reset --hard D &&
clear_hook_input &&
--
gitgitgadget
^ permalink raw reply related
* [PATCH v4 4/7] sequencer: factor out part of pick_commits()
From: Phillip Wood via GitGitGadget @ 2023-09-06 15:22 UTC (permalink / raw)
To: git
Cc: Johannes Schindelin, Junio C Hamano, Stefan Haller, Phillip Wood,
Eric Sunshine, Glen Choo, Phillip Wood, Phillip Wood
In-Reply-To: <pull.1492.v4.git.1694013771.gitgitgadget@gmail.com>
From: Phillip Wood <phillip.wood@dunelm.org.uk>
This simplifies the next commit. If a pick fails we now return the error
at the end of the loop body rather than returning early, a successful
"edit" command continues to return early. There are three things to
check to ensure that removing the early return for an error does not
change the behavior of the code:
(1) We could enter the block guarded by "if (reschedule)". This block
is not entered because "reschedlue" is always zero when picking a
commit.
(2) We could enter the block guarded by
"else if (is_rebase_i(opts) && check_todo && !res)". This block is
not entered when returning an error because "res" is non-zero in
that case.
(3) todo_list->current could be incremented before returning. That is
avoided by moving the increment which is of course a potential
change in behavior itself. The move is safe because none of the
callers look at todo_list after this function returns. Moving the
increment makes it clear we only want to advance the current item
if the command was successful.
Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
sequencer.c | 132 ++++++++++++++++++++++++++++------------------------
1 file changed, 71 insertions(+), 61 deletions(-)
diff --git a/sequencer.c b/sequencer.c
index 83be8bf2b6d..b434c5a2570 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -4645,6 +4645,72 @@ N_("Could not execute the todo command\n"
" git rebase --edit-todo\n"
" git rebase --continue\n");
+static int pick_one_commit(struct repository *r,
+ struct todo_list *todo_list,
+ struct replay_opts *opts,
+ int *check_todo)
+{
+ int res;
+ struct todo_item *item = todo_list->items + todo_list->current;
+ const char *arg = todo_item_get_arg(todo_list, item);
+ if (is_rebase_i(opts))
+ opts->reflog_message = reflog_message(
+ opts, command_to_string(item->command), NULL);
+
+ res = do_pick_commit(r, item, opts, is_final_fixup(todo_list),
+ check_todo);
+ if (is_rebase_i(opts) && res < 0) {
+ /* Reschedule */
+ advise(_(rescheduled_advice),
+ get_item_line_length(todo_list, todo_list->current),
+ get_item_line(todo_list, todo_list->current));
+ todo_list->current--;
+ if (save_todo(todo_list, opts))
+ return -1;
+ }
+ if (item->command == TODO_EDIT) {
+ struct commit *commit = item->commit;
+ if (!res) {
+ if (!opts->verbose)
+ term_clear_line();
+ fprintf(stderr, _("Stopped at %s... %.*s\n"),
+ short_commit_name(commit), item->arg_len, arg);
+ }
+ return error_with_patch(r, commit,
+ arg, item->arg_len, opts, res, !res);
+ }
+ if (is_rebase_i(opts) && !res)
+ record_in_rewritten(&item->commit->object.oid,
+ peek_command(todo_list, 1));
+ if (res && is_fixup(item->command)) {
+ if (res == 1)
+ intend_to_amend();
+ return error_failed_squash(r, item->commit, opts,
+ item->arg_len, arg);
+ } else if (res && is_rebase_i(opts) && item->commit) {
+ int to_amend = 0;
+ struct object_id oid;
+
+ /*
+ * If we are rewording and have either
+ * fast-forwarded already, or are about to
+ * create a new root commit, we want to amend,
+ * otherwise we do not.
+ */
+ if (item->command == TODO_REWORD &&
+ !repo_get_oid(r, "HEAD", &oid) &&
+ (oideq(&item->commit->object.oid, &oid) ||
+ (opts->have_squash_onto &&
+ oideq(&opts->squash_onto, &oid))))
+ to_amend = 1;
+
+ return res | error_with_patch(r, item->commit,
+ arg, item->arg_len, opts,
+ res, to_amend);
+ }
+ return res;
+}
+
static int pick_commits(struct repository *r,
struct todo_list *todo_list,
struct replay_opts *opts)
@@ -4700,66 +4766,9 @@ static int pick_commits(struct repository *r,
}
}
if (item->command <= TODO_SQUASH) {
- if (is_rebase_i(opts))
- opts->reflog_message = reflog_message(opts,
- command_to_string(item->command), NULL);
-
- res = do_pick_commit(r, item, opts,
- is_final_fixup(todo_list),
- &check_todo);
- if (is_rebase_i(opts) && res < 0) {
- /* Reschedule */
- advise(_(rescheduled_advice),
- get_item_line_length(todo_list,
- todo_list->current),
- get_item_line(todo_list,
- todo_list->current));
- todo_list->current--;
- if (save_todo(todo_list, opts))
- return -1;
- }
- if (item->command == TODO_EDIT) {
- struct commit *commit = item->commit;
- if (!res) {
- if (!opts->verbose)
- term_clear_line();
- fprintf(stderr,
- _("Stopped at %s... %.*s\n"),
- short_commit_name(commit),
- item->arg_len, arg);
- }
- return error_with_patch(r, commit,
- arg, item->arg_len, opts, res, !res);
- }
- if (is_rebase_i(opts) && !res)
- record_in_rewritten(&item->commit->object.oid,
- peek_command(todo_list, 1));
- if (res && is_fixup(item->command)) {
- if (res == 1)
- intend_to_amend();
- return error_failed_squash(r, item->commit, opts,
- item->arg_len, arg);
- } else if (res && is_rebase_i(opts) && item->commit) {
- int to_amend = 0;
- struct object_id oid;
-
- /*
- * If we are rewording and have either
- * fast-forwarded already, or are about to
- * create a new root commit, we want to amend,
- * otherwise we do not.
- */
- if (item->command == TODO_REWORD &&
- !repo_get_oid(r, "HEAD", &oid) &&
- (oideq(&item->commit->object.oid, &oid) ||
- (opts->have_squash_onto &&
- oideq(&opts->squash_onto, &oid))))
- to_amend = 1;
-
- return res | error_with_patch(r, item->commit,
- arg, item->arg_len, opts,
- res, to_amend);
- }
+ res = pick_one_commit(r, todo_list, opts, &check_todo);
+ if (!res && item->command == TODO_EDIT)
+ return 0;
} else if (item->command == TODO_EXEC) {
char *end_of_arg = (char *)(arg + item->arg_len);
int saved = *end_of_arg;
@@ -4820,9 +4829,10 @@ static int pick_commits(struct repository *r,
return -1;
}
- todo_list->current++;
if (res)
return res;
+
+ todo_list->current++;
}
if (is_rebase_i(opts)) {
--
gitgitgadget
^ permalink raw reply related
* [PATCH v4 1/7] rebase -i: move unlink() calls
From: Phillip Wood via GitGitGadget @ 2023-09-06 15:22 UTC (permalink / raw)
To: git
Cc: Johannes Schindelin, Junio C Hamano, Stefan Haller, Phillip Wood,
Eric Sunshine, Glen Choo, Phillip Wood, Phillip Wood
In-Reply-To: <pull.1492.v4.git.1694013771.gitgitgadget@gmail.com>
From: Phillip Wood <phillip.wood@dunelm.org.uk>
At the start of each iteration the loop that picks commits removes the
state files from the previous pick. However some of these files are only
written if there are conflicts in which case we exit the loop before the
end of the loop body. Therefore they only need to be removed when the
rebase continues, not at the start of each iteration.
Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
sequencer.c | 7 ++++---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/sequencer.c b/sequencer.c
index cc9821ece2c..de66bda9d5b 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -4656,6 +4656,10 @@ static int pick_commits(struct repository *r,
if (read_and_refresh_cache(r, opts))
return -1;
+ unlink(rebase_path_message());
+ unlink(rebase_path_stopped_sha());
+ unlink(rebase_path_amend());
+
while (todo_list->current < todo_list->nr) {
struct todo_item *item = todo_list->items + todo_list->current;
const char *arg = todo_item_get_arg(todo_list, item);
@@ -4679,10 +4683,7 @@ static int pick_commits(struct repository *r,
todo_list->total_nr,
opts->verbose ? "\n" : "\r");
}
- unlink(rebase_path_message());
unlink(rebase_path_author_script());
- unlink(rebase_path_stopped_sha());
- unlink(rebase_path_amend());
unlink(git_path_merge_head(r));
unlink(git_path_auto_merge(r));
delete_ref(NULL, "REBASE_HEAD", NULL, REF_NO_DEREF);
--
gitgitgadget
^ permalink raw reply related
* [PATCH v4 2/7] rebase -i: remove patch file after conflict resolution
From: Phillip Wood via GitGitGadget @ 2023-09-06 15:22 UTC (permalink / raw)
To: git
Cc: Johannes Schindelin, Junio C Hamano, Stefan Haller, Phillip Wood,
Eric Sunshine, Glen Choo, Phillip Wood, Phillip Wood
In-Reply-To: <pull.1492.v4.git.1694013771.gitgitgadget@gmail.com>
From: Phillip Wood <phillip.wood@dunelm.org.uk>
When a rebase stops for the user to resolve conflicts it writes a patch
for the conflicting commit to .git/rebase-merge/patch. This file has
been written since the introduction of "git-rebase-interactive.sh" in
1b1dce4bae7 (Teach rebase an interactive mode, 2007-06-25). I assume the
idea was to enable the user inspect the conflicting commit in the same
way as they could for the patch based rebase. This file should be
deleted when the rebase continues as if the rebase stops for a failed
"exec" command or a "break" command it is confusing to the user if there
is a stale patch lying around from an unrelated command. As the path is
now used in two different places rebase_path_patch() is added and used
to obtain the path for the patch.
To construct the path write_patch() previously used get_dir() which
returns different paths depending on whether we're rebasing or
cherry-picking/reverting. As this function is only called when
rebasing it is safe to use a hard coded string for the directory
instead. An assertion is added to make sure we don't starting calling
this function when cherry-picking in the future.
Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
sequencer.c | 16 ++++++++++++----
t/t3418-rebase-continue.sh | 18 ++++++++++++++++++
2 files changed, 30 insertions(+), 4 deletions(-)
diff --git a/sequencer.c b/sequencer.c
index de66bda9d5b..c1911b0fc14 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -138,6 +138,11 @@ static GIT_PATH_FUNC(rebase_path_amend, "rebase-merge/amend")
* the commit object name of the corresponding patch.
*/
static GIT_PATH_FUNC(rebase_path_stopped_sha, "rebase-merge/stopped-sha")
+/*
+ * When we stop for the user to resolve conflicts this file contains
+ * the patch of the commit that is being picked.
+ */
+static GIT_PATH_FUNC(rebase_path_patch, "rebase-merge/patch")
/*
* For the post-rewrite hook, we make a list of rewritten commits and
* their new sha1s. The rewritten-pending list keeps the sha1s of
@@ -3502,12 +3507,14 @@ static int make_patch(struct repository *r,
char hex[GIT_MAX_HEXSZ + 1];
int res = 0;
+ if (!is_rebase_i(opts))
+ BUG("make_patch should only be called when rebasing");
+
oid_to_hex_r(hex, &commit->object.oid);
if (write_message(hex, strlen(hex), rebase_path_stopped_sha(), 1) < 0)
return -1;
res |= write_rebase_head(&commit->object.oid);
- strbuf_addf(&buf, "%s/patch", get_dir(opts));
memset(&log_tree_opt, 0, sizeof(log_tree_opt));
repo_init_revisions(r, &log_tree_opt, NULL);
log_tree_opt.abbrev = 0;
@@ -3515,15 +3522,15 @@ static int make_patch(struct repository *r,
log_tree_opt.diffopt.output_format = DIFF_FORMAT_PATCH;
log_tree_opt.disable_stdin = 1;
log_tree_opt.no_commit_id = 1;
- log_tree_opt.diffopt.file = fopen(buf.buf, "w");
+ log_tree_opt.diffopt.file = fopen(rebase_path_patch(), "w");
log_tree_opt.diffopt.use_color = GIT_COLOR_NEVER;
if (!log_tree_opt.diffopt.file)
- res |= error_errno(_("could not open '%s'"), buf.buf);
+ res |= error_errno(_("could not open '%s'"),
+ rebase_path_patch());
else {
res |= log_tree_commit(&log_tree_opt, commit);
fclose(log_tree_opt.diffopt.file);
}
- strbuf_reset(&buf);
strbuf_addf(&buf, "%s/message", get_dir(opts));
if (!file_exists(buf.buf)) {
@@ -4659,6 +4666,7 @@ static int pick_commits(struct repository *r,
unlink(rebase_path_message());
unlink(rebase_path_stopped_sha());
unlink(rebase_path_amend());
+ unlink(rebase_path_patch());
while (todo_list->current < todo_list->nr) {
struct todo_item *item = todo_list->items + todo_list->current;
diff --git a/t/t3418-rebase-continue.sh b/t/t3418-rebase-continue.sh
index 2d0789e554b..261e7cd754c 100755
--- a/t/t3418-rebase-continue.sh
+++ b/t/t3418-rebase-continue.sh
@@ -244,6 +244,24 @@ test_expect_success 'the todo command "break" works' '
test_path_is_file execed
'
+test_expect_success 'patch file is removed before break command' '
+ test_when_finished "git rebase --abort" &&
+ cat >todo <<-\EOF &&
+ pick commit-new-file-F2-on-topic-branch
+ break
+ EOF
+
+ (
+ set_replace_editor todo &&
+ test_must_fail git rebase -i --onto commit-new-file-F2 HEAD
+ ) &&
+ test_path_is_file .git/rebase-merge/patch &&
+ echo 22>F2 &&
+ git add F2 &&
+ git rebase --continue &&
+ test_path_is_missing .git/rebase-merge/patch
+'
+
test_expect_success '--reschedule-failed-exec' '
test_when_finished "git rebase --abort" &&
test_must_fail git rebase -x false --reschedule-failed-exec HEAD^ &&
--
gitgitgadget
^ permalink raw reply related
* [PATCH v4 3/7] sequencer: use rebase_path_message()
From: Phillip Wood via GitGitGadget @ 2023-09-06 15:22 UTC (permalink / raw)
To: git
Cc: Johannes Schindelin, Junio C Hamano, Stefan Haller, Phillip Wood,
Eric Sunshine, Glen Choo, Phillip Wood, Phillip Wood
In-Reply-To: <pull.1492.v4.git.1694013771.gitgitgadget@gmail.com>
From: Phillip Wood <phillip.wood@dunelm.org.uk>
Rather than constructing the path in a struct strbuf use the ready
made function to get the path name instead. This was the last
remaining use of the strbuf so remove it as well.
As with the previous patch we now use a hard coded string rather than
git_dir() when constructing the path. This is safe for the same
reason (make_patch() is only called when rebasing) and is protected by
the assertion added in the previous patch.
Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
sequencer.c | 7 ++-----
1 file changed, 2 insertions(+), 5 deletions(-)
diff --git a/sequencer.c b/sequencer.c
index c1911b0fc14..83be8bf2b6d 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -3501,7 +3501,6 @@ static int make_patch(struct repository *r,
struct commit *commit,
struct replay_opts *opts)
{
- struct strbuf buf = STRBUF_INIT;
struct rev_info log_tree_opt;
const char *subject;
char hex[GIT_MAX_HEXSZ + 1];
@@ -3532,18 +3531,16 @@ static int make_patch(struct repository *r,
fclose(log_tree_opt.diffopt.file);
}
- strbuf_addf(&buf, "%s/message", get_dir(opts));
- if (!file_exists(buf.buf)) {
+ if (!file_exists(rebase_path_message())) {
const char *encoding = get_commit_output_encoding();
const char *commit_buffer = repo_logmsg_reencode(r,
commit, NULL,
encoding);
find_commit_subject(commit_buffer, &subject);
- res |= write_message(subject, strlen(subject), buf.buf, 1);
+ res |= write_message(subject, strlen(subject), rebase_path_message(), 1);
repo_unuse_commit_buffer(r, commit,
commit_buffer);
}
- strbuf_release(&buf);
release_revisions(&log_tree_opt);
return res;
--
gitgitgadget
^ permalink raw reply related
* [PATCH v4 0/7] rebase -i: impove handling of failed commands
From: Phillip Wood via GitGitGadget @ 2023-09-06 15:22 UTC (permalink / raw)
To: git
Cc: Johannes Schindelin, Junio C Hamano, Stefan Haller, Phillip Wood,
Eric Sunshine, Glen Choo, Phillip Wood
In-Reply-To: <pull.1492.v3.git.1690903412.gitgitgadget@gmail.com>
This series fixes several bugs in the way we handle a commit cannot be
picked because it would overwrite an untracked file.
* after a failed pick "git rebase --continue" will happily commit any
staged changes even though no commit was picked.
* the commit of the failed pick is recorded as rewritten even though no
commit was picked.
* the "done" file used by "git status" to show the recently executed
commands contains an incorrect entry.
Thanks to Eric, Glen and Junio for their comments on v2. Here are the
changes since v2:
Patch 1 - Reworded the commit message.
Patch 2 - Reworded the commit message, added a test and fixed error message
pointed out by Glen.
Patch 3 - New cleanup.
Patch 4 - Reworded the commit message, now only increments
todo_list->current if there is no error.
Patch 5 - Swapped with next patch. Reworded the commit message, stopped
testing implementation (suggested by Glen). Expanded post-rewrite hook test.
Patch 6 - Reworded the commit message, now uses the message file rather than
the author script to check if "rebase --continue" should commit staged
changes. Junio suggested using a separate file for this but I think that
would end up being more involved as we'd need to be careful about creating
and removing it.
Patch 7 - Reworded the commit message.
Thanks for the comments on V1, this series has now grown somewhat.
Previously I was worried that refactoring would change the behavior, but
having thought about it the current behavior is wrong and should be changed.
Changes since V1:
Rebased onto master to avoid a conflict with
ab/remove-implicit-use-of-the-repository
* Patches 1-3 are new preparatory changes
* Patches 4 & 5 are new and fix the first two issues listed above.
* Patch 6 is the old patch 1 which has been rebased and the commit message
reworded. It fixes the last issues listed above.
Phillip Wood (7):
rebase -i: move unlink() calls
rebase -i: remove patch file after conflict resolution
sequencer: use rebase_path_message()
sequencer: factor out part of pick_commits()
rebase: fix rewritten list for failed pick
rebase --continue: refuse to commit after failed command
rebase -i: fix adding failed command to the todo list
sequencer.c | 182 ++++++++++++++++++----------------
t/t3404-rebase-interactive.sh | 53 +++++++---
t/t3418-rebase-continue.sh | 18 ++++
t/t3430-rebase-merges.sh | 30 ++++--
t/t5407-post-rewrite-hook.sh | 48 +++++++++
5 files changed, 228 insertions(+), 103 deletions(-)
base-commit: a80be152923a46f04a06bade7bcc72870e46ca09
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1492%2Fphillipwood%2Frebase-dont-write-done-when-rescheduling-v4
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1492/phillipwood/rebase-dont-write-done-when-rescheduling-v4
Pull-Request: https://github.com/gitgitgadget/git/pull/1492
Range-diff vs v3:
1: 1ab1ad2ef07 ! 1: ae4f873b3d0 rebase -i: move unlink() calls
@@ Metadata
## Commit message ##
rebase -i: move unlink() calls
- At the start of each iteration the loop that picks commits removes
- state files from the previous pick. However some of these are only
- written if there are conflicts and so we break out of the loop after
- writing them. Therefore they only need to be removed when the rebase
- continues, not in each iteration.
+ At the start of each iteration the loop that picks commits removes the
+ state files from the previous pick. However some of these files are only
+ written if there are conflicts in which case we exit the loop before the
+ end of the loop body. Therefore they only need to be removed when the
+ rebase continues, not at the start of each iteration.
Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
2: e2a758eb4a5 ! 2: f540ed1d607 rebase -i: remove patch file after conflict resolution
@@ Commit message
now used in two different places rebase_path_patch() is added and used
to obtain the path for the patch.
+ To construct the path write_patch() previously used get_dir() which
+ returns different paths depending on whether we're rebasing or
+ cherry-picking/reverting. As this function is only called when
+ rebasing it is safe to use a hard coded string for the directory
+ instead. An assertion is added to make sure we don't starting calling
+ this function when cherry-picking in the future.
+
Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
## sequencer.c ##
@@ sequencer.c: static GIT_PATH_FUNC(rebase_path_amend, "rebase-merge/amend")
* For the post-rewrite hook, we make a list of rewritten commits and
* their new sha1s. The rewritten-pending list keeps the sha1s of
@@ sequencer.c: static int make_patch(struct repository *r,
+ char hex[GIT_MAX_HEXSZ + 1];
+ int res = 0;
+
++ if (!is_rebase_i(opts))
++ BUG("make_patch should only be called when rebasing");
++
+ oid_to_hex_r(hex, &commit->object.oid);
+ if (write_message(hex, strlen(hex), rebase_path_stopped_sha(), 1) < 0)
return -1;
res |= write_rebase_head(&commit->object.oid);
3: 8f6c0e40567 ! 3: 818bdaf772d sequencer: use rebase_path_message()
@@ Commit message
made function to get the path name instead. This was the last
remaining use of the strbuf so remove it as well.
+ As with the previous patch we now use a hard coded string rather than
+ git_dir() when constructing the path. This is safe for the same
+ reason (make_patch() is only called when rebasing) and is protected by
+ the assertion added in the previous patch.
+
Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
## sequencer.c ##
4: a1fad70f4b9 = 4: bd67765a864 sequencer: factor out part of pick_commits()
5: df401945866 ! 5: f6f330f7063 rebase: fix rewritten list for failed pick
@@ Commit message
disabled the user will see the messages from the merge machinery
detailing the problem.
- To simplify writing REBASE_HEAD in this case pick_one_commit() is
- modified to avoid duplicating the code that adds the failed command
- back into the todo list.
+ The code to add a failed command back into the todo list is duplicated
+ between pick_one_commit() and the loop in pick_commits(). Both sites
+ print advice about the command being rescheduled, decrement the current
+ item and save the todo list. To avoid duplicating this code
+ pick_one_commit() is modified to set a flag to indicate that the command
+ should be rescheduled in the main loop. This simplifies things as only
+ the remaining copy of the code needs to be modified to set REBASE_HEAD
+ rather than calling error_with_patch().
Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
6: 2ed7cbe5fff = 6: 0ca5fccca17 rebase --continue: refuse to commit after failed command
7: bbe0afde512 = 7: 8d5f6d51e19 rebase -i: fix adding failed command to the todo list
--
gitgitgadget
^ permalink raw reply
* [PATCH v2 1/1] doc/diff-options: fix link to generating patch section
From: Sergey Organov @ 2023-09-06 7:15 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, John Cai
In-Reply-To: <xmqq34zsqlr3.fsf@gitster.g>
When formatted as man-page, the section title is rendered
"GENERATING PATCH TEXT WITH -P" whereas reference still reads
"Generating patch text with -p", that is inconsistent and makes
searching harder than it needs to be.
Fix this by getting rid of custom reference text.
Also, documentation for every command that describes `-p` option by
including the "diff-options.txt" file does include the
"diff-generate-patch.txt" file as well (as it should), so the internal
link is in fact useful for any of them.
Fix this by getting rid of conditionals around the reference.
Fixes: ebdc46c242 (docs: link generating patch sections)
Signed-off-by: Sergey Organov <sorganov@gmail.com>
---
Documentation/diff-options.txt | 8 +-------
1 file changed, 1 insertion(+), 7 deletions(-)
diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt
index 9f33f887711d..c07488b123c6 100644
--- a/Documentation/diff-options.txt
+++ b/Documentation/diff-options.txt
@@ -22,13 +22,7 @@ ifndef::git-format-patch[]
-p::
-u::
--patch::
- Generate patch (see section titled
-ifdef::git-log[]
-<<generate_patch_text_with_p, "Generating patch text with -p">>).
-endif::git-log[]
-ifndef::git-log[]
-"Generating patch text with -p").
-endif::git-log[]
+ Generate patch (see <<generate_patch_text_with_p>>).
ifdef::git-diff[]
This is the default.
endif::git-diff[]
--
2.25.1
^ permalink raw reply related
* Re: [PATCH 1/1] doc/diff-options: fix link to generating patch section
From: Sergey Organov @ 2023-09-06 6:47 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, John Cai
In-Reply-To: <xmqq34zsqlr3.fsf@gitster.g>
Junio C Hamano <gitster@pobox.com> writes:
> Sergey Organov <sorganov@gmail.com> writes:
>
>> First, there is no need for conditional referencing, as all the files
>> that include "diff-options.txt" eventually include
>> "diff-generate-patch.txt" as well.
>
> Except for git-format-patch.txt which includes the former but not
> the latter. But this is inside ifndef::git-format-patch[], so the
> above description being a bit imprecise does not cause any actual
> damage.
>
> Documentation for all commands that want to describe the `-p`
> option by including the "diff-options.txt" file also include the
> "diff-generate-patch.txt" file, so an internal link would work
> for all of them.
>
> or something like that, perhaps.
In fact I just realized that removing conditionals in fact *fixes* those
documents by providing proper link in them as well, so I'll think of
better description taking into account your observation as well, and
then will re-roll.
Thanks,
-- Sergey Organov
^ permalink raw reply
* Re: [PATCH 1/1] doc/diff-options: fix link to generating patch section
From: Sergey Organov @ 2023-09-06 6:40 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, John Cai
In-Reply-To: <xmqq34zsqlr3.fsf@gitster.g>
Junio C Hamano <gitster@pobox.com> writes:
> Sergey Organov <sorganov@gmail.com> writes:
>
>> First, there is no need for conditional referencing, as all the files
>> that include "diff-options.txt" eventually include
>> "diff-generate-patch.txt" as well.
>
> Except for git-format-patch.txt which includes the former but not
> the latter. But this is inside ifndef::git-format-patch[], so the
> above description being a bit imprecise does not cause any actual
> damage.
Ah, nice catch!
>
> Documentation for all commands that want to describe the `-p`
> option by including the "diff-options.txt" file also include the
> "diff-generate-patch.txt" file, so an internal link would work
> for all of them.
Sounds fine. Would you re-phrase it yourself, or should I rather
re-roll?
Thanks,
-- Sergey Organov
^ permalink raw reply
* Re: [PATCH] show doc: redirect user to git log manual instead of git diff-tree
From: Han Young @ 2023-09-06 6:09 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <xmqq7cp4p5gd.fsf@gitster.g>
> Strictly speaking, "options to control how the changes are shown"
> are options that are meant for "diff" command (e.g. "--stat", "-w"),
> but "log" understands some of the "diff" command options, the
> updated text is *not* incorrect.
On a closer look, the manual page of git show does lists
all the "diff" options by including diff-options.txt.
The options omitted are revision parsing related.
Perhaps we remove the line
> The command takes options applicable to the git diff-tree command to
> control how the changes the commit introduces are shown.
And rephrase the line
> This manual page describes only the most frequently used options.
to
> This manual page describes only the most frequently used options.
> Some options that `git rev-list` command understands can be used to
> control how commits are shown.
^ permalink raw reply
* [PATCH] [diff-lib] Fix check_removed when fsmonitor is on
From: Josip Sokcevic @ 2023-09-06 6:02 UTC (permalink / raw)
To: jonathantanmy; +Cc: git, git, Josip Sokcevic
In-Reply-To: <20230829005606.136615-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`.
---
This patch is using jonathantanmy@ test case, which now passes. It's
possible there are similar edge cases with fsmonitor, so I'll do more
extensive e2e testing tomorrow.
diff-lib.c | 2 +-
t/t7527-builtin-fsmonitor.sh | 6 ++++++
2 files changed, 7 insertions(+), 1 deletion(-)
diff --git a/diff-lib.c b/diff-lib.c
index d8aa777a73..c67aa5ff89 100644
--- a/diff-lib.c
+++ b/diff-lib.c
@@ -46,7 +46,7 @@ static int check_removed(const struct index_state *istate, const struct cache_en
}
if (has_symlink_leading_path(ce->name, ce_namelen(ce)))
return 1;
- if (S_ISDIR(st->st_mode)) {
+ if (!(ce->ce_flags & CE_FSMONITOR_VALID) && S_ISDIR(st->st_mode)) {
struct object_id sub;
/*
diff --git a/t/t7527-builtin-fsmonitor.sh b/t/t7527-builtin-fsmonitor.sh
index 0c241d6c14..894a80fbe8 100755
--- a/t/t7527-builtin-fsmonitor.sh
+++ b/t/t7527-builtin-fsmonitor.sh
@@ -824,6 +824,10 @@ test_expect_success 'submodule always visited' '
create_super super &&
create_sub sub &&
+ 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 submodule add ../sub ./dir_1/dir_2/sub &&
git -C super commit -m "add sub" &&
@@ -837,6 +841,8 @@ test_expect_success 'submodule always visited' '
# some dirt in the submodule and confirm matching output.
# Completely clean status.
+ echo Now running for clean status &&
+
my_match_and_clean &&
# .M S..U
--
2.42.0.283.g2d96d420d3-goog
^ permalink raw reply related
* Re: [PATCH v2] cherry-pick: refuse cherry-pick sequence if index is dirty
From: Tao Klerks @ 2023-09-06 5:02 UTC (permalink / raw)
To: phillip.wood; +Cc: Tao Klerks via GitGitGadget, git, Junio C Hamano
In-Reply-To: <999f12b2-38d6-f446-e763-4985116ad37d@gmail.com>
On Tue, May 30, 2023 at 4:16 PM Phillip Wood <phillip.wood123@gmail.com> wrote:
>
> Hi Tao
>
> On 28/05/2023 10:08, Tao Klerks via GitGitGadget wrote:
> > From: Tao Klerks <tao@klerks.biz>
> >
> <SNIP>
>
> I found the changes up to this point a bit confusing. Maybe I've missed
> something but I don't think they are really related to fixing the bug
> described in the commit message. As such they're a distraction from the
> "real" fix.
>
Understood, thanks - *if* I kept them, they should be in a separate
"prep refactor" commit.
The reason I did all this was just that I needed to build a new
message that displayed the correct cherry-pick action name - something
that was, in the existing code, done by repeating the entire advice
message. I didn't want to do the same, and if I was going add what I
needed to construct the message more dynamically I figured I should
update the existing repetition-based approach.
>
> > + requested_action_name = cherry_pick_action_name(requested_action);
>
> We already have the function action_name() so I don't think we need to
> add cherry_pick_action_name().
The reason I had added a new one was that action_name() also supported
"REPLAY_INTERACTIVE_REBASE", which should not be an option in the
codepath that I was refactoring. I wanted to retain the existing
"defensiveness", but that clearly got in the way of both brevity and
clarity.
> Also the name of the new function is
> confusing as it may return "revert".
Yeah, the name was supposed to reflect the context ("cherry-pick logic
which also covers revert, as opposed to rebase which also uses
sequencer but is a substantially separate flow"), rather than the
output value.
>
> > + if (require_clean_index(r, requested_action_name,
> > + _("Please commit or stash them."), 1, 1))
>
> How does this interact with "--no-commit"? I think the check that you
> refer to in the commit message is in do_pick_commit() where we have
>
> if (opts->no_commit) {
> /*
> * We do not intend to commit immediately. We just want to
> * merge the differences in, so let's compute the tree
> * that represents the "current" state for the merge machinery
> * to work on.
> */
> if (write_index_as_tree(&head, r->index, r->index_file, 0, NULL))
> return error(_("your index file is unmerged."));
> } else {
> unborn = repo_get_oid(r, "HEAD", &head);
> /* Do we want to generate a root commit? */
> if (is_pick_or_similar(command) && opts->have_squash_onto &&
> oideq(&head, &opts->squash_onto)) {
> if (is_fixup(command))
> return error(_("cannot fixup root commit"));
> flags |= CREATE_ROOT_COMMIT;
> unborn = 1;
> } else if (unborn)
> oidcpy(&head, the_hash_algo->empty_tree);
> if (index_differs_from(r, unborn ? empty_tree_oid_hex() : "HEAD",
> NULL, 0))
> return error_dirty_index(r, opts);
> }
>
> I think it would be simpler to reuse the existing check by extracting
> the "else" clause above into a separate function in sequencer.c and call
> it here guarded by "if (!opts->no_commit)" as well as in that "else"
> clause in do_pick_commit()
That sounds very plausible.
I will (very belatedly) have a go, and submit another version sometime soon.
Thanks so much for taking the time to review, and my apologies for the
months-later context revival!
^ permalink raw reply
* Re: Plumbing for mapping from a remote tracking ref to the remote ref?
From: Tao Klerks @ 2023-09-06 4:21 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Johannes Schindelin, git
In-Reply-To: <xmqqpm2wqn6h.fsf@gitster.g>
On Wed, Sep 6, 2023 at 12:18 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> Tao Klerks <tao@klerks.biz> writes:
>
> > ...
> > Would something like the following be mutually agreeable?
> >
> > $ git remote origin map-ref
> > refs/remotes/my-favorite-remotes/origin/someref
> > refs/heads/someref
>
> With strainge line wrapping, I cannot quite tell what is the input
> and what is the output, but if you meant that the part up to the
> long-ish refname in the refs/remotes is the command line, and map-ref
> is the new subcommand name in the "git remote" command, i.e.
>
> $ git remote map-ref origin refs/remotes/my-favorite-remotes/origin/someref
>
> is the input, to which the output
>
> refs/heads/someref
>
> is given,
My apologies: in addition to automatic line wrapping, I also got the
arg order wrong.
Yes, this is what I meant.
> I am not sure what its value is. First of all, the user
> is giving a ref in a hierarchy that is usually used for the remote
> whose name is "my-favorite-remotes". What made this user _know_
> that the remote reference belongs to "origin"?
Understanding that it's dangerous to make assumptions about what's
typical, I am positing that the user typically knows what remote
they're working with / looking for stuff in. I would guess that the
set of repos, in the world, that have multiple remotes with different
ref path structures, mapped onto the same remote tracking ref
namespace, is much smaller than the set of repos that have some set of
refs mapped differently to the standard
"refs/heads/*:refs/remotes/originname/*" mapping. My "selfish" intent
was to address the latter, without worrying much about the former.
> Isn't that part of
> what the user may want to _find_ _out_, instead of required to give
> as input?
There's certainly value in enabling them to do so!
>
> So, no, I do not think it is agreeable at least not from this end,
> but I may be misunderstanding what you meant to present as your
> design.
No misunderstanding. I was unfortunately more concerned with "fitting
in" with other "git remote" subcommands (which take a remote name)
than making the most useful functionality.
>
> I would understand if it were more like
>
> $ git remote refmap refs/remotes/somepath/{branch-A,branch-B}
> origin:refs/heads/branch-A refs/remotes/somepath/branch-A
> origin:refs/heads/branch-B refs/remotes/somepath/branch-B
>
This is a much better proposal, from my perspective!
> that is,
>
> (1) the new subcommand (refmap) takes one or more refs from the
> command line; they typically are in the refs/remotes hiearchy
> and each asks which remote's which ref needs to be fetched to
> update the ref. Note that the user does *not* need to know
> which remote the refs will be updated from.
>
> (2) the subcommand goes through the "remote.*.fetch" configuration
> items (and its older equivalents in .git/remotes, whose support
> should come free if you used remote.c API properly) to find
> what ref from what remote is fetched to update the refs given
> from the command line.
>
> (3) the output is "<remote>:<ref-at-remote> <our-remote-tracking-branch>"
> one line at a time.
>
> Note that this format allows the "two remotes can both update the
> same remote tracking branches we have" arrangement.
>
^ permalink raw reply
* What's cooking in git.git (Sep 2023, #02; Tue, 5)
From: Junio C Hamano @ 2023-09-06 1:29 UTC (permalink / raw)
To: git
Here are the topics that have been cooking in my tree. Commits
prefixed with '+' are in 'next' (being in 'next' is a sign that a
topic is stable enough to be used and are candidate to be in a
future release). Commits prefixed with '-' are only in 'seen', and
aren't considered "accepted" at all and may be annotated with an URL
to a message that raises issues but they are no means exhaustive. A
topic without enough support may be discarded after a long period of
no activity (of course they can be resubmit when new interests
arise).
Copies of the source code to Git live in many repositories, and the
following is a list of the ones I push into or their mirrors. Some
repositories have only a subset of branches.
With maint, master, next, seen, todo:
git://git.kernel.org/pub/scm/git/git.git/
git://repo.or.cz/alt-git.git/
https://kernel.googlesource.com/pub/scm/git/git/
https://github.com/git/git/
https://gitlab.com/git-vcs/git/
With all the integration branches and topics broken out:
https://github.com/gitster/git/
Even though the preformatted documentation in HTML and man format
are not sources, they are published in these repositories for
convenience (replace "htmldocs" with "manpages" for the manual
pages):
git://git.kernel.org/pub/scm/git/git-htmldocs.git/
https://github.com/gitster/git-htmldocs.git/
Release tarballs are available at:
https://www.kernel.org/pub/software/scm/git/
--------------------------------------------------
[Graduated to 'master']
* jk/test-lsan-denoise-output (2023-08-28) 1 commit
(merged to 'next' on 2023-08-29 at 9642d76264)
+ test-lib: ignore uninteresting LSan output
Tests with LSan from time to time seem to emit harmless message
that makes our tests unnecessarily flakey; we work it around by
filtering the uninteresting output.
source: <20230828183735.GA3015072@coredump.intra.peff.net>
* js/ci-san-skip-p4-and-svn-tests (2023-08-29) 1 commit
(merged to 'next' on 2023-08-29 at 9617f99668)
+ ci(linux-asan-ubsan): let's save some time
Flakey "git p4" tests, as well as "git svn" tests, are now skipped
in the (rather expensive) sanitizer CI job.
source: <pull.1578.v2.git.1693342048633.gitgitgadget@gmail.com>
* rs/parse-options-help-text-is-optional (2023-08-28) 1 commit
(merged to 'next' on 2023-08-29 at 5466f7fcb2)
+ parse-options: allow omitting option help text
It may be tempting to leave the help text NULL for a command line
option that is either hidden or too obvious, but "git subcmd -h"
and "git subcmd --help-all" would have segfaulted if done so. Now
the help text is optional.
source: <2b08dc43-621d-2170-c4a6-c2aac33a9a19@web.de>
* tb/mark-more-tests-as-leak-free (2023-08-29) 3 commits
(merged to 'next' on 2023-08-29 at c1db288281)
+ leak tests: mark t5583-push-branches.sh as leak-free
+ leak tests: mark t3321-notes-stripspace.sh as leak-free
+ leak tests: mark a handful of tests as leak-free
Tests that are known to pass with LSan are now marked as such.
source: <cover.1693263171.git.me@ttaylorr.com>
--------------------------------------------------
[New Topics]
* bc/more-git-var (2023-09-05) 1 commit
- var: avoid a segmentation fault when `HOME` is unset
source: <pull.1580.git.1693808487058.gitgitgadget@gmail.com>
* ks/ref-filter-sort-numerically (2023-09-05) 1 commit
- ref-filter: sort numerically when ":size" is used
source: <20230902090155.8978-1-five231003@gmail.com>
* ob/sequencer-reword-error-message (2023-09-05) 1 commit
- sequencer: fix error message on failure to copy SQUASH_MSG
source: <20230903151132.739136-1-oswald.buddenhagen@gmx.de>
* rs/grep-parseopt-simplify (2023-09-05) 1 commit
- grep: use OPT_INTEGER_F for --max-depth
source: <4d2eb736-4f34-18f8-2eb7-20e7f7b8c2f8@web.de>
* rs/name-rev-use-opt-hidden-bool (2023-09-05) 1 commit
- name-rev: use OPT_HIDDEN_BOOL for --peel-tag
source: <5a86c8f8-fcdc-fee9-8af5-aa5ecb036d2e@web.de>
--------------------------------------------------
[Stalled]
* la/trailer-cleanups (2023-08-06) 5 commits
- trailer: rename *_DEFAULT enums to *_UNSPECIFIED
- trailer: teach find_patch_start about --no-divider
- trailer: split process_command_line_args into separate functions
- trailer: split process_input_file into separate pieces
- trailer: separate public from internal portion of trailer_iterator
Code clean-up.
Expecting a reroll.
cf. <owlyy1iifq0n.fsf@fine.c.googlers.com>
source: <pull.1563.git.1691211879.gitgitgadget@gmail.com>
* la/trailer-test-and-doc-updates (2023-08-10) 14 commits
- SQUASH???
- trailer doc: <token> is a <key> or <keyAlias>, not both
- trailer doc: separator within key suppresses default separator
- trailer doc: emphasize the effect of configuration variables
- trailer --unfold help: prefer "reformat" over "join"
- trailer --parse docs: add explanation for its usefulness
- trailer --only-input: prefer "configuration variables" over "rules"
- trailer --parse help: expose aliased options
- trailer --no-divider help: describe usual "---" meaning
- trailer: trailer location is a place, not an action
- trailer doc: narrow down scope of --where and related flags
- trailer: add tests to check defaulting behavior with --no-* flags
- trailer test description: this tests --where=after, not --where=before
- trailer tests: make test cases self-contained
Test coverage for trailers has been improved.
Expecting a reroll.
cf. <owlyh6p5fpi7.fsf@fine.c.googlers.com>
source: <pull.1564.v2.git.1691702283.gitgitgadget@gmail.com>
* js/doc-unit-tests (2023-08-17) 3 commits
- ci: run unit tests in CI
- unit tests: add TAP unit test framework
- unit tests: Add a project plan document
(this branch is used by js/doc-unit-tests-with-cmake.)
Process to add some form of low-level unit tests has started.
Comments?
source: <cover.1692297001.git.steadmon@google.com>
* ak/pretty-decorate-more (2023-08-21) 8 commits
- decorate: use commit color for HEAD arrow
- pretty: add pointer and tag options to %(decorate)
- pretty: add %(decorate[:<options>]) format
- decorate: color each token separately
- decorate: avoid some unnecessary color overhead
- decorate: refactor format_decorations()
- pretty-formats: enclose options in angle brackets
- pretty-formats: define "literal formatting code"
"git log --format" has been taught the %(decorate) placeholder.
What's the status of this thing?
source: <20230820185009.20095-1-andy.koppe@gmail.com>
* cc/repack-sift-filtered-objects-to-separate-pack (2023-08-13) 8 commits
. gc: add `gc.repackFilterTo` config option
. repack: implement `--filter-to` for storing filtered out objects
. gc: add `gc.repackFilter` config option
. repack: add `--filter=<filter-spec>` option
. repack: refactor finding pack prefix
. repack: refactor finishing pack-objects command
. t/helper: add 'find-pack' test-tool
. pack-objects: allow `--filter` without `--stdout`
"git repack" machinery learns to pay attention to the "--filter="
option.
Kicked out of the 'seen', as it still seems to be failing tests.
cf. https://github.com/git/git/actions/runs/5850998716/job/15861158252#step:4:1822
source: <20230812000011.1227371-1-christian.couder@gmail.com>
* cc/git-replay (2023-06-03) 15 commits
- replay: stop assuming replayed branches do not diverge
- replay: add --contained to rebase contained branches
- replay: add --advance or 'cherry-pick' mode
- replay: disallow revision specific options and pathspecs
- replay: use standard revision ranges
- replay: make it a minimal server side command
- replay: remove HEAD related sanity check
- replay: remove progress and info output
- replay: add an important FIXME comment about gpg signing
- replay: don't simplify history
- replay: introduce pick_regular_commit()
- replay: die() instead of failing assert()
- replay: start using parse_options API
- replay: introduce new builtin
- t6429: remove switching aspects of fast-rebase
No reviews?
source: <20230602102533.876905-1-christian.couder@gmail.com>
* tk/cherry-pick-sequence-requires-clean-worktree (2023-06-01) 1 commit
- cherry-pick: refuse cherry-pick sequence if index is dirty
"git cherry-pick A" that replays a single commit stopped before
clobbering local modification, but "git cherry-pick A..B" did not,
which has been corrected.
Expecting a reroll.
cf. <999f12b2-38d6-f446-e763-4985116ad37d@gmail.com>
source: <pull.1535.v2.git.1685264889088.gitgitgadget@gmail.com>
--------------------------------------------------
[Cooking]
* dd/format-patch-rfc-updates (2023-08-31) 1 commit
(merged to 'next' on 2023-09-01 at ad87c89ee3)
+ format-patch: --rfc honors what --subject-prefix sets
"git format-patch --rfc --subject-prefix=<foo>" used to ignore the
"--subject-prefix" option and used "[RFC PATCH]"; now we will add
"RFC" prefix to whatever subject prefix is specified.
This is a backward compatible change that may deserve a note.
Will merge to 'master'.
source: <20230830064646.30904-1-sir@cmpwn.com>
* jk/unused-post-2.42 (2023-08-29) 22 commits
(merged to 'next' on 2023-08-30 at ab0538e754)
+ update-ref: mark unused parameter in parser callbacks
+ gc: mark unused descriptors in scheduler callbacks
+ bundle-uri: mark unused parameters in callbacks
+ fetch: mark unused parameter in ref_transaction callback
+ credential: mark unused parameter in urlmatch callback
+ grep: mark unused parmaeters in pcre fallbacks
+ imap-send: mark unused parameters with NO_OPENSSL
+ worktree: mark unused parameters in noop repair callback
+ negotiator/noop: mark unused callback parameters
+ add-interactive: mark unused callback parameters
+ grep: mark unused parameter in output function
+ test-trace2: mark unused argv/argc parameters
+ trace2: mark unused config callback parameter
+ trace2: mark unused us_elapsed_absolute parameters
+ stash: mark unused parameter in diff callback
+ ls-tree: mark unused parameter in callback
+ commit-graph: mark unused data parameters in generation callbacks
+ worktree: mark unused parameters in each_ref_fn callback
+ pack-bitmap: mark unused parameters in show_object callback
+ ref-filter: mark unused parameters in parser callbacks
+ sequencer: mark repository argument as unused
+ sequencer: use repository parameter in short_commit_name()
Unused parameters to functions are marked as such, and/or removed,
in order to bring us closer to -Wunused-parameter clean.
Will merge to 'master'.
source: <20230829234305.GA226944@coredump.intra.peff.net>
* tb/multi-cruft-pack (2023-08-29) 4 commits
(merged to 'next' on 2023-08-30 at 15f0b56ed0)
+ Documentation/gitformat-pack.txt: drop mixed version section
+ Documentation/gitformat-pack.txt: remove multi-cruft packs alternative
+ builtin/pack-objects.c: support `--max-pack-size` with `--cruft`
+ builtin/pack-objects.c: remove unnecessary strbuf_reset()
Use of --max-pack-size to allow multiple packfiles to be created is
now supported even when we are sending unreachable objects to cruft
packs.
Will merge to 'master'.
source: <cover.1693262936.git.me@ttaylorr.com>
* jk/ci-retire-allow-ref (2023-08-30) 2 commits
(merged to 'next' on 2023-08-31 at 5fe4861f16)
+ ci: deprecate ci/config/allow-ref script
+ ci: allow branch selection through "vars"
CI update.
Will merge to 'master'.
source: <20230830194919.GA1709446@coredump.intra.peff.net>
* ws/git-svn-retire-faketerm (2023-08-30) 1 commit
(merged to 'next' on 2023-08-31 at cb6e0d658d)
+ git-svn: drop FakeTerm hack
Code clean-up.
Will merge to 'master'.
source: <xmqqa5u888lz.fsf_-_@gitster.g>
* jk/unused-post-2.42-part2 (2023-09-05) 10 commits
(merged to 'next' on 2023-09-05 at 308ca3a052)
+ parse-options: mark unused parameters in noop callback
+ interpret-trailers: mark unused "unset" parameters in option callbacks
+ parse-options: add more BUG_ON() annotations
+ merge: do not pass unused opt->value parameter
+ parse-options: mark unused "opt" parameter in callbacks
+ parse-options: prefer opt->value to globals in callbacks
+ checkout-index: delay automatic setting of to_tempfile
+ format-patch: use OPT_STRING_LIST for to/cc options
+ merge: simplify parsing of "-n" option
+ merge: make xopts a strvec
Unused parameters to functions are marked as such, and/or removed,
in order to bring us closer to -Wunused-parameter clean.
Will merge to 'master'.
source: <20230831211637.GA949188@coredump.intra.peff.net>
* jk/tree-name-and-depth-limit (2023-08-31) 10 commits
- lower core.maxTreeDepth default to 2048
- tree-diff: respect max_allowed_tree_depth
- list-objects: respect max_allowed_tree_depth
- read_tree(): respect max_allowed_tree_depth
- traverse_trees(): respect max_allowed_tree_depth
- add core.maxTreeDepth config
- fsck: detect very large tree pathnames
- tree-walk: rename "error" variable
- tree-walk: drop MAX_TRAVERSE_TREES macro
- tree-walk: reduce stack size for recursive functions
We now limit depth of the tree objects and maximum length of
pathnames recorded in tree objects.
source: <20230831061735.GA2702156@coredump.intra.peff.net>
* js/doc-unit-tests-with-cmake (2023-08-31) 4 commits
- artifacts-tar: when including `.dll` files, don't forget the unit-tests
- unit-tests: do show relative file paths
- unit-tests: do not mistake `.pdb` files for being executable
- cmake: also build unit tests
(this branch uses js/doc-unit-tests.)
Update the base topic to work with CMake builds.
source: <pull.1579.git.1693462532.gitgitgadget@gmail.com>
* ew/hash-with-openssl-evp (2023-08-31) 1 commit
(merged to 'next' on 2023-09-05 at 1ddc0078c8)
+ treewide: fix various bugs w/ OpenSSL 3+ EVP API
Fix-up new-ish code to support OpenSSL EVP API.
Will merge to 'master'.
source: <20230901020928.M610756@dcvr>
* tb/path-filter-fix (2023-08-30) 15 commits
- bloom: introduce `deinit_bloom_filters()`
- commit-graph: reuse existing Bloom filters where possible
- object.h: fix mis-aligned flag bits table
- commit-graph: drop unnecessary `graph_read_bloom_data_context`
- commit-graph.c: unconditionally load Bloom filters
- t/t4216-log-bloom.sh: harden `test_bloom_filters_not_used()`
- bloom: prepare to discard incompatible Bloom filters
- bloom: annotate filters with hash version
- commit-graph: new filter ver. that fixes murmur3
- repo-settings: introduce commitgraph.changedPathsVersion
- t4216: test changed path filters with high bit paths
- t/helper/test-read-graph: implement `bloom-filters` mode
- bloom.h: make `load_bloom_filter_from_graph()` public
- t/helper/test-read-graph.c: extract `dump_graph_info()`
- gitformat-commit-graph: describe version 2 of BDAT
The Bloom filter used for path limited history traversal was broken
on systems whose "char" is unsigned; update the implementation and
bump the format version to 2.
Needs more work?
cf. <20230830200218.GA5147@szeder.dev>
source: <cover.1693413637.git.jonathantanmy@google.com>
* js/config-parse (2023-08-23) 4 commits
- config-parse: split library out of config.[c|h]
- config.c: accept config_parse_options in git_config_from_stdin
- config: report config parse errors using cb
- config: split out config_parse_options
The parsing routines for the configuration files have been split
into a separate file.
Needs review.
source: <cover.1692827403.git.steadmon@google.com>
* jc/update-index-show-index-version (2023-08-18) 3 commits
- test-tool: retire "index-version"
- update-index: add --show-index-version
- update-index doc: v4 is OK with JGit and libgit2
"git update-index" learns "--show-index-version" to inspect
the index format version used by the on-disk index file.
Needs review.
source: <20230818233729.2766281-1-gitster@pobox.com>
* ob/revert-of-revert-is-reapply (2023-09-02) 2 commits
- git-revert.txt: add discussion
- sequencer: beautify subject of reverts of reverts
The default log message created by "git revert", when reverting a
commit that records a revert, has been tweaked.
Will merge to 'next'.
source: <20230821170720.577850-1-oswald.buddenhagen@gmx.de>
* jc/rerere-cleanup (2023-08-25) 4 commits
- rerere: modernize use of empty strbuf
- rerere: try_merge() should use LL_MERGE_ERROR when it means an error
- rerere: fix comment on handle_file() helper
- rerere: simplify check_one_conflict() helper function
(this branch uses jc/unresolve-removal.)
Code clean-up.
Not ready to be reviewed yet.
source: <20230731224409.4181277-1-gitster@pobox.com>
* pw/rebase-i-after-failure (2023-08-01) 7 commits
- rebase -i: fix adding failed command to the todo list
- rebase --continue: refuse to commit after failed command
- rebase: fix rewritten list for failed pick
- sequencer: factor out part of pick_commits()
- sequencer: use rebase_path_message()
- rebase -i: remove patch file after conflict resolution
- rebase -i: move unlink() calls
Various fixes to the behaviour of "rebase -i" when the command got
interrupted by conflicting changes.
Expecting a reroll.
The latter half of the series should be reviewed by those who have
more stake in the sequencer code than myself.
cf. <619e458b-218b-a790-dfb4-9200e201b513@gmail.com>
source: <pull.1492.v3.git.1690903412.gitgitgadget@gmail.com>
* jc/unresolve-removal (2023-07-31) 7 commits
- checkout: allow "checkout -m path" to unmerge removed paths
- checkout/restore: add basic tests for --merge
- checkout/restore: refuse unmerging paths unless checking out of the index
- update-index: remove stale fallback code for "--unresolve"
- update-index: use unmerge_index_entry() to support removal
- resolve-undo: allow resurrecting conflicted state that resolved to deletion
- update-index: do not read HEAD and MERGE_HEAD unconditionally
(this branch is used by jc/rerere-cleanup.)
"checkout --merge -- path" and "update-index --unresolve path" did
not resurrect conflicted state that was resolved to remove path,
but now they do.
Needs review.
source: <20230731224409.4181277-1-gitster@pobox.com>
* rj/status-bisect-while-rebase (2023-08-01) 1 commit
- status: fix branch shown when not only bisecting
"git status" is taught to show both the branch being bisected and
being rebased when both are in effect at the same time.
Needs review.
cf. <xmqqtttia3vn.fsf@gitster.g>
source: <48745298-f12b-8efb-4e48-90d2c22a8349@gmail.com>
^ permalink raw reply
* Re: [PATCH] advice: improve hint for diverging branches.
From: Konstantin Pereiaslov @ 2023-09-06 0:03 UTC (permalink / raw)
To: Junio C Hamano, Konstantin Pereiaslov via GitGitGadget; +Cc: git
In-Reply-To: <xmqqh6o8p5pl.fsf@gitster.g>
Thank you, all good points. I will work on the wording
improvements based on your suggestions.
"Consider the following options:" should be on a new line and
get a "hint:" prefix, I will fix that.
As far as the danger to the user, I was talking about the fact that
the user doing "git reset --hard" is going to lose any uncommitted
work as well as any commits currently in the branch.
> "But more importantly, what guarantees your recomputation using
> '*refspecs' here will match the result of the logic that computed
> 'divergent', which certainly would have already known what commit we
> tried to fast-forward our branch to, and where that commit came
> from? We shouldn't be computing the same thing twice, and in
> different ways; that is a sure way to introduce inconsistent
> results.
This makes sens, I did it this way because I wanted to get a remote and
branch name, not just hash id and it was difficult to get this information.
I will try to rework it so that it shares the code with the logic that
determines
the divergence status.
Any tips on what's the best way to do that would be highly appreciated.
On 9/5/23 18:20, Junio C Hamano wrote:
> "Konstantin Pereiaslov via GitGitGadget" <gitgitgadget@gmail.com>
> writes:
>
>> From: Konstantin Pereiaslov <perk11@perk11.info>
>>
>> Added a description of what the offered options will do and for pull
>> also offered a 3rd option during a pull - a hard reset.
>> This option should be helpful for the new users that accidentally
>> committed into the wrong branch which is a scenario I saw very
>> often.
> cf. Documentation/SubmittingPatches:[[describe-changes]]
>
>> The resulting tooltip looks like this for pull:
>>
>> hint: Diverging branches can't be fast-forwarded.
>> Consider the following options:
> We do not give "hint:" prefix to this line???
>
>> hint:
>> hint: To merge remote changes into your branch:
>> hint: git merge --no-ff
>> hint:
>> hint: To apply your changes on top of remote changes:
>> hint: git rebase
> Hmph, "apply" -> "replay" perhaps?
>
>> hint: To discard your local changes and apply the remote changes:
> Here "apply" is definitely a misnomer. Nothing is applied; you just
> discard your work and adopt (or "accept") the state of the remote as
> a whole.
>
>> hint: git reset --hard refs/remotes/upstream/branch-name
>> hint:
>> hint: Disable this message with "git config advice.diverging false"
> OK.
>
> Overall, "... looks like this" should be shown a bit indented so
> that the example stands out from the text that explains the example.
>
>> There is some danger because it's semi-destructive, but so are
>> other options offered if user doesn't know the commands to
>> revert back.
> Sorry, but I do not quite understand what you want to say here.
>
>> @@ -1112,8 +1126,10 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
>>
>> /* ff-only takes precedence over rebase */
>> if (opt_ff && !strcmp(opt_ff, "--ff-only")) {
>> - if (divergent)
>> - die_ff_impossible();
>> + if (divergent) {
>> + const char* pull_branch_spec = get_pull_branch(repo, *refspecs);
> In this codebase, asterisk sticks to the variable/function
> identifier, not types.
>
> But more importantly, what guarantees your recomputation using
> '*refspecs' here will match the result of the logic that computed
> 'divergent', which certainly would have already known what commit we
> tried to fast-forward our branch to, and where that commit came
> from? We shouldn't be computing the same thing twice, and in
> different ways; that is a sure way to introduce inconsistent
> results.
>
>> + die_ff_impossible_during_pull(pull_branch_spec);
>> + }
>> opt_rebase = REBASE_FALSE;
>> }
>> /* If no action specified and we can't fast forward, then warn. */
>>
>> base-commit: d814540bb75bbd2257f9a6bf59661a84fe8cf3cf
> Thanks.
>
^ permalink raw reply
* Re: [PATCH] advice: improve hint for diverging branches.
From: Konstantin Pereiaslov @ 2023-09-05 23:55 UTC (permalink / raw)
To: Junio C Hamano, Konstantin Pereiaslov via GitGitGadget; +Cc: git
In-Reply-To: <xmqqh6o8p5pl.fsf@gitster.g>
Thank you, all good points. I will work on the wording
improvements based on your suggestions.
"Consider the following options:" should be on a new line and
get a "hint:" prefix, I will fix that.
As far as the danger to the user, I was talking about the fact that
the user doing "git reset --hard" is going to lose any uncommitted
work as well as any commits currently in the branch.
> "But more importantly, what guarantees your recomputation using
> '*refspecs' here will match the result of the logic that computed
> 'divergent', which certainly would have already known what commit we
> tried to fast-forward our branch to, and where that commit came
> from? We shouldn't be computing the same thing twice, and in
> different ways; that is a sure way to introduce inconsistent
> results.
This makes sens, I did it this way because I wanted to get a remote and
branch name, not just hash id and it was difficult to get this information.
I will try to rework it so that it shares the code with the logic that
determines
the divergence status.
Any tips on what's the best way to do that would be highly appreciated.
On 9/5/23 18:20, Junio C Hamano wrote:
> "Konstantin Pereiaslov via GitGitGadget" <gitgitgadget@gmail.com>
> writes:
>
>> From: Konstantin Pereiaslov <perk11@perk11.info>
>>
>> Added a description of what the offered options will do and for pull
>> also offered a 3rd option during a pull - a hard reset.
>> This option should be helpful for the new users that accidentally
>> committed into the wrong branch which is a scenario I saw very
>> often.
> cf. Documentation/SubmittingPatches:[[describe-changes]]
>
>> The resulting tooltip looks like this for pull:
>>
>> hint: Diverging branches can't be fast-forwarded.
>> Consider the following options:
> We do not give "hint:" prefix to this line???
>
>> hint:
>> hint: To merge remote changes into your branch:
>> hint: git merge --no-ff
>> hint:
>> hint: To apply your changes on top of remote changes:
>> hint: git rebase
> Hmph, "apply" -> "replay" perhaps?
>
>> hint: To discard your local changes and apply the remote changes:
> Here "apply" is definitely a misnomer. Nothing is applied; you just
> discard your work and adopt (or "accept") the state of the remote as
> a whole.
>
>> hint: git reset --hard refs/remotes/upstream/branch-name
>> hint:
>> hint: Disable this message with "git config advice.diverging false"
> OK.
>
> Overall, "... looks like this" should be shown a bit indented so
> that the example stands out from the text that explains the example.
>
>> There is some danger because it's semi-destructive, but so are
>> other options offered if user doesn't know the commands to
>> revert back.
> Sorry, but I do not quite understand what you want to say here.
>
>> @@ -1112,8 +1126,10 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
>>
>> /* ff-only takes precedence over rebase */
>> if (opt_ff && !strcmp(opt_ff, "--ff-only")) {
>> - if (divergent)
>> - die_ff_impossible();
>> + if (divergent) {
>> + const char* pull_branch_spec = get_pull_branch(repo, *refspecs);
> In this codebase, asterisk sticks to the variable/function
> identifier, not types.
>
> But more importantly, what guarantees your recomputation using
> '*refspecs' here will match the result of the logic that computed
> 'divergent', which certainly would have already known what commit we
> tried to fast-forward our branch to, and where that commit came
> from? We shouldn't be computing the same thing twice, and in
> different ways; that is a sure way to introduce inconsistent
> results.
>
>> + die_ff_impossible_during_pull(pull_branch_spec);
>> + }
>> opt_rebase = REBASE_FALSE;
>> }
>> /* If no action specified and we can't fast forward, then warn. */
>>
>> base-commit: d814540bb75bbd2257f9a6bf59661a84fe8cf3cf
> Thanks.
^ permalink raw reply
* Re: [PATCH] show doc: redirect user to git log manual instead of git diff-tree
From: Junio C Hamano @ 2023-09-05 23:26 UTC (permalink / raw)
To: Han Young; +Cc: git
In-Reply-To: <20230905121219.69762-1-hanyang.tony@bytedance.com>
Han Young <hanyang.tony@bytedance.com> writes:
> While git show accepts options that apply to the git diff-tree command,
> some options do not make sense in the context of git show.
Wow, "diff-tree" is a bit too arcane, I would agree.
Strictly speaking, "options to control how the changes are shown"
are options that are meant for "diff" command (e.g. "--stat", "-w"),
but "log" understands some of the "diff" command options, the
updated text is *not* incorrect.
Because "show" is about displaying individual commits and not range,
some options that are meant for the "log" command (e.g.
"--first-parent", "--no-merges") do not make much sense. So
Some options that `git log` command understands can be used to
control how the changes the commit introduces are shown.
or something like that, perhaps? I dunno.
> -The command takes options applicable to the 'git diff-tree' command to
> +The command takes options applicable to the 'git log' command to
> control how the changes the commit introduces are shown.
>
> This manual page describes only the most frequently used options.
^ permalink raw reply
* Re: [PATCH] advice: improve hint for diverging branches.
From: Junio C Hamano @ 2023-09-05 23:20 UTC (permalink / raw)
To: Konstantin Pereiaslov via GitGitGadget; +Cc: git, Konstantin Pereiaslov
In-Reply-To: <pull.1570.git.git.1693861162353.gitgitgadget@gmail.com>
"Konstantin Pereiaslov via GitGitGadget" <gitgitgadget@gmail.com>
writes:
> From: Konstantin Pereiaslov <perk11@perk11.info>
>
> Added a description of what the offered options will do and for pull
> also offered a 3rd option during a pull - a hard reset.
> This option should be helpful for the new users that accidentally
> committed into the wrong branch which is a scenario I saw very
> often.
cf. Documentation/SubmittingPatches:[[describe-changes]]
> The resulting tooltip looks like this for pull:
>
> hint: Diverging branches can't be fast-forwarded.
> Consider the following options:
We do not give "hint:" prefix to this line???
> hint:
> hint: To merge remote changes into your branch:
> hint: git merge --no-ff
> hint:
> hint: To apply your changes on top of remote changes:
> hint: git rebase
Hmph, "apply" -> "replay" perhaps?
> hint: To discard your local changes and apply the remote changes:
Here "apply" is definitely a misnomer. Nothing is applied; you just
discard your work and adopt (or "accept") the state of the remote as
a whole.
> hint: git reset --hard refs/remotes/upstream/branch-name
> hint:
> hint: Disable this message with "git config advice.diverging false"
OK.
Overall, "... looks like this" should be shown a bit indented so
that the example stands out from the text that explains the example.
> There is some danger because it's semi-destructive, but so are
> other options offered if user doesn't know the commands to
> revert back.
Sorry, but I do not quite understand what you want to say here.
> @@ -1112,8 +1126,10 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
>
> /* ff-only takes precedence over rebase */
> if (opt_ff && !strcmp(opt_ff, "--ff-only")) {
> - if (divergent)
> - die_ff_impossible();
> + if (divergent) {
> + const char* pull_branch_spec = get_pull_branch(repo, *refspecs);
In this codebase, asterisk sticks to the variable/function
identifier, not types.
But more importantly, what guarantees your recomputation using
'*refspecs' here will match the result of the logic that computed
'divergent', which certainly would have already known what commit we
tried to fast-forward our branch to, and where that commit came
from? We shouldn't be computing the same thing twice, and in
different ways; that is a sure way to introduce inconsistent
results.
> + die_ff_impossible_during_pull(pull_branch_spec);
> + }
> opt_rebase = REBASE_FALSE;
> }
> /* If no action specified and we can't fast forward, then warn. */
>
> base-commit: d814540bb75bbd2257f9a6bf59661a84fe8cf3cf
Thanks.
^ permalink raw reply
* Re: [PATCH 1/1] doc/diff-options: fix link to generating patch section
From: Junio C Hamano @ 2023-09-05 22:49 UTC (permalink / raw)
To: Sergey Organov; +Cc: git, John Cai
In-Reply-To: <87zg20qzhg.fsf@osv.gnss.ru>
Sergey Organov <sorganov@gmail.com> writes:
> First, there is no need for conditional referencing, as all the files
> that include "diff-options.txt" eventually include
> "diff-generate-patch.txt" as well.
Except for git-format-patch.txt which includes the former but not
the latter. But this is inside ifndef::git-format-patch[], so the
above description being a bit imprecise does not cause any actual
damage.
Documentation for all commands that want to describe the `-p`
option by including the "diff-options.txt" file also include the
"diff-generate-patch.txt" file, so an internal link would work
for all of them.
or something like that, perhaps.
> Next, when formatted as man-page, the section title is rendered
> "GENERATING PATCH TEXT WITH -P" whereas reference still reads
> "Generating patch text with -p", that is both inconsistent and makes
> searching harder than it needs to be.
>
> Fix the issues by just referring to the section, without custom
> reference text, and then unconditionally.
That does make sense.
> Fixes: ebdc46c242 (docs: link generating patch sections)
> Signed-off-by: Sergey Organov <sorganov@gmail.com>
> ---
> Documentation/diff-options.txt | 8 +-------
> 1 file changed, 1 insertion(+), 7 deletions(-)
>
> diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt
> index 9f33f887711d..c07488b123c6 100644
> --- a/Documentation/diff-options.txt
> +++ b/Documentation/diff-options.txt
> @@ -22,13 +22,7 @@ ifndef::git-format-patch[]
> -p::
> -u::
> --patch::
> - Generate patch (see section titled
> -ifdef::git-log[]
> -<<generate_patch_text_with_p, "Generating patch text with -p">>).
> -endif::git-log[]
> -ifndef::git-log[]
> -"Generating patch text with -p").
> -endif::git-log[]
> + Generate patch (see <<generate_patch_text_with_p>>).
> ifdef::git-diff[]
> This is the default.
> endif::git-diff[]
^ permalink raw reply
* Re: Potential bug in `git checkout --quiet`
From: Junio C Hamano @ 2023-09-05 22:36 UTC (permalink / raw)
To: Bagas Sanjaya
Cc: Radovan Haluška, git, Elijah Newren, William Sprent,
Ævar Arnfjörð Bjarmason
In-Reply-To: <ZPXdeRnLu6oKoVt2@debian.me>
Bagas Sanjaya <bagasdotme@gmail.com> writes:
> On Mon, Sep 04, 2023 at 02:56:37PM +0200, Radovan Haluška wrote:
>> What did you do before the bug happened? (Steps to reproduce your issue)
>>
>> ```
>> git clone --quiet --branch master --depth 1 --no-checkout --filter blob:none \
>> git@github.com:acatai/Strategy-Card-Game-AI-Competition.git locm-agents
>> cd locm-agents
>> git sparse-checkout set --no-cone
>> git sparse-checkout add 'contest-2022-08-COG/ByteRL'
>> git checkout --quiet
>> ```
>>
>> What did you expect to happen? (Expected behavior)
>>
>> I expected to receive no output from any of the commands above.
>>
>> What happened instead? (Actual behavior)
>>
>> I received an output from the last command even though the `--quiet` switch was specified
>>
>> What's different between what you expected and what actually happened?
>>
>> This shouldn't have been printed on the screen:
>>
>> '''
>> remote: Enumerating objects: 28, done.
>> remote: Counting objects: 100% (28/28), done.
>> remote: Compressing objects: 100% (27/27), done.
>> remote: Total 28 (delta 0), reused 25 (delta 0), pack-reused 0
>> Receiving objects: 100% (28/28), 31.40 MiB | 4.94 MiB/s, done.
>> '''
>>
>
> I can reproduce this bug on v2.40.0 using your reproducer above. Yet,
> `git checkout --quiet` on normal repos (not partial ones) works as
> expected. Cc'ing people working on sparse-checkout recently.
I am not much involved in the lazy clone's on-demand downloading,
but I am torn between calling this a bug and a feature.
Just like the original reporter "expected to receive no output", for
a pure Git person like me, I expect to see *no* network activity
while performing a local operation like "checkout". And from that
point of view, "checkout --quiet" telling me that something
unexpected and unusual (i.e. the operation "checkout" that is
supposed to be local is lazily downloading blobs and trees from the
outside world) is happening with the extra output is something I may
even appreciate.
Having said that, I would not fundamentally oppose if those who want
to improve the lazy clone feature wants to squelch messages from the
transport layer while the top-level front end ("checkout" in this case)
wants to operate quietly.
Thanks for a report.
^ permalink raw reply
* Re: [PATCH v2 1/3] range-diff: treat notes like `log`
From: Junio C Hamano @ 2023-09-05 22:19 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: Kristoffer Haugsbakk, git, Denton Liu, Jeff King
In-Reply-To: <843bc10e-03ab-b37a-68f7-d6055dc9308c@gmx.de>
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> Hi Kristoffer,
>
> On Mon, 4 Sep 2023, Kristoffer Haugsbakk wrote:
>
>> I would like to use your solution instead. Thank you.
>
> Please do! You're welcome.
>
>> Maybe you could supply a commit message for v3? v3 would then consist of
>> two commits:
>>
>> 1. Your fix
>> 2. Those two tests
>>
>> Or should it be handled in some other way?
>
> Personally, I'd prefer it if you just squashed the changes into your
> patch. If you want, I'd be delighted about a Helped-by (or even
> Co-authored-by) footer, but taking ownership would be fine with me, too.
Sounds very sensible.
Thanks, both. Will look forward to seeing an updated series.
^ permalink raw reply
* Re: Plumbing for mapping from a remote tracking ref to the remote ref?
From: Junio C Hamano @ 2023-09-05 22:18 UTC (permalink / raw)
To: Tao Klerks; +Cc: Johannes Schindelin, git
In-Reply-To: <CAPMMpojUrfSmpgWVh3TTn_uamPCcyHRQf2R3APSpEjsqujNXvA@mail.gmail.com>
Tao Klerks <tao@klerks.biz> writes:
> On Sun, Jun 19, 2022 at 1:04 AM Junio C Hamano <gitster@pobox.com> wrote:
>>
>> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>>
>> >> $ git refmap refs/remotes/somepath/{branch-A,branch-B}
>> >> origin refs/heads/branch-A
>> >> origin refs/heads/branch-B
>> >>
>> >> IOW, you give name(s) of remote-tracking branches and then you get
>> >> the remote and their ref for these?
>> >
>> > Modulo introducing a new top-level command (a subcommand of `git remote`
>> > would make much more sense and make the feature eminently more
>> > discoverable), and modulo allowing patterns in the ref to match, I agree.
>>
>> "git remote" is primarily about "I have this remote---tell me more
>> about it", but this query goes in the other direction, and that is
>> why I threw a non-existing command to solicit alternatives that are
>> potentially better than "git remote".
>
> Thank you for the responses here, and my apologies for not following
> up (much) earlier.
> ...
> Would something like the following be mutually agreeable?
>
> $ git remote origin map-ref
> refs/remotes/my-favorite-remotes/origin/someref
> refs/heads/someref
With strainge line wrapping, I cannot quite tell what is the input
and what is the output, but if you meant that the part up to the
long-ish refname in the refs/remotes is the command line, and map-ref
is the new subcommand name in the "git remote" command, i.e.
$ git remote map-ref origin refs/remotes/my-favorite-remotes/origin/someref
is the input, to which the output
refs/heads/someref
is given, I am not sure what its value is. First of all, the user
is giving a ref in a hierarchy that is usually used for the remote
whose name is "my-favorite-remotes". What made this user _know_
that the remote reference belongs to "origin"? Isn't that part of
what the user may want to _find_ _out_, instead of required to give
as input?
So, no, I do not think it is agreeable at least not from this end,
but I may be misunderstanding what you meant to present as your
design.
I would understand if it were more like
$ git remote refmap refs/remotes/somepath/{branch-A,branch-B}
origin:refs/heads/branch-A refs/remotes/somepath/branch-A
origin:refs/heads/branch-B refs/remotes/somepath/branch-B
that is,
(1) the new subcommand (refmap) takes one or more refs from the
command line; they typically are in the refs/remotes hiearchy
and each asks which remote's which ref needs to be fetched to
update the ref. Note that the user does *not* need to know
which remote the refs will be updated from.
(2) the subcommand goes through the "remote.*.fetch" configuration
items (and its older equivalents in .git/remotes, whose support
should come free if you used remote.c API properly) to find
what ref from what remote is fetched to update the refs given
from the command line.
(3) the output is "<remote>:<ref-at-remote> <our-remote-tracking-branch>"
one line at a time.
Note that this format allows the "two remotes can both update the
same remote tracking branches we have" arrangement.
^ permalink raw reply
* Re: [PATCH v2 2/3] builtin/rebase.c: Emit warning when rebasing without a forkpoint
From: Junio C Hamano @ 2023-09-05 22:01 UTC (permalink / raw)
To: Wesley Schwengle; +Cc: git
In-Reply-To: <2e1f1d2a-4aec-4b60-bc96-685a27055c06@opperschaap.net>
Wesley Schwengle <wesleys@opperschaap.net> writes:
> I like the idea of the warning, but it could be loud indeed and you'll
> want to turn it off in that case.
I tend to think that a single-liner warning would not be too
intrusive (it might actually be too subtle to be noticed),
especially given that it is issued only when the fork-point does
move the target commit from what was given.
Giving a shortlog of what is lost in the history does sound like a
bit too loud, I am afraind, though.
^ permalink raw reply
* [PATCH 8/8] builtin/repack.c: extract common cruft pack loop
From: Taylor Blau @ 2023-09-05 20:37 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jeff King, Patrick Steinhardt
In-Reply-To: <cover.1693946195.git.me@ttaylorr.com>
When generating the list of packs to store in a MIDX (when given the
`--write-midx` option), we include any cruft packs both during
--geometric and non-geometric repacks.
But the rules for when we do and don't have to check whether any of
those cruft packs were queued for deletion differ slightly between the
two cases.
But the two can be unified, provided there is a little bit of extra
detail added in the comment to clarify when it is safe to avoid checking
for any pending deletions (and why it is OK to do so even when not
required).
Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
builtin/repack.c | 30 ++++++++++++++++++------------
1 file changed, 18 insertions(+), 12 deletions(-)
diff --git a/builtin/repack.c b/builtin/repack.c
index 6110598a69..e6a1ef9a09 100644
--- a/builtin/repack.c
+++ b/builtin/repack.c
@@ -695,25 +695,31 @@ static void midx_included_packs(struct string_list *include,
string_list_insert(include, strbuf_detach(&buf, NULL));
}
- for_each_string_list_item(item, &existing->cruft_packs) {
- /*
- * 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 (item->util)
continue;
string_list_insert(include, xstrfmt("%s.idx", item->string));
}
+ }
- for_each_string_list_item(item, &existing->cruft_packs) {
- if (item->util)
- continue;
- string_list_insert(include, xstrfmt("%s.idx", item->string));
- }
+ for_each_string_list_item(item, &existing->cruft_packs) {
+ /*
+ * When doing a --geometric repack, there is no need to check
+ * for deleted packs, since we're by definition not doing an
+ * ALL_INTO_ONE repack (hence no packs will be deleted).
+ * Otherwise we must check for and exclude any packs which are
+ * enqueued for deletion.
+ *
+ * So we could omit the conditional below in the --geometric
+ * case, but doing so is unnecessary since no packs are marked
+ * as pending deletion (since we only call
+ * `mark_packs_for_deletion()` when doing an all-into-one
+ * repack).
+ */
+ if (item->util)
+ continue;
+ string_list_insert(include, xstrfmt("%s.idx", item->string));
}
}
--
2.42.0.119.gca7d13e7bf
^ permalink raw reply related
* [PATCH 7/8] builtin/repack.c: drop `DELETE_PACK` macro
From: Taylor Blau @ 2023-09-05 20:36 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jeff King, Patrick Steinhardt
In-Reply-To: <cover.1693946195.git.me@ttaylorr.com>
Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
builtin/repack.c | 18 ++++++++++--------
1 file changed, 10 insertions(+), 8 deletions(-)
diff --git a/builtin/repack.c b/builtin/repack.c
index 478fab96c9..6110598a69 100644
--- a/builtin/repack.c
+++ b/builtin/repack.c
@@ -26,8 +26,6 @@
#define LOOSEN_UNREACHABLE 2
#define PACK_CRUFT 4
-#define DELETE_PACK 1
-
static int pack_everything;
static int delta_base_offset = 1;
static int pack_kept_objects = -1;
@@ -96,6 +94,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 +132,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 = (void*)1;
}
}
@@ -158,7 +160,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 +697,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.42.0.119.gca7d13e7bf
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox