Re: [PATCH 5/6] fetch-pack: move common check and marking together

[Jonathan Tan]

On Tue, Jun 5, 2018 at 5:01 PM, Jonathan Nieder  wrote:
> I like it.  I think it should be possible to describe the benefit of
> this patch without reference to the specifics of the subsequent one.
> Maybe something like:
>
> When receiving 'ACK  continue' for a common commit,
> check if the commit was already known to be common and mark it
> as such if not up front.  This should make future refactoring
> of how the information about common commits is stored more
> straightforward.
>
> No visible change intended.

Thanks, I'll use your suggestion.

Re: [PATCH 5/6] fetch-pack: move common check and marking together

[Jonathan Nieder]

Hi,

Jonathan Tan wrote:

> This enables the calculation of was_common and the invocation to
> mark_common() to be abstracted into a single call to the negotiator API
> (to be introduced in a subsequent patch).

I like it.  I think it should be possible to describe the benefit of
this patch without reference to the specifics of the subsequent one.
Maybe something like:

When receiving 'ACK  continue' for a common commit,
check if the commit was already known to be common and mark it
as such if not up front.  This should make future refactoring
of how the information about common commits is stored more
straightforward.

No visible change intended.

> Signed-off-by: Jonathan Tan 
> ---
>  fetch-pack.c | 6 --
>  1 file changed, 4 insertions(+), 2 deletions(-)

With or without such a clarification,
Reviewed-by: Jonathan Nieder 

Thanks.

[PATCH 5/6] fetch-pack: move common check and marking together

[Jonathan Tan]

This enables the calculation of was_common and the invocation to
mark_common() to be abstracted into a single call to the negotiator API
(to be introduced in a subsequent patch).

Signed-off-by: Jonathan Tan 
---
 fetch-pack.c | 6 --
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/fetch-pack.c b/fetch-pack.c
index ec92929bc..54dd3feb8 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -499,11 +499,14 @@ static int find_common(struct data *data, struct 
fetch_pack_args *args,
case ACK_continue: {
struct commit *commit =
lookup_commit(result_oid);
+   int was_common;
if (!commit)
die(_("invalid commit %s"), 
oid_to_hex(result_oid));
+   was_common = commit->object.flags & 
COMMON;
+   mark_common(data, commit, 0, 1);
if (args->stateless_rpc
 && ack == ACK_common
-&& !(commit->object.flags & COMMON)) {
+&& !was_common) {
/* We need to replay the have 
for this object
 * on the next RPC request so 
the peer knows
 * it is in common with us.
@@ -520,7 +523,6 @@ static int find_common(struct data *data, struct 
fetch_pack_args *args,
} else if (!args->stateless_rpc
   || ack != ACK_common)
in_vain = 0;
-   mark_common(data, commit, 0, 1);
retval = 0;
got_continue = 1;
if (ack == ACK_ready)
-- 
2.17.0.768.g1526ddbba1.dirty