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

From: Ghanshyam Thakkar <shyamthakkar001@gmail.com>
Date: Apr 1, 2024, 13:27 UTC
Message-ID: <h7yk7nk7cwyv35reqzfy7brpbn3xoaarhudteyvxfpkodvltt2@eggaahzrjryq>
In-Reply-To: <xmqqh6gnmzqs.fsf@gitster.g>

On Sat, 30 Mar 2024, Junio C Hamano <gitster@pobox.com> wrote:

> So, it was a fairly pointless thing that I was wondering about.  The
> current behaviour, and the new behaviour with the new check, are
> fine as-is.

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

Show 16 quoted lines

> If we wanted to make it "all-or-none", I think the way to do so is
> to tweak the final part of the cmd_add() function to skip committing
> the updated index, e.g.,
> 
>          finish:
>         -	if (write_locked_index(&the_index, &lock_file,
>         +	if (exit_status)
>         +		fputs(_("not updating the index due to failure(s)\n"), stderr);
>         +	else if (write_locked_index(&the_index, &lock_file,
>                                        COMMIT_LOCK | SKIP_IF_UNCHANGED))
>                         die(_("unable to write new index file"));
>  
> And if/when we do so, the existing code (with or without the updates
> made by the topic under discussion) needs no change.  We can do all
> steps regardless of the errors we notice along the way with earlier
> steps, and discard the in-core index if we saw any errors.

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.

Thanks.

Show 6 quoted lines

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