Re: [RFC] On watchman support
On Tue, Nov 18, 2014 at 7:25 AM, David Turner dtur...@twopensource.com wrote: So we got a few options: 1) Convince watchman devs to add something to make it work Based on the thread on the watchman github it looks like this won't happen. Yeah. I came to the conclusion that I needed an extra daemon. And because I would need an extra daemon anyway to speed up index read time, that one could be used for caching something else like watchman. It works out quite nice: because watchman is not tied to the main 'git' binary, people don't need libwatchman and libjansson by default. When people want watchman, they can install an extra package that includes the index-helper and its dependencies. This only matters for binary-based distros of course. Comments? I don't think it would be impossible to add Windows support to watchman; the necessary functions exist, although I don't know how well they work. My experience with watchman is that it is something of a stress test of a filesystem's notification layer. It has exposed bugs in inotify, and caused system instability on OS X. The way i'm adding watchman to index-helper should work on Windows as well, IPC is really simple. But let's wait and see. My patches are not the world's most beautiful, but they do work. I think some improvement might be possible by keeping info about tracked files in the index, and only storing the tree of ignored and untracked files separately. But I have not thought this through fully. In any case, making use of shared memory for the fs_cache (as some of your other patches do for the index) would definitely save time. By the way, what happened to your sse optimization in refs.c? I see it's reverted but I didn't follow closely to know why. Or will you go with cityhash now.. I ask because you have another sse optimization for hashmap on your watchman branch and that could reduce init time for name-hash. Name-hash is used often on case-insensitive fs (less often on case-sensitive fs). I did a simple test and your optimization could init name-hash (on webkit) in 35ms, while unmodified hashmap took 88ms. Loading index on this machine took 360ms for reference (probably down too 100ms with index-helper running, when that 88ms starts to become significant). -- 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 v3 00/14] ref-transactions-reflog
On 11/18/2014 02:35 AM, Stefan Beller wrote: The following patch series updates the reflog handling to use transactions. This patch series has previously been sent to the list[1]. [...] I was reviewing this patch series (I left some comments in Gerrit about the first few patches) when I realized that I'm having trouble understanding the big picture of where you want to go with this. I have the feeling that the operations that you are implementing are at too low a level of abstraction. What are the elementary write operations that are needed for a reflog? Off the top of my head, 1. Add a reflog entry when a reference is updated in a transaction. 2. Rename a reflog file when the corresponding reference is renamed. 3. Delete the reflog when the corresponding reference is deleted [1]. 4. Configure a reference to be reflogged. 5. Configure a reference to not be reflogged anymore and delete any existing reflog. 6. Selectively expire old reflog entries, e.g., based on their age. Have I forgotten any? The first three should be side-effects of the corresponding reference updates. Aside from the fact that renames are not yet done within a transaction, I think this is already the case. Number 4, I think, currently only happens in conjunction with adding a line to the reflog. So it could be implemented, say, as a FORCE_CREATE_REFLOG flag on a ref_update within a transaction. Number 5 is not very interesting, I think. For example, it could be a separate API function, disconnected from any transactions. Number 6 is more interesting, and from my quick reading, it looks like a lot of the work of this patch series is to allow number 6 to be implemented in builtin/reflog.c:expire_reflog(). But it seems to me that you are building API calls at the wrong level of abstraction. Expiring a reflog should be a single API call to the refs API, and ultimately it should be left up to the refs backend to decide how to implement it. For a filesystem-based backend, it would do what it does now. But (for example) a SQL-based backend might implement this as a single SELECT statement. I also don't have the feeling that reflog expiration has to be done within a ref_transaction. For example, is there ever a reason to combine expiration with other reference updates in a single atomic transaction? I think not. So it seems to me that it would be more practical to have a separate API function that is called to expire selected entries from a reflog [2], unconnected with any transaction. I am not nearly as steeped in this code as you and Ronnie, and it could be that I'm forgetting lots of details that make your design preferable. But other reviewers are probably in the same boat. So I think it would be really helpful if you would provide a high-level description of the API that you are proposing, and some discussion of its design and tradeoffs. A big part of this description could go straight into a file Documentation/technical/api-ref-transactions.txt, which will be a great (and necessary) resource soon anyway. Michael [1] Though hopefully there will be future reference backends that don't have to discard reflogs when a reference is deleted, so let's not bake this behavior too fundamentally into the API. [2] ...and/or possibly one to expire reflogs for multiple references, if performance would benefit significantly. -- Michael Haggerty mhag...@alum.mit.edu -- 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] l10n: de.po: translate 2 new messages
Hi Phillip, 2014-11-16 16:54 GMT+01:00 Phillip Sz phillip.sze...@gmail.com: Phillip There's no need for writing your name in the commit message body. Signed-off-by: Phillip Sz phillip.sze...@gmail.com --- po/de.po | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/po/de.po b/po/de.po index c807967..5af3f8f 100644 --- a/po/de.po +++ b/po/de.po @@ -5473,7 +5473,7 @@ msgstr prüft die Reflogs (Standard) #: builtin/fsck.c:613 msgid also consider packs and alternate objects -msgstr +msgstr ziehen Sie außerdem Pakete und alternative Objekte in Betracht Your message has a different style than other messages of this type. I wouldn't address the user directly and would write it as: msgstr ebenso Pakete und alternative Objekte betrachten What do you think? #: builtin/fsck.c:614 msgid enable more strict checking @@ -8253,7 +8253,7 @@ msgstr Referenz muss sich auf dem angegebenen Wert befinden #: builtin/push.c:495 msgid check -msgstr +msgstr Überprüfung Hm. If I understand it correctly, check is one option of the --recurse-submodules parameter for git-push, the other one is on-demand. Since the possible values are either check or on-demand, we shouldn't translate this string at all and should just write check|on-demand as shown in git help push. Perhaps this message need to be adjusted and excluded from translation. I dunno. #: builtin/push.c:496 msgid control recursive pushing of submodules -- 2.1.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 v3 0/1] Don't make $GIT_DIR/config executable
Here is a new, svelte version of the patch to avoid setting the u+x bit on $GIT_DIR/config. Thanks to the many people who reviewed versions v1 [1] and v2 [2]. This time there is no attempt to fix the permissions in existing repositories; it only avoids creating new problems. It also includes a test, as suggested by Eric and sketched by Junio. This version, like the previous versions, applies to maint. But (thanks to its new minimalist nature) this version can be merged to master without conflict. Michael [1] http://thread.gmane.org/gmane.comp.version-control.git/259620 [2] http://thread.gmane.org/gmane.comp.version-control.git/259644 Michael Haggerty (1): create_default_files(): don't set u+x bit on $GIT_DIR/config builtin/init-db.c | 3 ++- t/t0001-init.sh | 7 +++ 2 files changed, 9 insertions(+), 1 deletion(-) -- 2.1.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 v3 1/1] create_default_files(): don't set u+x bit on $GIT_DIR/config
Since time immemorial, the test of whether to set core.filemode has
been done by trying to toggle the u+x bit on $GIT_DIR/config and then
testing whether the change took. I find it somewhat odd to use the
config file for this test, but whatever.
The test code didn't set the u+x bit back to its original state
itself, instead relying on the subsequent call to git_config_set() to
re-write the config file with correct permissions.
But ever since
daa22c6f8d config: preserve config file permissions on edits (2014-05-06)
git_config_set() copies the permissions from the old config file to
the new one. This is a good change in and of itself, but it interacts
badly with create_default_files()'s sloppiness, causing git init to
leave the executable bit set on $GIT_DIR/config.
So change create_default_files() to reset the permissions on
$GIT_DIR/config after its test.
Signed-off-by: Michael Haggerty mhag...@alum.mit.edu
---
builtin/init-db.c | 3 ++-
t/t0001-init.sh | 7 +++
2 files changed, 9 insertions(+), 1 deletion(-)
diff --git a/builtin/init-db.c b/builtin/init-db.c
index 56f85e2..a6d58fd 100644
--- a/builtin/init-db.c
+++ b/builtin/init-db.c
@@ -254,7 +254,8 @@ static int create_default_files(const char *template_path)
struct stat st2;
filemode = (!chmod(path, st1.st_mode ^ S_IXUSR)
!lstat(path, st2)
- st1.st_mode != st2.st_mode);
+ st1.st_mode != st2.st_mode
+ !chmod(path, st1.st_mode));
}
git_config_set(core.filemode, filemode ? true : false);
diff --git a/t/t0001-init.sh b/t/t0001-init.sh
index e62c0ff..7de8d85 100755
--- a/t/t0001-init.sh
+++ b/t/t0001-init.sh
@@ -12,6 +12,13 @@ check_config () {
echo expected a directory $1, a file $1/config and $1/refs
return 1
fi
+
+ if test_have_prereq POSIXPERM test -x $1/config
+ then
+ echo $1/config is executable?
+ return 1
+ fi
+
bare=$(cd $1 git config --bool core.bare)
worktree=$(cd $1 git config core.worktree) ||
worktree=unset
--
2.1.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
Re: [PATCH] l10n: de.po: translate 2 new messages
Hi, There's no need for writing your name in the commit message body. I know, it was a mistake. Your message has a different style than other messages of this type. I wouldn't address the user directly and would write it as: msgstr ebenso Pakete und alternative Objekte betrachten What do you think? Yeah, I must learn that. You're suggestion is okey then. Hm. If I understand it correctly, check is one option of the --recurse-submodules parameter for git-push, the other one is on-demand. Since the possible values are either check or on-demand, we shouldn't translate this string at all and should just write check|on-demand as shown in git help push. Perhaps this message need to be adjusted and excluded from translation. I dunno. Okey. Thanks for your reply. Phillip -- 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] copy.c: make copy_fd preserve meaningful errno
Jonathan Nieder jrnie...@gmail.com writes: Do any callers care about errno? Does the function's API documentation say it will make errno meaningful on error, so people making changes to copy_fd in the future know to maintain that property? *searches* Looks like callers are: convert.c::filter_buffer_or_fd, which doesn't care copy.c::copy_file, which also doesn't care lockfile.c::hold_lock_file_for_append, which started caring in order to propagate errno in v2.2.0-rc0~53^2~2 (restore errno before returning, 2014-10-01). But no callers of that function care yet. So this is about fixing a bug-waiting-to-happen in hold_lock_file_for_append. That would be enough to motivate the change. OK. Perhaps convert.c wants to be fixed? +int save_errno = errno; +error(copy-fd: read returned %s, strerror(errno)); +errno = save_errno; +return -1; Any caller is presumably going to turn around and print strerror(errno) again, producing repetitive output. Can we do better? E.g., if the signature were int copy_fd(int ifd, int ofd, struct strbuf *err); then we could write the error message to the err strbuf for the caller to print. The problem you are solving is because the caller may want to do its own error message, stop the callee from emitting the error message unconditionally, but if we are addressing the caller may want to..., I think we should find a single solution that addresses other kind fo things the caller may want to do. For example, two callers may want to phrase the same error condition differently, e.g. depending on what the user asked to do. We'd want something better than the ERRORMSG trick used in unpack-trees.c, which does not scale, and I think passing some data that represents here is how the caller wants the errors to be handled and presented is probably a part of the solution, but strbuf *err is not that. In short I am not a very big fan of passing around strbuf *err to low level helper API functions myself. But the approach does not make things much worse than it currently is, other than code churns to pass an extra pointer around. -- 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] Introduce a hook to run after formatting patches
Stefan Beller sbel...@google.com writes: Do we have similar filters somewhere in place already, so I could have a look at the code architecture, the api, and how the user would operate that? The clean/smudge filters interacts with the payload data and the end user configuration in a similar way, I would say. The way you're proposing, doesn't sound as if a hook would be the right thing for such filtering. That depends on how you define what a hook is, I think. The one big thing I liked over the first patch in this thread was the 'maintainability', i.e. if it were a hook, I could set that up and forget about it. No need to change my behavior in using git, but still I have the desired postprocessed results. In the message you are responding, I said detect a request to use, exactly because I wanted to leave it up to you to design what form(s) that request detection takes. One of the forms could be a script with this $filename exists in $GIT_DIR/, and the $filname may be hooks/format-patch-redaction-filter. Of course, any configured into repository solution must have a command line override, so the order you would develop this would be: (0) make the code that interracts with the filter if given by the user work, without worrying about how the user specifies the filter. (1) add a --command-line-option=filter-name to trigger the code you wrote in (0) above. (2) add a --no-command-line-option to defeat the configured filter, without worrying about how the user configures. For your new command line option --frotz, git cmd --frotz --no-frotz should make your cmd refrain from doing frotz. (3) add configuration variable to point at a filter script, e.g. format.redactionFilter; you must make sure that this is disabled with --no-* you added in (2) above; (4) perhaps add support for hooks/format-patch-redaction-filter, if you strongly feel like it. The user must be able to disable this with the same --no-* added in (2). I'd say (4) is optional; by the time you reach (3), you already have the same write once and forget capability. -- 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] copy.c: make copy_fd preserve meaningful errno
Junio C Hamano gits...@pobox.com writes: In short I am not a very big fan of passing around strbuf *err to low level helper API functions myself. But the approach does not make things much worse than it currently is, other than code churns to pass an extra pointer around. Sorry I left the conclusion out of the message. As it does not make things much worse, and does give slightly better flexibility on error message emission to the callers, let's go with the strbuf *err arpporach for now. Until we hit a wall we cannot climb over, at which time we may need to redo it, but let's first see how it goes. -- 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] gitweb: hack around CGI's list-context param() handling
As of CGI.pm's 4.08 release, the behavior to call
CGI::param() in a list context is deprecated (because it can
be potentially unsafe if called inside a hash constructor).
This cause gitweb to issue a warning for some of our code,
which in turn causes the tests to fail.
Our use is in fact _not_ one of the dangerous cases, as we
are intentionally using a list context. The recommended
route by 4.08 is to use the new CGI::multi_param() call to
make it explicit that we know what we are doing.
However, that function is only available in 4.08, which is
about a month old; we cannot rely on having it.
One option would be to set $CGI::LIST_CONTEXT_WARN globally,
which turns off the warning. However, that would eliminate
the protection these newer releases are trying to provide.
We want to annotate each site as OK using the new function.
So instead, let's check whether CGI provides the
multi_param() function, and if not, provide an
implementation that just wraps param(). That will work on
both old and new versions of CGI. Sadly, we cannot just
check defined(\CGI::multi_param), because CGI uses the
autoload feature, which claims that all functions are
defined. Instead, we just do a version check.
Signed-off-by: Jeff King p...@peff.net
---
Without this patch, all of the gitweb tests consistently fail on Debian
testing/unstable when you have libcgi-pm-perl installed (it works
without that package installed, because an older version of CGI.pm is in
the perl base). I tested with both versions.
Another approach would be to live with the warning, but teach the tests
to be less meticulous. I think it probably makes sense to keep them
pedantic, though, because it can catch potential errors.
gitweb/gitweb.perl | 6 +-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index ccf7516..7a5b23a 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -20,6 +20,10 @@ use File::Basename qw(basename);
use Time::HiRes qw(gettimeofday tv_interval);
binmode STDOUT, ':utf8';
+if (!defined($CGI::VERSION) || $CGI::VERSION 4.08) {
+ eval 'sub CGI::multi_param { CGI::param(@_) }'
+}
+
our $t0 = [ gettimeofday() ];
our $number_of_git_cmds = 0;
@@ -871,7 +875,7 @@ sub evaluate_query_params {
while (my ($name, $symbol) = each %cgi_param_mapping) {
if ($symbol eq 'opt') {
- $input_params{$name} = [ map { decode_utf8($_) }
$cgi-param($symbol) ];
+ $input_params{$name} = [ map { decode_utf8($_) }
$cgi-multi_param($symbol) ];
} else {
$input_params{$name} =
decode_utf8($cgi-param($symbol));
}
--
2.1.2.596.g7379948
--
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 v3 1/1] create_default_files(): don't set u+x bit on $GIT_DIR/config
Reviewed-by: Stefan Beller sbel...@google.com
On Tue, Nov 18, 2014 at 5:50 AM, Michael Haggerty mhag...@alum.mit.edu wrote:
Since time immemorial, the test of whether to set core.filemode has
been done by trying to toggle the u+x bit on $GIT_DIR/config and then
testing whether the change took. I find it somewhat odd to use the
config file for this test, but whatever.
The test code didn't set the u+x bit back to its original state
itself, instead relying on the subsequent call to git_config_set() to
re-write the config file with correct permissions.
But ever since
daa22c6f8d config: preserve config file permissions on edits (2014-05-06)
git_config_set() copies the permissions from the old config file to
the new one. This is a good change in and of itself, but it interacts
badly with create_default_files()'s sloppiness, causing git init to
leave the executable bit set on $GIT_DIR/config.
So change create_default_files() to reset the permissions on
$GIT_DIR/config after its test.
Signed-off-by: Michael Haggerty mhag...@alum.mit.edu
---
builtin/init-db.c | 3 ++-
t/t0001-init.sh | 7 +++
2 files changed, 9 insertions(+), 1 deletion(-)
diff --git a/builtin/init-db.c b/builtin/init-db.c
index 56f85e2..a6d58fd 100644
--- a/builtin/init-db.c
+++ b/builtin/init-db.c
@@ -254,7 +254,8 @@ static int create_default_files(const char *template_path)
struct stat st2;
filemode = (!chmod(path, st1.st_mode ^ S_IXUSR)
!lstat(path, st2)
- st1.st_mode != st2.st_mode);
+ st1.st_mode != st2.st_mode
+ !chmod(path, st1.st_mode));
}
git_config_set(core.filemode, filemode ? true : false);
diff --git a/t/t0001-init.sh b/t/t0001-init.sh
index e62c0ff..7de8d85 100755
--- a/t/t0001-init.sh
+++ b/t/t0001-init.sh
@@ -12,6 +12,13 @@ check_config () {
echo expected a directory $1, a file $1/config and $1/refs
return 1
fi
+
+ if test_have_prereq POSIXPERM test -x $1/config
+ then
+ echo $1/config is executable?
+ return 1
+ fi
+
bare=$(cd $1 git config --bool core.bare)
worktree=$(cd $1 git config core.worktree) ||
worktree=unset
--
2.1.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
--
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] t0090: mark add-interactive test with PERL prerequisite
The add-interactive system is built in perl. If you build
with NO_PERL, running git commit --interactive will exit
with an error and the test will fail.
Signed-off-by: Jeff King p...@peff.net
---
Noticed by Michael while working around gitweb failures by setting
NO_PERL. :)
It didn't reproduce for me in my existing build directory, presumably
because I had an old git-add--interactive build product lying around.
But running the tests in a clean clone with NO_PERL set reproduces
easily.
t/t0090-cache-tree.sh | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/t/t0090-cache-tree.sh b/t/t0090-cache-tree.sh
index 158cf4f..067f4c6 100755
--- a/t/t0090-cache-tree.sh
+++ b/t/t0090-cache-tree.sh
@@ -131,7 +131,7 @@ test_expect_success 'second commit has cache-tree' '
test_cache_tree
'
-test_expect_success 'commit --interactive gives cache-tree on partial commit' '
+test_expect_success PERL 'commit --interactive gives cache-tree on partial
commit' '
cat -\EOT foo.c
int foo()
{
--
2.1.2.596.g7379948
--
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] t960[34]: mark cvsimport tests as requiring perl
Git-cvsimport is written in perl, which understandably causes the tests to fail if you build with NO_PERL (which will avoid building cvsimport at all). The earlier cvsimport tests in t9600-t9602 are all marked with a PERL prerequisite, but these ones are not. The one in t9603 was likely not noticed because it is an expected failure anyway. The ones in t9604 have been around for a long time, but it is likely that the combination of NO_PERL and having cvsps installed is rare enough that nobody noticed. Signed-off-by: Jeff King p...@peff.net --- It would probably make sense to have these scripts just skip_all if NO_PERL is set, but I opted to follow the pattern set by t9600, etc. If somebody feels like spending time refactoring the cvsimport test harness, be my guest. t/t9603-cvsimport-patchsets.sh | 2 +- t/t9604-cvsimport-timestamps.sh | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/t/t9603-cvsimport-patchsets.sh b/t/t9603-cvsimport-patchsets.sh index 52034c8..c4c3c49 100755 --- a/t/t9603-cvsimport-patchsets.sh +++ b/t/t9603-cvsimport-patchsets.sh @@ -16,7 +16,7 @@ test_description='git cvsimport testing for correct patchset estimation' setup_cvs_test_repository t9603 -test_expect_failure 'import with criss cross times on revisions' ' +test_expect_failure PERL 'import with criss cross times on revisions' ' git cvsimport -p-x -C module-git module (cd module-git diff --git a/t/t9604-cvsimport-timestamps.sh b/t/t9604-cvsimport-timestamps.sh index 1fd5142..a4b3db2 100755 --- a/t/t9604-cvsimport-timestamps.sh +++ b/t/t9604-cvsimport-timestamps.sh @@ -5,7 +5,7 @@ test_description='git cvsimport timestamps' setup_cvs_test_repository t9604 -test_expect_success 'check timestamps are UTC (TZ=CST6CDT)' ' +test_expect_success PERL 'check timestamps are UTC (TZ=CST6CDT)' ' TZ=CST6CDT git cvsimport -p-x -C module-1 module git cvsimport -p-x -C module-1 module @@ -34,7 +34,7 @@ test_expect_success 'check timestamps are UTC (TZ=CST6CDT)' ' test_cmp actual-1 expect-1 ' -test_expect_success 'check timestamps with author-specific timezones' ' +test_expect_success PERL 'check timestamps with author-specific timezones' ' cat cvs-authors -EOF user1=User One us...@domain.org -- 2.1.2.596.g7379948 -- 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] t0090: mark add-interactive test with PERL prerequisite
On Tue, Nov 18, 2014 at 12:22:31PM -0500, Jeff King wrote: The add-interactive system is built in perl. If you build with NO_PERL, running git commit --interactive will exit with an error and the test will fail. Signed-off-by: Jeff King p...@peff.net --- Noticed by Michael while working around gitweb failures by setting NO_PERL. :) It didn't reproduce for me in my existing build directory, presumably because I had an old git-add--interactive build product lying around. But running the tests in a clean clone with NO_PERL set reproduces easily. I don't think fixing this is really a high priority. You can't ever be free of odd interactions with previous build artifacts. After all, you might have a built git-foo from a previous version of git (or one from the future, or even an alternate reality from another branch), and our Makefile should not have to know about every previous version you may have built in the path. That is what git clean is for (or just using a clean build directory). But fixing this one in particular is pretty easy (and we _do_ know about the wrongly-built file; our dependencies are just incomplete): -- 8 -- Subject: Makefile: have perl scripts depend on NO_PERL setting If NO_PERL is not set, our perl scripts are built as usual. If it is set, then we build dummy versions that tell you git was built without perl support and exit gracefully. However, if you switch to NO_PERL in a directory with existing build artifacts, we do not notice that the files need rebuilt. We see only that they are newer than the unimplemented.sh wrapper and assume they are done. So doing: make make NO_PERL=Nope would result in a git-add--interactive script that uses perl (and running the test suite would make use of it). Instead, we should trigger a rebuild of the perl scripts anytime NO_PERL changes. Signed-off-by: Jeff King p...@peff.net --- Makefile | 3 +++ 1 file changed, 3 insertions(+) diff --git a/Makefile b/Makefile index 827006b..0fa02ff 100644 --- a/Makefile +++ b/Makefile @@ -1676,6 +1676,9 @@ git.res: git.rc GIT-VERSION-FILE $(join -DMAJOR= -DMINOR=, $(wordlist 1,2,$(subst -, ,$(subst ., ,$(GIT_VERSION) \ -DGIT_VERSION=\\\$(GIT_VERSION)\\\ $ -o $@ +# This makes sure we depend on the NO_PERL setting itself. +$(patsubst %.perl,%,$(SCRIPT_PERL)): GIT-BUILD-OPTIONS + ifndef NO_PERL $(patsubst %.perl,%,$(SCRIPT_PERL)): perl/perl.mak -- 2.1.2.596.g7379948 -- 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] builtin/push.c: fix description of --recurse-submodules option
The description of the option for argument recurse-submodules
is marked for translation even if it expects the untranslated
string and it's missing the option on-demand which was introduced
in eb21c73 (2014-03-29, push: teach --recurse-submodules the on-demand
option). Fix this by unmark the string for translation and add the
missing option.
Signed-off-by: Ralf Thielow ralf.thie...@gmail.com
---
builtin/push.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/builtin/push.c b/builtin/push.c
index a076b19..cfa20c2 100644
--- a/builtin/push.c
+++ b/builtin/push.c
@@ -503,7 +503,7 @@ int cmd_push(int argc, const char **argv, const char
*prefix)
0, CAS_OPT_NAME, cas, N_(refname:expect),
N_(require old value of ref to be at this value),
PARSE_OPT_OPTARG, parseopt_push_cas_option },
- { OPTION_CALLBACK, 0, recurse-submodules, flags, N_(check),
+ { OPTION_CALLBACK, 0, recurse-submodules, flags,
check|on-demand,
N_(control recursive pushing of submodules),
PARSE_OPT_OPTARG, option_parse_recurse_submodules },
OPT_BOOL( 0 , thin, thin, N_(use thin pack)),
--
2.2.0.rc2.258.gc851c5b
--
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: ag, **, and the GPL
On Mon, 2014-11-17 at 20:50 -0800, Matthew Kaniaris wrote: The Silver Search (https://github.com/ggreer/the_silver_searcher), is a small, open source, cross platform searching utility written as a replacement for ack. One of the major benefits of Ag (and a source for much of its speed) is that it obeys .gitignore. However, Ag currently treats gitignores as regexs which produces incorrect results for e.g. **. I'd like to add support to ag to obey the .gitignore spec but I'm not keen on implementing yet another fnmatch clone. Ag is licensed under the Apache License Version 2.0 which to the best of my understanding is incompatible with the GPLv2. Would you grant me permission to reuse wildmatch.c (and necessary includes) for use in Ag? I already implemented this without using any git code at https://github.com/novalis/the_silver_searcher. The patch was rejected because it slowed down ag slightly (or perhaps because it was overly complex). -- 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] gitweb: hack around CGI's list-context param() handling
Jeff King p...@peff.net writes:
As of CGI.pm's 4.08 release, the behavior to call
CGI::param() in a list context is deprecated (because it can
be potentially unsafe if called inside a hash constructor).
This cause gitweb to issue a warning for some of our code,
which in turn causes the tests to fail.
Our use is in fact _not_ one of the dangerous cases, as we
are intentionally using a list context. The recommended
route by 4.08 is to use the new CGI::multi_param() call to
make it explicit that we know what we are doing.
However, that function is only available in 4.08, which is
about a month old; we cannot rely on having it.
One option would be to set $CGI::LIST_CONTEXT_WARN globally,
which turns off the warning. However, that would eliminate
the protection these newer releases are trying to provide.
We want to annotate each site as OK using the new function.
So instead, let's check whether CGI provides the
multi_param() function, and if not, provide an
implementation that just wraps param(). That will work on
both old and new versions of CGI. Sadly, we cannot just
check defined(\CGI::multi_param), because CGI uses the
autoload feature, which claims that all functions are
defined. Instead, we just do a version check.
The solution does look like the best in the messy situation. Thanks
for looking into it.
+if (!defined($CGI::VERSION) || $CGI::VERSION 4.08) {
+ eval 'sub CGI::multi_param { CGI::param(@_) }'
+}
+
our $t0 = [ gettimeofday() ];
our $number_of_git_cmds = 0;
@@ -871,7 +875,7 @@ sub evaluate_query_params {
while (my ($name, $symbol) = each %cgi_param_mapping) {
if ($symbol eq 'opt') {
- $input_params{$name} = [ map { decode_utf8($_) }
$cgi-param($symbol) ];
+ $input_params{$name} = [ map { decode_utf8($_) }
$cgi-multi_param($symbol) ];
} else {
$input_params{$name} =
decode_utf8($cgi-param($symbol));
}
--
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] gitweb: hack around CGI's list-context param() handling
On Tue, Nov 18, 2014 at 09:58:26AM -0800, Junio C Hamano wrote: Jeff King p...@peff.net writes: As of CGI.pm's 4.08 release, the behavior to call CGI::param() in a list context is deprecated (because it can be potentially unsafe if called inside a hash constructor). This cause gitweb to issue a warning for some of our code, Oops, s/cause/causes/ of course. -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
[PATCH] l10n: de.po: translate 2 new messages
Signed-off-by: Phillip Sz phillip.sze...@gmail.com Signed-off-by: Ralf Thielow ralf.thie...@gmail.com --- po/de.po | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/po/de.po b/po/de.po index 0597cb2..5ad705b 100644 --- a/po/de.po +++ b/po/de.po @@ -5638,7 +5638,7 @@ msgstr die Reflogs prüfen (Standard) #: builtin/fsck.c:615 msgid also consider packs and alternate objects -msgstr +msgstr ebenso Pakete und alternative Objekte betrachten #: builtin/fsck.c:616 msgid enable more strict checking @@ -8460,7 +8460,7 @@ msgstr Referenz muss sich auf dem angegebenen Wert befinden #: builtin/push.c:495 msgid check -msgstr +msgstr check|on-demand #: builtin/push.c:496 msgid control recursive pushing of submodules -- 2.2.0.rc2.258.gc851c5b -- 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] On watchman support
On Tue, 2014-11-18 at 17:48 +0700, Duy Nguyen wrote: My patches are not the world's most beautiful, but they do work. I think some improvement might be possible by keeping info about tracked files in the index, and only storing the tree of ignored and untracked files separately. But I have not thought this through fully. In any case, making use of shared memory for the fs_cache (as some of your other patches do for the index) would definitely save time. By the way, what happened to your sse optimization in refs.c? I see it's reverted but I didn't follow closely to know why. I don't know why either -- it works just fine. There was a bug, but I fixed it. Junio? Or will you go with cityhash now.. I ask because you have another sse optimization for hashmap on your watchman branch and that could reduce init time for name-hash. Name-hash is used often on case-insensitive fs (less often on case-sensitive fs). Cityhash would be better, because it has actual engineering effort put into it; what I did on my branch is a hack that happens to work decently. As the comment notes, I did not spend much effort on tuning my implementation. Also, Cityhash doesn't require SSE, so it's more portable. I did a simple test and your optimization could init name-hash (on webkit) in 35ms, while unmodified hashmap took 88ms. Loading index on this machine took 360ms for reference (probably down too 100ms with index-helper running, when that 88ms starts to become significant). OK, that sounds like a big win. -- 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
Fix Penguin Penalty 17th October2014 ( mail-archive.com )
Dear Sir Did your website get hit by Google Penguin update on October 17th 2014? What basically is Google Penguin Update? It is actually a code name for Google algorithm which aims at decreasing your websites search engine rankings that violate Googles guidelines by using black hat SEO techniques to rank your webpage by giving number of spammy links to the page. We are one of those few SEO companies that can help you avoid penalties from Google Updates like Penguin and Panda. Our clients have survived all the previous and present updates with ease. They have never been hit because we use 100% white hat SEO techniques to rank Webpages. Simple thing that we do to keep websites away from any Penguin or Panda penalties is follow Google guidelines and we give Google users the best answers to their queries. If you are looking to increase the quality of your websites and to get more targeted traffic or save your websites from these Google penalties email us back with your interest. We will be glad to serve you and help you grow your business. Regards Anamika Sehgal SEO Manager ( TOB ) B7 Green Avenue, Amritsar 143001 Punjab NO CLICK in the subject to STOP EMAILS -- 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 v3 00/14] ref-transactions-reflog
On Tue, Nov 18, 2014 at 3:26 AM, Michael Haggerty mhag...@alum.mit.edu wrote: On 11/18/2014 02:35 AM, Stefan Beller wrote: The following patch series updates the reflog handling to use transactions. This patch series has previously been sent to the list[1]. [...] I was reviewing this patch series (I left some comments in Gerrit about the first few patches) when I realized that I'm having trouble understanding the big picture of where you want to go with this. I have the feeling that the operations that you are implementing are at too low a level of abstraction. What are the elementary write operations that are needed for a reflog? Off the top of my head, 1. Add a reflog entry when a reference is updated in a transaction. 2. Rename a reflog file when the corresponding reference is renamed. 3. Delete the reflog when the corresponding reference is deleted [1]. 4. Configure a reference to be reflogged. 5. Configure a reference to not be reflogged anymore and delete any existing reflog. 6. Selectively expire old reflog entries, e.g., based on their age. Have I forgotten any? The first three should be side-effects of the corresponding reference updates. Aside from the fact that renames are not yet done within a transaction, I think this is already the case. Number 4, I think, currently only happens in conjunction with adding a line to the reflog. So it could be implemented, say, as a FORCE_CREATE_REFLOG flag on a ref_update within a transaction. Number 5 is not very interesting, I think. For example, it could be a separate API function, disconnected from any transactions. Number 6 is more interesting, and from my quick reading, it looks like a lot of the work of this patch series is to allow number 6 to be implemented in builtin/reflog.c:expire_reflog(). But it seems to me that you are building API calls at the wrong level of abstraction. Expiring a reflog should be a single API call to the refs API, and ultimately it should be left up to the refs backend to decide how to implement it. For a filesystem-based backend, it would do what it does now. But (for example) a SQL-based backend might implement this as a single SELECT statement. I agree in principle. But things are more difficult since expire_reflog() has very complex semantics. To keep things simple for the reviews at this stage the logic is the same as the original code: loop over all entries: use very complex conditionals to decide which entries to keep/remove optionally modify the sha1 values for the records we keep write records we keep back to the file one record at a time So that as far as possible, we keep the same rules and behavior but we use a different API for the actual write entry to new reflog. We could wrap this inside a new specific transaction_expire_reflog() function so that other types of backends, for example an SQL backend, could optimize, but I think that should be in a separate later patch because expire_reflog is almost impossibly complex. It will not be a simple SELECT unfortunately. The current expire logic is something like : 1, expire all entries older than timestamp 2, optionally, also expire all entries that refer to unreachable objects using a different timestamp This involves actually reading the objects that the sha1 points to and parsing them! 3, optionally, if the sha1 objects can not be referenced, they are not commit objects or if they don't exist, then expire them too. This also involves reading the objects behind the sha1. 4, optionally, delete reflog entry #foo 5, optionally, if any log entries were discarded due to 2,3,4 then we might also re-write and modify some of the reflog entries we keep. or any combination thereof (6, if --dry-run is specified, just print what we would have expired) 2 and 3 requires that we need to read the objects for the entry 4 allows us to delete a specific entry 5 means that even for entries we keep we will need to mutate them. I also don't have the feeling that reflog expiration has to be done within a ref_transaction. For example, is there ever a reason to combine expiration with other reference updates in a single atomic transaction? --updateref In expire_reflog() we not only prune the reflog. When --updateref is used we update the actual ref itself. I think we want to have both the ref update and also the reflog update both be part of a single atomic transaction. I think not. So it seems to me that it would be more practical to have a separate API function that is called to expire selected entries from a reflog [2], unconnected with any transaction. I think it makes the API cleaner if we have a 'you can only update a ref/reflog/other things added in the future/ from within a transaction.' Since we need to do reflog changes within a transaction for the expire reflog case as well as the rename ref case I think it makes sense to enforce that reflog changes must be done
Re: [PATCH] t0090: mark add-interactive test with PERL prerequisite
Jeff King wrote:
Subject: Makefile: have perl scripts depend on NO_PERL setting
[...]
---
Makefile | 3 +++
1 file changed, 3 insertions(+)
Gah. Good catch.
Reviewed-by: Jonathan Nieder jrnie...@gmail.com
[...]
--- a/Makefile
+++ b/Makefile
@@ -1676,6 +1676,9 @@ git.res: git.rc GIT-VERSION-FILE
$(join -DMAJOR= -DMINOR=, $(wordlist 1,2,$(subst -, ,$(subst .,
,$(GIT_VERSION) \
-DGIT_VERSION=\\\$(GIT_VERSION)\\\ $ -o $@
+# This makes sure we depend on the NO_PERL setting itself.
+$(patsubst %.perl,%,$(SCRIPT_PERL)): GIT-BUILD-OPTIONS
+
ifndef NO_PERL
$(patsubst %.perl,%,$(SCRIPT_PERL)): perl/perl.mak
Why do these repeat the 'patsubst ...' expression instead of using
SCRIPT_PERL_GEN, by the way?
-- 8 --
Subject: Makefile: simplify by using SCRIPT_{PERL,SH}_GEN macros
SCRIPT_PERL_GEN is defined as $(patsubst %.perl,%,$(SCRIPT_PERL)) for
use in targets like build-perl-script used by makefiles in
subdirectories that override SCRIPT_PERL (see v1.8.2-rc0~17^2,
git-remote-mediawiki: use toplevel's Makefile, 2013-02-08).
The same expression is used in the rules that actually write the
generated perl scripts, and since this rules were introduced before
SCRIPT_PERL_GEN, they use the longhand instead of that macro. Use the
macro to make reading easier.
Likewise for SCRIPT_SH_GEN. The Python rules already got the same
simplification in v1.8.4-rc0~162^2~8 (2013-05-24).
Signed-off-by: Jonathan Nieder jrnie...@gmail.com
---
Makefile | 10 +-
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/Makefile b/Makefile
index 0fa02ff..8f980e0 100644
--- a/Makefile
+++ b/Makefile
@@ -1662,7 +1662,7 @@ GIT-SCRIPT-DEFINES: FORCE
fi
-$(patsubst %.sh,%,$(SCRIPT_SH)) : % : %.sh GIT-SCRIPT-DEFINES
+$(SCRIPT_SH_GEN) : % : %.sh GIT-SCRIPT-DEFINES
$(QUIET_GEN)$(cmd_munge_script) \
chmod +x $@+ \
mv $@+ $@
@@ -1677,10 +1677,10 @@ git.res: git.rc GIT-VERSION-FILE
-DGIT_VERSION=\\\$(GIT_VERSION)\\\ $ -o $@
# This makes sure we depend on the NO_PERL setting itself.
-$(patsubst %.perl,%,$(SCRIPT_PERL)): GIT-BUILD-OPTIONS
+$(SCRIPT_PERL_GEN): GIT-BUILD-OPTIONS
ifndef NO_PERL
-$(patsubst %.perl,%,$(SCRIPT_PERL)): perl/perl.mak
+$(SCRIPT_PERL_GEN): perl/perl.mak
perl/perl.mak: perl/PM.stamp
@@ -1693,7 +1693,7 @@ perl/perl.mak: GIT-CFLAGS GIT-PREFIX perl/Makefile
perl/Makefile.PL
$(QUIET_SUBDIR0)perl $(QUIET_SUBDIR1) PERL_PATH='$(PERL_PATH_SQ)'
prefix='$(prefix_SQ)' $(@F)
PERL_DEFINES = $(PERL_PATH_SQ):$(PERLLIB_EXTRA_SQ)
-$(patsubst %.perl,%,$(SCRIPT_PERL)): % : %.perl perl/perl.mak GIT-PERL-DEFINES
GIT-VERSION-FILE
+$(SCRIPT_PERL_GEN): % : %.perl perl/perl.mak GIT-PERL-DEFINES GIT-VERSION-FILE
$(QUIET_GEN)$(RM) $@ $@+ \
INSTLIBDIR=`MAKEFLAGS= $(MAKE) -C perl -s --no-print-directory
instlibdir` \
INSTLIBDIR_EXTRA='$(PERLLIB_EXTRA_SQ)' \
@@ -1727,7 +1727,7 @@ git-instaweb: git-instaweb.sh gitweb GIT-SCRIPT-DEFINES
chmod +x $@+ \
mv $@+ $@
else # NO_PERL
-$(patsubst %.perl,%,$(SCRIPT_PERL)) git-instaweb: % : unimplemented.sh
+$(SCRIPT_PERL_GEN) git-instaweb: % : unimplemented.sh
$(QUIET_GEN)$(RM) $@ $@+ \
sed -e '1s|#!.*/sh|#!$(SHELL_PATH_SQ)|' \
-e 's|@@REASON@@|NO_PERL=$(NO_PERL)|g' \
--
2.1.0.rc2.206.gedb03e5
--
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] t0090: mark add-interactive test with PERL prerequisite
Jonathan Nieder wrote: Jeff King wrote: Subject: Makefile: have perl scripts depend on NO_PERL setting [...] --- Makefile | 3 +++ 1 file changed, 3 insertions(+) Gah. Good catch. Reviewed-by: Jonathan Nieder jrnie...@gmail.com ... and here's a patch on top to give git-p4 the same treatment. -- 8 -- Subject: Makefile: have python scripts depend on NO_PYTHON setting Like the perl scripts, python scripts need a dependency to ensure they are rebuilt when switching between the dummy versions that run without Python and the real thing. Signed-off-by: Jonathan Nieder jrnie...@gmail.com --- Makefile | 3 +++ 1 file changed, 3 insertions(+) diff --git a/Makefile b/Makefile index 8f980e0..7482a4d 100644 --- a/Makefile +++ b/Makefile @@ -1736,6 +1736,9 @@ $(SCRIPT_PERL_GEN) git-instaweb: % : unimplemented.sh mv $@+ $@ endif # NO_PERL +# This makes sure we depend on the NO_PYTHON setting itself. +$(SCRIPT_PYTHON_GEN): GIT-BUILD-OPTIONS + ifndef NO_PYTHON $(SCRIPT_PYTHON_GEN): GIT-CFLAGS GIT-PREFIX GIT-PYTHON-VARS $(SCRIPT_PYTHON_GEN): % : %.py -- 2.1.0.rc2.206.gedb03e5 -- 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] t0090: mark add-interactive test with PERL prerequisite
On Tue, Nov 18, 2014 at 10:38:38AM -0800, Jonathan Nieder wrote:
Reviewed-by: Jonathan Nieder jrnie...@gmail.com
Thanks.
+# This makes sure we depend on the NO_PERL setting itself.
+$(patsubst %.perl,%,$(SCRIPT_PERL)): GIT-BUILD-OPTIONS
+
ifndef NO_PERL
$(patsubst %.perl,%,$(SCRIPT_PERL)): perl/perl.mak
Why do these repeat the 'patsubst ...' expression instead of using
SCRIPT_PERL_GEN, by the way?
Dunno. I just cargo-culted from the context lines.
-- 8 --
Subject: Makefile: simplify by using SCRIPT_{PERL,SH}_GEN macros
SCRIPT_PERL_GEN is defined as $(patsubst %.perl,%,$(SCRIPT_PERL)) for
use in targets like build-perl-script used by makefiles in
subdirectories that override SCRIPT_PERL (see v1.8.2-rc0~17^2,
git-remote-mediawiki: use toplevel's Makefile, 2013-02-08).
The same expression is used in the rules that actually write the
generated perl scripts, and since this rules were introduced before
SCRIPT_PERL_GEN, they use the longhand instead of that macro. Use the
macro to make reading easier.
Likewise for SCRIPT_SH_GEN. The Python rules already got the same
simplification in v1.8.4-rc0~162^2~8 (2013-05-24).
This makes sense, and looking over the Makefile, I don't see how it
could cause any bad side effects.
Minor nit:
s/this rules/these rules/
in your commit message. Otherwise:
Reviewed-by: Jeff King p...@peff.net
-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] t0090: mark add-interactive test with PERL prerequisite
On Tue, Nov 18, 2014 at 10:43:47AM -0800, Jonathan Nieder wrote: ... and here's a patch on top to give git-p4 the same treatment. -- 8 -- Subject: Makefile: have python scripts depend on NO_PYTHON setting Like the perl scripts, python scripts need a dependency to ensure they are rebuilt when switching between the dummy versions that run without Python and the real thing. Thanks, I didn't think to look for similar cases. It seems python is the only other thing that gets the unimplemented treatment. If you do: make make NO_TCLTK=Yes I think you'll still end up with a crufty gitk build, but the fix there is much more involved (it would have to create a Sorry, gitk wasn't built script). I don't think it's worth the effort. -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] t0090: mark add-interactive test with PERL prerequisite
Jeff King wrote: The add-interactive system is built in perl. If you build with NO_PERL, running git commit --interactive will exit with an error and the test will fail. Signed-off-by: Jeff King p...@peff.net --- Noticed by Michael while working around gitweb failures by setting NO_PERL. :) Heh. Reviewed-by: Jonathan Nieder jrnie...@gmail.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: [PATCH] t960[34]: mark cvsimport tests as requiring perl
Jeff King wrote: It would probably make sense to have these scripts just skip_all if NO_PERL is set, but I opted to follow the pattern set by t9600, etc. If somebody feels like spending time refactoring the cvsimport test harness, be my guest. Wouldn't it be a matter of the following, plus (optionally) dropping the existing PERL prerequisites on cvs tests? -- 8 -- Subject: test: cvsimport requires perl Git-cvsimport is written in perl, which understandably causes the tests to fail if you build with NO_PERL (which will avoid building cvsimport at all). The earlier cvsimport tests in t9600-t9602 are all marked with a PERL prerequisite, but t9603 and 9604 are not. The one in t9603 was likely not noticed because it is an expected failure. The ones in t9604 have been around for a long time, but it is likely that the combination of NO_PERL and having cvsps installed is rare enough that nobody noticed. Reported-by: Jeff King p...@peff.net Signed-off-by: Jonathan Nieder jrnie...@gmail.com --- t/lib-cvs.sh | 6 ++ 1 file changed, 6 insertions(+) diff --git a/t/lib-cvs.sh b/t/lib-cvs.sh index 9b2bcfb..b75df11 100644 --- a/t/lib-cvs.sh +++ b/t/lib-cvs.sh @@ -10,6 +10,12 @@ then test_done fi +if ! test_have_prereq PERL +then + skip_all='skipping cvsimport tests, perl not available' + test_done +fi + CVS=cvs -f export CVS -- 2.1.0.rc2.206.gedb03e5 -- 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
Reachability lists in git
The git rev-list A ^B command lists all the commits that are reachable from A but not from B. Is there a comparable command for the converse relation, that is, a command to list all the commits that A is reachable from but B isn't? And if there is such a command, can the output be limited to just the latest commits? That is, list commit X if and only if A is reachable from X, B isn't reachable from X, and B is reachable from each of X's children? Thanks, Alan Stern -- 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: ag, **, and the GPL
Hi, Matthew Kaniaris wrote: Ag is licensed under the Apache License Version 2.0 which to the best of my understanding is incompatible with the GPLv2. Would you grant me permission to reuse wildmatch.c (and necessary includes) for use in Ag? wildmatch.c comes from rsync. Conveniently, newer versions of rsync are under the GPLv3, which according to [1] is compatible with the Apache license. The code is at lib/wildmatch.c from git://git.samba.org/rsync.git. It hasn't changed since the rsync license change. Duy, what would you think of making git's wildmatch.c GPL v2 or v3, at your option? Thanks, Jonathan [1] http://www.gnu.org/licenses/quick-guide-gplv3.html -- 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] t960[34]: mark cvsimport tests as requiring perl
On Tue, Nov 18, 2014 at 10:56:22AM -0800, Jonathan Nieder wrote: Jeff King wrote: It would probably make sense to have these scripts just skip_all if NO_PERL is set, but I opted to follow the pattern set by t9600, etc. If somebody feels like spending time refactoring the cvsimport test harness, be my guest. Wouldn't it be a matter of the following, plus (optionally) dropping the existing PERL prerequisites on cvs tests? [...] t/lib-cvs.sh | 6 ++ Yeah, I think so. I was worried that lib-cvs was used by the other CVS tests (like t9200, and t940x), but it seems to be cvsimport-specific. If you do go this route (and that is fine with me), maybe it is worth changing the filename to make that more clear. -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] git-new-workdir: Don't fail if the target directory is empty
Junio C Hamano gits...@pobox.com writes: Paul Smith p...@mad-scientist.net writes: No need to resend only to correct the above, even though there may be comments on the patch itself from me or others that may make you want to reroll this patch, in which case I'd like to see these nits gone. I'll fix in the next iteration. -# don't recreate a workdir over an existing repository -if test -e $new_workdir +# make sure the links use full paths +git_dir=$(cd $git_dir; pwd) With this change, the comment gets much harder to understand. What links? would be the reaction from those who are reading the patch. I just moved this line; I don't think it had much more context beforehand, but I'll definitely rewrite the comment to be more clear. +if test $(ls -a1 $new_workdir/. | wc -l) -ne 2 I wonder if this check is portable for all platforms we care about, but that is OK, as it should be so for the ones I think of and care about ;-) Do you mean . and .. representing an empty directory? That will work on any system where /bin/sh works, for sure. Or do you mean using ls and wc? I can easily avoid this. Recall that new-workdir itself only works on systems that support symlinks; this limits its portability. For example it doesn't work on Windows (unfortunately). I spent the better part of a day a few months ago playing with the various symlink-ish capabilities of Windows NTFS and couldn't find any reliable way to make this work (although I have virtually no Windows fu). The script chdirs around; did you turn $new_workdir into an absolute path already, or given a relative $new_workdir this is attempting to remove a hierarchy that is different from what you created? Good point, I'll fix this up +} +siglist=0 1 2 15 +trap cleanup $siglist -# create the workdir -mkdir -p $new_workdir/.git || die unable to create \$new_workdir\! +# create embedded directories +for x in logs +do +mkdir -p $new_workdir/.git/$x || failed +done # create the links to the original repo. explicitly exclude index, HEAD and # logs/HEAD from the list since they are purely related to the current working # directory, and should not be shared. for x in config refs logs/refs objects info hooks packed-refs remotes rr-cache svn do -case $x in -*/*) -mkdir -p $(dirname $new_workdir/.git/$x) -;; -esac What's this removal about? If $new_workdir/.git/logs does not exist, would ln -s $there/logs/refs $new_workdir/.git/logs/refs succeed without first creating $new_workdir/.git/logs directory? I split the creation of the directories from the symlinks: see the new loop above. This allows us to avoid the icky dirname stuff. New iteration will follow shortly. Cheers! -- 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] date.c: phrase a comment gender neutral
When looking for suitable functions to print dates, I found this gem. Let's make it gender neutral as nowadays people get upset by this gender debate. Signed-off-by: Stefan Beller sbel...@google.com --- date.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/date.c b/date.c index 59dfe57..19fb439 100644 --- a/date.c +++ b/date.c @@ -732,7 +732,7 @@ int parse_expiry_date(const char *date, unsigned long *timestamp) /* * We take over now here, which usually translates * to the current timestamp. This is because the user -* really means to expire everything she has done in +* really means to expire everything they have done in * the past, and by definition reflogs are the record * of the past, and there is nothing from the future * to be kept. -- 2.2.0.rc2.5.gf7b9fb2 -- 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] git-new-workdir: Don't fail if the target directory is empty
Paul Smith p...@mad-scientist.net writes: + if test $(ls -a1 $new_workdir/. | wc -l) -ne 2 I wonder if this check is portable for all platforms we care about, but that is OK, as it should be so for the ones I think of and care about ;-) Do you mean . and .. representing an empty directory? That will work on any system where /bin/sh works, for sure. Even on network mounts from esoteric filesystems and such? When http://pubs.opengroup.org/onlinepubs/9699919799/utilities/ls.html mentions the -A option, it says: -A Write out all directory entries, including those whose names begin with a period ( '.' ) but excluding the entries dot and dot-dot (if they exist). The if they exist part suggests, at least to me, that it is valid for a POSIX filesystem to lack these two (I suspect that one may be able to find a more definitive answer from other parts of the POSIX but I didn't bother). -- 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: Reachability lists in git
Hi, Alan Stern wrote: The git rev-list A ^B command lists all the commits that are reachable from A but not from B. Is there a comparable command for the converse relation, that is, a command to list all the commits that A is reachable from but B isn't? And if there is such a command, can the output be limited to just the latest commits? That is, list commit X if and only if A is reachable from X, B isn't reachable from X, and B is reachable from each of X's children? Someone else can answer your direct question, but you've got my curiosity. What is the application? --ancestry-path is my current favorite tool for walking-forward needs. Curious, Jonathan -- 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] git-new-workdir: Don't fail if the target directory is empty
Allow new workdirs to be created in an empty directory (similar to git
clone). Provide more error checking and clean up on failure.
Signed-off-by: Paul Smith p...@mad-scientist.net
---
Getting rid of ls/wc was not as simple as I'd hoped, due to glob
pathname expansion (can't rely on nullglob e.g.) If you want this let
me know; it will require some yucky code to do the whole thing in native
shell. Since new-workdir only works on systems with ln -s I think we
can feel confident requiring ls and wc as well.
contrib/workdir/git-new-workdir | 55 +++--
1 file changed, 36 insertions(+), 19 deletions(-)
diff --git a/contrib/workdir/git-new-workdir b/contrib/workdir/git-new-workdir
index 75e8b25..aef632d 100755
--- a/contrib/workdir/git-new-workdir
+++ b/contrib/workdir/git-new-workdir
@@ -10,6 +10,10 @@ die () {
exit 128
}
+failed () {
+ die unable to create new workdir \$new_workdir\!
+}
+
if test $# -lt 2 || test $# -gt 3
then
usage $0 repository new_workdir [branch]
@@ -48,35 +52,48 @@ then
a complete repository.
fi
-# don't recreate a workdir over an existing repository
-if test -e $new_workdir
+# make sure the links in the workdir have full paths to the original repo
+git_dir=$(cd $git_dir; pwd)
+
+# don't recreate a workdir over an existing directory, unless it's empty
+if test -d $new_workdir
then
- die destination directory '$new_workdir' already exists.
+ if test $(ls -a1 $new_workdir/. | wc -l) -ne 2
+ then
+ die destination directory '$new_workdir' is not empty.
+ fi
+ cleandir=$new_workdir/.git
+else
+ mkdir -p $new_workdir || failed
+ cleandir=$new_workdir
fi
-# make sure the links use full paths
-git_dir=$(cd $git_dir; pwd)
+cleanup () {
+ rm -rf $cleandir
+}
+siglist=0 1 2 15
+trap cleanup $siglist
-# create the workdir
-mkdir -p $new_workdir/.git || die unable to create \$new_workdir\!
+# create embedded directories
+for x in logs
+do
+ mkdir -p $new_workdir/.git/$x || failed
+done
# create the links to the original repo. explicitly exclude index, HEAD and
# logs/HEAD from the list since they are purely related to the current working
# directory, and should not be shared.
for x in config refs logs/refs objects info hooks packed-refs remotes rr-cache
svn
do
- case $x in
- */*)
- mkdir -p $(dirname $new_workdir/.git/$x)
- ;;
- esac
- ln -s $git_dir/$x $new_workdir/.git/$x
+ ln -s $git_dir/$x $new_workdir/.git/$x || failed
done
-# now setup the workdir
-cd $new_workdir
# copy the HEAD from the original repository as a default branch
-cp $git_dir/HEAD .git/HEAD
-# checkout the branch (either the same as HEAD from the original repository, or
-# the one that was asked for)
-git checkout -f $branch
+cp $git_dir/HEAD $new_workdir/.git/HEAD || failed
+
+# the workdir is set up. if the checkout fails, the user can fix it.
+trap - $siglist
+
+# checkout the branch (either the same as HEAD from the original repository,
+# or the one that was asked for)
+git --git-dir=$new_workdir/.git --work-tree=$new_workdir checkout -f
$branch
--
1.8.5.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
Re: [PATCH] date.c: phrase a comment gender neutral
Stefan Beller wrote: When looking for suitable functions to print dates, I found this gem. Let's make it gender neutral as nowadays people get upset by this gender debate. Eh, I'm not upset. Hope that helps, Jonathan -- 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 v3 00/14] ref-transactions-reflog
On 11/18/2014 07:36 PM, Ronnie Sahlberg wrote: On Tue, Nov 18, 2014 at 3:26 AM, Michael Haggerty mhag...@alum.mit.edu wrote: On 11/18/2014 02:35 AM, Stefan Beller wrote: The following patch series updates the reflog handling to use transactions. This patch series has previously been sent to the list[1]. [...] I was reviewing this patch series (I left some comments in Gerrit about the first few patches) when I realized that I'm having trouble understanding the big picture of where you want to go with this. I have the feeling that the operations that you are implementing are at too low a level of abstraction. What are the elementary write operations that are needed for a reflog? Off the top of my head, 1. Add a reflog entry when a reference is updated in a transaction. 2. Rename a reflog file when the corresponding reference is renamed. 3. Delete the reflog when the corresponding reference is deleted [1]. 4. Configure a reference to be reflogged. 5. Configure a reference to not be reflogged anymore and delete any existing reflog. 6. Selectively expire old reflog entries, e.g., based on their age. Have I forgotten any? The first three should be side-effects of the corresponding reference updates. Aside from the fact that renames are not yet done within a transaction, I think this is already the case. Number 4, I think, currently only happens in conjunction with adding a line to the reflog. So it could be implemented, say, as a FORCE_CREATE_REFLOG flag on a ref_update within a transaction. Number 5 is not very interesting, I think. For example, it could be a separate API function, disconnected from any transactions. Number 6 is more interesting, and from my quick reading, it looks like a lot of the work of this patch series is to allow number 6 to be implemented in builtin/reflog.c:expire_reflog(). But it seems to me that you are building API calls at the wrong level of abstraction. Expiring a reflog should be a single API call to the refs API, and ultimately it should be left up to the refs backend to decide how to implement it. For a filesystem-based backend, it would do what it does now. But (for example) a SQL-based backend might implement this as a single SELECT statement. I agree in principle. But things are more difficult since expire_reflog() has very complex semantics. To keep things simple for the reviews at this stage the logic is the same as the original code: loop over all entries: use very complex conditionals to decide which entries to keep/remove optionally modify the sha1 values for the records we keep write records we keep back to the file one record at a time So that as far as possible, we keep the same rules and behavior but we use a different API for the actual write entry to new reflog. We could wrap this inside a new specific transaction_expire_reflog() function so that other types of backends, for example an SQL backend, could optimize, but I think that should be in a separate later patch because expire_reflog is almost impossibly complex. It will not be a simple SELECT unfortunately. The current expire logic is something like : 1, expire all entries older than timestamp 2, optionally, also expire all entries that refer to unreachable objects using a different timestamp This involves actually reading the objects that the sha1 points to and parsing them! 3, optionally, if the sha1 objects can not be referenced, they are not commit objects or if they don't exist, then expire them too. This also involves reading the objects behind the sha1. 4, optionally, delete reflog entry #foo 5, optionally, if any log entries were discarded due to 2,3,4 then we might also re-write and modify some of the reflog entries we keep. or any combination thereof (6, if --dry-run is specified, just print what we would have expired) 2 and 3 requires that we need to read the objects for the entry 4 allows us to delete a specific entry 5 means that even for entries we keep we will need to mutate them. Thanks for the explanation. I now understand that it might be more than a single SELECT statement. Regarding the complicated rules for expiring reflogs (1, 2, 3, 4): For now I think it would be fine for the new expire_reflog() API function to take a callback function as an argument. Regarding the stitching together of the survivors (5), it seems like the API function would be the right place to handle that. Regarding 6, it sounds like you could run the reflog entries through your callback and report what it *would* have expired. I also don't have the feeling that reflog expiration has to be done within a ref_transaction. For example, is there ever a reason to combine expiration with other reference updates in a single atomic transaction? --updateref In expire_reflog() we not only prune the reflog. When --updateref is used we update the actual ref itself. I think we
Re: [PATCH] date.c: phrase a comment gender neutral
Stefan Beller sbel...@google.com writes: When looking for suitable functions to print dates, I found this gem. Let's make it gender neutral as nowadays people get upset by this gender debate. For some time I used to use she/her on Mondays, Wednesdays and Fridays and he/his on other days to balance them, and you are seeing the artifact of that. Some people might feel that we would be better off using they all the time, but IMO it's such a minor thing that once it _is_ in the tree, it's not really worth the patch noise to go and fix it up. Signed-off-by: Stefan Beller sbel...@google.com --- date.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/date.c b/date.c index 59dfe57..19fb439 100644 --- a/date.c +++ b/date.c @@ -732,7 +732,7 @@ int parse_expiry_date(const char *date, unsigned long *timestamp) /* * We take over now here, which usually translates * to the current timestamp. This is because the user - * really means to expire everything she has done in + * really means to expire everything they have done in * the past, and by definition reflogs are the record * of the past, and there is nothing from the future * to be kept. -- 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] date.c: phrase a comment gender neutral
ok. I'll stop sending such gender related nits. On Tue, Nov 18, 2014 at 12:03 PM, Junio C Hamano gits...@pobox.com wrote: Stefan Beller sbel...@google.com writes: When looking for suitable functions to print dates, I found this gem. Let's make it gender neutral as nowadays people get upset by this gender debate. For some time I used to use she/her on Mondays, Wednesdays and Fridays and he/his on other days to balance them, and you are seeing the artifact of that. Some people might feel that we would be better off using they all the time, but IMO it's such a minor thing that once it _is_ in the tree, it's not really worth the patch noise to go and fix it up. Signed-off-by: Stefan Beller sbel...@google.com --- date.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/date.c b/date.c index 59dfe57..19fb439 100644 --- a/date.c +++ b/date.c @@ -732,7 +732,7 @@ int parse_expiry_date(const char *date, unsigned long *timestamp) /* * We take over now here, which usually translates * to the current timestamp. This is because the user - * really means to expire everything she has done in + * really means to expire everything they have done in * the past, and by definition reflogs are the record * of the past, and there is nothing from the future * to be kept. -- 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] git-new-workdir: Don't fail if the target directory is empty
Paul Smith p...@mad-scientist.net writes: Getting rid of ls/wc was not as simple as I'd hoped, I didn't say ls/wc was not portable. Assuming that a directory for which the output from ls -a has two entries is empty may be. -- 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: Reachability lists in git
Jonathan Nieder jrnie...@gmail.com writes: --ancestry-path is my current favorite tool for walking-forward needs. Curious. I often want to answer this question: Commit 982ac87 was reported to be faulty. What topic was it on and at which point was it merged to 'master'? - What is the 'bottom' of the topic, that is, the commit reachable from the faulty commit that was already on 'master' when the faulty commit was written the first time? - What is the 'top' of the topic, that is, were there more commits made on top to build on the faulty commit on the topic before the whole thing was merged to 'master'? - Were there follow-up fixes and enhancements on the topic after the topic was merged to 'master' (this is harder)? And my experiments with --ancestry-path has been less than ideal. -- 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: Reachability lists in git
Jonathan Nieder wrote: Junio C Hamano wrote: - Were there follow-up fixes and enhancements on the topic after the topic was merged to 'master' (this is harder)? There's only one line coming out of the-merge^2 in the ancestry-path graph, so there were no such follow-up fixes. Or rather, there are two lines, but the second is just a merge of the same topic to maint-1.8.1. And here following the railroad tracks becomes tedious. gitk has a nicer interface for list children and go to child. -- 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: Reachability lists in git
On Tue, 18 Nov 2014, Jonathan Nieder wrote: Hi, Alan Stern wrote: The git rev-list A ^B command lists all the commits that are reachable from A but not from B. Is there a comparable command for the converse relation, that is, a command to list all the commits that A is reachable from but B isn't? And if there is such a command, can the output be limited to just the latest commits? That is, list commit X if and only if A is reachable from X, B isn't reachable from X, and B is reachable from each of X's children? Someone else can answer your direct question, but you've got my curiosity. What is the application? Tracking down regressions. Bisection isn't perfect. Suppose a bisection run ends up saying that B is the first bad commit. It's easy enough to build B and test it, to verify that it really is bad. But to be sure that B introduced the fault, it would help to find the latest commit that doesn't include B's changes -- that is, the latest commit that B isn't reachable from (or the maximal elements in the set of all such commits). This is also important in cases where there are multiple bugs and you want to investigate only the commits that don't include one of the bugs. Alan Stern -- 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 v3 00/14] ref-transactions-reflog
Michael Haggerty mhag...@alum.mit.edu writes: I'm still not convinced. For me, reflog_expire() is an unusual outlier operation, much like git gc or git pack-refs or git fsck. None of these are part of the beautiful Git data model; they are messy maintenance operations. Forcing reference transactions to be general enough to allow reflog expiration to be implemented *outside* the refs API sacrificies their simplicity for lots of infrastructure that will probably only be used to implement this single operation. Better to implement reflog expiration *inside* the refs API. Sorry, but I lost track---which one is inside and which one is outside? -- 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: Reachability lists in git
Alan Stern wrote: Tracking down regressions. Bisection isn't perfect. Suppose a bisection run ends up saying that B is the first bad commit. It's easy enough to build B and test it, to verify that it really is bad. But to be sure that B introduced the fault, it would help to find the latest commit that doesn't include B's changes -- that is, the latest commit that B isn't reachable from (or the maximal elements in the set of all such commits). Isn't that B^ (or B^ and B^2, if B is a merge)? -- 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: Reachability lists in git
Jonathan Nieder jrnie...@gmail.com writes: Junio C Hamano wrote: Jonathan Nieder jrnie...@gmail.com writes: --ancestry-path is my current favorite tool for walking-forward needs. Curious. I often want to answer this question: [...] And my experiments with --ancestry-path has been less than ideal. Thanks for an example. I've found it works okay interactively, less so for scripted use (so I wish there were something better, though I haven't sketched out what that something better would look like). Commit 982ac87 was reported to be faulty. What topic was it on and at which point was it merged to 'master'? $ git log --graph --ancestry-path 982ac87^..origin/master Yup, that is what I've been using and was wishing that there would be better alternatives. -- 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: Reachability lists in git
On Tue, 18 Nov 2014, Jonathan Nieder wrote: Alan Stern wrote: Tracking down regressions. Bisection isn't perfect. Suppose a bisection run ends up saying that B is the first bad commit. It's easy enough to build B and test it, to verify that it really is bad. But to be sure that B introduced the fault, it would help to find the latest commit that doesn't include B's changes -- that is, the latest commit that B isn't reachable from (or the maximal elements in the set of all such commits). Isn't that B^ (or B^ and B^2, if B is a merge)? No. Here's a simple example: Y / / X--B In this diagram, X = B^. But B isn't reachable from either X or Y, whereas it is reachable from one of X's children (namely Y). Therefore Y is the unique maximal commit which B is not reachable from. Alan Stern -- 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] git-new-workdir: Don't fail if the target directory is empty
On Tue, 2014-11-18 at 12:15 -0800, Junio C Hamano wrote: Paul Smith p...@mad-scientist.net writes: Getting rid of ls/wc was not as simple as I'd hoped, I didn't say ls/wc was not portable. Assuming that a directory for which the output from ls -a has two entries is empty may be. Yes, I didn't get your email before I sent the latest patch. Sorry about that. On Tue, 2014-11-18 at 11:32 -0800, Junio C Hamano wrote: Even on network mounts from esoteric filesystems and such? When http://pubs.opengroup.org/onlinepubs/9699919799/utilities/ls.html mentions the -A option, it says: -A Write out all directory entries, including those whose names begin with a period ( '.' ) but excluding the entries dot and dot-dot (if they exist). The if they exist part suggests, at least to me, that it is valid for a POSIX filesystem to lack these two (I suspect that one may be able to find a more definitive answer from other parts of the POSIX but I didn't bother). Hm. Well, POSIX clearly reserves . and .. to be special and requires that directories containing only those values are considered empty (rmdir(2) says so). The definitions section contains special wording for dot and dot-dot. Looking at http://pubs.opengroup.org/onlinepubs/9699919799/xrat/V4_xbd_chap04.html Each directory has exactly one parent directory which is represented by the name dot-dot in the first directory. which implies that every directory must have ... This is in the Rationale, I realize. There are also various mentions to using . as a synonym for the current directory. I can't find a clear statement that both are required and that ls -a must show them. I've used a wide range of UNIX-en and filesystems for 30 years or so and never seen one that didn't provide them. It seems like it would break quite a few scripts, at least. Even virtual filesystems like ClearCase's MVFS provide . and ... If you want to allow for this possibility I can do so but it would be a bit crufty. Personally I think it would be overkill but you're the boss: let me know :-). Cheers! -- 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] On watchman support
David Turner dtur...@twopensource.com writes: On Tue, 2014-11-18 at 17:48 +0700, Duy Nguyen wrote: My patches are not the world's most beautiful, but they do work. I think some improvement might be possible by keeping info about tracked files in the index, and only storing the tree of ignored and untracked files separately. But I have not thought this through fully. In any case, making use of shared memory for the fs_cache (as some of your other patches do for the index) would definitely save time. By the way, what happened to your sse optimization in refs.c? I see it's reverted but I didn't follow closely to know why. I don't know why either -- it works just fine. There was a bug, but I fixed it. Junio? I vaguely recall that the reason why we dropped it was because it was too much code churn in an area that was being worked on in parallel, but you may need to go back to the list archive for details. -- 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] git-new-workdir: Don't fail if the target directory is empty
Paul Smith p...@mad-scientist.net writes: I can't find a clear statement that both are required and that ls -a must show them. I've used a wide range of UNIX-en and filesystems for 30 years or so and never seen one that didn't provide them. It seems like it would break quite a few scripts, at least. Even virtual filesystems like ClearCase's MVFS provide . and ... If you want to allow for this possibility I can do so but it would be a bit crufty. Personally I think it would be overkill but you're the boss: let me know :-). Doesn't the description of the -A option I quoted upthread hint a simpler and clearer solution? I.e. test $(ls -A | wc -l) = 0? -- 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: Reachability lists in git
Alan Stern st...@rowland.harvard.edu writes: On Tue, 18 Nov 2014, Jonathan Nieder wrote: Alan Stern wrote: Tracking down regressions. Bisection isn't perfect. Suppose a bisection run ends up saying that B is the first bad commit. It's easy enough to build B and test it, to verify that it really is bad. But to be sure that B introduced the fault, it would help to find the latest commit that doesn't include B's changes -- that is, the latest commit that B isn't reachable from (or the maximal elements in the set of all such commits). Isn't that B^ (or B^ and B^2, if B is a merge)? No. Here's a simple example: Y / / X--B In this diagram, X = B^. But B isn't reachable from either X or Y, whereas it is reachable from one of X's children (namely Y). Around here when we draw history horizontally we place parents on the left hand side and the children on the right hand side. X is B's parent and does not include B's changes. Y is not B's parent. Y is a child of X so it has all the imperfection of X inherited to it (except the ones that is fixed by Y itself), but there is no way it inherited the bug B introduced relative to X. Why do you say B is reachable from Y? If you mean that B is a merge between X and Y, then that is already covered by what Jonathan wrote (or B^ and B^2 if B is a merge). XY \\ .B Admittedly it is a needless merge (there should normally be one or more commits between X and B on the other branch to make a merge B worthwhile---you could just have fast forwarded Y to B), but that does not break the reachability or bisectability in any way. Confused... -- 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: Reachability lists in git
Junio C Hamano gits...@pobox.com writes: Alan Stern st...@rowland.harvard.edu writes: On Tue, 18 Nov 2014, Jonathan Nieder wrote: Alan Stern wrote: Tracking down regressions. Bisection isn't perfect. Suppose a bisection run ends up saying that B is the first bad commit. It's easy enough to build B and test it, to verify that it really is bad. But to be sure that B introduced the fault, it would help to find the latest commit that doesn't include B's changes -- that is, the latest commit that B isn't reachable from (or the maximal elements in the set of all such commits). Isn't that B^ (or B^ and B^2, if B is a merge)? No. Here's a simple example: Y / / X--B In this diagram, X = B^. But B isn't reachable from either X or Y, whereas it is reachable from one of X's children (namely Y). ... Why do you say B is reachable from Y? If you mean that B is a merge between X and Y, then that is already covered by what Jonathan wrote (or B^ and B^2 if B is a merge). XY \\ .B ... No, that cannot be what you meant. I was confused. The above picture does not make B reachable from Y (it is the other way around: B reaches Y). The topology where B isn't reachable from either X or Y and is reachable from Y would be X---Y i.e. B = Y^2, X = Y^1 = B^1 \ / B If B is broken, and X is not, then Y would be contaminated by the breakage B introduces relative to X, unless Y is an evil merge and fixed that breakage while merging. In any case, even if Y is found to be broken, its parent B is already broken, so that does not place the blame on Y, does it? Still confused why you feel Y is any significant... -- 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] On watchman support
On Tue, 2014-11-18 at 12:55 -0800, Junio C Hamano wrote: David Turner dtur...@twopensource.com writes: On Tue, 2014-11-18 at 17:48 +0700, Duy Nguyen wrote: My patches are not the world's most beautiful, but they do work. I think some improvement might be possible by keeping info about tracked files in the index, and only storing the tree of ignored and untracked files separately. But I have not thought this through fully. In any case, making use of shared memory for the fs_cache (as some of your other patches do for the index) would definitely save time. By the way, what happened to your sse optimization in refs.c? I see it's reverted but I didn't follow closely to know why. I don't know why either -- it works just fine. There was a bug, but I fixed it. Junio? I vaguely recall that the reason why we dropped it was because it was too much code churn in an area that was being worked on in parallel, but you may need to go back to the list archive for details. OK, in that case I'll try to remember to reroll it once the rest of the refs stuff lands. -- 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 v3 00/14] ref-transactions-reflog
On 11/18/2014 09:30 PM, Junio C Hamano wrote:
Michael Haggerty mhag...@alum.mit.edu writes:
I'm still not convinced. For me, reflog_expire() is an unusual outlier
operation, much like git gc or git pack-refs or git fsck. None of
these are part of the beautiful Git data model; they are messy
maintenance operations. Forcing reference transactions to be general
enough to allow reflog expiration to be implemented *outside* the refs
API sacrificies their simplicity for lots of infrastructure that will
probably only be used to implement this single operation. Better to
implement reflog expiration *inside* the refs API.
Sorry, but I lost track---which one is inside and which one is
outside?
By inside I mean the code that would be within the reference-handling
library if we had such a thing; i.e., implemented in refs.c. By
outside I mean in the code that calls the library; in this case the
outside code would live in builtin/reflog.c.
In other words, I'd prefer the outside code in builtin/reflog.c to
look vaguely like
expire_reflogs_for_me_please(refname,
should_expire_cb, cbdata, flags)
rather than
transaction = ...
for_each_reflog_entry {
if should_expire()
adjust neighbor reflog entries if necessary (actually,
they're transaction entries so we would have to
preprocess them before putting them in the
transaction)
else
add reflog entry to transaction
}
ref_transaction_commit()
and instead handle as much of the iteration, bookkeeping, and rewriting
as possible inside expire_reflogs_for_me_please().
Michael
--
Michael Haggerty
mhag...@alum.mit.edu
--
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: Reachability lists in git
On Tue, 18 Nov 2014, Junio C Hamano wrote: Alan Stern st...@rowland.harvard.edu writes: On Tue, 18 Nov 2014, Jonathan Nieder wrote: Alan Stern wrote: Tracking down regressions. Bisection isn't perfect. Suppose a bisection run ends up saying that B is the first bad commit. It's easy enough to build B and test it, to verify that it really is bad. But to be sure that B introduced the fault, it would help to find the latest commit that doesn't include B's changes -- that is, the latest commit that B isn't reachable from (or the maximal elements in the set of all such commits). Isn't that B^ (or B^ and B^2, if B is a merge)? No. Here's a simple example: Y / / X--B In this diagram, X = B^. But B isn't reachable from either X or Y, whereas it is reachable from one of X's children (namely Y). Around here when we draw history horizontally we place parents on the left hand side and the children on the right hand side. X is B's parent and does not include B's changes. Y is not B's parent. Y is a child of X so it has all the imperfection of X inherited to it (except the ones that is fixed by Y itself), but there is no way it inherited the bug B introduced relative to X. Why do you say B is reachable from Y? I omitted a negation by mistake, sorry. I meant to say: But B isn't reachable from either X or Y, and it isn't reachable from one of X's children (namely Y). Thus, if B introduced a bug, that bug would not be present in Y. But Y might be better for testing than X, because Y might fix some other problems that are present in X. Alan Stern -- 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: Reachability lists in git
Alan Stern st...@rowland.harvard.edu writes: No. Here's a simple example: Y / / X--B In this diagram, X = B^. But B isn't reachable from either X or Y, whereas it is reachable from one of X's children (namely Y). ... Thus, if B introduced a bug, that bug would not be present in Y. But Y might be better for testing than X, because Y might fix some other problems that are present in X. The problem with that line of reasoning is that in real life there will be unbound number of Y's that forked from a point before somebody wrote B. Which one among these Y's would you pick and why? If Y has fixed another problem that is present in X and make it easier to test, Z, a direct descendant of Y (i.e. Z^1 = Y), may have fixed yet another problem that is unrelated to the problem B introduced and it may make the result even easier to test. Where do you stop? Still confused... -- 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] On watchman support
David Turner dtur...@twopensource.com writes: On Tue, 2014-11-18 at 12:55 -0800, Junio C Hamano wrote: I vaguely recall that the reason why we dropped it was because it was too much code churn in an area that was being worked on in parallel, but you may need to go back to the list archive for details. OK, in that case I'll try to remember to reroll it once the rest of the refs stuff lands. Sure. But I would much prefer to see us explore an arch independent optimisation of the caller before starting to micro-optimize a leaf function. It is not check_refname_format() that is the real problem. It's the fact that we do O(# of refs) work whenever we have to access the packed-refs file. check_refname_format() is part of that, surely, but so is reading the file, creating all of the refname structs in memory, etc. (credit to 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 v3 00/14] ref-transactions-reflog
Michael Haggerty mhag...@alum.mit.edu writes: Sorry, but I lost track---which one is inside and which one is outside? By inside I mean the code that would be within the reference-handling library if we had such a thing; i.e., implemented in refs.c. By outside I mean in the code that calls the library; in this case the outside code would live in builtin/reflog.c. In other words, I'd prefer the outside code in builtin/reflog.c to look vaguely like expire_reflogs_for_me_please(refname, should_expire_cb, cbdata, flags) rather than ... (written as a client of the ref API) ... OK, I very much agree with that. -- 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: Reachability lists in git
On Tue, 18 Nov 2014, Junio C Hamano wrote: Alan Stern st...@rowland.harvard.edu writes: No. Here's a simple example: Y / / X--B In this diagram, X = B^. But B isn't reachable from either X or Y, whereas it is reachable from one of X's children (namely Y). ... Thus, if B introduced a bug, that bug would not be present in Y. But Y might be better for testing than X, because Y might fix some other problems that are present in X. The problem with that line of reasoning is that in real life there will be unbound number of Y's that forked from a point before somebody wrote B. Which one among these Y's would you pick and why? I don't know. But I would like to see what is available. I might even merge all those commits and test that (if there aren't any bad conflicts). If Y has fixed another problem that is present in X and make it easier to test, Z, a direct descendant of Y (i.e. Z^1 = Y), may have fixed yet another problem that is unrelated to the problem B introduced and it may make the result even easier to test. Where do you stop? If Y is maximal among the comments that B isn't reachable from and Z^ = Y then, by definition, B _is_ reachable from Z. Therefore the bug introduced in B will be present in Z, unless it got fixed somewhere in between. Either way, Z is not a good candidate for testing whereas Y is. Alan Stern -- 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] git-new-workdir: Don't fail if the target directory is empty
On Tue, 2014-11-18 at 12:58 -0800, Junio C Hamano wrote: Doesn't the description of the -A option I quoted upthread hint a simpler and clearer solution? I.e. test $(ls -A | wc -l) = 0? Yes, but unfortunately for us the -A flag was added to POSIX Issue 7. It's not present in the previous version of POSIX, Issue 6: http://pubs.opengroup.org/onlinepubs/009695399/utilities/ls.html It came from the BSD world, so it might not be available on older SysV-derived systems (AIX, HP-UX, even Solaris... I don't have access to these anymore so I can't say). Ultimately it's probably more portable to assume ls -a always prints . and .. than to assume ls -A is supported. Cheers! -- 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] refs.c: use a stringlist for repack_without_refs
This patch was heavily inspired by a part of the ref-transactions-rename
series[1], but people tend to dislike large series and this part is
relatively easy to take out and unrelated, so I'll send it as a single
patch.
This patch doesn't intend any functional changes. It is just a refactoring,
which replaces a char** array by a stringlist in the function
repack_without_refs.
[1] https://www.mail-archive.com/git@vger.kernel.org/msg60604.html
Idea-by: Ronnie Sahlberg sahlb...@google.com
Signed-off-by: Stefan Beller sbel...@google.com
---
builtin/remote.c | 22 +++---
refs.c | 41 -
refs.h | 3 +--
3 files changed, 28 insertions(+), 38 deletions(-)
diff --git a/builtin/remote.c b/builtin/remote.c
index 7f28f92..dca4ebf 100644
--- a/builtin/remote.c
+++ b/builtin/remote.c
@@ -750,16 +750,11 @@ static int mv(int argc, const char **argv)
static int remove_branches(struct string_list *branches)
{
struct strbuf err = STRBUF_INIT;
- const char **branch_names;
int i, result = 0;
- branch_names = xmalloc(branches-nr * sizeof(*branch_names));
- for (i = 0; i branches-nr; i++)
- branch_names[i] = branches-items[i].string;
- if (repack_without_refs(branch_names, branches-nr, err))
+ if (repack_without_refs(branches, err))
result |= error(%s, err.buf);
strbuf_release(err);
- free(branch_names);
for (i = 0; i branches-nr; i++) {
struct string_list_item *item = branches-items + i;
@@ -1317,7 +1312,6 @@ static int prune_remote(const char *remote, int dry_run)
int result = 0, i;
struct ref_states states;
struct string_list delete_refs_list = STRING_LIST_INIT_NODUP;
- const char **delete_refs;
const char *dangling_msg = dry_run
? _( %s will become dangling!)
: _( %s has become dangling!);
@@ -1325,6 +1319,11 @@ static int prune_remote(const char *remote, int dry_run)
memset(states, 0, sizeof(states));
get_remote_ref_states(remote, states, GET_REF_STATES);
+ for (i = 0; i states.stale.nr; i++)
+ string_list_insert(delete_refs_list,
+ states.stale.items[i].util);
+
+
if (states.stale.nr) {
printf_ln(_(Pruning %s), remote);
printf_ln(_(URL: %s),
@@ -1332,24 +1331,17 @@ static int prune_remote(const char *remote, int dry_run)
? states.remote-url[0]
: _((no URL)));
- delete_refs = xmalloc(states.stale.nr * sizeof(*delete_refs));
- for (i = 0; i states.stale.nr; i++)
- delete_refs[i] = states.stale.items[i].util;
if (!dry_run) {
struct strbuf err = STRBUF_INIT;
- if (repack_without_refs(delete_refs, states.stale.nr,
- err))
+ if (repack_without_refs(delete_refs_list, err))
result |= error(%s, err.buf);
strbuf_release(err);
}
- free(delete_refs);
}
for (i = 0; i states.stale.nr; i++) {
const char *refname = states.stale.items[i].util;
- string_list_insert(delete_refs_list, refname);
-
if (!dry_run)
result |= delete_ref(refname, NULL, 0);
diff --git a/refs.c b/refs.c
index 5ff457e..2333a9b 100644
--- a/refs.c
+++ b/refs.c
@@ -2639,23 +2639,23 @@ static int curate_packed_ref_fn(struct ref_entry
*entry, void *cb_data)
return 0;
}
-int repack_without_refs(const char **refnames, int n, struct strbuf *err)
+int repack_without_refs(struct string_list *without, struct strbuf *err)
{
struct ref_dir *packed;
struct string_list refs_to_delete = STRING_LIST_INIT_DUP;
struct string_list_item *ref_to_delete;
- int i, ret, removed = 0;
+ int count, ret, removed = 0;
assert(err);
- /* Look for a packed ref */
- for (i = 0; i n; i++)
- if (get_packed_ref(refnames[i]))
- break;
+ count = 0;
+ for_each_string_list_item(ref_to_delete, without)
+ if (get_packed_ref(ref_to_delete-string))
+ count++;
- /* Avoid locking if we have nothing to do */
- if (i == n)
- return 0; /* no refname exists in packed refs */
+ /* No refname exists in packed refs */
+ if (!count)
+ return 0;
if (lock_packed_refs(0)) {
unable_to_lock_message(git_path(packed-refs), errno, err);
@@ -2664,8 +2664,8 @@ int repack_without_refs(const char **refnames, int n,
struct strbuf *err)
packed = get_packed_refs(ref_cache);
/* Remove refnames from the
Re: [PATCH] git-new-workdir: Don't fail if the target directory is empty
OK, thanks for digging. Let's go with this version, then. On Tue, Nov 18, 2014 at 2:25 PM, Paul Smith p...@mad-scientist.net wrote: On Tue, 2014-11-18 at 12:58 -0800, Junio C Hamano wrote: Doesn't the description of the -A option I quoted upthread hint a simpler and clearer solution? I.e. test $(ls -A | wc -l) = 0? Yes, but unfortunately for us the -A flag was added to POSIX Issue 7. It's not present in the previous version of POSIX, Issue 6: http://pubs.opengroup.org/onlinepubs/009695399/utilities/ls.html It came from the BSD world, so it might not be available on older SysV-derived systems (AIX, HP-UX, even Solaris... I don't have access to these anymore so I can't say). Ultimately it's probably more portable to assume ls -a always prints . and .. than to assume ls -A is supported. Cheers! -- 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] refs.c: use a stringlist for repack_without_refs
Stefan Beller sbel...@google.com writes:
This patch was heavily inspired by a part of the ref-transactions-rename
series[1], but people tend to dislike large series and this part is
relatively easy to take out and unrelated, so I'll send it as a single
patch.
This patch doesn't intend any functional changes. It is just a refactoring,
which replaces a char** array by a stringlist in the function
repack_without_refs.
[1] https://www.mail-archive.com/git@vger.kernel.org/msg60604.html
Idea-by: Ronnie Sahlberg sahlb...@google.com
Signed-off-by: Stefan Beller sbel...@google.com
---
builtin/remote.c | 22 +++---
refs.c | 41 -
refs.h | 3 +--
3 files changed, 28 insertions(+), 38 deletions(-)
In one codepath we were already using a string_list delete_refs_list
anyway, so it makes sense to reuse that by movingan existing call to
string_list_insert() a bit higher, instead of maintaining another
array of pointers delete_refs[] to strings.
OK, it simplifies the code by reducing the line count, which is a
plus ;-)
Sounds good.
diff --git a/builtin/remote.c b/builtin/remote.c
index 7f28f92..dca4ebf 100644
--- a/builtin/remote.c
+++ b/builtin/remote.c
@@ -750,16 +750,11 @@ static int mv(int argc, const char **argv)
static int remove_branches(struct string_list *branches)
{
struct strbuf err = STRBUF_INIT;
- const char **branch_names;
int i, result = 0;
- branch_names = xmalloc(branches-nr * sizeof(*branch_names));
- for (i = 0; i branches-nr; i++)
- branch_names[i] = branches-items[i].string;
- if (repack_without_refs(branch_names, branches-nr, err))
+ if (repack_without_refs(branches, err))
result |= error(%s, err.buf);
strbuf_release(err);
- free(branch_names);
for (i = 0; i branches-nr; i++) {
struct string_list_item *item = branches-items + i;
@@ -1317,7 +1312,6 @@ static int prune_remote(const char *remote, int dry_run)
int result = 0, i;
struct ref_states states;
struct string_list delete_refs_list = STRING_LIST_INIT_NODUP;
- const char **delete_refs;
const char *dangling_msg = dry_run
? _( %s will become dangling!)
: _( %s has become dangling!);
@@ -1325,6 +1319,11 @@ static int prune_remote(const char *remote, int
dry_run)
memset(states, 0, sizeof(states));
get_remote_ref_states(remote, states, GET_REF_STATES);
+ for (i = 0; i states.stale.nr; i++)
+ string_list_insert(delete_refs_list,
+states.stale.items[i].util);
+
+
if (states.stale.nr) {
printf_ln(_(Pruning %s), remote);
printf_ln(_(URL: %s),
@@ -1332,24 +1331,17 @@ static int prune_remote(const char *remote, int
dry_run)
? states.remote-url[0]
: _((no URL)));
- delete_refs = xmalloc(states.stale.nr * sizeof(*delete_refs));
- for (i = 0; i states.stale.nr; i++)
- delete_refs[i] = states.stale.items[i].util;
if (!dry_run) {
struct strbuf err = STRBUF_INIT;
- if (repack_without_refs(delete_refs, states.stale.nr,
- err))
+ if (repack_without_refs(delete_refs_list, err))
result |= error(%s, err.buf);
strbuf_release(err);
}
- free(delete_refs);
}
for (i = 0; i states.stale.nr; i++) {
const char *refname = states.stale.items[i].util;
- string_list_insert(delete_refs_list, refname);
-
if (!dry_run)
result |= delete_ref(refname, NULL, 0);
diff --git a/refs.c b/refs.c
index 5ff457e..2333a9b 100644
--- a/refs.c
+++ b/refs.c
@@ -2639,23 +2639,23 @@ static int curate_packed_ref_fn(struct ref_entry
*entry, void *cb_data)
return 0;
}
-int repack_without_refs(const char **refnames, int n, struct strbuf *err)
+int repack_without_refs(struct string_list *without, struct strbuf *err)
{
struct ref_dir *packed;
struct string_list refs_to_delete = STRING_LIST_INIT_DUP;
struct string_list_item *ref_to_delete;
- int i, ret, removed = 0;
+ int count, ret, removed = 0;
assert(err);
- /* Look for a packed ref */
- for (i = 0; i n; i++)
- if (get_packed_ref(refnames[i]))
- break;
+ count = 0;
+ for_each_string_list_item(ref_to_delete, without)
+ if (get_packed_ref(ref_to_delete-string))
+ count++;
- /* Avoid locking if we have nothing to do */
- if (i == n)
- return 0; /* no refname exists in packed refs */
+ /* No
Subject: [PATCH/RFC] Documentation/git-stripspace: Update synopsis
Add synopsis with the '--comment-lines' option.
Signed-off-by: Slavomir Vlcek s...@inventati.org
---
Hi,
there were no mention of '--comment-lines' in the synopsis.
('--comment-lines' and '--strip-comments' options
are mutually exclusive).
I solved this by adding an extra (second) synopsis line
so it looks just like the 'usage_msg' in 'builtin/stripspace.c'.
But perhaps it would be wiser to have something like
git stripspace [[-s | --strip-comments] | [-c | --comment-lines]] input
instead (and perhaps ordered alphabetically).
This approach can be seen e.g. in the git-add man page.
For the 'master'.
Also, have a few questions about stripspace generally:
a) Should 'git stripspace --comment-lines' really leave the trailing
whitespace alone (example: ' hello '-'# hello ')?
b) In the documentation there is:
-s, --strip-comments
Skip and remove all lines starting with comment character (default
#).
part. This default word somehow suggests some new command option that would
allow to change the comment character. Would you accept a patch implementing
this or such functionality is not desired.
Thank you.
Documentation/git-stripspace.txt | 1 +
1 file changed, 1 insertion(+)
diff --git a/Documentation/git-stripspace.txt b/Documentation/git-stripspace.txt
index c87bfcb..6c6e989 100644
--- a/Documentation/git-stripspace.txt
+++ b/Documentation/git-stripspace.txt
@@ -10,6 +10,7 @@ SYNOPSIS
[verse]
'git stripspace' [-s | --strip-comments] input
+'git stripspace' [-c | --comment-lines] input
DESCRIPTION
---
--
2.0.1
--
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] refs.c: handle locking failure during transaction better
Change lock_ref_sha1_basic to return an error instead of dying,
when we fail to lock a file during a transaction. This function is
only called from transaction_commit() and it knows how to handle
these failures.
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
Signed-off-by: Jonathan Nieder jrnie...@gmail.com
Cherry-picked-to-master: Stefan Beller sbel...@google.com
Signed-off-by: Stefan Beller sbel...@google.com
---
refs.c | 10 --
1 file changed, 8 insertions(+), 2 deletions(-)
This was part of the ref-transactions-rename series[1],
but it makes sense to send this one as a separate patch.
Thanks,
Stefan
[1] https://www.mail-archive.com/git@vger.kernel.org/msg60604.html
diff --git a/refs.c b/refs.c
index 5ff457e..0347328 100644
--- a/refs.c
+++ b/refs.c
@@ -2318,6 +2318,7 @@ static struct ref_lock *lock_ref_sha1_basic(const char
*refname,
lock-lock_fd = hold_lock_file_for_update(lock-lk, ref_file, lflags);
if (lock-lock_fd 0) {
+ last_errno = errno;
if (errno == ENOENT --attempts_remaining 0)
/*
* Maybe somebody just deleted one of the
@@ -2325,8 +2326,13 @@ static struct ref_lock *lock_ref_sha1_basic(const char
*refname,
* again:
*/
goto retry;
- else
- unable_to_lock_die(ref_file, errno);
+ else {
+ struct strbuf err = STRBUF_INIT;
+ unable_to_lock_message(ref_file, errno, err);
+ error(%s, err.buf);
+ strbuf_reset(err);
+ goto error_return;
+ }
}
return old_sha1 ? verify_lock(lock, old_sha1, mustexist) : lock;
--
2.2.0.rc2.5.gf7b9fb2
--
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] t0090: mark add-interactive test with PERL prerequisite
jrnie...@gmail.com wrote on Tue, 18 Nov 2014 10:43 -0800: ... and here's a patch on top to give git-p4 the same treatment. -- 8 -- Subject: Makefile: have python scripts depend on NO_PYTHON setting Like the perl scripts, python scripts need a dependency to ensure they are rebuilt when switching between the dummy versions that run without Python and the real thing. Signed-off-by: Jonathan Nieder jrnie...@gmail.com --- Makefile | 3 +++ 1 file changed, 3 insertions(+) diff --git a/Makefile b/Makefile index 8f980e0..7482a4d 100644 --- a/Makefile +++ b/Makefile @@ -1736,6 +1736,9 @@ $(SCRIPT_PERL_GEN) git-instaweb: % : unimplemented.sh mv $@+ $@ endif # NO_PERL +# This makes sure we depend on the NO_PYTHON setting itself. +$(SCRIPT_PYTHON_GEN): GIT-BUILD-OPTIONS + ifndef NO_PYTHON $(SCRIPT_PYTHON_GEN): GIT-CFLAGS GIT-PREFIX GIT-PYTHON-VARS $(SCRIPT_PYTHON_GEN): % : %.py -- 2.1.0.rc2.206.gedb03e5 Looks obviously correct, thanks for remembering the other scripting languages. :) Acked-by: Pete Wyckoff p...@padd.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: Subject: [PATCH/RFC] Documentation/git-stripspace: Update synopsis
On 11/19/2014 12:16 AM, Slavomir Vlcek wrote: b) In the documentation there is: -s, --strip-comments Skip and remove all lines starting with comment character (default #). part. This default word somehow suggests some new command option that would allow to change the comment character. Would you accept a patch implementing this or such functionality is not desired. I take it back, just found that there is a global comment_line_char variable that controls this stuff. Apologizing. -- 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] refs.c: handle locking failure during transaction better
I messed up authorship on this one.
This was of course found and authored by Ronnie.
On Tue, Nov 18, 2014 at 3:17 PM, Stefan Beller sbel...@google.com wrote:
Change lock_ref_sha1_basic to return an error instead of dying,
when we fail to lock a file during a transaction. This function is
only called from transaction_commit() and it knows how to handle
these failures.
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
Signed-off-by: Jonathan Nieder jrnie...@gmail.com
Cherry-picked-to-master: Stefan Beller sbel...@google.com
Signed-off-by: Stefan Beller sbel...@google.com
---
refs.c | 10 --
1 file changed, 8 insertions(+), 2 deletions(-)
This was part of the ref-transactions-rename series[1],
but it makes sense to send this one as a separate patch.
Thanks,
Stefan
[1] https://www.mail-archive.com/git@vger.kernel.org/msg60604.html
diff --git a/refs.c b/refs.c
index 5ff457e..0347328 100644
--- a/refs.c
+++ b/refs.c
@@ -2318,6 +2318,7 @@ static struct ref_lock *lock_ref_sha1_basic(const char
*refname,
lock-lock_fd = hold_lock_file_for_update(lock-lk, ref_file, lflags);
if (lock-lock_fd 0) {
+ last_errno = errno;
if (errno == ENOENT --attempts_remaining 0)
/*
* Maybe somebody just deleted one of the
@@ -2325,8 +2326,13 @@ static struct ref_lock *lock_ref_sha1_basic(const char
*refname,
* again:
*/
goto retry;
- else
- unable_to_lock_die(ref_file, errno);
+ else {
+ struct strbuf err = STRBUF_INIT;
+ unable_to_lock_message(ref_file, errno, err);
+ error(%s, err.buf);
+ strbuf_reset(err);
+ goto error_return;
+ }
}
return old_sha1 ? verify_lock(lock, old_sha1, mustexist) : lock;
--
2.2.0.rc2.5.gf7b9fb2
--
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] refs.c: use a stringlist for repack_without_refs
Junio C Hamano gits...@pobox.com writes: Stefan Beller sbel...@google.com writes: This patch was heavily inspired by a part of the ref-transactions-rename series[1], but people tend to dislike large series and this part is relatively easy to take out and unrelated, so I'll send it as a single patch. This patch doesn't intend any functional changes. It is just a refactoring, which replaces a char** array by a stringlist in the function repack_without_refs. [1] https://www.mail-archive.com/git@vger.kernel.org/msg60604.html Idea-by: Ronnie Sahlberg sahlb...@google.com Signed-off-by: Stefan Beller sbel...@google.com --- builtin/remote.c | 22 +++--- refs.c | 41 - refs.h | 3 +-- 3 files changed, 28 insertions(+), 38 deletions(-) In one codepath we were already using a string_list delete_refs_list anyway, so it makes sense to reuse that by movingan existing call to string_list_insert() a bit higher, instead of maintaining another array of pointers delete_refs[] to strings. OK, it simplifies the code by reducing the line count, which is a plus ;-) Sounds good. I queued this but as I suspected yesterday had to drop all the other rs/ref-transaction-* topics that are not in 'next' yet. I am guessing that your plan is to make them come back one piece at a time in many easier-to-digest bite sized series. -- 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] refs.c: use a stringlist for repack_without_refs
Stefan Beller wrote:
This patch was heavily inspired by a part of the ref-transactions-rename
series[1], but people tend to dislike large series and this part is
relatively easy to take out and unrelated, so I'll send it as a single
patch.
[1] https://www.mail-archive.com/git@vger.kernel.org/msg60604.html
The above is a useful kind of comment to put below the three-dashes. It
doesn't explain what the intent behind the patch is, why I should want
this patch when considering whether to upgrade git, or what is going to
break when I consider reverting it as part of fixing something else, so
it doesn't belong in the commit message.
This patch doesn't intend any functional changes. It is just a refactoring,
which replaces a char** array by a stringlist in the function
repack_without_refs.
Thanks. Why, though? Is it about having something simpler to pass
from builtin/remote.c::remove_branches(), or something else?
Idea-by: Ronnie Sahlberg sahlb...@google.com
Signed-off-by: Stefan Beller sbel...@google.com
Isn't the patch by Ronnie?
Sometimes I send a patch by someone else and make some change that I
don't want them to be blamed for. Then I keep their sign-off and put
a note in the commit message about the change I made. See output from
git log origin/pu --grep='jc:'
for more examples of that.
Some nits below.
--- a/builtin/remote.c
+++ b/builtin/remote.c
[...]
@@ -1325,6 +1319,11 @@ static int prune_remote(const char *remote, int
dry_run)
[...]
memset(states, 0, sizeof(states));
get_remote_ref_states(remote, states, GET_REF_STATES);
+ for (i = 0; i states.stale.nr; i++)
+ string_list_insert(delete_refs_list,
+states.stale.items[i].util);
+
+
if (states.stale.nr) {
(style) The double blank line looks odd here.
printf_ln(_(Pruning %s), remote);
printf_ln(_(URL: %s),
@@ -1332,24 +1331,17 @@ static int prune_remote(const char *remote, int
dry_run)
? states.remote-url[0]
: _((no URL)));
- delete_refs = xmalloc(states.stale.nr * sizeof(*delete_refs));
Now that there's no delete_refs array duplicating the string list,
would it make sense to rename delete_refs_list to delete_refs?
As a nice side-effect, that would make the definition of
delete_refs_list and other places it is used appear in the patch.
for (i = 0; i states.stale.nr; i++) {
const char *refname = states.stale.items[i].util;
(optional) this could be
for_each_string_list_item(ref, delete_refs_list) {
const char *refname = ref-string;
...
which saves the reader from having to remember what states.stale.items
means.
[...]
+++ b/refs.c
[...]
@@ -2639,23 +2639,23 @@ int repack_without_refs(struct string_list *without,
struct strbuf *err)
[...]
- int i, ret, removed = 0;
+ int count, ret, removed = 0;
assert(err);
- /* Look for a packed ref */
The old code has comments marking sections of the function:
/* Look for a packed ref */
/* Avoid processing if we have nothing to do */
/* Remove refnames from the cache */
/* Remove any other accumulated cruft */
/* Write what remains */
Is dropping this comment intended?
- for (i = 0; i n; i++)
- if (get_packed_ref(refnames[i]))
- break;
+ count = 0;
+ for_each_string_list_item(ref_to_delete, without)
+ if (get_packed_ref(ref_to_delete-string))
+ count++;
The old code breaks out early as soon as it finds a ref to delete.
Can we do similar?
E.g.
for (i = 0; i without-nr; i++)
if (get_packed_ref(without-items[i].string))
break;
(not about this patch) Is refs_to_delete leaked?
[...]
@@ -3738,10 +3738,11 @@ static int ref_update_reject_duplicates(struct
ref_update **updates, int n,
int ref_transaction_commit(struct ref_transaction *transaction,
struct strbuf *err)
{
- int ret = 0, delnum = 0, i;
- const char **delnames;
+ int ret = 0, i;
int n = transaction-nr;
struct ref_update **updates = transaction-updates;
+ struct string_list refs_to_delete = STRING_LIST_INIT_DUP;
The old code doesn't xstrdup the list items, so _NODUP should work
fine (and be slightly more efficient).
[...]
@@ -3815,16 +3813,17 @@ int ref_transaction_commit(struct ref_transaction
*transaction,
}
if (!(update-flags REF_ISPRUNING))
- delnames[delnum++] = update-lock-ref_name;
+ string_list_insert(refs_to_delete,
+update-lock-ref_name);
string_list_append would be analagous to the old code.
[]
--- a/refs.h
+++ b/refs.h
@@ -163,8 +163,7 @@
Re: [PATCH] refs.c: use a stringlist for repack_without_refs
On Tue, Nov 18, 2014 at 3:45 PM, Jonathan Nieder jrnie...@gmail.com wrote:
The above is a useful kind of comment to put below the three-dashes. It
doesn't explain what the intent behind the patch is, why I should want
this patch when considering whether to upgrade git, or what is going to
break when I consider reverting it as part of fixing something else, so
it doesn't belong in the commit message.
Yes, I'll do a resend, removing this paragraph.
This patch doesn't intend any functional changes. It is just a refactoring,
which replaces a char** array by a stringlist in the function
repack_without_refs.
Thanks. Why, though? Is it about having something simpler to pass
from builtin/remote.c::remove_branches(), or something else?
Essentially it's simpler to read and maintain as we're having less
lines of code.
I'll add that to the commit message instead.
Idea-by: Ronnie Sahlberg sahlb...@google.com
Signed-off-by: Stefan Beller sbel...@google.com
Isn't the patch by Ronnie?
As it was part of the ref-transaction-rename series, it was authored by Ronnie.
Porting it back to the master branch brought up so many conflicts,
that I decided
to rewrite it from scratch while having an occasional look at the
original patch.
If you want we can retain Ronnies authorship, however I may have messed up
the rewriting, so I put my name as author and Ronnie as giving the idea.
Sometimes I send a patch by someone else and make some change that I
don't want them to be blamed for. Then I keep their sign-off and put
a note in the commit message about the change I made. See output from
Sounds reasonable, I can do something similar.
git log origin/pu --grep='jc:'
for more examples of that.
Some nits below.
Because of the nits, I'd rather be blamed. :)
--- a/builtin/remote.c
+++ b/builtin/remote.c
[...]
@@ -1325,6 +1319,11 @@ static int prune_remote(const char *remote, int
dry_run)
[...]
memset(states, 0, sizeof(states));
get_remote_ref_states(remote, states, GET_REF_STATES);
+ for (i = 0; i states.stale.nr; i++)
+ string_list_insert(delete_refs_list,
+states.stale.items[i].util);
+
+
if (states.stale.nr) {
(style) The double blank line looks odd here.
will fix
printf_ln(_(Pruning %s), remote);
printf_ln(_(URL: %s),
@@ -1332,24 +1331,17 @@ static int prune_remote(const char *remote, int
dry_run)
? states.remote-url[0]
: _((no URL)));
- delete_refs = xmalloc(states.stale.nr * sizeof(*delete_refs));
Now that there's no delete_refs array duplicating the string list,
would it make sense to rename delete_refs_list to delete_refs?
As a nice side-effect, that would make the definition of
delete_refs_list and other places it is used appear in the patch.
for (i = 0; i states.stale.nr; i++) {
const char *refname = states.stale.items[i].util;
(optional) this could be
for_each_string_list_item(ref, delete_refs_list) {
const char *refname = ref-string;
...
which saves the reader from having to remember what states.stale.items
means.
done
[...]
+++ b/refs.c
[...]
@@ -2639,23 +2639,23 @@ int repack_without_refs(struct string_list *without,
struct strbuf *err)
[...]
- int i, ret, removed = 0;
+ int count, ret, removed = 0;
assert(err);
- /* Look for a packed ref */
The old code has comments marking sections of the function:
/* Look for a packed ref */
/* Avoid processing if we have nothing to do */
/* Remove refnames from the cache */
/* Remove any other accumulated cruft */
/* Write what remains */
Is dropping this comment intended?
no, dropped the dropping in the reroll.
- for (i = 0; i n; i++)
- if (get_packed_ref(refnames[i]))
- break;
+ count = 0;
+ for_each_string_list_item(ref_to_delete, without)
+ if (get_packed_ref(ref_to_delete-string))
+ count++;
The old code breaks out early as soon as it finds a ref to delete.
Can we do similar?
done
E.g.
for (i = 0; i without-nr; i++)
if (get_packed_ref(without-items[i].string))
break;
(not about this patch) Is refs_to_delete leaked?
[...]
@@ -3738,10 +3738,11 @@ static int ref_update_reject_duplicates(struct
ref_update **updates, int n,
int ref_transaction_commit(struct ref_transaction *transaction,
struct strbuf *err)
{
- int ret = 0, delnum = 0, i;
- const char **delnames;
+ int ret = 0, i;
int n = transaction-nr;
struct ref_update **updates = transaction-updates;
+ struct string_list refs_to_delete = STRING_LIST_INIT_DUP;
The old code doesn't xstrdup the list items, so _NODUP should work
fine (and be
Re: [PATCH] builtin/push.c: fix description of --recurse-submodules option
2014-11-19 1:57 GMT+08:00 Ralf Thielow ralf.thie...@gmail.com:
The description of the option for argument recurse-submodules
is marked for translation even if it expects the untranslated
string and it's missing the option on-demand which was introduced
in eb21c73 (2014-03-29, push: teach --recurse-submodules the on-demand
option). Fix this by unmark the string for translation and add the
missing option.
Signed-off-by: Ralf Thielow ralf.thie...@gmail.com
---
builtin/push.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/builtin/push.c b/builtin/push.c
index a076b19..cfa20c2 100644
--- a/builtin/push.c
+++ b/builtin/push.c
@@ -503,7 +503,7 @@ int cmd_push(int argc, const char **argv, const char
*prefix)
0, CAS_OPT_NAME, cas, N_(refname:expect),
N_(require old value of ref to be at this value),
PARSE_OPT_OPTARG, parseopt_push_cas_option },
- { OPTION_CALLBACK, 0, recurse-submodules, flags,
N_(check),
+ { OPTION_CALLBACK, 0, recurse-submodules, flags,
check|on-demand,
Yes, should not mark this for translation, and only two available options
for the --recurse-submodules flag.
The following code snippet is from builtin/push.c:
451 static int option_parse_recurse_submodules(const struct option *opt,
452const char *arg, int unset)
453 {
454 int *flags = opt-value;
455
456 if (*flags (TRANSPORT_RECURSE_SUBMODULES_CHECK |
457 TRANSPORT_RECURSE_SUBMODULES_ON_DEMAND))
458 die(%s can only be used once., opt-long_name);
459
460 if (arg) {
461 if (!strcmp(arg, check))
462 *flags |= TRANSPORT_RECURSE_SUBMODULES_CHECK;
463 else if (!strcmp(arg, on-demand))
464 *flags |= TRANSPORT_RECURSE_SUBMODULES_ON_DEMAND;
465 else
466 die(bad %s argument: %s, opt-long_name, arg);
467 } else
468 die(option %s needs an argument (check|on-demand),
469 opt-long_name);
470
471 return 0;
472 }
N_(control recursive pushing of submodules),
PARSE_OPT_OPTARG, option_parse_recurse_submodules },
OPT_BOOL( 0 , thin, thin, N_(use thin pack)),
--
2.2.0.rc2.258.gc851c5b
--
Jiang Xin
--
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: ag, **, and the GPL
On Wed, Nov 19, 2014 at 12:57 AM, David Turner dtur...@twopensource.com wrote: On Mon, 2014-11-17 at 20:50 -0800, Matthew Kaniaris wrote: The Silver Search (https://github.com/ggreer/the_silver_searcher), is a small, open source, cross platform searching utility written as a replacement for ack. One of the major benefits of Ag (and a source for much of its speed) is that it obeys .gitignore. However, Ag currently treats gitignores as regexs which produces incorrect results for e.g. **. I'd like to add support to ag to obey the .gitignore spec but I'm not keen on implementing yet another fnmatch clone. Ag is licensed under the Apache License Version 2.0 which to the best of my understanding is incompatible with the GPLv2. Would you grant me permission to reuse wildmatch.c (and necessary includes) for use in Ag? I already implemented this without using any git code at https://github.com/novalis/the_silver_searcher. The patch was rejected because it slowed down ag slightly (or perhaps because it was overly complex). Interesting. Do you have a direct link to that discussion (I don't know how to navigate that novalis link). Generally you (or ag) should avoid fnmatch/wildmatch whenever possible. Hitting those *match() _will_ slow things down (and git tries hard to avoid it). I had some optimizations on top of rsync wildmatch to handle * case better, but I don't think it'll make big difference in practice. -- 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: ag, **, and the GPL
On Wed, Nov 19, 2014 at 2:09 AM, Jonathan Nieder jrnie...@gmail.com wrote: Duy, what would you think of making git's wildmatch.c GPL v2 or v3, at your option? wildmatch.c is not really owned by me (latest change is from Anthony Ramine), but I would be ok with that. -- 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] git-new-workdir: Don't fail if the target directory is empty
On Tue, 2014-11-18 at 14:51 -0800, Junio C Hamano wrote: OK, thanks for digging. Let's go with this version, then. Thanks for your attention, Junio! -- 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] refs.c: use a stringlist for repack_without_refs
This patch doesn't intend any functional changes. It is just
a refactoring, which replaces a char** array by a stringlist
in the function repack_without_refs.
This is easier to read and maintain as it delivers the same
functionality with less lines of code less pointers.
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
Signed-off-by: Stefan Beller sbel...@google.com
-
This patch was heavily inspired by a part of the ref-transactions-rename
series[1], but people tend to dislike large series and this part is
relatively easy to take out and unrelated, so I'll send it as a single
patch.
[1] https://www.mail-archive.com/git@vger.kernel.org/msg60604.html
---
builtin/remote.c | 31 +++
refs.c | 40 +---
refs.h | 10 --
3 files changed, 40 insertions(+), 41 deletions(-)
diff --git a/builtin/remote.c b/builtin/remote.c
index 7f28f92..5f5fa4c 100644
--- a/builtin/remote.c
+++ b/builtin/remote.c
@@ -750,16 +750,11 @@ static int mv(int argc, const char **argv)
static int remove_branches(struct string_list *branches)
{
struct strbuf err = STRBUF_INIT;
- const char **branch_names;
int i, result = 0;
- branch_names = xmalloc(branches-nr * sizeof(*branch_names));
- for (i = 0; i branches-nr; i++)
- branch_names[i] = branches-items[i].string;
- if (repack_without_refs(branch_names, branches-nr, err))
+ if (repack_without_refs(branches, err))
result |= error(%s, err.buf);
strbuf_release(err);
- free(branch_names);
for (i = 0; i branches-nr; i++) {
struct string_list_item *item = branches-items + i;
@@ -1316,8 +1311,8 @@ static int prune_remote(const char *remote, int dry_run)
{
int result = 0, i;
struct ref_states states;
- struct string_list delete_refs_list = STRING_LIST_INIT_NODUP;
- const char **delete_refs;
+ struct string_list delete_refs = STRING_LIST_INIT_NODUP;
+ struct string_list_item *ref;
const char *dangling_msg = dry_run
? _( %s will become dangling!)
: _( %s has become dangling!);
@@ -1325,6 +1320,9 @@ static int prune_remote(const char *remote, int dry_run)
memset(states, 0, sizeof(states));
get_remote_ref_states(remote, states, GET_REF_STATES);
+ for_each_string_list_item(ref, delete_refs)
+ string_list_append(delete_refs, ref-string);
+
if (states.stale.nr) {
printf_ln(_(Pruning %s), remote);
printf_ln(_(URL: %s),
@@ -1332,23 +1330,16 @@ static int prune_remote(const char *remote, int dry_run)
? states.remote-url[0]
: _((no URL)));
- delete_refs = xmalloc(states.stale.nr * sizeof(*delete_refs));
- for (i = 0; i states.stale.nr; i++)
- delete_refs[i] = states.stale.items[i].util;
if (!dry_run) {
struct strbuf err = STRBUF_INIT;
- if (repack_without_refs(delete_refs, states.stale.nr,
- err))
+ if (repack_without_refs(delete_refs, err))
result |= error(%s, err.buf);
strbuf_release(err);
}
- free(delete_refs);
}
- for (i = 0; i states.stale.nr; i++) {
- const char *refname = states.stale.items[i].util;
-
- string_list_insert(delete_refs_list, refname);
+ for_each_string_list_item(ref, delete_refs) {
+ const char *refname = ref-string;
if (!dry_run)
result |= delete_ref(refname, NULL, 0);
@@ -1361,8 +1352,8 @@ static int prune_remote(const char *remote, int dry_run)
abbrev_ref(refname, refs/remotes/));
}
- warn_dangling_symrefs(stdout, dangling_msg, delete_refs_list);
- string_list_clear(delete_refs_list, 0);
+ warn_dangling_symrefs(stdout, dangling_msg, delete_refs);
+ string_list_clear(delete_refs, 0);
free_remote_ref_states(states);
return result;
diff --git a/refs.c b/refs.c
index 5ff457e..2f6e08b 100644
--- a/refs.c
+++ b/refs.c
@@ -2639,23 +2639,26 @@ static int curate_packed_ref_fn(struct ref_entry
*entry, void *cb_data)
return 0;
}
-int repack_without_refs(const char **refnames, int n, struct strbuf *err)
+int repack_without_refs(struct string_list *without, struct strbuf *err)
{
struct ref_dir *packed;
struct string_list refs_to_delete = STRING_LIST_INIT_DUP;
struct string_list_item *ref_to_delete;
- int i, ret, removed = 0;
+ int ret, needs_repacking = 0, removed = 0;
assert(err);
/* Look for a packed ref */
- for (i = 0; i n; i++)
- if
[PATCH] refs.c: handle locking failure during transaction better
From: Ronnie Sahlberg sahlb...@google.com
Change lock_ref_sha1_basic to return an error instead of dying,
when we fail to lock a file during a transaction. This function is
only called from transaction_commit() and it knows how to handle
these failures.
[sb: This was part of a larger patch series, cherry-picked to master]
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
Signed-off-by: Jonathan Nieder jrnie...@gmail.com
Signed-off-by: Stefan Beller sbel...@google.com
---
refs.c | 10 --
1 file changed, 8 insertions(+), 2 deletions(-)
diff --git a/refs.c b/refs.c
index 5ff457e..0347328 100644
--- a/refs.c
+++ b/refs.c
@@ -2318,6 +2318,7 @@ static struct ref_lock *lock_ref_sha1_basic(const char
*refname,
lock-lock_fd = hold_lock_file_for_update(lock-lk, ref_file, lflags);
if (lock-lock_fd 0) {
+ last_errno = errno;
if (errno == ENOENT --attempts_remaining 0)
/*
* Maybe somebody just deleted one of the
@@ -2325,8 +2326,13 @@ static struct ref_lock *lock_ref_sha1_basic(const char
*refname,
* again:
*/
goto retry;
- else
- unable_to_lock_die(ref_file, errno);
+ else {
+ struct strbuf err = STRBUF_INIT;
+ unable_to_lock_message(ref_file, errno, err);
+ error(%s, err.buf);
+ strbuf_reset(err);
+ goto error_return;
+ }
}
return old_sha1 ? verify_lock(lock, old_sha1, mustexist) : lock;
--
2.2.0.rc2.5.gf7b9fb2
--
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 0/4] error cleanups in lock_ref_sha1_basic
On Tue, Nov 18, 2014 at 05:13:17PM -0800, Stefan Beller wrote: From: Ronnie Sahlberg sahlb...@google.com Change lock_ref_sha1_basic to return an error instead of dying, when we fail to lock a file during a transaction. This function is only called from transaction_commit() and it knows how to handle these failures. [sb: This was part of a larger patch series, cherry-picked to master] Signed-off-by: Ronnie Sahlberg sahlb...@google.com Signed-off-by: Jonathan Nieder jrnie...@gmail.com Signed-off-by: Stefan Beller sbel...@google.com I think this is a good thing to do. I independently wrote the same patch recently, along with some other cleanups. Here's the series I ended up with (I added Ronnie as the author of the final one, which replaces this; even though my discovery was independent, he wrote it first :) ). [1/4]: error: save and restore errno [2/4]: lock_ref_sha1_basic: simplify errno handling [3/4]: lock_ref_sha1_basic: simplify error code path [4/4]: lock_ref_sha1_basic: do not die on locking errors -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
[PATCH 1/4] error: save and restore errno
It's common to use error() to return from a function, like:
if (open(...) 0)
return error(open failed);
Unfortunately this may clobber the errno from the open()
call. So we often end up with code like this:
if (open(...) 0) {
int saved_errno = errno;
error(open failed);
errno = saved_errno;
return -1;
}
which is less nice. Let's teach error() to save and restore
errno in each call, so that the original errno is preserved.
This is slightly less efficient for callers which do not
care, but error code paths are generally not performance
critical anyway.
Signed-off-by: Jeff King p...@peff.net
---
It's pretty minor to just handle errno in the callers, but I feel like
I've wanted this at least a dozen times, and it seems like it cannot
possibly hurt (i.e., I imagine there are callers where we _should_ be
doing the errno dance but have not realized it).
usage.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/usage.c b/usage.c
index ed14645..ee44d57 100644
--- a/usage.c
+++ b/usage.c
@@ -142,10 +142,13 @@ void NORETURN die_errno(const char *fmt, ...)
int error(const char *err, ...)
{
va_list params;
+ int saved_errno = errno;
va_start(params, err);
error_routine(err, params);
va_end(params);
+
+ errno = saved_errno;
return -1;
}
--
2.1.2.596.g7379948
--
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 3/4] lock_ref_sha1_basic: simplify error code path
For most errors, we jump to a goto label that unlocks the
ref and returns NULL. However, in none of these error paths
would we ever have actually locked the ref. By the time we
actually take the lock, we follow a different path that does
not ever hit this goto (we rely on verify_lock to unlock if
it finds an error).
We can just drop the goto completely and return NULL as
appropriate.
Signed-off-by: Jeff King p...@peff.net
---
refs.c | 12
1 file changed, 4 insertions(+), 8 deletions(-)
diff --git a/refs.c b/refs.c
index 169a46d..fafae7e 100644
--- a/refs.c
+++ b/refs.c
@@ -2260,7 +2260,7 @@ static struct ref_lock *lock_ref_sha1_basic(const char
*refname,
ref_file = git_path(%s, orig_refname);
if (remove_empty_directories(ref_file)) {
error(there are still refs under '%s', orig_refname);
- goto error_return;
+ return NULL;
}
refname = resolve_ref_unsafe(orig_refname, resolve_flags,
lock-old_sha1, type);
@@ -2270,7 +2270,7 @@ static struct ref_lock *lock_ref_sha1_basic(const char
*refname,
if (!refname) {
error(unable to resolve reference %s: %s,
orig_refname, strerror(errno));
- goto error_return;
+ return NULL;
}
missing = is_null_sha1(lock-old_sha1);
/* When the ref did not exist and we are creating it,
@@ -2281,7 +2281,7 @@ static struct ref_lock *lock_ref_sha1_basic(const char
*refname,
if (missing
!is_refname_available(refname, skip, get_packed_refs(ref_cache)))
{
errno = ENOTDIR;
- goto error_return;
+ return NULL;
}
lock-lk = xcalloc(1, sizeof(struct lock_file));
@@ -2309,7 +2309,7 @@ static struct ref_lock *lock_ref_sha1_basic(const char
*refname,
/* fall through */
default:
error(unable to create directory for %s, ref_file);
- goto error_return;
+ return NULL;
}
lock-lock_fd = hold_lock_file_for_update(lock-lk, ref_file, lflags);
@@ -2325,10 +2325,6 @@ static struct ref_lock *lock_ref_sha1_basic(const char
*refname,
unable_to_lock_die(ref_file, errno);
}
return old_sha1 ? verify_lock(lock, old_sha1, mustexist) : lock;
-
- error_return:
- unlock_ref(lock);
- return NULL;
}
struct ref_lock *lock_any_ref_for_update(const char *refname,
--
2.1.2.596.g7379948
--
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/4] lock_ref_sha1_basic: simplify errno handling
Now that error() does not clobber errno, we do not have to
take pains to save it ourselves.
Signed-off-by: Jeff King p...@peff.net
---
refs.c | 7 +--
1 file changed, 1 insertion(+), 6 deletions(-)
diff --git a/refs.c b/refs.c
index 5ff457e..169a46d 100644
--- a/refs.c
+++ b/refs.c
@@ -2232,7 +2232,6 @@ static struct ref_lock *lock_ref_sha1_basic(const char
*refname,
char *ref_file;
const char *orig_refname = refname;
struct ref_lock *lock;
- int last_errno = 0;
int type, lflags;
int mustexist = (old_sha1 !is_null_sha1(old_sha1));
int resolve_flags = 0;
@@ -2260,7 +2259,6 @@ static struct ref_lock *lock_ref_sha1_basic(const char
*refname,
*/
ref_file = git_path(%s, orig_refname);
if (remove_empty_directories(ref_file)) {
- last_errno = errno;
error(there are still refs under '%s', orig_refname);
goto error_return;
}
@@ -2270,7 +2268,6 @@ static struct ref_lock *lock_ref_sha1_basic(const char
*refname,
if (type_p)
*type_p = type;
if (!refname) {
- last_errno = errno;
error(unable to resolve reference %s: %s,
orig_refname, strerror(errno));
goto error_return;
@@ -2283,7 +2280,7 @@ static struct ref_lock *lock_ref_sha1_basic(const char
*refname,
*/
if (missing
!is_refname_available(refname, skip, get_packed_refs(ref_cache)))
{
- last_errno = ENOTDIR;
+ errno = ENOTDIR;
goto error_return;
}
@@ -2311,7 +2308,6 @@ static struct ref_lock *lock_ref_sha1_basic(const char
*refname,
goto retry;
/* fall through */
default:
- last_errno = errno;
error(unable to create directory for %s, ref_file);
goto error_return;
}
@@ -2332,7 +2328,6 @@ static struct ref_lock *lock_ref_sha1_basic(const char
*refname,
error_return:
unlock_ref(lock);
- errno = last_errno;
return NULL;
}
--
2.1.2.596.g7379948
--
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 4/4] lock_ref_sha1_basic: do not die on locking errors
From: Ronnie Sahlberg sahlb...@google.com
lock_ref_sha1_basic is inconsistent about when it calls
die() and when it returns NULL to signal an error. This is
annoying to any callers that want to recover from a locking
error.
This seems to be mostly historical accident. It was added in
4bd18c4 (Improve abstraction of ref lock/write.,
2006-05-17), which returned an error in all cases except
calling safe_create_leading_directories, in which case it
died. Later, 40aaae8 (Better error message when we are
unable to lock the index file, 2006-08-12) asked
hold_lock_file_for_update to die for us, leaving the
resolve_ref code-path the only one which returned NULL.
We tried to correct that in 5cc3cef (lock_ref_sha1(): do not
sometimes error() and sometimes die()., 2006-09-30),
by converting all of the die() calls into returns. But we
missed the die flag passed to the lock code, leaving us
inconsistent. This state persisted until e5c223e
(lock_ref_sha1_basic(): if locking fails with ENOENT, retry,
2014-01-18). Because of its retry scheme, it does not ask
the lock code to die, but instead manually dies with
unable_to_lock_die().
We can make this consistent with the other return paths by
converting this to use unable_to_lock_message(), and
returning NULL. This is safe to do because all callers
already needed to check the return value of the function,
since it could fail (and return NULL) for other reasons.
[jk: Added excessive history explanation and rebased on
nearby cleanups.]
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
Signed-off-by: Jonathan Nieder jrnie...@gmail.com
Signed-off-by: Stefan Beller sbel...@google.com
Signed-off-by: Jeff King p...@peff.net
---
It's a little sad to have to do the errno dance here after our earlier
cleanups. But while error() is safe, unable_to_lock_message is anything
but (the original patch from Ronnie did not need this, because it leaves
the goto and last_errno in place). So I dunno. Maybe my cleanups were
all for naught, and this end result is worse.
refs.c | 11 +--
1 file changed, 9 insertions(+), 2 deletions(-)
diff --git a/refs.c b/refs.c
index fafae7e..1f7e136 100644
--- a/refs.c
+++ b/refs.c
@@ -2321,8 +2321,15 @@ static struct ref_lock *lock_ref_sha1_basic(const char
*refname,
* again:
*/
goto retry;
- else
- unable_to_lock_die(ref_file, errno);
+ else {
+ int saved_errno = errno;
+ struct strbuf buf = STRBUF_INIT;
+ unable_to_lock_message(ref_file, errno, buf);
+ error(%s, buf.buf);
+ strbuf_release(buf);
+ errno = saved_errno;
+ return NULL;
+ }
}
return old_sha1 ? verify_lock(lock, old_sha1, mustexist) : lock;
}
--
2.1.2.596.g7379948
--
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/4] error: save and restore errno
This one makes my day.
A really good fix as it minimizes maintenance burden for checking
incoming patches for that pattern.
Reviewed-by: Stefan Beller sbel...@google.com
On Tue, Nov 18, 2014 at 5:37 PM, Jeff King p...@peff.net wrote:
It's common to use error() to return from a function, like:
if (open(...) 0)
return error(open failed);
Unfortunately this may clobber the errno from the open()
call. So we often end up with code like this:
if (open(...) 0) {
int saved_errno = errno;
error(open failed);
errno = saved_errno;
return -1;
}
which is less nice. Let's teach error() to save and restore
errno in each call, so that the original errno is preserved.
This is slightly less efficient for callers which do not
care, but error code paths are generally not performance
critical anyway.
Signed-off-by: Jeff King p...@peff.net
---
It's pretty minor to just handle errno in the callers, but I feel like
I've wanted this at least a dozen times, and it seems like it cannot
possibly hurt (i.e., I imagine there are callers where we _should_ be
doing the errno dance but have not realized it).
usage.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/usage.c b/usage.c
index ed14645..ee44d57 100644
--- a/usage.c
+++ b/usage.c
@@ -142,10 +142,13 @@ void NORETURN die_errno(const char *fmt, ...)
int error(const char *err, ...)
{
va_list params;
+ int saved_errno = errno;
va_start(params, err);
error_routine(err, params);
va_end(params);
+
+ errno = saved_errno;
return -1;
}
--
2.1.2.596.g7379948
--
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/4] error: save and restore errno
Jeff King wrote:
It's common to use error() to return from a function, like:
if (open(...) 0)
return error(open failed);
Unfortunately this may clobber the errno from the open()
call. So we often end up with code like this:
if (open(...) 0) {
int saved_errno = errno;
error(open failed);
errno = saved_errno;
return -1;
}
which is less nice.
What the above doesn't explain is why the caller cares about errno.
Are they going to print another message with strerror(errno)? Or are
they going to consider some errors non-errors (like ENOENT when trying
to unlink a file), in which case why is printing a message to stderr
okay?
All that said, given that there are real examples of code already
doing this, the patch seems sane.
[...]
usage.c | 3 +++
1 file changed, 3 insertions(+)
Reviewed-by: Jonathan Nieder jrnie...@gmail.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: [RFC] On watchman support
On Tue, Nov 18, 2014 at 01:26:56PM -0800, Junio C Hamano wrote: It is not check_refname_format() that is the real problem. It's the fact that we do O(# of refs) work whenever we have to access the packed-refs file. check_refname_format() is part of that, surely, but so is reading the file, creating all of the refname structs in memory, etc. (credit to peff@). Yeah, I'd agree very much with that. I am not sure if I am cc'd here because of my general complaining about packed-refs, or if I have said something clever on the subject. I did implement at one point a packed-refs reader that does a binary search on the mmap'd packed-refs file, and can return a single value or even locate the first entry matching a prefix (like refs/tags/) and iterate until we're out of the prefix. Unfortunately this runs very contrary to the caching design of the refs.c code. It is focused on caching _loose_ refs, where we may read an outer directory (like refs/), and would like to avoid descending into an inner directory (likes refs/foo/) unless we are interested in what is in it. But caching partial reads of packed-refs like this is inside out; we might read all of refs/tags/*, but have no clue what else is in refs/. So integrating it into refs.c would take pretty major surgery. -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 1/4] error: save and restore errno
On Tue, Nov 18, 2014 at 05:43:44PM -0800, Jonathan Nieder wrote:
Jeff King wrote:
It's common to use error() to return from a function, like:
if (open(...) 0)
return error(open failed);
Unfortunately this may clobber the errno from the open()
call. So we often end up with code like this:
if (open(...) 0) {
int saved_errno = errno;
error(open failed);
errno = saved_errno;
return -1;
}
which is less nice.
What the above doesn't explain is why the caller cares about errno.
Are they going to print another message with strerror(errno)? Or are
they going to consider some errors non-errors (like ENOENT when trying
to unlink a file), in which case why is printing a message to stderr
okay?
I guess the unsaid bit is:
Unfortunately this may clobber the errno from the open() call. Even
though error() sees the correct errno, the caller to which we are
returning may see a bogus errno value.
-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 2/4] lock_ref_sha1_basic: simplify errno handling
Jeff King wrote: Now that error() does not clobber errno, we do not have to take pains to save it ourselves. Signed-off-by: Jeff King p...@peff.net --- refs.c | 7 +-- 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/refs.c b/refs.c index 5ff457e..169a46d 100644 --- a/refs.c +++ b/refs.c @@ -2232,7 +2232,6 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname, All the caller to lock_ref_sha1_basic cares about is whether errno == ENOTDIR, since in that case we can print a message suggesting running git remote prune. Longer term, I suspect the caller (transaction_commit) should call is_refname_available to check for conflicting refs and return early to give git fetch a chance to print its advice. If a conflicting ref appears after that point, then just printing a reasonable error message is enough --- it is not so useful to give the 'remote prune' advice when people are doing funny things like running fetch in one terminal and a ref update creating a D/F conflict against that fetch in another terminal.[*] By the way, Stefan was mentioning the other day that it might make sense for transaction_commit to prevent a conflicting ref from appearing mid-transaction by locking 'refs/heads' in addition to 'refs/heads/master'. Anyway, in the meantime this is a nice cleanup. Reviewed-by: Jonathan Nieder jrnie...@gmail.com [*] If we want to handle that, the 'struct strbuf *err' could be replaced with a larger struct with room for structured data, like Junio was hinting at in another thread. -- 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 3/4] lock_ref_sha1_basic: simplify error code path
Jeff King wrote: For most errors, we jump to a goto label that unlocks the ref and returns NULL. However, in none of these error paths would we ever have actually locked the ref. By the time we actually take the lock, we follow a different path that does not ever hit this goto (we rely on verify_lock to unlock if it finds an error). [...] refs.c | 12 1 file changed, 4 insertions(+), 8 deletions(-) Wouldn't this leak lock (in all cases) and lock-ref_name and lock-orig_ref_name (on safe_create_leading_directories failure)? -- 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 4/4] lock_ref_sha1_basic: do not die on locking errors
Jeff King wrote: From: Ronnie Sahlberg sahlb...@google.com lock_ref_sha1_basic is inconsistent about when it calls die() and when it returns NULL to signal an error. This is annoying to any callers that want to recover from a locking error. And in addition to the modern transaction stuff, rename_ref() has been such a caller for a long time. Thanks for a pleasant explanation. Reviewed-by: Jonathan Nieder jrnie...@gmail.com [...] Signed-off-by: Ronnie Sahlberg sahlb...@google.com Signed-off-by: Jonathan Nieder jrnie...@gmail.com No need for my sign-off here --- it was just an artifact of my forwarding the patch to the list before. -- 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 3/4] lock_ref_sha1_basic: simplify error code path
On Tue, Nov 18, 2014 at 06:00:09PM -0800, Jonathan Nieder wrote: Jeff King wrote: For most errors, we jump to a goto label that unlocks the ref and returns NULL. However, in none of these error paths would we ever have actually locked the ref. By the time we actually take the lock, we follow a different path that does not ever hit this goto (we rely on verify_lock to unlock if it finds an error). [...] refs.c | 12 1 file changed, 4 insertions(+), 8 deletions(-) Wouldn't this leak lock (in all cases) and lock-ref_name and lock-orig_ref_name (on safe_create_leading_directories failure)? Ah, you're right. I totally missed that unlock_ref is not just about unlocking, but about free()ing. We do need to keep the goto/unlock. Hmph. Should we just abandon my series in favor of taking Ronnie's original patch, then? We can apply the save/restore errno in error() patch independently if we like. -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 3/4] lock_ref_sha1_basic: simplify error code path
Jeff King wrote: Hmph. Should we just abandon my series in favor of taking Ronnie's original patch, then? We can apply the save/restore errno in error() patch independently if we like. I liked patches 1 and 2 and the explanation from patch 4. Perhaps patch 3 should be replaced with a patch renaming unlock_ref to free_ref_lock or something. Thanks, Jonathan -- 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: Getting a commit sha1 from fast-import in a remote-helper
On Tue, Nov 18, 2014 at 12:11:47PM +0900, Mike Hommey wrote: Oh, so `ls dataref` would print out what dataref is? That would definitely help, although with the trick above, I probably wouldn't actually need it anymore. So, in the end, I was able to do everything with what's currently provided by git fast-import, but one thing would probably make life easier for me: being able to initialize a commit tree from a commit that's not one of the direct parents. Because the data I'm using gives diffs against possibly unrelated commits, and because starting a tree from scratch is actually slow when you have thousands of subdirectories, it would be easier if I could just start from that tree I have a diff against and apply the changes. Without this, there would be a lot of `ls` command emitting involved, and I'm actually not sure that wouldn't be as slow as starting from scratch (haven't implemented yet, so I can't tell). Also, I'm not sure how I'm supposed to know how much to read back from `ls`. Mike -- 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: Getting a commit sha1 from fast-import in a remote-helper
Mike Hommey wrote: So, in the end, I was able to do everything with what's currently provided by git fast-import, but one thing would probably make life easier for me: being able to initialize a commit tree from a commit that's not one of the direct parents. IIRC then 'M 04' wants a tree object, not a commit object, so you'd have to do ls commit M 04 tree -- 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: Getting a commit sha1 from fast-import in a remote-helper
On Tue, Nov 18, 2014 at 06:21:22PM -0800, Jonathan Nieder wrote: Mike Hommey wrote: So, in the end, I was able to do everything with what's currently provided by git fast-import, but one thing would probably make life easier for me: being able to initialize a commit tree from a commit that's not one of the direct parents. IIRC then 'M 04' wants a tree object, not a commit object, so you'd have to do ls commit M 04 tree That's what I'm planning to try ; Would doing: M 04 tree M 0644 blob some/path D other/path work? Or do I have to actually build a tree from the combination of the output from various ls and those filedelete/filemodify? Mike -- 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: Getting a commit sha1 from fast-import in a remote-helper
On Wed, Nov 19, 2014 at 11:27:59AM +0900, Mike Hommey wrote: On Tue, Nov 18, 2014 at 06:21:22PM -0800, Jonathan Nieder wrote: IIRC then 'M 04' wants a tree object, not a commit object, so you'd have to do ls commit M 04 tree That's what I'm planning to try ; Would doing: M 04 tree M 0644 blob some/path D other/path work? Yes, that should work fine. That's how contrib/svn-fe/svn-fe.c deals with 'Node-copyfrom-rev' in Subversion dumpfiles, for what it's worth. -- 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-fetch: default globally to --no-tags
Hi, I have one main concern, and I can envision a few ways that git could solve it. I also see that there has been some previous discussion on reltaed topics over the years, but as far as I can tell, nothing has actually been implemented to address my concern. --- My concern --- When I fetch from a remote repository, the only ways to prevent fetching tags are: 1) git fetch --no-tags name 2) git config remote.name.tagopt --no-tags; git fetch name The former requires extra typing for a case that (arguably) should be the default. The latter requires configuration for each repository. This doesn't scale well, if a user desires this behavior on all repositories they use. I'd prefer something like this, to change the default tag-fetching behavior globally: git config --global remote.tagopt --no-tags The closest I see is that I can disable tag-fetching globally on a per-remote name basis: git config --global remote.name.tagopt --no-tags This brings me to a potential bug report: --- Bug --- When trying to use the remote.name.tagopt configuration option globally, I get something like this: $ git config --global remote.test.tagopt --no-tags $ git remote update ... Fetching test fatal: 'test' does not appear to be a git repository fatal: Could not read from remote repository. Please make sure you have the correct access rights and the repository exists. error: Could not fetch test ... Expected behavior: if the local repository does not have a remote named 'test', then no additional output should be printed. If the local repository has a remote named test, then it should be fetched with the --no-tags option. Actual behavior: git prints warnings about the 'test' remote, just because there is no remote named 'test.' --- Motivations --- This is all motivated by the fact that tag namespacing is completely broken in git. Tags are globally namespaced, and in a true DVCS environment, any particular developer has no control over another developer's tag naming conventions. So this namespace can easily become polluted, reducing the usefulness of tags as a whole [1]. This problem seems to have been acknowledged, and proposals appeared a few years ago [2]. But I don't see any solution for tag namespacing. So, my current approach is to try to limit the damage done by accidentally fetching tags. Unfortunately, I can't find any good supported mechanisms to help me in the previous two sections. --- TL;DR --- My email boils down to two questions: (A) Has there been progress on implementing a proposal like in [2]? (B) Can we allow disabling (auto)tag-fetching globally? Like: git config --global remote.tagopt --no-tags Thanks, Brian [1] I could elaborate with some horror stories, to describe how this becomes a day-to-day nuisance in using git for me, but I'll avoid the details for now. Please accept that this is a usability bug. [2] http://article.gmane.org/gmane.comp.version-control.git/165799 http://article.gmane.org/gmane.comp.version-control.git/165885 -- 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
.gitignore sub-dir exclusions not overriding '*'
The behaviour below is seen with both git 1.8.3.2 and git 2.1.3; I am not subscribed to the vger list, please keep me in the CC list. Use-case: git repo which only holds PGP-encrypted files with a .asc extension, no matter which sub-directory they're in, or if in the top directory. Simple layout; for demo purposes names starting 'incl' should end up included, those starting 'excl' should end up excluded, but not based on those prefices: they're success result indicators; so: cd wherever git init mkdir foo touch incl.asc excl excl.txt foo/incl.asc foo/excl.txt foo/excl $EDITOR .gitignore Expected to work as .gitignore in top-level of repo: * !**/*.asc !.gitignore With that, `git status` ignores the contents of foo/ thusly: $ git check-ignore -v foo/incl.asc .gitignore:1:* foo/incl.asc Commenting out the '*' line and removing the '!' from the second, the **/*.asc clearly matches. The only way I can make this style work is to set the first line to '**/*.*' which fails to exclude the plain 'excl' files (no extension). It seems that there's some magic around '*' as the entire final path component of a pattern which causes it to match against the entire directory, and excludes of the directory can not be overriden by matches against '*.ext' within the directory, even when they come later in the same config file at the same precedence. This does not seem to my reading to match the behaviour described by `git help gitignore` (checked in both versions of git) and so seems to me to be a bug, but if it's a failure of my understanding, please help me to understand where I messed up. Thanks, -Phil -- 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
