From: Kim Altintop <hidden> Date: 2021-07-30 13:59:26
When 'upload-pack' runs within the context of a git namespace, treat any
'want-ref' lines the client sends as relative to that namespace.
Also check if the wanted ref is hidden via 'hideRefs', and respond with
an error otherwise. It was previously possible to request any ref, but
note that this is still the case unless 'hideRefs' is in effect.
Signed-off-by: Kim Altintop <redacted>
---
Please excuse my newbie ness.
t/t5703-upload-pack-ref-in-want.sh | 77 ++++++++++++++++++++++++++++++
upload-pack.c | 15 +++---
2 files changed, 86 insertions(+), 6 deletions(-)
From: Kim Altintop <hidden> Date: 2021-07-30 14:04:52
Please excuse my newbie ness.
Oops I'm sorry. `send-email` remembered this from a dry run.
Jonathan: I took the test code from your original patch introducing ref-in-want,
but modified it substantially. Let me know if it is conventional to credit you
anyway, and by which trailer.
From: Kim Altintop <hidden> Date: 2021-07-31 20:37:10
When 'upload-pack' runs within the context of a git namespace, treat any
'want-ref' lines the client sends as relative to that namespace.
Also check if the wanted ref is hidden via 'hideRefs'. If it is hidden,
respond with an error as if the ref didn't exist.
Signed-off-by: Kim Altintop <redacted>
---
Changes from v1:
* Amend commit message
* upload-pack.c: fix variable renaming (how could this even work?)
* upload-pack.c: hide namespace in all output, including die()
* t5703: don't use subshell in repo setup
* t5703: use "env" keyword to correctly scope GIT_NAMESPACE
t/t5703-upload-pack-ref-in-want.sh | 72 ++++++++++++++++++++++++++++++
upload-pack.c | 17 ++++---
2 files changed, 82 insertions(+), 7 deletions(-)
@@ -298,6 +298,78 @@ test_expect_success 'fetching with wildcard that matches multiple refs' 'grep"want-ref refs/heads/o/bar"log'+REPO="$(pwd)/repo-ns"++test_expect_success'setup namespaced repo''+gitinit-bmain"$REPO"&&+cd"$REPO"&&+test_commita&&+test_commitb&&+gitcheckouta&&+test_commitc&&+gitcheckouta&&+test_commitd&&+gitupdate-refrefs/heads/ns-nob&&+gitupdate-refrefs/namespaces/ns/refs/heads/ns-yesc&&+gitupdate-refrefs/namespaces/ns/refs/heads/hiddend&&+git-C"$REPO"configuploadpack.allowRefInWanttrue&&+git-C"$REPO"configtransfer.hideRefsrefs/heads/hidden+'
If you're not using a subshell to set up the repo, you should add '-C
"$REPO"' to all the "git" commands (like you do in the last 2 lines)
instead of "cd"-ing halfway through the test. The helper function
test_commit also has that facility ('test_commit -C "$REPO" a', for
example).
For the failure tests, it's safer to write them in pairs - one that
succeeds and one that fails. Here, a typo in "ns-no" (e.g. if I wrote
"ns-noo" instead) would cause the exact same result, but if we were to
write a pair of tests, we wouldn't have this problem.
To do this, you can bundle the same code into a function and call them
from both tests. E.g.:
setup_want_ns_no () {
(common code)
}
test_expect_success 'want-ref without namespace works...' '
setup_want_ref_outside_namespace &&
test-tool ... &&
check_output
'
test_expect_success '...but, with namespace, does not work' '
setup_want_ref_outside_namespace &&
test_must_fail env GIT_NAMESPACE=ns ... &&
grep "unknown ref" out
'
The first test_expect_success does seem redundant, but I can't think of
a better way to ensure that the helper function (setup_want_ns_no in
this case) is written correctly.
Besides my comments about the tests, I think that this patch looks good.
Junio had a relevant question [1]:
OK. Assuming that it makes sense for the hideRefs mechanism to kick
in here (which I would prefer to hear from others who've worked with
this code, say Jonathan Tan?), the updated code makes sense.
I think it makes sense here. I checked my old code that Kim linked [2],
and in that patch I put it somewhere else (specifically, the part that
prints out the "wanted-ref" lines), but it makes sense that it is
different. In my version, "want-ref" supports globs and is allowed to
match 0 refs, but in Brandon's final version, "want-ref" does not
support globs and must match the ref, so checking it upon parse (as is
done in this commit) is the most reasonable.
Questions from Kim:
Jonathan: I took the test code from your original patch introducing ref-in-want,
but modified it substantially. Let me know if it is conventional to credit you
anyway, and by which trailer.
I don't think there's a convention, but you can use the "Helped-by:"
trailer if you want.
I have also updated the code for the v2 to use refname_nons for any die() calls,
as I realised that this may be transmitted to the client via sideband (is that
correct?).
From: Kim Altintop <hidden> Date: 2021-08-04 20:37:30
On Mon Aug 2, 2021 at 11:06 PM CEST, Jonathan Tan wrote:
quoted
+test_expect_success 'setup namespaced repo' '
+ git init -b main "$REPO" &&
+ cd "$REPO" &&
+ test_commit a &&
+ test_commit b &&
+ git checkout a &&
+ test_commit c &&
+ git checkout a &&
+ test_commit d &&
+ git update-ref refs/heads/ns-no b &&
+ git update-ref refs/namespaces/ns/refs/heads/ns-yes c &&
+ git update-ref refs/namespaces/ns/refs/heads/hidden d &&
+ git -C "$REPO" config uploadpack.allowRefInWant true &&
+ git -C "$REPO" config transfer.hideRefs refs/heads/hidden
+'
If you're not using a subshell to set up the repo, you should add '-C
"$REPO"' to all the "git" commands (like you do in the last 2 lines)
instead of "cd"-ing halfway through the test. The helper function
test_commit also has that facility ('test_commit -C "$REPO" a', for
example).
Ah, that answers the question raised by Junio in the first review. I'll revert
to using a subshell, as that seems clearer and is used throughout the file.
For the failure tests, it's safer to write them in pairs - one that
succeeds and one that fails. Here, a typo in "ns-no" (e.g. if I wrote
"ns-noo" instead) would cause the exact same result, but if we were to
write a pair of tests, we wouldn't have this problem.
That's a good suggestion. However, I'm having some difficulties finding just the
right amount of common code to extract due to
a. having to pass the namespace via env instead of --namespace (and the
different position for the success / test_must_fail cases), and
b. having to rely on _persistent_ config for hideRefs, as opposed to being able
to pass it via -c to upload-pack (ie. I'm worried about tests breaking if
they get reordered)
So I'm not exactly happy with what I came up with for v3, but I'd also be
reluctant to add --namespace / -c support to test-tool as part of this patch.
Let me know what you think.
From: Kim Altintop <hidden> Date: 2021-08-04 20:43:03
When 'upload-pack' runs within the context of a git namespace, treat any
'want-ref' lines the client sends as relative to that namespace.
Also check if the wanted ref is hidden via 'hideRefs'. If it is hidden,
respond with an error as if the ref didn't exist.
Helped-by: Jonathan Tan [off-list ref]
Signed-off-by: Kim Altintop <redacted>
---
Changes from v2:
* upload-pack.c: release strbuf
* t5730: revert to scoping to $REPO via subshell in setup
* t5730: add "cross-check" tests as per review comments from Jonathan
t/t5703-upload-pack-ref-in-want.sh | 128 +++++++++++++++++++++++++++++
upload-pack.c | 18 ++--
2 files changed, 139 insertions(+), 7 deletions(-)
From: Kim Altintop <hidden> Date: 2021-08-04 21:01:25
When 'upload-pack' runs within the context of a git namespace, treat any
'want-ref' lines the client sends as relative to that namespace.
Also check if the wanted ref is hidden via 'hideRefs'. If it is hidden,
respond with an error as if the ref didn't exist.
Helped-by: Jonathan Tan [off-list ref]
Signed-off-by: Kim Altintop <redacted>
---
Changes from v3:
* t5703: make tests actually pass :)
t/t5703-upload-pack-ref-in-want.sh | 140 +++++++++++++++++++++++++++++
upload-pack.c | 18 ++--
2 files changed, 151 insertions(+), 7 deletions(-)
From: Kim Altintop <hidden> Date: 2021-08-09 17:56:41
Fifth reroll of [0].
The patch has been split into three commits, where the first is a small
refactoring of the test file, the second is the original patch + tests, and the
last one attempts to express the `transfer.hideRefs` behaviour more accurately
in the docs.
Thanks for your help and patience!
This version of the patch is also available at [1].
[0]: https://lore.kernel.org/git/20210804205951.668140-1-kim@eagain.st/
[1]: https://github.com/kim/git/tree/ka/namespaced-want-ref-v5
Kim Altintop (3):
t5730: introduce fetch command helper
upload-pack.c: treat want-ref relative to namespace
docs: clarify the interaction of transfer.hideRefs and namespaces
Documentation/config/transfer.txt | 17 ++-
t/t5703-upload-pack-ref-in-want.sh | 236 +++++++++++++++++++++++++----
upload-pack.c | 18 ++-
3 files changed, 224 insertions(+), 47 deletions(-)
--
2.32.0
From: Kim Altintop <hidden> Date: 2021-08-09 17:57:32
Assembling a "raw" fetch command to be fed directly to "test-tool serve-v2"
is extracted into a test helper.
Suggested-by: Junio C Hamano <redacted>
Signed-off-by: Kim Altintop <redacted>
---
t/t5703-upload-pack-ref-in-want.sh | 107 ++++++++++++++++++++---------
1 file changed, 74 insertions(+), 33 deletions(-)
@@ -40,6 +40,54 @@ write_command () {fi}+# Write a complete fetch command to stdout, suitable for use with `test-tool+# pkt-line`. "want-ref", "want", and "have" values can be given in this order,+# with sections separated by "--".+#+# Examples:+#+# write_fetch_command refs/heads/main+#+# write_fetch_command \+# refs/heads/main \+# -- \+# -- \+# $(git rev-parse x)+#+# write_fetch_command \+# --+# $(git rev-parse a) \+# --+# $(git rev-parse b)+write_fetch_command(){+write_commandfetch&&+echo"0001"&&+echo"no-progress"||return+while:+do+case$#in0)break;;esac&&+case"$1"in--)shift;break;;esac&&+echo"want-ref $1"&&+shift||return+done&&+while:+do+case$#in0)break;;esac&&+case"$1"in--)shift;break;;esac&&+echo"want $1"&&+shift||return+done&&+while:+do+case$#in0)break;;esac&&+case"$1"in--)shift;break;;esac&&+echo"have $1"&&+shift||return+done&&+echo"done"&&+echo"0000"+}+# c(o/foo) d(o/bar)# \ /# b e(baz) f(main)
@@ -145,14 +189,13 @@ test_expect_success 'mix want and want-ref' 'gitrev-parseef>expected_commits&&test-toolpkt-linepack>in<<-EOF&&-$(write_commandfetch)-0001-no-progress-want-refrefs/heads/main-want$(gitrev-parsee)-have$(gitrev-parsea)-done-0000+$(write_fetch_command\+refs/heads/main\+--\+$(gitrev-parsee)\+--\+$(gitrev-parsea)\+)EOFtest-toolserve-v2--stateless-rpc>out<in&&
@@ -166,15 +209,13 @@ test_expect_success 'want-ref with ref we already have commit for' 'EOF>expected_commits&&-oid=$(gitrev-parsec)&&test-toolpkt-linepack>in<<-EOF&&-$(write_commandfetch)-0001-no-progress-want-refrefs/heads/o/foo-have$oid-done-0000+$(write_fetch_command\+refs/heads/o/foo\+--\+--\+$(gitrev-parsec)\+)EOFtest-toolserve-v2--stateless-rpc>out<in&&--
From: Kim Altintop <hidden> Date: 2021-08-09 17:59:52
When 'upload-pack' runs within the context of a git namespace, treat any
'want-ref' lines the client sends as relative to that namespace.
Also check if the wanted ref is hidden via 'hideRefs'. If it is hidden,
respond with an error as if the ref didn't exist.
Helped-by: Jonathan Tan [off-list ref]
Signed-off-by: Kim Altintop <redacted>
---
t/t5703-upload-pack-ref-in-want.sh | 129 +++++++++++++++++++++++++++++
upload-pack.c | 18 ++--
2 files changed, 140 insertions(+), 7 deletions(-)
From: Kim Altintop <hidden> Date: 2021-08-09 17:59:54
Expand the section about namespaces in the documentation of
`transfer.hideRefs` to point out the subtle differences between
`upload-pack` and `receive-pack`.
9bedd82017 (upload-pack.c: treat want-ref relative to namespace,
2021-07-30) taught `upload-pack` to reject `want-ref`s for hidden refs,
which is now documented.
Signed-off-by: Kim Altintop <redacted>
---
Documentation/config/transfer.txt | 17 ++++++++++-------
1 file changed, 10 insertions(+), 7 deletions(-)
@@ -52,13 +52,16 @@ If you have multiple hideRefs values, later entries override earlier ones (and entries in more-specific config files override less-specific ones). + If a namespace is in use, the namespace prefix is stripped from each-reference before it is matched against `transfer.hiderefs` patterns.-For example, if `refs/heads/master` is specified in `transfer.hideRefs` and-the current namespace is `foo`, then `refs/namespaces/foo/refs/heads/master`-is omitted from the advertisements but `refs/heads/master` and-`refs/namespaces/bar/refs/heads/master` are still advertised as so-called-"have" lines. In order to match refs before stripping, add a `^` in front of-the ref name. If you combine `!` and `^`, `!` must be specified first.+reference before it is matched against `transfer.hiderefs` patterns. For+example, if `refs/heads/master` is specified in `transfer.hideRefs` and the+current namespace is `foo`, then `refs/namespaces/foo/refs/heads/master` is+omitted from the advertisements. If `uploadpack.allowRefInWant` is set,+`upload-pack` will treat `want-ref refs/heads/master` in a protocol v2+`fetch` command as if `refs/heads/master` was unknown. Note, however, that+`receive-pack` will still advertise the object id `refs/heads/master` is+pointing to, but will conceil the name of the ref. In order to match refs+before stripping, add a `^` in front of the ref name. If you combine `!` and+`^`, `!` must be specified first. + Even if you hide refs, a client may still be able to steal the target objects via the techniques described in the "SECURITY" section of the--
From: Jonathan Nieder <hidden> Date: 2021-08-09 19:40:56
Hi,
Kim Altintop wrote:
Assembling a "raw" fetch command to be fed directly to "test-tool serve-v2"
is extracted into a test helper.
Suggested-by: Junio C Hamano <redacted>
Signed-off-by: Kim Altintop <redacted>
---
t/t5703-upload-pack-ref-in-want.sh | 107 ++++++++++++++++++++---------
1 file changed, 74 insertions(+), 33 deletions(-)
@@ -40,6 +40,54 @@ write_command () {fi}+# Write a complete fetch command to stdout, suitable for use with `test-tool+# pkt-line`. "want-ref", "want", and "have" values can be given in this order,+# with sections separated by "--".+#+# Examples:+#+# write_fetch_command refs/heads/main+#+# write_fetch_command \+# refs/heads/main \+# -- \+# -- \+# $(git rev-parse x)+#+# write_fetch_command \+# --+# $(git rev-parse a) \+# --+# $(git rev-parse b)+write_fetch_command(){
Hm, for comparison let me see what this looks like without the helper:
after some prior step
object_format=$(test_oid algo) && # probably just once in a setup step
x=$(git rev-parse x) &&
we can write
cat <<-EOF &&
command=fetch
object-format=$object_format
0001
no-progress
want-ref refs/heads/main
have $x
done
0000
EOF
I find that a little _easier_ to read than a write_fetch_command call,
because I don't have to chase the definition and x is labeled as a
'have'.
Is there some additional motivation for this helper?
Here the entirety of the input to "test-tool pkt-line pack" is the
entirety of the output from write_fetch_command, which would suggest
either
a. making write_fetch_command pipe its output to "test-tool pkt-line
pack", or
b. using a pipe instead of a command substitution, like
"write_fetch_command ... | test-tool pkt-line pack >in"
(although as mentioned above, I think it's simpler to inline the
write_fetch_command and even the write_command as well).
Thanks and hope that helps,
Jonathan
From: Kim Altintop <hidden> Date: 2021-08-09 21:57:29
Thanks for chiming in!
On Mon Aug 9, 2021 at 9:40 PM CEST, Jonathan Nieder wrote:
Hm, for comparison let me see what this looks like without the helper:
after some prior step
object_format=$(test_oid algo) && # probably just once in a setup step
x=$(git rev-parse x) &&
we can write
cat <<-EOF &&
command=fetch
object-format=$object_format
0001
no-progress
want-ref refs/heads/main
have $x
done
0000
EOF
I find that a little _easier_ to read than a write_fetch_command call,
because I don't have to chase the definition and x is labeled as a
'have'.
Is there some additional motivation for this helper?
It was suggested in earlier review rounds. I think it does improve readability
as quite some lines need to be repeated everywhere a fetch command is assembled.
I agree though that not having some sort of "named arguments" is a bit
detrimental.
Here the entirety of the input to "test-tool pkt-line pack" is the
entirety of the output from write_fetch_command, which would suggest
either
a. making write_fetch_command pipe its output to "test-tool pkt-line
pack", or
b. using a pipe instead of a command substitution, like
"write_fetch_command ... | test-tool pkt-line pack >in"
(although as mentioned above, I think it's simpler to inline the
write_fetch_command and even the write_command as well).
Yes, although I believe a pipe cannot be used as we don't have bash's `set -o
pipefail` (ie. the exit status will always be the status of the last command in
the pipe, even if an earlier one failed).
Perhaps an alternative would be:
write_fetch_command () {
write_command fetch &&
echo "0001" &&
echo "no-progress" &&
cat /dev/stdin &&
echo "done" &&
echo "0000"
}
Which would then be called like so:
write_fetch_command >pkt_cmd <<-EOF &&
want-ref refs/heads/main
have $(git rev-parse a)
EOF
test-tool pkt-line pack <pkt_cmd >in &&
test-tool serve-v2 --stateless-rpc >out <in &&
I'm not sure how portable that is, though. Maybe using `while read -r` instead
of `cat /dev/stdin`?
@@ -52,13 +52,16 @@ If you have multiple hideRefs values, later entries override earlier ones (and entries in more-specific config files override less-specific ones). + If a namespace is in use, the namespace prefix is stripped from each-reference before it is matched against `transfer.hiderefs` patterns.-For example, if `refs/heads/master` is specified in `transfer.hideRefs` and-the current namespace is `foo`, then `refs/namespaces/foo/refs/heads/master`-is omitted from the advertisements but `refs/heads/master` and-`refs/namespaces/bar/refs/heads/master` are still advertised as so-called-"have" lines. In order to match refs before stripping, add a `^` in front of-the ref name. If you combine `!` and `^`, `!` must be specified first.+reference before it is matched against `transfer.hiderefs` patterns. For+example, if `refs/heads/master` is specified in `transfer.hideRefs` and the+current namespace is `foo`, then `refs/namespaces/foo/refs/heads/master` is+omitted from the advertisements. If `uploadpack.allowRefInWant` is set,+`upload-pack` will treat `want-ref refs/heads/master` in a protocol v2+`fetch` command as if `refs/heads/master` was unknown. Note, however, that+`receive-pack` will still advertise the object id `refs/heads/master` is+pointing to, but will conceil the name of the ref. In order to match refs+before stripping, add a `^` in front of the ref name. If you combine `!` and+`^`, `!` must be specified first. + Even if you hide refs, a client may still be able to steal the target objects via the techniques described in the "SECURITY" section of the
I'd appreciate some feedback on this one before rerolling. Having looked at the
code many times recently, it makes sense to me, but that could be different for
someone with less fresh memory. Thanks!
From: Kim Altintop <hidden> Date: 2021-08-13 06:23:42
Round six:
* simplifies the test helper in 1/3 as per review discussion
* rephrases the doc change in 3/3 to make it read less dense
* 2/3 only updates the tests to use the revised helper
CC'ing Johannes Schindelin as suggested by git-contacts.
Published-As: https://github.com/kim/git/tree/ka/namespaced-want-ref-v6
Kim Altintop (3):
t5730: introduce fetch command helper
upload-pack.c: treat want-ref relative to namespace
docs: clarify the interaction of transfer.hideRefs and namespaces
Documentation/config/transfer.txt | 14 +-
t/t5703-upload-pack-ref-in-want.sh | 208 ++++++++++++++++++++++++-----
upload-pack.c | 18 ++-
3 files changed, 192 insertions(+), 48 deletions(-)
--
2.32.0
From: Kim Altintop <hidden> Date: 2021-08-13 06:23:53
Assembling a "raw" fetch command to be fed directly to "test-tool serve-v2"
is extracted into a test helper.
Suggested-by: Junio C Hamano <redacted>
Signed-off-by: Kim Altintop <redacted>
---
t/t5703-upload-pack-ref-in-want.sh | 73 +++++++++++++++---------------
1 file changed, 37 insertions(+), 36 deletions(-)
@@ -144,16 +154,12 @@ test_expect_success 'mix want and want-ref' 'EOFgitrev-parseef>expected_commits&&-test-toolpkt-linepack>in<<-EOF&&-$(write_commandfetch)-0001-no-progress+write_fetch_command>pkt<<-EOF&&want-refrefs/heads/mainwant$(gitrev-parsee)have$(gitrev-parsea)-done-0000EOF+test-toolpkt-linepack<pkt>in&&test-toolserve-v2--stateless-rpc>out<in&&check_output
@@ -166,16 +172,11 @@ test_expect_success 'want-ref with ref we already have commit for' 'EOF>expected_commits&&-oid=$(gitrev-parsec)&&-test-toolpkt-linepack>in<<-EOF&&-$(write_commandfetch)-0001-no-progress+write_fetch_command>pkt<<-EOF&&want-refrefs/heads/o/foo-have$oid-done-0000+have$(gitrev-parsec)EOF+test-toolpkt-linepack<pkt>in&&test-toolserve-v2--stateless-rpc>out<in&&check_output--
From: Kim Altintop <hidden> Date: 2021-08-13 06:24:00
When 'upload-pack' runs within the context of a git namespace, treat any
'want-ref' lines the client sends as relative to that namespace.
Also check if the wanted ref is hidden via 'hideRefs'. If it is hidden,
respond with an error as if the ref didn't exist.
Helped-by: Jonathan Tan [off-list ref]
Signed-off-by: Kim Altintop <redacted>
---
t/t5703-upload-pack-ref-in-want.sh | 135 +++++++++++++++++++++++++++++
upload-pack.c | 18 ++--
2 files changed, 146 insertions(+), 7 deletions(-)
From: Kim Altintop <hidden> Date: 2021-08-13 06:24:01
Expand the section about namespaces in the documentation of
`transfer.hideRefs` to point out the subtle differences between
`upload-pack` and `receive-pack`.
ffcfb68176 (upload-pack.c: treat want-ref relative to namespace,
2021-07-30) taught `upload-pack` to reject `want-ref`s for hidden refs,
which is now mentioned. It is clarified that at no point the name of a
hidden ref is revealed, but the object id it points to may.
Signed-off-by: Kim Altintop <redacted>
---
Documentation/config/transfer.txt | 14 +++++++++-----
1 file changed, 9 insertions(+), 5 deletions(-)
@@ -52,13 +52,17 @@ If you have multiple hideRefs values, later entries override earlier ones (and entries in more-specific config files override less-specific ones). + If a namespace is in use, the namespace prefix is stripped from each-reference before it is matched against `transfer.hiderefs` patterns.+reference before it is matched against `transfer.hiderefs` patterns. In+order to match refs before stripping, add a `^` in front of the ref name. If+you combine `!` and `^`, `!` must be specified first.++ For example, if `refs/heads/master` is specified in `transfer.hideRefs` and the current namespace is `foo`, then `refs/namespaces/foo/refs/heads/master`-is omitted from the advertisements but `refs/heads/master` and-`refs/namespaces/bar/refs/heads/master` are still advertised as so-called-"have" lines. In order to match refs before stripping, add a `^` in front of-the ref name. If you combine `!` and `^`, `!` must be specified first.+is omitted from the advertisements. If `uploadpack.allowRefInWant` is set,+`upload-pack` will treat `want-ref refs/heads/master` in a protocol v2+`fetch` command as if `refs/namespaces/foo/refs/heads/master` did not exist.+`receive-pack`, on the other hand, will still advertise the object id the+ref is pointing to without mentioning its name (a so-called ".have" line). + Even if you hide refs, a client may still be able to steal the target objects via the techniques described in the "SECURITY" section of the--
From: Johannes Schindelin <hidden> Date: 2021-08-14 21:56:03
Hi Kim,
On Fri, 13 Aug 2021, Kim Altintop wrote:
CC'ing Johannes Schindelin as suggested by git-contacts.
`git-contacts` wouldn't know that there are better experts on the
namespace matter.
My only comment is that I would find the diff to `upload-pack.c` much
easier to parse if the `arg` variable hadn't been renamed.
Ciao,
Johannes
From: Kim Altintop <hidden> Date: 2021-08-15 19:35:19
Hi Johannes,
thanks for your response. This is my very first patch to git.git, and a lot of
it is learning about the conventions and expectations towards contributors, as
well as getting familiar with the tooling while trying to avoid silly mistakes.
So please bear with me.
On Sat, 14 Aug 2021 Johannes Schindelin wrote:
quoted
CC'ing Johannes Schindelin as suggested by git-contacts.
`git-contacts` wouldn't know that there are better experts on the
namespace matter.
I can see now that this could come across weird. I should've written:
"CC'ing Johannes Schindelin, who started to turn up in `git-contacts` output,
although I couldn't quite infer why. I haven't received any feedback about the
documentation change yet, and didn't have much success trying to find reviewers
by inspecting the history (parts of the file where moved). I am assuming that
`git-contacts` is better than me at this, and Johannes' name shows up because of
touching the documentation. Johannes: feel free to ignore if this assumption is
wrong."
With this said, if you have any suggestions about finding reviewers for specific
parts of a patch, or who are the experts on a more cross-cutting topic, I would
appreciate if you'd share them!
My only comment is that I would find the diff to `upload-pack.c` much
easier to parse if the `arg` variable hadn't been renamed.
Can you explain why? Just because the diff would be smaller? I can see that in a
larger patch it might have been preferable to put the rename into a separate
commit, but in a hunk-sized change it seemed fine. It is also that this
particular naming ("refname_nons") is used in other places in upload-pack.c, so
it seemed obvious that, if I introduce namespace handling where it was
previously missing, the terminology (if you will) should be the same.
From you comment, it seems like the proposer of a patch should assume that the
reviewers only look at the diff as sent in the email, and not any context.
Junio's response suggests something else, but I guess it's fair that if someone
feels like they got CC'ed by mistake, they're not going to spend too much time.
So my question from above stands: are there better ways to find the right people
to CC, especially for newbies?
Thanks,
Kim
From: Johannes Schindelin <hidden> Date: 2021-08-16 12:40:45
Hi Kim,
On Sun, 15 Aug 2021, Kim Altintop wrote:
On Sat, 14 Aug 2021 Johannes Schindelin wrote:
quoted
My only comment is that I would find the diff to `upload-pack.c` much
easier to parse if the `arg` variable hadn't been renamed.
Can you explain why?
Yes. I prefer patches to be really obvious. That way, it is really easy to
spot bugs.
In this instance, the same patch that introduces a conditional block
_also_ renames an involved variable.
To satisfy myself that the patch does what is intended, I therefore have
to virtually split the patch into the rename part and the
added-conditional-block part.
It would be easier for me if the modifications were presented as two
separate patches. And I could imagine that I am not alone in this: you
yourself might also have an easier time looking at the commits in six
months from now if those concerns are separated into their own commit.
Just because the diff would be smaller? I can see that in a larger patch
it might have been preferable to put the rename into a separate commit,
but in a hunk-sized change it seemed fine. It is also that this
particular naming ("refname_nons") is used in other places in
upload-pack.c, so it seemed obvious that, if I introduce namespace
handling where it was previously missing, the terminology (if you will)
should be the same.
From you comment, it seems like the proposer of a patch should assume
that the reviewers only look at the diff as sent in the email, and not
any context. Junio's response suggests something else, but I guess it's
fair that if someone feels like they got CC'ed by mistake, they're not
going to spend too much time.
In this instance, I indeed did not spend more time than on reviewing the
patch, simply because I am (currently, at least) not all that familiar
with the `upload-pack.c` machinery. I probably touched it in the past, but
for the moment, all I can comment on is the shape of the patch series,
which is what I did.
So my question from above stands: are there better ways to find the
right people to CC, especially for newbies?
When I look for reviewers in projects other than the ones where I know the
usual reviewers' special areas of interest, I like to pick a function at
the center of my contribution, then look at `git log -L
:<function>:<file>` and try to figure out who was the last person to
implement non-stylistic changes on that function. This has worked
relatively well for me, in the past. Maybe it can help you here, too?
Ciao,
Dscho