linux-kernel - Re: [PATCH] ide: Fix bug caused by git merge

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <7vmxi1fbxs.fsf@alter.siamese.dyndns.org>
Date:	Wed, 01 Jun 2011 15:01:51 -0700
From:	Junio C Hamano <gitster@...ox.com>
To:	Linus Torvalds <torvalds@...ux-foundation.org>
Cc:	Steven Rostedt <rostedt@...dmis.org>,
	Jens Axboe <jaxboe@...ionio.com>,
	LKML <linux-kernel@...r.kernel.org>, axboe <axboe@...nel.dk>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Tejun Heo <tj@...nel.org>
Subject: Re: [PATCH] ide: Fix bug caused by git merge

Junio C Hamano <gitster@...ox.com> writes:

> Linus Torvalds <torvalds@...ux-foundation.org> writes:
>
>> Junio - what made it harder for Steven to see the reason may be due
>> the default history simplification. I do wonder if we should just make
>> "--simplify-merges" the default, because the aggressive and simple
>> default culling makes it hard to see merge commits like this that just
>> pick one side  over the other. --simplify-merges is more expensive,
>> but doesn't have some of the problems the aggressive simplification
>> has.
>
> It is more expensive not just in computation cycles but in latency, as it
> is inherently a "limited" operation that needs to first walk the history
> and then post-process.
>
>   $ time sh -c 'git log drivers/ide/ide-cd.c | head -n 200 >/dev/null'
>
> with and without "--simplify-merges" shows more than 50-fold differences
> (0.15s vs 8s).
>
> I am more disturbed by the fact that "git show 698567f3fa7" does not show
> the mismerge (even with -c). Of course I know that not showing "taking
> from one side" is by design of -c/--cc, but I still feel there should be
> something we could do about it.

Your comment was about digging history after problem happened, so this is
a tangent in that sense, but I am wondering if it makes sense to advertise
and encourage the use of --conflict=diff3 even more, to prevent accidents
like this from happening in the first place. 

Here is how our default merge conflict marker (modelled after RCS and CVS
output) look like when recreating the merge:

diff --cc drivers/ide/ide-cd.c
index 6e5123b,a5ec5a7..0000000
--- a/drivers/ide/ide-cd.c
+++ b/drivers/ide/ide-cd.c
@@@ -1781,8 -1781,7 +1781,12 @@@ static int ide_cd_probe(ide_drive_t *dr
  
        ide_cd_read_toc(drive, &sense);
        g->fops = &idecd_ops;
++<<<<<<< HEAD
 +      g->flags |= GENHD_FL_REMOVABLE | GENHD_FL_BLOCK_EVENTS_ON_EXCL_WRITE;
 +      g->events = DISK_EVENT_MEDIA_CHANGE;
++=======
+       g->flags |= GENHD_FL_REMOVABLE;
++>>>>>>> 61c4f2c
        add_disk(g);
        return 0;

With --conflict=diff3, it becomes crystal clear that g->events assignment
needs to be removed for even somebody who does not know the history of
this part of the kernel at all (like me).

diff --cc drivers/ide/ide-cd.c
index 6e5123b,a5ec5a7..0000000
--- a/drivers/ide/ide-cd.c
+++ b/drivers/ide/ide-cd.c
@@@ -1781,8 -1781,7 +1781,15 @@@ static int ide_cd_probe(ide_drive_t *dr
  
        ide_cd_read_toc(drive, &sense);
        g->fops = &idecd_ops;
++<<<<<<< ours
 +      g->flags |= GENHD_FL_REMOVABLE | GENHD_FL_BLOCK_EVENTS_ON_EXCL_WRITE;
 +      g->events = DISK_EVENT_MEDIA_CHANGE;
++||||||| base
+       g->flags |= GENHD_FL_REMOVABLE;
++      g->events = DISK_EVENT_MEDIA_CHANGE;
++=======
++      g->flags |= GENHD_FL_REMOVABLE;
++>>>>>>> theirs
        add_disk(g);
        return 0;

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/