Re: [PATCH 2/3] for-each-ref: let upstream/push optionally report merge name.

From: Junio C Hamano <gitster@pobox.com>
Date: Oct 4, 2017, 11:41 UTC
Message-ID: <xmqqvajv9m2k.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <xmqq4lrfb7jk.fsf@gitster.mtv.corp.google.com>

Junio C Hamano <gitster@pobox.com> writes:

Show 11 quoted lines

>> +		if (explicit)
>> +			*s = xstrdup(merge);
>> +		else
>> +			*s = "";
>
> Here is the same "who are we trying to help---users who want to know
> where their push goes, or users who are debugging where the push
> destination is defined?" question.  I do not have a strong opinion
> either way, but I think giving the end result with fallback (i.e.
> not nullifying when the result is not explicit) may be easier to use
> by "push" users.

Now after thinking about it a bit more, I actually have a moderate preference to doing it the way your patches do. With programatic %(if)%(else)%(end) support we acquired recently, the fallback can be coded in the --format=... language if the user wanted to using the "internal fallbacks, explicit==0, are ignored" behaviour that are implemented by these two patches. The reverse is not true.

I think the remaining points from my reviews are:

 - It would be better to compute precomputable stuff when used atoms
   are parsed, instead of running starts_with() in these functions;

 - We want to make sure there is no existing multi-word modifiers
   (in which case we can safely declare "multi-word" is the way we
   spell them from now on).  Or if there are existing ones, they
   already spell themselves as "multi-word".

   I have nothing against "remote-name"; I just want to make sure we
   are not making things inconsistent.  If there are only few (but
   non zero number of) multi-word modifiers that are not spelled
   "multi-word", as long as they are only few and their spelling are
   inferiour (e.g. concatenatedwords is much worse than
   concatenated-words), we could still declare "multi-word" is the
   right way to spell them going forward, declare that we will give
   them synonyms and deprecate the bad spelling out over time, and
   leave that plan as #leftover bits thing (i.e. not doing the
   deprecation of these other modifiers as part of this series.

   The only thing I want to happen in the scope of _this_ series, as
   due diligence, is to make sure we are happy with "multi-word",
   and also to know if we need a follow-up work (just yes/no,
   possibly with plans, but no actual work yet).

Thanks.