From: Michael Haggerty <hidden> Date: 2016-06-15 23:08:54
Patch 04/33 in David Turner's refs-backend-lmdb series v7 [1] did way
too much in a single patch, and in fact got a few minor things wrong.
Instead of that patch, I suggest this patch series, which
* Splits the changes into smaller steps.
* Adds a bunch of tests of deleting references with invalid but safe
names, including symbolic references and including references
reached via symbolic references. Two of these tests fail when run
against David's patch 04 due to changes in output.
* Arranges for the "flags" argument to read_raw_ref() always to be
non-NULL, which eliminates the need for a lot of "if (flags)"
guards.
* Eliminates the now-superfluous "bad_name" local variable.
* Move the management of the scratch space sb_path from
resolve_ref_unsafe() to read_raw_ref().
* Inlines resolve_ref_1() into resolve_ref_unsafe().
* Changes some callers of resolve_ref_unsafe() to pass flags=NULL
instead of creating a local flags variable that is never used.
* Changes some callers to check for errors before using the return
value of resolve_ref_unsafe().
I hope that the result is easier to understand and audit, even though
it consists of more patches (indeed, *because* of that).
This patch series applies on top of David's patch 03/33 the same place
David applied it in his repo [2]. It is also available in situ from my
GitHub repo [3] as branch "pluggable-backends-patch4"
If this series is used, later patches from David's series would need
to be rebased on top of it. This is a little bit messy but not
difficult; the result can be seen in branch
"pluggable-backends-rebased" in my GitHub repo [3] (albeit without
adjusting the LMDB-related patches).
Michael
[1] http://article.gmane.org/gmane.comp.version-control.git/287971
[2] https://github.com/dturner-tw/git/tree/dturner/pluggable-backends
[3] https://github.com/mhagger/git
David Turner (1):
files-backend: break out ref reading
Michael Haggerty (20):
t1430: test the output and error of some commands more carefully
t1430: clean up broken refs/tags/shadow
t1430: don't rely on symbolic-ref for creating broken symrefs
t1430: test for-each-ref in the presence of badly-named refs
t1430: improve test coverage of deletion of badly-named refs
resolve_missing_loose_ref(): simplify semantics
resolve_ref_unsafe(): use for loop to count up to MAXDEPTH
resolve_ref_unsafe(): ensure flags is always set
resolve_ref_1(): eliminate local variable
resolve_ref_1(): reorder code
resolve_ref_1(): eliminate local variable "bad_name"
read_raw_ref(): manage own scratch space
Inline resolve_ref_1() into resolve_ref_unsafe()
read_raw_ref(): change flags parameter to unsigned int
fsck_head_link(): remove unneeded flag variable
cmd_merge(): remove unneeded flag variable
get_default_remote(): remove unneeded flag variable
checkout_paths(): remove unneeded flag variable
check_aliased_update(): check that dst_name is non-NULL
show_head_ref(): check the result of resolve_ref_namespace()
builtin/checkout.c | 3 +-
builtin/fsck.c | 3 +-
builtin/merge.c | 4 +-
builtin/receive-pack.c | 2 +-
builtin/submodule--helper.c | 3 +-
http-backend.c | 4 +-
refs/files-backend.c | 341 ++++++++++++++++++++++++--------------------
t/t1430-bad-ref-name.sh | 132 +++++++++++++++--
8 files changed, 312 insertions(+), 180 deletions(-)
--
2.8.0.rc3
From: Michael Haggerty <hidden> Date: 2016-06-15 23:08:54
From: David Turner <redacted>
Refactor resolve_ref_1 in terms of a new function read_raw_ref, which
is responsible for reading ref data from the ref storage.
Later, we will make read_raw_ref a pluggable backend function, and make
resolve_ref_unsafe common.
Signed-off-by: David Turner <redacted>
Helped-by: Duy Nguyen [off-list ref]
Signed-off-by: Junio C Hamano <redacted>
Signed-off-by: Michael Haggerty <redacted>
---
refs/files-backend.c | 244 ++++++++++++++++++++++++++++++---------------------
1 file changed, 145 insertions(+), 99 deletions(-)
@@ -1390,6 +1390,141 @@ static int resolve_missing_loose_ref(const char *refname,return-1;}+/*+*Readarawreffromthefilesystemorpackedrefsfile.+*+*Iftherefisasha1,fillinsha1andreturn0.+*+*Iftherefissymbolic,fillin*symrefwiththereferrent+*(e.g."refs/heads/master")andreturn0.Thecallerisresponsible+*forvalidatingthereferrent.SetREF_ISSYMREFinflags.+*+*Iftherefdoesn'texist,seterrnotoENOENTandreturn-1.+*+*Iftherefexistsbutisneitherasymbolicrefnorasha1,itis+*broken.SetREF_ISBROKENinflags,seterrnotoEINVAL,andreturn+*-1.+*+*Ifthereisanothererrorreadingtheref,seterrnoappropriatelyand+*return-1.+*+*Backend-specificflagsmightbesetinflagsaswell,regardlessof+*outcome.+*+*sb_pathisworkspace:thecallershouldallocateandfreeit.+*+*ItisOKforrefnametopointintosymref.Inthiscase:+*-ifthefunctionsucceedswithREF_ISSYMREF,symrefwillbe+*overwrittenandthememorypointedtobyrefnamemightbechanged+*orevenfreed.+*-inallothercases,symrefwillbeuntouched,andtherefore+*refnamewillstillbevalidandunchanged.+*/+staticintread_raw_ref(constchar*refname,unsignedchar*sha1,+structstrbuf*symref,structstrbuf*sb_path,+structstrbuf*sb_contents,int*flags)+{+constchar*path;+constchar*buf;+structstatst;+intfd;++strbuf_reset(sb_path);+strbuf_git_path(sb_path,"%s",refname);+path=sb_path->buf;++stat_ref:+/*+*Wemighthavetoloopbackheretoavoidarace+*condition:firstwelstat()thefile,thenwetry+*toreaditasalinkorasafile.Butifsomebody+*changesthetypeofthefile(file<->directory+*<->symlink)betweenthelstat()andreading,then+*wedon'twanttoreportthatasanerrorbutrather+*tryagainstartingwiththelstat().+*/++if(lstat(path,&st)<0){+if(errno!=ENOENT)+return-1;+if(resolve_missing_loose_ref(refname,sha1,flags)){+errno=ENOENT;+return-1;+}+return0;+}++/* Follow "normalized" - ie "refs/.." symlinks by hand */+if(S_ISLNK(st.st_mode)){+strbuf_reset(sb_contents);+if(strbuf_readlink(sb_contents,path,0)<0){+if(errno==ENOENT||errno==EINVAL)+/* inconsistent with lstat; retry */+gotostat_ref;+else+return-1;+}+if(starts_with(sb_contents->buf,"refs/")&&+!check_refname_format(sb_contents->buf,0)){+strbuf_swap(sb_contents,symref);+*flags|=REF_ISSYMREF;+return0;+}+}++/* Is it a directory? */+if(S_ISDIR(st.st_mode)){+errno=EISDIR;+return-1;+}++/*+*Anythingelse,justopenitandtrytouseitas+*aref+*/+fd=open(path,O_RDONLY);+if(fd<0){+if(errno==ENOENT)+/* inconsistent with lstat; retry */+gotostat_ref;+else+return-1;+}+strbuf_reset(sb_contents);+if(strbuf_read(sb_contents,fd,256)<0){+intsave_errno=errno;+close(fd);+errno=save_errno;+return-1;+}+close(fd);+strbuf_rtrim(sb_contents);+buf=sb_contents->buf;+if(starts_with(buf,"ref:")){+buf+=4;+while(isspace(*buf))+buf++;++strbuf_reset(symref);+strbuf_addstr(symref,buf);+*flags|=REF_ISSYMREF;+return0;+}++/*+*PleasenotethatFETCH_HEADhasadditional+*dataafterthesha.+*/+if(get_sha1_hex(buf,sha1)||+(buf[40]!='\0'&&!isspace(buf[40]))){+*flags|=REF_ISBROKEN;+errno=EINVAL;+return-1;+}++return0;+}+/* This function needs to return a meaningful errno on failure */staticconstchar*resolve_ref_1(constchar*refname,intresolve_flags,
@@ -1422,118 +1557,29 @@ static const char *resolve_ref_1(const char *refname,}for(symref_count=0;symref_count<MAXDEPTH;symref_count++){-constchar*path;-structstatst;-intfd;+intread_flags=0;-strbuf_reset(sb_path);-strbuf_git_path(sb_path,"%s",refname);-path=sb_path->buf;--/*-*Wemighthavetoloopbackheretoavoidarace-*condition:firstwelstat()thefile,thenwetry-*toreaditasalinkorasafile.Butifsomebody-*changesthetypeofthefile(file<->directory-*<->symlink)betweenthelstat()andreading,then-*wedon'twanttoreportthatasanerrorbutrather-*tryagainstartingwiththelstat().-*/-stat_ref:-if(lstat(path,&st)<0){-if(errno!=ENOENT)+if(read_raw_ref(refname,sha1,sb_refname,+sb_path,sb_contents,&read_flags)){+*flags|=read_flags;+if(errno!=ENOENT||(resolve_flags&RESOLVE_REF_READING))returnNULL;-if(resolve_missing_loose_ref(refname,sha1,flags)){-if(resolve_flags&RESOLVE_REF_READING){-errno=ENOENT;-returnNULL;-}-hashclr(sha1);-}-if(*flags&REF_BAD_NAME){-hashclr(sha1);+hashclr(sha1);+if(*flags&REF_BAD_NAME)*flags|=REF_ISBROKEN;-}returnrefname;}-/* Follow "normalized" - ie "refs/.." symlinks by hand */-if(S_ISLNK(st.st_mode)){-strbuf_reset(sb_contents);-if(strbuf_readlink(sb_contents,path,0)<0){-if(errno==ENOENT||errno==EINVAL)-/* inconsistent with lstat; retry */-gotostat_ref;-else-returnNULL;-}-if(starts_with(sb_contents->buf,"refs/")&&-!check_refname_format(sb_contents->buf,0)){-strbuf_swap(sb_refname,sb_contents);-refname=sb_refname->buf;-*flags|=REF_ISSYMREF;-if(resolve_flags&RESOLVE_REF_NO_RECURSE){-hashclr(sha1);-returnrefname;-}-continue;-}-}+*flags|=read_flags;-/* Is it a directory? */-if(S_ISDIR(st.st_mode)){-errno=EISDIR;-returnNULL;-}--/*-*Anythingelse,justopenitandtrytouseitas-*aref-*/-fd=open(path,O_RDONLY);-if(fd<0){-if(errno==ENOENT)-/* inconsistent with lstat; retry */-gotostat_ref;-else-returnNULL;-}-strbuf_reset(sb_contents);-if(strbuf_read(sb_contents,fd,256)<0){-intsave_errno=errno;-close(fd);-errno=save_errno;-returnNULL;-}-close(fd);-strbuf_rtrim(sb_contents);--/*-*Isitasymbolicref?-*/-if(!starts_with(sb_contents->buf,"ref:")){-/*-*PleasenotethatFETCH_HEADhasasecond-*linecontainingotherdata.-*/-if(get_sha1_hex(sb_contents->buf,sha1)||-(sb_contents->buf[40]!='\0'&&!isspace(sb_contents->buf[40]))){-*flags|=REF_ISBROKEN;-errno=EINVAL;-returnNULL;-}+if(!(read_flags&REF_ISSYMREF)){if(*flags&REF_BAD_NAME){hashclr(sha1);*flags|=REF_ISBROKEN;}returnrefname;}-*flags|=REF_ISSYMREF;-refname=sb_contents->buf+4;-while(isspace(*refname))-refname++;-strbuf_reset(sb_refname);-strbuf_addstr(sb_refname,refname);+refname=sb_refname->buf;if(resolve_flags&RESOLVE_REF_NO_RECURSE){hashclr(sha1);
From: Michael Haggerty <hidden> Date: 2016-06-15 23:08:54
It is never read, so we can pass NULL to resolve_ref_unsafe().
Signed-off-by: Michael Haggerty <redacted>
---
builtin/checkout.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
@@ -242,7 +242,6 @@ static int checkout_paths(const struct checkout_opts *opts,structcheckoutstate;staticchar*ps_matched;unsignedcharrev[20];-intflag;structcommit*head;interrs=0;structlock_file*lock_file;
@@ -375,7 +374,7 @@ static int checkout_paths(const struct checkout_opts *opts,if(write_locked_index(&the_index,lock_file,COMMIT_LOCK))die(_("unable to write new index file"));-read_ref_full("HEAD",0,rev,&flag);+read_ref_full("HEAD",0,rev,NULL);head=lookup_commit_reference_gently(rev,1);errs|=post_checkout_hook(head,head,0);
From: Michael Haggerty <hidden> Date: 2016-06-15 23:08:54
It is never read, so we can pass NULL to resolve_ref_unsafe().
Signed-off-by: Michael Haggerty <redacted>
---
builtin/merge.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
From: Michael Haggerty <hidden> Date: 2016-06-15 23:08:54
Make resolve_missing_loose_ref() only responsible for looking up a
packed reference, without worrying about whether we want to read or
write the reference and without setting errno on failure. Move the other
logic to the caller.
Signed-off-by: Michael Haggerty <redacted>
---
refs/files-backend.c | 24 ++++++++++--------------
1 file changed, 10 insertions(+), 14 deletions(-)
@@ -1389,14 +1387,8 @@ static int resolve_missing_loose_ref(const char *refname,*flags|=REF_ISPACKED;return0;}-/* The reference is not a packed reference, either. */-if(resolve_flags&RESOLVE_REF_READING){-errno=ENOENT;-return-1;-}else{-hashclr(sha1);-return0;-}+/* refname is not a packed reference. */+return-1;}/* This function needs to return a meaningful errno on failure */
From: Michael Haggerty <hidden> Date: 2016-06-15 23:08:54
Only use the result of resolve_ref_namespace() if it is non-NULL.
Signed-off-by: Michael Haggerty <redacted>
---
http-backend.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
@@ -1539,14 +1539,16 @@ out:}/* This function needs to return a meaningful errno on failure */-staticconstchar*resolve_ref_1(constchar*refname,-intresolve_flags,-unsignedchar*sha1,-int*flags,-structstrbuf*sb_refname)+constchar*resolve_ref_unsafe(constchar*refname,intresolve_flags,+unsignedchar*sha1,int*flags){+staticstructstrbufsb_refname=STRBUF_INIT;+intunused_flags;intsymref_count;+if(!flags)+flags=&unused_flags;+*flags=0;if(check_refname_format(refname,REFNAME_ALLOW_ONELEVEL)){
From: Michael Haggerty <hidden> Date: 2016-06-15 23:08:54
It is never read, so we can pass NULL to resolve_ref_unsafe().
Signed-off-by: Michael Haggerty <redacted>
---
builtin/submodule--helper.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
From: Michael Haggerty <hidden> Date: 2016-06-15 23:08:54
We can use (*flags & REF_BAD_NAME) for that purpose.
Signed-off-by: Michael Haggerty <redacted>
---
refs/files-backend.c | 13 +++++--------
1 file changed, 5 insertions(+), 8 deletions(-)
From: Michael Haggerty <hidden> Date: 2016-06-15 23:08:54
If there is an error in resolve_ref_unsafe(), it returns NULL. We check
for this case, but not until after calling strip_namespace(). Instead,
call strip_namespace() *after* the NULL check.
Signed-off-by: Michael Haggerty <redacted>
---
builtin/receive-pack.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
From: Michael Haggerty <hidden> Date: 2016-06-15 23:08:54
The loop's there anyway; we might as well use it.
Signed-off-by: Michael Haggerty <redacted>
---
refs/files-backend.c | 13 ++++++-------
1 file changed, 6 insertions(+), 7 deletions(-)
From: Michael Haggerty <hidden> Date: 2016-06-15 23:08:54
Instead of creating scratch space in resolve_ref_unsafe() and passing it
down through resolve_ref_1 to read_raw_ref(), teach read_raw_ref() to
manage its own scratch space. This reduces coupling across the functions
at the cost of some extra allocations. Also, when read_raw_ref() is
implemented for different reference backends, the other implementations
might have different scratch space requirements.
Note that we now preserve errno across the calls to strbuf_release(),
which calls free() and can thus theoretically overwrite errno.
Signed-off-by: Michael Haggerty <redacted>
---
refs/files-backend.c | 76 ++++++++++++++++++++++++++++------------------------
1 file changed, 41 insertions(+), 35 deletions(-)
@@ -1421,17 +1421,20 @@ static int resolve_missing_loose_ref(const char *refname,*refnamewillstillbevalidandunchanged.*/staticintread_raw_ref(constchar*refname,unsignedchar*sha1,-structstrbuf*symref,structstrbuf*sb_path,-structstrbuf*sb_contents,int*flags)+structstrbuf*symref,int*flags){+structstrbufsb_contents=STRBUF_INIT;+structstrbufsb_path=STRBUF_INIT;constchar*path;constchar*buf;structstatst;intfd;+intret=-1;+intsave_errno;-strbuf_reset(sb_path);-strbuf_git_path(sb_path,"%s",refname);-path=sb_path->buf;+strbuf_reset(&sb_path);+strbuf_git_path(&sb_path,"%s",refname);+path=sb_path.buf;stat_ref:/*
@@ -1446,36 +1449,38 @@ stat_ref:if(lstat(path,&st)<0){if(errno!=ENOENT)-return-1;+gotoout;if(resolve_missing_loose_ref(refname,sha1,flags)){errno=ENOENT;-return-1;+gotoout;}-return0;+ret=0;+gotoout;}/* Follow "normalized" - ie "refs/.." symlinks by hand */if(S_ISLNK(st.st_mode)){-strbuf_reset(sb_contents);-if(strbuf_readlink(sb_contents,path,0)<0){+strbuf_reset(&sb_contents);+if(strbuf_readlink(&sb_contents,path,0)<0){if(errno==ENOENT||errno==EINVAL)/* inconsistent with lstat; retry */gotostat_ref;else-return-1;+gotoout;}-if(starts_with(sb_contents->buf,"refs/")&&-!check_refname_format(sb_contents->buf,0)){-strbuf_swap(sb_contents,symref);+if(starts_with(sb_contents.buf,"refs/")&&+!check_refname_format(sb_contents.buf,0)){+strbuf_swap(&sb_contents,symref);*flags|=REF_ISSYMREF;-return0;+ret=0;+gotoout;}}/* Is it a directory? */if(S_ISDIR(st.st_mode)){errno=EISDIR;-return-1;+gotoout;}/*
@@ -1488,18 +1493,18 @@ stat_ref:/* inconsistent with lstat; retry */gotostat_ref;else-return-1;+gotoout;}-strbuf_reset(sb_contents);-if(strbuf_read(sb_contents,fd,256)<0){+strbuf_reset(&sb_contents);+if(strbuf_read(&sb_contents,fd,256)<0){intsave_errno=errno;close(fd);errno=save_errno;-return-1;+gotoout;}close(fd);-strbuf_rtrim(sb_contents);-buf=sb_contents->buf;+strbuf_rtrim(&sb_contents);+buf=sb_contents.buf;if(starts_with(buf,"ref:")){buf+=4;while(isspace(*buf))
@@ -1519,10 +1525,17 @@ stat_ref:(buf[40]!='\0'&&!isspace(buf[40]))){*flags|=REF_ISBROKEN;errno=EINVAL;-return-1;+gotoout;}-return0;+ret=0;++out:+save_errno=errno;+strbuf_release(&sb_path);+strbuf_release(&sb_contents);+errno=save_errno;+returnret;}/* This function needs to return a meaningful errno on failure */
From: Michael Haggerty <hidden> Date: 2016-06-15 23:08:54
In place of `buf`, use `refname`, which is anyway a better description
of what is being pointed at.
Signed-off-by: Michael Haggerty <redacted>
---
refs/files-backend.c | 13 ++++++-------
1 file changed, 6 insertions(+), 7 deletions(-)
From: Michael Haggerty <hidden> Date: 2016-06-15 23:08:54
It is never read, so we can pass NULL to resolve_ref_unsafe().
Signed-off-by: Michael Haggerty <redacted>
---
builtin/fsck.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
From: Michael Haggerty <hidden> Date: 2016-06-15 23:08:54
read_raw_ref() is going to be part of the vtable for reference backends,
so clean up its interface to use "unsigned int flags" rather than "int
flags". Its caller still uses signed int for its flags arguments. But
changing that would touch a lot of code, so leave it for now.
Signed-off-by: Michael Haggerty <redacted>
---
refs/files-backend.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
From: Michael Haggerty <hidden> Date: 2016-06-15 23:08:54
There is no need to adjust *flags if we're just about to fail.
Signed-off-by: Michael Haggerty <redacted>
---
refs/files-backend.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
From: Michael Haggerty <hidden> Date: 2016-06-15 23:08:54
If the caller passes flags==NULL, then set it to point at a local
scratch variable. This removes the need for a lot of "if (flags)" guards
in resolve_ref_1() and resolve_missing_loose_ref().
Signed-off-by: Michael Haggerty <redacted>
---
refs/files-backend.c | 31 +++++++++++++------------------
1 file changed, 13 insertions(+), 18 deletions(-)
@@ -1383,8 +1383,7 @@ static int resolve_missing_loose_ref(const char *refname,entry=get_packed_ref(refname);if(entry){hashcpy(sha1,entry->u.value.oid.hash);-if(flags)-*flags|=REF_ISPACKED;+*flags|=REF_ISPACKED;return0;}/* refname is not a packed reference. */
From: David Turner <hidden> Date: 2016-06-15 23:08:55
On Wed, 2016-03-23 at 11:04 +0100, Michael Haggerty wrote:
Patch 04/33 in David Turner's refs-backend-lmdb series v7 [1] did way
too much in a single patch, and in fact got a few minor things wrong.
Instead of that patch, I suggest this patch series, which
...
LGTM. I think I would squash these patches:
fsck_head_link(): remove unneeded flag variable
cmd_merge(): remove unneeded flag variable
get_default_remote(): remove unneeded flag variable
checkout_paths(): remove unneeded flag variable
But that's up to you.
I incorporated your changes into the lmdb backend. To make merging
later more convenient, I rebased on top of pu -- I think this mainly
depends on jk/check-repository-format, but I also included some fixes
for a couple of tests that had been changed by other patches.
The current version can be found here:
https://github.com/dturner-tw/git/tree/dturner/pluggable-backends
I won't resend the full patchset to the list until I hear back on the
rest of the review.
It seems like maybe we should now split this into two patchsets:
everything up to and including "refs: move resolve_ref_unsafe into
common code" does not depend on the backend structure and could go in
earlier. If you agree, we could send that first series and get it in,
hopefully reducing later merge conflicts.
From: Michael Haggerty <hidden> Date: 2016-06-15 23:08:59
On 03/24/2016 07:47 AM, David Turner wrote:
[...]
I incorporated your changes into the lmdb backend. To make merging
later more convenient, I rebased on top of pu -- I think this mainly
depends on jk/check-repository-format, but I also included some fixes
for a couple of tests that had been changed by other patches.
I think rebasing changes on top of pu is counterproductive. I believe
that Junio had extra work rebasing your earlier series onto a merge of
the minimum number of topics that it really depended on. There is no way
that he could merge the branch in this form because it would imply
merging all of pu.
See the zeroth section of SubmittingPatches [1] for the guidelines.
The current version can be found here:
https://github.com/dturner-tw/git/tree/dturner/pluggable-backends
I won't resend the full patchset to the list until I hear back on the
rest of the review.
It seems like maybe we should now split this into two patchsets:
everything up to and including "refs: move resolve_ref_unsafe into
common code" does not depend on the backend structure and could go in
earlier. If you agree, we could send that first series and get it in,
hopefully reducing later merge conflicts.
That sounds like a good idea. It's always a relief to get work merged
and not have to keep porting it along.
There are three patches later in the series that (I think) also don't
have specifically to do with pluggable backends. These could potentially
also be considered for earlier merge to reduce the size of what remains:
* refs: don't dereference on rename
* refs: on symref reflog expire, lock symref not referrent
* refs: resolve symbolic refs first
But note that I haven't audited those patches yet, so I'm not saying
that they are necessarily ready to be merged.
Michael
[1] https://github.com/git/git/blob/master/Documentation/SubmittingPatches
From: David Turner <hidden> Date: 2016-06-15 23:09:06
On Sun, 2016-03-27 at 07:22 +0200, Michael Haggerty wrote:
On 03/24/2016 07:47 AM, David Turner wrote:
quoted
[...]
I incorporated your changes into the lmdb backend. To make merging
later more convenient, I rebased on top of pu -- I think this
mainly
depends on jk/check-repository-format, but I also included some
fixes
for a couple of tests that had been changed by other patches.
I think rebasing changes on top of pu is counterproductive. I believe
that Junio had extra work rebasing your earlier series onto a merge
of
the minimum number of topics that it really depended on. There is no
way
that he could merge the branch in this form because it would imply
merging all of pu.
See the zeroth section of SubmittingPatches [1] for the guidelines.
I'm a bit confused because
[PATCH 18/21] get_default_remote(): remove unneeded flag variable
doesn't do anything on master -- it depends on some patch in pu. And
we definitely want to pick up jk/check-repository-format (which doesn't
include whatever 18/21 depends on).
So what do you think our base should be?
From: Michael Haggerty <hidden> Date: 2016-06-15 23:09:07
On 03/29/2016 10:12 PM, David Turner wrote:
On Sun, 2016-03-27 at 07:22 +0200, Michael Haggerty wrote:
quoted
On 03/24/2016 07:47 AM, David Turner wrote:
quoted
[...]
I incorporated your changes into the lmdb backend. To make merging
later more convenient, I rebased on top of pu -- I think this
mainly
depends on jk/check-repository-format, but I also included some
fixes
for a couple of tests that had been changed by other patches.
I think rebasing changes on top of pu is counterproductive. I believe
that Junio had extra work rebasing your earlier series onto a merge
of
the minimum number of topics that it really depended on. There is no
way
that he could merge the branch in this form because it would imply
merging all of pu.
See the zeroth section of SubmittingPatches [1] for the guidelines.
I'm a bit confused because
[PATCH 18/21] get_default_remote(): remove unneeded flag variable
doesn't do anything on master -- it depends on some patch in pu. And
we definitely want to pick up jk/check-repository-format (which doesn't
include whatever 18/21 depends on).
So what do you think our base should be?
I think the preference is to base a patch series on the merge of master
plus the minimum number of topics in pu (ideally, none) that are
"essential" prerequisites of the changes in the patch series. For
example, the version of this patch series that Junio has in his tree was
based on master + sb/submodule-parallel-update. Even if there are minor
conflicts with another in-flight topic, it is easier for Junio to
resolve the conflicts when merging the topics together than to rebase
the patch series over and over as the other patch series evolves. The
goal of this practice is of course to allow patch series to evolve
independently of each other as much as possible.
Of course if you have insights into nontrivial conflicts between your
patch series and others, it would be helpful to discuss these in your
cover letter.
Michael
From: David Turner <hidden> Date: 2016-06-15 23:09:08
On Wed, 2016-03-30 at 08:37 +0200, Michael Haggerty wrote:
On 03/29/2016 10:12 PM, David Turner wrote:
quoted
On Sun, 2016-03-27 at 07:22 +0200, Michael Haggerty wrote:
quoted
On 03/24/2016 07:47 AM, David Turner wrote:
quoted
[...]
I incorporated your changes into the lmdb backend. To make
merging
later more convenient, I rebased on top of pu -- I think this
mainly
depends on jk/check-repository-format, but I also included some
fixes
for a couple of tests that had been changed by other patches.
I think rebasing changes on top of pu is counterproductive. I
believe
that Junio had extra work rebasing your earlier series onto a
merge
of
the minimum number of topics that it really depended on. There is
no
way
that he could merge the branch in this form because it would
imply
merging all of pu.
See the zeroth section of SubmittingPatches [1] for the
guidelines.
I'm a bit confused because
[PATCH 18/21] get_default_remote(): remove unneeded flag variable
doesn't do anything on master -- it depends on some patch in pu.
And
we definitely want to pick up jk/check-repository-format (which
doesn't
include whatever 18/21 depends on).
So what do you think our base should be?
I think the preference is to base a patch series on the merge of
master
plus the minimum number of topics in pu (ideally, none) that are
"essential" prerequisites of the changes in the patch series. For
example, the version of this patch series that Junio has in his tree
was
based on master + sb/submodule-parallel-update.
Even if there are minor
conflicts with another in-flight topic, it is easier for Junio to
resolve the conflicts when merging the topics together than to rebase
the patch series over and over as the other patch series evolves. The
goal of this practice is of course to allow patch series to evolve
independently of each other as much as possible.
Of course if you have insights into nontrivial conflicts between your
patch series and others, it would be helpful to discuss these in your
cover letter.
If I am reading this correctly, it looks like your series also has a
few more sb submodule patches, e.g. sb/submodule-init, which is
responsible for the code that 18/21 depends on.
I think jk/check-repository-format is also good to get in first,
because it changes the startup sequence a bit and it's a bit tricky to
figure out what needs to change in dt/refs-backend-lmdb as a result of
it.
But I can't just merge jk/check-repository-format on top of 71defe0047
-- some function signatures have changed in the run-command stuff and
it seems kind of annoying to fix up.
So I propose instead that we just drop 18/21 for now, and use just
jk/check-repository-format as the base.
Does this seem reasonable to you?
From: Michael Haggerty <hidden> Date: 2016-06-15 23:09:08
On 03/30/2016 10:05 PM, David Turner wrote:
On Wed, 2016-03-30 at 08:37 +0200, Michael Haggerty wrote:
quoted
On 03/29/2016 10:12 PM, David Turner wrote:
quoted
On Sun, 2016-03-27 at 07:22 +0200, Michael Haggerty wrote:
quoted
On 03/24/2016 07:47 AM, David Turner wrote:
quoted
[...]
I incorporated your changes into the lmdb backend. To make
merging
later more convenient, I rebased on top of pu -- I think this
mainly
depends on jk/check-repository-format, but I also included some
fixes
for a couple of tests that had been changed by other patches.
I think rebasing changes on top of pu is counterproductive. I
believe
that Junio had extra work rebasing your earlier series onto a
merge
of
the minimum number of topics that it really depended on. There is
no
way
that he could merge the branch in this form because it would
imply
merging all of pu.
See the zeroth section of SubmittingPatches [1] for the
guidelines.
I'm a bit confused because
[PATCH 18/21] get_default_remote(): remove unneeded flag variable
doesn't do anything on master -- it depends on some patch in pu.
And
we definitely want to pick up jk/check-repository-format (which
doesn't
include whatever 18/21 depends on).
So what do you think our base should be?
I think the preference is to base a patch series on the merge of
master
plus the minimum number of topics in pu (ideally, none) that are
"essential" prerequisites of the changes in the patch series. For
example, the version of this patch series that Junio has in his tree
was
based on master + sb/submodule-parallel-update.
Even if there are minor
conflicts with another in-flight topic, it is easier for Junio to
resolve the conflicts when merging the topics together than to rebase
the patch series over and over as the other patch series evolves. The
goal of this practice is of course to allow patch series to evolve
independently of each other as much as possible.
Of course if you have insights into nontrivial conflicts between your
patch series and others, it would be helpful to discuss these in your
cover letter.
If I am reading this correctly, it looks like your series also has a
few more sb submodule patches, e.g. sb/submodule-init, which is
responsible for the code that 18/21 depends on.
I think jk/check-repository-format is also good to get in first,
because it changes the startup sequence a bit and it's a bit tricky to
figure out what needs to change in dt/refs-backend-lmdb as a result of
it.
But I can't just merge jk/check-repository-format on top of 71defe0047
-- some function signatures have changed in the run-command stuff and
it seems kind of annoying to fix up.
So I propose instead that we just drop 18/21 for now, and use just
jk/check-repository-format as the base.
Does this seem reasonable to you?
Yes, that's fine. Patch 18/21 is just a random cleanup that nothing else
depends on. Will you do the rebasing? If so, please let me know where I
can fetch the result from.
Michael
From: David Turner <hidden> Date: 2016-06-15 23:09:09
On Thu, 2016-03-31 at 18:14 +0200, Michael Haggerty wrote:
On 03/30/2016 10:05 PM, David Turner wrote:
quoted
On Wed, 2016-03-30 at 08:37 +0200, Michael Haggerty wrote:
quoted
On 03/29/2016 10:12 PM, David Turner wrote:
quoted
On Sun, 2016-03-27 at 07:22 +0200, Michael Haggerty wrote:
quoted
On 03/24/2016 07:47 AM, David Turner wrote:
quoted
[...]
I incorporated your changes into the lmdb backend. To make
merging
later more convenient, I rebased on top of pu -- I think
this
mainly
depends on jk/check-repository-format, but I also included
some
fixes
for a couple of tests that had been changed by other
patches.
I think rebasing changes on top of pu is counterproductive. I
believe
that Junio had extra work rebasing your earlier series onto a
merge
of
the minimum number of topics that it really depended on.
There is
no
way
that he could merge the branch in this form because it would
imply
merging all of pu.
See the zeroth section of SubmittingPatches [1] for the
guidelines.
I'm a bit confused because
[PATCH 18/21] get_default_remote(): remove unneeded flag
variable
doesn't do anything on master -- it depends on some patch in
pu.
And
we definitely want to pick up jk/check-repository-format (which
doesn't
include whatever 18/21 depends on).
So what do you think our base should be?
I think the preference is to base a patch series on the merge of
master
plus the minimum number of topics in pu (ideally, none) that are
"essential" prerequisites of the changes in the patch series. For
example, the version of this patch series that Junio has in his
tree
was
based on master + sb/submodule-parallel-update.
Even if there are minor
conflicts with another in-flight topic, it is easier for Junio to
resolve the conflicts when merging the topics together than to
rebase
the patch series over and over as the other patch series evolves.
The
goal of this practice is of course to allow patch series to
evolve
independently of each other as much as possible.
Of course if you have insights into nontrivial conflicts between
your
patch series and others, it would be helpful to discuss these in
your
cover letter.
If I am reading this correctly, it looks like your series also has
a
few more sb submodule patches, e.g. sb/submodule-init, which is
responsible for the code that 18/21 depends on.
I think jk/check-repository-format is also good to get in first,
because it changes the startup sequence a bit and it's a bit tricky
to
figure out what needs to change in dt/refs-backend-lmdb as a result
of
it.
But I can't just merge jk/check-repository-format on top of
71defe0047
-- some function signatures have changed in the run-command stuff
and
it seems kind of annoying to fix up.
So I propose instead that we just drop 18/21 for now, and use just
jk/check-repository-format as the base.
Does this seem reasonable to you?
Yes, that's fine. Patch 18/21 is just a random cleanup that nothing
else
depends on. Will you do the rebasing? If so, please let me know where
I
can fetch the result from.
From: Stefan Beller <hidden> Date: 2016-06-15 23:09:09
On Wed, Mar 30, 2016 at 1:05 PM, David Turner [off-list ref] wrote:
On Wed, 2016-03-30 at 08:37 +0200, Michael Haggerty wrote:
quoted
On 03/29/2016 10:12 PM, David Turner wrote:
quoted
On Sun, 2016-03-27 at 07:22 +0200, Michael Haggerty wrote:
quoted
On 03/24/2016 07:47 AM, David Turner wrote:
quoted
[...]
I incorporated your changes into the lmdb backend. To make
merging
later more convenient, I rebased on top of pu -- I think this
mainly
depends on jk/check-repository-format, but I also included some
fixes
for a couple of tests that had been changed by other patches.
I think rebasing changes on top of pu is counterproductive. I
believe
that Junio had extra work rebasing your earlier series onto a
merge
of
the minimum number of topics that it really depended on. There is
no
way
that he could merge the branch in this form because it would
imply
merging all of pu.
See the zeroth section of SubmittingPatches [1] for the
guidelines.
I'm a bit confused because
[PATCH 18/21] get_default_remote(): remove unneeded flag variable
doesn't do anything on master -- it depends on some patch in pu.
And
we definitely want to pick up jk/check-repository-format (which
doesn't
include whatever 18/21 depends on).
So what do you think our base should be?
I think the preference is to base a patch series on the merge of
master
plus the minimum number of topics in pu (ideally, none) that are
"essential" prerequisites of the changes in the patch series. For
example, the version of this patch series that Junio has in his tree
was
based on master + sb/submodule-parallel-update.
Even if there are minor
conflicts with another in-flight topic, it is easier for Junio to
resolve the conflicts when merging the topics together than to rebase
the patch series over and over as the other patch series evolves. The
goal of this practice is of course to allow patch series to evolve
independently of each other as much as possible.
Of course if you have insights into nontrivial conflicts between your
patch series and others, it would be helpful to discuss these in your
cover letter.
If I am reading this correctly, it looks like your series also has a
few more sb submodule patches, e.g. sb/submodule-init, which is
responsible for the code that 18/21 depends on.
I think jk/check-repository-format is also good to get in first,
because it changes the startup sequence a bit and it's a bit tricky to
figure out what needs to change in dt/refs-backend-lmdb as a result of
it.
But I can't just merge jk/check-repository-format on top of 71defe0047
-- some function signatures have changed in the run-command stuff and
it seems kind of annoying to fix up.
So I propose instead that we just drop 18/21 for now, and use just
jk/check-repository-format as the base.
By 18/21 you mean
[PATCH 18/21] get_default_remote(): remove unneeded flag variable
in builtin/submodule--helper.c?
You could drop that and I'll pick it up in one of the submodule series',
if that is more convenient for you.
Does this seem reasonable to you?
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
From: David Turner <hidden> Date: 2016-06-15 23:09:10
On Thu, 2016-03-31 at 18:37 -0700, Stefan Beller wrote:
On Wed, Mar 30, 2016 at 1:05 PM, David Turner <
dturner@twopensource.com> wrote:
quoted
On Wed, 2016-03-30 at 08:37 +0200, Michael Haggerty wrote:
quoted
On 03/29/2016 10:12 PM, David Turner wrote:
quoted
On Sun, 2016-03-27 at 07:22 +0200, Michael Haggerty wrote:
quoted
On 03/24/2016 07:47 AM, David Turner wrote:
quoted
[...]
I incorporated your changes into the lmdb backend. To make
merging
later more convenient, I rebased on top of pu -- I think
this
mainly
depends on jk/check-repository-format, but I also included
some
fixes
for a couple of tests that had been changed by other
patches.
I think rebasing changes on top of pu is counterproductive. I
believe
that Junio had extra work rebasing your earlier series onto a
merge
of
the minimum number of topics that it really depended on.
There is
no
way
that he could merge the branch in this form because it would
imply
merging all of pu.
See the zeroth section of SubmittingPatches [1] for the
guidelines.
I'm a bit confused because
[PATCH 18/21] get_default_remote(): remove unneeded flag
variable
doesn't do anything on master -- it depends on some patch in
pu.
And
we definitely want to pick up jk/check-repository-format (which
doesn't
include whatever 18/21 depends on).
So what do you think our base should be?
I think the preference is to base a patch series on the merge of
master
plus the minimum number of topics in pu (ideally, none) that are
"essential" prerequisites of the changes in the patch series. For
example, the version of this patch series that Junio has in his
tree
was
based on master + sb/submodule-parallel-update.
Even if there are minor
conflicts with another in-flight topic, it is easier for Junio to
resolve the conflicts when merging the topics together than to
rebase
the patch series over and over as the other patch series evolves.
The
goal of this practice is of course to allow patch series to
evolve
independently of each other as much as possible.
Of course if you have insights into nontrivial conflicts between
your
patch series and others, it would be helpful to discuss these in
your
cover letter.
If I am reading this correctly, it looks like your series also has
a
few more sb submodule patches, e.g. sb/submodule-init, which is
responsible for the code that 18/21 depends on.
I think jk/check-repository-format is also good to get in first,
because it changes the startup sequence a bit and it's a bit tricky
to
figure out what needs to change in dt/refs-backend-lmdb as a result
of
it.
But I can't just merge jk/check-repository-format on top of
71defe0047
-- some function signatures have changed in the run-command stuff
and
it seems kind of annoying to fix up.
So I propose instead that we just drop 18/21 for now, and use just
jk/check-repository-format as the base.
By 18/21 you mean
[PATCH 18/21] get_default_remote(): remove unneeded flag variable
in builtin/submodule--helper.c?
You could drop that and I'll pick it up in one of the submodule
series',
if that is more convenient for you.