Re: [RFD/PATCH] add: ignore only ignored files
Jeff King schrieb am 20.11.2014 um 19:20:
> On Thu, Nov 20, 2014 at 09:23:21AM -0800, Junio C Hamano wrote:
>
>>> diff --git a/builtin/add.c b/builtin/add.c
>>> index ae6d3e2..1074e32 100644
>>> --- a/builtin/add.c
>>> +++ b/builtin/add.c
>>> @@ -284,7 +284,7 @@ static int add_files(struct dir_struct *dir, int flags)
>>> for (i = 0; i < dir->ignored_nr; i++)
>>> fprintf(stderr, "%s\n", dir->ignored[i]->name);
>>> fprintf(stderr, _("Use -f if you really want to add them.\n"));
>>> - die(_("no files added"));
>>> + exit_status = 1;
>>> }
>>>
>>> for (i = 0; i < dir->nr; i++)
>>>
>>> It needs a tweak to t3700.35, which expects the "fatal:" line on stderr.
>>> But other than that, it passes all tests. So it must be good, right? :)
>>
>> ;-)
>>
>> It indeed is a behaviour change, but I do not expect it would be too
>> heavy a change to require us a transition period or major version
>> bump. But because that is just my expectation, which is not what
>> real world users would expect, so I'd prefer to cook such a change
>> for at least a cycle and a half in 'next'.
>
> Oh, definitely. Showing the patch at all was my not-so-subtle attempt to
> convince Michael to take over the topic so I did not have to worry about
> such things. :)
Well, given that I figured out how to do it with the option, it may come
at no additional surprise that I figured out how to do it without the
option, as well.
The real extra surprise is - not only that test fix is in my tree
already - it even comes with a test that actually tests the new intended
behavior. Whoaaa!
I'll resend it soon so that Jeff can take over and adjust the commit
message ;)
Michael, just kidding
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFD/PATCH] add: ignore only ignored files
On Thu, Nov 20, 2014 at 09:23:21AM -0800, Junio C Hamano wrote:
> > diff --git a/builtin/add.c b/builtin/add.c
> > index ae6d3e2..1074e32 100644
> > --- a/builtin/add.c
> > +++ b/builtin/add.c
> > @@ -284,7 +284,7 @@ static int add_files(struct dir_struct *dir, int flags)
> > for (i = 0; i < dir->ignored_nr; i++)
> > fprintf(stderr, "%s\n", dir->ignored[i]->name);
> > fprintf(stderr, _("Use -f if you really want to add them.\n"));
> > - die(_("no files added"));
> > + exit_status = 1;
> > }
> >
> > for (i = 0; i < dir->nr; i++)
> >
> > It needs a tweak to t3700.35, which expects the "fatal:" line on stderr.
> > But other than that, it passes all tests. So it must be good, right? :)
>
> ;-)
>
> It indeed is a behaviour change, but I do not expect it would be too
> heavy a change to require us a transition period or major version
> bump. But because that is just my expectation, which is not what
> real world users would expect, so I'd prefer to cook such a change
> for at least a cycle and a half in 'next'.
Oh, definitely. Showing the patch at all was my not-so-subtle attempt to
convince Michael to take over the topic so I did not have to worry about
such things. :)
-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFD/PATCH] add: ignore only ignored files
Jeff King writes:
> On Thu, Nov 20, 2014 at 10:42:16AM +0100, Michael J Gruber wrote:
>
>> >> Perhaps we could do a hybrid: add the files that were not ignored, but
>> >> then still exit non-zero. Careful scripts need to check the exit status
>> >> of "git add" anyway, and sloppy humans with over-broad wildcards
>> >> typically do not care about the exit status.
>> >
>> > ;-)
>> >
>>
>> You can simply say "Michael" in your last subclause above :)
>>
>> I'm wondering whether that behaviour change (without --ignore-errors) is
>> OK - I don't mind, but hey, I usually don't.
>
> I can't think of a case that it really hurts, but then I have not
> thought too hard on it. If you want to play with it, I think the patch
> is as simple as:
>
> diff --git a/builtin/add.c b/builtin/add.c
> index ae6d3e2..1074e32 100644
> --- a/builtin/add.c
> +++ b/builtin/add.c
> @@ -284,7 +284,7 @@ static int add_files(struct dir_struct *dir, int flags)
> for (i = 0; i < dir->ignored_nr; i++)
> fprintf(stderr, "%s\n", dir->ignored[i]->name);
> fprintf(stderr, _("Use -f if you really want to add them.\n"));
> - die(_("no files added"));
> + exit_status = 1;
> }
>
> for (i = 0; i < dir->nr; i++)
>
> It needs a tweak to t3700.35, which expects the "fatal:" line on stderr.
> But other than that, it passes all tests. So it must be good, right? :)
;-)
It indeed is a behaviour change, but I do not expect it would be too
heavy a change to require us a transition period or major version
bump. But because that is just my expectation, which is not what
real world users would expect, so I'd prefer to cook such a change
for at least a cycle and a half in 'next'.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFD/PATCH] add: ignore only ignored files
On Thu, Nov 20, 2014 at 10:42:16AM +0100, Michael J Gruber wrote:
> >> Perhaps we could do a hybrid: add the files that were not ignored, but
> >> then still exit non-zero. Careful scripts need to check the exit status
> >> of "git add" anyway, and sloppy humans with over-broad wildcards
> >> typically do not care about the exit status.
> >
> > ;-)
> >
>
> You can simply say "Michael" in your last subclause above :)
>
> I'm wondering whether that behaviour change (without --ignore-errors) is
> OK - I don't mind, but hey, I usually don't.
I can't think of a case that it really hurts, but then I have not
thought too hard on it. If you want to play with it, I think the patch
is as simple as:
diff --git a/builtin/add.c b/builtin/add.c
index ae6d3e2..1074e32 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -284,7 +284,7 @@ static int add_files(struct dir_struct *dir, int flags)
for (i = 0; i < dir->ignored_nr; i++)
fprintf(stderr, "%s\n", dir->ignored[i]->name);
fprintf(stderr, _("Use -f if you really want to add them.\n"));
- die(_("no files added"));
+ exit_status = 1;
}
for (i = 0; i < dir->nr; i++)
It needs a tweak to t3700.35, which expects the "fatal:" line on stderr.
But other than that, it passes all tests. So it must be good, right? :)
-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFD/PATCH] add: ignore only ignored files
Junio C Hamano schrieb am 19.11.2014 um 22:43: > Jeff King writes: > >> Typically I keep a very neat .gitignore file and just use "git add .", >> which _does_ ignore those files. The real problem here is that git >> cannot tell the difference between "the user explicitly asked for >> foo.aux, so we should complain" and "oops, foo.aux got caught in a shell >> expansion". > > Yup. I also find myself doing "git cmd -- \*.ext" to let Git, not > my shell, handle the patterns. That is the correct way, of course. >> I almost wonder if skipping ignored files should _always_ be a warning, >> not a hard error. I guess that has unpleasant side effects for scripts >> which call "git add XXX" and check the exit code, who may be >> unpleasantly surprised that they missed out on some content. >> >> Perhaps we could do a hybrid: add the files that were not ignored, but >> then still exit non-zero. Careful scripts need to check the exit status >> of "git add" anyway, and sloppy humans with over-broad wildcards >> typically do not care about the exit status. > > ;-) > You can simply say "Michael" in your last subclause above :) I'm wondering whether that behaviour change (without --ignore-errors) is OK - I don't mind, but hey, I usually don't. I think it all comes down to the fact whether specifying an ignored file on the command line is considered an error or only "possibly a user error" we should dwim around. "git add" being plumbing, I'm somehow hesitant to change behaviour unless git is asked to ignore error. And if I'm hesitant already... oh wait. git add is declared porcelain? I would not have guessed without looking it up. Michael -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFD/PATCH] add: ignore only ignored files
Jeff King writes: > Typically I keep a very neat .gitignore file and just use "git add .", > which _does_ ignore those files. The real problem here is that git > cannot tell the difference between "the user explicitly asked for > foo.aux, so we should complain" and "oops, foo.aux got caught in a shell > expansion". Yup. I also find myself doing "git cmd -- \*.ext" to let Git, not my shell, handle the patterns. > I almost wonder if skipping ignored files should _always_ be a warning, > not a hard error. I guess that has unpleasant side effects for scripts > which call "git add XXX" and check the exit code, who may be > unpleasantly surprised that they missed out on some content. > > Perhaps we could do a hybrid: add the files that were not ignored, but > then still exit non-zero. Careful scripts need to check the exit status > of "git add" anyway, and sloppy humans with over-broad wildcards > typically do not care about the exit status. ;-) -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFD/PATCH] add: ignore only ignored files
On Wed, Nov 19, 2014 at 03:52:33PM +0100, Michael J Gruber wrote: > "git add foo bar" adds neither foo nor bar when bar is ignored, but dies > to let the user recheck their command invocation. This becomes less > helpful when "git add foo.*" is subject to shell expansion and some of > the expanded files are ignored. > > "git add --ignore-errors" is supposed to ignore errors when indexing > some files and adds the others. It does ignore errors from actual > indexing attempts, but does not ignore the error "file is ignored" as > outlined above. > > Change "git add --ignore-errors foo bar" to add foo when bar is ignored, > i.e. to ignore the ignore error. > > Signed-off-by: Michael J Gruber FWIW, your complaint and this new behavior makes sense to me. Typically I keep a very neat .gitignore file and just use "git add .", which _does_ ignore those files. The real problem here is that git cannot tell the difference between "the user explicitly asked for foo.aux, so we should complain" and "oops, foo.aux got caught in a shell expansion". I almost wonder if skipping ignored files should _always_ be a warning, not a hard error. I guess that has unpleasant side effects for scripts which call "git add XXX" and check the exit code, who may be unpleasantly surprised that they missed out on some content. Perhaps we could do a hybrid: add the files that were not ignored, but then still exit non-zero. Careful scripts need to check the exit status of "git add" anyway, and sloppy humans with over-broad wildcards typically do not care about the exit status. -Peff -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFD/PATCH] add: ignore only ignored files
Michael J Gruber writes: > ... and most would > expect "git add --ignore-errors" to ignore the "file is ignored" error > as well and continue adding the remaining files. Yeah, I think that makes sense (but please don't take it as the final decision yet---I'd like to hear from others). -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
