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

From: Junio C Hamano <gitster@pobox.com>
Date: Apr 1, 2024, 16:31 UTC
Message-ID: <xmqqjzlhavu0.fsf@gitster.g>
In-Reply-To: <h7yk7nk7cwyv35reqzfy7brpbn3xoaarhudteyvxfpkodvltt2@eggaahzrjryq>

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.

Show 11 quoted lines

> 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.