Re: Fwd: Delivery Status Notification (Failure)

[Jeff King <peff@peff.net>]

On Sat, Apr 06, 2013 at 12:15:33AM -0400, Drew Gross wrote:

> I'm am trying to patch git to refuse to commit if there are both staged and
> unstaged changes, and I pass the -a flag. I expected this simple addition
> to parse_and_validate_options() in commit.c to accomplish most of what I
> want:
> 
> if (all && s->workdir_dirty)
>     die(_("Cannot commit with -a if there are staged and unstaged changes"));
> 
> But when compiled, this will commit anyway even with staged and unstaged
> files. I think I misunderstanding the workdir_dirty flag. Can someone help
> me?

I am not sure if that is a good safety measure in general. But
ignoring that question for a moment, there are a few issues with your
proposed implementation:

  1. workdir_dirty only talks about unstaged changes; it sounds like you
     want to check for both staged and unstaged.

  2. no flags in in the wt_status struct are set until wt_status_collect
     is called; you need to put your check later. But of course by the
     time we call it, we have already updated the index, so you would
     not know anymore which changes were already in the index and which
     were added by "-a".

  3. we do not always run wt_collect_status (e.g., if you are not going
     to run an editor, we do not bother going to the effort to create
     the commit template).

So you'd probably need something more like this:

diff --git a/builtin/commit.c b/builtin/commit.c
index 4620437..ebb5480 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -1061,6 +1061,10 @@ static int parse_and_validate_options(int argc, const char *argv[],
 	if (status_format != STATUS_FORMAT_NONE)
 		dry_run = 1;
 
+	wt_status_collect(s);
+	if (all && s->change.nr && s->workdir_dirty)
+		die("Cannot use '-a' with staged and unstaged changes");
+
 	return argc;
 }
 

Note that this may run the diff-index and diff-files procedures twice
(once here, and once later if we actually call run_status). If were
doing this for real (and I do not think it is something we want to take
upstream anyway), you would want to make sure that information was
cached.

-Peff