From: Junio C Hamano <gitster@pobox.com>
To: Ghanshyam Thakkar <shyamthakkar001@gmail.com>
Cc: git@vger.kernel.org
Subject: Re: [PATCH v2 3/3] builtin/add: error out when passing untracked path with -u
Date: Sat, 30 Mar 2024 09:49:15 -0700 [thread overview]
Message-ID: <xmqqh6gnmzqs.fsf@gitster.g> (raw)
In-Reply-To: <b3j7l2ncstdiaxojtollxddmxvkbbeciou25yptguttr5qugmx@y3bzqbdxkyaw> (Ghanshyam Thakkar's message of "Sat, 30 Mar 2024 19:48:11 +0530")
Ghanshyam Thakkar <shyamthakkar001@gmail.com> writes:
> 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.
next prev parent reply other threads:[~2024-03-30 16:49 UTC|newest]
Thread overview: 31+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-03-18 15:51 [PATCH 0/2] fix certain cases of add and commit with untracked path not erroring out Ghanshyam Thakkar
2024-03-18 15:51 ` [PATCH 1/2] builtin/commit: error out when passing untracked path with -i Ghanshyam Thakkar
2024-03-18 17:27 ` Junio C Hamano
2024-03-18 15:52 ` [PATCH 2/2] builtin/add: error out when passing untracked path with -u Ghanshyam Thakkar
2024-03-18 17:31 ` Junio C Hamano
2024-03-29 20:56 ` [PATCH v2 0/3] commit, add: error out when passing untracked path Ghanshyam Thakkar
2024-04-02 21:36 ` [PATCH v3 0/3] commit, add: error out when passing untracked paths Ghanshyam Thakkar
2024-04-03 18:14 ` [PATCH v4 0/3] commit,add: error out when passing untracked path Ghanshyam Thakkar
2024-04-03 18:14 ` [PATCH v4 1/3] revision: optionally record matches with pathspec elements Ghanshyam Thakkar
2024-04-03 18:14 ` [PATCH v4 2/3] builtin/commit: error out when passing untracked path with -i Ghanshyam Thakkar
2024-04-03 18:14 ` [PATCH v4 3/3] builtin/add: error out when passing untracked path with -u Ghanshyam Thakkar
2024-04-02 21:36 ` [PATCH v3 1/3] revision: optionally record matches with pathspec elements Ghanshyam Thakkar
2024-04-02 21:36 ` [PATCH v3 2/3] builtin/commit: error out when passing untracked path with -i Ghanshyam Thakkar
2024-04-02 21:47 ` Junio C Hamano
2024-04-02 21:58 ` Ghanshyam Thakkar
2024-04-02 21:36 ` [PATCH v3 3/3] builtin/add: error out when passing untracked path with -u Ghanshyam Thakkar
2024-04-02 21:49 ` Junio C Hamano
2024-04-02 22:00 ` Ghanshyam Thakkar
2024-03-29 20:56 ` [PATCH v2 1/3] read-cache: optionally collect pathspec matching info Ghanshyam Thakkar
2024-03-29 21:35 ` Junio C Hamano
2024-03-29 22:16 ` Junio C Hamano
2024-03-30 14:27 ` Ghanshyam Thakkar
2024-03-30 16:27 ` Junio C Hamano
2024-03-29 20:56 ` [PATCH v2 2/3] builtin/commit: error out when passing untracked path with -i Ghanshyam Thakkar
2024-03-29 21:38 ` Junio C Hamano
2024-03-29 20:56 ` [PATCH v2 3/3] builtin/add: error out when passing untracked path with -u Ghanshyam Thakkar
2024-03-29 21:43 ` Junio C Hamano
2024-03-30 14:18 ` Ghanshyam Thakkar
2024-03-30 16:49 ` Junio C Hamano [this message]
2024-04-01 13:27 ` Ghanshyam Thakkar
2024-04-01 16:31 ` Junio C Hamano
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=xmqqh6gnmzqs.fsf@gitster.g \
--to=gitster@pobox.com \
--cc=git@vger.kernel.org \
--cc=shyamthakkar001@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).