Re: [PATCH v2 1/1] zlib.c: use size_t for size
On Sun, Oct 14, 2018 at 03:16:36AM +0100, Ramsay Jones wrote:
> diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
> index b059b86aee..3b5f2c38b3 100644
> --- a/builtin/pack-objects.c
> +++ b/builtin/pack-objects.c
> @@ -269,12 +269,12 @@ static void copy_pack_data(struct hashfile *f,
> off_t len)
> {
> unsigned char *in;
> - unsigned long avail;
> + size_t avail;
>
> while (len) {
> in = use_pack(p, w_curs, offset, );
> if (avail > len)
> - avail = (unsigned long)len;
> + avail = xsize_t(len);
We don't actually care about truncation here. The idea is that we take a
bite-sized chunk via use_pack, and loop as necessary. So mod-2^32
truncation via a cast would be bad (we might not make forward progress),
but truncating to SIZE_MAX would be fine.
That said, we know that would not truncate here, because we must
strictly be shrinking "avail", which was already a size_t (unless "len"
is negative, but then we are really screwed ;) ).
So I kind of wonder if a comment would be better than xsize_t here.
Something like:
if (avail > len) {
/*
* This can never truncate because we know that len is smaller
* than avail, which is already a size_t.
*/
avail = (size_t)len;
}
-Peff
Re: [PATCH v2 1/1] zlib.c: use size_t for size
On 14/10/18 03:16, Ramsay Jones wrote:
>
>
> On 13/10/18 06:00, Torsten Bögershausen wrote:
>> []
>>> Neither v1 nor v2 of this patch compiles on 32 bit Linux; see
>>>
>>> https://travis-ci.org/git/git/jobs/440514375#L628
>>>
>>> The fixup patch below makes it compile on 32 bit and the test suite
>>> passes as well, but I didn't thoroughly review the changes; I only
>>> wanted to get 'pu' build again.
>>>
>>
>> Oh, yes, I didn't test under Linux 32 bit (and neither Windows)
>> I will try to compose a proper v3 the next days.
>
> I had a look at this today, and the result is given below.
>
> The patch is effectively your v2 patch with SZEDER's fix-up
> patch on top! (I actually started with my own patch and 'pared
> it back' by removing the off_t -> size_t changes, etc; so I was
> somewhat amused to note that the end result was effectively
> SZEDER's patch on top of your v2 ;-) ).
>
> I have tested this on 32- and 64-bit Linux, along with 64-bit
> cygwin (the test suite run hasn't finished yet, but I don't
> expect any problem). I have an old Msys2 installation on Windows,
> which is the closest thing I have to a windows dev system, so I
> also built this on MINGW32 and MINGW64 along with some light
> manual testing (the test suite has never passed on Msys2 for me).
> This is not the same as testing on Gfw, of course.
Ho, Hum. Of course, I have just noticed that, similar to
copy_pack_data(), check_pack_crc() should probably use a
call to xsize_t(len) when assigning to avail.
Sorry about that. (I need some sleep now!)
ATB,
Ramsay Jones
> -- >8 --
> From: Martin Koegler
> Subject: [PATCH v3 1/1] zlib.c: use size_t for size
>
> Signed-off-by: Martin Koegler
> Signed-off-by: Junio C Hamano
> Signed-off-by: Torsten Bögershausen
> Helped-by: SZEDER Gábor
> Signed-off-by: Ramsay Jones
> ---
> builtin/pack-objects.c | 11 ++-
> cache.h| 10 +-
> pack-check.c | 4 ++--
> packfile.c | 4 ++--
> packfile.h | 2 +-
> wrapper.c | 8
> zlib.c | 8
> 7 files changed, 24 insertions(+), 23 deletions(-)
>
> diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
> index b059b86aee..3b5f2c38b3 100644
> --- a/builtin/pack-objects.c
> +++ b/builtin/pack-objects.c
> @@ -269,12 +269,12 @@ static void copy_pack_data(struct hashfile *f,
> off_t len)
> {
> unsigned char *in;
> - unsigned long avail;
> + size_t avail;
>
> while (len) {
> in = use_pack(p, w_curs, offset, );
> if (avail > len)
> - avail = (unsigned long)len;
> + avail = xsize_t(len);
> hashwrite(f, in, avail);
> offset += avail;
> len -= avail;
> @@ -1529,8 +1529,8 @@ static void check_object(struct object_entry *entry)
> struct pack_window *w_curs = NULL;
> const unsigned char *base_ref = NULL;
> struct object_entry *base_entry;
> - unsigned long used, used_0;
> - unsigned long avail;
> + size_t used, used_0;
> + size_t avail;
> off_t ofs;
> unsigned char *buf, c;
> enum object_type type;
> @@ -2002,7 +2002,8 @@ unsigned long oe_get_size_slow(struct packing_data
> *pack,
> struct pack_window *w_curs;
> unsigned char *buf;
> enum object_type type;
> - unsigned long used, avail, size;
> + unsigned long used, size;
> + size_t avail;
>
> if (e->type_ != OBJ_OFS_DELTA && e->type_ != OBJ_REF_DELTA) {
> read_lock();
> diff --git a/cache.h b/cache.h
> index 5d83022e3b..ba0ad73de1 100644
> --- a/cache.h
> +++ b/cache.h
> @@ -20,10 +20,10 @@
> #include
> typedef struct git_zstream {
> z_stream z;
> - unsigned long avail_in;
> - unsigned long avail_out;
> - unsigned long total_in;
> - unsigned long total_out;
> + size_t avail_in;
> + size_t avail_out;
> + size_t total_in;
> + size_t total_out;
> unsigned char *next_in;
> unsigned char *next_out;
> } git_zstream;
> @@ -40,7 +40,7 @@ void git_deflate_end(git_zstream *);
> int git_deflate_abort(git_zstream *);
> int git_deflate_end_gently(git_zstream *);
> int git_deflate(git_zstream *, int flush);
> -unsigned long git_deflate_bound(git_zstream *, unsigned long);
> +size_t git_deflate_bound(git_zstream *, size_t);
>
> /* The length in bytes and in hex digits of an object name (SHA-1 value). */
> #define GIT_SHA1_RAWSZ 20
> diff --git a/pack-check.c b/pack-check.c
> index fa5f0ff8fa..d1e7f554ae 100644
> --- a/pack-check.c
> +++ b/pack-check.c
> @@ -33,7 +33,7 @@ int check_pack_crc(struct packed_git *p, struct pack_window
> **w_curs,
> uint32_t data_crc = crc32(0, NULL, 0);
>
> do {
> - unsigned long avail;
> + size_t avail;
> void *data = use_pack(p,
Re: [PATCH v2 1/1] zlib.c: use size_t for size
On 13/10/18 06:00, Torsten Bögershausen wrote:
> []
>> Neither v1 nor v2 of this patch compiles on 32 bit Linux; see
>>
>> https://travis-ci.org/git/git/jobs/440514375#L628
>>
>> The fixup patch below makes it compile on 32 bit and the test suite
>> passes as well, but I didn't thoroughly review the changes; I only
>> wanted to get 'pu' build again.
>>
>
> Oh, yes, I didn't test under Linux 32 bit (and neither Windows)
> I will try to compose a proper v3 the next days.
I had a look at this today, and the result is given below.
The patch is effectively your v2 patch with SZEDER's fix-up
patch on top! (I actually started with my own patch and 'pared
it back' by removing the off_t -> size_t changes, etc; so I was
somewhat amused to note that the end result was effectively
SZEDER's patch on top of your v2 ;-) ).
I have tested this on 32- and 64-bit Linux, along with 64-bit
cygwin (the test suite run hasn't finished yet, but I don't
expect any problem). I have an old Msys2 installation on Windows,
which is the closest thing I have to a windows dev system, so I
also built this on MINGW32 and MINGW64 along with some light
manual testing (the test suite has never passed on Msys2 for me).
This is not the same as testing on Gfw, of course.
ATB,
Ramsay Jones
-- >8 --
From: Martin Koegler
Subject: [PATCH v3 1/1] zlib.c: use size_t for size
Signed-off-by: Martin Koegler
Signed-off-by: Junio C Hamano
Signed-off-by: Torsten Bögershausen
Helped-by: SZEDER Gábor
Signed-off-by: Ramsay Jones
---
builtin/pack-objects.c | 11 ++-
cache.h| 10 +-
pack-check.c | 4 ++--
packfile.c | 4 ++--
packfile.h | 2 +-
wrapper.c | 8
zlib.c | 8
7 files changed, 24 insertions(+), 23 deletions(-)
diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index b059b86aee..3b5f2c38b3 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -269,12 +269,12 @@ static void copy_pack_data(struct hashfile *f,
off_t len)
{
unsigned char *in;
- unsigned long avail;
+ size_t avail;
while (len) {
in = use_pack(p, w_curs, offset, );
if (avail > len)
- avail = (unsigned long)len;
+ avail = xsize_t(len);
hashwrite(f, in, avail);
offset += avail;
len -= avail;
@@ -1529,8 +1529,8 @@ static void check_object(struct object_entry *entry)
struct pack_window *w_curs = NULL;
const unsigned char *base_ref = NULL;
struct object_entry *base_entry;
- unsigned long used, used_0;
- unsigned long avail;
+ size_t used, used_0;
+ size_t avail;
off_t ofs;
unsigned char *buf, c;
enum object_type type;
@@ -2002,7 +2002,8 @@ unsigned long oe_get_size_slow(struct packing_data *pack,
struct pack_window *w_curs;
unsigned char *buf;
enum object_type type;
- unsigned long used, avail, size;
+ unsigned long used, size;
+ size_t avail;
if (e->type_ != OBJ_OFS_DELTA && e->type_ != OBJ_REF_DELTA) {
read_lock();
diff --git a/cache.h b/cache.h
index 5d83022e3b..ba0ad73de1 100644
--- a/cache.h
+++ b/cache.h
@@ -20,10 +20,10 @@
#include
typedef struct git_zstream {
z_stream z;
- unsigned long avail_in;
- unsigned long avail_out;
- unsigned long total_in;
- unsigned long total_out;
+ size_t avail_in;
+ size_t avail_out;
+ size_t total_in;
+ size_t total_out;
unsigned char *next_in;
unsigned char *next_out;
} git_zstream;
@@ -40,7 +40,7 @@ void git_deflate_end(git_zstream *);
int git_deflate_abort(git_zstream *);
int git_deflate_end_gently(git_zstream *);
int git_deflate(git_zstream *, int flush);
-unsigned long git_deflate_bound(git_zstream *, unsigned long);
+size_t git_deflate_bound(git_zstream *, size_t);
/* The length in bytes and in hex digits of an object name (SHA-1 value). */
#define GIT_SHA1_RAWSZ 20
diff --git a/pack-check.c b/pack-check.c
index fa5f0ff8fa..d1e7f554ae 100644
--- a/pack-check.c
+++ b/pack-check.c
@@ -33,7 +33,7 @@ int check_pack_crc(struct packed_git *p, struct pack_window
**w_curs,
uint32_t data_crc = crc32(0, NULL, 0);
do {
- unsigned long avail;
+ size_t avail;
void *data = use_pack(p, w_curs, offset, );
if (avail > len)
avail = len;
@@ -68,7 +68,7 @@ static int verify_packfile(struct packed_git *p,
the_hash_algo->init_fn();
do {
- unsigned long remaining;
+ size_t remaining;
unsigned char *in = use_pack(p, w_curs, offset, );
offset += remaining;
if
Re: [BUG] gitignore documentation inconsistent with actual behaviour
On 11 Oct 2018, at 06:08, Ævar Arnfjörð Bjarmason wrote: >Our matching function comes from rsync originally, whose manpage says: > > use ’**’ to match anything, including slashes. > >I believe this is accurate as far as the implementation goes. You can >also see the rather exhaustive tests here: >https://github.com/git/git/blob/master/t/t3070-wildmatch.sh Thanks. The tests are a little hard to follow at first glance, so i guess i'd have to study them more to see what the semantics actually are. When i tested it briefly it didn't seem like it was behaving as you described, but maybe i wasn't paying close enough attention. In any case, i'm sure these will be useful. On 11 Oct 2018, at 06:08, Ævar Arnfjörð Bjarmason wrote: >The reason I'm on this tangent is to ask whether if such a thing >existed, if it's something you can see something like ripgrep >using. I.e. ask git given its .gitignore and .gitattributes what thing >matches one of its pathspecs instead of carrying your own bug-compatible >implementation. My impression is that it probably isn't practical to use something like that as ripgrep's main pattern-matching facility — for one thing there are performance concerns, and for another it makes it dependent on git for some of its core functionality (and it's not a git-specific tool). But i was going to say that it'd be useful for testing, and when i e-mailed Andrew (the main ripgrep dev) about this he said the same. I've CCed him on this message in case he has anything to add. dana
[RFC PATCH v2 4/7] merge-recursive: fix rename/add conflict handling
This makes the rename/add conflict handling make use of the new
handle_file_collision() function, which fixes several bugs and improves
things for the rename/add case significantly. Previously, rename/add
would:
* Not leave any higher order stage entries in the index, making it
appear as if there were no conflict.
* Would place the rename file at the colliding path, and move the
added file elsewhere, which combined with the lack of higher order
stage entries felt really odd. It's not clear to me why the
rename should take precedence over the add; if one should be moved
out of the way, they both probably should.
* In the recursive case, it would do a two way merge of the added
file and the version of the renamed file on the renamed side,
completely excluding modifications to the renamed file on the
unrenamed side of history.
Using the new handle_file_collision() fixes all of these issues, and
adds smarts to allow two-way merge OR move colliding files to separate
paths depending on the similarity of the colliding files.
Signed-off-by: Elijah Newren
---
merge-recursive.c| 137 +--
t/t6036-recursive-corner-cases.sh| 24 ++---
t/t6042-merge-rename-corner-cases.sh | 4 +-
3 files changed, 101 insertions(+), 64 deletions(-)
diff --git a/merge-recursive.c b/merge-recursive.c
index 24d979022e..0805095168 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -186,6 +186,7 @@ static int oid_eq(const struct object_id *a, const struct
object_id *b)
enum rename_type {
RENAME_NORMAL = 0,
RENAME_VIA_DIR,
+ RENAME_ADD,
RENAME_DELETE,
RENAME_ONE_FILE_TO_ONE,
RENAME_ONE_FILE_TO_TWO,
@@ -228,6 +229,7 @@ static inline void setup_rename_conflict_info(enum
rename_type rename_type,
struct stage_data *src_entry1,
struct stage_data *src_entry2)
{
+ int ostage1 = 0, ostage2;
struct rename_conflict_info *ci;
/*
@@ -264,18 +266,22 @@ static inline void setup_rename_conflict_info(enum
rename_type rename_type,
dst_entry2->rename_conflict_info = ci;
}
- if (rename_type == RENAME_TWO_FILES_TO_ONE) {
- /*
-* For each rename, there could have been
-* modifications on the side of history where that
-* file was not renamed.
-*/
- int ostage1 = o->branch1 == branch1 ? 3 : 2;
- int ostage2 = ostage1 ^ 1;
+ /*
+* For each rename, there could have been
+* modifications on the side of history where that
+* file was not renamed.
+*/
+ if (rename_type == RENAME_ADD ||
+ rename_type == RENAME_TWO_FILES_TO_ONE) {
+ ostage1 = o->branch1 == branch1 ? 3 : 2;
ci->ren1_other.path = pair1->one->path;
oidcpy(>ren1_other.oid, _entry1->stages[ostage1].oid);
ci->ren1_other.mode = src_entry1->stages[ostage1].mode;
+ }
+
+ if (rename_type == RENAME_TWO_FILES_TO_ONE) {
+ ostage2 = ostage1 ^ 1;
ci->ren2_other.path = pair2->one->path;
oidcpy(>ren2_other.oid, _entry2->stages[ostage2].oid);
@@ -1559,7 +1565,6 @@ static struct diff_filespec *filespec_from_entry(struct
diff_filespec *target,
return target;
}
-#if 0 // #if-0-ing avoids unused function warning; will make live in next
commit
static int handle_file_collision(struct merge_options *o,
const char *collide_path,
const char *prev_path1,
@@ -1575,6 +1580,20 @@ static int handle_file_collision(struct merge_options *o,
char *alt_path = NULL;
const char *update_path = collide_path;
+ /*
+* It's easiest to get the correct things into stage 2 and 3, and
+* to make sure that the content merge puts HEAD before the other
+* branch if we just ensure that branch1 == o->branch1. So, simply
+* flip arguments around if we don't have that.
+*/
+ if (branch1 != o->branch1) {
+ return handle_file_collision(o, collide_path,
+prev_path2, prev_path1,
+branch2, branch1,
+b_oid, b_mode,
+a_oid, a_mode);
+ }
+
/*
* In the recursive case, we just opt to undo renames
*/
@@ -1678,7 +1697,38 @@ static int handle_file_collision(struct merge_options *o,
*/
return mfi.clean;
}
-#endif
+
+static int handle_rename_add(struct merge_options *o,
+struct rename_conflict_info *ci)
+{
+ /* a was renamed to c, and a separate c was added. */
+ struct
[RFC PATCH v2 1/7] Add testcases for consistency in file collision conflict handling
Add testcases dealing with file collisions for the following types of
conflicts:
* add/add
* rename/add
* rename/rename(2to1)
All these conflict types simplify down to two files "colliding"
and should thus be handled similarly. This means that rename/add and
rename/rename(2to1) conflicts need to be modified to behave the same as
add/add conflicts currently do: the colliding files should be two-way
merged (instead of the current behavior of writing the two colliding
files out to separate temporary unique pathnames). Add testcases which
check this; subsequent commits will fix the conflict handling to make
these tests pass.
Signed-off-by: Elijah Newren
---
t/t6042-merge-rename-corner-cases.sh | 162 +++
1 file changed, 162 insertions(+)
diff --git a/t/t6042-merge-rename-corner-cases.sh
b/t/t6042-merge-rename-corner-cases.sh
index b97aca7fa2..b6fed2cb9a 100755
--- a/t/t6042-merge-rename-corner-cases.sh
+++ b/t/t6042-merge-rename-corner-cases.sh
@@ -937,4 +937,166 @@ test_expect_failure 'mod6-check: chains of
rename/rename(1to2) and rename/rename
)
'
+test_conflicts_with_adds_and_renames() {
+ sideL=$1
+ sideR=$2
+ expect=$3
+
+ # Setup:
+ # L
+ # / \
+ # master ?
+ # \ /
+ # R
+ #
+ # Where:
+ # Both L and R have files named 'three' which collide. Each of
+ # the colliding files could have been involved in a rename, in
+ # which case there was a file named 'one' or 'two' that was
+ # modified on the opposite side of history and renamed into the
+ # collision on this side of history.
+ #
+ # Questions:
+ # 1) The index should contain both a stage 2 and stage 3 entry
+ # for the colliding file. Does it?
+ # 2) When renames are involved, the content merges are clean, so
+ # the index should reflect the content merges, not merely the
+ # version of the colliding file from the prior commit. Does
+ # it?
+ # 3) There should be a file in the worktree named 'three'
+ # containing the two-way merged contents of the content-merged
+ # versions of 'three' from each of the two colliding
+ # files. Is it present?
+ # 4) There should not be any three~* files in the working
+ # tree
+ test_expect_success "setup simple $sideL/$sideR conflict" '
+ test_create_repo simple_${sideL}_${sideR} &&
+ (
+ cd simple_${sideL}_${sideR} &&
+
+ # Create some related files now
+ for i in $(test_seq 1 10)
+ do
+ echo Random base content line $i
+ done >file_v1 &&
+ cp file_v1 file_v2 &&
+ echo modification >>file_v2 &&
+
+ cp file_v1 file_v3 &&
+ echo more stuff >>file_v3 &&
+ cp file_v3 file_v4 &&
+ echo yet more stuff >>file_v4 &&
+
+ # Use a tag to record both these files for simple
+ # access, and clean out these untracked files
+ git tag file_v1 $(git hash-object -w file_v1) &&
+ git tag file_v2 $(git hash-object -w file_v2) &&
+ git tag file_v3 $(git hash-object -w file_v3) &&
+ git tag file_v4 $(git hash-object -w file_v4) &&
+ git clean -f &&
+
+ # Setup original commit (or merge-base), consisting of
+ # files named "one" and "two" if renames were involved.
+ touch irrelevant_file &&
+ git add irrelevant_file &&
+ if [ $sideL = "rename" ]
+ then
+ git show file_v1 >one &&
+ git add one
+ fi &&
+ if [ $sideR = "rename" ]
+ then
+ git show file_v3 >two &&
+ git add two
+ fi &&
+ test_tick && git commit -m initial &&
+
+ git branch L &&
+ git branch R &&
+
+ # Handle the left side
+ git checkout L &&
+ if [ $sideL = "rename" ]
+ then
+ git mv one three
+ else
+ git show file_v2 >three &&
+ git add three
+ fi &&
+ if [ $sideR = "rename" ]
+ then
+ git show file_v4 >two &&
+
[RFC PATCH v2 3/7] merge-recursive: new function for better colliding conflict resolutions
There are three conflict types that represent two (possibly entirely unrelated) files colliding at the same location: * add/add * rename/add * rename/rename(2to1) These three conflict types already share more similarity than might be immediately apparent from their description: (1) the handling of the rename variants already involves removing any entries from the index corresponding to the original file names[*], thus only leaving entries in the index for the colliding path; (2) likewise, any trace of the original file name in the working tree is also removed. So, in all three cases we're left with how to represent two colliding files in both the index and the working copy. [*] Technically, this isn't quite true because rename/rename(2to1) conflicts in the recursive (o->call_depth > 0) case do an "unrename" since about seven years ago. But even in that case, Junio felt compelled to explain that my decision to "unrename" wasn't necessarily the only or right answer -- search for "Comment from Junio" in t6036 for details. My initial motivation for looking at these three conflict types was that if the handling of these three conflict types is the same, at least in the limited set of cases where a renamed file is unmodified on the side of history where the file is not renamed, then a significant performance improvement for rename detection during merges is possible. However, while that served as motivation to look at these three types of conflicts, the actual goal of this new function is to try to improve the handling for all three cases, not to merely make them the same as each other in that special circumstance. === Handling the working tree === The previous behavior for these conflict types in regards to the working tree (assuming the file collision occurs at 'foo') was: * add/add does a two-way merge of the two files and records it as 'foo'. * rename/rename(2to1) records the two different files into two new uniquely named files (foo~HEAD and foo~$MERGE), while removing 'foo' from the working tree. * rename/add records the two different files into two different locations, recording the add at foo~$SIDE and, oddly, recording the rename at foo (why is the rename more important than the add?) So, the question for what to write to the working tree boils down to whether the two colliding files should be two-way merged and recorded in place, or recorded into separate files. As per discussion on the git mailing lit, two-way merging was deemed to always be preferred, as that makes these cases all more like content conflicts that users can handle from within their favorite editor, IDE, or merge tool. Note that since renames already involve a content merge, rename/add and rename/rename(2to1) conflicts could result in nested conflict markers. === Handling of the index === For a typical rename, unpack_trees() would set up the index in the following fashion: old_path new_path stage1: 5ca1ab1e stage2: f005ba11 stage3: b0a710ad And merge-recursive would rewrite this to new_path stage1: 5ca1ab1e stage2: f005ba11 stage3: b0a710ad Removing old_path from the index means the user won't have to `git rm old_path` manually every time a renamed path has a content conflict. It also means they can use `git checkout [--ours|--theirs|--conflict|-m] new_path`, `git diff [--ours|--theirs]` and various other commands that would be difficult otherwise. This strategy becomes a problem when we have a rename/add or rename/rename(2to1) conflict, however, because then we have only three slots to store blob sha1s and we need either four or six. Previously, this was handled by continuing to delete old_path from the index, and just outright ignoring any blob shas from old_path. That had the downside of deleting any trace of changes made to old_path on the other side of history. This function instead does a three-way content merge of the renamed file, and stores the blob sha1 for that at either stage2 or stage3 for new_path (depending on which side the rename came from). That has the advantage of bringing information about changes on both sides and still allows for easy resolution (no need to git rm old_path, etc.), but does have the downside that if the content merge had conflict markers, then what we store in the index is the sha1 of a blob with conflict markers. While that is a downside, it seems less problematic than the downsides of any obvious alternatives, and certainly makes more sense than the previous handling. Further, it has a precedent in that when we do recursive merges, we may accept a file with conflict markers as the resolution for the merge of the merge-bases, which will then show up in the index of the outer merge at stage 1 if a conflict exists at the outer level. Signed-off-by: Elijah Newren --- merge-recursive.c | 121 ++ 1 file changed, 121 insertions(+) diff
[RFC PATCH v2 0/7] Improve path collision conflict resolutions
This RFC series depends on this series submitted yesterday: https://public-inbox.org/git/20181012212551.7689-1-new...@gmail.com/ (which in turn depends on en/merge-cleanup in next). Although this is an "update" to my previous RFC series from six months ago: https://public-inbox.org/git/20180305171125.22331-1-new...@gmail.com/ It's essentially a complete rewrite to instead use the strategy proposed by Jonathan and Junio (from a separate thread discussing that RFC series): https://public-inbox.org/git/20180312213521.gb58...@aiede.svl.corp.google.com/ https://public-inbox.org/git/capc5davu8vv9rdgon8jixeo3ycdvqq38yszzc-cpo+aqcak...@mail.gmail.com The basic idea is to make the "file collision" conflict types all behave like add/add. These types are: * add/add * rename/add * rename/rename(2to1) * each rename/add piece of a rename/rename(1to2)/add[/add] conflict Specific bits that make this RFC: * Depends on another series not even in pu yet. * In order to simplify review, I add a new common function for all these conflict types to call in one commit, and since the function is not yet used at that point, I wrap the function in an #ifdef 0. In later patches, I remove the #ifdef. Is that...okay? * There are a few FIXMEs in the code. I'm okay leaving them there, but are others upset by them? * Every time I attempt to complete this series, I spot more problems; many of the merge-recursive series I've submitted since March were issues I discovered while trying to complete this series -- or secondary tangents discovered while working on the side-issues to this one. I've gotten to the point of automatically second-guessing myself on whether I could have actually completed the necessary fixes. Elijah Newren (7): Add testcases for consistency in file collision conflict handling t6036, t6042: testcases for rename collision of already conflicting files merge-recursive: new function for better colliding conflict resolutions merge-recursive: fix rename/add conflict handling merge-recursive: improve handling for rename/rename(2to1) conflicts merge-recursive: use handle_file_collision for add/add conflicts merge-recursive: improve rename/rename(1to2)/add[/add] handling merge-recursive.c| 513 --- t/t6036-recursive-corner-cases.sh| 228 +++- t/t6042-merge-rename-corner-cases.sh | 333 - t/t6043-merge-rename-directories.sh | 107 +++--- 4 files changed, 893 insertions(+), 288 deletions(-) -- 2.19.0.3.g98f21ceff2.dirty
[RFC PATCH v2 5/7] merge-recursive: improve handling for rename/rename(2to1) conflicts
This makes the rename/rename(2to1) conflicts use the new
handle_file_collision() function. Since that function was based
originally on the rename/rename(2to1) handling code, the main
differences here are in what was added. In particular:
* Instead of storing files at collide_path~HEAD and collide_path~MERGE,
the files are two-way merged and recorded at collide_path.
* Instead of recording the version of the renamed file that existed
on the renamed side in the index (thus ignoring any changes that
were made to the file on the side of history without the rename),
we do a three-way content merge on the renamed path, then store
that at either stage 2 or stage 3.
* Note that since the content merge for each rename may have conflicts,
and then we have to merge the two renamed files, we can end up with
nested conflict markers.
Signed-off-by: Elijah Newren
---
merge-recursive.c| 104 ---
t/t6036-recursive-corner-cases.sh| 12 +---
t/t6042-merge-rename-corner-cases.sh | 38 ++
t/t6043-merge-rename-directories.sh | 83 -
4 files changed, 89 insertions(+), 148 deletions(-)
diff --git a/merge-recursive.c b/merge-recursive.c
index 0805095168..ead6054a75 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -696,27 +696,6 @@ static int update_stages(struct merge_options *opt, const
char *path,
return 0;
}
-static int update_stages_for_stage_data(struct merge_options *opt,
- const char *path,
- const struct stage_data *stage_data)
-{
- struct diff_filespec o, a, b;
-
- o.mode = stage_data->stages[1].mode;
- oidcpy(, _data->stages[1].oid);
-
- a.mode = stage_data->stages[2].mode;
- oidcpy(, _data->stages[2].oid);
-
- b.mode = stage_data->stages[3].mode;
- oidcpy(, _data->stages[3].oid);
-
- return update_stages(opt, path,
-is_null_oid() ? NULL : ,
-is_null_oid() ? NULL : ,
-is_null_oid() ? NULL : );
-}
-
static void update_entry(struct stage_data *entry,
struct diff_filespec *o,
struct diff_filespec *a,
@@ -1870,7 +1849,6 @@ static int handle_rename_rename_2to1(struct merge_options
*o,
char *path_side_2_desc;
struct merge_file_info mfi_c1;
struct merge_file_info mfi_c2;
- int ret;
output(o, 1, _("CONFLICT (rename/rename): "
"Rename %s->%s in %s. "
@@ -1878,81 +1856,22 @@ static int handle_rename_rename_2to1(struct
merge_options *o,
a->path, c1->path, ci->branch1,
b->path, c2->path, ci->branch2);
- remove_file(o, 1, a->path, o->call_depth ||
would_lose_untracked(a->path));
- remove_file(o, 1, b->path, o->call_depth ||
would_lose_untracked(b->path));
-
path_side_1_desc = xstrfmt("version of %s from %s", path, a->path);
path_side_2_desc = xstrfmt("version of %s from %s", path, b->path);
if (merge_mode_and_contents(o, a, c1, >ren1_other, path_side_1_desc,
o->branch1, o->branch2,
- o->call_depth * 2, _c1) ||
+ 1 + o->call_depth * 2, _c1) ||
merge_mode_and_contents(o, b, >ren2_other, c2, path_side_2_desc,
o->branch1, o->branch2,
- o->call_depth * 2, _c2))
+ 1 + o->call_depth * 2, _c2))
return -1;
free(path_side_1_desc);
free(path_side_2_desc);
- if (o->call_depth) {
- /*
-* If mfi_c1.clean && mfi_c2.clean, then it might make
-* sense to do a two-way merge of those results. But, I
-* think in all cases, it makes sense to have the virtual
-* merge base just undo the renames; they can be detected
-* again later for the non-recursive merge.
-*/
- remove_file(o, 0, path, 0);
- ret = update_file(o, 0, _c1.oid, mfi_c1.mode, a->path);
- if (!ret)
- ret = update_file(o, 0, _c2.oid, mfi_c2.mode,
- b->path);
- } else {
- char *new_path1 = unique_path(o, path, ci->branch1);
- 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);
- if (was_dirty(o, path))
- output(o, 1, _("Refusing to lose dirty file at %s"),
- path);
- else if (would_lose_untracked(path))
- /*
-* Only
[RFC PATCH v2 2/7] t6036, t6042: testcases for rename collision of already conflicting files
When a single file is renamed, it can also be modified, yielding the possibility of that renamed file having content conflicts. If two different such files are renamed into the same location, then two-way merging those files may result in nested conflicts. Add a testcase that makes sure we get this case correct, and uses different lengths of conflict markers to differentiate between the different nestings. Also add another case with an extra (i.e. third) level of conflict markers due to using merge.conflictstyle=diff3 and the virtual merge base also having conflicts present. Signed-off-by: Elijah Newren --- t/t6036-recursive-corner-cases.sh| 194 +++ t/t6042-merge-rename-corner-cases.sh | 118 2 files changed, 312 insertions(+) diff --git a/t/t6036-recursive-corner-cases.sh b/t/t6036-recursive-corner-cases.sh index 276b4e8792..a4c8041448 100755 --- a/t/t6036-recursive-corner-cases.sh +++ b/t/t6036-recursive-corner-cases.sh @@ -1553,4 +1553,198 @@ test_expect_success "check virtual merge base with nested conflicts" ' ) ' +# Setup: +# L1---L2 +# / \ / \ +# masterX? +# \ / \ / +# R1---R2 +# +# Where: +# master has two files, named 'b' and 'a' +# branches L1 and R1 both modify each of the two files in conflicting ways +# +# L2 is a merge of R1 into L1; more on it later. +# R2 is a merge of L1 into R1; more on it later. +# +# X is an auto-generated merge-base used when merging L2 and R2. +# since X is a merge of L1 and R1, it has conflicting versions of each file +# +# More about L2 and R2: +# - both resolve the conflicts in 'b' and 'a' differently +# - L2 renames 'b' to 'm' +# - R2 renames 'a' to 'm' +# +# In the end, in file 'm' we have four different conflicting files (from +# two versions of 'b' and two of 'a'). In addition, if +# merge.conflictstyle is diff3, then the base version also has +# conflict markers of its own, leading to a total of three levels of +# conflict markers. This is a pretty weird corner case, but we just want +# to ensure that we handle it as well as practical. + +test_expect_success "setup nested conflicts" ' + test_create_repo nested_conflicts && + ( + cd nested_conflicts && + + # Create some related files now + for i in $(test_seq 1 10) + do + echo Random base content line $i + done >initial && + + cp initial b_L1 && + cp initial b_R1 && + cp initial b_L2 && + cp initial b_R2 && + cp initial a_L1 && + cp initial a_R1 && + cp initial a_L2 && + cp initial a_R2 && + + test_write_lines b b_L1 >>b_L1 && + test_write_lines b b_R1 >>b_R1 && + test_write_lines b b_L2 >>b_L2 && + test_write_lines b b_R2 >>b_R2 && + test_write_lines a a_L1 >>a_L1 && + test_write_lines a a_R1 >>a_R1 && + test_write_lines a a_L2 >>a_L2 && + test_write_lines a a_R2 >>a_R2 && + + # Setup original commit (or merge-base), consisting of + # files named "b" and "a" + cp initial b && + cp initial a && + echo b >>b && + echo a >>a && + git add b a && + test_tick && git commit -m initial && + + git branch L && + git branch R && + + # Handle the left side + git checkout L && + mv -f b_L1 b && + mv -f a_L1 a && + git add b a && + test_tick && git commit -m "version L1 of files" && + git tag L1 && + + # Handle the right side + git checkout R && + mv -f b_R1 b && + mv -f a_R1 a && + git add b a && + test_tick && git commit -m "verson R1 of files" && + git tag R1 && + + # Create first merge on left side + git checkout L && + test_must_fail git merge R1 && + mv -f b_L2 b && + mv -f a_L2 a && + git add b a && + git mv b m && + test_tick && git commit -m "left merge, rename b->m" && + git tag L2 && + + # Create first merge on right side + git checkout R && + test_must_fail git merge L1 && + mv -f b_R2 b && + mv -f a_R2 a && + git add b a && + git mv a m && + test_tick && git commit -m "right merge, rename a->m" && + git tag R2 + ) +' + +test_expect_failure "check nested conflicts" ' + ( + cd nested_conflicts && + + git clean -f && +
[RFC PATCH v2 7/7] merge-recursive: improve rename/rename(1to2)/add[/add] handling
When we have a rename/rename(1to2) conflict, each of the renames can
collide with a file addition. Each of these rename/add conflicts suffered
from the same kinds of problems that normal rename/add suffered from.
Make the code use handle_file_conflicts() as well so that we get all the
same fixes and consistent behavior between the different conflict types.
Signed-off-by: Elijah Newren
---
merge-recursive.c| 154 +--
t/t6042-merge-rename-corner-cases.sh | 29 +++--
t/t6043-merge-rename-directories.sh | 24 +++--
3 files changed, 113 insertions(+), 94 deletions(-)
diff --git a/merge-recursive.c b/merge-recursive.c
index c78b347112..5986b6 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -1709,80 +1709,17 @@ static int handle_rename_add(struct merge_options *o,
ci->dst_entry1->stages[other_stage].mode);
}
-static int handle_file(struct merge_options *o,
- struct diff_filespec *rename,
- int stage,
- struct rename_conflict_info *ci)
-{
- char *dst_name = rename->path;
- struct stage_data *dst_entry;
- const char *cur_branch, *other_branch;
- struct diff_filespec other;
- struct diff_filespec *add;
- int ret;
-
- if (stage == 2) {
- dst_entry = ci->dst_entry1;
- cur_branch = ci->branch1;
- other_branch = ci->branch2;
- } else {
- dst_entry = ci->dst_entry2;
- cur_branch = ci->branch2;
- other_branch = ci->branch1;
- }
-
- add = filespec_from_entry(, dst_entry, stage ^ 1);
- if (add) {
- int ren_src_was_dirty = was_dirty(o, rename->path);
- char *add_name = unique_path(o, rename->path, other_branch);
- if (update_file(o, 0, >oid, add->mode, add_name))
- return -1;
-
- if (ren_src_was_dirty) {
- output(o, 1, _("Refusing to lose dirty file at %s"),
- rename->path);
- }
- /*
-* Because the double negatives somehow keep confusing me...
-*1) update_wd iff !ren_src_was_dirty.
-*2) no_wd iff !update_wd
-*3) so, no_wd == !!ren_src_was_dirty == ren_src_was_dirty
-*/
- remove_file(o, 0, rename->path, ren_src_was_dirty);
- dst_name = unique_path(o, rename->path, cur_branch);
- } else {
- if (dir_in_way(rename->path, !o->call_depth, 0)) {
- 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);
- } else if (!o->call_depth &&
- would_lose_untracked(rename->path)) {
- dst_name = unique_path(o, rename->path, cur_branch);
- output(o, 1, _("Refusing to lose untracked file at %s; "
- "adding as %s instead"),
- rename->path, dst_name);
- }
- }
- if ((ret = update_file(o, 0, >oid, rename->mode, dst_name)))
- ; /* fall through, do allow dst_name to be released */
- else if (stage == 2)
- ret = update_stages(o, rename->path, NULL, rename, add);
- else
- ret = update_stages(o, rename->path, NULL, add, rename);
-
- if (dst_name != rename->path)
- free(dst_name);
-
- return ret;
-}
-
static int handle_rename_rename_1to2(struct merge_options *o,
struct rename_conflict_info *ci)
{
/* One file was renamed in both branches, but to different names. */
+ struct merge_file_info mfi;
+ struct diff_filespec other;
+ struct diff_filespec *add;
struct diff_filespec *one = ci->pair1->one;
struct diff_filespec *a = ci->pair1->two;
struct diff_filespec *b = ci->pair2->two;
+ char *path_desc;
output(o, 1, _("CONFLICT (rename/rename): "
"Rename \"%s\"->\"%s\" in branch \"%s\" "
@@ -1790,15 +1727,16 @@ static int handle_rename_rename_1to2(struct
merge_options *o,
one->path, a->path, ci->branch1,
one->path, b->path, ci->branch2,
o->call_depth ? _(" (left unresolved)") : "");
- if (o->call_depth) {
- struct merge_file_info mfi;
- struct diff_filespec other;
- struct diff_filespec *add;
- if (merge_mode_and_contents(o, one, a, b, one->path,
- ci->branch1, ci->branch2,
- o->call_depth * 2, ))
-
[RFC PATCH v2 6/7] merge-recursive: use handle_file_collision for add/add conflicts
This results in no-net change of behavior, it simply ensures that all
file-collision conflict handling types are being handled the same by
calling the same function.
Signed-off-by: Elijah Newren
---
merge-recursive.c | 29 +
1 file changed, 21 insertions(+), 8 deletions(-)
diff --git a/merge-recursive.c b/merge-recursive.c
index ead6054a75..c78b347112 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -3355,14 +3355,27 @@ static int process_entry(struct merge_options *o,
clean_merge = -1;
}
} else if (a_oid && b_oid) {
- /* Case C: Added in both (check for same permissions) and */
- /* case D: Modified in both, but differently. */
- int is_dirty = 0; /* unpack_trees would have bailed if dirty */
- clean_merge = handle_content_merge(o, path, is_dirty,
- o_oid, o_mode,
- a_oid, a_mode,
- b_oid, b_mode,
- NULL);
+ if (!o_oid) {
+ /* Case C: Added in both (check for same permissions) */
+ output(o, 1,
+ _("CONFLICT (add/add): Merge conflict in %s"),
+ path);
+ clean_merge = handle_file_collision(o,
+ path, NULL, NULL,
+ o->branch1,
+ o->branch2,
+ a_oid, a_mode,
+ b_oid, b_mode);
+ } else {
+ /* case D: Modified in both, but differently. */
+ int is_dirty = 0; /* unpack_trees would have bailed if
dirty */
+ clean_merge = handle_content_merge(o, path,
+ is_dirty,
+ o_oid, o_mode,
+ a_oid, a_mode,
+ b_oid, b_mode,
+ NULL);
+ }
} else if (!o_oid && !a_oid && !b_oid) {
/*
* this entry was deleted altogether. a_mode == 0 means
--
2.19.0.3.g98f21ceff2.dirty
Re: bug when combined with etckeeper
Ok, my bad. I had a global pre-commit hook which had a lingering etckeeper command for another repository. Not quite sure why it only runs when commit has the '-a' option... Thanks for pointing to the hooks possibility. Naja Melan
Re: [PATCH] zlib.c: use size_t for size
Am 13.10.18 um 04:46 schrieb Jeff King: But no, right before that we have this line: offset -= win->offset; So offset is in fact no longer its original meaning of "offset into the packfile", but is now an offset of the specific request into the window we found. So I think it's correct, but it sure confused me. I wonder if another variable might help, like: size_t offset_in_window; ... /* * We know this difference will fit in a size_t, because our mmap * window by * definition can be no larger than a size_t. */ offset_in_window = xsize_t(offset - win->offset); if (left) *left = win->len - offset_in_window; return win->base + offset_in_window; I dunno. Maybe it is overkill. Thank you for your analysis. No, I don't think that such a new variable would be overkill. It is important to make such knowledge of value magnitudes explicit precisely because it reduces confusion and helps reviewers of the code verify correctness. -- Hannes
Re: issue: strange `git diff --numstat` behavior.
Sergey Andreenko writes:
> git.exe diff --numstat "C:\diff" "C:\base"
> ...
> output will be:
>
> 0 1 {iff => ase}/1.txt
>
> So (folder_name_length) symbols were cut from the path (“C:\\d” and “C:\\b”).
>
> Is any way to avoid that? I have checked several git versions and they
> all do the same.
Not an attempt to offer a solution (I don't do windows), but just
trying to see what random things we can try, I wonder what you'd get
if you did something like
$ git diff --numstat //c/diff //c/base
[Question] builtin/branch.c
Hi, I am learning `builtin/branch.c`. I find that it will call `branch_get`
before create and [un]set upstream, and die with "no such branch" if failed.
but `branch_get` seems never fail, it is a get_or_create. Also, it was
confused that getting a branch before it has created.
builtin/branch.c #811
} else if (argc > 0 && argc <= 2) {
struct branch *branch = branch_get(argv[0]);
if (!branch)
die(_("no such branch '%s'"), argv[0]);
if (filter.kind != FILTER_REFS_BRANCHES)
die(_("-a and -r options to 'git branch' do not make sense with a
branch name"));
if (track == BRANCH_TRACK_OVERRIDE)
die(_("the '--set-upstream' option is no longer supported. Please
use '--track' or '--set-upstream-to' instead."));
create_branch(argv[0], (argc == 2) ? argv[1] : head,
force, 0, reflog, quiet, track);
How do smart git helpers transport objects?
Hello, I'm writing a remote helper and decided implement the `fetch` and `push` capabilities, but one thing still remains a mystery for me. Namely, is my helper supposed to instantiate and pin git objects in `GIT_DIR` upon `fetch `? In general I'm trying to determine how exactly I need to proceed upon receiving this command and find out whether libgit2 will be able to help me with whatever that is. I'm also contemplating hooking up to libgit2's transport API, but IIUC its operation isn't very well suited for the backend I'm trying to use behind the scenes (IPFS,https://ipfs.io/, long story short it's P2P and doesn't have a "server side" per se). Thanks, Stan PS: If my libgit2 questions feel outside scope I'd gladly join a community more focused on libgit2; I'm already on #libgit2 on freenode, but there's only 11 people there ATM
Re: [PATCH v3 4/7] revision.c: begin refactoring --topo-order logic
Johannes Sixt writes:
> Am 12.10.18 um 08:33 schrieb Junio C Hamano:
>> "Derrick Stolee via GitGitGadget" writes:
>>> +struct topo_walk_info {};
>>> +
>>> +static void init_topo_walk(struct rev_info *revs)
>>> +{
>>> + struct topo_walk_info *info;
>>> + revs->topo_walk_info = xmalloc(sizeof(struct topo_walk_info));
>>> + info = revs->topo_walk_info;
>>> + memset(info, 0, sizeof(struct topo_walk_info));
>>
>> There is no member in the struct at this point. Are we sure this is
>> safe? Just being curious.
>
> sizeof cannot return 0. sizeof(struct topo_walk_info) will be 1 here.
Thanks.
Re: Bug: manpage for `git ls-files` uses instead of
Lily Ballard writes: > I haven’t checked any other commands that the glossary lists as > taking pathspecs (except `git add`, which does properly list it as > taking pathspecs), so I don’t know if any of the other commands > incorrectly list themselves as taking files when they take > pathspecs. I do not offhand think of a command that only takes pathname, including "ls-files". New users would think "git cmd README" is taking a single filename "README", and "ls-files" doc uses "file" in a misguided attempt to be "newbie friendly", but it always has used that "README" string as a pattern to match paths against.
Re: [PATCH 1/1] protocol: limit max protocol version per service
Jonathan Nieder writes: > Josh Steadmon wrote: >> On 2018.10.12 16:32, Jonathan Nieder wrote: >>> Josh Steadmon wrote: > For now, I'm going to try adding an --allowed_versions flag for the remote helpers, but if anyone has a better idea, let me know. >>> >>> I forgot to mention: the stateless-connect remote helper capability is >>> still experimental so you're free to change its interface (e.g. to >>> change the syntax of the stateless-connect command it provides). >> >> For v2 of this patch, I ended up using GIT_PROTOCOL to pass the version >> string to the remote helpers. > > Makes sense. Thanks. :) Yeah, it does.
Re: [PATCH] gc: remove redundant check for gc_auto_threshold
Ævar Arnfjörð Bjarmason writes: > Yeah, that's me :) I have some WIP gc cleanup, but want to sit on it a > bit before I submit it to think about the best way to do things. > > So in the meantime I was sending out a few WIP bits that I expected > could be reviewed stand-alone. I dunno. Unless the real body of the changes that "depend" on this small change comes before people forget the connection between them, I think it is detrimental to churn the codebase like this. If the real body of the changes do not conflict with other topics in flight when it materializes, then having this small clean-up as a preparatory step in that real series would cost us nothing---that clean-up would not conflict with other things either. If the real thing would conflict and need to be adjusted to play well with other topics before submission, having this small clean-up as a preparatory step in that real series would cost us nothing, either.
URGENT RESPONSE NEEDED
Hello my dear. Did you receive my email message to you? Please, get back to me ASAP as the matter is becoming late. Expecting your urgent response. Sean.
