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

Re: [PATCH/RFC/GSoC 04/17] builtin-rebase: parse rebase arguments into a common rebase_options struct

From
Johannes Schindelin <johannes.schindelin@gmx.de>
Date
Mar 15, 2016, 10:54 UTC
Message-ID
<alpine.DEB.2.20.1603151134230.4690@virtualbox>
In-Reply-To
<1457779597-6918-5-git-send-email-pyokagan@gmail.com>
Hi Paul,
On Sat, 12 Mar 2016, Paul Tan wrote:
Show 27 quoted lines
> diff --git a/builtin/rebase.c b/builtin/rebase.c
> index 04cc1bd..40176ca 100644
> --- a/builtin/rebase.c
> +++ b/builtin/rebase.c
> @@ -4,6 +4,112 @@
>  #include "cache.h"
>  #include "builtin.h"
>  #include "parse-options.h"
> +#include "rebase-common.h"
> +#include "remote.h"
> +#include "branch.h"
> +#include "refs.h"
> +
> +/**
> + * Used by get_curr_branch_upstream_name() as a for_each_remote() callback to
> + * retrieve the name of the remote if the repository only has one remote.
> + */
> +static int get_only_remote(struct remote *remote, void *cb_data)
> +{
> +	const char **remote_name = cb_data;
> +
> +	if (*remote_name)
> +		return -1;
> +
> +	*remote_name = remote->name;
> +	return 0;
> +}

This function gets only the remote's name, not only the remote. And this is not really a functionality specific to rebase, is it?

Show 15 quoted lines
> +const char *get_curr_branch_upstream_name(void)
> +{
> +	const char *upstream_name;
> +	struct branch *curr_branch;
> +
> +	curr_branch = branch_get("HEAD");
> +	if (!curr_branch) {
> +		fprintf_ln(stderr, _("You are not currently on a branch."));
> +		fprintf_ln(stderr, _("Please specify which branch you want to rebase against."));
> +		fprintf_ln(stderr, _("See git-rebase(1) for details."));
> +		fprintf(stderr, "\n");
> +		fprintf_ln(stderr, "    git rebase <branch>");
> +		fprintf(stderr, "\n");
> +		exit(1);
> +	}

Urgh. Elswhere we have _("Blabla\nBlublub\n") constructs, which is already a little bit ugly, but this mix of fprintf_ln() and fprintf() together with adding a whopping 3 strings (for the price of 1) for the translators (and missing one...) is too ugly for my taste.

Also, there is a horrible, horrible, horrible exit(1) there. I know, you put this into builtin/ and so we assume it is okay to just exit() left and right, but *why*? Is this not a function we might want to reuse elsewhere? As such, it should live in remote.[ch], take a "hint" parameter in case there is no current branch (and BTW "HEAD" should not be hard-coded to begin with, but instead be another parameter), and it should return -1 on error, not exit.

Show 22 quoted lines
> +
> +	upstream_name = branch_get_upstream(curr_branch, NULL);
> +	if (!upstream_name) {
> +		const char *remote_name = NULL;
> +
> +		if (for_each_remote(get_only_remote, &remote_name) || !remote_name)
> +			remote_name = "<remote>";
> +
> +		fprintf_ln(stderr, _("There is no tracking information for the current branch."));
> +		fprintf_ln(stderr, _("Please specify which branch you want to rebase against."));
> +		fprintf_ln(stderr, _("See git-rebase(1) for details."));
> +		fprintf(stderr, "\n");
> +		fprintf_ln(stderr, "    git rebase <branch>");
> +		fprintf(stderr, "\n");
> +		fprintf_ln(stderr, _("If you wish to set tracking information for this branch you can do so with:"));
> +		fprintf(stderr, "\n");
> +		fprintf_ln(stderr, _("If you wish to set tracking information for this branch you can do so with:\n"
> +		"\n"
> +		"    git branch --set-upstream-to=%s/<branch> %s\n"),
> +		remote_name, curr_branch->name);
> +		exit(1);
> +	}

Same here. The rebase-specific part of the hint should be a parameter, the thing should not die at all, and it really wants to live in remote.[ch].

Show 6 quoted lines
> +/**
> + * Given the --onto <name>, return the onto hash
> + */
> +static void get_onto_oid(const char *_onto_name, struct object_id *onto)
> +{
> +	char *onto_name = xstrdup(_onto_name);

By convention, variable names starting with an underscore are reserved for use by the standard library.

Show 39 quoted lines
> +	struct commit *onto_commit;
> +	char *dotdot;
> +
> +	dotdot = strstr(onto_name, "...");
> +	if (dotdot) {
> +		const char *left = onto_name;
> +		const char *right = dotdot + 3;
> +		struct commit *left_commit, *right_commit;
> +		struct commit_list *merge_bases;
> +
> +		*dotdot = 0;
> +		if (!*left)
> +			left = "HEAD";
> +		if (!*right)
> +			right = "HEAD";
> +
> +		/* git merge-base --all $left $right */
> +		left_commit = lookup_commit_reference_by_name(left);
> +		right_commit = lookup_commit_reference_by_name(right);
> +		if (!left_commit || !right_commit)
> +			die(_("%s: there is no merge base"), _onto_name);
> +
> +		merge_bases = get_merge_bases(left_commit, right_commit);
> +		if (merge_bases && merge_bases->next)
> +			die(_("%s: there are more than one merge bases"), _onto_name);
> +		else if (!merge_bases)
> +			die(_("%s: there is no merge base"), _onto_name);
> +
> +		onto_commit = merge_bases->item;
> +		free_commit_list(merge_bases);
> +	} else {
> +		onto_commit = lookup_commit_reference_by_name(onto_name);
> +		if (!onto_commit)
> +			die(_("invalid upstream %s"), onto_name);
> +	}
> +
> +	free(onto_name);
> +	oidcpy(onto, &onto_commit->object.oid);
> +}

A lot of this looks *awfully* like the parameters we throw at rev-list (or for that matter, log). Why can't we reuse that machinery?

Show 38 quoted lines
> @@ -12,20 +118,96 @@ static int git_rebase_config(const char *k, const char *v, void *cb)
>  
>  int cmd_rebase(int argc, const char **argv, const char *prefix)
>  {
> +	struct rebase_options rebase_opts;
> +	const char *onto_name = NULL;
> +	const char *branch_name;
> +
>  	const char * const usage[] = {
> -		N_("git rebase [options]"),
> +		N_("git rebase [options] [--onto <newbase>] [<upstream>] [<branch>]"),
>  		NULL
>  	};
>  	struct option options[] = {
> +		OPT_GROUP(N_("Available options are")),
> +		OPT_STRING(0, "onto", &onto_name, NULL,
> +			N_("rebase onto given branch instead of upstream")),
>  		OPT_END()
>  	};
>  
>  	git_config(git_rebase_config, NULL);
> +	rebase_options_init(&rebase_opts);
> +	rebase_opts.resolvemsg = _("\nWhen you have resolved this problem, run \"git rebase --continue\".\n"
> +			"If you prefer to skip this patch, run \"git rebase --skip\" instead.\n"
> +			"To check out the original branch and stop rebasing, run \"git rebase --abort\".");
>  
>  	argc = parse_options(argc, argv, prefix, options, usage, 0);
>  
>  	if (read_cache_preload(NULL) < 0)
>  		die(_("failed to read the index"));
>  
> +	/*
> +	 * Parse command-line arguments:
> +	 *    rebase [<options>] [<upstream_name>] [<branch_name>]
> +	 */
> +
> +	/* Parse <upstream_name> into rebase_opts.upstream */
> +	{

In Git, unless there are very compelling reasons, we avoid non-conditional blocks. Probably you did that to have this local declaration:

> +		const char *upstream_name;

But that declaration can easily live in the cmd_rebase() scope, simplifying the code and being easier on the reader's eyes.

Show 6 quoted lines
> +	/*
> +	 * Parse --onto <onto_name> into rebase_opts.onto and
> +	 * rebase_opts.onto_name
> +	 */
> +	get_onto_oid(onto_name, &rebase_opts.onto);
> +	rebase_opts.onto_name = xstrdup(onto_name);

My, this onto_name() sure gets strdup()ed a lot... Maybe we can avoid that?

Show 6 quoted lines
> +	/*
> +	 * Parse <branch_name> into rebase_opts.orig_head and
> +	 * rebase_opts.orig_refname
> +	 */
> +	branch_name = argv[0];
> +	if (branch_name) {

In Git's source code, we appear to rely on argc instead on argv[argc] being NULL.

Show 14 quoted lines
> +		/* Is branch_name a branch or commit? */
> +		char *ref_name = xstrfmt("refs/heads/%s", branch_name);
> +		struct object_id orig_head_id;
> +
> +		if (!read_ref(ref_name, orig_head_id.hash)) {
> +			rebase_opts.orig_refname = ref_name;
> +			if (get_oid_commit(ref_name, &rebase_opts.orig_head))
> +				die("get_sha1_commit failed");
> +		} else if (!get_oid_commit(branch_name, &rebase_opts.orig_head)) {
> +			rebase_opts.orig_refname = NULL;
> +			free(ref_name);
> +		} else {
> +			die(_("no such branch: %s"), branch_name);
> +		}

Here, ref_name does not get free()d. It lives on as rebase_opts.orig_refname but it gets increasingly fiddly to reason about the correctness of the code.

A better idea would be to leave the responsibility of keeping track completely with the caller, i.e. have the fields of the options struct as const char *. Then you can make the values strbufs as needed and in the case of a builtin that exits anyway, you do not even need to release in the end.

> diff --git a/rebase-common.c b/rebase-common.c

As pointed out elsewhere, it is not a good idea to put stuff used by the rebase into rebase-common.c. Either it is so specific to rebase that it can go into rebase.c, or it is so not specific to rebase that it can go into path.c, wt-status.c, diff.c etc

Ciao, Johannes

Previous: Stefan BellerNext: Paul Tan
Message 16 of 59 in “A barebones git-rebase in C”
  1. 00/17 A barebones git-rebase in CPaul Tan, Mar 12, 2016
  2. 01/17 perf: introduce performance tests for git-rebasePaul Tan, Mar 12, 2016
  3. Johannes SchindelinMar 16, 2016
  4. Paul TanMar 16, 2016
  5. Johannes SchindelinMar 16, 2016
  6. Thomas GummererMar 18, 2016
  7. Johannes SchindelinMar 18, 2016
  8. Thomas GummererMar 20, 2016
  9. Johannes SchindelinMar 21, 2016
  10. 02/17 sha1_name: implement get_oid() and friendsPaul Tan, Mar 12, 2016
  11. 03/17 builtin-rebase: implement skeletal builtin rebasePaul Tan, Mar 12, 2016
  12. Stefan BellerMar 14, 2016
  13. Johannes SchindelinMar 15, 2016
  14. 04/17 builtin-rebase: parse rebase arguments into a common rebase_options structPaul Tan, Mar 12, 2016
  15. Stefan BellerMar 14, 2016
  16. Johannes SchindelinMar 15, 2016
  17. 05/17 rebase-options: implement rebase_options_load() and rebase_options_save()Paul Tan, Mar 12, 2016
  18. Stefan BellerMar 14, 2016
  19. Johannes SchindelinMar 16, 2016
  20. Paul TanMar 16, 2016
  21. Johannes SchindelinMar 16, 2016
  22. Paul TanMar 21, 2016
  23. Paul TanMar 16, 2016
  24. Stefan BellerMar 16, 2016
  25. 06/17 rebase-am: introduce am backend for builtin rebasePaul Tan, Mar 12, 2016
  26. Johannes SchindelinMar 16, 2016
  27. 07/17 rebase-common: implement refresh_and_write_cache()Paul Tan, Mar 12, 2016
  28. Junio C HamanoMar 14, 2016
  29. Paul TanMar 16, 2016
  30. 08/17 rebase-common: let refresh_and_write_cache() take a flags argumentPaul Tan, Mar 12, 2016
  31. 09/17 rebase-common: implement cache_has_unstaged_changes()Paul Tan, Mar 12, 2016
  32. Johannes SchindelinMar 14, 2016
  33. Junio C HamanoMar 14, 2016
  34. Johannes SchindelinMar 15, 2016
  35. Duy NguyenMar 15, 2016
  36. Johannes SchindelinMar 15, 2016
  37. 10/17 rebase-common: implement cache_has_uncommitted_changes()Paul Tan, Mar 12, 2016
  38. 11/17 rebase-merge: introduce merge backend for builtin rebasePaul Tan, Mar 12, 2016
  39. 12/17 rebase-todo: introduce rebase_todo_itemPaul Tan, Mar 12, 2016
  40. Christian CouderMar 14, 2016
  41. Johannes SchindelinMar 14, 2016
  42. Paul TanMar 16, 2016
  43. Johannes SchindelinMar 16, 2016
  44. 13/17 rebase-todo: introduce rebase_todo_listPaul Tan, Mar 12, 2016
  45. 14/17 status: use rebase_todo_listPaul Tan, Mar 12, 2016
  46. 15/17 wrapper: implement append_file()Paul Tan, Mar 12, 2016
  47. 16/17 editor: implement git_sequence_editor() and launch_sequence_editor()Paul Tan, Mar 12, 2016
  48. Johannes SchindelinMar 15, 2016
  49. Paul TanMar 16, 2016
  50. Johannes SchindelinMar 16, 2016
  51. 17/17 rebase-interactive: introduce interactive backend for builtin rebasePaul Tan, Mar 12, 2016
  52. Johannes SchindelinMar 15, 2016
  53. Paul TanMar 15, 2016
  54. Johannes SchindelinMar 15, 2016
  55. Duy NguyenMar 14, 2016
  56. Stefan BellerMar 14, 2016
  57. Junio C HamanoMar 14, 2016
  58. Paul TanMar 16, 2016
  59. Johannes SchindelinMar 14, 2016

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.