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
