Re: [PATCH/RFCv2 1/2] git-rebase -i: add command drop to remove a commit
Junio C Hamano gits...@pobox.com writes: As long as what is given to 'drop' is checked when it matters (e.g. when the code in patch 2/2 tries see if some commits in the original list are no longer there in order to warn sees drop foo bar where foo is obviously not an object name in the original list, that should be checked), it is fine. And I agree 1/2 is not the place to do so, even though it may be easier from the implementation point of view (which is why I mentioned the possibility in the review of that patch). I disagree, I think that that either the checking for the 'drop' command should either be in the 1/2 where it is introduced or in the function check_commits introduced by 2/2 but in a separate commit/patch. The 2/2 checks if there are removed commits to have the possibility to avoid silent loss of information. It is not its role to check if the SHA-1 following 'drop' are correct. What I think should be the best for now is checking the SHA-1 following 'drop' in 1/2 (or not checking at all) and change the whole checking system of rebase in a later patch (e.g. check beforehand (probably in check_commits) if the commands exist, if the SHA-1 are correct and possibly other things). 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
Re: [PATCH/RFCv2 1/2] git-rebase -i: add command drop to remove a commit
Remi Galan Alfonso remi.galan-alfo...@ensimag.grenoble-inp.fr writes: Junio C Hamano gits...@pobox.com writes: As long as what is given to 'drop' is checked when it matters (e.g. when the code in patch 2/2 tries see if some commits in the original list are no longer there in order to warn sees drop foo bar where foo is obviously not an object name in the original list, that should be checked), it is fine. And I agree 1/2 is not the place to do so, even though it may be easier from the implementation point of view (which is why I mentioned the possibility in the review of that patch). I disagree, I think that that either the checking for the 'drop' command should either be in the 1/2 where it is introduced or in the function check_commits introduced by 2/2 but in a separate commit/patch. The 2/2 checks if there are removed commits to have the possibility to avoid silent loss of information. It is not its role to check if the SHA-1 following 'drop' are correct. Suppose you started from this insn sheet: pick 2c9c1c5 gostak: distim doshes pick e3b601d pull: use git-rev-parse... pick eb2a8d9 pull: handle git-fetch'... and then after letting the user edit, you got this back: pick 2c9c1c5 gostak: distim doshes drop e3b601d pull: use git-rev-parse... edit eb2a8d9 pull: handle git-fetch'... In the new world order to punish those who simply remove lines to signal that they want the commits omitted from replaying, you would want to see all commit object names that was in the original insn sheet appear in the post-edit insn sheet. I'd presume that the way to do so is to collect all the object names from each insn sheet and compute the set difference. The first one has three commit object names, the same three commit object names appear in the second one, and all is well. 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'... As a part of collecting object names from the list before and after editing into two separate sets, and computing the set difference in order to notice potential mistakes, you would need to make sure that you got these two sets collected _correctly_, but you do not know from the above sample input what the user wanted to do with the first line. Did the user tried to drop something else but the object name has gone missing by mistake? Did the user wanted to drop the first one but made mistake while editing 'pick' away into 'drop'? 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). So logically I would think 2/2 is where the verification should happen, but doing it as a part of 1/2 may be easier to do. The end result would not make a difference, and that is why I said it would be OK either way. I am puzzled as to what you are disagreeing with, and why. -- 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/RFCv2 1/2] git-rebase -i: add command drop to remove a commit
Remi Galan Alfonso remi.galan-alfo...@ensimag.grenoble-inp.fr writes: Ideally, I think we should do a sanity check before starting the rebase, and error out if we encounter an invalid command, a command that should be followed by a valid sha1 and does not, ... But currently, we do the verification while applying commands, and I don't think there's anything really helpful to do if we encounter a drop this-is-not-a-sha1 command. While I agree that doing a sanity check before starting the rebase is a better idea, it would not be logic to do so only for the 'drop' command, it should be done for all of the various commands (as you said, checking if the commands are valid, checking if the SHA-1 following the command is valid if it was expecting one...). Yes, that's what I had in mind. However I think that it should be in a different patch, changing the whole checking system of rebase is not directly linked to the idea of the 'drop' command. Agreed. Discussions on this list often lead to Oh, BTW, shall we do XYZ also?, but you shouldn't take this kind of remark as blocking (as long as XYZ is not incompatible with your patch, which is the case here). -- Matthieu Moy http://www-verimag.imag.fr/~moy/ -- 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/RFCv2 1/2] git-rebase -i: add command drop to remove a commit
Ideally, I think we should do a sanity check before starting the rebase, and error out if we encounter an invalid command, a command that should be followed by a valid sha1 and does not, ... But currently, we do the verification while applying commands, and I don't think there's anything really helpful to do if we encounter a drop this-is-not-a-sha1 command. While I agree that doing a sanity check before starting the rebase is a better idea, it would not be logic to do so only for the 'drop' command, it should be done for all of the various commands (as you said, checking if the commands are valid, checking if the SHA-1 following the command is valid if it was expecting one...). However I think that it should be in a different patch, changing the whole checking system of rebase is not directly linked to the idea of the 'drop' command. 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
Re: [PATCH/RFCv2 1/2] git-rebase -i: add command drop to remove a commit
Matthieu Moy matthieu@grenoble-inp.fr writes: Discussions on this list often lead to Oh, BTW, shall we do XYZ also?, but you shouldn't take this kind of remark as blocking (as long as XYZ is not incompatible with your patch, which is the case here). Yeah, thanks for clarification. As long as what is given to 'drop' is checked when it matters (e.g. when the code in patch 2/2 tries see if some commits in the original list are no longer there in order to warn sees drop foo bar where foo is obviously not an object name in the original list, that should be checked), it is fine. And I agree 1/2 is not the place to do so, even though it may be easier from the implementation point of view (which is why I mentioned the possibility in the review of that patch). -- 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/RFCv2 1/2] git-rebase -i: add command drop to remove a commit
Galan Rémi remi.galan-alfo...@ensimag.grenoble-inp.fr writes:
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
---
Matthieu, is this part of your class project?
I vaguely recall that your school wants your sign-off to release
patches to us or something like that, and that I saw some other
patches came with your sign-off, so I am being curious.
@@ -505,7 +506,7 @@ do_next () {
rm -f $msg $author_script $amend || exit
read -r command sha1 rest $todo
case $command in
- $comment_char*|''|noop)
+ $comment_char*|''|noop|drop|d)
mark_action_done
;;
pick|p)
Is this sufficient?
If you are going to do something in 2/2 that relies on the format of
this line being correct (as opposed to noop or # that can have
any garbage on the remainder of the line), wouldn't you want to at
least check $sha1 is sensible?
--
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/RFCv2 1/2] git-rebase -i: add command drop to remove a commit
Junio C Hamano gits...@pobox.com writes: Galan Rémi remi.galan-alfo...@ensimag.grenoble-inp.fr writes: 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 --- Matthieu, is this part of your class project? Yes it is. I vaguely recall that your school wants your sign-off to release patches to us or something like that, and that I saw some other patches came with your sign-off, so I am being curious. There's no strict requirement, but since the students are working under my guidance I think it makes sense to have my sign-off. The rule which applies by default for students is don't distribute your code (symmetrical to don't use other students code), which obviously does not apply for this project: I do allow them to contribute their code! -- Matthieu Moy http://www-verimag.imag.fr/~moy/ -- 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/RFCv2 1/2] git-rebase -i: add command drop to remove a commit
Junio C Hamano gits...@pobox.com writes: Is this sufficient? If you are going to do something in 2/2 that relies on the format of this line being correct (as opposed to noop or # that can have any garbage on the remainder of the line), wouldn't you want to at least check $sha1 is sensible? That's also something that I was wondering, I wrote about it in the 0/2 part of this patch, I wanted some opinion about it. If there is no opposition on the subject, I will have it ready for the v3 of the patch. Quote of that part of the 0/2 for more clarity: Galan Rémi remi.galan-alfo...@ensimag.grenoble-inp.fr writes: For the 'drop' command, maybe instead of just doing the same thing as noop, checking if the SHA-1 that supposedly follow does exist could be a good idea. -- 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
