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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Junio C Hamano <gitster@xxxxxxxxx> 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 <gitster@xxxxxxxxx> 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 majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html




[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]