git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / code / Atom feed
From: Jerry Zhang <jerry@skydio.com>
To: Junio C Hamano <gitster@pobox.com>
Cc: Git Mailing List <git@vger.kernel.org>,
	Elijah Newren <newren@gmail.com>, Ross Yeager <ross@skydio.com>,
	Abraham Bachrach <abe@skydio.com>,
	Brian Kubisiak <brian.kubisiak@skydio.com>,
	Jerry Zhang <jerryxzha@googlemail.com>
Subject: Re: [PATCH V2] git-apply: Allow simultaneous --cached and --3way options
Date: Tue, 6 Apr 2021 19:25:41 -0700	[thread overview]
Message-ID: <CAMKO5Cv01vFde5XqrM0Niu1jSrhMe=bdmc15KN1Uo8WP9cNKhw@mail.gmail.com> (raw)
In-Reply-To: <CAMKO5Ctoa8cf9T0reE9DduC7oX8QgQw-sQH315mQN=KiLDS8ag@mail.gmail.com>

On Tue, Apr 6, 2021 at 2:56 PM Jerry Zhang <jerry@skydio.com> wrote:
>
> On Mon, Apr 5, 2021 at 10:52 PM Junio C Hamano <gitster@pobox.com> wrote:
> >
> > Jerry Zhang <jerry@skydio.com> writes:
> >
> > > Thanks for the comments! I've updated v3 with the changes. Let me know
> > > if you have any
> > > more thoughts on whether to block / warn the user before clobbering their cache.
> >
> > Please do not top-post on this list.
> >
> > I've already said that I think we should ensure the index is clean
> > by default, because, unlike the case where the application is done
> > on the working tree files, the use of "--cached" is a sign that the
> > next step is likely to write a tree out.  As I've already said so in
> > earlier reviews, there is nothing more from me to add on that issue.
> Understood, but please bear with me to explain the risks a bit more. I'm
> having some difficulty coming up with a name and explanation for flags
> for this case, because I don't completely understand the safety issue
> we are trying to mitigate.
>
> Let me enumerate some behaviors in 3 different cases where the user
> has "file.txt" changes staged in the index, so index differs from HEAD.
>
> "git apply --cached" would either 1. combine the patch and cached version
> and put that in the cache or 2. do nothing (patch failed). In 2 nothing happened
> so the user's changes are safe. In 1 the user's changes may be gone, but
> since the user was forewarned, this is presumably what they wanted.
>
> "git apply --3way" would either 1. apply cleanly to working dir or
> 2. conflict, in which case user's changes would be moved to stage #2
> in cache. For 1 the user's changes are in the cache, so they can check that out
> to restore the original state, since this invocation requires the cache
> and working dir to match. For 2, the user's changes are moved to cache
> in stage #2. Although the changes are preserved, there doesn't seem to
> be any atomic way to move a cache entry from stage #2 to stage #0.
> Something like "git restore --staged --ours file.txt" seems like it should
> work, but "git restore" doesn't allow combining those flags.
> The non atomic way we can do is "git checkout --ours file.txt &&
> git add file.txt", this is ok in this case since we've required the index
> and working tree to match.
>
> "git apply --3way --cached" would either 1. apply cleanly to the cache or
> 2. conflict, and the user's changes are moved to stage #2. In 1, the user's
> changes are lost because they're combined with the patch, but this is
> the same as the "--cached" case by itself. In 2, the user's changes are
> preserved in stage #2 similar to "--3way" by itself. What's somewhat
> tricky here is restoring it to stage #0 since we can't use the working
> tree, but I think that is more of a limitation in "git restore", since moving
> a cache entry from stage #2 to stage #0 is a conceptually possible and
> simple operation.
Update: I found out that this can be done through "git update-index"
Say you start with
```
100755 adc1032303de8d262868c0a1b85a00fa3b97e9d8 1 file.sh
100755 d0bf2e33594ea7d9d7352df5511db562de819518 2 file.sh
100755 e9bf5dfdea804f4f1bcf24988bbc7c3f99991a09 3 file.sh
```
Pass into "git update-index --index-info the following
```
100755 d0bf2e33594ea7d9d7352df5511db562de819518 0 file.sh
0 0 1 file.sh
0 0 2 file.sh
0 0 3 file.sh
```
and you will have atomically moved the "ours" stage of file.sh into
stage #0. This is sort of what I'd expect "git checkout --ours file.sh"
to do, but it doesn't seem to do that.
>
> In summary it seems to me that merge or no merge, the safety semantics
> for "--3way" + "--cached" as it is are pretty similar to the existing semantics
> for those options individually. The user could be preparing to write
> a tree out in either the "--cached" or the "--cached --3way" operation
> so I don't understand why those must differ in safety. In addition, the
> both "--3way" and "--3way --cached" perform mergey operations that
> changes the stages of a file in cache, so I don't understand why those
> must differ in safety either.
>
> >
> > >> Give an order to the codebase to "be like so".  Here is my attempt.
> > >>
> > >>     Teach "git apply" to accept "--cached" and "--3way" at the same
> > >>     time.  Only when all changes to all paths involved in the
> > >>     application auto-resolve cleanly, the result is placed in the
> > >>     index at stage #0 and the command exits with 0 status.  If there
> > >>     is any path whose conflict cannot be cleanly auto-resolved, the
> > >>     original contents from common ancestor (stage #1), our version
> > >>     (stage #2) and the contents from the patch (stage #3) for the
> > >>     conflicted paths are left at separate stages without any attempt
> > >>     to resolve the conflict at the content level, and the command
> > >>     exists with non-zero status, because there is no place (like the
> > >>     working tree files) to leave a half-resolved conflicted merge
> > >>     result to ask the end-user to resolve.
> >
> > I wrote the above as an example to illustrate the tone and the level
> > of details expected in our proposed commit log message.  The
> > behaviour it describes may not necessarily match what you have
> > implemented in the patch.
> >
> > For example, imagine that we are applying a patch for two paths,
> > where one auto-resolves cleanly and the other does not.  The above
> > description expects both paths will leave the higher stages (instead
> > of recording the auto-resolved path at stage #0, and leaving the
> > other path that cannot be auto-resolved at higher stages) and the
> > command exits with non-zero status, which may not be what you
> > implemented.  As an illustration, I didn't necessarily mean such an
> > all-or-none behaviour wrt resolving should be what we implement---I
> > do not want to choose, as this is your itch and I want _you_ with
> > the itch to think long and hard before deciding what the best design
> > for end-users would be, and present it as a proposed solution.  An
> > obvious alternative is to record auto-resolved paths at stage #0 and
> > leave only the paths for which auto-resolution failed in conflicted
> > state.
> I missed the "all changes to all paths" requirement in that description,
> I'll update it to be more consistent with what it actually does. As you say,
> the leaving entries at higher orders behavior only happens for conflicting
> paths, not for all paths.
> >
> > Thanks.

  reply	other threads:[~2021-04-07  2:26 UTC|newest]

Thread overview: 29+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-04-03  1:34 [PATCH 0/1] " Jerry Zhang
2021-04-03  1:34 ` [PATCH 1/1] " Jerry Zhang
2021-04-03  3:46   ` Elijah Newren
2021-04-03  4:26     ` Junio C Hamano
2021-04-04  1:02       ` Junio C Hamano
2021-04-05 22:12         ` Jerry Zhang
2021-04-05 22:23           ` Junio C Hamano
2021-04-05 23:29             ` Jerry Zhang
2021-04-06  0:10               ` Junio C Hamano
2021-04-05 22:08     ` Jerry Zhang
2021-04-05 22:19   ` [PATCH V2] " Jerry Zhang
2021-04-05 22:46     ` Junio C Hamano
2021-04-06  2:52       ` Jerry Zhang
2021-04-06  5:52         ` Junio C Hamano
2021-04-06 21:56           ` Jerry Zhang
2021-04-07  2:25             ` Jerry Zhang [this message]
2021-04-06  2:49     ` [PATCH v3] git-apply: allow " Jerry Zhang
2021-04-07 18:03       ` [PATCH v4] " Jerry Zhang
2021-04-07 19:00         ` Junio C Hamano
2021-04-08  2:13         ` [PATCH v5] " Jerry Zhang
2021-04-08 13:33           ` Junio C Hamano
2021-04-12 15:45             ` Elijah Newren
2021-04-12 18:26               ` Junio C Hamano
2021-04-12 15:40           ` Elijah Newren
2021-04-12 18:27             ` Junio C Hamano
2021-04-03  3:04 ` [PATCH 0/1] git-apply: Allow " Elijah Newren
2021-04-05 22:05   ` Jerry Zhang
2021-04-03  5:24 ` Bagas Sanjaya
     [not found]   ` <CAMKO5CtiW84E4XjnPRf-yOPp+ua_u07LsAu=BB0YhmP3+3kYiw@mail.gmail.com>
2021-04-03  8:05     ` Bagas Sanjaya

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

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to='CAMKO5Cv01vFde5XqrM0Niu1jSrhMe=bdmc15KN1Uo8WP9cNKhw@mail.gmail.com' \
    --to=jerry@skydio.com \
    --cc=abe@skydio.com \
    --cc=brian.kubisiak@skydio.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=jerryxzha@googlemail.com \
    --cc=newren@gmail.com \
    --cc=ross@skydio.com \
    --subject='Re: [PATCH V2] git-apply: Allow simultaneous --cached and --3way options' \
    /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

Code repositories for project(s) associated with this inbox:

	https://80x24.org/mirrors/git.git

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).