Alternative to manual editing with git add --patch

Alternative to manual editing with git add --patch

[Sven Helmberger]

Hello,

I hope this hasn't been discussed before.

I'm a big fan of cleanliness in commits and therefore often use git add
--patch to sort code changes I made into the right commits etc.

What I then often encountered was the situation where I happened to have
inserted consecutive lines of code that conceptually belong to different
commits. Normally I can nicely split patches, but not in this case,
making manually editing the patch the only alternative.

Shouldn't there be at least a way to quickly say line-by-line if you
want to have it added or not?

Personally, I find manually editing just annoying, it seems overly
arcane, but it also prevents me from really recommending "add --patch"
as best practice. I think it's a really good idea for many reasons to do
so, but I can't really tell people already struggling with using git
that I expect them to edit patches manually.

Regards,
Sven Helmberger

Re: Alternative to manual editing with git add --patch

[Matthieu Moy]

Sven Helmberger <sven.helmberger@gmx.de> writes:

> Hello,
>
> I hope this hasn't been discussed before.
>
> I'm a big fan of cleanliness in commits and therefore often use git add
> --patch to sort code changes I made into the right commits etc.
>
> What I then often encountered was the situation where I happened to have
> inserted consecutive lines of code that conceptually belong to different
> commits. Normally I can nicely split patches, but not in this case,
> making manually editing the patch the only alternative.
>
> Shouldn't there be at least a way to quickly say line-by-line if you
> want to have it added or not?

Many GUI or text-editor plugins for Git allow you to just select lines
and stage the selection. Even though I love "git add -p", I find magit
(Git integration for Emacs) more convenient when it comes to staging
individual lines.

That said, a "split hunk line by line" option for "git add -p" could be
nice.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

Re: Alternative to manual editing with git add --patch

[Johannes Schindelin]

Hi Sven,

On 2015-10-14 17:07, Sven Helmberger wrote:

> What I then often encountered was the situation where I happened to have
> inserted consecutive lines of code that conceptually belong to different
> commits. Normally I can nicely split patches, but not in this case,
> making manually editing the patch the only alternative.

How about implementing it and then discussing the implementation? That would have two advantages:

1) there would be something tangential to discuss, and
2) even if Junio rejects it, you can carry the patch in your private fork and use it forever.

Ciao,
Johannes

Re: Alternative to manual editing with git add --patch

[Junio C Hamano]

Sven Helmberger <sven.helmberger@gmx.de> writes:

> I hope this hasn't been discussed before.
>
> I'm a big fan of cleanliness in commits and therefore often use git add
> --patch to sort code changes I made into the right commits etc.
>
> What I then often encountered was the situation where I happened to have
> inserted consecutive lines of code that conceptually belong to different
> commits. Normally I can nicely split patches, but not in this case,
> making manually editing the patch the only alternative.
>
> Shouldn't there be at least a way to quickly say line-by-line if you
> want to have it added or not?

I think this has been discussed a few times (and you should actually
hope that is the case---it shows that something that allows you to
split a hunk that consists of consecutive lines is not an obscure
and useless feature wish).  But I do not think we saw anybody came
up with a convincingly usable design (not the code design but how
the user interaction specifies where the hunk is cut in the first
place), let alone a prototype for it, so that we can discuss
further.  At least not yet.

As a quick-and-dirty change, you could invent a new variant of
's'plit that breaks a N-line hunk into N hunks with 1-line each, but
obviously that would not be a pleasant-enough UI to be called usable
when you have a hunk that adds 100 lines.  Perhaps "Split this hunk
into two by ending the earlier one immediately before the line that
has this substring" or something might be an idea?

Re: Alternative to manual editing with git add --patch

[Sven Helmberger]

Am 14.10.2015 um 19:50 schrieb Junio C Hamano:
> Sven Helmberger <sven.helmberger@gmx.de> writes:
> 
> As a quick-and-dirty change, you could invent a new variant of
> 's'plit that breaks a N-line hunk into N hunks with 1-line each, but
> obviously that would not be a pleasant-enough UI to be called usable
> when you have a hunk that adds 100 lines.  Perhaps "Split this hunk
> into two by ending the earlier one immediately before the line that
> has this substring" or something might be an idea?
> 

If we go by the style of interaction in git add --patch and git add
--interactive, I think the most canonical solution would be to implement
it like this.

If we know when we can't split the current patch any further ( the point
at which selecting s changes nothing anymore), why shouldn't add --patch
not work similar to add --interactive in that it prints the lines of the
diff prefixed with numbers and the user can define a numerical range to
"split off". Then they decide whether to add those lines or not and
return to the line-numbers until they're trough with the patch.

Regards,
Sven Helmberger

Re: Alternative to manual editing with git add --patch

[Johannes Schindelin]

Hi Sven & Junio,

On Thu, 15 Oct 2015, Sven Helmberger wrote:

> Am 14.10.2015 um 19:50 schrieb Junio C Hamano:
> > Sven Helmberger <sven.helmberger@gmx.de> writes:
> > 
> > As a quick-and-dirty change, you could invent a new variant of
> > 's'plit that breaks a N-line hunk into N hunks with 1-line each, but
> > obviously that would not be a pleasant-enough UI to be called usable
> > when you have a hunk that adds 100 lines.  Perhaps "Split this hunk
> > into two by ending the earlier one immediately before the line that
> > has this substring" or something might be an idea?
> > 
> 
> If we go by the style of interaction in git add --patch and git add
> --interactive, I think the most canonical solution would be to implement
> it like this.
> 
> If we know when we can't split the current patch any further ( the point
> at which selecting s changes nothing anymore), why shouldn't add --patch
> not work similar to add --interactive in that it prints the lines of the
> diff prefixed with numbers and the user can define a numerical range to
> "split off". Then they decide whether to add those lines or not and
> return to the line-numbers until they're trough with the patch.

This is all technically sound. From a usability perspective I would wish
more for a way to exclude or filter the lines by a pattern. Take for
example a diff like this (which is *quite* normal in my workflow):

-- snip --
diff --git a/80a8c9a..001a983 b/001a983
index 80a8c9a..001a983 100644
--- a/80a8c9a..001a983
+++ b/001a983
@@ -23,11 +23,15 @@ void *git_mmap(void *start, size_t length, int prot, int flags, int fd, off_t of

        hmap = CreateFileMapping((HANDLE)_get_osfhandle(fd), NULL,
                PAGE_WRITECOPY, 0, 0, NULL);
+error("hmap: %p", hmap);

-       if (!hmap)
+       if (!hmap) {
+               errno = EINVAL;
                return MAP_FAILED;
+       }

        temp = MapViewOfFileEx(hmap, FILE_MAP_COPY, h, l, length, start);
+error("temp: %p 0x%x", temp, (unsigned int)GetLastError());

        if (!CloseHandle(hmap))
                warning("unable to close file mapping handle");
-- snap --

(Yes, I am lazy and reuse the `error()` function for debug log messages.)

It is quite obvious what I would like to do in this case: keep the change
that sets the errno.

I would be really thankful if I had a shortcut in `git add -p` that let me
specify, say, "Xerror" to eXclude any + line (and replace the '-' by a ' '
in every - line) that contains the substring 'error'.

The way I envisage this command to work would be to present the filtered
diff in the next step:

-- snip --
@@ -23,11 +23,13 @@ void *git_mmap(void *start, size_t length, int prot, int flags, int fd, off_t of

        hmap = CreateFileMapping((HANDLE)_get_osfhandle(fd), NULL,
                PAGE_WRITECOPY, 0, 0, NULL);

-       if (!hmap)
+       if (!hmap) {
+               errno = EINVAL;
                return MAP_FAILED;
+       }

        temp = MapViewOfFileEx(hmap, FILE_MAP_COPY, h, l, length, start);

        if (!CloseHandle(hmap))
                warning("unable to close file mapping handle");
-- snap --

Likewise, I could imagine that something like "Ierrno" to keep only +
lines with matching substrings (and treating - lines correspondingly).

Having said that, I find myself occasionally powering up a full-fledged
`git gui` just to stage individual lines. If I had an 'L' command in `git
add -p` instead that would present the following hunks, I would be really
happy:

-- snip --
@@ -23,6 +23,7 @@ void *git_mmap(void *start, size_t length, int prot, int flags, int fd, off_t of

        hmap = CreateFileMapping((HANDLE)_get_osfhandle(fd), NULL,
                PAGE_WRITECOPY, 0, 0, NULL);
+error("hmap: %p", hmap);

        if (!hmap)
                return MAP_FAILED;
-- snap --

and then

-- snip --
@@ -24,6 +24,5 @@ void *git_mmap(void *start, size_t length, int prot, int flags, int fd, off_t of
        hmap = CreateFileMapping((HANDLE)_get_osfhandle(fd), NULL,
                PAGE_WRITECOPY, 0, 0, NULL);

-       if (!hmap)
                return MAP_FAILED;

        temp = MapViewOfFileEx(hmap, FILE_MAP_COPY, h, l, length, start);
-- snap --

and then

-- snip --
@@ -24,6 +24,7 @@ void *git_mmap(void *start, size_t length, int prot, int flags, int fd, off_t of
        hmap = CreateFileMapping((HANDLE)_get_osfhandle(fd), NULL,
                PAGE_WRITECOPY, 0, 0, NULL);

+       if (!hmap) {
                return MAP_FAILED;

        temp = MapViewOfFileEx(hmap, FILE_MAP_COPY, h, l, length, start);
-- snap --

and so on.

What do you think?

Ciao,
Dscho

Re: Alternative to manual editing with git add --patch

[Sven Helmberger]

Am 15.10.2015 um 12:11 schrieb Johannes Schindelin:
> 
> This is all technically sound. From a usability perspective I would wish
> more for a way to exclude or filter the lines by a pattern. 

Why not do both?

The only thing that is unfortunate is that the "/" already is a command.

Which would point to either a solution along the lines of "s<range>"
being the split-by-line and "s/" being the split-by-regex?

Or is this an argument for introducing yet another interaction mode
entered when "s" fails to split further -- with simple "/" and "<range>"
being the options in that mode.

Regards,
Sven Helmberger

Re: Alternative to manual editing with git add --patch

[Johannes Schindelin]

Hi Sven,

On Thu, 15 Oct 2015, Sven Helmberger wrote:

> Am 15.10.2015 um 12:11 schrieb Johannes Schindelin:
> > 
> > This is all technically sound. From a usability perspective I would wish
> > more for a way to exclude or filter the lines by a pattern. 
> 
> Why not do both?

Because sometimes more is less. If users are overwhelmed with many, many
options, they are *less* likely to benefit from the few that are easy to
use because they won't find out about them.

Ciao,
Johannes

Re: Alternative to manual editing with git add --patch

[Sven Helmberger]

Am 15.10.2015 um 17:06 schrieb Johannes Schindelin:

> 
> Because sometimes more is less. If users are overwhelmed with many, many
> options, they are *less* likely to benefit from the few that are easy to
> use because they won't find out about them.
> 

Going from "I want to split at 'x'" to doing so seems just fine. More
than "Ok.. let's find the match I want to search for to have the split I
want", which seems like the opposite of usable.

There are 13 options now not counting help. Not sure it matters much if
we increase that to 14 or 15.

Regards,
Sven