Re: [PATCHv2 1/2] merge: Add '--continue' option as a synonym for 'git commit'

From: Junio C Hamano <gitster@pobox.com>
Date: Dec 13, 2016, 18:02 UTC
Message-ID: <xmqq60mn671x.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <20161213084859.13426-1-judge.packham@gmail.com>

Chris Packham <judge.packham@gmail.com> writes:

Show 10 quoted lines

> +'git merge' --continue
>  
>  DESCRIPTION
>  -----------
> @@ -61,6 +62,9 @@ reconstruct the original (pre-merge) changes. Therefore:
>  discouraged: while possible, it may leave you in a state that is hard to
>  back out of in the case of a conflict.
>  
> +The fourth syntax ("`git merge --continue`") can only be run after the
> +merge has resulted in conflicts.

OK. I can see that the code refuses if there is no MERGE_HEAD, so "can only be run" is ensured correctly.

> 'git merge --continue' will take the
> +currently staged changes and complete the merge.

For Git-savvy folks, this may be sufficient to tell that they are expected to resolve conflicts in the working tree and register the resolusion by doing "git add" before running "git merge --continue", but I wonder if that is clear enough for new readers.

The same comment applies to the option description below. I suspect that it is better to remove the last sentence above, leaving "4th one can be run only with MERGE_HEAD" here, and enhance the explanation in the option description (see below).

Show 12 quoted lines

>  OPTIONS
>  -------
> @@ -99,6 +103,12 @@ commit or stash your changes before running 'git merge'.
>  'git merge --abort' is equivalent to 'git reset --merge' when
>  `MERGE_HEAD` is present.
>  
> +--continue::
> +	Take the currently staged changes and complete the merge.
> ++
> +'git merge --continue' is equivalent to 'git commit' when
> +`MERGE_HEAD` is present.
> +

These two sentences are even more technical and unfriendly to new readers, I am afraid. How about giving a hint by referring to an existing section, like this?

    --continue::
        After a "git merge" stops due to conflicts you can conclude
        the merge by running "git merge --continue" (see "How to
        resolve conflicts" section below).

Show 7 quoted lines

> @@ -277,7 +287,8 @@ After seeing a conflict, you can do two things:
>  
>   * Resolve the conflicts.  Git will mark the conflicts in
>     the working tree.  Edit the files into shape and
> -   'git add' them to the index.  Use 'git commit' to seal the deal.
> +   'git add' them to the index.  Use 'git merge --continue' to seal the
> +   deal.

Why do we want to make it harder to discover "git commit" here? I would understand:

	... Use 'git commit' to conclude (you can also say 'git
	merge --continue').

though. After all, we are merely introducing a synonym for those who want to type more. There is no plan to deprecate the use of 'git commit', which is a perfectly reasonable way to conclude an interrupted merge, that has worked well for us for the past 10 years and still works.

Show 23 quoted lines

> @@ -65,6 +66,7 @@ static int option_renormalize;
>  static int verbosity;
>  static int allow_rerere_auto;
>  static int abort_current_merge;
> +static int continue_current_merge;
>  static int allow_unrelated_histories;
>  static int show_progress = -1;
>  static int default_to_upstream = 1;
> @@ -223,6 +225,8 @@ static struct option builtin_merge_options[] = {
>  	OPT__VERBOSITY(&verbosity),
>  	OPT_BOOL(0, "abort", &abort_current_merge,
>  		N_("abort the current in-progress merge")),
> +	OPT_BOOL(0, "continue", &continue_current_merge,
> +		N_("continue the current in-progress merge")),
>  	OPT_BOOL(0, "allow-unrelated-histories", &allow_unrelated_histories,
>  		 N_("allow merging unrelated histories")),
>  	OPT_SET_INT(0, "progress", &show_progress, N_("force progress reporting"), 1),
> @@ -739,7 +743,7 @@ static void abort_commit(struct commit_list *remoteheads, const char *err_msg)
>  	if (err_msg)
>  		error("%s", err_msg);
>  	fprintf(stderr,
> -		_("Not committing merge; use 'git commit' to complete the merge.\n"));
> +		_("Not committing merge; use 'git merge --continue' to complete the merge.\n"));

Likewise.  I do not see a need to change this one at all.

Show 11 quoted lines

> @@ -1166,6 +1170,22 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
>  		goto done;
>  	}
>  
> +	if (continue_current_merge) {
> +		int nargc = 1;
> +		const char *nargv[] = {"commit", NULL};
> +
> +		if (argc)
> +			usage_msg_opt("--continue expects no arguments",
> +			      builtin_merge_usage, builtin_merge_options);

Peff already commented on "what about other options?", and I think his "check the number of args before parse-options ran to ensure that the '--abort' or '--continue' was the only thing" is probably a workable hack.

The "right" way to fix it would be way too involved to be worth for just this single change (and also fixing "--abort"). Just thinking aloud:

 * Update parse-options API to:

   - extend "struct option" with one field that holds what "command
     modes" the option the "struct option" describes is incompatible
     with.

   - make parse_options() to keep track of the set of command modes
     that are compatible with the options seen so far, and complain
     if an option that is not compatible with the command mode is
     given.

 * Use the above facility to update "git merge" so that --abort and
   --continue becomes OPT_CMDMODE.

Then, the updated parse_options() would:

 - start by making the "incompatible command modes" an empty set.

 - while it processes each option on the command line:

   - if it is not an OPTION_CMDMODE, add the set of command modes
     that are incompatible with the option to the "incompatible
     command modes".

   - if it is an OPTION_CMDMODE and we already saw another
     OPTION_CMDMODE, error out (we already do this).

 - after all options are read, check the final command mode and see
   if that is in "incompatible command modes".

You can mark almost all options "git merge" takes except a selected few like "--quiet" as incompatible with "--abort" and "--continue" and let parse_options() catch incompatible options. Of course you still need to check argc for non-option here.