Re: [PATCH v2] [GSOC] cherry-pick: fix bug when used with GIT_CHERRY_PICK_HELP

From: Junio C Hamano <gitster@pobox.com>
Date: Jul 28, 2021, 17:24 UTC
Message-ID: <xmqqr1fi9wsj.fsf@gitster.g>
In-Reply-To: <CAOLTT8RRbwss5sJAM-a=LQrZjr0fyOP4C_x8oRGdLFFnpc+M3g@mail.gmail.com>

ZheNing Hu <adlternative@gmail.com> writes:

Show 15 quoted lines

> I think this function "check_need_delete_cherry_pick_head()" should be before
> print_advice(), like this:
>
> +               const char *help_msgs = NULL;
> +
>                 error(command == TODO_REVERT
>                       ? _("could not revert %s... %s")
>                       : _("could not apply %s... %s"),
>                       short_commit_name(commit), msg.subject);
> -               print_advice(r, res == 1, opts);
> +               if (((opts->action == REPLAY_PICK &&
> +                     !opts->rebase_preserve_merges_mode) ||
> +                     (help_msgs = check_need_delete_cherry_pick_head(r))) &&
> +                     res == 1)
> +                       print_advice(opts, help_msgs);

Sorry, but I am not sure what problem this separation is trying to solve.

The root cause of the issue we have, I think, is because the decision to delete or keep the cherry-pick-head pseudoref is tied to what message we give to users in the current code, and the suggested split of concern is to limit print_advice() to only print the advice message, and a new helper to decide and remove the pseudoref, without relying on what is in the GIT_CHERRY_PICK_HELP environment.

It is unclear where you are making the decision to keep or remove the pseudoref in the above arrangement that lets the new check_need_delete_cherry_pick_head() decide if the advice is printed or not.