* Re: [PATCH] rerere: Separate libgit and builtin functions
From: Johannes Schindelin @ 2008-07-09 11:55 UTC (permalink / raw)
To: Stephan Beyer; +Cc: git, Junio C Hamano
In-Reply-To: <1215598683-9685-1-git-send-email-s-beyer@gmx.net>
Hi,
On Wed, 9 Jul 2008, Stephan Beyer wrote:
> This patch moves rerere()-related functions into a newly created
> rerere.c file. Also setup_rerere() is moved into rerere.c to decrease
> code duplications for rerere.c and builtin-rerere.c.
It is not moved to decrease code duplication, but because it is needed by
rerere().
Ciao,
Dscho
^ permalink raw reply
* Re: [PATCH] Fix problem with authentification on http repository.
From: Johannes Schindelin @ 2008-07-09 11:43 UTC (permalink / raw)
To: Mike Hommey; +Cc: Junio C Hamano, jean.guyader, git, Jean Guyader
In-Reply-To: <20080709085651.GA23453@glandium.org>
Hi,
On Wed, 9 Jul 2008, Mike Hommey wrote:
> Note that http://user:pass@server/path/ /should/ work (but that would
> need validation), though not a good idea on command line.
Well, now that the programs using URLs are all builtins, we can actually
do something about it. We can edit out the "user[:pass]@" part out of
argv, which usually means that "ps" will not see it anymore.
Ciao,
Dscho
^ permalink raw reply
* Re: [GSoC] What is status of Git's Google Summer of Code 2008 projects?
From: Jakub Narebski @ 2008-07-09 11:36 UTC (permalink / raw)
To: Sverre Rabbelier; +Cc: git, David Symonds, Shawn O. Pearce
In-Reply-To: <bd6139dc0807090318r4cbcac93s907871c6b45c03ac@mail.gmail.com>
On Wed, 9 July 2008, Sverre Rabbelier wrote:
> On Tue, Jul 8, 2008 at 2:27 AM, Jakub Narebski <jnareb@gmail.com> wrote:
> >
> > I'd very much like to have (or perhaps to wrote) some sort of interim
> > progress report for Google Summer of Code 2008 projects on
> > http://git.or.cz/gitwiki/SoC2008Projects. Therefore I'd like you to
> > expand and/or correct those mini-summaries below.
>
> Would you like us to go to the wiki page and edit them ourselves, yes?
Yes, thanks in advance.
> If so, I'll see about kicking up something interesting to put up there
> soon.
I'd rather students or mentors did write short summary on mentioned
wiki page, but I'll try to come up with some short summary for each
GSoC2008 project (if it won't be there already) after July 14.
[thanks for summary]
--
Jakub Narebski
Poland
^ permalink raw reply
* Re: [PATCH] better git-submodule status output
From: Sylvain Joyeux @ 2008-07-09 11:01 UTC (permalink / raw)
To: Andreas Ericsson; +Cc: git
In-Reply-To: <48749215.6070307@op5.se>
On Wed, Jul 09, 2008 at 12:25:25PM +0200, Andreas Ericsson wrote:
> Sylvain Joyeux wrote:
>> Here is a (tentative) summary of the whole discussion:
>>
>> * doing fetch in status is EVIL. Therefore, status should only report
>> whatever information is available.
>> * nonetheless, having a "peek" mode seem to be accepted as a useful
>> feature not only by me.
>> * changing the output format of git-submodules is not right either,
>> because it would break existing tools which parses it at the moment.
>>
>> Proposal
>> - remove fetch from status, and make the new output enabled when
>> --verbose is set (can also be set in the config file I guess).
>>
>> On the symbols side, I propose:
>> > submodule commit is a direct descendant of the commit in the
>> superproject
>> < submodule commit is a direct ancestor of the commit in the
>> superproject
>> + no direct relation between submodule commit and commit in the
>> superproject
>> ? the commit in the superproject cannot be found in the submodule
>> (replaces the initial '!' in my patch)
>>
>> A 'M' is appended if the submodule has uncommitted changes
>>
>> - define a git-submodule 'fetch' subcommand which call fetch in each
>> submodule and runs the verbose mode of git-status (can be disabled by
>> a --quiet option).
>>
>> Comments ? (I'm sure there are some ...)
>>
>
> Re-use the existing symbolism from fetch status output. Using '+' to
> denote "no direct relation" in a tool that shows patches half the
> time is just plain horrible.
Could you be more specific ? AFAIK, 'fetch' symbolism is based on files,
the one I need is comparing commits.
And I don't like the '+' either. I chose it because it is the current
symbol to show that there is a difference between
superproject/submodule.
--
=======================================================================
Dr. Ing. Sylvain Joyeux sylvain.joyeux@dfki.de
Researcher
DFKI Robotik Lab -- Bremen http://www.dfki.de/robotik
Tel: 0049 421 218 64136
=======================================================================
^ permalink raw reply
* Re: [GSoC] What is status of Git's Google Summer of Code 2008 projects?
From: Miklos Vajna @ 2008-07-09 10:56 UTC (permalink / raw)
To: sverre
Cc: Jakub Narebski, git, Sam Vilain, Joshua Roys, David Symonds,
Lea Wiemann, John Hawley, Marek Zawirski, Shawn O. Pearce,
Johannes Schindelin, Stephan Beyer, Christian Couder,
Daniel Barkalow
In-Reply-To: <bd6139dc0807090318r4cbcac93s907871c6b45c03ac@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 715 bytes --]
On Wed, Jul 09, 2008 at 12:18:41PM +0200, Sverre Rabbelier <alturin@gmail.com> wrote:
> I am not yet sure how I should send in my code for peer review.
> $ git rev-list master | wc -l
> 151
What I did squashing fixes regularly, but I keep a 'history' branch
where I always reference to the old HEAD, so others can still have a
look at the individual commits.
It's like:
$ cat rebase-builtin-merge.sh
#!/bin/sh -e
[ -n "$*" ]
old_head=$(git rev-parse HEAD)
git rebase $*
git update-ref refs/heads/rebase-history \
$(echo "Rebased with 'git rebase $*'" | \
git commit-tree HEAD^{tree} -p rebase-history -p $old_head -p HEAD)
It will not work properly if you get conflicts, but you got the idea.
[-- Attachment #2: Type: application/pgp-signature, Size: 197 bytes --]
^ permalink raw reply
* Re: [PATCH] better git-submodule status output
From: Andreas Ericsson @ 2008-07-09 10:25 UTC (permalink / raw)
To: Sylvain Joyeux; +Cc: git
In-Reply-To: <20080709101330.GA3525@joyeux>
Sylvain Joyeux wrote:
> Here is a (tentative) summary of the whole discussion:
>
> * doing fetch in status is EVIL. Therefore, status should only report
> whatever information is available.
> * nonetheless, having a "peek" mode seem to be accepted as a useful
> feature not only by me.
> * changing the output format of git-submodules is not right either,
> because it would break existing tools which parses it at the moment.
>
> Proposal
> - remove fetch from status, and make the new output enabled when
> --verbose is set (can also be set in the config file I guess).
>
> On the symbols side, I propose:
> > submodule commit is a direct descendant of the commit in the
> superproject
> < submodule commit is a direct ancestor of the commit in the
> superproject
> + no direct relation between submodule commit and commit in the
> superproject
> ? the commit in the superproject cannot be found in the submodule
> (replaces the initial '!' in my patch)
>
> A 'M' is appended if the submodule has uncommitted changes
>
> - define a git-submodule 'fetch' subcommand which call fetch in each
> submodule and runs the verbose mode of git-status (can be disabled by
> a --quiet option).
>
> Comments ? (I'm sure there are some ...)
>
Re-use the existing symbolism from fetch status output. Using '+' to
denote "no direct relation" in a tool that shows patches half the
time is just plain horrible.
--
Andreas Ericsson andreas.ericsson@op5.se
OP5 AB www.op5.se
Tel: +46 8-230225 Fax: +46 8-230231
^ permalink raw reply
* Re: [GSoC] What is status of Git's Google Summer of Code 2008 projects?
From: Sverre Rabbelier @ 2008-07-09 10:18 UTC (permalink / raw)
To: Jakub Narebski
Cc: git, Sam Vilain, Joshua Roys, David Symonds, Lea Wiemann,
John Hawley, Marek Zawirski, Shawn O. Pearce, Miklos Vajna,
Johannes Schindelin, Stephan Beyer, Christian Couder,
Daniel Barkalow
In-Reply-To: <200807080227.43515.jnareb@gmail.com>
[Once again I forgot to "reply to all", sorry Jakub ;)]
Heya,
On Tue, Jul 8, 2008 at 2:27 AM, Jakub Narebski <jnareb@gmail.com> wrote:
> I'd very much like to have (or perhaps to wrote) some sort of interim
> progress report for Google Summer of Code 2008 projects on
> http://git.or.cz/gitwiki/SoC2008Projects. Therefore I'd like you to
> expand and/or correct those mini-summaries below.
Would you like us to go to the wiki page and edit them ourselves, yes?
If so, I'll see about kicking up something interesting to put up there
soon.
> (It would be, I guess, good preparation for GSoC 2008 mid-term
> evaluations, which according to GSoC 2008 timeline
> http://code.google.com/opensource/gsoc/2008/faqs.html#0.1_timeline
> are to take place July 7 -- July 14.)
Agreed.
> 2. git-statistics (?)
>
> Student: Sverre Rabbelier
> Mentor: David Symonds
>
> There were some posts about how git-statistics can be used:
> http://thread.gmane.org/gmane.comp.version-control.git/81534
> http://thread.gmane.org/gmane.comp.version-control.git/82027
> but it was mainly theoretical thread
>
> The git-stats repository at http://repo.or.cz/w/git-stats.git (gitweb)
> has some commits, but I don't remember any of them on git mailing list.
> Not ready?
I am not yet sure how I should send in my code for peer review.
$ git rev-list master | wc -l
151
A bunch of these patches introduce significant changes, a few are
fixes, and some are only changes to the changelog [0] which is
actually a pretty good way to see what I've been up to. Credit here
goes to David for suggesting I keep one. I am contemplating sending in
one patch per module since I've structured my code in such a way that
each module is mostly a stand-alone file that can be reviewed in
itself. I am not sure if there is any way to get such code reviewed,
most other projects had several distinct steps (e.g., modifying parts
of existing code first) they had to go through before they could write
new code. My project has been "new code" from the get go, so any
advice as to how to send this in for peer review is very welcome.
I have been working on GitStats, an attempt to gather metrics on a git
repository. For those who would like to know more about my goals,
please have a look at [1] or the more general [2]. Currently I have
quite a few metrics done, most of them are aimed at detecting whether
a commit is a bugfix. A short listing of metrics done:
* stats.py author -d: Shows file activity of a specific developer
measured in how often they made a modification to a file and total
lines added/removed (much like a diffstat, but now for a specific
developer instead of one commit).
* stats.py author -f: Shows file activity of a specific file meaured
in how often they made a modification to a file, could be extended to
also count changes like "author -d"
* stats.py branch -b: Shows which branch a specific file belongs to,
for more information on this metric see below
* stats.py commit -v: Shows all commits that are reverted by the
specified commit, will be extended to allow detection of partial
reverts
* stats.py diff -e: Shows whether two specific commits introduce the
same changes
* stats.py diff -e -n: ditto, but ignores what changes were made, only
looks at the changed lines
* stats.py diff -e -i: ditto, but inverts the comparison, instead of
comparing addition with addition and deletions with deletions, the
additions of the first diff are compared with the deletions of the
second diff, and vise versa. This way a revert is easily detected.
* stats.py index -t: Shows which commits touched the same paths as the
staged changes
I am working now on getting the 'is a bugfix' metric going, it's
sub-metrics are mostly done ('branch contains' , 'is revert', 'commit
msg matches', 'commit diff matches') with the exception of a few
simple ones ('is partial revert', 'modifies same lines') that are
already almost done. As a sneak peek into what I've been up to I'll
zoom in a bit on the development of the 'belongs to' metric.
One of the hardest things to tackle was the 'belongs to'
metric. It's goal is to determine how much a certain commit
'belongs to' a specific branch. When aggregating this
metric one can pick the best fit. This is useful when
trying to figure out if a commit was made on a maintenance
branch, and as such whether it should be marked as a 'fix'.
The definition of "belonging to" a branch was made as follows: "Each
branch that contains the target commit begins with a 'dilution' of 0,
for each merge on the way between it and the commit, increase the
dilution by one if it was not the primary parent of the merge. This
means that if a commit was "merged into" a branch instead of having
been made on that branch, it will have a higher dilution than if the
commit was made on that branch. This way, the lower the dilution, the
more a commit belongs on a branch.
The problem with this metric was not in writing it, but in
getting it to not be terribly slow on large repositories.
A few of the major problems include:
* Do not call git-rev-list on each commit, but instead
gather all 'parentage' information beforehand. This
avoids a lot of calls to the git binary, which is
expensive. This simple optimization resulted in a huge
speed increase, even on the small tests.
With this in place it runs in under a second for the test
repository, but the git.git repo still takes ages.
* Do not revisit commits that have already been checked
_and had a lower or equal dilution_ in the previous
check.
Especially in the git.git repository this
optimization made the algorithm go from 'unusable' to
useful.
With this in place it runs in under a second for recent
commits. It takes under 20 seconds for the first commit
in the repository.
* Keep a global memory too, that is, when multiple branches
contain the commit, remember that we have already visited
a commit in a global memory as well. When visiting
a commit we check if we already visited it before but
with a lower dilution, if so, this means that another
branch is always going to be better than the current
branch at this point, as such, we stop walking there.
With this in place it still runs in under a second for
recent commits. It takes about 7 seconds for the first
commit in the repository.
* Do a rev-list on all branches we are interested in once,
instead of once for each branch. In order to cut down the
output by not listing 'subsets', the rev-lists of each
individual branch was needed. By not doing this filtering
a lot of time is saved.
With this in place it still runs in under a second for
recent commits. It takes a little over 5 seconds for the
first commit in the repository.
The above timings were made on a rather outdated version of git.git,
after updating to today's latest the timings are as follows:
$ time stats.py branch -b e83c516331
Matching branches:
pu
next
master
maint
offcuts
real 0m6.360s
user 0m6.228s
sys 0m0.244s
It is plausible that there is no way to do this any faster
with the current approach. Walking all commits, examining
them, and calculating the dilution, all the way to the root
commit (on the git.git repo) just takes that long. The
algorithm itself is almost instant, most of the time is
spent waiting for git rev-list to return.
For a recent commit on the maint branch we can find the following
information (the -d flag was included to include 'debug' information,
so that we can see what other information was found, but left out in
the regular report):
sverre@Laptop-Sverre:~/code/git$ time stats.py branch -b 2b2828b -d
Retreiving branches that contain the commit...
Gathering information on the history...
Done.
Checking branches now:
origin/next
master
origin/maint
origin/pu
Done.
Listing found metrics:
Branch next, dilution: 2.
Branch master, dilution: 1.
Branch maint, dilution: 0.
Done.
Matching branches:
The minimal dilution is: 0
maint
real 0m6.431s
user 0m6.164s
sys 0m0.260s
>From the above one can see that master has recently merged in that
commit, but that next did not include it until after it merged in
master. In this way a commit can cascade through multiple merge, earch
merge increasing it's dilution by one.
I am very interested to hear comments on my progress so far, but also
on what is thought to be "important to work on next". It would be
awesome if a few people could give it a test drive. I recommend using
the setupRepo.py script in 'src/scripts' which will create a
metricsrepo in /tmp/ that is very well suited to experiment with the
'belongs to' metric. The testrepo also created in /tmp is better
suited to test some of the other metrics. Both repositories are used
by the testcases under 'src/t', which should all pass :). My
repository can be found at [3].
Thank you for reading, and I'm looking forward to review/comments.
[0] http://repo.or.cz/w/git-stats.git?a=blob;f=doc/changelog.txt
[1] http://alturin.googlepages.com/Use_cases.html
[2] http://alturin.googlepages.com/gsoc2008
[3] http://repo.or.cz/w/git-stats.git
--
Cheers,
Sverre Rabbelier
^ permalink raw reply
* [PATCH] rerere: Separate libgit and builtin functions
From: Stephan Beyer @ 2008-07-09 10:18 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git, Junio C Hamano, Stephan Beyer
In-Reply-To: <alpine.DEB.1.00.0807090230560.5277@eeepc-johanness>
This patch moves rerere()-related functions into a newly created
rerere.c file. Also setup_rerere() is moved into rerere.c to decrease
code duplications for rerere.c and builtin-rerere.c.
Signed-off-by: Stephan Beyer <s-beyer@gmx.net>
---
> BTW I think it is a horrible thing that rerere() is declared in commit.h
> (a libgit header), but implemented in builtin-rerere.c, which is not part
> of libgit.a.
Ok, I give it a try.
Makefile | 2 +
builtin-rerere.c | 324 +++---------------------------------------
builtin-rerere.c => rerere.c | 133 +-----------------
rerere.h | 9 ++
4 files changed, 32 insertions(+), 436 deletions(-)
copy builtin-rerere.c => rerere.c (66%)
create mode 100644 rerere.h
diff --git a/Makefile b/Makefile
index bddd1a7..4e23c29 100644
--- a/Makefile
+++ b/Makefile
@@ -363,6 +363,7 @@ LIB_H += quote.h
LIB_H += reflog-walk.h
LIB_H += refs.h
LIB_H += remote.h
+LIB_H += rerere.h
LIB_H += revision.h
LIB_H += run-command.h
LIB_H += sha1-lookup.h
@@ -447,6 +448,7 @@ LIB_OBJS += read-cache.o
LIB_OBJS += reflog-walk.o
LIB_OBJS += refs.o
LIB_OBJS += remote.o
+LIB_OBJS += rerere.o
LIB_OBJS += revision.o
LIB_OBJS += run-command.o
LIB_OBJS += server-info.o
diff --git a/builtin-rerere.c b/builtin-rerere.c
index 85222d9..669a963 100644
--- a/builtin-rerere.c
+++ b/builtin-rerere.c
@@ -1,6 +1,7 @@
#include "builtin.h"
#include "cache.h"
#include "path-list.h"
+#include "rerere.h"
#include "xdiff/xdiff.h"
#include "xdiff-interface.h"
@@ -13,183 +14,11 @@ static const char git_rerere_usage[] =
static int cutoff_noresolve = 15;
static int cutoff_resolve = 60;
-/* if rerere_enabled == -1, fall back to detection of .git/rr-cache */
-static int rerere_enabled = -1;
-
-static char *merge_rr_path;
-
static const char *rr_path(const char *name, const char *file)
{
return git_path("rr-cache/%s/%s", name, file);
}
-static void read_rr(struct path_list *rr)
-{
- unsigned char sha1[20];
- char buf[PATH_MAX];
- FILE *in = fopen(merge_rr_path, "r");
- if (!in)
- return;
- while (fread(buf, 40, 1, in) == 1) {
- int i;
- char *name;
- if (get_sha1_hex(buf, sha1))
- die("corrupt MERGE_RR");
- buf[40] = '\0';
- name = xstrdup(buf);
- if (fgetc(in) != '\t')
- die("corrupt MERGE_RR");
- for (i = 0; i < sizeof(buf) && (buf[i] = fgetc(in)); i++)
- ; /* do nothing */
- if (i == sizeof(buf))
- die("filename too long");
- path_list_insert(buf, rr)->util = name;
- }
- fclose(in);
-}
-
-static struct lock_file write_lock;
-
-static int write_rr(struct path_list *rr, int out_fd)
-{
- int i;
- for (i = 0; i < rr->nr; i++) {
- const char *path = rr->items[i].path;
- int length = strlen(path) + 1;
- if (write_in_full(out_fd, rr->items[i].util, 40) != 40 ||
- write_in_full(out_fd, "\t", 1) != 1 ||
- write_in_full(out_fd, path, length) != length)
- die("unable to write rerere record");
- }
- if (commit_lock_file(&write_lock) != 0)
- die("unable to write rerere record");
- return 0;
-}
-
-static int handle_file(const char *path,
- unsigned char *sha1, const char *output)
-{
- SHA_CTX ctx;
- char buf[1024];
- int hunk = 0, hunk_no = 0;
- struct strbuf one, two;
- FILE *f = fopen(path, "r");
- FILE *out = NULL;
-
- if (!f)
- return error("Could not open %s", path);
-
- if (output) {
- out = fopen(output, "w");
- if (!out) {
- fclose(f);
- return error("Could not write %s", output);
- }
- }
-
- if (sha1)
- SHA1_Init(&ctx);
-
- strbuf_init(&one, 0);
- strbuf_init(&two, 0);
- while (fgets(buf, sizeof(buf), f)) {
- if (!prefixcmp(buf, "<<<<<<< "))
- hunk = 1;
- else if (!prefixcmp(buf, "======="))
- hunk = 2;
- else if (!prefixcmp(buf, ">>>>>>> ")) {
- int cmp = strbuf_cmp(&one, &two);
-
- hunk_no++;
- hunk = 0;
- if (cmp > 0) {
- strbuf_swap(&one, &two);
- }
- if (out) {
- fputs("<<<<<<<\n", out);
- fwrite(one.buf, one.len, 1, out);
- fputs("=======\n", out);
- fwrite(two.buf, two.len, 1, out);
- fputs(">>>>>>>\n", out);
- }
- if (sha1) {
- SHA1_Update(&ctx, one.buf ? one.buf : "",
- one.len + 1);
- SHA1_Update(&ctx, two.buf ? two.buf : "",
- two.len + 1);
- }
- strbuf_reset(&one);
- strbuf_reset(&two);
- } else if (hunk == 1)
- strbuf_addstr(&one, buf);
- else if (hunk == 2)
- strbuf_addstr(&two, buf);
- else if (out)
- fputs(buf, out);
- }
- strbuf_release(&one);
- strbuf_release(&two);
-
- fclose(f);
- if (out)
- fclose(out);
- if (sha1)
- SHA1_Final(sha1, &ctx);
- return hunk_no;
-}
-
-static int find_conflict(struct path_list *conflict)
-{
- int i;
- if (read_cache() < 0)
- return error("Could not read index");
- for (i = 0; i+1 < active_nr; i++) {
- struct cache_entry *e2 = active_cache[i];
- struct cache_entry *e3 = active_cache[i+1];
- if (ce_stage(e2) == 2 &&
- ce_stage(e3) == 3 &&
- ce_same_name(e2, e3) &&
- S_ISREG(e2->ce_mode) &&
- S_ISREG(e3->ce_mode)) {
- path_list_insert((const char *)e2->name, conflict);
- i++; /* skip over both #2 and #3 */
- }
- }
- return 0;
-}
-
-static int merge(const char *name, const char *path)
-{
- int ret;
- mmfile_t cur, base, other;
- mmbuffer_t result = {NULL, 0};
- xpparam_t xpp = {XDF_NEED_MINIMAL};
-
- if (handle_file(path, NULL, rr_path(name, "thisimage")) < 0)
- return 1;
-
- if (read_mmfile(&cur, rr_path(name, "thisimage")) ||
- read_mmfile(&base, rr_path(name, "preimage")) ||
- read_mmfile(&other, rr_path(name, "postimage")))
- return 1;
- ret = xdl_merge(&base, &cur, "", &other, "",
- &xpp, XDL_MERGE_ZEALOUS, &result);
- if (!ret) {
- FILE *f = fopen(path, "w");
- if (!f)
- return error("Could not write to %s", path);
- fwrite(result.ptr, result.size, 1, f);
- fclose(f);
- }
-
- free(cur.ptr);
- free(base.ptr);
- free(other.ptr);
- free(result.ptr);
-
- return ret;
-}
-
static void unlink_rr_item(const char *name)
{
unlink(rr_path(name, "thisimage"));
@@ -198,6 +27,17 @@ static void unlink_rr_item(const char *name)
rmdir(git_path("rr-cache/%s", name));
}
+static int git_rerere_gc_config(const char *var, const char *value, void *cb)
+{
+ if (!strcmp(var, "gc.rerereresolved"))
+ cutoff_resolve = git_config_int(var, value);
+ else if (!strcmp(var, "gc.rerereunresolved"))
+ cutoff_noresolve = git_config_int(var, value);
+ else
+ return git_default_config(var, value, cb);
+ return 0;
+}
+
static void garbage_collect(struct path_list *rr)
{
struct path_list to_remove = { NULL, 0, 0, 1 };
@@ -207,6 +47,7 @@ static void garbage_collect(struct path_list *rr)
int len, i, cutoff;
time_t now = time(NULL), then;
+ git_config(git_rerere_gc_config, NULL);
strlcpy(buf, git_path("rr-cache"), sizeof(buf));
len = strlen(buf);
dir = opendir(buf);
@@ -267,148 +108,19 @@ static int diff_two(const char *file1, const char *label1,
return 0;
}
-static int do_plain_rerere(struct path_list *rr, int fd)
-{
- struct path_list conflict = { NULL, 0, 0, 1 };
- int i;
-
- find_conflict(&conflict);
-
- /*
- * MERGE_RR records paths with conflicts immediately after merge
- * failed. Some of the conflicted paths might have been hand resolved
- * in the working tree since then, but the initial run would catch all
- * and register their preimages.
- */
-
- for (i = 0; i < conflict.nr; i++) {
- const char *path = conflict.items[i].path;
- if (!path_list_has_path(rr, path)) {
- unsigned char sha1[20];
- char *hex;
- int ret;
- ret = handle_file(path, sha1, NULL);
- if (ret < 1)
- continue;
- hex = xstrdup(sha1_to_hex(sha1));
- path_list_insert(path, rr)->util = hex;
- if (mkdir(git_path("rr-cache/%s", hex), 0755))
- continue;;
- handle_file(path, NULL, rr_path(hex, "preimage"));
- fprintf(stderr, "Recorded preimage for '%s'\n", path);
- }
- }
-
- /*
- * Now some of the paths that had conflicts earlier might have been
- * hand resolved. Others may be similar to a conflict already that
- * was resolved before.
- */
-
- for (i = 0; i < rr->nr; i++) {
- struct stat st;
- int ret;
- const char *path = rr->items[i].path;
- const char *name = (const char *)rr->items[i].util;
-
- if (!stat(rr_path(name, "preimage"), &st) &&
- !stat(rr_path(name, "postimage"), &st)) {
- if (!merge(name, path)) {
- fprintf(stderr, "Resolved '%s' using "
- "previous resolution.\n", path);
- goto tail_optimization;
- }
- }
-
- /* Let's see if we have resolved it. */
- ret = handle_file(path, NULL, NULL);
- if (ret)
- continue;
-
- fprintf(stderr, "Recorded resolution for '%s'.\n", path);
- copy_file(rr_path(name, "postimage"), path, 0666);
-tail_optimization:
- if (i < rr->nr - 1)
- memmove(rr->items + i,
- rr->items + i + 1,
- sizeof(rr->items[0]) * (rr->nr - i - 1));
- rr->nr--;
- i--;
- }
-
- return write_rr(rr, fd);
-}
-
-static int git_rerere_config(const char *var, const char *value, void *cb)
-{
- if (!strcmp(var, "gc.rerereresolved"))
- cutoff_resolve = git_config_int(var, value);
- else if (!strcmp(var, "gc.rerereunresolved"))
- cutoff_noresolve = git_config_int(var, value);
- else if (!strcmp(var, "rerere.enabled"))
- rerere_enabled = git_config_bool(var, value);
- else
- return git_default_config(var, value, cb);
- return 0;
-}
-
-static int is_rerere_enabled(void)
-{
- struct stat st;
- const char *rr_cache;
- int rr_cache_exists;
-
- if (!rerere_enabled)
- return 0;
-
- rr_cache = git_path("rr-cache");
- rr_cache_exists = !stat(rr_cache, &st) && S_ISDIR(st.st_mode);
- if (rerere_enabled < 0)
- return rr_cache_exists;
-
- if (!rr_cache_exists &&
- (mkdir(rr_cache, 0777) || adjust_shared_perm(rr_cache)))
- die("Could not create directory %s", rr_cache);
- return 1;
-}
-
-static int setup_rerere(struct path_list *merge_rr)
-{
- int fd;
-
- git_config(git_rerere_config, NULL);
- if (!is_rerere_enabled())
- return -1;
-
- merge_rr_path = xstrdup(git_path("rr-cache/MERGE_RR"));
- fd = hold_lock_file_for_update(&write_lock, merge_rr_path, 1);
- read_rr(merge_rr);
- return fd;
-}
-
-int rerere(void)
-{
- struct path_list merge_rr = { NULL, 0, 0, 1 };
- int fd;
-
- fd = setup_rerere(&merge_rr);
- if (fd < 0)
- return 0;
- return do_plain_rerere(&merge_rr, fd);
-}
-
int cmd_rerere(int argc, const char **argv, const char *prefix)
{
struct path_list merge_rr = { NULL, 0, 0, 1 };
int i, fd;
+ if (argc < 2)
+ return rerere();
+
fd = setup_rerere(&merge_rr);
if (fd < 0)
return 0;
- if (argc < 2)
- return do_plain_rerere(&merge_rr, fd);
- else if (!strcmp(argv[1], "clear")) {
+ if (!strcmp(argv[1], "clear")) {
for (i = 0; i < merge_rr.nr; i++) {
struct stat st;
const char *name = (const char *)merge_rr.items[i].util;
@@ -417,7 +129,7 @@ int cmd_rerere(int argc, const char **argv, const char *prefix)
stat(rr_path(name, "postimage"), &st))
unlink_rr_item(name);
}
- unlink(merge_rr_path);
+ unlink(git_path("rr-cache/MERGE_RR"));
} else if (!strcmp(argv[1], "gc"))
garbage_collect(&merge_rr);
else if (!strcmp(argv[1], "status"))
diff --git a/builtin-rerere.c b/rerere.c
similarity index 66%
copy from builtin-rerere.c
copy to rerere.c
index 85222d9..5c22bed 100644
--- a/builtin-rerere.c
+++ b/rerere.c
@@ -1,18 +1,11 @@
-#include "builtin.h"
#include "cache.h"
#include "path-list.h"
+#include "rerere.h"
#include "xdiff/xdiff.h"
#include "xdiff-interface.h"
#include <time.h>
-static const char git_rerere_usage[] =
-"git-rerere [clear | status | diff | gc]";
-
-/* these values are days */
-static int cutoff_noresolve = 15;
-static int cutoff_resolve = 60;
-
/* if rerere_enabled == -1, fall back to detection of .git/rr-cache */
static int rerere_enabled = -1;
@@ -190,83 +183,6 @@ static int merge(const char *name, const char *path)
return ret;
}
-static void unlink_rr_item(const char *name)
-{
- unlink(rr_path(name, "thisimage"));
- unlink(rr_path(name, "preimage"));
- unlink(rr_path(name, "postimage"));
- rmdir(git_path("rr-cache/%s", name));
-}
-
-static void garbage_collect(struct path_list *rr)
-{
- struct path_list to_remove = { NULL, 0, 0, 1 };
- char buf[1024];
- DIR *dir;
- struct dirent *e;
- int len, i, cutoff;
- time_t now = time(NULL), then;
-
- strlcpy(buf, git_path("rr-cache"), sizeof(buf));
- len = strlen(buf);
- dir = opendir(buf);
- strcpy(buf + len++, "/");
- while ((e = readdir(dir))) {
- const char *name = e->d_name;
- struct stat st;
- if (name[0] == '.' && (name[1] == '\0' ||
- (name[1] == '.' && name[2] == '\0')))
- continue;
- i = snprintf(buf + len, sizeof(buf) - len, "%s", name);
- strlcpy(buf + len + i, "/preimage", sizeof(buf) - len - i);
- if (stat(buf, &st))
- continue;
- then = st.st_mtime;
- strlcpy(buf + len + i, "/postimage", sizeof(buf) - len - i);
- cutoff = stat(buf, &st) ? cutoff_noresolve : cutoff_resolve;
- if (then < now - cutoff * 86400) {
- buf[len + i] = '\0';
- path_list_insert(xstrdup(name), &to_remove);
- }
- }
- for (i = 0; i < to_remove.nr; i++)
- unlink_rr_item(to_remove.items[i].path);
- path_list_clear(&to_remove, 0);
-}
-
-static int outf(void *dummy, mmbuffer_t *ptr, int nbuf)
-{
- int i;
- for (i = 0; i < nbuf; i++)
- if (write_in_full(1, ptr[i].ptr, ptr[i].size) != ptr[i].size)
- return -1;
- return 0;
-}
-
-static int diff_two(const char *file1, const char *label1,
- const char *file2, const char *label2)
-{
- xpparam_t xpp;
- xdemitconf_t xecfg;
- xdemitcb_t ecb;
- mmfile_t minus, plus;
-
- if (read_mmfile(&minus, file1) || read_mmfile(&plus, file2))
- return 1;
-
- printf("--- a/%s\n+++ b/%s\n", label1, label2);
- fflush(stdout);
- xpp.flags = XDF_NEED_MINIMAL;
- memset(&xecfg, 0, sizeof(xecfg));
- xecfg.ctxlen = 3;
- ecb.outf = outf;
- xdi_diff(&minus, &plus, &xpp, &xecfg, &ecb);
-
- free(minus.ptr);
- free(plus.ptr);
- return 0;
-}
-
static int do_plain_rerere(struct path_list *rr, int fd)
{
struct path_list conflict = { NULL, 0, 0, 1 };
@@ -341,11 +257,7 @@ tail_optimization:
static int git_rerere_config(const char *var, const char *value, void *cb)
{
- if (!strcmp(var, "gc.rerereresolved"))
- cutoff_resolve = git_config_int(var, value);
- else if (!strcmp(var, "gc.rerereunresolved"))
- cutoff_noresolve = git_config_int(var, value);
- else if (!strcmp(var, "rerere.enabled"))
+ if (!strcmp(var, "rerere.enabled"))
rerere_enabled = git_config_bool(var, value);
else
return git_default_config(var, value, cb);
@@ -372,7 +284,7 @@ static int is_rerere_enabled(void)
return 1;
}
-static int setup_rerere(struct path_list *merge_rr)
+int setup_rerere(struct path_list *merge_rr)
{
int fd;
@@ -396,42 +308,3 @@ int rerere(void)
return 0;
return do_plain_rerere(&merge_rr, fd);
}
-
-int cmd_rerere(int argc, const char **argv, const char *prefix)
-{
- struct path_list merge_rr = { NULL, 0, 0, 1 };
- int i, fd;
-
- fd = setup_rerere(&merge_rr);
- if (fd < 0)
- return 0;
-
- if (argc < 2)
- return do_plain_rerere(&merge_rr, fd);
- else if (!strcmp(argv[1], "clear")) {
- for (i = 0; i < merge_rr.nr; i++) {
- struct stat st;
- const char *name = (const char *)merge_rr.items[i].util;
- if (!stat(git_path("rr-cache/%s", name), &st) &&
- S_ISDIR(st.st_mode) &&
- stat(rr_path(name, "postimage"), &st))
- unlink_rr_item(name);
- }
- unlink(merge_rr_path);
- } else if (!strcmp(argv[1], "gc"))
- garbage_collect(&merge_rr);
- else if (!strcmp(argv[1], "status"))
- for (i = 0; i < merge_rr.nr; i++)
- printf("%s\n", merge_rr.items[i].path);
- else if (!strcmp(argv[1], "diff"))
- for (i = 0; i < merge_rr.nr; i++) {
- const char *path = merge_rr.items[i].path;
- const char *name = (const char *)merge_rr.items[i].util;
- diff_two(rr_path(name, "preimage"), path, path, path);
- }
- else
- usage(git_rerere_usage);
-
- path_list_clear(&merge_rr, 1);
- return 0;
-}
diff --git a/rerere.h b/rerere.h
new file mode 100644
index 0000000..35b0fa8
--- /dev/null
+++ b/rerere.h
@@ -0,0 +1,9 @@
+#ifndef RERERE_H
+#define RERERE_H
+
+#include "path-list.h"
+
+extern int setup_rerere(struct path_list *);
+extern int rerere(void);
+
+#endif
--
1.5.6.2.574.gd4568
^ permalink raw reply related
* Re: [PATCH] better git-submodule status output
From: Sylvain Joyeux @ 2008-07-09 10:13 UTC (permalink / raw)
To: git
In-Reply-To: <20080701150119.GE5852@joyeux>
Here is a (tentative) summary of the whole discussion:
* doing fetch in status is EVIL. Therefore, status should only report
whatever information is available.
* nonetheless, having a "peek" mode seem to be accepted as a useful
feature not only by me.
* changing the output format of git-submodules is not right either,
because it would break existing tools which parses it at the moment.
Proposal
- remove fetch from status, and make the new output enabled when
--verbose is set (can also be set in the config file I guess).
On the symbols side, I propose:
> submodule commit is a direct descendant of the commit in the
superproject
< submodule commit is a direct ancestor of the commit in the
superproject
+ no direct relation between submodule commit and commit in the
superproject
? the commit in the superproject cannot be found in the submodule
(replaces the initial '!' in my patch)
A 'M' is appended if the submodule has uncommitted changes
- define a git-submodule 'fetch' subcommand which call fetch in each
submodule and runs the verbose mode of git-status (can be disabled by
a --quiet option).
Comments ? (I'm sure there are some ...)
Sylvain
^ permalink raw reply
* Re: [PATCH] git-rerere.txt: Mention rr-cache directory
From: Stephan Beyer @ 2008-07-09 9:21 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: Junio C Hamano, git
In-Reply-To: <alpine.DEB.1.00.0807090230560.5277@eeepc-johanness>
Hi,
On Wed, Jul 09, Johannes Schindelin wrote:
> On Wed, 9 Jul 2008, Johannes Schindelin wrote:
> > On Wed, 9 Jul 2008, Stephan Beyer wrote:
> >
> > > If a user reads the rerere documentation, he or she is not told to
> > > create the $GIT_DIR/rr-cache directory to be able to use git-rerere.
> >
> > Is it? The config setting is not enough? Then that is a bug, and should
> > not be blessed by a bug in the documentation.
I don't know if this is exactly a bug, at least config.txt says:
rerere.enabled::
Activate recording of resolved conflicts, so that identical
conflict hunks can be resolved automatically, should they
be encountered again. linkgit:git-rerere[1] command is by
default enabled if you create `rr-cache` directory under
`$GIT_DIR`, but can be disabled by setting this option to false.
So according to *that* documentation it is right, according to the
git-rerere.txt documentation it was wrong, and according to the
source it is...
> -- snip --
> static int is_rerere_enabled(void)
> {
> struct stat st;
> const char *rr_cache;
> int rr_cache_exists;
>
> if (!rerere_enabled)
> return 0;
>
> rr_cache = git_path("rr-cache");
> rr_cache_exists = !stat(rr_cache, &st) && S_ISDIR(st.st_mode);
> if (rerere_enabled < 0)
> return rr_cache_exists;
>
> if (!rr_cache_exists &&
> (mkdir(rr_cache, 0777) || adjust_shared_perm(rr_cache)))
> die("Could not create directory %s", rr_cache);
> return 1;
> }
> -- snap --
>
> As you can see, in the case rerere_enabled < 0 (i.e. the config did not
> say anything about rerere), it is assumed enabled _exactly_ when
> .git/rr_cache/ exists.
>
> But if it is > 0, the directory is created.
Yes.
There is also an important part in the file:
-- snip --
/* if rerere_enabled == -1, fall back to detection of .git/rr-cache */
static int rerere_enabled = -1;
-- snap --
So this means it's the following behavior:
- rerere_enabled < 0: enable, iff rr-cache exists
- rerere_enabled ==0: disable
- rerere_enabled > 0: enable and create rr-cache if it does not exist
So this is my fault, sorry :) Both documentations (config.txt and
git-rerere.txt) are right.
It was not explicitly enabled on this one machine which lead me to the
wrong assumption. I should sync my ~/.gitconfig more often ;)
Sorry for that,
Stephan
--
Stephan Beyer <s-beyer@gmx.net>, PGP 0x6EDDD207FCC5040F
^ permalink raw reply
* Re: [PATCH 2/2] branch --merged/--not-merged: allow specifying arbitrary commit
From: Junio C Hamano @ 2008-07-09 9:13 UTC (permalink / raw)
To: Pierre Habouzit; +Cc: git, Lars Hjemli
In-Reply-To: <20080709074516.GA23482@artemis.madism.org>
Pierre Habouzit <madcoder@debian.org> writes:
> There's a way: declare --merge as (PARSE_OPT_)NONEG to tell parse-opt
> not to generate --no-merge by itself, and declare a --no-merge option as
> well. I think it works, and if not, we should make parse-opt work with
> that. This is a quite disgusting hack, and no *new* options should be
> written this way, but we have to be flexible enough for backward
> compatibility's sake.
Nice, thanks.
With the obvious s/_FAKELASTARG/_LASTARG_DEFAULT/g to the first one, this
replaces the second one (with documentation updates for a change ;-), and
it seems to work Ok.
-- >8 --
Subject: branch --merged/--no-merged: allow specifying arbitrary commit
"git-branch --merged" is a handy way to list all the branches that have
already been merged to the current branch, but it did not allow checking
against anything but the current branch. Having to switch branches only
to list the branches that are merged with another branch made the feature
practically useless.
This updates the option parser so that "git branch --merged next" is
accepted when you are on 'master' branch.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
Documentation/git-branch.txt | 27 ++++++++++++----------
builtin-branch.c | 50 +++++++++++++++++++++++++++++++++++------
2 files changed, 57 insertions(+), 20 deletions(-)
diff --git a/Documentation/git-branch.txt b/Documentation/git-branch.txt
index 0fd5808..450f903 100644
--- a/Documentation/git-branch.txt
+++ b/Documentation/git-branch.txt
@@ -8,24 +8,27 @@ git-branch - List, create, or delete branches
SYNOPSIS
--------
[verse]
-'git-branch' [--color | --no-color] [-r | -a] [--merged | --no-merged]
- [-v [--abbrev=<length> | --no-abbrev]]
- [--contains <commit>]
+'git-branch' [--color | --no-color] [-r | -a]
+ [-v [--abbrev=<length> | --no-abbrev]]
+ [(--merged | --no-merged | --contains) [<commit>]]
'git-branch' [--track | --no-track] [-l] [-f] <branchname> [<start-point>]
'git-branch' (-m | -M) [<oldbranch>] <newbranch>
'git-branch' (-d | -D) [-r] <branchname>...
DESCRIPTION
-----------
-With no arguments given a list of existing branches
-will be shown, the current branch will be highlighted with an asterisk.
-Option `-r` causes the remote-tracking branches to be listed,
-and option `-a` shows both.
-With `--contains <commit>`, shows only the branches that
-contains the named commit (in other words, the branches whose
-tip commits are descendant of the named commit).
-With `--merged`, only branches merged into HEAD will be listed, and
-with `--no-merged` only branches not merged into HEAD will be listed.
+
+With no arguments, existing branches are listed, the current branch will
+be highlighted with an asterisk. Option `-r` causes the remote-tracking
+branches to be listed, and option `-a` shows both.
+
+With `--contains`, shows only the branches that contains the named commit
+(in other words, the branches whose tip commits are descendant of the
+named commit). With `--merged`, only branches merged into the named
+commit (i.e. the branches whose tip commits are reachable from the named
+commit) will be listed. With `--no-merged` only branches not merged into
+the named commit will be listed. Missing <commit> argument defaults to
+'HEAD' (i.e. the tip of the current branch).
In its second form, a new branch named <branchname> will be created.
It will start out with a head equal to the one given as <start-point>.
diff --git a/builtin-branch.c b/builtin-branch.c
index 50cbc9c..1926c47 100644
--- a/builtin-branch.c
+++ b/builtin-branch.c
@@ -46,7 +46,12 @@ enum color_branch {
COLOR_BRANCH_CURRENT = 4,
};
-static int mergefilter = -1;
+static enum merge_filter {
+ NO_FILTER = 0,
+ SHOW_NOT_MERGED,
+ SHOW_MERGED,
+} merge_filter;
+static unsigned char merge_filter_ref[20];
static int parse_branch_color_slot(const char *var, int ofs)
{
@@ -234,13 +239,15 @@ static int append_ref(const char *refname, const unsigned char *sha1, int flags,
if ((kind & ref_list->kinds) == 0)
return 0;
- if (mergefilter > -1) {
+ if (merge_filter != NO_FILTER) {
branch.item = lookup_commit_reference_gently(sha1, 1);
if (!branch.item)
die("Unable to lookup tip of branch %s", refname);
- if (mergefilter == 0 && has_commit(head_sha1, &branch))
+ if (merge_filter == SHOW_NOT_MERGED &&
+ has_commit(merge_filter_ref, &branch))
return 0;
- if (mergefilter == 1 && !has_commit(head_sha1, &branch))
+ if (merge_filter == SHOW_MERGED &&
+ !has_commit(merge_filter_ref, &branch))
return 0;
}
@@ -421,6 +428,20 @@ static int opt_parse_with_commit(const struct option *opt, const char *arg, int
return 0;
}
+static int opt_parse_merge_filter(const struct option *opt, const char *arg, int unset)
+{
+ merge_filter = ((opt->long_name[0] == 'n')
+ ? SHOW_NOT_MERGED
+ : SHOW_MERGED);
+ if (unset)
+ merge_filter = SHOW_NOT_MERGED; /* b/c for --no-merged */
+ if (!arg)
+ arg = "HEAD";
+ if (get_sha1(arg, merge_filter_ref))
+ die("malformed object name %s", arg);
+ return 0;
+}
+
int cmd_branch(int argc, const char **argv, const char *prefix)
{
int delete = 0, rename = 0, force_create = 0;
@@ -461,7 +482,18 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
OPT_BIT('M', NULL, &rename, "move/rename a branch, even if target exists", 2),
OPT_BOOLEAN('l', NULL, &reflog, "create the branch's reflog"),
OPT_BOOLEAN('f', NULL, &force_create, "force creation (when already exists)"),
- OPT_SET_INT(0, "merged", &mergefilter, "list only merged branches", 1),
+ {
+ OPTION_CALLBACK, 0, "no-merged", &merge_filter_ref,
+ "commit", "print only not merged branches",
+ PARSE_OPT_LASTARG_DEFAULT | PARSE_OPT_NONEG,
+ opt_parse_merge_filter, (intptr_t) "HEAD",
+ },
+ {
+ OPTION_CALLBACK, 0, "merged", &merge_filter_ref,
+ "commit", "print only merged branches",
+ PARSE_OPT_LASTARG_DEFAULT | PARSE_OPT_NONEG,
+ opt_parse_merge_filter, (intptr_t) "HEAD",
+ },
OPT_END(),
};
@@ -471,9 +503,6 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
branch_use_color = git_use_color_default;
track = git_branch_track;
- argc = parse_options(argc, argv, options, builtin_branch_usage, 0);
- if (!!delete + !!rename + !!force_create > 1)
- usage_with_options(builtin_branch_usage, options);
head = resolve_ref("HEAD", head_sha1, 0, NULL);
if (!head)
@@ -486,6 +515,11 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
die("HEAD not found below refs/heads!");
head += 11;
}
+ hashcpy(merge_filter_ref, head_sha1);
+
+ argc = parse_options(argc, argv, options, builtin_branch_usage, 0);
+ if (!!delete + !!rename + !!force_create > 1)
+ usage_with_options(builtin_branch_usage, options);
if (delete)
return delete_branches(argc, argv, delete > 1, kinds);
--
1.5.6.2.276.gbb293
^ permalink raw reply related
* Re: [PATCH] Fix problem with authentification on http repository.
From: Mike Hommey @ 2008-07-09 8:56 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Johannes Schindelin, jean.guyader, git, Jean Guyader
In-Reply-To: <7vhcazlbch.fsf@gitster.siamese.dyndns.org>
On Wed, Jul 09, 2008 at 01:18:22AM -0700, Junio C Hamano <gitster@pobox.com> wrote:
> Mike Hommey <mh@glandium.org> writes:
>
> > On Tue, Jul 08, 2008 at 11:28:15PM -0700, Junio C Hamano <gitster@pobox.com> wrote:
> > ...
> >> >> Actually Documentation/howto/setup-git-server-over-http.txt
> >> >> talks about http://user@server/path/ format. How well does this
> >> >> work in practice? If it does, we should update Documentation/urls.txt
> >> >> to allow optional user@ there like...
> >> >
> >> > AFAIK, there is nothing to deal with asking a password to the user in git
> >> > in this case, so that doesn't work.
> >>
> >> Then perhaps the howto needs updating?
> >
> > Actually, the howto tells to setup a ~/.netrc file, which should make
> > the http://user@server/path/ format work. I don't know if dropping user@
> > would still work, that depends how libcurl works with it.
>
> Ok, so the documentation update I suggested earlier may actually be
> valid, right?
The problem is that these urls won't work out of the box if the user
doesn't setup a .netrc file. I'd say it would be better not to add these
forms in Documentation/urls.txt until git prompts for the password
when it is missing.
Note that http://user:pass@server/path/ /should/ work (but that would
need validation), though not a good idea on command line.
Mike
^ permalink raw reply
* Re: [PATCH] Fix problem with authentification on http repository.
From: Junio C Hamano @ 2008-07-09 8:18 UTC (permalink / raw)
To: Mike Hommey; +Cc: Johannes Schindelin, jean.guyader, git, Jean Guyader
In-Reply-To: <20080709075345.GA22022@glandium.org>
Mike Hommey <mh@glandium.org> writes:
> On Tue, Jul 08, 2008 at 11:28:15PM -0700, Junio C Hamano <gitster@pobox.com> wrote:
> ...
>> >> Actually Documentation/howto/setup-git-server-over-http.txt
>> >> talks about http://user@server/path/ format. How well does this
>> >> work in practice? If it does, we should update Documentation/urls.txt
>> >> to allow optional user@ there like...
>> >
>> > AFAIK, there is nothing to deal with asking a password to the user in git
>> > in this case, so that doesn't work.
>>
>> Then perhaps the howto needs updating?
>
> Actually, the howto tells to setup a ~/.netrc file, which should make
> the http://user@server/path/ format work. I don't know if dropping user@
> would still work, that depends how libcurl works with it.
Ok, so the documentation update I suggested earlier may actually be
valid, right?
^ permalink raw reply
* Re: [PATCH] Fix problem with authentification on http repository.
From: Daniel Stenberg @ 2008-07-09 8:04 UTC (permalink / raw)
To: git
In-Reply-To: <20080709075345.GA22022@glandium.org>
On Wed, 9 Jul 2008, Mike Hommey wrote:
> Actually, the howto tells to setup a ~/.netrc file, which should make
> the http://user@server/path/ format work. I don't know if dropping user@
> would still work, that depends how libcurl works with it.
git has the netrc parser set to "optional", which makes libcurl use the login
name given in the URL (if present) and search for the associated password in
the .netrc file.
Typical users probably only have one user per host name in the .netrc file and
then the name part in the URL provides no particular extra goodness but can
just as well be excluded.
--
/ daniel.haxx.se
^ permalink raw reply
* Re: [PATCH] Fix problem with authentification on http repository.
From: Mike Hommey @ 2008-07-09 7:53 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Johannes Schindelin, jean.guyader, git, Jean Guyader
In-Reply-To: <7vlk0blgg0.fsf@gitster.siamese.dyndns.org>
On Tue, Jul 08, 2008 at 11:28:15PM -0700, Junio C Hamano <gitster@pobox.com> wrote:
> Mike Hommey <mh@glandium.org> writes:
>
> > On Tue, Jul 08, 2008 at 10:44:58PM -0700, Junio C Hamano wrote:
> >> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> >>
> >> > On Sat, 29 Sep 2007, Junio C Hamano wrote:
> >> >
> >> >> We never supported URLs with embedded credentials (see
> >> >> Documentation/urls.txt), partly because nobody asked for it, but
> >> >> more importantly because giving -n to curl to have it read from
> >> >> user's .netrc is generally much more preferred approach.
> >> >
> >> > To elaborate on that: if you fetch from somewhere, your url, username and
> >> > password can be read from the output of "ps ax | grep http" very easily.
> >>
> >> Actually Documentation/howto/setup-git-server-over-http.txt
> >> talks about http://user@server/path/ format. How well does this
> >> work in practice? If it does, we should update Documentation/urls.txt
> >> to allow optional user@ there like...
> >
> > AFAIK, there is nothing to deal with asking a password to the user in git
> > in this case, so that doesn't work.
>
> Then perhaps the howto needs updating?
Actually, the howto tells to setup a ~/.netrc file, which should make
the http://user@server/path/ format work. I don't know if dropping user@
would still work, that depends how libcurl works with it.
Mike
^ permalink raw reply
* Re: [PATCH] parse-options: add PARSE_OPT_FAKELASTARG flag.
From: Pierre Habouzit @ 2008-07-09 7:47 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Lars Hjemli
In-Reply-To: <7vvdzfoo1s.fsf@gitster.siamese.dyndns.org>
[-- Attachment #1: Type: text/plain, Size: 1790 bytes --]
On Wed, Jul 09, 2008 at 01:15:43AM +0000, Junio C Hamano wrote:
> Pierre Habouzit <madcoder@debian.org> writes:
>
> > If you set this for a given flag, and the flag appears without a value on
> > the command line, then the `defval' is used to fake a new argument.
> >
> > Note that this flag is meaningless in presence of OPTARG or NOARG flags.
> > (in the current implementation it will be ignored, but don't rely on it).
> >
> > Signed-off-by: Pierre Habouzit <madcoder@debian.org>
> > ---
> >
> > > (3) inspired from (1) and (2), have a flag for options that says
> > > "I do take an argument, but if I'm the last option on the
> > > command line, please fake this argument for me.
> > >
> > > I really like (3) more FWIW as it doesn't generate ambiguous
> > > parsers like (2) would, and it's not horrible like (1). And cherry
> > > on top it's pretty trivial to implement I think.
>
> Yeah, I do not particularly want a major rewrite that only introduces
> possible ambiguity to the option parser (even though arguably it would add
> to the usability very much, just like we accept revs and paths when
> unambiguous), so I think this is a reasonable compromise.
>
> It feels more like LASTARG_DEFAULT but that is bikeshedding ;-)
I absolutely don't like this FAKELASTARG name, so really, use what you
like.
> But I see one thinko (fix below) and another issue I am not sure what the
> best fix would be.
Like I said it was just a draft, I did not test the new feature, so
I'm not really surprised it's partly broken ;)
--
·O· Pierre Habouzit
··O madcoder@debian.org
OOO http://www.madism.org
[-- Attachment #2: Type: application/pgp-signature, Size: 197 bytes --]
^ permalink raw reply
* Re: [PATCH 2/2] branch --merged/--not-merged: allow specifying arbitrary commit
From: Pierre Habouzit @ 2008-07-09 7:45 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Lars Hjemli
In-Reply-To: <7vmykronr1.fsf_-_@gitster.siamese.dyndns.org>
[-- Attachment #1: Type: text/plain, Size: 1632 bytes --]
On Wed, Jul 09, 2008 at 01:22:10AM +0000, Junio C Hamano wrote:
> "git-branch --merged" is a handy way to list all the branches that have
> already been merged to the current branch, but it did not allow checking
> against anything but the current branch. Having to check out only for
> that purpose made the command practically useless.
>
> This updates the option parser so that "git branch --merged next" is
> accepted when you are on 'master' branch.
>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
>
> * This does have an issue. --no-<option>=<value> is often nonsense and
> parse-options does not accept it (and I do not think we would want to
> change it). The use of "--no-merged" was a mistake, but nobody has
> perfect foresight.
>
> This adds --not-merged <commit> and allows the <commit> to default to
> HEAD if not given to work it around. This and the previous one are not
> for application but primarily meant for discussion on what further
> flexibility we may want to have in parse-options.
There's a way: declare --merge as (PARSE_OPT_)NONEG to tell parse-opt
not to generate --no-merge by itself, and declare a --no-merge option as
well. I think it works, and if not, we should make parse-opt work with
that. This is a quite disgusting hack, and no *new* options should be
written this way, but we have to be flexible enough for backward
compatibility's sake.
--
·O· Pierre Habouzit
··O madcoder@debian.org
OOO http://www.madism.org
[-- Attachment #2: Type: application/pgp-signature, Size: 197 bytes --]
^ permalink raw reply
* [PATCH] Correct pack memory leak causing git gc to try to exceed ulimit
From: Shawn O. Pearce @ 2008-07-09 7:10 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Nicolas Pitre
When recursing to unpack a delta base we must unuse_pack() so that
the pack window for the current object does not remain pinned in
memory while the delta base is itself being unpacked and materialized
for our use.
On a long delta chain of 50 objects we may need to access 6 different
windows from a very large (>3G) pack file in order to obtain all
of the delta base content. If the process ulimit permits us to
map/allocate only 1.5G we must release windows during this recursion
to ensure we stay within the ulimit and transition memory from pack
cache to standard malloc, or other mmap needs.
Inserting an unuse_pack() call prior to the recursion allows us to
avoid pinning the current window, making it available for garbage
collection if memory runs low.
This has been broken since at least before 1.5.1-rc1, and very
likely earlier than that. Its fixed now. :)
Signed-off-by: Shawn O. Pearce <spearce@spearce.org>
---
sha1_file.c | 1 +
1 files changed, 1 insertions(+), 0 deletions(-)
diff --git a/sha1_file.c b/sha1_file.c
index d1c4060..10346b6 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -1609,6 +1609,7 @@ static void *unpack_delta_entry(struct packed_git *p,
off_t base_offset;
base_offset = get_delta_base(p, w_curs, &curpos, *type, obj_offset);
+ unuse_pack(w_curs);
base = cache_or_unpack_entry(p, base_offset, &base_size, type, 0);
if (!base)
die("failed to read delta base object"
--
1.5.6.74.g8a5e
^ permalink raw reply related
* Re: [PATCH] Fix problem with authentification on http repository.
From: Junio C Hamano @ 2008-07-09 6:28 UTC (permalink / raw)
To: Mike Hommey; +Cc: Johannes Schindelin, jean.guyader, git, Jean Guyader
In-Reply-To: <20080709055150.GA28482@glandium.org>
Mike Hommey <mh@glandium.org> writes:
> On Tue, Jul 08, 2008 at 10:44:58PM -0700, Junio C Hamano wrote:
>> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>>
>> > On Sat, 29 Sep 2007, Junio C Hamano wrote:
>> >
>> >> We never supported URLs with embedded credentials (see
>> >> Documentation/urls.txt), partly because nobody asked for it, but
>> >> more importantly because giving -n to curl to have it read from
>> >> user's .netrc is generally much more preferred approach.
>> >
>> > To elaborate on that: if you fetch from somewhere, your url, username and
>> > password can be read from the output of "ps ax | grep http" very easily.
>>
>> Actually Documentation/howto/setup-git-server-over-http.txt
>> talks about http://user@server/path/ format. How well does this
>> work in practice? If it does, we should update Documentation/urls.txt
>> to allow optional user@ there like...
>
> AFAIK, there is nothing to deal with asking a password to the user in git
> in this case, so that doesn't work.
Then perhaps the howto needs updating?
^ permalink raw reply
* Re: [PATCH 1/3] cherry: cache patch-ids to avoid repeating work
From: Junio C Hamano @ 2008-07-09 6:24 UTC (permalink / raw)
To: Geoffrey Irving; +Cc: Johannes Schindelin, git@vger.kernel.org
In-Reply-To: <7f9d599f0807082226oee83bedrf13d254ae12be274@mail.gmail.com>
"Geoffrey Irving" <irving@naml.us> writes:
> On Tue, Jul 8, 2008 at 10:14 PM, Junio C Hamano <gitster@pobox.com> wrote:
> ...
>>> }
Please don't quote the whole thing without trimming if you do not have any
interspersed comments/responses to quoted part.
> Should I rewrite the patch sequence to incorporate these changes into
> the first commit, or add them as a forth commit off the end?
I strongly encourage the latter. We try not to keep early mistakes in the
history (see my comments on your [2/3]).
It is not unusal for any sizeable new code to go through a few round of
review cycle without even queued to 'pu', and the general rule is until
the series hits 'next', it is either "rejected (dropped on the floor),
please resend an improved version" or "ok now it is good, will queue".
After queued in 'next', improvements will continue incrementally.
Think of this procedure as giving a chance for you to hide early
embarrassment under the rug ;-)
^ permalink raw reply
* Re: [PATCH] git-submodule - make "submodule add" more strict, and document it
From: Junio C Hamano @ 2008-07-09 6:04 UTC (permalink / raw)
To: Mark Levedahl; +Cc: git, sylvain.joyeux, hjemli, pkufranky
In-Reply-To: <1215575965-3588-1-git-send-email-mlevedahl@gmail.com>
Mark Levedahl <mlevedahl@gmail.com> writes:
> This change makes "submodule add" much more strict in the arguments it
> takes, and is intended to address confusion as recently noted on the
> git-list. With this change, the required syntax is:
> $ git submodule add URL path
>
Please have an extra blank line on both sides of examples.
> Specifically, this eliminates the form
> $git submodule add URL
> which was confused by more than one person as
> $git submodule add path
s/\$/& /;
> The change eliminates one form of URL: a path relative to the current
> working directory....
> ...
> Following this change, there are exactly four variants of
> submodule-add, as both arguments have two flavors:
>
> URL can be absolute, or can begin with ./|../ and thus name the origin
> relative to the top-level origin.
Now this _is_ confusing. Examples of a path relative to the current
working directory would be ./foo or ../../foo but the previous paragraph
says the form is eliminated. I think I know what you want to say, but it
still is confusing.
> add::
> Add the given repository as a submodule at the given path
> + to the changeset to be committed next. This requires two arguments,
> + <repository> and <path>. <repository> is the URL of the new
> + submodule's origin repository. This may be either an absolute URL,
> + or (if it begins with ./ or ../), the location relative
> + to the parent repository's origin.
This is much better than the part I found confusing above. Here, "parent
repository" actually means "this repository", right? It is the one that
owns the index you are adding the gitlink to and tracks the .gitmodules
file you are adding an entry for this submodule to.
> + <path> is the relative location for the cloned submodule to
> + exist in the current tree. If <path> does not exist, then the
> + module is created by cloning from the named URL. If <path> does
> + exist and is already a valid git repository, then this is added
> + to the changeset without cloning. This second form is provided
> + to ease adding a submodule to a project the first time, and presumes
> + the user will later push the new submodule to the given URL.
> +
> + In either case, the given URL is recorded into .gitmodules for
> + use by subsequent users cloning the project. If the URL is given
> + relative to the parent, the presumption is the main and sub-modules
We seem to say "superproject" and "submodule" elsewhere, including Linus's
talk.
> + will be kept together in the same relative location, and only the
> + top-level URL need be provided: git-submodule will correctly
> + locat the submodules using the hint in .gitmodules.
s/locat/&e/;
> @@ -150,16 +138,27 @@ cmd_add()
> ...
> + # assure repo is absolute or relative to parent
> + case "$repo" in
> + ./*|../*)
> + # dereference source url relative to parent's url
> + realrepo=$(resolve_relative_url "$repo") || exit
> + ;;
> + *:*|/*)
Funny indentation; "case/esac" and its arms align, like so:
case "$repo" in
./* | ../*)
...
;;
esac
^ permalink raw reply
* Re: [PATCH] Fix problem with authentification on http repository.
From: Mike Hommey @ 2008-07-09 5:51 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Johannes Schindelin, jean.guyader, git, Jean Guyader
In-Reply-To: <7v4p6zmx0l.fsf@gitster.siamese.dyndns.org>
On Tue, Jul 08, 2008 at 10:44:58PM -0700, Junio C Hamano wrote:
> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>
> > On Sat, 29 Sep 2007, Junio C Hamano wrote:
> >
> >> We never supported URLs with embedded credentials (see
> >> Documentation/urls.txt), partly because nobody asked for it, but
> >> more importantly because giving -n to curl to have it read from
> >> user's .netrc is generally much more preferred approach.
> >
> > To elaborate on that: if you fetch from somewhere, your url, username and
> > password can be read from the output of "ps ax | grep http" very easily.
>
> Actually Documentation/howto/setup-git-server-over-http.txt
> talks about http://user@server/path/ format. How well does this
> work in practice? If it does, we should update Documentation/urls.txt
> to allow optional user@ there like...
AFAIK, there is nothing to deal with asking a password to the user in git
in this case, so that doesn't work.
Mike
^ permalink raw reply
* Re: [PATCH] Fix problem with authentification on http repository.
From: Junio C Hamano @ 2008-07-09 5:44 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: jean.guyader, git, Jean Guyader
In-Reply-To: <7v4phdja01.fsf@gitster.siamese.dyndns.org>
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> On Sat, 29 Sep 2007, Junio C Hamano wrote:
>
>> We never supported URLs with embedded credentials (see
>> Documentation/urls.txt), partly because nobody asked for it, but
>> more importantly because giving -n to curl to have it read from
>> user's .netrc is generally much more preferred approach.
>
> To elaborate on that: if you fetch from somewhere, your url, username and
> password can be read from the output of "ps ax | grep http" very easily.
Actually Documentation/howto/setup-git-server-over-http.txt
talks about http://user@server/path/ format. How well does this
work in practice? If it does, we should update Documentation/urls.txt
to allow optional user@ there like...
---
Documentation/urls.txt | 4 ++--
1 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/Documentation/urls.txt b/Documentation/urls.txt
index e67f914..c9aab86 100644
--- a/Documentation/urls.txt
+++ b/Documentation/urls.txt
@@ -6,8 +6,8 @@ to name the remote repository:
===============================================================
- rsync://host.xz/path/to/repo.git/
-- http://host.xz/path/to/repo.git/
-- https://host.xz/path/to/repo.git/
+- http://{startsb}user@{endsb}host.xz/path/to/repo.git/
+- https://{startsb}user@{endsb}host.xz/path/to/repo.git/
- git://host.xz/path/to/repo.git/
- git://host.xz/~user/path/to/repo.git/
- ssh://{startsb}user@{endsb}host.xz{startsb}:port{endsb}/path/to/repo.git/
^ permalink raw reply related
* Re: [PATCH 2/3] cached-sha1-map: refactoring hash traversal code
From: Junio C Hamano @ 2008-07-09 5:27 UTC (permalink / raw)
To: Geoffrey Irving; +Cc: Johannes Schindelin, git@vger.kernel.org, Junio C Hamano
In-Reply-To: <7f9d599f0807082056o666fced9nf87cc81447e16e05@mail.gmail.com>
"Geoffrey Irving" <irving@naml.us> writes:
> From c4e60c28fe66985ac8224da832589c982010744e Mon Sep 17 00:00:00 2001
> From: Geoffrey Irving <irving@naml.us>
> Date: Tue, 8 Jul 2008 19:47:22 -0700
> Subject: [PATCH 2/3] cached-sha1-map: refactoring hash traversal code
>
> Pulling common code from get_cached_sha1_entry and set_cached_sha1_entry
> into static find_helper function.
> ---
Sign-off?
> cached-sha1-map.c | 68 +++++++++++++++++++++++++++++-----------------------
> 1 files changed, 38 insertions(+), 30 deletions(-)
The refactoring is good, and it should have been that way from the
beginning. Please don't send in "introduce foo.c [1/N]", "oops, initial
version of foo.c was crap, here is a fixup [2/N]".
> diff --git a/cached-sha1-map.c b/cached-sha1-map.c
> index e363745..147c7a2 100644
> --- a/cached-sha1-map.c
> +++ b/cached-sha1-map.c
> @@ -140,43 +144,47 @@ int get_cached_sha1_entry(struct cached_sha1_map *cache,
> mask = cache->size - 1;
>
> for (i = get_hash_index(key) & mask; ; i = (i+1) & mask) {
> - if (!hashcmp(key, cache->entries[i].key)) {
> - hashcpy(value, cache->entries[i].value);
> - return 0;
> - } else if (is_null_sha1(cache->entries[i].key))
> - return -1;
> + if (!hashcmp(key, cache->entries[i].key))
> + return i;
> + else if (is_null_sha1(cache->entries[i].key))
> + return ~i;
> }
> }
>
> +int get_cached_sha1_entry(struct cached_sha1_map *cache,
> + const unsigned char *key, unsigned char *value)
> +{
> + long i = find_helper(cache, key);
> + if(i < 0)
> + return -1;
Does this have to be extern?
If you are designing an API from scratch, and if you want a long, use it
consistently. Do not demote an int to shorter int in a callchain
unnecessarily.
^ permalink raw reply
* Re: [PATCH 1/3] cherry: cache patch-ids to avoid repeating work
From: Geoffrey Irving @ 2008-07-09 5:26 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Johannes Schindelin, git@vger.kernel.org
In-Reply-To: <7vfxqjmyg2.fsf@gitster.siamese.dyndns.org>
On Tue, Jul 8, 2008 at 10:14 PM, Junio C Hamano <gitster@pobox.com> wrote:
> "Geoffrey Irving" <irving@naml.us> writes:
>
>> From a3afd1455d215a541e1481e0f064df743d9219cc Mon Sep 17 00:00:00 2001
>
> Please drop this line.
>
>> From: Geoffrey Irving <irving@naml.us>
>> Date: Sat, 7 Jun 2008 16:03:49 -0700
>> Subject: [PATCH 1/3] cherry: cache patch-ids to avoid repeating work
>
> These are Ok _if_ the difference between them and what you have in your
> e-mail header really matter (e.g. you are forwarding somebody else's
> patch). I don't think it is in this case, though.
>
>> Added cached-sha-map.[hc] implementing a persistent hash map from sha1 to
>> sha1.
>
> "Add cached-sha1-map.[ch]" (imperative mood), not "(here is what I) _did_".
>
>> diff --git a/cached-sha1-map.c b/cached-sha1-map.c
>> new file mode 100644
>> index 0000000..e363745
>> --- /dev/null
>> +++ b/cached-sha1-map.c
>> @@ -0,0 +1,182 @@
>> +#include "cached-sha1-map.h"
>> +
>> +union cached_sha1_map_header {
>> + struct {
>> + char signature[4]; /* HASH */
>> + off_t count, size;
>> + };
>> + struct cached_sha1_entry padding; /* pad header out to 40 bytes */
>> +};
>> +
>> +static const char *signature = "HASH";
>
> That sounds a bit too generic, doesn't it, to protect ourselves from
> getting confused by some other filetype?
>
>> +static void init_cached_sha1_map(struct cached_sha1_map *cache)
>> +{
>> + int fd;
>> + union cached_sha1_map_header header;
>> +
>> + if (cache->initialized)
>> + return;
>> +
>> + fd = open(git_path(cache->filename), O_RDONLY);
>> + if (fd < 0) {
>> + init_empty_map(cache, 64);
>> + return;
>
> Check errno and do this only when ENOENT. Other errors should be caught
> and reported.
>
>> + }
>> +
>> + if (read_in_full(fd, &header, sizeof(header)) != sizeof(header))
>> + die("cannot read %s header", cache->filename);
>> +
>> + if (memcmp(header.signature, signature, 4))
>> + die("%s has invalid header", cache->filename);
>> +
>> + if (header.size & (header.size-1))
>> + die("%s size %lld is not a power of two", cache->filename,
>> + (long long)header.size);
>
> Two issues and a half:
>
> - Isn't it gcc extension to be able to say header.signature, bypassing
> the anonymous structure inside the union that the "header" itself is?
>
> - The signature header (count and size) is defined to be off_t, which
> makes the cached file unusable across architectures. The map header
> structure should be specified with explicit size:
>
> union {
> struct {
> char sig[4];
> uint32_t version;
> uint32_t count;
> unit32_t size;
> } u;
> struct cached_sha1_entry pad;
> };
>
> the uint32_t fields should be treated as network byte order integers,
> e.g.
>
> cache->count = ntohl(header.u.count);
> header.u.size = htonl(cache->size);
>
> - If this file is truly intended as "cache", shouldn't corruption of it
> be detected, reported but otherwise ignored, so that the lookup would
> continue in degraded uncached mode?
>
>> + /* check off_t to size_t conversion */
>> + if (cache->count != header.count || cache->size != header.size)
>> + die("%s is too large to hold in memory", cache->filename);
>
> This does not make sense to me. What you are checking does not match the
> error message.
>
> If you are making the file format architecture dependent (which I would
> suggest strongly against), you can use the same type and be done with it.
> Otherwise, if you are making the format portable across architectures,
> then you would know how large the on-disk integer will be, so as long as
> you use appropriate type that is large enough for cache->count you should
> be Ok.
>
> What you may want to check is that (header.u.size + 1) * sizeof(entry)
> does not wrap around, but you don't.
>
>> + /* mmap entire file so that file / memory blocks are aligned */
>> + cache->entries = xmmap(NULL,
>> + sizeof(struct cached_sha1_entry) * (header.size + 1),
>> + PROT_READ | PROT_WRITE, MAP_PRIVATE, fd, 0);
>
> I think this will die() if the file is too large to map. Ideally you
> would want to allow this mmap to fail if the cache is too large, in which
> case you can gracefully degrade to cacheless mode of operation, but that
> can probably be left to 47th round.
>
>> +int write_cached_sha1_map(struct cached_sha1_map *cache)
>> +{
>> + union cached_sha1_map_header header;
>> + struct lock_file update_lock;
>> + int fd;
>> + size_t entry_size;
>> +
>> + if (!cache->initialized || !cache->dirty)
>> + return 0;
>> +
>> + fd = hold_lock_file_for_update(&update_lock,
>> + git_path(cache->filename), 0);
>> +
>> + if (fd < 0)
>> + return error("could not construct %s", cache->filename);
>
> Use a "const char *" to hold git_path(cache->filename) upfront in the
> function, use it to obtain lock _and_ for reporting.
>
>> + memcpy(header.signature, signature, 4);
>> + header.count = cache->count;
>> + header.size = cache->size;
>
> And here will be htonl().
>
>> + entry_size = sizeof(struct cached_sha1_entry) * cache->size;
>
> Typically "entry_size" means the size of individual entry; this is the
> size of the whole thing.
>
>> + if (write_in_full(fd, &header, sizeof(header)) != sizeof(header)
>> + || write_in_full(fd, cache->entries, entry_size) != entry_size)
>> + return error("could not write %s", cache->filename);
>> +
>> + if (commit_lock_file(&update_lock) < 0)
>> + return error("could not write %s", cache->filename);
>> +
>> + cache->dirty = 0;
>> + return 0;
>> +}
>
> But it is good that you used this intermediate variable; the above
> write_in_full() is much easier to read than the xmmap() above at the end
> of init_cached_sha1_map() function.
>
>> +static size_t get_hash_index(const unsigned char *sha1)
>> +{
>> + return ntohl(*(size_t*)sha1);
>> +}
>
> Two issues:
>
> - I do not see any guarantee that sha1 is suitably aligned for reading
> size_t bytes off of;
>
> - size_t is architecture dependent, so you would get different hash value
> depending on the architecture, which again makes this file format
> unportable.
>
>> diff --git a/patch-ids.c b/patch-ids.c
>> index 3be5d31..36332f3 100644
>> --- a/patch-ids.c
>> +++ b/patch-ids.c
>> @@ -2,17 +2,31 @@
>> #include "diff.h"
>> #include "commit.h"
>> #include "patch-ids.h"
>> +#include "cached-sha1-map.h"
>> +
>> +struct cached_sha1_map patch_id_cache;
>
> Does this have to be extern?
>
>> static int commit_patch_id(struct commit *commit, struct diff_options *options,
>> unsigned char *sha1)
>> {
>> + /* pull patch-id out of the cache if possible */
>> + patch_id_cache.filename = "patch-id-cache";
>> + if (!get_cached_sha1_entry(&patch_id_cache, commit->object.sha1, sha1))
>> + return 0;
>> +
>> if (commit->parents)
>> diff_tree_sha1(commit->parents->item->object.sha1,
>> commit->object.sha1, "", options);
>> else
>> diff_root_tree_sha1(commit->object.sha1, "", options);
>> diffcore_std(options);
>> - return diff_flush_patch_id(options, sha1);
>> + int ret = diff_flush_patch_id(options, sha1);
>
> Decl-after-statement.
>
>> + if (ret)
>> + return ret;
>> +
>> + /* record commit, patch-id pair in cache */
>> + set_cached_sha1_entry(&patch_id_cache, commit->object.sha1, sha1);
>> + return 0;
>> }
Thanks. I'll fix these in the next few days.
Should I rewrite the patch sequence to incorporate these changes into
the first commit, or add them as a forth commit off the end?
Geoffrey
^ 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