Re: [PATCH 1/1] gitk: po/ru.po russian translation typo fixed
Hello Max and Paul, thank you for your feedback, so what's must be my next workflow? Resend patch with Reviewed-By:... or somethine else? -- Best regards. 0xAX -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] config: clear the executable bits (if any) on $GIT_DIR/config
On 11/16/2014 07:49 PM, Junio C Hamano wrote: Michael Haggerty mhag...@alum.mit.edu writes: There is no reason for $GIT_DIR/config to be executable, plus this change will help clean up repositories affected by the bug that was fixed by the previous commit. I do not think we want to do this. It is a welcome bugfix to create $GIT_DIR/config without executable bit when and only when we create it. It is very much in line with There is no reason for $GIT_DIR/config to be executable---we do not need to make it executable ourselves, so we shouldn't, but we did which was a bug we want to fix in patch 1/2 you posted. But with the preserve existing permissions fix we did earlier, the end users are now allowed to flip the executable bit on for their own purpose, and dropping it without knowing why they are doing so is simply rude. And honestly, Git do *not* even want to know why the users want to flip the bit. So I would suggest not to spend any cycle or any code complexity to repair existing repositories. Having that bit on does not hurt anybody. Those who found it curious can flip that bit off and then Git with preserve existing permissions fix will keep that bit off from then on. I disagree. The point of preserve existing permissions was to allow people to make their config files more readable/writable than the default, with the main use case being to help users who want to hide secret information in their config files. I think it is really far-fetched to imagine that anybody made his config file executable on purpose. Whereas we are *sure* that we have created lots of repositories with config files that were set executable by accident. Let's redefine the feature that was added in 2.1 from preserve existing permissions to preserve existing read/write permissions and thereby help people clean up the mess we made. That being said, I still believe that executable config files are not a significant risk in practice, so I'm not going to lose sleep about it either way. 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: [PATCH v2 2/2] config: clear the executable bits (if any) on $GIT_DIR/config
On 11/16/2014 09:06 AM, Johannes Sixt wrote:
Am 16.11.2014 um 08:21 schrieb Michael Haggerty:
@@ -559,9 +562,21 @@ int cmd_config(int argc, const char **argv, const char
*prefix)
if (given_config_source.blob)
die(editing blobs is not supported);
git_config(git_default_config, NULL);
-launch_editor(given_config_source.file ?
- given_config_source.file : git_path(config),
- NULL, NULL);
+config_file = xstrdup(given_config_source.file ?
+ given_config_source.file :
git_path(config));
+launch_editor(config_file, NULL, NULL);
+
+/*
+ * In git 2.1, there was a bug in git init that left
+ * the u+x bit set on the config file. To clean up any
+ * repositories affected by that bug, and just because
+ * it doesn't make sense for a config file to be
+ * executable anyway, clear any executable bits from
+ * the file (on a best effort basis):
+ */
+if (!lstat(config_file, st) (st.st_mode 0111))
At this point we cannot be sure that config_file is a regular file, can
we? It could also be a symbolic link. Wouldn't plain stat() be more
correct then?
You make a good point. But I'm a little nervous about following symlinks
and changing permissions on some distant file. Also, the bug that we are
trying clean up after would not have created a symlink in this place, so
I think the cleanup is not so important if config is a symlink.
So I suggest that we stick with lstat(), but add S_ISREG(st.st_mode) to
the chain above. Does that sound reasonable?
+chmod(config_file, st.st_mode 07666);
+free(config_file);
}
else if (actions == ACTION_SET) {
int ret;
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: [PATCH v2 1/2] create_default_files(): don't set u+x bit on $GIT_DIR/config
On 11/17/2014 02:40 AM, Eric Sunshine wrote:
On Sun, Nov 16, 2014 at 2:21 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. It is 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
---
Should this patch include a test in t1300 to ensure that this bug does
not resurface (and to prove that this patch indeed fixes it)?
builtin/init-db.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/builtin/init-db.c b/builtin/init-db.c
index 56f85e2..4c8021d 100644
--- a/builtin/init-db.c
+++ b/builtin/init-db.c
@@ -255,6 +255,7 @@ static int create_default_files(const char *template_path)
filemode = (!chmod(path, st1.st_mode ^ S_IXUSR)
!lstat(path, st2)
st1.st_mode != st2.st_mode);
+ filemode = !chmod(path, st1.st_mode);
}
git_config_set(core.filemode, filemode ? true : false);
--
Sorry for the late reply, I actually had prepared a complete different patch
for a different problem, but it touches the very same lines of code.
(And we may want to add a
!chmod(path, st1.st_mode ~S_IXUSR)
at the end of the operation)
There are systems when a file created with 666 have 766, and we
do not handle this situation yet.
Michael, if there is a chance that you integrate my small code changes
in your patch ?
-
commit 3228bedef6d45bfaf8986b6367f9388738476345
Author: Torsten Bögershausen tbo...@web.de
Date: Sun Oct 19 00:15:00 2014 +0200
Improve the filemode trustability check
Some file systems do not fully support the executable bit:
a) The user executable bit is always 0
b) The user executable bit is always 1
c) Is similar to b):
When a file is created with mode 0666 the file mode on disc is 766
and the user executable bit is 1 even if it should be 0 like b)
There are smbfs implementations where the file mode can be
maintained
locally and chmod -x changes the file mode from 0766 to 0666.
When the file system is unmounted and remounted,
the file mode is 0766 and executable bit is 1 again.
A typical example for a) is a VFAT drive mounted with -onoexec,
or cifs with -ofile_mode=0644.
b) is VFAT mounted with -oexec or cifs is mounted with -ofile_mode=0755
The behaviour described in c) has been observed when a Windows
machine with
NTFS exports a share via smb (or afp) to Mac OS X.
(CIFS is an enhanced version of SMB
The command to mount on the command line may differ,
e.g mount.cifs under Linux or mount_smbfs under Mac OS X)
Case a) and b) are detected by the current code.
Case c) qualifies as non trustable executable bit,
and core.filemode should be false by default.
Solution:
Detect when .git/config has the user executable bit set after
creat(.git/config, 0666) and set core.filemode to false.
diff --git a/builtin/init-db.c b/builtin/init-db.c
index 587a505..d3e4fb3 100644
--- a/builtin/init-db.c
+++ b/builtin/init-db.c
@@ -249,13 +249,11 @@ static int create_default_files(const char
*template_path)
strcpy(path + len, config);
/* Check filemode trustability */
-filemode = TEST_FILEMODE;
-if (TEST_FILEMODE !lstat(path, st1)) {
-struct stat st2;
-filemode = (!chmod(path, st1.st_mode ^ S_IXUSR)
-!lstat(path, st2)
-st1.st_mode != st2.st_mode);
-}
+filemode = TEST_FILEMODE
+!lstat(path, st1) !(st1.st_mode S_IXUSR)
+!chmod(path, st1.st_mode | S_IXUSR)
+!lstat(path, st1) (st1.st_mode S_IXUSR);
+
git_config_set(core.filemode, filemode ? true : false);
if (is_bare_repository())
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 1/2] create_default_files(): don't set u+x bit on $GIT_DIR/config
On 11/16/2014 08:08 PM, Junio C Hamano wrote: Michael Haggerty mhag...@alum.mit.edu writes: 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. It is somewhat odd to use the config file for this test, but whatever. The last sentence should read We could create a test file and use it for this purpose and then remove it, but config is a file we know exists at this point in the code (and it is the only file we know that exists), so it was a very sensible trick. Or remove it altogether. In other words, do not sound as if you do not know what you are doing in your log message. That would rob confidence in the change from the person who is reading git log output later. The sentence is not meant to rob confidence in this change, but rather to stimulate the reader's critical thinking about nearby code that I am *not* changing. By making this change without changing the function to use a temporary file for its chmod experiments, I might otherwise give future readers the impression that I like this shortcut, which I do not. For example, if the original code had used a temporary file rather than config, then we would never have had the bug that I'm fixing. The but whatever is meant to indicate that I don't disagree so strongly with the choice of tradeoffs made by the original author that I think it is worth changing. So maybe I am a coward (or lazy) for not proposing to change to using a temporary file instead. But since this patch is suggested for maint, I wanted to make the smallest change that would fix the bug. Feel free to delete the controversial sentence if you prefer. @@ -255,6 +255,7 @@ static int create_default_files(const char *template_path) filemode = (!chmod(path, st1.st_mode ^ S_IXUSR) !lstat(path, st2) st1.st_mode != st2.st_mode); +filemode = !chmod(path, st1.st_mode); Sounds good. You could also -chain this flip it back to the above statement. If filemode is not trustable on a filesytem, doing one extra chmod() to correct would not help us anyway, no? Yes, that would be better. I will fix it. 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: [PATCH v2 1/2] create_default_files(): don't set u+x bit on $GIT_DIR/config
On 11/17/2014 02:40 AM, Eric Sunshine wrote: On Sun, Nov 16, 2014 at 2:21 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. It is 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 --- Should this patch include a test in t1300 to ensure that this bug does not resurface (and to prove that this patch indeed fixes it)? This seems like a one-off bug caused by a specific instance of odd code. It could only recur if somebody were to remove the line that I added, which would be a *very* odd mistake to make given that its purpose is pretty obvious. I tested manually that this patch fixes the problem. Admittedly, my manual testing was only on one particular version of Linux. But it seems to me that the function being used is sufficiently portable to be trusted, and the fact that the same function is being used a few lines earlier suggests that any portability problems would have wider ramifications anyway. So in summary, I think the chance that such a test would ever catch a new problem is too small to justify the effort of writing and maintaining it. 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: [PATCH v2 1/2] create_default_files(): don't set u+x bit on $GIT_DIR/config
On 11/17/2014 10:08 AM, Torsten Bögershausen wrote: On 11/17/2014 02:40 AM, Eric Sunshine wrote: On Sun, Nov 16, 2014 at 2:21 AM, Michael Haggerty mhag...@alum.mit.edu wrote: [...] Sorry for the late reply, I actually had prepared a complete different patch for a different problem, but it touches the very same lines of code. (And we may want to add a !chmod(path, st1.st_mode ~S_IXUSR) at the end of the operation) There are systems when a file created with 666 have 766, and we do not handle this situation yet. Michael, if there is a chance that you integrate my small code changes in your patch ? Your change seems like a good idea, but is is logically separate from mine and I think it would be awkward to yoke them together. Both changes are short enough that conflict resolution shouldn't be a big problem. It will be easier for you to follow up on your patch if you submit it separately. The first question you get might be: Should it be applied to maint or is it more suitable for master?, which also hints at one of the reasons that yoking the patches together might be awkward. Regarding your patch itself: [...] b) The user executable bit is always 1 c) Is similar to b): When a file is created with mode 0666 the file mode on disc is 766 and the user executable bit is 1 even if it should be 0 like b) There are smbfs implementations where the file mode can be maintained locally and chmod -x changes the file mode from 0766 to 0666. When the file system is unmounted and remounted, the file mode is 0766 and executable bit is 1 again. It seems surprising that there isn't also a case (d) like (c) except that a file created with mode 0666 initially has the correct mode, and the executable bits can be maintained locally while the filesystem is mounted, but where executable bits are lost whenever the filesystem is unmounted and remounted. Does that case not arise, or is it simply that it is impossible to test for it in create_default_files()? 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: [PATCH 2/2] config: clear the executable bits (if any) on $GIT_DIR/config
Michael Haggerty mhag...@alum.mit.edu writes: On 11/16/2014 07:49 PM, Junio C Hamano wrote: ... So I would suggest not to spend any cycle or any code complexity to repair existing repositories. Having that bit on does not hurt anybody. Those who found it curious can flip that bit off and then Git with preserve existing permissions fix will keep that bit off from then on. I disagree. The point of preserve existing permissions was to allow people to make their config files more readable/writable than the default,... s/more/less/, I think, was the original motivation. Having to limit more tightly than usual was what made the config unusual among files under $GIT_DIR. If it were to loosen, Eric's change should not have been done in the first place. The sharedRepository setting to defeat the default umask is there for that kind of thing. That being said, I still believe that executable config files are not a significant risk ... It is merely an annoyance, to the same degree of annoyance we find when we see all files appear executable on a FAT floppy mounted on Linux ;-) I do not think it is a risk at all, and I do not see a point in going into people's repository and actively fixing it. People who notice can fix, and people who do not care do not care and are not harmed. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 1/2] create_default_files(): don't set u+x bit on $GIT_DIR/config
Michael Haggerty mhag...@alum.mit.edu writes:
This seems like a one-off bug caused by a specific instance of odd code.
It could only recur if somebody were to remove the line that I added,
which would be a *very* odd mistake to make given that its purpose is
pretty obvious.
Or some other code that comes _after_ your new code touches the
file.
You cannot anticipate what mistake others make.
Shouldn't something like this be sufficient?
t/t0001-init.sh | 7 +++
1 file changed, 7 insertions(+)
diff --git a/t/t0001-init.sh b/t/t0001-init.sh
index e62c0ff..acdc1df 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
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] config: clear the executable bits (if any) on $GIT_DIR/config
On 11/17/2014 04:33 PM, Junio C Hamano wrote: Michael Haggerty mhag...@alum.mit.edu writes: On 11/16/2014 07:49 PM, Junio C Hamano wrote: ... So I would suggest not to spend any cycle or any code complexity to repair existing repositories. Having that bit on does not hurt anybody. Those who found it curious can flip that bit off and then Git with preserve existing permissions fix will keep that bit off from then on. I disagree. The point of preserve existing permissions was to allow people to make their config files more readable/writable than the default,... s/more/less/, I think, was the original motivation. Having to limit more tightly than usual was what made the config unusual among files under $GIT_DIR. If it were to loosen, Eric's change should not have been done in the first place. The sharedRepository setting to defeat the default umask is there for that kind of thing. Oops, you are right. I actually meant to type less or more, but I see that more would be pretty useless. That being said, I still believe that executable config files are not a significant risk ... It is merely an annoyance, to the same degree of annoyance we find when we see all files appear executable on a FAT floppy mounted on Linux ;-) I do not think it is a risk at all, and I do not see a point in going into people's repository and actively fixing it. People who notice can fix, and people who do not care do not care and are not harmed. OK, then, I'll send a new copy of patch 1/2 and drop 2/2. 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: [PATCH v2 1/2] create_default_files(): don't set u+x bit on $GIT_DIR/config
On 11/17/2014 04:42 PM, Junio C Hamano wrote:
Michael Haggerty mhag...@alum.mit.edu writes:
This seems like a one-off bug caused by a specific instance of odd code.
It could only recur if somebody were to remove the line that I added,
which would be a *very* odd mistake to make given that its purpose is
pretty obvious.
Or some other code that comes _after_ your new code touches the
file.
You cannot anticipate what mistake others make.
And yet we do so all the time, adding tests for the things we consider
most likely to break in the future and omitting them for breakages that
seem unlikely. Otherwise, what frees us from the obligation to add a '!
test -x $GIT_DIR/config' following every single git operation?
But it's OK with me, we can add a test.
Shouldn't something like this be sufficient?
t/t0001-init.sh | 7 +++
1 file changed, 7 insertions(+)
diff --git a/t/t0001-init.sh b/t/t0001-init.sh
index e62c0ff..acdc1df 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
I think the logic should be
if test_have_prereq POSIXPERM test -x $1/config
, right? If the system doesn't have POSIXPERM, then aren't all bets off
regarding file permissions?
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
Doing a git add '' will add more files then expected
Hello, I first asked on stackoverflow (http://stackoverflow.com/questions/26933761/python-sh-module-and-git-try-to-add-more-files-then-in-command/26934517#26934517) about this behaviour. Then on the conversation that happened on the git-users mailing list other agreed that this behaviour is probably not as intended. Steps to reproduce: In bash (not sure this is bash specific) do: git add '' (that's to apostrophes, an empty argument) Results same as doing git add . Expected no files added or error about not finding file '' Hope this helps. Guilherme -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH/RFC] builtin: move builtin retrieval to get_builtin()
Slavomir Vlcek s...@inventati.org writes: I noticed that the patch has been modified (suggested 'static' scope modification, commit message) and added to the 'next' branch. So does this mean my task is done [...]? Even after the change hits 'next', other people may still find problems and rooms for improvements that you and those who reviewed the patch missed, and you may have to respond with follow-up patches on top. We'll usually give 3 days to a few weeks, depending on the complexity of the change, for that to happen and call it cooking the patch in 'next'. After that the change will be merged to 'master' so that it can appear in the next release. For now, the patch you sent is done, even though you can still send improvements on top if you want to. Thanks. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 1/2] create_default_files(): don't set u+x bit on $GIT_DIR/config
Michael Haggerty mhag...@alum.mit.edu writes: I think the logic should be if test_have_prereq POSIXPERM test -x $1/config , right? Yeah ;-) -- 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:
From 545c0d526eaa41f9306b567275a7d53799987482 Mon Sep 17 00:00:00 2001
From: Paul Smith p...@mad-scientist.net
Date: Fri, 14 Nov 2014 17:11:19 -0500
Subject: [PATCH] git-new-workdir: Don't fail if the target directory is empty
Please do not paste these in your mail message body. The first line
is there only so that the output looks like traditinal mbox format
(i.e. each of these act as a signal that a new message starts there
in the file), the other three are there only for rare cases in which
you want to override what your e-mail's headers say and is only used
when you are sending somebody else's patch.
Also provide more error checking and clean up on failure.
As the body of the log message is supposed to be complete by itself,
not a continuation of a half-sentence started on the Subject:
(i.e. consider the subject line to be the title of an article you
are writing), starting it with Also is strange.
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.
Thanks.
diff --git a/contrib/workdir/git-new-workdir b/contrib/workdir/git-new-workdir
index 75e8b25..c402000 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,55 @@ then
a complete repository.
fi
-# 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.
+
+# 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.
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 ;-)
+ fi
+ was_existing=true
+else
+ mkdir -p $new_workdir || failed
+ was_existing=false
fi
-# make sure the links use full paths
-git_dir=$(cd $git_dir; pwd)
+cleanup () {
+ if $was_existing
+ then
+ rm -rf $new_workdir/* $new_workdir/.[!.] $new_workdir/.??*
+ else
+ rm -rf $new_workdir
+ fi
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?
+}
+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?
- 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
+cd $new_workdir || failed
# copy the HEAD from the original repository as a default branch
-cp $git_dir/HEAD .git/HEAD
+cp $git_dir/HEAD .git/HEAD || failed
+
+# don't delete the new workdir on exit
+trap - $siglist
+
# checkout the branch (either the same as HEAD from the original repository,
or
# the one that was asked for)
git checkout -f $branch
--
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] cmd_config(): Make a copy of path obtained from git_path()
Michael Haggerty mhag...@alum.mit.edu writes: The strings returned by git_path() are recycled after a while. So make a copy of the config filename rather than holding onto the return value from git_path(). Good thinking. I agree that is an accident waiting to happen. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [GIT PULL] l10n updates for 2.2.0
Thanks. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Doing a git add '' will add more files then expected
Guilherme guibuf...@gmail.com writes: Steps to reproduce: In bash (not sure this is bash specific) do: git add '' (that's to apostrophes, an empty argument) Results same as doing git add . Expected no files added or error about not finding file '' The argument to git add is a pathspec, and the empty pathspec matches all files. Andreas. -- Andreas Schwab, sch...@linux-m68k.org GPG Key fingerprint = 58CA 54C7 6D53 942B 1756 01D3 44D5 214B 8276 4ED5 And now for something completely different. -- 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: Doing a git add '' will add more files then expected
Andreas Schwab sch...@linux-m68k.org writes: The argument to git add is a pathspec, and the empty pathspec matches all files. Err, why does the empty pathspec match all files? Isn't that a bug? -- Matthieu Moy http://www-verimag.imag.fr/~moy/ -- 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 merge a b when a == b but neither == o is always a successful merge?
Given a repository setup thusly: $ git --version git version 2.2.0.rc2 git init . echo '0.0' version git add version git commit -m master for i in a b ; do git checkout -b $i master echo '0.1' version git commit -a -m leg $i done git checkout -b c master echo '0.2' version git commit -a -m leg c git checkout --detach a git merge c produces the expected edit conflict. git merge b produces a successful merge, as both branches perform the same work. For the body of content in question, this is a merge conflict. Git seems to have the hard-coded assumption otherwise. I had to change three source files to get the result I expected, and wasn't seeing any indications of parameterization. Am I missing some means of getting the results I need? Thanks! -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Introduce a hook to run after formatting patches
On Sun, Nov 16, 2014 at 10:40 AM, Junio C Hamano gits...@pobox.com wrote: Stefan Beller sbel...@google.com writes: +post-format-patch + + +This hook is called after format-patch created a patch and it is +invoked with the filename of the patch as the first parameter. Such an interface would not work well with --stdout mode, would it? And if this only works with output generated into the files, then $ git format-patch $range | xargs -n1 $your_post_processing_script would do the same without any change to Git, I would imagine. Ok I'll try to use these commands. So I would have to say that I am fairly negative on this change in the presented form. Ok, I definitely did not expect this patch to be accepted the way it is, but rather was just proposing an idea. The post-format-patch.sample hook file would be missing for example. An alternative design to implement this as a post-processing filter to work for both to individual files and to standard output stream output filter may be possible, but even in that case I am not sure if it is worth the churn. In general I'd look at post-anything hook that works locally with a great suspicion, so that may partly be where my comment above is coming from. I dunno. Git is very easy to use when importing from other VCS, because of all the helpers like high level git-svn or low level fast-import. Patches on mailing lists can also be easily imported as a commit, so I think having an exporting system in place with various options like custom post processing would help for some use cases. Anyway I'll just drop this patch and go with your suggestion. Thanks, Stefan -- 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
Junio C Hamano gits...@pobox.com writes: Stefan Beller sbel...@google.com writes: +post-format-patch + + +This hook is called after format-patch created a patch and it is +invoked with the filename of the patch as the first parameter. Such an interface would not work well with --stdout mode, would it? And if this only works with output generated into the files, then $ git format-patch $range | xargs -n1 $your_post_processing_script would do the same without any change to Git, I would imagine. So I would have to say that I am fairly negative on this change in the presented form. An alternative design to implement this as a post-processing filter to work for both to individual files and to standard output stream output filter may be possible, but even in that case I am not sure if it is worth the churn. In general I'd look at post-anything hook that works locally with a great suspicion, so that may partly be where my comment above is coming from. I dunno. Another reason, in addition to that this only works on the already created output files, why I find this particular design distasteful (I am not saying that there should be an easy way to drop cruft left by third-party systems such as Change-id: line) is because the mechanism the patch adds does not attempt to take advantage of being inside Git, so the xargs -n1 above is strictly an equivalent. You have a chance to make the life better for users, but not you are not doing so. The design of this feature could be made to allow the user to specify a filter to munge _ONLY_ the log message part. For example, just after logmsg_reencode() returns the proposed commit log message to msg in pretty.c::pretty_print_commit(), you can detect a request to use some external filter program and let the program munge the message. With such a design: * The external filter your users would write does not have to worry about limiting its damage only to the log message part, as it will never see the patch text part; and * The same mechanism would work just as well for --stdout mode. The former is what I mean by to take advantage of being inside. Incidentally, it falls into #2 of 5 valid reasons to admit a new hook [*1*]. [Reference] *1* http://thread.gmane.org/gmane.comp.version-control.git/232809/focus=71069 -- 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: Doing a git add '' will add more files then expected
Matthieu Moy matthieu@grenoble-inp.fr writes: Andreas Schwab sch...@linux-m68k.org writes: The argument to git add is a pathspec, and the empty pathspec matches all files. Err, why does the empty pathspec match all files? Isn't that a bug? That is debatable. cd Documentation git add a would be equivalent to typing git add Documentation/a so cd Documentation git add would be equivalent to typing git add Documentation/ And doing the same from the top-level of the working tree can be argued to be a natural extension. -- 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
Junio C Hamano gits...@pobox.com writes: (I am not saying that there should be an easy way to drop cruft left by third-party systems such as Change-id: line) ... Heh, that was should not be, but I guess it was probably obvious. Sorry for the noise. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 01/22] dir.c: optionally compute sha-1 of a .gitignore file
On Sat, 2014-11-08 at 16:39 +0700, Nguyễn Thái Ngọc Duy wrote:
+ * If ss is not NULL, compute SHA-1 of the exclude file and fill
+ * stat data from disk (only valid if add_excludes returns zero). If
+ * ss_valid is non-zero, ss must contain good value as input.
ss and ss_valid should be sha1_stat and sha1_stat.valid
+struct sha1_stat {
+ struct stat_data stat;
+ unsigned char sha1[20];
+ int valid;
+};
It might be good to document what valid means here e.g. a sha1_stat
is valid if both sha1 and stat_data match the working tree's version of
the file or whatever.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 02/22] untracked cache: record .gitignore information and dir hierarchy
On Sat, 2014-11-08 at 16:39 +0700, Nguyễn Thái Ngọc Duy wrote:
+ d = xmalloc(sizeof(*d) + len);
+ memset(d, 0, sizeof(*d) + len);
+ memcpy(d-name, name, len);
calloc instead of malloc+memset? But do we really need this memset to
include name if we're about to use a memcpy? Couldn't we just add a
trailing zero?
+ * - The list of files and directories of the direction in question
s/direction/directory/
+struct untracked_cache_dir {
+ struct untracked_cache_dir **dirs;
+ char **untracked;
+ struct stat_data stat_data;
+ unsigned int untracked_alloc, dirs_nr, dirs_alloc;
+ unsigned int untracked_nr;
+ unsigned int check_only : 1;
+ /* null SHA-1 means this directory does not have .gitignore */
+ unsigned char exclude_sha1[20];
+ char name[1];
For consistency, should this be char name[FLEX_ARRAY]? (this will entail
some changes when allocating these, of course)
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: git merge a b when a == b but neither == o is always a successful merge?
On Mon, Nov 17, 2014 at 01:39:43PM -0500, Daniel Hagerty wrote: git merge b produces a successful merge, as both branches perform the same work. Just to be clear, you were expecting git merge b to produce a conflict? For the body of content in question, this is a merge conflict. Git seems to have the hard-coded assumption otherwise. I had to change three source files to get the result I expected, and wasn't seeing any indications of parameterization. I can imagine there might be times you would like to notice this case and visit it manually (e.g., even though the conflict would show both sides with the same content, you might want the resolution to take the two sides sequentially, duplicating them). But there are also cases where choosing the new content is helpful (e.g., one side cherry-picks a commit from the other, then later merges; you would not want to see a conflict there). Am I missing some means of getting the results I need? Thanks! I don't think there is an easy way to get what you want. You would have to write a new merge 3-way strategy that handles this case differently. And most of the file-level heavy lifting in merge strategies is done by the low-level unpack_trees code, which handles this case. From git help read-tree, which describes the index-level 3-way merge: · stage 2 and 3 are the same; take one or the other (it makes no difference - the same work has been done on our branch in stage 2 and their branch in stage 3) So I think you would have to add an option to git to handle this, unless you want to reimplement quite a lot of code in your merge strategy. -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: Fwd: Add git ignore as builtin
On Mon, Nov 17, 2014 at 12:12:25AM +, Ryan Jacobs wrote: Alberto Fanjul Alonso albertofanjul at gmail.com writes: git ignore whatever adds whatever to .git/info/exclude This should be git exclude not git ignore. Difference between the two: http://stackoverflow.com/questions/10066749/git- excludes-vs-ignores I am not sure that the name difference is all that meaningful. Yes, we call the repo-wide file .git/info/exclude and the in-tree ones .gitignore, but I do not know if the distinction is more than historical accident. I'd second the notion of a git ignore, however it would have to modify the `.gitignore` not `.git/info/exclude`. And I think this is a good reason why we do not have a git ignore tool to write such things. If I say git ignore foo should it go into .git/info/exclude or .gitignore? If the latter, should it be foo to match everywhere, or /foo to match only the single path at the root? If the file is subdir/foo, should it go as /subdir/foo into the top-level .gitignore, or as foo into subdir/.gitignore? If you ignore foo.o and bar.o, should we suggest that you ignore *.o instead? Trying to accomodate all of those possibilities in a command-line tool is hard, and probably counter-productive. We already have a simple domain-specific language for specifying .gitignore files. You can just try to cover a common case, like always put the full slash-prefixed path into the top-level .gitignore. But then I wonder if git ignore is really adding much value, as it is just a thin wrapper around echo. -Peff PS The more interesting case to automate (to me, anyway) is _checking_ paths against the hand-written .gitignore rules, which is complicated to do by hand. You can do that already with git check-ignore. -- 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: How safe are signed git tags? Only as safe as SHA-1 or somehow safer?
On Sun, Nov 16, 2014 at 03:31:10PM +, Patrick Schleizer wrote: How safe are signed git tags? Especially because git uses SHA-1. There is contradictory information around. So if one verifies a git tag (`git tag -v tagname`), then `checksout`s the tag, and checks that `git status` reports no untracked/modified files, without further manually auditing the code, how secure is this actually? Is it only as safe as SHA-1? Yes, it is only as safe as SHA-1 in the sense that you have GPG-signed only a SHA-1 hash. If somebody can find a collision with a hash you have signed, they can substitute the colliding data for the data you signed. Of course, safe as SHA-1 and find a collision are vague. If pre-image attacks are feasible (i.e., given already-published SHA-1, I can find a different input with the same SHA-1), then attacks are trivial. But when people talk about attacks on SHA-1, they are usually referring to finding a collision between two new pieces of data. You can also use that in an attack, but it's much less straightforward (basically, you need to get somebody to sign one of the colliding pieces of data and then replace it with the other). And of course there is the question of getting the colliding data to the victim. Git does collision checks whenever a remote (e.g., from a git fetch) gives us data that we already have. So you could poison new cloners with bad data, but you could not convince a repository with the existing good half of the collision to fetch the evil half. Git uses SHA-1 not for security And goes on. The security parts are elsewhere Could you please elaborate on this? Where are the security parts? Can you please briefly explain how these work? Where can I read more about this? I cannot speak for Linus, but I would not agree that SHA-1 is not part of Git's security model. If we consider the GPG signature as a black box (and we largely do in Git), then we _never_ sign the tree contents itself. We always sign the SHA-1 of the tree, along with some metadata (whether you are signing a tag or a commit). If an attacker can create SHA-1 collisions (either by pre-image, or if you agree to sign a tree containing a potential collision from an attacker), then you are vulnerable to having the tree contents swapped out after the fact (and the signature still checking out). I am not sure that is what Linus is saying, though. In the paragraph you quote: The source control management system Git uses SHA-1 not for security but for ensuring that the data has not changed due to accidental corruption. Linus Torvalds has said, If you have disk corruption, if you have DRAM corruption, if you have any kind of problems at all, Git will notice them. It's not a question of if, it's a guarantee. You can have people who try to be malicious. They won't succeed. [...] Nobody has been able to break SHA-1, but the point is the SHA-1, as far as Git is concerned, isn't even a security feature. It's purely a consistency check. The security parts are elsewhere, so a lot of people assume that since Git uses SHA-1 and SHA-1 is used for cryptographically secure stuff, they think that, OK, it's a huge security feature. It has nothing at all to do with security, it's just the best hash you can get. [...] I guarantee you, if you put your data in Git, you can trust the fact that five years later, after it was converted from your hard disk to DVD to whatever new technology and you copied it along, five years later you can verify that the data you get back out is the exact same data you put in. [...] One of the reasons I care is for the kernel, we had a break in on one of the BitKeeper sites where people tried to corrupt the kernel source code repositories. [6] I think he is saying more SHA-1 is about data integrity, not about authenticity; if you want authenticity, that's elsewhere [handled by gpg]. Unsaid there is that you can't really have authenticity without the integrity, and that I think he was assuming that SHA-1 works (he says Nobody has been able to break SHA-1..). If (!) I understand Mike Gerwitz ([...] GNU [...]) 's opinion, his opinion is, that for best security each and every commit should be signed for best possible git verification security. I think it would depend on your threat model. You haven't defined best security. Even without a break in SHA-1, there is value to signing every commit versus signing just tags. If Linus signs a tag, all it says is I think this tree state and the history leading up to it is called v3.16. But Linus can lie all he likes about who made each commit, and what happened in each one. Signing commits is more about authenticating individual commits: who made them, what was the state they were based on, and what was the end state (and between the two, you can calculate the introduced changes). Of course, that comes with its own headaches, too. E.g., in mailing-list development, the patch is picked up and applied by a maintainer,
Re: [PATCH] gc: support temporarily preserving garbage
On Fri, Nov 14, 2014 at 03:01:05PM -0800, Junio C Hamano wrote: 23 files changed, 375 insertions(+), 7 deletions(-) [...] I am not sure if this much of code churn is warranted to work around issues that only happen on repositories on NFS servers that do not keep open-but-deleted files available. Is it an option to instead have a copy of repository locally off NFS? I think it is also not sufficient. This patch seems to cover only objects. But we assume that we can atomically rename() new versions of files into place whenever we like without disrupting existing readers. This is the case for ref updates (and packed-refs), as well as the index file. The destination end of the rename is an unlink() in disguise, and would be susceptible to the same problems. -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: Fwd: Add git ignore as builtin
Jeff King p...@peff.net writes: On Mon, Nov 17, 2014 at 12:12:25AM +, Ryan Jacobs wrote: Alberto Fanjul Alonso albertofanjul at gmail.com writes: git ignore whatever adds whatever to .git/info/exclude This should be git exclude not git ignore. Difference between the two: http://stackoverflow.com/questions/10066749/git- excludes-vs-ignores I am not sure that the name difference is all that meaningful. Yes, we call the repo-wide file .git/info/exclude and the in-tree ones .gitignore, but I do not know if the distinction is more than historical accident. It is merely a historical accident. -- 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] copy.c: make copy_fd preserve meaningful errno
From: Ronnie Sahlberg sahlb...@google.com
Update copy_fd to return a meaningful errno on failure and also
preserve the existing errno variable.
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
Signed-off-by: Jonathan Nieder jrnie...@gmail.com
Signed-off-by: Stefan Beller sbel...@google.com
---
As announced in [1], I'm going to take over the
ref-transactions-reflog series by Ronnie Sahlberg.
This patch was sent previously to the list as part of
that series[2], but it seems to be unrelated to me.
Thanks,
Stefan
[1] http://www.mail-archive.com/git@vger.kernel.org/msg61051.html
[2] http://www.spinics.net/lists/git/msg240784.html
copy.c | 15 ++-
1 file changed, 10 insertions(+), 5 deletions(-)
diff --git a/copy.c b/copy.c
index f2970ec..a8d366e 100644
--- a/copy.c
+++ b/copy.c
@@ -8,12 +8,17 @@ int copy_fd(int ifd, int ofd)
if (!len)
break;
if (len 0) {
- return error(copy-fd: read returned %s,
-strerror(errno));
+ int save_errno = errno;
+ error(copy-fd: read returned %s, strerror(errno));
+ errno = save_errno;
+ return -1;
+ }
+ if (write_in_full(ofd, buffer, len) 0) {
+ int save_errno = errno;
+ error(copy-fd: write returned %s, strerror(errno));
+ errno = save_errno;
+ return -1;
}
- if (write_in_full(ofd, buffer, len) 0)
- return error(copy-fd: write returned %s,
-strerror(errno));
}
return 0;
}
--
2.2.0.rc1.24.g562add4
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: git merge a b when a == b but neither == o is always a successful merge?
Just to be clear, you were expecting git merge b to produce a
conflict?
Yessir.
I can imagine there might be times you would like to notice this case
and visit it manually (e.g., even though the conflict would show both
sides with the same content, you might want the resolution to take the
two sides sequentially, duplicating them).
This is roughly the case here. Judgement is needed from the
person at the wheel.
I don't think there is an easy way to get what you want. You would have
to write a new merge 3-way strategy that handles this case differently.
And most of the file-level heavy lifting in merge strategies is done by
the low-level unpack_trees code, which handles this case. From git help
I have a very rough draft that seems to do what I want, exposed
through .gitattributes (below). Given that this is something you probably
want tightly scoped, does it make sense to expose it anywhere else?
Is it accurate to speak of this as exposing merge minimal?
Thanks!
commit 3a8bc89950576c0445167e4f5ee5f42d9d737d2d
Author: Daniel Hagerty h...@linnaean.org
Date: Mon Nov 17 15:42:04 2014 -0500
merge: expose XDL_MERGE_MINIMAL behavior:
When performing a 3-way merge, an identical change to a file in both
branches is not considered a conflict by default. There exist content
for which this isn't true; simultaneous, identical edits are a
conflict.
Expose the ability to perform a minimal merge for selective files as
directed by gitattributes.
diff --git a/ll-merge.c b/ll-merge.c
index da59738..2a06ac9 100644
--- a/ll-merge.c
+++ b/ll-merge.c
@@ -84,7 +84,16 @@ static int ll_xdl_merge(const struct ll_merge_driver
*drv_unused,
}
memset(xmp, 0, sizeof(xmp));
- xmp.level = XDL_MERGE_ZEALOUS;
+
+ struct git_attr_check check;
+ check.attr = git_attr(merge-minimal);
+ (void) git_check_attr(path, 1, check);
+
+ if(ATTR_TRUE(check.value))
+ xmp.level = XDL_MERGE_MINIMAL;
+ else
+ xmp.level = XDL_MERGE_ZEALOUS;
+
xmp.favor = opts-variant;
xmp.xpp.flags = opts-xdl_opts;
if (git_xmerge_style = 0)
diff --git a/merge-recursive.c b/merge-recursive.c
index 3efc04e..dac6252 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -924,7 +924,11 @@ static struct merge_file_info merge_file_1(struct
merge_options *o,
}
}
- if (sha_eq(a-sha1, b-sha1) || sha_eq(a-sha1, one-sha1))
+ struct git_attr_check check;
+ check.attr = git_attr(merge-minimal);
+ (void) git_check_attr(a-path, 1, check);
+
+ if (!ATTR_TRUE(check.value) (sha_eq(a-sha1, b-sha1) ||
sha_eq(a-sha1, one-sha1)))
hashcpy(result.sha, b-sha1);
else if (sha_eq(b-sha1, one-sha1))
hashcpy(result.sha, a-sha1);
diff --git a/unpack-trees.c b/unpack-trees.c
index cc616c3..627356a 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -1597,7 +1597,12 @@ int threeway_merge(struct cache_entry **stages, struct
unpack_trees_options *o)
if (head) {
/* #5ALT, #15 */
- if (same(head, remote))
+
+ struct git_attr_check check;
+ check.attr = git_attr(merge-minimal);
+ (void) git_check_attr(head-name, 1, check);
+
+ if (!ATTR_TRUE(check.value) same(head, remote))
return merged_entry(head, index, o);
/* #13, #3ALT */
if (!df_conflict_remote remote_match !head_match)
--
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: difftool: honor --trust-exit-code for builtin tools
At 10:11 -0800 16 Nov 2014, Junio C Hamano gits...@pobox.com wrote: It does not have any significance that a random shell implementation is not POSIX compliant. That would merely mean that such a shell cannot be used to run POSIX shell scripts like our Porcelain. Right, and I suspect that it's very rare for zsh to be used as /bin/sh. I've heard of people doing it just to see what would fail, but not of anybody doing that for regular use. I would suspect that zsh has more posixly correct mode, with which it _can_ run POSIX shell scripts, and I would imagine that this $status is an alias $? business is disabled in that mode? Yes, if zsh is invoked as either sh or ksh it attempts to emulate the usual semantics of the named shell. One of the differences is that $status isn't special in the emulation modes. -- 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
Merge without marking conflicts in working tree
Is there a way to do a merge but only record conflicts in the index, not update the working versions of files with conflict markers? Like many people, I use git to manage configuration files for my shell, editor, git itself, and a number of other things. The vast majority of times that I update things no conflicts are occur and everything just works, so I'd like to avoid extra work in this case. But occasionally a conflict will occur, and if it's in a file that will be read while trying to resolve the conflict this can make things more difficult. I'd like to find a way to have the conflict recorded in just the index without touching the working tree. I could then use my usual tools to resolve the conflict without the errors caused by the conflict markers. I generally use vim+fugitive to resolve conflicts anyway, and typically the first step I take is to replace the working-tree version with the merge-base version, completely ignoring any conflict markers. If there isn't currently a way to do this, I was thinking of implementing something like an ours value for merge.conflictstyle configuration. -- 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
Stefan Beller sbel...@google.com writes:
This patch was sent previously to the list as part of
that series[2], but it seems to be unrelated to me.
I am fine to queue obvious and trivial bits first before the larger
main course. For now I'll queue this one and also the series that
has been queued for a while, but at some point I suspect we would
have to drop the latter.
Thanks.
[1] http://www.mail-archive.com/git@vger.kernel.org/msg61051.html
[2] http://www.spinics.net/lists/git/msg240784.html
copy.c | 15 ++-
1 file changed, 10 insertions(+), 5 deletions(-)
diff --git a/copy.c b/copy.c
index f2970ec..a8d366e 100644
--- a/copy.c
+++ b/copy.c
@@ -8,12 +8,17 @@ int copy_fd(int ifd, int ofd)
if (!len)
break;
if (len 0) {
- return error(copy-fd: read returned %s,
- strerror(errno));
+ int save_errno = errno;
+ error(copy-fd: read returned %s, strerror(errno));
+ errno = save_errno;
+ return -1;
+ }
+ if (write_in_full(ofd, buffer, len) 0) {
+ int save_errno = errno;
+ error(copy-fd: write returned %s, strerror(errno));
+ errno = save_errno;
+ return -1;
}
- if (write_in_full(ofd, buffer, len) 0)
- return error(copy-fd: write returned %s,
- strerror(errno));
}
return 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: [PATCH] copy.c: make copy_fd preserve meaningful errno
I am reviewing the series and about to resend it with very minor nits
fixed. I just want to point out this fix is orthogonal to the series
and can be picked up no matter how long the reviewing/discussion of
the series goes.
Thanks,
Stefan
On Mon, Nov 17, 2014 at 3:08 PM, Junio C Hamano gits...@pobox.com wrote:
Stefan Beller sbel...@google.com writes:
This patch was sent previously to the list as part of
that series[2], but it seems to be unrelated to me.
I am fine to queue obvious and trivial bits first before the larger
main course. For now I'll queue this one and also the series that
has been queued for a while, but at some point I suspect we would
have to drop the latter.
Thanks.
[1] http://www.mail-archive.com/git@vger.kernel.org/msg61051.html
[2] http://www.spinics.net/lists/git/msg240784.html
copy.c | 15 ++-
1 file changed, 10 insertions(+), 5 deletions(-)
diff --git a/copy.c b/copy.c
index f2970ec..a8d366e 100644
--- a/copy.c
+++ b/copy.c
@@ -8,12 +8,17 @@ int copy_fd(int ifd, int ofd)
if (!len)
break;
if (len 0) {
- return error(copy-fd: read returned %s,
- strerror(errno));
+ int save_errno = errno;
+ error(copy-fd: read returned %s, strerror(errno));
+ errno = save_errno;
+ return -1;
+ }
+ if (write_in_full(ofd, buffer, len) 0) {
+ int save_errno = errno;
+ error(copy-fd: write returned %s, strerror(errno));
+ errno = save_errno;
+ return -1;
}
- if (write_in_full(ofd, buffer, len) 0)
- return error(copy-fd: write returned %s,
- strerror(errno));
}
return 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: Merge without marking conflicts in working tree
Aaron Schrab aa...@schrab.com writes: Is there a way to do a merge but only record conflicts in the index, not update the working versions of files with conflict markers? Not with Porcelain, but read-tree -m ancestor ours theirs should give you something close to it. merge-recursive is probably beyond salvaging and coaxing --cached option (i.e. tell a command that usually works both on the index and on the working tree to only work on the index) into it would be too messy to think about. Restructuring it so that it first computes the end result only in the index and then optionally allows it to check out to the working tree has been something I've wanted to do for a long time, but there is only limited amount of time in a day and the list is never dormant, so... X-. -- 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: Merge without marking conflicts in working tree
Aaron Schrab aa...@schrab.com writes: Is there a way to do a merge but only record conflicts in the index, not update the working versions of files with conflict markers? Like many people, I use git to manage configuration files for my shell, editor, git itself, and a number of other things. The vast majority of times that I update things no conflicts are occur and everything just works, so I'd like to avoid extra work in this case. But occasionally a conflict will occur, and if it's in a file that will be read while trying to resolve the conflict this can make things more difficult. You could perform the merge in a separate working directory. The only extra step required is a checkout to deploy the new revision, which could be triggered automatically by a hook. Andreas. -- Andreas Schwab, sch...@linux-m68k.org GPG Key fingerprint = 58CA 54C7 6D53 942B 1756 01D3 44D5 214B 8276 4ED5 And now for something completely different. -- 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
Hi,
Stefan Beller wrote:
This patch was sent previously to the list as part of
that series[2], but it seems to be unrelated to me.
Thanks. Good call.
[...]
From: Ronnie Sahlberg sahlb...@google.com
Update copy_fd to return a meaningful errno on failure and also
preserve the existing errno variable.
Some functions in git make errno meaningful on error and others don't.
In general, the more we only use errno immediately after a system
call, the better, so based on the above description this seems like a
step in the wrong direction.
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.
[...]
copy.c | 15 ++-
1 file changed, 10 insertions(+), 5 deletions(-)
Please also update the API documentation in cache.h so we remember not
to backslide in the future.
[...]
--- a/copy.c
+++ b/copy.c
@@ -8,12 +8,17 @@ int copy_fd(int ifd, int ofd)
if (!len)
break;
if (len 0) {
- return error(copy-fd: read returned %s,
- strerror(errno));
+ 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 error handling would be more explicit so
there would be no need to protect errno from clobbering by other
system calls (both here and in callers).
Something like this:
Signed-off-by: Jonathan Nieder jrnie...@gmail.com
diff --git i/cache.h w/cache.h
index 99ed096..ddaa30f 100644
--- i/cache.h
+++ w/cache.h
@@ -1479,7 +1479,7 @@ extern const char *git_mailmap_blob;
extern void maybe_flush_or_die(FILE *, const char *);
__attribute__((format (printf, 2, 3)))
extern void fprintf_or_die(FILE *, const char *fmt, ...);
-extern int copy_fd(int ifd, int ofd);
+extern int copy_fd(int ifd, int ofd, struct strbuf *err);
extern int copy_file(const char *dst, const char *src, int mode);
extern int copy_file_with_time(const char *dst, const char *src, int mode);
extern void write_or_die(int fd, const void *buf, size_t count);
diff --git i/convert.c w/convert.c
index 9a5612e..e301447 100644
--- i/convert.c
+++ w/convert.c
@@ -358,7 +358,11 @@ static int filter_buffer_or_fd(int in, int out, void *data)
if (params-src) {
write_err = (write_in_full(child_process.in, params-src,
params-size) 0);
} else {
- write_err = copy_fd(params-fd, child_process.in);
+ struct strbuf err = STRBUF_INIT;
+ write_err = copy_fd(params-fd, child_process.in, err);
+ if (write_err)
+ error(copy-fd: %s, err.buf);
+ strbuf_release(err);
}
if (close(child_process.in))
diff --git i/copy.c w/copy.c
index f2970ec..828661a 100644
--- i/copy.c
+++ w/copy.c
@@ -1,19 +1,22 @@
#include cache.h
-int copy_fd(int ifd, int ofd)
+int copy_fd(int ifd, int ofd, struct strbuf *err)
{
+ assert(err);
+
while (1) {
char buffer[8192];
ssize_t len = xread(ifd, buffer, sizeof(buffer));
if (!len)
break;
if (len 0) {
- return error(copy-fd: read returned %s,
-strerror(errno));
+ strbuf_addf(err, read returned %s, strerror(errno));
+ return -1;
+ }
+ if (write_in_full(ofd, buffer, len) 0) {
+ strbuf_addf(err, write returned %s, strerror(errno));
+ return -1;
}
- if (write_in_full(ofd, buffer, len) 0)
- return error(copy-fd: write returned %s,
-strerror(errno));
}
return 0;
}
@@ -33,7 +36,8 @@ static int copy_times(const char *dst, const char *src)
int copy_file(const char *dst, const char *src, int mode)
{
- int fdi, fdo, status;
+ int fdi, fdo;
+ struct strbuf err =
Re: [PATCH] gc: support temporarily preserving garbage
On 18 November 2014 08:34, Jeff King p...@peff.net wrote: I am not sure if this much of code churn is warranted to work around issues that only happen on repositories on NFS servers that do not keep open-but-deleted files available. Is it an option to instead have a copy of repository locally off NFS? I think it is also not sufficient. This patch seems to cover only objects. But we assume that we can atomically rename() new versions of files into place whenever we like without disrupting existing readers. This is the case for ref updates (and packed-refs), as well as the index file. The destination end of the rename is an unlink() in disguise, and would be susceptible to the same problems. I’m going out on a limb here as my NFS knowledge is rather shallow but a rename should be atomic even on NFS. The RENAME operation must be atomic to the client.” (https://www.ietf.org/rfc/rfc1813.txt: 3.3.14) Does git do something differently here for that not to be the case? -- 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
On Mon, Nov 17, 2014 at 3:35 PM, Jonathan Nieder jrnie...@gmail.com wrote:
Hi,
Stefan Beller wrote:
This patch was sent previously to the list as part of
that series[2], but it seems to be unrelated to me.
Thanks. Good call.
[...]
From: Ronnie Sahlberg sahlb...@google.com
Update copy_fd to return a meaningful errno on failure and also
preserve the existing errno variable.
Some functions in git make errno meaningful on error and others don't.
In general, the more we only use errno immediately after a system
call, the better, so based on the above description this seems like a
step in the wrong direction.
I did reword the commit message badly. Before it just read
Update copy_fd to return a meaningful errno.
In fact the proposed patch doesn't guarantee the errno, which is set
at the beginning of the function to be the same at the end.
What it really should preserve is the errno from xread, while
evaluating error(copy-fd: read returned %s, strerror(errno));
So the function call of error(...) or strerror(...) may change the errno.
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.
[...]
copy.c | 15 ++-
1 file changed, 10 insertions(+), 5 deletions(-)
Please also update the API documentation in cache.h so we remember not
to backslide in the future.
[...]
--- a/copy.c
+++ b/copy.c
@@ -8,12 +8,17 @@ int copy_fd(int ifd, int ofd)
if (!len)
break;
if (len 0) {
- return error(copy-fd: read returned %s,
- strerror(errno));
+ 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 error handling would be more explicit so
there would be no need to protect errno from clobbering by other
system calls (both here and in callers).
Something like this:
I like this approach, though your patch is not about the original
intention as this one
(having strerror(...) not messing with the errno), but rather
accumulating the errors not
in numbers but string buffers?
Signed-off-by: Jonathan Nieder jrnie...@gmail.com
diff --git i/cache.h w/cache.h
index 99ed096..ddaa30f 100644
--- i/cache.h
+++ w/cache.h
@@ -1479,7 +1479,7 @@ extern const char *git_mailmap_blob;
extern void maybe_flush_or_die(FILE *, const char *);
__attribute__((format (printf, 2, 3)))
extern void fprintf_or_die(FILE *, const char *fmt, ...);
-extern int copy_fd(int ifd, int ofd);
+extern int copy_fd(int ifd, int ofd, struct strbuf *err);
extern int copy_file(const char *dst, const char *src, int mode);
extern int copy_file_with_time(const char *dst, const char *src, int mode);
extern void write_or_die(int fd, const void *buf, size_t count);
diff --git i/convert.c w/convert.c
index 9a5612e..e301447 100644
--- i/convert.c
+++ w/convert.c
@@ -358,7 +358,11 @@ static int filter_buffer_or_fd(int in, int out, void
*data)
if (params-src) {
write_err = (write_in_full(child_process.in, params-src,
params-size) 0);
} else {
- write_err = copy_fd(params-fd, child_process.in);
+ struct strbuf err = STRBUF_INIT;
+ write_err = copy_fd(params-fd, child_process.in, err);
+ if (write_err)
+ error(copy-fd: %s, err.buf);
+ strbuf_release(err);
}
if (close(child_process.in))
diff --git i/copy.c w/copy.c
index f2970ec..828661a 100644
--- i/copy.c
+++ w/copy.c
@@ -1,19 +1,22 @@
#include cache.h
-int copy_fd(int ifd, int ofd)
+int copy_fd(int ifd, int ofd, struct strbuf *err)
{
+ assert(err);
+
while (1) {
char buffer[8192];
ssize_t len = xread(ifd, buffer, sizeof(buffer));
if (!len)
break;
if (len 0) {
- return error(copy-fd: read returned %s,
-
Re: [PATCH] gc: support temporarily preserving garbage
On Tue, Nov 18, 2014 at 10:59:14AM +1100, Stefan Saasen wrote: I am not sure if this much of code churn is warranted to work around issues that only happen on repositories on NFS servers that do not keep open-but-deleted files available. Is it an option to instead have a copy of repository locally off NFS? I think it is also not sufficient. This patch seems to cover only objects. But we assume that we can atomically rename() new versions of files into place whenever we like without disrupting existing readers. This is the case for ref updates (and packed-refs), as well as the index file. The destination end of the rename is an unlink() in disguise, and would be susceptible to the same problems. I’m going out on a limb here as my NFS knowledge is rather shallow but a rename should be atomic even on NFS. The RENAME operation must be atomic to the client.” (https://www.ietf.org/rfc/rfc1813.txt: 3.3.14) Does git do something differently here for that not to be the case? I don't mean the atomicity of the rename itself. But rather what happens to an existing file at the destination of the rename, and processes that have it open. E.g., consider this sequence of events: 1. Process A calls open(index, O_RDONLY). Possibly it also mmap()s the result. 2. Process B calls open(index.lock, O_WRONLY|O_CREAT|O_EXCL), write()s some data to it, and close()s it. 3. Process B calls rename(index.lock, index); Normally, process A's descriptor continues to point to the old index, and it does not see the new version unless it calls open() again. But on NFS, what happens to process A when it tries to read? I could imagine one of: a. It acts like the unlink call under discussion. The old file has gone away, and anybody who had it mmap'd is going to SIGBUS. b. We silently read data from the replacement file. This is bad, because we may be in the middle of reading a data structure. We expect to get an atomic view of the file once we've opened it. I don't know which happens, or if it all just works. But it seems like another potential problem point of the same type. -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: [RFC] On watchman support
On Tue, 2014-11-11 at 19:49 +0700, Duy Nguyen wrote: I've come to the last piece to speed up git status, watchman support. And I realized it's not as good as I thought. Watchman could be used for two things: to avoid refreshing the index, and to avoid searching for ignored files. The first one can be done (with the patch below as demonstration). And it should keep refresh cost to near zero in the best case, the cost is proportional to the number of modified files. For avoiding searching for ignored files. My intention was to build on top of untracked cache. If watchman can tell me what files are added or deleted since last observed time, then I can invalidate just directories that contain them, or even better, calculate ignore status for those files only. This is important because in reality compilers and editors tend to update files by creating a new version then rename them, updating directory mtime and invalidating untracked cache as a consequence. As you edit more files (or your rebuild touches more dirs), untracked cache performance drops (until the next git status). The numbers I posted so far are the best case. The problem with watchman is it cannot tell me new files since the last observed time (let's say 'T'). If a file exists at 'T', gets deleted then recreated, then watchman tells me it's a new file. I want to separate those from ones that do not exist before 'T'. David's watchman approach does not have this problem because he keeps track of all entries under $GIT_WORK_TREE and knows which files are truely new. But I don't really want to keep the whole file list around, especially when watchman already manages the same list. 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. 2) Fork watchman 3) Make another daemon to keep file list around, or put it in a shared memory. 4) Move David's watchman series forward (and maybe make use of shared mem for fs_cache). 5) Go with something similar to the patch below and accept untracked cache performance degrades from time to time 6) ?? I'm working on 1). 2) is just bad taste, listed for completeness only. If we go with 3) and watchman starts to support Windows (seems to be in their plan), we'll need to rework some how. And I really don't like 3) If 1-3 does not work out, we're left without 4) and 5). We could support both, but proobably not worth the code complexity and should just go with one. And if we go with 4) we should probably think of dropping untracked cache if watchman will support Windows in the end. 4) also has another advantage over untracked cache, that it could speed up listing ignored files as well as untracked files. 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. 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. -- 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
(meta-comment: please snip out the context you are not responding to, to make reading easier) Stefan Beller wrote: On Mon, Nov 17, 2014 at 3:35 PM, Jonathan Nieder jrnie...@gmail.com wrote: Stefan Beller wrote: Update copy_fd to return a meaningful errno on failure and also preserve the existing errno variable. Some functions in git make errno meaningful on error and others don't. In general, the more we only use errno immediately after a system call, the better, so based on the above description this seems like a step in the wrong direction. I did reword the commit message badly. Before it just read Update copy_fd to return a meaningful errno. In fact the proposed patch doesn't guarantee the errno, which is set at the beginning of the function to be the same at the end. What it really should preserve is the errno from xread, while evaluating error(copy-fd: read returned %s, strerror(errno)); So the function call of error(...) or strerror(...) may change the errno. A successful call to strerror() is guaranteed not to change errno, but a call to error() does I/O so it can clobber errno. The basic question is whether errno is and should be part of copy_fd()'s contract. Until v2.2.0-rc0~53^2~2 (2014-10-01), it wasn't. Even after that change, there's no user-visible effect to clobbering errno (so this patch is about maintainability, as opposed to fixing a user-visible bad behavior). [...] 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 error handling would be more explicit so there would be no need to protect errno from clobbering by other system calls (both here and in callers). Something like this: I like this approach, though your patch is not about the original intention as this one (having strerror(...) not messing with the errno), but rather accumulating the errors not in numbers but string buffers? After this patch, setting errno is not part of the contract of copy_fd, so the bug Ronnie was fixing is gone. But it's a little more invasive. What do you think? 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
Getting a commit sha1 from fast-import in a remote-helper
Hi, For some reason, I need to know the sha1 corresponding to some marks I'm putting in a fast-import stream. Unfortunately, this does not appear to be possible. - I'd rather not require a checkpoint to export marks each time I need such a sha1, and I'd rather not do that work that requires them after all the marks have been created (although currently, I'm forced to). - fast-import's cat-blob only allows to cat blobs, which, well, is kind of in its name; how about adding an equivalent to the git-cat-file command? - fast-import's `ls` command documentation about its output format mentions that the output may contain commits, so I tried the trick of creating a tree with commits, but fast-import then fails with: fatal: Not a blob (actually a commit) which I totally understand, but then I wonder why the documentation mentions it and how one would get a tree containing references to commits. I guess the documentation should be fixed. So, while there's a possible solution with an equivalent to the git-cat-file command if that'd be accepted, that's also overkill for my need, which is to simply get the sha1 corresponding to a mark. Would you consider a patch adding a command that allows such a resolution? Cheers, 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: [PATCH] copy.c: make copy_fd preserve meaningful errno
On Mon, Nov 17, 2014 at 4:48 PM, Jonathan Nieder jrnie...@gmail.com wrote: (meta-comment: please snip out the context you are not responding to, to make reading easier) will do After this patch, setting errno is not part of the contract of copy_fd, so the bug Ronnie was fixing is gone. But it's a little more invasive. What do you think? I really like that approach and would be happy if we'd take it instead of the one I sent. -- 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 02/14] refs.c: make ref_transaction_delete a wrapper for ref_transaction_update
From: Ronnie Sahlberg sahlb...@google.com
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 | 22 ++
refs.h | 2 +-
2 files changed, 3 insertions(+), 21 deletions(-)
diff --git a/refs.c b/refs.c
index 005eb18..05cb299 100644
--- a/refs.c
+++ b/refs.c
@@ -3633,26 +3633,8 @@ int ref_transaction_delete(struct ref_transaction
*transaction,
int flags, int have_old, const char *msg,
struct strbuf *err)
{
- struct ref_update *update;
-
- assert(err);
-
- if (transaction-state != REF_TRANSACTION_OPEN)
- die(BUG: delete called for transaction that is not open);
-
- if (have_old !old_sha1)
- die(BUG: have_old is true but old_sha1 is NULL);
-
- update = add_update(transaction, refname);
- update-flags = flags;
- update-have_old = have_old;
- if (have_old) {
- assert(!is_null_sha1(old_sha1));
- hashcpy(update-old_sha1, old_sha1);
- }
- if (msg)
- update-msg = xstrdup(msg);
- return 0;
+ return ref_transaction_update(transaction, refname, null_sha1,
+ old_sha1, flags, have_old, msg, err);
}
int update_ref(const char *action, const char *refname,
diff --git a/refs.h b/refs.h
index 2bc3556..7d675b7 100644
--- a/refs.h
+++ b/refs.h
@@ -283,7 +283,7 @@ struct ref_transaction *ref_transaction_begin(struct strbuf
*err);
/*
* Add a reference update to transaction. new_sha1 is the value that
- * the reference should have after the update, or zeros if it should
+ * the reference should have after the update, or null_sha1 if it should
* be deleted. If have_old is true, then old_sha1 holds the value
* that the reference should have had before the update, or zeros if
* it must not have existed beforehand.
--
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 v3 00/14] ref-transactions-reflog
Hi, The following patch series updates the reflog handling to use transactions. This patch series has previously been sent to the list[1]. This series converts the reflog handling and builtin/reflog.c to use a transaction for both the ref as well as the reflog updates. As a side effect of this it simplifies the reflog marshalling code so that we only have one place where we marshall the entry. It also means that we can remove several functions from the public api towards the end of the series since we no longer need those functions. This series can also be found at github[2] or at googlesource[3]. Feel free to review, where it suits you best. Version 3: * Go over the commit messages and reword them slightly where appropriate. (only cosmetics, like missing/double words, spelling, clarify) * As Ronnie announced to change employers soon, he'll have only limited time to work on git in the near future. As this is a rather large patch series, he is handing this work over to me. That's why I'm sending the patches this time. Thanks, Stefan [1] http://www.spinics.net/lists/git/msg241186.html [2] https://github.com/stefanbeller/git/tree/ref-transactions-reflog [3] https://code-review.googlesource.com/#/q/topic:ref-transaction-reflog Ronnie Sahlberg (14): refs.c: make ref_transaction_create a wrapper for ref_transaction_update refs.c: make ref_transaction_delete a wrapper for ref_transaction_update refs.c: rename the transaction functions refs.c: add a function to append a reflog entry to a fd refs.c: add a new update_type field to ref_update refs.c: add a transaction function to append a reflog entry refs.c: add a flag to allow reflog updates to truncate the log refs.c: only write reflog update if msg is non-NULL refs.c: allow multiple reflog updates during a single transaction reflog.c: use a reflog transaction when writing during expire refs.c: rename log_ref_setup to create_reflog refs.c: Remove unlock_ref/close_ref/commit_ref from the refs api refs.c: remove lock_any_ref_for_update refs.c: allow deleting refs with a broken sha1 branch.c| 13 +- builtin/branch.c| 5 +- builtin/checkout.c | 8 +- builtin/commit.c| 10 +- builtin/fetch.c | 12 +- builtin/receive-pack.c | 13 +- builtin/reflog.c| 85 -- builtin/replace.c | 10 +- builtin/tag.c | 10 +- builtin/update-ref.c| 26 +-- cache.h | 7 + fast-import.c | 22 +-- refs.c | 403 +--- refs.h | 87 +- sequencer.c | 12 +- t/t1402-check-ref-format.sh | 8 + walker.c| 10 +- 17 files changed, 440 insertions(+), 301 deletions(-) -- 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 v3 14/14] refs.c: allow deleting refs with a broken sha1
From: Ronnie Sahlberg sahlb...@google.com
Add back support to make it possible to delete refs that have a broken
sha1.
Add new internal flags REF_ALLOW_BROKEN and RESOLVE_REF_ALLOW_BAD_SHA1
to pass intent from branch.c that we are willing to allow
resolve_ref_unsafe and lock_ref_sha1_basic to allow broken refs.
Since these refs can not actually be resolved to a sha1, they instead resolve
to null_sha1 when these flags are used.
For example, the ref:
echo Broken ref .git/refs/heads/foo-broken-1
can now be deleted using git branch -d foo-broken-1
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
Signed-off-by: Jonathan Nieder jrnie...@gmail.com
Signed-off-by: Stefan Beller sbel...@google.com
---
builtin/branch.c| 5 +++--
cache.h | 7 +++
refs.c | 6 ++
refs.h | 6 --
t/t1402-check-ref-format.sh | 8
5 files changed, 28 insertions(+), 4 deletions(-)
diff --git a/builtin/branch.c b/builtin/branch.c
index 3b79c50..04f57d4 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -238,7 +238,8 @@ static int delete_branches(int argc, const char **argv, int
force, int kinds,
target = resolve_ref_unsafe(name,
RESOLVE_REF_READING
| RESOLVE_REF_NO_RECURSE
- | RESOLVE_REF_ALLOW_BAD_NAME,
+ | RESOLVE_REF_ALLOW_BAD_NAME
+ | RESOLVE_REF_ALLOW_BAD_SHA1,
sha1, flags);
if (!target) {
error(remote_branch
@@ -255,7 +256,7 @@ static int delete_branches(int argc, const char **argv, int
force, int kinds,
continue;
}
- if (delete_ref(name, sha1, REF_NODEREF)) {
+ if (delete_ref(name, sha1, REF_NODEREF|REF_ALLOW_BROKEN)) {
error(remote_branch
? _(Error deleting remote branch '%s')
: _(Error deleting branch '%s'),
diff --git a/cache.h b/cache.h
index 99ed096..61e61af 100644
--- a/cache.h
+++ b/cache.h
@@ -1000,10 +1000,17 @@ extern int read_ref(const char *refname, unsigned char
*sha1);
* resolved. The function returns NULL for such ref names.
* Caps and underscores refers to the special refs, such as HEAD,
* FETCH_HEAD and friends, that all live outside of the refs/ directory.
+ *
+ * RESOLVE_REF_ALLOW_BAD_SHA1 when this flag is set and the ref contains
+ * an invalid sha1, resolve_ref_unsafe will clear the sha1 argument,
+ * set the REF_ISBROKEN flag and return the refname.
+ * This allows using resolve_ref_unsafe to check for existence of such
+ * broken refs.
*/
#define RESOLVE_REF_READING 0x01
#define RESOLVE_REF_NO_RECURSE 0x02
#define RESOLVE_REF_ALLOW_BAD_NAME 0x04
+#define RESOLVE_REF_ALLOW_BAD_SHA1 0x08
extern const char *resolve_ref_unsafe(const char *ref, int resolve_flags,
unsigned char *sha1, int *flags);
extern char *resolve_refdup(const char *ref, int resolve_flags, unsigned char
*sha1, int *flags);
diff --git a/refs.c b/refs.c
index b31..d75af6c 100644
--- a/refs.c
+++ b/refs.c
@@ -1584,6 +1584,10 @@ const char *resolve_ref_unsafe(const char *refname, int
resolve_flags, unsigned
(buffer[40] != '\0' !isspace(buffer[40]))) {
if (flags)
*flags |= REF_ISBROKEN;
+ if (resolve_flags RESOLVE_REF_ALLOW_BAD_SHA1)
{
+ hashclr(sha1);
+ return refname;
+ }
errno = EINVAL;
return NULL;
}
@@ -2265,6 +2269,8 @@ static struct ref_lock *lock_ref_sha1_basic(const char
*refname,
if (flags REF_NODEREF)
resolve_flags |= RESOLVE_REF_NO_RECURSE;
}
+ if (flags REF_ALLOW_BROKEN)
+ resolve_flags |= RESOLVE_REF_ALLOW_BAD_SHA1;
refname = resolve_ref_unsafe(refname, resolve_flags,
lock-old_sha1, type);
diff --git a/refs.h b/refs.h
index 721e21f..2e97f4f 100644
--- a/refs.h
+++ b/refs.h
@@ -185,11 +185,13 @@ extern int peel_ref(const char *refname, unsigned char
*sha1);
* REF_NODEREF: act on the ref directly, instead of dereferencing
* symbolic references.
* REF_DELETING: tolerate broken refs
+ * REF_ALLOW_BROKEN: allow locking refs that are broken.
*
* Flags = 0x100 are reserved for internal use.
*/
-#define REF_NODEREF0x01
-#define REF_DELETING 0x02
+#define REF_NODEREF0x01
+#define REF_DELETING 0x02
+#define REF_ALLOW_BROKEN 0x04
/** Reads log for the
[PATCH v3 03/14] refs.c: rename the transaction functions
From: Ronnie Sahlberg sahlb...@google.com
Rename the transaction functions. Remove the leading ref_ from the
names and append _ref to the names for functions that create/delete/
update sha1 refs.
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
Signed-off-by: Jonathan Nieder jrnie...@gmail.com
Signed-off-by: Stefan Beller sbel...@google.com
---
branch.c | 13 +
builtin/commit.c | 10 +++
builtin/fetch.c| 12
builtin/receive-pack.c | 13 -
builtin/replace.c | 10 +++
builtin/tag.c | 10 +++
builtin/update-ref.c | 26 -
fast-import.c | 22 +++---
refs.c | 78 +-
refs.h | 36 +++
sequencer.c| 12
walker.c | 10 +++
12 files changed, 126 insertions(+), 126 deletions(-)
diff --git a/branch.c b/branch.c
index 4bab55a..c8462de 100644
--- a/branch.c
+++ b/branch.c
@@ -279,16 +279,17 @@ void create_branch(const char *head,
log_all_ref_updates = 1;
if (!dont_change_ref) {
- struct ref_transaction *transaction;
+ struct transaction *transaction;
struct strbuf err = STRBUF_INIT;
- transaction = ref_transaction_begin(err);
+ transaction = transaction_begin(err);
if (!transaction ||
- ref_transaction_update(transaction, ref.buf, sha1,
- null_sha1, 0, !forcing, msg, err) ||
- ref_transaction_commit(transaction, err))
+ transaction_update_ref(transaction, ref.buf, sha1,
+ null_sha1, 0, !forcing, msg,
+ err) ||
+ transaction_commit(transaction, err))
die(%s, err.buf);
- ref_transaction_free(transaction);
+ transaction_free(transaction);
strbuf_release(err);
}
diff --git a/builtin/commit.c b/builtin/commit.c
index e108c53..f50b7df 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -1673,7 +1673,7 @@ int cmd_commit(int argc, const char **argv, const char
*prefix)
struct stat statbuf;
struct commit *current_head = NULL;
struct commit_extra_header *extra = NULL;
- struct ref_transaction *transaction;
+ struct transaction *transaction;
struct strbuf err = STRBUF_INIT;
if (argc == 2 !strcmp(argv[1], -h))
@@ -1804,17 +1804,17 @@ int cmd_commit(int argc, const char **argv, const char
*prefix)
strbuf_insert(sb, 0, reflog_msg, strlen(reflog_msg));
strbuf_insert(sb, strlen(reflog_msg), : , 2);
- transaction = ref_transaction_begin(err);
+ transaction = transaction_begin(err);
if (!transaction ||
- ref_transaction_update(transaction, HEAD, sha1,
+ transaction_update_ref(transaction, HEAD, sha1,
current_head
? current_head-object.sha1 : NULL,
0, !!current_head, sb.buf, err) ||
- ref_transaction_commit(transaction, err)) {
+ transaction_commit(transaction, err)) {
rollback_index_files();
die(%s, err.buf);
}
- ref_transaction_free(transaction);
+ transaction_free(transaction);
unlink(git_path(CHERRY_PICK_HEAD));
unlink(git_path(REVERT_HEAD));
diff --git a/builtin/fetch.c b/builtin/fetch.c
index 7b84d35..0be0b09 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -404,7 +404,7 @@ static int s_update_ref(const char *action,
{
char msg[1024];
char *rla = getenv(GIT_REFLOG_ACTION);
- struct ref_transaction *transaction;
+ struct transaction *transaction;
struct strbuf err = STRBUF_INIT;
int ret, df_conflict = 0;
@@ -414,23 +414,23 @@ static int s_update_ref(const char *action,
rla = default_rla.buf;
snprintf(msg, sizeof(msg), %s: %s, rla, action);
- transaction = ref_transaction_begin(err);
+ transaction = transaction_begin(err);
if (!transaction ||
- ref_transaction_update(transaction, ref-name, ref-new_sha1,
+ transaction_update_ref(transaction, ref-name, ref-new_sha1,
ref-old_sha1, 0, check_old, msg, err))
goto fail;
- ret = ref_transaction_commit(transaction, err);
+ ret = transaction_commit(transaction, err);
if (ret) {
df_conflict = (ret == TRANSACTION_NAME_CONFLICT);
goto fail;
}
- ref_transaction_free(transaction);
+ transaction_free(transaction);
strbuf_release(err);
return 0;
fail:
- ref_transaction_free(transaction);
+
[PATCH v3 10/14] reflog.c: use a reflog transaction when writing during expire
From: Ronnie Sahlberg sahlb...@google.com
Use a transaction for all updates during expire_reflog.
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
Signed-off-by: Jonathan Nieder jrnie...@gmail.com
Signed-off-by: Stefan Beller sbel...@google.com
---
builtin/reflog.c | 85
refs.c | 4 +--
refs.h | 2 +-
3 files changed, 40 insertions(+), 51 deletions(-)
diff --git a/builtin/reflog.c b/builtin/reflog.c
index 2d85d26..6bb7454 100644
--- a/builtin/reflog.c
+++ b/builtin/reflog.c
@@ -32,8 +32,11 @@ struct cmd_reflog_expire_cb {
int recno;
};
+static struct strbuf err = STRBUF_INIT;
+
struct expire_reflog_cb {
- FILE *newlog;
+ struct transaction *t;
+ const char *refname;
enum {
UE_NORMAL,
UE_ALWAYS,
@@ -316,20 +319,18 @@ static int expire_reflog_ent(unsigned char *osha1,
unsigned char *nsha1,
if (cb-cmd-recno --(cb-cmd-recno) == 0)
goto prune;
- if (cb-newlog) {
- char sign = (tz 0) ? '-' : '+';
- int zone = (tz 0) ? (-tz) : tz;
- fprintf(cb-newlog, %s %s %s %lu %c%04d\t%s,
- sha1_to_hex(osha1), sha1_to_hex(nsha1),
- email, timestamp, sign, zone,
- message);
+ if (cb-t) {
+ if (transaction_update_reflog(cb-t, cb-refname, nsha1, osha1,
+ email, timestamp, tz, message, 0,
+ err))
+ return -1;
hashcpy(cb-last_kept_sha1, nsha1);
}
if (cb-cmd-verbose)
printf(keep %s, message);
return 0;
prune:
- if (!cb-newlog)
+ if (!cb-t)
printf(would prune %s, message);
else if (cb-cmd-verbose)
printf(prune %s, message);
@@ -353,29 +354,26 @@ static int expire_reflog(const char *ref, const unsigned
char *sha1, int unused,
{
struct cmd_reflog_expire_cb *cmd = cb_data;
struct expire_reflog_cb cb;
- struct ref_lock *lock;
- char *log_file, *newlog_path = NULL;
struct commit *tip_commit;
struct commit_list *tips;
int status = 0;
memset(cb, 0, sizeof(cb));
+ cb.refname = ref;
- /*
-* we take the lock for the ref itself to prevent it from
-* getting updated.
-*/
- lock = lock_any_ref_for_update(ref, sha1, 0, NULL);
- if (!lock)
- return error(cannot lock ref '%s', ref);
- log_file = git_pathdup(logs/%s, ref);
if (!reflog_exists(ref))
goto finish;
- if (!cmd-dry_run) {
- newlog_path = git_pathdup(logs/%s.lock, ref);
- cb.newlog = fopen(newlog_path, w);
+ cb.t = transaction_begin(err);
+ if (!cb.t) {
+ status |= error(%s, err.buf);
+ goto cleanup;
+ }
+ if (transaction_update_reflog(cb.t, cb.refname, null_sha1, null_sha1,
+ NULL, 0, 0, NULL, REFLOG_TRUNCATE,
+ err)) {
+ status |= error(%s, err.buf);
+ goto cleanup;
}
-
cb.cmd = cmd;
if (!cmd-expire_unreachable || !strcmp(ref, HEAD)) {
@@ -407,7 +405,10 @@ static int expire_reflog(const char *ref, const unsigned
char *sha1, int unused,
mark_reachable(cb);
}
- for_each_reflog_ent(ref, expire_reflog_ent, cb);
+ if (for_each_reflog_ent(ref, expire_reflog_ent, cb)) {
+ status |= error(%s, err.buf);
+ goto cleanup;
+ }
if (cb.unreachable_expire_kind != UE_ALWAYS) {
if (cb.unreachable_expire_kind == UE_HEAD) {
@@ -420,32 +421,20 @@ static int expire_reflog(const char *ref, const unsigned
char *sha1, int unused,
}
}
finish:
- if (cb.newlog) {
- if (fclose(cb.newlog)) {
- status |= error(%s: %s, strerror(errno),
- newlog_path);
- unlink(newlog_path);
- } else if (cmd-updateref
- (write_in_full(lock-lock_fd,
- sha1_to_hex(cb.last_kept_sha1), 40) != 40 ||
-write_str_in_full(lock-lock_fd, \n) != 1 ||
-close_ref(lock) 0)) {
- status |= error(Couldn't write %s,
- lock-lk-filename.buf);
- unlink(newlog_path);
- } else if (rename(newlog_path, log_file)) {
- status |= error(cannot rename %s to %s,
- newlog_path, log_file);
- unlink(newlog_path);
- } else if (cmd-updateref
[PATCH v3 05/14] refs.c: add a new update_type field to ref_update
From: Ronnie Sahlberg sahlb...@google.com
Add a field that describes what type of update this refers to. For now
the only type is UPDATE_SHA1 but we will soon add more types.
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 | 27 +++
1 file changed, 23 insertions(+), 4 deletions(-)
diff --git a/refs.c b/refs.c
index f0f0d23..84e086f 100644
--- a/refs.c
+++ b/refs.c
@@ -3516,6 +3516,10 @@ int for_each_reflog(each_ref_fn fn, void *cb_data)
return retval;
}
+enum transaction_update_type {
+ UPDATE_SHA1 = 0
+};
+
/**
* Information needed for a single ref update. Set new_sha1 to the
* new value or to zero to delete the ref. To check the old value
@@ -3523,6 +3527,7 @@ int for_each_reflog(each_ref_fn fn, void *cb_data)
* value or to zero to ensure the ref does not exist before update.
*/
struct ref_update {
+ enum transaction_update_type update_type;
unsigned char new_sha1[20];
unsigned char old_sha1[20];
int flags; /* REF_NODEREF? */
@@ -3583,12 +3588,14 @@ void transaction_free(struct transaction *transaction)
}
static struct ref_update *add_update(struct transaction *transaction,
-const char *refname)
+const char *refname,
+enum transaction_update_type update_type)
{
size_t len = strlen(refname);
struct ref_update *update = xcalloc(1, sizeof(*update) + len + 1);
strcpy((char *)update-refname, refname);
+ update-update_type = update_type;
ALLOC_GROW(transaction-updates, transaction-nr + 1,
transaction-alloc);
transaction-updates[transaction-nr++] = update;
return update;
@@ -3618,7 +3625,7 @@ int transaction_update_ref(struct transaction
*transaction,
return -1;
}
- update = add_update(transaction, refname);
+ update = add_update(transaction, refname, UPDATE_SHA1);
hashcpy(update-new_sha1, new_sha1);
update-flags = flags;
update-have_old = have_old;
@@ -3696,13 +3703,17 @@ static int ref_update_reject_duplicates(struct
ref_update **updates, int n,
assert(err);
- for (i = 1; i n; i++)
+ for (i = 1; i n; i++) {
+ if (updates[i - 1]-update_type != UPDATE_SHA1 ||
+ updates[i]-update_type != UPDATE_SHA1)
+ continue;
if (!strcmp(updates[i - 1]-refname, updates[i]-refname)) {
strbuf_addf(err,
Multiple updates for ref '%s' not
allowed.,
updates[i]-refname);
return 1;
}
+ }
return 0;
}
@@ -3734,13 +3745,17 @@ int transaction_commit(struct transaction *transaction,
goto cleanup;
}
- /* Acquire all locks while verifying old values */
+ /* Acquire all ref locks while verifying old values */
for (i = 0; i n; i++) {
struct ref_update *update = updates[i];
int flags = update-flags;
+ if (update-update_type != UPDATE_SHA1)
+ continue;
+
if (is_null_sha1(update-new_sha1))
flags |= REF_DELETING;
+
update-lock = lock_ref_sha1_basic(update-refname,
(update-have_old ?
update-old_sha1 :
@@ -3762,6 +3777,8 @@ int transaction_commit(struct transaction *transaction,
for (i = 0; i n; i++) {
struct ref_update *update = updates[i];
+ if (update-update_type != UPDATE_SHA1)
+ continue;
if (!is_null_sha1(update-new_sha1)) {
if (write_ref_sha1(update-lock, update-new_sha1,
update-msg)) {
@@ -3779,6 +3796,8 @@ int transaction_commit(struct transaction *transaction,
for (i = 0; i n; i++) {
struct ref_update *update = updates[i];
+ if (update-update_type != UPDATE_SHA1)
+ continue;
if (update-lock) {
if (delete_ref_loose(update-lock, update-type, err)) {
ret = TRANSACTION_GENERIC_ERROR;
--
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 v3 08/14] refs.c: only write reflog update if msg is non-NULL
From: Ronnie Sahlberg sahlb...@google.com
When performing a reflog transaction update, only write to the reflog iff
msg is non-NULL. This can then be combined with REFLOG_TRUNCATE to perform
an update that only truncates but does not write.
This change only affects whether or not a reflog entry should be generated
and written. If msg==NULL then no such entry will be written.
Orthogonal to this we have a boolean flag REFLOG_TRUNCATE which is used to
tell the transaction system to truncate the reflog and thus discard all
previous users.
At the current time the only place where we use msg==NULL is also the
place, where we use REFLOG_TRUNCATE. Even though these two settings are
currently only ever used together it still makes sense to have them through
two separate knobs.
This allows future consumers of this API that may want to do things
differently. For example someone can do:
msg=Reflog truncated by Bob because ... + REFLOG_TRUNCATE
and have it truncate the log and have it start fresh with an initial message
that explains the log was truncated. This API allows that.
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 | 5 +++--
refs.h | 1 +
2 files changed, 4 insertions(+), 2 deletions(-)
diff --git a/refs.c b/refs.c
index d21ecb9..3572977 100644
--- a/refs.c
+++ b/refs.c
@@ -3895,8 +3895,9 @@ int transaction_commit(struct transaction *transaction,
update-reflog_fd = -1;
continue;
}
- if (log_ref_write_fd(update-reflog_fd, update-old_sha1,
-update-new_sha1,
+ if (update-msg
+ log_ref_write_fd(update-reflog_fd,
+update-old_sha1, update-new_sha1,
update-committer, update-msg)) {
error(Could write to reflog: %s. %s,
update-refname, strerror(errno));
diff --git a/refs.h b/refs.h
index 5075073..bf96b36 100644
--- a/refs.h
+++ b/refs.h
@@ -337,6 +337,7 @@ int transaction_delete_ref(struct transaction *transaction,
/*
* Append a reflog entry for refname. If the REFLOG_TRUNCATE flag is set
* this update will first truncate the reflog before writing the entry.
+ * If msg is NULL no update will be written to the log.
*/
int transaction_update_reflog(struct transaction *transaction,
const char *refname,
--
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 v3 09/14] refs.c: allow multiple reflog updates during a single transaction
From: Ronnie Sahlberg sahlb...@google.com
Allow to make multiple reflog updates to the same ref during a transaction.
This means we only need to lock the reflog once, during the first update
that touches the reflog, and that all further updates can just write the
reflog entry since the reflog is already locked.
This allows us to write code such as:
t = transaction_begin()
transaction_reflog_update(t, foo, REFLOG_TRUNCATE, NULL);
loop-over-something...
transaction_reflog_update(t, foo, 0, message);
transaction_commit(t)
where we first truncate the reflog and then build the new content one line
at a time.
While this technically looks like O(n2) behavior it is not that bad.
We only do this loop for transactions that cover a single ref during
reflog expire. This means that the linear search inside
transaction_update_reflog() will find the match on the very first entry
thus making it O(1) and not O(n) or our usecases. Thus the whole expire
becomes O(n) instead of O(n2). If in the future we start doing this for many
refs in one single transaction we might want to optimize this.
But there is no need to complexify the code and optimize for future usecases
that might never materialize at this stage.
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 | 48 +++-
1 file changed, 39 insertions(+), 9 deletions(-)
diff --git a/refs.c b/refs.c
index 3572977..0fb4196 100644
--- a/refs.c
+++ b/refs.c
@@ -31,6 +31,12 @@ static unsigned char refname_disposition[256] = {
*/
#define REF_ISPRUNING 0x0100
/*
+ * Only the first reflog update needs to lock the reflog file. Further updates
+ * just use the lock taken by the first update.
+ */
+#define UPDATE_REFLOG_NOLOCK 0x0200
+
+/*
* Try to read one refname component from the front of refname.
* Return the length of the component found, or -1 if the component is
* not legal. It is legal if it is something reasonable to have under
@@ -3521,7 +3527,7 @@ enum transaction_update_type {
UPDATE_LOG = 1
};
-/**
+/*
* Information needed for a single ref update. Set new_sha1 to the
* new value or to zero to delete the ref. To check the old value
* while locking the ref, set have_old to 1 and set old_sha1 to the
@@ -3531,7 +3537,9 @@ struct ref_update {
enum transaction_update_type update_type;
unsigned char new_sha1[20];
unsigned char old_sha1[20];
- int flags; /* REF_NODEREF? */
+ int flags; /* The flags to transaction_update_ref[log] are defined
+* in refs.h
+*/
int have_old; /* 1 if old_sha1 is valid, 0 otherwise */
struct ref_lock *lock;
int type;
@@ -3539,8 +3547,9 @@ struct ref_update {
/* used by reflog updates */
int reflog_fd;
- struct lock_file reflog_lock;
+ struct lock_file *reflog_lock;
char *committer;
+ struct ref_update *orig_update; /* For UPDATE_REFLOG_NOLOCK */
const char refname[FLEX_ARRAY];
};
@@ -3619,11 +3628,26 @@ int transaction_update_reflog(struct transaction
*transaction,
struct strbuf *err)
{
struct ref_update *update;
+ int i;
if (transaction-state != TRANSACTION_OPEN)
die(BUG: update_reflog called for transaction that is not
open);
update = add_update(transaction, refname, UPDATE_LOG);
+ update-flags = flags;
+ for (i = 0; i transaction-nr - 1; i++) {
+ if (transaction-updates[i]-update_type != UPDATE_LOG)
+ continue;
+ if (!strcmp(transaction-updates[i]-refname,
+ update-refname)) {
+ update-flags |= UPDATE_REFLOG_NOLOCK;
+ update-orig_update = transaction-updates[i];
+ break;
+ }
+ }
+ if (!(update-flags UPDATE_REFLOG_NOLOCK))
+ update-reflog_lock = xcalloc(1, sizeof(struct lock_file));
+
hashcpy(update-new_sha1, new_sha1);
hashcpy(update-old_sha1, old_sha1);
update-reflog_fd = -1;
@@ -3639,7 +3663,6 @@ int transaction_update_reflog(struct transaction
*transaction,
}
if (msg)
update-msg = xstrdup(msg);
- update-flags = flags;
return 0;
}
@@ -3822,10 +3845,15 @@ int transaction_commit(struct transaction *transaction,
if (update-update_type != UPDATE_LOG)
continue;
+ if (update-flags UPDATE_REFLOG_NOLOCK) {
+ update-reflog_fd = update-orig_update-reflog_fd;
+ update-reflog_lock = update-orig_update-reflog_lock;
+ continue;
+ }
update-reflog_fd = hold_lock_file_for_append(
-
[PATCH v3 11/14] refs.c: rename log_ref_setup to create_reflog
From: Ronnie Sahlberg sahlb...@google.com
log_ref_setup is used to do several semi-related things:
* Sometimes it will create a new reflog including missing parent
directories and cleaning up any conflicting stale directories
in the path.
* Fill in a filename buffer for the full path to the reflog.
* Unconditionally re-adjust the permissions for the file.
This function is only called from two places: In checkout.c, where
it is always used to create a reflog and in refs.c log_ref_write,
where it is used to create a reflog sometimes, and sometimes just
to fill in the filename.
Rename log_ref_setup to create_reflog and change it to only take the
refname as an argument to make its signature similar to delete_reflog
and reflog_exists. Change create_reflog to ignore log_all_ref_updates
and unconditionally create the reflog when called. Since checkout.c
always wants to create a reflog we can call create_reflog directly and
avoid the temp-and-log_all_ref_update dance.
In log_ref_write, only call create_reflog, iff we want to create a reflog
and if the reflog does not yet exist. This means that for the common case
where the log already exists we now only need to perform a single lstat()
instead of a open(O_CREAT)+lstat()+close().
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
Signed-off-by: Jonathan Nieder jrnie...@gmail.com
Signed-off-by: Stefan Beller sbel...@google.com
---
builtin/checkout.c | 8 +---
refs.c | 22 +-
refs.h | 8 +++-
3 files changed, 17 insertions(+), 21 deletions(-)
diff --git a/builtin/checkout.c b/builtin/checkout.c
index 5410dac..8550b6d 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -587,19 +587,13 @@ static void update_refs_for_switch(const struct
checkout_opts *opts,
if (opts-new_branch) {
if (opts-new_orphan_branch) {
if (opts-new_branch_log !log_all_ref_updates) {
- int temp;
- char log_file[PATH_MAX];
char *ref_name = mkpath(refs/heads/%s,
opts-new_orphan_branch);
- temp = log_all_ref_updates;
- log_all_ref_updates = 1;
- if (log_ref_setup(ref_name, log_file,
sizeof(log_file))) {
+ if (create_reflog(ref_name)) {
fprintf(stderr, _(Can not do reflog
for '%s'\n),
opts-new_orphan_branch);
- log_all_ref_updates = temp;
return;
}
- log_all_ref_updates = temp;
}
}
else
diff --git a/refs.c b/refs.c
index 1571fa5..11cf26b 100644
--- a/refs.c
+++ b/refs.c
@@ -2947,16 +2947,16 @@ static int copy_msg(char *buf, const char *msg)
}
/* This function must set a meaningful errno on failure */
-int log_ref_setup(const char *refname, char *logfile, int bufsize)
+int create_reflog(const char *refname)
{
int logfd, oflags = O_APPEND | O_WRONLY;
+ char logfile[PATH_MAX];
- git_snpath(logfile, bufsize, logs/%s, refname);
- if (log_all_ref_updates
- (starts_with(refname, refs/heads/) ||
-starts_with(refname, refs/remotes/) ||
-starts_with(refname, refs/notes/) ||
-!strcmp(refname, HEAD))) {
+ git_snpath(logfile, sizeof(logfile), logs/%s, refname);
+ if (starts_with(refname, refs/heads/) ||
+ starts_with(refname, refs/remotes/) ||
+ starts_with(refname, refs/notes/) ||
+ !strcmp(refname, HEAD)) {
if (safe_create_leading_directories(logfile) 0) {
int save_errno = errno;
error(unable to create directory for %s, logfile);
@@ -3025,16 +3025,20 @@ static int log_ref_write_fd(int fd, const unsigned char
*old_sha1,
static int log_ref_write(const char *refname, const unsigned char *old_sha1,
const unsigned char *new_sha1, const char *msg)
{
- int logfd, result, oflags = O_APPEND | O_WRONLY;
+ int logfd, result = 0, oflags = O_APPEND | O_WRONLY;
char log_file[PATH_MAX];
if (log_all_ref_updates 0)
log_all_ref_updates = !is_bare_repository();
- result = log_ref_setup(refname, log_file, sizeof(log_file));
+ if (log_all_ref_updates !reflog_exists(refname))
+ result = create_reflog(refname);
+
if (result)
return result;
+ git_snpath(log_file, sizeof(log_file), logs/%s, refname);
+
logfd = open(log_file, oflags);
if (logfd 0)
return 0;
diff --git a/refs.h b/refs.h
index 9f70b89..17e3a3c 100644
--- a/refs.h
+++ b/refs.h
@@ -207,11 +207,6 @@ extern int
[PATCH v3 06/14] refs.c: add a transaction function to append a reflog entry
From: Ronnie Sahlberg sahlb...@google.com
Define a new transaction update type, UPDATE_LOG, and a new function
transaction_update_reflog. This function will lock the reflog and append
an entry to it during transaction commit.
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 | 102 +++--
refs.h | 12
2 files changed, 112 insertions(+), 2 deletions(-)
diff --git a/refs.c b/refs.c
index 84e086f..9a46e1c 100644
--- a/refs.c
+++ b/refs.c
@@ -3517,7 +3517,8 @@ int for_each_reflog(each_ref_fn fn, void *cb_data)
}
enum transaction_update_type {
- UPDATE_SHA1 = 0
+ UPDATE_SHA1 = 0,
+ UPDATE_LOG = 1
};
/**
@@ -3535,6 +3536,12 @@ struct ref_update {
struct ref_lock *lock;
int type;
char *msg;
+
+ /* used by reflog updates */
+ int reflog_fd;
+ struct lock_file reflog_lock;
+ char *committer;
+
const char refname[FLEX_ARRAY];
};
@@ -3581,6 +3588,7 @@ void transaction_free(struct transaction *transaction)
for (i = 0; i transaction-nr; i++) {
free(transaction-updates[i]-msg);
+ free(transaction-updates[i]-committer);
free(transaction-updates[i]);
}
free(transaction-updates);
@@ -3601,6 +3609,41 @@ static struct ref_update *add_update(struct transaction
*transaction,
return update;
}
+int transaction_update_reflog(struct transaction *transaction,
+ const char *refname,
+ const unsigned char *new_sha1,
+ const unsigned char *old_sha1,
+ const unsigned char *email,
+ unsigned long timestamp, int tz,
+ const char *msg, int flags,
+ struct strbuf *err)
+{
+ struct ref_update *update;
+
+ if (transaction-state != TRANSACTION_OPEN)
+ die(BUG: update_reflog called for transaction that is not
open);
+
+ update = add_update(transaction, refname, UPDATE_LOG);
+ hashcpy(update-new_sha1, new_sha1);
+ hashcpy(update-old_sha1, old_sha1);
+ update-reflog_fd = -1;
+ if (email) {
+ struct strbuf buf = STRBUF_INIT;
+ char sign = (tz 0) ? '-' : '+';
+ int zone = (tz 0) ? (-tz) : tz;
+
+ strbuf_addf(buf, %s %lu %c%04d, email, timestamp, sign,
+ zone);
+ update-committer = xstrdup(buf.buf);
+ strbuf_release(buf);
+ }
+ if (msg)
+ update-msg = xstrdup(msg);
+ update-flags = flags;
+
+ return 0;
+}
+
int transaction_update_ref(struct transaction *transaction,
const char *refname,
const unsigned char *new_sha1,
@@ -3773,7 +3816,28 @@ int transaction_commit(struct transaction *transaction,
}
}
- /* Perform updates first so live commits remain referenced */
+ /* Lock all reflog files */
+ for (i = 0; i n; i++) {
+ struct ref_update *update = updates[i];
+
+ if (update-update_type != UPDATE_LOG)
+ continue;
+ update-reflog_fd = hold_lock_file_for_append(
+ update-reflog_lock,
+ git_path(logs/%s, update-refname),
+ 0);
+ if (update-reflog_fd 0) {
+ const char *str = Cannot lock reflog for '%s'. %s;
+
+ ret = -1;
+ if (err)
+ strbuf_addf(err, str, update-refname,
+ strerror(errno));
+ goto cleanup;
+ }
+ }
+
+ /* Perform ref updates first so live commits remain referenced */
for (i = 0; i n; i++) {
struct ref_update *update = updates[i];
@@ -3809,6 +3873,40 @@ int transaction_commit(struct transaction *transaction,
}
}
+ /* Update all reflog files */
+ for (i = 0; i n; i++) {
+ struct ref_update *update = updates[i];
+
+ if (update-update_type != UPDATE_LOG)
+ continue;
+ if (update-reflog_fd == -1)
+ continue;
+
+ if (log_ref_write_fd(update-reflog_fd, update-old_sha1,
+update-new_sha1,
+update-committer, update-msg)) {
+ error(Could write to reflog: %s. %s,
+ update-refname, strerror(errno));
+
[PATCH v3 01/14] refs.c: make ref_transaction_create a wrapper for ref_transaction_update
From: Ronnie Sahlberg sahlb...@google.com
The ref_transaction_update function can already be used to create refs by
passing null_sha1 as the old_sha1 parameter. Simplify by replacing
transaction_create with a thin wrapper.
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 | 27 ++-
1 file changed, 2 insertions(+), 25 deletions(-)
diff --git a/refs.c b/refs.c
index 5ff457e..005eb18 100644
--- a/refs.c
+++ b/refs.c
@@ -3623,31 +3623,8 @@ int ref_transaction_create(struct ref_transaction
*transaction,
int flags, const char *msg,
struct strbuf *err)
{
- struct ref_update *update;
-
- assert(err);
-
- if (transaction-state != REF_TRANSACTION_OPEN)
- die(BUG: create called for transaction that is not open);
-
- if (!new_sha1 || is_null_sha1(new_sha1))
- die(BUG: create ref with null new_sha1);
-
- if (check_refname_format(refname, REFNAME_ALLOW_ONELEVEL)) {
- strbuf_addf(err, refusing to create ref with bad name %s,
- refname);
- return -1;
- }
-
- update = add_update(transaction, refname);
-
- hashcpy(update-new_sha1, new_sha1);
- hashclr(update-old_sha1);
- update-flags = flags;
- update-have_old = 1;
- if (msg)
- update-msg = xstrdup(msg);
- return 0;
+ return ref_transaction_update(transaction, refname, new_sha1,
+ null_sha1, flags, 1, msg, err);
}
int ref_transaction_delete(struct ref_transaction *transaction,
--
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 v3 13/14] refs.c: remove lock_any_ref_for_update
From: Ronnie Sahlberg sahlb...@google.com
No one is using this function so we can delete it.
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 | 7 ---
refs.h | 9 +
2 files changed, 1 insertion(+), 15 deletions(-)
diff --git a/refs.c b/refs.c
index e49ae11..b31 100644
--- a/refs.c
+++ b/refs.c
@@ -2352,13 +2352,6 @@ static struct ref_lock *lock_ref_sha1_basic(const char
*refname,
return NULL;
}
-struct ref_lock *lock_any_ref_for_update(const char *refname,
-const unsigned char *old_sha1,
-int flags, int *type_p)
-{
- return lock_ref_sha1_basic(refname, old_sha1, NULL, flags, type_p);
-}
-
/*
* Write an entry to the packed-refs file for the specified refname.
* If peeled is non-NULL, write it as the entry's peeled value.
diff --git a/refs.h b/refs.h
index 025e2cb..721e21f 100644
--- a/refs.h
+++ b/refs.h
@@ -181,8 +181,7 @@ extern int is_branch(const char *refname);
extern int peel_ref(const char *refname, unsigned char *sha1);
/*
- * Flags controlling lock_any_ref_for_update(), transaction_update_ref(),
- * transaction_create_ref(), etc.
+ * Flags controlling transaction_update_ref(), transaction_create_ref(), etc.
* REF_NODEREF: act on the ref directly, instead of dereferencing
* symbolic references.
* REF_DELETING: tolerate broken refs
@@ -191,12 +190,6 @@ extern int peel_ref(const char *refname, unsigned char
*sha1);
*/
#define REF_NODEREF0x01
#define REF_DELETING 0x02
-/*
- * This function sets errno to something meaningful on failure.
- */
-extern struct ref_lock *lock_any_ref_for_update(const char *refname,
- const unsigned char *old_sha1,
- int flags, int *type_p);
/** Reads log for the value of ref during at_time. **/
extern int read_ref_at(const char *refname, unsigned int flags,
--
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 v3 07/14] refs.c: add a flag to allow reflog updates to truncate the log
From: Ronnie Sahlberg sahlb...@google.com
Add a flag that allows us to truncate the reflog before we write the
update.
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 | 17 +++--
refs.h | 10 +-
2 files changed, 24 insertions(+), 3 deletions(-)
diff --git a/refs.c b/refs.c
index 9a46e1c..d21ecb9 100644
--- a/refs.c
+++ b/refs.c
@@ -3873,7 +3873,12 @@ int transaction_commit(struct transaction *transaction,
}
}
- /* Update all reflog files */
+ /*
+* Update all reflog files
+* We have already done all ref updates and deletes.
+* There is not much we can do here if there are any reflog
+* update errors other than complain.
+*/
for (i = 0; i n; i++) {
struct ref_update *update = updates[i];
@@ -3881,7 +3886,15 @@ int transaction_commit(struct transaction *transaction,
continue;
if (update-reflog_fd == -1)
continue;
-
+ if (update-flags REFLOG_TRUNCATE)
+ if (lseek(update-reflog_fd, 0, SEEK_SET) 0 ||
+ ftruncate(update-reflog_fd, 0)) {
+ error(Could not truncate reflog: %s. %s,
+ update-refname, strerror(errno));
+ rollback_lock_file(update-reflog_lock);
+ update-reflog_fd = -1;
+ continue;
+ }
if (log_ref_write_fd(update-reflog_fd, update-old_sha1,
update-new_sha1,
update-committer, update-msg)) {
diff --git a/refs.h b/refs.h
index 8220d18..5075073 100644
--- a/refs.h
+++ b/refs.h
@@ -328,7 +328,15 @@ int transaction_delete_ref(struct transaction *transaction,
struct strbuf *err);
/*
- * Append a reflog entry for refname.
+ * Flags controlling transaction_update_reflog().
+ * REFLOG_TRUNCATE: Truncate the reflog.
+ *
+ * Flags = 0x100 are reserved for internal use.
+ */
+#define REFLOG_TRUNCATE 0x01
+/*
+ * Append a reflog entry for refname. If the REFLOG_TRUNCATE flag is set
+ * this update will first truncate the reflog before writing the entry.
*/
int transaction_update_reflog(struct transaction *transaction,
const char *refname,
--
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 v3 04/14] refs.c: add a function to append a reflog entry to a fd
From: Ronnie Sahlberg sahlb...@google.com
Break out the code to create the string and writing it to the file
descriptor from log_ref_write and add it into a dedicated function
log_ref_write_fd. For now this is only used from log_ref_write,
but later on we will call this function from reflog transactions too,
which means that we will end up with only a single place,
where we write a reflog entry to a file instead of the current two
places (log_ref_write and builtin/reflog.c).
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 | 48 ++--
1 file changed, 30 insertions(+), 18 deletions(-)
diff --git a/refs.c b/refs.c
index 2044d8f..f0f0d23 100644
--- a/refs.c
+++ b/refs.c
@@ -2990,15 +2990,37 @@ int log_ref_setup(const char *refname, char *logfile,
int bufsize)
return 0;
}
+static int log_ref_write_fd(int fd, const unsigned char *old_sha1,
+ const unsigned char *new_sha1,
+ const char *committer, const char *msg)
+{
+ int msglen, written;
+ unsigned maxlen, len;
+ char *logrec;
+
+ msglen = msg ? strlen(msg) : 0;
+ maxlen = strlen(committer) + msglen + 100;
+ logrec = xmalloc(maxlen);
+ len = sprintf(logrec, %s %s %s\n,
+ sha1_to_hex(old_sha1),
+ sha1_to_hex(new_sha1),
+ committer);
+ if (msglen)
+ len += copy_msg(logrec + len - 1, msg) - 1;
+
+ written = len = maxlen ? write_in_full(fd, logrec, len) : -1;
+ free(logrec);
+ if (written != len)
+ return -1;
+
+ return 0;
+}
+
static int log_ref_write(const char *refname, const unsigned char *old_sha1,
const unsigned char *new_sha1, const char *msg)
{
- int logfd, result, written, oflags = O_APPEND | O_WRONLY;
- unsigned maxlen, len;
- int msglen;
+ int logfd, result, oflags = O_APPEND | O_WRONLY;
char log_file[PATH_MAX];
- char *logrec;
- const char *committer;
if (log_all_ref_updates 0)
log_all_ref_updates = !is_bare_repository();
@@ -3010,19 +3032,9 @@ static int log_ref_write(const char *refname, const
unsigned char *old_sha1,
logfd = open(log_file, oflags);
if (logfd 0)
return 0;
- msglen = msg ? strlen(msg) : 0;
- committer = git_committer_info(0);
- maxlen = strlen(committer) + msglen + 100;
- logrec = xmalloc(maxlen);
- len = sprintf(logrec, %s %s %s\n,
- sha1_to_hex(old_sha1),
- sha1_to_hex(new_sha1),
- committer);
- if (msglen)
- len += copy_msg(logrec + len - 1, msg) - 1;
- written = len = maxlen ? write_in_full(logfd, logrec, len) : -1;
- free(logrec);
- if (written != len) {
+ result = log_ref_write_fd(logfd, old_sha1, new_sha1,
+ git_committer_info(0), msg);
+ if (result) {
int save_errno = errno;
close(logfd);
error(Unable to append to %s, log_file);
--
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 v3 12/14] refs.c: Remove unlock_ref/close_ref/commit_ref from the refs api
From: Ronnie Sahlberg sahlb...@google.com
unlock|close|commit_ref can be made static since there are no more external
callers.
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 | 24
refs.h | 9 -
2 files changed, 12 insertions(+), 21 deletions(-)
diff --git a/refs.c b/refs.c
index 11cf26b..e49ae11 100644
--- a/refs.c
+++ b/refs.c
@@ -2096,6 +2096,16 @@ int refname_match(const char *abbrev_name, const char
*full_name)
return 0;
}
+static void unlock_ref(struct ref_lock *lock)
+{
+ /* Do not free lock-lk -- atexit() still looks at them */
+ if (lock-lk)
+ rollback_lock_file(lock-lk);
+ free(lock-ref_name);
+ free(lock-orig_ref_name);
+ free(lock);
+}
+
/* This function should make sure errno is meaningful on error */
static struct ref_lock *verify_lock(struct ref_lock *lock,
const unsigned char *old_sha1, int mustexist)
@@ -2894,7 +2904,7 @@ int rename_ref(const char *oldrefname, const char
*newrefname, const char *logms
return 1;
}
-int close_ref(struct ref_lock *lock)
+static int close_ref(struct ref_lock *lock)
{
if (close_lock_file(lock-lk))
return -1;
@@ -2902,7 +2912,7 @@ int close_ref(struct ref_lock *lock)
return 0;
}
-int commit_ref(struct ref_lock *lock)
+static int commit_ref(struct ref_lock *lock)
{
if (commit_lock_file(lock-lk))
return -1;
@@ -2910,16 +2920,6 @@ int commit_ref(struct ref_lock *lock)
return 0;
}
-void unlock_ref(struct ref_lock *lock)
-{
- /* Do not free lock-lk -- atexit() still looks at them */
- if (lock-lk)
- rollback_lock_file(lock-lk);
- free(lock-ref_name);
- free(lock-orig_ref_name);
- free(lock);
-}
-
/*
* copy the reflog message msg to buf, which has been allocated sufficiently
* large, while cleaning up the whitespaces. Especially, convert LF to space,
diff --git a/refs.h b/refs.h
index 17e3a3c..025e2cb 100644
--- a/refs.h
+++ b/refs.h
@@ -198,15 +198,6 @@ extern struct ref_lock *lock_any_ref_for_update(const char
*refname,
const unsigned char *old_sha1,
int flags, int *type_p);
-/** Close the file descriptor owned by a lock and return the status */
-extern int close_ref(struct ref_lock *lock);
-
-/** Close and commit the ref locked by the lock */
-extern int commit_ref(struct ref_lock *lock);
-
-/** Release any lock taken but not written. **/
-extern void unlock_ref(struct ref_lock *lock);
-
/** Reads log for the value of ref during at_time. **/
extern int read_ref_at(const char *refname, unsigned int flags,
unsigned long at_time, int cnt,
--
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: Getting a commit sha1 from fast-import in a remote-helper
Mike Hommey wrote:
- fast-import's `ls` command documentation about its output format
mentions that the output may contain commits, so I tried the trick of
creating a tree with commits, but fast-import then fails with:
fatal: Not a blob (actually a commit)
which I totally understand, but then I wonder why the documentation
mentions it and how one would get a tree containing references to
commits. I guess the documentation should be fixed.
Odd. Here's what happens when I try:
$ echo ls $(git rev-parse HEAD) | git fast-import --quiet
fatal: Missing space after tree-ish: ls
a4a226a366ab0a173ed9e5f70f2a95d0d21e54c5
fast-import: dumping crash report to .git/fast_import_crash_14080
$ echo ls $(git rev-parse HEAD) | git fast-import --quiet
04 tree d3d38e7d71cb40ebbaf2798b01837b3de43fd4a1
How did you get that Not a blob message?
I think a good fix would be to teach parse_ls a mode with no path
parameter. Something like this (untested; needs cleanup and tests):
Signed-off-by: Jonathan Nieder jrnie...@gmail.com
diff --git i/fast-import.c w/fast-import.c
index d0bd285..a9a46be 100644
--- i/fast-import.c
+++ w/fast-import.c
@@ -278,6 +278,8 @@ struct recent_command {
char *buf;
};
+extern const char *tag_type;
+
/* Configured limits on output */
static unsigned long max_depth = 10;
static off_t max_packsize;
@@ -3047,6 +3049,49 @@ static void parse_ls(const char *p, struct branch *b)
struct tree_entry *root = NULL;
struct tree_entry leaf = {NULL};
+ /* ls SP tree-ish */
+ if (*p != '' !strchr(p, ' ')) {
+ unsigned char sha1[20];
+ struct object_entry *e;
+ static struct strbuf line = STRBUF_INIT;
+ const char *type;
+
+ if (*p == ':') {/* mark */
+ e = find_mark(parse_mark_ref_eol(p));
+ if (!e)
+ die(Unknown mark: %s, command_buf.buf);
+ hashcpy(sha1, e-idx.sha1);
+ } else {/* sha1 */
+ if (get_sha1_hex(p, sha1))
+ die(Invalid dataref: %s, command_buf.buf);
+ e = find_object(sha1);
+ p += 40;
+ if (*p)
+ die(Garbage after dataref: %s,
command_buf.buf);
+ }
+
+ switch (e-type) {
+ case OBJ_COMMIT:
+ type = commit_type;
+ break;
+ case OBJ_TREE:
+ type = tree_type;
+ break;
+ case OBJ_BLOB:
+ type = blob_type;
+ break;
+ case OBJ_TAG:
+ type = tag_type;
+ break;
+ default:
+ die(Not a tree-ish: %s, command_buf.buf);
+ }
+
+ strbuf_reset(line);
+ strbuf_addf(line, %s %s\n, type, sha1_to_hex(sha1));
+ cat_blob_write(line.buf, line.len);
+ }
+
/* ls SP (tree-ish SP)? path */
if (*p == '') {
if (!b)
--
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 v4 01/16] refs.c: allow passing raw git_committer_info as email to _update_reflog
From: Ronnie Sahlberg sahlb...@google.com
In many places in the code we do not have access to the individual fields
in the committer data. Instead we might only have access to prebaked data
such as what is returned by git_committer_info() containing a string
that consists of email, timestamp, zone etc.
This makes it inconvenient to use transaction_update_reflog since it means
you would have to first parse git_committer_info before you can call
update_reflog.
Add a new flag REFLOG_COMMITTER_INFO_IS_VALID to _update_reflog to tell it
that we pass in a fully prebaked committer info string that can be used as is.
At the same time, also go over and change all references from email
to id where the code actually refers to a committer id and not just an email
address. I.e. where the string is : NAME EMAIL
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
Signed-off-by: Jonathan Nieder jrnie...@gmail.com
Signed-off-by: Stefan Beller sbel...@google.com
---
builtin/reflog.c | 19 +--
refs.c | 21 -
refs.h | 25 +++--
3 files changed, 48 insertions(+), 17 deletions(-)
diff --git a/builtin/reflog.c b/builtin/reflog.c
index 6bb7454..be88a53 100644
--- a/builtin/reflog.c
+++ b/builtin/reflog.c
@@ -292,7 +292,7 @@ static int unreachable(struct expire_reflog_cb *cb, struct
commit *commit, unsig
}
static int expire_reflog_ent(unsigned char *osha1, unsigned char *nsha1,
- const char *email, unsigned long timestamp, int tz,
+ const char *id, unsigned long timestamp, int tz,
const char *message, void *cb_data)
{
struct expire_reflog_cb *cb = cb_data;
@@ -320,9 +320,14 @@ static int expire_reflog_ent(unsigned char *osha1,
unsigned char *nsha1,
goto prune;
if (cb-t) {
+ struct reflog_committer_info ci;
+
+ memset(ci, 0, sizeof(ci));
+ ci.id = id;
+ ci.timestamp = timestamp;
+ ci.tz = tz;
if (transaction_update_reflog(cb-t, cb-refname, nsha1, osha1,
- email, timestamp, tz, message, 0,
- err))
+ ci, message, 0, err))
return -1;
hashcpy(cb-last_kept_sha1, nsha1);
}
@@ -356,6 +361,7 @@ static int expire_reflog(const char *ref, const unsigned
char *sha1, int unused,
struct expire_reflog_cb cb;
struct commit *tip_commit;
struct commit_list *tips;
+ struct reflog_committer_info ci;
int status = 0;
memset(cb, 0, sizeof(cb));
@@ -368,9 +374,10 @@ static int expire_reflog(const char *ref, const unsigned
char *sha1, int unused,
status |= error(%s, err.buf);
goto cleanup;
}
+
+ memset(ci, 0, sizeof(ci));
if (transaction_update_reflog(cb.t, cb.refname, null_sha1, null_sha1,
- NULL, 0, 0, NULL, REFLOG_TRUNCATE,
- err)) {
+ ci, NULL, REFLOG_TRUNCATE, err)) {
status |= error(%s, err.buf);
goto cleanup;
}
@@ -672,7 +679,7 @@ static int cmd_reflog_expire(int argc, const char **argv,
const char *prefix)
}
static int count_reflog_ent(unsigned char *osha1, unsigned char *nsha1,
- const char *email, unsigned long timestamp, int tz,
+ const char *id, unsigned long timestamp, int tz,
const char *message, void *cb_data)
{
struct cmd_reflog_expire_cb *cb = cb_data;
diff --git a/refs.c b/refs.c
index d75af6c..f30512f 100644
--- a/refs.c
+++ b/refs.c
@@ -3226,7 +3226,7 @@ struct read_ref_at_cb {
};
static int read_ref_at_ent(unsigned char *osha1, unsigned char *nsha1,
- const char *email, unsigned long timestamp, int tz,
+ const char *id, unsigned long timestamp, int tz,
const char *message, void *cb_data)
{
struct read_ref_at_cb *cb = cb_data;
@@ -3273,7 +3273,7 @@ static int read_ref_at_ent(unsigned char *osha1, unsigned
char *nsha1,
}
static int read_ref_at_ent_oldest(unsigned char *osha1, unsigned char *nsha1,
- const char *email, unsigned long timestamp,
+ const char *id, unsigned long timestamp,
int tz, const char *message, void *cb_data)
{
struct read_ref_at_cb *cb = cb_data;
@@ -3625,8 +3625,7 @@ int transaction_update_reflog(struct transaction
*transaction,
const char *refname,
const unsigned char *new_sha1,
const unsigned char *old_sha1,
- const char *email,
- unsigned long timestamp, int tz,
+
[PATCH v4 04/16] refs.c: use a stringlist for repack_without_refs
From: Ronnie Sahlberg sahlb...@google.com
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
Signed-off-by: Stefan Beller sbel...@google.com
---
builtin/remote.c | 23 ---
refs.c | 42 +-
refs.h | 2 +-
3 files changed, 30 insertions(+), 37 deletions(-)
diff --git a/builtin/remote.c b/builtin/remote.c
index c25420f..6806251 100644
--- a/builtin/remote.c
+++ b/builtin/remote.c
@@ -751,7 +751,6 @@ 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;
if (lock_packed_refs(0)) {
@@ -763,13 +762,9 @@ static int remove_branches(struct string_list *branches)
return -1;
}
- 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;
@@ -1327,7 +1322,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!);
@@ -1335,6 +1329,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),
@@ -1342,9 +1341,6 @@ 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;
@@ -1353,19 +1349,16 @@ static int prune_remote(const char *remote, int dry_run)
errno, err);
result |= error(%s, err.buf);
} else
- 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 3d309bb..519bfed 100644
--- a/refs.c
+++ b/refs.c
@@ -2663,31 +2663,32 @@ static int curate_packed_ref_fn(struct ref_entry
*entry, void *cb_data)
/*
* Must be called with packed refs already locked (and sorted)
*/
-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 processing if we have nothing to do */
- if (i == n) {
+ /* No refname exists in packed refs */
+ if (!count) {
rollback_packed_refs();
- return 0; /* no refname exists in packed refs */
+ return 0;
}
packed = get_packed_refs(ref_cache);
/* Remove refnames from the cache */
-
[PATCH v4 11/16] refs.c: make repack_without_refs static
From: Ronnie Sahlberg sahlb...@google.com
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
Signed-off-by: Stefan Beller sbel...@google.com
---
refs.c | 2 +-
refs.h | 3 ---
2 files changed, 1 insertion(+), 4 deletions(-)
diff --git a/refs.c b/refs.c
index 130d240..899c33e 100644
--- a/refs.c
+++ b/refs.c
@@ -2668,7 +2668,7 @@ static int curate_packed_ref_fn(struct ref_entry *entry,
void *cb_data)
/*
* Must be called with packed refs already locked (and sorted)
*/
-int repack_without_refs(struct string_list *without, struct strbuf *err)
+static 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;
diff --git a/refs.h b/refs.h
index 0ba078e..ce290b1 100644
--- a/refs.h
+++ b/refs.h
@@ -163,9 +163,6 @@ extern void rollback_packed_refs(void);
*/
int pack_refs(unsigned int flags);
-extern int repack_without_refs(struct string_list *without,
- struct strbuf *err);
-
extern int ref_exists(const char *);
extern int is_branch(const char *refname);
--
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 v4 03/16] refs.c: use packed refs when deleting refs during a transaction
From: Ronnie Sahlberg sahlb...@google.com
Make the deletion of refs during a transaction more atomic.
Start by first copying all loose refs we will be deleting to the packed
refs file and then commit the packed refs file. Then re-lock the packed refs
file to stop anyone else from modifying these refs and keep it locked until
we are finished.
Since all refs we are about to delete are now safely held in the packed refs
file we can proceed to immediately unlink any corresponding loose refs
and still be fully rollback-able.
The exception is for refs that can not be resolved. Those refs are never
added to the packed refs and will just be un-rollback-ably deleted during
commit.
By deleting all the loose refs at the start of the transaction we make
it possible to both delete one ref and then re-create a different ref in
the same transaction even if the two refs would normally conflict.
Example: rename m-m/m
In that example we want to delete the file 'm' so that we make room so
that we can create a directory with the same name in order to lock and
write to the ref m/m and its lock-file m/m.lock.
If there is a failure during the commit phase we can rollback without losing
any refs since we have so far only deleted loose refs that that are
guaranteed to also have a corresponding entry in the packed refs file.
Once we have finished all updates for refs and their reflogs we can repack
the packed refs file and remove the to-be-deleted refs from the packed refs,
at which point all the deleted refs will disappear in one atomic rename
operation.
This also means that for an outside observer, deletion of multiple refs
in a single transaction will look atomic instead of one ref being deleted
at a time.
In order to do all this we need to change the semantics for the
repack_without_refs function so that we can lock the packed refs file,
do other stuff, and later be able to call repack_without_refs with the
lock already taken.
This means we need some additional changes in remote.c to reflect the
changes to the repack_without_refs semantics.
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
Signed-off-by: Jonathan Nieder jrnie...@gmail.com
Signed-off-by: Stefan Beller sbel...@google.com
---
builtin/remote.c | 20 ++-
refs.c | 155 ++-
2 files changed, 138 insertions(+), 37 deletions(-)
diff --git a/builtin/remote.c b/builtin/remote.c
index 7f28f92..c25420f 100644
--- a/builtin/remote.c
+++ b/builtin/remote.c
@@ -1,4 +1,5 @@
#include builtin.h
+#include lockfile.h
#include parse-options.h
#include transport.h
#include remote.h
@@ -753,6 +754,15 @@ static int remove_branches(struct string_list *branches)
const char **branch_names;
int i, result = 0;
+ if (lock_packed_refs(0)) {
+ struct strbuf err = STRBUF_INIT;
+
+ unable_to_lock_message(git_path(packed-refs), errno, err);
+ error(%s, err.buf);
+ strbuf_release(err);
+ return -1;
+ }
+
branch_names = xmalloc(branches-nr * sizeof(*branch_names));
for (i = 0; i branches-nr; i++)
branch_names[i] = branches-items[i].string;
@@ -1337,9 +1347,15 @@ static int prune_remote(const char *remote, int dry_run)
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 (lock_packed_refs(0)) {
+ unable_to_lock_message(git_path(packed-refs),
+ errno, err);
result |= error(%s, err.buf);
+ } else
+ if (repack_without_refs(delete_refs,
+ states.stale.nr, err))
+ result |= error(%s, err.buf);
strbuf_release(err);
}
free(delete_refs);
diff --git a/refs.c b/refs.c
index 78eb46f..3d309bb 100644
--- a/refs.c
+++ b/refs.c
@@ -2660,6 +2660,9 @@ static int curate_packed_ref_fn(struct ref_entry *entry,
void *cb_data)
return 0;
}
+/*
+ * Must be called with packed refs already locked (and sorted)
+ */
int repack_without_refs(const char **refnames, int n, struct strbuf *err)
{
struct ref_dir *packed;
@@ -2674,14 +2677,12 @@ int repack_without_refs(const char **refnames, int n,
struct strbuf *err)
if (get_packed_ref(refnames[i]))
break;
- /* Avoid locking if we have nothing to do */
- if (i == n)
+ /* Avoid processing if we have nothing to do */
+ if (i == n) {
+ rollback_packed_refs();
return 0; /* no refname
[PATCH v4 16/16] refs.c: add an err argument to pack_refs
From: Ronnie Sahlberg sahlb...@google.com
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
Signed-off-by: Stefan Beller sbel...@google.com
---
builtin/pack-refs.c | 8 +++-
refs.c | 7 +++
refs.h | 3 ++-
3 files changed, 12 insertions(+), 6 deletions(-)
diff --git a/builtin/pack-refs.c b/builtin/pack-refs.c
index b20b1ec..299768e 100644
--- a/builtin/pack-refs.c
+++ b/builtin/pack-refs.c
@@ -10,6 +10,7 @@ static char const * const pack_refs_usage[] = {
int cmd_pack_refs(int argc, const char **argv, const char *prefix)
{
unsigned int flags = PACK_REFS_PRUNE;
+ struct strbuf err = STRBUF_INIT;
struct option opts[] = {
OPT_BIT(0, all, flags, N_(pack everything),
PACK_REFS_ALL),
OPT_BIT(0, prune, flags, N_(prune loose refs (default)),
PACK_REFS_PRUNE),
@@ -17,5 +18,10 @@ int cmd_pack_refs(int argc, const char **argv, const char
*prefix)
};
if (parse_options(argc, argv, prefix, opts, pack_refs_usage, 0))
usage_with_options(pack_refs_usage, opts);
- return pack_refs(flags);
+ if (pack_refs(flags, err)) {
+ error(%s, err.buf);
+ strbuf_release(err);
+ return -1;
+ }
+ return 0;
}
diff --git a/refs.c b/refs.c
index 725945e..ddb5fc6 100644
--- a/refs.c
+++ b/refs.c
@@ -2593,16 +2593,15 @@ static void prune_refs(struct ref_to_prune *r)
}
}
-int pack_refs(unsigned int flags)
+int pack_refs(unsigned int flags, struct strbuf *err)
{
struct pack_refs_cb_data cbdata;
- struct strbuf err = STRBUF_INIT;
memset(cbdata, 0, sizeof(cbdata));
cbdata.flags = flags;
- if (lock_packed_refs(err))
- die(%s, err.buf);
+ if (lock_packed_refs(err))
+ return -1;
cbdata.packed_refs = get_packed_refs(ref_cache);
diff --git a/refs.h b/refs.h
index b5ba685..489aa9d 100644
--- a/refs.h
+++ b/refs.h
@@ -130,8 +130,9 @@ extern void warn_dangling_symrefs(FILE *fp, const char
*msg_fmt, const struct st
/*
* Write a packed-refs file for the current repository.
* flags: Combination of the above PACK_REFS_* flags.
+ * Returns 0 on success and fills in err on failure.
*/
-int pack_refs(unsigned int flags);
+int pack_refs(unsigned int flags, struct strbuf *err);
extern int ref_exists(const char *);
--
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 v4 07/16] refs.c: rollback the lockfile before we die() in repack_without_refs
From: Ronnie Sahlberg sahlb...@google.com
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 | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/refs.c b/refs.c
index 780acd5..2db1dff 100644
--- a/refs.c
+++ b/refs.c
@@ -2707,8 +2707,10 @@ int repack_without_refs(struct string_list *without,
struct strbuf *err)
/* Remove any other accumulated cruft */
do_for_each_entry_in_dir(packed, 0, curate_packed_ref_fn,
refs_to_delete);
for_each_string_list_item(ref_to_delete, refs_to_delete) {
- if (remove_entry(packed, ref_to_delete-string) == -1)
+ if (remove_entry(packed, ref_to_delete-string) == -1) {
+ rollback_packed_refs();
die(internal error);
+ }
}
/* Write what remains */
--
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 v4 14/16] refs.c: make add_packed_ref return an error instead of calling die
From: Ronnie Sahlberg sahlb...@google.com
Change add_packed_ref to return an error instead of calling die().
Update all callers to check the return value of add_packed_ref.
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
Signed-off-by: Stefan Beller sbel...@google.com
---
refs.c | 21 +
1 file changed, 17 insertions(+), 4 deletions(-)
diff --git a/refs.c b/refs.c
index 3146288..c59cc3f 100644
--- a/refs.c
+++ b/refs.c
@@ -1229,15 +1229,16 @@ static struct ref_dir *get_packed_refs(struct ref_cache
*refs)
return get_packed_ref_dir(get_packed_ref_cache(refs));
}
-static void add_packed_ref(const char *refname, const unsigned char *sha1)
+static int add_packed_ref(const char *refname, const unsigned char *sha1)
{
struct packed_ref_cache *packed_ref_cache =
get_packed_ref_cache(ref_cache);
if (!packed_ref_cache-lock)
- die(internal error: packed refs not locked);
+ return -1;
add_ref(get_packed_ref_dir(packed_ref_cache),
create_ref_entry(refname, sha1, REF_ISPACKED, 1));
+ return 0;
}
/*
@@ -3827,7 +3828,13 @@ int transaction_commit(struct transaction *transaction,
sha1, NULL))
continue;
- add_packed_ref(update-refname, sha1);
+ if (add_packed_ref(update-refname, sha1)) {
+ if (err)
+ strbuf_addf(err, Failed to add %s to packed
+ refs, update-refname);
+ ret = -1;
+ goto cleanup;
+ }
need_repack = 1;
}
if (need_repack) {
@@ -3941,7 +3948,13 @@ int transaction_commit(struct transaction *transaction,
packed = get_packed_refs(ref_cache);
remove_entry(packed, update-refname);
- add_packed_ref(update-refname, update-new_sha1);
+ if (add_packed_ref(update-refname, update-new_sha1)) {
+ if (err)
+ strbuf_addf(err, Failed to add %s to packed
+ refs, update-refname);
+ ret = -1;
+ goto cleanup;
+ }
need_repack = 1;
try_remove_empty_parents((char *)update-refname);
--
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 v4 13/16] refs.c: replace the onerr argument in update_ref with a strbuf err
From: Ronnie Sahlberg sahlb...@google.com
Get rid of the action_on_err enum and replace the action argument to
update_ref with a strbuf *err for error reporting.
Update all callers to the new api including two callers in transport*.c
which used the literal 0 instead of an enum.
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
Signed-off-by: Stefan Beller sbel...@google.com
---
builtin/checkout.c | 7 +--
builtin/clone.c | 20
builtin/merge.c | 20 +---
builtin/notes.c | 24 ++--
builtin/reset.c | 12
builtin/update-ref.c | 7 +--
notes-cache.c| 2 +-
notes-utils.c| 5 +++--
refs.c | 14 +++---
refs.h | 10 ++
transport-helper.c | 7 ++-
transport.c | 9 ++---
12 files changed, 78 insertions(+), 59 deletions(-)
diff --git a/builtin/checkout.c b/builtin/checkout.c
index 8550b6d..60a68f7 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -584,6 +584,8 @@ static void update_refs_for_switch(const struct
checkout_opts *opts,
{
struct strbuf msg = STRBUF_INIT;
const char *old_desc, *reflog_msg;
+ struct strbuf err = STRBUF_INIT;
+
if (opts-new_branch) {
if (opts-new_orphan_branch) {
if (opts-new_branch_log !log_all_ref_updates) {
@@ -621,8 +623,9 @@ static void update_refs_for_switch(const struct
checkout_opts *opts,
if (!strcmp(new-name, HEAD) !new-path !opts-force_detach) {
/* Nothing to do. */
} else if (opts-force_detach || !new-path) { /* No longer on any
branch. */
- update_ref(msg.buf, HEAD, new-commit-object.sha1, NULL,
- REF_NODEREF, UPDATE_REFS_DIE_ON_ERR);
+ if (update_ref(msg.buf, HEAD, new-commit-object.sha1, NULL,
+ REF_NODEREF, err))
+ die(%s, err.buf);
if (!opts-quiet) {
if (old-path advice_detached_head)
detach_advice(new-name);
diff --git a/builtin/clone.c b/builtin/clone.c
index 9e4c42b..49a72ac 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -519,6 +519,7 @@ static void write_remote_refs(const struct ref *local_refs)
static void write_followtags(const struct ref *refs, const char *msg)
{
const struct ref *ref;
+ struct strbuf err = STRBUF_INIT;
for (ref = refs; ref; ref = ref-next) {
if (!starts_with(ref-name, refs/tags/))
continue;
@@ -526,8 +527,9 @@ static void write_followtags(const struct ref *refs, const
char *msg)
continue;
if (!has_sha1_file(ref-old_sha1))
continue;
- update_ref(msg, ref-name, ref-old_sha1,
- NULL, 0, UPDATE_REFS_DIE_ON_ERR);
+ if (update_ref(msg, ref-name, ref-old_sha1,
+ NULL, 0, err))
+ die(%s, err.buf);
}
}
@@ -590,28 +592,30 @@ static void update_remote_refs(const struct ref *refs,
static void update_head(const struct ref *our, const struct ref *remote,
const char *msg)
{
+ struct strbuf err = STRBUF_INIT;
const char *head;
if (our skip_prefix(our-name, refs/heads/, head)) {
/* Local default branch link */
create_symref(HEAD, our-name, NULL);
if (!option_bare) {
- update_ref(msg, HEAD, our-old_sha1, NULL, 0,
- UPDATE_REFS_DIE_ON_ERR);
+ update_ref(msg, HEAD, our-old_sha1, NULL, 0, err);
install_branch_config(0, head, option_origin,
our-name);
}
} else if (our) {
struct commit *c = lookup_commit_reference(our-old_sha1);
/* --branch specifies a non-branch (i.e. tags), detach HEAD */
- update_ref(msg, HEAD, c-object.sha1,
- NULL, REF_NODEREF, UPDATE_REFS_DIE_ON_ERR);
+ if (update_ref(msg, HEAD, c-object.sha1,
+ NULL, REF_NODEREF, err))
+ die(%s, err.buf);
} else if (remote) {
/*
* We know remote HEAD points to a non-branch, or
* HEAD points to a branch but we don't know which one.
* Detach HEAD in all these cases.
*/
- update_ref(msg, HEAD, remote-old_sha1,
- NULL, REF_NODEREF, UPDATE_REFS_DIE_ON_ERR);
+ if (update_ref(msg, HEAD, remote-old_sha1,
+NULL, REF_NODEREF, err))
+ die(%s, err.buf);
}
}
diff --git a/builtin/merge.c b/builtin/merge.c
index bebbe5b..a787b6a 100644
--- a/builtin/merge.c
+++
[PATCH v4 08/16] refs.c: move reflog updates into its own function
From: Ronnie Sahlberg sahlb...@google.com
write_ref_sha1 tries to update the reflog while updating the ref.
Move these reflog changes out into its own function so that we can do the
same thing if we write a sha1 ref differently, for example by writing a ref
to the packed refs file instead.
No functional changes intended. We only move some code out into a separate
function.
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
Signed-off-by: Stefan Beller sbel...@google.com
---
refs.c | 60 +++-
1 file changed, 35 insertions(+), 25 deletions(-)
diff --git a/refs.c b/refs.c
index 2db1dff..75c6d3b 100644
--- a/refs.c
+++ b/refs.c
@@ -3028,6 +3028,40 @@ int is_branch(const char *refname)
return !strcmp(refname, HEAD) || starts_with(refname, refs/heads/);
}
+static int write_sha1_update_reflog(struct ref_lock *lock,
+ const unsigned char *sha1, const char *logmsg)
+{
+ if (log_ref_write(lock-ref_name, lock-old_sha1, sha1, logmsg) 0 ||
+ (strcmp(lock-ref_name, lock-orig_ref_name)
+log_ref_write(lock-orig_ref_name, lock-old_sha1, sha1, logmsg)
0)) {
+ unlock_ref(lock);
+ return -1;
+ }
+ if (strcmp(lock-orig_ref_name, HEAD) != 0) {
+ /*
+* Special hack: If a branch is updated directly and HEAD
+* points to it (may happen on the remote side of a push
+* for example) then logically the HEAD reflog should be
+* updated too.
+* A generic solution implies reverse symref information,
+* but finding all symrefs pointing to the given branch
+* would be rather costly for this rare event (the direct
+* update of a branch) to be worth it. So let's cheat and
+* check with HEAD only which should cover 99% of all usage
+* scenarios (even 100% of the default ones).
+*/
+ unsigned char head_sha1[20];
+ int head_flag;
+ const char *head_ref;
+ head_ref = resolve_ref_unsafe(HEAD, RESOLVE_REF_READING,
+ head_sha1, head_flag);
+ if (head_ref (head_flag REF_ISSYMREF)
+ !strcmp(head_ref, lock-ref_name))
+ log_ref_write(HEAD, lock-old_sha1, sha1, logmsg);
+ }
+ return 0;
+}
+
/*
* Write sha1 into the ref specified by the lock. Make sure that errno
* is sane on error.
@@ -3071,34 +3105,10 @@ static int write_ref_sha1(struct ref_lock *lock,
return -1;
}
clear_loose_ref_cache(ref_cache);
- if (log_ref_write(lock-ref_name, lock-old_sha1, sha1, logmsg) 0 ||
- (strcmp(lock-ref_name, lock-orig_ref_name)
-log_ref_write(lock-orig_ref_name, lock-old_sha1, sha1, logmsg)
0)) {
+ if (write_sha1_update_reflog(lock, sha1, logmsg)) {
unlock_ref(lock);
return -1;
}
- if (strcmp(lock-orig_ref_name, HEAD) != 0) {
- /*
-* Special hack: If a branch is updated directly and HEAD
-* points to it (may happen on the remote side of a push
-* for example) then logically the HEAD reflog should be
-* updated too.
-* A generic solution implies reverse symref information,
-* but finding all symrefs pointing to the given branch
-* would be rather costly for this rare event (the direct
-* update of a branch) to be worth it. So let's cheat and
-* check with HEAD only which should cover 99% of all usage
-* scenarios (even 100% of the default ones).
-*/
- unsigned char head_sha1[20];
- int head_flag;
- const char *head_ref;
- head_ref = resolve_ref_unsafe(HEAD, RESOLVE_REF_READING,
- head_sha1, head_flag);
- if (head_ref (head_flag REF_ISSYMREF)
- !strcmp(head_ref, lock-ref_name))
- log_ref_write(HEAD, lock-old_sha1, sha1, logmsg);
- }
if (commit_ref(lock)) {
error(Couldn't set %s, lock-ref_name);
unlock_ref(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 v4 12/16] refs.c: make the *_packed_refs functions static
From: Ronnie Sahlberg sahlb...@google.com
We no longer need to expose the lock/add/commit/rollback functions
for packed refs anymore so make them static and remove them from the
public api.
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
Signed-off-by: Stefan Beller sbel...@google.com
---
refs.c | 8
refs.h | 30 --
2 files changed, 4 insertions(+), 34 deletions(-)
diff --git a/refs.c b/refs.c
index 899c33e..49d62b0 100644
--- a/refs.c
+++ b/refs.c
@@ -1229,7 +1229,7 @@ static struct ref_dir *get_packed_refs(struct ref_cache
*refs)
return get_packed_ref_dir(get_packed_ref_cache(refs));
}
-void add_packed_ref(const char *refname, const unsigned char *sha1)
+static void add_packed_ref(const char *refname, const unsigned char *sha1)
{
struct packed_ref_cache *packed_ref_cache =
get_packed_ref_cache(ref_cache);
@@ -2398,7 +2398,7 @@ static int write_packed_entry_fn(struct ref_entry *entry,
void *cb_data)
}
/* This should return a meaningful errno on failure */
-int lock_packed_refs(int flags)
+static int lock_packed_refs(int flags)
{
struct packed_ref_cache *packed_ref_cache;
@@ -2421,7 +2421,7 @@ int lock_packed_refs(int flags)
* Commit the packed refs changes.
* On error we must make sure that errno contains a meaningful value.
*/
-int commit_packed_refs(void)
+static int commit_packed_refs(void)
{
struct packed_ref_cache *packed_ref_cache =
get_packed_ref_cache(ref_cache);
@@ -2450,7 +2450,7 @@ int commit_packed_refs(void)
return error;
}
-void rollback_packed_refs(void)
+static void rollback_packed_refs(void)
{
struct packed_ref_cache *packed_ref_cache =
get_packed_ref_cache(ref_cache);
diff --git a/refs.h b/refs.h
index ce290b1..70a2819 100644
--- a/refs.h
+++ b/refs.h
@@ -120,36 +120,6 @@ extern void warn_dangling_symref(FILE *fp, const char
*msg_fmt, const char *refn
extern void warn_dangling_symrefs(FILE *fp, const char *msg_fmt, const struct
string_list *refnames);
/*
- * Lock the packed-refs file for writing. Flags is passed to
- * hold_lock_file_for_update(). Return 0 on success.
- * Errno is set to something meaningful on error.
- */
-extern int lock_packed_refs(int flags);
-
-/*
- * Add a reference to the in-memory packed reference cache. This may
- * only be called while the packed-refs file is locked (see
- * lock_packed_refs()). To actually write the packed-refs file, call
- * commit_packed_refs().
- */
-extern void add_packed_ref(const char *refname, const unsigned char *sha1);
-
-/*
- * Write the current version of the packed refs cache from memory to
- * disk. The packed-refs file must already be locked for writing (see
- * lock_packed_refs()). Return zero on success.
- * Sets errno to something meaningful on error.
- */
-extern int commit_packed_refs(void);
-
-/*
- * Rollback the lockfile for the packed-refs file, and discard the
- * in-memory packed reference cache. (The packed-refs file will be
- * read anew if it is needed again after this function is called.)
- */
-extern void rollback_packed_refs(void);
-
-/*
* Flags for controlling behaviour of pack_refs()
* PACK_REFS_PRUNE: Prune loose refs after packing
* PACK_REFS_ALL: Pack _all_ refs, not just tags and already packed refs
--
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 v4 10/16] remote.c: use a transaction for deleting refs
From: Ronnie Sahlberg sahlb...@google.com
Transactions now use packed refs when deleting multiple refs so there is no
need to do it manually from remote.c any more.
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
Signed-off-by: Stefan Beller sbel...@google.com
---
builtin/remote.c | 80
1 file changed, 40 insertions(+), 40 deletions(-)
diff --git a/builtin/remote.c b/builtin/remote.c
index 6806251..42702d7 100644
--- a/builtin/remote.c
+++ b/builtin/remote.c
@@ -750,30 +750,27 @@ static int mv(int argc, const char **argv)
static int remove_branches(struct string_list *branches)
{
- struct strbuf err = STRBUF_INIT;
int i, result = 0;
+ struct transaction *transaction;
+ struct strbuf err = STRBUF_INIT;
- if (lock_packed_refs(0)) {
- struct strbuf err = STRBUF_INIT;
-
- unable_to_lock_message(git_path(packed-refs), errno, err);
- error(%s, err.buf);
- strbuf_release(err);
- return -1;
- }
+ transaction = transaction_begin(err);
+ if (!transaction)
+ die(%s, err.buf);
- if (repack_without_refs(branches, err))
+ for (i = 0; i branches-nr; i++)
+ if (transaction_delete_ref(transaction,
+ branches-items[i].string, NULL,
+ 0, 0, remote-branches, err)) {
+ result |= error(%s, err.buf);
+ goto cleanup;
+ }
+ if (transaction_commit(transaction, err))
result |= error(%s, err.buf);
- strbuf_release(err);
-
- for (i = 0; i branches-nr; i++) {
- struct string_list_item *item = branches-items + i;
- const char *refname = item-string;
-
- if (delete_ref(refname, NULL, 0))
- result |= error(_(Could not remove branch %s),
refname);
- }
+ cleanup:
+ strbuf_release(err);
+ transaction_free(transaction);
return result;
}
@@ -1325,42 +1322,38 @@ static int prune_remote(const char *remote, int dry_run)
const char *dangling_msg = dry_run
? _( %s will become dangling!)
: _( %s has become dangling!);
+ struct transaction *transaction = NULL;
+ struct strbuf err = STRBUF_INIT;
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),
states.remote-url_nr
? states.remote-url[0]
: _((no URL)));
-
- if (!dry_run) {
- struct strbuf err = STRBUF_INIT;
-
- if (lock_packed_refs(0)) {
- unable_to_lock_message(git_path(packed-refs),
- errno, err);
- result |= error(%s, err.buf);
- } else
- if (repack_without_refs(delete_refs_list,
- err))
- result |= error(%s, err.buf);
- strbuf_release(err);
- }
}
+ if (!dry_run) {
+ transaction = transaction_begin(err);
+ if (!transaction)
+ die(%s, err.buf);
+ }
for (i = 0; i states.stale.nr; i++) {
const char *refname = states.stale.items[i].util;
- if (!dry_run)
- result |= delete_ref(refname, NULL, 0);
+ string_list_insert(delete_refs_list, refname);
+
+ if (!dry_run) {
+ if (transaction_delete_ref(transaction,
+ refname, NULL,
+ 0, 0, remote-branches, err)) {
+ result |= error(%s, err.buf);
+ goto cleanup;
+ }
+ }
if (dry_run)
printf_ln(_( * [would prune] %s),
@@ -1370,6 +1363,13 @@ static int prune_remote(const char *remote, int dry_run)
abbrev_ref(refname, refs/remotes/));
}
+ if (!dry_run)
+ if (transaction_commit(transaction, err))
+ result |= error(%s, err.buf);
+
+ cleanup:
+ strbuf_release(err);
+ transaction_free(transaction);
warn_dangling_symrefs(stdout, dangling_msg, delete_refs_list);
[PATCH v4 05/16] refs.c: add transaction support for renaming a reflog
From: Ronnie Sahlberg sahlb...@google.com
Add a new transaction function transaction_rename_reflog.
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
Signed-off-by: Stefan Beller sbel...@google.com
---
refs.c | 72 +-
refs.h | 8
2 files changed, 79 insertions(+), 1 deletion(-)
diff --git a/refs.c b/refs.c
index 519bfed..c721184 100644
--- a/refs.c
+++ b/refs.c
@@ -35,6 +35,11 @@ static unsigned char refname_disposition[256] = {
* just use the lock taken by the first update.
*/
#define UPDATE_REFLOG_NOLOCK 0x0200
+/*
+ * This update is used to replace a new or existing reflog with new content
+ * held in update-new_reflog.
+ */
+#define REFLOG_REPLACE 0x0400
/*
* Try to read one refname component from the front of refname.
@@ -2821,6 +2826,37 @@ static int rename_ref_available(const char *oldname,
const char *newname)
static int write_ref_sha1(struct ref_lock *lock, const unsigned char *sha1,
const char *logmsg);
+/*
+ * This is an optimized function to read the whole reflog as a blob
+ * into a strbuf. It is used during ref_rename so that we can use an
+ * efficient method to read the whole log and later write it back to a
+ * different file.
+ */
+static int copy_reflog_into_strbuf(const char *refname, struct strbuf *buf)
+{
+ struct stat st;
+ int fd;
+
+ if (lstat(git_path(logs/%s, refname), st) == -1)
+ return 1;
+ if ((fd = open(git_path(logs/%s, refname), O_RDONLY)) == -1) {
+ error(failed to open reflog %s, %s,
+ refname, strerror(errno));
+ return 1;
+ }
+ strbuf_init(buf, st.st_size);
+ strbuf_setlen(buf, st.st_size);
+ if (read_in_full(fd, buf-buf, st.st_size) != st.st_size) {
+ close(fd);
+ error(failed to read reflog %s, %s,
+ refname, strerror(errno));
+ return 1;
+ }
+ close(fd);
+
+ return 0;
+}
+
int rename_ref(const char *oldrefname, const char *newrefname, const char
*logmsg)
{
unsigned char sha1[20], orig_sha1[20];
@@ -3561,6 +3597,7 @@ struct ref_update {
struct lock_file *reflog_lock;
char *committer;
struct ref_update *orig_update; /* For UPDATE_REFLOG_NOLOCK */
+ struct strbuf new_reflog;
const char refname[FLEX_ARRAY];
};
@@ -3607,6 +3644,7 @@ void transaction_free(struct transaction *transaction)
return;
for (i = 0; i transaction-nr; i++) {
+ strbuf_release(transaction-updates[i]-new_reflog);
free(transaction-updates[i]-msg);
free(transaction-updates[i]-committer);
free(transaction-updates[i]);
@@ -3622,6 +3660,7 @@ static struct ref_update *add_update(struct transaction
*transaction,
size_t len = strlen(refname);
struct ref_update *update = xcalloc(1, sizeof(*update) + len + 1);
+ strbuf_init(update-new_reflog, 0);
strcpy((char *)update-refname, refname);
update-update_type = update_type;
ALLOC_GROW(transaction-updates, transaction-nr + 1,
transaction-alloc);
@@ -3681,6 +3720,27 @@ int transaction_update_reflog(struct transaction
*transaction,
return 0;
}
+int transaction_rename_reflog(struct transaction *transaction,
+ const char *oldrefname,
+ const char *newrefname,
+ struct strbuf *err)
+{
+ struct ref_update *update;
+
+ if (transaction-state != TRANSACTION_OPEN)
+ die(BUG: transaction_replace_reflog called for transaction
+ that is not open);
+
+ update = add_update(transaction, newrefname, UPDATE_LOG);
+ update-flags = REFLOG_REPLACE;
+ update-reflog_lock = xcalloc(1, sizeof(struct lock_file));
+ if (copy_reflog_into_strbuf(oldrefname, update-new_reflog))
+ die(BUG: failed to read old reflog in
+ transaction_rename_reflog);
+
+ return 0;
+}
+
int transaction_update_ref(struct transaction *transaction,
const char *refname,
const unsigned char *new_sha1,
@@ -4012,7 +4072,7 @@ int transaction_commit(struct transaction *transaction,
continue;
if (update-reflog_fd == -1)
continue;
- if (update-flags REFLOG_TRUNCATE)
+ if (update-flags (REFLOG_TRUNCATE|REFLOG_REPLACE))
if (lseek(update-reflog_fd, 0, SEEK_SET) 0 ||
ftruncate(update-reflog_fd, 0)) {
error(Could not truncate reflog: %s. %s,
@@ -4030,6 +4090,16 @@ int transaction_commit(struct transaction *transaction,
rollback_lock_file(update-reflog_lock);
[PATCH v4 09/16] refs.c: write updates to packed refs when a transaction has more than one ref
From: Ronnie Sahlberg sahlb...@google.com
When we are updating more than one single ref, i.e. not a commit, then
write the updated refs directly to the packed refs file instead of writing
them as loose refs.
Change clone to use a transaction instead of using the packed refs API.
This changes the behavior of clone slightly. Previously clone would always
clone all refs into a packed refs file. With this change clone will only
clone into packed refs iff there are two or more refs being cloned.
If the repository we are cloning from only contains exactly one single ref
then clone will now store this as a loose ref. The benefit here is that
we no longer need to export a bunch of API functions to clone to access
packed refs directly. Clone can now just use a normal transaction and all
the packed refs goodness will happen automatically.
Update the t5516 test to cope with the fact that clone now only uses
packed refs if there are more than one ref being cloned.
We still use loose refs for single ref transactions, such as are used
by 'git commit' and friends. The reason for this is that if you have very
many refs then having to re-write the whole packed refs file for every
common operation like commit would have a performance impact.
That said, with these changes it should now be fairly straightforward to
add support to optionally start using packed refs for ALL updates
which could solve existing issues with name clashes in case insensitive
filesystems.
This change also means that multi-ref updates will now appear as a single
atomic change to any external observers instead of a sequence of discreete
changes.
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
Signed-off-by: Stefan Beller sbel...@google.com
---
builtin/clone.c | 16 ++---
refs.c| 89 ++-
t/t5516-fetch-push.sh | 2 +-
3 files changed, 72 insertions(+), 35 deletions(-)
diff --git a/builtin/clone.c b/builtin/clone.c
index d5e7532..9e4c42b 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -495,17 +495,25 @@ static struct ref *wanted_peer_refs(const struct ref
*refs,
static void write_remote_refs(const struct ref *local_refs)
{
const struct ref *r;
+ struct transaction *transaction;
+ struct strbuf err = STRBUF_INIT;
- lock_packed_refs(LOCK_DIE_ON_ERROR);
+ transaction = transaction_begin(err);
+ if (!transaction)
+ die(%s, err.buf);
for (r = local_refs; r; r = r-next) {
if (!r-peer_ref)
continue;
- add_packed_ref(r-peer_ref-name, r-old_sha1);
+ if (transaction_update_ref(transaction, r-peer_ref-name,
+ r-old_sha1, NULL, 0, 0, NULL,
+ err))
+ die(%s, err.buf);
}
- if (commit_packed_refs())
- die_errno(unable to overwrite old ref-pack file);
+ if (transaction_commit(transaction, err))
+ die(%s, err.buf);
+ transaction_free(transaction);
}
static void write_followtags(const struct ref *refs, const char *msg)
diff --git a/refs.c b/refs.c
index 75c6d3b..130d240 100644
--- a/refs.c
+++ b/refs.c
@@ -2673,36 +2673,15 @@ 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 count, ret, removed = 0;
+ int ret;
assert(err);
- /* Look for a packed ref */
- count = 0;
- for_each_string_list_item(ref_to_delete, without)
- if (get_packed_ref(ref_to_delete-string))
- count++;
-
- /* No refname exists in packed refs */
- if (!count) {
- rollback_packed_refs();
- return 0;
- }
-
packed = get_packed_refs(ref_cache);
/* Remove refnames from the cache */
for_each_string_list_item(ref_to_delete, without)
- if (remove_entry(packed, ref_to_delete-string) != -1)
- removed = 1;
- if (!removed) {
- /*
-* All packed entries disappeared while we were
-* acquiring the lock.
-*/
- rollback_packed_refs();
- return 0;
- }
+ remove_entry(packed, ref_to_delete-string);
/* Remove any other accumulated cruft */
do_for_each_entry_in_dir(packed, 0, curate_packed_ref_fn,
refs_to_delete);
@@ -3791,6 +3770,7 @@ int transaction_commit(struct transaction *transaction,
struct strbuf *err)
{
int ret = 0, i, need_repack = 0;
+ int num_updates = 0;
int n = transaction-nr;
struct packed_ref_cache *packed_ref_cache;
struct ref_update **updates = transaction-updates;
@@ -3824,14
[PATCH v4 15/16] refs.c: make lock_packed_refs take an err argument
From: Ronnie Sahlberg sahlb...@google.com
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
Signed-off-by: Stefan Beller sbel...@google.com
---
refs.c | 25 +
1 file changed, 13 insertions(+), 12 deletions(-)
diff --git a/refs.c b/refs.c
index c59cc3f..725945e 100644
--- a/refs.c
+++ b/refs.c
@@ -2398,13 +2398,17 @@ static int write_packed_entry_fn(struct ref_entry
*entry, void *cb_data)
return 0;
}
-/* This should return a meaningful errno on failure */
-static int lock_packed_refs(int flags)
+static int lock_packed_refs(struct strbuf *err)
{
struct packed_ref_cache *packed_ref_cache;
- if (hold_lock_file_for_update(packlock, git_path(packed-refs),
flags) 0)
+ if (hold_lock_file_for_update(packlock, git_path(packed-refs),
+ 0) 0) {
+ if (err)
+ unable_to_lock_message(git_path(packed-refs),
+ errno, err);
return -1;
+ }
/*
* Get the current packed-refs while holding the lock. If the
* packed-refs file has been modified since we last read it,
@@ -2592,11 +2596,14 @@ static void prune_refs(struct ref_to_prune *r)
int pack_refs(unsigned int flags)
{
struct pack_refs_cb_data cbdata;
+ struct strbuf err = STRBUF_INIT;
memset(cbdata, 0, sizeof(cbdata));
cbdata.flags = flags;
- lock_packed_refs(LOCK_DIE_ON_ERROR);
+ if (lock_packed_refs(err))
+ die(%s, err.buf);
+
cbdata.packed_refs = get_packed_refs(ref_cache);
do_for_each_entry_in_dir(get_loose_refs(ref_cache), 0,
@@ -3789,10 +3796,7 @@ int transaction_commit(struct transaction *transaction,
}
/* Lock packed refs during commit */
- if (lock_packed_refs(0)) {
- if (err)
- unable_to_lock_message(git_path(packed-refs),
- errno, err);
+ if (lock_packed_refs(err)) {
ret = -1;
goto cleanup;
}
@@ -3847,10 +3851,7 @@ int transaction_commit(struct transaction *transaction,
goto cleanup;
}
/* lock the packed refs again so no one can change it */
- if (lock_packed_refs(0)) {
- if (err)
- unable_to_lock_message(git_path(packed-refs),
- errno, err);
+ if (lock_packed_refs(err)) {
ret = -1;
goto cleanup;
}
--
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 v4 02/16] refs.c: return error instead of dying when locking fails during transaction
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.
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 f30512f..78eb46f 100644
--- a/refs.c
+++ b/refs.c
@@ -2340,6 +2340,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
@@ -2347,8 +2348,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 v4 00/16] ref-transaction-rename
Hi, This series builds on the previous series : ref-transaction-reflog as applied to master. This series has been sent to the list before[1] This series can also be found at github[2] as well as googlesource[3]. This series converts ref rename to use a transaction. This addesses several issues in the old implementation, such as colliding renames might overwrite someone elses reflog, and it makes the rename atomic. As part of the series we also move changes that cover multiple refs to happen as an atomic transaction/rename to the pacekd refs file. This makes it possible to have both the rename case (one deleted ref + one created ref) as well as any operation that updates multiple refs to become one atomic rename() applied to the packed refs file. Thus all such changes are now also atomic to all external observers. Thanks, Stefan Version 2: - Changed to not use potentially iterators to copy the reflog entries one by one. Instead adding two new functions. One to read an existing reflog as one big blob, and a second function to, in a transaction, write a new complete reflog from said blob. The idea is that each future reflog backend will provide optimized versions for these read whole reflog write whole reflog functions. Version 3: - Rename and redo the API for updating a whole reflog in one single operation to transaction_rename_reflog() Version 4: * As Ronnie announced to change employers soon, he'll have only limited time to work on git in the near future. As this is a rather large patch series, he is handing this work over to me. That's why I'm sending the patches this time. [1] http://www.spinics.net/lists/git/msg241352.html [2] https://github.com/stefanbeller/git/tree/ref-transactions-rename [3] https://code-review.googlesource.com/#/q/topic:ref-transaction-rename Ronnie Sahlberg (16): refs.c: allow passing raw git_committer_info as email to _update_reflog refs.c: return error instead of dying when locking fails during transaction refs.c: use packed refs when deleting refs during a transaction refs.c: use a stringlist for repack_without_refs refs.c: add transaction support for renaming a reflog refs.c: update rename_ref to use a transaction refs.c: rollback the lockfile before we die() in repack_without_refs refs.c: move reflog updates into its own function refs.c: write updates to packed refs when a transaction has more than one ref remote.c: use a transaction for deleting refs refs.c: make repack_without_refs static refs.c: make the *_packed_refs functions static refs.c: replace the onerr argument in update_ref with a strbuf err refs.c: make add_packed_ref return an error instead of calling die refs.c: make lock_packed_refs take an err argument refs.c: add an err argument to pack_refs builtin/checkout.c| 7 +- builtin/clone.c | 36 ++- builtin/merge.c | 20 +- builtin/notes.c | 24 +- builtin/pack-refs.c | 8 +- builtin/reflog.c | 19 +- builtin/remote.c | 69 +++--- builtin/reset.c | 12 +- builtin/update-ref.c | 7 +- notes-cache.c | 2 +- notes-utils.c | 5 +- refs.c| 616 ++ refs.h| 79 +++ t/t3200-branch.sh | 7 - t/t5516-fetch-push.sh | 2 +- transport-helper.c| 7 +- transport.c | 9 +- 17 files changed, 552 insertions(+), 377 deletions(-) -- 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 v4 06/16] refs.c: update rename_ref to use a transaction
From: Ronnie Sahlberg sahlb...@google.com
Change refs.c to use a single transaction to perform the rename.
Change the function to return 1 on failure instead of either -1 or 1.
These changes make the rename_ref operation atomic.
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
Signed-off-by: Stefan Beller sbel...@google.com
---
refs.c| 168 ++
t/t3200-branch.sh | 7 ---
2 files changed, 43 insertions(+), 132 deletions(-)
diff --git a/refs.c b/refs.c
index c721184..780acd5 100644
--- a/refs.c
+++ b/refs.c
@@ -2757,60 +2757,6 @@ int delete_ref(const char *refname, const unsigned char
*sha1, int delopt)
return 0;
}
-/*
- * People using contrib's git-new-workdir have .git/logs/refs -
- * /some/other/path/.git/logs/refs, and that may live on another device.
- *
- * IOW, to avoid cross device rename errors, the temporary renamed log must
- * live into logs/refs.
- */
-#define TMP_RENAMED_LOG logs/refs/.tmp-renamed-log
-
-static int rename_tmp_log(const char *newrefname)
-{
- int attempts_remaining = 4;
-
- retry:
- switch (safe_create_leading_directories(git_path(logs/%s,
newrefname))) {
- case SCLD_OK:
- break; /* success */
- case SCLD_VANISHED:
- if (--attempts_remaining 0)
- goto retry;
- /* fall through */
- default:
- error(unable to create directory for %s, newrefname);
- return -1;
- }
-
- if (rename(git_path(TMP_RENAMED_LOG), git_path(logs/%s, newrefname)))
{
- if ((errno==EISDIR || errno==ENOTDIR) --attempts_remaining
0) {
- /*
-* rename(a, b) when b is an existing
-* directory ought to result in ISDIR, but
-* Solaris 5.8 gives ENOTDIR. Sheesh.
-*/
- if (remove_empty_directories(git_path(logs/%s,
newrefname))) {
- error(Directory not empty: logs/%s,
newrefname);
- return -1;
- }
- goto retry;
- } else if (errno == ENOENT --attempts_remaining 0) {
- /*
-* Maybe another process just deleted one of
-* the directories in the path to newrefname.
-* Try again from the beginning.
-*/
- goto retry;
- } else {
- error(unable to move logfile TMP_RENAMED_LOG to
logs/%s: %s,
- newrefname, strerror(errno));
- return -1;
- }
- }
- return 0;
-}
-
static int rename_ref_available(const char *oldname, const char *newname)
{
struct string_list skip = STRING_LIST_INIT_NODUP;
@@ -2859,91 +2805,63 @@ static int copy_reflog_into_strbuf(const char *refname,
struct strbuf *buf)
int rename_ref(const char *oldrefname, const char *newrefname, const char
*logmsg)
{
- unsigned char sha1[20], orig_sha1[20];
- int flag = 0, logmoved = 0;
- struct ref_lock *lock;
- struct stat loginfo;
- int log = !lstat(git_path(logs/%s, oldrefname), loginfo);
+ unsigned char sha1[20];
+ int flag = 0;
+ int log;
+ struct transaction *transaction = NULL;
+ struct strbuf err = STRBUF_INIT;
const char *symref = NULL;
+ struct reflog_committer_info ci;
- if (log S_ISLNK(loginfo.st_mode))
- return error(reflog for %s is a symlink, oldrefname);
+ memset(ci, 0, sizeof(ci));
+ ci.committer_info = git_committer_info(0);
symref = resolve_ref_unsafe(oldrefname, RESOLVE_REF_READING,
- orig_sha1, flag);
- if (flag REF_ISSYMREF)
- return error(refname %s is a symbolic ref, renaming it is not
supported,
- oldrefname);
- if (!symref)
- return error(refname %s not found, oldrefname);
-
- if (!rename_ref_available(oldrefname, newrefname))
+ sha1, flag);
+ if (flag REF_ISSYMREF) {
+ error(refname %s is a symbolic ref, renaming it is not
+ supported, oldrefname);
return 1;
-
- if (log rename(git_path(logs/%s, oldrefname),
git_path(TMP_RENAMED_LOG)))
- return error(unable to move logfile logs/%s to
TMP_RENAMED_LOG: %s,
- oldrefname, strerror(errno));
-
- if (delete_ref(oldrefname, orig_sha1, REF_NODEREF)) {
- error(unable to delete old %s, oldrefname);
- goto rollback;
}
-
- if (!read_ref_full(newrefname, RESOLVE_REF_READING, sha1, NULL)
- delete_ref(newrefname, sha1, REF_NODEREF)) {
-
[PATCH v4 0/7] ref-transaction-send-pack
Hi, This series has been posted before[1], but is now rebased on the previous ref-transaction-rename. It can also be found at github[2] and googlesource[3] This series finishes the transaction work to provide atomic pushes. With this series we can now perform atomic pushes to a repository. Version 2: - Reordered the capabilities we send so that agent= remains the last capability listed. - Reworded the paragraph for atomic push in git-send-pack.txt - Dropped the patch for receive.preferatomicpush Version 3: - Fix a typo in a commit message. Version 4: * As Ronnie announced to change employers soon, he'll have only limited time to work on git in the near future. As this is a rather large patch series, he is handing this work over to me. That's why I'm sending the patches this time. [1] http://www.spinics.net/lists/git/msg241365.html [2] https://github.com/stefanbeller/git/tree/ref-transactions-send-pack [3] https://code-review.googlesource.com/#/q/topic:ref-transaction-sendpack Thanks, Stefan Ronnie Sahlberg (7): receive-pack.c: add protocol support to negotiate atomic-push send-pack.c: add an --atomic-push command line argument receive-pack.c: use a single transaction when atomic-push is negotiated push.c: add an --atomic-push argument t5543-atomic-push.sh: add basic tests for atomic pushes refs.c: add an err argument to create_reflog refs.c: add an err argument to create_symref Documentation/git-push.txt| 7 +- Documentation/git-send-pack.txt | 7 +- Documentation/technical/protocol-capabilities.txt | 12 ++- builtin/branch.c | 7 +- builtin/checkout.c| 21 +++-- builtin/clone.c | 15 +++- builtin/init-db.c | 8 +- builtin/notes.c | 7 +- builtin/push.c| 2 + builtin/receive-pack.c| 79 + builtin/remote.c | 26 -- builtin/send-pack.c | 6 +- builtin/symbolic-ref.c| 6 +- cache.h | 1 - refs.c| 93 ++-- refs.h| 5 +- remote.h | 3 +- send-pack.c | 45 -- send-pack.h | 1 + t/t5543-atomic-push.sh| 101 ++ transport.c | 5 ++ transport.h | 1 + 22 files changed, 358 insertions(+), 100 deletions(-) create mode 100755 t/t5543-atomic-push.sh -- 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 v4 7/7] refs.c: add an err argument to create_symref
From: Ronnie Sahlberg sahlb...@google.com
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
Signed-off-by: Stefan Beller sbel...@google.com
---
builtin/branch.c | 7 +--
builtin/checkout.c | 13 ++---
builtin/clone.c| 15 +++
builtin/init-db.c | 8 ++--
builtin/notes.c| 7 ---
builtin/remote.c | 26 ++
builtin/symbolic-ref.c | 6 +-
cache.h| 1 -
refs.c | 30 ++
refs.h | 1 +
10 files changed, 78 insertions(+), 36 deletions(-)
diff --git a/builtin/branch.c b/builtin/branch.c
index 04f57d4..ab6d9f4 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -698,6 +698,7 @@ static void rename_branch(const char *oldname, const char
*newname, int force)
{
struct strbuf oldref = STRBUF_INIT, newref = STRBUF_INIT, logmsg =
STRBUF_INIT;
struct strbuf oldsection = STRBUF_INIT, newsection = STRBUF_INIT;
+ struct strbuf err = STRBUF_INIT;
int recovery = 0;
int clobber_head_ok;
@@ -734,8 +735,10 @@ static void rename_branch(const char *oldname, const char
*newname, int force)
warning(_(Renamed a misnamed branch '%s' away), oldref.buf +
11);
/* no need to pass logmsg here as HEAD didn't really move */
- if (!strcmp(oldname, head) create_symref(HEAD, newref.buf, NULL))
- die(_(Branch renamed to %s, but HEAD is not updated!),
newname);
+ if (!strcmp(oldname, head)
+ create_symref(HEAD, newref.buf, NULL, err))
+ die(_(Branch renamed to %s, but HEAD is not updated!. %s),
+ newname, err.buf);
strbuf_addf(oldsection, branch.%s, oldref.buf + 11);
strbuf_release(oldref);
diff --git a/builtin/checkout.c b/builtin/checkout.c
index d9cb9c3..1efe353 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -634,7 +634,10 @@ static void update_refs_for_switch(const struct
checkout_opts *opts,
describe_detached_head(_(HEAD is now at),
new-commit);
}
} else if (new-path) { /* Switch branches. */
- create_symref(HEAD, new-path, msg.buf);
+ if (create_symref(HEAD, new-path, msg.buf, err)) {
+ error(%s, err.buf);
+ strbuf_release(err);
+ }
if (!opts-quiet) {
if (old-path !strcmp(new-path, old-path)) {
if (opts-new_branch_force)
@@ -1020,12 +1023,16 @@ static int parse_branchname_arg(int argc, const char
**argv,
static int switch_unborn_to_new_branch(const struct checkout_opts *opts)
{
int status;
- struct strbuf branch_ref = STRBUF_INIT;
+ struct strbuf branch_ref = STRBUF_INIT, err = STRBUF_INIT;
if (!opts-new_branch)
die(_(You are on a branch yet to be born));
strbuf_addf(branch_ref, refs/heads/%s, opts-new_branch);
- status = create_symref(HEAD, branch_ref.buf, checkout -b);
+ status = create_symref(HEAD, branch_ref.buf, checkout -b, err);
+ if (status) {
+ error(%s, err.buf);
+ strbuf_release(err);
+ }
strbuf_release(branch_ref);
if (!opts-quiet)
fprintf(stderr, _(Switched to a new branch '%s'\n),
diff --git a/builtin/clone.c b/builtin/clone.c
index 49a72ac..465818a 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -562,6 +562,7 @@ static void update_remote_refs(const struct ref *refs,
int check_connectivity)
{
const struct ref *rm = mapped_refs;
+ struct strbuf err = STRBUF_INIT;
if (check_connectivity) {
if (transport-progress)
@@ -583,9 +584,12 @@ static void update_remote_refs(const struct ref *refs,
struct strbuf head_ref = STRBUF_INIT;
strbuf_addstr(head_ref, branch_top);
strbuf_addstr(head_ref, HEAD);
- create_symref(head_ref.buf,
- remote_head_points_at-peer_ref-name,
- msg);
+ if (create_symref(head_ref.buf,
+ remote_head_points_at-peer_ref-name,
+ msg, err)) {
+ error(%s, err.buf);
+ strbuf_release(err);
+ }
}
}
@@ -596,7 +600,10 @@ static void update_head(const struct ref *our, const
struct ref *remote,
const char *head;
if (our skip_prefix(our-name, refs/heads/, head)) {
/* Local default branch link */
- create_symref(HEAD, our-name, NULL);
+ if (create_symref(HEAD, our-name, NULL, err)) {
+ error(%s, err.buf);
+ strbuf_release(err);
+ }
if (!option_bare) {
[PATCH v4 3/7] receive-pack.c: use a single transaction when atomic-push is negotiated
From: Ronnie Sahlberg sahlb...@google.com
Update receive-pack to use an atomic transaction iff the client negotiated
that it wanted atomic-push.
This leaves the default behavior to be the old non-atomic one ref at a
time update. This is to cause as little disruption as possible to existing
clients. It is unknown if there are client scripts that depend on the old
non-atomic behavior so we make it opt-in for now.
Later patch in this series also adds a configuration variable where you can
override the atomic push behavior on the receiving repo and force it
to use atomic updates always.
If it turns out over time that there are no client scripts that depend on the
old behavior we can change git to default to use atomic pushes and instead
offer an opt-out argument for people that do not want atomic pushes.
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
Signed-off-by: Stefan Beller sbel...@google.com
---
builtin/receive-pack.c | 73 +++---
1 file changed, 58 insertions(+), 15 deletions(-)
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index 63acebf..aa43a5e 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -67,6 +67,8 @@ static const char *NONCE_SLOP = SLOP;
static const char *nonce_status;
static long nonce_stamp_slop;
static unsigned long nonce_stamp_slop_limit;
+struct strbuf err = STRBUF_INIT;
+struct transaction *transaction;
static enum deny_action parse_deny_action(const char *var, const char *value)
{
@@ -832,33 +834,55 @@ static const char *update(struct command *cmd, struct
shallow_info *si)
cmd-did_not_exist = 1;
}
}
- if (delete_ref(namespaced_name, old_sha1, 0)) {
- rp_error(failed to delete %s, name);
- return failed to delete;
+ if (!use_atomic_push) {
+ if (delete_ref(namespaced_name, old_sha1, 0)) {
+ rp_error(failed to delete %s, name);
+ return failed to delete;
+ }
+ } else {
+ if (transaction_delete_ref(transaction,
+ namespaced_name,
+ old_sha1,
+ 0, old_sha1 != NULL,
+ push, err)) {
+ rp_error(%s, err.buf);
+ strbuf_release(err);
+ return failed to delete;
+ }
}
return NULL; /* good */
}
else {
- struct strbuf err = STRBUF_INIT;
- struct transaction *transaction;
-
if (shallow_update si-shallow_ref[cmd-index]
update_shallow_ref(cmd, si))
return shallow error;
- transaction = transaction_begin(err);
- if (!transaction ||
- transaction_update_ref(transaction, namespaced_name,
- new_sha1, old_sha1, 0, 1, push,
- err) ||
- transaction_commit(transaction, err)) {
- transaction_free(transaction);
+ if (!use_atomic_push) {
+ transaction = transaction_begin(err);
+ if (!transaction) {
+ rp_error(%s, err.buf);
+ strbuf_release(err);
+ return failed to start transaction;
+ }
+ }
+ if (transaction_update_ref(transaction,
+ namespaced_name,
+ new_sha1, old_sha1,
+ 0, 1, push,
+ err)) {
rp_error(%s, err.buf);
strbuf_release(err);
return failed to update ref;
}
-
- transaction_free(transaction);
+ if (!use_atomic_push) {
+ if (transaction_commit(transaction, err)) {
+ transaction_free(transaction);
+ rp_error(%s, err.buf);
+ strbuf_release(err);
+ return failed to update ref;
+ }
+ transaction_free(transaction);
+ }
strbuf_release(err);
return NULL; /* good */
}
@@ -1058,6 +1082,16 @@ static void execute_commands(struct command *commands,
return;
}
+ if (use_atomic_push) {
+
[PATCH v4 4/7] push.c: add an --atomic-push argument
From: Ronnie Sahlberg sahlb...@google.com
Add a command line argument to the git push command to request atomic
pushes.
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
Signed-off-by: Stefan Beller sbel...@google.com
---
Documentation/git-push.txt | 7 ++-
builtin/push.c | 2 ++
transport.c| 1 +
transport.h| 1 +
4 files changed, 10 insertions(+), 1 deletion(-)
diff --git a/Documentation/git-push.txt b/Documentation/git-push.txt
index 21b3f29..04de8d8 100644
--- a/Documentation/git-push.txt
+++ b/Documentation/git-push.txt
@@ -9,7 +9,7 @@ git-push - Update remote refs along with associated objects
SYNOPSIS
[verse]
-'git push' [--all | --mirror | --tags] [--follow-tags] [-n | --dry-run]
[--receive-pack=git-receive-pack]
+'git push' [--all | --mirror | --tags] [--follow-tags] [--atomic-push] [-n |
--dry-run] [--receive-pack=git-receive-pack]
[--repo=repository] [-f | --force] [--prune] [-v | --verbose]
[-u | --set-upstream] [--signed]
[--force-with-lease[=refname[:expect]]]
@@ -136,6 +136,11 @@ already exists on the remote side.
logged. See linkgit:git-receive-pack[1] for the details
on the receiving end.
+--atomic-push::
+ Try using atomic push. If atomic push is negotiated with the server
+ then any push covering multiple refs will be atomic. Either all
+ refs are updated, or on error, no refs are updated.
+
--receive-pack=git-receive-pack::
--exec=git-receive-pack::
Path to the 'git-receive-pack' program on the remote
diff --git a/builtin/push.c b/builtin/push.c
index a076b19..ae393c5 100644
--- a/builtin/push.c
+++ b/builtin/push.c
@@ -518,6 +518,8 @@ int cmd_push(int argc, const char **argv, const char
*prefix)
OPT_BIT(0, follow-tags, flags, N_(push missing but relevant
tags),
TRANSPORT_PUSH_FOLLOW_TAGS),
OPT_BIT(0, signed, flags, N_(GPG sign the push),
TRANSPORT_PUSH_CERT),
+ OPT_BIT(0, atomic-push, flags, N_(use atomic push, if
available),
+ TRANSPORT_ATOMIC_PUSH),
OPT_END()
};
diff --git a/transport.c b/transport.c
index 2111986..cd2b63a 100644
--- a/transport.c
+++ b/transport.c
@@ -833,6 +833,7 @@ static int git_transport_push(struct transport *transport,
struct ref *remote_re
args.dry_run = !!(flags TRANSPORT_PUSH_DRY_RUN);
args.porcelain = !!(flags TRANSPORT_PUSH_PORCELAIN);
args.push_cert = !!(flags TRANSPORT_PUSH_CERT);
+ args.use_atomic_push = !!(flags TRANSPORT_ATOMIC_PUSH);
args.url = transport-url;
ret = send_pack(args, data-fd, data-conn, remote_refs,
diff --git a/transport.h b/transport.h
index 3e0091e..25fa1da 100644
--- a/transport.h
+++ b/transport.h
@@ -125,6 +125,7 @@ struct transport {
#define TRANSPORT_PUSH_NO_HOOK 512
#define TRANSPORT_PUSH_FOLLOW_TAGS 1024
#define TRANSPORT_PUSH_CERT 2048
+#define TRANSPORT_ATOMIC_PUSH 4096
#define TRANSPORT_SUMMARY_WIDTH (2 * DEFAULT_ABBREV + 3)
#define TRANSPORT_SUMMARY(x) (int)(TRANSPORT_SUMMARY_WIDTH + strlen(x) -
gettext_width(x)), (x)
--
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 v4 1/7] receive-pack.c: add protocol support to negotiate atomic-push
From: Ronnie Sahlberg sahlb...@google.com
This adds support to the protocol between send-pack and receive-pack to
* allow receive-pack to inform the client that it has atomic push capability
* allow send-pack to request atomic push back.
There is currently no setting in send-pack to actually request that atomic
pushes are to be used yet. This only adds protocol capability not ability
for the user to activate it.
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
Signed-off-by: Stefan Beller sbel...@google.com
---
Documentation/technical/protocol-capabilities.txt | 12 ++--
builtin/receive-pack.c| 6 +-
send-pack.c | 6 ++
3 files changed, 21 insertions(+), 3 deletions(-)
diff --git a/Documentation/technical/protocol-capabilities.txt
b/Documentation/technical/protocol-capabilities.txt
index 6d5424c..763120c 100644
--- a/Documentation/technical/protocol-capabilities.txt
+++ b/Documentation/technical/protocol-capabilities.txt
@@ -18,8 +18,9 @@ was sent. Server MUST NOT ignore capabilities that client
requested
and server advertised. As a consequence of these rules, server MUST
NOT advertise capabilities it does not understand.
-The 'report-status', 'delete-refs', 'quiet', and 'push-cert' capabilities
-are sent and recognized by the receive-pack (push to server) process.
+The 'atomic-push', 'report-status', 'delete-refs', 'quiet', and 'push-cert'
+capabilities are sent and recognized by the receive-pack (push to server)
+process.
The 'ofs-delta' and 'side-band-64k' capabilities are sent and recognized
by both upload-pack and receive-pack protocols. The 'agent' capability
@@ -244,6 +245,13 @@ respond with the 'quiet' capability to suppress
server-side progress
reporting if the local progress reporting is also being suppressed
(e.g., via `push -q`, or if stderr does not go to a tty).
+atomic-push
+---
+
+If the server sends the 'atomic-push' capability, it means it is
+capable of accepting atomic pushes. If the pushing client requests this
+capability, the server will update the refs in one single atomic transaction.
+
allow-tip-sha1-in-want
--
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index 397abc9..63acebf 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -40,6 +40,7 @@ static int transfer_unpack_limit = -1;
static int unpack_limit = 100;
static int report_status;
static int use_sideband;
+static int use_atomic_push;
static int quiet;
static int prefer_ofs_delta = 1;
static int auto_update_server_info;
@@ -171,7 +172,8 @@ static void show_ref(const char *path, const unsigned char
*sha1)
struct strbuf cap = STRBUF_INIT;
strbuf_addstr(cap,
- report-status delete-refs side-band-64k quiet);
+ report-status delete-refs side-band-64k quiet
+ atomic-push);
if (prefer_ofs_delta)
strbuf_addstr(cap, ofs-delta);
if (push_cert_nonce)
@@ -1178,6 +1180,8 @@ static struct command *read_head_info(struct sha1_array
*shallow)
use_sideband = LARGE_PACKET_MAX;
if (parse_feature_request(feature_list, quiet))
quiet = 1;
+ if (parse_feature_request(feature_list, atomic-push))
+ use_atomic_push = 1;
}
if (!strcmp(line, push-cert)) {
diff --git a/send-pack.c b/send-pack.c
index 949cb61..1ccc84c 100644
--- a/send-pack.c
+++ b/send-pack.c
@@ -294,6 +294,8 @@ int send_pack(struct send_pack_args *args,
int use_sideband = 0;
int quiet_supported = 0;
int agent_supported = 0;
+ int atomic_push_supported = 0;
+ int atomic_push = 0;
unsigned cmds_sent = 0;
int ret;
struct async demux;
@@ -314,6 +316,8 @@ int send_pack(struct send_pack_args *args,
agent_supported = 1;
if (server_supports(no-thin))
args-use_thin_pack = 0;
+ if (server_supports(atomic-push))
+ atomic_push_supported = 1;
if (args-push_cert) {
int len;
@@ -335,6 +339,8 @@ int send_pack(struct send_pack_args *args,
strbuf_addstr(cap_buf, side-band-64k);
if (quiet_supported (args-quiet || !args-progress))
strbuf_addstr(cap_buf, quiet);
+ if (atomic_push)
+ strbuf_addstr(cap_buf, atomic-push);
if (agent_supported)
strbuf_addf(cap_buf, agent=%s, git_user_agent_sanitized());
--
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 v4 6/7] refs.c: add an err argument to create_reflog
From: Ronnie Sahlberg sahlb...@google.com
Add an err argument to create_reflog that can explain the reason for a
failure. This then eliminates the need to manage errno through this
function since we can just add strerror(errno) to the err string when
meaningful. No callers relied on errno from this function for anything
else than the error message.
log_ref_write is a private function that calls create_reflog. Update
this function to also take an err argument and pass it back to the caller.
This again eliminates the need to manage errno in this function.
Update the private function write_sha1_update_reflog to also take an
err argument.
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
Signed-off-by: Stefan Beller sbel...@google.com
---
builtin/checkout.c | 8 +++---
refs.c | 71 +++---
refs.h | 4 +--
3 files changed, 42 insertions(+), 41 deletions(-)
diff --git a/builtin/checkout.c b/builtin/checkout.c
index 60a68f7..d9cb9c3 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -590,10 +590,12 @@ static void update_refs_for_switch(const struct
checkout_opts *opts,
if (opts-new_orphan_branch) {
if (opts-new_branch_log !log_all_ref_updates) {
char *ref_name = mkpath(refs/heads/%s,
opts-new_orphan_branch);
+ struct strbuf err = STRBUF_INIT;
- if (create_reflog(ref_name)) {
- fprintf(stderr, _(Can not do reflog
for '%s'\n),
- opts-new_orphan_branch);
+ if (create_reflog(ref_name, err)) {
+ fprintf(stderr, _(Can not do reflog
for '%s'. %s\n),
+ opts-new_orphan_branch, err.buf);
+ strbuf_release(err);
return;
}
}
diff --git a/refs.c b/refs.c
index ddb5fc6..d2f7cea 100644
--- a/refs.c
+++ b/refs.c
@@ -2895,8 +2895,7 @@ static int copy_msg(char *buf, const char *msg)
return cp - buf;
}
-/* This function must set a meaningful errno on failure */
-int create_reflog(const char *refname)
+int create_reflog(const char *refname, struct strbuf *err)
{
int logfd, oflags = O_APPEND | O_WRONLY;
char logfile[PATH_MAX];
@@ -2907,9 +2906,8 @@ int create_reflog(const char *refname)
starts_with(refname, refs/notes/) ||
!strcmp(refname, HEAD)) {
if (safe_create_leading_directories(logfile) 0) {
- int save_errno = errno;
- error(unable to create directory for %s, logfile);
- errno = save_errno;
+ strbuf_addf(err, unable to create directory for %s.
+ %s, logfile, strerror(errno));
return -1;
}
oflags |= O_CREAT;
@@ -2922,20 +2920,16 @@ int create_reflog(const char *refname)
if (errno == EISDIR) {
if (remove_empty_directories(logfile)) {
- int save_errno = errno;
- error(There are still logs under '%s',
- logfile);
- errno = save_errno;
+ strbuf_addf(err, There are still logs under
+ '%s', logfile);
return -1;
}
logfd = open(logfile, oflags, 0666);
}
if (logfd 0) {
- int save_errno = errno;
- error(Unable to append to %s: %s, logfile,
- strerror(errno));
- errno = save_errno;
+ strbuf_addf(err, Unable to append to %s: %s,
+ logfile, strerror(errno));
return -1;
}
}
@@ -2972,7 +2966,8 @@ static int log_ref_write_fd(int fd, const unsigned char
*old_sha1,
}
static int log_ref_write(const char *refname, const unsigned char *old_sha1,
-const unsigned char *new_sha1, const char *msg)
+const unsigned char *new_sha1, const char *msg,
+struct strbuf *err)
{
int logfd, result = 0, oflags = O_APPEND | O_WRONLY;
char log_file[PATH_MAX];
@@ -2981,7 +2976,7 @@ static int log_ref_write(const char *refname, const
unsigned char *old_sha1,
log_all_ref_updates = !is_bare_repository();
if (log_all_ref_updates !reflog_exists(refname))
- result = create_reflog(refname);
+ result =
[PATCH v4 2/7] send-pack.c: add an --atomic-push command line argument
From: Ronnie Sahlberg sahlb...@google.com
This adds support to send-pack to negotiate and use atomic pushes
iff the server supports it. Atomic pushes are activated by a new command
line flag --atomic-push.
In order to do this we also need to change the semantics for send_pack()
slightly. The existing send_pack() function actually doesn't send all the
refs back to the server when multiple refs are involved, for example
when using --all. Several of the failure modes for pushes can already be
detected locally in the send_pack client based on the information from the
initial server side list of all the refs as generated by receive-pack.
Any such refs that we thus know would fail to push are thus pruned from
the list of refs we send to the server to update.
For atomic pushes, we have to deal thus with both failures that are detected
locally as well as failures that are reported back from the server. In order
to do so we treat all local failures as push failures too.
We introduce a new status code REF_STATUS_ATOMIC_PUSH_FAILED so we can
flag all refs that we would normally have tried to push to the server
but we did not due to local failures. This is to improve the error message
back to the end user to flag that these refs failed to update since the
atomic push operation failed.
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
Signed-off-by: Stefan Beller sbel...@google.com
---
Documentation/git-send-pack.txt | 7 ++-
builtin/send-pack.c | 6 +-
remote.h| 3 ++-
send-pack.c | 39 ++-
send-pack.h | 1 +
transport.c | 4
6 files changed, 52 insertions(+), 8 deletions(-)
diff --git a/Documentation/git-send-pack.txt b/Documentation/git-send-pack.txt
index 2a0de42..9296587 100644
--- a/Documentation/git-send-pack.txt
+++ b/Documentation/git-send-pack.txt
@@ -9,7 +9,7 @@ git-send-pack - Push objects over Git protocol to another
repository
SYNOPSIS
[verse]
-'git send-pack' [--all] [--dry-run] [--force]
[--receive-pack=git-receive-pack] [--verbose] [--thin] [host:]directory
[ref...]
+'git send-pack' [--all] [--dry-run] [--force]
[--receive-pack=git-receive-pack] [--verbose] [--thin] [--atomic-push]
[host:]directory [ref...]
DESCRIPTION
---
@@ -62,6 +62,11 @@ be in a separate packet, and the list must end with a flush
packet.
Send a thin pack, which records objects in deltified form based
on objects not included in the pack to reduce network traffic.
+--atomic-push::
+ Use an atomic transaction for updating the refs. If any of the refs
+ fails to update then the entire push will fail without changing any
+ refs.
+
host::
A remote host to house the repository. When this
part is specified, 'git-receive-pack' is invoked via
diff --git a/builtin/send-pack.c b/builtin/send-pack.c
index b564a77..93cb17c 100644
--- a/builtin/send-pack.c
+++ b/builtin/send-pack.c
@@ -13,7 +13,7 @@
#include sha1-array.h
static const char send_pack_usage[] =
-git send-pack [--all | --mirror] [--dry-run] [--force]
[--receive-pack=git-receive-pack] [--verbose] [--thin] [host:]directory
[ref...]\n
+git send-pack [--all | --mirror] [--dry-run] [--force]
[--receive-pack=git-receive-pack] [--verbose] [--thin] [--atomic-push]
[host:]directory [ref...]\n
--all and explicit ref specification are mutually exclusive.;
static struct send_pack_args args;
@@ -170,6 +170,10 @@ int cmd_send_pack(int argc, const char **argv, const char
*prefix)
args.use_thin_pack = 1;
continue;
}
+ if (!strcmp(arg, --atomic-push)) {
+ args.use_atomic_push = 1;
+ continue;
+ }
if (!strcmp(arg, --stateless-rpc)) {
args.stateless_rpc = 1;
continue;
diff --git a/remote.h b/remote.h
index 8b62efd..f346524 100644
--- a/remote.h
+++ b/remote.h
@@ -115,7 +115,8 @@ struct ref {
REF_STATUS_REJECT_SHALLOW,
REF_STATUS_UPTODATE,
REF_STATUS_REMOTE_REJECT,
- REF_STATUS_EXPECTING_REPORT
+ REF_STATUS_EXPECTING_REPORT,
+ REF_STATUS_ATOMIC_PUSH_FAILED
} status;
char *remote_status;
struct ref *peer_ref; /* when renaming */
diff --git a/send-pack.c b/send-pack.c
index 1ccc84c..08602a8 100644
--- a/send-pack.c
+++ b/send-pack.c
@@ -190,7 +190,7 @@ static void advertise_shallow_grafts_buf(struct strbuf *sb)
for_each_commit_graft(advertise_shallow_grafts_cb, sb);
}
-static int ref_update_to_be_sent(const struct ref *ref, const struct
send_pack_args *args)
+static int ref_update_to_be_sent(const struct ref *ref, const struct
send_pack_args *args, int
[PATCH v4 5/7] t5543-atomic-push.sh: add basic tests for atomic pushes
From: Ronnie Sahlberg sahlb...@google.com
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
Signed-off-by: Stefan Beller sbel...@google.com
---
t/t5543-atomic-push.sh | 101 +
1 file changed, 101 insertions(+)
create mode 100755 t/t5543-atomic-push.sh
diff --git a/t/t5543-atomic-push.sh b/t/t5543-atomic-push.sh
new file mode 100755
index 000..4903227
--- /dev/null
+++ b/t/t5543-atomic-push.sh
@@ -0,0 +1,101 @@
+#!/bin/sh
+
+test_description='pushing to a mirror repository'
+
+. ./test-lib.sh
+
+D=`pwd`
+
+invert () {
+ if $@; then
+ return 1
+ else
+ return 0
+ fi
+}
+
+mk_repo_pair () {
+ rm -rf master mirror
+ mkdir mirror
+ (
+ cd mirror
+ git init
+ git config receive.denyCurrentBranch warn
+ )
+ mkdir master
+ (
+ cd master
+ git init
+ git remote add $1 up ../mirror
+ )
+}
+
+
+test_expect_success 'atomic push works for a single branch' '
+
+ mk_repo_pair
+ (
+ cd master
+ echo one foo git add foo git commit -m one
+ git push --mirror up
+ echo two foo git add foo git commit -m two
+ git push --atomic-push --mirror up
+ )
+ master_master=$(cd master git show-ref -s --verify
refs/heads/master)
+ mirror_master=$(cd mirror git show-ref -s --verify
refs/heads/master)
+ test $master_master = $mirror_master
+
+'
+
+test_expect_success 'atomic push works for two branches' '
+
+ mk_repo_pair
+ (
+ cd master
+ echo one foo git add foo git commit -m one
+ git branch second
+ git push --mirror up
+ echo two foo git add foo git commit -m two
+ git checkout second
+ echo three foo git add foo git commit -m three
+ git checkout master
+ git push --atomic-push --mirror up
+ )
+ master_master=$(cd master git show-ref -s --verify
refs/heads/master)
+ mirror_master=$(cd mirror git show-ref -s --verify
refs/heads/master)
+ test $master_master = $mirror_master
+
+ master_second=$(cd master git show-ref -s --verify
refs/heads/second)
+ mirror_second=$(cd mirror git show-ref -s --verify
refs/heads/second)
+ test $master_second = $mirror_second
+'
+
+# set up two branches where master can be pushed but second can not
+# (non-fast-forward). Since second can not be pushed the whole operation
+# will fail and leave master untouched.
+test_expect_success 'atomic push fails if one branch fails' '
+ mk_repo_pair
+ (
+ cd master
+ echo one foo git add foo git commit -m one
+ git branch second
+ git checkout second
+ echo two foo git add foo git commit -m two
+ echo three foo git add foo git commit -m three
+ echo four foo git add foo git commit -m four
+ git push --mirror up
+ git reset --hard HEAD~2
+ git checkout master
+ echo five foo git add foo git commit -m five
+ ! git push --atomic-push --all up
+ )
+ master_master=$(cd master git show-ref -s --verify
refs/heads/master)
+ mirror_master=$(cd mirror git show-ref -s --verify
refs/heads/master)
+ test $master_master != $mirror_master
+
+ master_second=$(cd master git show-ref -s --verify
refs/heads/second)
+ mirror_second=$(cd mirror git show-ref -s --verify
refs/heads/second)
+ test $master_second != $mirror_second
+'
+
+test_done
--
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: Getting a commit sha1 from fast-import in a remote-helper
On Tue, Nov 18, 2014 at 09:34:26AM +0900, Mike Hommey wrote: Hi, For some reason, I need to know the sha1 corresponding to some marks I'm putting in a fast-import stream. Unfortunately, this does not appear to be possible. - I'd rather not require a checkpoint to export marks each time I need such a sha1, and I'd rather not do that work that requires them after all the marks have been created (although currently, I'm forced to). BTW, if it so happens that all the operations that were done end up creating objects that already existed for some reason, checkpoint doesn't do anything, which is fine for the pack and tags, but not necessarily so for export-marks. 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: [PATCH] Introduce a hook to run after formatting patches
Junio, thanks for pointing out, why my patch doesn't make sense here. 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 way you're proposing, doesn't sound as if a hook would be the right thing for such filtering. 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. A command line option for such filtering is inconvenient because of having it type all the time and not just having it setup and forgetting about it. Sure we can have a command line option to specify such a filter, but I'd be more interested in having an option in maybe the git config file like [format-patch] external-commit-message-stream-processor = $(GIT_DIR)/../foo/bar.sh The command line option would rather only be used to override that config in non-default cases. Thanks for your thoughts, Stefan On Mon, Nov 17, 2014 at 11:06 AM, Junio C Hamano gits...@pobox.com wrote: Junio C Hamano gits...@pobox.com writes: Stefan Beller sbel...@google.com writes: +post-format-patch + + +This hook is called after format-patch created a patch and it is +invoked with the filename of the patch as the first parameter. Such an interface would not work well with --stdout mode, would it? And if this only works with output generated into the files, then $ git format-patch $range | xargs -n1 $your_post_processing_script would do the same without any change to Git, I would imagine. So I would have to say that I am fairly negative on this change in the presented form. An alternative design to implement this as a post-processing filter to work for both to individual files and to standard output stream output filter may be possible, but even in that case I am not sure if it is worth the churn. In general I'd look at post-anything hook that works locally with a great suspicion, so that may partly be where my comment above is coming from. I dunno. Another reason, in addition to that this only works on the already created output files, why I find this particular design distasteful (I am not saying that there should be an easy way to drop cruft left by third-party systems such as Change-id: line) is because the mechanism the patch adds does not attempt to take advantage of being inside Git, so the xargs -n1 above is strictly an equivalent. You have a chance to make the life better for users, but not you are not doing so. The design of this feature could be made to allow the user to specify a filter to munge _ONLY_ the log message part. For example, just after logmsg_reencode() returns the proposed commit log message to msg in pretty.c::pretty_print_commit(), you can detect a request to use some external filter program and let the program munge the message. With such a design: * The external filter your users would write does not have to worry about limiting its damage only to the log message part, as it will never see the patch text part; and * The same mechanism would work just as well for --stdout mode. The former is what I mean by to take advantage of being inside. Incidentally, it falls into #2 of 5 valid reasons to admit a new hook [*1*]. [Reference] *1* http://thread.gmane.org/gmane.comp.version-control.git/232809/focus=71069 -- 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 Mon, Nov 17, 2014 at 05:40:28PM -0800, Jonathan Nieder wrote:
Mike Hommey wrote:
- fast-import's `ls` command documentation about its output format
mentions that the output may contain commits, so I tried the trick of
creating a tree with commits, but fast-import then fails with:
fatal: Not a blob (actually a commit)
which I totally understand, but then I wonder why the documentation
mentions it and how one would get a tree containing references to
commits. I guess the documentation should be fixed.
Odd. Here's what happens when I try:
$ echo ls $(git rev-parse HEAD) | git fast-import --quiet
fatal: Missing space after tree-ish: ls
a4a226a366ab0a173ed9e5f70f2a95d0d21e54c5
fast-import: dumping crash report to .git/fast_import_crash_14080
$ echo ls $(git rev-parse HEAD) | git fast-import --quiet
04 tree d3d38e7d71cb40ebbaf2798b01837b3de43fd4a1
How did you get that Not a blob message?
When trying to *create* a tree with a commit in it, so instead of giving
the mark for a blob to a filemodify command, giving a mark for a commit.
That is what fails with Not a blob.
So it's not possible to create a tree with a reference to a commit, at
least with fast-import.
But, the documentation for the ls command says this:
Output uses the same format as git ls-tree tree -- path:
mode SP ('blob' | 'tree' | 'commit') SP dataref HT path LF
The 'commit' string certainly seems it cannot be there.
I think a good fix would be to teach parse_ls a mode with no path
parameter. Something like this (untested; needs cleanup and tests):
This would make both your commands output the same thing, right? It
wouldn't help my case :)
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 Tue, Nov 18, 2014 at 11:21:37AM +0900, Mike Hommey wrote: On Tue, Nov 18, 2014 at 09:34:26AM +0900, Mike Hommey wrote: Hi, For some reason, I need to know the sha1 corresponding to some marks I'm putting in a fast-import stream. Unfortunately, this does not appear to be possible. - I'd rather not require a checkpoint to export marks each time I need such a sha1, and I'd rather not do that work that requires them after all the marks have been created (although currently, I'm forced to). BTW, if it so happens that all the operations that were done end up creating objects that already existed for some reason, checkpoint doesn't do anything, which is fine for the pack and tags, but not necessarily so for export-marks. And while I'm here, it's sad that one needs to emit a dummy cat-blob or ls command to wait for a checkpoint to be finished because they are the only commands that trigger something written on the cat-blob fd that can be waited for. 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: On Mon, Nov 17, 2014 at 05:40:28PM -0800, Jonathan Nieder wrote: How did you get that Not a blob message? When trying to *create* a tree with a commit in it, so instead of giving the mark for a blob to a filemodify command, giving a mark for a commit. That is what fails with Not a blob. Ah, I see what you were trying now. It's complaining that the data and mode don't match up. See mode under 'filemodify' in the manual. Something like M 16 :1 mycommit should work fine, though that's a pretty ugly workaround for the inability to do ls :1 [...] I think a good fix would be to teach parse_ls a mode with no path parameter. Something like this (untested; needs cleanup and tests): This would make both your commands output the same thing, right? It wouldn't help my case :) It's easily possible my patch has a typo somewhere, but the expected output format would be commit 6066a7eac4b2bcdb86971783b583e4e408b32e81 That wouldn't help? Puzzled, 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
Mike Hommey wrote:
BTW, if it so happens that all the operations that were done end up
creating objects that already existed for some reason, checkpoint
doesn't do anything, which is fine for the pack and tags, but not
necessarily so for export-marks.
Does something like this help?
Do you have a short script that can demonstrate the failure?
Lazily,
Jonathan
Signed-off-by: Jonathan Nieder jrnie...@gmail.com
diff --git i/fast-import.c w/fast-import.c
index d0bd285..c3d53c8 100644
--- i/fast-import.c
+++ w/fast-import.c
@@ -3088,12 +3088,11 @@ static void parse_ls(const char *p, struct branch *b)
static void checkpoint(void)
{
checkpoint_requested = 0;
- if (object_count) {
+ if (object_count)
cycle_packfile();
- dump_branches();
- dump_tags();
- dump_marks();
- }
+ dump_branches();
+ dump_tags();
+ dump_marks();
}
static void parse_checkpoint(void)
--
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 Mon, Nov 17, 2014 at 06:51:31PM -0800, Jonathan Nieder wrote: Mike Hommey wrote: On Mon, Nov 17, 2014 at 05:40:28PM -0800, Jonathan Nieder wrote: How did you get that Not a blob message? When trying to *create* a tree with a commit in it, so instead of giving the mark for a blob to a filemodify command, giving a mark for a commit. That is what fails with Not a blob. Ah, I see what you were trying now. It's complaining that the data and mode don't match up. See mode under 'filemodify' in the manual. Something like M 16 :1 mycommit should work fine, though that's a pretty ugly workaround for the inability to do ls :1 Actually, for my use, that ugly workaround actually improves things for me, avoiding to use blobs in some of the stuff I want to store :) How did I miss that? Thanks a lot for the enlightenment. [...] I think a good fix would be to teach parse_ls a mode with no path parameter. Something like this (untested; needs cleanup and tests): This would make both your commands output the same thing, right? It wouldn't help my case :) It's easily possible my patch has a typo somewhere, but the expected output format would be commit 6066a7eac4b2bcdb86971783b583e4e408b32e81 That wouldn't help? 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. 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 Mon, Nov 17, 2014 at 06:53:59PM -0800, Jonathan Nieder wrote: Mike Hommey wrote: BTW, if it so happens that all the operations that were done end up creating objects that already existed for some reason, checkpoint doesn't do anything, which is fine for the pack and tags, but not necessarily so for export-marks. Does something like this help? I'm not sure about branches and tags, as they would mostly be noops, but dump_marks definitely needs to happen. I did try to move dump_marks out of the if already, and that did what I wanted. 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:
And while I'm here, it's sad that one needs to emit a dummy cat-blob or
ls command to wait for a checkpoint to be finished
That's a good point. (Though relying on checkpoints to read back
information is an ugly trick, so if we can get other commands to
provide the information you need then that would be better.)
The old-fashioned way is to do progress checkpoint done. Alas, that
writes to the progress fd, which doesn't go to the remote helper's
stdin. How about something like this?
Signed-off-by: Jonathan Nieder jrnie...@gmail.com
diff --git i/Documentation/git-fast-import.txt
w/Documentation/git-fast-import.txt
index 377eeaa..c0dedac 100644
--- i/Documentation/git-fast-import.txt
+++ w/Documentation/git-fast-import.txt
@@ -1055,6 +1055,12 @@ ls::
rather than wasting time on the early part of an import
before the unsupported command is detected.
+notify-on-checkpoint::
+ Cause the backend to write `checkpoint complete` to the file
+ descriptor set with `--cat-blob-fd`, or to `stdout` if
+ `--cat-blob-fd` was unspecified, when finishing a checkpoint
+ requested via a `checkpoint` command or SIGUSR1.
+
notes::
Require that the backend support the 'notemodify' (N)
subcommand to the 'commit' command.
diff --git i/fast-import.c w/fast-import.c
index d0bd285..ccf4768 100644
--- i/fast-import.c
+++ w/fast-import.c
@@ -329,6 +329,7 @@ static const char *import_marks_file;
static int import_marks_file_from_stream;
static int import_marks_file_ignore_missing;
static int relative_marks_paths;
+static int notify_on_checkpoint;
/* Our last blob */
static struct last_object last_blob = { STRBUF_INIT, 0, 0, 0 };
@@ -3094,6 +3095,9 @@ static void checkpoint(void)
dump_tags();
dump_marks();
}
+ if (notify_on_checkpoint)
+ cat_blob_write(checkpoint complete\n,
+ strlen(checkpoint complete\n));
}
static void parse_checkpoint(void)
@@ -3244,6 +3248,8 @@ static int parse_one_feature(const char *feature, int
from_stream)
option_export_marks(arg);
} else if (!strcmp(feature, cat-blob)) {
; /* Don't die - this feature is supported */
+ } else if (!strcmp(feature, notify-on-checkpoint)) {
+ notify_on_checkpoint = 1;
} else if (!strcmp(feature, relative-marks)) {
relative_marks_paths = 1;
} else if (!strcmp(feature, no-relative-marks)) {
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [GIT PULL] l10n updates for 2.2.0
Hi Junio, Please pull again in order to merge Catalan translation. Now l10n for Git 2.2.0 is almost completed. bg.po : 2296 translated messages. ca.po : 2296 translated messages. de.po : 2293 translated messages, 2 untranslated messages. fr.po : 2296 translated messages. it.po : 716 translated messages, 350 untranslated messages. pt_PT.po : 306 translated messages, 687 untranslated messages. sv.po : 2296 translated messages. vi.po : 2296 translated messages. zh_CN.po : 2296 translated messages. The following changes since commit d544b2d495295142cb3418b13b5a106d415d2216: l10n: de.po: translate 62 new messages (2014-11-15 18:22:05 +0100) are available in the git repository at: git://github.com/git-l10n/git-po master for you to fetch changes up to b3e4c47565711f5927d87392bd80ff1d6fb25c57: l10n: Update Catalan translation (2014-11-17 20:22:48 -0700) Alex Henrie (1): l10n: Update Catalan translation po/ca.po | 4557 +- 1 file changed, 2392 insertions(+), 2165 deletions(-) -- 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: Getting a commit sha1 from fast-import in a remote-helper
On Mon, Nov 17, 2014 at 07:27:41PM -0800, Jonathan Nieder wrote: Mike Hommey wrote: And while I'm here, it's sad that one needs to emit a dummy cat-blob or ls command to wait for a checkpoint to be finished That's a good point. (Though relying on checkpoints to read back information is an ugly trick, so if we can get other commands to provide the information you need then that would be better.) The old-fashioned way is to do progress checkpoint done. Alas, that writes to the progress fd, which doesn't go to the remote helper's stdin. How about something like this? How about something more generic, like sync, which purpose would be to request synchronisation with fast-import, which would respond sync on the cat-blob fd? Or ping-pong. Although... I wonder if that would really be useful for anything else than checkpoint... That said, I, for one, would be fine if this is not fixed as long as marks can be resolved some other way (and, in fact, it may turn out that I don't need to resolve marks to sha1s at all) Thanks for you help 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
[PATCH/RFC] Add contrib Clearcase Base import utility
This is a simple perl script that dumps the history of a Base Clearcase VOB and then walks this history retrieving the file contents, version, and branch information. Equivalent git commits are created from the Clearcase history via git-fast-import(1). This does not support Clearcase UCM. Signed-off-by: Jason Woodward woodwa...@jaos.org --- contrib/cc-base-import/cc-base-import | 579 ++ contrib/cc-base-import/cc-base-import.txt | 111 ++ 2 files changed, 690 insertions(+) create mode 100755 contrib/cc-base-import/cc-base-import create mode 100644 contrib/cc-base-import/cc-base-import.txt diff --git a/contrib/cc-base-import/cc-base-import b/contrib/cc-base-import/cc-base-import new file mode 100755 index 000..4a89703 --- /dev/null +++ b/contrib/cc-base-import/cc-base-import @@ -0,0 +1,579 @@ +#!/usr/bin/perl +use strict; +use warnings; +use POSIX qw/mktime/; +use File::Spec qw/splitdir catdir join splitpath curdir rel2abs/; +use IPC::Open3 qw/open3/; +use Getopt::Long qw/GetOptions/; + +=head1 NAME + +cc-base-import - Import a ClearCase VOB into git + +=head1 SYNOPSIS + +cc-base-import [command] [option(s)] + +commands: init --vob path | fetch | commit + +options: [--verbose | --savetemps] + +example: + + $ mkdir /tmp/project + $ cd /tmp/project + $ git cc-base-import init --vob /view/view_main/vobs/vob1 --verbose +Initialized empty Git repository in /tmp/project/.git/ + $ git cc-base-import fetch --verbose +Fetching clearcase events + $ git cc-base-import commit +... [generates git commits from clearcase events] + $ git checkout master + $ ls +... + +=head1 DESCRIPTION + +This utility walks the history of a ClearCase VOB creating equivalent commits +in git. Branch names, commit messages, and author information is preserved. + +The only requirement is the Bcleartool utility must be present in the current +PATH. + +BNOTE this does not work with ClearCase UCM, only base ClearCase. + + +=head2 Error Handling + +Certain data loss messages are currently ignored, such as the following: + + cleartool: Error: Operation file copy failed: No such device or address. + +This implies that for some reason a version of the file in question is not available. +This is ignored in hopes that a later version is available from Clearcase. + +Other errors are not, such as: + + cleartool: Error: Operation file copy failed: Input/output error. + +This error implies some underlying issue such as MVFS communication errors. + + cleartool: Error: Operation get cleartext failed: not a ClearCase object. + +This error indicates a file was deleted during a long running import. + +=head2 Deficiencies + +There is no attempt to handle tracking branch renames in clearcase. + +File deletions are also not handled. Some info can be gleamed from the directory +history but that information is not enforced and can be overridden. This is +also true for file moves. + +A suggested strategy is to run the conversion and enforce no file moves, +deletions, or branch renames during a set window. + +Merge arrows and related clearcase meta data are not preserved in the conversion, +only the file contents. + +Labels are not yet accounted for. Since labels do not represent an atomic operation +in clearcase, there is not a general way to represent a git tag as a clearcase label. + +=head1 COMMANDS + +=head2 init --vob path + +Initialize the current working directory as a git repository and setup the +configuration for the BVOB to be imported. The results of this are a .git +directory within the current working directory. + +=head2 fetch + +Retrieve the clearcase history for the previously configured BVOB. The first +time this is run, the entire history is logged using the following clearcase +format (cleartool man fmt_ccase): + +%o%m#1#%Nd#1#%Fu %u#1#%En#1#%Vn#1#%[slink_text]p#1#%Nc\n + + %o | Operation kind (checkin, lock, mkelem, and so on) + %m | Object kind (version, derived object, and so on) + | (modifiers: K) + %Nd | (Numeric) Date and time in numeric form -- mmdd.time + | (time reported in 24-hour format). + %Fu | Full name of the user. This information is taken from + | the password database. + %u | User/group information associated with the object's + | creation event (modifiers: F, G, L); see also %[owner]p and + | %[group]p. + %En | Element name: For a file system object, its standard file + | or element name, or its path name; for a type object, its + | name. + %Vn | Version ID: For a version or derived object, the version + | ID; for other objects, the null string. + %Nc | comment string. No newline. Do not append a newline character to + | the comment string. + %[slink_text]p | VOB symbolic links | Target of symbolic link, as +|| displayed by cleartool ls. + +Subsequent runs only pull in the changes since the last run. + +=head2 commit + +This generates git commits from the clearcase event
