[PATCH] git add -e documentation: rephrase note

[PATCH] git add -e documentation: rephrase note

[Miklos Vajna]

The original note probably wanted to suggest "edit the patch with care",
but actually suggested just editing the first characters on certain
lines, which is a pretty bad suggestion.

Signed-off-by: Miklos Vajna <vmiklos@frugalware.org>
---
 Documentation/git-add.txt |    5 ++---
 1 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/Documentation/git-add.txt b/Documentation/git-add.txt
index 45ebf87..daa1add 100644
--- a/Documentation/git-add.txt
+++ b/Documentation/git-add.txt
@@ -86,9 +86,8 @@ OPTIONS
 	edit it.  After the editor was closed, adjust the hunk headers
 	and apply the patch to the index.
 +
-*NOTE*: Obviously, if you change anything else than the first character
-on lines beginning with a space or a minus, the patch will no longer
-apply.
+*NOTE*: Obviously, if you change the first characters of the lines or
+lines beginning with a space or a minus, the patch will no longer apply.
 
 -u::
 --update::
-- 
1.6.4

Re: [PATCH] git add -e documentation: rephrase note

[Jeff King]

On Thu, Oct 15, 2009 at 12:26:28AM +0200, Miklos Vajna wrote:

> The original note probably wanted to suggest "edit the patch with care",
> but actually suggested just editing the first characters on certain
> lines, which is a pretty bad suggestion.

The intent of "-e" is to pick and choose lines of the diff to apply, or
even to munge the lines. So it is safe to:

  - remove lines with a '+' (don't stage the addition)

  - munge any lines with a '+' (stage modified contents)

  - add lines with a '+' (stage an addition)

  - convert lines with a ' ' to '-' (stage removal)

  - convert lines with a '-' to ' ' (don't stage removal)

It is a bad idea to:

  - delete '-' lines

  - delete ' ' lines

  - add ' ' or '-' line

  - munge the contents of ' ' or '-' lines

But that is just off the top of my head; perhaps others can come up with
corner cases where one of the safe things breaks.

> -*NOTE*: Obviously, if you change anything else than the first character
> -on lines beginning with a space or a minus, the patch will no longer
> -apply.
> +*NOTE*: Obviously, if you change the first characters of the lines or
> +lines beginning with a space or a minus, the patch will no longer apply.

So this is warning about changing or deleting ' ' or '-' lines, which is
good. But you can change the first characters of those lines, but only
by interchanging the two. So I think both the previous text and your
text don't cover all cases.

Maybe it makes sense instead to do a quick overview of what is OK and
what is not (like the list above).

-Peff

[PATCH] git add -e documentation: rephrase note

[Miklos Vajna]

Add a quick overview about what is OK and what is not to cover all
cases.

Signed-off-by: Miklos Vajna <vmiklos@frugalware.org>
Signed-off-by: Jeff King <peff@peff.net>
---

On Wed, Oct 14, 2009 at 07:04:34PM -0400, Jeff King <peff@peff.net> wrote:
> Maybe it makes sense instead to do a quick overview of what is OK and
> what is not (like the list above).

Here it is.

 Documentation/git-add.txt |   18 +++++++++++++++---
 1 files changed, 15 insertions(+), 3 deletions(-)

diff --git a/Documentation/git-add.txt b/Documentation/git-add.txt
index 45ebf87..1ea074c 100644
--- a/Documentation/git-add.txt
+++ b/Documentation/git-add.txt
@@ -86,9 +86,21 @@ OPTIONS
 	edit it.  After the editor was closed, adjust the hunk headers
 	and apply the patch to the index.
 +
-*NOTE*: Obviously, if you change anything else than the first character
-on lines beginning with a space or a minus, the patch will no longer
-apply.
+The intent of this option is to pick and choose lines of the diff to
+apply, or even to munge the lines. So it is safe to:
++
+* remove lines with a "+" (don't stage the addition)
+* munge any lines with a "+" (stage modified contents)
+* add lines with a "+" (stage an addition)
+* convert lines with a " " to "-" (stage removal)
+* convert lines with a "-" to " " (don't stage removal)
++
+It is a bad idea to:
++
+* delete "-" lines
+* delete " " lines
+* add " " or "-" lines
+* munge the contents of " " or "-" lines
 
 -u::
 --update::
-- 
1.6.5

Re: [PATCH] git add -e documentation: rephrase note

[Jeff King]

On Mon, Oct 19, 2009 at 02:09:00AM +0200, Miklos Vajna wrote:

> Add a quick overview about what is OK and what is not to cover all
> cases.
> 
> Signed-off-by: Miklos Vajna <vmiklos@frugalware.org>
> Signed-off-by: Jeff King <peff@peff.net>

Please be careful with sign-offs. While your patch text is an adaptation
of what I said, and while it is true that I personally would never say
anything on the list that was not free for use in git, I did not in fact
do such a sign-off, which is what that line is supposed to be
documenting.

In this case, I think you probably just wanted to say "text from Jeff
King" or even "From: Jeff King <peff@peff.net>".

That being said,

  Signed-off-by: Jeff King <peff@peff.net>

:)

I still have a few comments, though.

> +The intent of this option is to pick and choose lines of the diff to
> +apply, or even to munge the lines. So it is safe to:
> ++
> +* remove lines with a "+" (don't stage the addition)
> +* munge any lines with a "+" (stage modified contents)

Do we want to use "munge" here? Maybe something a little more specific
like "modify the content of" is more appropriate for documentation.

> +* add lines with a "+" (stage an addition)
> +* convert lines with a " " to "-" (stage removal)
> +* convert lines with a "-" to " " (don't stage removal)

Is " " going to render in an obvious-to-read way? I checked the html
version and the manpage version in my terminal, and they looked OK, but
I would not be surprised if somebody has a font setup that makes it a
little hard to read.

> ++
> +It is a bad idea to:
> ++

Annoyingly, this renders for me as:

           ·   convert lines with a "-" to " " (don’t stage removal)

               It is a bad idea to:

           ·   delete "-" lines

(the spacing of which will hopefully not be destroyed in transit). In
other words, the "+" list continuation makes the "it is a bad idea" text
part of the "good idea" list, instead of continuing the definition of
the "-e" option. I think we can fix it with an open block marker. I'll
see what I can do.

-Peff

Re: [PATCH] git add -e documentation: rephrase note

[Jeff King]

On Mon, Oct 19, 2009 at 12:34:18AM -0400, Jeff King wrote:

> I still have a few comments, though.
> [...]
> (the spacing of which will hopefully not be destroyed in transit). In
> other words, the "+" list continuation makes the "it is a bad idea" text
> part of the "good idea" list, instead of continuing the definition of
> the "-e" option. I think we can fix it with an open block marker. I'll
> see what I can do.

Hmph. Here is my attempt. The text is (I hope) more clear, but I am
still having trouble with the formatting. It looks fine in the HTML
version, and if I am reading the XML right, the XML is correct. But
docbook seems to screw up converting the XML to roff, giving this:

           ·   convert removal lines to context lines (don’t stage removal)
               Similarly, your patch will likely not apply if you:

           ·   add context or removal lines

Am I missing something, or is it really a docbook bug? Does it render
better for anybody else?

-- >8 --
Subject: [PATCH] docs: give more hints about how "add -e" works

The original text was not very descriptive about what you
can and can't do; let's try to enumerate all cases.

Signed-off-by: Jeff King <peff@peff.net>
---
 Documentation/git-add.txt |   22 +++++++++++++++++++---
 1 files changed, 19 insertions(+), 3 deletions(-)

diff --git a/Documentation/git-add.txt b/Documentation/git-add.txt
index 45ebf87..b0a8420 100644
--- a/Documentation/git-add.txt
+++ b/Documentation/git-add.txt
@@ -86,9 +86,25 @@ OPTIONS
 	edit it.  After the editor was closed, adjust the hunk headers
 	and apply the patch to the index.
 +
-*NOTE*: Obviously, if you change anything else than the first character
-on lines beginning with a space or a minus, the patch will no longer
-apply.
+The intent of this option is to pick and choose lines of the diff to
+apply, or even to modify the contents of lines to be staged. There are
+three line types in a diff: addition lines (beginning with a plus),
+removal lines (beginning with a minus), and context lines (beginning
+with a space). In general, it should be safe to:
++
+--
+* remove addition lines (don't stage the line)
+* modify the content of any addition lines (stage modified contents)
+* add new addition lines (stage the new line)
+* convert context lines to removal lines (stage removal of line)
+* convert removal lines to context lines (don't stage removal)
+--
++
+Similarly, your patch will likely not apply if you:
++
+* add context or removal lines
+* delete removal or context lines
+* modify the contents of context or removal lines
 
 -u::
 --update::
-- 
1.6.5.1.123.gcaaf

Re: [PATCH] git add -e documentation: rephrase note

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> Hmph. Here is my attempt. The text is (I hope) more clear, but I am
> still having trouble with the formatting. It looks fine in the HTML
> version, and if I am reading the XML right, the XML is correct. But
> docbook seems to screw up converting the XML to roff, giving this:
>
>            ·   convert removal lines to context lines (don’t stage removal)
>                Similarly, your patch will likely not apply if you:
>
>            ·   add context or removal lines
>
> Am I missing something, or is it really a docbook bug? Does it render
> better for anybody else?

Not for me, and I have seen manpages lacking a blank line like the above
where I expect one in other places, so it is not very surprising.

> -- >8 --
> Subject: [PATCH] docs: give more hints about how "add -e" works
>
> The original text was not very descriptive about what you
> can and can't do; let's try to enumerate all cases.
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
>  Documentation/git-add.txt |   22 +++++++++++++++++++---
>  1 files changed, 19 insertions(+), 3 deletions(-)
>
> diff --git a/Documentation/git-add.txt b/Documentation/git-add.txt
> index 45ebf87..b0a8420 100644
> --- a/Documentation/git-add.txt
> +++ b/Documentation/git-add.txt
> @@ -86,9 +86,25 @@ OPTIONS
>  	edit it.  After the editor was closed, adjust the hunk headers
>  	and apply the patch to the index.
>  +
> -*NOTE*: Obviously, if you change anything else than the first character
> -on lines beginning with a space or a minus, the patch will no longer
> -apply.
> +The intent of this option is to pick and choose lines of the diff to

I'd slightly prefer "patch" over "diff" in this context.

> +apply, or even to modify the contents of lines to be staged. There are
> +three line types in a diff: addition lines (beginning with a plus),
> +removal lines (beginning with a minus), and context lines (beginning
> +with a space). In general, it should be safe to:
> ++
> +--
> +* remove addition lines (don't stage the line)

This is more like "don't add the line", isn't it?  Also if this "+" line
has corresponding "-" line (i.e. it is a "rewrite to this" line), removal
of such a line would mean "instead of rewriting, remove it".

> +* modify the content of any addition lines (stage modified contents)

Would be "add differently".

> +* add new addition lines (stage the new line)

"Add more than what you have in the work tree"

While kibitzing like the above, I noticed that there is another thing that
may deserve warning even more.  Suppose you added lines and the patch
between HEAD and the work tree looks like this:

    diff --git a/t-f b/t-f
    index e69de29..d68dd40 100644
    --- a/t-f
    +++ b/t-f
    @@ -0,0 +1,4 @@
    +a
    +b
    +c
    +d

And you do "add -e" and edit the patch to:

    diff --git a/t-f b/t-f
    index e69de29..17c3f0b 100644
    --- a/t-f
    +++ b/t-f
    @@ -0,0 +1,3 @@
    +a
    +e
    +d

Obviously, the above patch is what "diff --cached" would show after such
an "add -e", but this does _not_ touch anything in the work tree (which is
correct; add is about moving changed contents to the index and it should
fundamentally not affect work tree).  As a result, "diff" between the
index and the work tree now would read like this:

    diff --git a/t-f b/t-f
    index 17c3f0b..d68dd40 100644
    --- a/t-f
    +++ b/t-f
    @@ -1,3 +1,4 @@
     a
    -e
    +b
    +c
     d

IOW, your request to "add -e" was "I do not want to put the addition of
'c' in the index at this point (that is why you removed '+c')" and "I do
not want to put the addition of 'b' in the index; I want 'e' instead (that
is why you changed '+b' to '+e')".  After "add -e" granted both requests,
what is left in the work tree can confuse the user.  The "not at this
point" part of the first request is clearly visible that the leftover diff
has an addition of '+c'.  But the user may expect that the second request,
"I don't want 'b', I want 'e'", would carry over to the work tree and not
see the apparent reversion of the wish (i.e. you changed it to add 'e'
instead of 'b' in "add -e" session, but it is reverted and "commit -a"
would record the version with 'b' instead).

Re: [PATCH] git add -e documentation: rephrase note

[Jeff King]

On Sun, Oct 18, 2009 at 10:38:46PM -0700, Junio C Hamano wrote:

> > -*NOTE*: Obviously, if you change anything else than the first character
> > -on lines beginning with a space or a minus, the patch will no longer
> > -apply.
> > +The intent of this option is to pick and choose lines of the diff to
> 
> I'd slightly prefer "patch" over "diff" in this context.

Sure, I have no problem with that.

> > +apply, or even to modify the contents of lines to be staged. There are
> > +three line types in a diff: addition lines (beginning with a plus),
> > +removal lines (beginning with a minus), and context lines (beginning
> > +with a space). In general, it should be safe to:
> > ++
> > +--
> > +* remove addition lines (don't stage the line)
> 
> This is more like "don't add the line", isn't it?  Also if this "+" line
> has corresponding "-" line (i.e. it is a "rewrite to this" line), removal
> of such a line would mean "instead of rewriting, remove it".

I was trying not to use "add" because we are already talking about
addition and removal in the patch itself, and I thought it made sense to
map those actions to what the user is conceptually doing (staging or not
staging). Of course, I used "remove" which has a similar problem (you
are removing the addition of the line to the index); probably "delete"
would have been better. So I am not sure I agree that using "add" is any
better than "stage", but I don't feel that strongly about it.

But beyond that, yes, you are right that removing a "+" line may have a
different conceptual meaning to the user depending on the surrounding
text. I wonder if such a "check-list" document really makes much sense,
given that using "-e" at all means you need to understand the patch
format and what makes sense (i.e., anybody who understands 'patch' knows
that you can't just delete context lines and expect it to apply).

> Obviously, the above patch is what "diff --cached" would show after such
> an "add -e", but this does _not_ touch anything in the work tree (which is
> correct; add is about moving changed contents to the index and it should
> fundamentally not affect work tree).  As a result, "diff" between the
> index and the work tree now would read like this:
> 
>     diff --git a/t-f b/t-f
>     index 17c3f0b..d68dd40 100644
>     --- a/t-f
>     +++ b/t-f
>     @@ -1,3 +1,4 @@
>      a
>     -e
>     +b
>     +c
>      d
> 
> IOW, your request to "add -e" was "I do not want to put the addition of
> 'c' in the index at this point (that is why you removed '+c')" and "I do
> not want to put the addition of 'b' in the index; I want 'e' instead (that
> is why you changed '+b' to '+e')".  After "add -e" granted both requests,
> what is left in the work tree can confuse the user.  The "not at this

Yeah, again, this comes down to understanding what you are doing:
applying a patch to the index. So it really requires that the user
understand what that means. Maybe instead of trying to enumerate cases
and their effects, we should just say sometihng like "this is a patch to
the index. While you can make arbitrary changes, keep in mind that (1)
your changes must apply to the content in the index, and (2) any new
changes you introduce in the patch will not be reflected in the working
tree".

-Peff

Re: [PATCH] git add -e documentation: rephrase note

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

>> > +* remove addition lines (don't stage the line)
>> 
>> This is more like "don't add the line", isn't it?  Also if this "+" line
>> has corresponding "-" line (i.e. it is a "rewrite to this" line), removal
>> of such a line would mean "instead of rewriting, remove it".
>
> I was trying not to use "add" because we are already talking about
> addition and removal in the patch itself,

Ah, I wasn't saying "add" is more kosher than "stage" by the above.

By "don't add", I meant that the user is saying "I inserted a new line to
the file, but I actually did not want to add that line to the file for the
next commit."  In other words, I was more focusing on the act of inserting
the line to the contents, not on staging the change to the index.

> ... So I am not sure I agree that using "add" is any
> better than "stage", but I don't feel that strongly about it.

That is Ok; the comment was not about stage vs add.

> But beyond that, yes, you are right that removing a "+" line may have a
> different conceptual meaning to the user depending on the surrounding
> text. I wonder if such a "check-list" document really makes much sense,
> given that using "-e" at all means you need to understand the patch
> format and what makes sense (i.e., anybody who understands 'patch' knows
> that you can't just delete context lines and expect it to apply).

Yeah, that is really what I wanted people who are in this discussion to
eventually realize ;-)

> Yeah, again, this comes down to understanding what you are doing:

Re: [PATCH] git add -e documentation: rephrase note

[Wesley J. Landaker]

On Monday 19 October 2009 01:07:23 Junio C Hamano wrote:
> That is Ok; the comment was not about stage vs add.
>
> > But beyond that, yes, you are right that removing a "+" line may have a
> > different conceptual meaning to the user depending on the surrounding
> > text. I wonder if such a "check-list" document really makes much sense,
> > given that using "-e" at all means you need to understand the patch
> > format and what makes sense (i.e., anybody who understands 'patch'
> > knows that you can't just delete context lines and expect it to apply).
>
> Yeah, that is really what I wanted people who are in this discussion to
> eventually realize ;-)

Comment from the peanut gallery:

I still think a quick summary checklist is useful even for a seasoned 
developer that is intimate with the 'patch' format, as it lets users know 
what git will do with your patch modifications.

For example, when I first tried "add -e", my first thought was: "Awesome, 
but, I wonder if git will do the right thing if I modify the patch in THIS 
way ...". Fortunately, git did the right thing, but I wasn't really sure 
until I tried it.

Re: [PATCH] git add -e documentation: rephrase note

[Junio C Hamano]

"Wesley J. Landaker" <wjl@icecavern.net> writes:

> On Monday 19 October 2009 01:07:23 Junio C Hamano wrote:
>> That is Ok; the comment was not about stage vs add.
>>
>> > But beyond that, yes, you are right that removing a "+" line may have a
>> > different conceptual meaning to the user depending on the surrounding
>> > text. I wonder if such a "check-list" document really makes much sense,
>> > given that using "-e" at all means you need to understand the patch
>> > format and what makes sense (i.e., anybody who understands 'patch'
>> > knows that you can't just delete context lines and expect it to apply).
>>
>> Yeah, that is really what I wanted people who are in this discussion to
>> eventually realize ;-)
>
> Comment from the peanut gallery:
>
> I still think a quick summary checklist is useful even for a seasoned 
> developer that is intimate with the 'patch' format, as it lets users know 
> what git will do with your patch modifications.

Oh, that is not something I am against at all.  I (mis)took Peff's "what
makes sense" to cover more than what a patch format is, namely, "what the
user by modifying the patch and 'add -e' by accepting a modified patch are
doing."

> For example, when I first tried "add -e", my first thought was: "Awesome, 
> but, I wonder if git will do the right thing if I modify the patch in THIS 
> way ...". Fortunately, git did the right thing, but I wasn't really sure 
> until I tried it.

Actually, my "change +b to +e" example was meant to illustrate that git
is not necessarily doing the right thing.

Admittedly, there is no way to always do the right thing unless git can
read user's mind in this case, and realization of why it is impossible to
do is necessary before using "add -e"; otherwise you would end up getting
utterly confused.  I do not use "add -e" myself.

In one edit (i.e. "remove +c"), the intention is clear that the insertion
is merely kept out of the index and it might even eventually be added in a
later commit.  "c" could be a debugging "printf()" that the user may not
ever want to commit.  In either case, it is clear that the user wants to
keep that change in the work tree version.

In another edit (i.e. "change +b to +e"), the only thing that is clear is
that the user wants a version with "e" in the next commit.

Changing from "+b" to "+e" in "add -e" might have been done, in order to
change "+if (1 || debug)" in the work tree version to "+if (debug)" in the
version to be committed (so that the user can keep debugging without
giving the --debug option to the program, for example).  In this case, it
is quite similar to the earlier "remove +c" ("I know I inserted 'c' in my
work tree version, do not commit that change, but I do want to keep it in
my work tree") case, and keeping the work tree intact is the right thing
to do.

But it might have been done because the user spotted a typo in "b" and
wanted to correct it to "e", in which case the user may wish the change
be reflected to the work tree.

As there is no way to distinguish the last two cases (and I do not think
the code even attempts to tell the difference); half of the time you would
(perhaps mistakenly) think that "add -e" did a wrong thing to your typofix
edit, after adding the updated contents correctly to the index: "I told
git to fix the typo 'b' to 'e', and I committed the corrected version, but
then it discarded my fix---the typo is still there in my work tree".