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.

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

Adding git-ignore command, tests, and documentation. #264

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

tstone2077
Copy link

@tstone2077 tstone2077 commented Jun 13, 2019

git-ignore will allow users to quickly add entries to the gitignore files in the repositories.

Many times, I'll have a config file or log file buried in a series of sub-directories and find it frustrating to edit the right git ignore with the right relative path to the file that i want to add. This script makes adding items to a gitignore file easier.

Instead of managing paths and relative paths such as
echo "path/to/the/file.txt" >../../../../../../.gitignore
git ignore path/to/the/file.txt
No matter what directory that is in, the correct relative path will be added to the gitignore.

This script will also give the ability to add directories, extention globs, and files to any gitignore in parent directories. Furthermore, it allows you to easily open the gitignore file in your favorite editor from anywhere using:
git ignore --edit

I have been using this script for years and it has made things much easier for me, so I figured I'd contribute.

@gitgitgadget
Copy link

gitgitgadget bot commented Jun 13, 2019

Welcome to GitGitGadget

Hi @tstone2077, and welcome to GitGitGadget, the GitHub App to send patch series to the Git mailing list from GitHub Pull Requests.

Please make sure that this Pull Request has a good description, as it will be used as cover letter.

Also, it is a good idea to review the commit messages one last time, as the Git project expects them in a quite specific form:

  • the lines should not exceed 76 columns,
  • the first line should be like a header and typically start with a prefix like "tests:" or "commit:", and
  • the commit messages' body should be describing the "why?" of the change.
  • Finally, the commit messages should end in a Signed-off-by: line matching the commits' author.

It is in general a good idea to await the automated test ("Checks") in this Pull Request before contributing the patches, e.g. to avoid trivial issues such as unportable code.

Contributing the patches

Before you can contribute the patches, your GitHub username needs to be added to the list of permitted users. Any already-permitted user can do that, by adding a PR comment of the form /allow <username>.

Once on the list of permitted usernames, you can contribute the patches to the Git mailing list by adding a PR comment /submit.

After you submit, GitGitGadget will respond with another comment that contains the link to the cover letter mail in the Git mailing list archive. Please make sure to monitor the discussion in that thread and to address comments and suggestions.

If you do not want to subscribe to the Git mailing list just to be able to respond to a mail, you can download the mbox ("raw") file corresponding to the mail you want to reply to from the Git mailing list. If you use GMail, you can upload that raw mbox file via:

curl -g --user "<EMailAddress>:<Password>" --url "imaps://imap.gmail.com/INBOX" -T /path/to/raw.txt

@dscho
Copy link
Member

dscho commented Jun 13, 2019

Please edit this desctription before submitting, as it will be sent as cover letter (and you don't really want to repeat the commit message, but instead give a bit more background/motivation for your work).

@dscho
Copy link
Member

dscho commented Jun 13, 2019

/allow tstone2077

@gitgitgadget
Copy link

gitgitgadget bot commented Jun 13, 2019

User tstone2077 is now allowed to use GitGitGadget.

@dscho
Copy link
Member

dscho commented Jun 13, 2019

Also note that the tests failed on Windows:

++ git ignore -p 2 --edit
/usr/bin/seq: missing operand
Try '/usr/bin/seq --help' for more information.

Maybe this tell s you immediately what's wrong?

@tstone2077 tstone2077 changed the title git-ignore.sh: Adding git-ignore command Adding git-ignore command, tests, and documentation. Jun 13, 2019
git-ignore.sh Outdated
@@ -77,8 +77,9 @@ get_git_ignore () {

if test $_parent_level -ne 0
then
for i in $(seq 1 $_parent_level)
Copy link
Member

Choose a reason for hiding this comment

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

If the problem is a missing operand, isn't it more likely that $_parent_level is set to empty?

Copy link
Author

Choose a reason for hiding this comment

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

interesting... i don't have a windows box to test on, so I was guessing that the seq binary worked differently on windows (which I've seen in other utilities before). I'm not sure how $_parent_level could be empty since it's defaulted on line 162. The only guess I have is on line 182,

_parent_level="${1#--parent-level=}"

evaluates to empty on windows... maybe... Tricky to debug without being able to reproduce it. I'll see what I can come up with.

Thanks!

Copy link
Member

@dscho dscho Jun 13, 2019

Choose a reason for hiding this comment

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

One simple trick I use all the time is to insert something like

test -z "$DDDEBUG" || set -x

into the shell script in question, and then run the command in the test suite prefixed with DDDEBUG=1, like so:

DDDEBUG=1 git ignore ...

Another trick I use when debugging things via Azure Pipelines (I don't have a macOS setup, for example) is to edit the azure-pipelines.yml definition to execute only the job that I am really interested in, and also substitute the make test in ci/run-build-and-test.sh with something like (cd t && sh t7070-*.sh -i -v -x), to accelerate the turnaround between force-push and test result.

Maybe that helps you, too?

Copy link
Author

Choose a reason for hiding this comment

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

Awesome tricks which I will be sure to employ. Do you think it's necessary at this point since the last CI succeeded? Also, are there any more suggestions on getting this submitted? I changed the description, but I don't know if that is good enough. I think the most descriptive message I could write about this change is in the original commit message.

Copy link
Member

Choose a reason for hiding this comment

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

Do you think it's necessary at this point since the last CI succeeded?

I fear that the value of _parent_level might still be bogus and that the most recent changes might still just paper over that issue...

are there any more suggestions on getting this submitted?

Yes. The commit 4462b7f should be squashed into the first commit: On the Git mailing list, adding a patch with a bug that is immediately fixed by the next patch is frowned upon. Better to not even introduce the bug in the first place.

But then, I think that seq might be the better choice, still, you just need to figure out why it gets an incorrect number of parameters.

Copy link
Author

@tstone2077 tstone2077 Jun 15, 2019

Choose a reason for hiding this comment

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

In the tests,
line 3659 shows _parent_level is getting set correctly to 2:

+ _parent_level=2

and line 3691 shows seq is being used correctly:

++ seq 1 2
/usr/bin/seq: missing operand
Try '/usr/bin/seq --help' for more information.

it's just that version of windows (or mingw) isn't supporting it correctly. I'm not up for troubleshooting why 'seq 1 2' doesn't work on that environment of windows. That's why I went with the while loop to replace it... it seems like a reasonable, less fragile, and more portable implementation than 'seq'.

Thoughts?

Copy link
Member

Choose a reason for hiding this comment

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

Hmm. Works here:

$ /usr/bin/seq 1 2
1
2

git-ignore.sh Outdated Show resolved Hide resolved
@tstone2077
Copy link
Author

shall i submit this?

The 'git ignore' command modifies a .gitignore file in your path
easily. By default, it adds lines to the .gitignore found in the
root of your repository. It can, however, add lines to a gitignore
anywhere inbetween the file(s) passed in and the root of the
repository. The lines added to the gitignore can be based on
filename, extension, directory, or recursive glob.

Also, you can easily open the gitignore file using your $EDITOR
with 'git ignore --edit'.

This can make things much easier when ignore files in subdirectories.
No longer will you have to run:
echo "path/to/the/file.txt" >../../../../../../.gitignore
instead:
git ignore file.txt

Signed-off-by: Thurston Stone <tstone2077@gmail.com>
@dscho
Copy link
Member

dscho commented Jul 1, 2019

shall i submit this?

I am still not sure that the seq problem has been properly root-caused, but if you are not worried about that as much as I am, I see no obstacle against submitting now.

@tstone2077
Copy link
Author

/submit

@gitgitgadget
Copy link

gitgitgadget bot commented Jul 17, 2019

Submitted as pull.264.git.gitgitgadget@gmail.com

@gitgitgadget
Copy link

gitgitgadget bot commented Jul 17, 2019

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

"Thurston via GitGitGadget" <gitgitgadget@gmail.com> writes:

> Instead of managing paths and relative paths such as echo
> "path/to/the/file.txt" >../../../../../../.gitignore git ignore
> path/to/the/file.txt No matter what directory that is in, the correct
> relative path will be added to the gitignore.

Hmph, do you mean you type something like this?

	$ cd path/to/the
	... work in that deep directory ...
	... realize that file.txt in that directory needs ignoring ...
	$ echo path/to/the/file.txt >../../../../../../.gitignore

Wouldn't this simpler to type and less error prone, as you do not
have to count ../?

	$ cd path/to/the
	... work in that deep directory ...
	... realize that file.txt in that directory needs ignoring ...
	$ echo file.txt >.gitignore

@gitgitgadget
Copy link

gitgitgadget bot commented Jul 18, 2019

On the Git mailing list, Jeff King wrote (reply to this):

On Wed, Jul 17, 2019 at 09:35:34AM -0700, Junio C Hamano wrote:

> "Thurston via GitGitGadget" <gitgitgadget@gmail.com> writes:
> 
> > Instead of managing paths and relative paths such as echo
> > "path/to/the/file.txt" >../../../../../../.gitignore git ignore
> > path/to/the/file.txt No matter what directory that is in, the correct
> > relative path will be added to the gitignore.
> 
> Hmph, do you mean you type something like this?
> 
> 	$ cd path/to/the
> 	... work in that deep directory ...
> 	... realize that file.txt in that directory needs ignoring ...
> 	$ echo path/to/the/file.txt >../../../../../../.gitignore
> 
> Wouldn't this simpler to type and less error prone, as you do not
> have to count ../?
> 
> 	$ cd path/to/the
> 	... work in that deep directory ...
> 	... realize that file.txt in that directory needs ignoring ...
> 	$ echo file.txt >.gitignore

It's also much more efficient, at least with our current implementation,
as we do not have to worry about matching this entry when we are dealing
with /some/other/path. See [1] for a pathological case.

I see "with our current implementation" because I could imagine a world
in which we have a more trie-like structure for non-wildcard patterns.
But I don't think anybody is working on such a thing.

-Peff

[1] https://public-inbox.org/git/20120329211136.GA1112@sigill.intra.peff.net/

@dscho
Copy link
Member

dscho commented Oct 1, 2019

@tstone2077 are you still interested in getting this patch into Git? If so, maybe you can engage in a discussion with Junio and Peff?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants