On Wed, Jun 5, 2013 at 1:13 PM, Junio C Hamano <gitster@xxxxxxxxx> wrote: > Felipe Contreras <felipe.contreras@xxxxxxxxx> writes: > >> On Tue, Jun 4, 2013 at 1:30 PM, Junio C Hamano <gitster@xxxxxxxxx> wrote: >>> Felipe Contreras <felipe.contreras@xxxxxxxxx> writes: >>> >>>> You didn't answer, what happens when you run --skip-empty and --allow-empty? >>> >>> I'll answer to a slightly different question: What should happen? >>> >>> I think it should error out, because --allow-empty is about >>> "allowing empty commits to be preserved", and --skip-empty is about >>> "skipping empty commits (as it says on the package)". >> >> Exactly, so they are related after all. However, --allow-empty says this: >> >> "By default, cherry-picking an empty commit will fail," > > OK, that is a very good point. Especially because by the time > reader reaches this new description, --allow-empty has already > mentioned this, we can just be brief and it is sufficient to say > "Instead of failing," upfront. > >> In fact, it might make sense to change the default in v2.0 to >> --empty-commits=skip. If it makes sense in 'git rebase', why doesn't >> it in 'git cherry-pick'? > > I think the primary reason behind the "Why are you picking a no-op? > Let me stop to make sure you didn't make a mistake." is because > cherry-pick and revert have long been operations for a single commit > explicitly given by the user, not bunch of commits in a range. Yeah, but this has changed already. > These two operating modes are conceptually very different, even when > we consider scripted use. A script that feeds one commit at a time > has a chance to do patch equivalence or user-interactive filtering > on its own, and would be helped by the "are you sure you meant to > record that no-op?" check if it filtered in a wrong way (e.g. the > user specified an empty commit by mistake). One that feeds a range, > on the other hand, relies on or at least expects cherry-pick to have > such smart, and a smarter cherry-pick could select what to pick from > the given range. How would a script know that a single pick ends up as a no-op? It can't know what is the reason the cherry-pick failed. The only way to know for sure is to check the last commit, and for that we don't need the last cherry-pick to fail. > In the longer term, especially if we envision to move large part of > logic to prepare the sequencer insn list from rebase to cherry-pick, > a ranged cherry-pick should learn a way to filter the range with > patch equivalence, for example. Once that happens, the behaviour > would become inconsistent at the end-user level if we stopped at a > commit only because it was not exactly patch equivalent to another > one that is already on the branch we are cherry-picking to, but > ended up to be a no-op, while we did not stop at another commit > because patch equivalence filtered it out beforehand to skip it. > Skipping a no-op by default would remove that inconsistency. > > So in that sense, one could argue that it may be a bugfix to skip > commits that become no-op when replayed, when picking a range of > commits (but not a single one). And I do not think you would need > to wait until 2.0 for such a change. Right. But first we need to agree that failing an empty cherry-pick serves no purpose. -- Felipe Contreras -- 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