Re: [PATCH v2 0/1] Make 'git commit' not accidentally lose staged content

From: Duy Nguyen <pclouds@gmail.com>
Date: Sep 17, 2018, 17:29 UTC
Message-ID: <CACsJy8C5QOLvg4pzy_pThQoyGh9ohdeVHXsuYwQHQypn3oBxkw@mail.gmail.com>
In-Reply-To: <xmqq1s9s82zx.fsf@gitster-ct.c.googlers.com>

On Mon, Sep 17, 2018 at 7:09 PM Junio C Hamano <gitster@pobox.com> wrote:

Show 20 quoted lines

>
> Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:
>
> > This is about mixing "git add -p" and "git commit -a" (or "git commit
> > <path>") where you may accidentally lose staged changes. After the
> > discussion with Jonathan, I'm going with a bit different approach than
> > v1, this behavior now becomes default, and if the user wants the old
> > behavior back, they can use --clobber-index.
> >
> > Another change is "git commit <path>" is covered as well, as pointed
> > out by Jacob.
> >
> > I will need to add some test cases of course, if this direction is
> > still promising. One thing I'm not sure about is whether want to
> > deliberately clobber the index often, then perhaps we should add a
> > config key to bring the old behavior back.
>
> It usually is safer (simply because you do not have to think about
> it) to start a behaviour change like this as a strict opt-in to gain
> confidence.

Heh. The RFC was opt-in. Jonathan suggested changing default behavior and I went along just to see how far I could push it :)

Show 11 quoted lines

> As I often see myself futzing with the same file, adding changes to
> it incrementally, so that I can view progress in "diff --cached" and
> "diff" output, it would be a serious usability regression if the
> last step in the following sequence is rejected by default:
>
>         edit X
>         git add X
>         git diff --cached X
>         git diff X
>         ... repeat the above number of times ...
>         git commit X ;# or "git commit -a" to finally conclude

But yes if there's a valid use case where this behavior change becomes a problem, then opt-in makes more sense.

Show 18 quoted lines

> On the other hand, if I am keeping some change that should never be
> in a commit in the working tree file, and building the contents in
> the index using "add -p" to incrementally, it would be the same
> disaster as you are trying to prevent if I by mistake did a whole
> path 'add', even if I catch myself doing so before running 'commit'
> i.e.
>
>         edit X
>         git add -p X
>         git diff --cached X
>         git diff X
>         ... repeat the above number of times ...
>         git add X ;# OOPS!
>         git add . ;# OOPS! even worse!
>
> Even though this does not involve "git commit -a" or "git commit X",
> an unrecoverable damage that requires redoing the manual work is
> already done.

I don't see a good way to get to recover this situation. I could go back to the "index log" idea, where we keep a log of index changes (or just "interesting" changes). That way there's no behavior change at all. The user who accidentally updates/deletes something can always retrieve the old content back (assuming that they realize quickly since we can't keep very long log).

I've been thinking about allowing to undo worktree changes too (e.g. accidental "git reset --hard") and this log can cover it as well.

The only downside is we need a new command for the UI (or perhaps I can just add "git add --log" or something like that).

Should I just drop this patch and go that direction instead? More work, but maybe better end result.

> How should this new check intract with paths added with "add -N", by
> the way?

Good point. The check here is basically "git diff --cached". I would need to turn it to "git diff --cached --ita-invisible-in-index" to make ita entries disappear.

-- 
Duy