Ghanshyam Thakkar <shyamthakkar001@xxxxxxxxx> writes:
> On Fri, 29 Mar 2024, Junio C Hamano <gitster@xxxxxxxxx> wrote:
>> Ghanshyam Thakkar <shyamthakkar001@xxxxxxxxx> writes:
>> > + if (take_worktree_changes)
>> > + exit_status |= report_path_error(ps_matched, &pathspec);
>>
>> Hmph, are we sure take_worktree_changes is true only when
>> add_renormalize is false?
>>
>> > if (add_new_files)
>> > exit_status |= add_files(&dir, flags);
>>
>> If report_path_error() detected that the pathspec were faulty,
>> should we still proceed to add new files? This is NOT a rhetorical
>> question, as I do not know the answer myself. I do not even know
>> offhand what add_files_to_cache() above did when pathspec elements
>> are not all consumed---if it does not complain and does not refrain
>> from doing any change to the index, then we should follow suite and
>> add_files() here, too.
> Sorry if I'm missing something, but in your last line after '---', do you mean
> that we should proceed even after report_path_error() detected error like in
> the above patch or perhaps something like this:
We roughly do:
if (add_renorm)
exit_status |= renorm();
else
exit_status |= add_files_to_cache();
+ if (take_worktree_changes)
+ exit_status |= report_path_error();
if (add_new_files)
exit_status |= add_files();
I was wondering if we should refrain from adding new files when we
exit_status is true to avoid making "further damage", and was
wondering if the last one should become:
if (!exit_status && add_new_files)
exit_status |= add_files();
But that was merely because I was not thinking things through. If
we were to go that route, the whole thing needs to become (because
there are other things that notice errors before this part of the
code):
if (!exit_status) {
if (add_renorm)
exit_status |= renorm();
else
exit_status |= add_files_to_cache();
}
if (!exit_status && take_worktree_changes)
exit_status |= report_path_error();
if (!exit_status && add_new_files)
exit_status |= add_files();
but (1) that is far bigger change of behaviour to the code than
suitable for "notice unmatched pathspec elements and report an
error" topic, and more importantly (2) it is still not sufficient to
make it "all-or-none". E.g., if "add_files_to_cache()" call added
contents from a few paths and then noticed that some pathspec
elements were not used, we are not restoring the previous state to
recover. The damage is already done, and not making further damage
does not help the user all that much.
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.
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.
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.
