Re: [PATCH/RFCv4 1/2] git-rebase -i: add command drop to remove a commit
Junio C Hamano gits...@pobox.com writes: But what if you got this back after the user edits? drop pick 2c9c1c5 gostak: distim doshes drop e3b601d pull: use git-rev-parse... edit eb2a8d9 pull: handle git-fetch'... [...] Did the user tried to drop something else but the object name has gone missing by mistake? If the user tried to drop something but the SHA-1 has gone missing, it would be picked up by 2/2 if rebase.missingCommitsCheck is set to warn or error. Did the user wanted to drop the first one but made mistake while editing 'pick' away into 'drop'? I see your point here. Noticing and flagging malformed 'drop' lines (or line with any command, for that matter) as such is part of that process to make sure you collected the object names from the after image correctly, which is the job of 2/2 in your series (if I am reading the description of your series right). I agree on the fact that noticing and flagging malformed lines for the various commands is a part of the process to make sure the collected object names are correct. However, the original job of 2/2 was to avoid the silent loss of information caused by deleting a line by mistake, not to check the correctness of the whole todo-file (though I think that it is a good idea, to develop in another commit of this patch/in another patch). On the opposite, in some way it could be seen as some loss of information, as your previous example suggests: Did the user wanted to drop the first one but made mistake while editing 'pick' away into 'drop'? We lose the information that the user wanted to drop the line and not pick it. Aside from that, checking specifically the 'drop' command beforehand (that's what happens in 2/2) while not doing any checking on the other commands (or checking on the fly) doesn't really makes sense, I think the commands should be treated equally on the matter of checking. In doing so, checking that the various commands exist, that they are followed by the correct argument and that their argument is of the right type (SHA-1 representing a commit for example), in top of checking that no commit is removed, without forgetting tests to make sure everything has the right behaviour, I am afraid that it would make this part of the patch far too long thus why I think it should be in another commit/patch. By the way In the new world order to punish those who simply remove lines to signal that they want the commits omitted from replaying Aside from liking the wording here, I don't think it can really be considered as a punishment since it is the user that chooses if he wants to be punished or not; I guess it can be considered BDSM though. Junio C Hamano gits...@pobox.com writes: +drop|d) +if test -z $sha1 +then +warn Missing SHA-1 in 'drop' command. +die Please fix this using 'git rebase --edit-todo'. Is this a sensible piece of advice, though? The user edited the line away and it does not have the commit object name anymore. How does she fix it in the editor? The same puzzlement applies to the other message. For a missing SHA-1, if it is a 'drop' that he forgot to remove after changing his mind, then it can be fixed. For an incorrect SHA-1, maybe he can find it back using git log or others commands of the kind, though I don't think this way of fixing things is user-friendly at all. However, as you point out, in most cases it will be a line edited away, I agree that the fix it doesn't really help. The only alternative I see right now (in 1/2) is aborting the rebase (die_abort) though it seems overkill, as the user I wouldn't want to lose all of the modifications I have done on the todo-list. Through this I see your point about checking in 2/2 since we would have more information (for example if he has an error because a drop has no SHA-1 or the SHA-1 is incorrect and at the same time an error because a SHA-1 has disappeared, it would make more sense to give him fix it since he would have most of the informations he needs to do so). However, right now, I think that my previous points about 2/2 still stand. Thanks, Rémi -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH/RFCv4 1/2] git-rebase -i: add command drop to remove a commit
Instead of removing a line to remove the commit, you can use the
command drop (just like pick or edit). It has the same effect as
deleting the line (removing the commit) except that you keep a visual
trace of your actions, allowing a better control and reducing the
possibility of removing a commit by mistake.
Signed-off-by: Galan Rémi remi.galan-alfo...@ensimag.grenoble-inp.fr
---
Documentation/git-rebase.txt | 3 +++
git-rebase--interactive.sh| 18 ++
t/lib-rebase.sh | 4 ++--
t/t3404-rebase-interactive.sh | 10 ++
4 files changed, 33 insertions(+), 2 deletions(-)
diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
index 1d01baa..9cf3760 100644
--- a/Documentation/git-rebase.txt
+++ b/Documentation/git-rebase.txt
@@ -514,6 +514,9 @@ rebasing.
If you just want to edit the commit message for a commit, replace the
command pick with the command reword.
+To drop a commit, replace the command pick with drop, or just
+delete its line.
+
If you want to fold two or more commits into one, replace the command
pick for the second and subsequent commits with squash or fixup.
If the commits had different authors, the folded commit will be
diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index dc3133f..869cc60 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -152,6 +152,7 @@ Commands:
s, squash = use commit, but meld into previous commit
f, fixup = like squash, but discard this commit's log message
x, exec = run command (the rest of the line) using shell
+ d, drop = remove commit
These lines can be re-ordered; they are executed from top to bottom.
@@ -508,6 +509,23 @@ do_next () {
$comment_char*|''|noop)
mark_action_done
;;
+ drop|d)
+ if test -z $sha1
+ then
+ warn Missing SHA-1 in 'drop' command.
+ die Please fix this using 'git rebase --edit-todo'.
+ fi
+
+ sha1_verif=$(git rev-parse --verify --quiet $sha1^{commit})
+ if test -z $sha1_verif
+ then
+ warn '$sha1' is not a SHA-1 or does not represent \
+ a commit in 'drop' command.
+ die Please fix this using 'git rebase --edit-todo'.
+ fi
+
+ mark_action_done
+ ;;
pick|p)
comment_for_reflog pick
diff --git a/t/lib-rebase.sh b/t/lib-rebase.sh
index 6bd2522..fdbc900 100644
--- a/t/lib-rebase.sh
+++ b/t/lib-rebase.sh
@@ -14,7 +14,7 @@
# specified line.
#
# cmd lineno -- add a line with the specified command
-# (squash, fixup, edit, or reword) and the SHA1 taken
+# (squash, fixup, edit, reword or drop) and the SHA1 taken
# from the specified line.
#
# exec_cmd_with_args -- add an exec cmd with args line.
@@ -46,7 +46,7 @@ set_fake_editor () {
action=pick
for line in $FAKE_LINES; do
case $line in
- squash|fixup|edit|reword)
+ squash|fixup|edit|reword|drop)
action=$line;;
exec*)
echo $line | sed 's/_/ /g' $1;;
diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index ac429a0..8960083 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -1102,4 +1102,14 @@ test_expect_success 'rebase -i commits that overwrite
untracked files (no ff)' '
test $(git cat-file commit HEAD | sed -ne \$p) = I
'
+test_expect_success 'drop' '
+ test_when_finished git checkout master
+ git checkout -b dropBranchTest master
+ set_fake_editor
+ FAKE_LINES=1 drop 2 3 drop 4 5 git rebase -i --root
+ test E = $(git cat-file commit HEAD | sed -ne \$p)
+ test C = $(git cat-file commit HEAD^ | sed -ne \$p)
+ test A = $(git cat-file commit HEAD^^ | sed -ne \$p)
+'
+
test_done
--
2.4.2.389.geaf7ccf
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH/RFCv4 1/2] git-rebase -i: add command drop to remove a commit
Galan Rémi remi.galan-alfo...@ensimag.grenoble-inp.fr writes:
diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index dc3133f..869cc60 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -152,6 +152,7 @@ Commands:
s, squash = use commit, but meld into previous commit
f, fixup = like squash, but discard this commit's log message
x, exec = run command (the rest of the line) using shell
+ d, drop = remove commit
These lines can be re-ordered; they are executed from top to bottom.
@@ -508,6 +509,23 @@ do_next () {
$comment_char*|''|noop)
mark_action_done
;;
+ drop|d)
+ if test -z $sha1
+ then
+ warn Missing SHA-1 in 'drop' command.
+ die Please fix this using 'git rebase --edit-todo'.
Is this a sensible piece of advice, though? The user edited the
line away and it does not have the commit object name anymore.
How does she fix it in the editor? The same puzzlement applies
to the other message.
+ sha1_verif=$(git rev-parse --verify --quiet $sha1^{commit})
+ if test -z $sha1_verif
As you are not using the returned string at all, instead of wasting
a shell variable, it would be better to decide to come into this
block by using the exit status from rev-parse (it exits with
non-zero status upon failure).
+ then
+ warn '$sha1' is not a SHA-1 or does not represent \
+ a commit in 'drop' command.
+ die Please fix this using 'git rebase --edit-todo'.
+ fi
+
+ mark_action_done
+ ;;
pick|p)
comment_for_reflog pick
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
