Re: [PATCH v2 3/3] builtin/add: error out when passing untracked path with -u

[Junio C Hamano <gitster@pobox.com>]

Ghanshyam Thakkar <shyamthakkar001@gmail.com> writes:

> Well I think we should be going 'all-or-none' way as I can't think of
> any major user-facing command that does partial changes incase of
> error (besides two testcase below).

I agree that in the longer run, all-or-none would be something we
should aim for, but I'd strongly prefer leaving that outside this
topic, especially the existing ones that set exit_status to non-zero
but still commits the index changes.

I am OK, as a place to stop for now, if the topic had something like

+	if (take_worktree_changes) {
+		if (report_path_error(ps_matched, &pathspec))
+			exit(128);
+	}

in it, though, because this is a new behaviour.

> Doing this, we would need to take care of atleast 4 tests breaking in
> t3700-add:
>  error out when attempting to add ignored ones but add others
>  git add --ignore-errors
>  git add (add.ignore-errors)
>  git add --chmod fails with non regular files (but updates the other paths)
>
> while ignore-errors ones would be trivial to fix, fixing other 2 would
> probably require some more than trivial code changes, as from the title,
> their behavior seems pretty much set in stone. That's why I did the
> 'goto cleanup' approach to not break these.

I am not sure if these are expecting the right outcome in the first
place, and the need to examine what the right behaviour should be is
what makes me say "I do not want to make the all-or-none thing part
of this topic".

>> The renormalize() thing is not noticing unused pathspec elements,
>> which we might want to fix, but I suspect it is far less commonly
>> used mode of operation, so it may be OK to leave it to future
>> follow-up series.

Thanks.