* STGIT: Deathpatch in linus tree
From: "Andy Green (林安廸)" @ 2012-02-07 13:02 UTC (permalink / raw)
To: git
Hi -
Linus just pushed 105e5180936d69b1aee46ead8a5fc6c68f4d5f65 to
linux-2.6... along the lines of the Monty Python joke that was so funny
it kills anyone who hears it, if I have a stgit branch based on a HEAD
that includes this commit then stgit dies when pushing on top of it.
So...
[agreen@build linux-2.6]$ stg pop --all
[agreen@build linux-2.6]$ git reset --hard 96e02d1
HEAD is now at 96e02d1 exec: fix use-after-free bug in setup_new_exec()
[agreen@build linux-2.6]$ stg push
Pushing patch "subject-patch-1-3-arm-dt-add-p" ... done (empty)
Now at patch "subject-patch-1-3-arm-dt-add-p"
[agreen@build linux-2.6]$ stg pop
Popped subject-patch-1-3-arm-dt-add-p
No patch applied
So the commit just before the bad guy is happy. Then -->
[agreen@build linux-2.6]$ git reset --hard 105e518
HEAD is now at 105e518 Merge tag 'hwmon-fixes-for-3.3-rc3' of
git://git.kernel.org/pub/scm/linux/kernel/git/groeck/linux-staging
[agreen@build linux-2.6]$ stg push
Error: Unhandled exception:
Traceback (most recent call last):
File "/usr/lib/python2.7/site-packages/stgit/main.py", line 152, in _main
ret = command.func(parser, options, args)
File "/usr/lib/python2.7/site-packages/stgit/commands/push.py", line
68, in func
check_clean_iw = clean_iw)
File "/usr/lib/python2.7/site-packages/stgit/lib/transaction.py", line
95, in __init__
self.__current_tree = self.__stack.head.data.tree
File "/usr/lib/python2.7/site-packages/stgit/lib/git.py", line 426, in
data
self.__repository.cat_object(self.sha1))
File "/usr/lib/python2.7/site-packages/stgit/lib/git.py", line 408, in
parse
assert False
AssertionError
It also dies if I use Linus' current HEAD as the basis I am stgitting on
top of, which is one patch ahead of the deathpatch.
I'm using Fedora rawhide versions of git and stgit
git-1.7.9-1.fc17.x86_64
stgit-0.15-2.fc17.noarch
-Andy
^ permalink raw reply
* Re: Git performance results on a large repository
From: Nguyen Thai Ngoc Duy @ 2012-02-07 13:43 UTC (permalink / raw)
To: Joey Hess; +Cc: git@vger.kernel.org
In-Reply-To: <20120206154043.GA14632@gnu.kitenet.net>
On Mon, Feb 6, 2012 at 10:40 PM, Joey Hess <joey@kitenet.net> wrote:
>> Someone on HN suggested making assume-unchanged files read-only to
>> avoid 90% accidentally changing a file without telling git. When
>> assume-unchanged bit is cleared, the file is made read-write again.
>
> That made me think about using assume-unchanged with git-annex since it
> already has read-only files.
>
> But, here's what seems a misfeature...
because, well.. assume-unchanged was designed to avoid stat() and
nothing else. We are basing a new feature on top of it.
> If an assume-unstaged file has
> modifications and I git add it, nothing happens. To stage a change, I
> have to explicitly git update-index --no-assume-unchanged and only then
> git add, and then I need to remember to reset the assume-unstaged bit
> when I'm done working on that file for now. Compare with running git mv
> on the same file, which does stage the move despite assume-unstaged. (So
> does git rm.)
This is normal in the lock-based "checkout/edit/checkin" model. mv/rm
operates on directory content, which is not "locked - no edit allowed"
(in our case --assume-unchanged) in git. But lock-based model does not
map really well to git anyway. It does not have the index (which may
make things more complicated). Also at index level, git does not
really understand directories.
I think we could add a protection layer to index, where any changes
(including removal) to an index entry are only allowed if the entry is
"unlocked" (i.e no assume-unchanged bit). Locked entries are read-only
and have assume-unchanged bit set. "git (un)lock" are introduced as
new UI. Does that make assume-unchanged friendlier?
--
Duy
^ permalink raw reply
* Re: [RFD] Rewriting safety - warn before/when rewriting published history
From: Jakub Narebski @ 2012-02-07 14:31 UTC (permalink / raw)
To: Johan Herland; +Cc: Philip Oakley, git
In-Reply-To: <CALKQrgcUdigB5zB_bqgpW8=o-TuGChs+q2nYoXu5YdyWu+oWZw@mail.gmail.com>
On Mon, 6 Feb 2012, Johan Herland wrote:
> On Mon, Feb 6, 2012 at 18:14, Jakub Narebski <jnareb@gmail.com> wrote:
>> On Mon, 6 Feb 2012, Johan Herland wrote:
>>> On Mon, Feb 6, 2012 at 15:44, Jakub Narebski <jnareb@gmail.com> wrote:
[...]
>> Yes, starting with prototype implementation using existing infrastructure
>> (hooks) would be a very good idea. (That's how first versions of what
>> became submodules were implemented.)
>>
>> OTOH we should be aware of limitations of said prototype due to the fact
>> that it is a prototype...
>
> Agreed, but AFAICS (and modulo the addition of pre-rewrite and
> pre/post-push hooks mentioned earlier) all of the things discussed so
> far in this thread can be implemented as hooks.
That would be nice.
And the new hooks (pre-rewrite, pre/post-push) would be useful anyway,
I think.
[...]
>>> And trying to plug too many holes might end
>>> up annoying more experienced users who "know what they're doing".
>>
>> Second, forcing via command line parameter should always be an option.
>
> Obviously, but if a large portion of the Git community felt the need
> to always disable this feature, I'd say that we'd failed. The best
> features are those that Just Work(tm).
Well, we have some features in git like advice.* that cater to newbies.
This could be such newbie-mostly feature too.
Anyway what I was thinking about is that forcing is for those hopefully
rare cases where Git safety net is too strict, where DWIM-mery fails.
>>> Instead we might want to add a client-side check at push time. I
>>> realize that this check is already done by the remote end, but the
>>> client-side might be able to give a more helpful response along the
>>> lines of:
>>
>> [...]
>>
>> Explanation is good, but the whole idea of rewriting safety is that you
>> are informed (warned or denied) _before_ attempting rewrite and doing much
>> work.
>
> True, but there may be cases where the rewrite is not apparent until
> after it has happened. E.g. a novice user may use 'git reset --hard'
> in order to get to an earlier state for testing purposes, and then -
> after completing the test - 'git reset --hard' back to the starting
> point. I know this is not best practice, but is it bad enough that we
> want to refuse it?
I'd like to have safety net also for using 'git reset --hard' to rewind
publishable branch past published history. Though preventing e.g. force
rename of other branch to publishable branch so it doesn't fast-forward
WRT its upstream would be going to far, I guess...
>>> First, we don't need to annotate _all_ commits. For the 'public'
>>> state, we only annotate the last/tip commit that was pushed/published.
>>> From there, we can defer that all ancestor commits are also 'public'.
>>
>> Right.
>>
>>> For the 'secret' state, we do indeed annotate _all_ secret commits,
>>> but I believe this will be a somewhat limited number of commits. If
>>> your workflow forces you to annotate millions of commits as 'secret',
>>> I claim there is something wrong with your workflow.
>>
>> Well, for the 'secret' we can rely on the fact that child of 'secret'
>> commit must also be 'secret' (non-publishable) if secret is to stay
>> secret. Still marking all 'secret' commits might be better idea from
>> UI and from performance point of view.
>
> I don't think we should automatically assume that all children of a
> 'secret' commit are also 'secret'.
All right. What is important is that safety net related to 'secret'
commits would prevent publishing such commits even if you build non-secret
commits on top of 'secret' one.
> First of all, the git DAG was not
> made for iterating forwards in history, so given a 'secret' commit, it
> is computationally expensive to enumerate all its implied-'secret'
> descendants.
Right, this would enormously complicate detecting 'secret'-ness of commit
when browsing history / querying "phase" of a commit..
> More importantly though, I don't agree with the premise.
> I would typically use the 'secret' state as follows: While debugging a
> piece of code, I might commit a few debug print statements, and I
> would typically mark this debug commit as 'secret', in order to
> prevent myself from accidentally pushing this. Although it probably
> doesn't matter in practice, I think it is wrong for the commits made
> (temporarily) on top of this debug commit to be also considered
> 'secret'. They are only unpublishable as a consequence of being based
> on the debug commit, and only until I get around to rebasing away the
> 'secret' debug commit.
Right, thanks for sanity check.
So another difference that in its full complexity the 'secret' is an
attribute of an individual commit which is (usually) local to repository,
while 'public' is in its full complexity an attribute of revision walking
rather than something that can be attached to a commit.
Also, when thinking about different scenarios of why one would like to
mark commit as 'secret', we might want to be able to mark commit as
secret / unpublishable with respect to _subset_ of remotes, so e.g.
I am prevented from accidentally publishing commits marked as 'secret'
to public repository, or to CI/QA repository, but I can push (perhaps
with warning) to group repository, together with 'secret'-ness state
of said commit...
... though it wouldn't be as much 'secret' as 'confidential' ;-)
>>>> Second, I have doubts if "phase" is really state of an individual commit,
>>>> and not the feature of revision walking.
>>
>> It matters to presentation: can commit be simultaneously 'public' because
>> of one branch, and 'draft' because of other.
>>
>>> I believe the 'public' state is a "feature of revision walking" (i.e.
>>> one annotated 'public' commit implies that all its ancestors are also
>>> 'public'). However, the 'secret' state should be bound to the
>>> individual commit, IMHO.
>>
>> Good call, otherwise 'secret' commit could have been "side-leaked"
>> by other refs being pushed.
>>
>> This means though that 'public' / 'draft' while looking similar to 'secret'
>> are in fact a bit different things. In other words 'immutable' and
>> 'impushable' traits are quite a bit different in behavior...
>>
>> Especially that one acts at pre-rewrite time, and second pre-push time.
>
> Exactly. I find Mercurial 'phase' language confusing, precisely for
> the reason that 'public' and 'secret' are DIFFERENT concepts. One
> hinders rewrite and naturally applies to a commit AND its ancestors,
> while the other hinders push and only applies to the commit itself.
> The fact that they could be implemented by the same mechanisms (hooks
> and notes) does not make them the same thing.
Well, if one doesn't think about all possible usages and complications,
the simplicity of metaphor of "phases" is quite compelling. We have
(at first glance)
public < draft < secret
states of a commit (changeset), just like we have
solid < liquid < gaseous
phases of matter.
If we treat traits of 'immutability' and 'publishability' as true boolean,
such ordering of "phases" looks natural. Just like matter goes from gas
to liquid to solid with lowering temperature, changeset "phase" goes from
'secret' to 'draft' to 'public' following ancestry chain.
But if we go for expressivity and power like the rest of Git, rather than
for simplicity like Mercurial does, the differences between 'secret'-ness
and 'public'-ness make it unlikely that it would be a good idea to encompass
those in a single UI and a single concept. I guess that it can be another
issue where Mercurial oversimplifies its approach (c.f. branches which
started from clone to branch and anonymous branches, to "named branches"
which are really commit labels, to local-only bookmark branches, to
transferrable Git-like bookmark branches but with single namespace, and
only just considering equivalent of Git's refspec and refs mapping; c.f.
revision ranges based on local numbering of commits which made ranges
local too, to new Git-like topological `--ancestry-path`-like ranges).
Also with per-remote 'secret'-ness, we might have the scenario where
a commit is marked as 'secret' for public repo, we have pushed it to
group repo (and it's 'secret'-ness trait together with it), an now we
want to be warned if we try to rewrite said commit.
So, separate traits it is (of revision walk, or of a commit), rather
than "smush-together" antity / attribute of "phase" of revision.
That is BTW why I wanted us to have this discussion... :-D
[...]
>>> Basically, there are three different "levels" for this rewrite/publish
>>> protection to run at:
>>>
>>> 1. Do not meddle at all. This is the current behavior, and assumes
>>> that if the user rewrites and pushes something, the user knows what
>>> he/she is doing, and Git should not meddle (obviously unless the
>>> server refuses the push).
>>
>> I think that there should be some easy way to force such behavior,
>> i.e. to discard rewrite safeties.
>
> Indeed. We should probably have a simple config flag to enable the
> rewrite protection. In fact I would argue that the flag should default
> to false (disable protection) when unset, and then we should let
> init/clone set the config flag to true (enable protection) in newly
> created repos (unless explicitly disabled in the user/system configs).
> This way, behavior does not change for existing repos, but new repos
> are protected by default (with only a single command needed to disable
> the protection).
And commit option for discarding safeties for specific push, or for
specific rewrite (e.g. pushed at night, noticed an error in commit
message, force-amended, and force-pushed again, hoping that the window
of opportunity is sufficintly small so that nobody will be affected).
>>> 2. Warn/refuse rewriting commits in your upstream. This would only
>>> check branch X against its registered upstream. Only if there is a
>>> registered upstream, and you're about to rewrite commits that are
>>> reachable from the upstream remote-tracking branch, should Git
>>> intervene and warn/refuse the rewrite. This level would IMHO provide
>>> most of the benefit, and little or no trouble (i.e. false positives).
>>
>> Right. I wonder if we can get usage statistics from Mercurial users
>> about usage of their "phases" feature... though mapping terminology
>> for example 'upstream' from Git to Mercurial and vice versa can be
>> a pain, I guess.
>
> I'm unsure how useful it would be. IMHO Git and Mercurial are
> different enough (and promote sightly different workflows) that I
> don't trust the the average Mercurial user's preference for
> Mercurial's 'phase' behavior to be transferable to the average Git
> user's preference for a similar behavior in Git.
Well, bad statistics might be better than no statistics. What would
be especially interesting is _complaints_ about "phases" feature,
don't you think, i.e. where the "phases" metaphor fails.
[...]
--
Jakub Narebski
Poland
^ permalink raw reply
* Re: [RFD] Rewriting safety - warn before/when rewriting published history
From: Johan Herland @ 2012-02-07 15:09 UTC (permalink / raw)
To: Jakub Narebski; +Cc: Philip Oakley, git
In-Reply-To: <201202071531.08385.jnareb@gmail.com>
(we are pretty much in violent agreement, so I will only comment where
I find it necessary)
On Tue, Feb 7, 2012 at 15:31, Jakub Narebski <jnareb@gmail.com> wrote:
> Also, when thinking about different scenarios of why one would like to
> mark commit as 'secret', we might want to be able to mark commit as
> secret / unpublishable with respect to _subset_ of remotes, so e.g.
> I am prevented from accidentally publishing commits marked as 'secret'
> to public repository, or to CI/QA repository, but I can push (perhaps
> with warning) to group repository, together with 'secret'-ness state
> of said commit...
>
> ... though it wouldn't be as much 'secret' as 'confidential' ;-)
Another way to achieve this would be to have a config flag to control
whether Git checks for the 'secret' flag before pushing. This config
flag could be set at the system/user level (to enable/disable the
feature as a whole), at the repo level (to enable/disable it in a
given repo), at the remote level (to enable/disable it on a given
repo), and finally at the branch level (to enable-disable it for a
given branch (and its upstream)). Thus you could have a .git/config
that looked like this:
[core]
refusePushSecret = true
[remote "foo"]
refusePushSecret = false
url = ...
fetch = ...
[branch "baz"]
remote = foo
merge = refs/heads/baz
refusePushSecret = true
This config would:
- refuse to push 'secret' commits from branch 'baz'
(branch.baz.refusePushSecret == true)
- but allow to push other branches with 'secret' commits to remote
'foo' (remote.foo.refusePushSecret == false)
- but refuse to push 'secret' commits to other remotes
(core.refusePushSecret == true)
(The order of precedence would be: branch config > remote config >
repo config > user config > system config > default when unset)
I am unsure whether the 'secret'-ness of a commit should follow across
the push, but if you do (assuming we store the 'secret' flag using
git-notes) this is simply a matter of synchronizing the
refs/notes/secret to the same remote.
Have fun! :)
...Johan
--
Johan Herland, <johan@herland.net>
www.herland.net
^ permalink raw reply
* Re: cloning a tree which has detached branch checked out
From: Jeff King @ 2012-02-07 15:32 UTC (permalink / raw)
To: Nguyen Thai Ngoc Duy; +Cc: Michael S. Tsirkin, Jakub Narebski, git
In-Reply-To: <CACsJy8DtmQLX+Lfng-QRzVg9sfo8gQMXB-xTtPYpt+R2gModTg@mail.gmail.com>
On Tue, Feb 07, 2012 at 07:57:08PM +0700, Nguyen Thai Ngoc Duy wrote:
> On Tue, Feb 7, 2012 at 5:41 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
> >> > #git clone -n lab:/home/mst/scm/linux
> >> > ....
> >> > error: Trying to write ref HEAD with nonexistant object
> >> > cec64082f689f949a397cb9b39423dc41545fa0e
> >> > fatal: Cannot update the ref 'HEAD'.
> >> >
> >> > Looks like a bug, doesn't it?
> >>
> >> Which git version? IIRC there was some bugfix recently in that
> >> area...
> >
> > Sorry, forgot to mention that. It's pretty recent:
> > $ git --version
> > git version 1.7.9.111.gf3fb0
>
> The series that Jakub mentioned is probably nd/clone-detached 5ce2b97,
> which is already included in your tree. Does 1.7.9 work?
>
> I tried but it was ok for me. I think ref processing at local probably
> goes wrong and does not request commit from HEAD. If the repo is not
> confidential, you can zip it and send me.
This particular bug should have been fixed before that, even, with my
c1921c1 (clone: always fetch remote HEAD, 2011-06-07). And it is tested
explicitly in t5707, so your series does not regress at least a simple
case. So I think your guess about a subtle error in the local ref
processing is a reasonable one.
-Peff
^ permalink raw reply
* Re: Merging only a subdirectory from one branch to the other
From: Taylor Hedberg @ 2012-02-07 15:24 UTC (permalink / raw)
To: Howard Miller; +Cc: git
In-Reply-To: <CAHVO_90MQamw7oB8ry5YBEWSnRnxDZvQ4ApVuuv4AYR6VRuXSw@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 790 bytes --]
Howard Miller, Tue 2012-02-07 @ 09:38:22+0000:
> I have a branch with a particular subdirectory tree. The tree has a
> lot of history. However, all the history for that subdirectory is
> exclusive to it (no commits changed anything outside it). I now need
> to merge that subdirectory into a completely different branch without
> loosing any history. I think git-read-tree might have something to do
> with it but I don't understand the help file at all. Any help
> appreciated.
You might find git-subtree [1] useful for this. The `git subtree split`
sub-command in particular enables you to factor out the history of a
particular subdirectory in your repo into a separate branch, which you
could then merge into some other branch as desired.
[1] https://github.com/apenwarr/git-subtree
[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply
* Re: [PATCHv2] tag: add --points-at list option
From: Jeff King @ 2012-02-07 16:05 UTC (permalink / raw)
To: Tom Grennan; +Cc: git, gitster, jasampler
In-Reply-To: <1328598076-7773-2-git-send-email-tmgrennan@gmail.com>
On Mon, Feb 06, 2012 at 11:01:16PM -0800, Tom Grennan wrote:
> +struct points_at {
> + struct points_at *next;
> + unsigned char *sha1;
> +};
Would using sha1_array save us from having to create our own data
structure? As a bonus, it can do O(lg n) lookups, though I seriously
doubt anyone will provide a large number of "--points-at".
> +static void free_points_at (struct points_at *points_at)
> +{
> + while (points_at) {
> + struct points_at *next = points_at->next;
> + free(points_at->sha1);
> + free(points_at);
> + points_at = next;
> + }
> +}
Then this could go away in favor of sha1_array_clear.
> +int parse_opt_points_at(const struct option *opt, const char *arg, int unset)
> +{
> + struct points_at *new, **opt_value = (struct points_at **)opt->value;
> + unsigned char *sha1;
> +
> + if (!arg)
> + return error(_("missing <object>"));
> + new = xmalloc(sizeof(struct points_at));
> + sha1 = xmalloc(20);
> + if (get_sha1(arg, sha1)) {
> + free(new);
> + free(sha1);
> + return error(_("malformed object name '%s'"), arg);
> + }
> + new->sha1 = sha1;
> + new->next = *opt_value;
> + *opt_value = new;
> + return 0;
> +}
And this can drop all of the memory management bits, like:
unsigned char sha1[20];
if (!arg)
return error(_("missing <object>"));
if (get_sha1(arg, sha1))
return error(_("malformed object name '%s'"), arg);
sha1_array_append(opt->value, sha1);
return 0;
Also, should you check "unset"? When we have options that build a list,
usually doing "--no-foo" will clear the list. E.g., this:
git tag --points-at=foo --points-at=bar --no-points-at --points-at=baz
should look only for "baz".
> +static struct points_at *match_points_at(struct points_at *points_at,
> + const unsigned char *sha1)
> +{
> + char *buf;
> + struct tag *tag;
> + unsigned long size;
> + enum object_type type;
> +
> + buf = read_sha1_file(sha1, &type, &size);
> + if (!buf)
> + return NULL;
> + if (type != OBJ_TAG
> + || (tag = lookup_tag(sha1), !tag)
> + || parse_tag_buffer(tag, buf, size) < 0) {
> + free(buf);
> + return NULL;
> + }
> + while (points_at && hashcmp(points_at->sha1, tag->tagged->sha1))
> + points_at = points_at->next;
> + free(buf);
> + return points_at;
> +}
Sorry, I threw a lot of object lookup code at you last time, so I think
my point may have been lost in the noise. But I think this is slightly
nicer as:
static int tag_points_at(struct sha1_array *sa,
const unsigned char *sha1)
{
struct object *obj = parse_object(sha1);
if (!obj)
return 0; /* or probably we should even just die() */
if (obj->type != OBJ_TAG)
return 0;
if (sha1_array_lookup(sa, ((struct tag *)obj)->tagged->sha1) < 0)
return 0;
return 1;
}
I.e., using parse_object lets you avoid dealing with memory management
yourself. And as a bonus, it will reuse the cached information if you
happen to have already parsed that object (not likely in typical
repositories, but a huge win in certain pathological cases, like repos
storing shared objects and refs for a large number of forks).
> + {
> + OPTION_CALLBACK, 0, "points-at", &points_at, "object",
> + "print only annotated|signed tags of the object",
> + PARSE_OPT_LASTARG_DEFAULT,
> + parse_opt_points_at, (intptr_t)NULL,
> + },
I think you can drop the LASTARG_DEFAULT here, as it is no longer
optional, no?
-Peff
^ permalink raw reply
* Re: User authentication in GIT
From: supadhyay @ 2012-02-07 16:40 UTC (permalink / raw)
To: git
In-Reply-To: <CALKQrgdvOhfhTPg+g+LqCb6XOQczcz-nYC61B9x4W5dB4Up5oA@mail.gmail.com>
Thank you Johan,freak.
you have clear my doubts at some extent and I think let me work on it.
I will work on giloite and get back to you.
Thanks...
--
View this message in context: http://git.661346.n2.nabble.com/User-authentication-in-GIT-tp7261349p7262934.html
Sent from the git mailing list archive at Nabble.com.
^ permalink raw reply
* Re: [PATCH v2 0/4] Deprecate "not allow as-is commit with i-t-a entries"
From: Junio C Hamano @ 2012-02-07 16:41 UTC (permalink / raw)
To: Nguyễn Thái Ngọc Duy; +Cc: git, Jonathan Nieder
In-Reply-To: <1328618804-31796-1-git-send-email-pclouds@gmail.com>
Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:
> - git-add.txt changes are removed. In the end all kinds of commit
> behave the same way, not worth putting more explanation during the
> transition.
>
> - reword config text and warning text (or more precisely copy/paste
> from Junio/Jonathan's words)
>
> - Hard coded release numbers are removed. Now it's simply "in future".
>
> - Step 2 may be too annoying. Users are warned on every commit if
> commit.ignoreIntentToAdd is set. I think it's good because it keeps
> config file clean, but people may think otherwise.
Ahh, thanks.
But when I said "let's admit that this is just fixing an UI mistake, no
configuration, no options", I really meant it. Without the backward
compatiblity "For now please do not fix this bug for me and keep being
buggy until I get used to the non-buggy behaviour" fuss, which we never do
to any bugfix.
That is how we are planning to handle "git merge" update to spawn editor
in interactive session in the next release. There is no "Please keep the
buggy behaviour" option; only an environment variable to help when we
mistake a scripted use as interactive, whose support is not going away
because it is not about "until I get used to the new behaviour".
^ permalink raw reply
* Re: [PATCH] gitweb: Deal with HEAD pointing to unborn branch in "heads" view
From: Jakub Narebski @ 2012-02-07 16:53 UTC (permalink / raw)
To: rajesh boyapati; +Cc: git
In-Reply-To: <CA+EqV8w6k2VrEtMydhGKZHbQdXHxCE3WA_0rtS-AY4cmQvii=A@mail.gmail.com>
On Mon, 6 Feb 2012, rajesh boyapati wrote:
>
> Thanks for your work.
I'm sorry I was able to find a fix only for the part of issue.
>>>>>>>>> [2012-01-25 18:50:35,658] ERROR
>>>>>>>>> com.google.gerrit.httpd.gitweb.GitWebServlet : CGI: [Wed Jan 25
>>>>>>>>> 18:50:35 2012] gitweb.cgi: Use of uninitialized value $commit_id
>>>>>>>>> in open at /usr/lib/cgi-bin/gitweb.cgi line 2817.
>>>>>>>
>>>>>>> sub parse_commits {
>>>>>>> my ($commit_id, $maxcount, $skip, $filename, @args) = @_;
>>>>>>> my @cos;
>>>>>>>
>>>>>>> $maxcount ||= 1;
>>>>>>> $skip ||= 0;
>>>>>>>
>>>>>>> local $/ = "\0";
>>>>>>>
>>>>>>> open my $fd, "-|", git_cmd(), "rev-list",
>>>>>>> "--header",
>>>>>>> @args,
>>>>>>> ("--max-count=" . $maxcount),
>>>>>>> ("--skip=" . $skip),
>>>>>>> @extra_options,
>>>>>>> $commit_id,
>>>>>>> "--",
>>>>>>> ($filename ? ($filename) : ())
>>>>>>> or die_error(500, "Open git-rev-list failed");
>>
>> But I was not able to fix this, at least not currently. I wrote a failing
>> test case for "commit" and similar views on unborn HEAD... but they fail
>> _without_ error message like the one quoted.
>>
>> I'd have to go slower route of examining gitweb code in how it deals with
>> "invalid" HEAD (i.e. HEAD not pointing to commit, being on unborn branch).
[...]
>> And here is the patch:
>> -->8 ------------>8 ---
>> From: Jakub Narebski <jnareb@gmail.com>
>> Subject: [PATCH] gitweb: Deal with HEAD pointing to unborn branch in
>> "heads" view
[...]
>> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
>> index 9cf7e71..1f0ec12 100755
>> --- a/gitweb/gitweb.perl
>> +++ b/gitweb/gitweb.perl
>> @@ -5570,7 +5570,7 @@ sub git_tags_body {
>>
>> sub git_heads_body {
>> # uses global variable $project
>> - my ($headlist, $head, $from, $to, $extra) = @_;
>> + my ($headlist, $head_at, $from, $to, $extra) = @_;
>> $from = 0 unless defined $from;
>> $to = $#{$headlist} if (!defined $to || $#{$headlist} < $to);
>>
>
> I didn't see a file called "gitweb.perl" in /usr/share/gitweb/
The file "gitweb.perl", or rather "gitweb/gitweb.perl" is the name
of the script in git.git repository. From it "make gitweb" would
generate "gitweb.cgi" file...
> I applied this patch to file "index.cgi" in /usr/share/gitweb/index.cgi at
> line 4711.
[...]
>
> I applied this patch to file "index.cgi" in /usr/share/gitweb/index.cgi at
> line 4720.
...and I guess Gerrit build process generates "index.cgi" from that.
> Had I applied the patch to the correct file "index.cgi", which is a link to
> file "gitweb.cgi" in /usr/lib/cgi-bin/gitweb.cgi ?
Ah, right.
> Then, I restarted gerrit server to take changes.
> Now the error log of gerrit shows:
> [2012-02-06 11:21:46,726] ERROR
> com.google.gerrit.httpd.gitweb.GitWebServlet : CGI: fatal: bad revision
> 'HEAD'
> [2012-02-06 11:21:49,167] ERROR
> com.google.gerrit.httpd.gitweb.GitWebServlet : CGI: [Mon Feb 6 11:21:49
> 2012] gitweb.cgi: Use of uninitialized value $commit_id in open at
> /usr/lib/cgi-bin/gitweb.cgi line 2817.
> [2012-02-06 11:21:49,169] ERROR
> com.google.gerrit.httpd.gitweb.GitWebServlet : CGI: fatal: bad revision ''
[the same errors repeated few times]
> <<<<<<<<<<<<<<<<
> Previously, there is a error showing at line 4720. Now, with this patch,
> that error has gone.
As I said I was able to find a fix only for part of the issue.
Unfortunately I was not able to reproduce this error in this form.
Note that the error location doesn't help much, because it is more
interesting for find which callers of parse_commits() pass undefined
$commit_id.
I can try to harden parse_commits() against bogus parameters; maybe
this would help.
> I tried to upgrade gitweb with the command "sudo apt-get install gitweb",
> but, it didn't find any upgrade.
> Am I doing in a right way?
There is no new version of gitweb yet; it haven't even been accepted
by Junio Hamano, maintainer of git of which gitweb is part, into git
repository (I might have to resend this patch for better visibility).
> Is there any place like "Github" (where we can place git projects) for
> gitweb ?
Gitweb is for quite some time developed within git repository. From
it the 'gitweb' package is created.
Clones of canonical, official git repository can be found in a few places:
git://git.kernel.org/pub/scm/git/git.git
git://repo.or.cz/alt-git.git
https://code.google.com/p/git-core/
https://github.com/git/git
My own clone of git, with my work, can be found at:
git://repo.or.cz/git/jnareb-git.git
https://github.com/jnareb/git
>> diff --git a/t/t9500-gitweb-standalone-no-errors.sh
>> b/t/t9500-gitweb-standalone-no-errors.sh
>> index 0f771c6..81246a6 100755
>> --- a/t/t9500-gitweb-standalone-no-errors.sh
>> +++ b/t/t9500-gitweb-standalone-no-errors.sh
>> @@ -739,4 +739,13 @@ test_expect_success \
>> 'echo "\$projects_list_group_categories = 1;">>gitweb_config.perl
>> &&
>> gitweb_run'
>>
>> +# ----------------------------------------------------------------------
>> +# unborn branches
>> +
>> +test_expect_success \
>> + 'unborn HEAD: "summary" page (with "heads" subview)' \
>> + 'git checkout orphan_branch || git checkout --orphan orphan_branch
>> &&
>> + test_when_finished "git checkout master" &&
>> + gitweb_run "p=.git;a=summary"'
>> +
>> test_done
>>
>
> I didn't find a file where to apply this patch.
> Is this file to test your patch for you?
Yes, this is to test that my patch fixes the issue correctly, and to
ensure that further changes don't re-break it. It is not usually
installed with git or gitweb, so don't worry about it.
--
Jakub Narebski
Poland
^ permalink raw reply
* Re: cloning a tree which has detached branch checked out
From: Junio C Hamano @ 2012-02-07 17:11 UTC (permalink / raw)
To: Jeff King; +Cc: Nguyen Thai Ngoc Duy, Michael S. Tsirkin, Jakub Narebski, git
In-Reply-To: <20120207153225.GA14773@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> On Tue, Feb 07, 2012 at 07:57:08PM +0700, Nguyen Thai Ngoc Duy wrote:
>
>> On Tue, Feb 7, 2012 at 5:41 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
>> >> > #git clone -n lab:/home/mst/scm/linux
>> >> > ....
>> >> > error: Trying to write ref HEAD with nonexistant object
>> >> > cec64082f689f949a397cb9b39423dc41545fa0e
>> >> > fatal: Cannot update the ref 'HEAD'.
>> >> >
>> >> > Looks like a bug, doesn't it?
>> >> ...
>
> This particular bug should have been fixed before that, even, with my
> c1921c1 (clone: always fetch remote HEAD, 2011-06-07). And it is tested
> explicitly in t5707,...
That was my thought when I first saw it. The failure case was that HEAD
was pointing at something no ref reaches, and clone was only grabbing the
tips of branches, which failed to transfer the history leading to HEAD.
> ... So I think your guess about a subtle error in the local ref
> processing is a reasonable one.
What is funny is "error: Trying to write ref HEAD with nonexistant object".
"git grep -e nonexistant f3fb0" does not register any hit.
Could it be that mysterious "lab:" protocol handler that is misbehaving?
^ permalink raw reply
* Re: cloning a tree which has detached branch checked out
From: Jeff King @ 2012-02-07 17:16 UTC (permalink / raw)
To: Junio C Hamano
Cc: Nguyen Thai Ngoc Duy, Michael S. Tsirkin, Jakub Narebski, git
In-Reply-To: <7v4nv236xn.fsf@alter.siamese.dyndns.org>
On Tue, Feb 07, 2012 at 09:11:00AM -0800, Junio C Hamano wrote:
> > This particular bug should have been fixed before that, even, with my
> > c1921c1 (clone: always fetch remote HEAD, 2011-06-07). And it is tested
> > explicitly in t5707,...
> [...]
> What is funny is "error: Trying to write ref HEAD with nonexistant object".
> "git grep -e nonexistant f3fb0" does not register any hit.
That misspelling of "nonexistent" was fixed by 7be8b3b (Fix typo:
existant->existent, 2011-06-16), around the same time as my clone patch.
Which really makes me wonder if the OP is accidentally running an old
version of git during the tests.
> >> On Tue, Feb 7, 2012 at 5:41 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
> >> >> > #git clone -n lab:/home/mst/scm/linux
> [...]
> Could it be that mysterious "lab:" protocol handler that is misbehaving?
I think you are misreading. It would be "lab::" in that case, no? The
command above should do ssh transport to the machine "lab".
-Peff
^ permalink raw reply
* Re: [PATCH 3/6] Stop producing index version 2
From: Thomas Rast @ 2012-02-07 17:25 UTC (permalink / raw)
To: Shawn Pearce
Cc: Junio C Hamano, Nguyễn Thái Ngọc, git,
Joshua Redstone
In-Reply-To: <CAJo=hJvtRnmvALcn3vKpYTr3j6ada8iboPjWN3cQnwwKzRvrDA@mail.gmail.com>
Shawn Pearce <spearce@spearce.org> writes:
> I have long wanted to scrap the current index format. I unfortunately
> don't have the time to do it myself. But I suspect there may be a lot
> of gains by making the index format match the canonical tree format
> better by keeping the tree structure within a single file stream,
> nesting entries below their parent directory, and keeping tree SHA-1
> data along with the directory entry.
If I may add to this: the one thing that I would like to see fixed about
the index is that it's flat out impossible to change a single thing in
it without re"writing" it from scratch.
I'm saying "writing" because it is possible to change a few things
around, but recomputing the trailing SHA1 swamps that by a large margin
unless you are writing to a floppy disk, so it doesn't matter. I'm sure
using a CRC32 helps here, but if we're going to make an incompatible
change, why not go all the way?
A tree layout can fix that if it is properly arranged so that if you
'git add path/to/file', it only updates the SHA1s for path/to/file,
path/to and path. For this to work, the checks would have to correspond
to the trees, perhaps even directly use the actual tree SHA1. This
would at least be natural in some sense; getting to actual log(n)
complexity for hilariously large directories would require dynamically
splitting directories where appropriate.
Along the same lines the format should allow for changing the extension
data for a single extension while only rehashing the new data.
When I worked on cache-tree, I considered making a change to the latter
effect, but thought the impact too great for a little gain. Now from
this thread, I'm getting the impression that such a change would be ok,
even if users would have to scrap the index if they downgrade. Is that
right?
--
Thomas Rast
trast@{inf,student}.ethz.ch
^ permalink raw reply
* Re: [PATCH 0/2] config includes, take 2
From: Jeff King @ 2012-02-07 17:30 UTC (permalink / raw)
To: David Aguilar; +Cc: git
In-Reply-To: <CAJDDKr5yiKvNnpVV29jFK1Z1yuUnA-=dn0yMB8iW9y53vRGDHQ@mail.gmail.com>
On Tue, Feb 07, 2012 at 02:05:34AM -0800, David Aguilar wrote:
> I won't claim it's the best solution, but this has worked extremely
> well in practice:
>
> https://github.com/git-cola/git-cola/blob/master/cola/gitcfg.py
>
> Here's the problem. I need to know which config items are "user"
> config (~/.gitconfig), which are "repo" config (.git/config), and
> which are "system" config (/etc/gitconfig).
I'm not a git cola user. But from peeking at that code and a brief
"apt-get install git-cola && git cola", it looks like the point is to
show a dialog with per-repo and per-user settings, let the user tweak
them, and save them back to the appropriate spot.
But how does that interact with includes? Let's imagine I do this:
git config --global include.path .gitidentity
git config -f ~/.gitidentity user.name "Jeff King"
Without having git-cola expand includes, your call to "git config
--global --list" will not show the user.name variable, and it will
appear blank. Which is not great.
If git-cola does expand includes, then the name will appear. But what
will happen when I modify that field and try to save it? It will save it
using "git config --global", putting it into ~/.gitconfig. So now you'll
have two names in your config, one in .gitconfig and one in
.gitidentity.
And which one will take precedence? That depends on what was in your
config file before. If there was a [user] section before the include
already in your .gitconfig, then the newly written value will go in that
section _before_ the include, and the include will still take
precedence. Otherwise, a new section will be written _after_ the
include, and that will take precedence.
So...yuck. It's kind of a complex and messy situation. But the important
thing is that when you are looking at a bidirectional editing situation
like this, blindly expanding includes is not going to solve the problem.
It's just going to paper over the complexity. In the long term,
something like git-cola that is trying to present and edit config to the
user would need to learn about includes, track the sources of each
variable, and then edit appropriately.
You can do that with my current patch by manually following include.path
directives. However, that's a pain. Git-config could potentially help
with that (and even simplify the current code) by allowing something
like:
$ git config --list-with-sources
/home/peff/.gitconfig user.name=Jeff King
/home/peff/.gitconfig user.email=peff@peff.net
.git/config core.repositoryformatversion=0
.git/config core.bare=false
[etc]
(you would use the "-z" form, of course, and the filenames would be
NUL-separated, but I made up a human-readable output format above for
illustration purposes).
And then it would be natural to mention included files there, and you
could actually modify the file that contains the value in question. Of
course, you might not have _access_ to the file in question. You could
be pulling in options from a shared config file, and the right thing to
do is not to edit the shared file, but to override it. And that raises
the question of where, and how to tell git-config to make sure that the
option goes _after_ the include.
So fundamentally, includes make the idea of "overwrite this value" much
more complex. It's possible that git-config should learn all of this
complexity, and that git cola could rely on git-config being smart. But
I was rather hoping that most of the automated tools could remain simple
and stupid, and that people who wanted to do complex things with
includes would be left to their own devices (i.e., using $EDITOR, or
smartly using "git config -f" on their individual files).
> Figuring out the repo config is tricky. You can't just say "git
> config --list" because that includes the user config stuff in
> ~/.gitconfig.
>
> Figuring out the user config is easy because you can say "git config
> --global --list". This is inconsistent with the behavior for "git
> config --list" because it does not include the --system config, which
> one would expect given the overlay semantics.
I think you are mistaking a lack of source options for "look at the repo
config". But without providing a source, the request is "look everywhere
that git knows about". With "--global", it's about "look at this one
file" (just as "--system" is about "look at this other file"). So I
think what you are really lamenting is the lack of "git config --repo",
which would basically be:
git config --list --file="$(git rev-parse --git-dir)/config"
That might be something we could improve in the documentation.
> The generic interface for getting a concise listing of values from
> these sources is to use "git config -f" on ~/.gitconfig, .git/config,
> and $(prefix)/etc/gitconfig.
Right. And --global and --system are basically just synonyms for the
prior two.
> git config --global and git config --system are both consistent in
> that they return just the information relevant to them. Is --global
> just a shortcut for "-f ~/.gitconfig"? If yes, then does that mean
> "git config --global" will not honor includes? If it is not a
> shortcut, does that mean that "git config --global" and "git config -f
> ~/.gitconfig" are not really the same thing? The documentation does
> lead one to believe that they should be equivalent...
Yes, it is a shortcut, and it follows the same code path (i.e., we
expand "--global" into "-f ~/.gitconfig" early on).
> > Yes, that's a general problem of adding new command-line options to turn
> > features off or on. We could add an environment variable to control this
> > feature. But I'd really like to hear the compelling use case first
> > (i.e., both why it cannot simply be spelled "git config -z --list", and
> > why not following includes is not OK).
>
> Hopefully my explanation conveys why "git config -z --list" is insufficient.
Yes, I agree it's insufficient in the face of includes. Unfortunately, I
don't think simply expanding includes by default is sufficient, either.
Probably the simplest solution for this case would be something like:
1. Always expand includes on listing.
2. When editing, always override items in includes. So do _not_ reuse
sections, but always put the values at the end of the file (or at
least after any includes).
That's not as nice as doing it "right" (you'll end up with duplicated
values in included files and their parents, and sections may appear
multiple times). But it's simple and should have the desired semantics.
> From a naive user's POV --
> I am asking "git config" to evaluate this file. I don't care what's
> inside of it; it's a black box to me. All I know is that I get config
> values out the other side. Sometimes git respects the included files.
> Sometimes it does not (when using -f).
> ^^^^^^^^^
> I think this subtle difference in behavior is best avoided.
It is not "sometimes when I ask git config to evaluate this file". It is
"git never expands includes when you ask it to evaluate a file".
Includes are expanded only when you ask git "search all files you know
about" (i.e., when you do not provide a file argument).
There is also a more complex issue with "-f", which is that git-config
is used not just to parse git configuration, but also to parse other
config-like files that should not respect includes (e.g.,
".gitmodules"). So to remain backwards-compatible, we need the default
for "-f" to not respect includes, and that is not really negotiable.
We could convert "--global" and "--system" to respect includes by
default (since we at least know these are actual git config files). But
that is introducing a subtle difference, which you complained about
above.
But if scripted uses like "git cola" are the ones that need it, I'd
rather provide a backwards-compatible way of tweaking the behavior (if a
command line option is not sufficient, then I'd rather provide an
environment variable).
> Here's another. The naive user will do: `git config --list -f file`.
> They will then edit that file to add an include statement. They will
> then run `git config --list -f file` again and be confused as to why
> their included configuration is not honored.
As I mentioned above, that one is not really negotiable. But it seems
like it is a mismatch of expectations to behavior, and that is something
we can at least address with documentation.
> Right now these two are synonymous: "git config --system --list" and
> "git config -f $(prefix)/etc/gitconfig --list". Having one form honor
> includes and not the other is inconsistent.
With my patch, they are consistent, and neither should honor includes
(that being said, there is a bug in my patch which makes --list honor
includes in all cases; however, you are responding correctly to the
intended and advertised behavior of my patch, and that is what we should
be discussing).
> What if we frame this the other way around -- when would we not want
> it to follow includes? I can imagine someone writing a "git config"
> editing program, but I think that use case is rare (and they haven't
> spoken up, either ;-).
But isn't "git cola" a git-config editing program?
> Well.. okay. My use case is rare too! The dumb "cache the values
> behind a stat() call" thing has worked really well in practice so I've
> been happy with it so far. It's dumb, and it won't notice when
> included files change, but I really don't care because it solves the
> 99% case.
I don't get the caching. How often would you need to call "git config"?
Isn't is only when the edit-config dialog appears? Is it really that
expensive to call when opening a user-editable dialog?
> Rewriting it (for me) would mean using --global and --system and
> probably not caring so much about the distinction of repo vs. user
> configuration (or rather, deal with the fact that `git config --list`
> returns both the user config and the repo config). At that point I
> might as well just write (reuse) a proper .gitconfig parser, but
> what's the fun in that when "git config" is so nice and easy to parse?
I definitely feel your pain. I don't want you to have to rewrite yet
another .gitconfig parser. Let's see if we can figure out reasonable
editing semantics that can go into git-config.
-Peff
^ permalink raw reply
* Re: [RFD] Rewriting safety - warn before/when rewriting published history
From: Ronan Keryell @ 2012-02-07 17:27 UTC (permalink / raw)
To: Jakub Narebski; +Cc: git
In-Reply-To: <201202071531.08385.jnareb@gmail.com>
>>>>> On Tue, 7 Feb 2012 15:31:07 +0100, Jakub Narebski <jnareb@gmail.com> said:
>> Agreed, but AFAICS (and modulo the addition of pre-rewrite and
>> pre/post-push hooks mentioned earlier) all of the things
>> discussed so far in this thread can be implemented as hooks.
Jakub> That would be nice.
Jakub> And the new hooks (pre-rewrite, pre/post-push) would be
Jakub> useful anyway, I think.
Yes, to deal with file metadata in git for example. :-)
--
Ronan KERYELL |\/ Phone: +1 408 844 4720
Wild Systems / HPC Project, Inc. |/) Cell: +33 613 143 766
5201 Great America Parkway #3241 K Ronan.Keryell@hpc-project.com
Santa Clara, CA 95054 |\ skype:keryell
USA | \ http://hpc-project.com
^ permalink raw reply
* Re: STGIT: Deathpatch in linus tree
From: Junio C Hamano @ 2012-02-07 17:37 UTC (permalink / raw)
To: Andy Green (林安廸), Catalin Marinas; +Cc: git
In-Reply-To: <4F3120D4.1050604@warmcat.com>
"Andy Green (林安廸)" <andy@warmcat.com> writes:
> [agreen@build linux-2.6]$ git reset --hard 105e518
> HEAD is now at 105e518 Merge tag 'hwmon-fixes-for-3.3-rc3' of
> git://git.kernel.org/pub/scm/linux/kernel/git/groeck/linux-staging
> [agreen@build linux-2.6]$ stg push
> Error: Unhandled exception:
> Traceback (most recent call last):
> File "/usr/lib/python2.7/site-packages/stgit/main.py", line 152, in _main
> ret = command.func(parser, options, args)
> File "/usr/lib/python2.7/site-packages/stgit/commands/push.py", line
> 68, in func
> check_clean_iw = clean_iw)
> File "/usr/lib/python2.7/site-packages/stgit/lib/transaction.py", line
> 95, in __init__
> self.__current_tree = self.__stack.head.data.tree
> File "/usr/lib/python2.7/site-packages/stgit/lib/git.py", line 426, in
> data
> self.__repository.cat_object(self.sha1))
> File "/usr/lib/python2.7/site-packages/stgit/lib/git.py", line 408, in
> parse
> assert False
> AssertionError
Stgit at least at version 0.15 (which is you used, and which is what I
checked its source for) assumes it knows every possible kind of header
that can be recorded in the commit object and hits this assert when it
seems something it does not understand.
Version 0.16 seems to have removed this assert, like this hunk on the file
between 0.15 and 0.16:
@@ -404,8 +404,6 @@ class CommitData(Immutable, Repr):
cd = cd.set_author(Person.parse(value))
elif key == 'committer':
cd = cd.set_committer(Person.parse(value))
- else:
- assert False
assert False
The above code in StGit that tries to parse commit object header is
broken, and I am surprised that nobody caught it back in late 2006 when
the optional encoding header was added to the commit object, which the
above does not understand.
I am not sure if that removal of the extra assertion is enough, though.
It does not skip lines that it does not understand until the first blank
line, i.e. the end of the header, as it should. Instead it has this
before starting to inspect if a line in the header is what it knows about:
line = lines[i].strip()
if not line:
return cd.set_message(''.join(lines[i+1:]))
key, value = line.split(None, 1)
I do not think this will work on a continuation line inside a header that
has only one token, and it will also ignore the leading whitespace that
protects the continuation line.
I think the loop should at least changed line this, until StGit starts
caring about multi-line headers with continuation lines.
diff --git a/stgit/lib/git.py b/stgit/lib/git.py
index 56287f6..f2b284d 100644
--- a/stgit/lib/git.py
+++ b/stgit/lib/git.py
@@ -392,18 +392,20 @@ class CommitData(Immutable, Repr):
cd = cls(parents = [])
lines = list(s.splitlines(True))
for i in xrange(len(lines)):
- line = lines[i].strip()
+ line = lines[i]
if not line:
return cd.set_message(''.join(lines[i+1:]))
- key, value = line.split(None, 1)
- if key == 'tree':
- cd = cd.set_tree(repository.get_tree(value))
- elif key == 'parent':
- cd = cd.add_parent(repository.get_commit(value))
- elif key == 'author':
- cd = cd.set_author(Person.parse(value))
- elif key == 'committer':
- cd = cd.set_committer(Person.parse(value))
+ ix = line.find(' ')
+ if 0 < ix:
+ key, value = line[0:ix], line[ix+1:]
+ if key == 'tree':
+ cd = cd.set_tree(repository.get_tree(value))
+ elif key == 'parent':
+ cd = cd.add_parent(repository.get_commit(value))
+ elif key == 'author':
+ cd = cd.set_author(Person.parse(value))
+ elif key == 'committer':
+ cd = cd.set_committer(Person.parse(value))
assert False
class Commit(GitObject):
^ permalink raw reply related
* Re: [PATCH 0/2] config includes, take 2
From: Junio C Hamano @ 2012-02-07 18:03 UTC (permalink / raw)
To: Jeff King; +Cc: David Aguilar, git
In-Reply-To: <20120207173025.GA22225@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> So fundamentally, includes make the idea of "overwrite this value" much
> more complex.
I do not think it is anything new that include.path brings. To give the
user a complete view of "what customization applies when working in this
repository?", and to allow the user to edit variables in the right file
among usual three places, the tool already needs to be aware of possible
places and give users a way to tell where the edit needs to go anyway.
include.path only adds one more thing for the tool to be aware of.
With your example, the editor can show
git config -f .git/config --list
with "include.path=~/.gitident" listed as one of the key=value pairs
without showing key=value pairs included from that file. Or it can show
user.name in effect is this value from .git/config, and optionally also
show that there are other definitions of user.name in ~/.gitconfig (which
we use as if we have "include.path=~/.gitconfig" at the top of .git/config
file) or ~/.gitident specified with include.path.
The tool needs to make it easy to jump to ~/.gitident; it needs to know
what include.path means. The user can edit the value in ~/.gitident, or
after looking at ~/.gitident, choose to come back to .git/config and add
an overriding entry there.
> But isn't "git cola" a git-config editing program?
Yes, that is really the right "rhetorical" question to ask. It needs to
know what it is editing, so it at least needs to be _aware_ of the
inclusion, where each item comes from, and how to edit contents of _one_
particular file.
And that issue is not something new that was introduced by include.path.
^ permalink raw reply
* Re: [PATCHv2] tag: add --points-at list option
From: Tom Grennan @ 2012-02-07 18:05 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, peff, jasampler
In-Reply-To: <7vd39r2g8o.fsf@alter.siamese.dyndns.org>
On Tue, Feb 07, 2012 at 12:35:19AM -0800, Junio C Hamano wrote:
>Tom Grennan <tmgrennan@gmail.com> writes:
>
>> +struct points_at {
>> + struct points_at *next;
>> + unsigned char *sha1;
>> +};
>
>struct points_at {
> struct points_at *next;
> unsigned char sha1[20];
>};
>
>would save you from having to allocate and free always in pairs, no?
Yep
>> +static void free_points_at (struct points_at *points_at)
>
>Please lose the SP before (.
Oops
>> + if (type != OBJ_TAG
>> + || (tag = lookup_tag(sha1), !tag)
>> + || parse_tag_buffer(tag, buf, size) < 0) {
>
>Even though I personally prefer to cascade a long expression like this, so
>that you see a parse tree when you tilt your head 90-degrees to the left,
>I think the prevalent style in Git codebase is
>
> if (A-long-long-expression ||
> B-long-long-expression ||
> C-long-long-expression) {
>
>Also we try to avoid assignment in the conditional.
I like to compact multiple conditions to a common exit but also appreciate
the fear and loathing of comma's.
While rearranging this I finally understand how to include lightweight tags.
struct points_at *pa;
const unsigned char *tagged_sha1 = (const unsigned char *)"";
/* First look for lightweight tags - those with matching sha's
* but different names */
for (pa = points_at; pa; pa = pa->next)
if (!hashcmp(pa->sha1, sha1) && strcmp(pa->refname, refname))
return pa;
buf = read_sha1_file(sha1, &type, &size);
if (buf) {
if (type == OBJ_TAG) {
tag = lookup_tag(sha1);
if (parse_tag_buffer(tag, buf, size) >= 0)
tagged_sha1 = tag->tagged->sha1;
}
free(buf);
}
while (points_at && hashcmp(points_at->sha1, tagged_sha1))
points_at = points_at->next;
return points_at;
For example,
$ ./git-tag tomg-lw-v1.7.9 v1.7.9
$ ./git-tag -a tomg-lw-v1.7.9 v1.7.9
$ ./git-tag -s tomg-lw-v1.7.9 v1.7.9
$ ./git-tag -s tomg-README HEAD:README
$ ./git-tag -l --points-at v1.7.9 --points-at HEAD:README
tomg-README
tomg-annotate-v1.7.9
tomg-lw-v1.7.9
tomg-signed-v1.7.9
$ ./git-tag -l --points-at v1.7.9 --points-at HEAD:README \*v1.7.9
tomg-annotate-v1.7.9
tomg-lw-v1.7.9
tomg-signed-v1.7.9
>> @@ -432,6 +500,12 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
>> PARSE_OPT_LASTARG_DEFAULT,
>> parse_opt_with_commit, (intptr_t)"HEAD",
>> },
>> + {
>> + OPTION_CALLBACK, 0, "points-at", &points_at, "object",
>> + "print only annotated|signed tags of the object",
>> + PARSE_OPT_LASTARG_DEFAULT,
>> + parse_opt_points_at, (intptr_t)NULL,
>> + },
>
>If you are going to reject NULL anyway, do you still need to mark this as
>lastarg-default?
>
>Looking for example in parse-options.h, I found this:
>
> #define OPT_STRING_LIST(s, l, v, a, h) \
> { OPTION_CALLBACK, (s), (l), (v), (a), \
> (h), 0, &parse_opt_string_list }
>
>which is used by "git clone" to mark its -c option.
>
>Running "git clone -c" gives me
>
> error: switch 'c' requires a value
>
>without any extra code in the caller of parse_options().
Cool
>Other than that, looks cleanly done.
>
>Thanks. I'll take another look after I wake up in the morning.
Thanks, I'll send v3 later today.
--
TomG
^ permalink raw reply
* Re: [PATCH 1/2] docs: add a basic description of the config API
From: Jeff King @ 2012-02-07 18:06 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <7vbopb61cd.fsf@alter.siamese.dyndns.org>
On Mon, Feb 06, 2012 at 02:31:14PM -0800, Junio C Hamano wrote:
> > +Config files are parsed linearly, and each variable found is passed to a
> > +caller-provided callback function. The callback function is responsible
> > +for any actions to be taken on the config option, and is free to ignore
> > +some options (it is not uncommon for the configuration to be parsed
> > +several times during the run of a git program, with different callbacks
> > +picking out different variables useful to themselves).
>
> It woud be easeier to read if you stopped the sentence after "some
> options" and made the "It is not uncommon..." a first-class sentence
> outside the parentheses.
Thanks. Putting in too many parenthetical phrases is a bad habit of
mine. Usually I notice around the time I try nesting a parenthetical
phrase instead of existing parentheses, and go back and at least promote
the outer one to a real sentence. :)
Will fix for the re-roll.
> > +A config callback should return 0 for success, or -1 if the variable
> > +could not be parsed properly.
>
> This matches what I have always thought, but I think I recently saw a
> series that adds callbacks that return 1 to mean "I have understood this
> variable, so callers should not look at it any more". It felt wrong, but
> I did not find anything in the config.c API framework to prvent such a
> local calling convention.
The return value is propagated from git_parse_file. I assume the
original intent was that you could actually use it to return an integer
from your callback. In practice, though, callbacks either modify global
data (for older instances), or modify a pointer passed through the void
data pointer.
The "1 means I understood this" convention is used by userdiff_config. I
don't like that it is unlike every other config callback, but I think
it's necessary to deal with the ambiguity of "diff.color.*", which could
be either a userdiff entry or a diff color. E.g., if we see
"diff.color.binary", we know that it is the "binary" variable of the
"color" diff driver, not the color spec for the "binary" slot.
Looking at the code again, though, it seems that there is no overlap
between the userdiff slots and the color slots, and that the
color-parsing code will silently ignore any unknown slots. So it would
be safe to further investigate diff.color.binary as a color, as we would
silently ignore it.
Hmm. Yeah. The userdiff calling convention dates back to late 2008. At
that time, parse_diff_color_slot would die() if it did not understand
the slot, making the "I understood this" flag required. Then later, in
8b8e862 (ignore unknown color configuration, 2009-12-12), it was
relaxed.
So I think we could go back and simplify the userdiff_config code now.
> > +There is a special version of `git_config` called `git_config_early`
> > +that takes an additional parameter to specify the repository config.
> > +This should be used early in a git program when the repository location
> > +has not yet been determined (and calling the usual lazy-evaluation
> > +lookup rules would yield an incorrect location).
>
> Do you want to say somethink like "Ordinary programs should not have to
> worry about git_config_early()"? Differently put, if you are learning the
> config API by reading this document and cannot tell which one you should
> be calling, you are way too inexperienced to call git_config_early() and
> you would always want to call git_config()?
Yes, I think that would be sensible.
Though having looked a lot at the config code recently, I wonder if
git_config_early is really necessary. The only caller (besides
git_config) is check_repository_format_early, which just wants to see if
core.repositoryformatversion in a directory is sane.
But why in the world would it want to do the full config lookup?
Shouldn't it be checking git_config_from_file directly on the config
file in the proposed repo?
-Peff
^ permalink raw reply
* Re: [PATCH 1/2] docs: add a basic description of the config API
From: Jeff King @ 2012-02-07 18:23 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <20120207180625.GA27189@sigill.intra.peff.net>
On Tue, Feb 07, 2012 at 01:06:25PM -0500, Jeff King wrote:
> So I think we could go back and simplify the userdiff_config code now.
That patch would look like this.
-- >8 --
Subject: drop odd return value semantics from userdiff_config
When the userdiff_config function was introduced in be58e70
(diff: unify external diff and funcname parsing code,
2008-10-05), it used a return value convention unlike any
other config callback. Like other callbacks, it used "-1" to
signal error. But it returned "1" to indicate that it found
something, and "0" otherwise; other callbacks simply
returned "0" to indicate that no error occurred.
This distinction was necessary at the time, because the
userdiff namespace overlapped slightly with the color
configuration namespace. So "diff.color.foo" could mean "the
'foo' slot of diff coloring" or "the 'foo' component of the
"color" userdiff driver". Because the color-parsing code
would die on an unknown color slot, we needed the userdiff
code to indicate that it had matched the variable, letting
us bypass the color-parsing code entirely.
Later, in 8b8e862 (ignore unknown color configuration,
2009-12-12), the color-parsing code learned to silently
ignore unknown slots. This means we no longer need to
protect userdiff-matched variables from reaching the
color-parsing code.
We can therefore change the userdiff_config calling
convention to a more normal one. This drops some code from
each caller, which is nice. But more importantly, it reduces
the cognitive load for readers who may wonder why
userdiff_config is unlike every other config callback.
There's no need to add a new test confirming that this
works; t4020 already contains a test that sets
diff.color.external.
Signed-off-by: Jeff King <peff@peff.net>
---
builtin/blame.c | 8 +-------
builtin/cat-file.c | 8 +-------
builtin/grep.c | 7 ++-----
diff.c | 7 ++-----
userdiff.c | 19 ++++++-------------
5 files changed, 12 insertions(+), 37 deletions(-)
diff --git a/builtin/blame.c b/builtin/blame.c
index 5a67c20..01956c8 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -2050,14 +2050,8 @@ static int git_blame_config(const char *var, const char *value, void *cb)
return 0;
}
- switch (userdiff_config(var, value)) {
- case 0:
- break;
- case -1:
+ if (userdiff_config(var, value) < 0)
return -1;
- default:
- return 0;
- }
return git_default_config(var, value, cb);
}
diff --git a/builtin/cat-file.c b/builtin/cat-file.c
index 07bd984..8ed501f 100644
--- a/builtin/cat-file.c
+++ b/builtin/cat-file.c
@@ -226,14 +226,8 @@ static const char * const cat_file_usage[] = {
static int git_cat_file_config(const char *var, const char *value, void *cb)
{
- switch (userdiff_config(var, value)) {
- case 0:
- break;
- case -1:
+ if (userdiff_config(var, value) < 0)
return -1;
- default:
- return 0;
- }
return git_default_config(var, value, cb);
}
diff --git a/builtin/grep.c b/builtin/grep.c
index 5c2ae94..dc6de83 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -325,11 +325,8 @@ static int grep_config(const char *var, const char *value, void *cb)
struct grep_opt *opt = cb;
char *color = NULL;
- switch (userdiff_config(var, value)) {
- case 0: break;
- case -1: return -1;
- default: return 0;
- }
+ if (userdiff_config(var, value) < 0)
+ return -1;
if (!strcmp(var, "grep.extendedregexp")) {
if (git_config_bool(var, value))
diff --git a/diff.c b/diff.c
index 7e15426..a656f5e 100644
--- a/diff.c
+++ b/diff.c
@@ -177,11 +177,8 @@ int git_diff_basic_config(const char *var, const char *value, void *cb)
return 0;
}
- switch (userdiff_config(var, value)) {
- case 0: break;
- case -1: return -1;
- default: return 0;
- }
+ if (userdiff_config(var, value) < 0)
+ return -1;
if (!prefixcmp(var, "diff.color.") || !prefixcmp(var, "color.diff.")) {
int slot = parse_diff_color_slot(var, 11);
diff --git a/userdiff.c b/userdiff.c
index 76109da..1e7184f 100644
--- a/userdiff.c
+++ b/userdiff.c
@@ -210,14 +210,7 @@ static int parse_funcname(struct userdiff_funcname *f, const char *k,
if (git_config_string(&f->pattern, k, v) < 0)
return -1;
f->cflags = cflags;
- return 1;
-}
-
-static int parse_string(const char **d, const char *k, const char *v)
-{
- if (git_config_string(d, k, v) < 0)
- return -1;
- return 1;
+ return 0;
}
static int parse_tristate(int *b, const char *k, const char *v)
@@ -226,13 +219,13 @@ static int parse_tristate(int *b, const char *k, const char *v)
*b = -1;
else
*b = git_config_bool(k, v);
- return 1;
+ return 0;
}
static int parse_bool(int *b, const char *k, const char *v)
{
*b = git_config_bool(k, v);
- return 1;
+ return 0;
}
int userdiff_config(const char *k, const char *v)
@@ -246,13 +239,13 @@ int userdiff_config(const char *k, const char *v)
if ((drv = parse_driver(k, v, "binary")))
return parse_tristate(&drv->binary, k, v);
if ((drv = parse_driver(k, v, "command")))
- return parse_string(&drv->external, k, v);
+ return git_config_string(&drv->external, k, v);
if ((drv = parse_driver(k, v, "textconv")))
- return parse_string(&drv->textconv, k, v);
+ return git_config_string(&drv->textconv, k, v);
if ((drv = parse_driver(k, v, "cachetextconv")))
return parse_bool(&drv->textconv_want_cache, k, v);
if ((drv = parse_driver(k, v, "wordregex")))
- return parse_string(&drv->word_regex, k, v);
+ return git_config_string(&drv->word_regex, k, v);
return 0;
}
--
1.7.8.4.12.g3a22e3
^ permalink raw reply related
* Re: User authentication in GIT
From: Sitaram Chamarty @ 2012-02-07 18:26 UTC (permalink / raw)
To: supadhyay; +Cc: git
In-Reply-To: <1328632848471-7262934.post@n2.nabble.com>
On Tue, Feb 7, 2012 at 10:10 PM, supadhyay <supadhyay@imany.com> wrote:
> Thank you Johan,freak.
>
> you have clear my doubts at some extent and I think let me work on it.
>
> I will work on giloite and get back to you.
I've also been working on some nice pictures... maybe they will help,
who knows...
http://sitaramc.github.com/gitolite/pictures.html
^ permalink raw reply
* Re: [PATCH 0/2] config includes, take 2
From: Jeff King @ 2012-02-07 18:29 UTC (permalink / raw)
To: Junio C Hamano; +Cc: David Aguilar, git
In-Reply-To: <7vpqdq1pxt.fsf@alter.siamese.dyndns.org>
On Tue, Feb 07, 2012 at 10:03:26AM -0800, Junio C Hamano wrote:
> > But isn't "git cola" a git-config editing program?
>
> Yes, that is really the right "rhetorical" question to ask. It needs to
> know what it is editing, so it at least needs to be _aware_ of the
> inclusion, where each item comes from, and how to edit contents of _one_
> particular file.
>
> And that issue is not something new that was introduced by include.path.
True, but it was made more complex. David has already dealt with the 3
sources in his code. Adding includes expands this to N sources, and his
code needs to adapt. It would be nice if we could at least make the
adaptation backwards-compatible and as painless as possible.
But I just see the "right" answer in a complex case needing user input
that a simple call to "git config" can't provide. That is, pretending
that "git config --global" is still a single file for reading and
writing can get you something that _works_, but it will make a mess of
the user's config. The most elegant thing to me is to expand git-cola's
config editor into an N-pane editor instead of a 2-pane editor (not 3,
because it doesn't make sense to edit /etc/gitconfig with it, and nor
does the current version support it). People without includes wouldn't
notice the difference, and people with includes might appreciate the
power and flexibility.
-Peff
^ permalink raw reply
* Re: [PATCH 2/2] config: add include directive
From: Jeff King @ 2012-02-07 18:36 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <7v62fj60ya.fsf@alter.siamese.dyndns.org>
On Mon, Feb 06, 2012 at 02:39:41PM -0800, Junio C Hamano wrote:
> > +Includes
> > +~~~~~~~~
> > +
> > +You can include one config file from another by setting the special
> > +`include.path` variable to the name of the file to be included. The
> > +included file is expanded immediately, as if its contents had been
> > +found at the location of the include directive. If the value of the
> > +`include.path` variable is a relative path, the path is considered to be
> > +relative to the configuration file in which the include directive was
> > +found. See below for examples.
>
> If the file referenced by this directive does not exist, what should
> happen? Should it be signalled as an error? Should it stop the whole
> calling process with die()?
I silently ignore it. My thinking was that you might want to have
something like:
[include]
path = .gitconfig-local
in a stock .gitconfig that you ship to all of your machines[1]. Then
machines that need it can put things in .gitconfig-local, and those that
don't can just ignore it.
It is a tradeoff, of course, in that typos will be silently ignored. For
this use case, you could also just create an empty .gitconfig-local on
machines that don't have anything to put there.
[1] Actually, a similar use might be a ~/.gitconfig that is shared by a
mounted home directory (e.g., via NFS) NFS, and a ~/.gitconfig-$HOST
that is specific to each machine. The current code doesn't expand
environment variables (nor tildes), but perhaps it should.
> I think "die() when we are honoring the include, ignore when we are not"
> would be a good way to handle this, as it allows us to catch mistakes
> while allowing the user to fix broken configuration files using "git
> config --unset include.path", but I may be overlooking something.
The writing path does not use the include callback wrapper at all; so
include.path can be manipulated just as any other variable, and the
value is not treated specially.
-Peff
^ permalink raw reply
* Re: [PATCH 1/2] docs: add a basic description of the config API
From: Junio C Hamano @ 2012-02-07 18:44 UTC (permalink / raw)
To: Jeff King; +Cc: git, Nguyễn Thái Ngọc Duy
In-Reply-To: <20120207180625.GA27189@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
>> > +A config callback should return 0 for success, or -1 if the variable
>> > +could not be parsed properly.
>>
>> This matches what I have always thought, but I think I recently saw a
>> series that adds callbacks that return 1 to mean "I have understood this
>> variable, so callers should not look at it any more". It felt wrong, but
>> I did not find anything in the config.c API framework to prvent such a
>> local calling convention.
>
> ...
> The "1 means I understood this" convention is used by userdiff_config. I
> don't like that it is unlike every other config callback,...
> Looking at the code again, though, ...
> Hmm. Yeah. The userdiff calling convention dates back to late 2008....
> So I think we could go back and simplify the userdiff_config code now.
I remembered where I saw the new "offender"; it was nd/columns
topic (Cc'ing Nguyễn).
^ permalink raw reply
* Re: [PATCH 1/2] docs: add a basic description of the config API
From: Junio C Hamano @ 2012-02-07 18:45 UTC (permalink / raw)
To: Jeff King; +Cc: git
In-Reply-To: <20120207182302.GA31059@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> -- >8 --
> Subject: drop odd return value semantics from userdiff_config
> ...
> 5 files changed, 12 insertions(+), 37 deletions(-)
>
> diff --git a/builtin/blame.c b/builtin/blame.c
> index 5a67c20..01956c8 100644
> --- a/builtin/blame.c
> +++ b/builtin/blame.c
> @@ -2050,14 +2050,8 @@ static int git_blame_config(const char *var, const char *value, void *cb)
> return 0;
> }
>
> - switch (userdiff_config(var, value)) {
> - case 0:
> - break;
> - case -1:
> + if (userdiff_config(var, value) < 0)
> return -1;
> - default:
> - return 0;
> - }
>
> return git_default_config(var, value, cb);
> }
> ...
Looks very nice ;-)
^ 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