Re: [PATCH v4 2/4] git-cherry-pick: Add keep-redundant-commits option

From: Junio C Hamano <gitster@pobox.com>
Date: Apr 12, 2012, 03:15 UTC
Message-ID: <7vty0phc8s.fsf@alter.siamese.dyndns.org>
In-Reply-To: <20120411235509.GB16937@hmsreliant.think-freely.org>

Neil Horman <nhorman@tuxdriver.com> writes:

Show 23 quoted lines

>> > -static int run_git_commit(const char *defmsg, struct replay_opts *opts)
>> > +int run_git_commit(const char *defmsg, struct replay_opts *opts, int empty)
>> >  {
>> > -	/* 7 is max possible length of our args array including NULL */
>> > -	const char *args[7];
>> > -	int i = 0;
>> > +	struct argv_array array;
>> > +	int rc;
>> > +
>> > +	if (!empty && !opts->keep_if_made_empty) {
>> > +		unsigned char head_sha1[20];
>> > +		struct commit *head_commit;
>> > +		int need_free = 0;
>> > +
>> > +		resolve_ref_unsafe("HEAD", head_sha1, 1, NULL);
>> > +		head_commit = lookup_commit(head_sha1);
>> 
>> No error checking whatsoever?  HEAD might be pointing at a branch that
>> hasn't been born, for example.
>> 
> Sorry, I had presumed that HEAD could be guaranteed to be good here, given that
> we validate it in rollback_single_pick via read_ref_full.  But I can add
> additional checking here.

As long as it is clear that we always have "HEAD" here, it is not strictly necessary, but we'd probably feel better, especially now that the function is (for some reason) made into a global one and may gain other new callers outside this file scope.

Show 13 quoted lines

>> >  	if (opts->allow_empty)
>> > -		args[i++] = "--allow-empty";
>> > +		argv_array_push(&array, "--allow-empty");
>> 
>> If --keep-if-made-empty is not given but --allow-empty was, it is fine to
>> give --allow-empty here, but otherwise, it logically is iffy and is likely
>> to become a cause of future bugs to pass --allow-empty to "git commit",
>> even though the earlier check _ought_ to catch problematic cases, no?
>> 
> Not sure I follow your reasoning here.  We need to pass allow-empty to git
> commit here if git cherry-pick set either allow-empty or keep-redundant-commits
> (the latter setting opts->empty), Can you give an example of what might be
> problematic in the future?

Thinking about it again, I think this part of your patch is correct.

We may pass an index that yields the same tree as "HEAD" if the original was empty, or the original was not empty but the merge found the commit unneeded. In either case, if "--keep-unnecessary" or "--allow-empty" was given, we want to record the empty commit, and opts->allow_empty is set when either option was given from the command line.

Thanks.