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

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