git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Elijah Newren <newren@gmail.com>
To: Junio C Hamano <gitster@pobox.com>
Cc: Chris Torek via GitGitGadget <gitgitgadget@gmail.com>,
	Git Mailing List <git@vger.kernel.org>,
	Chris Torek <chris.torek@gmail.com>
Subject: Re: [PATCH] git-mv: improve error message for conflicted file
Date: Sat, 18 Jul 2020 18:48:08 -0700	[thread overview]
Message-ID: <CABPp-BGJdwpwhQUp4Wa4bKBp4hQFB9OM3N1FXH7SzY0mvLDa7Q@mail.gmail.com> (raw)
In-Reply-To: <xmqq8sfgd8cu.fsf@gitster.c.googlers.com>

On Sat, Jul 18, 2020 at 10:46 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> Elijah Newren <newren@gmail.com> writes:
>
> > Or, even better, make ce_stage(ce) not be an error; see
> > https://lore.kernel.org/git/xmqqk1ozb6qy.fsf@gitster-ct.c.googlers.com/.
>
> Hmph, I actually am not convinced with that one, even though I know
> I wrote it, I do not think I had thought things through before I
> did.  It may make sense in a narrow special case where there is a
> single entry at a higher stage to rename it to the destination path
> and drop it down to stage #0, but I do not think of a good behaviour
> for more general cases.
>
> What should happen, for example, if two or more entries at higher
> stages exist for the path being moved?  Renaming all of them to the
> destination path without changing their stage number may make more
> sense [*1*].  At least, that solution still lets the user to choose
> object at which stage [*2*] to use in the final resolution.
>
> Now, if we define that "git mv src dst", when 'src' is a conflicted
> path, moves the working tree file src to dst at the same time moves
> all the higher stage entries for 'src' to 'dst' while retaining
> their stage number, what does the degenerate case of having only one
> such entry look like?  You start from the state where you have
> $that_path at stage #3, "git mv $that_path $over_there" would put
> you in the state where you have the same contents at $over_there and
> at stage #3.  If you want to just take "their" contents as the
> resolution, "git add $over_there" after doing that "git mv" would
> let you record the resolution, so it does not look too bad.
>
> It would also be a handy way to recover from a mistake made by
> "directory level rename" heuristics.  Instead of resolving the
> content-level conflicts at the wrong path and then moving that
> resolved result to the right path, you can first correct the wrong
> path by moving the conflicted whole to the right path and then
> perform the content-level conflict resolution.  The advantage of the
> latter is obvious.  You have to do two things (rename and edit) and
> with the former way of doing 'edit' first and then 'rename', after
> resolving the conflict and adding the result at the wrong path, "git
> ls-files -u" or "git status" no longer help you remember that the
> path still needs to be moved.  Instead, you can move first and "git
> ls-files -u" would still remember that even after the move you still
> need to deal with content-level conflicts.

Oh, good, I actually like the idea of not auto-resolving better; I was
always a bit uncomfortable with it.  And now that you brought up this
case, there's another good case I can think of too: D/F conflicts that
arise from a rename/add-like situation, but where the add is a new
directory rather than a new file.  In such a case, there can be three
higher order stages for the file due to content conflicts carried with
the rename and auto-resolving them when renaming doesn't make any
sense.  So, yeah, keeping the higher order stages but just renaming
the path seems like a nice improvement for the user.

> Anyway, I think the "separate missing entry and conflicted entry
> when issuing an error message" is a strict improvement.

Agreed.

> [Footnotes]
>
> *1* Of course, the "D/F conflicts are issue only among the entries
>     at the same stage" and other usual rules apply when we check if
>     the move of these higher stage entries is possible.  I am not
>     sure if the low-level API functions like rename_index_entry_at()
>     are prepared to deal with higher stage entries, though, so this
>     may be a nontrivial amount of new work.  It is unclear to me if
>     it is worth doing.

Yeah, if it were trivial, I would have just done it back when I
mentioned it a couple years ago.  I think it's still worth doing, but
I agree it might need some new low-level API function(s) or some
modifications to some existing one(s).

> *2* Actually, "the object at stage #1 for conflicted path P" is a
>     wrong thing to say.  The index is designed to hold multiple
>     stage #1 entries at the same path so that it can express the
>     case where there are more than one common ancestors, and
>     multiple stage #3 entries to express an octopus merge in
>     progress.

Really?  Wow.  I never had any clue that this was possible and never
ran across it.  Is it documented anywhere?

>     The notation to name the object in the index punts
>     and does not let you say "git diff :1.0:path :1.1:path" to
>     compare the first and the second common ancestor versions of
>     path, though it probably should if we wanted to be consistent.

  reply	other threads:[~2020-07-19  1:48 UTC|newest]

Thread overview: 11+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-07-17 23:24 Chris Torek via GitGitGadget
2020-07-17 23:47 ` Eric Sunshine
2020-07-18  1:35   ` Chris Torek
2020-07-18  6:55     ` Eric Sunshine
2020-07-18  0:07 ` Junio C Hamano
2020-07-18  2:00   ` Elijah Newren
2020-07-18 17:46     ` Junio C Hamano
2020-07-19  1:48       ` Elijah Newren [this message]
2020-07-19  6:16         ` Junio C Hamano
2020-07-20  6:17 ` [PATCH v2] " Chris Torek via GitGitGadget
2020-07-20 18:28   ` 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=CABPp-BGJdwpwhQUp4Wa4bKBp4hQFB9OM3N1FXH7SzY0mvLDa7Q@mail.gmail.com \
    --to=newren@gmail.com \
    --cc=chris.torek@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=gitgitgadget@gmail.com \
    --cc=gitster@pobox.com \
    --subject='Re: [PATCH] git-mv: improve error message for conflicted file' \
    /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

This is a public inbox, see mirroring instructions
on how to clone and mirror all data and code used for this inbox