* Re: [PATCH v4 2/4] diff: introduce diff.submodule configuration variable
From: Ramkumar Ramachandra @ 2012-11-22 9:40 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Git List
In-Reply-To: <7vk3tht7yz.fsf@alter.siamese.dyndns.org>
Junio C Hamano wrote:
> Ramkumar Ramachandra <artagnon@gmail.com> writes:
>
>> diff --git a/t/t4041-diff-submodule-option.sh b/t/t4041-diff-submodule-option.sh
>> index 6c01d0c..e401814 100755
>> --- a/t/t4041-diff-submodule-option.sh
>> +++ b/t/t4041-diff-submodule-option.sh
>> @@ -33,6 +33,7 @@ test_create_repo sm1 &&
>> add_file . foo >/dev/null
>>
>> head1=$(add_file sm1 foo1 foo2)
>> +fullhead1=$(cd sm1; git rev-list --max-count=1 $head1)
>
> That looks like quite a roundabout way to say
>
> $(cd sm1; git rev-parse --verify HEAD)
>
> doesn't it? I know this is just moved code from the original, so I
> am not saying this should be fixed in this commit.
Exactly what I was thinking.
> Existing code in t7401 may want to be cleaned up, perhaps after this
> topic settles. The add_file() function, for example, has at least
> these points:
>
> - do we know 7 hexdigits is always the right precision?
> - what happens when it fails to create a commit in one of the steps
> while looping over "$@" in its loop?
> - the function uses the "cd there and then come back", not
> "go there in a subshell and do whatever it needs to" pattern.
Okay, will do.
>> +test_expect_success 'added submodule, set diff.submodule' "
>
> s/added/add/;
I see that the topic is already in `next`. Do you want to fix it up there?
> Shouldn't it test the base case where no diff.submodule is set? For
> those people, the patch should not change the output when they do or
> do not pass --submodule option, right?
When no diff.submodule is set, `git diff --submodule` should just work
as before; isn't this tested by the other tests in the file?
Ram
^ permalink raw reply
* [PATCH v2 3/4] format-patch: update append_signoff prototype
From: Nguyễn Thái Ngọc Duy @ 2012-11-22 16:38 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Nguyễn Thái Ngọc Duy
In-Reply-To: <1353602289-9418-1-git-send-email-pclouds@gmail.com>
This is a preparation step for merging with append_signoff from
sequencer.c
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
builtin/log.c | 13 +------------
log-tree.c | 21 +++++++++++++--------
revision.h | 2 +-
3 files changed, 15 insertions(+), 21 deletions(-)
diff --git a/builtin/log.c b/builtin/log.c
index 09cf43e..f201070 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -1053,7 +1053,6 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
struct commit *origin = NULL, *head = NULL;
const char *in_reply_to = NULL;
struct patch_ids ids;
- char *add_signoff = NULL;
struct strbuf buf = STRBUF_INIT;
int use_patch_format = 0;
int quiet = 0;
@@ -1148,16 +1147,6 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
PARSE_OPT_KEEP_ARGV0 | PARSE_OPT_KEEP_UNKNOWN |
PARSE_OPT_KEEP_DASHDASH);
- if (do_signoff) {
- const char *committer;
- const char *endpos;
- committer = git_committer_info(IDENT_STRICT);
- endpos = strchr(committer, '>');
- if (!endpos)
- die(_("bogus committer info %s"), committer);
- add_signoff = xmemdupz(committer, endpos - committer + 1);
- }
-
for (i = 0; i < extra_hdr.nr; i++) {
strbuf_addstr(&buf, extra_hdr.items[i].string);
strbuf_addch(&buf, '\n');
@@ -1348,7 +1337,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
total++;
start_number--;
}
- rev.add_signoff = add_signoff;
+ rev.add_signoff = do_signoff;
while (0 <= --nr) {
int shown;
commit = list[nr];
diff --git a/log-tree.c b/log-tree.c
index aff7c0a..7e50545 100644
--- a/log-tree.c
+++ b/log-tree.c
@@ -253,9 +253,11 @@ static int detect_any_signoff(char *letter, int size)
return seen_head && seen_name;
}
-static void append_signoff(struct strbuf *sb, const char *signoff)
+static void append_signoff(struct strbuf *sb, int flags)
{
- static const char signed_off_by[] = "Signed-off-by: ";
+ char* signoff = xstrdup(fmt_name(getenv("GIT_COMMITTER_NAME"),
+ getenv("GIT_COMMITTER_EMAIL")));
+ static const char sign_off_header[] = "Signed-off-by: ";
size_t signoff_len = strlen(signoff);
int has_signoff = 0;
char *cp;
@@ -274,9 +276,9 @@ static void append_signoff(struct strbuf *sb, const char *signoff)
}
/* First see if we already have the sign-off by the signer */
- while ((cp = strstr(cp, signed_off_by))) {
+ while ((cp = strstr(cp, sign_off_header))) {
const char *s = cp;
- cp += strlen(signed_off_by);
+ cp += strlen(sign_off_header);
if (!has_signoff && s > sb->buf) {
/*
@@ -317,9 +319,10 @@ static void append_signoff(struct strbuf *sb, const char *signoff)
if (!has_signoff)
strbuf_addch(sb, '\n');
- strbuf_addstr(sb, signed_off_by);
+ strbuf_addstr(sb, sign_off_header);
strbuf_add(sb, signoff, signoff_len);
strbuf_addch(sb, '\n');
+ free(signoff);
}
static unsigned int digits_in_number(unsigned int number)
@@ -684,8 +687,10 @@ void show_log(struct rev_info *opt)
/*
* And then the pretty-printed message itself
*/
- if (ctx.need_8bit_cte >= 0)
- ctx.need_8bit_cte = has_non_ascii(opt->add_signoff);
+ if (ctx.need_8bit_cte >= 0 && opt->add_signoff)
+ ctx.need_8bit_cte =
+ has_non_ascii(fmt_name(getenv("GIT_COMMITTER_NAME"),
+ getenv("GIT_COMMITTER_EMAIL")));
ctx.date_mode = opt->date_mode;
ctx.date_mode_explicit = opt->date_mode_explicit;
ctx.abbrev = opt->diffopt.abbrev;
@@ -696,7 +701,7 @@ void show_log(struct rev_info *opt)
pretty_print_commit(&ctx, commit, &msgbuf);
if (opt->add_signoff)
- append_signoff(&msgbuf, opt->add_signoff);
+ append_signoff(&msgbuf, 0);
if (opt->show_log_size) {
printf("log size %i\n", (int)msgbuf.len);
graph_show_oneline(opt->graph);
diff --git a/revision.h b/revision.h
index a95bd0b..af35325 100644
--- a/revision.h
+++ b/revision.h
@@ -136,7 +136,7 @@ struct rev_info {
int numbered_files;
char *message_id;
struct string_list *ref_message_ids;
- const char *add_signoff;
+ int add_signoff;
const char *extra_headers;
const char *log_reencode;
const char *subject_prefix;
--
1.8.0.4.g5d0415a
^ permalink raw reply related
* Re: [PATCH 0/8] fix git-config with duplicate variable entries
From: Junio C Hamano @ 2012-11-21 20:42 UTC (permalink / raw)
To: Jeff King; +Cc: Ævar Arnfjörð Bjarmason, Git Mailing List
In-Reply-To: <20121121200624.GF16280@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> So what do we want to do?
Nothing. We'd just let it graduate along with other topics, and
deal with a case where somebody screams, which is unlikely to happen
;-).
^ permalink raw reply
* Re: [PATCH 2/2] pickaxe: use textconv for -S counting
From: Jeff King @ 2012-11-21 20:27 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Peter Oberndorfer, git
In-Reply-To: <7vr4npt7zd.fsf@alter.siamese.dyndns.org>
On Mon, Nov 19, 2012 at 04:48:22PM -0800, Junio C Hamano wrote:
> Junio C Hamano <gitster@pobox.com> writes:
>
> >> Exact renames are the obvious one, but they are not handled here.
> >
> > That is half true. Before this change, we will find the same number
> > of needles and this function would have said "no differences" in a
> > very inefficient way. After this change, we may apply different
> > textconv filters and this function will say "there is a difference",
> > even though we wouldn't see such a difference at the content level
> > if there wasn't any rename.
>
> ... but I think that is a good thing anyway.
>
> If you renamed foo.c to foo.cc with different conversions from C
> code to the text that explain what the code does, if we special case
> only the exact rename case but let pickaxe examine the converted
> result in a case where blobs are modified only by one byte, we would
> get drastically different results between the two cases.
Right, exactly. I think the only sane thing is to always textconv or
always not textconv (whether they are identical renames or not), and any
"these are the same" optimization for identical content needs to take
into account whether we _would have_ done a different textconv (which
most of the time is going to be "no", as textconv is either not in use,
or both paths use the same diff driver; but it is not too expensive to
look up).
The diff_unmodified_pair at the top off diff_flush_patch is correct,
because it treats renames as interesting (because we have to show the
diff header, anyway). I do not know offhand if we avoid feeding
identical content to xdiff at all, but if so, we should be doing so only
after checking that the textconv filters are identical.
> Corollary to this is what should happen when you update the attributes
> between two trees so that textconv for a path that did not change
> between preimage and postimage are different. Ideally, we should
> notice that the two converted result are different, perhaps, but I
> do not like the performance implications very much.
The content to compare cannot be different unless either the input
content changed or the path changed, and we treat either as
"interesting" in most code paths. So I do not think there are any
performance implications, except that we may need to make sure to look
up textconvs a few lines sooner in some cases.
I'll re-roll the series next week and break out the rename-optimization
bits separately so it is more obvious that it is doing the right thing.
As an aside, I also need to revisit the regex half of that code, which
is still buggy (before and after my patch, due to the expecting-a-NUL
behavior we talked about a week or two ago). That is a separate topic,
but the same area of code.
-Peff
^ permalink raw reply
* Re: Topics currently in the Stalled category
From: Jeff King @ 2012-11-21 20:13 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Krzysztof Mazur, Paul Fox, git
In-Reply-To: <7va9ua20nz.fsf@alter.siamese.dyndns.org>
On Wed, Nov 21, 2012 at 11:53:04AM -0800, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
>
> > [1] Given the core-dumping behavior of SIGQUIT, I suspect it is not
> > nearly as widely used as SIGINT, but it sounds more like the
> > principle of least surprise to treat them the same.
>
> Sounds sensible. I wonder what happens when the editor is suspended
> ;-)
I think we would want to leave SIGTSTP alone; the editor should
typically respect it, and we would want to also pause until we get
SIGCONT (although even if we did continue, we would just be blocking on
wait() for the editor, anyway, so it is not a big deal).
Implicit in my "least surprise" comment above is that handling SIGQUIT
would match what system(3) does, so it makes sense to me to match that
(it also blocks SIGCHLD, but I do not think that really matters from a
user-visible perspective).
-Peff
^ permalink raw reply
* Re: [PATCH 0/8] fix git-config with duplicate variable entries
From: Jeff King @ 2012-11-21 20:06 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Ævar Arnfjörð Bjarmason, Git Mailing List
In-Reply-To: <7vehjm20yu.fsf@alter.siamese.dyndns.org>
On Wed, Nov 21, 2012 at 11:46:33AM -0800, Junio C Hamano wrote:
> > The problem is that scripts currently using "--get" are broken by
> > duplicate entries in include files, whereas internal git handles them
> > just fine. Introducing a new switch means that everybody also has to
> > start using it.
>
> Not exactly. There are three classes of people:
>
> - wrote scripts using --get; found out that --get barfs if you feed
> two or more of the same, and have long learned to accept it as a
> limitation and adjusted their configuration files to avoid it.
> They have been doing just fine and wouldn't benefit from this
> series at all.
>
> - wrote scripts using --get, relying on it barfing if fed zero
> (i.e. missing) or two or more (i.e. ambiguous), perhaps a way to
> keep their configuration files (arguably unnecessarily) clean.
> They are directly harmed by this series.
>
> - wrote scripts using --get-all and did the last-one-wins
> themselves. They wouldn't benefit directly from this series,
> unless they are willing to spend a bit of time to remove their
> own last-one-wins logic and replace --get-all with --get (but the
> same effort is needed to migrate to --get-one).
>
> - wanted to write scripts using --get, but after finding out that
> it barfs if you feed two, gave up emulating the internal, without
> realizing that they could do so with --get-all. They can now
> write their scripts without using --get-all.
I have a feeling that your cases 2 and 4 do not really exist. Because we
did "last one wins" in the case that it mattered (between different
files), it was always "good enough" to just assume that using "--get"
behaved like git did internally, and nobody really noticed or cared that
we did duplicate detection at all. But that is just from my own
perspective; it is not like I did a complete survey of git-config users.
More compelling to me is that the only ones negatively affected are your
case 2, and that is qualified by the "arguably unnecessary" you wrote.
Everyone else is not negatively impacted, and can later move to using
"--get" if they want to give up any home-grown --get-all code.
> > 2. If you are a script that only wants to mimic how get retrieves
> > a single value, then this is a bug fix, as it brings "--get" in
> > line with git's internal use.
>
> But by definition, no such script exists (if it used "--get", it
> didn't mimic the internal in the first place).
They do not exist if we assume that anyone using "--get" carefully
thought about the distinction. But I have the impression that is not the
case, and that they _meant_ to behave just like git, and did not realize
they were not doing so. Even our own scripts are guilty of this.
> Yeah, so the only ones that can be harmed is a config lint that uses
> the --get option with --file to make sure variables they know must
> be single value are indeed so, and they are not doing a thorough
> job, unless they are checking across files themselves, at which
> point they are better off using --get-all piped to "sort | uniq -c"
> or something. So breakages do not matter much for correctly written
> scripts.
Right.
> > IOW, I do not see a legitimate use case for this, but see it as an extra
> > check on broken config.
>
> Yes, I agree with the latter half of that sentence.
So what do we want to do?
-Peff
^ permalink raw reply
* Re: Topics currently in the Stalled category
From: Junio C Hamano @ 2012-11-21 19:53 UTC (permalink / raw)
To: Jeff King; +Cc: Krzysztof Mazur, Paul Fox, git
In-Reply-To: <20121121193401.GC16280@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> [1] Given the core-dumping behavior of SIGQUIT, I suspect it is not
> nearly as widely used as SIGINT, but it sounds more like the
> principle of least surprise to treat them the same.
Sounds sensible. I wonder what happens when the editor is suspended
;-)
^ permalink raw reply
* Re: Topics currently in the Stalled category
From: Paul Fox @ 2012-11-21 19:50 UTC (permalink / raw)
To: Jeff King; +Cc: Krzysztof Mazur, git
In-Reply-To: <20121121193401.GC16280@sigill.intra.peff.net>
jeff wrote:
> On Wed, Nov 21, 2012 at 10:27:50AM +0100, Krzysztof Mazur wrote:
>
> > > > * pf/editor-ignore-sigint (2012-11-11) 5 commits
> > > >
> > > > Avoid confusing cases where the user hits Ctrl-C while in the editor
> > > > session, not realizing git will receive the signal. Since most editors
> > > > will take over the terminal and will block SIGINT, this is not likely
> > > > to confuse anyone.
> > > >
> > > > Some people raised issues with emacsclient, which are addressed by this
> > > > re-roll. It should probably also handle SIGQUIT, and there were a
> > > > handful of other review comments.
> > > >
> > > > Anybody interested in moving this forward?
> > >
> > > i started this, but then jeff took it and ran with it and made it
> > > right. i think the remaining changes are small -- if jeff would
> > > prefer, i can probably finish it. (but i won't guarantee not to
> > > mess up the "From:" lines. :-)
> > >
> >
> > I'm also interested. I sometimes use ":r !command" in vim, so far I never
> > needed to use Ctrl-C, but maybe in future.
> >
> > The SIGINT part seems to be finished, we need to decide what about SIGQUIT.
>
> My plan was to just add in SIGQUIT[1] alongside SIGINT (and I think
> there may have been one or two other minor comments in response to the
> series). I am on vacation this week, but can revisit it next week. If
> somebody wants to re-roll it in the meantime, that would be fine with
> me.
>
> -Peff
>
> [1] Given the core-dumping behavior of SIGQUIT, I suspect it is not
> nearly as widely used as SIGINT, but it sounds more like the
> principle of least surprise to treat them the same.
i see no real point in treating them the same -- as you suggest, one
would only use SIGQUIT if SIGINT didn't work, and then you'd want them
to be treated differently. so i'd be happy with the current code in
that regard.
i think krzysiek said that since editors usually catch SIGQUIT, git
should kill the editor when receiving SIGQUIT, essentially translating
the SIGQUIT to SIGTERM for the editor. (please correct me if i
misunderstood.) since well-behaved editors will die quickly anyway
(they get EIO on their next read from stdin), i'm not sure there's a
compelling reason for that extra step.
but i have no real objection to that behavior if others think it's
right -- there's certainly logic in saying that if git dies it should
ensure the editor does too.
(i'm away for the rest of the week also.)
paul
=---------------------
paul fox, pgf@foxharp.boston.ma.us (arlington, ma, where it's 44.6 degrees)
^ permalink raw reply
* Re: [PATCH v5 15/15] fast-export: don't handle uninteresting refs
From: Jeff King @ 2012-11-21 19:48 UTC (permalink / raw)
To: Junio C Hamano
Cc: Jonathan Nieder, Felipe Contreras, git, Johannes Schindelin,
Max Horn, Sverre Rabbelier, Brandon Casey, Brandon Casey,
Ilari Liusvaara, Pete Wyckoff, Ben Walton, Matthieu Moy,
Julian Phillips
In-Reply-To: <7vfw43pmp7.fsf@alter.siamese.dyndns.org>
On Tue, Nov 20, 2012 at 09:08:36PM -0800, Junio C Hamano wrote:
> With such a one-sided discussion, I've been having a hard time
> convincing myself if Felipe's effort is making the interface better,
> or just breaking it even more for existing remote helpers, only to
> fit his world model better.
Felipe responded in more detail, but I will just add the consensus we
came to earlier in the discussion: the series does make things better
for users of fast-export that use marks, but does not make things any
better for users of negative refs on the command line. However, I do not
think that it makes things worse for them, either (neither by changing
the behavior negatively, nor by making the code harder for a more
complete fix later).
So while fixing everybody might be nice, there is no need to hold up
progress for the marks case. Which, as he has noted, is probably the
sanest way to implement a remote-helper[1].
-Peff
[1] There are other possible use cases for fast-export which might
benefit from negative refs working more sanely, but since they are
in the minority and are not being made worse, I think the partial
fix is OK.
^ permalink raw reply
* Re: [PATCH 0/8] fix git-config with duplicate variable entries
From: Junio C Hamano @ 2012-11-21 19:46 UTC (permalink / raw)
To: Jeff King; +Cc: Ævar Arnfjörð Bjarmason, Git Mailing List
In-Reply-To: <20121121192738.GA16280@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
>> The way for the Porcelain scripts to mimick the internal "last one
>> wins" has been to grab values out of --get-all and do the "last one
>> wins" themselves, and I agree that a mode that frees them from
>> having to worry about it is a good *addition*. I would even say
>> that if we were designing "git config" plumbing without existing
>> users, it would be the only sensible behaviour for "--get".
>>
>> But "git config --get" users have been promised to get errors on
>> duplicate values so far, so I have to say this needs to come with a
>> big red sign about API breakage.
>
> The problem is that scripts currently using "--get" are broken by
> duplicate entries in include files, whereas internal git handles them
> just fine. Introducing a new switch means that everybody also has to
> start using it.
Not exactly. There are three classes of people:
- wrote scripts using --get; found out that --get barfs if you feed
two or more of the same, and have long learned to accept it as a
limitation and adjusted their configuration files to avoid it.
They have been doing just fine and wouldn't benefit from this
series at all.
- wrote scripts using --get, relying on it barfing if fed zero
(i.e. missing) or two or more (i.e. ambiguous), perhaps a way to
keep their configuration files (arguably unnecessarily) clean.
They are directly harmed by this series.
- wrote scripts using --get-all and did the last-one-wins
themselves. They wouldn't benefit directly from this series,
unless they are willing to spend a bit of time to remove their
own last-one-wins logic and replace --get-all with --get (but the
same effort is needed to migrate to --get-one).
- wanted to write scripts using --get, but after finding out that
it barfs if you feed two, gave up emulating the internal, without
realizing that they could do so with --get-all. They can now
write their scripts without using --get-all.
The only people who benefit are from the last class; it does not
matter if they have to write --get-one or --get.
> That is not an excuse for breakage, of course, but only a motivation for
> considering it. And here is what I think mitigates that breakage:
>
> 1. If you are a script that cares about multiple values and choosing
> one (whether last-one-wins or otherwise), you are already using
> --get-all and are not affected.
Correct.
> 2. If you are a script that only wants to mimic how get retrieves
> a single value, then this is a bug fix, as it brings "--get" in
> line with git's internal use.
But by definition, no such script exists (if it used "--get", it
didn't mimic the internal in the first place).
> 3. If you are a script that only wants a single value, but cares about
> duplicates, you are already failing to notice them when the
> duplicates are cross-file. That is, we already do "last one wins"
> if an entry is found in ~/.gitconfig and .git/config.
Yeah, so the only ones that can be harmed is a config lint that uses
the --get option with --file to make sure variables they know must
be single value are indeed so, and they are not doing a thorough
job, unless they are checking across files themselves, at which
point they are better off using --get-all piped to "sort | uniq -c"
or something. So breakages do not matter much for correctly written
scripts.
> I would argue that anybody fetching standard git config variables (and
> not using --list or --get-all) falls into case (2) and is being fixed,
> as they will not otherwise match the behavior of git itself.
As people shove random stuff that git itself does not care about in
their config files, they may not care, though.
> IOW, I do not see a legitimate use case for this, but see it as an extra
> check on broken config.
Yes, I agree with the latter half of that sentence.
^ permalink raw reply
* Re: [PATCH] remote-curl.c: Fix a compiler warning
From: Jeff King @ 2012-11-21 19:34 UTC (permalink / raw)
To: Ramsay Jones; +Cc: Junio C Hamano, GIT Mailing-list
In-Reply-To: <50AD26C3.2090407@ramsay1.demon.co.uk>
On Wed, Nov 21, 2012 at 07:08:51PM +0000, Ramsay Jones wrote:
> In particular, gcc issues an "'gzip_size' might be used uninitialized"
> warning (-Wuninitialized). However, this warning is a false positive,
> since the 'gzip_size' variable would not, in fact, be used uninitialized.
> In order to suppress the warning, we simply initialise the variable to
> zero in it's declaration.
>
> Signed-off-by: Ramsay Jones <ramsay@ramsay1.demon.co.uk>
> ---
>
> Hi Junio,
>
> This is on top of next. (commit df126e108: "remote-curl: hoist gzip
> buffer size to top of post_rpc", 31-10-2012).
Thanks. I meant to apply this during my tenure as maintainer, but it
slipped through the cracks.
-Peff
^ permalink raw reply
* Re: Topics currently in the Stalled category
From: Jeff King @ 2012-11-21 19:34 UTC (permalink / raw)
To: Krzysztof Mazur; +Cc: Paul Fox, Junio C Hamano, git
In-Reply-To: <20121121092750.GA8262@shrek.podlesie.net>
On Wed, Nov 21, 2012 at 10:27:50AM +0100, Krzysztof Mazur wrote:
> > > * pf/editor-ignore-sigint (2012-11-11) 5 commits
> > >
> > > Avoid confusing cases where the user hits Ctrl-C while in the editor
> > > session, not realizing git will receive the signal. Since most editors
> > > will take over the terminal and will block SIGINT, this is not likely
> > > to confuse anyone.
> > >
> > > Some people raised issues with emacsclient, which are addressed by this
> > > re-roll. It should probably also handle SIGQUIT, and there were a
> > > handful of other review comments.
> > >
> > > Anybody interested in moving this forward?
> >
> > i started this, but then jeff took it and ran with it and made it
> > right. i think the remaining changes are small -- if jeff would
> > prefer, i can probably finish it. (but i won't guarantee not to
> > mess up the "From:" lines. :-)
> >
>
> I'm also interested. I sometimes use ":r !command" in vim, so far I never
> needed to use Ctrl-C, but maybe in future.
>
> The SIGINT part seems to be finished, we need to decide what about SIGQUIT.
My plan was to just add in SIGQUIT[1] alongside SIGINT (and I think
there may have been one or two other minor comments in response to the
series). I am on vacation this week, but can revisit it next week. If
somebody wants to re-roll it in the meantime, that would be fine with
me.
-Peff
[1] Given the core-dumping behavior of SIGQUIT, I suspect it is not
nearly as widely used as SIGINT, but it sounds more like the
principle of least surprise to treat them the same.
^ permalink raw reply
* Re: Re* [PATCH] gitweb: make remote_heads config setting work.
From: Jeff King @ 2012-11-21 19:31 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Phil Pennock, git
In-Reply-To: <7vpq37rk3v.fsf_-_@alter.siamese.dyndns.org>
On Tue, Nov 20, 2012 at 02:21:40PM -0800, Junio C Hamano wrote:
> > Good catch. I think the "return" in the existing code suffers from the
> > same problem: it will bail on non-word characters in the $mi part, but
> > that part should allow arbitrary characters.
>
> I am tired of keeping the "expecting reroll" entries and having to
> worry about them, so let's do this
>
> -- >8 --
> Subject: [squash] gitweb: make remote_heads config setting work
>
> Only the top-level and bottom-level names are case insensitive and
> spelled without "_". Protect future support of subsection names
> from name mangling.
I think the end result is fine, though you are really fixing a bug here
(the /\W/ check is moved up), and then also putting the remote_heads
fix (s/_//g) into the right place. IOW, if you are going to squash, you
should probably note the bug-fix part in the commit message explicitly.
-Peff
^ permalink raw reply
* Re: [PATCH 0/8] fix git-config with duplicate variable entries
From: Jeff King @ 2012-11-21 19:27 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Ævar Arnfjörð Bjarmason, Git Mailing List
In-Reply-To: <7vsj84rt1g.fsf@alter.siamese.dyndns.org>
On Tue, Nov 20, 2012 at 11:08:43AM -0800, Junio C Hamano wrote:
> > Thanks. It had a few minor flaws (like a memory leak). I fixed those,
> > updated the tests, and split it out into a few more readable commits. In
> > the process, I managed to uncover and fix a few other memory leaks in
> > the area. I think this version is much more readable, and writing the
> > rationale for patch 7 convinced me that it's the right thing to do.
> > Another round of review would be appreciated.
> >
> > [1/8]: t1300: style updates
> > [2/8]: t1300: remove redundant test
> > [3/8]: t1300: test "git config --get-all" more thoroughly
> > [4/8]: git-config: remove memory leak of key regexp
> > [5/8]: git-config: fix regexp memory leaks on error conditions
> > [6/8]: git-config: collect values instead of immediately printing
> > [7/8]: git-config: do not complain about duplicate entries
> > [8/8]: git-config: use git_config_with_options
> >
> > For those just joining us, the interesting bit is patch 7, which fixes
> > some inconsistencies between the "git-config" tool and how the internal
> > config callbacks work.
>
> The way for the Porcelain scripts to mimick the internal "last one
> wins" has been to grab values out of --get-all and do the "last one
> wins" themselves, and I agree that a mode that frees them from
> having to worry about it is a good *addition*. I would even say
> that if we were designing "git config" plumbing without existing
> users, it would be the only sensible behaviour for "--get".
>
> But "git config --get" users have been promised to get errors on
> duplicate values so far, so I have to say this needs to come with a
> big red sign about API breakage.
The problem is that scripts currently using "--get" are broken by
duplicate entries in include files, whereas internal git handles them
just fine. Introducing a new switch means that everybody also has to
start using it.
That is not an excuse for breakage, of course, but only a motivation for
considering it. And here is what I think mitigates that breakage:
1. If you are a script that cares about multiple values and choosing
one (whether last-one-wins or otherwise), you are already using
--get-all and are not affected.
2. If you are a script that only wants to mimic how get retrieves
a single value, then this is a bug fix, as it brings "--get" in
line with git's internal use.
3. If you are a script that only wants a single value, but cares about
duplicates, you are already failing to notice them when the
duplicates are cross-file. That is, we already do "last one wins"
if an entry is found in ~/.gitconfig and .git/config.
I would argue that anybody fetching standard git config variables (and
not using --list or --get-all) falls into case (2) and is being fixed,
as they will not otherwise match the behavior of git itself.
For other variables that porcelains want to stuff inside the config
files, I suppose they could fall into case (3). But I am not sure why
they would care about duplicates. They have asked git for a single
value, and if they care more deeply about multiple values (but only
within a single file!), what do they hope to accomplish by not calling
--get-all? That is, what is the use case where this makes any sense?
The only case I can think of where the distinction even matters is:
1. Porcelain foo writes to the .gitfoo file via "git config -f
.gitfoo".
2. It accidentally writes using "--add" instead of just setting the
value.
3. It fetches via "git config -f .gitfoo --get foo.var". Before my
patch, duplicate detection would notice the bug in (2) and barf.
Now, it silently takes the most recently added value (which is
probably what was meant anyway).
IOW, I do not see a legitimate use case for this, but see it as an extra
check on broken config. But it is catching an unlikely condition, and is
an overly restrictive check, in that it is triggering on totally
reasonable config. So we are not breaking a use case so much as a
loosening a (in my opinion) useless check.
But maybe I am missing some more sane case.
> I would feel safer to introduce --get-one or something now, and
> worry about migration as a separate step.
Part of my impetus for fixing --get is that it let us cleanup and
harmonize the git_config() and git-config implementations. If we are not
going to do that, adding an extra option is not that appealing, as we
are stuck with the old duplicated code, anyway. As I mentioned above,
this only really affects include files (because from git-config's
perspective, the entries it sees are all "the same file", as they come
from the same call into git_config_from_file). If we are not going to
fix --get for all callers, it would probably make more sense to just
omit the duplicate suppression for entries across include-file
boundaries (which are something that callers would have to opt into when
using a specific file, anyway). IOW, to treat it as a bug in the include
system and fix it there.
-Peff
^ permalink raw reply
* Re: [PATCH v5 11/15] remote-testgit: make clear the 'done' feature
From: Sverre Rabbelier @ 2012-11-21 19:20 UTC (permalink / raw)
To: Junio C Hamano
Cc: Max Horn, Felipe Contreras, git, Johannes Schindelin, Jeff King,
Brandon Casey, Brandon Casey, Jonathan Nieder, Ilari Liusvaara,
Pete Wyckoff, Ben Walton, Matthieu Moy, Julian Phillips
In-Reply-To: <7vy5hu3h11.fsf@alter.siamese.dyndns.org>
On Wed, Nov 21, 2012 at 10:11 AM, Junio C Hamano <gitster@pobox.com> wrote:
> I'd state it like this, but I may have guessed what Felipe intended
> incorrectly.
>
> remote-testgit: advertise "done" feature and write "done" ourselves
>
> Instead of letting "fast-export" advertise the feature and ending
> its stream with "done", do it ourselves. This way, it would make it
> more clear to people who want to write their own remote-helper to
> produce fast-export streams without using "fast-export
> --use-done-feature" that they are supposed to end their stream with
> "done".
With that commit message:
Acked-by: Sverre Rabbelier <srabbelier@gmail.com>
--
Cheers,
Sverre Rabbelier
^ permalink raw reply
* Re: [regression] Newer gits cannot clone any remote repos
From: Douglas Mencken @ 2012-11-21 19:20 UTC (permalink / raw)
To: Ramsay Jones; +Cc: Torsten Bögershausen, git
In-Reply-To: <50A53FAC.8020401@ramsay1.demon.co.uk>
> The threaded index-pack code did not fail for
> me on cygwin at all during development, including tests, but failed
> immediately I installed v1.7.11. On real repositories, it failed
> intermittently. On some repos it always failed, on some it never
> failed and on some others it would sometimes fail, sometimes not.
Then why did you commit it? If it has so high random failure rate.
^ permalink raw reply
* Duplicate test numbers in pu.
From: Ramsay Jones @ 2012-11-21 19:14 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jeff King, git, GIT Mailing-list, felipe.contreras
Hi Junio,
I noticed that the pu branch has two tests with number t0007, viz:
$ cd t
$ make test-lint-duplicates
duplicate test numbers: t0007
make: *** [test-lint-duplicates] Error 1
$
In particular, t/t0007-git-var.sh is added by branch 'jk/send-email-\
sender-prompt', while t/t0007-ignores.sh is added by branch 'as/check-ignore'.
Also:
$ make test-lint-executable
non-executable tests: t5801-remote-helpers.sh
make: *** [test-lint-executable] Error 1
$
(added in branch 'fc/fast-export-fixes').
HTH
ATB,
Ramsay Jones
^ permalink raw reply
* [PATCH] remote-curl.c: Fix a compiler warning
From: Ramsay Jones @ 2012-11-21 19:08 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jeff King, GIT Mailing-list
In particular, gcc issues an "'gzip_size' might be used uninitialized"
warning (-Wuninitialized). However, this warning is a false positive,
since the 'gzip_size' variable would not, in fact, be used uninitialized.
In order to suppress the warning, we simply initialise the variable to
zero in it's declaration.
Signed-off-by: Ramsay Jones <ramsay@ramsay1.demon.co.uk>
---
Hi Junio,
This is on top of next. (commit df126e108: "remote-curl: hoist gzip
buffer size to top of post_rpc", 31-10-2012).
Thanks!
ATB,
Ramsay Jones
remote-curl.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/remote-curl.c b/remote-curl.c
index d8b3600..9a8b123 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -400,7 +400,7 @@ static int post_rpc(struct rpc_state *rpc)
struct curl_slist *headers = NULL;
int use_gzip = rpc->gzip_request;
char *gzip_body = NULL;
- size_t gzip_size;
+ size_t gzip_size = 0;
int err, large_request = 0;
/* Try to load the entire request, if we can fit it into the
--
1.8.0
^ permalink raw reply related
* Re: [PATCH v5 00/15] fast-export and remote-testgit improvements
From: Junio C Hamano @ 2012-11-21 19:05 UTC (permalink / raw)
To: Felipe Contreras
Cc: git, Johannes Schindelin, Max Horn, Jeff King, Sverre Rabbelier,
Brandon Casey, Brandon Casey, Jonathan Nieder, Ilari Liusvaara,
Pete Wyckoff, Ben Walton, Matthieu Moy, Julian Phillips
In-Reply-To: <CAMP44s3h5+KS3ixoLkJeiS+n_neBV-Dyj=Cww0ZrU6UKsNxphQ@mail.gmail.com>
Felipe Contreras <felipe.contreras@gmail.com> writes:
> On Sun, Nov 11, 2012 at 2:59 PM, Felipe Contreras
> <felipe.contreras@gmail.com> wrote:
>
> Since these are having some problems getting in, let me point out
> which I think are important, and which not.
I finished reading the series, and found them mostly sensible.
I'll send out comments on individual patches, and will push them
out, interspersed with "fixup!" commits, later on 'pu' when I am
done for the day, perhaps in 7 hours or so.
There is one thing I am not sure about with this series, though.
I can agree that the updates to fast-export will make remote-testgit
script work better, but I cannot tell how big an impact the changes
will have to people's existing use of fast-export. Some of them may
be relying on the current behaviour (in other words, they may be
relying on "existing bugs"), which may mean that this series will
bring regression to them. I am still open to reasonable objections
along the lines of "This script X uses fast-export and is broken
when used with the updated behaviour." if there is any.
^ permalink raw reply
* Re: [PATCH v5 08/15] remote-testgit: cleanup tests
From: Junio C Hamano @ 2012-11-21 18:28 UTC (permalink / raw)
To: Felipe Contreras
Cc: git, Johannes Schindelin, Max Horn, Jeff King, Sverre Rabbelier,
Brandon Casey, Brandon Casey, Jonathan Nieder, Ilari Liusvaara,
Pete Wyckoff, Ben Walton, Matthieu Moy, Julian Phillips
In-Reply-To: <1352642392-28387-9-git-send-email-felipe.contreras@gmail.com>
Felipe Contreras <felipe.contreras@gmail.com> writes:
> We don't need a bare 'server' and an intermediary 'public'. The repos
> can talk to each other directly; that's what we want to exercise.
The previous patch to remove the test (the one that covered a case
where a bug was fixed in an older git-remote-testpy and tried to
catch the bug when it resurfaced) made sense even with its
ultra-short justification "irrelevant".
But I am not sure if this one is so cut-and-dried. The repos can
talk to each other directly, but at the same time the tests were
exercising interactions between bare and non-bare repositories,
weren't they? Talking to each other may be one of the things we
want to exercise, but that does not necessarily be the only thing.
If it were explained like this (note that I am *guessing* what you
meant to achieve by this patch, which may be wrong, in which case
the log message needs further clarification):
Going through an intermediary 'public' may have exercised
interactions among combinations of bare and non-bare
repositories a bit more, but that is not an issue specific
to the remote-helper transfer that we want to be testing in
this script. Simplify the tests to let two repositories
talk directly with each other.
I think the changes themselves make sense.
^ permalink raw reply
* Re: [PATCH v5 06/15] remote-testgit: get rid of non-local functionality
From: Junio C Hamano @ 2012-11-21 18:26 UTC (permalink / raw)
To: Felipe Contreras
Cc: git, Johannes Schindelin, Max Horn, Jeff King, Sverre Rabbelier,
Brandon Casey, Brandon Casey, Jonathan Nieder, Ilari Liusvaara,
Pete Wyckoff, Ben Walton, Matthieu Moy, Julian Phillips
In-Reply-To: <1352642392-28387-7-git-send-email-felipe.contreras@gmail.com>
Felipe Contreras <felipe.contreras@gmail.com> writes:
> This only makes sense for the python remote helpers framework.
A better explanation is sorely needed for this. If the test were
feeding python snippet to be sourced by python remote helper to be
tested, the new remote-testgit.bash would not have any hope (nor
need) to grok it, and "this only makes sense for python" makes
perfect sense and clear enough, but that is not the case.
If the justification were like this:
remote-testgit: remove non-local tests
The simplified remote-testgit does not talk with any remote
repository and incapable of running non-local tests. Remove
them.
I would understand it, and I wouldn't say it is a regression in the
test not to test "non-local", as that is not essential aspect of
these tests (we are only interested in testing the object/ref
transfer over remote-helper interface and do not care what the
"other side" really is).
But I am not quite sure what you really mean by "non-local"
functionality in the first place. The original test weren't opening
network port to emulate multi-host remote transfer, were it?
Thanks.
^ permalink raw reply
* Re: [PATCH v5 09/15] remote-testgit: exercise more features
From: Junio C Hamano @ 2012-11-21 18:26 UTC (permalink / raw)
To: Felipe Contreras
Cc: git, Johannes Schindelin, Max Horn, Jeff King, Sverre Rabbelier,
Brandon Casey, Brandon Casey, Jonathan Nieder, Ilari Liusvaara,
Pete Wyckoff, Ben Walton, Matthieu Moy, Julian Phillips
In-Reply-To: <1352642392-28387-10-git-send-email-felipe.contreras@gmail.com>
Felipe Contreras <felipe.contreras@gmail.com> writes:
> Unfortunately they do not work.
As far as I can tell, "more features" simply mean one, no? Perhaps
remote-testgit: exercise non-default refspec feature
or something.
> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
> ---
> git-remote-testgit | 18 +++++++++++++----
> t/t5801-remote-helpers.sh | 49 +++++++++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 63 insertions(+), 4 deletions(-)
> mode change 100755 => 100644 t/t5801-remote-helpers.sh
Oops.
Again, please check the fixup! interspersed in the result I'll queue
on 'pu'.
Thanks.
^ permalink raw reply
* Re: [PATCH v5 05/15] Add new simplified git-remote-testgit
From: Junio C Hamano @ 2012-11-21 18:26 UTC (permalink / raw)
To: Felipe Contreras
Cc: git, Johannes Schindelin, Max Horn, Jeff King, Sverre Rabbelier,
Brandon Casey, Brandon Casey, Jonathan Nieder, Ilari Liusvaara,
Pete Wyckoff, Ben Walton, Matthieu Moy, Julian Phillips
In-Reply-To: <1352642392-28387-6-git-send-email-felipe.contreras@gmail.com>
Felipe Contreras <felipe.contreras@gmail.com> writes:
> It's way simpler. It exerceises the same features of remote helpers.
> It's easy to read and understand. It doesn't depend on python.
>
> It does _not_ exercise the python remote helper framework; there's
> another tool and another test for that.
You mention why you _think_ it is better, and what it is _not_, but
with your excitement, end up failing to mention what it is. I'll
try to reword the commit with this sentence:
This script is to test the remote-helper interface.
somewhere. Please check what I'll push out on 'pu' after I'm done
for the day (probably in 8 hours).
> git-remote-testgit | 62 ++++++++++++++++
> t/t5801-remote-helpers.sh | 139 +++++++++++++++++++++++++++++++++++
> 3 files changed, 202 insertions(+), 1 deletion(-)
> create mode 100755 git-remote-testgit
I hinted at this in an earlier message, but creating this file as a
tracked file at this point in the history is a bit irritating for
bisectability. After you build and test an earlier commit, this
path is a generated and ignored, and then checking out this commit
or a later one will fail without "make clean". It is only a minor
irritation, but still noticeable.
Thanks.
^ permalink raw reply
* Re: [PATCH v5 14/15] fast-export: make sure updated refs get updated
From: Junio C Hamano @ 2012-11-21 18:12 UTC (permalink / raw)
To: Max Horn
Cc: Felipe Contreras, git, Johannes Schindelin, Jeff King,
Sverre Rabbelier, Brandon Casey, Brandon Casey, Jonathan Nieder,
Ilari Liusvaara, Pete Wyckoff, Ben Walton, Matthieu Moy,
Julian Phillips
In-Reply-To: <D39D26A9-16B4-4A9F-9102-BD2C92FA10AF@quendi.de>
Max Horn <max@quendi.de> writes:
> On 11.11.2012, at 14:59, Felipe Contreras wrote:
>
>> When an object has already been exported (and thus is in the marks) it's
>> flagged as SHOWN, so it will not be exported again, even if in a later
>> time it's exported through a different ref.
>>
>> We don't need the object to be exported again, but we want the ref
>> updated, which doesn't happen.
>>
>> Since we can't know if a ref was exported or not, let's just assume that
>> if the commit was marked (flags & SHOWN), the user still wants the ref
>> updated.
>>
>> IOW: If it's specified in the command line, it will get updated,
>> regardless of wihether or not the object was marked.
>
> Typo: wihether => whether
>
>>
>> So:
>>
>> % git branch test master
>> % git fast-export $mark_flags master
>> % git fast-export $mark_flags test
>>
>> Would export 'test' properly.
>>
>> Additionally, this fixes issues with remote helpers; now they can push
>> refs wich objects have already been exported, and a few other issues as
>
> Typo: wich => which
I'd rather use "whose" there.
^ permalink raw reply
* Re: [PATCH v5 15/15] fast-export: don't handle uninteresting refs
From: Junio C Hamano @ 2012-11-21 18:14 UTC (permalink / raw)
To: Felipe Contreras
Cc: git, Johannes Schindelin, Max Horn, Jeff King, Sverre Rabbelier,
Brandon Casey, Brandon Casey, Jonathan Nieder, Ilari Liusvaara,
Pete Wyckoff, Ben Walton, Matthieu Moy, Julian Phillips
In-Reply-To: <1352642392-28387-16-git-send-email-felipe.contreras@gmail.com>
Felipe Contreras <felipe.contreras@gmail.com> writes:
> They have been marked as UNINTERESTING for a reason, lets respect that.
> ...
> The current behavior is most certainly not what we want. After this
> patch, nothing gets exported, because nothing was selected (everything
> is UNINTERESTING).
The old behaviour was an incorrect "workaround" that has been
superseded by your 14/15 "make sure updated refs get updated", no?
Mentioning that would help people realize that this patch would not
cause regression on them, I would think.
^ 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