Re: [PATCH v2 2/3] cherry-pick/revert: add scissors line on merge conflict

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 7, 2019, 07:52 UTC
Message-ID: <xmqqa7i7m886.fsf@gitster-ct.c.googlers.com>
In-Reply-To: <8fdc5bfb151e4224eba01021c6f0be3e61e1f535.1551940635.git.liu.denton@gmail.com>

Denton Liu <liu.denton@gmail.com> writes:

Show 11 quoted lines

> Fix a bug where the scissors line is placed after the Conflicts:
> section, in the case where a merge conflict occurs and
> commit.cleanup = scissors.
>
> Note that the removal of the if-else tower in git_sequencer_config may
> appear to be a no-op refactor but it actually isn't. First of all, we
> now accept "default". More importantly, though, if
> commit.cleanup = scissors, the cleanup enum will be set to
> COMMIT_MSG_CLEANUP_SCISSORS instead of COMMIT_MSG_CLEANUP_SPACE when we
> are reverting or cherry-picking. This allows us to append scissors to
> MERGE_MSG in the case of a conflict.

Good thing to point out in the log message.

Show 16 quoted lines

> diff --git a/Documentation/git-cherry-pick.txt b/Documentation/git-cherry-pick.txt
> index d35d771fc8..5c086d78c8 100644
> diff --git a/Documentation/git-revert.txt b/Documentation/git-revert.txt
> index 837707a8fd..1894010e60 100644
> --- a/Documentation/git-revert.txt
> +++ b/Documentation/git-revert.txt
> @@ -66,6 +66,13 @@ more details.
>  	With this option, 'git revert' will not start the commit
>  	message editor.
>  
> +--cleanup=<mode>::
> +	This option determines how the commit message will be cleaned up before
> +	being passed on. See linkgit:git-commit[1] for more details. In
> +	addition, if the '<mode>' is given a value of `scissors`, scissors will
> +	be prepended to MERGE_MSG before being passed on in the case of a
> +	conflict.

These both say "prepended", but shouldn't the code

 - add the merge message proper, with the expectation that it would
   be used more or less intact in the final commit message; then

 - add the scissors; then

 - append informative cruft that would be removed, only to help
   human users.

in this order? I'd expect that most people would consider that the primary payload of MERGE_MSG file is the part that would become the commit message, so I would have expected the second step would be described as "appended to".

Another thing I notice is that this singles out "scissors" mode; doesn't the code do anything worth describing with other clean-up modes?

> +	sed "s/[a-f0-9]\{40\}/objid/" .git/MERGE_MSG >actual &&

Use $_x40 (or $OID_REGEX) to make it more readable and also future-proof?