git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [PATCH v3 3/8] upload/receive-pack: allow hiding ref hierarchies

From
Jeff King <peff@peff.net>
Date
Feb 6, 2013, 11:31 UTC
Message-ID
<20130206113112.GB5267@sigill.intra.peff.net>
In-Reply-To
<7vwqumvk76.fsf@alter.siamese.dyndns.org>
On Tue, Feb 05, 2013 at 07:45:01AM -0800, Junio C Hamano wrote:
Show 6 quoted lines
> > In the earlier review, I mentioned making this per-service, but I see
> > that is not the case here. Do you have an argument against doing so?
> 
> Perhaps then I misunderstood your intention.  By reminding me of the
> receive-pack side, I thought you were hinting to unify these two
> into one, which I did.  There is no argument against it.

What I meant was that there should be transfer.hiderefs, and an individual {receive,uploadpack}.hiderefs, similar to the way we have transfer.unpacklimit. That makes the easy case (hiding the refs completely) easy, but leaves the flexibility for more.

Like this:
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index a8248d9..131c163 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -59,7 +59,7 @@ static int receive_pack_config(const char *var, const char *value, void *cb)
 
 static int receive_pack_config(const char *var, const char *value, void *cb)
 {
-	int status = parse_hide_refs_config(var, value, cb);
+	int status = parse_hide_refs_config(var, value, "receive");
 
 	if (status)
 		return status;
diff --git a/refs.c b/refs.c
index e3574ca..9bfea58 100644
--- a/refs.c
+++ b/refs.c
@@ -2560,9 +2560,13 @@ int parse_hide_refs_config(const char *var, const char *value, void *unused)
 
 static struct string_list *hide_refs;
 
-int parse_hide_refs_config(const char *var, const char *value, void *unused)
+int parse_hide_refs_config(const char *var, const char *value, void *vsection)
 {
-	if (!strcmp("transfer.hiderefs", var)) {
+	const char *section = vsection;
+
+	if (!strcmp("transfer.hiderefs", var) ||
+	    (!prefixcmp(var, section) &&
+	     !strcmp(var + strlen(section), ".hiderefs"))) {
 		char *ref;
 		int len;
 
diff --git a/upload-pack.c b/upload-pack.c
index 37977e2..c0390af 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -794,7 +794,7 @@ static int upload_pack_config(const char *var, const char *value, void *unused)
 {
 	if (!strcmp("uploadpack.allowtipsha1inwant", var))
 		allow_tip_sha1_in_want = git_config_bool(var, value);
-	return parse_hide_refs_config(var, value, unused);
+	return parse_hide_refs_config(var, value, "uploadpack");
 }
 
 int main(int argc, char **argv)


As an aside, I wonder if there is any point to the void pointer
parameter of parse_hide_refs_config. It is not used as a git_config
callback anywhere.

> > And I
> > have not seen complaints about the current system.
> 
> Immediately after I added github to the set of places I push into,
> which I think is long before you joined GitHub, I noticed that _my_
> repository gets contaminated by second rate commits called pull
> requests, and I may even have complained, but most likely I didn't,
> as I could easily tell that, even though I know it is _not_ the only
> way, nor even the best way [*1*], to implement the GitHub's pull
> request workflow, I perfectly well understood that it would be the
> most expedient way for GitHub folks to implement this feature.
> 
> I think you should take lack of complaints with a huge grain of
> salt.  It does not suggest much.

Sure, I do not pretend that nobody cares. But it is certainly not a
pressing issue, or there would probably be more outcry. And we must also
weigh it against the silent majority that are perfectly happy with the
status quo, that lets them fetch refs/pull/* as any other ref.

In your case, I really think the problem is less that you have a problem
with PR refs in the repository, and more that you do not care about the
pull request feature at all. To you it is useless noise, both in the
repo and on the web. Your arguments about provenance could apply equally
well to PRs accessible via the web interface.

I think the refs/ clutter is only an issue if you want to do mirroring,
and then you have an obvious conflict: did the fetcher want to mirror
everything, including refs/pull, or do they consider that to be clutter?
Only the client knows, which is why I think refspecs are the right place
to deal with clutter (the fact that we cannot say "everything except
refs/pull/*" is a weakness in our refspecs).

> *1* From the ownership point of view, objects that are only
> reachable from these refs/pull/* refs do *not* belong to the
> requestee, until the requestee chooses to accept the changes.
> 
> A malicious requestor can fork your repository, add an objectionable
> blob to it, and throw a pull request at you.  GitHub shows that the
> blob now belongs to your repository, so the requestor turns around
> and file a DMCA takedown complaint against your repository.  A
> clueful judge would then agree with the complaint after running a
> "clone --mirror" and seeing the blob in your repository.  Oops?

I don't think this is a problem in practice. DMCA notices do not go to
the repository owner; they go to GitHub. And as far as I know, our
support staff deals with them on a case by case basis (and knows what a
pull request is, and who is responsible for the content in question). It
is not like they see a report of something in refs/pull and lock down
the parent repository; they can see where the request came from and deal
with it appropriately.

But again, such a notice could just as easily come from the list of open
PRs against your repo in the web interface.

> A funny thing is that you cannot "push :refs/pull/1/head" to remove
> it anymore (I think in the early days, I took them out by doing this
> a few times, but I may be misremembering),

We block updates to them explicitly in a hook; it looks like that went
in around mid-2011.

> The e-mail sent to you to let you know about outstanding pull
> requests and the web UI could just point at that forked repository,
> not your own (you also could choose to leave the outging pull
> requests in the requestor's repository, but that is only OK if you
> do not worry about (1) a requestor sending a pull request, then
> updating the branch the pull request talks about later, to trick you
> with bait-and-switch, or (2) a requestor sending a pull request,
> thinks he is done with the topic and removes the repository).

Yes, point (2) is the main reason they are not simply attached to the
sender's repository.

-Peff
Previous: Junio C HamanoNext: Junio C Hamano
Message 7 of 54 in “Hiding refs”
  1. 0/8 Hiding refsJunio C Hamano, Jan 30, 2013
  2. 1/8 upload-pack: share more codeJunio C Hamano, Jan 30, 2013
  3. 2/8 upload-pack: simplify request validationJunio C Hamano, Jan 30, 2013
  4. 3/8 upload/receive-pack: allow hiding ref hierarchiesJunio C Hamano, Jan 30, 2013
  5. Jeff KingFeb 5, 2013
  6. Junio C HamanoFeb 5, 2013
  7. Jeff KingFeb 6, 2013
  8. Junio C HamanoFeb 6, 2013
  9. 4/8 parse_fetch_refspec(): clarify the codeflow a bitJunio C Hamano, Jan 30, 2013
  10. 5/8 fetch: use struct ref to represent refs to be fetchedJunio C Hamano, Jan 30, 2013
  11. 6/8 upload-pack: optionally allow fetching from the tips of hidden refsJunio C Hamano, Jan 30, 2013
  12. 7/8 fetch: fetch objects by their exact SHA-1 object namesJunio C Hamano, Jan 30, 2013
  13. Jeff KingFeb 5, 2013
  14. Jeff KingFeb 5, 2013
  15. Junio C HamanoFeb 5, 2013
  16. 8/8 WIP: receive.allowupdatestohiddenJunio C Hamano, Jan 30, 2013
  17. Michael HaggertyFeb 5, 2013
  18. Jonathan NiederFeb 5, 2013
  19. Michael HaggertyFeb 5, 2013
  20. Junio C HamanoFeb 5, 2013
  21. Duy NguyenFeb 6, 2013
  22. Junio C HamanoFeb 6, 2013
  23. Jonathan NiederFeb 6, 2013
  24. Michael HaggertyFeb 6, 2013
  25. Junio C HamanoFeb 6, 2013
  26. Ævar Arnfjörð BjarmasonFeb 6, 2013
  27. Junio C HamanoFeb 7, 2013
  28. Jeff KingFeb 7, 2013
  29. Ævar Arnfjörð BjarmasonFeb 7, 2013
  30. Junio C HamanoFeb 7, 2013
  31. Duy NguyenFeb 23, 2014
  32. Jeff KingMar 11, 2014
  33. Junio C HamanoMar 11, 2014
  34. Jeff KingMar 11, 2014
  35. Junio C HamanoMar 11, 2014
  36. Jeff KingMar 11, 2014
  37. Duy NguyenMar 14, 2014
  38. Shawn PearceMar 14, 2014
  39. Duy NguyenMar 14, 2014
  40. Shawn PearceMar 15, 2014
  41. Jeff KingMar 18, 2014
  42. Duy NguyenMar 18, 2014
  43. Duy NguyenMar 18, 2014
  44. Duy NguyenMar 15, 2014
  45. Jeff KingMar 18, 2014
  46. Jeff KingFeb 6, 2013
  47. Junio C HamanoFeb 5, 2013
  48. Junio C HamanoFeb 5, 2013
  49. Michael HaggertyFeb 6, 2013
  50. Jonathan NiederFeb 6, 2013
  51. Michael HaggertyFeb 6, 2013
  52. Jed BrownFeb 7, 2013
  53. Junio C HamanoFeb 9, 2013
  54. Jed BrownFeb 10, 2013

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.