Re: [PATCH 4/8] rebase-helper --make-script: introduce a flag to recreate merges

> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
> 
> > structure (similar in spirit to --preserve-merges, but with a
> > substantially less-broken design).
> > ...
> > @@ -2785,6 +2787,335 @@ void append_signoff(struct strbuf *msgbuf, int ignore_footer, unsigned flag)
> >  	strbuf_release(&sob);
> >  }
> >  
> > +struct labels_entry {
> > +	struct hashmap_entry entry;
> > +	char label[FLEX_ARRAY];
> > +};
> > +
> > +static int labels_cmp(const void *fndata, const struct labels_entry *a,
> > +		      const struct labels_entry *b, const void *key)
> > +{
> > +	return key ? strcmp(a->label, key) : strcmp(a->label, b->label);
> > +}
> 
> label_oid() accesses state->labels hash using strihash() as the hash
> function, but the final comparison between the entries in the same
> hash buckets are done with case sensitivity.  It is unclear to me if
> that is what was intended, and why.

> > +struct string_entry {
> > +	struct oidmap_entry entry;
> > +	char string[FLEX_ARRAY];
> > +};
> > +
> > +struct label_state {
> > +	struct oidmap commit2label;
> > +	struct hashmap labels;
> > +	struct strbuf buf;
> > +};
> > +
> > +static const char *label_oid(struct object_id *oid, const char *label,
> > +			     struct label_state *state)
> > +{
> > +	struct labels_entry *labels_entry;
> > +	struct string_entry *string_entry;
> > +	struct object_id dummy;
> > +	size_t len;
> > +	int i;
> > +
> > +	string_entry = oidmap_get(&state->commit2label, oid);
> > +	if (string_entry)
> > +		return string_entry->string;
> > +
> > +	/*
> > +	 * For "uninteresting" commits, i.e. commits that are not to be
> > +	 * rebased, and which can therefore not be labeled, we use a unique
> > +	 * abbreviation of the commit name. This is slightly more complicated
> > +	 * than calling find_unique_abbrev() because we also need to make
> > +	 * sure that the abbreviation does not conflict with any other
> > +	 * label.
> > +	 *
> > +	 * We disallow "interesting" commits to be labeled by a string that
> > +	 * is a valid full-length hash, to ensure that we always can find an
> > +	 * abbreviation for any uninteresting commit's names that does not
> > +	 * clash with any other label.
> > +	 */
> > +	if (!label) {
> > +		char *p;
> > +
> > +		strbuf_reset(&state->buf);
> > +		strbuf_grow(&state->buf, GIT_SHA1_HEXSZ);
> > +		label = p = state->buf.buf;
> > +
> > +		find_unique_abbrev_r(p, oid->hash, default_abbrev);
> > +
> > +		/*
> > +		 * We may need to extend the abbreviated hash so that there is
> > +		 * no conflicting label.
> > +		 */
> > +		if (hashmap_get_from_hash(&state->labels, strihash(p), p)) {
> > +			size_t i = strlen(p) + 1;
> > +
> > +			oid_to_hex_r(p, oid);
> > +			for (; i < GIT_SHA1_HEXSZ; i++) {
> > +				char save = p[i];
> > +				p[i] = '\0';
> > +				if (!hashmap_get_from_hash(&state->labels,
> > +							   strihash(p), p))
> > +					break;
> > +				p[i] = save;
> > +			}
> > +		}
> 
> If oid->hash required full 40-hex to disambiguate, then
> find-unique-abbrev would give 40-hex and we'd want the same "-<num>"
> suffix technique employed below to make it consistently unique.  I
> wonder if organizing the function this way ...
> 
> 	if (!label)
> 		label = oid-to-hex(oid);
> 
> 	if (label already exists or full oid) {
> 		make it unambiguous;
> 	}

> > +static int make_script_with_merges(struct pretty_print_context *pp,
> > +				   struct rev_info *revs, FILE *out,
> > +				   unsigned flags)
> > +{
> > + ...
> > +	int abbr = flags & TODO_LIST_ABBREVIATE_CMDS;
> > +	const char *p = abbr ? "p" : "pick", *l = abbr ? "l" : "label",
> > +		 *t = abbr ? "t" : "reset", *b = abbr ? "b" : "bud",
> > +		 *m = abbr ? "m" : "merge";
> 
> It would be easier to understand if these short named variables are
> reserved only for temporary use, not as constants.  It is not too
> much to spell 
> 
> 	fprintf(out, "%s onto\n", cmd_label);
> 
> than
> 
> 	fprintf(out, "%s onto\n", l);
> 
> and would save readers from head-scratching, wondering where the
> last assignment to variable "l" is.

> > +	oidmap_init(&commit2todo, 0);
> > +	oidmap_init(&state.commit2label, 0);
> > +	hashmap_init(&state.labels, (hashmap_cmp_fn) labels_cmp, NULL, 0);
> > +	strbuf_init(&state.buf, 32);
> > +
> > +	if (revs->cmdline.nr && (revs->cmdline.rev[0].flags & BOTTOM)) {
> > +		struct object_id *oid = &revs->cmdline.rev[0].item->oid;
> > +		FLEX_ALLOC_STR(entry, string, "onto");
> > +		oidcpy(&entry->entry.oid, oid);
> > +		oidmap_put(&state.commit2label, entry);
> > +	}
> > +
> > +	/*
> > +	 * First phase:
> > +	 * - get onelines for all commits
> > +	 * - gather all branch tips (i.e. 2nd or later parents of merges)
> > +	 * - label all branch tips
> > +	 */
> 
> When an early part of a branch is merged and then the remaining part
> of the same branch is merged again, "branch tip" and "2nd or later
> parents of merges" would become different concepts.  The 2nd parent
> of an early merge is not among the branch tips.
> 
> For the purpose of the "recreate the topology" algorithm, I am
> imagining that you would need not just the tips but all the 2nd and
> subsequent parents of merges, and my quick skimming tells me that
> the following code grabs them correctly.