Message-ID: <20160318050018.GA97010@jaegeuk.hsd1.ca.comcast.net>
Date:	Thu, 17 Mar 2016 22:00:18 -0700
From:	Jaegeuk Kim <jaegeuk@...nel.org>
To:	Linus Torvalds <torvalds@...ux-foundation.org>
Cc:	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
	Linux FS Dev Mailing List <linux-fsdevel@...r.kernel.org>,
	Linux F2FS Dev Mailing List 
	<linux-f2fs-devel@...ts.sourceforge.net>
Subject: Re: [GIT PULL] f2fs updates for v4.6

On Thu, Mar 17, 2016 at 07:32:34PM -0700, Linus Torvalds wrote:
> On Thu, Mar 17, 2016 at 5:58 PM, Jaegeuk Kim <jaegeuk@...nel.org> wrote:
> >
> > Note that, I did cherry-pick one patch and add another patch to resolve the
> > conflict against recent crypto changes.
> 
> *please* don't do things like this!

My apologies.
I'll definitely keep in mind.

And, thank you for explaining what I did so insanely.
I can understand how I made a mistake which makes the git history much uglier
as well as unnecessary commits which make it much harder to investigate any
problem.

Let me undo my recent unnecessary extra work and add a single commit that
migrates the crypto codes.
Then, let me repost a pull request later again.
Sorry for the confusion.

Thanks,

> 
> Now I have a conflict _anyway_, and because you added extra commits it
> ends up being just messier. Git notices that you renamed
> fs/f2fs/crypto_fname.c into fs/crypto/fname.c, and sees that different
> people did different things, so now your "merge resolution" conflicts
> with the changes I already merged to the original file from the crypto
> tree.
> 
> Yes, yes, since you also did that "f2fs: Use skcipher", the resolution
> now is to just throw the extra merge resolution away, but it's
> literally just messier and more work for me, because it makes even
> less sense than it would have done to just do the merge resolution
> directly.
> 
> I do *not* want maintainers of two different trees trying to resolve
> things against each other ahead of time. It makes no sense, and it
> makes the history make no sense.
> 
> Now, if a merge ends up being really painful, I might ask you to help
> resolve a merge, but quite frankly, that is very very rare.
> 
> I'd much *much* rather see you just do stuff that makes sense for your
> tree. In particular, you really should have done that "move file" as a
> single file move commit, and just left it at that. Instead, your tree
> does something insane:
> 
>  - first it cherry-picks the change from the crypto tree
> 
>  - then it *copies* the old file to fs/crypto/fname.c, and changes the
> naming to match.
> 
>  - at this point nothing actually *uses* the new file or new naming at all.
> 
>  - then it "resolves the merge conflict" by re-doing *AGAIN* that
> crypto change that it shouldn't have cared about in the first place
> 
>  - then it removes the old file and switches to the new naming in the users
> 
> That makes no sense what-so-ever.
> 
> What would have been *sensible* would have been to split it up in a
> *natural* way:
> 
>  - move the file to a new subdirectory in one commit (together with
> the Makefile/Kconfig stuff that enables it)
> 
>  - change the naming to match the new location in another.
> 
> or even just do it all in one single step. And at no point worry about
> the fact that the crypto tree did something unrelated to your work.
> 
> You should aim to make the changes make sense in *your* tree. Not copy
> and create files that aren't used until the next stage. Not pre-merge
> with another completely unrelated tree.
> 
> Now you actually did extra work apparently to "help", but what it
> caused was for me to have to try to figure out those crazy steps, and
> it made the history just uglier.
> 
> And note that even if I didn't have a conflict (git didn't notice that
> there were a few other files that moved too, because the renaming made
> the file contents too different), I would still have preferred to
> simply do the conflict resolution at the time of the merge, rather
> than have it pre-done.
> 
> Put another way: you doing the "pre-merge" resolution ends up making
> my merge simpler from a purely technical standpoint, but it also makes
> the history make very little sense. I'd much rather have the history
> make sense, and then in the actual *merge* when the f2fs tree really
> meets the crypto tree updates, the resolution fixes things up (and
> then the resolution makes sense within those confines too).
> 
> And never add files that aren't even used. Now we literally have this
> insane situation that commit 7690143802f1 ("fs crypto: add crypto.c
> for encrypt/decrypt functions") adds a new directory, and a new file
> in it, but there is absolutely nothing that references that new file.
> So it's all completely empty work.
> 
> Then later, you add the Makefile and Kconfig rules to start building
> the files, but still nothing *uses* it.
> 
> And then, after that, you have one commit that removes the old code,
> and switches over to the new code.
> 
> Not only does none of this make ANY SENSE WHAT-SO-EVER, but it's a
> complete nightmare if something breaks. The point you *notice* that
> things break is when you switch over the new code, but that isn't
> actually when the new code happens, so now you have this situation
> where the commit that introduces the breakage basically silently
> introduces all that old code that was never actually used - but it's
> not shown in that commit!
> 
> This is why I think you should have just renamed the files one by one,
> and actually started using them as you go along. See? Not only would
> it make more sense, but when a problem happens, you actually see what
> causes it, instead of it being a "oh, we switched over to that
> completely different code that had never been used by anything at all
> before".
> 
>                  Linus

Powered by blists - more mailing lists