* Re: [PATCH v3 2/2] rebase-interactive: warn if commit is dropped with `rebase --edit-todo'
From: Phillip Wood @ 2020-01-11 14:44 UTC (permalink / raw)
To: Alban Gruin, phillip.wood, Junio C Hamano; +Cc: git, Johannes Schindelin
In-Reply-To: <85e3d40a-9829-0f50-8d91-7e8e8fa319f1@gmail.com>
Hi Alban
On 10/01/2020 21:31, Alban Gruin wrote:
> Hi Phillip,
>
> Le 10/01/2020 à 18:13, Phillip Wood a écrit :
>> Hi Alban
>>
>> On 09/01/2020 21:13, Alban Gruin wrote:
>>> Hi Phillip,
>>> [...]
>>>
>>> In which case, if the check did not pass at the previous edit, the new
>>> todo list should be compared to the backup. As sequencer_continue()
>>> already does this, extract this to its own function in
>>> rebase-interactive.c. To keep track of this, a file is created on the
>>> disk (as you suggested in your other email.) At the next edit, if this
>>> file exists and no errors were found, it is deleted. The backup is only
>>> created if there is no errors in `todo_list' and in `new_todo'.
>>>
>>> This would guarantee that there is no errors in the backup, and that the
>>> edited list is always compared to a list exempt of errors.
>>>
>>> This approach also has the benefit to detect if a commit part of a
>>> badcmd was dropped.
>>>
>>> After some tweaks (ie. `expect' now lists 2 commits instead of one),
>>> this passes the test with the change you suggested, and the one you sent
>>> in your other email.
>>
>> That sounds good. I'm not sure how it passes the test in my other email
>> though, if sequencer_continue() compares the todo list to the backup
>> wont it still fail when continuing after conflicts as the backup is out
>> of date?
>>
>
> I changed sequencer_continue() to check the todo list only if the file
> indicating an error exists.
That makes sense
> I still have to rewrite the commit message, then I’ll re-send this series.
Excellent, I look forward to reading them
Best Wishes
Phillip
> Cheers,
> Alban
>
>> Best Wishes
>>
>> Phillip
>>
^ permalink raw reply
* Re: [PATCH v3 15/15] rebase: change the default backend from "am" to "merge"
From: Phillip Wood @ 2020-01-11 14:41 UTC (permalink / raw)
To: Elijah Newren, Jonathan Nieder
Cc: Elijah Newren via GitGitGadget, Git Mailing List,
Johannes Schindelin, Phillip Wood, Denton Liu, Junio C Hamano,
Pavel Roskin, Alban Gruin, SZEDER Gábor
In-Reply-To: <CABPp-BEGwJ=0+6TMYXvMzpds0h6bz4gZA8Pi95SH9M4CKBtfOw@mail.gmail.com>
Hi Jonathan & Elijah
On 11/01/2020 01:16, Elijah Newren wrote:
> Hi Jonathan,
>
> On Fri, Jan 10, 2020 at 3:14 PM Jonathan Nieder <jrnieder@gmail.com> wrote:
>>
>> Hi,
>>
>> Elijah Newren via GitGitGadget wrote:
>>
>>> The am-backend drops information and thus limits what we can do:
>>>
>>> * lack of full tree information from the original commits means we
>>> cannot do directory rename detection and warn users that they might
>>> want to move some of their new files that they placed in old
>>> directories to prevent their becoming orphaned.[1]
>>> * reduction in context from only having a few lines beyond those
>>> changed means that when context lines are non-unique we can apply
>>> patches incorrectly.[2]
>>> * lack of access to original commits means that conflict marker
>>> annotation has less information available.
>>>
>>> Also, the merge/interactive backend have far more abilities, appear to
>>> currently have a slight performance advantage[3] and have room for more
>>> optimizations than the am backend[4] (and work is underway to take
>>> advantage of some of those possibilities).
>>>
>>> [1] https://lore.kernel.org/git/xmqqh8jeh1id.fsf@gitster-ct.c.googlers.com/
>>> [2] https://lore.kernel.org/git/CABPp-BGiu2nVMQY_t-rnFR5GQUz_ipyEE8oDocKeO+h+t4Mn4A@mail.gmail.com/
>>> [3] https://public-inbox.org/git/CABPp-BF=ev03WgODk6TMQmuNoatg2kiEe5DR__gJ0OTVqHSnfQ@mail.gmail.com/
>>> [4] https://lore.kernel.org/git/CABPp-BGh7yW69QwxQb13K0HM38NKmQif3A6C6UULEKYnkEJ5vA@mail.gmail.com/
>>>
>>> Signed-off-by: Elijah Newren <newren@gmail.com>
>>> ---
>>> Documentation/git-rebase.txt | 2 +-
>>> builtin/rebase.c | 4 ++--
>>> t/t5520-pull.sh | 10 ++++++----
>>> t/t9106-git-svn-commit-diff-clobber.sh | 3 ++-
>>> 4 files changed, 11 insertions(+), 8 deletions(-)
>>
>> Thanks for writing this. We finally rolled this out to our internal
>> population at $DAYJOB and ran into a couple of issues:
>
> Cool, thanks for testing it out.
>
>> 1. "git rebase --am" does not invoke the post-commit hook, but "git
>> rebase --merge" does. Is this behavior change intended?
>>
>> Noticed because jiri[1] installs a post-commit hook that warns
>> about commits on detached HEAD, so this change makes rebases more
>> noisy in repositories that were set up using jiri.
Perhaps that hook could learn not to warn if a branch is being rebased?
git could be more helpful there by having a porcelain option to status
that prints the branch name if we're rebasing (`git worktree --list`
shows the branch correctly when it's being rebased but does not (yet - I
have a patch to do it) mark the current worktree so isn't very helpful.)
> I've never used a post-commit hook or seen one in the wild. Certainly
> wasn't intentional, but it's not clear to me if it's wrong or right
> either. I don't see why it would make sense to distinguish between
> any of git rebase --am/--merge/--interactive, but it isn't too
> surprising that by historical accident the two rebase backends which
> happened to call git-commit behind the scenes would call a post-commit
> hook and the other rebase backend that didn't call git-commit
> wouldn't.
Looking through the history the am based rebase has never run the
post-commit hook as am has its own set of hooks and the scripted version
used commit-tree. The merge based rebase ran `git commit` which ran the
post commit hook. The interactive rebase ran the hook until and I broke
it in a356ee4659b ("sequencer: try to commit without forking 'git
commit'", 2017-11-24) and after I fixed it in 4627bc777e ("sequencer:
run post-commit hook", 2019-10-15). As it was broken for two years with
no one noticing it can't be that popular.
> But the big question here, is what is correct behavior? Should rebase
> call the post-commit hook, or should it skip it? I haven't any clue
> what the answer to that is.
It's creating a new commit so I lean towards thinking it should run the
post-commit hook. As an example I have a post-commit hook that prints a
warning if a commit is created on a branch that is being rewritten by
one of my scripts in another worktree. There are pre-commit and
pre-rebase hooks to try and prevent that, but the warning is there as a
last resort if those hooks are by-passed.
>> 2. GIT_REFLOG_ACTION contains "rebase -i" even though the rebase is
>> not interactive.
If this is important to people I think it should be easy enough to set
GIT_REFLOG_ACTION to the appropriate string in builtin/rebase.c (so long
as it hasn't already been set by the user) rather than relying on
sequencer.c to do it.
> Yep, as does --keep, --exec, --rebase-merges, etc. There are lots of
> rebases which use the interactive machinery even if they aren't
> explicitly interactive. I've never seen the "-i" in the reflog
> message defined, but clearly it has always been used whenever the
> interactive machinery was in play regardless of whether the rebase was
> interactive. In that regard, I figured that --merge fit in rather
> nicely. (And I noted the fact that reflog messages were different
> between the backends among the "BEHAVIORAL DIFFERENCES" section of
> git-rebase.txt). But if others think we should just drop the -i (much
> as we did for the bash prompt), I'd be happy with that too. If we go
> that route, I think I'd rather drop the -i in the reflog for all
> rebases, not just the
> using-the-interactive-machinery-but-not-explicitly-interactive ones.
>
>> 3. In circumstances I haven't pinned down yet, we get the error
>> message "invalid date format: @@2592000 +0000":
>>
>> $ git rebase --committer-date-is-author-date --onto branch_K branch_L~1 branch_L
>> $ git checkout --theirs file
>> $ git add file
>> $ git rebase --continue
>> fatal: invalid date format: @@2592000 +0000
>> error: could not commit staged changes.
>>
>> This isn't reproducible without --committer-date-is-author-date.
>> More context (the test where it happens) is in [2].
>
> Interesting. Do you happen to know if this started happening with
> ra/rebase-i-more-options, or did it just become an issue with
> en/rebase-backend? I looked around at the link you provided and feel
> a bit confused; I'm not sure which test does this or how I'd
> reproduce.
I'm confused by the test as well. As ra/rebase-i-more-options only
touched the sequencer then any bugs would only show up in this test
(which runs a non-interactive rebase) once en/rbease-backend switched to
that backend. It seems likely that ra/rebase-i-more-options is to blame.
Jonathan - do you happen to know if your users create empty commits at
all? and if so what do they expect rebase to do with them (and any that
become empty when they are rebased) - cf
https://lore.kernel.org/git/<CABPp-BEH=9qejeqysHYE+AJ+JPaBympZizq-bx_OjArYFa4xUQ@mail.gmail.com>
Best Wishes
Phillip
>> 4. I suspect the exit status in the "you need to resolve conflicts"
>> case has changed. With rebase --am, [3] would automatically
>> invoke rebase --abort when conflicts are present, but with rebase
>> --merge it does not.
>>
>> Known?
>
> Nope, but I would certainly hope that "you need to resolve conflicts"
> would result in a non-zero exit status. If it doesn't, that sounds
> like a bug in the interactive backend that we need to fix. I'll dig
> in.
>
>
>
> Thanks for the reports!
> Elijah
>
^ permalink raw reply
* Re: [PATCH] branch: let '--edit-description' default to rebased branch during rebase
From: Marc-André Lureau @ 2020-01-11 14:54 UTC (permalink / raw)
To: Eric Sunshine; +Cc: Git List, SZEDER Gábor
In-Reply-To: <CAPig+cQXkiFOz5HczPEgXuSOH_3KsCwXwVwe0qvQzLDtFgnAXw@mail.gmail.com>
Hi
On Sat, Jan 11, 2020 at 5:28 PM Eric Sunshine <sunshine@sunshineco.com> wrote:
>
> On Sat, Jan 11, 2020 at 7:36 AM <marcandre.lureau@redhat.com> wrote:
> > Defaulting to editing the description of the rebased branch without an
> > explicit branchname argument would be useful. Even the git bash prompt
> > shows the name of the rebased branch, and then
> >
> > ~/src/git (mybranch|REBASE-i 1/2)$ git branch --edit-description
> > fatal: Cannot give description to detached HEAD
> >
> > looks quite unhelpful.
> >
> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > ---
> > diff --git a/builtin/branch.c b/builtin/branch.c
> > @@ -745,15 +745,27 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
> > if (!argc) {
> > - if (filter.detached)
> > - die(_("Cannot give description to detached HEAD"));
> > - branch_name = head;
> > + if (filter.detached) {
> > + struct wt_status_state state;
> > +
> > + memset(&state, 0, sizeof(state));
> > +
> > + if (wt_status_check_rebase(NULL, &state)) {
> > + branch_name = state.branch;
> > + }
>
> Style: drop unneeded braces.
ok
>
> > +
> > + if (!branch_name)
> > + die(_("Cannot give description to detached HEAD"));
> > +
> > + free(state.onto);
>
> Also, no need for all the blank lines which eat up valuable vertical
> screen real-estate without making the code clearer.
ok
>
> > + } else
> > + branch_name = xstrdup(head);
>
> It would be easier to see what happens in the common case (when not
> rebasing) if you invert the condition to `if (!filter.detached)` and
> turn this one-line 'else' branch into the 'if' branch.
indeed
>
> > @@ -772,6 +784,8 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
> > if (edit_branch_description(branch_name))
> > return 1;
> > +
> > + free(branch_name);
>
> That `return 1` just above this free() is leaking 'branch_name', isn't it?
right, let's fix that too
>
> > diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
> > @@ -1260,6 +1260,25 @@ test_expect_success 'use --edit-description' '
> > +test_expect_success 'use --edit-description during rebase' '
> > + write_script editor <<-\EOF &&
> > + echo "Rebase contents" >"$1"
> > + EOF
> > + (
> > + set_fake_editor &&
> > + FAKE_LINES="break 1" git rebase -i HEAD^ &&
> > + EDITOR=./editor git branch --edit-description &&
> > + git rebase --continue
> > + ) &&
> > + write_script editor <<-\EOF &&
> > + git stripspace -s <"$1" >"EDITOR_OUTPUT"
> > + EOF
> > + EDITOR=./editor git branch --edit-description &&
> > + echo "Rebase contents" >expect &&
> > + test_cmp expect EDITOR_OUTPUT
> > +'
> > +test_done
>
> Strange place for a test_done() invocation considering that existing
> tests follow the new one added by this patch.
doh, sorry
thanks for the review!
>
> > test_expect_success 'detect typo in branch name when using --edit-description' '
> > write_script editor <<-\EOF &&
> > echo "New contents" >"$1"
--
Marc-André Lureau
^ permalink raw reply
* [PATCH v4 0/2] rebase -i: extend rebase.missingCommitsCheck
From: Alban Gruin @ 2020-01-11 17:39 UTC (permalink / raw)
To: git; +Cc: Johannes Schindelin, Phillip Wood, Junio C Hamano, Alban Gruin
In-Reply-To: <20191202234759.26201-1-alban.gruin@gmail.com>
To prevent mistakes when editing a branch, rebase features a knob,
rebase.missingCommitsCheck, to warn the user if a commit was dropped.
Unfortunately, this check is only effective for the initial edit, which
means that if you edit the todo list at a later point of the rebase and
drop a commit, no warnings or errors would be issued.
This adds the ability to check if commits were dropped when editing the
todo list with `--edit-todo', and when resuming a rebase.
The first patch moves moves check_todo_list_from_file() and
`edit_todo_list_advice' from sequencer.c to rebase-interactive.c so the
latter can be used by edit_todo_list() and todo_list_check(). The
second patch adds the check to `--edit-todo' and `--continue' and tests.
This is based on 393adf7a6f ("sequencer: directly call pick_commits()
from complete_action()", 2019-11-24).
The tip of this series is tagged as "edit-todo-drop-v4" at
https://github.com/agrn/git.
Changes since v3:
- Set the fake editor in a subshell in the tests to avoid conflicts with
pw/post-commit-from-sequencer.
- Added the `dropped' file to indicate that the todo list is incorrect
(ie. no invalid commands or dropped commits.)
- The todo list is copied only if it is correct.
- sequencer_continue() checks the todo list for dropped commits only
when `dropped' exists.
- Added a new test to check that `rebase --continue' does not warn for
dropped commits after solving a conflict. This was provided by
Phillip Wood.
- check_todo_list_from_file() no longer prints `edit_todo_list_advice'
if there is dropped commits since todo_list_check() do it now.
Alban Gruin (2):
sequencer: move check_todo_list_from_file() to rebase-interactive.c
rebase-interactive: warn if commit is dropped with `rebase
--edit-todo'
rebase-interactive.c | 89 ++++++++++++++++++++++++---
rebase-interactive.h | 5 ++
sequencer.c | 51 ++++------------
sequencer.h | 2 +-
t/t3404-rebase-interactive.sh | 109 ++++++++++++++++++++++++++++++++++
5 files changed, 206 insertions(+), 50 deletions(-)
Diff-intervalle contre v3:
1: 996045a300 = 1: 996045a300 sequencer: move check_todo_list_from_file() to rebase-interactive.c
2: ba6d27e5b4 < -: ---------- rebase-interactive: warn if commit is dropped with `rebase --edit-todo'
-: ---------- > 2: 11b0e1e78c rebase-interactive: warn if commit is dropped with `rebase --edit-todo'
--
2.24.1
^ permalink raw reply
* [PATCH v4 1/2] sequencer: move check_todo_list_from_file() to rebase-interactive.c
From: Alban Gruin @ 2020-01-11 17:39 UTC (permalink / raw)
To: git; +Cc: Johannes Schindelin, Phillip Wood, Junio C Hamano, Alban Gruin
In-Reply-To: <20200111173917.15690-1-alban.gruin@gmail.com>
The message contained in `edit_todo_list_advice' (sequencer.c) is
printed after the initial edit of the todo list if it can't be parsed or
if commits were dropped. This is done either in complete_action() for
`rebase -i', or in check_todo_list_from_file() for `rebase -p'.
Since we want to add this check when editing the list, we also want to
use this message from edit_todo_list() (rebase-interactive.c). To this
end, check_todo_list_from_file() is moved to rebase-interactive.c, and
`edit_todo_list_advice' is copied there. In the next commit,
complete_action() will stop using it, and `edit_todo_list_advice' will
be removed from sequencer.c.
Signed-off-by: Alban Gruin <alban.gruin@gmail.com>
---
rebase-interactive.c | 35 +++++++++++++++++++++++++++++++++++
rebase-interactive.h | 2 ++
sequencer.c | 29 -----------------------------
sequencer.h | 1 -
4 files changed, 37 insertions(+), 30 deletions(-)
diff --git a/rebase-interactive.c b/rebase-interactive.c
index aa18ae82b7..ad5dd49c31 100644
--- a/rebase-interactive.c
+++ b/rebase-interactive.c
@@ -6,6 +6,12 @@
#include "commit-slab.h"
#include "config.h"
+static const char edit_todo_list_advice[] =
+N_("You can fix this with 'git rebase --edit-todo' "
+"and then run 'git rebase --continue'.\n"
+"Or you can abort the rebase with 'git rebase"
+" --abort'.\n");
+
enum missing_commit_check_level {
MISSING_COMMIT_CHECK_IGNORE = 0,
MISSING_COMMIT_CHECK_WARN,
@@ -187,3 +193,32 @@ int todo_list_check(struct todo_list *old_todo, struct todo_list *new_todo)
clear_commit_seen(&commit_seen);
return res;
}
+
+int check_todo_list_from_file(struct repository *r)
+{
+ struct todo_list old_todo = TODO_LIST_INIT, new_todo = TODO_LIST_INIT;
+ int res = 0;
+
+ if (strbuf_read_file(&new_todo.buf, rebase_path_todo(), 0) < 0) {
+ res = error(_("could not read '%s'."), rebase_path_todo());
+ goto out;
+ }
+
+ if (strbuf_read_file(&old_todo.buf, rebase_path_todo_backup(), 0) < 0) {
+ res = error(_("could not read '%s'."), rebase_path_todo_backup());
+ goto out;
+ }
+
+ res = todo_list_parse_insn_buffer(r, old_todo.buf.buf, &old_todo);
+ if (!res)
+ res = todo_list_parse_insn_buffer(r, new_todo.buf.buf, &new_todo);
+ if (!res)
+ res = todo_list_check(&old_todo, &new_todo);
+ if (res)
+ fprintf(stderr, _(edit_todo_list_advice));
+out:
+ todo_list_release(&old_todo);
+ todo_list_release(&new_todo);
+
+ return res;
+}
diff --git a/rebase-interactive.h b/rebase-interactive.h
index 44dbb06311..5f41bf5a28 100644
--- a/rebase-interactive.h
+++ b/rebase-interactive.h
@@ -13,4 +13,6 @@ int edit_todo_list(struct repository *r, struct todo_list *todo_list,
const char *shortonto, unsigned flags);
int todo_list_check(struct todo_list *old_todo, struct todo_list *new_todo);
+int check_todo_list_from_file(struct repository *r);
+
#endif
diff --git a/sequencer.c b/sequencer.c
index ec0b793fc5..181bb35f5f 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -4992,35 +4992,6 @@ N_("You can fix this with 'git rebase --edit-todo' "
"Or you can abort the rebase with 'git rebase"
" --abort'.\n");
-int check_todo_list_from_file(struct repository *r)
-{
- struct todo_list old_todo = TODO_LIST_INIT, new_todo = TODO_LIST_INIT;
- int res = 0;
-
- if (strbuf_read_file_or_whine(&new_todo.buf, rebase_path_todo()) < 0) {
- res = -1;
- goto out;
- }
-
- if (strbuf_read_file_or_whine(&old_todo.buf, rebase_path_todo_backup()) < 0) {
- res = -1;
- goto out;
- }
-
- res = todo_list_parse_insn_buffer(r, old_todo.buf.buf, &old_todo);
- if (!res)
- res = todo_list_parse_insn_buffer(r, new_todo.buf.buf, &new_todo);
- if (!res)
- res = todo_list_check(&old_todo, &new_todo);
- if (res)
- fprintf(stderr, _(edit_todo_list_advice));
-out:
- todo_list_release(&old_todo);
- todo_list_release(&new_todo);
-
- return res;
-}
-
/* skip picking commits whose parents are unchanged */
static int skip_unnecessary_picks(struct repository *r,
struct todo_list *todo_list,
diff --git a/sequencer.h b/sequencer.h
index 574260f621..75ddc5db3a 100644
--- a/sequencer.h
+++ b/sequencer.h
@@ -155,7 +155,6 @@ int sequencer_make_script(struct repository *r, struct strbuf *out, int argc,
void todo_list_add_exec_commands(struct todo_list *todo_list,
struct string_list *commands);
-int check_todo_list_from_file(struct repository *r);
int complete_action(struct repository *r, struct replay_opts *opts, unsigned flags,
const char *shortrevisions, const char *onto_name,
struct commit *onto, const char *orig_head, struct string_list *commands,
--
2.24.1
^ permalink raw reply related
* [PATCH v4 2/2] rebase-interactive: warn if commit is dropped with `rebase --edit-todo'
From: Alban Gruin @ 2020-01-11 17:39 UTC (permalink / raw)
To: git; +Cc: Johannes Schindelin, Phillip Wood, Junio C Hamano, Alban Gruin
In-Reply-To: <20200111173917.15690-1-alban.gruin@gmail.com>
When set to "warn" or "error", `rebase.missingCommitsCheck' would make
`rebase -i' warn if the user removed commits from the todo list to
prevent mistakes. Unfortunately, `rebase --edit-todo' and `rebase
--continue' don't take it into account.
This adds the ability for `rebase --edit-todo' and `rebase --continue'
to check if commits were dropped by the user. As both edit_todo_list()
and complete_action() parse the todo list and check for dropped commits,
the code doing so in the latter is removed to reduce duplication.
`edit_todo_list_advice' is removed from sequencer.c as it is no longer
used there.
This changes when a backup of the todo list is made. Until now, it
was saved only once, before the initial edit. Now, it is also made if
after the user edited the list, if it has no errors or if no commits
were dropped and `rebase.missingCommitsCheck' is set. Thus, the
backup should be error-free. Without this, sequencer_continue()
(`rebase --continue') could only compare the current todo list against
the original, unedited list. Before this change, this file was only
used by edit_todo_list() and `rebase -p' to create the backup before
the initial edit, and check_todo_list_from_file(), only used by
`rebase -p' to check for dropped commits after its own initial edit.
If the edited list has an error, a file, `dropped', is created to
report the issue. Otherwise, it is deleted. Usually, the edited list
is compared against the list before editing, but if this file exists,
it will be compared to the backup. Also, if the file exists,
sequencer_continue() checks the list for dropped commits. If the
check was performed every time, it would fail when resuming a rebase
after resolving a conflict, as the backup will contain commits that
were picked, but they will not be in the new list. It's safe to
ignore this check if `dropped' does not exist, because that means that
no errors were found at the last edition, so any missing commits here
have already been picked.
Four tests are added to t3404. The tests for
`rebase.missingCommitsCheck = warn' and `rebase.missingCommitsCheck =
error' have a similar structure. First, we start a rebase with an
incorrect command on the first line. Then, we edit the todo list,
removing the first and the last lines. This demonstrates that
`--edit-todo' notices dropped commits, but not when the command is
incorrect. Then, we restore the original todo list, and edit it to
remove the last line. This demonstrates that if we add a commit after
the initial edit, then remove it, `--edit-todo' will notice that it
has been dropped. Then, the actual rebase takes place. In the third
test, it is also checked that `--continue' will refuse to resume the
rebase if commits were dropped. The fourth test checks that no errors
are raised when resuming a rebase after resolving a conflict.
Signed-off-by: Alban Gruin <alban.gruin@gmail.com>
---
Notes:
I don't think the way I create `expect.3' files in "rebase --edit-todo
respects rebase.missingCommitsCheck = warning" & "... = error" is the
best practice. Perhaps I should create a new file from scratch instead
of calling `head' and `tail' successively?
rebase-interactive.c | 58 ++++++++++++++----
rebase-interactive.h | 3 +
sequencer.c | 22 +++----
sequencer.h | 1 +
t/t3404-rebase-interactive.sh | 109 ++++++++++++++++++++++++++++++++++
5 files changed, 171 insertions(+), 22 deletions(-)
diff --git a/rebase-interactive.c b/rebase-interactive.c
index ad5dd49c31..36b08a55ef 100644
--- a/rebase-interactive.c
+++ b/rebase-interactive.c
@@ -5,6 +5,7 @@
#include "strbuf.h"
#include "commit-slab.h"
#include "config.h"
+#include "dir.h"
static const char edit_todo_list_advice[] =
N_("You can fix this with 'git rebase --edit-todo' "
@@ -97,22 +98,25 @@ int edit_todo_list(struct repository *r, struct todo_list *todo_list,
struct todo_list *new_todo, const char *shortrevisions,
const char *shortonto, unsigned flags)
{
- const char *todo_file = rebase_path_todo();
+ const char *todo_file = rebase_path_todo(),
+ *todo_backup = rebase_path_todo_backup();
unsigned initial = shortrevisions && shortonto;
+ int incorrect = 1;
/* If the user is editing the todo list, we first try to parse
* it. If there is an error, we do not return, because the user
* might want to fix it in the first place. */
if (!initial)
- todo_list_parse_insn_buffer(r, todo_list->buf.buf, todo_list);
+ incorrect = todo_list_parse_insn_buffer(r, todo_list->buf.buf, todo_list);
+
+ incorrect |= file_exists(rebase_path_dropped());
if (todo_list_write_to_file(r, todo_list, todo_file, shortrevisions, shortonto,
-1, flags | TODO_LIST_SHORTEN_IDS | TODO_LIST_APPEND_TODO_HELP))
return error_errno(_("could not write '%s'"), todo_file);
- if (initial && copy_file(rebase_path_todo_backup(), todo_file, 0666))
- return error(_("could not copy '%s' to '%s'."), todo_file,
- rebase_path_todo_backup());
+ if (initial && copy_file(todo_backup, todo_file, 0666))
+ return error(_("could not copy '%s' to '%s'."), todo_file, todo_backup);
if (launch_sequence_editor(todo_file, &new_todo->buf, NULL))
return -2;
@@ -121,10 +125,26 @@ int edit_todo_list(struct repository *r, struct todo_list *todo_list,
if (initial && new_todo->buf.len == 0)
return -3;
- /* For the initial edit, the todo list gets parsed in
- * complete_action(). */
- if (!initial)
- return todo_list_parse_insn_buffer(r, new_todo->buf.buf, new_todo);
+ if (todo_list_parse_insn_buffer(r, new_todo->buf.buf, new_todo)) {
+ fprintf(stderr, _(edit_todo_list_advice));
+ return -4;
+ }
+
+ if (incorrect) {
+ if (todo_list_check_against_backup(r, new_todo)) {
+ write_file(rebase_path_dropped(), "");
+ return -4;
+ }
+
+ if (incorrect > 0)
+ unlink(rebase_path_dropped());
+ } else if (todo_list_check(todo_list, new_todo)) {
+ write_file(rebase_path_dropped(), "");
+ return -4;
+ } else {
+ todo_list_write_to_file(r, todo_list, todo_backup, shortrevisions, shortonto,
+ -1, flags | TODO_LIST_SHORTEN_IDS | TODO_LIST_APPEND_TODO_HELP);
+ }
return 0;
}
@@ -189,11 +209,27 @@ int todo_list_check(struct todo_list *old_todo, struct todo_list *new_todo)
"the level of warnings.\n"
"The possible behaviours are: ignore, warn, error.\n\n"));
+ fprintf(stderr, _(edit_todo_list_advice));
+
leave_check:
clear_commit_seen(&commit_seen);
return res;
}
+int todo_list_check_against_backup(struct repository *r, struct todo_list *todo_list)
+{
+ struct todo_list backup = TODO_LIST_INIT;
+ int res = 0;
+
+ if (strbuf_read_file(&backup.buf, rebase_path_todo_backup(), 0) > 0) {
+ todo_list_parse_insn_buffer(r, backup.buf.buf, &backup);
+ res = todo_list_check(&backup, todo_list);
+ }
+
+ todo_list_release(&backup);
+ return res;
+}
+
int check_todo_list_from_file(struct repository *r)
{
struct todo_list old_todo = TODO_LIST_INIT, new_todo = TODO_LIST_INIT;
@@ -212,10 +248,10 @@ int check_todo_list_from_file(struct repository *r)
res = todo_list_parse_insn_buffer(r, old_todo.buf.buf, &old_todo);
if (!res)
res = todo_list_parse_insn_buffer(r, new_todo.buf.buf, &new_todo);
- if (!res)
- res = todo_list_check(&old_todo, &new_todo);
if (res)
fprintf(stderr, _(edit_todo_list_advice));
+ if (!res)
+ res = todo_list_check(&old_todo, &new_todo);
out:
todo_list_release(&old_todo);
todo_list_release(&new_todo);
diff --git a/rebase-interactive.h b/rebase-interactive.h
index 5f41bf5a28..4af0c1fcc7 100644
--- a/rebase-interactive.h
+++ b/rebase-interactive.h
@@ -11,7 +11,10 @@ void append_todo_help(unsigned keep_empty, int command_count,
int edit_todo_list(struct repository *r, struct todo_list *todo_list,
struct todo_list *new_todo, const char *shortrevisions,
const char *shortonto, unsigned flags);
+
int todo_list_check(struct todo_list *old_todo, struct todo_list *new_todo);
+int todo_list_check_against_backup(struct repository *r,
+ struct todo_list *todo_list);
int check_todo_list_from_file(struct repository *r);
diff --git a/sequencer.c b/sequencer.c
index 181bb35f5f..2ff18943fb 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -57,6 +57,8 @@ static GIT_PATH_FUNC(rebase_path, "rebase-merge")
GIT_PATH_FUNC(rebase_path_todo, "rebase-merge/git-rebase-todo")
GIT_PATH_FUNC(rebase_path_todo_backup, "rebase-merge/git-rebase-todo.backup")
+GIT_PATH_FUNC(rebase_path_dropped, "rebase-merge/dropped")
+
/*
* The rebase command lines that have already been processed. A line
* is moved here when it is first handled, before any associated user
@@ -4273,6 +4275,14 @@ int sequencer_continue(struct repository *r, struct replay_opts *opts)
if (is_rebase_i(opts)) {
if ((res = read_populate_todo(r, &todo_list, opts)))
goto release_todo_list;
+
+ if (file_exists(rebase_path_dropped())) {
+ if ((res = todo_list_check_against_backup(r, &todo_list)))
+ goto release_todo_list;
+
+ unlink(rebase_path_dropped());
+ }
+
if (commit_staged_changes(r, opts, &todo_list))
return -1;
} else if (!file_exists(get_todo_path(opts)))
@@ -4986,12 +4996,6 @@ int todo_list_write_to_file(struct repository *r, struct todo_list *todo_list,
return res;
}
-static const char edit_todo_list_advice[] =
-N_("You can fix this with 'git rebase --edit-todo' "
-"and then run 'git rebase --continue'.\n"
-"Or you can abort the rebase with 'git rebase"
-" --abort'.\n");
-
/* skip picking commits whose parents are unchanged */
static int skip_unnecessary_picks(struct repository *r,
struct todo_list *todo_list,
@@ -5089,11 +5093,7 @@ int complete_action(struct repository *r, struct replay_opts *opts, unsigned fla
todo_list_release(&new_todo);
return error(_("nothing to do"));
- }
-
- if (todo_list_parse_insn_buffer(r, new_todo.buf.buf, &new_todo) ||
- todo_list_check(todo_list, &new_todo)) {
- fprintf(stderr, _(edit_todo_list_advice));
+ } else if (res == -4) {
checkout_onto(r, opts, onto_name, &onto->object.oid, orig_head);
todo_list_release(&new_todo);
diff --git a/sequencer.h b/sequencer.h
index 75ddc5db3a..00debf5107 100644
--- a/sequencer.h
+++ b/sequencer.h
@@ -11,6 +11,7 @@ const char *git_path_commit_editmsg(void);
const char *git_path_seq_dir(void);
const char *rebase_path_todo(void);
const char *rebase_path_todo_backup(void);
+const char *rebase_path_dropped(void);
#define APPEND_SIGNOFF_DEDUP (1u << 0)
diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index 29a35840ed..f5c3da33bf 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -1343,6 +1343,115 @@ test_expect_success 'rebase -i respects rebase.missingCommitsCheck = error' '
test B = $(git cat-file commit HEAD^ | sed -ne \$p)
'
+test_expect_success 'rebase --edit-todo respects rebase.missingCommitsCheck = ignore' '
+ test_config rebase.missingCommitsCheck ignore &&
+ rebase_setup_and_clean missing-commit &&
+ (
+ set_fake_editor &&
+ FAKE_LINES="break 1 2 3 4 5" git rebase -i --root &&
+ FAKE_LINES="1 2 3 4" git rebase --edit-todo &&
+ git rebase --continue 2>actual
+ ) &&
+ test D = $(git cat-file commit HEAD | sed -ne \$p) &&
+ test_i18ngrep \
+ "Successfully rebased and updated refs/heads/missing-commit" \
+ actual
+'
+
+test_expect_success 'rebase --edit-todo respects rebase.missingCommitsCheck = warn' '
+ cat >expect <<-EOF &&
+ error: invalid line 1: badcmd $(git rev-list --pretty=oneline --abbrev-commit -1 master~4)
+ Warning: some commits may have been dropped accidentally.
+ Dropped commits (newer to older):
+ - $(git rev-list --pretty=oneline --abbrev-commit -1 master)
+ - $(git rev-list --pretty=oneline --abbrev-commit -1 master~4)
+ To avoid this message, use "drop" to explicitly remove a commit.
+ EOF
+ head -n4 expect >expect.2 &&
+ tail -n1 expect >>expect.2 &&
+ tail -n4 expect.2 >expect.3 &&
+ test_config rebase.missingCommitsCheck warn &&
+ rebase_setup_and_clean missing-commit &&
+ (
+ set_fake_editor &&
+ test_must_fail env FAKE_LINES="bad 1 2 3 4 5" \
+ git rebase -i --root &&
+ cp .git/rebase-merge/git-rebase-todo.backup orig &&
+ FAKE_LINES="2 3 4" git rebase --edit-todo 2>actual.2 &&
+ head -n6 actual.2 >actual &&
+ test_i18ncmp expect actual &&
+ cp orig .git/rebase-merge/git-rebase-todo &&
+ FAKE_LINES="1 2 3 4" git rebase --edit-todo 2>actual.2 &&
+ head -n4 actual.2 >actual &&
+ test_i18ncmp expect.3 actual &&
+ git rebase --continue 2>actual
+ ) &&
+ test D = $(git cat-file commit HEAD | sed -ne \$p) &&
+ test_i18ngrep \
+ "Successfully rebased and updated refs/heads/missing-commit" \
+ actual
+'
+
+test_expect_success 'rebase --edit-todo respects rebase.missingCommitsCheck = error' '
+ cat >expect <<-EOF &&
+ error: invalid line 1: badcmd $(git rev-list --pretty=oneline --abbrev-commit -1 master~4)
+ Warning: some commits may have been dropped accidentally.
+ Dropped commits (newer to older):
+ - $(git rev-list --pretty=oneline --abbrev-commit -1 master)
+ - $(git rev-list --pretty=oneline --abbrev-commit -1 master~4)
+ To avoid this message, use "drop" to explicitly remove a commit.
+
+ Use '\''git config rebase.missingCommitsCheck'\'' to change the level of warnings.
+ The possible behaviours are: ignore, warn, error.
+
+ You can fix this with '\''git rebase --edit-todo'\'' and then run '\''git rebase --continue'\''.
+ Or you can abort the rebase with '\''git rebase --abort'\''.
+ EOF
+ tail -n11 expect >expect.2 &&
+ head -n3 expect.2 >expect.3 &&
+ tail -n7 expect.2 >>expect.3 &&
+ test_config rebase.missingCommitsCheck error &&
+ rebase_setup_and_clean missing-commit &&
+ (
+ set_fake_editor &&
+ test_must_fail env FAKE_LINES="bad 1 2 3 4 5" \
+ git rebase -i --root &&
+ cp .git/rebase-merge/git-rebase-todo.backup orig &&
+ test_must_fail env FAKE_LINES="2 3 4" \
+ git rebase --edit-todo 2>actual &&
+ test_i18ncmp expect actual &&
+ test_must_fail git rebase --continue 2>actual &&
+ test_i18ncmp expect.2 actual &&
+ test_must_fail git rebase --edit-todo &&
+ cp orig .git/rebase-merge/git-rebase-todo &&
+ test_must_fail env FAKE_LINES="1 2 3 4" \
+ git rebase --edit-todo 2>actual &&
+ test_i18ncmp expect.3 actual &&
+ test_must_fail git rebase --continue 2>actual &&
+ test_i18ncmp expect.3 actual &&
+ cp orig .git/rebase-merge/git-rebase-todo &&
+ FAKE_LINES="1 2 3 4 drop 5" git rebase --edit-todo &&
+ git rebase --continue 2>actual
+ ) &&
+ test D = $(git cat-file commit HEAD | sed -ne \$p) &&
+ test_i18ngrep \
+ "Successfully rebased and updated refs/heads/missing-commit" \
+ actual
+'
+
+test_expect_success 'rebase.missingCommitsCheck = error after resolving conflicts' '
+ test_config rebase.missingCommitsCheck error &&
+ (
+ set_fake_editor &&
+ FAKE_LINES="drop 1 break 2 3 4" git rebase -i A E
+ ) &&
+ git rebase --edit-todo &&
+ test_must_fail git rebase --continue &&
+ echo x >file1 &&
+ git add file1 &&
+ git rebase --continue
+'
+
test_expect_success 'respects rebase.abbreviateCommands with fixup, squash and exec' '
rebase_setup_and_clean abbrevcmd &&
test_commit "first" file1.txt "first line" first &&
--
2.24.1
^ permalink raw reply related
* [PATCH v3] branch: let '--edit-description' default to rebased branch during rebase
From: marcandre.lureau @ 2020-01-11 17:57 UTC (permalink / raw)
To: git; +Cc: szeder.dev, sunshine, Marc-André Lureau
From: Marc-André Lureau <marcandre.lureau@redhat.com>
Defaulting to editing the description of the rebased branch without an
explicit branchname argument would be useful. Even the git bash prompt
shows the name of the rebased branch, and then
~/src/git (mybranch|REBASE-i 1/2)$ git branch --edit-description
fatal: Cannot give description to detached HEAD
looks quite unhelpful.
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
Changed in v3:
- fix memory leaks on error paths
- remove test_done left-over
- style fixes
builtin/branch.c | 40 +++++++++++++++++++++++++++++-----------
t/t3200-branch.sh | 18 ++++++++++++++++++
2 files changed, 47 insertions(+), 11 deletions(-)
diff --git a/builtin/branch.c b/builtin/branch.c
index d8297f80ff..033a045d40 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -745,33 +745,51 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
string_list_clear(&output, 0);
return 0;
} else if (edit_description) {
- const char *branch_name;
+ char *branch_name = NULL;
struct strbuf branch_ref = STRBUF_INIT;
if (!argc) {
- if (filter.detached)
- die(_("Cannot give description to detached HEAD"));
- branch_name = head;
+ if (!filter.detached)
+ branch_name = xstrdup(head);
+ else {
+ struct wt_status_state state;
+
+ memset(&state, 0, sizeof(state));
+ if (wt_status_check_rebase(NULL, &state))
+ branch_name = state.branch;
+ if (!branch_name)
+ die(_("Cannot give description to detached HEAD"));
+ free(state.onto);
+ }
} else if (argc == 1)
- branch_name = argv[0];
+ branch_name = xstrdup(argv[0]);
else
die(_("cannot edit description of more than one branch"));
strbuf_addf(&branch_ref, "refs/heads/%s", branch_name);
if (!ref_exists(branch_ref.buf)) {
- strbuf_release(&branch_ref);
+ int ret;
if (!argc)
- return error(_("No commit on branch '%s' yet."),
- branch_name);
+ ret = error(_("No commit on branch '%s' yet."),
+ branch_name);
else
- return error(_("No branch named '%s'."),
- branch_name);
+ ret = error(_("No branch named '%s'."),
+ branch_name);
+
+ strbuf_release(&branch_ref);
+ free(branch_name);
+ return ret;
+
}
strbuf_release(&branch_ref);
- if (edit_branch_description(branch_name))
+ if (edit_branch_description(branch_name)) {
+ free(branch_name);
return 1;
+ }
+
+ free(branch_name);
} else if (copy) {
if (!argc)
die(_("branch name required"));
diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
index 411a70b0ce..d2bdaf324d 100755
--- a/t/t3200-branch.sh
+++ b/t/t3200-branch.sh
@@ -1260,6 +1260,24 @@ test_expect_success 'use --edit-description' '
test_cmp expect EDITOR_OUTPUT
'
+test_expect_success 'use --edit-description during rebase' '
+ write_script editor <<-\EOF &&
+ echo "Rebase contents" >"$1"
+ EOF
+ (
+ set_fake_editor &&
+ FAKE_LINES="break 1" git rebase -i HEAD^ &&
+ EDITOR=./editor git branch --edit-description &&
+ git rebase --continue
+ ) &&
+ write_script editor <<-\EOF &&
+ git stripspace -s <"$1" >"EDITOR_OUTPUT"
+ EOF
+ EDITOR=./editor git branch --edit-description &&
+ echo "Rebase contents" >expect &&
+ test_cmp expect EDITOR_OUTPUT
+'
+
test_expect_success 'detect typo in branch name when using --edit-description' '
write_script editor <<-\EOF &&
echo "New contents" >"$1"
base-commit: 7a6a90c6ec48fc78c83d7090d6c1b95d8f3739c0
--
2.25.0.rc2.2.g5aece98438
^ permalink raw reply related
* Re: [PATCH 9/9] commit-graph: add GIT_TEST_COMMIT_GRAPH_BLOOM_FILTERS test flag
From: Jakub Narebski @ 2020-01-11 19:56 UTC (permalink / raw)
To: Garima Singh via GitGitGadget
Cc: git, stolee, szeder.dev, jonathantanmy, jeffhost, me, peff,
Junio C Hamano, Garima Singh
In-Reply-To: <e1c315d0a766af147eb4ead41a172f724e90cc34.1576879520.git.gitgitgadget@gmail.com>
"Garima Singh via GitGitGadget" <gitgitgadget@gmail.com> writes:
> From: Garima Singh <garima.singh@microsoft.com>
>
> Add GIT_TEST_COMMIT_GRAPH_BLOOM_FILTERS test flag to the test setup suite in
> order to toggle writing bloom filters when running any of the git tests. If set
> to true, we will compute and write bloom filters every time a test calls
> `git commit-graph write`.
OK, so it works in addition to GIT_TEST_COMMIT_GRAPH.
>
> The test suite passes when GIT_TEST_COMMIT_GRAPH and
> GIT_COMMIT_GRAPH_BLOOM_FILTERS are enabled.
Good. Very good.
No errors found by Continuous Integration setup either?
>
> Helped-by: Derrick Stolee <dstolee@microsoft.com>
> Signed-off-by: Garima Singh <garima.singh@microsoft.com>
> ---
> builtin/commit-graph.c | 2 +-
> ci/run-build-and-tests.sh | 1 +
> commit-graph.h | 1 +
> t/README | 3 +++
> t/t4216-log-bloom.sh | 3 +++
> t/t5318-commit-graph.sh | 2 ++
> t/t5324-split-commit-graph.sh | 1 +
> t/t5325-commit-graph-bloom.sh | 3 +++
> 8 files changed, 15 insertions(+), 1 deletion(-)
>
> diff --git a/builtin/commit-graph.c b/builtin/commit-graph.c
> index 9bd1e11161..97167959b2 100644
> --- a/builtin/commit-graph.c
> +++ b/builtin/commit-graph.c
> @@ -146,7 +146,7 @@ static int graph_write(int argc, const char **argv)
> flags |= COMMIT_GRAPH_WRITE_SPLIT;
> if (opts.progress)
> flags |= COMMIT_GRAPH_WRITE_PROGRESS;
> - if (opts.enable_bloom_filters)
> + if (opts.enable_bloom_filters || git_env_bool(GIT_TEST_COMMIT_GRAPH_BLOOM_FILTERS, 0))
> flags |= COMMIT_GRAPH_WRITE_BLOOM_FILTERS;
>
Very minor nitpick: not to make this line long, I would break it at the
boolean operator, that is write
- if (opts.enable_bloom_filters)
+ if (opts.enable_bloom_filters ||
+ git_env_bool(GIT_TEST_COMMIT_GRAPH_BLOOM_FILTERS, 0))
flags |= COMMIT_GRAPH_WRITE_BLOOM_FILTERS;
I agree that this is a good place to put this check, by pretending that
`--changed-paths` option was given on command line. Looks good.
> read_replace_refs = 0;
> diff --git a/ci/run-build-and-tests.sh b/ci/run-build-and-tests.sh
> index ff0ef7f08e..19d0846d34 100755
> --- a/ci/run-build-and-tests.sh
> +++ b/ci/run-build-and-tests.sh
> @@ -19,6 +19,7 @@ linux-gcc)
> export GIT_TEST_OE_SIZE=10
> export GIT_TEST_OE_DELTA_SIZE=5
> export GIT_TEST_COMMIT_GRAPH=1
> + export GIT_TEST_COMMIT_GRAPH_BLOOM_FILTERS=1
> export GIT_TEST_MULTI_PACK_INDEX=1
> make test
> ;;
All right, adding this to CI would certainly exercise this feature.
> diff --git a/commit-graph.h b/commit-graph.h
> index 2202ad91ae..d914e6abf1 100644
> --- a/commit-graph.h
> +++ b/commit-graph.h
> @@ -8,6 +8,7 @@
>
> #define GIT_TEST_COMMIT_GRAPH "GIT_TEST_COMMIT_GRAPH"
> #define GIT_TEST_COMMIT_GRAPH_DIE_ON_LOAD "GIT_TEST_COMMIT_GRAPH_DIE_ON_LOAD"
> +#define GIT_TEST_COMMIT_GRAPH_BLOOM_FILTERS "GIT_TEST_COMMIT_GRAPH_BLOOM_FILTERS"
>
All right (the ordering is a mater of taste).
> struct commit;
> struct bloom_filter_settings;
> diff --git a/t/README b/t/README
> index caa125ba9a..399b190437 100644
> --- a/t/README
> +++ b/t/README
> @@ -378,6 +378,9 @@ GIT_TEST_COMMIT_GRAPH=<boolean>, when true, forces the commit-graph to
> be written after every 'git commit' command, and overrides the
> 'core.commitGraph' setting to true.
>
> +GIT_TEST_COMMIT_GRAPH_BLOOM_FILTERS=<boolean>, when true, forces commit-graph
> +write to compute and write bloom filters for every 'git commit-graph write'
> +
Thanks for documenting this.
Missing full stop '.' at the end of the sentence. (Minor nit).
We might want to add ", as if '--changed-paths' option was given.", but
it is not strictly necessary.
> GIT_TEST_FSMONITOR=$PWD/t7519/fsmonitor-all exercises the fsmonitor
> code path for utilizing a file system monitor to speed up detecting
> new or changed files.
> diff --git a/t/t4216-log-bloom.sh b/t/t4216-log-bloom.sh
> index d42f077998..0e092b387c 100755
> --- a/t/t4216-log-bloom.sh
> +++ b/t/t4216-log-bloom.sh
> @@ -3,6 +3,9 @@
> test_description='git log for a path with bloom filters'
> . ./test-lib.sh
>
> +GIT_TEST_COMMIT_GRAPH=0
> +GIT_TEST_COMMIT_GRAPH_BLOOM_FILTERS=0
> +
OK, neither of those setting increase the coverage for this test.
On the other hand they won't make tests fail (or at least they
shouldn't, I think). The t5318-commit-graph.sh doesn't include
GIT_TEST_COMMIT_GRAPH=0, after all.
> test_expect_success 'setup repo' '
> git init &&
> git config core.commitGraph true &&
> diff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh
> index 3f03de6018..613228bb12 100755
> --- a/t/t5318-commit-graph.sh
> +++ b/t/t5318-commit-graph.sh
> @@ -3,6 +3,8 @@
> test_description='commit graph'
> . ./test-lib.sh
>
> +GIT_TEST_COMMIT_GRAPH_BLOOM_FILTERS=0
> +
All right, adding Bloom filters to commit-graph file would make test
cases utilizing graph_read_expect() fail.
> test_expect_success 'setup full repo' '
> mkdir full &&
> cd "$TRASH_DIRECTORY/full" &&
> diff --git a/t/t5324-split-commit-graph.sh b/t/t5324-split-commit-graph.sh
> index c24823431f..181ca7e0cb 100755
> --- a/t/t5324-split-commit-graph.sh
> +++ b/t/t5324-split-commit-graph.sh
> @@ -4,6 +4,7 @@ test_description='split commit graph'
> . ./test-lib.sh
>
> GIT_TEST_COMMIT_GRAPH=0
> +GIT_TEST_COMMIT_GRAPH_BLOOM_FILTERS=0
All right, same here: adding Bloom filters to commit-graph would make
test cases utilizing graph_read_expect() fail.
Sidenote: Here without GIT_TEST_COMMIT_GRAPH=0 tests that rely on
precise timing of writing commit-graph to create split commit-graph
would fail.
>
> test_expect_success 'setup repo' '
> git init &&
> diff --git a/t/t5325-commit-graph-bloom.sh b/t/t5325-commit-graph-bloom.sh
> index d7ef0e7fb3..a9c9e9fef6 100755
> --- a/t/t5325-commit-graph-bloom.sh
> +++ b/t/t5325-commit-graph-bloom.sh
> @@ -3,6 +3,9 @@
> test_description='commit graph with bloom filters'
> . ./test-lib.sh
>
> +GIT_TEST_COMMIT_GRAPH=0
> +GIT_TEST_COMMIT_GRAPH_BLOOM_FILTERS=0
> +
This test also includes some split commit-graph test cases, so the above
is necessary.
All right.
> test_expect_success 'setup repo' '
> git init &&
> git config core.commitGraph true &&
Best,
--
Jakub Narębski
^ permalink raw reply
* [PATCH v3 0/2] Un-regress rev-list --exclude-promisor-objects
From: Jonathan Tan @ 2020-01-11 22:34 UTC (permalink / raw)
To: git; +Cc: Jonathan Tan, gitster, jrnieder
In-Reply-To: <20191228003430.241283-1-jonathantanmy@google.com>
I took another look at this and tried to simplify things. The main
points were:
- there is a real bug
- it can be fixed by relying on get_reference() more
- but there was some discussion about what get_reference() does, so
I added some documentation first
Hopefully those main points were adequately conveyed in the new commit
messages, and I didn't oversimplify things.
There was some discussion about whether get_reference() should treat
corrupt objects as missing. After some thought, I think the best
argument for doing so is that this has been its behavior for some time,
and have wrote that in the first commit.
Jonathan Tan (2):
revision: document get_reference()
revision: un-regress --exclude-promisor-objects
revision.c | 12 +++++++++++-
t/t0410-partial-clone.sh | 12 +++---------
2 files changed, 14 insertions(+), 10 deletions(-)
--
2.25.0.rc1.283.g88dfdc4193-goog
^ permalink raw reply
* [PATCH v3 1/2] revision: document get_reference()
From: Jonathan Tan @ 2020-01-11 22:34 UTC (permalink / raw)
To: git; +Cc: Jonathan Tan, gitster, jrnieder
In-Reply-To: <cover.1578781770.git.jonathantanmy@google.com>
In particular, document the behavior when the object is corrupt. The
existing behavior when parse_object() encounters a hash mismatch has
been there since cc243c3ceb ("show: --ignore-missing", 2011-05-19), and
the existing behavior when the code disagrees on whether an object is a
commit has been there since ec0c5798ee ("revision: use commit graph in
get_reference()", 2018-12-28).
---
revision.c | 10 ++++++++++
1 file changed, 10 insertions(+)
diff --git a/revision.c b/revision.c
index 8136929e23..91ca194388 100644
--- a/revision.c
+++ b/revision.c
@@ -355,6 +355,16 @@ void add_head_to_pending(struct rev_info *revs)
add_pending_object(revs, obj, "HEAD");
}
+/*
+ * Returns the object corresponding to "oid" and sets the given flags on
+ * it.
+ *
+ * If that object is missing or corrupt, this function returns NULL if
+ * "revs" permits it (that is, if revs->ignore_missing is true or if
+ * revs->exclude_promisor_objects is true and the object is a promisor
+ * object), and dies otherwise. Note that corrupt objects are treated
+ * like missing objects, to preserve existing behavior.
+ */
static struct object *get_reference(struct rev_info *revs, const char *name,
const struct object_id *oid,
unsigned int flags)
--
2.25.0.rc1.283.g88dfdc4193-goog
^ permalink raw reply related
* [PATCH v3 2/2] revision: un-regress --exclude-promisor-objects
From: Jonathan Tan @ 2020-01-11 22:34 UTC (permalink / raw)
To: git; +Cc: Jonathan Tan, gitster, jrnieder
In-Reply-To: <cover.1578781770.git.jonathantanmy@google.com>
Before commit 4cf67869b2 ("list-objects.c: don't segfault for missing
cmdline objects", 2018-12-06),
git rev-list --exclude-promisor-objects $A_MISSING_PROMISOR_OBJECT
succeeds. But after that commit, this invocation produces a non-zero
result.
Restore this functionality: since get_reference() already does what we
need, we can just use its return value; skip the arg if the return value
is NULL, and use it otherwise (if the arg is invalid, get_reference()
already dies). With this commit, --exclude-promisor-objects treats both
promisor objects passed through the CLI and promisor objects found
through traversal in the same say: it excludes them, so it does not
matter whether they're missing or not.
---
revision.c | 2 +-
t/t0410-partial-clone.sh | 12 +++---------
2 files changed, 4 insertions(+), 10 deletions(-)
diff --git a/revision.c b/revision.c
index 91ca194388..0659a09b02 100644
--- a/revision.c
+++ b/revision.c
@@ -1917,7 +1917,7 @@ int handle_revision_arg(const char *arg_, struct rev_info *revs, int flags, unsi
verify_non_filename(revs->prefix, arg);
object = get_reference(revs, arg, &oid, flags ^ local_flags);
if (!object)
- return revs->ignore_missing ? 0 : -1;
+ return 0;
add_rev_cmdline(revs, object, arg_, REV_CMD_REV, flags ^ local_flags);
add_pending_object_with_path(revs, object, arg, oc.mode, oc.path);
free(oc.path);
diff --git a/t/t0410-partial-clone.sh b/t/t0410-partial-clone.sh
index a3988bd4b8..b251985e82 100755
--- a/t/t0410-partial-clone.sh
+++ b/t/t0410-partial-clone.sh
@@ -397,7 +397,7 @@ test_expect_success 'rev-list stops traversal at promisor commit, tree, and blob
grep $(git -C repo rev-parse bar) out # sanity check that some walking was done
'
-test_expect_success 'rev-list dies for missing objects on cmd line' '
+test_expect_success 'rev-list accepts missing and promised objects on command line ' '
rm -rf repo &&
test_create_repo repo &&
test_commit -C repo foo &&
@@ -416,15 +416,9 @@ test_expect_success 'rev-list dies for missing objects on cmd line' '
git -C repo config extensions.partialclone "arbitrary string" &&
for OBJ in "$COMMIT" "$TREE" "$BLOB"; do
- test_must_fail git -C repo rev-list --objects \
+ git -C repo rev-list --objects \
--exclude-promisor-objects "$OBJ" &&
- test_must_fail git -C repo rev-list --objects-edge-aggressive \
- --exclude-promisor-objects "$OBJ" &&
-
- # Do not die or crash when --ignore-missing is passed.
- git -C repo rev-list --ignore-missing --objects \
- --exclude-promisor-objects "$OBJ" &&
- git -C repo rev-list --ignore-missing --objects-edge-aggressive \
+ git -C repo rev-list --objects-edge-aggressive \
--exclude-promisor-objects "$OBJ"
done
'
--
2.25.0.rc1.283.g88dfdc4193-goog
^ permalink raw reply related
* Re: [PATCH] branch: let '--edit-description' default to rebased branch during rebase
From: Eric Sunshine @ 2020-01-12 1:27 UTC (permalink / raw)
To: Marc-André Lureau; +Cc: Git List, SZEDER Gábor
In-Reply-To: <CAJ+F1CKW3NACgPdPbmAzYGVwR4iO3r+LCNq+g5st0gcz4X+fzA@mail.gmail.com>
On Sat, Jan 11, 2020 at 9:55 AM Marc-André Lureau
<marcandre.lureau@gmail.com> wrote:
> On Sat, Jan 11, 2020 at 5:28 PM Eric Sunshine <sunshine@sunshineco.com> wrote:
> > On Sat, Jan 11, 2020 at 7:36 AM <marcandre.lureau@redhat.com> wrote:
> > > + if (wt_status_check_rebase(NULL, &state)) {
> > > + branch_name = state.branch;
> > > + }
Taking a deeper look at the code, I'm wondering it would make more
sense to call wt_status_get_state(), which handles 'rebase' and
'bisect'. Is there a reason that you limited this check to only
'rebase'?
> > > if (edit_branch_description(branch_name))
> > > return 1;
> > > +
> > > + free(branch_name);
> >
> > That `return 1` just above this free() is leaking 'branch_name', isn't it?
>
> right, let's fix that too
Looking at the code itself (rather than consulting only the patch), I
see that there are a couple more early returns leaking 'branch_name',
so they need to be handled, as well.
^ permalink raw reply
* [PATCH 0/2] Skip a connectivity check during fetch --filter
From: Jonathan Tan @ 2020-01-12 4:15 UTC (permalink / raw)
To: git; +Cc: Jonathan Tan
The same optimization in dfa33a298d ("clone: do faster object check for
partial clones", 2019-04-21) can be applied to fetch as well, so this
patch set does so. Patch 1 makes the check more robust, and patch 2
applies it to one of two connectivity checks performed during the fetch.
As mentioned in patch 2, when fetching from a local repo, I got a
speedup of 6.63s to 3.39s.
Jonathan Tan (2):
connected: verify promisor-ness of partial clone
fetch: forgo full connectivity check if --filter
builtin/clone.c | 5 +++--
builtin/fetch.c | 11 ++++++++++-
connected.c | 19 ++++++++++++++-----
connected.h | 11 ++++++-----
4 files changed, 33 insertions(+), 13 deletions(-)
--
2.25.0.rc1.283.g88dfdc4193-goog
^ permalink raw reply
* [PATCH 1/2] connected: verify promisor-ness of partial clone
From: Jonathan Tan @ 2020-01-12 4:15 UTC (permalink / raw)
To: git; +Cc: Jonathan Tan
In-Reply-To: <cover.1578802317.git.jonathantanmy@google.com>
Commit dfa33a298d ("clone: do faster object check for partial clones",
2019-04-21) optimized the connectivity check done when cloning with
--filter to check only the existence of objects directly pointed to by
refs. But this is not sufficient: they also need to be promisor objects.
Make this check more robust by instead checking that these objects are
promisor objects, that is, they appear in a promisor pack.
Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
---
builtin/clone.c | 5 +++--
connected.c | 19 ++++++++++++++-----
connected.h | 11 ++++++-----
3 files changed, 23 insertions(+), 12 deletions(-)
diff --git a/builtin/clone.c b/builtin/clone.c
index 0fc89ae2b9..0516181052 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -673,7 +673,7 @@ static void update_remote_refs(const struct ref *refs,
const char *msg,
struct transport *transport,
int check_connectivity,
- int check_refs_only)
+ int check_refs_are_promisor_objects_only)
{
const struct ref *rm = mapped_refs;
@@ -682,7 +682,8 @@ static void update_remote_refs(const struct ref *refs,
opt.transport = transport;
opt.progress = transport->progress;
- opt.check_refs_only = !!check_refs_only;
+ opt.check_refs_are_promisor_objects_only =
+ !!check_refs_are_promisor_objects_only;
if (check_connected(iterate_ref_map, &rm, &opt))
die(_("remote did not send all necessary objects"));
diff --git a/connected.c b/connected.c
index c337f5f7f4..7e9bd1bc62 100644
--- a/connected.c
+++ b/connected.c
@@ -52,19 +52,28 @@ int check_connected(oid_iterate_fn fn, void *cb_data,
strbuf_release(&idx_file);
}
- if (opt->check_refs_only) {
+ if (opt->check_refs_are_promisor_objects_only) {
/*
* For partial clones, we don't want to have to do a regular
* connectivity check because we have to enumerate and exclude
* all promisor objects (slow), and then the connectivity check
* itself becomes a no-op because in a partial clone every
* object is a promisor object. Instead, just make sure we
- * received the objects pointed to by each wanted ref.
+ * received, in a promisor packfile, the objects pointed to by
+ * each wanted ref.
*/
do {
- if (!repo_has_object_file_with_flags(the_repository, &oid,
- OBJECT_INFO_SKIP_FETCH_OBJECT))
- return 1;
+ struct packed_git *p;
+
+ for (p = get_all_packs(the_repository); p; p = p->next) {
+ if (!p->pack_promisor)
+ continue;
+ if (find_pack_entry_one(oid.hash, p))
+ goto promisor_pack_found;
+ }
+ return 1;
+promisor_pack_found:
+ ;
} while (!fn(cb_data, &oid));
return 0;
}
diff --git a/connected.h b/connected.h
index ce2e7d8f2e..eba5c261ba 100644
--- a/connected.h
+++ b/connected.h
@@ -48,12 +48,13 @@ struct check_connected_options {
unsigned is_deepening_fetch : 1;
/*
- * If non-zero, only check the top-level objects referenced by the
- * wanted refs (passed in as cb_data). This is useful for partial
- * clones, where enumerating and excluding all promisor objects is very
- * slow and the commit-walk itself becomes a no-op.
+ * If non-zero, only check that the top-level objects referenced by the
+ * wanted refs (passed in as cb_data) are promisor objects. This is
+ * useful for partial clones, where enumerating and excluding all
+ * promisor objects is very slow and the commit-walk itself becomes a
+ * no-op.
*/
- unsigned check_refs_only : 1;
+ unsigned check_refs_are_promisor_objects_only : 1;
};
#define CHECK_CONNECTED_INIT { 0 }
--
2.25.0.rc1.283.g88dfdc4193-goog
^ permalink raw reply related
* [PATCH 2/2] fetch: forgo full connectivity check if --filter
From: Jonathan Tan @ 2020-01-12 4:15 UTC (permalink / raw)
To: git; +Cc: Jonathan Tan
In-Reply-To: <cover.1578802317.git.jonathantanmy@google.com>
If a filter is specified, we do not need a full connectivity check on
the contents of the packfile we just fetched; we only need to check that
the objects referenced are promisor objects.
This significantly speeds up fetches into repositories that have many
promisor objects, because during the connectivity check, all promisor
objects are enumerated (to mark them UNINTERESTING), and that takes a
significant amount of time.
Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
---
For example, a local fetch was sped up from 6.63s to 3.39s. The bulk of
the remaining time is spent in yet another connectivity check
(fetch_refs -> check_exist_and_connected) prior to the fetch - that will
hopefully be done in a subsequent patch.
---
builtin/fetch.c | 11 ++++++++++-
1 file changed, 10 insertions(+), 1 deletion(-)
diff --git a/builtin/fetch.c b/builtin/fetch.c
index b4c6d921d0..6fb50320eb 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -906,8 +906,17 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,
url = xstrdup("foreign");
if (!connectivity_checked) {
+ struct check_connected_options opt = CHECK_CONNECTED_INIT;
+
+ if (filter_options.choice)
+ /*
+ * Since a filter is specified, objects indirectly
+ * referenced by refs are allowed to be absent.
+ */
+ opt.check_refs_are_promisor_objects_only = 1;
+
rm = ref_map;
- if (check_connected(iterate_ref_map, &rm, NULL)) {
+ if (check_connected(iterate_ref_map, &rm, &opt)) {
rc = error(_("%s did not send all necessary objects\n"), url);
goto abort;
}
--
2.25.0.rc1.283.g88dfdc4193-goog
^ permalink raw reply related
* Re: [PATCH] branch: let '--edit-description' default to rebased branch during rebase
From: Marc-André Lureau @ 2020-01-12 6:44 UTC (permalink / raw)
To: Eric Sunshine; +Cc: Git List, SZEDER Gábor
In-Reply-To: <CAPig+cRCMXjjPHc2O8fLmaSm9m-ZO3qR2BoZwG3s5dLHNbiFFQ@mail.gmail.com>
Hi Eric
On Sun, Jan 12, 2020 at 5:27 AM Eric Sunshine <sunshine@sunshineco.com> wrote:
>
> On Sat, Jan 11, 2020 at 9:55 AM Marc-André Lureau
> <marcandre.lureau@gmail.com> wrote:
> > On Sat, Jan 11, 2020 at 5:28 PM Eric Sunshine <sunshine@sunshineco.com> wrote:
> > > On Sat, Jan 11, 2020 at 7:36 AM <marcandre.lureau@redhat.com> wrote:
> > > > + if (wt_status_check_rebase(NULL, &state)) {
> > > > + branch_name = state.branch;
> > > > + }
>
> Taking a deeper look at the code, I'm wondering it would make more
> sense to call wt_status_get_state(), which handles 'rebase' and
> 'bisect'. Is there a reason that you limited this check to only
> 'rebase'?
No reason, I just didn't try it yet. Done, thanks
>
> > > > if (edit_branch_description(branch_name))
> > > > return 1;
> > > > +
> > > > + free(branch_name);
> > >
> > > That `return 1` just above this free() is leaking 'branch_name', isn't it?
> >
> > right, let's fix that too
>
> Looking at the code itself (rather than consulting only the patch), I
> see that there are a couple more early returns leaking 'branch_name',
> so they need to be handled, as well.
I think I covered them now, sending v4.
thanks
--
Marc-André Lureau
^ permalink raw reply
* [PATCH v4] branch: let '--edit-description' default to rebased/bisected branch
From: marcandre.lureau @ 2020-01-12 6:47 UTC (permalink / raw)
To: git; +Cc: szeder.dev, sunshine, Marc-André Lureau
From: Marc-André Lureau <marcandre.lureau@redhat.com>
Defaulting to editing the description of the rebased or bisected branch
without an explicit branchname argument would be useful. Even the git
bash prompt shows the name of the rebased branch, and then
~/src/git (mybranch|REBASE-i 1/2)$ git branch --edit-description
fatal: Cannot give description to detached HEAD
looks quite unhelpful.
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
Changed in v4:
- use wt_status_get_state() that handles bisect state
- add a bisecting test
builtin/branch.c | 41 ++++++++++++++++++++++++++++++-----------
t/t3200-branch.sh | 35 +++++++++++++++++++++++++++++++++++
2 files changed, 65 insertions(+), 11 deletions(-)
diff --git a/builtin/branch.c b/builtin/branch.c
index d8297f80ff..cda9fd53e6 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -745,33 +745,52 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
string_list_clear(&output, 0);
return 0;
} else if (edit_description) {
- const char *branch_name;
+ char *branch_name = NULL;
struct strbuf branch_ref = STRBUF_INIT;
if (!argc) {
- if (filter.detached)
- die(_("Cannot give description to detached HEAD"));
- branch_name = head;
+ if (!filter.detached)
+ branch_name = xstrdup(head);
+ else {
+ struct wt_status_state state;
+
+ memset(&state, 0, sizeof(state));
+ wt_status_get_state(the_repository, &state, 0);
+ branch_name = state.branch;
+ if (!branch_name)
+ die(_("Cannot give description to detached HEAD"));
+ free(state.onto);
+ free(state.detached_from);
+ }
} else if (argc == 1)
- branch_name = argv[0];
+ branch_name = xstrdup(argv[0]);
else
die(_("cannot edit description of more than one branch"));
strbuf_addf(&branch_ref, "refs/heads/%s", branch_name);
if (!ref_exists(branch_ref.buf)) {
- strbuf_release(&branch_ref);
+ int ret;
if (!argc)
- return error(_("No commit on branch '%s' yet."),
- branch_name);
+ ret = error(_("No commit on branch '%s' yet."),
+ branch_name);
else
- return error(_("No branch named '%s'."),
- branch_name);
+ ret = error(_("No branch named '%s'."),
+ branch_name);
+
+ strbuf_release(&branch_ref);
+ free(branch_name);
+ return ret;
+
}
strbuf_release(&branch_ref);
- if (edit_branch_description(branch_name))
+ if (edit_branch_description(branch_name)) {
+ free(branch_name);
return 1;
+ }
+
+ free(branch_name);
} else if (copy) {
if (!argc)
die(_("branch name required"));
diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
index 411a70b0ce..7ea6876fe7 100755
--- a/t/t3200-branch.sh
+++ b/t/t3200-branch.sh
@@ -1260,6 +1260,41 @@ test_expect_success 'use --edit-description' '
test_cmp expect EDITOR_OUTPUT
'
+test_expect_success 'use --edit-description during rebase' '
+ write_script editor <<-\EOF &&
+ echo "Rebase contents" >"$1"
+ EOF
+ (
+ set_fake_editor &&
+ FAKE_LINES="break 1" git rebase -i HEAD^ &&
+ EDITOR=./editor git branch --edit-description &&
+ git rebase --continue
+ ) &&
+ write_script editor <<-\EOF &&
+ git stripspace -s <"$1" >"EDITOR_OUTPUT"
+ EOF
+ EDITOR=./editor git branch --edit-description &&
+ echo "Rebase contents" >expect &&
+ test_cmp expect EDITOR_OUTPUT
+'
+
+test_expect_success 'use --edit-description during bisect' '
+ write_script editor <<-\EOF &&
+ echo "Bisect contents" >"$1"
+ EOF
+ git bisect start &&
+ git bisect bad &&
+ git bisect good HEAD~2 &&
+ EDITOR=./editor git branch --edit-description &&
+ git bisect reset &&
+ write_script editor <<-\EOF &&
+ git stripspace -s <"$1" >"EDITOR_OUTPUT"
+ EOF
+ EDITOR=./editor git branch --edit-description &&
+ echo "Bisect contents" >expect &&
+ test_cmp expect EDITOR_OUTPUT
+'
+
test_expect_success 'detect typo in branch name when using --edit-description' '
write_script editor <<-\EOF &&
echo "New contents" >"$1"
base-commit: 7a6a90c6ec48fc78c83d7090d6c1b95d8f3739c0
--
2.25.0.rc2.1.ga00adf396b.dirty
^ permalink raw reply related
* Re: [PATCH v4] branch: let '--edit-description' default to rebased/bisected branch
From: Eric Sunshine @ 2020-01-12 10:17 UTC (permalink / raw)
To: marcandre.lureau; +Cc: git, szeder.dev
In-Reply-To: <20200112064706.2030292-1-marcandre.lureau@redhat.com>
On Sun, Jan 12, 2020 at 10:47:06AM +0400, marcandre.lureau@redhat.com wrote:
> diff --git a/builtin/branch.c b/builtin/branch.c
> @@ -745,33 +745,52 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
> string_list_clear(&output, 0);
> return 0;
> } else if (edit_description) {
> - const char *branch_name;
> + char *branch_name = NULL;
Do you need to assign NULL here? Doesn't 'branch_name' get assigned in
all cases in which the code doesn't otherwise die()?
> if (!argc) {
> - if (filter.detached)
> - die(_("Cannot give description to detached HEAD"));
> - branch_name = head;
> + if (!filter.detached)
> + branch_name = xstrdup(head);
> + else {
> + struct wt_status_state state;
> +
> + memset(&state, 0, sizeof(state));
> + wt_status_get_state(the_repository, &state, 0);
> + branch_name = state.branch;
> + if (!branch_name)
> + die(_("Cannot give description to detached HEAD"));
> + free(state.onto);
> + free(state.detached_from);
I was wondering if it would make sense to attempt this branch name
lookup much earlier in the function when it assigns 'head' (if 'head'
is detached), with the idea that perhaps other git-branch modes might
benefit from it rather than doing it only for this one special-case.
However, it looks like other code (such as branch copy and branch
rename) would actively be hurt by such a change.
At any rate, it might make the 'edit_description' case easier to read
if this special-case branch lookup code was factored out into its own
function. Not itself worth a re-roll, but something to consider if you
do re-roll.
> + }
> } else if (argc == 1)
> - branch_name = argv[0];
> + branch_name = xstrdup(argv[0]);
> else
> die(_("cannot edit description of more than one branch"));
>
> strbuf_addf(&branch_ref, "refs/heads/%s", branch_name);
> if (!ref_exists(branch_ref.buf)) {
> - strbuf_release(&branch_ref);
> + int ret;
>
> if (!argc)
> - return error(_("No commit on branch '%s' yet."),
> - branch_name);
> + ret = error(_("No commit on branch '%s' yet."),
> + branch_name);
> else
> - return error(_("No branch named '%s'."),
> - branch_name);
> + ret = error(_("No branch named '%s'."),
> + branch_name);
> +
> + strbuf_release(&branch_ref);
> + free(branch_name);
> + return ret;
> +
> }
Unnecessary blank line after 'return'.
A couple observations...
The extra cleanup needed to handle 'branch_name' makes this code quite
a bit more verbose. I was wondering if it would be possible to
consolidate the cleanup in a "failure path" as the target of a 'goto'
(which is a common way to perform cleanup in the Git code-base).
However, doing it that way doesn't really make the code much nicer,
which leads to the next observation...
Those `return error(...)` invocations are anomalies in this function.
Every other case of error in cmd_branch() simply die()s -- without
bothering to clean up. There is no apparent reason why this code
instead uses error(). Changing these two cases to die() would simplify
cleanup since you wouldn't have to do any, which would make the code
clearer, shorter, and more consistent with the rest of cmd_branch().
(Such a change probably ought to be done first in a preparatory patch,
making this a two-patch series.)
> strbuf_release(&branch_ref);
>
> - if (edit_branch_description(branch_name))
> + if (edit_branch_description(branch_name)) {
> + free(branch_name);
> return 1;
> + }
> +
> + free(branch_name);
Taking the above comments and observations into account, perhaps
something like this would be cleaner:
--- >8 ---
diff --git a/builtin/branch.c b/builtin/branch.c
index d8297f80ff..0eb519561e 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -601,6 +601,22 @@ static int edit_branch_description(const char *branch_name)
return 0;
}
+/*
+ * Return branch name of current worktree -- even if HEAD is detached -- or
+ * NULL if no branch is associated with worktree. Caller is responsible for
+ * freeing result.
+ */
+static char *get_worktree_branch()
+{
+ struct wt_status_state state;
+
+ memset(&state, 0, sizeof(state));
+ wt_status_get_state(the_repository, &state, 0);
+ free(state.onto);
+ free(state.detached_from);
+ return state.branch;
+}
+
int cmd_branch(int argc, const char **argv, const char *prefix)
{
int delete = 0, rename = 0, copy = 0, force = 0, list = 0;
@@ -745,13 +761,16 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
string_list_clear(&output, 0);
return 0;
} else if (edit_description) {
+ int ret;
const char *branch_name;
+ char *to_free = NULL;
struct strbuf branch_ref = STRBUF_INIT;
if (!argc) {
- if (filter.detached)
+ if (!filter.detached)
+ branch_name = head;
+ else if (!(branch_name = to_free = get_worktree_branch()))
die(_("Cannot give description to detached HEAD"));
- branch_name = head;
} else if (argc == 1)
branch_name = argv[0];
else
@@ -759,19 +778,16 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
strbuf_addf(&branch_ref, "refs/heads/%s", branch_name);
if (!ref_exists(branch_ref.buf)) {
- strbuf_release(&branch_ref);
-
if (!argc)
- return error(_("No commit on branch '%s' yet."),
- branch_name);
+ die(_("No commit on branch '%s' yet."), branch_name);
else
- return error(_("No branch named '%s'."),
- branch_name);
+ die(_("No branch named '%s'."), branch_name);
}
strbuf_release(&branch_ref);
- if (edit_branch_description(branch_name))
- return 1;
+ ret = edit_branch_description(branch_name);
+ free(to_free);
+ return ret;
} else if (copy) {
if (!argc)
die(_("branch name required"));
--- >8 ---
^ permalink raw reply related
* Re: [PATCH v2 04/16] t2018: use test_expect_code for failing git commands
From: Eric Sunshine @ 2020-01-12 10:50 UTC (permalink / raw)
To: Denton Liu
Cc: Git Mailing List, Jakub Narebski, Johannes Sixt, Junio C Hamano
In-Reply-To: <587e53053f02ad0867dc903688c8887602950bd3.1578372682.git.liu.denton@gmail.com>
On Mon, Jan 6, 2020 at 11:53 PM Denton Liu <liu.denton@gmail.com> wrote:
> t2018: use test_expect_code for failing git commands
>
> When we are expecting `git diff` to fail, we negate its status with
> `!`. However, if git ever exits unexpectedly, say due to a segfault, we
> would not be able to tell the difference between that and a controlled
> failure. Use `test_expect_code 1 git diff` instead so that if an
> unexpected failure occurs, we can catch it.
>
> We have some instances of `test_must_fail test_dirty_{un,}mergable`.
> However, `test_must_fail` should only be used on git commands. Convert
> these instances to use the `test_dirty_{un,}mergeable_discards_changes`
> helper functions so that we remove the double-negation.
I had to read all of this several times to understand what it was
trying to say. I think what made it difficult was a combination of
talking about using test_expect_code() for "failing git commands",
coupled with the use "we" in "When we are ... we negate..." which (to
me) sounds as if you are describing the _desired_ way of coding this,
not the incorrect way. A possible rewrite:
t2018: be more discerning when checking for expected exit codes
Functions test_dirty_unmergeable() and test_dirty_mergeable()
expect git-diff to exit with the specific code 1. However, rather
than checking for that value explicitly, they instead negate the
exit code. Unfortunately, this negation makes it impossible to
distinguish the expected code from some other unexpected non-zero
code, for instance, from a segmentation fault. Therefore, be more
discerning by checking the exit code explicitly using
test_expect_code().
Furthermore, some callers of those functions want to negate the
result again, and do so with test_must_fail(). However,
test_must_fail() should only be used with git commands. Address
this by introducing a couple new tiny helper functions which test
the exact condition expected (without the unnecessarily confusing
double-negation).
> While we're at it, remove redirections to /dev/null since output is
> silenced when running without `-v` anyway.
> Signed-off-by: Denton Liu <liu.denton@gmail.com>
^ permalink raw reply
* Re: [PATCH v4] branch: let '--edit-description' default to rebased/bisected branch
From: Eric Sunshine @ 2020-01-12 10:55 UTC (permalink / raw)
To: marcandre.lureau; +Cc: Git List, SZEDER Gábor
In-Reply-To: <20200112101735.GA19676@flurp.local>
On Sun, Jan 12, 2020 at 5:17 AM Eric Sunshine <sunshine@sunshineco.com> wrote:
> +/*
> + * Return branch name of current worktree -- even if HEAD is detached -- or
> + * NULL if no branch is associated with worktree. Caller is responsible for
> + * freeing result.
> + */
> +static char *get_worktree_branch()
This would of course be:
static char *get_worktree_branch(void)
^ permalink raw reply
* [GIT PULL] l10n updates for 2.25.0 round 1
From: Jiang Xin @ 2020-01-12 12:09 UTC (permalink / raw)
To: Junio C Hamano
Cc: Jiang Xin, Git List, Alessandro Menti, Alexander Shopov,
Christopher Díaz, Dimitriy Ryazantcev, Gwan-gyeong Mun,
Jean-Noël Avila, Jimmy Angelakos, Jordi Mas,
Matthias Rüster, Peter Krefting, Thomas Braun,
Trần Ngọc Quân, Vasco Almeida, Yi-Jyun Pan
From: Jiang Xin <worldhello.net@gmail.com>
Hi Junio,
This is pull request for git 2.25.0. In this round, ten l10n teams made
theire contributions, and Yi-Jyun Pan contributed Traditional Chinese
translations, which is a new l10n language support for Git.
Please pull the following l10n updates for Git 2.25.0.
The following changes since commit 99c33bed562b41de6ce9bd3fd561303d39645048:
Git 2.25-rc0 (2019-12-25 11:22:02 -0800)
are available in the Git repository at:
git@github.com:git-l10n/git-po.git tags/l10n-2.25.0-rnd1
for you to fetch changes up to ddc12c429b63912032cbabfaac689093de43c2b9:
l10n: zh_CN: for git v2.25.0 l10n round 1 (2020-01-12 19:22:02 +0800)
----------------------------------------------------------------
l10n-2.25.0-rnd1
----------------------------------------------------------------
Alessandro Menti (1):
l10n: it.po: update the Italian translation for Git 2.25.0
Alexander Shopov (1):
l10n: bg.po: Updated Bulgarian translation (4800t)
Christopher Diaz Riveros (1):
l10n: es: 2.25.0 round #1
Jean-Noël Avila (1):
l10n: fr.po v2.25.0 rnd 1
Jiang Xin (6):
Merge tag 'v2.25.0-rc0' into git-po-master
l10n: git.pot: v2.25.0 round 1 (119 new, 13 removed)
Merge branch 'translation_191231' of github.com:l10n-tw/git-po into git-po-master
Merge branch 'fr_v2.25.0_rnd1' of github.com:jnavila/git into master
Merge branch 'master' of github.com:Softcatala/git-po into git-po-master
l10n: zh_CN: for git v2.25.0 l10n round 1
Jordi Mas (2):
l10n: Update Catalan translation
l10n: Update Catalan translation
Matthias Rüster (1):
l10n: de.po: Update German translation v2.25.0 round 1
Peter Krefting (1):
l10n: sv.po: Update Swedish translation (4800t0f0u)
Thomas Braun (1):
l10n: de.po: Reword generation numbers
Trần Ngọc Quân (1):
l10n: vi(4800t): Updated Vietnamese translation v2.25.0
Yi-Jyun Pan (1):
l10n: zh_TW: add translation for v2.24.0
pan93412 (1):
l10n: zh_TW.po: update translation for v2.25.0 round 1
po/TEAMS | 5 +
po/bg.po | 5258 +++++++------
po/ca.po | 8047 +++++++++++---------
po/de.po | 5128 +++++++------
po/es.po | 5120 +++++++------
po/fr.po | 5085 +++++++------
po/git.pot | 5007 ++++++------
po/it.po | 5131 +++++++------
po/sv.po | 5107 +++++++------
po/vi.po | 5112 +++++++------
po/zh_CN.po | 5094 +++++++------
po/zh_TW.po | 24000 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
12 files changed, 53761 insertions(+), 24333 deletions(-)
create mode 100644 po/zh_TW.po
--
Jiang Xin
^ permalink raw reply
* Re: [PATCH] branch: let '--edit-description' default to rebased branch during rebase
From: SZEDER Gábor @ 2020-01-12 12:14 UTC (permalink / raw)
To: Eric Sunshine; +Cc: Marc-André Lureau, Git List
In-Reply-To: <CAPig+cRCMXjjPHc2O8fLmaSm9m-ZO3qR2BoZwG3s5dLHNbiFFQ@mail.gmail.com>
On Sat, Jan 11, 2020 at 08:27:11PM -0500, Eric Sunshine wrote:
> On Sat, Jan 11, 2020 at 9:55 AM Marc-André Lureau
> <marcandre.lureau@gmail.com> wrote:
> > On Sat, Jan 11, 2020 at 5:28 PM Eric Sunshine <sunshine@sunshineco.com> wrote:
> > > On Sat, Jan 11, 2020 at 7:36 AM <marcandre.lureau@redhat.com> wrote:
> > > > + if (wt_status_check_rebase(NULL, &state)) {
> > > > + branch_name = state.branch;
> > > > + }
>
> Taking a deeper look at the code, I'm wondering it would make more
> sense to call wt_status_get_state(), which handles 'rebase' and
> 'bisect'. Is there a reason that you limited this check to only
> 'rebase'?
While I do think that defaulting to edit the description of the
rebased branch makes sense, I'm not sure how that would work with
bisect.
What branch name does wt_status_get_state() return while bisecting?
The branch where I started from? Because that's what 'git status'
shows:
~/src/git (mybranch)$ git bisect start v2.21.0 v2.20.0
Bisecting: 334 revisions left to test after this (roughly 8 steps)
[b99a579f8e434a7757f90895945b5711b3f159d5] Merge branch 'sb/more-repo-in-api'
~/src/git ((b99a579f8e...)|BISECTING)$ git status
HEAD detached at b99a579f8e
You are currently bisecting, started from branch 'mybranch'.
(use "git bisect reset" to get back to the original branch)
nothing to commit, working tree clean
But am I really on that branch? Does it really makes sense to edit
the description of 'mybranch' by default while bisecting through an
old revision range? I do not think so.
> > > > if (edit_branch_description(branch_name))
> > > > return 1;
> > > > +
> > > > + free(branch_name);
> > >
> > > That `return 1` just above this free() is leaking 'branch_name', isn't it?
> >
> > right, let's fix that too
>
> Looking at the code itself (rather than consulting only the patch), I
> see that there are a couple more early returns leaking 'branch_name',
> so they need to be handled, as well.
'git branch --edit-description' is a one-shot operation: it allows to
edit only one branch description per invocation, and then the process
exits right away, whether the operation was successful or some error
occurred. I'm not sure free()ing 'branch_name' is worth the effort
(and even if it does, I think it should be a separate preparatory
patch).
^ permalink raw reply
* Problems with ra/rebase-i-more-options - should we revert it?
From: Phillip Wood @ 2020-01-12 16:12 UTC (permalink / raw)
To: Junio C Hamano, Git Mailing List, Johannes Schindelin,
Rohit Ashiwal
I'm concerned that there are some bugs in this series and think
it may be best to revert it before releasing 2.25.0. Jonathan
Nieder posted a bug report on Friday [1] which I think is caused
by this series. While trying to reproduce Jonathan's bug I came
up with the test below which fails, but not in the same way. The
test coverage of this series has always been pretty poor and I
think it needs improving for us to have confidence in it. I'm
also concerned that at least one of the
tests ('--committer-date-is-author-date works with rebase -r')
does not detect failures properly in the code below
while read HASH
do
git show $HASH --pretty="format:%ai" >authortime
git show $HASH --pretty="format:%ci" >committertime
test_cmp authortime committertime
done <rev_list
Best Wishes
Phillip
[1] https://lore.kernel.org/git/20200110231436.GA24315@google.com/
--- >8 ---
diff --git a/t/t3433-rebase-options-compatibility.sh b/t/t3433-rebase-options-compatibility.sh
index 5166f158dd..c81e1d7167 100755
--- a/t/t3433-rebase-options-compatibility.sh
+++ b/t/t3433-rebase-options-compatibility.sh
@@ -6,6 +6,7 @@
test_description='tests to ensure compatibility between am and interactive backends'
. ./test-lib.sh
+. "$TEST_DIRECTORY"/lib-rebase.sh
GIT_AUTHOR_DATE="1999-04-02T08:03:20+05:30"
export GIT_AUTHOR_DATE
@@ -99,6 +100,22 @@ test_expect_success '--committer-date-is-author-date works with rebase -r' '
done <rev_list
'
+test_expect_success '--committer-date-is-author-date works when committing conflict resolution' '
+ git checkout commit2 &&
+ (
+ set_fake_editor &&
+ FAKE_LINES=2 &&
+ export FAKE_LINES &&
+ test_must_fail git rebase -i HEAD^^
+ ) &&
+ echo resolved > foo &&
+ git add foo &&
+ git rebase --continue &&
+ git log -1 --format=%at commit2 >expect &&
+ git log -1 --format=%ct HEAD >actual &&
+ test_cmp expect actual
+'
+
# Checking for +0000 in author time is enough since default
# timezone is UTC, but the timezone used while committing
# sets to +0530.
^ permalink raw reply related
* Re: Problems with ra/rebase-i-more-options - should we revert it?
From: Phillip Wood @ 2020-01-12 17:31 UTC (permalink / raw)
To: Junio C Hamano, Git Mailing List, Johannes Schindelin,
Rohit Ashiwal
Cc: Jonathan Nieder
In-Reply-To: <f2fe7437-8a48-3315-4d3f-8d51fe4bb8f1@gmail.com>
On 12/01/2020 16:12, Phillip Wood wrote:
> I'm concerned that there are some bugs in this series and think
> it may be best to revert it before releasing 2.25.0. Jonathan
> Nieder posted a bug report on Friday [1] which I think is caused
> by this series. While trying to reproduce Jonathan's bug I came
> up with the test below which fails, but not in the same way.
Doh I forgot to add --committer-date-is-author-date to the rebase
command line in that test. It passes with that added - how
embarrassing. However it does appear that it prefixes the date in
GIT_COMMITTER_DATE with @@ rather than @. I think (though am not
completely certain yet) the reason the test still passes is that
the date has more than 8 digits so although
match_object_header_date() fails because of the '@@'
match_digit() succeeds once the loop in parse_date_basic() strips
that prefix. Jonathan's test date only has 7 digits so
match_digit() does not treat it as a number of seconds since the
start of the epoch and fails to parse it. The fix for the @@ is
quite simple, the date we read from the author script already has
an @ so we don't need to add another. The diff below shows a
basic fix but we should get rid of datebuf altogether as we don't
need it. I need a break now I'll try and put a patch together
later in the week if no one else has by then.
Best Wishes
Phillip
--- >8 ---
diff --git a/sequencer.c b/sequencer.c
index 763ccbbc45..22a38de47b 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -988,7 +988,7 @@ static int run_git_commit(struct repository *r,
if (!date)
return -1;
- strbuf_addf(&datebuf, "@%s", date);
+ strbuf_addf(&datebuf, "%s", date);
res = setenv("GIT_COMMITTER_DATE",
opts->ignore_date ? "" : datebuf.buf, 1);
> The
> test coverage of this series has always been pretty poor and I
> think it needs improving for us to have confidence in it. I'm
> also concerned that at least one of the
> tests ('--committer-date-is-author-date works with rebase -r')
> does not detect failures properly in the code below
>
> while read HASH
> do
> git show $HASH --pretty="format:%ai" >authortime
> git show $HASH --pretty="format:%ci" >committertime
> test_cmp authortime committertime
> done <rev_list
>
>
> Best Wishes
>
> Phillip
>
> [1] https://lore.kernel.org/git/20200110231436.GA24315@google.com/
>
> --- >8 ---
> diff --git a/t/t3433-rebase-options-compatibility.sh b/t/t3433-rebase-options-compatibility.sh
> index 5166f158dd..c81e1d7167 100755
> --- a/t/t3433-rebase-options-compatibility.sh
> +++ b/t/t3433-rebase-options-compatibility.sh
> @@ -6,6 +6,7 @@
> test_description='tests to ensure compatibility between am and interactive backends'
>
> . ./test-lib.sh
> +. "$TEST_DIRECTORY"/lib-rebase.sh
>
> GIT_AUTHOR_DATE="1999-04-02T08:03:20+05:30"
> export GIT_AUTHOR_DATE
> @@ -99,6 +100,22 @@ test_expect_success '--committer-date-is-author-date works with rebase -r' '
> done <rev_list
> '
>
> +test_expect_success '--committer-date-is-author-date works when committing conflict resolution' '
> + git checkout commit2 &&
> + (
> + set_fake_editor &&
> + FAKE_LINES=2 &&
> + export FAKE_LINES &&
> + test_must_fail git rebase -i HEAD^^
> + ) &&
> + echo resolved > foo &&
> + git add foo &&
> + git rebase --continue &&
> + git log -1 --format=%at commit2 >expect &&
> + git log -1 --format=%ct HEAD >actual &&
> + test_cmp expect actual
> +'
> +
> # Checking for +0000 in author time is enough since default
> # timezone is UTC, but the timezone used while committing
> # sets to +0530.
>
^ permalink raw reply related
* Re: [PATCH v3 15/15] rebase: change the default backend from "am" to "merge"
From: Johannes Schindelin @ 2020-01-12 17:59 UTC (permalink / raw)
To: Phillip Wood
Cc: Elijah Newren, Jonathan Nieder, Elijah Newren via GitGitGadget,
Git Mailing List, Denton Liu, Junio C Hamano, Pavel Roskin,
Alban Gruin, SZEDER Gábor
In-Reply-To: <052fdedc-2beb-91ab-c5c3-53fb99e64810@gmail.com>
Hi,
On Sat, 11 Jan 2020, Phillip Wood wrote:
> On 11/01/2020 01:16, Elijah Newren wrote:
> >
> > On Fri, Jan 10, 2020 at 3:14 PM Jonathan Nieder <jrnieder@gmail.com> wrote:
> > >
> > > Elijah Newren via GitGitGadget wrote:
> > >
> > > 1. "git rebase --am" does not invoke the post-commit hook, but "git
> > > rebase --merge" does. Is this behavior change intended?
> > >
> > > Noticed because jiri[1] installs a post-commit hook that warns
> > > about commits on detached HEAD, so this change makes rebases more
> > > noisy in repositories that were set up using jiri.
>
> Perhaps that hook could learn not to warn if a branch is being rebased?
> git could be more helpful there by having a porcelain option to status
> that prints the branch name if we're rebasing (`git worktree --list`
> shows the branch correctly when it's being rebased but does not (yet - I
> have a patch to do it) mark the current worktree so isn't very helpful.)
>
> > I've never used a post-commit hook or seen one in the wild. Certainly
> > wasn't intentional, but it's not clear to me if it's wrong or right
> > either. I don't see why it would make sense to distinguish between
> > any of git rebase --am/--merge/--interactive, but it isn't too
> > surprising that by historical accident the two rebase backends which
> > happened to call git-commit behind the scenes would call a post-commit
> > hook and the other rebase backend that didn't call git-commit
> > wouldn't.
>
> Looking through the history the am based rebase has never run the post-commit
> hook as am has its own set of hooks and the scripted version used commit-tree.
> The merge based rebase ran `git commit` which ran the post commit hook. The
> interactive rebase ran the hook until and I broke it in a356ee4659b
> ("sequencer: try to commit without forking 'git commit'", 2017-11-24) and
> after I fixed it in 4627bc777e ("sequencer: run post-commit hook",
> 2019-10-15). As it was broken for two years with no one noticing it can't be
> that popular.
Maybe a crazy idea, but maybe not: how about running the `post-commit`
hook _only_ if `--merge` was specified explicitly, and in that case (and
guarded behind a check verifying that the `post-commit` hook _actually_
exists _and_ is executable) warn the user that this hook won't be run in
future versions?
To make things better for users who actually want to run that hook during
rebases, we could introduce a config option, say,
`rebase.runPostCommitHook` that is a tri-state (`true`, `false`,
`onlyForDashDashMerge`, at first defaulting to the last, eventually to
`false`).
Crazy? Or helpful?
> > But the big question here, is what is correct behavior? Should rebase
> > call the post-commit hook, or should it skip it? I haven't any clue
> > what the answer to that is.
>
> It's creating a new commit so I lean towards thinking it should run the
> post-commit hook. As an example I have a post-commit hook that prints a
> warning if a commit is created on a branch that is being rewritten by
> one of my scripts in another worktree. There are pre-commit and
> pre-rebase hooks to try and prevent that, but the warning is there as a
> last resort if those hooks are by-passed.
I guess you're right, it is quite surprising that the `post-commit` hook
is _not_ run for `--am` rebases even though commits are created.
> > > 2. GIT_REFLOG_ACTION contains "rebase -i" even though the rebase is
> > > not interactive.
>
> If this is important to people I think it should be easy enough to set
> GIT_REFLOG_ACTION to the appropriate string in builtin/rebase.c (so long
> as it hasn't already been set by the user) rather than relying on
> sequencer.c to do it.
I agree (but won't have time to implement it, so maybe I should shut up
already...)
> > Yep, as does --keep, --exec, --rebase-merges, etc. There are lots of
> > rebases which use the interactive machinery even if they aren't
> > explicitly interactive. I've never seen the "-i" in the reflog
> > message defined, but clearly it has always been used whenever the
> > interactive machinery was in play regardless of whether the rebase was
> > interactive. In that regard, I figured that --merge fit in rather
> > nicely. (And I noted the fact that reflog messages were different
> > between the backends among the "BEHAVIORAL DIFFERENCES" section of
> > git-rebase.txt). But if others think we should just drop the -i (much
> > as we did for the bash prompt), I'd be happy with that too. If we go
> > that route, I think I'd rather drop the -i in the reflog for all
> > rebases, not just the
> > using-the-interactive-machinery-but-not-explicitly-interactive ones.
> >
> > > 3. In circumstances I haven't pinned down yet, we get the error
> > > message "invalid date format: @@2592000 +0000":
> > >
> > > $ git rebase --committer-date-is-author-date --onto branch_K
> > > branch_L~1 branch_L
> > > $ git checkout --theirs file
> > > $ git add file
> > > $ git rebase --continue
> > > fatal: invalid date format: @@2592000 +0000
> > > error: could not commit staged changes.
> > >
> > > This isn't reproducible without --committer-date-is-author-date.
> > > More context (the test where it happens) is in [2].
> >
> > Interesting. Do you happen to know if this started happening with
> > ra/rebase-i-more-options, or did it just become an issue with
> > en/rebase-backend? I looked around at the link you provided and feel
> > a bit confused; I'm not sure which test does this or how I'd
> > reproduce.
>
> I'm confused by the test as well. As ra/rebase-i-more-options only touched the
> sequencer then any bugs would only show up in this test (which runs a
> non-interactive rebase) once en/rbease-backend switched to that backend. It
> seems likely that ra/rebase-i-more-options is to blame.
>
> Jonathan - do you happen to know if your users create empty commits at all?
> and if so what do they expect rebase to do with them (and any that become
> empty when they are rebased) - cf
> https://lore.kernel.org/git/<CABPp-BEH=9qejeqysHYE+AJ+JPaBympZizq-bx_OjArYFa4xUQ@mail.gmail.com>
The double `@` looks very funny. I would be interested in seeing an MCVE.
> > > 4. I suspect the exit status in the "you need to resolve conflicts"
> > > case has changed. With rebase --am, [3] would automatically
> > > invoke rebase --abort when conflicts are present, but with rebase
> > > --merge it does not.
> > >
> > > Known?
> >
> > Nope, but I would certainly hope that "you need to resolve conflicts"
> > would result in a non-zero exit status. If it doesn't, that sounds
> > like a bug in the interactive backend that we need to fix. I'll dig
> > in.
Yes, exiting with status 0 would be a major bug, and I think it might even
be a bug that was introduced by me when I re-implemented the core loop of
the interactive rebase in C.
But to me it sounds as if 4. is not so about the exit code but about
aborting immediately. I do not recall seeing --am rebases to abort,
though, but to exit with error (and I saw the same behavior in interactive
rebases).
We will need to see a reduced concrete example (preferably as a new test
case) of the described behavior.
Ciao,
Dscho
^ 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