Re: Re* [PATCH 8/9] fast-export: respect the possibly-overridden default branch name

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Jun 13, 2020, 14:44 UTC
Message-ID: <nycvar.QRO.7.76.6.2006130703340.56@tvgsbejvaqbjf.bet>
In-Reply-To: <xmqqy2os2u55.fsf@gitster.c.googlers.com>

Hi Junio,

On Fri, 12 Jun 2020, Junio C Hamano wrote:

Show 23 quoted lines

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>
> > I just realized that the comment above reads:
> >
> >         /*
> >          * We also leave "master" as a special case, since it does not reveal
> >          * anything interesting.
> >          */
> >
> >
> > Obviously, we need to change that comment here because we do not leave the
> > name unchanged. How about this?
> >
> >         /*
> >          * We special-case the main branch, anonymizing it to `ref0`.
> >          */
>
> If you are going to update it, why not make it useful?
>
> I complained number of times during the discussion that the original
> comment explains why leaving 'master' as-is does not reveal anything
> useful to adversaries but does not justify what the code attempts to
> achieve by special casing 'master' in the first place.

True. Sorry about forgetting that when adjusting the code comment.

In my defense, I am/was much more worried about transmogrifying the patch series to reflect the separation between `init.defaultBranch` and `core.mainBranch` and the associated fall-out (I highly doubt that the range-diff between v1 and v2 will be useful...).

Show 9 quoted lines

> It is not an improvement to literally adjust that inadequate comment
> to the new world order to just parrot what the code already says
> without explaining why it does so.
>
> 	/*
> 	 * Anonymize the name used for the primary branch in this
> 	 * repository, but reserve `ref0` for it, so that it can
> 	 * be identified among other refs in the output.
> 	 */

That is indeed an improvement, thank you so much.

Show 10 quoted lines

> is the minimum I would expect before calling it an improvement.  We
> could add
>
> 	It is often `main` for new repositories (and `master` for
> 	aged ones) and such well-known names may not need
> 	anonymizing, but it could be configured to use a secret word
> 	that the user may not want to reveal.
>
> at the end to explain the motivation behind anonymizing even more,
> if we wanted to.

Maybe we add that to the comment in the patch that teaches `fast-export` about `core.mainBranch`? Yeah, I think I like that idea best.

> Now, "so that ..." part is totally a fabrication based on my best
> guess.  I do not know what the original author was thinking when the
> decision to leave the master as-is was made.

This comment comes from a8722750985 (teach fast-export an --anonymize option, 2014-08-27), and I agree that there is no explicit explanation why the main branch is special-cased.

However, I think that your guess is a good one: it might be an interesting aspect to identify the commits from the main branch, without necessarily needing to know the actual name of said branch, e.g. to reproduce a reported issue.

Ciao, Dscho