Re: [PATCH] git.el: handle default excludesfile properly

[Ævar Arnfjörð Bjarmason]

On Wed, Mar 07 2018, Dorab Patel jotted:

> Thanks for updating us with the status of git.el.
>
> The last time I looked at magit, it was too heavyweight for my needs.
> I like the simplicity of git.el. But perhaps it's time for me to take
> another look at magit.

You can also check out the VC mode that ships with Emacs itself. I
prefer Magit, but the VC mode is more lightweight.

> On Tue, Mar 6, 2018 at 3:54 AM, Alexandre Julliard  
> wrote:
>> Junio C Hamano  writes:
>>
>>> Having said that, I am sorry to say that I am not sure if the copy
>>> we have is the one to be patched, so I would appreciate if Alexandre
>>> (cc'ed) can clarify the situation.  The last change done to our copy
>>> of the script is from 2012, and I do not know if Alexandre is still
>>> taking care of it here but the script is so perfect that there was
>>> no need to update it for the past 5 years and we haven't seen an
>>> update, or the caninical copy is now being maintained elsewhere and
>>> we only have a stale copy, or what.
>>
>> This is the canonical version, and I guess in theory I'm still taking
>> care of it. However, the need that git.el was originally addressing is
>> now fulfilled by better tools. As such, I feel that it has outlived its
>> usefulness, and I'm no longer actively developing it.
>>
>> I'd recommend that anybody still using it switch to Magit, which is
>> being actively maintained, and IMO superior to git.el in all respects.
>>
>> --
>> Alexandre Julliard
>> julli...@winehq.org

Re: [PATCH] git.el: handle default excludesfile properly

[Dorab Patel]

Thanks for updating us with the status of git.el.

The last time I looked at magit, it was too heavyweight for my needs.
I like the simplicity of git.el. But perhaps it's time for me to take
another look at magit.


On Tue, Mar 6, 2018 at 3:54 AM, Alexandre Julliard  wrote:
> Junio C Hamano  writes:
>
>> Having said that, I am sorry to say that I am not sure if the copy
>> we have is the one to be patched, so I would appreciate if Alexandre
>> (cc'ed) can clarify the situation.  The last change done to our copy
>> of the script is from 2012, and I do not know if Alexandre is still
>> taking care of it here but the script is so perfect that there was
>> no need to update it for the past 5 years and we haven't seen an
>> update, or the caninical copy is now being maintained elsewhere and
>> we only have a stale copy, or what.
>
> This is the canonical version, and I guess in theory I'm still taking
> care of it. However, the need that git.el was originally addressing is
> now fulfilled by better tools. As such, I feel that it has outlived its
> usefulness, and I'm no longer actively developing it.
>
> I'd recommend that anybody still using it switch to Magit, which is
> being actively maintained, and IMO superior to git.el in all respects.
>
> --
> Alexandre Julliard
> julli...@winehq.org

Re: [PATCH] git.el: handle default excludesfile properly

[Alexandre Julliard]

Junio C Hamano  writes:

> Having said that, I am sorry to say that I am not sure if the copy
> we have is the one to be patched, so I would appreciate if Alexandre
> (cc'ed) can clarify the situation.  The last change done to our copy
> of the script is from 2012, and I do not know if Alexandre is still
> taking care of it here but the script is so perfect that there was
> no need to update it for the past 5 years and we haven't seen an
> update, or the caninical copy is now being maintained elsewhere and
> we only have a stale copy, or what.

This is the canonical version, and I guess in theory I'm still taking
care of it. However, the need that git.el was originally addressing is
now fulfilled by better tools. As such, I feel that it has outlived its
usefulness, and I'm no longer actively developing it.

I'd recommend that anybody still using it switch to Magit, which is
being actively maintained, and IMO superior to git.el in all respects.

-- 
Alexandre Julliard
julli...@winehq.org

Re: [PATCH] git.el: handle default excludesfile properly

[Junio C Hamano]

Dorab Patel  writes:

> Looking deeper into how the function git-get-exclude-files is used, I
> see that it is only being called from git-run-ls-files-with-excludes.
> So, perhaps, a better (or additional) fix might be to add the
> parameter "--exclude-standard" in the call to git-run-ls-files from
> within git-run-ls-files-with-excludes. And remove the need for
> get-get-exclude-files altogether.

It is absolutely the right thing to depend on --exclude-standard, I
would think, so that we do not have to worry about details like XDG
paths and such.  Thanks for working that out between both of you.

Having said that, I am sorry to say that I am not sure if the copy
we have is the one to be patched, so I would appreciate if Alexandre
(cc'ed) can clarify the situation.  The last change done to our copy
of the script is from 2012, and I do not know if Alexandre is still
taking care of it here but the script is so perfect that there was
no need to update it for the past 5 years and we haven't seen an
update, or the caninical copy is now being maintained elsewhere and
we only have a stale copy, or what.

Thanks.

Re: [PATCH] git.el: handle default excludesfile properly

[Eric Sunshine]

On Sat, Mar 3, 2018 at 9:57 PM, Dorab Patel  wrote:
> Thanks for reviewing and locating the commits.
>
> OK, I'll re-roll and add the relevant commits. It may take some time.
>
> Should I just send the revised patch as a separate thread (with the
> relevant commits and history)?

That would work. You can use "git format-patch -v2 ..." to mark the
patch as "[PATCH v2]", and "git send-email
--in-reply-to=20180303034803.21589-1-dorabpa...@gmail.com ..." to tie
it back to this thread when you send it.

As a an aid to reviewers, it's a good idea to add commentary
explaining what changed since v1 and provide a link back to v1, like
this[1]. Place the commentary below the "---" line following your
sign-off.

(One more minor comment: etiquette on this list is to avoid top-posting [2].)

Thanks.

[1]: https://public-inbox.org/git/20180303034803.21589-1-dorabpa...@gmail.com/
[2]: https://lkml.org/lkml/2005/1/11/111

Re: [PATCH] git.el: handle default excludesfile properly

[Dorab Patel]

Thanks for reviewing and locating the commits.

OK, I'll re-roll and add the relevant commits. It may take some time.

Should I just send the revised patch as a separate thread (with the
relevant commits and history)?

On Sat, Mar 3, 2018 at 6:12 PM, Eric Sunshine  wrote:
> On Sat, Mar 3, 2018 at 8:36 PM, Dorab Patel  wrote:
>> Correct me if I'm wrong, but my understanding, from
>> https://git-scm.com/docs/gitignore, is that $HOME/.gitignore is used
>> only if it is specified as the value of core.excludesfile in
>> ~/.gitconfig. It is not used by default. If that is so, then the
>> proposed (and original) code works. The changes I am proposing handle
>> the default case, when core.excludesfile is not specified.
>
> You're right. I must have set core.excludesfile so long ago that I
> forgot about it and assumed $HOME/.gitignore was consulted by default.
>
>> Looking deeper into how the function git-get-exclude-files is used, I
>> see that it is only being called from git-run-ls-files-with-excludes.
>> So, perhaps, a better (or additional) fix might be to add the
>> parameter "--exclude-standard" in the call to git-run-ls-files from
>> within git-run-ls-files-with-excludes. And remove the need for
>> get-get-exclude-files altogether.  Presumably, "--exclude-standard"
>> handles the default case with/without XDG_CONFIG_HOME correctly. The
>> question I'd have then is: why didn't the original author use that
>> option? Either I'm missing something? Or the option was added later,
>> after the original code was written? Or something else?
>
> Using --exclude-standard rather than --exclude-from and retiring
> git-get-exclude-files() makes sense to me.
>
> As for why the original author didn't use --exclude-standard, project
> history tells us that. In particular, git-get-exclude-files() was
> implemented by 274e13e0e9 (git.el: Take into account the
> core.excludesfile config option., 2007-07-31), whereas
> --exclude-standard was introduced by 8e7b07c8a7 (git-ls-files: add
> --exclude-standard, 2007-11-15), three and a half months later.
>
> If you do re-roll to use --exclude-standard, then it would be good for
> your commit message to explain this history, citing the relevant
> commits.
>
> Thanks.

Re: [PATCH] git.el: handle default excludesfile properly

[Eric Sunshine]

On Sat, Mar 3, 2018 at 8:36 PM, Dorab Patel  wrote:
> Correct me if I'm wrong, but my understanding, from
> https://git-scm.com/docs/gitignore, is that $HOME/.gitignore is used
> only if it is specified as the value of core.excludesfile in
> ~/.gitconfig. It is not used by default. If that is so, then the
> proposed (and original) code works. The changes I am proposing handle
> the default case, when core.excludesfile is not specified.

You're right. I must have set core.excludesfile so long ago that I
forgot about it and assumed $HOME/.gitignore was consulted by default.

> Looking deeper into how the function git-get-exclude-files is used, I
> see that it is only being called from git-run-ls-files-with-excludes.
> So, perhaps, a better (or additional) fix might be to add the
> parameter "--exclude-standard" in the call to git-run-ls-files from
> within git-run-ls-files-with-excludes. And remove the need for
> get-get-exclude-files altogether.  Presumably, "--exclude-standard"
> handles the default case with/without XDG_CONFIG_HOME correctly. The
> question I'd have then is: why didn't the original author use that
> option? Either I'm missing something? Or the option was added later,
> after the original code was written? Or something else?

Using --exclude-standard rather than --exclude-from and retiring
git-get-exclude-files() makes sense to me.

As for why the original author didn't use --exclude-standard, project
history tells us that. In particular, git-get-exclude-files() was
implemented by 274e13e0e9 (git.el: Take into account the
core.excludesfile config option., 2007-07-31), whereas
--exclude-standard was introduced by 8e7b07c8a7 (git-ls-files: add
--exclude-standard, 2007-11-15), three and a half months later.

If you do re-roll to use --exclude-standard, then it would be good for
your commit message to explain this history, citing the relevant
commits.

Thanks.

Re: [PATCH] git.el: handle default excludesfile properly

[Dorab Patel]

[[Pardon the duplicate. I forgot to send via text/plain and the
mailing list bounced it, so resending.]]

Thanks for the feedback.

Correct me if I'm wrong, but my understanding, from
https://git-scm.com/docs/gitignore, is that $HOME/.gitignore is used
only if it is specified as the value of core.excludesfile in
~/.gitconfig. It is not used by default. If that is so, then the
proposed (and original) code works. The changes I am proposing handle
the default case, when core.excludesfile is not specified.

Looking deeper into how the function git-get-exclude-files is used, I
see that it is only being called from git-run-ls-files-with-excludes.
So, perhaps, a better (or additional) fix might be to add the
parameter "--exclude-standard" in the call to git-run-ls-files from
within git-run-ls-files-with-excludes. And remove the need for
get-get-exclude-files altogether.  Presumably, "--exclude-standard"
handles the default case with/without XDG_CONFIG_HOME correctly. The
question I'd have then is: why didn't the original author use that
option? Either I'm missing something? Or the option was added later,
after the original code was written? Or something else?

Thoughts?

On Sat, Mar 3, 2018 at 12:42 AM, Eric Sunshine  wrote:
> On Fri, Mar 2, 2018 at 10:48 PM, Dorab Patel  wrote:
>> The previous version only looked at core.excludesfile for locating the
>> excludesfile.  So, when core.excludesfile was not defined, it did not
>> use the possible default locations.
>>
>> The current version uses either $XDG_CONFIG_HOME/git/ignore or
>> $HOME/.config/git/ignore for the default excludesfile location
>> depending on whether XDG_CONFIG_HOME is defined or not.  As per the
>> documentation of gitignore.
>
> Perhaps take $HOME/.gitignore into account too?
>
>> Signed-off-by: Dorab Patel 

Re: [PATCH] git.el: handle default excludesfile properly

[Eric Sunshine]

On Fri, Mar 2, 2018 at 10:48 PM, Dorab Patel  wrote:
> The previous version only looked at core.excludesfile for locating the
> excludesfile.  So, when core.excludesfile was not defined, it did not
> use the possible default locations.
>
> The current version uses either $XDG_CONFIG_HOME/git/ignore or
> $HOME/.config/git/ignore for the default excludesfile location
> depending on whether XDG_CONFIG_HOME is defined or not.  As per the
> documentation of gitignore.

Perhaps take $HOME/.gitignore into account too?

> Signed-off-by: Dorab Patel