Re: [PATCH V2] git-apply: add --allow-empty flag

From: Jerry Zhang <jerry@skydio.com>
Date: Apr 28, 2021, 18:40 UTC
Message-ID: <CAMKO5CvHWDQNFoPeon99K97vUY0Y8WCbBcJghGyJT9XUr=nYqA@mail.gmail.com>
In-Reply-To: <xmqqmttjate1.fsf@gitster.g>

On Tue, Apr 27, 2021 at 10:08 PM Junio C Hamano <gitster@pobox.com> wrote:

Show 42 quoted lines

>
> Jerry Zhang <jerry@skydio.com> writes:
>
> > Some users or scripts will pipe "git diff"
> > output to "git apply" when replaying diffs
> > or commits. In these cases, they will rely
> > on the return value of "git apply" to know
> > whether the diff was applied successfully.
> >
> > However, for empty commits, "git apply" will
> > fail. This complicates scripts since they
> > have to either buffer the diff and check
> > its length, or run diff again with "exit-code",
> > essentially doing the diff twice.
> >
> > Add the "--allow-empty" flag to "git apply"
> > which allows it to handle both empty diffs
> > and empty commits created by "git format-patch
> > --always" by doing nothing and returning 0.
> >
> > Add tests for both with and without --allow-empty.
> >
> > Signed-off-by: Jerry Zhang <jerry@skydio.com>
> > ---
> > This patch applies on top of "git-apply: add --quiet flag".
> > The conflict is in Documentation -> Synopsis and is
> > trivial to solve.
>
> > V1 -> V2:
> > - Moved behavior under a flag
> > - Added tests for both cases
>
> When people add a flag (a boolean option), it becomes tempting to
> add a corresponding configuration variable.
>
> I wonder it this step should start calling
>
>         git apply --no-allow-empty
>
> when "git am" drives it, so that a futre end-user configuration
> variable would not break it?
>

That's currently not necessary, since "git am" can catch empty patches before it ever calls into apply.c:

     if (is_empty_or_missing_file(am_path(state, "patch"))) {
         printf_ln(_("Patch is empty."));
         die_user_resolve(state);
     }

If someone does make a config variable I think it might be convenient to teach "git am" to use "allow-empty" as well, since applying empty commits can be useful for documenting the history. The config variable could then control the behavior of both "am" and "apply" since they are used for very similar things. Of course that's up to the decision of that hypothetical person since I don't currently have a need to change "am".