Re: [RFC/PATCH 2/4] Add git-sequencer prototype documentation

[Jakub Narebski <jnareb@gmail.com>]

Hi,

On Tue, 1 Jul 2008, Stephan Beyer wrote:
> On Tue, Jul 01, 2008 at 06:02:54AM -0700,
> Jakub Narebski <jnareb@gmail.com> wrote:
>> Stephan Beyer wrote:
>>>
>>> +git-sequencer will usually be called by another git porcelain, like
>>> +linkgit:git-am[1] or linkgit:git-rebase[1].
>> 
>> I hope that it could be also used by git-cherry-pick and git-revert,
>> so it would be possible to pick more than one commit...
> 
> Right, you've already mentioned it several times. ;-)

:blush: I'm sorry about that...

> But I haven't included it currently, because git-sequencer currently *uses*
> cherry-pick/revert to pick/revert commits, and I think this can lead to
> some confusion.
[...]
> So I'm only concentrating on rebase and am users until the builtin sequencer
> is in good shape.

Ah, I understand.  It _is_ quite reasonable.

>>> +merge [options] <commit-ish1> <commit-ish2> ... <commit-ishN>::
>>> +	Merge commits into HEAD.
>> 
>> Nice.
>> 
>> "HEAD" mean last picked up / created commit, isn't it?
> 
> Right. This is used throughout the document...
> I thought it is clear and better to use than always describing around it
> by "last created commit".

O.K.

>>> +ref <ref>::
>>> +	Set ref `<ref>` to the current HEAD, see also
>>> +	linkgit:git-update-ref[1].
>> 
>> So this functions like "git reset --soft <ref>", isn't it?
> 
> No. Why do you think that? `ref` is set, and not HEAD.
> I think the description makes that clear.

Ah.  I'm sorry.  So it is like "git branch <ref>", isn't it?

What is important is: does it update reflog (correctly)?
 
>>> +squash [options] --from <mark>::
>>> +	Squash all commits from the given mark into one commit.
>>> +	There must not be any `merge` instructions between the
>>> +	`mark` instruction and this `squash --from` instruction.
>> 
>> Can you use <commit> instead of <mark> here?
> 
> I wanted to make it clear that you can only specify a mark:
> a commit that has been somehow used in this sequencer session.
> 
> Or have you meant that you think it is a bad restriction to only
> allow marks and no commits?
> I think this is a useful thing, because it's user-safe and I
> cannot imagine a case where you want to give a sha1 or a
> tag or branch or something.
> 
> Here an example why it is useful for user-editing:
> 
> (on commit f00babe)
> 	mark :1
> 	pick badcebab
> 	patch foo
> 	pick dadadada
> 	squash -m "Foo the cebab" --signoff --from :1
> 
> This squashes all between the mark and the squash into one commit,
> on top of f00babe.
 
Ah, so squash --from <mark> picks up everything since "mark <mark>",
but does not include marked commit!  Clever!  In this case allowing
only <mark> is a good idea, IMVHO.
 
>>> +	--include-merges;;
>>> +		Sanity check does not fail if you have merges
>>> +		between HEAD and <mark>.
>> 
>> How do you squash merges?  Creating merge with an union of parents,
>> excluding commits which got squashed?
> 
> My squashes are realized using git reset --soft ... and then commit.
> I think this makes only sense when there are no merges in between,
> so I added the check, but if someone wants to squash merges, he should
> be able to do it.
>
> To somehow answer your question: I do not care what the result is,
> because I do not know what the result "should be".

O.K.  I guess that is something left for later, especially that
forbidding merges in squashed commit is default (and you can always
do without merges, I think).

>>> +RETURN VALUES
>>> +-------------
>>> +* any other value on error, e.g.
>>> +  running git-sequencer on a bare repository.
>> 
>> Don't you enumerate those return values?
> 
> In one of the very first spec versions, it was value 1, but:
[...]
> 	$ grep -m 1 -A 4 die_builtin usage.c
> 	static NORETURN void die_builtin(const char *err, va_list params)
> 	{
> 		report("fatal: ", err, params);
> 		exit(128);
> 	}

Perhaps stating that it returns 128 on fatal error, or rewording
that any other return value means some fatal error (does it?)
would be better.  But that are details...

-- 
Jakub Narebski
Poland