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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Chris Packham <judge.packham@xxxxxxxxx> writes:

> +'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).

>  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).

> @@ -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.

> @@ -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.

> @@ -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.




[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]