Re: [PATCH] git-add-interactive: edit current file in editor

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Jul 29, 2015, 19:05 UTC
Message-ID: <CAPig+cRRda=DVGAqpUjOsFk9Fqjqxi-apvWLuEW5xK1XCu5_+g@mail.gmail.com>
In-Reply-To: <1438040482-4599-1-git-send-email-siadat@gmail.com>

Aside from whether or not this change is desirable, see a few pointers below to improve the patch...

On Monday, July 27, 2015, Sina Siadat <siadat@gmail.com> wrote:

> Adds a new option 'o' to the 'add -p' command that lets you open and edit the
> current file.

Imperative mood: "Add a new option..."

Show 5 quoted lines

> The existing 'e' mode is used to manually edit the hunk.  The new 'o' option
> allows you to open and edit the file without having to quit the loop. The hunks
> are updated when the editing is done, and the user will be able to review the
> updated hunks.  Without this option you would have to quit the loop, edit the
> file, and execute 'add -p filename' again.

This descriptive material belongs in the commit message. Good.

> I would appreciate it if you could let me know what you think about this
> option. I will write more tests if there is any interest at all.

This, however, is commentary, which, while useful as part of the submission process, does not belong in the commit message. Therefore, it should be placed below the "---" line just before the diffstat.

> Thank you. :)

Missing sign-off. See Documentation/SubmittingPatches.

More below...

Show 11 quoted lines

> ---
> diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh
> index deae948..e5dd1c6 100755
> --- a/t/t3701-add-interactive.sh
> +++ b/t/t3701-add-interactive.sh
> @@ -98,6 +98,12 @@ test_expect_success 'dummy edit works' '
>         test_cmp expected diff
>  '
>
> +test_expect_success 'dummy open works' '
> +       (echo o; echo a) | git add -p &&

Some alternatives, which may or may not read better, but at least avoid a process creation or two:

    { echo o; echo a; } | git add -p &&

    printf "%s\n" o a | git add -p &&

    printf "o\na\n" | git add -p &&

Those are just suggestions; not necessarily a request for change.

> +       git diff > diff &&

Style: >diff

Show 8 quoted lines

> +       test_cmp expected diff
> +'
> +
>  test_expect_success 'setup patch' '
>  cat >patch <<EOF
>  @@ -1,1 +1,4 @@
> --
> 2.5.0.rc3.2.g6f9504c.dirty