Re: Git GSoC 2014
Jeff King writes: > - somebody to be the backup admin (I am assuming I'll be the primary > admin, but as always, if somebody else wants to...) I can be backup, if Shawn doesn't want it. > - ideas ideas ideas Here's my moonshot: --- 8< --- Replace object loading/writing layer by libgit2 Git reads objects from storage (loose and packed) through functions in sha1_file.c. Most commands only require very simple, opaque read and write access to the object storage. As a weatherballoon, show that it is feasible to use libgit2 git_odb_* routines for these simple callers. Aim for passing the git test suite using git_odb_* object storage access, except for tests that verify behavior in the face of storage corruption, replacement objects, alternate storage locations, and similar quirks. Of course it is even better if you pass the test suite without exception. Language: C Difficulty: hard Possible mentors: Thomas Rast and --- >8 --- That absolutely requires a co-mentor from the libgit2 side to do, however. Perhaps you could talk someone into it? ;-) Motivation: I believe that migrating to libgit2 is the better approach, medium term, than rewriting everything ourselves to be nice, clean and thread-safe. I took a shot a while ago at making the pack reading code thread-safe, but it's adding mess when we could simply replace it all by the already thread-safe libgit2 calls. It also helps shake out incompatibilities in libgit2. Downside: not listing "code merged" as a goal may not make the project as shiny, neither for Git nor for the student. -- Thomas Rast t...@thomasrast.ch -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Git GSoC 2014
Junio C Hamano writes: > Thomas Rast writes: > >> Downside: not listing "code merged" as a goal may not make the project >> as shiny, neither for Git nor for the student. > > I'd actually view that as an upside. This sounds like a good first > step for a feasibility study that is really necessary. > > I wonder why the handling of storage corruption and replacement > could be left broken, though. Is that because libgit2 has known > breakages in these areas, or is there some other reason? It's because I don't know enough about what libgit2's state is, and I wanted to keep the scope limited. Naturally, the next step would then be to implement the lacking functionality (if any) in libgit2 so that the test suite passes. I just don't know if that's trivial, or something for the "if we have time" section of the project, or too much work. (I did do a quick "can we reasonably link against libgit2" test where I gave git-cat-file a --libgit2 option that loads blobs with libgit2. There are some name collisions in the git_config* identifiers that need to be resolved, but otherwise it seems entirely possible.) -- Thomas Rast t...@thomasrast.ch -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Git GSoC 2014
David Kastrup writes: > Thomas Rast writes: > >> Motivation: I believe that migrating to libgit2 is the better approach, >> medium term, than rewriting everything ourselves to be nice, clean and >> thread-safe. I took a shot a while ago at making the pack reading code >> thread-safe, but it's adding mess when we could simply replace it all by >> the already thread-safe libgit2 calls. It also helps shake out >> incompatibilities in libgit2. > > That would either require forking libgit2 for Git use or stop dead any > contributions to that rather central part of the git codebase from > contributors not wanting their contributions to get reused in binary > proprietary software. > > It would also mean that no serious forward-going work (like developing > new packing formats or network protocols) can be done on a pure GPLv2 > codebase any more. So anybody insisting on contributing work under the > current Git license only would be locked out from working on significant > parts of Git and could no longer propose changes in central parts. > > Now this can all be repealed by the "developing the atomic bomb does not > mean that one has to use it" argument but even if one does not use it, > the world with and without it are different worlds and occupy mindshare > and suggest "solutions" and "diplomacy" involving it. > > So this is definitely a large step towards a situation where erosion of > the existing license and related parts of the community becomes more > attractive. > > There is the rationale "we can always say "no" at the end". How do you > explain this "no" to the student who invested significant amounts of > work into this, in a project proposed by the Git developers? > > This definitely should not be "we'll think about it if and when that > project is finished" material. Yes, all of this is true. However, you are painting a big devil on the wall. First, one very plausible outcome of such a project is that there is a more narrowly defined interface to the object-reading component of the git codebase, and that libgit2 can be "plugged in" at that interface instead of the existing routines. This would help both clean up our code, and test libgit2 against existing git tests. Your scenario above mostly applies if and when we really go the way of my dream and scrap the code that is in git. (I have similar long-term dreams for other git components like ref storage and diffs, but that's just me.) Second, how many contributions would actually have been prevented by GPLv2+LE licensing? The only data I have on this is libgit2/git.git-authors, which records who has consented for their _existing_ code to be relicensed. I consider this to be a higher barrier than contributing new code, since there's no clear gain for the author in the relicensing. That file is a sizeable list, and covers most contributions to sha1_file.c lately: [git shortlog -sn --since=2.year.ago sha1_file.c, edited in the answer from git.git-authors] ok 15 Jeff King ok 11 Michael Haggerty 6 Nguyễn Thái Ngọc Duy ok 5 Christian Couder ask 5 Thomas Rast ok 2 Brandon Casey 2 Heiko Voigt 2 Pete Wyckoff 1 Felipe Contreras 1 Joachim Schmitz 1 Johan Herland ask 1 Jonathan Nieder 1 Ramsay Allan Jones 1 Steven Walter 1 Vicent Marti (My "ask" is because of $DAYJOB legal reasons, and in fact the contributions above fall under the "ok before Oct 7, 2013" remark in git.git-authors.) It also includes an "ok" from Nicolas Pitre, who has been the driving force behind packv5 development. The only thing that makes me uneasy is that Duy is not in the list (Duy, have you been asked by libgit2 about possible relicensing?). Other than that, I do not see a cause for concern. Conversely, contributions to clean up that corner of code have not exactly been forthcoming at a great rate in the first place. The recent work I can recall off hand was bug fixing and the introduction of pack bitmaps. The only work that I know is within reach is the packv5 drafts that Nicolas and Duy tossed around. (It is an odd coincidence that this thread runs in parallel with [1]. If that gains some traction, more power to them.) So you may disagree, and other contributors should probably comment on their stance. But taking the above together, I think we stand to gain more than we would lose. [1] http://thread.gmane.org/gmane.comp.version-control.git/241965 -- Thomas Rast t...@thomasrast.ch -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFH] hackday and GSoC topic suggestions
Duy Nguyen writes: > On Sun, Feb 9, 2014 at 2:03 AM, Thomas Rast wrote: >> Easy: >> >> * Add -p 'e' when it fails to apply should offer an obvious way of >> starting from the original hunk (not the broken one) or both > > If it's too easy, you can add a command to change diff display > settings (--color-words, context size...) If you mean for GSoC, that violates the "previously suggested" rule -- I had it as a project proposal for at least two years and nobody was ever interested. -- Thomas Rast t...@thomasrast.ch -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Profiling support?
David Kastrup writes: > Looking in the Makefile, I just find support for coverage reports using > gcov. Whatever is there with "profile" in it seems to be for > profile-based compilation rather than using gprof. [...] > Is there a reason there are no prewired recipes or advice for using > gprof on git? Is there a way to get the work done, namely seeing the > actual distribution of call times (rather than iterations) using gcov so > that this is not necessary? No reason I'm aware of, other than that nobody ever wrote it. Note that I wouldn't exactly be surprised if the gcov targets had bitrotted without anyone noticing. I haven't heard of any heavy users. I originally wrote them to do some basic test coverage analysis, but that's about it. -- Thomas Rast t...@thomasrast.ch -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] diff: do not reuse_worktree_file for submodules
The GIT_EXTERNAL_DIFF calling code attempts to reuse existing worktree
files for the worktree side of diffs, for performance reasons.
However, that code also tries to do the same with submodules. This
results in calls to $GIT_EXTERNAL_DIFF where the old-file is a file of
the form "Submodule commit $sha1", but the new-file is a directory in
the worktree.
Fix it by never reusing a worktree "file" in the submodule case.
Reported-by: Grégory Pakosz
Signed-off-by: Thomas Rast
---
diff.c | 5 +++--
t/t4020-diff-external.sh | 30 +-
2 files changed, 32 insertions(+), 3 deletions(-)
diff --git a/diff.c b/diff.c
index 7c59bfe..e9a8874 100644
--- a/diff.c
+++ b/diff.c
@@ -2845,8 +2845,9 @@ static struct diff_tempfile *prepare_temp_file(const char
*name,
remove_tempfile_installed = 1;
}
- if (!one->sha1_valid ||
- reuse_worktree_file(name, one->sha1, 1)) {
+ if (!S_ISGITLINK(one->mode) &&
+ (!one->sha1_valid ||
+reuse_worktree_file(name, one->sha1, 1))) {
struct stat st;
if (lstat(name, &st) < 0) {
if (errno == ENOENT)
diff --git a/t/t4020-diff-external.sh b/t/t4020-diff-external.sh
index bcae35a..0446201 100755
--- a/t/t4020-diff-external.sh
+++ b/t/t4020-diff-external.sh
@@ -226,12 +226,13 @@ keep_only_cr () {
}
test_expect_success 'external diff with autocrlf = true' '
- git config core.autocrlf true &&
+ test_config core.autocrlf true &&
GIT_EXTERNAL_DIFF=./fake-diff.sh git diff &&
test $(wc -l < crlfed.txt) = $(cat crlfed.txt | keep_only_cr | wc -c)
'
test_expect_success 'diff --cached' '
+ test_config core.autocrlf true &&
git add file &&
git update-index --assume-unchanged file &&
echo second >file &&
@@ -239,4 +240,31 @@ test_expect_success 'diff --cached' '
test_cmp "$TEST_DIRECTORY"/t4020/diff.NUL actual
'
+test_expect_success 'clean up crlf leftovers' '
+ git update-index --no-assume-unchanged file &&
+ rm -f file* &&
+ git reset --hard
+'
+
+test_expect_success 'submodule diff' '
+ git init sub &&
+ ( cd sub && test_commit sub1 ) &&
+ git add sub &&
+ test_tick &&
+ git commit -m "add submodule" &&
+ ( cd sub && test_commit sub2 ) &&
+ write_script gather_pre_post.sh <<-\EOF &&
+ echo "$1 $4" # path, mode
+ cat "$2" # old file
+ cat "$5" # new file
+ EOF
+ GIT_EXTERNAL_DIFF=./gather_pre_post.sh git diff >actual &&
+ cat >expected <<-EOF &&
+ sub 16
+ Subproject commit $(git rev-parse HEAD:sub)
+ Subproject commit $(cd sub && git rev-parse HEAD)
+ EOF
+ test_cmp expected actual
+'
+
test_done
--
1.9.0.313.g3d0a325
--
To unsubscribe from this list: send the line "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: Profiling support?
David Kastrup writes: > Thomas Rast writes: > >> David Kastrup writes: >> >>> Looking in the Makefile, I just find support for coverage reports using >>> gcov. Whatever is there with "profile" in it seems to be for >>> profile-based compilation rather than using gprof. >> [...] >>> Is there a reason there are no prewired recipes or advice for using >>> gprof on git? Is there a way to get the work done, namely seeing the >>> actual distribution of call times (rather than iterations) using gcov so >>> that this is not necessary? >> >> No reason I'm aware of, other than that nobody ever wrote it. > > A solid testing/benchmarking framework would quite seem like a useful > GSoC project as it would make it easy for casual programmers to dip > their feet into their personal bottlenecks, and it would make it much > easier to find worthwhile hotspots for future projects taking the > challenge of speeding up core and/or specific operations. > >> Note that I wouldn't exactly be surprised if the gcov targets had >> bitrotted without anyone noticing. I haven't heard of any heavy users. >> I originally wrote them to do some basic test coverage analysis, but >> that's about it. > > I've managed to make use of the outer sandwich layers: the prepare and > the evaluate stuff. I ran my own tests for benchmarking though. Umm, are we even discussing the same thing here? Are you saying you ran profiling-instrumented code under the t/perf/ support code? Sounds nice... -- Thomas Rast t...@thomasrast.ch -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: diff weirdness (bug?)
Dario Bertini writes: > On 02/14/2014 09:03 PM, Junio C Hamano wrote: >> This is a combined diff, and yaml-related lines are added relative >> to your _other_ branch you are merging (notice these + are indented >> by one place). Relative to what you had at the tip of your branch >> before you started this operation that ended up conflicted, the >> half-merged result removes if/else that sets DIST_MODULE_PATH and >> replaces it with a single line (their +/- are on the first column, >> signifying that these are differences relative to the first parent, >> i.e. your state before you started the operation). >> >>> if we remove these 3 lines, we'll get this diff: >> >> With that understanding, I think the output after removing these >> three lines is perfectlyh understandable and correct. You are >> looking at the three lines that used to exist in the version you >> started from, that were missing from the other side. If you remoe >> them, it will show as removal from _your_ version (notice these - >> that shows what _you_ did manually are on the first column, saying >> that that is relative to _your_ version). >> > > Thank you, I was completely unaware of combined diffs. Still: I can't > see how this would explain the empty diff when deleting 4 lines instead > of 3. With a --cc diff (which it is: it says 'diff --cc' in the file headers) git doesn't show the combined diff for hunks that fully agree with one side. So if you (even manually) resolve the merge so that it fully matches one side, that will not show up in a --cc diff. -- Thomas Rast t...@thomasrast.ch -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Is there something like a blamed diff?
David Kastrup writes: > When comparing two branches, decorating the flat diff with the > respectively responsible commits seems like it would be nice to do/have > (the blame on the identical parts, in contrast, is not really > interesting). Is there any tool that provides something like that? This seems to come up every year or so: http://thread.gmane.org/gmane.comp.version-control.git/110369/focus=110383 -- Thomas Rast t...@thomasrast.ch -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Is there something like a blamed diff?
David Kastrup writes: > Thomas Rast writes: > >> David Kastrup writes: >> >>> When comparing two branches, decorating the flat diff with the >>> respectively responsible commits seems like it would be nice to do/have >>> (the blame on the identical parts, in contrast, is not really >>> interesting). Is there any tool that provides something like that? >> >> This seems to come up every year or so: >> >> http://thread.gmane.org/gmane.comp.version-control.git/110369/focus=110383 > > Nice. That one could likely be sped up by calling git-blame just once > on each file with multiple -L options. Yes. You'll note that the email predates the support for multiple -L options by a few years :-) I never had a need for such a script, I just wrote that in response to someone asking about it. If you find it useful, please clean it up for inclusion in contrib/. -- Thomas Rast t...@thomasrast.ch -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 0/8] log --remerge-diff
This is the second iteration of http://thread.gmane.org/gmane.comp.version-control.git/241565 Changes since the last version: * Dropped patches 4 and 5 (log --merge-bases) * Implemented the "full-file conflict" scheme explained in the previous cover letter: if the conflicted version is lacking one stage of a file, it synthesizes a conflict file of the form <<<<<<< === content of the stage that exists >>>>>>> (or the other way around if stage3 is missing). This occurs at least with delete/modify conflicts. * Implemented some basic handling of directory/file conflicts. I'm not completely happy yet -- see the NEEDSWORK comments -- but at least it gives consistent input to the diffing stage. This required access to the dir hash, so there's a new patch 7 that makes this possible. Patches 1-6 (used to be 1-3 and 6-8) are unchanged. Thomas Rast (8): 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 combine-diff: do not pass revs->dense_combined_merges redundantly Fold all merge diff variants into an enum merge-recursive: allow storing conflict hunks in index name-hash: allow dir hashing even when !ignore_case log --remerge-diff: show what the conflict resolution changed Documentation/merge-strategies.txt | 9 ++ Documentation/rev-list-options.txt | 7 + builtin/diff-files.c | 5 +- builtin/diff-tree.c| 2 +- builtin/diff.c | 12 +- builtin/fmt-merge-msg.c| 2 +- builtin/log.c | 9 +- builtin/merge.c| 1 - cache.h| 2 + combine-diff.c | 13 +- diff-lib.c | 13 +- diff.h | 6 +- log-tree.c | 304 - merge-recursive.c | 52 --- merge-recursive.h | 3 + name-hash.c| 19 ++- revision.c | 15 +- revision.h | 24 ++- submodule.c| 3 +- t/t3030-merge-recursive.sh | 33 t/t4213-log-remerge-diff.sh| 222 +++ 21 files changed, 678 insertions(+), 78 deletions(-) create mode 100755 t/t4213-log-remerge-diff.sh -- 1.9.0.313.g3d0a325 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 7/8] name-hash: allow dir hashing even when !ignore_case
The directory hash (for fast checks if the index already has a
directory) was only used in ignore_case mode and so depended on that
flag.
Make it generally available on request.
Signed-off-by: Thomas Rast
---
cache.h | 2 ++
name-hash.c | 19 ---
2 files changed, 14 insertions(+), 7 deletions(-)
diff --git a/cache.h b/cache.h
index dc040fb..e162021 100644
--- a/cache.h
+++ b/cache.h
@@ -276,6 +276,7 @@ struct index_state {
struct cache_tree *cache_tree;
struct cache_time timestamp;
unsigned name_hash_initialized : 1,
+has_dir_hash : 1,
initialized : 1;
struct hash_table name_hash;
struct hash_table dir_hash;
@@ -284,6 +285,7 @@ struct index_state {
extern struct index_state the_index;
/* Name hashing */
+extern void init_name_hash(struct index_state *istate, int force_dir_hash);
extern void add_name_hash(struct index_state *istate, struct cache_entry *ce);
extern void remove_name_hash(struct index_state *istate, struct cache_entry
*ce);
extern void free_name_hash(struct index_state *istate);
diff --git a/name-hash.c b/name-hash.c
index e5b6e1a..c8953be 100644
--- a/name-hash.c
+++ b/name-hash.c
@@ -141,16 +141,19 @@ static void hash_index_entry(struct index_state *istate,
struct cache_entry *ce)
*pos = ce;
}
- if (ignore_case && !(ce->ce_flags & CE_UNHASHED))
+ if (istate->has_dir_hash && !(ce->ce_flags & CE_UNHASHED))
add_dir_entry(istate, ce);
}
-static void lazy_init_name_hash(struct index_state *istate)
+void init_name_hash(struct index_state *istate, int force_dir_hash)
{
int nr;
if (istate->name_hash_initialized)
return;
+
+ istate->has_dir_hash = force_dir_hash || ignore_case;
+
if (istate->cache_nr)
preallocate_hash(&istate->name_hash, istate->cache_nr);
for (nr = 0; nr < istate->cache_nr; nr++)
@@ -161,7 +164,7 @@ static void lazy_init_name_hash(struct index_state *istate)
void add_name_hash(struct index_state *istate, struct cache_entry *ce)
{
/* if already hashed, add reference to directory entries */
- if (ignore_case && (ce->ce_flags & CE_STATE_MASK) == CE_STATE_MASK)
+ if (istate->has_dir_hash && (ce->ce_flags & CE_STATE_MASK) ==
CE_STATE_MASK)
add_dir_entry(istate, ce);
ce->ce_flags &= ~CE_UNHASHED;
@@ -181,7 +184,7 @@ void add_name_hash(struct index_state *istate, struct
cache_entry *ce)
void remove_name_hash(struct index_state *istate, struct cache_entry *ce)
{
/* if already hashed, release reference to directory entries */
- if (ignore_case && (ce->ce_flags & CE_STATE_MASK) == CE_HASHED)
+ if (istate->has_dir_hash && (ce->ce_flags & CE_STATE_MASK) == CE_HASHED)
remove_dir_entry(istate, ce);
ce->ce_flags |= CE_UNHASHED;
@@ -228,7 +231,7 @@ struct cache_entry *index_dir_exists(struct index_state
*istate, const char *nam
struct cache_entry *ce;
struct dir_entry *dir;
- lazy_init_name_hash(istate);
+ init_name_hash(istate, 0);
dir = find_dir_entry(istate, name, namelen);
if (dir && dir->nr)
return dir->ce;
@@ -250,7 +253,7 @@ struct cache_entry *index_file_exists(struct index_state
*istate, const char *na
unsigned int hash = hash_name(name, namelen);
struct cache_entry *ce;
- lazy_init_name_hash(istate);
+ init_name_hash(istate, 0);
ce = lookup_hash(hash, &istate->name_hash);
while (ce) {
@@ -286,9 +289,11 @@ void free_name_hash(struct index_state *istate)
if (!istate->name_hash_initialized)
return;
istate->name_hash_initialized = 0;
- if (ignore_case)
+ if (istate->has_dir_hash) {
/* free directory entries */
for_each_hash(&istate->dir_hash, free_dir_entry, NULL);
+ istate->has_dir_hash = 0;
+ }
free_hash(&istate->name_hash);
free_hash(&istate->dir_hash);
--
1.9.0.313.g3d0a325
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 5/8] 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
---
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" o
[PATCH v2 8/8] 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
---
Documentation/rev-list-options.txt | 7 +
log-tree.c | 298 +
merge-recursive.c | 3 +-
merge-recursive.h | 1 +
revision.c | 2 +
revision.h | 4 +-
t/t4213-log-remerge-diff.sh| 222 +++
7 files changed, 535 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 03533af..73264dc 100644
--- a/Documentation/rev-list-options.txt
+++ b/Documentation/rev-list-options.txt
@@ -790,6 +790,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 30b3063..9b831e9 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" };
@@ -723,6 +725,300 @@ static int do_diff_combined(struct rev_info *opt, struct
commit *commit)
}
/*
+ * Helpers for make_asymmetric_conflict_entries() below.
+ */
+static char *load_cache_entry_blob(struct cache_entry *entry,
+ unsigned long *size)
+{
+ enum object_type type;
+ void *data;
+
+ if (!entry)
+ return NULL;
+
+ data = read_sha1_file(entry->sha1, &type, size);
+ if (type != OBJ_BLOB)
+ die("BUG: load_cache_entry_blob for non-blob");
+
+ return data;
+}
+
+static void strbuf_append_cache_entry_blob(struct strbuf *sb,
+ struct cache_entry *entry)
+{
+ unsigned long size;
+ char *data = load_cache_entry_blob(entry, &size);;
+
+ if (!data)
+ return;
+
+ strbuf_add(sb, data, size);
+ free(data);
+}
+
+static void assemble_conflict_entry(struct strbuf *sb,
+ const char *branch1,
+ const char *branch2,
+ struct cache_entry *entry1,
+ struct cache_entry *entry2)
+{
+ strbuf_addf(sb, "<<<<<<< %s\n", branch1);
+ strbuf_append_cache_entry_blob(sb, entry1);
+ strbuf_addstr(sb, "===\n");
+ strbuf_append_cache_entry_blob(sb, entry2);
+ strbuf_addf(sb, ">>>>>>> %s\n", branch2);
+}
+
+/*
+ * For --remerge-diff, we need conflicted (<<<<<<< ... >>>>>>>)
+ * representations of as many conflicts as possible. Default conflict
+ * generation only applies to files that have all three stages.
+ *
+ * This function generates conflict hunk representations for files
+ * that have only one of stage 2 or 3. The corresponding side in the
+ * conflict hunk format will be empty. A stage 1, if any, will be
+ * dropped in the process.
+ */
+static void make_asymme
[PATCH v2 4/8] 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
---
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->d
[PATCH v2 1/8] merge-recursive: remove dead conditional in update_stages()
From: Thomas Rast 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 Signed-off-by: Junio C Hamano --- 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.0.313.g3d0a325 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 2/8] merge-recursive: internal flag to avoid touching the worktree
From: Thomas Rast
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
Signed-off-by: Junio C Hamano
---
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,
[PATCH v2 6/8] 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
---
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.0.313.g3d0a325
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 3/8] merge-recursive: -Xindex-only to leave worktree unchanged
From: Thomas Rast 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 --- 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[=];; 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.0.313.g3d0a325 -- To unsubscribe from this list: send the line "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] diff: do not reuse_worktree_file for submodules
Junio C Hamano writes:
> Thomas Rast writes:
>
>> @@ -2845,8 +2845,9 @@ static struct diff_tempfile *prepare_temp_file(const
>> char *name,
>> remove_tempfile_installed = 1;
>> }
>>
>> -if (!one->sha1_valid ||
>> -reuse_worktree_file(name, one->sha1, 1)) {
>> +if (!S_ISGITLINK(one->mode) &&
>> +(!one->sha1_valid ||
>> + reuse_worktree_file(name, one->sha1, 1))) {
>
> I agree with the goal/end result, but I have to wonder if the
> reuse_worktree_file() be the helper function that ought to
> encapsulate such a logic?
>
> Instead of feeding it an object name and a path, if we passed a
> diff_filespec to the helper, it would have access to the mode as
> well. It would result in a more intrusive change, so I'd prefer to
> see your patch applied first and then build such a refactor on top,
> perhaps like the attached.
I see that you already queued 721e727, which has the change you
described plus moving the S_ISGITLINK test into reuse_worktree_file.
The change looks good to me. However, two nits about the comments:
diff.c now says
/*
* Given a name and sha1 pair, if the index tells us the file in
* the work tree has that object contents, return true, so that
* prepare_temp_file() does not have to inflate and extract.
*/
static int reuse_worktree_file(const struct diff_filespec *spec, int
want_file)
{
const struct cache_entry *ce;
struct stat st;
int pos, len;
const char *name = spec->path;
const unsigned char *sha1 = spec->sha1;
/* reading the directory will not give us "Submodule commit XYZ" */
if (S_ISGITLINK(spec->mode))
return 0;
But the function comment is no longer accurate, and the comment about
the S_ISGITLINK exit is rather obscure if one doesn't know what the
callers want. So how about this on top?
diff.c | 17 +
1 file changed, 13 insertions(+), 4 deletions(-)
diff --git i/diff.c w/diff.c
index a342ea6..dabf913 100644
--- i/diff.c
+++ w/diff.c
@@ -2578,9 +2578,14 @@ void fill_filespec(struct diff_filespec *spec, const
unsigned char *sha1,
}
/*
- * Given a name and sha1 pair, if the index tells us the file in
- * the work tree has that object contents, return true, so that
- * prepare_temp_file() does not have to inflate and extract.
+ * Given a diff_filespec, determine if the corresponding worktree file
+ * can be used for diffing instead of reading the object from the
+ * repository.
+ *
+ * We normally try packfiles, worktree, loose objects in this order.
+ *
+ * If want_file=1 or git was compiled with NO_FAST_WORKING_DIRECTORY,
+ * the order is: worktree, packfiles, loose objects.
*/
static int reuse_worktree_file(const struct diff_filespec *spec, int want_file)
{
@@ -2590,7 +2595,11 @@ static int reuse_worktree_file(const struct
diff_filespec *spec, int want_file)
const char *name = spec->path;
const unsigned char *sha1 = spec->sha1;
- /* reading the directory will not give us "Submodule commit XYZ" */
+ /*
+* The diff representation of a submodule is "Submodule commit
+* XYZ", but in the worktree we have a directory. So they
+* never match.
+*/
if (S_ISGITLINK(spec->mode))
return 0;
--
Thomas Rast
t...@thomasrast.ch
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] sha1_file: fix delta_stack memory leak in unpack_entry
Junio C Hamano writes: > Jeff King writes: > >> On Fri, Feb 21, 2014 at 06:47:47AM +0700, Nguyễn Thái Ngọc Duy wrote: >> >>> This delta_stack array can grow to any length depending on the actual >>> delta chain, but we forget to free it. Normally it does not matter >>> because we use small_delta_stack[] from stack and small_delta_stack >>> can hold 64-delta chains, more than standard --depth=50 in pack-objects. >>> >>> Signed-off-by: Nguyễn Thái Ngọc Duy >>> --- >>> Found when trying to see if making some objects loose at this phase >>> could help git-blame and how many objects will be loosened. Gotta go >>> soon, didn't really test it, but I bet it'll work. >> >> This looks correct to me. > > This comes from abe601bb, right? The change looks correct to me, too. Ow, sorry about that. Thanks for the fix! -- Thomas Rast t...@thomasrast.ch -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Fwd: git-reviewed: linking commits to review discussion in git
Jeff King writes: > On Mon, Feb 17, 2014 at 03:12:48PM -0500, Murtuza Mukadam wrote: > >> We have linked peer review discussions on >> git@vger.kernel.org to their respective commits within the main >> git.git repository. You can view the linked reviews from 2012 >> until present in the GitHub repo at: >> https://github.com/mmukadam/git/tree/review > > Neat. We've experimented in the past with mapping commits back to > mailing list discussions. Thomas (cc'd) has a script that creates > git-notes trees mapping commits to the relevant message-id, which can > then be found in the list archive. > > To me, the interesting bits of such a project are: > > 1. How do we decide which messages led to which commits? There is > definitely some room for heuristics here, as patches are sometimes > tweaked in transit, or come in multiple stages (e.g., the original > patch, then somebody suggests a fixup on top). You might want to > compare your work with the script from Thomas here: > >http://repo.or.cz/w/trackgit.git Eh, or don't. My script nowadays uses Junio's suggestion of matching on (author, authordate) with a little bit of tweaking in case there is no match. The name/date match works for most cases even in slightly tweaked forms. (The very first version elaborately tried all sorts of things, including attempting to patch on master, next etc. to see where it applies, and turned out to be wy too slow.) I'm no longer convinced that there's anything a computer can do beyond (author, authordate), anyway. Perhaps someone with a clue in UIs -- that's definitely not me -- could make a website where users can complete or correct the autogenerated mappings to go further. -- Thomas Rast t...@thomasrast.ch -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 3/8] merge-recursive: -Xindex-only to leave worktree unchanged
Eric Sunshine writes: > On Sat, Feb 22, 2014 at 4:17 AM, Thomas Rast wrote: >> 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 >> --- >> 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[=];; >> is prefixed (or stripped from the beginning) to make the shape of >> two trees to match. >> >> +index-only;; > > s/;;/::/ I think ;; is actually correct: this continues the sub-list of options to the recursive strategy. The :: level lists the available strategies. >> + 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 > -- > To unsubscribe from this list: send the line "unsubscribe git" in > the body of a message to majord...@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- Thomas Rast t...@thomasrast.ch -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] diff: do not reuse_worktree_file for submodules
Thomas Rast writes:
> Junio C Hamano writes:
>
>> Thomas Rast writes:
>>
>>> @@ -2845,8 +2845,9 @@ static struct diff_tempfile *prepare_temp_file(const
>>> char *name,
>>> remove_tempfile_installed = 1;
>>> }
>>>
>>> - if (!one->sha1_valid ||
>>> - reuse_worktree_file(name, one->sha1, 1)) {
>>> + if (!S_ISGITLINK(one->mode) &&
>>> + (!one->sha1_valid ||
>>> +reuse_worktree_file(name, one->sha1, 1))) {
>>
>> I agree with the goal/end result, but I have to wonder if the
>> reuse_worktree_file() be the helper function that ought to
>> encapsulate such a logic?
>>
>> Instead of feeding it an object name and a path, if we passed a
>> diff_filespec to the helper, it would have access to the mode as
>> well. It would result in a more intrusive change, so I'd prefer to
>> see your patch applied first and then build such a refactor on top,
>> perhaps like the attached.
>
> I see that you already queued 721e727, which has the change you
> described plus moving the S_ISGITLINK test into reuse_worktree_file.
> The change looks good to me.
I spoke too soon; it breaks the test I wrote to cover this case, for a
reason that gives me a headache.
When we hit the conditional
>>> - if (!one->sha1_valid ||
>>> - reuse_worktree_file(name, one->sha1, 1)) {
>>> + if (!S_ISGITLINK(one->mode) &&
>>> + (!one->sha1_valid ||
>>> +reuse_worktree_file(name, one->sha1, 1))) {
sha1_valid=0 for the submodule on the worktree side of the diff. The
reason is that we start out with sha1_valid=0 and sha1=000..000 for the
worktree side of all dirty entries, which makes sense at that point. We
later set the sha1 by looking inside the submodule in
diff_fill_sha1_info(), but we never set sha1_valid. So the above
conditional will now trigger on the !one->sha1_valid arm, completely
defeating the change to reuse_worktree_file().
We can fix it like below, but it feels a bit wrong to me. Are
submodules the only case where it makes sense to set sha1_valid when we
fill the sha1?
diff.c | 7 +--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git i/diff.c w/diff.c
index dabf913..cf7281d 100644
--- i/diff.c
+++ w/diff.c
@@ -3081,6 +3082,8 @@ static void diff_fill_sha1_info(struct diff_filespec *one)
die_errno("stat '%s'", one->path);
if (index_path(one->sha1, one->path, &st, 0))
die("cannot hash %s", one->path);
+ if (S_ISGITLINK(one->mode))
+ one->sha1_valid = 1;
}
}
else
--
Thomas Rast
t...@thomasrast.ch
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] mv: prevent mismatched data when ignoring errors.
"brian m. carlson" writes: > We shrink the source and destination arrays, but not the modes or > submodule_gitfile arrays, resulting in potentially mismatched data. Shrink > all the arrays at the same time to prevent this. > > Signed-off-by: brian m. carlson > --- > builtin/mv.c | 5 + > 1 file changed, 5 insertions(+) > > diff --git a/builtin/mv.c b/builtin/mv.c > index f99c91e..b20cd95 100644 > --- a/builtin/mv.c > +++ b/builtin/mv.c > @@ -230,6 +230,11 @@ int cmd_mv(int argc, const char **argv, const char > *prefix) > memmove(destination + i, > destination + i + 1, > (argc - i) * sizeof(char *)); > + memmove(modes + i, modes + i + 1, > + (argc - i) * sizeof(char *)); This isn't right -- you are computing the size of things to be moved based on a type of char*, but 'modes' is an enum. (Valgrind spotted this.) -- Thomas Rast t...@thomasrast.ch -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Rewrite diff-no-index.c:read_directory() to use is_dot_or_dotdot() and rename it to read_dir()
Akshay Aurora writes: > Forgot to mention, this is one of the microprojects for GSoC this > year. Would be great to have some feedback. > > On Fri, Mar 14, 2014 at 6:09 PM, Akshay Aurora wrote: >> I have renamed diff-no-index.c:read_directory() to read_dir() to avoid name >> collision with dir.c:read_directory() >> >> Signed-off-by: Akshay Aurora Hmm, the original mail never made it through to me, and gmane doesn't seem to have it either. What happened here? The headers suggest you used git-send-email, which should avoid these problems. Can you dig up the command and configuration you used to send it (but be careful to not post your password!)? On the patch itself: > Subject: Re: [PATCH] Rewrite diff-no-index.c:read_directory() to use > is_dot_or_dotdot() and rename it to read_dir() The subject line is very long. Aim for 50 characters, but certainly no more than 72. You are also conflating two separate things into one patch. Try to avoid doing that. Furthermore I am unconvinced that renaming a function from read_directory() to read_dir() is a win. What are you trying to improve by the rename? Renames are good if they improve clarity and/or consistency, but read_dir() just risks confusion with readdir() and I cannot see any gain in consistency that would compensate for it. >> I have renamed diff-no-index.c:read_directory() to read_dir() to avoid name >> collision with dir.c:read_directory() Please stick to the style outlined in SubmittingPatches: The body should provide a meaningful commit message, which: . explains the problem the change tries to solve, iow, what is wrong with the current code without the change. . justifies the way the change solves the problem, iow, why the result with the change is better. . alternate solutions considered but discarded, if any. Describe your changes in imperative mood, e.g. "make xyzzy do frotz" instead of "[This patch] makes xyzzy do frotz" or "[I] changed xyzzy to do frotz", as if you are giving orders to the codebase to change its behaviour. Try to make sure your explanation can be understood without external resources. Instead of giving a URL to a mailing list archive, summarize the relevant points of the discussion. Also, please wrap your commit messages at 72 characters. >> --- >> diff-no-index.c | 9 + The microproject idea said Rewrite diff-no-index.c:read_directory() to use is_dot_or_dotdot(). Try to find other sites that can use that function. Are there any others? -- Thomas Rast t...@thomasrast.ch -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] add: Use struct argv_array in run_add_interactive()
Fabian Ruch writes:
> run_add_interactive() in builtin/add.c manually computes array bounds
> and allocates a static args array to build the add--interactive command
> line, which is error-prone. Use the argv-array helper functions instead.
>
> Signed-off-by: Fabian Ruch
Thanks, this is a nicely done cleanup.
> ---
> builtin/add.c | 21 ++---
> 1 file changed, 10 insertions(+), 11 deletions(-)
>
> diff --git a/builtin/add.c b/builtin/add.c
> index 4b045ba..459208a 100644
> --- a/builtin/add.c
> +++ b/builtin/add.c
> @@ -15,6 +15,7 @@
> #include "diffcore.h"
> #include "revision.h"
> #include "bulk-checkin.h"
> +#include "argv-array.h"
>
> static const char * const builtin_add_usage[] = {
> N_("git add [options] [--] ..."),
> @@ -141,23 +142,21 @@ static void refresh(int verbose, const struct pathspec
> *pathspec)
> int run_add_interactive(const char *revision, const char *patch_mode,
> const struct pathspec *pathspec)
> {
> + int status, i;
> + struct argv_array argv = ARGV_ARRAY_INIT;
>
> - args = xcalloc(sizeof(const char *), (pathspec->nr + 6));
> - ac = 0;
> - args[ac++] = "add--interactive";
> + argv_array_push(&argv, "add--interactive");
> if (patch_mode)
> - args[ac++] = patch_mode;
> + argv_array_push(&argv, patch_mode);
> if (revision)
> - args[ac++] = revision;
> - args[ac++] = "--";
> + argv_array_push(&argv, revision);
> + argv_array_push(&argv, "--");
> for (i = 0; i < pathspec->nr; i++)
> /* pass original pathspec, to be re-parsed */
> - args[ac++] = pathspec->items[i].original;
> + argv_array_push(&argv, pathspec->items[i].original);
>
> - status = run_command_v_opt(args, RUN_GIT_CMD);
> - free(args);
> + status = run_command_v_opt(argv.argv, RUN_GIT_CMD);
> + argv_array_clear(&argv);
> return status;
> }
--
Thomas Rast
t...@thomasrast.ch
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Make XDF_NEED_MINIMAL default in blame.
Michael Andreen writes: > The --minimal flag is still there, but didn't want to break scripts > depending on it. If I specify --no-minimal, does that turn it off again? -- Thomas Rast t...@thomasrast.ch -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 7/8] name-hash: allow dir hashing even when !ignore_case
Eric Sunshine writes:
> On Sat, Feb 22, 2014 at 4:17 AM, Thomas Rast wrote:
>> -static void lazy_init_name_hash(struct index_state *istate)
>> +void init_name_hash(struct index_state *istate, int force_dir_hash)
>> {
>> int nr;
>>
>> if (istate->name_hash_initialized)
>> return;
>> +
>> + istate->has_dir_hash = force_dir_hash || ignore_case;
>
> This is getting a bit convoluted. Refactoring lazy_init_name_hash()
> into two functions would eliminate the complexity. For instance:
>
> void init_name_hash(struct index_state *istate)
> {
> ...pure initialization code...
> }
>
> static void init_name_hash_if_needed(struct index_state *istate)
> {
> if (ignore_case && !istate->name_hash_initialized)
> init_name_hash(istate);
> }
>
> The two existing callers of lazy_init_name_hash() in name-hash.c,
> which rely upon the lazy/ignore-case logic, would invoke the static
> init_name_hash_if_needed().
>
> The new caller in patch 8/8, which knows explicitly if and when it
> wants the hash initialized can invoke the public init_name_hash().
That unfortunately doesn't really help because the conditional part only
affects the dir hash. Callers request the name hash for other reasons,
but we only do the dir hashing when ignore_case is in effect.
So it's not simply about overriding lazy initialization, but about
choosing to trigger a specific subpart of the initialization.
--
Thomas Rast
t...@thomasrast.ch
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v3 0/8] --remerge-diff
This is a resend of the remerge-diff patch series, previously posted
here:
http://thread.gmane.org/gmane.comp.version-control.git/242514
Differences to the previous version:
- Rebased onto the new {name,dir}_hash maps (7/8 looks very different
now). This also allows freeing index entries that we no longer need
(in 8/8); previously, the insert-only name-hash kept them alive.
- Adaptations to match Duy's changes to cache_tree handling (in 8/8).
Please review the cache_tree handling extra carefully, as I'm not
100% convinced the dance there is all that is needed.
Thomas Rast (8):
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
combine-diff: do not pass revs->dense_combined_merges redundantly
Fold all merge diff variants into an enum
merge-recursive: allow storing conflict hunks in index
name-hash: allow dir hashing even when !ignore_case
log --remerge-diff: show what the conflict resolution changed
Documentation/merge-strategies.txt | 9 ++
Documentation/rev-list-options.txt | 7 +
builtin/diff-files.c | 5 +-
builtin/diff-tree.c| 2 +-
builtin/diff.c | 12 +-
builtin/fmt-merge-msg.c| 2 +-
builtin/log.c | 9 +-
builtin/merge.c| 1 -
cache.h| 2 +
combine-diff.c | 13 +-
diff-lib.c | 13 +-
diff.h | 6 +-
log-tree.c | 303 -
merge-recursive.c | 52 ---
merge-recursive.h | 3 +
name-hash.c| 13 +-
revision.c | 15 +-
revision.h | 24 ++-
submodule.c| 3 +-
t/t3030-merge-recursive.sh | 33
t/t4213-log-remerge-diff.sh| 222 +++
21 files changed, 673 insertions(+), 76 deletions(-)
create mode 100755 t/t4213-log-remerge-diff.sh
--
2.1.0.72.g9b94086
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v3 1/8] merge-recursive: remove dead conditional in update_stages()
From: Thomas Rast 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 Signed-off-by: Junio C Hamano --- merge-recursive.c | 6 ++ 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/merge-recursive.c b/merge-recursive.c index 1d332b8..4459607 100644 --- a/merge-recursive.c +++ b/merge-recursive.c @@ -547,11 +547,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; -- 2.1.0.72.g9b94086 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v3 4/8] 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
---
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 91edce5..221ab22 100644
--- a/combine-diff.c
+++ b/combine-diff.c
@@ -966,7 +966,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;
@@ -981,6 +981,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);
@@ -1228,7 +1229,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;
@@ -1238,7 +1238,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)
@@ -1388,7 +1388,6 @@ static struct combine_diff_path *find_paths_multitree(
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;
@@ -1516,7 +1515,7 @@ void diff_tree_combined(const unsigned char *sha1,
printf("%s%c", diff_line_prefix(opt),
opt->line_termination);
for (p = paths; p; p = p->next)
- show_patch_diff(p, num_parent, dense,
+ show_patch_diff(p, num_parent,
0, rev);
}
}
@@ -1531,7 +1530,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);
@@ -1541,6 +1540,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 875aff8..3e533d2 100644
--- a/diff-lib.c
+++ b/diff-lib.c
@@ -171,9 +171,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,
-
[PATCH v3 2/8] merge-recursive: internal flag to avoid touching the worktree
From: Thomas Rast
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
Signed-off-by: Junio C Hamano
---
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 4459607..059ad03 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -410,10 +410,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. */
@@ -743,7 +743,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) {
@@ -950,7 +950,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);
@@ -1018,7 +1019,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);
}
@@ -1143,10 +1144,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);
@@ -1253,7 +1254,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 {
@@ -1261,7 +1262,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);
@@ -1420,6 +1421,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,
[PATCH v3 5/8] 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
---
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 1c4ad62..1a4bcf1 100644
--- a/builtin/diff-tree.c
+++ b/builtin/diff-tree.c
@@ -90,7 +90,7 @@ COMMON_DIFF_OPTIONS_HELP;
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 79df05e..db23626 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 4389722..ba057d5 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -496,13 +496,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" o
[PATCH v3 7/8] name-hash: allow dir hashing even when !ignore_case
The directory hash (for fast checks if the index already has a
directory) was only used in ignore_case mode and so depended on that
flag.
Make it generally available on request.
Signed-off-by: Thomas Rast
---
cache.h | 2 ++
name-hash.c | 13 -
2 files changed, 10 insertions(+), 5 deletions(-)
diff --git a/cache.h b/cache.h
index 4d5b76c..c54b2e1 100644
--- a/cache.h
+++ b/cache.h
@@ -306,6 +306,7 @@ struct index_state {
struct split_index *split_index;
struct cache_time timestamp;
unsigned name_hash_initialized : 1,
+has_dir_hash : 1,
initialized : 1;
struct hashmap name_hash;
struct hashmap dir_hash;
@@ -315,6 +316,7 @@ struct index_state {
extern struct index_state the_index;
/* Name hashing */
+extern void init_name_hash(struct index_state *istate, int force_dir_hash);
extern void add_name_hash(struct index_state *istate, struct cache_entry *ce);
extern void remove_name_hash(struct index_state *istate, struct cache_entry
*ce);
extern void free_name_hash(struct index_state *istate);
diff --git a/name-hash.c b/name-hash.c
index 702cd05..22e3ec6 100644
--- a/name-hash.c
+++ b/name-hash.c
@@ -106,7 +106,7 @@ static void hash_index_entry(struct index_state *istate,
struct cache_entry *ce)
hashmap_entry_init(ce, memihash(ce->name, ce_namelen(ce)));
hashmap_add(&istate->name_hash, ce);
- if (ignore_case)
+ if (istate->has_dir_hash)
add_dir_entry(istate, ce);
}
@@ -121,7 +121,7 @@ static int cache_entry_cmp(const struct cache_entry *ce1,
return remove ? !(ce1 == ce2) : 0;
}
-static void lazy_init_name_hash(struct index_state *istate)
+void init_name_hash(struct index_state *istate, int force_dir_hash)
{
int nr;
@@ -130,6 +130,9 @@ static void lazy_init_name_hash(struct index_state *istate)
hashmap_init(&istate->name_hash, (hashmap_cmp_fn) cache_entry_cmp,
istate->cache_nr);
hashmap_init(&istate->dir_hash, (hashmap_cmp_fn) dir_entry_cmp, 0);
+
+ istate->has_dir_hash = force_dir_hash || ignore_case;
+
for (nr = 0; nr < istate->cache_nr; nr++)
hash_index_entry(istate, istate->cache[nr]);
istate->name_hash_initialized = 1;
@@ -148,7 +151,7 @@ void remove_name_hash(struct index_state *istate, struct
cache_entry *ce)
ce->ce_flags &= ~CE_HASHED;
hashmap_remove(&istate->name_hash, ce, ce);
- if (ignore_case)
+ if (istate->has_dir_hash)
remove_dir_entry(istate, ce);
}
@@ -193,7 +196,7 @@ struct cache_entry *index_dir_exists(struct index_state
*istate, const char *nam
struct cache_entry *ce;
struct dir_entry *dir;
- lazy_init_name_hash(istate);
+ init_name_hash(istate, 0);
dir = find_dir_entry(istate, name, namelen);
if (dir && dir->nr)
return dir->ce;
@@ -214,7 +217,7 @@ struct cache_entry *index_file_exists(struct index_state
*istate, const char *na
{
struct cache_entry *ce;
- lazy_init_name_hash(istate);
+ init_name_hash(istate, 0);
ce = hashmap_get_from_hash(&istate->name_hash,
memihash(name, namelen), NULL);
--
2.1.0.72.g9b94086
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v3 6/8] 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
---
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 7716fda..19f3a5d 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 d54bac2..20d3051 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -743,6 +743,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;
@@ -2110,6 +2112,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 be07705..39841a9 100755
--- a/t/t3030-merge-recursive.sh
+++ b/t/t3030-merge-recursive.sh
@@ -310,6 +310,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] &&
--
2.1.0.72.g9b94086
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v3 3/8] merge-recursive: -Xindex-only to leave worktree unchanged
From: Thomas Rast 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 --- 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 7bbd19b..7716fda 100644 --- a/Documentation/merge-strategies.txt +++ b/Documentation/merge-strategies.txt @@ -92,6 +92,10 @@ subtree[=];; 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 059ad03..d54bac2 100644 --- a/merge-recursive.c +++ b/merge-recursive.c @@ -2108,6 +2108,8 @@ int parse_merge_opt(struct merge_options *o, const char *s) if ((o->rename_score = parse_rename_score(&arg)) == -1 || *arg != 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 82e1854..be07705 100755 --- a/t/t3030-merge-recursive.sh +++ b/t/t3030-merge-recursive.sh @@ -297,6 +297,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] && -- 2.1.0.72.g9b94086 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v3 8/8] 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
---
Documentation/rev-list-options.txt | 7 +
log-tree.c | 297 +
merge-recursive.c | 3 +-
merge-recursive.h | 1 +
revision.c | 2 +
revision.h | 4 +-
t/t4213-log-remerge-diff.sh| 222 +++
7 files changed, 534 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 deb8cca..7128350 100644
--- a/Documentation/rev-list-options.txt
+++ b/Documentation/rev-list-options.txt
@@ -805,6 +805,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 8f57651..4db1385 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" };
@@ -719,6 +721,299 @@ static int do_diff_combined(struct rev_info *opt, struct
commit *commit)
}
/*
+ * Helpers for make_asymmetric_conflict_entries() below.
+ */
+static char *load_cache_entry_blob(struct cache_entry *entry,
+ unsigned long *size)
+{
+ enum object_type type;
+ void *data;
+
+ if (!entry)
+ return NULL;
+
+ data = read_sha1_file(entry->sha1, &type, size);
+ if (type != OBJ_BLOB)
+ die("BUG: load_cache_entry_blob for non-blob");
+
+ return data;
+}
+
+static void strbuf_append_cache_entry_blob(struct strbuf *sb,
+ struct cache_entry *entry)
+{
+ unsigned long size;
+ char *data = load_cache_entry_blob(entry, &size);;
+
+ if (!data)
+ return;
+
+ strbuf_add(sb, data, size);
+ free(data);
+}
+
+static void assemble_conflict_entry(struct strbuf *sb,
+ const char *branch1,
+ const char *branch2,
+ struct cache_entry *entry1,
+ struct cache_entry *entry2)
+{
+ strbuf_addf(sb, "<<<<<<< %s\n", branch1);
+ strbuf_append_cache_entry_blob(sb, entry1);
+ strbuf_addstr(sb, "===\n");
+ strbuf_append_cache_entry_blob(sb, entry2);
+ strbuf_addf(sb, ">>>>>>> %s\n", branch2);
+}
+
+/*
+ * For --remerge-diff, we need conflicted (<<<<<<< ... >>>>>>>)
+ * representations of as many conflicts as possible. Default conflict
+ * generation only applies to files that have all three stages.
+ *
+ * This function generates conflict hunk representations for files
+ * that have only one of stage 2 or 3. The corresponding side in the
+ * conflict hunk format will be empty. A stage 1, if any, will be
+ * dropped in the process.
+ */
+static void make_asymme
Re: [PATCH] l10n: de.po: translate 45 new messages
Ralf Thielow writes: > Translate 45 new messages came from git.pot update in 5e078fc > (l10n: git.pot: v2.0.0 round 1 (45 new, 28 removed)). Thanks for sending this with extra context, it really helps reviewing! With the small changes below, Acked-by: Thomas Rast > #: diffcore-rename.c:517 > msgid "Performing inexact rename detection" > -msgstr "" > +msgstr "Führe Erkennung für ungenaue Umbenennung aus" I don't have a better idea, but I wish we had a less unwieldy term for "inexact rename detection". > #: wt-status.c:266 > -#, fuzzy, c-format > +#, c-format > msgid "bug: unhandled unmerged status %x" > -msgstr "Fehler: unbehandelter Differenz-Status %c" > +msgstr "Fehler: unbehandelter Unmerged-Status %x" s/Fehler/Bug/? I interpret "Fehler" in such a message as a user-caused problem, whereas this message means the program doesn't handle a case that it really should. > #: builtin/commit.c:1131 > -#, fuzzy > msgid "Explicit paths specified without -i or -o; assuming --only paths..." > msgstr "" > "Explizite Pfade ohne -i oder -o angegeben; unter der Annahme von --only " > "Pfaden..." Not new, but "[I am] assuming --only paths" should really be translated as such, e.g. nehme --only an (Leaving off the "paths"; I think it reads better without it even in English.) > #: builtin/gc.c:318 > -#, fuzzy, c-format > +#, c-format > msgid "Auto packing the repository in background for optimum performance.\n" > msgstr "" > -"Die Datenbank des Projektarchivs wird für eine optimale Performance " > -"komprimiert.\n" > +"Die Datenbank des Repositories wird für eine optimale Performance > automatisch\n" > +"im Hintergrund komprimiert.\n" > > #: builtin/gc.c:320 > #, c-format > msgid "Auto packing the repository for optimum performance.\n" > msgstr "" > "Die Datenbank des Projektarchivs wird für eine optimale Performance " > "komprimiert.\n" Nit: you added "automatisch" in the first one, but the second one doesn't have it even though the difference between them is only "in background". > #: builtin/gc.c:321 > #, c-format > msgid "See \"git help gc\" for manual housekeeping.\n" > -msgstr "" > +msgstr "Siehe \"git help gc\" für manuelle Einstellungen.\n" für manuelles Aufräumen? > #: builtin/notes.c:276 > -#, fuzzy, c-format > +#, c-format > msgid "Cannot read note data from non-blob object '%s'." > -msgstr "Kann existierendes Objekt %s nicht lesen." > +msgstr "Kann Notiz-Daten von Nicht-Blob Objekt '%s' nicht lesen." This would read better as Kann Notiz-Daten nicht von Nicht-Blob Objekt '%s' lesen. since the key point is that we refuse any non-blob object, not that we can't read it. -- Thomas Rast t...@thomasrast.ch -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 10/10] t9904: new __git_ps1 tests for Zsh
Richard Hansen writes: > These are the same tests as in t9903, but run in zsh instead of bash. > > Signed-off-by: Richard Hansen > --- > t/lib-zsh.sh | 30 ++ > t/t9904-zsh-prompt.sh | 10 ++ > 2 files changed, 40 insertions(+) > create mode 100644 t/lib-zsh.sh > create mode 100755 t/t9904-zsh-prompt.sh This doesn't appear to work in valgrind mode: $ ./t9904-zsh-prompt.sh --valgrind error: Test script did not set test_description. t9903 however works. I'm not sure how much of a difference it makes, but: I use bash as my shell and as /bin/sh, but I do have zsh installed. Can you look into it? -- Thomas Rast t...@thomasrast.ch -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH RFC v2 00/19] Enable options --signoff, --reset-author for pick, reword
Fabian Ruch writes: > On 07/08/2014 10:45 PM, Junio C Hamano wrote: >> Fabian Ruch writes: >>> Fabian Ruch (19): >>> rebase -i: Failed reword prints redundant error message >>> rebase -i: reword complains about empty commit despite --keep-empty >>> rebase -i: reword executes pre-commit hook on interim commit >>> rebase -i: Teach do_pick the option --edit >>> rebase -i: Implement reword in terms of do_pick >>> rebase -i: Stop on root commits with empty log messages >>> rebase -i: The replay of root commits is not shown with --verbose >>> rebase -i: Root commits are replayed with an unnecessary option >>> rebase -i: Commit only once when rewriting picks >>> rebase -i: Do not die in do_pick >>> rebase -i: Teach do_pick the option --amend >>> rebase -i: Teach do_pick the option --file >>> rebase -i: Prepare for squash in terms of do_pick --amend >>> rebase -i: Implement squash in terms of do_pick >>> rebase -i: Explicitly distinguish replay commands and exec tasks >>> rebase -i: Parse to-do list command line options >>> rebase -i: Teach do_pick the option --reset-author >>> rebase -i: Teach do_pick the option --signoff >>> rebase -i: Enable options --signoff, --reset-author for pick, reword >> >> After "rebase -i:", some begin with lowercase and many begin with >> capital, which makes the short-log output look distracting. > > The ones that begin with lower-case letters are the ones that begin with > the command name "reword". All first lines are typed in lower case now. You could spell it 'reword' (with the quotes), which also disambiguates the command from the verb. -- Thomas Rast t...@thomasrast.ch -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v1] rebase --root: sentinel commit cloaks empty commits
Hi Fabian
Impressive analysis!
> Concerning the bugfix: Obviously, the patch misuses the `squash_onto`
> flag because it assumes that the new base is empty except for the
> sentinel commit. The variable name does not imply anything close to
> that. An additional flag to disable the use of the git-rev-list
> option `--cherry-pick` would work and make sense again (for instance,
> `keep_redundant`).
Seeing as there are only two existing uses of the variable, you could
also rename it to make it more obvious what is going on. I think either
way is fine.
[...]
> Please take a closer look at the last two test cases that specify the
> expected behaviour of rebasing a branch that tracks the empty tree.
> At this point they expect the "Nothing to do" error (aborts with
> untouched history). This is consistent with rebasing only empty
> commits without `--root`, which also doesn't just delete them from
> the history. Furthermore, I think the two alternatives adding a note
> that all commits in the range were empty, and removing the empty
> commits (thus making the branch empty) are better discussed in a
> separate bug report.
Makes sense to me, though I have never thought much about rebasing empty
commits. Maybe Chris has a more informed opinion?
> is_empty_commit() {
> - tree=$(git rev-parse -q --verify "$1"^{tree} 2>/dev/null ||
> - die "$1: not a commit that can be picked")
> - ptree=$(git rev-parse -q --verify "$1"^^{tree} 2>/dev/null ||
> - ptree=4b825dc642cb6eb9a060e54bf8d69288fbee4904)
> + tree=$(git rev-parse -q --verify "$1"^{tree} 2>/dev/null) ||
> + die "$1: not a commit that can be picked"
> + ptree=$(git rev-parse -q --verify "$1"^^{tree} 2>/dev/null) ||
> + ptree=4b825dc642cb6eb9a060e54bf8d69288fbee4904
> test "$tree" = "$ptree"
> }
Nice catch!
> @@ -958,7 +958,17 @@ then
> revisions=$upstream...$orig_head
> shortrevisions=$shortupstream..$shorthead
> else
> - revisions=$onto...$orig_head
> + if test -n "$squash_onto"
> + then
> + # $onto points to an empty commit (the sentinel
> + # commit) which was not created by the user.
> + # Exclude it from the rev list to avoid skipping
> + # empty user commits prematurely, i. e. before
> + # --keep-empty can take effect.
> + revisions=$orig_head
> + else
> + revisions=$onto...$orig_head
> + fi
> shortrevisions=$shorthead
Nit: I think this would be clearer if you phrased it using an 'elif',
instead of nesting (but keep the comment!).
--
Thomas Rast
t...@thomasrast.ch
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v7 25/31] prune: strategies for linked checkouts
Nguyễn Thái Ngọc Duy writes:
> (alias R=$GIT_COMMON_DIR/repos/)
>
> - linked checkouts are supposed to keep its location in $R/gitdir up
>to date. The use case is auto fixup after a manual checkout move.
>
> - linked checkouts are supposed to update mtime of $R/gitdir. If
>$R/gitdir's mtime is older than a limit, and it points to nowhere,
>repos/ is to be pruned.
>
> - If $R/locked exists, repos/ is not supposed to be pruned. If
>$R/locked exists and $R/gitdir's mtime is older than a really long
>limit, warn about old unused repo.
>
> - "git checkout --to" is supposed to make a hard link named $R/link
>pointing to the .git file on supported file systems to help detect
>the user manually deleting the checkout. If $R/link exists and its
>link count is greated than 1, the repo is kept.
>
> Signed-off-by: Nguyễn Thái Ngọc Duy
> ---
> Documentation/git-prune.txt| 3 +
> Documentation/gitrepository-layout.txt | 19 ++
> builtin/checkout.c | 14 +
> builtin/prune.c| 99
> ++
> setup.c| 13
> t/t2026-prune-linked-checkouts.sh (new +x) | 84 +
I get this from t2026.2 under valgrind:
==21334== Conditional jump or move depends on uninitialised value(s)
==21334==at 0x46D49B: prune_repos_dir (prune.c:182)
==21334==by 0x46D8C0: cmd_prune (prune.c:252)
==21334==by 0x405C2F: run_builtin (git.c:351)
==21334==by 0x405E47: handle_builtin (git.c:530)
==21334==by 0x405F6B: run_argv (git.c:576)
==21334==by 0x40610B: main (git.c:663)
==21334== Uninitialised value was created by a stack allocation
==21334==at 0x46D3BB: prune_repos_dir (prune.c:169)
==21334==
{
Memcheck:Cond
fun:prune_repos_dir
fun:cmd_prune
fun:run_builtin
fun:handle_builtin
fun:run_argv
fun:main
}
not ok 2 - prune files inside $GIT_DIR/repos
#
# mkdir .git/repos &&
# : >.git/repos/abc &&
# git prune --repos --verbose >actual &&
# cat >expect < +static int prune_repo_dir(const char *id, struct stat *st, struct strbuf
> *reason)
> +{
> + char *path;
> + int fd, len;
> +
> + if (!is_directory(git_path("repos/%s", id))) {
> + strbuf_addf(reason, _("Removing repos/%s: not a valid
> directory"), id);
> + return 1;
> + }
> + if (file_exists(git_path("repos/%s/locked", id)))
> + return 0;
in this line, before the stat() actually runs, which then in the
condition ...
> + if (stat(git_path("repos/%s/gitdir", id), st)) {
> + st->st_mtime = expire;
> + strbuf_addf(reason, _("Removing repos/%s: gitdir file does not
> exist"), id);
> + return 1;
> + }
[...]
> +}
> +
> +static void prune_repos_dir(void)
> +{
[...]
> + struct stat st;
[...]
> + if (!prune_repo_dir(d->d_name, &st, &reason) ||
> + st.st_mtime > expire)
causes the second arm to be evaluated when st.st_mtime is not
initialized yet. Can you look into this?
--
Thomas Rast
t...@thomasrast.ch
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 04/23] rebase -i: hide interactive command messages in verbose mode
Fabian Ruch writes: > @@ -923,6 +923,8 @@ EOF > ;; > esac > > +mkdir -p "$state_dir" || die "Could not create temporary $state_dir" > + > git var GIT_COMMITTER_IDENT >/dev/null || > die "You need to set your committer info first" > > @@ -938,7 +940,6 @@ then > fi > > orig_head=$(git rev-parse --verify HEAD) || die "No HEAD?" > -mkdir -p "$state_dir" || die "Could not create temporary $state_dir" > > : > "$state_dir"/interactive || die "Could not mark as interactive" > write_basic_state Why this change? I can't figure out how it relates to the output change. > @@ -873,9 +873,8 @@ test_expect_success 'running "git rebase -i --exec git > show HEAD"' ' > ( > FAKE_LINES="1 exec_git_show_HEAD 2 exec_git_show_HEAD" && > export FAKE_LINES && > - git rebase -i HEAD~2 >expect > + git rebase -i HEAD~2 >expected > ) && > - sed -e "1,9d" expect >expected && > test_cmp expected actual > ' Getting rid of these magic removals is a very nice change, thank you. -- Thomas Rast t...@thomasrast.ch -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 08/23] rebase -i: reword executes pre-commit hook on interim commit
Fabian Ruch writes: > Subject: Re: [PATCH v2 08/23] rebase -i: reword executes pre-commit hook on > interim commit I think the change makes sense, but can you reword the subjects that it describes the state after the commit (i.e. what you are doing), instead of before the commit? -- Thomas Rast t...@thomasrast.ch -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 23/23] rebase -i: enable options --signoff, --reset-author for pick, reword
Fabian Ruch writes:
> @@ -634,21 +644,24 @@ do_replay () {
> comment_for_reflog pick
>
> mark_action_done
> - do_pick $sha1 || die_with_patch $sha1 "Could not apply $sha1...
> $rest"
> + eval do_pick $opts $sha1 \
> + || die_with_patch $sha1 "Could not apply $sha1... $rest"
You had me a little puzzled at the switch to 'eval' here. That is
necessary to match the quoting added in 20/23, not for any change in
this commit. This commit is simply the first one to trigger this.
Also, are you sure $sha1 does not require quoting through an eval?
Please add tests to this patch.
--
Thomas Rast
t...@thomasrast.ch
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 20/23] rebase -i: parse to-do list command line options
Fabian Ruch writes:
[...]
> are not supported at the moment. Neither are options that contain
> spaces because the shell expansion of `args` in `do_next` interprets
> white space characters as argument separator, that is a command line
> like
>
> pick --author "A U Thor" fa1afe1 Some change
>
> is parsed as the pick command
>
> pick --author
>
> and the commit hash
>
> "A
>
> which obviously results in an unknown revision error. For the sake of
> completeness, in the example above the message title variable `rest`
> is assigned the string 'U Thor" fa1afe1 Some change' (without the
> single quotes).
You could probably trim down the non-example a bit and instead give an
example :-)
> Print an error message for unknown or unsupported command line
> options, which means an error for all specified options at the
> moment.
Can you add a test that verifies we catch an obvious unknown option
(such as --unknown-option)?
> Cleanly break the `do_next` loop by assigning the special
> value 'unknown' to the local variable `command`, which triggers the
> unknown command case in `do_cmd`.
[...]
> do_replay () {
> command=$1
> - sha1=$2
> - rest=$3
> + shift
> +
> + opts=
> + while test $# -gt 0
> + do
> + case "$1" in
> + -*)
> + warn "Unknown option: $1"
> + command=unknown
> + ;;
> + *)
> + break
> + ;;
This seems a rather hacky solution to me. Doesn't this now print
warning: Unknown option: --unknown-option
warning: Unknown command: pick --unknown-option
?
It shouldn't claim the command is unknown if the command itself was
valid.
Also, you speak of do_cmd above, but the unknown command handling seems
to be part of do_replay?
--
Thomas Rast
t...@thomasrast.ch
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 04/23] rebase -i: hide interactive command messages in verbose mode
Fabian Ruch writes: > Hi Thomas, > > Thomas Rast writes: >> Fabian Ruch writes: >>> @@ -923,6 +923,8 @@ EOF >>> ;; >>> esac >>> >>> +mkdir -p "$state_dir" || die "Could not create temporary $state_dir" >>> + >>> git var GIT_COMMITTER_IDENT >/dev/null || >>> die "You need to set your committer info first" >>> >>> @@ -938,7 +940,6 @@ then >>> fi >>> >>> orig_head=$(git rev-parse --verify HEAD) || die "No HEAD?" >>> -mkdir -p "$state_dir" || die "Could not create temporary $state_dir" >>> >>> : > "$state_dir"/interactive || die "Could not mark as interactive" >>> write_basic_state >> >> Why this change? I can't figure out how it relates to the output >> change. > > Creating the state directory a few steps earlier into > 'git_rebase__interactive' is necessary because the changed definition of > 'output' needs it for 'editor.sh'. This change was triggered by a > failing test case that used the argument with git-rebase. The > 'git checkout ', which is executed if 'switch_to' is set to > , is wrapped into an 'output' line and 'output' failed because > it wasn't able to create 'editor.sh'. [...] >> In order to temporarily redirect the editor output, the new >> definition of `output` creates a script in the state directory to be >> used as `GIT_EDITOR`. Make sure the state directory exists before >> `output` is called for the first time. Ah, makes sense. Thanks for the explanations! -- Thomas Rast t...@thomasrast.ch -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 08/23] rebase -i: reword executes pre-commit hook on interim commit
Fabian Ruch writes: > Hi Thomas, > > Thomas Rast writes: >> Fabian Ruch writes: >>> Subject: Re: [PATCH v2 08/23] rebase -i: reword executes pre-commit >>> hook on interim commit >> >> I think the change makes sense, but can you reword the subjects that it >> describes the state after the commit (i.e. what you are doing), instead >> of before the commit? > > The log message subject now reads as follows: > >> rebase -i: do not verify reworded patches using pre-commit Excellent wording, thanks! -- Thomas Rast t...@thomasrast.ch -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3] l10n: de.po: translate 68 new messages
Ralf Thielow writes: > Translate 68 new messages came from git.pot update in 727b957 > (l10n: git.pot: v1.8.5 round 1 (68 new, 9 removed)). > > Signed-off-by: Ralf Thielow Acked-by: Thomas Rast Thanks for your work! -- Thomas Rast t...@thomasrast.ch -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] config: arbitrary number of matches for --unset and --replace-all
git-config used a static match array to hold the matches we want to
unset/replace when using --unset or --replace-all. Use a
variable-sized array instead.
This in particular fixes the symptoms git-svn had when storing large
numbers of svn-remote.*.added-placeholder entries in the config file.
While the tests are rather more paranoid than just --unset and
--replace-all, the other operations already worked. Indeed git-svn's
usage only breaks the first time *after* creating so many entries,
when it wants to unset and re-add them all.
Reported-by: Jess Hottenstein
Signed-off-by: Thomas Rast
---
This should fix it, though I haven't actually tested with such a funny
use-case, nor written a proper git-svn test for it. My analysis about
the failure mode is from briefly looking at the code.
git-svn should probably be changed to use another way of storing
these, as git-config is not a very efficient database, but I'm leaving
that for someone who cares about SVN. (Note that it's also much
harder as it'll need a migration plan.)
config.c| 19 ++-
t/t1303-wacky-config.sh | 64 +
2 files changed, 77 insertions(+), 6 deletions(-)
diff --git a/config.c b/config.c
index 3c2434a..ef63bf2 100644
--- a/config.c
+++ b/config.c
@@ -1197,15 +1197,14 @@ int git_config(config_fn_t fn, void *data)
* Find all the stuff for git_config_set() below.
*/
-#define MAX_MATCHES 512
-
static struct {
int baselen;
char *key;
int do_not_match;
regex_t *value_regex;
int multi_replace;
- size_t offset[MAX_MATCHES];
+ size_t *offset;
+ unsigned int offset_alloc;
enum { START, SECTION_SEEN, SECTION_END_SEEN, KEY_SEEN } state;
int seen;
} store;
@@ -1228,11 +1227,11 @@ static int store_aux(const char *key, const char
*value, void *cb)
if (matches(key, value)) {
if (store.seen == 1 && store.multi_replace == 0) {
warning("%s has multiple values", key);
- } else if (store.seen >= MAX_MATCHES) {
- error("too many matches for %s", key);
- return 1;
}
+ ALLOC_GROW(store.offset, store.seen+1,
+ store.offset_alloc);
+
store.offset[store.seen] = cf->do_ftell(cf);
store.seen++;
}
@@ -1260,11 +1259,15 @@ static int store_aux(const char *key, const char
*value, void *cb)
* Do not increment matches: this is no match, but we
* just made sure we are in the desired section.
*/
+ ALLOC_GROW(store.offset, store.seen+1,
+ store.offset_alloc);
store.offset[store.seen] = cf->do_ftell(cf);
/* fallthru */
case SECTION_END_SEEN:
case START:
if (matches(key, value)) {
+ ALLOC_GROW(store.offset, store.seen+1,
+ store.offset_alloc);
store.offset[store.seen] = cf->do_ftell(cf);
store.state = KEY_SEEN;
store.seen++;
@@ -1272,6 +1275,9 @@ static int store_aux(const char *key, const char *value,
void *cb)
if (strrchr(key, '.') - key == store.baselen &&
!strncmp(key, store.key, store.baselen)) {
store.state = SECTION_SEEN;
+ ALLOC_GROW(store.offset,
+ store.seen+1,
+ store.offset_alloc);
store.offset[store.seen] =
cf->do_ftell(cf);
}
}
@@ -1570,6 +1576,7 @@ int git_config_set_multivar_in_file(const char
*config_filename,
}
}
+ ALLOC_GROW(store.offset, 1, store.offset_alloc);
store.offset[0] = 0;
store.state = START;
store.seen = 0;
diff --git a/t/t1303-wacky-config.sh b/t/t1303-wacky-config.sh
index 46103a1..7d55730 100755
--- a/t/t1303-wacky-config.sh
+++ b/t/t1303-wacky-config.sh
@@ -3,17 +3,28 @@
test_description='Test wacky input to git config'
. ./test-lib.sh
+# Leaving off the newline is intentional!
setup() {
(printf "[section]\n" &&
printf " key = foo") >.git/config
}
+# 'check section.key value' verifies that the entry for section.key is
+# 'value'
check() {
echo "$2" >expected
git config --get "$1" >a
[PATCH] config: arbitrary number of matches for --unset and --replace-all
git-config used a static match array to hold the matches we want to
unset/replace when using --unset or --replace-all. Use a
variable-sized array instead.
This in particular fixes the symptoms git-svn had when storing large
numbers of svn-remote.*.added-placeholder entries in the config file.
While the tests are rather more paranoid than just --unset and
--replace-all, the other operations already worked. Indeed git-svn's
usage only breaks the first time *after* creating so many entries,
when it wants to unset and re-add them all.
Reported-by: Jess Hottenstein
Signed-off-by: Thomas Rast
---
Eric Sunshine wrote:
> On Wed, Nov 13, 2013 at 5:19 AM, Thomas Rast wrote:
> > +setup_many() {
[...]
> > + cat >5to1 <
> Broken &&-chain.
Oops, thanks for catching.
config.c| 19 ++-
t/t1303-wacky-config.sh | 64 +
2 files changed, 77 insertions(+), 6 deletions(-)
diff --git a/config.c b/config.c
index 3c2434a..ef63bf2 100644
--- a/config.c
+++ b/config.c
@@ -1197,15 +1197,14 @@ int git_config(config_fn_t fn, void *data)
* Find all the stuff for git_config_set() below.
*/
-#define MAX_MATCHES 512
-
static struct {
int baselen;
char *key;
int do_not_match;
regex_t *value_regex;
int multi_replace;
- size_t offset[MAX_MATCHES];
+ size_t *offset;
+ unsigned int offset_alloc;
enum { START, SECTION_SEEN, SECTION_END_SEEN, KEY_SEEN } state;
int seen;
} store;
@@ -1228,11 +1227,11 @@ static int store_aux(const char *key, const char
*value, void *cb)
if (matches(key, value)) {
if (store.seen == 1 && store.multi_replace == 0) {
warning("%s has multiple values", key);
- } else if (store.seen >= MAX_MATCHES) {
- error("too many matches for %s", key);
- return 1;
}
+ ALLOC_GROW(store.offset, store.seen+1,
+ store.offset_alloc);
+
store.offset[store.seen] = cf->do_ftell(cf);
store.seen++;
}
@@ -1260,11 +1259,15 @@ static int store_aux(const char *key, const char
*value, void *cb)
* Do not increment matches: this is no match, but we
* just made sure we are in the desired section.
*/
+ ALLOC_GROW(store.offset, store.seen+1,
+ store.offset_alloc);
store.offset[store.seen] = cf->do_ftell(cf);
/* fallthru */
case SECTION_END_SEEN:
case START:
if (matches(key, value)) {
+ ALLOC_GROW(store.offset, store.seen+1,
+ store.offset_alloc);
store.offset[store.seen] = cf->do_ftell(cf);
store.state = KEY_SEEN;
store.seen++;
@@ -1272,6 +1275,9 @@ static int store_aux(const char *key, const char *value,
void *cb)
if (strrchr(key, '.') - key == store.baselen &&
!strncmp(key, store.key, store.baselen)) {
store.state = SECTION_SEEN;
+ ALLOC_GROW(store.offset,
+ store.seen+1,
+ store.offset_alloc);
store.offset[store.seen] =
cf->do_ftell(cf);
}
}
@@ -1570,6 +1576,7 @@ int git_config_set_multivar_in_file(const char
*config_filename,
}
}
+ ALLOC_GROW(store.offset, 1, store.offset_alloc);
store.offset[0] = 0;
store.state = START;
store.seen = 0;
diff --git a/t/t1303-wacky-config.sh b/t/t1303-wacky-config.sh
index 46103a1..c2706ea 100755
--- a/t/t1303-wacky-config.sh
+++ b/t/t1303-wacky-config.sh
@@ -3,17 +3,28 @@
test_description='Test wacky input to git config'
. ./test-lib.sh
+# Leaving off the newline is intentional!
setup() {
(printf "[section]\n" &&
printf " key = foo") >.git/config
}
+# 'check section.key value' verifies that the entry for section.key is
+# 'value'
check() {
echo "$2" >expected
git config --get "$1" >actual 2>&1
test_cmp actual expected
}
+# 'check section.key regex value' verifies that the entry for
+# section.key *that matches 'regex'* is 'value'
+check_regex() {
+ echo "$3" >expected
+
Re: can we prevent reflog deletion when branch is deleted?
Sitaram Chamarty writes: > Whatever it was that happened to a hundred or more repos on the Jenkins > project seems to be stirring up this debate in some circles. Making us so curious ... and then you just leave us hanging there ;-) Any pointers to this debate? -- Thomas Rast t...@thomasrast.ch -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] config: arbitrary number of matches for --unset and --replace-all
Jeff King writes:
> This code is weird to follow because of the fall-throughs. I do not
> think you have introduced any bugs with your patch, but it seems weird
> to me that we set the offset at the top of the hunk. If we hit the
> conditional in the bottom half, we do actually increment storer.seen,
> but only _after_ having overwritten the value from above (with the same
> value, no less).
>
> But if we do not follow that code path, we may end up here:
>
>> @@ -1272,6 +1275,9 @@ static int store_aux(const char *key, const char
>> *value, void *cb)
>> if (strrchr(key, '.') - key == store.baselen &&
>>!strncmp(key, store.key, store.baselen)) {
>> store.state = SECTION_SEEN;
>> +ALLOC_GROW(store.offset,
>> + store.seen+1,
>> + store.offset_alloc);
>> store.offset[store.seen] =
>> cf->do_ftell(cf);
>> }
>> }
>
> where we overwrite it again, but do not update store.seen. Or we may
> trigger neither, and leave the function with our offset stored, but
> store.seen not incremented.
It's doubly strange that we write in this hunk without any protection
against overflow. I was too lazy to think about it long enough to come
up with a possible example that triggers this, and instead just put in
the defensive ALLOC_GROW(). But if you can trigger it, it will probably
cause the algorithm to go off the rails because it overwrote store.state
and possibly even store.seen.
--
Thomas Rast
t...@thomasrast.ch
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 0/21] pack bitmaps
Jeff King writes: >> > - the ewah code used gcc's __builtin_ctzll, but did not provide a >> > suitable fallback. We now provide a fallback in C. >> >> I was messing around with several implementations (including the use of >> msvc compiler intrinsics) with the intention of doing some timing tests >> etc. [I suspected my C fallback function (a different implementation to >> yours) would be slightly faster.] > > Yeah, I looked around for several implementations, and ultimately wrote > one that was the most readable to me. The one I found shortest and most > inscrutable was: > > return popcount((x & -x) - 1); In two's complement, -x = ~x + 1 [1]. If you have a bunch of 0s at the end, as in (binary; a=~A etc) x = abcdef1000 then ~x = ABCDEF0111 ~x + 1 = -x = ABCDEF1000 (x&-x) = 001000 (x&-x) - 1 = 000111 popcount() of that is the number of trailing zeroes you started with. Please don't ask me to work out what happens in border cases; my head hurts already. [1] because x + ~x is all one bits. +1 makes it overflow to 0, so that x + -x = 0 as it should. -- Thomas Rast t...@thomasrast.ch -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v3 gitk 1/5] gitk: support -G option from the command line
From: Thomas Rast
The -G option's usage is exactly analogous to that of -S, so
supporting it is easy.
Signed-off-by: Thomas Rast
---
gitk | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/gitk b/gitk
index 5cd00d8..0e95814 100755
--- a/gitk
+++ b/gitk
@@ -227,7 +227,7 @@ proc parseviewargs {n arglist} {
"--until=*" - "--before=*" - "--max-age=*" - "--min-age=*" -
"--author=*" - "--committer=*" - "--grep=*" - "-[iE]" -
"--remove-empty" - "--first-parent" - "--cherry-pick" -
- "-S*" - "--pickaxe-all" - "--pickaxe-regex" -
+ "-S*" - "-G*" - "--pickaxe-all" - "--pickaxe-regex" -
"--simplify-by-decoration" {
# These mean that we get a subset of the commits
set filtered 1
--
1.8.5.rc2.348.gb73b695
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v3 gitk 3/5] gitk: split out diff part in $commitinfo
From: Thomas Rast
So far we just parsed everything after the headers into the "comment"
bit of $commitinfo, including notes and -- if you gave weird options
-- the diff.
Split out the diff, if any, into a separate field. It's easy to
recognize, since it always starts with /^diff/ and is preceded by an
empty line.
We take care to snip away said empty line. The display code already
properly spaces the end of the message from the first diff, and
leaving another empty line at the end looks ugly.
Signed-off-by: Thomas Rast
---
gitk | 11 ++-
1 file changed, 10 insertions(+), 1 deletion(-)
diff --git a/gitk b/gitk
index 11e988e..78b4354 100755
--- a/gitk
+++ b/gitk
@@ -1704,8 +1704,17 @@ proc parsecommit {id contents listed} {
set comment $newcomment
}
set hasnote [string first "\nNotes:\n" $contents]
+set diff ""
+# If there is diff output shown in the git-log stream, split it
+# out. But get rid of the empty line that always precedes the
+# diff.
+set i [string first "\n\ndiff" $comment]
+if {$i >= 0} {
+ set diff [string range $comment $i+1 end]
+ set comment [string range $comment 0 $i-1]
+}
set commitinfo($id) [list $headline $auname $audate \
-$comname $comdate $comment $hasnote]
+$comname $comdate $comment $hasnote $diff]
}
proc getcommit {id} {
--
1.8.5.rc2.348.gb73b695
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v3 3/3] Documentation/gitk: document -L option
The -L option is the same as for git-log, so the entire block is just copied from git-log.txt. However, until the parser is fixed we add a caveat that gitk only understands the stuck form. Signed-off-by: Thomas Rast --- Documentation/gitk.txt | 16 1 file changed, 16 insertions(+) diff --git a/Documentation/gitk.txt b/Documentation/gitk.txt index d44e14c..1e9e38a 100644 --- a/Documentation/gitk.txt +++ b/Documentation/gitk.txt @@ -98,6 +98,22 @@ linkgit:git-rev-list[1] for a complete list. (See "History simplification" in linkgit:git-log[1] for a more detailed explanation.) +-L,::: +-L + + Trace the evolution of the line range given by "," + (or the funcname regex ) within the . You may + not give any pathspec limiters. This is currently limited to + a walk starting from a single revision, i.e., you may only + give zero or one positive revision arguments. + You can specify this option more than once. ++ +*Note:* gitk (unlike linkgit:git-log[1]) currently only understands +this option if you specify it "glued together" with its argument. Do +*not* put a space after `-L`. ++ +include::line-range-format.txt[] + :: Limit the revisions to show. This can be either a single revision -- 1.8.5.rc2.348.gb73b695 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v3 2/3] Documentation: convert to --option=arg form where possible
We generally encourage the stuck form (that is, -oarg or
--option=arg), for example gitcli(7) says
when a command line option takes an argument, use the stuck form
[...] An option that takes optional option-argument must be written
in the stuck form.
However the manpages do not express the options in this form in many
places. Switch the long option descriptions to use --option=arg where
possible. The short options are a more difficult choice, so this
patch leaves them unchanged. In particular it's not clear whether the
readability tradeoff is worth it for mandatory arguments, where -o foo
is equally valid and sets 'foo' off visually from the option.
I found the possible matches by running
git grep '^--[^,]* .*::'
The following are not fixed by this patch:
* git-filter-branch
* git-quiltimport
* git-rev-parse
All of them have an ad-hoc parser that does not understand stuck
forms.
The following are fixes beyond the pure choice of style:
* git-grep actually uses parseopt to take an optional argument for
--open-files-in-pager. Therefore the unstuck form was not valid to
begin with.
* In git-revert.txt, added <...> to signal that is a
placeholder (all other options in the file already use this style).
Signed-off-by: Thomas Rast
---
Documentation/blame-options.txt| 4 ++--
Documentation/fetch-options.txt| 2 +-
Documentation/git-branch.txt | 6 +++---
Documentation/git-checkout.txt | 2 +-
Documentation/git-cherry-pick.txt | 2 +-
Documentation/git-clone.txt| 18 +-
Documentation/git-config.txt | 4 ++--
Documentation/git-credential-cache.txt | 4 ++--
Documentation/git-cvsserver.txt| 2 +-
Documentation/git-describe.txt | 2 +-
Documentation/git-fmt-merge-msg.txt| 4 ++--
Documentation/git-format-patch.txt | 4 ++--
Documentation/git-grep.txt | 10 +-
Documentation/git-notes.txt| 2 +-
Documentation/git-p4.txt | 14 +++---
Documentation/git-prune.txt| 4 ++--
Documentation/git-rebase.txt | 4 ++--
Documentation/git-replace.txt | 2 +-
Documentation/git-revert.txt | 6 +++---
git-cvsserver.perl | 2 +-
20 files changed, 49 insertions(+), 49 deletions(-)
diff --git a/Documentation/blame-options.txt b/Documentation/blame-options.txt
index 0cebc4f..3d768f1 100644
--- a/Documentation/blame-options.txt
+++ b/Documentation/blame-options.txt
@@ -55,14 +55,14 @@ include::line-range-format.txt[]
discussion about encoding in the linkgit:git-log[1]
manual page.
---contents ::
+--contents=::
When is not specified, the command annotates the
changes starting backwards from the working tree copy.
This flag makes the command pretend as if the working
tree copy has the contents of the named file (specify
`-` to make the command read from the standard input).
---date ::
+--date=::
The value is one of the following alternatives:
{relative,local,default,iso,rfc,short}. If --date is not
provided, the value of the blame.date config variable is
diff --git a/Documentation/fetch-options.txt b/Documentation/fetch-options.txt
index f0ef7d0..4a03e06 100644
--- a/Documentation/fetch-options.txt
+++ b/Documentation/fetch-options.txt
@@ -109,7 +109,7 @@ endif::git-pull[]
implementing your own Porcelain you are not supposed to
use it.
---upload-pack ::
+--upload-pack=::
When given, and the repository to fetch from is handled
by 'git fetch-pack', '--exec=' is passed to
the command to specify non-default path for the command
diff --git a/Documentation/git-branch.txt b/Documentation/git-branch.txt
index 311b336..91fc23f 100644
--- a/Documentation/git-branch.txt
+++ b/Documentation/git-branch.txt
@@ -195,15 +195,15 @@ start-point is either a local or remote-tracking branch.
Open an editor and edit the text to explain what the branch is
for, to be used by various other commands (e.g. `request-pull`).
---contains []::
+--contains[=]::
Only list branches which contain the specified commit (HEAD
if not specified). Implies `--list`.
---merged []::
+--merged[=]::
Only list branches whose tips are reachable from the
specified commit (HEAD if not specified). Implies `--list`.
---no-merged []::
+--no-merged[=]::
Only list branches whose tips are not reachable from the
specified commit (HEAD if not specified). Implies `--list`.
diff --git a/Documentation/git-checkout.txt b/Documentation/git-checkout.txt
index 91294f8..c95aed2 100644
--- a/Documentation/git-checkout.txt
+++ b/Documentation/git-checkout.txt
@@ -157,7 +157,7 @@ explicitly give a name with '-b' in such a case.
is not a branch name. See the "DETACHED
[PATCH v3 0/3] Documentation: stuck arguments and gitk log -L
This gathers three rather separate patches. 1/3 is an unrelated fix for a string comparison issue that I noticed while surveying the state of stickiness. 2/3 is the "document" plan hinted at here: > So my short-term plan just became: document instead of fix; clean up > manpages towards the stuck form for long options; have gitk only parse > -Lstuck. > > Medium term we can move gitk to a different option parser, resolving at > least that inconsistency. > > Longer term we can see about moving some more of the remaining craziness > towards parseopt, getting consistency for free. 3/3 is the change to gitk(1) to match the 'gitk -L' support that I'm sending out separately (rebased to the gitk repo) and that will appear here: http://mid.gmane.org/cover.1384622392.git...@thomasrast.ch Of course it should only be applied once the corresponding code change is in place. Thomas Rast (3): commit-tree: use prefixcmp instead of memcmp(..., N) Documentation: convert to --option=arg form where possible Documentation/gitk: document -L option Documentation/blame-options.txt| 4 ++-- Documentation/fetch-options.txt| 2 +- Documentation/git-branch.txt | 6 +++--- Documentation/git-checkout.txt | 2 +- Documentation/git-cherry-pick.txt | 2 +- Documentation/git-clone.txt| 18 +- Documentation/git-config.txt | 4 ++-- Documentation/git-credential-cache.txt | 4 ++-- Documentation/git-cvsserver.txt| 2 +- Documentation/git-describe.txt | 2 +- Documentation/git-fmt-merge-msg.txt| 4 ++-- Documentation/git-format-patch.txt | 4 ++-- Documentation/git-grep.txt | 10 +- Documentation/git-notes.txt| 2 +- Documentation/git-p4.txt | 14 +++--- Documentation/git-prune.txt| 4 ++-- Documentation/git-rebase.txt | 4 ++-- Documentation/git-replace.txt | 2 +- Documentation/git-revert.txt | 6 +++--- Documentation/gitk.txt | 16 builtin/commit-tree.c | 2 +- git-cvsserver.perl | 2 +- 22 files changed, 66 insertions(+), 50 deletions(-) -- 1.8.5.rc2.348.gb73b695 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v3 gitk 5/5] gitk: recognize -L option
From: Thomas Rast
This gives line-log support to gitk, by exploiting the new support for
processing and showing "inline" diffs straight from the git-log
output.
Note that we 'set allknown 0', which is a bit counterintuitive since
this is a "known" option. But that flag prevents gitk from thinking
it can optimize the view by running rev-list to see the topology; in
the -L case that doesn't work.
Signed-off-by: Thomas Rast
---
gitk | 8
1 file changed, 8 insertions(+)
diff --git a/gitk b/gitk
index 5ece2a1..3679467 100755
--- a/gitk
+++ b/gitk
@@ -235,6 +235,14 @@ proc parseviewargs {n arglist} {
set filtered 1
lappend glflags $arg
}
+ "-L*" {
+ # Line-log with 'sticked' argument (unsticked form is
+ # not supported)
+ set filtered 1
+ set vinlinediff($n) 1
+ set allknown 0
+ lappend glflags $arg
+ }
"-n" {
# This appears to be the only one that has a value as a
# separate word following it
--
1.8.5.rc2.348.gb73b695
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v3 gitk 0/5] gitk -L
These patches implement 'gitk -L'. They are exactly the same as the gitk patches from v2 at http://thread.gmane.org/gmane.comp.version-control.git/227151/focus=236903 except that they apply to the gitk-git tree at git://ozlabs.org/~paulus/gitk The documentation change is in the parallel series that will appear at http://mid.gmane.org/cover.1384622379.git...@thomasrast.ch Thomas Rast (5): gitk: support -G option from the command line gitk: refactor per-line part of getblobdiffline and its support gitk: split out diff part in $commitinfo gitk: support showing the gathered inline diffs gitk: recognize -L option gitk | 467 +++ 1 file changed, 270 insertions(+), 197 deletions(-) -- 1.8.5.rc2.348.gb73b695 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v3 gitk 4/5] gitk: support showing the gathered inline diffs
From: Thomas Rast
The previous commit split the diffs into a separate field. Now we
actually want to show them.
To that end we use the stored diff, and
- process it once to build a fake "tree diff", i.e., a list of all
changed files;
- feed it through parseblobdiffline to actually format it into the
$ctext field, like the existing diff machinery would.
Signed-off-by: Thomas Rast
---
gitk | 38 ++
1 file changed, 38 insertions(+)
diff --git a/gitk b/gitk
index 78b4354..5ece2a1 100755
--- a/gitk
+++ b/gitk
@@ -156,10 +156,12 @@ proc unmerged_files {files} {
proc parseviewargs {n arglist} {
global vdatemode vmergeonly vflags vdflags vrevs vfiltered vorigargs env
+global vinlinediff
global worddiff git_version
set vdatemode($n) 0
set vmergeonly($n) 0
+set vinlinediff($n) 0
set glflags {}
set diffargs {}
set nextisval 0
@@ -7089,6 +7091,7 @@ proc selectline {l isnew {desired_loc {}}} {
global cmitmode showneartags allcommits
global targetrow targetid lastscrollrows
global autoselect autosellen jump_to_here
+global vinlinediff
catch {unset pending_select}
$canv delete hover
@@ -7230,6 +7233,8 @@ proc selectline {l isnew {desired_loc {}}} {
init_flist [mc "Comments"]
if {$cmitmode eq "tree"} {
gettree $id
+} elseif {$vinlinediff($curview) == 1} {
+ showinlinediff $id
} elseif {[llength $olds] <= 1} {
startdiff $id
} else {
@@ -7566,6 +7571,39 @@ proc startdiff {ids} {
}
}
+proc showinlinediff {ids} {
+global commitinfo commitdata ctext
+global treediffs
+
+set info $commitinfo($ids)
+set diff [lindex $info 7]
+set difflines [split $diff "\n"]
+
+initblobdiffvars
+set treediff {}
+
+set inhdr 0
+foreach line $difflines {
+ if {![string compare -length 5 "diff " $line]} {
+ set inhdr 1
+ } elseif {$inhdr && ![string compare -length 4 "+++ " $line]} {
+ # offset also accounts for the b/ prefix
+ lappend treediff [string range $line 6 end]
+ set inhdr 0
+ }
+}
+
+set treediffs($ids) $treediff
+add_flist $treediff
+
+$ctext conf -state normal
+foreach line $difflines {
+ parseblobdiffline $ids $line
+}
+maybe_scroll_ctext 1
+$ctext conf -state disabled
+}
+
# If the filename (name) is under any of the passed filter paths
# then return true to include the file in the listing.
proc path_filter {filter name} {
--
1.8.5.rc2.348.gb73b695
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v3 1/3] commit-tree: use prefixcmp instead of memcmp(..., N)
Handrolling the prefix comparison is harder to read and overruns if
the argument is an empty string. Use our prefixcmp() instead.
Signed-off-by: Thomas Rast
---
builtin/commit-tree.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/builtin/commit-tree.c b/builtin/commit-tree.c
index f641ff2..19d58f9 100644
--- a/builtin/commit-tree.c
+++ b/builtin/commit-tree.c
@@ -61,7 +61,7 @@ int cmd_commit_tree(int argc, const char **argv, const char
*prefix)
continue;
}
- if (!memcmp(arg, "-S", 2)) {
+ if (!prefixcmp(arg, "-S")) {
sign_commit = arg + 2;
continue;
}
--
1.8.5.rc2.348.gb73b695
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v3 gitk 2/5] gitk: refactor per-line part of getblobdiffline and its support
From: Thomas Rast
For later use with data sources other than a pipe, refactor the big
worker part of getblobdiffline to a separate function
parseblobdiffline. Also refactor its initialization and wrap-up to
separate routines.
Signed-off-by: Thomas Rast
---
gitk | 408 +++
1 file changed, 213 insertions(+), 195 deletions(-)
diff --git a/gitk b/gitk
index 0e95814..11e988e 100755
--- a/gitk
+++ b/gitk
@@ -7710,15 +7710,25 @@ proc changeworddiff {name ix op} {
reselectline
}
+proc initblobdiffvars {} {
+global diffencoding targetline diffnparents
+global diffinhdr currdiffsubmod diffseehere
+set targetline {}
+set diffnparents 0
+set diffinhdr 0
+set diffencoding [get_path_encoding {}]
+set currdiffsubmod ""
+set diffseehere -1
+}
+
proc getblobdiffs {ids} {
global blobdifffd diffids env
-global diffinhdr treediffs
+global treediffs
global diffcontext
global ignorespace
global worddiff
global limitdiffs vfilelimit curview
-global diffencoding targetline diffnparents
-global git_version currdiffsubmod
+global git_version
set textconv {}
if {[package vcompare $git_version "1.6.1"] >= 0} {
@@ -7742,13 +7752,9 @@ proc getblobdiffs {ids} {
error_popup [mc "Error getting diffs: %s" $err]
return
}
-set targetline {}
-set diffnparents 0
-set diffinhdr 0
-set diffencoding [get_path_encoding {}]
fconfigure $bdf -blocking 0 -encoding binary -eofchar {}
set blobdifffd($ids) $bdf
-set currdiffsubmod ""
+initblobdiffvars
filerun $bdf [list getblobdiffline $bdf $diffids]
}
@@ -7814,13 +7820,17 @@ proc makediffhdr {fname ids} {
set diffline 0
}
+proc blobdiffmaybeseehere {ateof} {
+global diffseehere
+if {$diffseehere >= 0} {
+ mark_ctext_line [lindex [split $diffseehere .] 0]
+}
+maybe_scroll_ctext ateof
+}
+
proc getblobdiffline {bdf ids} {
-global diffids blobdifffd ctext curdiffstart
-global diffnexthead diffnextnote difffilestart
-global ctext_file_names ctext_file_lines
-global diffinhdr treediffs mergemax diffnparents
-global diffencoding jump_to_here targetline diffline currdiffsubmod
-global worddiff
+global diffids blobdifffd
+global ctext
set nr 0
$ctext conf -state normal
@@ -7829,212 +7839,220 @@ proc getblobdiffline {bdf ids} {
catch {close $bdf}
return 0
}
- if {![string compare -length 5 "diff " $line]} {
- if {![regexp {^diff (--cc|--git) } $line m type]} {
- set line [encoding convertfrom $line]
- $ctext insert end "$line\n" hunksep
- continue
+ parseblobdiffline $ids $line
+}
+$ctext conf -state disabled
+blobdiffmaybeseehere [eof $bdf]
+if {[eof $bdf]} {
+ catch {close $bdf}
+ return 0
+}
+return [expr {$nr >= 1000? 2: 1}]
+}
+
+proc parseblobdiffline {ids line} {
+global ctext curdiffstart
+global diffnexthead diffnextnote difffilestart
+global ctext_file_names ctext_file_lines
+global diffinhdr treediffs mergemax diffnparents
+global diffencoding jump_to_here targetline diffline currdiffsubmod
+global worddiff diffseehere
+
+if {![string compare -length 5 "diff " $line]} {
+ if {![regexp {^diff (--cc|--git) } $line m type]} {
+ set line [encoding convertfrom $line]
+ $ctext insert end "$line\n" hunksep
+ continue
+ }
+ # start of a new file
+ set diffinhdr 1
+ $ctext insert end "\n"
+ set curdiffstart [$ctext index "end - 1c"]
+ lappend ctext_file_names ""
+ lappend ctext_file_lines [lindex [split $curdiffstart "."] 0]
+ $ctext insert end "\n" filesep
+
+ if {$type eq "--cc"} {
+ # start of a new file in a merge diff
+ set fname [string range $line 10 end]
+ if {[lsearch -exact $treediffs($ids) $fname] < 0} {
+ lappend treediffs($ids) $fname
+ add_flist [list $fname]
}
- # start of a new file
- set diffinhdr 1
- $ctext insert end "\n"
- set curdiffstart [$ctext index "end - 1c"]
- lappend ctext_file_names ""
- lappend ctext_file_lines [lindex [split $curdiffstart "."] 0]
- $ctext insert end "\n" filesep
-
- if {$type eq "--cc"} {
- # start of a new file in a merge diff
- set fname [string range $line 10 end]
- if {[lsearch -exact $treediffs($ids) $fname] < 0} {
- lappend treediffs($ids) $fname
- add_flist [list $fname]
Re: submodule update and core.askpass
+cc Daniel and Jens Dmitry Neverov writes: > git -c core.askpass=pass.sh clone main-repo > cd main-repo > git submodule init > git submodule sync > git -c core.askpass=pass.sh submodule update > > The last command asks for a password interactively. > > I've run bisect, it seems like it started failing from commit > be8779f7ac9a3be9aa783df008d59082f4054f67. I've checked: submodule update > works fine in 1.8.5.rc2 with removed call to clear_local_git_env. Aside from GIT_CONFIG_PARAMETERS, which this needs, I wonder if we should also let other variables pass through. For example, if the user went out of their way to set GIT_ALTERNATE_OBJECT_DIRECTORIES, shouldn't we also respect that? The list of variables that is unset by clear_local_git_env is $(git rev-parse --local-env-vars), currently GIT_ALTERNATE_OBJECT_DIRECTORIES GIT_CONFIG GIT_CONFIG_PARAMETERS GIT_OBJECT_DIRECTORY GIT_DIR GIT_WORK_TREE GIT_IMPLICIT_WORK_TREE GIT_GRAFT_FILE GIT_INDEX_FILE GIT_NO_REPLACE_OBJECTS GIT_PREFIX -- Thomas Rast t...@thomasrast.ch -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Suggestion for git reference page
Jim Garrison writes: > The master reference TOC page at http://git-scm.com/docs links to all > the associated command reference pages, except it seems to be missing > a link for gitrevisions(7) > (http://git-scm.com/docs/gitrevisions.html). > > I've never submitted a patch and thought I would learn how... except > the website source doesn't seem to be in the git source tree. It's in this repo: https://github.com/github/gitscm-next.git Judging from a quick look, the page you linked to is generated from app/views/doc/index.html.haml Note that they work by pull requests, not patches, i.e. you should push your changes to a github fork of gitscm-next and send a pull request. -- Thomas Rast t...@thomasrast.ch -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: GSoC 2014: Summary so far, discussion starter: how to improve?
Thomas Rast writes:
> * Does libgit2 want to remain under the Git umbrella, or participate
> on its own?
>
> * Figure out the wiki situation. In previous years the project
> proposals and other important information were hosted at k.org [5] and
> github wikis [6]. Other options were floated, such as an official
> wiki hosted by github. (This is somewhat of a contentious issue that
> spans beyond GSoC.)
>
> * Find an org admin and backup. In previous years Shawn and Peff did
> this. Would you do it again?
Any opinions on these points?
I would actually favor a move to a wiki of the style that Peff proposed,
hosted by github and backed by git with either a very mild ACL or none
("bots don't know git push"). k.org wiki had a grand total of three
edits in the last 30 days (plus some bot edits for user creation).
And of course without an org admin we don't really need to go any
further...
> [5] https://git.wiki.kernel.org/index.php/SoC2011Projects
> similarly for previous years
> [6] https://github.com/peff/git/wiki/SoC-2012-Ideas
> https://github.com/trast/git/wiki/SoC-2013-Ideas
--
Thomas Rast
t...@thomasrast.ch
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[RFC PATCH] Revamp git-cherry(1)
git-cherry(1)'s "description" section has never really managed to explain to me what the command does. It contains too much explanation of the algorithm instead of simply saying what goals it achieves, and too much terminology that we otherwise do not use (fork-point instead of merge-base). Try a much more concise approach: state what it finds out, why this is neat, and how the output is formatted, in a few short paragraphs. In return, provide a longer example of how it fits into a format-patch/am based workflow. Also carefully avoid using "merge" in a context where it does not mean something that comes from git-merge(1). Instead, say "apply" in an attempt to further link to patch workflow concepts. While there, also omit the language about _which_ upstream branch we treat as the default. I literally just learned that we support having several, so let's not confuse new users here, especially considering that git-config(1) does _not_ document this. Prompted-by: a.hue...@commend.com on #git Signed-off-by: Thomas Rast --- Documentation/git-cherry.txt | 73 +--- 1 file changed, 41 insertions(+), 32 deletions(-) diff --git a/Documentation/git-cherry.txt b/Documentation/git-cherry.txt index 2d0daae..78ffddf 100644 --- a/Documentation/git-cherry.txt +++ b/Documentation/git-cherry.txt @@ -3,7 +3,7 @@ git-cherry(1) NAME -git-cherry - Find commits not merged upstream +git-cherry - Find commits not applied in upstream SYNOPSIS @@ -12,46 +12,27 @@ SYNOPSIS DESCRIPTION --- -The changeset (or "diff") of each commit between the fork-point and -is compared against each commit between the fork-point and . -The diffs are compared after removing any whitespace and line numbers. +Determine whether there are commits in `..` that are +equivalent to those in the range `..`. -Every commit that doesn't exist in the branch -has its id (sha1) reported, prefixed by a symbol. The ones that have -equivalent change already -in the branch are prefixed with a minus (-) sign, and those -that only exist in the branch are prefixed with a plus (+) symbol: - - __*__*__*__*__> - / -fork-point - \__+__+__-__+__+__-__+__> - - -If a has been given then the commits along the branch up -to and including are not reported: - - __*__*__*__*__> - / -fork-point - \__*__*-__+__> - - -Because 'git cherry' compares the changeset rather than the commit id -(sha1), you can use 'git cherry' to find out if a commit you made locally -has been applied under a different commit id. For example, -this will happen if you're feeding patches via email rather -than pushing or pulling commits directly. +The equivalence test is based on the diff, after removing whitespace +and line numbers. git-cherry therefore detects when commits have been +"copied" by means of linkgit:git-cherry-pick[1], linkgit:git-am[1] or +linkgit:git-rebase[1]. +Outputs the SHA1 of every commit in `..`, prefixed with +`-` for commits that have an equivalent in , and `+` for +commits that do not. OPTIONS --- -v:: - Verbose. + Verbose. Currently shows the commit subjects next to their + SHA1. :: Upstream branch to compare against. - Defaults to the first tracked remote branch, if available. + Defaults to the upstream branch of HEAD. :: Working branch; defaults to HEAD. @@ -59,6 +40,34 @@ OPTIONS :: Do not report commits up to (and including) limit. +EXAMPLES + + +git-cherry is frequently used in patch-based workflows (see +linkgit:gitworkflows[7]) to determine if a series of patches has been +applied by the upstream maintainer. In such a workflow you might +create and send a topic branch like this (fill in appropriate +arguments for `...`): ++ + +git checkout -b topic origin/master +# work and create some commits +git format-patch origin/master +git send-email ... 00* + ++ +Later, you can whether your changes have been applied by saying (still +on `topic`): ++ + +git fetch # update your notion of origin/master +git cherry -v + ++ +Note that this uses , and assumes that +`core.autosetupmerge` is enabled (the default). + + SEE ALSO linkgit:git-patch-id[1] -- 1.8.5.rc2.355.g6969a19 -- To unsubscribe from this list: send the line "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] Revamp git-cherry(1)
Junio C Hamano writes: >> OPTIONS >> --- >> -v:: >> -Verbose. >> +Verbose. Currently shows the commit subjects next to their >> +SHA1. > > Whenever I see "Currently", it makes me wonder "why does it need to > say that? Is there a plan to change it soon, and if so where is the > plan described?". I wanted to avoid documenting exactly what it does, so that in the future it could do more than that. Is that overly paranoid? >> +EXAMPLES >> + >> + >> +git-cherry is frequently used in patch-based workflows (see >> +linkgit:gitworkflows[7]) to determine if a series of patches has been >> +applied by the upstream maintainer. In such a workflow you might >> +create and send a topic branch like this (fill in appropriate >> +arguments for `...`): > > I think the ASCII art commit graph that shows topology which we lost > by this patch gave a more intiutive sense of what "a topic branch > like this" looked like than an incomplete skeleton of a command > sequence that would be understood by those who already know how to > work with multiple branches. Perhaps we want both? Hmm. I'll ponder for a moment and try to cook something up for v2. I can't say exactly what, but after initially trying to keep it, something felt wrong to me about the ascii art. Perhaps it's that it is only vaguely related to the actual output format. -- Thomas Rast t...@thomasrast.ch -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH] Revamp git-cherry(1)
Jeff King writes: > On Thu, Nov 21, 2013 at 12:30:56PM +0100, Thomas Rast wrote: > >> +Later, you can whether your changes have been applied by saying (still >> +on `topic`): > > s/can/& see/ ? > >> +Note that this uses , and assumes that >> +`core.autosetupmerge` is enabled (the default). > > I couldn't quite parse this. Is there a word missing before the comma, > or is it "uses and assumes that..."? I will just let this stand as evidence that I had a bad day. Two sentences ruined by botched editing in all of five paragraphs. Sheesh. Thanks for reading carefully. > Given that it is the default, I wonder if it is worth mentioning at all. > Even I, who knows what autosetupmerge does, took a minute to figure out > why it is relevant here. I suspect it may just confuse most readers. Ok, then let's remove it. -- Thomas Rast t...@thomasrast.ch -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2] Revamp git-cherry(1)
git-cherry(1)'s "description" section has never really managed to explain to me what the command does. It contains too much explanation of the algorithm instead of simply saying what goals it achieves, and too much terminology that we otherwise do not use (fork-point instead of merge-base). Try a much more concise approach: state what it finds out, why this is neat, and how the output is formatted, in a few short paragraphs. In return, provide much longer examples of how it fits into a format-patch/am based workflow, and how it compares to reading the same from git-log. Also carefully avoid using "merge" in a context where it does not mean something that comes from git-merge(1). Instead, say "apply" in an attempt to further link to patch workflow concepts. While there, also omit the language about _which_ upstream branch we treat as the default. I literally just learned that we support having several, so let's not confuse new users here, especially considering that git-config(1) does _not_ document this. Prompted-by: a.hue...@commend.com on #git Signed-off-by: Thomas Rast --- Junio C Hamano wrote: > > +EXAMPLES > > + > > + > > +git-cherry is frequently used in patch-based workflows (see > > +linkgit:gitworkflows[7]) to determine if a series of patches has been > > +applied by the upstream maintainer. In such a workflow you might > > +create and send a topic branch like this (fill in appropriate > > +arguments for `...`): > > I think the ASCII art commit graph that shows topology which we lost > by this patch gave a more intiutive sense of what "a topic branch > like this" looked like than an incomplete skeleton of a command > sequence that would be understood by those who already know how to > work with multiple branches. Perhaps we want both? Perhaps like this? I tried to tie in directly with what a user might see from git-log. This does push the ascii art rather far down in the manpage, but even with a puny laptop display and a large font size the new EXAMPLES is well on the first page of the manpage. So the hope is that a still-confused user would at least see that there are examples. Documentation/git-cherry.txt | 136 --- 1 file changed, 103 insertions(+), 33 deletions(-) diff --git a/Documentation/git-cherry.txt b/Documentation/git-cherry.txt index 2d0daae..6d14b3e 100644 --- a/Documentation/git-cherry.txt +++ b/Documentation/git-cherry.txt @@ -3,7 +3,7 @@ git-cherry(1) NAME -git-cherry - Find commits not merged upstream +git-cherry - Find commits not applied in upstream SYNOPSIS @@ -12,46 +12,26 @@ SYNOPSIS DESCRIPTION --- -The changeset (or "diff") of each commit between the fork-point and -is compared against each commit between the fork-point and . -The diffs are compared after removing any whitespace and line numbers. +Determine whether there are commits in `..` that are +equivalent to those in the range `..`. -Every commit that doesn't exist in the branch -has its id (sha1) reported, prefixed by a symbol. The ones that have -equivalent change already -in the branch are prefixed with a minus (-) sign, and those -that only exist in the branch are prefixed with a plus (+) symbol: - - __*__*__*__*__> - / -fork-point - \__+__+__-__+__+__-__+__> - - -If a has been given then the commits along the branch up -to and including are not reported: - - __*__*__*__*__> - / -fork-point - \__*__*-__+__> - - -Because 'git cherry' compares the changeset rather than the commit id -(sha1), you can use 'git cherry' to find out if a commit you made locally -has been applied under a different commit id. For example, -this will happen if you're feeding patches via email rather -than pushing or pulling commits directly. +The equivalence test is based on the diff, after removing whitespace +and line numbers. git-cherry therefore detects when commits have been +"copied" by means of linkgit:git-cherry-pick[1], linkgit:git-am[1] or +linkgit:git-rebase[1]. +Outputs the SHA1 of every commit in `..`, prefixed with +`-` for commits that have an equivalent in , and `+` for +commits that do not. OPTIONS --- -v:: - Verbose. + Show the commit subjects next to the SHA1s. :: - Upstream branch to compare against. - Defaults to the first tracked remote branch, if available. + Upstream branch to search for equivalent commits. + Defaults to the upstream branch of HEAD. :: Working branch; defaults to HEAD. @@ -59,6 +39,96 @@ OPTIONS :: Do not report commits up to (and including) limit. +EXAMPLES + + +Patch workflows +~~~ + +git-cherry is frequently used in patch
Re: What's cooking in git.git (Nov 2013, #05; Thu, 21)
Jeff King writes: >> * jk/pack-bitmap (2013-11-18) 22 commits [...] >> Borrows the bitmap index into packfiles from JGit to speed up >> enumeration of objects involved in a commit range without having to >> fully traverse the history. > > Looks like you picked up my latest re-roll with Ramsay's fix on top. > There wasn't a lot of review on this past round (I'm not surprised; it's > a dauntingly large chunk to review). I outlined a few possible open > issues in the cover letter, but I'd be happy to build those on top, > which I think will make review of them a lot easier. > > Do we want to try this in 'next' post-1.8.5, or should I try to prod an > area expert like Shawn into doing another round of review? Hmm, maybe I missed something, but AFAICS you (or Vicent) never acted on or responded to my June reviews in this thread: http://thread.gmane.org/gmane.comp.version-control.git/228918 and again mentioned here, though I didn't point out all of them: http://thread.gmane.org/gmane.comp.version-control.git/236587/focus=236740 Granted, the way I verified this was checking whether you renamed rlw_xor_run_bit() to something more fitting, so perhaps you just forgot that one thing but did all the rest. -- Thomas Rast t...@thomasrast.ch -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: What's cooking in git.git (Nov 2013, #05; Thu, 21)
Jeff King writes: >> Hmm, maybe I missed something, but AFAICS you (or Vicent) never acted on >> or responded to my June reviews in this thread: >> >> http://thread.gmane.org/gmane.comp.version-control.git/228918 [...] >> Granted, the way I verified this was checking whether you renamed >> rlw_xor_run_bit() to something more fitting, so perhaps you just forgot >> that one thing but did all the rest. > > I didn't touch that. Vicent, did you have a comment on the name (it > really does look like it is a negation, and the only caller is > ewah_not). Hmm, so it really was that one unlucky thing :-) I don't have much to say on the area, but if you think it helps you I can set aside some time RSN to review the second half of the series, too. Back in June I only looked at the first half. -- Thomas Rast t...@thomasrast.ch -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] Revamp git-cherry(1)
Junio C Hamano writes: > We are listing those that need to be added to the upstream with "+", > while listing those that can be dropped from yours if you rebase > with "-". Hinting the rationale behind the choice of "+/-" > somewhere may help as a mnemonic to the readers (see below). [...] > And the earlier "why +/-" could be done after this picture, > perhaps like: > > Here, we see that the commits A and C (marked with `-`) can > be dropped from your `topic` branch when you rebase it on > top of `origin/master`, while the commit B (marked with `+`) > still needs to be kept so that it will be sent to be applied > to `origin/master`. > > or somesuch? Good idea, thanks. Will integrate this more "what still needs to be integrated"-minded wording into a v3. -- Thomas Rast t...@thomasrast.ch -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: What's cooking in git.git (Nov 2013, #05; Thu, 21)
Vicent Marti writes: > On Fri, Nov 22, 2013 at 6:26 PM, Jeff King wrote: >> I didn't touch that. Vicent, did you have a comment on the name (it >> really does look like it is a negation, and the only caller is >> ewah_not). > > Yes, the name was ported straight from the original library and kept > as-is to make the translation more straightforward. These sources are > --again-- a translation, so I tried to remain as close to the original > Java implementation as possible. > > I agree the name is not ideal, but it does make quite a bit of sense. > It effectively inverts the word based on the run bit, which is the > equivalent of xoring it with the bit if it's one. It's a funny xor that doesn't take a second argument ;-) Anyway, let's not argue forever about the choice of this name. It's just the first thing that came to my mind from the original review, so I used it as an indicator to see if you had done something about it. It seems I picked an indicator that is not significant for the overall state. -- Thomas Rast t...@thomasrast.ch -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: What's cooking in git.git (Nov 2013, #05; Thu, 21)
Jeff King writes: > Re-rolling such a big chunk of code _is_ a pain for both me and for > reviewers, so I wouldn't mind switching to "fixes on top" instead of > re-rolling at some point. But we can do another round or two of re-roll > first. No, actually I think the plan that you sketched in the other side thread sounded nice: give it some exposure in next. I'll still try and read the rest, but that way it hopefully gets (much) more testing. -- Thomas Rast t...@thomasrast.ch -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] git-svn: Support svn:global-ignores property
Hi Aleksey Thanks for your patch. I added Eric Wong to the Cc list; all git-svn patches should go to him. Aleksey Vasenev writes: > --- Can you write a commit message? If you need a guideline for what to write there, consider this snippet from Documentation/SubmittingPatches: The body should provide a meaningful commit message, which: . explains the problem the change tries to solve, iow, what is wrong with the current code without the change. . justifies the way the change solves the problem, iow, why the result with the change is better. . alternate solutions considered but discarded, if any. In particular, I'm curious about how global-ignores are different from ordinary ignores. After reading http://svnbook.red-bean.com/en/1.7/svn.advanced.props.special.ignore.html I don't understand why the above document speaks of a "config area" that holds the global-ignores configuration, while your patch seems to treat them as "just another property" set in the same way as existing svn:ignore. How does this work? > Documentation/git-svn.txt | 12 ++-- > git-svn.perl | 46 -- > 2 files changed, 38 insertions(+), 20 deletions(-) Can you add a test or two? -- Thomas Rast t...@thomasrast.ch -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: gettext CTYPE for libc
Trần Ngọc Quân writes:
> $ git status
> fatal: Unable to read current working directory: Kh?ng c? t?p tin ho?c
> th? m?c nh? v?y
>
> So, somthing wrong with our charset.
[...]
> $ gettext --domain=libc "No such file or directory"
> Không có tập tin hoặc thư mục như vậy
>
> in git's gettext.c, it not allow CTYPE="" for all domain, so we will set
> this one individually. In this ex. I set it for libc:
>
> $ git diff
> diff --git a/gettext.c b/gettext.c
> index 71e9545..abd3978 100644
> --- a/gettext.c
> +++ b/gettext.c
> @@ -115,6 +115,7 @@ static void init_gettext_charset(const char *domain)
> setlocale(LC_CTYPE, "");
> charset = locale_charset();
> bind_textdomain_codeset(domain, charset);
> + bind_textdomain_codeset("libc", charset);
> setlocale(LC_CTYPE, "C");
> }
Do you know why this "suddenly" broke? The long comment in
init_gettext_charset() suggests that the *existing* code is there to
handle exactly this problem, and apparently it doesn't. Why? Has libc
moved the perror() strings into a separate domain in some version?
--
Thomas Rast
t...@thomasrast.ch
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 10/21] pack-bitmap: add support for bitmap indexes
Jeff King writes:
> khash.h | 338
[...]
> diff --git a/khash.h b/khash.h
> new file mode 100644
> index 000..57ff603
> --- /dev/null
> +++ b/khash.h
> @@ -0,0 +1,338 @@
> +/* The MIT License
> +
> + Copyright (c) 2008, 2009, 2011 by Attractive Chaos
> +
> + Permission is hereby granted, free of charge, to any person obtaining
> + a copy of this software and associated documentation files (the
> + "Software"), to deal in the Software without restriction, including
> + without limitation the rights to use, copy, modify, merge, publish,
> + distribute, sublicense, and/or sell copies of the Software, and to
> + permit persons to whom the Software is furnished to do so, subject to
> + the following conditions:
> +
> + The above copyright notice and this permission notice shall be
> + included in all copies or substantial portions of the Software.
> +
> + THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
> + EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
> + MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND
> + NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS
> + BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN
> + ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN
> + CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
> + SOFTWARE.
> +*/
> +
> +#ifndef __AC_KHASH_H
> +#define __AC_KHASH_H
[...]
> +static inline khint_t __kh_oid_hash(const unsigned char *oid)
> +{
> + khint_t hash;
> + memcpy(&hash, oid, sizeof(hash));
> + return hash;
> +}
> +
> +#define __kh_oid_cmp(a, b) (hashcmp(a, b) == 0)
> +
> +KHASH_INIT(sha1, const unsigned char *, void *, 1, __kh_oid_hash,
> __kh_oid_cmp)
> +typedef kh_sha1_t khash_sha1;
> +
> +KHASH_INIT(sha1_pos, const unsigned char *, int, 1, __kh_oid_hash,
> __kh_oid_cmp)
> +typedef kh_sha1_pos_t khash_sha1_pos;
> +
> +#endif /* __AC_KHASH_H */
AFAICS, the part after the [...] are additions specific to git. Is that
right?
Can we store them in a separate khash-sha1.h or some such, to make it
clearer what's what? As things stand, one has to look for an identifier
that is built from macros at the far end of a file that looks like it
was imported verbatim from klib(?). I don't know about you, but that
just took me far too long to get right.
I think I'll also lend you a hand writing Documentation/technical/api-khash.txt
(expect it tomorrow) so that we also have documentation in the git
style, where gitters can be expected to find it on their own.
All that could then nicely fit into a commit that actually says where
you conjured khash.h from and such. This information seems
conspicuously absent from this commit's message :-)
Furthermore, would it be a problem to name the second hash sha1_int
instead? I have another use for such a hash, and I can't imagine I'm
the only one. (That's not critical however, I can do the required
editing in that other series.)
--
Thomas Rast
t...@thomasrast.ch
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] Document khash
For squashing into a commit that adds khash.h.
Signed-off-by: Thomas Rast
---
> I think I'll also lend you a hand writing
> Documentation/technical/api-khash.txt
> (expect it tomorrow) so that we also have documentation in the git
> style, where gitters can be expected to find it on their own.
Here goes.
> Furthermore, would it be a problem to name the second hash sha1_int
> instead? I have another use for such a hash, and I can't imagine I'm
> the only one. (That's not critical however, I can do the required
> editing in that other series.)
Actually, let's not do that. Since everything is 'static inline'
anyway, there's no cost to simply instantiating more hashes as needed.
Documentation/technical/api-khash.txt | 109 ++
1 file changed, 109 insertions(+)
create mode 100644 Documentation/technical/api-khash.txt
diff --git a/Documentation/technical/api-khash.txt
b/Documentation/technical/api-khash.txt
new file mode 100644
index 000..e7ca883
--- /dev/null
+++ b/Documentation/technical/api-khash.txt
@@ -0,0 +1,109 @@
+khash
+=
+
+The khash API is a collection of macros that instantiate hash table
+routines for arbitrary key/value types.
+
+It lives in 'khash.h', which we imported from
+ https://github.com/attractivechaos/klib/blob/master/khash.h
+and then gitified somewhat.
+
+Usage
+-
+
+
+#import "khash.h"
+KHASH_INIT(NAME, key_t, value_t, is_map, key_hash_fn, key_equal_fn)
+
+
+The arguments are as follows:
+
+`NAME`::
+ Used to give your hash and its API a unique name. Spelled
+ `NAME` here to set it apart from the rest of the names, but
+ generally should be a lowercase C identifier.
+
+`key_t`::
+ Type of the keys for your hashes. Generally should be
+ `const`.
+
+`value_t`::
+ Type of the values of your hashes.
+
+`is_map`::
+ Whether the hash holds values. Set to 0 to create a hash for
+ use as a set.
+
+`khint_t key_hash_fn(key_t)`::
+ Hash function.
+
+`int key_equal_fn(key_t a, key_t b)`::
+ Comparison function. Return 1 if the two keys are the same, 0
+ otherwise.
+
+These two functions may also be macros.
+
+
+API
+---
+
+The above instantiation defines a single type:
+
+`kh_NAME_t`::
+ A struct that holds your hash. You should only ever use
+ pointers to it.
+
+After the above instantiation, the following functions and
+function-like macros are defined:
+
+`kh_NAME_t *kh_init_NAME(void)`::
+ Allocate and initialize a new hash table.
+
+`void kh_destroy_NAME(kh_NAME_t *)`::
+ Free the hash table.
+
+`void kh_clear_NAME(kh_NAME_t *)`::
+ Clear (but do not free) the hash table.
+
+`khint_t kh_get_NAME(const kh_NAME_t *hash, key_t key)`::
+ Find the given key in the hash table. The returned khint_t
+ should be treated as an opaque iterator token that indexes
+ into the hash. Use `kh_value` to get the value from the
+ token. If the key does not exist, returns kh_end(hash).
+
+`khint_t kh_put_NAME(const kh_NAME_t *hash, key_t key, int *ret)`::
+ Put the given key in the hash table. The returned khint_t
+ should be treated as an opaque iterator token that indexes
+ into the hash. Use `kh_value(hash, token) = value` to assign
+ a value after putting the key.
++
+'ret' tells you whether the key already existed: it is -1 if the
+operation failed; 0 if the key was already present; 1 if the bucket is
+empty; 2 if the bucket has been deleted.
+
+`kh_key(kh_NAME_t *, khint_t)`::
+ The key slot for the given iterator token. This can be used
+ as an lvalue, but you must not change the key!
+
+`kh_value(kh_NAME_t *, khint_t)`::
+`kh_val(kh_NAME_t *, khint_t)`::
+ The value slot for the given iterator token. This can be used
+ as an lvalue.
+
+`int kh_exist(const kh_NAME_t *, key_t)`::
+ Tests if the given bucket contains data.
+
+`khint_t kh_begin(const kh_NAME_t *)`::
+ The smallest iterator token.
+
+`khint_t kh_end(const kh_NAME_t *)`::
+ The beyond-the-hash iterator token; it is one larger than the
+ largest token that points to a hash member.
+
+`khint_t kh_size(const kh_NAME_t *)`::
+ The number of elements in the hash.
+
+`kh_foreach(const kh_NAME_t *, keyvar, valuevar, { ... loop body ... })`::
+ Iterate over every key-value pair in the hash. This is a
+ macro, and keyvar and valuevar must be pre-declared of type
+ key_t and value_t, respectively.
--
1.8.5.rc3.397.g2a3acd5
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 1/2] commit-slab: document clear_$slabname()
The clear_$slabname() function was only documented by source code so far. Write something about it. Signed-off-by: Thomas Rast --- commit-slab.h | 4 1 file changed, 4 insertions(+) diff --git a/commit-slab.h b/commit-slab.h index d4c8286..d77aaea 100644 --- a/commit-slab.h +++ b/commit-slab.h @@ -24,6 +24,10 @@ * to each commit. 'stride' specifies how big each array is. The slab * that id initialied by the variant without "_with_stride" associates * each commit with an array of one integer. + * + * - void clear_indegree(struct indegree *); + * + * Free the slab's data structures. */ /* allocate ~512kB at once, allowing for malloc overhead */ -- 1.8.5.rc3.397.g2a3acd5 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 2/2] commit-slab: declare functions "static inline"
This shuts up compiler warnings about unused functions.
While there, also remove the redundant second declaration of
stat_##slabname##realloc.
Signed-off-by: Thomas Rast
---
commit-slab.h | 17 -
1 file changed, 8 insertions(+), 9 deletions(-)
diff --git a/commit-slab.h b/commit-slab.h
index d77aaea..d5c353e 100644
--- a/commit-slab.h
+++ b/commit-slab.h
@@ -45,8 +45,8 @@ struct slabname {
\
}; \
static int stat_ ##slabname## realloc; \
\
-static void init_ ##slabname## _with_stride(struct slabname *s,
\
- unsigned stride)\
+static inline void init_ ##slabname## _with_stride(struct slabname *s, \
+ unsigned stride) \
{ \
unsigned int elem_size; \
if (!stride)\
@@ -58,12 +58,12 @@ struct slabname {
\
s->slab = NULL; \
} \
\
-static void init_ ##slabname(struct slabname *s) \
+static inline void init_ ##slabname(struct slabname *s)
\
{ \
init_ ##slabname## _with_stride(s, 1); \
} \
\
-static void clear_ ##slabname(struct slabname *s) \
+static inline void clear_ ##slabname(struct slabname *s) \
{ \
int i; \
for (i = 0; i < s->slab_count; i++) \
@@ -73,8 +73,8 @@ struct slabname {
\
s->slab = NULL; \
} \
\
-static elemtype *slabname## _at(struct slabname *s,\
- const struct commit *c) \
+static inline elemtype *slabname## _at(struct slabname *s, \
+ const struct commit *c) \
{ \
int nth_slab, nth_slot; \
\
@@ -94,8 +94,7 @@ struct slabname {
\
s->slab[nth_slab] = xcalloc(s->slab_size, \
sizeof(**s->slab) * s->stride);
\
return &s->slab[nth_slab][nth_slot * s->stride];
\
-} \
- \
-static int stat_ ##slabname## realloc
+}
+
#endif /* COMMIT_SLAB_H */
--
1.8.5.rc3.397.g2a3acd5
--
To unsubscribe from this list: send the line "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/2] commit-slab cleanups
I gathered these while writing an "all merge bases simultaneously" algorithm. Turns out this fancy algorithm loses vs. simply calling get_merge_bases() a lot, so I dropped that. But the cleanups seem valid anyway. Thomas Rast (2): commit-slab: document clear_$slabname() commit-slab: declare functions "static inline" commit-slab.h | 21 - 1 file changed, 12 insertions(+), 9 deletions(-) -- 1.8.5.rc3.397.g2a3acd5 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] commit-slab: declare functions "static inline"
Junio C Hamano writes:
> Thomas Rast writes:
>
>> This shuts up compiler warnings about unused functions.
>
> Thanks.
>
>> While there, also remove the redundant second declaration of
>> stat_##slabname##realloc.
>
> I think the latter was done very much deliberately to allow the
> using code to say:
>
> define_commit_slab(name, type);
>
> by ending the macro with something that requires a terminating
> semicolon. If you just remove it, doesn't it break the compilation
> by forcing the expanded source to define a function
>
> slabname ## _at(...)
> {
> ...
> };
>
> with a trailing and undesired semicolon?
Oooh. The sudden enlightenment.
--
Thomas Rast
t...@thomasrast.ch
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2] commit-slab: declare functions "static inline"
This shuts up compiler warnings about unused functions. No such
warnings are currently triggered, but if someone were to actually use
init_NAME_with_stride() as documented, they would get a warning about
init_NAME() being unused.
While there, write a comment about why we need two declarations of the
same variable.
Signed-off-by: Thomas Rast
---
Here's a version that has a fat comment instead of the removal.
Also, since I was rerolling anyway I put a reason why we need this.
In the original motivation I actually created more functions
afterwards, which made it more convincing, but the problem already
exists.
commit-slab.h | 24 ++--
1 file changed, 18 insertions(+), 6 deletions(-)
diff --git a/commit-slab.h b/commit-slab.h
index d77aaea..21d54f1 100644
--- a/commit-slab.h
+++ b/commit-slab.h
@@ -45,8 +45,8 @@ struct slabname {
\
}; \
static int stat_ ##slabname## realloc; \
\
-static void init_ ##slabname## _with_stride(struct slabname *s,
\
- unsigned stride)\
+static inline void init_ ##slabname## _with_stride(struct slabname *s, \
+ unsigned stride) \
{ \
unsigned int elem_size; \
if (!stride)\
@@ -58,12 +58,12 @@ struct slabname {
\
s->slab = NULL; \
} \
\
-static void init_ ##slabname(struct slabname *s) \
+static inline void init_ ##slabname(struct slabname *s)
\
{ \
init_ ##slabname## _with_stride(s, 1); \
} \
\
-static void clear_ ##slabname(struct slabname *s) \
+static inline void clear_ ##slabname(struct slabname *s) \
{ \
int i; \
for (i = 0; i < s->slab_count; i++) \
@@ -73,8 +73,8 @@ struct slabname {
\
s->slab = NULL; \
} \
\
-static elemtype *slabname## _at(struct slabname *s,\
- const struct commit *c) \
+static inline elemtype *slabname## _at(struct slabname *s, \
+ const struct commit *c) \
{ \
int nth_slab, nth_slot; \
\
@@ -98,4 +98,16 @@ struct slabname {
\
\
static int stat_ ##slabname## realloc
+/*
+ * Note that this seemingly redundant second declaration is required
+ * to allow a terminating semicolon, which makes instantiations look
+ * like function declarations. I.e., the expansion of
+ *
+ *define_commit_slab(indegree, int);
+ *
+ * ends in 'static int stat_indegreerealloc;'. This would otherwise
+ * be a syntax error according (at least) to ISO C. It's hard to
+ * catch because GCC silently parses it by default.
+ */
+
#endif /* COMMIT_SLAB_H */
--
1.8.5.rc2.355.g6969a19
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] commit-slab: declare functions "static inline"
Jonathan Nieder writes: > Thomas Rast wrote: > >> This shuts up compiler warnings about unused functions. > > If that is the only goal, I think it would be cleaner to use > > #define MAYBE_UNUSED __attribute__((__unused__)) > > static MAYBE_UNUSED void init_ ... > > like was done in the vcs-svn/ directory until cba3546 (drop obj_pool, > 2010-12-13) et al. > > I haven't thought carefully about whether encouraging inlining here > (or encouraging the reader to think of these functions as inline) is a > good or bad change. Hmm. I actually had this idea after seeing the same trick in khash.h. Is __atribute__((__unused__)) universal? If so, maybe we could apply the same also to khash? If not, I'd rather go with the inline. -- Thomas Rast t...@thomasrast.ch -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] bash prompt: add option to disable for a repository
Heikki Hokkanen writes: > On Sat, Nov 23, 2013 at 4:42 PM, Johannes Sixt wrote: >> Gah! This adds a fork+exec each time the prompt is shown. Not good, >> particularly on Windows. >> >> Since your intent is to disable the prompt in the home directory, >> wouldn't that mean that most of the time you *don't* want the prompt? >> Wouldn't you be better served with a method that *turns on* the prompt? >> For example, a shell function that sets PS1 and another one that unsets >> it? Or a wrapper that inspects a shell variable and calls __git_ps1 only >> when you want a prompt. > > Actually, I do want the prompt for all other git repositories. The > problem with $HOME is that it's the default directory after logging in > or opening a terminal, so if you have git prompt sourced and your > $HOME under git, you get an unbearable delay every time you open a > terminal, or type a command, anywhere, except for a separate git > repository. Umm... is __git_ps1 by itself so slow that you find it unbearable, or is it the worktree status discovery? Because the latter can already be controlled per repository via bash.showUntrackedFiles and bash.showUpstream. -- Thomas Rast t...@thomasrast.ch -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: How to pre-empt git pull merge error?
Antoine Pelisse writes: >>> On Wed, 27 Nov 2013 15:17:27 + >>> Pete Forman wrote: >>> >>>> I am looking for a way of detecting up front whether a git pull or git >>>> merge would fail. The sort of script I want to perform is to update a >>>> server. >>>> >>>> git fetch >>>> git okay >>>> stop server >>>> backup data >>>> git merge >>>> start server >>>> >> I don't know a simple way to do the pre-merge check without actually >> doing the merge (other than patching git merge to add a --dry-run >> option) > > Wouldn't that be a nice use-case for git-recursive-merge --index-only > ($gmane/236753) ? Possibly, but most of the use-cases for merge --dry-run are better answered by the XY Problem question: Can you step back and explain what the *underlying* goal is? The above sounds a lot like a deployment script, and such scripts are almost always better served by using an actual deployment tool, or failing that, by using some form of checkout -f instead, to ensure that they get whatever they are supposed to deploy. (Using a merge to update is really terrible in the face of non-fast-forward updates, especially when caused by rewriting history to not include some commits.) -- Thomas Rast t...@thomasrast.ch -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] gitk: make pointer selection visible in highlighted lines
Max Kirillov writes:
> Custom tags have higher priority than sel, and when they define
> their own background, it makes selection invisible. Especially
> inconvenient for filesep (to select filenames), but may aslo affect
> other tags.
>
> Signed-off-by: Max Kirillov
Nice.
Tested-by: Thomas Rast
> ---
> gitk-git/gitk | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/gitk-git/gitk b/gitk-git/gitk
> index 5cd00d8..9f350ab 100755
> --- a/gitk-git/gitk
> +++ b/gitk-git/gitk
> @@ -2385,6 +2385,7 @@ proc makewindow {} {
> $ctext tag conf found -back $foundbgcolor
> $ctext tag conf currentsearchhit -back $currentsearchhitbgcolor
> $ctext tag conf wwrap -wrap word
> +$ctext tag raise sel
>
> .pwbottom add .bleft
> if {!$use_ttk} {
--
Thomas Rast
t...@thomasrast.ch
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] subtree: fix argument validation in add/pull/push
Anthony Baire writes: > When working with a remote repository add/pull/push do not accept a > as parameter but just a . They should accept any > well-formatted ref name. [...] > - update the doc to use instead of [...] > OPTS_SPEC="\ > git subtree add --prefix= > -git subtree add --prefix= > +git subtree add --prefix= > git subtree merge --prefix= > -git subtree pull --prefix= > -git subtree push --prefix= > +git subtree pull --prefix= > +git subtree push --prefix= > git subtree split --prefix= [...] > @@ -68,7 +68,7 @@ COMMANDS > > add:: > Create the subtree by importing its contents > - from the given or and remote . > + from the given or and remote . AFAICS you are changing refspec->commit in the manpage, but commit->ref in the usage message for 'subtree add'? How does this line up? -- Thomas Rast t...@thomasrast.ch -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH/WIP] Repair DF conflicts during fetch.
Tom Miller writes:
> When a DF conflict occurs during a fetch, --prune should be able to fix
> it. When fetching with --prune, the fetching process happens before
> pruning causing the DF conflict to persist and report an error. This
> patch prunes before fetching, thus correcting DF conflicts during a
> fetch.
>
> Signed-off-by: Tom Miller
> ---
> builtin/fetch.c | 10 +-
> 1 file changed, 5 insertions(+), 5 deletions(-)
Good catch.
I can't comment on the correctness of the patch right now, but here's a
test you could steal. It just reproduces what you describe, and I did
verify that it confirms the fix ;-)
diff --git i/t/t5510-fetch.sh w/t/t5510-fetch.sh
index 5d4581d..a981125 100755
--- i/t/t5510-fetch.sh
+++ w/t/t5510-fetch.sh
@@ -614,4 +614,18 @@ test_expect_success 'all boundary commits are excluded' '
test_bundle_object_count .git/objects/pack/pack-${pack##pack}.pack 3
'
+test_expect_success 'branchname D/F conflict resolved by --prune' '
+ git branch dir/file &&
+ git clone . prune-df-conflict &&
+ git branch -D dir/file &&
+ git branch dir &&
+ (
+ cd prune-df-conflict &&
+ git fetch --prune &&
+ git rev-parse origin/dir >../actual
+ ) &&
+ git rev-parse dir >expect &&
+ test_cmp expect actual
+'
+
test_done
--
Thomas Rast
t...@thomasrast.ch
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] commit-slab: document clear_$slabname()
Jonathan Nieder writes: > Thomas Rast wrote: > >> + * >> + * - void clear_indegree(struct indegree *); >> + * >> + * Free the slab's data structures. > > Tense shift (previous descriptions were in the present tense, while > this one is in the imperative). > > More importantly, this doesn't answer the questions I'd have if I were > in a hurry, which are what exactly is being freed (has the slab taken > ownership of any memory from the user, e.g. when elemtype is a > pointer?) and whether the slab needs to be init_ ed again. > > Maybe something like the following would work? [...] Ok, I see that while I was procrastinating, you sorted this out and Junio merged it to next. Thanks, both. -- Thomas Rast t...@thomasrast.ch -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] stash: handle specifying stashes with spaces
Øystein Walle writes:
> When trying to pop/apply a stash specified with an argument containing
> spaces the user will see an error:
>
> $ git stash pop 'stash@{two hours ago}'
> Too many revisions specified: stash@{two hours ago}
>
> This happens because word splitting is used to count non-option
> arguments. Instead shift the positional arguments as the options are
> processed; the number of arguments left is then the number we're after.
[...]
> for opt
> do
> case "$opt" in
> -q|--quiet)
> GIT_QUIET=-t
> + shift
> ;;
> --index)
> INDEX_OPTION=--index
> + shift
> ;;
> -*)
> FLAGS="${FLAGS}${FLAGS:+ }$opt"
> + shift
> ;;
> esac
> done
>
> - set -- $REV
> -
But this isn't correct any more, is it? You unconditionally shift off
arguments when you see something of the form -*, even if what you shift
is not what you're currently looking at.
For example, without this patch:
$ g stash apply stash@{0} --index
On branch next
Your branch is ahead of 'origin/next' by 41 commits.
(use "git push" to publish your local commits)
[blah blah]
but with this patch:
$ g stash apply stash@{0} --index
--index is not valid reference
Granted, git-stash is extremely inconsistent in its handling of options.
For example, 'git stash save foo -k' does _not_ treat -k as an option.
If you set out to unify this (not just randomly (un)break one
subroutine) I'd be all for it.
--
Thomas Rast
t...@thomasrast.ch
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 10/21] pack-bitmap: add support for bitmap indexes
TLDR: nitpicks. Thanks for a very nice read.
I do think it's worth fixing the syntax pedantry at the end so that we
can keep supporting arcane compilers, but otherwise, meh.
> +static int open_pack_bitmap_1(struct packed_git *packfile)
This goes somewhat against the naming convention (if you can call it
that) used elsewhere in git. Usually foo_1() is an implementation
detail of foo(), used because it is convenient to wrap the main part in
another function, e.g. so that it can consistently free resources or
some such. But this one operates on one pack file, so in the terms of
the rest of git, it should probably be called open_pack_bitmap_one().
> +static void show_object(struct object *object, const struct name_path *path,
> + const char *last, void *data)
> +{
> + struct bitmap *base = data;
> + int bitmap_pos;
> +
> + bitmap_pos = bitmap_position(object->sha1);
> +
> + if (bitmap_pos < 0) {
> + char *name = path_name(path, last);
> + bitmap_pos = ext_index_add_object(object, name);
> + free(name);
> + }
> +
> + bitmap_set(base, bitmap_pos);
> +}
> +
> +static void show_commit(struct commit *commit, void *data)
> +{
> +}
A bit unfortunate that you inherit the strange show_* naming from
builtin/pack-objects.c, which seems to have stolen some code from
builtin/rev-list.c at some point without worrying about better naming...
> +static void show_objects_for_type(
> + struct bitmap *objects,
> + struct ewah_bitmap *type_filter,
> + enum object_type object_type,
> + show_reachable_fn show_reach)
> +{
[...]
> + while (i < objects->word_alloc && ewah_iterator_next(&filter, &it)) {
> + eword_t word = objects->words[i] & filter;
> +
> + for (offset = 0; offset < BITS_IN_WORD; ++offset) {
> + const unsigned char *sha1;
> + struct revindex_entry *entry;
> + uint32_t hash = 0;
> +
> + if ((word >> offset) == 0)
> + break;
> +
> + offset += ewah_bit_ctz64(word >> offset);
> +
> + if (pos + offset < bitmap_git.reuse_objects)
> + continue;
> +
> + entry = &bitmap_git.reverse_index->revindex[pos +
> offset];
> + sha1 = nth_packed_object_sha1(bitmap_git.pack,
> entry->nr);
> +
> + show_reach(sha1, object_type, 0, hash, bitmap_git.pack,
> entry->offset);
> + }
You have a very nice bitmap_each_bit() function in ewah/bitmap.c, why
not use it here?
> +int reuse_partial_packfile_from_bitmap(struct packed_git **packfile,
> +uint32_t *entries,
> +off_t *up_to)
> +{
> + /*
> + * Reuse the packfile content if we need more than
> + * 90% of its objects
> + */
> + static const double REUSE_PERCENT = 0.9;
Curious: is this based on some measurements or just a guess?
> diff --git a/pack-bitmap.h b/pack-bitmap.h
[...]
> +static const char BITMAP_IDX_SIGNATURE[] = {'B', 'I', 'T', 'M'};;
There's a stray ; at the end of the line that is technically not
permitted:
pack-bitmap.h:22:65: warning: ISO C does not allow extra ‘;’ outside of a
function [-Wpedantic]
> +enum pack_bitmap_opts {
> + BITMAP_OPT_FULL_DAG = 1,
And I think this trailing comma on the last enum item is also strictly
speaking not allowed, even though it is very nice to have:
pack-bitmap.h:28:27: warning: comma at end of enumerator list [-Wpedantic]
--
Thomas Rast
t...@thomasrast.ch
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] stash: handle specifying stashes with spaces
Thanks for looking into this!
Øystein Walle writes:
> - REV=$(git rev-parse --quiet --symbolic --verify $1 2>/dev/null) || {
> + REV=$(git rev-parse --quiet --symbolic --verify "$1" 2>/dev/null) || {
> reference="$1"
It's somewhat ironic that the one place where the original did use
quoting is where it's actually not required.
> - i_commit=$(git rev-parse --quiet --verify $REV^2 2>/dev/null) &&
> - set -- $(git rev-parse $REV $REV^1 $REV: $REV^1: $REV^2: 2>/dev/null) &&
> + i_commit=$(git rev-parse --quiet --verify "$REV^2" 2>/dev/null) &&
> + set -- $(git rev-parse "$REV" "$REV^1" "$REV:" "$REV^1:" "$REV^2:"
> 2>/dev/null) &&
Thanks for being careful here.
I wonder what we would lose by dropping the --symbolic in the line I
quoted above (which is the second parsing pass), so that it resolves to
a SHA1. We would gain some robustness, as I'm not sure "$REV:" works
correctly in the face of weird revision expressions like ":/foo".
--
Thomas Rast
t...@thomasrast.ch
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: git stash doesn't honor --work-tree or GIT_WORK_TREE
Øystein Walle writes: > Aaron Brooks brooks1.net> writes: > >> Unlike other commands, git stash doesn't work outside of the worktree, >> even when --work-tree is specified: [...] > The environment variables are properly exported. I verified this by > adding 'echo $GIT_WORK_TREE; echo $GIT_DIR' at the top of git-stash.sh. > So these should propagate to "child gits" just fine, and so it shouldn't > be necessary to test them explicitly. > > The problem seems to be that git rev-parse --is-inside-work-tree does > not honor these. In fact it doesn't even honor --git-dir or --work-tree. > Judging by the name this may be intentional. Thanks for investigating this. Duy, you are the expert on the worktree detection logic. Do you know if there is a reason for --is-inside-work-tree to not honor the GIT_WORK_TREE / GIT_DIR overrides? -- Thomas Rast t...@thomasrast.ch -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: git stash doesn't honor --work-tree or GIT_WORK_TREE
Duy Nguyen writes:
> On Sun, Dec 1, 2013 at 6:12 PM, Thomas Rast wrote:
>> Øystein Walle writes:
>>> The problem seems to be that git rev-parse --is-inside-work-tree does
>>> not honor these. In fact it doesn't even honor --git-dir or --work-tree.
>>> Judging by the name this may be intentional.
>>
>> Thanks for investigating this.
>>
>> Duy, you are the expert on the worktree detection logic. Do you know if
>> there is a reason for --is-inside-work-tree to not honor the
>> GIT_WORK_TREE / GIT_DIR overrides?
>
> It should. At the beginning of cmd_rev_parse(), setup_git_directory()
> is called, which will check and follow all GIT_* or their command line
> equivalent. I'll look into this some time later.
Hrm, I'm wrong, and it's not clear what to actually do in this case.
--is-inside-work-tree works as claimed: if we can detect a git
repository and your current directory is inside it, you are "inside a
worktree".
The problem here is that shell scripts that want to do something with a
worktree tend to call require_work_tree in git-sh-setup:
require_work_tree () {
test "$(git rev-parse --is-inside-work-tree 2>/dev/null)" = true ||
die "fatal: $0 cannot be used without a working tree."
}
However, when an explicit GIT_WORK_TREE is in effect, that seems a bit
silly. The _intent_ of that command is "I need a worktree to work
with". But what it currently checks is something completely different,
namely "am I _inside_ the worktree".
--show-cdup might be a better candidate, because it actually (and
despite what the docs suggest, sigh) shows the path to the worktree as
with --show-toplevel for the case where you are not --is-inside-work-tree.
Which means that the output --show-cdup is in fact not necessarily "up"
from the worktree, but something you could cd to to get to its top.
Long story short, this might work (and it passes tests):
diff --git i/git-sh-setup.sh w/git-sh-setup.sh
index 190a539..47c7f1d 100644
--- i/git-sh-setup.sh
+++ w/git-sh-setup.sh
@@ -192,8 +192,12 @@ require_work_tree_exists () {
}
require_work_tree () {
- test "$(git rev-parse --is-inside-work-tree 2>/dev/null)" = true ||
+ test "$(git rev-parse --is-inside-work-tree 2>/dev/null)" = true &&
+ return 0
+ cdup="$(git rev-parse --show-cdup 2>/dev/null)"
+ test -z "$cdup" &&
die "fatal: $0 cannot be used without a working tree."
+ cd "$cdup"
}
require_clean_work_tree () {
But it would give require_work_tree somewhat interesting and unintuitive
side effects.
--
Thomas Rast
t...@thomasrast.ch
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] commit-slab: sizeof() the right type in xrealloc
When allocating the slab, the code accidentally computed the array
size from s->slab (an elemtype**). The slab is an array of elemtype*,
however, so we should take the size of *s->slab.
Noticed-by: Nguyễn Thái Ngọc Duy
Signed-off-by: Thomas Rast
---
[I hope this comes through clean. git-send-email is currently broken
for me, and I'm still investigating, so I have to kludge around it.]
I browsed around for a while, and couldn't find out whether any
architecture actually has any hope of running git (i.e. is at least
mostly POSIX conformant) but still violates the assumption that all
pointers[*] are the same size.
The comp.lang.c FAQ has some interesting examples of wildly different
pointer representations at:
http://c-faq.com/null/machexamp.html
Consider the cases mentioned there where void* and char* have
different representations from, e.g., int*. Then if elemtype is char,
the slab will be too small before this patch.
But I have no idea if any of those are POSIXish.
One interesting, though orthogonal, tidbit is that POSIX actually
requires _function_ pointers to have the same representation as void*.
>From the specification of dlsym(), which depends on this to be able to
return function pointers:
RATIONALE
[...] Indeed, the ISO C standard does not require that an object of
type void * can hold a pointer to a function. Implementations
supporting the XSI extension, however, do require that an object of
type void * can hold a pointer to a function.
Thank god for POSIX. So much craziness averted.
[*] Note that C++ method pointers are yet another story. This only
applies to the kinds of pointers that C supports.
commit-slab.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/commit-slab.h b/commit-slab.h
index d068e2d..cc114b5 100644
--- a/commit-slab.h
+++ b/commit-slab.h
@@ -91,7 +91,7 @@ struct slabname {
\
if (s->slab_count <= nth_slab) {\
int i; \
s->slab = xrealloc(s->slab, \
- (nth_slab + 1) * sizeof(s->slab)); \
+ (nth_slab + 1) * sizeof(*s->slab)); \
stat_ ##slabname## realloc++; \
for (i = s->slab_count; i <= nth_slab; i++) \
s->slab[i] = NULL; \
--
1.8.5.427.g6d3141d
--
To unsubscribe from this list: send the line "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/3] send-email: --smtp-ssl-cert-path takes an argument
35035bb (send-email: be explicit with SSL certificate verification,
2013-07-18) forgot to specify that --smtp-ssl-cert-path takes a string
argument. This means that the option could not actually be used as
intended. Presumably noone noticed because it's much easier to set it
through configs anyway.
Add the required "=s".
Signed-off-by: Thomas Rast
---
git-send-email.perl | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/git-send-email.perl b/git-send-email.perl
index f7468b6..9f31c68 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -291,7 +291,7 @@ sub signal_handler {
"smtp-pass:s" => \$smtp_authpass,
"smtp-ssl" => sub { $smtp_encryption = 'ssl' },
"smtp-encryption=s" => \$smtp_encryption,
- "smtp-ssl-cert-path" => \$smtp_ssl_cert_path,
+ "smtp-ssl-cert-path=s" => \$smtp_ssl_cert_path,
"smtp-debug:i" => \$debug_net_smtp,
"smtp-domain:s" => \$smtp_domain,
"identity=s" => \$identity,
--
1.8.5.rc3.5.g2a1fe2f
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
