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

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

[Jeff King]

The previous text was not exactly accurate; it is OK to
change space and minus lines, but only in certain ways. This
patch attempts to cover explicitly what can be done at the
individual line level, and cautions the user that
conceptually larger changes (like modifying a line) require
some understanding of the patch format.

Signed-off-by: Jeff King <peff@peff.net>
---
I'm sorting through some old topics in my personal repository and either
abandoning them or cleaning them up and submitting them. I thought this
one was worth doing. Original thread here:

  http://article.gmane.org/gmane.comp.version-control.git/130345

This version tries to address your points, Junio. It also seems that the
formatting issues have been magically fixed upstream in the past year. :)

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

diff --git a/Documentation/git-add.txt b/Documentation/git-add.txt
index 73378b2..40deb03 100644
--- a/Documentation/git-add.txt
+++ b/Documentation/git-add.txt
@@ -92,9 +92,35 @@ See ``Interactive mode'' for details.
 	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 patch to
+apply, or even to modify the contents of lines to be staged. There are
+three line types in a patch: 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
+--
++
+NOTE: In the first list above, the results given for each action are
+with respect to that patch line only. Conceptual changes like
+modification of a line in the original file are actually represented by
+removal of the old line followed by addition of the new line. Deleting
+only the addition line of this pair but leaving the removal line would
+therefore convert the modification into a deletion. In other words, use
+this feature with caution, as it is easy to stage unintended changes.
 
 -u::
 --update::
-- 
1.7.3.1.235.gdd6c0

Re: [PATCH] docs: give more hints about how "add -e" works

[Jonathan Nieder]

Jeff King wrote:

>                                                         This
> patch attempts to cover explicitly what can be done at the
> individual line level, and cautions the user that
> conceptually larger changes (like modifying a line) require
> some understanding of the patch format.

For what it's worth:
Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>

Re: [PATCH] docs: give more hints about how "add -e" works

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> -*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.

It is a good change to lose this "Obviously", which is often a sign of the
lazyness of the author who didn't even bother to explain certain things,
handwaving them away as "Obvious" ;-)

> +The intent of this option is to pick and choose lines of the patch to
> +apply, or even to modify the contents of lines to be staged. There are
> +three line types in a patch: 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)

I am not sure if the use of the word "stage" here is correct, even when
read from the "git stage" lovers' viewpoint.

If the "+" line is a pure addition without any corresponding line in the
preimage (which is removed by "-"), then this is "Don't add that line".
If it has a corresponding "-" line somewhere, that is rather "Remove
the corresponding line in the preimage".

"Don't add the updated line" might be a good compromise.

> +* 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)

Alternatively, to be consistent with "stage modification", "stage
removal", the removal of "+" can be phrased as "do not stage the addition
of the line".

I'd recommend to phrase them as "don't add", "add modified contents
instead", "add new line", "remove line" and "do not remove line", which
would be simpler to read, though.

> +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
> +--
> ++
> +NOTE: In the first list above, the results given for each action are
> +with respect to that patch line only. Conceptual changes like
> +modification of a line in the original file are actually represented by
> +removal of the old line followed by addition of the new line. Deleting
> +only the addition line of this pair but leaving the removal line would
> +therefore convert the modification into a deletion. In other words, use
> +this feature with caution, as it is easy to stage unintended changes.

Is there a way to move this note way upwards?  Once the reader understands
what this paragraph teaches, it becomes much easier to understand the
implication of "remove addition".

Re: [PATCH] docs: give more hints about how "add -e" works

[Jeff King]

On Thu, Oct 21, 2010 at 03:32:15PM -0700, Junio C Hamano wrote:

> > +The intent of this option is to pick and choose lines of the patch to
> > +apply, or even to modify the contents of lines to be staged. There are
> > +three line types in a patch: 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)
> 
> I am not sure if the use of the word "stage" here is correct, even when
> read from the "git stage" lovers' viewpoint.
> 
> If the "+" line is a pure addition without any corresponding line in the
> preimage (which is removed by "-"), then this is "Don't add that line".
> If it has a corresponding "-" line somewhere, that is rather "Remove
> the corresponding line in the preimage".
> 
> "Don't add the updated line" might be a good compromise.

I'm not sure that using the word "add" here is really any different than
"stage". Because we are really talking about adding _to_ the index,
which to me is the same as staging. IOW, I could say "Don't stage the
updated line" and it would mean the exact same thing.

It is a separate issue that adding/staging that line may not be a true
addition, but rather "don't add the updated line". That was what I was
trying to address in the final paragraph.

Perhaps the list should be structured not as "what you can do to each
line" but rather "here are some _concepts_ you might see, here's how
they are represented, and how you might want to edit them". So
basically:

  - added lines; represented by "+" lines. You can prevent staging any
    addition lines by deleting them.

  - removed lines; represented by "-" lines. You can prevent staging any
    removal lines by converting "-" to " ".

  - modified lines; represented by "-" followed by "+". You can prevent
    staging the modification by converting the "-" to a " ", and
    removing the "+" lines. And this would be a good place to warn that
    just deleting half of the pair is going to cause problems.

  - existing lines; represented by " " lines. You can:

      - remove them, by converting " " to "-".

      - modify them, by converting " " to "-", and adding a new "+" line
        with the new content.

  - adding new lines; do not yet exist in the patch. You can add new
    lines by inserting "+" lines with the new content.

which is perhaps better, as it directs the user according to what they
actually want to accomplish.

I shied away from that intially because there are some more complex
cases (e.g., added lines next to modified lines in the same hunk). But I
expect anybody with half a clue can probably figure out how to handle
that using the primitives outlined above.

> > +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
> > +--
> > ++
> > +NOTE: In the first list above, the results given for each action are
> > +with respect to that patch line only. Conceptual changes like
> > +modification of a line in the original file are actually represented by
> > +removal of the old line followed by addition of the new line. Deleting
> > +only the addition line of this pair but leaving the removal line would
> > +therefore convert the modification into a deletion. In other words, use
> > +this feature with caution, as it is easy to stage unintended changes.
> 
> Is there a way to move this note way upwards?  Once the reader understands
> what this paragraph teaches, it becomes much easier to understand the
> implication of "remove addition".

I agree it would be better at the top, but I think formatting it as I
just wrote above would mean we can actually explain the issue in a more
appropriate place. And then this bottom warning can just go away. I
think I may still put a few words of caution at the top.

-Peff

Re: [PATCH] docs: give more hints about how "add -e" works

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> Perhaps the list should be structured not as "what you can do to each
> line" but rather "here are some _concepts_ you might see, here's how
> they are represented, and how you might want to edit them". So
> basically:
>
>   - added lines; represented by "+" lines. You can prevent staging any
>     addition lines by deleting them.
>
>   - removed lines; represented by "-" lines. You can prevent staging any
>     removal lines by converting "-" to " ".
>
>   - modified lines; represented by "-" followed by "+". You can prevent
>     staging the modification by converting the "-" to a " ", and
>     removing the "+" lines. And this would be a good place to warn that
>     just deleting half of the pair is going to cause problems.
>
>   - existing lines; represented by " " lines. You can:
>
>       - remove them, by converting " " to "-".
>
>       - modify them, by converting " " to "-", and adding a new "+" line
>         with the new content.
>
>   - adding new lines; do not yet exist in the patch. You can add new
>     lines by inserting "+" lines with the new content.
>
> which is perhaps better, as it directs the user according to what they
> actually want to accomplish.

Yes, the above reads much better than starting from "when you see a '+'
you can do..." (which I think is a wrong approach that is backwards).

>> Is there a way to move this note way upwards?  Once the reader understands
>> what this paragraph teaches, it becomes much easier to understand the
>> implication of "remove addition".
>
> I agree it would be better at the top, but I think formatting it as I
> just wrote above would mean we can actually explain the issue in a more
> appropriate place. And then this bottom warning can just go away.

Agreed, again.

Re: [PATCH] docs: give more hints about how "add -e" works

[Jeff King]

[long quote, since it has been a few weeks]

On Fri, Oct 22, 2010 at 02:54:31PM -0700, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > Perhaps the list should be structured not as "what you can do to each
> > line" but rather "here are some _concepts_ you might see, here's how
> > they are represented, and how you might want to edit them". So
> > basically:
> >
> >   - added lines; represented by "+" lines. You can prevent staging any
> >     addition lines by deleting them.
> >
> >   - removed lines; represented by "-" lines. You can prevent staging any
> >     removal lines by converting "-" to " ".
> >
> >   - modified lines; represented by "-" followed by "+". You can prevent
> >     staging the modification by converting the "-" to a " ", and
> >     removing the "+" lines. And this would be a good place to warn that
> >     just deleting half of the pair is going to cause problems.
> >
> >   - existing lines; represented by " " lines. You can:
> >
> >       - remove them, by converting " " to "-".
> >
> >       - modify them, by converting " " to "-", and adding a new "+" line
> >         with the new content.
> >
> >   - adding new lines; do not yet exist in the patch. You can add new
> >     lines by inserting "+" lines with the new content.
> >
> > which is perhaps better, as it directs the user according to what they
> > actually want to accomplish.
> 
> Yes, the above reads much better than starting from "when you see a '+'
> you can do..." (which I think is a wrong approach that is backwards).
> 
> >> Is there a way to move this note way upwards?  Once the reader understands
> >> what this paragraph teaches, it becomes much easier to understand the
> >> implication of "remove addition".
> >
> > I agree it would be better at the top, but I think formatting it as I
> > just wrote above would mean we can actually explain the issue in a more
> > appropriate place. And then this bottom warning can just go away.
> 
> Agreed, again.

I found a few minutes to look at this today. However, it looks like you
already merged my original to next. :) The patch below implements the
strategy I discussed above, and IMHO it ended up being much clearer.

It should replace what's in next. You can either start a new branch and
revert what's in next, or I can prepare it on top of what's in next if
you prefer.

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

The previous text was not exactly accurate; it is OK to
change space and minus lines, but only in certain ways.

This patch takes a whole new approach, which is to describe
the sorts of conceptual operations you might want to
perform. It also includes a healthy dose of warnings about
how things can go wrong.

Since the size of the text is getting quite long, it also
splits this out into an "editing patches" section. This
makes more sense with the current structure, anyway, which
already splits out the interactive mode description.

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

diff --git a/Documentation/git-add.txt b/Documentation/git-add.txt
index 73378b2..5048309 100644
--- a/Documentation/git-add.txt
+++ b/Documentation/git-add.txt
@@ -92,9 +92,11 @@ See ``Interactive mode'' for details.
 	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 patch to
+apply, or even to modify the contents of lines to be staged. This can be
+quicker and more flexible than using the interactive hunk selector.
+However, it is easy to confuse oneself and create a patch that does not
+apply to the index. See EDITING PATCHES below.
 
 -u::
 --update::
@@ -295,6 +297,61 @@ diff::
   This lets you review what will be committed (i.e. between
   HEAD and index).
 
+
+EDITING PATCHES
+---------------
+
+Invoking `git add -e` or selecting `e` from the interactive hunk
+selector will open a patch in your editor; after the editor exits, the
+result is applied to the index. You are free to make arbitrary changes
+to the patch, but note that only a small subset of possible changes will
+result in a patch that can be applied. If you want to abort the
+operation entirely (i.e., stage nothing new in the index), simply delete
+all lines of the patch. The list below describes common things you may
+see in a patch, and which editing operations make sense on them.
+
+added content::
+
+Added content is represented by lines beginning with "{plus}". You can
+prevent staging any addition lines by deleting them.
+
+removed content::
+
+Removed content is represented by lines beginning with "-". You can
+prevent staging their removal by converting the "-" to a " " (space).
+
+modified content::
+
+Modified content is represented by "-" lines (removing the old content)
+followed by "{plus}" lines (adding the replacement content). You can
+prevent staging the modification by converting "-" lines to " ", and
+removing "{plus}" lines. Beware that modifying only half of the pair is
+likely to introduce confusing changes to the index.
+
+untouched content::
+
+Untouched content exists in context lines, beginning with a " " (space).
+You can stage context lines for removal by converting the space to a
+"-".
+
+new content::
+
+You may also add new content that does not exist in the patch. Simply
+add new lines, each starting with "{plus}".
+
+You can also perform more complex operations, such as modifying the
+content to be staged by a "{plus}" line. However, note that this impacts
+_only_ the index; the working tree file will remain unchanged, and will
+appear to "undo" the content you have staged. Such operations should be
+performed only if you know what you are doing.
+
+There are also several operations which should be avoided entirely, as
+they will make the patch impossible to apply:
+
+* adding context (" ") or removal ("-") lines
+* deleting context or removal lines
+* modifying the contents of context or removal lines
+
 SEE ALSO
 --------
 linkgit:git-status[1]
-- 
1.7.3.2.218.g4ee9d

Re: [PATCH] docs: give more hints about how "add -e" works

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> It should replace what's in next. You can either start a new branch and
> revert what's in next, or I can prepare it on top of what's in next if
> you prefer.

I can just apply an interdiff if I wanted to, so that is no reason for
asking a resend.

The new text indeed looks much clearer, except for one part I am not
absolutely sure...

> +new content::
> +
> +You may also add new content that does not exist in the patch. Simply
> +add new lines, each starting with "{plus}".
> +
> +You can also perform more complex operations, such as modifying the
> +content to be staged by a "{plus}" line. However, note that this impacts
> +_only_ the index; the working tree file will remain unchanged, and will
> +appear to "undo" the content you have staged. Such operations should be
> +performed only if you know what you are doing.

This "However, note that" part should apply not only to newly introduced
{plus} lines but also to {plus} lines whose change were edited (lines from
"added content" and from post-image half of "modified content"), no?

I tried to be careless when reading the two paragraphs above, and managed
to get an incorrect impression that the caveat applies only to "more
complex operations", even though it actually applies not just the previous
"new content" but also "added/modified" content.

Thinking about it a bit more, it is worse than that.  Turning " " into "-"
has the same "getting reverted" issue, no?

Re: [PATCH] docs: give more hints about how "add -e" works

[Jeff King]

On Mon, Nov 08, 2010 at 04:57:34PM -0800, Junio C Hamano wrote:

> The new text indeed looks much clearer, except for one part I am not
> absolutely sure...
> 
> > +new content::
> > +
> > +You may also add new content that does not exist in the patch. Simply
> > +add new lines, each starting with "{plus}".
> > +
> > +You can also perform more complex operations, such as modifying the
> > +content to be staged by a "{plus}" line. However, note that this impacts
> > +_only_ the index; the working tree file will remain unchanged, and will
> > +appear to "undo" the content you have staged. Such operations should be
> > +performed only if you know what you are doing.
> 
> This "However, note that" part should apply not only to newly introduced
> {plus} lines but also to {plus} lines whose change were edited (lines from
> "added content" and from post-image half of "modified content"), no?

Right. The final paragraph you quoted is not part of the list, and it
looks better when rendered by asciidoc, as it's indented differently. So
I think some of the confusion is from the source formatting. But...

> I tried to be careless when reading the two paragraphs above, and managed
> to get an incorrect impression that the caveat applies only to "more
> complex operations", even though it actually applies not just the previous
> "new content" but also "added/modified" content.
> 
> Thinking about it a bit more, it is worse than that.  Turning " " into "-"
> has the same "getting reverted" issue, no?

Yeah, some of the operations described in the upper list are actually
"more complex" in the sense of looking like reverts. Basically any time
you are _introducing_ a change during the diff edit rather than simply
selecting or not-selecting changes that exist in the working tree, you
are going to get confusing results. So let me take one more stab at it,
and I think the correct breakdown is:

  1. stuff you might want to do: staging or not staging added, removed,
     and modified lines

  2. stuff you might want to do if you're insane: marking context lines
     for removal, adding new content, changing content on existing add
     lines

  3. stuff that you never want to do, because it makes the patch
     impossible to apply: deleting, adding, or changing context or
     removal lines

I'll try to do a patch later tonight.

-Peff

Re: [PATCH] docs: give more hints about how "add -e" works

[Jeff King]

On Mon, Nov 08, 2010 at 10:03:58PM -0500, Jeff King wrote:

> > This "However, note that" part should apply not only to newly introduced
> > {plus} lines but also to {plus} lines whose change were edited (lines from
> > "added content" and from post-image half of "modified content"), no?
> 
> Right. The final paragraph you quoted is not part of the list, and it
> looks better when rendered by asciidoc, as it's indented differently. So
> I think some of the confusion is from the source formatting. But...

In the version below I used asciidoc's "--" block separator. It makes no
difference to the formatted output, but hopefully makes the source a
little more obvious.

> Yeah, some of the operations described in the upper list are actually
> "more complex" in the sense of looking like reverts. Basically any time
> you are _introducing_ a change during the diff edit rather than simply
> selecting or not-selecting changes that exist in the working tree, you
> are going to get confusing results. So let me take one more stab at it,
> and I think the correct breakdown is:

Patch is below. Thanks for bearing with me through all the versions; it
turned out to be harder to explain than I thought. :) I think the back
and forth has led us to a much more natural organization of the
information.

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

The previous text was not exactly accurate; it is OK to
change space and minus lines, but only in certain ways.

This patch takes a whole new approach, which is to describe
the sorts of conceptual operations you might want to
perform. It also includes a healthy dose of warnings about
how things can go wrong.

Since the size of the text is getting quite long, it also
splits this out into an "editing patches" section. This
makes more sense with the current structure, anyway, which
already splits out the interactive mode description.

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

diff --git a/Documentation/git-add.txt b/Documentation/git-add.txt
index 73378b2..54aaaeb 100644
--- a/Documentation/git-add.txt
+++ b/Documentation/git-add.txt
@@ -92,9 +92,11 @@ See ``Interactive mode'' for details.
 	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 patch to
+apply, or even to modify the contents of lines to be staged. This can be
+quicker and more flexible than using the interactive hunk selector.
+However, it is easy to confuse oneself and create a patch that does not
+apply to the index. See EDITING PATCHES below.
 
 -u::
 --update::
@@ -295,6 +297,78 @@ diff::
   This lets you review what will be committed (i.e. between
   HEAD and index).
 
+
+EDITING PATCHES
+---------------
+
+Invoking `git add -e` or selecting `e` from the interactive hunk
+selector will open a patch in your editor; after the editor exits, the
+result is applied to the index. You are free to make arbitrary changes
+to the patch, but note that some changes may have confusing results, or
+even result in a patch that cannot be applied.  If you want to abort the
+operation entirely (i.e., stage nothing new in the index), simply delete
+all lines of the patch. The list below describes some common things you
+may see in a patch, and which editing operations make sense on them.
+
+--
+added content::
+
+Added content is represented by lines beginning with "{plus}". You can
+prevent staging any addition lines by deleting them.
+
+removed content::
+
+Removed content is represented by lines beginning with "-". You can
+prevent staging their removal by converting the "-" to a " " (space).
+
+modified content::
+
+Modified content is represented by "-" lines (removing the old content)
+followed by "{plus}" lines (adding the replacement content). You can
+prevent staging the modification by converting "-" lines to " ", and
+removing "{plus}" lines. Beware that modifying only half of the pair is
+likely to introduce confusing changes to the index.
+--
+
+There are also more complex operations that can be performed. But beware
+that because the patch is applied only to the index and not the working
+tree, the working tree will appear to "undo" the change in the index.
+For example, introducing a a new line into the index that is in neither
+the HEAD nor the working tree will stage the new line for commit, but
+the line will appear to be reverted in the working tree.
+
+Avoid using these constructs, or do so with extreme caution.
+
+--
+removing untouched content::
+
+Content which does not differ between the index and working tree may be
+shown on context lines, beginning with a " " (space).  You can stage
+context lines for removal by converting the space to a "-". The
+resulting working tree file will appear to re-add the content.
+
+modifying existing content::
+
+One can also modify context lines by staging them for removal (by
+converting " " to "-") and adding a "{plus}" line with the new content.
+Similarly, one can modify "{plus}" lines for existing additions or
+modifications. In all cases, the new modification will appear reverted
+in the working tree.
+
+new content::
+
+You may also add new content that does not exist in the patch; simply
+add new lines, each starting with "{plus}". The addition will appear
+reverted in the working tree.
+--
+
+There are also several operations which should be avoided entirely, as
+they will make the patch impossible to apply:
+
+* adding context (" ") or removal ("-") lines
+* deleting context or removal lines
+* modifying the contents of context or removal lines
+
 SEE ALSO
 --------
 linkgit:git-status[1]
-- 
1.7.3.2.357.g419a4

Re: [PATCH] docs: give more hints about how "add -e" works

[Miklos Vajna]

[-- Attachment #1: Type: text/plain, Size: 459 bytes --]

On Thu, Oct 21, 2010 at 10:30:35AM -0400, Jeff King <peff@peff.net> wrote:
> The previous text was not exactly accurate; it is OK to
> change space and minus lines, but only in certain ways. This
> patch attempts to cover explicitly what can be done at the
> individual line level, and cautions the user that
> conceptually larger changes (like modifying a line) require
> some understanding of the patch format.

Thanks for collecting those corner cases. :)

[-- Attachment #2: Type: application/pgp-signature, Size: 198 bytes --]