Re: [PATCH] commit: make --only --allow-empty work without paths
- From
Jeff King <peff@peff.net>
- Date
- Dec 3, 2016, 04:32 UTC
- Message-ID
- <20161203043254.7ozjyucfn6uivnsh@sigill.intra.peff.net>
- In-Reply-To
- <20161202221513.GA5370@inner.h.apk.li>
On Fri, Dec 02, 2016 at 11:15:13PM +0100, Andreas Krey wrote:
> --only is implied when paths are present, and required > them unless --amend. But with --allow-empty it should > be allowed as well - it is the only way to create an > empty commit in the presence of staged changes.
OK. I'm not sure why you would want to create an empty commit in such a case. But I do agree that this seems like a natural outcome for "--only --allow-empty". So whether it is particularly useful or not, it seems like the right thing to do. The patch itself looks good to me.
> Arguably, requiring paths with --only is > pointless anyway because it is implicit > in that case, but I'm happy when it works > like in this patch.
I think the point is just to warn the user that what they've asked for is by definition a noop (and that's why there's already an exception for --amend, which _does_ make it do something). The fact that --only is implicit with paths is mostly historical; at one point it was not the default. These days it's unnecessary, but retained for backwards compatibility.
> (The interdepence of the tests is a strange thing; > making --run=N somewhat pointless.)
Yes, I think --run is a misfeature (I actually had to look it up, as I had completely forgotten that it was added). It's too hard to know which tests are required setup for later ones, and often the dependency is implicit. If a single test script is annoyingly long to run, I'd argue it should be broken out into its own script (and that will let it run in parallel when the full suite is run, too). I don't know that t7501 qualifies, though; it runs in about 800ms on my machine.
Show 13 quoted lines
> diff --git a/builtin/commit.c b/builtin/commit.c
> index 8976c3d29..89b66816f 100644
> --- a/builtin/commit.c
> +++ b/builtin/commit.c
> @@ -1206,7 +1206,7 @@ static int parse_and_validate_options(int argc, const char *argv[],
>
> if (also + only + all + interactive > 1)
> die(_("Only one of --include/--only/--all/--interactive/--patch can be used."));
> - if (argc == 0 && (also || (only && !amend)))
> + if (argc == 0 && (also || (only && !amend && !allow_empty)))
> die(_("No paths with --include/--only does not make sense."));
> if (argc == 0 && only && amend)
> only_include_assumed = _("Clever... amending the last one with dirty index.");I think this should be sufficient. Obviously we'll end up with an empty commit, but allow_empty should cover that case later on.
Show 17 quoted lines
> diff --git a/t/t7501-commit.sh b/t/t7501-commit.sh > index d84897a67..0d8d89309 100755 > --- a/t/t7501-commit.sh > +++ b/t/t7501-commit.sh > @@ -155,6 +155,15 @@ test_expect_success 'amend --only ignores staged contents' ' > git diff --exit-code > ' > > +test_expect_success 'allow-empty --only ignores staged contents' ' > + echo changed-again >file && > + git add file && > + git commit --allow-empty --only -m "empty" && > + git cat-file blob HEAD:file >file.actual && > + test_cmp file.expect file.actual && > + git diff --exit-code > +' > +
Usually we'd put new tests at the end. I guess you wanted this here to match the "--amend --only" test before it. That kind of sticks this oddball in the middle of a bunch of --amend tests, but I'm not sure it would go better anywhere else. So I'm fine with it here.
-Peff