* Re: [PATCH] gitk: remove translated message from comments
From: Junio C Hamano @ 2017-01-18 18:27 UTC (permalink / raw)
To: Paul Mackerras; +Cc: David Aguilar, git
In-Reply-To: <20170118101515.GA12161@fergus.ozlabs.ibm.com>
Paul Mackerras <paulus@ozlabs.org> writes:
> On Tue, Jan 17, 2017 at 07:52:45PM -0800, David Aguilar wrote:
>> "make update-po" fails because a previously untranslated string
>> has now been translated:
>>
>> Updating po/sv.po
>> po/sv.po:1388: duplicate message definition...
>> po/sv.po:380: ...this is the location of the first definition
>>
>> Remove the duplicate message definition.
>>
>> Reported-by: Junio C Hamano <gitster@pobox.com>
>> Signed-off-by: David Aguilar <davvid@gmail.com>
>
> Thanks, applied.
>
> Junio, please do a pull from my repository to get this fix.
> The new head is 7f03c6e32891.
Thanks.
^ permalink raw reply
* Re: [PATCH v6 4/6] builtin/tag: add --format argument for tag -v
From: Santiago Torres @ 2017-01-18 18:28 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, peff, sunshine, walters, Lukas Puehringer
In-Reply-To: <xmqqh94wb4y0.fsf@gitster.mtv.corp.google.com>
[-- Attachment #1: Type: text/plain, Size: 1515 bytes --]
On Wed, Jan 18, 2017 at 10:25:59AM -0800, Junio C Hamano wrote:
> Junio C Hamano <gitster@pobox.com> writes:
>
> > santiago@nyu.edu writes:
> >
> >> @@ -428,9 +443,12 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
> >> if (filter.merge_commit)
> >> die(_("--merged and --no-merged option are only allowed with -l"));
> >> if (cmdmode == 'd')
> >> - return for_each_tag_name(argv, delete_tag);
> >> - if (cmdmode == 'v')
> >> - return for_each_tag_name(argv, verify_tag);
> >> + return for_each_tag_name(argv, delete_tag, NULL);
> >> + if (cmdmode == 'v') {
> >> + if (format)
> >> + verify_ref_format(format);
> >> + return for_each_tag_name(argv, verify_tag, format);
> >> + }
> >
> > This triggers:
> >
> > builtin/tag.c: In function 'cmd_tag':
> > builtin/tag.c:451:3: error: passing argument 3 of
> > 'for_each_tag_name' discards 'const' qualifier from pointer target type [-Werror]
> > return for_each_tag_name(argv, verify_tag, format);
> >
> > Either for-each-tag-name's new parameter needs to be typed
> > correctly, or the type of the "format" variable needs to be updated.
>
> Squashing the following into this commit solves this issue with the
> former approach. The lines it touches are all from 4/6 and I view
> all of it as general improvement, including type correctness and
> code formatting.
Thanks!
Should I re-roll this really quick? Or would you rather apply this on
your tree directly?
-Santiago.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: problem with insider build for windows and git
From: Junio C Hamano @ 2017-01-18 18:32 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: Michael Gooch, git
In-Reply-To: <alpine.DEB.2.20.1701181738490.3469@virtualbox>
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> And just to prove me wrong, today I got the first update to `maint` in six
> weeks, with a message "Almost ready for 2.11.1" at its tip, featuring a
> whopping 141 commits (95 of which are not merge commits).
>
> So it seems that v2.11.1 may happen soon, after all.
Sorry for being late. I had a short travel around the year boundary,
got sick and have been slow.
Aside from the "ouch, one topic has merged earlier iteration, that
was merged to 'master', also now merged to 'maint', and we need to
follow up on both" you sent out earlier, are there any other topic
that are already in 'master' that should go to 2.11.x track?
Thanks.
^ permalink raw reply
* Re: [PATCH v6 4/6] builtin/tag: add --format argument for tag -v
From: Jeff King @ 2017-01-18 18:28 UTC (permalink / raw)
To: Junio C Hamano; +Cc: santiago, git, sunshine, walters, Lukas Puehringer
In-Reply-To: <xmqqh94wb4y0.fsf@gitster.mtv.corp.google.com>
On Wed, Jan 18, 2017 at 10:25:59AM -0800, Junio C Hamano wrote:
> > This triggers:
> >
> > builtin/tag.c: In function 'cmd_tag':
> > builtin/tag.c:451:3: error: passing argument 3 of
> > 'for_each_tag_name' discards 'const' qualifier from pointer target type [-Werror]
> > return for_each_tag_name(argv, verify_tag, format);
> >
> > Either for-each-tag-name's new parameter needs to be typed
> > correctly, or the type of the "format" variable needs to be updated.
>
> Squashing the following into this commit solves this issue with the
> former approach. The lines it touches are all from 4/6 and I view
> all of it as general improvement, including type correctness and
> code formatting.
>
> diff --git a/builtin/tag.c b/builtin/tag.c
> index f81273a85a..fbb85ba3dc 100644
> --- a/builtin/tag.c
> +++ b/builtin/tag.c
> @@ -66,10 +66,10 @@ static int list_tags(struct ref_filter *filter, struct ref_sorting *sorting, con
> }
>
> typedef int (*each_tag_name_fn)(const char *name, const char *ref,
> - const unsigned char *sha1, void *cb_data);
> + const unsigned char *sha1, const void *cb_data);
This would bite us later if one of the iterators really does need to
pass something mutable. But as this iteration interface is confined to
builtin/tag.c, I think it's a nice simple fix.
A more general fix would be to pass a non-const pointer to const pointer
(preferably inside a struct for readability). But I don't see any need
for that complexity here.
-Peff
^ permalink raw reply
* Re: [PATCH v6 4/6] builtin/tag: add --format argument for tag -v
From: Junio C Hamano @ 2017-01-18 18:25 UTC (permalink / raw)
To: santiago; +Cc: git, peff, sunshine, walters, Lukas Puehringer
In-Reply-To: <xmqqmvepb4oj.fsf@gitster.mtv.corp.google.com>
Junio C Hamano <gitster@pobox.com> writes:
> santiago@nyu.edu writes:
>
>> @@ -428,9 +443,12 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
>> if (filter.merge_commit)
>> die(_("--merged and --no-merged option are only allowed with -l"));
>> if (cmdmode == 'd')
>> - return for_each_tag_name(argv, delete_tag);
>> - if (cmdmode == 'v')
>> - return for_each_tag_name(argv, verify_tag);
>> + return for_each_tag_name(argv, delete_tag, NULL);
>> + if (cmdmode == 'v') {
>> + if (format)
>> + verify_ref_format(format);
>> + return for_each_tag_name(argv, verify_tag, format);
>> + }
>
> This triggers:
>
> builtin/tag.c: In function 'cmd_tag':
> builtin/tag.c:451:3: error: passing argument 3 of
> 'for_each_tag_name' discards 'const' qualifier from pointer target type [-Werror]
> return for_each_tag_name(argv, verify_tag, format);
>
> Either for-each-tag-name's new parameter needs to be typed
> correctly, or the type of the "format" variable needs to be updated.
Squashing the following into this commit solves this issue with the
former approach. The lines it touches are all from 4/6 and I view
all of it as general improvement, including type correctness and
code formatting.
diff --git a/builtin/tag.c b/builtin/tag.c
index f81273a85a..fbb85ba3dc 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -66,10 +66,10 @@ static int list_tags(struct ref_filter *filter, struct ref_sorting *sorting, con
}
typedef int (*each_tag_name_fn)(const char *name, const char *ref,
- const unsigned char *sha1, void *cb_data);
+ const unsigned char *sha1, const void *cb_data);
static int for_each_tag_name(const char **argv, each_tag_name_fn fn,
- void *cb_data)
+ const void *cb_data)
{
const char **p;
char ref[PATH_MAX];
@@ -95,7 +95,7 @@ static int for_each_tag_name(const char **argv, each_tag_name_fn fn,
}
static int delete_tag(const char *name, const char *ref,
- const unsigned char *sha1, void *cb_data)
+ const unsigned char *sha1, const void *cb_data)
{
if (delete_ref(ref, sha1, 0))
return 1;
@@ -104,10 +104,10 @@ static int delete_tag(const char *name, const char *ref,
}
static int verify_tag(const char *name, const char *ref,
- const unsigned char *sha1, void *cb_data)
+ const unsigned char *sha1, const void *cb_data)
{
int flags;
- char *fmt_pretty = cb_data;
+ const char *fmt_pretty = cb_data;
flags = GPG_VERIFY_VERBOSE;
if (fmt_pretty)
^ permalink raw reply related
* Re: problem with insider build for windows and git
From: Johannes Schindelin @ 2017-01-18 16:41 UTC (permalink / raw)
To: Michael Gooch; +Cc: git
In-Reply-To: <alpine.DEB.2.20.1701131300390.3469@virtualbox>
Hi,
On Sat, 14 Jan 2017, Johannes Schindelin wrote:
> Footnote *2*: http://tinyurl.com/gitCal suggests that Git v2.12.0 will
> be released early February soon after Git Merge (and Git for Windows
> v2.12.0 will follow soon thereafter), and with no patches applied to the
> `maint` branch since v2.11.0, I do actually not expect any v2.11.1 to
> happen before v2.12.0 comes out.
And just to prove me wrong, today I got the first update to `maint` in six
weeks, with a message "Almost ready for 2.11.1" at its tip, featuring a
whopping 141 commits (95 of which are not merge commits).
So it seems that v2.11.1 may happen soon, after all.
Sorry for my misjudgement,
Johannes
^ permalink raw reply
* Re: [RFC] stash --continue
From: Johannes Schindelin @ 2017-01-18 16:34 UTC (permalink / raw)
To: Marc Branchaud; +Cc: Stephan Beyer, git
In-Reply-To: <d5456165-bdf2-e9e7-117f-aeab0ff4b417@xiplink.com>
Hi Marc,
On Wed, 18 Jan 2017, Marc Branchaud wrote:
> On 2017-01-16 05:54 AM, Johannes Schindelin wrote:
>
> > On Mon, 16 Jan 2017, Stephan Beyer wrote:
> >
> > > a git-newbie-ish co-worker uses git-stash sometimes. Last time he
> > > used "git stash pop", he got into a merge conflict. After he
> > > resolved the conflict, he did not know what to do to get the
> > > repository into the wanted state. In his case, it was only "git add
> > > <resolved files>" followed by a "git reset" and a "git stash drop",
> > > but there may be more involved cases when your index is not clean
> > > before "git stash pop" and you want to have your index as before.
> > >
> > > This led to the idea to have something like "git stash
> > > --continue"[1]
> >
> > More like "git stash pop --continue". Without the "pop" command, it
> > does not make too much sense.
>
> Why not? git should be able to remember what stash command created the
> conflict. Why should I have to? Maybe the fire alarm goes off right when I
> run the stash command, and by the time I get back to it I can't remember
> which operation I did. It would be nice to be able to tell git to "just
> finish off (or abort) the stash operation, whatever it was".
That reeks of a big potential for confusion.
Imagine for example a total Git noob who calls `git stash list`, scrolls
two pages down, then hits `q` by mistake. How would you explain to that
user that `git stash --continue` does not continue showing the list at the
third page?
Even worse: `git stash` (without arguments) defaults to the `save`
operation, so any user who does not read the documentation (and who does?)
would assume that `git stash --continue` *also* implies `save`.
If that was not enough, there would still be the overall design of Git's
user interface. You can call it confusing, inconsistent, with a lot of
room for improvement, and you would be correct. But none of Git's commands
has a `--continue` option that remembers the latest subcommand and
continues that. To introduce that behavior in `git stash` would disimprove
the situation.
With every new feature, it is not enough to consider its benefits. You
always have to take the potential fallout into account, too.
At least `git stash pop --continue` would be consistent with all other
`--continue` options in Git that I can think of...
Ciao,
Johannes
^ permalink raw reply
* Re: [PATCH 2/2] Be more careful when determining whether a remote was configured
From: Johannes Schindelin @ 2017-01-18 16:22 UTC (permalink / raw)
To: Jeff King; +Cc: git, Junio C Hamano, Thomas Gummerer, Andrew Arnott
In-Reply-To: <20170118125422.gi3ppqiqhyykk7iy@sigill.intra.peff.net>
Hi Peff,
On Wed, 18 Jan 2017, Jeff King wrote:
> On Wed, Jan 18, 2017 at 01:34:28PM +0100, Johannes Schindelin wrote:
>
> > > > Let's fix this by telling Git that a remote is not configured
> > > > unless any fetch/push URL or refspect is configured explicitly.
> > >
> > > Hmm. Old versions of GitHub for Windows used to set fetch refspecs
> > > in the system gitconfig, for a similar purpose to what you want to
> > > do with remote.origin.prune.
> > >
> > > I notice here that setting a refspec _does_ define a remote. Is
> > > there a reason you drew the line there, and not at, say, whether it
> > > has a URL?
> >
> > I want to err on the side of caution. That's why.
>
> I guess I just don't see why changing the behavior with respect to
> "prune" or "proxy" is any less conservative than changing the one for
> "refspec".
Let's take a step back and consider the problem I try to solve, okay?
The problem is that `git remote rename <old> <new>` refuses to do its job
if it detects a configured `<new>`. And it detects a configured `<new>`
even in cases where there is not *really* any remote configured.
The example use case is to configure `remote.origin.prune = true` in
~/.gitconfig, i.e. changing Git's default for all "origin" remotes in all
of the user's repositories.
Now, the *real* fix would be to detect whether the remote was "configured"
in the current repository, or in ~/.gitconfig. But that may not even be
desirable, as we would want a more general fix for the question: can `git
remote` rename a given remote to a new name, i.e. is that new name already
taken?
And if you try to answer that last question, you will probably pick the
same set of keys for which you assume that remote.<name>.<key> really
configures a remote or not.
Originally, I would even have put the "vcs" into that set, as I could see
a legitimate use case for users to configure "remote.svn.vcs = vcs" in
~/.gitconfig. But the regression test suite specifically tests for that
case, and I trust that there was a good reason, even if Thomas did not
describe that good reason in the commit message nor in any reply to this
patch pair.
So how can things go wrong?
Let's take your example that the user may have specified refspecs (or
prune, or proxy) for a URL via "remote.<url>.fetch", and that `git rename`
incorrectly allows that as a new remote name. You know what? Let's do a
real code review here, not just a patch glance-over. Let's test this and
*know* whether it can be a problem:
git remote rename origin https://github.com/git/git
fatal: 'https://github.com/git/git' is not a valid remote name
A ha! It is *not* possible to hit that case because `git remote rename`
already complains much earlier about the new name not being a valid name.
So let's see what could go wrong with another example you mentioned, that
the proxy may not be used because we changed the logic of
remote_is_configured(). But the only user of the remote proxy settings is
in http_init() and reads:
if (remote && remote->http_proxy)
curl_http_proxy = xstrdup(remote->http_proxy);
if (remote)
var_override(&http_proxy_authmethod, remote->http_proxy_authmethod);
It does not even test whether the remote is configured! So maybe the
caller does? Nope, the only caller of http_init() that passes a remote is
remote-curl's cmd_main() function, and the relevant part reads:
remote = remote_get(argv[1]);
if (argc > 2) {
end_url_with_slash(&url, argv[2]);
} else {
end_url_with_slash(&url, remote->url[0]);
}
http_init(remote, url.buf, 0);
This code also does not care whether the remote "is configured" or not.
So maybe there are any other downsides with callers of
remote_is_configured()?
There is one caller in builtin/fetch.c's add_remote_or_group() which
clearly is covered (we need a URL, unless the vcs is configured).
All other callers are in builtin/remote.c:
- in add(), to test whether we can add a new remote,
- in mv(), to test whether the remote to rename is configured,
- in mv(), to test whether the new name is already taken,
- in rm(), to test whether the remote exists,
- in set_remote_branches(), to test whether the remote to be changed
exists,
- in get_url(), to test whether the remote exists, and
- in set_url(), to test whether the remote exists.
It appears pretty obvious that in all of these cases, the suggested patch
still makes sense and does not introduce any nasty surprise.
> I can think of one alternative approach that might be easier for users
> to understand, and that we already use elsewhere (e.g., with "http.*"
> config): have a set of "default" remote keys (e.g., just "remote.key")
> that git falls back to when the remote.*.key isn't set. Then your use
> case becomes something like:
>
> [remote]
> prune = true
>
> That's not _exactly_ the same, as it applies to all remotes, not just
> "origin" (other remotes can override it, but would need to do so
> explicitly). But I have a suspicion that may actually be what users
> want.
Sure. That'll be another patch series, though. And another contributor.
Ciao,
Johannes
^ permalink raw reply
* Re: [RFC] stash --continue
From: Marc Branchaud @ 2017-01-18 15:41 UTC (permalink / raw)
To: Johannes Schindelin, Stephan Beyer; +Cc: git
In-Reply-To: <alpine.DEB.2.20.1701161153340.3469@virtualbox>
On 2017-01-16 05:54 AM, Johannes Schindelin wrote:
> Hi Stephan,
>
> On Mon, 16 Jan 2017, Stephan Beyer wrote:
>
>> a git-newbie-ish co-worker uses git-stash sometimes. Last time he used
>> "git stash pop", he got into a merge conflict. After he resolved the
>> conflict, he did not know what to do to get the repository into the
>> wanted state. In his case, it was only "git add <resolved files>"
>> followed by a "git reset" and a "git stash drop", but there may be more
>> involved cases when your index is not clean before "git stash pop" and
>> you want to have your index as before.
>>
>> This led to the idea to have something like "git stash --continue"[1]
>
> More like "git stash pop --continue". Without the "pop" command, it does
> not make too much sense.
Why not? git should be able to remember what stash command created the
conflict. Why should I have to? Maybe the fire alarm goes off right
when I run the stash command, and by the time I get back to it I can't
remember which operation I did. It would be nice to be able to tell git
to "just finish off (or abort) the stash operation, whatever it was".
M.
^ permalink raw reply
* Re: git fast-import crashing on big imports
From: Jeff King @ 2017-01-18 14:38 UTC (permalink / raw)
To: Ulrich Spoerlein; +Cc: git, Ed Maste, Junio C Hamano
In-Reply-To: <20170118140117.GK4426@acme.spoerlein.net>
On Wed, Jan 18, 2017 at 03:01:17PM +0100, Ulrich Spoerlein wrote:
> Yo Jeff, your commit 8261e1f139db3f8aa6f9fd7d98c876cbeb0f927c from Aug
> 22nd, that changes delta_base_cache to use hashmap.h is the culprit for
> git fast-import crashing on large imports.
I actually saw your bug report the other day and tried to download the
dump file, but got a 404. Can you double check that it is available?
-Peff
^ permalink raw reply
* Re: git fast-import crashing on big imports
From: Ulrich Spoerlein @ 2017-01-18 14:01 UTC (permalink / raw)
To: git, Jeff King; +Cc: Ed Maste, Junio C Hamano
In-Reply-To: <20170112082138.GJ4426@acme.spoerlein.net>
Yo Jeff, your commit 8261e1f139db3f8aa6f9fd7d98c876cbeb0f927c from Aug
22nd, that changes delta_base_cache to use hashmap.h is the culprit for
git fast-import crashing on large imports.
Please read below, you can find a 55G SVN dump that should show the
problem after a couple of minutes to less than an hour. Please also see
similar issues from 2009 and 2011. This seems to be a rather fragile
part of the code, could you add unit tests that make sure this
regression is not re-introduce again once you fix it? Thanks!
I'm happy to test any patches that you can provide.
Cheers,
Uli
On Do., 2017-01-12 at 09:21:38 +0100, Ulrich Spörlein wrote:
> Hey,
>
> the FreeBSD svn2git conversion is crashing somewhat
> non-deterministically during its long conversion process. From memory,
> this was not as bad is it is with more recent versions of git (but I
> can't be sure, really).
>
> I have a dump file that you can grab at
> http://scan.spoerlein.net/pub/freebsd-base.dump.xz (19G, 55G uncompressed)
> that shows this problem after a couple of minutes of runtime. The caveat is
> that for another member of the team on a different machine the crashes are on
> different revisions.
>
> Googling around I found two previous threads that were discussing
> problems just like this (memory corruption, bad caching, etc)
>
> https://www.spinics.net/lists/git/msg93598.html from 2009
> and
> http://git.661346.n2.nabble.com/long-fast-import-errors-out-quot-failed-to-apply-delta-quot-td6557884.html
> from 2011
>
> % git fast-import --stats < ../freebsd-base.dump
> ...
> progress SVN r49318 branch master = :49869
> progress SVN r49319 branch stable/3 = :49870
> progress SVN r49320 branch master = :49871
> error: failed to apply delta
> error: bad offset for revindex
> error: bad offset for revindex
> error: bad offset for revindex
> error: bad offset for revindex
> error: bad offset for revindex
> fatal: Can't load tree b35ae4e9c2a41677e84a3f14bed09f584c3ff25e
> fast-import: dumping crash report to fast_import_crash_29613
>
>
> fast-import crash report:
> fast-import process: 29613
> parent process : 29612
> at 2017-01-11 19:33:37 +0000
>
> fatal: Can't load tree b35ae4e9c2a41677e84a3f14bed09f584c3ff25e
>
>
> git fsck shows a somewhat incomplete pack file (I guess that's expected if the
> process dies mid-stream?)
>
> % git fsck
> Checking object directories: 100% (256/256), done.
> error: failed to apply delta6/614500)
> error: cannot unpack d1d7ee1f81c6767c5e0f75d14d400d7512a85a0f from ./objects/pack/pack-e28fcea43fc221d2ebe92857b484da58bb888237.pack at offset 122654805
> error: failed to apply delta
> error: failed to read delta base object d1d7ee1f81c6767c5e0f75d14d400d7512a85a0f at offset 122654805 from ./objects/pack/pack-e28fcea43fc221d2ebe92857b484da58bb888237.pack
> error: cannot unpack 8523bde63ef34bef725347994fdaec996d756510 from ./objects/pack/pack-e28fcea43fc221d2ebe92857b484da58bb888237.pack at offset 122671596
> error: failed to apply delta0/614500)
> error: failed to read delta base object d1d7ee1f81c6767c5e0f75d14d400d7512a85a0f at offset 122654805 from ./objects/pack/pack-e28fcea43fc221d2ebe92857b484da58bb888237.pack
> ...
>
>
> Any comments on whether the original problems from 2009 and 2011 were ever
> fixed and committed?
>
> Some more facts:
> - git version 2.11.0
> - I don't recall these sorts of crashes with a git from 2-3 years ago
> - adding more checkpoints does help, but not fix the problem, it merely shifts
> the crashes around to different revisions
> - incremental runs of the conversion *will* complete most of the time, but
> depending on how often checkpoints are used, I've seen it croak on specific
> commits and not being able to progress further :(
>
> Thanks for any pointers or things to try!
> Cheers
> Uli
^ permalink raw reply
* Re: What's cooking in git.git (Jan 2017, #02; Sun, 15)
From: Johannes Schindelin @ 2017-01-18 13:54 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <xmqqbmv5fp22.fsf@gitster.mtv.corp.google.com>
Hi Junio,
On Tue, 17 Jan 2017, Junio C Hamano wrote:
> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>
> > On Sun, 15 Jan 2017, Junio C Hamano wrote:
> >
> >> * js/prepare-sequencer-more (2017-01-09) 38 commits
>
> > The only outstanding review comments I know about are your objection
> > to the name of the read_env_script() function (which I shot down as
> > bogus),
>
> Not the "name", but the implementation not sharing the same code
> with "am" codepath even though they originate from the same shell
> function and serve the same purpose.
They do not originate from the same shell function at all. The scripted
git-am used a function called get_author_ident_from_commit to read the
author-script file. It also used "eval $(cat .../author-script)" to
evaluate the script later on.
And while git-rebase--interactive.sh's ". .../author-script" is very
similar to that latter invocation, I grant you that, the claim that those
codepaths originate from the same shell function is false.
Worse.
git-am.sh not only used a different method to read out the author-script,
and not only left no way for the user to modify that author-script, there
are many more differences in the exact code paths when comparing git-am.sh
to git-rebase--interactive.sh.
git-am.sh goes out of its way to not only apply the patch manually but
also to use the low-level `write-tree` and `commit-tree` commands. The
builtin am reproduces this as faithfully as possible (the author-script is
not actually evaluated, of course, but the builtin am knows that it has
to validate the author-script's contents for the first code path reading
the author-script anyway, so it reuses that function for the second code
path, knowing fully well the exact format of that file because it knows it
wrote it without any chance of the user messing it up).
The builtin am also avoids spawning the low-level commands, opting instead
to call the functions in libgit.a directly. This is all done properly, and
as a consequence, the environment variables GIT_AUTHOR_* never become,
ahem, environment variables in the builtin am, but an "author" parameter
(which is a const char * pointing to a ready-to-use ident line) that gets
passed to the commit_tree() function.
Now, let's have a brief look at git-rebase--interactive.sh.
It was rightfully nicknamed "cherry-pick on drugs" in its early days,
because that is what it does: it calls `git cherry-pick` *most* of the
time.
So what does the (sequencer via being called from the) rebase--helper do?
Yep, exactly: it calls the internal do_pick() function that is the working
horse of the cherry-pick. And does that working horse call write_tree()
and commit_tree()? No, it does not. It *spawns a git-commit process*! And
the way to specify the author there is to pass the GIT_AUTHOR_*
environment variables.
Let's recapitulate.
Not only did the scripted am use a totally different code path to *read*
the author-script than rebase -i, the code path after that also differed
substantially from rebase -i to the point that very, very different Git
commands were called.
It also so happens that the proper way to convert those code paths into
builtin code uses very, very different functions from libgit.a that
require very, very different handling.
The builtin am never sets the environment variables GIT_AUTHOR_* but
instead constructs a complete ident line from it, and therefore *must*
validate the contents of author-script.
The sequencer *has to* set the GIT_AUTHOR_* environment variables because
it calls `git commit` in a different process *that already validates the
GIT_AUTHOR_* variables*.
Even worse (and this is not the first, or second, or third time I pointed
this out): if you mistook the identical file name, and identical content,
to mean that the different code paths *must* use a *single* function to
read the author-script (nevermind that the am code path reads it into a
structure that is specifically designed to support the "am" operation, and
nothing else), you would not only force the sequencer call to provide a
data structure it does not want, not only to validate the contents
unnecessarily, but it would have to lego the parsed contents *back into
almost the original shape as the original contents of the author-script*.
So it would have to add tons of code relative to the current version, for
no benefit at all, *increasing your maintenance burden for no good
reason*.
This repeated suggestion to reuse the code path to read the author-script
repeatedly ignores the fact that we have to do very, very different things
with the contents in those two code paths.
And if that was not enough: we are talking about code that is rock solid,
has been tested for almost a *year*, half of that year with a painful
cross-validation by running old and new rebase -i and comparing their
output. And this rock solid code is what you want to force me to change to
code that neither makes sense nor is tested well.
And I refuse to be forced to do that: we just proved collectively how good
a job we do when reviewing patches: we just flimsically introduced a
regression at the core of the operation of rebase -i. Not despite patch
review. Because of it.
In short: the objections against keeping the read_env_script() function
as-is (now fixed, of course) make no sense from several points of view:
logically, time-wise, and robustness.
> > and the rather real bug fix I sent out as a fixup! which you may want
> > to squash in (in the alternative, I can mailbomb v4 of the entire
> > sequencer-i patch series, that is your choice).
>
> I'd rather see the "make elengant" thing redone in a more sensible
> way,
No, no, no, NO!
This is starting to become really, really annoying. The reasons you keep
repeating for rewriting this to use the same functions as builtin/am.c *do
not make sense*. Even when you repeat them a hundred times (I know, in the
US you have this really public example where somebody is really successful
repeating incorrect things over and over until some people start to
believe them, but this is not an example you should try to follow).
Just because you keep repeating that "am" and "rebase -i" both read
"author-script" files with the same syntax does not mean that it is
sensible to force them to use the same code path when the data needs to be
processed very differently afterwards!
And you can repeat another thousand times that it would make maintaining
the code easier, and it *still* would not make that statement less false.
The "am" command needs to validate the contents and process them into an
ident line.
The sequencer needs to pass the contents as environment variables to the
"git commit" command that validates the contents itself.
> I'll squash the fixup! thing in, and as I already said a few days ago, I
> do not think we'd want v4 (rather we'd want to go incremental).
Now, those are three statements that all make sense.
Thank you,
Johannes
^ permalink raw reply
* Re: [BUG/RFC] Git Gui: GIT_DIR leaks to spawned Git Bash
From: Johannes Schindelin @ 2017-01-18 13:04 UTC (permalink / raw)
To: Max Kirillov; +Cc: Pat Thoyts, git, Junio C Hamano
In-Reply-To: <20170117184224.GA23841@jessie.local>
Hi Max,
On Tue, 17 Jan 2017, Max Kirillov wrote:
> On Tue, Jan 17, 2017 at 11:52:29AM +0100, Johannes Schindelin wrote:
>
> > And having said *that*, I have to admit that I was unable to reproduce
>
> I have found exact way to reproduce it and actually the reason. Turns
> out to be obvious mistake in code. PR is in github:
> https://github.com/git-for-windows/git/pull/1032
As I pointed out in my review, I think that GIT_DIR is actually only set
for `gitk` in the non-submodule case. So I guess if I run Git GUI, spawn
gitk, and then Git Bash, it will have GIT_DIR set.
*clicketyclick*
Bingo. That reproduces the problem here.
Thanks,
Johannes
^ permalink raw reply
* Re: [PATCH 2/2] Be more careful when determining whether a remote was configured
From: Jeff King @ 2017-01-18 12:54 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git, Junio C Hamano, Thomas Gummerer, Andrew Arnott
In-Reply-To: <alpine.DEB.2.20.1701181334040.3469@virtualbox>
On Wed, Jan 18, 2017 at 01:34:28PM +0100, Johannes Schindelin wrote:
> > > Let's fix this by telling Git that a remote is not configured unless any
> > > fetch/push URL or refspect is configured explicitly.
> >
> > Hmm. Old versions of GitHub for Windows used to set fetch refspecs in
> > the system gitconfig, for a similar purpose to what you want to do with
> > remote.origin.prune.
> >
> > I notice here that setting a refspec _does_ define a remote. Is there a
> > reason you drew the line there, and not at, say, whether it has a URL?
>
> I want to err on the side of caution. That's why.
I guess I just don't see why changing the behavior with respect to
"prune" or "proxy" is any less conservative than changing the one for
"refspec". Both can make some real-world cases work, and both can cause
breakage in some possible real-world cases. If we are going to change
anything, it seems like we should at least aim for a simple and
consistent rule (since users have to know which keys can be put in
~/.gitconfig and which cannot).
I can think of one alternative approach that might be easier for users
to understand, and that we already use elsewhere (e.g., with "http.*"
config): have a set of "default" remote keys (e.g., just "remote.key")
that git falls back to when the remote.*.key isn't set. Then your use
case becomes something like:
[remote]
prune = true
That's not _exactly_ the same, as it applies to all remotes, not just
"origin" (other remotes can override it, but would need to do so
explicitly). But I have a suspicion that may actually be what users
want.
-Peff
^ permalink raw reply
* Re: [PATCH 5/5] describe: teach describe negative pattern matches
From: Johannes Schindelin @ 2017-01-18 12:44 UTC (permalink / raw)
To: Jacob Keller; +Cc: Johannes Sixt, Jacob Keller, Git mailing list
In-Reply-To: <CA+P7+xqFHG52Xo8ncUwa3owDn3OOz+rr3ZaGwfcUDCiXJmh80Q@mail.gmail.com>
Hi Jake,
On Tue, 17 Jan 2017, Jacob Keller wrote:
> On Fri, Jan 13, 2017 at 1:31 PM, Johannes Sixt <j6t@kdbg.org> wrote:
> > Am 13.01.2017 um 07:57 schrieb Jacob Keller:
> >>
> >> On Thu, Jan 12, 2017 at 10:43 PM, Johannes Sixt <j6t@kdbg.org> wrote:
> >>>
> >>> When you write
> >>>
> >>> git log --branches --exclude=origin/* --remotes
> >>>
> >>> --exclude=origin/* applies only to --remotes, but not to --branches.
> >>
> >>
> >> Well for describe I don't think the order matters.
> >
> >
> > That is certainly true today. But I would value consistency more. We would
> > lose it if some time in the future 'describe' accepts --branches and
> > --remotes in addition to --tags and --all.
> >
> > -- Hannes
> >
>
> I am not sure that the interface for git-log and git-describe are
> similar enough to make this distinction work. --match already seems to
> imply that it only works on refs in refs/tags, as it says it considers
> globs matching excluding the "refs/tags" prefix.
>
> In git-describe, we already have "--tags" and "--all" but they are
> mutually exclusive. We don't support using more than one at once, and
> I'm not really convinced that describe will ever support more than one
> at a time. Additionally, match already doesn't respect order.
I agree that it would keep the code much simpler if you kept the order
"exclude before include".
However, should you decide to look into making the logic dependent on the
order in which the flags were specified in the command-line, we do have a
data structure for such a beast: we use it in gitignore and in
sparse-checkout, it is called struct exclude_list.
Just some food for thought,
Johannes
^ permalink raw reply
* Re: [PATCH 2/2] Be more careful when determining whether a remote was configured
From: Johannes Schindelin @ 2017-01-18 12:38 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jeff King, git, Thomas Gummerer, Andrew Arnott
In-Reply-To: <xmqqd1flcosv.fsf@gitster.mtv.corp.google.com>
Hi Junio,
On Tue, 17 Jan 2017, Junio C Hamano wrote:
> Perhaps instead of adding "is it configured?" flag that is too
> broadly named and has too narrow meaning, would it make more sense
> to introduce "int can_prune(struct remote *remote)" that looks at
> the remote refspecs?
That ("can we prune?") is not the question we need to ask when determining
whether we can rename a remote to a new name.
Ciao,
Johannes
^ permalink raw reply
* Re: [PATCH 2/2] Be more careful when determining whether a remote was configured
From: Johannes Schindelin @ 2017-01-18 12:34 UTC (permalink / raw)
To: Jeff King; +Cc: git, Junio C Hamano, Thomas Gummerer, Andrew Arnott
In-Reply-To: <20170117214723.p5rni6wwggei366j@sigill.intra.peff.net>
Hi Peff,
On Tue, 17 Jan 2017, Jeff King wrote:
> On Tue, Jan 17, 2017 at 10:19:24PM +0100, Johannes Schindelin wrote:
>
> > One of the really nice features of the ~/.gitconfig file is that users
> > can override defaults by their own preferred settings for all of their
> > repositories.
> >
> > One such default that some users like to override is whether the
> > "origin" remote gets auto-pruned or not. The user would simply call
> >
> > git config --global remote.origin.prune true
> >
> > and from now on all "origin" remotes would be pruned automatically when
> > fetching into the local repository.
> >
> > There is just one catch: now Git thinks that the "origin" remote is
> > configured, as it does not discern between having a remote whose
> > fetch (and/or push) URL and refspec is set, and merely having
> > preemptively-configured, global flags for specific remotes.
> >
> > Let's fix this by telling Git that a remote is not configured unless any
> > fetch/push URL or refspect is configured explicitly.
>
> Hmm. Old versions of GitHub for Windows used to set fetch refspecs in
> the system gitconfig, for a similar purpose to what you want to do with
> remote.origin.prune.
>
> I notice here that setting a refspec _does_ define a remote. Is there a
> reason you drew the line there, and not at, say, whether it has a URL?
I want to err on the side of caution. That's why.
Ciao,
Johannes
^ permalink raw reply
* Re: [PATCH v5 3/3] Retire the scripted difftool
From: Johannes Schindelin @ 2017-01-18 12:33 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, David Aguilar, Dennis Kaarsemaker, Paul Sbarra
In-Reply-To: <xmqqk29tcqb8.fsf@gitster.mtv.corp.google.com>
Hi Junio,
On Tue, 17 Jan 2017, Junio C Hamano wrote:
> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
>
> > It served its purpose, but now we have a builtin difftool. Time for the
> > Perl script to enjoy Florida.
> >
> > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> > ---
>
> The endgame makes a lot of sense. Both in the cover letter and in
> the previous patch you talk about having both in the released
> version, so do you want this step to proceed slower than the other
> two?
I did proceed that slowly. Already three Git for Windows versions have
been released with both.
But I submitted this iteration with this patch, so my intent is clearly to
retire the Perl script.
Ciao,
Johannes
^ permalink raw reply
* [PATCH] mingw: follow-up to "replace isatty() hack"
From: Johannes Schindelin @ 2017-01-18 12:14 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Pranit Bauva, Johannes Sixt, Beat Bolli
The version of the "replace isatty() hack" that got applied to the
`maint` branch did not actually reflect the latest iteration of the
patch series: v3 was sent out with these changes, as requested by
the reviewer Johannes Sixt:
- reworded the comment about "recycling handles"
- moved the reassignment of the `console` variable before the dup2()
call so that it is valid at all times
- removed the "handle = INVALID_HANDLE_VALUE" assignment, as the local
variable `handle` is not used afterwards anyway
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
Published-As: https://github.com/dscho/git/releases/tag/mingw-isatty-fixup-fixup-v1
Fetch-It-Via: git fetch https://github.com/dscho/git mingw-isatty-fixup-fixup-v1
compat/winansi.c | 11 ++++-------
1 file changed, 4 insertions(+), 7 deletions(-)
diff --git a/compat/winansi.c b/compat/winansi.c
index 3c9ed3cfe0..82b89ab137 100644
--- a/compat/winansi.c
+++ b/compat/winansi.c
@@ -494,19 +494,16 @@ static HANDLE swap_osfhnd(int fd, HANDLE new_handle)
* It is because of this implicit close() that we created the
* copy of the original.
*
- * Note that the OS can recycle HANDLE (numbers) just like it
- * recycles fd (numbers), so we must update the cached value
- * of "console". You can use GetFileType() to see that
- * handle and _get_osfhandle(fd) may have the same number
- * value, but they refer to different actual files now.
+ * Note that we need to update the cached console handle to the
+ * duplicated one because the dup2() call will implicitly close
+ * the original one.
*
* Note that dup2() when given target := {0,1,2} will also
* call SetStdHandle(), so we don't need to worry about that.
*/
- dup2(new_fd, fd);
if (console == handle)
console = duplicate;
- handle = INVALID_HANDLE_VALUE;
+ dup2(new_fd, fd);
/* Close the temp fd. This explicitly closes "new_handle"
* (because it has been associated with it).
base-commit: 3313b78c145ba9212272b5318c111cde12bfef4a
--
2.11.0.windows.3
^ permalink raw reply related
* Re: [PATCH v3 3/3] mingw: replace isatty() hack
From: Johannes Schindelin @ 2017-01-18 12:13 UTC (permalink / raw)
To: git; +Cc: Jeff Hostetler, Junio C Hamano, Pranit Bauva, Johannes Sixt,
Beat Bolli
In-Reply-To: <2ce6060b891f8313cc63a95a9cba9064b7f82eb8.1482448531.git.johannes.schindelin@gmx.de>
Hi,
On Fri, 23 Dec 2016, Johannes Schindelin wrote:
> + /*
> + * Use stock dup2() to re-bind fd to the new handle. Note that
> + * this will implicitly close(1) and close both fd=1 and the
> + * originally associated handle. It will open a new fd=1 and
> + * call DuplicateHandle() on the handle associated with new_fd.
> + * It is because of this implicit close() that we created the
> + * copy of the original.
> + *
> + * Note that we need to update the cached console handle to the
> + * duplicated one because the dup2() call will implicitly close
> + * the original one.
> + *
> + * Note that dup2() when given target := {0,1,2} will also
> + * call SetStdHandle(), so we don't need to worry about that.
> + */
> + if (console == handle)
> + console = duplicate;
> + dup2(new_fd, fd);
Sadly, v2 was applied to `maint` instead of this revision, so Hannes'
suggestions that I addressed in v3 were dropped silently.
I will send out a follow-up patch in a moment.
Ciao,
Johannes
^ permalink raw reply
* Re: "git diff --ignore-space-change --stat" lists files with only whitespace differences as "changed"
From: Jeff King @ 2017-01-18 11:17 UTC (permalink / raw)
To: Matt McCutchen; +Cc: git
In-Reply-To: <1484704915.2096.16.camel@mattmccutchen.net>
On Tue, Jan 17, 2017 at 09:01:55PM -0500, Matt McCutchen wrote:
> A bug report: I noticed that "git diff --ignore-space-change --stat"
> lists files with only whitespace differences as having changed with 0
> differing lines. This is inconsistent with the behavior without --
> stat, which doesn't list such files at all. (Same behavior with all
> the --ignore*space* flags.) I can reproduce this with the current
> "next", af746e4. Quick test case:
Hmm. This is pretty easy to do naively, but the special-casing for
addition/deletion (which I think we _do_ need, and which certainly we
fail t4205 without) makes me feel dirty. I'd worry there are other
cases, too (perhaps renames?). And I also notice that the
binary-diffstat code path just above my changes explicitly creates 0/0
diffstats, but I'm not even sure how one would trigger that.
So I dunno. A sensible rule to me is "iff -p would show a diff header,
then --stat should mention it". I think we'd want to somehow extract the
logic from builtin_diff() and reuse it.
---
diff --git a/diff.c b/diff.c
index e2eb6d66a..57ff5c1dc 100644
--- a/diff.c
+++ b/diff.c
@@ -2105,17 +2105,20 @@ static void show_dirstat_by_line(struct diffstat_t *data, struct diff_options *o
gather_dirstat(options, &dir, changed, "", 0);
}
+static void free_diffstat_file(struct diffstat_file *f)
+{
+ if (f->name != f->print_name)
+ free(f->print_name);
+ free(f->name);
+ free(f->from_name);
+ free(f);
+}
+
static void free_diffstat_info(struct diffstat_t *diffstat)
{
int i;
- for (i = 0; i < diffstat->nr; i++) {
- struct diffstat_file *f = diffstat->files[i];
- if (f->name != f->print_name)
- free(f->print_name);
- free(f->name);
- free(f->from_name);
- free(f);
- }
+ for (i = 0; i < diffstat->nr; i++)
+ free_diffstat_file(diffstat->files[i]);
free(diffstat->files);
}
@@ -2603,6 +2606,23 @@ static void builtin_diffstat(const char *name_a, const char *name_b,
if (xdi_diff_outf(&mf1, &mf2, diffstat_consume, diffstat,
&xpp, &xecfg))
die("unable to generate diffstat for %s", one->path);
+
+ /*
+ * Omit diffstats where nothing changed. Even if
+ * !same_contents, this might be the case due to ignoring
+ * whitespace changes, etc.
+ *
+ * But note that we special-case additions and deletions,
+ * as adding an empty file, for example, is still of interest.
+ */
+ if (DIFF_FILE_VALID(one) && DIFF_FILE_VALID(two)) {
+ struct diffstat_file *file =
+ diffstat->files[diffstat->nr - 1];
+ if (!file->added && !file->deleted) {
+ free_diffstat_file(file);
+ diffstat->nr--;
+ }
+ }
}
diff_free_filespec_data(one);
diff --git a/t/t4015-diff-whitespace.sh b/t/t4015-diff-whitespace.sh
index 289806d0c..2805db411 100755
--- a/t/t4015-diff-whitespace.sh
+++ b/t/t4015-diff-whitespace.sh
@@ -736,7 +736,7 @@ test_expect_success 'checkdiff allows new blank lines' '
cat <<EOF >expect
EOF
-test_expect_success 'whitespace-only changes not reported' '
+test_expect_success 'whitespace-only changes not reported (diff)' '
git reset --hard &&
echo >x "hello world" &&
git add x &&
@@ -746,6 +746,12 @@ test_expect_success 'whitespace-only changes not reported' '
test_cmp expect actual
'
+test_expect_success 'whitespace-only changes not reported (diffstat)' '
+ # reuse state from previous test
+ git diff --stat -b >actual &&
+ test_cmp expect actual
+'
+
cat <<EOF >expect
diff --git a/x b/z
similarity index NUM%
^ permalink raw reply related
* Git: new feature suggestion
From: Joao Pinto @ 2017-01-18 10:40 UTC (permalink / raw)
To: git; +Cc: Linus Torvalds, CARLOS.PALMINHA@synopsys.com
Hello,
My name is Joao Pinto, I work at Synopsys and I am a frequent Linux Kernel
contributor.
Let me start by congratulate you for the fantastic work you have been doing with
Git which is an excellent tool.
The Linux Kernel as all systems needs to be improved and re-organized to be
better prepared for future development and sometimes we need to change
folder/files names or even move things around.
I have seen a lot of Linux developers avoid this re-organization operations
because they would lose the renamed file history, because a new log is created
for the new file, even if it is a renamed version of itself.
I am sending you this e-mail to suggest the creation of a new feature in Git:
when renamed, a file or folder should inherit his parent’s log and a “rename: …”
would be automatically created or have some kind of pointer to its “old” form to
make history analysis easier.
I volunteer to help in the new feature if you find it useful. I think it would
improve log history analysis and would enable developers to better organize old
code.
Thank you for your attention.
Best Regards,
Joao Pinto
^ permalink raw reply
* [ANNOUNCE] git-cinnabar 0.4.0
From: Mike Hommey @ 2017-01-18 9:22 UTC (permalink / raw)
To: git
Hi,
Git-cinnabar is a git remote helper to interact with mercurial
repositories. It allows to clone, pull and push from/to mercurial remote
repositories, using git.
Code on https://github.com/glandium/git-cinnabar
This release on
https://github.com/glandium/git-cinnabar/releases/tag/0.4.0
What's new since 0.3.2?
- Various bug fixes.
- Updated git to 2.11.0 for cinnabar-helper.
- Now supports bundle2 for both fetch/clone and push
(https://www.mercurial-scm.org/wiki/BundleFormat2).
- Now supports `git credential` for HTTP authentication.
- Now supports `git push --dry-run`.
- Added a new `git cinnabar fetch` command to fetch a specific revision
that is not necessarily a head.
- Added a new `git cinnabar download` command to download a helper on
platforms where one is available.
- Removed upgrade path from repositories used with version < 0.3.0.
- Experimental (and partial) support for using git-cinnabar without
having mercurial installed.
- Use a mercurial subprocess to access local mercurial repositories.
- Cinnabar-helper now handles fast-import, with workarounds for
performance issues on macOS.
- Fixed some corner cases involving empty files. This prevented cloning
Mozilla's stylo incubator repository.
- Fixed some correctness issues in file parenting when pushing
changesets pulled from one mercurial repository to another.
- Various improvements to the rules to build the helper.
- Experimental (and slow) support for pushing merges, with caveats. See
https://github.com/glandium/git-cinnabar/issues/20 for details about
the current status.
- Fail graft earlier when no commit was found to graft
- Allow graft to work with git version < 1.9
- Allow `git cinnabar bundle` to do the same grafting as git push
Mike
^ permalink raw reply
* Re: [PATCH] gitk: remove translated message from comments
From: Paul Mackerras @ 2017-01-18 10:15 UTC (permalink / raw)
To: David Aguilar; +Cc: Junio C Hamano, git
In-Reply-To: <20170118035245.1757-1-davvid@gmail.com>
On Tue, Jan 17, 2017 at 07:52:45PM -0800, David Aguilar wrote:
> "make update-po" fails because a previously untranslated string
> has now been translated:
>
> Updating po/sv.po
> po/sv.po:1388: duplicate message definition...
> po/sv.po:380: ...this is the location of the first definition
>
> Remove the duplicate message definition.
>
> Reported-by: Junio C Hamano <gitster@pobox.com>
> Signed-off-by: David Aguilar <davvid@gmail.com>
Thanks, applied.
Junio, please do a pull from my repository to get this fix.
The new head is 7f03c6e32891.
Paul.
^ permalink raw reply
* Re: [RFH - Tcl/Tk] use of procedure before declaration?
From: Konstantin Khomoutov @ 2017-01-18 6:43 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: Philip Oakley, Git List, Pat Thoyts
In-Reply-To: <alpine.DEB.2.20.1701171218260.3469@virtualbox>
On Tue, 17 Jan 2017 12:29:23 +0100 (CET)
Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:
> > In
> > https://github.com/git/git/blob/master/git-gui/lib/choose_repository.tcl#L242
> > the procedure `_unset_recentrepo` is called, however the procedure
> > isn't declared until line 248. My reading of the various Tcl
> > tutorials suggest (but not explictly) that this isn't the right way.
>
> Indeed, calling a procedure before it is declared sounds incorrect.
[...]
> And it is perfectly legitimate to use not-yet-declared procedures in
> other procedures, otherwise recursion would not work.
[...]
Sorry for chiming in too late, but I'd throw a bit of theory in.
Since Tcl is an interpreter (though it automatically compiles certain
stuff to bytecode as it goes through the script, and caches this
representation), everything is interpreted in the normal script order --
top to bottom as we usually see it in a text editor.
That is, there are simply no declaration vs definition: the main script
passed to tclsh / wish is read and interpreted from top to bottom;
as soon as it calls the [source] command, the specified script is read
and interpreted from top to bottom etc; after that the control is back
to the original script and its interpretation continues.
Hence when Tcl sees a command (everything it executes is a command; this
includes stuff like [proc], [foreach] and others, which are syntax in
other languages) it looks up this command in the current list of
commands it knows and this either succeeds or fails. The built-in
command [proc] defines a new Tcl procedure with the given name, and
registers it in that list of known commands.
So the general rule for user-defined procedures is relatively
straightforward: to call a procedure, the interpreter should have read
and executed its definition before the attempted call.
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox