Re: [PATCH 04/17] Name local variables more consistently
On 08/23/2012 10:39 AM, Jeff King wrote:
> On Thu, Aug 23, 2012 at 10:10:29AM +0200, mhag...@alum.mit.edu wrote:
>
>> From: Michael Haggerty
>>
>> Use the names (nr_heads, heads) consistently across functions, instead
>> of sometimes naming the same values (nr_match, match).
>
> I think this is fine, although:
>
>> --- a/builtin/fetch-pack.c
>> +++ b/builtin/fetch-pack.c
>> @@ -521,7 +521,7 @@ static void mark_recent_complete_commits(unsigned long
>> cutoff)
>> }
>> }
>>
>> -static void filter_refs(struct ref **refs, int nr_match, char **match)
>> +static void filter_refs(struct ref **refs, int nr_heads, char **heads)
>> {
>> struct ref **return_refs;
>> struct ref *newlist = NULL;
>> @@ -530,12 +530,12 @@ static void filter_refs(struct ref **refs, int
>> nr_match, char **match)
>> struct ref *fastarray[32];
>> int match_pos;
>
> This match_pos is an index into the "match" array, which becomes "head".
> Should it become head_pos?
>
> And then bits like this:
>
>> -while (match_pos < nr_match) {
>> -cmp = strcmp(ref->name, match[match_pos]);
>> +while (match_pos < nr_heads) {
>> +cmp = strcmp(ref->name, heads[match_pos]);
>
> Would be:
>
> while (head_pos < nr_heads)
>
> which makes more sense to me.
I was up in the air about this, because match_pos *is* the position at
which a match is attempted. But since the name also strikes you as
wrong, I will change it in the next version.
Thanks for this and all of your other comments!
Michael
--
Michael Haggerty
mhag...@alum.mit.edu
http://softwareswirl.blogspot.com/
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Re*: mergetool: support --tool-help option like difftool does
On Thu, Aug 23, 2012 at 10:39 AM, Junio C Hamano wrote: > David Aguilar writes: >> Would the ability to resolve the various merge situations using >> the command-line be a wanted addition? >> >> This would let a submodule or deleted/modified encountering >> user do something like: >> >> $ git mergetool --theirs -- submodule >> >> ...and not have to remember the various git commands that it runs. > > Does it have to run various git commands? For a normal path, all it > needs to do is "git checkout --theirs $path", no? > > What does it mean to resolve a conflicted merge of a gitlink to take > "theirs"? We obviously would want to point the resolved gitlink at > the submodule commit their side wants in the resulting index but what, > if any, should we do to the submodule itself? > > Stepping back a bit, if there is no conflict, and as a result of a > clean merge (this applies to the case where you check out another > branch that has different commit at the submodule path), if gitlink > changed to point at a different commit in the submodule, what should > happen? > > If you start from a clean working tree, with your gitlink pointing > at the commit that matches HEAD in the submodule, and if the working > tree of the submodule does not have any local modification, it may > be ideal to check out the new commit in the submodule (are there > cases where "git checkout other_branch" in the superproject does not > want to touch the submodule working tree?). > > There are cases where it is not possible; checking out the new > commit in the submodule working tree may not succeed due to local > modifications. But is that kind of complication limited to the > merge resolution case? Isn't it shared with normal "switching > branches" case as well? > > If so, it could be that your "git mergetool --theirs -- submodule" > is working around a wrong problem, and the right solution may be to > make "git checkout --theirs -- $path" to always do an appropriate > thing regardless of what kind of object $path is, no? True. Admittedly, I'm not a heavy submodule user myself so I tried crafting the directory vs submodule conflict to see what the usability is like. checkout --theirs and --ours could learn a few tricks. When trying to choose the directory in that situation I had to had to "git rm --cached" the submodule path so that git would recognize that there was no longer a conflict. That makes sense to me because I was following along by reading the mergetool code, but I don't think most users would know to "git rm" a path which they intend to keep. Afterwards, the .git file is left behind, which could cause problems elsewhere since we really don't want a .git file in that situation. I'm not even sure what to do about the .gitmodules file either. Here's the script I was using to setup the merge conflict in case anyone wants to get a feel for the usability around this edge case. Pass --submodule if you want the remote side to have a submodule. By default, the local side has a submodule and the remote side of the merge brings along a directory. That said, this really isn't an issue, per say. I first poked at it because I noticed that mergetool still needed stdin for some things. It's certainly an edge case, and perhaps this just shows that mergetool really is the right porcelain for the job when a user runs into these types of conflicts (the stdin thing really isn't an issue). #!/bin/sh if test "$1" = "--submodule" then first=with-directory second=with-submodule echo "local will be a directory, remote will be a submodule" else first=with-submodule second=with-directory echo "local will be a submodule, remote will be a directory" fi repo=submodule-conflict-$$ && echo "creating $repo" && mkdir "$repo" && cd "$repo" && git init && git commit --allow-empty -m'initial' && git checkout -b with-directory master && mkdir the-conflict && touch the-conflict/path && git add the-conflict/path && git commit -m'added path into the-conflict' && git checkout -b with-submodule master && git submodule add ./ the-conflict && git commit -m'added submodule as the-conflict' && git checkout -b tmp-merge master && git merge $first && git merge $second -- David -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/6] Gettext poison rework
On Fri, Aug 24, 2012 at 7:43 AM, Nguyễn Thái Ngọc Duy wrote: > Still WIP but I'm getting closer. I dropped test-poisongen and started > to use podebug [2] instead. Less code in git. podebug does not preserve > shell variables yet. I'll follow that up at upstream [1]. > > With this series, if you have translation toolkit installed, you could > do > > make pseudo-locale L= > make GETTEXT_POISON=$LANG test > > podebug supports a few way of rewriting translations. Currently > "unicode" is used but you can change it via PODEBUG_OPTS > > t9001 is not happy with $LANG != C though. May need to add some > prereq there. > > [1] http://bugs.locamotion.org/show_bug.cgi?id=2450 > [2] http://translate.sourceforge.net/wiki/toolkit/podebug The reason I didn't do something like this to begin with is that gettext/glibc doesn't have support for fake locales, so you'd have to appropriate a real one for tests. It's good to see you poking the gettext mailing list about adding support far thot. But something like podebug gets around that quite nicely, so we can still have the testing the poison stuff was intended for, without the complexity of supporting it throughout all our i18n code. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [Q] Comparing differences introduced by two commits?
On Wednesday 22-August-2012 10:55:29 Jonathan del Strother wrote: > On 22 August 2012 17:58, Junio C Hamano wrote: > > Jonathan del Strother writes: > >> On 22 August 2012 13:10, Brian Foster wrote: > >> ... > >>> In the past I've done: > >>> > >>> diff <(git show A) <(git show B) > >>> > >>> which produces rather messy output [...] > > > > Isn't this what interdiff is for? I'd never(?) heard of interdiff(1) — THANKS! With my current problem it produces (1) Some false results, and (2) Gets enough patch-rejects so as to be useful only in getting a 10km-high overview. Nonetheless, it's a help. > >>> Some searching hasn't found any suggestions I'm too happy > >>> with, albeit I've very possibly overlooked something. > >> > >> What about cherry picking B onto A, then showing the cherry-picked commit? > >>[...] > > I often do > > > > git checkout A^ > > git cherry-pick B > > git diff A > > > > when queuing an updated patch. This works fairly well. I get conflicts (not surprising), which _probably_ corrolate rather well to the interdiff patch-rejects (not checked), but the advantage here is I can easily see what's going on (what the conflict _is_). Neither compares commit-comments, but that is a obviously a scriptable problem. As it so happens, it turns out my number of A/B pairs is rather less than expected (c.50 not the estimated c.90), of which c.10 get cherry-pick conflicts. So the problem is now looking quite tractable. Thanks for the help! cheers, -blf- -- Brian Foster Principal MTS, Software| La Ciotat, France Maxim Integrated Products | Web: http://www.maxim-ic.com/ -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Reg:Git-ssh bug
On Thu, Aug 23, 2012 at 06:22:01PM -0400, Gokulramkumar Subramaniam wrote: > I am new to Git and I am trying to add my machine with Git but it is failing > through ssh method. > > Error received: > > $ ssh-add -l > 2048 5f:6f:39:ed:b0:76:2e:d0:xx:xx:xx:xx:xx:xx:xx:xx id_rsa (RSA) > > Gokul$ ssh -vT g...@github.com > [...] > debug1: Authentications that can continue: publickey > debug1: Next authentication method: publickey > debug1: Offering RSA public key: id_rsa > debug1: Authentications that can continue: publickey > debug1: Offering RSA public key: /Users/Gokul/.ssh/id_rsa > debug1: Authentications that can continue: publickey > debug1: Trying private key: /Users/Gokul/.ssh/id_dsa > debug1: No more authentication methods to try. > Permission denied (publickey). That means that the server does not like your public key, typically because it does not match a keypair that has been uploaded. This is a GitHub issue, not a Git issue. Double-check that your key is listed at: https://github.com/settings/ssh and add it there if necessary. If it is still not working, then contact GitHub support (supp...@github.com). -Peff -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: in_merge_bases() is too expensive for recent "pu" update
Junio C Hamano writes:
> As a corollary, the "is pu@{0} a fast-forward of pu@{1}?" check does
> not need merge base computation at all. The only thing it needs to
> do is to prove pu@{1} is reachable from pu@{0}, and that can be done
> the same way in which '1' can be proved unreachable from '2' in the
> post processing phase as described above, i.e. it needs only the
> first phase of running merge_bases_many() without postprocessing.
Well, yeah, you snipped this part from my original post :-)
} Even if this turns out to be flawed, we should also identify uses of
} in_merge_bases() where the real question was is_descendant_of() [I
} somewhat suspect that's all of them], and then replace is_descendant_of
} with a much cheaper lookup. This can be as simple as propagating a mark
} from the candidate until it either goes beyond all possible ancestors,
} or hits one of them.
As far as the "real question" goes, I'm purely guessing here -- it is
entirely possible that a bunch of the in_merge_bases() checks really do
need the pruned set of merge bases. But this particular check, and I
suspect a bunch of others, does not.
Then there's the next bit:
} By the way, the internal slowness of git-merge-base also affects the
} A...B syntax. For example,
}
} git rev-list --left-right --count @{upstream}...
}
} is used by the __git_ps1 code to determine the prompt display, which
} just got very slow for me today. Again, it should be easy to figure out
} the boundary of the symmetric difference simply by propagating two
} marks. I do not think that the result of A...B actually depends on
} figuring out exactly what the merge bases are, simply excluding *any*
} candidate without pruning is enough.
Apart from __git_ps1, this is also used by git-status, git-checkout and
'git branch -v' to show "your branch is N behind and M ahead". Again
it's a bit of a hunch, but I think figuring out the symmetric difference
is a simple matter of propagating two marks and including only commits
that ended up having exactly one of them. At least the counting case
should be easy, rev-list is slightly harder to fix.
--
Thomas Rast
trast@{inf,student}.ethz.ch
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: in_merge_bases() is too expensive for recent "pu" update
Junio C Hamano writes:
> Junio C Hamano writes:
>
>> Thomas Rast writes:
>>
>>> At the very least it should be possible to change in_merge_bases() to
>>> not do any of the post-filtering; perhaps like the patch below.
>>
>> I do not recall the details but the post-filtering was added after
>> the initial naive version without it that was posted to the list did
>> not work correctly in corner cases. I wouldn't doubt that N*(N-1)
>> loop can be optimized to something with log(N), but I offhand find
>> it hard to believe "to not do any" could be correct without seeing
>> more detailed analysis.
>
> If "one on one side, many on the other side" in merge_bases_many()
> confuses any of you in the readers, you can just ignore many-ness.
> Getting merge bases for "one" against many "two"s using
> merge_bases_many() is exactly the same as getting merge bases for
> "one" against a (fictitious) single commit you can make by merging
> all "two"s.
>
> So we can think about the degenerate case of merge base between two
> commits A and B in the following discussion.
>
> A merge-base between A and B is defined to be a commit that is a
> common ancestor of both A and B, and that is not an ancestor of any
> other merge-base between A and B.
Ok, under that definition I really do think that it's "easy".
You have all the pieces here but one:
> Start from A and B. Follow from B to find 'x' and paint it in blue,
> follow from A to find 'y' and paint it in amber. Follow from 'x' to
> '1', paint it in blue. Follow from 'y' to '1', paint it in amber
> but notice that it already is painted in blue.
[...]
> o---o
>/ \
> / y---A
> / /
> ---2---z---1---x---B
> \ /
> o---o
[...]
> So we need to notice that '1' and '2' have ancestry relation in
> order to reject '2' as "common but not merge-base". One way of
> doing so is not to stop at '1' and keep digging (and eventually we
> find that '2' is what we could reach from '1' that already is a
> merge base), but then we will be susceptible to the same kind of
> clock skew issue as the revision traverser.
I think that is *the* way to do it.
And the way to fix the clock skew issue (also in the revision traversal)
is Peff's generation number cache. Just keep propagating marks, digging
in generation order, until the generations prove that nothing new can
happen.
[Side note, in reply to an earlier comment in the rev-list thread: I
agree with Peff's reasoning that a cache is better than a commit
header.]
The precise termination condition should be:
There are no more one-colored commits to walk, and
The maximum generation of the remaining commits in the walk is less
than the minimum generation of the merge base candidates
Then the generation numbers ensure that no commit in the walk (which by
then only propagates STALE marks) can possibly be a descendant of any of
the candidates. At that point, the candidates are the true set of merge
bases.
I conjecture that every history walking problem can be solved in time
linear in the number of commits once we properly use the generation
numbers ;-)
--
Thomas Rast
trast@{inf,student}.ethz.ch
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2] Support non-WIN32 system lacking poll() while keeping the WIN32 part intact
Signed-off-by: Joachim Schmitz --- This time I hopefully didn't screw up whitespace and line breaks. Makefile| 18 ++ compat/win32/poll.c | 8 ++-- 2 files changed, 20 insertions(+), 6 deletions(-) diff --git a/Makefile b/Makefile index 66e8216..e150816 100644 --- a/Makefile +++ b/Makefile @@ -152,6 +152,11 @@ all:: # # Define NO_MMAP if you want to avoid mmap. # +# Define NO_SYS_POLL_H if you don't have sys/poll.h. +# +# Define NO_POLL if you do not have or do not want to use poll. +# This also implies NO_SYS_POLL_H. +# # Define NO_PTHREADS if you do not have or do not want to use Pthreads. # # Define NO_PREAD if you have a problem with pread() system call (e.g. @@ -1216,7 +1221,7 @@ ifeq ($(uname_S),Windows) NO_PREAD = YesPlease NEEDS_CRYPTO_WITH_SSL = YesPlease NO_LIBGEN_H = YesPlease - NO_SYS_POLL_H = YesPlease + NO_POLL = YesPlease NO_SYMLINK_HEAD = YesPlease NO_IPV6 = YesPlease NO_UNIX_SOCKETS = YesPlease @@ -1257,7 +1262,7 @@ ifeq ($(uname_S),Windows) BASIC_CFLAGS = -nologo -I. -I../zlib -Icompat/vcbuild -Icompat/vcbuild/include -DWIN32 -D_CONSOLE -DHAVE_STRING_H -D_CRT_SECURE_NO_WARNINGS -D_CRT_NONSTDC_NO_DEPRECATE COMPAT_OBJS = compat/msvc.o compat/winansi.o \ compat/win32/pthread.o compat/win32/syslog.o \ - compat/win32/poll.o compat/win32/dirent.o + compat/win32/dirent.o COMPAT_CFLAGS = -D__USE_MINGW_ACCESS -DNOGDI -DHAVE_STRING_H -DHAVE_ALLOCA_H -Icompat -Icompat/regex -Icompat/win32 -DSTRIP_EXTENSION=\".exe\" BASIC_LDFLAGS = -IGNORE:4217 -IGNORE:4049 -NOLOGO -SUBSYSTEM:CONSOLE -NODEFAULTLIB:MSVCRT.lib EXTLIBS = user32.lib advapi32.lib shell32.lib wininet.lib ws2_32.lib @@ -1312,7 +1317,7 @@ ifneq (,$(findstring MINGW,$(uname_S))) NO_PREAD = YesPlease NEEDS_CRYPTO_WITH_SSL = YesPlease NO_LIBGEN_H = YesPlease - NO_SYS_POLL_H = YesPlease + NO_POLL = YesPlease NO_SYMLINK_HEAD = YesPlease NO_UNIX_SOCKETS = YesPlease NO_SETENV = YesPlease @@ -1347,7 +1352,7 @@ ifneq (,$(findstring MINGW,$(uname_S))) COMPAT_CFLAGS += -DSTRIP_EXTENSION=\".exe\" COMPAT_OBJS += compat/mingw.o compat/winansi.o \ compat/win32/pthread.o compat/win32/syslog.o \ - compat/win32/poll.o compat/win32/dirent.o + compat/win32/dirent.o EXTLIBS += -lws2_32 PTHREAD_LIBS = X = .exe @@ -1601,6 +1606,11 @@ ifdef NO_GETTEXT BASIC_CFLAGS += -DNO_GETTEXT USE_GETTEXT_SCHEME ?= fallthrough endif +ifdef NO_POLL + NO_SYS_POLL_H = YesPlease + COMPAT_CFLAGS += -DNO_POLL -Icompat/win32 # so it finds poll.h + COMPAT_OBJS += compat/win32/poll.o +endif ifdef NO_STRCASESTR COMPAT_CFLAGS += -DNO_STRCASESTR COMPAT_OBJS += compat/strcasestr.o diff --git a/compat/win32/poll.c b/compat/win32/poll.c index 403eaa7..49541f1 100644 --- a/compat/win32/poll.c +++ b/compat/win32/poll.c @@ -24,7 +24,9 @@ # pragma GCC diagnostic ignored "-Wtype-limits" #endif -#include +#if defined(WIN32) +# include +#endif #include @@ -48,7 +50,9 @@ #else # include # include -# include +# ifndef NO_SYS_SELECT_H +# include +# endif # include #endif -- 1.7.12 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2] Prefer sysconf(_SC_OPEN_MAX) over getrlimit(RLIMIT_NOFILE,...)
Signed-off-by: Joachim Schmitz
---
As discussed now as a small helper function rather than #ifdef/#endif in the
primary flow of the code.
And hopefully without having screwed up whitespace and line breaks
sha1_file.c | 22 +++---
1 file changed, 15 insertions(+), 7 deletions(-)
diff --git a/sha1_file.c b/sha1_file.c
index af5cfbd..427f9e6 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -731,6 +731,20 @@ void free_pack_by_name(const char *pack_name)
}
}
+static unsigned int get_max_fd_limit(void)
+{
+#ifdef _SC_OPEN_MAX
+ return sysconf(_SC_OPEN_MAX);
+#else
+ struct rlimit lim;
+
+ if (getrlimit(RLIMIT_NOFILE, &lim))
+ die_errno("cannot get RLIMIT_NOFILE");
+
+ return lim.rlim_cur;
+#endif
+}
+
/*
* Do not call this directly as this leaks p->pack_fd on error return;
* call open_packed_git() instead.
@@ -747,13 +761,7 @@ static int open_packed_git_1(struct packed_git *p)
return error("packfile %s index unavailable", p->pack_name);
if (!pack_max_fds) {
- struct rlimit lim;
- unsigned int max_fds;
-
- if (getrlimit(RLIMIT_NOFILE, &lim))
- die_errno("cannot get RLIMIT_NOFILE");
-
- max_fds = lim.rlim_cur;
+ unsigned int max_fds = get_max_fd_limit();
/* Save 3 for stdin/stdout/stderr, 22 for work */
if (25 < max_fds)
--
1.7.12
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2] Don't use curl_easy_strerror prior to curl-7.12.0
This reverts be22d92 (http: avoid empty error messages for some curl errors,
2011-09-05) on platforms with older versions of libcURL.
Signed-off-by: Joachim Schmitz
---
Resend, regardless that Junio said this not to be needed, as I don't see it
applied yet.
Also tried to fix the formatting issues too, to practice submissions ;-)
http.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/http.c b/http.c
index b61ac85..18bc6bf 100644
--- a/http.c
+++ b/http.c
@@ -806,10 +806,12 @@ static int http_request(const char *url, void *result,
int target, int options)
ret = HTTP_REAUTH;
}
} else {
+#if LIBCURL_VERSION_NUM >= 0x070c00
if (!curl_errorstr[0])
strlcpy(curl_errorstr,
curl_easy_strerror(results.curl_result),
sizeof(curl_errorstr));
+#endif
ret = HTTP_ERROR;
}
} else {
--
1.7.12
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 1/2] Ignore trailing slash in mkdir() on platforms that can't deal with this
Signed-off-by: Joachim Schmitz
---
compat/mkdir.c | 24
1 file changed, 24 insertions(+)
create mode 100644 compat/mkdir.c
diff --git a/compat/mkdir.c b/compat/mkdir.c
new file mode 100644
index 000..9e253fb
--- /dev/null
+++ b/compat/mkdir.c
@@ -0,0 +1,24 @@
+#include "../git-compat-util.h"
+#undef mkdir
+
+/* for platforms that can't deal with a trailing '/' */
+int compat_mkdir_wo_trailing_slash(const char *dir, mode_t mode)
+{
+ int retval;
+ char *tmp_dir = NULL;
+ size_t len = strlen(dir);
+
+ if (len && dir[len-1] == '/') {
+ if ((tmp_dir = strdup(dir)) == NULL)
+ return -1;
+ tmp_dir[len-1] = '\0';
+ }
+ else
+ tmp_dir = (char *)dir;
+
+ retval = mkdir(tmp_dir, mode);
+ if (tmp_dir != dir)
+ free(tmp_dir);
+
+ return retval;
+}
--
1.7.12
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 2/2] Ignore trailing slash in mkdir() on platforms that can't deal with this
Signed-off-by: Joachim Schmitz --- git-compat-util.h | 5 + 1 file changed, 5 insertions(+) diff --git a/git-compat-util.h b/git-compat-util.h index 35b095e..34f040f 100644 --- a/git-compat-util.h +++ b/git-compat-util.h @@ -162,6 +162,11 @@ #define probe_utf8_pathname_composition(a,b) #endif +#ifdef MKDIR_WO_TRAILING_SLASH +#define mkdir(a,b) compat_mkdir_wo_trailing_slash((a),(b)) +extern int compat_mkdir_wo_trailing_slash(const char*, mode_t); +#endif + #ifndef NO_LIBGEN_H #include #else -- 1.7.12 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 1/2] Support for setitimer() on platforms lacking it
Implementation includes getitimer(), but for now it is static.
Supports ITIMER_REAL only.
Signed-off-by: Joachim Schmitz
---
May need a header file for ITIMER_*, struct itimerval and the prototypes,
But for now, and the HP NonStop platform this isn't needed, here
has ITIMER_* and struct timeval, and the prototypes can
vo into git-compat-util.h for now (Patch 2/2)
compat/itimer.c | 50 ++
1 file changed, 50 insertions(+)
create mode 100644 compat/itimer.c
diff --git a/compat/itimer.c b/compat/itimer.c
new file mode 100644
index 000..713f1ff
--- /dev/null
+++ b/compat/itimer.c
@@ -0,0 +1,50 @@
+#include "../git-compat-util.h"
+
+static int git_getitimer(int which, struct itimerval *value)
+{
+ int ret = 0;
+
+ switch (which) {
+ case ITIMER_REAL:
+ value->it_value.tv_usec = 0;
+ value->it_value.tv_sec = alarm(0);
+ ret = 0; /* if alarm() fails, we get a SIGLIMIT */
+ break;
+ case ITIMER_VIRTUAL: /* FALLTHRU */
+ case ITIMER_PROF: errno = ENOTSUP; ret = -1; break;
+ default: errno = EINVAL; ret = -1;
+ }
+ return ret;
+}
+
+int git_setitimer(int which, const struct itimerval *value,
+ struct itimerval *ovalue)
+{
+ int ret = 0;
+
+ if (!value
+ || value->it_value.tv_usec < 0
+ || value->it_value.tv_usec > 100
+ || value->it_value.tv_sec < 0) {
+ errno = EINVAL;
+ return -1;
+ }
+
+ else if (ovalue)
+ if (!git_getitimer(which, ovalue))
+ return -1; /* errno set in git_getitimer() */
+
+ else
+ switch (which) {
+ case ITIMER_REAL:
+ alarm(value->it_value.tv_sec +
+ (value->it_value.tv_usec > 0) ? 1 : 0);
+ ret = 0; /* if alarm() fails, we get a SIGLIMIT */
+ break;
+ case ITIMER_VIRTUAL: /* FALLTHRU */
+ case ITIMER_PROF: errno = ENOTSUP; ret = -1; break;
+ default: errno = EINVAL; ret = -1;
+ }
+
+ return ret;
+}
--
1.7.12
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH 2/2] Support for setitimer() on platforms lacking it
Signed-off-by: Joachim Schmitz --- Seems it needs my mkdir() "ignoretraile slash" patch first to be applied cleanly... It is independent of it otherwise. git-compat-util.h | 5 + 1 file changed, 5 insertions(+) diff --git a/git-compat-util.h b/git-compat-util.h index 34f040f..a047221 100644 --- a/git-compat-util.h +++ b/git-compat-util.h @@ -162,6 +162,11 @@ #define probe_utf8_pathname_composition(a,b) #endif +#ifdef NO_SETITIMER +#define setitimer(a,b,c) git_setitimer((a),(b),(c)) +extern int git_setitimer(int, const struct itimerval *, struct itimerval *); +#endif + #ifdef MKDIR_WO_TRAILING_SLASH #define mkdir(a,b) compat_mkdir_wo_trailing_slash((a),(b)) extern int compat_mkdir_wo_trailing_slash(const char*, mode_t); -- 1.7.12 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/6] Gettext poison rework
On Fri, Aug 24, 2012 at 3:51 PM, Ævar Arnfjörð Bjarmason wrote: >> [1] http://bugs.locamotion.org/show_bug.cgi?id=2450 >> [2] http://translate.sourceforge.net/wiki/toolkit/podebug > > The reason I didn't do something like this to begin with is that > gettext/glibc doesn't have support for fake locales, so you'd have to > appropriate a real one for tests. It's good to see you poking the > gettext mailing list about adding support far thot. I don't see glibc getting fake locale support any time soon even if it's more open than before. I can try to make gettext support pseudo translations, though not sure if I can make it. > But something like podebug gets around that quite nicely, so we can > still have the testing the poison stuff was intended for, without the > complexity of supporting it throughout all our i18n code. But yes, even if gettext does not have native support, relying on podebug should be ok. Whatever changes we need, other projects might too. It's better to do it outside of git. -- Duy1 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: in_merge_bases() is too expensive for recent "pu" update
On Thu, Aug 23, 2012 at 9:20 PM, Thomas Rast wrote:
> At the very least it should be possible to change in_merge_bases() to
> not do any of the post-filtering; perhaps like the patch below. It
> passes the test suite. The whole "merge bases of A and a list of Bs"
> thing is blowing my overheated mind, though, so I'm not able to convince
> myself that it is correct in all cases.
>
> diff --git i/commit.c w/commit.c
> index 65a8485..70427ab 100644
> --- i/commit.c
> +++ w/commit.c
> @@ -837,10 +837,13 @@ int in_merge_bases(struct commit *commit, struct commit
> **reference, int num)
> struct commit_list *bases, *b;
> int ret = 0;
>
> - if (num == 1)
> - bases = get_merge_bases(commit, *reference, 1);
> - else
> + if (num != 1)
> die("not yet");
> +
> + bases = merge_bases_many(commit, 1, reference);
> + clear_commit_marks(commit, all_flags);
> + clear_commit_marks(*reference, all_flags);
> +
> for (b = bases; b; b = b->next) {
> if (!hashcmp(commit->object.sha1, b->item->object.sha1)) {
> ret = 1;
Without looking into detail (as I'm not familiar with this code), this
patch does not help much. Without the patch:
$ time ./git merge-base a4f2db3 b95a282
ed36e5bd41f7192e42e9b4c573875a343a9daf48
real0m19.988s
user0m19.797s
sys 0m0.082s
With the patch:
$ time ./git merge-base a4f2db3 b95a282
ed36e5bd41f7192e42e9b4c573875a343a9daf48
real0m19.560s
user0m19.448s
sys 0m0.037s
--
Duy
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 00/17] Clean up how fetch_pack() handles the heads list
From: "Junio C Hamano" Sent: Friday, August 24, 2012 5:23 AM Jeff King writes: It may be (?) that it is a good time to think about a 'datedepth' capability to bypass the current counted-depth shallow fetch that can cause so much trouble. With a date limited depth the relevant tags could also be fetched. I don't have anything against such an idea, but I think it is orthogonal to the issue being discussed here. Correct. The biggest problem with the "shallow" hack is that the deepening fetch counts from the tip of the refs at the time of deepening when deepening the history (i.e. "clone --depth", followed by number of "fetch", followed by "fetch --depth"). If you start from a shallow clone of depth 1, repeated fetch to keep up while the history grew by 100, you would still have a connected history down to the initial cauterization point, and "fetch --depth=200" would give you a history that is deeper than your original clone by 100 commits. But if you start from the same shallow clone of depth 1, did not do anything while the history grew by 100, and then decide to fetch again with "fetch --depth=20", it does not grow. It just makes 20-commit deep history from the updated tip, and leave the commit from your original clone dangling. The problem with "depth" does not have anything to do with how it is specified at the UI level. That is, correct me if I'm wrong, the server currently lacks a capability to provide anything other than the counted depth shallow response (if available). Hence my comment about needing an additional server side capability, though that would need a well thought out set of use cases to make it useful. The end-user input is used to find out at what set of commits the history is cauterized, and once they are computed, the "shallow" logic solely works on "is the history before these cauterization points, or after, in topological terms." (and it has to be that way to make sure we get reproducible results). Even if a new way to specify these cauterization points in terms of date is introduced, it does not change anything and does not solve the fundamental problem the code has when deepening. fundamental problem = no server capability other than counted depth. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: a small git proposal
Thanks for the tip. It should give me a good starting point for what I'm about to do, since notes seem to be able to add comments for objects without changing the commit tree (which was one of the things I was aiming for and quite frankly, one of the parts that worried me on the implementation side). However, what I want to implement has a few differences: a) the flags I'm proposing form a limited set of strings, as I'm interested in finding which files have a particular flag (I did mention the idea to add comment as well when adding a certain flag, but that was something extra, since most of the searching/listing was around the set of flags of a certain file, not around the comment itself... I can cook up some more usage and output examples if anyone thinks it can clear things up). I realize this can be achieved by using a sound naming convention (and a simple grep for a particular prefix when listing all notes would handle the search by flag I mentioned) - unfortunately, some other differences don't seem to be covered as you'll see below b) I would like to have all subsequent versions of a file to inherit the flags/tags of the original file, until specified otherwise; in the original idea that a 'feature tag' (or 'flag' as I keep calling them lately - seems a better name that 'feature tag') remains on the file until someone decides that feature is no longer implemented in the file (for example, a file implements a certain technique since version 3 until version 15, when the implementation of a particular method changed entirely). Unfortunately, what seems to be a good choice to preserve a state of a file until not needed are branches, but then I would need to have the same version of the file on different branches (a file can have multiple flags after all, since multiple features are usually implemented in a file) Anyway, I just wanted to point out that the notes you suggested are not quite what I was looking for, but it should be a good implementation starting point, so again, lots of thanks. Catalin Pol On Thu, Aug 23, 2012 at 6:16 PM, Hilco Wijbenga wrote: > On 23 August 2012 08:10, Catalin Pol wrote: >> Hi everyone, >> >> This is my first email to this mailing list, so this may be somehow >> too straight forward... the idea is that I was thinking to develop a >> new feature in Git (although I'm kind of new to git myself). >> I wrote a small description of what I intend to do and I figured I >> could use some pointers (how I can improve it, any possible usage >> scenarios you can think for it and so on). Details are available at >> the gist link below or as attachment to this email (I zipped the text >> file since it was more it is larger than 10k and I didn't want it to >> get rejected by the email server... although it still might) >> >> gist link:https://gist.github.com/3437530 >> >> I made the gist public, so feel free to edit it directly... or, if you >> prefer, just email me with any comments. I'm opened to any suggestion, >> so feel free to send me any kind of comment (maybe I'm trying to >> implement something that is already in git for example, and since I'm >> a bit of a newbie in the git world, I didn't notice any way to obtain >> my desired workflow). >> >> Thanks in advance for any feedback, > > Have you looked at "git notes"? -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH v2] Support non-WIN32 system lacking poll() while keeping the WIN32 part intact
> From: Joachim Schmitz [mailto:j...@schmitz-digital.de] > Sent: Friday, August 24, 2012 11:45 AM > To: Junio C Hamano (gits...@pobox.com) > Cc: git@vger.kernel.org; Erik Faye-Lund (kusmab...@gmail.com) > Subject: [PATCH v2] Support non-WIN32 system lacking poll() while keeping the > WIN32 part intact > > > Signed-off-by: Joachim Schmitz > --- > This time I hopefully didn't screw up whitespace and line breaks. > > Makefile| 18 ++ > compat/win32/poll.c | 8 ++-- > 2 files changed, 20 insertions(+), 6 deletions(-) > > diff --git a/Makefile b/Makefile > index 66e8216..e150816 100644 > --- a/Makefile > +++ b/Makefile > @@ -152,6 +152,11 @@ all:: > # > # Define NO_MMAP if you want to avoid mmap. > # > +# Define NO_SYS_POLL_H if you don't have sys/poll.h. > +# > +# Define NO_POLL if you do not have or do not want to use poll. > +# This also implies NO_SYS_POLL_H. > +# > # Define NO_PTHREADS if you do not have or do not want to use Pthreads. > # > # Define NO_PREAD if you have a problem with pread() system call (e.g. > @@ -1216,7 +1221,7 @@ ifeq ($(uname_S),Windows) > NO_PREAD = YesPlease > NEEDS_CRYPTO_WITH_SSL = YesPlease > NO_LIBGEN_H = YesPlease > - NO_SYS_POLL_H = YesPlease > + NO_POLL = YesPlease > NO_SYMLINK_HEAD = YesPlease > NO_IPV6 = YesPlease > NO_UNIX_SOCKETS = YesPlease > @@ -1257,7 +1262,7 @@ ifeq ($(uname_S),Windows) > BASIC_CFLAGS = -nologo -I. -I../zlib -Icompat/vcbuild > -Icompat/vcbuild/include -DWIN32 -D_CONSOLE -DHAVE_STRING_H - > D_CRT_SECURE_NO_WARNINGS -D_CRT_NONSTDC_NO_DEPRECATE > COMPAT_OBJS = compat/msvc.o compat/winansi.o \ > compat/win32/pthread.o compat/win32/syslog.o \ > - compat/win32/poll.o compat/win32/dirent.o > + compat/win32/dirent.o > COMPAT_CFLAGS = -D__USE_MINGW_ACCESS -DNOGDI -DHAVE_STRING_H > -DHAVE_ALLOCA_H -Icompat -Icompat/regex - > Icompat/win32 -DSTRIP_EXTENSION=\".exe\" > BASIC_LDFLAGS = -IGNORE:4217 -IGNORE:4049 -NOLOGO -SUBSYSTEM:CONSOLE > -NODEFAULTLIB:MSVCRT.lib > EXTLIBS = user32.lib advapi32.lib shell32.lib wininet.lib ws2_32.lib > @@ -1312,7 +1317,7 @@ ifneq (,$(findstring MINGW,$(uname_S))) > NO_PREAD = YesPlease > NEEDS_CRYPTO_WITH_SSL = YesPlease > NO_LIBGEN_H = YesPlease > - NO_SYS_POLL_H = YesPlease > + NO_POLL = YesPlease > NO_SYMLINK_HEAD = YesPlease > NO_UNIX_SOCKETS = YesPlease > NO_SETENV = YesPlease > @@ -1347,7 +1352,7 @@ ifneq (,$(findstring MINGW,$(uname_S))) > COMPAT_CFLAGS += -DSTRIP_EXTENSION=\".exe\" > COMPAT_OBJS += compat/mingw.o compat/winansi.o \ > compat/win32/pthread.o compat/win32/syslog.o \ > - compat/win32/poll.o compat/win32/dirent.o > + compat/win32/dirent.o > EXTLIBS += -lws2_32 > PTHREAD_LIBS = > X = .exe > @@ -1601,6 +1606,11 @@ ifdef NO_GETTEXT > BASIC_CFLAGS += -DNO_GETTEXT > USE_GETTEXT_SCHEME ?= fallthrough > endif > +ifdef NO_POLL > + NO_SYS_POLL_H = YesPlease > + COMPAT_CFLAGS += -DNO_POLL -Icompat/win32 # so it finds poll.h > + COMPAT_OBJS += compat/win32/poll.o > +endif > ifdef NO_STRCASESTR > COMPAT_CFLAGS += -DNO_STRCASESTR > COMPAT_OBJS += compat/strcasestr.o > diff --git a/compat/win32/poll.c b/compat/win32/poll.c > index 403eaa7..49541f1 100644 > --- a/compat/win32/poll.c > +++ b/compat/win32/poll.c > @@ -24,7 +24,9 @@ > # pragma GCC diagnostic ignored "-Wtype-limits" > #endif > > -#include > +#if defined(WIN32) > +# include > +#endif > > #include > > @@ -48,7 +50,9 @@ > #else > # include > # include > -# include > +# ifndef NO_SYS_SELECT_H > +# include > +# endif > # include > #endif > > -- > 1.7.12 There is a downside with this: In order to make use of it, in Makefile it adds "-Icompat/win32" to COMPAR_CFLAGS. This results in compat/win32/dirent.h to be found, rather than /usr/include/dirent.h. This should be fine for WIN32, but for everybody else may not. For HP NonStop in particular it results in a warning: }; ^ "... /compat/win32/dirent.h", line 17: warning(133): expected an identifier And this is because there it uses an unnamed union, which is a GCC extension (just like unnamed struct), but not part of C89/C99/POSIX. One possible solution might be to move compat/win32/poll.[ch] to compat/. Another is to just ignore the warning, at least here it seems to work just fine? Or to avoid using an unnamed union. But the later 2 cases would still mean that we include the wrong dirent.h, so the 1st solution seems the cleanest. Any other idea? Let me know your thoughts... Bye, Jojo -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
libgit2 status
What is the status of libgit2 WRT the overall git project? I recall that there was some discussion of basing bits of git on libgit2 once it matures. I ask because I'm starting a project to improve the abysmal speed of git-subtree split. It's unbearably slow at the moment and as far as I can puzzle out it's due almost entirely to repeated subshell invocations to run git commands. I was planning on doing some experiments rewriting bits of git-subtree using libgit2 but I want to make sure that that isn't wasted work. It appears to be exactly what I need to code bits of git-subtree natively. Thoughts? -Dave -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] branch -v: align even when the first column is in UTF-8
Branch names are usually in ASCII so they are not the problem. The
problem most likely comes from "(no branch)" translation, which is in
UTF-8 and makes length calculation just wrong.
Signed-off-by: Nguyễn Thái Ngọc Duy
---
So far all git translations are utf-8 compatible. Branch names may
use filesystem encoding, but then packed-refs specifies no encoding.
Anyway branch names should be in utf-8.. at least internally, imo.
builtin/branch.c | 8 +---
1 tập tin đã bị thay đổi, 5 được thêm vào(+), 3 bị xóa(-)
diff --git a/builtin/branch.c b/builtin/branch.c
index 0e060f2..7c1ffa8 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -17,6 +17,7 @@
#include "revision.h"
#include "string-list.h"
#include "column.h"
+#include "utf8.h"
static const char * const builtin_branch_usage[] = {
"git branch [options] [-r | -a] [--merged | --no-merged]",
@@ -490,11 +491,12 @@ static void print_ref_item(struct ref_item *item, int
maxwidth, int verbose,
}
strbuf_addf(&name, "%s%s", prefix, item->name);
- if (verbose)
+ if (verbose) {
+ int utf8_compensation = strlen(name.buf) -
utf8_strwidth(name.buf);
strbuf_addf(&out, "%c %s%-*s%s", c, branch_get_color(color),
- maxwidth, name.buf,
+ maxwidth + utf8_compensation, name.buf,
branch_get_color(BRANCH_COLOR_RESET));
- else
+ } else
strbuf_addf(&out, "%c %s%s%s", c, branch_get_color(color),
name.buf, branch_get_color(BRANCH_COLOR_RESET));
--
1.7.12.rc2.18.g61b472e
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: misleading diff-hunk header
On Tue, Aug 21, 2012 at 10:52:03AM -0700, Junio C Hamano wrote:
> >>> diff.{type}.xfuncname seems to start searching backwards in
> >>> from the beginning of the hunk, not the first differing line.
> >> [...]
> >>> @@ -4,4 +4,5 @@ int call_me(int maybe)
> >>>
> >>>int main()
> >>>{
> >>> + return 0;
> >>>}
> >>>
> >>> misleadingly suggesting that the change occurred in the call_me()
> >>> function, rather than in main()
> >>
> >> I think that's intentional, and matches what 'diff -p' does. It gives
> >> you the context before the hunk. After all, if a new function starts in
> >> the leading context lines, you can see that in the usual diff data.
>
> Correct. It is about "give the user _more_ hint/clue on the context
> of the hunk", in addition to what the user can see in the
> pre-context of the hunk, so it is pointless to hoist "int main()"
> there.
I don't think it is pointless. If you are skimming a diff, then the hunk
headers stand out to easily show which functions were touched. Of
course, as you mentioned later in your email, it is not an exhaustive
list, and I think for Tim's use case, he needs to actually read and
parse the whole patch.
But mentioning call_me here _is_ pointless, because it is not relevant
context at all (it was not modified; it just happens to be located near
the code in question). So I would argue that showing main() is more
useful to a reader.
It gets even more obvious as you increase the context. Imagine I have
code like this:
int foo(void)
{
return 1;
}
int bar(void)
{
return 2;
}
int baz(void)
{
return 3;
}
and I modify "baz" to return "4" instead. With the regular diff
settings, the hunk header would claim that "bar()" is the context in the
hunk header. But if I ask for -U7, then "foo()" is mentioned in the hunk
header. To me, that doesn't make sense; the modification is exactly the
same, so why would the hunk header differ?
I suppose one could argue that the hunk header is not showing the
context of the change, but rather the context of the surrounding context
lines. But that doesn't seem useful to me.
We discussed this a while ago and you did a "how about this" patch:
http://article.gmane.org/gmane.comp.version-control.git/181385
Gmane seems to be acting up this morning, so here is the patch (and your
comment) for reference:
> Would this be sufficient? Instead of looking for the first line that
> matches the "beginning" pattern going backwards starting from one line
> before the displayed context, we start our examination at the first line
> shown in the context.
>
> xdiff/xemit.c |2 +-
> 1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/xdiff/xemit.c b/xdiff/xemit.c
> index 277e2ee..5f9c0e0 100644
> --- a/xdiff/xemit.c
> +++ b/xdiff/xemit.c
> @@ -131,7 +131,7 @@ int xdl_emit_diff(xdfenv_t *xe, xdchange_t *xscr,
> xdemitcb_t *ecb,
>
> if (xecfg->flags & XDL_EMIT_FUNCNAMES) {
> long l;
> - for (l = s1 - 1; l >= 0 && l > funclineprev; l--) {
> + for (l = s1; l >= 0 && l > funclineprev; l--) {
> const char *rec;
> long reclen = xdl_get_rec(&xe->xdf1, l, &rec);
> long newfunclen = ff(rec, reclen, funcbuf,
In the case we were discussing then, the modified function started on
the first line of context. But as Tim's example shows, it doesn't
necessarily have to. I think it would make more sense to start counting
from the first modified line.
-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: misleading diff-hunk header
On 08/24/12 09:29, Jeff King wrote:
> On Tue, Aug 21, 2012 at 10:52:03AM -0700, Junio C Hamano wrote:
>
> diff.{type}.xfuncname seems to start searching backwards in
> from the beginning of the hunk, not the first differing line.
[...]
> @@ -4,4 +4,5 @@ int call_me(int maybe)
>
>int main()
>{
> + return 0;
>}
>
> misleadingly suggesting that the change occurred in the call_me()
> function, rather than in main()
I think that's intentional, and matches what 'diff -p' does.
...
>> xdiff/xemit.c |2 +-
>> 1 files changed, 1 insertions(+), 1 deletions(-)
>>
>> diff --git a/xdiff/xemit.c b/xdiff/xemit.c
>> index 277e2ee..5f9c0e0 100644
>> --- a/xdiff/xemit.c
>> +++ b/xdiff/xemit.c
>> @@ -131,7 +131,7 @@ int xdl_emit_diff(xdfenv_t *xe, xdchange_t *xscr,
>> xdemitcb_t *ecb,
>>
>> if (xecfg->flags & XDL_EMIT_FUNCNAMES) {
>> long l;
>> -for (l = s1 - 1; l >= 0 && l > funclineprev; l--) {
>> +for (l = s1; l >= 0 && l > funclineprev; l--) {
>> const char *rec;
>> long reclen = xdl_get_rec(&xe->xdf1, l, &rec);
>> long newfunclen = ff(rec, reclen, funcbuf,
>
> In the case we were discussing then, the modified function started on
> the first line of context. But as Tim's example shows, it doesn't
> necessarily have to. I think it would make more sense to start counting
> from the first modified line.
Junio mentions that it matches the "diff -p" output, though I'd
consider that a bug in diff as well, since the diff(1) man/info
pages state "-p Show which C function each change is in." In the
above (both with "diff -p" and with git), the change was clearly in
main() but it's not showing main(). Documented behavior and
implemented behavior conflict.
Starting at the first differing line rather than the first line of
context in the hunk would ameliorate this. It doesn't address what
happens if multiple functions were changed in the same hunk, but at
least it becomes correct for the first one. More complex code might
be doable to split hunks if an xfuncname match occurs between two
disjoint changes in the same hunk. But for my purposes here, the
above should suffice.
-tkc
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: in_merge_bases() is too expensive for recent "pu" update
On Fri, Aug 24, 2012 at 11:43:40AM +0200, Thomas Rast wrote: > > Start from A and B. Follow from B to find 'x' and paint it in blue, > > follow from A to find 'y' and paint it in amber. Follow from 'x' to > > '1', paint it in blue. Follow from 'y' to '1', paint it in amber > > but notice that it already is painted in blue. > [...] > > o---o > >/ \ > > / y---A > > / / > > ---2---z---1---x---B > > \ / > > o---o > [...] > > So we need to notice that '1' and '2' have ancestry relation in > > order to reject '2' as "common but not merge-base". One way of > > doing so is not to stop at '1' and keep digging (and eventually we > > find that '2' is what we could reach from '1' that already is a > > merge base), but then we will be susceptible to the same kind of > > clock skew issue as the revision traverser. > > I think that is *the* way to do it. > > And the way to fix the clock skew issue (also in the revision traversal) > is Peff's generation number cache. Just keep propagating marks, digging > in generation order, until the generations prove that nothing new can > happen. I thought you were just interested in speeding up is_descendent_of. You should be able to do that without a generation number. Just start from A and B as above, do the two-color painting, and do not add the parents of any two-color commits (because you know they are ancestors of both, and therefore you cannot find either by looking backwards). If you paint "B" amber, then it is a descendent of "A" (and vice versa if you paint "A" blue). Clock skew may make you take longer (because you may go depth-first down an uninteresting chain of commits), but it should never give you the wrong answer. It's not as fast as using a generation-based cutoff (because you have to keep walking to the actual shared ancestor), but in practice it's usually fine. The reason I did not do that for "tag --contains" is that I wanted to do a simultaneous walk for all tags. That would require N color bits for N tags, and we have a limited space in each commit object. I didn't time using a separate hash table to store those bits outside of the commit objects. That would have a higher constant, but should still yield good big-O complexity. > [Side note, in reply to an earlier comment in the rev-list thread: I > agree with Peff's reasoning that a cache is better than a commit > header.] I still think it's a good idea to keep it out of the commit header. But I think I might lean towards tying it to the pack index (i.e., generating it when we generate the index, and depending on the sha1 table in the index). That would be smaller, and gets rid of any complexity or performance issues with making incremental updates to the cache. Loose objects wouldn't have a cached version number, but that's OK; in practice you can quickly walk backwards to a commit that _is_ cached. -Peff -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: in_merge_bases() is too expensive for recent "pu" update
Thomas Rast writes: > Well, yeah, you snipped this part from my original post :-) > > } Even if this turns out to be flawed, we should also identify uses of > } in_merge_bases() where the real question was is_descendant_of() [I > } somewhat suspect that's all of them], and then replace is_descendant_of > } with a much cheaper lookup. This can be as simple as propagating a mark > } from the candidate until it either goes beyond all possible ancestors, > } or hits one of them. Yeah, I agree with the above, and the "cheaper lookup" would probably be merge-bases-many() without postprocess. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: in_merge_bases() is too expensive for recent "pu" update
Thomas Rast writes: > Junio C Hamano writes: > ... >> Start from A and B. Follow from B to find 'x' and paint it in blue, >> follow from A to find 'y' and paint it in amber. Follow from 'x' to >> '1', paint it in blue. Follow from 'y' to '1', paint it in amber >> but notice that it already is painted in blue. > [...] >> o---o >>/ \ >> / y---A >> / / >> ---2---z---1---x---B >> \ / >> o---o > [...] >> So we need to notice that '1' and '2' have ancestry relation in >> order to reject '2' as "common but not merge-base". One way of >> doing so is not to stop at '1' and keep digging (and eventually we >> find that '2' is what we could reach from '1' that already is a >> merge base), but then we will be susceptible to the same kind of >> clock skew issue as the revision traverser. > > I think that is *the* way to do it. But we do not live in *that* world. At least not yet. > I conjecture that every history walking problem can be solved in time > linear in the number of commits once we properly use the generation > numbers ;-) I would conjecture that too, but we do not live in that world yet. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/6] Gettext poison rework
Nguyễn Thái Ngọc Duy writes: > Still WIP but I'm getting closer. I dropped test-poisongen and started > to use podebug [2] instead. Less code in git. podebug does not preserve > shell variables yet. I'll follow that up at upstream [1]. Thanks; this looks promising. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] Support non-WIN32 system lacking poll() while keeping the WIN32 part intact
"Joachim Schmitz" writes: > There is a downside with this: In order to make use of it, in Makefile it > adds "-Icompat/win32" to COMPAR_CFLAGS. This results in > compat/win32/dirent.h to be found, rather than /usr/include/dirent.h. > This should be fine for WIN32, but for everybody else may not. > For HP NonStop in particular it results in a warning: > > }; > ^ > "... /compat/win32/dirent.h", line 17: warning(133): expected an identifier > > And this is because there it uses an unnamed union, which is a GCC extension > (just like unnamed struct), but not part of C89/C99/POSIX. > > One possible solution might be to move compat/win32/poll.[ch] to compat/. I think that is the most sensible way to go, because poll.[ch] (1) has proven itself to be useful outside the context of win32, and (2) the code is coming from gnulib anyway. I thought I already suggested going that route in my previous review, but perhaps I forgot to do so. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: in_merge_bases() is too expensive for recent "pu" update
Jeff King writes: > I thought you were just interested in speeding up is_descendent_of. You > should be able to do that without a generation number. Just start from A > and B as above, do the two-color painting, and do not add the parents of > any two-color commits (because you know they are ancestors of both, and > therefore you cannot find either by looking backwards). If you paint "B" > amber, then it is a descendent of "A" (and vice versa if you paint "A" > blue). Yes, that is what merge_bases_many() (the one without the post processing to cull candidates) is primarily doing. The function also does the "STALE" bit processing that aims to reduce the number of candidates in "no clock skew" cases with minimum effort, to mimimize the cost of get_merge_bases_many() in the post-processing phase, but removing the "STALE" bit processing shouldn't affect the correctness of get_merge_bases_many() and would help performance of merge_bases_many() proper, which is useful for is_descendant_of(). > The reason I did not do that for "tag --contains" is that I wanted to > do a simultaneous walk for all tags. That would require N color bits for > N tags, and we have a limited space in each commit object. I didn't time > using a separate hash table to store those bits outside of the commit > objects. That would have a higher constant, but should still yield good > big-O complexity. It may not be a bad idea to at least try and see the performance implications of moving many users of object->flags to a new implementation of per-purpose flag bits based on the decoration infrastracture. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] Prefer sysconf(_SC_OPEN_MAX) over getrlimit(RLIMIT_NOFILE,...)
"Joachim Schmitz" writes:
> Signed-off-by: Joachim Schmitz
> ---
> As discussed now as a small helper function rather than #ifdef/#endif in the
> primary flow of the code.
> And hopefully without having screwed up whitespace and line breaks
The formatting looks fine.
Perhaps I am being overly paranoid, but I would prefer not to change
things for people who have been using getrlimit(). For them, if
they also have sysconf(_SC_OPEN_MAX), your code _ought to_ work, but
if it does not work for whatever reason (perhaps some platforms
claim to have both, but getrlimit() works and sysconf(_SC_OPEN_MAX)
is broken), it will given them an unnecessary regression.
So how about doing it this way instead?
-- >8 --
Subject: sha1_file.c: introduce get_max_fd_limit() helper
Not all platforms have getrlimit(), but there are other ways to see
the maximum number of files that a process can have open. If
getrlimit() is unavailable, fall back to sysconf(_SC_OPEN_MAX) if
available, and use OPEN_MAX from .
Signed-off-by: Joachim Schmitz
Signed-off-by: Junio C Hamano
---
sha1_file.c | 26 +++---
1 file changed, 19 insertions(+), 7 deletions(-)
diff --git c/sha1_file.c w/sha1_file.c
index af5cfbd..9152974 100644
--- c/sha1_file.c
+++ w/sha1_file.c
@@ -731,6 +731,24 @@ void free_pack_by_name(const char *pack_name)
}
}
+static unsigned int get_max_fd_limit(void)
+{
+#ifdef RLIMIT_NOFILE
+ struct rlimit lim;
+
+ if (getrlimit(RLIMIT_NOFILE, &lim))
+ die_errno("cannot get RLIMIT_NOFILE");
+
+ return lim.rlim_cur;
+#elif defined(_SC_OPEN_MAX)
+ return sysconf(_SC_OPEN_MAX);
+#elif defined(OPEN_MAX)
+ return OPEN_MAX;
+#else
+ return 1; /* see the caller ;-) */
+#endif
+}
+
/*
* Do not call this directly as this leaks p->pack_fd on error return;
* call open_packed_git() instead.
@@ -747,13 +765,7 @@ static int open_packed_git_1(struct packed_git *p)
return error("packfile %s index unavailable", p->pack_name);
if (!pack_max_fds) {
- struct rlimit lim;
- unsigned int max_fds;
-
- if (getrlimit(RLIMIT_NOFILE, &lim))
- die_errno("cannot get RLIMIT_NOFILE");
-
- max_fds = lim.rlim_cur;
+ unsigned int max_fds = get_max_fd_limit();
/* Save 3 for stdin/stdout/stderr, 22 for work */
if (25 < max_fds)
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: misleading diff-hunk header
On Fri, Aug 24, 2012 at 10:29:09AM -0400, Jeff King wrote:
> > Would this be sufficient? Instead of looking for the first line that
> > matches the "beginning" pattern going backwards starting from one line
> > before the displayed context, we start our examination at the first line
> > shown in the context.
> >
> > xdiff/xemit.c |2 +-
> > 1 files changed, 1 insertions(+), 1 deletions(-)
> >
> > diff --git a/xdiff/xemit.c b/xdiff/xemit.c
> > index 277e2ee..5f9c0e0 100644
> > --- a/xdiff/xemit.c
> > +++ b/xdiff/xemit.c
> > @@ -131,7 +131,7 @@ int xdl_emit_diff(xdfenv_t *xe, xdchange_t *xscr,
> > xdemitcb_t *ecb,
> >
> > if (xecfg->flags & XDL_EMIT_FUNCNAMES) {
> > long l;
> > - for (l = s1 - 1; l >= 0 && l > funclineprev; l--) {
> > + for (l = s1; l >= 0 && l > funclineprev; l--) {
> > const char *rec;
> > long reclen = xdl_get_rec(&xe->xdf1, l, &rec);
> > long newfunclen = ff(rec, reclen, funcbuf,
>
> In the case we were discussing then, the modified function started on
> the first line of context. But as Tim's example shows, it doesn't
> necessarily have to. I think it would make more sense to start counting
> from the first modified line.
That patch would look something like this:
diff --git a/xdiff/xemit.c b/xdiff/xemit.c
index d11dbf9..441ecf5 100644
--- a/xdiff/xemit.c
+++ b/xdiff/xemit.c
@@ -194,7 +194,7 @@ int xdl_emit_diff(xdfenv_t *xe, xdchange_t *xscr,
xdemitcb_t *ecb,
if (xecfg->flags & XDL_EMIT_FUNCNAMES) {
get_func_line(xe, xecfg, &func_line,
- s1 - 1, funclineprev);
+ xche->i1 - 1, funclineprev);
funclineprev = s1 - 1;
}
if (xdl_emit_hunk_hdr(s1 + 1, e1 - s1, s2 + 1, e2 - s2,
Note that this breaks a ton of tests. Some of them are just noise (e.g.,
t4042 changes line 2, so line 1 is the top of the context; before, we
would show no hunk header, since we were at the top of the file, but now
we will show line 1). Some of them are improved in the way that this
patch intends (e.g., t4051).
But some I'm not sure of. For instance, the failure in t4018.38 is odd.
I think it's because the pattern it is looking for is a somewhat odd toy
example (it's looking for a line with "s" in it, so naturally when we
shift the start-point of our search, we are likely to find some other
false positive). But it raises an interesting point: what if the pattern
is just looking for lines in a list, and not an enclosing function?
For example, imagine you have a file a list of items, one per line.
With the old code, you'd get:
diff --git a/old b/new
index f384549..1066a25 100644
--- a/old
+++ b/new
@@ -2,3 +2,3 @@ one
two
-three
+three -- modified
four
So the hunk header is showing you something useful; the element just
above your context. But with my patch, you'd see:
diff --git a/old b/new
index f384549..1066a25 100644
--- a/old
+++ b/new
@@ -2,3 +2,3 @@ two
two
-three
+three -- modified
four
I.e., it shows the element just before the change, which is already in
the context anyway. So it's actually less useful. Although note that the
current behavior is not all that useful, either; it is not really giving
you any information about the change, but rather just showing one extra
line of context.
So I would say that which you would prefer might depend on exactly what
you are diffing. But I would also argue that in any case where the new
code produces a worse result, the hunk header was not all that useful to
begin with.
-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] branch -v: align even when the first column is in UTF-8
Nguyễn Thái Ngọc Duy writes:
> Branch names are usually in ASCII so they are not the problem. The
> problem most likely comes from "(no branch)" translation, which is in
> UTF-8 and makes length calculation just wrong.
>
> Signed-off-by: Nguyễn Thái Ngọc Duy
> ---
> So far all git translations are utf-8 compatible. Branch names may
> use filesystem encoding, but then packed-refs specifies no encoding.
> Anyway branch names should be in utf-8.. at least internally, imo.
I agree with all of the above, but shouldn't you be computing the
"maxwidth" based on the strwidth in the first place? The use of
maxwidth in strbuf_addf() here clearly wants "we know N columns is
sufficient to show all output items, so pad the string to N columns"
here. Looking for assignment "item.len = xxx" in the same file
shows these are computed as byte length, so you are offsetting off
of an incorrectly computed value.
Giving fewer padding bytes when showing a string that will occupy
fewer columns than it has bytes is independently necessary, once we
have the correct maxwidth that is computed in terms of the strwidth,
so this patch is not wrong per-se, but it is incomplete without a
correct maxwidth, no?
> builtin/branch.c | 8 +---
> 1 tập tin đã bị thay đổi, 5 được thêm vào(+), 3 bị xóa(-)
>
> diff --git a/builtin/branch.c b/builtin/branch.c
> index 0e060f2..7c1ffa8 100644
> --- a/builtin/branch.c
> +++ b/builtin/branch.c
> @@ -17,6 +17,7 @@
> #include "revision.h"
> #include "string-list.h"
> #include "column.h"
> +#include "utf8.h"
>
> static const char * const builtin_branch_usage[] = {
> "git branch [options] [-r | -a] [--merged | --no-merged]",
> @@ -490,11 +491,12 @@ static void print_ref_item(struct ref_item *item, int
> maxwidth, int verbose,
> }
>
> strbuf_addf(&name, "%s%s", prefix, item->name);
> - if (verbose)
> + if (verbose) {
> + int utf8_compensation = strlen(name.buf) -
> utf8_strwidth(name.buf);
> strbuf_addf(&out, "%c %s%-*s%s", c, branch_get_color(color),
> - maxwidth, name.buf,
> + maxwidth + utf8_compensation, name.buf,
> branch_get_color(BRANCH_COLOR_RESET));
> - else
> + } else
> strbuf_addf(&out, "%c %s%s%s", c, branch_get_color(color),
> name.buf, branch_get_color(BRANCH_COLOR_RESET));
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] Ignore trailing slash in mkdir() on platforms that can't deal with this
As the compat/mkdir.c file includes git-compat-util.h and expects the declaration of the new function to be found in it, it does not make any sense to have this as two patches. I'll squash them into one for now, but it would have been even more complete to have an update to the Makefile to actually compile these files in the same change. These things go together. The other itimer set shares the same issue. I've queued mkdir and itimer series as one patch each; please check the result in 'pu' after I push it out. Thanks. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] contrib: GnomeKeyring support + generic helper implementation
"Philipp A. Hartmann" writes: > All, > > the following patch series proposes enhancements to the credential helper > implementations in the contrib section. The detailed development history > can be found at GitHub [1]. > > The first patch adds a GnomeKeyring credential backend. The GnomeKeyring > specific parts are based on the work by John Szakmeister, who wrote the > helper originally for the initial, unpublished version of the credential > helper protocol. > > The second patch adds a generic implementation of a credential helper > based on a simplified credential API and common helper functions. Helpers > based on this implementation do not need to worry about the credential > protocol and only need to implement callback functions for the different > credential operations. > > The third and fourth patches port the existing helpers to this generic > implementation. > > Looking forward to your thoughts. It struck me somewhat odd to see a new one added as the first step in the series, and then "the generic", the third patch only to lose code from the first one, and then use the same code reduction of existing one with the last one in the series. A natural progression would have been the introduction of generic infrastructure (including the tiny bit this series had to add as part of 4/4) as the first patch, update existing OSX one to it as the second, and then build a new Gnome one on the infrastructure as the third and final patch; that way we have to review less code, too ;-). I gave it a cursory look; other than getting distracted by inconsistent coding styles here and there, the patches seemed reasonably clean and well thought out. Thanks. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH 1/2] Ignore trailing slash in mkdir() on platforms that can't deal with this
> From: Junio C Hamano [mailto:gits...@pobox.com] > Sent: Friday, August 24, 2012 7:44 PM > To: Joachim Schmitz > Cc: git@vger.kernel.org > Subject: Re: [PATCH 1/2] Ignore trailing slash in mkdir() on platforms that > can't deal with this > > As the compat/mkdir.c file includes git-compat-util.h and expects > the declaration of the new function to be found in it, it does not > make any sense to have this as two patches. I'll squash them into > one for now, but it would have been even more complete to have an > update to the Makefile to actually compile these files in the same > change. These things go together. A changed Makefile is in the pipeline, but right now it is pretty NonStop specific, while I tried to keep compat/itimer.cm compat/mkdir.c and git-compat-util.h as NonStop independent as I possible could. That Makefile also depends on the outcome of the discussion about my NonStop specific changed in git-compat.util.h > The other itimer set shares the same issue. I've queued mkdir and > itimer series as one patch each; please check the result in 'pu' > after I push it out. Thanks. What does 'pu' stand for? Bye, Jojo -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH v2] Prefer sysconf(_SC_OPEN_MAX) over getrlimit(RLIMIT_NOFILE,...)
> From: Junio C Hamano [mailto:gits...@pobox.com]
> Sent: Friday, August 24, 2012 6:44 PM
> To: Joachim Schmitz
> Cc: git@vger.kernel.org
> Subject: Re: [PATCH v2] Prefer sysconf(_SC_OPEN_MAX) over
> getrlimit(RLIMIT_NOFILE,...)
>
> "Joachim Schmitz" writes:
>
> > Signed-off-by: Joachim Schmitz
> > ---
> > As discussed now as a small helper function rather than #ifdef/#endif in
> > the primary flow of the code.
> > And hopefully without having screwed up whitespace and line breaks
>
> The formatting looks fine.
>
> Perhaps I am being overly paranoid, but I would prefer not to change
> things for people who have been using getrlimit(). For them, if
> they also have sysconf(_SC_OPEN_MAX), your code _ought to_ work, but
> if it does not work for whatever reason (perhaps some platforms
> claim to have both, but getrlimit() works and sysconf(_SC_OPEN_MAX)
> is broken), it will given them an unnecessary regression.
Sounds reasonable, so reasonable that I wonder why I didn't have that idea ;-)
> So how about doing it this way instead?
>
> -- >8 --
> Subject: sha1_file.c: introduce get_max_fd_limit() helper
>
> Not all platforms have getrlimit(), but there are other ways to see
> the maximum number of files that a process can have open. If
> getrlimit() is unavailable, fall back to sysconf(_SC_OPEN_MAX) if
> available, and use OPEN_MAX from .
>
> Signed-off-by: Joachim Schmitz
> Signed-off-by: Junio C Hamano
> ---
> sha1_file.c | 26 +++---
> 1 file changed, 19 insertions(+), 7 deletions(-)
>
> diff --git c/sha1_file.c w/sha1_file.c
> index af5cfbd..9152974 100644
> --- c/sha1_file.c
> +++ w/sha1_file.c
> @@ -731,6 +731,24 @@ void free_pack_by_name(const char *pack_name)
> }
> }
>
> +static unsigned int get_max_fd_limit(void)
> +{
> +#ifdef RLIMIT_NOFILE
> + struct rlimit lim;
> +
> + if (getrlimit(RLIMIT_NOFILE, &lim))
> + die_errno("cannot get RLIMIT_NOFILE");
> +
> + return lim.rlim_cur;
> +#elif defined(_SC_OPEN_MAX)
> + return sysconf(_SC_OPEN_MAX);
> +#elif defined(OPEN_MAX)
> + return OPEN_MAX;
> +#else
> + return 1; /* see the caller ;-) */
> +#endif
> +}
> +
> /*
> * Do not call this directly as this leaks p->pack_fd on error return;
> * call open_packed_git() instead.
> @@ -747,13 +765,7 @@ static int open_packed_git_1(struct packed_git *p)
> return error("packfile %s index unavailable", p->pack_name);
>
> if (!pack_max_fds) {
> - struct rlimit lim;
> - unsigned int max_fds;
> -
> - if (getrlimit(RLIMIT_NOFILE, &lim))
> - die_errno("cannot get RLIMIT_NOFILE");
> -
> - max_fds = lim.rlim_cur;
> + unsigned int max_fds = get_max_fd_limit();
>
> /* Save 3 for stdin/stdout/stderr, 22 for work */
> if (25 < max_fds)
Looks good to me.
Stupid newbie question: how would I revert my commit to my clone, to then add
(and test) this one?
Bye, Jojo
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH v2] Support non-WIN32 system lacking poll() while keeping the WIN32 part intact
> From: Junio C Hamano [mailto:gits...@pobox.com] > Sent: Friday, August 24, 2012 6:07 PM > To: Joachim Schmitz > Cc: git@vger.kernel.org; Erik Faye-Lund > Subject: Re: [PATCH v2] Support non-WIN32 system lacking poll() while keeping > the WIN32 part intact > > "Joachim Schmitz" writes: > > > There is a downside with this: In order to make use of it, in Makefile it > > adds "-Icompat/win32" to COMPAR_CFLAGS. This results in > > compat/win32/dirent.h to be found, rather than /usr/include/dirent.h. > > This should be fine for WIN32, but for everybody else may not. > > For HP NonStop in particular it results in a warning: > > > > }; > > ^ > > "... /compat/win32/dirent.h", line 17: warning(133): expected an identifier > > > > And this is because there it uses an unnamed union, which is a GCC extension > > (just like unnamed struct), but not part of C89/C99/POSIX. > > > > One possible solution might be to move compat/win32/poll.[ch] to compat/. > > I think that is the most sensible way to go, because poll.[ch] > > (1) has proven itself to be useful outside the context of win32, and > (2) the code is coming from gnulib anyway. > > I thought I already suggested going that route in my previous > review, but perhaps I forgot to do so. No, I believe you did, but I had forgotten about it. Could/should that be a 2nd patch? Or a v3 of this one? Different, but related question: would poll.[ch] be allowed to #include "git-compat-util.h"? Bye, Jojo -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[RFC] Support for HP NonStop
Hi folks On top of the patches Ive submitted so far, which were needed for HP NonStop, but possibly useful for other platforms too, here is one that is at least in parts NonStop specific diff --git a/git-compat-util.h b/git-compat-util.h index a047221..d6a142a 100644 --- a/git-compat-util.h +++ b/git-compat-util.h @@ -74,7 +74,8 @@ # define _XOPEN_SOURCE 500 # endif #elif !defined(__APPLE__) && !defined(__FreeBSD__) && !defined(__USLC__) && \ - !defined(_M_UNIX) && !defined(__sgi) && !defined(__DragonFly__) + !defined(_M_UNIX) && !defined(__sgi) && !defined(__DragonFly__) && \ + !defined(__TANDEM) #define _XOPEN_SOURCE 600 /* glibc2 and AIX 5.3L need 500, OpenBSD needs 600 fo #define _XOPEN_SOURCE_EXTENDED 1 /* AIX 5.3L needs this */ #endif +#ifdef __TANDEM /* or HAVE_STRINGS_H ? */ +#include /* for strcasecmp() */ +#endif #include #include #include @@ -141,6 +145,10 @@ #else #include #endif +#ifdef __TANDEM /* or NO_INTPTR_T resp. NO_UINTPTR_T? */ +typedef int intptr_t; +typedef unsigned int uintptr_t; +#endif #if defined(__CYGWIN__) #undef _XOPEN_SOURCE #include The 1st hunk avoids a is already defined with a different value warning, and I believe this is the only and right way to fix this, but on the 2nd and 3rd hunk Id need advice on how to properly add those. The #ifdef __TANDEM #endif works fine for me, but doesnt seem 100% clean to me. In the comment I mention alternatives. strcasecamp() is declared in as per C99/POSIX, and in C99 mode a prototype has to be seen by the compiler. intptr_t and uintprt_t seem to be optional in C99 and are not provided for NonStop What do you think? Bye, Jojo -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] Support non-WIN32 system lacking poll() while keeping the WIN32 part intact
"Joachim Schmitz" writes: > Different, but related question: would poll.[ch] be allowed to #include > "git-compat-util.h"? Seeing other existing generic wrappers directly under compat/, e.g. fopen.c, mkdtemp.c, doing so, I would say why not. Windows folks (I see Erik is already CC'ed, which is good ;-), please work with Joachim to make sure such a move won't break your builds. I believe that it should just be the matter of updating a couple of paths in the top-level Makefile. Thanks. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC] Support for HP NonStop
"Joachim Schmitz" writes: > Hi folks > > On top of the patches I’ve submitted so far, which were needed for HP > NonStop, > but possibly useful for other platforms too, here is one that is at least in > parts NonStop specific > > diff --git a/git-compat-util.h b/git-compat-util.h > index a047221..d6a142a 100644 > --- a/git-compat-util.h > +++ b/git-compat-util.h > @@ -74,7 +74,8 @@ > # define _XOPEN_SOURCE 500 > # endif > #elif !defined(__APPLE__) && !defined(__FreeBSD__) && !defined(__USLC__) && \ > - !defined(_M_UNIX) && !defined(__sgi) && !defined(__DragonFly__) > + !defined(_M_UNIX) && !defined(__sgi) && !defined(__DragonFly__) && \ > + !defined(__TANDEM) > #define _XOPEN_SOURCE 600 /* glibc2 and AIX 5.3L need 500, OpenBSD needs 600 > fo > #define _XOPEN_SOURCE_EXTENDED 1 /* AIX 5.3L needs this */ > #endif > +#ifdef __TANDEM /* or HAVE_STRINGS_H ? */ > +#include /* for strcasecmp() */ > +#endif > #include > #include > #include Yeah, it appears that glibc headers have strcasecmp() and friends in the and that was why majority of us were fine without including . A cursory look of /usr/include/strings.h on a GNU system suggests that it is safe to include after we incude on that platform. I think it is OK to leave it "__TANDEM /* or HAVE_STRINGS_H? */" for now and let the next person who wants to port us to a platform that needs this inclusion turn it to HAVE_STRINGS_H. Alternatively, we bite the bullet now and include on any platform that has the header file and see if anybody complains (that reminds me; I at least should get one flavor of BSD build environment for this kind of thing myself). > @@ -141,6 +145,10 @@ > #else > #include > #endif > +#ifdef __TANDEM /* or NO_INTPTR_T resp. NO_UINTPTR_T? */ > +typedef int intptr_t; > +typedef unsigned int uintptr_t; > +#endif A bit wider context for this hunk is #ifndef NO_INTTYPES_H #include #else #include #endif So we have been assuming that has intptr_t but __TANDEM apparently doesn't. POSIX requires intptr_t and uintptr_t to be declared for systems conforming to XSI, but otherwise these are optional (in other words, some XSI non-conforming platforms may have them in ), so it would not help to check _XOPEN_UNIX to see if the system is XSI X-<. We would need NO_INTPTR_T as you hinted above, perhaps like this. #ifndef NO_INTTYPES_H #include #else #include #endif #ifdef NO_INTPTR_T typedef int intptr_t; typedef unsigned int uintptr_t; #endif By the way, is "int" wide enough, or should they be "long"? -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
git no longer prompting for password
Hi List, A recent update to git 1.7.12 from 1.7.3.5 seems to have changed something - trying to push to a smart http backend no longer prompts for a password and hence fails the server auth. The server is currently running git 1.7.9 behind apache 2.4.3 with an almost verbatim copy of the apache config from the git-http-backend manpage. Backtracking through the versions I've skipped and this doesn't seem to be a new problem, client side up to 1.7.7.7 works, 1.7.8 onwards don't. Server side version doesn't seem to make a difference. user@fubar01:~/test# git --version git version 1.7.7.7 user@fubar01:~/test# git push http://ipaton@10.0.0.1/git/test.git master Password: type the password in and the push is successful user@fubar01:~/test# git --version git version 1.7.8 user@fubar01:~/test# git push http://ipaton@10.0.0.1/git/test.git master --verbose Pushing to http://ipaton@10.0.0.1/git/test.git Counting objects: 6, done. Delta compression using up to 8 threads. Compressing objects: 100% (3/3), done. Writing objects: 100% (5/5), 491 bytes, done. Total 5 (delta 0), reused 0 (delta 0) error: RPC failed; result=22, HTTP code = 401 fatal: The remote end hung up unexpectedly fatal: The remote end hung up unexpectedly Watching the connection with wireshark shows that it does appear to try to authenticate with the correct username, but without a password. Not surprising since it doesn't ask for one.. googling for git and password just seems to give results where people want it to stop asking for a password, which is the oppsite of what I want! Looking at changelogs for 1.7.8 and I'm not really seeing anything that says I need to do something different. Any help or pointers appreciated. Thanks, Iain -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [RFC] Support for HP NonStop
> From: Junio C Hamano [mailto:gits...@pobox.com] > Sent: Friday, August 24, 2012 10:13 PM > To: Joachim Schmitz > Cc: git@vger.kernel.org > Subject: Re: [RFC] Support for HP NonStop > > "Joachim Schmitz" writes: > > > Hi folks > > > > On top of the patches I’ve submitted so far, which were needed for HP > > NonStop, > > but possibly useful for other platforms too, here is one that is at least > > in parts NonStop specific > > > > diff --git a/git-compat-util.h b/git-compat-util.h > > index a047221..d6a142a 100644 > > --- a/git-compat-util.h > > +++ b/git-compat-util.h > > @@ -74,7 +74,8 @@ > > # define _XOPEN_SOURCE 500 > > # endif > > #elif !defined(__APPLE__) && !defined(__FreeBSD__) && !defined(__USLC__) && > > \ > > - !defined(_M_UNIX) && !defined(__sgi) && !defined(__DragonFly__) > > + !defined(_M_UNIX) && !defined(__sgi) && !defined(__DragonFly__) && \ > > + !defined(__TANDEM) > > #define _XOPEN_SOURCE 600 /* glibc2 and AIX 5.3L need 500, OpenBSD needs > > 600 fo > > #define _XOPEN_SOURCE_EXTENDED 1 /* AIX 5.3L needs this */ > > #endif > > +#ifdef __TANDEM /* or HAVE_STRINGS_H ? */ > > +#include /* for strcasecmp() */ > > +#endif > > #include > > #include > > #include > > Yeah, it appears that glibc headers have strcasecmp() and friends in > the and that was why majority of us were fine without > including . A cursory look of /usr/include/strings.h on > a GNU system suggests that it is safe to include after > we incude on that platform. > > I think it is OK to leave it "__TANDEM /* or HAVE_STRINGS_H? */" for > now and let the next person who wants to port us to a platform that > needs this inclusion turn it to HAVE_STRINGS_H. Alternatively, we > bite the bullet now and include on any platform that has > the header file and see if anybody complains That's exaclty why I'm asking here ;-), seems a decision needs to be made. How would one differentiate platrots that have strings.h from those that don't? Guess it wont'f work without some ifdef. But it could be NO_STRINGS_H and force the platforms that don't have to update this in Makefile? Reminds me of a related issue: in compat/fnmatch/fnmatch.c there is this: #if HAVE_STRING_H || defined _LIBC # include #else # include #endif There's no place where HAVE_STRING_H get set This looks wrong to me, as here, at least for NonStop, I have to takes measure in Makefile, because there's no other place where HAVE_STRING_H ever gets set: COMPAT_CFLAGS += -DHAVE_STRING_H=1 # needed in compat/fnmatch/fnmatch.c Do platforms exist without string.h? Maybe fnmatch.c should look like this instead? #if HAVE_STRING_H || defined _LIBC # include #endif # ifndef NO_STRINGS_H # include #endif > (that reminds me; I at > least should get one flavor of BSD build environment for this kind > of thing myself). > > > @@ -141,6 +145,10 @@ > > #else > > #include > > #endif > > +#ifdef __TANDEM /* or NO_INTPTR_T resp. NO_UINTPTR_T? */ > > +typedef int intptr_t; > > +typedef unsigned int uintptr_t; > > +#endif > > A bit wider context for this hunk is > > #ifndef NO_INTTYPES_H > #include > #else > #include > #endif > > So we have been assuming that has intptr_t but __TANDEM > apparently doesn't. Exactly. Our stdint.h says: /* * Special integer types (optional types intptr_t/uintptr_t not defined) */ This may change in the future though. One reason why __TANDEM might not be the best check :-) > POSIX requires intptr_t and uintptr_t to be > declared for systems conforming to XSI, but otherwise these are > optional (in other words, some XSI non-conforming platforms may have > them in ), so it would not help to check _XOPEN_UNIX to > see if the system is XSI X-<. We would need NO_INTPTR_T as you > hinted above, perhaps like this. > > #ifndef NO_INTTYPES_H > #include > #else > #include > #endif > #ifdef NO_INTPTR_T > typedef int intptr_t; > typedef unsigned int uintptr_t; > #endif NO_INTPTR_T for both types? OK by me. If need be an NOUINTPTR could get added later, I guess > By the way, is "int" wide enough, or should they be "long"? int and long have the same size, 32-bit, here on NonStop. But we do have 64-bit types too. Not sure which to take though. Bye, Jojo -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: git no longer prompting for password
On Fri, Aug 24, 2012 at 09:19:28PM +0100, Iain Paton wrote: > A recent update to git 1.7.12 from 1.7.3.5 seems to have changed > something - trying to push to a smart http backend no longer prompts > for a password and hence fails the server auth. > [...] > Backtracking through the versions I've skipped and this doesn't seem > to be a new problem, client side up to 1.7.7.7 works, 1.7.8 onwards > don't. Server side version doesn't seem to make a difference. There was some work in v1.7.8 to avoid prompting for a password when it is not necessary; I suspect this is a fallout of that. You could try bisecting the bug. My guess is that you will end up at commit 986bbc0 (http: don't always prompt for password, 2011-11-04). > user@fubar01:~/test# git --version > git version 1.7.7.7 > user@fubar01:~/test# git push http://ipaton@10.0.0.1/git/test.git master > Password: As per the discussion in 986bbc0, this is actually prompting you before git makes any request. Whereas here: > user@fubar01:~/test# git --version > git version 1.7.8 > user@fubar01:~/test# git push http://ipaton@10.0.0.1/git/test.git master > --verbose We should get an HTTP 401 from the server, then prompt, then retry. What's weird is that it sort of works: > Pushing to http://ipaton@10.0.0.1/git/test.git > Counting objects: 6, done. > Delta compression using up to 8 threads. > Compressing objects: 100% (3/3), done. > Writing objects: 100% (5/5), 491 bytes, done. > Total 5 (delta 0), reused 0 (delta 0) > error: RPC failed; result=22, HTTP code = 401 > fatal: The remote end hung up unexpectedly > fatal: The remote end hung up unexpectedly It's like the initial http requests do not get a 401, and the push proceeds, and then some later request causes a 401 when we do not expect it. Which is doubly odd, since we should also be able to handle that case (the first 401 we get should cause us to ask for a password). Can you show us the result of running with GIT_CURL_VERBOSE=1? I'd really like to see which requests are being made with and without authentication. > Looking at changelogs for 1.7.8 and I'm not really seeing anything > that says I need to do something different. No, you shouldn't need to do anything different. I'd suspect the weirdness you are seeing is from a credential helper trying to supply a blank password, except that you would have to have configured one manually for it to run (I assume you are not on a shared machine where somebody might have tweaked /etc/gitconfig or anything like that). -Peff -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] contrib: GnomeKeyring support + generic helper implementation
On Fri, Aug 24, 2012 at 11:15:36AM -0700, Junio C Hamano wrote: > > The third and fourth patches port the existing helpers to this generic > > implementation. > > > It struck me somewhat odd to see a new one added as the first step > in the series, and then "the generic", the third patch only to lose > code from the first one, and then use the same code reduction of > existing one with the last one in the series. A natural progression > would have been the introduction of generic infrastructure > (including the tiny bit this series had to add as part of 4/4) as > the first patch, update existing OSX one to it as the second, and > then build a new Gnome one on the infrastructure as the third and > final patch; that way we have to review less code, too ;-). I think this is partially because I talked with Phillipp off-list and was somewhat unsure how much we wanted to factor out of the helpers. My initial thought was that the protocol would be sufficiently simple that helpers would just be stand-alone and not need to share code with each other. Then the generic bits would not have to worry about being cross-platform compatible. However, the shared bits are simple enough that maybe that is not a concern. An interesting test would be to add a 5/4 porting Erik's win32 credential helper, since that is the platform least like our other ones. So I am OK with this series, but I am also OK with leaving it at patch 1, and just keeping the implementations separate. -Peff -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] contrib: GnomeKeyring support + generic helper implementation
Jeff King writes: > However, the shared bits are simple enough that maybe that is not a > concern. An interesting test would be to add a 5/4 porting Erik's win32 > credential helper, since that is the platform least like our other ones. Very true. > So I am OK with this series, but I am also OK with leaving it at patch > 1, and just keeping the implementations separate. Amen. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC] Support for HP NonStop
"Joachim Schmitz" writes: > Reminds me of a related issue: in compat/fnmatch/fnmatch.c there is this: > #if HAVE_STRING_H || defined _LIBC > # include > #else > # include > #endif > > There's no place where HAVE_STRING_H get set > This looks wrong to me,... This is because it is a borrowed file from glibc, and we try to minimize changes to such a file. If you need HAVE_STRING_H to force inclusion of on your platform, doing this: >COMPAT_CFLAGS += -DHAVE_STRING_H=1 # needed in compat/fnmatch/fnmatch.c is perfectly the right thing to do. > Do platforms exist without string.h? > Maybe fnmatch.c should look like this instead? We try to minimize changes to such a file we borrow from upstream; especially we do not do so lightly when we have to ask "do platforms exist?" Of course, they do---otherwise glibc folks wouldn't have written such a conditional. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
bug: when applying binary diffs, -p0 is ignored
I found a bug in git's handling of binary diff segments.
When applying binary diffs using -p0, the prefix (or --strip) argument
is ignored.
For example, try this:
git apply -p0 --binary <<'EOF'
diff --git a/init.tar.gz a/init.tar.gz
new file mode 100644
index ..
386b94f511a17a8a3d62eb6cec14694cb9b9b51d
GIT binary patch
literal 118
zcmb2|=3qGT-8_JS`RzGFp(Y0bmIKvX{ySXzs`-OhSfm*K#a}SG%CY!eN_LjnjGuP-
zFHDep|-nAGgFaXfQAU0L+LoA^-pY
literal 0
HcmV?d1
EOF
It will create init.tar.gz, but in the root of the repository, not in a/
(Sorry if my mailer wraps the long index line)
cheers,
Colin McCabe
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Document the --done option.
Junio C Hamano writes: > "Eric S. Raymond" writes: > >> --- > > A forgotten Sign-off? Ping? Just telling us that this is Signed-off is fine. Thanks. > > Sverre, the text matches my understanding as well as what be56862 > (fast-import: introduce 'done' command, 2011-07-16) says it did. > Ack? > >> Documentation/git-fast-import.txt |8 +++- >> 1 file changed, 7 insertions(+), 1 deletion(-) >> >> diff --git a/Documentation/git-fast-import.txt >> b/Documentation/git-fast-import.txt >> index 2620d28..9291ea0 100644 >> --- a/Documentation/git-fast-import.txt >> +++ b/Documentation/git-fast-import.txt >> @@ -39,6 +39,10 @@ OPTIONS >> See ``Date Formats'' below for details about which formats >> are supported, and their syntax. >> >> +-- done:: >> +Terminate with error if there is no 'done' command at the >> +end of the stream. >> + >> --force:: >> Force updating modified existing branches, even if doing >> so would cause commits to be lost (as the new commit does >> @@ -1047,7 +1051,9 @@ done:: >> Error out if the stream ends without a 'done' command. >> Without this feature, errors causing the frontend to end >> abruptly at a convenient point in the stream can go >> -undetected. >> +undetected. This may occur, for example, if an import >> +front end dies in mid-operation without emitting SIGTERM >> +or SIGKILL at its subordinate git fast-import instance. >> >> `option` >> >> -- >> 1.7.9.5 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH/RFC] git svn: don't introduce new paragraph for git-svn-id
Junio C Hamano wrote: > Eric Wong writes: >> >> I think having "svn" in "svn.trimsvnlog" twice is redundant and not ideal. >> Perhaps just --trim-log and svn.trimlog? > > Do we ever want to trim "our" log when relaying the Git commits back > to subversion? Having "svn" in "trimsvnlog" makes it clear that the > logs from subversion side is getting trimmed. `git commit' already trims the messages (except for removing the leading whitespaces from the first non-whitespace-only line) and git svn doesn't change that. The new option affects the way the messages are imported from svn to git, with one exception when the --add-author-from option of dcommit is used (in which case it may skip adding an extra new line character before the `From: ' line). For that reason --trim-svn-log might be a better name. Regards, robert -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH/RFC] git svn: optionally trim imported log messages
Junio C Hamano writes:
Hi,
Junio, for some reason I don't get mails from you, I've just discovered
your e-mails on gmane news list. Anyway many thanks for your comments,
I'll fix them and send updated patch next week.
>> +When committing to svn from git (as part of 'commit-diff', 'set-tree'
>> +or 'dcommit') and '--add-author-from' is in effect, a new line character
>> +is not added before the `From: ` line if the log message ends with
>> +a pseudo-headers section.
>
> I think it would be saner to call them "trailers" to avoid
> confusion.
Thanks, I haven't got any idea how to call them, especially because
existing git documentation refers to them just by using the word `line',
e.g.:
git-am.txt: Add a `Signed-off-by:` line to the commit message,
git-cherry-pick.txt:Add Signed-off-by line at the end of the
(The "trailer" keyword appears once in SubmittingPatches and - in a bit
different meaning in technical/pack-format.txt)
>
> I've seen S-o-b, Cc and Change-Id there, but does anybody actually
> put "From: " there?
Yes, `git svn dcommit --add-author-from' adds such a trailer to the svn
log message, and then resets or rebases the git index against svn, so
that the message with the trailer appears in git.
>
> There needs an explanation to the reader why this is an optional
> feature.
OK, I'll add some explanation. Basically it is optional, per Eric
request, for backward compatibility to make it possible to work on a
centralized clone of svn repository by people using both old and new
versions of git svn.
>
> The prerequisite mechanism is to allow some tests in an environment
> where they cannot be run (e.g. no SVN available on the platform);
> any code that uses set_prereq unconditionally looks extremely
> strange. It is OK while the patch is in RFC stage under active
> debugging, but they would want to go away before the polishing is
> done.
OK, I used it merely for checking that the tests work on older version
of git svn, and I didn't break the backward compatibility by accident.
Will be dropped from next version of the patch.
>
> What does En-El stand for? We often see (but not in Git where we
> prefer LF for that purpose)
>
> NL='
> ' ;# newline
>
> and using $NL to mean "empty" may be misleading to the readers.
NL means newline. The new line characters implicitly added after each
commit message line, that's why the value is empty. But, yes, this can
be misleading. I'd prefer to keep it short, so would EL (i.e.
`empty-line') be an acceptable name?
>> +N=$((N + 1)) &&
>
Sorry, it was a typo, I meant to use $(($N + 1)). Thanks for catching this.
>
> next_N () {
> N=$(($N + 1)) &&
> ...
> }
>
> (the above also has two style fixes).
Just to be sure: shall the `...' line start a new level of indentation
or is it a typo? (I guess that the two style fixes are just after the
function name.)
Thanks a lot,
robert
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH/RFC] git svn: optionally trim imported log messages
Robert Luberda writes:
>> I think it would be saner to call them "trailers" to avoid
>> confusion.
>
> Thanks, I haven't got any idea how to call them, especially because
> existing git documentation refers to them just by using the word `line',
> e.g.:
>
> git-am.txt: Add a `Signed-off-by:` line to the commit message,
> git-cherry-pick.txt:Add Signed-off-by line at the end of the
Then "line" is fine; they never come before the body, and are
certainly not headers.
>> There needs an explanation to the reader why this is an optional
>> feature.
>
> OK, I'll add some explanation. Basically it is optional, per Eric
> request, for backward compatibility to make it possible to work on a
> centralized clone of svn repository by people using both old and new
> versions of git svn.
That matches my recollection. I didn't ask you to explain it to me,
by the way, as I've skimmed the discussion during the review.
I wanted the resulting history and the documentation to explain that
to git-svn users.
> NL means newline. The new line characters implicitly added after each
> commit message line, that's why the value is empty. But, yes, this can
> be misleading. I'd prefer to keep it short, so would EL (i.e.
> `empty-line') be an acceptable name?
I'd rather call it "$EMPTY"; $NL is already obscure, nobody uses $EL.
>> next_N () {
>> N=$(($N + 1)) &&
>> ...
>> }
>>
>> (the above also has two style fixes).
>
> Just to be sure: shall the `...' line start a new level of indentation
> or is it a typo?
It was meant to align with "N=", but perhaps HT and quoting
interacted badly or something.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH/RFC] git svn: handle errors and concurrent commits in dcommit
Eric Wong wrote: Hi, > > Oops, I'll push the following out since Junio already merged your > original: I can see that you haven't pushed the change yet. Maybe it would be a good idea to fix other style mistakes (extra spaces after redirections, lack of spaces after function names, `[' used instead of `test', etc) as well? I can prepare a patch if you think it is a good idea. Regards, robert -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: misleading diff-hunk header
On 08/24/12 11:44, Jeff King wrote:
> With the old code, you'd get:
>
> diff --git a/old b/new
> index f384549..1066a25 100644
> --- a/old
> +++ b/new
> @@ -2,3 +2,3 @@ one
>two
> -three
> +three -- modified
>four
>
> So the hunk header is showing you something useful; the element just
> above your context. But with my patch, you'd see:
>
> diff --git a/old b/new
> index f384549..1066a25 100644
> --- a/old
> +++ b/new
> @@ -2,3 +2,3 @@ two
>two
> -three
> +three -- modified
>four
>
> I.e., it shows the element just before the change, which is already in
> the context anyway. So it's actually less useful. Although note that the
> current behavior is not all that useful, either; it is not really giving
> you any information about the change, but rather just showing one extra
> line of context.
>
> So I would say that which you would prefer might depend on exactly what
> you are diffing. But I would also argue that in any case where the new
> code produces a worse result, the hunk header was not all that useful to
> begin with.
If the documented purpose of "diff -p" (and by proxy
diff.{type}.xfuncname) is to show the name of the *function*
containing the changed lines, and all you have is a list of lines
with no function names, it's pretty arbitrary to call either
behavior "worse". :-)
-tkc
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] Improve "Not a git repository" error messages
From: Travis Carden
The former messages changed grammatical subject in the middle.
Signed-off-by: Travis Carden
---
This is my first attempt at contributing to the Git project.
I'm kind of testing the water with a simple patch to see how
friendly the community is. Thanks!
setup.c |4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/setup.c b/setup.c
index 9139bee..0cd88e5 100644
--- a/setup.c
+++ b/setup.c
@@ -599,7 +599,7 @@ static const char *setup_bare_git_dir(char *cwd, int
offset, int len, int *nongi
static const char *setup_nongit(const char *cwd, int *nongit_ok)
{
if (!nongit_ok)
- die("Not a git repository (or any of the parent directories):
%s", DEFAULT_GIT_DIR_ENVIRONMENT);
+ die("Not a git repository (or a descendant of one): %s",
DEFAULT_GIT_DIR_ENVIRONMENT);
if (chdir(cwd))
die_errno("Cannot come back to cwd");
*nongit_ok = 1;
@@ -706,7 +706,7 @@ static const char *setup_git_directory_gently_1(int
*nongit_ok)
return NULL;
}
cwd[offset] = '\0';
- die("Not a git repository (or any parent up to
mount point %s)\n"
+ die("Not a git repository (or a descendant of
one up to mount point %s)\n"
"Stopping at filesystem boundary
(GIT_DISCOVERY_ACROSS_FILESYSTEM not set).", cwd);
}
}
--
1.7.9.5
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: misleading diff-hunk header
Tim Chase writes:
> If the documented purpose of "diff -p" (and by proxy
> diff.{type}.xfuncname) is to show the name of the *function*
> containing the changed lines,
Yeah, the documentation is misleading, but I do not offhand think of
a better phrasing. Perhaps you could send in a patch to improve it.
How does GNU manual explain the option?
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 06/17] Let fetch_pack() inform caller about number of unique heads
On 08/23/2012 10:54 AM, Jeff King wrote: > On Thu, Aug 23, 2012 at 10:10:31AM +0200, mhag...@alum.mit.edu wrote: > >> From: Michael Haggerty >> >> fetch_pack() remotes duplicates from the list (nr_heads, heads), >> thereby shrinking the list. But previously, the caller was not >> informed about the shrinkage. This would cause a spurious error >> message to be emitted by cmd_fetch_pack() if "git fetch-pack" is >> called with duplicate refnames. >> >> So change the signature of fetch_pack() to accept nr_heads by >> reference, and if any duplicates were removed then modify it to >> reflect the number of remaining references. >> >> The last test of t5500 inexplicably *required* "git fetch-pack" to >> fail when fetching a list of references that contains duplicates; >> i.e., it insisted on the buggy behavior. So change the test to expect >> the correct behavior. > > Eek, yeah, the current behavior is obviously wrong. The > remove_duplicates code comes from 310b86d (fetch-pack: do not barf when > duplicate re patterns are given, 2006-11-25) and clearly meant for > fetch-pack to handle this case gracefully. > >> diff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh >> index 3cc3346..0d4edcb 100755 >> --- a/t/t5500-fetch-pack.sh >> +++ b/t/t5500-fetch-pack.sh >> @@ -391,7 +391,7 @@ test_expect_success 'fetch mixed refs from cmdline and >> stdin' ' >> test_expect_success 'test duplicate refs from stdin' ' >> ( >> cd client && >> -test_must_fail git fetch-pack --stdin --no-progress .. <../input.dup >> +git fetch-pack --stdin --no-progress .. <../input.dup >> ) >output && >> cut -d " " -f 2 actual && >> test_cmp expect actual > > It's interesting that the output was the same before and after the fix. > I guess that is because the error comes at the very end, when we are > making sure all of the provided heads have been consumed. "git fetch-pack" emits information about successfully-received references regardless of whether some requested references were not received. The "no such remote ref %s" output goes to stderr. So the only difference between before/after fix should be what is written to stderr, whereas the test only looks at stdout. Michael -- Michael Haggerty mhag...@alum.mit.edu http://softwareswirl.blogspot.com/ -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: bug: when applying binary diffs, -p0 is ignored
Colin McCabe writes:
> I found a bug in git's handling of binary diff segments.
>
> When applying binary diffs using -p0, the prefix (or --strip) argument
> is ignored.
Thanks for a report. An ancient bug well spotted.
Perhaps this will help.
-- >8 --
Subject: apply: compute patch->def_name correctly under -p0
Back when "git apply" was written, we made sure that the user can
skip more than the default number of path components (i.e. 1) by
giving "-p", but the logic for doing so was built around the
notion of "we skip N slashes and stop". This obviously does not
work well when running under -p0 where we do not want to skip any,
but still want to skip SP/HT that separates the pathnames of
preimage and postimage and want to reject absolute pathnames.
Stop using "stop_at_slash()", and instead introduce a new helper
"skip_tree_prefix()" with similar logic but works correctly even for
the -p0 case.
This is an ancient bug, but has been masked for a long time because
most of the patches are text and have other clues to tell us the
name of the preimage and the postimage.
Signed-off-by: Junio C Hamano
---
builtin/apply.c | 55 ++-
1 file changed, 30 insertions(+), 25 deletions(-)
diff --git i/builtin/apply.c w/builtin/apply.c
index 3bf71dc..4a511b3 100644
--- i/builtin/apply.c
+++ w/builtin/apply.c
@@ -1095,15 +1095,23 @@ static int gitdiff_unrecognized(const char *line,
struct patch *patch)
return -1;
}
-static const char *stop_at_slash(const char *line, int llen)
+/*
+ * Skip p_value leading components from "line"; as we do not accept
+ * absolute paths, return NULL in that case.
+ */
+static const char *skip_tree_prefix(const char *line, int llen)
{
- int nslash = p_value;
+ int nslash;
int i;
+ if (!p_value)
+ return (llen && line[0] == '/') ? NULL : line;
+
+ nslash = p_value;
for (i = 0; i < llen; i++) {
int ch = line[i];
if (ch == '/' && --nslash <= 0)
- return &line[i];
+ return (i == 0) ? NULL : &line[i + 1];
}
return NULL;
}
@@ -1133,12 +1141,11 @@ static char *git_header_name(const char *line, int llen)
if (unquote_c_style(&first, line, &second))
goto free_and_fail1;
- /* advance to the first slash */
- cp = stop_at_slash(first.buf, first.len);
- /* we do not accept absolute paths */
- if (!cp || cp == first.buf)
+ /* strip the a/b prefix including trailing slash */
+ cp = skip_tree_prefix(first.buf, first.len);
+ if (!cp)
goto free_and_fail1;
- strbuf_remove(&first, 0, cp + 1 - first.buf);
+ strbuf_remove(&first, 0, cp - first.buf);
/*
* second points at one past closing dq of name.
@@ -1152,22 +1159,21 @@ static char *git_header_name(const char *line, int llen)
if (*second == '"') {
if (unquote_c_style(&sp, second, NULL))
goto free_and_fail1;
- cp = stop_at_slash(sp.buf, sp.len);
- if (!cp || cp == sp.buf)
+ cp = skip_tree_prefix(sp.buf, sp.len);
+ if (!cp)
goto free_and_fail1;
/* They must match, otherwise ignore */
- if (strcmp(cp + 1, first.buf))
+ if (strcmp(cp, first.buf))
goto free_and_fail1;
strbuf_release(&sp);
return strbuf_detach(&first, NULL);
}
/* unquoted second */
- cp = stop_at_slash(second, line + llen - second);
- if (!cp || cp == second)
+ cp = skip_tree_prefix(second, line + llen - second);
+ if (!cp)
goto free_and_fail1;
- cp++;
- if (line + llen - cp != first.len + 1 ||
+ if (line + llen - cp != first.len ||
memcmp(first.buf, cp, first.len))
goto free_and_fail1;
return strbuf_detach(&first, NULL);
@@ -1179,10 +1185,9 @@ static char *git_header_name(const char *line, int llen)
}
/* unquoted first name */
- name = stop_at_slash(line, llen);
- if (!name || name == line)
+ name = skip_tree_prefix(line, llen);
+ if (!name)
return NULL;
- name++;
/*
* since the first name is unquoted, a dq if exists must be
@@ -1196,10 +1201,9 @@ static char *git_header_name(const char *line, int llen)
if (unquote_c_style(&sp, second, NULL))
goto free_and_fail2;
-
Re: [PATCH 17/17] fetch_refs(): simplify logic
On 08/23/2012 11:07 AM, Jeff King wrote: > On Thu, Aug 23, 2012 at 10:10:42AM +0200, mhag...@alum.mit.edu wrote: > >> Subject: Re: [PATCH 17/17] fetch_refs(): simplify logic >> [...] >> static void filter_refs(struct ref **refs, int *nr_heads, char **heads) > > The subject should be "filter_refs", no? Yes, thanks. Also thanks for the typo correction in [PATCH 16/17], and in general for your review of the patch series. Re-roll to follow shortly. Michael -- Michael Haggerty mhag...@alum.mit.edu http://softwareswirl.blogspot.com/ -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 01/17] t5500: add tests of error output for missing refs
From: Michael Haggerty If "git fetch-pack" is called with reference names that do not exist on the remote, then it should emit an error message error: no such remote ref refs/heads/xyzzy This is currently broken if *only* missing references are passed to "git fetch-pack". Signed-off-by: Michael Haggerty --- t/t5500-fetch-pack.sh | 30 ++ 1 file changed, 30 insertions(+) diff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh index e80a2af..3cc3346 100755 --- a/t/t5500-fetch-pack.sh +++ b/t/t5500-fetch-pack.sh @@ -397,4 +397,34 @@ test_expect_success 'test duplicate refs from stdin' ' test_cmp expect actual ' +test_expect_success 'set up tests of missing reference' ' + cat >expect-error <<-\EOF + error: no such remote ref refs/heads/xyzzy + EOF +' + +test_expect_failure 'test lonely missing ref' ' + ( + cd client && + test_must_fail git fetch-pack --no-progress .. refs/heads/xyzzy + ) >/dev/null 2>error-m && + test_cmp expect-error error-m +' + +test_expect_success 'test missing ref after existing' ' + ( + cd client && + test_must_fail git fetch-pack --no-progress .. refs/heads/A refs/heads/xyzzy + ) >/dev/null 2>error-em && + test_cmp expect-error error-em +' + +test_expect_success 'test missing ref before existing' ' + ( + cd client && + test_must_fail git fetch-pack --no-progress .. refs/heads/xyzzy refs/heads/A + ) >/dev/null 2>error-me && + test_cmp expect-error error-me +' + test_done -- 1.7.11.3 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 02/17] Rename static function fetch_pack() to http_fetch_pack()
From: Michael Haggerty
Avoid confusion with the non-static function of the same name from
fetch-pack.h.
Signed-off-by: Michael Haggerty
---
http-walker.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/http-walker.c b/http-walker.c
index 51a906e..1516c5e 100644
--- a/http-walker.c
+++ b/http-walker.c
@@ -396,7 +396,7 @@ static int fetch_indices(struct walker *walker, struct
alt_base *repo)
return ret;
}
-static int fetch_pack(struct walker *walker, struct alt_base *repo, unsigned
char *sha1)
+static int http_fetch_pack(struct walker *walker, struct alt_base *repo,
unsigned char *sha1)
{
struct packed_git *target;
int ret;
@@ -524,7 +524,7 @@ static int fetch(struct walker *walker, unsigned char *sha1)
if (!fetch_object(walker, altbase, sha1))
return 0;
while (altbase) {
- if (!fetch_pack(walker, altbase, sha1))
+ if (!http_fetch_pack(walker, altbase, sha1))
return 0;
fetch_alternates(walker, data->alt->base);
altbase = altbase->next;
--
1.7.11.3
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 04/17] Name local variables more consistently
From: Michael Haggerty
Use the names (nr_heads, heads) consistently across functions, instead
of sometimes naming the same values (nr_match, match).
Signed-off-by: Michael Haggerty
---
builtin/fetch-pack.c | 35 +--
1 file changed, 17 insertions(+), 18 deletions(-)
diff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c
index cc21047..76288a2 100644
--- a/builtin/fetch-pack.c
+++ b/builtin/fetch-pack.c
@@ -521,27 +521,27 @@ static void mark_recent_complete_commits(unsigned long
cutoff)
}
}
-static void filter_refs(struct ref **refs, int nr_match, char **match)
+static void filter_refs(struct ref **refs, int nr_heads, char **heads)
{
struct ref **return_refs;
struct ref *newlist = NULL;
struct ref **newtail = &newlist;
struct ref *ref, *next;
struct ref *fastarray[32];
- int match_pos;
+ int head_pos;
- if (nr_match && !args.fetch_all) {
- if (ARRAY_SIZE(fastarray) < nr_match)
- return_refs = xcalloc(nr_match, sizeof(struct ref *));
+ if (nr_heads && !args.fetch_all) {
+ if (ARRAY_SIZE(fastarray) < nr_heads)
+ return_refs = xcalloc(nr_heads, sizeof(struct ref *));
else {
return_refs = fastarray;
- memset(return_refs, 0, sizeof(struct ref *) * nr_match);
+ memset(return_refs, 0, sizeof(struct ref *) * nr_heads);
}
}
else
return_refs = NULL;
- match_pos = 0;
+ head_pos = 0;
for (ref = *refs; ref; ref = next) {
next = ref->next;
if (!memcmp(ref->name, "refs/", 5) &&
@@ -556,17 +556,17 @@ static void filter_refs(struct ref **refs, int nr_match,
char **match)
}
else {
int cmp = -1;
- while (match_pos < nr_match) {
- cmp = strcmp(ref->name, match[match_pos]);
+ while (head_pos < nr_heads) {
+ cmp = strcmp(ref->name, heads[head_pos]);
if (cmp < 0) /* definitely do not have it */
break;
else if (cmp == 0) { /* definitely have it */
- match[match_pos][0] = '\0';
- return_refs[match_pos] = ref;
+ heads[head_pos][0] = '\0';
+ return_refs[head_pos] = ref;
break;
}
else /* might have it; keep looking */
- match_pos++;
+ head_pos++;
}
if (!cmp)
continue; /* we will link it later */
@@ -576,7 +576,7 @@ static void filter_refs(struct ref **refs, int nr_match,
char **match)
if (!args.fetch_all) {
int i;
- for (i = 0; i < nr_match; i++) {
+ for (i = 0; i < nr_heads; i++) {
ref = return_refs[i];
if (ref) {
*newtail = ref;
@@ -595,7 +595,7 @@ static void mark_alternate_complete(const struct ref *ref,
void *unused)
mark_complete(NULL, ref->old_sha1, 0, NULL);
}
-static int everything_local(struct ref **refs, int nr_match, char **match)
+static int everything_local(struct ref **refs, int nr_heads, char **heads)
{
struct ref *ref;
int retval;
@@ -646,7 +646,7 @@ static int everything_local(struct ref **refs, int
nr_match, char **match)
}
}
- filter_refs(refs, nr_match, match);
+ filter_refs(refs, nr_heads, heads);
for (retval = 1, ref = *refs; ref ; ref = ref->next) {
const unsigned char *remote = ref->old_sha1;
@@ -777,8 +777,7 @@ static int get_pack(int xd[2], char **pack_lockfile)
static struct ref *do_fetch_pack(int fd[2],
const struct ref *orig_ref,
- int nr_match,
- char **match,
+ int nr_heads, char **heads,
char **pack_lockfile)
{
struct ref *ref = copy_ref_list(orig_ref);
@@ -819,7 +818,7 @@ static struct ref *do_fetch_pack(int fd[2],
fprintf(stderr, "Server supports ofs-delta\n");
} else
prefer_ofs_delta = 0;
- if (everything_local(&ref, nr_match, match)) {
+ if (everything_local(&ref, nr_heads, heads)) {
packet_flush(fd[1]);
goto all_done;
}
--
1.7.11.3
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More m
[PATCH v2 07/17] Pass nr_heads to do_pack_ref() by reference
From: Michael Haggerty
This is the first of a few baby steps towards changing filter_refs()
to compress matched refs out of the list rather than overwriting the
first character of such references with '\0'.
Signed-off-by: Michael Haggerty
---
builtin/fetch-pack.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c
index cf65998..fae4f7c 100644
--- a/builtin/fetch-pack.c
+++ b/builtin/fetch-pack.c
@@ -777,7 +777,7 @@ static int get_pack(int xd[2], char **pack_lockfile)
static struct ref *do_fetch_pack(int fd[2],
const struct ref *orig_ref,
- int nr_heads, char **heads,
+ int *nr_heads, char **heads,
char **pack_lockfile)
{
struct ref *ref = copy_ref_list(orig_ref);
@@ -818,7 +818,7 @@ static struct ref *do_fetch_pack(int fd[2],
fprintf(stderr, "Server supports ofs-delta\n");
} else
prefer_ofs_delta = 0;
- if (everything_local(&ref, nr_heads, heads)) {
+ if (everything_local(&ref, *nr_heads, heads)) {
packet_flush(fd[1]);
goto all_done;
}
@@ -1086,7 +1086,7 @@ struct ref *fetch_pack(struct fetch_pack_args *my_args,
packet_flush(fd[1]);
die("no matching remote head");
}
- ref_cpy = do_fetch_pack(fd, ref, *nr_heads, heads, pack_lockfile);
+ ref_cpy = do_fetch_pack(fd, ref, nr_heads, heads, pack_lockfile);
if (args.depth > 0) {
struct cache_time mtime;
--
1.7.11.3
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 08/17] Pass nr_heads to everything_local() by reference
From: Michael Haggerty
Signed-off-by: Michael Haggerty
---
builtin/fetch-pack.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c
index fae4f7c..a4bb0ff 100644
--- a/builtin/fetch-pack.c
+++ b/builtin/fetch-pack.c
@@ -595,7 +595,7 @@ static void mark_alternate_complete(const struct ref *ref,
void *unused)
mark_complete(NULL, ref->old_sha1, 0, NULL);
}
-static int everything_local(struct ref **refs, int nr_heads, char **heads)
+static int everything_local(struct ref **refs, int *nr_heads, char **heads)
{
struct ref *ref;
int retval;
@@ -646,7 +646,7 @@ static int everything_local(struct ref **refs, int
nr_heads, char **heads)
}
}
- filter_refs(refs, nr_heads, heads);
+ filter_refs(refs, *nr_heads, heads);
for (retval = 1, ref = *refs; ref ; ref = ref->next) {
const unsigned char *remote = ref->old_sha1;
@@ -818,7 +818,7 @@ static struct ref *do_fetch_pack(int fd[2],
fprintf(stderr, "Server supports ofs-delta\n");
} else
prefer_ofs_delta = 0;
- if (everything_local(&ref, *nr_heads, heads)) {
+ if (everything_local(&ref, nr_heads, heads)) {
packet_flush(fd[1]);
goto all_done;
}
--
1.7.11.3
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 10/17] Remove ineffective optimization
From: Michael Haggerty
I cannot find a scenario in which this function is called any
significant number of times, so simplify the code by always allocating
an array for return_refs rather than trying to use a stack-allocated
array for small lists.
Signed-off-by: Michael Haggerty
---
builtin/fetch-pack.c | 14 +++---
1 file changed, 3 insertions(+), 11 deletions(-)
diff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c
index c47090d..ca1ddd9 100644
--- a/builtin/fetch-pack.c
+++ b/builtin/fetch-pack.c
@@ -527,17 +527,10 @@ static void filter_refs(struct ref **refs, int *nr_heads,
char **heads)
struct ref *newlist = NULL;
struct ref **newtail = &newlist;
struct ref *ref, *next;
- struct ref *fastarray[32];
int head_pos;
- if (*nr_heads && !args.fetch_all) {
- if (ARRAY_SIZE(fastarray) < *nr_heads)
- return_refs = xcalloc(*nr_heads, sizeof(struct ref *));
- else {
- return_refs = fastarray;
- memset(return_refs, 0, sizeof(struct ref *) *
*nr_heads);
- }
- }
+ if (*nr_heads && !args.fetch_all)
+ return_refs = xcalloc(*nr_heads, sizeof(struct ref *));
else
return_refs = NULL;
@@ -584,8 +577,7 @@ static void filter_refs(struct ref **refs, int *nr_heads,
char **heads)
newtail = &ref->next;
}
}
- if (return_refs != fastarray)
- free(return_refs);
+ free(return_refs);
}
*refs = newlist;
}
--
1.7.11.3
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 12/17] filter_refs(): compress unmatched refs in heads array
From: Michael Haggerty
Remove any references that were received from the remote from the list
(*nr_heads, heads) of requested references by squeezing them out of
the list (rather than overwriting their names with NUL characters, as
before). On exit, *nr_heads is the number of requested references
that were not received.
Document this aspect of fetch_pack() in a comment in the header file.
(More documentation is obviously still needed.)
Signed-off-by: Michael Haggerty
---
builtin/fetch-pack.c | 21 +
fetch-pack.h | 6 ++
transport.c | 3 ++-
3 files changed, 21 insertions(+), 9 deletions(-)
diff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c
index a995357..5ba1cef 100644
--- a/builtin/fetch-pack.c
+++ b/builtin/fetch-pack.c
@@ -527,7 +527,7 @@ static void filter_refs(struct ref **refs, int *nr_heads,
char **heads)
struct ref *newlist = NULL;
struct ref **newtail = &newlist;
struct ref *ref, *next;
- int head_pos = 0, matched = 0;
+ int head_pos = 0, matched = 0, unmatched = 0;
if (*nr_heads && !args.fetch_all)
return_refs = xcalloc(*nr_heads, sizeof(struct ref *));
@@ -554,11 +554,12 @@ static void filter_refs(struct ref **refs, int *nr_heads,
char **heads)
break;
else if (cmp == 0) { /* definitely have it */
return_refs[matched++] = ref;
- heads[head_pos++][0] = '\0';
+ head_pos++;
break;
}
- else /* might have it; keep looking */
- head_pos++;
+ else { /* might have it; keep looking */
+ heads[unmatched++] = heads[head_pos++];
+ }
}
if (!cmp)
continue; /* we will link it later */
@@ -568,6 +569,12 @@ static void filter_refs(struct ref **refs, int *nr_heads,
char **heads)
if (!args.fetch_all) {
int i;
+
+ /* copy remaining unmatched heads: */
+ while (head_pos < *nr_heads)
+ heads[unmatched++] = heads[head_pos++];
+ *nr_heads = unmatched;
+
for (i = 0; i < matched; i++) {
ref = return_refs[i];
*newtail = ref;
@@ -1028,10 +1035,8 @@ int cmd_fetch_pack(int argc, const char **argv, const
char *prefix)
* silently succeed without issuing an error.
*/
for (i = 0; i < nr_heads; i++)
- if (heads[i] && heads[i][0]) {
- error("no such remote ref %s", heads[i]);
- ret = 1;
- }
+ error("no such remote ref %s", heads[i]);
+ ret = 1;
}
while (ref) {
printf("%s %s\n",
diff --git a/fetch-pack.h b/fetch-pack.h
index a9d505b..915858e 100644
--- a/fetch-pack.h
+++ b/fetch-pack.h
@@ -17,6 +17,12 @@ struct fetch_pack_args {
stateless_rpc:1;
};
+/*
+ * (*nr_heads, heads) is an array of pointers to the full names of
+ * remote references that should be updated from. On return, both
+ * will have been changed to list only the names that were not found
+ * on the remote.
+ */
struct ref *fetch_pack(struct fetch_pack_args *args,
int fd[], struct child_process *conn,
const struct ref *ref,
diff --git a/transport.c b/transport.c
index f7bbf58..3c38d44 100644
--- a/transport.c
+++ b/transport.c
@@ -520,6 +520,7 @@ static int fetch_refs_via_pack(struct transport *transport,
struct git_transport_data *data = transport->data;
char **heads = xmalloc(nr_heads * sizeof(*heads));
char **origh = xmalloc(nr_heads * sizeof(*origh));
+ int orig_nr_heads = nr_heads;
const struct ref *refs;
char *dest = xstrdup(transport->url);
struct fetch_pack_args args;
@@ -558,7 +559,7 @@ static int fetch_refs_via_pack(struct transport *transport,
free_refs(refs_tmp);
- for (i = 0; i < nr_heads; i++)
+ for (i = 0; i < orig_nr_heads; i++)
free(origh[i]);
free(origh);
free(heads);
--
1.7.11.3
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 13/17] cmd_fetch_pack: return early if finish_connect() returns an error
From: Michael Haggerty
This simplifies the logic without changing the behavior.
Signed-off-by: Michael Haggerty
---
builtin/fetch-pack.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c
index 5ba1cef..f04fd59 100644
--- a/builtin/fetch-pack.c
+++ b/builtin/fetch-pack.c
@@ -1025,10 +1025,10 @@ int cmd_fetch_pack(int argc, const char **argv, const
char *prefix)
close(fd[0]);
close(fd[1]);
if (finish_connect(conn))
- ref = NULL;
- ret = !ref;
+ return 1;
- if (!ret && nr_heads) {
+ ret = !ref;
+ if (ref && nr_heads) {
/* If the heads to pull were given, we should have
* consumed all of them by matching the remote.
* Otherwise, 'git fetch remote no-such-ref' would
--
1.7.11.3
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 14/17] Report missing refs even if no existing refs were received
From: Michael Haggerty
This fixes a test in t5500.
Signed-off-by: Michael Haggerty
---
builtin/fetch-pack.c | 2 +-
t/t5500-fetch-pack.sh | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c
index f04fd59..00ac3b1 100644
--- a/builtin/fetch-pack.c
+++ b/builtin/fetch-pack.c
@@ -1028,7 +1028,7 @@ int cmd_fetch_pack(int argc, const char **argv, const
char *prefix)
return 1;
ret = !ref;
- if (ref && nr_heads) {
+ if (nr_heads) {
/* If the heads to pull were given, we should have
* consumed all of them by matching the remote.
* Otherwise, 'git fetch remote no-such-ref' would
diff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh
index 0d4edcb..f78a118 100755
--- a/t/t5500-fetch-pack.sh
+++ b/t/t5500-fetch-pack.sh
@@ -403,7 +403,7 @@ test_expect_success 'set up tests of missing reference' '
EOF
'
-test_expect_failure 'test lonely missing ref' '
+test_expect_success 'test lonely missing ref' '
(
cd client &&
test_must_fail git fetch-pack --no-progress .. refs/heads/xyzzy
--
1.7.11.3
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 15/17] cmd_fetch_pack(): simplify computation of return value
From: Michael Haggerty
Set the final value at initialization rather than initializing it then
sometimes changing it.
Signed-off-by: Michael Haggerty
---
builtin/fetch-pack.c | 21 ++---
1 file changed, 10 insertions(+), 11 deletions(-)
diff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c
index 00ac3b1..c49dadf 100644
--- a/builtin/fetch-pack.c
+++ b/builtin/fetch-pack.c
@@ -1027,17 +1027,16 @@ int cmd_fetch_pack(int argc, const char **argv, const
char *prefix)
if (finish_connect(conn))
return 1;
- ret = !ref;
- if (nr_heads) {
- /* If the heads to pull were given, we should have
-* consumed all of them by matching the remote.
-* Otherwise, 'git fetch remote no-such-ref' would
-* silently succeed without issuing an error.
-*/
- for (i = 0; i < nr_heads; i++)
- error("no such remote ref %s", heads[i]);
- ret = 1;
- }
+ ret = !ref || nr_heads;
+
+ /*
+* If the heads to pull were given, we should have consumed
+* all of them by matching the remote. Otherwise, 'git fetch
+* remote no-such-ref' would silently succeed without issuing
+* an error.
+*/
+ for (i = 0; i < nr_heads; i++)
+ error("no such remote ref %s", heads[i]);
while (ref) {
printf("%s %s\n",
sha1_to_hex(ref->old_sha1), ref->name);
--
1.7.11.3
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 16/17] fetch_pack(): free matching heads
From: Michael Haggerty
fetch_pack() used to delete entries from the input list (*nr_heads,
heads) and drop them on the floor. (Even earlier versions dropped
some names on the floor and modified others.) This forced
fetch_refs_via_pack() to create a separate copy of the original list
so that it could free all of the names.
Instead, teach fetch_pack() to free any names that it discards from
the list, and change fetch_refs_via_pack() to free only the remaining
(unmatched) names.
Document the change in the function comment in the header file.
Signed-off-by: Michael Haggerty
---
builtin/fetch-pack.c | 4 +++-
fetch-pack.h | 7 ---
transport.c | 9 +++--
3 files changed, 10 insertions(+), 10 deletions(-)
diff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c
index c49dadf..1bc4599 100644
--- a/builtin/fetch-pack.c
+++ b/builtin/fetch-pack.c
@@ -554,7 +554,7 @@ static void filter_refs(struct ref **refs, int *nr_heads,
char **heads)
break;
else if (cmp == 0) { /* definitely have it */
return_refs[matched++] = ref;
- head_pos++;
+ free(heads[head_pos++]);
break;
}
else { /* might have it; keep looking */
@@ -844,6 +844,8 @@ static int remove_duplicates(int nr_heads, char **heads)
for (src = dst = 1; src < nr_heads; src++)
if (strcmp(heads[src], heads[dst-1]))
heads[dst++] = heads[src];
+ else
+ free(heads[src]);
return dst;
}
diff --git a/fetch-pack.h b/fetch-pack.h
index 915858e..d1860eb 100644
--- a/fetch-pack.h
+++ b/fetch-pack.h
@@ -19,9 +19,10 @@ struct fetch_pack_args {
/*
* (*nr_heads, heads) is an array of pointers to the full names of
- * remote references that should be updated from. On return, both
- * will have been changed to list only the names that were not found
- * on the remote.
+ * remote references that should be updated from. The names must have
+ * been allocated from the heap. The names that are found in the
+ * remote will be removed from the list and freed; the others will be
+ * left in the front of the array with *nr_heads adjusted accordingly.
*/
struct ref *fetch_pack(struct fetch_pack_args *args,
int fd[], struct child_process *conn,
diff --git a/transport.c b/transport.c
index 3c38d44..f94b753 100644
--- a/transport.c
+++ b/transport.c
@@ -519,8 +519,6 @@ static int fetch_refs_via_pack(struct transport *transport,
{
struct git_transport_data *data = transport->data;
char **heads = xmalloc(nr_heads * sizeof(*heads));
- char **origh = xmalloc(nr_heads * sizeof(*origh));
- int orig_nr_heads = nr_heads;
const struct ref *refs;
char *dest = xstrdup(transport->url);
struct fetch_pack_args args;
@@ -539,7 +537,7 @@ static int fetch_refs_via_pack(struct transport *transport,
args.depth = data->options.depth;
for (i = 0; i < nr_heads; i++)
- origh[i] = heads[i] = xstrdup(to_fetch[i]->name);
+ heads[i] = xstrdup(to_fetch[i]->name);
if (!data->got_remote_heads) {
connect_setup(transport, 0, 0);
@@ -559,9 +557,8 @@ static int fetch_refs_via_pack(struct transport *transport,
free_refs(refs_tmp);
- for (i = 0; i < orig_nr_heads; i++)
- free(origh[i]);
- free(origh);
+ for (i = 0; i < nr_heads; i++)
+ free(heads[i]);
free(heads);
free(dest);
return (refs ? 0 : -1);
--
1.7.11.3
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 17/17] filter_refs(): simplify logic
From: Michael Haggerty
* Build linked list of return values as we go rather than recording
them in a temporary array and linking them up later.
* Handle ref in a single if...else statement in the main loop, to make
it clear that each ref has exactly two possible destinies.
Signed-off-by: Michael Haggerty
---
builtin/fetch-pack.c | 56 ++--
1 file changed, 19 insertions(+), 37 deletions(-)
diff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c
index 1bc4599..db77ee6 100644
--- a/builtin/fetch-pack.c
+++ b/builtin/fetch-pack.c
@@ -523,66 +523,48 @@ static void mark_recent_complete_commits(unsigned long
cutoff)
static void filter_refs(struct ref **refs, int *nr_heads, char **heads)
{
- struct ref **return_refs;
struct ref *newlist = NULL;
struct ref **newtail = &newlist;
struct ref *ref, *next;
- int head_pos = 0, matched = 0, unmatched = 0;
-
- if (*nr_heads && !args.fetch_all)
- return_refs = xcalloc(*nr_heads, sizeof(struct ref *));
- else
- return_refs = NULL;
+ int head_pos = 0, unmatched = 0;
for (ref = *refs; ref; ref = next) {
+ int keep_ref = 0;
next = ref->next;
if (!memcmp(ref->name, "refs/", 5) &&
check_refname_format(ref->name, 0))
; /* trash */
else if (args.fetch_all &&
-(!args.depth || prefixcmp(ref->name, "refs/tags/") )) {
- *newtail = ref;
- ref->next = NULL;
- newtail = &ref->next;
- continue;
- }
- else {
- int cmp = -1;
+ (!args.depth || prefixcmp(ref->name, "refs/tags/")))
+ keep_ref = 1;
+ else
while (head_pos < *nr_heads) {
- cmp = strcmp(ref->name, heads[head_pos]);
- if (cmp < 0) /* definitely do not have it */
+ int cmp = strcmp(ref->name, heads[head_pos]);
+ if (cmp < 0) { /* definitely do not have it */
break;
- else if (cmp == 0) { /* definitely have it */
- return_refs[matched++] = ref;
+ } else if (cmp == 0) { /* definitely have it */
free(heads[head_pos++]);
+ keep_ref = 1;
break;
- }
- else { /* might have it; keep looking */
+ } else { /* might have it; keep looking */
heads[unmatched++] = heads[head_pos++];
}
}
- if (!cmp)
- continue; /* we will link it later */
- }
- free(ref);
- }
-
- if (!args.fetch_all) {
- int i;
- /* copy remaining unmatched heads: */
- while (head_pos < *nr_heads)
- heads[unmatched++] = heads[head_pos++];
- *nr_heads = unmatched;
-
- for (i = 0; i < matched; i++) {
- ref = return_refs[i];
+ if (keep_ref) {
*newtail = ref;
ref->next = NULL;
newtail = &ref->next;
+ } else {
+ free(ref);
}
- free(return_refs);
}
+
+ /* copy any remaining unmatched heads: */
+ while (head_pos < *nr_heads)
+ heads[unmatched++] = heads[head_pos++];
+ *nr_heads = unmatched;
+
*refs = newlist;
}
--
1.7.11.3
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 00/17] Clean up how fetch_pack() handles the heads list
From: Michael Haggerty Re-roll, incorporating Jeff's suggestions. Some commit messages have also been improved, but the only interdiff is that match_pos is renamed to head_pos in filter_refs(). This patch series applies to the merge between master and jc/maint-push-refs-all, though the dependency on the latter is only textual. Michael Haggerty (17): t5500: add tests of error output for missing refs Rename static function fetch_pack() to http_fetch_pack() Fix formatting. Name local variables more consistently Do not check the same head_pos twice Let fetch_pack() inform caller about number of unique heads Pass nr_heads to do_pack_ref() by reference Pass nr_heads to everything_local() by reference Pass nr_heads to filter_refs() by reference Remove ineffective optimization filter_refs(): do not leave gaps in return_refs filter_refs(): compress unmatched refs in heads array cmd_fetch_pack: return early if finish_connect() returns an error Report missing refs even if no existing refs were received cmd_fetch_pack(): simplify computation of return value fetch_pack(): free matching heads filter_refs(): simplify logic builtin/fetch-pack.c | 128 -- fetch-pack.h | 19 +--- http-walker.c | 4 +- t/t5500-fetch-pack.sh | 32 - transport.c | 8 ++-- 5 files changed, 101 insertions(+), 90 deletions(-) -- 1.7.11.3 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 03/17] Fix formatting.
From: Michael Haggerty
Signed-off-by: Michael Haggerty
---
builtin/fetch-pack.c | 8
fetch-pack.h | 12 ++--
2 files changed, 10 insertions(+), 10 deletions(-)
diff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c
index 7912d2b..cc21047 100644
--- a/builtin/fetch-pack.c
+++ b/builtin/fetch-pack.c
@@ -1062,10 +1062,10 @@ static int compare_heads(const void *a, const void *b)
struct ref *fetch_pack(struct fetch_pack_args *my_args,
int fd[], struct child_process *conn,
const struct ref *ref,
- const char *dest,
- int nr_heads,
- char **heads,
- char **pack_lockfile)
+ const char *dest,
+ int nr_heads,
+ char **heads,
+ char **pack_lockfile)
{
struct stat st;
struct ref *ref_cpy;
diff --git a/fetch-pack.h b/fetch-pack.h
index 7c2069c..1dbe90f 100644
--- a/fetch-pack.h
+++ b/fetch-pack.h
@@ -18,11 +18,11 @@ struct fetch_pack_args {
};
struct ref *fetch_pack(struct fetch_pack_args *args,
- int fd[], struct child_process *conn,
- const struct ref *ref,
- const char *dest,
- int nr_heads,
- char **heads,
- char **pack_lockfile);
+ int fd[], struct child_process *conn,
+ const struct ref *ref,
+ const char *dest,
+ int nr_heads,
+ char **heads,
+ char **pack_lockfile);
#endif
--
1.7.11.3
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 05/17] Do not check the same head_pos twice
From: Michael Haggerty
Once a match has been found at head_pos, the entry is zeroed and no
future attempts will match that entry. So increment head_pos to avoid
checking against the zeroed-out entry during the next iteration.
Signed-off-by: Michael Haggerty
---
builtin/fetch-pack.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c
index 76288a2..bda3c0c 100644
--- a/builtin/fetch-pack.c
+++ b/builtin/fetch-pack.c
@@ -561,8 +561,8 @@ static void filter_refs(struct ref **refs, int nr_heads,
char **heads)
if (cmp < 0) /* definitely do not have it */
break;
else if (cmp == 0) { /* definitely have it */
- heads[head_pos][0] = '\0';
return_refs[head_pos] = ref;
+ heads[head_pos++][0] = '\0';
break;
}
else /* might have it; keep looking */
--
1.7.11.3
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 09/17] Pass nr_heads to filter_refs() by reference
From: Michael Haggerty
Signed-off-by: Michael Haggerty
---
builtin/fetch-pack.c | 16
1 file changed, 8 insertions(+), 8 deletions(-)
diff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c
index a4bb0ff..c47090d 100644
--- a/builtin/fetch-pack.c
+++ b/builtin/fetch-pack.c
@@ -521,7 +521,7 @@ static void mark_recent_complete_commits(unsigned long
cutoff)
}
}
-static void filter_refs(struct ref **refs, int nr_heads, char **heads)
+static void filter_refs(struct ref **refs, int *nr_heads, char **heads)
{
struct ref **return_refs;
struct ref *newlist = NULL;
@@ -530,12 +530,12 @@ static void filter_refs(struct ref **refs, int nr_heads,
char **heads)
struct ref *fastarray[32];
int head_pos;
- if (nr_heads && !args.fetch_all) {
- if (ARRAY_SIZE(fastarray) < nr_heads)
- return_refs = xcalloc(nr_heads, sizeof(struct ref *));
+ if (*nr_heads && !args.fetch_all) {
+ if (ARRAY_SIZE(fastarray) < *nr_heads)
+ return_refs = xcalloc(*nr_heads, sizeof(struct ref *));
else {
return_refs = fastarray;
- memset(return_refs, 0, sizeof(struct ref *) * nr_heads);
+ memset(return_refs, 0, sizeof(struct ref *) *
*nr_heads);
}
}
else
@@ -556,7 +556,7 @@ static void filter_refs(struct ref **refs, int nr_heads,
char **heads)
}
else {
int cmp = -1;
- while (head_pos < nr_heads) {
+ while (head_pos < *nr_heads) {
cmp = strcmp(ref->name, heads[head_pos]);
if (cmp < 0) /* definitely do not have it */
break;
@@ -576,7 +576,7 @@ static void filter_refs(struct ref **refs, int nr_heads,
char **heads)
if (!args.fetch_all) {
int i;
- for (i = 0; i < nr_heads; i++) {
+ for (i = 0; i < *nr_heads; i++) {
ref = return_refs[i];
if (ref) {
*newtail = ref;
@@ -646,7 +646,7 @@ static int everything_local(struct ref **refs, int
*nr_heads, char **heads)
}
}
- filter_refs(refs, *nr_heads, heads);
+ filter_refs(refs, nr_heads, heads);
for (retval = 1, ref = *refs; ref ; ref = ref->next) {
const unsigned char *remote = ref->old_sha1;
--
1.7.11.3
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 06/17] Let fetch_pack() inform caller about number of unique heads
From: Michael Haggerty
fetch_pack() removes duplicates from the list (nr_heads, heads),
thereby shrinking the list. But previously, the caller was not
informed about the shrinkage. This would cause a spurious error
message to be emitted by cmd_fetch_pack() if "git fetch-pack" is
called with duplicate refnames.
So change the signature of fetch_pack() to accept nr_heads by
reference, and if any duplicates were removed then modify it to
reflect the number of remaining references.
The last test of t5500 inexplicably *required* "git fetch-pack" to
fail when fetching a list of references that contains duplicates;
i.e., it insisted on the buggy behavior. So change the test to expect
the correct behavior.
Signed-off-by: Michael Haggerty
---
builtin/fetch-pack.c | 12 ++--
fetch-pack.h | 2 +-
t/t5500-fetch-pack.sh | 2 +-
transport.c | 2 +-
4 files changed, 9 insertions(+), 9 deletions(-)
diff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c
index bda3c0c..cf65998 100644
--- a/builtin/fetch-pack.c
+++ b/builtin/fetch-pack.c
@@ -1021,7 +1021,7 @@ int cmd_fetch_pack(int argc, const char **argv, const
char *prefix)
get_remote_heads(fd[0], &ref, 0, NULL);
ref = fetch_pack(&args, fd, conn, ref, dest,
- nr_heads, heads, pack_lockfile_ptr);
+ &nr_heads, heads, pack_lockfile_ptr);
if (pack_lockfile) {
printf("lock %s\n", pack_lockfile);
fflush(stdout);
@@ -1062,7 +1062,7 @@ struct ref *fetch_pack(struct fetch_pack_args *my_args,
int fd[], struct child_process *conn,
const struct ref *ref,
const char *dest,
- int nr_heads,
+ int *nr_heads,
char **heads,
char **pack_lockfile)
{
@@ -1077,16 +1077,16 @@ struct ref *fetch_pack(struct fetch_pack_args *my_args,
st.st_mtime = 0;
}
- if (heads && nr_heads) {
- qsort(heads, nr_heads, sizeof(*heads), compare_heads);
- nr_heads = remove_duplicates(nr_heads, heads);
+ if (heads && *nr_heads) {
+ qsort(heads, *nr_heads, sizeof(*heads), compare_heads);
+ *nr_heads = remove_duplicates(*nr_heads, heads);
}
if (!ref) {
packet_flush(fd[1]);
die("no matching remote head");
}
- ref_cpy = do_fetch_pack(fd, ref, nr_heads, heads, pack_lockfile);
+ ref_cpy = do_fetch_pack(fd, ref, *nr_heads, heads, pack_lockfile);
if (args.depth > 0) {
struct cache_time mtime;
diff --git a/fetch-pack.h b/fetch-pack.h
index 1dbe90f..a9d505b 100644
--- a/fetch-pack.h
+++ b/fetch-pack.h
@@ -21,7 +21,7 @@ struct ref *fetch_pack(struct fetch_pack_args *args,
int fd[], struct child_process *conn,
const struct ref *ref,
const char *dest,
- int nr_heads,
+ int *nr_heads,
char **heads,
char **pack_lockfile);
diff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh
index 3cc3346..0d4edcb 100755
--- a/t/t5500-fetch-pack.sh
+++ b/t/t5500-fetch-pack.sh
@@ -391,7 +391,7 @@ test_expect_success 'fetch mixed refs from cmdline and
stdin' '
test_expect_success 'test duplicate refs from stdin' '
(
cd client &&
- test_must_fail git fetch-pack --stdin --no-progress .. <../input.dup
+ git fetch-pack --stdin --no-progress .. <../input.dup
) >output &&
cut -d " " -f 2 actual &&
test_cmp expect actual
diff --git a/transport.c b/transport.c
index 1811b50..f7bbf58 100644
--- a/transport.c
+++ b/transport.c
@@ -548,7 +548,7 @@ static int fetch_refs_via_pack(struct transport *transport,
refs = fetch_pack(&args, data->fd, data->conn,
refs_tmp ? refs_tmp : transport->remote_refs,
- dest, nr_heads, heads, &transport->pack_lockfile);
+ dest, &nr_heads, heads, &transport->pack_lockfile);
close(data->fd[0]);
close(data->fd[1]);
if (finish_connect(data->conn))
--
1.7.11.3
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 11/17] filter_refs(): do not leave gaps in return_refs
From: Michael Haggerty
It used to be that this function processed refnames in some arbitrary
order but wanted to return them in the order that they were requested,
not the order that they were processed. Now, the refnames are
processed in sorted order, so there is no reason to go to the extra
effort.
Signed-off-by: Michael Haggerty
---
builtin/fetch-pack.c | 15 ++-
1 file changed, 6 insertions(+), 9 deletions(-)
diff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c
index ca1ddd9..a995357 100644
--- a/builtin/fetch-pack.c
+++ b/builtin/fetch-pack.c
@@ -527,14 +527,13 @@ static void filter_refs(struct ref **refs, int *nr_heads,
char **heads)
struct ref *newlist = NULL;
struct ref **newtail = &newlist;
struct ref *ref, *next;
- int head_pos;
+ int head_pos = 0, matched = 0;
if (*nr_heads && !args.fetch_all)
return_refs = xcalloc(*nr_heads, sizeof(struct ref *));
else
return_refs = NULL;
- head_pos = 0;
for (ref = *refs; ref; ref = next) {
next = ref->next;
if (!memcmp(ref->name, "refs/", 5) &&
@@ -554,7 +553,7 @@ static void filter_refs(struct ref **refs, int *nr_heads,
char **heads)
if (cmp < 0) /* definitely do not have it */
break;
else if (cmp == 0) { /* definitely have it */
- return_refs[head_pos] = ref;
+ return_refs[matched++] = ref;
heads[head_pos++][0] = '\0';
break;
}
@@ -569,13 +568,11 @@ static void filter_refs(struct ref **refs, int *nr_heads,
char **heads)
if (!args.fetch_all) {
int i;
- for (i = 0; i < *nr_heads; i++) {
+ for (i = 0; i < matched; i++) {
ref = return_refs[i];
- if (ref) {
- *newtail = ref;
- ref->next = NULL;
- newtail = &ref->next;
- }
+ *newtail = ref;
+ ref->next = NULL;
+ newtail = &ref->next;
}
free(return_refs);
}
--
1.7.11.3
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
