Re: [PATCH] git p4 test: fix failure in 9814-git-p4-rename.sh Was: Re: Test failure in t9814-git-p4-rename.sh - my environment or bad test?
On Thu, Jul 24, 2014 at 8:45 PM, Johannes Sixt j...@kdbg.org wrote: Am 23.07.2014 23:28, schrieb Christoph Bonitz: - test $src = file10 || test $src = file11 + test $src = file2 || test $src = file10 || test $src = file11 You can't test for alternatives in this way. It's already wrong in the original line, which is from 795fcb0e (avoid test cond -a/-o cond), and breaks the chain. Thank you, I'm sorry I missed that! -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] git p4 test: fix failure in 9814-git-p4-rename.sh Was: Re: Test failure in t9814-git-p4-rename.sh - my environment or bad test?
On Fri, Jul 25, 2014 at 12:05 AM, Junio C Hamano gits...@pobox.com wrote: Johannes Sixt j...@kdbg.org writes: I see a few other no-nos in the context of the changes, in particular, pipelines where git is not the last command; these would not catch failures in the git commands. But a fix for that is certainly outside the scope of this patch. Yuck. Thanks for spotting. Perhaps we should apply a preliminary clean-up before doing anything else, perhaps? The change in 9814 is a post 2.0 regression. Apart from your change and the word wrap adjustments suggested by Pete, would the following also make sense, to fix the other flaw Johannes pointed out? With regards to failing, git diff-tree should be idempotent. I think those are the two occurrences in this file: diff --git a/t/t9814-git-p4-rename.sh b/t/t9814-git-p4-rename.sh index 1fc1f5f..7815f9a 100755 --- a/t/t9814-git-p4-rename.sh +++ b/t/t9814-git-p4-rename.sh @@ -176,6 +176,7 @@ test_expect_success 'detect copies' ' git diff-tree -r -C --find-copies-harder HEAD level=$(git diff-tree -r -C --find-copies-harder HEAD | sed 1d | cut -f1 | cut -d -f5 | sed s/C test -n $level test $level -gt 0 test $level -lt 98 + git diff-tree -r -C --find-copies-harder HEAD src=$(git diff-tree -r -C --find-copies-harder HEAD | sed 1d | cut -f2) test $src = file10 || test $src = file11 git config git-p4.detectCopies $(($level + 2)) @@ -190,6 +191,7 @@ test_expect_success 'detect copies' ' git diff-tree -r -C --find-copies-harder HEAD level=$(git diff-tree -r -C --find-copies-harder HEAD | sed 1d | cut -f1 | cut -d -f5 | sed s/C test -n $level test $level -gt 2 test $level -lt 100 + git diff-tree -r -C --find-copies-harder HEAD src=$(git diff-tree -r -C --find-copies-harder HEAD | sed 1d | cut -f2) test $src = file10 || test $src = file11 || test $src = file12 git config git-p4.detectCopies $(($level - 2)) -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 4/9] index-helper: new daemon for caching index and related stuff
On Monday, July 28, 2014, Nguyễn Thái Ngọc Duy pclo...@gmail.com wrote:
Instead of reading the index from disk and worrying about disk
corruption, the index is cached in memory (memory bit-flips happen
too, but hopefully less often). The result is faster read.
The biggest gain is not having to verify the trailing SHA-1, which
takes lots of time especially on large index files. But this also
opens doors for further optimiztions:
s/optimiztions/optimizations/
- we could create an in-memory format that's essentially the memory
dump of the index to eliminate most of parsing/allocation
overhead. The mmap'd memory can be used straight away.
- we could cache non-index info such as name hash
The shared memory's name folows the template git-object-SHA1
s/folows/follows/
where SHA1 is the trailing SHA-1 of the index file. object is
index for cached index files (and may be name-hash for name-hash
cache). If such shared memory exists, it contains the same index
content as on disk. The content is already validated by the daemon and
git won't validate it again (except comparing the trailing SHA-1s).
Git can poke the daemon to tell it to refresh the index cache, or to
keep it alive some more minutes via UNIX signals. It can't give any
real index data directly to the daemon. Real data goes to disk first,
then the daemon reads and verifies it from there.
$GIT_DIR/index-helper.pid contains a reference to daemon process (and
it's pid on *nix). The file's mtime is updated every time it's accessed
s/it's pid/its pid/
(or should be updated often enough). Old index-helper.pid is considered
stale and ignored.
Signed-off-by: Nguyễn Thái Ngọc Duy pclo...@gmail.com
---
diff --git a/index-helper.c b/index-helper.c
new file mode 100644
index 000..8fa0af9
--- /dev/null
+++ b/index-helper.c
@@ -0,0 +1,157 @@
+static void share_index(struct index_state *istate, struct index_shm *is)
+{
+ void *new_mmap;
+ if (istate-mmap_size = 20 ||
+ hashcmp(istate-sha1,
+ (unsigned char *)istate-mmap + istate-mmap_size - 20) ||
+ !hashcmp(istate-sha1, is-sha1) ||
+ git_shm_map(O_CREAT | O_EXCL | O_RDWR, 0700, istate-mmap_size,
+ new_mmap, PROT_READ | PROT_WRITE, MAP_SHARED,
+ git-index-%s, sha1_to_hex(istate-sha1)) 0)
Do any of these failure conditions deserve a diagnostic message to let
the user know that there was a problem?
+ return;
+
+ free_index_shm(is);
+ is-size = istate-mmap_size;
+ is-shm = new_mmap;
+ hashcpy(is-sha1, istate-sha1);
+ memcpy(new_mmap, istate-mmap, istate-mmap_size - 20);
+
+ /*
+* The trailing hash must be written last after everything is
+* written. It's the indication that the shared memory is now
+* ready.
+*/
+ hashcpy((unsigned char *)new_mmap + istate-mmap_size - 20, is-sha1);
+}
diff --git a/read-cache.c b/read-cache.c
index b679781..ff28142 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -1465,6 +1467,65 @@ static struct cache_entry *create_from_disk(struct
ondisk_cache_entry *ondisk,
return ce;
}
+/*
+ * Try to open and verify a cached shm index if available. Return 0 if
+ * succeeds (istate-mmap and istate-mmap_size are updated). Return
+ * negative otherwise.
+ */
+static int try_shm(struct index_state *istate)
+{
+ void *new_mmap = NULL;
+ size_t old_size = istate-mmap_size;
Is old_size needed? Can you not simply reference istate-mmap_size
directly each place old_size is mentioned?
+ ssize_t new_length;
Nit: 'size' vs. 'length' inconsistency in variable name choices.
+ const unsigned char *sha1;
+ struct stat st;
+
+ if (old_size = 20 ||
+ stat(git_path(index-helper.pid), st))
+ return -1;
+ sha1 = (unsigned char *)istate-mmap + old_size - 20;
+ new_length = git_shm_map(O_RDONLY, 0700, -1, new_mmap,
+PROT_READ, MAP_SHARED,
+git-index-%s, sha1_to_hex(sha1));
+ if (new_length = 20 ||
+ hashcmp((unsigned char *)istate-mmap + old_size - 20,
+ (unsigned char *)new_mmap + new_length - 20)) {
+ if (new_mmap)
+ munmap(new_mmap, new_length);
+ poke_daemon(st, 1);
+ return -1;
+ }
+ munmap(istate-mmap, istate-mmap_size);
+ istate-mmap = new_mmap;
+ istate-mmap_size = new_length;
+ poke_daemon(st, 0);
+ return 0;
+}
+
/* remember to discard_cache() before reading a different cache! */
int do_read_index(struct index_state *istate, const char *path, int
must_exist)
{
diff --git a/shm.c b/shm.c
new file mode 100644
index 000..4ec1a00
--- /dev/null
+++ b/shm.c
@@ -0,0 +1,67 @@
+#include git-compat-util.h
+#include
Re: Amending merge commits?
Nico Williams n...@cryptonector.com writes: On Tue, Jul 29, 2014 at 4:58 AM, Sergei Organov o...@javad.com wrote: Nico Williams n...@cryptonector.com writes: That exception aside, keeping all local commits on top by always rebasing them onto the upstream is extremely useful: a) in simplifying conflict resolution, b) making it easy to identify as-yet-unintegrated local commits, c) making it easy to contribute local commits. But 'pull --rebase=preserve' does rebase local commits onto the upstream, and result is exactly the same as 'pull --rebase=true', unless you have some of your own merges to be rebased. That's where the difference between these two options appears. It's --rebase=false that performs merges rather than rebase. Local merge commits mean that you either didn't rebase to keep all your local commits on top of the upstream, or that you have multiple upstreams (the example exception I gave). I rather have multiple (release) branches on single upstream, say, v2.3 and v2.4. When something needs to be fixed in 2.3, it's fixed there and pushed upstream, then, on 2.4, the 2.3 is merged to it, and result is pushed upstream. When I do this merge, I need to push the merge upstream, and this won't work reliably when --rebase=true is acitve (through pull.merge=rebase). If nothing changes upstream, I can simply push this, and the merge is correctly preserved. However, if somebody makes any changes upstream while I perform the merge, I'll need to pull before pushing, and this immediately flattens-out my merge, that is absolutely not what is needed here. Or I can simply pull before push, just in case, and this flattens history even when there are no any changes upstream! Once again, nobody yet gave any clue of when/why pull.merge=preserve configuration is inferior to pull.merge=rebase, as for all the scenario you seem to care about they bring the same result. Conversely, if you always rebase your local commits on top of the upstream then you won't have merge commits to worry about. Wrong. I do alwys rebase my local commits on top of upstream, but I still do have my own merge commits to worry about, as explained above. -- Sergey. -- 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] commit --amend: test specifies authorship but forgets to check
The test case --amend option copies authorship specifies that the git-commit option `--amend` uses the authorship of the replaced commit for the new commit. Add the omitted check that this property actually holds. Signed-off-by: Fabian Ruch baf...@gmail.com --- Without the check, the test case succeeds even with nonsense in the `expected` file. An `--amend` implementation which simply uses the committer name and date as if it was not amending would have been deemed correct. This is not the case, the implementation still passes the test suite after the correction. Quickly skimming over the rest of the file, I couldn't find the same thing twice. t/t7509-commit.sh | 1 + 1 file changed, 1 insertion(+) diff --git a/t/t7509-commit.sh b/t/t7509-commit.sh index b61fd3c..9ac7940 100755 --- a/t/t7509-commit.sh +++ b/t/t7509-commit.sh @@ -77,6 +77,7 @@ test_expect_success '--amend option copies authorship' ' git commit -a --amend -m amend test author_header Initial expect author_header HEAD actual + test_cmp expect actual echo amend test expect message_body HEAD actual -- 2.0.1 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] utf8.c: fix strbuf_utf8_replace copying the last NUL to dst string
On Tue, Jul 29, 2014 at 12:56:24PM -0700, Junio C Hamano wrote:
Nguyễn Thái Ngọc Duy pclo...@gmail.com writes:
When utf8_width(src) is called with *src == NULL (because the
source string ends with an ansi sequence),
I am not sure what you mean by because here. Do you mean somebody
(who?) decides to call the utf8_width() with NULL pointer stored in
*src because of ansi sequence?
What do you mean by ansi sequence? I'll assume that you mean
those terminal control that all use bytes with hi-bit clear.
OK let's try with an example, strbuf_utf8_replace(sb, 0, 0, ) where
sb contains \033[m. The expected result is exactly the same string
with length of 3. After this block
while ((n = display_mode_esc_sequence_len(src))) {
memcpy(dst, src, n);
src += n;
dst += n;
}
src will be right after 'm', *src is NUL (iow. src == end). After
n = utf8_width((const char**)src, NULL);
n is zero but 'src' is advanced by another character, so
src - old_src == 1. This NUL character is counted as part of the
string, so after the _setlen, we have the same string with length of
_4_ (instead of 3).
At the very beginning of utf8_width(), *start can be cleared to
point at a NULL pointer by pick_one_utf8_char() if the pointer that
comes into utf8_width() originally points at an invalid UTF-8
string, but as far as I can see, ESC (or any bytes that would be
used in those terminal control sequences like colors and cursor
control) will simply be returned as a single byte, without going
into error path that clears *start = NULL.
Puzzled...
it returns 0 and steps 'src' by one.
Here it refers to utf8_width()? Who steps 'src' by one?
utf8_width() steps 'src'.
Ahh, did you mean *src == NUL, i.e. already at the end of the
string?
Yes.. I guess you have a better commit message prepared for me now :)
I think utf8_width() called with an empty string should not move the
pointer past that end-of-string NUL in the first place. It makes me
wonder if it would be a better fix to make it not to do that (and
return 0), but if we declare it is the caller's fault, perhaps we
may want to add
if (!**start)
die(BUG: empty string to utf8_width()???);
at the very beginning of utf8_width(), even before it calls
pick-one-utf8-char.
Still puzzled...
I don't know. I mean, in a UTF-8 byte sequence, NUL is a valid
character (part of the ASCII subset), so advancing '*start' by one
makes sense. Whether we have any call sites crazy enough to do that on
purpose is a different matter.
This stepping makes strbuf_utf8_replace add NUL to the
destination string at the end of the loop. Check and break the loop
early.
Signed-off-by: Nguyễn Thái Ngọc Duy pclo...@gmail.com
---
utf8.c | 2 ++
1 file changed, 2 insertions(+)
Tests?
Ugh.. this was triggered by one of the alignment specifier in log
--pretty. Probably easier to export strbuf_utf8_replace() as a
test-command and verify the output?
--
Duy
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 7/8] checkout: prefix --to argument properly when cwd is moved
On Wed, Jul 30, 2014 at 3:51 AM, Junio C Hamano gits...@pobox.com wrote: Nguyễn Thái Ngọc Duy pclo...@gmail.com writes: Signed-off-by: Nguyễn Thái Ngọc Duy pclo...@gmail.com --- builtin/checkout.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/builtin/checkout.c b/builtin/checkout.c index 173aab1..4fbb9c1 100644 --- a/builtin/checkout.c +++ b/builtin/checkout.c @@ -1342,7 +1342,7 @@ int cmd_checkout(int argc, const char **argv, const char *prefix) N_(do not limit pathspecs to sparse entries only)), OPT_HIDDEN_BOOL(0, guess, dwim_new_local_branch, N_(second guess 'git checkout no-such-branch')), - OPT_STRING(0, to, opts.new_worktree, N_(path), + OPT_FILENAME(0, to, opts.new_worktree, N_(check a branch out in a separate working directory)), OPT_END(), }; Good thinking. Otherwise this would not work from within a subdirectory. Perhaps you would want a test for it? Will do at the next round. An unrelated tangent, but would we want to further enhance OPT_FILENAME() to understand ~/path and ~user/path perhaps? It is easy to rely on users' shells to do so, so it is not a big deal and it certainly is unrelated to this particular topic. Yeah, the only user group that probably wants this is cmd.exe users. We already have expand_user_path for this, all needed is some glue. I'll let Windows users do this if they really want to. -- Duy -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 4/9] index-helper: new daemon for caching index and related stuff
On Wed, Jul 30, 2014 at 3:08 PM, Eric Sunshine sunsh...@sunshineco.com wrote:
+static void share_index(struct index_state *istate, struct index_shm *is)
+{
+ void *new_mmap;
+ if (istate-mmap_size = 20 ||
+ hashcmp(istate-sha1,
+ (unsigned char *)istate-mmap + istate-mmap_size - 20)
||
+ !hashcmp(istate-sha1, is-sha1) ||
+ git_shm_map(O_CREAT | O_EXCL | O_RDWR, 0700, istate-mmap_size,
+ new_mmap, PROT_READ | PROT_WRITE, MAP_SHARED,
+ git-index-%s, sha1_to_hex(istate-sha1)) 0)
Do any of these failure conditions deserve a diagnostic message to let
the user know that there was a problem?
Hmm.. diagnostic messages are a problem already. This most likely runs
as a daemon, nowhere to print to. But if running on foreground, some
messages would help.
+static int try_shm(struct index_state *istate)
+{
+ void *new_mmap = NULL;
+ size_t old_size = istate-mmap_size;
Is old_size needed? Can you not simply reference istate-mmap_size
directly each place old_size is mentioned?
+ ssize_t new_length;
Nit: 'size' vs. 'length' inconsistency in variable name choices.
Leftovers after many iterations. Will fix.
+#define SHM_PATH_LEN 72/* we don't create very long paths..
*/
+
+ssize_t git_shm_map(int oflag, int perm, ssize_t length, void **mmap,
+ int prot, int flags, const char *fmt, ...)
+{
+ va_list ap;
+ char path[SHM_PATH_LEN];
Is there a reason to avoid strbuf here?
Laziness (which is ironic as I'm working on patches to clean up fixed
size buffers)
--
Duy
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Bug report about symlinks
Greetings from Russia, comrads! I've noticed something strange with git status when replacing a folder with symlink to another folder. There is a git repo with script with demo in the attachment. Yours sincerely, NickKolok aka Nikolay Avdeev. Доброго времени суток, товарищи! При замене папки на симлинк на другую папку с похожими, но не одинаковыми файлами git status ведёт себя странно. Прилагаю архив с репозиторием. В скрипте - пример и пояснения. С уважением, Николай Авдеев aka NickKolok. git-bug-demo.tar.bz2 Description: Binary data
Kindly reply
My name is Carlos Ramirez I am lawyer here in Madrid Spain. I want to transfer an abandoned sum of 3.5 Millions USD to your account.50% will be for you. No risk involved. Contact me for more details. Kindly reply me back to my alternative email address ( carlos...@aol.com ) Regards Carlos Ramirez -- 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 0/5] git_config callers rewritten with the new config cache API
[PATCH v4]: Tiny style nits corrected. Patch 2/5 has been totally reworked. One thing to check is if the config variables I changed in the series are single valued or multi valued. Though I have tried to ascertain if the variable was single valued or not by reading the docs and code, mistakes may creep in. [PATCH v3]: Most of Eric's suggestions has been implemented. See [2] for discussion. Also, new helpers introduced in v7 of the config-set API series have been used. This series builds on the top of 4c715ebb in pu (ta/config-set). See [1] for the documentation of the new API functions and a general description of the new API. [1]: http://thread.gmane.org/gmane.comp.version-control.git/254286/ [2]: http://thread.gmane.org/gmane.comp.version-control.git/252334 Tanay Abhra (5): alias.c | 25 ++--- branch.c| 24 imap-send.c | 61 +++-- notes.c | 29 ++--- pager.c | 40 +--- 5 files changed, 64 insertions(+), 115 deletions(-) -- 1.9.0.GIT -- 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 4/5] branch.c: replace `git_config()` with `git_config_get_string()
Use `git_config_get_string()` instead of `git_config()` to take advantage of
the config-set API which provides a cleaner control flow.
Signed-off-by: Tanay Abhra tanay...@gmail.com
---
branch.c | 24
1 file changed, 4 insertions(+), 20 deletions(-)
diff --git a/branch.c b/branch.c
index 46e8aa8..df6b120 100644
--- a/branch.c
+++ b/branch.c
@@ -140,33 +140,17 @@ static int setup_tracking(const char *new_ref, const char
*orig_ref,
return 0;
}
-struct branch_desc_cb {
- const char *config_name;
- const char *value;
-};
-
-static int read_branch_desc_cb(const char *var, const char *value, void *cb)
-{
- struct branch_desc_cb *desc = cb;
- if (strcmp(desc-config_name, var))
- return 0;
- free((char *)desc-value);
- return git_config_string(desc-value, var, value);
-}
-
int read_branch_desc(struct strbuf *buf, const char *branch_name)
{
- struct branch_desc_cb cb;
+ char *v = NULL;
struct strbuf name = STRBUF_INIT;
strbuf_addf(name, branch.%s.description, branch_name);
- cb.config_name = name.buf;
- cb.value = NULL;
- if (git_config(read_branch_desc_cb, cb) 0) {
+ if (git_config_get_string(name.buf, v)) {
strbuf_release(name);
return -1;
}
- if (cb.value)
- strbuf_addstr(buf, cb.value);
+ strbuf_addstr(buf, v);
+ free(v);
strbuf_release(name);
return 0;
}
--
1.9.0.GIT
--
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 2/5] notes.c: replace `git_config()` with `git_config_get_value_multi()`
Use `git_config_get_value_multi()` instead of `git_config()` to take
advantage of the config-set API which provides a cleaner control flow,
also previously 'string_list_add_refs_by_glob()' was called even when
the retrieved value was NULL, correct it while we are at it.
Signed-off-by: Tanay Abhra tanay...@gmail.com
---
notes.c | 29 ++---
1 file changed, 14 insertions(+), 15 deletions(-)
diff --git a/notes.c b/notes.c
index 5fe691d..abb0ce0 100644
--- a/notes.c
+++ b/notes.c
@@ -961,19 +961,6 @@ void string_list_add_refs_from_colon_sep(struct
string_list *list,
free(globs_copy);
}
-static int notes_display_config(const char *k, const char *v, void *cb)
-{
- int *load_refs = cb;
-
- if (*load_refs !strcmp(k, notes.displayref)) {
- if (!v)
- config_error_nonbool(k);
- string_list_add_refs_by_glob(display_notes_refs, v);
- }
-
- return 0;
-}
-
const char *default_notes_ref(void)
{
const char *notes_ref = NULL;
@@ -1041,7 +1028,8 @@ struct notes_tree **load_notes_trees(struct string_list
*refs)
void init_display_notes(struct display_notes_opt *opt)
{
char *display_ref_env;
- int load_config_refs = 0;
+ const struct string_list *values;
+ int load_config_refs = 0, i;
display_notes_refs.strdup_strings = 1;
assert(!display_notes_trees);
@@ -1058,7 +1046,18 @@ void init_display_notes(struct display_notes_opt *opt)
load_config_refs = 1;
}
- git_config(notes_display_config, load_config_refs);
+ if (load_config_refs) {
+ values = git_config_get_value_multi(notes.displayref);
+ if (values) {
+ for (i = 0; i values-nr; i++) {
+ if (!values-items[i].string)
+
config_error_nonbool(notes.displayref);
+ else
+
string_list_add_refs_by_glob(display_notes_refs,
+
values-items[i].string);
+ }
+ }
+ }
if (opt) {
struct string_list_item *item;
--
1.9.0.GIT
--
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/5] pager.c: replace `git_config()` with `git_config_get_value()`
Use `git_config_get_value()` instead of `git_config()` to take advantage of
the config-set API which provides a cleaner control flow.
Signed-off-by: Tanay Abhra tanay...@gmail.com
---
pager.c | 40 +---
1 file changed, 13 insertions(+), 27 deletions(-)
diff --git a/pager.c b/pager.c
index 8b5cbc5..b7eb7e7 100644
--- a/pager.c
+++ b/pager.c
@@ -6,12 +6,6 @@
#define DEFAULT_PAGER less
#endif
-struct pager_config {
- const char *cmd;
- int want;
- char *value;
-};
-
/*
* This is split up from the rest of git so that we can do
* something different on Windows.
@@ -155,30 +149,22 @@ int decimal_width(int number)
return width;
}
-static int pager_command_config(const char *var, const char *value, void *data)
+/* returns 0 for no pager, 1 for use pager, and -1 for not specified */
+int check_pager_config(const char *cmd)
{
- struct pager_config *c = data;
- if (starts_with(var, pager.) !strcmp(var + 6, c-cmd)) {
- int b = git_config_maybe_bool(var, value);
+ int want = -1;
+ struct strbuf key = STRBUF_INIT;
+ const char *value = NULL;
+ strbuf_addf(key, pager.%s, cmd);
+ if (!git_config_get_value(key.buf, value)) {
+ int b = git_config_maybe_bool(key.buf, value);
if (b = 0)
- c-want = b;
+ want = b;
else {
- c-want = 1;
- c-value = xstrdup(value);
+ want = 1;
+ pager_program = xstrdup(value);
}
}
- return 0;
-}
-
-/* returns 0 for no pager, 1 for use pager, and -1 for not specified */
-int check_pager_config(const char *cmd)
-{
- struct pager_config c;
- c.cmd = cmd;
- c.want = -1;
- c.value = NULL;
- git_config(pager_command_config, c);
- if (c.value)
- pager_program = c.value;
- return c.want;
+ strbuf_release(key);
+ return want;
}
--
1.9.0.GIT
--
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 5/5] alias.c: replace `git_config()` with `git_config_get_string()`
Use `git_config_get_string()` instead of `git_config()` to take advantage of
the config-set API which provides a cleaner control flow.
Signed-off-by: Tanay Abhra tanay...@gmail.com
---
alias.c | 25 ++---
1 file changed, 6 insertions(+), 19 deletions(-)
diff --git a/alias.c b/alias.c
index 758c867..6aa164a 100644
--- a/alias.c
+++ b/alias.c
@@ -1,26 +1,13 @@
#include cache.h
-static const char *alias_key;
-static char *alias_val;
-
-static int alias_lookup_cb(const char *k, const char *v, void *cb)
-{
- const char *name;
- if (skip_prefix(k, alias., name) !strcmp(name, alias_key)) {
- if (!v)
- return config_error_nonbool(k);
- alias_val = xstrdup(v);
- return 0;
- }
- return 0;
-}
-
char *alias_lookup(const char *alias)
{
- alias_key = alias;
- alias_val = NULL;
- git_config(alias_lookup_cb, NULL);
- return alias_val;
+ char *v = NULL;
+ struct strbuf key = STRBUF_INIT;
+ strbuf_addf(key, alias.%s, alias);
+ git_config_get_string(key.buf, v);
+ strbuf_release(key);
+ return v;
}
#define SPLIT_CMDLINE_BAD_ENDING 1
--
1.9.0.GIT
--
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 3/5] imap-send.c: replace `git_config()` with `git_config_get_*()` family
Use `git_config_get_*()` family instead of `git_config()` to take advantage of
the config-set API which provides a cleaner control flow.
Signed-off-by: Tanay Abhra tanay...@gmail.com
---
imap-send.c | 61 +++--
1 file changed, 27 insertions(+), 34 deletions(-)
diff --git a/imap-send.c b/imap-send.c
index 524fbab..586bdd8 100644
--- a/imap-send.c
+++ b/imap-send.c
@@ -1326,43 +1326,36 @@ static int split_msg(struct strbuf *all_msgs, struct
strbuf *msg, int *ofs)
static char *imap_folder;
-static int git_imap_config(const char *key, const char *val, void *cb)
+static void git_imap_config(void)
{
- if (!skip_prefix(key, imap., key))
- return 0;
+ const char *val = NULL;
+
+ git_config_get_bool(imap.sslverify, server.ssl_verify);
+ git_config_get_bool(imap.preformattedhtml, server.use_html);
+ git_config_get_string(imap.folder, imap_folder);
- /* check booleans first, and barf on others */
- if (!strcmp(sslverify, key))
- server.ssl_verify = git_config_bool(key, val);
- else if (!strcmp(preformattedhtml, key))
- server.use_html = git_config_bool(key, val);
- else if (!val)
- return config_error_nonbool(key);
-
- if (!strcmp(folder, key)) {
- imap_folder = xstrdup(val);
- } else if (!strcmp(host, key)) {
- if (starts_with(val, imap:))
- val += 5;
- else if (starts_with(val, imaps:)) {
- val += 6;
- server.use_ssl = 1;
+ if (!git_config_get_value(imap.host, val)) {
+ if (!val) {
+ config_error_nonbool(imap.host);
+ git_die_config(imap.host);
+ } else {
+ if (starts_with(val, imap:))
+ val += 5;
+ else if (starts_with(val, imaps:)) {
+ val += 6;
+ server.use_ssl = 1;
+ }
+ if (starts_with(val, //))
+ val += 2;
+ server.host = xstrdup(val);
}
- if (starts_with(val, //))
- val += 2;
- server.host = xstrdup(val);
- } else if (!strcmp(user, key))
- server.user = xstrdup(val);
- else if (!strcmp(pass, key))
- server.pass = xstrdup(val);
- else if (!strcmp(port, key))
- server.port = git_config_int(key, val);
- else if (!strcmp(tunnel, key))
- server.tunnel = xstrdup(val);
- else if (!strcmp(authmethod, key))
- server.auth_method = xstrdup(val);
+ }
- return 0;
+ git_config_get_string(imap.user, server.user);
+ git_config_get_string(imap.pass, server.pass);
+ git_config_get_int(imap.port, server.port);
+ git_config_get_string(imap.tunnel, server.tunnel);
+ git_config_get_string(imap.authmethod, server.auth_method);
}
int main(int argc, char **argv)
@@ -1383,7 +1376,7 @@ int main(int argc, char **argv)
usage(imap_send_usage);
setup_git_directory_gently(nongit_ok);
- git_config(git_imap_config, NULL);
+ git_imap_config();
if (!server.port)
server.port = server.use_ssl ? 993 : 143;
--
1.9.0.GIT
--
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 v4 0/5] git_config callers rewritten with the new config cache API
Tanay Abhra tanay...@gmail.com writes: [PATCH v4]: Tiny style nits corrected. Patch 2/5 has been totally reworked. One thing to check is if the config variables I changed in the series are single valued or multi valued. Though I have tried to ascertain if the variable was single valued or not by reading the docs and code, mistakes may creep in. [PATCH v3]: Most of Eric's suggestions has been implemented. See [2] for discussion. Also, new helpers introduced in v7 of the config-set API series have been used. This series builds on the top of 4c715ebb in pu (ta/config-set). I think it semantically needs your other WIP series, that makes git_config_get_* die in case of error. Otherwise, there's an unexpected behavior change in case of error. -- 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
Re: [PATCH v4 0/5] git_config callers rewritten with the new config cache API
On 7/30/2014 7:16 PM, Matthieu Moy wrote: Tanay Abhra tanay...@gmail.com writes: [PATCH v4]: Tiny style nits corrected. Patch 2/5 has been totally reworked. One thing to check is if the config variables I changed in the series are single valued or multi valued. Though I have tried to ascertain if the variable was single valued or not by reading the docs and code, mistakes may creep in. [PATCH v3]: Most of Eric's suggestions has been implemented. See [2] for discussion. Also, new helpers introduced in v7 of the config-set API series have been used. This series builds on the top of 4c715ebb in pu (ta/config-set). I think it semantically needs your other WIP series, that makes git_config_get_* die in case of error. Otherwise, there's an unexpected behavior change in case of error. Yes you are right, there is a call to git_die_config() also in the series. I will add the info in the next reroll. Also, any thoughts on what to do with git_default_config()? We can, 1 make a new function git_load_default_config(), use it for the rewrites. or 2 git_default_config() is rewritten to be loaded only once even if it is called again and again during the process run, so that we use the same function in callbacks and in the new API using rewrites. 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 v4 2/5] notes.c: replace `git_config()` with `git_config_get_value_multi()`
Tanay Abhra tanay...@gmail.com writes:
- git_config(notes_display_config, load_config_refs);
+ if (load_config_refs) {
+ values = git_config_get_value_multi(notes.displayref);
+ if (values) {
+ for (i = 0; i values-nr; i++) {
+ if (!values-items[i].string)
+
config_error_nonbool(notes.displayref);
+ else
+
string_list_add_refs_by_glob(display_notes_refs,
+
values-items[i].string);
+ }
+ }
+ }
It seems to me that you're doing a lot here that should have been done
once in the config API:
* if (values) {
for (i = 0; i values-nr
= We could avoid the if statement if git_config_get_value_multi was
always returning a string_list, possibly empty (values-nr == 0
instead of values == NULL).
Not as obvious as it seems, because you normally return a pointer to
the string_list that is already in the hashmap, so you can't just
malloc() an empty one if you don't want to leak it.
Another option would be to provide an iterator that would call a
function on each value of the list, and do nothing when there's no
list at all (back to the callback-style API, but you would iterate
only through the values for the right key).
* if (!values-items[i].string)
config_error_nonbool(
= This check could be done once and for all in a function, say
git_config_get_value_multi_nonbool, a trivial wrapper around
git_config_get_value_multi like
const struct string_list *git_configset_get_value_multi_nonbool(struct
config_set *cs, const char *key)
{
struct string_list l = git_configset_get_value_multi(cs, key);
// possibly if(l) depending on the point above.
for (i = 0; i values-nr; i++) {
if (!values-items[i].string)
git_config_die(key);
}
return l;
}
const struct string_list *git_config_get_value_multi_nonbool(const char *key)
{
git_config_check_init();
return git_configset_get_value_multi_nonbool(the_config_set, key);
}
(totally untested)
BTW, is it intentional that you call config_error_nonbool() without
die-ing?
--
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
Re: [PATCH v4 2/5] notes.c: replace `git_config()` with `git_config_get_value_multi()`
On 7/30/2014 7:43 PM, Matthieu Moy wrote:
Tanay Abhra tanay...@gmail.com writes:
-git_config(notes_display_config, load_config_refs);
+if (load_config_refs) {
+values = git_config_get_value_multi(notes.displayref);
+if (values) {
+for (i = 0; i values-nr; i++) {
+if (!values-items[i].string)
+
config_error_nonbool(notes.displayref);
+else
+
string_list_add_refs_by_glob(display_notes_refs,
+
values-items[i].string);
+}
+}
+}
It seems to me that you're doing a lot here that should have been done
once in the config API:
* if (values) {
for (i = 0; i values-nr
= We could avoid the if statement if git_config_get_value_multi was
always returning a string_list, possibly empty (values-nr == 0
instead of values == NULL).
or we can do something like,
if (!git_config_get_value_multi(notes.displayref, values)) {
/* return 0 if there is a value_list for the key */
Not as obvious as it seems, because you normally return a pointer to
the string_list that is already in the hashmap, so you can't just
malloc() an empty one if you don't want to leak it.
Another option would be to provide an iterator that would call a
function on each value of the list, and do nothing when there's no
list at all (back to the callback-style API, but you would iterate
only through the values for the right key).
This is also a good idea, but still we are back to the callback API,
and what we are gaining is fewer loop iterations than git_config().
Which way do you prefer, a reroll is easy but Junio might have been sick
of replacing the patches in pu by now. :)
* if (!values-items[i].string)
config_error_nonbool(
= This check could be done once and for all in a function, say
git_config_get_value_multi_nonbool, a trivial wrapper around
git_config_get_value_multi like
const struct string_list *git_configset_get_value_multi_nonbool(struct
config_set *cs, const char *key)
{
struct string_list l = git_configset_get_value_multi(cs, key);
// possibly if(l) depending on the point above.
for (i = 0; i values-nr; i++) {
if (!values-items[i].string)
git_config_die(key);
}
return l;
}
Not worth it, most the multi value calls do not die on a nonbool.
const struct string_list *git_config_get_value_multi_nonbool(const char *key)
{
git_config_check_init();
return git_configset_get_value_multi_nonbool(the_config_set, key);
}
(totally untested)
BTW, is it intentional that you call config_error_nonbool() without
die-ing?
Yup, it's intentional, original code didn't die for empty values, and it seemed
logical to me to emulate that over to the rewrite.
--
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 v4 4/5] branch.c: replace `git_config()` with `git_config_get_string()
Tanay Abhra tanay...@gmail.com writes:
int read_branch_desc(struct strbuf *buf, const char *branch_name)
{
- struct branch_desc_cb cb;
+ char *v = NULL;
struct strbuf name = STRBUF_INIT;
strbuf_addf(name, branch.%s.description, branch_name);
- cb.config_name = name.buf;
- cb.value = NULL;
- if (git_config(read_branch_desc_cb, cb) 0) {
+ if (git_config_get_string(name.buf, v)) {
strbuf_release(name);
return -1;
}
- if (cb.value)
- strbuf_addstr(buf, cb.value);
+ strbuf_addstr(buf, v);
+ free(v);
strbuf_release(name);
return 0;
}
I think this is a behavior change. if (git_config(read_branch_desc_cb,
cb) 0) was never true in practice, so the return -1 was essentially
dead code. You now return -1 when no value is found.
It probably doesn't matter, since all caller except
fmt-merge-msg.c:add_branch_desc() ignore the return value, and if I read
correctly, add_branch_desc does not need the test on the return value,
as the then branch of the if does nothing if no value is found anyway.
But here again, I have to wonder why the function does not just return
void.
--
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
Re: [PATCH v4 2/5] notes.c: replace `git_config()` with `git_config_get_value_multi()`
Tanay Abhra tanay...@gmail.com writes:
On 7/30/2014 7:43 PM, Matthieu Moy wrote:
Tanay Abhra tanay...@gmail.com writes:
- git_config(notes_display_config, load_config_refs);
+ if (load_config_refs) {
+ values = git_config_get_value_multi(notes.displayref);
+ if (values) {
+ for (i = 0; i values-nr; i++) {
+ if (!values-items[i].string)
+
config_error_nonbool(notes.displayref);
+ else
+
string_list_add_refs_by_glob(display_notes_refs,
+
values-items[i].string);
+ }
+ }
+ }
It seems to me that you're doing a lot here that should have been done
once in the config API:
* if (values) {
for (i = 0; i values-nr
= We could avoid the if statement if git_config_get_value_multi was
always returning a string_list, possibly empty (values-nr == 0
instead of values == NULL).
or we can do something like,
if (!git_config_get_value_multi(notes.displayref, values)) {
/* return 0 if there is a value_list for the key */
Not as obvious as it seems, because you normally return a pointer to
the string_list that is already in the hashmap, so you can't just
malloc() an empty one if you don't want to leak it.
Another option would be to provide an iterator that would call a
function on each value of the list, and do nothing when there's no
list at all (back to the callback-style API, but you would iterate
only through the values for the right key).
This is also a good idea, but still we are back to the callback API,
and what we are gaining is fewer loop iterations than git_config().
Regardless of performance, the code would also be a bit shorter, since
the callback just gets the values for the right key, so it doesn't need
to re-test that the key is the right one.
Here, the callback would basically be the body of the for loop above.
Which way do you prefer, a reroll is easy but Junio might have been sick
of replacing the patches in pu by now. :)
No need to replace anything, you can add new helpers on top of the
existing.
Do it the way you feel is better, I'm just giving ideas.
* if (!values-items[i].string)
config_error_nonbool(
= This check could be done once and for all in a function, say
git_config_get_value_multi_nonbool, a trivial wrapper around
git_config_get_value_multi like
const struct string_list *git_configset_get_value_multi_nonbool(struct
config_set *cs, const char *key)
{
struct string_list l = git_configset_get_value_multi(cs, key);
// possibly if(l) depending on the point above.
for (i = 0; i values-nr; i++) {
if (!values-items[i].string)
git_config_die(key);
}
return l;
}
Not worth it, most the multi value calls do not die on a nonbool.
Can you cite some multi-value variables that can be nonbool? I can't
find many multi-valued variables, and I can't find any which would allow
bool and nonbool.
const struct string_list *git_config_get_value_multi_nonbool(const char *key)
{
git_config_check_init();
return git_configset_get_value_multi_nonbool(the_config_set, key);
}
(totally untested)
BTW, is it intentional that you call config_error_nonbool() without
die-ing?
Yup, it's intentional, original code didn't die for empty values, and it
seemed
logical to me to emulate that over to the rewrite.
The old code was doing
if (*load_refs !strcmp(k, notes.displayref)) {
if (!v)
config_error_nonbool(k);
string_list_add_refs_by_glob(display_notes_refs, v);
It seems that the intent of the programmer was
if (*load_refs !strcmp(k, notes.displayref)) {
if (!v)
return config_error_nonbool(k); // ---
string_list_add_refs_by_glob(display_notes_refs, v);
At least, that would explain why the code uses v even after testing that
it is a NULL pointer.
You're already fixing a bug in your patch by not using NULL values, but
then I don't see any reason to keep the old weird behavior (display an
error but do not die).
--
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
Re: [PATCH v4 0/5] git_config callers rewritten with the new config cache API
Tanay Abhra tanay...@gmail.com writes: Yes you are right, there is a call to git_die_config() also in the series. I will add the info in the next reroll. If unsure, rebase your branch locally on the commit on which it is meant to apply, and check that it works for you. Also, any thoughts on what to do with git_default_config()? We can, 1 make a new function git_load_default_config(), use it for the rewrites. That seems the most sensible option. It could be called it git.c before the command-dependant part, so that any call to git loads this. I didn't check if it was correct (e.g. do some command rely on the fact that the default config is not loaded?) 2 git_default_config() is rewritten to be loaded only once even if it is called again and again during the process run, so that we use the same function in callbacks and in the new API using rewrites. Seems like a workaround, and to me the point of your GSoC is to make the code cleaner, hence avoid workaraounds ... -- 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
Transaction patch series overview
List, please see here an overview and ordering of the ref transaction patch series. These series build on each other and needs to be applied in the order listed below. rs/ref-transaction-0 --- Early part of the ref transaction topic. * rs/ref-transaction-0: refs.c: change ref_transaction_update() to do error checking and return status refs.c: remove the onerr argument to ref_transaction_commit update-ref: use err argument to get error from ref_transaction_commit refs.c: make update_ref_write update a strbuf on failure refs.c: make ref_update_reject_duplicates take a strbuf argument for errors refs.c: log_ref_write should try to return meaningful errno refs.c: make resolve_ref_unsafe set errno to something meaningful on error refs.c: commit_packed_refs to return a meaningful errno on failure refs.c: make remove_empty_directories always set errno to something sane refs.c: verify_lock should set errno to something meaningful refs.c: make sure log_ref_setup returns a meaningful errno refs.c: add an err argument to repack_without_refs lockfile.c: make lock_file return a meaningful errno on failurei lockfile.c: add a new public function unable_to_lock_message refs.c: add a strbuf argument to ref_transaction_commit for error logging refs.c: allow passing NULL to ref_transaction_free refs.c: constify the sha arguments for ref_transaction_create|delete|update refs.c: ref_transaction_commit should not free the transaction refs.c: remove ref_transaction_rollback Has been merged into next. ref-transaction-1 (2014-07-16) 20 commits - Second batch of ref transactions - refs.c: make delete_ref use a transaction - refs.c: make prune_ref use a transaction to delete the ref - refs.c: remove lock_ref_sha1 - refs.c: remove the update_ref_write function - refs.c: remove the update_ref_lock function - refs.c: make lock_ref_sha1 static - walker.c: use ref transaction for ref updates - fast-import.c: use a ref transaction when dumping tags - receive-pack.c: use a reference transaction for updating the refs - refs.c: change update_ref to use a transaction - branch.c: use ref transaction for all ref updates - fast-import.c: change update_branch to use ref transactions - sequencer.c: use ref transactions for all ref updates - commit.c: use ref transactions for updates - replace.c: use the ref transaction functions for updates - tag.c: use ref transactions when doing updates - refs.c: add transaction.status and track OPEN/CLOSED/ERROR - refs.c: make ref_transaction_begin take an err argument - refs.c: update ref_transaction_delete to check for error and return status - refs.c: change ref_transaction_create to do error checking and return status (this branch is used by rs/ref-transaction, rs/ref-transaction-multi, rs/ref-transaction-reflog and rs/ref-transaction-rename.) The second batch of the transactional ref update series. Has been merged into pu rs/ref-transaction (2014-07-17) 12 commits - - refs.c: fix handling of badly named refs - refs.c: make write_ref_sha1 static - fetch.c: change s_update_ref to use a ref transaction - refs.c: propagate any errno==ENOTDIR from _commit back to the callers - refs.c: pass a skip list to name_conflict_fn - refs.c: call lock_ref_sha1_basic directly from commit - refs.c: move the check for valid refname to lock_ref_sha1_basic - refs.c: pass NULL as *flags to read_ref_full - refs.c: pass the ref log message to _create/delete/update instead of _commit - refs.c: add an err argument to delete_ref_loose - wrapper.c: add a new function unlink_or_msg - wrapper.c: simplify warn_if_unremovable (this branch is used by rs/ref-transaction-multi, rs/ref-transaction-reflog and rs/ref-transaction-rename; uses rs/ref-transaction-1.) The third and final part of the basic ref-transaction work. Has been merged into pu. rs/ref-transaction-reflog (2014-07-23) 15 commits --- - refs.c: allow deleting refs with a broken sha1 - refs.c: remove lock_any_ref_for_update - refs.c: make unlock_ref/close_ref/commit_ref static - refs.c: rename log_ref_setup to create_reflog - reflog.c: use a reflog transaction when writing during expire - refs.c: allow multiple reflog updates during a single transaction - refs.c: only write reflog update if msg is non-NULL - refs.c: add a flag to allow reflog updates to truncate the log - refs.c: add a transaction function to append a reflog entry - lockfile.c: make hold_lock_file_for_append preserve meaningful errno - 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: rename the transaction functions - refs.c: make
Everyday contents (was Re: What's cooking in git.git (Jul 2014, #04; Tue, 22))
Continued: this message only covers the third part (out of the four sections). | Integrator[[Integrator]] | | | A fairly central person acting as the integrator in a group | project receives changes made by others, reviews and integrates | them and publishes the result for others to use, using these | commands in addition to the ones needed by participants. This definition of who an integrator is, and it being a separate role when we discuss various workflows, are still sound, I think. | * linkgit:git-pull[1] to merge from your trusted lieutenants. Among these enumerated items, we may want to reword this a little bit to hint that this section also applies to GitHub pull-request workflow. However, I am not sure how their merge without first locally checking action on their website fits in the picture. | Examples | | | My typical Git day.:: This probably shouldn't talk about My in the first place, but in any case I work somewhat differently (cf. howto/maintain-git.txt) these days. | + | | $ git status 1 | $ git show-branch 2 This is more like git branch --no-merged master (and similarly for 'next' and 'pu'), and is helped by Meta/cook -w but this document is a wrong place to talk about the latter. | $ mailx 3 | s 2 3 4 5 ./+to-apply | s 7 8 ./+hold-linus | q | $ git checkout -b topic/one master | $ git am -3 -i -s -u ./+to-apply 4 No need to give -u these days. | $ compile/test | $ git checkout -b hold/linus git am -3 -i -s -u ./+hold-linus 5 Again, no -u necessary. | $ git checkout topic/one git rebase master 6 | $ git checkout pu git reset --hard next 7 | $ git merge topic/one topic/two git merge hold/linus 8 | $ git checkout maint | $ git cherry-pick master~4 9 | $ compile/test | $ git tag -s -m GIT 0.99.9x v0.99.9x 10 | $ git fetch ko git show-branch master maint 'tags/ko-*' 11 This step I still use show-branch, but like this: for branch in master maint next pu do git show-branch ko/$branch $branch done and the purpose explained in the footnote is still valid. | $ git push ko 12 | $ git push ko v0.99.9x 13 I no longer have to do the last step 13, instead the step 12 reads more like git push --follow-tags ko | | + | 1 see what I was in the middle of doing, if any. | 2 see what topic branches I have and think about how ready | they are. With show-branch replaced with branch --no-merged, the purpose of this step is still the same. | 3 read mails, save ones that are applicable, and save others | that are not quite ready. | 4 apply them, interactively, with my sign-offs. | 5 create topic branch as needed and apply, again with my | sign-offs. | 6 rebase internal topic branch that has not been merged to the | master or exposed as a part of a stable branch. | 7 restart `pu` every time from the next. | 8 and bundle topic branches still cooking. | 9 backport a critical fix. | 10 create a signed tag. | 11 make sure I did not accidentally rewind master beyond what I | already pushed out. `ko` shorthand points at the repository I have | at kernel.org, and looks like this: No longer it looks like that ;-) | + | | $ cat .git/remotes/ko | URL: kernel.org:/pub/scm/git/git.git | Pull: master:refs/tags/ko-master | Pull: next:refs/tags/ko-next | Pull: maint:refs/tags/ko-maint | Push: master | Push: next | Push: +pu | Push: maint | | + ... because we encourage people to use in-config description of remotes these days, which should look like this: (in .git/config) [remote ko] url = kernel.org:/pub/scm/git/git.git fetch = refs/heads/*:refs/remotes/ko/* push = refs/heads/master push = refs/heads/next push = +refs/heads/pu push = refs/heads/maint Also tracking is done via refs/remotes/ko/, no longer with tags/. | In the output from `git show-branch`, `master` should have | everything `ko-master` has, and `next` should have | everything `ko-next` has. With s|ko-master|ko/master| and s|ko-next|ko/next|, the above is still valid. | 12 push out the bleeding edge. s/edge./edge, together with new tags that point into my history./ | 13 push the tag out, too. Drop this. -- 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: Amending merge commits?
On Wed, Jul 30, 2014 at 3:42 AM, Sergei Organov o...@javad.com wrote: Nico Williams n...@cryptonector.com writes: Local merge commits mean that you either didn't rebase to keep all your local commits on top of the upstream, or that you have multiple upstreams (the example exception I gave). I rather have multiple (release) branches on single upstream, say, v2.3 and v2.4. When something needs to be fixed in 2.3, it's fixed there and pushed upstream, then, on 2.4, the 2.3 is merged to it, and result is pushed upstream. When I do this merge, I need to push the merge Hmm, why not cherry-pick the fix? That's how every project I know that ports fixes across release branches does it. upstream, and this won't work reliably when --rebase=true is acitve (through pull.merge=rebase). If nothing changes upstream, I can simply push this, and the merge is correctly preserved. However, if somebody makes any changes upstream while I perform the merge, I'll need to pull before pushing, and this immediately flattens-out my merge, that is absolutely not what is needed here. Or I can simply pull before push, just in case, and this flattens history even when there are no any changes upstream! Does this change if you give your merge commits an different commit message? Conversely, if you always rebase your local commits on top of the upstream then you won't have merge commits to worry about. Wrong. I do alwys rebase my local commits on top of upstream, but I still do have my own merge commits to worry about, as explained above. If you cherry-pick the cross-release-branch commits you'll not have a merge commit to worry about. Nico -- -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/5] nd/multiple-work-trees follow-ups
Nguyễn Thái Ngọc Duy pclo...@gmail.com writes: The series has entered 'next' so I can't replace patches any more. Besides the brown paper bag fixes, checkout now rejects if a branch is already checked out elsewhere. I do not think we would want to rush the entire series to 'master' before the upcoming release, so we could squash these fixes into the original when we rewind 'next' post release, if you wanted to. The fix-ups are easier to review than wholesale replacements; keep them coming as needed. 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 3/5] checkout --to: no auto-detach if the ref is already checked out
Michael J Gruber g...@drmicha.warpmail.net writes: As an error message that is completely sufficient. The advice messages are meant to teach the user about the normal parts of the toolchest to use in a situation of conflict, aren't they? Not really. They are to remind (to those who learned but forgot) and to hint (to those who haven't realized they have things to learn in this area). Wall of text that tries to do more than that, like teaching, risks not getting read by anybody. Maybe we should ask someone who hasn't turned them off... Actually, I run without most of the 'advice.*' turned off. -- 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] utf8.c: fix strbuf_utf8_replace copying the last NUL to dst string
Duy Nguyen pclo...@gmail.com writes:
it returns 0 and steps 'src' by one.
Here it refers to utf8_width()? Who steps 'src' by one?
utf8_width() steps 'src'.
Ahh, did you mean *src == NUL, i.e. already at the end of the
string?
Yes.. I guess you have a better commit message prepared for me now :)
Heh. At least you realized that the log message was uninformative ;-)
I think utf8_width() called with an empty string should not move the
pointer past that end-of-string NUL in the first place. It makes me
wonder if it would be a better fix to make it not to do that (and
return 0), but if we declare it is the caller's fault, perhaps we
may want to add
if (!**start)
die(BUG: empty string to utf8_width()???);
at the very beginning of utf8_width(), even before it calls
pick-one-utf8-char.
Still puzzled...
I don't know. I mean, in a UTF-8 byte sequence, NUL is a valid
character (part of the ASCII subset), so advancing '*start' by one
makes sense.
Dubious. NUL may be valid but it is valid only in the sense that it
will mark the end of the string, i.e. the caller is expected to do:
const char *string = ...;
while (not reached the end of the string) {
this_char = pick_one_char(string);
do something to this_char;
}
and there are two ways to structure such a loop:
(1) Make pick-one-char signal the termination condition. i.e.
while ((this_char = pick_one(string)) != EOF)
do something to this_char;
That's a typical getchar()-style loop. It would better not
advance string when it returns EOF.
(2) Make it the responsibility of the caller not to call beyond the
end. i.e.
while (string end) {
this_char = pick_one_char(string);
do something to this char;
}
and your patch takes the latter, which I think is in line with the
way how existing callchain works. So the addition of BUG merely
is to make sure that everybody follows that same convention.
We cannot declare that it is caller's responsibility to feed
sensible input to the function only at one callsite and allow other
callsites feed garbage to the same function, after all, no?
--
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] use a hashmap to make remotes faster
Please don't do this: Content-Type: multipart/related; boundary=MIME delimiter for sendEmail-128858.688128279 -- 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: Amending merge commits?
Nico Williams n...@cryptonector.com writes: On Wed, Jul 30, 2014 at 3:42 AM, Sergei Organov o...@javad.com wrote: Nico Williams n...@cryptonector.com writes: Local merge commits mean that you either didn't rebase to keep all your local commits on top of the upstream, or that you have multiple upstreams (the example exception I gave). I rather have multiple (release) branches on single upstream, say, v2.3 and v2.4. When something needs to be fixed in 2.3, it's fixed there and pushed upstream, then, on 2.4, the 2.3 is merged to it, and result is pushed upstream. When I do this merge, I need to push the merge Hmm, why not cherry-pick the fix? That's how every project I know that ports fixes across release branches does it. Cherry-pick? Why bother? What problem do we solve, having no merges whatsoever? Why? GIT is so good at merges! My impression is that people mostly rather do topic branches, and merge them wherever they need the fixes, no? upstream, and this won't work reliably when --rebase=true is acitve (through pull.merge=rebase). If nothing changes upstream, I can simply push this, and the merge is correctly preserved. However, if somebody makes any changes upstream while I perform the merge, I'll need to pull before pushing, and this immediately flattens-out my merge, that is absolutely not what is needed here. Or I can simply pull before push, just in case, and this flattens history even when there are no any changes upstream! Does this change if you give your merge commits an different commit message? Different from what? I'm almost sure commit message has nothing to do with it. Please refer to this explanation to see for yourself how git behaves when rebasing: http://www.mail-archive.com/git%40vger.kernel.org/msg55605.html Conversely, if you always rebase your local commits on top of the upstream then you won't have merge commits to worry about. Wrong. I do alwys rebase my local commits on top of upstream, but I still do have my own merge commits to worry about, as explained above. If you cherry-pick the cross-release-branch commits you'll not have a merge commit to worry about. I fail to see why do you consider merge commits to be an evil, really. I didn't think about cherry-picking carefully, but I don't feel cherry-picking is the best tool for the job here. I suspect random cherry-picking would create a mess, sooner or later. -- Sergey. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/5] checkout --to: no auto-detach if the ref is already checked out
Junio C Hamano gits...@pobox.com writes: Michael J Gruber g...@drmicha.warpmail.net writes: As an error message that is completely sufficient. The advice messages are meant to teach the user about the normal parts of the toolchest to use in a situation of conflict, aren't they? Not really. They are to remind (to those who learned but forgot) and to hint (to those who haven't realized they have things to learn in this area). Wall of text that tries to do more than that, like teaching, risks not getting read by anybody. Maybe we should ask someone who hasn't turned them off... Actually, I run without most of the 'advice.*' turned off. Ehh, what I meant was that I do not have advice.foo = false for most of 'foo', i.e. I run with most of them still active. -- 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 p4 test: fix failure in 9814-git-p4-rename.sh Was: Re: Test failure in t9814-git-p4-rename.sh - my environment or bad test?
Christoph Bonitz ml.christophbon...@gmail.com writes: Apart from your change and the word wrap adjustments suggested by Pete, would the following also make sense, to fix the other flaw Johannes pointed out? With regards to failing, git diff-tree should be idempotent. I think those are the two occurrences in this file: As a band-aid, that might be OK, but I think these pipelines are unnecessarily and overly wasteful in the first place. All the sed 1d you see here is only because the upstream uses the one-tree form diff-tree options $commit; by comparing two, i.e. diff-tree options $commit^ $commit, they can be dropped. All the cut -f2 is to grab the pathname; we have --name-only these days. I.e. src=$(git diff-tree -r -C --find-copies-harder HEAD | sed 1d | cut -f2) should become src=$(git diff-tree --name-only -r -C --find-copies-harder HEAD^ HEAD) I would think. Extracting C[0-9]* manually with sed is bad, and expecting that the score is within certain range is even worse, because there is no formal guarantee that the definition of similarity indices will not improve in the future. --diff-filter=C to limit the output to only copied paths, without looking at the similarity index, would be more appropriate, e.g. git diff-tree --name-only --diff-filter=C -r -C HEAD^ HEAD or something along those lines. Otherwise, run them outside $(), keep the result in a temporary file, and process the temporary file with the pipeline. That has an added benefit that lets you inspect the file when something goes wrong. I.e. git diff-tree ... diff-tree-out level=$( sed 1d diff-tree-out | cut -f1 | ... ) -- 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/5] refs.c: write updates to packed refs when a transaction has more than one ref
On Tue, Jul 29, 2014 at 2:09 PM, Junio C Hamano gits...@pobox.com wrote:
Ronnie Sahlberg sahlb...@google.com writes:
+ /*
+ * Always copy loose refs that are to be deleted to the packed refs.
+ * If we are updating multiple refs then copy all non symref refs
+ * to the packed refs too.
+ */
for (i = 0; i n; i++) {
struct ref_update *update = updates[i];
unsigned char sha1[20];
+ int flag;
if (update-update_type != UPDATE_SHA1)
continue;
- if (!is_null_sha1(update-new_sha1))
+ if (num_updates 2 !is_null_sha1(update-new_sha1))
continue;
if (get_packed_ref(update-refname))
continue;
if (!resolve_ref_unsafe(update-refname, sha1,
- RESOLVE_REF_READING, NULL))
+ RESOLVE_REF_READING, flag))
+ continue;
+ if (flag REF_ISSYMREF)
continue;
This skipping of symref didn't use to be here.
Is this an enhancement needed to implement the new feature
(i.e. use packed refs to represent multi-update as an atomic
operation)? It looks to me more like an unrelated oops, forgot
that we cannot use packed refs to store symrefs fix-up, unless no
caller passed symref in updates[] in the original code, and now we
have callers that do.
Puzzled...
It was mainly as an enhancement.
Prior to this patch we were only using the packed-refs trick for the
delete+rename operation part of a rename_ref() call.
And for that case we already explicitly test for and disallow symbolic
refs already in rename_ref().
I added this just for extra safety for now that we possibly delete
multiple refs in one transaction, should I special case/disallow the
packed refs trick for symrefs?
Looking at it again I don't think we need this special casing and it
only makes the code more complex and confusing.
I will review the use of this a little and run some tests and if all
looks sane I will remove this ISSYMREF special casing.
Thank!
ronnie sahlberg
--
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/5] refs.c: write updates to packed refs when a transaction has more than one ref
On Tue, Jul 29, 2014 at 2:11 PM, Junio C Hamano gits...@pobox.com wrote: Ronnie Sahlberg sahlb...@google.com writes: + packed = get_packed_refs(ref_cache);; s/;;/;/; ;-) Sorry, I couldn't resist the urge to type many semicolons ;-) Fixed, 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] imap-send: clarify CRAM-MD5 vs LOGIN documentation
Tony Finch d...@dotat.at writes: Explicitly mention that leaving imap.authMethod unset makes git imap-send use the basic IMAP plaintext LOGIN command. --- Documentation/git-imap-send.txt | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/Documentation/git-imap-send.txt b/Documentation/git-imap-send.txt index 875d283..770cbe8 100644 --- a/Documentation/git-imap-send.txt +++ b/Documentation/git-imap-send.txt @@ -76,7 +76,8 @@ imap.preformattedHTML:: imap.authMethod:: Specify authenticate method for authentication with IMAP server. - Current supported method is 'CRAM-MD5' only. + Current supported method is 'CRAM-MD5' only. If this is not set + then 'git imap-send' uses the basic IMAP plaintext LOGIN command. Examples Both patches make sense to me, but can you please sign-off your patches? See Documentation/SubmittingPatches for details. 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 5/6] stash: default listing to --cc --simplify-combined-diff
Junio C Hamano gits...@pobox.com writes:
Jeff King p...@peff.net writes:
When you list stashes, you can provide arbitrary git-log
options to change the display. However, adding just -p
does nothing, because each stash is actually a merge commit.
This implementation detail is easy to forget, leading to
confused users who think -p is not working. We can make
this easier by specifying --cc as a default ourselves
(which does nothing if no diff format is requested by the
user).
Sigh.
git log --cc is one of the things I wanted for a long time to fix.
When the user explicitly asks --cc, we currently ignore it, but
because we know the user wants to view combined diff, we should turn
-p on automatically. And the change this patch introduces will be
broken when we fix log --cc (stash list will end up always
showing the patch, without a way to disable it).
Can you make this conditional? Do this only when options are
given to git stash list command and that includes -p or
something?
Perhaps something along this line?
git-stash.sh | 11 +--
1 file changed, 9 insertions(+), 2 deletions(-)
diff --git a/git-stash.sh b/git-stash.sh
index ae73ba4..0db1b19 100755
--- a/git-stash.sh
+++ b/git-stash.sh
@@ -297,8 +297,15 @@ have_stash () {
list_stash () {
have_stash || return 0
- git log --format=%gd: %gs -g --cc --simplify-combined-diff \
- $@ $ref_stash --
+ case $* in
+ *' --cc '*)
+ ;; # the user knows what she is doing
+ *' -p '* | *' -U'*)
+ set x --cc --simplify-combined-diff $@
+ shift
+ ;;
+ esac
+ git log --format=%gd: %gs -g $@ $ref_stash --
}
show_stash () {
--
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] commit --amend: test specifies authorship but forgets to check
Fabian Ruch baf...@gmail.com writes: The test case --amend option copies authorship specifies that the git-commit option `--amend` uses the authorship of the replaced commit for the new commit. Add the omitted check that this property actually holds. Signed-off-by: Fabian Ruch baf...@gmail.com --- Without the check, the test case succeeds even with nonsense in the `expected` file. An `--amend` implementation which simply uses the committer name and date as if it was not amending would have been deemed correct. This is not the case, the implementation still passes the test suite after the correction. Quickly skimming over the rest of the file, I couldn't find the same thing twice. t/t7509-commit.sh | 1 + 1 file changed, 1 insertion(+) diff --git a/t/t7509-commit.sh b/t/t7509-commit.sh index b61fd3c..9ac7940 100755 --- a/t/t7509-commit.sh +++ b/t/t7509-commit.sh @@ -77,6 +77,7 @@ test_expect_success '--amend option copies authorship' ' git commit -a --amend -m amend test author_header Initial expect author_header HEAD actual + test_cmp expect actual echo amend test expect message_body HEAD actual Makes sense; 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 2/4] refs.c: refactor resolve_ref_unsafe() to use strbuf internally
Eric Sunshine sunsh...@sunshineco.com writes:
char *resolve_refdup(const char *ref, unsigned char *sha1, int reading, int
*flag)
{
- const char *ret = resolve_ref_unsafe(ref, sha1, reading, flag);
- return ret ? xstrdup(ret) : NULL;
+ struct strbuf buf = STRBUF_INIT;
+ if (!resolve_ref(ref, buf, sha1, reading, flag))
+ return buf.buf;
return strbuf_detach(buf, NULL);
Yeah, the end result is the same, but it is a very good discipline.
--
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] remove duplicate entry from 2.1.0 release notes
Signed-off-by: Thomas Ackermann th.ac...@arcor.de --- Documentation/RelNotes/2.1.0.txt | 3 --- 1 file changed, 3 deletions(-) diff --git a/Documentation/RelNotes/2.1.0.txt b/Documentation/RelNotes/2.1.0.txt index be598ad..e958498 100644 --- a/Documentation/RelNotes/2.1.0.txt +++ b/Documentation/RelNotes/2.1.0.txt @@ -137,9 +137,6 @@ Performance, Internal Implementation, etc. introduced; this may reduce I/O cost of rewriting a large index when only small part of the working tree changes. - * Effort to shrink the size of patches Windows folks maintain on top - by upstreaming them continues. - * Patches maintained by msysgit folks for Windows port are being upstreamed here a bit by bit. -- 1.9.4.msysgit.0 --- Thomas -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 5/5] refs.c: rollback the lockfile before we die() in repack_without_refs
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
---
refs.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/refs.c b/refs.c
index 9c813f9..3e98ca1 100644
--- a/refs.c
+++ b/refs.c
@@ -2574,8 +2574,10 @@ int repack_without_refs(const char **refnames, int n,
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.0.1.523.g0041e8a
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 1/5] refs.c: allow passing raw git_committer_info as email to _update_reflog
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
---
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 f11fee3..c6f3a2f 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;
}
@@ -671,7 +678,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 2662ef6..41cb51d 100644
--- a/refs.c
+++ b/refs.c
@@ -3096,7 +3096,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;
@@ -3143,7 +3143,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;
@@ -3491,8 +3491,7 @@ int transaction_update_reflog(struct ref_transaction
*transaction,
const char *refname,
const unsigned char *new_sha1,
const unsigned char *old_sha1,
- const char *email,
- unsigned long timestamp, int tz,
+ struct reflog_committer_info *ci,
const char *msg, int flags,
[PATCH v2 4/5] refs.c: update rename_ref to use a transaction
Change refs.c to use a single transaction to copy/rename both the refs and
its reflog. Since we are no longer using rename() to move the reflog file
we no longer need to disallow rename_ref for refs with a symlink for its
reflog so we can remove that test from the testsuite.
Change the function to return 1 on failure instead of either -1 or 1.
These changes make the rename_ref operation atomic. This also eliminates the
need to use rename() to shift the reflog around via a temporary filename.
As an extension to this, since we no longer use rename() on the reflog file,
we can now safely perform renames even if the reflog is a symbolic link
and thus can remove the check and fail for that condition.
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
---
refs.c| 197 --
t/t3200-branch.sh | 7 --
2 files changed, 71 insertions(+), 133 deletions(-)
diff --git a/refs.c b/refs.c
index 32f48ea..9c813f9 100644
--- a/refs.c
+++ b/refs.c
@@ -2620,82 +2620,52 @@ 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
+struct rename_reflog_cb {
+ struct ref_transaction *transaction;
+ const char *refname;
+ struct strbuf *err;
+};
-static int rename_tmp_log(const char *newrefname)
+static int rename_reflog_ent(unsigned char *osha1, unsigned char *nsha1,
+const char *id, unsigned long timestamp, int tz,
+const char *message, void *cb_data)
{
- int attempts_remaining = 4;
+ struct rename_reflog_cb *cb = cb_data;
+ struct reflog_committer_info ci;
- 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;
+ memset(ci, 0, sizeof(ci));
+ ci.id = id;
+ ci.timestamp = timestamp;
+ ci.tz = tz;
+ return transaction_update_reflog(cb-transaction, cb-refname,
+nsha1, osha1, ci, message, 0,
+cb-err);
}
-static int write_ref_sha1(struct ref_lock *lock, const unsigned char *sha1,
- const char *logmsg);
-
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, log;
+ struct ref_transaction *transaction = NULL;
+ struct strbuf err = STRBUF_INIT;
const char *symref = NULL;
+ struct rename_reflog_cb cb;
+ 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, orig_sha1,
+ symref = resolve_ref_unsafe(oldrefname, sha1,
[PATCH v2 0/5] ref-transactions-rename
This patch series adds support for using transactions and atomic renames. It focuses on what needs to be done in order to support fully atomic and rollbackable renames that may or may not involve name conflicts. By performing the actual delete old/create new via a single operation to the packed refs file this process will also appear as an atomic change for any external observers. (While this series is short it contains a very important change where we start performing ref changes as operations inside a locked packed refs file instead of as discreete operations of loose ref files. This approach will be extended in the next patch series where we will start using it also to make multi-ref creations/updates become fully atomic for external observers.) Version 2: -- Updated with Jonathans suggestions from last week. This series is built on iand depend on the previous reflog series: * rs/ref-transaction-reflog (2014-07-23) 15 commits - refs.c: allow deleting refs with a broken sha1 - refs.c: remove lock_any_ref_for_update - refs.c: make unlock_ref/close_ref/commit_ref static - refs.c: rename log_ref_setup to create_reflog - reflog.c: use a reflog transaction when writing during expire - refs.c: allow multiple reflog updates during a single transaction - refs.c: only write reflog update if msg is non-NULL - refs.c: add a flag to allow reflog updates to truncate the log - refs.c: add a transaction function to append a reflog entry - lockfile.c: make hold_lock_file_for_append preserve meaningful errno - 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: rename the transaction functions - refs.c: make ref_transaction_delete a wrapper for ref_transaction_update - refs.c: make ref_transaction_create a wrapper to ref_transaction_update (this branch uses rs/ref-transaction and rs/ref-transaction-1.) Ronnie Sahlberg (5): 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: update rename_ref to use a transaction refs.c: rollback the lockfile before we die() in repack_without_refs builtin/reflog.c | 19 ++- builtin/remote.c | 14 +- refs.c| 377 ++ refs.h| 25 +++- t/t3200-branch.sh | 7 - 5 files changed, 255 insertions(+), 187 deletions(-) -- 2.0.1.523.g0041e8a -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 2/5] refs.c: return error instead of dying when locking fails during transaction
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
---
refs.c | 7 +--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/refs.c b/refs.c
index 41cb51d..35f6db1 100644
--- a/refs.c
+++ b/refs.c
@@ -2206,6 +2206,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
@@ -2213,8 +2214,10 @@ static struct ref_lock *lock_ref_sha1_basic(const char
*refname,
* again:
*/
goto retry;
- else
- unable_to_lock_index_die(ref_file, errno);
+ else {
+ unable_to_lock_error(ref_file, errno);
+ goto error_return;
+ }
}
if (bad_ref)
return lock;
--
2.0.1.523.g0041e8a
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 3/5] refs.c: use packed refs when deleting refs during a transaction
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 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
---
builtin/remote.c | 14 --
refs.c | 148 +++
2 files changed, 128 insertions(+), 34 deletions(-)
diff --git a/builtin/remote.c b/builtin/remote.c
index be8ebac..bfcc823 100644
--- a/builtin/remote.c
+++ b/builtin/remote.c
@@ -753,6 +753,9 @@ static int remove_branches(struct string_list *branches)
const char **branch_names;
int i, result = 0;
+ if (lock_packed_refs(0))
+ return unable_to_lock_error(git_path(packed-refs), errno);
+
branch_names = xmalloc(branches-nr * sizeof(*branch_names));
for (i = 0; i branches-nr; i++)
branch_names[i] = branches-items[i].string;
@@ -1333,9 +1336,14 @@ static int prune_remote(const char *remote, int dry_run)
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)
- result |= repack_without_refs(delete_refs,
- states.stale.nr, NULL);
+ if (!dry_run) {
+ if (lock_packed_refs(0))
+ result |= unable_to_lock_error(
+ git_path(packed-refs), errno);
+ else
+ result |= repack_without_refs(delete_refs,
+ states.stale.nr, NULL);
+ }
free(delete_refs);
}
diff --git a/refs.c b/refs.c
index 35f6db1..32f48ea 100644
--- a/refs.c
+++ b/refs.c
@@ -2535,6 +2535,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;
@@ -2547,19 +2550,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 exists in packed refs */
-
- if (lock_packed_refs(0)) {
- if (err) {
- unable_to_lock_message(git_path(packed-refs), errno,
- err);
- return -1;
- }
- unable_to_lock_error(git_path(packed-refs), errno);
- return error(cannot delete '%s' from packed refs,
refnames[i]);
}
+
packed = get_packed_refs(ref_cache);
[PATCH v2 2/5] refs.c: write updates to packed refs when a transaction has more than one ref
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
---
builtin/clone.c | 16 +++---
refs.c| 83 ---
t/t5516-fetch-push.sh | 2 +-
3 files changed, 72 insertions(+), 29 deletions(-)
diff --git a/builtin/clone.c b/builtin/clone.c
index f7307e6..7737e12 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -497,17 +497,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 ref_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_sha1(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 ed65f4a..bacce94 100644
--- a/refs.c
+++ b/refs.c
@@ -2543,33 +2543,18 @@ int repack_without_refs(const char **refnames, int n,
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 i, ret;
/* Look for a packed ref */
for (i = 0; i n; i++)
if (get_packed_ref(refnames[i]))
break;
- /* Avoid processing if we have nothing to do */
- if (i == n) {
- rollback_packed_refs();
- return 0; /* no refname exists in packed refs */
- }
-
packed = get_packed_refs(ref_cache);
/* Remove refnames from the cache */
for (i = 0; i n; i++)
- if (remove_entry(packed, refnames[i]) != -1)
- removed = 1;
- if (!removed) {
- /*
-* All packed entries disappeared while we were
-* acquiring the lock.
-*/
- rollback_packed_refs();
- return 0;
- }
+ remove_entry(packed, refnames[i]);
/* Remove any other accumulated cruft */
do_for_each_entry_in_dir(packed, 0, curate_packed_ref_fn,
refs_to_delete);
@@ -3624,6 +3609,7 @@ int transaction_commit(struct ref_transaction
*transaction,
struct strbuf *err)
{
int ret = 0, delnum = 0, i, df_conflict = 0, need_repack = 0;
+ int num_updates = 0;
const char **delnames;
int n = transaction-nr;
struct packed_ref_cache *packed_ref_cache;
@@ -3657,14 +3643,30 @@ int transaction_commit(struct ref_transaction
*transaction,
goto cleanup;
}
- /* any loose refs are to be
[PATCH v2 1/5] refs.c: move reflog updates into its own function
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.
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
---
refs.c | 62 --
1 file changed, 36 insertions(+), 26 deletions(-)
diff --git a/refs.c b/refs.c
index 3e98ca1..ed65f4a 100644
--- a/refs.c
+++ b/refs.c
@@ -2880,6 +2880,39 @@ static 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)) {
+ 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, head_sha1,
+ RESOLVE_REF_READING, 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;
+}
+
/*
* Writes sha1 into the ref specified by the lock. Makes sure that errno
* is sane on error.
@@ -2923,34 +2956,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, head_sha1,
- RESOLVE_REF_READING, 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);
@@ -3659,7 +3668,8 @@ int transaction_commit(struct ref_transaction
*transaction,
continue;
if (get_packed_ref(update-refname))
continue;
- if (!resolve_ref_unsafe(update-refname, sha1, 1, NULL))
+ if (!resolve_ref_unsafe(update-refname, sha1,
+ RESOLVE_REF_READING, NULL))
continue;
add_packed_ref(update-refname, sha1);
--
2.0.1.523.g0041e8a
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 3/5] remote.c: use a transaction for deleting refs
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
---
builtin/remote.c | 69 +++-
1 file changed, 38 insertions(+), 31 deletions(-)
diff --git a/builtin/remote.c b/builtin/remote.c
index bfcc823..3e6ef08 100644
--- a/builtin/remote.c
+++ b/builtin/remote.c
@@ -750,26 +750,27 @@ static int mv(int argc, const char **argv)
static int remove_branches(struct string_list *branches)
{
- const char **branch_names;
int i, result = 0;
+ struct ref_transaction *transaction;
+ struct strbuf err = STRBUF_INIT;
- if (lock_packed_refs(0))
- return unable_to_lock_error(git_path(packed-refs), errno);
+ transaction = transaction_begin(err);
+ if (!transaction)
+ die(%s, err.buf);
- branch_names = xmalloc(branches-nr * sizeof(*branch_names));
for (i = 0; i branches-nr; i++)
- branch_names[i] = branches-items[i].string;
- result |= repack_without_refs(branch_names, branches-nr, NULL);
- free(branch_names);
-
- 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);
- }
+ if (transaction_delete_sha1(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);
+ cleanup:
+ strbuf_release(err);
+ transaction_free(transaction);
return result;
}
@@ -1318,10 +1319,11 @@ 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!);
+ struct ref_transaction *transaction = NULL;
+ struct strbuf err = STRBUF_INIT;
memset(states, 0, sizeof(states));
get_remote_ref_states(remote, states, GET_REF_STATES);
@@ -1332,28 +1334,26 @@ static int prune_remote(const char *remote, int dry_run)
states.remote-url_nr
? 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) {
- if (lock_packed_refs(0))
- result |= unable_to_lock_error(
- git_path(packed-refs), errno);
- else
- result |= repack_without_refs(delete_refs,
- states.stale.nr, NULL);
- }
- free(delete_refs);
}
+ 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;
string_list_insert(delete_refs_list, refname);
- if (!dry_run)
- result |= delete_ref(refname, NULL, 0);
+ if (!dry_run) {
+ if (transaction_delete_sha1(transaction,
+ refname, NULL,
+ 0, 0, remote-branches, err)) {
+ result |= error(%s, err.buf);
+ goto cleanup;
+ }
+ }
if (dry_run)
printf_ln(_( * [would prune] %s),
@@ -1363,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);
string_list_clear(delete_refs_list, 0);
--
2.0.1.523.g0041e8a
--
To unsubscribe from this list: send the line unsubscribe git in
the body
[PATCH v2 0/5] use packed refs for ref-transactions
This is a small patch series that continues ontop of the ref-transactions-rename series I posted earlier today. This series expands the use of the packed refs file to make multi-ref updates much more atomic. Additionally it allows us to continue pushing external caller to use transactions instead of accessing (packed) refs directly which allows us to remove a whole bunch of refs functions from the public api. After this series there is no real need any more to expose that packed refs even exist to external callers. This series is built ontop of the previous series that was posted earlier today as : Version 2: -- Updated to address Junio's concerns Ronnie Sahlberg (5): 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 builtin/clone.c | 16 -- builtin/remote.c | 69 -- refs.c| 155 -- refs.h| 33 --- t/t5516-fetch-push.sh | 2 +- 5 files changed, 151 insertions(+), 124 deletions(-) -- 2.0.1.523.g0041e8a -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 4/5] refs.c: make repack_without_refs static
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
---
refs.c | 2 +-
refs.h | 3 ---
2 files changed, 1 insertion(+), 4 deletions(-)
diff --git a/refs.c b/refs.c
index bacce94..fc445e8 100644
--- a/refs.c
+++ b/refs.c
@@ -2538,7 +2538,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(const char **refnames, int n, struct strbuf *err)
+static int repack_without_refs(const char **refnames, int n, 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 23067a2..93f7bd5 100644
--- a/refs.h
+++ b/refs.h
@@ -155,9 +155,6 @@ extern void rollback_packed_refs(void);
*/
int pack_refs(unsigned int flags);
-extern int repack_without_refs(const char **refnames, int n,
- struct strbuf *err);
-
extern int ref_exists(const char *);
/*
--
2.0.1.523.g0041e8a
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 5/5] refs.c: make the *_packed_refs functions static
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
---
refs.c | 8
refs.h | 30 --
2 files changed, 4 insertions(+), 34 deletions(-)
diff --git a/refs.c b/refs.c
index fc445e8..46a9595 100644
--- a/refs.c
+++ b/refs.c
@@ -1135,7 +1135,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);
@@ -2272,7 +2272,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;
@@ -2295,7 +2295,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);
@@ -2320,7 +2320,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 93f7bd5..54dbe4b 100644
--- a/refs.h
+++ b/refs.h
@@ -112,36 +112,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.0.1.523.g0041e8a
--
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
[ANNOUNCE] Git v2.0.4
The latest maintenance release Git v2.0.4 is now available at the usual places. This is primarily to fix a regression of git diff-tree in v2.0.2. The tarballs are found at: https://www.kernel.org/pub/software/scm/git/ The following public repositories all have a copy of the 'v2.0.4' tag and the 'maint' branch that the tag points at: url = https://kernel.googlesource.com/pub/scm/git/git url = git://repo.or.cz/alt-git.git url = https://code.google.com/p/git-core/ url = git://git.sourceforge.jp/gitroot/git-core/git.git url = git://git-core.git.sourceforge.net/gitroot/git-core/git-core url = https://github.com/gitster/git Git v2.0.4 Release Notes * An earlier update to v2.0.2 broken output from git diff-tree, which is fixed in this release. Changes since v2.0.3 are as follows: Fabian Ruch (1): commit --amend: test specifies authorship but forgets to check Jeff King (8): alloc: write out allocator definitions move setting of object-type to alloc_* functions parse_object_buffer: do not set object type add object_as_type helper for casting objects alloc: factor out commit index object_as_type: set commit index diff-tree: avoid lookup_unknown_object t4013: test diff-tree's --stdin commit formatting Junio C Hamano (1): Git 2.0.4 Ramsay Allan Jones (1): alloc.c: remove the alloc_raw_commit_node() function -- 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 5/6] stash: default listing to --cc --simplify-combined-diff
On Wed, Jul 30, 2014 at 12:43:09PM -0700, Junio C Hamano wrote:
git log --cc is one of the things I wanted for a long time to fix.
When the user explicitly asks --cc, we currently ignore it, but
because we know the user wants to view combined diff, we should turn
-p on automatically. And the change this patch introduces will be
broken when we fix log --cc (stash list will end up always
showing the patch, without a way to disable it).
Can you make this conditional? Do this only when options are
given to git stash list command and that includes -p or
something?
I'm definitely sympathetic. Using --cc with the diff family _does_
imply -p already, so it would be nice to do the same for the log
family.
Perhaps something along this line?
git-stash.sh | 11 +--
1 file changed, 9 insertions(+), 2 deletions(-)
diff --git a/git-stash.sh b/git-stash.sh
index ae73ba4..0db1b19 100755
--- a/git-stash.sh
+++ b/git-stash.sh
@@ -297,8 +297,15 @@ have_stash () {
list_stash () {
have_stash || return 0
- git log --format=%gd: %gs -g --cc --simplify-combined-diff \
- $@ $ref_stash --
+ case $* in
+ *' --cc '*)
+ ;; # the user knows what she is doing
+ *' -p '* | *' -U'*)
+ set x --cc --simplify-combined-diff $@
+ shift
+ ;;
+ esac
+ git log --format=%gd: %gs -g $@ $ref_stash --
Ugh. I'd generally like to avoid this sort of ad-hoc command line
parsing, as it is easy to get confused by option arguments or
even non-options. At least we do not have to worry about pathspecs here,
as we already do an explicit -- (so we might be confused by them, but
they are broken anyway).
Still, I wonder if a cleaner solution is to provide alternate default
to this options for the revision.c option parser. We already have
--default to pick the default starting point. Could we do something
like --default-merge-handling=cc or something?
There's a similar ad-hoc parsing case in stash show, where we add
--stat if no arguments are given, but we have no clue if a diff format
was given (so git stash show --color accidentally turns on patch
format!). Something like --default-diff-format=stat would be more
robust.
I dunno. Maybe it is over-engineering. I just hate to see solutions that
work most of the time and crumble in weird corner cases, even if people
don't hit those corner cases very often.
-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: stash-p broken?
On Tue, Jul 29, 2014 at 12:27:07PM -0700, Junio C Hamano wrote: Jeff King p...@peff.net writes: I think that is my point, though. The user is _not_ aware that the commit in question is a merge. They stashed some stuff, and now they want to see the result. I'd like to show them a vanilla commit if the index is irrelevant, and otherwise show them something more interesting. I actually think git stash list users should be made very aware that they are looking at merge commits, and also what two states each of these merge commits represents. Yeah, I kind of agree, but I also am not optimistic about most users understanding that. I was trying to gamble on the fact that: 1. Naive users who do not understand the index will not stage files and then stash in the first place. To them, the stash would be a simple diff between two states, the commit and the working tree. 2. Advanced users would see the more complicated diff, but only because they were doing more interesting things with the index. They know what the index is, and therefore know that stash must be saving it somehow. Of course that breaks down when the naive user somehow ends up with staged content in the index (e.g., they are resolving a merge and try to stash). Then they are thrust into the more complicated situation either way. I dunno. I suspect that gamble would work _most_ of the time, and makes an OK default. On the other hand, git stash list was completely useless for showing diffs for many years, and since becoming useful has required --cc to show anything good. And this is the first complaint we've seen on the list. Maybe people don't actually care, and educating people to use --cc -p (or --first-parent -p) is fine. -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/PATCH 3/2] stash: show combined diff with stash show
On Tue, Jul 29, 2014 at 11:13:37AM -0700, Junio C Hamano wrote: Jeff King p...@peff.net writes: ... People might be doing things like git stash show | git apply, and would want to ignore the index content ... FWIW, that is exactly how I use git stash show -p most of the time. Like I said, I'm iffy on this part of the series for that reason. But I'm curious: what do you think should happen in such a use case when there are staged contents in the index? Right now we completely ignore them. -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: stash-p broken?
On Tue, Jul 29, 2014 at 11:23:16AM -0700, Junio C Hamano wrote: I see you added --simplify-combined-diffs to avoid breaking log, so that is not too bad, but I am still unsure what should happen when the first parent and the result is the same and only the second parent is different (i.e. you have changes in the index and your working tree changes have been reverted---then you create a stash). Should it show as single source diff, with all changes reverted? Should it show as a normal combined diff for a merge? Should it show as no changes whatsoever? Interesting question. I think right now we would show nothing in the combined diff, as we simply took one side. This is really showing the weakness of the stash is a merge storage format. If we use it as a storage format, and pick it apart in git stash, that's fine. But as soon as we start treating it like a real merge and expecting git log to behave, the cracks begin to show. Hmph. To be honest, I am starting to wonder if implying --first-parent is a more sensible option for stash list. It matches stash show, at least, and it is not unreasonable to simply present the changes in the working tree by default, and ignore the index. People who are more clueful can pick apart the commits using git log themselves. -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/PATCH 3/2] stash: show combined diff with stash show
On Wed, Jul 30, 2014 at 5:17 PM, Jeff King p...@peff.net wrote: Like I said, I'm iffy on this part of the series for that reason. But I'm curious: what do you think should happen in such a use case when there are staged contents in the index? Right now we completely ignore them. I think ignoring is absolutely the right thing ;-) Unlike stash pop, which is try to bring me back to exactly the same state, it is a strong indication that the user wants to further tweak the previous work to grab a patch and apply to the working tree---and while you are working inside the working tree, you haven't started adding the index contents, so git diff after such an operation should show what you grabbed out of the stash, applied to the working tree, relative to what you had in the index. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 4/9] index-helper: new daemon for caching index and related stuff
On Mon, 2014-07-28 at 19:03 +0700, Nguyễn Thái Ngọc Duy wrote: +# Define HAVE_SHM if you platform support shm_* functions in librt. s/you/your/ +static void free_index_shm(struct index_shm *is) Does not actually free its argument; should be release_index_shm. -- 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] Make locked paths absolute when current directory is changed
Hi: 2014-07-23 19:55 GMT+08:00 Duy Nguyen pclo...@gmail.com: On Tue, Jul 22, 2014 at 12:04 AM, Junio C Hamano gits...@pobox.com wrote: Duy Nguyen pclo...@gmail.com writes: [snip] OK, we should center these efforts around the strbuf_getcwd() topic, basing the other topic on realpath() and this one on it then? OK. -- Duy Excuse me. How do I trace these patches applied? I just fetch from https://github.com/gitster/git.git Then tried to find these patches if it is applied. (Seems not.) Then, I took a look at http://git.661346.n2.nabble.com/What-s-cooking-in-git-git-Jul-2014-04-Tue-22-td7615627.html seems no related information there. So, could you please tell me how to trace it? Thank you. ^_^ Yue Lin Ho -- View this message in context: http://git.661346.n2.nabble.com/PATCH-Make-locked-paths-absolute-when-current-directory-is-changed-tp7615398p7616119.html Sent from the git mailing list archive at Nabble.com. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
