* Re: [PATCH] fmt-merge-msg: use branch.$name.description
From: Jonathan Nieder @ 2011-10-07 20:12 UTC (permalink / raw)
To: Michael J Gruber; +Cc: Junio C Hamano, git, Jeff King
In-Reply-To: <4E8EED39.1060607@drmicha.warpmail.net>
Hi again,
Michael J Gruber wrote:
> I really haven't seen any convincing argument against yet. The details
> (note attached to refname object, or literal refanmes in the notes tree
> as per Jeff) should be discussed further, of course, but if branch
> descriptions aren't "notesy" then I don't know what is.
As mentioned before, I don't want to debate how Junio should spend his
time (better for each person to provide relevant information and
improvements to help out or to spend time on the alternatives one is
interested in), but as a general question, this statement looked
interesting to me.
"git notes" has a funny name, but deep down, as you know, it's about
attaching additional versioned text to commit objects without changing
their names. Branch descriptions are not per commit object (and as a
mapping, the _keys_ are not shared), and personally I don't think they
should be versioned any more than the branch names are.
I wanted to emphasize this because
"git notes" is not the best tool for all annotations!
This ends this public service announcement.
> Alternatively, one could store the description in a blob and refer to
> that directly, of course. I.e., have
>
> refs/description/foo
>
> point to a blob whose content is the description of the ref
>
> ref/foo
Sure, that would be a sane alternative design. It has the advantage
of having the pumbing for fetching and pulling already set up, as you
mention. The only disadvantages I know of are
- "git branch -m" and "git remote rename" don't know about it yet
- there's not one flat file you can edit to run a search/replace on
all branch descriptions
and those aren't very serious problems.
>> I personally would prefer my branch descriptions to be non-versioned,
>> though I realize that is a matter of taste.
>
> Do you prefer you commit notes to be non-versioned?
No, I like them versioned. If I didn't, why wouldn't I have sent a
patch to change that? Maybe some day there will be a "git notes log"
tool to track the history of a note, taking changes in fanout into
account.
Hope that clarifies a little.
^ permalink raw reply
* Re: [PATCH] git-difftool: allow skipping file by typing 'n' at prompt
From: Junio C Hamano @ 2011-10-07 20:09 UTC (permalink / raw)
To: Sitaram Chamarty; +Cc: Phil Hord, Sitaram Chamarty, git
In-Reply-To: <20111006181522.GA2936@sita-lt.atc.tcs.com>
Sitaram Chamarty <sitaramc@gmail.com> writes:
> This is useful if you forgot to restrict the diff to the paths you want
> to see, or selecting precisely the ones you want is too much typing.
>
> Signed-off-by: Sitaram Chamarty <sitaram@atc.tcs.com>
> ---
>
> On Thu, Oct 06, 2011 at 10:36:40AM -0700, Junio C Hamano wrote:
>
>> Thanks. It is clear from the subject and the patch text that you are
>> changing "hit return to unconditionally launch" into "launch it if you
>> want to", but can you give justification why a choice not to launch is
>> needed in the log message?
>
> OK; done.
Looks OK from a cursory viewing. Do we want some additional tests?
For that matter, have you run the test suite with this patch applied (I
haven't)?
> git-difftool--helper.sh | 9 +++++----
> 1 files changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/git-difftool--helper.sh b/git-difftool--helper.sh
> index 8452890..0468446 100755
> --- a/git-difftool--helper.sh
> +++ b/git-difftool--helper.sh
> @@ -38,15 +38,16 @@ launch_merge_tool () {
>
> # $LOCAL and $REMOTE are temporary files so prompt
> # the user with the real $MERGED name before launching $merge_tool.
> + ans=y
> if should_prompt
> then
> printf "\nViewing: '$MERGED'\n"
> if use_ext_cmd
> then
> - printf "Hit return to launch '%s': " \
> + printf "Launch '%s' [Y/n]: " \
> "$GIT_DIFFTOOL_EXTCMD"
> else
> - printf "Hit return to launch '%s': " "$merge_tool"
> + printf "Launch '%s' [Y/n]: " "$merge_tool"
> fi
> read ans
> fi
> @@ -54,9 +55,9 @@ launch_merge_tool () {
> if use_ext_cmd
> then
> export BASE
> - eval $GIT_DIFFTOOL_EXTCMD '"$LOCAL"' '"$REMOTE"'
> + test "$ans" != "n" && eval $GIT_DIFFTOOL_EXTCMD '"$LOCAL"' '"$REMOTE"'
> else
> - run_merge_tool "$merge_tool"
> + test "$ans" != "n" && run_merge_tool "$merge_tool"
> fi
> }
^ permalink raw reply
* Re: [PATCH] fmt-merge-msg: use branch.$name.description
From: Junio C Hamano @ 2011-10-07 19:59 UTC (permalink / raw)
To: Jonathan Nieder; +Cc: Michael J Gruber, git, Jeff King
In-Reply-To: <20111007195023.GA29712@elie.hsd1.il.comcast.net>
Jonathan Nieder <jrnieder@gmail.com> writes:
> I am surprised that you seem to have missed what I meant by "branch
> names are local"....
> This matters, a lot, because there is no easy way to partition a
> namespace of names descriptive for humans without a central authority
> to negotiate conflicts.
I think we are in total agreement. The branch names are local, but the
users may want to annotate to describe _the history_ they intend to build
on it.
Various ways to convey the description when the end product (i.e. the
history built on it) is shiped were outlined in the series, so that the
shipper can help the receiver understand the history. The information in
the annotation (i.e. the _value_ of branch.$name.description) is something
the shipper wants to share with the receiver, but the mapping between the
local name of the branch the shipper used to build that history (i.e. the
key "$name" in branch.$name.description) is immaterial in the end result.
I do not think there is much more for me to add to this topic, as I think
you covered all the important bases already.
^ permalink raw reply
* Re: [PATCH] fmt-merge-msg: use branch.$name.description
From: Jonathan Nieder @ 2011-10-07 19:50 UTC (permalink / raw)
To: Michael J Gruber; +Cc: Junio C Hamano, git, Jeff King
In-Reply-To: <4E8EED39.1060607@drmicha.warpmail.net>
Michael J Gruber wrote:
> Jonathan Nieder venit, vidit, dixit 07.10.2011 12:06:
>> [1] http://thread.gmane.org/gmane.comp.version-control.git/181953/focus=182000
>
> Funny to point me at a thread I participated in, when I'm saying this
> thread should have continued there ;)
Ah, sorry, I wasn't trying to initiate a debate. I provided the
pointer to save time for others who haven't looked up the thread
you were referring to yet.
I am surprised that you seem to have missed what I meant by "branch
names are local". I meant that what I call "master" is likely to be
very different from what you call "master". Yes, we share commits and
fetch them from each other, and I might even _decide_ that what I call
"master" will be similar to what you call "master". Luckily, in
practice people don't seem to feel constrained to decide so, and there
are many workflows where my "master" is not your "master" and does not
even contain commits that would be acceptable for your "master".
This matters, a lot, because there is no easy way to partition a
namespace of names descriptive for humans without a central authority
to negotiate conflicts.
^ permalink raw reply
* Re: [PATCH] Teach merge the '[-e|--edit]' option
From: Junio C Hamano @ 2011-10-07 19:40 UTC (permalink / raw)
To: Jay Soffian; +Cc: git
In-Reply-To: <CAG+J_DxrQCS8zn5KJ8HnpqShVbMw=zCbqDVa=w08EEibw=tsAA@mail.gmail.com>
Jay Soffian <jaysoffian@gmail.com> writes:
> The other inconsistencies I'm aware of between "merge --no-commit &&
> commit" vs "merge" on a clean merge are:
Perhaps you would want to add these to a list of todo items when gitwiki
comes back.
^ permalink raw reply
* Re: [PATCH] Teach merge the '[-e|--edit]' option
From: Jay Soffian @ 2011-10-07 19:22 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <7vobxsvd69.fsf@alter.siamese.dyndns.org>
On Fri, Oct 7, 2011 at 3:01 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Think about it. What I suggested does no way make the situation
> worse. Your patch _does_ make it worse by changing the hook behaviour
> between "merge -m 'foo'" vs "merge -m 'foo' -e"
I think it's arguable how -e should behave. With -e opening my editor,
now I really feel like I'm making a commit and would be surprised by
not having the various commit hooks run.
j.
^ permalink raw reply
* Re: [PATCH] Teach merge the '[-e|--edit]' option
From: Jay Soffian @ 2011-10-07 19:07 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <CAG+J_Dz7-tTdgT=cqoKhK+fAhmESLnp93yHyxOF_NOY5Wx01+w@mail.gmail.com>
On Fri, Oct 7, 2011 at 2:01 PM, Jay Soffian <jaysoffian@gmail.com> wrote:
> I actually think a better choice would be to remove commit_tree() from
> merge and always have it run commit externally. I'm not seriously
> suggesting that be done, but it would make git more consistent. But
> I'm not going to send in a patch which makes the situation worse.
The other inconsistencies I'm aware of between "merge --no-commit &&
commit" vs "merge" on a clean merge are:
* reflog
- merge uses either "Merge made by the '...' strategy." OR "In-index merge"
- commit uses "commit (merge) <subject>"
* hooks
- merge calls
1) "prepare-commit-msg MERGE_MSG merge"
2) "post-merge [0|1]" where [0|1] indicates a squash or not.
- commit calls
1) "pre-commit"
2) "prepare-commit-msg COMMIT_EDITMSG merge"
3) "commit-msg COMMIT_EDITMSG"
4) "post-commit"
* gc
- merge calls "git gc --auto" after a successful merge unless
--squash was used
- commit does not call "git gc --auto"
* diffstat: merge shows it, commit does not
j.
^ permalink raw reply
* Re: [PATCH] Teach merge the '[-e|--edit]' option
From: Junio C Hamano @ 2011-10-07 19:01 UTC (permalink / raw)
To: Jay Soffian; +Cc: git
In-Reply-To: <CAG+J_Dz7-tTdgT=cqoKhK+fAhmESLnp93yHyxOF_NOY5Wx01+w@mail.gmail.com>
Jay Soffian <jaysoffian@gmail.com> writes:
> I actually think a better choice would be to remove commit_tree() from
> merge and always have it run commit externally. I'm not seriously
> suggesting that be done, but it would make git more consistent. But
> I'm not going to send in a patch which makes the situation worse.
Think about it. What I suggested does no way make the situation
worse. Your patch _does_ make it worse by changing the hook behaviour
between "merge -m 'foo'" vs "merge -m 'foo' -e".
^ permalink raw reply
* Re: Scalable reference handling
From: Martin Fick @ 2011-10-07 18:51 UTC (permalink / raw)
To: Michael Haggerty; +Cc: git
In-Reply-To: <4E8F2012.90108@alum.mit.edu>
On Friday, October 07, 2011 09:51:46 am Michael Haggerty
wrote:
> I can't write more now, but Martin, if you have time to
> benchmark 9944c7faf903a95d4ed9de284ace32debe21cdc1
> against your repository, I would be very interested to
> learn the results.
The fetch no longer seems to suffer from the large
regression, it is now faster (~7m) than 1.7.7 (which was
+15m).
As a quick note, if I comment out the
invalidate_cached_refs() call in write_ref_sha1() on line
2065 (on top of 9944c7), it is still much faster, only ~2m.
Perhaps growing the array on the fly with many refs is still
be too inefficient?
-Martin
--
Employee of Qualcomm Innovation Center, Inc. which is a
member of Code Aurora Forum
^ permalink raw reply
* Re: git remote doesn't show remotes from .git/remotes
From: Junio C Hamano @ 2011-10-07 18:08 UTC (permalink / raw)
To: Jeff King; +Cc: Kirill Likhodedov, git
In-Reply-To: <20111007150423.GA2076@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> On Thu, Oct 06, 2011 at 07:33:23PM +0400, Kirill Likhodedov wrote:
>
>> It seems that 'git remote' doesn't display remotes registered not in
>> .git/config but in .git/remotes/.
>> Is it a bug?
>
> It seems to have been lost in 211c896 (Make git-remote a builtin,
> 2008-02-29).
Sad.
> I don't think there is a specific plan. They're kept for backwards
> compatibility. But really, there is no reason to be using them at all at
> this point.
I've been thinking about making a list of deprecations/deletions for Git
2.0. The only two requirements needed to be added to the list are that it
gets list concensus and it is backed by a solid patch (or patch series).
^ permalink raw reply
* Re: How pretty is pretty? git cat-file -p inconsistency
From: Junio C Hamano @ 2011-10-07 18:04 UTC (permalink / raw)
To: Michael J Gruber; +Cc: Git Mailing List
In-Reply-To: <4E8EBC00.90909@drmicha.warpmail.net>
Michael J Gruber <git@drmicha.warpmail.net> writes:
> That is, "cat file -p" pretty prints dates for tag objects but not for
> commit objects. In fact, "-p" on commit objects does not prettify at all
> compared to the raw content. Is that intentional?
"cat-file -p" is an ill-conceived half-ass afterthought, and I do not
think anybody sane considers it as part of the "plumbing" ultra stable
interface for machine consumption. See a0f15fa (Pretty-print tagger
dates., 2006-03-01).
> I'd suggest
> prettifying dates with "-p" for commit objects also.
Please make it so. It is your choice to do a patch to update this single
thing first, or to discuss the output with "-p" for all the other object
types at the same time to get the list concensus before proceeding.
Thanks.
^ permalink raw reply
* "Use of uninitialized value" running "git svn clone" when having svn tag branches
From: Luiz-Otavio Zorzella @ 2011-10-07 18:01 UTC (permalink / raw)
To: git
I'm trying to convert a project (hosted in googlecode.com) from svn to
git, using the "git svn clone" command, and I'm getting an "Use of
uninitialized value" error. Here's the truncated output:
$ git svn clone https://test-libraries-for-java.googlecode.com/svn
--no-metadata -A ~/tmp/authors.txt -t tags -b branches -T trunk
test-libraries-for-java
r1 = c3adafa93a420f19b1bcfb6765fe0eb90aaa751c (refs/remotes/trunk)
A .classpath
A .project
A COPYING
A build.properties
A build.xml
W: +empty_dir: trunk/src
[...]
r10 = 8d5d7fdebdb7f822388fd3e4f4061abbfd1fb0cf (refs/remotes/trunk)
M test/com/google/common/testing/junit3/JUnitAssertsTest.java
r11 = 4c8a77660bf353ed55c9d583b39e263203c685a4 (refs/remotes/trunk)
Found possible branch point:
https://test-libraries-for-java.googlecode.com/svn/trunk =>
https://test-libraries-for-java.googlecode.com/svn/tags/release-1.0,
11
Use of uninitialized value $u in substitution (s///) at
/usr/lib/git-core/git-svn line 1731.
Use of uninitialized value $u in concatenation (.) or string at
/usr/lib/git-core/git-svn line 1731.
refs/remotes/trunk:
'https://test-libraries-for-java.googlecode.com/svn' not found in ''
For completeness, here's the authors.txt file I'm using:
$ cat ~/tmp/authors.txt
zorzella = Luiz-Otavio 'Z' Zorzella <zorzella@gmail.com>
(no author) = Luiz-Otavio 'Z' Zorzella <zorzella@gmail.com>
**************
The same command works fine if I don't use the "-t tags" part, but
then it does not create the tag branches in my converted git.
Thanks in advance,
Z
^ permalink raw reply
* Re: [PATCH] Teach merge the '[-e|--edit]' option
From: Jay Soffian @ 2011-10-07 18:01 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <7vk48gwvyd.fsf@alter.siamese.dyndns.org>
On Fri, Oct 7, 2011 at 1:30 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Jay Soffian <jaysoffian@gmail.com> writes:
>
>> Implement "git merge [-e|--edit]" as "git merge --no-commit && git commit"
>> as a convenience for the user.
>>
>> Signed-off-by: Jay Soffian <jaysoffian@gmail.com>
>> ---
>> ...
>> @@ -1447,6 +1457,10 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
>> }
>>
>> if (merge_was_ok) {
>> + if (option_edit) {
>> + const char *args[] = {"commit", "-e", NULL};
>> + return run_command_v_opt(args, RUN_GIT_CMD);
>> + }
>> fprintf(stderr, _("Automatic merge went well; "
>> "stopped before committing as requested\n"));
>> return 0;
>
>
> I wanted to like this approach, thinking this approach might be safer and
> with the least chance of breaking other codepaths, but this feels like an
> ugly hack.
>
> Are we still honoring all the hooks "git merge" honors? More importantly,
> isn't this make it impossible for future maintainers of this command to
> enhance the command by adding other hooks after the commit is made?
Git is already inconsistent with respect to which hooks are called
when. Shouldn't post-merge be called on a merge commit regardless of
whether you use --no-commit or not? Well, it isn't, it's only called
when merge performs the commit internally. The post-merge hook was
probably a mistake -- git calls the post-commit hook passing the
context as an argument, so probably merge should just call post-commit
"merge". But that ship has sailed.
See also 65969d43d1 (merge: honor prepare-commit-msg hook, 2011-02-14).
> If we wanted to do this properly, we should update builtin/merge.c to call
> launch_editor() before it runs commit_tree(), in a way similar to how
> prepare_to_commit() in builtin/commit.c does so when e.g. "commit -m foo -e"
> is run. An editmsg is prepared (you already have it in MERGE_MSG), the
> editor is allowed to update it, and then the original code before such a
> patch will run using the updated contents of MERGE_MSG. That way, the _only_
> change in behaviour when "-e" is used is to let the user update the message
> used in the commit log, and everything else would run exactly the same way
> as if no "-e" was given, including the invocation of hooks.
I find git very difficult to reason about (and inconsistent in its
behavior) due to piecemeal hoisting of some functionality into
porcelain commands (another example, revert.c building in the
recursive merge strategy but not any others).
I actually think a better choice would be to remove commit_tree() from
merge and always have it run commit externally. I'm not seriously
suggesting that be done, but it would make git more consistent. But
I'm not going to send in a patch which makes the situation worse.
j.
^ permalink raw reply
* Re: [PATCH WIP 0/3] git log --exclude
From: Junio C Hamano @ 2011-10-07 17:57 UTC (permalink / raw)
To: Nguyen Thai Ngoc Duy; +Cc: git, Jonathan Nieder
In-Reply-To: <CACsJy8DJs_cmCZegyPk=tB-bxWp4izrsTn8T=xeV1sU4fS5oTQ@mail.gmail.com>
Nguyen Thai Ngoc Duy <pclouds@gmail.com> writes:
> There may be a solution to mix depth limit and negative pathspec. I
> haven't thought carefully about that because I lean towards a simpler
> behaviour that only allows one negation level: a file is in if it
> matches any positive pathspecs and none of negative ones.
I tend to think it probably is acceptable solution to define "struct
pathspec" to have one positive and one negative set, traversing and
declaring a match on what matches something from the positive set only if
it does not match anything in the negative set.
That is similar to how we define revision ranges in the sense that the
range notation to have two ranges (A..B C..D) does not mean union of two
sets (A..B + C..D), but is difference between two unions ((B D)-(A C)).
> This is enough if narrow clones consist of positive pathspec only. I
> really doubt if users would want to make a narrow clone that "contains
> A but not A/B, storage-wise".
And if you define "struct pathspec" to have one positive and one negative
set, you do not have to limit narrow clone only to positive. The repository
specific narrow pathspec will have one such "struct pathspec", but the
user may give us from the command line another "struct pathspec". At
runtime, we would merge them to form into one negative and one positive
set, and apply the same rule: nothing that matches any negative will
appear in the traversal or in the output.
^ permalink raw reply
* unexpected behavior with `git log --skip filename`
From: Andrew McNabb @ 2011-10-07 17:15 UTC (permalink / raw)
To: git
The "--skip" option to "git log" did not behave as I expected, but I'm
not sure whether this was user error, unclear documentation, or a bug.
Specifically, I ran the following, intending to find the previous
revision of a given file:
git log --skip=1 -n 1 --oneline some-filename
My expectation was that this would behave the same as:
git log -n 2 --oneline some-filename |tail -n 1
Instead, the --skip=1 parameter seemed to be ignored. After I tried
several different values, it appears that the commits are skipped before
path matching with "some-filename".
Is this the intended behavior? If so, should the documentation be
clarified by changing "Note that they are applied before commit ordering
and formatting options, such as --reverse" to something like "Note that
they are applied before path matching, commit ordering, and formatting
options, such as --reverse"?
--
Andrew McNabb
http://www.mcnabbs.org/andrew/
PGP Fingerprint: 8A17 B57C 6879 1863 DE55 8012 AB4D 6098 8826 6868
^ permalink raw reply
* Re: [PATCH] Teach merge the '[-e|--edit]' option
From: Junio C Hamano @ 2011-10-07 17:30 UTC (permalink / raw)
To: Jay Soffian; +Cc: git
In-Reply-To: <1318001347-11347-1-git-send-email-jaysoffian@gmail.com>
Jay Soffian <jaysoffian@gmail.com> writes:
> Implement "git merge [-e|--edit]" as "git merge --no-commit && git commit"
> as a convenience for the user.
>
> Signed-off-by: Jay Soffian <jaysoffian@gmail.com>
> ---
> ...
> @@ -1447,6 +1457,10 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
> }
>
> if (merge_was_ok) {
> + if (option_edit) {
> + const char *args[] = {"commit", "-e", NULL};
> + return run_command_v_opt(args, RUN_GIT_CMD);
> + }
> fprintf(stderr, _("Automatic merge went well; "
> "stopped before committing as requested\n"));
> return 0;
I wanted to like this approach, thinking this approach might be safer and
with the least chance of breaking other codepaths, but this feels like an
ugly hack.
Are we still honoring all the hooks "git merge" honors? More importantly,
isn't this make it impossible for future maintainers of this command to
enhance the command by adding other hooks after the commit is made?
If we wanted to do this properly, we should update builtin/merge.c to call
launch_editor() before it runs commit_tree(), in a way similar to how
prepare_to_commit() in builtin/commit.c does so when e.g. "commit -m foo -e"
is run. An editmsg is prepared (you already have it in MERGE_MSG), the
editor is allowed to update it, and then the original code before such a
patch will run using the updated contents of MERGE_MSG. That way, the _only_
change in behaviour when "-e" is used is to let the user update the message
used in the commit log, and everything else would run exactly the same way
as if no "-e" was given, including the invocation of hooks.
^ permalink raw reply
* Re: [PATCH 3/3] fetch: treat --tags like refs/tags/*:refs/tags/* when pruning
From: Carlos Martín Nieto @ 2011-10-07 16:40 UTC (permalink / raw)
To: Jeff King; +Cc: git, Junio C Hamano, mathstuf
In-Reply-To: <20111007163319.GC4399@sigill.intra.peff.net>
[-- Attachment #1: Type: text/plain, Size: 1392 bytes --]
On Fri, 2011-10-07 at 12:33 -0400, Jeff King wrote:
> On Thu, Oct 06, 2011 at 11:21:47PM +0200, Carlos Martín Nieto wrote:
>
> > diff --git a/builtin/fetch.c b/builtin/fetch.c
> > index b937d71..94b2bd3 100644
> > --- a/builtin/fetch.c
> > +++ b/builtin/fetch.c
> > @@ -699,8 +699,17 @@ static int do_fetch(struct transport *transport,
> > free_refs(ref_map);
> > return 1;
> > }
> > - if (prune)
> > + if (prune) {
> > + /* If --tags was specified, we need to tell prune_refs
> > + * that we're filtering the refs from the remote */
> > + if (tags == TAGS_SET) {
> > + const char * tags_refspec = "refs/tags/*:refs/tags/*";
> > + refs = xrealloc(refs, (ref_count + 1) * sizeof(struct refspec));
> > + refs[ref_count] = *parse_fetch_refspec(1, &tags_refspec);
> > + ref_count++;
> > + }
> > prune_refs(transport, refs, ref_count, ref_map);
> > + }
>
> I don't think we can realloc refs here. It's passed into do_fetch. When
> we realloc it, the old pointer value will be invalid. But when we return
> from do_fetch, the caller (fetch_one) will still have that old value,
> and will call free() on it.
Yes, you're right. I guess it's been working by luck and generous amount
of memory.
>
> Instead, you have to make a whole new list, copy the old values in, add
> your new one, and then free the result.
Will do.
cmn
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 490 bytes --]
^ permalink raw reply
* Re: [PATCH 2/3] fetch: honor the user-provided refspecs when pruning refs
From: Jeff King @ 2011-10-07 16:39 UTC (permalink / raw)
To: Carlos Martín Nieto; +Cc: git, Junio C Hamano, mathstuf
In-Reply-To: <1318005433.4579.5.camel@centaur.lab.cmartin.tk>
On Fri, Oct 07, 2011 at 06:37:13PM +0200, Carlos Martín Nieto wrote:
> > I assume you mean s/tag/branch/ in the last line?
>
> Yeah, maybe ref would be better?
Yeah, agreed.
> > Tests?
>
> Good point.
It sounds like you already have a reproduction recipe for this, and for
the --tags thing in the next commit.
> OK, so take a step back and figure out what we want the rules to be
> before we call get_stale_heads. It does sound like a more elegant
> approach. I was trying to disrupt the callers as little as possible, but
> then again, there's only two. Will change.
Yeah. Sometimes we try hard to make a minimal patch, because it makes it
easier to review. At the same time, I think it's more important to make
the code clean if it needs it. Especially when there aren't many callers
to disrupt.
-Peff
^ permalink raw reply
* Re: [PATCH 2/3] fetch: honor the user-provided refspecs when pruning refs
From: Carlos Martín Nieto @ 2011-10-07 16:37 UTC (permalink / raw)
To: Jeff King; +Cc: git, Junio C Hamano, mathstuf
In-Reply-To: <20111007162625.GB4399@sigill.intra.peff.net>
[-- Attachment #1: Type: text/plain, Size: 2471 bytes --]
On Fri, 2011-10-07 at 12:26 -0400, Jeff King wrote:
> On Thu, Oct 06, 2011 at 11:21:46PM +0200, Carlos Martín Nieto wrote:
>
> > If the user gave us refspecs on the command line, we should use those
> > when deciding whether to prune a ref instead of relying on the
> > refspecs in the config.
> >
> > Previously, running
> >
> > git fetch --prune origin refs/heads/master:refs/remotes/origin/master
> >
> > would delete every other tag under the origin namespace because we
>
> I assume you mean s/tag/branch/ in the last line?
Yeah, maybe ref would be better?
>
> > ---
> > builtin/fetch.c | 6 ++--
> > builtin/remote.c | 2 +-
> > remote.c | 78 +++++++++++++++++++++++++++++++++++++++++++++++++-----
> > remote.h | 3 +-
> > 4 files changed, 77 insertions(+), 12 deletions(-)
>
> Tests?
Good point.
>
> > static int get_stale_heads_cb(const char *refname,
> > const unsigned char *sha1, int flags, void *cb_data)
> > {
> > struct stale_heads_info *info = cb_data;
> > struct refspec refspec;
> > + int ret;
> > memset(&refspec, 0, sizeof(refspec));
> > refspec.dst = (char *)refname;
> > - if (!remote_find_tracking(info->remote, &refspec)) {
> > - if (!((flags & REF_ISSYMREF) ||
> > - string_list_has_string(info->ref_names, refspec.src))) {
> > - struct ref *ref = make_linked_ref(refname, &info->stale_refs_tail);
> > - hashcpy(ref->new_sha1, sha1);
> > - }
> > +
> > + /*
> > + * If the user speicified refspecs on the command line, we
> > + * should only use those to check. Otherwise, look in the
> > + * remote's configuration for the branch.
> > + */
> > + if (info->ref_count)
> > + ret = find_in_refs(info->refs, info->ref_count, &refspec);
> > + else
> > + ret = remote_find_tracking(info->remote, &refspec);
>
> Minor typo in the comment. But more importantly, this feels like a very
> low-level place to be thinking about things like what the user gave us
> on the command line.
>
> Shouldn't get_stale_heads not get a remote at all, and just get a set of
> refspecs? Those should be the minimal information it needs to get its
> answer, right?
OK, so take a step back and figure out what we want the rules to be
before we call get_stale_heads. It does sound like a more elegant
approach. I was trying to disrupt the callers as little as possible, but
then again, there's only two. Will change.
cmn
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 490 bytes --]
^ permalink raw reply
* Re: [PATCH 3/3] fetch: treat --tags like refs/tags/*:refs/tags/* when pruning
From: Jeff King @ 2011-10-07 16:33 UTC (permalink / raw)
To: Carlos Martín Nieto; +Cc: git, Junio C Hamano, mathstuf
In-Reply-To: <1317936107-1230-4-git-send-email-cmn@elego.de>
On Thu, Oct 06, 2011 at 11:21:47PM +0200, Carlos Martín Nieto wrote:
> diff --git a/builtin/fetch.c b/builtin/fetch.c
> index b937d71..94b2bd3 100644
> --- a/builtin/fetch.c
> +++ b/builtin/fetch.c
> @@ -699,8 +699,17 @@ static int do_fetch(struct transport *transport,
> free_refs(ref_map);
> return 1;
> }
> - if (prune)
> + if (prune) {
> + /* If --tags was specified, we need to tell prune_refs
> + * that we're filtering the refs from the remote */
> + if (tags == TAGS_SET) {
> + const char * tags_refspec = "refs/tags/*:refs/tags/*";
> + refs = xrealloc(refs, (ref_count + 1) * sizeof(struct refspec));
> + refs[ref_count] = *parse_fetch_refspec(1, &tags_refspec);
> + ref_count++;
> + }
> prune_refs(transport, refs, ref_count, ref_map);
> + }
I don't think we can realloc refs here. It's passed into do_fetch. When
we realloc it, the old pointer value will be invalid. But when we return
from do_fetch, the caller (fetch_one) will still have that old value,
and will call free() on it.
Instead, you have to make a whole new list, copy the old values in, add
your new one, and then free the result.
-Peff
^ permalink raw reply
* Re: [PATCH 2/3] fetch: honor the user-provided refspecs when pruning refs
From: Jeff King @ 2011-10-07 16:26 UTC (permalink / raw)
To: Carlos Martín Nieto; +Cc: git, Junio C Hamano, mathstuf
In-Reply-To: <1317936107-1230-3-git-send-email-cmn@elego.de>
On Thu, Oct 06, 2011 at 11:21:46PM +0200, Carlos Martín Nieto wrote:
> If the user gave us refspecs on the command line, we should use those
> when deciding whether to prune a ref instead of relying on the
> refspecs in the config.
>
> Previously, running
>
> git fetch --prune origin refs/heads/master:refs/remotes/origin/master
>
> would delete every other tag under the origin namespace because we
I assume you mean s/tag/branch/ in the last line?
> ---
> builtin/fetch.c | 6 ++--
> builtin/remote.c | 2 +-
> remote.c | 78 +++++++++++++++++++++++++++++++++++++++++++++++++-----
> remote.h | 3 +-
> 4 files changed, 77 insertions(+), 12 deletions(-)
Tests?
> static int get_stale_heads_cb(const char *refname,
> const unsigned char *sha1, int flags, void *cb_data)
> {
> struct stale_heads_info *info = cb_data;
> struct refspec refspec;
> + int ret;
> memset(&refspec, 0, sizeof(refspec));
> refspec.dst = (char *)refname;
> - if (!remote_find_tracking(info->remote, &refspec)) {
> - if (!((flags & REF_ISSYMREF) ||
> - string_list_has_string(info->ref_names, refspec.src))) {
> - struct ref *ref = make_linked_ref(refname, &info->stale_refs_tail);
> - hashcpy(ref->new_sha1, sha1);
> - }
> +
> + /*
> + * If the user speicified refspecs on the command line, we
> + * should only use those to check. Otherwise, look in the
> + * remote's configuration for the branch.
> + */
> + if (info->ref_count)
> + ret = find_in_refs(info->refs, info->ref_count, &refspec);
> + else
> + ret = remote_find_tracking(info->remote, &refspec);
Minor typo in the comment. But more importantly, this feels like a very
low-level place to be thinking about things like what the user gave us
on the command line.
Shouldn't get_stale_heads not get a remote at all, and just get a set of
refspecs? Those should be the minimal information it needs to get its
answer, right?
-Peff
^ permalink raw reply
* Re: [PATCH] show git tag output in pager
From: Jeff King @ 2011-10-07 16:16 UTC (permalink / raw)
To: Matthieu Moy; +Cc: Michal Vyskocil, git
In-Reply-To: <vpqsjn4dfi4.fsf@bauges.imag.fr>
On Fri, Oct 07, 2011 at 04:48:35PM +0200, Matthieu Moy wrote:
> > On Mon, Oct 03, 2011 at 02:57:09PM +0200, Matthieu Moy wrote:
> >
> >> I like the try_subcommand_pager idea. Ideally, there would also be a
> >> nice mechanism to set defaults for subcommands, so that "git tag
> >> <whatever>" does the right thing without configuration.
> >
> > That's easy enough. Something like the patch below?
>
> It may have been better with a big centralized array of configurations
> like
>
> static struct cmd_struct commands[] = {
> { "add", cmd_add, RUN_SETUP | NEED_WORK_TREE },
> ...
>
> in git.c, but if we have only a few instances of this, your system is
> probably fine. I like it.
I don't think you can centralize it in the same way, because some of it
will have to be implemented in shell script (unlike the full-command
ones, which we can always trigger at the git.c wrapper layer).
So you could have:
static struct pager_default subcommands[] = {
{ "tag.list", 1 },
{ "branch.list", 1 },
};
but you'll never be able to put "stash.list" into that structure (and as
you probably guessed, my patch isn't enough to implement stash.list,
either; it would need a shell implementation of try_subcommand_pager).
-Peff
^ permalink raw reply
* Scalable reference handling
From: Michael Haggerty @ 2011-10-07 15:51 UTC (permalink / raw)
To: Martin Fick; +Cc: git
In-Reply-To: <4E8E6E8E.5070909@alum.mit.edu>
[-- Attachment #1: Type: text/plain, Size: 1666 bytes --]
On 10/07/2011 05:14 AM, Michael Haggerty wrote:
> On 10/07/2011 12:16 AM, Martin Fick wrote:
>> I downloaded your patch series and tested it on my repos.
>
> Very cool (though a bit premature, as you discovered). The patch series
> still has a known performance regression in the area of
> do_for_each_ref(), which I hope to figure out soon. I will definitely
> tell you when I think that the patch series is ready for more serious
> testing (hopefully today) in the hopes that you can benchmark it against
> your repo.
I just pushed versions to github that I think are ready for some
preliminary testing. There were some silly inefficiencies in the
version that you tested earlier, so this version is considerably faster
in a few key tests.
I don't have complete benchmarking results, but I have attached what I
have. I wouldn't put much weight on small differences in the numbers
because the computer was not 100% quiescent while I ran the tests. But
I think the results are impressive: the new code (columns 5-8) is a bit
slower in only a few cases but faster (sometimes by a large factor) in
many other cases.
I can't write more now, but Martin, if you have time to benchmark
9944c7faf903a95d4ed9de284ace32debe21cdc1 against your repository, I
would be very interested to learn the results.
BTW I am not asking anybody to review the patch series yet; I would like
to do some more tests and cleanup first. But of course I wouldn't
object to feedback. A good starting point would be the comments at the
top of refs.c, where the basic data structures are explained.
Michael
--
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/
[-- Attachment #2: refperf-summary-3.out --]
[-- Type: text/plain, Size: 4983 bytes --]
=================================== ======= ======= ======= ======= ======= ======= ======= ======= =======
Test name [0] [1] [2] [3] [4] [5] [6] [7] [8]
=================================== ======= ======= ======= ======= ======= ======= ======= ======= =======
branch-loose-cold 5.14 5.80 4.98 4.55 5.03 0.41 0.45 0.45 0.51
branch-loose-warm 0.10 0.12 0.12 0.19 0.11 0.02 0.02 0.03 0.02
for-each-ref-loose-cold 6.02 6.36 7.48 5.62 6.27 5.32 5.02 5.24 5.01
for-each-ref-loose-warm 0.25 0.25 0.27 0.25 0.25 0.26 0.26 0.25 0.26
checkout-loose-cold 5.47 5.03 5.66 5.39 5.72 0.65 0.77 0.95 0.59
checkout-loose-warm 0.11 0.11 0.11 0.11 0.11 0.02 0.03 0.03 0.03
checkout-orphan-loose 0.09 0.09 0.10 0.10 0.10 0.03 0.01 0.01 0.02
checkout-from-detached-loose-cold N/A N/A N/A N/A N/A N/A N/A N/A N/A
checkout-from-detached-loose-warm N/A N/A N/A N/A N/A N/A N/A N/A N/A
branch-contains-loose-cold 14.69 13.95 13.76 13.84 14.07 14.08 14.12 14.01 14.77
branch-contains-loose-warm 8.99 8.89 8.74 8.82 8.80 8.81 8.84 8.76 8.91
pack-refs-loose 1.23 1.02 1.02 1.02 1.01 1.02 1.00 0.99 1.03
branch-packed-cold 0.66 0.65 0.84 0.67 0.52 0.76 0.58 0.55 0.59
branch-packed-warm 0.01 0.01 0.03 0.03 0.02 0.02 0.05 0.02 0.05
for-each-ref-packed-cold 1.38 1.29 1.07 1.13 1.05 1.08 1.26 1.05 1.27
for-each-ref-packed-warm 0.17 0.16 0.17 0.17 0.17 0.16 0.17 0.16 0.16
checkout-packed-cold 8.74 7.75 7.75 1.59 1.61 1.79 1.54 1.55 1.53
checkout-packed-warm 0.03 0.05 0.04 0.04 0.05 0.05 0.03 0.05 0.06
checkout-orphan-packed 0.04 0.01 0.01 0.03 0.02 0.02 0.05 0.05 0.02
checkout-from-detached-packed-cold 8.88 8.22 8.12 1.98 1.81 1.94 1.99 1.88 1.97
checkout-from-detached-packed-warm 6.55 6.43 6.44 0.44 0.46 0.45 0.46 0.46 0.49
branch-contains-packed-cold 11.59 10.73 10.37 9.42 9.24 9.40 9.37 9.56 9.34
branch-contains-packed-warm 10.20 9.79 9.83 8.68 8.61 8.68 8.61 8.82 9.33
clone-loose-cold 105.58 86.98 89.42 88.96 88.19 87.01 87.20 87.11 88.20
clone-loose-warm 3.26 3.11 3.18 3.10 3.17 3.14 3.14 3.21 3.16
fetch-nothing-loose 0.85 0.84 0.86 0.85 0.84 0.88 0.84 0.84 0.84
pack-refs 0.12 0.14 0.16 0.13 0.15 0.14 0.14 0.15 0.14
fetch-nothing-packed 0.84 0.84 0.86 0.85 0.89 0.85 0.84 0.84 0.83
clone-packed-cold 2.72 2.33 2.11 2.19 2.23 2.14 2.24 2.01 2.16
clone-packed-warm 0.40 0.40 0.33 0.40 0.28 0.29 0.33 0.31 0.31
fetch-everything-cold 106.28 92.92 94.12 88.76 91.62 88.75 89.49 89.78 91.43
fetch-everything-warm 5.56 5.63 5.70 5.62 5.63 5.66 5.69 5.71 5.78
=================================== ======= ======= ======= ======= ======= ======= ======= ======= =======
[0] v1.7.6 (refperf.times.d78c84e8698e750139667bc724b08eb34e795b65)
[1] v1.7.7 (refperf.times.a258e475eb74e183e9e68ca30e32c5253081356d)
[2] origin/master (refperf.times.27897d25f1b36d400b82b655701b87fd205dbc2f)
[3] 16583974c20b856bb60b5a733020425c16a19670^ (refperf.times.558b49c8489c95cf966b959c3444c95d177dc4dc)
[4] 16583974c20b856bb60b5a733020425c16a19670 (refperf.times.16583974c20b856bb60b5a733020425c16a19670)
[5] origin/hierarchical-refs^^^^ (refperf.times.5f5a126553eef88455f7deb2745c5f93073bfe69)
[6] origin/hierarchical-refs^^^ (refperf.times.a306af145856f8296bf2ff4a3ace5e86ac3b1fc8)
[7] origin/hierarchical-refs^ (refperf.times.fd53cf7a7fcc360f30ea0ec5b964cefeec70c11b)
[8] origin/hierarchical-refs (refperf.times.9944c7faf903a95d4ed9de284ace32debe21cdc1)
^ permalink raw reply
* Re: git remote doesn't show remotes from .git/remotes
From: Kirill Likhodedov @ 2011-10-07 15:49 UTC (permalink / raw)
To: Jeff King; +Cc: git
In-Reply-To: <20111007150423.GA2076@sigill.intra.peff.net>
07.10.2011, в 19:04, Jeff King wrote
> I'm not sure how much we
> care, though. We haven't generated .git/remotes files in a long time,
> and this is the first notice of the bug after 3.5 years. Is this an old
> repo that has remotes, or are you wondering if you should use them in a
> new repo?
>
No, I never came across any usages of .git/remotes.
I am writing a Git integration plugin for an IDE, and was studying documentation for git-remote where I saw this method to define remotes.
So I've just decided to report the lack of the 'git remote' output and make sure that I didn't miss anything. And also decide if I have to support them in the product - probably, not.
I agree that the bug is not worth fixing since nobody uses .git/remotes anyway.
Thanks for the clarification.
^ permalink raw reply
* [PATCH] Teach merge the '[-e|--edit]' option
From: Jay Soffian @ 2011-10-07 15:29 UTC (permalink / raw)
To: git; +Cc: Jay Soffian, Junio C Hamano, Todd A. Jacobs
Implement "git merge [-e|--edit]" as "git merge --no-commit && git commit"
as a convenience for the user.
Signed-off-by: Jay Soffian <jaysoffian@gmail.com>
---
Documentation/merge-options.txt | 6 ++++++
builtin/merge.c | 14 ++++++++++++++
t/t7600-merge.sh | 15 +++++++++++++++
3 files changed, 35 insertions(+), 0 deletions(-)
diff --git a/Documentation/merge-options.txt b/Documentation/merge-options.txt
index b613d4ed08..6bd0b041c3 100644
--- a/Documentation/merge-options.txt
+++ b/Documentation/merge-options.txt
@@ -7,6 +7,12 @@ With --no-commit perform the merge but pretend the merge
failed and do not autocommit, to give the user a chance to
inspect and further tweak the merge result before committing.
+--edit::
+-e::
++
+ Invoke editor before committing successful merge to further
+ edit the default merge message.
+
--ff::
--no-ff::
Do not generate a merge commit if the merge resolved as
diff --git a/builtin/merge.c b/builtin/merge.c
index ee56974371..815e151487 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -46,6 +46,7 @@ static const char * const builtin_merge_usage[] = {
static int show_diffstat = 1, shortlog_len, squash;
static int option_commit = 1, allow_fast_forward = 1;
+static int option_edit = 0;
static int fast_forward_only;
static int allow_trivial = 1, have_message;
static struct strbuf merge_msg;
@@ -190,6 +191,8 @@ static struct option builtin_merge_options[] = {
"create a single commit instead of doing a merge"),
OPT_BOOLEAN(0, "commit", &option_commit,
"perform a commit if the merge succeeds (default)"),
+ OPT_BOOLEAN('e', "edit", &option_edit,
+ "edit message before committing"),
OPT_BOOLEAN(0, "ff", &allow_fast_forward,
"allow fast-forward (default)"),
OPT_BOOLEAN(0, "ff-only", &fast_forward_only,
@@ -1092,6 +1095,13 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
option_commit = 0;
}
+ /* if not committing, edit is nonsensical */
+ if (!option_commit)
+ option_edit = 0;
+ /* if editing, invoke 'git commit -e' after successful merge */
+ if (option_edit)
+ option_commit = 0;
+
if (!allow_fast_forward && fast_forward_only)
die(_("You cannot combine --no-ff with --ff-only."));
@@ -1447,6 +1457,10 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
}
if (merge_was_ok) {
+ if (option_edit) {
+ const char *args[] = {"commit", "-e", NULL};
+ return run_command_v_opt(args, RUN_GIT_CMD);
+ }
fprintf(stderr, _("Automatic merge went well; "
"stopped before committing as requested\n"));
return 0;
diff --git a/t/t7600-merge.sh b/t/t7600-merge.sh
index 87aac835a1..8c6b811718 100755
--- a/t/t7600-merge.sh
+++ b/t/t7600-merge.sh
@@ -643,4 +643,19 @@ test_expect_success 'amending no-ff merge commit' '
test_debug 'git log --graph --decorate --oneline --all'
+cat >editor <<\EOF
+#!/bin/sh
+# strip comments and blank lines from end of message
+sed -e '/^#/d' < "$1" | sed -e :a -e '/^\n*$/{$d;N;ba' -e '}' > expected
+EOF
+chmod 755 editor
+
+test_expect_success 'merge --no-ff --edit' '
+ git reset --hard c0 &&
+ EDITOR=./editor git merge --no-ff --edit c1 &&
+ verify_parents $c0 $c1 &&
+ git cat-file commit HEAD | sed "1,/^$/d" > actual &&
+ test_cmp actual expected
+'
+
test_done
--
1.7.7.147.g00fdf
^ permalink raw reply related
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