'next'ed --allow-unrelated-histories could cause lots of grief

'next'ed --allow-unrelated-histories could cause lots of grief

[Yaroslav Halchenko]

Dear Git Gurus,

I guess whenever it rains it indeed pours, so it is me whining again.

I have decided to try 2.8.1.369.geae769a available from debian
experimental and through our (datalad) tests failing I became
aware of the 

    https://github.com/git/git/pull/158/commits/e379fdf34fee96cd205be83ff4e71699bdc32b18
    merge: refuse to create too cool a merge by default

which is planned for the next release.  I guess it is indeed a
worthwhile accident-prevention measure BUT not sure if it is so
important as to cause a change in behavior on which some projects using
git through the cmdline interface might have been relying upon for
years!

Given that git is quite 'self-healing', i.e. if someone has managed to
make a merge he didn't intend to, there is always a way back (e.g., as
simple as git reset --hard HEAD^), I am really not sure how valuable
such change of default behavior would be?  Could it may be made into a
warning instead? or reversed option "--dont-allow-unrelated-histories"?

Moreover, it was explicitly stated that "no configuration variable to
enable this by default exists and will not be added", which would cause
3rd party scripts/code/projects relying on previous behavior  to provide
version specific handling (either to add that
--allow-unrelated-histories or not)... very cumbersome!  If nothing else
remains, could there at least be a config option which we could
then use regardless of the version of git we are using for such merges?

P.S. Please maintain CC list

Thank you in advance for your consideration,
-- 
Yaroslav O. Halchenko
Center for Open Neuroscience     http://centerforopenneuroscience.org
Dartmouth College, 419 Moore Hall, Hinman Box 6207, Hanover, NH 03755
Phone: +1 (603) 646-9834                       Fax: +1 (603) 646-1419
WWW:   http://www.linkedin.com/in/yarik        

Re: 'next'ed --allow-unrelated-histories could cause lots of grief

[Junio C Hamano]

Yaroslav Halchenko <yoh@onerussian.com> writes:

> I have decided to try 2.8.1.369.geae769a available from debian
> experimental and through our (datalad) tests failing I became
> aware of the 
>
>     https://github.com/git/git/pull/158/commits/e379fdf34fee96cd205be83ff4e71699bdc32b18
>     merge: refuse to create too cool a merge by default
>
> which is planned for the next release.

See http://thread.gmane.org/gmane.linux.kernel.gpio/15365/focus=15401
for the backstory.

As this is to allow maintainers at higher levels of hierarchy not to
have to worry about stupid mistakes happen at maintainers at lower
levels, I'm afraid that turning this into an opt-in safety would
defeat the point of the change in a major way.

> ... BUT not sure if it is so
> important as to cause a change in behavior on which some projects using
> git through the cmdline interface might have been relying upon for
> years!

It is not very productive to make such an emotional statement
without substantiating _why_ a merge that adds a new root, which was
declared in the thread above as "crap" (in the context of the kernel
project), is necessary and is a good idea in "some projects".  Maybe
there is a valid use case that those from the kernel land didn't
think about.

> Given that git is quite 'self-healing', i.e. if someone has managed to
> make a merge he didn't intend to, there is always a way back (e.g., as
> simple as git reset --hard HEAD^),

That is only true if people notice.  A mere warning would not be an
effective prevention measure for a user who has to perform dozens of
merges a day.

I am personally on the fence, but right now I am on the side of
keeping the behaviour as implemented and documented, simply because
I haven't heard anything concrete to convince me why some people
need to regularly do a "crap" merge (in other words, in what context
these are not "crap" merges but ability to create them is a valuable
part of everyday workflow).

Re: 'next'ed --allow-unrelated-histories could cause lots of grief

[Yaroslav Halchenko]

On Thu, 21 Apr 2016, Junio C Hamano wrote:

> Yaroslav Halchenko <yoh@onerussian.com> writes:

> > I have decided to try 2.8.1.369.geae769a available from debian
> > experimental and through our (datalad) tests failing I became
> > aware of the 

> >     https://github.com/git/git/pull/158/commits/e379fdf34fee96cd205be83ff4e71699bdc32b18
> >     merge: refuse to create too cool a merge by default

> > which is planned for the next release.

> See http://thread.gmane.org/gmane.linux.kernel.gpio/15365/focus=15401
> for the backstory.

THANKS for the link!

> As this is to allow maintainers at higher levels of hierarchy not to
> have to worry about stupid mistakes happen at maintainers at lower
> levels, I'm afraid that turning this into an opt-in safety would
> defeat the point of the change in a major way.

> > ... BUT not sure if it is so
> > important as to cause a change in behavior on which some projects using
> > git through the cmdline interface might have been relying upon for
> > years!

> It is not very productive to make such an emotional statement
> without substantiating _why_ a merge that adds a new root, which was
> declared in the thread above as "crap" (in the context of the kernel
> project),

Sorry if my statement sounded too emotional ;)  I will outline some of
the use-cases below.

Meanwhile, I could argue that 'crap' in the above thread refers to the
entirety of the branch and thus more to the originating detached
root commit.  Action of creating of such a branch was also described  as
someone 'has done something TOTALLY INSANE'. And as "maintainers at
higher levels" expressed: "You actually have to *work* at making shit
like this".

I do see now the reason for introduced change of behavior clearer BUT I
would still argue that it should better be supported somehow by at least
a configuration option to not make poor mortals like yours truly
to sweat to stay compatible with multiple versions of git.

> is necessary and is a good idea in "some projects".  Maybe
> there is a valid use case that those from the kernel land didn't
> think about.

FWIW

- for git-annex (Joey was CCed from the beginning, not sure if annex
  would be affected though), it would be merging of git-annex
  branches while joining multiple annexes for the sync (e.g. by git
  annex assistant). 

- In our (datalad) case, repository is initialized with stock 'master'
  branch which carries configuration, which then used by the 'crawler'
  to initiate 'incoming' branch to contain (only) incoming data, which
  is later branched into 'incoming-processed' and later merged into
  'master', which is where such problematic merge would happen.  With
  such a workflow we can easily maintain custom changes within master,
  while automate changes to the crawled and processed data within
  those other branches being merged into master for final consumption.

  As a somewhat ugly workaround, we could probably artificially
  create an empty initial commit (forgot even how to do this magic) and
  branch-off it I guess, but I see other unit-tests failing as well, so
  I guess the situation is more chronic.

- In Debian packaging world, people often use 'debian' overlay branch
  which has no common ancestor with 'upstream' sources, but which need
  to be merged for 'in-tree' work.  I don't remember though if any of
  the tools rely on this feature (gbp iirc overlays not through the
  merge), but I know that I have used it quite a few times
  (interactively though, so yes -- could add a flag).

> > Given that git is quite 'self-healing', i.e. if someone has managed to
> > make a merge he didn't intend to, there is always a way back (e.g., as
> > simple as git reset --hard HEAD^),

> That is only true if people notice.  A mere warning would not be an
> effective prevention measure for a user who has to perform dozens of
> merges a day.

Could be argued...  Generally git's warnings is not something to be
ignored IMHO.  Do you ignore a balk of them on a daily basis?  if so --
they might better be downgraded to some kind of 'info' level msg then.

> I am personally on the fence, but right now I am on the side of
> keeping the behaviour as implemented and documented, simply because
> I haven't heard anything concrete to convince me why some people
> need to regularly do a "crap" merge (in other words, in what context
> these are not "crap" merges but ability to create them is a valuable
> part of everyday workflow).

once again -- IMHO it wasn't a 'merge' which was a crap, but the state
of the branch to be merged, and getting to that stage was non-trivial
endeavor as well ;)

Since the referenced situation happened only in 2016, I think that such
merges leading to undesired outcomes were quite a rare incident.  On the
other hand I do not have any statistic on how many of similar
merges were intensional, but I guess there could easily be thousands of
such merges intended at least in the git-annex world alone.  The point
is that it would be change in behavior require custom version-dependent
handling.

If it is more of a policy to be enforced per project, then IMHO it would
be better if disallowing such merges was a configuration option which
developers of adhering to such a policy project would configure for
their environments.

Cheers and sorry for a long post (again).
-- 
Yaroslav O. Halchenko
Center for Open Neuroscience     http://centerforopenneuroscience.org
Dartmouth College, 419 Moore Hall, Hinman Box 6207, Hanover, NH 03755
Phone: +1 (603) 646-9834                       Fax: +1 (603) 646-1419
WWW:   http://www.linkedin.com/in/yarik        

Re: 'next'ed --allow-unrelated-histories could cause lots of grief

[Junio C Hamano]

Yaroslav Halchenko <yoh@onerussian.com> writes:

>> It is not very productive to make such an emotional statement
>> without substantiating _why_ a merge that adds a new root, which was
>> declared in the thread above as "crap" (in the context of the kernel
>> project),
>
> Sorry if my statement sounded too emotional ;)  I will outline some of
> the use-cases below.

Thanks.  Emotional is fine, as long as you _also_ have useful
information.

Re: 'next'ed --allow-unrelated-histories could cause lots of grief

[Yaroslav Halchenko]

On Thu, 21 Apr 2016, Junio C Hamano wrote:
> >> It is not very productive to make such an emotional statement
> >> without substantiating _why_ a merge that adds a new root, which was
> >> declared in the thread above as "crap" (in the context of the kernel
> >> project),

> > Sorry if my statement sounded too emotional ;)  I will outline some of
> > the use-cases below.

> Thanks.  Emotional is fine, as long as you _also_ have useful
> information.

Gotcha:  I will follow "emotional + useful == fine" advice closer
next time ;)

Thank you a lot for the suggested patch with the env variable
workaround!
-- 
Yaroslav O. Halchenko
Center for Open Neuroscience     http://centerforopenneuroscience.org
Dartmouth College, 419 Moore Hall, Hinman Box 6207, Hanover, NH 03755
Phone: +1 (603) 646-9834                       Fax: +1 (603) 646-1419
WWW:   http://www.linkedin.com/in/yarik        

Re: 'next'ed --allow-unrelated-histories could cause lots of grief

[Joey Hess]

[-- Attachment #1: Type: text/plain, Size: 1258 bytes --]

Yaroslav Halchenko wrote:
> - for git-annex (Joey was CCed from the beginning, not sure if annex
>   would be affected though), it would be merging of git-annex
>   branches while joining multiple annexes for the sync (e.g. by git
>   annex assistant). 

Not entirely accurate (git-annex merges its git-annex branches using a
custom merge method and not involving git-merge). The actual use case is
two users (or one user with two devices) each with a git-annex
repository who decide to share their files by combining the two
repositories. This is pretty far from the kernel world, so it's not like
bisection is something they care about.

However, I also see --allow-unrelated-histories as very useful to
prevent many foot-shooting maneuvers. Especially when a repository has
special-purpose branches, like git-annex's git-annex branch, or other
branches that are never intended to be merged into master. It's a not
entirely uncommon mistake for users to merge in such a branch, and the
users who make such a mistake often don't know enough git to easily
recover from it.

Junio C Hamano wrote:
>   merge: GIT_MERGE_ALLOW_UNRELATED_HISTORIES environment

I hope this patch lands, it will save me a lot of bother.

-- 
see shy jo

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 811 bytes --]

Re: 'next'ed --allow-unrelated-histories could cause lots of grief

[Junio C Hamano]

Joey Hess <id@joeyh.name> writes:

> Junio C Hamano wrote:
>>   merge: GIT_MERGE_ALLOW_UNRELATED_HISTORIES environment
>
> I hope this patch lands, it will save me a lot of bother.

Yup, it is somewhat ugly but the least bad option among command line
option (which would break with older versions of Git), configuration
variables (which would encourage users to opt out of safety
unconditionallly) and environment (which allows scripts to opt-in),
I would say.

Re: 'next'ed --allow-unrelated-histories could cause lots of grief

[Joey Hess]

[-- Attachment #1: Type: text/plain, Size: 1203 bytes --]

Yaroslav Halchenko wrote:
> which is planned for the next release.  I guess it is indeed a
> worthwhile accident-prevention measure BUT not sure if it is so
> important as to cause a change in behavior on which some projects using
> git through the cmdline interface might have been relying upon for
> years!

Not only through the command line interface. The git-annex webapp has
common use cases that will be broken by this change.

> Moreover, it was explicitly stated that "no configuration variable to
> enable this by default exists and will not be added", which would cause
> 3rd party scripts/code/projects relying on previous behavior  to provide
> version specific handling (either to add that
> --allow-unrelated-histories or not)... very cumbersome!

Agreed, a configuration setting that could be passed via -c would be
much less cumbersome than checking the version of git in order to only
pass the option to git versions that understand it. This would also
provide a way to get git pull to allow such merges.

Compare with, for example, the change to default to an interactive
merge, where GIT_MERGE_AUTOEDIT=no was provided to ease compatability.

-- 
see shy jo

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 811 bytes --]