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

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 30, 2024, 16:49 UTC
Message-ID: <xmqqh6gnmzqs.fsf@gitster.g>
In-Reply-To: <b3j7l2ncstdiaxojtollxddmxvkbbeciou25yptguttr5qugmx@y3bzqbdxkyaw>

Ghanshyam Thakkar <shyamthakkar001@gmail.com> writes:

Show 21 quoted lines

> On Fri, 29 Mar 2024, Junio C Hamano <gitster@pobox.com> wrote:
>> Ghanshyam Thakkar <shyamthakkar001@gmail.com> 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.