| Message ID | xmqq5y5ltqwd.fsf_-_@gitster.g (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Re* [PATCH v3 2/2] doc: revert: add discussion | expand |
Junio C Hamano <gitster@pobox.com> writes: > + > + if (starts_with(msg.subject, "Reapply \"Reapply \"")) > + /* fifth time is too many - force reference format*/ > + use_reference = 1; Come to think of it, as the documentation patch in the series cited double reapply as too unwieldy, we probably should stop before producing such commit. We can update "Reapply \"Reapply" above to "Revert \"Reapply" and then "fifth" -> "fourth". The test update below must also be adjusted, if we want to take that route.
On Friday, August 11, 2023 1:54 PM, Junio C Hamano wrote: >Junio C Hamano <gitster@pobox.com> writes: > >> + >> + if (starts_with(msg.subject, "Reapply \"Reapply \"")) >> + /* fifth time is too many - force reference format*/ >> + use_reference = 1; > >Come to think of it, as the documentation patch in the series cited double reapply as >too unwieldy, we probably should stop before producing such commit. We can >update "Reapply \"Reapply" above to "Revert \"Reapply" and then "fifth" -> "fourth". >The test update below must also be adjusted, if we want to take that route. May I suggest a potential quick solution. Perhaps we could leave this up to users by putting in an --amend or --reword option to cause a prompt for a comment for the reverted commit instead of trying to come up with a one-size-fits-all solution.
On Fri, Aug 11, 2023 at 2:12 PM Junio C Hamano <gitster@pobox.com> wrote: > Subject: [PATCH 3/2] revert: force explaining overly complex revert chain > > Once we revert reverts of revert and reach "Reapply "Reapply "..."", > it becomes too unweirdly to read a reversion of such a comit. s/unweirdly/unwieldy/ s/comit/commit/ > We instruct the user to explain why the reversion is done in their > own words when using the revert.reference mode, and the instruction > applies equally for such an overly complex revert chain. The > rationale for such a sequence of events should be recorded to help > future developers. > > Building on top of the recent Oswald's work to turn "revert revert" > into "reapply", let's turn the reference mode automatically on in > such a case. > > Signed-off-by: Junio C Hamano <gitster@pobox.com>
On Fri, Aug 11, 2023 at 10:44:50AM -0700, Junio C Hamano wrote: >Junio C Hamano <gitster@pobox.com> writes: > >> Linus Arver <linusa@google.com> writes: >> >>> Oswald Buddenhagen <oswald.buddenhagen@gmx.de> writes: >>> >>>> while thinking about what to write, i came up with an idea for another >>>> improvement: with (implicit) --edit, the template message would end up >>>> being: >>>> >>>> This reverts commit <sha1>, >>>> because <PUT REASON HERE>. >>> >>> This sounds great to me. >> >> Oh, absolutely. I rarely do a revert myself (other than reverting a >> premature merge out of 'next'), but giving a better instruction in >> the commit log editor buffer as template is a very good idea. > >It might be just the matter of doing something like the attached >patch on top of Oswald's, reusing the existing code to instruct the >user to describe the reversion. > hmm, this seems to be going down a too narrow road - my idea was to make this fully orthogonal to reverting reverts in particular (note that i attached it to the generic "discussion" patch rather than the "reverts of reverts" one). i didn't think about the integration with existing options yet. regards
Oswald Buddenhagen <oswald.buddenhagen@gmx.de> writes: > make this fully orthogonal to reverting reverts in particular (note > that i attached it to the generic "discussion" patch rather than the > "reverts of reverts" one). I viewed that as a patch to add "discussion on how to explain revert" (not necessarily "revert of revert", but how "revert" in general should be explained). > i didn't think about the integration with existing options yet. Now I did ;-). The discussion of what the right way to present and justify a revert was done before, and I think revert.reference and the "--reference" option came out of it. It aims the same "do not just describe the fact what was reverted, but you should explain why" spirit. While what I showed in my illustration patch was not meant as "integration with existing options", it is inevitable that reuse of existing code that was written earlier for improving the workflow in the same spirit may get involved. Perhaps we should also tweak the commit log template to give a gentle knudge to the user to use revert.reference and then enhance the help text we give. Instead of Revert "doc: revert: add discussion" This reverts commit 7139d1298993b0148ad429cd7cb4824223b7f420. you may see something along the lines of ... # Consider retitling to reflect WHY you are reverting. # You may also want to set revert.reference configuration to # help encuraging a better title for revert commits. Revert "doc: revert: add discussion" This reverts commit 7139d1298993b0148ad429cd7cb4824223b7f420 because <REASON OF REVERSION HERE> I ran out of time budget for today to think about a topic that is not releant to the current release, so I'd stop here. THanks.
diff --git c/sequencer.c w/sequencer.c index 7dc13fdcca..43bb558518 100644 --- c/sequencer.c +++ w/sequencer.c @@ -2130,10 +2130,10 @@ static int should_edit(struct replay_opts *opts) { return opts->edit; } -static void refer_to_commit(struct replay_opts *opts, +static void refer_to_commit(int use_reference, struct strbuf *msgbuf, struct commit *commit) { - if (opts->commit_use_reference) { + if (use_reference) { struct pretty_print_context ctx = { .abbrev = DEFAULT_ABBREV, .date_mode.type = DATE_SHORT, @@ -2250,12 +2250,18 @@ static int do_pick_commit(struct repository *r, if (command == TODO_REVERT) { const char *orig_subject; + int use_reference = opts->commit_use_reference; base = commit; base_label = msg.label; next = parent; next_label = msg.parent_label; - if (opts->commit_use_reference) { + + if (starts_with(msg.subject, "Reapply \"Reapply \"")) + /* fifth time is too many - force reference format*/ + use_reference = 1; + + if (use_reference) { strbuf_addstr(&msgbuf, "# *** SAY WHY WE ARE REVERTING ON THE TITLE LINE ***"); } else if (skip_prefix(msg.subject, "Revert \"", &orig_subject) && @@ -2273,11 +2279,11 @@ static int do_pick_commit(struct repository *r, strbuf_addstr(&msgbuf, "\""); } strbuf_addstr(&msgbuf, "\n\nThis reverts commit "); - refer_to_commit(opts, &msgbuf, commit); + refer_to_commit(use_reference, &msgbuf, commit); if (commit->parents && commit->parents->next) { strbuf_addstr(&msgbuf, ", reversing\nchanges made to "); - refer_to_commit(opts, &msgbuf, parent); + refer_to_commit(opts->commit_use_reference, &msgbuf, parent); } strbuf_addstr(&msgbuf, ".\n"); } else { diff --git c/t/t3501-revert-cherry-pick.sh w/t/t3501-revert-cherry-pick.sh index 7011e3a421..7a8715d3f4 100755 --- c/t/t3501-revert-cherry-pick.sh +++ w/t/t3501-revert-cherry-pick.sh @@ -190,7 +190,16 @@ test_expect_success 'title of fresh reverts' ' git revert --no-edit HEAD && echo "Revert \"Reapply \"B\"\"" >expect && git log -1 --pretty=%s >actual && - test_cmp expect actual + test_cmp expect actual && + git revert --no-edit HEAD && + echo "Reapply \"Reapply \"B\"\"" >expect && + git log -1 --pretty=%s >actual && + test_cmp expect actual && + + # Give the stronger instruction for unusually complex case + git revert --no-edit HEAD && + git log -1 --pretty=%s >actual && + grep -F -e "# *** SAY WHY WE ARE REVERTING" actual ' test_expect_success 'title of legacy double revert' '