Re: [PATCH 2/5] rebase: fix 'cherry' mode storage

From: Felipe Contreras <felipe.contreras@gmail.com>
Date: May 29, 2013, 03:03 UTC
Message-ID: <51a57014a1184_807b33e18100353@nysa.mail>
In-Reply-To: <7v8v2yyc04.fsf@alter.siamese.dyndns.org>

Junio C Hamano wrote:

Show 5 quoted lines

> Felipe Contreras <felipe.contreras@gmail.com> writes:
> 
> > We don't use the 'rebase-apply'.
> 
> s/.$/; we will use rebase-merge instead./ I think.

We could use 'rebase-apply' or any directory, but currently we don't use any, and 'rebase-apply' is for 'git am'.

Show 20 quoted lines

> > Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
> > ---
> >  git-rebase--cherry.sh | 4 ++++
> >  git-rebase.sh         | 5 ++++-
> >  2 files changed, 8 insertions(+), 1 deletion(-)
> >
> > diff --git a/git-rebase--cherry.sh b/git-rebase--cherry.sh
> > index cbf80f9..ab1f8b7 100644
> > --- a/git-rebase--cherry.sh
> > +++ b/git-rebase--cherry.sh
> > @@ -18,6 +18,9 @@ esac
> >  
> >  test -n "$rebase_root" && root_flag=--root
> >  
> > +mkdir "$state_dir" || die "Could not create temporary $state_dir"
> > +: > "$state_dir"/cherry || die "Could not mark as cherry"
> 
> Style:
> 
> 	>"$state_dir/cherry-pick"

Fix that in git-rebase--interactive.sh then, where this code came from.

> I am not sure if the user _cares_ that internally we use cherry-pick
> when s/he asks us to do a keep-empty, and I suspect "mark as cherry"
> incomprehensible.  I do not even know what is going on at this point
> (yet).

It's the same we do in git-rebase--interactive.sh.

> I _suspect_ that you are using cherry-pick when --keep-empty is given,
> so it might even make sense to talk in the end-users' terms,
> i.e. call this "$state_dir/keep-empty" (I am not sure if calling
> this split half "git-rebase--keep-empty.sh" makes sense, yet).

If git-rebase--cherry(-pick)?.sh does the same as 'git-rebase--am.sh', we can replace it, and make yet another step of replacing scripts (git am) with C code (git cherry-pick). I thought I explained that somewhow, maybe I didn't.

It still makes sense to fix the broken --keep-empty behavior.

-- 
Felipe Contreras