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

[Junio C Hamano <gitster@pobox.com>]

Neil Horman <nhorman@tuxdriver.com> writes:

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

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