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?
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.
"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
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.
I am afraid that the whole cc/sequencer-rebase-i series needs a serious
reroll before it gets near 'next'.
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.
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.
0ccc92b sequencer: free memory used in "make_patch" function
Should be squashed to the previous.
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. 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).
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.
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?
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.
ff312f0 revert: libify pick
Almost good.
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?
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").
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.
--
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