Re: [GSoC update] Sequencer for inclusion

From: Jonathan Nieder <jrnieder@gmail.com>
Date: Jul 11, 2011, 17:17 UTC
Message-ID: <20110711171713.GA5963@elie>
In-Reply-To: <1310396048-24925-1-git-send-email-artagnon@gmail.com>

Hi Ram!

Ramkumar Ramachandra wrote:

> I'm excited to announce the first iteration of a fresh series

The basic questions:

 - has the result of applying each patch in the new iteration been
   tested (for example by rebasing interactively with "exec make test"
   after each "pick" line)?

 - what changed since last time, for each patch?  (For the future, the
   space under the "---" is generally a good place to put that
   information.)

 - what proposed changes did not make it in, for each patch?  If any,
   did they not fit well with the design, or was it more a matter of
   "sure, that would be nice, but let's get this in first"?

[...]

> I would have liked to reuse the gitconfig parser as-is for the opts
> parsing, but it's too tangled up in config.c.  I think it's safe to
> say that the opts file format deviates only slightly from the
> gitconfig format, and I'm quite happy with the end result.

To be precise, the format used includes

	strategy-option = patience | renormalize

to represent the effect of "-Xpatience -Xrenormalize". My only worry about that is that the "|" can sound like "or", which would seem strange to a user that does not necessarily develop software (so is not thinking about bitfields). The format used in config files puts

	strategy-option = patience
	strategy-option = renormalize

as separate lines.

> 4. New tests and documentation.  There's really no end to this

Once each new feature has been documented and each new feature or fixed bug has an associated test, you've reached the end of this.

Meanwhile, it's true that it's possible to improve tests and documentation beyond that, but that would not fit well in the context of this series anyway.

[...]

> The series is becoming large and unmanagable --
> we can fix minor issues after the merge.

[...]

Show 17 quoted lines

>   advice: Introduce error_resolve_conflict
>   revert: Inline add_message_to_msg function
>   revert: Don't check lone argument in get_encoding
>   revert: Rename no_replay to record_origin
>   revert: Propogate errors upwards from do_pick_commit
>   revert: Eliminate global "commit" variable
>   revert: Introduce struct to keep command-line options
>   revert: Separate cmdline parsing from functional code
>   revert: Don't create invalid replay_opts in parse_args
>   sequencer: Announce sequencer state location
>   revert: Save data for continuing after conflict resolution
>   revert: Save command-line options for continuing operation
>   revert: Introduce a layer of indirection over pick_commits
>   reset: Make hard reset remove the sequencer state
>   revert: Remove sequencer state when no commits are pending
>   revert: Introduce --reset to remove sequencer state
>   revert: Introduce --continue to continue the operation

My main worry is still the commit messages. They don't need to be elaborate but they should explain the purpose and effect of each patch. Part of the reason I care is that it makes the life of future readers using "git log -S" or "git bisect" before changing that code much easier. Think of them like a special kind of comments that don't interfere with reading the code straight through.

The main reason I care is that that information makes the code itself easier to review.

I don't know how to move forward on that. I can explain what's missing in each message, but I get the impression that you already understand that and there's something else (e.g., time) preventing them from getting fixed. I could rewrite each commit message, but I am likely to miss things and come up with something that sounds vaguely plausible but doesn't accurately explain the intent. What do you suggest?

Jonathan