Re: [PATCH/RFCv2 1/2] git-rebase -i: add command "drop" to remove a commit

[Junio C Hamano <gitster@pobox.com>]

Remi Galan Alfonso <remi.galan-alfonso@ensimag.grenoble-inp.fr>
writes:

> Junio C Hamano <gitster@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.