Hi Eric,
On Fri, 19 Jan 2018, Eric Sunshine wrote:
> On Thu, Jan 18, 2018 at 10:35 AM, Johannes Schindelin
> <johannes.schinde...@gmx.de> wrote:
>
> > structure (similar in spirit to --preserve-merges, but with a
> > substantially less-broken design).
> > [...]
> > Signed-off-by: Johannes Schindelin <johannes.schinde...@gmx.de>
> > ---
> > diff --git a/sequencer.c b/sequencer.c
> > @@ -2785,6 +2787,335 @@ void append_signoff(struct strbuf *msgbuf, int
> > ignore_footer, unsigned flag)
> > +static const char *label_oid(struct object_id *oid, const char *label,
> > + struct label_state *state)
> > +{
> > + [...]
> > + } else if (((len = strlen(label)) == GIT_SHA1_RAWSZ &&
> > + !get_oid_hex(label, &dummy)) ||
> > + hashmap_get_from_hash(&state->labels,
> > + strihash(label), label)) {
> > + /*
> > + * If the label already exists, or if the label is a valid
> > full
> > + * OID, we append a dash and a number to make it unique.
> > + */
> > + [...]
> > + for (i = 2; ; i++) {
>
> Why '2'? Is there some non-obvious significance to this value?
I personally found it irritating to have labels "sequencer",
"sequencer-1". It sounds *wrong* to have a "-1". Because it is the second
label referring to the term "sequencer". So if there are two labels that
both want to be named "sequencer", the first one wins, and the second one
will be called "sequencer-2".
Hence the 2.
> > +static int make_script_with_merges(struct pretty_print_context *pp,
> > + struct rev_info *revs, FILE *out,
> > + unsigned flags)
> > +{
> > + [...]
> > + is_octopus = to_merge && to_merge->next;
> > +
> > + if (is_octopus)
> > + BUG("Octopus merges not yet supported");
>
> Is this a situation which the end-user can trigger by specifying a
> merge with more than two parents? If so, shouldn't this be just a
> normal error message rather than a (developer) bug message? Or, am I
> misunderstanding?
You are misunderstanding.
This is just a place-holder here. The patches to introduce support for
octopus merges are already written. They are lined up after this here
patch series, is all.
As such, please do not occupy your mind on the specifics or even the
upper-case of the "Octopus". This line is here only as a hint for the
reviewer that this is not yet implemented. And BUG(...) was chosen because
that way, we are not even tempted to waste the time of translators.
Speaking of wasting time... let's move on to further interesting code
reviews.
Ciao,
Dscho
