* Re: segmentation fault (nullpointer) with git log --submodule -p
From: Junio C Hamano @ 2013-01-25 3:59 UTC (permalink / raw)
To: Jeff King
Cc: Duy Nguyen, Stefan Näwe, Armin, Jonathon Mah,
git@vger.kernel.org
In-Reply-To: <7va9ry87a0.fsf@alter.siamese.dyndns.org>
Junio C Hamano <gitster@pobox.com> writes:
> Jeff King <peff@peff.net> writes:
>
>> ... (e.g., how should "log" know that a submodule diff might later want
>> to see the same entry? Should we optimistically free and then make it
>> easier for the later user to reliably ensure the buffer is primed? Or
>> should we err on the side of keeping it in place?).
>
> My knee-jerk reaction is that we should consider that commit->buffer
> belongs to the revision traversal machinery. Any other uses bolted
> on later can borrow it if buffer still exists (I do not think pretty
> code rewrites the buffer contents in place in any way), or they can
> ask read_sha1_file() to read it themselves and free when they are
> done.
I've been toying with an idea along this line.
commit.h | 16 ++++++++++++++++
builtin/blame.c | 27 ++++++++-------------------
commit.c | 20 ++++++++++++++++++++
3 files changed, 44 insertions(+), 19 deletions(-)
diff --git a/commit.h b/commit.h
index c16c8a7..b559535 100644
--- a/commit.h
+++ b/commit.h
@@ -226,4 +226,20 @@ extern void print_commit_list(struct commit_list *list,
const char *format_cur,
const char *format_last);
+extern int ensure_commit_buffer(struct commit *);
+extern void discard_commit_buffer(struct commit *);
+
+#define with_commit_buffer(commit) \
+ do { \
+ int had_buffer_ = !!commit->buffer; \
+ if (!had_buffer_) \
+ ensure_commit_buffer(commit); \
+ do
+
+#define done_with_commit_buffer(commit) \
+ while (0); \
+ if (!had_buffer_) \
+ discard_commit_buffer(commit); \
+ } while (0)
+
#endif /* COMMIT_H */
diff --git a/builtin/blame.c b/builtin/blame.c
index b431ba3..8b2e4a5 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -1424,25 +1424,14 @@ static void get_commit_info(struct commit *commit,
commit_info_init(ret);
- /*
- * We've operated without save_commit_buffer, so
- * we now need to populate them for output.
- */
- if (!commit->buffer) {
- enum object_type type;
- unsigned long size;
- commit->buffer =
- read_sha1_file(commit->object.sha1, &type, &size);
- if (!commit->buffer)
- die("Cannot read commit %s",
- sha1_to_hex(commit->object.sha1));
- }
- encoding = get_log_output_encoding();
- reencoded = logmsg_reencode(commit, encoding);
- message = reencoded ? reencoded : commit->buffer;
- get_ac_line(message, "\nauthor ",
- &ret->author, &ret->author_mail,
- &ret->author_time, &ret->author_tz);
+ with_commit_buffer(commit) {
+ encoding = get_log_output_encoding();
+ reencoded = logmsg_reencode(commit, encoding);
+ message = reencoded ? reencoded : commit->buffer;
+ get_ac_line(message, "\nauthor ",
+ &ret->author, &ret->author_mail,
+ &ret->author_time, &ret->author_tz);
+ } done_with_commit_buffer(commit);
if (!detailed) {
free(reencoded);
diff --git a/commit.c b/commit.c
index e8eb0ae..a627eea 100644
--- a/commit.c
+++ b/commit.c
@@ -1357,3 +1357,23 @@ void print_commit_list(struct commit_list *list,
printf(format, sha1_to_hex(list->item->object.sha1));
}
}
+
+int ensure_commit_buffer(struct commit *commit)
+{
+ enum object_type type;
+ unsigned long size;
+
+ if (commit->buffer)
+ return 0;
+ commit->buffer = read_sha1_file(commit->object.sha1, &type, &size);
+ if (commit->buffer)
+ return -1;
+ else
+ return 0;
+}
+
+void discard_commit_buffer(struct commit *commit)
+{
+ free(commit->buffer);
+ commit->buffer = NULL;
+}
^ permalink raw reply related
* Re: [PATCH 1/7] sha1_file: keep track of where an SHA-1 object comes from
From: Duy Nguyen @ 2013-01-25 4:02 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Jens Lehmann
In-Reply-To: <7v4ni59ano.fsf@alter.siamese.dyndns.org>
On Fri, Jan 25, 2013 at 10:58 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Duy Nguyen <pclouds@gmail.com> writes:
>
>> On Fri, Jan 25, 2013 at 12:45 AM, Junio C Hamano <gitster@pobox.com> wrote:
>>>> How about this way instead: we keep track of where objects come from
>>>> so we can verify object source when we create or update something
>>>> that contains SHA-1.
>>>
>>> The overall approach taken by this series may be worth considering, but
>>> I do not think the check implemented here is correct.
>>>
>>> An object may be found in an alternate odb but we may also have our
>>> own copy of the same object. You need to prove that a suspicious
>>> object is visible to us *ONLY* through add_submodule_odb().
>>
>> The way alt odbs are linked (new odbs area always at the end), if we
>> have the same copy, their copy will never be read (we check out alt
>> odbs from head to tail). So those duplicate suspicious objects are
>> actually invisible to us.
>
> The way I read find_pack_entry() is that our heuristics to start
> our search by looking at the pack in which we found an object the
> last time will invalidate your assumption above. Am I mistaken?
No, you are right. Loose objects are always searched from the start of
alt odb list. Packs are searched till the end then back to head again.
--
Duy
^ permalink raw reply
* Re: segmentation fault (nullpointer) with git log --submodule -p
From: Jeff King @ 2013-01-25 4:08 UTC (permalink / raw)
To: Junio C Hamano
Cc: Duy Nguyen, Stefan Näwe, Armin, Jonathon Mah,
git@vger.kernel.org
In-Reply-To: <7vzjzx7w01.fsf@alter.siamese.dyndns.org>
On Thu, Jan 24, 2013 at 07:59:58PM -0800, Junio C Hamano wrote:
> Junio C Hamano <gitster@pobox.com> writes:
>
> > Jeff King <peff@peff.net> writes:
> >
> >> ... (e.g., how should "log" know that a submodule diff might later want
> >> to see the same entry? Should we optimistically free and then make it
> >> easier for the later user to reliably ensure the buffer is primed? Or
> >> should we err on the side of keeping it in place?).
> >
> > My knee-jerk reaction is that we should consider that commit->buffer
> > belongs to the revision traversal machinery. Any other uses bolted
> > on later can borrow it if buffer still exists (I do not think pretty
> > code rewrites the buffer contents in place in any way), or they can
> > ask read_sha1_file() to read it themselves and free when they are
> > done.
>
> I've been toying with an idea along this line.
>
> commit.h | 16 ++++++++++++++++
> builtin/blame.c | 27 ++++++++-------------------
> commit.c | 20 ++++++++++++++++++++
> 3 files changed, 44 insertions(+), 19 deletions(-)
I think we are on the same page as far as what needs to happen at the
call sites.
My suggested implementation had a separate buffer, but you are right
that we may need to actually set "commit->buffer" because sub-functions
expect to find it there (the alternative might be cleaning up the
sub-function interfaces). I haven't looked at the call-sites yet.
This:
> +extern int ensure_commit_buffer(struct commit *);
> +extern void discard_commit_buffer(struct commit *);
> +
> +#define with_commit_buffer(commit) \
> + do { \
> + int had_buffer_ = !!commit->buffer; \
> + if (!had_buffer_) \
> + ensure_commit_buffer(commit); \
> + do
> +
> +#define done_with_commit_buffer(commit) \
> + while (0); \
> + if (!had_buffer_) \
> + discard_commit_buffer(commit); \
> + } while (0)
is pretty nasty, though. I know it gets the job done, but in my
experience, macros which do not behave syntactically like functions are
usually a good sign that you are doing something gross and
unmaintainable.
I dunno.
-Peff
^ permalink raw reply
* Re: [PATCH] t9902: protect test from stray build artifacts
From: Junio C Hamano @ 2013-01-25 4:11 UTC (permalink / raw)
To: Jeff King; +Cc: Jean-Noël AVILA, git
In-Reply-To: <20130125011349.GB27657@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> This looks good to me.
>
> The only thing I might add is a test just to double-check that "git help
> -a" is parsed correctly. Like:
>
> test_expect_success 'command completion works without test harness' '
> GIT_TESTING_COMMAND_COMPLETION= run_completion "git bun" &&
> grep "^bundle\$" out
> '
>
> (we know we are running bash here, so the one-shot variable is OK to be
> used with a function).
I think you meant "^bundle $" there, but don't we have the same
problem when there is an end-user subcommand "git bunny"?
Ahh, ok, we show one element per line and just make sure "bundle"
is there, and we do not care what other buns appear in the output.
Will squash in, then.
^ permalink raw reply
* Re: [PATCH] t9902: protect test from stray build artifacts
From: Jeff King @ 2013-01-25 4:13 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jean-Noël AVILA, git
In-Reply-To: <7vvcal7vhg.fsf@alter.siamese.dyndns.org>
On Thu, Jan 24, 2013 at 08:11:07PM -0800, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
>
> > This looks good to me.
> >
> > The only thing I might add is a test just to double-check that "git help
> > -a" is parsed correctly. Like:
> >
> > test_expect_success 'command completion works without test harness' '
> > GIT_TESTING_COMMAND_COMPLETION= run_completion "git bun" &&
> > grep "^bundle\$" out
> > '
> >
> > (we know we are running bash here, so the one-shot variable is OK to be
> > used with a function).
>
> I think you meant "^bundle $" there, but don't we have the same
> problem when there is an end-user subcommand "git bunny"?
>
> Ahh, ok, we show one element per line and just make sure "bundle"
> is there, and we do not care what other buns appear in the output.
Exactly. At least that was the intent; I typed it straight into my MUA. :)
-Peff
^ permalink raw reply
* Re: [PATCH] t9902: protect test from stray build artifacts
From: Junio C Hamano @ 2013-01-25 4:19 UTC (permalink / raw)
To: Jeff King; +Cc: Jean-Noël AVILA, git
In-Reply-To: <7vvcal7vhg.fsf@alter.siamese.dyndns.org>
Junio C Hamano <gitster@pobox.com> writes:
> Jeff King <peff@peff.net> writes:
>
>> This looks good to me.
>>
>> The only thing I might add is a test just to double-check that "git help
>> -a" is parsed correctly. Like:
>>
>> test_expect_success 'command completion works without test harness' '
>> GIT_TESTING_COMMAND_COMPLETION= run_completion "git bun" &&
>> grep "^bundle\$" out
>> '
>>
>> (we know we are running bash here, so the one-shot variable is OK to be
>> used with a function).
>
> I think you meant "^bundle $" there, but don't we have the same
> problem when there is an end-user subcommand "git bunny"?
>
> Ahh, ok, we show one element per line and just make sure "bundle"
> is there, and we do not care what other buns appear in the output.
>
> Will squash in, then.
Not so quick, though. The lower level "read from help -a" is only
run once and its output kept in a two-level cache hierarchy; we need
to reset both.
It starts to look a bit too intimately tied to the implementation of
what is being tested for my taste, though.
t/t9902-completion.sh | 10 ++++++++++
1 file changed, 10 insertions(+)
diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
index adc1372..bb6ee1a 100755
--- a/t/t9902-completion.sh
+++ b/t/t9902-completion.sh
@@ -286,4 +286,14 @@ test_expect_success 'send-email' '
test_completion "git send-email ma" "master "
'
+# This is better to be at the end, as it resets the state by tweaking
+# the internals.
+test_expect_success 'help -a read correctly by command list generator' '
+ __git_all_commands= &&
+ __git_porcelain_commands= &&
+ GIT_TESTING_COMMAND_COMPLETION= &&
+ run_completion "git bun" &&
+ grep "^bundle $" out
+'
+
test_done
--
1.8.1.1.525.gdace530
^ permalink raw reply related
* Re: segmentation fault (nullpointer) with git log --submodule -p
From: Junio C Hamano @ 2013-01-25 4:21 UTC (permalink / raw)
To: Jeff King
Cc: Duy Nguyen, Stefan Näwe, Armin, Jonathon Mah,
git@vger.kernel.org
In-Reply-To: <20130125040856.GA30533@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> ... I know it gets the job done, but in my
> experience, macros which do not behave syntactically like functions are
> usually a good sign that you are doing something gross and
> unmaintainable.
Yeah, it is a bit too cute for my taste, too, even though it was fun
;-)
^ permalink raw reply
* Re: [PATCH] t9902: protect test from stray build artifacts
From: Jeff King @ 2013-01-25 4:23 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jean-Noël AVILA, git
In-Reply-To: <7vr4l97v3h.fsf@alter.siamese.dyndns.org>
On Thu, Jan 24, 2013 at 08:19:30PM -0800, Junio C Hamano wrote:
> > Ahh, ok, we show one element per line and just make sure "bundle"
> > is there, and we do not care what other buns appear in the output.
> >
> Not so quick, though. The lower level "read from help -a" is only
> run once and its output kept in a two-level cache hierarchy; we need
> to reset both.
Ugh, I didn't even think about that.
I wonder if it would be simpler if the completion tests actually ran a
new bash for each test. That would be slower, but it somehow seems
cleaner.
> It starts to look a bit too intimately tied to the implementation of
> what is being tested for my taste, though.
> [...]
> +test_expect_success 'help -a read correctly by command list generator' '
> + __git_all_commands= &&
> + __git_porcelain_commands= &&
> + GIT_TESTING_COMMAND_COMPLETION= &&
> + run_completion "git bun" &&
> + grep "^bundle $" out
> +'
Agreed. I could take or leave it at this point. It's nice to check that
changes to "help -a" will not break it, but ultimately it feels a bit
too contrived to catch anything useful.
-Peff
^ permalink raw reply
* Re: [PATCH v4 0/3] Finishing touches to "push" advises
From: Chris Rorvick @ 2013-01-25 4:31 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Jeff King
In-Reply-To: <1358978130-12216-1-git-send-email-gitster@pobox.com>
On Wed, Jan 23, 2013 at 3:55 PM, Junio C Hamano <gitster@pobox.com> wrote:
> This builds on Chris Rorvick's earlier effort to forbid unforced
> updates to refs/tags/ hierarchy and giving sensible error and advise
> messages for that case (we are not rejecting such a push due to fast
> forwardness, and suggesting to fetch and integrate before pushing
> again does not make sense).
FWIW, these changes look good to me. The logic in
set_ref_status_for_push() is easier to follow and the additional error
statuses (and associated advice) make things much clearer.
Had I written the the "already exists" advice in the context of these
additional statuses I would have said "the destination *tag* reference
already exists", or maybe even just "the destination *tag* already
exists". It's probably fine the way it is, but I only avoided using
"tag" in the advice because I was abusing it.
Thanks,
Chris
^ permalink raw reply
* Git 1.8.2 l10n round 1
From: Jiang Xin @ 2013-01-25 4:54 UTC (permalink / raw)
To: Byrial Jensen, Ralf Thielow,
Ævar Arnfjörð Bjarmason, Marco Paolone,
Vincent van Ravesteijn, Marco Sousa, Peter Krefting,
Trần Ngọc Quân, David Hrbáč
Cc: Git List
Dear l10n team members,
New "git.pot" is generated from v1.8.1-476-gec3ae6e in the master branch.
l10n: Update git.pot (11 new, 7 removed messages)
Generate po/git.pot from v1.8.1-476-gec3ae6e, and there are 11 new and 7
removed messages.
Signed-off-by: Jiang Xin <worldhello.net@gmail.com>
This update is for the l10n of upcoming git 1.8.2. You can get it from
the usual place:
https://github.com/git-l10n/git-po/
--
Jiang Xin
^ permalink raw reply
* Re: [PATCH v3 01/10] wildmatch: fix "**" special case
From: Junio C Hamano @ 2013-01-25 4:55 UTC (permalink / raw)
To: Duy Nguyen; +Cc: git
In-Reply-To: <CACsJy8Bq8aQ69twWtwHyNzez2OwTN1wHxqHwb_AM-Qo4TUuv8Q@mail.gmail.com>
Duy Nguyen <pclouds@gmail.com> writes:
> On Fri, Jan 25, 2013 at 1:22 AM, Junio C Hamano <gitster@pobox.com> wrote:
>> Duy Nguyen <pclouds@gmail.com> writes:
>>
>>> I don't think we need to support two different sets of wildcards in
>>> the long run. I'm thinking of adding ":(glob)" with WM_PATHNAME.
>>> Pathspec without :(glob) still uses the current wildcards (i.e. no
>>> FNM_PATHNAME). At some point, like 2.0, we either switch the behavior
>>> of patterns-without-:(glob) to WM_PATHNAME, or just disable wildcards
>>> when :(glob) is not present.
>>
>> Yeah, I think that is sensible.
>>
>> I am meaning to merge your retire-fnmatch topic to 'master'.
>
> I thought you wanted it to stay in 'next' longer :-)
I only said "a bit longer than other topics", and the topic has been
cooking on 'next' for three weeks by now, which is a lot longer. I
haven't been keeping exact tallies, but trivial ones graduate from
'next' to 'master' in 3 to 5 days, ones with medium complexity in 7
to 10 days on average, I think.
> That looks ok. You may want to mention that "**" syntax is enabled in
> .gitignore and .gitattributes as well (even without USE_WILDMATCH).
Yeah, I forgot about that behaviour, and it is a bit confusing
story. You did that in 237ec6e (Support "**" wildcard in .gitignore
and .gitattributes, 2012-10-15).
c41244e (wildmatch: support "no FNM_PATHNAME" mode, 2013-01-01) that
is part of the retire-fnmatch topic, changes the behaviour of
wildmatch() with respect to /**/ magic drastically, but everything
cancels out in the end:
- With the current master without nd/retire-fnmatch, wildmatch()
always works in WM_PATHNAME mode wrt '/**/'; match_pathname()
that is used for attr/ignore matching uses wildmatch() so a
pattern "**/Makefile" matches "Makefile" at the top, affected by
the "**/" magic;
- After merging nd/retire-fnmatch, wildmatch() needs WM_PATHNAME
passed in order to grok '/**/' magic, but match_pathname() is
changed in the same commit to pass WM_PATHNAME, so the "**/"
magic is retained for ignore/attr matching.
> We
> could even stop the behavior change in for-each-ref (FNM_PATHNAME in
> general) but I guess because it's a nice regression, nobody would
> complain.
That is not a "regression" (whose definition is "we inadvertently
changed the behaviour and fixed that once, but the breakage came
back"). It is a behaviour change that is backward incompatible.
I would agree that it is a nice behaviour change, though ;-)
Thanks.
^ permalink raw reply
* Re: What's cooking in git.git (Jan 2013, #08; Tue, 22)
From: Chris Rorvick @ 2013-01-25 4:55 UTC (permalink / raw)
To: John Keeping; +Cc: Junio C Hamano, git, Eric S. Raymond
In-Reply-To: <20130123211237.GR7498@serenity.lan>
On Wed, Jan 23, 2013 at 3:12 PM, John Keeping <john@keeping.me.uk> wrote:
> The existing script (git-cvsimport.perl) won't ever work with cvsps-3
> since features it relies on have been removed.
Not reporting the ancestry branch seems to be the big one. Are there
others? I had a version of the Perl script sort of working, but only
well enough to pass the t9600 and t9604 tests.
Chris
^ permalink raw reply
* Re: [PATCH] Update renamed file merge-file.h in Makefile
From: Jiang Xin @ 2013-01-25 4:59 UTC (permalink / raw)
To: Git List
In-Reply-To: <1359083188-31866-1-git-send-email-worldhello.net@gmail.com>
Oops, I find it is already fixed in commit
a60521bc6099ce89d05ef2160d2e3c30a106fda7.
commit a60521bc6099ce89d05ef2160d2e3c30a106fda7
Author: Ramsay Jones <ramsay@ramsay1.demon.co.uk>
Date: Tue Jan 22 16:47:47 2013 +0000
Makefile: Replace merge-file.h with merge-blobs.h in LIB_H
Commit fa2364ec ("Which merge_file() function do you mean?", 06-12-2012)
renamed the files merge-file.[ch] to merge-blobs.[ch], but forgot to
rename the header file in the definition of the LIB_H macro.
Signed-off-by: Ramsay Jones <ramsay@ramsay1.demon.co.uk>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
(kick back to the list)
2013/1/25 Jiang Xin <worldhello.net@gmail.com>:
> Commit v1.8.1-rc0-3-gfa2364e renamed merge-file.h to merge-blobs.h, but
> forgot to update the reference of merge-file.h in Makefile. This would
> break the build of "po/git.pot", which depends on $(LOCALIZED_C), then
> fallback to the missing file "merge-file.h".
>
> Signed-off-by: Jiang Xin <worldhello.net@gmail.com>
> ---
> Makefile | 2 +-
> 1 个文件被修改，插入 1 行(+)，删除 1 行(-)
>
> diff --git a/Makefile b/Makefile
> index 1b30d7b..a786d4c 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -649,7 +649,7 @@ LIB_H += list-objects.h
> LIB_H += ll-merge.h
> LIB_H += log-tree.h
> LIB_H += mailmap.h
> -LIB_H += merge-file.h
> +LIB_H += merge-blobs.h
> LIB_H += merge-recursive.h
> LIB_H += mergesort.h
> LIB_H += notes-cache.h
> --
> 1.8.1.1
>
^ permalink raw reply
* Re: [PATCH v4 0/3] Finishing touches to "push" advises
From: Junio C Hamano @ 2013-01-25 5:04 UTC (permalink / raw)
To: Chris Rorvick; +Cc: git, Jeff King
In-Reply-To: <CAEUsAPYAikZUTf9OE=PoGBYot6Udnw9XTYDs6Ug7h=PWbCYM1Q@mail.gmail.com>
Chris Rorvick <chris@rorvick.com> writes:
> Had I written the the "already exists" advice in the context of these
> additional statuses I would have said "the destination *tag* reference
> already exists", or maybe even just "the destination *tag* already
> exists".
Yeah, now we do not use "already exists" for anything other than
refs/tags/, right? Your rewording sounds like the right thing to
make it even clearer.
Thanks for bringing it up.
Would it be sufficient to do this? I think "the tag already exists
in the remote" is already clear that we are talking about the
destination.
diff --git a/builtin/push.c b/builtin/push.c
index a2b3fbe..78789be 100644
--- a/builtin/push.c
+++ b/builtin/push.c
@@ -228,7 +228,7 @@ static const char message_advice_ref_fetch_first[] =
"See the 'Note about fast-forwards' in 'git push --help' for details.");
static const char message_advice_ref_already_exists[] =
- N_("Updates were rejected because the destination reference already exists\n"
+ N_("Updates were rejected because the tag already exists\n"
"in the remote.");
static const char message_advice_ref_needs_force[] =
^ permalink raw reply related
* Re: [PATCH v4 0/3] Finishing touches to "push" advises
From: Chris Rorvick @ 2013-01-25 5:14 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Jeff King
In-Reply-To: <7va9rx7t0e.fsf@alter.siamese.dyndns.org>
On Thu, Jan 24, 2013 at 11:04 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Would it be sufficient to do this? I think "the tag already exists
> in the remote" is already clear that we are talking about the
> destination.
Good point.
> diff --git a/builtin/push.c b/builtin/push.c
> index a2b3fbe..78789be 100644
> --- a/builtin/push.c
> +++ b/builtin/push.c
> @@ -228,7 +228,7 @@ static const char message_advice_ref_fetch_first[] =
> "See the 'Note about fast-forwards' in 'git push --help' for details.");
>
> static const char message_advice_ref_already_exists[] =
> - N_("Updates were rejected because the destination reference already exists\n"
> + N_("Updates were rejected because the tag already exists\n"
> "in the remote.");
>
> static const char message_advice_ref_needs_force[] =
Looks like the new-line is now unnecessary, but that looks good to me.
Thanks,
Chris
^ permalink raw reply
* Re: [PATCH 1/3] mergetool--lib: fix startup options for gvimdiff tool
From: David Aguilar @ 2013-01-25 5:07 UTC (permalink / raw)
To: Alexey Shumkin; +Cc: git, Pat Thoyts, Junio C Hamano
In-Reply-To: <1359011768-7665-1-git-send-email-Alex.Crezoff@gmail.com>
On Wed, Jan 23, 2013 at 11:16 PM, Alexey Shumkin <alex.crezoff@gmail.com> wrote:
> Options are taken from <Git source>/mergetools/vim
>
> Signed-off-by: Alexey Shumkin <Alex.Crezoff@gmail.com>
> ---
> git-gui/lib/mergetool.tcl | 8 +++++++-
> 1 file changed, 7 insertions(+), 1 deletion(-)
A better long-term solution might be to teach git gui to use "git difftool".
Would it be better to teach git-gui (and gitk) about mergetool/difftool?
That would allow us to possibly eliminate this duplication.
We did start towards that path when difftool learned the --extcmd
option (for use by gitk) but I have not followed through.
What do you think about trying that approach?
> diff --git a/git-gui/lib/mergetool.tcl b/git-gui/lib/mergetool.tcl
> index 3c8e73b..4fc1cab 100644
> --- a/git-gui/lib/mergetool.tcl
> +++ b/git-gui/lib/mergetool.tcl
> @@ -211,7 +211,13 @@ proc merge_resolve_tool2 {} {
> }
> }
> gvimdiff {
> - set cmdline [list "$merge_tool_path" -f "$LOCAL" "$MERGED" "$REMOTE"]
> + if {$base_stage ne {}} {
> + set cmdline [list "$merge_tool_path" -f -d -c "wincmd J" \
> + "$MERGED" "$LOCAL" "$BASE" "$REMOTE"]
> + } else {
> + set cmdline [list "$merge_tool_path" -f -d -c "wincmd l" \
> + "$LOCAL" "$MERGED" "$REMOTE"]
> + }
> }
> kdiff3 {
> if {$base_stage ne {}} {
> --
> 1.8.1.1.10.g9255f3f
>
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
David
^ permalink raw reply
* Re: [PATCH 4/4] git-difftool: use git-mergetool--lib for "--tool-help"
From: David Aguilar @ 2013-01-25 5:29 UTC (permalink / raw)
To: John Keeping; +Cc: git
In-Reply-To: <b791e866c02b0c118f08bde1d7ca6c41d6239989.1359057056.git.john@keeping.me.uk>
On Thu, Jan 24, 2013 at 11:55 AM, John Keeping <john@keeping.me.uk> wrote:
> The "--tool-help" option to git-difftool currently displays incorrect
> output since it uses the names of the files in
> "$GIT_EXEC_PATH/mergetools/" rather than the list of command names in
> git-mergetool--lib.
>
> Fix this by simply delegating the "--tool-help" argument to the
> show_tool_help function in git-mergetool--lib.
Very nice.
One thought I had was that the unified show_tool_help should
probably check TOOL_MODE=diff and skip over the
!can_diff entries.
The current output of "git difftool --tool-help" before your
patches has the problem that it will list tools such as
"tortoisemerge" as "valid but not available" because it
does not differentiate between missing and !can_diff.
> diff --git a/git-difftool.perl b/git-difftool.perl
> index edd0493..0a90de4 100755
> --- a/git-difftool.perl
> +++ b/git-difftool.perl
> @@ -59,57 +59,16 @@ sub find_worktree
> return $worktree;
> }
>
> -sub filter_tool_scripts
> -{
> - my ($tools) = @_;
> - if (-d $_) {
> - if ($_ ne ".") {
> - # Ignore files in subdirectories
> - $File::Find::prune = 1;
> - }
> - } else {
> - if ((-f $_) && ($_ ne "defaults")) {
> - push(@$tools, $_);
> - }
> - }
> -}
> -
> sub print_tool_help
> {
> - my ($cmd, @found, @notfound, @tools);
> - my $gitpath = Git::exec_path();
> -
> - find(sub { filter_tool_scripts(\@tools) }, "$gitpath/mergetools");
> -
> - foreach my $tool (@tools) {
> - $cmd = "TOOL_MODE=diff";
> - $cmd .= ' && . "$(git --exec-path)/git-mergetool--lib"';
> - $cmd .= " && get_merge_tool_path $tool >/dev/null 2>&1";
> - $cmd .= " && can_diff >/dev/null 2>&1";
> - if (system('sh', '-c', $cmd) == 0) {
> - push(@found, $tool);
> - } else {
> - push(@notfound, $tool);
> - }
> - }
This is where we conflated can_diff with "is the tool available?".
The nice thing is that the old code did actually check can_diff,
but since it conflated these things it ended up putting them
in the @notfound list. It should ignore those completely, IMO.
That could be a nice follow-up patch. What do you think?
--
David
^ permalink raw reply
* Re: segmentation fault (nullpointer) with git log --submodule -p
From: Jonathan Nieder @ 2013-01-25 5:53 UTC (permalink / raw)
To: Junio C Hamano
Cc: Jeff King, Duy Nguyen, Stefan Näwe, Armin, Jonathon Mah,
git@vger.kernel.org
In-Reply-To: <7vzjzx7w01.fsf@alter.siamese.dyndns.org>
Hi,
Junio C Hamano wrote:
> I've been toying with an idea along this line.
Heh. Just for fun, here's an uglier version:
struct wcb_data {
int had_buffer;
int using_buffer;
};
#define WITH_COMMIT_BUFFER_DATA_INIT { 0, 0 }
extern void acquire_commit_buffer(struct commit *, struct wcb_data *);
extern void done_with_commit_buffer(struct commit *, struct wcb_data *);
/*
* usage:
* struct wcb_data buf = WITH_COMMIT_BUFFER_INIT;
*
* with_commit_buffer(commit, buf) {
* ...
* }
*/
#define with_commit_buffer(commit, i) \
for (acquire_commit_buffer(commit, &i); \
i.using_buffer; \
done_with_commit_buffer(commit, &i))
void acquire_commit_buffer(struct commit *commit, struct wcb_data *i)
{
enum object_type type;
unsigned long size;
assert(!i->using_buffer);
i->using_buffer = 1;
i->had_buffer = !!commit->buffer;
if (i->had_buffer)
return;
commit->buffer = read_sha1_file(commit->object.sha1, &type, &size);
if (!commit->buffer)
die("unable to read commit %s", sha1_to_hex(commit->object.sha1));
}
void done_with_commit_buffer(struct commit *commit, struct wcb_data *i)
{
assert(i->using_buffer);
i->using_buffer = 0;
if (!i->had_buffer) {
free(commit->buffer);
commit->buffer = NULL;
}
}
^ permalink raw reply
* [PATCH] .gitignore: ignore generated gitk-git/gitk-wish
From: Nguyễn Thái Ngọc Duy @ 2013-01-25 6:09 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Nguyễn Thái Ngọc Duy
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
.gitignore | 1 +
1 file changed, 1 insertion(+)
diff --git a/.gitignore b/.gitignore
index aa258a6..63d4904 100644
--- a/.gitignore
+++ b/.gitignore
@@ -171,6 +171,7 @@
/git-whatchanged
/git-write-tree
/git-core-*/?*
+/gitk-git/gitk-wish
/gitweb/GITWEB-BUILD-OPTIONS
/gitweb/gitweb.cgi
/gitweb/static/gitweb.js
--
1.8.0.rc3.18.g0d9b108
^ permalink raw reply related
* Re: [PATCH] mergetools: Add tortoisegitmerge helper
From: David Aguilar @ 2013-01-25 6:11 UTC (permalink / raw)
To: Sven Strickroth; +Cc: Junio C Hamano, git, Sebastian Schuberth, Jeff King
In-Reply-To: <7vpq0u8bxd.fsf@alter.siamese.dyndns.org>
On Thu, Jan 24, 2013 at 2:15 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Sven Strickroth <sven.strickroth@tu-clausthal.de> writes:
>
>> Am 24.01.2013 20:51 schrieb Junio C Hamano:
>>> Sven Strickroth <sven.strickroth@tu-clausthal.de> writes:
>>>
>>>> - The TortoiseGit team renamed TortoiseMerge.exe to TortoiseGitMerge.exe
>>>> (starting with 1.8.0) in order to make clear that this one has special
>>>> support for git and prevent confusion with the TortoiseSVN TortoiseMerge
>>>> version.
>>>
>>> Wouldn't it make more sense in such a situation if your users can
>>> keep using the old "tortoisemerge" configured in their configuration
>>> and when the renamed one is found the mergetool automatically used
>>> it, rather than the way your patch is done?
>>
>> That was also my first idea, however, TortoiseMerge uses parameters as
>> follows: '-base:"$BASE"'. TortoiseGitMerge uses values separated by
>> space from keys: '-base "$BASE"'. So both are incompatible (the first
>> approach has problems with spaces in filenames, the TortoiseGitMerge
>> approach fixes this).
>
> OK. Please unconfuse future readers of "git log" by saying why such
> a unification does not work in the proposed log message.
Even though the old tortoisemerge and the new tortoisegitmerge
have completely different syntax, could we still use the existence
of one when deciding which syntax to use?
pseudo-code at the top of the scriptlet:
if test -z "$tortoisegitmerge"
then
if type tortoisegitmerge 2>&1 >/dev/null
then
tortoisegitmerge=true
else
tortoisegitmerge=false
fi
fi
...and then later merge_cmd and diff_cmd
can delegate to {diff,merge}_cmd_legacy() and
{diff,merge}_cmd_gitmerge() functions to do the work.
It's just a thought. translate_merge_tool_path()
is too low-level to do it, but it seems like we could
get away with it by having some extra smarts in the
scriptlet.
>>>> diff --git a/Documentation/diff-config.txt b/Documentation/diff-config.txt
>>>> index 4314ad0..13cbe5b 100644
>>>> --- a/Documentation/diff-config.txt
>>>> +++ b/Documentation/diff-config.txt
>>>> @@ -151,7 +151,7 @@ diff.<driver>.cachetextconv::
>>>> diff.tool::
>>>> The diff tool to be used by linkgit:git-difftool[1]. This
>>>> option overrides `merge.tool`, and has the same valid built-in
>>>> - values as `merge.tool` minus "tortoisemerge" and plus
>>>> - "kompare". Any other value is treated as a custom diff tool,
>>>> + values as `merge.tool` minus "tortoisemerge"/"tortoisegitmerge" and
>>>> + plus "kompare". Any other value is treated as a custom diff tool,
>>>> and there must be a corresponding `difftool.<tool>.cmd`
>>>> option.
>>>
>>> So in short, two tortoises and kompare are only valid as mergetool
>>> but cannot be used as difftool? No, I am reading it wrong.
>>> merge.tool can be used for both, kompare can be used as difftool,
>>> and two tortoises can only be used as mergetool.
>>>
>>> This paragraph needs to be rewritten to unconfuse readers. The
>>> original is barely intelligible, and it becomes unreadable as the
>>> set of tools subtracted by "minus" and added by "plus" grows.
>>
>> But I think this should not be part of this patch.
>
> I agree that it can be done (and it is better to be done) as a
> preparatory step. The current text is barely readable, but with
> this patch there will be two "minus", and the result becomes
> unreadable at that point.
>
> It also could be done as a follow-up documentation readability fix.
Another thought would be to minimize this section as much
as possible and point users to "git difftool --tool-help".
We can then improve --tool-help (there are already preliminary
patches in-flight to do so) so that we do not have to maintain
this documentation in the future.
Likewise, if we are able to teach the scriptlet to choose
the best one (and not require a new scriptlet) then this
section could be left as-is for this patch.
--
David
^ permalink raw reply
* Re: [PATCH] mergetools: Add tortoisegitmerge helper
From: Junio C Hamano @ 2013-01-25 7:21 UTC (permalink / raw)
To: David Aguilar; +Cc: Sven Strickroth, git, Sebastian Schuberth, Jeff King
In-Reply-To: <CAJDDKr5O70tTfwuipWcYVJL6gM3bUyQh-22yVO89xn8OFsQOpw@mail.gmail.com>
David Aguilar <davvid@gmail.com> writes:
> Even though the old tortoisemerge and the new tortoisegitmerge
> have completely different syntax, could we still use the existence
> of one when deciding which syntax to use?
> ...
> ...and then later merge_cmd and diff_cmd
> can delegate to {diff,merge}_cmd_legacy() and
> {diff,merge}_cmd_gitmerge() functions to do the work.
>
> It's just a thought. translate_merge_tool_path()
> is too low-level to do it, but it seems like we could
> get away with it by having some extra smarts in the
> scriptlet.
Sounds like a far better approach to me. I'd like to at least see
an attempt be made to make that work first.
>>>> This paragraph needs to be rewritten to unconfuse readers. The
>>>> original is barely intelligible, and it becomes unreadable as the
>>>> set of tools subtracted by "minus" and added by "plus" grows.
>>>
>>> But I think this should not be part of this patch.
>>
>> I agree that it can be done (and it is better to be done) as a
>> preparatory step. The current text is barely readable, but with
>> this patch there will be two "minus", and the result becomes
>> unreadable at that point.
>>
>> It also could be done as a follow-up documentation readability fix.
>
> Another thought would be to minimize this section as much
> as possible and point users to "git difftool --tool-help".
We had a similar discussion here:
http://thread.gmane.org/gmane.comp.version-control.git/201913/focus=201976
and Documentation/git-{diff,merge}tool.txt have stayed quiet since
then.
But Documentation/merge-config.txt tries to list everything that _could_
be enabled, and I do not necessarily think having one single
location that lists everything is such a bad idea.
Is there a way for me to programatically tell what merge.tool and
diff.tool could be enabled for a particular source checkout of Git
regardless of what platform am I on (that is, even though I won't
touch Windows, I want to see 'tortoise' appear in the output of such
a procedure)? We could generate a small text file from the Makefile
in Documentation and include it when building the manual pages if
such a procedure is available.
^ permalink raw reply
* Re: segmentation fault (nullpointer) with git log --submodule -p
From: Junio C Hamano @ 2013-01-25 7:27 UTC (permalink / raw)
To: Jonathan Nieder
Cc: Jeff King, Duy Nguyen, Stefan Näwe, Armin, Jonathon Mah,
git@vger.kernel.org
In-Reply-To: <20130125055331.GC26524@elie.Belkin>
Jonathan Nieder <jrnieder@gmail.com> writes:
> Hi,
>
> Junio C Hamano wrote:
>
>> I've been toying with an idea along this line.
>
> Heh. Just for fun, here's an uglier version:
Much nicer, though.
>
> struct wcb_data {
> int had_buffer;
> int using_buffer;
> };
> #define WITH_COMMIT_BUFFER_DATA_INIT { 0, 0 }
>
> extern void acquire_commit_buffer(struct commit *, struct wcb_data *);
> extern void done_with_commit_buffer(struct commit *, struct wcb_data *);
>
> /*
> * usage:
> * struct wcb_data buf = WITH_COMMIT_BUFFER_INIT;
> *
> * with_commit_buffer(commit, buf) {
> * ...
> * }
> */
> #define with_commit_buffer(commit, i) \
> for (acquire_commit_buffer(commit, &i); \
> i.using_buffer; \
> done_with_commit_buffer(commit, &i))
>
> void acquire_commit_buffer(struct commit *commit, struct wcb_data *i)
> {
> enum object_type type;
> unsigned long size;
>
> assert(!i->using_buffer);
> i->using_buffer = 1;
> i->had_buffer = !!commit->buffer;
>
> if (i->had_buffer)
> return;
> commit->buffer = read_sha1_file(commit->object.sha1, &type, &size);
> if (!commit->buffer)
> die("unable to read commit %s", sha1_to_hex(commit->object.sha1));
> }
>
> void done_with_commit_buffer(struct commit *commit, struct wcb_data *i)
> {
> assert(i->using_buffer);
> i->using_buffer = 0;
>
> if (!i->had_buffer) {
> free(commit->buffer);
> commit->buffer = NULL;
> }
> }
^ permalink raw reply
* Re: segmentation fault (nullpointer) with git log --submodule -p
From: Jonathon Mah @ 2013-01-25 7:32 UTC (permalink / raw)
To: Jonathan Nieder
Cc: Junio C Hamano, Jeff King, Duy Nguyen, Stefan Näwe, Armin,
git@vger.kernel.org
In-Reply-To: <20130125055331.GC26524@elie.Belkin>
Just to note, the proposals so far don't prevent a "smart-ass" function from freeing the buffer when it's called underneath the use/release scope, as in:
with_commit_buffer(commit); {
fn1_needing_buffer(commit);
walk_rev_tree_or_something();
fn2_needing_buffer(commit);
} done_with_commit_buffer(commit);
walk_rev_tree_or_something() might need to read commits to do its thing, and it could still choose to free their buffers (as in rev-list.c finish_commit()). If those commits includes the one being "retained", the call to fn2 will still see NULL despite it being in a 'protected scope'.
Are the objections to using a reference count?
Jonathon Mah
me@JonathonMah.com
^ permalink raw reply
* Re: [PATCH] mergetools: Add tortoisegitmerge helper
From: David Aguilar @ 2013-01-25 7:54 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Sven Strickroth, git, Sebastian Schuberth, Jeff King
In-Reply-To: <7vvcal683y.fsf@alter.siamese.dyndns.org>
On Thu, Jan 24, 2013 at 11:21 PM, Junio C Hamano <gitster@pobox.com> wrote:
> David Aguilar <davvid@gmail.com> writes:
>>>>> This paragraph needs to be rewritten to unconfuse readers. The
>>>>> original is barely intelligible, and it becomes unreadable as the
>>>>> set of tools subtracted by "minus" and added by "plus" grows.
>>>>
>>>> But I think this should not be part of this patch.
>>>
>>> I agree that it can be done (and it is better to be done) as a
>>> preparatory step. The current text is barely readable, but with
>>> this patch there will be two "minus", and the result becomes
>>> unreadable at that point.
>>>
>>> It also could be done as a follow-up documentation readability fix.
>>
>> Another thought would be to minimize this section as much
>> as possible and point users to "git difftool --tool-help".
>
> We had a similar discussion here:
>
> http://thread.gmane.org/gmane.comp.version-control.git/201913/focus=201976
>
> and Documentation/git-{diff,merge}tool.txt have stayed quiet since
> then.
>
> But Documentation/merge-config.txt tries to list everything that _could_
> be enabled, and I do not necessarily think having one single
> location that lists everything is such a bad idea.
>
> Is there a way for me to programatically tell what merge.tool and
> diff.tool could be enabled for a particular source checkout of Git
> regardless of what platform am I on (that is, even though I won't
> touch Windows, I want to see 'tortoise' appear in the output of such
> a procedure)? We could generate a small text file from the Makefile
> in Documentation and include it when building the manual pages if
> such a procedure is available.
That's a good idea.
Here's one way... (typed into gmail, so probably broken)
LF='
'
mergetools=
difftools=
scriptlets="$(git --exec-path)"/mergetools
for script in "$scriptlets"/*
do
tool="$(basename "$script")"
if test "$tool" = "defaults"
then
continue
fi
. "$scriptlets"/defaults
can_diff && difftools="$difftools$tool$LF"
can_merge && mergetools="$mergetools$tool$LF"
done
I can follow up with a Documentation patch along these lines.
I'm would imagine it would be hooked up similarly to how the
command lists are constructed.
This should allow the tortoisemerge improvements to happen independently.
--
David
^ permalink raw reply
* Separately repository access in GitWeb
From: Андрей Баранов @ 2013-01-25 8:10 UTC (permalink / raw)
To: git
good day.
I`m trying make separately repository access in GitWeb by NGINX
separation of access based on URL strings, namely, the presence of the
query strings:
'?p=repo.git'
with a regular expression:
"^.*p=(.*?)(\.git|;|&|=|\s).*$"
I am wondering how much it is correct to protect against unauthorized access.
Thanks in advance :)
complete example of a configuration file:
server {
listen 80;
root /home/git/gitweb;
access_log /var/log/nginx/gitweb.access_log main;
error_log /var/log/nginx/gitweb.error_log info;
index gitweb.cgi;
gzip off;
location ~* \.(jpg|txt|jpeg|gif|png|ico|css|zip|js|swf)$ {
access_log off;
expires 1d;
}
location = / {
set $htpasswd "opened@";
if ($args ~* "^.*p=(.*?)(\.git|;|&|=|\s).*$") {
set $htpasswd /home/git/.gitolite/conf/$1_htpasswd;
}
if (-f $htpasswd) {
rewrite ^.*$ /closed last;
}
rewrite ^.*$ /guest last;
}
location = /closed {
internal;
access_log /var/log/nginx/gitweb-closed.access_log main;
auth_basic "Unauthorized";
auth_basic_user_file $htpasswd;
include fastcgi_params;
fastcgi_param SCRIPT_NAME gitweb.cgi;
fastcgi_param SCRIPT_FILENAME /home/git/gitweb/gitweb.cgi;
fastcgi_pass unix:/var/run/fcgiwrap.socket;
}
location = /guest {
internal;
access_log /var/log/nginx/gitweb-guest.access_log main;
include fastcgi_params;
fastcgi_param SCRIPT_NAME gitweb.cgi;
fastcgi_param SCRIPT_FILENAME /home/git/gitweb/gitweb.cgi;
fastcgi_pass unix:/var/run/fcgiwrap.socket;
}
location / {
rewrite (.*) / permanent;
}
}
^ 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