| Message ID | 227aea031b588977f22f3f97faee981d79ade05c.1682089074.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | rebase -i: impove handling of failed commands | expand |
"Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes: > From: Phillip Wood <phillip.wood@dunelm.org.uk> > > When rebase stops for the user to resolve conflicts it writes a patch > for the conflicting commit to .git/rebase-merge/patch. This file > should be deleted when the rebase continues. Could you describe the reason why this file "should" be deleted a bit better? Once the user edits the files in the working tree and tell "git rebase" with the "--continue" option that they finished helping the command, and the command creates a commit out of the resolution left by the user in the working tree and in the index, the patch may no longer is needed, so I can understand if this were "this file can be deleted"---in other words, again, this explains why such a change would not be a wrong change that hurts the users, but it does not explain why we want such a change very well. Is there a reason why a left-over patch file is a bad thing (perhaps causing end-user confusion upon seeing such a patch that apparently is for a much earlier step in the rebase in progress? If so, that might be a good justification to say we "should"). > As the path is now used > in two different places rebase_path_patch() is added and used to > obtain the path for the patch. OK. > Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk> > --- > sequencer.c | 9 +++++++-- > 1 file changed, 7 insertions(+), 2 deletions(-) The patch text itself looks good in the sense that it correctly implements what the proposed log message claims it "should". Thanks.
On 21/04/2023 20:01, Junio C Hamano wrote: > "Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes: > >> From: Phillip Wood <phillip.wood@dunelm.org.uk> >> >> When rebase stops for the user to resolve conflicts it writes a patch >> for the conflicting commit to .git/rebase-merge/patch. This file >> should be deleted when the rebase continues. > > Could you describe the reason why this file "should" be deleted a > bit better? Once the user edits the files in the working tree and > tell "git rebase" with the "--continue" option that they finished > helping the command, and the command creates a commit out of the > resolution left by the user in the working tree and in the index, > the patch may no longer is needed, so I can understand if this were > "this file can be deleted"---in other words, again, this explains > why such a change would not be a wrong change that hurts the users, > but it does not explain why we want such a change very well. Is > there a reason why a left-over patch file is a bad thing (perhaps > causing end-user confusion upon seeing such a patch that apparently > is for a much earlier step in the rebase in progress? If so, that > might be a good justification to say we "should"). Yes that's the reason - we don't want a stale patch when we stop for another reason, I'll improve the commit message. Best Wishes Phillip >> As the path is now used >> in two different places rebase_path_patch() is added and used to >> obtain the path for the patch. > > OK. > >> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk> >> --- >> sequencer.c | 9 +++++++-- >> 1 file changed, 7 insertions(+), 2 deletions(-) > > The patch text itself looks good in the sense that it correctly > implements what the proposed log message claims it "should". > > Thanks.
"Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes: > @@ -3490,7 +3495,6 @@ static int make_patch(struct repository *r, > return -1; > res |= write_rebase_head(&commit->object.oid); > > - strbuf_addf(&buf, "%s/patch", get_dir(opts)); > memset(&log_tree_opt, 0, sizeof(log_tree_opt)); > repo_init_revisions(r, &log_tree_opt, NULL); > log_tree_opt.abbrev = 0; I was checking to see if we could remove buf or whether we are reusing it for unrelated reasons (which is a common Git-ism). We can't remove it because we reuse it, however... > @@ -3498,7 +3502,7 @@ static int make_patch(struct repository *r, > log_tree_opt.diffopt.output_format = DIFF_FORMAT_PATCH; > log_tree_opt.disable_stdin = 1; > log_tree_opt.no_commit_id = 1; > - log_tree_opt.diffopt.file = fopen(buf.buf, "w"); > + log_tree_opt.diffopt.file = fopen(rebase_path_patch(), "w"); > log_tree_opt.diffopt.use_color = GIT_COLOR_NEVER; > if (!log_tree_opt.diffopt.file) > res |= error_errno(_("could not open '%s'"), buf.buf); this buf.buf was supposed to be the value we populated earlier - this should be rebase_path_patch() instead. As an aside, I have a mild distaste the Git-ism of reusing "struct strbuf buf" - using a variable for just a single purpose and naming it as such makes these sorts of errors much easier to spot. That isn't something we need to fix here, I'm just venting a little :)
On 21/06/2023 21:14, Glen Choo wrote: > "Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes: > >> @@ -3490,7 +3495,6 @@ static int make_patch(struct repository *r, >> return -1; >> res |= write_rebase_head(&commit->object.oid); >> >> - strbuf_addf(&buf, "%s/patch", get_dir(opts)); >> memset(&log_tree_opt, 0, sizeof(log_tree_opt)); >> repo_init_revisions(r, &log_tree_opt, NULL); >> log_tree_opt.abbrev = 0; > > I was checking to see if we could remove buf or whether we are reusing > it for unrelated reasons (which is a common Git-ism). We can't remove it > because we reuse it, however... I had a look at that and we're using it to construct a path that we should obtain by calling rebase_path_message() - I'll add a fix when I re-roll. >> @@ -3498,7 +3502,7 @@ static int make_patch(struct repository *r, >> log_tree_opt.diffopt.output_format = DIFF_FORMAT_PATCH; >> log_tree_opt.disable_stdin = 1; >> log_tree_opt.no_commit_id = 1; >> - log_tree_opt.diffopt.file = fopen(buf.buf, "w"); >> + log_tree_opt.diffopt.file = fopen(rebase_path_patch(), "w"); >> log_tree_opt.diffopt.use_color = GIT_COLOR_NEVER; >> if (!log_tree_opt.diffopt.file) >> res |= error_errno(_("could not open '%s'"), buf.buf); > > this buf.buf was supposed to be the value we populated earlier - this > should be rebase_path_patch() instead. Oh, well spotted, thanks for pointing that out. > As an aside, I have a mild distaste the Git-ism of reusing "struct > strbuf buf" - using a variable for just a single purpose and naming it > as such makes these sorts of errors much easier to spot. That isn't > something we need to fix here, I'm just venting a little :) I agree it can get confusing. We occasionally forget to call strbuf_reset() before reusing the buffer (my first contribution to git fixed such a case in 4ab867b8fc8 (rebase -i: fix reflog message, 2017-05-18)), or forget to remove a call to reset the buffer that is no-longer necessary when refactoring. However it does save quite a few calls to malloc()/free() in the rebase/sequencer code. Best Wishes Phillip
Phillip Wood <phillip.wood123@gmail.com> writes: > On 21/06/2023 21:14, Glen Choo wrote: >> "Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes: >> >>> @@ -3490,7 +3495,6 @@ static int make_patch(struct repository *r, >>> return -1; >>> res |= write_rebase_head(&commit->object.oid); >>> - strbuf_addf(&buf, "%s/patch", get_dir(opts)); >>> memset(&log_tree_opt, 0, sizeof(log_tree_opt)); >>> repo_init_revisions(r, &log_tree_opt, NULL); >>> log_tree_opt.abbrev = 0; >> I was checking to see if we could remove buf or whether we are >> reusing >> it for unrelated reasons (which is a common Git-ism). We can't remove it >> because we reuse it, however... > > I had a look at that and we're using it to construct a path that we > should obtain by calling rebase_path_message() - I'll add a fix when I > re-roll. Wow, a patch from April commented in June and responded in July ;-). I'll salvage the topic from the "will discard" bin and mark it again as "Expecting a reroll" in my draft of the next "What's cooking" report. Thanks.
On 14/07/2023 17:51, Junio C Hamano wrote: > Phillip Wood <phillip.wood123@gmail.com> writes: > >> On 21/06/2023 21:14, Glen Choo wrote: >>> "Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes: >>> >>>> @@ -3490,7 +3495,6 @@ static int make_patch(struct repository *r, >>>> return -1; >>>> res |= write_rebase_head(&commit->object.oid); >>>> - strbuf_addf(&buf, "%s/patch", get_dir(opts)); >>>> memset(&log_tree_opt, 0, sizeof(log_tree_opt)); >>>> repo_init_revisions(r, &log_tree_opt, NULL); >>>> log_tree_opt.abbrev = 0; >>> I was checking to see if we could remove buf or whether we are >>> reusing >>> it for unrelated reasons (which is a common Git-ism). We can't remove it >>> because we reuse it, however... >> >> I had a look at that and we're using it to construct a path that we >> should obtain by calling rebase_path_message() - I'll add a fix when I >> re-roll. > > Wow, a patch from April commented in June and responded in July ;-). > > I'll salvage the topic from the "will discard" bin and mark it again > as "Expecting a reroll" in my draft of the next "What's cooking" > report. Thanks, sorry it has taken so long, I've been struggling to find time to work on this but hopefully will have a new version ready in the next couple of weeks Best Wishes Phillip > Thanks.
diff --git a/sequencer.c b/sequencer.c index 5073ec5902b..c4a548f2c98 100644 --- a/sequencer.c +++ b/sequencer.c @@ -132,6 +132,11 @@ static GIT_PATH_FUNC(rebase_path_amend, "rebase-merge/amend") * the commit object name of the corresponding patch. */ static GIT_PATH_FUNC(rebase_path_stopped_sha, "rebase-merge/stopped-sha") +/* + * When we stop for the user to resolve conflicts this file contains + * the patch of the commit that is being picked. + */ +static GIT_PATH_FUNC(rebase_path_patch, "rebase-merge/patch") /* * For the post-rewrite hook, we make a list of rewritten commits and * their new sha1s. The rewritten-pending list keeps the sha1s of @@ -3490,7 +3495,6 @@ static int make_patch(struct repository *r, return -1; res |= write_rebase_head(&commit->object.oid); - strbuf_addf(&buf, "%s/patch", get_dir(opts)); memset(&log_tree_opt, 0, sizeof(log_tree_opt)); repo_init_revisions(r, &log_tree_opt, NULL); log_tree_opt.abbrev = 0; @@ -3498,7 +3502,7 @@ static int make_patch(struct repository *r, log_tree_opt.diffopt.output_format = DIFF_FORMAT_PATCH; log_tree_opt.disable_stdin = 1; log_tree_opt.no_commit_id = 1; - log_tree_opt.diffopt.file = fopen(buf.buf, "w"); + log_tree_opt.diffopt.file = fopen(rebase_path_patch(), "w"); log_tree_opt.diffopt.use_color = GIT_COLOR_NEVER; if (!log_tree_opt.diffopt.file) res |= error_errno(_("could not open '%s'"), buf.buf); @@ -4642,6 +4646,7 @@ static int pick_commits(struct repository *r, unlink(rebase_path_message()); unlink(rebase_path_stopped_sha()); unlink(rebase_path_amend()); + unlink(rebase_path_patch()); while (todo_list->current < todo_list->nr) { struct todo_item *item = todo_list->items + todo_list->current;