Re: [PATCH v3 1/4] Introduce CHERRY_PICK_HEAD

From: Junio C Hamano <gitster@pobox.com>
Date: Feb 17, 2011, 20:01 UTC
Message-ID: <7vipwibfc5.fsf@alter.siamese.dyndns.org>
In-Reply-To: <1297916325-89688-2-git-send-email-jaysoffian@gmail.com>

Jay Soffian <jaysoffian@gmail.com> writes:

Show 15 quoted lines

> diff --git a/builtin/merge.c b/builtin/merge.c
> index 9403747..454dad2 100644
> --- a/builtin/merge.c
> +++ b/builtin/merge.c
> @@ -971,6 +971,13 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
>  		else
>  			die("You have not concluded your merge (MERGE_HEAD exists).");
>  	}
> +	if (file_exists(git_path("CHERRY_PICK_HEAD"))) {
> +		if (advice_resolve_conflict)
> +			die("You have not concluded your cherry-pick (CHERRY_PICK_HEAD exists).\n"
> +			    "Please, commit your changes before you can merge.");
> +		else
> +			die("You have not concluded your cherry-pick (CHERRY_PICK_HEAD exists).");
> +	}

Micronit: "Please, commit your changes before you can merge."

 - We are not merging in this codepath to begin with;

 - I'd suggest rephrasing this (together with "MERGE_HEAD" codepath) to
   something like:

	"Commit your changes first before retrying."

Show 8 quoted lines

> +test_expect_success 'cherry-pick --no-commit sets CHERRY_PICK_HEAD' '
> +
> +	git checkout -f initial^0 &&
> +	git read-tree -u --reset HEAD &&
> +	git clean -d -f -f -q -x &&
> +
> +	git update-index --refresh &&
> +	git diff-index --exit-code HEAD &&

Getting tired of seeing these five lines repeated over and over. Perhaps it is time to introduce:

	pristine_detach () {
        	git checkout -f "$1^0" &&
                git read-tree -u --reset HEAD &&
                git clean -d -f -f -q -x
	}

I don't think "diff-index --exit-code HEAD" is necessary as the point of this testsuite is not about testing "read-tree --reset", and the index refresh is just a prerequisite for diff-index that can go together with it.

> +	git cherry-pick --no-commit base &&
> +
> +	test_cmp_rev base CHERRY_PICK_HEAD

If the next "git commit" would notice and use this information, that would introduce an unpleasant regression to one use case in my workflow, which is to pick and consolidate one or more small pieces made on a private "misc" branch, possibly with a bit of further work, into a new commit with a readable explanation that is unrelated to any of the original commits:

	git cherry-pick --no-commit $some_commit
	git cherry-pick --no-commit $another_commit ;# optional
	edit ;# optional
        git commit

I'd prefer to see a way to tell cherry-pick not to leave CHERRY_PICK_HEAD behind when "cherry-pick --no-commit" results in a successful cherry-pick to avoid a backward incompatibility surprise. Otherwise people need to retrain their fingers to say --reset-author when they run "git commit".

Other then the above three points, this patch looks good.  Thanks.