All of lore.kernel.org
 help / color / mirror / Atom feed
From: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
To: Jonathan <git.jonathan.bressat@gmail.com>
Cc: cogoni.guillaume@gmail.com, Matthieu.Moy@univ-lyon1.fr,
	git@vger.kernel.org, guillaume.cogoni@gmail.com,
	Jonathan <Jonathan.bressat@etu.univ-lyon1.fr>
Subject: Re: [PATCH 0/1] Be nicer to the user on tracked/untracked merge conflicts
Date: Tue, 12 Apr 2022 21:24:34 +0200	[thread overview]
Message-ID: <220412.868rsagkus.gmgdl@evledraar.gmail.com> (raw)
In-Reply-To: <20220412191556.21135-1-Jonathan.bressat@etu.univ-lyon1.fr>


On Tue, Apr 12 2022, Jonathan wrote:

> When doing a merge while there is untracked files with the same name
> as merged files, git refuses to proceed. This patch make git overwrite
> files if their content are the same.
>
> We added a statement to check_ok_to_remove() (unpack-trees.c) 
> with ie_modified() (read-cache.c) to test if the untracked file 
> has the same content as the merged one. It seems to work well 
> with all three o->result, o->dst_index and o->src_index,
> We are not sure of what is the usage of those three, did we used it
> properly?
>
> Our tests need some improvement, for example using test_commit,
> and testing more possibilities, it's not a real patch, just 
> to comfirm if we are on the right track.
>
> The next idea is when it's a fastforward, attempt to merge the
> untracked file and the upstream version (like if the file had
> just been committed, but without introducing an extra commit).
>
> you can see this idea here: 
> https://git.wiki.kernel.org/index.php/SmallProjectsIdeas#Be_nicer_to_the_user_on_tracked.2Funtracked_merge_conflicts

I left some comments on the patch itself, but structurally it wolud be
really nice to make this and similar changes:

 1. Test for current behavior
 2. Change behavior and relevant (new) tests

Rather than the current one-step, that would also communicate that wiki
link (and better) via code.

> Questions:
> The old behaviour was here for technical reasons?
> The new behavior that we introduce here become the default one?
> If the old behavior was important for some people or for some reasons,
> we can set a global variable to switch between the old and the new one.
> And if we define a global variable, should we print a warning to let 
> users know that there is a new behavior when a merge is called and that
> he can switch between the old and new one.

I don't know if we need a config etc., but FWIW my first reaction to
this is that it's a bit iffy/fragile, i.e. before this we'd basically
error out and say "fix your index/working tree".

But now just because the newly merged content happens to be identical
we'll silently merge it over that "staged" content?

Anyway, I can also see how that would be useful for some people.

I've personally been annoyed by a subset of this behavior in the past, I
can't remember if it's with merge or rebase that we'll refuse to do
anything because we have a locally modified/staged (can't remember) file
"X", even though "X" won't be touched at all if the merge/rebase
happens.

But I haven't wanted git to have quite this level of DWYM behavior in
this area, just my 0.02.

> For some reason, test_commit make the merge not working like if it's the
> old behaviour of merge, I dont understand why ?

Ah, I left some comments on "why not test_commit"...

Do you have an example of such a non-working case? I'm not sure why it
wouldn't work.

  parent reply	other threads:[~2022-04-12 19:31 UTC|newest]

Thread overview: 27+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-03-27 15:41 [WIP]: make merge nicer to the user Guillaume Cogoni
2022-04-12 19:15 ` [PATCH 0/1] Be nicer to the user on tracked/untracked merge conflicts Jonathan
2022-04-12 19:15   ` [PATCH 1/1] Merge with untracked file that are the same without failure and test Jonathan
2022-04-12 19:21     ` Ævar Arnfjörð Bjarmason
2022-04-13 18:18     ` Junio C Hamano
2022-04-25 20:27       ` [PATCH v1 0/2] Be nicer to the user on tracked/untracked merge conflicts Jonathan
2022-04-25 20:27         ` [PATCH v1 1/2] t7615: test how merge behave when there is untracked file Jonathan
2022-04-25 20:27         ` [PATCH v1 2/2] merge with untracked file that are the same without failure Jonathan
2022-04-25 21:16         ` [PATCH v1 0/2] Be nicer to the user on tracked/untracked merge conflicts Junio C Hamano
2022-04-25 22:28           ` Guillaume Cogoni
2022-04-25 23:10             ` Junio C Hamano
     [not found]           ` <fdd9f13d14e942f3a1572866761b9580@SAMBXP02.univ-lyon1.fr>
2022-04-26  6:38             ` Matthieu Moy
2022-04-26 16:13               ` Junio C Hamano
2022-04-28 10:33                 ` Jonathan Bressat
2022-05-27 19:55                   ` [PATCH v2 0/4] " Jonathan Bressat
2022-05-27 19:55                     ` [PATCH v2 1/4] t6436: tests how merge behave when there is untracked file with the same content Jonathan Bressat
2022-05-27 19:55                     ` [PATCH v2 2/4] merge with untracked file that are the same without failure Jonathan Bressat
2022-05-27 19:55                     ` [PATCH v2 3/4] add configuration variable corresponding to --overwrite-same-content Jonathan Bressat
2022-05-27 19:55                     ` [PATCH v2 4/4] error message now advice to use the new option Jonathan Bressat
     [not found]                     ` <dfea1d98c15047428b1a11adbc002eef@SAMBXP02.univ-lyon1.fr>
2022-06-04  9:44                       ` [PATCH v2 1/4] t6436: tests how merge behave when there is untracked file with the same content Matthieu Moy
     [not found]                     ` <be2297bdcd724c3f8abfde2d5d74fb18@SAMBXP02.univ-lyon1.fr>
2022-06-04  9:45                       ` [PATCH v2 2/4] merge with untracked file that are the same without failure Matthieu Moy
     [not found]                     ` <82beb916d9c44a069f30ec4ff261e3be@SAMBXP02.univ-lyon1.fr>
2022-06-04  9:45                       ` [PATCH v2 4/4] error message now advice to use the new option Matthieu Moy
2022-06-10 12:58                         ` Guillaume Cogoni
     [not found]                     ` <4efbe7d9c95841c691f51954670a1d9f@SAMBXP02.univ-lyon1.fr>
2022-06-04  9:49                       ` [PATCH v2 3/4] add configuration variable corresponding to --overwrite-same-content Matthieu Moy
     [not found]         ` <eca66375d8b34154856b7da303bf96d7@SAMBXP02.univ-lyon1.fr>
2022-04-26  6:48           ` [PATCH v1 1/2] t7615: test how merge behave when there is untracked file Matthieu Moy
2022-04-12 19:24   ` Ævar Arnfjörð Bjarmason [this message]
2022-04-14  8:57     ` [PATCH 0/1] Be nicer to the user on tracked/untracked merge conflicts Jonathan Bressat

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=220412.868rsagkus.gmgdl@evledraar.gmail.com \
    --to=avarab@gmail.com \
    --cc=Jonathan.bressat@etu.univ-lyon1.fr \
    --cc=Matthieu.Moy@univ-lyon1.fr \
    --cc=cogoni.guillaume@gmail.com \
    --cc=git.jonathan.bressat@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=guillaume.cogoni@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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.