Re: bug? git push triggers auto pack when gc.auto = 0
Duy Nguyen pclouds at gmail.com writes: On Tue, Feb 4, 2014 at 12:13 PM, chris jugg at hotmail.com wrote: However, I question why I should even care about this message? I'm going to assume that simply it is a lengthy synchronous operation that someone felt deserved some verbosity to why the client push action is taking longer than it should. Yet that makes me question why I'm being penalized for this server side operation. My client time should not be consumed for server side house keeping. An obvious fix is to disable gc on the server and implement a cron job for the house keeping task. However, as often the case one does not have control over the server, so it is unfortunate that git has this server side house keeping as a blocking operation to a client action. I agree it should not block the client. I think you can Ctrl-C git push at this point without losing anything (data has already been pushed at this point) but that's not a good advice to general cases. Maybe we can do something at the server side to not block the client.. I'd like to avoid a Ctrl-C approach, but if an indication existed that assured me the push part of the operation had completed successfully, then that would be sufficient for when I'm impatient. Another thing we could do is put remote: in front of these strings, even in ssh case. They seem to confuse you (and me too) that things happened locally. Yes, I would like to see more explicit clarity in what messages are coming from the server. That has always been a source of uncertainty for me with any remote git command output. Thanks for the patches and attention to this issue, I appreciate it. Chris -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: bug? git push triggers auto pack when gc.auto = 0
chris j...@hotmail.com writes: Duy Nguyen pclouds at gmail.com writes: On Tue, Feb 4, 2014 at 9:20 AM, chris jugg at hotmail.com wrote: $ git push origin next Counting objects: 56, done. Delta compression using up to 4 threads. Compressing objects: 100% (9/9), done. Writing objects: 100% (9/9), 895 bytes | 0 bytes/s, done. Total 9 (delta 8), reused 0 (delta 0) Auto packing the repository for optimum performance. However, I question why I should even care about this message? I'm going to assume that simply it is a lengthy synchronous operation that someone felt deserved some verbosity to why the client push action is taking longer than it should. Yet that makes me question why I'm being penalized for this server side operation. My client time should not be consumed for server side house keeping. Your client time is not consumed: this is not a busy wait. Git server processes are synchronous: they are initiated and completed under the control of a client. That means that if you run a batch script executing a number of commands in a client, it will not saturate the server with half-finished processes and/or will refuse to honor requests because the repository is locked. An obvious fix is to disable gc on the server and implement a cron job for the house keeping task. However, as often the case one does not have control over the server, so it is unfortunate that git has this server side house keeping as a blocking operation to a client action. _Any_ git operation is blocking the respective initiating client. So my question is, should gc.auto = 0 disable auto-packing from occurring on git push and other non-gc commands? Yes it should. Thanks for the confirmation. And indeed, there is no autopacking occuring on your site when doing git push. The server administrator will be rather glad that the clients' configuration variables don't affect his server's operation. -- David Kastrup -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: bug? git push triggers auto pack when gc.auto = 0
David Kastrup dak at gnu.org writes: chris jugg at hotmail.com writes: Duy Nguyen pclouds at gmail.com writes: On Tue, Feb 4, 2014 at 9:20 AM, chris jugg at hotmail.com wrote: $ git push origin next Counting objects: 56, done. Delta compression using up to 4 threads. Compressing objects: 100% (9/9), done. Writing objects: 100% (9/9), 895 bytes | 0 bytes/s, done. Total 9 (delta 8), reused 0 (delta 0) Auto packing the repository for optimum performance. However, I question why I should even care about this message? I'm going to assume that simply it is a lengthy synchronous operation that someone felt deserved some verbosity to why the client push action is taking longer than it should. Yet that makes me question why I'm being penalized for this server side operation. My client time should not be consumed for server side house keeping. Your client time is not consumed: this is not a busy wait. Git server processes are synchronous: they are initiated and completed under the control of a client. That means that if you run a batch script executing a number of commands in a client, it will not saturate the server with half-finished processes and/or will refuse to honor requests because the repository is locked. I'm slightly confused by your response. You say client time is not consumed, but then go on to say that git server processes are synchronous to avoid build up from batched client requests. I expect you took client time to have some specific technical meaning, while I simply meant that the client command did not return until the server completed its own house keeping. But I do think we are on the same page otherwise in that the client command is blocked until the server process completes. That said I would naively assume that a server side house keeping operation that does not get invoked with every client request be a nice candidate for asynchronous handling without any need to tell the client about it. Regards, Chris -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: bug? git push triggers auto pack when gc.auto = 0
chris j...@hotmail.com writes: David Kastrup dak at gnu.org writes: chris jugg at hotmail.com writes: Duy Nguyen pclouds at gmail.com writes: On Tue, Feb 4, 2014 at 9:20 AM, chris jugg at hotmail.com wrote: $ git push origin next Counting objects: 56, done. Delta compression using up to 4 threads. Compressing objects: 100% (9/9), done. Writing objects: 100% (9/9), 895 bytes | 0 bytes/s, done. Total 9 (delta 8), reused 0 (delta 0) Auto packing the repository for optimum performance. However, I question why I should even care about this message? I'm going to assume that simply it is a lengthy synchronous operation that someone felt deserved some verbosity to why the client push action is taking longer than it should. Yet that makes me question why I'm being penalized for this server side operation. My client time should not be consumed for server side house keeping. Your client time is not consumed: this is not a busy wait. Git server processes are synchronous: they are initiated and completed under the control of a client. That means that if you run a batch script executing a number of commands in a client, it will not saturate the server with half-finished processes and/or will refuse to honor requests because the repository is locked. I'm slightly confused by your response. You say client time is not consumed, but then go on to say that git server processes are synchronous to avoid build up from batched client requests. I expect you took client time to have some specific technical meaning, while I simply meant that the client command did not return until the server completed its own house keeping. Until the server completed the house keeping initiated under the control of the client and on behalf of its command. But I do think we are on the same page otherwise in that the client command is blocked until the server process completes. Sure. That said I would naively assume that a server side house keeping operation that does not get invoked with every client request be a nice candidate for asynchronous handling without any need to tell the client about it. Except that there are _no_ asynchronously handled repository actions executed on behalf of a client action. If the repository owner decided to disable demand-based garbage collection in favor of a cron job, that's his call to make. It makes some sense when there are frequent and multiple accesses to the repository since it avoids getting denied access because of somebody _else_ triggering garbage collection predominantly when times are busiest. Usually you are not denied access by your _own_ garbage collection since the client waits until completion. It would be quite bad for scripting git if you constantly had to check after every action whether any associated garbage collection might or might not have completed. Note also that when pushing without a separate server process (like when pushing into a local repository), there is no other job which could be responsible for packing the repository rather than the one doing the push. -- David Kastrup -- To unsubscribe from this list: send the line 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: relative core.worktree is resolved from symlink and not its target
Hi, when using a submodule sm, there is a relative worktree in its config: .git/modules/sm/config: [core] worktree = ../../../smworktree git-new-worktree (from contrib) symlinks this config the new worktree. From inside the new worktree, git reads the config, but resolves the relative worktree setting based on the symlink's location. A fix would be to resolve any relative worktree setting based on the symlink target's location (the actual config file), and not from the symlink. This is with git version 1.8.5.3. Please consider fixing this. (I know about various workarounds, e.g. copying and adjusting config or manually setting $GIT_WORK_TREE; more relevant discussion would be at http://comments.gmane.org/gmane.comp.version-control.git/196019) Thanks, Daniel. -- http://daniel.hahler.de/ signature.asc Description: OpenPGP digital signature
Re: bug? git push triggers auto pack when gc.auto = 0
David Kastrup dak at gnu.org writes: chris jugg at hotmail.com writes: That said I would naively assume that a server side house keeping operation that does not get invoked with every client request be a nice candidate for asynchronous handling without any need to tell the client about it. Except that there are _no_ asynchronously handled repository actions executed on behalf of a client action. If the repository owner decided to disable demand-based garbage collection in favor of a cron job, that's his call to make. It makes some sense when there are frequent and multiple accesses to the repository since it avoids getting denied access because of somebody _else_ triggering garbage collection predominantly when times are busiest. Usually you are not denied access by your _own_ garbage collection since the client waits until completion. It would be quite bad for scripting git if you constantly had to check after every action whether any associated garbage collection might or might not have completed. I can't comment for every use case, but I find it strange that a client script should need to care whether the server is currently garbage collecting or not. If such a detail must be exposed to a client, then I'd put forth that there is a deeper issue here. But any details there are moving well beyond the scope I'm able to comment on. That said, I think I understand you that it currently does matter in the sense that a client can't perform other actions while garbage collection is running. Note also that when pushing without a separate server process (like when pushing into a local repository), there is no other job which could be responsible for packing the repository rather than the one doing the push. Ok, given your full response, I understand how this is being conceptualized now, thanks. However, if you look at it purely from a user's perspective who is manually invoking these commands for the command's primary purpose, the current behavior is annoying. If we assume Git is right in implementing that no server async actions are executed on behalf of a client action, then this falls under the category of an ill-behaved server in my opinion. Anything a server does that is not directly related to fulfilling the requested client action is now considered bad behavior as it blocks the client from continuing whatever it needs to get on with. I see such implementation in Git as favoring server's needs over clients. Regards, Chris -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: bug? git push triggers auto pack when gc.auto = 0
chris j...@hotmail.com writes: Ok, given your full response, I understand how this is being conceptualized now, thanks. However, if you look at it purely from a user's perspective who is manually invoking these commands for the command's primary purpose, the current behavior is annoying. If we assume Git is right in implementing that no server async actions are executed on behalf of a client action, then this falls under the category of an ill-behaved server in my opinion. Anything a server does that is not directly related to fulfilling the requested client action is now considered bad behavior as it blocks the client from continuing whatever it needs to get on with. I see such implementation in Git as favoring server's needs over clients. There are no server's needs at all. Git only reacts to client requests. It is in the clients' own interest when garbage collection is periodically done since it improves response time. It's arguable that it would be nicer to use an incremental compaction process that hides the periodic costs by distributing them over the request totality. That replaces the periodic why does it have to garbage collect when _I_ am using it annoyance with why is this generally slow. There is no net benefit to that approach safe for a) avoiding complaints of smart people who have discovered that they can speed up git by disabling garbage collection, but eventually find that git is becoming slow for them but not for others. b) avoiding these mailing list discussions. The second benefit could likely be achieved by displaying Server unreachable... retrying... instead of reporting about git gc. -- David Kastrup -- To unsubscribe from this list: send the line 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 v6 5/6] setup: Add 'abspath_part_inside_repo' function
In order to extract the part of an absolute path which lies inside the
repo, it is not possible to directly use real_path, since that would
dereference symlinks both outside and inside the work tree.
Add an 'abspath_part_inside_repo' function which first checks if the
work tree is already the prefix, then incrementally checks each path
level by temporarily NUL-terminating at each '/' and comparing against
the work tree path. If a match is found, it overwrites the input path
with the remainder past the work tree (which will be the part inside the
work tree).
This function is currently only intended for use in
'prefix_path_gently'.
Signed-off-by: Martin Erik Werner martinerikwer...@gmail.com
Reviewed-by: Duy Nguyen pclo...@gmail.com
---
setup.c | 64
1 file changed, 64 insertions(+)
diff --git a/setup.c b/setup.c
index 5432a31..906c8e2 100644
--- a/setup.c
+++ b/setup.c
@@ -6,6 +6,70 @@ static int inside_git_dir = -1;
static int inside_work_tree = -1;
/*
+ * The input parameter must contain an absolute path, and it must already be
+ * normalized.
+ *
+ * Find the part of an absolute path that lies inside the work tree by
+ * dereferencing symlinks outside the work tree, for example:
+ * /dir1/repo/dir2/file (work tree is /dir1/repo) - dir2/file
+ * /dir/file (work tree is /) - dir/file
+ * /dir/symlink1/symlink2 (symlink1 points to work tree) - symlink2
+ * /dir/repolink/file (repolink points to /dir/repo) - file
+ * /dir/repo (exactly equal to work tree) - (empty string)
+ */
+static int abspath_part_inside_repo(char *path)
+{
+ size_t len;
+ size_t wtlen;
+ char *path0;
+ int off;
+
+ const char* work_tree = get_git_work_tree();
+ if (!work_tree)
+ return -1;
+ wtlen = strlen(work_tree);
+ len = strlen(path);
+ off = 0;
+
+ /* check if work tree is already the prefix */
+ if (wtlen = len !strncmp(path, work_tree, wtlen)) {
+ if (path[wtlen] == '/') {
+ memmove(path, path + wtlen + 1, len - wtlen);
+ return 0;
+ } else if (path[wtlen - 1] == '/' || path[wtlen] == '\0') {
+ /* work tree is the root, or the whole path */
+ memmove(path, path + wtlen, len - wtlen + 1);
+ return 0;
+ }
+ /* work tree might match beginning of a symlink to work tree */
+ off = wtlen;
+ }
+ path0 = path;
+ path += offset_1st_component(path) + off;
+
+ /* check each '/'-terminated level */
+ while (*path) {
+ path++;
+ if (*path == '/') {
+ *path = '\0';
+ if (strcmp(real_path(path0), work_tree) == 0) {
+ memmove(path0, path + 1, len - (path - path0));
+ return 0;
+ }
+ *path = '/';
+ }
+ }
+
+ /* check whole path */
+ if (strcmp(real_path(path0), work_tree) == 0) {
+ *path0 = '\0';
+ return 0;
+ }
+
+ return -1;
+}
+
+/*
* Normalize path, prepending the prefix for relative paths. If
* remaining_prefix is not NULL, return the actual prefix still
* remains in the path. For example, prefix = sub1/sub2/ and path is
--
1.8.5.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
[PATCH v6 6/6] setup: Don't dereference in-tree symlinks for absolute paths
The 'prefix_path_gently' function currently applies real_path to
everything if given an absolute path, dereferencing symlinks both
outside and inside the work tree.
This causes most high-level functions to misbehave when acting on
symlinks given via absolute paths. For example
$ git add /dir/repo/symlink
attempts to add the target of the symlink rather than the symlink
itself, which is usually not what the user intends to do.
In order to manipulate symlinks in the work tree using absolute paths,
symlinks should only be dereferenced outside the work tree.
Modify the 'prefix_path_gently' to first normalize the path in order to
make sure path levels are separated by '/', then pass the result to
'abspath_part_inside_repo' to find the part inside the work tree
(without dereferencing any symlinks inside the work tree).
For absolute paths, 'prefix_path_gently' did not, nor does now do, any
actual prefixing, hence the result from 'abspath_part_in_repo' is
returned as-is.
Fixes t0060-82 and t3004-5.
Signed-off-by: Martin Erik Werner martinerikwer...@gmail.com
Reviewed-by: Duy Nguyen pclo...@gmail.com
---
setup.c | 30 ++
t/t0060-path-utils.sh | 2 +-
t/t3004-ls-files-basic.sh | 2 +-
3 files changed, 12 insertions(+), 22 deletions(-)
diff --git a/setup.c b/setup.c
index 906c8e2..ba08885 100644
--- a/setup.c
+++ b/setup.c
@@ -86,11 +86,17 @@ char *prefix_path_gently(const char *prefix, int len,
const char *orig = path;
char *sanitized;
if (is_absolute_path(orig)) {
- const char *temp = real_path(path);
- sanitized = xmalloc(len + strlen(temp) + 1);
- strcpy(sanitized, temp);
+ sanitized = xmalloc(strlen(path) + 1);
if (remaining_prefix)
*remaining_prefix = 0;
+ if (normalize_path_copy_len(sanitized, path, remaining_prefix))
{
+ free(sanitized);
+ return NULL;
+ }
+ if (abspath_part_inside_repo(sanitized)) {
+ free(sanitized);
+ return NULL;
+ }
} else {
sanitized = xmalloc(len + strlen(path) + 1);
if (len)
@@ -98,26 +104,10 @@ char *prefix_path_gently(const char *prefix, int len,
strcpy(sanitized + len, path);
if (remaining_prefix)
*remaining_prefix = len;
- }
- if (normalize_path_copy_len(sanitized, sanitized, remaining_prefix))
- goto error_out;
- if (is_absolute_path(orig)) {
- size_t root_len, len, total;
- const char *work_tree = get_git_work_tree();
- if (!work_tree)
- goto error_out;
- len = strlen(work_tree);
- root_len = offset_1st_component(work_tree);
- total = strlen(sanitized) + 1;
- if (strncmp(sanitized, work_tree, len) ||
- (len root_len sanitized[len] != '\0' sanitized[len]
!= '/')) {
- error_out:
+ if (normalize_path_copy_len(sanitized, sanitized,
remaining_prefix)) {
free(sanitized);
return NULL;
}
- if (sanitized[len] == '/')
- len++;
- memmove(sanitized, sanitized + len, total - len);
}
return sanitized;
}
diff --git a/t/t0060-path-utils.sh b/t/t0060-path-utils.sh
index c0a14f6..f04b82d 100755
--- a/t/t0060-path-utils.sh
+++ b/t/t0060-path-utils.sh
@@ -190,7 +190,7 @@ test_expect_success SYMLINKS 'real path works on symlinks' '
test $sym = $(test-path-utils real_path $dir2/syml)
'
-test_expect_failure SYMLINKS 'prefix_path works with absolute paths to work
tree symlinks' '
+test_expect_success SYMLINKS 'prefix_path works with absolute paths to work
tree symlinks' '
ln -s target symlink
test $(test-path-utils prefix_path prefix $(pwd)/symlink) =
symlink
'
diff --git a/t/t3004-ls-files-basic.sh b/t/t3004-ls-files-basic.sh
index e20c077..9c7adbd 100755
--- a/t/t3004-ls-files-basic.sh
+++ b/t/t3004-ls-files-basic.sh
@@ -36,7 +36,7 @@ test_expect_success 'ls-files -h in corrupt repository' '
test_i18ngrep [Uu]sage: git ls-files broken/usage
'
-test_expect_failure SYMLINKS 'ls-files with absolute paths to symlinks' '
+test_expect_success SYMLINKS 'ls-files with absolute paths to symlinks' '
mkdir subs
ln -s nosuch link
ln -s ../nosuch subs/link
--
1.8.5.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
[PATCH v6 0/6] Handling of in-tree symlinks for absolute paths
The amount of tweaks seemed deserving of a reroll, so here it is:
* Added your test Junio, with what I figured was an appropriate commit
message describing the user-visible effect (to git-add since I think it's the
simplest to explain), the commit message for the second commit has been
reduced somewhat, to not duplicate the message completely.
* Duplicated quite a bit of the explanation from this first commit into
the last commit, in order to be more obvious about the issue it fixes, I'm
not sure if it's a bit too much?
* Reworded the last commit to not mention the full-path special case, and
replaced all occurences of in-repo with inside work tree or similar.
* Content-wise compared to 'pu' I've tweaked a few comments, un-inlined and
added the wtlen = len check (and the ls-files test is new of course):
diff --git a/setup.c b/setup.c
index 32a6f6b..ba08885 100644
--- a/setup.c
+++ b/setup.c
@@ -6,8 +6,8 @@ static int inside_git_dir = -1;
static int inside_work_tree = -1;
/*
- * No checking if the path is in fact an absolute path is done, and it must
- * already be normalized.
+ * The input parameter must contain an absolute path, and it must already be
+ * normalized.
*
* Find the part of an absolute path that lies inside the work tree by
* dereferencing symlinks outside the work tree, for example:
@@ -17,7 +17,7 @@ static int inside_work_tree = -1;
* /dir/repolink/file (repolink points to /dir/repo) - file
* /dir/repo (exactly equal to work tree) - (empty string)
*/
-static inline int abspath_part_inside_repo(char *path)
+static int abspath_part_inside_repo(char *path)
{
size_t len;
size_t wtlen;
@@ -32,7 +32,7 @@ static inline int abspath_part_inside_repo(char *path)
off = 0;
/* check if work tree is already the prefix */
- if (strncmp(path, work_tree, wtlen) == 0) {
+ if (wtlen = len !strncmp(path, work_tree, wtlen)) {
if (path[wtlen] == '/') {
memmove(path, path + wtlen + 1, len - wtlen);
return 0;
@@ -47,7 +47,7 @@ static inline int abspath_part_inside_repo(char *path)
path0 = path;
path += offset_1st_component(path) + off;
- /* check each level */
+ /* check each '/'-terminated level */
while (*path) {
path++;
if (*path == '/') {
Junio C Hamano (1):
t3004: Add test for ls-files on symlinks via absolute paths
Martin Erik Werner (5):
t0060: Add test for prefix_path on symlinks via absolute paths
t0060: Add test for prefix_path when path == work tree
t0060: Add tests for prefix_path when path begins with work tree
setup: Add 'abspath_part_inside_repo' function
setup: Don't dereference in-tree symlinks for absolute paths
setup.c | 94 +--
t/t0060-path-utils.sh | 21 +++
t/t3004-ls-files-basic.sh | 17 +
3 files changed, 112 insertions(+), 20 deletions(-)
--
1.8.5.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
[PATCH v6 1/6] t3004: Add test for ls-files on symlinks via absolute paths
From: Junio C Hamano gits...@pobox.com When symlinks in the working tree are manipulated using the absolute path, git dereferences them, and tries to manipulate the link target instead. This causes most high-level functions to misbehave when acting on symlinks given via absolute paths. For example $ git add /dir/repo/symlink attempts to add the target of the symlink rather than the symlink itself, which is usually not what the user intends to do. This is a regression introduced by 18e051a: setup: translate symlinks in filename when using absolute paths (which did not take symlinks inside the work tree into consideration). Add a known-breakage test using the ls-files function, checking both if the symlink leads to a target in the same directory, and a target in the above directory. Signed-off-by: Martin Erik Werner martinerikwer...@gmail.com Tested-by: Martin Erik Werner martinerikwer...@gmail.com --- t/t3004-ls-files-basic.sh | 17 + 1 file changed, 17 insertions(+) diff --git a/t/t3004-ls-files-basic.sh b/t/t3004-ls-files-basic.sh index 8d9bc3c..e20c077 100755 --- a/t/t3004-ls-files-basic.sh +++ b/t/t3004-ls-files-basic.sh @@ -36,4 +36,21 @@ test_expect_success 'ls-files -h in corrupt repository' ' test_i18ngrep [Uu]sage: git ls-files broken/usage ' +test_expect_failure SYMLINKS 'ls-files with absolute paths to symlinks' ' + mkdir subs + ln -s nosuch link + ln -s ../nosuch subs/link + git add link subs/link + git ls-files -s link subs/link expect + git ls-files -s $(pwd)/link $(pwd)/subs/link actual + test_cmp expect actual + + ( + cd subs + git ls-files -s link ../expect + git ls-files -s $(pwd)/link ../actual + ) + test_cmp expect actual +' + test_done -- 1.8.5.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
[PATCH v6 3/6] t0060: Add test for prefix_path when path == work tree
The current behaviour of prefix_path is to return an empty string if prefixing and absolute path that only contains exactly the work tree. This behaviour is a potential regression point. Signed-off-by: Martin Erik Werner martinerikwer...@gmail.com Reviewed-by: Duy Nguyen pclo...@gmail.com --- t/t0060-path-utils.sh | 6 ++ 1 file changed, 6 insertions(+) diff --git a/t/t0060-path-utils.sh b/t/t0060-path-utils.sh index 0bba988..b8e92e1 100755 --- a/t/t0060-path-utils.sh +++ b/t/t0060-path-utils.sh @@ -195,6 +195,12 @@ test_expect_failure SYMLINKS 'prefix_path works with absolute paths to work tree test $(test-path-utils prefix_path prefix $(pwd)/symlink) = symlink ' +test_expect_success 'prefix_path works with only absolute path to work tree' ' + echo expected + test-path-utils prefix_path prefix $(pwd) actual + test_cmp expected actual +' + relative_path /foo/a/b/c/ /foo/a/b/ c/ relative_path /foo/a/b/c/ /foo/a/bc/ relative_path /foo/a//b//c////foo/a/b//c/ POSIX -- 1.8.5.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
[PATCH v6 4/6] t0060: Add tests for prefix_path when path begins with work tree
One edge-case that isn't currently checked in the tests is the beginning of the path matching the work tree, despite the target not actually being the work tree, for example: path = /dir/repoa work_tree = /dir/repo should fail since the path is outside the repo. However, if /dir/repoa is in fact a symlink that points to /dir/repo, it should instead succeed. Add two tests covering these cases, since they might be potential regression points. Signed-off-by: Martin Erik Werner martinerikwer...@gmail.com Reviewed-by: Duy Nguyen pclo...@gmail.com --- t/t0060-path-utils.sh | 10 ++ 1 file changed, 10 insertions(+) diff --git a/t/t0060-path-utils.sh b/t/t0060-path-utils.sh index b8e92e1..c0a14f6 100755 --- a/t/t0060-path-utils.sh +++ b/t/t0060-path-utils.sh @@ -201,6 +201,16 @@ test_expect_success 'prefix_path works with only absolute path to work tree' ' test_cmp expected actual ' +test_expect_success 'prefix_path rejects absolute path to dir with same beginning as work tree' ' + test_must_fail test-path-utils prefix_path prefix $(pwd)a +' + +test_expect_success 'prefix_path works with absolute path to a symlink to work tree having same beginning as work tree' ' + git init repo + ln -s repo repolink + test a = $(cd repo test-path-utils prefix_path prefix $(pwd)/../repolink/a) +' + relative_path /foo/a/b/c/ /foo/a/b/ c/ relative_path /foo/a/b/c/ /foo/a/bc/ relative_path /foo/a//b//c////foo/a/b//c/ POSIX -- 1.8.5.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
[PATCH v6 2/6] t0060: Add test for prefix_path on symlinks via absolute paths
When symlinks in the working tree are manipulated using the absolute path, git dereferences them, and tries to manipulate the link target instead. This applies to most high-level commands but prefix_path is the common denominator for all of them. Add a known-breakage tests using the prefix_path function, which currently uses real_path, causing the dereference. Signed-off-by: Martin Erik Werner martinerikwer...@gmail.com Reviewed-by: Duy Nguyen pclo...@gmail.com --- t/t0060-path-utils.sh | 5 + 1 file changed, 5 insertions(+) diff --git a/t/t0060-path-utils.sh b/t/t0060-path-utils.sh index 07c10c8..0bba988 100755 --- a/t/t0060-path-utils.sh +++ b/t/t0060-path-utils.sh @@ -190,6 +190,11 @@ test_expect_success SYMLINKS 'real path works on symlinks' ' test $sym = $(test-path-utils real_path $dir2/syml) ' +test_expect_failure SYMLINKS 'prefix_path works with absolute paths to work tree symlinks' ' + ln -s target symlink + test $(test-path-utils prefix_path prefix $(pwd)/symlink) = symlink +' + relative_path /foo/a/b/c/ /foo/a/b/ c/ relative_path /foo/a/b/c/ /foo/a/bc/ relative_path /foo/a//b//c////foo/a/b//c/ POSIX -- 1.8.5.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 1/2] t7101, t7014: rename test files to indicate what that file is for
Nguyễn Thái Ngọc Duy wrote: Signed-off-by: Nguyễn Thái Ngọc Duy pclo...@gmail.com Makes sense. [...] t/t7101-reset-empty-subdirs.sh (new +x) | 63 + t/t7101-reset.sh (gone) | 63 - t/t7104-reset-hard.sh (new +x) | 46 t/t7104-reset.sh (gone) | 46 Hm, summary incorporated in the diffstat. Neat. :) format-patch -M tells me that this indeed just renamed the files, so fwiw Reviewed-by: Jonathan Nieder jrnie...@gmail.com -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 7/8] combine-diff: Fast changed-to-all-parents paths scanning
On Mon, Feb 03, 2014 at 03:39:02PM -0800, Junio C Hamano wrote:
Junio C Hamano gits...@pobox.com writes:
Kirill Smelkov k...@mns.spb.ru writes:
As was recently shown (c839f1bd combine-diff: optimize
combine_diff_path sets intersection), combine-diff runs very slowly. In
that commit we optimized paths sets intersection, but that accounted
only for ~ 25% of the slowness, and as my tracing showed, for linux.git
v3.10..v3.11, for merges a lot of time is spent computing
diff(commit,commit^2) just to only then intersect that huge diff to
almost small set of files from diff(commit,commit^1).
That's because at present, to compute combine-diff, for first finding
paths, that every parent touches, we use the following combine-diff
property/definition:
D(A,P1...Pn) = D(A,P1) ^ ... ^ D(A,Pn) (w.r.t. paths)
where
D(A,P1...Pn) is combined diff between commit A, and parents Pi
and
D(A,Pi) is usual two-tree diff Pi..A
and A ^ B means what???
Silly me; obviously it is the set intersection operator.
We probably could instead use the current set of paths as literal
pathspec to compute subsequent paths, i.e.
D(A,Pi,PS) is two tree diff P1..A limited to paths PS
D(A,P1...Pn) = D(A,P1,[]) ^
D(A,P2,D(A,P1,[])) ^
...
D(A,Pn,D(A,P1...Pn-1))
if we did not want to reinvent the whole tree walking thing, which
would add risks for new bugs and burden to maintain this and the
usual two-tree diff tree walking in sync.
Junio, I understand it is not good to duplicate code and increase
maintenance risks. On the other hand, I don't quite like the approach
with keeping current paths - it could work and be faster compared to
what we had, but to me it is still suboptimal, because if the first diff
D(A,P1) is huge then oops. Also, to implement it rationally, without
delving into unneeded recursion, we would need to do the diffing without
recursion, intersect results, and then recurse into overlapping subtrees,
which means tree-walker rework anyway.
Instead I propose we switch to the new tree walker completely. With the
attached patch which draftly shows it (diff_tree is gone, the work is
done through diff_tree_combined_paths, and then the result is
exported to diff_filepairs) all the tests pass. Also, timings for
git log --raw --no-abbrev --no-renames
( without -c - it would be the same - we are not touching that code, it
would only add, irrelevant-to-the-change constant time )
are
linux.git v3.10..v3.11became 0.1% _faster_
navy.gitbecame 1.4% slower
That means, that the new tree-walker is correct and should be ok
performance-wise (I had not optimized it at all, yet).
What would you say?
Thanks,
Kirill
P.S. Thanks for commenting on other patches. I'll try to correct them
tomorrow, as I have no more time today and need to run.
8
From: Kirill Smelkov k...@mns.spb.ru
Date: Tue, 4 Feb 2014 20:11:16 +0400
Subject: [PATCH] X Show new tree-walker could be used instead of the old one
All tests pass. Timings for git log --raw --no-abrev --no-renames
linux.git v3.10..v3.11became 0.1% _faster_
navy.gitbecame 1.4% slower
---
diff.h | 2 ++
line-log.c | 12 +++--
revision.c | 16 ++-
tree-diff.c | 88 +++--
4 files changed, 90 insertions(+), 28 deletions(-)
diff --git a/diff.h b/diff.h
index 5496560..0a9845a 100644
--- a/diff.h
+++ b/diff.h
@@ -189,8 +189,10 @@ const char *diff_line_prefix(struct diff_options *);
extern const char mime_boundary_leader[];
+#if 0
extern int diff_tree(struct tree_desc *t1, struct tree_desc *t2,
const char *base, struct diff_options *opt);
+#endif
extern int diff_tree_sha1(const unsigned char *old, const unsigned char *new,
const char *base, struct diff_options *opt);
extern int diff_root_tree_sha1(const unsigned char *new, const char *base,
diff --git a/line-log.c b/line-log.c
index 717638b..5739bbf 100644
--- a/line-log.c
+++ b/line-log.c
@@ -766,6 +766,7 @@ void line_log_init(struct rev_info *rev, const char
*prefix, struct string_list
}
}
+#if 0
static void load_tree_desc(struct tree_desc *desc, void **tree,
const unsigned char *sha1)
{
@@ -775,6 +776,7 @@ static void load_tree_desc(struct tree_desc *desc, void
**tree,
die(Unable to read tree (%s), sha1_to_hex(sha1));
init_tree_desc(desc, *tree, size);
}
+#endif
static int count_parents(struct commit *commit)
{
@@ -843,17 +845,23 @@ static void queue_diffs(struct line_log_data *range,
struct commit *commit, struct commit *parent)
{
void *tree1 = NULL, *tree2 = NULL;
- struct tree_desc desc1, desc2;
+// struct tree_desc desc1, desc2;
assert(commit);
git log history simplification problem
Hi, I was trying to understand the history of a piece of code in LibreOffice and I'm facing a behaviour of git-log which is not something I can explain. I'm not sure if this is a git bug or a user error. ;) Here is the situation: git clone git://anongit.freedesktop.org/libreoffice/core cd core git log --full-history -p -S'mnTitleBarHeight =' sd/source/ui/dlg/PaneDockingWindow.cxx Here the first output I get from git-log is b390fae1706b9c511158a03e4fd61f263be4e511, where you can see that the commit *added* that string. So it should be there on master, I would assume. But then I run: git grep 'mnTitleBarHeight =' sd and it's not there. Am I missing something, as in e.g. even with --full-history git-log does some simplification? Thanks, Miklos signature.asc Description: Digital signature
Re: [PATCH v5 4/5] setup: Add 'abspath_part_inside_repo' function
Martin Erik Werner martinerikwer...@gmail.com writes: Will you add that test or should I place it in the series with you as author? Either is fine. Thanks. On Mon, Feb 03, 2014 at 01:00:48PM -0800, Junio C Hamano wrote: Martin Erik Werner martinerikwer...@gmail.com writes: The path being exactly equal to the work tree is handled separately, since then there is no directory separator between the work tree and in-repo part. What is an in-repo part? Whatever it is, I am not sure if I follow that logic. After the while (*path) loop checks each level, you check the whole path---wouldn't that code handle that special case already? Given /dir1/repo/dir2/file I've used 'in-repo' to refer to dir2/file, sometimes interchangably with part inside the work tree which might be better terminology, and should replace it? Yes, inside the working tree is much clearer than inside repo, because the word repository often is used to mean only the stuff inside $GIT_DIR, i.e. what controls the working tree files. I was trying to convey that if path is simply /dir/repo, then the while loop method of replacing a '/' and checking from the beginning won't work for the last level, since it has no terminating '/' to replace, so hence it's a special case, mentioning the part inside the work tree is arguably confusing in that case, since there isn't really one, maybe it should be left out completely, since the check each level explanation covers it already? I dunno about the explanation, but it still looks strange to have the special case to deal with /dir/repo before you enter the while loop, and then also have code immediately after the loop that seems to handle the same case. Isn't the latter one redundant? Because it is probably the normal case not to have any symbolic link in the leading part (hence not having to dereference them), I think checking is work_tree[] a prefix of path[] early is justified from performance point of view, though. /* + * No checking if the path is in fact an absolute path is done, and it must + * already be normalized. This is not quite parsable to me. Hmm, what about The input parameter must contain an absolute path, and it must already be normalized. OK. + const char* work_tree = get_git_work_tree(); + if (!work_tree) + return -1; + wtlen = strlen(work_tree); + len = strlen(path); I expect that this is called from a callsite that _knows_ how long path[] is. Shouldn't len be a parameter to this function instead? Currently, it actually doesn't, since 'normalize_path_copy_len' is called on it prior, which can mess with the string length. OK, strlen() here is perfectly fine, then. Hmph How do our callers treat an empty path? In other words, should these three be equivalent? cd /a git ls-files /a cd /a git ls-files cd /a git ls-files . Here I have only gone by the assumption that previous code did the right thing,... Good to know. And the answer to the above question I think is yes, these should be equivalent. 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/RFC 2/2] receive-pack: hint that the user can stop git push at auto gc time
Nguyễn Thái Ngọc Duy pclo...@gmail.com writes: Housekeeping jobs like auto gc generally should not get in the way. Users who are pushing may not want to wait until auto gc is done on the server. Give a hint for those users that it's safe now to break git push and stop waiting. Signed-off-by: Nguyễn Thái Ngọc Duy pclo...@gmail.com --- This bandage patch may be a good compromise between running auto gc and not annoying users much. If I'm not mistaken, when ^C on git push this way, gc will still be running until it needs to print something out (which it should not normally because of --quiet). The user won't see gc errors, but the user generally can't do much anyway. If you are over local transport, I would think you would kill the both ends. Also, wouldn't killing git push before it is done talking with the receive-pack stop it before it has a chance to update the remote tracking refs to pretend as if it fetched from there immediately after a push? So, no. I do not think we should ever encourage if this bothers you, you can ^C it. Making it not to bother is fine, though. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH/RFC 2/2] receive-pack: hint that the user can stop git push at auto gc time
Junio C Hamano gits...@pobox.com writes: Nguyễn Thái Ngọc Duy pclo...@gmail.com writes: Housekeeping jobs like auto gc generally should not get in the way. Users who are pushing may not want to wait until auto gc is done on the server. Give a hint for those users that it's safe now to break git push and stop waiting. Signed-off-by: Nguyễn Thái Ngọc Duy pclo...@gmail.com --- This bandage patch may be a good compromise between running auto gc and not annoying users much. If I'm not mistaken, when ^C on git push this way, gc will still be running until it needs to print something out (which it should not normally because of --quiet). The user won't see gc errors, but the user generally can't do much anyway. If you are over local transport, I would think you would kill the both ends. Also, wouldn't killing git push before it is done talking with the receive-pack stop it before it has a chance to update the remote tracking refs to pretend as if it fetched from there immediately after a push? So, no. I do not think we should ever encourage if this bothers you, you can ^C it. Making it not to bother is fine, though. Instead of adding a boolean --break-ok that is hidden, why not adding an exposed boolean --daemonize, and let auto-gc run in the background? With the recent do not let more than one gc run at the same time, that should give a lot more pleasant end user experience, 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 v5 4/5] setup: Add 'abspath_part_inside_repo' function
On Tue, 2014-02-04 at 10:09 -0800, Junio C Hamano wrote: Martin Erik Werner martinerikwer...@gmail.com writes: (...) I was trying to convey that if path is simply /dir/repo, then the while loop method of replacing a '/' and checking from the beginning won't work for the last level, since it has no terminating '/' to replace, so hence it's a special case, mentioning the part inside the work tree is arguably confusing in that case, since there isn't really one, maybe it should be left out completely, since the check each level explanation covers it already? I dunno about the explanation, but it still looks strange to have the special case to deal with /dir/repo before you enter the while loop, and then also have code immediately after the loop that seems to handle the same case. Isn't the latter one redundant? The check before the loop doesn't use 'real_path', the one after does: /dir/repo vs /dir/repolink -- Martin Erik Werner martinerikwer...@gmail.com -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 7/8] combine-diff: Fast changed-to-all-parents paths scanning
Kirill Smelkov k...@mns.spb.ru writes: if we did not want to reinvent the whole tree walking thing, which would add risks for new bugs and burden to maintain this and the usual two-tree diff tree walking in sync. Junio, I understand it is not good to duplicate code and increase maintenance risks Instead I propose we switch to the new tree walker completely. I am not fundamentally opposed to the idea. We'll see what happens. -- To unsubscribe from this list: send the line 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-tag.txt: commit for --contains is optional
Nguyễn Thái Ngọc Duy pclo...@gmail.com writes: This goes far back to e84fb2f (branch --contains: default to HEAD - 2008-07-08) where the same parsing code is shared with builtin/tag.c. git-branch.txt correctly states that commit for --contains is optional while git-tag.txt does not. Correct it. Signed-off-by: Nguyễn Thái Ngọc Duy pclo...@gmail.com --- Documentation/git-tag.txt | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/Documentation/git-tag.txt b/Documentation/git-tag.txt index c418c44..404257d 100644 --- a/Documentation/git-tag.txt +++ b/Documentation/git-tag.txt @@ -103,8 +103,9 @@ OPTIONS + This option is only applicable when listing tags without annotation lines. ---contains commit:: - Only list tags which contain the specified commit. +--contains [commit]:: + Only list tags which contain the specified commit (HEAD if not + specified). Thanks. --points-at object:: Only list tags of the given object. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: git log history simplification problem
On Tue, Feb 04, 2014 at 06:37:13PM +0100, Miklos Vajna vmik...@collabora.co.uk wrote: But then I run: git grep 'mnTitleBarHeight =' sd and it's not there. Am I missing something, as in e.g. even with --full-history git-log does some simplification? I tried to reproduce this with a repo from scratch, and it seems my problem is the following: 1) A creates a feature branch 2) A works on it, and in the meantime master progresses as well 3) A merges master to the feature branch 4) A does some additional changes, and -- in an evil way -- uses git commit -a --amend to squeeze these into the merge commit 5) B (that's me) comes and try to find out where a string got deleted, but can't. Here is a reproducer script: rm -rf scratch mkdir scratch cd scratch git init echo -e a\na\na\na\na\na\na\na\n a git add a git commit -m init git branch feature echo b a git add a git commit -m more master changes git checkout feature sed -i '1iXXX' a # insert first row git add a git commit -m feature git merge -m merge master sed -i '1d' a # delete first row git add a git commit --amend -m merge I now know that the XXX got removed by the merge commit, but how can I see it that I'm right? If I run 'git log --all -p' in the result, I see that XXX got inserted by one commit, now I don't have it, but I don't see any deletion, which confuses me. Any ideas? :-) Thanks, Miklos signature.asc Description: Digital signature
Re: [PATCH 2/2] reset: support --mixed --intent-to-add mode
Nguyễn Thái Ngọc Duy pclo...@gmail.com writes:
@@ -128,13 +129,20 @@ static void update_index_from_diff(struct
diff_queue_struct *q,
one-path);
add_cache_entry(ce, ADD_CACHE_OK_TO_ADD |
ADD_CACHE_OK_TO_REPLACE);
+ } else if (*intent_to_add) {
+ int pos = cache_name_pos(one-path, strlen(one-path));
+ if (pos 0)
+ die(_(%s does not exist in index),
+ one-path);
+ set_intent_to_add(the_index, active_cache[pos]);
While I do not have any problem with adding an optional keep lost
paths as intent-to-add entries feature, I am not sure why this has
to be so different from the usual add-cache-entry codepath. The
if/elseif chain you are touching inside this loop does:
- If the tree you are resetting to has something at the path
(which is different from the current index, obviously), create
a cache entry to represent that state from the tree and stuff
it in the index;
- Otherwise, the tree you are resetting to does not have that
path. We used to say remove it from the index, but now we have
an option to instead add it as an intent-to-add entry.
So, why doesn't the new codepath do exactly the same thing as the
first branch of the if/else chain and call add_cache_entry but with
a ce marked with CE_INTENT_TO_ADD? That would parallel what happens
in git add -N better, I would think, 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 v6 5/6] setup: Add 'abspath_part_inside_repo' function
Martin Erik Werner martinerikwer...@gmail.com writes: + const char* work_tree = get_git_work_tree(); Style: asterisk sticks to the variable, not type. No need to resend---all patches looked reasonable. Thanks, will queue. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: git log history simplification problem
Hi, Miklos Vajna wrote: git clone git://anongit.freedesktop.org/libreoffice/core cd core git log --full-history -p -S'mnTitleBarHeight =' sd/source/ui/dlg/PaneDockingWindow.cxx Here the first output I get from git-log is b390fae1706b9c511158a03e4fd61f263be4e511, where you can see that the commit *added* that string. So it should be there on master, I would assume. df76bfb0695d19d201936df80192108e7ce51b8c (a merge) removed it. Plain 'git log' doesn't notice because in the default mode it skips merges. Since the culprit commit is not in the first-parent history of HEAD, my usual approach doesn't help, either: $ git log -m --first-parent -S'mnTitleBarHeight =' \ -- sd/source/ui/dlg/PaneDockingWindow.cxx $ Using -c or --cc produces too many hits. Luckily '-m -p' without --first-parent worked and the first commit it showed was the right one. It produces more hits than I'd like, too, though. The -L option doesn't interact well enough with --reverse to handle this case: $ git grep -p -e'mnTitleBarHeight =' b390fae1 -- sd/source/ui/dlg/PaneDockingWindow.cxx b390fae1:sd/source/ui/dlg/PaneDockingWindow.cxx=void PaneDockingWindow::Layout (void) b390fae1:sd/source/ui/dlg/PaneDockingWindow.cxx:mnTitleBarHeight = GetSettings().GetStyleSettings().GetTitleHeight(); b390fae1:sd/source/ui/dlg/PaneDockingWindow.cxx: mnTitleBarHeight = aToolBoxSize.Height(); b390fae1:sd/source/ui/dlg/PaneDockingWindow.cxx: mnTitleBarHeight = aToolBoxSize.Height(); $ git log --reverse b390fae1..HEAD \ -L:Layout:sd/source/ui/dlg/PaneDockingWindow.cxx fatal: -L parameter 'Layout' starting at line 1: no match Thanks for a useful example. Jonathan -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v6 4/6] t0060: Add tests for prefix_path when path begins with work tree
On 2014-02-04 15.25, Martin Erik Werner wrote: t/t0060-path-utils.sh | 10 ++ 1 file changed, 10 insertions(+) diff --git a/t/t0060-path-utils.sh b/t/t0060-path-utils.sh index b8e92e1..c0a14f6 100755 --- a/t/t0060-path-utils.sh +++ b/t/t0060-path-utils.sh @@ -201,6 +201,16 @@ test_expect_success 'prefix_path works with only absolute path to work tree' ' test_cmp expected actual ' +test_expect_success 'prefix_path rejects absolute path to dir with same beginning as work tree' ' + test_must_fail test-path-utils prefix_path prefix $(pwd)a +' + +test_expect_success 'prefix_path works with absolute path to a symlink to work tree having same beginning as work tree' ' + git init repo + ln -s repo repolink + test a = $(cd repo test-path-utils prefix_path prefix $(pwd)/../repolink/a) +' I think we need SYMLINKS here. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: git log history simplification problem
Hi Jonathan, On Tue, Feb 04, 2014 at 11:48:42AM -0800, Jonathan Nieder jrnie...@gmail.com wrote: Luckily '-m -p' without --first-parent worked and the first commit it showed was the right one. It produces more hits than I'd like, too, though. Ah, excellent! :-) '-m' does what I need. Thanks a lot, Miklos signature.asc Description: Digital signature
[PATCH] blame.c: prepare_lines should not call xrealloc for every line
Making a single preparation run for counting the lines will avoid memory
fragmentation. Also, fix the allocated memory size which was wrong
when sizeof(int *) != sizeof(int), and would have been too small
for sizeof(int *) sizeof(int), admittedly unlikely.
Signed-off-by: David Kastrup d...@gnu.org
---
builtin/blame.c | 40
1 file changed, 24 insertions(+), 16 deletions(-)
diff --git a/builtin/blame.c b/builtin/blame.c
index e44a6bb..522986d 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -1772,25 +1772,33 @@ static int prepare_lines(struct scoreboard *sb)
{
const char *buf = sb-final_buf;
unsigned long len = sb-final_buf_size;
- int num = 0, incomplete = 0, bol = 1;
+ const char *end = buf + len;
+ const char *p;
+ int *lineno;
+
+ int num = 0, incomplete = 0;
+
+ for (p = buf;;) {
+ if ((p = memchr(p, '\n', end-p)) == NULL)
+ break;
+ ++num, ++p;
+ }
- if (len buf[len-1] != '\n')
+ if (len end[-1] != '\n')
incomplete++; /* incomplete line at the end */
- while (len--) {
- if (bol) {
- sb-lineno = xrealloc(sb-lineno,
- sizeof(int *) * (num + 1));
- sb-lineno[num] = buf - sb-final_buf;
- bol = 0;
- }
- if (*buf++ == '\n') {
- num++;
- bol = 1;
- }
+
+ sb-lineno = lineno = xmalloc(sizeof(int) * (num + incomplete + 1));
+
+ for (p = buf;;) {
+ *lineno++ = p-buf;
+ if ((p = memchr(p, '\n', end-p)) == NULL)
+ break;
+ ++p;
}
- sb-lineno = xrealloc(sb-lineno,
- sizeof(int *) * (num + incomplete + 1));
- sb-lineno[num + incomplete] = buf - sb-final_buf;
+
+ if (incomplete)
+ *lineno++ = len;
+
sb-num_lines = num + incomplete;
return sb-num_lines;
}
--
1.8.3.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] blame.c: prepare_lines should not call xrealloc for every line
Whitespace error in line 1778. Should I be reposting? -- David Kastrup -- To unsubscribe from this list: send the line 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 v6 4/6] t0060: Add tests for prefix_path when path begins with work tree
Torsten Bögershausen tbo...@web.de writes: On 2014-02-04 15.25, Martin Erik Werner wrote: t/t0060-path-utils.sh | 10 ++ 1 file changed, 10 insertions(+) diff --git a/t/t0060-path-utils.sh b/t/t0060-path-utils.sh index b8e92e1..c0a14f6 100755 --- a/t/t0060-path-utils.sh +++ b/t/t0060-path-utils.sh @@ -201,6 +201,16 @@ test_expect_success 'prefix_path works with only absolute path to work tree' ' test_cmp expected actual ' +test_expect_success 'prefix_path rejects absolute path to dir with same beginning as work tree' ' +test_must_fail test-path-utils prefix_path prefix $(pwd)a +' + +test_expect_success 'prefix_path works with absolute path to a symlink to work tree having same beginning as work tree' ' +git init repo +ln -s repo repolink +test a = $(cd repo test-path-utils prefix_path prefix $(pwd)/../repolink/a) +' I think we need SYMLINKS here. Yes. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: git log history simplification problem
Miklos Vajna vmik...@collabora.co.uk writes: Hi, I was trying to understand the history of a piece of code in LibreOffice and I'm facing a behaviour of git-log which is not something I can explain. I'm not sure if this is a git bug or a user error. ;) Here is the situation: git clone git://anongit.freedesktop.org/libreoffice/core cd core git log --full-history -p -S'mnTitleBarHeight =' sd/source/ui/dlg/PaneDockingWindow.cxx Lack of -m is what I would first suspect when somebody misunderstands merge simplification. I am not saying that will be the issue, but merely pointing out that that is the first thing that jumps at me when I view the above command line. Here the first output I get from git-log is b390fae1706b9c511158a03e4fd61f263be4e511, where you can see that the commit *added* that string. So it should be there on master, I would assume. But then I run: git grep 'mnTitleBarHeight =' sd and it's not there. Am I missing something, as in e.g. even with --full-history git-log does some simplification? Thanks, Miklos -- To unsubscribe from this list: send the line 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] blame.c: prepare_lines should not call xrealloc for every line
David Kastrup d...@gnu.org writes:
Making a single preparation run for counting the lines will avoid memory
fragmentation. Also, fix the allocated memory size which was wrong
when sizeof(int *) != sizeof(int), and would have been too small
for sizeof(int *) sizeof(int), admittedly unlikely.
Signed-off-by: David Kastrup d...@gnu.org
---
builtin/blame.c | 40
1 file changed, 24 insertions(+), 16 deletions(-)
diff --git a/builtin/blame.c b/builtin/blame.c
index e44a6bb..522986d 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -1772,25 +1772,33 @@ static int prepare_lines(struct scoreboard *sb)
{
const char *buf = sb-final_buf;
unsigned long len = sb-final_buf_size;
- int num = 0, incomplete = 0, bol = 1;
+ const char *end = buf + len;
+ const char *p;
+ int *lineno;
+
+ int num = 0, incomplete = 0;
Is there any significance to the blank line between these two
variable definitions?
+
+ for (p = buf;;) {
+ if ((p = memchr(p, '\n', end-p)) == NULL)
+ break;
+ ++num, ++p;
You have a peculiar style that is somewhat distracting. Why isn't
this more like so?
for (p = buf; p++, num++; ) {
p = memchr(p, '\n', end - p);
if (!p)
break;
}
which I think is the prevalent style in our codebase. The same for
the other loop we see in the new code below.
- favor post-increment unless you use it as rvalue and need
pre-increment;
- SP around each binary ops e.g. 'end - p';
- avoid assignments in conditionals when you do not have to.
+ }
- if (len buf[len-1] != '\n')
+ if (len end[-1] != '\n')
incomplete++; /* incomplete line at the end */
OK, so far we counted num complete lines and incomplete may be
one if there is an incomplete line after them.
- while (len--) {
- if (bol) {
- sb-lineno = xrealloc(sb-lineno,
- sizeof(int *) * (num + 1));
- sb-lineno[num] = buf - sb-final_buf;
- bol = 0;
- }
- if (*buf++ == '\n') {
- num++;
- bol = 1;
- }
+
+ sb-lineno = lineno = xmalloc(sizeof(int) * (num + incomplete + 1));
OK, this function is called only once, so we know sb-lineno is NULL
originally and there is no reason to start from xrealloc().
+ for (p = buf;;) {
+ *lineno++ = p-buf;
+ if ((p = memchr(p, '\n', end-p)) == NULL)
+ break;
+ ++p;
}
- sb-lineno = xrealloc(sb-lineno,
- sizeof(int *) * (num + incomplete + 1));
These really *were* unnecessary reallocations.
Thanks for catching them, but this patch needs heavy style fixes.
- sb-lineno[num + incomplete] = buf - sb-final_buf;
+
+ if (incomplete)
+ *lineno++ = len;
+
sb-num_lines = num + incomplete;
return sb-num_lines;
}
--
To unsubscribe from this list: send the line 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] blame.c: prepare_lines should not call xrealloc for every line
David Kastrup d...@gnu.org writes: Whitespace error in line 1778. Should I be reposting? Heh, let me try to clean it up first and then repost for your review. 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] blame.c: prepare_lines should not call xrealloc for every line
Junio C Hamano gits...@pobox.com writes:
David Kastrup d...@gnu.org writes:
Making a single preparation run for counting the lines will avoid memory
fragmentation. Also, fix the allocated memory size which was wrong
when sizeof(int *) != sizeof(int), and would have been too small
for sizeof(int *) sizeof(int), admittedly unlikely.
Signed-off-by: David Kastrup d...@gnu.org
---
builtin/blame.c | 40
1 file changed, 24 insertions(+), 16 deletions(-)
diff --git a/builtin/blame.c b/builtin/blame.c
index e44a6bb..522986d 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -1772,25 +1772,33 @@ static int prepare_lines(struct scoreboard *sb)
{
const char *buf = sb-final_buf;
unsigned long len = sb-final_buf_size;
-int num = 0, incomplete = 0, bol = 1;
+const char *end = buf + len;
+const char *p;
+int *lineno;
+
+int num = 0, incomplete = 0;
Is there any significance to the blank line between these two
variable definitions?
Well, I needed more than the whitespace error to be motivated for
redoing. Cough, cough.
+
+for (p = buf;;) {
+if ((p = memchr(p, '\n', end-p)) == NULL)
+break;
+++num, ++p;
You have a peculiar style that is somewhat distracting. Why isn't
this more like so?
for (p = buf; p++, num++; ) {
More likely
for (p = buf;; p++, num++)
p = memchr(p, '\n', end - p);
if (!p)
break;
}
which I think is the prevalent style in our codebase. The same for
the other loop we see in the new code below.
I rearranged a few times in order to have both loops be closely
analogous. The second loop would then have to be
for (p = buf;; p++) {
*lineno++ = p-buf;
p = memchr(p, '\n', end-p)
if (!p)
break;
}
Admittedly, that works. I am not too happy about the termination
condition being at the end of the loop but not in the for statement, but
yes, this seems somewhat nicer than what I proposed.
- favor post-increment unless you use it as rvalue and need
pre-increment;
In my youth, the very non-optimizing C compiler I used under CP/M
produced less efficient code for x++ than for ++x even when not using
the resulting expression. Surprisingly habit-forming.
- SP around each binary ops e.g. 'end - p';
Ok.
+}
-if (len buf[len-1] != '\n')
+if (len end[-1] != '\n')
incomplete++; /* incomplete line at the end */
OK, so far we counted num complete lines and incomplete may be
one if there is an incomplete line after them.
That's pretty much the gist of the original code.
-while (len--) {
-if (bol) {
-sb-lineno = xrealloc(sb-lineno,
- sizeof(int *) * (num + 1));
-sb-lineno[num] = buf - sb-final_buf;
-bol = 0;
-}
-if (*buf++ == '\n') {
-num++;
-bol = 1;
-}
+
+sb-lineno = lineno = xmalloc(sizeof(int) * (num + incomplete + 1));
OK, this function is called only once, so we know sb-lineno is NULL
originally and there is no reason to start from xrealloc().
[...]
These really *were* unnecessary reallocations.
Well, if a realloc will increase the allocation size by a constant
factor each time, the amortization cost is O(n) for n entries. So with
a suitable realloc, the effect will not really be noticeable. It still
offends my sense of aesthetics.
Thanks for catching them, but this patch needs heavy style fixes.
Well, does not look all that heavy, but I'll repost.
There is another oversight: I am using memchr here, but there is no
obvious header file definiting it (the respective header will likely be
pulled in indirectly via something unrelated).
Anybody know offhand what I should be including here? It looks like Git
has some fallback definitions of its own, so it's probably not just
string.h I should include?
--
David Kastrup
--
To unsubscribe from this list: send the line 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] blame.c: prepare_lines should not call xrealloc for every line
Junio C Hamano gits...@pobox.com writes:
David Kastrup d...@gnu.org writes:
Whitespace error in line 1778. Should I be reposting?
Heh, let me try to clean it up first and then repost for your
review.
Thanks.
-- 8 --
From: David Kastrup d...@gnu.org
Making a single preparation run for counting the lines will avoid memory
fragmentation. Also, fix the allocated memory size which was wrong
when sizeof(int *) != sizeof(int), and would have been too small
for sizeof(int *) sizeof(int), admittedly unlikely.
Signed-off-by: David Kastrup d...@gnu.org
---
One logic difference from what was posted is that sb-lineno[num]
is filled with the length of the entire buffer when the file ends
with a complete line. I do not remember if the rest of the logic
actually depends on it (I think I use lineno[n+1] - lineno[n] to
find the end of line, so this may matter for the last line),
though.
The original code dates back to 2006 when the author of the code
was not paid for doing anything for Git but was doing it as a
weekend and evening hobby, so it may not be so surprising to find
this kind of what was I thinking when I wrote it inefficiency in
such a code with $0 monetary value ;-)
builtin/blame.c | 37 +
1 file changed, 21 insertions(+), 16 deletions(-)
diff --git a/builtin/blame.c b/builtin/blame.c
index e44a6bb..2364661 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -1772,25 +1772,30 @@ static int prepare_lines(struct scoreboard *sb)
{
const char *buf = sb-final_buf;
unsigned long len = sb-final_buf_size;
- int num = 0, incomplete = 0, bol = 1;
+ const char *end = buf + len;
+ const char *p;
+ int *lineno;
+ int num = 0, incomplete = 0;
+
+ for (p = buf; p end; p++) {
+ p = memchr(p, '\n', end - p);
+ if (!p)
+ break;
+ num++;
+ }
- if (len buf[len-1] != '\n')
+ if (len end[-1] != '\n')
incomplete++; /* incomplete line at the end */
- while (len--) {
- if (bol) {
- sb-lineno = xrealloc(sb-lineno,
- sizeof(int *) * (num + 1));
- sb-lineno[num] = buf - sb-final_buf;
- bol = 0;
- }
- if (*buf++ == '\n') {
- num++;
- bol = 1;
- }
+
+ sb-lineno = lineno = xmalloc(sizeof(int) * (num + incomplete + 1));
+
+ for (p = buf; p end; p++) {
+ *lineno++ = p - buf;
+ p = memchr(p, '\n', end - p);
+ if (!p)
+ break;
}
- sb-lineno = xrealloc(sb-lineno,
- sizeof(int *) * (num + incomplete + 1));
- sb-lineno[num + incomplete] = buf - sb-final_buf;
+ sb-lineno[num + incomplete] = len;
sb-num_lines = num + incomplete;
return sb-num_lines;
}
--
To unsubscribe from this list: send the line 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] blame.c: prepare_lines should not call xrealloc for every line
David Kastrup d...@gnu.org writes: Anybody know offhand what I should be including here? It looks like Git has some fallback definitions of its own, so it's probably not just string.h I should include? In general, no *.c files outside the compatibility layer should include anything #include system-header.h, as there seem to be finicky systems that care about order of inclusion and feature macro definitions, all of which are meant to be handled by including git-compat-util.h as the very first thing. -- To unsubscribe from this list: send the line 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] howto/maintain-git.txt: new version numbering scheme
From: Junio C Hamano gits...@pobox.com Philip Oakley philipoak...@iee.org writes: If we are progressing from V1.9 to V2.0 quickly (one cycle?), which I understand is the plan, then mixing the minor development items (patch series which progress to master) with the maintenance fixes over the next few months, thus only having 1.9.x releases, sounds reasonable. If there is going to be separate maintenance fixes from the patch series developments then keeping to the previous 1.9.x.y for maintenance would be better. Will the new rapid counting continue after V2.0, such that we get to V2.9 - V3.0 rather more quickly than V1.0 - V2.0 ? The key discriminator would be to say when V2.0 will be out for deciding the V1.9 sequence. I do not quite follow. The time distance between v1.9 and v2.0 should not affect anything. If it is a long road, there may be v1.10, v1.11, v1.12, ... I wasn't sure if you were considering going past either 1.9.9 to 1.9.10, and going from 1.9 to 1.10 was something that hadn't occurred to me (somewhat of a Doh! moment maybe). before we have v2.0. If not, v2.0 may immediately follow v1.9 as a new feature release. There may be maintenance releases based on v1.9 that does not add any new features. Right now, if you count the maintenance releases, there are potentially four kinds of version gaps: - Between v1.8.5 and v1.8.5.1, there are fixes but no new features; - Between v1.8.5 and v1.8.6, there are new features but no compatibility worries; - Between v1.8.6 and v1.9.0, there are new features, no compatibility worries, but somehow the jump is larger than the one between v1.8.5 and v1.8.6; and - Between v1.9.0 and v2.0.0, there are new features and also compatibility concerns. Switching to 2-digit scheme and calling the upcoming one v1.9 (and the next major one v2.0) was meant to make the naming more flat, OK I'd buy that flattening approach. as the third item in the above list somehow the jump is larger does not seem to add much value to the end users. So the logical numbering becomes more like this: - Between v1.9 and v1.9.1, there are fixes but no new features; - Between v1.9.x and v1.10, there are new features but no compatibility worries; - Between v1.9.x and v2.0, there are new features and also compatibility concerns. With a twist, though. There seem to be many places where at least three digits are assumed to exist in our version numbers, so in order to make life easier, the updated document says vX.Y (a feature release) will identify itself as vX.Y.0 Yes. I'd be happy to support that third 'digit' for the maint releases, with zero as the initial release. Git Gui has a version string checking routine but its regex only needs two parts X.Y (we looked into the version string back in $gmane/217189 Thanks for the clarifications. Philip. -- To unsubscribe from this list: send the line 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] blame.c: prepare_lines should not call xrealloc for every line
Junio C Hamano gits...@pobox.com writes: Junio C Hamano gits...@pobox.com writes: David Kastrup d...@gnu.org writes: Whitespace error in line 1778. Should I be reposting? Heh, let me try to clean it up first and then repost for your review. Thanks. -- 8 -- From: David Kastrup d...@gnu.org Making a single preparation run for counting the lines will avoid memory fragmentation. Also, fix the allocated memory size which was wrong when sizeof(int *) != sizeof(int), and would have been too small for sizeof(int *) sizeof(int), admittedly unlikely. Signed-off-by: David Kastrup d...@gnu.org --- One logic difference from what was posted is that sb-lineno[num] is filled with the length of the entire buffer when the file ends with a complete line. Where's the difference? This is exactly what will happen with my code as well. I _do_ rely on memchr(whatever, '\n', 0) to return NULL without looking at any memory for that. If there is a fear of memchr not being able to deal with a count of 0, this code needs to be somewhat more complex. I do not remember if the rest of the logic actually depends on it (I think I use lineno[n+1] - lineno[n] to find the end of line, Well, you do it about _half_ the time. The other half, you scan for the '\n' explicitly. The original code dates back to 2006 when the author of the code was not paid for doing anything for Git but was doing it as a weekend and evening hobby, so it may not be so surprising to find this kind of what was I thinking when I wrote it inefficiency in such a code with $0 monetary value ;-) Oh, _this_ patch is not in the I want to make money from it range. If that were the case, I should not have bothered at all. This is just the this code offends my sense of aesthetics class. It's purely optional to apply. It's conceivable that it will make a performance difference on non-glibc (or what it's called) platforms. -- David Kastrup -- To unsubscribe from this list: send the line 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] blame.c: prepare_lines should not call xrealloc for every line
Junio C Hamano gits...@pobox.com writes: David Kastrup d...@gnu.org writes: Anybody know offhand what I should be including here? It looks like Git has some fallback definitions of its own, so it's probably not just string.h I should include? In general, no *.c files outside the compatibility layer should include anything #include system-header.h, as there seem to be finicky systems that care about order of inclusion and feature macro definitions, all of which are meant to be handled by including git-compat-util.h as the very first thing. Ok, and that one's not yet in blame.c. Will include, thanks. -- David Kastrup -- To unsubscribe from this list: send the line 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] blame.c: prepare_lines should not call xrealloc for every line
Junio C Hamano gits...@pobox.com writes:
David Kastrup d...@gnu.org writes:
Making a single preparation run for counting the lines will avoid memory
fragmentation. Also, fix the allocated memory size which was wrong
when sizeof(int *) != sizeof(int), and would have been too small
for sizeof(int *) sizeof(int), admittedly unlikely.
Signed-off-by: David Kastrup d...@gnu.org
---
builtin/blame.c | 40
1 file changed, 24 insertions(+), 16 deletions(-)
diff --git a/builtin/blame.c b/builtin/blame.c
index e44a6bb..522986d 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -1772,25 +1772,33 @@ static int prepare_lines(struct scoreboard *sb)
{
const char *buf = sb-final_buf;
unsigned long len = sb-final_buf_size;
-int num = 0, incomplete = 0, bol = 1;
+const char *end = buf + len;
+const char *p;
+int *lineno;
+
+int num = 0, incomplete = 0;
Is there any significance to the blank line between these two
variable definitions?
+
+for (p = buf;;) {
+if ((p = memchr(p, '\n', end-p)) == NULL)
+break;
+++num, ++p;
You have a peculiar style that is somewhat distracting. Why isn't
this more like so?
for (p = buf; p++, num++; ) {
p = memchr(p, '\n', end - p);
if (!p)
break;
}
Ok, I now wrote
for (p = buf;; num++, p++) {
p = memchr(p, '\n', end - p);
if (!p)
break;
}
and you know what? Its logic seems wrong. The reason is that the p++
does not really have anything to do with the iteration, but rather steps
past the '\n' from the memchr. So it's more like
for (p = buf;; num++) {
p = memchr(p, '\n', end - p);
if (p) {
p++;
continue;
}
break;
}
So barring protests, that's what I'm going to use instead.
--
David Kastrup
--
To unsubscribe from this list: send the line 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] blame.c: prepare_lines should not call xrealloc for every line
Making a single preparation run for counting the lines will avoid memory
fragmentation. Also, fix the allocated memory size which was wrong
when sizeof(int *) != sizeof(int), and would have been too small
for sizeof(int *) sizeof(int), admittedly unlikely.
Signed-off-by: David Kastrup d...@gnu.org
---
builtin/blame.c | 44 +---
1 file changed, 29 insertions(+), 15 deletions(-)
diff --git a/builtin/blame.c b/builtin/blame.c
index e44a6bb..c422584 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -4,6 +4,7 @@
* Copyright (c) 2006, Junio C Hamano
*/
+#include git-compat-util.h
#include cache.h
#include builtin.h
#include blob.h
@@ -1772,25 +1773,38 @@ static int prepare_lines(struct scoreboard *sb)
{
const char *buf = sb-final_buf;
unsigned long len = sb-final_buf_size;
- int num = 0, incomplete = 0, bol = 1;
+ const char *end = buf + len;
+ const char *p;
+ int *lineno;
+ int num = 0, incomplete = 0;
- if (len buf[len-1] != '\n')
- incomplete++; /* incomplete line at the end */
- while (len--) {
- if (bol) {
- sb-lineno = xrealloc(sb-lineno,
- sizeof(int *) * (num + 1));
- sb-lineno[num] = buf - sb-final_buf;
- bol = 0;
+ for (p = buf;; num++) {
+ p = memchr(p, '\n', end - p);
+ if (p) {
+ p++;
+ continue;
}
- if (*buf++ == '\n') {
- num++;
- bol = 1;
+ break;
+ }
+
+ if (len end[-1] != '\n')
+ incomplete++; /* incomplete line at the end */
+
+ sb-lineno = lineno = xmalloc(sizeof(int) * (num + incomplete + 1));
+
+ for (p = buf;;) {
+ *lineno++ = p - buf;
+ p = memchr(p, '\n', end - p);
+ if (p) {
+ p++;
+ continue;
}
+ break;
}
- sb-lineno = xrealloc(sb-lineno,
- sizeof(int *) * (num + incomplete + 1));
- sb-lineno[num + incomplete] = buf - sb-final_buf;
+
+ if (incomplete)
+ *lineno++ = len;
+
sb-num_lines = num + incomplete;
return sb-num_lines;
}
--
1.8.3.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] blame.c: prepare_lines should not call xrealloc for every line
David Kastrup d...@gnu.org writes: Junio C Hamano gits...@pobox.com writes: David Kastrup d...@gnu.org writes: Anybody know offhand what I should be including here? It looks like Git has some fallback definitions of its own, so it's probably not just string.h I should include? In general, no *.c files outside the compatibility layer should include anything #include system-header.h, as there seem to be finicky systems that care about order of inclusion and feature macro definitions, all of which are meant to be handled by including git-compat-util.h as the very first thing. Ok, and that one's not yet in blame.c. Will include, thanks. No, don't. Some well-known *.h files of ourse, most notably cache.h, are known to include compat-util as the first thing, and that is what *.c files typically include. -- To unsubscribe from this list: send the line 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] blame.c: prepare_lines should not call xrealloc for every line
David Kastrup d...@gnu.org writes:
Ok, I now wrote
for (p = buf;; num++, p++) {
p = memchr(p, '\n', end - p);
if (!p)
break;
}
Looks still wrong (perhaps this is a taste issue).
num++ is not loop control, but the real action of this
loop to count lines. It is better left inside.
p++ is loop control, and belongs to the third part of
for(;;).
Isn't the normal continuation condition p end?
so something like
for (p = buf; p end; p++) {
p = find the end of this line
if (!p)
break;
num++;
}
perhaps?
--
To unsubscribe from this list: send the line 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] blame.c: prepare_lines should not call xrealloc for every line
Making a single preparation run for counting the lines will avoid memory
fragmentation. Also, fix the allocated memory size which was wrong
when sizeof(int *) != sizeof(int), and would have been too small
for sizeof(int *) sizeof(int), admittedly unlikely.
Signed-off-by: David Kastrup d...@gnu.org
---
Now without including git-compat-util.h (included by cache.h already).
builtin/blame.c | 43 ---
1 file changed, 28 insertions(+), 15 deletions(-)
diff --git a/builtin/blame.c b/builtin/blame.c
index e44a6bb..59d62dc 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -1772,25 +1772,38 @@ static int prepare_lines(struct scoreboard *sb)
{
const char *buf = sb-final_buf;
unsigned long len = sb-final_buf_size;
- int num = 0, incomplete = 0, bol = 1;
+ const char *end = buf + len;
+ const char *p;
+ int *lineno;
+ int num = 0, incomplete = 0;
- if (len buf[len-1] != '\n')
- incomplete++; /* incomplete line at the end */
- while (len--) {
- if (bol) {
- sb-lineno = xrealloc(sb-lineno,
- sizeof(int *) * (num + 1));
- sb-lineno[num] = buf - sb-final_buf;
- bol = 0;
+ for (p = buf;; num++) {
+ p = memchr(p, '\n', end - p);
+ if (p) {
+ p++;
+ continue;
}
- if (*buf++ == '\n') {
- num++;
- bol = 1;
+ break;
+ }
+
+ if (len end[-1] != '\n')
+ incomplete++; /* incomplete line at the end */
+
+ sb-lineno = lineno = xmalloc(sizeof(int) * (num + incomplete + 1));
+
+ for (p = buf;;) {
+ *lineno++ = p - buf;
+ p = memchr(p, '\n', end - p);
+ if (p) {
+ p++;
+ continue;
}
+ break;
}
- sb-lineno = xrealloc(sb-lineno,
- sizeof(int *) * (num + incomplete + 1));
- sb-lineno[num + incomplete] = buf - sb-final_buf;
+
+ if (incomplete)
+ *lineno++ = len;
+
sb-num_lines = num + incomplete;
return sb-num_lines;
}
--
1.8.3.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] blame.c: prepare_lines should not call xrealloc for every line
Junio C Hamano gits...@pobox.com writes:
David Kastrup d...@gnu.org writes:
Ok, I now wrote
for (p = buf;; num++, p++) {
p = memchr(p, '\n', end - p);
if (!p)
break;
}
Looks still wrong (perhaps this is a taste issue).
num++ is not loop control, but the real action of this
loop to count lines. It is better left inside.
Ok.
p++ is loop control, and belongs to the third part of
for(;;).
No, it isn't. The real loop control is the p = memchr line. p++ only
skips over the newline.
Isn't the normal continuation condition p end?
memchr returns NULL when not finding anything any more.
so something like
for (p = buf; p end; p++) {
p = find the end of this line
if (!p)
break;
num++;
}
perhaps?
Would crash on incomplete last line.
--
David Kastrup
--
To unsubscribe from this list: send the line 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] blame.c: prepare_lines should not call xrealloc for every line
David Kastrup d...@gnu.org writes:
so something like
for (p = buf; p end; p++) {
p = find the end of this line
if (!p)
break;
num++;
}
perhaps?
Would crash on incomplete last line.
Hmph, even with if !p? From your other message:
+ for (p = buf;; num++) {
+ p = memchr(p, '\n', end - p);
+ if (p) {
+ p++;
+ continue;
}
+ break;
+ }
If you flip the if statement around that would be the same as:
+ for (p = buf;; num++) {
+ p = memchr(p, '\n', end - p);
+ if (!p)
break;
p++;
+ }
And with loop action not in the control part, that would be the
same as:
for (p = buf; ; p++) {
p = memchr...
if (!p)
break;
num++;
}
no? Would this crash on incomplete last line?
Ahh, p end in for (p = buf; p end; p++) { is not correct;
you depend on overrunning the end of the buffer to feed length 0 to
memchr and returning NULL inside the loop. So to be equivalent to
your version, the contination condition needs to be
for (p = buf; p = end; p++) {
But that last round of the loop will be no-op, so p end vs p =
end does not make any difference. It is not even strictly
necessary because memchr() limits the scan to end, but it would
still be a good belt-and-suspenders defensive coding practice, I
would think.
+ for (p = buf;;) {
+ *lineno++ = p - buf;
+ p = memchr(p, '\n', end - p);
+ if (p) {
+ p++;
+ continue;
}
+ break;
}
Can we do the same transformation here?
for (p = buf;;) {
*lineno++ = p - buf;
p = memchr...
if (!p)
break;
p++;
}
which is the same as
for (p = buf; ; p++) {
*lineno++ = p - buf;
p = memchr...
if (!p)
break;
}
and the latter has the loop control p++ at where it belongs to. The
post-condition of one iteration of the body of the loop being p
points at the terminating LF of this line, incrementing the pointer
is how the loop control advances the pointer to the beginning of the
next line.
If we wanted to have a belt-and-suspenders safety, we need p =
end here, not p end, because of how p is used when it is past
the last LF. As we want to make these two loops look similar, that
means we should use p = end in the previous loop as well.
This was fun ;-)
--
To unsubscribe from this list: send the line 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/3] setup_pager: set MORE=R
On Tue, Jan 21, 2014 at 11:23:30AM -0800, Junio C Hamano wrote: does complicate the point of my series, which was to add more intimate logic about how we handle LESS. ... return !x || strchr(x, 'R'); [...] I am not sure if it is even a good idea for us to have so intimate logic for various pagers in the first place. I'd seriously wonder if it is better to take this position: A platform packager who sets the default pager and/or the default environment for the pager at the build time, or an individual user who tells your Git what pager you want to use and/or with what setting that pager should be run under with environment variables. These people ought to know far better than Git what their specific choices do. Do not try to second-guess them. Sorry for letting this topic lapse; I wanted to look at some possible Makefile improvements, which I'll be posting in a moment. I did want to address this point, though. If we are just talking about packagers and defaults (e.g., setting MORE=R on FreeBSD), then I agree that the right tool for the job is empowering the packagers, and the Makefile knob we've discussed does that. But there's another set of people that I was intending to help with the patch, which is people that have set up LESS, and did not necessarily care about the R flag in the past (e.g., for many years before git came along, I set LESS=giM, and never even knew that R existed). Since git comes out of the box these days with color and the pager turned on, that means people with such a setup see broken output from day one. And I think it is Git's fault here, not the user or the packager. Our defaults assume that the user's pager can handle color, and that is not the default for many pagers, including our default less! The user did not turn off R here; they simply set other options they cared about, and our hacky workaround to auto-enable R did not kick in. It seems a shame to me that we cannot do better for such users. However, the level of intimacy with the pager is getting to be a bit too much for my taste, and the saving grace is that not that many people set LESS themselves (and git is widely enough known that R is a common recommendation when people do). So it's probably the lesser of two evils to ignore the situation, and let people who run into it find the answers on the web. So I think there is nothing to be done. But I did want to mention it in case somebody else can come up with some clever solution. :) -Peff -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/3] setup_pager: set MORE=R
Jeff King p...@peff.net writes: But there's another set of people that I was intending to help with the patch, which is people that have set up LESS, and did not necessarily care about the R flag in the past (e.g., for many years before git came along, I set LESS=giM, and never even knew that R existed). Since git comes out of the box these days with color and the pager turned on, that means people with such a setup see broken output from day one. And I think it is Git's fault here, not the user or the packager. I am not particularly itnterested in whose fault it is ;-) If the user sets LESS himself, he knows how to set it (and if he is setting it automatically for all of his sessions, he knows where to do so), and would know better than Git about less, his pager of choice. If he did not know about R and did not see color, that is even better. Now he knows and his update to his LESS settings will let him view colors in outputs from programs that are not Git. So I think there is nothing to be done. But I did want to mention it in case somebody else can come up with some clever solution. :) Sure. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 0/9] remerge diff proof of concept/RFC
Hi,
This may look intimidating, but it's actually 3.5 separate things:
merge-recursive: remove dead conditional in update_stages()
merge-recursive: internal flag to avoid touching the worktree
merge-recursive: -Xindex-only to leave worktree unchanged
These are unchanged from tr/merge-recursive-index-only. I'm just
resending them here because it's been a while, the rest depends on it,
and it makes it more obvious how patch 8 fits in.
pretty: refactor add_merge_info() into parts
log: add a merge base inspection option
This seemed a good idea when I wrote it, but the more I think about
it, the less useful it appears to me. I left it here as a
demonstration that computing the merge bases as we go is feasible.
combine-diff: do not pass revs-dense_combined_merges redundantly
Fold all merge diff variants into an enum
Some initial refactoring.
merge-recursive: allow storing conflict hunks in index
log --remerge-diff: show what the conflict resolution changed
The real meat.
I'll only describe the last patches here.
Context: last summer (yes, it's been a while) I investigated some
angles of looking at merges that make it easier to investigate
mismerges. The problem space is: it is exceedingly hard to find
instances of
git merge -s ours foo
git merge -Xours foo
manually resolve some hunks in favor of one side
after some time has passed. The only ways I found with existing
machinery involve too much manual inspection.
At the time I made some scripts that look at the problem in various
ways:
https://github.com/trast/evilmergediff
This series explores another angle, which I call remerge diff. It
works by re-doing the merge in core, using features from patches 1-3
and 7. Likely that will result in conflicts, which are formatted in
the usual way. Then it diffs this remerge against the
merge's tree that is recorded in history.
This tends to give nice results in practice; I pasted a particularly
pretty example at the bottom.
However, this is RFC because nontrivial merges can easily lead to
states that I cannot handle with the current -- pretty neat and
simple, I think -- implementation.
For example, a delete/modify conflict does not result in a nice
conflict-hunk formatted file, it just leaves 2 (out of 3) stages in
the index. I can then not write a tree from that.
This could be fixed by adding a pass that turns a missing stage 2 or 3
into an empty file at that stage, so that the resulting file looks
like
===
... stage 3 content goes here ...
(analogously for stage 3 missing).
But that still doesn't handle at least directory/file conflicts.
So I would welcome comments, and/or better ideas, on the following
proposed resolution:
* Implement what I described last, to take care of delete/modify
conflicts.
* Punt on more complex conflicts, by removing those files from the
index, and emitting a warning about those files instead.
Note that even without delete/modify handling, this covers the vast
majority of merges: among some 8090 merges I sampled from git.git,
there are only 13 that trigger the BUG: ... message, and 34
octopuses.
Example run:
$ git show --remerge-diff a39b15b4f6a3f08b67b17d968935d177821e680f
[...]
diff --git a/builtin/ls-files.c b/builtin/ls-files.c
index 0f4473e..175e6e3 100644
--- a/builtin/ls-files.c
+++ b/builtin/ls-files.c
@@ -490,16 +490,9 @@ int cmd_ls_files(int argc, const char **argv, const char
*cmd_prefix)
OPT_BOOLEAN('u', unmerged, show_unmerged,
N_(show unmerged files in the output)),
OPT_BOOLEAN(0, resolve-undo, show_resolve_undo,
- f12e49ae877ad0644b9b9939b7cb742da98691d2
N_(show resolve-undo information)),
- { OPTION_CALLBACK, 'x', exclude, dir.exclude_list[EXC_CMDL],
N_(pattern),
+ { OPTION_CALLBACK, 'x', exclude, exclude_list, N_(pattern),
N_(skip files matching pattern),
-===
- show resolve-undo information),
- { OPTION_CALLBACK, 'x', exclude,
- exclude_list, pattern,
- skip files matching pattern,
- 72aeb18772deeb386da7dd8997b969877bd29e41
0, option_parse_exclude },
{ OPTION_CALLBACK, 'X', exclude-from, dir, N_(file),
N_(exclude patterns are read from file),
diff --git a/dir.c b/dir.c
index 8c58ce4..cf1e6b0 100644
--- a/dir.c
+++ b/dir.c
@@ -1674,14 +1674,14 @@ void free_pathspec(struct pathspec *pathspec)
pathspec-items = NULL;
}
- f12e49ae877ad0644b9b9939b7cb742da98691d2
int limit_pathspec_to_literal(void)
{
static int flag = -1;
if (flag 0)
flag = git_env_bool(GIT_LITERAL_PATHSPECS_ENVIRONMENT, 0);
return flag;
-===
+}
+
/*
* Frees memory within dir which was allocated for exclude lists and
* the
[POC PATCH 4/9] pretty: refactor add_merge_info() into parts
pp_commit_list() will be reused later.
Signed-off-by: Thomas Rast t...@thomasrast.ch
---
Necessary only for the next patch, which may be of dubious value.
commit.h | 1 +
pretty.c | 40 ++--
2 files changed, 27 insertions(+), 14 deletions(-)
diff --git a/commit.h b/commit.h
index 16d9c43..b51817b 100644
--- a/commit.h
+++ b/commit.h
@@ -101,6 +101,7 @@ struct pretty_print_context {
struct string_list *mailmap;
int color;
struct ident_split *from_ident;
+ struct commit_list *merge_bases;
/*
* Fields below here are manipulated internally by pp_* functions and
diff --git a/pretty.c b/pretty.c
index 87db08b..5e44cf8 100644
--- a/pretty.c
+++ b/pretty.c
@@ -517,6 +517,31 @@ static const char *skip_empty_lines(const char *msg)
return msg;
}
+static const char *pp_sha1_to_hex(const struct pretty_print_context *pp,
+ const unsigned char *sha1)
+{
+ const char *hex = NULL;
+ if (pp-abbrev)
+ hex = find_unique_abbrev(sha1, pp-abbrev);
+ if (!hex)
+ hex = sha1_to_hex(sha1);
+ return hex;
+}
+
+static void pp_commit_list(const struct pretty_print_context *pp,
+ struct strbuf *sb,
+ const char *prefix,
+ const struct commit_list *list)
+{
+ strbuf_addstr(sb, prefix);
+ while (list) {
+ struct commit *commit = list-item;
+ strbuf_addf(sb, %s, pp_sha1_to_hex(pp, commit-object.sha1));
+ list = list-next;
+ }
+ strbuf_addch(sb, '\n');
+}
+
static void add_merge_info(const struct pretty_print_context *pp,
struct strbuf *sb, const struct commit *commit)
{
@@ -526,20 +551,7 @@ static void add_merge_info(const struct
pretty_print_context *pp,
!parent || !parent-next)
return;
- strbuf_addstr(sb, Merge:);
-
- while (parent) {
- struct commit *p = parent-item;
- const char *hex = NULL;
- if (pp-abbrev)
- hex = find_unique_abbrev(p-object.sha1, pp-abbrev);
- if (!hex)
- hex = sha1_to_hex(p-object.sha1);
- parent = parent-next;
-
- strbuf_addf(sb, %s, hex);
- }
- strbuf_addch(sb, '\n');
+ pp_commit_list(pp, sb, Merge:, parent);
}
static char *get_header(const struct commit *commit, const char *msg,
--
1.9.rc2.232.gdd31389
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 1/9] merge-recursive: remove dead conditional in update_stages()
From: Thomas Rast tr...@inf.ethz.ch 650467c (merge-recursive: Consolidate different update_stages functions, 2011-08-11) changed the former argument 'clear' to always be true. Remove the useless conditional. Signed-off-by: Thomas Rast tr...@inf.ethz.ch Signed-off-by: Junio C Hamano gits...@pobox.com --- merge-recursive.c | 6 ++ 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/merge-recursive.c b/merge-recursive.c index 8400a8e..c36dc79 100644 --- a/merge-recursive.c +++ b/merge-recursive.c @@ -545,11 +545,9 @@ static int update_stages(const char *path, const struct diff_filespec *o, * would_lose_untracked). Instead, reverse the order of the calls * (executing update_file first and then update_stages). */ - int clear = 1; int options = ADD_CACHE_OK_TO_ADD | ADD_CACHE_SKIP_DFCHECK; - if (clear) - if (remove_file_from_cache(path)) - return -1; + if (remove_file_from_cache(path)) + return -1; if (o) if (add_cacheinfo(o-mode, o-sha1, path, 1, 0, options)) return -1; -- 1.9.rc2.232.gdd31389 -- To unsubscribe from this list: send the line 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 7/9] Fold all merge diff variants into an enum
The four ways of displaying merge diffs,
* none: no diff
* -m: against each parent
* -c: combined
* --cc: combined-condensed
were encoded in three flag bits in struct rev_info. Fold them all
into a single enum field that captures the variants.
This makes it easier to add new merge diff variants without yet more
special casing. It should also be slightly easier to read because one
does not have to ensure that the flag bits are set in an expected
combination.
Signed-off-by: Thomas Rast t...@thomasrast.ch
---
builtin/diff-files.c| 5 +++--
builtin/diff-tree.c | 2 +-
builtin/diff.c | 9 +
builtin/fmt-merge-msg.c | 2 +-
builtin/log.c | 9 -
builtin/merge.c | 1 -
combine-diff.c | 2 +-
diff-lib.c | 7 ---
log-tree.c | 4 ++--
revision.c | 13 +++--
revision.h | 22 +++---
submodule.c | 4 +---
12 files changed, 44 insertions(+), 36 deletions(-)
diff --git a/builtin/diff-files.c b/builtin/diff-files.c
index 9200069..172b50d 100644
--- a/builtin/diff-files.c
+++ b/builtin/diff-files.c
@@ -57,9 +57,10 @@ int cmd_diff_files(int argc, const char **argv, const char
*prefix)
* was not asked to. diff-files -c -p should not densify
* (the user should ask with diff-files --cc explicitly).
*/
- if (rev.max_count == -1 !rev.combine_merges
+ if (rev.max_count == -1
+ !merge_diff_mode_is_any_combined(rev)
(rev.diffopt.output_format DIFF_FORMAT_PATCH))
- rev.combine_merges = rev.dense_combined_merges = 1;
+ rev.merge_diff_mode = MERGE_DIFF_COMBINED_CONDENSED;
if (read_cache_preload(rev.diffopt.pathspec) 0) {
perror(read_cache_preload);
diff --git a/builtin/diff-tree.c b/builtin/diff-tree.c
index be6417d..2950f80 100644
--- a/builtin/diff-tree.c
+++ b/builtin/diff-tree.c
@@ -96,7 +96,7 @@ static int diff_tree_stdin(char *line)
static void diff_tree_tweak_rev(struct rev_info *rev, struct
setup_revision_opt *opt)
{
if (!rev-diffopt.output_format) {
- if (rev-dense_combined_merges)
+ if (rev-merge_diff_mode == MERGE_DIFF_COMBINED_CONDENSED)
rev-diffopt.output_format = DIFF_FORMAT_PATCH;
else
rev-diffopt.output_format = DIFF_FORMAT_RAW;
diff --git a/builtin/diff.c b/builtin/diff.c
index 47f663b..fd4c75f 100644
--- a/builtin/diff.c
+++ b/builtin/diff.c
@@ -192,8 +192,8 @@ static int builtin_diff_combined(struct rev_info *revs,
if (argc 1)
usage(builtin_diff_usage);
- if (!revs-dense_combined_merges !revs-combine_merges)
- revs-dense_combined_merges = revs-combine_merges = 1;
+ if (!merge_diff_mode_is_any_combined(revs))
+ revs-merge_diff_mode = MERGE_DIFF_COMBINED_CONDENSED;
for (i = 1; i ents; i++)
sha1_array_append(parents, ent[i].item-sha1);
diff_tree_combined(ent[0].item-sha1, parents, revs);
@@ -242,9 +242,10 @@ static int builtin_diff_files(struct rev_info *revs, int
argc, const char **argv
* dense one, --cc can be explicitly asked for, or just rely
* on the default).
*/
- if (revs-max_count == -1 !revs-combine_merges
+ if (revs-max_count == -1
+ !merge_diff_mode_is_any_combined(revs)
(revs-diffopt.output_format DIFF_FORMAT_PATCH))
- revs-combine_merges = revs-dense_combined_merges = 1;
+ revs-merge_diff_mode = MERGE_DIFF_COMBINED_CONDENSED;
setup_work_tree();
if (read_cache_preload(revs-diffopt.pathspec) 0) {
diff --git a/builtin/fmt-merge-msg.c b/builtin/fmt-merge-msg.c
index 3906eda..2deeacd 100644
--- a/builtin/fmt-merge-msg.c
+++ b/builtin/fmt-merge-msg.c
@@ -637,7 +637,7 @@ int fmt_merge_msg(struct strbuf *in, struct strbuf *out,
head = lookup_commit_or_die(head_sha1, HEAD);
init_revisions(rev, NULL);
rev.commit_format = CMIT_FMT_ONELINE;
- rev.ignore_merges = 1;
+ rev.merge_diff_mode = MERGE_DIFF_IGNORE;
rev.limited = 1;
strbuf_complete_line(out);
diff --git a/builtin/log.c b/builtin/log.c
index b97373d..cebebea 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -499,13 +499,12 @@ static int show_tree_object(const unsigned char *sha1,
static void show_rev_tweak_rev(struct rev_info *rev, struct setup_revision_opt
*opt)
{
- if (rev-ignore_merges) {
+ if (!rev-merge_diff_mode) {
/* There was no -m on the command line */
- rev-ignore_merges = 0;
- if (!rev-first_parent_only !rev-combine_merges) {
+ rev-merge_diff_mode = MERGE_DIFF_EACH;
+ if (!rev-first_parent_only) {
/* No --first-parent, -c, nor --cc
[POC PATCH 5/9] log: add a merge base inspection option
With the new --bases, print merge commits' parents' merge bases. This
is mostly a proof of viability, in particular wrt. revision walk
decoupling and speed.
We can do inline get_merge_bases() (via get_octopus_merge_bases)
because the walks in get_merge_bases() only use flag bits 16-19, and
we reset them after use. The get_revision()/log display walk OTOH
uses only flag bits 0-15 (actually only 0-10 as of this commit).
Speed-wise it turns out to be better than attempting to compute merge
bases in one go, mostly because the latter approach would require
extensive data structures to track flags. This commit does not have
to: the commit graph will be loaded anyway, and the room for flags is
already there. As a big plus, this approach also works in a streaming
fashion, showing the first few commits very quickly.
Signed-off-by: Thomas Rast t...@thomasrast.ch
---
As indicated in the cover letter, this is cute, but I'm not married to
it. It's probably just a useless instance of feature creep.
Documentation/rev-list-options.txt | 7 +++
log-tree.c | 3 +++
pretty.c | 3 +++
revision.c | 2 ++
revision.h | 2 ++
t/t4202-log.sh | 31 +++
6 files changed, 48 insertions(+)
diff --git a/Documentation/rev-list-options.txt
b/Documentation/rev-list-options.txt
index 03533af..d023290 100644
--- a/Documentation/rev-list-options.txt
+++ b/Documentation/rev-list-options.txt
@@ -705,6 +705,13 @@ endif::git-rev-list[]
Print also the children of the commit (in the form commit child...).
Also enables parent rewriting, see 'History Simplification' below.
+ifndef::git-rev-list[]
+--bases::
+ For merge commits, print the merge bases of the commit's
+ parents. (These are the bases that were used in the creation
+ of the merge itself.)
+endif::git-rev-list[]
+
ifdef::git-rev-list[]
--timestamp::
Print the raw commit timestamp.
diff --git a/log-tree.c b/log-tree.c
index 08970bf..080f412 100644
--- a/log-tree.c
+++ b/log-tree.c
@@ -622,6 +622,8 @@ void show_log(struct rev_info *opt)
ctx.output_encoding = get_log_output_encoding();
if (opt-from_ident.mail_begin opt-from_ident.name_begin)
ctx.from_ident = opt-from_ident;
+ if (opt-show_merge_bases commit-parents commit-parents-next)
+ ctx.merge_bases = get_octopus_merge_bases(commit-parents);
pretty_print_commit(ctx, commit, msgbuf);
if (opt-add_signoff)
@@ -662,6 +664,7 @@ void show_log(struct rev_info *opt)
strbuf_release(msgbuf);
free(ctx.notes_message);
+ free(ctx.merge_bases);
}
int log_tree_diff_flush(struct rev_info *opt)
diff --git a/pretty.c b/pretty.c
index 5e44cf8..8b28664 100644
--- a/pretty.c
+++ b/pretty.c
@@ -552,6 +552,9 @@ static void add_merge_info(const struct
pretty_print_context *pp,
return;
pp_commit_list(pp, sb, Merge:, parent);
+
+ if (pp-merge_bases)
+ pp_commit_list(pp, sb, Bases:, pp-merge_bases);
}
static char *get_header(const struct commit *commit, const char *msg,
diff --git a/revision.c b/revision.c
index a0df72f..72255fb 100644
--- a/revision.c
+++ b/revision.c
@@ -1729,6 +1729,8 @@ static int handle_revision_opt(struct rev_info *revs, int
argc, const char **arg
} else if (!strcmp(arg, --parents)) {
revs-rewrite_parents = 1;
revs-print_parents = 1;
+ } else if (!strcmp(arg, --merge-bases)) {
+ revs-show_merge_bases = 1;
} else if (!strcmp(arg, --dense)) {
revs-dense = 1;
} else if (!strcmp(arg, --sparse)) {
diff --git a/revision.h b/revision.h
index 88967d6..3111228 100644
--- a/revision.h
+++ b/revision.h
@@ -19,6 +19,7 @@
#define PATCHSAME (1u9)
#define BOTTOM (1u10)
#define ALL_REV_FLAGS ((1u11)-1)
+/* merge-base.c uses bits 16-19. --merge-bases will break if they overlap! */
#define DECORATE_SHORT_REFS1
#define DECORATE_FULL_REFS 2
@@ -137,6 +138,7 @@ struct rev_info {
preserve_subject:1;
unsigned intdisable_stdin:1;
unsigned intleak_pending:1;
+ unsigned intshow_merge_bases:1;
enum date_mode date_mode;
diff --git a/t/t4202-log.sh b/t/t4202-log.sh
index cb03d28..64f34a6 100755
--- a/t/t4202-log.sh
+++ b/t/t4202-log.sh
@@ -841,4 +841,35 @@ test_expect_success 'dotdot is a parent directory' '
test_cmp expect actual
'
+shorten () {
+ for arg; do
+ git rev-parse --short $arg
+ done
+}
+
+fill_in_merge_bases () {
+ while IFS= read line; do
+ case $line in
+ Merge:*)
+ printf %s\n $line
+ printf %s Bases:
+ printf %s $(shorten \
+ $(git merge-base
[PATCH 6/9] combine-diff: do not pass revs-dense_combined_merges redundantly
The existing code passed revs-dense_combined_merges along revs itself
into the combine-diff functions, which is rather redundant. Remove
the 'dense' argument until much further down the callchain to simplify
callers.
Note that while the caller in submodule.c needs to do extra work now,
the next commit will simplify this to a single setting again.
Signed-off-by: Thomas Rast t...@thomasrast.ch
---
builtin/diff.c | 3 +--
combine-diff.c | 13 ++---
diff-lib.c | 6 ++
diff.h | 6 +++---
log-tree.c | 2 +-
submodule.c| 5 -
6 files changed, 17 insertions(+), 18 deletions(-)
diff --git a/builtin/diff.c b/builtin/diff.c
index 0f247d2..47f663b 100644
--- a/builtin/diff.c
+++ b/builtin/diff.c
@@ -196,8 +196,7 @@ static int builtin_diff_combined(struct rev_info *revs,
revs-dense_combined_merges = revs-combine_merges = 1;
for (i = 1; i ents; i++)
sha1_array_append(parents, ent[i].item-sha1);
- diff_tree_combined(ent[0].item-sha1, parents,
- revs-dense_combined_merges, revs);
+ diff_tree_combined(ent[0].item-sha1, parents, revs);
sha1_array_clear(parents);
return 0;
}
diff --git a/combine-diff.c b/combine-diff.c
index 3b92c448..6e80a73 100644
--- a/combine-diff.c
+++ b/combine-diff.c
@@ -952,7 +952,7 @@ static void show_combined_header(struct combine_diff_path
*elem,
}
static void show_patch_diff(struct combine_diff_path *elem, int num_parent,
- int dense, int working_tree_file,
+ int working_tree_file,
struct rev_info *rev)
{
struct diff_options *opt = rev-diffopt;
@@ -967,6 +967,7 @@ static void show_patch_diff(struct combine_diff_path *elem,
int num_parent,
struct userdiff_driver *textconv = NULL;
int is_binary;
const char *line_prefix = diff_line_prefix(opt);
+ int dense = rev-dense_combined_merges;
context = opt-context;
userdiff = userdiff_find_by_path(elem-path);
@@ -1214,7 +1215,6 @@ static void show_raw_diff(struct combine_diff_path *p,
int num_parent, struct re
*/
void show_combined_diff(struct combine_diff_path *p,
int num_parent,
- int dense,
struct rev_info *rev)
{
struct diff_options *opt = rev-diffopt;
@@ -1226,7 +1226,7 @@ void show_combined_diff(struct combine_diff_path *p,
DIFF_FORMAT_NAME_STATUS))
show_raw_diff(p, num_parent, rev);
else if (opt-output_format DIFF_FORMAT_PATCH)
- show_patch_diff(p, num_parent, dense, 1, rev);
+ show_patch_diff(p, num_parent, 1, rev);
}
static void free_combined_pair(struct diff_filepair *pair)
@@ -1297,7 +1297,6 @@ static void handle_combined_callback(struct diff_options
*opt,
void diff_tree_combined(const unsigned char *sha1,
const struct sha1_array *parents,
- int dense,
struct rev_info *rev)
{
struct diff_options *opt = rev-diffopt;
@@ -1365,7 +1364,7 @@ void diff_tree_combined(const unsigned char *sha1,
opt-line_termination);
for (p = paths; p; p = p-next) {
if (p-len)
- show_patch_diff(p, num_parent, dense,
+ show_patch_diff(p, num_parent,
0, rev);
}
}
@@ -1381,7 +1380,7 @@ void diff_tree_combined(const unsigned char *sha1,
free_pathspec(diffopts.pathspec);
}
-void diff_tree_combined_merge(const struct commit *commit, int dense,
+void diff_tree_combined_merge(const struct commit *commit,
struct rev_info *rev)
{
struct commit_list *parent = get_saved_parents(rev, commit);
@@ -1391,6 +1390,6 @@ void diff_tree_combined_merge(const struct commit
*commit, int dense,
sha1_array_append(parents, parent-item-object.sha1);
parent = parent-next;
}
- diff_tree_combined(commit-object.sha1, parents, dense, rev);
+ diff_tree_combined(commit-object.sha1, parents, rev);
sha1_array_clear(parents);
}
diff --git a/diff-lib.c b/diff-lib.c
index 346cac6..8d0f572 100644
--- a/diff-lib.c
+++ b/diff-lib.c
@@ -174,9 +174,7 @@ int run_diff_files(struct rev_info *revs, unsigned int
option)
i--;
if (revs-combine_merges num_compare_stages == 2) {
- show_combined_diff(dpath, 2,
- revs-dense_combined_merges,
- revs);
+ show_combined_diff(dpath, 2, revs);
[PATCH 8/9] merge-recursive: allow storing conflict hunks in index
Add a --conflicts-in-index option to merge-recursive, which instructs
it to always store the 3-way merged result in the index. (Normally it
only does so in recursive invocations, but not for the final result.)
This serves as a building block for the remerge diff feature coming
up in a subsequent patch. The external option lets us easily use it
from tests, where we'd otherwise need a new test-* helper to access
the feature.
Furthermore, it might occasionally be useful for scripts that want to
look at the result of invoking git-merge without tampering with the
worktree. They could already get the _conflicts_ with --index-only,
but not (conveniently) the conflict-hunk formatted files that would
normally be written to the worktree.
Signed-off-by: Thomas Rast t...@thomasrast.ch
---
Documentation/merge-strategies.txt | 5 +
merge-recursive.c | 4
merge-recursive.h | 1 +
t/t3030-merge-recursive.sh | 20
4 files changed, 30 insertions(+)
diff --git a/Documentation/merge-strategies.txt
b/Documentation/merge-strategies.txt
index 2934e99..3468d99 100644
--- a/Documentation/merge-strategies.txt
+++ b/Documentation/merge-strategies.txt
@@ -96,6 +96,11 @@ index-only;;
Write the merge result only to the index; do not touch the
worktree.
+conflicts-in-index;;
+ For conflicted files, write 3-way merged contents with
+ conflict hunks to the index, instead of leaving their entries
+ unresolved.
+
octopus::
This resolves cases with more than two heads, but refuses to do
a complex merge that needs manual resolution. It is
diff --git a/merge-recursive.c b/merge-recursive.c
index f59c1d3..b682812 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -724,6 +724,8 @@ static void update_file_flags(struct merge_options *o,
int update_cache,
int update_wd)
{
+ if (o-conflicts_in_index)
+ update_cache = 1;
if (o-call_depth || o-no_worktree)
update_wd = 0;
@@ -2098,6 +2100,8 @@ int parse_merge_opt(struct merge_options *o, const char
*s)
}
else if (!strcmp(s, index-only))
o-no_worktree = 1;
+ else if (!strcmp(s, conflicts-in-index))
+ o-conflicts_in_index = 1;
else
return -1;
return 0;
diff --git a/merge-recursive.h b/merge-recursive.h
index d8dd7a1..9b8e20b 100644
--- a/merge-recursive.h
+++ b/merge-recursive.h
@@ -16,6 +16,7 @@ struct merge_options {
unsigned buffer_output : 1;
unsigned renormalize : 1;
unsigned no_worktree : 1; /* do not touch worktree */
+ unsigned conflicts_in_index : 1; /* index will contain conflict hunks */
long xdl_opts;
int verbosity;
int diff_rename_limit;
diff --git a/t/t3030-merge-recursive.sh b/t/t3030-merge-recursive.sh
index 2f3a16c..4192fd3 100755
--- a/t/t3030-merge-recursive.sh
+++ b/t/t3030-merge-recursive.sh
@@ -309,6 +309,26 @@ test_expect_success 'merge-recursive --index-only' '
test_cmp expected-diff actual-diff
'
+test_expect_success 'merge-recursive --index-only --conflicts-in-index' '
+ # first pass: do a merge as usual to obtain expected
+ rm -fr [abcd]
+ git checkout -f $c2
+ test_expect_code 1 git merge-recursive $c0 -- $c2 $c1
+ git add [abcd]
+ git ls-files -s expected
+ # second pass: actual test
+ rm -fr [abcd]
+ git checkout -f $c2
+ test_expect_code 1 \
+ git merge-recursive --index-only --conflicts-in-index \
+ $c0 -- $c2 $c1
+ git ls-files -s actual
+ test_cmp expected actual
+ git diff HEAD actual-diff
+ : expected-diff
+ test_cmp expected-diff actual-diff
+'
+
test_expect_success 'fail if the index has unresolved entries' '
rm -fr [abcd]
--
1.9.rc2.232.gdd31389
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 2/9] merge-recursive: internal flag to avoid touching the worktree
From: Thomas Rast tr...@inf.ethz.ch
o-call_depth has a double function: a nonzero call_depth means we
want to construct virtual merge bases, but it also means we want to
avoid touching the worktree. Introduce a new flag o-no_worktree to
trigger only the latter.
Signed-off-by: Thomas Rast tr...@inf.ethz.ch
Signed-off-by: Junio C Hamano gits...@pobox.com
---
merge-recursive.c | 37 +
merge-recursive.h | 1 +
2 files changed, 22 insertions(+), 16 deletions(-)
diff --git a/merge-recursive.c b/merge-recursive.c
index c36dc79..35be144 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -408,10 +408,10 @@ static void record_df_conflict_files(struct merge_options
*o,
int i;
/*
-* If we're merging merge-bases, we don't want to bother with
-* any working directory changes.
+* If we're working in-core only (e.g., merging merge-bases),
+* we don't want to bother with any working directory changes.
*/
- if (o-call_depth)
+ if (o-call_depth || o-no_worktree)
return;
/* Ensure D/F conflicts are adjacent in the entries list. */
@@ -724,7 +724,7 @@ static void update_file_flags(struct merge_options *o,
int update_cache,
int update_wd)
{
- if (o-call_depth)
+ if (o-call_depth || o-no_worktree)
update_wd = 0;
if (update_wd) {
@@ -931,7 +931,8 @@ static struct merge_file_info merge_file_1(struct
merge_options *o,
result.clean = merge_submodule(result.sha,
one-path, one-sha1,
a-sha1, b-sha1,
- !o-call_depth);
+ !(o-call_depth ||
+o-no_worktree));
} else if (S_ISLNK(a-mode)) {
hashcpy(result.sha, a-sha1);
@@ -1003,7 +1004,7 @@ static void handle_change_delete(struct merge_options *o,
const char *change, const char *change_past)
{
char *renamed = NULL;
- if (dir_in_way(path, !o-call_depth)) {
+ if (dir_in_way(path, !(o-call_depth || o-no_worktree))) {
renamed = unique_path(o, path, a_sha ? o-branch1 : o-branch2);
}
@@ -1128,10 +1129,10 @@ static void handle_file(struct merge_options *o,
char *add_name = unique_path(o, rename-path, other_branch);
update_file(o, 0, add-sha1, add-mode, add_name);
- remove_file(o, 0, rename-path, 0);
+ remove_file(o, 0, rename-path, o-call_depth ||
o-no_worktree);
dst_name = unique_path(o, rename-path, cur_branch);
} else {
- if (dir_in_way(rename-path, !o-call_depth)) {
+ if (dir_in_way(rename-path, !(o-call_depth ||
o-no_worktree))) {
dst_name = unique_path(o, rename-path, cur_branch);
output(o, 1, _(%s is a directory in %s adding as %s
instead),
rename-path, other_branch, dst_name);
@@ -1238,7 +1239,7 @@ static void conflict_rename_rename_2to1(struct
merge_options *o,
* merge base just undo the renames; they can be detected
* again later for the non-recursive merge.
*/
- remove_file(o, 0, path, 0);
+ remove_file(o, 0, path, o-call_depth || o-no_worktree);
update_file(o, 0, mfi_c1.sha, mfi_c1.mode, a-path);
update_file(o, 0, mfi_c2.sha, mfi_c2.mode, b-path);
} else {
@@ -1246,7 +1247,7 @@ static void conflict_rename_rename_2to1(struct
merge_options *o,
char *new_path2 = unique_path(o, path, ci-branch2);
output(o, 1, _(Renaming %s to %s and %s to %s instead),
a-path, new_path1, b-path, new_path2);
- remove_file(o, 0, path, 0);
+ remove_file(o, 0, path, o-call_depth || o-no_worktree);
update_file(o, 0, mfi_c1.sha, mfi_c1.mode, new_path1);
update_file(o, 0, mfi_c2.sha, mfi_c2.mode, new_path2);
free(new_path2);
@@ -1405,6 +1406,7 @@ static int process_renames(struct merge_options *o,
* add-source case).
*/
remove_file(o, 1, ren1_src,
+ o-call_depth || o-no_worktree ||
renamed_stage == 2 ||
!was_tracked(ren1_src));
hashcpy(src_other.sha1,
ren1-src_entry-stages[other_stage].sha);
@@ -1601,7 +1603,7 @@ static int merge_content(struct merge_options *o,
o-branch2 == rename_conflict_info-branch1) ?
[PATCH 3/9] merge-recursive: -Xindex-only to leave worktree unchanged
From: Thomas Rast tr...@inf.ethz.ch Using the new no_worktree flag from the previous commit, we can teach merge-recursive to leave the worktree untouched. Expose this with a new strategy option so that scripts can use it. Signed-off-by: Junio C Hamano gits...@pobox.com --- Documentation/merge-strategies.txt | 4 merge-recursive.c | 2 ++ t/t3030-merge-recursive.sh | 13 + 3 files changed, 19 insertions(+) diff --git a/Documentation/merge-strategies.txt b/Documentation/merge-strategies.txt index fb6e593..2934e99 100644 --- a/Documentation/merge-strategies.txt +++ b/Documentation/merge-strategies.txt @@ -92,6 +92,10 @@ subtree[=path];; is prefixed (or stripped from the beginning) to make the shape of two trees to match. +index-only;; + Write the merge result only to the index; do not touch the + worktree. + octopus:: This resolves cases with more than two heads, but refuses to do a complex merge that needs manual resolution. It is diff --git a/merge-recursive.c b/merge-recursive.c index 35be144..f59c1d3 100644 --- a/merge-recursive.c +++ b/merge-recursive.c @@ -2096,6 +2096,8 @@ int parse_merge_opt(struct merge_options *o, const char *s) if ((o-rename_score = parse_rename_score(score)) == -1 || *score != 0) return -1; } + else if (!strcmp(s, index-only)) + o-no_worktree = 1; else return -1; return 0; diff --git a/t/t3030-merge-recursive.sh b/t/t3030-merge-recursive.sh index 2f96100..2f3a16c 100755 --- a/t/t3030-merge-recursive.sh +++ b/t/t3030-merge-recursive.sh @@ -296,6 +296,19 @@ test_expect_success 'merge-recursive result' ' ' +test_expect_success 'merge-recursive --index-only' ' + + rm -fr [abcd] + git checkout -f $c2 + test_expect_code 1 git merge-recursive --index-only $c0 -- $c2 $c1 + git ls-files -s actual + # reuses expected from previous test! + test_cmp expected actual + git diff HEAD actual-diff + : expected-diff + test_cmp expected-diff actual-diff +' + test_expect_success 'fail if the index has unresolved entries' ' rm -fr [abcd] -- 1.9.rc2.232.gdd31389 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[RFC PATCH 9/9] log --remerge-diff: show what the conflict resolution changed
Git has --cc as a very fast inspection tool that shows a brief summary
of what a conflicted merge looks like, and -c/-m as give me the
full information data dumps.
But --cc actually loses information: if the merge lost(!) some changes
from one side, that hunk would fully agree with the other side, and
therefore be elided. So --cc cannot be used to investigate mismerges.
Indeed it is rather hard to find a merge that has lost changes, unless
one knows where to look.
The new option --remerge-diff is an attempt at filling this gap,
admittedly at the cost of a lot of CPU cycles. For each merge commit,
it diffs the merge result against a recursive merge of the merge's
parents.
For files that can be auto-merged cleanly, it will typically show
nothing. However, it will make it obvious when the merge introduces
extra changes.
For files that result in merge conflicts, we diff against the
representation with conflict hunks (what the user would usually see in
the worktree). So the diff will show what was changed in the conflict
hunks to resolve the conflict.
It still takes a bit of staring to tell an evil from a regular merge.
But at least the information is there, unlike with --cc; and the
output is usually much shorter than with -c.
Signed-off-by: Thomas Rast t...@thomasrast.ch
---
Documentation/rev-list-options.txt | 7 ++
log-tree.c | 60 +++
merge-recursive.c | 3 +-
merge-recursive.h | 1 +
revision.c | 2 +
revision.h | 4 +-
t/t4202-log.sh | 3 +
t/t4213-log-remerge-diff.sh| 145 +
8 files changed, 223 insertions(+), 2 deletions(-)
create mode 100755 t/t4213-log-remerge-diff.sh
diff --git a/Documentation/rev-list-options.txt
b/Documentation/rev-list-options.txt
index d023290..6611557 100644
--- a/Documentation/rev-list-options.txt
+++ b/Documentation/rev-list-options.txt
@@ -797,6 +797,13 @@ options may be given. See linkgit:git-diff-files[1] for
more options.
in that case, the output represents the changes the merge
brought _into_ the then-current branch.
+--remerge-diff::
+ Diff merge commits against a recursive merge of their parents,
+ with conflict hunks. Intuitively speaking, this shows what
+ the author of the merge changed to resolve the merge. It
+ assumes that all (or most) merges are recursive merges; other
+ strategies are not supported.
+
-r::
Show recursive diffs.
diff --git a/log-tree.c b/log-tree.c
index 4ab3ffe..fa22737 100644
--- a/log-tree.c
+++ b/log-tree.c
@@ -11,6 +11,8 @@
#include gpg-interface.h
#include sequencer.h
#include line-log.h
+#include cache-tree.h
+#include merge-recursive.h
struct decoration name_decoration = { object names };
@@ -725,6 +727,62 @@ static int do_diff_combined(struct rev_info *opt, struct
commit *commit)
return !opt-loginfo;
}
+static int do_diff_remerge(struct rev_info *opt, struct commit *commit)
+{
+ struct commit_list *merge_bases;
+ struct commit *result, *parent1, *parent2;
+ struct merge_options o;
+ char *branch1, *branch2;
+
+ if (commit-parents-next-next) {
+ printf(--remerge-diff not supported for octopus merges.\n);
+ return 0;
+ }
+
+ parent1 = commit-parents-item;
+ parent2 = commit-parents-next-item;
+ parse_commit(parent1);
+ parse_commit(parent2);
+ branch1 = xstrdup(sha1_to_hex(parent1-object.sha1));
+ branch2 = xstrdup(sha1_to_hex(parent2-object.sha1));
+
+ merge_bases = get_octopus_merge_bases(commit-parents);
+ init_merge_options(o);
+ o.verbosity = -1;
+ o.no_worktree = 1;
+ o.conflicts_in_index = 1;
+ o.use_ondisk_index = 0;
+ o.branch1 = branch1;
+ o.branch2 = branch2;
+ merge_recursive(o, parent1, parent2, merge_bases, result);
+ free(branch1);
+ free(branch2);
+
+ active_cache_tree = cache_tree();
+ if (cache_tree_update(active_cache_tree,
+ (const struct cache_entry * const *)active_cache,
+ active_nr, WRITE_TREE_SILENT) 0) {
+ printf(BUG: merge conflicts not fully folded, cannot diff.\n);
+ return 0;
+ }
+
+ if (opt-loginfo !opt-no_commit_id) {
+ show_log(opt);
+
+ if (opt-verbose_header opt-diffopt.output_format)
+ printf(%s%c, diff_line_prefix(opt-diffopt),
+ opt-diffopt.line_termination);
+ }
+
+ diff_tree_sha1(active_cache_tree-sha1, commit-tree-object.sha1,
+ , opt-diffopt);
+ log_tree_diff_flush(opt);
+
+ cache_tree_free(active_cache_tree);
+
+ return !opt-loginfo;
+}
+
/*
* Show the diff of a commit.
*
@@ -758,6 +816,8 @@ static int
Re: [PATCH 2/2] reset: support --mixed --intent-to-add mode
Junio C Hamano gits...@pobox.com writes:
While I do not have any problem with adding an optional keep lost
paths as intent-to-add entries feature, I am not sure why this has
to be so different from the usual add-cache-entry codepath. The
if/elseif chain you are touching inside this loop does:
- If the tree you are resetting to has something at the path
(which is different from the current index, obviously), create
a cache entry to represent that state from the tree and stuff
it in the index;
- Otherwise, the tree you are resetting to does not have that
path. We used to say remove it from the index, but now we have
an option to instead add it as an intent-to-add entry.
So, why doesn't the new codepath do exactly the same thing as the
first branch of the if/else chain and call add_cache_entry but with
a ce marked with CE_INTENT_TO_ADD? That would parallel what happens
in git add -N better, I would think, no?
In other words, something along this line, perhaps?
-- 8 --
From: Nguyễn Thái Ngọc Duy pclo...@gmail.com
Date: Tue, 4 Feb 2014 09:20:09 +0700
When --mixed is used, entries could be removed from index if the
target ref does not have them. When reset is used in preparation for
commit spliting (in a dirty worktree), it could be hard to track what
files to be added back. The new option --intent-to-add simplifies it
by marking all removed files intent-to-add.
Signed-off-by: Nguyễn Thái Ngọc Duy pclo...@gmail.com
---
Documentation/git-reset.txt | 5 -
builtin/reset.c | 38 ++
cache.h | 1 +
read-cache.c| 4 ++--
t/t7102-reset.sh| 9 +
5 files changed, 42 insertions(+), 15 deletions(-)
diff --git a/Documentation/git-reset.txt b/Documentation/git-reset.txt
index f445cb3..a077ba0 100644
--- a/Documentation/git-reset.txt
+++ b/Documentation/git-reset.txt
@@ -10,7 +10,7 @@ SYNOPSIS
[verse]
'git reset' [-q] [tree-ish] [--] paths...
'git reset' (--patch | -p) [tree-ish] [--] [paths...]
-'git reset' [--soft | --mixed | --hard | --merge | --keep] [-q] [commit]
+'git reset' [--soft | --mixed [-N] | --hard | --merge | --keep] [-q] [commit]
DESCRIPTION
---
@@ -60,6 +60,9 @@ section of linkgit:git-add[1] to learn how to operate the
`--patch` mode.
Resets the index but not the working tree (i.e., the changed files
are preserved but not marked for commit) and reports what has not
been updated. This is the default action.
++
+If `-N` is specified, removed paths are marked as intent-to-add (see
+linkgit:git-add[1]).
--hard::
Resets the index and working tree. Any changes to tracked files in the
diff --git a/builtin/reset.c b/builtin/reset.c
index 6004803..d363bc5 100644
--- a/builtin/reset.c
+++ b/builtin/reset.c
@@ -116,25 +116,32 @@ static void update_index_from_diff(struct
diff_queue_struct *q,
struct diff_options *opt, void *data)
{
int i;
+ int intent_to_add = *(int *)data;
for (i = 0; i q-nr; i++) {
struct diff_filespec *one = q-queue[i]-one;
- if (one-mode !is_null_sha1(one-sha1)) {
- struct cache_entry *ce;
- ce = make_cache_entry(one-mode, one-sha1, one-path,
- 0, 0);
- if (!ce)
- die(_(make_cache_entry failed for path '%s'),
- one-path);
- add_cache_entry(ce, ADD_CACHE_OK_TO_ADD |
- ADD_CACHE_OK_TO_REPLACE);
- } else
+ int is_missing = !(one-mode !is_null_sha1(one-sha1));
+ struct cache_entry *ce;
+
+ if (is_missing !intent_to_add) {
remove_file_from_cache(one-path);
+ continue;
+ }
+
+ ce = make_cache_entry(one-mode, one-sha1, one-path,
+ 0, 0);
+ if (!ce)
+ die(_(make_cache_entry failed for path '%s'),
+ one-path);
+ if (is_missing)
+ mark_intent_to_add(ce);
+ add_cache_entry(ce, ADD_CACHE_OK_TO_ADD |
ADD_CACHE_OK_TO_REPLACE);
}
}
static int read_from_tree(const struct pathspec *pathspec,
- unsigned char *tree_sha1)
+ unsigned char *tree_sha1,
+ int intent_to_add)
{
struct diff_options opt;
@@ -142,6 +149,7 @@ static int read_from_tree(const struct pathspec *pathspec,
copy_pathspec(opt.pathspec, pathspec);
opt.output_format = DIFF_FORMAT_CALLBACK;
opt.format_callback = update_index_from_diff;
+ opt.format_callback_data = intent_to_add;
if (do_diff_cache(tree_sha1, opt))
return 1;
@@ -258,6 +266,7 @@ int
Re: [PATCH 2/3] setup_pager: set MORE=R
On Tue, Feb 04, 2014 at 02:17:57PM -0800, Junio C Hamano wrote: Jeff King p...@peff.net writes: But there's another set of people that I was intending to help with the patch, which is people that have set up LESS, and did not necessarily care about the R flag in the past (e.g., for many years before git came along, I set LESS=giM, and never even knew that R existed). Since git comes out of the box these days with color and the pager turned on, that means people with such a setup see broken output from day one. And I think it is Git's fault here, not the user or the packager. I am not particularly itnterested in whose fault it is ;-) If the user sets LESS himself, he knows how to set it (and if he is setting it automatically for all of his sessions, he knows where to do so), and would know better than Git about less, his pager of choice. If he did not know about R and did not see color, that is even better. Now he knows and his update to his LESS settings will let him view colors in outputs from programs that are not Git. Right. If git just disabled the color, I think that would be sane (and that is what my patch was shooting for). But somebody who sees: $ git log ESC[33mcommit 3c6b385c655a52fd9db176ce1e01469dc9954f91ESC[mESC[33m (ESC[1;36mHEADESC[mESC[33m, ESC[1;32mjk/meta-makeESC[mESC[33m)ESC[m does not necessarily know what is going on. They do not know that it is a less problem, nor that their less settings are relevant. They only see that Git is broken out of the box. Anyway, I will leave it at that. It's an unfortunate problem, but one not worth fixing. -Peff -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] blame.c: prepare_lines should not call xrealloc for every line
From: David Kastrup d...@gnu.org To: Junio C Hamano gits...@pobox.com Cc: git@vger.kernel.org Sent: Tuesday, February 04, 2014 9:09 PM Subject: Re: [PATCH] blame.c: prepare_lines should not call xrealloc for every line Junio C Hamano gits...@pobox.com writes: Junio C Hamano gits...@pobox.com writes: David Kastrup d...@gnu.org writes: Whitespace error in line 1778. Should I be reposting? Heh, let me try to clean it up first and then repost for your review. Thanks. -- 8 -- From: David Kastrup d...@gnu.org Making a single preparation run for counting the lines will avoid memory fragmentation. Also, fix the allocated memory size which was wrong when sizeof(int *) != sizeof(int), and would have been too small for sizeof(int *) sizeof(int), admittedly unlikely. Signed-off-by: David Kastrup d...@gnu.org --- One logic difference from what was posted is that sb-lineno[num] is filled with the length of the entire buffer when the file ends with a complete line. Where's the difference? This is exactly what will happen with my code as well. I _do_ rely on memchr(whatever, '\n', 0) to return NULL without looking at any memory for that. If there is a fear of memchr not being able to deal with a count of 0, this code needs to be somewhat more complex. A bit of googling found http://marc.info/?l=gnulib-bugm=130108029329021 which suggests that behaviour can't be relied upon, and that perhaps some code is 'buggy' relative to expectations (hence the patch it proposed). It suggests that one can't properly reference a zero length object. I do not remember if the rest of the logic actually depends on it (I think I use lineno[n+1] - lineno[n] to find the end of line, Well, you do it about _half_ the time. The other half, you scan for the '\n' explicitly. The original code dates back to 2006 when the author of the code was not paid for doing anything for Git but was doing it as a weekend and evening hobby, so it may not be so surprising to find this kind of what was I thinking when I wrote it inefficiency in such a code with $0 monetary value ;-) Oh, _this_ patch is not in the I want to make money from it range. If that were the case, I should not have bothered at all. This is just the this code offends my sense of aesthetics class. It's purely optional to apply. It's conceivable that it will make a performance difference on non-glibc (or what it's called) platforms. -- David Kastrup -- Philip -- To unsubscribe from this list: send the line 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/3] setup_pager: set MORE=R
On 02/04/2014 14:25, Jeff King wrote: Right. If git just disabled the color, I think that would be sane (and that is what my patch was shooting for). But somebody who sees: $ git log ESC[33mcommit 3c6b385c655a52fd9db176ce1e01469dc9954f91ESC[mESC[33m (ESC[1;36mHEADESC[mESC[33m, ESC[1;32mjk/meta-makeESC[mESC[33m)ESC[m does not necessarily know what is going on. They do not know that it is a less problem, nor that their less settings are relevant. They only see that Git is broken out of the box. Maybe, instead of doing all the elaborate guess and assumption work, have configure script check if the current PAGER supports colors and build git accordingly? configure could run the pager as one of its tests, and determine if ESC appears on the output. Yuri -- To unsubscribe from this list: send the line 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] blame.c: prepare_lines should not call xrealloc for every line
From: Philip Oakley philipoak...@iee.org From: David Kastrup d...@gnu.org To: Junio C Hamano gits...@pobox.com Cc: git@vger.kernel.org Sent: Tuesday, February 04, 2014 9:09 PM Subject: Re: [PATCH] blame.c: prepare_lines should not call xrealloc for every line [...] Where's the difference? This is exactly what will happen with my code as well. I _do_ rely on memchr(whatever, '\n', 0) to return NULL without looking at any memory for that. If there is a fear of memchr not being able to deal with a count of 0, this code needs to be somewhat more complex. A bit of googling found http://marc.info/?l=gnulib-bugm=130108029329021 which suggests that behaviour can't be relied upon, and that perhaps some code is 'buggy' relative to expectations (hence the patch it proposed). It suggests that one can't properly reference a zero length object. I think I've confused myself between the patch https://lkml.org/lkml/2010/11/24/429 which fixed a bug in memchr() and the discussion in http://lists.gnu.org/archive/html/bug-gnulib/2011-03/msg00273.html (alternate link to marc.info) that discusses realloc() which has we changed gnulib's test-memchr to instead test memchr(zerosize_ptr(),a,0) rather than memchr(NULL,a,0). However, once you can handle memchr(,0) on any other zero-size object, most implementations also happen to do what you want for memchr(NULL,a,0), even though it is technically undefined behavior. ending with 'technically undefined behavior' which I misconstrued. Apologies for the noise. I do not remember if the rest of the logic actually depends on it (I think I use lineno[n+1] - lineno[n] to find the end of line, ... -- David Kastrup -- Philip -- -- To unsubscribe from this list: send the line 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/3] setup_pager: set MORE=R
On Tue, Feb 04, 2014 at 02:45:11PM -0800, Yuri wrote: On 02/04/2014 14:25, Jeff King wrote: Right. If git just disabled the color, I think that would be sane (and that is what my patch was shooting for). But somebody who sees: $ git log ESC[33mcommit 3c6b385c655a52fd9db176ce1e01469dc9954f91ESC[mESC[33m (ESC[1;36mHEADESC[mESC[33m, ESC[1;32mjk/meta-makeESC[mESC[33m)ESC[m does not necessarily know what is going on. They do not know that it is a less problem, nor that their less settings are relevant. They only see that Git is broken out of the box. Maybe, instead of doing all the elaborate guess and assumption work, have configure script check if the current PAGER supports colors and build git accordingly? configure could run the pager as one of its tests, and determine if ESC appears on the output. But this is not a build-time problem, but rather a run-time problem. We do not know what the environment of the user will be at run-time. The safest thing would be to turn off auto-color when LESS (or any of the pager environment variables) is set at all (and not worry about whether R is set; only know that _we_ are not setting it, so we cannot count on it). But that would potentially inconvenience a lot of people whose default color would suddenly go away. -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
[Bug] branch.*.merge interpreted too strictly by tracking logic
Start from an empty repository like so:
: gitster track; git init
Initialized empty Git repository in /var/tmp/x/track/.git/
: gitster track/master; git commit --allow-empty -m initial
[master (root-commit) abdcd1c] initial
: gitster track/master; git branch foo
: gitster track/master; git branch bar
: gitster track/master; git commit --allow-empty -m second
[master 78e28f4] second
Now, 'master' has two commits, while 'foo' and 'bar' both are one
commit behind, pointing at 'master^1'.
Let's tell these branches that they are both supposed to be building
on top of 'master'.
: gitster track/master; git config branch.foo.remote .
: gitster track/master; git config branch.foo.merge refs/heads/master
: gitster track/master; git config branch.bar.remote .
: gitster track/master; git config branch.bar.merge master
The difference between the two is that 'foo' spells the @{upstream}
branch in full (which is the recommended practice), while 'bar' is
loose and asks for 'master'.
Let's see what happens on these two branches. First 'foo':
: gitster track/master; git checkout foo
Switched to branch 'foo'
Your branch is behind 'master' by 1 commit, and can be
fast-forwarded.
(use git pull to update your local branch)
: gitster track/foo; git pull
From .
* branchmaster - FETCH_HEAD
Updating abdcd1c..78e28f4
Fast-forward
The 'checkout' correctly reports that 'foo' is building on (local)
'master'. 'pull' works as expected, of course.
Now, here is the bug part. The same thing on 'bar':
: gitster track/foo; git checkout bar
Switched to branch 'bar'
Your branch is based on 'master', but the upstream is gone.
(use git branch --unset-upstream to fixup)
It knows about 'master', but what is the upstream is gone? It is
our local repository and it definitely is not gone.
But 'pull' of course works as expected (this behaviour is older and
stable for a long time since even before @{upstream} was invented).
: gitster track/bar; git pull
From .
* branchmaster - FETCH_HEAD
Updating abdcd1c..78e28f4
Fast-forward
I suspect that the real culprit is somewhere in wt-status.c
: gitster track/bar; git status
On branch bar
Your branch is based on 'master', but the upstream is gone.
(use git branch --unset-upstream to fixup)
nothing to commit, working directory clean
Resolving @{upstream} works just fine for both.
: gitster track/bar; git rev-parse --symbolic-full-name foo@{upstream}
refs/heads/master
: gitster track/bar; git rev-parse --symbolic-full-name bar@{upstream}
refs/heads/master
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/3] setup_pager: set MORE=R
Jeff King p...@peff.net writes: The safest thing would be to turn off auto-color when LESS (or any of the pager environment variables) is set at all (and not worry about whether R is set; only know that _we_ are not setting it, so we cannot count on it). But that would potentially inconvenience a lot of people whose default color would suddenly go away. That is just as safe as disabling color for everybody, isn't it? Half of existing users have LESS with R, and the other half do not have LESS at all. The former will be harmed, the latter will not see any difference. Oh, and then new users who do not know R for LESS will not even notice that Git could support coloured output. Those among them who read manpages and find --color option will then see ESC[33m in their output and we are back to where we started X-. So I think we are already at the safest place. Those who see ESC[33m will know they are missing some good stuff and can ask around, which is better than doing anything else at this point. I think that is the same conclusion as yours, there is nothing to be done. -- To unsubscribe from this list: send the line 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/3] setup_pager: set MORE=R
On 02/04/2014 14:48, Jeff King wrote: But this is not a build-time problem, but rather a run-time problem. We do not know what the environment of the user will be at run-time. Ok, git can test the pager on the given system before its first use and remember the result in ~/.git for later use for example. Such 'experimental' approach is maybe more reliable. Yuri -- To unsubscribe from this list: send the line 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/9] remerge diff proof of concept/RFC
Thomas Rast t...@thomasrast.ch writes: log --remerge-diff: show what the conflict resolution changed Yay ;-) This series explores another angle, which I call remerge diff. It works by re-doing the merge in core, using features from patches 1-3 and 7. Likely that will result in conflicts, which are formatted in the usual way. Then it diffs this remerge against the merge's tree that is recorded in history. Yup, that is the obvious way to recreate what would have been shown to the person who recorded the merge result. I like that approach. So I would welcome comments, and/or better ideas, on the following proposed resolution: * Implement what I described last, to take care of delete/modify conflicts. Naively, I would have thought that - If the recorded result of the merge does not have the path, then show the deletion of the contents relative to the side that modified it. - If the recorded result of the merge has the path, then show the change between the side that modified it and the recorded result. might be more useful. Then we will know what we lost in full (if the recorded result is to delete it), but it is very likely that we won't see anything if the recorded result blindly took what the modifying side left. Comparing with the synthetic stages 2+3 will at least show us there was something funky going on, so your approach would be reasonable in this case. * Punt on more complex conflicts, by removing those files from the index, and emitting a warning about those files instead. Sensible. -- To unsubscribe from this list: send the line 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] repack.c: rename and unlink pack file if it exists
This comment in builtin/repack.c:
First see if there are packs of the same name and if so
if we can move them out of the way (this can happen if we
repacked immediately after packing fully).
When a repo was fully repacked, and is repacked again, we may run
into the situation that new packfiles have the same name as
already existing ones (traditionally packfiles have been named after
the list of names of objects in them, so repacking all the objects
in a single pack would have produced a packfile with the same name).
The logic is to rename the existing ones into filename like
old-XXX, create the new ones and then remove the old- ones.
When something went wrong in the middle, this sequence is rolled
back by renaming the old- files back.
The renaming into old- did not work as designed, because
file_exists() was done on the wrong file name. This could give
problems for a repo on a network share under Windows, as reported by
Jochen Haag zwanzi...@googlemail.com.
Formulate the filenames objects/pack/pack-.{pack,idx} correctly
(the code originally lacked pack- prefix when checking if the file
exists).
Also rename the variables to match what they are used for:
fname for the final name of the new packfile, fname_old for the name
of the existing one, and fname_tmp for the temporary name pack-objects
created the new packfile in.
Signed-off-by: Torsten Bögershausen tbo...@web.de
Signed-off-by: Junio C Hamano gits...@pobox.com
---
* Somehow this came to my private mailbox without Cc to list, so I
am forwarding it.
I think with 1190a1ac (pack-objects: name pack files after
trailer hash, 2013-12-05), repacking the same set of objects may
have less chance of producing colliding names, especially if you
are on a box with more than one core, but it still would be a
good idea to get this part right in the upcoming release.
The bug in the first hunk will cause us not to find any existing
file, even when there is a name clash. And then we try to
immediately the final rename without any provision for rolling
back. The other two hunks are pure noise renaming variables.
I think the patch looks correct, but I'd appreciate a second set
of eyeballs.
Thanks.
builtin/repack.c | 24
1 file changed, 12 insertions(+), 12 deletions(-)
diff --git a/builtin/repack.c b/builtin/repack.c
index bca7710..3b01353 100644
--- a/builtin/repack.c
+++ b/builtin/repack.c
@@ -260,7 +260,7 @@ int cmd_repack(int argc, const char **argv, const char
*prefix)
for_each_string_list_item(item, names) {
for (ext = 0; ext 2; ext++) {
char *fname, *fname_old;
- fname = mkpathdup(%s/%s%s, packdir,
+ fname = mkpathdup(%s/pack-%s%s, packdir,
item-string, exts[ext]);
if (!file_exists(fname)) {
free(fname);
@@ -316,33 +316,33 @@ int cmd_repack(int argc, const char **argv, const char
*prefix)
/* Now the ones with the same name are out of the way... */
for_each_string_list_item(item, names) {
for (ext = 0; ext 2; ext++) {
- char *fname, *fname_old;
+ char *fname, *fname_tmp;
struct stat statbuffer;
fname = mkpathdup(%s/pack-%s%s,
packdir, item-string, exts[ext]);
- fname_old = mkpathdup(%s-%s%s,
+ fname_tmp = mkpathdup(%s-%s%s,
packtmp, item-string, exts[ext]);
- if (!stat(fname_old, statbuffer)) {
+ if (!stat(fname_tmp, statbuffer)) {
statbuffer.st_mode = ~(S_IWUSR | S_IWGRP |
S_IWOTH);
- chmod(fname_old, statbuffer.st_mode);
+ chmod(fname_tmp, statbuffer.st_mode);
}
- if (rename(fname_old, fname))
- die_errno(_(renaming '%s' failed), fname_old);
+ if (rename(fname_tmp, fname))
+ die_errno(_(renaming '%s' into '%s' failed),
fname_tmp, fname);
free(fname);
- free(fname_old);
+ free(fname_tmp);
}
}
/* Remove the old- files */
for_each_string_list_item(item, names) {
for (ext = 0; ext 2; ext++) {
- char *fname;
- fname = mkpath(%s/old-pack-%s%s,
+ char *fname_old;
+ fname_old = mkpath(%s/old-%s%s,
packdir,
item-string,
exts[ext]);
-
Re: [PATCH] repack.c: rename and unlink pack file if it exists
Junio C Hamano gits...@pobox.com writes:
This comment in builtin/repack.c:
...
Oops; there was supposed to be these lines before anythning else:
From: Torsten Bögershausen tbo...@web.de
Date: Sun Feb 2 16:09:56 2014 +0100
First see if there are packs of the same name and if so
if we can move them out of the way (this can happen if we
repacked immediately after packing fully).
When a repo was fully repacked, and is repacked again, we may run
into the situation that new packfiles have the same name as
already existing ones (traditionally packfiles have been named after
the list of names of objects in them, so repacking all the objects
in a single pack would have produced a packfile with the same name).
The logic is to rename the existing ones into filename like
old-XXX, create the new ones and then remove the old- ones.
When something went wrong in the middle, this sequence is rolled
back by renaming the old- files back.
The renaming into old- did not work as designed, because
file_exists() was done on the wrong file name. This could give
problems for a repo on a network share under Windows, as reported by
Jochen Haag zwanzi...@googlemail.com.
Formulate the filenames objects/pack/pack-.{pack,idx} correctly
(the code originally lacked pack- prefix when checking if the file
exists).
Also rename the variables to match what they are used for:
fname for the final name of the new packfile, fname_old for the name
of the existing one, and fname_tmp for the temporary name pack-objects
created the new packfile in.
Signed-off-by: Torsten Bögershausen tbo...@web.de
Signed-off-by: Junio C Hamano gits...@pobox.com
---
* Somehow this came to my private mailbox without Cc to list, so I
am forwarding it.
I think with 1190a1ac (pack-objects: name pack files after
trailer hash, 2013-12-05), repacking the same set of objects may
have less chance of producing colliding names, especially if you
are on a box with more than one core, but it still would be a
good idea to get this part right in the upcoming release.
The bug in the first hunk will cause us not to find any existing
file, even when there is a name clash. And then we try to
immediately the final rename without any provision for rolling
back. The other two hunks are pure noise renaming variables.
I think the patch looks correct, but I'd appreciate a second set
of eyeballs.
Thanks.
builtin/repack.c | 24
1 file changed, 12 insertions(+), 12 deletions(-)
diff --git a/builtin/repack.c b/builtin/repack.c
index bca7710..3b01353 100644
--- a/builtin/repack.c
+++ b/builtin/repack.c
@@ -260,7 +260,7 @@ int cmd_repack(int argc, const char **argv, const char
*prefix)
for_each_string_list_item(item, names) {
for (ext = 0; ext 2; ext++) {
char *fname, *fname_old;
- fname = mkpathdup(%s/%s%s, packdir,
+ fname = mkpathdup(%s/pack-%s%s, packdir,
item-string, exts[ext]);
if (!file_exists(fname)) {
free(fname);
@@ -316,33 +316,33 @@ int cmd_repack(int argc, const char **argv, const char
*prefix)
/* Now the ones with the same name are out of the way... */
for_each_string_list_item(item, names) {
for (ext = 0; ext 2; ext++) {
- char *fname, *fname_old;
+ char *fname, *fname_tmp;
struct stat statbuffer;
fname = mkpathdup(%s/pack-%s%s,
packdir, item-string, exts[ext]);
- fname_old = mkpathdup(%s-%s%s,
+ fname_tmp = mkpathdup(%s-%s%s,
packtmp, item-string, exts[ext]);
- if (!stat(fname_old, statbuffer)) {
+ if (!stat(fname_tmp, statbuffer)) {
statbuffer.st_mode = ~(S_IWUSR | S_IWGRP |
S_IWOTH);
- chmod(fname_old, statbuffer.st_mode);
+ chmod(fname_tmp, statbuffer.st_mode);
}
- if (rename(fname_old, fname))
- die_errno(_(renaming '%s' failed), fname_old);
+ if (rename(fname_tmp, fname))
+ die_errno(_(renaming '%s' into '%s' failed),
fname_tmp, fname);
free(fname);
- free(fname_old);
+ free(fname_tmp);
}
}
/* Remove the old- files */
for_each_string_list_item(item, names) {
for (ext = 0; ext 2; ext++) {
- char *fname;
- fname = mkpath(%s/old-pack-%s%s,
+ char *fname_old;
+
Re: [PATCH 2/2] reset: support --mixed --intent-to-add mode
On Tue, Feb 04, 2014 at 02:25:25PM -0800, Junio C Hamano wrote: Junio C Hamano gits...@pobox.com writes: While I do not have any problem with adding an optional keep lost paths as intent-to-add entries feature, I am not sure why this has to be so different from the usual add-cache-entry codepath. The if/elseif chain you are touching inside this loop does: - If the tree you are resetting to has something at the path (which is different from the current index, obviously), create a cache entry to represent that state from the tree and stuff it in the index; - Otherwise, the tree you are resetting to does not have that path. We used to say remove it from the index, but now we have an option to instead add it as an intent-to-add entry. So, why doesn't the new codepath do exactly the same thing as the first branch of the if/else chain and call add_cache_entry but with a ce marked with CE_INTENT_TO_ADD? That would parallel what happens in git add -N better, I would think, no? In other words, something along this line, perhaps? snip Yes. But you need something like this on top to actually set CE_INTENT_TO_ADD -- 8 -- diff --git a/read-cache.c b/read-cache.c index 325d193..87f1367 100644 --- a/read-cache.c +++ b/read-cache.c @@ -585,6 +585,7 @@ void mark_intent_to_add(struct cache_entry *ce) if (write_sha1_file(, 0, blob_type, sha1)) die(cannot create an empty blob in the object database); hashcpy(ce-sha1, sha1); + ce-ce_flags |= CE_INTENT_TO_ADD; } int add_to_index(struct index_state *istate, const char *path, struct stat *st, int flags) -- 8 -- -- To unsubscribe from this list: send the line 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] repack.c: rename and unlink pack file if it exists
On Tue, Feb 04, 2014 at 03:40:15PM -0800, Junio C Hamano wrote: * Somehow this came to my private mailbox without Cc to list, so I am forwarding it. I think with 1190a1ac (pack-objects: name pack files after trailer hash, 2013-12-05), repacking the same set of objects may have less chance of producing colliding names, especially if you are on a box with more than one core, but it still would be a good idea to get this part right in the upcoming release. Actually, since 1190a1ac, if you have repacked and gotten the same pack name, then you do not have to do any rename dance at all; you can throw away what you just generated because you know that it is byte-for-byte identical. You could collide with a pack created by an older version of git that used the original scheme, but that is quite unlikely (on the order of 2^-160). -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: 'git log' escape symbols shown as ESC[33 and ESC[m
On 01/16/2014 17:47, Jeff King wrote: Are you using less as your pager (it is the default in git unless you have set your PAGER environment variable)? If so, do you have the R option set to pass through ANSI codes? Git will set this automatically in your LESS variable if you do not already have such a variable (but it will not touch it if you already have it set, and are missing R). I think the 'experimental' approach is simpler and better. When the git command requiring pager is first run, git would run the pager with some simple one line escape sequence, and see if sequence is preserved. If it was preserved, git should just run with escape sequences. If the pager destroyed the sequence, git should issue a warning to the user: git: your default pager PAGER=more doesn't pass escape sequences, and they will be disabled them. You can revert this decision by changing the file ~/.git/pager.conf This way: * damaged sequences will not show up by default * colors will be displayed by default when possible * user would be informed, and will have a clear choice * this is easy to implement, and no elaborate and obscure reasoning should be employed in the implementation For me, if git would have told me that my pager doesn't support escape sequences, I could have taken it from there. Yuri -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: 'git log' escape symbols shown as ESC[33 and ESC[m
On Tue, Feb 04, 2014 at 05:24:55PM -0800, Yuri wrote:
I think the 'experimental' approach is simpler and better.
When the git command requiring pager is first run, git would run the
pager with some simple one line escape sequence, and see if sequence
is preserved.
See how? If less's stdout is not connected to a terminal, it simply
passes through the output as-is. E.g., try:
foo() {
# blue foo
printf '\x1b[34mfoo\x1b[m'
}
unset LESS
foo | less
which has the problematic output. Now try:
foo | less | cat
which passes through the ANSI codes unscathed. I have no idea how other
pagers behave. So I think this is going back down the road of
hard-coding lots of pager-specific behaviors.
-Peff
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/3] setup_pager: set MORE=R
On Feb 4, 2014, at 14:12, Jeff King wrote: On Tue, Jan 21, 2014 at 11:23:30AM -0800, Junio C Hamano wrote: does complicate the point of my series, which was to add more intimate logic about how we handle LESS. ... return !x || strchr(x, 'R'); [...] I am not sure if it is even a good idea for us to have so intimate logic for various pagers in the first place. I'd seriously wonder if it is better to take this position: A platform packager who sets the default pager and/or the default environment for the pager at the build time, or an individual user who tells your Git what pager you want to use and/or with what setting that pager should be run under with environment variables. These people ought to know far better than Git what their specific choices do. Do not try to second-guess them. Sorry for letting this topic lapse; I wanted to look at some possible Makefile improvements, which I'll be posting in a moment. I did want to address this point, though. If we are just talking about packagers and defaults (e.g., setting MORE=R on FreeBSD), then I agree that the right tool for the job is empowering the packagers, and the Makefile knob we've discussed does that. [snip] It seems a shame to me that we cannot do better for such users. However, the level of intimacy with the pager is getting to be a bit too much for my taste, and the saving grace is that not that many people set LESS themselves (and git is widely enough known that R is a common recommendation when people do). So it's probably the lesser of two evils to ignore the situation, and let people who run into it find the answers on the web. So I think there is nothing to be done. But I did want to mention it in case somebody else can come up with some clever solution. :) I think we can do better without adding intimate pager knowledge. And at the same time make it a simple matter of tweaking the Makefile to deal with new pagers on specific systems. There are two parts to the proposal. Part 1 Add a new configuration item which I will call pager.env for now that can have multiple values of the form ENV=value (whether multiple lines or whitespace separated on a single line to be decided later). On a system where more can support color by setting MORE=-R it might look like this: [pager] env = LESS=-FRSX LV=-c MORE=-R On a system where more does not it might look like this: [pager] env = LESS=-FRSX LV=-c The default value of pager.env is to be configured in a system- specific way (i.e. Makefile knob) at build time. Then, if Git is going to output color AND start a pager (that logic remains unchanged by this proposal), then it processes the pager.env value by examining each ENV=value setting and if the environment variable ENV is NOT already set, then sets it to value before starting the pager. This is mostly a clean up and shouldn't really be controversial since before commit e54c1f2d2 the system basically behaved like this where pager.env was always set to LESS=FRSX and after it behaves as though pager.env is always set to LESS=FRSX LV=-c. Part 2 Alongside the current false/never, always, true/auto settings for color.ui a new carefully setting is introduced and the color.ui default if not set is changed from auto (commit 4c7f1819) to the new carefully setting. Why a new setting? So that people who have explicitly set color.ui to auto (or one of the other values) will not be affected by the proposed new logic. Another new configuration item which I will call pager.carefully for now is introduced that again can have multiple values of the form name=ENV. It might look like this: [pager] carefully = less=LESS LV=lv more=MORE Again the default value of pager.carefully can be configured in a system-specific way (i.e. Makefile knob) at build time. If color.ui is set to carefully, then the logic is as follows: a) Decide whether to output color the same way as for color.ui=auto b) Select the pager the same way as for color.ui=auto c) If (a) and (b) have selected a pager AND enabled color output then check to see if the selected pager appears in pager.carefully as one of the name values (perhaps a suffix match on the first whitespace- separated word of the selected pager value). d) If the selected pager does not appear in pager.carefully, disable color output. e) If the selected pager appears in pager.carefully, but the ENV associated with it does NOT appear in pager.env, disable color output. f) If the pager appears in pager.carefully, but the ENV associated with it is already set, disable color output. g) Otherwise, go ahead with color output as the change in Part 1 above will properly set the pager's options to enable it. This has the following advantages: 1. No intimate pager knowledge. 2. Color output will be selected on supported systems by
[PATCH v3] l10n: de.po: translate 28 new messages
Translate 28 new messages came from git.pot update in df49095 (l10n: git.pot: v1.9 round 1 (27 new, 11 removed) and d57b24b (l10n: git.pot: v1.9 round 2 (1 new)). Signed-off-by: Ralf Thielow ralf.thie...@gmail.com --- v3 adds one new message from l10n rnd 2. po/de.po | 92 1 file changed, 46 insertions(+), 46 deletions(-) diff --git a/po/de.po b/po/de.po index 7f8aa75..432a2b5 100644 --- a/po/de.po +++ b/po/de.po @@ -442,9 +442,9 @@ msgstr[0] vor %lu Jahr msgstr[1] vor %lu Jahren #: diffcore-order.c:24 -#, fuzzy, c-format +#, c-format msgid failed to read orderfile '%s' -msgstr Fehler beim Lesen des Objektes '%s'. +msgstr Fehler beim Lesen der Reihenfolgedatei '%s'. #: diff.c:113 #, c-format @@ -975,21 +975,23 @@ msgid There is nothing to exclude from by :(exclude) patterns.\n Perhaps you forgot to add either ':/' or '.' ? msgstr +Es gibt nichts, was durch :(exclude) Muster ausgeschlossen wird.\n +Vielleicht haben Sie vergessen entweder ':/' oder '.' hinzuzufügen? #: remote.c:753 -#, fuzzy, c-format +#, c-format msgid Cannot fetch both %s and %s to %s -msgstr Kann keine Commit-Beschreibung für %s bekommen +msgstr Kann 'fetch' nicht für %s und %s nach %s ausführen. #: remote.c:757 #, c-format msgid %s usually tracks %s, not %s -msgstr +msgstr %s folgt üblicherweise %s, nicht %s #: remote.c:761 #, c-format msgid %s tracks both %s and %s -msgstr +msgstr %s folgt %s und %s #. #. * This last possibility doesn't occur because @@ -997,9 +999,8 @@ msgstr #. * the end of the list. #. #: remote.c:769 -#, fuzzy msgid Internal error -msgstr interner Fehler +msgstr Interner Fehler #: remote.c:1871 #, c-format @@ -1575,33 +1576,28 @@ msgid both modified: msgstr von beiden geändert: #: wt-status.c:275 -#, fuzzy msgid new file -msgstr neue Datei: %s +msgstr neue Datei #: wt-status.c:277 msgid copied -msgstr +msgstr kopiert #: wt-status.c:279 -#, fuzzy msgid deleted msgstr gelöscht #: wt-status.c:285 -#, fuzzy msgid typechange -msgstr Typänderung: %s +msgstr Typänderung #: wt-status.c:287 -#, fuzzy msgid unknown -msgstr unbekannt:%s +msgstr unbekannt #: wt-status.c:289 -#, fuzzy msgid unmerged -msgstr zusammenführen +msgstr nicht zusammengeführt #: wt-status.c:336 msgid new commits, @@ -1633,6 +1629,8 @@ msgid Do not touch the line above.\n Everything below will be removed. msgstr +Ändern Sie nicht die Zeile oberhalb.\n +Alles unterhalb wird entfernt. #: wt-status.c:899 msgid You have unmerged paths. @@ -3991,14 +3989,14 @@ msgid reference repository '%s' is not a local repository. msgstr Referenziertes Repository '%s' ist kein lokales Repository. #: builtin/clone.c:256 -#, fuzzy, c-format +#, c-format msgid reference repository '%s' is shallow -msgstr Referenziertes Repository '%s' ist kein lokales Repository. +msgstr Referenziertes Repository '%s' ist unvollständig (shallow). #: builtin/clone.c:259 -#, fuzzy, c-format +#, c-format msgid reference repository '%s' is grafted -msgstr referenziert Repository +msgstr Referenziertes Repository '%s' ist gesondert eingehängt (graft). #: builtin/clone.c:321 #, c-format @@ -4099,16 +4097,16 @@ msgstr #: builtin/clone.c:805 msgid source repository is shallow, ignoring --local -msgstr +msgstr Quelle ist ein unvollständiges Repository (shallow), ignoriere --local #: builtin/clone.c:810 msgid --local is ignored msgstr --local wird ignoriert #: builtin/clone.c:814 builtin/fetch.c:1119 -#, fuzzy, c-format +#, c-format msgid depth %s is not a positive number -msgstr Objekt %s ist kein Blob +msgstr Tiefe %s ist keine positive Zahl #: builtin/clone.c:824 #, c-format @@ -5215,9 +5213,8 @@ msgid default mode for recursion msgstr Standard-Modus für Rekursion #: builtin/fetch.c:109 -#, fuzzy msgid accept refs that update .git/shallow -msgstr Konnte aktualisierte .gitmodules-Datei nicht lesen +msgstr akzeptiert Referenzen die .git/shallow aktualisieren #: builtin/fetch.c:347 msgid Couldn't find remote ref HEAD @@ -5287,7 +5284,8 @@ msgstr %s hat nicht alle erforderlichen Objekte gesendet\n #: builtin/fetch.c:579 #, c-format msgid reject %s because shallow roots are not allowed to be updated -msgstr +msgstr %s wurde zurückgewiesen, da Ursprungs-Commits von unvollständigen\n +Repositories (shallow) nicht aktualisiert werden dürfen. #: builtin/fetch.c:667 builtin/fetch.c:750 #, c-format @@ -7182,9 +7180,8 @@ msgid git merge-base --is-ancestor commit commit msgstr git merge-base --is-ancestor Commit Commit #: builtin/merge-base.c:33 -#, fuzzy msgid git merge-base --fork-point ref [commit] -msgstr git merge-base --is-ancestor Commit Commit +msgstr git merge-base --fork-point Referenz [Commit] #: builtin/merge-base.c:214 msgid output all common ancestors @@ -7204,7 +7201,7 @@ msgstr ist der Erste ein Vorgänger-Commit von dem Anderen? #: builtin/merge-base.c:222 msgid find where
[PATCH 2/3] diff_scoreopt_parse(): use skip_prefix_if_present()
Whenever skip_prefix_defval() was called, opt was still equal to
*opt_p. So we can use skip_prefix_if_present() instead.
Signed-off-by: Michael Haggerty mhag...@alum.mit.edu
---
diff.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/diff.c b/diff.c
index d629cc5..a07fe9d 100644
--- a/diff.c
+++ b/diff.c
@@ -3890,13 +3890,13 @@ static int diff_scoreopt_parse(const char **opt_p)
*opt_p = opt;
if (cmd == '-') {
/* convert the long-form arguments into short-form versions */
- if ((opt = skip_prefix_defval(opt, break-rewrites, *opt_p))
!= *opt_p) {
+ if ((opt = skip_prefix_if_present(opt, break-rewrites)) !=
*opt_p) {
if (*opt == 0 || *opt++ == '=')
cmd = 'B';
- } else if ((opt = skip_prefix_defval(opt, find-copies,
*opt_p)) != *opt_p) {
+ } else if ((opt = skip_prefix_if_present(opt, find-copies))
!= *opt_p) {
if (*opt == 0 || *opt++ == '=')
cmd = 'C';
- } else if ((opt = skip_prefix_defval(opt, find-renames,
*opt_p)) != *opt_p) {
+ } else if ((opt = skip_prefix_if_present(opt, find-renames))
!= *opt_p) {
if (*opt == 0 || *opt++ == '=')
cmd = 'M';
}
--
1.8.5.3
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 3/3] rev_is_head(): use skip_prefix()
Change the logic a little bit so that we can use skip_prefix() instead
of doing pointer arithmetic with hardcoded numbers.
Signed-off-by: Michael Haggerty mhag...@alum.mit.edu
---
builtin/show-branch.c | 9 +
1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/builtin/show-branch.c b/builtin/show-branch.c
index f2c3b19..aee7fe5 100644
--- a/builtin/show-branch.c
+++ b/builtin/show-branch.c
@@ -475,14 +475,15 @@ static void snarf_refs(int head, int remotes)
static int rev_is_head(const char *head, int headlen, const char *name,
unsigned char *head_sha1, unsigned char *sha1)
{
+ const char *short_name;
+
if ((!head[0]) ||
(head_sha1 sha1 hashcmp(head_sha1, sha1)))
return 0;
head = skip_prefix_if_present(head, refs/heads/);
- if (starts_with(name, refs/heads/))
- name += 11;
- else if (starts_with(name, heads/))
- name += 6;
+ if ((short_name = skip_prefix(name, refs/heads/)) ||
+ (short_name = skip_prefix(name, heads/)))
+ return !strcmp(head, short_name);
return !strcmp(head, name);
}
--
1.8.5.3
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 0/3] Add a function skip_prefix_if_present()
These patches apply on top of gitster/nd/more-skip-prefix. I noticed that Duy's new function skip_prefix_defval() was mostly being called with its first and third arguments the same. So introduce a function skip_prefix_if_present() that implements this pattern. Michael Haggerty (3): Add and use a function skip_prefix_if_present() diff_scoreopt_parse(): use skip_prefix_if_present() rev_is_head(): use skip_prefix() builtin/checkout.c| 4 ++-- builtin/fast-export.c | 2 +- builtin/merge.c | 2 +- builtin/show-branch.c | 15 --- diff.c| 6 +++--- git-compat-util.h | 5 + git.c | 2 +- notes.c | 4 ++-- refs.c| 2 +- wt-status.c | 4 ++-- 10 files changed, 26 insertions(+), 20 deletions(-) -- 1.8.5.3 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 1/3] Add and use a function skip_prefix_if_present()
Most of the calls to skip_prefix_defval() had equal first and third
arguments, with the effect of skipping the prefix if present, but
otherwise returning the original string. So define a new function
that does exactly that.
Signed-off-by: Michael Haggerty mhag...@alum.mit.edu
---
builtin/checkout.c| 4 ++--
builtin/fast-export.c | 2 +-
builtin/merge.c | 2 +-
builtin/show-branch.c | 6 +++---
git-compat-util.h | 5 +
git.c | 2 +-
notes.c | 4 ++--
refs.c| 2 +-
wt-status.c | 4 ++--
9 files changed, 18 insertions(+), 13 deletions(-)
diff --git a/builtin/checkout.c b/builtin/checkout.c
index 6531ed4..84682f1 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -1151,8 +1151,8 @@ int cmd_checkout(int argc, const char **argv, const char
*prefix)
const char *argv0 = argv[0];
if (!argc || !strcmp(argv0, --))
die (_(--track needs a branch name));
- argv0 = skip_prefix_defval(argv0, refs/, argv0);
- argv0 = skip_prefix_defval(argv0, remotes/, argv0);
+ argv0 = skip_prefix_if_present(argv0, refs/);
+ argv0 = skip_prefix_if_present(argv0, remotes/);
argv0 = strchr(argv0, '/');
if (!argv0 || !argv0[1])
die (_(Missing branch name; try -b));
diff --git a/builtin/fast-export.c b/builtin/fast-export.c
index cd0a302..c87c7ea 100644
--- a/builtin/fast-export.c
+++ b/builtin/fast-export.c
@@ -476,7 +476,7 @@ static void handle_tag(const char *name, struct tag *tag)
}
}
- name = skip_prefix_defval(name, refs/tags/, name);
+ name = skip_prefix_if_present(name, refs/tags/);
printf(tag %s\nfrom :%d\n%.*s%sdata %d\n%.*s\n,
name, tagged_mark,
(int)(tagger_end - tagger), tagger,
diff --git a/builtin/merge.c b/builtin/merge.c
index 603f80a..7b01dcf 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -1109,7 +1109,7 @@ int cmd_merge(int argc, const char **argv, const char
*prefix)
*/
branch = branch_to_free = resolve_refdup(HEAD, head_sha1, 0, flag);
if (branch)
- branch = skip_prefix_defval(branch, refs/heads/, branch);
+ branch = skip_prefix_if_present(branch, refs/heads/);
if (!branch || is_null_sha1(head_sha1))
head_commit = NULL;
else
diff --git a/builtin/show-branch.c b/builtin/show-branch.c
index 6078132..f2c3b19 100644
--- a/builtin/show-branch.c
+++ b/builtin/show-branch.c
@@ -284,7 +284,7 @@ static void show_one_commit(struct commit *commit, int
no_name)
pp_commit_easy(CMIT_FMT_ONELINE, commit, pretty);
pretty_str = pretty.buf;
}
- pretty_str = skip_prefix_defval(pretty_str, [PATCH] , pretty_str);
+ pretty_str = skip_prefix_if_present(pretty_str, [PATCH] );
if (!no_name) {
if (name name-head_name) {
@@ -478,7 +478,7 @@ static int rev_is_head(const char *head, int headlen, const
char *name,
if ((!head[0]) ||
(head_sha1 sha1 hashcmp(head_sha1, sha1)))
return 0;
- head = skip_prefix_defval(head, refs/heads/, head);
+ head = skip_prefix_if_present(head, refs/heads/);
if (starts_with(name, refs/heads/))
name += 11;
else if (starts_with(name, heads/))
@@ -810,7 +810,7 @@ int cmd_show_branch(int ac, const char **av, const char
*prefix)
has_head++;
}
if (!has_head)
- append_one_rev(skip_prefix_defval(head, refs/heads/,
head));
+ append_one_rev(skip_prefix_if_present(head,
refs/heads/));
}
if (!ref_name_cnt) {
diff --git a/git-compat-util.h b/git-compat-util.h
index 59265af..cff946c 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -368,6 +368,11 @@ static inline const char *skip_prefix(const char *str,
const char *prefix)
return skip_prefix_defval(str, prefix, NULL);
}
+static inline const char *skip_prefix_if_present(const char *str, const char
*prefix)
+{
+ return skip_prefix_defval(str, prefix, str);
+}
+
static inline int starts_with(const char *str, const char *prefix)
{
return skip_prefix(str, prefix) != NULL;
diff --git a/git.c b/git.c
index 321ae81..f3357d8 100644
--- a/git.c
+++ b/git.c
@@ -579,7 +579,7 @@ int main(int argc, char **av)
argc--;
handle_options(argv, argc, NULL);
if (argc 0) {
- argv[0] = skip_prefix_defval(argv[0], --, argv[0]);
+ argv[0] = skip_prefix_if_present(argv[0], --);
} else {
/* The user didn't specify a command; give them help */
commit_pager_choice();
diff --git a/notes.c b/notes.c
index 31f513b..15c49d8 100644
--- a/notes.c
+++ b/notes.c
@@ -1243,8 +1243,8 @@ static
Re: [PATCH 0/3] Add a function skip_prefix_if_present()
On 02/05/2014 07:25 AM, Michael Haggerty wrote:
These patches apply on top of gitster/nd/more-skip-prefix.
I noticed that Duy's new function skip_prefix_defval() was mostly
being called with its first and third arguments the same. So
introduce a function skip_prefix_if_present() that implements this
pattern.
I see I should have read the whole previous thread [1] before firing off
this patch series. What I learned when I read it just now:
* Johannes Sixt didn't think changes like the following improve
readability:
- if (starts_with(arg, --upload-pack=)) {
- args.uploadpack = arg + 14;
+ if ((optarg = skip_prefix(arg, --upload-pack=)) != NULL) {
+ args.uploadpack = optarg;
* René Scharfe submitted a patch to use a function parse_prefix()
(originally suggested by Peff) instead of Duy's suggested approach:
http://article.gmane.org/gmane.comp.version-control.git/239569
His patch appears to have been overlooked.
* Duy seemed to offer to rewrite his patch series, but I don't think
that it has happened yet.
And then the conversation was drowned by Christmas eggnog.
I don't have a strong feeling about (Duy's proposal plus my patches) vs.
(René's parse_prefix() approach). But I definitely *do* like the idea
of getting rid of all those awkward magic numbers everywhere.
Michael
[1]
http://thread.gmane.org/gmane.comp.version-control.git/239438/focus=239569
--
Michael Haggerty
mhag...@alum.mit.edu
http://softwareswirl.blogspot.com/
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
