Re: [PATCH v18 3/3] am: support --allow-empty to record specific empty patches

> > +	index_changed = repo_index_has_changes(the_repository, NULL, NULL);
> > +	if (allow_empty &&
> > +	    !(!index_changed && is_empty_or_missing_file(am_path(state, "patch"))))
> >  		die_user_resolve(state);
> 
> Why do we need this?  
> 
> Intuitively, because "--allow-XYZ" is "we normally die when the
> condition XYZ holds, but with the option, we do not die in such a
> case", any new condition that leads to "die" that only kicks in
> when "--allow-XYZ" is given smell like a BUG.
> 
> The condition reads:
> 
>     unless we saw an empty patch (i.e. "patch" file is missing or
>     empty, and did not result in any change to the index), it is an
>     error to give "--allow-empty" to the command.
> 
> That somehow does not make any sense to me.  Whether failed patch
> application and manual tweaking resulted in the same tree as HEAD,
> or a tree different from HEAD, if the user wants to say "I allow
> Git to create an empty commit as necessary, instead of stopping",
> shouldn't the user be allowed to?  After all, the option is not
> "create nothing but an empty commit, it is an error if there is
> something to commit."  It merely allows creation of an empty one.
> 
> This series has been trying to add code to punish users who give
> "--allow-empty" when the command would have worked the same way
> without it at least since the last round, and I am not sure where
> that wish to stop users comes from.  Please explain.  I am starting
> to think I may be missing something and this extra rigidity may be
> helping a user (but again, I do not see how).

> > +	if (!index_changed) {
> > +		if (allow_empty) {
> 
> The index_changed variable being false is "the result is an empty
> change", and is-empty-or-missing on "patch" file is "the input is an
> empty change".  Ideally we would want to create an empty commit only
> when both input and result are empty, but in order to prevent mistakes,
> we may want to react to the case where the result is empty but the
> input is not empty.
> 
> Here is where we can use is-empty-or-missing on "patch" and give a
> different message (i.e. "even though the patch file is not empty,
> the result of your patch application did not change anything"), if
> we wanted to.  Or you could choose to reject such an attempt, which
> might be simpler.  I.e.
> 
> 		if (allow_empty &&
> 		    is_empty_or_missing_file(am_path(state, "patch"))
> 			"No changes - recorded an empty commit."
> 		else
> 			... the original error for no changes ...
> 
> 
> Hmm?