[PATCHv2,00/24] Bring more repository handles into our code base]
mbox series

Message ID 20181030220817.61691-1-sbeller@google.com
Headers show
Series
  • Bring more repository handles into our code base]
Related show

Message

Stefan Beller Oct. 30, 2018, 10:07 p.m. UTC
This resends sb/more-repo-in-api

On Thu, Oct 25, 2018 at 2:14 AM SZEDER Gábor <szeder.dev@gmail.com> wrote:
> On Tue, Oct 16, 2018 at 04:35:49PM -0700, Stefan Beller wrote:
> > This converts the 'show_submodule_header' function to use
> > the repository API properly, such that the submodule objects
> > are not added to the main object store.
>
> This patch breaks the test suite with 'GIT_TEST_COMMIT_GRAPH=1', in
> particular 't4041-diff-submodule-option.sh' fails with:
> [...]

I debugged into this and the last four patches will fix it.

I also picked up the patch for pending semantic patches, as the
first patch, can I have your sign off, please?

This also addresses Jonathans feedback in
https://public-inbox.org/git/20181019203750.110741-1-jonathantanmy@google.com/

A range-diff is below.

Thanks,
Stefan

SZEDER Gábor (1):
  Makefile: add pending semantic patches

Stefan Beller (23):
  sha1_file: allow read_object to read objects in arbitrary repositories
  packfile: allow has_packed_and_bad to handle arbitrary repositories
  object-store: allow read_object_file_extended to read from arbitrary
    repositories
  object-store: prepare read_object_file to deal with arbitrary
    repositories
  object-store: prepare has_{sha1, object}_file[_with_flags] to handle
    arbitrary repositories
  object: parse_object to honor its repository argument
  commit: allow parse_commit* to handle arbitrary repositories
  commit-reach.c: allow paint_down_to_common to handle arbitrary
    repositories
  commit-reach.c: allow merge_bases_many to handle arbitrary
    repositories
  commit-reach.c: allow remove_redundant to handle arbitrary
    repositories
  commit-reach.c: allow get_merge_bases_many_0 to handle arbitrary
    repositories
  commit-reach: prepare get_merge_bases to handle arbitrary repositories
  commit-reach: prepare in_merge_bases[_many] to handle arbitrary
    repositories
  commit: prepare get_commit_buffer to handle arbitrary repositories
  commit: prepare repo_unuse_commit_buffer to handle arbitrary
    repositories
  commit: prepare logmsg_reencode to handle arbitrary repositories
  pretty: prepare format_commit_message to handle arbitrary repositories
  submodule: use submodule repos for object lookup
  submodule: don't add submodule as odb for push
  commit-graph: convert remaining function to handle arbitrary
    repositories
  commit: make free_commit_buffer and release_commit_memory repository
    agnostic
  path.h: make REPO_GIT_PATH_FUNC repository agnostic
  t/helper/test-repository: celebrate independence from the_repository

 Makefile                                      |   6 +-
 builtin/fsck.c                                |   3 +-
 builtin/log.c                                 |   6 +-
 builtin/rev-list.c                            |   3 +-
 cache.h                                       |   2 +
 commit-graph.c                                |  40 +++--
 commit-reach.c                                |  73 +++++----
 commit-reach.h                                |  38 +++--
 commit.c                                      |  41 ++---
 commit.h                                      |  43 +++++-
 .../coccinelle/the_repository.pending.cocci   | 144 ++++++++++++++++++
 object-store.h                                |  35 ++++-
 object.c                                      |   8 +-
 packfile.c                                    |   5 +-
 packfile.h                                    |   2 +-
 path.h                                        |   2 +-
 pretty.c                                      |  28 ++--
 pretty.h                                      |   7 +-
 sha1-file.c                                   |  34 +++--
 streaming.c                                   |   2 +-
 submodule.c                                   |  79 +++++++---
 t/helper/test-repository.c                    |  10 ++
 22 files changed, 459 insertions(+), 152 deletions(-)
 create mode 100644 contrib/coccinelle/the_repository.pending.cocci

git range-diff origin/sb/more-repo-in-api...
[...] # rebased to origin/master
  -:  ---------- > 116:  4ede3d42df Seventh batch for 2.20
  -:  ---------- > 117:  b1de196491 Makefile: add pending semantic patches
  1:  1b9b5c695e = 118:  f1be5eb487 sha1_file: allow read_object to read objects in arbitrary repositories
  2:  33b94066f2 = 119:  9100a5705d packfile: allow has_packed_and_bad to handle arbitrary repositories
  3:  5217b6b1e1 = 120:  a4ad7791da object-store: allow read_object_file_extended to read from arbitrary repositories
  4:  2b7239b55b ! 121:  5d7b3a6dd9 object-store: prepare read_object_file to deal with arbitrary repositories
    @@ -19,10 +19,10 @@
         Signed-off-by: Stefan Beller <sbeller@google.com>
         Signed-off-by: Junio C Hamano <gitster@pobox.com>
     
    - diff --git a/contrib/coccinelle/the_repository.cocci b/contrib/coccinelle/the_repository.cocci
    + diff --git a/contrib/coccinelle/the_repository.pending.cocci b/contrib/coccinelle/the_repository.pending.cocci
      new file mode 100644
      --- /dev/null
    - +++ b/contrib/coccinelle/the_repository.cocci
    + +++ b/contrib/coccinelle/the_repository.pending.cocci
     @@
     +// This file is used for the ongoing refactoring of
     +// bringing the index or repository struct in all of
    @@ -36,6 +36,7 @@
     +- read_object_file(
     ++ repo_read_object_file(the_repository,
     +  E, F, G)
    ++
     
      diff --git a/object-store.h b/object-store.h
      --- a/object-store.h
  5:  24291f4d84 ! 122:  77d406fc51 object-store: prepare has_{sha1, object}_file[_with_flags] to handle arbitrary repositories
    @@ -3,16 +3,14 @@
         object-store: prepare has_{sha1, object}_file[_with_flags] to handle arbitrary repositories
     
         Signed-off-by: Stefan Beller <sbeller@google.com>
    -    Signed-off-by: Junio C Hamano <gitster@pobox.com>
     
    - diff --git a/contrib/coccinelle/the_repository.cocci b/contrib/coccinelle/the_repository.cocci
    - --- a/contrib/coccinelle/the_repository.cocci
    - +++ b/contrib/coccinelle/the_repository.cocci
    + diff --git a/contrib/coccinelle/the_repository.pending.cocci b/contrib/coccinelle/the_repository.pending.cocci
    + --- a/contrib/coccinelle/the_repository.pending.cocci
    + +++ b/contrib/coccinelle/the_repository.pending.cocci
     @@
    - - read_object_file(
      + repo_read_object_file(the_repository,
        E, F, G)
    -+
    + 
     +@@
     +expression E;
     +@@
  6:  6aa209978e = 123:  7224b378d8 object: parse_object to honor its repository argument
  7:  9b2d6aa7c3 ! 124:  e4d4ae08ca commit: allow parse_commit* to handle arbitrary repositories
    @@ -93,9 +93,9 @@
      
      struct buffer_slab;
     
    - diff --git a/contrib/coccinelle/the_repository.cocci b/contrib/coccinelle/the_repository.cocci
    - --- a/contrib/coccinelle/the_repository.cocci
    - +++ b/contrib/coccinelle/the_repository.cocci
    + diff --git a/contrib/coccinelle/the_repository.pending.cocci b/contrib/coccinelle/the_repository.pending.cocci
    + --- a/contrib/coccinelle/the_repository.pending.cocci
    + +++ b/contrib/coccinelle/the_repository.pending.cocci
     @@
      - has_object_file_with_flags(
      + repo_has_object_file_with_flags(the_repository,
  8:  0e7e681118 ! 125:  f739ef6078 commit-reach.c: allow paint_down_to_common to handle arbitrary repositories
    @@ -5,7 +5,6 @@
         As the function is file local and not widely used, migrate it all at once.
     
         Signed-off-by: Stefan Beller <sbeller@google.com>
    -    Signed-off-by: Junio C Hamano <gitster@pobox.com>
     
      diff --git a/commit-reach.c b/commit-reach.c
      --- a/commit-reach.c
  9:  e83b26f5b3 ! 126:  c054a80564 commit-reach.c: allow merge_bases_many to handle arbitrary repositories
    @@ -3,7 +3,6 @@
         commit-reach.c: allow merge_bases_many to handle arbitrary repositories
     
         Signed-off-by: Stefan Beller <sbeller@google.com>
    -    Signed-off-by: Junio C Hamano <gitster@pobox.com>
     
      diff --git a/commit-reach.c b/commit-reach.c
      --- a/commit-reach.c
 10:  d80b9de832 ! 127:  3434a5a262 commit-reach.c: allow remove_redundant to handle arbitrary repositories
    @@ -3,7 +3,6 @@
         commit-reach.c: allow remove_redundant to handle arbitrary repositories
     
         Signed-off-by: Stefan Beller <sbeller@google.com>
    -    Signed-off-by: Junio C Hamano <gitster@pobox.com>
     
      diff --git a/commit-reach.c b/commit-reach.c
      --- a/commit-reach.c
 11:  3ec21ad503 ! 128:  82f6e797bf commit-reach.c: allow get_merge_bases_many_0 to handle arbitrary repositories
    @@ -3,7 +3,6 @@
         commit-reach.c: allow get_merge_bases_many_0 to handle arbitrary repositories
     
         Signed-off-by: Stefan Beller <sbeller@google.com>
    -    Signed-off-by: Junio C Hamano <gitster@pobox.com>
     
      diff --git a/commit-reach.c b/commit-reach.c
      --- a/commit-reach.c
 12:  3f21279f50 ! 129:  b66b636a97 commit-reach: prepare get_merge_bases to handle arbitrary repositories
    @@ -9,7 +9,6 @@
         functions behind a wrapper macro.
     
         Signed-off-by: Stefan Beller <sbeller@google.com>
    -    Signed-off-by: Junio C Hamano <gitster@pobox.com>
     
      diff --git a/commit-reach.c b/commit-reach.c
      --- a/commit-reach.c
    @@ -91,9 +90,9 @@
      int is_descendant_of(struct commit *commit, struct commit_list *with_commit);
      int in_merge_bases_many(struct commit *commit, int nr_reference, struct commit **reference);
     
    - diff --git a/contrib/coccinelle/the_repository.cocci b/contrib/coccinelle/the_repository.cocci
    - --- a/contrib/coccinelle/the_repository.cocci
    - +++ b/contrib/coccinelle/the_repository.cocci
    + diff --git a/contrib/coccinelle/the_repository.pending.cocci b/contrib/coccinelle/the_repository.pending.cocci
    + --- a/contrib/coccinelle/the_repository.pending.cocci
    + +++ b/contrib/coccinelle/the_repository.pending.cocci
     @@
      - parse_commit(
      + repo_parse_commit(the_repository,
    @@ -124,3 +123,4 @@
     +- get_merge_bases_many_dirty(
     ++ repo_get_merge_bases_many_dirty(the_repository,
     +  E, F, G);
    ++
 13:  4decc3acc1 ! 130:  e68e48ca91 commit-reach: prepare in_merge_bases[_many] to handle arbitrary repositories
    @@ -3,7 +3,6 @@
         commit-reach: prepare in_merge_bases[_many] to handle arbitrary repositories
     
         Signed-off-by: Stefan Beller <sbeller@google.com>
    -    Signed-off-by: Junio C Hamano <gitster@pobox.com>
     
      diff --git a/commit-reach.c b/commit-reach.c
      --- a/commit-reach.c
    @@ -76,14 +75,13 @@
      /*
       * Takes a list of commits and returns a new list where those
     
    - diff --git a/contrib/coccinelle/the_repository.cocci b/contrib/coccinelle/the_repository.cocci
    - --- a/contrib/coccinelle/the_repository.cocci
    - +++ b/contrib/coccinelle/the_repository.cocci
    + diff --git a/contrib/coccinelle/the_repository.pending.cocci b/contrib/coccinelle/the_repository.pending.cocci
    + --- a/contrib/coccinelle/the_repository.pending.cocci
    + +++ b/contrib/coccinelle/the_repository.pending.cocci
     @@
    - - get_merge_bases_many_dirty(
      + repo_get_merge_bases_many_dirty(the_repository,
        E, F, G);
    -+
    + 
     +@@
     +expression E;
     +expression F;
 14:  141b86ea89 ! 131:  aac401ca98 commit: prepare get_commit_buffer to handle arbitrary repositories
    @@ -3,7 +3,6 @@
         commit: prepare get_commit_buffer to handle arbitrary repositories
     
         Signed-off-by: Stefan Beller <sbeller@google.com>
    -    Signed-off-by: Junio C Hamano <gitster@pobox.com>
     
      diff --git a/commit.c b/commit.c
      --- a/commit.c
    @@ -46,9 +45,9 @@
      /*
       * Tell the commit subsytem that we are done with a particular commit buffer.
     
    - diff --git a/contrib/coccinelle/the_repository.cocci b/contrib/coccinelle/the_repository.cocci
    - --- a/contrib/coccinelle/the_repository.cocci
    - +++ b/contrib/coccinelle/the_repository.cocci
    + diff --git a/contrib/coccinelle/the_repository.pending.cocci b/contrib/coccinelle/the_repository.pending.cocci
    + --- a/contrib/coccinelle/the_repository.pending.cocci
    + +++ b/contrib/coccinelle/the_repository.pending.cocci
     @@
      - in_merge_bases_many(
      + repo_in_merge_bases_many(the_repository,
 15:  2cc415c1db ! 132:  c45e6d2c0c commit: prepare repo_unuse_commit_buffer to handle arbitrary repositories
    @@ -3,7 +3,6 @@
         commit: prepare repo_unuse_commit_buffer to handle arbitrary repositories
     
         Signed-off-by: Stefan Beller <sbeller@google.com>
    -    Signed-off-by: Junio C Hamano <gitster@pobox.com>
     
      diff --git a/commit.c b/commit.c
      --- a/commit.c
    @@ -42,9 +41,9 @@
      /*
       * Free any cached object buffer associated with the commit.
     
    - diff --git a/contrib/coccinelle/the_repository.cocci b/contrib/coccinelle/the_repository.cocci
    - --- a/contrib/coccinelle/the_repository.cocci
    - +++ b/contrib/coccinelle/the_repository.cocci
    + diff --git a/contrib/coccinelle/the_repository.pending.cocci b/contrib/coccinelle/the_repository.pending.cocci
    + --- a/contrib/coccinelle/the_repository.pending.cocci
    + +++ b/contrib/coccinelle/the_repository.pending.cocci
     @@
      - get_commit_buffer(
      + repo_get_commit_buffer(the_repository,
 16:  554daa8cfc ! 133:  25e8f5dec4 commit: prepare logmsg_reencode to handle arbitrary repositories
    @@ -24,9 +24,9 @@
      
      /** Removes the first commit from a list sorted by date, and adds all
     
    - diff --git a/contrib/coccinelle/the_repository.cocci b/contrib/coccinelle/the_repository.cocci
    - --- a/contrib/coccinelle/the_repository.cocci
    - +++ b/contrib/coccinelle/the_repository.cocci
    + diff --git a/contrib/coccinelle/the_repository.pending.cocci b/contrib/coccinelle/the_repository.pending.cocci
    + --- a/contrib/coccinelle/the_repository.pending.cocci
    + +++ b/contrib/coccinelle/the_repository.pending.cocci
     @@
      - unuse_commit_buffer(
      + repo_unuse_commit_buffer(the_repository,
 17:  bd8737176b ! 134:  2cab6c18b6 pretty: prepare format_commit_message to handle arbitrary repositories
    @@ -5,9 +5,9 @@
         Signed-off-by: Stefan Beller <sbeller@google.com>
         Signed-off-by: Junio C Hamano <gitster@pobox.com>
     
    - diff --git a/contrib/coccinelle/the_repository.cocci b/contrib/coccinelle/the_repository.cocci
    - --- a/contrib/coccinelle/the_repository.cocci
    - +++ b/contrib/coccinelle/the_repository.cocci
    + diff --git a/contrib/coccinelle/the_repository.pending.cocci b/contrib/coccinelle/the_repository.pending.cocci
    + --- a/contrib/coccinelle/the_repository.pending.cocci
    + +++ b/contrib/coccinelle/the_repository.pending.cocci
     @@
      - logmsg_reencode(
      + repo_logmsg_reencode(the_repository,
 18:  b303ef65e7 ! 135:  794592c573 submodule: use submodule repos for object lookup
    @@ -7,7 +7,6 @@
         are not added to the main object store.
     
         Signed-off-by: Stefan Beller <sbeller@google.com>
    -    Signed-off-by: Junio C Hamano <gitster@pobox.com>
     
      diff --git a/submodule.c b/submodule.c
      --- a/submodule.c
    @@ -46,24 +45,26 @@
     + * in .gitmodules. This function exists only to preserve historical behavior,
     + *
     + * Returns 0 on success, -1 when the submodule is not present.
    -+ */
    -+static int open_submodule(struct repository *out, const char *path)
    +  */
    +-static void show_submodule_header(struct diff_options *o, const char *path,
    ++static struct repository *open_submodule(const char *path)
     +{
     +	struct strbuf sb = STRBUF_INIT;
    ++	struct repository *out = xmalloc(sizeof(*out));
     +
     +	if (submodule_to_gitdir(&sb, path) || repo_init(out, sb.buf, NULL)) {
     +		strbuf_release(&sb);
    -+		return -1;
    ++		free(out);
    ++		return NULL;
     +	}
     +
    -+	out->submodule_prefix = xstrdup(path);
     +	out->submodule_prefix = xstrfmt("%s%s/",
     +					the_repository->submodule_prefix ?
     +					the_repository->submodule_prefix :
     +					"", path);
     +
     +	strbuf_release(&sb);
    -+	return 0;
    ++	return out;
     +}
     +
     +/*
    @@ -73,8 +74,7 @@
     + * If it can locate the submodule git directory it will create a repository
     + * handle for the submodule and lookup both the left and right commits and
     + * put them into the left and right pointers.
    -  */
    --static void show_submodule_header(struct diff_options *o, const char *path,
    ++ */
     +static void show_submodule_header(struct diff_options *o,
     +		const char *path,
      		struct object_id *one, struct object_id *two,
    @@ -116,11 +116,9 @@
      	struct rev_info rev;
      	struct commit *left = NULL, *right = NULL;
      	struct commit_list *merge_bases = NULL;
    -+	struct repository subrepo, *sub = &subrepo;
    -+
    -+	if (open_submodule(&subrepo, path) < 0)
    -+		sub = NULL;
    ++	struct repository *sub;
      
    ++	sub = open_submodule(path);
      	show_submodule_header(o, path, one, two, dirty_submodule,
     -			      &left, &right, &merge_bases);
     +			      sub, &left, &right, &merge_bases);
    @@ -147,8 +145,10 @@
      		free_commit_list(merge_bases);
      	clear_commit_marks(left, ~0);
      	clear_commit_marks(right, ~0);
    -+	if (sub)
    ++	if (sub) {
     +		repo_clear(sub);
    ++		free(sub);
    ++	}
      }
      
      void show_submodule_inline_diff(struct diff_options *o, const char *path,
    @@ -156,11 +156,9 @@
      	struct commit_list *merge_bases = NULL;
      	struct child_process cp = CHILD_PROCESS_INIT;
      	struct strbuf sb = STRBUF_INIT;
    -+	struct repository subrepo, *sub = &subrepo;
    -+
    -+	if (open_submodule(&subrepo, path) < 0)
    -+		sub = NULL;
    ++	struct repository *sub;
      
    ++	sub = open_submodule(path);
      	show_submodule_header(o, path, one, two, dirty_submodule,
     -			      &left, &right, &merge_bases);
     +			      sub, &left, &right, &merge_bases);
    @@ -171,8 +169,10 @@
      		clear_commit_marks(left, ~0);
      	if (right)
      		clear_commit_marks(right, ~0);
    -+	if (sub)
    ++	if (sub) {
     +		repo_clear(sub);
    ++		free(sub);
    ++	}
      }
      
      int should_update_submodules(void)
 19:  dbb0dd9322 ! 136:  df748a859f submodule: don't add submodule as odb for push
    @@ -8,8 +8,10 @@
         objects, and the actual push is done by spawning another process,
         which handles object access by itself.)
     
    +    This code of push_submodule() is exercised in t5531 and continues
    +    to work, showing that the submodule odbc is not needed.
    +
         Signed-off-by: Stefan Beller <sbeller@google.com>
    -    Signed-off-by: Junio C Hamano <gitster@pobox.com>
     
      diff --git a/submodule.c b/submodule.c
      --- a/submodule.c
  -:  ---------- > 137:  c9e990afac commit-graph: convert remaining function to handle arbitrary repositories
  -:  ---------- > 138:  a086f3dd11 commit: make free_commit_buffer and release_commit_memory repository agnostic
  -:  ---------- > 139:  878bd34799 path.h: make REPO_GIT_PATH_FUNC repository agnostic
  -:  ---------- > 140:  641824bbeb t/helper/test-repository: celebrate independence from the_repository

Comments

Junio C Hamano Oct. 31, 2018, 6:41 a.m. UTC | #1
Stefan Beller <sbeller@google.com> writes:

> I also picked up the patch for pending semantic patches, as the
> first patch, can I have your sign off, please?

I find this step quite lacking.  

What was posted would have been perfectly fine as a "how about doing
it this way" weatherbaloon patch, but as a part of real series, it
needs to explain to the developers what the distinctions between two
classes are, and who is to use the cocci patch at what point in the
development cycle, etc., in an accompanying documentation update.

So can we have both sign-off and write-up (perhaps cut&paste from
the original e-mail discussion)?

> Stefan Beller (23):
>   sha1_file: allow read_object to read objects in arbitrary repositories
>   packfile: allow has_packed_and_bad to handle arbitrary repositories
>   object-store: allow read_object_file_extended to read from arbitrary
>     repositories
>   object-store: prepare read_object_file to deal with arbitrary
>     repositories
>   object-store: prepare has_{sha1, object}_file[_with_flags] to handle
>     arbitrary repositories
>   object: parse_object to honor its repository argument
>   commit: allow parse_commit* to handle arbitrary repositories
>   commit-reach.c: allow paint_down_to_common to handle arbitrary
>     repositories
>   commit-reach.c: allow merge_bases_many to handle arbitrary
>     repositories
>   commit-reach.c: allow remove_redundant to handle arbitrary
>     repositories
>   commit-reach.c: allow get_merge_bases_many_0 to handle arbitrary
>     repositories
>   commit-reach: prepare get_merge_bases to handle arbitrary repositories
>   commit-reach: prepare in_merge_bases[_many] to handle arbitrary
>     repositories
>   commit: prepare get_commit_buffer to handle arbitrary repositories
>   commit: prepare repo_unuse_commit_buffer to handle arbitrary
>     repositories
>   commit: prepare logmsg_reencode to handle arbitrary repositories
>   pretty: prepare format_commit_message to handle arbitrary repositories
>   submodule: use submodule repos for object lookup
>   submodule: don't add submodule as odb for push
>   commit-graph: convert remaining function to handle arbitrary
>     repositories
>   commit: make free_commit_buffer and release_commit_memory repository
>     agnostic
>   path.h: make REPO_GIT_PATH_FUNC repository agnostic
>   t/helper/test-repository: celebrate independence from the_repository

It seems that this topic is full of commits with overly long title.

>
>  Makefile                                      |   6 +-
>  builtin/fsck.c                                |   3 +-
>  builtin/log.c                                 |   6 +-
>  builtin/rev-list.c                            |   3 +-
>  cache.h                                       |   2 +
>  commit-graph.c                                |  40 +++--
>  commit-reach.c                                |  73 +++++----
>  commit-reach.h                                |  38 +++--
>  commit.c                                      |  41 ++---
>  commit.h                                      |  43 +++++-
>  .../coccinelle/the_repository.pending.cocci   | 144 ++++++++++++++++++
>  object-store.h                                |  35 ++++-
>  object.c                                      |   8 +-
>  packfile.c                                    |   5 +-
>  packfile.h                                    |   2 +-
>  path.h                                        |   2 +-
>  pretty.c                                      |  28 ++--
>  pretty.h                                      |   7 +-
>  sha1-file.c                                   |  34 +++--
>  streaming.c                                   |   2 +-
>  submodule.c                                   |  79 +++++++---
>  t/helper/test-repository.c                    |  10 ++
>  22 files changed, 459 insertions(+), 152 deletions(-)
>  create mode 100644 contrib/coccinelle/the_repository.pending.cocci
>
> git range-diff origin/sb/more-repo-in-api...
> [...] # rebased to origin/master

I offhand do not recall what happened during these 100+ pacthes.
DId we acquire something significantly new that would have
conflicted with this new round of the topic?  I do not mind at all
seeing that a series gets adjusted to the updated codebase, but I do
dislike seeing it done without explanation---an unexplained rebase
to a new base is a wasted opportunity to learn what areas of the
codebase are so hot that multiple and separate topics would want to
touch them.

>   -:  ---------- > 116:  4ede3d42df Seventh batch for 2.20
>   -:  ---------- > 117:  b1de196491 Makefile: add pending semantic patches
>   1:  1b9b5c695e = 118:  f1be5eb487 sha1_file: allow read_object to read objects in arbitrary repositories
>   2:  33b94066f2 = 119:  9100a5705d packfile: allow has_packed_and_bad to handle arbitrary repositories
>   3:  5217b6b1e1 = 120:  a4ad7791da object-store: allow read_object_file_extended to read from arbitrary repositories
>   4:  2b7239b55b ! 121:  5d7b3a6dd9 object-store: prepare read_object_file to deal with arbitrary repositories
>     @@ -19,10 +19,10 @@
>          Signed-off-by: Stefan Beller <sbeller@google.com>
>          Signed-off-by: Junio C Hamano <gitster@pobox.com>
>      
>     - diff --git a/contrib/coccinelle/the_repository.cocci b/contrib/coccinelle/the_repository.cocci
>     + diff --git a/contrib/coccinelle/the_repository.pending.cocci b/contrib/coccinelle/the_repository.pending.cocci
>       new file mode 100644
>       --- /dev/null
>     - +++ b/contrib/coccinelle/the_repository.cocci
>     + +++ b/contrib/coccinelle/the_repository.pending.cocci
>      @@
>      +// This file is used for the ongoing refactoring of
>      +// bringing the index or repository struct in all of
>     @@ -36,6 +36,7 @@
>      +- read_object_file(
>      ++ repo_read_object_file(the_repository,
>      +  E, F, G)
>     ++

Is it necessary for this file to end with a trailing blank line?



Thanks.
Stefan Beller Nov. 1, 2018, 7:45 p.m. UTC | #2
On Tue, Oct 30, 2018 at 11:41 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Stefan Beller <sbeller@google.com> writes:
>
> > I also picked up the patch for pending semantic patches, as the
> > first patch, can I have your sign off, please?
>
> I find this step quite lacking.
>
> What was posted would have been perfectly fine as a "how about doing
> it this way" weatherbaloon patch, but as a part of real series, it
> needs to explain to the developers what the distinctions between two
> classes are, and who is to use the cocci patch at what point in the
> development cycle, etc., in an accompanying documentation update.

if only we had documentation [as found via "git grep coccicheck"]
that I could update ... I'd totally do that.
I guess this asks for documentation to begin with, now?

> So can we have both sign-off and write-up (perhaps cut&paste from
> the original e-mail discussion)?

I'll see where to put the docs; I assumed commit messages are enough.
63f0a758a0 (add coccicheck make target, 2016-09-15)
is what I found nice.


> >   t/helper/test-repository: celebrate independence from the_repository
>
> It seems that this topic is full of commits with overly long title.

yep.
> > git range-diff origin/sb/more-repo-in-api...
> > [...] # rebased to origin/master
>
> I offhand do not recall what happened during these 100+ pacthes.
> DId we acquire something significantly new that would have
> conflicted with this new round of the topic?  I do not mind at all
> seeing that a series gets adjusted to the updated codebase, but I do
> dislike seeing it done without explanation---an unexplained rebase
> to a new base is a wasted opportunity to learn what areas of the
> codebase are so hot that multiple and separate topics would want to
> touch them.

From the point of view of these large scale refactorings,
all of the code is hot, e.g. the patch that was present in the RFC
"apply all semantic patches" would clash with nearly any topic.

As I do not carry that patch any more, I do not recall any conflicts
that needed to be resolved.

Thanks.
Junio C Hamano Nov. 2, 2018, 2 a.m. UTC | #3
Stefan Beller <sbeller@google.com> writes:

>> What was posted would have been perfectly fine as a "how about doing
>> it this way" weatherbaloon patch, but as a part of real series, it
>> needs to explain to the developers what the distinctions between two
>> classes are, and who is to use the cocci patch at what point in the
>> development cycle, etc., in an accompanying documentation update.
>
> if only we had documentation [as found via "git grep coccicheck"]
> that I could update ... I'd totally do that.
> I guess this asks for documentation to begin with, now?

So far, we didn't even need any, as there was no "workflow" to speak
of.  It's just "any random developer finds a suggested update,
either by running 'make coccicheck' oneself or by peeking Travis
log, and sends in a patch".

But the "pending" thing has a lot more workflow elements, who is
responsible for noticing update suggested by "pending" ones, for
updating the code, and for promoting "pending" to the normal.  These
are all new, and these are all needed as ongoing basis to help
developers---I'd say the way Documentation/SubmittingPatches helps
developers is very close to the way this new document would help them.