Re: [GSoC][PATCH v2 1/3] sequencer: add advice for revert

From: Junio C Hamano <gitster@pobox.com>
Date: Jun 11, 2019, 21:25 UTC
Message-ID: <xmqqr27zke5m.fsf@gitster-ct.c.googlers.com>
In-Reply-To: <20190611073152.12214-2-rohit.ashiwal265@gmail.com>

Rohit Ashiwal <rohit.ashiwal265@gmail.com> writes:

Show 17 quoted lines

> -static int create_seq_dir(void)
> +static int create_seq_dir(struct repository *r)
>  {
> -	if (file_exists(git_path_seq_dir())) {
> -		error(_("a cherry-pick or revert is already in progress"));
> -		advise(_("try \"git cherry-pick (--continue | --quit | --abort)\""));
> -		return -1;
> -	} else if (mkdir(git_path_seq_dir(), 0777) < 0)
> +	enum replay_action action;
> +
> +	if (!sequencer_get_last_command(r, &action)) {
> +		switch (action) {
> +		case REPLAY_REVERT:
> +		case REPLAY_PICK:
> +			error(_("a %s is already in progress"),
> +				 action == REPLAY_REVERT ?
> +				 "revert" : "cherry-pick");

I wonder if this poses a challenge to translators (imagine an alternate world, in which the name of one of these subcommands began with a vowel---your "a %s is already ..." would not be correct even without localization).

The same comment applies to the other one, too.

> +			advise(_("try \"git %s (--continue | "
> +				 "--quit | --abort)\""),
> +				 action == REPLAY_REVERT ?
> +				 "revert" : "cherry-pick");

This is horrible but it is not a fault of yours---you merely inherited it. A call to advise() that is not behind any "advise.*" configuration variable like this one should be cleaned up before we do anything else.

The obvious and straight-forward way to deal with it is to do

	const char *in_progress_error;
        const char *in_progress_advice;

	if (action == REPLAY_REVERT) {
		in_progress_error = _("a revert is already in progress");
		in_progress_advise = _("try ...");
	} else if (action == REPLAY_PICK) {
		... likewise ...
	} else {
		BUG("should not come here");
	}

and then do

	error(in_progress_error);
	if (advise_verbosely_how_to_continue_sequencing)
		advise(in_progress_advise);

Show 17 quoted lines

> +			return -1;
> +	if (mkdir(git_path_seq_dir(), 0777) < 0)
>  		return error_errno(_("could not create sequencer directory '%s'"),
>  				   git_path_seq_dir());
> +
>  	return 0;
>  }
>  
> @@ -4237,7 +4252,7 @@ int sequencer_pick_revisions(struct repository *r,
>  	 */
>  
>  	if (walk_revs_populate_todo(&todo_list, opts) ||
> -			create_seq_dir() < 0)
> +			create_seq_dir(r) < 0)
>  		return -1;
>  	if (get_oid("HEAD", &oid) && (opts->action == REPLAY_REVERT))
>  		return error(_("can't revert as initial commit"));