Re: [GSoC][PATCH v3 2/3] cherry-pick/revert: add --skip option

[Junio C Hamano <gitster@pobox.com>]

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

> diff --git a/Documentation/git-cherry-pick.txt b/Documentation/git-cherry-pick.txt
> index 754b16ce0c..955880ab88 100644
> --- a/Documentation/git-cherry-pick.txt
> +++ b/Documentation/git-cherry-pick.txt
> @@ -10,9 +10,7 @@ SYNOPSIS
>  [verse]
>  'git cherry-pick' [--edit] [-n] [-m parent-number] [-s] [-x] [--ff]
>  		  [-S[<keyid>]] <commit>...
> -'git cherry-pick' --continue
> -'git cherry-pick' --quit
> -'git cherry-pick' --abort
> +'git cherry-pick' --continue | --skip | --abort | --quit

Is this correct, or do we need to enclose these choices inside (),
i.e.

	'git cherry-pick' ( --continue | --skip | --abort | --quit )

?  

> -static int rollback_single_pick(struct repository *r)
> +static int rollback_single_pick(struct repository *r, unsigned int is_skip)
>  {
>  	struct object_id head_oid;
>  
>  	if (!file_exists(git_path_cherry_pick_head(r)) &&
> -	    !file_exists(git_path_revert_head(r)))
> +	    !file_exists(git_path_revert_head(r)) && !is_skip)
>  		return error(_("no cherry-pick or revert in progress"));
>  	if (read_ref_full("HEAD", 0, &head_oid, NULL))
>  		return error(_("cannot resolve HEAD"));
> -	if (is_null_oid(&head_oid))
> +	if (is_null_oid(&head_oid) && !is_skip)
>  		return error(_("cannot abort from a branch yet to be born"));
>  	return reset_for_rollback(&head_oid);
>  }

It is unclear *why* the function (and more importantly, its callers)
would want to omit two sanity checks when is_skip is in effect.

Before this patch introduced such conditional behaviour, the name
was descriptive enough for this single-purpose function that is a
file-local helper, but it is no longer a case.  The function needs a
bit of commentary before it.

When &&-chaining error checks that are optional, check the condition
that makes the error checks optional first, i.e.

	if (!is_skip &&
		!file_exists(...) && !file_exists(...))
		return error(...);

The same comment applies to the "do not barf by checking is-null-oid
under is-skip mode, as that is a sign that we are on an unborn
branch and reset-for-rollback knows how to handle it now".

It may even be a good idea to group the checks that are guarded by
the condition for readability, i.e.

	if (!is_skip &&
		(!file_exists(...) && !file_exists(...)))
		return error(...);

> +int sequencer_skip(struct repository *r, struct replay_opts *opts)
> +{
> +	enum replay_action action = -1;
> +	sequencer_get_last_command(r, &action);
> +
> +	switch (opts->action) {
> +	case REPLAY_REVERT:
> +		if (!file_exists(git_path_revert_head(r))) {
> +			if (action == REPLAY_REVERT) {
> +				if (!rollback_is_safe())
> +					goto give_advice;
> +				else
> +					break;
> +			}
> +			return error(_("no revert in progress"));
> +		}

This part probably deserves a bit of in-code comment.  

    The Git subcommand (i.e. opts->action) tells us that we are
    asked to "git revert --skip".  When REVERT_HEAD is not there, we
    look at the last command of the sequencer state and make sure it
    is 'revert'; all other cases we barf.

That much we can read from the code.  But what are "all other cases"?
Do we cover a single-revert case (i.e. "git revert <commit>" got
conflict and the user is saying "git revert --skip")?  Was the user
in the middle of "git rebase -i" and the last command before we gave
the control back was 'pick'?

> +		break;
> +	case REPLAY_PICK:
> +		if (!file_exists(git_path_cherry_pick_head(r))) {
> +			if (action == REPLAY_PICK) {
> +				if (!rollback_is_safe())
> +					goto give_advice;
> +				else
> +					break;
> +			}
> +			return error(_("no cherry-pick in progress"));
> +		}
> +		break;
> +	default:
> +		BUG("the control must not reach here");
> +	}
> +
> +	if (rollback_single_pick(r, 1))
> +		return error(_("failed to skip the commit"));

And this takes us back to the previous comment.  By passing '1'
here, this caller is asking the callee to omit certain sanity check
the original version of the callee used to do.  What makes it an
appropriate thing to do so here?  "Because we reach at this point
under such and such condition, we would never have CHERRY_PICK_HEAD
or REVERT_HEAD---we do not want it to barf" is a good answer (and
no, do not merely give answer to me in your response---write the
answer as in-code comment to help future readers of the code).

"Because when we come here, sometimes the XXX_HEAD must exist but
some other times XXX_HEAD may not exist, so insisting that either
exists would make the function fail" is *NOT* a good answer, on the
other hand.  Somebody must still check that the necessary file
exists when it must exist.

Thanks.