On Sat, 30 Mar 2024, Junio C Hamano <gitster@xxxxxxxxx> 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).
> 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.
> 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.
