Re: [PATCH] revert: add --stdin option to read commits from stdin

From: Christian Couder <chriscool@tuxfamily.org>
Date: Jun 15, 2010, 03:28 UTC
Message-ID: <201006150528.05265.chriscool@tuxfamily.org>
In-Reply-To: <20100614052027.GA1509@burratino>

On Monday 14 June 2010 07:20:27 Jonathan Nieder wrote:

Show 15 quoted lines

> Christian Couder wrote:
> > --- a/t/t3508-cherry-pick-many-commits.sh
> > +++ b/t/t3508-cherry-pick-many-commits.sh
> > @@ -92,4 +92,14 @@ test_expect_failure 'cherry-pick -3 fourth works' '
> >  	test "$(git rev-parse --verify HEAD)" != "$(git rev-parse --verify
> > fourth)" '
> >
> > +test_expect_success 'cherry-pick --stdin works' '
> > +	git checkout master &&
> > +	git reset --hard first &&
> 
> [...]
> 
> This test fails for me as written, since the previous test leaves some
> files in an unmerged state.  Patch 1 below works around that.

Ok.

Show 24 quoted lines

> > --- a/builtin/revert.c
> > +++ b/builtin/revert.c
> > @@ -79,7 +80,7 @@ static void parse_args(int argc, const char **argv)
> >  	}
> >
> >  	commit_argc = parse_options(argc, argv, NULL, options, usage_str, 0);
> 
> [...]
> 
> > @@ -527,10 +528,12 @@ static void prepare_revs(struct rev_info *revs)
> >  {
> >  	int argc = 0;
> >  	int i;
> > -	const char **argv = xmalloc((commit_argc + 4) * sizeof(*argv));
> > +	const char **argv = xmalloc((commit_argc + 5) * sizeof(*argv));
> >
> >  	argv[argc++] = NULL;
> >  	argv[argc++] = "--no-walk";
> > +	if (read_stdin)
> > +		argv[argc++] = "--stdin";
> 
> Ah, I see the problem now.  But it would be even nicer to allow arbitrary
> rev-list options, so a person could ‘git cherry-pick --reverse a..b’,
> for example.

Yeah, I agree that is nicer.

Show 10 quoted lines

> In other words, how about something like patch 2 below?
> 
> Patch 3 is a small cleanup, as a bonus.
> 
> Christian Couder (1):
>   revert: accept arbitrary rev-list options
> 
> Jonathan Nieder (2):
>   t3508 (cherry-pick): futureproof against unmerged files
>   revert: do not rebuild argv on heap

I get "indent with spaces" errors when I apply patches 2/3 and 3/3:

$ git am ../messages/\[PATCH\ 2_3\]\ revert_\ accept\ arbitrary\ rev-list\ 
options.mbox
Applying: revert: accept arbitrary rev-list options                                                                      
/home/christian/work/git/.git/rebase-apply/patch:35: indent with spaces.                                                 
                                    PARSE_OPT_KEEP_UNKNOWN);

$ git am ../messages/\[PATCH\ 3_3\]\ revert_\ do\ not\ rebuild\ argv\ on\ 
heap.mbox
Applying: revert: do not rebuild argv on heap                                                                        
/home/christian/work/git/.git/rebase-apply/patch:33: indent with spaces.                                             
                                    PARSE_OPT_KEEP_ARGV0 |                                                           
warning: 1 line adds whitespace errors.

But otherwise it all looks very good to me.

Even something like "git cherry-pick -3 fourth" is now working after patch 2/3, so all test cases now pass. This is because "--no-walk" does not always take over "-3" it looks like it depends on the order of the arguments.

For example I get:

$ git rev-list --no-walk -3 --reverse fourth 453a04748224b3f212580d1195b452334d346e75 e85abe28a2b8ef771f760575b325f4c41f9c815f 94d3184b3f0dcfebb393faf5a122dc429d775538

but:

$ git rev-list -3 --no-walk --reverse fourth 94d3184b3f0dcfebb393faf5a122dc429d775538

I will post an updated v2 series without the whitespace errors and with a few documentation and test updates in patch 2/3.

Thanks, Christian.