Re: [RFC] Two conceptually distinct commit commands

Carl Worth [off-list ref] writes:

    ... The case I would use it for is fairly common,
    (and something that I think will speak to Junio who often brings
    up a similar scenario).

    Here's where I would like this functionality:

	I receive a patch while I'm in the middle of doing other work,
	(but with a clean index compared to HEAD, which is what I've
	usually). The patch looks good, so I want to commit it right
	away, but I do want to separate it into two or more pieces,
	(commonly this is because I want to separate the "add a test
	case demonstrating a bug" part from the "fix the bug"
	part).

(This is offtopic)

I often faced situations like that during git.git history.  One
patch to expose the bug in the existing code, and another to fix
it.  And there are three ways to make that commit.

 (1) one commit exposes, then another fixes.
 (2) one commit fixes, then another verifies the bug is no more.
 (3) one commit to include both.

In my experience, (1) is only useful during the time I am coming
up with the fix (if I am fixing it myself) or during the time I
am reviewing and committing the fix (if I am applying somebody
else's patch).  Committing in that order lets me validate the
brokenness after making the first commit, and then lets me feel
good by not seeing that problem after the second commit.  But
this means I deliberately record a state that is known not to
pass the test, which means it is a problem for somebody else in
the future when the history needs to be bisected to hunt for an
unrelated bug.  If the "test" is just an optional test in the
test suite, then it is easy to work around (the person who is
bisecting can ignore that bug by not running that particular
test), but if it is an assert somewhere deep inside the code,
ignoring it is not very easy, especially if the person who is
bisecting is not familiar with that part of the code.

What I recommend people to do these days is either (2) or (3),
but do so _after_ verifying the fix in the reverse order.  The
criteria to choose between (2) or (3) is fairly simple: if the
"test" is easily separable (e.g. changes to a test script file
that does not overlap with the "fix" patch), roll both in one
commit.  Then it would not later cause problems for bisection.

Enough of offtopic.

The sequence to split a patch in place would be (I'll speak in
the present tense and pretend Nico's "git add" does not exist
yet):

	git apply
        git update-index <files for the first batch>
        git commit
        git commit -a ;# the remainder

so you do not necessarily need a new "concept".  It is
inconvenient that you need to deal with new files, but that is a
minor detail compared to a bigger problem I'll mention in the
next paragraph.

I think the problem with your thinking is that you still are
talking from "file boundary matters" point of view.  The above
sequence is only useful if the patch to be split is separable
cleanly at the file boundary, in which case, I would (and I've
done so often) split the patch file in my editor and run two
independent "git apply + git commit" sequences.  That way, I
could test each in isolation (perhaps with some dirty working
tree state if I do not stash them away and do reset --hard).
Anything more realistic and practically useful would require
splitting of the patch in semantic ways regardless of file
boundaries.

As I have already said (and you seemed to share the same
discipline), I do not like people committing anything
non-trivial that is not tested.  The patch you received might
not have been tested by the submitter, but there is a chance
that it might have been ;-).  But with the way you said you want
to make the commits in the message I am responding to, the first
commit would never have been tested by anybody in isolation, not
by the original submitter even if he tested the patch before
giving it to you, nor you -- your working tree had either none
of his patch or all of it, and never was in the state with only
the first batch.

So while at the theoretical level I understand what you would
want to achieve with the "single patch that should have been
sent as two patch series" example, from the practical point of
view I do not see much value in it (because "file boundary
matters" is a minority case that is not very interesting), and
from the discipline point of view I would rather not want to
have such a too-convenient way to commit things that were
different in nontrivial ways from what you had in your working
tree (if we use something like Darcs record to update
hunk-by-hunk).

    The old behavior is still available with the --include option, but
    nobody has ever come out in favor of that being a useful command,

That is a slight overstatement.  When committing with paths
argument to conclude a merge, --include semantics is the only
variant that makes sense (--only semantics is so wrong that
there is a safety valve that catches it).  Most of the time,
however, if I need to resolve and record a complicated merge, I
would either do "update-index" to clear the deck of the paths
that I already dealt with, and by the time I would type "git
commit", I have an index that has exactly what I want in the
merge commit.  That makes --include a less often used form.  If
a merge is small and easy to resolve at only a few paths, it
still is handy to say "git commit -i resolved-path.c".  It does
not add anything to the semantics -- it is only a typesaver.

On Tue, 05 Dec 2006 20:53:30 -0800, Carl Worth wrote:

On Tue, 05 Dec 2006 17:13:20 -0800, Junio C Hamano wrote:
	2. Avoiding a change of semantics triggered by merely applying
	   pathname arguments without any command-line option or
	   alternate command name.

By the way, the original command-line convention I used in the
proposal was that the omission of an optional argument should be
equivalent to supplying some default argument. Here's another
convention that is also useful to examine:

	Adding path-name arguments limits the behavior of the command,
	(and does not otherwise change the semantics).

I don't know that this is as universal a convention outside of git,
but it's quite strong within git. The path name limiting exists in
deep parts of the machinery and allows for things like:

	git log -- paths...	# path-limited version of "git log"
	git diff -- paths...	# path-limited version of "git diff"
	etc.

It's interesting to look at how the various commit commands fit (or
do not fit) this convention:

  git commit paths...
  git commit --only paths...

	This command cannot be explained in terms of the semantics of
	"git commit" (without command-line options). This command
	_can_ be explained as a path-limited version of "git commit -a".

  git commit --include paths...

	This command does something _extra_ to the given paths before
	executing the equivalent of "git commit". I think this is a
	fairly unique violation of the path-limiting convention.

The proposal I made with commit-working-tree-content and
commit-index-content consistently follow the path-limiting convention.

I think consistency of command-line conventions like this are
important for making the tool usable. And there have been notable
improvements to consistency of convention in git recently, (for
example, using <since>..[<until>] in git-format-patch rather than
<his> <mine>).

-Carl

On Wed, 06 Dec 2006 10:31:21 -0800, Junio C Hamano wrote:

I am not sure what needs to be commented on at this point, since
it is not yet clear to me where you want your proposal to lead
us.

Thanks for the comments you made here---that's the kind of thing I was
looking for.

As for where I'm trying to lead us, what I really want to do is to
help improve the learnability of git. A big part of that is about
improving the set of "use-oriented" documentation, (which describes
how to achieve tasks, as opposed to what might be termed "technically
oriented" documentation which describes how individual tools work). I
think too much of the existing documentation falls into the second
class.

A parallel thread is already talking about some of the important
organizational aspects of use-oriented documentation. And I agree with
that thread is that the short "attention span" is a primary
consideration for this kind of documentation. The user has a task to
be accomplish, and any text or concepts that aren't contributing to
the solution of that task should be eliminated.

Note that when I talk about eliminating unnecessary concepts, I do not
mean lying to the user about the underlying model or any concepts. We
can't have a sugar-coated tutorial that says one thing, and then
expect users to "unlearn" that if they go deeper into the reference
manual. That's a recipe for disaster.

Also, when I say "use-oriented" I'm not suggesting that the
documentation be shallow. It can go as deep as any workflow we care to
document and introduce whatever concepts of git are necessary to
support that workflow. (There is, though a level at which "technically
oriented" documentation is all that's needed, or even desired, and
that's when the documentation is targeting authors of interfaces that
build on top of git---not users trying to use git to get work done at
the command line).

OK, so if my concern is all about documentation, then what am I doing
proposing new commands or new ways of thinking about existing commands
rather than just sending documentation patches? The problem is that
the current semantics of the following variations of "git commit":

	git commit
	git commit -a
	git commit paths...

defeat the goal of writing good, clean use-oriented documentation. So
there's some adjustment that should be made first. And I don't even
care what the adjustment is, (for example, it doesn't have to be
"commit -a by default"), but please recognize the problem and help me
come up with an acceptable way to fix it.

To demonstrate, let's take the simplest of use cases and try to
document it in as clear a way as possible. Let's imagine we're in a
tutorial where we've just guided the user to making modifications to
several existing, tracked files, (starting from an initial clone, not
an init-db), and the next task to teach the user commit for the first
time. We would like to document both "commit a single modified file"
and "commit all modified files". Here are two approaches that I can
come up with:

1. Any commit involves first "add"ing together new content, and then
   committing the result. For example to commit a single file:

	git add file		# add new content from file
	git commit		# commit the result

   As a shortcut, "commit -a", (or --all) can be used to automatically
   "add" the content of all tracked files before the commit. So the
   common case of committing all tracked files is as easy as:

	git commit -a		# commit content of all tracked files

2. The new content of modified files can be committed by naming the
   files on the "git commit" command line. For example:

	git commit file		# commit new content of file

   As a shortcut, "commit -a", (or --all) can be used to commit the
   content of all tracked files:

	git commit -a		# commit content of all tracked files

Neither of the above is totally satisfactory.

In (1) the user is not presented with a framework that will make sense
of "git commit files...". The expansion of "-a" as "--all" could
easily give the user the impression that "git commit files..." is a
shortcut for "git add files...; git commit", but that's wrong and
could lead to unexpected results and confusion.

In (2) the user is not presented with a framework that will make sense
of "git commit" with no arguments. The user is left to wonder about
why the --all is needed and what it means exactly, (particularly since
"git commit" also commits the content of all tracked files.

Various fixes have been proposed for these potential confusions. For
example, making "git commit files..." default to the behavior of
--include instead of --only would eliminate the confusion I described
for (1). And making -a the default for "git commit" would eliminate
the confusion I described for (2).

However, actually implementing either of those fixes would then break
the initial "commit one file" example from the other approach. Because
of that, the conversation has often fallen into debate over whether
(1) or (2) is the "one true way" to describe git, and which one leads
the user to have an incorrect mental model.

But I think that debate is misguided since both descriptions are
worthwhile and valid. (1) is based around an explanation of what "git
commit" does, and (2) is based around an explanation of what "git
commit files..." does. And both of these commands are very useful
exactly how they are.

It's almost coincidental that "commit -a" fits in logically with
either description.

So what I was trying to get across in this latest thread is that git's
command-line interface already has two slightly different models for
what's going on in a commit. You don't agree with me on that point
yet, (more on that below in my reply).

I really don't care what the final fix is, but I would love to see
documentation with no more complexity than the above that accurately
captures the useful functionality.

And I don't actually have a concrete proposal for a fix yet---I was
just offering the commit-index-content and commit-working-tree-content
ideas as ways to think about the issue. Maybe the two documentation
blurbs above capture it in a better way.

Do you feel like you have a better understanding of what I'm trying to
do now?

I do not agree with your "three commands" or "two semantics"
characterization of the current way "git commit" works.  "git
commit" without any optional argument already acts as if a
sensible default arguments are given, that is "no funny business
with additional paths, commit just what the user has staged
already."

I agree that "git commit" does nothing funny by default. What I was
pointing out is that "git commit" and "git commit paths..." do not
have the same semantics. There's really nothing to debate about
there. There is no argument you can substitute for <paths...> to give
you identical behavior as "git commit". That's a fact.

"git commit" is primarily about committing what has been staged
in the index, and "--all" is just a type-saver short-hand (just
like "--include" is) to perform update-index the last minute and
nothing more.  In other words, "--all" is a variant of the
pathname-less form "git commit".  It is not a variant of "git
commit --only paths..." form, as you characterized.

I hope the documentation blurbs (1) and (2) above show how "commit -a"
can be seen as a variant of either "commit" or "commit files...",
(which themselves are both useful semantics, but demonstrably
distinct).

The pathname form (the "--only" variant) on the surface seem to
work differently, but when you think about it, it is not all
that different from the normal commit.  We explain that it
ignores index, but in the bigger picture, it does not really.

No, it really is different.

the first commit does "jump" the changes already made to the
index, but after it makes the commit, the index has the same
contents as if you did "git update-index a b" where you ran that
"git commit".  In other words, it is just a handy short-hand to
pretend as if you did the above sequence in this order instead:

How could you document "git commit files..." as a shorthand? A
shorthand for what exactly? A shorthand for pretending you didn't just
type the commands you did type that got the index into its current
state, but had instead typed different commands before the commit and
other commands afterwards?

That's crazy. That's not a shorthand. That's just plain different
semantics. The current "git commit files..." command never does commit
the contents of "the" index as a concept presented by "git
commit". (This is independent of the fact that the implementation of
"git commit files..." certainly does use an index file somewhere and
uses it to create a commit object in the same way that "git commit"
uses "the" index).

So I actually think it is a mistake to stress the fact that "git
commit --only paths..." seems to act differently from the normal
"git commit" too much.

I think that would be lying to the users and setting them up to get
confused later. I discussed this above as the confusion that can
result with the explanation of (1). If you teach "git commit" as
commiting "the" index, and de-emphasize that "git commit files..."
is semantically distinct, then how is a user ever supposed to learn
what it is that "git commit files..." is actually doing?

In short, while I understand that your "proposal" shows your own
way to summarize the semantics of "git commit", I am not seeing
what it buys us, and I do not see the need to come up with a
pair of new two commands for making commits (if that is what the
proposal is about, that is, but it is not clear to me if that is
what you are driving at).  I think it would only confuse users.

Forgive me again for being obtuse. I don't think we should necessarily
add two new commands. I was trying to illustrate a problem in the
existing command set, and propose a new way of thinking about the
tasks that the current commands help a user to perform, (committing
content from the working tree or committing content from the index). I
don't actually have a concrete proposal for how to take that way of
thinking and map it to a command set, (and one that would disrupt
current git users as little as possible). I'd love to have some help
with that part.

Is it just me who finds the above a very much made-up example?

Fine. We can ignore that example.

In any case, I should clarify my aversion to partial commits a
bit.  What is more important is to notice that, while you cannot
compile-and-run test what is in the index in isolation (without
a fuse that exports the index contents as a virtual filesystem
-- anybody interested?), you _can_ preview and verify the text
that is going to be committed by comparing the index and the
HEAD.  And for that, your "staging" action (i.e. Nico's "git
add") needs to be a separate step from your "committing" action.

Yes, I often use the index as a place to preview things. And it is
true that I find myself using update-index when I could have used
"commit paths..." precisely because I can preview it once more. But I
do use the "commit paths..." form at times as well. If I have just
reviewed things in "git diff" and there are _really_ obviously
separable pieces I will commit them alone with staging into the index
and reviewing again.

It's probably the case that I skip the explicit staging and extra
preview when I can use a single pathname as the argument to "git
commit".

In other words, I would even love Johannes's "per hunk commit"
idea, at least if it had an option to preview the whole thing
just one more time before committing, and I would love it better
if it had an option for not committing but just updating.

Yes! I've wanted tools to help with per-hunk separation before, but
since I'm so likely to make mistakes while doing that I would only
want that to go into the index so that I could review it before
committing. I guess I might need a per-hunk way to fix up my mistakes
too if I put a hunk into the index that I didn't want to be there.

-Carl

Carl Worth [off-list ref] writes:

... Even if this functionality
weren't made available at all, I'd still be interested in your
comments on the main thrust of my proposal. I think that consists of:

	1. Unifying the two current commands that provide
	   commit-working-tree-content semantics into a single,
	   use-oriented description.

	2. Avoiding a change of semantics triggered by merely applying
	   pathname arguments without any command-line option or
	   alternate command name.

I am not sure what needs to be commented on at this point, since
it is not yet clear to me where you want your proposal to lead
us.

I do not agree with your "three commands" or "two semantics"
characterization of the current way "git commit" works.  "git
commit" without any optional argument already acts as if a
sensible default arguments are given, that is "no funny business
with additional paths, commit just what the user has staged
already."

"git commit" is primarily about committing what has been staged
in the index, and "--all" is just a type-saver short-hand (just
like "--include" is) to perform update-index the last minute and
nothing more.  In other words, "--all" is a variant of the
pathname-less form "git commit".  It is not a variant of "git
commit --only paths..." form, as you characterized.

The pathname form (the "--only" variant) on the surface seem to
work differently, but when you think about it, it is not all
that different from the normal commit.  We explain that it
ignores index, but in the bigger picture, it does not really.

In this sequence:

	edit a b
	git update-index a
	git commit --only b
	git commit --all

the first commit does "jump" the changes already made to the
index, but after it makes the commit, the index has the same
contents as if you did "git update-index a b" where you ran that
"git commit".  In other words, it is just a handy short-hand to
pretend as if you did the above sequence in this order instead:

	edit a b
        git update-index b
        git commit
        git update-index a
        git commit

So I actually think it is a mistake to stress the fact that "git
commit --only paths..." seems to act differently from the normal
"git commit" too much.  It just helps to split the changes in
your working tree if the changes happen to be cleanly separable
at file boundaries (aka "CVS mentality").  When the changes are
not cleanly separable at file boundaries, the "more painfully
index aware" variant also allows you to split the changes in
your working tree in the time dimension:

        edit a
	git update-index a
        edit a
        git commit ;# without paths
	git update-index a
        git commit

In short, while I understand that your "proposal" shows your own
way to summarize the semantics of "git commit", I am not seeing
what it buys us, and I do not see the need to come up with a
pair of new two commands for making commits (if that is what the
proposal is about, that is, but it is not clear to me if that is
what you are driving at).  I think it would only confuse users.

	I receive a patch while I'm in the middle of doing other work,
	(but with a clean index compared to HEAD, which is what I've
	usually). The patch looks good, so I want to commit it right
	away, but I do want to separate it into two or more pieces,
	(commonly this is because I want to separate the "add a test
	case demonstrating a bug" part from the "fix the bug"
	part).
...
Who said I wouldn't test it? I do split commits like this precisely so
that I _can_ test it this way---and git helps a lot here. I do the
split commit, then easily back up to the revision that adds the test
case, verify the test fails before the bug fix, (which is something
the maintainer doesn't get a chance to do with your (2) approach),
then move forward and verify that the test passes after the fix.

So, sure, I haven't ever had that working tree before the commit. But
git makes it easy to get that working tree after I commit and test
everything before I push anything out.

You saw a good patch in the middle of something that you did not
want to lose your working tree changes for.  That good patch was
not really good enough to be applied straight into your tree but
needed tweaking and splitting.  Nevertheless you went ahead and
made two commits out of that patch, even though you were in the
middle of something.  You could not test them right away after
committing because your tree was in no shape to test them in
isolation.  But that is excusable because you would not push
these commits out right away, before you have a chance to test
them by rewinding your working tree when you are done with what
you were originally doing.

Is it just me who finds the above a very much made-up example?

It means the patch (which is good and not good at the same time)
was not all that urgent after all, and it could well have waited
until you are done with what you were originally doing.

In any case, I should clarify my aversion to partial commits a
bit.  What is more important is to notice that, while you cannot
compile-and-run test what is in the index in isolation (without
a fuse that exports the index contents as a virtual filesystem
-- anybody interested?), you _can_ preview and verify the text
that is going to be committed by comparing the index and the
HEAD.  And for that, your "staging" action (i.e. Nico's "git
add") needs to be a separate step from your "committing" action.

In other words, I would even love Johannes's "per hunk commit"
idea, at least if it had an option to preview the whole thing
just one more time before committing, and I would love it better
if it had an option for not committing but just updating.  You
could:

	$ edit foo bar
	: the whole mess in working tree is in no shape to be committed.
        $ git add foo	;# stage the state of the entire file
        $ git hunk-add bar ;# go interactive and update index selectively
	$ git status -v	;# that is "git commit --dry-run --diff"

to review what would be committed.  So while the commit that
would be made may not be compile-and-run tested, I would not
mind partial commit that much (and after all not all the
projects that track their contents with git are not "compiled"
nor "need testing" projects -- they could be tracking plain text
documentation, and the last-minute eyeballing may be a good
enough test for such contents).

On Tue, 05 Dec 2006 17:13:20 -0800, Junio C Hamano wrote:

(This is offtopic)

It's an interesting topic nonetheless, so I'll comment anyway.

 (1) one commit exposes, then another fixes.
 (2) one commit fixes, then another verifies the bug is no more.
 (3) one commit to include both.

I feel very strongly that I want (1) in the history.

unrelated bug.  If the "test" is just an optional test in the
test suite, then it is easy to work around (the person who is
bisecting can ignore that bug by not running that particular
test), but if it is an assert somewhere deep inside the code,
ignoring it is not very easy, especially if the person who is
bisecting is not familiar with that part of the code.

Granted, something like an assert that breaks the library would not be
a useful thing to have in the history. I'm certainly not in favor of
something like that.

I'm talking about tests that demonstrate pre-existing broken-ness in
the code. In the case of cairo, our test suite is entirely optional,
and each test is out-of-process, so even if a test totally crashes the
suite continues and it's easy to ignore that.

But it's not just the correctness test suite. We also have a
performance test suite, and I encourage the same "add test case, then
performance fix" pattern there. This shows an even more obvious
example of why it's useful to have separate commits in the history,
(since someone may want to verify the performance impact on a separate
system at any point in the future, and the two commits makes it easy
to get "before" and "after" for the performance fix results from the
new test case).

The sequence to split a patch in place would be (I'll speak in
the present tense and pretend Nico's "git add" does not exist
yet):

	git apply
        git update-index <files for the first batch>
        git commit
        git commit -a ;# the remainder

Yes, I can use this today, and I do, (as I mentioned in my mail). The
only requirement is that I start with a non-dirty working tree. I can
arrange that, but it would be just a bit less inconvenient if I didn't
have to.

so you do not necessarily need a new "concept".

No, I don't need it. And this "commit-index-content [paths...]" was
the least significant part of my proposal. As I said, originally I was
just going to say this "might be useful in some cases", but then
someone just happened to request this feature on the list at the same
time I was considering the proposal.

Anyway, it spite of this being an accidental feature of y proposal, it
seems to be the only part you commented on. Even if this functionality
weren't made available at all, I'd still be interested in your
comments on the main thrust of my proposal. I think that consists of:

	1. Unifying the two current commands that provide
	   commit-working-tree-content semantics into a single,
	   use-oriented description.

	2. Avoiding a change of semantics triggered by merely applying
	   pathname arguments without any command-line option or
	   alternate command name.

As I have already said (and you seemed to share the same
discipline), I do not like people committing anything
non-trivial that is not tested.

Indeed. I like that discipline very much. And in fact, that's an
important reason that I split a patch that I receive like this, (or
just bounce it, which I often do, and would definitely do if it's not
easy to split)

                                But with the way you said you want
to make the commits in the message I am responding to, the first
commit would never have been tested by anybody in isolation, not
by the original submitter even if he tested the patch before
giving it to you, nor you -- your working tree had either none
of his patch or all of it, and never was in the state with only
the first batch.

Who said I wouldn't test it? I do split commits like this precisely so
that I _can_ test it this way---and git helps a lot here. I do the
split commit, then easily back up to the revision that adds the test
case, verify the test fails before the bug fix, (which is something
the maintainer doesn't get a chance to do with your (2) approach),
then move forward and verify that the test passes after the fix.

So, sure, I haven't ever had that working tree before the commit. But
git makes it easy to get that working tree after I commit and test
everything before I push anything out.

[Incidentally, that's yet _another_ thing people can mention to
friends who come from cvs and think that the separation of commit and
push is annoying. And that's in addition to all of the performance
advantages, the ability to work entirely offline, etc. etc.]

quoted

    The old behavior is still available with the --include option, but
    nobody has ever come out in favor of that being a useful command,

That is a slight overstatement.

OK. I should have worded that as "I wasn't aware of any arguments...".

               That makes --include a less often used form.  If
a merge is small and easy to resolve at only a few paths, it
still is handy to say "git commit -i resolved-path.c".  It does
not add anything to the semantics -- it is only a typesaver.

Oh, OK. I see it now. That's for combining the update-index, (or
"resolve/resolved"---is there any consensus on that being a useful
synonym for update-index here?) and the commit into a single
command. I guess that's just a shortcut I've never used.

So, yes, that shortcut would not fit cleanly into either of my
proposed commit-working-tree-content nor commit-index-content. That
would still require a two-stage:

	git update-index resolved-path.c
	git commit-index-content

-Carl

Hi,

On Tue, 5 Dec 2006, Carl Worth wrote:

On Tue, 05 Dec 2006 17:13:20 -0800, Junio C Hamano wrote:

quoted

(This is offtopic)

It's an interesting topic nonetheless, so I'll comment anyway.

quoted

 (1) one commit exposes, then another fixes.
 (2) one commit fixes, then another verifies the bug is no more.
 (3) one commit to include both.

I feel very strongly that I want (1) in the history.

Note that (1) maybe would reflect history better, but (2) and (3) are way 
nicer to bisecting.

I fell very strongly that I want (3) in the history.

(Though I am guilty of many instances of (2)...)

Ciao,
Dscho