On Saturday 22 August 2009, Junio C Hamano wrote: > Christian Couder <chriscool@xxxxxxxxxxxxx> writes: > > From: Stephan Beyer <s-beyer@xxxxxxx> > > > > This code is taken from the sequencer GSoC project: > > > > git://repo.or.cz/git/sbeyer.git > > > > (commit e7b8dab0c2a73ade92017a52bb1405ea1534ef20) > > > > but the messages have been changed to be the same as those > > displayed by git-rebase--interactive.sh. > > Hmm, forgot to amend, or perhaps you sent out a wrong series? Your comments on the v2 series were in a message replying to patch 5/9, so I amended only 5/9 and after it, because I thought that you had already reviewed those before 5/9 and they were ok. > The log message does not explain: > > - why the patch adds a new static function that nobody calls; > - what the new function is good for; > > which are the most important things in order to defend the change. Yes, but the function contains only 5 lines of code and it mostly only calls reset_almost_hard() that is already documented by a big comment before its definition. > "The messages have been changed to..." hints that the original commit by > Stephan had different messages produced, perhaps so that it can be used > in a different context. I hoped, in an ideal world, perhaps Stephan > defended why the change was relevant to his project in some way, and > because you are using it in a different context that needs modification > of the message, perhaps Stephan's defense of his commit could be reworded > to defend your change here. > > So I decided to take a look at the quoted commit to see if I can reword > this mess. > > But the quoted commit e7b8dab0c2a73ade92017a52bb1405ea1534ef20 does not > even seem to be a commit that corresponds to this change. It is a merge > from upstream. > > commit e7b8dab0c2a73ade92017a52bb1405ea1534ef20 > Merge: 0c73ae7 99ddd24 > Author: Stephan Beyer <s-beyer@xxxxxxx> > Date: Wed May 20 10:54:37 2009 +0200 > > Merge branch 'junio/master' into seq-builtin-dev > > After this merge f79d4c8 "teach git-am to apply a patch to an > unborn branch" has to be reimplemented in sequencer by allowing the > "patch" insn on unborn branches. > The related test in t/t4150-am.sh is set to test_expect_failure. > > Conflicts: > git-am.sh This is the commit at the point where I took the code of the function, not the commit that introduce the function. This is because some functions like pick() in patch 5/9 evolve a lot in the sequencer repo after they are introduced. > It does not help that the function that is crucial to the implemention of > this new function, reset_almost_hard(), is not explained at all in the > earlier commit in the previous series (36f692b (sequencer: add > "reset_almost_hard()" and related functions, 2009-08-05). > > The log message to the commit does not even hint in what sense "almost" > the function is, iow, in what situation it behaves exactly like "reset > --hard", and in what other situation it doesn't, and more importantly why > that distinction is there. I thought I asked these questions when the > previous series was submitted, but I do not remember ever seeing > satisfactory answers to them. You asked questions about reset_almost_hard() and I added the big comment before its definition, see: http://git.kernel.org/?p=git/git.git;a=commitdiff;h=9d41fbd28f1ba3d07cd2e0f547521f9dced4cfd2 As you merged the series in pu, I thought that it was ok. > I am afraid that the whole cc/sequencer-rebase-i series needs a serious > reroll before it gets near 'next'. Ok, I will reroll everything to try to improve commit messages. > Before giving up, I'll quickly re-review how (un)readable the log of each > commit is in the series. The following comments are mostly about the log > messages, which are supposed to entice people to review the code, and > more importantly, used as part of the release notes to summarize what the > newly added toys are about. If they are horrible, the code has little > chance to be even read, and I'll have a hard time merging the series up > into a new release. About the release notes, as "git sequencer--helper" is not for public use, and as many functions like reset_almost_hard() are static, perhaps it would be ok to have only something like: (developers) - parts of "git rebase -i" have been ported to C using "git sequencer--helper" - cherry-pick and revert functionality is available using new functions declared in "pick.h" > 6db6551 sequencer: add "builtin-sequencer--helper.c" > > Good. > > b512803 sequencer: add "make_patch" function to save a patch > > Passably okay, but the limitation that it always writes into a file with > a fixed name "$SEQ_DIR/patch" should be noted in the log. Ok. > 0ccc92b sequencer: free memory used in "make_patch" function > > Should be squashed to the previous. Will do. > f121b06 rebase -i: use "git sequencer--helper --make-patch" > > Good. > > 36f692b sequencer: add "reset_almost_hard()" and related functions > > Horrible. See above. > > 9d41fbd sequencer: add comments about reset_almost_hard() > > Should be squashed to the previous---lift some text to justify the > existence of the function in the commit log message. Will do. > Even though > allow_dirty is referred to in the comment as affecting the behaviour, it > is unclear who sets that global variable using what interface, making the > reader suspect that maybe it should be a function parameter instead of a > global (but the other parts of the helper may also look at allow_dirty > and the internal implementation might be--I am just guessing--simpler > this way, in which case _that_ should be explained and justified). Will have a look. > 022a9e7 sequencer: add "--reset-hard" option to "git sequencer--helper" > > This by itself is Okay, provided if 36f692b were made readable. Then you > can expect the reader to know why reset_almost_hard() needs to be there, > and you need an interface to that function. Until then, it is totally > unclear why you need this, instead of using "reset --hard" itself. Ok. > ad28459 rebase -i: use "git sequencer--helper --reset-hard" > > Ditto. > > e4b3f0f sequencer: add "do_fast_forward()" to perform a fast forward > > See above. > > 1d88073 sequencer: add "--fast-forward" option to "git sequencer--helper" > > Okay. > > 6eff656 sequencer: let "git sequencer--helper" callers set "allow_dirty" > > Why? What for? Daniel asked me to make it available so people interested can test. I will state this in the commit message. > 877ddc1 rebase -i: use "git sequencer--helper --fast-forward" > > It is unclear how this relates to the previous one, nor why it is more > appropriate than "reset-hard" it replaces. It is more appropriate because it makes the rebase -i shell code a little bit shorter. I will add that to the commit message. > ff312f0 revert: libify pick > > Almost good. Will try to improve. > ab67716 pick: libify "pick_help_msg()" > > Good. > > d871b0e sequencer: add "do_commit()" and related functions > > We can see from "git show" what static functions that are never called in > this commit are added, but nobody explains why they are needed. For > example, do_commit() may create a new commit object, but does not share > the code with what "git commit" and/or "git commit-tree" do? If so, how? > If not, why not? Will have a look. > ac5fc4d sequencer: add "--cherry-pick" option to "git sequencer--helper" > > Passably okay. I can see ff312f0 made about a half of cherry-pick > accessible to the sequencer, and this patch uses it to finish the other > half, although that is not explained in the log message. Also it is > unclear why the resulting "libified" code does not share more > infrastructure with "git cherry-pick" itself (and "git revert"). Will have a look. > 664c7ab rebase -i: use "git sequencer--helper --cherry-pick" > > Passably okay, even though it is not quite convincing why using > sequencer-helper's cherry-pick option makes it easier to later port the > script, than keeping calls to cherry-pick. Will have a look. Thanks, Christian. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html