Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Two fixes for the built-in git add -i #528

Closed
wants to merge 2 commits into from
Closed

Two fixes for the built-in git add -i #528

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
353c748
built-in add -i: do not try to `patch`/`diff` an empty list of files
dscho Jan 14, 2020
7e4bf4b
built-in add -i: accept open-ended ranges again
dscho Jan 14, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions add-interactive.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -915,7 +915,7 @@ static int run_patch(struct add_i_state *s, const struct pathspec *ps,

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Junio C Hamano wrote (reply to this):

"Johannes Schindelin via GitGitGadget" <[email protected]>
writes:

> From: Johannes Schindelin <[email protected]>
>
> When the user does not select any files to `patch` or `diff`, there is
> no need to call `run_add_p()` on them.
>
> Even worse: we _have_ to avoid calling `parse_pathspec()` with an empty
> list because that would trigger this error:
>
> 	BUG: pathspec.c:557: PATHSPEC_PREFER_CWD requires arguments
>
> So let's avoid doing any work on a list of files that is empty anyway.
>
> This fixes https://github.com/git-for-windows/git/issues/2466.
>
> Signed-off-by: Johannes Schindelin <[email protected]>
> ---
>  add-interactive.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)

Makes sense.  No tests?

>
> diff --git a/add-interactive.c b/add-interactive.c
> index f395d54c08..14d4688c26 100644
> --- a/add-interactive.c
> +++ b/add-interactive.c
> @@ -915,7 +915,7 @@ static int run_patch(struct add_i_state *s, const struct pathspec *ps,
>  
>  	opts->prompt = N_("Patch update");
>  	count = list_and_choose(s, files, opts);
> -	if (count >= 0) {
> +	if (count > 0) {
>  		struct argv_array args = ARGV_ARRAY_INIT;
>  
>  		argv_array_pushl(&args, "git", "add--interactive", "--patch",
> @@ -953,7 +953,7 @@ static int run_diff(struct add_i_state *s, const struct pathspec *ps,
>  	opts->flags = IMMEDIATE;
>  	count = list_and_choose(s, files, opts);
>  	opts->flags = 0;
> -	if (count >= 0) {
> +	if (count > 0) {
>  		struct argv_array args = ARGV_ARRAY_INIT;
>  
>  		argv_array_pushl(&args, "git", "diff", "-p", "--cached",

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Johannes Schindelin wrote (reply to this):

Hi Junio,

On Thu, 16 Jan 2020, Junio C Hamano wrote:

> "Johannes Schindelin via GitGitGadget" <[email protected]>
> writes:
>
> > From: Johannes Schindelin <[email protected]>
> >
> > When the user does not select any files to `patch` or `diff`, there is
> > no need to call `run_add_p()` on them.
> >
> > Even worse: we _have_ to avoid calling `parse_pathspec()` with an empty
> > list because that would trigger this error:
> >
> > 	BUG: pathspec.c:557: PATHSPEC_PREFER_CWD requires arguments
> >
> > So let's avoid doing any work on a list of files that is empty anyway.
> >
> > This fixes https://github.com/git-for-windows/git/issues/2466.
> >
> > Signed-off-by: Johannes Schindelin <[email protected]>
> > ---
> >  add-interactive.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
>
> Makes sense.  No tests?

Do we really need tests here? ;-)

Honestly, I would deem the likelihood of this to get broken again very
low, and the effort for writing a regression test relatively high...

If you insist, I will add some, of course, but not today.

Ciao,
Dscho

>
> >
> > diff --git a/add-interactive.c b/add-interactive.c
> > index f395d54c08..14d4688c26 100644
> > --- a/add-interactive.c
> > +++ b/add-interactive.c
> > @@ -915,7 +915,7 @@ static int run_patch(struct add_i_state *s, const struct pathspec *ps,
> >
> >  	opts->prompt = N_("Patch update");
> >  	count = list_and_choose(s, files, opts);
> > -	if (count >= 0) {
> > +	if (count > 0) {
> >  		struct argv_array args = ARGV_ARRAY_INIT;
> >
> >  		argv_array_pushl(&args, "git", "add--interactive", "--patch",
> > @@ -953,7 +953,7 @@ static int run_diff(struct add_i_state *s, const struct pathspec *ps,
> >  	opts->flags = IMMEDIATE;
> >  	count = list_and_choose(s, files, opts);
> >  	opts->flags = 0;
> > -	if (count >= 0) {
> > +	if (count > 0) {
> >  		struct argv_array args = ARGV_ARRAY_INIT;
> >
> >  		argv_array_pushl(&args, "git", "diff", "-p", "--cached",
>

opts->prompt = N_("Patch update");
count = list_and_choose(s, files, opts);
if (count >= 0) {
if (count > 0) {
struct argv_array args = ARGV_ARRAY_INIT;

argv_array_pushl(&args, "git", "add--interactive", "--patch",
Expand Down Expand Up @@ -953,7 +953,7 @@ static int run_diff(struct add_i_state *s, const struct pathspec *ps,
opts->flags = IMMEDIATE;
count = list_and_choose(s, files, opts);
opts->flags = 0;
if (count >= 0) {
if (count > 0) {
struct argv_array args = ARGV_ARRAY_INIT;

argv_array_pushl(&args, "git", "diff", "-p", "--cached",
Expand Down